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 <noreply@oh-my-pi.dev>
This commit is contained in:
David Marshall
2026-05-13 16:34:54 -05:00
parent c7722838b7
commit 4882d1e386
3 changed files with 89 additions and 11 deletions
+1 -1
View File
@@ -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
@@ -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<ManagedSessionRecord> {
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);
}
+55 -3
View File
@@ -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({