diff --git a/packages/ai-proxy/src/forest-integration-client.ts b/packages/ai-proxy/src/forest-integration-client.ts index 1ffb1e021b..06c0321310 100644 --- a/packages/ai-proxy/src/forest-integration-client.ts +++ b/packages/ai-proxy/src/forest-integration-client.ts @@ -1,3 +1,4 @@ +import type { McpServerLoadFailure } from './mcp-client'; import type RemoteTool from './remote-tool'; import type { ToolProvider } from './tool-provider'; import type { Logger } from '@forestadmin/datasource-toolkit'; @@ -38,8 +39,12 @@ export default class ForestIntegrationClient implements ToolProvider { this.configs = configs; } - async loadTools(): Promise { + async loadToolsWithFailures(): Promise<{ + tools: RemoteTool[]; + failures: McpServerLoadFailure[]; + }> { const tools: RemoteTool[] = []; + const failures: McpServerLoadFailure[] = []; this.configs.forEach(({ id: mcpServerId, integrationName, config }) => { switch (integrationName) { @@ -54,10 +59,22 @@ export default class ForestIntegrationClient implements ToolProvider { break; default: this.logger?.('Warn', `Unsupported integration: ${integrationName}`); + // Reporting it is what stops a caller reading an integration this build doesn't know as + // a healthy connector that publishes nothing. + failures.push({ + server: integrationName, + mcpServerId, + kind: 'unknown', + error: new Error(`Unsupported integration: ${integrationName}`), + }); } }); - return tools; + return { tools, failures }; + } + + async loadTools(): Promise { + return (await this.loadToolsWithFailures()).tools; } async checkConnection(): Promise { diff --git a/packages/ai-proxy/test/forest-integration-client.test.ts b/packages/ai-proxy/test/forest-integration-client.test.ts index 104fb6f80d..414df2a3ef 100644 --- a/packages/ai-proxy/test/forest-integration-client.test.ts +++ b/packages/ai-proxy/test/forest-integration-client.test.ts @@ -126,6 +126,63 @@ describe('ForestIntegrationClient', () => { }); }); + describe('loadToolsWithFailures', () => { + it('reports an unsupported integration as a failure carrying its name and id', async () => { + const client = new ForestIntegrationClient( + // @ts-expect-error Testing unsupported integration + [{ id: '7', integrationName: 'unknown', config: {} as any, isForestConnector: true }], + ); + + const { tools, failures } = await client.loadToolsWithFailures(); + + expect(tools).toEqual([]); + expect(failures).toEqual([ + { + server: 'unknown', + mcpServerId: '7', + kind: 'unknown', + error: new Error('Unsupported integration: unknown'), + }, + ]); + }); + + it('reports no failure when every integration is supported', async () => { + const client = new ForestIntegrationClient([ + { + id: '1', + integrationName: 'Zendesk', + config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, + isForestConnector: true, + }, + ]); + + const { tools, failures } = await client.loadToolsWithFailures(); + + expect(tools).toEqual(mockZendeskTools); + expect(failures).toEqual([]); + }); + + // A supported connector alongside a broken one must still contribute its tools. + it('keeps a supported integration tools when another one is unsupported', async () => { + const client = new ForestIntegrationClient([ + { + id: '1', + integrationName: 'Zendesk', + config: { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }, + isForestConnector: true, + }, + // @ts-expect-error Testing unsupported integration + { id: '2', integrationName: 'unknown', config: {} as any, isForestConnector: true }, + ]); + + const { tools, failures } = await client.loadToolsWithFailures(); + + expect(tools).toEqual(mockZendeskTools); + expect(failures).toHaveLength(1); + expect(failures[0].mcpServerId).toBe('2'); + }); + }); + describe('checkConnection', () => { it('should call validateZendeskConfig for Zendesk integration', async () => { const zendeskConfig = { subdomain: 'test', email: 'a@b.com', apiToken: 'tok' }; diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index c0070b30f4..77ea807720 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/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`. diff --git a/packages/workflow-executor/README.md b/packages/workflow-executor/README.md index bf15b324a9..eb5c4797dc 100644 --- a/packages/workflow-executor/README.md +++ b/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 (HTTP 401). On an OAuth2 connector the executor refreshes the token and retries once on its own, so this line concerns static credentials; an OAuth2 connector that recovered logs `MCP tools loaded after refreshing the credential` at `Info`, and one that cannot pauses the run for re-authentication instead. +- `connection` — unreachable, refused, or slower than the 15s per-server load timeout. +- `unknown` — the server answered but the load failed anyway, including HTTP 403 (the credential is valid but lacks the permission, which no refresh can fix) and an integration this build does not support; 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. diff --git a/packages/workflow-executor/src/adapters/ai-client-adapter.ts b/packages/workflow-executor/src/adapters/ai-client-adapter.ts index 43369caf16..1fb47481f1 100644 --- a/packages/workflow-executor/src/adapters/ai-client-adapter.ts +++ b/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 { diff --git a/packages/workflow-executor/src/adapters/pretty-logger.ts b/packages/workflow-executor/src/adapters/pretty-logger.ts index 7f845c9ab3..4b7091b72b 100644 --- a/packages/workflow-executor/src/adapters/pretty-logger.ts +++ b/packages/workflow-executor/src/adapters/pretty-logger.ts @@ -12,7 +12,11 @@ const LABEL: Record = { }; function formatContext(context: Record): 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(' ')); diff --git a/packages/workflow-executor/src/adapters/server-ai-adapter.ts b/packages/workflow-executor/src/adapters/server-ai-adapter.ts index 2d05307d30..6932553771 100644 --- a/packages/workflow-executor/src/adapters/server-ai-adapter.ts +++ b/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,20 +12,25 @@ 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, }); } @@ -31,6 +38,7 @@ export default class ServerAiAdapter implements AiModelPort { try { const client = new AiClient({ aiConfigurations: [ServerAiAdapter.buildProxyConfiguration(this.options, userId)], + logger: this.aiProxyLogger, }); return client.getModel(); diff --git a/packages/workflow-executor/src/adapters/to-ai-proxy-logger.ts b/packages/workflow-executor/src/adapters/to-ai-proxy-logger.ts new file mode 100644 index 0000000000..8f46201571 --- /dev/null +++ b/packages/workflow-executor/src/adapters/to-ai-proxy-logger.ts @@ -0,0 +1,34 @@ +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), + // `extractErrorMessage` only short-circuits on undefined, so a null cause would print + // the string "null" — the artifact the pretty logger drops undefined keys to avoid. + cause: cause == null ? undefined : extractErrorMessage(cause), + stack: error instanceof Error ? error.stack : undefined, + }); + } catch { + // A broken logger must not become control flow. + } + }; +} diff --git a/packages/workflow-executor/src/build-workflow-executor.ts b/packages/workflow-executor/src/build-workflow-executor.ts index a440bac6d8..3096427d7a 100644 --- a/packages/workflow-executor/src/build-workflow-executor.ts +++ b/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. diff --git a/packages/workflow-executor/src/ports/ai-model-port.ts b/packages/workflow-executor/src/ports/ai-model-port.ts index 30d0f18abf..06a193914d 100644 --- a/packages/workflow-executor/src/ports/ai-model-port.ts +++ b/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): Promise; - // 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, ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }>; diff --git a/packages/workflow-executor/src/remote-tool-fetcher.ts b/packages/workflow-executor/src/remote-tool-fetcher.ts index 21c854a690..d64c9379e8 100644 --- a/packages/workflow-executor/src/remote-tool-fetcher.ts +++ b/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'; +import { OAuthReauthRequiredError, causeMessage, extractErrorMessage } 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, @@ -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 => { 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,26 @@ 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, - 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, + // Read the way the bridge does: a `fetch failed` keeps its ECONNREFUSED, and a wrapped + // infra error with an empty message still names something. + error: extractErrorMessage(failure.error), + cause: causeMessage(failure.error), + })), }); return true; diff --git a/packages/workflow-executor/test/adapters/ai-client-adapter.test.ts b/packages/workflow-executor/test/adapters/ai-client-adapter.test.ts index 5dee091eac..b7a0246d89 100644 --- a/packages/workflow-executor/test/adapters/ai-client-adapter.test.ts +++ b/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(); + }); + }); }); diff --git a/packages/workflow-executor/test/adapters/pretty-logger.test.ts b/packages/workflow-executor/test/adapters/pretty-logger.test.ts index 8f9ffe74af..5c777562dd 100644 --- a/packages/workflow-executor/test/adapters/pretty-logger.test.ts +++ b/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 }); diff --git a/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts b/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts index 42b917973e..b1a7fe9e54 100644 --- a/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts +++ b/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts @@ -1,3 +1,6 @@ +import type { AiProxyLogger } from '../../src/adapters/to-ai-proxy-logger'; +import type { Logger } from '../../src/ports/logger-port'; + import ServerAiAdapter from '../../src/adapters/server-ai-adapter'; const mockGetModel = jest.fn().mockReturnValue({ id: 'fake-model' }); @@ -152,4 +155,52 @@ describe('ServerAiAdapter', () => { expect(mockCloseConnections).toHaveBeenCalled(); }); }); + + describe('logger', () => { + const buildAdapter = (logger?: Logger) => + new ServerAiAdapter({ + forestServerUrl: 'https://api.forestadmin.com', + envSecret: ENV_SECRET, + logger, + }); + + const aiProxyLoggerGivenToLatestClient = () => { + const { calls } = mockAiClientConstructor.mock; + + return (calls[calls.length - 1][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'); + + aiProxyLoggerGivenToLatestClient()?.('Error', 'Error loading tools for notion', cause); + + expect(executorLogger).toHaveBeenCalledWith('Error', 'Error loading tools for notion', { + error: '401 Unauthorized', + stack: cause.stack, + }); + }); + + // Wiring only: the captured logger is invoked directly, since a client built for a single + // unnamed configuration reaches none of AiClient's own emit sites. + it('wires the same logger into the per-call AiClient built by getModel', () => { + const executorLogger = jest.fn(); + buildAdapter(executorLogger).getModel({ userId: 42 }); + + aiProxyLoggerGivenToLatestClient()?.('Warn', 'Error during remote tool connection cleanup'); + + expect(executorLogger).toHaveBeenCalledWith( + 'Warn', + 'Error during remote tool connection cleanup', + ); + }); + + it('leaves AiClient without a logger when none is configured', () => { + buildAdapter(); + + expect(aiProxyLoggerGivenToLatestClient()).toBeUndefined(); + }); + }); }); diff --git a/packages/workflow-executor/test/adapters/to-ai-proxy-logger.test.ts b/packages/workflow-executor/test/adapters/to-ai-proxy-logger.test.ts new file mode 100644 index 0000000000..4e933f588f --- /dev/null +++ b/packages/workflow-executor/test/adapters/to-ai-proxy-logger.test.ts @@ -0,0 +1,167 @@ +import createConsoleLogger from '../../src/adapters/console-logger'; +import toAiProxyLogger from '../../src/adapters/to-ai-proxy-logger'; + +describe('toAiProxyLogger', () => { + let logger: jest.Mock; + let aiProxyLogger: ReturnType; + + beforeEach(() => { + logger = jest.fn(); + aiProxyLogger = toAiProxyLogger(logger); + }); + + describe('without a cause', () => { + it('forwards every level and message unchanged, adding no context', () => { + aiProxyLogger('Debug', 'Loaded 3 tools from MCP server "notion" in 12ms'); + aiProxyLogger('Info', 'Using AI configuration default'); + aiProxyLogger('Warn', 'Unsupported integration: stripe'); + aiProxyLogger('Error', 'Error during tool provider cleanup'); + + expect(logger.mock.calls).toEqual([ + ['Debug', 'Loaded 3 tools from MCP server "notion" in 12ms'], + ['Info', 'Using AI configuration default'], + ['Warn', 'Unsupported integration: stripe'], + ['Error', 'Error during tool provider cleanup'], + ]); + }); + }); + + describe('with an Error cause', () => { + it('flattens the cause into enumerable error and stack context', () => { + const cause = new Error('401 Unauthorized'); + + aiProxyLogger('Error', 'Error loading tools for notion', cause); + + expect(logger).toHaveBeenCalledWith('Error', 'Error loading tools for notion', { + error: '401 Unauthorized', + stack: cause.stack, + }); + }); + + // A raw Error spread into the context emits nothing: its own properties are non-enumerable. + it('keeps the cause readable in the default-level console logger output', () => { + const spy = jest.spyOn(console, 'error').mockImplementation(); + + toAiProxyLogger(createConsoleLogger())( + 'Error', + 'Error loading tools for notion', + new Error('401 Unauthorized'), + ); + + const output = JSON.parse(spy.mock.calls[0][0]); + expect(output).toMatchObject({ + level: 'Error', + message: 'Error loading tools for notion', + error: '401 Unauthorized', + }); + expect(output.stack).toContain('Error: 401 Unauthorized'); + + spy.mockRestore(); + }); + + // A wrapped `TypeError: fetch failed` is the difference between "server unreachable" and "401". + it('reports the cause of a wrapped error alongside its own message', () => { + const wrapped = Object.assign(new Error('fetch failed'), { + cause: new Error('ECONNREFUSED 127.0.0.1:9100'), + }); + + aiProxyLogger('Error', 'Error loading tools for notion', wrapped); + + expect(logger).toHaveBeenCalledWith('Error', 'Error loading tools for notion', { + error: 'fetch failed', + cause: 'ECONNREFUSED 127.0.0.1:9100', + stack: wrapped.stack, + }); + }); + + // `extractErrorMessage` stringifies anything non-undefined, so an explicit null would print + // the word "null" and survive the pretty logger's undefined-only filter. + it('omits the cause key when the error carries an explicit null cause', () => { + const cause = Object.assign(new Error('boom'), { cause: null }); + + aiProxyLogger('Error', 'Error loading tools for notion', cause); + + expect(logger).toHaveBeenCalledWith('Error', 'Error loading tools for notion', { + error: 'boom', + cause: undefined, + stack: cause.stack, + }); + }); + + it('reports a stackless Error by its message alone', () => { + const cause = new Error('boom'); + delete cause.stack; + + aiProxyLogger('Error', 'Error loading tools for notion', cause); + + expect(logger).toHaveBeenCalledWith('Error', 'Error loading tools for notion', { + error: 'boom', + stack: undefined, + }); + }); + + it('falls back to the error name when the Error carries an empty message', () => { + aiProxyLogger('Error', 'Error loading tools for notion', new Error('')); + + expect(logger).toHaveBeenCalledWith( + 'Error', + 'Error loading tools for notion', + expect.objectContaining({ error: 'Error' }), + ); + }); + + it('keeps each call independent when several servers fail in the same load', () => { + aiProxyLogger('Error', 'Error loading tools for notion', new Error('401 Unauthorized')); + aiProxyLogger('Error', 'Error loading tools for jira', new Error('ECONNREFUSED')); + aiProxyLogger('Warn', 'Unsupported integration: stripe'); + + expect(logger.mock.calls).toEqual([ + [ + 'Error', + 'Error loading tools for notion', + { error: '401 Unauthorized', stack: expect.any(String) }, + ], + [ + 'Error', + 'Error loading tools for jira', + { error: 'ECONNREFUSED', stack: expect.any(String) }, + ], + ['Warn', 'Unsupported integration: stripe'], + ]); + }); + }); + + // A throw escaping here fails a whole tool load — including the OAuth reauth path. + describe('when the host logger throws', () => { + it('keeps the throw away from ai-proxy, with and without a cause', () => { + const throwing = jest.fn(() => { + throw new Error('host logger exploded'); + }); + const guarded = toAiProxyLogger(throwing); + + expect(() => + guarded('Error', 'Error loading tools for notion', new Error('401 Unauthorized')), + ).not.toThrow(); + expect(() => guarded('Warn', 'Unsupported integration: stripe')).not.toThrow(); + expect(throwing).toHaveBeenCalledTimes(2); + }); + }); + + describe('with a cause that is not an Error', () => { + // ai-proxy casts what it catches (`error as Error`), so any thrown value reaches the wrapper. + it('stringifies the thrown value and reports no stack', () => { + aiProxyLogger('Error', 'Error loading tools for notion', 'kaboom' as unknown as Error); + + expect(logger).toHaveBeenCalledWith('Error', 'Error loading tools for notion', { + error: 'kaboom', + stack: undefined, + }); + }); + + it('treats a null cause as no cause instead of logging "null"', () => { + aiProxyLogger('Error', 'Error loading tools for notion', null as unknown as Error); + + expect(logger).toHaveBeenCalledWith('Error', 'Error loading tools for notion'); + }); + }); +}); diff --git a/packages/workflow-executor/test/build-workflow-executor.test.ts b/packages/workflow-executor/test/build-workflow-executor.test.ts index da5421dcb1..71f187f3db 100644 --- a/packages/workflow-executor/test/build-workflow-executor.test.ts +++ b/packages/workflow-executor/test/build-workflow-executor.test.ts @@ -136,7 +136,10 @@ describe('buildInMemoryExecutor', () => { buildInMemoryExecutor(BASE_OPTIONS); - expect(AiClientAdapter).toHaveBeenCalledWith(BASE_OPTIONS.aiConfigurations); + expect(AiClientAdapter).toHaveBeenCalledWith( + BASE_OPTIONS.aiConfigurations, + expect.any(Function), + ); }); it('creates ServerAiAdapter when aiConfigurations is not provided', () => { @@ -149,6 +152,33 @@ describe('buildInMemoryExecutor', () => { expect(ServerAiAdapter).toHaveBeenCalledWith({ forestServerUrl: 'https://api.forestadmin.com', envSecret: BASE_OPTIONS.envSecret, + logger: expect.any(Function), + }); + }); + + // ai-proxy holds the host logger optionally and no-ops every emit without one. + it('gives AiClientAdapter the executor logger', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires, global-require + const AiClientAdapter = require('../src/adapters/ai-client-adapter').default; + const logger = jest.fn(); + + buildInMemoryExecutor({ ...BASE_OPTIONS, logger }); + + expect(AiClientAdapter).toHaveBeenCalledWith(BASE_OPTIONS.aiConfigurations, logger); + }); + + it('gives ServerAiAdapter the executor logger', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires, global-require + const ServerAiAdapter = require('../src/adapters/server-ai-adapter').default; + const logger = jest.fn(); + + const { aiConfigurations, ...optionsWithoutAi } = BASE_OPTIONS; + buildInMemoryExecutor({ ...optionsWithoutAi, logger }); + + expect(ServerAiAdapter).toHaveBeenCalledWith({ + forestServerUrl: 'https://api.forestadmin.com', + envSecret: BASE_OPTIONS.envSecret, + logger, }); }); @@ -176,7 +206,10 @@ describe('buildInMemoryExecutor', () => { buildInMemoryExecutor({ ...BASE_OPTIONS, forceAiError: true }); expect(AlwaysErrorAiModelPort).not.toHaveBeenCalled(); - expect(AiClientAdapter).toHaveBeenCalledWith(BASE_OPTIONS.aiConfigurations); + expect(AiClientAdapter).toHaveBeenCalledWith( + BASE_OPTIONS.aiConfigurations, + expect.any(Function), + ); } finally { process.env.NODE_ENV = original; } diff --git a/packages/workflow-executor/test/integration/workflow-execution.test.ts b/packages/workflow-executor/test/integration/workflow-execution.test.ts index 0593ac5f10..d40b5af2ae 100644 --- a/packages/workflow-executor/test/integration/workflow-execution.test.ts +++ b/packages/workflow-executor/test/integration/workflow-execution.test.ts @@ -140,6 +140,7 @@ function createMockAiClient(model: BaseChatModel): AiModelPort { return { getModel: jest.fn().mockReturnValue(model), loadRemoteTools: jest.fn().mockResolvedValue([]), + loadRemoteToolsWithFailures: jest.fn().mockResolvedValue({ tools: [], failures: [] }), closeConnections: jest.fn().mockResolvedValue(undefined), } as unknown as AiModelPort; } @@ -619,7 +620,10 @@ describe('workflow execution (integration)', () => { ); const aiClient = createMockAiClient(model); - (aiClient.loadRemoteTools as jest.Mock).mockResolvedValue([fakeRemoteTool]); + (aiClient.loadRemoteToolsWithFailures as jest.Mock).mockResolvedValue({ + tools: [fakeRemoteTool], + failures: [], + }); const step = buildPendingStep({ stepDefinition: { @@ -635,7 +639,7 @@ describe('workflow execution (integration)', () => { .fn() .mockResolvedValue({ step, auth: { forestServerToken: 'test-forest-token' } }), // Two configs but only one matches step.mcpServerId — the assertion below proves - // RemoteToolFetcher actually scopes the Record before calling loadRemoteTools. + // RemoteToolFetcher actually scopes the Record before loading tools. getMcpServerConfigs: jest.fn().mockResolvedValue({ 'mcp-server-1': { id: 'mcp-1', url: 'http://fake' }, 'mcp-server-2': { id: 'mcp-2', url: 'http://other' }, @@ -676,7 +680,7 @@ describe('workflow execution (integration)', () => { expect.objectContaining({ type: 'mcp', status: 'success' }), ); // Scoping must reach the AI port — only the matching server is forwarded, not the full map. - expect(aiClient.loadRemoteTools).toHaveBeenCalledWith({ + expect(aiClient.loadRemoteToolsWithFailures).toHaveBeenCalledWith({ 'mcp-server-1': expect.objectContaining({ id: 'mcp-1' }), }); }); diff --git a/packages/workflow-executor/test/remote-tool-fetcher.test.ts b/packages/workflow-executor/test/remote-tool-fetcher.test.ts index 2da8bfe4a9..69562e82a4 100644 --- a/packages/workflow-executor/test/remote-tool-fetcher.test.ts +++ b/packages/workflow-executor/test/remote-tool-fetcher.test.ts @@ -2,7 +2,7 @@ import type OAuthTokenService from '../src/oauth/token-service'; import type { AiModelPort } from '../src/ports/ai-model-port'; import type { Logger } from '../src/ports/logger-port'; import type { WorkflowPort } from '../src/ports/workflow-port'; -import type { RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; +import type { McpServerLoadFailure, RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; import { OAuthReauthRequiredError } from '../src/errors'; import RemoteToolFetcher, { scopeConfigsToServer } from '../src/remote-tool-fetcher'; @@ -30,6 +30,14 @@ function makeRemoteTool(sourceId: string, mcpServerId?: string): RemoteTool { return { sourceId, mcpServerId } as unknown as RemoteTool; } +function makeFailure(server: string, kind: string, message: string): McpServerLoadFailure { + return { server, kind, error: new Error(message) } as McpServerLoadFailure; +} + +function loadsWithFailures(tools: RemoteTool[], failures: McpServerLoadFailure[] = []) { + return jest.fn().mockResolvedValue({ tools, failures }); +} + function makeFetcher(overrides?: { workflowPort?: Partial>>; aiModelPort?: Partial>; @@ -127,10 +135,10 @@ describe('RemoteToolFetcher.fetch', () => { await fetcher.fetch('id-A', USER_ID); - expect(aiModelPort.loadRemoteTools).toHaveBeenCalledWith({ 'srv-a': cfg('id-A') }); + expect(aiModelPort.loadRemoteToolsWithFailures).toHaveBeenCalledWith({ 'srv-a': cfg('id-A') }); }); - it('returns no tools and an undefined mcpServerName, skipping loadRemoteTools, when the scoped Record is empty', async () => { + it('returns no tools and an undefined mcpServerName, skipping the tool load, when the scoped Record is empty', async () => { const { fetcher, aiModelPort } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({}) }, }); @@ -138,7 +146,7 @@ describe('RemoteToolFetcher.fetch', () => { const result = await fetcher.fetch('id-A', USER_ID); expect(result).toEqual({ tools: [], mcpServerName: undefined }); - expect(aiModelPort.loadRemoteTools).not.toHaveBeenCalled(); + expect(aiModelPort.loadRemoteToolsWithFailures).not.toHaveBeenCalled(); }); it('resolves mcpServerName from the scoped Record key', async () => { @@ -147,7 +155,7 @@ describe('RemoteToolFetcher.fetch', () => { workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), }, - aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue(remoteTools) }, + aiModelPort: { loadRemoteToolsWithFailures: loadsWithFailures(remoteTools) }, }); const result = await fetcher.fetch('id-A', USER_ID); @@ -208,12 +216,17 @@ describe('RemoteToolFetcher.fetch', () => { expect(logger.mock.calls.find(c => c[0] === 'Warn')).toBeUndefined(); }); - it('flags the scoped MCP config when no tool was loaded for its id', async () => { + it('names the failing server, its failure kind and its cause', async () => { const { fetcher, logger } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), }, - aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue([]) }, + aiModelPort: { + loadRemoteToolsWithFailures: loadsWithFailures( + [], + [makeFailure('srv-a', 'connection', 'connect ECONNREFUSED 10.0.4.12:8080')], + ), + }, }); await fetcher.fetch('id-A', USER_ID); @@ -221,28 +234,85 @@ describe('RemoteToolFetcher.fetch', () => { expect(logger).toHaveBeenCalledWith('Error', 'MCP servers failed to load tools', { requestedMcpServerId: 'id-A', mcpServerName: 'srv-a', - failedConfigNames: ['srv-a'], + failures: [ + { + server: 'srv-a', + kind: 'connection', + error: 'connect ECONNREFUSED 10.0.4.12:8080', + cause: undefined, + }, + ], }); }); - it('sets loadFailed when the scoped server produced no tools', async () => { + // Node wraps the real reason in `.cause`, which is the distinction the failure line exists for. + it('names the nested cause when the reported error only wraps it', async () => { + const wrapped = Object.assign(new Error('fetch failed'), { + cause: new Error('connect ECONNREFUSED 10.0.4.12:8080'), + }); + const { fetcher, logger } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), + }, + aiModelPort: { + loadRemoteToolsWithFailures: loadsWithFailures([], [ + { server: 'srv-a', kind: 'connection', error: wrapped }, + ] as McpServerLoadFailure[]), + }, + }); + + await fetcher.fetch('id-A', USER_ID); + + expect(logger).toHaveBeenCalledWith('Error', 'MCP servers failed to load tools', { + requestedMcpServerId: 'id-A', + mcpServerName: 'srv-a', + failures: [ + { + server: 'srv-a', + kind: 'connection', + error: 'fetch failed', + cause: 'connect ECONNREFUSED 10.0.4.12:8080', + }, + ], + }); + }); + + it('sets loadFailed when a server reported a load failure', async () => { const { fetcher } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), }, - aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue([]) }, + aiModelPort: { + loadRemoteToolsWithFailures: loadsWithFailures( + [], + [makeFailure('srv-a', 'auth', '401 Unauthorized')], + ), + }, }); expect((await fetcher.fetch('id-A', USER_ID)).loadFailed).toBe(true); }); + // A reachable server can expose nothing, and loadFailed answers 503 on the tool-listing endpoint. + it('does not set loadFailed when a healthy server exposes no tools', async () => { + const { fetcher, logger } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), + }, + aiModelPort: { loadRemoteToolsWithFailures: loadsWithFailures([]) }, + }); + + expect((await fetcher.fetch('id-A', USER_ID)).loadFailed).toBe(false); + expect(logger.mock.calls.find(call => call[0] === 'Error')).toBeUndefined(); + }); + it('does not set loadFailed when tools load successfully', async () => { const { fetcher } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), }, aiModelPort: { - loadRemoteTools: jest.fn().mockResolvedValue([makeRemoteTool('srv-a', 'id-A')]), + loadRemoteToolsWithFailures: loadsWithFailures([makeRemoteTool('srv-a', 'id-A')]), }, }); @@ -255,7 +325,7 @@ describe('RemoteToolFetcher.fetch', () => { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), }, aiModelPort: { - loadRemoteTools: jest.fn().mockResolvedValue([makeRemoteTool('srv-a', 'id-A')]), + loadRemoteToolsWithFailures: loadsWithFailures([makeRemoteTool('srv-a', 'id-A')]), }, }); @@ -277,7 +347,7 @@ describe('RemoteToolFetcher.fetch', () => { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'zendesk-prod': forestConfig }), }, aiModelPort: { - loadRemoteTools: jest.fn().mockResolvedValue([makeRemoteTool('zendesk', 'id-zendesk')]), + loadRemoteToolsWithFailures: loadsWithFailures([makeRemoteTool('zendesk', 'id-zendesk')]), }, }); @@ -286,7 +356,9 @@ describe('RemoteToolFetcher.fetch', () => { expect(logger.mock.calls.find(c => c[0] === 'Error')).toBeUndefined(); }); - it('flags a Forest connector that fails to load entirely', async () => { + // The provider names the integration, not the Record key, when it doesn't recognise it — this is + // the version-drift case where the orchestrator advertises something this build can't load. + it('flags a Forest connector whose integration this build does not support', async () => { const forestConfig = { id: 'id-zendesk', isForestConnector: true as const, @@ -296,7 +368,12 @@ describe('RemoteToolFetcher.fetch', () => { workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'zendesk-prod': forestConfig }), }, - aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue([]) }, + aiModelPort: { + loadRemoteToolsWithFailures: loadsWithFailures( + [], + [makeFailure('Zendesk', 'unknown', 'Unsupported integration: Zendesk')], + ), + }, }); await fetcher.fetch('id-zendesk', USER_ID); @@ -304,17 +381,24 @@ describe('RemoteToolFetcher.fetch', () => { expect(logger).toHaveBeenCalledWith('Error', 'MCP servers failed to load tools', { requestedMcpServerId: 'id-zendesk', mcpServerName: 'zendesk-prod', - failedConfigNames: ['zendesk-prod'], + failures: [ + { + server: 'Zendesk', + kind: 'unknown', + error: 'Unsupported integration: Zendesk', + cause: undefined, + }, + ], }); }); - it('returns the tools produced by loadRemoteTools verbatim', async () => { + it('returns the tools produced by the port verbatim', async () => { const remoteTools = [makeRemoteTool('srv-a', 'id-A')]; const { fetcher } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), }, - aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue(remoteTools) }, + aiModelPort: { loadRemoteToolsWithFailures: loadsWithFailures(remoteTools) }, }); const result = await fetcher.fetch('id-A', USER_ID); @@ -322,13 +406,13 @@ describe('RemoteToolFetcher.fetch', () => { expect(result.tools).toBe(remoteTools); }); - it('propagates a rejection from loadRemoteTools without logging partial-failure', async () => { + it('propagates a rejection from the tool load without logging partial-failure', async () => { const { fetcher, logger } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }), }, aiModelPort: { - loadRemoteTools: jest.fn().mockRejectedValue(new Error('MCP unreachable')), + loadRemoteToolsWithFailures: jest.fn().mockRejectedValue(new Error('MCP unreachable')), }, }); @@ -336,7 +420,7 @@ describe('RemoteToolFetcher.fetch', () => { expect(logger.mock.calls.find(c => c[0] === 'Error')).toBeUndefined(); }); - it('propagates a rejection from getMcpServerConfigs without calling loadRemoteTools', async () => { + it('propagates a rejection from getMcpServerConfigs without loading tools', async () => { const { fetcher, aiModelPort } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockRejectedValue(new Error('orchestrator down')), @@ -344,7 +428,7 @@ describe('RemoteToolFetcher.fetch', () => { }); await expect(fetcher.fetch('id-A', USER_ID)).rejects.toThrow('orchestrator down'); - expect(aiModelPort.loadRemoteTools).not.toHaveBeenCalled(); + expect(aiModelPort.loadRemoteToolsWithFailures).not.toHaveBeenCalled(); }); }); @@ -426,6 +510,61 @@ describe('RemoteToolFetcher.fetch — OAuth2 servers', () => { expect(result.tools).toEqual([tool]); }); + // The rejected credential is logged at Error by the provider, so a run that recovers reads as a + // pure failure at the default level unless the recovery is stated too. + it('reports the recovery after a forced refresh succeeds', async () => { + const loadRemoteToolsWithFailures = jest + .fn() + .mockResolvedValueOnce({ tools: [], failures: [authFailure] }) + .mockResolvedValueOnce({ tools: [makeRemoteTool('srv-a', 'id-A')], failures: [] }); + const { fetcher, logger } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + aiModelPort: { loadRemoteToolsWithFailures }, + tokenService: makeTokenService(jest.fn().mockResolvedValue('tok')), + }); + + await fetcher.fetch('id-A', USER_ID); + + expect(logger).toHaveBeenCalledWith( + 'Info', + 'MCP tools loaded after refreshing the credential', + { + requestedMcpServerId: 'id-A', + mcpServerName: 'srv-a', + }, + ); + }); + + // The tool-listing endpoint answers 503 on loadFailed, so a retry that fails for a non-auth + // reason must reach the caller as a failure rather than as an empty success. + it('sets loadFailed when the retry fails for a reason other than auth', async () => { + const loadRemoteToolsWithFailures = jest + .fn() + .mockResolvedValueOnce({ tools: [], failures: [authFailure] }) + .mockResolvedValueOnce({ + tools: [], + failures: [makeFailure('srv-a', 'connection', 'socket hang up')], + }); + const { fetcher, logger } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + aiModelPort: { loadRemoteToolsWithFailures }, + tokenService: makeTokenService(jest.fn().mockResolvedValue('tok')), + }); + + const result = await fetcher.fetch('id-A', USER_ID); + + expect(result.loadFailed).toBe(true); + expect(logger).not.toHaveBeenCalledWith( + 'Info', + 'MCP tools loaded after refreshing the credential', + expect.anything(), + ); + }); + it('raises OAuthReauthRequiredError when the auth failure persists after a forced refresh', async () => { const getAccessToken = jest.fn().mockResolvedValue('tok'); const loadRemoteToolsWithFailures = jest @@ -482,14 +621,14 @@ describe('RemoteToolFetcher.fetch — OAuth2 servers', () => { const tool = makeRemoteTool('srv-a', 'id-A'); const { fetcher, aiModelPort } = makeFetcher({ workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }) }, - aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue([tool]) }, + aiModelPort: { loadRemoteToolsWithFailures: loadsWithFailures([tool]) }, tokenService: makeTokenService(getAccessToken), }); const result = await fetcher.fetch('id-A', USER_ID); expect(getAccessToken).not.toHaveBeenCalled(); - expect(aiModelPort.loadRemoteTools).toHaveBeenCalled(); + expect(aiModelPort.loadRemoteToolsWithFailures).toHaveBeenCalledWith({ 'srv-a': cfg('id-A') }); expect(result.reloadWithFreshAuth).toBeUndefined(); }); }); diff --git a/packages/workflow-executor/test/runner.test.ts b/packages/workflow-executor/test/runner.test.ts index 9f278ed56f..8da4c00392 100644 --- a/packages/workflow-executor/test/runner.test.ts +++ b/packages/workflow-executor/test/runner.test.ts @@ -61,6 +61,7 @@ function createMockAiClient() { return { getModel: jest.fn().mockReturnValue({} as BaseChatModel), loadRemoteTools: jest.fn().mockResolvedValue([]), + loadRemoteToolsWithFailures: jest.fn().mockResolvedValue({ tools: [], failures: [] }), closeConnections: jest.fn().mockResolvedValue(undefined), }; } @@ -1339,10 +1340,10 @@ describe('MCP lazy loading (via once thunk)', () => { await runner.triggerPoll('run-1'); expect(workflowPort.getMcpServerConfigs).not.toHaveBeenCalled(); - expect(aiClient.loadRemoteTools).not.toHaveBeenCalled(); + expect(aiClient.loadRemoteToolsWithFailures).not.toHaveBeenCalled(); }); - it('skips loadRemoteTools when the orchestrator returns an empty Record', async () => { + it('skips the tool load when the orchestrator returns an empty Record', async () => { const workflowPort = createMockWorkflowPort(); const aiClient = createMockAiClient(); const step = makePendingStep({ @@ -1362,7 +1363,7 @@ describe('MCP lazy loading (via once thunk)', () => { await runner.triggerPoll('run-1'); expect(workflowPort.getMcpServerConfigs).toHaveBeenCalledTimes(1); - expect(aiClient.loadRemoteTools).not.toHaveBeenCalled(); + expect(aiClient.loadRemoteToolsWithFailures).not.toHaveBeenCalled(); // Distinguish the short-circuit from a regression that throws before reaching the guard: // the step must actually have executed and reported a success outcome. expect(workflowPort.updateStepExecution).toHaveBeenCalledWith( @@ -1373,7 +1374,7 @@ describe('MCP lazy loading (via once thunk)', () => { }); describe('MCP fetch scoping', () => { - it('passes only the matching config to loadRemoteTools when step.mcpServerId is set', async () => { + it('passes only the matching config to the tool load when step.mcpServerId is set', async () => { const workflowPort = createMockWorkflowPort(); const aiClient = createMockAiClient(); const step = makePendingStep({ @@ -1400,8 +1401,8 @@ describe('MCP fetch scoping', () => { ); await runner.triggerPoll('run-1'); - expect(aiClient.loadRemoteTools).toHaveBeenCalledTimes(1); - expect(aiClient.loadRemoteTools).toHaveBeenCalledWith({ + expect(aiClient.loadRemoteToolsWithFailures).toHaveBeenCalledTimes(1); + expect(aiClient.loadRemoteToolsWithFailures).toHaveBeenCalledWith({ 'server-A': expect.objectContaining({ id: 'id-A' }), }); }); @@ -1435,12 +1436,12 @@ describe('MCP fetch scoping', () => { ); await runner.triggerPoll('run-1'); - expect(aiClient.loadRemoteTools).toHaveBeenCalledWith({ + expect(aiClient.loadRemoteToolsWithFailures).toHaveBeenCalledWith({ 'server-B': expect.objectContaining({ id: 'server-A' }), }); }); - it('skips loadRemoteTools and warns with availableMcpServerIds when no config matches', async () => { + it('skips the tool load and warns with availableMcpServerIds when no config matches', async () => { const workflowPort = createMockWorkflowPort(); const aiClient = createMockAiClient(); const logger = createMockLogger(); @@ -1473,7 +1474,7 @@ describe('MCP fetch scoping', () => { await runner.triggerPoll('run-1'); expect(workflowPort.getMcpServerConfigs).toHaveBeenCalledTimes(1); - expect(aiClient.loadRemoteTools).not.toHaveBeenCalled(); + expect(aiClient.loadRemoteToolsWithFailures).not.toHaveBeenCalled(); expect(logger).toHaveBeenCalledWith( 'Warn', 'MCP step targets a server not advertised by the orchestrator', @@ -1514,7 +1515,7 @@ describe('MCP fetch scoping', () => { ); await runner.triggerPoll('run-1'); - expect(aiClient.loadRemoteTools).not.toHaveBeenCalled(); + expect(aiClient.loadRemoteToolsWithFailures).not.toHaveBeenCalled(); expect(logger).toHaveBeenCalledWith( 'Warn', 'MCP step targets a server but orchestrator returned no MCP configs', @@ -1530,7 +1531,7 @@ describe('MCP fetch scoping', () => { // The diagnostic must not short-circuit dispatch — the executor is still constructed (and // will surface NoMcpToolsError downstream). Asserting on executeSpy.mock.instances bypasses // the global execute() spy to confirm the executor saw the (empty) tool list. - it('logs partial-failure and still dispatches to the executor when the scoped server loaded zero tools', async () => { + it('logs the reported failure and still dispatches to the executor with no tools', async () => { const workflowPort = createMockWorkflowPort(); const aiClient = createMockAiClient(); const logger = createMockLogger(); @@ -1551,7 +1552,10 @@ describe('MCP fetch scoping', () => { workflowPort.getMcpServerConfigs.mockResolvedValue({ 'server-A': { id: 'id-A', url: 'https://a.example', type: 'http', headers: {} }, }); - aiClient.loadRemoteTools.mockResolvedValue([]); + aiClient.loadRemoteToolsWithFailures.mockResolvedValue({ + tools: [], + failures: [{ server: 'server-A', kind: 'connection', error: new Error('socket hang up') }], + }); runner = new Runner( createRunnerConfig({ @@ -1565,7 +1569,7 @@ describe('MCP fetch scoping', () => { expect(logger).toHaveBeenCalledWith('Error', 'MCP servers failed to load tools', { requestedMcpServerId: 'id-A', mcpServerName: 'server-A', - failedConfigNames: ['server-A'], + failures: [{ server: 'server-A', kind: 'connection', error: 'socket hang up' }], }); expect(executeSpy).toHaveBeenCalledTimes(1); const executorInstance = executeSpy.mock.instances[0]; @@ -1575,7 +1579,7 @@ describe('MCP fetch scoping', () => { ).toEqual([]); }); - it('re-scopes loadRemoteTools per dispatch when chained MCP steps target different servers', async () => { + it('re-scopes the tool load per dispatch when chained MCP steps target different servers', async () => { const workflowPort = createMockWorkflowPort(); const aiClient = createMockAiClient(); const mcpDef = (id: string) => @@ -1615,11 +1619,11 @@ describe('MCP fetch scoping', () => { ); await runner.triggerPoll('run-1'); - expect(aiClient.loadRemoteTools).toHaveBeenCalledTimes(2); - expect(aiClient.loadRemoteTools).toHaveBeenNthCalledWith(1, { + expect(aiClient.loadRemoteToolsWithFailures).toHaveBeenCalledTimes(2); + expect(aiClient.loadRemoteToolsWithFailures).toHaveBeenNthCalledWith(1, { 'server-A': expect.objectContaining({ id: 'id-A' }), }); - expect(aiClient.loadRemoteTools).toHaveBeenNthCalledWith(2, { + expect(aiClient.loadRemoteToolsWithFailures).toHaveBeenNthCalledWith(2, { 'server-B': expect.objectContaining({ id: 'id-B' }), }); });