From f354be500c062373accb9698e2d5e01a505aff77 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 18 Aug 2026 22:09:50 +0200 Subject: [PATCH 1/8] feat(workflow-executor): evaluate deterministic condition steps without AI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decision steps in the new Automatic mode carry their branching logic as build-time preRecordedArgs (optionConditions + fallbackOption, wire-final operator names from the PRD-472 contract). The executor now resolves each condition's value from the run's Get Data outputs and evaluates top-to-bottom, first-match-wins — never calling the AI and never awaiting input, because the builder chose this mode precisely to remove AI judgement from the branch. A null/missing/unresolvable value is "not met" (met: null), never an error, and no match selects the fallback, so the step can never end undefined. The evaluation trace is persisted in executionParams for the run view; unknown operators are rejected at the schema boundary so a run never reaches evaluation with a comparison it cannot honor. Part of PRD-472 Co-Authored-By: Claude Opus 5 (1M context) --- packages/workflow-executor/CLAUDE.md | 4 +- .../src/adapters/server-types.ts | 21 +- .../src/adapters/step-definition-mapper.ts | 1 + .../src/executors/condition-step-executor.ts | 93 ++++- .../deterministic-condition-evaluator.ts | 127 ++++++ .../src/types/step-execution-data.ts | 19 +- .../src/types/validated/step-definition.ts | 72 +++- .../adapters/step-definition-mapper.test.ts | 46 ++- .../executors/condition-step-executor.test.ts | 367 +++++++++++++++++- .../deterministic-condition-evaluator.test.ts | 216 +++++++++++ .../test/types/step-definition.test.ts | 123 +++++- 11 files changed, 1068 insertions(+), 21 deletions(-) create mode 100644 packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts create mode 100644 packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 49e30e2fe3..4d51699b59 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -33,7 +33,9 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p ## Step types & execution modes -`StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`. +`StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`, `Deterministic` (condition steps only). + +- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (schema-required when the mode is deterministic; wire-final operator names in `CONDITION_OPERATORS`) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input. Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. - **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`. diff --git a/packages/workflow-executor/src/adapters/server-types.ts b/packages/workflow-executor/src/adapters/server-types.ts index 101fbd67c3..9973038409 100644 --- a/packages/workflow-executor/src/adapters/server-types.ts +++ b/packages/workflow-executor/src/adapters/server-types.ts @@ -35,6 +35,7 @@ export enum ServerStepExecutionTypeEnum { Manual = 'manual', AutomatedWithConfirmation = 'automated-with-confirmation', FullyAutomated = 'fully-automated', + Deterministic = 'deterministic', } interface ServerWorkflowStepBase { @@ -122,9 +123,27 @@ export interface ServerWorkflowEnd extends ServerWorkflowStepBase { export interface ServerWorkflowCondition extends ServerWorkflowStepBase { type: ServerStepTypeEnum.Condition; - executionType: ServerStepExecutionTypeEnum.Manual | ServerStepExecutionTypeEnum.FullyAutomated; + executionType: + | ServerStepExecutionTypeEnum.Manual + | ServerStepExecutionTypeEnum.FullyAutomated + | ServerStepExecutionTypeEnum.Deterministic; prompt: string | null; automaticCompletion: false; + // Parsed from `forest:optionConditions` server-side (flowId → answer). Present when + // executionType is deterministic. Validated by the step-definition schema. + preRecordedArgs?: { + optionConditions: Array<{ + option: string; + aggregator: 'and' | 'or'; + conditions: Array<{ + sourceStepId: string; + fieldName: string; + operator: string; + value?: unknown; + }>; + }>; + fallbackOption: string; + }; } export interface ServerWorkflowEscalation extends ServerWorkflowStepBase { diff --git a/packages/workflow-executor/src/adapters/step-definition-mapper.ts b/packages/workflow-executor/src/adapters/step-definition-mapper.ts index bf809ed538..cd56b81cec 100644 --- a/packages/workflow-executor/src/adapters/step-definition-mapper.ts +++ b/packages/workflow-executor/src/adapters/step-definition-mapper.ts @@ -103,6 +103,7 @@ function mapCondition(condition: ServerWorkflowCondition): ConditionStepDefiniti executionType: condition.executionType, title: condition.title, options, + preRecordedArgs: condition.preRecordedArgs, }); } diff --git a/packages/workflow-executor/src/executors/condition-step-executor.ts b/packages/workflow-executor/src/executors/condition-step-executor.ts index 36580d0956..2b3fef9591 100644 --- a/packages/workflow-executor/src/executors/condition-step-executor.ts +++ b/packages/workflow-executor/src/executors/condition-step-executor.ts @@ -1,5 +1,9 @@ import type { StepExecutionResult } from '../types/execution-context'; -import type { ConditionStepDefinition } from '../types/validated/step-definition'; +import type { ConditionEvaluation, StepExecutionData } from '../types/step-execution-data'; +import type { + ConditionStepDefinition, + DeterministicCondition, +} from '../types/validated/step-definition'; import type { ConditionStepOutcome } from '../types/validated/step-outcome'; import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; @@ -7,8 +11,9 @@ import { z } from 'zod'; import { StepStateError } from '../errors'; import BaseStepExecutor from './base-step-executor'; +import evaluateOperator from './deterministic-condition-evaluator'; import patchBodySchemas from '../http/pending-data-validators'; -import { StepExecutionMode } from '../types/validated/step-definition'; +import { StepExecutionMode, StepType } from '../types/validated/step-definition'; interface GatewayToolArgs { option: string | null; @@ -62,6 +67,12 @@ export default class ConditionStepExecutor extends BaseStepExecutor { const { stepDefinition: step, incomingPendingData } = this.context; + // Deterministic mode: pure evaluation of build-time conditions against the run's step + // history — never calls the AI, never awaits input. + if (step.executionType === StepExecutionMode.Deterministic) { + return this.evaluateDeterministically(step); + } + // Manual mode: the user picks the option from the frontend. Wait for their input // without ever calling the AI. const isManual = step.executionType === StepExecutionMode.Manual; @@ -92,6 +103,84 @@ export default class ConditionStepExecutor extends BaseStepExecutor { + // Guaranteed by the schema's superRefine for the deterministic mode. + const { optionConditions, fallbackOption } = step.preRecordedArgs!; + const stepExecutions = await this.context.runStore.getStepExecutions(this.context.runId); + + let matchedOption: string | undefined; + const evaluations = optionConditions.map(({ option, aggregator, conditions }) => { + if (matchedOption !== undefined) { + return { option, outcome: 'not-evaluated' } satisfies ConditionEvaluation; + } + + const results = conditions.map((condition, index) => ({ + index, + met: this.evaluateCondition(condition, stepExecutions), + })); + const matched = + aggregator === 'or' + ? results.some(result => result.met === true) + : results.every(result => result.met === true); + if (matched) matchedOption = option; + + return { + option, + outcome: matched ? 'matched' : 'not-matched', + conditions: results, + } satisfies ConditionEvaluation; + }); + + const usedFallback = matchedOption === undefined; + const selectedOption = matchedOption ?? fallbackOption; + + await this.context.runStore.saveStepExecution(this.context.runId, { + type: 'condition', + stepIndex: this.context.stepIndex, + executionParams: { evaluations, selectedOption, usedFallback }, + executionResult: { answer: selectedOption }, + }); + + return this.buildOutcomeResult({ status: 'success', selectedOption }); + } + + private evaluateCondition( + condition: DeterministicCondition, + stepExecutions: StepExecutionData[], + ): boolean | null { + const resolved = this.resolveConditionValue(condition, stepExecutions); + // Unresolvable reference (step never ran, field not read, read error) → not evaluable, even + // for present/blank — a value that was never read is not the same as a blank one. + if (!resolved.found) return null; + + return evaluateOperator(condition.operator, resolved.value, condition.value); + } + + // Same live-path + most-recent-occurrence resolution as resolveSourceRecordRef: previousSteps + // are already restricted to the live path, and in a loop the same step id repeats. + private resolveConditionValue( + condition: DeterministicCondition, + stepExecutions: StepExecutionData[], + ): { found: true; value: unknown } | { found: false } { + const matches = this.context.previousSteps.filter( + step => + step.stepDefinition.type === StepType.ReadRecord && + step.stepOutcome.stepId === condition.sourceStepId, + ); + const sourceStep = matches[matches.length - 1]; + if (!sourceStep) return { found: false }; + + const execution = this.resolveStepExecution(sourceStep, stepExecutions); + if (execution?.type !== 'read-record') return { found: false }; + + const field = execution.executionResult.fields.find(f => f.name === condition.fieldName); + if (!field || !('value' in field)) return { found: false }; + + return { found: true, value: field.value }; + } + private readUserChoice( step: ConditionStepDefinition, incomingPendingData: unknown, diff --git a/packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts b/packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts new file mode 100644 index 0000000000..8501a2f38f --- /dev/null +++ b/packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts @@ -0,0 +1,127 @@ +import type { ConditionOperator } from '../types/validated/step-definition'; + +// Guard against Date.parse's laxity ("5" parses as a year in some engines): only strings that +// start like an ISO date are treated as dates. +const ISO_DATE_PREFIX = /^\d{4}-\d{2}-\d{2}/; + +function toTimestamp(value: unknown): number | null { + if (typeof value !== 'string' || !ISO_DATE_PREFIX.test(value)) return null; + + const parsed = Date.parse(value); + + return Number.isNaN(parsed) ? null : parsed; +} + +function scalarEqual(actual: unknown, expected: unknown): boolean { + if (actual === expected) return true; + + const actualTs = toTimestamp(actual); + const expectedTs = toTimestamp(expected); + + return actualTs !== null && expectedTs !== null && actualTs === expectedTs; +} + +function isEqual(actual: unknown, expected: unknown): boolean { + if (Array.isArray(actual) && Array.isArray(expected)) { + return ( + actual.length === expected.length && + actual.every((item, index) => scalarEqual(item, expected[index])) + ); + } + + if (Array.isArray(actual) || Array.isArray(expected)) return false; + + return scalarEqual(actual, expected); +} + +function compare(actual: unknown, expected: unknown): number | null { + if (typeof actual === 'number' && typeof expected === 'number') { + if (Number.isNaN(actual) || Number.isNaN(expected)) return null; + + return actual - expected; + } + + const actualTs = toTimestamp(actual); + const expectedTs = toTimestamp(expected); + if (actualTs !== null && expectedTs !== null) return actualTs - expectedTs; + + return null; +} + +function isPresent(value: unknown): boolean { + if (value === null || value === undefined) return false; + if (typeof value === 'string') return value.length > 0; + if (Array.isArray(value)) return value.length > 0; + + return true; +} + +function isMemberOf(list: unknown, candidate: unknown): boolean { + return Array.isArray(list) && list.some(item => scalarEqual(item, candidate)); +} + +/** + * Pure evaluation of one deterministic condition. Never throws for data reasons: + * - `null` = not evaluable (the resolved value is null/missing) — treated as "not met"; + * - a type-mismatched comparison (including for negated operators) is "not met" (`false`), + * so a broken config can never accidentally satisfy a condition. + */ +export default function evaluateOperator( + operator: ConditionOperator, + actual: unknown, + expected: unknown, +): boolean | null { + if (operator === 'present') return isPresent(actual); + if (operator === 'blank') return !isPresent(actual); + if (actual === null || actual === undefined) return null; + + switch (operator) { + case 'equal': + return isEqual(actual, expected); + case 'not_equal': + return !isEqual(actual, expected); + + case 'greater_than': { + const diff = compare(actual, expected); + + return diff !== null && diff > 0; + } + + case 'less_than': { + const diff = compare(actual, expected); + + return diff !== null && diff < 0; + } + + case 'greater_than_or_equal': { + const diff = compare(actual, expected); + + return diff !== null && diff >= 0; + } + + case 'less_than_or_equal': { + const diff = compare(actual, expected); + + return diff !== null && diff <= 0; + } + + case 'in': + return isMemberOf(expected, actual); + case 'not_in': + return Array.isArray(expected) && !isMemberOf(expected, actual); + case 'contains': + if (typeof actual === 'string' && typeof expected === 'string') { + return actual.includes(expected); + } + + return isMemberOf(actual, expected); + case 'not_contains': + if (typeof actual === 'string' && typeof expected === 'string') { + return !actual.includes(expected); + } + + return Array.isArray(actual) && !isMemberOf(actual, expected); + default: + return null; + } +} diff --git a/packages/workflow-executor/src/types/step-execution-data.ts b/packages/workflow-executor/src/types/step-execution-data.ts index 80da7c91cc..3d5a683123 100644 --- a/packages/workflow-executor/src/types/step-execution-data.ts +++ b/packages/workflow-executor/src/types/step-execution-data.ts @@ -27,9 +27,26 @@ export interface WithUserConfirmation = Record // -- Condition -- +export interface ConditionEvaluation { + option: string; + outcome: 'matched' | 'not-matched' | 'not-evaluated'; + /** Absent when outcome is 'not-evaluated'. `met: null` = the value could not be evaluated. */ + conditions?: Array<{ index: number; met: boolean | null }>; +} + +// Deterministic evaluation trace read by the run view (PRD-472 contract shape). The fallback +// never appears in `evaluations` — the front derives its display from `usedFallback`. +export interface DeterministicConditionExecutionParams { + evaluations: ConditionEvaluation[]; + selectedOption: string; + usedFallback: boolean; +} + export interface ConditionStepExecutionData extends BaseStepExecutionData { type: 'condition'; - executionParams: { answer: string | null; reasoning?: string }; + executionParams: + | { answer: string | null; reasoning?: string } + | DeterministicConditionExecutionParams; executionResult?: { answer: string }; } diff --git a/packages/workflow-executor/src/types/validated/step-definition.ts b/packages/workflow-executor/src/types/validated/step-definition.ts index dc80f540f5..4501a118be 100644 --- a/packages/workflow-executor/src/types/validated/step-definition.ts +++ b/packages/workflow-executor/src/types/validated/step-definition.ts @@ -18,6 +18,7 @@ export enum StepExecutionMode { Manual = 'manual', AutomatedWithConfirmation = 'automated-with-confirmation', FullyAutomated = 'fully-automated', + Deterministic = 'deterministic', } // Shared fields across all step types. executionType is intentionally excluded — @@ -38,17 +39,70 @@ const sharedFields = { }; // Use z.enum(EnumObject), not z.nativeEnum — the latter is deprecated in zod 4. -const { Manual, AutomatedWithConfirmation, FullyAutomated } = StepExecutionMode; +const { Manual, AutomatedWithConfirmation, FullyAutomated, Deterministic } = StepExecutionMode; -export const ConditionStepDefinitionSchema = z.object({ - ...sharedFields, - type: z.literal(StepType.Condition), - // NO `.catch` — coercing an unknown mode (e.g. a future `deterministic` from a newer - // orchestrator) to FullyAutomated would silently let the AI decide instead of the conditions - // the builder configured precisely because they don't trust the AI. - executionType: z.enum([Manual, FullyAutomated]).default(FullyAutomated), - options: z.array(z.string()).min(2), +// Wire-final operator names (PRD-472 cross-repo contract). An unknown operator is rejected here, +// at the schema boundary, so a run never reaches evaluation with a comparison it cannot honor. +export const CONDITION_OPERATORS = [ + 'equal', + 'not_equal', + 'present', + 'blank', + 'greater_than', + 'less_than', + 'greater_than_or_equal', + 'less_than_or_equal', + 'in', + 'not_in', + 'contains', + 'not_contains', +] as const; +export type ConditionOperator = (typeof CONDITION_OPERATORS)[number]; + +const DeterministicConditionSchema = z.object({ + /** Stable BPMN id of the upstream Get Data step whose output holds the value. */ + sourceStepId: z.string().min(1), + fieldName: z.string().min(1), + operator: z.enum(CONDITION_OPERATORS), + /** Absent for `present`/`blank`. */ + value: z.unknown().optional(), }); +export type DeterministicCondition = z.infer; + +const OptionConditionsSchema = z.object({ + option: z.string().min(1), + aggregator: z.enum(['and', 'or']), + conditions: z.array(DeterministicConditionSchema).min(1), +}); +export type OptionConditions = z.infer; + +export const ConditionStepDefinitionSchema = z + .object({ + ...sharedFields, + type: z.literal(StepType.Condition), + // NO `.catch` — coercing an unknown mode from a newer orchestrator to FullyAutomated would + // silently let the AI decide instead of the conditions the builder configured precisely + // because they don't trust the AI. + executionType: z.enum([Manual, FullyAutomated, Deterministic]).default(FullyAutomated), + /** Ordered — evaluation priority for the deterministic mode (top-to-bottom, first-match-wins). */ + options: z.array(z.string()).min(2), + preRecordedArgs: z + .object({ + optionConditions: z.array(OptionConditionsSchema).min(1), + fallbackOption: z.string().min(1), + }) + .optional(), + }) + // No silent fallback to manual/AI: a deterministic step without its conditions must fail loud. + .superRefine((step, ctx) => { + if (step.executionType === Deterministic && step.preRecordedArgs === undefined) { + ctx.addIssue({ + code: 'custom', + path: ['preRecordedArgs'], + message: 'preRecordedArgs is required when executionType is "deterministic"', + }); + } + }); export type ConditionStepDefinition = z.infer; export const ReadRecordStepDefinitionSchema = z.object({ diff --git a/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts b/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts index 398df8c12c..8808972a1d 100644 --- a/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts +++ b/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts @@ -309,7 +309,7 @@ describe('toStepDefinition', () => { }); }); - // A newer orchestrator may send a deterministic mode this executor version does not know. + // A newer orchestrator may send an execution mode this executor version does not know. // The `.catch(FullyAutomated)` that used to sit on the condition schema would have silently // handed the decision to the AI; the mapper must reject the run as malformed instead. it('should throw InvalidStepDefinitionError for an unknown executionType instead of coercing to Full AI', () => { @@ -318,13 +318,55 @@ describe('toStepDefinition', () => { { stepId: 's1', buttonText: null, answer: 'Yes' }, { stepId: 's2', buttonText: null, answer: 'No' }, ], - { executionType: 'deterministic' as ServerWorkflowCondition['executionType'] }, + { executionType: 'not-a-mode' as ServerWorkflowCondition['executionType'] }, ); expect(() => toStepDefinition(condition)).toThrow(InvalidStepDefinitionError); expect(() => toStepDefinition(condition)).toThrow(/executionType/); }); + it('should forward preRecordedArgs on a deterministic condition', () => { + const preRecordedArgs = { + optionConditions: [ + { + option: 'Yes', + aggregator: 'and' as const, + conditions: [ + { sourceStepId: 'get-data-1', fieldName: 'status', operator: 'equal', value: 'ok' }, + ], + }, + ], + fallbackOption: 'No', + }; + const condition = makeCondition( + [ + { stepId: 's1', buttonText: null, answer: 'Yes' }, + { stepId: 's2', buttonText: null, answer: 'No' }, + ], + { executionType: ServerStepExecutionTypeEnum.Deterministic, preRecordedArgs }, + ); + + expect(toStepDefinition(condition)).toMatchObject({ + type: StepType.Condition, + executionType: StepExecutionMode.Deterministic, + options: ['Yes', 'No'], + preRecordedArgs, + }); + }); + + it('should throw InvalidStepDefinitionError for a deterministic condition without preRecordedArgs', () => { + const condition = makeCondition( + [ + { stepId: 's1', buttonText: null, answer: 'Yes' }, + { stepId: 's2', buttonText: null, answer: 'No' }, + ], + { executionType: ServerStepExecutionTypeEnum.Deterministic }, + ); + + expect(() => toStepDefinition(condition)).toThrow(InvalidStepDefinitionError); + expect(() => toStepDefinition(condition)).toThrow(/preRecordedArgs/); + }); + it('should throw InvalidStepDefinitionError when fewer than 2 options', () => { const condition = makeCondition([{ stepId: 's1', buttonText: 'Only' }]); diff --git a/packages/workflow-executor/test/executors/condition-step-executor.test.ts b/packages/workflow-executor/test/executors/condition-step-executor.test.ts index 0242da473b..55d6cac1c6 100644 --- a/packages/workflow-executor/test/executors/condition-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/condition-step-executor.test.ts @@ -2,7 +2,8 @@ import type { ActivityLogPort } from '../../src/ports/activity-log-port'; 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 { ExecutionContext, Step } from '../../src/types/execution-context'; +import type { FieldReadResult, StepExecutionData } from '../../src/types/step-execution-data'; import type { RecordRef } from '../../src/types/validated/collection'; import type { ConditionStepDefinition } from '../../src/types/validated/step-definition'; import type { ConditionStepOutcome } from '../../src/types/validated/step-outcome'; @@ -25,6 +26,39 @@ function makeStep(overrides: Partial = {}): ConditionSt }; } +type ConditionPreRecordedArgs = NonNullable; + +function makeDeterministicStep(preRecordedArgs: ConditionPreRecordedArgs): ConditionStepDefinition { + return makeStep({ + executionType: StepExecutionMode.Deterministic, + options: [ + ...preRecordedArgs.optionConditions.map(o => o.option), + preRecordedArgs.fallbackOption, + ], + preRecordedArgs, + }); +} + +function makeGetDataStep(stepId: string, stepIndex: number): Step { + return { + stepDefinition: { + type: StepType.ReadRecord, + executionType: StepExecutionMode.FullyAutomated, + }, + stepOutcome: { type: 'record', stepId, stepIndex, status: 'success' }, + }; +} + +function makeReadRecordExecution(stepIndex: number, fields: FieldReadResult[]): StepExecutionData { + return { + type: 'read-record', + stepIndex, + executionParams: { fields: fields.map(({ name, displayName }) => ({ name, displayName })) }, + executionResult: { fields }, + selectedRecordRef: { collectionName: 'orders', recordId: [1], stepIndex: 0 }, + }; +} + function makeMockRunStore(overrides: Partial = {}): RunStore { return { init: jest.fn().mockResolvedValue(undefined), @@ -421,6 +455,337 @@ describe('ConditionStepExecutor', () => { }); }); + describe('executionType=Deterministic', () => { + const amountArgs: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'High', + aggregator: 'and', + conditions: [ + { sourceStepId: 'get-1', fieldName: 'amount', operator: 'greater_than', value: 100 }, + ], + }, + { + option: 'Low', + aggregator: 'and', + conditions: [ + { + sourceStepId: 'get-1', + fieldName: 'amount', + operator: 'less_than_or_equal', + value: 100, + }, + ], + }, + ], + fallbackOption: 'Other', + }; + + function makeDeterministicContext( + preRecordedArgs: ConditionPreRecordedArgs, + fields: FieldReadResult[], + overrides: Partial> = {}, + ) { + const mockModel = makeMockModel(); + const runStore = makeMockRunStore({ + getStepExecutions: jest.fn().mockResolvedValue([makeReadRecordExecution(1, fields)]), + }); + const context = makeContext({ + model: mockModel.model, + runStore, + stepDefinition: makeDeterministicStep(preRecordedArgs), + previousSteps: [makeGetDataStep('get-1', 1)], + ...overrides, + }); + + return { context, mockModel, runStore }; + } + + it('selects the first matching option without calling the AI or awaiting input', async () => { + const { context, mockModel, runStore } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + ]); + const executor = new ConditionStepExecutor(context); + + const result = await executor.execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('High'); + expect(mockModel.bindTools).not.toHaveBeenCalled(); + expect(mockModel.invoke).not.toHaveBeenCalled(); + expect(runStore.saveStepExecution).toHaveBeenCalledWith('run-1', { + type: 'condition', + stepIndex: 0, + executionParams: { + evaluations: [ + { option: 'High', outcome: 'matched', conditions: [{ index: 0, met: true }] }, + { option: 'Low', outcome: 'not-evaluated' }, + ], + selectedOption: 'High', + usedFallback: false, + }, + executionResult: { answer: 'High' }, + }); + }); + + it('does not evaluate options after the first match, even if they would also match', async () => { + const bothMatch: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'First', + aggregator: 'and', + conditions: [{ sourceStepId: 'get-1', fieldName: 'amount', operator: 'present' }], + }, + { + option: 'Second', + aggregator: 'and', + conditions: [{ sourceStepId: 'get-1', fieldName: 'amount', operator: 'present' }], + }, + ], + fallbackOption: 'Other', + }; + const { context, runStore } = makeDeterministicContext(bothMatch, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('First'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ + evaluations: [ + { option: 'First', outcome: 'matched', conditions: [{ index: 0, met: true }] }, + { option: 'Second', outcome: 'not-evaluated' }, + ], + }), + }), + ); + }); + + it('requires all conditions with the and aggregator', async () => { + const andArgs: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'High paid', + aggregator: 'and', + conditions: [ + { sourceStepId: 'get-1', fieldName: 'amount', operator: 'greater_than', value: 100 }, + { sourceStepId: 'get-1', fieldName: 'status', operator: 'equal', value: 'paid' }, + ], + }, + ], + fallbackOption: 'Other', + }; + const { context, runStore } = makeDeterministicContext(andArgs, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + { name: 'status', displayName: 'Status', value: 'pending' }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith('run-1', { + type: 'condition', + stepIndex: 0, + executionParams: { + evaluations: [ + { + option: 'High paid', + outcome: 'not-matched', + conditions: [ + { index: 0, met: true }, + { index: 1, met: false }, + ], + }, + ], + selectedOption: 'Other', + usedFallback: true, + }, + executionResult: { answer: 'Other' }, + }); + }); + + it('matches with the or aggregator when any condition is met', async () => { + const orArgs: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'High or paid', + aggregator: 'or', + conditions: [ + { sourceStepId: 'get-1', fieldName: 'status', operator: 'equal', value: 'paid' }, + { sourceStepId: 'get-1', fieldName: 'amount', operator: 'greater_than', value: 100 }, + ], + }, + ], + fallbackOption: 'Other', + }; + const { context, runStore } = makeDeterministicContext(orArgs, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + { name: 'status', displayName: 'Status', value: 'pending' }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('High or paid'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ + evaluations: [ + { + option: 'High or paid', + outcome: 'matched', + conditions: [ + { index: 0, met: false }, + { index: 1, met: true }, + ], + }, + ], + }), + }), + ); + }); + + it('treats a null field value as not evaluable and falls back — never an error', async () => { + const { context, runStore } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', value: null }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith('run-1', { + type: 'condition', + stepIndex: 0, + executionParams: { + evaluations: [ + { option: 'High', outcome: 'not-matched', conditions: [{ index: 0, met: null }] }, + { option: 'Low', outcome: 'not-matched', conditions: [{ index: 0, met: null }] }, + ], + selectedOption: 'Other', + usedFallback: true, + }, + executionResult: { answer: 'Other' }, + }); + }); + + it('treats an unresolvable source step as not evaluable and falls back', async () => { + const unknownSource: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'High', + aggregator: 'and', + conditions: [ + { + sourceStepId: 'never-ran', + fieldName: 'amount', + operator: 'greater_than', + value: 100, + }, + ], + }, + ], + fallbackOption: 'Other', + }; + const { context } = makeDeterministicContext(unknownSource, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); + }); + + it('treats a field the Get Data step failed to read as not evaluable', async () => { + const { context, runStore } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', error: 'Field not found: amount' }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ + evaluations: expect.arrayContaining([ + { option: 'High', outcome: 'not-matched', conditions: [{ index: 0, met: null }] }, + ]), + }), + }), + ); + }); + + it('lets blank match a resolved null value (unlike an unresolvable one)', async () => { + const blankArgs: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'No amount', + aggregator: 'and', + conditions: [{ sourceStepId: 'get-1', fieldName: 'amount', operator: 'blank' }], + }, + ], + fallbackOption: 'Other', + }; + const { context } = makeDeterministicContext(blankArgs, [ + { name: 'amount', displayName: 'Amount', value: null }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('No amount'); + }); + + it('selects the fallback when no option matches', async () => { + const { context, runStore } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', value: 'not a number' }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith('run-1', { + type: 'condition', + stepIndex: 0, + executionParams: { + evaluations: [ + { option: 'High', outcome: 'not-matched', conditions: [{ index: 0, met: false }] }, + { option: 'Low', outcome: 'not-matched', conditions: [{ index: 0, met: false }] }, + ], + selectedOption: 'Other', + usedFallback: true, + }, + executionResult: { answer: 'Other' }, + }); + }); + + it('uses the most recent occurrence of a repeated source step id (loop)', async () => { + const runStore = makeMockRunStore({ + getStepExecutions: jest + .fn() + .mockResolvedValue([ + makeReadRecordExecution(1, [{ name: 'amount', displayName: 'Amount', value: 50 }]), + makeReadRecordExecution(2, [{ name: 'amount', displayName: 'Amount', value: 150 }]), + ]), + }); + const context = makeContext({ + model: makeMockModel().model, + runStore, + stepDefinition: makeDeterministicStep(amountArgs), + previousSteps: [makeGetDataStep('get-1', 1), makeGetDataStep('get-1', 2)], + }); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('High'); + }); + }); + describe('executionType=Manual', () => { it('returns awaiting-input without calling AI or saving when no incomingPendingData', async () => { const mockModel = makeMockModel(); diff --git a/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts b/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts new file mode 100644 index 0000000000..f1d0247ad7 --- /dev/null +++ b/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts @@ -0,0 +1,216 @@ +import evaluateOperator from '../../src/executors/deterministic-condition-evaluator'; + +describe('evaluateOperator', () => { + describe('null / missing actual value (never an error)', () => { + it.each([ + 'equal', + 'not_equal', + 'greater_than', + 'less_than', + 'greater_than_or_equal', + 'less_than_or_equal', + 'in', + 'not_in', + 'contains', + 'not_contains', + ] as const)('returns null (not evaluable) for %s on a null actual', operator => { + expect(evaluateOperator(operator, null, 'anything')).toBeNull(); + expect(evaluateOperator(operator, undefined, 'anything')).toBeNull(); + }); + }); + + describe('equal', () => { + it('matches identical scalars', () => { + expect(evaluateOperator('equal', 'active', 'active')).toBe(true); + expect(evaluateOperator('equal', 5, 5)).toBe(true); + expect(evaluateOperator('equal', false, false)).toBe(true); + }); + + it('rejects different scalars', () => { + expect(evaluateOperator('equal', 'active', 'inactive')).toBe(false); + }); + + it('rejects a type mismatch (no coercion)', () => { + expect(evaluateOperator('equal', 5, '5')).toBe(false); + expect(evaluateOperator('equal', true, 'true')).toBe(false); + }); + + it('matches ISO dates by timestamp, not by string', () => { + expect(evaluateOperator('equal', '2026-01-01T00:00:00Z', '2026-01-01T00:00:00.000Z')).toBe( + true, + ); + expect(evaluateOperator('equal', '2026-01-01T00:00:00Z', '2026-01-02T00:00:00Z')).toBe(false); + }); + + it('matches arrays elementwise in order', () => { + expect(evaluateOperator('equal', [1, 2], [1, 2])).toBe(true); + expect(evaluateOperator('equal', [1, 2], [2, 1])).toBe(false); + expect(evaluateOperator('equal', [1, 2], [1, 2, 3])).toBe(false); + }); + + it('rejects an array compared to a scalar', () => { + expect(evaluateOperator('equal', [1], 1)).toBe(false); + }); + }); + + describe('not_equal', () => { + it('matches different values', () => { + expect(evaluateOperator('not_equal', 'active', 'inactive')).toBe(true); + expect(evaluateOperator('not_equal', 5, '5')).toBe(true); + }); + + it('rejects identical values', () => { + expect(evaluateOperator('not_equal', 'active', 'active')).toBe(false); + expect( + evaluateOperator('not_equal', '2026-01-01T00:00:00Z', '2026-01-01T00:00:00.000Z'), + ).toBe(false); + }); + }); + + describe('present', () => { + it('matches non-empty values', () => { + expect(evaluateOperator('present', 'a', undefined)).toBe(true); + expect(evaluateOperator('present', 0, undefined)).toBe(true); + expect(evaluateOperator('present', false, undefined)).toBe(true); + expect(evaluateOperator('present', [1], undefined)).toBe(true); + }); + + it('rejects null, undefined, empty string and empty array', () => { + expect(evaluateOperator('present', null, undefined)).toBe(false); + expect(evaluateOperator('present', undefined, undefined)).toBe(false); + expect(evaluateOperator('present', '', undefined)).toBe(false); + expect(evaluateOperator('present', [], undefined)).toBe(false); + }); + }); + + describe('blank', () => { + it('matches null, undefined, empty string and empty array', () => { + expect(evaluateOperator('blank', null, undefined)).toBe(true); + expect(evaluateOperator('blank', undefined, undefined)).toBe(true); + expect(evaluateOperator('blank', '', undefined)).toBe(true); + expect(evaluateOperator('blank', [], undefined)).toBe(true); + }); + + it('rejects non-empty values including falsy ones', () => { + expect(evaluateOperator('blank', 'a', undefined)).toBe(false); + expect(evaluateOperator('blank', 0, undefined)).toBe(false); + expect(evaluateOperator('blank', false, undefined)).toBe(false); + }); + }); + + describe('numeric comparisons', () => { + it('greater_than compares numbers', () => { + expect(evaluateOperator('greater_than', 5, 3)).toBe(true); + expect(evaluateOperator('greater_than', 3, 5)).toBe(false); + expect(evaluateOperator('greater_than', 5, 5)).toBe(false); + }); + + it('less_than compares numbers', () => { + expect(evaluateOperator('less_than', 3, 5)).toBe(true); + expect(evaluateOperator('less_than', 5, 3)).toBe(false); + expect(evaluateOperator('less_than', 5, 5)).toBe(false); + }); + + it('greater_than_or_equal includes equality', () => { + expect(evaluateOperator('greater_than_or_equal', 5, 5)).toBe(true); + expect(evaluateOperator('greater_than_or_equal', 4, 5)).toBe(false); + }); + + it('less_than_or_equal includes equality', () => { + expect(evaluateOperator('less_than_or_equal', 5, 5)).toBe(true); + expect(evaluateOperator('less_than_or_equal', 6, 5)).toBe(false); + }); + + it('is not met on a type mismatch or non-comparable operands', () => { + expect(evaluateOperator('greater_than', 5, '3')).toBe(false); + expect(evaluateOperator('greater_than', '5', 3)).toBe(false); + expect(evaluateOperator('greater_than', 'abc', 'abd')).toBe(false); + expect(evaluateOperator('less_than', Number.NaN, 5)).toBe(false); + }); + }); + + describe('date comparisons', () => { + it('compares ISO strings as timestamps when both sides parse', () => { + expect(evaluateOperator('greater_than', '2026-02-01', '2026-01-01')).toBe(true); + expect(evaluateOperator('less_than', '2026-01-01T10:00:00Z', '2026-01-01T12:00:00Z')).toBe( + true, + ); + expect(evaluateOperator('greater_than_or_equal', '2026-01-01T00:00:00Z', '2026-01-01')).toBe( + true, + ); + expect(evaluateOperator('less_than_or_equal', '2026-01-02', '2026-01-01')).toBe(false); + }); + + it('is not met when one side does not parse as an ISO date', () => { + expect(evaluateOperator('greater_than', '2026-02-01', 'not a date')).toBe(false); + expect(evaluateOperator('less_than', 'not a date', '2026-02-01')).toBe(false); + }); + }); + + describe('in', () => { + it('matches when the value is in the list', () => { + expect(evaluateOperator('in', 'b', ['a', 'b'])).toBe(true); + expect(evaluateOperator('in', 2, [1, 2, 3])).toBe(true); + expect(evaluateOperator('in', '2026-01-01T00:00:00Z', ['2026-01-01T00:00:00.000Z'])).toBe( + true, + ); + }); + + it('rejects when the value is not in the list', () => { + expect(evaluateOperator('in', 'c', ['a', 'b'])).toBe(false); + expect(evaluateOperator('in', 2, ['2'])).toBe(false); + }); + + it('is not met when the expected value is not an array', () => { + expect(evaluateOperator('in', 'a', 'a')).toBe(false); + }); + }); + + describe('not_in', () => { + it('matches when the value is absent from the list', () => { + expect(evaluateOperator('not_in', 'c', ['a', 'b'])).toBe(true); + }); + + it('rejects when the value is in the list', () => { + expect(evaluateOperator('not_in', 'a', ['a', 'b'])).toBe(false); + }); + + it('is not met (never satisfied by mismatch) when the expected value is not an array', () => { + expect(evaluateOperator('not_in', 'a', 'b')).toBe(false); + }); + }); + + describe('contains', () => { + it('matches a substring on strings', () => { + expect(evaluateOperator('contains', 'hello world', 'world')).toBe(true); + expect(evaluateOperator('contains', 'hello', 'world')).toBe(false); + }); + + it('matches membership on arrays', () => { + expect(evaluateOperator('contains', ['a', 'b'], 'b')).toBe(true); + expect(evaluateOperator('contains', ['a', 'b'], 'c')).toBe(false); + }); + + it('is not met on a type mismatch', () => { + expect(evaluateOperator('contains', 5, '5')).toBe(false); + expect(evaluateOperator('contains', 'abc', 5)).toBe(false); + }); + }); + + describe('not_contains', () => { + it('matches when the substring is absent', () => { + expect(evaluateOperator('not_contains', 'hello', 'world')).toBe(true); + expect(evaluateOperator('not_contains', 'hello world', 'world')).toBe(false); + }); + + it('matches when the array does not contain the value', () => { + expect(evaluateOperator('not_contains', ['a'], 'b')).toBe(true); + expect(evaluateOperator('not_contains', ['a'], 'a')).toBe(false); + }); + + it('is not met (never satisfied by mismatch) on a type mismatch', () => { + expect(evaluateOperator('not_contains', 5, '5')).toBe(false); + expect(evaluateOperator('not_contains', 'abc', 5)).toBe(false); + }); + }); +}); diff --git a/packages/workflow-executor/test/types/step-definition.test.ts b/packages/workflow-executor/test/types/step-definition.test.ts index ff4337d8ad..0d517ee7b5 100644 --- a/packages/workflow-executor/test/types/step-definition.test.ts +++ b/packages/workflow-executor/test/types/step-definition.test.ts @@ -26,17 +26,132 @@ describe('ConditionStepDefinitionSchema executionType', () => { }); // No `.catch` on the enum: an unknown value must be rejected, not silently coerced to - // FullyAutomated (which would let the AI decide in place of a future deterministic mode). + // FullyAutomated (which would let the AI decide in place of the deterministic mode). it('rejects an invalid executionType instead of coercing it', () => { - expect( - ConditionStepDefinitionSchema.safeParse({ ...base, executionType: 'deterministic' }).success, - ).toBe(false); expect( ConditionStepDefinitionSchema.safeParse({ ...base, executionType: 'not-a-mode' }).success, ).toBe(false); }); }); +describe('ConditionStepDefinitionSchema deterministic mode', () => { + const base = { type: StepType.Condition as const, options: ['High value', 'Fallback'] }; + const preRecordedArgs = { + optionConditions: [ + { + option: 'High value', + aggregator: 'and', + conditions: [ + { sourceStepId: 'get-data-1', fieldName: 'amount', operator: 'greater_than', value: 100 }, + ], + }, + ], + fallbackOption: 'Fallback', + }; + + it('accepts deterministic with preRecordedArgs and round-trips them', () => { + const parsed = ConditionStepDefinitionSchema.parse({ + ...base, + executionType: 'deterministic', + preRecordedArgs, + }); + + expect(parsed.executionType).toBe(StepExecutionMode.Deterministic); + expect(parsed.preRecordedArgs).toEqual(preRecordedArgs); + }); + + it('rejects deterministic without preRecordedArgs — no silent fallback to AI', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + }); + + expect(result.success).toBe(false); + expect(JSON.stringify(!result.success && result.error.issues)).toContain('preRecordedArgs'); + }); + + it('rejects an unknown operator at the schema boundary', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + preRecordedArgs: { + ...preRecordedArgs, + optionConditions: [ + { + option: 'High value', + aggregator: 'and', + conditions: [{ sourceStepId: 'get-data-1', fieldName: 'amount', operator: 'ilike' }], + }, + ], + }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects preRecordedArgs missing fallbackOption', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + preRecordedArgs: { optionConditions: preRecordedArgs.optionConditions }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects an option with an unknown aggregator', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + preRecordedArgs: { + ...preRecordedArgs, + optionConditions: [{ ...preRecordedArgs.optionConditions[0], aggregator: 'xor' }], + }, + }); + + expect(result.success).toBe(false); + }); + + it('rejects an option with zero conditions', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + preRecordedArgs: { + ...preRecordedArgs, + optionConditions: [{ option: 'High value', aggregator: 'and', conditions: [] }], + }, + }); + + expect(result.success).toBe(false); + }); + + it('accepts a value-less condition for present/blank operators', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + preRecordedArgs: { + ...preRecordedArgs, + optionConditions: [ + { + option: 'High value', + aggregator: 'or', + conditions: [{ sourceStepId: 'get-data-1', fieldName: 'amount', operator: 'present' }], + }, + ], + }, + }); + + expect(result.success).toBe(true); + }); + + it('still accepts non-deterministic modes without preRecordedArgs', () => { + expect( + ConditionStepDefinitionSchema.safeParse({ ...base, executionType: 'fully-automated' }) + .success, + ).toBe(true); + }); +}); + describe('LoadRelatedRecordStepDefinitionSchema executionType', () => { const base = { type: StepType.LoadRelatedRecord as const }; From b7d5cfa303d349db28ac3b121d630cd6cd75f7a9 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 19 Aug 2026 08:29:59 +0200 Subject: [PATCH 2/8] fix(workflow-executor): make deterministic condition evaluation type-safe and routable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deterministic evaluator could turn a data or config mismatch into a silent misroute — a `status: 'success'` step carrying the wrong branch. - Numeric strings: Sequelize returns Postgres/MySQL `numeric`/`decimal`/`bigint` columns as strings while datasource-sequelize maps those types to the `Number` primitive, so the builder's `value: 100` met `"150.00"` at runtime and every comparison bailed → no option matched → silent fallback. A strictly numeric string is now coerced against a real number (both sides stay uncoerced when neither is a number, since ordering operators are Number/Date-only per the contract); `'abc'` vs `100` is still not evaluable. - Selected option: the deterministic path emitted `matchedOption ?? fallbackOption` without checking `step.options`, unlike the manual path. `optionConditions` and `options` are two different server-side derivations, so drift produced a success outcome the orchestrator cannot route and the run died far from the cause. It now throws `InvalidStepDefinitionError` before persisting anything. - `not_equal` contradicted the file's own documented policy ("a type mismatch can never satisfy a negated operator") by returning true on mismatch; equality is now tri-state, so a mismatch satisfies neither `equal` nor `not_equal`. - Timezone: an offset-less ISO datetime was parsed host-local, so "deterministic" evaluation varied per machine. Offset-less datetimes are read as UTC, pinned by a test that runs under a non-UTC TZ. - `contains`/`not_contains` were extended to array membership beyond the contract (§1: String only). Restricted back to strings — dead flexibility the builder never emits, and the "mismatch is never satisfied" invariant keeps it from misrouting. - Removed the unreachable `default` branch by replacing the operator switch with an exhaustive lookup keyed by `ConditionOperator`, so a new operator fails to compile instead of silently returning null (lint's `default-case` forbids a bare switch). Also asserts three spec behaviors that were unasserted: deterministic mode ignores `incomingPendingData`, `or` matches on a mix of not-evaluable and true, and an unresolvable reference satisfies neither `blank` nor `present`. Part of PRD-472 Co-Authored-By: Claude Opus 5 (1M context) --- packages/workflow-executor/CLAUDE.md | 3 +- .../src/executors/condition-step-executor.ts | 11 +- .../deterministic-condition-evaluator.ts | 125 ++++++++-------- .../executors/condition-step-executor.test.ts | 137 ++++++++++++++++++ .../deterministic-condition-evaluator.test.ts | 82 ++++++++--- 5 files changed, 277 insertions(+), 81 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 4d51699b59..929bb3e311 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -35,7 +35,8 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p `StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`, `Deterministic` (condition steps only). -- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (schema-required when the mode is deterministic; wire-final operator names in `CONDITION_OPERATORS`) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input. Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. +- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (schema-required when the mode is deterministic; wire-final operator names in `CONDITION_OPERATORS`) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input (`incomingPendingData` is ignored — no user override). The selected option (match or fallback) is checked against `step.options` before persisting → `InvalidStepDefinitionError`, because `optionConditions` and `options` come from two different server-side derivations and an unroutable option must not travel as a "success". Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. + - Comparison semantics (`deterministic-condition-evaluator.ts`): a strictly numeric string is coerced **against a real number only** (Sequelize returns `numeric`/`decimal`/`bigint` as strings while the datasource types them `Number`); an offset-less ISO datetime is read as **UTC** (host-TZ parsing would make the same run route differently per machine); a type mismatch satisfies no operator, negated ones included (`not_equal(true, 'true')` = not met); `contains`/`not_contains` are strings-only, per the contract. - **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`. diff --git a/packages/workflow-executor/src/executors/condition-step-executor.ts b/packages/workflow-executor/src/executors/condition-step-executor.ts index 2b3fef9591..b151d92775 100644 --- a/packages/workflow-executor/src/executors/condition-step-executor.ts +++ b/packages/workflow-executor/src/executors/condition-step-executor.ts @@ -9,7 +9,7 @@ import type { ConditionStepOutcome } from '../types/validated/step-outcome'; import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; import { z } from 'zod'; -import { StepStateError } from '../errors'; +import { InvalidStepDefinitionError, StepStateError } from '../errors'; import BaseStepExecutor from './base-step-executor'; import evaluateOperator from './deterministic-condition-evaluator'; import patchBodySchemas from '../http/pending-data-validators'; @@ -136,6 +136,15 @@ export default class ConditionStepExecutor extends BaseStepExecutor scalarEqual(item, expected[index])) + actual.every((item, index) => scalarEqual(item, expected[index]) === true) ); } - if (Array.isArray(actual) || Array.isArray(expected)) return false; + if (Array.isArray(actual) || Array.isArray(expected)) return null; return scalarEqual(actual, expected); } function compare(actual: unknown, expected: unknown): number | null { - if (typeof actual === 'number' && typeof expected === 'number') { - if (Number.isNaN(actual) || Number.isNaN(expected)) return null; - - return actual - expected; - } + const numbers = toNumberPair(actual, expected); + if (numbers) return numbers[0] - numbers[1]; const actualTs = toTimestamp(actual); const expectedTs = toTimestamp(expected); @@ -57,9 +82,35 @@ function isPresent(value: unknown): boolean { } function isMemberOf(list: unknown, candidate: unknown): boolean { - return Array.isArray(list) && list.some(item => scalarEqual(item, candidate)); + return Array.isArray(list) && list.some(item => scalarEqual(item, candidate) === true); +} + +function ordering(satisfies: (diff: number) => boolean) { + return (actual: unknown, expected: unknown): boolean => { + const diff = compare(actual, expected); + + return diff !== null && satisfies(diff); + }; } +const EVALUATORS: Record< + Exclude, + (actual: unknown, expected: unknown) => boolean +> = { + equal: (actual, expected) => isEqual(actual, expected) === true, + not_equal: (actual, expected) => isEqual(actual, expected) === false, + greater_than: ordering(diff => diff > 0), + less_than: ordering(diff => diff < 0), + greater_than_or_equal: ordering(diff => diff >= 0), + less_than_or_equal: ordering(diff => diff <= 0), + in: (actual, expected) => isMemberOf(expected, actual), + not_in: (actual, expected) => Array.isArray(expected) && !isMemberOf(expected, actual), + contains: (actual, expected) => + typeof actual === 'string' && typeof expected === 'string' && actual.includes(expected), + not_contains: (actual, expected) => + typeof actual === 'string' && typeof expected === 'string' && !actual.includes(expected), +}; + /** * Pure evaluation of one deterministic condition. Never throws for data reasons: * - `null` = not evaluable (the resolved value is null/missing) — treated as "not met"; @@ -75,53 +126,5 @@ export default function evaluateOperator( if (operator === 'blank') return !isPresent(actual); if (actual === null || actual === undefined) return null; - switch (operator) { - case 'equal': - return isEqual(actual, expected); - case 'not_equal': - return !isEqual(actual, expected); - - case 'greater_than': { - const diff = compare(actual, expected); - - return diff !== null && diff > 0; - } - - case 'less_than': { - const diff = compare(actual, expected); - - return diff !== null && diff < 0; - } - - case 'greater_than_or_equal': { - const diff = compare(actual, expected); - - return diff !== null && diff >= 0; - } - - case 'less_than_or_equal': { - const diff = compare(actual, expected); - - return diff !== null && diff <= 0; - } - - case 'in': - return isMemberOf(expected, actual); - case 'not_in': - return Array.isArray(expected) && !isMemberOf(expected, actual); - case 'contains': - if (typeof actual === 'string' && typeof expected === 'string') { - return actual.includes(expected); - } - - return isMemberOf(actual, expected); - case 'not_contains': - if (typeof actual === 'string' && typeof expected === 'string') { - return !actual.includes(expected); - } - - return Array.isArray(actual) && !isMemberOf(actual, expected); - default: - return null; - } + return EVALUATORS[operator](actual, expected); } diff --git a/packages/workflow-executor/test/executors/condition-step-executor.test.ts b/packages/workflow-executor/test/executors/condition-step-executor.test.ts index 55d6cac1c6..7f6bf469f9 100644 --- a/packages/workflow-executor/test/executors/condition-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/condition-step-executor.test.ts @@ -764,6 +764,143 @@ describe('ConditionStepExecutor', () => { }); }); + it('ignores incomingPendingData: no user override, no awaiting-input', async () => { + const { context, mockModel, runStore } = makeDeterministicContext( + amountArgs, + [{ name: 'amount', displayName: 'Amount', value: 150 }], + { incomingPendingData: { selectedOption: 'Low' } }, + ); + + const result = await new ConditionStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('High'); + expect(mockModel.bindTools).not.toHaveBeenCalled(); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ selectedOption: 'High' }), + }), + ); + }); + + it('matches an or option when one condition is not evaluable and another is met', async () => { + const orArgs: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'Unknown or high', + aggregator: 'or', + conditions: [ + { sourceStepId: 'get-1', fieldName: 'status', operator: 'equal', value: 'paid' }, + { sourceStepId: 'get-1', fieldName: 'amount', operator: 'greater_than', value: 100 }, + ], + }, + ], + fallbackOption: 'Other', + }; + const { context, runStore } = makeDeterministicContext(orArgs, [ + { name: 'status', displayName: 'Status', value: null }, + { name: 'amount', displayName: 'Amount', value: 150 }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Unknown or high'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ + evaluations: [ + { + option: 'Unknown or high', + outcome: 'matched', + conditions: [ + { index: 0, met: null }, + { index: 1, met: true }, + ], + }, + ], + }), + }), + ); + }); + + it.each(['blank', 'present'] as const)( + 'does not satisfy %s when the reference cannot be resolved at all', + async operator => { + const unresolvable: ConditionPreRecordedArgs = { + optionConditions: [ + { + option: 'Matched', + aggregator: 'and', + conditions: [{ sourceStepId: 'never-ran', fieldName: 'amount', operator }], + }, + ], + fallbackOption: 'Other', + }; + const { context, runStore } = makeDeterministicContext(unresolvable, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ + evaluations: [ + { + option: 'Matched', + outcome: 'not-matched', + conditions: [{ index: 0, met: null }], + }, + ], + }), + }), + ); + }, + ); + + it('compares a decimal column returned as a string against the builder number', async () => { + const { context } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', value: '150.00' }, + ]); + + const result = await new ConditionStepExecutor(context).execute(); + + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('High'); + }); + + it('fails loud when the matched option is not one of the step options', async () => { + const { context, runStore } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + ]); + const result = await new ConditionStepExecutor({ + ...context, + stepDefinition: { ...context.stepDefinition, options: ['Low', 'Other'] }, + }).execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome.error).toBe( + 'The workflow step configuration is invalid. Please check the workflow designer.', + ); + expect(runStore.saveStepExecution).not.toHaveBeenCalled(); + }); + + it('fails loud when the fallback option is not one of the step options', async () => { + const { context, runStore } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', value: 'not a number' }, + ]); + const result = await new ConditionStepExecutor({ + ...context, + stepDefinition: { ...context.stepDefinition, options: ['High', 'Low'] }, + }).execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(runStore.saveStepExecution).not.toHaveBeenCalled(); + }); + it('uses the most recent occurrence of a repeated source step id (loop)', async () => { const runStore = makeMockRunStore({ getStepExecutions: jest diff --git a/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts b/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts index f1d0247ad7..b0cf3f3de7 100644 --- a/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts +++ b/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts @@ -30,9 +30,9 @@ describe('evaluateOperator', () => { expect(evaluateOperator('equal', 'active', 'inactive')).toBe(false); }); - it('rejects a type mismatch (no coercion)', () => { - expect(evaluateOperator('equal', 5, '5')).toBe(false); + it('rejects a type mismatch', () => { expect(evaluateOperator('equal', true, 'true')).toBe(false); + expect(evaluateOperator('equal', 'abc', 100)).toBe(false); }); it('matches ISO dates by timestamp, not by string', () => { @@ -56,7 +56,7 @@ describe('evaluateOperator', () => { describe('not_equal', () => { it('matches different values', () => { expect(evaluateOperator('not_equal', 'active', 'inactive')).toBe(true); - expect(evaluateOperator('not_equal', 5, '5')).toBe(true); + expect(evaluateOperator('not_equal', 5, 6)).toBe(true); }); it('rejects identical values', () => { @@ -65,6 +65,39 @@ describe('evaluateOperator', () => { evaluateOperator('not_equal', '2026-01-01T00:00:00Z', '2026-01-01T00:00:00.000Z'), ).toBe(false); }); + + it('is not satisfied by a type mismatch, like every other operator', () => { + expect(evaluateOperator('not_equal', true, 'true')).toBe(false); + expect(evaluateOperator('not_equal', 5, 'abc')).toBe(false); + expect(evaluateOperator('not_equal', ['a'], 'a')).toBe(false); + }); + }); + + describe('numeric strings (decimal/bigint columns come back as strings)', () => { + it('compares a numeric string against a number', () => { + expect(evaluateOperator('greater_than', '150.00', 100)).toBe(true); + expect(evaluateOperator('greater_than', '50.00', 100)).toBe(false); + expect(evaluateOperator('less_than', 100, '150.00')).toBe(true); + expect(evaluateOperator('greater_than_or_equal', '100', 100)).toBe(true); + expect(evaluateOperator('less_than_or_equal', '-3', 0)).toBe(true); + }); + + it('equates a numeric string with a number', () => { + expect(evaluateOperator('equal', '42', 42)).toBe(true); + expect(evaluateOperator('equal', 42, '42.0')).toBe(true); + expect(evaluateOperator('not_equal', '42', 42)).toBe(false); + expect(evaluateOperator('in', '150.00', [100, 150])).toBe(true); + }); + + it('leaves a non-numeric string uncoerced', () => { + expect(evaluateOperator('greater_than', 'abc', 100)).toBe(false); + expect(evaluateOperator('greater_than', '12abc', 100)).toBe(false); + expect(evaluateOperator('equal', '', 0)).toBe(false); + }); + + it('does not coerce when neither side is a number', () => { + expect(evaluateOperator('greater_than', '5', '3')).toBe(false); + }); }); describe('present', () => { @@ -122,9 +155,8 @@ describe('evaluateOperator', () => { }); it('is not met on a type mismatch or non-comparable operands', () => { - expect(evaluateOperator('greater_than', 5, '3')).toBe(false); - expect(evaluateOperator('greater_than', '5', 3)).toBe(false); expect(evaluateOperator('greater_than', 'abc', 'abd')).toBe(false); + expect(evaluateOperator('greater_than', true, 3)).toBe(false); expect(evaluateOperator('less_than', Number.NaN, 5)).toBe(false); }); }); @@ -145,6 +177,28 @@ describe('evaluateOperator', () => { expect(evaluateOperator('greater_than', '2026-02-01', 'not a date')).toBe(false); expect(evaluateOperator('less_than', 'not a date', '2026-02-01')).toBe(false); }); + + describe('on a host whose timezone is not UTC', () => { + const originalTz = process.env.TZ; + + beforeAll(() => { + process.env.TZ = 'Pacific/Kiritimati'; + }); + + afterAll(() => { + process.env.TZ = originalTz; + }); + + it('reads a datetime without an offset as UTC, not as host-local time', () => { + expect(evaluateOperator('equal', '2026-01-01T10:00:00', '2026-01-01T10:00:00Z')).toBe(true); + expect( + evaluateOperator('greater_than', '2026-01-01T12:00:00', '2026-01-01T11:00:00Z'), + ).toBe(true); + expect( + evaluateOperator('less_than', '2026-01-01T10:00:00', '2026-01-01T11:00:00+00:00'), + ).toBe(true); + }); + }); }); describe('in', () => { @@ -158,7 +212,7 @@ describe('evaluateOperator', () => { it('rejects when the value is not in the list', () => { expect(evaluateOperator('in', 'c', ['a', 'b'])).toBe(false); - expect(evaluateOperator('in', 2, ['2'])).toBe(false); + expect(evaluateOperator('in', 2, ['3'])).toBe(false); }); it('is not met when the expected value is not an array', () => { @@ -186,12 +240,8 @@ describe('evaluateOperator', () => { expect(evaluateOperator('contains', 'hello', 'world')).toBe(false); }); - it('matches membership on arrays', () => { - expect(evaluateOperator('contains', ['a', 'b'], 'b')).toBe(true); - expect(evaluateOperator('contains', ['a', 'b'], 'c')).toBe(false); - }); - - it('is not met on a type mismatch', () => { + it('is not met on anything but two strings (contract: String fields only)', () => { + expect(evaluateOperator('contains', ['a', 'b'], 'b')).toBe(false); expect(evaluateOperator('contains', 5, '5')).toBe(false); expect(evaluateOperator('contains', 'abc', 5)).toBe(false); }); @@ -203,12 +253,8 @@ describe('evaluateOperator', () => { expect(evaluateOperator('not_contains', 'hello world', 'world')).toBe(false); }); - it('matches when the array does not contain the value', () => { - expect(evaluateOperator('not_contains', ['a'], 'b')).toBe(true); - expect(evaluateOperator('not_contains', ['a'], 'a')).toBe(false); - }); - - it('is not met (never satisfied by mismatch) on a type mismatch', () => { + it('is not met (never satisfied by mismatch) on anything but two strings', () => { + expect(evaluateOperator('not_contains', ['a'], 'b')).toBe(false); expect(evaluateOperator('not_contains', 5, '5')).toBe(false); expect(evaluateOperator('not_contains', 'abc', 5)).toBe(false); }); From d3395a5f718d90b31b66267cb089e4714ccac6cc Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 25 Aug 2026 23:31:01 +0200 Subject: [PATCH 3/8] fix(workflow-executor): reject impossible dates and value-less comparisons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Date.parse normalises an impossible calendar date — 2026-02-30 becomes March 2nd — so a typo in a condition compared against a date the author never wrote, and matched. Dates are now round-tripped and rejected when the parse moved them. A value-bearing operator with no value reached the evaluator and compared against undefined, which is not evaluable, so the option silently never matched and the run took the fallback. The schema now refuses it, in line with what the orchestrator already refuses at parse time. Part of PRD-472 Co-Authored-By: Claude Opus 5 (1M context) --- packages/workflow-executor/CLAUDE.md | 4 +-- .../deterministic-condition-evaluator.ts | 9 ++++++ .../src/types/validated/step-definition.ts | 30 ++++++++++++++----- .../deterministic-condition-evaluator.test.ts | 13 ++++++++ .../test/types/step-definition.test.ts | 22 ++++++++++++++ 5 files changed, 68 insertions(+), 10 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 929bb3e311..4a833eb6a1 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -35,8 +35,8 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p `StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`, `Deterministic` (condition steps only). -- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (schema-required when the mode is deterministic; wire-final operator names in `CONDITION_OPERATORS`) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input (`incomingPendingData` is ignored — no user override). The selected option (match or fallback) is checked against `step.options` before persisting → `InvalidStepDefinitionError`, because `optionConditions` and `options` come from two different server-side derivations and an unroutable option must not travel as a "success". Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. - - Comparison semantics (`deterministic-condition-evaluator.ts`): a strictly numeric string is coerced **against a real number only** (Sequelize returns `numeric`/`decimal`/`bigint` as strings while the datasource types them `Number`); an offset-less ISO datetime is read as **UTC** (host-TZ parsing would make the same run route differently per machine); a type mismatch satisfies no operator, negated ones included (`not_equal(true, 'true')` = not met); `contains`/`not_contains` are strings-only, per the contract. +- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (schema-required when the mode is deterministic; wire-final operator names in `CONDITION_OPERATORS`; a value-bearing operator missing its `value` is rejected at the schema boundary — comparing against `undefined` can never be met, so the step would route to the fallback instead of surfacing the broken config) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input (`incomingPendingData` is ignored — no user override). The selected option (match or fallback) is checked against `step.options` before persisting → `InvalidStepDefinitionError`, because `optionConditions` and `options` come from two different server-side derivations and an unroutable option must not travel as a "success". Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. + - Comparison semantics (`deterministic-condition-evaluator.ts`): a strictly numeric string is coerced **against a real number only** (Sequelize returns `numeric`/`decimal`/`bigint` as strings while the datasource types them `Number`); an offset-less ISO datetime is read as **UTC** (host-TZ parsing would make the same run route differently per machine); an impossible calendar date is **not a date** (`Date.parse('2026-02-30')` rolls over to 2026-03-02, which would make a nonsensical config compare equal to a real value); a type mismatch satisfies no operator, negated ones included (`not_equal(true, 'true')` = not met); `contains`/`not_contains` are strings-only, per the contract. - **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`. diff --git a/packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts b/packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts index 78ba442d4e..67296e0e12 100644 --- a/packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts +++ b/packages/workflow-executor/src/executors/deterministic-condition-evaluator.ts @@ -8,8 +8,17 @@ const TIMEZONE_SUFFIX = /(Z|[+-]\d{2}:?\d{2})$/i; // maps them to the Number primitive, so the builder's JSON number meets a string at runtime. const NUMERIC_STRING = /^-?\d+(\.\d+)?$/; +// Date.parse rolls an out-of-range day over ("2026-02-30" becomes 2026-03-02), which would make a +// nonsensical config compare equal to a real date instead of being inert. +function isRealCalendarDate(datePart: string): boolean { + const parsed = new Date(`${datePart}T00:00:00.000Z`); + + return !Number.isNaN(parsed.getTime()) && parsed.toISOString().startsWith(datePart); +} + function toTimestamp(value: unknown): number | null { if (typeof value !== 'string' || !ISO_DATE_PREFIX.test(value)) return null; + if (!isRealCalendarDate(value.slice(0, 10))) return null; // Date.parse reads an offset-less datetime as host-local (date-only as UTC), which would route // the same run differently per machine — pin every offset-less datetime to UTC. diff --git a/packages/workflow-executor/src/types/validated/step-definition.ts b/packages/workflow-executor/src/types/validated/step-definition.ts index 4501a118be..06c2afe9e8 100644 --- a/packages/workflow-executor/src/types/validated/step-definition.ts +++ b/packages/workflow-executor/src/types/validated/step-definition.ts @@ -59,14 +59,28 @@ export const CONDITION_OPERATORS = [ ] as const; export type ConditionOperator = (typeof CONDITION_OPERATORS)[number]; -const DeterministicConditionSchema = z.object({ - /** Stable BPMN id of the upstream Get Data step whose output holds the value. */ - sourceStepId: z.string().min(1), - fieldName: z.string().min(1), - operator: z.enum(CONDITION_OPERATORS), - /** Absent for `present`/`blank`. */ - value: z.unknown().optional(), -}); +const VALUE_LESS_OPERATORS: readonly ConditionOperator[] = ['present', 'blank']; + +const DeterministicConditionSchema = z + .object({ + /** Stable BPMN id of the upstream Get Data step whose output holds the value. */ + sourceStepId: z.string().min(1), + fieldName: z.string().min(1), + operator: z.enum(CONDITION_OPERATORS), + /** Absent for `present`/`blank`. */ + value: z.unknown().optional(), + }) + // A value-bearing operator without its value compares against `undefined`: it can never be met, + // so the step would silently route to the fallback instead of reporting the broken config. + .superRefine((condition, ctx) => { + if (!VALUE_LESS_OPERATORS.includes(condition.operator) && condition.value === undefined) { + ctx.addIssue({ + code: 'custom', + path: ['value'], + message: `value is required for the "${condition.operator}" operator`, + }); + } + }); export type DeterministicCondition = z.infer; const OptionConditionsSchema = z.object({ diff --git a/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts b/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts index b0cf3f3de7..026bd1162e 100644 --- a/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts +++ b/packages/workflow-executor/test/executors/deterministic-condition-evaluator.test.ts @@ -178,6 +178,19 @@ describe('evaluateOperator', () => { expect(evaluateOperator('less_than', 'not a date', '2026-02-01')).toBe(false); }); + it('treats an impossible calendar date as not a date instead of rolling it over', () => { + expect(evaluateOperator('equal', '2026-03-02', '2026-02-30')).toBe(false); + expect(evaluateOperator('equal', '2026-05-01', '2026-04-31')).toBe(false); + expect(evaluateOperator('equal', '2025-03-01', '2025-02-29')).toBe(false); + expect(evaluateOperator('greater_than', '2026-02-30', '2026-01-01')).toBe(false); + expect(evaluateOperator('less_than_or_equal', '2026-01-01', '2026-02-30')).toBe(false); + expect(evaluateOperator('in', '2026-03-02', ['2026-02-30'])).toBe(false); + }); + + it('still accepts a leap day that exists', () => { + expect(evaluateOperator('equal', '2024-02-29', '2024-02-29T00:00:00.000Z')).toBe(true); + }); + describe('on a host whose timezone is not UTC', () => { const originalTz = process.env.TZ; diff --git a/packages/workflow-executor/test/types/step-definition.test.ts b/packages/workflow-executor/test/types/step-definition.test.ts index 0d517ee7b5..e96dc68ee5 100644 --- a/packages/workflow-executor/test/types/step-definition.test.ts +++ b/packages/workflow-executor/test/types/step-definition.test.ts @@ -125,6 +125,28 @@ describe('ConditionStepDefinitionSchema deterministic mode', () => { expect(result.success).toBe(false); }); + it.each(['equal', 'not_equal', 'greater_than', 'in', 'contains'])( + 'rejects a "%s" condition with no value', + operator => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + preRecordedArgs: { + ...preRecordedArgs, + optionConditions: [ + { + option: 'High value', + aggregator: 'and', + conditions: [{ sourceStepId: 'get-data-1', fieldName: 'amount', operator }], + }, + ], + }, + }); + + expect(result.success).toBe(false); + }, + ); + it('accepts a value-less condition for present/blank operators', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, From 982efc2a44f537ba54c7288e562a1484f3785c8d Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 26 Aug 2026 14:24:14 +0200 Subject: [PATCH 4/8] refactor(workflow-executor): drop the deterministic execution mode from the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A condition step is deterministic iff it carries preRecordedArgs.optionConditions. The orchestrator derived the mode from that very presence, so the wire carried two sources of truth for one fact — and the mode was the half an older executor could not read. Dropping it also removes the reason the mode existed: a deterministic gateway publishes with aiDecision stripped, so it now arrives as Manual. An executor blind to the args degrades to a visible manual decision, never a silent AI one. An unknown mode still fails loud (no .catch), and malformed args are still rejected rather than dropped. Co-Authored-By: Claude Opus 5 (1M context) --- packages/workflow-executor/CLAUDE.md | 4 +- .../src/adapters/server-types.ts | 10 +-- .../src/executors/condition-step-executor.ts | 10 +-- .../src/types/validated/step-definition.ts | 48 ++++------ .../adapters/step-definition-mapper.test.ts | 53 ++++++----- .../executors/condition-step-executor.test.ts | 69 +++++++++++++-- .../test/types/step-definition.test.ts | 87 ++++++++++++++----- 7 files changed, 182 insertions(+), 99 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 4a833eb6a1..28d8630dd2 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -33,9 +33,9 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p ## Step types & execution modes -`StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`, `Deterministic` (condition steps only). +`StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`. There is **no deterministic mode on the wire**: a condition step is deterministic iff it carries `preRecordedArgs.optionConditions`. A deterministic gateway publishes with `aiDecision` stripped, so it arrives as `Manual` — an executor blind to the args degrades to a visible manual decision, never a silent AI one. -- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (schema-required when the mode is deterministic; wire-final operator names in `CONDITION_OPERATORS`; a value-bearing operator missing its `value` is rejected at the schema boundary — comparing against `undefined` can never be met, so the step would route to the fallback instead of surfacing the broken config) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input (`incomingPendingData` is ignored — no user override). The selected option (match or fallback) is checked against `step.options` before persisting → `InvalidStepDefinitionError`, because `optionConditions` and `options` come from two different server-side derivations and an unroutable option must not travel as a "success". Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. +- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (wire-final operator names in `CONDITION_OPERATORS`; a value-bearing operator missing its `value` is rejected at the schema boundary — comparing against `undefined` can never be met, so the step would route to the fallback instead of surfacing the broken config) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input (`incomingPendingData` is ignored — no user override). The selected option (match or fallback) is checked against `step.options` before persisting → `InvalidStepDefinitionError`, because `optionConditions` and `options` come from two different server-side derivations and an unroutable option must not travel as a "success". Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. - Comparison semantics (`deterministic-condition-evaluator.ts`): a strictly numeric string is coerced **against a real number only** (Sequelize returns `numeric`/`decimal`/`bigint` as strings while the datasource types them `Number`); an offset-less ISO datetime is read as **UTC** (host-TZ parsing would make the same run route differently per machine); an impossible calendar date is **not a date** (`Date.parse('2026-02-30')` rolls over to 2026-03-02, which would make a nonsensical config compare equal to a real value); a type mismatch satisfies no operator, negated ones included (`not_equal(true, 'true')` = not met); `contains`/`not_contains` are strings-only, per the contract. - **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. diff --git a/packages/workflow-executor/src/adapters/server-types.ts b/packages/workflow-executor/src/adapters/server-types.ts index 9973038409..e7d9d7e728 100644 --- a/packages/workflow-executor/src/adapters/server-types.ts +++ b/packages/workflow-executor/src/adapters/server-types.ts @@ -35,7 +35,6 @@ export enum ServerStepExecutionTypeEnum { Manual = 'manual', AutomatedWithConfirmation = 'automated-with-confirmation', FullyAutomated = 'fully-automated', - Deterministic = 'deterministic', } interface ServerWorkflowStepBase { @@ -123,14 +122,11 @@ export interface ServerWorkflowEnd extends ServerWorkflowStepBase { export interface ServerWorkflowCondition extends ServerWorkflowStepBase { type: ServerStepTypeEnum.Condition; - executionType: - | ServerStepExecutionTypeEnum.Manual - | ServerStepExecutionTypeEnum.FullyAutomated - | ServerStepExecutionTypeEnum.Deterministic; + executionType: ServerStepExecutionTypeEnum.Manual | ServerStepExecutionTypeEnum.FullyAutomated; prompt: string | null; automaticCompletion: false; - // Parsed from `forest:optionConditions` server-side (flowId → answer). Present when - // executionType is deterministic. Validated by the step-definition schema. + // Parsed from `forest:optionConditions` server-side (flowId → answer). Presence *is* the + // deterministic mode. Validated by the step-definition schema. preRecordedArgs?: { optionConditions: Array<{ option: string; diff --git a/packages/workflow-executor/src/executors/condition-step-executor.ts b/packages/workflow-executor/src/executors/condition-step-executor.ts index b151d92775..53d1d7336a 100644 --- a/packages/workflow-executor/src/executors/condition-step-executor.ts +++ b/packages/workflow-executor/src/executors/condition-step-executor.ts @@ -67,10 +67,9 @@ export default class ConditionStepExecutor extends BaseStepExecutor { const { stepDefinition: step, incomingPendingData } = this.context; - // Deterministic mode: pure evaluation of build-time conditions against the run's step - // history — never calls the AI, never awaits input. - if (step.executionType === StepExecutionMode.Deterministic) { - return this.evaluateDeterministically(step); + // Conditions present *is* the deterministic mode: pure evaluation, no AI, no user input. + if (step.preRecordedArgs) { + return this.evaluateDeterministically(step, step.preRecordedArgs); } // Manual mode: the user picks the option from the frontend. Wait for their input @@ -105,9 +104,8 @@ export default class ConditionStepExecutor extends BaseStepExecutor, ): Promise { - // Guaranteed by the schema's superRefine for the deterministic mode. - const { optionConditions, fallbackOption } = step.preRecordedArgs!; const stepExecutions = await this.context.runStore.getStepExecutions(this.context.runId); let matchedOption: string | undefined; diff --git a/packages/workflow-executor/src/types/validated/step-definition.ts b/packages/workflow-executor/src/types/validated/step-definition.ts index 06c2afe9e8..1ba735328b 100644 --- a/packages/workflow-executor/src/types/validated/step-definition.ts +++ b/packages/workflow-executor/src/types/validated/step-definition.ts @@ -18,7 +18,6 @@ export enum StepExecutionMode { Manual = 'manual', AutomatedWithConfirmation = 'automated-with-confirmation', FullyAutomated = 'fully-automated', - Deterministic = 'deterministic', } // Shared fields across all step types. executionType is intentionally excluded — @@ -39,7 +38,7 @@ const sharedFields = { }; // Use z.enum(EnumObject), not z.nativeEnum — the latter is deprecated in zod 4. -const { Manual, AutomatedWithConfirmation, FullyAutomated, Deterministic } = StepExecutionMode; +const { Manual, AutomatedWithConfirmation, FullyAutomated } = StepExecutionMode; // Wire-final operator names (PRD-472 cross-repo contract). An unknown operator is rejected here, // at the schema boundary, so a run never reaches evaluation with a comparison it cannot honor. @@ -90,33 +89,24 @@ const OptionConditionsSchema = z.object({ }); export type OptionConditions = z.infer; -export const ConditionStepDefinitionSchema = z - .object({ - ...sharedFields, - type: z.literal(StepType.Condition), - // NO `.catch` — coercing an unknown mode from a newer orchestrator to FullyAutomated would - // silently let the AI decide instead of the conditions the builder configured precisely - // because they don't trust the AI. - executionType: z.enum([Manual, FullyAutomated, Deterministic]).default(FullyAutomated), - /** Ordered — evaluation priority for the deterministic mode (top-to-bottom, first-match-wins). */ - options: z.array(z.string()).min(2), - preRecordedArgs: z - .object({ - optionConditions: z.array(OptionConditionsSchema).min(1), - fallbackOption: z.string().min(1), - }) - .optional(), - }) - // No silent fallback to manual/AI: a deterministic step without its conditions must fail loud. - .superRefine((step, ctx) => { - if (step.executionType === Deterministic && step.preRecordedArgs === undefined) { - ctx.addIssue({ - code: 'custom', - path: ['preRecordedArgs'], - message: 'preRecordedArgs is required when executionType is "deterministic"', - }); - } - }); +export const ConditionStepDefinitionSchema = z.object({ + ...sharedFields, + type: z.literal(StepType.Condition), + // NO `.catch` — coercing an unknown mode from a newer orchestrator to FullyAutomated would + // silently let the AI decide instead of the conditions the builder configured precisely + // because they don't trust the AI. + executionType: z.enum([Manual, FullyAutomated]).default(FullyAutomated), + /** Ordered — evaluation priority for the deterministic mode (top-to-bottom, first-match-wins). */ + options: z.array(z.string()).min(2), + // Presence *is* the deterministic mode. A malformed config is rejected here rather than dropped, + // so it can never degrade to a manual/AI decision. + preRecordedArgs: z + .object({ + optionConditions: z.array(OptionConditionsSchema).min(1), + fallbackOption: z.string().min(1), + }) + .optional(), +}); export type ConditionStepDefinition = z.infer; export const ReadRecordStepDefinitionSchema = z.object({ diff --git a/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts b/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts index 8808972a1d..c1e809dd04 100644 --- a/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts +++ b/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts @@ -309,23 +309,29 @@ describe('toStepDefinition', () => { }); }); - // A newer orchestrator may send an execution mode this executor version does not know. - // The `.catch(FullyAutomated)` that used to sit on the condition schema would have silently - // handed the decision to the AI; the mapper must reject the run as malformed instead. - it('should throw InvalidStepDefinitionError for an unknown executionType instead of coercing to Full AI', () => { - const condition = makeCondition( - [ - { stepId: 's1', buttonText: null, answer: 'Yes' }, - { stepId: 's2', buttonText: null, answer: 'No' }, - ], - { executionType: 'not-a-mode' as ServerWorkflowCondition['executionType'] }, - ); - - expect(() => toStepDefinition(condition)).toThrow(InvalidStepDefinitionError); - expect(() => toStepDefinition(condition)).toThrow(/executionType/); - }); - - it('should forward preRecordedArgs on a deterministic condition', () => { + // A newer orchestrator may send an execution mode this executor version does not know, and + // `deterministic` is now one of them. The `.catch(FullyAutomated)` that used to sit on the + // condition schema would have silently handed the decision to the AI; the mapper must reject + // the run as malformed instead. + it.each(['not-a-mode', 'deterministic'])( + 'should throw InvalidStepDefinitionError for the unknown executionType "%s" instead of coercing to Full AI', + executionType => { + const condition = makeCondition( + [ + { stepId: 's1', buttonText: null, answer: 'Yes' }, + { stepId: 's2', buttonText: null, answer: 'No' }, + ], + { executionType: executionType as ServerWorkflowCondition['executionType'] }, + ); + + expect(() => toStepDefinition(condition)).toThrow(InvalidStepDefinitionError); + expect(() => toStepDefinition(condition)).toThrow(/executionType/); + }, + ); + + // A deterministic gateway publishes with `aiDecision` stripped, so it arrives as `manual` and + // the preRecordedArgs are the only thing that marks it deterministic. + it('should forward preRecordedArgs on a manual condition', () => { const preRecordedArgs = { optionConditions: [ { @@ -343,28 +349,31 @@ describe('toStepDefinition', () => { { stepId: 's1', buttonText: null, answer: 'Yes' }, { stepId: 's2', buttonText: null, answer: 'No' }, ], - { executionType: ServerStepExecutionTypeEnum.Deterministic, preRecordedArgs }, + { executionType: ServerStepExecutionTypeEnum.Manual, preRecordedArgs }, ); expect(toStepDefinition(condition)).toMatchObject({ type: StepType.Condition, - executionType: StepExecutionMode.Deterministic, + executionType: StepExecutionMode.Manual, options: ['Yes', 'No'], preRecordedArgs, }); }); - it('should throw InvalidStepDefinitionError for a deterministic condition without preRecordedArgs', () => { + it('should throw InvalidStepDefinitionError for malformed preRecordedArgs rather than dropping them', () => { const condition = makeCondition( [ { stepId: 's1', buttonText: null, answer: 'Yes' }, { stepId: 's2', buttonText: null, answer: 'No' }, ], - { executionType: ServerStepExecutionTypeEnum.Deterministic }, + { + executionType: ServerStepExecutionTypeEnum.Manual, + preRecordedArgs: { optionConditions: [], fallbackOption: 'No' }, + }, ); expect(() => toStepDefinition(condition)).toThrow(InvalidStepDefinitionError); - expect(() => toStepDefinition(condition)).toThrow(/preRecordedArgs/); + expect(() => toStepDefinition(condition)).toThrow(/optionConditions/); }); it('should throw InvalidStepDefinitionError when fewer than 2 options', () => { diff --git a/packages/workflow-executor/test/executors/condition-step-executor.test.ts b/packages/workflow-executor/test/executors/condition-step-executor.test.ts index 7f6bf469f9..6a528471f7 100644 --- a/packages/workflow-executor/test/executors/condition-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/condition-step-executor.test.ts @@ -28,9 +28,14 @@ function makeStep(overrides: Partial = {}): ConditionSt type ConditionPreRecordedArgs = NonNullable; -function makeDeterministicStep(preRecordedArgs: ConditionPreRecordedArgs): ConditionStepDefinition { +// A deterministic gateway is published with `aiDecision` stripped, so the orchestrator sends it as +// `manual` + preRecordedArgs — the args are what make it deterministic, not the mode. +function makeDeterministicStep( + preRecordedArgs: ConditionPreRecordedArgs, + executionType: ConditionStepDefinition['executionType'] = StepExecutionMode.Manual, +): ConditionStepDefinition { return makeStep({ - executionType: StepExecutionMode.Deterministic, + executionType, options: [ ...preRecordedArgs.optionConditions.map(o => o.option), preRecordedArgs.fallbackOption, @@ -455,7 +460,7 @@ describe('ConditionStepExecutor', () => { }); }); - describe('executionType=Deterministic', () => { + describe('deterministic evaluation driven by preRecordedArgs', () => { const amountArgs: ConditionPreRecordedArgs = { optionConditions: [ { @@ -501,6 +506,53 @@ describe('ConditionStepExecutor', () => { return { context, mockModel, runStore }; } + it('evaluates a manual condition instead of awaiting input, with no incomingPendingData', async () => { + const { context, mockModel, runStore } = makeDeterministicContext(amountArgs, [ + { name: 'amount', displayName: 'Amount', value: 150 }, + ]); + + expect(context.stepDefinition.executionType).toBe(StepExecutionMode.Manual); + expect(context.incomingPendingData).toBeUndefined(); + + const result = await new ConditionStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('High'); + expect(mockModel.bindTools).not.toHaveBeenCalled(); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ selectedOption: 'High', usedFallback: false }), + }), + ); + }); + + it('evaluates a fully-automated condition carrying preRecordedArgs without calling the AI', async () => { + const { context, mockModel, runStore } = makeDeterministicContext( + amountArgs, + [{ name: 'amount', displayName: 'Amount', value: 150 }], + { stepDefinition: makeDeterministicStep(amountArgs, StepExecutionMode.FullyAutomated) }, + ); + + const result = await new ConditionStepExecutor(context).execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('High'); + expect(mockModel.bindTools).not.toHaveBeenCalled(); + expect(mockModel.invoke).not.toHaveBeenCalled(); + expect(runStore.saveStepExecution).toHaveBeenCalledWith( + 'run-1', + expect.objectContaining({ + executionParams: expect.objectContaining({ + evaluations: [ + { option: 'High', outcome: 'matched', conditions: [{ index: 0, met: true }] }, + { option: 'Low', outcome: 'not-evaluated' }, + ], + }), + }), + ); + }); + it('selects the first matching option without calling the AI or awaiting input', async () => { const { context, mockModel, runStore } = makeDeterministicContext(amountArgs, [ { name: 'amount', displayName: 'Amount', value: 150 }, @@ -924,17 +976,16 @@ describe('ConditionStepExecutor', () => { }); describe('executionType=Manual', () => { - it('returns awaiting-input without calling AI or saving when no incomingPendingData', async () => { + it('returns awaiting-input without preRecordedArgs, no AI and no save, when no incomingPendingData', async () => { const mockModel = makeMockModel(); const runStore = makeMockRunStore(); + const stepDefinition = makeStep({ executionType: StepExecutionMode.Manual }); const executor = new ConditionStepExecutor( - makeContext({ - model: mockModel.model, - runStore, - stepDefinition: makeStep({ executionType: StepExecutionMode.Manual }), - }), + makeContext({ model: mockModel.model, runStore, stepDefinition }), ); + expect(stepDefinition.preRecordedArgs).toBeUndefined(); + const result = await executor.execute(); expect(result.stepOutcome.status).toBe('awaiting-input'); diff --git a/packages/workflow-executor/test/types/step-definition.test.ts b/packages/workflow-executor/test/types/step-definition.test.ts index e96dc68ee5..e56c9a4cc9 100644 --- a/packages/workflow-executor/test/types/step-definition.test.ts +++ b/packages/workflow-executor/test/types/step-definition.test.ts @@ -26,15 +26,39 @@ describe('ConditionStepDefinitionSchema executionType', () => { }); // No `.catch` on the enum: an unknown value must be rejected, not silently coerced to - // FullyAutomated (which would let the AI decide in place of the deterministic mode). - it('rejects an invalid executionType instead of coercing it', () => { - expect( - ConditionStepDefinitionSchema.safeParse({ ...base, executionType: 'not-a-mode' }).success, - ).toBe(false); + // FullyAutomated, which would hand the decision to the AI. `deterministic` is one such unknown + // value now that it is gone from the wire contract. + it.each(['not-a-mode', 'deterministic', 'whatever'])( + 'rejects the unknown executionType "%s" instead of coercing it', + executionType => { + expect(ConditionStepDefinitionSchema.safeParse({ ...base, executionType }).success).toBe( + false, + ); + }, + ); + + it('rejects the removed "deterministic" mode even when preRecordedArgs are present', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'deterministic', + preRecordedArgs: { + optionConditions: [ + { + option: 'Yes', + aggregator: 'and', + conditions: [{ sourceStepId: 'get-data-1', fieldName: 'amount', operator: 'present' }], + }, + ], + fallbackOption: 'No', + }, + }); + + expect(result.success).toBe(false); + expect(JSON.stringify(!result.success && result.error.issues)).toContain('executionType'); }); }); -describe('ConditionStepDefinitionSchema deterministic mode', () => { +describe('ConditionStepDefinitionSchema deterministic conditions', () => { const base = { type: StepType.Condition as const, options: ['High value', 'Fallback'] }; const preRecordedArgs = { optionConditions: [ @@ -49,50 +73,54 @@ describe('ConditionStepDefinitionSchema deterministic mode', () => { fallbackOption: 'Fallback', }; - it('accepts deterministic with preRecordedArgs and round-trips them', () => { + // A deterministic gateway publishes with `aiDecision` stripped, so it reaches the executor as + // `manual` + preRecordedArgs: the args carry the mode, the executionType does not. + it('accepts manual with preRecordedArgs and round-trips them', () => { const parsed = ConditionStepDefinitionSchema.parse({ ...base, - executionType: 'deterministic', + executionType: 'manual', preRecordedArgs, }); - expect(parsed.executionType).toBe(StepExecutionMode.Deterministic); + expect(parsed.executionType).toBe(StepExecutionMode.Manual); expect(parsed.preRecordedArgs).toEqual(preRecordedArgs); }); - it('rejects deterministic without preRecordedArgs — no silent fallback to AI', () => { - const result = ConditionStepDefinitionSchema.safeParse({ - ...base, - executionType: 'deterministic', - }); + it('accepts preRecordedArgs with no executionType at all', () => { + const parsed = ConditionStepDefinitionSchema.parse({ ...base, preRecordedArgs }); - expect(result.success).toBe(false); - expect(JSON.stringify(!result.success && result.error.issues)).toContain('preRecordedArgs'); + expect(parsed.executionType).toBe(StepExecutionMode.FullyAutomated); + expect(parsed.preRecordedArgs).toEqual(preRecordedArgs); }); + // The `value` is supplied on purpose: without it the value-less-operator refinement would + // reject the condition anyway, and the test would pass without pinning the operator enum. it('rejects an unknown operator at the schema boundary', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, - executionType: 'deterministic', + executionType: 'manual', preRecordedArgs: { ...preRecordedArgs, optionConditions: [ { option: 'High value', aggregator: 'and', - conditions: [{ sourceStepId: 'get-data-1', fieldName: 'amount', operator: 'ilike' }], + conditions: [ + { sourceStepId: 'get-data-1', fieldName: 'amount', operator: 'ilike', value: '%x%' }, + ], }, ], }, }); expect(result.success).toBe(false); + expect(JSON.stringify(!result.success && result.error.issues)).toContain('operator'); }); it('rejects preRecordedArgs missing fallbackOption', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, - executionType: 'deterministic', + executionType: 'manual', preRecordedArgs: { optionConditions: preRecordedArgs.optionConditions }, }); @@ -102,7 +130,7 @@ describe('ConditionStepDefinitionSchema deterministic mode', () => { it('rejects an option with an unknown aggregator', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, - executionType: 'deterministic', + executionType: 'manual', preRecordedArgs: { ...preRecordedArgs, optionConditions: [{ ...preRecordedArgs.optionConditions[0], aggregator: 'xor' }], @@ -115,7 +143,7 @@ describe('ConditionStepDefinitionSchema deterministic mode', () => { it('rejects an option with zero conditions', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, - executionType: 'deterministic', + executionType: 'manual', preRecordedArgs: { ...preRecordedArgs, optionConditions: [{ option: 'High value', aggregator: 'and', conditions: [] }], @@ -125,12 +153,23 @@ describe('ConditionStepDefinitionSchema deterministic mode', () => { expect(result.success).toBe(false); }); + it('rejects preRecordedArgs with zero optionConditions', () => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + executionType: 'manual', + preRecordedArgs: { ...preRecordedArgs, optionConditions: [] }, + }); + + expect(result.success).toBe(false); + expect(JSON.stringify(!result.success && result.error.issues)).toContain('optionConditions'); + }); + it.each(['equal', 'not_equal', 'greater_than', 'in', 'contains'])( 'rejects a "%s" condition with no value', operator => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, - executionType: 'deterministic', + executionType: 'manual', preRecordedArgs: { ...preRecordedArgs, optionConditions: [ @@ -150,7 +189,7 @@ describe('ConditionStepDefinitionSchema deterministic mode', () => { it('accepts a value-less condition for present/blank operators', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, - executionType: 'deterministic', + executionType: 'manual', preRecordedArgs: { ...preRecordedArgs, optionConditions: [ @@ -166,7 +205,7 @@ describe('ConditionStepDefinitionSchema deterministic mode', () => { expect(result.success).toBe(true); }); - it('still accepts non-deterministic modes without preRecordedArgs', () => { + it('still accepts a condition with no preRecordedArgs at all', () => { expect( ConditionStepDefinitionSchema.safeParse({ ...base, executionType: 'fully-automated' }) .success, From fbbea7640eb069b351dc5a7b51389c3020853b41 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 26 Aug 2026 15:08:18 +0200 Subject: [PATCH 5/8] refactor(workflow-executor): name the deterministic predicate, trace an ignored choice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isDeterministicConditionStep replaces the inline truthiness check: the invariant gets a name instead of a comment, and narrowing the step type removes the second parameter that only existed to carry it. A deterministic step already ignored incomingPendingData by design, but silently: a mixed-version fleet can pause the step on an args-blind instance, and the user's click then vanished with no log line. It now warns. Tests: the four .min(1) guards on sourceStepId / fieldName / option / fallbackOption were mutation-provably dead — an empty reference resolves to "not found", so the step routed to its fallback instead of reporting the broken config, the same failure the value-less-operator refinement exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/adapters/server-types.ts | 6 +- .../src/executors/condition-step-executor.ts | 26 ++++++-- .../src/types/validated/step-definition.ts | 18 ++++-- .../adapters/step-definition-mapper.test.ts | 7 +-- .../executors/condition-step-executor.test.ts | 3 +- .../test/types/step-definition.test.ts | 62 ++++++++++++++++--- 6 files changed, 91 insertions(+), 31 deletions(-) diff --git a/packages/workflow-executor/src/adapters/server-types.ts b/packages/workflow-executor/src/adapters/server-types.ts index e7d9d7e728..831479a87b 100644 --- a/packages/workflow-executor/src/adapters/server-types.ts +++ b/packages/workflow-executor/src/adapters/server-types.ts @@ -93,7 +93,7 @@ interface ServerWorkflowTaskLoadRelatedRecord extends ServerWorkflowTaskBase { executionType: | ServerStepExecutionTypeEnum.FullyAutomated | ServerStepExecutionTypeEnum.AutomatedWithConfirmation; - // Deterministic build-time config. Validated by the step-definition schema. + // Validated by the step-definition schema. preRecordedArgs?: { selectedRecordStepId?: string; relationName?: string }; } @@ -125,8 +125,8 @@ export interface ServerWorkflowCondition extends ServerWorkflowStepBase { executionType: ServerStepExecutionTypeEnum.Manual | ServerStepExecutionTypeEnum.FullyAutomated; prompt: string | null; automaticCompletion: false; - // Parsed from `forest:optionConditions` server-side (flowId → answer). Presence *is* the - // deterministic mode. Validated by the step-definition schema. + // Parsed server-side from `forest:optionConditions` (flowId → answer). Its presence is what makes + // the gateway deterministic. preRecordedArgs?: { optionConditions: Array<{ option: string; diff --git a/packages/workflow-executor/src/executors/condition-step-executor.ts b/packages/workflow-executor/src/executors/condition-step-executor.ts index 53d1d7336a..0955dca416 100644 --- a/packages/workflow-executor/src/executors/condition-step-executor.ts +++ b/packages/workflow-executor/src/executors/condition-step-executor.ts @@ -3,6 +3,7 @@ import type { ConditionEvaluation, StepExecutionData } from '../types/step-execu import type { ConditionStepDefinition, DeterministicCondition, + DeterministicConditionStep, } from '../types/validated/step-definition'; import type { ConditionStepOutcome } from '../types/validated/step-outcome'; @@ -13,7 +14,11 @@ import { InvalidStepDefinitionError, StepStateError } from '../errors'; import BaseStepExecutor from './base-step-executor'; import evaluateOperator from './deterministic-condition-evaluator'; import patchBodySchemas from '../http/pending-data-validators'; -import { StepExecutionMode, StepType } from '../types/validated/step-definition'; +import { + StepExecutionMode, + StepType, + isDeterministicConditionStep, +} from '../types/validated/step-definition'; interface GatewayToolArgs { option: string | null; @@ -67,9 +72,18 @@ export default class ConditionStepExecutor extends BaseStepExecutor { const { stepDefinition: step, incomingPendingData } = this.context; - // Conditions present *is* the deterministic mode: pure evaluation, no AI, no user input. - if (step.preRecordedArgs) { - return this.evaluateDeterministically(step, step.preRecordedArgs); + if (isDeterministicConditionStep(step)) { + // The config wins over an explicit human action, so say so: a mixed-version fleet can pause + // the step on an args-blind instance, and the user's click would otherwise vanish untraced. + if (incomingPendingData !== undefined) { + this.context.logger( + 'Warn', + 'Ignoring a submitted option: this decision is evaluated from its conditions', + this.logCtx, + ); + } + + return this.evaluateDeterministically(step); } // Manual mode: the user picks the option from the frontend. Wait for their input @@ -103,9 +117,9 @@ export default class ConditionStepExecutor extends BaseStepExecutor, + step: DeterministicConditionStep, ): Promise { + const { optionConditions, fallbackOption } = step.preRecordedArgs; const stepExecutions = await this.context.runStore.getStepExecutions(this.context.runId); let matchedOption: string | undefined; diff --git a/packages/workflow-executor/src/types/validated/step-definition.ts b/packages/workflow-executor/src/types/validated/step-definition.ts index 1ba735328b..0a3f2cc284 100644 --- a/packages/workflow-executor/src/types/validated/step-definition.ts +++ b/packages/workflow-executor/src/types/validated/step-definition.ts @@ -92,14 +92,12 @@ export type OptionConditions = z.infer; export const ConditionStepDefinitionSchema = z.object({ ...sharedFields, type: z.literal(StepType.Condition), - // NO `.catch` — coercing an unknown mode from a newer orchestrator to FullyAutomated would - // silently let the AI decide instead of the conditions the builder configured precisely - // because they don't trust the AI. + // NO `.catch` — coercing an unknown mode to FullyAutomated would turn a gateway the builder made + // manual into an AI decision. executionType: z.enum([Manual, FullyAutomated]).default(FullyAutomated), - /** Ordered — evaluation priority for the deterministic mode (top-to-bottom, first-match-wins). */ options: z.array(z.string()).min(2), - // Presence *is* the deterministic mode. A malformed config is rejected here rather than dropped, - // so it can never degrade to a manual/AI decision. + // A malformed config is rejected here rather than dropped, so it can never degrade to a + // manual/AI decision. preRecordedArgs: z .object({ optionConditions: z.array(OptionConditionsSchema).min(1), @@ -109,6 +107,14 @@ export const ConditionStepDefinitionSchema = z.object({ }); export type ConditionStepDefinition = z.infer; +export type DeterministicConditionStep = ConditionStepDefinition & + Required>; + +// Carrying the conditions *is* the deterministic mode — no executionType says so. +export const isDeterministicConditionStep = ( + step: ConditionStepDefinition, +): step is DeterministicConditionStep => step.preRecordedArgs !== undefined; + export const ReadRecordStepDefinitionSchema = z.object({ ...sharedFields, type: z.literal(StepType.ReadRecord), diff --git a/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts b/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts index c1e809dd04..4e1b00f660 100644 --- a/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts +++ b/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts @@ -309,10 +309,7 @@ describe('toStepDefinition', () => { }); }); - // A newer orchestrator may send an execution mode this executor version does not know, and - // `deterministic` is now one of them. The `.catch(FullyAutomated)` that used to sit on the - // condition schema would have silently handed the decision to the AI; the mapper must reject - // the run as malformed instead. + // 'deterministic' is now just another unknown mode: the wire contract dropped it. it.each(['not-a-mode', 'deterministic'])( 'should throw InvalidStepDefinitionError for the unknown executionType "%s" instead of coercing to Full AI', executionType => { @@ -329,8 +326,6 @@ describe('toStepDefinition', () => { }, ); - // A deterministic gateway publishes with `aiDecision` stripped, so it arrives as `manual` and - // the preRecordedArgs are the only thing that marks it deterministic. it('should forward preRecordedArgs on a manual condition', () => { const preRecordedArgs = { optionConditions: [ diff --git a/packages/workflow-executor/test/executors/condition-step-executor.test.ts b/packages/workflow-executor/test/executors/condition-step-executor.test.ts index 6a528471f7..402b737eb7 100644 --- a/packages/workflow-executor/test/executors/condition-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/condition-step-executor.test.ts @@ -28,8 +28,7 @@ function makeStep(overrides: Partial = {}): ConditionSt type ConditionPreRecordedArgs = NonNullable; -// A deterministic gateway is published with `aiDecision` stripped, so the orchestrator sends it as -// `manual` + preRecordedArgs — the args are what make it deterministic, not the mode. +// Published with aiDecision stripped, so a deterministic gateway arrives as manual: the args make it deterministic, not the mode. function makeDeterministicStep( preRecordedArgs: ConditionPreRecordedArgs, executionType: ConditionStepDefinition['executionType'] = StepExecutionMode.Manual, diff --git a/packages/workflow-executor/test/types/step-definition.test.ts b/packages/workflow-executor/test/types/step-definition.test.ts index e56c9a4cc9..36a6dc8758 100644 --- a/packages/workflow-executor/test/types/step-definition.test.ts +++ b/packages/workflow-executor/test/types/step-definition.test.ts @@ -25,10 +25,8 @@ describe('ConditionStepDefinitionSchema executionType', () => { ); }); - // No `.catch` on the enum: an unknown value must be rejected, not silently coerced to - // FullyAutomated, which would hand the decision to the AI. `deterministic` is one such unknown - // value now that it is gone from the wire contract. - it.each(['not-a-mode', 'deterministic', 'whatever'])( + // 'deterministic' is now an unknown mode — and unknown modes must be rejected, not coerced. + it.each(['not-a-mode', 'deterministic'])( 'rejects the unknown executionType "%s" instead of coercing it', executionType => { expect(ConditionStepDefinitionSchema.safeParse({ ...base, executionType }).success).toBe( @@ -73,8 +71,6 @@ describe('ConditionStepDefinitionSchema deterministic conditions', () => { fallbackOption: 'Fallback', }; - // A deterministic gateway publishes with `aiDecision` stripped, so it reaches the executor as - // `manual` + preRecordedArgs: the args carry the mode, the executionType does not. it('accepts manual with preRecordedArgs and round-trips them', () => { const parsed = ConditionStepDefinitionSchema.parse({ ...base, @@ -93,8 +89,7 @@ describe('ConditionStepDefinitionSchema deterministic conditions', () => { expect(parsed.preRecordedArgs).toEqual(preRecordedArgs); }); - // The `value` is supplied on purpose: without it the value-less-operator refinement would - // reject the condition anyway, and the test would pass without pinning the operator enum. + // value supplied so the failure is the operator enum, not the value-less-operator refinement. it('rejects an unknown operator at the schema boundary', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, @@ -117,6 +112,57 @@ describe('ConditionStepDefinitionSchema deterministic conditions', () => { expect(JSON.stringify(!result.success && result.error.issues)).toContain('operator'); }); + // An empty reference resolves to "not found" → met: null → not met → the step routes to the + // fallback. Same silent-fallback failure the value-less-operator refinement exists to prevent. + it.each([ + ['sourceStepId', { sourceStepId: '', fieldName: 'amount' }], + ['fieldName', { sourceStepId: 'get-data-1', fieldName: '' }], + ])('rejects a condition with an empty %s', (_label, reference) => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + preRecordedArgs: { + ...preRecordedArgs, + optionConditions: [ + { + option: 'High value', + aggregator: 'and', + conditions: [{ ...reference, operator: 'equal', value: 1 }], + }, + ], + }, + }); + + expect(result.success).toBe(false); + expect(JSON.stringify(!result.success && result.error.issues)).toContain(_label); + }); + + it.each([ + ['option', { option: '', fallbackOption: 'Otherwise' }], + ['fallbackOption', { option: 'High value', fallbackOption: '' }], + ])( + 'rejects an empty %s, which no outgoing flow can answer', + (_label, { option, fallbackOption }) => { + const result = ConditionStepDefinitionSchema.safeParse({ + ...base, + preRecordedArgs: { + fallbackOption, + optionConditions: [ + { + option, + aggregator: 'and', + conditions: [ + { sourceStepId: 'get-data-1', fieldName: 'amount', operator: 'equal', value: 1 }, + ], + }, + ], + }, + }); + + expect(result.success).toBe(false); + expect(JSON.stringify(!result.success && result.error.issues)).toContain(_label); + }, + ); + it('rejects preRecordedArgs missing fallbackOption', () => { const result = ConditionStepDefinitionSchema.safeParse({ ...base, From d3a7eee5011ca512ec166481953fd12eb71897c0 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 26 Aug 2026 15:39:17 +0200 Subject: [PATCH 6/8] fix(workflow-executor): log the reference a deterministic condition could not resolve A step that reaches its fallback because a Get Data never ran reports success, so the run view is the only place saying why. An operator watching the logs saw nothing at all. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/executors/condition-step-executor.ts | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/workflow-executor/src/executors/condition-step-executor.ts b/packages/workflow-executor/src/executors/condition-step-executor.ts index 0955dca416..f6f541c3fa 100644 --- a/packages/workflow-executor/src/executors/condition-step-executor.ts +++ b/packages/workflow-executor/src/executors/condition-step-executor.ts @@ -172,9 +172,20 @@ export default class ConditionStepExecutor extends BaseStepExecutor Date: Wed, 26 Aug 2026 15:55:13 +0200 Subject: [PATCH 7/8] fix(workflow-executor): fail loud when a condition's source data was never loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deterministic condition reading a field the Get Data step did not load resolved to "not met", so the run took its fallback and reported success — the decision looked taken when its input never arrived. It now throws ConditionSourceNotLoadedError, naming the field and the step. This is reachable from the editor, not just from hand-edited BPMN: a Get Data step may let the AI pick its fields, and then no build-time validation can know which ones will come back. The orchestrator's parse-time check is structurally unable to cover it, so the runtime has to. Replaces the Warn added in d8cc48e9b, and aligns the Condition step with every other type (FieldNotFoundError, RelationNotFoundError, ActionNotFoundError) — it was the only one swallowing an unresolvable reference. A resolved value that is null still counts as not met, per spec: data must not fail a decision. Only a missing reference does. Co-Authored-By: Claude Opus 5 (1M context) --- packages/workflow-executor/CLAUDE.md | 2 +- packages/workflow-executor/src/errors.ts | 13 +++++ .../src/executors/condition-step-executor.ts | 22 ++++----- .../executors/condition-step-executor.test.ts | 49 +++++++------------ 4 files changed, 41 insertions(+), 45 deletions(-) diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 28d8630dd2..5e5a033084 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -35,7 +35,7 @@ Front ◀──▶ Orchestrator ◀──pull/push──▶ Executor (this p `StepExecutionMode` (domain enum, mapped from the server contract in `step-definition-mapper.ts`): `Manual`, `AutomatedWithConfirmation`, `FullyAutomated`. There is **no deterministic mode on the wire**: a condition step is deterministic iff it carries `preRecordedArgs.optionConditions`. A deterministic gateway publishes with `aiDecision` stripped, so it arrives as `Manual` — an executor blind to the args degrades to a visible manual decision, never a silent AI one. -- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (wire-final operator names in `CONDITION_OPERATORS`; a value-bearing operator missing its `value` is rejected at the schema boundary — comparing against `undefined` can never be met, so the step would route to the fallback instead of surfacing the broken config) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; unresolvable/null value = `met: null` = not met (**never an error**); no match → `fallbackOption`. Never calls AI, never awaits input (`incomingPendingData` is ignored — no user override). The selected option (match or fallback) is checked against `step.options` before persisting → `InvalidStepDefinitionError`, because `optionConditions` and `options` come from two different server-side derivations and an unroutable option must not travel as a "success". Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. +- **Deterministic Condition** (`condition-step-executor.ts` + `deterministic-condition-evaluator.ts`, PRD-472) — evaluates build-time `preRecordedArgs.optionConditions` (wire-final operator names in `CONDITION_OPERATORS`; a value-bearing operator missing its `value` is rejected at the schema boundary — comparing against `undefined` can never be met, so the step would route to the fallback instead of surfacing the broken config) against Get Data outputs in the run history (`sourceStepId` + `fieldName` → read-record `executionResult.fields`). Top-to-bottom, first-match-wins, `and`/`or` aggregator; no match → `fallbackOption`. **A resolved value that is null = `met: null` = not met, never an error** (data must not fail a decision) — but an **unresolvable reference throws** `ConditionSourceNotLoadedError`: "no value was read" is a broken config, not data, and routing to the fallback would report a decision as taken when its input never arrived. Build-time validation cannot cover it, because a Get Data step may let the AI pick its fields. Same choice as every other step type (`FieldNotFoundError`, `RelationNotFoundError`, `ActionNotFoundError`). Never calls AI, never awaits input (`incomingPendingData` is ignored — no user override). The selected option (match or fallback) is checked against `step.options` before persisting → `InvalidStepDefinitionError`, because `optionConditions` and `options` come from two different server-side derivations and an unroutable option must not travel as a "success". Evaluation trace persisted in `executionParams` (`evaluations`/`selectedOption`/`usedFallback`) for the run view; the fallback never appears in `evaluations`. - Comparison semantics (`deterministic-condition-evaluator.ts`): a strictly numeric string is coerced **against a real number only** (Sequelize returns `numeric`/`decimal`/`bigint` as strings while the datasource types them `Number`); an offset-less ISO datetime is read as **UTC** (host-TZ parsing would make the same run route differently per machine); an impossible calendar date is **not a date** (`Date.parse('2026-02-30')` rolls over to 2026-03-02, which would make a nonsensical config compare equal to a real value); a type mismatch satisfies no operator, negated ones included (`not_equal(true, 'true')` = not met); `contains`/`not_contains` are strings-only, per the contract. - **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. diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 657afc5228..96a950b8af 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -543,6 +543,19 @@ export class InvalidStepDefinitionError extends WorkflowExecutorError { } } +// A deterministic condition reads a field a Get Data step was supposed to have loaded. Build-time +// validation cannot always catch this: when that step lets the AI pick its fields, nobody knows +// which ones it will return until the run. Treating it as "not met" would route to the fallback and +// report success — the decision would look taken when its input never arrived. +export class ConditionSourceNotLoadedError extends WorkflowExecutorError { + constructor(fieldName: string, sourceStepId: string) { + super( + `Condition reads "${fieldName}" from step "${sourceStepId}", which did not load it`, + `This decision compares the field "${fieldName}", but the step it reads from did not load that field. Add it to that step's fields, or remove the condition.`, + ); + } +} + // Thrown when zod validation fails on a domain object produced internally (e.g. by the // run-to-pending-step mapper). Distinct from InvalidStepDefinitionError (which flags wire-format // bugs coming from the orchestrator) so the two can be triaged separately in Sentry. diff --git a/packages/workflow-executor/src/executors/condition-step-executor.ts b/packages/workflow-executor/src/executors/condition-step-executor.ts index f6f541c3fa..b770c23805 100644 --- a/packages/workflow-executor/src/executors/condition-step-executor.ts +++ b/packages/workflow-executor/src/executors/condition-step-executor.ts @@ -10,7 +10,11 @@ import type { ConditionStepOutcome } from '../types/validated/step-outcome'; import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; import { z } from 'zod'; -import { InvalidStepDefinitionError, StepStateError } from '../errors'; +import { + ConditionSourceNotLoadedError, + InvalidStepDefinitionError, + StepStateError, +} from '../errors'; import BaseStepExecutor from './base-step-executor'; import evaluateOperator from './deterministic-condition-evaluator'; import patchBodySchemas from '../http/pending-data-validators'; @@ -173,18 +177,12 @@ export default class ConditionStepExecutor extends BaseStepExecutor { }); }); - it('treats an unresolvable source step as not evaluable and falls back', async () => { + it('fails loud when the source step never ran', async () => { const unknownSource: ConditionPreRecordedArgs = { optionConditions: [ { @@ -741,34 +741,29 @@ describe('ConditionStepExecutor', () => { ], fallbackOption: 'Other', }; - const { context } = makeDeterministicContext(unknownSource, [ + const { context, runStore } = makeDeterministicContext(unknownSource, [ { name: 'amount', displayName: 'Amount', value: 150 }, ]); const result = await new ConditionStepExecutor(context).execute(); - expect(result.stepOutcome.status).toBe('success'); - expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome.error).toContain('did not load that field'); + expect(runStore.saveStepExecution).not.toHaveBeenCalled(); }); - it('treats a field the Get Data step failed to read as not evaluable', async () => { + // Build-time validation cannot catch this one: the Get Data step may let the AI pick its + // fields, so nobody knows which ones it returns until the run. + it('fails loud when the Get Data step failed to read the field', async () => { const { context, runStore } = makeDeterministicContext(amountArgs, [ { name: 'amount', displayName: 'Amount', error: 'Field not found: amount' }, ]); const result = await new ConditionStepExecutor(context).execute(); - expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); - expect(runStore.saveStepExecution).toHaveBeenCalledWith( - 'run-1', - expect.objectContaining({ - executionParams: expect.objectContaining({ - evaluations: expect.arrayContaining([ - { option: 'High', outcome: 'not-matched', conditions: [{ index: 0, met: null }] }, - ]), - }), - }), - ); + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome.error).toContain('did not load that field'); + expect(runStore.saveStepExecution).not.toHaveBeenCalled(); }); it('lets blank match a resolved null value (unlike an unresolvable one)', async () => { @@ -876,8 +871,10 @@ describe('ConditionStepExecutor', () => { ); }); + // Not even present/blank get an answer out of a reference that was never loaded: "no value was + // read" is not the same claim as "the value is empty". it.each(['blank', 'present'] as const)( - 'does not satisfy %s when the reference cannot be resolved at all', + 'fails loud on %s when the reference cannot be resolved at all', async operator => { const unresolvable: ConditionPreRecordedArgs = { optionConditions: [ @@ -895,21 +892,9 @@ describe('ConditionStepExecutor', () => { const result = await new ConditionStepExecutor(context).execute(); - expect((result.stepOutcome as ConditionStepOutcome).selectedOption).toBe('Other'); - expect(runStore.saveStepExecution).toHaveBeenCalledWith( - 'run-1', - expect.objectContaining({ - executionParams: expect.objectContaining({ - evaluations: [ - { - option: 'Matched', - outcome: 'not-matched', - conditions: [{ index: 0, met: null }], - }, - ], - }), - }), - ); + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome.error).toContain('did not load that field'); + expect(runStore.saveStepExecution).not.toHaveBeenCalled(); }, ); From 243534e1bc755ec0089b10a03c3659cb6c96b966 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 26 Aug 2026 16:50:12 +0200 Subject: [PATCH 8/8] fix(workflow-executor): report every hydration failure, not just domain ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catch in getAvailableRuns only pushed WorkflowExecutorError into malformed; anything else was logged and dropped. Nothing reached the orchestrator, so the run stayed pending and came back on every poll, failing the same way forever. Every hydration path currently throws a domain error, so this branch is unreachable today — but it is unreachable by construction, not by proof. A future mapper throwing a TypeError would silently reintroduce the stuck-run bug this epic already fixed once. toMalformedInfo now tolerates a non-array workflowHistory: it is called for runs that are broken in arbitrary ways, and it used to rethrow while building the report. A run so malformed that no step can be identified still cannot be reported — updateStepExecution needs a stepId — so that case stays a log. Co-Authored-By: Claude Opus 5 (1M context) --- .../adapters/forest-server-workflow-port.ts | 26 ++++++++++++------- packages/workflow-executor/src/errors.ts | 9 +++++++ .../forest-server-workflow-port.test.ts | 20 +++++++++----- 3 files changed, 40 insertions(+), 15 deletions(-) diff --git a/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts b/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts index ef7ea8f755..1a25c46775 100644 --- a/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts +++ b/packages/workflow-executor/src/adapters/forest-server-workflow-port.ts @@ -20,6 +20,7 @@ import toUpdateStepRequest from './step-outcome-to-update-step-mapper'; import withRetry from './with-retry'; import { DomainValidationError, + HydrationFailedError, InvalidStepDefinitionError, MalformedRunError, WorkflowExecutorError, @@ -71,14 +72,20 @@ export default class ForestServerWorkflowPort implements WorkflowPort { const dispatch = this.toDispatch(run); if (dispatch) pending.push(dispatch); } catch (error) { - if (error instanceof WorkflowExecutorError) { - malformed.push(this.toMalformedInfo(run, error)); - } else { - this.logger('Error', 'Failed to hydrate pending run — unexpected error', { - runId: run.id, - error: extractErrorMessage(error), - }); - } + // Reported whatever it is: an unreported failure leaves the run pending, so every poll + // returns it and fails again. + this.logger('Error', 'Failed to hydrate pending run', { + runId: run.id, + error: extractErrorMessage(error), + }); + malformed.push( + this.toMalformedInfo( + run, + error instanceof WorkflowExecutorError + ? error + : new HydrationFailedError(extractErrorMessage(error)), + ), + ); } } @@ -137,7 +144,8 @@ export default class ForestServerWorkflowPort implements WorkflowPort { run: ServerHydratedWorkflowRun, err: WorkflowExecutorError, ): MalformedRunInfo { - const pending = run.workflowHistory.at(-1) ?? null; + const history = Array.isArray(run.workflowHistory) ? run.workflowHistory : []; + const pending = history.at(-1) ?? null; return { runId: String(run.id), diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 96a950b8af..afd302ceb5 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -543,6 +543,15 @@ export class InvalidStepDefinitionError extends WorkflowExecutorError { } } +export class HydrationFailedError extends WorkflowExecutorError { + constructor(detail: string) { + super( + `Failed to hydrate run: ${detail}`, + 'This workflow run could not be prepared for execution. Please contact support.', + ); + } +} + // A deterministic condition reads a field a Get Data step was supposed to have loaded. Build-time // validation cannot always catch this: when that step lets the AI pick its fields, nobody knows // which ones it will return until the run. Treating it as "not met" would route to the fallback and 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..896e6d7265 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 @@ -313,21 +313,29 @@ describe('ForestServerWorkflowPort', () => { ); }); - it('logs and skips when the mapping throws a non-WorkflowExecutorError', async () => { + it('reports a non-WorkflowExecutorError as malformed instead of skipping the run', async () => { const logger = jest.fn(); const portWithLogger = new ForestServerWorkflowPort({ ...options, logger }); - // Simulate a non-domain error by passing a run whose workflowHistory will - // blow up a pure JS operation inside the mapper (missing `find` on non-array). - const brokenRun = { ...makeRun({ id: 111 }), workflowHistory: null as never }; + // A numeric selectedRecordId passes the mapper's truthiness guard, then blows up on .split + // — a TypeError, not a domain error, on a run whose history is otherwise intact. + const brokenRun = { ...makeRun({ id: 111 }), selectedRecordId: 42 as never }; mockQuery.mockResolvedValue([brokenRun]); const result = await portWithLogger.getAvailableRuns(); expect(result.pending).toEqual([]); - expect(result.malformed).toEqual([]); + expect(result.malformed).toEqual([ + expect.objectContaining({ + runId: '111', + stepId: expect.any(String), + stepIndex: expect.any(Number), + userMessage: + 'This workflow run could not be prepared for execution. Please contact support.', + }), + ]); expect(logger).toHaveBeenCalledWith( 'Error', - 'Failed to hydrate pending run — unexpected error', + 'Failed to hydrate pending run', expect.objectContaining({ runId: 111 }), ); });