fix(workflow-executor): match MCP tools by config id#1584
Conversation
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (8)
🛟 Help
|
hercemer42
force-pushed
the
fix/prd-357-fetchremotetools-record-shape
branch
from
32c1afb to
480148c
Compare
| const extendedTools = loadedTools.map( | ||
| tool => new McpServerRemoteTool({ tool, sourceId: name }), | ||
| tool => | ||
| new McpServerRemoteTool({ tool, sourceId: name, id: this.idsByServerName[name] }), |
There was a problem hiding this comment.
I think id is badly named in the code, It should be name mcpServerId, what do you think?
There was a problem hiding this comment.
Well, for me 'id' always means database id, but I can rename if you prefer.
There was a problem hiding this comment.
Yes i'm agree but we are in the tool class, not in the mcp, by reading id we can understand that the id of the tool but it's the mcp's id.
There was a problem hiding this comment.
Ok fair point, agreed. I made the changes - I changed ID to mcpServerId only where it concerns the private maps, and left it alone in the type and wiring - otherwise I'll have to change it in the orchestrator contract, and I think it's correct there as we call the DB on that layer. Is that ok ?
| @@ -4,15 +4,18 @@ export default abstract class RemoteTool<ToolType = unknown> { | |||
| base: StructuredToolInterface<ToolType>; | |||
| sourceId: string; | |||
There was a problem hiding this comment.
sourceId is currently the mcp server name. And Id the mcp server id. Could we improve it by adding a commentary or an alias or ... on other idea?
Maybe id could be sourcerMcpServerId and sourceId => sourceMcpServerName but maybe it's breaking, we can maybe find a way to add a getter to encapsulate the wrong naming ?
There was a problem hiding this comment.
get sourceMcpServerName() => return sourceId.
There was a problem hiding this comment.
Is RemoteTool.mcpServerId ok to keep consistent with the other changes ? As for sourceId we can but it'll make more changes so if you really want to do that I suggest we open a refacto ticket.
There was a problem hiding this comment.
If I'm reading this right, mcpServerId is accurate on McpServerRemoteTool but inherited by ServerRemoteTool (Zendesk, Snowflake, ...), whose value is the DB id of an ai_mcp_configs row with isForestConnector: true. The name reads accurately on half the subclasses, ambiguously on the other half. A dev seeing tool.mcpServerId === step.mcpServerId on a Snowflake tool might wonder "since when is Snowflake an MCP server?".
Would a short JSDoc here clarifying that it covers both user MCP and Forest connectors be enough, or are you leaning toward the broader sourceId rename you mentioned? Happy either way.
There was a problem hiding this comment.
Well, the Forest connectors and user MCP servers live in the same ai_mcp_configs so currently we treat them all as MCP servers. But yes I think a comment is suitable. Not pushing for the broader sourceId change because its opportunistic and out of scope for this PR, happy to do it in another ticket as mentioned to Alban.
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.
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.
…ig.id optional, drop redundant comments Aligns ForestIntegrationConfig with RemoteTool/McpServerConfig where id is already optional, and removes WHAT-explaining comments that the named code already documents.
`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.
hercemer42
force-pushed
the
fix/prd-362-match-mcp-tools-by-id
branch
from
a61082e to
4edc2e4
Compare
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.
…ubclasses The orchestrator stores Forest connectors alongside user MCP servers in the same `ai_mcp_configs` table, so the FK lives on the base class.
hercemer42
merged commit 53618a7
into
fix/prd-357-fetchremotetools-record-shape
3 participants
Summary
idfrom eachToolConfigentry throughMcpClient,ForestIntegrationClientand the integration tool factories (getZendeskTools,getKolarTools,getSnowflakeTools) ontoRemoteTool.idMcpStepExecutor.getFilteredToolsnow matchestool.id === step.mcpServerId(wastool.sourceId), so workflows targeting a specific MCP server actually resolve their tools instead of always hittingNoMcpToolsErrorNoMcpToolsErrorcarries the misconfiguredmcpServerIdand the list of loaded config ids in its technical message; user-facing message stays generic per the dual-message conventionfixes PRD-362
Stacked on
Targets
fix/prd-357-fetchremotetools-record-shape(#1583). PR #1583 introduces theRecord<string, ToolConfig>shape this change consumes. Once #1583 merges intofeat/prd-214-server-step-mapper, rebase this onto the same base.Prerequisite
End-to-end resolution also depends on the orchestrator (forestadmin-server, PRD-360) exposing the DB
idon eachRecord<string, ToolConfig>entry, including Forest-connector entries. Until that lands, the executor still firesNoMcpToolsErrorwith an enriched diagnostic message — which is now actionable from the activity log.Test plan
mcpServerIdto a non-existent id and check the activity log carries the requested id + loaded id listNote
Fix MCP tool filtering in
McpStepExecutorto match by configidinstead ofsourceIdidfield toMcpServerConfigandForestIntegrationConfig, which is threaded through tool construction so eachRemoteToolinstance carries a matchingmcpServerIdproperty.McpStepExecutor.getFilteredToolsto filter tools bytool.mcpServerIdinstead ofsourceId, fixing cross-config tool leakage.NoMcpToolsErrorto include the requestedmcpServerIdand list of loaded IDs in its technical message for easier diagnostics.sourceIdwill now only match tools whosemcpServerIdaligns with the step'smcpServerId; steps relying on the oldsourceIdmatching will no longer find tools.Changes since #1584 opened
mcpServerIdproperty inRemoteToolclass documenting shared usage acrossMcpServerRemoteToolandServerRemoteTool[095113b]Macroscope summarized 4edc2e4.