From 59630418d9ae17d92bef0d7540c998382e113ac0 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 11 Sep 2026 22:12:23 -0700 Subject: [PATCH 1/2] Preserve MCP authorization marker after timeout --- src/agent/tools-mcp-disconnect.test.ts | 31 +++++++++++++++++++++++++- src/agent/tools.ts | 19 ++++++++++++++-- src/mcp/client-auth-reauth-cap.test.ts | 25 ++++++++++++++++++++- src/mcp/client.ts | 23 ++++++++++++++++--- src/tui/product-host.ts | 11 +++++++-- src/tui/runtime-channels.test.ts | 26 +++++++++++++++++++++ src/tui/runtime-notices.test.ts | 19 ++++++++++++++++ src/tui/runtime-notices.ts | 10 ++++++++- 8 files changed, 154 insertions(+), 10 deletions(-) diff --git a/src/agent/tools-mcp-disconnect.test.ts b/src/agent/tools-mcp-disconnect.test.ts index 4a167b7f1..3c59f3fb7 100644 --- a/src/agent/tools-mcp-disconnect.test.ts +++ b/src/agent/tools-mcp-disconnect.test.ts @@ -17,7 +17,7 @@ const closedGenerations: number[] = []; let connectGeneration = 0; let connectOptions: MCPConnectOptions[] = []; let releaseDeferredConnect: (() => void) | undefined; -let connectMode: "success" | "deferred" = "success"; +let connectMode: "success" | "deferred" | "auth-pending" = "success"; // Reconnect tests repoint this to simulate a server whose tool set drifted // between generations; the default matches the original static payload. let connectedTools: MCPTool[] = [ @@ -56,6 +56,14 @@ await withMockedModule( }; } } + if (connectMode === "auth-pending") { + return { + ok: false as const, + serverName: config.name, + error: "timed out waiting for the browser", + authPending: true, + }; + } return { ok: true as const, client: { @@ -440,4 +448,25 @@ describe("setMcpServersSource", () => { await toolset.dispose(); } }); + + test("an auth-pending connect result reaches onStatus marked as such", async () => { + const toolset = await makeToolset(); + const states: MCPServerState[] = []; + connectMode = "auth-pending"; + try { + await toolset.connectMCPServer(acme, callbacks(states)); + const failed = states.filter((s) => s.state === "failed"); + expect(failed).toEqual([ + { + name: "acme", + state: "failed", + error: "timed out waiting for the browser", + authPending: true, + }, + ]); + expect(toolset.hasMCPServer("acme")).toBe(false); + } finally { + await toolset.dispose(); + } + }); }); diff --git a/src/agent/tools.ts b/src/agent/tools.ts index db3ed89f0..66c4fa079 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -29,6 +29,7 @@ import { } from "../plugins/result-truncation-plugin.js"; import type { CompactionArchive } from "../session/compaction-archive.js"; import { + BrowserAuthPendingError, connectMCPServer as connectMCPClient, type MCPClient, type MCPConnectResult, @@ -267,7 +268,13 @@ export type MCPServerState = | { name: string; state: "connecting" } | { name: string; state: "needs-auth"; url: string } | { name: string; state: "connected"; tools: string[] } - | { name: string; state: "failed"; error: string } + | { + name: string; + state: "failed"; + error: string; + /** Browser auth was offered but never finished — the auth marker owns it. */ + authPending?: boolean; + } | { name: string; state: "disconnected" }; export interface MCPConnectCallbacks { @@ -956,7 +963,14 @@ export async function createAgentToolset( }); } if (!disposed) - callbacks.onStatus({ name: config.name, state: "failed", error }); + callbacks.onStatus({ + name: config.name, + state: "failed", + error, + ...(err instanceof BrowserAuthPendingError + ? { authPending: true } + : {}), + }); return; } if (disposed) { @@ -981,6 +995,7 @@ export async function createAgentToolset( name: config.name, state: "failed", error: result.error, + ...(result.authPending === true ? { authPending: true } : {}), }); return; } diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index 1e8f80d2e..d506084c6 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -266,6 +266,7 @@ const config = { async function connectWithAuthPrompt(): Promise<{ ok: boolean; error?: string; + authPending?: boolean; }> { const result = await connectMCPServer(config, { onAuthURL: () => { @@ -273,7 +274,13 @@ async function connectWithAuthPrompt(): Promise<{ authEvents.push("authURL"); }, }); - return result.ok ? { ok: true } : { ok: false, error: result.error }; + return result.ok + ? { ok: true } + : { + ok: false, + error: result.error, + ...(result.authPending === true ? { authPending: true } : {}), + }; } describe("HTTP MCP re-auth loop prevention", () => { @@ -755,6 +762,9 @@ describe("HTTP MCP re-auth loop prevention", () => { for (let episode = 0; episode < 2; episode += 1) { const result = await connectWithAuthPrompt(); expect(result.ok).toBe(false); + // The cap is an unfinished authorization, not a dead server: the TUI + // keeps the prompt-box auth marker rather than painting a failure row. + expect(result.authPending).toBe(true); expect(result.error).toContain( `MCP authorization for linear failed after ${MAX_BROWSER_AUTH_ATTEMPTS} ${MAX_BROWSER_AUTH_ATTEMPTS === 1 ? "attempt" : "attempts"}`, ); @@ -817,6 +827,7 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(await connectWithAuthPrompt()).toEqual({ ok: false, error: expect.stringContaining("retrying paused"), + authPending: true, }); expect(authURLCount).toBe(MAX_BROWSER_AUTH_ATTEMPTS); @@ -1008,6 +1019,7 @@ describe("HTTP MCP re-auth loop prevention", () => { const result = await connectWithAuthPrompt(); expect(result.ok).toBe(false); + expect(result.authPending).toBe(true); expect(result.error).toContain("timed out waiting for the browser"); expect(result.error).toContain("disconnected"); expect(authURLCount).toBe(1); @@ -1015,7 +1027,18 @@ describe("HTTP MCP re-auth loop prevention", () => { const capped = await connectWithAuthPrompt(); expect(capped.ok).toBe(false); + expect(capped.authPending).toBe(true); expect(capped.error).toContain("retrying paused"); expect(authURLCount).toBe(1); }); + + test("a failure that is not the authorization itself is not auth-pending", async () => { + connectFailuresLeft = Number.POSITIVE_INFINITY; + + const result = await connectWithAuthPrompt(); + + expect(result.ok).toBe(false); + expect(result.error).toContain("finishAuth exploded"); + expect(result.authPending).toBeUndefined(); + }); }); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 3269a2691..19a4f54af 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -46,7 +46,16 @@ export interface MCPClient { export type MCPConnectResult = | { ok: true; client: MCPClient } - | { ok: false; serverName: string; error: string }; + | { + ok: false; + serverName: string; + error: string; + /** + * The failure is a browser authorization that was offered but never + * finished — a standing operator action, not a dead server. + */ + authPending?: boolean; + }; export interface MCPConnectOptions { stderr?: "inherit" | "ignore" | "pipe"; onAuthURL?: (serverName: string, authorizationUrl: string) => void; @@ -165,20 +174,27 @@ export function setBrowserAuthWaitMs(ms: number): void { browserAuthWaitMs = ms; } +/** + * Browser authorization was offered but never finished — the wait timed out + * or hit the attempt cap. The TUI keeps the prompt-box auth marker for these + * instead of painting a generic connect-failure row. + */ +export class BrowserAuthPendingError extends Error {} + function browserAuthCapError(serverName: string): Error { const minutes = Math.round(BROWSER_AUTH_COOLDOWN_MS / 60_000); const attempts = MAX_BROWSER_AUTH_ATTEMPTS === 1 ? "1 attempt" : `${String(MAX_BROWSER_AUTH_ATTEMPTS)} attempts`; - return new Error( + return new BrowserAuthPendingError( `MCP authorization for ${serverName} failed after ${attempts}; ` + `retrying paused for ${minutes} minutes. Retry later after the cooldown.`, ); } function browserAuthWaitError(serverName: string): Error { - return new Error( + return new BrowserAuthPendingError( `MCP authorization for ${serverName} timed out waiting for the browser; ` + `the server is disconnected. Retry later after the cooldown.`, ); @@ -735,6 +751,7 @@ async function connectHttp( ok: false, serverName: config.name, error: err instanceof Error ? err.message : String(err), + ...(err instanceof BrowserAuthPendingError ? { authPending: true } : {}), }; } } diff --git a/src/tui/product-host.ts b/src/tui/product-host.ts index f75350331..3b1a9417b 100644 --- a/src/tui/product-host.ts +++ b/src/tui/product-host.ts @@ -477,8 +477,15 @@ export async function mountProductHost( if (disposed) return; const parsed = mcpServerState(state); if (parsed === null) return; - if (parsed.state === "needs-auth") mcpUnauthorized.add(parsed.name); - else mcpUnauthorized.delete(parsed.name); + // An auth wait that timed out is still waiting on the operator — keep + // the marker until the server connects, leaves config, or fails for a + // reason that is not the authorization itself. + if ( + parsed.state === "needs-auth" || + (parsed.state === "failed" && parsed.authPending === true) + ) { + mcpUnauthorized.add(parsed.name); + } else mcpUnauthorized.delete(parsed.name); setMcpNeedsAuth(shell, [...mcpUnauthorized]); show(mcpNotice(parsed)); } diff --git a/src/tui/runtime-channels.test.ts b/src/tui/runtime-channels.test.ts index 0dd978608..e11a7cb96 100644 --- a/src/tui/runtime-channels.test.ts +++ b/src/tui/runtime-channels.test.ts @@ -121,6 +121,32 @@ describe("mcp.status channel", () => { } }); + test("an auth wait that timed out keeps the prompt-box mark and paints no row", async () => { + const { host, emitter, frame, cleanup } = await mountHeadless(); + try { + emitter.emit("mcp.status", { + name: "granola", + state: "needs-auth", + url: "https://mcp.test/auth", + }); + expect(await frame()).toContain("mcp !"); + + emitter.emit("mcp.status", { + name: "granola", + state: "failed", + error: "timed out waiting for the browser", + authPending: true, + }); + const painted = await frame(); + expect(painted).toContain("mcp !"); + expect(host.shell.mcpNeedsAuth).toEqual(["granola"]); + expect(host.shell.streamLog).toEqual([]); + expect(host.shell.statusFlash ?? "").not.toContain("did not connect"); + } finally { + cleanup(); + } + }); + test("connected clears the standing auth mark from state and the painted frame", async () => { const { host, emitter, frame, cleanup } = await mountHeadless(); try { diff --git a/src/tui/runtime-notices.test.ts b/src/tui/runtime-notices.test.ts index 56dcb0b97..6577c66b1 100644 --- a/src/tui/runtime-notices.test.ts +++ b/src/tui/runtime-notices.test.ts @@ -111,6 +111,17 @@ describe("mcpNotice", () => { expect(notice?.text).toContain("its tools are unavailable"); }); + test("an unfinished browser authorization stays on the marker, not a row", () => { + expect( + mcpNotice({ + name: "linear", + state: "failed", + error: "timed out waiting for the browser", + authPending: true, + }), + ).toBeNull(); + }); + test("disconnected is not news — the operator chose it", () => { expect(mcpNotice({ name: "linear", state: "disconnected" })).toBeNull(); }); @@ -163,6 +174,14 @@ describe("payload validation", () => { "disconnected", ); expect(mcpServerState({ name: "a", state: "needs-auth" })).toBeNull(); + expect( + mcpServerState({ + name: "a", + state: "failed", + error: "x", + authPending: true, + }), + ).toMatchObject({ state: "failed", authPending: true }); expect(mcpServerState("nope")).toBeNull(); }); diff --git a/src/tui/runtime-notices.ts b/src/tui/runtime-notices.ts index 6f8e267af..bc3e3578e 100644 --- a/src/tui/runtime-notices.ts +++ b/src/tui/runtime-notices.ts @@ -99,6 +99,9 @@ export function mcpNotice(state: MCPServerState): RuntimeNotice | null { case "disconnected": return null; case "failed": + // An unfinished browser authorization is the same standing condition + // as needs-auth — the prompt-box marker and /mcp own it, not a row. + if (state.authPending === true) return null; return { kind: "row", text: `mcp ${state.name} did not connect (${state.error}) — its tools are unavailable; /mcp for detail`, @@ -164,7 +167,12 @@ export function lifecycleHookEvent(raw: unknown): LifecycleHookEvent | null { const mcpState = type({ name: "string", state: "'connecting'" }) .or({ name: "string", state: "'needs-auth'", url: "string" }) .or({ name: "string", state: "'connected'", tools: "string[]" }) - .or({ name: "string", state: "'failed'", error: "string" }) + .or({ + name: "string", + state: "'failed'", + error: "string", + "authPending?": "boolean", + }) .or({ name: "string", state: "'disconnected'" }); export function mcpServerState(raw: unknown): MCPServerState | null { From d92e3d3fc86c9c593efe67d501432351897285fe Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 11 Sep 2026 22:28:15 -0700 Subject: [PATCH 2/2] Add tests for the MCP auth-marker clearing and retry-copy transitions --- src/tui/command-surfaces.test.ts | 25 +++++++++++++++++++++++++ src/tui/runtime-channels.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/tui/command-surfaces.test.ts b/src/tui/command-surfaces.test.ts index d4ff6d6c6..cf55f18d3 100644 --- a/src/tui/command-surfaces.test.ts +++ b/src/tui/command-surfaces.test.ts @@ -1975,6 +1975,31 @@ describe("mcp surface", () => { }); }); + test("a timed-out authorization failure still offers Enter-retry copy", async () => { + await withWiredShell(async (shell, harness) => { + openCommandSurface(shell, "mcp", { + notify: () => undefined, + mcp: { + // Short timeout wording so the two-line describe zone has room + // left for the impact line — a `what` that wraps to both lines + // crowds `impact` out by design (see describeZoneLines). + list: () => [ + { + name: "granola", + state: "failed", + error: "timed out waiting for the browser", + }, + ], + openAuthURL: () => undefined, + }, + }); + await harness.renderOnce(); + const frame = harness.captureCharFrame(); + expect(frame).toContain("granola — failed"); + expect(frame).toContain("Enter retries"); + }); + }); + test("Alt+R confirms before removing a custom server", async () => { await withWiredShell(async (shell, harness) => { const removed: string[] = []; diff --git a/src/tui/runtime-channels.test.ts b/src/tui/runtime-channels.test.ts index e11a7cb96..d3c592365 100644 --- a/src/tui/runtime-channels.test.ts +++ b/src/tui/runtime-channels.test.ts @@ -169,6 +169,30 @@ describe("mcp.status channel", () => { } }); + test("an ordinary failure after needs-auth clears the standing auth mark", async () => { + const { host, emitter, frame, cleanup } = await mountHeadless(); + try { + emitter.emit("mcp.status", { + name: "granola", + state: "needs-auth", + url: "https://mcp.test/auth", + }); + expect(await frame()).toContain("mcp !"); + + emitter.emit("mcp.status", { + name: "granola", + state: "failed", + error: "ECONNREFUSED", + }); + const painted = await frame(); + expect(host.shell.mcpNeedsAuth).toEqual([]); + expect(painted).not.toContain("mcp !"); + expect(host.shell.statusFlash).toContain("mcp granola did not connect"); + } finally { + cleanup(); + } + }); + test("a failed connect keeps the landing mountain and rides the notice strip (CL-5600)", async () => { // Full product-host path: mcp.status → mcpNotice → surfaceSystemNotice. // The unit landing suite covers surfaceSystemNotice alone; this locks the