From 206a96d4179c078ce3b6ec849237d7d7e7e6ea2f Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Fri, 24 Jul 2026 21:17:36 -0700 Subject: [PATCH 1/3] fix(agent): honor a cancel that lands while a turn is being set up A cancel arriving after `prompt()` was called but before the prompt reached the SDK was silently dropped. Setup can take seconds: `ensureLocalToolsConnected` ("pre-prompt") awaits an MCP status RPC bounded at 5s, and an unknown slash command reloads skills first. Incoming messages are dispatched concurrently, so a cancel in that window found an empty turn queue and no active turn. `interrupt()` had nothing to settle and armed no force-cancel backstop, and setup went on to hand the message to the SDK, which ran the turn to completion and reported `end_turn` while tools kept executing. `session.cancelSeq` counts cancels. `prompt()` snapshots it on entry, before any await, and re-checks it once setup finishes: a higher count means the cancel targets this prompt, so it returns `cancelled` without pushing to the SDK. That is the last point where the turn can still be stopped, and no await separates the check from the push. `session.cancelled` is left standing, because an earlier turn may still be settling against it; activation clears it for the next turn that runs. Cancels landing outside that window are unchanged. Once a turn is queued `interrupt()` sweeps it, and once it is active the existing backstop applies. `cancel()` now assigns `interruptReason` on every call, including when the client supplies none, so it always describes the current cancel instead of reporting a stale reason left by an earlier one. Raise the package's `engines.node` to match the root's Node 22 requirement, which the new test's `Promise.withResolvers` depends on. Claude-Session: https://claude.ai/code/session_01KT3ZQ47bqT9mW4fi458Q6V --- products/desktop/packages/agent/package.json | 2 +- .../agent/src/adapters/base-acp-agent.ts | 15 +- .../claude-agent.cancel-during-setup.test.ts | 230 ++++++++++++++++++ .../claude-agent.permission-mode.test.ts | 1 + .../claude/claude-agent.refresh.test.ts | 1 + .../claude/claude-agent.slash-command.test.ts | 1 + .../claude/claude-agent.streamed-text.test.ts | 1 + .../claude-agent.task-notification.test.ts | 1 + .../agent/src/adapters/claude/claude-agent.ts | 16 ++ .../codex-app-server-agent.ts | 1 + 10 files changed, 265 insertions(+), 4 deletions(-) create mode 100644 products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts diff --git a/products/desktop/packages/agent/package.json b/products/desktop/packages/agent/package.json index e159b71d0e2d..012826a2b553 100644 --- a/products/desktop/packages/agent/package.json +++ b/products/desktop/packages/agent/package.json @@ -147,7 +147,7 @@ "clean": "node ../../scripts/rimraf.mjs dist .turbo" }, "engines": { - "node": ">=20.0.0" + "node": ">=22.19.0" }, "devDependencies": { "@posthog/shared": "workspace:*", diff --git a/products/desktop/packages/agent/src/adapters/base-acp-agent.ts b/products/desktop/packages/agent/src/adapters/base-acp-agent.ts index eddfcf2db710..f8021a62b3a3 100644 --- a/products/desktop/packages/agent/src/adapters/base-acp-agent.ts +++ b/products/desktop/packages/agent/src/adapters/base-acp-agent.ts @@ -47,6 +47,13 @@ export interface BaseSettingsManager { export interface BaseSession { notificationHistory: SessionNotification[]; cancelled: boolean; + /** + * Bumped on every cancel. An adapter whose `prompt()` awaits before registering + * the turn must snapshot this on entry and re-check it before handing the prompt + * to the backend: a cancel landing in that window reaches no turn, and `cancelled` + * alone cannot distinguish it from a stale flag left by an earlier cancel. + */ + cancelSeq: number; interruptReason?: string; abortController: AbortController; settingsManager: BaseSettingsManager; @@ -78,10 +85,12 @@ export abstract class BaseAcpAgent implements Agent { throw new Error("Session ID mismatch"); } this.session.cancelled = true; + this.session.cancelSeq += 1; const meta = params._meta as { interruptReason?: string } | undefined; - if (meta?.interruptReason) { - this.session.interruptReason = meta.interruptReason; - } + // Assign even when absent, so the reason always describes the current cancel. + // A turn cancelled during setup keeps this value through activation, so a + // leftover reason would otherwise be reported for a cancel that supplied none. + this.session.interruptReason = meta?.interruptReason; await this.interrupt(); } diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts new file mode 100644 index 000000000000..7cdb06cea6e1 --- /dev/null +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts @@ -0,0 +1,230 @@ +import type { AgentSideConnection } from "@agentclientprotocol/sdk"; +import type { SDKUserMessage } from "@anthropic-ai/claude-agent-sdk"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { + createMockQuery, + createSuccessResult, + type MockQuery, +} from "../../test/mocks/claude-sdk"; +import { Pushable } from "../../utils/streams"; + +vi.mock("@anthropic-ai/claude-agent-sdk", () => ({ + query: vi.fn(), +})); + +vi.mock("./mcp/tool-metadata", () => ({ + fetchMcpToolMetadata: vi.fn().mockResolvedValue(undefined), + getConnectedMcpServerNames: vi.fn().mockReturnValue([]), + getCachedMcpTools: vi.fn().mockReturnValue([]), + clearMcpToolMetadataCache: vi.fn(), + setMcpToolApprovalStates: vi.fn(), + isMcpToolReadOnly: vi.fn().mockReturnValue(false), + getMcpToolMetadata: vi.fn().mockReturnValue(undefined), + getMcpToolApprovalState: vi.fn().mockReturnValue(undefined), +})); + +const { ClaudeAcpAgent } = await import("./claude-agent"); +type Agent = InstanceType; + +const SESSION_ID = "s-cancel"; + +/** Lets the consumer loop re-park in `query.next()` between messages. */ +function tick(): Promise { + return new Promise((resolve) => setImmediate(resolve)); +} + +interface Harness { + agent: Agent; + session: { cancelled: boolean; cancelSeq: number }; + /** Resolves once prompt() has entered the pre-prompt MCP status await. */ + inSetup: Promise; + /** Lets the stalled status check return, so setup continues. */ + finishSetup: () => void; + /** Drives the queued turn to a normal completion. */ + completeTurn: () => Promise; + /** Whether anything was handed to the SDK. */ + sdkReceivedMessage: () => boolean; + prompt: () => Promise<{ stopReason: string; _meta?: unknown }>; +} + +/** + * A session whose pre-prompt `ensureLocalToolsConnected` can be held open, which + * is the window a Ctrl-C lands in before the prompt reaches the SDK. + * + * `query.interrupt` is a deferred no-op on purpose. In production it asks the + * Claude subprocess to stop and the message stream keeps running until the + * subprocess acknowledges; the default mock ends the stream synchronously, so the + * consumer takes the stream-`done` branch and rejects the still-queued turn as + * session-ended, never reaching `activateTurn`. + */ +function makeHarness(): Harness { + const client = { + sessionUpdate: vi.fn().mockResolvedValue(undefined), + extNotification: vi.fn().mockResolvedValue(undefined), + }; + const agent = new ClaudeAcpAgent(client as unknown as AgentSideConnection); + + const query = createMockQuery(); + query.interrupt = vi.fn(async () => {}); + const input = new Pushable(); + const pushToSdk = vi.spyOn(input, "push"); + const abortController = new AbortController(); + + const session = { + query, + queryOptions: { sessionId: SESSION_ID, cwd: "/tmp/repo", abortController }, + // Non-empty, so ensureLocalToolsConnected does not short-circuit. + localToolsServerNames: ["posthog-code-tools"], + buildInProcessMcpServers: () => ({ + "posthog-code-tools": { + type: "sdk", + name: "posthog-code-tools", + instance: {}, + }, + }), + input, + cancelled: false, + cancelSeq: 0, + interruptReason: undefined as string | undefined, + settingsManager: { dispose: vi.fn(), getRepoRoot: () => "/tmp/repo" }, + permissionMode: "auto" as const, + abortController, + accumulatedUsage: { + inputTokens: 0, + outputTokens: 0, + cachedReadTokens: 0, + cachedWriteTokens: 0, + }, + sessionResources: new Set(), + configOptions: [], + turnQueue: [], + activeTurn: null, + pendingOrphanResults: 0, + queryGeneration: 0, + cwd: "/tmp/repo", + notificationHistory: [] as unknown[], + taskRunId: "run-1", + lastContextWindowSize: 200_000, + modelId: "claude-sonnet-4-6", + taskState: new Map(), + }; + (agent as unknown as { session: typeof session }).session = session; + (agent as unknown as { sessionId: string }).sessionId = SESSION_ID; + + const { promise: inSetup, resolve: signalInSetup } = + Promise.withResolvers(); + const { promise: held, resolve: releaseSetup } = Promise.withResolvers<[]>(); + query.mcpServerStatus = vi.fn(() => { + signalInSetup(); + return held; + }) as unknown as MockQuery["mcpServerStatus"]; + + return { + agent, + session, + inSetup, + finishSetup: () => releaseSetup([]), + prompt: () => + agent.prompt({ + sessionId: SESSION_ID, + prompt: [{ type: "text", text: "do the thing" }], + }) as Promise<{ stopReason: string; _meta?: unknown }>, + // Echo the turn's own user message back, then send the terminal result, the + // way the SDK would for a turn that ran to completion. + completeTurn: async () => { + const { value: pushed } = await input[Symbol.asyncIterator]().next(); + query._mockHelpers.sendMessage(pushed as never); + await tick(); + query._mockHelpers.complete(createSuccessResult()); + }, + sdkReceivedMessage: () => pushToSdk.mock.calls.length > 0, + }; +} + +/** + * `cancelSeq` lets `prompt()` tell a cancel aimed at the prompt it is setting up + * from a stale one that already stopped an earlier turn. The first drops the + * prompt before it reaches the SDK; the second leaves it alone. + */ +describe("cancel arriving while a prompt is being set up", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("drops the prompt instead of handing it to the SDK", async () => { + const h = makeHarness(); + + const pending = h.prompt(); + await h.inSetup; + // Ctrl-C lands here: nothing is queued yet, so cancel() finds no turn to stop + // and arms no backstop. Only the early return in prompt() can honor it. + await h.agent.cancel({ sessionId: SESSION_ID }); + h.finishSetup(); + + await expect(pending).resolves.toMatchObject({ stopReason: "cancelled" }); + expect(h.sdkReceivedMessage()).toBe(false); + expect(h.session.cancelled).toBe(true); + }); + + it("carries the interrupt reason the cancel supplied", async () => { + const h = makeHarness(); + + const pending = h.prompt(); + await h.inSetup; + await h.agent.cancel({ + sessionId: SESSION_ID, + _meta: { interruptReason: "user_stopped" }, + }); + h.finishSetup(); + + await expect(pending).resolves.toMatchObject({ + stopReason: "cancelled", + _meta: { interruptReason: "user_stopped" }, + }); + }); + + it("runs the next prompt on the same session normally", async () => { + const h = makeHarness(); + + const cancelled = h.prompt(); + await h.inSetup; + await h.agent.cancel({ sessionId: SESSION_ID }); + h.finishSetup(); + await expect(cancelled).resolves.toMatchObject({ stopReason: "cancelled" }); + // The flag is left standing so a still-settling earlier turn can read it. + expect(h.session.cancelled).toBe(true); + + const pending = h.prompt(); + await h.completeTurn(); + + await expect(pending).resolves.toMatchObject({ stopReason: "end_turn" }); + expect(h.session.cancelled).toBe(false); + }); + + it("runs a prompt normally when the cancel predates it", async () => { + const h = makeHarness(); + + // Cancel with nothing in flight, which is how a cancel for an + // already-finished turn looks by the time the next prompt arrives. + await h.agent.cancel({ sessionId: SESSION_ID }); + expect(h.session.cancelled).toBe(true); + + const pending = h.prompt(); + await h.inSetup; + h.finishSetup(); + await h.completeTurn(); + + await expect(pending).resolves.toMatchObject({ stopReason: "end_turn" }); + expect(h.session.cancelled).toBe(false); + }); + + it("leaves a mismatched cancel uncounted", async () => { + const h = makeHarness(); + + await expect(h.agent.cancel({ sessionId: "other" })).rejects.toThrow( + /Session ID mismatch/, + ); + expect(h.session.cancelSeq).toBe(0); + expect(h.session.cancelled).toBe(false); + }); +}); diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.permission-mode.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.permission-mode.test.ts index adfc6415f962..81be4e8d8fa0 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.permission-mode.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.permission-mode.test.ts @@ -54,6 +54,7 @@ function installFakeSession( localToolsServerNames: [] as string[], input, cancelled: false, + cancelSeq: 0, interruptReason: undefined, settingsManager: { dispose: vi.fn(), getRepoRoot: () => "/tmp/repo" }, permissionMode, diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts index 4222b68dd5b4..5524493dcf18 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts @@ -117,6 +117,7 @@ function installFakeSession( localToolsServerNames: ["posthog-code-tools"], input, cancelled: false, + cancelSeq: 0, settingsManager: { dispose: vi.fn() }, permissionMode: "default", abortController, diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts index 0e9f422a089f..2239cb9f455a 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts @@ -50,6 +50,7 @@ function installFakeSession( localToolsServerNames: [] as string[], input, cancelled: false, + cancelSeq: 0, interruptReason: undefined, settingsManager: { dispose: vi.fn(), getRepoRoot: () => "/tmp/repo" }, permissionMode: "default" as const, diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts index de9903954225..c650ae9ed90f 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.streamed-text.test.ts @@ -54,6 +54,7 @@ function installFakeSession( localToolsServerNames: [] as string[], input, cancelled: false, + cancelSeq: 0, interruptReason: undefined, settingsManager: { dispose: vi.fn(), getRepoRoot: () => "/tmp/repo" }, permissionMode: "default" as const, diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.task-notification.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.task-notification.test.ts index 5151e8dae111..35f8fdd85db9 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.task-notification.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.task-notification.test.ts @@ -54,6 +54,7 @@ function installFakeSession( localToolsServerNames: [] as string[], input, cancelled: false, + cancelSeq: 0, interruptReason: undefined, settingsManager: { dispose: vi.fn(), getRepoRoot: () => "/tmp/repo" }, permissionMode: "default" as const, diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts index e07a2c404fe4..4c0c58c8b97d 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts @@ -455,6 +455,9 @@ export class ClaudeAcpAgent extends BaseAcpAgent { } async prompt(params: PromptRequest): Promise { + // Read before any await: setup below can take seconds (the pre-prompt MCP + // reconnect), and a cancel landing in that window belongs to this prompt. + const cancelSeqAtEntry = this.session.cancelSeq; const userMessage = promptToClaude(params); const promptUuid = randomUUID(); userMessage.uuid = promptUuid; @@ -520,6 +523,15 @@ export class ClaudeAcpAgent extends BaseAcpAgent { }); } + // A cancel counted during setup targets this prompt. It is not queued yet, so + // `interrupt()` had nothing to stop and armed no backstop; returning here, + // before the message reaches the SDK, is what actually cancels it. Leave + // `cancelled` standing: an earlier turn may still be settling against it, and + // `activateTurn` clears it for the next turn that runs. + if (this.session.cancelSeq > cancelSeqAtEntry && this.session.cancelled) { + return this.cancelledResponse(); + } + const turn: Turn = { promptUuid, pendingSteerUuids: new Set(), @@ -696,6 +708,9 @@ export class ClaudeAcpAgent extends BaseAcpAgent { const activateTurn = async (turn: Turn) => { session.activeTurn = turn; + // Any cancel aimed at this turn already settled it: during setup `prompt()` + // returns early, and once queued `interrupt()` sweeps it. Reaching here means + // the flag is left over from an earlier turn. session.cancelled = false; session.interruptReason = undefined; session.pendingOrphanResults = 0; @@ -2111,6 +2126,7 @@ export class ClaudeAcpAgent extends BaseAcpAgent { localToolsServerNames, input, cancelled: false, + cancelSeq: 0, settingsManager, permissionMode, cloudMode: cloudRun, diff --git a/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts b/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts index 25e54abe5b46..ed3a01603416 100644 --- a/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts +++ b/products/desktop/packages/agent/src/adapters/codex-app-server/codex-app-server-agent.ts @@ -342,6 +342,7 @@ export class CodexAppServerAgent extends BaseAcpAgent { ), notificationHistory: [], cancelled: false, + cancelSeq: 0, }; } From 91fc880782234d471a58a3d5eaf6e74ea7170e53 Mon Sep 17 00:00:00 2001 From: Phil Haack Date: Fri, 24 Jul 2026 22:14:19 -0700 Subject: [PATCH 2/3] fix(agent): decide the setup-window cancel on the counter alone The bail also required `session.cancelled`, which any turn's activation clears. A local-only command skips the pre-prompt status check the stalled prompt is parked in, so it can queue and activate in that window; the reset erased the very cancel the check exists to catch, and the cancelled prompt went to the SDK anyway. `cancelSeq` is monotonic and incremented only in `cancel()`, in the same synchronous block that sets `cancelled`, so comparing it against the entry snapshot survives an unrelated activation. The flag stays untouched here, because an earlier turn may still be settling against it. Claude-Session: https://claude.ai/code/session_01KT3ZQ47bqT9mW4fi458Q6V --- .../claude-agent.cancel-during-setup.test.ts | 62 +++++++++++++++++-- .../agent/src/adapters/claude/claude-agent.ts | 11 ++-- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts index 7cdb06cea6e1..41bc641c3f2a 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts @@ -1,5 +1,8 @@ import type { AgentSideConnection } from "@agentclientprotocol/sdk"; -import type { SDKUserMessage } from "@anthropic-ai/claude-agent-sdk"; +import type { + SDKMessage, + SDKUserMessage, +} from "@anthropic-ai/claude-agent-sdk"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { createMockQuery, @@ -42,9 +45,15 @@ interface Harness { finishSetup: () => void; /** Drives the queued turn to a normal completion. */ completeTurn: () => Promise; + /** Promotes the queued turn the way a local-only command's output does. */ + activateQueuedTurn: () => Promise; + /** Ends the active turn with the SDK's terminal result. */ + finishTurn: () => void; /** Whether anything was handed to the SDK. */ sdkReceivedMessage: () => boolean; - prompt: () => Promise<{ stopReason: string; _meta?: unknown }>; + /** The text of every message handed to the SDK, in order. */ + sdkPromptTexts: () => string[]; + prompt: (text?: string) => Promise<{ stopReason: string; _meta?: unknown }>; } /** @@ -124,11 +133,22 @@ function makeHarness(): Harness { session, inSetup, finishSetup: () => releaseSetup([]), - prompt: () => + prompt: (text = "do the thing") => agent.prompt({ sessionId: SESSION_ID, - prompt: [{ type: "text", text: "do the thing" }], + prompt: [{ type: "text", text }], }) as Promise<{ stopReason: string; _meta?: unknown }>, + activateQueuedTurn: async () => { + query._mockHelpers.sendMessage({ + type: "system", + subtype: "local_command_output", + content: "context report", + uuid: crypto.randomUUID(), + session_id: SESSION_ID, + } as SDKMessage); + await tick(); + }, + finishTurn: () => query._mockHelpers.complete(createSuccessResult()), // Echo the turn's own user message back, then send the terminal result, the // way the SDK would for a turn that ran to completion. completeTurn: async () => { @@ -138,6 +158,16 @@ function makeHarness(): Harness { query._mockHelpers.complete(createSuccessResult()); }, sdkReceivedMessage: () => pushToSdk.mock.calls.length > 0, + sdkPromptTexts: () => + pushToSdk.mock.calls.map(([message]) => { + const content = message.message.content; + if (typeof content === "string") { + return content; + } + return content + .map((block) => (block.type === "text" ? block.text : "")) + .join(""); + }), }; } @@ -218,6 +248,30 @@ describe("cancel arriving while a prompt is being set up", () => { expect(h.session.cancelled).toBe(false); }); + it("drops the prompt even when a later turn activates during setup", async () => { + const h = makeHarness(); + + const cancelled = h.prompt(); + await h.inSetup; + await h.agent.cancel({ sessionId: SESSION_ID }); + + // A local-only command skips the pre-prompt status check the first prompt is + // parked in, so it queues and activates while that prompt is still stalled. + // Activation clears `session.cancelled`, leaving the count as the only record + // that a cancel landed. + const local = h.prompt("/context"); + await h.activateQueuedTurn(); + expect(h.session.cancelled).toBe(false); + + h.finishSetup(); + + await expect(cancelled).resolves.toMatchObject({ stopReason: "cancelled" }); + expect(h.sdkPromptTexts()).toEqual(["/context"]); + + h.finishTurn(); + await expect(local).resolves.toMatchObject({ stopReason: "end_turn" }); + }); + it("leaves a mismatched cancel uncounted", async () => { const h = makeHarness(); diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts index 4c0c58c8b97d..070d1b027d13 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts @@ -525,10 +525,13 @@ export class ClaudeAcpAgent extends BaseAcpAgent { // A cancel counted during setup targets this prompt. It is not queued yet, so // `interrupt()` had nothing to stop and armed no backstop; returning here, - // before the message reaches the SDK, is what actually cancels it. Leave - // `cancelled` standing: an earlier turn may still be settling against it, and - // `activateTurn` clears it for the next turn that runs. - if (this.session.cancelSeq > cancelSeqAtEntry && this.session.cancelled) { + // before the message reaches the SDK, is what actually cancels it. The counter + // alone decides: `session.cancelled` belongs to the session, and `activateTurn` + // clears it for whichever turn runs next, so a prompt that raced past this one + // into the queue would otherwise erase the cancel this check exists to catch. + // The flag is left standing here, because an earlier turn may still be settling + // against it. + if (this.session.cancelSeq > cancelSeqAtEntry) { return this.cancelledResponse(); } From 3dcfba4527b2821a031da09010d41d59ad03e28d Mon Sep 17 00:00:00 2001 From: Charles Vien Date: Sun, 2 Aug 2026 18:36:26 -0700 Subject: [PATCH 3/3] chore(agent): remove redundant comments --- .../agent/src/adapters/base-acp-agent.ts | 13 +++---- .../claude-agent.cancel-during-setup.test.ts | 39 ++++--------------- .../agent/src/adapters/claude/claude-agent.ts | 17 +++----- 3 files changed, 20 insertions(+), 49 deletions(-) diff --git a/products/desktop/packages/agent/src/adapters/base-acp-agent.ts b/products/desktop/packages/agent/src/adapters/base-acp-agent.ts index f8021a62b3a3..9b0bf22b71ea 100644 --- a/products/desktop/packages/agent/src/adapters/base-acp-agent.ts +++ b/products/desktop/packages/agent/src/adapters/base-acp-agent.ts @@ -48,10 +48,10 @@ export interface BaseSession { notificationHistory: SessionNotification[]; cancelled: boolean; /** - * Bumped on every cancel. An adapter whose `prompt()` awaits before registering - * the turn must snapshot this on entry and re-check it before handing the prompt - * to the backend: a cancel landing in that window reaches no turn, and `cancelled` - * alone cannot distinguish it from a stale flag left by an earlier cancel. + * Bumped on every cancel. A `prompt()` that awaits before registering its turn + * snapshots this on entry and re-checks it before handing the prompt to the + * backend: `cancelled` alone cannot tell a cancel landing in that window from a + * stale flag left by an earlier cancel. */ cancelSeq: number; interruptReason?: string; @@ -87,9 +87,8 @@ export abstract class BaseAcpAgent implements Agent { this.session.cancelled = true; this.session.cancelSeq += 1; const meta = params._meta as { interruptReason?: string } | undefined; - // Assign even when absent, so the reason always describes the current cancel. - // A turn cancelled during setup keeps this value through activation, so a - // leftover reason would otherwise be reported for a cancel that supplied none. + // Assign even when absent, so a cancel that supplies no reason does not + // report a leftover reason from an earlier cancel. this.session.interruptReason = meta?.interruptReason; await this.interrupt(); } diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts index 41bc641c3f2a..bcd9245e99fd 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.cancel-during-setup.test.ts @@ -31,7 +31,6 @@ type Agent = InstanceType; const SESSION_ID = "s-cancel"; -/** Lets the consumer loop re-park in `query.next()` between messages. */ function tick(): Promise { return new Promise((resolve) => setImmediate(resolve)); } @@ -39,32 +38,21 @@ function tick(): Promise { interface Harness { agent: Agent; session: { cancelled: boolean; cancelSeq: number }; - /** Resolves once prompt() has entered the pre-prompt MCP status await. */ inSetup: Promise; - /** Lets the stalled status check return, so setup continues. */ finishSetup: () => void; - /** Drives the queued turn to a normal completion. */ completeTurn: () => Promise; - /** Promotes the queued turn the way a local-only command's output does. */ activateQueuedTurn: () => Promise; - /** Ends the active turn with the SDK's terminal result. */ finishTurn: () => void; - /** Whether anything was handed to the SDK. */ sdkReceivedMessage: () => boolean; - /** The text of every message handed to the SDK, in order. */ sdkPromptTexts: () => string[]; prompt: (text?: string) => Promise<{ stopReason: string; _meta?: unknown }>; } /** - * A session whose pre-prompt `ensureLocalToolsConnected` can be held open, which - * is the window a Ctrl-C lands in before the prompt reaches the SDK. - * - * `query.interrupt` is a deferred no-op on purpose. In production it asks the - * Claude subprocess to stop and the message stream keeps running until the - * subprocess acknowledges; the default mock ends the stream synchronously, so the - * consumer takes the stream-`done` branch and rejects the still-queued turn as - * session-ended, never reaching `activateTurn`. + * A session whose pre-prompt `ensureLocalToolsConnected` can be held open, the + * window a cancel lands in before the prompt reaches the SDK. `query.interrupt` + * is a deferred no-op on purpose: the default mock ends the stream synchronously, + * which rejects the still-queued turn as session-ended before `activateTurn`. */ function makeHarness(): Harness { const client = { @@ -149,8 +137,7 @@ function makeHarness(): Harness { await tick(); }, finishTurn: () => query._mockHelpers.complete(createSuccessResult()), - // Echo the turn's own user message back, then send the terminal result, the - // way the SDK would for a turn that ran to completion. + // Echo the pushed user message back then complete, as the real SDK would. completeTurn: async () => { const { value: pushed } = await input[Symbol.asyncIterator]().next(); query._mockHelpers.sendMessage(pushed as never); @@ -171,11 +158,6 @@ function makeHarness(): Harness { }; } -/** - * `cancelSeq` lets `prompt()` tell a cancel aimed at the prompt it is setting up - * from a stale one that already stopped an earlier turn. The first drops the - * prompt before it reaches the SDK; the second leaves it alone. - */ describe("cancel arriving while a prompt is being set up", () => { beforeEach(() => { vi.clearAllMocks(); @@ -186,8 +168,6 @@ describe("cancel arriving while a prompt is being set up", () => { const pending = h.prompt(); await h.inSetup; - // Ctrl-C lands here: nothing is queued yet, so cancel() finds no turn to stop - // and arms no backstop. Only the early return in prompt() can honor it. await h.agent.cancel({ sessionId: SESSION_ID }); h.finishSetup(); @@ -234,8 +214,6 @@ describe("cancel arriving while a prompt is being set up", () => { it("runs a prompt normally when the cancel predates it", async () => { const h = makeHarness(); - // Cancel with nothing in flight, which is how a cancel for an - // already-finished turn looks by the time the next prompt arrives. await h.agent.cancel({ sessionId: SESSION_ID }); expect(h.session.cancelled).toBe(true); @@ -255,10 +233,9 @@ describe("cancel arriving while a prompt is being set up", () => { await h.inSetup; await h.agent.cancel({ sessionId: SESSION_ID }); - // A local-only command skips the pre-prompt status check the first prompt is - // parked in, so it queues and activates while that prompt is still stalled. - // Activation clears `session.cancelled`, leaving the count as the only record - // that a cancel landed. + // A local-only command skips the status check the first prompt is parked in, + // so it activates during the stall and clears `session.cancelled`, leaving + // the count as the only record of the cancel. const local = h.prompt("/context"); await h.activateQueuedTurn(); expect(h.session.cancelled).toBe(false); diff --git a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts index 070d1b027d13..6bb8b4e6eb9f 100644 --- a/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts +++ b/products/desktop/packages/agent/src/adapters/claude/claude-agent.ts @@ -523,14 +523,10 @@ export class ClaudeAcpAgent extends BaseAcpAgent { }); } - // A cancel counted during setup targets this prompt. It is not queued yet, so - // `interrupt()` had nothing to stop and armed no backstop; returning here, - // before the message reaches the SDK, is what actually cancels it. The counter - // alone decides: `session.cancelled` belongs to the session, and `activateTurn` - // clears it for whichever turn runs next, so a prompt that raced past this one - // into the queue would otherwise erase the cancel this check exists to catch. - // The flag is left standing here, because an earlier turn may still be settling - // against it. + // A cancel counted during setup targets this prompt: nothing was queued, so + // `interrupt()` had nothing to stop, and returning before the push is what + // cancels it. `session.cancelled` is left standing because an earlier turn + // may still be settling against it. if (this.session.cancelSeq > cancelSeqAtEntry) { return this.cancelledResponse(); } @@ -711,9 +707,8 @@ export class ClaudeAcpAgent extends BaseAcpAgent { const activateTurn = async (turn: Turn) => { session.activeTurn = turn; - // Any cancel aimed at this turn already settled it: during setup `prompt()` - // returns early, and once queued `interrupt()` sweeps it. Reaching here means - // the flag is left over from an earlier turn. + // A cancel aimed at this turn already settled it (early return during + // setup, `interrupt()` sweep once queued), so a set flag here is stale. session.cancelled = false; session.interruptReason = undefined; session.pendingOrphanResults = 0;