diff --git a/src/mcp/AGENTS.md b/src/mcp/AGENTS.md index d3f73d62..45081183 100644 --- a/src/mcp/AGENTS.md +++ b/src/mcp/AGENTS.md @@ -34,7 +34,7 @@ Two MCP protocol revisions are served, each by its own adapter: - `tool_call_engine.ts` — shared `tools/call` orchestration. `prepareToolCall()` handles preparation; `executeSyncToolCall()` runs synchronous calls. - `client.ts` — `connectMCPClient(url, token)`: transport negotiation. -- `proxy.ts` — MCP-in-MCP: `getMCPServerID(url)`. +- `proxy.ts` — MCP-in-MCP: `getMCPServerID(url)`, `getProxyMCPServerToolName(url, toolName)`. - `actors.ts` — `getActorMCPServerPath()`: parses an Actor's `webServerMcpPath`. - `utils.ts` — `processParamsGetTools()`: turns `?actors=` URL params into tools. - `tool_call_error_mapper.ts` — shared tool-call error classification. @@ -61,8 +61,9 @@ Two MCP protocol revisions are served, each by its own adapter: each other. - **Tool names: capped + hash-deduped.** Names are capped at `MAX_TOOL_NAME_LENGTH`; over-length or colliding names get a `TOOL_NAME_HASH_LENGTH` hash suffix so the - exposed set stays unique within the limit (the hashing is in `../tools/actor_tool_naming.ts`). - Never widen the cap — downstream clients depend on it. + exposed set stays unique within the limit (Actor tools: `../tools/actor_tool_naming.ts`; + proxied Actor-MCP tools: `proxy.ts` `getProxyMCPServerToolName`). Never widen the + cap — downstream clients depend on it. - **Proxy server IDs are keyed by URL, not Actor ID.** `getMCPServerID(url)` is `sha256(url)` sliced to `SERVER_ID_LENGTH`. One Actor can expose both an SSE and a streamable endpoint; keying by URL keeps those distinct. Keying by Actor ID would diff --git a/src/mcp/proxy.ts b/src/mcp/proxy.ts index 66256e4c..fdb9f6bc 100644 --- a/src/mcp/proxy.ts +++ b/src/mcp/proxy.ts @@ -6,7 +6,7 @@ import { fixedAjvCompile } from '../tools/actor_input_schema.js'; import type { ActorMcpTool, ToolEntry } from '../types.js'; import { TOOL_TYPE } from '../types.js'; import { ajv } from '../utils/ajv.js'; -import { MAX_TOOL_NAME_LENGTH, SERVER_ID_LENGTH } from './const.js'; +import { MAX_TOOL_NAME_LENGTH, SERVER_ID_LENGTH, TOOL_NAME_HASH_LENGTH } from './const.js'; /** * Generates a unique server ID by hashing the URL. @@ -20,14 +20,20 @@ export function getMCPServerID(url: string): string { } /** - * Prefixes the tool name with the server ID hash and truncates to MAX_TOOL_NAME_LENGTH. - * Truncation can in theory collide two different origin tool names. + * Prefixes the tool name with the server ID hash. Over-length names get a hash suffix + * (same pattern as actor tool names) so bare truncation cannot collide two different + * origin tool names into one exposed name. */ -function getProxyMCPServerToolName(url: string, toolName: string): string { +export function getProxyMCPServerToolName(url: string, toolName: string): string { const prefix = getMCPServerID(url); - const fullName = `${prefix}-${toolName}`; - return fullName.slice(0, MAX_TOOL_NAME_LENGTH); + + if (fullName.length <= MAX_TOOL_NAME_LENGTH) { + return fullName; + } + + const hash = createHash('sha256').update(fullName).digest('hex').slice(0, TOOL_NAME_HASH_LENGTH); + return `${fullName.slice(0, MAX_TOOL_NAME_LENGTH - TOOL_NAME_HASH_LENGTH - 1)}-${hash}`; } export async function getMCPServerTools(actorID: string, client: Client, serverUrl: string): Promise { diff --git a/src/mcp/tool_dispatch.ts b/src/mcp/tool_dispatch.ts index 21616954..2a924a38 100644 --- a/src/mcp/tool_dispatch.ts +++ b/src/mcp/tool_dispatch.ts @@ -181,6 +181,7 @@ export async function dispatchToolCall(params: { CallToolResultSchema, { timeout: EXTERNAL_TOOL_CALL_TIMEOUT_MSEC, + signal, }, ); @@ -198,6 +199,11 @@ export async function dispatchToolCall(params: { result = { ...res }; } catch (error) { + if (signal.aborted) { + // Yield a macrotask first: the SDK sends notifications/cancelled fire-and-forget on + // the transport's AbortController, which the finally's close() would abort. + await new Promise((resolve) => setImmediate(resolve)); + } ({ toolStatus, callDiagnostics } = buildExecutionDiagnostics({ error, isAborted: Boolean(signal.aborted), diff --git a/src/tools/actors/call_actor.ts b/src/tools/actors/call_actor.ts index 3e30be29..44aaacb0 100644 --- a/src/tools/actors/call_actor.ts +++ b/src/tools/actors/call_actor.ts @@ -1,5 +1,6 @@ import type { Client } from '@modelcontextprotocol/sdk/client/index.js'; import type { ContentBlock } from '@modelcontextprotocol/sdk/types.js'; +import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js'; import dedent from 'dedent'; import { z } from 'zod'; @@ -15,6 +16,7 @@ import { } from '../../const.js'; import { ACTOR_LOAD_ERROR_KIND, ActorLoadError } from '../../errors.js'; import { connectMCPClient } from '../../mcp/client.js'; +import { EXTERNAL_TOOL_CALL_TIMEOUT_MSEC } from '../../mcp/const.js'; import type { PaymentProvider } from '../../payments/types.js'; import type { ActorInfo, ApifyToken, InternalToolArgs, ToolEntry, ToolInputSchema } from '../../types.js'; import { TOOL_TYPE } from '../../types.js'; @@ -324,8 +326,10 @@ export async function handleMcpToolCall(params: { mcpServerUrl: string | false; apifyToken: string; mcpSessionId?: string; + signal: AbortSignal; }): Promise { - const { baseActorName, mcpToolName, input, isActorMcpServer, mcpServerUrl, apifyToken, mcpSessionId } = params; + const { baseActorName, mcpToolName, input, isActorMcpServer, mcpServerUrl, apifyToken, mcpSessionId, signal } = + params; if (!isActorMcpServer) { return respondServerError(`Actor '${baseActorName}' is not an MCP server.`); @@ -337,6 +341,10 @@ export async function handleMcpToolCall(params: { ); } + if (signal.aborted) { + return respondAborted(); + } + let client: Client | null = null; try { client = await connectMCPClient(mcpServerUrl as string, apifyToken, mcpSessionId); @@ -344,10 +352,17 @@ export async function handleMcpToolCall(params: { return respondServerError(`Failed to connect to MCP server ${mcpServerUrl}`); } - const result = await client.callTool({ - name: mcpToolName, - arguments: input, - }); + const result = await client.callTool( + { + name: mcpToolName, + arguments: input, + }, + CallToolResultSchema, + { + timeout: EXTERNAL_TOOL_CALL_TIMEOUT_MSEC, + signal, + }, + ); // `call-actor` declares `actorRunOutputSchema`, so MCP SDK ≥ 1.11.4 rejects any response // without `structuredContent` (unless `isError: true`) with -32600. The pass-through has no @@ -369,6 +384,12 @@ export async function handleMcpToolCall(params: { }, }); } catch (error) { + if (signal.aborted) { + // Yield a macrotask first: the SDK sends notifications/cancelled fire-and-forget on the + // transport's AbortController, which the finally's close() would abort before it flushes. + await new Promise((resolve) => setImmediate(resolve)); + return respondAborted(); + } logHttpError(error, `Failed to call MCP tool '${mcpToolName}' on Actor '${baseActorName}'`, { actorName: baseActorName, toolName: mcpToolName, @@ -529,6 +550,7 @@ export async function callActorPreExecute( mcpServerUrl: mcpServerUrlOrFalse, apifyToken, mcpSessionId, + signal: toolArgs.signal, }); if (mcpResult) { return { earlyResponse: mcpResult }; diff --git a/tests/unit/mcp.proxy.test.ts b/tests/unit/mcp.proxy.test.ts new file mode 100644 index 00000000..333ca8f3 --- /dev/null +++ b/tests/unit/mcp.proxy.test.ts @@ -0,0 +1,51 @@ +import { createHash } from 'node:crypto'; + +import { describe, expect, it } from 'vitest'; + +import { MAX_TOOL_NAME_LENGTH, SERVER_ID_LENGTH, TOOL_NAME_HASH_LENGTH } from '../../src/mcp/const.js'; +import { getMCPServerID, getProxyMCPServerToolName } from '../../src/mcp/proxy.js'; + +describe('getMCPServerID()', () => { + it('returns a stable SERVER_ID_LENGTH hex prefix of sha256(url)', () => { + const url = 'https://example.com/mcp'; + const expected = createHash('sha256').update(url).digest('hex').slice(0, SERVER_ID_LENGTH); + expect(getMCPServerID(url)).toBe(expected); + expect(getMCPServerID(url)).toBe(getMCPServerID(url)); + }); + + it('keys by URL so SSE and streamable endpoints stay distinct', () => { + expect(getMCPServerID('https://actor.example/sse')).not.toBe(getMCPServerID('https://actor.example/mcp')); + }); +}); + +describe('getProxyMCPServerToolName()', () => { + const url = 'https://example.com/mcp'; + + it('returns prefix-toolName when under the length cap', () => { + const name = getProxyMCPServerToolName(url, 'list-items'); + expect(name).toBe(`${getMCPServerID(url)}-list-items`); + expect(name.length).toBeLessThanOrEqual(MAX_TOOL_NAME_LENGTH); + }); + + it('hash-suffixes over-length names instead of bare truncation', () => { + const longTool = `very-long-origin-tool-name-${'x'.repeat(80)}`; + const fullName = `${getMCPServerID(url)}-${longTool}`; + const hash = createHash('sha256').update(fullName).digest('hex').slice(0, TOOL_NAME_HASH_LENGTH); + const name = getProxyMCPServerToolName(url, longTool); + + expect(name.length).toBe(MAX_TOOL_NAME_LENGTH); + expect(name.endsWith(`-${hash}`)).toBe(true); + // Bare slice would drop the distinguishing suffix and collide; hash must survive. + expect(name).not.toBe(fullName.slice(0, MAX_TOOL_NAME_LENGTH)); + }); + + it('keeps two over-length origin names distinct after capping', () => { + const sharedPrefix = `shared-prefix-${'y'.repeat(80)}`; + const a = getProxyMCPServerToolName(url, `${sharedPrefix}-alpha`); + const b = getProxyMCPServerToolName(url, `${sharedPrefix}-beta`); + + expect(a).not.toBe(b); + expect(a.length).toBe(MAX_TOOL_NAME_LENGTH); + expect(b.length).toBe(MAX_TOOL_NAME_LENGTH); + }); +}); diff --git a/tests/unit/mcp.server.tool_call_contracts.test.ts b/tests/unit/mcp.server.tool_call_contracts.test.ts index 25be1470..52392adc 100644 --- a/tests/unit/mcp.server.tool_call_contracts.test.ts +++ b/tests/unit/mcp.server.tool_call_contracts.test.ts @@ -702,6 +702,37 @@ describe('ACTOR_MCP remote-McpError containment (sync tools/call catch)', () => expect(properties.failure_category).toBe(FAILURE_CATEGORY.INVALID_INPUT); }); + it('forwards the abort signal into the remote Actor-MCP callTool options', async () => { + // Without `signal` in callTool options, cancel/disconnect cannot stop the remote call before + // EXTERNAL_TOOL_CALL_TIMEOUT_MSEC — ACTOR/INTERNAL already abort; ACTOR_MCP must match. + await withServer(async (server) => { + silenceLogs(); + const callTool = vi.fn().mockResolvedValue({ content: [{ type: 'text', text: 'ok' }] }); + const stubClient = { + callTool, + close: vi.fn().mockResolvedValue(undefined), + setNotificationHandler: vi.fn(), + } as unknown as Client; + vi.spyOn(mcpClient, 'connectMCPClient').mockResolvedValue(stubClient); + + const controller = new AbortController(); + server.upsertTools([makeActorMcpTool()]); + const handler = getRequestHandler(server, 'tools/call'); + await handler( + { + method: 'tools/call', + params: { name: 'test-actor-mcp-tool', arguments: {}, _meta: { mcpSessionId: 's1' } }, + }, + { signal: controller.signal, sendNotification: vi.fn() }, + ); + + expect(callTool).toHaveBeenCalledTimes(1); + const options = callTool.mock.calls[0][2] as { signal?: AbortSignal; timeout?: number }; + expect(options.signal).toBe(controller.signal); + expect(options.timeout).toBeGreaterThan(0); + }); + }); + it('re-throws an escaped McpError as a JSON-RPC error, not an isError tool result', async () => { // Escaped McpErrors must remain JSON-RPC errors, including 402-coded errors. await withServer(async (server) => { diff --git a/tests/unit/tools.call_actor_common.test.ts b/tests/unit/tools.call_actor_common.test.ts index d5d7b32c..8fd4be6c 100644 --- a/tests/unit/tools.call_actor_common.test.ts +++ b/tests/unit/tools.call_actor_common.test.ts @@ -1,3 +1,4 @@ +import type { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { ApifyApiError } from 'apify-client'; import type { AxiosResponse } from 'axios'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -8,11 +9,14 @@ import { HELPER_TOOLS, TOOL_STATUS, } from '../../src/const.js'; +import * as mcpClient from '../../src/mcp/client.js'; +import { EXTERNAL_TOOL_CALL_TIMEOUT_MSEC } from '../../src/mcp/const.js'; import { buildCallActorAppsDescription, buildCallActorDescription, buildCallActorErrorResponse, callActorArgs, + handleMcpToolCall, resolveAndValidateActor, } from '../../src/tools/actors/call_actor.js'; import type { InternalToolArgs, ToolEntry } from '../../src/types.js'; @@ -331,4 +335,80 @@ describe('call_actor_common', () => { }); }); }); + + describe('handleMcpToolCall()', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it('forwards signal and timeout into client.callTool options', async () => { + const callTool = vi.fn().mockResolvedValue({ + content: [{ type: 'text', text: 'remote ok' }], + isError: false, + }); + vi.spyOn(mcpClient, 'connectMCPClient').mockResolvedValue({ + callTool, + close: vi.fn().mockResolvedValue(undefined), + } as unknown as Client); + + const controller = new AbortController(); + const result = await handleMcpToolCall({ + baseActorName: 'apify/mcp-demo', + mcpToolName: 'search', + input: { q: 'x' }, + isActorMcpServer: true, + mcpServerUrl: 'https://example.invalid/mcp', + apifyToken: 'token', + signal: controller.signal, + }); + + expect(result?.isError).not.toBe(true); + expect(callTool).toHaveBeenCalledTimes(1); + const options = callTool.mock.calls[0][2] as { signal?: AbortSignal; timeout?: number }; + expect(options.signal).toBe(controller.signal); + expect(options.timeout).toBe(EXTERNAL_TOOL_CALL_TIMEOUT_MSEC); + }); + + it('returns aborted when the signal is already aborted before the remote call', async () => { + const connectSpy = vi.spyOn(mcpClient, 'connectMCPClient').mockResolvedValue(null); + const controller = new AbortController(); + controller.abort(); + + const result = await handleMcpToolCall({ + baseActorName: 'apify/mcp-demo', + mcpToolName: 'search', + input: { q: 'x' }, + isActorMcpServer: true, + mcpServerUrl: 'https://example.invalid/mcp', + apifyToken: 'token', + signal: controller.signal, + }); + + expect(result).toEqual({}); + expect(connectSpy).not.toHaveBeenCalled(); + }); + + it('returns aborted when the remote call rejects after the signal aborts', async () => { + const controller = new AbortController(); + vi.spyOn(mcpClient, 'connectMCPClient').mockResolvedValue({ + callTool: vi.fn().mockImplementation(async () => { + controller.abort(); + throw new Error('aborted'); + }), + close: vi.fn().mockResolvedValue(undefined), + } as unknown as Client); + + const result = await handleMcpToolCall({ + baseActorName: 'apify/mcp-demo', + mcpToolName: 'search', + input: { q: 'x' }, + isActorMcpServer: true, + mcpServerUrl: 'https://example.invalid/mcp', + apifyToken: 'token', + signal: controller.signal, + }); + + expect(result).toEqual({}); + }); + }); });