From 2dc8db4a9da777dc03aec112aec891d5bd79211d Mon Sep 17 00:00:00 2001 From: Aiden Cline <63023139+rekram1-node@users.noreply.github.com> Date: Sat, 24 Oct 2026 12:45:34 -0500 Subject: [PATCH] fix(core): skip malformed MCP tools instead of failing the server (#58350) --- packages/core/src/mcp/client.ts | 37 ++++++++++++++++++++++++- packages/core/test/mcp.test.ts | 49 +++++++++++++++++++++++++++++---- 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/packages/core/src/mcp/client.ts b/packages/core/src/mcp/client.ts index 55ef10dd207..cceb8ad53f9 100644 --- a/packages/core/src/mcp/client.ts +++ b/packages/core/src/mcp/client.ts @@ -10,6 +10,7 @@ import { StreamableHTTPClientTransport, UnauthorizedError, UnsupportedProtocolVersionError, + isJSONRPCResultResponse, type CallToolResult as SdkCallToolResult, type ElicitRequestFormParams, type ElicitRequestParams, @@ -61,6 +62,14 @@ const isTransient = (error: unknown) => export type { GetPromptResult, Prompt, ReadResourceResult, Resource, Tool } export type ResourceTemplate = ResourceTemplateType +// The era codec is protected; it checks a tools/list result against the exact schema the SDK applies +// for the negotiated protocol era, so a tool is skipped only when the SDK itself would reject it. +class ToolCheckingClient extends Client { + checkToolsList(result: unknown) { + return this._wireCodec().validateResult("tools/list", result) + } +} + export class NeedsAuthError extends Schema.TaggedError()("MCP.NeedsAuthError", { server: Schema.String, message: Schema.String, @@ -198,7 +207,7 @@ export const connect = Effect.fnUntraced(function* ( const context = yield* Effect.context() const runFork = Effect.runForkWith(context) const runPromise = Effect.runPromiseWith(context) - const client = new Client(clientInfo, { + const client = new ToolCheckingClient(clientInfo, { capabilities: { ...(elicitation ? { elicitation: { form: {}, url: {} } } : {}), // Legacy era only: roots are deprecated as of 2026-07-28 and modern servers cannot request them. @@ -228,6 +237,32 @@ export const connect = Effect.fnUntraced(function* ( ) } + // The SDK validates tools/list as a whole, so one malformed tool would fail every tool on the server. + // The SDK runs a handler set on the transport before connect ahead of its own, on the same message, + // so tools it would reject are dropped there and it still caches the rest. A page that fails for + // another reason is left for the SDK to report. + const onmessage = transport.onmessage + transport.onmessage = (message, extra) => { + if ( + isJSONRPCResultResponse(message) && + Array.isArray(message.result.tools) && + client.checkToolsList({ ...message.result, tools: [] }).ok + ) + message.result.tools = message.result.tools.filter((tool: unknown) => { + const outcome = client.checkToolsList({ ...message.result, tools: [tool] }) + if (outcome.ok) return true + runFork( + Effect.logWarning("skipping invalid MCP tool", { + server, + tool: typeof tool === "object" && tool !== null && "name" in tool ? tool.name : undefined, + error: outcome.reason === "invalid" ? outcome.message : outcome.reason, + }), + ) + return false + }) + onmessage?.(message, extra) + } + yield* Effect.tryPromise({ try: (signal) => client.connect(transport, { timeout: timeout?.startup ?? DEFAULT_STARTUP_TIMEOUT, signal }), catch: (error) => error, diff --git a/packages/core/test/mcp.test.ts b/packages/core/test/mcp.test.ts index 70ad0cd9a33..8d2d3ae4152 100644 --- a/packages/core/test/mcp.test.ts +++ b/packages/core/test/mcp.test.ts @@ -98,6 +98,11 @@ function resourceServer( listChanged?: boolean emptyElicitation?: boolean urlElicitation?: boolean + /** + * List a tool whose input schema is not an object, and one whose `required` is a string, which + * only the 2026-07-28 schema accepts, ahead of the valid tools. + */ + malformedTool?: boolean /** Legacy servers give up on the URL elicitation after this many milliseconds. */ elicitationTimeout?: number respond?: (request: Request) => Response | undefined | Promise @@ -147,11 +152,23 @@ function resourceServer( protocol.setRequestHandler("tools/list", () => { state.toolLists += 1 return Promise.resolve({ - tools: input.emptyElicitation - ? [{ name: "empty-elicitation", inputSchema: { type: "object" as const, properties: {} } }] - : input.urlElicitation - ? [{ name: "url-elicitation", inputSchema: { type: "object" as const, properties: {} } }] - : [{ name: "echo", inputSchema: { type: "object" as const, properties: {} } }], + tools: [ + // Servers send raw JSON; the cast lets the fixture send a tool the schema rejects. + ...(input.malformedTool + ? [ + { name: "malformed", inputSchema: { type: "string" } } as unknown as McpClient.Tool, + { + name: "modern-only", + inputSchema: { type: "object", required: "text" }, + } as unknown as McpClient.Tool, + ] + : []), + ...(input.emptyElicitation + ? [{ name: "empty-elicitation", inputSchema: { type: "object" as const, properties: {} } }] + : input.urlElicitation + ? [{ name: "url-elicitation", inputSchema: { type: "object" as const, properties: {} } }] + : [{ name: "echo", inputSchema: { type: "object" as const, properties: {} } }]), + ], }) }) if (input.emptyElicitation) { @@ -1182,6 +1199,28 @@ describe.each([ ), ) }) + + test("skips tools the SDK rejects and keeps the rest of the server's tools", async () => { + await Effect.runPromise( + Effect.scoped( + Effect.gen(function* () { + const server = yield* resourceServer({ modern: era === "modern", resources: false, malformedTool: true }) + const connection = yield* connect( + "malformed", + new ConfigMCP.Remote({ type: "remote", url: server.url, oauth: false, protocol }), + import.meta.dir, + ) + + // Only tools the SDK itself would reject for the negotiated era are skipped. + expect((yield* connection.tools()).map((tool) => tool.name)).toEqual( + era === "modern" ? ["modern-only", "echo"] : ["echo"], + ) + yield* connection.callTool({ name: "echo", args: { text: "hi" } }) + expect(server.state.toolCalls.at(-1)).toMatchObject({ name: "echo", arguments: { text: "hi" } }) + }), + ), + ) + }) }) test("lists, reads, and reports MCP resource changes", async () => {