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 change worth a reviewer's eye

Reading the failures channel removes a false positive the id-diff could not avoid: a healthy server that exposes no tools 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.

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; a healthy server exposing no tools is not a failure.
• 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.
• 10 suites / 306 tests green. ai-proxy's own suite runs unchanged and green (38 suites / 429 tests) — AC#5.

Notes for the reviewer

• The per-server Loaded N tools … in Yms line is Debug, so it needs LOG_LEVEL=debug; failure lines are visible at the default Info (business rules 2 and 3).
• 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.
• Not runtime-verified against a live executor — booting one needs a live orchestrator plus agent, so the emitted payloads are proven by piping the wrapper through the real createConsoleLogger().
• The package CLAUDE.md Logging bullet gained one clause, per that file's own keep-current instruction.

fixes PRD-876

[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.

[macroscopeapp]

🟠 High src/remote-tool-fetcher.ts:113

fetchOAuthTools discards non-auth failures from the forced-refresh retry: when the cached token is rejected and the retry then fails with a connection error, loadFailed is set to undefined, so the caller gets a 200 with an empty tool list instead of the intended 503. The retry's failure is logged but never propagated as loadFailed, and the success log "tools loaded after refreshing" is emitted even though the retry produced no tools. Consider preserving the retry's failure state by setting loadFailed from reloadWithFreshAuth instead of hardcoding undefined.

     const reloadWithFreshAuth = async (): Promise<RemoteTool[]> => {
       const attempt = await attemptLoad(true);
       if (hasAuthFailure(attempt.failures)) throw new OAuthReauthRequiredError(mcpServerId);
-      this.errorOnPartialLoadFailure(attempt.failures, mcpServerId, mcpServerName);
+      if (attempt.failures.length > 0) {
+        this.errorOnPartialLoadFailure(attempt.failures, mcpServerId, mcpServerName);
+        throw new Error(`MCP server failed to load after credential refresh (mcpServerId="${mcpServerId}")`);
+      }
       // The rejected credential was already logged at Error. Without this the default level shows
       // the failure and never says it recovered.
       this.logger('Info', 'MCP tools loaded after refreshing the credential', {
         requestedMcpServerId: mcpServerId,
         mcpServerName,
       });

       return attempt.tools;
     };

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/remote-tool-fetcher.ts around lines 113-125:

`fetchOAuthTools` discards non-auth failures from the forced-refresh retry: when the cached token is rejected and the retry then fails with a connection error, `loadFailed` is set to `undefined`, so the caller gets a 200 with an empty tool list instead of the intended 503. The retry's failure is logged but never propagated as `loadFailed`, and the success log "tools loaded after refreshing" is emitted even though the retry produced no tools. Consider preserving the retry's failure state by setting `loadFailed` from `reloadWithFreshAuth` instead of hardcoding `undefined`.