From fec266a5ae164e50836b8e312c9536ba82117ad4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Fri, 31 Jul 2026 14:10:30 +0200 Subject: [PATCH] test(server): strengthen MCP catalog and gating coverage (#23630) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three MCP testing improvements discussed in #23613 (now merged; this branch has been rebased onto main). ## What **1. Name-set assertions in `mcp-protocol.service.spec.ts`.** The two "exactly 6 tools" tests only used `expect.objectContaining` subset matches, so the size claim in the titles was never enforced and a new meta-tool would pass silently. They now assert the exact sorted key set of the ToolSet handed to the executor. This immediately caught real drift: the toolset has seven tools, and `get_tool_catalog` was missing from the expected list. **2. Catalog contract integration test** (`test/integration/ai/suites/mcp-tool-catalog.integration-spec.ts`). Calls `get_tool_catalog` over real HTTP with an API-key bearer, then for every advertised category dispatches one read-only tool (`find_/list_/get_/search_` prefixed, up to 3 candidates) through `execute_tool` and asserts a success envelope. Any newly registered provider is covered the moment it appears in the catalog, with no new test code. Categories with no read-only tool are compared exactly against a deliberate exception list, currently empty since every advertised category ships a read-only tool, so drift in either direction fails loudly. **3. Permission gating integration test** (same suite). Creates two API keys: one bound to Admin, one bound to a freshly created role with `canUpdateAllSettings: false` and no settings flags. Asserts the ROLE category (from #23613) is present in the admin catalog and absent from the restricted one, while the restricted key still sees DATABASE_CRUD read tools, proving it is gating rather than a broken catalog. This locks the provider `isAvailable` contract at the real HTTP boundary, which the unit mocks cannot. ## Testing - `npx jest src/engine/api/mcp` — 43 tests pass - `test/integration/ai/suites` — 4 suites, 24 tests pass locally against a reset DB - `npx nx typecheck twenty-server` clean; oxlint and oxfmt clean on the touched files --- _Generated by [Claude Code](https://claude.ai/code/session_0131sLKVsRuaDoaKCxFM8g4Z)_ ``<img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg">`` --- .../__tests__/mcp-protocol.service.spec.ts | 20 +- .../mcp-tool-catalog.integration-spec.ts | 249 ++++++++++++++++++ 2 files changed, 267 insertions(+), 2 deletions(-) create mode 100644 packages/twenty-server/test/integration/ai/suites/mcp-tool-catalog.integration-spec.ts diff --git a/packages/twenty-server/src/engine/api/mcp/services/__tests__/mcp-protocol.service.spec.ts b/packages/twenty-server/src/engine/api/mcp/services/__tests__/mcp-protocol.service.spec.ts index 240cd4e263..41789f0926 100644 --- a/packages/twenty-server/src/engine/api/mcp/services/__tests__/mcp-protocol.service.spec.ts +++ b/packages/twenty-server/src/engine/api/mcp/services/__tests__/mcp-protocol.service.spec.ts @@ -18,6 +18,7 @@ import { type McpToolAnnotations } from 'src/engine/api/mcp/types/mcp-tool-annot import { type FlatApiKey } from 'src/engine/core-modules/api-key/types/flat-api-key.type'; import { ApiKeyRoleService } from 'src/engine/core-modules/api-key/services/api-key-role.service'; import { EXECUTE_TOOL_TOOL_NAME } from 'src/engine/core-modules/tool-provider/tools/execute-tool.tool'; +import { GET_TOOL_CATALOG_TOOL_NAME } from 'src/engine/core-modules/tool-provider/tools/get-tool-catalog.tool'; import { LEARN_TOOLS_TOOL_NAME } from 'src/engine/core-modules/tool-provider/tools/learn-tools.tool'; import { LOAD_SKILL_TOOL_NAME } from 'src/engine/core-modules/tool-provider/tools/load-skill.tool'; import { ToolRegistryService } from 'src/engine/core-modules/tool-provider/services/tool-registry.service'; @@ -46,6 +47,7 @@ describe('McpProtocolService', () => { const EXPECTED_MCP_TOOL_NAMES = [ LEARN_TOOLS_TOOL_NAME, EXECUTE_TOOL_TOOL_NAME, + GET_TOOL_CATALOG_TOOL_NAME, LOAD_SKILL_TOOL_NAME, LIST_OBJECT_METADATA_NAMES_TOOL_NAME, LIST_SKILLS_TOOL_NAME, @@ -58,6 +60,7 @@ describe('McpProtocolService', () => { > = { [LEARN_TOOLS_TOOL_NAME]: MCP_CLOSED_WORLD_READ_ONLY_TOOL_ANNOTATIONS, [EXECUTE_TOOL_TOOL_NAME]: MCP_EXECUTE_TOOL_ANNOTATIONS, + [GET_TOOL_CATALOG_TOOL_NAME]: MCP_CLOSED_WORLD_READ_ONLY_TOOL_ANNOTATIONS, [LOAD_SKILL_TOOL_NAME]: MCP_CLOSED_WORLD_READ_ONLY_TOOL_ANNOTATIONS, [LIST_OBJECT_METADATA_NAMES_TOOL_NAME]: MCP_CLOSED_WORLD_READ_ONLY_TOOL_ANNOTATIONS, @@ -265,7 +268,7 @@ describe('McpProtocolService', () => { expect(result).toBeNull(); }); - it('should build a ToolSet with exactly 6 tools and pass it to executor for tools/call', async () => { + it('should build the meta-tool set by name and pass it to executor for tools/call', async () => { userRoleService.getRoleIdForUserWorkspace.mockResolvedValue(mockRoleId); const mockToolCallResponse = { @@ -314,9 +317,15 @@ describe('McpProtocolService', () => { mockRequest.params, undefined, ); + + const [, toolSet] = mcpToolExecutorService.handleToolCall.mock.calls[0]; + + expect(Object.keys(toolSet).sort()).toEqual( + [...EXPECTED_MCP_TOOL_NAMES].sort(), + ); }); - it('should build a ToolSet with exactly 6 tools and pass it to executor for tools/list', async () => { + it('should build the meta-tool set by name and pass it to executor for tools/list', async () => { userRoleService.getRoleIdForUserWorkspace.mockResolvedValue(mockRoleId); mcpToolExecutorService.handleToolsListing.mockReturnValue({ @@ -351,6 +360,13 @@ describe('McpProtocolService', () => { ), ), ); + + const [, toolSet] = + mcpToolExecutorService.handleToolsListing.mock.calls[0]; + + expect(Object.keys(toolSet).sort()).toEqual( + [...EXPECTED_MCP_TOOL_NAMES].sort(), + ); }); it('should pass actorContext with FieldActorSource.AGENT to getToolsByName', async () => { diff --git a/packages/twenty-server/test/integration/ai/suites/mcp-tool-catalog.integration-spec.ts b/packages/twenty-server/test/integration/ai/suites/mcp-tool-catalog.integration-spec.ts new file mode 100644 index 0000000000..7ffd3aa252 --- /dev/null +++ b/packages/twenty-server/test/integration/ai/suites/mcp-tool-catalog.integration-spec.ts @@ -0,0 +1,249 @@ +import { gql } from 'graphql-tag'; +import request from 'supertest'; +import { generateApiKeyToken } from 'test/integration/graphql/utils/generate-api-key-token.util'; +import { createOneRole } from 'test/integration/metadata/suites/role/utils/create-one-role.util'; +import { deleteOneRole } from 'test/integration/metadata/suites/role/utils/delete-one-role.util'; +import { makeMetadataAPIRequest } from 'test/integration/metadata/suites/utils/make-metadata-api-request.util'; +import { jestExpectToBeDefined } from 'test/utils/jest-expect-to-be-defined.util.test'; +import { ToolCategory } from 'twenty-shared/ai'; + +/** + * Contract tests for the MCP tool catalog. + * + * 1. Catalog contract: every category advertised by get_tool_catalog must be + * dispatchable end to end through execute_tool. Scales automatically: a + * newly registered provider is covered the moment it appears in the + * catalog, with no new test code. + * 2. Permission gating: the catalog is role-dependent. An API key bound to a + * role without settings permissions must not see settings-gated tools + * (e.g. the ROLE category), while an admin-bound key must. + */ + +const baseUrl = `http://localhost:${APP_PORT}`; + +const postMcp = (body: object, bearer: string) => + request(baseUrl) + .post('/mcp') + .set('Authorization', `Bearer ${bearer}`) + .set('Content-Type', 'application/json') + .set('Accept', 'application/json') + .send(JSON.stringify(body)); + +const callMcpTool = async ( + bearer: string, + toolName: string, + toolArguments: object, +) => { + const response = await postMcp( + { + jsonrpc: '2.0', + method: 'tools/call', + params: { name: toolName, arguments: toolArguments }, + id: '1', + }, + bearer, + ).expect(200); + + return response.body.result as { + content: { type: string; text: string }[]; + isError: boolean; + }; +}; + +const getToolCatalog = async ( + bearer: string, +): Promise> => { + const result = await callMcpTool(bearer, 'get_tool_catalog', {}); + + expect(result.isError).toBe(false); + + return JSON.parse(result.content[0].text).catalog; +}; + +const READ_ONLY_TOOL_NAME_PATTERN = /^(find_|list_|get_|search_)/; + +// Deliberate exceptions to the "every advertised category is dispatchable +// through a read-only tool" contract. Currently none: every category the MCP +// catalog advertises ships at least one read-only tool. Adding a category +// here must be a conscious decision, not silent drift. +const EXPECTED_CATEGORIES_WITHOUT_READ_ONLY_TOOLS: string[] = []; + +const createApiKeyToken = async (roleId: string): Promise => { + const createResponse = await makeMetadataAPIRequest({ + query: gql` + mutation CreateApiKey($input: CreateApiKeyInput!) { + createApiKey(input: $input) { + id + } + } + `, + variables: { + input: { + name: `MCP catalog test key ${roleId}`, + expiresAt: new Date(Date.now() + 60 * 60 * 1000).toISOString(), + roleId, + }, + }, + }); + + const apiKeyId = createResponse.body.data?.createApiKey?.id; + + jestExpectToBeDefined(apiKeyId); + createdApiKeyIds.push(apiKeyId); + + const tokenResponse = await generateApiKeyToken({ + apiKeyId, + accessToken: APPLE_JANE_ADMIN_ACCESS_TOKEN, + }); + + const token = tokenResponse.body.data?.generateApiKeyToken?.token; + + jestExpectToBeDefined(token); + + return token; +}; + +const createdApiKeyIds: string[] = []; + +describe('MCP tool catalog (integration)', () => { + let adminApiKeyToken: string; + let restrictedApiKeyToken: string; + let restrictedRoleId: string; + + beforeAll(async () => { + const rolesResponse = await makeMetadataAPIRequest({ + query: gql` + query GetRoles { + getRoles { + id + label + } + } + `, + }); + + const adminRoleId = rolesResponse.body.data?.getRoles?.find( + (role: { label: string }) => role.label === 'Admin', + )?.id; + + jestExpectToBeDefined(adminRoleId); + + const { data: restrictedRoleData } = await createOneRole({ + expectToFail: false, + input: { + label: 'MCP Catalog Restricted Role', + description: 'API-key role without settings permissions', + icon: 'IconKey', + canUpdateAllSettings: false, + canAccessAllTools: true, + canReadAllObjectRecords: true, + canUpdateAllObjectRecords: false, + canSoftDeleteAllObjectRecords: false, + canDestroyAllObjectRecords: false, + canBeAssignedToUsers: false, + canBeAssignedToAgents: false, + canBeAssignedToApiKeys: true, + }, + }); + + restrictedRoleId = restrictedRoleData?.createOneRole?.id as string; + jestExpectToBeDefined(restrictedRoleId); + + adminApiKeyToken = await createApiKeyToken(adminRoleId); + restrictedApiKeyToken = await createApiKeyToken(restrictedRoleId); + }); + + afterAll(async () => { + for (const apiKeyId of createdApiKeyIds) { + await testDataSource + .query('DELETE FROM core."apiKey" WHERE id = $1', [apiKeyId]) + .catch(() => {}); + } + + if (restrictedRoleId) { + await deleteOneRole({ + expectToFail: false, + input: { idToDelete: restrictedRoleId }, + }); + } + }); + + describe('catalog contract', () => { + it('should dispatch one read-only tool per advertised category through execute_tool', async () => { + const catalog = await getToolCatalog(adminApiKeyToken); + const categories = Object.keys(catalog); + + expect(categories.length).toBeGreaterThan(0); + + const categoriesWithoutReadOnlyTool: string[] = []; + + for (const category of categories) { + const readOnlyCandidates = catalog[category] + .filter((tool) => READ_ONLY_TOOL_NAME_PATTERN.test(tool.name)) + .slice(0, 3); + + if (readOnlyCandidates.length === 0) { + // A write-only category has nothing safe to dispatch in CI; it is + // collected and checked against the deliberate exception list below. + categoriesWithoutReadOnlyTool.push(category); + continue; + } + + let dispatched = false; + + for (const candidate of readOnlyCandidates) { + const result = await callMcpTool(adminApiKeyToken, 'execute_tool', { + toolName: candidate.name, + arguments: {}, + }); + + if (!result.isError) { + dispatched = true; + break; + } + } + + expect({ category, dispatched }).toEqual({ + category, + dispatched: true, + }); + } + + // Exact equality fails in both directions, so gaining or losing a + // skipped category forces a deliberate update of the exception list. + expect([...categoriesWithoutReadOnlyTool].sort()).toEqual( + EXPECTED_CATEGORIES_WITHOUT_READ_ONLY_TOOLS, + ); + }); + }); + + describe('permission gating', () => { + it('should expose role tools to an admin-bound API key', async () => { + const catalog = await getToolCatalog(adminApiKeyToken); + + jestExpectToBeDefined(catalog[ToolCategory.ROLE]); + + const roleToolNames = catalog[ToolCategory.ROLE].map((tool) => tool.name); + + expect(roleToolNames).toEqual( + expect.arrayContaining(['list_roles', 'create_role', 'update_role']), + ); + }); + + it('should hide role tools from an API key without settings permissions', async () => { + const catalog = await getToolCatalog(restrictedApiKeyToken); + + expect(catalog[ToolCategory.ROLE]).toBeUndefined(); + + const allToolNames = Object.values(catalog) + .flat() + .map((tool) => tool.name); + + expect(allToolNames).not.toEqual(expect.arrayContaining(['create_role'])); + + // The restricted role still sees record read tools, proving the empty + // ROLE category is gating rather than a broken catalog. + expect(catalog[ToolCategory.DATABASE_CRUD]?.length).toBeGreaterThan(0); + }); + }); +});