From 89d26e0ce8cb2d5d999dd5cc1b09cfe466e86b5f Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 16 Oct 2026 13:28:54 -0700 Subject: [PATCH 1/3] refactor(mcp): let the SDK finish HTTP auth and SSE reconnect Drop the extra probe on the ordinary path, finish auth on the transport that already dialed, and remove MCPClient.call. After an OAuth error that arrives before redirect, wipe credentials and dial once: a silent connect lets the real client retry, and a redirect prompts once. --- docs/MCP.md | 21 ++-- e2e/mcp-late-dispatch.test.ts | 14 +-- e2e/subagent-permission.test.ts | 4 - src/mcp/client-auth-policy.test.ts | 17 ++- src/mcp/client-auth-reauth-cap.test.ts | 120 +++++++++++++----- src/mcp/client-envelope.test.ts | 19 --- src/mcp/client.ts | 163 ++++++++++++++----------- src/mcp/oauth-provider.ts | 10 +- src/mcp/plugin.test.ts | 14 +-- src/tools/web-fetch.test.ts | 1 - src/tools/web-search.test.ts | 1 - testkit/mcp-connect-mock.ts | 8 -- 12 files changed, 218 insertions(+), 174 deletions(-) diff --git a/docs/MCP.md b/docs/MCP.md index 39842675f..9f556dde6 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -130,14 +130,19 @@ list is in effect, these actions edit that local file; add remains blocked. ## Dropped Transports Reconnect in the Background -When a connected server's transport dies unexpectedly, the row goes -**reconnecting** instead of failed: the redial attempt and the retained tool -count stay on the row (for example, `acme — reconnecting · attempt 3 · 2 -tools`), and backoff redials behind the scenes with a jittered delay that -grows per attempt and caps at 30 seconds. While reconnecting, the server's -tools stay mounted but fail fast — calling one returns "MCP server is -reconnecting; retry the call once it reports connected." rather than hanging -or dispatching against a dead transport. +Stdio and HTTP do not share a redial. A stdio child that exits is respawned +here: the row goes **reconnecting** instead of failed, the redial attempt and +the retained tool count stay on the row (for example, `acme — reconnecting · +attempt 3 · 2 tools`), and backoff redials behind the scenes with a jittered +delay that grows per attempt and caps at 30 seconds. While that redial is in +progress, the server's tools stay mounted but fail fast — calling one returns +"MCP server is reconnecting; retry the call once it reports connected." rather +than hanging or dispatching against a dead transport. + +An HTTP SSE drop does not start that second dial. The Streamable HTTP +transport resumes the stream itself (`reconnectionOptions`: 1s start, doubling, +30s cap). The row stays connected and tools stay live; only a real stdio death +uses the fail-fast stubs and attempt count above. Pressing Enter on a failed or reconnecting row retries now with a single dial instead of waiting for backoff, reusing the saved settings row — it never diff --git a/e2e/mcp-late-dispatch.test.ts b/e2e/mcp-late-dispatch.test.ts index ad2a9b8e4..9b7ae1b49 100644 --- a/e2e/mcp-late-dispatch.test.ts +++ b/e2e/mcp-late-dispatch.test.ts @@ -43,8 +43,13 @@ function permissionGate() { function linearClient(opts: { inputSchema?: MCPClient["tools"][number]["inputSchema"]; - call?: MCPClient["call"]; + call?: ( + toolName: string, + args: Record, + signal: AbortSignal, + ) => Promise; }): MCPClient { + const invoke = opts.call ?? (async () => "ISSUE-1"); return { serverName: "linear", tools: [ @@ -54,13 +59,8 @@ function linearClient(opts: { inputSchema: opts.inputSchema ?? LATE_MCP_SCHEMA, }, ], - call: opts.call ?? (async () => "ISSUE-1"), async callResult(toolName, args, signal) { - const text = await (opts.call ?? (async () => "ISSUE-1"))( - toolName, - args, - signal, - ); + const text = await invoke(toolName, args, signal); return { blocks: [{ type: "text", text }], isError: false, diff --git a/e2e/subagent-permission.test.ts b/e2e/subagent-permission.test.ts index 698b2379e..eb1eda418 100644 --- a/e2e/subagent-permission.test.ts +++ b/e2e/subagent-permission.test.ts @@ -296,7 +296,6 @@ test.serial( inputSchema: { type: "object", properties: {} }, }, ], - call: async () => "changed", callResult: async () => { calls++; return { @@ -354,7 +353,6 @@ test.serial( inputSchema: { type: "object", properties: {} }, }, ], - call: async () => "changed", callResult: async () => { calls++; return { @@ -488,7 +486,6 @@ test.serial( inputSchema: { type: "object", properties: {} }, }, ], - call: async () => "changed", callResult: async () => { calls++; return { @@ -573,7 +570,6 @@ test("storing parent-gated MCP tools then wrapping again denies without requestA inputSchema: { type: "object", properties: {} }, }, ], - call: async () => "changed", callResult: async () => { calls++; return { diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index 0d71fc5ce..c1a29705b 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -143,7 +143,8 @@ await mockMcpOAuthProviderModule(async (options: { serverURL: string }) => { return authProvider; }); -const { connectMCPServer, fetchWithConnectAbort } = await import("./client.js"); +const { connectMCPServer, fetchWithConnectAbort, MCP_HTTP_RECONNECTION_OPTIONS } = + await import("./client.js"); describe("HTTP MCP auth policy", () => { beforeEach(() => { @@ -186,7 +187,9 @@ describe("HTTP MCP auth policy", () => { expect(providerCreates).toBe(0); // No signal and no provider: leave redirectPolicy unset so the SDK // default `same-origin` applies. Do not pin `follow`. - expect(transportOptions).toEqual([{}]); + expect(transportOptions).toEqual([ + { reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS }, + ]); }); test("closes a bound OAuth callback server when provider setup fails", async () => { @@ -262,13 +265,14 @@ describe("HTTP MCP auth policy", () => { if (!result.ok) return; expect(transportOptions).toEqual([ { + reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS, authProvider, requestInit: { signal: expect.any(AbortSignal) }, fetch: expect.any(Function), }, ]); - const call = result.client.call("ping", {}, abort.signal); + const call = result.client.callResult("ping", {}, abort.signal); while (tokenRefreshSignals.length === 0) await Promise.resolve(); expect(tokenRefreshSignals[0]).toBeDefined(); expect(tokenRefreshSignals[0]?.aborted).toBe(false); @@ -298,6 +302,7 @@ describe("HTTP MCP auth policy", () => { expect(providerServerURL).toBe("https://custom.example/mcp?mode=full"); expect(transportOptions).toEqual([ { + reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS, authProvider, requestInit: { signal: expect.any(AbortSignal) }, fetch: expect.any(Function), @@ -322,7 +327,11 @@ describe("HTTP MCP auth policy", () => { expect(transport.requestInit?.signal).not.toBe(connectAbort.signal); expect(transport.fetch).toBeTypeOf("function"); - const call = result.client.call("ping", {}, new AbortController().signal); + const call = result.client.callResult( + "ping", + {}, + new AbortController().signal, + ); while (tokenRefreshSignals.length === 0) await Promise.resolve(); expect(tokenRefreshSignals[0]).not.toBe(connectAbort.signal); expect(tokenRefreshSignals[0]?.aborted).toBe(false); diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index ea18d32a1..0fc4dbcb1 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -7,6 +7,7 @@ import { test, } from "bun:test"; import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; +import { InvalidScopeError } from "@modelcontextprotocol/sdk/server/auth/errors.js"; import { mockMcpCallbackServerModule, mockMcpClientModule, @@ -19,6 +20,9 @@ interface MockState { finishAuthCalls: number; finishAuthError: Error | undefined; connectFailuresLeft: number; + connectCalls: number; + /** Thrown once, before any redirect, then cleared. */ + connectOAuthError: Error | undefined; listFailuresLeft: number; callFailuresLeft: number; callToolCalls: number; @@ -31,6 +35,7 @@ interface MockState { redirectVerifier: string | undefined; providerCreates: number; refreshCalls: number; + resetCalls: number; refreshSucceeds: boolean; authEvents: string[]; authURLCount: number; @@ -57,6 +62,8 @@ function freshMockState(): MockState { finishAuthCalls: 0, finishAuthError: new Error("finishAuth exploded"), connectFailuresLeft: 0, + connectCalls: 0, + connectOAuthError: undefined, listFailuresLeft: 0, callFailuresLeft: 0, callToolCalls: 0, @@ -69,6 +76,7 @@ function freshMockState(): MockState { redirectVerifier: undefined, providerCreates: 0, refreshCalls: 0, + resetCalls: 0, refreshSucceeds: false, authEvents: [], authURLCount: 0, @@ -95,7 +103,9 @@ function freshMockState(): MockState { const mock = freshMockState(); const fakeProvider = { - resetAuthorization: async () => undefined, + resetAuthorization: async () => { + mock.resetCalls += 1; + }, tokens: () => ({ access_token: "stale", refresh_token: "refresh-me" }), refreshToken: async () => { mock.refreshCalls += 1; @@ -184,6 +194,12 @@ function waitForGate(signal: AbortSignal | undefined): Promise { await mockMcpClientModule(mock, { connect: async (provider) => { + mock.connectCalls += 1; + if (mock.connectOAuthError !== undefined) { + const error = mock.connectOAuthError; + mock.connectOAuthError = undefined; + throw error; + } if (mock.connectFailuresLeft > 0) { mock.connectFailuresLeft -= 1; await emitRedirects(provider); @@ -254,6 +270,7 @@ const { connectMCPServer, resetBrowserAuthState, setBrowserAuthWaitMs, + unwrapToolContent, MAX_BROWSER_AUTH_ATTEMPTS, BROWSER_AUTH_COOLDOWN_MS, } = await import("./client.js"); @@ -269,6 +286,17 @@ type ConnectedClient = Extract< { ok: true } >; +function callText( + client: ConnectedClient["client"], + toolName: string, + args: Record, + signal: AbortSignal, +): Promise { + return client + .callResult(toolName, args, signal) + .then((envelope) => unwrapToolContent(envelope.blocks)); +} + async function connectWithAuthPrompt(): Promise<{ ok: boolean; error?: string; @@ -301,14 +329,14 @@ async function runCappedEpisodes( for (let episode = 0; episode < MAX_BROWSER_AUTH_ATTEMPTS; episode += 1) { mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).rejects.toThrow("finishAuth exploded"); } expect(mock.authURLCount).toBe(authURLsBefore + MAX_BROWSER_AUTH_ATTEMPTS); mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).rejects.toThrow("retrying paused"); expect(mock.authURLCount).toBe(authURLsBefore + MAX_BROWSER_AUTH_ATTEMPTS); } @@ -363,7 +391,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.listFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).rejects.toThrow("authorization required"); expect(mock.refreshCalls).toBe(1); @@ -380,18 +408,17 @@ describe("HTTP MCP re-auth loop prevention", () => { }); expect(connected.ok).toBe(true); if (!connected.ok) return; - mock.redirectsPerFailure = 0; mock.callFailuresLeft = 2; - mock.listFailuresLeft = 1; - mock.redirectOnListFailure = true; const calls = [ - connected.client.call( + callText( + connected.client, "first", { value: 1 }, new AbortController().signal, ), - connected.client.call( + callText( + connected.client, "second", { value: 2 }, new AbortController().signal, @@ -399,7 +426,8 @@ describe("HTTP MCP re-auth loop prevention", () => { ]; await expect(Promise.all(calls)).resolves.toEqual(["", ""]); - expect(mock.refreshCalls).toBe(1); + // SDK already redirected, so recovery must not refresh again. + expect(mock.refreshCalls).toBe(0); expect(mock.authURLCount).toBe(1); expect(mock.waitForCodeCalls).toBe(1); expect(mock.finishAuthCalls).toBe(1); @@ -418,7 +446,7 @@ describe("HTTP MCP re-auth loop prevention", () => { }); mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const first = connected.client.call( + const first = callText(connected.client, "first", {}, new AbortController().signal, @@ -427,7 +455,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.redirectsPerFailure = 1; mock.callFailuresLeft = 1; - const second = connected.client.call( + const second = callText(connected.client, "second", {}, new AbortController().signal, @@ -456,8 +484,8 @@ describe("HTTP MCP re-auth loop prevention", () => { if (!connected.ok) return; mock.callFailuresLeft = 2; const firstAbort = new AbortController(); - const first = connected.client.call("first", {}, firstAbort.signal); - const second = connected.client.call( + const first = callText(connected.client, "first", {}, firstAbort.signal); + const second = callText(connected.client, "second", {}, new AbortController().signal, @@ -487,7 +515,7 @@ describe("HTTP MCP re-auth loop prevention", () => { if (!connected.ok) return; mock.callFailuresLeft = 1; const abort = new AbortController(); - const call = connected.client.call("ping", {}, abort.signal); + const call = callText(connected.client, "ping", {}, abort.signal); while (mock.authURLCount === 0 || mock.waitForCodeCalls === 0) await Promise.resolve(); @@ -516,7 +544,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).rejects.toThrow("finishAuth exploded"); expect(mock.authURLCount).toBe(1); expect(mock.authorizedCount).toBe(0); @@ -526,7 +554,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).resolves.toBe(""); expect(mock.authorizedCount).toBe(1); expect(mock.authURLCount).toBe(1); @@ -548,7 +576,7 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(connected.ok).toBe(true); if (!connected.ok) return; mock.callFailuresLeft = 1; - const call = connected.client.call( + const call = callText(connected.client, "ping", {}, new AbortController().signal, @@ -570,7 +598,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.refreshGate = new Promise(() => undefined); mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const call = connected.client.call( + const call = callText(connected.client, "ping", {}, new AbortController().signal, @@ -594,7 +622,7 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(connected.ok).toBe(true); if (!connected.ok) return; mock.callFailuresLeft = 1; - const call = connected.client.call( + const call = callText(connected.client, "ping", {}, new AbortController().signal, @@ -622,12 +650,12 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.releaseRetry = resolve; }); mock.callFailuresLeft = 2; - const first = connected.client.call( + const first = callText(connected.client, "first", {}, new AbortController().signal, ); - const second = connected.client.call( + const second = callText(connected.client, "second", {}, new AbortController().signal, @@ -638,7 +666,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.refreshSucceeds = true; mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const third = connected.client.call( + const third = callText(connected.client, "third", {}, new AbortController().signal, @@ -667,6 +695,36 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(mock.authURLCount).toBe(1); }); + test("InvalidScopeError before redirect still prompts once and retries", async () => { + mock.finishAuthError = undefined; + mock.connectOAuthError = new InvalidScopeError("scope not allowed"); + mock.connectFailuresLeft = 1; + + const result = await connectWithAuthPrompt(); + + expect(result.ok).toBe(true); + expect(mock.resetCalls).toBe(1); + expect(mock.authURLCount).toBe(1); + expect(mock.waitForCodeCalls).toBe(1); + expect(mock.finishAuthCalls).toBe(1); + expect(mock.refreshCalls).toBe(0); + expect(mock.authEvents).toEqual(["authURL"]); + }); + + test("InvalidScopeError with a silent post-wipe connect retries the real client", async () => { + mock.connectOAuthError = new InvalidScopeError("scope not allowed"); + + const result = await connectWithAuthPrompt(); + + expect(result.ok).toBe(true); + expect(result).not.toHaveProperty("error"); + expect(mock.connectCalls).toBe(3); + expect(mock.resetCalls).toBe(1); + expect(mock.authURLCount).toBe(0); + expect(mock.waitForCodeCalls).toBe(0); + expect(mock.finishAuthCalls).toBe(0); + }); + test("reconnect during in-flight waitForCode does not emit a second prompt", async () => { mock.connectFailuresLeft = Number.POSITIVE_INFINITY; mock.callbackGate = new Promise(() => undefined); @@ -715,7 +773,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).rejects.toThrow("finishAuth exploded"); expect(mock.authURLCount).toBe(1); @@ -810,7 +868,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).resolves.toBe(""); expect(mock.authURLCount).toBe(1); @@ -837,7 +895,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).resolves.toBe(""); expect(mock.authURLCount).toBe(1); @@ -864,7 +922,7 @@ describe("HTTP MCP re-auth loop prevention", () => { }); mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const first = connected.client.call( + const first = callText(connected.client, "first", {}, new AbortController().signal, @@ -887,7 +945,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.redirectsPerFailure = 1; mock.callFailuresLeft = 1; await expect( - connected.client.call("second", {}, new AbortController().signal), + callText(connected.client, "second", {}, new AbortController().signal), ).resolves.toBe(""); expect(mock.authURLCount).toBe(1); @@ -912,7 +970,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callFailuresLeft = 2; mock.callRedirectsLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).rejects.toThrow("authorization required"); expect(mock.authorizedCount).toBe(0); expect(mock.authURLCount).toBe(1); @@ -923,7 +981,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callRedirectsLeft = Number.POSITIVE_INFINITY; mock.callFailuresLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).rejects.toThrow("retrying paused"); expect(mock.authURLCount).toBe(1); expect(mock.authorizedCount).toBe(0); @@ -945,7 +1003,7 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callFailuresLeft = 1; mock.callRedirectsLeft = 1; await expect( - connected.client.call("ping", {}, new AbortController().signal), + callText(connected.client, "ping", {}, new AbortController().signal), ).resolves.toBe(""); expect(mock.authorizedCount).toBe(1); expect(mock.authURLCount).toBe(1); diff --git a/src/mcp/client-envelope.test.ts b/src/mcp/client-envelope.test.ts index 38dc3a819..86a8735cd 100644 --- a/src/mcp/client-envelope.test.ts +++ b/src/mcp/client-envelope.test.ts @@ -50,23 +50,4 @@ describe("mcp client tool envelope", () => { expect(envelope.structuredContent).toEqual({ reason: "bad input" }); await connected.client.close(); }); - - test("legacy call still flattens text blocks", async () => { - scriptedCallToolResult = { - content: [{ type: "text", text: "hello" }], - }; - const connected = await connectMCPServer( - { name: "envelope", command: "true" }, - {}, - ); - if (!connected.ok) throw new Error("expected stdio connect to succeed"); - const text = await connected.client.call( - "do_thing", - {}, - new AbortController().signal, - ); - - expect(text).toBe("hello"); - await connected.client.close(); - }); }); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 899126cfe..df5e90bfe 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -56,11 +56,6 @@ export interface MCPToolResultEnvelope { export interface MCPClient { serverName: string; tools: MCPTool[]; - call( - toolName: string, - args: Record, - signal: AbortSignal, - ): Promise; /** Full tool-result envelope: blocks plus tool-level * isError/structuredContent. */ callResult( @@ -139,7 +134,7 @@ interface HTTPAuthContext { url: URL; authProvider: CorbitsOAuthProvider; callback: CallbackServer; - coordinator: HTTPAuthCoordinator; + coordinator: SdkAuthFinish; interactive: boolean; serverName: string; onAuthorized?: (serverName: string) => void; @@ -150,7 +145,7 @@ interface BrowserAuthFlow { promptEmitted: Promise; } -interface HTTPAuthCoordinator { +interface SdkAuthFinish { lifecycle: AbortController; inFlight?: Promise; refreshInFlight?: Promise; @@ -161,7 +156,8 @@ interface HTTPAuthCoordinator { notified?: boolean; recoveryGeneration?: number; waiters?: number; - probe(): Promise; + /** Transport the SDK already dialed. finishAuth reuses it; no second client. */ + transport?: StreamableHTTPClientTransport; } function isRecoverableAuthError( @@ -275,7 +271,7 @@ function beginBrowserAuth(context: HTTPAuthContext): { clear(): void } { }; } -function unfreezePkce(coordinator: HTTPAuthCoordinator): void { +function unfreezePkce(coordinator: SdkAuthFinish): void { coordinator.pkceFrozen = false; delete coordinator.pkceSavePromise; } @@ -390,6 +386,19 @@ export function fetchWithConnectAbort( }; } +/** + * SDK SSE resume: 1s start, double each attempt, 30s cap. Same envelope as + * stdio `mcpReconnectDelayMs` without a random factor — the SDK option has + * no jitter hook. maxRetries stays finite so a dead stream cannot pin the + * process forever; stdio death still redials in the owner loop. + */ +export const MCP_HTTP_RECONNECTION_OPTIONS = { + initialReconnectionDelay: 1_000, + maxReconnectionDelay: 30_000, + reconnectionDelayGrowFactor: 2, + maxRetries: 1_000, +} as const; + export function streamableHTTPTransportOptions( authProvider: CorbitsOAuthProvider | undefined, signal: AbortSignal | undefined, @@ -398,6 +407,7 @@ export function streamableHTTPTransportOptions( // also wraps OAuth discovery and token requests, so a 307/308 can post // refresh tokens or auth codes to another host. return { + reconnectionOptions: MCP_HTTP_RECONNECTION_OPTIONS, ...(authProvider === undefined ? {} : { authProvider }), ...(signal === undefined ? {} @@ -426,6 +436,38 @@ export async function retryAfterInteractiveAuth( } } +async function dialForFreshAuthorization( + context: HTTPAuthContext, +): Promise { + // InvalidScope / access_denied / invalid_request / unsupported_grant arrive + // before the SDK calls redirectToAuthorization. resetAuthorization already + // dropped the bad client, so this dial is the same attempt's browser path: + // the SDK registers again and redirects. A separate client avoids + // "Already connected" on the transport that just failed. + const coordinator = context.coordinator; + const probeClient = new Client({ + name: MCP_CLIENT_NAME, + version: "1.0.0", + }); + const transport = new StreamableHTTPClientTransport( + context.url, + streamableHTTPTransportOptions( + context.authProvider, + coordinator.lifecycle.signal, + ), + ); + try { + await probeClient.connect(transport as unknown as Transport, { + signal: coordinator.lifecycle.signal, + }); + } catch (probeErr) { + if (!isRecoverableAuthError(probeErr)) throw probeErr; + if (coordinator.browserFlow === undefined) throw probeErr; + } finally { + await closeQuietly(() => probeClient.close()); + } +} + async function driveRecovery( err: UnauthorizedError | OAuthError, context: HTTPAuthContext, @@ -433,37 +475,41 @@ async function driveRecovery( const coordinator = context.coordinator; if (err instanceof OAuthError) await context.authProvider.resetAuthorization(); + // The SDK already tried the token, refreshed, and redirected before throwing. + // A 401 that arrived without that redirect gets one shared refresh; anything + // else finishes the browser flow the provider already started. + let refreshed = false; if ( err instanceof UnauthorizedError && coordinator.browserFlow === undefined ) { - await getOrStartRefresh(context); - // SDK redirects waiting on this refresh must reserve the browser flow - // before the probe. + refreshed = await getOrStartRefresh(context); + // Redirects waiting on this refresh must reserve the browser flow + // before we decide the episode failed. await Promise.resolve(); } - if (coordinator.browserFlow === undefined) { - try { - await coordinator.probe(); - return; - } catch (probeErr) { - if (!isRecoverableAuthError(probeErr)) throw probeErr; - if (coordinator.browserFlow === undefined) throw probeErr; - } + // Credential wipe without a redirect logs the user out. Continue this + // attempt into the browser flow instead of failing the connect. A dial + // that succeeds with no redirect is the old probe success: return so the + // real client retries instead of rethrowing the wiped error. + if (err instanceof OAuthError && coordinator.browserFlow === undefined) { + await dialForFreshAuthorization(context); + if (coordinator.browserFlow === undefined) return; } const browserFlow = coordinator.browserFlow; + if (browserFlow === undefined) { + if (refreshed) return; + throw err; + } + await browserFlow.promptEmitted; const code = await waitForBrowserAuthCode(context); - await new StreamableHTTPClientTransport( - context.url, - streamableHTTPTransportOptions( - context.authProvider, - coordinator.lifecycle.signal, - ), - ).finishAuth(code); - await coordinator.probe(); + const transport = coordinator.transport; + if (transport === undefined) + throw new Error("no live HTTP transport to finish authorization"); + await transport.finishAuth(code); } function completeVerifiedRecovery( @@ -620,11 +666,6 @@ async function finishClient( } return envelope; }, - async call(this: MCPClient, toolName, args, signal) { - return unwrapToolContent( - (await this.callResult(toolName, args, signal)).blocks, - ); - }, async close() { // Disarm first: the owner's hook reports unintentional death only. if (liveTransport !== undefined) liveTransport.onclose = () => undefined; @@ -684,9 +725,16 @@ async function connectStdio( transport, options.signal === undefined ? undefined : { signal: options.signal }, ); - // A dead child surfaces here; intentional close() disarms it first. - if (options.onDisconnect !== undefined) - transport.onclose = options.onDisconnect; + // Child exit is the only death the owner redials. Chain the protocol + // handler so in-flight requests still reject; HTTP SSE resume stays + // inside the SDK and must not start a second dial. + if (options.onDisconnect !== undefined) { + const prior = transport.onclose; + transport.onclose = () => { + prior?.(); + options.onDisconnect?.(); + }; + } return { ok: true, client: await finishClient( @@ -750,26 +798,16 @@ async function connectHttp( onAuthorizationState: callback.expectState, fetchFn: fetchWithConnectAbort(lifecycle.signal), }); - makeTransport = () => - new StreamableHTTPClientTransport( + const coordinator: SdkAuthFinish = { + lifecycle, + }; + makeTransport = () => { + const transport = new StreamableHTTPClientTransport( url, streamableHTTPTransportOptions(authProvider, lifecycle.signal), - ) as unknown as Transport; - const coordinator: HTTPAuthCoordinator = { - lifecycle, - probe: async () => { - const probeClient = new Client({ - name: MCP_CLIENT_NAME, - version: "1.0.0", - }); - try { - await probeClient.connect(makeTransport(), { - signal: lifecycle.signal, - }); - } finally { - await closeQuietly(() => probeClient.close()); - } - }, + ); + coordinator.transport = transport; + return transport as unknown as Transport; }; authContext = { url, @@ -789,7 +827,7 @@ async function connectHttp( version: "1.0.0", }); client = connectedClient; - // The auth-recovery wrapper can dial twice; the last transport wins. + // Recovery may dial again after finishAuth; the last transport wins. let liveTransport: Transport | undefined; await withHTTPAuthorizationRecovery( authContext, @@ -802,15 +840,6 @@ async function connectHttp( }, lifecycle.signal, ); - // A dropped SSE stream surfaces here; intentional close() disarms it. - if (options.onDisconnect !== undefined && liveTransport !== undefined) - liveTransport.onclose = options.onDisconnect; - if (authContext !== undefined) { - authContext.coordinator.probe = () => - connectedClient - .listTools(undefined, { signal: lifecycle.signal }) - .then(() => undefined); - } return { ok: true, client: await finishClient( @@ -825,11 +854,7 @@ async function connectHttp( } catch (err) { closeLifecycle(); await closeQuietly(() => client?.close() ?? Promise.resolve()); - try { - callback?.close(); - } catch { - // Setup has already failed; callback teardown is best effort. - } + await closeQuietly(() => Promise.resolve(callback?.close())); return { ok: false, serverName: config.name, diff --git a/src/mcp/oauth-provider.ts b/src/mcp/oauth-provider.ts index c900cca3d..09bb6f2fc 100644 --- a/src/mcp/oauth-provider.ts +++ b/src/mcp/oauth-provider.ts @@ -22,6 +22,7 @@ import { type MCPAuthIdentity, type MCPAuthState, } from "./auth-store.js"; +import { isAbortError } from "./quiet.js"; import { MCP_CLIENT_NAME } from "../branding.js"; export interface OAuthProviderOptions { @@ -47,15 +48,6 @@ function redirectUrisInclude( return uris.includes(redirectUrl); } -function isAbortError(err: unknown): boolean { - return ( - typeof err === "object" && - err !== null && - "name" in err && - err.name === "AbortError" - ); -} - // DCR bakes in the loopback redirect_uri (ephemeral port); a new-port session // cannot reuse that client_id, so drop the stale registration when no tokens remain. function dropStaleClientRegistration( diff --git a/src/mcp/plugin.test.ts b/src/mcp/plugin.test.ts index e8e10202f..5e3d4ffa2 100644 --- a/src/mcp/plugin.test.ts +++ b/src/mcp/plugin.test.ts @@ -31,7 +31,6 @@ function fakeEnvelopeClient(envelope: ScriptedMcpEnvelope): MCPClient { inputSchema: { type: "object", properties: {} }, }, ], - call: async () => "", callResult: async () => ({ blocks: envelope.blocks, isError: envelope.isError === true, @@ -69,7 +68,6 @@ function fakeClient(reply: string): MCPClient { inputSchema: { type: "object", properties: {} }, }, ], - call: async () => reply, callResult: async () => ({ blocks: [{ type: "text", text: reply }], isError: false, @@ -573,7 +571,7 @@ describe("mcpClientToAgentTools", () => { expect(payloads.map((payload) => payload.isError)).toEqual([false, true]); }); - test("returns callResult text when call is also present", async () => { + test("returns callResult text", async () => { let calls = 0; const client: MCPClient = { serverName: "legacy", @@ -584,7 +582,6 @@ describe("mcpClientToAgentTools", () => { inputSchema: { type: "object", properties: {} }, }, ], - call: async () => "unused", callResult: async () => { calls++; return { @@ -620,9 +617,6 @@ describe("mcpClientToAgentTools", () => { inputSchema: { type: "object", properties: {} }, }, ], - call: async () => { - throw new Error("transport exploded"); - }, callResult: async () => { throw new Error("transport exploded"); }, @@ -681,9 +675,6 @@ function makeFakeClient(serverName: string, toolNames: string[]): MCPClient { description: `${name} tool`, inputSchema: { type: "object", properties: {} }, })), - async call() { - return "result"; - }, async callResult() { return { blocks: [{ type: "text", text: "result" }], @@ -710,9 +701,6 @@ describe("mcpClientToAgentTools (production gated path)", () => { inputSchema: { type: "object" }, }, ], - async call() { - return ""; - }, async callResult(toolName, args) { capturedName = toolName; capturedArgs = args; diff --git a/src/tools/web-fetch.test.ts b/src/tools/web-fetch.test.ts index df35d5f23..aab931542 100644 --- a/src/tools/web-fetch.test.ts +++ b/src/tools/web-fetch.test.ts @@ -149,7 +149,6 @@ describe("createExaMCPWebFetchTool", () => { tools: [ { name: "web_fetch_exa", description: "Fetch", inputSchema: {} }, ], - call: async () => "", callResult: async (name, args, signal) => ({ blocks: [{ type: "text", text: await call(name, args, signal) }], isError: false, diff --git a/src/tools/web-search.test.ts b/src/tools/web-search.test.ts index 3a6d94f7c..7eeb650a6 100644 --- a/src/tools/web-search.test.ts +++ b/src/tools/web-search.test.ts @@ -19,7 +19,6 @@ await withMockedModule( client: { serverName: config.name, tools: [], - call: async () => "", callResult: async ( toolName: string, args: Record, diff --git a/testkit/mcp-connect-mock.ts b/testkit/mcp-connect-mock.ts index 02cf5b39c..46ba62080 100644 --- a/testkit/mcp-connect-mock.ts +++ b/testkit/mcp-connect-mock.ts @@ -259,14 +259,6 @@ export async function installMcpConnectMock( client: { serverName: config.name, tools: resolveTools(mode), - call: async ( - toolName: string, - args: Record, - signal: AbortSignal, - ) => { - mock.calls.push({ toolName, args, signal }); - return mock.toolCallResult; - }, callResult: async ( toolName: string, args: Record, From 931a497c825856475e5f6b53befd987e7d89b779 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 16 Oct 2026 13:59:25 -0700 Subject: [PATCH 2/3] style(mcp): format auth recovery tests --- src/mcp/client-auth-policy.test.ts | 7 ++++-- src/mcp/client-auth-reauth-cap.test.ts | 30 +++++++++++++++++--------- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index c1a29705b..d83856747 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -143,8 +143,11 @@ await mockMcpOAuthProviderModule(async (options: { serverURL: string }) => { return authProvider; }); -const { connectMCPServer, fetchWithConnectAbort, MCP_HTTP_RECONNECTION_OPTIONS } = - await import("./client.js"); +const { + connectMCPServer, + fetchWithConnectAbort, + MCP_HTTP_RECONNECTION_OPTIONS, +} = await import("./client.js"); describe("HTTP MCP auth policy", () => { beforeEach(() => { diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index 0fc4dbcb1..f20a68014 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -446,7 +446,8 @@ describe("HTTP MCP re-auth loop prevention", () => { }); mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const first = callText(connected.client, + const first = callText( + connected.client, "first", {}, new AbortController().signal, @@ -455,7 +456,8 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.redirectsPerFailure = 1; mock.callFailuresLeft = 1; - const second = callText(connected.client, + const second = callText( + connected.client, "second", {}, new AbortController().signal, @@ -485,7 +487,8 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.callFailuresLeft = 2; const firstAbort = new AbortController(); const first = callText(connected.client, "first", {}, firstAbort.signal); - const second = callText(connected.client, + const second = callText( + connected.client, "second", {}, new AbortController().signal, @@ -576,7 +579,8 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(connected.ok).toBe(true); if (!connected.ok) return; mock.callFailuresLeft = 1; - const call = callText(connected.client, + const call = callText( + connected.client, "ping", {}, new AbortController().signal, @@ -598,7 +602,8 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.refreshGate = new Promise(() => undefined); mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const call = callText(connected.client, + const call = callText( + connected.client, "ping", {}, new AbortController().signal, @@ -622,7 +627,8 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(connected.ok).toBe(true); if (!connected.ok) return; mock.callFailuresLeft = 1; - const call = callText(connected.client, + const call = callText( + connected.client, "ping", {}, new AbortController().signal, @@ -650,12 +656,14 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.releaseRetry = resolve; }); mock.callFailuresLeft = 2; - const first = callText(connected.client, + const first = callText( + connected.client, "first", {}, new AbortController().signal, ); - const second = callText(connected.client, + const second = callText( + connected.client, "second", {}, new AbortController().signal, @@ -666,7 +674,8 @@ describe("HTTP MCP re-auth loop prevention", () => { mock.refreshSucceeds = true; mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const third = callText(connected.client, + const third = callText( + connected.client, "third", {}, new AbortController().signal, @@ -922,7 +931,8 @@ describe("HTTP MCP re-auth loop prevention", () => { }); mock.redirectsPerFailure = 0; mock.callFailuresLeft = 1; - const first = callText(connected.client, + const first = callText( + connected.client, "first", {}, new AbortController().signal, From 4ca8bb1c4865425df1a200aa47a40e0abb2575f0 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Fri, 16 Oct 2026 14:56:04 -0700 Subject: [PATCH 3/3] fix(mcp): exchange OAuth codes on the redirecting transport finishAuth was running on the pre-failure HTTP transport after a probe client built the authorize URL, so the code exchange could use a different resource than the redirect. A wiped session is replaced before the retry. --- docs/MCP.md | 6 +- src/mcp/client-auth-reauth-cap.test.ts | 152 ++++++++++++++++++++++++- src/mcp/client.ts | 61 +++++++--- src/mcp/quiet.ts | 6 +- testkit/mcp-sdk-mock.ts | 83 ++++++++++---- 5 files changed, 266 insertions(+), 42 deletions(-) diff --git a/docs/MCP.md b/docs/MCP.md index 9f556dde6..14ea76c01 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -141,8 +141,10 @@ than hanging or dispatching against a dead transport. An HTTP SSE drop does not start that second dial. The Streamable HTTP transport resumes the stream itself (`reconnectionOptions`: 1s start, doubling, -30s cap). The row stays connected and tools stay live; only a real stdio death -uses the fail-fast stubs and attempt count above. +30s cap). Those options have no random factor and no exhaustion callback, so +the delay is not jittered and a stream that stops retrying stays on the +connected row instead of flipping to failed. Tools stay live; only a real +stdio death uses the fail-fast stubs and attempt count above. Pressing Enter on a failed or reconnecting row retries now with a single dial instead of waiting for backoff, reusing the saved settings row — it never diff --git a/src/mcp/client-auth-reauth-cap.test.ts b/src/mcp/client-auth-reauth-cap.test.ts index f20a68014..1d0a38413 100644 --- a/src/mcp/client-auth-reauth-cap.test.ts +++ b/src/mcp/client-auth-reauth-cap.test.ts @@ -19,6 +19,27 @@ import { interface MockState { finishAuthCalls: number; finishAuthError: Error | undefined; + finishAuthInstanceId: number | undefined; + finishAuthResourceMetadataUrl: string | undefined; + finishAuthScope: string | undefined; + redirectingInstanceId: number | undefined; + preFailureResourceMetadataUrl: string | undefined; + preFailureScope: string | undefined; + challengeResourceMetadataUrl: string | undefined; + challengeScope: string | undefined; + boundTransport: + | { + instanceId: number; + resourceMetadataUrl?: string; + scope?: string; + sessionId?: string; + } + | undefined; + stampSessionId: string | undefined; + sessionStamped: boolean; + callSessionIds: (string | undefined)[]; + callOAuthError: Error | undefined; + callbackCloseThrows: boolean; connectFailuresLeft: number; connectCalls: number; /** Thrown once, before any redirect, then cleared. */ @@ -61,6 +82,20 @@ function freshMockState(): MockState { return { finishAuthCalls: 0, finishAuthError: new Error("finishAuth exploded"), + finishAuthInstanceId: undefined, + finishAuthResourceMetadataUrl: undefined, + finishAuthScope: undefined, + redirectingInstanceId: undefined, + preFailureResourceMetadataUrl: undefined, + preFailureScope: undefined, + challengeResourceMetadataUrl: undefined, + challengeScope: undefined, + boundTransport: undefined, + stampSessionId: undefined, + sessionStamped: false, + callSessionIds: [], + callOAuthError: undefined, + callbackCloseThrows: false, connectFailuresLeft: 0, connectCalls: 0, connectOAuthError: undefined, @@ -193,18 +228,44 @@ function waitForGate(signal: AbortSignal | undefined): Promise { } await mockMcpClientModule(mock, { - connect: async (provider) => { + connect: async (provider, transport) => { mock.connectCalls += 1; + if (transport !== undefined) mock.boundTransport = transport; if (mock.connectOAuthError !== undefined) { + if ( + transport !== undefined && + mock.preFailureResourceMetadataUrl !== undefined + ) { + transport.resourceMetadataUrl = mock.preFailureResourceMetadataUrl; + if (mock.preFailureScope !== undefined) + transport.scope = mock.preFailureScope; + } const error = mock.connectOAuthError; mock.connectOAuthError = undefined; throw error; } if (mock.connectFailuresLeft > 0) { mock.connectFailuresLeft -= 1; + if ( + transport !== undefined && + mock.challengeResourceMetadataUrl !== undefined + ) { + transport.resourceMetadataUrl = mock.challengeResourceMetadataUrl; + if (mock.challengeScope !== undefined) + transport.scope = mock.challengeScope; + mock.redirectingInstanceId = transport.instanceId; + } await emitRedirects(provider); throw new UnauthorizedError("authorization required"); } + if ( + transport !== undefined && + mock.stampSessionId !== undefined && + !mock.sessionStamped + ) { + transport.sessionId = mock.stampSessionId; + mock.sessionStamped = true; + } }, listTools: async () => { if (mock.listFailuresLeft > 0) { @@ -218,8 +279,23 @@ await mockMcpClientModule(mock, { } return { tools: [] }; }, - callTool: async () => { + callTool: async (sessionId) => { mock.callToolCalls += 1; + mock.callSessionIds.push(sessionId); + if (mock.callOAuthError !== undefined) { + if ( + mock.boundTransport !== undefined && + mock.preFailureResourceMetadataUrl !== undefined + ) { + mock.boundTransport.resourceMetadataUrl = + mock.preFailureResourceMetadataUrl; + if (mock.preFailureScope !== undefined) + mock.boundTransport.scope = mock.preFailureScope; + } + const error = mock.callOAuthError; + mock.callOAuthError = undefined; + throw error; + } if (mock.callFailuresLeft > 0) { mock.callFailuresLeft -= 1; if (mock.callRedirectsLeft > 0) { @@ -237,6 +313,9 @@ await mockMcpClientModule(mock, { await mockMcpTransportModule(mock, { finishAuth: async (self) => { mock.finishAuthCalls += 1; + mock.finishAuthInstanceId = self.instanceId; + mock.finishAuthResourceMetadataUrl = self.resourceMetadataUrl; + mock.finishAuthScope = self.scope; mock.exchangedCodeVerifier = self.provider?.codeVerifier?.(); if (mock.finishAuthError !== undefined) throw mock.finishAuthError; }, @@ -248,7 +327,9 @@ await mockMcpCallbackServerModule({ await waitForGate(signal); return "code"; }, - close: () => undefined, + close: () => { + if (mock.callbackCloseThrows) throw new Error("callback close threw"); + }, }); await mockMcpOAuthProviderModule( @@ -734,6 +815,71 @@ describe("HTTP MCP re-auth loop prevention", () => { expect(mock.finishAuthCalls).toBe(0); }); + const defaultDiscovery = + "https://mcp.linear.app/.well-known/oauth-protected-resource"; + const challengeMetadata = + "https://resource.linear.app/.well-known/oauth-protected-resource/mcp"; + + test("finishAuth uses the redirecting transport's 401 resource metadata", async () => { + mock.finishAuthError = undefined; + mock.preFailureResourceMetadataUrl = defaultDiscovery; + mock.preFailureScope = "legacy"; + mock.challengeResourceMetadataUrl = challengeMetadata; + mock.challengeScope = "mcp:read"; + mock.connectOAuthError = new InvalidScopeError("scope not allowed"); + mock.connectFailuresLeft = 1; + + const result = await connectWithAuthPrompt(); + + expect(result.ok).toBe(true); + expect(mock.finishAuthCalls).toBe(1); + expect(mock.redirectingInstanceId).toBeDefined(); + expect(mock.finishAuthInstanceId).toBe(mock.redirectingInstanceId); + expect(mock.finishAuthResourceMetadataUrl).toBe(challengeMetadata); + expect(mock.finishAuthScope).toBe("mcp:read"); + expect(mock.finishAuthResourceMetadataUrl).not.toBe(defaultDiscovery); + }); + + test("open-session code exchange does not retry the wiped mcp-session-id", async () => { + mock.finishAuthError = undefined; + mock.stampSessionId = "sess-wiped"; + mock.preFailureResourceMetadataUrl = defaultDiscovery; + mock.preFailureScope = "legacy"; + mock.challengeResourceMetadataUrl = challengeMetadata; + mock.challengeScope = "mcp:read"; + const connected = await connectMCPServer(config, { + onAuthURL: () => { + mock.authURLCount += 1; + }, + }); + expect(connected.ok).toBe(true); + if (!connected.ok) return; + + mock.callOAuthError = new InvalidScopeError("scope not allowed"); + mock.connectFailuresLeft = 1; + + await expect( + callText(connected.client, "ping", {}, new AbortController().signal), + ).resolves.toBe(""); + + expect(mock.finishAuthInstanceId).toBe(mock.redirectingInstanceId); + expect(mock.finishAuthResourceMetadataUrl).toBe(challengeMetadata); + expect(mock.finishAuthScope).toBe("mcp:read"); + expect(mock.callSessionIds[0]).toBe("sess-wiped"); + expect(mock.callSessionIds.at(-1)).toBeUndefined(); + }); + + test("a sync callback close throw stays a failed connect result", async () => { + mock.callbackCloseThrows = true; + mock.connectOAuthError = new Error("dial failed"); + + const result = await connectMCPServer(config, {}); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toBe("dial failed"); + }); + test("reconnect during in-flight waitForCode does not emit a second prompt", async () => { mock.connectFailuresLeft = Number.POSITIVE_INFINITY; mock.callbackGate = new Promise(() => undefined); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index df5e90bfe..ef2fca151 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -137,6 +137,9 @@ interface HTTPAuthContext { coordinator: SdkAuthFinish; interactive: boolean; serverName: string; + client: Client; + /** Builds the next HTTP transport and records it as the live session. */ + openTransport: () => Transport; onAuthorized?: (serverName: string) => void; } @@ -156,7 +159,11 @@ interface SdkAuthFinish { notified?: boolean; recoveryGeneration?: number; waiters?: number; - /** Transport the SDK already dialed. finishAuth reuses it; no second client. */ + /** + * Transport that finishAuth must use. A probe dial replaces the pre-failure + * transport with the instance that called redirectToAuthorization, so the + * code exchange sends that instance's resource metadata and scope. + */ transport?: StreamableHTTPClientTransport; } @@ -456,6 +463,7 @@ async function dialForFreshAuthorization( coordinator.lifecycle.signal, ), ); + let redirected = false; try { await probeClient.connect(transport as unknown as Transport, { signal: coordinator.lifecycle.signal, @@ -463,9 +471,13 @@ async function dialForFreshAuthorization( } catch (probeErr) { if (!isRecoverableAuthError(probeErr)) throw probeErr; if (coordinator.browserFlow === undefined) throw probeErr; + redirected = true; } finally { await closeQuietly(() => probeClient.close()); } + // The probe, not the pre-failure transport, holds the resource metadata and + // scope the authorize URL was built from. finishAuth has to run on it. + if (redirected) coordinator.transport = transport; } async function driveRecovery( @@ -510,6 +522,19 @@ async function driveRecovery( if (transport === undefined) throw new Error("no live HTTP transport to finish authorization"); await transport.finishAuth(code); + // The probe is a second client. After its close, the live session still + // carries the mcp-session-id from before resetAuthorization wiped tokens. + const liveSession = context.client.transport; + if ( + liveSession !== undefined && + liveSession !== transport && + liveSession.sessionId !== undefined + ) { + await context.client.close(); + await context.client.connect(context.openTransport(), { + signal: coordinator.lifecycle.signal, + }); + } } function completeVerifiedRecovery( @@ -619,7 +644,7 @@ async function finishClient( authContext?: HTTPAuthContext, signal?: AbortSignal, closeLifecycle?: () => void, - liveTransport?: { onclose?: () => void }, + live?: { current?: { onclose?: () => void } }, ): Promise { const result = await withHTTPAuthorizationRecovery(authContext, () => signal === undefined @@ -668,7 +693,7 @@ async function finishClient( }, async close() { // Disarm first: the owner's hook reports unintentional death only. - if (liveTransport !== undefined) liveTransport.onclose = () => undefined; + if (live?.current !== undefined) live.current.onclose = () => undefined; closeLifecycle?.(); authContext?.callback.close(); await closeQuietly(() => client.close()); @@ -735,6 +760,7 @@ async function connectStdio( options.onDisconnect?.(); }; } + const live = { current: transport }; return { ok: true, client: await finishClient( @@ -743,7 +769,7 @@ async function connectStdio( undefined, options.signal, undefined, - transport, + live, ), }; } catch (err) { @@ -781,13 +807,22 @@ async function connectHttp( const normalizedURL = normalizeMCPServerURL(config.url); const url = new URL(normalizedURL); let authContext: HTTPAuthContext | undefined; + const live: { current?: { onclose?: () => void } } = {}; + const connectedClient = new Client({ + name: MCP_CLIENT_NAME, + version: "1.0.0", + }); + client = connectedClient; let makeTransport: () => Transport; if (config.oauth === false) { - makeTransport = () => - new StreamableHTTPClientTransport( + makeTransport = () => { + const transport = new StreamableHTTPClientTransport( url, streamableHTTPTransportOptions(undefined, options.signal), - ) as unknown as Transport; + ); + live.current = transport; + return transport as unknown as Transport; + }; } else { callback = await startCallbackServer(config.name); const authProvider = await createOAuthProvider({ @@ -807,6 +842,7 @@ async function connectHttp( streamableHTTPTransportOptions(authProvider, lifecycle.signal), ); coordinator.transport = transport; + live.current = transport; return transport as unknown as Transport; }; authContext = { @@ -816,24 +852,19 @@ async function connectHttp( coordinator, interactive: options.onAuthURL !== undefined, serverName: config.name, + client: connectedClient, + openTransport: () => makeTransport(), ...(options.onAuthorized !== undefined ? { onAuthorized: options.onAuthorized } : {}), }; gateRedirectToAuthorization(authContext); } - const connectedClient = new Client({ - name: MCP_CLIENT_NAME, - version: "1.0.0", - }); - client = connectedClient; // Recovery may dial again after finishAuth; the last transport wins. - let liveTransport: Transport | undefined; await withHTTPAuthorizationRecovery( authContext, () => { const transport = makeTransport(); - liveTransport = transport; return connectedClient.connect(transport, { signal: lifecycle.signal, }); @@ -848,7 +879,7 @@ async function connectHttp( authContext, options.signal, closeLifecycle, - liveTransport, + live, ), }; } catch (err) { diff --git a/src/mcp/quiet.ts b/src/mcp/quiet.ts index eebfeee0e..cec0249fe 100644 --- a/src/mcp/quiet.ts +++ b/src/mcp/quiet.ts @@ -10,5 +10,9 @@ export function isAbortError(err: unknown): boolean { export async function closeQuietly( close: () => Promise, ): Promise { - await close().catch(() => undefined); + try { + await close(); + } catch { + return undefined; + } } diff --git a/testkit/mcp-sdk-mock.ts b/testkit/mcp-sdk-mock.ts index 8352cbb3e..39e6cffc2 100644 --- a/testkit/mcp-sdk-mock.ts +++ b/testkit/mcp-sdk-mock.ts @@ -31,15 +31,54 @@ export interface MockTransportSelf { options: | { authProvider?: MockAuthProvider; requestInit?: RequestInit } | undefined; + /** Identity of the StreamableHTTPClientTransport instance. */ + instanceId: number; + resourceMetadataUrl?: string; + scope?: string; + sessionId?: string; +} + +export interface MockLiveTransport { + provider?: MockAuthProvider; + instanceId: number; + resourceMetadataUrl?: string; + scope?: string; + sessionId?: string; +} + +function transportSnapshot(self: { + provider?: MockAuthProvider; + options?: + | { authProvider?: MockAuthProvider; requestInit?: RequestInit } + | undefined; + instanceId: number; + resourceMetadataUrl?: string; + scope?: string; + sessionId?: string; +}): MockTransportSelf { + return { + provider: self.provider, + signal: self.options?.requestInit?.signal, + options: self.options, + instanceId: self.instanceId, + ...(self.resourceMetadataUrl !== undefined + ? { resourceMetadataUrl: self.resourceMetadataUrl } + : {}), + ...(self.scope !== undefined ? { scope: self.scope } : {}), + ...(self.sessionId !== undefined ? { sessionId: self.sessionId } : {}), + }; } export interface McpClientMockHooks { - connect: (provider: MockAuthProvider | undefined) => Promise; + connect: ( + provider: MockAuthProvider | undefined, + transport?: MockLiveTransport, + ) => Promise; listTools: ( params: unknown, options: { signal?: AbortSignal } | undefined, ) => Promise<{ tools: [] }>; - callTool: () => Promise<{ content: [] }>; + callTool: (sessionId?: string) => Promise<{ content: [] }>; close: () => void | Promise; } @@ -64,11 +103,16 @@ export async function mockMcpClientModule( (real: typeof import("@modelcontextprotocol/sdk/client/index.js")) => ({ ...real, Client: class { - async connect(transport?: { - provider?: MockAuthProvider; - }): Promise { + #transport: MockLiveTransport | undefined = undefined; + + get transport(): MockLiveTransport | undefined { + return this.#transport; + } + + async connect(transport?: MockLiveTransport): Promise { + this.#transport = transport; state.liveProvider = transport?.provider; - await hooks.connect(transport?.provider); + await hooks.connect(transport?.provider, transport); } async listTools( params?: unknown, @@ -77,9 +121,10 @@ export async function mockMcpClientModule( return hooks.listTools(params, options); } async callTool(): Promise<{ content: [] }> { - return hooks.callTool(); + return hooks.callTool(this.#transport?.sessionId); } async close(): Promise { + this.#transport = undefined; await hooks.close(); } }, @@ -91,6 +136,7 @@ export async function mockMcpTransportModule( state: McpSdkMockState, hooks: McpTransportMockHooks, ): Promise { + let nextTransportId = 1; await withMockedModule( import.meta.resolve("@modelcontextprotocol/sdk/client/streamableHttp.js"), ( @@ -102,6 +148,10 @@ export async function mockMcpTransportModule( options?: | { authProvider?: MockAuthProvider; requestInit?: RequestInit } | undefined; + readonly instanceId: number; + resourceMetadataUrl?: string; + scope?: string; + sessionId?: string; constructor( _url: URL, options?: { @@ -109,30 +159,21 @@ export async function mockMcpTransportModule( requestInit?: RequestInit; }, ) { + this.instanceId = nextTransportId; + nextTransportId += 1; if (options?.authProvider !== undefined) this.provider = options.authProvider; this.options = options; const signal = options?.requestInit?.signal; if (signal !== undefined && signal !== null) state.lastRequestSignal = signal; - hooks.construct?.({ provider: this.provider, signal, options }); + hooks.construct?.(transportSnapshot(this)); } async finishAuth(): Promise { - await hooks.finishAuth({ - provider: this.provider, - signal: this.options?.requestInit?.signal, - options: this.options, - }); + await hooks.finishAuth(transportSnapshot(this)); } async auth(): Promise { - await hooks.auth?.({ - provider: this.provider, - signal: this.options?.requestInit?.signal, - options: this.options, - }); - } - get sessionId(): string | undefined { - return undefined; + await hooks.auth?.(transportSnapshot(this)); } }, }),