From d10a8fd0d310c3729c6ea9c050eb049c80d9b545 Mon Sep 17 00:00:00 2001 From: Ben Wachman Date: Fri, 23 Oct 2026 19:28:45 -0700 Subject: [PATCH] fix(mcp): drop placeholder arguments the tool schema would reject --- e2e/mcp-late-dispatch.test.ts | 65 ++++++++++++++ src/mcp/placeholder-args.test.ts | 140 +++++++++++++++++++++++++++++++ src/mcp/placeholder-args.ts | 106 +++++++++++++++++++++++ src/mcp/plugin.ts | 5 +- 4 files changed, 315 insertions(+), 1 deletion(-) create mode 100644 src/mcp/placeholder-args.test.ts create mode 100644 src/mcp/placeholder-args.ts diff --git a/e2e/mcp-late-dispatch.test.ts b/e2e/mcp-late-dispatch.test.ts index 9b7ae1b49..1885e5a31 100644 --- a/e2e/mcp-late-dispatch.test.ts +++ b/e2e/mcp-late-dispatch.test.ts @@ -369,4 +369,69 @@ describe("integration — late MCP dispatch", () => { }); }, ); + + test.serial( + "padded optional MCP arguments reach the server minus the values its schema rejects", + async () => { + const received: Record[] = []; + await withSession(async (session) => { + session.toolset.dynamicRunner.addTools( + mcpClientTools( + linearClient({ + inputSchema: { + type: "object", + properties: { + customView: { type: "string", minLength: 1 }, + cursor: { type: "string" }, + query: { type: "string" }, + team: { type: "string" }, + fields: { type: "array", items: { type: "string" } }, + priority: { type: "number" }, + assignee: { type: ["string", "null"] }, + release: { type: "string" }, + }, + }, + call: async (_toolName, args) => { + received.push(args); + return "ISSUE-1"; + }, + }), + ), + ); + // The padding gpt-5.6-terra sent on a real release lookup. + replyScript( + session, + [ + { + name: LATE_MCP, + args: { + customView: " ", + cursor: "", + query: "", + team: "", + fields: [], + priority: 0, + assignee: null, + release: "v0345-ef53e73667fa", + }, + }, + ], + "listed", + ); + + await runUntilDone(session, "list the issues in this release"); + }); + expect(received).toEqual([ + { + cursor: "", + query: "", + team: "", + fields: [], + priority: 0, + assignee: null, + release: "v0345-ef53e73667fa", + }, + ]); + }, + ); }); diff --git a/src/mcp/placeholder-args.test.ts b/src/mcp/placeholder-args.test.ts new file mode 100644 index 000000000..99a041588 --- /dev/null +++ b/src/mcp/placeholder-args.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, test } from "bun:test"; +import { stripPlaceholderArguments } from "./placeholder-args.js"; + +const schema = { + type: "object", + properties: { + id: { type: "string" }, + query: { type: "string" }, + note: { type: "string", minLength: 0 }, + customView: { type: "string", minLength: 1 }, + view: { anyOf: [{ type: "string", minLength: 1 }, { type: "null" }] }, + label: { oneOf: [{ type: "string", minLength: 1 }, { type: "string" }] }, + state: { type: "string", enum: ["open", "closed"] }, + stage: { type: "string", enum: ["", "planned"] }, + scope: { type: "string", const: "all" }, + slug: { type: "string", pattern: "^[a-z]+$" }, + suffix: { type: "string", pattern: "[a-z]*" }, + malformed: { type: "string", pattern: "(" }, + fields: { type: "array", items: { type: "string" } }, + labels: { type: "array", items: { type: "string" }, minItems: 1 }, + team: { type: "string" }, + assignee: { type: ["string", "null"] }, + parentId: { anyOf: [{ type: "string" }, { type: "null" }] }, + cycle: { type: "string", nullable: true }, + priority: { type: "number" }, + includeArchived: { type: "boolean" }, + anything: {}, + }, + required: ["id"], +}; + +describe("stripPlaceholderArguments", () => { + test("forwards empty and whitespace strings the property schema permits", () => { + const args = { query: "", note: "", team: " ", suffix: "", stage: "" }; + expect(stripPlaceholderArguments(args, schema)).toEqual(args); + }); + + test("drops an empty or whitespace string where minLength is at least 1", () => { + expect( + stripPlaceholderArguments({ customView: " ", query: "x" }, schema), + ).toEqual({ query: "x" }); + expect(stripPlaceholderArguments({ customView: "" }, schema)).toEqual({}); + }); + + test("drops an empty string that an enum, const, or pattern rejects", () => { + expect( + stripPlaceholderArguments({ state: "", scope: " ", slug: "" }, schema), + ).toEqual({}); + }); + + test("forwards an empty string when the pattern does not compile", () => { + expect(stripPlaceholderArguments({ malformed: "" }, schema)).toEqual({ + malformed: "", + }); + }); + + test("drops an empty array only where minItems is at least 1", () => { + expect( + stripPlaceholderArguments({ fields: [], labels: [] }, schema), + ).toEqual({ fields: [] }); + }); + + test("drops an empty value the property type does not admit", () => { + expect( + stripPlaceholderArguments( + { priority: "", includeArchived: " ", query: [] }, + schema, + ), + ).toEqual({}); + }); + + test("drops null only where the property schema disallows it", () => { + expect( + stripPlaceholderArguments({ team: null, priority: null }, schema), + ).toEqual({}); + const nullable = { + assignee: null, + parentId: null, + cycle: null, + anything: null, + }; + expect(stripPlaceholderArguments(nullable, schema)).toEqual(nullable); + }); + + test("drops a composite value only when every branch rejects it", () => { + expect(stripPlaceholderArguments({ view: " " }, schema)).toEqual({}); + expect(stripPlaceholderArguments({ view: null }, schema)).toEqual({ + view: null, + }); + expect(stripPlaceholderArguments({ label: "" }, schema)).toEqual({ + label: "", + }); + }); + + test("never changes required arguments", () => { + expect(stripPlaceholderArguments({ id: "" }, schema)).toEqual({ id: "" }); + expect(stripPlaceholderArguments({ id: null }, schema)).toEqual({ + id: null, + }); + }); + + test("keeps zero and false", () => { + const args = { priority: 0, includeArchived: false }; + expect(stripPlaceholderArguments(args, schema)).toEqual(args); + }); + + test("keeps meaningful values and leaves the caller's object intact", () => { + const args = { query: "release", fields: ["title"], customView: "" }; + expect(stripPlaceholderArguments(args, schema)).toEqual({ + query: "release", + fields: ["title"], + }); + expect(args).toEqual({ + query: "release", + fields: ["title"], + customView: "", + }); + }); + + test("forwards a key the schema does not declare", () => { + expect( + stripPlaceholderArguments({ id: "CL-1", extra: "" }, schema), + ).toEqual({ id: "CL-1", extra: "" }); + }); + + test("returns missing or non-object arguments as-is", () => { + expect(stripPlaceholderArguments(null, schema)).toBeNull(); + expect(stripPlaceholderArguments(undefined, schema)).toBeUndefined(); + expect(stripPlaceholderArguments("query", schema)).toBe("query"); + const list = [""]; + expect(stripPlaceholderArguments(list, schema)).toBe(list); + }); + + test("forwards arguments unchanged when the schema shape is unusable", () => { + const args = { query: "" }; + expect( + stripPlaceholderArguments(args, { type: "object", required: "query" }), + ).toEqual(args); + }); +}); diff --git a/src/mcp/placeholder-args.ts b/src/mcp/placeholder-args.ts new file mode 100644 index 000000000..83134c532 --- /dev/null +++ b/src/mcp/placeholder-args.ts @@ -0,0 +1,106 @@ +import { type } from "arktype"; + +const ObjectSchema = type({ + "properties?": "Record", + "required?": "string[]", +}); + +const Arguments = type("Record"); + +const PropertySchema = type({ + "type?": "string | string[]", + "nullable?": "boolean", + "enum?": "unknown[]", + "const?": "unknown", + "minLength?": "number", + "minItems?": "number", + "pattern?": "string", + "anyOf?": "unknown[]", + "oneOf?": "unknown[]", +}); + +type PropertySchema = typeof PropertySchema.infer; + +function typeName(value: unknown): string { + if (value === null) return "null"; + if (Array.isArray(value)) return "array"; + return typeof value; +} + +function admitsType(schema: PropertySchema, value: unknown): boolean { + if (schema.type === undefined) return true; + if (value === null && schema.nullable === true) return true; + const allowed = Array.isArray(schema.type) ? schema.type : [schema.type]; + return allowed.includes(typeName(value)); +} + +// An uncompilable pattern proves nothing about the value, so it never rejects. +function matchesPattern(pattern: string, value: string): boolean { + try { + return new RegExp(pattern, "u").test(value); + } catch { + return true; + } +} + +// Only the constraints an empty value can violate are checked; anything the +// schema does not express is left for the server to judge. +function admits(property: unknown, value: unknown): boolean { + const schema = PropertySchema(property); + if (schema instanceof type.errors) return true; + const branches = [...(schema.anyOf ?? []), ...(schema.oneOf ?? [])]; + if (branches.length > 0 && !branches.some((branch) => admits(branch, value))) + return false; + if (schema.enum !== undefined && !schema.enum.includes(value)) return false; + if (schema.const !== undefined && schema.const !== value) return false; + if (!admitsType(schema, value)) return false; + if (typeof value === "string") { + if ((schema.minLength ?? 0) > value.length) return false; + if (schema.pattern !== undefined && !matchesPattern(schema.pattern, value)) + return false; + } + if (Array.isArray(value) && (schema.minItems ?? 0) > value.length) + return false; + return true; +} + +function isRejectedPlaceholder(value: unknown, property: unknown): boolean { + // Servers trim before checking length, so whitespace is judged as "". + if (typeof value === "string") + return value.trim() === "" && !admits(property, ""); + if (Array.isArray(value)) return value.length === 0 && !admits(property, []); + return value === null && !admits(property, null); +} + +/** + * Drops optional arguments a model filled with an empty placeholder (`""`, + * whitespace, `[]`, or `null`) that the property's own schema would reject, + * so one padded field does not fail the whole call. Empty values the schema + * permits, required arguments, and any other value are forwarded unchanged. + */ +export function stripPlaceholderArguments( + args: Record, + inputSchema: Record, +): Record; +export function stripPlaceholderArguments( + args: unknown, + inputSchema: Record, +): unknown; +export function stripPlaceholderArguments( + args: unknown, + inputSchema: Record, +): unknown { + const parsedArgs = Arguments(args); + if (parsedArgs instanceof type.errors || Array.isArray(parsedArgs)) + return args; + const schema = ObjectSchema(inputSchema); + if (schema instanceof type.errors) return args; + const required = new Set(schema.required ?? []); + return Object.fromEntries( + Object.entries(parsedArgs).filter( + ([key, value]) => + required.has(key) || + !isRejectedPlaceholder(value, schema.properties?.[key]), + ), + ); +} diff --git a/src/mcp/plugin.ts b/src/mcp/plugin.ts index c5a177db3..68b706a16 100644 --- a/src/mcp/plugin.ts +++ b/src/mcp/plugin.ts @@ -18,6 +18,7 @@ import { type MCPClient, type MCPContentBlock, } from "./client.js"; +import { stripPlaceholderArguments } from "./placeholder-args.js"; import { mcpToolName } from "./tool-name.js"; import { LOG_NAMESPACE_ROOT } from "../branding.js"; @@ -107,9 +108,11 @@ export function mcpClientTools( signal: AbortSignal, ): Promise => { try { + // Some models pad every optional field with empty values; one the + // property schema rejects (customView minLength) fails the call. const envelope = await client.callResult( tool.name, - call.arguments, + stripPlaceholderArguments(call.arguments, tool.inputSchema), signal, ); const authorizedBlocks = applyPolicyToBlocks(envelope.blocks);