Skip to content

Commit 263ae42

Browse files
icecrasher321claude
andcommitted
revert(mcp): drop the enum-member recovery layer
Checked against the prod replica rather than reasoning about it, and every case this guarded has zero instances: - `workflow_mcp_tool`: 470 properties, 0 declare an `enum` at all. - MCP blocks' cached `_toolSchema`: 6 enums, all `type: 'string'` with string members. No untyped enum, no `null`/number/boolean member. - Stored MCP block arguments (179): 0 stringified booleans, 0 stringified nulls. Booleans and numbers are already persisted with their real types. - Agent MCP tool-row params (10,022): 0 stringified booleans, 0 stringified nulls, 4 stringified numbers — already covered by the `type: number` coercion that predates this branch. For a string enum `String(member) === value` returns the same string, so the whole path was a no-op on every row in production while adding a coercion at an authorized use case plus a validation widening to keep that coercion safe. The dropdown already persists the member (679325a), so nothing downstream has to reverse the encoding. `enumMemberShape` goes with it — every real enum declares a type, so member inference never runs. The declared-type-first ordering stays: it is a precedence fix, not a layer. `getJsonSchemaValueShape` is private again. `lib/mcp/application/execute-tool.ts` is byte-identical to 679325a. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a622de1 commit 263ae42

6 files changed

Lines changed: 32 additions & 235 deletions

File tree

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/mcp-dynamic-args/mcp-dynamic-args.tsx

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import { useActiveSearchTarget } from '@/app/workspace/[workspaceId]/w/[workflow
1212
import type { SubBlockConfig } from '@/blocks/types'
1313
import { useMcpTools } from '@/hooks/mcp/use-mcp-tools'
1414
import {
15-
decodeJsonSchemaValue,
1615
type JsonSchemaProperty,
1716
jsonSchemaType,
1817
subBlockTypeForJsonSchema,
@@ -236,13 +235,7 @@ export function McpDynamicArgs({
236235

237236
const renderParameterInput = (paramName: string, paramSchema: any) => {
238237
const current = currentArgs()
239-
// An argument entered before this control was derived from the NORMALIZED schema type
240-
// was collected by a text field, so it is stored as a string — `'false'` would tick the
241-
// switch it now renders as. Decoding on read normalizes it to the shape the control
242-
// expects; already-typed values pass through untouched, so this is a no-op thereafter.
243-
// This is the same decode `coerceToolArguments` applies at execution, so the control
244-
// shows what the server will actually receive.
245-
const value = decodeJsonSchemaValue(current[paramName], paramSchema)
238+
const value = current[paramName]
246239
const inputType = getInputType(paramSchema)
247240

248241
switch (inputType) {

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

Lines changed: 0 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -160,95 +160,6 @@ describe('executeMcpToolUseCase', () => {
160160
expect(mocks.telemetry).toHaveBeenCalledOnce()
161161
})
162162

163-
it('recovers enum members a dropdown stored as their stringified form', async () => {
164-
// The editor's dropdown option ids are `String(member)`, so an argument saved before
165-
// that control persisted real members holds text. Only the member list inverts it —
166-
// the declared-type branches cannot tell `'null'` the text from `null` the member.
167-
mocks.discoverServerTools.mockResolvedValueOnce([
168-
{
169-
name: 'lookup',
170-
inputSchema: {
171-
type: 'object',
172-
properties: {
173-
mode: { type: 'string', enum: ['fast', null] },
174-
level: { enum: [1, 2, 3] },
175-
verbose: { type: 'boolean', enum: [true, false] },
176-
},
177-
},
178-
},
179-
])
180-
181-
await executeMcpToolUseCase.execute({
182-
principal: PRINCIPAL,
183-
input: {
184-
workspaceId: WORKSPACE.workspaceId,
185-
serverId: SERVER.id,
186-
toolName: 'lookup',
187-
arguments: { mode: 'null', level: '2', verbose: 'false' },
188-
},
189-
})
190-
191-
expect(mocks.executeTool).toHaveBeenCalledWith(
192-
'user-1',
193-
SERVER.id,
194-
{ name: 'lookup', arguments: { mode: null, level: 2, verbose: false } },
195-
WORKSPACE.workspaceId,
196-
undefined,
197-
undefined,
198-
expect.anything()
199-
)
200-
})
201-
202-
it('accepts a declared enum member that satisfies no declared-type branch', async () => {
203-
// `mode` declares `type: 'string'`, so a recovered `null` member would fail every
204-
// type check. The enum is narrower than the type and decides on its own.
205-
mocks.discoverServerTools.mockResolvedValueOnce([
206-
{
207-
name: 'lookup',
208-
inputSchema: {
209-
type: 'object',
210-
properties: { mode: { type: 'string', enum: ['fast', null] } },
211-
},
212-
},
213-
])
214-
215-
await expect(
216-
executeMcpToolUseCase.execute({
217-
principal: PRINCIPAL,
218-
input: {
219-
workspaceId: WORKSPACE.workspaceId,
220-
serverId: SERVER.id,
221-
toolName: 'lookup',
222-
arguments: { mode: null },
223-
},
224-
})
225-
).resolves.toMatchObject({ success: true })
226-
})
227-
228-
it('still rejects a value the schema does not allow', async () => {
229-
mocks.discoverServerTools.mockResolvedValueOnce([
230-
{
231-
name: 'lookup',
232-
inputSchema: {
233-
type: 'object',
234-
properties: { mode: { type: 'string', enum: ['fast', null] } },
235-
},
236-
},
237-
])
238-
239-
await expect(
240-
executeMcpToolUseCase.execute({
241-
principal: PRINCIPAL,
242-
input: {
243-
workspaceId: WORKSPACE.workspaceId,
244-
serverId: SERVER.id,
245-
toolName: 'lookup',
246-
arguments: { mode: 42 },
247-
},
248-
})
249-
).rejects.toMatchObject({ code: 'validation' })
250-
})
251-
252163
it('rejects foreign or missing servers before permission and provider work', async () => {
253164
mocks.getServer.mockResolvedValueOnce(null)
254165

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

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,6 @@ import {
1616
McpToolsNotAllowedError,
1717
} from '@/ee/access-control/utils/permission-check'
1818
import type { ResolvedSecretTraceProvenanceV1 } from '@/executor/utils/resolved-secret-trace-registry'
19-
import {
20-
jsonSchemaEnumMember,
21-
jsonSchemaEnumMembers,
22-
jsonSchemaProperties,
23-
} from '@/tools/param-shape'
2419

2520
const logger = createLogger('McpToolExecution')
2621

@@ -52,22 +47,13 @@ function coerceToolArguments(
5247
input: Record<string, unknown>
5348
): Record<string, unknown> {
5449
const result = { ...input }
50+
if (!tool.inputSchema?.properties) return result
5551

56-
for (const [name, property] of jsonSchemaProperties(tool.inputSchema)) {
52+
for (const [name, property] of Object.entries(tool.inputSchema.properties)) {
53+
if (!hasType(property)) continue
5754
const value = result[name]
5855
if (value === undefined || value === null) continue
5956

60-
// An enum renders as a dropdown whose option ids are `String(member)`. Only the
61-
// member list inverts that, and it has to run before the declared-type branches
62-
// below, which never see an untyped property and cannot recover a `null` member.
63-
const member = jsonSchemaEnumMember(value, property)
64-
if (member !== undefined) {
65-
result[name] = member
66-
continue
67-
}
68-
69-
if (!hasType(property)) continue
70-
7157
if ((property.type === 'number' || property.type === 'integer') && typeof value === 'string') {
7258
const numberValue =
7359
property.type === 'integer' ? Number.parseInt(value) : Number.parseFloat(value)
@@ -112,14 +98,9 @@ function validateToolArguments(tool: McpTool, args: Record<string, unknown>): vo
11298
}
11399
}
114100

115-
for (const [name, property] of jsonSchemaProperties(schema)) {
101+
for (const [name, property] of Object.entries(schema.properties ?? {})) {
116102
const value = args[name]
117-
if (value === undefined) continue
118-
// `enum` is narrower than `type`, so a declared member is valid by definition. This
119-
// has to precede the type branches: a `null` member satisfies none of them, and
120-
// `coerceToolArguments` above has just recovered exactly that member.
121-
if (jsonSchemaEnumMembers(property)?.includes(value)) continue
122-
if (!hasType(property)) continue
103+
if (value === undefined || !hasType(property)) continue
123104
const isValid =
124105
(property.type === 'string' && typeof value === 'string') ||
125106
(property.type === 'number' && typeof value === 'number') ||

apps/sim/tools/param-shape.test.ts

Lines changed: 15 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,10 @@ import {
77
buildSubBlockForToolParam,
88
buildSubBlocksFromJsonSchema,
99
buildToolParamShapes,
10-
decodeJsonSchemaValue,
1110
decodeToolParams,
1211
decodeToolParamValue,
1312
encodeToolParamValue,
1413
expandSubBlockValueToParams,
15-
getJsonSchemaValueShape,
1614
getSubBlockValueShape,
1715
getToolParamValueShape,
1816
subBlockTypeForValueType,
@@ -530,87 +528,43 @@ describe('buildJsonSchemaParamShapes', () => {
530528
expect(shapes.get('declaredStr')).toBe('string')
531529
})
532530

533-
it('infers an untyped enum from its members', () => {
531+
it('reads an untyped enum from its members only to tell text from JSON', () => {
534532
const shapes = buildJsonSchemaParamShapes({
535-
properties: {
536-
nums: { enum: [1, 2] },
537-
bools: { enum: [true, false] },
538-
strs: { enum: ['x', 'y'] },
539-
structured: { enum: [{ a: 1 }] },
540-
mixed: { enum: [1, 'x'] },
541-
// `every` is vacuously true on an empty enum — this must not read as numeric.
542-
empty: { enum: [] },
543-
},
533+
properties: { primitive: { enum: ['x', 1, null] }, structured: { enum: [{ a: 1 }] } },
544534
})
545535

546-
expect(shapes.get('nums')).toBe('number')
547-
expect(shapes.get('bools')).toBe('boolean')
548-
expect(shapes.get('strs')).toBe('string')
536+
// A structured member renders in a JSON editor; anything else renders as a dropdown,
537+
// which stores text. The dropdown persists the member itself, so nothing has to
538+
// reverse `String(member)` afterwards.
539+
expect(shapes.get('primitive')).toBe('string')
549540
expect(shapes.get('structured')).toBe('json')
550-
expect(shapes.get('mixed')).toBe('string')
551-
expect(shapes.get('empty')).toBe('string')
552541
})
553542

554543
it('round-trips a numeric enum through the dropdown it renders as', () => {
555-
const property = { type: 'integer', enum: [1, 2, 3] }
556-
const [subBlock] = buildSubBlocksFromJsonSchema({ properties: { n: property } }, (id) => id)
544+
const schema = { properties: { n: { type: 'integer', enum: [1, 2, 3] } } }
545+
const [subBlock] = buildSubBlocksFromJsonSchema(schema, (id) => id)
557546

558547
expect(subBlock.type).toBe('dropdown')
559-
// The dropdown stores the stringified member; the shape decodes it back.
560-
expect(decodeToolParamValue('2', getJsonSchemaValueShape(property))).toBe(2)
561-
})
562-
563-
it('inverts the dropdown encoding against the enum members', () => {
564-
// Options are `String(member)`, and a shape cannot tell `'null'` the text from
565-
// `null` the member — only the member list can.
566-
const mixed = { enum: ['a', null, 2, true] }
567-
568-
expect(decodeJsonSchemaValue('null', mixed)).toBe(null)
569-
expect(decodeJsonSchemaValue('2', mixed)).toBe(2)
570-
expect(decodeJsonSchemaValue('true', mixed)).toBe(true)
571-
expect(decodeJsonSchemaValue('a', mixed)).toBe('a')
548+
expect(decodeToolParamValue('2', buildJsonSchemaParamShapes(schema).get('n')!)).toBe(2)
572549
})
573550

574551
it('normalizes a property the server sent as something other than an object', () => {
575-
// `properties: { foo: null }` is malformed but arrives over the wire, and the MCP
576-
// executor reads its properties through here before any type guard runs.
552+
// `properties: { foo: null }` is malformed but arrives over the wire.
577553
const shapes = buildJsonSchemaParamShapes({
578554
properties: { a: null, b: true, c: 'text', d: 42 },
579555
} as unknown as Parameters<typeof buildJsonSchemaParamShapes>[0])
580556

581557
expect([...shapes.values()]).toEqual(['string', 'string', 'string', 'string'])
582558
})
583559

584-
it('leaves a value that is not an enum member to the shape codec', () => {
585-
// A variable reference must survive, and an untouched field stays the '' sentinel.
586-
expect(decodeJsonSchemaValue('<start.choice>', { enum: ['a', null] })).toBe('<start.choice>')
587-
expect(decodeJsonSchemaValue('', { enum: ['a', null] })).toBe('')
588-
expect(decodeJsonSchemaValue('false', { type: 'boolean' })).toBe(false)
589-
})
590-
591560
it('normalizes a legacy string left by a control that has since changed type', () => {
592561
// A union-typed property used to render as a text field and now renders as a switch;
593562
// its stored 'false' must not tick the box.
594-
expect(
595-
decodeToolParamValue('false', getJsonSchemaValueShape({ type: ['boolean', 'null'] }))
596-
).toBe(false)
597-
expect(decodeToolParamValue(true, getJsonSchemaValueShape({ type: 'boolean' }))).toBe(true)
598-
})
599-
})
600-
601-
describe('enum members that are not plain strings', () => {
602-
it('keeps a nullable enum as text so a real null member survives the decode', () => {
603-
const shape = getJsonSchemaValueShape({ enum: ['a', null] })
604-
605-
expect(shape).toBe('string')
606-
// The dropdown persists the member itself, so `null` arrives already typed and the
607-
// decode must not touch it.
608-
expect(decodeToolParamValue(null, shape)).toBeNull()
609-
})
563+
const shapes = buildJsonSchemaParamShapes({
564+
properties: { flag: { type: ['boolean', 'null'] }, plain: { type: 'boolean' } },
565+
})
610566

611-
it('decodes a numeric enum member that is still stored as its stringified form', () => {
612-
expect(
613-
decodeToolParamValue('1', getJsonSchemaValueShape({ type: 'integer', enum: [1, 2] }))
614-
).toBe(1)
567+
expect(decodeToolParamValue('false', shapes.get('flag')!)).toBe(false)
568+
expect(decodeToolParamValue(true, shapes.get('plain')!)).toBe(true)
615569
})
616570
})

apps/sim/tools/param-shape.ts

Lines changed: 11 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -409,67 +409,26 @@ export function subBlockTypeForJsonSchema(property: JsonSchemaProperty): SubBloc
409409
* the control would answer `'string'` and the argument would reach the MCP server
410410
* undecoded. The same holds for a non-primitive enum, which renders as free text.
411411
*/
412-
export function getJsonSchemaValueShape(property: JsonSchemaProperty): ToolParamValueShape {
412+
function getJsonSchemaValueShape(property: JsonSchemaProperty): ToolParamValueShape {
413413
const type = jsonSchemaType(property)
414414
if (type === 'boolean') return 'boolean'
415415
if (type === 'number' || type === 'integer') return 'number'
416416
if (type === 'object' || type === 'array') return 'json'
417417
if (type === 'string') return 'string'
418418

419-
// No declared type leaves the enum members as the only signal. This runs AFTER the
420-
// declared type, not before: a dropdown stores `String(option)`, so a numeric enum
421-
// read as text would send `'1'` where the server expects `1`.
422-
if (Array.isArray(property.enum)) return enumMemberShape(property.enum)
423-
424-
return 'string'
425-
}
419+
// Read AFTER the declared type, not before: the dropdown an enum renders as stores
420+
// `String(option)`, so `{ type: 'integer', enum: [1, 2] }` read as text would send
421+
// `'1'` where the server expects `1`. With no declared type only a structured member
422+
// is informative — it renders as free JSON text rather than a dropdown.
423+
if (Array.isArray(property.enum)) {
424+
return property.enum.some((member) => member !== null && typeof member === 'object')
425+
? 'json'
426+
: 'string'
427+
}
426428

427-
/** The shape an enum's members share, for a property that declares no type. */
428-
function enumMemberShape(members: readonly unknown[]): ToolParamValueShape {
429-
// `every` is vacuously true on an empty enum, which a third-party MCP schema may send.
430-
if (members.length === 0) return 'string'
431-
if (members.some((member) => member !== null && typeof member === 'object')) return 'json'
432-
if (members.every((member) => typeof member === 'number')) return 'number'
433-
if (members.every((member) => typeof member === 'boolean')) return 'boolean'
434429
return 'string'
435430
}
436431

437-
/**
438-
* The enum member a dropdown's stored option id came from, or `undefined` for a value
439-
* that is not one of the members.
440-
*
441-
* Options are encoded as `String(member)` ({@link buildSubBlocksFromJsonSchema}), and
442-
* `String` is invertible only against the member list. A {@link ToolParamValueShape}
443-
* cannot recover a mixed enum: `['a', null]` shapes as `'string'`, so the selected
444-
* `null` reaches the server as the text `"null"`. Matching the stored text against the
445-
* members IS the exact inverse of the encoding, so it takes precedence over the shape.
446-
*
447-
* A value that matches nothing — a variable reference, an untouched field — is left for
448-
* the shape codec.
449-
*/
450-
export function jsonSchemaEnumMember(value: unknown, property: JsonSchemaProperty): unknown {
451-
if (typeof value !== 'string') return undefined
452-
return jsonSchemaEnumMembers(property)?.find((member) => String(member) === value)
453-
}
454-
455-
/** The members an untrusted property declares, or `undefined` when it declares none. */
456-
export function jsonSchemaEnumMembers(
457-
property: JsonSchemaProperty
458-
): readonly unknown[] | undefined {
459-
return Array.isArray(property.enum) ? property.enum : undefined
460-
}
461-
462-
/**
463-
* Decode a value stored by the control {@link subBlockTypeForJsonSchema} picked for a
464-
* JSON Schema property — the enum-aware counterpart of {@link decodeToolParamValue}.
465-
*/
466-
export function decodeJsonSchemaValue(value: unknown, property: JsonSchemaProperty): unknown {
467-
const member = jsonSchemaEnumMember(value, property)
468-
// Tested against `undefined`, never `??`: `null` is a legal member and must win.
469-
if (member !== undefined) return member
470-
return decodeToolParamValue(value, getJsonSchemaValueShape(property))
471-
}
472-
473432
/** The value shape of every argument an MCP or custom tool's schema declares. */
474433
export function buildJsonSchemaParamShapes(
475434
schema: JsonSchemaObject | undefined
@@ -504,7 +463,7 @@ export interface JsonSchemaObject {
504463
}
505464

506465
/** The declared properties of an untrusted schema, as `paramId -> property` pairs. */
507-
export function jsonSchemaProperties(
466+
function jsonSchemaProperties(
508467
schema: JsonSchemaObject | undefined
509468
): Array<[string, JsonSchemaProperty]> {
510469
const { properties } = schema ?? {}

bun.lock

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)