From 5be97f1fe21d22c51ce06a52e19bb06685675e10 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 20 Oct 2026 16:48:21 -0700 Subject: [PATCH 1/2] fix(tools): advertise json and any params without an object type, and name the invalid mcp argument --- .../lib/mcp/application/execute-tool.test.ts | 29 +++++++++++++++- apps/sim/lib/mcp/application/execute-tool.ts | 25 ++++++++++++-- apps/sim/lib/mcp/client.ts | 7 ++-- apps/sim/lib/mcp/tool-schema.ts | 2 +- apps/sim/tools/params.test.ts | 34 ++++++++++++++++++- apps/sim/tools/params.ts | 13 +++---- 6 files changed, 95 insertions(+), 15 deletions(-) diff --git a/apps/sim/lib/mcp/application/execute-tool.test.ts b/apps/sim/lib/mcp/application/execute-tool.test.ts index 4de6ad29275..14cd3842816 100644 --- a/apps/sim/lib/mcp/application/execute-tool.test.ts +++ b/apps/sim/lib/mcp/application/execute-tool.test.ts @@ -36,7 +36,7 @@ vi.mock('@/ee/access-control/utils/permission-check', () => permissionCheckMock) vi.mock('@/lib/mcp/service', () => mcpServiceMock) vi.mock('@/lib/core/telemetry', () => telemetryMock) -import { executeMcpToolUseCase } from '@/lib/mcp/application/execute-tool' +import { executeMcpToolUseCase, validateToolArguments } from '@/lib/mcp/application/execute-tool' const mocks = { assertPermissionsAllowed: permissionCheckMockFns.mockAssertPermissionsAllowed, @@ -391,3 +391,30 @@ describe('executeMcpToolUseCase', () => { expect(mocks.executeTool).not.toHaveBeenCalled() }) }) + +describe('validateToolArguments', () => { + const tool = { + name: 'search', + serverId: 'mcp-server-1', + serverName: 'Search', + inputSchema: { + type: 'object' as const, + required: ['query'], + additionalProperties: false, + properties: { + query: { type: 'string' }, + limit: { type: 'integer' }, + }, + }, + } + + it('names each invalid argument so the caller can correct the call', () => { + expect(() => validateToolArguments(tool, { limit: 'ten', extra: true })).toThrow( + "Invalid MCP operation arguments: must have required property 'query'; must NOT have additional properties 'extra'; /limit must be integer" + ) + }) + + it('accepts arguments the schema allows', () => { + expect(() => validateToolArguments(tool, { query: 'sim', limit: 5 })).not.toThrow() + }) +}) diff --git a/apps/sim/lib/mcp/application/execute-tool.ts b/apps/sim/lib/mcp/application/execute-tool.ts index 647917c7968..22e8a050359 100644 --- a/apps/sim/lib/mcp/application/execute-tool.ts +++ b/apps/sim/lib/mcp/application/execute-tool.ts @@ -1,5 +1,6 @@ import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' +import type { ErrorObject } from 'ajv' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { SIM_VIA_HEADER, serializeCallChain } from '@/lib/execution/call-chain' @@ -94,8 +95,28 @@ export function coerceToolArguments( /** Validates the complete discovered JSON Schema, including nested constraints and additional properties. */ export function validateToolArguments(tool: McpTool, args: Record): void { - if (!compileMcpToolSchema(tool.inputSchema)(args)) - throw new OrchestrationError('validation', 'Invalid MCP operation arguments') + const validate = compileMcpToolSchema(tool.inputSchema) + if (validate(args)) return + const details = (validate.errors ?? []) + .slice(0, MAX_REPORTED_ARGUMENT_ERRORS) + .map(describeArgumentError) + throw new OrchestrationError( + 'validation', + details.length > 0 + ? `Invalid MCP operation arguments: ${details.join('; ')}` + : 'Invalid MCP operation arguments' + ) +} + +const MAX_REPORTED_ARGUMENT_ERRORS = 3 + +/** Names the failing argument so the caller can correct it; the schema and arguments are not secret. */ +function describeArgumentError(error: ErrorObject): string { + const extra = + error.keyword === 'additionalProperties' && typeof error.params.additionalProperty === 'string' + ? ` '${error.params.additionalProperty}'` + : '' + return [error.instancePath, `${error.message ?? 'is invalid'}${extra}`].filter(Boolean).join(' ') } export function transformToolResult(result: McpToolResult): ExecuteMcpToolResult { diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 3f843362398..5e577fa94ce 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -239,12 +239,11 @@ export class McpClient { logger.info(`[${this.config.name}] Registered tools/list_changed notification handler`) } - const serverVersion = this.client.getServerVersion() logger.info(`Successfully connected to MCP server: ${this.config.name}`, { ...diagnostics, durationMs: Date.now() - startedAt, outcome: 'connected' satisfies ConnectionOutcome, - protocolVersion: serverVersion, + protocolVersion: this.getNegotiatedVersion(), }) } catch (error) { this.isConnected = false @@ -543,9 +542,9 @@ export class McpClient { } } + /** The protocol version agreed during initialize, which the transport records. */ getNegotiatedVersion(): string | undefined { - const serverVersion = this.client.getServerVersion() - return typeof serverVersion === 'string' ? serverVersion : undefined + return this.transport.protocolVersion } getSessionId(): string | undefined { diff --git a/apps/sim/lib/mcp/tool-schema.ts b/apps/sim/lib/mcp/tool-schema.ts index 302f42e0992..260bf00543a 100644 --- a/apps/sim/lib/mcp/tool-schema.ts +++ b/apps/sim/lib/mcp/tool-schema.ts @@ -13,7 +13,7 @@ export function compileMcpToolSchema(schema: McpToolSchema): ValidateFunction { const validate = new Validator({ strict: false, validateFormats: false, - allErrors: false, + allErrors: true, }).compile(schema) if ('$async' in validate && validate.$async) throw new Error('Asynchronous schemas are not supported') diff --git a/apps/sim/tools/params.test.ts b/apps/sim/tools/params.test.ts index 443106f4aa6..e00166e6e40 100644 --- a/apps/sim/tools/params.test.ts +++ b/apps/sim/tools/params.test.ts @@ -1,3 +1,4 @@ +import Ajv from 'ajv' import { afterAll, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest' import type { ExecutorDelegationOrigin } from '@/executor/types' import { mergeToolParameters } from '@/tools/merge-params' @@ -186,11 +187,42 @@ describe('Tool Parameters Utils', () => { const { schema } = await createLLMToolSchema(legacyJsonTool, {}) expect(schema.properties.payload).toEqual({ - type: 'object', description: 'Legacy JSON array payload', }) }) + it('lets a json or any param take an array as well as an object', async () => { + const untypedTool = { + ...mockToolConfig, + id: 'untyped_tool', + params: { + orgIds: { + type: 'json', + required: true, + visibility: 'user-or-llm' as ParameterVisibility, + description: 'Organization IDs, e.g. [129410, 1034157]', + }, + value: { + type: 'any', + required: true, + visibility: 'user-or-llm' as ParameterVisibility, + }, + }, + } + + const { schema: llmSchema } = await createLLMToolSchema(untypedTool, {}) + const userSchema = createUserToolSchema(untypedTool, { surface: 'copilot' }) + + for (const schema of [llmSchema, userSchema]) { + const validate = new Ajv({ strict: false }).compile(schema) + expect(validate({ orgIds: [129410, 1034157], value: ['a'] })).toBe(true) + expect(validate({ orgIds: { id: 129410 }, value: { a: 1 } })).toBe(true) + expect(schema.properties.orgIds.description).toBe( + 'Organization IDs, e.g. [129410, 1034157]' + ) + } + }) + it.concurrent('should create schema excluding user-provided parameters', async () => { const userProvidedParams = { apiKey: 'user-provided-key', diff --git a/apps/sim/tools/params.ts b/apps/sim/tools/params.ts index 8ff780bcc11..dc71edd7135 100644 --- a/apps/sim/tools/params.ts +++ b/apps/sim/tools/params.ts @@ -35,7 +35,8 @@ type ToolParamDefinition = ToolConfig['params'][string] type ToolInputBlockConfig = Pick interface SchemaProperty { - type: string + /** Absent for a `json` or `any` param, which accepts any JSON value. */ + type?: string description?: string items?: ToolParameterItemSchema properties?: Record @@ -162,13 +163,13 @@ function buildParameterSchema( : buildFileReferenceParameterSchema(param) } - let schemaType = param.type - if (schemaType === 'json' || schemaType === 'any') { - schemaType = 'object' - } + // `json` and `any` params take arrays as often as objects, so they carry no + // `type`: an `object` claim would contradict their descriptions and make a + // validating runtime reject a correct array argument. + const isUntyped = param.type === 'json' || param.type === 'any' const propertySchema: SchemaProperty = { - type: schemaType, + ...(isUntyped ? {} : { type: param.type }), description: param.description || '', ...(param.minItems !== undefined ? { minItems: param.minItems } : {}), ...(param.maxItems !== undefined ? { maxItems: param.maxItems } : {}), From 0ed54080a256254137c318f9f62cffd485571ddf Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 20 Oct 2026 17:01:11 -0700 Subject: [PATCH 2/2] fix(mcp): report the first invalid argument without collecting every error allErrors made Ajv collect every failure on untrusted input before the message kept three; validation stops at the first failure again and the message names it. --- apps/sim/lib/mcp/application/execute-tool.test.ts | 9 ++++++--- apps/sim/lib/mcp/application/execute-tool.ts | 11 ++++------- apps/sim/lib/mcp/tool-schema.ts | 1 - 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/apps/sim/lib/mcp/application/execute-tool.test.ts b/apps/sim/lib/mcp/application/execute-tool.test.ts index 14cd3842816..d4870e2a3f8 100644 --- a/apps/sim/lib/mcp/application/execute-tool.test.ts +++ b/apps/sim/lib/mcp/application/execute-tool.test.ts @@ -408,9 +408,12 @@ describe('validateToolArguments', () => { }, } - it('names each invalid argument so the caller can correct the call', () => { - expect(() => validateToolArguments(tool, { limit: 'ten', extra: true })).toThrow( - "Invalid MCP operation arguments: must have required property 'query'; must NOT have additional properties 'extra'; /limit must be integer" + it('names the invalid argument so the caller can correct the call', () => { + expect(() => validateToolArguments(tool, { query: 'sim', limit: 'ten' })).toThrow( + 'Invalid MCP operation arguments: /limit must be integer' + ) + expect(() => validateToolArguments(tool, { query: 'sim', extra: true })).toThrow( + "Invalid MCP operation arguments: must NOT have additional properties 'extra'" ) }) diff --git a/apps/sim/lib/mcp/application/execute-tool.ts b/apps/sim/lib/mcp/application/execute-tool.ts index 22e8a050359..538fa445a8b 100644 --- a/apps/sim/lib/mcp/application/execute-tool.ts +++ b/apps/sim/lib/mcp/application/execute-tool.ts @@ -97,19 +97,16 @@ export function coerceToolArguments( export function validateToolArguments(tool: McpTool, args: Record): void { const validate = compileMcpToolSchema(tool.inputSchema) if (validate(args)) return - const details = (validate.errors ?? []) - .slice(0, MAX_REPORTED_ARGUMENT_ERRORS) - .map(describeArgumentError) + // Ajv stops at the first failure (no allErrors: unbounded work on untrusted input). + const first = validate.errors?.[0] throw new OrchestrationError( 'validation', - details.length > 0 - ? `Invalid MCP operation arguments: ${details.join('; ')}` + first + ? `Invalid MCP operation arguments: ${describeArgumentError(first)}` : 'Invalid MCP operation arguments' ) } -const MAX_REPORTED_ARGUMENT_ERRORS = 3 - /** Names the failing argument so the caller can correct it; the schema and arguments are not secret. */ function describeArgumentError(error: ErrorObject): string { const extra = diff --git a/apps/sim/lib/mcp/tool-schema.ts b/apps/sim/lib/mcp/tool-schema.ts index 260bf00543a..43d65244f44 100644 --- a/apps/sim/lib/mcp/tool-schema.ts +++ b/apps/sim/lib/mcp/tool-schema.ts @@ -13,7 +13,6 @@ export function compileMcpToolSchema(schema: McpToolSchema): ValidateFunction { const validate = new Validator({ strict: false, validateFormats: false, - allErrors: true, }).compile(schema) if ('$async' in validate && validate.$async) throw new Error('Asynchronous schemas are not supported')