From ecbeed024fb86ede36907808562a21bd67e05110 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 19 May 2026 16:06:34 +0200 Subject: [PATCH 1/6] 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.id, and switches the executor to match by id. Enriches NoMcpToolsError's technical message with the requested id and the loaded config id list so misconfigurations are diagnosable from the activity log; the user-facing message stays generic per the dual-message convention. --- .../ai-proxy/src/forest-integration-client.ts | 9 +- .../ai-proxy/src/integrations/kolar/tools.ts | 3 +- .../src/integrations/snowflake/tools.ts | 3 +- .../src/integrations/zendesk/tools.ts | 3 +- packages/ai-proxy/src/mcp-client.ts | 17 +++- .../ai-proxy/src/mcp-server-remote-tool.ts | 6 +- packages/ai-proxy/src/remote-tool.ts | 6 ++ packages/ai-proxy/src/server-remote-tool.ts | 6 +- .../test/forest-integration-client.test.ts | 81 +++++++++++++-- .../test/integrations/kolar/tools.test.ts | 24 +++++ .../test/integrations/snowflake/tools.test.ts | 24 +++++ .../test/integrations/zendesk/tools.test.ts | 24 +++++ packages/ai-proxy/test/mcp-client.test.ts | 50 ++++++++++ .../test/tool-provider-factory.test.ts | 13 +-- packages/workflow-executor/src/errors.ts | 9 +- .../src/executors/mcp-step-executor.ts | 12 ++- .../workflow-executor/test/errors.test.ts | 44 ++++++++- .../test/executors/mcp-step-executor.test.ts | 99 ++++++++++++++++--- 18 files changed, 387 insertions(+), 46 deletions(-) diff --git a/packages/ai-proxy/src/forest-integration-client.ts b/packages/ai-proxy/src/forest-integration-client.ts index 449b3d41e0..11ffec59e4 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, integrationName, config }) => { switch (integrationName) { case 'Zendesk': - tools.push(...getZendeskTools(config as ZendeskConfig)); + tools.push(...getZendeskTools(config as ZendeskConfig, id)); break; case 'Kolar': - tools.push(...getKolarTools(config as KolarConfig)); + tools.push(...getKolarTools(config as KolarConfig, id)); break; case 'Snowflake': - tools.push(...getSnowflakeTools(config as SnowflakeConfig)); + tools.push(...getSnowflakeTools(config as SnowflakeConfig, id)); 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..cc8f7c708d 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, id?: string): RemoteTool[] { const { baseUrl, headers } = getKolarConfig(config); return [ @@ -23,6 +23,7 @@ export default function getKolarTools(config: KolarConfig): RemoteTool[] { tool => new ServerRemoteTool({ sourceId: 'kolar', + id, tool, }), ); diff --git a/packages/ai-proxy/src/integrations/snowflake/tools.ts b/packages/ai-proxy/src/integrations/snowflake/tools.ts index 3b0bad7818..06d9621700 100644 --- a/packages/ai-proxy/src/integrations/snowflake/tools.ts +++ b/packages/ai-proxy/src/integrations/snowflake/tools.ts @@ -15,7 +15,7 @@ export interface SnowflakeConfig { defaultRole?: string; } -export default function getSnowflakeTools(config: SnowflakeConfig): RemoteTool[] { +export default function getSnowflakeTools(config: SnowflakeConfig, id?: string): RemoteTool[] { const headers = getSnowflakeAuthHeaders(config); const baseUrl = getSnowflakeBaseUrl(config.accountIdentifier); @@ -27,6 +27,7 @@ export default function getSnowflakeTools(config: SnowflakeConfig): RemoteTool[] tool => new ServerRemoteTool({ sourceId: 'snowflake', + id, tool, }), ); diff --git a/packages/ai-proxy/src/integrations/zendesk/tools.ts b/packages/ai-proxy/src/integrations/zendesk/tools.ts index 9886f86c49..80b3b5bdd1 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, id?: string): RemoteTool[] { const { baseUrl, headers } = getZendeskConfig(config); return [ @@ -29,6 +29,7 @@ export default function getZendeskTools(config: ZendeskConfig): RemoteTool[] { tool => new ServerRemoteTool({ sourceId: 'zendesk', + id, tool, }), ); diff --git a/packages/ai-proxy/src/mcp-client.ts b/packages/ai-proxy/src/mcp-client.ts index 71f2868fc6..76e82ec95a 100644 --- a/packages/ai-proxy/src/mcp-client.ts +++ b/packages/ai-proxy/src/mcp-client.ts @@ -8,14 +8,20 @@ import McpServerRemoteTool from './mcp-server-remote-tool'; export type McpServers = MultiServerMCPClient['config']['mcpServers']; -export type McpServerConfig = MultiServerMCPClient['config']['mcpServers'][string]; +// Base orchestrator entry shape from `@langchain/mcp-adapters`, widened with the optional +// stable DB `id` exposed by Forest Admin's orchestrator. The id flows down to RemoteTool +// so the workflow executor can match a step's `mcpServerId` against the config record. +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 idsByServerName: Record = {}; private readonly logger?: Logger; constructor(config: McpConfiguration, logger?: Logger) { @@ -23,8 +29,10 @@ 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, ...rest } = serverConfig as McpServerConfig & Record; + this.idsByServerName[name] = id; this.mcpClients[name] = new MultiServerMCPClient({ - mcpServers: { [name]: serverConfig }, + mcpServers: { [name]: rest as McpServerConfig }, ...config, }); }); @@ -39,7 +47,8 @@ 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, id: this.idsByServerName[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..70113a2eff 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; + id?: 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..f7051c0cf8 100644 --- a/packages/ai-proxy/src/remote-tool.ts +++ b/packages/ai-proxy/src/remote-tool.ts @@ -4,15 +4,21 @@ export default abstract class RemoteTool { base: StructuredToolInterface; sourceId: string; sourceType: string; + // Stable DB id of the originating config entry. Distinct from `sourceId`, which is the + // human-readable server/integration name. Use `id` for cross-system references (e.g. + // matching a workflow step's mcpServerId to its config); use `sourceId` for display. + id?: string; constructor(options: { tool: StructuredToolInterface; sourceId?: string; sourceType?: string; + id?: string; }) { this.base = options.tool; this.sourceId = options.sourceId; this.sourceType = options.sourceType; + this.id = options.id; } get sanitizedName() { diff --git a/packages/ai-proxy/src/server-remote-tool.ts b/packages/ai-proxy/src/server-remote-tool.ts index 0ed5dcf487..6e7ae44ff1 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; + id?: 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..385ed03571 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,62 @@ describe('ForestIntegrationClient', () => { await expect(client.dispose()).resolves.toBeUndefined(); }); }); + + describe('id threaded from ForestIntegrationConfig 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, + } as any, + ]); + + await client.loadTools(); + + // The exact signature shape (positional vs. options bag) is an implementation + // detail of the production change; the test pins down that id reaches the factory. + 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, + } as any, + ]); + + 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, + } as any, + ]); + + await client.loadTools(); + + expect(getSnowflakeTools).toHaveBeenCalledWith( + expect.objectContaining({ accountIdentifier: 'a' }), + 'forest-snowflake-99', + ); + }); + }); }); diff --git a/packages/ai-proxy/test/integrations/kolar/tools.test.ts b/packages/ai-proxy/test/integrations/kolar/tools.test.ts index df670e3c72..e0bbb9bdb1 100644 --- a/packages/ai-proxy/test/integrations/kolar/tools.test.ts +++ b/packages/ai-proxy/test/integrations/kolar/tools.test.ts @@ -26,4 +26,28 @@ describe('getKolarTools', () => { 'kolar_get_screening_result', ]); }); + + describe('id propagation', () => { + it('sets RemoteTool.id on every produced tool when id is provided', () => { + const tools = ( + getKolarTools as unknown as ( + cfg: typeof config, + id?: string, + ) => ReturnType + )(config, 'forest-kolar-7'); + + expect(tools).not.toHaveLength(0); + tools.forEach(tool => { + expect((tool as ServerRemoteTool & { id?: string }).id).toBe('forest-kolar-7'); + }); + }); + + it('leaves RemoteTool.id undefined when no id is provided (backwards-compatible call site)', () => { + const tools = getKolarTools(config); + + tools.forEach(tool => { + expect((tool as ServerRemoteTool & { id?: string }).id).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..70066c9bdc 100644 --- a/packages/ai-proxy/test/integrations/snowflake/tools.test.ts +++ b/packages/ai-proxy/test/integrations/snowflake/tools.test.ts @@ -28,4 +28,28 @@ describe('getSnowflakeTools', () => { 'snowflake_execute_query', ]); }); + + describe('id propagation', () => { + it('sets RemoteTool.id on every produced tool when id is provided', () => { + const tools = ( + getSnowflakeTools as unknown as ( + cfg: SnowflakeConfig, + id?: string, + ) => ReturnType + )(config, 'forest-snowflake-99'); + + expect(tools).not.toHaveLength(0); + tools.forEach(tool => { + expect((tool as ServerRemoteTool & { id?: string }).id).toBe('forest-snowflake-99'); + }); + }); + + it('leaves RemoteTool.id undefined when no id is provided (backwards-compatible call site)', () => { + const tools = getSnowflakeTools(config); + + tools.forEach(tool => { + expect((tool as ServerRemoteTool & { id?: string }).id).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..bf75cf1ce7 100644 --- a/packages/ai-proxy/test/integrations/zendesk/tools.test.ts +++ b/packages/ai-proxy/test/integrations/zendesk/tools.test.ts @@ -28,4 +28,28 @@ describe('getZendeskTools', () => { 'zendesk_update_ticket', ]); }); + + describe('id propagation', () => { + it('sets RemoteTool.id on every produced tool when id is provided', () => { + const tools = ( + getZendeskTools as unknown as ( + cfg: typeof config, + id?: string, + ) => ReturnType + )(config, 'forest-zendesk-42'); + + expect(tools).not.toHaveLength(0); + tools.forEach(tool => { + expect((tool as ServerRemoteTool & { id?: string }).id).toBe('forest-zendesk-42'); + }); + }); + + it('leaves RemoteTool.id undefined when no id is provided (backwards-compatible call site)', () => { + const tools = getZendeskTools(config); + + tools.forEach(tool => { + expect((tool as ServerRemoteTool & { id?: string }).id).toBeUndefined(); + }); + }); + }); }); diff --git a/packages/ai-proxy/test/mcp-client.test.ts b/packages/ai-proxy/test/mcp-client.test.ts index cab68489be..9e8d525ef4 100644 --- a/packages/ai-proxy/test/mcp-client.test.ts +++ b/packages/ai-proxy/test/mcp-client.test.ts @@ -1,4 +1,5 @@ import type { McpConfiguration } from '../src'; +import type RemoteTool from '../src/remote-tool'; import { tool } from '@langchain/core/tools'; @@ -80,6 +81,54 @@ describe('McpClient', () => { }); }); + describe('id threaded from config entry into McpServerRemoteTool', () => { + it('sets RemoteTool.id from the config entry id alongside sourceId from the map key', async () => { + const tool1 = tool(() => {}, { + name: 'tool1', + description: 'description1', + schema: undefined, + responseFormat: 'content', + }); + // Each config entry carries the stable DB id surfaced by the orchestrator. + // The McpClient must thread that id through to RemoteTool.id so the executor + // can match workflow steps against it. + 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] as RemoteTool & { id?: string }).id).toBe('config-id-42'); + }); + + it('leaves RemoteTool.id 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] as RemoteTool & { id?: string }).id).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 +445,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..114ecbd927 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(requestedId?: string, loadedIds?: readonly string[]) { + const technical = requestedId + ? `No MCP tools available for mcpServerId="${requestedId}". Loaded MCP config ids: [${( + loadedIds ?? [] + ).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..45ad131724 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -224,13 +224,19 @@ export default class McpStepExecutor extends BaseStepExecutor ); } - /** Returns tools filtered by mcpServerId (if specified). Throws NoMcpToolsError if empty. */ + // Match the workflow step's mcpServerId against the stable DB id carried on each tool — + // not the display-oriented sourceId — so server-name collisions cannot leak tools across + // configs and the same id selects both regular MCP entries and Forest-connector entries. private getFilteredTools(): RemoteTool[] { const { mcpServerId } = this.context.stepDefinition; const tools = mcpServerId - ? this.remoteTools.filter(t => t.sourceId === mcpServerId) + ? this.remoteTools.filter(t => t.id === mcpServerId) : [...this.remoteTools]; - if (tools.length === 0) throw new NoMcpToolsError(); + + if (tools.length === 0) { + const loadedIds = this.remoteTools.map(t => t.id).filter((value): value is string => !!value); + throw new NoMcpToolsError(mcpServerId, loadedIds); + } return tools; } diff --git a/packages/workflow-executor/test/errors.test.ts b/packages/workflow-executor/test/errors.test.ts index a936567012..52cdaacc3c 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,45 @@ describe('extractErrorMessage', () => { expect(extractErrorMessage(err)).toBe('from cause'); }); }); + +describe('NoMcpToolsError', () => { + const Ctor = NoMcpToolsError as unknown as new ( + requestedId?: string, + loadedIds?: readonly string[], + ) => NoMcpToolsError; + + it('produces a fully generic technical message when no id was requested (no filter case)', () => { + const err = new Ctor(); + + 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 Ctor('id-missing', ['id-A', 'id-B']); + + expect(err.message).toMatch(/id-missing/); + }); + + it('lists the loaded MCP config ids in the technical message so misconfigurations are diagnosable', () => { + const err = new Ctor('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 Ctor('id-missing', []); + + expect(err.message).toMatch(/id-missing/); + // Should not produce undefined/null/[object Object] artefacts in the message. + expect(err.message).not.toMatch(/undefined|null|\[object/i); + }); + + it('keeps the user-facing message generic — no internal ids must leak', () => { + const err = new Ctor('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..e99eb131c2 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,7 @@ 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; id?: string; invoke?: jest.Mock }) { const invokeFn = options.invoke ?? jest.fn().mockResolvedValue('tool-result'); super({ tool: { @@ -27,7 +27,8 @@ class MockRemoteTool extends RemoteTool { } as unknown as RemoteTool['base'], sourceId: options.sourceId ?? 'mcp-server-1', sourceType: 'mcp', - }); + id: options.id, + } as ConstructorParameters[0] & { id?: string }); } } @@ -439,14 +440,17 @@ 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.id, not tool.sourceId)', () => { + it('passes only tools whose id matches step.mcpServerId to the AI', async () => { + // Both tools share sourceId='zendesk' (e.g. same integration server name) but carry + // distinct stable DB ids — proves the filter goes through id, not sourceId. + const toolA = new MockRemoteTool({ name: 'tool_a', sourceId: 'zendesk', id: 'id-A' }); + const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'zendesk', id: 'id-B' }); const invokeFn = jest.fn().mockResolvedValue('ok'); const toolB2 = new MockRemoteTool({ name: 'tool_b2', - sourceId: 'server-B', + sourceId: 'zendesk', + id: 'id-B', invoke: invokeFn, }); @@ -455,19 +459,78 @@ 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 () => { + // mcpServerId='server-B' should NOT match a tool that merely has sourceId='server-B' but + // a different DB id; only tool.id is authoritative. + const tool = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-B', id: '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', id: 'id-A' }); + const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'server-B', id: '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 id is threaded through', async () => { + // Simulates a Forest connector entry (e.g. Zendesk) where ForestIntegrationConfig.id + // has been propagated all the way to RemoteTool.id by the ai-proxy construction site. + const invokeFn = jest.fn().mockResolvedValue('done'); + const forestTool = new MockRemoteTool({ + name: 'zendesk_get_tickets', + sourceId: 'zendesk', + id: '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 +545,9 @@ 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', id: 'id-A' }); const context = makeContext({ - stepDefinition: makeStep({ mcpServerId: 'server-B' }), + stepDefinition: makeStep({ mcpServerId: 'id-B' }), }); const executor = new McpStepExecutor(context, [tool]); @@ -493,6 +556,20 @@ 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 id', async () => { + // Technical-message enrichment is covered in errors.test.ts by exercising the + // NoMcpToolsError constructor directly; here we lock down the dual-message + // contract: no internal id leaks into the user-facing message. + const tool = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A', id: '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/); + }); }); describe('McpToolNotFoundError', () => { From acffd6b6db7e000016c5856a0f2a4f6a197b0810 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 19 May 2026 17:06:42 +0200 Subject: [PATCH 2/6] fix(workflow-executor): log NoMcpToolsError diagnostic at throw site Surface the requested mcpServerId and loaded-id list in structured logs when the MCP step filter misses. The dual-message convention keeps the activity log generic for end users, so without this the enriched technical message would only live on the Error object and never become observable to engineers. --- .../src/executors/mcp-step-executor.ts | 10 +++++++- .../test/executors/mcp-step-executor.test.ts | 24 ++++++++++++++++--- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/packages/workflow-executor/src/executors/mcp-step-executor.ts b/packages/workflow-executor/src/executors/mcp-step-executor.ts index 45ad131724..810d301bf8 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -235,7 +235,15 @@ export default class McpStepExecutor extends BaseStepExecutor if (tools.length === 0) { const loadedIds = this.remoteTools.map(t => t.id).filter((value): value is string => !!value); - throw new NoMcpToolsError(mcpServerId, loadedIds); + const error = new NoMcpToolsError(mcpServerId, loadedIds); + this.context.logger.error(error.message, { + runId: this.context.runId, + stepId: this.context.stepId, + stepIndex: this.context.stepIndex, + requestedMcpServerId: mcpServerId, + loadedIds, + }); + throw error; } return tools; 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 e99eb131c2..4f43d76109 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -558,9 +558,6 @@ describe('McpStepExecutor', () => { }); it('keeps the user-facing error message generic regardless of the misconfigured id', async () => { - // Technical-message enrichment is covered in errors.test.ts by exercising the - // NoMcpToolsError constructor directly; here we lock down the dual-message - // contract: no internal id leaks into the user-facing message. const tool = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A', id: 'id-A' }); const context = makeContext({ stepDefinition: makeStep({ mcpServerId: 'id-B' }) }); const executor = new McpStepExecutor(context, [tool]); @@ -570,6 +567,27 @@ describe('McpStepExecutor', () => { 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 id and loaded ids when filter misses', async () => { + const logger = { info: jest.fn(), error: jest.fn() }; + const toolA = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A', id: 'id-A' }); + const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'server-B', id: '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', + loadedIds: expect.arrayContaining(['id-A', 'id-B']), + }), + ); + }); }); describe('McpToolNotFoundError', () => { From 63567f9ba03bf530f388c4c057c04a2a86b83d8f Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 19 May 2026 20:27:46 +0200 Subject: [PATCH 3/6] =?UTF-8?q?chore(workflow-executor):=20address=20revie?= =?UTF-8?q?w=20=E2=80=94=20make=20ForestIntegrationConfig.id=20optional,?= =?UTF-8?q?=20drop=20redundant=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligns ForestIntegrationConfig with RemoteTool/McpServerConfig where id is already optional, and removes WHAT-explaining comments that the named code already documents. --- .../ai-proxy/src/forest-integration-client.ts | 2 +- packages/ai-proxy/src/mcp-client.ts | 3 --- packages/ai-proxy/src/remote-tool.ts | 3 --- .../test/forest-integration-client.test.ts | 25 +++++++++++++++---- packages/ai-proxy/test/mcp-client.test.ts | 3 --- .../src/executors/mcp-step-executor.ts | 3 --- .../workflow-executor/test/errors.test.ts | 1 - .../test/executors/mcp-step-executor.test.ts | 6 ----- 8 files changed, 21 insertions(+), 25 deletions(-) diff --git a/packages/ai-proxy/src/forest-integration-client.ts b/packages/ai-proxy/src/forest-integration-client.ts index 11ffec59e4..cde4d0ddba 100644 --- a/packages/ai-proxy/src/forest-integration-client.ts +++ b/packages/ai-proxy/src/forest-integration-client.ts @@ -14,7 +14,7 @@ export type CustomConfig = ZendeskConfig | KolarConfig | SnowflakeConfig; export type ForestIntegrationName = 'Zendesk' | 'Kolar' | 'Snowflake'; export interface ForestIntegrationConfig { - id: string; + id?: string; integrationName: ForestIntegrationName; config: CustomConfig; isForestConnector: true; diff --git a/packages/ai-proxy/src/mcp-client.ts b/packages/ai-proxy/src/mcp-client.ts index 76e82ec95a..3f742d670e 100644 --- a/packages/ai-proxy/src/mcp-client.ts +++ b/packages/ai-proxy/src/mcp-client.ts @@ -8,9 +8,6 @@ import McpServerRemoteTool from './mcp-server-remote-tool'; export type McpServers = MultiServerMCPClient['config']['mcpServers']; -// Base orchestrator entry shape from `@langchain/mcp-adapters`, widened with the optional -// stable DB `id` exposed by Forest Admin's orchestrator. The id flows down to RemoteTool -// so the workflow executor can match a step's `mcpServerId` against the config record. export type McpServerConfig = MultiServerMCPClient['config']['mcpServers'][string] & { id?: string; }; diff --git a/packages/ai-proxy/src/remote-tool.ts b/packages/ai-proxy/src/remote-tool.ts index f7051c0cf8..8b6be49de4 100644 --- a/packages/ai-proxy/src/remote-tool.ts +++ b/packages/ai-proxy/src/remote-tool.ts @@ -4,9 +4,6 @@ export default abstract class RemoteTool { base: StructuredToolInterface; sourceId: string; sourceType: string; - // Stable DB id of the originating config entry. Distinct from `sourceId`, which is the - // human-readable server/integration name. Use `id` for cross-system references (e.g. - // matching a workflow step's mcpServerId to its config); use `sourceId` for display. id?: string; constructor(options: { diff --git a/packages/ai-proxy/test/forest-integration-client.test.ts b/packages/ai-proxy/test/forest-integration-client.test.ts index 385ed03571..86c8737356 100644 --- a/packages/ai-proxy/test/forest-integration-client.test.ts +++ b/packages/ai-proxy/test/forest-integration-client.test.ts @@ -204,13 +204,11 @@ describe('ForestIntegrationClient', () => { integrationName: 'Zendesk', config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, isForestConnector: true, - } as any, + }, ]); await client.loadTools(); - // The exact signature shape (positional vs. options bag) is an implementation - // detail of the production change; the test pins down that id reaches the factory. expect(getZendeskTools).toHaveBeenCalledWith( expect.objectContaining({ subdomain: 'test' }), 'forest-zendesk-42', @@ -224,7 +222,7 @@ describe('ForestIntegrationClient', () => { integrationName: 'Kolar', config: { apiKey: 'key' }, isForestConnector: true, - } as any, + }, ]); await client.loadTools(); @@ -242,7 +240,7 @@ describe('ForestIntegrationClient', () => { integrationName: 'Snowflake', config: { accountIdentifier: 'a', programmaticAccessToken: 'tok' }, isForestConnector: true, - } as any, + }, ]); await client.loadTools(); @@ -252,5 +250,22 @@ describe('ForestIntegrationClient', () => { '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/mcp-client.test.ts b/packages/ai-proxy/test/mcp-client.test.ts index 9e8d525ef4..20a24c8712 100644 --- a/packages/ai-proxy/test/mcp-client.test.ts +++ b/packages/ai-proxy/test/mcp-client.test.ts @@ -89,9 +89,6 @@ describe('McpClient', () => { schema: undefined, responseFormat: 'content', }); - // Each config entry carries the stable DB id surfaced by the orchestrator. - // The McpClient must thread that id through to RemoteTool.id so the executor - // can match workflow steps against it. const configWithId = { configs: { slack: { diff --git a/packages/workflow-executor/src/executors/mcp-step-executor.ts b/packages/workflow-executor/src/executors/mcp-step-executor.ts index 810d301bf8..1c724ecb49 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -224,9 +224,6 @@ export default class McpStepExecutor extends BaseStepExecutor ); } - // Match the workflow step's mcpServerId against the stable DB id carried on each tool — - // not the display-oriented sourceId — so server-name collisions cannot leak tools across - // configs and the same id selects both regular MCP entries and Forest-connector entries. private getFilteredTools(): RemoteTool[] { const { mcpServerId } = this.context.stepDefinition; const tools = mcpServerId diff --git a/packages/workflow-executor/test/errors.test.ts b/packages/workflow-executor/test/errors.test.ts index 52cdaacc3c..ba156eec28 100644 --- a/packages/workflow-executor/test/errors.test.ts +++ b/packages/workflow-executor/test/errors.test.ts @@ -89,7 +89,6 @@ describe('NoMcpToolsError', () => { const err = new Ctor('id-missing', []); expect(err.message).toMatch(/id-missing/); - // Should not produce undefined/null/[object Object] artefacts in the message. expect(err.message).not.toMatch(/undefined|null|\[object/i); }); 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 4f43d76109..c1ce5ae922 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -442,8 +442,6 @@ describe('McpStepExecutor', () => { describe('mcpServerId filter (matches by tool.id, not tool.sourceId)', () => { it('passes only tools whose id matches step.mcpServerId to the AI', async () => { - // Both tools share sourceId='zendesk' (e.g. same integration server name) but carry - // distinct stable DB ids — proves the filter goes through id, not sourceId. const toolA = new MockRemoteTool({ name: 'tool_a', sourceId: 'zendesk', id: 'id-A' }); const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'zendesk', id: 'id-B' }); const invokeFn = jest.fn().mockResolvedValue('ok'); @@ -473,8 +471,6 @@ describe('McpStepExecutor', () => { }); it('does not match by sourceId — server-name collisions must not leak tools across configs', async () => { - // mcpServerId='server-B' should NOT match a tool that merely has sourceId='server-B' but - // a different DB id; only tool.id is authoritative. const tool = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-B', id: 'id-99' }); const context = makeContext({ stepDefinition: makeStep({ mcpServerId: 'server-B' }), @@ -505,8 +501,6 @@ describe('McpStepExecutor', () => { }); it('resolves a Forest-connector-backed tool when its id is threaded through', async () => { - // Simulates a Forest connector entry (e.g. Zendesk) where ForestIntegrationConfig.id - // has been propagated all the way to RemoteTool.id by the ai-proxy construction site. const invokeFn = jest.fn().mockResolvedValue('done'); const forestTool = new MockRemoteTool({ name: 'zendesk_get_tickets', From 4edc2e4f816bebec105a768222193a349e74ca40 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Wed, 20 May 2026 12:54:55 +0200 Subject: [PATCH 4/6] refactor(ai-proxy): rename RemoteTool.id to mcpServerId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RemoteTool.id` was misleading — the field carries the foreign-key id of the originating MCP server config, not the tool's own identity. The consumer-site comparison `tool.mcpServerId === step.mcpServerId` now reads the relationship plainly. McpServerConfig.id and ForestIntegrationConfig.id stay unchanged — those legitimately are the entry's own DB id. --- .../ai-proxy/src/integrations/kolar/tools.ts | 4 +- .../src/integrations/snowflake/tools.ts | 7 +- .../src/integrations/zendesk/tools.ts | 4 +- packages/ai-proxy/src/mcp-client.ts | 6 +- .../ai-proxy/src/mcp-server-remote-tool.ts | 2 +- packages/ai-proxy/src/remote-tool.ts | 6 +- packages/ai-proxy/src/server-remote-tool.ts | 2 +- .../test/forest-integration-client.test.ts | 2 +- .../test/integrations/kolar/tools.test.ts | 17 ++-- .../test/integrations/snowflake/tools.test.ts | 17 ++-- .../test/integrations/zendesk/tools.test.ts | 17 ++-- packages/ai-proxy/test/mcp-client.test.ts | 11 ++- packages/workflow-executor/src/errors.ts | 8 +- .../src/executors/mcp-step-executor.ts | 10 ++- .../workflow-executor/test/errors.test.ts | 19 ++--- .../test/executors/mcp-step-executor.test.ts | 81 ++++++++++++++----- 16 files changed, 121 insertions(+), 92 deletions(-) diff --git a/packages/ai-proxy/src/integrations/kolar/tools.ts b/packages/ai-proxy/src/integrations/kolar/tools.ts index cc8f7c708d..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, id?: string): RemoteTool[] { +export default function getKolarTools(config: KolarConfig, mcpServerId?: string): RemoteTool[] { const { baseUrl, headers } = getKolarConfig(config); return [ @@ -23,7 +23,7 @@ export default function getKolarTools(config: KolarConfig, id?: string): RemoteT tool => new ServerRemoteTool({ sourceId: 'kolar', - id, + mcpServerId, tool, }), ); diff --git a/packages/ai-proxy/src/integrations/snowflake/tools.ts b/packages/ai-proxy/src/integrations/snowflake/tools.ts index 06d9621700..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, id?: string): RemoteTool[] { +export default function getSnowflakeTools( + config: SnowflakeConfig, + mcpServerId?: string, +): RemoteTool[] { const headers = getSnowflakeAuthHeaders(config); const baseUrl = getSnowflakeBaseUrl(config.accountIdentifier); @@ -27,7 +30,7 @@ export default function getSnowflakeTools(config: SnowflakeConfig, id?: string): tool => new ServerRemoteTool({ sourceId: 'snowflake', - id, + mcpServerId, tool, }), ); diff --git a/packages/ai-proxy/src/integrations/zendesk/tools.ts b/packages/ai-proxy/src/integrations/zendesk/tools.ts index 80b3b5bdd1..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, id?: string): RemoteTool[] { +export default function getZendeskTools(config: ZendeskConfig, mcpServerId?: string): RemoteTool[] { const { baseUrl, headers } = getZendeskConfig(config); return [ @@ -29,7 +29,7 @@ export default function getZendeskTools(config: ZendeskConfig, id?: string): Rem tool => new ServerRemoteTool({ sourceId: 'zendesk', - id, + mcpServerId, tool, }), ); diff --git a/packages/ai-proxy/src/mcp-client.ts b/packages/ai-proxy/src/mcp-client.ts index 3f742d670e..9b1b3cca2c 100644 --- a/packages/ai-proxy/src/mcp-client.ts +++ b/packages/ai-proxy/src/mcp-client.ts @@ -45,7 +45,11 @@ export default class McpClient implements ToolProvider { const loadedTools = (await client.getTools()) ?? []; const extendedTools = loadedTools.map( tool => - new McpServerRemoteTool({ tool, sourceId: name, id: this.idsByServerName[name] }), + new McpServerRemoteTool({ + tool, + sourceId: name, + mcpServerId: this.idsByServerName[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 70113a2eff..85791d895e 100644 --- a/packages/ai-proxy/src/mcp-server-remote-tool.ts +++ b/packages/ai-proxy/src/mcp-server-remote-tool.ts @@ -6,7 +6,7 @@ export default class McpServerRemoteTool extends RemoteTool constructor(options: { tool: StructuredToolInterface; sourceId?: string; - id?: 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 8b6be49de4..05b970ec6d 100644 --- a/packages/ai-proxy/src/remote-tool.ts +++ b/packages/ai-proxy/src/remote-tool.ts @@ -4,18 +4,18 @@ export default abstract class RemoteTool { base: StructuredToolInterface; sourceId: string; sourceType: string; - id?: string; + mcpServerId?: string; constructor(options: { tool: StructuredToolInterface; sourceId?: string; sourceType?: string; - id?: string; + mcpServerId?: string; }) { this.base = options.tool; this.sourceId = options.sourceId; this.sourceType = options.sourceType; - this.id = options.id; + 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 6e7ae44ff1..2341411972 100644 --- a/packages/ai-proxy/src/server-remote-tool.ts +++ b/packages/ai-proxy/src/server-remote-tool.ts @@ -6,7 +6,7 @@ export default class ServerRemoteTool extends RemoteTool { constructor(options: { tool: StructuredToolInterface; sourceId?: string; - id?: 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 86c8737356..104fb6f80d 100644 --- a/packages/ai-proxy/test/forest-integration-client.test.ts +++ b/packages/ai-proxy/test/forest-integration-client.test.ts @@ -196,7 +196,7 @@ describe('ForestIntegrationClient', () => { }); }); - describe('id threaded from ForestIntegrationConfig into integration tool factories', () => { + 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([ { diff --git a/packages/ai-proxy/test/integrations/kolar/tools.test.ts b/packages/ai-proxy/test/integrations/kolar/tools.test.ts index e0bbb9bdb1..c4428e1161 100644 --- a/packages/ai-proxy/test/integrations/kolar/tools.test.ts +++ b/packages/ai-proxy/test/integrations/kolar/tools.test.ts @@ -27,26 +27,21 @@ describe('getKolarTools', () => { ]); }); - describe('id propagation', () => { - it('sets RemoteTool.id on every produced tool when id is provided', () => { - const tools = ( - getKolarTools as unknown as ( - cfg: typeof config, - id?: string, - ) => ReturnType - )(config, 'forest-kolar-7'); + 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 as ServerRemoteTool & { id?: string }).id).toBe('forest-kolar-7'); + expect(tool.mcpServerId).toBe('forest-kolar-7'); }); }); - it('leaves RemoteTool.id undefined when no id is provided (backwards-compatible call site)', () => { + it('leaves RemoteTool.mcpServerId undefined when no mcpServerId is provided', () => { const tools = getKolarTools(config); tools.forEach(tool => { - expect((tool as ServerRemoteTool & { id?: string }).id).toBeUndefined(); + 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 70066c9bdc..be75a9cb8c 100644 --- a/packages/ai-proxy/test/integrations/snowflake/tools.test.ts +++ b/packages/ai-proxy/test/integrations/snowflake/tools.test.ts @@ -29,26 +29,21 @@ describe('getSnowflakeTools', () => { ]); }); - describe('id propagation', () => { - it('sets RemoteTool.id on every produced tool when id is provided', () => { - const tools = ( - getSnowflakeTools as unknown as ( - cfg: SnowflakeConfig, - id?: string, - ) => ReturnType - )(config, 'forest-snowflake-99'); + 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 as ServerRemoteTool & { id?: string }).id).toBe('forest-snowflake-99'); + expect(tool.mcpServerId).toBe('forest-snowflake-99'); }); }); - it('leaves RemoteTool.id undefined when no id is provided (backwards-compatible call site)', () => { + it('leaves RemoteTool.mcpServerId undefined when no mcpServerId is provided', () => { const tools = getSnowflakeTools(config); tools.forEach(tool => { - expect((tool as ServerRemoteTool & { id?: string }).id).toBeUndefined(); + 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 bf75cf1ce7..24bd2e0fc9 100644 --- a/packages/ai-proxy/test/integrations/zendesk/tools.test.ts +++ b/packages/ai-proxy/test/integrations/zendesk/tools.test.ts @@ -29,26 +29,21 @@ describe('getZendeskTools', () => { ]); }); - describe('id propagation', () => { - it('sets RemoteTool.id on every produced tool when id is provided', () => { - const tools = ( - getZendeskTools as unknown as ( - cfg: typeof config, - id?: string, - ) => ReturnType - )(config, 'forest-zendesk-42'); + 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 as ServerRemoteTool & { id?: string }).id).toBe('forest-zendesk-42'); + expect(tool.mcpServerId).toBe('forest-zendesk-42'); }); }); - it('leaves RemoteTool.id undefined when no id is provided (backwards-compatible call site)', () => { + it('leaves RemoteTool.mcpServerId undefined when no mcpServerId is provided', () => { const tools = getZendeskTools(config); tools.forEach(tool => { - expect((tool as ServerRemoteTool & { id?: string }).id).toBeUndefined(); + 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 20a24c8712..ad128c2fbd 100644 --- a/packages/ai-proxy/test/mcp-client.test.ts +++ b/packages/ai-proxy/test/mcp-client.test.ts @@ -1,5 +1,4 @@ import type { McpConfiguration } from '../src'; -import type RemoteTool from '../src/remote-tool'; import { tool } from '@langchain/core/tools'; @@ -81,8 +80,8 @@ describe('McpClient', () => { }); }); - describe('id threaded from config entry into McpServerRemoteTool', () => { - it('sets RemoteTool.id from the config entry id alongside sourceId from the map key', async () => { + 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', @@ -107,10 +106,10 @@ describe('McpClient', () => { expect(tools).toHaveLength(1); expect(tools[0].sourceId).toBe('slack'); - expect((tools[0] as RemoteTool & { id?: string }).id).toBe('config-id-42'); + expect(tools[0].mcpServerId).toBe('config-id-42'); }); - it('leaves RemoteTool.id undefined when the config entry has no id (legacy / unenriched payload)', async () => { + it('leaves RemoteTool.mcpServerId undefined when the config entry has no id (legacy / unenriched payload)', async () => { const tool1 = tool(() => {}, { name: 'tool1', description: 'description1', @@ -122,7 +121,7 @@ describe('McpClient', () => { const tools = await mcpClient.loadTools(); - expect((tools[0] as RemoteTool & { id?: string }).id).toBeUndefined(); + expect(tools[0].mcpServerId).toBeUndefined(); }); }); diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 114ecbd927..4aeb7f4618 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -203,10 +203,10 @@ export class StepTimeoutError extends WorkflowExecutorError { } export class NoMcpToolsError extends WorkflowExecutorError { - constructor(requestedId?: string, loadedIds?: readonly string[]) { - const technical = requestedId - ? `No MCP tools available for mcpServerId="${requestedId}". Loaded MCP config ids: [${( - loadedIds ?? [] + 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 1c724ecb49..2723976888 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -227,18 +227,20 @@ export default class McpStepExecutor extends BaseStepExecutor private getFilteredTools(): RemoteTool[] { const { mcpServerId } = this.context.stepDefinition; const tools = mcpServerId - ? this.remoteTools.filter(t => t.id === mcpServerId) + ? this.remoteTools.filter(t => t.mcpServerId === mcpServerId) : [...this.remoteTools]; if (tools.length === 0) { - const loadedIds = this.remoteTools.map(t => t.id).filter((value): value is string => !!value); - const error = new NoMcpToolsError(mcpServerId, loadedIds); + 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, - loadedIds, + loadedMcpServerIds, }); throw error; } diff --git a/packages/workflow-executor/test/errors.test.ts b/packages/workflow-executor/test/errors.test.ts index ba156eec28..369d99f46a 100644 --- a/packages/workflow-executor/test/errors.test.ts +++ b/packages/workflow-executor/test/errors.test.ts @@ -60,40 +60,35 @@ describe('extractErrorMessage', () => { }); describe('NoMcpToolsError', () => { - const Ctor = NoMcpToolsError as unknown as new ( - requestedId?: string, - loadedIds?: readonly string[], - ) => NoMcpToolsError; - - it('produces a fully generic technical message when no id was requested (no filter case)', () => { - const err = new Ctor(); + 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 Ctor('id-missing', ['id-A', 'id-B']); + const err = new NoMcpToolsError('id-missing', ['id-A', 'id-B']); expect(err.message).toMatch(/id-missing/); }); - it('lists the loaded MCP config ids in the technical message so misconfigurations are diagnosable', () => { - const err = new Ctor('id-missing', ['id-A', 'id-B']); + 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 Ctor('id-missing', []); + 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 Ctor('id-missing', ['id-A', 'id-B']); + 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 c1ce5ae922..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; id?: 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,8 +32,8 @@ class MockRemoteTool extends RemoteTool { } as unknown as RemoteTool['base'], sourceId: options.sourceId ?? 'mcp-server-1', sourceType: 'mcp', - id: options.id, - } as ConstructorParameters[0] & { id?: string }); + mcpServerId: options.mcpServerId, + }); } } @@ -440,15 +445,23 @@ describe('McpStepExecutor', () => { }); }); - describe('mcpServerId filter (matches by tool.id, not tool.sourceId)', () => { - it('passes only tools whose id matches step.mcpServerId to the AI', async () => { - const toolA = new MockRemoteTool({ name: 'tool_a', sourceId: 'zendesk', id: 'id-A' }); - const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'zendesk', id: 'id-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: 'zendesk', - id: 'id-B', + mcpServerId: 'id-B', invoke: invokeFn, }); @@ -471,7 +484,11 @@ describe('McpStepExecutor', () => { }); 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', id: 'id-99' }); + const tool = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'server-B', + mcpServerId: 'id-99', + }); const context = makeContext({ stepDefinition: makeStep({ mcpServerId: 'server-B' }), }); @@ -484,8 +501,16 @@ describe('McpStepExecutor', () => { }); it('returns all tools (no filter) when step.mcpServerId is absent', async () => { - const toolA = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A', id: 'id-A' }); - const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'server-B', id: 'id-B' }); + 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, @@ -500,12 +525,12 @@ describe('McpStepExecutor', () => { expect(boundNames).toEqual(expect.arrayContaining(['tool_a', 'tool_b'])); }); - it('resolves a Forest-connector-backed tool when its id is threaded through', async () => { + 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', - id: 'forest-connector-42', + mcpServerId: 'forest-connector-42', invoke: invokeFn, }); const { model, bindTools } = makeMockModel('zendesk_get_tickets', {}); @@ -539,7 +564,11 @@ describe('McpStepExecutor', () => { }); it('returns error when mcpServerId filter yields no tools', async () => { - const tool = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A', id: 'id-A' }); + const tool = new MockRemoteTool({ + name: 'tool_a', + sourceId: 'server-A', + mcpServerId: 'id-A', + }); const context = makeContext({ stepDefinition: makeStep({ mcpServerId: 'id-B' }), }); @@ -551,8 +580,12 @@ describe('McpStepExecutor', () => { 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 id', async () => { - const tool = new MockRemoteTool({ name: 'tool_a', sourceId: 'server-A', id: 'id-A' }); + 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]); @@ -562,10 +595,18 @@ describe('McpStepExecutor', () => { expect(result.stepOutcome.error).not.toMatch(/id-B/); }); - it('logs the technical message with the requested id and loaded ids when filter misses', async () => { + 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', id: 'id-A' }); - const toolB = new MockRemoteTool({ name: 'tool_b', sourceId: 'server-B', id: 'id-B' }); + 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' }), @@ -578,7 +619,7 @@ describe('McpStepExecutor', () => { expect.stringMatching(/id-missing/), expect.objectContaining({ requestedMcpServerId: 'id-missing', - loadedIds: expect.arrayContaining(['id-A', 'id-B']), + loadedMcpServerIds: expect.arrayContaining(['id-A', 'id-B']), }), ); }); From ceacd3525c984d724222544a800c59b958bf2f64 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Wed, 20 May 2026 14:20:51 +0200 Subject: [PATCH 5/6] refactor(ai-proxy): rename local id bindings to mcpServerId for symmetry The config types (`ForestIntegrationConfig.id`, `McpServerConfig.id`) keep the wire-shape field name `id`, but the local pass-through bindings inside `ForestIntegrationClient.loadTools` and `McpClient` are renamed via destructure-aliasing so the variables and the private map read symmetric with the `RemoteTool.mcpServerId` they ultimately feed. --- packages/ai-proxy/src/forest-integration-client.ts | 8 ++++---- packages/ai-proxy/src/mcp-client.ts | 9 +++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/ai-proxy/src/forest-integration-client.ts b/packages/ai-proxy/src/forest-integration-client.ts index cde4d0ddba..dff89c8e82 100644 --- a/packages/ai-proxy/src/forest-integration-client.ts +++ b/packages/ai-proxy/src/forest-integration-client.ts @@ -40,16 +40,16 @@ export default class ForestIntegrationClient implements ToolProvider { async loadTools(): Promise { const tools: RemoteTool[] = []; - this.configs.forEach(({ id, integrationName, config }) => { + this.configs.forEach(({ id: mcpServerId, integrationName, config }) => { switch (integrationName) { case 'Zendesk': - tools.push(...getZendeskTools(config as ZendeskConfig, id)); + tools.push(...getZendeskTools(config as ZendeskConfig, mcpServerId)); break; case 'Kolar': - tools.push(...getKolarTools(config as KolarConfig, id)); + tools.push(...getKolarTools(config as KolarConfig, mcpServerId)); break; case 'Snowflake': - tools.push(...getSnowflakeTools(config as SnowflakeConfig, id)); + tools.push(...getSnowflakeTools(config as SnowflakeConfig, mcpServerId)); break; default: this.logger?.('Warn', `Unsupported integration: ${integrationName}`); diff --git a/packages/ai-proxy/src/mcp-client.ts b/packages/ai-proxy/src/mcp-client.ts index 9b1b3cca2c..bb834c576e 100644 --- a/packages/ai-proxy/src/mcp-client.ts +++ b/packages/ai-proxy/src/mcp-client.ts @@ -18,7 +18,7 @@ export type McpConfiguration = { export default class McpClient implements ToolProvider { private readonly mcpClients: Record = {}; - private readonly idsByServerName: Record = {}; + private readonly mcpServerIdsByName: Record = {}; private readonly logger?: Logger; constructor(config: McpConfiguration, logger?: Logger) { @@ -26,8 +26,9 @@ 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, ...rest } = serverConfig as McpServerConfig & Record; - this.idsByServerName[name] = id; + const { id: mcpServerId, ...rest } = serverConfig as McpServerConfig & + Record; + this.mcpServerIdsByName[name] = mcpServerId; this.mcpClients[name] = new MultiServerMCPClient({ mcpServers: { [name]: rest as McpServerConfig }, ...config, @@ -48,7 +49,7 @@ export default class McpClient implements ToolProvider { new McpServerRemoteTool({ tool, sourceId: name, - mcpServerId: this.idsByServerName[name], + mcpServerId: this.mcpServerIdsByName[name], }), ); tools.push(...extendedTools); From 095113b9324c1953dd21c44164287a8d562f1f72 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Wed, 20 May 2026 15:26:16 +0200 Subject: [PATCH 6/6] docs(ai-proxy): clarify why mcpServerId is shared across RemoteTool subclasses The orchestrator stores Forest connectors alongside user MCP servers in the same `ai_mcp_configs` table, so the FK lives on the base class. --- packages/ai-proxy/src/remote-tool.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/ai-proxy/src/remote-tool.ts b/packages/ai-proxy/src/remote-tool.ts index 05b970ec6d..a59911664a 100644 --- a/packages/ai-proxy/src/remote-tool.ts +++ b/packages/ai-proxy/src/remote-tool.ts @@ -4,6 +4,8 @@ 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: {