Skip to content

fix(workflow-executor): align getMcpServerConfigs port with Record shape#1583

Merged
hercemer42 merged 3 commits into
feat/prd-214-server-step-mapperfrom
fix/prd-357-fetchremotetools-record-shape
May 20, 2026
Merged

fix(workflow-executor): align getMcpServerConfigs port with Record shape#1583
hercemer42 merged 3 commits into
feat/prd-214-server-step-mapperfrom
fix/prd-357-fetchremotetools-record-shape

Conversation

@hercemer42
Copy link
Copy Markdown

@hercemer42 hercemer42 commented May 18, 2026

Summary

  • Runner.fetchRemoteTools() crashed with TypeError: configs.map is not a function on every MCP-typed step. The orchestrator's GET /liana/mcp-server-configs-with-details returns Record<string, ToolConfig>, but the executor port was typed Promise<McpConfiguration[]> and the runner called .map on the object.
  • Aligns the executor with the wire contract: port now returns McpServers (Record<string, McpServerConfig>, from @forestadmin/ai-proxy), runner wraps the record into { configs } for loadRemoteTools, with an empty-Record short-circuit.
  • Updates 9 test mock sites (8 mockResolvedValue([]) plus the [{ configs: {} }] as never cast in runner.test.ts) to the real Record shape so the contract can't silently regress again. Adds two TDD tests under MCP lazy loading (via once thunk) for the non-empty-Record happy path and the empty-Record short-circuit.

Dependency bumps (heads-up)

packages/workflow-executor/package.json bumps:

  • @forestadmin/ai-proxy 1.8.0 → 1.9.0 — load-bearing for this fix (the McpServers type only exists from 1.9.0 onward).
  • @forestadmin/forestadmin-client 1.39.5 → 1.39.7 — incidental, was already staged in the user's pre-existing main-branch state.

These cannot be split into a separate PR because the workspace-resolved import in src/ports/workflow-port.ts would fail to compile against ai-proxy 1.8.0.

Test plan

  • yarn workspace @forestadmin/workflow-executor jest --runInBand --forceExit --testMatch='**/runner.test.ts' — 83/83 pass (including 3 new MCP-lazy-loading tests)
  • yarn workspace @forestadmin/workflow-executor jest --runInBand --forceExit on the 9 changed test files — 361 tests green
  • npx eslint, npx prettier --check, npx tsc --noEmit on all 13 changed files — clean
  • End-to-end repro against data-gouv MCP server on the executor dev project — pre-fix run aborted with the generic "An unexpected error occurred." (masking the TypeError); post-fix run reports NoMcpToolsError cleanly, which is the legitimate downstream behaviour when no tools materialize

Refs: PRD-357

Note

Fix getMcpServerConfigs return type to use McpServers Record shape in workflow-executor

  • Replaces McpConfiguration[] array with McpServers (a Record<string, ToolConfig> map) as the return type of getMcpServerConfigs across workflow-port.ts, forest-server-workflow-port.ts, and the public exports in index.ts.
  • Updates Runner.fetchRemoteTools in runner.ts to skip calling aiModelPort.loadRemoteTools when the config map is empty, and passes { configs } (the Record wrapped in an object) when non-empty.
  • Bumps @forestadmin/ai-proxy to 1.9.0 and @forestadmin/forestadmin-client to 1.39.7.
  • Behavioral Change: loadRemoteTools is no longer called when the orchestrator returns an empty MCP config map; callers relying on the old array format for getMcpServerConfigs will need to update to the Record shape.

Changes since #1583 opened

  • Fixed McpStepExecutor.getFilteredTools to filter tools by tool.mcpServerId instead of tool.sourceId when a step specifies mcpServerId, and updated NoMcpToolsError to include diagnostic context showing the requested mcpServerId and list of available mcpServerIds when filtering yields no matches [53618a7]
  • Added optional mcpServerId property to RemoteTool class and threaded it through ServerRemoteTool and McpServerRemoteTool subclass constructors [53618a7]
  • Modified McpClient to extract id from each server config entry, store it in an internal mcpServerIdsByName map, and populate mcpServerId on McpServerRemoteTool instances during tool loading, and changed McpConfiguration.configs type from MultiServerMCPClient['config']['mcpServers'] to Record<string, McpServerConfig> where McpServerConfig includes optional id?: string [53618a7]
  • Extended ForestIntegrationConfig interface with optional id?: string and updated ForestIntegrationClient.loadTools to thread this id as mcpServerId parameter to getZendeskTools, getKolarTools, and getSnowflakeTools factory functions, which were each extended to accept optional mcpServerId and pass it to ServerRemoteTool constructor [53618a7]
  • Added test coverage for mcpServerId propagation across ForestIntegrationClient, integration tool factories (getZendeskTools, getKolarTools, getSnowflakeTools), McpClient, McpStepExecutor filtering behavior, and NoMcpToolsError message construction [53618a7]

Macroscope summarized 480148c.

@linear
Copy link
Copy Markdown

linear Bot commented May 18, 2026

PRD-357

@qltysh
Copy link
Copy Markdown

qltysh Bot commented May 18, 2026

Qlty


Coverage Impact

⬆️ Merging this pull request will increase total coverage on feat/prd-214-server-step-mapper by 0.01%.

Modified Files with Diff Coverage (10)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
packages/workflow-executor/src/runner.ts100.0%
Coverage rating: A Coverage rating: A
packages/ai-proxy/src/integrations/snowflake/tools.ts100.0%
Coverage rating: A Coverage rating: A
packages/ai-proxy/src/remote-tool.ts100.0%
Coverage rating: A Coverage rating: A
packages/ai-proxy/src/integrations/zendesk/tools.ts100.0%
Coverage rating: A Coverage rating: A
packages/ai-proxy/src/forest-integration-client.ts100.0%
Coverage rating: A Coverage rating: A
.../workflow-executor/src/adapters/forest-server-workflow-port.ts100.0%
Coverage rating: A Coverage rating: A
packages/ai-proxy/src/mcp-client.ts100.0%
Coverage rating: A Coverage rating: A
packages/ai-proxy/src/integrations/kolar/tools.ts100.0%
Coverage rating: A Coverage rating: A
packages/workflow-executor/src/errors.ts100.0%
Coverage rating: A Coverage rating: A
packages/workflow-executor/src/executors/mcp-step-executor.ts100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

Comment thread packages/workflow-executor/src/ports/workflow-port.ts Outdated
Comment thread packages/workflow-executor/src/index.ts Outdated
Comment thread packages/workflow-executor/test/runner.test.ts Outdated
@hercemer42 hercemer42 marked this pull request as ready for review May 18, 2026 15:24
Comment thread packages/workflow-executor/package.json
The orchestrator's /liana/mcp-server-configs-with-details endpoint returns
Record<string, ToolConfig>, 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
…mize test server name

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
@hercemer42 hercemer42 force-pushed the fix/prd-357-fetchremotetools-record-shape branch from 32c1afb to 480148c Compare May 20, 2026 08:30
Copy link
Copy Markdown
Member

@matthv matthv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Clean fix 🏅

* 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
@hercemer42 hercemer42 merged commit 74e4b8f into feat/prd-214-server-step-mapper May 20, 2026
30 checks passed
@hercemer42 hercemer42 deleted the fix/prd-357-fetchremotetools-record-shape branch May 20, 2026 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants