fix(ai-proxy): route Forest connectors via tool-provider split in AiClient#1599
Conversation
…lient AiClient.loadRemoteTools was instantiating McpClient directly, forwarding every config (including ForestIntegrationConfig entries like Zendesk) to MultiServerMCPClient. Forest connectors lack the stdio/HTTP transport fields, so @langchain/mcp-adapters threw a Zod union error and any workflow run touching a Forest-hosted connector crashed before the step could execute. Delegate to createToolProviders so Forest connectors are routed to ForestIntegrationClient — same path Router and ToolSourceChecker already use. Fixes PRD-400. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…test The previous "does not crash on Forest connector configs" test in ai-client.test.ts mocked createToolProviders, so it would have passed against the buggy code too. Replace it with a dedicated routing test that uses the real createToolProviders and asserts McpClient is never constructed with a Forest connector entry — which is the exact bug PRD-400 fixed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Coverage Impact Unable to calculate total coverage change because base branch coverage was not found. Modified Files with Diff Coverage (5)
🛟 Help
|
| dispose: disposeMock, | ||
| } as unknown as McpClient), | ||
| ); | ||
| it('disposes every stored provider', async () => { |
There was a problem hiding this comment.
Claude Opus 4.7 · Should fix
The dispose-failure tests all use a single provider, and this two-provider test has both dispose resolve — so nothing pins the Promise.allSettled isolation contract that this PR introduces. If disposeToolProviders were refactored back to Promise.all, a leading rejection would skip the second provider's disposal and every test here would still pass. Add a case with two providers where the first dispose rejects and the second resolves, then assert the second's dispose still ran and the logger was called once with the rejected reason.
There was a problem hiding this comment.
(Claude's human here) - Good safeguard to have but it's more of a preferential for me
| this.mcpClient = newClient; | ||
| const providers = createToolProviders(configs, this.logger); | ||
| const toolsByProvider = await Promise.all(providers.map(p => p.loadTools())); | ||
| this.toolProviders = providers; |
There was a problem hiding this comment.
Claude Opus 4.7 · Preferential
this.toolProviders = providers runs only after await Promise.all(...) resolves. If any provider's loadTools() rejects, the created providers are never stored, so a provider that already opened connections can't be disposed via closeConnections(). This is currently latent — ForestIntegrationClient.loadTools opens nothing and McpClient.loadTools swallows per-server errors rather than rejecting — and it matches the previous single-McpClient pattern, so not a regression. Defensive option: assign before awaiting, or wrap the load in a try/catch that disposes the freshly-created providers before rethrowing. (The test at line 229 currently codifies the drop-on-failure behaviour.)
| } | ||
|
|
||
| async getMcpServerConfigs(): Promise<McpServers> { | ||
| async getMcpServerConfigs(): Promise<Record<string, ToolConfig>> { |
There was a problem hiding this comment.
getToolServerConfigs would be more consistent, but I think it's out of scope for this PR. If you agree, perhaps file a refactor ticket ?
christophebrun-forest
merged commit a5f01b5
into
feat/prd-214-server-step-mapper
christophebrun-forest
deleted the
fix/prd-400-forest-connectors-loadremotetools
branch
2 participants
Summary
AiClient.loadRemoteToolsnow delegates tocreateToolProviders(same path asRouterandToolSourceChecker), so Forest-hosted connectors (Zendesk, Kolar, Snowflake) are routed toForestIntegrationClientinstead of being forwarded toMultiServerMCPClient.getMcpServerConfigs()response included a Forest connector crashed at step setup withZodError invalid_union mcpServers.<name>— Forest connector entries have neithercommand+args(stdio) norurl(HTTP), so the@langchain/mcp-adaptersschema rejected them.Record<string, ToolConfig>shape throughAiModelPort, the three adapters (ServerAiAdapter,AiClientAdapter,AlwaysErrorAiModelPort),Runner.fetchRemoteTools(drops the now-redundant{ configs }wrapping), andWorkflowPort.getMcpServerConfigs(was mistyped asMcpServerswhile the orchestrator already returns a mixed record).Fixes PRD-400.
Test plan
yarn workspace @forestadmin/ai-proxy test— 400 passing (new regression test inai-client.test.tscovers Forest connector configs lacking MCP transport fields).yarn workspace @forestadmin/workflow-executor test— 786 passing.yarn workspace @forestadmin/ai-proxy lint && yarn workspace @forestadmin/workflow-executor lint— clean (preexisting warnings only).yarn workspace @forestadmin/ai-proxy build && yarn workspace @forestadmin/workflow-executor build— clean.🤖 Generated with Claude Code
Note
Route Forest connectors via a generic tool-provider split in
AiClientMcpClientfield inAiClientwith atoolProvidersarray, allowing multiple provider kinds (e.g. MCP and Forest) to be loaded together viacreateToolProviders.loadRemoteToolsnow acceptsRecord<string, ToolConfig>instead ofMcpConfiguration, creates providers for each config entry, flattens their tools, and disposes any previously loaded providers before replacing them.AiModelPort,WorkflowPort) and adapters are updated to useRecord<string, ToolConfig>end-to-end, removing theMcpServerswrapper type.disposeToolProvidersusesPromise.allSettledto isolate per-provider failures and log each rejection individually without interrupting others.Macroscope summarized fcc1305.