From e6cdfd618755d17ad534e7538ebbaeb8bd553d4e Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 23 Jul 2026 19:57:57 +0000 Subject: [PATCH] fix(session): cleared session-scoped tool state Cleared queued tool-choice directives and ACP always decisions after successful logical session transitions. Added new-session and cross-session regression coverage for staged resolves and both ACP always decisions. Fixes #4093 --- packages/coding-agent/CHANGELOG.md | 2 + .../coding-agent/src/session/agent-session.ts | 10 +++ .../coding-agent/src/session/session-tools.ts | 5 ++ .../test/agent-session-acp-permission.test.ts | 69 ++++++++++++++++++- .../agent-session-resolve-reminder.test.ts | 38 +++++++++- 5 files changed, 121 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e9a4b4d43..fd7873d45 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -45,6 +45,8 @@ - Fixed bash internal-URL expansion skipping unquoted `skill://` (and other supported schemes) inside a legacy backtick command substitution nested directly in double quotes (e.g. ``echo "`cat skill://valid-skill/SKILL.md`"``); `isInsideShellQuote` now treats `` ` `` as an expansion-context boundary like `$()`, including `$()`/backtick nesting in either order, while single-quoted and escaped-backtick text stay literal ([#5645](https://github.com/can1357/oh-my-pi/issues/5645)). - Fixed `omp say` playing no audio for a short single-segment clip on hosts where the first streaming backend (the bundled ffmpeg built without pulse/alsa output) spawns then exits nonzero: the pipe write succeeds before that death and `player.end()` has already closed the input, so neither the broken-pipe replay nor the early-exit handler advanced to `paplay`/`aplay`. `StreamingAudioPlayer` now retains the utterance PCM and, when the streaming backend exits nonzero, replays it through per-file playback so short clips still reach the speakers ([#5875](https://github.com/can1357/oh-my-pi/issues/5875)). - Fixed mid-session `memory.backend` changes leaving runtime state, tools, listeners, and prompt context on different backends; Mnemopi clear/enqueue now rehydrate listeners, and legacy `memories.enabled` no longer activates the local pipeline after migration ([#5638](https://github.com/can1357/oh-my-pi/issues/5638)). +- Fixed `/new` and cross-session switches retaining staged preview resolve callbacks and per-session ACP `allow_always`/`reject_always` decisions from the previous session ([#4093](https://github.com/can1357/oh-my-pi/issues/4093)). + - Fixed `error.notify` raising a "Stopped with error" toast for provider failures while an auto-retry or async-delivery continuation was pending; the toast now waits for the true terminal settle. - Fixed concurrent MCP config mutations losing updates and racing on a shared temp path: every `mcp.json` read-modify-write (add/update/remove server, disabled/force-enabled lists) is now serialized under a per-file lock, and each atomic write uses a unique temp file so overlapping writers no longer rename each other's `.tmp` out from under them (ENOENT or clobbered config) — reachable in-process via the fire-and-forget extensions-dashboard toggle and across processes on a shared `~/.omp/mcp.json` ([#4104](https://github.com/can1357/oh-my-pi/issues/4104)). - Fixed transient provider stream stalls after tool calls failing to auto-retry even when every call already had a tool result, including synthetic `executed:false` results from OpenAI-completions stalls ([#6414](https://github.com/can1357/oh-my-pi/issues/6414)). diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index f72d5d7b4..0020f8ea7 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -4023,6 +4023,12 @@ export class AgentSession { this.#rewoundToolResultIds.clear(); } + /** Drop mutable tool decisions and directives owned by the previous logical session. */ + #clearSessionScopedToolState(): void { + this.#toolChoiceQueue.clear(); + this.#tools.clearAcpPermissionDecisions(); + } + /** * Rebuild checkpoint/rewind runtime state from the current branch. Handles two * cases surfaced by session resume, `switchSession()` reloading the same file, @@ -5690,6 +5696,7 @@ export class AgentSession { this.#bash.finishSessionTransition(bashTransition, sessionTransitioned); } + this.#clearSessionScopedToolState(); this.#clearCheckpointRuntimeState(); this.setTodoPhases([]); this.#freshProviderSessionId = undefined; @@ -6810,6 +6817,9 @@ export class AgentSession { if (switchingToDifferentSession) { await this.#memory.resetContextForNewTranscript(); } + if (switchingToDifferentSession) { + this.#clearSessionScopedToolState(); + } this.#reconnectToAgent(); try { await this.#sessionSwitchReconciler?.(); diff --git a/packages/coding-agent/src/session/session-tools.ts b/packages/coding-agent/src/session/session-tools.ts index fab146740..ec69324cd 100644 --- a/packages/coding-agent/src/session/session-tools.ts +++ b/packages/coding-agent/src/session/session-tools.ts @@ -158,6 +158,11 @@ export class SessionTools { return this.#skillsSettings; } + /** Drops cached per-session ACP `allow_always`/`reject_always` decisions. */ + clearAcpPermissionDecisions(): void { + this.#acpPermissionDecisions.clear(); + } + /** Re-wraps active and mounted tools after the ACP client changes. */ refreshAcpPermissionGates(): void { this.#acpPermissionDecisions.clear(); diff --git a/packages/coding-agent/test/agent-session-acp-permission.test.ts b/packages/coding-agent/test/agent-session-acp-permission.test.ts index 2b5b25bd8..ad031bd9d 100644 --- a/packages/coding-agent/test/agent-session-acp-permission.test.ts +++ b/packages/coding-agent/test/agent-session-acp-permission.test.ts @@ -32,6 +32,12 @@ import { type } from "arktype"; let tempDir: TempDir; let session: AgentSession | undefined; +const boundaryCases: Array<[decision: "allow_always" | "reject_always", transition: "new" | "switch"]> = [ + ["allow_always", "new"], + ["allow_always", "switch"], + ["reject_always", "new"], + ["reject_always", "switch"], +]; /** Fake tool that records execute calls. */ function makeFakeTool(name: string): AgentTool & { executeCalls: number } { const tool = { @@ -77,13 +83,20 @@ async function createSession( tools: AgentTool[], bridge?: ClientBridge, settingsOverrides: Partial> = {}, - options?: { xdevRegistry?: XdevRegistry; builtInToolNames?: string[]; initialMountedXdevToolNames?: string[] }, + options?: { + xdevRegistry?: XdevRegistry; + builtInToolNames?: string[]; + initialMountedXdevToolNames?: string[]; + persist?: boolean; + }, ): Promise { const model = getBundledModel("anthropic", "claude-sonnet-4-5"); if (!model) throw new Error("Expected claude-sonnet-4-5 model to exist"); const settings = Settings.isolated({ "compaction.enabled": false, ...settingsOverrides }); - const sessionManager = SessionManager.inMemory(tempDir.path()); + const sessionManager = options?.persist + ? SessionManager.create(tempDir.path(), `${tempDir.path()}/sessions`) + : SessionManager.inMemory(tempDir.path()); const agent = new Agent({ getApiKey: () => "test-key", @@ -838,6 +851,58 @@ it("allow_always: caches decision and calls bridge only once for subsequent exec expect(bashTool.executeCalls).toBe(2); }); +it.each(boundaryCases)( + "%s permission decisions prompt again after a successful %s session boundary", + async (decision, transition) => { + const bashTool = makeFakeTool("bash"); + const bridge = makeBridge({ outcome: "selected", optionId: decision, kind: decision }); + const permissionSpy = spyOn(bridge, "requestPermission"); + session = await createSession([bashTool], bridge, {}, { persist: true }); + + await session.setActiveToolsByName(["bash"]); + const wrappedBash = session.agent.state.tools.find(tool => tool.name === "bash"); + if (!wrappedBash) throw new Error("Expected wrapped bash tool"); + + for (let callIndex = 0; callIndex < 2; callIndex++) { + if (callIndex === 1) { + if (transition === "new") { + expect(await session.newSession()).toBe(true); + } else { + const targetId = `permission-target-${Bun.nanoseconds()}`; + const targetPath = `${tempDir.path()}/${targetId}.jsonl`; + await Bun.write( + targetPath, + `${JSON.stringify({ + type: "session", + version: 3, + id: targetId, + timestamp: new Date().toISOString(), + cwd: tempDir.path(), + })}\n`, + ); + expect(await session.switchSession(targetPath)).toBe(true); + } + } + + const execution = wrappedBash.execute( + `call-${callIndex}`, + { command: "echo boundary" }, + undefined, + undefined as never, + undefined as never, + ); + if (decision === "reject_always") { + await expect(execution).rejects.toThrow(/rejected by user/); + } else { + await execution; + } + } + + expect(permissionSpy).toHaveBeenCalledTimes(2); + expect(bashTool.executeCalls).toBe(decision === "allow_always" ? 2 : 0); + }, +); + // --------------------------------------------------------------------------- // 4. Read tool not gated: bridge never called even when bridge is set // --------------------------------------------------------------------------- diff --git a/packages/coding-agent/test/agent-session-resolve-reminder.test.ts b/packages/coding-agent/test/agent-session-resolve-reminder.test.ts index efd2a000b..b5d933037 100644 --- a/packages/coding-agent/test/agent-session-resolve-reminder.test.ts +++ b/packages/coding-agent/test/agent-session-resolve-reminder.test.ts @@ -22,6 +22,7 @@ describe("AgentSession resolve reminder", () => { let mock: MockModel; let authStorage: AuthStorage | undefined; + const transitions: Array<"new" | "switch"> = ["new", "switch"]; beforeEach(async () => { tempDir = path.join(os.tmpdir(), `pi-resolve-reminder-test-${Snowflake.next()}`); fs.mkdirSync(tempDir, { recursive: true }); @@ -54,7 +55,7 @@ describe("AgentSession resolve reminder", () => { session = new AgentSession({ agent, - sessionManager: SessionManager.inMemory(), + sessionManager: SessionManager.create(tempDir, path.join(tempDir, "sessions")), settings: Settings.isolated(), modelRegistry, }); @@ -69,6 +70,26 @@ describe("AgentSession resolve reminder", () => { } }); + async function changeLogicalSession(transition: "new" | "switch"): Promise { + if (transition === "new") { + expect(await session.newSession()).toBe(true); + return; + } + const targetId = `target-${Snowflake.next()}`; + const targetPath = path.join(tempDir, `${targetId}.jsonl`); + await Bun.write( + targetPath, + `${JSON.stringify({ + type: "session", + version: 3, + id: targetId, + timestamp: new Date().toISOString(), + cwd: tempDir, + })}\n`, + ); + expect(await session.switchSession(targetPath)).toBe(true); + } + it("delivers the resolve reminder via a non-forcing soft requirement, not a steer or a forced tool_choice", () => { queueResolveHandler(toolSession, { label: "AST Edit: 1 replacement in 1 file", @@ -93,6 +114,21 @@ describe("AgentSession resolve reminder", () => { expect(session.agent.peekSteeringQueue()).toHaveLength(0); }); + it.each(transitions)("clears a staged preview after a successful %s session boundary", async transition => { + queueResolveHandler(toolSession, { + label: "AST Edit: 1 replacement in 1 file", + sourceToolName: "ast_edit", + apply: async () => ({ content: [{ type: "text", text: "Applied" }] }), + }); + expect(session.peekPendingInvoker()).toBeDefined(); + expect(isSoftToolRequirement(session.nextToolChoiceDirective())).toBe(true); + + await changeLogicalSession(transition); + + expect(session.peekPendingInvoker()).toBeUndefined(); + expect(session.nextToolChoiceDirective()).toBeUndefined(); + }); + it("dispatches a staged preview through the production toolSession wiring and drains the gate", async () => { let applyRuns = 0; queueResolveHandler(toolSession, {