fix(workflow-executor): surface the cause when MCP tool loading fails

[hercemer42]

What

A workflow MCP step whose tool load fails told the customer that it failed, never why. Two gaps, both fixed here.

1. The executor's own line had no cause. errorOnPartialLoadFailure inferred failure by diffing config ids against loaded tool ids, so it could name the server but never the reason — a revoked token, an unreachable host and a 15s timeout all logged identically. The providers already classify each failure and carry its error, but the main load path called the tools-only loadRemoteTools and dropped them. It now reads the failures channel:

{"level":"Error","message":"MCP servers failed to load tools","requestedMcpServerId":"39","mcpServerName":"acme-crm",
 "failures":[{"server":"acme-crm","kind":"connection","error":"connect ECONNREFUSED 10.0.4.12:8080"}]}

2. ai-proxy's own diagnostics went nowhere. AiClient holds an optional host logger and no-ops every emit when none is passed; workflow-executor never passed one. Both construction sites now receive the logger the executor already builds, through to-ai-proxy-logger.ts, which bridges the two logger contracts — ai-proxy's third parameter is an Error, the executor's is a context object.

Behaviour changes worth a reviewer's eye

A healthy server exposing no tools is no longer a failure. Reading the failures channel removes a false positive the id-diff could not avoid: such a server contributed no ids, so it was reported as failed and GET /list-mcp-tools answered 503 The MCP server could not be reached to list its tools. It now answers 200 with an empty list. A server that genuinely errors still sets loadFailed and still 503s. The frontend counterpart is ForestAdmin/forestadmin#9892, which drops the matching "empty list means the load failed" inference; without it this change never reaches the user.

A recovered OAuth load now says so. When a cached token is rejected and the forced refresh succeeds, the provider has already emitted its rejection at Error, so at the default level the run read as a pure failure with nothing saying it continued. It now states the recovery at Info. Observed on the rig before it was fixed — the retry was Debug-only, so two Error lines were all an operator saw.

Pretty output no longer prints absent keys. formatContext rendered JSON.stringify(undefined) as the literal undefined, so every bridged line trailed cause=undefined — most MCP errors don't chain, since @langchain/mcp-adapters interpolates the nested text into its own message. It now skips undefined values, which also cleans up base-step-executor's existing lines.

Why the cause is flattened rather than passed through

An Error's own properties are non-enumerable, so handing it to the executor's logger as the context object drops the cause from the line that gets emitted. All three consumers agree: console-logger spreads it into JSON.stringify, pretty-logger iterates Object.entries, and the agent-embedded formatLog takes its message-only branch when Object.keys(context).length === 0. The bridge flattens to { error, cause, stack } — the shape base-step-executor.ts:94 already uses — and carries the cause chain, so a wrapped fetch failed still names the ECONNREFUSED underneath it.

The bridge also guards its own body: ai-proxy logs from inside its per-server catch before recording the failure it caught (mcp-client.ts:113, failures.push at :114), so a host logger that threw would reject the whole Promise.all — discarding tools from healthy servers and skipping the OAuth reauth pause. ExecutorOptions.logger is host-supplied, and agent/src/embedded-workflow-executor.ts:12 already states that invariant for the neighbouring edge.

Scope

The agent-nodejs half of PRD-876. The forestadmin-server half (make-ai-router-service.ts, the POST /api/ai-proxy/ai-query path — AC#1–#3) ships separately; neither PR blocks the other. No ai-proxy file is touched: the timeout, classification and failures channel stay as PRD-863 left them.

AC#4 is implemented as reworded in the ticket comment — the existing executor log line carries the failure kind and cause, sourced from loadRemoteToolsWithFailures rather than inferred from absent tools — and the logger threading it originally called for, which is what makes ai-proxy's other diagnostics (per-server load errors with stacks, Unsupported integration:, connection-cleanup failures) visible at all.

Tests

• to-ai-proxy-logger.test.ts — level/message pass-through for all four levels, cause flattening, the cause chain, the cause surviving createConsoleLogger() at its default level (business rule 3), a throwing host logger, stackless / empty-message / non-Error / null causes, independence across calls. 100% coverage.
• remote-tool-fetcher.test.ts — the log line names server, kind and cause; loadFailed follows the reported failures, including when the post-refresh retry fails for a non-auth reason; a healthy server exposing no tools is not a failure; the recovery line fires only when the retry actually recovered.
• Both AI adapter suites assert the logger reaching AiClient; build-workflow-executor asserts it reaching both adapters; runner and the integration suite cover the dispatch path.
• The 8 changed test files run 225 tests green. ai-proxy's own suite runs unchanged and green (38 suites / 429 tests) — AC#5.

Verified on a live rig

Driven through GET /list-mcp-tools, which runs the whole changed path (fetch → loadRemoteToolsWithFailures → errorOnPartialLoadFailure → ai-proxy's McpClient), against a standalone executor and the frontend:

• Unreachable server → 503, with ai-proxy's per-server Error loading tools for … carrying error/stack, its Failed to load tools from 1/1 … summary, and the executor's line with kind:"connection".
• 401 from the server → same shape with kind:"auth", so a rejected credential and an unreachable host are one grep apart. This is the support-deflection claim, and it holds.
• Healthy server → Loaded 10 tools from MCP server "data-gouv" in 925ms at Debug, absent at Info, while the failure lines stay visible there (business rules 2 and 3).
• A/B on the 503→200 change — origin/main's fetcher answered 503 and logged failedConfigNames for a reachable zero-tool server; this branch answers 200 {"tools":[]} and logs nothing at Error.
• OAuth retry, against a live OAuth MCP server with its access revoked mid-session: attempt 1 logs the auth failure, attempt 2 succeeds, 200 with 3 tools.

Notes for the reviewer

• Neither log line carries runId / stepId: RemoteToolFetcher and the AiClient are both built once per executor with the process-level logger. Concurrent runs against the same connector are still distinguished only by server name and timestamp. Separate ticket.
• Two manual checks are still open, both needing a harness that doesn't exist yet — the example app never calls addWorkflowExecutor: the embedded-agent rendering of the same failure, and a host logger that throws passed via addWorkflowExecutor({ logger }). The latter is covered by unit test, not end to end.
• The package CLAUDE.md gained the Logging clause and the failures-channel invariant, per that file's own keep-current instruction; the README gained a "When an MCP step fails to load its tools" section.

fixes PRD-876

Note

Surface MCP tool load failure causes in workflow executor logs

• Switches RemoteToolFetcher from loadRemoteTools to loadRemoteToolsWithFailures, so failures are determined from provider-reported data rather than inferred from an absent tool list; an empty tool list from a healthy server is no longer treated as a failure.
• Logs structured failure details (server, kind, error message) via errorOnPartialLoadFailure().
• Adds a new to-ai-proxy-logger bridge that flattens Error objects into readable fields and guards against host logger throws before delegating to the executor logger.
• Wires the executor logger into AiClientAdapter and ServerAiAdapter so ai-proxy internal logs (including tool load diagnostics) are forwarded by default.
• Behavioral Change: OAuth servers with a rejected cached token now trigger a forced token refresh and retry; persistent auth failures throw OAuthReauthRequiredError; a successful retry logs an Info-level recovery message.

^{Macroscope summarized 1c2aabc.}

[linear-code]

PRD-876

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (6)

Rating	File	% Diff	Uncovered Line #s
	packages/workflow-executor/src/build-workflow-executor.ts	100.0%
	packages/workflow-executor/src/adapters/pretty-logger.ts	100.0%
	packages/workflow-executor/src/remote-tool-fetcher.ts	100.0%
	packages/workflow-executor/src/adapters/server-ai-adapter.ts	100.0%
	packages/workflow-executor/src/adapters/ai-client-adapter.ts	100.0%
	packages/workflow-executor/src/adapters/to-ai-proxy-logger.ts	100.0%
	Total	100.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.

[hercemer42]

Automated validator pass (Claude Opus 5, claude-opus-5[1m]) — 6 inline findings: 2 Should fix, 4 Preferential. No approval implied; the spec check against PRD-876 conforms for this PR's half (AC#4, AC#5). Submitted rather than left pending so the findings are visible in the workflow.

[hercemer42]

Claude Opus 5 (claude-opus-5[1m]): review resolution — 3 of 6 findings applied in b7b165c, 3 declined with reasoning in the threads.

Applied

• A throwing host logger could abort a whole tool load. The bridge now guards its body, so logging cannot become control flow at the boundary where a host callback enters ai-proxy. New test covers the cause and no-cause paths.
• The cause chain stopped at the outer message. The payload carries cause, read the way base-step-executor.ts:94 does. New test wraps fetch failed over ECONNREFUSED — the unreachable-server-vs-rejected-token distinction this ticket is buying.
• A test asserted a fallback warning the per-call client cannot emit. Reworded as an explicit wiring check with a neutral message and a comment saying so.

Declined

• Drop logger from the getModel client (server-ai-adapter.ts:41). Inert today, but removing it leaves one AiClient in the class with a logger and one without — the asymmetry that becomes a silent gap the moment that client is reused.
• Recovered-OAuth Error noise. Real and confirmed, but a rejected token is a genuine tool-load failure that business rule 3 wants at Error, and neither fix is reachable without touching mcp-client.ts (business rule 4 forbids it) or the retry state that never reaches this logger.
• Missing runId/stepId, and the stale ecosystem-pack line. Both accurate, neither fixable here: the first needs a design change or the remote-tool-fetcher.ts:105 follow-up, the second lives in the claudine repo.

Checks after the fix: 4 suites / 78 tests green, to-ai-proxy-logger.ts still 100% covered, eslint + prettier + tsc clean.

Paused for a human call on the three declines before this leaves draft.

[qltysh]

All good ✅

[hercemer42]

Claude Opus 5 (claude-opus-5[1m]): review closed out. All six threads answered and resolved; CI green on 16f90896 — the whole rollup, after re-running LLM Integration Tests (ai-proxy), which had failed on an upstream OpenAI 500 for gpt-3.5-turbo in its live-API tool-support test (identical failure on all three commits of this branch, including the first; no ai-proxy file is touched here). Supersedes my earlier resolution comment, which predates the widened scope.

Fixed

• Throwing host logger could abort a whole tool load → the bridge guards its body (b7b165c2).
• Cause chain stopped at the outer message → the payload carries cause, read the way base-step-executor.ts:94 does (b7b165c2).
• A test asserted a message the per-call getModel client cannot emit → reworded as an explicit wiring check (b7b165c2).
• Cause and run context on separate lines → half of this is now moot: remote-tool-fetcher.ts reads the failures channel, so MCP servers failed to load tools carries { server, kind, error } instead of an id-diff (16f90896).

Declined, reasoning in the threads

• Dropping logger from the getModel client: inert today, but removing it leaves one AiClient in the class with a logger and one without.
• Recovered-OAuth Error noise: a rejected token is a genuine tool-load failure that business rule 3 wants at Error, and neither fix is reachable without editing mcp-client.ts (business rule 4) or the retry state.
• Stale ecosystem-pack line: accurate, but it lives in the claudine repo.

Still open, deliberately

• Neither log line carries runId / stepId — RemoteToolFetcher and the AiClient are both built once per executor. Concurrent runs against one connector are distinguished only by server name and timestamp. Own ticket.

Checked and dismissed: whether the failures-channel switch could regress a config map holding two entries under one server id (a partial failure would now 503 where it used to return the working subset). It cannot happen — ai-service.ts:773 merges two disjoint queries on aiMcpConfigs (isForestConnector false/true), each entry's id is its own row id, and a name collision overwrites rather than duplicates. No guard added.

[hercemer42]

Claude Opus 5 (claude-opus-5[1m]): qlty's Function with many returns (count = 4): fetchOAuthTools is fixed in b1a1fe9 — the two tail exits differed only in which attempt produced the tools, so the rejected-token case is named once and the retry's result read once. Three returns now; qlty reports All good ✅.

Worth recording that it wasn't introduced here: origin/main has the same four returns in that function. It surfaced as a new issue because this PR edited the body, and the analyser attributes findings to changed lines. Fixed rather than argued, since it's two lines either way.

[hercemer42]

Claude Opus 5 (claude-opus-5[1m]): manual testing on a live rig found one gap, fixed in db5ba65.

Ran the executor from this branch (pm2 agent-stack, tsx watch cli.ts --pretty) against the OAuth sim, with a credential deposited through POST /mcp-oauth-credentials and the server's access tokens revoked mid-flight. The run recovers — HTTP 200, 3 tools — but the log read as a pure failure:

error Error loading tools for sim-mcp b  error="Authentication failed for HTTP server…"
error Failed to load tools from 1/1 MCP server(s): sim-mcp b: Authentication failed…
debug Loaded 3 tools from MCP server "sim-mcp b" in 26ms      ← the recovery, invisible at Info

The retry that fixes it only logged at Debug, so at the production default an operator saw the two Error lines and nothing saying it continued. RemoteToolFetcher now states the recovery at Info after a forced refresh succeeds:

info  MCP tools loaded after refreshing the credential requestedMcpServerId="34" mcpServerName="sim-mcp b"

This reverses my earlier decline on the recovered-OAuth thread. I argued there that "token rejected at 14:02, refreshed, continued" was information support wants — that was wrong: support only ever saw the rejection. The noise itself stays (the failure is real, and business rule 3 wants it at Error), but it is no longer misleading.

[PMerlet]

Adversarial pass on the fetcher + logger bridge. The OAuth retry logic is sound (and fixes the loadFailed-left-undefined hole main had on the rejected-token path), tests assert full payloads, CI is green. Three inline points below: one contract gap worth addressing before merge (or an explicit follow-up ticket, since the fix lives in ai-proxy), two one-liners in files already touched.

[PMerlet]

The failures channel is only populated by McpClient — in AiClient.loadRemoteToolsWithFailures, a provider without loadToolsWithFailures contributes failures: [], and ForestIntegrationClient doesn't implement it. Worse, its loadTools swallows an unsupported integrationName with a Warn and returns no tools.

So for a Forest connector whose integration name isn't recognized (version drift: the orchestrator advertises an integration this ai-proxy build doesn't support yet), the old id-diff answered loadFailed=true → 503 + Error log, while this now reads as a healthy empty server → 200 {"tools":[]}. That's a different case from the false positive this PR legitimately removes — and it's exactly the drift scenario where you'd want the failure surfaced. (Mitigation: the Warn is at least visible now that the logger is wired — but at Warn, and without the 503.)

Note the unit test that covers this (makeFailure('zendesk-prod', 'unknown', 'Unsupported integration: Zendesk')) mocks a failure the real ai-proxy can never emit for a Forest connector, so it encodes a contract the upstream implementation doesn't honor. The new CLAUDE.md invariant ("never infer failure from absent tools") codifies a rule with this known hole.

Suggested fix: give ForestIntegrationClient a loadToolsWithFailures that pushes a kind: 'unknown' failure from the default: branch of its switch — or a follow-up ticket, since this PR deliberately doesn't touch ai-proxy.

[PMerlet]

failure.error.message drops the cause chain: Node's fetch failed errors carry the actual reason (ECONNREFUSED, …) in .cause, and wrapped infra errors can have an empty .message — the exact cases extractErrorMessage/causeMessage in errors.ts exist for. The bridged ai-proxy line does name the cause, but this structured summary is the greppable line the PR body presents as the deliverable, and here it would say error: "fetch failed" alone.

Suggested change

	error: failure.error.message,
	error: extractErrorMessage(failure.error),
	cause: causeMessage(failure.error),

[PMerlet]

extractErrorMessage only returns undefined for undefined; for null it falls through to String(null) → the string "null". So new Error('x', { cause: null }) emits cause="null" in pretty output — the same artifact this PR removes for undefined, in another guise (the pretty-logger filter also only tests !== undefined).

Suggested change

	cause: extractErrorMessage(cause),
	cause: cause == null ? undefined : extractErrorMessage(cause),