diff --git a/core/tools/callTool.vitest.ts b/core/tools/callTool.vitest.ts new file mode 100644 index 00000000000..f1801057f8f --- /dev/null +++ b/core/tools/callTool.vitest.ts @@ -0,0 +1,85 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { Tool, ToolExtras } from ".."; + +const { mcpCallTool } = vi.hoisted(() => ({ mcpCallTool: vi.fn() })); + +vi.mock("../context/mcp/MCPManagerSingleton", () => ({ + MCPManagerSingleton: { + getInstance: () => ({ + getConnection: () => ({ + client: { callTool: mcpCallTool }, + options: { timeout: 1000 }, + }), + }), + }, +})); + +import { callTool, encodeMCPToolUri } from "./callTool"; + +describe("callTool with MCP tools", () => { + const tool: Tool = { + type: "function", + displayTitle: "filesystem edit_file", + readonly: false, + group: "filesystem", + uri: encodeMCPToolUri("filesystem", "edit_file"), + function: { + name: "filesystem_edit_file", + parameters: { + type: "object", + properties: { + path: { type: "string" }, + edits: { type: "array", items: { type: "object" } }, + dryRun: { type: "boolean" }, + }, + required: ["path", "edits"], + }, + }, + }; + + beforeEach(() => { + mcpCallTool.mockReset(); + mcpCallTool.mockResolvedValue({ content: [{ type: "text", text: "ok" }] }); + }); + + it("should send schema-typed arguments for sequential tool calls", async () => { + const rawArgs = [ + // Arrays and booleans serialized as JSON text, as in #13350 + '{"path": "a.ts", "edits": "[{\\"oldText\\": \\"a\\", \\"newText\\": \\"b\\"}]", "dryRun": "false"}', + '{"path": "b.ts", "edits": [{"oldText": "c", "newText": "d"}], "dryRun": true}', + '{"path": "[c].ts", "edits": "[]"}', + ]; + for (const [i, args] of rawArgs.entries()) { + const result = await callTool( + tool, + { + id: `call-${i}`, + type: "function", + function: { name: tool.function.name, arguments: args }, + }, + { tool } as unknown as ToolExtras, + ); + expect(result.errorMessage).toBeUndefined(); + } + + expect(mcpCallTool.mock.calls.map(([params]) => params)).toEqual([ + { + name: "edit_file", + arguments: { + path: "a.ts", + edits: [{ oldText: "a", newText: "b" }], + dryRun: false, + }, + }, + { + name: "edit_file", + arguments: { + path: "b.ts", + edits: [{ oldText: "c", newText: "d" }], + dryRun: true, + }, + }, + { name: "edit_file", arguments: { path: "[c].ts", edits: [] } }, + ]); + }); +}); diff --git a/core/tools/parseArgs.ts b/core/tools/parseArgs.ts index 64a4f67d09a..393d50f070e 100644 --- a/core/tools/parseArgs.ts +++ b/core/tools/parseArgs.ts @@ -24,12 +24,250 @@ export function safeParseToolCallArgs( } } +type JsonType = + | "null" + | "boolean" + | "integer" + | "number" + | "string" + | "array" + | "object"; + +const JSON_TYPES: string[] = [ + "null", + "boolean", + "integer", + "number", + "string", + "array", + "object", +]; + +// "integer" for integral numbers, "number" for all other numbers +function getJsonType(value: unknown): JsonType { + if (value === null) { + return "null"; + } + if (Array.isArray(value)) { + return "array"; + } + if (typeof value === "number") { + return Number.isInteger(value) ? "integer" : "number"; + } + return typeof value as JsonType; +} + +// Schema nodes visited per argument; each $ref is expanded once, so real +// schemas need few, and larger or adversarial ones are left unanalyzed +const MAX_SCHEMA_NODES = 1000; + +interface SchemaTraversal { + root: Record; + nodesLeft: number; + refTypes: Map | undefined>; + activeRefs: Set; + // Set when the schema cannot be analyzed safely; the argument is kept as-is + unsupported: boolean; +} + +// $id (or id in draft 4) starts a nested schema resource, whose references +// resolve against its own base URI instead of the root +function hasResourceId(node: unknown): boolean { + if (typeof node !== "object" || node === null) { + return false; + } + const { $id, id } = node as Record; + return typeof $id === "string" || typeof id === "string"; +} + +function resolveLocalRef(root: Record, ref: string): unknown { + if (ref !== "#" && !ref.startsWith("#/")) { + return undefined; + } + let node: unknown = root; + for (const token of ref.split("/").slice(1)) { + const key = token.replace(/~1/g, "/").replace(/~0/g, "~"); + if ( + typeof node !== "object" || + node === null || + !Object.hasOwn(node, key) + ) { + return undefined; + } + node = (node as Record)[key]; + if (hasResourceId(node)) { + return undefined; + } + } + return node; +} + +/** + * Returns the JSON types a schema accepts, or undefined when it may accept + * any type (no type constraint, ...). Keywords that can only narrow this + * (not, if/then/else, $ref siblings, ...) are ignored, so the result never + * omits a type the schema accepts. Schemas that are too large, recursive or + * use references that cannot be resolved within the root resource set + * traversal.unsupported instead. + */ +function getAcceptedTypes( + schema: unknown, + traversal: SchemaTraversal, +): Set | undefined { + if (traversal.unsupported || --traversal.nodesLeft < 0) { + traversal.unsupported = true; + return undefined; + } + if (schema === false) { + return new Set(); + } + if (typeof schema !== "object" || schema === null || Array.isArray(schema)) { + return undefined; + } + const s = schema as Record; + + if ( + (s !== traversal.root && hasResourceId(s)) || + "$dynamicRef" in s || + "$recursiveRef" in s + ) { + traversal.unsupported = true; + return undefined; + } + if (s.$ref !== undefined) { + return getRefTypes(s.$ref, traversal); + } + + const constraints: (Set | undefined)[] = []; + if (s.type !== undefined) { + const names: unknown[] = Array.isArray(s.type) ? s.type : [s.type]; + constraints.push( + names.every((name) => JSON_TYPES.includes(name as string)) + ? new Set( + names.flatMap((name) => + name === "number" ? ["integer", "number"] : [name as JsonType], + ) as JsonType[], + ) + : undefined, + ); + } + if (Array.isArray(s.enum)) { + constraints.push(new Set(s.enum.map(getJsonType))); + } + if ("const" in s) { + constraints.push(new Set([getJsonType(s.const)])); + } + for (const keyword of ["anyOf", "oneOf"]) { + if (Array.isArray(s[keyword])) { + const branches: (Set | undefined)[] = s[keyword].map( + (branch: unknown) => getAcceptedTypes(branch, traversal), + ); + constraints.push( + branches.every((types): types is Set => !!types) + ? new Set(branches.flatMap((types) => [...types])) + : undefined, + ); + } + } + if (Array.isArray(s.allOf)) { + for (const branch of s.allOf) { + constraints.push(getAcceptedTypes(branch, traversal)); + } + } + + let accepted: Set | undefined; + for (const types of constraints) { + if (types) { + accepted = accepted + ? new Set([...accepted].filter((type) => types.has(type))) + : types; + } + } + return accepted; +} + +function getRefTypes( + ref: unknown, + traversal: SchemaTraversal, +): Set | undefined { + // Non-string, or pointing back into a schema that is being expanded + if (typeof ref !== "string" || traversal.activeRefs.has(ref)) { + traversal.unsupported = true; + return undefined; + } + if (traversal.refTypes.has(ref)) { + return traversal.refTypes.get(ref); + } + const target = resolveLocalRef(traversal.root, ref); + if (target === undefined) { + traversal.unsupported = true; + return undefined; + } + traversal.activeRefs.add(ref); + const types = getAcceptedTypes(target, traversal); + traversal.activeRefs.delete(ref); + traversal.refTypes.set(ref, types); + return types; +} + +// Accepted types of one argument's schema, or undefined to keep the argument +function getPropertyTypes( + propSchema: unknown, + root: Record, +): Set | undefined { + const traversal: SchemaTraversal = { + root, + nodesLeft: MAX_SCHEMA_NODES, + refTypes: new Map(), + activeRefs: new Set(), + unsupported: false, + }; + const types = getAcceptedTypes(propSchema, traversal); + return traversal.unsupported ? undefined : types; +} + +/** + * Parses a JSON-serialized argument (e.g. "[\"a.ts\"]", "42", "false"). + * The first element is false unless the parsed value has an accepted type + * and can be represented without loss. + */ +function parseSerializedArg( + value: string, + accepted: Set, +): [boolean, unknown] { + let parsed: unknown; + try { + parsed = JSON.parse(value); + } catch { + // Same case-insensitive boolean strings as getBooleanArg + const literal = value.trim().toLowerCase(); + if (literal !== "true" && literal !== "false") { + return [false, undefined]; + } + parsed = literal === "true"; + } + if ( + typeof parsed === "number" && + !(Number.isInteger(parsed) + ? Number.isSafeInteger(parsed) + : Number.isFinite(parsed)) + ) { + return [false, undefined]; + } + return [accepted.has(getJsonType(parsed)), parsed]; +} + /** * Coerce parsed args to match the tool's input schema types. * JSON.parse() deeply parses all values, so string-typed parameters * that contain valid JSON (e.g. file content for a .json file) get * converted to objects. This checks the schema and re-stringifies * any values that should be strings. + * + * Conversely, some models and providers send arrays, objects, numbers and + * booleans as JSON text (e.g. {"paths": "[\"a.ts\"]"}). A string is parsed + * only when the schema rejects strings and accepts the parsed value's type; + * anything else is passed through for the tool to validate. */ export function coerceArgsToSchema( args: Record, @@ -56,6 +294,14 @@ export function coerceArgsToSchema( } catch { // leave as-is if stringify fails } + } else if (typeof value === "string") { + const accepted = getPropertyTypes(propSchema, schema); + if (accepted && !accepted.has("string")) { + const [isAccepted, parsed] = parseSerializedArg(value, accepted); + if (isAccepted) { + coerced[key] = parsed; + } + } } } diff --git a/core/tools/parseArgs.vitest.ts b/core/tools/parseArgs.vitest.ts index e21254b686d..4ec7f4ca511 100644 --- a/core/tools/parseArgs.vitest.ts +++ b/core/tools/parseArgs.vitest.ts @@ -416,6 +416,418 @@ describe("coerceArgsToSchema", () => { }); }); +describe("coerceArgsToSchema with JSON-serialized values", () => { + // Some providers return non-string arguments as JSON text, e.g. + // {"paths": "[\"a.jsx\"]"}; strict MCP servers reject those (#13350) + it("should decode a serialized array when the schema expects an array", () => { + const args = { + paths: + '["frontend/src/pages/Accounts.jsx","frontend/src/components/Translate.jsx"]', + }; + const schema = { + type: "object", + properties: { + paths: { type: "array", items: { type: "string" } }, + }, + }; + const result = coerceArgsToSchema(args, schema); + expect(result.paths).toEqual([ + "frontend/src/pages/Accounts.jsx", + "frontend/src/components/Translate.jsx", + ]); + }); + + it("should decode nested arrays", () => { + const args = { matrix: " [[1, 2], [3, [4, 5]], []] " }; + const schema = { + type: "object", + properties: { + matrix: { type: "array", items: { type: "array" } }, + }, + }; + const result = coerceArgsToSchema(args, schema); + expect(result.matrix).toEqual([[1, 2], [3, [4, 5]], []]); + }); + + it("should decode a serialized object, keeping nested values native", () => { + const args = { + settings: + '{"enabled": true, "mode": "fast", "size": {"width": 800, "scale": 1.5}, "tags": ["a", "b"], "parent": null}', + }; + const schema = { + type: "object", + properties: { settings: { type: "object" } }, + }; + const result = coerceArgsToSchema(args, schema); + expect(result.settings).toEqual({ + enabled: true, + mode: "fast", + size: { width: 800, scale: 1.5 }, + tags: ["a", "b"], + parent: null, + }); + }); + + it("should decode serialized integers, numbers, booleans and null", () => { + const args = { + count: "42", + negative: "-7", + ratio: "3.14", + exponent: "1e3", + dryRun: "false", + verbose: "TRUE", + limit: "null", + flag: "null", + }; + const schema = { + type: "object", + properties: { + count: { type: "integer" }, + negative: { type: "integer" }, + ratio: { type: "number" }, + exponent: { type: "number" }, + dryRun: { type: "boolean" }, + verbose: { type: "boolean" }, + limit: { anyOf: [{ type: "integer" }, { type: "null" }] }, + flag: { type: ["boolean", "null"] }, + }, + }; + expect(coerceArgsToSchema(args, schema)).toEqual({ + count: 42, + negative: -7, + ratio: 3.14, + exponent: 1000, + dryRun: false, + verbose: true, + limit: null, + flag: null, + }); + }); + + it("should leave strings unchanged when the schema accepts a string", () => { + const args = { + content: '{"key": [1, 2]}', + list: "[1, 2]", + either: '["a"]', + nullableText: "null", + choice: "true", + anything: "42", + described: "[3]", + }; + const schema = { + type: "object", + properties: { + content: { type: "string" }, + list: { type: ["string", "array"] }, + either: { anyOf: [{ type: "array" }, { type: "string" }] }, + nullableText: { anyOf: [{ type: "string" }, { type: "null" }] }, + choice: { enum: ["true", "false", "auto"] }, + anything: {}, + described: { description: "no type constraint" }, + }, + }; + expect(coerceArgsToSchema(args, schema)).toEqual(args); + }); + + it("should not truncate, round or reinterpret numeric strings", () => { + const args = { + fraction: "3.7", + unsafe: "9007199254740993", + overflow: "1e400", + hex: "0x1A", + nan: "NaN", + infinity: "Infinity", + empty: "", + leadingZero: "042", + suffix: "12px", + }; + const integer = { type: "integer" }; + const number = { type: "number" }; + const schema = { + type: "object", + properties: { + fraction: integer, + unsafe: integer, + overflow: number, + hex: integer, + nan: number, + infinity: number, + empty: number, + leadingZero: integer, + suffix: integer, + }, + }; + expect(coerceArgsToSchema(args, schema)).toEqual(args); + }); + + it("should not decode a value into a type the schema does not accept", () => { + const args = { + paths: '{"a": 1}', + config: "[1, 2]", + enabled: "yes", + toggle: "1", + count: "true", + broken: '["unterminated"', + }; + const schema = { + type: "object", + properties: { + paths: { type: "array" }, + config: { anyOf: [{ type: "object" }, { type: "null" }] }, + enabled: { type: "boolean" }, + toggle: { type: "boolean" }, + count: { type: "integer" }, + broken: { type: "array" }, + }, + }; + expect(coerceArgsToSchema(args, schema)).toEqual(args); + }); + + it("should resolve $ref, anyOf, oneOf and allOf", () => { + const args = { + config: '{"depth": 2, "targets": ["a"]}', + retries: "3", + mode: "[1]", + edits: '[{"oldText": "a", "newText": "b"}]', + }; + // Pydantic/FastMCP style: Optional[Model] -> anyOf [$ref, null] + const schema = { + type: "object", + $defs: { + ProbeConfig: { + type: "object", + properties: { depth: { type: "integer" } }, + }, + Edit: { type: "object" }, + }, + properties: { + config: { anyOf: [{ $ref: "#/$defs/ProbeConfig" }, { type: "null" }] }, + retries: { oneOf: [{ type: "integer" }, { type: "null" }] }, + mode: { allOf: [{ type: ["string", "array"] }, { type: "array" }] }, + edits: { type: "array", items: { $ref: "#/$defs/Edit" } }, + }, + }; + expect(coerceArgsToSchema(args, schema)).toEqual({ + config: { depth: 2, targets: ["a"] }, + retries: 3, + mode: [1], + edits: [{ oldText: "a", newText: "b" }], + }); + }); + + it("should leave values unchanged when a $ref cannot be resolved", () => { + const args = { missing: "[1]", remote: "[2]", cycle: "[3]", text: "[4]" }; + const schema = { + type: "object", + definitions: { + Loop: { $ref: "#/definitions/Loop" }, + Text: { type: "string" }, + }, + properties: { + missing: { $ref: "#/definitions/Missing" }, + remote: { $ref: "https://example.com/schema.json" }, + cycle: { $ref: "#/definitions/Loop" }, + text: { $ref: "#/definitions/Text" }, + }, + }; + expect(coerceArgsToSchema(args, schema)).toEqual(args); + }); + + it("should preserve sibling, optional and unknown arguments", () => { + const args = { + path: "frontend/src/App.jsx", + edits: '[{"oldText": "a", "newText": "b"}]', + dryRun: "false", + unknownJson: "[1, 2]", + nativeList: ["x"], + }; + const schema = { + type: "object", + properties: { + path: { type: "string" }, + edits: { type: "array" }, + dryRun: { type: "boolean" }, + nativeList: { type: "array" }, + optionalLimit: { type: "integer" }, + }, + required: ["path", "edits"], + }; + const result = coerceArgsToSchema(args, schema); + expect(result).toEqual({ + path: "frontend/src/App.jsx", + edits: [{ oldText: "a", newText: "b" }], + dryRun: false, + unknownJson: "[1, 2]", + nativeList: ["x"], + }); + expect(Object.keys(result)).toEqual(Object.keys(args)); + expect(result.nativeList).toBe(args.nativeList); + }); + + it("should coerce a streamed provider payload against a strict object schema", () => { + // Raw arguments string as streamed by an OpenAI-compatible provider + const toolCall: ToolCallDelta = { + id: "1", + function: { + name: "example_run_job", + arguments: + '{"settings": "{\\"enabled\\": true, \\"mode\\": \\"fast\\"}", "target": "{\\"id\\": \\"t1\\"}", "steps": "[{\\"name\\": \\"first\\", \\"retries\\": 2}]"}', + }, + }; + const schema = { + $schema: "https://json-schema.org/draft/2020-12/schema", + type: "object", + properties: { + settings: { type: "object" }, + target: { type: "object" }, + steps: { + type: "array", + items: { type: ["null", "boolean", "object", "array"], items: {} }, + }, + }, + required: ["settings", "target", "steps"], + additionalProperties: false, + }; + const result = coerceArgsToSchema(safeParseToolCallArgs(toolCall), schema); + expect(result).toEqual({ + settings: { enabled: true, mode: "fast" }, + target: { id: "t1" }, + steps: [{ name: "first", retries: 2 }], + }); + }); + + it("should give the same result for repeated calls without mutating inputs", () => { + const schema = { + type: "object", + $defs: { Limit: { anyOf: [{ type: "integer" }, { type: "null" }] } }, + properties: { + fields: { type: "array", items: { type: "string" } }, + limit: { $ref: "#/$defs/Limit" }, + path: { type: "string" }, + }, + }; + const schemaBefore = structuredClone(schema); + const calls = [ + { path: "a.txt", fields: '["id"]', limit: "5" }, + { path: "b.txt", fields: ["name"], limit: 7 }, + { path: "c.txt", fields: '["created", "updated"]', limit: "null" }, + ]; + const callsBefore = structuredClone(calls); + for (let round = 0; round < 2; round++) { + expect(calls.map((args) => coerceArgsToSchema(args, schema))).toEqual([ + { path: "a.txt", fields: ["id"], limit: 5 }, + { path: "b.txt", fields: ["name"], limit: 7 }, + { path: "c.txt", fields: ["created", "updated"], limit: null }, + ]); + } + expect(schema).toEqual(schemaBefore); + expect(calls).toEqual(callsBefore); + }); +}); + +describe("coerceArgsToSchema with adversarial schemas", () => { + it("should expand a shared $ref only once", () => { + // Each level references the next twice: 2^40 paths, 41 distinct schemas + const $defs: Record = { D40: { type: "array" } }; + for (let i = 0; i < 40; i++) { + $defs[`D${i}`] = { + anyOf: [{ $ref: `#/$defs/D${i + 1}` }, { $ref: `#/$defs/D${i + 1}` }], + }; + } + const schema = { + type: "object", + $defs, + properties: { list: { $ref: "#/$defs/D0" } }, + }; + expect(coerceArgsToSchema({ list: "[1]" }, schema)).toEqual({ list: [1] }); + }); + + it("should keep an argument whose schema exceeds the traversal budget", () => { + let nested: Record = { type: "array" }; + for (let i = 0; i < 20000; i++) { + nested = { allOf: [nested] }; + } + const $defs: Record = { C2000: { type: "array" } }; + for (let i = 0; i < 2000; i++) { + $defs[`C${i}`] = { $ref: `#/$defs/C${i + 1}` }; + } + const schema = { + type: "object", + $defs, + properties: { + nested, + chained: { $ref: "#/$defs/C0" }, + wide: { + anyOf: Array.from({ length: 5000 }, () => ({ type: "array" })), + }, + list: { type: "array" }, + }, + }; + const args = { nested: "[1]", chained: "[2]", wide: "[3]", list: "[4]" }; + // The budget is per argument, so list is still decoded + expect(coerceArgsToSchema(args, schema)).toEqual({ ...args, list: [4] }); + }); + + it("should keep arguments with recursive references", () => { + const schema = { + type: "object", + $defs: { + A: { anyOf: [{ $ref: "#/$defs/B" }, { type: "array" }] }, + B: { anyOf: [{ $ref: "#/$defs/A" }, { type: "null" }] }, + Self: { allOf: [{ $ref: "#/$defs/Self" }, { type: "array" }] }, + }, + properties: { + mutual: { $ref: "#/$defs/A" }, + self: { $ref: "#/$defs/Self" }, + }, + }; + const args = { mutual: "[1]", self: "[2]" }; + expect(coerceArgsToSchema(args, schema)).toEqual(args); + }); + + it("should not resolve references across nested schema resources", () => { + // Inside a schema with its own $id, "#/$defs/T" refers to that schema's + // T, which accepts strings, so these strings are already valid + const subResource = (id: string) => ({ + $id: id, + $defs: { T: { type: ["string", "array"] } }, + allOf: [{ $ref: "#/$defs/T" }], + }); + const schema = { + $id: "urn:example:root", + type: "object", + $defs: { + T: { type: "array" }, + Sub: subResource("urn:example:sub"), + Legacy: { id: "urn:example:legacy", type: "array" }, + }, + properties: { + embedded: subResource("urn:example:embedded"), + viaRef: { $ref: "#/$defs/Sub" }, + intoResource: { $ref: "#/$defs/Sub/$defs/T" }, + draft4: { $ref: "#/$defs/Legacy" }, + dynamic: { $dynamicRef: "#/$defs/T" }, + malformed: { $ref: 42 }, + rootRelative: { $ref: "#/$defs/T" }, + }, + }; + const args = { + embedded: "[1]", + viaRef: "[2]", + intoResource: "[3]", + draft4: "[4]", + dynamic: "[5]", + malformed: "[6]", + rootRelative: "[7]", + }; + // Only the reference resolved within the root resource is decoded + expect(coerceArgsToSchema(args, schema)).toEqual({ + ...args, + rootRelative: [7], + }); + }); +}); + describe("getNumberArg", () => { it("should return number argument when valid", () => { const args = { count: 42 };