diff --git a/.changeset/subflow-child-refusal-propagation.md b/.changeset/subflow-child-refusal-propagation.md new file mode 100644 index 0000000000..247a4f771f --- /dev/null +++ b/.changeset/subflow-child-refusal-propagation.md @@ -0,0 +1,9 @@ +--- +'@objectstack/service-automation': patch +--- + +Answer a delegated subflow child's retryable refusal as a refusal, not as a terminal child failure. + +A run paused at a `subflow` node forwards a resume down to the child it is parked on — the screen-flow path, where the caller holds one stable run id (the parent's) and posts every wizard step to it. When the child *refused* that bag (`INVALID_SCREEN_INPUT` for a missing `required` field, `INVALID_SIGNAL`, `RESUME_IN_PROGRESS`, `STORE_UNAVAILABLE`), the delegation read it as a child that ran and died: it failed the parent run, consumed the parent's suspension, orphaned the still-paused child, and answered a **code-less** envelope, which a transport maps to `400 FLOW_FAILED`. The corrected retry on the same run id then answered `RUN_NOT_FOUND` — one mistyped form field destroyed a running workflow. + +The delegation now branches on the child's own `code`. A refusal is returned verbatim with its `code` intact (and the parent's `durationMs`), **both** pauses left live, so the corrected submission still lands on the same parent run id. A child that genuinely ran and failed still fails the parent exactly as before. diff --git a/packages/services/service-automation/src/builtin/subflow-child-refusal.test.ts b/packages/services/service-automation/src/builtin/subflow-child-refusal.test.ts new file mode 100644 index 0000000000..a6278fd9c0 --- /dev/null +++ b/packages/services/service-automation/src/builtin/subflow-child-refusal.test.ts @@ -0,0 +1,276 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A subflow parent's resume must answer a delegated child's RETRYABLE REFUSAL + * as a refusal — not as a terminal child failure (#14379). + * + * The screen-flow path gives the caller ONE stable run id: the parent's. Every + * wizard step is posted to it, and `resumeInternal`'s subflow delegation block + * forwards the bag down to the child the parent is parked on. When the child + * REFUSES that bag — `INVALID_SCREEN_INPUT` for a missing `required` field, + * `INVALID_SIGNAL` for a reserved variable name — nothing ran and the child's + * pause is deliberately still live (#4477: "a rejected bag leaves the pause + * live and the legitimate submission still lands"). + * + * The delegation block used to read every `!childRes.success` as a child that + * RAN and DIED: it called `failSuspendedRun` on the parent and answered a + * CODE-LESS `{ success: false, error }`. One mistyped form field therefore + * destroyed the run — the parent's suspension consumed and a failure recorded, + * the still-paused child orphaned with nothing to bubble into, the caller told + * `400 FLOW_FAILED` ("it ran and was rejected") for something that never ran, + * and their corrected retry on the same run id answered `RUN_NOT_FOUND`. + * + * The discriminator is PRODUCER-FIRST (triage ruling, 2026-09-02): the child's + * own `code` being one of the refusal codes the engine itself answers — ⛔ not + * "is the child's suspension still live", which is a second store read whose + * answer can race and which infers intent from state. `failSuspendedRun` is + * reserved for a child that genuinely ran and failed, which the last test here + * is the negative control for. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import type { NodeExecutor } from '../engine.js'; +import { installBuiltinNodes } from './index.js'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +function silentLogger() { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; +} +function ctx() { + return { logger: silentLogger(), getService() { return undefined; } } as any; +} + +/** + * `resumeAuthority: 'any'` on the fixture pausers: the resume gate (#5561) + * follows the linked-run chain to the CHILD's node, so the type the child + * parks on is what a resume of the parent is judged against. These tests are + * about delegation mechanics, not the gate (`resume-authority-gate.test.ts` + * owns that), so the fixtures state the posture they rely on. + */ +const openPauser = (type: string) => defineActionDescriptor({ + type, version: '1.0.0', name: type, + supportsPause: true, resumeAuthority: 'any', +}); + +/** The child's screen declares exactly one unconditional required field. */ +const REQUIRED_KIND = [{ name: 'kind', label: 'Kind', type: 'text', required: true }]; + +/** A child flow that parks on a real `screen` node and exports what it collected. */ +const screenChild = (name: string, tail: Array> = []) => ({ + name, + label: name, + type: 'screen', + status: 'active', + version: 1, + variables: [{ name: 'kind', type: 'text', isOutput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'ask', type: 'screen', label: 'Ask', config: { fields: REQUIRED_KIND } }, + ...tail, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'ask', type: 'default' }, + ...tail.map((n, i) => ({ + id: `t${i}`, + source: i === 0 ? 'ask' : (tail[i - 1] as { id: string }).id, + target: (n as { id: string }).id, + type: 'default', + })), + { + id: 'e2', + source: tail.length ? (tail[tail.length - 1] as { id: string }).id : 'ask', + target: 'end', + type: 'default', + }, + ], +}); + +/** A child flow that parks on a pause declaring NO screen contract. */ +const openChild = (name: string) => ({ + name, + label: name, + type: 'autolaunched', + status: 'active', + version: 1, + variables: [{ name: 'kind', type: 'text', isOutput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'hold', type: 'openpauser', label: 'Hold' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'hold', type: 'default' }, + { id: 'e2', source: 'hold', target: 'end', type: 'default' }, + ], +}); + +/** The parent: start → subflow(child) → recorder → end. */ +const parentFlow = (childName: string) => ({ + name: 'parent_flow', + label: 'Parent Flow', + type: 'autolaunched', + status: 'active', + version: 1, + nodes: [ + { id: 'ps', type: 'start', label: 'Start' }, + { id: 'call', type: 'subflow', label: 'Call Child', config: { flowName: childName, outputVariable: 'childOut' } }, + { id: 'rec', type: 'recorder', label: 'Record' }, + { id: 'pe', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'p1', source: 'ps', target: 'call', type: 'default' }, + { id: 'p2', source: 'call', target: 'rec', type: 'default' }, + { id: 'p3', source: 'rec', target: 'pe', type: 'default' }, + ], +}); + +describe('subflow delegation: a child REFUSAL is answered as a refusal (#14379)', () => { + let engine: AutomationEngine; + let captured: unknown[]; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + captured = []; + // Downstream of the parent's subflow node: proves the parent really + // continued and what the child's output mapped to. + engine.registerNodeExecutor({ + type: 'recorder', + async execute(_node, variables) { + captured.push(variables.get('childOut')); + return { success: true }; + }, + } as NodeExecutor); + // A pause that declares no screen contract (for the INVALID_SIGNAL arm). + engine.registerNodeExecutor({ + type: 'openpauser', + descriptor: openPauser('openpauser'), + async execute() { return { success: true, suspend: true }; }, + } as NodeExecutor); + // Terminal child failure, downstream of the child's screen. + engine.registerNodeExecutor({ + type: 'boomer', + async execute() { throw new Error('boom in the child'); }, + } as NodeExecutor); + }); + + /** Start the parent and return `[parentRunId, childRunId]`. */ + async function startPair(): Promise<[string, string]> { + const started = await engine.execute('parent_flow', {} as any); + expect(started.status).toBe('paused'); + const parentRunId = started.runId!; + const child = engine.listSuspendedRuns().find((r) => r.runId !== parentRunId)!; + expect(child).toBeDefined(); + return [parentRunId, child.runId]; + } + + describe('INVALID_SCREEN_INPUT — the child screen refuses the bag', () => { + beforeEach(() => { + engine.registerFlow('child_flow', screenChild('child_flow') as any); + engine.registerFlow('parent_flow', parentFlow('child_flow') as any); + }); + + it('answers the child refusal with its code and leaves BOTH pauses intact', async () => { + const [parentRunId, childRunId] = await startPair(); + + const res = await engine.resume(parentRunId, { variables: {} }); + + // ADR-0112 envelope: the code the child produced, propagated intact. + // A code-less envelope is what made the transport answer + // `400 FLOW_FAILED` for something that never ran. + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + // The child's own actionable text, not "subflow run '…' failed:". + expect(res.error).toMatch(/^Invalid screen input: /); + expect(res.error).toContain('"kind"'); + expect(res.error).toMatch(/required/i); + // Nothing was consumed on either level. + expect(await engine.hasSuspendedRun(parentRunId)).toBe(true); + expect(await engine.hasSuspendedRun(childRunId)).toBe(true); + // The parent still surfaces the child's screen, unchanged. + expect((await engine.getSuspendedScreen(parentRunId))?.nodeId).toBe('ask'); + expect(captured).toEqual([]); // the parent did NOT continue + }); + + it('completes the corrected retry on the SAME parent run id', async () => { + const [parentRunId, childRunId] = await startPair(); + expect((await engine.resume(parentRunId, { variables: {} })).code).toBe('INVALID_SCREEN_INPUT'); + + const good = await engine.resume(parentRunId, { variables: { kind: 'normal' } }); + + expect(good.success).toBe(true); + expect(good.code).toBeUndefined(); + expect(good.status).toBeUndefined(); // ran to completion + expect(captured).toEqual([{ kind: 'normal' }]); // child output mapped into the parent + expect(await engine.hasSuspendedRun(parentRunId)).toBe(false); + expect(await engine.hasSuspendedRun(childRunId)).toBe(false); + }); + + it('refuses the signal-less gesture the same way, both pauses intact', async () => { + // #13648 normalises an absent signal to `{}` at the public door, so + // `resume(parentRunId)` lands on this same delegation path. + const [parentRunId, childRunId] = await startPair(); + + const bare = await engine.resume(parentRunId); + + expect(bare.success).toBe(false); + expect(bare.code).toBe('INVALID_SCREEN_INPUT'); + expect(await engine.hasSuspendedRun(parentRunId)).toBe(true); + expect(await engine.hasSuspendedRun(childRunId)).toBe(true); + // And the corrected retry still lands. + const good = await engine.resume(parentRunId, { variables: { kind: 'late' } }); + expect(good.success).toBe(true); + expect(captured).toEqual([{ kind: 'late' }]); + }); + }); + + describe('INVALID_SIGNAL — a second refusal code on the same path', () => { + beforeEach(() => { + engine.registerFlow('child_flow', openChild('child_flow') as any); + engine.registerFlow('parent_flow', parentFlow('child_flow') as any); + }); + + it('answers the child refusal with its code and leaves BOTH pauses intact', async () => { + const [parentRunId, childRunId] = await startPair(); + + const res = await engine.resume(parentRunId, { variables: { $sneaky: 1 } }); + + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SIGNAL'); + expect(res.error).toMatch(/engine-internal variables/); + expect(await engine.hasSuspendedRun(parentRunId)).toBe(true); + expect(await engine.hasSuspendedRun(childRunId)).toBe(true); + expect(captured).toEqual([]); + + // The legitimate submission still lands on the same parent run id. + const good = await engine.resume(parentRunId, { variables: { kind: 'ok' } }); + expect(good.success).toBe(true); + expect(captured).toEqual([{ kind: 'ok' }]); + }); + }); + + describe('negative control — a child that genuinely RAN and FAILED', () => { + beforeEach(() => { + engine.registerFlow('child_flow', screenChild('child_flow', [{ id: 'boom', type: 'boomer', label: 'Boom' }]) as any); + engine.registerFlow('parent_flow', parentFlow('child_flow') as any); + }); + + it('still fails the parent terminally, with the envelope shape unchanged', async () => { + const [parentRunId, childRunId] = await startPair(); + + // The screen ACCEPTS this bag; the node after it throws. + const res = await engine.resume(parentRunId, { variables: { kind: 'normal' } }); + + expect(res.success).toBe(false); + expect(res.code).toBeUndefined(); // a terminal child failure carries none — unchanged + expect(res.error).toMatch(/^subflow run '.*' \(child_flow\) failed: /); + expect(res.error).toContain('boom in the child'); + // Both suspensions are consumed: the parent was failed, the child ran. + expect(await engine.hasSuspendedRun(parentRunId)).toBe(false); + expect(await engine.hasSuspendedRun(childRunId)).toBe(false); + expect(captured).toEqual([]); + }); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index fd009382c2..111b718544 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -961,6 +961,38 @@ function isSuspendSignal(err: unknown): err is FlowSuspendSignal { */ class InputSchemaViolationError extends Error {} +/** + * The refusal codes {@link AutomationEngine.resumeInternal} answers for a + * resume that NEVER RAN — the run is untouched, nothing executed, and the + * identical call succeeds once its cause is corrected or has passed (#14379). + * + * Read by the subflow delegation path, which forwards a parent's resume down + * to the child the parent is parked on: a child answering one of these has + * REFUSED, not failed, so the parent must answer the refusal rather than + * record a failure. The set is the PRODUCER's own vocabulary — the codes this + * file returns from that one method — which is why it is a closed list here + * and not a predicate over state (triage ruling 2026-09-02: branch on the + * child's `code`, ⛔ never on "is the child's suspension still live", a second + * store read whose answer can race and which infers intent from state). + * + * ⛔ `RUN_NOT_FOUND` is deliberately absent, though `resumeInternal` returns it + * too: it is the engine's terminal "this pause is gone for good" class (#8684) + * — no suspension, an unregistered flow, or a node edited away underneath a + * parked run — which a transport answers **404** and which no retry can fix. + * A child in that state can never continue, so its parent cannot either. + */ +const RETRYABLE_RESUME_REFUSAL_CODES: ReadonlySet = new Set([ + 'INVALID_SCREEN_INPUT', + 'INVALID_SIGNAL', + 'RESUME_IN_PROGRESS', + 'STORE_UNAVAILABLE', +] satisfies ReadonlyArray>); + +/** Whether an {@link AutomationResult} code names a resume that never ran. */ +function isRetryableResumeRefusal(code: AutomationResult['code']): boolean { + return code !== undefined && RETRYABLE_RESUME_REFUSAL_CODES.has(code); +} + /** * Marks a {@link ResumeSignal} the ENGINE built for its own continuations — * the subflow output mapping and the `map` item handoff. Module-private and @@ -4817,6 +4849,39 @@ export class AutomationEngine implements IAutomationService { screen: childRes.screen, }; } + // [#14379] A child REFUSAL is not a child failure. The + // codes in {@link RETRYABLE_RESUME_REFUSAL_CODES} are this + // method's own answers for a resume that never ran: the + // child's screen contract is checked BEFORE + // `forgetSuspendedRun` precisely so "a rejected bag leaves + // the pause live and the legitimate submission still lands" + // (#4477), so the child is parked exactly where it was. + // + // Reading those as a terminal failure consumed the PARENT's + // pause over a mistyped form field — and the screen-flow + // path is where a caller holds ONE stable run id, the + // parent's, and posts every wizard step to it. The run was + // gone, the still-paused child orphaned with nothing left + // to bubble into, the caller told `400 FLOW_FAILED` ("it + // ran and was rejected") for something that never ran, and + // their corrected retry on that same id answered + // `RUN_NOT_FOUND`. + // + // The child's envelope is answered VERBATIM but for the + // parent's `durationMs`. Propagating the `code` is half the + // fix: a code-less envelope is exactly what forced the + // transport onto `400 FLOW_FAILED`, and leaving both pauses + // alive while still answering one repairs the state and + // leaves the caller equally misled. The child's `error` is + // the actionable half — `Screen field "kind" is required` — + // where the failure text below names neither the problem + // nor anything a caller can act on. Nothing else moves: + // neither pause is consumed, and the parent's surfaced + // screen needs no refresh because the child did not + // advance. + if (!childRes.success && isRetryableResumeRefusal(childRes.code)) { + return { ...childRes, durationMs: Date.now() - run.startTime }; + } if (!childRes.success) { const error = `subflow run '${childRunId}' (${childRun.flowName}) failed: ${childRes.error ?? 'unknown error'}`; await this.failSuspendedRun(run, error);