From 940a0093b25d50719dcf198534c3d9a86e6db152 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 18 Aug 2026 20:19:24 +0200 Subject: [PATCH] feat(workflow-executor): reject unknown condition executionType instead of coercing to Full AI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An out-of-enum executionType on a condition step was silently coerced to fully-automated by the schema's .catch, handing the decision to the AI — the exact opposite of the upcoming deterministic mode's intent (PRD-472). The schema now rejects unknown values, and every mapper parse failure is wrapped in InvalidStepDefinitionError so the orchestrator reports the run as malformed instead of logging-and-dropping it on every poll. Part of PRD-472 Co-Authored-By: Claude Opus 5 (1M context) --- .../src/adapters/step-definition-mapper.ts | 40 ++++++++++++++----- .../src/types/validated/step-definition.ts | 10 +++-- .../adapters/step-definition-mapper.test.ts | 18 ++++++++- .../test/types/step-definition.test.ts | 32 +++++++++++++++ 4 files changed, 87 insertions(+), 13 deletions(-) diff --git a/packages/workflow-executor/src/adapters/step-definition-mapper.ts b/packages/workflow-executor/src/adapters/step-definition-mapper.ts index a45e99c29f..bf809ed538 100644 --- a/packages/workflow-executor/src/adapters/step-definition-mapper.ts +++ b/packages/workflow-executor/src/adapters/step-definition-mapper.ts @@ -4,6 +4,7 @@ import type { ServerWorkflowTask, } from './server-types'; import type { ConditionStepDefinition, StepDefinition } from '../types/validated/step-definition'; +import type { z } from 'zod'; import { ServerTaskTypeEnum } from './server-types'; import { InvalidStepDefinitionError, UnsupportedStepTypeError } from '../errors'; @@ -18,41 +19,62 @@ import { UpdateRecordStepDefinitionSchema, } from '../types/validated/step-definition'; +// A bare ZodError escaping this mapper is logged-and-dropped by the port's getAvailableRuns +// (only WorkflowExecutorError instances are reported as malformed), leaving the run silently +// re-fetched on every poll — wrap parse failures so the run is reported to the orchestrator. +function parseStepDefinition( + schema: Schema, + input: unknown, +): z.infer { + const result = schema.safeParse(input); + if (result.success) return result.data; + + const detail = result.error.issues + .map(issue => `${issue.path.join('.') || '(root)'}: ${issue.message}`) + .join('; '); + + throw new InvalidStepDefinitionError(detail); +} + function mapTask(task: ServerWorkflowTask): StepDefinition { // executionType is passed through as-is. Each schema applies its own `.default()` for a missing - // value; schemas that accept `manual` (guidance, load-related, trigger-action) drop `.catch` and - // reject an out-of-enum value rather than coercing it — server values are a 1:1 enum mapping today. + // value; schemas that accept `manual` (guidance, load-related, trigger-action) drop `.catch` + // and reject an out-of-enum value rather than coercing it — server values are a 1:1 enum + // mapping today. const base = { prompt: task.prompt, executionType: task.executionType, title: task.title }; switch (task.taskType) { case ServerTaskTypeEnum.McpServer: - return McpStepDefinitionSchema.parse({ + return parseStepDefinition(McpStepDefinitionSchema, { ...base, type: StepType.Mcp, mcpServerId: task.mcpServerId, }); case ServerTaskTypeEnum.Guideline: - return GuidanceStepDefinitionSchema.parse({ ...base, type: StepType.Guidance }); + return parseStepDefinition(GuidanceStepDefinitionSchema, { + ...base, + type: StepType.Guidance, + }); case ServerTaskTypeEnum.GetData: - return ReadRecordStepDefinitionSchema.parse({ + return parseStepDefinition(ReadRecordStepDefinitionSchema, { ...base, type: StepType.ReadRecord, preRecordedArgs: task.preRecordedArgs, }); case ServerTaskTypeEnum.UpdateData: - return UpdateRecordStepDefinitionSchema.parse({ + return parseStepDefinition(UpdateRecordStepDefinitionSchema, { ...base, type: StepType.UpdateRecord, preRecordedArgs: task.preRecordedArgs, }); case ServerTaskTypeEnum.TriggerAction: - return TriggerActionStepDefinitionSchema.parse({ + return parseStepDefinition(TriggerActionStepDefinitionSchema, { ...base, type: StepType.TriggerAction, preRecordedArgs: task.preRecordedArgs, }); case ServerTaskTypeEnum.LoadRelatedRecord: - return LoadRelatedRecordStepDefinitionSchema.parse({ + return parseStepDefinition(LoadRelatedRecordStepDefinitionSchema, { ...base, type: StepType.LoadRelatedRecord, preRecordedArgs: task.preRecordedArgs, @@ -75,7 +97,7 @@ function mapCondition(condition: ServerWorkflowCondition): ConditionStepDefiniti ); } - return ConditionStepDefinitionSchema.parse({ + return parseStepDefinition(ConditionStepDefinitionSchema, { type: StepType.Condition, prompt: condition.prompt, executionType: condition.executionType, diff --git a/packages/workflow-executor/src/types/validated/step-definition.ts b/packages/workflow-executor/src/types/validated/step-definition.ts index 471b1255c8..dc80f540f5 100644 --- a/packages/workflow-executor/src/types/validated/step-definition.ts +++ b/packages/workflow-executor/src/types/validated/step-definition.ts @@ -21,8 +21,9 @@ export enum StepExecutionMode { } // Shared fields across all step types. executionType is intentionally excluded — -// each schema declares its own valid modes (most with .default().catch() for normalization; -// guidance deliberately omits .catch to fail loud on an unknown mode). +// each schema declares its own valid modes (read/update/mcp normalize with .default().catch(); +// condition, trigger-action, load-related-record and guidance deliberately omit .catch to fail +// loud on an unknown mode). // The orchestrator serializes missing BPMN attributes as JSON null (DOM getAttribute), not as // absent keys — accept both and normalize to undefined. const optionalString = z @@ -42,7 +43,10 @@ const { Manual, AutomatedWithConfirmation, FullyAutomated } = StepExecutionMode; export const ConditionStepDefinitionSchema = z.object({ ...sharedFields, type: z.literal(StepType.Condition), - executionType: z.enum([Manual, FullyAutomated]).default(FullyAutomated).catch(FullyAutomated), + // 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), }); export type ConditionStepDefinition = z.infer; 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 a10fad9a6d..398df8c12c 100644 --- a/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts +++ b/packages/workflow-executor/test/adapters/step-definition-mapper.test.ts @@ -146,7 +146,7 @@ describe('toStepDefinition', () => { it('rejects an mcp-server task missing mcpServerId at the zod boundary', () => { const task = makeTask({ taskType: ServerTaskTypeEnum.McpServer, prompt: 'run mcp' }); - expect(() => toStepDefinition(task)).toThrow(); + expect(() => toStepDefinition(task)).toThrow(InvalidStepDefinitionError); }); it('should map task with guideline taskType to guidance', () => { @@ -309,6 +309,22 @@ describe('toStepDefinition', () => { }); }); + // A newer orchestrator may send a deterministic 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: 'deterministic' as ServerWorkflowCondition['executionType'] }, + ); + + expect(() => toStepDefinition(condition)).toThrow(InvalidStepDefinitionError); + expect(() => toStepDefinition(condition)).toThrow(/executionType/); + }); + it('should throw InvalidStepDefinitionError when fewer than 2 options', () => { const condition = makeCondition([{ stepId: 's1', buttonText: 'Only' }]); diff --git a/packages/workflow-executor/test/types/step-definition.test.ts b/packages/workflow-executor/test/types/step-definition.test.ts index 5118d59ca5..ff4337d8ad 100644 --- a/packages/workflow-executor/test/types/step-definition.test.ts +++ b/packages/workflow-executor/test/types/step-definition.test.ts @@ -1,10 +1,42 @@ import { + ConditionStepDefinitionSchema, GuidanceStepDefinitionSchema, LoadRelatedRecordStepDefinitionSchema, StepExecutionMode, StepType, } from '../../src/types/validated/step-definition'; +describe('ConditionStepDefinitionSchema executionType', () => { + const base = { type: StepType.Condition as const, options: ['Yes', 'No'] }; + + it('parses each valid execution mode to its own value', () => { + expect( + ConditionStepDefinitionSchema.parse({ ...base, executionType: 'manual' }).executionType, + ).toBe(StepExecutionMode.Manual); + expect( + ConditionStepDefinitionSchema.parse({ ...base, executionType: 'fully-automated' }) + .executionType, + ).toBe(StepExecutionMode.FullyAutomated); + }); + + it('defaults a missing executionType to FullyAutomated', () => { + expect(ConditionStepDefinitionSchema.parse(base).executionType).toBe( + StepExecutionMode.FullyAutomated, + ); + }); + + // 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). + 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('LoadRelatedRecordStepDefinitionSchema executionType', () => { const base = { type: StepType.LoadRelatedRecord as const };