Merge pull request #1060 from dmarsh-gusto/dmarsh/acp-thinking-level-push

fix(coding-agent/acp): push config_option_update on every thinking-level change
2026-05-14 04:44:29 +02:00
parent 9530f94a36 3ad9255b18
commit 9e28dd7252
5 changed files with 227 additions and 44 deletions
@@ -22,6 +22,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 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`.
 - Fixed MCP OAuth refresh failing with `HTTP 401 invalid_client` for servers that require Dynamic Client Registration (RFC 7591) and have no `oauth.clientId` configured (e.g. `mcp.linear.app`). `MCPOAuthFlow` registered a fresh public PKCE client on each authorize and discarded the issued `client_id` once the flow object went out of scope; refresh then called the provider's `/token` endpoint without a `client_id`. The flow now exposes `resolvedClientId` / `registeredClientSecret` getters, `MCPCommandController#handleOAuthFlow` returns them alongside `credentialId`, and both the initial-connect and `/mcp reauth` paths persist them into `auth.{clientId,clientSecret}` (used at refresh) and `oauth.{clientId,clientSecret}` (used by subsequent `/mcp reauth` to skip re-registration). The `MCPAddWizard` `onOAuth` callback type is now `Promise<MCPAddWizardOAuthResult>` and `#launchOAuthFlow` folds the registered credentials into wizard state. Servers with a statically-configured `oauth.clientId` (Notion, Slack, Datadog) are unaffected — `#tryRegisterClient` short-circuits and the write-back is a no-op. ([#1061](https://github.com/can1357/oh-my-pi/pull/1061) by [@ldx](https://github.com/ldx)).

 ## [15.0.0] - 2026-05-13
@@ -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,6 +106,9 @@ type ManagedSessionRecord = {
 	liveMessageId: string | undefined;
 	liveMessageProgress: { textEmitted: boolean; thoughtEmitted: boolean } | undefined;
 	extensionsConfigured: boolean;
+	// Installed inside `#scheduleBootstrapUpdates` (post-race-guard); released
+	// in `#disposeSessionRecord`. Lives independent of any prompt turn.
+	lifetimeUnsubscribe: (() => void) | undefined;
 };

 type ReplayableMessage = {
@@ -314,13 +326,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 +360,21 @@ 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 };
+		// 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) {
+			await this.#pushConfigOptionUpdate(record);
+		}
+		return { configOptions: this.#buildConfigOptions(record.session) };
 	}

 	async unstable_setSessionModel(params: SetSessionModelRequest): Promise<SetSessionModelResponse> {
 		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 +432,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 +682,8 @@ 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` 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);
@@ -707,9 +703,24 @@ export class AcpAgent implements Agent {
 			liveMessageId: undefined,
 			liveMessageProgress: undefined,
 			extensionsConfigured: false,
+			lifetimeUnsubscribe: undefined,
 		};
 	}

+	async #handleLifetimeEvent(record: ManagedSessionRecord, event: AgentSessionEvent): Promise<void> {
+		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 +923,16 @@ export class AcpAgent implements Agent {
 		};
 	}

+	async #pushConfigOptionUpdate(record: ManagedSessionRecord): Promise<void> {
+		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 => ({
@@ -1124,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;
@@ -1144,8 +1172,13 @@ export class AcpAgent implements Agent {
 			if (!record) {
 				return;
 			}
+			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<void> {
@@ -1674,6 +1707,7 @@ export class AcpAgent implements Agent {
 	}

 	async #disposeSessionRecord(record: ManagedSessionRecord): Promise<void> {
+		record.lifetimeUnsubscribe?.();
 		if (record.mcpManager) {
 			try {
 				await record.mcpManager.disconnectAll();
@@ -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;
 	}

@@ -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 });
 		}
 	}

@@ -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 {
@@ -365,6 +374,15 @@ async function createHarness(): Promise<AgentHarness> {
 	};
 }

+/**
+ * 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<void> {
+	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();
@@ -477,6 +495,133 @@ 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. 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 waitForBootstrapGuard();
+
+		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("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 waitForBootstrapGuard();
+		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
+		// 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: [] });
+		// Wait past the bootstrap guard so the lifetime subscription is
+		// installed and the client-driven setSessionConfigOption produces
+		// exactly one notification through it.
+		await waitForBootstrapGuard();
+
+		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();