fix(session): persist SafetyPolicy across turns#3685
Conversation
The Session struct grew a SafetyPolicy field, and the API + dispatcher learned to read and write it, but none of the store code carried it through. As a result: - The SQLite schema had no column for it, so every UpdateSession silently dropped the value on write and every scanSession returned the empty string on read. - The InMemory and SQLite UpdateSession snapshot builders copy fields explicitly for lock-safety; both were missing SafetyPolicy, so even once the column existed the value would still be dropped. - SessionManager.ResumeSession forwarded approve-safe / approve-safer / approve-session to the runtime and returned. The dispatcher mutates the in-memory session pointer to record the new policy, but PersistenceObserver only calls UpdateSession on OnRunStart. Nothing else re-persisted the row before the next turn. The net effect: a caller could opt into safe-auto (or any of the other mid-turn policy transitions), the runtime would honor it for the rest of the current turn, and then the next turn would reload the session from storage and see an empty SafetyPolicy, prompting again for everything the opt-in was supposed to skip. Fix: - Add a migration for a safety_policy column and thread it through scanSession, sessionSelectColumns, and every INSERT / ON CONFLICT UPDATE that persists a session row. - Include SafetyPolicy in both UpdateSession snapshot builders. - In SessionManager.ResumeSession, apply the same mutation the dispatcher will apply and persist synchronously before forwarding. - In SetSessionSafetyPolicy, mirror onto the live runtime session when one is attached so the current turn also picks up the change.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR correctly fixes the three identified persistence holes for SafetyPolicy: the new SQL migration, both snapshot builders, and the synchronous pre-persist in ResumeSession and SetSessionSafetyPolicy. One medium-confidence finding: the new mid-turn persistence switch omits ResumeTypeApproveTool, leaving per-tool approvals with the same durability gap the PR fixes for the other resume types.
The Persist-on-resume switch symmetry: approve-safe / approve-safer / approve-session mutate session state and are now persisted here. approve-tool mutates session.Permissions.Allow with the same lifetime characteristics, and needs the same treatment or its in-turn grant is silently dropped by the next OnRunStart cycle (or a mid-turn process restart). Skip the persist when toolName is empty — the dispatcher's fallback to the pending tool call's name isn't reachable from here, so the mutation happens in-runtime as before but doesn't get the durable write.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The implementation correctly addresses all three persistence holes described in the PR:
-
SQL migration — Migration 24 adds
safety_policy TEXT DEFAULT ''to the sessions table. TheDEFAULT ''aligns with the Go zero value (SafetyPolicy("") = strict), so pre-existing rows are treated correctly on read. -
Snapshot builders — Both
InMemorySessionStore.UpdateSessionandSQLiteSessionStore.UpdateSessionnow copySafetyPolicyin the field-by-field snapshot, making the persistence path symmetric withToolsApproved. -
ResumeSession mid-turn persistence — The new
switchblock mirrors the dispatcher's mutation ontort.sessionand synchronously callsUpdateSessionbefore forwarding the resume signal. The warn-and-continue approach onUpdateSessionfailure is a deliberate, defensible design choice: returning an error here would silently drop the user's approval and leave the session hanging, which is a worse outcome than a transient DB write failure. -
SetSessionSafetyPolicy live mirroring — The new early-return path correctly updates the live runtime session pointer under
sm.mux, ensuring an explicit PATCH takes effect on the next tool call. -
Pinned migrations test —
wantDigestinmigrations_pinned_test.gowas correctly updated from the previous value to reflect migration 24 (confirmed via git diff against merge base).
No bugs introduced by this PR.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR correctly closes all three gaps in the SafetyPolicy persistence round-trip:
- Migration — adds and is correctly sequenced after migration 23.
- Store layer — , , both INSERT paths, and the upsert are all updated symmetrically. The snapshot builder is also updated.
- Manager layer — now calls synchronously for all four mid-turn resume types before forwarding the resume signal, and mirrors the policy onto the live runtime session immediately.
No bugs introduced by this PR were found.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR correctly closes all three gaps in the SafetyPolicy persistence round-trip:
- Migration —
024_add_safety_policy_columnaddssafety_policy TEXT DEFAULT ''and is correctly sequenced after migration 23. - Store layer —
sessionSelectColumns,scanSession, bothAddSessionINSERT paths, and theUpdateSessionupsert are all updated symmetrically. TheInMemorySessionStoresnapshot builder is also updated. - Manager layer —
ResumeSessionnow callsUpdateSessionsynchronously for all four mid-turn resume types before forwarding the resume signal, andSetSessionSafetyPolicymirrors the policy onto the live runtime session immediately.
No bugs introduced by this PR were found.
Why
The per-session
SafetyPolicyfield (added in #3647) is supposed to let a caller opt intosafe-autoorsafermid-turn — either byPATCHing the field, or by resuming withapprove-safe/approve-safer/approve-session— and have that choice survive to the next turn.Today it doesn't. The choice is remembered in-memory for the rest of the current turn, then quietly forgotten when the next turn starts. The user gets prompted again for the exact tool calls their opt-in was meant to auto-approve.
The value falls into a gap between three separate pieces of the persistence path:
safety_policy, so every write drops it and every read returns"".UpdateSessionsnapshot builders (SQLite + in-memory) rebuild the outgoing session field-by-field and neither of them copiedsafety_policy.safety_policymid-turn fromhandleResume, but nothing callsUpdateSessionat that point — the observer only writes onOnRunStart.Each of the three gaps has to close for the round-trip to actually work; fixing any subset in isolation still loses the value.
What this PR changes
safety_policy TEXT DEFAULT ''column and threads it throughsessionSelectColumns,scanSession, and every INSERT / ON CONFLICT UPDATE in the store.UpdateSessionsnapshot builders now copySafetyPolicyalongside the neighbouringToolsApproved— the same symmetric persistence pattern that column already uses.SessionManager.ResumeSessioncallsUpdateSessionsynchronously for the three mid-turn transitions (approve-safe,approve-safer,approve-session) before forwarding the resume signal, so the mutation is durable before the dispatcher goroutine gets a chance to run.SetSessionSafetyPolicymirrors the value onto the live runtime session when one is attached, so an explicitPATCHtakes effect on the very next tool call instead of only after the nextRunSession.Notes for the reviewer
SafetyPolicy— the field still defaults to''(equivalent tostrict).ResumeSessionmutation is intentionally redundant with the dispatcher's ownSetSafetyPolicy/SetToolsApprovedcall. They operate on the same pointer, so the second call is a no-op — the point is to triggerUpdateSessionsynchronously.