fix(tools): advertise json and any params without an object type, and… · simstudioai/sim@72eef16 · GitHub
Skip to content

Commit 72eef16

Browse files
waleedlatif1waleedlatif1
authored
fix(tools): advertise json and any params without an object type, and name the invalid mcp argument (#9261)
* fix(tools): advertise json and any params without an object type, and name the invalid mcp argument * 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.
1 parent 4be882a commit 72eef16

6 files changed

Lines changed: 94 additions & 15 deletions

File tree

‎apps/sim/lib/mcp/application/execute-tool.test.ts‎

Lines changed: 31 additions & 1 deletion

‎apps/sim/lib/mcp/application/execute-tool.ts‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { createLogger } from '@sim/logger'
22
import { getErrorMessage } from '@sim/utils/errors'
3+
import type { ErrorObject } from 'ajv'
34
import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application'
45
import { OrchestrationError } from '@/lib/core/orchestration/types'
56
import { SIM_VIA_HEADER, serializeCallChain } from '@/lib/execution/call-chain'
@@ -94,8 +95,25 @@ export function coerceToolArguments(
9495

9596
/** Validates the complete discovered JSON Schema, including nested constraints and additional properties. */
9697
export function validateToolArguments(tool: McpTool, args: Record<string, unknown>): void {
97-
if (!compileMcpToolSchema(tool.inputSchema)(args))
98-
throw new OrchestrationError('validation', 'Invalid MCP operation arguments')
98+
const validate = compileMcpToolSchema(tool.inputSchema)
99+
if (validate(args)) return
100+
// Ajv stops at the first failure (no allErrors: unbounded work on untrusted input).
101+
const first = validate.errors?.[0]
102+
throw new OrchestrationError(
103+
'validation',
104+
first
105+
? `Invalid MCP operation arguments: ${describeArgumentError(first)}`
106+
: 'Invalid MCP operation arguments'
107+
)
108+
}
109+
110+
/** Names the failing argument so the caller can correct it; the schema and arguments are not secret. */
111+
function describeArgumentError(error: ErrorObject): string {
112+
const extra =
113+
error.keyword === 'additionalProperties' && typeof error.params.additionalProperty === 'string'
114+
? ` '${error.params.additionalProperty}'`
115+
: ''
116+
return [error.instancePath, `${error.message ?? 'is invalid'}${extra}`].filter(Boolean).join(' ')
99117
}
100118

101119
export function transformToolResult(result: McpToolResult): ExecuteMcpToolResult {

‎apps/sim/lib/mcp/client.ts‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -239,12 +239,11 @@ export class McpClient {
239239
logger.info(`[${this.config.name}] Registered tools/list_changed notification handler`)
240240
}
241241

242-
const serverVersion = this.client.getServerVersion()
243242
logger.info(`Successfully connected to MCP server: ${this.config.name}`, {
244243
...diagnostics,
245244
durationMs: Date.now() - startedAt,
246245
outcome: 'connected' satisfies ConnectionOutcome,
247-
protocolVersion: serverVersion,
246+
protocolVersion: this.getNegotiatedVersion(),
248247
})
249248
} catch (error) {
250249
this.isConnected = false
@@ -543,9 +542,9 @@ export class McpClient {
543542
}
544543
}
545544

545+
/** The protocol version agreed during initialize, which the transport records. */
546546
getNegotiatedVersion(): string | undefined {
547-
const serverVersion = this.client.getServerVersion()
548-
return typeof serverVersion === 'string' ? serverVersion : undefined
547+
return this.transport.protocolVersion
549548
}
550549

551550
getSessionId(): string | undefined {

‎apps/sim/lib/mcp/tool-schema.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ export function compileMcpToolSchema(schema: McpToolSchema): ValidateFunction {
1313
const validate = new Validator({
1414
strict: false,
1515
validateFormats: false,
16-
allErrors: false,
1716
}).compile(schema)
1817
if ('$async' in validate && validate.$async)
1918
throw new Error('Asynchronous schemas are not supported')

‎apps/sim/tools/params.test.ts‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import Ajv from 'ajv'
12
import { afterAll, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest'
23
import type { ExecutorDelegationOrigin } from '@/executor/types'
34
import { mergeToolParameters } from '@/tools/merge-params'
@@ -186,11 +187,42 @@ describe('Tool Parameters Utils', () => {
186187
const { schema } = await createLLMToolSchema(legacyJsonTool, {})
187188

188189
expect(schema.properties.payload).toEqual({
189-
type: 'object',
190190
description: 'Legacy JSON array payload',
191191
})
192192
})
193193

194+
it('lets a json or any param take an array as well as an object', async () => {
195+
const untypedTool = {
196+
...mockToolConfig,
197+
id: 'untyped_tool',
198+
params: {
199+
orgIds: {
200+
type: 'json',
201+
required: true,
202+
visibility: 'user-or-llm' as ParameterVisibility,
203+
description: 'Organization IDs, e.g. [129410, 1034157]',
204+
},
205+
value: {
206+
type: 'any',
207+
required: true,
208+
visibility: 'user-or-llm' as ParameterVisibility,
209+
},
210+
},
211+
}
212+
213+
const { schema: llmSchema } = await createLLMToolSchema(untypedTool, {})
214+
const userSchema = createUserToolSchema(untypedTool, { surface: 'copilot' })
215+
216+
for (const schema of [llmSchema, userSchema]) {
217+
const validate = new Ajv({ strict: false }).compile(schema)
218+
expect(validate({ orgIds: [129410, 1034157], value: ['a'] })).toBe(true)
219+
expect(validate({ orgIds: { id: 129410 }, value: { a: 1 } })).toBe(true)
220+
expect(schema.properties.orgIds.description).toBe(
221+
'Organization IDs, e.g. [129410, 1034157]'
222+
)
223+
}
224+
})
225+
194226
it.concurrent('should create schema excluding user-provided parameters', async () => {
195227
const userProvidedParams = {
196228
apiKey: 'user-provided-key',

‎apps/sim/tools/params.ts‎

Lines changed: 7 additions & 6 deletions

0 commit comments

Comments
 (0)