Skip to content

Commit d975ede

Browse files
committed
fix(mcp): address review on mcp output schema snapshots
- Read the MCP target with the block's saved canonical modes in lint, workflows deps and effective outputs, so a dormant basic/advanced member never selects or drops the snapshot. - Expand repeated bracket indices before the nested lint check, as the run resolves them. - Stop expanding a schema once its node budget is spent and leave that object open, instead of saving an untyped node per remaining field. - Keep every declared field name, constructor and __proto__ included. - Snapshot only blocks the edit changed, and discover best effort, so an unreachable server neither slows unrelated edits nor gets marked unhealthy.
1 parent 203e732 commit d975ede

9 files changed

Lines changed: 140 additions & 29 deletions

File tree

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ export const MCP_STRUCTURED_CONTENT_OUTPUT = 'structuredContent'
1313

1414
const MAX_SHAPE_DEPTH = 8
1515
const MAX_SHAPE_NODES = 500
16-
const UNSAFE_PROPERTY_NAMES = new Set(['__proto__', 'constructor', 'prototype'])
1716

1817
/**
1918
* The structure of a declared output schema: types, properties and array items
@@ -72,12 +71,14 @@ function reduceNode(node: unknown, depth: number, budget: { nodes: number }): Mc
7271
(node.additionalProperties !== undefined && node.additionalProperties !== false) ||
7372
node.patternProperties !== undefined
7473
if (open || !isRecordLike(node.properties)) return { type: 'object' }
75-
const properties: Record<string, McpToolOutputShape> = {}
74+
const entries: [string, McpToolOutputShape][] = []
7675
for (const [name, child] of Object.entries(node.properties)) {
77-
if (UNSAFE_PROPERTY_NAMES.has(name)) continue
78-
properties[name] = reduceNode(child, depth + 1, budget)
76+
/** Past the budget the object stays open: any field resolves, none is saved. */
77+
if (budget.nodes <= 0) return { type: 'object' }
78+
entries.push([name, reduceNode(child, depth + 1, budget)])
7979
}
80-
return { type: 'object', properties }
80+
/** `fromEntries` keeps every declared name, `__proto__` included, as an own field. */
81+
return { type: 'object', properties: Object.fromEntries(entries) }
8182
}
8283
case 'array':
8384
return isRecordLike(node.items)

‎apps/sim/lib/mothership/agent-cli/engines/deps.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { MCP_STRUCTURED_CONTENT_OUTPUT } from '@/lib/mcp/output-schema'
33
import { fetchWorkflowState } from '@/lib/mothership/agent-cli/engines/workflow-state'
44
import { type AgentCliEngine, agentCliFail, agentCliOk } from '@/lib/mothership/agent-cli/types'
55
import { getMcpStructuredContentOutputs } from '@/lib/workflows/blocks/outputs'
6+
import type { CanonicalModeOverrides } from '@/lib/workflows/subblocks/visibility'
67
import { TriggerUtils } from '@/lib/workflows/triggers/triggers'
78
import { normalizeName, SPECIAL_REFERENCE_PREFIXES } from '@/executor/constants'
89
import { createEnvVarPattern, createReferencePattern } from '@/executor/utils/reference-validation'
@@ -133,7 +134,9 @@ function fillDeclaredStructuredContent(
133134
block: Record<string, unknown> | undefined
134135
): void {
135136
if (block?.type !== 'mcp' || !Object.hasOwn(mock, MCP_STRUCTURED_CONTENT_OUTPUT)) return
136-
const outputs = getMcpStructuredContentOutputs(toRecord(block.subBlocks))
137+
// An invalid mode makes resolveMcpBlockConfig throw, which reads as no typed output.
138+
const modes = toRecord(toRecord(block.data).canonicalModes) as CanonicalModeOverrides
139+
const outputs = getMcpStructuredContentOutputs(toRecord(block.subBlocks), modes)
137140
const declared = outputs?.[MCP_STRUCTURED_CONTENT_OUTPUT]
138141
if (!declared) return
139142

‎apps/sim/lib/workflows/application/apply-workflow-operations.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -320,10 +320,14 @@ export const applyWorkflowOperations = defineAuthorizedWorkflowUseCase({
320320
input.blockEnabledChanges ?? [],
321321
skippedItems
322322
)
323-
modifiedGraph.blocks = await snapshotMcpToolOutputSchemas(enablement.blocks, {
324-
principal,
325-
workspaceId: context.workspaceId,
326-
})
323+
modifiedGraph.blocks = await snapshotMcpToolOutputSchemas(
324+
enablement.blocks,
325+
asGraph(baseGraph).blocks ?? {},
326+
{
327+
principal,
328+
workspaceId: context.workspaceId,
329+
}
330+
)
327331
input.checkAborted?.()
328332
const applied = appliedOperations + enablement.applied
329333

‎apps/sim/lib/workflows/application/mcp-output-schemas.test.ts‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ describe('snapshotMcpToolOutputSchemas', () => {
6666
toolSelector: 'add_note',
6767
}),
6868
},
69+
{},
6970
options
7071
)
7172

@@ -76,10 +77,23 @@ describe('snapshotMcpToolOutputSchemas', () => {
7677
expect(discover).toHaveBeenCalledOnce()
7778
expect(discover.mock.calls[0]?.[0]).toMatchObject({
7879
principal,
79-
input: { workspaceId: 'workspace-1', serverId: 'server-1' },
80+
input: { workspaceId: 'workspace-1', serverId: 'server-1', bestEffort: true },
8081
})
8182
})
8283

84+
it('skips a block the edit did not touch, even one with no snapshot yet', async () => {
85+
const untouched = mcpBlock('a', {
86+
operation: 'run',
87+
serverSelector: 'server-1',
88+
toolSelector: 'on_call',
89+
})
90+
const input = { a: untouched }
91+
expect(
92+
await snapshotMcpToolOutputSchemas(input, { a: structuredClone(untouched) }, options)
93+
).toBe(input)
94+
expect(discover).not.toHaveBeenCalled()
95+
})
96+
8397
it('rediscovers a block whose operation changed since its snapshot', async () => {
8498
const stale = createMcpToolOutputSnapshot('server-1', 'add_note', undefined)
8599
const blocks = await snapshotMcpToolOutputSchemas(
@@ -91,6 +105,7 @@ describe('snapshotMcpToolOutputSchemas', () => {
91105
toolOutputSchema: stale,
92106
}),
93107
},
108+
{},
94109
options
95110
)
96111
expect(snapshotOf(blocks, 'a')).toMatchObject({ tool: 'on_call', schema: { type: 'object' } })
@@ -118,7 +133,7 @@ describe('snapshotMcpToolOutputSchemas', () => {
118133
['no operation yet', { operation: 'run', serverSelector: 'server-1', toolSelector: '' }],
119134
])('leaves %s alone without discovering', async (_, values) => {
120135
const input = { a: mcpBlock('a', values) }
121-
const blocks = await snapshotMcpToolOutputSchemas(input, options)
136+
const blocks = await snapshotMcpToolOutputSchemas(input, {}, options)
122137
expect(blocks).toBe(input)
123138
expect(discover).not.toHaveBeenCalled()
124139
})
@@ -137,6 +152,7 @@ describe('snapshotMcpToolOutputSchemas', () => {
137152
{ tool: 'advanced' }
138153
),
139154
},
155+
{},
140156
options
141157
)
142158
expect(snapshotOf(blocks, 'a')).toMatchObject({ tool: 'on_call' })
@@ -147,12 +163,12 @@ describe('snapshotMcpToolOutputSchemas', () => {
147163
const failed = {
148164
a: mcpBlock('a', { operation: 'run', serverSelector: 's', toolSelector: 'x' }),
149165
}
150-
expect(await snapshotMcpToolOutputSchemas(failed, options)).toEqual(failed)
166+
expect(await snapshotMcpToolOutputSchemas(failed, {}, options)).toEqual(failed)
151167

152168
const unlisted = {
153169
a: mcpBlock('a', { operation: 'run', serverSelector: 'server-1', toolSelector: 'gone' }),
154170
}
155-
expect(await snapshotMcpToolOutputSchemas(unlisted, options)).toEqual(unlisted)
171+
expect(await snapshotMcpToolOutputSchemas(unlisted, {}, options)).toEqual(unlisted)
156172
})
157173

158174
it('discovers a managed connection through its own use case', async () => {
@@ -164,6 +180,7 @@ describe('snapshotMcpToolOutputSchemas', () => {
164180
{
165181
a: mcpBlock('a', { operation: 'run', serverSelector: connection, toolSelector: 'on_call' }),
166182
},
183+
{},
167184
options
168185
)
169186
expect(discover).not.toHaveBeenCalled()

‎apps/sim/lib/workflows/application/mcp-output-schemas.ts‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,17 +85,24 @@ async function discoverTools(
8585
discoverMcpServerToolsUseCase,
8686
{ mcpServerId: serverId }
8787
),
88-
input: { workspaceId, serverId, signal },
88+
// Best effort: a failure here never marks the server unhealthy or starts its shared cooldown.
89+
input: { workspaceId, serverId, signal, bestEffort: true },
8990
})
9091
return tools
9192
}
9293

94+
/** The block's resolved target and fields as an edit can change them, to tell an edited block from an untouched one. */
95+
function editFingerprint(block: BlockState | undefined): string {
96+
return JSON.stringify([block?.type, block?.subBlocks ?? {}, block?.data?.canonicalModes ?? {}])
97+
}
98+
9399
/**
94-
* Records, on each MCP block an edit left targeting a literal server and
95-
* operation, the reply shape that operation declares, so the block's
100+
* Records, on each MCP block this edit changed and left targeting a literal
101+
* server and operation, the reply shape that operation declares, so the block's
96102
* `structuredContent` output is typed for lint, references, `workflows deps` and
97-
* the run. Blocks whose snapshot already matches their target are not
98-
* rediscovered; one per server is discovered, through the same authorized use
103+
* the run. Blocks the edit did not touch, and blocks whose snapshot already
104+
* matches their target, are not rediscovered, so an unreachable server never
105+
* slows an edit that leaves its block alone; one per server is discovered, through the same authorized use
99106
* case the editor's tool list uses, as the editing principal.
100107
*
101108
* Best effort: a server that cannot be discovered, or an operation it does not
@@ -104,9 +111,11 @@ async function discoverTools(
104111
*/
105112
export async function snapshotMcpToolOutputSchemas(
106113
blocks: Record<string, BlockState>,
114+
baseBlocks: Record<string, BlockState>,
107115
options: SnapshotOptions
108116
): Promise<Record<string, BlockState>> {
109117
const targets = Object.entries(blocks).flatMap(([blockId, block]) => {
118+
if (editFingerprint(block) === editFingerprint(baseBlocks[blockId])) return []
110119
const target = staleSnapshotTarget(blockId, block)
111120
return target ? [target] : []
112121
})

‎apps/sim/lib/workflows/blocks/outputs.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { resolveMcpBlockConfig } from '@/lib/mcp/workflow-config'
1414
import { getJevAnswerOutput } from '@/lib/workflows/blocks/jev-outputs'
1515
import { normalizeInputFormatValue } from '@/lib/workflows/input-format'
1616
import { containsReference } from '@/lib/workflows/sanitization/references'
17+
import type { CanonicalModeOverrides } from '@/lib/workflows/subblocks/visibility'
1718
import { classifyStartBlockType, StartBlockPath } from '@/lib/workflows/triggers/triggers'
1819
import { TRIGGER_TYPES } from '@/lib/workflows/triggers/types'
1920
import {
@@ -47,6 +48,8 @@ interface EffectiveOutputOptions {
4748
triggerMode?: boolean
4849
preferToolOutputs?: boolean
4950
includeHidden?: boolean
51+
/** The block's saved canonical modes, which pick the active member of a basic/advanced pair. */
52+
canonicalModes?: CanonicalModeOverrides
5053
}
5154

5255
type ConditionValue = string | number | boolean
@@ -461,7 +464,8 @@ function toOutputNode(shape: McpToolOutputShape): DeclaredOutputNode {
461464
* targets now applies; anything else leaves the block's outputs as declared.
462465
*/
463466
export function getMcpStructuredContentOutputs(
464-
subBlocks?: Record<string, unknown>
467+
subBlocks?: Record<string, unknown>,
468+
canonicalModes?: CanonicalModeOverrides
465469
): OutputDefinition | undefined {
466470
if (!subBlocks) return undefined
467471
const values = Object.fromEntries(
@@ -471,7 +475,7 @@ export function getMcpStructuredContentOutputs(
471475
if (!snapshot?.schema) return undefined
472476
let target: ReturnType<typeof resolveMcpBlockConfig>
473477
try {
474-
target = resolveMcpBlockConfig(values)
478+
target = resolveMcpBlockConfig(values, canonicalModes)
475479
} catch {
476480
return undefined
477481
}
@@ -540,7 +544,7 @@ export function getEffectiveBlockOutputs(
540544
}
541545

542546
if (blockType === 'mcp' && !triggerMode) {
543-
const structuredOutputs = getMcpStructuredContentOutputs(subBlocks)
547+
const structuredOutputs = getMcpStructuredContentOutputs(subBlocks, options?.canonicalModes)
544548
if (structuredOutputs) return { ...baseOutputs, ...structuredOutputs }
545549
}
546550

‎apps/sim/lib/workflows/editing/dangling-refs.test.ts‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ function graph(
7878
type?: string
7979
name?: string
8080
triggerMode?: boolean
81+
data?: { canonicalModes?: Record<string, 'basic' | 'advanced'> }
8182
subBlocks?: Record<string, { value?: unknown }>
8283
}
8384
>
@@ -287,15 +288,21 @@ describe('collectDanglingBlockOutputReferences', () => {
287288
},
288289
}
289290

290-
function mcpGraph(reference: string, snapshot?: unknown) {
291+
function mcpGraph(
292+
reference: string,
293+
snapshot?: unknown,
294+
extra: { toolReference?: string; canonicalModes?: Record<string, 'basic' | 'advanced'> } = {}
295+
) {
291296
return graph({
292297
oncall: {
293298
type: 'mcp',
294299
name: 'On call',
300+
...(extra.canonicalModes ? { data: { canonicalModes: extra.canonicalModes } } : {}),
295301
subBlocks: {
296302
operation: { value: 'run' },
297303
serverSelector: { value: 'server-1' },
298304
toolSelector: { value: 'on_call' },
305+
...(extra.toolReference ? { toolReference: { value: extra.toolReference } } : {}),
299306
...(snapshot === undefined ? {} : { toolOutputSchema: { value: snapshot } }),
300307
},
301308
},
@@ -340,6 +347,41 @@ describe('collectDanglingBlockOutputReferences', () => {
340347
expect(finding?.reason).toContain('structuredContent.person.id')
341348
})
342349

350+
it('reads the operation from the saved mode, not a dormant member', () => {
351+
expect(
352+
collectDanglingBlockOutputReferences(
353+
mcpGraph('<oncall.structuredContent.role>', declared, {
354+
toolReference: 'add_note',
355+
canonicalModes: { tool: 'basic' },
356+
})
357+
)
358+
).toEqual([])
359+
})
360+
361+
it('checks repeated array indices the way the run resolves them', () => {
362+
const grid = createMcpToolOutputSnapshot('server-1', 'on_call', {
363+
type: 'object',
364+
properties: {
365+
rows: {
366+
type: 'array',
367+
items: {
368+
type: 'array',
369+
items: { type: 'object', properties: { id: { type: 'string' } } },
370+
},
371+
},
372+
},
373+
})
374+
expect(
375+
collectDanglingBlockOutputReferences(
376+
mcpGraph('<oncall.structuredContent.rows[0][1].id>', grid)
377+
)
378+
).toEqual([])
379+
const [finding] = collectDanglingBlockOutputReferences(
380+
mcpGraph('<oncall.structuredContent.rows[0][1].nope>', grid)
381+
)
382+
expect(finding?.reason).toContain('Declared fields: structuredContent.rows.id')
383+
})
384+
343385
it('keeps flagging structuredContent when the operation declares no schema', () => {
344386
for (const snapshot of [
345387
undefined,

‎apps/sim/lib/workflows/editing/lint.ts‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
getMcpStructuredContentOutputs,
1212
getResponseFormatOutputs,
1313
} from '@/lib/workflows/blocks/outputs'
14+
import type { CanonicalModeOverrides } from '@/lib/workflows/subblocks/visibility'
1415
import { getBlock } from '@/blocks'
1516
import {
1617
isTriggerBlockType,
@@ -20,6 +21,7 @@ import {
2021
} from '@/executor/constants'
2122
import { isPathInSchema } from '@/executor/utils/block-reference'
2223
import { collectStringLeaves } from '@/executor/utils/reference-validation'
24+
import { splitLeadingBracketPath } from '@/executor/variables/resolvers/reference'
2325
import {
2426
collectBlockFieldIssues,
2527
extractBlockParams,
@@ -34,6 +36,7 @@ type BlockState = {
3436
name?: string
3537
triggerMode?: boolean
3638
subBlocks?: Record<string, { value?: unknown } | undefined>
39+
data?: { canonicalModes?: CanonicalModeOverrides }
3740
}
3841

3942
type EdgeState = {
@@ -643,6 +646,7 @@ function declaredOutputKeys(block: BlockState): string[] | undefined {
643646
triggerMode: false,
644647
preferToolOutputs: true,
645648
includeHidden: true,
649+
canonicalModes: block.data?.canonicalModes,
646650
})
647651
)
648652
if (keys.length === 0) return undefined
@@ -666,10 +670,8 @@ function declaredChildPaths(outputs: Record<string, unknown>, prefix: string[]):
666670
node = toRecordOrNull(node?.items)
667671
continue
668672
}
669-
const field = part.replace(INDEX_SUFFIX, '')
670-
node = toRecordOrNull(toRecordOrNull(node?.properties)?.[field])
671-
if (field !== part) node = toRecordOrNull(node?.items)
672-
named.push(field)
673+
node = toRecordOrNull(toRecordOrNull(node?.properties)?.[part])
674+
named.push(part)
673675
}
674676
if (node?.type === 'array') node = toRecordOrNull(node.items)
675677
return Object.keys(toRecordOrNull(node?.properties) ?? {}).map((name) =>
@@ -689,9 +691,15 @@ function undeclaredNestedPath(
689691
token: string
690692
): UndeclaredNestedPath | undefined {
691693
if (block?.type !== 'mcp') return undefined
692-
const parts = token.split('.').slice(1)
694+
const parts = token
695+
.split('.')
696+
.slice(1)
697+
.flatMap((part) => {
698+
const { property, pathParts } = splitLeadingBracketPath(part)
699+
return [property, ...pathParts]
700+
})
693701
if (parts[0] !== MCP_STRUCTURED_CONTENT_OUTPUT || parts.length < 2) return undefined
694-
const outputs = getMcpStructuredContentOutputs(subBlockValues(block))
702+
const outputs = getMcpStructuredContentOutputs(subBlockValues(block), block.data?.canonicalModes)
695703
if (!outputs || isPathInSchema(outputs, parts)) return undefined
696704
let valid = 1
697705
while (valid < parts.length && isPathInSchema(outputs, parts.slice(0, valid + 1))) valid++

‎apps/sim/serializer/mcp-output-schema.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,29 @@ describe('MCP block output typed from a declared output schema', () => {
9090
expect(resolve(declared, ['content', '0', 'text'])).toBe('{"role":"sales-ops"}')
9191
})
9292

93+
it('keeps declared fields whose names are object built-ins', () => {
94+
const snapshot = createMcpToolOutputSnapshot(
95+
'server-1',
96+
'on_call',
97+
JSON.parse(
98+
'{"type":"object","properties":{"constructor":{"type":"string"},"__proto__":{"type":"number"}}}'
99+
)
100+
)
101+
expect(Object.keys(snapshot.schema?.properties ?? {})).toEqual(['constructor', '__proto__'])
102+
})
103+
104+
it('leaves a schema wider than the snapshot budget open instead of saving every field', () => {
105+
const wide = createMcpToolOutputSnapshot('server-1', 'on_call', {
106+
type: 'object',
107+
properties: Object.fromEntries(
108+
Array.from({ length: 2_000 }, (_, index) => [`field${index}`, { type: 'string' }])
109+
),
110+
})
111+
expect(wide.schema).toEqual({ type: 'object' })
112+
expect(resolve(wide, ['structuredContent', 'role'])).toBe('sales-ops')
113+
expect(resolve(wide, ['structuredContent', 'anything'])).toBeUndefined()
114+
})
115+
93116
it('carries the schema structure without description strings into block params', () => {
94117
const params = serializeMcpBlock(declared).config.params
95118
expect(params.toolOutputSchema).toEqual(declared)

0 commit comments

Comments
 (0)