From 194ab77dbd6a27fdfbb4951a4c3ddfc48ece02d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 15:27:42 +0000 Subject: [PATCH] fix(service-automation): route every retry attempt through the node input-schema guard (#9889) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit executeWithoutRetry — the method retryExecution re-runs the flow through on every retry attempt — never called validateNodeInputSchemas, so a flow whose node config violates its own declared inputSchema under errorHandling.strategy: 'retry' was refused on attempt 1 and executed for real on attempts 2..N. Both attempt paths now call the same guard (the seedRunVariables chokepoint discipline from #9704); retry accounting is unchanged. Pins assert the side-effecting node never runs on any attempt, and that a valid flow still retries normally. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../automation-input-schema-retry-parity.md | 23 +++ .../services/service-automation/src/engine.ts | 40 ++++- .../src/input-schema-retry-parity.test.ts | 150 ++++++++++++++++++ 3 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 .changeset/automation-input-schema-retry-parity.md create mode 100644 packages/services/service-automation/src/input-schema-retry-parity.test.ts diff --git a/.changeset/automation-input-schema-retry-parity.md b/.changeset/automation-input-schema-retry-parity.md new file mode 100644 index 0000000000..391126f8c8 --- /dev/null +++ b/.changeset/automation-input-schema-retry-parity.md @@ -0,0 +1,23 @@ +--- +"@objectstack/service-automation": patch +--- + +fix(service-automation): node input-schema validation now guards EVERY attempt of a retried flow, not only the first (#9889) + +Before this fix, `validateNodeInputSchemas` — the guard that refuses to run a +flow whose node `config` violates its own declared `inputSchema` — was called +only by `execute()` (attempt 1). Under `errorHandling.strategy: 'retry'`, the +guard's throw routed into `retryExecution`, and every retry attempt ran +through `executeWithoutRetry` with no guard at all: the nodes attempt 1 +refused to run were executed for real, with the config the guard rejected. A +side-effecting node (a data write, an HTTP call, an email) behind a +mis-declared `inputSchema` was reachable simply by declaring `retry`. + +Now both attempt paths call the same guard, so flows that were previously +running on retry with a mis-declared `inputSchema` will be refused on every +attempt (`success: false`, `status: 'failed'`, with the guard's message). +Retry accounting is unchanged: each refused attempt still consumes retry +budget, and valid flows retry exactly as before. If a flow of yours starts +failing with `missing required input parameter` or `expected type ... but +got ...` after this release, it was already being refused on its first +attempt — fix the node's `config` to match its declared `inputSchema`. diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 2d61de4d9c..35d3e3f51e 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -3184,7 +3184,10 @@ export class AutomationEngine implements IAutomationService { reentryHeld = true; } - // Validate node input schemas before execution + // Validate node input schemas before execution. [#9889] The same + // call sits in `executeWithoutRetry` — every retry attempt must be + // refused by the same guard that refused attempt 1; see the guard's + // own doc for the chokepoint contract. this.validateNodeInputSchemas(flow, variables); // DAG traversal execution @@ -5257,6 +5260,18 @@ export class AutomationEngine implements IAutomationService { /** * Validate node input schemas before execution. * Checks that node config matches declared inputSchema if present. + * + * [#9889] The ONE definition-level input-schema chokepoint, called by BOTH + * attempt paths — `execute()` (attempt 1) and `executeWithoutRetry` (every + * retry attempt) — the same chokepoint discipline `seedRunVariables` + * carries for the variable environment (#9704). Its verdict is a pure + * function of the flow definition (`_variables` is deliberately unused), + * so a refusal on attempt 1 must hold on every attempt: when only + * `execute()` called it, `errorHandling.strategy: 'retry'` ran the very + * nodes attempt 1 refused to run. ⛔ A repair to the validation rules + * belongs HERE, in the shared method — re-inlining either caller's copy + * re-opens the execute/executeWithoutRetry drift this file has now paid + * for six times (#9378, #9415, #9414, #9510, #9704, #9889). */ private validateNodeInputSchemas(flow: FlowParsed, _variables: Map): void { for (const node of flow.nodes) { @@ -6433,6 +6448,29 @@ export class AutomationEngine implements IAutomationService { return { success: false, code: 'FLOW_NO_START_NODE', error: 'Flow has no start node' }; } + // [#9889] The SAME definition-level guard attempt 1 runs under — + // the sixth instance of this method drifting from `execute()` + // (#9378, #9415, #9414, #9510, #9704 before it), and the first + // that skipped a GUARD rather than an exit or the environment. + // Without this call, a flow whose node config violates its own + // declared `inputSchema` under `errorHandling.strategy: 'retry'` + // was refused on attempt 1 (the guard throws before any node + // executes) and then RUN FOR REAL on attempts 2..N, because the + // retry handoff lives in `execute()`'s catch and every retry + // attempt comes back through here — a refusal that holds only + // until the flow is retried, i.e. `retry` as a way past + // authoring-time validation. The guard's verdict is a pure + // function of the flow definition (`_variables` is unused), so + // re-running it cannot refuse anything attempt 1 would have + // allowed; the throw lands in this method's generic failure arm + // below, so each refused attempt still consumes retry budget and + // retry accounting is unchanged. Same chokepoint discipline as + // `seedRunVariables` (#9704): ONE method holds the rules, both + // attempt paths call it — `input-schema-retry-parity.test.ts` + // pins the per-attempt refusal so this call cannot be dropped + // silently. + this.validateNodeInputSchemas(flow, variables); + await this.executeNode(startNode, flow, variables, runContext, steps); const output: Record = {}; diff --git a/packages/services/service-automation/src/input-schema-retry-parity.test.ts b/packages/services/service-automation/src/input-schema-retry-parity.test.ts new file mode 100644 index 0000000000..3fc5956873 --- /dev/null +++ b/packages/services/service-automation/src/input-schema-retry-parity.test.ts @@ -0,0 +1,150 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from './engine.js'; + +/** + * #9889 — node input-schema validation must hold on EVERY attempt, not only + * the first. + * + * `validateNodeInputSchemas` reports by throwing, and the retry handoff lives + * inside `execute()`'s catch. Before the repair, only `execute()` called the + * guard: for a flow whose node config violates its own declared `inputSchema` + * under `errorHandling.strategy: 'retry'`, attempt 1 threw in the guard before + * any node executed, the catch routed to `retryExecution`, and attempts 2..N + * ran through `executeWithoutRetry` — which never called the guard — so the + * nodes attempt 1 refused permission to run were executed for real, with the + * config the guard rejected. A `retry` strategy was a way past authoring-time + * validation. + * + * The pins here are written against the OBSERVABLE side effect (an executor + * spy counting real node executions), not only the thrown error: the defect's + * whole harm is a side-effecting node (a data write, an HTTP call, an email) + * running with rejected config, and an assertion on the returned error alone + * stays green while that node runs. + * + * On the envelope: the refusal is asserted as `success: false` + + * `status: 'failed'` + the guard's own message. There is no ADR-0112 `code` + * to assert — deliberately: #9378's classification gives `code` to the + * NEVER-DISPATCHED exits (`FLOW_DISABLED`, `FLOW_NO_START_NODE`) and `status: + * 'failed'` to the dispatched-and-failed exits, and the guard's throw rides + * the latter family on both attempt paths. Whether a definition-level refusal + * should instead be classified non-retryable (its verdict cannot change per + * attempt) is the open question #9889 leaves to a maintainer ruling; these + * pins assert the parity floor only. + */ + +function createTestLogger(): any { + return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => createTestLogger() }; +} + +/** + * An engine holding one flow whose single work node counts every REAL + * execution — the observable the negative pins are written against. + */ +function countingFlowEngine(opts: { + config: Record; + inputSchema: Record; + /** What the spy executor returns; defaults to success. */ + executeResult?: (attempt: number) => { success: boolean; error?: string }; +}) { + const engine = new AutomationEngine(createTestLogger()); + const runs = { count: 0 }; + + engine.registerNodeExecutor({ + type: 'script', + async execute() { + runs.count++; + return opts.executeResult ? opts.executeResult(runs.count) : { success: true }; + }, + } as any); + + engine.registerFlow('guarded', { + name: 'guarded', + label: 'Guarded', + type: 'autolaunched', + errorHandling: { strategy: 'retry', maxRetries: 2, backoffMs: 0 }, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'work', + type: 'script' as any, + label: 'Work', + config: opts.config, + inputSchema: opts.inputSchema, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'work' }, + { id: 'e1', source: 'work', target: 'end' }, + ], + } as any); + + return { engine, runs }; +} + +describe("#9889 — input-schema refusal holds on every attempt under strategy: 'retry'", () => { + it('never executes a node whose config mis-types its declared inputSchema — on ANY attempt', async () => { + const { engine, runs } = countingFlowEngine({ + config: { count: 'not_a_number' }, + inputSchema: { count: { type: 'number', required: true } }, + }); + + const result = await engine.execute('guarded'); + + // The refusal, as the caller sees it (see header for why no `code`). + expect(result.success).toBe(false); + expect(result.status).toBe('failed'); + expect(result.error).toContain("expected type 'number' but got 'string'"); + + // The point of the card: the side-effecting node ran ZERO times. + // Pre-repair this was 2 — refused on attempt 1, executed for real on + // attempts 2 and 3. + expect(runs.count).toBe(0); + + // And the refusal happened PER ATTEMPT, not by short-circuiting the + // retry loop: every attempt still dispatched and consumed budget + // (retry accounting unchanged — the non-retryable classification is + // the open question, not this repair), so the run log holds one + // failed row per attempt (1 initial + maxRetries), each carrying the + // guard's own message. + const attemptRows = await engine.listRuns('guarded', { status: 'failed' }); + expect(attemptRows).toHaveLength(3); + for (const row of attemptRows) { + expect(row.error).toContain("expected type 'number' but got 'string'"); + } + }); + + it('never executes a node missing a required declared input — on ANY attempt', async () => { + const { engine, runs } = countingFlowEngine({ + config: {}, + inputSchema: { url: { type: 'string', required: true } }, + }); + + const result = await engine.execute('guarded'); + + expect(result.success).toBe(false); + expect(result.status).toBe('failed'); + expect(result.error).toContain("missing required input parameter 'url'"); + expect(runs.count).toBe(0); + }); + + it('still retries a VALID flow normally — the guard refuses nothing attempt 1 allowed', async () => { + const { engine, runs } = countingFlowEngine({ + config: { count: 42 }, + inputSchema: { count: { type: 'number', required: true } }, + // Attempt 1 fails downstream (a transient error, the case retry + // exists for); attempt 2 succeeds. + executeResult: attempt => + attempt === 1 ? { success: false, error: 'downstream 503' } : { success: true }, + }); + + const result = await engine.execute('guarded'); + + expect(result.success).toBe(true); + // Attempt 1 ran and failed, attempt 2 ran and succeeded — the fix must + // not turn a legitimate retry into a refusal. + expect(runs.count).toBe(2); + }); +});