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:
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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({
|
||||
|
||||
Reference in New Issue
Block a user