Skip to content

feat: connect orchestrator and frontend#1564

Open
Scra3 wants to merge 193 commits into
mainfrom
feat/prd-214-server-step-mapper
Open

feat: connect orchestrator and frontend#1564
Scra3 wants to merge 193 commits into
mainfrom
feat/prd-214-server-step-mapper

Conversation

@Scra3
Copy link
Copy Markdown
Member

@Scra3 Scra3 commented Apr 20, 2026

MAIN BRANCH TO INTRODUCE WORKFLOW EXECUTOR.

image

Note

Add @forestadmin/workflow-executor package connecting the orchestrator and frontend

  • Introduces a new @forestadmin/workflow-executor package with a CLI (forest-workflow-executor), an HTTP server with JWT auth, and a polling runner that fetches and executes workflow steps from the Forest server.
  • Implements step executors for all supported step types: condition, read-record, update-record, trigger-action, load-related-record, MCP, and guidance — each with idempotency, activity logging, and AI-driven field/record/action selection.
  • Adds adapters for all ports: AgentClientAgentPort, ForestServerWorkflowPort, AiClientAdapter, ServerAiAdapter, database and in-memory RunStore, and ForestadminClientActivityLogPort with retry and drain semantics.
  • Adds proxy routes to the agent (GET/_internal/workflow-executions/:runId and POST .../trigger) in workflow-executor-proxy.ts, gated on the new workflowExecutorUrl agent option.
  • Extends @forestadmin/ai-proxy with AiClient, createBaseChatModel, getAiConfiguration, and mcpServerId propagation across MCP and Forest integration tool factories.
  • Risk: The new package requires FOREST_ENV_SECRET, FOREST_AUTH_SECRET, AGENT_URL, and DATABASE_URL at startup; missing or invalid values throw a ConfigurationError and prevent the process from starting.

Macroscope summarized 82dd057.

matthv and others added 30 commits March 17, 2026 15:00
…premature deps, add smoke test

- Rewrite CLAUDE.md with project overview and architecture principles, remove changelog
- Remove unused dependencies (ai-proxy, sequelize, zod) per YAGNI
- Add smoke test so CI passes

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… document system architecture

- Lint now covers src and test directories
- Replace require() with import, use stronger assertion (toHaveLength)
- Add System Architecture section describing Front/Orchestrator/Executor/Agent
- Mark Architecture Principles as planned (not yet implemented)
- Remove redundant test/.gitkeep
- Make index.ts a valid module with export {}

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…erver (#1504)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: alban bertolini <albanb@forestadmin.com>
…ain (#1512)

Co-authored-by: alban bertolini <albanb@forestadmin.com>
- Remove McpClient.tools property, loadTools() returns local array
- Rename closeConnections() → dispose()
- Rename testConnections() → checkConnection()
- Add McpServers type export
- Rename mcpServerConfigs → toolConfigs in create-ai-provider
- Update all tests accordingly

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eability

Add sourceId to McpToolRef so that persisted execution data (pendingData,
executionParams) tracks which MCP server provided the tool. This fixes
tool lookup on re-entry (confirmation flow) when multiple servers expose
tools with the same name.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… collection schema

- Replace non-null assertion with explicit McpToolNotFoundError when AI
  selects a tool name that doesn't match any available tool
- Resolve related collection name from parent schema before looking up
  the related schema in cache, fixing cases where relation name differs
  from target collection name

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merge main into feature branch, resolve conflicts by taking main's
versions of router.ts, create-ai-provider.ts and their tests.
Remove deleted mcp-config-checker.ts. Bump workflow-executor internal
deps to match main.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Scra3 Scra3 changed the base branch from main to feat/prd-214-setup-workflow-executor-package May 18, 2026 13:00
@Scra3 Scra3 changed the base branch from feat/prd-214-setup-workflow-executor-package to main May 18, 2026 13:03
alban bertolini and others added 6 commits May 19, 2026 18:37
The model sometimes returns snake_case (first_name) when the actual
displayName is camelCase or unseparated (firstname). Add a normalized
fallback that strips separators and lowercases before matching, so a
hallucinated variation doesn't kill an otherwise correct step.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…allback

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ape (#1583)

fix(workflow-executor): fix MCP step path — type shape and id matching

MCP-typed workflow steps failed end-to-end before this change. Two
independent bugs on the path getMcpServerConfigs → loadRemoteTools →
getFilteredTools:

1. Port type mismatch (PRD-357). The orchestrator's
   /liana/mcp-server-configs-with-details endpoint returns
   Record<string, ToolConfig>, but the executor port was typed
   McpConfiguration[] and runner.fetchRemoteTools called .map on it —
   every MCP step crashed with "TypeError: configs.map is not a
   function" before reaching loadRemoteTools. Tests masked the bug
   by mocking mockResolvedValue([]) at 8 call sites, which matched
   the wrong type and short-circuited the buggy branch.

   The port now returns McpServers. The { configs } wrap to
   langchain's McpConfiguration shape lives in runner.fetchRemoteTools,
   one site, after the empty-record short-circuit.

2. Tool id matching (PRD-362). The executor's filter compared
   tool.sourceId (server display name) against step.mcpServerId (DB
   id written by the frontend), so any workflow that specified an
   MCP server failed with NoMcpToolsError regardless of
   configuration.

   The stable DB id from each ToolConfig entry is now threaded
   through ai-proxy (McpClient, ForestIntegrationClient, and the
   integration factories) onto RemoteTool.mcpServerId, and the
   executor matches by id. NoMcpToolsError's technical message now
   includes the requested id and the loaded id list so
   misconfigurations are diagnosable from structured logs; the
   user-facing message stays generic per the dual-message convention.

   The new tool-side field is named mcpServerId (not id) to read
   honestly at the consumer site — tool.mcpServerId === step.mcpServerId
   expresses the FK relationship plainly. McpServerConfig.id and
   ForestIntegrationConfig.id keep id since those mirror the wire
   shape (which itself mirrors the ai_mcp_configs PK column).

Also drops the now-unused McpConfiguration re-export from the port
and barrel (used only internally by AiModelPort/adapters, imported
directly from @forestadmin/ai-proxy), drops a wire-shape comment
that the type and route URL already document, and renames a test
config key from "data-gouv" to "mcp-server-1" to keep the test free
of real-world references.

fixes PRD-357
fixes PRD-362
Comment thread packages/workflow-executor/src/executors/base-step-executor.ts Outdated
Comment on lines +64 to +67
const status: GuidanceStepOutcome['status'] = ctx.status === 'error' ? 'error' : 'success';

return { type: 'guidance', ...baseFromCtx, status };
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low adapters/run-to-available-step-mapper.ts:64

The guidance branch converts 'awaiting-input' status to 'success' because it uses ctx.status === 'error' ? 'error' : 'success' instead of toRecordStatus. Since GuidanceStepOutcomeSchema explicitly allows 'awaiting-input' via RecordStepStatusSchema, this valid state is incorrectly overwritten. Consider using toRecordStatus(ctx.status) to preserve all valid status values.

Suggested change
const status: GuidanceStepOutcome['status'] = ctx.status === 'error' ? 'error' : 'success';
return { type: 'guidance', ...baseFromCtx, status };
}
if (outcomeType === 'guidance') {
const status: GuidanceStepOutcome['status'] = toRecordStatus(ctx.status);
return { type: 'guidance', ...baseFromCtx, status };
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts around lines 64-67:

The `guidance` branch converts `'awaiting-input'` status to `'success'` because it uses `ctx.status === 'error' ? 'error' : 'success'` instead of `toRecordStatus`. Since `GuidanceStepOutcomeSchema` explicitly allows `'awaiting-input'` via `RecordStepStatusSchema`, this valid state is incorrectly overwritten. Consider using `toRecordStatus(ctx.status)` to preserve all valid status values.

Evidence trail:
packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts lines 30-35 (toRecordStatus helper), line 64 (guidance branch using ternary instead of toRecordStatus); packages/workflow-executor/src/types/validated/step-outcome.ts lines 11 (RecordStepStatusSchema allows 'awaiting-input'), lines 57-63 (GuidanceStepOutcomeSchema uses RecordStepStatusSchema); packages/workflow-executor/src/executors/guidance-step-executor.ts line 14 (executor produces 'awaiting-input' for guidance steps)

Comment on lines +185 to +192
await this.context.runStore.saveStepExecution(this.context.runId, {
...existingExecution,
type: 'trigger-action',
stepIndex: this.context.stepIndex,
executionParams: { displayName, name },
executionResult: { success: true, actionResult },
selectedRecordRef,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium executors/trigger-record-action-step-executor.ts:185

saveFrontendResult spreads existingExecution without setting idempotencyPhase: 'done', so after Branch C completes the step data lacks that field. If the executor restarts, checkIdempotency() (line 53) fails to recognize completion because idempotencyPhase is undefined, causing the step to incorrectly re-enter the confirmation flow.

    await this.context.runStore.saveStepExecution(this.context.runId, {
      ...existingExecution,
      type: 'trigger-action',
      stepIndex: this.context.stepIndex,
      executionParams: { displayName, name },
      executionResult: { success: true, actionResult },
      selectedRecordRef,
+      idempotencyPhase: 'done',
    });
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts around lines 185-192:

`saveFrontendResult` spreads `existingExecution` without setting `idempotencyPhase: 'done'`, so after Branch C completes the step data lacks that field. If the executor restarts, `checkIdempotency()` (line 53) fails to recognize completion because `idempotencyPhase` is undefined, causing the step to incorrectly re-enter the confirmation flow.

Evidence trail:
packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts lines 48-62 (checkIdempotency), 134-142 (Branch C save without idempotencyPhase), 178-195 (saveFrontendResult missing idempotencyPhase:'done'), 146-174 (executeOnExecutor correctly sets idempotencyPhase). packages/workflow-executor/src/executors/update-record-step-executor.ts lines 205-239 (resolveAndUpdate sets idempotencyPhase:'done' even in confirmation flow). packages/workflow-executor/src/types/step-execution-data.ts lines 13-14 (MutatingStepExecutionData.idempotencyPhase definition), lines 76-83 (TriggerRecordActionStepExecutionData extends MutatingStepExecutionData). packages/workflow-executor/src/executors/base-step-executor.ts lines 194-234 (findPendingExecution, patchAndReloadPendingData), lines 238-262 (handleConfirmationFlow).


export default class ConsoleLogger implements Logger {
error(message: string, context: Record<string, unknown>): void {
console.error(JSON.stringify({ message, timestamp: new Date().toISOString(), ...context }));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low adapters/console-logger.ts:5

In error, warn, and info, the spread order { message, timestamp, ...context } allows context to overwrite message and timestamp when the caller passes those keys. This corrupts the log output by replacing the actual message or timestamp with values from context. Reorder the spread to { ...context, message, timestamp } to protect the primary log fields.

-      console.error(JSON.stringify({ message, timestamp: new Date().toISOString(), ...context }));
+      console.error(JSON.stringify({ ...context, message, timestamp: new Date().toISOString() }));
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/console-logger.ts around line 5:

In `error`, `warn`, and `info`, the spread order `{ message, timestamp, ...context }` allows `context` to overwrite `message` and `timestamp` when the caller passes those keys. This corrupts the log output by replacing the actual message or timestamp with values from context. Reorder the spread to `{ ...context, message, timestamp }` to protect the primary log fields.

Evidence trail:
packages/workflow-executor/src/adapters/console-logger.ts lines 4-14 at REVIEWED_COMMIT — `...context` is spread last in the object literal on lines 5, 9, and 13, meaning any `message` or `timestamp` key in the `context: Record<string, unknown>` parameter overwrites the primary fields. JavaScript spec (ES2018+) defines that later spread properties overwrite earlier ones with the same key.

Comment on lines +14 to +18
function isRetryable(err: unknown): boolean {
const { status } = err as { status?: number };

return typeof status === 'number' && RETRYABLE_STATUS.has(status);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low adapters/with-retry.ts:14

When fn throws null or undefined, the destructuring const { status } = err as { status?: number } throws a TypeError before line 17 can check the status. This crashes the retry mechanism instead of treating the error as non-retryable. Consider adding a null guard before destructuring, or use optional chaining to safely access status.

 function isRetryable(err: unknown): boolean {
-  const { status } = err as { status?: number };
+  if (err == null) return false;
+  const status = (err as { status?: number }).status;
 
   return typeof status === 'number' && RETRYABLE_STATUS.has(status);
 }
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/with-retry.ts around lines 14-18:

When `fn` throws `null` or `undefined`, the destructuring `const { status } = err as { status?: number }` throws a `TypeError` before line 17 can check the status. This crashes the retry mechanism instead of treating the error as non-retryable. Consider adding a null guard before destructuring, or use optional chaining to safely access `status`.

Evidence trail:
packages/workflow-executor/src/adapters/with-retry.ts lines 14-18 at REVIEWED_COMMIT: `isRetryable` destructures `err` without null guard. JavaScript behavior: `const { x } = null` throws TypeError. Callers at packages/workflow-executor/src/adapters/forest-server-workflow-port.ts:231, packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts:26,58,80.

Comment thread packages/ai-proxy/src/ai-client.ts Outdated
Comment on lines +39 to +49
async loadRemoteTools(
mcpConfig: McpConfiguration,
): Promise<Awaited<ReturnType<McpClient['loadTools']>>> {
await this.disposeMcpClient('Error closing previous MCP connection');

const newClient = new McpClient(mcpConfig, this.logger);
const tools = await newClient.loadTools();
this.mcpClient = newClient;

return tools;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium src/ai-client.ts:39

If newClient.loadTools() throws, the McpClient instance created on line 44 is never disposed. Since this.mcpClient is only assigned after loadTools() succeeds, a failed call leaves newClient dangling with open connections, causing a resource leak. Consider wrapping loadTools() in a try/finally to ensure cleanup on failure.

  async loadRemoteTools(
    mcpConfig: McpConfiguration,
  ): Promise<Awaited<ReturnType<McpClient['loadTools']>>> {
    await this.disposeMcpClient('Error closing previous MCP connection');

    const newClient = new McpClient(mcpConfig, this.logger);
+    try {
      const tools = await newClient.loadTools();
      this.mcpClient = newClient;

      return tools;
+    } catch (error) {
+      await newClient.dispose();
+      throw error;
+    }
  }
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/ai-proxy/src/ai-client.ts around lines 39-49:

If `newClient.loadTools()` throws, the `McpClient` instance created on line 44 is never disposed. Since `this.mcpClient` is only assigned after `loadTools()` succeeds, a failed call leaves `newClient` dangling with open connections, causing a resource leak. Consider wrapping `loadTools()` in a try/finally to ensure cleanup on failure.

Evidence trail:
packages/ai-proxy/src/ai-client.ts lines 39-49 (REVIEWED_COMMIT): loadRemoteTools creates newClient on line 44, calls loadTools() on line 45, assigns to this.mcpClient on line 46. No try/finally wrapping.
packages/ai-proxy/src/mcp-client.ts lines 24-37 (REVIEWED_COMMIT): McpClient constructor creates MultiServerMCPClient instances stored in this.mcpClients.
packages/ai-proxy/src/mcp-client.ts lines 95-118 (REVIEWED_COMMIT): dispose() method calls client.close() on each MultiServerMCPClient - explicit cleanup needed.
packages/ai-proxy/src/ai-client.ts lines 55-66 (REVIEWED_COMMIT): disposeMcpClient only disposes this.mcpClient, not a local newClient variable.

});
}

async markFailed(handle: ActivityLogHandle, errorMessage: string): Promise<void> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low adapters/forestadmin-client-activity-log-port.ts:77

In markFailed, the errorMessage parameter is accepted but never passed to updateActivityLogStatus. The status is set to 'failed' without persisting the error context to the audit log, so the failure reason is lost. The message is only used for local logging when the update itself fails.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts around line 77:

In `markFailed`, the `errorMessage` parameter is accepted but never passed to `updateActivityLogStatus`. The status is set to `'failed'` without persisting the error context to the audit log, so the failure reason is lost. The message is only used for local logging when the update itself fails.

Evidence trail:
packages/workflow-executor/src/adapters/forestadmin-client-activity-log-port.ts lines 77-98 (markFailed method), packages/workflow-executor/src/ports/activity-log-port.ts line 20 (ActivityLogPort interface), packages/forestadmin-client/src/types.ts lines 254-258 (UpdateActivityLogStatusParams - no errorMessage field), packages/forestadmin-client/src/activity-logs/index.ts lines 97-107 (updateActivityLogStatus implementation), packages/forestadmin-client/CHANGELOG.md line 43 (intentional removal of errorMessage from updateActivityLogStatus)

Scra3 and others added 8 commits May 28, 2026 09:30
…mpat (#1604)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lient (#1599)

* fix(ai-proxy): route Forest connectors via tool-provider split in AiClient

AiClient.loadRemoteTools was instantiating McpClient directly, forwarding
every config (including ForestIntegrationConfig entries like Zendesk) to
MultiServerMCPClient. Forest connectors lack the stdio/HTTP transport
fields, so @langchain/mcp-adapters threw a Zod union error and any
workflow run touching a Forest-hosted connector crashed before the step
could execute. Delegate to createToolProviders so Forest connectors are
routed to ForestIntegrationClient — same path Router and ToolSourceChecker
already use. Fixes PRD-400.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ver (PRD-363) (#1607)

Before this change, every MCP step opened connections to all configured MCP servers
and listed their tools, then filtered in-memory by `step.mcpServerId`. With N
configured servers, an MCP step paid N× the connect/list-tools cost and hit every
upstream server unnecessarily.

- Extract a `RemoteToolFetcher` module that scopes configs by `cfg.id === mcpServerId`
  (the persisted DB id from PRD-360) before calling `loadRemoteTools`. Only the
  targeted server is contacted.
- Make `mcpServerId` required on `McpStepDefinition` (zod `z.string().min(1)`);
  remove the pre-PRD-360 legacy fallback. A missing id is now a malformed run, not
  a silent broad fetch.
- Unify the partial-failure check across MCP and Forest connectors via
  `tool.mcpServerId`; a Forest connector that fails to load is now flagged in
  `failedConfigNames` instead of surfacing as a generic `NoMcpToolsError`.
- Collapse `McpStepExecutor.getFilteredTools` into `requireTools` (pre-scoped list →
  assert non-empty); make `NoMcpToolsError`'s message actionable.
- Distinct ops diagnostics: "no configs at all" vs "no match" warns, and
  `failedConfigNames` error log on partial loads.

fixes PRD-363
---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tep logs (PRD-418) (#1616)

* feat(workflow-executor): include mcpServerId + mcpServerName in MCP step logs (PRD-418)

Adds a getExtraLogContext() hook on BaseStepExecutor (default {}) merged into
logCtx, overridden by McpStepExecutor to stamp every MCP step log line with
mcpServerId and mcpServerName. RemoteToolFetcher.fetch() now resolves the
serverName from the scoped config Record key and returns { tools, serverName },
forwarded to the executor via StepExecutorFactory. Diagnostic log lines in the
fetcher also carry mcpServerName.

fixes PRD-418

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants