diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3c073f95c..9f31ac6a6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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` 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 diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 2a3556c9b..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,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 { 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 { 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 { + 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 { + 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 { @@ -1674,6 +1707,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 6f6d94fb1..b86ca481b 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 b187484a3..42ff9cea9 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 { @@ -365,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(); @@ -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();