From 9245af4e45b9415b5d59846299da7ece62acfaec Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 18 May 2026 16:12:15 +0200 Subject: [PATCH 1/3] fix(workflow-executor): align getMcpServerConfigs port with Record shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The orchestrator's /liana/mcp-server-configs-with-details endpoint returns Record, but the executor port was typed McpConfiguration[] and runner.fetchRemoteTools called .map on it — every MCP-typed step crashed with "TypeError: configs.map is not a function" before reaching loadRemoteTools. Tests masked the bug by mocking mockResolvedValue([]) at 8 call sites, which matched the wrong type and short-circuited the buggy branch. Refs: PRD-357 --- packages/workflow-executor/package.json | 4 +- .../adapters/forest-server-workflow-port.ts | 6 +-- packages/workflow-executor/src/index.ts | 2 +- .../src/ports/workflow-port.ts | 8 ++-- packages/workflow-executor/src/runner.ts | 16 ++------ .../forest-server-workflow-port.test.ts | 14 ++++++- .../load-related-record-step-executor.test.ts | 2 +- .../test/executors/mcp-step-executor.test.ts | 2 +- .../read-record-step-executor.test.ts | 2 +- ...rigger-record-action-step-executor.test.ts | 2 +- .../update-record-step-executor.test.ts | 2 +- .../test/http/executor-http-server.test.ts | 2 +- .../integration/workflow-execution.test.ts | 6 +-- .../workflow-executor/test/runner.test.ts | 40 +++++++++++++++++-- yarn.lock | 15 ------- 15 files changed, 70 insertions(+), 53 deletions(-) diff --git a/packages/workflow-executor/package.json b/packages/workflow-executor/package.json index bf0217813b..b8fc33da6d 100644 --- a/packages/workflow-executor/package.json +++ b/packages/workflow-executor/package.json @@ -27,9 +27,9 @@ }, "dependencies": { "@forestadmin/agent-client": "1.5.6", - "@forestadmin/ai-proxy": "1.8.0", + "@forestadmin/ai-proxy": "1.9.0", "@langchain/openai": "1.2.5", - "@forestadmin/forestadmin-client": "1.39.5", + "@forestadmin/forestadmin-client": "1.39.7", "@koa/bodyparser": "^6.1.0", "@koa/router": "^13.1.0", "jsonwebtoken": "^9.0.3", diff --git a/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts b/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts index 5fc111d2e2..0c0203c9f4 100644 --- a/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts +++ b/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts @@ -4,7 +4,7 @@ import type { AvailableRunDispatch, AvailableRunsBatch, MalformedRunInfo, - McpConfiguration, + McpServers, WorkflowPort, } from '../ports/workflow-port'; import type { StepUser } from '../types/execution-context'; @@ -203,10 +203,10 @@ export default class ForestServerWorkflowPort implements WorkflowPort { ); } - async getMcpServerConfigs(): Promise { + async getMcpServerConfigs(): Promise { return this.callPort( 'getMcpServerConfigs', - () => ServerUtils.query(this.options, 'get', ROUTES.mcpServerConfigs), + () => ServerUtils.query(this.options, 'get', ROUTES.mcpServerConfigs), { retry: true }, ); } diff --git a/packages/workflow-executor/src/index.ts b/packages/workflow-executor/src/index.ts index 607cf2fe9c..51e8db149d 100644 --- a/packages/workflow-executor/src/index.ts +++ b/packages/workflow-executor/src/index.ts @@ -68,7 +68,7 @@ export type { Limit, UpdateRecordQuery, } from './ports/agent-port'; -export type { McpConfiguration, WorkflowPort } from './ports/workflow-port'; +export type { McpConfiguration, McpServers, WorkflowPort } from './ports/workflow-port'; export type { RunStore } from './ports/run-store'; export type { Logger } from './ports/logger-port'; export { default as ConsoleLogger } from './adapters/console-logger'; diff --git a/packages/workflow-executor/src/ports/workflow-port.ts b/packages/workflow-executor/src/ports/workflow-port.ts index 7eee6419b5..76cb8dddcb 100644 --- a/packages/workflow-executor/src/ports/workflow-port.ts +++ b/packages/workflow-executor/src/ports/workflow-port.ts @@ -3,9 +3,9 @@ import type { AvailableStepExecution, StepUser } from '../types/execution-context'; import type { CollectionSchema } from '../types/validated/collection'; import type { StepOutcome } from '../types/validated/step-outcome'; -import type { McpConfiguration } from '@forestadmin/ai-proxy'; +import type { McpConfiguration, McpServers } from '@forestadmin/ai-proxy'; -export type { McpConfiguration }; +export type { McpConfiguration, McpServers }; export interface MalformedRunInfo { runId: string; @@ -39,6 +39,8 @@ export interface WorkflowPort { stepOutcome: StepOutcome, ): Promise; getCollectionSchema(collectionName: string, runId: string): Promise; - getMcpServerConfigs(): Promise; + // The orchestrator's GET /liana/mcp-server-configs-with-details returns a + // Record map keyed by server name — not an array. + getMcpServerConfigs(): Promise; hasRunAccess(runId: string, user: StepUser): Promise; } diff --git a/packages/workflow-executor/src/runner.ts b/packages/workflow-executor/src/runner.ts index d43df7562a..16a6f115a1 100644 --- a/packages/workflow-executor/src/runner.ts +++ b/packages/workflow-executor/src/runner.ts @@ -4,12 +4,7 @@ import type { AgentPort } from './ports/agent-port'; import type { AiModelPort } from './ports/ai-model-port'; import type { Logger } from './ports/logger-port'; import type { RunStore } from './ports/run-store'; -import type { - AvailableRunDispatch, - MalformedRunInfo, - McpConfiguration, - WorkflowPort, -} from './ports/workflow-port'; +import type { AvailableRunDispatch, MalformedRunInfo, WorkflowPort } from './ports/workflow-port'; import type SchemaCache from './schema-cache'; import type { AvailableStepExecution, StepExecutionResult } from './types/execution-context'; import type { StepExecutionData } from './types/step-execution-data'; @@ -262,14 +257,9 @@ export default class Runner { private async fetchRemoteTools(): Promise { const configs = await this.config.workflowPort.getMcpServerConfigs(); - if (configs.length === 0) return []; + if (Object.keys(configs).length === 0) return []; - const mergedConfig: McpConfiguration = { - ...configs[0], - configs: Object.assign({}, ...configs.map(c => c.configs)), - }; - - return this.config.aiModelPort.loadRemoteTools(mergedConfig); + return this.config.aiModelPort.loadRemoteTools({ configs }); } private executeStep( diff --git a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts index dc4c27f1f7..269c88ca81 100644 --- a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts +++ b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts @@ -732,8 +732,10 @@ describe('ForestServerWorkflowPort', () => { }); describe('getMcpServerConfigs', () => { - it('fetches mcp server configs', async () => { - const configs = [{ name: 'mcp-1' }]; + it('returns the Record map verbatim from the orchestrator', async () => { + const configs = { + 'data-gouv': { url: 'https://mcp.example.com', type: 'http', headers: {} }, + }; mockQuery.mockResolvedValue(configs); const result = await port.getMcpServerConfigs(); @@ -745,6 +747,14 @@ describe('ForestServerWorkflowPort', () => { ); expect(result).toEqual(configs); }); + + it('returns an empty Record when the orchestrator has no MCP servers configured', async () => { + mockQuery.mockResolvedValue({}); + + const result = await port.getMcpServerConfigs(); + + expect(result).toEqual({}); + }); }); describe('hasRunAccess', () => { diff --git a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts index e7fdab82ef..16ec0256cb 100644 --- a/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/load-related-record-step-executor.test.ts @@ -102,7 +102,7 @@ function makeMockWorkflowPort( schemasByCollection[name] ?? makeCollectionSchema({ collectionName: name }), ), ), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), }; } diff --git a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts index bd52a2e751..e58aa8346e 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -61,7 +61,7 @@ function makeMockWorkflowPort(): WorkflowPort { fields: [], actions: [], }), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), }; } diff --git a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts index 03e25f4a5b..87c708ca2f 100644 --- a/packages/workflow-executor/test/executors/read-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/read-record-step-executor.test.ts @@ -85,7 +85,7 @@ function makeMockWorkflowPort( schemasByCollection[name] ?? makeCollectionSchema({ collectionName: name }), ), ), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), }; } diff --git a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts index f2c62ee988..46d58934cd 100644 --- a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts @@ -87,7 +87,7 @@ function makeMockWorkflowPort( schemasByCollection[name] ?? makeCollectionSchema({ collectionName: name }), ), ), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), }; } diff --git a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts index 9c1866e092..10b35137eb 100644 --- a/packages/workflow-executor/test/executors/update-record-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/update-record-step-executor.test.ts @@ -85,7 +85,7 @@ function makeMockWorkflowPort( schemasByCollection[name] ?? makeCollectionSchema({ collectionName: name }), ), ), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), }; } diff --git a/packages/workflow-executor/test/http/executor-http-server.test.ts b/packages/workflow-executor/test/http/executor-http-server.test.ts index d27d9a4e17..bca0b23420 100644 --- a/packages/workflow-executor/test/http/executor-http-server.test.ts +++ b/packages/workflow-executor/test/http/executor-http-server.test.ts @@ -30,7 +30,7 @@ function createMockWorkflowPort(overrides: Partial = {}): Workflow getAvailableRun: jest.fn(), updateStepExecution: jest.fn().mockResolvedValue(undefined), getCollectionSchema: jest.fn(), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), ...overrides, } as unknown as WorkflowPort; diff --git a/packages/workflow-executor/test/integration/workflow-execution.test.ts b/packages/workflow-executor/test/integration/workflow-execution.test.ts index 103f25c14c..592bd7cb8c 100644 --- a/packages/workflow-executor/test/integration/workflow-execution.test.ts +++ b/packages/workflow-executor/test/integration/workflow-execution.test.ts @@ -139,7 +139,7 @@ function createMockWorkflowPort(overrides: Partial = {}): jest.Moc getAvailableRun: jest.fn().mockResolvedValue(null), updateStepExecution: jest.fn().mockResolvedValue(null), getCollectionSchema: jest.fn().mockResolvedValue(COLLECTION_SCHEMA), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), ...overrides, } as jest.Mocked; @@ -573,9 +573,7 @@ describe('workflow execution (integration)', () => { getAvailableRun: jest .fn() .mockResolvedValue({ step, auth: { forestServerToken: 'test-forest-token' } }), - getMcpServerConfigs: jest - .fn() - .mockResolvedValue([{ type: 'sse', configs: { 'mcp-1': { url: 'http://fake' } } }]), + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'mcp-1': { url: 'http://fake' } }), }); const { server, runStore } = createIntegrationSetup({ diff --git a/packages/workflow-executor/test/runner.test.ts b/packages/workflow-executor/test/runner.test.ts index f1bc218aa0..c4b63d4c69 100644 --- a/packages/workflow-executor/test/runner.test.ts +++ b/packages/workflow-executor/test/runner.test.ts @@ -45,7 +45,7 @@ function createMockWorkflowPort(): jest.Mocked { getAvailableRun: jest.fn(), updateStepExecution: jest.fn().mockResolvedValue(null), getCollectionSchema: jest.fn(), - getMcpServerConfigs: jest.fn().mockResolvedValue([]), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), hasRunAccess: jest.fn().mockResolvedValue(true), }; } @@ -1150,7 +1150,7 @@ describe('MCP lazy loading (via once thunk)', () => { expect(aiClient.loadRemoteTools).not.toHaveBeenCalled(); }); - it('calls fetchRemoteTools once for an McpTask step', async () => { + it('wraps the orchestrator Record-shape configs as { configs } and calls loadRemoteTools once', async () => { const workflowPort = createMockWorkflowPort(); const aiClient = createMockAiClient(); const step = makePendingStep({ @@ -1162,8 +1162,10 @@ describe('MCP lazy loading (via once thunk)', () => { step, auth: { forestServerToken: 'test-forest-token' }, }); - // Provide a non-empty config so fetchRemoteTools actually calls loadRemoteTools - workflowPort.getMcpServerConfigs.mockResolvedValue([{ configs: {} }] as never); + const realConfigs = { + 'data-gouv': { url: 'https://mcp.example.com', type: 'http' as const, headers: {} }, + }; + workflowPort.getMcpServerConfigs.mockResolvedValue(realConfigs); runner = new Runner( createRunnerConfig({ workflowPort, aiModelPort: aiClient as unknown as AiModelPort }), @@ -1172,6 +1174,36 @@ describe('MCP lazy loading (via once thunk)', () => { expect(workflowPort.getMcpServerConfigs).toHaveBeenCalledTimes(1); expect(aiClient.loadRemoteTools).toHaveBeenCalledTimes(1); + expect(aiClient.loadRemoteTools).toHaveBeenCalledWith({ configs: realConfigs }); + }); + + it('skips loadRemoteTools when the orchestrator returns an empty Record', async () => { + const workflowPort = createMockWorkflowPort(); + const aiClient = createMockAiClient(); + const step = makePendingStep({ + runId: 'run-1', + stepId: 'step-mcp-1', + stepType: StepType.Mcp, + }); + workflowPort.getAvailableRun.mockResolvedValue({ + step, + auth: { forestServerToken: 'test-forest-token' }, + }); + workflowPort.getMcpServerConfigs.mockResolvedValue({}); + + runner = new Runner( + createRunnerConfig({ workflowPort, aiModelPort: aiClient as unknown as AiModelPort }), + ); + await runner.triggerPoll('run-1'); + + expect(workflowPort.getMcpServerConfigs).toHaveBeenCalledTimes(1); + expect(aiClient.loadRemoteTools).not.toHaveBeenCalled(); + // Distinguish the short-circuit from a regression that throws before reaching the guard: + // the step must actually have executed and reported a success outcome. + expect(workflowPort.updateStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ status: 'success' }), + ); }); }); diff --git a/yarn.lock b/yarn.lock index 3bbada2fc3..b934602860 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1851,21 +1851,6 @@ jsonapi-serializer "^3.6.9" superagent "^10.3.0" -"@forestadmin/ai-proxy@1.8.0": - version "1.8.0" - resolved "https://registry.yarnpkg.com/@forestadmin/ai-proxy/-/ai-proxy-1.8.0.tgz#fac92da48c852484e4d18693388bf08918b41ef9" - integrity sha512-UrKM5hfo7WQAjVBJTspKNjixReVEsge2cVHBs1swcD/0sZlLBQHz7+wNE0YTuTFWOC0cSAKQz0UuKd0769Yciw== - dependencies: - "@forestadmin/agent-toolkit" "1.2.0" - "@forestadmin/datasource-toolkit" "1.53.1" - "@langchain/anthropic" "1.3.17" - "@langchain/community" "^1.1.19" - "@langchain/core" "1.1.15" - "@langchain/langgraph" "^1.1.0" - "@langchain/mcp-adapters" "1.1.1" - "@langchain/openai" "1.2.5" - zod "^4.3.5" - "@forestadmin/context@1.37.1": version "1.37.1" resolved "https://registry.yarnpkg.com/@forestadmin/context/-/context-1.37.1.tgz#301486c456061d43cb653b3e8be60644edb3f71a" From 480148cd71674be917961522b7ae316156e38788 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 18 May 2026 17:13:07 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(workflow-executor):=20address=20review?= =?UTF-8?q?=20=E2=80=94=20drop=20unused=20re-export,=20anonymize=20test=20?= =?UTF-8?q?server=20name?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @hercemer42: - Remove `McpConfiguration` re-export from the port and barrel: it's only used internally by the AiModelPort/adapters now, imported directly from @forestadmin/ai-proxy. - Drop the wire-shape comment on getMcpServerConfigs: the McpServers type and the route URL in the adapter are self-documenting. - Rename test config key from "data-gouv" to "mcp-server-1" to keep the test free of real-world references. Refs: PRD-357 --- packages/workflow-executor/src/index.ts | 2 +- packages/workflow-executor/src/ports/workflow-port.ts | 6 ++---- .../test/adapters/forest-server-workflow-port.test.ts | 2 +- packages/workflow-executor/test/runner.test.ts | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/workflow-executor/src/index.ts b/packages/workflow-executor/src/index.ts index 51e8db149d..9303e2affc 100644 --- a/packages/workflow-executor/src/index.ts +++ b/packages/workflow-executor/src/index.ts @@ -68,7 +68,7 @@ export type { Limit, UpdateRecordQuery, } from './ports/agent-port'; -export type { McpConfiguration, McpServers, WorkflowPort } from './ports/workflow-port'; +export type { McpServers, WorkflowPort } from './ports/workflow-port'; export type { RunStore } from './ports/run-store'; export type { Logger } from './ports/logger-port'; export { default as ConsoleLogger } from './adapters/console-logger'; diff --git a/packages/workflow-executor/src/ports/workflow-port.ts b/packages/workflow-executor/src/ports/workflow-port.ts index 76cb8dddcb..956539f566 100644 --- a/packages/workflow-executor/src/ports/workflow-port.ts +++ b/packages/workflow-executor/src/ports/workflow-port.ts @@ -3,9 +3,9 @@ import type { AvailableStepExecution, StepUser } from '../types/execution-context'; import type { CollectionSchema } from '../types/validated/collection'; import type { StepOutcome } from '../types/validated/step-outcome'; -import type { McpConfiguration, McpServers } from '@forestadmin/ai-proxy'; +import type { McpServers } from '@forestadmin/ai-proxy'; -export type { McpConfiguration, McpServers }; +export type { McpServers }; export interface MalformedRunInfo { runId: string; @@ -39,8 +39,6 @@ export interface WorkflowPort { stepOutcome: StepOutcome, ): Promise; getCollectionSchema(collectionName: string, runId: string): Promise; - // The orchestrator's GET /liana/mcp-server-configs-with-details returns a - // Record map keyed by server name — not an array. getMcpServerConfigs(): Promise; hasRunAccess(runId: string, user: StepUser): Promise; } diff --git a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts index 269c88ca81..04ffc40074 100644 --- a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts +++ b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts @@ -734,7 +734,7 @@ describe('ForestServerWorkflowPort', () => { describe('getMcpServerConfigs', () => { it('returns the Record map verbatim from the orchestrator', async () => { const configs = { - 'data-gouv': { url: 'https://mcp.example.com', type: 'http', headers: {} }, + 'mcp-server-1': { url: 'https://mcp.example.com', type: 'http', headers: {} }, }; mockQuery.mockResolvedValue(configs); diff --git a/packages/workflow-executor/test/runner.test.ts b/packages/workflow-executor/test/runner.test.ts index c4b63d4c69..de4dcfe41a 100644 --- a/packages/workflow-executor/test/runner.test.ts +++ b/packages/workflow-executor/test/runner.test.ts @@ -1163,7 +1163,7 @@ describe('MCP lazy loading (via once thunk)', () => { auth: { forestServerToken: 'test-forest-token' }, }); const realConfigs = { - 'data-gouv': { url: 'https://mcp.example.com', type: 'http' as const, headers: {} }, + 'mcp-server-1': { url: 'https://mcp.example.com', type: 'http' as const, headers: {} }, }; workflowPort.getMcpServerConfigs.mockResolvedValue(realConfigs); From 53618a706879b83ecfff526417d72680c951ebf1 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Wed, 20 May 2026 16:23:24 +0200 Subject: [PATCH 3/3] fix(workflow-executor): match MCP tools by config id (#1584) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workflow-executor): match MCP tools by config id The executor's filter compared `tool.sourceId` (server display name) against `step.mcpServerId` (DB id written by the frontend), so any workflow that specified an MCP server failed with NoMcpToolsError regardless of configuration. Threads the stable DB id from each ToolConfig entry through ai-proxy (McpClient, ForestIntegrationClient and the integration factories) onto RemoteTool.mcpServerId, and switches the executor to match by id. Enriches NoMcpToolsError's technical message with the requested id and the loaded id list so misconfigurations are diagnosable from the structured logs; the user-facing message stays generic per the dual-message convention. The new tool-side field is named `mcpServerId` (not `id`) to read honestly at the consumer site — `tool.mcpServerId === step.mcpServerId` expresses the FK relationship plainly. `McpServerConfig.id` and `ForestIntegrationConfig.id` keep `id` since those mirror the wire shape (which itself mirrors the `ai_mcp_configs` PK column). fixes PRD-362 --- .../ai-proxy/src/forest-integration-client.ts | 9 +- .../ai-proxy/src/integrations/kolar/tools.ts | 3 +- .../src/integrations/snowflake/tools.ts | 6 +- .../src/integrations/zendesk/tools.ts | 3 +- packages/ai-proxy/src/mcp-client.ts | 19 ++- .../ai-proxy/src/mcp-server-remote-tool.ts | 6 +- packages/ai-proxy/src/remote-tool.ts | 5 + packages/ai-proxy/src/server-remote-tool.ts | 6 +- .../test/forest-integration-client.test.ts | 96 ++++++++++- .../test/integrations/kolar/tools.test.ts | 19 +++ .../test/integrations/snowflake/tools.test.ts | 19 +++ .../test/integrations/zendesk/tools.test.ts | 19 +++ packages/ai-proxy/test/mcp-client.test.ts | 46 ++++++ .../test/tool-provider-factory.test.ts | 13 +- packages/workflow-executor/src/errors.ts | 9 +- .../src/executors/mcp-step-executor.ts | 19 ++- .../workflow-executor/test/errors.test.ts | 38 ++++- .../test/executors/mcp-step-executor.test.ts | 150 ++++++++++++++++-- 18 files changed, 440 insertions(+), 45 deletions(-) diff --git a/packages/ai-proxy/src/forest-integration-client.ts b/packages/ai-proxy/src/forest-integration-client.ts index 449b3d41e0..dff89c8e82 100644 --- a/packages/ai-proxy/src/forest-integration-client.ts +++ b/packages/ai-proxy/src/forest-integration-client.ts @@ -14,6 +14,7 @@ export type CustomConfig = ZendeskConfig | KolarConfig | SnowflakeConfig; export type ForestIntegrationName = 'Zendesk' | 'Kolar' | 'Snowflake'; export interface ForestIntegrationConfig { + id?: string; integrationName: ForestIntegrationName; config: CustomConfig; isForestConnector: true; @@ -39,16 +40,16 @@ export default class ForestIntegrationClient implements ToolProvider { async loadTools(): Promise { const tools: RemoteTool[] = []; - this.configs.forEach(({ integrationName, config }) => { + this.configs.forEach(({ id: mcpServerId, integrationName, config }) => { switch (integrationName) { case 'Zendesk': - tools.push(...getZendeskTools(config as ZendeskConfig)); + tools.push(...getZendeskTools(config as ZendeskConfig, mcpServerId)); break; case 'Kolar': - tools.push(...getKolarTools(config as KolarConfig)); + tools.push(...getKolarTools(config as KolarConfig, mcpServerId)); break; case 'Snowflake': - tools.push(...getSnowflakeTools(config as SnowflakeConfig)); + tools.push(...getSnowflakeTools(config as SnowflakeConfig, mcpServerId)); break; default: this.logger?.('Warn', `Unsupported integration: ${integrationName}`); diff --git a/packages/ai-proxy/src/integrations/kolar/tools.ts b/packages/ai-proxy/src/integrations/kolar/tools.ts index e3ae18701f..57f8bb730c 100644 --- a/packages/ai-proxy/src/integrations/kolar/tools.ts +++ b/packages/ai-proxy/src/integrations/kolar/tools.ts @@ -11,7 +11,7 @@ export interface KolarConfig { apiKey: string; } -export default function getKolarTools(config: KolarConfig): RemoteTool[] { +export default function getKolarTools(config: KolarConfig, mcpServerId?: string): RemoteTool[] { const { baseUrl, headers } = getKolarConfig(config); return [ @@ -23,6 +23,7 @@ export default function getKolarTools(config: KolarConfig): RemoteTool[] { tool => new ServerRemoteTool({ sourceId: 'kolar', + mcpServerId, tool, }), ); diff --git a/packages/ai-proxy/src/integrations/snowflake/tools.ts b/packages/ai-proxy/src/integrations/snowflake/tools.ts index 3b0bad7818..35cbc277ea 100644 --- a/packages/ai-proxy/src/integrations/snowflake/tools.ts +++ b/packages/ai-proxy/src/integrations/snowflake/tools.ts @@ -15,7 +15,10 @@ export interface SnowflakeConfig { defaultRole?: string; } -export default function getSnowflakeTools(config: SnowflakeConfig): RemoteTool[] { +export default function getSnowflakeTools( + config: SnowflakeConfig, + mcpServerId?: string, +): RemoteTool[] { const headers = getSnowflakeAuthHeaders(config); const baseUrl = getSnowflakeBaseUrl(config.accountIdentifier); @@ -27,6 +30,7 @@ export default function getSnowflakeTools(config: SnowflakeConfig): RemoteTool[] tool => new ServerRemoteTool({ sourceId: 'snowflake', + mcpServerId, tool, }), ); diff --git a/packages/ai-proxy/src/integrations/zendesk/tools.ts b/packages/ai-proxy/src/integrations/zendesk/tools.ts index 9886f86c49..d106a69b16 100644 --- a/packages/ai-proxy/src/integrations/zendesk/tools.ts +++ b/packages/ai-proxy/src/integrations/zendesk/tools.ts @@ -15,7 +15,7 @@ export interface ZendeskConfig { apiToken: string; } -export default function getZendeskTools(config: ZendeskConfig): RemoteTool[] { +export default function getZendeskTools(config: ZendeskConfig, mcpServerId?: string): RemoteTool[] { const { baseUrl, headers } = getZendeskConfig(config); return [ @@ -29,6 +29,7 @@ export default function getZendeskTools(config: ZendeskConfig): RemoteTool[] { tool => new ServerRemoteTool({ sourceId: 'zendesk', + mcpServerId, tool, }), ); diff --git a/packages/ai-proxy/src/mcp-client.ts b/packages/ai-proxy/src/mcp-client.ts index 71f2868fc6..bb834c576e 100644 --- a/packages/ai-proxy/src/mcp-client.ts +++ b/packages/ai-proxy/src/mcp-client.ts @@ -8,14 +8,17 @@ import McpServerRemoteTool from './mcp-server-remote-tool'; export type McpServers = MultiServerMCPClient['config']['mcpServers']; -export type McpServerConfig = MultiServerMCPClient['config']['mcpServers'][string]; +export type McpServerConfig = MultiServerMCPClient['config']['mcpServers'][string] & { + id?: string; +}; export type McpConfiguration = { - configs: McpServers; + configs: Record; } & Omit; export default class McpClient implements ToolProvider { private readonly mcpClients: Record = {}; + private readonly mcpServerIdsByName: Record = {}; private readonly logger?: Logger; constructor(config: McpConfiguration, logger?: Logger) { @@ -23,8 +26,11 @@ export default class McpClient implements ToolProvider { // split the config into several clients to be more resilient // if a mcp server is down, the others will still work Object.entries(config.configs).forEach(([name, serverConfig]) => { + const { id: mcpServerId, ...rest } = serverConfig as McpServerConfig & + Record; + this.mcpServerIdsByName[name] = mcpServerId; this.mcpClients[name] = new MultiServerMCPClient({ - mcpServers: { [name]: serverConfig }, + mcpServers: { [name]: rest as McpServerConfig }, ...config, }); }); @@ -39,7 +45,12 @@ export default class McpClient implements ToolProvider { try { const loadedTools = (await client.getTools()) ?? []; const extendedTools = loadedTools.map( - tool => new McpServerRemoteTool({ tool, sourceId: name }), + tool => + new McpServerRemoteTool({ + tool, + sourceId: name, + mcpServerId: this.mcpServerIdsByName[name], + }), ); tools.push(...extendedTools); } catch (error) { diff --git a/packages/ai-proxy/src/mcp-server-remote-tool.ts b/packages/ai-proxy/src/mcp-server-remote-tool.ts index 8bc4f4e7bf..85791d895e 100644 --- a/packages/ai-proxy/src/mcp-server-remote-tool.ts +++ b/packages/ai-proxy/src/mcp-server-remote-tool.ts @@ -3,7 +3,11 @@ import type { StructuredToolInterface } from '@langchain/core/tools'; import RemoteTool from './remote-tool'; export default class McpServerRemoteTool extends RemoteTool { - constructor(options: { tool: StructuredToolInterface; sourceId?: string }) { + constructor(options: { + tool: StructuredToolInterface; + sourceId?: string; + mcpServerId?: string; + }) { super({ ...options, sourceType: 'mcp-server' }); } } diff --git a/packages/ai-proxy/src/remote-tool.ts b/packages/ai-proxy/src/remote-tool.ts index 9c31f3f1d1..a59911664a 100644 --- a/packages/ai-proxy/src/remote-tool.ts +++ b/packages/ai-proxy/src/remote-tool.ts @@ -4,15 +4,20 @@ export default abstract class RemoteTool { base: StructuredToolInterface; sourceId: string; sourceType: string; + // Shared across McpServerRemoteTool and ServerRemoteTool because the orchestrator + // stores Forest connectors alongside user MCP servers in the same ai_mcp_configs table. + mcpServerId?: string; constructor(options: { tool: StructuredToolInterface; sourceId?: string; sourceType?: string; + mcpServerId?: string; }) { this.base = options.tool; this.sourceId = options.sourceId; this.sourceType = options.sourceType; + this.mcpServerId = options.mcpServerId; } get sanitizedName() { diff --git a/packages/ai-proxy/src/server-remote-tool.ts b/packages/ai-proxy/src/server-remote-tool.ts index 0ed5dcf487..2341411972 100644 --- a/packages/ai-proxy/src/server-remote-tool.ts +++ b/packages/ai-proxy/src/server-remote-tool.ts @@ -3,7 +3,11 @@ import type { StructuredToolInterface } from '@langchain/core/tools'; import RemoteTool from './remote-tool'; export default class ServerRemoteTool extends RemoteTool { - constructor(options: { tool: StructuredToolInterface; sourceId?: string }) { + constructor(options: { + tool: StructuredToolInterface; + sourceId?: string; + mcpServerId?: string; + }) { super({ ...options, sourceType: 'server' }); } } diff --git a/packages/ai-proxy/test/forest-integration-client.test.ts b/packages/ai-proxy/test/forest-integration-client.test.ts index 84427975cb..104fb6f80d 100644 --- a/packages/ai-proxy/test/forest-integration-client.test.ts +++ b/packages/ai-proxy/test/forest-integration-client.test.ts @@ -1,6 +1,9 @@ import ForestIntegrationClient from '../src/forest-integration-client'; +import getKolarTools from '../src/integrations/kolar/tools'; import { validateKolarConfig } from '../src/integrations/kolar/utils'; +import getSnowflakeTools from '../src/integrations/snowflake/tools'; import { validateSnowflakeConfig } from '../src/integrations/snowflake/utils'; +import getZendeskTools from '../src/integrations/zendesk/tools'; import { validateZendeskConfig } from '../src/integrations/zendesk/utils'; const mockZendeskTools = [{ name: 'zendesk_get_tickets' }, { name: 'zendesk_get_ticket' }]; @@ -37,6 +40,7 @@ describe('ForestIntegrationClient', () => { it('should load zendesk tools when integration is zendesk', async () => { const client = new ForestIntegrationClient([ { + id: '1', integrationName: 'Zendesk', config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, isForestConnector: true, @@ -52,7 +56,7 @@ describe('ForestIntegrationClient', () => { const logger = jest.fn(); const client = new ForestIntegrationClient( // @ts-expect-error Testing unsupported integration - [{ integrationName: 'unknown', config: {} as any, isForestConnector: true }], + [{ id: '1', integrationName: 'unknown', config: {} as any, isForestConnector: true }], logger, ); @@ -64,6 +68,7 @@ describe('ForestIntegrationClient', () => { it('should load kolar tools when integration is Kolar', async () => { const client = new ForestIntegrationClient([ { + id: '1', integrationName: 'Kolar', config: { apiKey: 'key' }, isForestConnector: true, @@ -78,6 +83,7 @@ describe('ForestIntegrationClient', () => { it('should load snowflake tools when integration is Snowflake', async () => { const client = new ForestIntegrationClient([ { + id: '1', integrationName: 'Snowflake', config: { accountIdentifier: 'a', @@ -101,11 +107,13 @@ describe('ForestIntegrationClient', () => { it('should load tools from multiple configs', async () => { const client = new ForestIntegrationClient([ { + id: '1', integrationName: 'Zendesk', config: { subdomain: 'a', email: 'a@b.com', apiToken: 'tok' }, isForestConnector: true, }, { + id: '2', integrationName: 'Zendesk', config: { subdomain: 'b', email: 'c@d.com', apiToken: 'tok2' }, isForestConnector: true, @@ -122,7 +130,7 @@ describe('ForestIntegrationClient', () => { it('should call validateZendeskConfig for Zendesk integration', async () => { const zendeskConfig = { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }; const client = new ForestIntegrationClient([ - { integrationName: 'Zendesk', config: zendeskConfig, isForestConnector: true }, + { id: '1', integrationName: 'Zendesk', config: zendeskConfig, isForestConnector: true }, ]); await client.checkConnection(); @@ -133,6 +141,7 @@ describe('ForestIntegrationClient', () => { it('should return true on success', async () => { const client = new ForestIntegrationClient([ { + id: '1', integrationName: 'Zendesk', config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, isForestConnector: true, @@ -147,7 +156,7 @@ describe('ForestIntegrationClient', () => { it('should call validateKolarConfig for Kolar integration', async () => { const kolarConfig = { apiKey: 'key' }; const client = new ForestIntegrationClient([ - { integrationName: 'Kolar', config: kolarConfig, isForestConnector: true }, + { id: '1', integrationName: 'Kolar', config: kolarConfig, isForestConnector: true }, ]); await client.checkConnection(); @@ -161,7 +170,7 @@ describe('ForestIntegrationClient', () => { programmaticAccessToken: 'tok', }; const client = new ForestIntegrationClient([ - { integrationName: 'Snowflake', config: snowflakeConfig, isForestConnector: true }, + { id: '1', integrationName: 'Snowflake', config: snowflakeConfig, isForestConnector: true }, ]); await client.checkConnection(); @@ -172,12 +181,10 @@ describe('ForestIntegrationClient', () => { it('should throw for unsupported integration', async () => { const client = new ForestIntegrationClient([ // @ts-expect-error Testing unsupported integration - { integrationName: 'Unknown', config: {}, isForestConnector: true }, + { id: '1', integrationName: 'Unknown', config: {}, isForestConnector: true }, ]); - await expect(client.checkConnection()).rejects.toThrow( - 'Unsupported integration: Unknown', - ); + await expect(client.checkConnection()).rejects.toThrow('Unsupported integration: Unknown'); }); }); @@ -188,4 +195,77 @@ describe('ForestIntegrationClient', () => { await expect(client.dispose()).resolves.toBeUndefined(); }); }); + + describe('ForestIntegrationConfig.id is threaded as mcpServerId into integration tool factories', () => { + it('passes the config id to getZendeskTools so produced tools can be matched by step.mcpServerId', async () => { + const client = new ForestIntegrationClient([ + { + id: 'forest-zendesk-42', + integrationName: 'Zendesk', + config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, + isForestConnector: true, + }, + ]); + + await client.loadTools(); + + expect(getZendeskTools).toHaveBeenCalledWith( + expect.objectContaining({ subdomain: 'test' }), + 'forest-zendesk-42', + ); + }); + + it('passes the config id to getKolarTools', async () => { + const client = new ForestIntegrationClient([ + { + id: 'forest-kolar-7', + integrationName: 'Kolar', + config: { apiKey: 'key' }, + isForestConnector: true, + }, + ]); + + await client.loadTools(); + + expect(getKolarTools).toHaveBeenCalledWith( + expect.objectContaining({ apiKey: 'key' }), + 'forest-kolar-7', + ); + }); + + it('passes the config id to getSnowflakeTools', async () => { + const client = new ForestIntegrationClient([ + { + id: 'forest-snowflake-99', + integrationName: 'Snowflake', + config: { accountIdentifier: 'a', programmaticAccessToken: 'tok' }, + isForestConnector: true, + }, + ]); + + await client.loadTools(); + + expect(getSnowflakeTools).toHaveBeenCalledWith( + expect.objectContaining({ accountIdentifier: 'a' }), + 'forest-snowflake-99', + ); + }); + + it('passes undefined to the factory when the config entry has no id', async () => { + const client = new ForestIntegrationClient([ + { + integrationName: 'Zendesk', + config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, + isForestConnector: true, + }, + ]); + + await client.loadTools(); + + expect(getZendeskTools).toHaveBeenCalledWith( + expect.objectContaining({ subdomain: 'test' }), + undefined, + ); + }); + }); }); diff --git a/packages/ai-proxy/test/integrations/kolar/tools.test.ts b/packages/ai-proxy/test/integrations/kolar/tools.test.ts index df670e3c72..c4428e1161 100644 --- a/packages/ai-proxy/test/integrations/kolar/tools.test.ts +++ b/packages/ai-proxy/test/integrations/kolar/tools.test.ts @@ -26,4 +26,23 @@ describe('getKolarTools', () => { 'kolar_get_screening_result', ]); }); + + describe('mcpServerId propagation', () => { + it('sets RemoteTool.mcpServerId on every produced tool when mcpServerId is provided', () => { + const tools = getKolarTools(config, 'forest-kolar-7'); + + expect(tools).not.toHaveLength(0); + tools.forEach(tool => { + expect(tool.mcpServerId).toBe('forest-kolar-7'); + }); + }); + + it('leaves RemoteTool.mcpServerId undefined when no mcpServerId is provided', () => { + const tools = getKolarTools(config); + + tools.forEach(tool => { + expect(tool.mcpServerId).toBeUndefined(); + }); + }); + }); }); diff --git a/packages/ai-proxy/test/integrations/snowflake/tools.test.ts b/packages/ai-proxy/test/integrations/snowflake/tools.test.ts index 5e8336c23a..be75a9cb8c 100644 --- a/packages/ai-proxy/test/integrations/snowflake/tools.test.ts +++ b/packages/ai-proxy/test/integrations/snowflake/tools.test.ts @@ -28,4 +28,23 @@ describe('getSnowflakeTools', () => { 'snowflake_execute_query', ]); }); + + describe('mcpServerId propagation', () => { + it('sets RemoteTool.mcpServerId on every produced tool when mcpServerId is provided', () => { + const tools = getSnowflakeTools(config, 'forest-snowflake-99'); + + expect(tools).not.toHaveLength(0); + tools.forEach(tool => { + expect(tool.mcpServerId).toBe('forest-snowflake-99'); + }); + }); + + it('leaves RemoteTool.mcpServerId undefined when no mcpServerId is provided', () => { + const tools = getSnowflakeTools(config); + + tools.forEach(tool => { + expect(tool.mcpServerId).toBeUndefined(); + }); + }); + }); }); diff --git a/packages/ai-proxy/test/integrations/zendesk/tools.test.ts b/packages/ai-proxy/test/integrations/zendesk/tools.test.ts index b446bcb7ef..24bd2e0fc9 100644 --- a/packages/ai-proxy/test/integrations/zendesk/tools.test.ts +++ b/packages/ai-proxy/test/integrations/zendesk/tools.test.ts @@ -28,4 +28,23 @@ describe('getZendeskTools', () => { 'zendesk_update_ticket', ]); }); + + describe('mcpServerId propagation', () => { + it('sets RemoteTool.mcpServerId on every produced tool when mcpServerId is provided', () => { + const tools = getZendeskTools(config, 'forest-zendesk-42'); + + expect(tools).not.toHaveLength(0); + tools.forEach(tool => { + expect(tool.mcpServerId).toBe('forest-zendesk-42'); + }); + }); + + it('leaves RemoteTool.mcpServerId undefined when no mcpServerId is provided', () => { + const tools = getZendeskTools(config); + + tools.forEach(tool => { + expect(tool.mcpServerId).toBeUndefined(); + }); + }); + }); }); diff --git a/packages/ai-proxy/test/mcp-client.test.ts b/packages/ai-proxy/test/mcp-client.test.ts index cab68489be..ad128c2fbd 100644 --- a/packages/ai-proxy/test/mcp-client.test.ts +++ b/packages/ai-proxy/test/mcp-client.test.ts @@ -80,6 +80,51 @@ describe('McpClient', () => { }); }); + describe('mcpServerId threaded from config entry into McpServerRemoteTool', () => { + it('sets RemoteTool.mcpServerId from the config entry id alongside sourceId from the map key', async () => { + const tool1 = tool(() => {}, { + name: 'tool1', + description: 'description1', + schema: undefined, + responseFormat: 'content', + }); + const configWithId = { + configs: { + slack: { + transport: 'stdio' as const, + command: 'npx', + args: ['-y', '@modelcontextprotocol/server-slack'], + env: {}, + id: 'config-id-42', + }, + }, + } as unknown as McpConfiguration; + const mcpClient = new McpClient(configWithId); + getToolsMock.mockResolvedValue([tool1]); + + const tools = await mcpClient.loadTools(); + + expect(tools).toHaveLength(1); + expect(tools[0].sourceId).toBe('slack'); + expect(tools[0].mcpServerId).toBe('config-id-42'); + }); + + it('leaves RemoteTool.mcpServerId undefined when the config entry has no id (legacy / unenriched payload)', async () => { + const tool1 = tool(() => {}, { + name: 'tool1', + description: 'description1', + schema: undefined, + responseFormat: 'content', + }); + const mcpClient = new McpClient(aConfig); + getToolsMock.mockResolvedValue([tool1]); + + const tools = await mcpClient.loadTools(); + + expect(tools[0].mcpServerId).toBeUndefined(); + }); + }); + describe('when there is an error while loading the tools', () => { it('should not throw an error and try to load every mcp tools', async () => { const mcpClient = new McpClient({ @@ -396,6 +441,7 @@ describe('McpClient', () => { const configs = { server1: { type: 'http' as const, url: 'https://server1.com' }, zendesk: { + id: '1', isForestConnector: true as const, integrationName: 'Zendesk' as const, config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, diff --git a/packages/ai-proxy/test/tool-provider-factory.test.ts b/packages/ai-proxy/test/tool-provider-factory.test.ts index a3fbe44d7e..e56b436577 100644 --- a/packages/ai-proxy/test/tool-provider-factory.test.ts +++ b/packages/ai-proxy/test/tool-provider-factory.test.ts @@ -35,14 +35,12 @@ describe('createToolProviders', () => { const providers = createToolProviders(configs as any); expect(providers).toHaveLength(1); - expect(McpClient).toHaveBeenCalledWith( - { configs: { slack: configs.slack } }, - undefined, - ); + expect(McpClient).toHaveBeenCalledWith({ configs: { slack: configs.slack } }, undefined); }); it('should create ForestIntegrationClient for ForestIntegration configs', () => { const zendeskConfig = { + id: '1', isForestConnector: true as const, integrationName: 'Zendesk' as const, config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, @@ -58,6 +56,7 @@ describe('createToolProviders', () => { const configs = { slack: { command: 'npx', args: [] }, zendesk: { + id: '1', isForestConnector: true as const, integrationName: 'Zendesk' as const, config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, @@ -67,10 +66,7 @@ describe('createToolProviders', () => { const providers = createToolProviders(configs as any); expect(providers).toHaveLength(2); - expect(McpClient).toHaveBeenCalledWith( - { configs: { slack: configs.slack } }, - undefined, - ); + expect(McpClient).toHaveBeenCalledWith({ configs: { slack: configs.slack } }, undefined); expect(ForestIntegrationClient).toHaveBeenCalledWith([configs.zendesk], undefined); }); @@ -85,6 +81,7 @@ describe('createToolProviders', () => { const configs = { slack: { command: 'npx', args: [] }, zendesk: { + id: '1', isForestConnector: true as const, integrationName: 'Zendesk' as const, config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 6c1090d5d8..4aeb7f4618 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -203,8 +203,13 @@ export class StepTimeoutError extends WorkflowExecutorError { } export class NoMcpToolsError extends WorkflowExecutorError { - constructor() { - super('No MCP tools available', 'No tools are available to execute this step.'); + constructor(requestedMcpServerId?: string, loadedMcpServerIds?: readonly string[]) { + const technical = requestedMcpServerId + ? `No MCP tools available for mcpServerId="${requestedMcpServerId}". Loaded MCP server ids: [${( + loadedMcpServerIds ?? [] + ).join(', ')}]` + : 'No MCP tools available'; + super(technical, 'No tools are available to execute this step.'); } } diff --git a/packages/workflow-executor/src/executors/mcp-step-executor.ts b/packages/workflow-executor/src/executors/mcp-step-executor.ts index fca94387a5..2723976888 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -224,13 +224,26 @@ export default class McpStepExecutor extends BaseStepExecutor ); } - /** Returns tools filtered by mcpServerId (if specified). Throws NoMcpToolsError if empty. */ private getFilteredTools(): RemoteTool[] { const { mcpServerId } = this.context.stepDefinition; const tools = mcpServerId - ? this.remoteTools.filter(t => t.sourceId === mcpServerId) + ? this.remoteTools.filter(t => t.mcpServerId === mcpServerId) : [...this.remoteTools]; - if (tools.length === 0) throw new NoMcpToolsError(); + + if (tools.length === 0) { + const loadedMcpServerIds = this.remoteTools + .map(t => t.mcpServerId) + .filter((value): value is string => !!value); + const error = new NoMcpToolsError(mcpServerId, loadedMcpServerIds); + this.context.logger.error(error.message, { + runId: this.context.runId, + stepId: this.context.stepId, + stepIndex: this.context.stepIndex, + requestedMcpServerId: mcpServerId, + loadedMcpServerIds, + }); + throw error; + } return tools; } diff --git a/packages/workflow-executor/test/errors.test.ts b/packages/workflow-executor/test/errors.test.ts index a936567012..369d99f46a 100644 --- a/packages/workflow-executor/test/errors.test.ts +++ b/packages/workflow-executor/test/errors.test.ts @@ -1,4 +1,4 @@ -import { extractErrorMessage } from '../src/errors'; +import { NoMcpToolsError, extractErrorMessage } from '../src/errors'; describe('extractErrorMessage', () => { it('returns err.message when non-empty', () => { @@ -58,3 +58,39 @@ describe('extractErrorMessage', () => { expect(extractErrorMessage(err)).toBe('from cause'); }); }); + +describe('NoMcpToolsError', () => { + it('produces a fully generic technical message when no mcpServerId was requested (no filter case)', () => { + const err = new NoMcpToolsError(); + + expect(err.message).toBe('No MCP tools available'); + expect(err.userMessage).toBe('No tools are available to execute this step.'); + }); + + it('includes the requested mcpServerId in the technical message when a filter was active', () => { + const err = new NoMcpToolsError('id-missing', ['id-A', 'id-B']); + + expect(err.message).toMatch(/id-missing/); + }); + + it('lists the loaded mcpServerIds in the technical message so misconfigurations are diagnosable', () => { + const err = new NoMcpToolsError('id-missing', ['id-A', 'id-B']); + + expect(err.message).toMatch(/id-A/); + expect(err.message).toMatch(/id-B/); + }); + + it('handles an empty loaded-id list without producing a malformed message', () => { + const err = new NoMcpToolsError('id-missing', []); + + expect(err.message).toMatch(/id-missing/); + expect(err.message).not.toMatch(/undefined|null|\[object/i); + }); + + it('keeps the user-facing message generic — no internal ids must leak', () => { + const err = new NoMcpToolsError('id-missing', ['id-A', 'id-B']); + + expect(err.userMessage).toBe('No tools are available to execute this step.'); + expect(err.userMessage).not.toMatch(/id-missing|id-A|id-B/); + }); +}); diff --git a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts index e58aa8346e..96c3d393a9 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -16,7 +16,12 @@ import { StepType } from '../../src/types/validated/step-definition'; // --------------------------------------------------------------------------- class MockRemoteTool extends RemoteTool { - constructor(options: { name: string; sourceId?: string; invoke?: jest.Mock }) { + constructor(options: { + name: string; + sourceId?: string; + mcpServerId?: string; + invoke?: jest.Mock; + }) { const invokeFn = options.invoke ?? jest.fn().mockResolvedValue('tool-result'); super({ tool: { @@ -27,6 +32,7 @@ class MockRemoteTool extends RemoteTool { } as unknown as RemoteTool['base'], sourceId: options.sourceId ?? 'mcp-server-1', sourceType: 'mcp', + mcpServerId: options.mcpServerId, }); } } @@ -439,14 +445,23 @@ describe('McpStepExecutor', () => { }); }); - describe('mcpServerId filter', () => { - it('passes only tools from the specified server to the AI', async () => { - const toolA = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A' }); - const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'server-B' }); + describe('mcpServerId filter (matches by tool.mcpServerId, not tool.sourceId)', () => { + it('passes only tools whose mcpServerId matches step.mcpServerId to the AI', async () => { + const toolA = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'zendesk', + mcpServerId: 'id-A', + }); + const toolB = new MockRemoteTool({ + name: 'tool_b', + sourceId: 'zendesk', + mcpServerId: 'id-B', + }); const invokeFn = jest.fn().mockResolvedValue('ok'); const toolB2 = new MockRemoteTool({ name: 'tool_b2', - sourceId: 'server-B', + sourceId: 'zendesk', + mcpServerId: 'id-B', invoke: invokeFn, }); @@ -455,19 +470,86 @@ describe('McpStepExecutor', () => { const context = makeContext({ model, runStore, - stepDefinition: makeStep({ mcpServerId: 'server-B', automaticExecution: true }), + stepDefinition: makeStep({ mcpServerId: 'id-B', automaticExecution: true }), }); const executor = new McpStepExecutor(context, [toolA, toolB, toolB2]); await executor.execute(); - // bindTools should only receive server-B tools const boundTools = bindTools.mock.calls[0][0] as Array<{ name: string }>; const boundNames = boundTools.map(t => t.name); expect(boundNames).not.toContain('tool_a'); expect(boundNames).toContain('tool_b'); expect(boundNames).toContain('tool_b2'); }); + + it('does not match by sourceId — server-name collisions must not leak tools across configs', async () => { + const tool = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'server-B', + mcpServerId: 'id-99', + }); + const context = makeContext({ + stepDefinition: makeStep({ mcpServerId: 'server-B' }), + }); + const executor = new McpStepExecutor(context, [tool]); + + const result = await executor.execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome.error).toBe('No tools are available to execute this step.'); + }); + + it('returns all tools (no filter) when step.mcpServerId is absent', async () => { + const toolA = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'server-A', + mcpServerId: 'id-A', + }); + const toolB = new MockRemoteTool({ + name: 'tool_b', + sourceId: 'server-B', + mcpServerId: 'id-B', + }); + const { model, bindTools } = makeMockModel('tool_a', {}); + const context = makeContext({ + model, + stepDefinition: makeStep({ automaticExecution: true }), + }); + const executor = new McpStepExecutor(context, [toolA, toolB]); + + await executor.execute(); + + const boundTools = bindTools.mock.calls[0][0] as Array<{ name: string }>; + const boundNames = boundTools.map(t => t.name); + expect(boundNames).toEqual(expect.arrayContaining(['tool_a', 'tool_b'])); + }); + + it('resolves a Forest-connector-backed tool when its mcpServerId is threaded through', async () => { + const invokeFn = jest.fn().mockResolvedValue('done'); + const forestTool = new MockRemoteTool({ + name: 'zendesk_get_tickets', + sourceId: 'zendesk', + mcpServerId: 'forest-connector-42', + invoke: invokeFn, + }); + const { model, bindTools } = makeMockModel('zendesk_get_tickets', {}); + const context = makeContext({ + model, + stepDefinition: makeStep({ + mcpServerId: 'forest-connector-42', + automaticExecution: true, + }), + }); + const executor = new McpStepExecutor(context, [forestTool]); + + const result = await executor.execute(); + + expect(result.stepOutcome.status).toBe('success'); + const boundTools = bindTools.mock.calls[0][0] as Array<{ name: string }>; + expect(boundTools.map(t => t.name)).toEqual(['zendesk_get_tickets']); + expect(invokeFn).toHaveBeenCalled(); + }); }); describe('NoMcpToolsError', () => { @@ -482,9 +564,13 @@ describe('McpStepExecutor', () => { }); it('returns error when mcpServerId filter yields no tools', async () => { - const tool = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A' }); + const tool = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'server-A', + mcpServerId: 'id-A', + }); const context = makeContext({ - stepDefinition: makeStep({ mcpServerId: 'server-B' }), + stepDefinition: makeStep({ mcpServerId: 'id-B' }), }); const executor = new McpStepExecutor(context, [tool]); @@ -493,6 +579,50 @@ describe('McpStepExecutor', () => { expect(result.stepOutcome.status).toBe('error'); expect(result.stepOutcome.error).toBe('No tools are available to execute this step.'); }); + + it('keeps the user-facing error message generic regardless of the misconfigured mcpServerId', async () => { + const tool = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'server-A', + mcpServerId: 'id-A', + }); + const context = makeContext({ stepDefinition: makeStep({ mcpServerId: 'id-B' }) }); + const executor = new McpStepExecutor(context, [tool]); + + const result = await executor.execute(); + + expect(result.stepOutcome.error).toBe('No tools are available to execute this step.'); + expect(result.stepOutcome.error).not.toMatch(/id-B/); + }); + + it('logs the technical message with the requested mcpServerId and loaded mcpServerIds when filter misses', async () => { + const logger = { info: jest.fn(), error: jest.fn() }; + const toolA = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'server-A', + mcpServerId: 'id-A', + }); + const toolB = new MockRemoteTool({ + name: 'tool_b', + sourceId: 'server-B', + mcpServerId: 'id-B', + }); + const context = makeContext({ + logger, + stepDefinition: makeStep({ mcpServerId: 'id-missing' }), + }); + const executor = new McpStepExecutor(context, [toolA, toolB]); + + await executor.execute(); + + expect(logger.error).toHaveBeenCalledWith( + expect.stringMatching(/id-missing/), + expect.objectContaining({ + requestedMcpServerId: 'id-missing', + loadedMcpServerIds: expect.arrayContaining(['id-A', 'id-B']), + }), + ); + }); }); describe('McpToolNotFoundError', () => {