From 5e2bdc8167a4ef2a802b6d7443f32e5522ae9064 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Wed, 2 Sep 2026 02:27:48 -0700 Subject: [PATCH] fix(agent): ignore empty advanced MCP bindings --- .../handlers/agent/agent-handler.test.ts | 26 +++++++++++++++++++ .../executor/handlers/agent/agent-handler.ts | 2 ++ .../mothership/mothership-handler.test.ts | 19 ++++++++++++++ .../handlers/mothership/mothership-handler.ts | 3 ++- apps/sim/lib/mcp/shared.test.ts | 11 +++++++- apps/sim/lib/mcp/shared.ts | 3 ++- 6 files changed, 61 insertions(+), 3 deletions(-) diff --git a/apps/sim/executor/handlers/agent/agent-handler.test.ts b/apps/sim/executor/handlers/agent/agent-handler.test.ts index 1932e0f5fed..820c30217de 100644 --- a/apps/sim/executor/handlers/agent/agent-handler.test.ts +++ b/apps/sim/executor/handlers/agent/agent-handler.test.ts @@ -3779,6 +3779,32 @@ describe('AgentBlockHandler', () => { ]) }) + it('does not create tools for a blank advanced MCP server binding', async () => { + await handler.execute( + { + ...mockContext, + workspaceId: 'test-workspace-123', + workflowId: 'test-workflow-456', + }, + mockBlock, + { + model: 'gpt-4o', + userPrompt: 'Continue without MCP tools', + apiKey: 'test-api-key', + tools: [ + { + type: 'mcp-server-advanced', + params: { serverId: '' }, + usageControl: 'auto' as const, + }, + ], + } + ) + + expect(mockDiscoverMcpServerToolsAsExecutor).not.toHaveBeenCalled() + expect(mockExecuteProviderRequest.mock.calls[0][1].tools).toEqual([]) + }) + describe('customToolId resolution - DB as source of truth', () => { const staleInlineSchema = { function: { diff --git a/apps/sim/executor/handlers/agent/agent-handler.ts b/apps/sim/executor/handlers/agent/agent-handler.ts index 00a7dfc196f..ca3098dde42 100644 --- a/apps/sim/executor/handlers/agent/agent-handler.ts +++ b/apps/sim/executor/handlers/agent/agent-handler.ts @@ -803,6 +803,8 @@ export class AgentBlockHandler implements BlockHandler { if (entry.tool.type === 'mcp') { mcpTools.push(entry) } else if (entry.tool.type === MCP_SERVER_ADVANCED_TOOL_TYPE) { + const serverId = entry.tool.params?.serverId + if (typeof serverId === 'string' && !serverId.trim()) continue advancedMcpServers.push(entry) } else { otherTools.push(entry) diff --git a/apps/sim/executor/handlers/mothership/mothership-handler.test.ts b/apps/sim/executor/handlers/mothership/mothership-handler.test.ts index 98a90bd6823..858bd02788d 100644 --- a/apps/sim/executor/handlers/mothership/mothership-handler.test.ts +++ b/apps/sim/executor/handlers/mothership/mothership-handler.test.ts @@ -1030,6 +1030,25 @@ describe('MothershipBlockHandler', () => { ]) }) + it('does not forward tools for a blank advanced MCP server binding', async () => { + fetchMock.mockResolvedValue(createJsonResponse({ content: 'done', toolCalls: [] })) + + await handler.execute(context, block, { + prompt: 'Continue without MCP tools', + tools: [ + { + type: 'mcp-server-advanced', + params: { serverId: '' }, + usageControl: 'auto', + }, + ], + }) + + expect(mockDiscoverMcpServerToolsAsExecutor).not.toHaveBeenCalled() + const [, options] = fetchMock.mock.calls[0] as [string, RequestInit] + expect(JSON.parse(String(options.body))).not.toHaveProperty('mcpTools') + }) + it('does not scan arbitrary Mothership metadata, attachment names, or payloads', async () => { const secret = 'boundary-secret' const registry = new ResolvedSecretTraceRegistry([ diff --git a/apps/sim/executor/handlers/mothership/mothership-handler.ts b/apps/sim/executor/handlers/mothership/mothership-handler.ts index bf2b275ed59..0d758bfd1f4 100644 --- a/apps/sim/executor/handlers/mothership/mothership-handler.ts +++ b/apps/sim/executor/handlers/mothership/mothership-handler.ts @@ -161,9 +161,10 @@ async function expandMothershipMcpTools( throw new Error('MCP Server (Advanced) requires params.serverId') } const serverId = candidate.params.serverId - if (typeof serverId !== 'string' || !serverId.trim()) { + if (typeof serverId !== 'string') { throw new Error('MCP Server (Advanced) requires params.serverId') } + if (!serverId.trim()) return [] const usageControl: 'auto' | 'force' = candidate.usageControl === 'force' ? 'force' : 'auto' return [{ serverId, usageControl }] } diff --git a/apps/sim/lib/mcp/shared.test.ts b/apps/sim/lib/mcp/shared.test.ts index 91ccaecb54f..2de40f97495 100644 --- a/apps/sim/lib/mcp/shared.test.ts +++ b/apps/sim/lib/mcp/shared.test.ts @@ -46,9 +46,18 @@ describe('assertValidMcpServerToolBindings', () => { ).not.toThrow() }) + it('ignores server-wide bindings with blank server IDs', () => { + expect(() => + assertValidMcpServerToolBindings([ + { type: 'mcp-server-advanced', params: { serverId: '' } }, + { type: 'mcp-server-advanced', params: { serverId: ' ' } }, + ]) + ).not.toThrow() + }) + it('fails fast on a malformed active server-wide binding', () => { expect(() => - assertValidMcpServerToolBindings([{ type: 'mcp-server-advanced', params: { serverId: '' } }]) + assertValidMcpServerToolBindings([{ type: 'mcp-server-advanced', params: {} }]) ).toThrow('requires params.serverId') }) }) diff --git a/apps/sim/lib/mcp/shared.ts b/apps/sim/lib/mcp/shared.ts index 7e0430cc793..522fe514e52 100644 --- a/apps/sim/lib/mcp/shared.ts +++ b/apps/sim/lib/mcp/shared.ts @@ -49,9 +49,10 @@ export function assertValidMcpServerToolBindings(value: unknown): void { } if (tool.type !== MCP_SERVER_ADVANCED_TOOL_TYPE) continue const serverId = tool.params?.serverId - if (typeof serverId !== 'string' || !serverId.trim()) { + if (typeof serverId !== 'string') { throw new Error('MCP Server (Advanced) requires params.serverId') } + if (!serverId.trim()) continue if (advancedServerIds.has(serverId)) { throw new Error(`Duplicate MCP Server (Advanced) binding for ${serverId}`) }