From c7722838b70988ecdc4b011fe99190b3e19504dd Mon Sep 17 00:00:00 2001 From: David Marshall Date: Wed, 13 May 2026 16:14:18 -0500 Subject: [PATCH 1/3] fix(coding-agent/acp): pushed config_option_update on every thinking-level change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACP clients (Zed, etc.) only received `config_option_update` notifications when they themselves drove the change via `session/set_session_config_option`. Internal thinking-level updates (slash commands, automatic model-driven adjustments, extension UI) bypassed the notification path, so client config panels went stale until the next user-initiated change. AgentSession now emits a `thinking_level_changed` event from `setThinkingLevel`, and AcpAgent installs a session-lifetime subscription on each managed session that pushes a fresh `config_option_update` whenever the event fires — independent of prompt-turn lifecycle. The `session/set_session_config_option` handler no longer pushes its own notification for the `thinking` config (lifetime subscription covers it); the response still returns fresh `configOptions` so callers see the new state synchronously. Subscriptions are released in `#disposeSessionRecord`. Also consolidated four duplicate `config_option_update` send sites into a new `#pushConfigOptionUpdate(record)` helper. Tests: added two cases to `test/acp-agent.test.ts` — one verifying internal `setThinkingLevel` calls produce a `config_option_update` and a no-op re-set produces none, and one verifying client-driven `setSessionConfigOption(thinking, …)` produces exactly one notification. Co-Authored-By: omp --- packages/coding-agent/CHANGELOG.md | 4 + .../coding-agent/src/modes/acp/acp-agent.ts | 70 +++++++++------- .../src/modes/controllers/event-controller.ts | 1 + .../coding-agent/src/session/agent-session.ts | 4 +- packages/coding-agent/test/acp-agent.test.ts | 84 +++++++++++++++++++ 5 files changed, 132 insertions(+), 31 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 21e3ba775..a6a9c669f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -19,6 +19,10 @@ - Changed search truncation metadata/renderer output from match/result-based limits to file-based limits (`fileLimitReached`, `perFileLimitReached`) and updated truncation labels accordingly - Lowered `read.defaultLimit` default from `500` to `300` lines, and split the per-range context padding into asymmetric `RANGE_LEADING_CONTEXT_LINES = 1` / `RANGE_TRAILING_CONTEXT_LINES = 3` (was symmetric `RANGE_CONTEXT_LINES = 3`). Replay analysis over post-summarizer sessions (`scripts/session-stats/optimize_read_config.py`) showed that bare-path reads are over-provisioned at the median (file p50 = 220 lines) and that most follow-up reads are disjoint hops rather than adjacent extensions — so a smaller default plus narrower leading context reclaims tokens without measurably changing first-cover rate. Trailing context stays at 3 lines to keep anchor-stale recovery on narrow reads. Explicit `read.defaultLimit` overrides in settings are honoured unchanged. +### Fixed + +- Fixed ACP clients missing `config_option_update` notifications when the thinking level changed via any path other than the client's own `session/set_session_config_option` call (slash commands, model auto-adjust, extension UI). `AgentSession` now emits a `thinking_level_changed` event from `setThinkingLevel`, and `AcpAgent` subscribes to each managed session for the session's lifetime — independent of any active prompt turn — and pushes a fresh `config_option_update` whenever the effective level changes. The `session/set_session_config_option` handler no longer pushes its own duplicate notification for the `thinking` config; clients still get the new state both via the lifetime subscription and the response payload. Subscriptions are released in `#disposeSessionRecord`. + ## [15.0.0] - 2026-05-13 ### Breaking Changes diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 2a3556c9b..8f7ebb748 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -97,6 +97,8 @@ type ManagedSessionRecord = { liveMessageId: string | undefined; liveMessageProgress: { textEmitted: boolean; thoughtEmitted: boolean } | undefined; extensionsConfigured: boolean; + // Independent of prompt-turn lifecycle — see `#handleLifetimeEvent`. + lifetimeUnsubscribe: (() => void) | undefined; }; type ReplayableMessage = { @@ -314,13 +316,7 @@ export class AcpAgent implements Agent { sessionId: record.session.sessionId, update: this.#buildCurrentModeUpdate(record.session), }); - await this.#connection.sessionUpdate({ - sessionId: record.session.sessionId, - update: { - sessionUpdate: "config_option_update", - configOptions: this.#buildConfigOptions(record.session), - }, - }); + await this.#pushConfigOptionUpdate(record); return {}; } @@ -354,27 +350,18 @@ export class AcpAgent implements Agent { }); } - const configOptions = this.#buildConfigOptions(record.session); - await this.#connection.sessionUpdate({ - sessionId: record.session.sessionId, - update: { - sessionUpdate: "config_option_update", - configOptions, - }, - }); - return { configOptions }; + // Thinking-level changes are pushed via the lifetime subscription on + // `thinking_level_changed`; skipping the push here avoids a duplicate. + if (params.configId !== THINKING_CONFIG_ID) { + await this.#pushConfigOptionUpdate(record); + } + return { configOptions: this.#buildConfigOptions(record.session) }; } async unstable_setSessionModel(params: SetSessionModelRequest): Promise { const record = this.#getSessionRecord(params.sessionId); await this.#setModelById(record.session, params.modelId); - await this.#connection.sessionUpdate({ - sessionId: record.session.sessionId, - update: { - sessionUpdate: "config_option_update", - configOptions: this.#buildConfigOptions(record.session), - }, - }); + await this.#pushConfigOptionUpdate(record); return {}; } @@ -432,13 +419,7 @@ export class AcpAgent implements Agent { }); }, notifyConfigChanged: async () => { - await this.#connection.sessionUpdate({ - sessionId: record.session.sessionId, - update: { - sessionUpdate: "config_option_update", - configOptions: this.#buildConfigOptions(record.session), - }, - }); + await this.#pushConfigOptionUpdate(record); }, }); if (builtinResult !== false) { @@ -688,6 +669,9 @@ export class AcpAgent implements Agent { async #registerPreparedSession(session: AgentSession, mcpServers: McpServer[]): Promise { const record = this.#createManagedSessionRecord(session); session.setClientBridge(createAcpClientBridge(this.#connection, session.sessionId, this.#clientCapabilities)); + record.lifetimeUnsubscribe = session.subscribe(event => { + void this.#handleLifetimeEvent(record, event); + }); try { await this.#configureExtensions(record); await this.#configureMcpServers(record, mcpServers); @@ -707,9 +691,24 @@ export class AcpAgent implements Agent { liveMessageId: undefined, liveMessageProgress: undefined, extensionsConfigured: false, + lifetimeUnsubscribe: undefined, }; } + async #handleLifetimeEvent(record: ManagedSessionRecord, event: AgentSessionEvent): Promise { + if (event.type !== "thinking_level_changed") { + return; + } + try { + await this.#pushConfigOptionUpdate(record); + } catch (error) { + logger.warn("Failed to push thinking-level config_option_update", { + sessionId: record.session.sessionId, + error, + }); + } + } + #getSessionRecord(sessionId: string): ManagedSessionRecord { const record = this.#sessions.get(sessionId); if (!record) { @@ -912,6 +911,16 @@ export class AcpAgent implements Agent { }; } + async #pushConfigOptionUpdate(record: ManagedSessionRecord): Promise { + await this.#connection.sessionUpdate({ + sessionId: record.session.sessionId, + update: { + sessionUpdate: "config_option_update", + configOptions: this.#buildConfigOptions(record.session), + }, + }); + } + #buildConfigOptions(session: AgentSession): SessionConfigOption[] { const currentModeId = this.#getCurrentModeId(session); const modeOptions = this.#getAvailableModes(session).map(mode => ({ @@ -1674,6 +1683,7 @@ export class AcpAgent implements Agent { } async #disposeSessionRecord(record: ManagedSessionRecord): Promise { + record.lifetimeUnsubscribe?.(); if (record.mcpManager) { try { await record.mcpManager.disconnectAll(); diff --git a/packages/coding-agent/src/modes/controllers/event-controller.ts b/packages/coding-agent/src/modes/controllers/event-controller.ts index 1bfa15f83..5a0d71bbc 100644 --- a/packages/coding-agent/src/modes/controllers/event-controller.ts +++ b/packages/coding-agent/src/modes/controllers/event-controller.ts @@ -61,6 +61,7 @@ export class EventController { todo_auto_clear: e => this.#handleTodoAutoClear(e), irc_message: e => this.#handleIrcMessage(e), notice: e => this.#handleNotice(e), + thinking_level_changed: async () => {}, } satisfies AgentSessionEventHandlers; } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index f61f35a13..e056bddb5 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -214,7 +214,8 @@ export type AgentSessionEvent = | { type: "todo_reminder"; todos: TodoItem[]; attempt: number; maxAttempts: number } | { type: "todo_auto_clear" } | { type: "irc_message"; message: CustomMessage } - | { type: "notice"; level: "info" | "warning" | "error"; message: string; source?: string }; + | { type: "notice"; level: "info" | "warning" | "error"; message: string; source?: string } + | { type: "thinking_level_changed"; thinkingLevel: ThinkingLevel | undefined }; /** Listener function for agent session events */ export type AgentSessionEventListener = (event: AgentSessionEvent) => void; @@ -4551,6 +4552,7 @@ export class AgentSession { if (persist && effectiveLevel !== undefined && effectiveLevel !== ThinkingLevel.Off) { this.settings.set("defaultThinkingLevel", effectiveLevel); } + this.#emit({ type: "thinking_level_changed", thinkingLevel: effectiveLevel }); } } diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index ee169311a..8db0f7908 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -125,7 +125,16 @@ class FakeAgentSession { } setThinkingLevel(level: string | undefined): void { + const isChanging = this.thinkingLevel !== level; this.thinkingLevel = level; + if (isChanging) { + for (const listener of this.#listeners) { + listener({ + type: "thinking_level_changed", + thinkingLevel: level, + } as AgentSessionEvent); + } + } } setSlashCommands(_commands: unknown[]): void { @@ -477,6 +486,81 @@ describe("ACP agent", () => { await Bun.sleep(0); }); + it("pushes config_option_update when thinking level changes internally", async () => { + // Internal callers (slash commands, model auto-adjust, extension UI) call + // AgentSession.setThinkingLevel directly without going through the ACP + // setSessionConfigOption surface. The session-lifetime subscription on + // AcpAgent must surface those changes to clients as `config_option_update` + // so TORTAS-style fleet views stay in sync. + const harness = await createHarness(); + const created = await harness.agent.newSession({ cwd: harness.cwdA, mcpServers: [] }); + const session = harness.findSession(created.sessionId)!; + + const updatesBefore = harness.updates.length; + session.setThinkingLevel("high"); + + const pushedAfter = harness.updates.slice(updatesBefore); + const configUpdates = pushedAfter.filter( + notification => + notification.sessionId === created.sessionId && + notification.update.sessionUpdate === "config_option_update", + ); + expect(configUpdates.length).toBeGreaterThanOrEqual(1); + expectAcpNotifications(configUpdates); + const firstUpdate = configUpdates[0]!.update; + if (firstUpdate.sessionUpdate !== "config_option_update") { + throw new Error("expected config_option_update"); + } + const thinkingConfig = firstUpdate.configOptions.find(option => option.id === "thinking") as + | { currentValue?: unknown } + | undefined; + expect(thinkingConfig?.currentValue).toBe("high"); + + // Setting to the same level must not produce a redundant notification. + const updatesBeforeRedundant = harness.updates.length; + session.setThinkingLevel("high"); + expect(harness.updates.length).toBe(updatesBeforeRedundant); + + harness.abortController.abort(); + await Bun.sleep(0); + }); + + it("emits a single config_option_update per setSessionConfigOption(thinking) call", async () => { + // Client-initiated thinking changes flow through #setThinkingLevelById, + // which fires `thinking_level_changed` and lets the lifetime subscription + // push the notification. The ACP surface must not also push a duplicate + // `config_option_update` of its own. + const harness = await createHarness(); + const created = await harness.agent.newSession({ cwd: harness.cwdA, mcpServers: [] }); + + const updatesBefore = harness.updates.length; + const response = await harness.agent.setSessionConfigOption({ + sessionId: created.sessionId, + configId: "thinking", + value: "high", + }); + + const configUpdates = harness.updates + .slice(updatesBefore) + .filter( + notification => + notification.sessionId === created.sessionId && + notification.update.sessionUpdate === "config_option_update", + ); + expect(configUpdates.length).toBe(1); + expectAcpNotifications(configUpdates); + + // The response still carries the fresh configOptions tree so the caller + // gets the new state without relying on the notification. + const thinkingOption = response.configOptions.find(option => option.id === "thinking") as + | { currentValue?: unknown } + | undefined; + expect(thinkingOption?.currentValue).toBe("high"); + + harness.abortController.abort(); + await Bun.sleep(0); + }); + it("accepts only ACP underscore-prefixed extension methods", async () => { const harness = await createHarness(); From 4882d1e3867589bca5c4499870ff8e7f7e0dc9a6 Mon Sep 17 00:00:00 2001 From: David Marshall Date: Wed, 13 May 2026 16:34:54 -0500 Subject: [PATCH 2/3] fix(coding-agent/acp): deferred thinking-level lifetime subscription until after bootstrap-guard Addresses codex review on #1060: an extension session_start handler that calls setThinkingLevel via the exposed extension action (line 1541) would have run BEFORE #registerPreparedSession set the record into #sessions and BEFORE the session/new response was delivered to the client, causing config_option_update to be pushed for a session id the client did not yet know about. This is the exact race that #scheduleBootstrapUpdates already documents and guards for available_commands_update / session_info_update (Zed's 'Received session notification for unknown session' drop). Moved the session.subscribe(...) installation out of #registerPreparedSession and into #scheduleBootstrapUpdates's 50ms timer callback so the lifetime subscription shares the same response-delivery guard as the existing bootstrap notifications. The pre-bootstrap thinking level is still communicated to the client through the response payload's configOptions (newSession / loadSession / resumeSession / unstable_forkSession all return it), so no state is lost; it is only the notification that is deferred. For client-driven setSessionConfigOption({thinking}) the handler now only skips its own push when the lifetime subscription is already installed. Pre-bootstrap the handler keeps pushing (the client knows the session id because they passed it in), post-bootstrap the subscription pushes exactly once. No double-push, no missing pre-bootstrap notification. Tests: - updated existing pushes-config-option-update test to await past the 50ms bootstrap timer before driving the internal setThinkingLevel - updated the single-config_option_update-per-setSessionConfigOption test the same way - added 'suppresses lifetime config_option_update during the bootstrap window' regression that drives setThinkingLevel synchronously after newSession and asserts zero notifications, then asserts notifications resume after the bootstrap timer fires - bun test test/acp-agent.test.ts: 11/11 pass Co-Authored-By: omp --- packages/coding-agent/CHANGELOG.md | 2 +- .../coding-agent/src/modes/acp/acp-agent.ts | 40 ++++++++++--- packages/coding-agent/test/acp-agent.test.ts | 58 ++++++++++++++++++- 3 files changed, 89 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a6a9c669f..72617423c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -21,7 +21,7 @@ ### Fixed -- Fixed ACP clients missing `config_option_update` notifications when the thinking level changed via any path other than the client's own `session/set_session_config_option` call (slash commands, model auto-adjust, extension UI). `AgentSession` now emits a `thinking_level_changed` event from `setThinkingLevel`, and `AcpAgent` subscribes to each managed session for the session's lifetime — independent of any active prompt turn — and pushes a fresh `config_option_update` whenever the effective level changes. The `session/set_session_config_option` handler no longer pushes its own duplicate notification for the `thinking` config; clients still get the new state both via the lifetime subscription and the response payload. Subscriptions are released in `#disposeSessionRecord`. +- Fixed ACP clients missing `config_option_update` notifications when the thinking level changed via any path other than the client's own `session/set_session_config_option` call (slash commands, model auto-adjust, extension UI). `AgentSession` now emits a `thinking_level_changed` event from `setThinkingLevel`, and `AcpAgent` subscribes to each managed session for the session's lifetime and pushes a fresh `config_option_update` whenever the effective level changes — independent of any active prompt turn. The subscription is installed inside `#scheduleBootstrapUpdates`'s 50 ms timer so it shares the same race guard that prevents Zed's `Received session notification for unknown session` drop when notifications fire before `session/new` (or fork) returns; the pre-bootstrap thinking level is reported in the response's `configOptions`. The `session/set_session_config_option` handler keeps its own push only when the subscription has not yet been installed, so client-driven thinking changes still notify pre-bootstrap, post-bootstrap they flow through the subscription exactly once. Subscriptions are released in `#disposeSessionRecord`. ## [15.0.0] - 2026-05-13 ### Breaking Changes diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 8f7ebb748..0da6ec94e 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -97,7 +97,9 @@ type ManagedSessionRecord = { liveMessageId: string | undefined; liveMessageProgress: { textEmitted: boolean; thoughtEmitted: boolean } | undefined; extensionsConfigured: boolean; - // Independent of prompt-turn lifecycle — see `#handleLifetimeEvent`. + // Installed by `#scheduleBootstrapUpdates` (after the 50ms response-race + // guard) and torn down by `#disposeSessionRecord`. Independent of the + // prompt-turn lifecycle — see `#handleLifetimeEvent`. lifetimeUnsubscribe: (() => void) | undefined; }; @@ -350,9 +352,15 @@ export class AcpAgent implements Agent { }); } - // Thinking-level changes are pushed via the lifetime subscription on - // `thinking_level_changed`; skipping the push here avoids a duplicate. - if (params.configId !== THINKING_CONFIG_ID) { + // For `thinking` the lifetime subscription pushes a fresh + // `config_option_update` whenever the effective level changes. Skip the + // handler's own push when that subscription is already installed + // (post-bootstrap) to avoid a duplicate notification. Pre-bootstrap we + // still need to push here so the client sees the change — the + // subscription only starts firing once `#scheduleBootstrapUpdates` runs. + const thinkingHandledBySubscription = + params.configId === THINKING_CONFIG_ID && record.lifetimeUnsubscribe !== undefined; + if (!thinkingHandledBySubscription) { await this.#pushConfigOptionUpdate(record); } return { configOptions: this.#buildConfigOptions(record.session) }; @@ -669,9 +677,14 @@ export class AcpAgent implements Agent { async #registerPreparedSession(session: AgentSession, mcpServers: McpServer[]): Promise { const record = this.#createManagedSessionRecord(session); session.setClientBridge(createAcpClientBridge(this.#connection, session.sessionId, this.#clientCapabilities)); - record.lifetimeUnsubscribe = session.subscribe(event => { - void this.#handleLifetimeEvent(record, event); - }); + // Lifetime subscription is installed in `#scheduleBootstrapUpdates` so it + // shares the 50ms guard that protects against Zed's + // `Received session notification for unknown session` race — the + // `session/new` (or fork) response has to land before we start pushing + // `config_option_update` notifications for this session id. The + // post-extension thinking level is already reported in the response's + // `configOptions`, so no notifications are dropped — they're just + // deferred until the client knows the session id. try { await this.#configureExtensions(record); await this.#configureMcpServers(record, mcpServers); @@ -1153,6 +1166,19 @@ export class AcpAgent implements Agent { if (!record) { return; } + // Install the session-lifetime subscription now — same 50ms guard. + // Subscribing earlier in `#registerPreparedSession` would let an + // extension's `session_start` handler (or any async work it + // schedules) call `setThinkingLevel` and push a + // `config_option_update` for a session id the client hasn't been + // told about yet (same Zed race the bootstrap delay solves). + // `#disposeSessionRecord` releases this and tolerates `undefined` + // when the session is closed before the timer fires. + if (!record.lifetimeUnsubscribe) { + record.lifetimeUnsubscribe = record.session.subscribe(event => { + void this.#handleLifetimeEvent(record, event); + }); + } void this.#emitBootstrapUpdates(sessionId, record); }, 50); } diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index 8db0f7908..f7ab1e82d 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -489,12 +489,16 @@ describe("ACP agent", () => { it("pushes config_option_update when thinking level changes internally", async () => { // Internal callers (slash commands, model auto-adjust, extension UI) call // AgentSession.setThinkingLevel directly without going through the ACP - // setSessionConfigOption surface. The session-lifetime subscription on - // AcpAgent must surface those changes to clients as `config_option_update` - // so TORTAS-style fleet views stay in sync. + // setSessionConfigOption surface. Once the session-lifetime subscription + // is installed (after the 50ms bootstrap guard so the response has + // reached the client first), those changes must surface to clients as + // `config_option_update` so TORTAS-style fleet views stay in sync. const harness = await createHarness(); const created = await harness.agent.newSession({ cwd: harness.cwdA, mcpServers: [] }); const session = harness.findSession(created.sessionId)!; + // Wait past the 50ms bootstrap timer so the lifetime subscription is + // installed before we drive an internal thinking-level change. + await Bun.sleep(80); const updatesBefore = harness.updates.length; session.setThinkingLevel("high"); @@ -525,6 +529,50 @@ describe("ACP agent", () => { await Bun.sleep(0); }); + it("suppresses lifetime config_option_update during the bootstrap window", async () => { + // Regression for codex review on #1060: an extension `session_start` + // handler calling `setThinkingLevel` must not push a + // `config_option_update` for a session id the client has not been told + // about yet (matches Zed's `Received session notification for unknown + // session` race that `#scheduleBootstrapUpdates` already guards). + // The fake harness lets us simulate that pre-bootstrap window by + // driving the change before sleeping past the 50ms guard. + const harness = await createHarness(); + const created = await harness.agent.newSession({ cwd: harness.cwdA, mcpServers: [] }); + const session = harness.findSession(created.sessionId)!; + + const updatesBefore = harness.updates.length; + // Synchronously after `newSession` returns, the bootstrap timer has + // not fired yet, so the lifetime subscription is not installed. + session.setThinkingLevel("high"); + + const beforeBootstrap = harness.updates + .slice(updatesBefore) + .filter( + notification => + notification.sessionId === created.sessionId && + notification.update.sessionUpdate === "config_option_update", + ); + expect(beforeBootstrap.length).toBe(0); + + // After the 50ms bootstrap timer fires the subscription is installed, + // and subsequent changes do surface. + await Bun.sleep(80); + const baseline = harness.updates.length; + session.setThinkingLevel("medium"); + const afterBootstrap = harness.updates + .slice(baseline) + .filter( + notification => + notification.sessionId === created.sessionId && + notification.update.sessionUpdate === "config_option_update", + ); + expect(afterBootstrap.length).toBeGreaterThanOrEqual(1); + + harness.abortController.abort(); + await Bun.sleep(0); + }); + it("emits a single config_option_update per setSessionConfigOption(thinking) call", async () => { // Client-initiated thinking changes flow through #setThinkingLevelById, // which fires `thinking_level_changed` and lets the lifetime subscription @@ -532,6 +580,10 @@ describe("ACP agent", () => { // `config_option_update` of its own. const harness = await createHarness(); const created = await harness.agent.newSession({ cwd: harness.cwdA, mcpServers: [] }); + // Wait past the bootstrap guard so the lifetime subscription is + // installed and the client-driven setSessionConfigOption produces + // exactly one notification through it. + await Bun.sleep(80); const updatesBefore = harness.updates.length; const response = await harness.agent.setSessionConfigOption({ From b62660931419893f1c866ce30406eab22fcaf93b Mon Sep 17 00:00:00 2001 From: David Marshall Date: Wed, 13 May 2026 16:43:45 -0500 Subject: [PATCH 3/3] refactor(coding-agent/acp): extracted ACP_BOOTSTRAP_RACE_GUARD_MS and consolidated race notes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /simplify pass on 4882d1e38. Three small cleanups, no behavior change. * Extracted the inline 50ms bootstrap-race guard into an exported ACP_BOOTSTRAP_RACE_GUARD_MS constant at the top of acp-agent.ts. Source uses it in the #scheduleBootstrapUpdates setTimeout. Tests import it and call a new waitForBootstrapGuard() helper (constant + 30ms slack for setTimeout drift) instead of three hardcoded Bun.sleep(80) sites — tests now bind to the source-of-truth instead of dueling magic numbers. * Consolidated four block comments that all narrated the same race story into one canonical explanation at the install site (#scheduleBootstrapUpdates). Field declaration, #registerPreparedSession, and the setSessionConfigOption handler keep brief one/two-line pointers. Net change is roughly 30 lines of comments removed without losing the diagnosis. * Trimmed the handler-site thinkingHandledBySubscription comment from six lines to three; the local-variable name carries the intent. Verified: * bun test test/acp-agent.test.ts: 11/11 pass * biome check on touched files: clean * No behavior change (no test had to be updated) Co-Authored-By: omp --- .../coding-agent/src/modes/acp/acp-agent.ts | 74 +++++++++---------- packages/coding-agent/test/acp-agent.test.ts | 17 ++++- 2 files changed, 49 insertions(+), 42 deletions(-) diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 0da6ec94e..43255b477 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -73,6 +73,15 @@ const MODEL_CONFIG_ID = "model"; const THINKING_CONFIG_ID = "thinking"; const THINKING_OFF = "off"; const SESSION_PAGE_SIZE = 50; +/** + * Delay between `session/new` (or `session/load` / `session/resume` / + * `unstable_session/fork`) returning and the agent firing the first + * notifications against the new session id. Mitigates Zed's + * `Received session notification for unknown session` race — see + * `#scheduleBootstrapUpdates`. Exported so the ACP test harness can + * wait past this guard without hard-coding the literal. + */ +export const ACP_BOOTSTRAP_RACE_GUARD_MS = 50; type AgentImageContent = { type: "image"; @@ -97,9 +106,8 @@ type ManagedSessionRecord = { liveMessageId: string | undefined; liveMessageProgress: { textEmitted: boolean; thoughtEmitted: boolean } | undefined; extensionsConfigured: boolean; - // Installed by `#scheduleBootstrapUpdates` (after the 50ms response-race - // guard) and torn down by `#disposeSessionRecord`. Independent of the - // prompt-turn lifecycle — see `#handleLifetimeEvent`. + // Installed inside `#scheduleBootstrapUpdates` (post-race-guard); released + // in `#disposeSessionRecord`. Lives independent of any prompt turn. lifetimeUnsubscribe: (() => void) | undefined; }; @@ -352,12 +360,9 @@ export class AcpAgent implements Agent { }); } - // For `thinking` the lifetime subscription pushes a fresh - // `config_option_update` whenever the effective level changes. Skip the - // handler's own push when that subscription is already installed - // (post-bootstrap) to avoid a duplicate notification. Pre-bootstrap we - // still need to push here so the client sees the change — the - // subscription only starts firing once `#scheduleBootstrapUpdates` runs. + // For `thinking` the lifetime subscription pushes post-bootstrap; only + // push here when it's not yet installed so pre-bootstrap callers still + // see the change without a post-bootstrap duplicate. const thinkingHandledBySubscription = params.configId === THINKING_CONFIG_ID && record.lifetimeUnsubscribe !== undefined; if (!thinkingHandledBySubscription) { @@ -677,14 +682,8 @@ export class AcpAgent implements Agent { async #registerPreparedSession(session: AgentSession, mcpServers: McpServer[]): Promise { const record = this.#createManagedSessionRecord(session); session.setClientBridge(createAcpClientBridge(this.#connection, session.sessionId, this.#clientCapabilities)); - // Lifetime subscription is installed in `#scheduleBootstrapUpdates` so it - // shares the 50ms guard that protects against Zed's - // `Received session notification for unknown session` race — the - // `session/new` (or fork) response has to land before we start pushing - // `config_option_update` notifications for this session id. The - // post-extension thinking level is already reported in the response's - // `configOptions`, so no notifications are dropped — they're just - // deferred until the client knows the session id. + // `record.lifetimeUnsubscribe` is installed in `#scheduleBootstrapUpdates` + // so it shares the bootstrap race guard — see that comment for why. try { await this.#configureExtensions(record); await this.#configureMcpServers(record, mcpServers); @@ -1146,18 +1145,25 @@ export class AcpAgent implements Agent { } #scheduleBootstrapUpdates(sessionId: string): void { - // Delay the bootstrap so the client has time to handle the `session/new` - // (or `session/load` / `session/resume`) RPC response and register the - // new sessionId before we start firing notifications against it. Zed's - // agent-client-protocol reader dispatches responses and notifications - // to different async tasks; sending the first `available_commands_update` - // from `setTimeout(0)` reliably loses the race against the response - // handler and Zed logs `Received session notification for unknown - // session` then drops the update — leaving the slash-command palette - // empty (#1015 follow-up; see zed-industries/zed#55965 for the same - // race biting other ACP agents). 50ms is invisible to the operator and - // large enough that the response future has scheduled before our timer - // fires on stdio-only transports. + // Defer first notifications until the response has reached the client. + // Zed's agent-client-protocol reader dispatches responses and + // notifications to different async tasks; sending the first + // `available_commands_update` from `setTimeout(0)` reliably loses the + // race against the response handler and Zed logs `Received session + // notification for unknown session` then drops the update — leaving + // the slash-command palette empty (#1015 follow-up; see + // zed-industries/zed#55965 for the same race biting other ACP agents). + // `ACP_BOOTSTRAP_RACE_GUARD_MS` is invisible to the operator and large + // enough that the response future has scheduled before our timer fires + // on stdio-only transports. + // + // The session-lifetime subscription is installed inside the same timer + // so it shares this guard — without it, an extension's `session_start` + // handler (or any async work it schedules) calling `setThinkingLevel` + // would push a `config_option_update` for a session id the client + // hasn't been told about yet. The pre-bootstrap thinking level is + // reported in the response's `configOptions`, so deferring the + // notification loses no state. setTimeout(() => { if (this.#connection.signal.aborted) { return; @@ -1166,21 +1172,13 @@ export class AcpAgent implements Agent { if (!record) { return; } - // Install the session-lifetime subscription now — same 50ms guard. - // Subscribing earlier in `#registerPreparedSession` would let an - // extension's `session_start` handler (or any async work it - // schedules) call `setThinkingLevel` and push a - // `config_option_update` for a session id the client hasn't been - // told about yet (same Zed race the bootstrap delay solves). - // `#disposeSessionRecord` releases this and tolerates `undefined` - // when the session is closed before the timer fires. if (!record.lifetimeUnsubscribe) { record.lifetimeUnsubscribe = record.session.subscribe(event => { void this.#handleLifetimeEvent(record, event); }); } void this.#emitBootstrapUpdates(sessionId, record); - }, 50); + }, ACP_BOOTSTRAP_RACE_GUARD_MS); } async #emitBootstrapUpdates(sessionId: string, record: ManagedSessionRecord): Promise { diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index f7ab1e82d..6cb67cef6 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -13,7 +13,7 @@ import { import type { Model } from "@oh-my-pi/pi-ai"; import { getConfigRootDir, setAgentDir } from "@oh-my-pi/pi-utils"; import { _resetSettingsForTest, Settings } from "../src/config/settings"; -import { AcpAgent } from "../src/modes/acp/acp-agent"; +import { ACP_BOOTSTRAP_RACE_GUARD_MS, AcpAgent } from "../src/modes/acp/acp-agent"; import type { PlanModeState } from "../src/plan-mode/state"; import type { AgentSession, AgentSessionEvent } from "../src/session/agent-session"; import { SessionManager } from "../src/session/session-manager"; @@ -374,6 +374,15 @@ async function createHarness(): Promise { }; } +/** + * Wait until `#scheduleBootstrapUpdates`'s timer has fired and the + * session-lifetime subscription is installed. 30 ms of slack absorbs + * `setTimeout` drift without slowing tests meaningfully. + */ +async function waitForBootstrapGuard(): Promise { + await Bun.sleep(ACP_BOOTSTRAP_RACE_GUARD_MS + 30); +} + describe("ACP agent", () => { it("supports multiple live ACP sessions with model and lifecycle handlers", async () => { const harness = await createHarness(); @@ -498,7 +507,7 @@ describe("ACP agent", () => { const session = harness.findSession(created.sessionId)!; // Wait past the 50ms bootstrap timer so the lifetime subscription is // installed before we drive an internal thinking-level change. - await Bun.sleep(80); + await waitForBootstrapGuard(); const updatesBefore = harness.updates.length; session.setThinkingLevel("high"); @@ -557,7 +566,7 @@ describe("ACP agent", () => { // After the 50ms bootstrap timer fires the subscription is installed, // and subsequent changes do surface. - await Bun.sleep(80); + await waitForBootstrapGuard(); const baseline = harness.updates.length; session.setThinkingLevel("medium"); const afterBootstrap = harness.updates @@ -583,7 +592,7 @@ describe("ACP agent", () => { // Wait past the bootstrap guard so the lifetime subscription is // installed and the client-driven setSessionConfigOption produces // exactly one notification through it. - await Bun.sleep(80); + await waitForBootstrapGuard(); const updatesBefore = harness.updates.length; const response = await harness.agent.setSessionConfigOption({