Skip to content

Commit 4be882a

Browse files
authored
fix(mothership): keep integration discovery working when a tagged mcp server fails or lists no tools (#9260)
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:<id>, 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.
1 parent 4bd6871 commit 4be882a

8 files changed

Lines changed: 166 additions & 258 deletions

File tree

‎apps/sim/app/api/mothership/execute/route.test.ts‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,10 @@ import { createMockRequest } from '@sim/testing/mocks/request.mock'
2323
import { beforeEach, describe, expect, it, vi } from 'vitest'
2424

2525
const {
26-
mockBuildSelectedMcpToolSchemas,
27-
mockBuildTaggedMcpToolSchemas,
2826
mockComputeWorkspaceEntitlements,
2927
mockProcessContextsServer,
3028
mockRequestExplicitStreamAbort,
3129
} = vi.hoisted(() => ({
32-
mockBuildSelectedMcpToolSchemas: vi.fn(),
33-
mockBuildTaggedMcpToolSchemas: vi.fn(),
3430
mockComputeWorkspaceEntitlements: vi.fn(),
3531
mockProcessContextsServer: vi.fn(),
3632
mockRequestExplicitStreamAbort: vi.fn(),
@@ -53,11 +49,6 @@ vi.mock('@/lib/mothership/entitlements', () => ({
5349
computeWorkspaceEntitlements: mockComputeWorkspaceEntitlements,
5450
}))
5551

56-
vi.mock('@/lib/mothership/mcp-tools', () => ({
57-
buildSelectedMcpToolSchemas: mockBuildSelectedMcpToolSchemas,
58-
buildTaggedMcpToolSchemas: mockBuildTaggedMcpToolSchemas,
59-
}))
60-
6152
vi.mock('@/lib/mothership/request/lifecycle/headless', () => mothershipHeadlessLifecycleMock)
6253

6354
vi.mock('@/lib/mothership/request/session/explicit-abort', () => ({
@@ -144,8 +135,6 @@ describe('mothership private trace provenance transport', () => {
144135
decryptionFailures: [],
145136
})
146137
mockBuildIntegrationToolSchemas.mockResolvedValue([])
147-
mockBuildSelectedMcpToolSchemas.mockResolvedValue([])
148-
mockBuildTaggedMcpToolSchemas.mockResolvedValue([])
149138
mockComputeWorkspaceEntitlements.mockResolvedValue([])
150139
mockDecryptSecret.mockResolvedValue({ decrypted: 'secret-value' })
151140
mockProcessContextsServer.mockResolvedValue([])
@@ -484,9 +473,6 @@ describe('mothership private trace provenance transport', () => {
484473
})
485474

486475
it('sends enabled MCP IDs without schemas or activating matching configured secrets', async () => {
487-
mockBuildTaggedMcpToolSchemas.mockResolvedValueOnce([
488-
{ name: 'mcp-docs', description: 'Uses secret-value' },
489-
])
490476
mockRunHeadlessCopilotLifecycle.mockImplementation(
491477
async (payload: Record<string, unknown>, options: CopilotLifecycleOptions) => {
492478
const registry =

‎apps/sim/lib/mothership/chat/payload.test.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -112,11 +112,6 @@ permissionGroupsResolveMockFns.mockGetUserPermissionConfigForOrganization.mockIm
112112

113113
vi.mock('@/lib/billing/plan-helpers', () => billingPlanHelpersMock)
114114

115-
vi.mock('@/lib/mothership/mcp-tools', () => ({
116-
buildTaggedMcpToolSchemas: vi.fn(async () => []),
117-
buildOrganizationTaggedMcpToolSchemas: vi.fn(async () => []),
118-
}))
119-
120115
vi.mock('@/lib/workflows/utils', () => workflowsUtilsMock)
121116

122117
/** Denied-operation projection walks the block map only for blocks the mocked tool list never names. */
@@ -891,9 +886,7 @@ describe('desktop request capabilities', () => {
891886

892887
it('carries only enabled MCP IDs without eager catalog discovery while preserving desktop capabilities', async () => {
893888
const { getBlockVisibilityForCopilot } = await import('@/lib/mothership/block-visibility')
894-
const { buildOrganizationTaggedMcpToolSchemas } = await import('@/lib/mothership/mcp-tools')
895889
vi.mocked(getBlockVisibilityForCopilot).mockClear()
896-
vi.mocked(buildOrganizationTaggedMcpToolSchemas).mockClear()
897890
const principal = createSessionPrincipal({ userId: 'user-org' })
898891
const payload = await buildCopilotRequestPayload({
899892
message: 'Use my tools',
@@ -909,7 +902,6 @@ it('carries only enabled MCP IDs without eager catalog discovery while preservin
909902
terminalCapable: true,
910903
})
911904
expect(getBlockVisibilityForCopilot).not.toHaveBeenCalled()
912-
expect(buildOrganizationTaggedMcpToolSchemas).not.toHaveBeenCalled()
913905
expect(payload.integrationCatalog).toEqual({ mcpServerIds: ['server-a', 'server-b'] })
914906
expect(payload).not.toHaveProperty('integrationTools')
915907
expect(payload).not.toHaveProperty('mothershipTools')

‎apps/sim/lib/mothership/chat/post.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -514,11 +514,12 @@ function normalizeContexts(contexts: UnifiedChatRequest['contexts']) {
514514
* reloads and reopened chats with no extra state to keep in sync. There is
515515
* deliberately no off switch: history is append-only.
516516
*
517-
* Only the ids travel forward, not the contexts themselves. The tools ride the
518-
* tool array on every turn, so the model always sees their names and schemas;
519-
* re-expanding the prompt listing each turn would just duplicate that. Keeping
520-
* inherited servers out of the persisted contexts also keeps the `/name` chips
521-
* on a sent message showing only what the user actually typed that turn.
517+
* Only the ids travel forward, not the contexts themselves. The ids scope the
518+
* request's integration catalog, and the agent discovers these servers'
519+
* operations on demand, so re-expanding the prompt listing each turn adds
520+
* nothing. Keeping inherited servers out of the persisted contexts also keeps
521+
* the `/name` chips on a sent message showing only what the user actually typed
522+
* that turn.
522523
*/
523524
function collectChatMcpServerIds(
524525
chatMcpServerIds: string[],

‎apps/sim/lib/mothership/chat/process-contents.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -635,7 +635,7 @@ describe('processContextsServer - MCP contexts', () => {
635635
},
636636
])
637637
expect(discoverServerTools).not.toHaveBeenCalled()
638-
const catalog = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1'])
638+
const { tools: catalog } = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1'])
639639
expect(discoverServerTools).toHaveBeenCalledTimes(1)
640640
expect(discoverServerTools).toHaveBeenCalledWith({
641641
principal: expect.objectContaining({

‎apps/sim/lib/mothership/integrations/application/catalog.test.ts‎

Lines changed: 85 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ import {
1010
mothershipWorkspaceTargetMock,
1111
mothershipWorkspaceTargetMockFns,
1212
} from '@sim/testing/mocks/mothership-workspace-target.mock'
13+
import {
14+
permissionCheckMock,
15+
permissionCheckMockFns,
16+
} from '@sim/testing/mocks/permission-check.mock'
1317
import {
1418
permissionGroupsResolveMock,
1519
permissionGroupsResolveMockFns,
@@ -19,6 +23,7 @@ import {
1923
workspaceContextMockFns,
2024
} from '@sim/testing/mocks/workspace-context.mock'
2125
import { beforeEach, describe, expect, it, vi } from 'vitest'
26+
import { OrchestrationError } from '@/lib/core/orchestration/types'
2227
import {
2328
createTrustedCopilotPrincipal,
2429
createTrustedOrganizationCopilotPrincipal,
@@ -30,9 +35,11 @@ import {
3035
readIntegrationCatalog,
3136
} from '@/lib/mothership/integrations/application/catalog'
3237

33-
const hoisted = vi.hoisted(() => ({ mcp: vi.fn(), flag: vi.fn(async () => true) }))
38+
const hoisted = vi.hoisted(() => ({ flag: vi.fn(async () => true) }))
3439
const mocks = {
3540
...hoisted,
41+
discover: mcpUseCasesMockFns.mockDiscoverMcpServerToolsUseCase,
42+
mcpPermission: permissionCheckMockFns.mockAssertPermissionsAllowed,
3643
build: mothershipChatPayloadMockFns.mockBuildIntegrationToolSchemas,
3744
banned: authBanMockFns.mockGetActivelyBannedUserIds,
3845
target: mothershipWorkspaceTargetMockFns.mockResolveInvocationWorkspace,
@@ -42,7 +49,8 @@ const mocks = {
4249
}
4350
vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock)
4451
vi.mock('@/lib/mothership/chat/payload', () => mothershipChatPayloadMock)
45-
vi.mock('@/lib/mothership/mcp-tools', () => ({ buildTaggedMcpToolSchemas: hoisted.mcp }))
52+
vi.mock('@/lib/internal/mcp/discover-tools', () => ({ discoverMcpServerToolsAsExecutor: vi.fn() }))
53+
vi.mock('@/ee/access-control/utils/permission-check', () => permissionCheckMock)
4654
vi.mock('@/lib/mothership/application/workspace-target', () => mothershipWorkspaceTargetMock)
4755
vi.mock('@/lib/workspaces/application/context', () => workspaceContextMock)
4856
vi.mock('@/lib/auth/ban', () => authBanMock)
@@ -64,6 +72,9 @@ const tools = [
6472
input_schema: { type: 'object' },
6573
},
6674
]
75+
function discoveredServerIds() {
76+
return mocks.discover.mock.calls.map(([args]) => args.input.serverId)
77+
}
6778
function principal() {
6879
return createTrustedOrganizationCopilotPrincipal(
6980
{ userId: 'actor', organizationId: 'org-1', chatId: 'chat-1', delegationId: 'catalog-1' },
@@ -82,7 +93,9 @@ beforeEach(() => {
8293
mocks.banned.mockResolvedValue([])
8394
mocks.config.mockResolvedValue(null)
8495
mocks.build.mockResolvedValue([...tools])
85-
mocks.mcp.mockResolvedValue([])
96+
mocks.discover.mockReset()
97+
mocks.discover.mockResolvedValue({ tools: [] })
98+
mocks.mcpPermission.mockResolvedValue(undefined)
8699
mocks.listServers.mockResolvedValue({ servers: [{ id: 'mcp-abc' }] })
87100
})
88101
describe('integration catalog projection', () => {
@@ -238,7 +251,9 @@ describe('catalog authorization', () => {
238251

239252
it('does not discover MCP operations in Search even with selected servers', async () => {
240253
queueChat()
241-
mocks.mcp.mockResolvedValue([{ ...tools[0], name: 'mcp-abc-send', service: 'mcp:mcp-abc' }])
254+
mocks.discover.mockResolvedValue({
255+
tools: [{ serverId: 'mcp-abc', name: 'send', description: 'Send', inputSchema: {} }],
256+
})
242257
const result = await readIntegrationCatalog.execute({
243258
principal: principal(),
244259
input: { ...input, service: 'mcp:mcp-abc', mcpServerIds: ['mcp-abc'] },
@@ -297,10 +312,12 @@ it.each([undefined, 'mcp-abc-write'])(
297312
async (toolId) => {
298313
queueChat('agent')
299314
mocks.target.mockResolvedValue({ workspaceId: 'workspace-1' })
300-
mocks.mcp.mockResolvedValue([
301-
{ name: 'mcp-abc-read', service: 'mcp:mcp-abc', description: 'Read', input_schema: {} },
302-
{ name: 'mcp-abc-write', service: 'mcp:mcp-abc', description: 'Write', input_schema: {} },
303-
])
315+
mocks.discover.mockResolvedValue({
316+
tools: [
317+
{ serverId: 'mcp-abc', name: 'read', description: 'Read', inputSchema: {} },
318+
{ serverId: 'mcp-abc', name: 'write', description: 'Write', inputSchema: {} },
319+
],
320+
})
304321
const result = await readIntegrationCatalog.execute({
305322
principal: principal(),
306323
input: {
@@ -312,7 +329,7 @@ it.each([undefined, 'mcp-abc-write'])(
312329
...(toolId ? { toolId } : {}),
313330
},
314331
})
315-
expect(mocks.mcp).toHaveBeenCalledWith('actor', 'workspace-1', ['mcp-abc'], undefined)
332+
expect(discoveredServerIds()).toEqual(['mcp-abc'])
316333
expect(result.operations.map((operation) => operation.toolId)).toEqual(
317334
toolId ? [] : ['mcp-abc-read']
318335
)
@@ -325,7 +342,7 @@ it('keeps native discovery available in organization chats with tagged MCP serve
325342
input: { ...input, mode: 'agent', mcpServerIds: ['mcp-abc'], toolId: 'gmail_send' },
326343
})
327344
expect(result.operations.map((operation) => operation.toolId)).toEqual(['gmail_send'])
328-
expect(mocks.mcp).not.toHaveBeenCalled()
345+
expect(mocks.discover).not.toHaveBeenCalled()
329346
})
330347
it('requires an explicit workspace target for organization MCP discovery', async () => {
331348
queueChat('agent')
@@ -335,7 +352,7 @@ it('requires an explicit workspace target for organization MCP discovery', async
335352
input: { ...input, mode: 'agent', mcpServerIds: ['mcp-abc'], toolId: 'mcp-abc-read' },
336353
})
337354
).rejects.toThrow('explicit workspace ID')
338-
expect(mocks.mcp).not.toHaveBeenCalled()
355+
expect(mocks.discover).not.toHaveBeenCalled()
339356
})
340357

341358
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
381398
principal: expect.objectContaining({ workspaceId: 'workspace-1', subjectUserId: 'actor' }),
382399
input: { workspaceId: 'workspace-1' },
383400
})
384-
expect(mocks.mcp).toHaveBeenCalledWith('actor', 'workspace-1', ['mcp-abc'], undefined)
401+
expect(discoveredServerIds()).toEqual(['mcp-abc'])
385402
})
386403

387404
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
394411
)
395412
}
396413
})
414+
415+
describe('tagged MCP servers that cannot be discovered', () => {
416+
const failures: Record<string, () => Promise<never> | Promise<{ tools: never[] }>> = {
417+
'mcp-deleted': async () => {
418+
throw new OrchestrationError('not_found', 'MCP server not found')
419+
},
420+
'mcp-down': async () => {
421+
throw new Error('connect ECONNREFUSED')
422+
},
423+
'mcp-empty': async () => ({ tools: [] }),
424+
}
425+
const serverIds = ['mcp-abc', ...Object.keys(failures)]
426+
function catalog(request: Partial<IntegrationCatalogRequest>) {
427+
queueChat('agent')
428+
return readIntegrationCatalog.execute({
429+
principal: principal(),
430+
input: {
431+
...input,
432+
mode: 'agent',
433+
workspaceId: 'workspace-1',
434+
mcpServerIds: serverIds,
435+
...request,
436+
},
437+
})
438+
}
439+
beforeEach(() => {
440+
mocks.target.mockResolvedValue({ workspaceId: 'workspace-1' })
441+
mocks.listServers.mockResolvedValue({ servers: serverIds.map((id) => ({ id })) })
442+
mocks.discover.mockImplementation(async ({ input: { serverId } }) => {
443+
const failure = failures[serverId]
444+
if (failure) return failure()
445+
return { tools: [{ serverId, name: 'send', description: 'Send a note', inputSchema: {} }] }
446+
})
447+
})
448+
449+
it.each([undefined, 'send'])(
450+
'still lists native and healthy MCP operations (query: %s)',
451+
async (query) => {
452+
const result = await catalog(query ? { query } : {})
453+
expect(result.operations.map((operation) => operation.toolId)).toEqual(
454+
expect.arrayContaining(['gmail_send', 'slack_send', 'mcp-abc-send'])
455+
)
456+
expect(result.operations).toHaveLength(3)
457+
expect(discoveredServerIds().sort()).toEqual([...serverIds].sort())
458+
}
459+
)
460+
461+
it.each([
462+
['mcp-deleted', 'MCP server not found'],
463+
['mcp-down', 'connect ECONNREFUSED'],
464+
['mcp-empty', 'No permitted MCP operations are available for mcp-empty'],
465+
])('fails a request that targets %s with its own error', async (serverId, message) => {
466+
await expect(catalog({ service: `mcp:${serverId}` })).rejects.toThrow(message)
467+
await expect(catalog({ toolId: `${serverId}-send` })).rejects.toThrow(message)
468+
})
469+
})

‎apps/sim/lib/mothership/integrations/application/catalog.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
resolvePrincipalSubjectUserId,
88
} from '@sim/auth/principal'
99
import { INTEGRATION_METADATA } from '@sim/deployment-config/integration-metadata'
10+
import { createLogger } from '@sim/logger'
1011
import { stripVersionSuffix } from '@sim/utils/string'
1112
import { defineWorkspaceOperation } from '@/lib/core/application'
1213
import { defineOrganizationOperation } from '@/lib/core/application/organization-operation'
@@ -33,6 +34,8 @@ import type {
3334
import { buildTaggedMcpToolSchemas } from '@/lib/mothership/mcp-tools'
3435
import { resolveActiveWorkspaceApplicationContext } from '@/lib/workspaces/application/context'
3536

37+
const logger = createLogger('IntegrationCatalog')
38+
3639
export const INTEGRATION_CATALOG_AUDIENCE = 'sim:integration-catalog'
3740
export const readIntegrationCatalogOperation = defineWorkspaceOperation({
3841
id: 'mothership.integrations.catalog',
@@ -227,12 +230,25 @@ const catalogUseCase = defineAuthorizedChatUseCase({
227230
const executorContext = input.mcpExecution
228231
? catalogExecutorContext(input, context.userId, workspaceId)
229232
: undefined
230-
const mcpTools = await buildTaggedMcpToolSchemas(
233+
const { tools: mcpTools, failures } = await buildTaggedMcpToolSchemas(
231234
context.userId,
232235
workspaceId,
233236
candidates,
234237
executorContext
235238
)
239+
const targetFailure = failures.find(
240+
({ serverId }) =>
241+
input.service === `mcp:${serverId}` ||
242+
(input.toolId?.startsWith(`${serverId}-`) &&
243+
!mcpTools.some((tool) => tool.name === input.toolId))
244+
)
245+
if (targetFailure) throw targetFailure.error
246+
for (const { serverId, error } of failures) {
247+
logger.warn('Omitting an undiscoverable tagged MCP server from the integration catalog', {
248+
serverId,
249+
reason: error.message,
250+
})
251+
}
236252
tools.push(
237253
...mcpTools.filter(
238254
(tool) => selectedIds.has(tool.name) || serverIds.has(tool.service?.slice(4) ?? '')

0 commit comments

Comments
 (0)