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

packages/workflow-executor/CLAUDE.md

@@ -53,7 +53,8 @@ Front  ◀──▶  Orchestrator  ◀──pull/push──▶  Executor (this p
 - **Boundary validation** — wire/mapper types live in `types/validated/` as zod. Strictness by origin: executor-produced + frontend bodies use `.strict()`; the orchestrator collection schema **strips** unknowns and asserts step-specific props at use-time (resilient to orchestrator drift). Parse failure → `DomainValidationError`/`InvalidStepDefinitionError`. `StepOutcome` is validated only when it arrives via `previousSteps`; executor outputs are trusted by construction.
 - **DatabaseStore** — table `workflow_step_executions` + migration registry namespaced under a schema (default `forest`, override via `DATABASE_SCHEMA`), so a DB shared with the agent/server is safe. The schema is created idempotently at `init()`, but gated on a `pg_namespace` existence probe (not `CREATE SCHEMA IF NOT EXISTS` alone): Postgres checks database-level `CREATE` even for `IF NOT EXISTS`, so probing lets a pre-created schema boot with only schema-level `CREATE`. SQLite (tests) skips schemas. Migrations run behind a **transaction-scoped Postgres advisory lock** (`pg_advisory_xact_lock`, safe behind RDS Proxy / PgBouncer) so HA cold-starts migrate once; migrations are transactional + idempotent. Postgres-only; the lock key is a fixed constant — never change it.
 - **Graceful shutdown** — `stop()` drains in-flight steps (`idle → running → draining → stopped`), `stopTimeoutMs` default 30s, HTTP stays up during drain. Signal handling is the consumer's job.
-- **Logging** — `Logger = (level, message, context?) => void`. `BaseStepExecutor` stamps `logCtx` (runId/stepId/stepIndex/stepType); type-specific ids via `getExtraLogContext()`. `createConsoleLogger`/`createPrettyLogger(minLevel)` factories; CLI level from `LOG_LEVEL` (default `Info`).
+- **Logging** — `Logger = (level, message, context?) => void`. `BaseStepExecutor` stamps `logCtx` (runId/stepId/stepIndex/stepType); type-specific ids via `getExtraLogContext()`. `createConsoleLogger`/`createPrettyLogger(minLevel)` factories; CLI level from `LOG_LEVEL` (default `Info`). ai-proxy's logger takes the cause as a third `Error` argument instead of a context object, so both AI adapters bridge it with `toAiProxyLogger` (flattens to `{ error, cause, stack }` — an `Error`'s own properties are non-enumerable and would vanish from the emitted line — and swallows a throwing host logger, which ai-proxy calls from inside its catch blocks).
+- **MCP load failures come from the `failures` channel** — `RemoteToolFetcher` loads through `loadRemoteToolsWithFailures` and reports what the providers classified (`server`/`kind`/`error`); never infer failure from absent tools, which flags a healthy server exposing none. `loadFailed` drives the 503 on `GET /list-mcp-tools`, so a wrong inference is user-visible.
 - **Config comes from the boundary, never `process.env`** — no executor *config* is read from `process.env` outside `cli-core`: every knob is parsed there (standalone) or injected as an option (`ExecutorOptions` / the agent's `addWorkflowExecutor` options), and the check for a value is `Boolean(options.x)`, not `process.env`. Runtime-mode flags — `NODE_ENV` (forceAiError prod-guard, token-endpoint dev check) and the `OTEL_*` observability vars in `tracing.ts` — are the deliberate exception. This keeps the executor identically configurable standalone and embedded, and testable without mutating env. (Regression fixed once: `FOREST_EXECUTOR_ENCRYPTION_KEY` was read in `crypto/` — now injected via `executorEncryptionKey`.)
 - **AI** — import every AI type (`BaseChatModel`, `DynamicStructuredTool`, `SystemMessage`/`HumanMessage`, `RemoteTool`/`ToolConfig`) from `@forestadmin/ai-proxy`, **not** `@langchain/core` (which is not a dependency). `ExecutionContext.model` is a `BaseChatModel`. The only langchain mention in src is a comment in `cli.ts` about transitively loading `@langchain/openai`.
 

packages/workflow-executor/README.md

@@ -166,6 +166,34 @@ When your workflows use OAuth-protected MCP connectors, the executor stores each
 
 ---
 
+## When an MCP step fails to load its tools
+
+The executor names the reason at `Error`, so it is in your logs at the default level:
+
+```json
+{
+  "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" }
+  ]
+}
+```
+
+`kind` tells you where to look:
+
+- `auth` — the server rejected the credential. Reconnect the connector, or renew its token.
+- `connection` — unreachable, refused, or slower than the 15s per-server load timeout.
+- `unknown` — the server answered but the load failed anyway; the `error` text carries the reason.
+
+A server that answers but exposes no tools is not a failure: you get an empty tool list and no error.
+
+Set `LOG_LEVEL=Debug` to add one line per server with its tool count and load time, which is how you find the connector that is slowing a step down.
+
+---
+
 ## Testing only
 
 The following modes skip the database requirement but are **not suitable for production** — state is lost on restart.

packages/workflow-executor/src/adapters/ai-client-adapter.ts

@@ -1,4 +1,5 @@
 import type { AiModelPort, GetModelOptions } from '../ports/ai-model-port';
+import type { Logger } from '../ports/logger-port';
 import type {
   AiConfiguration,
   BaseChatModel,
@@ -10,13 +11,17 @@ import type {
 import { AiClient } from '@forestadmin/ai-proxy';
 
 import { AiModelPortError, WorkflowExecutorError } from '../errors';
+import toAiProxyLogger from './to-ai-proxy-logger';
 
 export default class AiClientAdapter implements AiModelPort {
   private readonly aiClient: AiClient;
 
-  constructor(aiConfigurations: AiConfiguration[]) {
+  constructor(aiConfigurations: AiConfiguration[], logger?: Logger) {
     const withRetries = aiConfigurations.map(c => ({ maxRetries: 2, ...c }));
-    this.aiClient = new AiClient({ aiConfigurations: withRetries as AiConfiguration[] });
+    this.aiClient = new AiClient({
+      aiConfigurations: withRetries as AiConfiguration[],
+      logger: logger ? toAiProxyLogger(logger) : undefined,
+    });
   }
 
   getModel({ aiConfigName }: GetModelOptions = {}): BaseChatModel {

packages/workflow-executor/src/adapters/pretty-logger.ts

@@ -12,7 +12,11 @@ const LABEL: Record<LoggerLevel, string> = {
 };
 
 function formatContext(context: Record<string, unknown>): string {
-  const parts = Object.entries(context).map(([key, value]) => `${key}=${JSON.stringify(value)}`);
+  // Callers build a fixed context shape and leave the fields they have nothing for undefined,
+  // which JSON.stringify would render as the literal `undefined`.
+  const parts = Object.entries(context)
+    .filter(([, value]) => value !== undefined)
+    .map(([key, value]) => `${key}=${JSON.stringify(value)}`);
   if (parts.length === 0) return '';
 
   return pc.dim(parts.join(' '));

packages/workflow-executor/src/adapters/server-ai-adapter.ts

@@ -1,4 +1,6 @@
+import type { AiProxyLogger } from './to-ai-proxy-logger';
 import type { AiModelPort, GetModelOptions } from '../ports/ai-model-port';
+import type { Logger } from '../ports/logger-port';
 import type {
   AiConfiguration,
   BaseChatModel,
@@ -10,27 +12,33 @@ import type {
 import { AiClient } from '@forestadmin/ai-proxy';
 
 import { AiModelPortError, WorkflowExecutorError } from '../errors';
+import toAiProxyLogger from './to-ai-proxy-logger';
 
 export interface ServerAiAdapterOptions {
   forestServerUrl: string;
   envSecret: string;
+  logger?: Logger;
 }
 
 export default class ServerAiAdapter implements AiModelPort {
   private readonly options: ServerAiAdapterOptions;
+  private readonly aiProxyLogger?: AiProxyLogger;
   private readonly aiClient: AiClient;
 
   constructor(options: ServerAiAdapterOptions) {
     this.options = options;
+    this.aiProxyLogger = options.logger ? toAiProxyLogger(options.logger) : undefined;
     this.aiClient = new AiClient({
       aiConfigurations: [ServerAiAdapter.buildProxyConfiguration(options)],
+      logger: this.aiProxyLogger,
     });
   }
 
   getModel({ userId }: GetModelOptions = {}): BaseChatModel {
     try {
       const client = new AiClient({
         aiConfigurations: [ServerAiAdapter.buildProxyConfiguration(this.options, userId)],
+        logger: this.aiProxyLogger,
       });
 
       return client.getModel();

packages/workflow-executor/src/adapters/to-ai-proxy-logger.ts

@@ -0,0 +1,32 @@
+import type { Logger, LoggerLevel } from '../ports/logger-port';
+
+import { extractErrorMessage } from '../errors';
+
+// ai-proxy hands the cause as an Error, the executor's logger expects a context object.
+export type AiProxyLogger = (level: LoggerLevel, message: string, error?: Error) => void;
+
+// An Error's own properties are non-enumerable, so forwarding it as the context would emit the line
+// with the cause silently stripped — flatten it the way the rest of the executor logs causes.
+export default function toAiProxyLogger(logger: Logger): AiProxyLogger {
+  return (level, message, error) => {
+    // ai-proxy logs from inside its catch blocks before recording the failure it caught, so a host
+    // logger that throws here would abort a whole tool load instead of one server's.
+    try {
+      if (error === undefined || error === null) {
+        logger(level, message);
+
+        return;
+      }
+
+      const { cause } = error as { cause?: unknown };
+
+      logger(level, message, {
+        error: extractErrorMessage(error),
+        cause: extractErrorMessage(cause),
+        stack: error instanceof Error ? error.stack : undefined,
+      });
+    } catch {
+      // A broken logger must not become control flow.
+    }
+  };
+}

packages/workflow-executor/src/build-workflow-executor.ts

@@ -119,9 +119,9 @@ function buildCommonDependencies(options: ExecutorOptions) {
   if (forceAiError) {
     aiModelPort = new AlwaysErrorAiModelPort();
   } else if (options.aiConfigurations?.length) {
-    aiModelPort = new AiClientAdapter(options.aiConfigurations);
+    aiModelPort = new AiClientAdapter(options.aiConfigurations, logger);
   } else {
-    aiModelPort = new ServerAiAdapter({ forestServerUrl, envSecret: options.envSecret });
+    aiModelPort = new ServerAiAdapter({ forestServerUrl, envSecret: options.envSecret, logger });
   }
 
   // A TTL of 0/negative/non-finite would silently make the cache always-stale, so fall back.

packages/workflow-executor/src/ports/ai-model-port.ts

@@ -13,8 +13,8 @@ export interface GetModelOptions {
 export interface AiModelPort {
   getModel(options?: GetModelOptions): BaseChatModel;
   loadRemoteTools(configs: Record<string, ToolConfig>): Promise<RemoteTool[]>;
-  // Loads tools and exposes per-server failures classified by cause (auth vs connection), so the
-  // OAuth path can tell a revoked token from an unreachable server. Default consumers use loadRemoteTools.
+  // Loads tools and exposes per-server failures classified by cause (auth vs connection), so a
+  // caller can tell a revoked token from an unreachable server and name it in its logs.
   loadRemoteToolsWithFailures(
     configs: Record<string, ToolConfig>,
   ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }>;

packages/workflow-executor/src/remote-tool-fetcher.ts

@@ -2,14 +2,18 @@ import type OAuthTokenService from './oauth/token-service';
 import type { AiModelPort } from './ports/ai-model-port';
 import type { Logger } from './ports/logger-port';
 import type { WorkflowPort } from './ports/workflow-port';
-import type { RemoteTool, ToolConfig } from '@forestadmin/ai-proxy';
+import type { McpServerLoadFailure, RemoteTool, ToolConfig } from '@forestadmin/ai-proxy';
 
 import { injectOauthTokens } from '@forestadmin/ai-proxy';
 
 import { OAuthReauthRequiredError } from './errors';
 
 const OAUTH2_AUTH_TYPE = 'oauth2';
 
+function hasAuthFailure(failures: McpServerLoadFailure[]): boolean {
+  return failures.some(failure => failure.kind === 'auth');
+}
+
 // Match by config.id, not by Record key: server names can collide across configs.
 export function scopeConfigsToServer(
   configs: Record<string, ToolConfig>,
@@ -71,8 +75,8 @@ export default class RemoteToolFetcher {
       return this.fetchOAuthTools(scoped, mcpServerName, mcpServerId, userId);
     }
 
-    const tools = await this.aiModelPort.loadRemoteTools(scoped);
-    const loadFailed = this.errorOnPartialLoadFailure(scoped, tools, mcpServerId, mcpServerName);
+    const { tools, failures } = await this.aiModelPort.loadRemoteToolsWithFailures(scoped);
+    const loadFailed = this.errorOnPartialLoadFailure(failures, mcpServerId, mcpServerName);
 
     return { tools, mcpServerName, loadFailed };
   }
@@ -90,7 +94,7 @@ export default class RemoteToolFetcher {
 
     const attemptLoad = async (
       forceRefresh: boolean,
-    ): Promise<{ tools: RemoteTool[]; hasAuthFailure: boolean }> => {
+    ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }> => {
       const token = await tokenService.getAccessToken(userId, mcpServerId, { forceRefresh });
       const bearer = `Bearer ${token}`;
       // All scoped configs share this mcpServerId, so inject the token for every one — not just the
@@ -102,33 +106,40 @@ export default class RemoteToolFetcher {
             Object.keys(scoped).map(name => [name, bearer]),
           ),
         }) ?? scoped;
-      const { tools, failures } = await this.aiModelPort.loadRemoteToolsWithFailures(injected);
 
-      return { tools, hasAuthFailure: failures.some(failure => failure.kind === 'auth') };
+      return this.aiModelPort.loadRemoteToolsWithFailures(injected);
     };
 
+    // The reload hook is handed to the caller for its own post-401 retry, so it cannot widen its
+    // return type; it reports the retry's outcome here instead.
+    let reloadFailed: boolean | undefined;
+
     const reloadWithFreshAuth = async (): Promise<RemoteTool[]> => {
       const attempt = await attemptLoad(true);
-      if (attempt.hasAuthFailure) throw new OAuthReauthRequiredError(mcpServerId);
-      this.errorOnPartialLoadFailure(scoped, attempt.tools, mcpServerId, mcpServerName);
+      if (hasAuthFailure(attempt.failures)) throw new OAuthReauthRequiredError(mcpServerId);
+      reloadFailed = this.errorOnPartialLoadFailure(attempt.failures, mcpServerId, mcpServerName);
+
+      // The rejected credential was already logged at Error. Without this the default level shows
+      // the failure and never says it recovered.
+      if (!reloadFailed) {
+        this.logger('Info', 'MCP tools loaded after refreshing the credential', {
+          requestedMcpServerId: mcpServerId,
+          mcpServerName,
+        });
+      }
 
       return attempt.tools;
     };
 
     const initial = await attemptLoad(false);
-
-    if (initial.hasAuthFailure) {
-      return { tools: await reloadWithFreshAuth(), mcpServerName, reloadWithFreshAuth };
-    }
-
-    const loadFailed = this.errorOnPartialLoadFailure(
-      scoped,
-      initial.tools,
-      mcpServerId,
-      mcpServerName,
-    );
-
-    return { tools: initial.tools, mcpServerName, reloadWithFreshAuth, loadFailed };
+    const hasRejectedToken = hasAuthFailure(initial.failures);
+    // The retry supersedes a rejected cached token, so its outcome is the one that counts.
+    const tools = hasRejectedToken ? await reloadWithFreshAuth() : initial.tools;
+    const loadFailed = hasRejectedToken
+      ? reloadFailed
+      : this.errorOnPartialLoadFailure(initial.failures, mcpServerId, mcpServerName);
+
+    return { tools, mcpServerName, reloadWithFreshAuth, loadFailed };
   }
 
   // Distinguish "no configs at all" (deployment misconfig) from "configs exist but none match"
@@ -155,26 +166,23 @@ export default class RemoteToolFetcher {
     );
   }
 
-  // Partial-failure detection: McpClient swallows per-server load errors and returns whatever
-  // succeeded. Match config.id against tool.mcpServerId — both providers populate it from the
-  // orchestrator's persisted id, so the check is uniform across MCP and Forest connectors.
+  // Inferring failure from absent tools would flag a healthy server that exposes none, and could
+  // never name a cause.
   private errorOnPartialLoadFailure(
-    scoped: Record<string, ToolConfig>,
-    tools: RemoteTool[],
+    failures: McpServerLoadFailure[],
     mcpServerId: string,
     mcpServerName: string | undefined,
   ): boolean {
-    const loadedMcpServerIds = new Set(tools.map(t => t.mcpServerId));
-    const failedConfigNames = Object.entries(scoped)
-      .filter(([, cfg]) => !loadedMcpServerIds.has(cfg.id))
-      .map(([name]) => name);
-
-    if (failedConfigNames.length === 0) return false;
+    if (failures.length === 0) return false;
 
     this.logger('Error', 'MCP servers failed to load tools', {
       requestedMcpServerId: mcpServerId,
       mcpServerName,
-      failedConfigNames,
+      failures: failures.map(failure => ({
+        server: failure.server,
+        kind: failure.kind,
+        error: failure.error.message,
+      })),
     });
 
     return true;

packages/workflow-executor/test/adapters/ai-client-adapter.test.ts

@@ -1,17 +1,25 @@
+import type { AiProxyLogger } from '../../src/adapters/to-ai-proxy-logger';
+import type { Logger } from '../../src/ports/logger-port';
+
 import AiClientAdapter from '../../src/adapters/ai-client-adapter';
 
 const mockGetModel = jest.fn().mockReturnValue({ invoke: jest.fn() });
 const mockLoadRemoteTools = jest.fn().mockResolvedValue([]);
 const mockLoadRemoteToolsWithFailures = jest.fn().mockResolvedValue({ tools: [], failures: [] });
 const mockCloseConnections = jest.fn().mockResolvedValue(undefined);
+const mockAiClientConstructor = jest.fn();
 
 jest.mock('@forestadmin/ai-proxy', () => ({
-  AiClient: jest.fn().mockImplementation(() => ({
-    getModel: mockGetModel,
-    loadRemoteTools: mockLoadRemoteTools,
-    loadRemoteToolsWithFailures: mockLoadRemoteToolsWithFailures,
-    closeConnections: mockCloseConnections,
-  })),
+  AiClient: jest.fn().mockImplementation((...args: unknown[]) => {
+    mockAiClientConstructor(...args);
+
+    return {
+      getModel: mockGetModel,
+      loadRemoteTools: mockLoadRemoteTools,
+      loadRemoteToolsWithFailures: mockLoadRemoteToolsWithFailures,
+      closeConnections: mockCloseConnections,
+    };
+  }),
 }));
 
 describe('AiClientAdapter', () => {
@@ -62,4 +70,30 @@ describe('AiClientAdapter', () => {
 
     expect(mockCloseConnections).toHaveBeenCalled();
   });
+
+  describe('logger', () => {
+    const buildAdapter = (logger?: Logger) => new AiClientAdapter([], logger);
+
+    const aiProxyLoggerGivenToClient = () =>
+      (mockAiClientConstructor.mock.calls[0][0] as { logger?: AiProxyLogger }).logger;
+
+    it("routes ai-proxy's MCP diagnostics to the executor logger with the cause flattened", () => {
+      const executorLogger = jest.fn();
+      buildAdapter(executorLogger);
+      const cause = new Error('401 Unauthorized');
+
+      aiProxyLoggerGivenToClient()?.('Error', 'Error loading tools for notion', cause);
+
+      expect(executorLogger).toHaveBeenCalledWith('Error', 'Error loading tools for notion', {
+        error: '401 Unauthorized',
+        stack: cause.stack,
+      });
+    });
+
+    it('leaves AiClient without a logger when the adapter is built without one', () => {
+      buildAdapter();
+
+      expect(aiProxyLoggerGivenToClient()).toBeUndefined();
+    });
+  });
 });

packages/workflow-executor/test/adapters/pretty-logger.test.ts

@@ -45,6 +45,20 @@ describe('createPrettyLogger', () => {
       expect(output).toMatch(/^\d{2}:\d{2}:\d{2} info {2}Ready$/);
     });
 
+    it('drops keys whose value is undefined, keeping the rest', () => {
+      logger('Info', 'Tools loaded', { server: 'acme-crm', cause: undefined, stack: undefined });
+
+      const output = stripAnsi(infoSpy.mock.calls[0][0] as string);
+      expect(output).toMatch(/^\d{2}:\d{2}:\d{2} info {2}Tools loaded server="acme-crm"$/);
+    });
+
+    it('omits the context chunk when every value is undefined', () => {
+      logger('Info', 'Ready', { cause: undefined });
+
+      const output = stripAnsi(infoSpy.mock.calls[0][0] as string);
+      expect(output).toMatch(/^\d{2}:\d{2}:\d{2} info {2}Ready$/);
+    });
+
     it('JSON-quotes string values in context', () => {
       logger('Info', 'Step execution started', { runId: '42', stepIndex: 2 });