diff --git a/.changeset/18110-subflow-map-refused-rollup.md b/.changeset/18110-subflow-map-refused-rollup.md new file mode 100644 index 00000000000..03fbde67e8f --- /dev/null +++ b/.changeset/18110-subflow-map-refused-rollup.md @@ -0,0 +1,19 @@ +--- +"@objectstack/service-automation": minor +--- + +fix(service-automation): on the synchronous path, a child run that REFUSES stops its parent, in `subflow` and in `map` alike (#18110, #18555) + +**Clause-②: yes (widening)** — `NodeExecutionResult` is barrel-exported from this package's single entry point, and it gains two new optional members. Nothing previously accepted is refused and nothing is retired, so this is a widening of the published executor contract, not a narrowing. Contract-review tier. + +A child flow that runs to completion in one go and ends on an `end` node declaring `outcome: 'refused'` used to roll up to its parent as an ordinary success. `subflow-node.ts` branched only on `child.status === 'paused'` and `!child.success`; a refused child is neither (`{ success: true, status: 'refused' }` — *a refusal is a successful evaluation that says no*), so it fell through the success exit. The parent walked the node's out-edges, recorded `completed` and fired its **own** `successMessage` over the child's refusal — the author got the exact opposite of what they wrote, fail-open. `map-node.ts` had the identical branch set and the identical hole: a refusing row let every row after it through. + +- **New on `NodeExecutionResult`: `refuse?: boolean` and `refusalMessage?: string`.** The executor-facing half of the unwinding protocol `suspend?: boolean` already uses. A node that sets `refuse` terminates its run as `refused` — a terminal status this package has published since #15788, so **no new status value** and nothing authorable changes. +- **`subflow` and `map` both set it** when their child run returns `status: 'refused'`. One channel, two call sites. +- **The child's `selected` / `acted` / `unmeasuredEffect` rollup (#4354) survives the refusal**, because the engine throws the refusal signal from the same position it throws the suspend signal: after the node's success step is pushed, after its `childSteps` are folded and after its output is written back. A child that refused really can have written rows before it said no. +- ⛔ **A refusal is still not a failure.** It does not consume retry budget, is not routable by a `fault` edge, and is not counted in `nodes[].failures`. +- **Region-boundary diagnostic, text only**: the message a structured region raises when a refusal tries to cross it now names whichever node carried the refusal, instead of asserting it was an `end` node — which, for a refusing `subflow`/`map` inside a region, sent the author looking for a node that was not in their region. Region **semantics** are unchanged. + +**Scope — the RESUMED leg is not covered.** This fixes the path where the child run finishes inside the parent's own `engine.execute` call and its outcome is read from that return value. A child that durably PAUSES first — a nested `approval` / `screen` / `wait` — and only refuses when it is later resumed still reaches its parent through the resume machinery, which reads the child's outcome at different seams and does not consult `status: 'refused'` at any of them. Both of those seams pre-date this change and neither is a regression of it, but neither is closed by it either, and the resumed leg is the one a screen flow actually takes. A follow-up card covers it: #18714. + +For third-party node executors this is additive: an executor that never sets `refuse` behaves exactly as before. diff --git a/packages/services/service-automation/src/builtin/map-node.ts b/packages/services/service-automation/src/builtin/map-node.ts index 6ec4ee9326d..25be25302a9 100644 --- a/packages/services/service-automation/src/builtin/map-node.ts +++ b/packages/services/service-automation/src/builtin/map-node.ts @@ -226,6 +226,49 @@ export function registerMapNode(engine: AutomationEngine, ctx: PluginContext): v }, }; } + if (child.status === 'refused') { + // [#18555] This item's child REFUSED — an `end` inside it declared + // `outcome: 'refused'`, a successful evaluation that says NO. The + // identical hole #18110 describes for `subflow`, in this second file: + // until this arm existed a refusing item fell through the synchronous + // completion below, its output was pushed into `state.results`, the + // batch carried on to the NEXT item, and the parent recorded + // `completed` and fired its own `successMessage`. For a `map` that is + // the worked "approve each row" shape answering *no* on row 3 and + // approving rows 4..n anyway — fail-open, and finishing green. + // + // ⛔ Not the failure arm above: a refusal is not a failure (⛔ no + // retry budget, ⛔ no `fault` routing, ⛔ not counted in + // `nodes[].failures`), and ⛔ not `state.results`: this item did not + // produce a result, it declined. The engine throws the refusal signal + // only after this step and these `metrics` are already in the run + // log, so the batch's #4354 totals survive the refusal. + // + // The totals are the COMPLETION rule, ⛔ not the failure one: the + // items that already ran keep theirs, and the refusing item's own + // `selected` / `acted` / `unmeasured` / `failed` roll up too, because + // a child that refused did not fail — nothing counts its `failed` + // a second time through `nodes[].failures` the way a failed child's + // would be. A refusing child really can have written rows before it + // said no. + // + // ⛔ The progress state is not advanced and not deleted: the run is + // terminating, nothing resumes it, and `started` is the resume + // program counter — moving it would claim this item completed. + return { + success: true, + refuse: true, + refusalMessage: child.refusalMessage, + metrics: { + selected: selected + (child.summary?.selected ?? 0), + acted: acted + (child.summary?.acted ?? 0), + ...(unmeasured || child.summary?.unmeasured ? { unmeasuredEffect: true } : {}), + ...(child.summary?.failed !== undefined + ? { failures: failures + child.summary.failed } + : rolledFailures()), + }, + }; + } // Synchronous completion — record and advance. state.started = idx + 1; state.results.push(child.output ?? null); diff --git a/packages/services/service-automation/src/builtin/map-refused-rollup.test.ts b/packages/services/service-automation/src/builtin/map-refused-rollup.test.ts new file mode 100644 index 00000000000..6a75c28a681 --- /dev/null +++ b/packages/services/service-automation/src/builtin/map-refused-rollup.test.ts @@ -0,0 +1,228 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #18555 — a refusing child inside a `map` unit must STOP the parent. + * + * The identical hole #18110 describes for `subflow`, in a second file: + * `map-node.ts` branched on `child.status === 'paused'` and `!child.success` + * and had no `refused` arm, so a refused child (`{ success: true, status: + * 'refused' }`) fell through the synchronous-completion path — its output was + * pushed into `state.results`, the batch carried on to the NEXT item, and the + * parent recorded `completed` and fired its own `successMessage`. + * + * ⭐ For a `map` that is the worked "approve each row" shape answering *no* on + * one row and approving every row after it anyway. Fail-open, and finishing + * green. + * + * ⚠️ Pinned SEPARATELY from the `subflow` arm on purpose: one channel + * (`NodeExecutionResult.refuse`), two call sites, and deleting either site must + * fail a test by itself. `subflow-refused-rollup.test.ts` is the other half — + * neither file's green covers the other's arm. + * + * ⚠️ Direction, predicted before running. "The defect" tests FAIL against the + * unfixed executor; the CONTROL is green on both sides on purpose. + * + * ⚠️ `refused` here is the run OUTCOME — *a refusal is a successful evaluation + * that says no* — ⛔ NOT this package's GUARD refusal, which is a FAILURE. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import type { NodeExecutor } from '../engine.js'; +import { registerMapNode } from './map-node.js'; + +function silentLogger(): any { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } }; +} +function pluginCtx(): any { + return { logger: silentLogger(), getService() { return undefined; } }; +} + +/** The parent's own completion toast — it must never ride an item's refusal. */ +const PARENT_TOAST = 'Batch complete!'; +/** + * The authored refusal template. `{val}` is the child's own declared INPUT + * variable (the mapped item, handed down as `params.val`) — what makes the + * rendered reason per-item. ⛔ Not the parent's `{item}` iterator: that lives in + * the PARENT's variable map and resolves to the empty string down here. + */ +const REFUSAL_TEMPLATE = 'Refused: {val} is not eligible'; +/** Per-item #4354 work, so the rollup on the refusal path is assertable. */ +const ITEM_METRICS = { selected: 2, acted: 1 } as const; + +/** + * Child flow: judge the item, then leave by one of TWO ends — a plain + * completion, or an `end` declaring `outcome: 'refused'`. Routed by the + * judge's `branchLabel`, so WHICH item refuses is a property of the data. + */ +function childFlow() { + return { + name: 'per_item', label: 'Per item', type: 'autolaunched', + variables: [{ name: 'val', type: 'text', isInput: true }], + nodes: [ + { id: 'c_start', type: 'start', label: 'Start' }, + { id: 'c_judge', type: 'judge', label: 'Judge' }, + { id: 'c_ok', type: 'end', label: 'Ok' }, + { id: 'c_no', type: 'end', label: 'No', config: { outcome: 'refused', message: REFUSAL_TEMPLATE } }, + ], + edges: [ + { id: 'ce0', source: 'c_start', target: 'c_judge' }, + { id: 'ce1', source: 'c_judge', target: 'c_ok', label: 'allow' }, + { id: 'ce2', source: 'c_judge', target: 'c_no', label: 'deny' }, + ], + }; +} + +/** start -> map(per_item over {items}) -> recorder -> end, with the parent toast. */ +function parentFlow() { + return { + name: 'batch', label: 'batch', type: 'autolaunched', + successMessage: PARENT_TOAST, + variables: [{ name: 'items', type: 'list', isInput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'each', type: 'map', label: 'For each', + config: { + flowName: 'per_item', collection: '{items}', + iteratorVariable: 'item', input: { val: '{item}' }, outputVariable: 'mapped', + }, + }, + { id: 'after', type: 'downstream', label: 'After' }, + { id: 'fin', type: 'end', label: 'Fin' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'each' }, + { id: 'e1', source: 'each', target: 'after' }, + { id: 'e2', source: 'after', target: 'fin' }, + ], + }; +} + +describe('#18555 — a refusing child inside a `map` stops the parent', () => { + let engine: AutomationEngine; + let judged: string[]; + let ran: string[]; + /** The item value the judge refuses. `null` = refuse nothing (the control). */ + let deny: string | null; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + judged = []; + ran = []; + deny = 'b'; + registerMapNode(engine, pluginCtx()); + + engine.registerNodeExecutor({ + type: 'judge', + async execute(_node, _variables, context) { + const val = String((context as any)?.params?.val); + judged.push(val); + return { + success: true, + branchLabel: deny !== null && val === deny ? 'deny' : 'allow', + metrics: { ...ITEM_METRICS }, + }; + }, + } as NodeExecutor); + // The node AFTER the map. Its presence in `ran` IS the "the parent walked + // on" assertion — ⛔ not a proxy for it. + engine.registerNodeExecutor({ + type: 'downstream', + async execute() { + ran.push('downstream'); + return { success: true }; + }, + } as NodeExecutor); + + engine.registerFlow('per_item', childFlow() as never); + engine.registerFlow('batch', parentFlow() as never); + }); + + const runBatch = () => engine.execute('batch', { params: { items: ['a', 'b', 'c'] } } as never); + + async function newestRun(flowName: string) { + const runs = await engine.listRuns(flowName, { limit: 5 }); + expect(runs.length).toBeGreaterThan(0); + return engine.getRun(runs[0]!.id); + } + + describe('the defect', () => { + it('the parent run REFUSES — it does not record `completed`', async () => { + const result = await runBatch(); + + expect(result.success).toBe(true); // a refusal is a successful evaluation + expect(result.status).toBe('refused'); + expect((await newestRun('batch'))?.status).toBe('refused'); + }); + + it("the refusing item's rendered reason reaches the parent's caller", async () => { + const result = await runBatch(); + + // Interpolated in the CHILD against the child's own variables (the + // iterator value), passed through — ⛔ not re-rendered by the map node. + expect(result.refusalMessage).toBe('Refused: b is not eligible'); + expect((await newestRun('batch'))?.refusalMessage).toBe('Refused: b is not eligible'); + }); + + it("the parent's own `successMessage` stays silent over the refusal", async () => { + const result = await runBatch(); + + expect(result.successMessage).toBeUndefined(); + }); + + it('the batch STOPS at the refusing item — later items never run', async () => { + // ⭐ The `map`-specific half of the defect: "approve each row" answering + // no on row 2 and approving row 3 anyway. + await runBatch(); + + expect(judged).toEqual(['a', 'b']); + expect(judged).not.toContain('c'); + }); + + it('downstream nodes do NOT run — the map node is not walked past', async () => { + await runBatch(); + + expect(ran).toEqual([]); + }); + + it("preserves the batch's #4354 rollup on the refusal path (`selected` / `acted`)", async () => { + // ⭐ The property the unwinding POSITION exists for: the signal is thrown + // after this node's success step and its metrics are already in the run + // log. Items that already ran really did write rows, and so did the + // refusing item before it said no — both count. Two items x {2,1}. + const result = await runBatch(); + + // ⛔ The refusal assertion belongs IN this test, not next door: without it + // the totals below are equally true of the unfixed engine, which rolled + // the same metrics up and then carried on to the next item. + expect(result.status).toBe('refused'); + expect(result.summary).toBeDefined(); + expect(result.summary).toMatchObject({ selected: 4, acted: 2 }); + expect(result.summary!.nodes.find((n) => n.nodeId === 'each')).toMatchObject({ + selected: 4, acted: 2, + }); + }); + }); + + describe('the control — an ordinary batch still rolls up as an ordinary success', () => { + // ⛔ Mandatory, not decoration: every assertion above is also satisfied by + // a map that had started refusing EVERY batch. + it('a batch with no refusing item completes, fires the toast, and walks on', async () => { + deny = null; + + const result = await runBatch(); + + expect(result.success).toBe(true); + expect(result.status).toBeUndefined(); // the terminal-success exit stamps none + expect(result.refusalMessage).toBeUndefined(); + expect(result.successMessage).toBe(PARENT_TOAST); + expect(judged).toEqual(['a', 'b', 'c']); // every item ran + expect(ran).toEqual(['downstream']); + expect((await newestRun('batch'))?.status).toBe('completed'); + // Three items this time — the refusal arm added a branch, ⛔ it did not + // move the totals. + expect(result.summary).toMatchObject({ selected: 6, acted: 3 }); + }); + }); +}); diff --git a/packages/services/service-automation/src/builtin/subflow-node.ts b/packages/services/service-automation/src/builtin/subflow-node.ts index ea77e607424..32711d64c2e 100644 --- a/packages/services/service-automation/src/builtin/subflow-node.ts +++ b/packages/services/service-automation/src/builtin/subflow-node.ts @@ -171,9 +171,10 @@ export function registerSubflowNode(engine: AutomationEngine, ctx: PluginContext // Deliberately the SAME exit the three totals above already leave by, so // this adds one total to an existing rollup and decides nothing new // about which child outcomes reach it. (A `refused` child — the run - // OUTCOME sense, an `end` node saying no — reaches this exit today and - // has since refusals existed; whether a parent should go on from one at - // all is a separate open question about this node, not this slot's.) + // OUTCOME sense, an `end` node saying no — used to reach this exit as an + // ordinary success; #18110 answered the open question this comment left + // and gives it the arm below, which leaves by this same exit with the + // same totals.) // // Absent, never zero: a child summary with no `failed` is a row recorded // before the count existed, and `0` would claim it was measured. @@ -185,7 +186,43 @@ export function registerSubflowNode(engine: AutomationEngine, ctx: PluginContext // directly to the parent variable map). if (outVar) variables.set(outVar, child.output ?? null); - return { success: true, output: { output: child.output ?? null }, ...(metrics ? { metrics } : {}) }; + const rollup = { + success: true as const, + output: { output: child.output ?? null }, + ...(metrics ? { metrics } : {}), + }; + + // [#18110] The child REFUSED — an `end` inside it declared + // `outcome: 'refused'`, which is a successful evaluation that says NO. + // + // Until this arm existed every child status other than `paused` fell + // through the success exit below, so the parent walked straight down this + // node's out-edges, recorded `completed` and fired its OWN + // `successMessage` over the child's refusal: the author got the opposite + // of what they wrote, and fail-open — a refusing gate (approval, + // eligibility, a precondition) that lets the run through is the one kind + // of wrong nobody notices, because the flow finishes green. + // + // ⛔ NOT folded into the failure arm above. A refusal is not a failure: + // it must not consume retry budget, must not be routable by a `fault` + // edge, and must not be counted in `nodes[].failures` — all of which it + // would inherit from `success: false`. `refuse` is the channel for + // exactly this (`NodeExecutionResult.refuse`), and the engine throws its + // signal only after this step and these `metrics` are already in the run + // log — which is why the child's #4354 totals survive on the refusal path + // instead of being lost to the unwind. + // + // The envelope is the success one PLUS the refusal, deliberately: the + // child's output really was produced and the nodes before its refusing + // `end` really ran, so withholding them here would make the parent's + // answer depend on how the child ended rather than on what it did — + // the same reasoning `finishRefusedRun` applies to a refused run's own + // declared outputs. + if (child.status === 'refused') { + return { ...rollup, refuse: true, refusalMessage: child.refusalMessage }; + } + + return rollup; }, }); diff --git a/packages/services/service-automation/src/builtin/subflow-refused-rollup.test.ts b/packages/services/service-automation/src/builtin/subflow-refused-rollup.test.ts new file mode 100644 index 00000000000..ce390aafd4e --- /dev/null +++ b/packages/services/service-automation/src/builtin/subflow-refused-rollup.test.ts @@ -0,0 +1,216 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #18110 — a `subflow` child that ends `refused` must STOP the parent. + * + * `subflow-node.ts` branched only on `child.status === 'paused'` and + * `!child.success`. A refused child is neither: `finishRefusedRun` answers + * `{ success: true, status: 'refused' }` — *a refusal is a successful + * evaluation that says no* — so it fell through the ordinary success exit. The + * parent walked this node's out-edges, recorded `completed` and fired its OWN + * `successMessage` over the child's refusal: the author got the exact opposite + * of what they wrote, and got it FAIL-OPEN. A refusing gate (an approval, an + * eligibility check, a precondition) that lets the run through is the one kind + * of wrong nobody notices, because the flow finishes green. + * + * ⚠️ Direction, predicted before running. The tests under "the defect" FAIL + * against the unfixed executor — `undefined` where `'refused'` is expected, + * the parent's toast where silence is expected, and the downstream node in + * `ran`. The CONTROL is green on both sides on purpose: a green that only + * proved the refusal path would pass equally over a broken ordinary path. + * + * ⚠️ `refused` here is the run OUTCOME, ⛔ NOT this package's other `refused`. + * A GUARD refusal (`guard-refusal.ts`, `refuseNode`, the resume-authority gate) + * means "the engine declined to execute" and is a kind of FAILURE; the two + * senses are distinguished at `engine.ts`'s `FlowRefusalSignal` docblock. + * `subflow-child-refusal.test.ts` in this directory is about a THIRD thing + * again (#14379's retryable resume-bag codes) — same word, different subject. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import type { NodeExecutor } from '../engine.js'; +import { registerSubflowNode } from './subflow-node.js'; +import type { AutomationContext } from '@objectstack/spec/contracts'; + +function silentLogger(): any { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } }; +} +function pluginCtx(): any { + return { logger: silentLogger(), getService() { return undefined; } }; +} + +/** The parent's own completion toast — it must never ride a child's refusal. */ +const PARENT_TOAST = 'Parent completed!'; +/** The authored refusal template. `{record.name}` is what makes it per-record. */ +const REFUSAL_TEMPLATE = 'Refused: {record.name} is a confirmed duplicate'; +const ACME = { id: 'rec_1', name: 'Acme Corp' } as const; + +/** + * The child's pre-refusal work, reported as #4354 metrics. Its whole job is to + * make the rollup assertion possible: a child that refuses really can have + * written rows before it said no, and those counts must survive the refusal + * unwind — the property option B was rejected for losing. + */ +const CHILD_METRICS = { selected: 3, acted: 2, unmeasuredEffect: true } as const; + +function triggerCtx(): AutomationContext { + return { event: 'manual', object: 'account', record: { ...ACME } } as unknown as AutomationContext; +} + +/** A child flow whose `end` carries `endConfig` (absent = a plain completion). */ +function childFlow(name: string, endConfig?: Record) { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'c_start', type: 'start', label: 'Start' }, + { id: 'c_work', type: 'childwork', label: 'Work' }, + { id: 'c_end', type: 'end', label: 'End', ...(endConfig ? { config: endConfig } : {}) }, + ], + edges: [ + { id: 'ce0', source: 'c_start', target: 'c_work' }, + { id: 'ce1', source: 'c_work', target: 'c_end' }, + ], + }; +} + +/** start -> subflow(child) -> recorder -> end, with the parent's own toast. */ +function parentFlow(childName: string) { + return { + name: 'parent', label: 'parent', type: 'autolaunched', + successMessage: PARENT_TOAST, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'sub', type: 'subflow', label: 'Sub', config: { flowName: childName, outputVariable: 'subResult' } }, + { id: 'after', type: 'downstream', label: 'After' }, + { id: 'fin', type: 'end', label: 'Fin' }, + ], + edges: [ + { id: 'e0', source: 'start', target: 'sub' }, + { id: 'e1', source: 'sub', target: 'after' }, + { id: 'e2', source: 'after', target: 'fin' }, + ], + }; +} + +describe('#18110 — a refusing `subflow` child stops the parent', () => { + let engine: AutomationEngine; + let ran: string[]; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + ran = []; + registerSubflowNode(engine, pluginCtx()); + + engine.registerNodeExecutor({ + type: 'childwork', + async execute() { + ran.push('child-work'); + return { success: true, metrics: { ...CHILD_METRICS } }; + }, + } as NodeExecutor); + // The node AFTER the subflow. Its presence in `ran` is the whole + // "the parent walked on" assertion — ⛔ not a proxy for it. + engine.registerNodeExecutor({ + type: 'downstream', + async execute() { + ran.push('downstream'); + return { success: true }; + }, + } as NodeExecutor); + + engine.registerFlow('gate_refuses', childFlow('gate_refuses', { outcome: 'refused', message: REFUSAL_TEMPLATE }) as never); + engine.registerFlow('gate_allows', childFlow('gate_allows') as never); + }); + + /** The newest run row for a flow — a refused run carries no `runId`. */ + async function newestRun(flowName: string) { + const runs = await engine.listRuns(flowName, { limit: 5 }); + expect(runs.length).toBeGreaterThan(0); + return engine.getRun(runs[0]!.id); + } + + describe('the defect', () => { + it('the parent run REFUSES — it does not record `completed`', async () => { + engine.registerFlow('parent', parentFlow('gate_refuses') as never); + + const result = await engine.execute('parent', triggerCtx()); + + expect(result.success).toBe(true); // a refusal is a successful evaluation + expect(result.status).toBe('refused'); + expect((await newestRun('parent'))?.status).toBe('refused'); + }); + + it("the child's rendered reason reaches the parent's caller", async () => { + engine.registerFlow('parent', parentFlow('gate_refuses') as never); + + const result = await engine.execute('parent', triggerCtx()); + + // Per-record, interpolated in the CHILD against the child's variables and + // passed through — ⛔ not re-rendered and ⛔ not invented by the parent. + expect(result.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + expect((await newestRun('parent'))?.refusalMessage).toBe('Refused: Acme Corp is a confirmed duplicate'); + }); + + it("the parent's own `successMessage` stays silent over the refusal", async () => { + engine.registerFlow('parent', parentFlow('gate_refuses') as never); + + const result = await engine.execute('parent', triggerCtx()); + + expect(result.successMessage).toBeUndefined(); + }); + + it('downstream nodes do NOT run — the out-edges are not walked', async () => { + engine.registerFlow('parent', parentFlow('gate_refuses') as never); + + await engine.execute('parent', triggerCtx()); + + expect(ran).toEqual(['child-work']); + expect(ran).not.toContain('downstream'); + }); + + it("preserves the child's #4354 rollup on the refusal path (`selected` / `acted` / `unmeasuredEffect`)", async () => { + // ⭐ The property the unwinding POSITION exists for: the signal is thrown + // after this node's success step, its `childSteps` fold and its output + // write-back, so the child's counts are already in the run log when the + // run terminates. A refusing child really can have written rows before it + // said no, and a summary that forgot them would read "nothing happened, + // safe to re-run". + engine.registerFlow('parent', parentFlow('gate_refuses') as never); + + const result = await engine.execute('parent', triggerCtx()); + + // ⛔ The refusal assertion belongs IN this test, not next door: without it + // the totals below are equally true of the unfixed engine, which rolled + // the same metrics up and then carried on. "The rollup survives the + // refusal path" is a claim about both halves at once. + expect(result.status).toBe('refused'); + expect(result.summary).toBeDefined(); + expect(result.summary).toMatchObject({ selected: 3, acted: 2, unmeasured: 1 }); + expect(result.summary!.nodes.find((n) => n.nodeId === 'sub')).toMatchObject({ + selected: 3, acted: 2, unmeasured: 1, + }); + }); + }); + + describe('the control — an ordinary child still rolls up as an ordinary success', () => { + // ⛔ Mandatory, not decoration: every assertion above is also satisfied by + // an executor that had started refusing EVERYTHING. This is the leg that + // tells the fix apart from that. + it('a non-refused child completes the parent, fires its toast, and walks on', async () => { + engine.registerFlow('parent', parentFlow('gate_allows') as never); + + const result = await engine.execute('parent', triggerCtx()); + + expect(result.success).toBe(true); + expect(result.status).toBeUndefined(); // the terminal-success exit stamps none + expect(result.refusalMessage).toBeUndefined(); + expect(result.successMessage).toBe(PARENT_TOAST); + expect(ran).toEqual(['child-work', 'downstream']); + expect((await newestRun('parent'))?.status).toBe('completed'); + // The same rollup, by the same route — the refusal arm added a branch, + // ⛔ it did not move the totals. + expect(result.summary).toMatchObject({ selected: 3, acted: 2, unmeasured: 1 }); + }); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index ec75d9cf722..d5d46ca3473 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -392,6 +392,69 @@ export interface NodeExecutionResult { * the form and `resume()` with the values. */ screen?: ScreenSpec; + /** + * [#18110 / #18555] Terminal REFUSAL. When `true`, the node evaluated + * successfully and the answer is *no*: the engine stops traversal here and + * the run finishes as a TERMINAL `refused` (a member of + * {@link TERMINAL_RUN_STATUSES} since #15788 — ⛔ no new status value), + * carrying {@link NodeExecutionResult.refusalMessage} onto the result and + * the run-history row. + * + * **The twin of {@link NodeExecutionResult.suspend}, deliberately.** Both + * are executor-facing flags asking {@link AutomationEngine.executeNode} to + * throw an internal unwinding signal, and both are read at the same point: + * AFTER the node's success step is pushed, after its `childSteps` are + * folded and after its output is written back. That position is the whole + * design — it is what keeps a refusing node's own #4354 `metrics` + * (`selected` / `acted` / `unmeasuredEffect`) in the run log and therefore + * in the run summary, instead of losing them to an unwind that began + * earlier. ⛔ Not a second unwinding protocol: `FlowRefusalSignal` already + * reuses `FlowSuspendSignal`'s, and this member is that protocol's + * executor-facing half, exactly as `suspend` is the pause's. + * + * ⚠️ `refused` here is the run OUTCOME — *a refusal is a successful + * evaluation that says no* — ⛔ NOT this package's other `refused`, the + * GUARD refusal (`refuseNode`, `guard-refusal.ts`, the resume-authority + * gate), which is a kind of FAILURE. A node that failed says so with + * `success: false`, and this flag is read only past the failure arm: on a + * failing result it changes nothing, which is the right answer rather than + * an oversight. + * + * **Precedence over `suspend`**: a refusal is terminal and a pause is a + * promise to come back, so a result carrying both REFUSES. Persisting a + * continuation for a decision the author already made would drop the + * refusal on the floor — the same fail-open direction this channel exists + * to close. No first-party executor sets both (`subflow` / `map` read one + * child status); one that does has declared a contradiction. + * + * ⚠️ With ONE exception, and it is FAIL-CLOSED. The #6667 + * undeclared-suspension guard runs ahead of all of this and reads + * {@link NodeExecutionResult.suspend} alone: when the node type resolves to + * an action descriptor that does not declare `supportsPause: true`, that + * guard REPLACES the whole result with a guard refusal, the failure arm + * answers it, and `refuse` is never read at all — the run ends `failed`, + * not `refused`. That is the correct end for a declaration defect (⛔ no + * `fault` edge may route it, and re-running the flow unchanged can never + * fix it), so ⛔ do not reorder the guard to let this member through. The + * paragraph above describes the case the guard has nothing to say about: + * a type with no descriptor, or one that declares the pause it uses. + * + * Set today by `subflow` (#18110) and `map` (#18555) when their child run + * returned `status: 'refused'`: a refusal an author wrote inside a child + * flow must not roll up to the parent as an ordinary success. + */ + refuse?: boolean; + /** + * The rendered reason for {@link NodeExecutionResult.refuse}, surfaced as + * `AutomationResult.refusalMessage` and on the terminal run-history row. + * + * For `subflow` / `map` this is the CHILD run's own `refusalMessage`, + * already interpolated against the child's live variables — passed through, + * ⛔ never re-rendered and ⛔ never replaced with text this node invented. + * `undefined` only when the refusal carried none, recorded honestly rather + * than filled in. + */ + refusalMessage?: string; /** * #1479: step logs produced inside the node's structured region(s). A * container node (`loop` / `parallel` / `try_catch`) collects the @@ -1095,7 +1158,10 @@ function isSuspendSignal(err: unknown): err is FlowSuspendSignal { /** * [#15788] Internal sentinel thrown by {@link AutomationEngine.executeNode} - * when an `end` node declares `outcome: 'refused'` (#14945 ruling 2′, lane 2). + * when a node REFUSES (#14945 ruling 2′, lane 2). Two producers, one signal: + * an `end` node declaring `outcome: 'refused'` (#15788), and any executor that + * returns {@link NodeExecutionResult.refuse} — `subflow` (#18110) and `map` + * (#18555) do, when their child run refused. * The twin of {@link FlowSuspendSignal}: it unwinds the synchronous DAG * recursion up to `execute()` / `resume()` / `executeWithoutRetry`, which * convert it into a TERMINAL `refused` run rather than a failed one. @@ -1118,13 +1184,19 @@ function isSuspendSignal(err: unknown): err is FlowSuspendSignal { class FlowRefusalSignal { readonly __flowRefused = true as const; constructor( - /** The `end` node that refused — the last node the run reached. */ + /** + * The node that carried the refusal — the last node the run reached. + * The refusing `end` itself, or the `subflow` / `map` whose child run + * refused. + */ readonly nodeId: string, /** - * The author's `message`, already interpolated against the run's live - * variables. `undefined` only when the config carried none, which - * `EndConfigSchema`'s refinement refuses at the flow parse — recorded - * honestly rather than filled in with invented text. + * The rendered reason, already interpolated against the live variables + * of the run that produced it — the author's `end` `message`, or the + * child run's own `refusalMessage` passed through. `undefined` only + * when the refusal carried none, which `EndConfigSchema`'s refinement + * refuses at the flow parse — recorded honestly rather than filled in + * with invented text. */ readonly message?: string, ) {} @@ -5276,8 +5348,9 @@ export class AutomationEngine implements IAutomationService { summary, }; } catch (err: unknown) { - // [#15788] The run reached an `end` node declaring - // `outcome: 'refused'` (#14945 ruling 2′). Tested FIRST, beside the + // [#15788] The run REFUSED — an `end` node declaring + // `outcome: 'refused'`, or (#18110 / #18555) a node whose own child + // run refused. Tested FIRST, beside the // pause and for the same reason: this is NOT a failure either, and // a signal recognised only by the arm below would be recorded as // one. The shape is `finishRefusedRun`'s — one method, all three @@ -8493,9 +8566,11 @@ export class AutomationEngine implements IAutomationService { } /** - * [#15788] Finish a run that reached an `end` node declaring - * `outcome: 'refused'` — record the terminal row and build the caller's - * result (#14945 ruling 2′, lane 2). + * [#15788] Finish a REFUSED run — record the terminal row and build the + * caller's result (#14945 ruling 2′, lane 2). Reached from either producer + * of {@link FlowRefusalSignal}: an `end` node declaring + * `outcome: 'refused'`, or a node returning + * {@link NodeExecutionResult.refuse} (#18110 / #18555). * * **ONE method, three producers.** `execute()`, `resumeInternal` and * `executeWithoutRetry` each own a terminal exit, and this file's own @@ -8580,7 +8655,8 @@ export class AutomationEngine implements IAutomationService { // meta?)`; the `Error` slot stays empty on purpose (#5575). this.logger.error( `[Automation] run '${args.runId}' of flow '${args.flowName}' REFUSED (an 'end' node with ` + - `outcome: 'refused') but its run-history bookkeeping threw, so its terminal history row ` + + `outcome: 'refused', or a node whose child run refused) but its run-history bookkeeping ` + + `threw, so its terminal history row ` + `never landed — nothing retries it, the caller is told the run refused, and after the next ` + `restart this run is invisible to the Runs surfaces while the approvals sweeps read it as ` + `never-finished. The run itself is TERMINAL and must NOT be re-run, retried or resumed. ` + @@ -9640,6 +9716,30 @@ export class AutomationEngine implements IAutomationService { } } + // [#18110 / #18555] Terminal refusal: the node evaluated and the + // answer is no. Thrown from HERE — the position the suspend signal + // below is thrown from — and that position is the point of the + // design, not a convenience: the node's success step is already + // pushed, its `childSteps` are already folded and its output is + // already written back, so a refusing `subflow` / `map` keeps the + // child's #4354 rollup (`selected` / `acted` / `unmeasuredEffect`) + // in the run summary. An unwind that began any earlier would drop + // exactly those counts — a refusing child really can have written + // rows before it said no. + // + // The step stays a SUCCESS on purpose: the node did evaluate, and + // what it evaluated to is the run's outcome, not this step's. The + // three terminal exits (`execute` / `resumeInternal` / + // `executeWithoutRetry`) already convert the signal into a + // `refused` run through the one `finishRefusedRun` chokepoint, so + // nothing downstream of here needed a second arm. + // + // Ahead of `suspend` deliberately — see `NodeExecutionResult.refuse` + // for why a result carrying both refuses rather than pausing. + if (result.refuse) { + throw new FlowRefusalSignal(node.id, result.refusalMessage); + } + // ADR-0019 durable pause: the node did its on-entry work and asked to // suspend here. Output is already written above; unwind the recursion // up to execute()/resume(), which persists a continuation. Traversal @@ -9986,11 +10086,23 @@ export class AutomationEngine implements IAutomationService { // question and ⛔ not one this lane rules on — the #14945 ruling // says nothing about regions, and "prefer failing to falling back" // decides the interim. + // + // [#18110 / #18555] The sentence NAMES the node that carried the + // refusal and nothing more, because there are now two producers: an + // `end` declaring the refusal itself, and a `subflow` / `map` whose + // CHILD run refused. Hard-wired to the first, it told an author + // inside a region to go find an `end` node that is not in their + // region at all, and handed them a prescription they could not + // follow. ⛔ TEXT only — region SEMANTICS are untouched (#18112's + // option B is not implemented, no container is taught to rethrow), + // and the authoring-time half of this boundary is #15646's, ⛔ not + // this change's. if (isRefusalSignal(err)) { throw new Error( - `an 'end' node declaring outcome: 'refused' inside a structured region (node ` + - `'${err.nodeId}') is not supported — a refusal terminates the RUN, and a region ` + - `body cannot end one. Put the refusing 'end' on the top-level graph and route the ` + + `a refusal inside a structured region (node '${err.nodeId}') is not supported — a ` + + `refusal terminates the RUN, and a region body cannot end one. The refusing node is ` + + `either an 'end' declaring outcome: 'refused', or a node whose own child run refused ` + + `(a 'subflow' / 'map'). Move that node onto the top-level graph and route the ` + `region's exit to it.`, ); }