feat: connect orchestrator and frontend#1564
Conversation
…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>
…+ DatabaseStore) (#1506)
…xecutor factories (#1510)
…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>
…pending data When the executor picks up a guidance step from the polling loop (before the user has submitted anything), it now returns awaiting-input instead of throwing StepStateError. This keeps the step pending so the user trigger can process it with the submitted data. Also makes userInput optional so users can submit a guidance step without providing any text input. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…restoreFieldNames Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… rethrow path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ect to fix OpenAI rejection
OpenAI requires type: "object" at the root of a tool schema. Using
z.discriminatedUnion directly as schema serialized to anyOf, causing
a 400 "got type: None" error on update-data steps with ≥2 fields.
Wrap in z.object({ input: ... }) (same pattern as the frontend) so
the root is always type: "object". Switch to z.union for the field
variants — discriminatedUnion brought no benefit for LLM selection.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… root schema shape Add two tests to prevent the OpenAI 400 regression from silently re-appearing: one asserting the root schema is a ZodObject (not a union), and one covering the multi-field z.union path with a flat payload rejection check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…apper for update-record-field
The integration tests were still passing the old flat args shape
{ fieldName, value, reasoning } to the mock model. After wrapping
the tool schema in z.object({ input: ... }), the executor now
destructures result.input — so the mocks need the wrapper too.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r) when finding pending step After a back change, errored steps are stored as done:false + context.error instead of done:true. The find() must exclude them so the executor picks the next genuinely pending step and not the already-failed one. Also includes a temporary throw in ReadRecordStepExecutor for manual front-end error testing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… first non-done The orchestrator is the source of truth for which step to execute next. Always pick the last entry in workflowHistory rather than scanning for the first non-done/non-cancelled/non-errored step. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| run: ServerHydratedWorkflowRun, | ||
| err: WorkflowExecutorError, | ||
| ): MalformedRunInfo { | ||
| const pending = run.workflowHistory.at(-1) ?? null; |
There was a problem hiding this comment.
🟡 Medium adapters/forest-server-workflow-port.ts:134
In toMalformedInfo, accessing run.workflowHistory.at(-1) throws TypeError: Cannot read properties of undefined (reading 'at') when workflowHistory is null or undefined. Since this method is only called from error handlers during malformed run recovery, a missing workflowHistory causes the error handling itself to crash. Consider guarding the access with optional chaining: run.workflowHistory?.at(-1).
| const pending = run.workflowHistory.at(-1) ?? null; | |
| const pending = run.workflowHistory?.at(-1) ?? null; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/forest-server-workflow-port.ts around line 134:
In `toMalformedInfo`, accessing `run.workflowHistory.at(-1)` throws `TypeError: Cannot read properties of undefined (reading 'at')` when `workflowHistory` is `null` or `undefined`. Since this method is only called from error handlers during malformed run recovery, a missing `workflowHistory` causes the error handling itself to crash. Consider guarding the access with optional chaining: `run.workflowHistory?.at(-1)`.
Evidence trail:
packages/workflow-executor/src/adapters/forest-server-workflow-port.ts line 134: `run.workflowHistory.at(-1)` without optional chaining
packages/workflow-executor/src/adapters/server-types.ts line 120: `workflowHistory: ServerStepHistory[]` (non-nullable type, but data from network)
packages/workflow-executor/src/adapters/forest-server-workflow-port.ts lines 63-76: catch block calling `toMalformedInfo` at line 69 when error is `WorkflowExecutorError`
packages/workflow-executor/src/adapters/forest-server-workflow-port.ts lines 107-128: `toDispatch` throws `InvalidStepDefinitionError` at lines 109 and 118 BEFORE `workflowHistory` is accessed
packages/workflow-executor/src/adapters/forest-server-workflow-port.ts lines 55-57: data comes from server HTTP response with no runtime type validation
… run) at(-1) picks the orchestrator's current step but must still return null when that step is already done — the run is complete and nothing to execute. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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>
…fails before timeout (#1587)
…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
| const status: GuidanceStepOutcome['status'] = ctx.status === 'error' ? 'error' : 'success'; | ||
|
|
||
| return { type: 'guidance', ...baseFromCtx, status }; | ||
| } |
There was a problem hiding this comment.
🟢 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.
| 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)
| await this.context.runStore.saveStepExecution(this.context.runId, { | ||
| ...existingExecution, | ||
| type: 'trigger-action', | ||
| stepIndex: this.context.stepIndex, | ||
| executionParams: { displayName, name }, | ||
| executionResult: { success: true, actionResult }, | ||
| selectedRecordRef, | ||
| }); |
There was a problem hiding this comment.
🟡 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 })); |
There was a problem hiding this comment.
🟢 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.
| function isRetryable(err: unknown): boolean { | ||
| const { status } = err as { status?: number }; | ||
|
|
||
| return typeof status === 'number' && RETRYABLE_STATUS.has(status); | ||
| } |
There was a problem hiding this comment.
🟢 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.
MAIN BRANCH TO INTRODUCE WORKFLOW EXECUTOR.
Note
Connect workflow executor to agent frontend with proxy routes and full executor implementation
@forestadmin/workflow-executorpackage: a standalone service that polls the Forest server for pending workflow runs, executes steps (condition, read-record, update-record, trigger-action, load-related-record, MCP, guidance), and reports results back.GET /runs/:runIdandPOST /runs/:runId/trigger, and aGET /healthendpoint.WorkflowExecutorProxyRouteto the agent (workflow-executor-proxy.ts) that proxies/_internal/workflow-executions/:runIdand/_internal/workflow-executions/:runId/triggerto the executor whenworkflowExecutorUrlis configured.AiClientand related adapters in@forestadmin/ai-proxyfor BaseChatModel caching, MCP tool loading, and per-toolmcpServerIdpropagation.RunStoreimplementations, a CLI entrypoint (forest-workflow-executor), and example deployment configuration.workflowExecutorUrl; omitting it silently disables all workflow execution endpoints.Macroscope summarized 44363ac.