From a1bb62bf393091c4ac7efb38460212ed566157f9 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 20 Oct 2026 17:45:02 -0700 Subject: [PATCH] feat(mothership): discover every workspace mcp server without tagging The integration catalog only inspected MCP servers the user had /-tagged in the chat, so a configured server's tools were invisible to the agent until tagged. Claude Code, Codex, Cursor and OpenCode treat every configured server as available and discover tools lazily; built-in integrations here already did. In agent and plan chats, the catalog's MCP candidates are now every enabled, non-Credential-Group server the requester can list in the target workspace. Search stays MCP-free, and workflow-block requests (mcpExecution or selected operations) stay scoped to their explicit selections. - Untagged servers are discovered best-effort: cache-aside through the existing McpService discovery cache (in-flight deduped, negative cache honored), and a server that does not answer within 3s is skipped and logged while its discovery keeps running to warm the cache for the next query. - Tagged servers and an explicitly targeted server (service mcp: or an exact tool id, which the worker re-resolves before executing) are still listed live and completely, and a targeted server still fails with its own error. - Tagged servers rank first among equally relevant matches via a preferredServices tie-break in projectIntegrationCatalog. - A permission group that disables MCP now drops MCP from broad requests and keeps built-in operations listed; targeted and workflow-block requests still fail with the permission error. Tagging persistence is unchanged; comments that said untagged servers stay unavailable now describe tags as a priority hint. --- .../integrations/catalog/route.test.ts | 2 +- apps/sim/lib/mothership/chat/lifecycle.ts | 2 +- apps/sim/lib/mothership/chat/payload.ts | 4 +- apps/sim/lib/mothership/chat/post.ts | 15 +- .../mothership/chat/process-contents.test.ts | 6 +- .../application/catalog-registry.test.ts | 2 +- .../integrations/application/catalog.test.ts | 125 ++++++++++++- .../integrations/application/catalog.ts | 167 ++++++++++++------ apps/sim/lib/mothership/mcp-tools.test.ts | 40 +++-- apps/sim/lib/mothership/mcp-tools.ts | 105 +++++++---- 10 files changed, 340 insertions(+), 128 deletions(-) diff --git a/apps/sim/app/api/mothership/integrations/catalog/route.test.ts b/apps/sim/app/api/mothership/integrations/catalog/route.test.ts index 74a14af3d34..2f0d11e64ea 100644 --- a/apps/sim/app/api/mothership/integrations/catalog/route.test.ts +++ b/apps/sim/app/api/mothership/integrations/catalog/route.test.ts @@ -14,7 +14,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' vi.unmock('@/lib/mothership/request/http') vi.mock('@/lib/mothership/chat/payload', () => mothershipChatPayloadMock) -vi.mock('@/lib/mothership/mcp-tools', () => ({ buildTaggedMcpToolSchemas: vi.fn() })) +vi.mock('@/lib/mothership/mcp-tools', () => ({ buildMcpToolSchemas: vi.fn() })) vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock) vi.mock('@/lib/workspaces/application/context', () => workspaceContextMock) vi.mock('@sim/platform-authz/workspace', () => workspaceAuthzMock) diff --git a/apps/sim/lib/mothership/chat/lifecycle.ts b/apps/sim/lib/mothership/chat/lifecycle.ts index bc596ab99b5..a873e538ca4 100644 --- a/apps/sim/lib/mothership/chat/lifecycle.ts +++ b/apps/sim/lib/mothership/chat/lifecycle.ts @@ -256,7 +256,7 @@ export async function loadChatFileAttachmentsNamed( * MCP server ids tagged (`/name`) on a chat's live messages, in first-tagged * transcript order. Reads only the `mcp` entries of each message's `contexts` * instead of materializing the whole transcript, and skips soft-deleted - * messages so a removed turn no longer enables its servers. + * messages so a removed turn no longer tags its servers. */ export async function loadChatMcpServerIds(chatId: string): Promise { const rows = await db diff --git a/apps/sim/lib/mothership/chat/payload.ts b/apps/sim/lib/mothership/chat/payload.ts index 3db1f6fe0ee..df4b74aa1f1 100644 --- a/apps/sim/lib/mothership/chat/payload.ts +++ b/apps/sim/lib/mothership/chat/payload.ts @@ -70,8 +70,8 @@ interface BuildPayloadParams { provider?: string contexts?: Array<{ type: string; content: string; tag?: string; path?: string }> /** - * MCP servers enabled for this chat — every server tagged on this or any - * earlier turn. Servers never tagged in the chat stay unavailable. + * MCP servers tagged on this or any earlier turn of the chat. Every usable + * workspace server is discoverable without a tag; tagged ones rank first. */ mcpServerIds?: string[] fileAttachments?: Array<{ id: string; key: string; size: number; [key: string]: unknown }> diff --git a/apps/sim/lib/mothership/chat/post.ts b/apps/sim/lib/mothership/chat/post.ts index 1796faf074c..bde5019d46e 100644 --- a/apps/sim/lib/mothership/chat/post.ts +++ b/apps/sim/lib/mothership/chat/post.ts @@ -508,14 +508,15 @@ function normalizeContexts(contexts: UnifiedChatRequest['contexts']) { } /** - * An MCP server tagged with `/name` stays enabled for the rest of the chat, not - * just the turn it was tagged on. Persisted user messages already carry their - * `mcp` contexts, so the transcript is the source of truth — enablement survives - * reloads and reopened chats with no extra state to keep in sync. There is - * deliberately no off switch: history is append-only. + * An MCP server tagged with `/name` stays tagged for the rest of the chat, not + * just the turn it was tagged on. Every usable workspace server is discoverable + * without a tag; a tag marks the servers the user means, whose operations the + * integration catalog ranks first and lists completely. Persisted user messages + * already carry their `mcp` contexts, so the transcript is the source of truth — + * tags survive 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 ids scope the - * request's integration catalog, and the agent discovers these servers' + * Only the ids travel forward, not the contexts themselves. The agent discovers * 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 diff --git a/apps/sim/lib/mothership/chat/process-contents.test.ts b/apps/sim/lib/mothership/chat/process-contents.test.ts index 7536d364a32..a0ca991855d 100644 --- a/apps/sim/lib/mothership/chat/process-contents.test.ts +++ b/apps/sim/lib/mothership/chat/process-contents.test.ts @@ -47,7 +47,7 @@ import { MAX_TABLE_SELECTION_PREVIEW_LENGTH, MAX_TABLE_SELECTION_ROWS, } from '@/lib/mothership/chat/selection-context' -import { buildTaggedMcpToolSchemas } from '@/lib/mothership/mcp-tools' +import { buildMcpToolSchemas } from '@/lib/mothership/mcp-tools' import { getBlock as registryGetBlock, getBlockRegistry as registryGetBlockRegistry, @@ -635,7 +635,9 @@ describe('processContextsServer - MCP contexts', () => { }, ]) expect(discoverServerTools).not.toHaveBeenCalled() - const { tools: catalog } = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1']) + const { tools: catalog } = await buildMcpToolSchemas('user-1', 'ws-1', { + complete: ['mcp-server-1'], + }) expect(discoverServerTools).toHaveBeenCalledTimes(1) expect(discoverServerTools).toHaveBeenCalledWith({ principal: expect.objectContaining({ diff --git a/apps/sim/lib/mothership/integrations/application/catalog-registry.test.ts b/apps/sim/lib/mothership/integrations/application/catalog-registry.test.ts index f91ada6c562..8a59af94a23 100644 --- a/apps/sim/lib/mothership/integrations/application/catalog-registry.test.ts +++ b/apps/sim/lib/mothership/integrations/application/catalog-registry.test.ts @@ -10,7 +10,7 @@ import { projectIntegrationCatalog } from '@/lib/mothership/integrations/applica /** Registration assertions must exercise the executable registry, not the global empty mock. */ vi.unmock('@/tools/registry') vi.mock('@/lib/mothership/chat/payload', () => mothershipChatPayloadMock) -vi.mock('@/lib/mothership/mcp-tools', () => ({ buildTaggedMcpToolSchemas: vi.fn() })) +vi.mock('@/lib/mothership/mcp-tools', () => ({ buildMcpToolSchemas: vi.fn() })) vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock) const tools: ToolSchema[] = getExposedIntegrationTools().map((tool) => ({ diff --git a/apps/sim/lib/mothership/integrations/application/catalog.test.ts b/apps/sim/lib/mothership/integrations/application/catalog.test.ts index 60e2f310a00..becedafbe08 100644 --- a/apps/sim/lib/mothership/integrations/application/catalog.test.ts +++ b/apps/sim/lib/mothership/integrations/application/catalog.test.ts @@ -11,6 +11,7 @@ import { mothershipWorkspaceTargetMockFns, } from '@sim/testing/mocks/mothership-workspace-target.mock' import { + MockMcpToolsNotAllowedError, permissionCheckMock, permissionCheckMockFns, } from '@sim/testing/mocks/permission-check.mock' @@ -22,7 +23,7 @@ import { workspaceContextMock, workspaceContextMockFns, } from '@sim/testing/mocks/workspace-context.mock' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { OrchestrationError } from '@/lib/core/orchestration/types' import { createTrustedCopilotPrincipal, @@ -72,6 +73,9 @@ const tools = [ input_schema: { type: 'object' }, }, ] +function usableServer(id: string, overrides: Record = {}) { + return { id, enabled: true, credentialGroupId: null, ...overrides } +} function discoveredServerIds() { return mocks.discover.mock.calls.map(([args]) => args.input.serverId) } @@ -96,7 +100,7 @@ beforeEach(() => { mocks.discover.mockReset() mocks.discover.mockResolvedValue({ tools: [] }) mocks.mcpPermission.mockResolvedValue(undefined) - mocks.listServers.mockResolvedValue({ servers: [{ id: 'mcp-abc' }] }) + mocks.listServers.mockResolvedValue({ servers: [usableServer('mcp-abc')] }) }) describe('integration catalog projection', () => { it.each(['google_calendar', 'google-calendar', 'Google Calendar', ' GOOGLE-CALENDAR '])( @@ -384,7 +388,6 @@ it('ranks query tokens across operation names, descriptions, and service while p it('filters organization enabled servers to the authorized target before broad MCP discovery', async () => { queueChat('agent') mocks.target.mockResolvedValue({ workspaceId: 'workspace-1' }) - mocks.listServers.mockResolvedValue({ servers: [{ id: 'mcp-abc' }] }) await readIntegrationCatalog.execute({ principal: principal(), input: { @@ -438,7 +441,7 @@ describe('tagged MCP servers that cannot be discovered', () => { } beforeEach(() => { mocks.target.mockResolvedValue({ workspaceId: 'workspace-1' }) - mocks.listServers.mockResolvedValue({ servers: serverIds.map((id) => ({ id })) }) + mocks.listServers.mockResolvedValue({ servers: serverIds.map((id) => usableServer(id)) }) mocks.discover.mockImplementation(async ({ input: { serverId } }) => { const failure = failures[serverId] if (failure) return failure() @@ -467,3 +470,117 @@ describe('tagged MCP servers that cannot be discovered', () => { await expect(catalog({ toolId: `${serverId}-send` })).rejects.toThrow(message) }) }) + +describe('untagged workspace MCP servers', () => { + function catalog( + request: Partial, + mode: IntegrationCatalogRequest['mode'] = 'agent' + ) { + queueChat(mode) + return readIntegrationCatalog.execute({ + principal: principal(), + input: { ...input, mode, workspaceId: 'workspace-1', ...request }, + }) + } + function toolIds(result: { operations: Array<{ toolId: string }> }) { + return result.operations.map((operation) => operation.toolId) + } + beforeEach(() => { + mocks.target.mockResolvedValue({ workspaceId: 'workspace-1' }) + mocks.listServers.mockResolvedValue({ + servers: ['mcp-docs', 'mcp-crm'].map((id) => usableServer(id)), + }) + mocks.discover.mockImplementation(async ({ input: { serverId } }) => ({ + tools: [{ serverId, name: 'lookup', description: 'Look up a record', inputSchema: {} }], + })) + }) + afterEach(() => { + vi.useRealTimers() + }) + + it.each(['agent', 'plan'] as const)( + 'discovers every usable server without a tag in %s mode, from the discovery cache', + async (mode) => { + const result = await catalog({ query: 'lookup' }, mode) + expect(toolIds(result)).toEqual(['mcp-crm-lookup', 'mcp-docs-lookup']) + expect(mocks.discover).toHaveBeenCalledWith({ + principal: expect.objectContaining({ workspaceId: 'workspace-1', subjectUserId: 'actor' }), + input: { workspaceId: 'workspace-1', serverId: 'mcp-docs' }, + }) + } + ) + + it('keeps Search free of MCP even with usable workspace servers', async () => { + const result = await catalog({}, 'assistant') + expect(toolIds(result)).toEqual(['gmail_send', 'slack_send']) + expect(mocks.listServers).not.toHaveBeenCalled() + expect(mocks.discover).not.toHaveBeenCalled() + }) + + it('never discovers disabled, deleted, or Credential Group servers', async () => { + mocks.listServers.mockResolvedValue({ + servers: [ + usableServer('mcp-docs'), + usableServer('mcp-off', { enabled: false }), + usableServer('mcp-group', { credentialGroupId: 'group-1' }), + ], + }) + const result = await catalog({ mcpServerIds: ['mcp-deleted'], query: 'lookup' }) + expect(toolIds(result)).toEqual(['mcp-docs-lookup']) + expect(discoveredServerIds()).toEqual(['mcp-docs']) + }) + + it('keeps a workflow selection scoped to the operations it selected', async () => { + const result = await catalog({ mcpToolIds: ['mcp-docs-lookup'] }) + expect(toolIds(result)).toEqual(['gmail_send', 'mcp-docs-lookup', 'slack_send']) + expect(discoveredServerIds()).toEqual(['mcp-docs']) + expect(mocks.listServers).not.toHaveBeenCalled() + }) + + it('ranks a tagged server ahead of an equally relevant untagged one', async () => { + const result = await catalog({ mcpServerIds: ['mcp-docs'], query: 'lookup' }) + expect(toolIds(result)).toEqual(['mcp-docs-lookup', 'mcp-crm-lookup']) + }) + + it('skips an untagged server that does not answer in time without failing the query', async () => { + vi.useFakeTimers() + mocks.discover.mockImplementation(async ({ input: { serverId } }) => + serverId === 'mcp-crm' + ? new Promise(() => {}) + : { + tools: [{ serverId, name: 'lookup', description: 'Look up a record', inputSchema: {} }], + } + ) + const pending = catalog({ query: 'lookup' }) + await vi.advanceTimersByTimeAsync(3_000) + expect(toolIds(await pending)).toEqual(['mcp-docs-lookup']) + }) + + it('lists native operations when a permission group disables MCP', async () => { + mocks.mcpPermission.mockRejectedValue(new MockMcpToolsNotAllowedError()) + const result = await catalog({ mcpServerIds: ['mcp-docs'] }) + expect(toolIds(result)).toEqual(['gmail_send', 'slack_send']) + expect(mocks.discover).not.toHaveBeenCalled() + await expect(catalog({ service: 'mcp:mcp-docs' })).rejects.toThrow('MCP tools are not allowed') + }) + + it('resolves an untagged server operation by exact ID for execution', async () => { + const result = await catalog({ toolId: 'mcp-crm-lookup' }) + expect(result.operations).toEqual([ + { + toolId: 'mcp-crm-lookup', + service: 'mcp:mcp-crm', + description: 'Look up a record', + inputSchema: {}, + }, + ]) + expect(mocks.discover).toHaveBeenCalledTimes(1) + expect(mocks.discover).toHaveBeenCalledWith( + expect.objectContaining({ + input: { workspaceId: 'workspace-1', serverId: 'mcp-crm', requireComplete: true }, + }) + ) + mocks.discover.mockRejectedValue(new Error('connect ECONNREFUSED')) + await expect(catalog({ toolId: 'mcp-crm-lookup' })).rejects.toThrow('connect ECONNREFUSED') + }) +}) diff --git a/apps/sim/lib/mothership/integrations/application/catalog.ts b/apps/sim/lib/mothership/integrations/application/catalog.ts index 793ed6503ae..4a9cadedd3a 100644 --- a/apps/sim/lib/mothership/integrations/application/catalog.ts +++ b/apps/sim/lib/mothership/integrations/application/catalog.ts @@ -31,8 +31,12 @@ import type { IntegrationCatalogRequest, IntegrationCatalogResponse, } from '@/lib/mothership/generated/integration-catalog' -import { buildTaggedMcpToolSchemas } from '@/lib/mothership/mcp-tools' +import { buildMcpToolSchemas } from '@/lib/mothership/mcp-tools' import { resolveActiveWorkspaceApplicationContext } from '@/lib/workspaces/application/context' +import { + assertPermissionsAllowed, + McpToolsNotAllowedError, +} from '@/ee/access-control/utils/permission-check' const logger = createLogger('IntegrationCatalog') @@ -47,11 +51,19 @@ export const readIntegrationCatalogOperation = defineWorkspaceOperation({ delegatedServices: ['copilot', 'executor'], }) +interface CatalogProjectionOptions { + /** Services the user named in the conversation; they rank first among equally relevant matches. */ + preferredServices?: ReadonlySet +} + /** Return schemas only for targeted discovery, while preserving ranked search and explicit full listings. */ export function projectIntegrationCatalog( tools: ToolSchema[], - input: IntegrationCatalogRequest + input: IntegrationCatalogRequest, + { preferredServices }: CatalogProjectionOptions = {} ): IntegrationCatalogResponse { + const preferred = (tool: ToolSchema) => + Number(Boolean(tool.service && preferredServices?.has(tool.service))) const normalized = input.query?.trim().toLowerCase() ?? '' const terms = normalized.split(/[^a-z0-9]+/).filter(Boolean) const services = resolveServiceFilter(tools, input.service) @@ -73,6 +85,7 @@ export function projectIntegrationCatalog( Number(b.tool.name.toLowerCase() === normalized) - Number(a.tool.name.toLowerCase() === normalized) || b.score - a.score || + preferred(b.tool) - preferred(a.tool) || a.tool.name.localeCompare(b.tool.name) ) const selected = input.limit === 0 ? matches : matches.slice(0, input.limit) @@ -196,69 +209,107 @@ const catalogUseCase = defineAuthorizedChatUseCase({ context.mode !== 'assistant' && (!input.toolId || input.toolId.startsWith('mcp-')) && (!input.service || input.service.startsWith('mcp:')) - if (includeMcp && (input.mcpServerIds.length || input.mcpToolIds?.length)) { - if (!workspaceId) { - if (input.toolId?.startsWith('mcp-') || input.service?.startsWith('mcp:')) - throw new OrchestrationError( - 'validation', - 'MCP discovery requires an explicit workspace ID' - ) - return projectIntegrationCatalog(tools, input) - } - const selectedIds = new Set(input.mcpToolIds ?? []) - const serverIds = new Set(input.mcpServerIds) - const selectedServers = [...selectedIds].map((id) => { - const target = parseMcpToolTarget(id) - return target.kind === 'shared_server' ? target.serverId : target.credentialId - }) - let candidates = [...new Set([...serverIds, ...selectedServers])].filter( - (serverId) => - (!input.service || input.service === `mcp:${serverId}`) && - (!input.toolId || input.toolId.startsWith(`${serverId}-`)) - ) - if (context.organizationId && candidates.length) { - const { servers } = await listMcpServersUseCase.execute({ - principal: createCopilotChatPrincipal( - { userId: context.userId, workspaceId }, - MCP_SERVER_DELEGATION_AUDIENCE - ), - input: { workspaceId }, - }) - const available = new Set(servers.map((server) => server.id)) - candidates = candidates.filter((id) => available.has(id)) - } - const executorContext = input.mcpExecution - ? catalogExecutorContext(input, context.userId, workspaceId) - : undefined - 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)) + /** A workflow block's request names its operations; a chat may use every usable server. */ + const selectionScoped = Boolean(input.mcpExecution || input.mcpToolIds?.length) + const selectsNothing = !input.mcpServerIds.length && !input.mcpToolIds?.length + if (!includeMcp || (selectionScoped && selectsNothing)) + return projectIntegrationCatalog(tools, input) + const targeted = Boolean(input.toolId?.startsWith('mcp-') || input.service?.startsWith('mcp:')) + if (!workspaceId) { + if (targeted) + throw new OrchestrationError( + 'validation', + 'MCP discovery requires an explicit workspace ID' + ) + return projectIntegrationCatalog(tools, input) + } + try { + await assertPermissionsAllowed({ userId: context.userId, workspaceId, toolKind: 'mcp' }) + } catch (error) { + if (selectionScoped || targeted || !(error instanceof McpToolsNotAllowedError)) throw error + return projectIntegrationCatalog(tools, input) + } + + const selectedIds = new Set(input.mcpToolIds ?? []) + const taggedIds = new Set(input.mcpServerIds) + const isTarget = (serverId: string) => + (!input.service || input.service === `mcp:${serverId}`) && + (!input.toolId || input.toolId.startsWith(`${serverId}-`)) + const selectedServers = [...selectedIds].map((id) => { + const target = parseMcpToolTarget(id) + return target.kind === 'shared_server' ? target.serverId : target.credentialId + }) + const named = [...new Set([...taggedIds, ...selectedServers])].filter(isTarget) + let complete = named + let bestEffort: string[] = [] + if (!selectionScoped) { + const usable = await listUsableMcpServerIds(context.userId, workspaceId) + const untagged = [...usable].filter( + (serverId) => isTarget(serverId) && !taggedIds.has(serverId) ) - 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, - }) + /** A targeted server is listed completely; a tagged one keeps its own error even once unusable. */ + if (targeted) complete = [...named, ...untagged] + else { + complete = named.filter((serverId) => usable.has(serverId)) + bestEffort = untagged } - tools.push( - ...mcpTools.filter( - (tool) => selectedIds.has(tool.name) || serverIds.has(tool.service?.slice(4) ?? '') + } + const executorContext = input.mcpExecution + ? catalogExecutorContext(input, context.userId, workspaceId) + : undefined + const { tools: mcpTools, failures } = await buildMcpToolSchemas( + context.userId, + workspaceId, + { complete, bestEffort }, + 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) { + const details = { serverId, reason: error.message } + if (taggedIds.has(serverId) || selectedServers.includes(serverId)) + logger.warn( + 'Omitting an undiscoverable tagged MCP server from the integration catalog', + details ) - ) + else + logger.info('Omitting an undiscoverable MCP server from the integration catalog', details) } - return projectIntegrationCatalog(tools, input) + tools.push( + ...(selectionScoped + ? mcpTools.filter( + (tool) => selectedIds.has(tool.name) || taggedIds.has(tool.service?.slice(4) ?? '') + ) + : mcpTools) + ) + return projectIntegrationCatalog(tools, input, { + preferredServices: new Set([...taggedIds].map((serverId) => `mcp:${serverId}`)), + }) }, }) +/** + * The enabled, live servers in the workspace that the requester may use through + * chat. Credential Group servers need an explicit managed connection, so only a + * workflow block's selection reaches them. + */ +async function listUsableMcpServerIds(userId: string, workspaceId: string): Promise> { + const { servers } = await listMcpServersUseCase.execute({ + principal: createCopilotChatPrincipal({ userId, workspaceId }, MCP_SERVER_DELEGATION_AUDIENCE), + input: { workspaceId }, + }) + return new Set( + servers + .filter((server) => server.enabled && !server.credentialGroupId) + .map((server) => server.id) + ) +} + /** Restore only the canonical nonsecret origin, never a bearer token from request state. */ function catalogExecutorContext( input: IntegrationCatalogRequest, diff --git a/apps/sim/lib/mothership/mcp-tools.test.ts b/apps/sim/lib/mothership/mcp-tools.test.ts index 51b5264210b..1da2df8302d 100644 --- a/apps/sim/lib/mothership/mcp-tools.test.ts +++ b/apps/sim/lib/mothership/mcp-tools.test.ts @@ -1,9 +1,5 @@ import { mcpUseCasesMock, mcpUseCasesMockFns } from '@sim/testing/mocks/mcp-use-cases.mock' -import { - permissionCheckMock, - permissionCheckMockFns, -} from '@sim/testing/mocks/permission-check.mock' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { describe, expect, it, vi } from 'vitest' const { discoverServerTools } = vi.hoisted(() => ({ discoverServerTools: vi.fn(), @@ -13,20 +9,14 @@ vi.mock('@/lib/internal/mcp/discover-tools', () => ({ discoverMcpServerToolsAsExecutor: discoverServerTools, })) vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock) -vi.mock('@/ee/access-control/utils/permission-check', () => permissionCheckMock) -import { buildTaggedMcpToolSchemas } from '@/lib/mothership/mcp-tools' +import { buildMcpToolSchemas } from '@/lib/mothership/mcp-tools' -const assertPermissionsAllowed = permissionCheckMockFns.mockAssertPermissionsAllowed mcpUseCasesMockFns.mockDiscoverMcpServerToolsUseCase.mockImplementation(async (args: unknown) => ({ tools: await discoverServerTools(args), })) describe('mothership MCP tool schemas', () => { - beforeEach(() => { - assertPermissionsAllowed.mockResolvedValue(undefined) - }) - it('discovers tools only for explicitly tagged servers', async () => { discoverServerTools.mockResolvedValue([ { @@ -38,7 +28,9 @@ describe('mothership MCP tool schemas', () => { }, ]) - const { tools, failures } = await buildTaggedMcpToolSchemas('user-1', 'ws-1', ['mcp-server-1']) + const { tools, failures } = await buildMcpToolSchemas('user-1', 'ws-1', { + complete: ['mcp-server-1'], + }) expect(discoverServerTools).toHaveBeenCalledTimes(1) expect(discoverServerTools).toHaveBeenCalledWith( @@ -64,11 +56,9 @@ describe('mothership MCP tool schemas', () => { return [{ serverId, name: 'search', inputSchema: { type: 'object' } }] }) - const discovery = await buildTaggedMcpToolSchemas('user-1', 'ws-1', [ - 'mcp-down', - 'mcp-server-1', - 'mcp-empty', - ]) + const discovery = await buildMcpToolSchemas('user-1', 'ws-1', { + complete: ['mcp-down', 'mcp-server-1', 'mcp-empty'], + }) expect(discovery.tools.map((tool) => tool.name)).toEqual(['mcp-server-1-search']) expect(discovery.failures.map(({ serverId, error }) => [serverId, error.message])).toEqual([ @@ -76,4 +66,18 @@ describe('mothership MCP tool schemas', () => { ['mcp-empty', 'No permitted MCP operations are available for mcp-empty'], ]) }) + + it('treats a best-effort server with no permitted operations as contributing nothing', async () => { + discoverServerTools.mockImplementation(async ({ input: { serverId } }) => + serverId === 'mcp-empty' ? [] : [{ serverId, name: 'search', inputSchema: {} }] + ) + + const discovery = await buildMcpToolSchemas('user-1', 'ws-1', { + complete: [], + bestEffort: ['mcp-empty', 'mcp-server-1'], + }) + + expect(discovery.tools.map((tool) => tool.name)).toEqual(['mcp-server-1-search']) + expect(discovery.failures).toEqual([]) + }) }) diff --git a/apps/sim/lib/mothership/mcp-tools.ts b/apps/sim/lib/mothership/mcp-tools.ts index 1e778d1b43f..8d5d67ae562 100644 --- a/apps/sim/lib/mothership/mcp-tools.ts +++ b/apps/sim/lib/mothership/mcp-tools.ts @@ -1,4 +1,5 @@ import { toError } from '@sim/utils/errors' +import { withinDeadline } from '@/lib/core/utils/deadline' 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' @@ -7,17 +8,34 @@ import type { McpToolSchema } from '@/lib/mcp/types' import { createMcpToolId } from '@/lib/mcp/utils' 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' -interface TaggedMcpServerFailure { +/** + * How long one best-effort server may hold up a catalog query. A warm server is + * a cache read; a cold or slow one is skipped for this query while its + * discovery keeps running and fills the cache for the next one. + */ +const BEST_EFFORT_DISCOVERY_WAIT_MS = 3_000 + +interface McpServerDiscoveryFailure { serverId: string error: Error } -interface TaggedMcpDiscovery { +interface McpDiscovery { tools: ToolSchema[] - /** Servers that could not be discovered or listed no permitted operations. */ - failures: TaggedMcpServerFailure[] + /** Servers that could not be discovered in time or listed no permitted operations. */ + failures: McpServerDiscoveryFailure[] +} + +interface McpDiscoveryServers { + /** Servers the request names: listed live and completely, however long that takes. */ + complete: string[] + /** + * Chat-only: servers available without being named. Served from the discovery + * cache when fresh, otherwise skipped after {@link BEST_EFFORT_DISCOVERY_WAIT_MS}. + * One with no permitted operations contributes nothing and is not a failure. + */ + bestEffort?: string[] } function toMothershipMcpTool(tool: { @@ -37,45 +55,64 @@ function toMothershipMcpTool(tool: { } /** - * 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. + * Resolves MCP server tools into request-local catalog operations. Each server + * settles on its own: one that cannot be discovered in time, or a named one + * that lists no permitted operations, is reported in `failures` without + * discarding the others. Permission-group gating is the caller's, since only it + * knows whether the request depends on MCP. */ -export async function buildTaggedMcpToolSchemas( +export async function buildMcpToolSchemas( userId: string, workspaceId: string, - serverIds: string[], + servers: McpDiscoveryServers, context?: InternalToolOperationContext -): Promise { - const uniqueServerIds = [...new Set(serverIds.filter(Boolean))] - if (uniqueServerIds.length === 0) return { tools: [], failures: [] } +): Promise { + const complete = [...new Set(servers.complete.filter(Boolean))] + const bestEffort = [...new Set(servers.bestEffort ?? [])].filter( + (serverId) => serverId && !complete.includes(serverId) + ) + if (context && bestEffort.length) + throw new Error('Executor MCP discovery is limited to the servers its block selected') + const serverIds = [...complete, ...bestEffort] + if (serverIds.length === 0) return { tools: [], failures: [] } - await assertPermissionsAllowed({ userId, workspaceId, toolKind: 'mcp' }) - const outcomes = await Promise.allSettled( - uniqueServerIds.map(async (serverId) => { - const tools = context - ? await discoverMcpServerToolsAsExecutor({ workspaceId, context, serverId }) - : ( - await discoverMcpServerToolsUseCase.execute({ - principal: createCopilotChatPrincipal( - { userId, workspaceId }, - MCP_SERVER_DELEGATION_AUDIENCE - ), - input: { workspaceId, serverId, requireComplete: true }, - }) - ).tools - if (!tools.length) - throw new Error(`No permitted MCP operations are available for ${serverId}`) - return tools + const principal = () => + createCopilotChatPrincipal({ userId, workspaceId }, MCP_SERVER_DELEGATION_AUDIENCE) + const discoverComplete = async (serverId: string) => { + const tools = context + ? await discoverMcpServerToolsAsExecutor({ workspaceId, context, serverId }) + : ( + await discoverMcpServerToolsUseCase.execute({ + principal: principal(), + input: { workspaceId, serverId, requireComplete: true }, + }) + ).tools + if (!tools.length) throw new Error(`No permitted MCP operations are available for ${serverId}`) + return tools + } + /** Ignores the deadline's signal so a slow server's discovery still finishes and warms the cache. */ + const discoverBestEffort = async (serverId: string) => { + const discovery = discoverMcpServerToolsUseCase.execute({ + principal: principal(), + input: { workspaceId, serverId }, }) - ) + const { tools } = await withinDeadline( + () => discovery, + Date.now() + BEST_EFFORT_DISCOVERY_WAIT_MS + ) + return tools + } + + const outcomes = await Promise.allSettled([ + ...complete.map(discoverComplete), + ...bestEffort.map(discoverBestEffort), + ]) const seen = new Set() - const discovery: TaggedMcpDiscovery = { tools: [], failures: [] } + const discovery: McpDiscovery = { tools: [], failures: [] } outcomes.forEach((outcome, index) => { if (outcome.status === 'rejected') { - discovery.failures.push({ serverId: uniqueServerIds[index], error: toError(outcome.reason) }) + discovery.failures.push({ serverId: serverIds[index], error: toError(outcome.reason) }) return } for (const tool of outcome.value.map(toMothershipMcpTool)) {