From ab652c949f0818d40c4dcc1189df2e136756986e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 20 Oct 2026 16:45:27 -0700 Subject: [PATCH] fix(mothership): keep integration discovery working when a tagged mcp server fails or lists no tools buildTaggedMcpToolSchemas discovered tagged servers with Promise.all and threw when a server listed no tools, and the integration catalog includes MCP on every unfiltered request. One deleted, disabled, unreachable, truncated or tool-less /-tagged server therefore failed every unfiltered integrations list, grep and search_integration_tools call for the rest of the chat. Discovery now settles per server and returns { tools, failures }. The catalog drops failed servers from unfiltered and query results, logging each server id and reason, and still throws that server's own error when the request targets it (service mcp:, or a tool id on it that no healthy server provides). The MCP permission-group check still runs once up front and still throws. Also removes the unread defer_loading/executeLocally/params fields on MCP catalog operations, the test-only buildSelectedMcpToolSchemas and buildOrganizationTaggedMcpToolSchemas with their tests and stale mocks, and corrects the post.ts comment that said tagged MCP tools ride the tool array. --- .../app/api/mothership/execute/route.test.ts | 14 -- apps/sim/lib/mothership/chat/payload.test.ts | 8 - apps/sim/lib/mothership/chat/post.ts | 11 +- .../mothership/chat/process-contents.test.ts | 2 +- .../integrations/application/catalog.test.ts | 97 ++++++++++-- .../integrations/application/catalog.ts | 18 ++- apps/sim/lib/mothership/mcp-tools.test.ts | 135 +++-------------- apps/sim/lib/mothership/mcp-tools.ts | 139 +++++------------- 8 files changed, 166 insertions(+), 258 deletions(-) diff --git a/apps/sim/app/api/mothership/execute/route.test.ts b/apps/sim/app/api/mothership/execute/route.test.ts index 8f691810e30..7783c11eae8 100644 --- a/apps/sim/app/api/mothership/execute/route.test.ts +++ b/apps/sim/app/api/mothership/execute/route.test.ts @@ -23,14 +23,10 @@ import { createMockRequest } from '@sim/testing/mocks/request.mock' import { beforeEach, describe, expect, it, vi } from 'vitest' const { - mockBuildSelectedMcpToolSchemas, - mockBuildTaggedMcpToolSchemas, mockComputeWorkspaceEntitlements, mockProcessContextsServer, mockRequestExplicitStreamAbort, } = vi.hoisted(() => ({ - mockBuildSelectedMcpToolSchemas: vi.fn(), - mockBuildTaggedMcpToolSchemas: vi.fn(), mockComputeWorkspaceEntitlements: vi.fn(), mockProcessContextsServer: vi.fn(), mockRequestExplicitStreamAbort: vi.fn(), @@ -53,11 +49,6 @@ vi.mock('@/lib/mothership/entitlements', () => ({ computeWorkspaceEntitlements: mockComputeWorkspaceEntitlements, })) -vi.mock('@/lib/mothership/mcp-tools', () => ({ - buildSelectedMcpToolSchemas: mockBuildSelectedMcpToolSchemas, - buildTaggedMcpToolSchemas: mockBuildTaggedMcpToolSchemas, -})) - vi.mock('@/lib/mothership/request/lifecycle/headless', () => mothershipHeadlessLifecycleMock) vi.mock('@/lib/mothership/request/session/explicit-abort', () => ({ @@ -144,8 +135,6 @@ describe('mothership private trace provenance transport', () => { decryptionFailures: [], }) mockBuildIntegrationToolSchemas.mockResolvedValue([]) - mockBuildSelectedMcpToolSchemas.mockResolvedValue([]) - mockBuildTaggedMcpToolSchemas.mockResolvedValue([]) mockComputeWorkspaceEntitlements.mockResolvedValue([]) mockDecryptSecret.mockResolvedValue({ decrypted: 'secret-value' }) mockProcessContextsServer.mockResolvedValue([]) @@ -484,9 +473,6 @@ describe('mothership private trace provenance transport', () => { }) it('sends enabled MCP IDs without schemas or activating matching configured secrets', async () => { - mockBuildTaggedMcpToolSchemas.mockResolvedValueOnce([ - { name: 'mcp-docs', description: 'Uses secret-value' }, - ]) mockRunHeadlessCopilotLifecycle.mockImplementation( async (payload: Record, options: CopilotLifecycleOptions) => { const registry = diff --git a/apps/sim/lib/mothership/chat/payload.test.ts b/apps/sim/lib/mothership/chat/payload.test.ts index 1151409d15d..61fef4c6fcc 100644 --- a/apps/sim/lib/mothership/chat/payload.test.ts +++ b/apps/sim/lib/mothership/chat/payload.test.ts @@ -112,11 +112,6 @@ permissionGroupsResolveMockFns.mockGetUserPermissionConfigForOrganization.mockIm vi.mock('@/lib/billing/plan-helpers', () => billingPlanHelpersMock) -vi.mock('@/lib/mothership/mcp-tools', () => ({ - buildTaggedMcpToolSchemas: vi.fn(async () => []), - buildOrganizationTaggedMcpToolSchemas: vi.fn(async () => []), -})) - vi.mock('@/lib/workflows/utils', () => workflowsUtilsMock) /** Denied-operation projection walks the block map only for blocks the mocked tool list never names. */ @@ -891,9 +886,7 @@ describe('desktop request capabilities', () => { it('carries only enabled MCP IDs without eager catalog discovery while preserving desktop capabilities', async () => { const { getBlockVisibilityForCopilot } = await import('@/lib/mothership/block-visibility') - const { buildOrganizationTaggedMcpToolSchemas } = await import('@/lib/mothership/mcp-tools') vi.mocked(getBlockVisibilityForCopilot).mockClear() - vi.mocked(buildOrganizationTaggedMcpToolSchemas).mockClear() const principal = createSessionPrincipal({ userId: 'user-org' }) const payload = await buildCopilotRequestPayload({ message: 'Use my tools', @@ -909,7 +902,6 @@ it('carries only enabled MCP IDs without eager catalog discovery while preservin terminalCapable: true, }) expect(getBlockVisibilityForCopilot).not.toHaveBeenCalled() - expect(buildOrganizationTaggedMcpToolSchemas).not.toHaveBeenCalled() expect(payload.integrationCatalog).toEqual({ mcpServerIds: ['server-a', 'server-b'] }) expect(payload).not.toHaveProperty('integrationTools') expect(payload).not.toHaveProperty('mothershipTools') diff --git a/apps/sim/lib/mothership/chat/post.ts b/apps/sim/lib/mothership/chat/post.ts index 075740beab9..1796faf074c 100644 --- a/apps/sim/lib/mothership/chat/post.ts +++ b/apps/sim/lib/mothership/chat/post.ts @@ -514,11 +514,12 @@ function normalizeContexts(contexts: UnifiedChatRequest['contexts']) { * reloads and reopened chats with no extra state to keep in sync. There is * deliberately no off switch: history is append-only. * - * Only the ids travel forward, not the contexts themselves. The tools ride the - * tool array on every turn, so the model always sees their names and schemas; - * re-expanding the prompt listing each turn would just duplicate that. Keeping - * inherited servers out of the persisted contexts also keeps the `/name` chips - * on a sent message showing only what the user actually typed that turn. + * Only the ids travel forward, not the contexts themselves. The ids scope the + * request's integration catalog, and the agent discovers these servers' + * operations on demand, so re-expanding the prompt listing each turn adds + * nothing. Keeping inherited servers out of the persisted contexts also keeps + * the `/name` chips on a sent message showing only what the user actually typed + * that turn. */ function collectChatMcpServerIds( chatMcpServerIds: string[], diff --git a/apps/sim/lib/mothership/chat/process-contents.test.ts b/apps/sim/lib/mothership/chat/process-contents.test.ts index 511f6be2a16..7536d364a32 100644 --- a/apps/sim/lib/mothership/chat/process-contents.test.ts +++ b/apps/sim/lib/mothership/chat/process-contents.test.ts @@ -635,7 +635,7 @@ describe('processContextsServer - MCP contexts', () => { }, ]) expect(discoverServerTools).not.toHaveBeenCalled() - const catalog = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1']) + const { tools: catalog } = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1']) expect(discoverServerTools).toHaveBeenCalledTimes(1) expect(discoverServerTools).toHaveBeenCalledWith({ principal: expect.objectContaining({ diff --git a/apps/sim/lib/mothership/integrations/application/catalog.test.ts b/apps/sim/lib/mothership/integrations/application/catalog.test.ts index 78ef337b5c8..60e2f310a00 100644 --- a/apps/sim/lib/mothership/integrations/application/catalog.test.ts +++ b/apps/sim/lib/mothership/integrations/application/catalog.test.ts @@ -10,6 +10,10 @@ import { mothershipWorkspaceTargetMock, mothershipWorkspaceTargetMockFns, } from '@sim/testing/mocks/mothership-workspace-target.mock' +import { + permissionCheckMock, + permissionCheckMockFns, +} from '@sim/testing/mocks/permission-check.mock' import { permissionGroupsResolveMock, permissionGroupsResolveMockFns, @@ -19,6 +23,7 @@ import { workspaceContextMockFns, } from '@sim/testing/mocks/workspace-context.mock' import { beforeEach, describe, expect, it, vi } from 'vitest' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { createTrustedCopilotPrincipal, createTrustedOrganizationCopilotPrincipal, @@ -30,9 +35,11 @@ import { readIntegrationCatalog, } from '@/lib/mothership/integrations/application/catalog' -const hoisted = vi.hoisted(() => ({ mcp: vi.fn(), flag: vi.fn(async () => true) })) +const hoisted = vi.hoisted(() => ({ flag: vi.fn(async () => true) })) const mocks = { ...hoisted, + discover: mcpUseCasesMockFns.mockDiscoverMcpServerToolsUseCase, + mcpPermission: permissionCheckMockFns.mockAssertPermissionsAllowed, build: mothershipChatPayloadMockFns.mockBuildIntegrationToolSchemas, banned: authBanMockFns.mockGetActivelyBannedUserIds, target: mothershipWorkspaceTargetMockFns.mockResolveInvocationWorkspace, @@ -42,7 +49,8 @@ const mocks = { } vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock) vi.mock('@/lib/mothership/chat/payload', () => mothershipChatPayloadMock) -vi.mock('@/lib/mothership/mcp-tools', () => ({ buildTaggedMcpToolSchemas: hoisted.mcp })) +vi.mock('@/lib/internal/mcp/discover-tools', () => ({ discoverMcpServerToolsAsExecutor: vi.fn() })) +vi.mock('@/ee/access-control/utils/permission-check', () => permissionCheckMock) vi.mock('@/lib/mothership/application/workspace-target', () => mothershipWorkspaceTargetMock) vi.mock('@/lib/workspaces/application/context', () => workspaceContextMock) vi.mock('@/lib/auth/ban', () => authBanMock) @@ -64,6 +72,9 @@ const tools = [ input_schema: { type: 'object' }, }, ] +function discoveredServerIds() { + return mocks.discover.mock.calls.map(([args]) => args.input.serverId) +} function principal() { return createTrustedOrganizationCopilotPrincipal( { userId: 'actor', organizationId: 'org-1', chatId: 'chat-1', delegationId: 'catalog-1' }, @@ -82,7 +93,9 @@ beforeEach(() => { mocks.banned.mockResolvedValue([]) mocks.config.mockResolvedValue(null) mocks.build.mockResolvedValue([...tools]) - mocks.mcp.mockResolvedValue([]) + mocks.discover.mockReset() + mocks.discover.mockResolvedValue({ tools: [] }) + mocks.mcpPermission.mockResolvedValue(undefined) mocks.listServers.mockResolvedValue({ servers: [{ id: 'mcp-abc' }] }) }) describe('integration catalog projection', () => { @@ -238,7 +251,9 @@ describe('catalog authorization', () => { it('does not discover MCP operations in Search even with selected servers', async () => { queueChat() - mocks.mcp.mockResolvedValue([{ ...tools[0], name: 'mcp-abc-send', service: 'mcp:mcp-abc' }]) + mocks.discover.mockResolvedValue({ + tools: [{ serverId: 'mcp-abc', name: 'send', description: 'Send', inputSchema: {} }], + }) const result = await readIntegrationCatalog.execute({ principal: principal(), input: { ...input, service: 'mcp:mcp-abc', mcpServerIds: ['mcp-abc'] }, @@ -297,10 +312,12 @@ it.each([undefined, 'mcp-abc-write'])( async (toolId) => { queueChat('agent') mocks.target.mockResolvedValue({ workspaceId: 'workspace-1' }) - mocks.mcp.mockResolvedValue([ - { name: 'mcp-abc-read', service: 'mcp:mcp-abc', description: 'Read', input_schema: {} }, - { name: 'mcp-abc-write', service: 'mcp:mcp-abc', description: 'Write', input_schema: {} }, - ]) + mocks.discover.mockResolvedValue({ + tools: [ + { serverId: 'mcp-abc', name: 'read', description: 'Read', inputSchema: {} }, + { serverId: 'mcp-abc', name: 'write', description: 'Write', inputSchema: {} }, + ], + }) const result = await readIntegrationCatalog.execute({ principal: principal(), input: { @@ -312,7 +329,7 @@ it.each([undefined, 'mcp-abc-write'])( ...(toolId ? { toolId } : {}), }, }) - expect(mocks.mcp).toHaveBeenCalledWith('actor', 'workspace-1', ['mcp-abc'], undefined) + expect(discoveredServerIds()).toEqual(['mcp-abc']) expect(result.operations.map((operation) => operation.toolId)).toEqual( toolId ? [] : ['mcp-abc-read'] ) @@ -325,7 +342,7 @@ it('keeps native discovery available in organization chats with tagged MCP serve input: { ...input, mode: 'agent', mcpServerIds: ['mcp-abc'], toolId: 'gmail_send' }, }) expect(result.operations.map((operation) => operation.toolId)).toEqual(['gmail_send']) - expect(mocks.mcp).not.toHaveBeenCalled() + expect(mocks.discover).not.toHaveBeenCalled() }) it('requires an explicit workspace target for organization MCP discovery', async () => { queueChat('agent') @@ -335,7 +352,7 @@ it('requires an explicit workspace target for organization MCP discovery', async input: { ...input, mode: 'agent', mcpServerIds: ['mcp-abc'], toolId: 'mcp-abc-read' }, }) ).rejects.toThrow('explicit workspace ID') - expect(mocks.mcp).not.toHaveBeenCalled() + expect(mocks.discover).not.toHaveBeenCalled() }) it('ranks query tokens across operation names, descriptions, and service while prioritizing exact IDs', () => { @@ -381,7 +398,7 @@ it('filters organization enabled servers to the authorized target before broad M principal: expect.objectContaining({ workspaceId: 'workspace-1', subjectUserId: 'actor' }), input: { workspaceId: 'workspace-1' }, }) - expect(mocks.mcp).toHaveBeenCalledWith('actor', 'workspace-1', ['mcp-abc'], undefined) + expect(discoveredServerIds()).toEqual(['mcp-abc']) }) it('removes previously discoverable Search operations when the runtime flag turns off', async () => { @@ -394,3 +411,59 @@ it('removes previously discoverable Search operations when the runtime flag turn ) } }) + +describe('tagged MCP servers that cannot be discovered', () => { + const failures: Record Promise | Promise<{ tools: never[] }>> = { + 'mcp-deleted': async () => { + throw new OrchestrationError('not_found', 'MCP server not found') + }, + 'mcp-down': async () => { + throw new Error('connect ECONNREFUSED') + }, + 'mcp-empty': async () => ({ tools: [] }), + } + const serverIds = ['mcp-abc', ...Object.keys(failures)] + function catalog(request: Partial) { + queueChat('agent') + return readIntegrationCatalog.execute({ + principal: principal(), + input: { + ...input, + mode: 'agent', + workspaceId: 'workspace-1', + mcpServerIds: serverIds, + ...request, + }, + }) + } + beforeEach(() => { + mocks.target.mockResolvedValue({ workspaceId: 'workspace-1' }) + mocks.listServers.mockResolvedValue({ servers: serverIds.map((id) => ({ id })) }) + mocks.discover.mockImplementation(async ({ input: { serverId } }) => { + const failure = failures[serverId] + if (failure) return failure() + return { tools: [{ serverId, name: 'send', description: 'Send a note', inputSchema: {} }] } + }) + }) + + it.each([undefined, 'send'])( + 'still lists native and healthy MCP operations (query: %s)', + async (query) => { + const result = await catalog(query ? { query } : {}) + expect(result.operations.map((operation) => operation.toolId)).toEqual( + expect.arrayContaining(['gmail_send', 'slack_send', 'mcp-abc-send']) + ) + expect(result.operations).toHaveLength(3) + expect(discoveredServerIds().sort()).toEqual([...serverIds].sort()) + } + ) + + it.each([ + ['mcp-deleted', 'MCP server not found'], + ['mcp-down', 'connect ECONNREFUSED'], + ['mcp-empty', 'No permitted MCP operations are available for mcp-empty'], + ])('fails a request that targets %s with its own error', async (serverId, message) => { + await expect(catalog({ service: `mcp:${serverId}` })).rejects.toThrow(message) + await expect(catalog({ toolId: `${serverId}-send` })).rejects.toThrow(message) + }) +}) diff --git a/apps/sim/lib/mothership/integrations/application/catalog.ts b/apps/sim/lib/mothership/integrations/application/catalog.ts index 491c86a6407..793ed6503ae 100644 --- a/apps/sim/lib/mothership/integrations/application/catalog.ts +++ b/apps/sim/lib/mothership/integrations/application/catalog.ts @@ -7,6 +7,7 @@ import { resolvePrincipalSubjectUserId, } from '@sim/auth/principal' import { INTEGRATION_METADATA } from '@sim/deployment-config/integration-metadata' +import { createLogger } from '@sim/logger' import { stripVersionSuffix } from '@sim/utils/string' import { defineWorkspaceOperation } from '@/lib/core/application' import { defineOrganizationOperation } from '@/lib/core/application/organization-operation' @@ -33,6 +34,8 @@ import type { import { buildTaggedMcpToolSchemas } from '@/lib/mothership/mcp-tools' import { resolveActiveWorkspaceApplicationContext } from '@/lib/workspaces/application/context' +const logger = createLogger('IntegrationCatalog') + export const INTEGRATION_CATALOG_AUDIENCE = 'sim:integration-catalog' export const readIntegrationCatalogOperation = defineWorkspaceOperation({ id: 'mothership.integrations.catalog', @@ -227,12 +230,25 @@ const catalogUseCase = defineAuthorizedChatUseCase({ const executorContext = input.mcpExecution ? catalogExecutorContext(input, context.userId, workspaceId) : undefined - const mcpTools = await buildTaggedMcpToolSchemas( + const { tools: mcpTools, failures } = await buildTaggedMcpToolSchemas( context.userId, workspaceId, candidates, executorContext ) + const targetFailure = failures.find( + ({ serverId }) => + input.service === `mcp:${serverId}` || + (input.toolId?.startsWith(`${serverId}-`) && + !mcpTools.some((tool) => tool.name === input.toolId)) + ) + if (targetFailure) throw targetFailure.error + for (const { serverId, error } of failures) { + logger.warn('Omitting an undiscoverable tagged MCP server from the integration catalog', { + serverId, + reason: error.message, + }) + } tools.push( ...mcpTools.filter( (tool) => selectedIds.has(tool.name) || serverIds.has(tool.service?.slice(4) ?? '') diff --git a/apps/sim/lib/mothership/mcp-tools.test.ts b/apps/sim/lib/mothership/mcp-tools.test.ts index f58fb64258e..51b5264210b 100644 --- a/apps/sim/lib/mothership/mcp-tools.test.ts +++ b/apps/sim/lib/mothership/mcp-tools.test.ts @@ -1,9 +1,4 @@ -import { createSessionPrincipal } from '@sim/testing/factories/principal.factory' import { mcpUseCasesMock, mcpUseCasesMockFns } from '@sim/testing/mocks/mcp-use-cases.mock' -import { - mothershipWorkspaceTargetMock, - mothershipWorkspaceTargetMockFns, -} from '@sim/testing/mocks/mothership-workspace-target.mock' import { permissionCheckMock, permissionCheckMockFns, @@ -17,19 +12,12 @@ const { discoverServerTools } = vi.hoisted(() => ({ vi.mock('@/lib/internal/mcp/discover-tools', () => ({ discoverMcpServerToolsAsExecutor: discoverServerTools, })) -vi.mock('@/lib/mothership/application/workspace-target', () => mothershipWorkspaceTargetMock) vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock) vi.mock('@/ee/access-control/utils/permission-check', () => permissionCheckMock) -import { - buildOrganizationTaggedMcpToolSchemas, - buildSelectedMcpToolSchemas, - buildTaggedMcpToolSchemas, -} from '@/lib/mothership/mcp-tools' +import { buildTaggedMcpToolSchemas } from '@/lib/mothership/mcp-tools' const assertPermissionsAllowed = permissionCheckMockFns.mockAssertPermissionsAllowed -const getServer = mcpUseCasesMockFns.mockGetMcpServerUseCase -const resolveTarget = mothershipWorkspaceTargetMockFns.mockResolveInvocationWorkspace mcpUseCasesMockFns.mockDiscoverMcpServerToolsUseCase.mockImplementation(async (args: unknown) => ({ tools: await discoverServerTools(args), })) @@ -50,7 +38,7 @@ describe('mothership MCP tool schemas', () => { }, ]) - const tools = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1']) + const { tools, failures } = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1']) expect(discoverServerTools).toHaveBeenCalledTimes(1) expect(discoverServerTools).toHaveBeenCalledWith( @@ -59,114 +47,33 @@ describe('mothership MCP tool schemas', () => { }) ) expect(tools).toEqual([ - expect.objectContaining({ - name: 'mcp-server-1-search', - // Explicitly enabled by the user, so it is callable without an unlock step. - defer_loading: false, - executeLocally: false, - params: expect.objectContaining({ - mothershipToolKind: 'mcp', - mothershipToolName: 'mcp-server-1-search', - serverId: 'mcp-server-1', - toolName: 'search', - }), - }), - ]) - }) - - it('rediscovers a selected block tool even with a cached schema', async () => { - discoverServerTools.mockResolvedValue([ { - serverId: 'mcp-server-1', - name: 'search', - inputSchema: { type: 'object', properties: { query: { type: 'string' } } }, + name: 'mcp-server-1-search', + description: 'Search docs', + input_schema: { type: 'object', properties: { query: { type: 'string' } } }, + service: 'mcp:mcp-server-1', }, ]) - const tools = await buildSelectedMcpToolSchemas( - 'user-1', - 'ws-1', - [ - { - type: 'mcp', - params: { serverId: 'mcp-server-1', toolName: 'search', serverName: 'Docs' }, - schema: { type: 'object', properties: { query: { type: 'string' } } }, - }, - ], - { workflowId: 'workflow-1', workspaceId: 'ws-1', mcpBlockId: 'block-1' } - ) + expect(failures).toEqual([]) + }) - expect(discoverServerTools).toHaveBeenCalledOnce() - expect(tools[0]).toMatchObject({ - name: 'mcp-server-1-search', - input_schema: { type: 'object', properties: { query: { type: 'string' } } }, + it('reports a failing or tool-less server without discarding the other servers', async () => { + discoverServerTools.mockImplementation(async ({ input: { serverId } }) => { + if (serverId === 'mcp-down') throw new Error('connect ECONNREFUSED') + if (serverId === 'mcp-empty') return [] + return [{ serverId, name: 'search', inputSchema: { type: 'object' } }] }) - }) - it('discovers a selected legacy tool without a cached schema', async () => { - discoverServerTools.mockResolvedValueOnce([ - { - serverId: 'mcp-server-1', - name: 'search', - inputSchema: { type: 'object' }, - }, + const discovery = await buildTaggedMcpToolSchemas('user-1', 'ws-1', [ + 'mcp-down', + 'mcp-server-1', + 'mcp-empty', ]) - const tools = await buildSelectedMcpToolSchemas( - 'user-1', - 'ws-1', - [ - { - type: 'mcp', - params: { serverId: 'mcp-server-1', toolName: 'search' }, - }, - ], - { workflowId: 'workflow-1', workspaceId: 'ws-1', mcpBlockId: 'block-1' } - ) - - expect(discoverServerTools).toHaveBeenCalledWith( - expect.objectContaining({ serverId: 'mcp-server-1', workspaceId: 'ws-1' }) - ) - expect(tools[0]).toMatchObject({ name: 'mcp-server-1-search' }) - }) -}) - -describe('organization tagged MCP targets', () => { - const owner = { userId: 'user-1', organizationId: 'org-1', chatId: 'chat-1' } - const principal = createSessionPrincipal() - beforeEach(() => { - getServer.mockImplementation(async ({ input }) => ({ - server: { workspaceId: input.serverId === 'server-a' ? 'workspace-a' : 'workspace-b' }, - })) - resolveTarget.mockResolvedValue({}) - assertPermissionsAllowed.mockResolvedValue(undefined) - discoverServerTools.mockImplementation(async ({ input }) => [ - { serverId: input.serverId, name: 'search', inputSchema: { type: 'object' } }, + expect(discovery.tools.map((tool) => tool.name)).toEqual(['mcp-server-1-search']) + expect(discovery.failures.map(({ serverId, error }) => [serverId, error.message])).toEqual([ + ['mcp-down', 'connect ECONNREFUSED'], + ['mcp-empty', 'No permitted MCP operations are available for mcp-empty'], ]) }) - it('retains distinct callable schemas for tagged servers in independently authorized targets', async () => { - const tools = await buildOrganizationTaggedMcpToolSchemas(principal, owner, [ - 'server-a', - 'server-b', - 'server-a', - ]) - expect(getServer).toHaveBeenCalledTimes(2) - expect(resolveTarget).toHaveBeenCalledWith(owner, 'workspace-a') - expect(resolveTarget).toHaveBeenCalledWith(owner, 'workspace-b') - expect(tools.map((tool) => tool.name)).toEqual(['mcp-server-a-search', 'mcp-server-b-search']) - expect(tools[0]?.description).toContain('workspace-a') - expect(tools[1]?.description).toContain('workspace-b') - expect(discoverServerTools).toHaveBeenCalledTimes(2) - }) - it('does not discover remote schemas after current target access is denied', async () => { - resolveTarget.mockRejectedValueOnce(new Error('target denied')) - await expect( - buildOrganizationTaggedMcpToolSchemas(principal, owner, ['server-a']) - ).rejects.toThrow('target denied') - expect(discoverServerTools).not.toHaveBeenCalled() - }) - it('does not enumerate untagged servers', async () => { - expect(await buildOrganizationTaggedMcpToolSchemas(principal, owner, [])).toEqual([]) - expect(getServer).not.toHaveBeenCalled() - expect(discoverServerTools).not.toHaveBeenCalled() - }) }) diff --git a/apps/sim/lib/mothership/mcp-tools.ts b/apps/sim/lib/mothership/mcp-tools.ts index 4ecb2b217b8..1e778d1b43f 100644 --- a/apps/sim/lib/mothership/mcp-tools.ts +++ b/apps/sim/lib/mothership/mcp-tools.ts @@ -1,16 +1,24 @@ -import type { Principal } from '@sim/auth/principal' +import { toError } from '@sim/utils/errors' import { discoverMcpServerToolsAsExecutor } from '@/lib/internal/mcp/discover-tools' import type { InternalToolOperationContext } from '@/lib/internal/tool-operations/types' import { MCP_SERVER_DELEGATION_AUDIENCE } from '@/lib/mcp/application/authorization' -import { discoverMcpServerToolsUseCase, getMcpServerUseCase } from '@/lib/mcp/application/use-cases' -import { resolveMcpToolBinding } from '@/lib/mcp/tool-binding' -import type { McpTool, McpToolSchema } from '@/lib/mcp/types' +import { discoverMcpServerToolsUseCase } from '@/lib/mcp/application/use-cases' +import type { McpToolSchema } from '@/lib/mcp/types' import { createMcpToolId } from '@/lib/mcp/utils' -import { resolveInvocationWorkspace } from '@/lib/mothership/application/workspace-target' import { createCopilotChatPrincipal } from '@/lib/mothership/auth/application-delegation' import type { ToolSchema } from '@/lib/mothership/chat/payload' import { assertPermissionsAllowed } from '@/ee/access-control/utils/permission-check' -import type { ToolInput } from '@/executor/handlers/agent/types' + +interface TaggedMcpServerFailure { + serverId: string + error: Error +} + +interface TaggedMcpDiscovery { + tools: ToolSchema[] + /** Servers that could not be discovered or listed no permitted operations. */ + failures: TaggedMcpServerFailure[] +} function toMothershipMcpTool(tool: { serverId: string @@ -19,54 +27,32 @@ function toMothershipMcpTool(tool: { description?: string inputSchema: McpToolSchema | Record }): ToolSchema { - const callableName = createMcpToolId(tool.serverId, tool.name) return { - name: callableName, + name: createMcpToolId(tool.serverId, tool.name), description: tool.description || `MCP tool ${tool.name} from ${tool.serverName || tool.serverId}`, input_schema: tool.inputSchema, - // Not deferred: deferral keeps the 200+ unrequested integration schemas out - // of the tool array, but an MCP server is explicitly enabled by the user and - // stays enabled for the chat. Deferring it means every turn after the first - // needs a load_custom_tool round-trip the model has no listing to prompt it - // for, and a direct call is rejected as unavailable. - defer_loading: false, - executeLocally: false, service: `mcp:${tool.serverId}`, - params: { - mothershipToolKind: 'mcp', - mothershipToolName: callableName, - mothershipToolTitle: tool.serverName ? `${tool.serverName}: ${tool.name}` : tool.name, - serverId: tool.serverId, - toolName: tool.name, - }, } } -function dedupeMcpTools(tools: ToolSchema[]): ToolSchema[] { - const seen = new Set() - return tools.filter((tool) => { - if (seen.has(tool.name)) return false - seen.add(tool.name) - return true - }) -} - /** - * Resolves every tool from explicitly tagged MCP servers into request-local, - * deferred tool schemas. Untagged workspace servers are never inspected. + * Resolves every tool from explicitly tagged MCP servers into request-local + * catalog operations. Untagged workspace servers are never inspected. Each + * server settles on its own: one that cannot be discovered, or lists no + * permitted operations, is reported in `failures` without discarding the others. */ export async function buildTaggedMcpToolSchemas( userId: string, workspaceId: string, serverIds: string[], context?: InternalToolOperationContext -): Promise { +): Promise { const uniqueServerIds = [...new Set(serverIds.filter(Boolean))] - if (uniqueServerIds.length === 0) return [] + if (uniqueServerIds.length === 0) return { tools: [], failures: [] } await assertPermissionsAllowed({ userId, workspaceId, toolKind: 'mcp' }) - const discovered = await Promise.all( + const outcomes = await Promise.allSettled( uniqueServerIds.map(async (serverId) => { const tools = context ? await discoverMcpServerToolsAsExecutor({ workspaceId, context, serverId }) @@ -84,72 +70,19 @@ export async function buildTaggedMcpToolSchemas( return tools }) ) - return dedupeMcpTools(discovered.flat().map(toMothershipMcpTool)) -} -/** - * Resolves selected MCP tools through authorized discovery, including saved legacy bindings. - */ -export async function buildSelectedMcpToolSchemas( - userId: string, - workspaceId: string, - selections: ToolInput[], - context?: InternalToolOperationContext -): Promise { - const selected = selections.filter( - (tool) => tool.type === 'mcp' && (tool.usageControl || 'auto') !== 'none' - ) - if (selected.length === 0) return [] - - await assertPermissionsAllowed({ userId, workspaceId, toolKind: 'mcp' }) - const discoveredByServer = new Map>() - const resolved = await Promise.all( - selected.map(async (selection) => { - const { serverId, toolName } = resolveMcpToolBinding(selection) - - let discovery = discoveredByServer.get(serverId) - if (!discovery) { - if (!context) - throw new Error('MCP workflow tool discovery requires trusted execution scope') - discovery = discoverMcpServerToolsAsExecutor({ workspaceId, context, serverId }) - discoveredByServer.set(serverId, discovery) - } - const match = (await discovery).find((tool) => tool.name === toolName) - if (!match) throw new Error(`MCP operation "${toolName}" is missing or not permitted`) - return toMothershipMcpTool(match) - }) - ) - - return dedupeMcpTools(resolved) -} - -/** Only explicitly enabled servers contribute schemas; each canonical target is freshly authorized. */ -export async function buildOrganizationTaggedMcpToolSchemas( - principal: Principal, - owner: { userId: string; organizationId: string; chatId: string }, - serverIds: string[] -): Promise { - const targets = new Map() - for (const serverId of new Set(serverIds.filter(Boolean))) { - const { server } = await getMcpServerUseCase.execute({ - principal, - input: { serverId, organizationId: owner.organizationId }, - }) - if (!server.workspaceId) throw new Error('Tagged MCP server has no workspace target') - await resolveInvocationWorkspace(owner, server.workspaceId) - const ids = targets.get(server.workspaceId) ?? [] - ids.push(serverId) - targets.set(server.workspaceId, ids) - } - const tools: ToolSchema[] = [] - for (const [workspaceId, ids] of targets) { - const schemas = await buildTaggedMcpToolSchemas(owner.userId, workspaceId, ids) - tools.push( - ...schemas.map((schema) => ({ - ...schema, - description: `${schema.description} Workspace target: ${workspaceId}.`, - })) - ) - } - return dedupeMcpTools(tools) + const seen = new Set() + const discovery: TaggedMcpDiscovery = { tools: [], failures: [] } + outcomes.forEach((outcome, index) => { + if (outcome.status === 'rejected') { + discovery.failures.push({ serverId: uniqueServerIds[index], error: toError(outcome.reason) }) + return + } + for (const tool of outcome.value.map(toMothershipMcpTool)) { + if (seen.has(tool.name)) continue + seen.add(tool.name) + discovery.tools.push(tool) + } + }) + return discovery }