diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2d3cce630..fa2f5c1ea 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,6 +12,12 @@ ### Fixed +- `/mcp auth ` now completes an interactive OAuth login instead of stalling forever. The command announced + `Opening browser to authorize ...` but never launched a browser, and the notification carrying the + authorization URL was emitted immediately before that announcement, so the TUI's consecutive-status coalescing + overwrote it. With no browser and no reachable URL, the flow waited on its loopback callback until the session + ended. The auth command bridge now calls the real browser launcher, and each branch emits a single notification + that includes the authorization URL as a fallback for when the launch fails. - Steering queued while a provider stream-start timeout retry is running now starts automatically when that managed retry exhausts its budget, instead of remaining parked until another user prompt. Generic terminal provider errors and user-aborted retries keep their existing queue-retention behavior diff --git a/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth-dispatch.ts b/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth-dispatch.ts index 0184345de..e5b6544f9 100644 --- a/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth-dispatch.ts +++ b/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth-dispatch.ts @@ -1,3 +1,4 @@ +import { openBrowser as launchBrowser } from "../../../../../utils/open-browser.ts"; import type { ExtensionAPI, ExtensionCommandContext } from "../../../types.ts"; import { createMcpLogger } from "../log.ts"; import type { McpService } from "../service.ts"; @@ -34,7 +35,9 @@ export async function handleMcpAuthCommand( } await service.attachSession({ type: "session_start", reason: "reload" }, ctx, pi).catch(() => undefined); }, - openBrowser: (url) => ctx.ui.notify(`Open this URL to authorize ${name}:\n${url.toString()}`), + // Actually launch the browser. The URL is surfaced by the auth flow itself in a single + // status line, because consecutive ui.notify() status lines overwrite each other in the TUI. + openBrowser: (url) => launchBrowser(url.toString()), pending: service.getPendingAuth(), interactiveGuard: { begin: (serverName) => service.beginInteractiveAuth(serverName), diff --git a/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth.ts b/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth.ts index da526f617..d6d7c9145 100644 --- a/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth.ts +++ b/packages/coding-agent/src/core/extensions/builtin/mcp/auth/commands-auth.ts @@ -121,7 +121,8 @@ async function runInteractive(deps: AuthCommandDeps): Promise { const loopbackResult = channel.usesLoopback ? channel.waitForCode() : undefined; provider = buildProvider(deps, channel.redirectUrl, (url) => deps.openBrowser?.(url)); const begin = await beginAuthorization(provider, deps.flow); - if (begin.authorizationUrl !== undefined) deps.notify(`Opening browser to authorize ${deps.serverName}...`); + // One notification per branch: consecutive status lines overwrite each other in the TUI, so a + // separate "opening browser" line would erase the URL the user needs when the launch fails. if (!channel.usesLoopback) { deps.pending.set(deps.serverName, provider); if (begin.authorizationUrl === undefined) { @@ -134,10 +135,15 @@ async function runInteractive(deps: AuthCommandDeps): Promise { ); } deps.notify( - `Complete the browser flow, then run /mcp auth-complete ${deps.serverName} with the final redirect URL.`, + `Opening browser to authorize ${deps.serverName}. If it does not open, visit:\n${begin.authorizationUrl.toString()}\nComplete the browser flow, then run /mcp auth-complete ${deps.serverName} with the final redirect URL.`, ); return; } + if (begin.authorizationUrl !== undefined) { + deps.notify( + `Opening browser to authorize ${deps.serverName}. If it does not open, visit:\n${begin.authorizationUrl.toString()}`, + ); + } const { code } = await (loopbackResult ?? channel.waitForCode()); await finishAuthorization(provider, code, deps.flow); await deps.onReconnect(); diff --git a/packages/coding-agent/src/core/extensions/builtin/mcp/changes.md b/packages/coding-agent/src/core/extensions/builtin/mcp/changes.md index af4176042..b1d8338d5 100644 --- a/packages/coding-agent/src/core/extensions/builtin/mcp/changes.md +++ b/packages/coding-agent/src/core/extensions/builtin/mcp/changes.md @@ -1,5 +1,21 @@ # mcp Extension Changes +## Interactive OAuth actually opens a browser and keeps the URL visible (2026-08-17) + +### What changed +- `auth/commands-auth-dispatch.ts` wires `openBrowser` to `utils/open-browser.ts` instead of emitting a notification; the launch now really happens. +- `auth/commands-auth.ts` `runInteractive` emits exactly one notification per branch, carrying the authorization URL, instead of an announcement line that followed and erased the URL line. +- `test/mcp/oauth-callback.test.ts` asserts the single announcement contains the authorization URL for both the loopback and the callback-override branch. + +### Why +- `/mcp auth ` said "Opening browser..." but nothing ever spawned a browser, and the preceding URL notification was overwritten: consecutive `ui.notify` status lines coalesce in the TUI (`showStatus` replaces the trailing status text in place). The flow then blocked on the loopback callback with no reachable authorization URL, so interactive login could never complete. + +### Why extension system couldn't handle this alone +- The auth command bridge owns the `AuthCommandDeps` wiring and the notification sequence of the built-in `/mcp` command namespace; no external extension can reorder them. + +### Expected merge conflict zones +- LOW: `auth/commands-auth-dispatch.ts` deps literal; `auth/commands-auth.ts` `runInteractive` notification block. + ## Explicit pgrep match-all pattern for process-tree collection (2026-08-12) ### What changed diff --git a/packages/coding-agent/test/mcp/oauth-callback.test.ts b/packages/coding-agent/test/mcp/oauth-callback.test.ts index 8af23de68..21fd8aa4a 100644 --- a/packages/coding-agent/test/mcp/oauth-callback.test.ts +++ b/packages/coding-agent/test/mcp/oauth-callback.test.ts @@ -54,11 +54,13 @@ async function agentDir(): Promise { interface Harness { deps: AuthCommandDeps; browsered: URL[]; + notified: string[]; store: McpTokenStore; } function makeHarness(dir: string, mcpUrl: string, overrides: Partial = {}): Harness { const browsered: URL[] = []; + const notified: string[] = []; const config: McpServerConfig = { type: "http", url: mcpUrl, @@ -78,14 +80,21 @@ function makeHarness(dir: string, mcpUrl: string, overrides: Partial undefined, + notify: (message) => { + notified.push(message); + }, openBrowser: (url) => { browsered.push(url); }, onReconnect: () => Promise.resolve(), pending: new Map(), }; - return { deps, browsered, store: new McpTokenStore({ agentDir: dir, serverName: "fix", serverUrl: mcpUrl }) }; + return { + deps, + browsered, + notified, + store: new McpTokenStore({ agentDir: dir, serverName: "fix", serverUrl: mcpUrl }), + }; } async function followAuthorize(url: URL): Promise { @@ -242,6 +251,26 @@ describe("LoopbackCallbackServer", () => { expect(harness.browsered).toHaveLength(0); }); + // Regression: the authorization URL used to be announced in its own notification, immediately + // followed by a second status line. Consecutive status lines overwrite each other in the TUI, so + // the URL vanished and a failed browser launch left the flow waiting forever with no way out. + it("carries the authorization URL in the same single notification that announces the browser", async () => { + const fixture = await idp(); + const dir = await agentDir(); + const harness = makeHarness(dir, fixture.mcpUrl); + + const auth = runAuth(harness.deps); + await vi.waitFor(() => expect(harness.browsered).toHaveLength(1)); + const authorizationUrl = harness.browsered[0]; + if (authorizationUrl === undefined) throw new Error("no authorization URL"); + await vi.waitFor(() => expect(harness.notified).toHaveLength(1)); + expect(harness.notified[0]).toContain(authorizationUrl.toString()); + + const callback = await followAuthorize(authorizationUrl); + expect(callback.status).toBe(200); + await auth; + }); + it("runAuth with a callback URL override opens no listener and completes through pasted redirect", async () => { const fixture = await idp(); const dir = await agentDir(); @@ -256,6 +285,9 @@ describe("LoopbackCallbackServer", () => { expect(harness.deps.pending.has(harness.deps.serverName)).toBe(true); const authorizationUrl = harness.browsered[0]; if (authorizationUrl === undefined) throw new Error("no authorization URL"); + expect(harness.notified).toHaveLength(1); + expect(harness.notified[0]).toContain(authorizationUrl.toString()); + expect(harness.notified[0]).toContain("/mcp auth-complete fix"); const redirect = await authorizeRedirectLocation(authorizationUrl); await runAuthComplete(harness.deps, redirect); expect(await portOpen(port)).toBe(false);