From 01356a306b6db1e821515a6e7958dcd90468eae8 Mon Sep 17 00:00:00 2001 From: Victor Vogel Date: Fri, 16 Oct 2026 17:36:38 -0500 Subject: [PATCH] fix(mcp): optional tool parameters accept the null their schema advertises MCP tool input schemas come from each parameter schema's JSON codec, which lists null for every Schema.optional field, but Effect's Toolkit decodes with the parameter schema itself and rejects that null. Agents that send an explicit null for an optional argument got InvalidParams and had to retry. The server now decodes tool arguments through the same JSON codec, for toolkit registrations and for the hand-registered image tools. A null optional argument decodes like an omitted one; nulls a schema declares keep their meaning. delegate_task.timeoutMs and t3_thread_wait.timeoutMs become Schema.Finite, so the codec does not start accepting "Infinity" or "NaN" there and those two fields stop advertising them. Fixes #16809 Co-Authored-By: Claude Opus 5.5 --- apps/server/src/mcp/McpHttpServer.test.ts | 99 ++++++++++++++++++++++- apps/server/src/mcp/McpHttpServer.ts | 26 ++++-- packages/contracts/src/orchestratorMcp.ts | 4 +- 3 files changed, 119 insertions(+), 10 deletions(-) diff --git a/apps/server/src/mcp/McpHttpServer.test.ts b/apps/server/src/mcp/McpHttpServer.test.ts index 9d28aeb4dfc1..90c4af4d6e0d 100644 --- a/apps/server/src/mcp/McpHttpServer.test.ts +++ b/apps/server/src/mcp/McpHttpServer.test.ts @@ -34,7 +34,10 @@ import * as McpToolAccess from "./McpToolAccess.ts"; import * as McpToolAccessTestkit from "./McpToolAccess.testkit.ts"; import * as McpInvocationContext from "./McpInvocationContext.ts"; import * as McpSessionRegistry from "./McpSessionRegistry.ts"; +import * as OrchestratorMcpService from "./OrchestratorMcpService.ts"; import * as PreviewAutomationBroker from "./PreviewAutomationBroker.ts"; +import { OrchestratorToolkit } from "./toolkits/orchestrator/tools.ts"; +import { ProjectToolkit } from "./toolkits/project/tools.ts"; const environmentId = EnvironmentId.make("environment-mcp-test"); const threadId = ThreadId.make("thread-mcp-test"); @@ -125,17 +128,55 @@ const serveSnapshots = (clientId: string, result: unknown) => return inputs; }); -const callSnapshot = (args: Record) => +const callTool = (name: string, args: Record) => Effect.gen(function* () { const server = yield* McpServer.McpServer; return yield* server - .callTool({ name: "preview_snapshot", arguments: args }) + .callTool({ name, arguments: args }) .pipe( Effect.provideService(McpInvocationContext.McpInvocationContext, invocation), Effect.provideService(McpSchema.McpServerClient, client), ); }); +const callSnapshot = (args: Record) => callTool("preview_snapshot", args); + +// Real tool definitions with handlers that record what they decoded. +const layerRecordingTools = (calls: Array) => { + const RecordingToolkit = Toolkit.make( + OrchestratorToolkit.tools.delegate_task, + OrchestratorToolkit.tools.t3_thread_read, + OrchestratorToolkit.tools.t3_thread_wait, + ProjectToolkit.tools.t3_project_update, + ); + const record = McpToolAccess.reads((params: unknown) => + Effect.sync(() => calls.push(params)).pipe( + Effect.andThen( + Effect.fail(new OrchestratorMcpFailure({ code: "thread_not_found", message: "Recorded." })), + ), + ), + ); + return McpHttpServer.toolkitRegistration( + RecordingToolkit, + McpToolAccess.toLayer(RecordingToolkit, { + delegate_task: record, + t3_thread_read: record, + t3_thread_wait: record, + t3_project_update: record, + }), + ).pipe( + Layer.provideMerge(McpServer.McpServer.layer), + Layer.provide( + Layer.mergeAll( + McpToolAccessTestkit.liveThreadsLayer, + Layer.mock(OrchestratorMcpService.OrchestratorMcpService)({}), + Layer.mock(ProjectService.ProjectService)({}), + NodeServices.layer, + ), + ), + ); +}; + it("normalizes empty successful notification responses to accepted", () => { const notificationResponse = McpHttpServer.normalizeMcpHttpResponse( HttpServerResponse.text("", { status: 200, contentType: "application/json" }), @@ -390,7 +431,7 @@ it.effect("refuses preview tools to a client outside a thread before they run", it.effect("rejects non-boolean snapshot image options before selecting a browser host", () => Effect.gen(function* () { const server = yield* McpServer.McpServer; - for (const includeImage of ["false", 0, null]) { + for (const includeImage of ["false", 0]) { const result = yield* server .callTool({ name: "preview_snapshot", @@ -447,6 +488,58 @@ it.effect("saves the snapshot PNG on request and reports its path", () => ).pipe(Effect.provide(layerTest)), ); +it.effect("treats null snapshot options like omitted ones", () => + Effect.scoped( + Effect.gen(function* () { + const inputs = yield* serveSnapshots("mcp-null-options-client", snapshotResult); + + const omitted = yield* callSnapshot({}); + const nulls = yield* callSnapshot({ tabId: null, includeImage: null, save: null }); + + expect(nulls).toEqual(omitted); + expect(inputs).toEqual([{}, {}]); + }), + ).pipe(Effect.provide(layerTest)), +); + +it.effect("decodes null optional arguments the way it decodes omitted ones", () => { + const calls: Array = []; + return Effect.gen(function* () { + const thread = { threadId: "thread-null-arguments" }; + yield* callTool("t3_thread_read", thread); + yield* callTool("t3_thread_read", { ...thread, afterPosition: null, view: null }); + yield* callTool("delegate_task", { task: "Inspect", target: {} }); + yield* callTool("delegate_task", { task: "Inspect", title: null, target: { model: null } }); + // A null the schema declares keeps its meaning: here it clears the project icon. + yield* callTool("t3_project_update", { projectId: "project-1", projectIcon: null }); + + expect(calls).toHaveLength(5); + expect(calls[1]).toEqual(calls[0]); + expect(calls[3]).toEqual(calls[2]); + expect(calls[4]).toStrictEqual({ projectId: "project-1", projectIcon: null }); + }).pipe(Effect.provide(layerRecordingTools(calls))); +}); + +it.effect("accepts finite wait budgets and refuses non-finite ones", () => { + const calls: Array = []; + return Effect.gen(function* () { + yield* callTool("delegate_task", { task: "Inspect", timeoutMs: 1500.5 }); + yield* callTool("t3_thread_wait", { threadId: "thread-wait", timeoutMs: 5000 }); + expect(calls).toMatchObject([{ timeoutMs: 1500.5 }, { timeoutMs: 5000 }]); + + for (const [name, args] of [ + ["delegate_task", { task: "Inspect", timeoutMs: "Infinity" }], + ["t3_thread_wait", { threadId: "thread-wait", timeoutMs: "NaN" }], + ["t3_thread_wait", { threadId: "thread-wait", timeoutMs: "-Infinity" }], + ["t3_thread_read", { threadId: "thread-wait", afterPosition: "2" }], + ] as const) { + const refused = yield* callTool(name, args).pipe(Effect.flip); + expect(refused._tag, `${name} ${JSON.stringify(args)}`).toBe("InvalidParams"); + } + expect(calls).toHaveLength(2); + }).pipe(Effect.provide(layerRecordingTools(calls))); +}); + it.effect("reports a tagged error when the screenshot cannot be saved", () => Effect.scoped( Effect.gen(function* () { diff --git a/apps/server/src/mcp/McpHttpServer.ts b/apps/server/src/mcp/McpHttpServer.ts index cd2ccf141638..80d43043df6a 100644 --- a/apps/server/src/mcp/McpHttpServer.ts +++ b/apps/server/src/mcp/McpHttpServer.ts @@ -11,7 +11,7 @@ import * as Schema from "effect/Schema"; import * as Sink from "effect/Sink"; import * as Stream from "effect/Stream"; import type * as Types from "effect/Types"; -import { AiError, McpProtocol, McpSchema, McpServer, Tool, type Toolkit } from "effect/ai"; +import { AiError, McpProtocol, McpSchema, McpServer, Tool, Toolkit } from "effect/ai"; import { HttpRouter, HttpServerRequest, HttpServerResponse } from "effect/http"; import { OrchestratorMcpFailure, PreviewAutomationError } from "@t3tools/contracts"; @@ -386,6 +386,22 @@ const saveScreenshot = Effect.fn("McpHttpServer.saveScreenshot")(function* ( return screenshotPath; }); +/** + * Tool input schemas are advertised through each schema's JSON codec, which + * offers `null` for every `Schema.optional` field, but Toolkit decodes with the + * schema itself and rejects that `null`. Decoding through the same codec makes + * a tool accept what it advertises. + */ +const decodeAsAdvertised = >( + toolkit: Toolkit.Toolkit, +): Toolkit.Toolkit => + // The clones keep each tool's id, name and decoded parameter type, so handlers still match. + Toolkit.make( + ...Object.values(toolkit.tools).map((tool) => + tool.setParameters(Schema.toCodecJson(tool.parametersSchema)), + ), + ) as unknown as Toolkit.Toolkit; + const isPreviewAutomationError = Schema.is(PreviewAutomationError); const previewSnapshotFailure = (cause: Cause.Cause) => { @@ -431,7 +447,7 @@ const registerPreviewSnapshot = Effect.fn("McpHttpServer.registerPreviewSnapshot const saveServices = yield* Effect.context< ServerConfig.ServerConfig | FileSystem.FileSystem | Path.Path | Crypto.Crypto >(); - const built = yield* PreviewSnapshotToolkit; + const built = yield* decodeAsAdvertised(PreviewSnapshotToolkit); const tool = PreviewSnapshotTool; yield* server.addTool({ tool: new McpSchema.Tool({ @@ -687,7 +703,7 @@ const registerImageTool = ( const registerDeviceScreenshot = Effect.fn("McpHttpServer.registerDeviceScreenshot")(function* () { const devices = yield* DeviceService.DeviceService; const threads = yield* ThreadManagementService.ThreadManagementService; - const built = yield* DeviceScreenshotToolkit; + const built = yield* decodeAsAdvertised(DeviceScreenshotToolkit); yield* registerImageTool( DeviceScreenshotTool, (payload) => @@ -709,7 +725,7 @@ const isOrchestratorMcpFailure = Schema.is(OrchestratorMcpFailure); const registerHtmlPreview = Effect.fn("McpHttpServer.registerHtmlPreview")(function* () { const htmlRender = yield* HtmlRender.HtmlRender; const threads = yield* ThreadManagementService.ThreadManagementService; - const built = yield* HtmlPreviewToolkit; + const built = yield* decodeAsAdvertised(HtmlPreviewToolkit); yield* registerImageTool( HtmlPreviewTool, (payload) => @@ -740,7 +756,7 @@ export const toolkitRegistration = , EX, toolkit: Toolkit.Toolkit, handlers: McpToolAccess.HandlersLayer, ) => { - const registration = McpServer.toolkit(toolkit); + const registration = McpServer.toolkit(decodeAsAdvertised(toolkit)); // @effect-diagnostics-next-line unsafeEffectTypeAssertion:off - the auth middleware provides it per request. const registered = registration as Layer.Layer< never, diff --git a/packages/contracts/src/orchestratorMcp.ts b/packages/contracts/src/orchestratorMcp.ts index 382cea55bb15..5e178c21551a 100644 --- a/packages/contracts/src/orchestratorMcp.ts +++ b/packages/contracts/src/orchestratorMcp.ts @@ -173,7 +173,7 @@ export const OrchestratorMcpDelegateTaskInput = Schema.Struct({ "Defaults to async. Use wait only when this turn needs the child's result before you can continue.", }), ), - timeoutMs: Schema.optional(Schema.Number).annotate({ + timeoutMs: Schema.optional(Schema.Finite).annotate({ description: "Wait budget for mode=wait only. Default 10 minutes. Elapsing it returns waitTimedOut=true on that call and does not cancel the child.", }), @@ -443,7 +443,7 @@ export type OrchestratorMcpThreadSendResult = typeof OrchestratorMcpThreadSendRe export const OrchestratorMcpThreadWaitInput = Schema.Struct({ threadId: ThreadId, runId: Schema.optional(RunId), - timeoutMs: Schema.optional(Schema.Number), + timeoutMs: Schema.optional(Schema.Finite), }); export type OrchestratorMcpThreadWaitInput = typeof OrchestratorMcpThreadWaitInput.Type;