diff --git a/docs/MCP.md b/docs/MCP.md index 39842675f..14ea76c01 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -130,14 +130,21 @@ 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). 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/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..d83856747 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -143,7 +143,11 @@ 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 +190,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 +268,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 +305,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 +330,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..1d0a38413 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, @@ -18,7 +19,31 @@ 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. */ + connectOAuthError: Error | undefined; listFailuresLeft: number; callFailuresLeft: number; callToolCalls: number; @@ -31,6 +56,7 @@ interface MockState { redirectVerifier: string | undefined; providerCreates: number; refreshCalls: number; + resetCalls: number; refreshSucceeds: boolean; authEvents: string[]; authURLCount: number; @@ -56,7 +82,23 @@ 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, listFailuresLeft: 0, callFailuresLeft: 0, callToolCalls: 0, @@ -69,6 +111,7 @@ function freshMockState(): MockState { redirectVerifier: undefined, providerCreates: 0, refreshCalls: 0, + resetCalls: 0, refreshSucceeds: false, authEvents: [], authURLCount: 0, @@ -95,7 +138,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; @@ -183,12 +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) { @@ -202,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) { @@ -221,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; }, @@ -232,7 +327,9 @@ await mockMcpCallbackServerModule({ await waitForGate(signal); return "code"; }, - close: () => undefined, + close: () => { + if (mock.callbackCloseThrows) throw new Error("callback close threw"); + }, }); await mockMcpOAuthProviderModule( @@ -254,6 +351,7 @@ const { connectMCPServer, resetBrowserAuthState, setBrowserAuthWaitMs, + unwrapToolContent, MAX_BROWSER_AUTH_ATTEMPTS, BROWSER_AUTH_COOLDOWN_MS, } = await import("./client.js"); @@ -269,6 +367,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 +410,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 +472,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 +489,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 +507,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 +527,8 @@ 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 +537,8 @@ 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 +567,9 @@ 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 +599,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 +628,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 +638,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 +660,8 @@ 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 +683,8 @@ 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 +708,8 @@ 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 +737,14 @@ 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 +755,8 @@ 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 +785,101 @@ 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); + }); + + 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); @@ -715,7 +928,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 +1023,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 +1050,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 +1077,8 @@ 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 +1101,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 +1126,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 +1137,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 +1159,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..ef2fca151 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,9 +134,12 @@ interface HTTPAuthContext { url: URL; authProvider: CorbitsOAuthProvider; callback: CallbackServer; - coordinator: HTTPAuthCoordinator; + 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; } @@ -150,7 +148,7 @@ interface BrowserAuthFlow { promptEmitted: Promise; } -interface HTTPAuthCoordinator { +interface SdkAuthFinish { lifecycle: AbortController; inFlight?: Promise; refreshInFlight?: Promise; @@ -161,7 +159,12 @@ interface HTTPAuthCoordinator { notified?: boolean; recoveryGeneration?: number; waiters?: number; - probe(): Promise; + /** + * 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; } function isRecoverableAuthError( @@ -275,7 +278,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 +393,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 +414,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 +443,43 @@ 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, + ), + ); + let redirected = false; + 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; + 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( err: UnauthorizedError | OAuthError, context: HTTPAuthContext, @@ -433,37 +487,54 @@ 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); + // 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( @@ -573,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 @@ -620,14 +691,9 @@ 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; + if (live?.current !== undefined) live.current.onclose = () => undefined; closeLifecycle?.(); authContext?.callback.close(); await closeQuietly(() => client.close()); @@ -684,9 +750,17 @@ 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?.(); + }; + } + const live = { current: transport }; return { ok: true, client: await finishClient( @@ -695,7 +769,7 @@ async function connectStdio( undefined, options.signal, undefined, - transport, + live, ), }; } catch (err) { @@ -733,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({ @@ -750,26 +833,17 @@ 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; + live.current = transport; + return transport as unknown as Transport; }; authContext = { url, @@ -778,39 +852,25 @@ 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; - // The auth-recovery wrapper can dial twice; the last transport wins. - let liveTransport: Transport | undefined; + // Recovery may dial again after finishAuth; the last transport wins. await withHTTPAuthorizationRecovery( authContext, () => { const transport = makeTransport(); - liveTransport = transport; return connectedClient.connect(transport, { signal: lifecycle.signal, }); }, 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( @@ -819,17 +879,13 @@ async function connectHttp( authContext, options.signal, closeLifecycle, - liveTransport, + live, ), }; } 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/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/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, 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)); } }, }),