Skip to content

fix(session): persist SafetyPolicy across turns - #3685

Merged
dgageot merged 2 commits into
docker:mainfrom
trungutt:persist-safety-policy
Jul 17, 2026
Merged

dgageot merged 2 commits into
docker:mainfrom
trungutt:persist-safety-policy

Conversation

@trungutt

@trungutt trungutt commented Jul 16, 2026 •

Copy link
Copy Markdown
Contributor

Why

The per-session SafetyPolicy field (added in #3647) is supposed to let a caller opt into safe-auto or safer mid-turn — either by PATCHing the field, or by resuming with approve-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.

Turn 1  RunSession loads session      safety_policy=""
        User resumes with approve-safe  (dispatcher mutates struct in memory)
        rest of turn works              (auto-approves safe tools)
        turn ends                       (mutation was never persisted)

Turn 2  RunSession loads session      safety_policy=""   ← forgot the opt-in
        Asks for approve-safe again

The value falls into a gap between three separate pieces of the persistence path:

  1. The sessions table has no column for safety_policy, so every write drops it and every read returns "".
  2. Even with a column, both UpdateSession snapshot builders (SQLite + in-memory) rebuild the outgoing session field-by-field and neither of them copied safety_policy.
  3. Even with the snapshots fixed, the dispatcher mutates safety_policy mid-turn from handleResume, but nothing calls UpdateSession at that point — the observer only writes on OnRunStart.

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

  • A new migration adds the safety_policy TEXT DEFAULT '' column and threads it through sessionSelectColumns, scanSession, and every INSERT / ON CONFLICT UPDATE in the store.
  • Both UpdateSession snapshot builders now copy SafetyPolicy alongside the neighbouring ToolsApproved — the same symmetric persistence pattern that column already uses.
  • SessionManager.ResumeSession calls UpdateSession synchronously 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.
  • SetSessionSafetyPolicy mirrors the value onto the live runtime session when one is attached, so an explicit PATCH takes effect on the very next tool call instead of only after the next RunSession.

Notes for the reviewer

  • No behaviour change for callers that never set SafetyPolicy — the field still defaults to '' (equivalent to strict).
  • The ResumeSession mutation is intentionally redundant with the dispatcher's own SetSafetyPolicy / SetToolsApproved call. They operate on the same pointer, so the second call is a no-op — the point is to trigger UpdateSession synchronously.

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 docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pkg/server/session_manager.go
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.
@aheritier aheritier added area/sessions For features/issues/fixes related to session lifecycle (resume, persistence, export) kind/fix PR fixes a bug (maps to fix:). Use on PRs only. labels Jul 16, 2026
@trungutt
trungutt requested a review from docker-agent July 16, 2026 19:03

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The implementation correctly addresses all three persistence holes described in the PR:

  1. SQL migration — Migration 24 adds safety_policy TEXT DEFAULT '' to the sessions table. The DEFAULT '' aligns with the Go zero value (SafetyPolicy("") = strict), so pre-existing rows are treated correctly on read.

  2. Snapshot builders — Both InMemorySessionStore.UpdateSession and SQLiteSessionStore.UpdateSession now copy SafetyPolicy in the field-by-field snapshot, making the persistence path symmetric with ToolsApproved.

  3. ResumeSession mid-turn persistence — The new switch block mirrors the dispatcher's mutation onto rt.session and synchronously calls UpdateSession before forwarding the resume signal. The warn-and-continue approach on UpdateSession failure 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.

  4. 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.

  5. Pinned migrations test — wantDigest in migrations_pinned_test.go was correctly updated from the previous value to reflect migration 24 (confirmed via git diff against merge base).

No bugs introduced by this PR.

@trungutt
trungutt marked this pull request as ready for review July 16, 2026 19:20
@trungutt
trungutt requested a review from a team as a code owner July 16, 2026 19:20

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The PR correctly closes all three gaps in the SafetyPolicy persistence round-trip:

  1. Migration — adds and is correctly sequenced after migration 23.
  2. Store layer — , , both INSERT paths, and the upsert are all updated symmetrically. The snapshot builder is also updated.
  3. 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 docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The PR correctly closes all three gaps in the SafetyPolicy persistence round-trip:

  1. Migration — 024_add_safety_policy_column adds safety_policy TEXT DEFAULT '' and is correctly sequenced after migration 23.
  2. Store layer — sessionSelectColumns, scanSession, both AddSession INSERT paths, and the UpdateSession upsert are all updated symmetrically. The InMemorySessionStore snapshot builder is also updated.
  3. Manager layer — ResumeSession now calls UpdateSession synchronously for all four mid-turn resume types before forwarding the resume signal, and SetSessionSafetyPolicy mirrors the policy onto the live runtime session immediately.

No bugs introduced by this PR were found.

@dgageot
dgageot merged commit bf7ca85 into docker:main Jul 17, 2026
13 of 14 checks passed
social4hyq pushed a commit to social4hyq/homebrew-core that referenced this pull request Sep 20, 2026
docker-agent 1.111.0

Created-by: HarmonybrewBot
Commit-by: HarmonybrewBot
Merged-by: HarmonybrewBot
Description: Created by `brew bump`

---

Created with `brew bump-formula-pr`.<details>
  <summary>release notes</summary>
  <pre>This release adds a new scheduler toolset, inline Mermaid diagram rendering, model switching in the lean TUI, project config autodiscovery, and a range of bug fixes and performance improvements across the agent, TUI, and server.

## What's New

- Adds a built-in `scheduler` toolset with `create_schedule`, `list_schedules`, and `cancel_schedule` tools, enabling agents to schedule instructions to run once at a time, after a delay, or on a recurring interval
- Adds prompt cache miss warnings via an opt-in `warn_on_cache_miss` user setting that emits notifications when cached input tokens are absent after the first session response
- Adds inline Mermaid diagram rendering in the TUI
- Adds model switching (`/model`) to the lean TUI and shares fuzzy model search between the lean and full TUIs
- Adds autodiscovery of `docker-agent.yaml`, `docker-agent.yml`, and `docker-agent.hcl` project config files for no-argument local `docker agent run`
- Surfaces background-job elicitation (user-input) requests in the TUI, ensuring each request opens exactly one dialog regardless of which runtime delivers it
- Makes a custom `base_url` on a model automatically imply `bypass_models_gateway: true`, removing the need to set it explicitly

## Improvements

- Improves session cost details in the TUI: aligns layout, color-codes cost percentages per message, groups averages together, and puts "Total" on its own line
- Speeds up model switcher credential discovery by deduplicating credential names and resolving lookups concurrently, reducing model-picker latency
- Parallelizes toolset startup and prefetches the MCP catalog off the load critical path, reducing agent startup time

## Bug Fixes

- Fixes glob wildcard matching in permissions so `*` and `?` span path separators (`/`) in argument values such as file paths and URLs
- Fixes a bug where setting `defer_all: true` on a toolset caused the toolset's custom `Instructions` to be permanently dropped from the agent's context
- Fixes `SafetyPolicy` not persisting across turns, causing users to be re-prompted for tool approvals they had already opted into
- Fixes background elicitations over the API so requests from concurrent background jobs are replayable and answerable instead of being auto-declined
- Fixes Anthropic cache breakpoints exceeding the hard limit of 4 when deferred tools are present, which caused API errors at runtime
- Fixes the TUI editor not regaining focus after external editing (`Ctrl+G`), which caused Enter to route to the transcript instead of sending the composed message
- Fixes the scheduler schema so `type: scheduler` is accepted in agent config files (was previously rejected by JSON schema validation)
- Fixes foreground elicitations being delivered twice into the app event stream
- Fixes a mutex unlock in the scheduler's `setRuntime` method to use `defer` for safer lock release
- Fixes VCR cassette path normalization for portable prompt-file matching
- Fixes CI workflow YAML indentation that caused all CI runs to fail before any job could start

## Technical Changes

- Adds `MapSlice` fan-out helper in the concurrent utilities package and uses it for parallel toolset operations
- Serializes interactive OAuth flows across toolsets to prevent concurrent conflicts
- Brings the documentation portal to WCAG 2.1 AA conformance across light and dark themes and adds a pa11y-ci CI gate to maintain it
- Adds new documentation pages for the scheduler toolset, sandbox templates, custom commands, user settings, context and compaction guide, and fixes stale command syntax in the Named Commands docs
---

## What's Changed
* docs: update CHANGELOG.md for v1.110.0 by @docker-read-write[bot] in docker/docker-agent#3665
* fix: repair CI workflow YAML and failing AgentsMd e2e test on main by @dgageot in docker/docker-agent#3666
* feat(tools): add "scheduler" toolset to run instructions on a time or recurring interval by @dwin-gharibi in docker/docker-agent#3632
* fix(config): add scheduler toolset to agent-schema.json type enum by @aheritier in docker/docker-agent#3670
* feat: custom base_url implies bypass_models_gateway by @dgageot in docker/docker-agent#3667
* feat: add prompt cache miss warnings by @rumpl in docker/docker-agent#3671
* Improve session cost details by @rumpl in docker/docker-agent#3672
* perf: speed up model switcher credential discovery by @rumpl in docker/docker-agent#3673
* Add model switching to the lean TUI by @rumpl in docker/docker-agent#3674
* feat(tui,app): surface & correlate background-job elicitations (#3584) by @aheritier in docker/docker-agent#3624
* chore: bump go-isatty, openai-go, and libopenapi by @dgageot in docker/docker-agent#3675
* ci: bump golangci-lint-action to v9.3.0 and fix DeferMutexUnlock lint offense by @dgageot in docker/docker-agent#3676
* ci: bump pinned actions to latest same-major versions by @dgageot in docker/docker-agent#3677
* feat(server): session-scoped elicitation sink for API/server runtimes (#3584) by @aheritier in docker/docker-agent#3625
* Render Mermaid diagrams inline by @rumpl in docker/docker-agent#3679
* fix(server): surface background elicitations over API by @Sayt-0 in docker/docker-agent#3678
* fix(teamloader): preserve deferred toolset instructions by @Piyush0049 in docker/docker-agent#3680
* fix(permissions): match glob wildcards across path separators by @parveshsaini in docker/docker-agent#3605
* docs(agents): fix stale command syntax and sub_agents claim in Named Commands by @aheritier in docker/docker-agent#3684
* docs(sandbox): document Docker Sandboxes integration and published sbx templates by @aheritier in docker/docker-agent#3682
* docs: surface orphan pages and add compaction, commands, and user-settings docs by @aheritier in docker/docker-agent#3683
* fix(docs): WCAG 2.1 AA accessibility remediation for the docs portal by @aheritier in docker/docker-agent#3687
* fix(tui): refocus the editor after external editing so Enter sends the content by @dgageot in docker/docker-agent#3690
* Implements the path-aware, two-tier documentation accessibility CI gate by @aheritier in docker/docker-agent#3691
* feat(run): autodiscover project agent config by @HajimohammadiNet in docker/docker-agent#3681
* perf: parallelize toolset startup and prefetch MCP catalog by @dgageot in docker/docker-agent#3688
* fix(anthropic): keep cache breakpoints within Anthropic's limit of 4 with deferred tools by @dgageot in docker/docker-agent#3692
* fix(docs): render --link-hover on light-mode callout-link hover by @aheritier in docker/docker-agent#3693
* build(deps): bump the pip group across 1 directory with 2 updates by @dependabot[bot] in docker/docker-agent#3697
* fix(session): persist SafetyPolicy across turns by @trungutt in docker/docker-agent#3685

## New Contributors
* @parveshsaini made their first contribution in docker/docker-agent#3605
* @HajimohammadiNet made their first contribution in docker/docker-agent#3681

**Full Changelog**: docker/docker-agent@v1.110.0...v1.111.0
</pre>
  <p>View the full release notes at <a href="https://github.com/docker/docker-agent/releases/tag/v1.111.0">https://github.com/docker/docker-agent/releases/tag/v1.111.0</a>.</p>
</details>
<hr>

See merge request: Harmonybrew/homebrew-core!14465
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions For features/issues/fixes related to session lifecycle (resume, persistence, export) kind/fix PR fixes a bug (maps to fix:). Use on PRs only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants