From 47a1cf64a9c19d465a0e876a502b8085b435c489 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 17 Aug 2026 18:54:18 +0200 Subject: [PATCH 1/6] feat(workflow-executor): classify step errors by who has to act Every step error reads as a system failure today, so an operator is told to contact an administrator over a record they deleted themselves. Error outcomes now carry errorKind (operator / configuration / system) and, when the error is about a different step, errorSourceStepIndex. A missing source record decides on whether the operator had an alternative rather than on the shape of the source step's result: persistSkip is Full-AI-only, so deciding on the shape would make the same empty relation read one way unattended and another with a human at the pause. A source execution the guard cannot read names nobody, since that is as likely our own bug. Nothing renders differently yet. An unclassified error produces the payload it produces today, and the front equality-matches operator once its half lands. Co-Authored-By: Claude Fable 5 --- packages/workflow-executor/CLAUDE.md | 2 + .../adapters/run-to-available-step-mapper.ts | 13 +- .../step-outcome-to-update-step-mapper.ts | 6 + packages/workflow-executor/src/errors.ts | 50 ++++- .../src/executors/base-step-executor.ts | 21 +- .../src/executors/condition-step-executor.ts | 4 +- .../src/executors/guidance-step-executor.ts | 4 +- .../src/executors/mcp-step-executor.ts | 8 +- .../src/executors/record-step-executor.ts | 29 ++- .../src/types/validated/step-outcome.ts | 13 ++ .../forest-server-workflow-port.test.ts | 38 ++++ .../run-to-available-step-mapper.test.ts | 115 ++++++++++ ...step-outcome-to-update-step-mapper.test.ts | 112 ++++++++++ .../workflow-executor/test/errors.test.ts | 93 ++++++++ .../test/executors/base-step-executor.test.ts | 25 ++- ...rigger-record-action-step-executor.test.ts | 199 ++++++++++++++++-- .../test/types/step-outcome.test.ts | 97 +++++++++ 17 files changed, 797 insertions(+), 32 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 49e30e2fe3..ca274c5320 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -45,6 +45,8 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p - *Step-execution errors* extend `WorkflowExecutorError` → caught by `base-step-executor.ts`, turned into `stepOutcome.error`. Never reach HTTP. - *Boundary errors* (`ConfigurationError`, `PendingDataNotFoundError`, `AgentProbeError`, …) extend plain `Error` → caught at HTTP/Runner layer. They must **not** extend `WorkflowExecutorError` (or the base executor would swallow them). - `WorkflowExecutorError` carries `message` (technical, logs) **and** `userMessage` (end-user, surfaced via `stepOutcome.error`). New subclasses must set a distinct, jargon-free `userMessage`. Request-level errors extend a *category* (`NotFoundError`/`AccessDeniedError`/`UnavailableError`) so `toHttpError` maps status by category — no per-error binding. + - **Who has to act** — `errorKind` (`operator`/`configuration`/`system`) is declared once per subclass as `static defaultErrorKind`, and overridden at the throw site only where the same error has different audiences (`SourceRecordMissingError`, which decides on whether the operator had a candidate to pick). Unset ⇒ absent from the outcome ⇒ the front frames it as it always has, so leaving a new error unclassified is safe. `errorSourceStepIndex` names the step an error is *about*, by index rather than step id — a LinkTo loop repeats ids, so only the index identifies the iteration. + - Both fields ride `context` on the update-step request and are read back by `run-to-available-step-mapper`, which drops an off-vocabulary value instead of passing it on: it would fail `AvailableStepExecutionSchema.parse` and take the whole run down. The front equality-matches `operator`, so a widened enum degrades an older front rather than breaking it. - **`displayName` vs technical name** — AI tools/prompts use `displayName` (the admin-configured label end users write against), never `fieldName`. Map AI-returned display names back to technical names before any datasource op. - **Idempotency (mutating steps: update-record, trigger-action, mcp)** — write-ahead log in the RunStore: save `idempotencyPhase: 'executing'` before the side effect, `'done'` + `executionResult` after. On re-dispatch `(runId, stepIndex)`: `done` → rebuild success outcome without re-running or re-logging; `executing` → throw `StepStateError`. `checkIdempotency()` runs before `doExecute()`; the `executing` marker is set in the `beforeCall` thunk passed to `AgentWithLog` (after `createPending`) so a log-creation failure leaves no orphan marker. Non-mutating steps don't override it (replay is safe). - **Fetched steps must execute** — any step from `getAvailableRuns()` must run; silently dropping one breaks the orchestrator contract. The only allowed pre-filter is `inFlightRuns` dedup (keyed by `runId`, not step — a chain advances `stepId`). diff --git a/packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts b/packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts index 36d0d0e4ae..942bd28899 100644 --- a/packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts +++ b/packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts @@ -27,7 +27,11 @@ import { type Step, type StepUser, } from '../types/validated/execution'; -import { stepTypeToOutcomeType } from '../types/validated/step-outcome'; +import { + ErrorKindSchema, + ErrorSourceStepIndexSchema, + stepTypeToOutcomeType, +} from '../types/validated/step-outcome'; function toRecordStatus(ctxStatus: unknown): RecordStepOutcome['status'] { if (ctxStatus === 'error') return 'error'; @@ -44,10 +48,17 @@ function toStepOutcome(s: ServerStepHistory): StepOutcome { const outcomeType = stepTypeToOutcomeType(stepDef.type); const ctx = (s.context ?? {}) as Record; + // A value the executor didn't write (legacy frontend, or a newer executor's vocabulary) is dropped + // rather than passed on: AvailableStepExecutionSchema.parse below would fail the whole run. + const parsedErrorKind = ErrorKindSchema.safeParse(ctx.errorKind); + const parsedSourceStepIndex = ErrorSourceStepIndexSchema.safeParse(ctx.errorSourceStepIndex); + const baseFromCtx = { stepId: s.stepName, stepIndex: s.stepIndex, error: typeof ctx.error === 'string' ? ctx.error : undefined, + ...(parsedErrorKind.success && { errorKind: parsedErrorKind.data }), + ...(parsedSourceStepIndex.success && { errorSourceStepIndex: parsedSourceStepIndex.data }), }; if (outcomeType === 'condition') { diff --git a/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts b/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts index 3965001a14..a672a0fb70 100644 --- a/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts +++ b/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts @@ -27,6 +27,12 @@ export default function toUpdateStepRequest( ): ServerUpdateStepRequest { const context: Record = { status: outcome.status }; if (outcome.error !== undefined) context.error = outcome.error; + if (outcome.errorKind !== undefined) context.errorKind = outcome.errorKind; + + // Index 0 is a real step, so this cannot be a truthiness check. + if (outcome.errorSourceStepIndex !== undefined) { + context.errorSourceStepIndex = outcome.errorSourceStepIndex; + } if (outcome.type === 'condition' && outcome.selectedOption !== undefined) { context.selectedOption = outcome.selectedOption; diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 657afc5228..c8c85d0fab 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -1,7 +1,7 @@ /* eslint-disable max-classes-per-file */ import type { MalformedRunInfo } from './ports/workflow-port'; import type { RecordId } from './types/validated/collection'; -import type { AwaitingInputReason } from './types/validated/step-outcome'; +import type { AwaitingInputReason, ErrorKind } from './types/validated/step-outcome'; import type { z } from 'zod'; export function causeMessage(error: unknown): string | undefined { @@ -30,10 +30,19 @@ export abstract class WorkflowExecutorError extends Error { readonly userMessage: string; cause?: unknown; + // Who has to act. Subclasses declare it once via defaultErrorKind; the throw site overrides only + // where the same error has different audiences depending on why it was raised. + errorKind?: ErrorKind; + static readonly defaultErrorKind?: ErrorKind; + + // Set when the error is about a different step than the one being executed. + errorSourceStepIndex?: number; + constructor(message: string, userMessage?: string) { super(message); this.name = this.constructor.name; this.userMessage = userMessage ?? message; + this.errorKind = (this.constructor as typeof WorkflowExecutorError).defaultErrorKind; } } @@ -68,6 +77,8 @@ export class MalformedToolCallError extends WorkflowExecutorError { } export class RecordNotFoundError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'operator'; + constructor(collectionName: string, recordId: RecordId) { super( `Record not found: collection "${collectionName}", id "${recordId.join('|')}"`, @@ -77,12 +88,16 @@ export class RecordNotFoundError extends WorkflowExecutorError { } export class NoRecordsError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'operator'; + constructor() { super('No records available'); } } export class NoReadableFieldsError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(collectionName: string) { super( `No readable fields on record from collection "${collectionName}"`, @@ -101,6 +116,8 @@ export class NoResolvedFieldsError extends WorkflowExecutorError { } export class NoWritableFieldsError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(collectionName: string) { super( `No writable fields on record from collection "${collectionName}"`, @@ -110,6 +127,8 @@ export class NoWritableFieldsError extends WorkflowExecutorError { } export class NoActionsError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(collectionName: string) { super( `No actions available on collection "${collectionName}"`, @@ -131,6 +150,8 @@ export class UnsupportedActionFormError extends WorkflowExecutorError { // NOT an infra failure. Full AI treats this as a fallback-to-AI-assisted reason // so a human can fix the values and resubmit. export class ActionFormValidationError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'operator'; + constructor(actionName: string, cause?: unknown) { super( `Action "${actionName}" rejected the submitted form values`, @@ -145,6 +166,8 @@ export class ActionFormValidationError extends WorkflowExecutorError { // falls back to AI-assisted so the native front handles the approval flow. The executor // MUST NOT self-sign an approval request. export class ActionRequiresApprovalError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'operator'; + readonly roleIdsAllowedToApprove?: number[]; constructor(actionName: string, roleIdsAllowedToApprove?: number[]) { @@ -178,6 +201,8 @@ export class RunStorePortError extends UnavailableError { } export class NoRelationshipFieldsError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(collectionName: string) { super( `No relationship fields on record from collection "${collectionName}"`, @@ -187,6 +212,8 @@ export class NoRelationshipFieldsError extends WorkflowExecutorError { } export class RelatedRecordNotFoundError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'operator'; + constructor(collectionName: string, relationName: string) { super( `No related record found for relation "${relationName}" on collection "${collectionName}"`, @@ -202,12 +229,16 @@ export class InvalidAIResponseError extends WorkflowExecutorError { } export class InvalidAiRequestError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(message: string) { super(message, 'Step configuration error — please contact your administrator.'); } } export class RelationNotFoundError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(name: string, collectionName: string) { super( `Relation "${name}" not found in collection "${collectionName}"`, @@ -217,6 +248,8 @@ export class RelationNotFoundError extends WorkflowExecutorError { } export class FieldNotFoundError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(name: string, collectionName: string) { super( `Field "${name}" not found in collection "${collectionName}"`, @@ -226,6 +259,8 @@ export class FieldNotFoundError extends WorkflowExecutorError { } export class FieldTypeMissingError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(name: string, collectionName: string) { super( `Field "${name}" in collection "${collectionName}" has no column type`, @@ -236,6 +271,8 @@ export class FieldTypeMissingError extends WorkflowExecutorError { } export class ActionNotFoundError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(name: string, collectionName: string) { super( `Action "${name}" not found in collection "${collectionName}"`, @@ -486,6 +523,8 @@ export class InvalidPendingDataError extends WorkflowExecutorError { } export class InvalidPreRecordedArgsError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; + constructor(detail: string) { super(`Invalid pre-recorded args: ${detail}`, 'The pre-configured step parameters are invalid'); } @@ -494,13 +533,20 @@ export class InvalidPreRecordedArgsError extends WorkflowExecutorError { // A "Related to" / "On record" source step ran but loaded no record, so the step that uses it has // no source to act on ("no source record"). Distinct from a bad config — the // user can continue without. Wording is step-type-neutral (shared by load-related and trigger-action). +// The kind comes from the throw site: only there is it known whether the operator had a record to +// pick, which is what decides who can act on it. export class SourceRecordMissingError extends WorkflowExecutorError { - constructor(sourceTitle?: string) { + constructor( + sourceTitle?: string, + options: { errorKind?: ErrorKind; errorSourceStepIndex?: number } = {}, + ) { const from = sourceTitle ? `"${sourceTitle}"` : 'its source step'; super( `Source step ${from} loaded no record`, `This step uses ${from} as its source, but that step didn't load any record.`, ); + this.errorKind = options.errorKind; + this.errorSourceStepIndex = options.errorSourceStepIndex; } } diff --git a/packages/workflow-executor/src/executors/base-step-executor.ts b/packages/workflow-executor/src/executors/base-step-executor.ts index 60021c0c90..97657f496b 100644 --- a/packages/workflow-executor/src/executors/base-step-executor.ts +++ b/packages/workflow-executor/src/executors/base-step-executor.ts @@ -6,7 +6,7 @@ import type { import type { ConfirmableStepExecutionData, StepExecutionData } from '../types/step-execution-data'; import type { Step } from '../types/validated/execution'; import type { StepDefinition } from '../types/validated/step-definition'; -import type { StepStatus } from '../types/validated/step-outcome'; +import type { ErrorKind, StepStatus } from '../types/validated/step-outcome'; import type { BaseMessage, DynamicStructuredTool, @@ -75,7 +75,7 @@ export default abstract class BaseStepExecutor; protected checkIdempotency(): Promise { @@ -146,6 +159,8 @@ export default abstract class BaseStepExecutor( diff --git a/packages/workflow-executor/src/executors/condition-step-executor.ts b/packages/workflow-executor/src/executors/condition-step-executor.ts index 36580d0956..a18780d614 100644 --- a/packages/workflow-executor/src/executors/condition-step-executor.ts +++ b/packages/workflow-executor/src/executors/condition-step-executor.ts @@ -1,6 +1,6 @@ import type { StepExecutionResult } from '../types/execution-context'; import type { ConditionStepDefinition } from '../types/validated/step-definition'; -import type { ConditionStepOutcome } from '../types/validated/step-outcome'; +import type { ConditionStepOutcome, ErrorKind } from '../types/validated/step-outcome'; import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; import { z } from 'zod'; @@ -47,6 +47,8 @@ export default class ConditionStepExecutor extends BaseStepExecutor protected buildOutcomeResult(outcome: { status: RecordStepStatus; error?: string; + errorKind?: ErrorKind; + errorSourceStepIndex?: number; awaitingInputReason?: AwaitingInputReason; }): StepExecutionResult { return { diff --git a/packages/workflow-executor/src/executors/record-step-executor.ts b/packages/workflow-executor/src/executors/record-step-executor.ts index 5bc15ab656..003d431e35 100644 --- a/packages/workflow-executor/src/executors/record-step-executor.ts +++ b/packages/workflow-executor/src/executors/record-step-executor.ts @@ -1,7 +1,8 @@ import type { StepExecutionResult } from '../types/execution-context'; +import type { StepExecutionData } from '../types/step-execution-data'; import type { CollectionSchema, FieldSchema, RecordRef } from '../types/validated/collection'; import type { StepDefinition } from '../types/validated/step-definition'; -import type { RecordStepStatus } from '../types/validated/step-outcome'; +import type { ErrorKind, RecordStepStatus } from '../types/validated/step-outcome'; import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; import { z } from 'zod'; @@ -15,12 +16,33 @@ import { import BaseStepExecutor from './base-step-executor'; import { StepType, WORKFLOW_START_STEP_ID } from '../types/validated/step-definition'; +// The operator if they passed on a candidate the source step offered, whoever owns the workflow if +// it had none to offer. An unreadable execution is as likely our own bug, so it names nobody. +function classifyMissingSourceRecord(execution?: StepExecutionData): ErrorKind | undefined { + if (execution?.type !== 'load-related-record') return undefined; + + const { pendingData, executionResult } = execution; + + // Full AI continues without a record on its own judgment, having found no candidate to offer. + if (executionResult !== undefined) { + return 'skipped' in executionResult ? 'configuration' : undefined; + } + + // It paused instead, so the candidates it offered say whether the operator could have chosen + // otherwise — and without that list there is nothing to read the situation from. + if (!pendingData) return undefined; + + return pendingData.availableRecordIds.length > 0 ? 'operator' : 'configuration'; +} + export default abstract class RecordStepExecutor< TStep extends StepDefinition = StepDefinition, > extends BaseStepExecutor { protected buildOutcomeResult(outcome: { status: RecordStepStatus; error?: string; + errorKind?: ErrorKind; + errorSourceStepIndex?: number; }): StepExecutionResult { return { stepOutcome: { @@ -84,7 +106,10 @@ export default abstract class RecordStepExecutor< // The source step exists but loaded nothing → clear "no source record" message, // distinct from a config pointing at a non-existent step. - throw new SourceRecordMissingError(sourceStep.stepDefinition.title); + throw new SourceRecordMissingError(sourceStep.stepDefinition.title, { + errorKind: classifyMissingSourceRecord(execution), + errorSourceStepIndex: sourceStep.stepOutcome.stepIndex, + }); } throw new InvalidPreRecordedArgsError(`No source record found for step "${stepId}"`); diff --git a/packages/workflow-executor/src/types/validated/step-outcome.ts b/packages/workflow-executor/src/types/validated/step-outcome.ts index 35d086d2a0..ed9983c676 100644 --- a/packages/workflow-executor/src/types/validated/step-outcome.ts +++ b/packages/workflow-executor/src/types/validated/step-outcome.ts @@ -14,6 +14,15 @@ export type RecordStepStatus = z.infer; export const AwaitingInputReasonSchema = z.enum(['needs-oauth-reauth']); export type AwaitingInputReason = z.infer; +// Who has to act on a step error. All three cross the wire even though only 'operator' drives a UI +// branch today: widening an enum is cheap, changing a cross-service contract is not. +export const ErrorKindSchema = z.enum(['operator', 'configuration', 'system']); +export type ErrorKind = z.infer; + +// Identifies the step an error is about by index rather than by step id: a LinkTo loop repeats ids, +// so only the index says which iteration the error came from. +export const ErrorSourceStepIndexSchema = z.number().int().nonnegative(); + export type StepStatus = BaseStepStatus | RecordStepStatus; /** @@ -26,6 +35,10 @@ const baseOutcomeFields = { stepIndex: z.number().int().nonnegative(), /** Present when status is 'error'. */ error: z.string().optional(), + /** Present when the error has been classified. Absent leaves the error framed as it is today. */ + errorKind: ErrorKindSchema.optional(), + /** Present when the error is about another step, e.g. a source step that loaded no record. */ + errorSourceStepIndex: ErrorSourceStepIndexSchema.optional(), }; export const ConditionStepOutcomeSchema = z diff --git a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts index 97fb52c6b3..a8743fbc8b 100644 --- a/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts +++ b/packages/workflow-executor/test/adapters/forest-server-workflow-port.test.ts @@ -458,6 +458,44 @@ describe('ForestServerWorkflowPort', () => { ); }); + it('posts the classification and the source step alongside the error', async () => { + mockQuery.mockResolvedValue(undefined); + const stepOutcome: StepOutcome = { + type: 'record', + stepId: 'step-1', + stepIndex: 1, + status: 'error', + error: 'boom', + errorKind: 'operator', + errorSourceStepIndex: 0, + }; + + await port.updateStepExecution('42', stepOutcome); + + expect(mockQuery).toHaveBeenCalledWith( + options, + 'post', + '/api/workflow-orchestrator/update-step', + {}, + { + runId: 42, + stepUpdate: { + stepIndex: 1, + attributes: { + done: true, + context: { + status: 'error', + error: 'boom', + errorKind: 'operator', + errorSourceStepIndex: 0, + }, + }, + }, + executionStatus: { type: 'error', message: 'boom' }, + }, + ); + }); + it('posts the mapped body for an awaiting-input outcome', async () => { mockQuery.mockResolvedValue(undefined); const stepOutcome: StepOutcome = { diff --git a/packages/workflow-executor/test/adapters/run-to-available-step-mapper.test.ts b/packages/workflow-executor/test/adapters/run-to-available-step-mapper.test.ts index 4397c00e91..fd7c30996b 100644 --- a/packages/workflow-executor/test/adapters/run-to-available-step-mapper.test.ts +++ b/packages/workflow-executor/test/adapters/run-to-available-step-mapper.test.ts @@ -5,6 +5,7 @@ import type { ServerWorkflowCondition, ServerWorkflowTask, } from '../../src/adapters/server-types'; +import type { StepOutcome } from '../../src/types/validated/step-outcome'; import { z } from 'zod'; @@ -15,6 +16,7 @@ import { ServerTaskTypeEnum, ServerWorkflowTriggerType, } from '../../src/adapters/server-types'; +import toUpdateStepRequest from '../../src/adapters/step-outcome-to-update-step-mapper'; import { DomainValidationError, InvalidStepDefinitionError } from '../../src/errors'; import { TriggerType } from '../../src/types/validated/execution'; import { StepType } from '../../src/types/validated/step-definition'; @@ -519,6 +521,119 @@ describe('toAvailableStepExecution', () => { expect(() => toAvailableStepExecution(run)).toThrow(InvalidStepDefinitionError); }); + + describe('errorKind', () => { + it('should read errorKind back from the step context', () => { + const run = makeRun({ + workflowHistory: [ + makeStepHistory({ + stepName: 's0', + stepIndex: 0, + done: true, + context: { status: 'error', error: 'No records available', errorKind: 'operator' }, + }), + makeStepHistory({ stepName: 's1', stepIndex: 1, done: false }), + ], + }); + + const result = toAvailableStepExecution(run); + + expect(result?.previousSteps[0].stepOutcome).toEqual({ + type: 'record', + stepId: 's0', + stepIndex: 0, + status: 'error', + error: 'No records available', + errorKind: 'operator', + }); + }); + + // `context` is free-form on the wire and this mapper zod-parses what it builds, so an + // off-vocabulary kind must be dropped rather than fail the whole run. + it.each(['user', 42, null])( + 'should drop the errorKind %p instead of failing the run', + badKind => { + const run = makeRun({ + workflowHistory: [ + makeStepHistory({ + stepName: 's0', + stepIndex: 0, + done: true, + context: { status: 'error', error: 'No records available', errorKind: badKind }, + }), + makeStepHistory({ stepName: 's1', stepIndex: 1, done: false }), + ], + }); + + const result = toAvailableStepExecution(run); + + expect(result?.previousSteps[0].stepOutcome).not.toHaveProperty('errorKind'); + expect(result?.previousSteps[0].stepOutcome.status).toBe('error'); + }, + ); + + it.each(['2', -1, 1.5, null])( + 'should drop the errorSourceStepIndex %p instead of failing the run', + badIndex => { + const run = makeRun({ + workflowHistory: [ + makeStepHistory({ + stepName: 's0', + stepIndex: 0, + done: true, + context: { + status: 'error', + error: 'No records available', + errorSourceStepIndex: badIndex, + }, + }), + makeStepHistory({ stepName: 's1', stepIndex: 1, done: false }), + ], + }); + + const result = toAvailableStepExecution(run); + + expect(result?.previousSteps[0].stepOutcome).not.toHaveProperty('errorSourceStepIndex'); + expect(result?.previousSteps[0].stepOutcome.status).toBe('error'); + }, + ); + + // The two mappers are each other's inverse over `context`, and source index 0 is the case a + // falsy-value check on either side would silently drop. + it('should round trip errorKind and errorSourceStepIndex written by the forward mapper', () => { + const reported: StepOutcome = { + type: 'record', + stepId: 's1', + stepIndex: 1, + status: 'error', + error: 'The record no longer exists. It may have been deleted.', + errorKind: 'operator', + errorSourceStepIndex: 0, + }; + const { stepUpdate } = toUpdateStepRequest('42', reported); + const run = makeRun({ + workflowHistory: [ + makeStepHistory({ + stepName: 's0', + stepIndex: 0, + done: true, + context: { status: 'success' }, + }), + makeStepHistory({ + stepName: 's1', + stepIndex: 1, + done: true, + context: stepUpdate.attributes.context, + }), + makeStepHistory({ stepName: 's2', stepIndex: 2, done: false }), + ], + }); + + const result = toAvailableStepExecution(run); + + expect(result?.previousSteps[1].stepOutcome).toEqual(reported); + }); + }); }); describe('revision handling', () => { diff --git a/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts b/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts index 620cf111d0..6320c943f0 100644 --- a/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts +++ b/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts @@ -255,4 +255,116 @@ describe('toUpdateStepRequest', () => { expect(body.stepUpdate.attributes.context).toEqual({ status: 'success' }); }); }); + + describe('errorKind propagation', () => { + it('writes errorKind beside error in the update-step context', () => { + const outcome: StepOutcome = { + type: 'record', + stepId: 'step-1', + stepIndex: 2, + status: 'error', + error: 'The record no longer exists. It may have been deleted.', + errorKind: 'operator', + }; + + const body = toUpdateStepRequest('42', outcome); + + expect(body.stepUpdate.attributes).toEqual({ + done: true, + context: { + status: 'error', + error: 'The record no longer exists. It may have been deleted.', + errorKind: 'operator', + }, + }); + expect(body.executionStatus).toEqual({ + type: 'error', + message: 'The record no longer exists. It may have been deleted.', + }); + }); + + // Only 'operator' drives a UI branch today, but all three cross the wire — widening the enum + // later must not require another cross-service change. + it.each(['operator', 'configuration', 'system'] as const)('forwards the %s kind', kind => { + const outcome: StepOutcome = { + type: 'mcp', + stepId: 'step-1', + stepIndex: 0, + status: 'error', + error: 'The tool failed to execute.', + errorKind: kind, + }; + + const body = toUpdateStepRequest('42', outcome); + + expect(body.stepUpdate.attributes.context).toEqual({ + status: 'error', + error: 'The tool failed to execute.', + errorKind: kind, + }); + }); + + it('writes errorSourceStepIndex alongside the kind', () => { + const outcome: StepOutcome = { + type: 'record', + stepId: 'step-3', + stepIndex: 3, + status: 'error', + error: + 'This step uses "Load the order" as its source, but that step didn\'t load any record.', + errorKind: 'operator', + errorSourceStepIndex: 2, + }; + + const body = toUpdateStepRequest('42', outcome); + + expect(body.stepUpdate.attributes.context).toEqual({ + status: 'error', + error: + 'This step uses "Load the order" as its source, but that step didn\'t load any record.', + errorKind: 'operator', + errorSourceStepIndex: 2, + }); + }); + + // Index 0 is a real step, so a falsy-value check in the mapper would drop the first step of a run. + it('writes errorSourceStepIndex 0', () => { + const outcome: StepOutcome = { + type: 'record', + stepId: 'step-1', + stepIndex: 1, + status: 'error', + error: 'boom', + errorKind: 'operator', + errorSourceStepIndex: 0, + }; + + const body = toUpdateStepRequest('42', outcome); + + expect(body.stepUpdate.attributes.context).toEqual({ + status: 'error', + error: 'boom', + errorKind: 'operator', + errorSourceStepIndex: 0, + }); + }); + + it('sends the payload unchanged for an unclassified error', () => { + const outcome: StepOutcome = { + type: 'condition', + stepId: 'step-1', + stepIndex: 0, + status: 'error', + error: 'AI gateway unreachable', + }; + + const body = toUpdateStepRequest('7', outcome); + + expect(body.stepUpdate.attributes.context).toEqual({ + status: 'error', + error: 'AI gateway unreachable', + }); + expect(body.stepUpdate.attributes.context).not.toHaveProperty('errorKind'); + }); + }); }); diff --git a/packages/workflow-executor/test/errors.test.ts b/packages/workflow-executor/test/errors.test.ts index bdc98ef8a7..085d8422d9 100644 --- a/packages/workflow-executor/test/errors.test.ts +++ b/packages/workflow-executor/test/errors.test.ts @@ -1,11 +1,33 @@ +import type { WorkflowExecutorError } from '../src/errors'; + import { + ActionFormValidationError, + ActionNotFoundError, + ActionRequiresApprovalError, + AgentPortError, AiModelPortError, + FieldNotFoundError, + FieldTypeMissingError, + InvalidAiRequestError, InvalidPendingDataError, + InvalidPreRecordedArgsError, + MissingToolCallError, + NoActionsError, NoMcpToolsError, + NoReadableFieldsError, + NoRecordsError, + NoRelationshipFieldsError, + NoWritableFieldsError, OAuthInvalidGrantError, OAuthReauthRequiredError, OAuthRefreshError, PendingDataNotFoundError, + RecordNotFoundError, + RelatedRecordNotFoundError, + RelationNotFoundError, + SourceRecordMissingError, + StepStateError, + StepTimeoutError, causeMessage, extractErrorMessage, } from '../src/errors'; @@ -199,3 +221,74 @@ describe('OAuthInvalidGrantError', () => { expect(new OAuthInvalidGrantError('token expired').message).toMatch(/token expired/); }); }); + +describe('errorKind classification', () => { + // The operator can resolve these themselves — nothing is broken in the workflow or the agent. + it.each<[string, WorkflowExecutorError]>([ + ['NoRecordsError', new NoRecordsError()], + ['RecordNotFoundError', new RecordNotFoundError('customers', [42])], + ['RelatedRecordNotFoundError', new RelatedRecordNotFoundError('customers', 'orders')], + ['ActionFormValidationError', new ActionFormValidationError('send-welcome-email')], + ['ActionRequiresApprovalError', new ActionRequiresApprovalError('send-welcome-email')], + ])('classifies %s as operator', (_, error) => { + expect(error.errorKind).toBe('operator'); + }); + + // The workflow or the Forest Admin schema needs an edit, which the operator running the step cannot + // do. Every member's own userMessage already says so; the kind only makes it machine-readable. + it.each<[string, WorkflowExecutorError]>([ + ['FieldNotFoundError', new FieldNotFoundError('emailz', 'customers')], + ['ActionNotFoundError', new ActionNotFoundError('sned-email', 'customers')], + ['RelationNotFoundError', new RelationNotFoundError('orderz', 'customers')], + ['NoActionsError', new NoActionsError('customers')], + ['NoWritableFieldsError', new NoWritableFieldsError('customers')], + ['NoReadableFieldsError', new NoReadableFieldsError('customers')], + ['NoRelationshipFieldsError', new NoRelationshipFieldsError('customers')], + ['FieldTypeMissingError', new FieldTypeMissingError('status', 'customers')], + ['InvalidAiRequestError', new InvalidAiRequestError('SystemMessage at position 3')], + ['InvalidPreRecordedArgsError', new InvalidPreRecordedArgsError('no record at step index 4')], + ])('classifies %s as configuration', (_, error) => { + expect(error.errorKind).toBe('configuration'); + }); + + // Unclassified is the starting default: an error nobody has triaged keeps today's framing. + it.each<[string, WorkflowExecutorError]>([ + ['StepTimeoutError', new StepTimeoutError(30)], + ['AgentPortError', new AgentPortError('getRecord', new Error('ECONNREFUSED'))], + ['AiModelPortError', new AiModelPortError('invoke', new Error('timeout'))], + ['MissingToolCallError', new MissingToolCallError()], + ['StepStateError', new StepStateError('Step at index 0 has no pending data')], + ])('leaves %s unclassified', (_, error) => { + expect(error.errorKind).toBeUndefined(); + }); + + describe('SourceRecordMissingError', () => { + it('is unclassified and names no source step by default', () => { + const error = new SourceRecordMissingError('Load the order'); + + expect(error.errorKind).toBeUndefined(); + expect(error.errorSourceStepIndex).toBeUndefined(); + }); + + // The only error whose kind depends on why it was thrown: the throw site is the one place that + // knows whether the operator had a candidate to pick. + it.each(['operator', 'configuration'] as const)( + 'takes the %s kind from the throw site', + kind => { + const error = new SourceRecordMissingError('Load the order', { errorKind: kind }); + + expect(error.errorKind).toBe(kind); + expect(error.userMessage).toContain("didn't load any record"); + }, + ); + + it('carries the index of the source step the guard resolved', () => { + const error = new SourceRecordMissingError('Load the order', { + errorKind: 'operator', + errorSourceStepIndex: 4, + }); + + expect(error.errorSourceStepIndex).toBe(4); + }); + }); +}); diff --git a/packages/workflow-executor/test/executors/base-step-executor.test.ts b/packages/workflow-executor/test/executors/base-step-executor.test.ts index b2b17fd97e..45140117a0 100644 --- a/packages/workflow-executor/test/executors/base-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/base-step-executor.test.ts @@ -9,7 +9,11 @@ import type { StepExecutionData } from '../../src/types/step-execution-data'; import type { RecordRef } from '../../src/types/validated/collection'; import type { Step } from '../../src/types/validated/execution'; import type { StepDefinition } from '../../src/types/validated/step-definition'; -import type { BaseStepStatus, StepOutcome } from '../../src/types/validated/step-outcome'; +import type { + BaseStepStatus, + ErrorKind, + StepOutcome, +} from '../../src/types/validated/step-outcome'; import type { BaseMessage, DynamicStructuredTool } from '@forestadmin/ai-proxy'; import { HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; @@ -45,6 +49,7 @@ class TestableExecutor extends BaseStepExecutor { protected buildOutcomeResult(outcome: { status: BaseStepStatus; error?: string; + errorKind?: ErrorKind; }): StepExecutionResult { return { stepOutcome: { @@ -53,6 +58,7 @@ class TestableExecutor extends BaseStepExecutor { stepIndex: this.context.stepIndex, status: outcome.status, ...(outcome.error !== undefined && { error: outcome.error }), + ...(outcome.errorKind !== undefined && { errorKind: outcome.errorKind }), }, }; } @@ -407,21 +413,36 @@ describe('BaseStepExecutor', () => { }); describe('execute error handling', () => { - it('converts NoRecordsError to error outcome', async () => { + it('converts NoRecordsError to an operator-classified error outcome', async () => { const executor = new TestableExecutor(makeContext(), new NoRecordsError()); const result = await executor.execute(); expect(result.stepOutcome.status).toBe('error'); expect(result.stepOutcome.error).toBe('No records available'); + expect(result.stepOutcome.errorKind).toBe('operator'); + }); + + it('reports an unclassified error without an errorKind', async () => { + const executor = new TestableExecutor( + makeContext(), + new StepStateError('Step at index 0 has no pending data'), + ); + + const result = await executor.execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome).not.toHaveProperty('errorKind'); }); describe('unexpected error handling', () => { + // A thrown non-WorkflowExecutorError has no kind to carry — it keeps today's framing. it('returns error outcome instead of rethrowing', async () => { const executor = new TestableExecutor(makeContext(), new Error('db connection refused')); const result = await executor.execute(); expect(result.stepOutcome.status).toBe('error'); expect(result.stepOutcome.error).toBe('Unexpected error during step execution'); + expect(result.stepOutcome).not.toHaveProperty('errorKind'); }); it('logs the full error context when logger is provided', async () => { diff --git a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts index fdef521cd8..d6ad694d49 100644 --- a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts @@ -3,7 +3,10 @@ import type { AgentPort } from '../../src/ports/agent-port'; import type { RunStore } from '../../src/ports/run-store'; import type { WorkflowPort } from '../../src/ports/workflow-port'; import type { ExecutionContext } from '../../src/types/execution-context'; -import type { TriggerRecordActionStepExecutionData } from '../../src/types/step-execution-data'; +import type { + LoadRelatedRecordStepExecutionData, + TriggerRecordActionStepExecutionData, +} from '../../src/types/step-execution-data'; import type { CollectionSchema, RecordRef } from '../../src/types/validated/collection'; import type { Step } from '../../src/types/validated/execution'; import type { TriggerActionStepDefinition } from '../../src/types/validated/step-definition'; @@ -1989,27 +1992,185 @@ describe('TriggerRecordActionStepExecutor', () => { ); }); - it('errors when the pinned source step (a Load Related Record) loaded no record', async () => { - const agentPort = makeMockAgentPort(); - // The source Load Related Record step is on the live path but has no execution record stored - // (it loaded nothing) → SourceRecordMissingError, no action triggered. - const runStore = makeMockRunStore({ getStepExecutions: jest.fn().mockResolvedValue([]) }); - const context = makeContext({ - agentPort, - runStore, - previousSteps: [makeLoadRelatedPreviousStep(2)], - stepDefinition: makeStep({ - executionType: StepExecutionMode.FullyAutomated, - preRecordedArgs: { selectedRecordStepId: 'load-2', actionName: 'send-welcome-email' }, - }), + describe('a source step that loaded no record', () => { + const relation = { name: 'orders', displayName: 'Orders' }; + const oneCandidate = [{ recordId: [99], referenceFieldValue: 'Order #99' }]; + + // Every case here pins the action to the same Load Related Record source (step id 'load-2' at + // index 2) and varies only what that step left behind in the run store. + async function runPinnedToSource({ + executions = [], + executionType = StepExecutionMode.FullyAutomated, + selectedRecordStepId = 'load-2', + }: { + executions?: LoadRelatedRecordStepExecutionData[]; + executionType?: StepExecutionMode; + selectedRecordStepId?: string; + } = {}) { + const agentPort = makeMockAgentPort(); + const runStore = makeMockRunStore({ + getStepExecutions: jest.fn().mockResolvedValue(executions), + }); + const context = makeContext({ + agentPort, + runStore, + previousSteps: [makeLoadRelatedPreviousStep(2)], + stepDefinition: makeStep({ + executionType, + preRecordedArgs: { selectedRecordStepId, actionName: 'send-welcome-email' }, + }), + }); + + const { stepOutcome } = await new TriggerRecordActionStepExecutor(context).execute(); + + return { stepOutcome, agentPort }; + } + + it('errors without triggering the action when no execution record was stored', async () => { + const { stepOutcome, agentPort } = await runPinnedToSource(); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome.error).toContain("didn't load any record"); + expect(agentPort.executeAction).not.toHaveBeenCalled(); + // As likely our own missing run-store entry as anything the operator did, so it names nobody + // — but which step is implicated is known regardless. + expect(stepOutcome).not.toHaveProperty('errorKind'); + expect(stepOutcome.errorSourceStepIndex).toBe(2); }); - const executor = new TriggerRecordActionStepExecutor(context); - const result = await executor.execute(); + it('classifies a manually completed source as an operator error', async () => { + // Paused (pendingData saved, no executionResult), then completed out of band, which never + // comes back through the executor. A candidate was on the table and they passed on it. + const { stepOutcome, agentPort } = await runPinnedToSource({ + executions: [ + { + type: 'load-related-record', + stepIndex: 2, + pendingData: { + availableFields: [relation], + suggestedField: relation, + availableRecordIds: oneCandidate, + suggestNoRecord: true, + }, + }, + ], + }); - expect(result.stepOutcome.status).toBe('error'); - expect(result.stepOutcome.error).toContain("didn't load any record"); - expect(agentPort.executeAction).not.toHaveBeenCalled(); + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome.errorKind).toBe('operator'); + // A LinkTo loop repeats step ids, so the index is the only thing that identifies which + // iteration lost its record. + expect(stepOutcome.errorSourceStepIndex).toBe(2); + expect(agentPort.executeAction).not.toHaveBeenCalled(); + }); + + it('classifies a source that offered candidates with no AI suggestion as operator', async () => { + // A Manual source pause populates the candidate list and never sets suggestNoRecord, so the + // list is what says the operator had a choice — reading the flag would miss this. + const { stepOutcome } = await runPinnedToSource({ + executionType: StepExecutionMode.Manual, + executions: [ + { + type: 'load-related-record', + stepIndex: 2, + pendingData: { + availableFields: [relation], + suggestedField: relation, + availableRecordIds: oneCandidate, + }, + }, + ], + }); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome.errorKind).toBe('operator'); + expect(stepOutcome.errorSourceStepIndex).toBe(2); + }); + + it('classifies a source step id that matches no step as a configuration error', async () => { + const { stepOutcome, agentPort } = await runPinnedToSource({ + selectedRecordStepId: 'load-9', + }); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome.errorKind).toBe('configuration'); + // Nothing resolved, so there is no history entry to name. + expect(stepOutcome).not.toHaveProperty('errorSourceStepIndex'); + expect(agentPort.executeAction).not.toHaveBeenCalled(); + }); + + // The next two are the same empty relation seen from the two execution modes that reach it. + // They must agree: who has to act does not depend on which mode ran the source step. + it('classifies a relation the executor skipped with no candidates as configuration', async () => { + // Full AI found nothing to offer and continued on its own judgment (persistSkip). Nobody was + // there to decide, and the workflow routes a record-consuming step off an emptiable relation. + const { stepOutcome } = await runPinnedToSource({ + executions: [ + { + type: 'load-related-record', + stepIndex: 2, + executionParams: relation, + executionResult: { skipped: true }, + }, + ], + }); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome.error).toContain("didn't load any record"); + expect(stepOutcome.errorKind).toBe('configuration'); + }); + + it('classifies an acknowledged empty relation as configuration', async () => { + // Same empty relation, AI-assisted: the step paused with nothing to offer and the operator + // acknowledged it. They decided, but never had an alternative to decide between. + const { stepOutcome } = await runPinnedToSource({ + executionType: StepExecutionMode.AutomatedWithConfirmation, + executions: [ + { + type: 'load-related-record', + stepIndex: 2, + pendingData: { + availableFields: [relation], + suggestedField: relation, + availableRecordIds: [], + suggestNoRecord: true, + }, + }, + ], + }); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome.errorKind).toBe('configuration'); + expect(stepOutcome.errorSourceStepIndex).toBe(2); + }); + + it('leaves a source with neither a result nor a candidate list unclassified', async () => { + // Nothing to read the situation from: it neither finished nor recorded what it offered. + const { stepOutcome } = await runPinnedToSource({ + executions: [{ type: 'load-related-record', stepIndex: 2 }], + }); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome).not.toHaveProperty('errorKind'); + expect(stepOutcome.errorSourceStepIndex).toBe(2); + }); + + it('leaves a source with an unreadable result shape unclassified', async () => { + // A result the guard cannot read is as likely our own shape mismatch as anything that + // happened in the run, so it must not name a culprit. + const { stepOutcome } = await runPinnedToSource({ + executions: [ + { + type: 'load-related-record', + stepIndex: 2, + executionResult: { relation }, + } as unknown as LoadRelatedRecordStepExecutionData, + ], + }); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome).not.toHaveProperty('errorKind'); + }); }); }); diff --git a/packages/workflow-executor/test/types/step-outcome.test.ts b/packages/workflow-executor/test/types/step-outcome.test.ts index 419077e64b..0709dbe981 100644 --- a/packages/workflow-executor/test/types/step-outcome.test.ts +++ b/packages/workflow-executor/test/types/step-outcome.test.ts @@ -1,6 +1,10 @@ import { StepType } from '../../src/types/validated/step-definition'; import { + ConditionStepOutcomeSchema, + ErrorKindSchema, + GuidanceStepOutcomeSchema, McpStepOutcomeSchema, + RecordStepOutcomeSchema, stepTypeToOutcomeType, } from '../../src/types/validated/step-outcome'; @@ -77,3 +81,96 @@ describe('McpStepOutcomeSchema — awaitingInputReason', () => { ).toThrow(); }); }); + +describe('ErrorKindSchema', () => { + // All three kinds cross the wire even though only 'operator' drives a UI branch — the vocabulary + // is a cross-service contract, so it is pinned here rather than by server-side validation. + it('accepts the three kinds of the classification vocabulary', () => { + expect(ErrorKindSchema.parse('operator')).toBe('operator'); + expect(ErrorKindSchema.parse('configuration')).toBe('configuration'); + expect(ErrorKindSchema.parse('system')).toBe('system'); + }); + + it('rejects a kind outside the vocabulary', () => { + expect(() => ErrorKindSchema.parse('catastrophic')).toThrow(); + }); +}); + +describe('errorKind on step outcomes', () => { + const errored = { stepId: 'step-1', stepIndex: 0, status: 'error' as const, error: 'boom' }; + + // errorKind joins baseOutcomeFields, so a consumer reads the same key whatever the step type was. + it('is accepted on every outcome type', () => { + expect( + RecordStepOutcomeSchema.parse({ ...errored, type: 'record', errorKind: 'operator' }) + .errorKind, + ).toBe('operator'); + expect( + ConditionStepOutcomeSchema.parse({ + ...errored, + type: 'condition', + errorKind: 'configuration', + }).errorKind, + ).toBe('configuration'); + expect( + McpStepOutcomeSchema.parse({ ...errored, type: 'mcp', errorKind: 'system' }).errorKind, + ).toBe('system'); + expect( + GuidanceStepOutcomeSchema.parse({ ...errored, type: 'guidance', errorKind: 'operator' }) + .errorKind, + ).toBe('operator'); + }); + + it('is absent from an unclassified error outcome', () => { + const parsed = RecordStepOutcomeSchema.parse({ ...errored, type: 'record' }); + + expect(parsed.errorKind).toBeUndefined(); + }); + + it('rejects an unknown kind on an outcome', () => { + expect(() => + RecordStepOutcomeSchema.parse({ ...errored, type: 'record', errorKind: 'user' }), + ).toThrow(); + }); +}); + +describe('errorSourceStepIndex on step outcomes', () => { + const errored = { stepId: 'step-1', stepIndex: 3, status: 'error' as const, error: 'boom' }; + + it('carries the index of the step the error is about', () => { + const parsed = RecordStepOutcomeSchema.parse({ + ...errored, + type: 'record', + errorSourceStepIndex: 1, + }); + + expect(parsed.errorSourceStepIndex).toBe(1); + }); + + // The first step of a run is index 0, so the floor has to be inclusive. + it('accepts the first step of a run', () => { + const parsed = RecordStepOutcomeSchema.parse({ + ...errored, + type: 'record', + errorSourceStepIndex: 0, + }); + + expect(parsed.errorSourceStepIndex).toBe(0); + }); + + it('is absent when the error names no source step', () => { + const parsed = RecordStepOutcomeSchema.parse({ ...errored, type: 'record' }); + + expect(parsed.errorSourceStepIndex).toBeUndefined(); + }); + + it.each([-1, 1.5, '2'])('rejects %p as a step index', badIndex => { + expect(() => + RecordStepOutcomeSchema.parse({ + ...errored, + type: 'record', + errorSourceStepIndex: badIndex, + }), + ).toThrow(); + }); +}); From b8b26804ec09aab4dbd2c68175680c08a24db470 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 17 Aug 2026 19:18:17 +0200 Subject: [PATCH 2/6] fix(workflow-executor): classify a declined record as the operator's choice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The confirmation flow records a decline as a skipped result while keeping the candidate list, so reading the result shape before the list sorted an operator who passed on an offered record as a configuration problem — the inversion this classification exists to remove. Decide on the candidate list first; fall back to the result shape only when nothing was ever offered, which keeps an unreadable result naming nobody. Co-Authored-By: Claude Fable 5 --- packages/workflow-executor/CLAUDE.md | 1 + packages/workflow-executor/src/errors.ts | 2 +- .../src/executors/record-step-executor.ts | 12 ++++------ .../trigger-record-action-step-executor.ts | 4 +++- ...rigger-record-action-step-executor.test.ts | 24 +++++++++++++++++++ 5 files changed, 34 insertions(+), 9 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index ca274c5320..40138ed9a3 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -47,6 +47,7 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p - `WorkflowExecutorError` carries `message` (technical, logs) **and** `userMessage` (end-user, surfaced via `stepOutcome.error`). New subclasses must set a distinct, jargon-free `userMessage`. Request-level errors extend a *category* (`NotFoundError`/`AccessDeniedError`/`UnavailableError`) so `toHttpError` maps status by category — no per-error binding. - **Who has to act** — `errorKind` (`operator`/`configuration`/`system`) is declared once per subclass as `static defaultErrorKind`, and overridden at the throw site only where the same error has different audiences (`SourceRecordMissingError`, which decides on whether the operator had a candidate to pick). Unset ⇒ absent from the outcome ⇒ the front frames it as it always has, so leaving a new error unclassified is safe. `errorSourceStepIndex` names the step an error is *about*, by index rather than step id — a LinkTo loop repeats ids, so only the index identifies the iteration. - Both fields ride `context` on the update-step request and are read back by `run-to-available-step-mapper`, which drops an off-vocabulary value instead of passing it on: it would fail `AvailableStepExecutionSchema.parse` and take the whole run down. The front equality-matches `operator`, so a widened enum degrades an older front rather than breaking it. + - `errorKind` is unrelated to ai-proxy's `McpLoadFailureKind` (`auth`/`connection`/`unknown`, reported per server on the MCP `failures` channel): that one says where a tool load broke, this one says who has to act on a step. They are deliberately separate vocabularies — don't map one onto the other. - **`displayName` vs technical name** — AI tools/prompts use `displayName` (the admin-configured label end users write against), never `fieldName`. Map AI-returned display names back to technical names before any datasource op. - **Idempotency (mutating steps: update-record, trigger-action, mcp)** — write-ahead log in the RunStore: save `idempotencyPhase: 'executing'` before the side effect, `'done'` + `executionResult` after. On re-dispatch `(runId, stepIndex)`: `done` → rebuild success outcome without re-running or re-logging; `executing` → throw `StepStateError`. `checkIdempotency()` runs before `doExecute()`; the `executing` marker is set in the `beforeCall` thunk passed to `AgentWithLog` (after `createPending`) so a log-creation failure leaves no orphan marker. Non-mutating steps don't override it (replay is safe). - **Fetched steps must execute** — any step from `getAvailableRuns()` must run; silently dropping one breaks the orchestrator contract. The only allowed pre-filter is `inFlightRuns` dedup (keyed by `runId`, not step — a chain advances `stepId`). diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index c8c85d0fab..743ad32e3a 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -545,7 +545,7 @@ export class SourceRecordMissingError extends WorkflowExecutorError { `Source step ${from} loaded no record`, `This step uses ${from} as its source, but that step didn't load any record.`, ); - this.errorKind = options.errorKind; + this.errorKind = options.errorKind ?? this.errorKind; this.errorSourceStepIndex = options.errorSourceStepIndex; } } diff --git a/packages/workflow-executor/src/executors/record-step-executor.ts b/packages/workflow-executor/src/executors/record-step-executor.ts index 003d431e35..fe550051ce 100644 --- a/packages/workflow-executor/src/executors/record-step-executor.ts +++ b/packages/workflow-executor/src/executors/record-step-executor.ts @@ -23,15 +23,13 @@ function classifyMissingSourceRecord(execution?: StepExecutionData): ErrorKind | const { pendingData, executionResult } = execution; - // Full AI continues without a record on its own judgment, having found no candidate to offer. - if (executionResult !== undefined) { - return 'skipped' in executionResult ? 'configuration' : undefined; - } + // A result we cannot read is as likely our own bug as anything that happened in the run. + if (executionResult !== undefined && !('skipped' in executionResult)) return undefined; - // It paused instead, so the candidates it offered say whether the operator could have chosen - // otherwise — and without that list there is nothing to read the situation from. - if (!pendingData) return undefined; + // Nothing was ever offered: only Full AI continues without pausing, so no operator had a choice. + if (!pendingData) return executionResult !== undefined ? 'configuration' : undefined; + // Whether it paused or the operator declined, the candidates it offered say who had an alternative. return pendingData.availableRecordIds.length > 0 ? 'operator' : 'configuration'; } diff --git a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts index 3fe536b0f3..a35091b3c4 100644 --- a/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts +++ b/packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts @@ -7,7 +7,7 @@ import type { } from '../types/step-execution-data'; import type { ActionSchema, CollectionSchema, RecordRef } from '../types/validated/collection'; import type { TriggerActionStepDefinition } from '../types/validated/step-definition'; -import type { RecordStepStatus } from '../types/validated/step-outcome'; +import type { ErrorKind, RecordStepStatus } from '../types/validated/step-outcome'; import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; import { z } from 'zod'; @@ -51,6 +51,8 @@ export default class TriggerRecordActionStepExecutor extends RecordStepExecutor< protected override buildOutcomeResult(outcome: { status: RecordStepStatus; error?: string; + errorKind?: ErrorKind; + errorSourceStepIndex?: number; approvalRequest?: { id: string }; }): StepExecutionResult { return super.buildOutcomeResult(outcome); diff --git a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts index d6ad694d49..c9e965762d 100644 --- a/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/trigger-record-action-step-executor.test.ts @@ -2087,6 +2087,30 @@ describe('TriggerRecordActionStepExecutor', () => { expect(stepOutcome.errorSourceStepIndex).toBe(2); }); + it('classifies a declined confirmation with candidates as an operator error', async () => { + // The confirmation flow records a decline as a skipped result while keeping the candidate + // list, so the result shape alone would read this as nobody's choice. + const { stepOutcome } = await runPinnedToSource({ + executions: [ + { + type: 'load-related-record', + stepIndex: 2, + pendingData: { + availableFields: [relation], + suggestedField: relation, + availableRecordIds: oneCandidate, + }, + userConfirmation: { userConfirmed: false }, + executionResult: { skipped: true }, + }, + ], + }); + + expect(stepOutcome.status).toBe('error'); + expect(stepOutcome.errorKind).toBe('operator'); + expect(stepOutcome.errorSourceStepIndex).toBe(2); + }); + it('classifies a source step id that matches no step as a configuration error', async () => { const { stepOutcome, agentPort } = await runPinnedToSource({ selectedRecordStepId: 'load-9', From ed979765638b7e6904f62b0ff53f9e432a8d8397 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 18 Aug 2026 10:22:36 +0200 Subject: [PATCH 3/6] fix(workflow-executor): keep the error classification out of the AI step summary The previous-steps summary spreads every outcome field except stepId/stepIndex/type into the model's context, so the new classification would have reached it for any step that errored and was then continued manually. It addresses the operator and the UI: context.error already states that the record is absent, which is the only fact that constrains what a later step can write, and naming a culprit cannot change that. Co-Authored-By: Claude Fable 5 --- .../executors/summary/step-summary-builder.ts | 5 ++++- .../executors/step-summary-builder.test.ts | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/workflow-executor/src/executors/summary/step-summary-builder.ts b/packages/workflow-executor/src/executors/summary/step-summary-builder.ts index aafbbb6013..adee90133c 100644 --- a/packages/workflow-executor/src/executors/summary/step-summary-builder.ts +++ b/packages/workflow-executor/src/executors/summary/step-summary-builder.ts @@ -52,7 +52,10 @@ export default class StepSummaryBuilder { } } } else { - const { stepId, stepIndex, type, ...historyDetails } = stepOutcome; + // The classification addresses the operator and the UI, not the model: `error` already carries + // the only fact that constrains a later step, and naming a culprit cannot change what it writes. + const { stepId, stepIndex, type, errorKind, errorSourceStepIndex, ...historyDetails } = + stepOutcome; lines.push(` History: ${JSON.stringify(historyDetails)}`); } diff --git a/packages/workflow-executor/test/executors/step-summary-builder.test.ts b/packages/workflow-executor/test/executors/step-summary-builder.test.ts index 371111da20..156ea9d587 100644 --- a/packages/workflow-executor/test/executors/step-summary-builder.test.ts +++ b/packages/workflow-executor/test/executors/step-summary-builder.test.ts @@ -80,6 +80,26 @@ describe('StepSummaryBuilder', () => { expect(result).not.toContain('"type"'); }); + // A step that errored and was then completed manually reaches this branch, so the classification + // would otherwise land in the model's context. `error` already says the record is absent. + it('keeps the error classification out of History', () => { + const step = makeConditionStep('Pick one'); + const outcome = makeConditionOutcome('cond-1', 0, { + status: 'error', + error: 'that step did not load any record', + errorKind: 'operator', + errorSourceStepIndex: 2, + }); + + const result = StepSummaryBuilder.build(step, outcome, undefined); + + expect(result).toContain( + 'History: {"status":"error","error":"that step did not load any record"}', + ); + expect(result).not.toContain('errorKind'); + expect(result).not.toContain('errorSourceStepIndex'); + }); + it('includes selectedOption in History for condition steps', () => { const step = makeConditionStep('Approved?'); const outcome = makeConditionOutcome('cond-approval', 0, { selectedOption: 'Yes' }); From c39c577e2230c5f4e507ad998df5c99db987d002 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 18 Aug 2026 16:15:33 +0200 Subject: [PATCH 4/6] refactor(workflow-executor): inherit the configuration error kind Ten classes repeated the same defaultErrorKind declaration. They now extend WorkflowConfigurationError, which declares it once, so a new member of that family joins by extending the right base instead of remembering a line. A global default on WorkflowExecutorError would have been simpler still, but it would classify the ~25 errors that must stay unclassified for the framing to be unchanged. Also drops the who-has-to-act phrasing from the comments and the invariant: errorKind classifies the kind of failure, and mapping configuration and system to admin-phrased copy is a front-end default rather than a property of the enum. The classifier's comments no longer say 'our own bug' either, which named nobody in particular. Co-Authored-By: Claude Fable 5 --- packages/workflow-executor/CLAUDE.md | 2 +- packages/workflow-executor/src/errors.ts | 50 +++++++------------ .../src/executors/record-step-executor.ts | 10 ++-- .../src/types/validated/step-outcome.ts | 4 +- .../workflow-executor/test/errors.test.ts | 8 +-- 5 files changed, 30 insertions(+), 44 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 40138ed9a3..9f275e5c07 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -45,7 +45,7 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p - *Step-execution errors* extend `WorkflowExecutorError` → caught by `base-step-executor.ts`, turned into `stepOutcome.error`. Never reach HTTP. - *Boundary errors* (`ConfigurationError`, `PendingDataNotFoundError`, `AgentProbeError`, …) extend plain `Error` → caught at HTTP/Runner layer. They must **not** extend `WorkflowExecutorError` (or the base executor would swallow them). - `WorkflowExecutorError` carries `message` (technical, logs) **and** `userMessage` (end-user, surfaced via `stepOutcome.error`). New subclasses must set a distinct, jargon-free `userMessage`. Request-level errors extend a *category* (`NotFoundError`/`AccessDeniedError`/`UnavailableError`) so `toHttpError` maps status by category — no per-error binding. - - **Who has to act** — `errorKind` (`operator`/`configuration`/`system`) is declared once per subclass as `static defaultErrorKind`, and overridden at the throw site only where the same error has different audiences (`SourceRecordMissingError`, which decides on whether the operator had a candidate to pick). Unset ⇒ absent from the outcome ⇒ the front frames it as it always has, so leaving a new error unclassified is safe. `errorSourceStepIndex` names the step an error is *about*, by index rather than step id — a LinkTo loop repeats ids, so only the index identifies the iteration. + - **Error kind** — `errorKind` (`operator`/`configuration`/`system`) classifies what kind of failure a step error is. It does not encode ownership: the front maps `configuration`/`system` to admin-phrased copy as a reasonable default, which is a front-end choice, not a property of the enum. Declared once per family or class as `static defaultErrorKind` — the `configuration` family extends `WorkflowConfigurationError` — and overridden at the throw site only where the same error can be either kind (`SourceRecordMissingError`, on whether a candidate was offered). Unset ⇒ absent from the outcome ⇒ the front frames it as it always has, so leaving a new error unclassified is safe. `errorSourceStepIndex` names the step an error is *about*, by index rather than step id — a LinkTo loop repeats ids, so only the index identifies the iteration. - Both fields ride `context` on the update-step request and are read back by `run-to-available-step-mapper`, which drops an off-vocabulary value instead of passing it on: it would fail `AvailableStepExecutionSchema.parse` and take the whole run down. The front equality-matches `operator`, so a widened enum degrades an older front rather than breaking it. - `errorKind` is unrelated to ai-proxy's `McpLoadFailureKind` (`auth`/`connection`/`unknown`, reported per server on the MCP `failures` channel): that one says where a tool load broke, this one says who has to act on a step. They are deliberately separate vocabularies — don't map one onto the other. - **`displayName` vs technical name** — AI tools/prompts use `displayName` (the admin-configured label end users write against), never `fieldName`. Map AI-returned display names back to technical names before any datasource op. diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 743ad32e3a..f6ad80ea52 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -30,8 +30,8 @@ export abstract class WorkflowExecutorError extends Error { readonly userMessage: string; cause?: unknown; - // Who has to act. Subclasses declare it once via defaultErrorKind; the throw site overrides only - // where the same error has different audiences depending on why it was raised. + // The kind of failure. A family or a class declares it once via defaultErrorKind; the throw site + // overrides only where the same error can be either kind depending on why it was raised. errorKind?: ErrorKind; static readonly defaultErrorKind?: ErrorKind; @@ -55,6 +55,12 @@ export abstract class NotFoundError extends WorkflowExecutorError {} export abstract class AccessDeniedError extends WorkflowExecutorError {} export abstract class UnavailableError extends WorkflowExecutorError {} +// The workflow or the Forest Admin schema needs an edit: the step cannot succeed as configured, +// whatever the run does. Extending this is how a step-execution error joins that family. +export abstract class WorkflowConfigurationError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'configuration'; +} + export class MissingToolCallError extends WorkflowExecutorError { constructor() { super( @@ -95,9 +101,7 @@ export class NoRecordsError extends WorkflowExecutorError { } } -export class NoReadableFieldsError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class NoReadableFieldsError extends WorkflowConfigurationError { constructor(collectionName: string) { super( `No readable fields on record from collection "${collectionName}"`, @@ -115,9 +119,7 @@ export class NoResolvedFieldsError extends WorkflowExecutorError { } } -export class NoWritableFieldsError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class NoWritableFieldsError extends WorkflowConfigurationError { constructor(collectionName: string) { super( `No writable fields on record from collection "${collectionName}"`, @@ -126,9 +128,7 @@ export class NoWritableFieldsError extends WorkflowExecutorError { } } -export class NoActionsError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class NoActionsError extends WorkflowConfigurationError { constructor(collectionName: string) { super( `No actions available on collection "${collectionName}"`, @@ -200,9 +200,7 @@ export class RunStorePortError extends UnavailableError { } } -export class NoRelationshipFieldsError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class NoRelationshipFieldsError extends WorkflowConfigurationError { constructor(collectionName: string) { super( `No relationship fields on record from collection "${collectionName}"`, @@ -228,17 +226,13 @@ export class InvalidAIResponseError extends WorkflowExecutorError { } } -export class InvalidAiRequestError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class InvalidAiRequestError extends WorkflowConfigurationError { constructor(message: string) { super(message, 'Step configuration error — please contact your administrator.'); } } -export class RelationNotFoundError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class RelationNotFoundError extends WorkflowConfigurationError { constructor(name: string, collectionName: string) { super( `Relation "${name}" not found in collection "${collectionName}"`, @@ -247,9 +241,7 @@ export class RelationNotFoundError extends WorkflowExecutorError { } } -export class FieldNotFoundError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class FieldNotFoundError extends WorkflowConfigurationError { constructor(name: string, collectionName: string) { super( `Field "${name}" not found in collection "${collectionName}"`, @@ -258,9 +250,7 @@ export class FieldNotFoundError extends WorkflowExecutorError { } } -export class FieldTypeMissingError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class FieldTypeMissingError extends WorkflowConfigurationError { constructor(name: string, collectionName: string) { super( `Field "${name}" in collection "${collectionName}" has no column type`, @@ -270,9 +260,7 @@ export class FieldTypeMissingError extends WorkflowExecutorError { } } -export class ActionNotFoundError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class ActionNotFoundError extends WorkflowConfigurationError { constructor(name: string, collectionName: string) { super( `Action "${name}" not found in collection "${collectionName}"`, @@ -522,9 +510,7 @@ export class InvalidPendingDataError extends WorkflowExecutorError { } } -export class InvalidPreRecordedArgsError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'configuration'; - +export class InvalidPreRecordedArgsError extends WorkflowConfigurationError { constructor(detail: string) { super(`Invalid pre-recorded args: ${detail}`, 'The pre-configured step parameters are invalid'); } diff --git a/packages/workflow-executor/src/executors/record-step-executor.ts b/packages/workflow-executor/src/executors/record-step-executor.ts index fe550051ce..a883c7885b 100644 --- a/packages/workflow-executor/src/executors/record-step-executor.ts +++ b/packages/workflow-executor/src/executors/record-step-executor.ts @@ -16,20 +16,20 @@ import { import BaseStepExecutor from './base-step-executor'; import { StepType, WORKFLOW_START_STEP_ID } from '../types/validated/step-definition'; -// The operator if they passed on a candidate the source step offered, whoever owns the workflow if -// it had none to offer. An unreadable execution is as likely our own bug, so it names nobody. +// A source step that offered a candidate and was passed over is an operator situation; one that had +// nothing to offer is a configuration one. An execution the guard cannot read stays unclassified. function classifyMissingSourceRecord(execution?: StepExecutionData): ErrorKind | undefined { if (execution?.type !== 'load-related-record') return undefined; const { pendingData, executionResult } = execution; - // A result we cannot read is as likely our own bug as anything that happened in the run. + // A result outside the declared shape signals an executor defect, not a decision the run made. if (executionResult !== undefined && !('skipped' in executionResult)) return undefined; - // Nothing was ever offered: only Full AI continues without pausing, so no operator had a choice. + // Nothing was ever offered: only Full AI continues without pausing, so there was no choice to make. if (!pendingData) return executionResult !== undefined ? 'configuration' : undefined; - // Whether it paused or the operator declined, the candidates it offered say who had an alternative. + // Whether it paused or recorded a decline, the candidate list says whether there was a choice. return pendingData.availableRecordIds.length > 0 ? 'operator' : 'configuration'; } diff --git a/packages/workflow-executor/src/types/validated/step-outcome.ts b/packages/workflow-executor/src/types/validated/step-outcome.ts index ed9983c676..b7a774a710 100644 --- a/packages/workflow-executor/src/types/validated/step-outcome.ts +++ b/packages/workflow-executor/src/types/validated/step-outcome.ts @@ -14,8 +14,8 @@ export type RecordStepStatus = z.infer; export const AwaitingInputReasonSchema = z.enum(['needs-oauth-reauth']); export type AwaitingInputReason = z.infer; -// Who has to act on a step error. All three cross the wire even though only 'operator' drives a UI -// branch today: widening an enum is cheap, changing a cross-service contract is not. +// What kind of failure a step error is. All three cross the wire even though only 'operator' drives +// a UI branch today: widening an enum is cheap, changing a cross-service contract is not. export const ErrorKindSchema = z.enum(['operator', 'configuration', 'system']); export type ErrorKind = z.infer; diff --git a/packages/workflow-executor/test/errors.test.ts b/packages/workflow-executor/test/errors.test.ts index 085d8422d9..f3302bc5fc 100644 --- a/packages/workflow-executor/test/errors.test.ts +++ b/packages/workflow-executor/test/errors.test.ts @@ -223,7 +223,7 @@ describe('OAuthInvalidGrantError', () => { }); describe('errorKind classification', () => { - // The operator can resolve these themselves — nothing is broken in the workflow or the agent. + // The record or the submitted input is the problem — nothing is broken in the workflow or the agent. it.each<[string, WorkflowExecutorError]>([ ['NoRecordsError', new NoRecordsError()], ['RecordNotFoundError', new RecordNotFoundError('customers', [42])], @@ -234,8 +234,8 @@ describe('errorKind classification', () => { expect(error.errorKind).toBe('operator'); }); - // The workflow or the Forest Admin schema needs an edit, which the operator running the step cannot - // do. Every member's own userMessage already says so; the kind only makes it machine-readable. + // The step cannot succeed as configured, whatever the run does. Every member's own userMessage + // already says so; the kind only makes it machine-readable. it.each<[string, WorkflowExecutorError]>([ ['FieldNotFoundError', new FieldNotFoundError('emailz', 'customers')], ['ActionNotFoundError', new ActionNotFoundError('sned-email', 'customers')], @@ -271,7 +271,7 @@ describe('errorKind classification', () => { }); // The only error whose kind depends on why it was thrown: the throw site is the one place that - // knows whether the operator had a candidate to pick. + // knows whether a candidate was offered. it.each(['operator', 'configuration'] as const)( 'takes the %s kind from the throw site', kind => { From e6e29590ec89f4a03aa1665730d9bdc220b78b24 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 18 Aug 2026 16:50:00 +0200 Subject: [PATCH 5/6] refactor(workflow-executor): declare each error kind on one family base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operator members now extend WorkflowOperatorError, mirroring the configuration family, so the whole classification is two declarations instead of fifteen. Naming each abstract after the kind it declares makes no claim about a shared semantic type — which is what the five operator errors lack — while keeping one mechanism instead of two and making the classified set greppable by its base. Co-Authored-By: Claude Fable 5 --- packages/workflow-executor/CLAUDE.md | 2 +- packages/workflow-executor/src/errors.ts | 32 ++++++++++-------------- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 9f275e5c07..bffb794e91 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -45,7 +45,7 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p - *Step-execution errors* extend `WorkflowExecutorError` → caught by `base-step-executor.ts`, turned into `stepOutcome.error`. Never reach HTTP. - *Boundary errors* (`ConfigurationError`, `PendingDataNotFoundError`, `AgentProbeError`, …) extend plain `Error` → caught at HTTP/Runner layer. They must **not** extend `WorkflowExecutorError` (or the base executor would swallow them). - `WorkflowExecutorError` carries `message` (technical, logs) **and** `userMessage` (end-user, surfaced via `stepOutcome.error`). New subclasses must set a distinct, jargon-free `userMessage`. Request-level errors extend a *category* (`NotFoundError`/`AccessDeniedError`/`UnavailableError`) so `toHttpError` maps status by category — no per-error binding. - - **Error kind** — `errorKind` (`operator`/`configuration`/`system`) classifies what kind of failure a step error is. It does not encode ownership: the front maps `configuration`/`system` to admin-phrased copy as a reasonable default, which is a front-end choice, not a property of the enum. Declared once per family or class as `static defaultErrorKind` — the `configuration` family extends `WorkflowConfigurationError` — and overridden at the throw site only where the same error can be either kind (`SourceRecordMissingError`, on whether a candidate was offered). Unset ⇒ absent from the outcome ⇒ the front frames it as it always has, so leaving a new error unclassified is safe. `errorSourceStepIndex` names the step an error is *about*, by index rather than step id — a LinkTo loop repeats ids, so only the index identifies the iteration. + - **Error kind** — `errorKind` (`operator`/`configuration`/`system`) classifies what kind of failure a step error is. It does not encode ownership: the front maps `configuration`/`system` to admin-phrased copy as a reasonable default, which is a front-end choice, not a property of the enum. One abstract per classified kind declares it once (`WorkflowOperatorError`, `WorkflowConfigurationError`, each setting `static defaultErrorKind`); a new member joins a family by extending it, and an error extending neither stays unclassified. The throw site overrides only where the same error can be either kind (`SourceRecordMissingError`, on whether a candidate was offered). Unset ⇒ absent from the outcome ⇒ the front frames it as it always has, so leaving a new error unclassified is safe. `errorSourceStepIndex` names the step an error is *about*, by index rather than step id — a LinkTo loop repeats ids, so only the index identifies the iteration. - Both fields ride `context` on the update-step request and are read back by `run-to-available-step-mapper`, which drops an off-vocabulary value instead of passing it on: it would fail `AvailableStepExecutionSchema.parse` and take the whole run down. The front equality-matches `operator`, so a widened enum degrades an older front rather than breaking it. - `errorKind` is unrelated to ai-proxy's `McpLoadFailureKind` (`auth`/`connection`/`unknown`, reported per server on the MCP `failures` channel): that one says where a tool load broke, this one says who has to act on a step. They are deliberately separate vocabularies — don't map one onto the other. - **`displayName` vs technical name** — AI tools/prompts use `displayName` (the admin-configured label end users write against), never `fieldName`. Map AI-returned display names back to technical names before any datasource op. diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index f6ad80ea52..ebbced99c3 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -30,8 +30,8 @@ export abstract class WorkflowExecutorError extends Error { readonly userMessage: string; cause?: unknown; - // The kind of failure. A family or a class declares it once via defaultErrorKind; the throw site - // overrides only where the same error can be either kind depending on why it was raised. + // The kind of failure, declared once by each family below via defaultErrorKind. The throw site + // overrides it only where the same error can be either kind depending on why it was raised. errorKind?: ErrorKind; static readonly defaultErrorKind?: ErrorKind; @@ -55,12 +55,16 @@ export abstract class NotFoundError extends WorkflowExecutorError {} export abstract class AccessDeniedError extends WorkflowExecutorError {} export abstract class UnavailableError extends WorkflowExecutorError {} -// The workflow or the Forest Admin schema needs an edit: the step cannot succeed as configured, -// whatever the run does. Extending this is how a step-execution error joins that family. +// One abstract per classified kind: the family declares it once and a new member joins by extending +// it. An error extending neither stays unclassified, which is what preserves today's framing. export abstract class WorkflowConfigurationError extends WorkflowExecutorError { static override readonly defaultErrorKind: ErrorKind = 'configuration'; } +export abstract class WorkflowOperatorError extends WorkflowExecutorError { + static override readonly defaultErrorKind: ErrorKind = 'operator'; +} + export class MissingToolCallError extends WorkflowExecutorError { constructor() { super( @@ -82,9 +86,7 @@ export class MalformedToolCallError extends WorkflowExecutorError { } } -export class RecordNotFoundError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'operator'; - +export class RecordNotFoundError extends WorkflowOperatorError { constructor(collectionName: string, recordId: RecordId) { super( `Record not found: collection "${collectionName}", id "${recordId.join('|')}"`, @@ -93,9 +95,7 @@ export class RecordNotFoundError extends WorkflowExecutorError { } } -export class NoRecordsError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'operator'; - +export class NoRecordsError extends WorkflowOperatorError { constructor() { super('No records available'); } @@ -149,9 +149,7 @@ export class UnsupportedActionFormError extends WorkflowExecutorError { // The action submission was rejected by the agent's server-side validation (bad/missing values), // NOT an infra failure. Full AI treats this as a fallback-to-AI-assisted reason // so a human can fix the values and resubmit. -export class ActionFormValidationError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'operator'; - +export class ActionFormValidationError extends WorkflowOperatorError { constructor(actionName: string, cause?: unknown) { super( `Action "${actionName}" rejected the submitted form values`, @@ -165,9 +163,7 @@ export class ActionFormValidationError extends WorkflowExecutorError { // CustomActionRequiresApprovalError. Distinct from a plain permission 403 — Full AI // falls back to AI-assisted so the native front handles the approval flow. The executor // MUST NOT self-sign an approval request. -export class ActionRequiresApprovalError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'operator'; - +export class ActionRequiresApprovalError extends WorkflowOperatorError { readonly roleIdsAllowedToApprove?: number[]; constructor(actionName: string, roleIdsAllowedToApprove?: number[]) { @@ -209,9 +205,7 @@ export class NoRelationshipFieldsError extends WorkflowConfigurationError { } } -export class RelatedRecordNotFoundError extends WorkflowExecutorError { - static override readonly defaultErrorKind: ErrorKind = 'operator'; - +export class RelatedRecordNotFoundError extends WorkflowOperatorError { constructor(collectionName: string, relationName: string) { super( `No related record found for relation "${relationName}" on collection "${collectionName}"`, From 5d19d43e1225f0039f73f937512440600b82dbde Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Thu, 20 Aug 2026 12:12:42 +0200 Subject: [PATCH 6/6] docs(workflow-executor): correct how record steps pin their source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four record steps check selectedRecordStepId first and resolve it through resolveSourceRecordRef; the runtime index is only a fallback on read-record and update-record. The previous wording said the opposite for those two, which led a review to conclude the editor could not pin a Get Data step by step id — it can, and it requires one before fields can be chosen. Co-Authored-By: Claude Fable 5 --- packages/workflow-executor/CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index bffb794e91..3cefa25014 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -36,7 +36,7 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p `StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`. - **Trigger Action** (`trigger-record-action-step-executor.ts`, `handleFirstCall`) — detects the form via `getActionForm` (full field list, not `getActionFormInfo`). Formless: `FullyAutomated` runs it *in the executor* via the audited agent; otherwise pauses. With a form: `Manual` pauses with the native form (no AI fill); `AutomatedWithConfirmation` AI-fills then pauses for the user to submit natively; `FullyAutomated` AI-fills (`fillFormWithAi`) and, if `filledForm.canExecute`, submits in the executor — falling back to `awaiting-input` (pause) when required fields are missing, or on `ActionFormValidationError`/`ActionRequiresApprovalError` (a human can finish those). `UnsupportedActionFormError` is declared/exported but **never thrown** in src. -- **Pre-recorded args** — record steps accept `preRecordedArgs` to skip AI. Technical names (`fieldName`/`fieldNames`/`actionName`/`relationName`) are matched exactly via `findFieldByTechnicalName` (no fuzz); `resolveAiFieldName` (exact-then-normalized) is reserved for AI-returned display names. The source record differs by step: read-record/update-record use `selectedRecordStepIndex` (a runtime index, resolved in `resolveRecordRef`); trigger-action/load-related-record use `selectedRecordStepId` — a **stable BPMN step id** (or `WORKFLOW_START_STEP_ID` sentinel) resolved by `resolveSourceRecordRef`, chosen to survive the index shifts a revision causes. Partial args supported. Unresolvable → `FieldNotFoundError`/`ActionNotFoundError`/`RelationNotFoundError`; bad shape / out-of-range index → `InvalidPreRecordedArgsError`. +- **Pre-recorded args** — record steps accept `preRecordedArgs` to skip AI. Technical names (`fieldName`/`fieldNames`/`actionName`/`relationName`) are matched exactly via `findFieldByTechnicalName` (no fuzz); `resolveAiFieldName` (exact-then-normalized) is reserved for AI-returned display names. All four record steps pin the source by `selectedRecordStepId` — a **stable BPMN step id** (or the `WORKFLOW_START_STEP_ID` sentinel) resolved by `resolveSourceRecordRef`, chosen to survive the index shifts a revision causes; the editor writes it and treats it as a precondition for choosing fields. `selectedRecordStepIndex` (a runtime index, resolved in `resolveRecordRef`) survives only as a fallback on read-record/update-record, checked after the step id. Partial args supported. Unresolvable → `FieldNotFoundError`/`ActionNotFoundError`/`RelationNotFoundError`; bad shape / out-of-range index → `InvalidPreRecordedArgsError`. ## Invariants (read before changing executors)