diff --git a/.changeset/contained-failure-visibility.md b/.changeset/contained-failure-visibility.md index fd9080a7bf..c749b5ffd5 100644 --- a/.changeset/contained-failure-visibility.md +++ b/.changeset/contained-failure-visibility.md @@ -10,7 +10,7 @@ Four changes populate the contract `@objectstack/spec` already declares: - **`FlowRunSummary.failed`** — `summarizeRun` now folds `failed = Σ nodes[].failures` over the per-node array it publishes, so the run-level count can never disagree with the breakdown it summarizes. It counts every node execution that failed, contained or fatal; on a run that completed, all of them were contained. - **`failed=N` on the run summary line** — `formatRunSummaryLine` prints the token whenever the count is present, `failed=0` included. That is the opposite of the `unmeasured` rule beside it and deliberate: `unmeasured` qualifies `acted`, while `failed` answers a question a completed run's line otherwise cannot be asked at all. Read `failed=0` precisely: **no node execution of this run failed**. It is the node fold and only that, so a `subflow` child's own contained failures stay on the child's summary rather than rolling up the way `acted` does — see #15617, where the declaration's two paragraphs are being reconciled. -- **Iteration through `try_catch`** — a step that ran in a `try` or `catch` region inside a loop body now carries the enclosing loop's `iteration`, with `regionKind` still `try` / `catch`. The step says which region ran it *and* which row it ran for. `parallel` branch tagging is unchanged. +- **Iteration through `try_catch`** — a step that ran in a `try` or `catch` region inside a loop body now carries the enclosing loop's `iteration`, with `regionKind` still `try` / `catch`. The step says which region ran it *and* which row it ran for. (`parallel` branch tagging was unchanged by *this* change; the entry below retires the `iteration` overload it left standing.) - **`$error` binds the row** — the value bound to `errorVariable` (default `$error`) is the declared `TryCatchErrorValue`: `nodeId` and `message` as before, plus `iteration` and the loop's current `item` when the failure happened inside a loop body. A `subflow` / `map` child run has its own variable scope and therefore binds neither, so a parent's row identity never leaks into a child's `$error`. **`failed` absent means "not tracked", never `0`.** Runs recorded before this change keep it absent — no migration and no default, the same convention `unmeasured` carries. Defaulting it to zero would tell an operator "nothing failed" about a run nobody measured. Absent, the summary line prints no `failed=` token at all; present-and-zero prints `failed=0`. The count rides in the persisted `summary_json`, including on a summary compacted past the size cap, where the per-node `failures` it folds are exactly what gets dropped. diff --git a/.changeset/region-tagger-branch-key.md b/.changeset/region-tagger-branch-key.md new file mode 100644 index 0000000000..c888377190 --- /dev/null +++ b/.changeset/region-tagger-branch-key.md @@ -0,0 +1,16 @@ +--- +"@objectstack/service-automation": minor +--- + +The run step log tells a `parallel` branch apart from a `loop` row: `iteration` is the enclosing loop's iteration, always, and the branch index moves to `branch`. + +The engine half of the ruling `@objectstack/spec` already declares (`ExecutionStepLogSchema.branch`). One field used to hold both meanings, told apart only by reading `regionKind` first, and `runRegion`'s tagger let the innermost region win outright — it skipped any step a nested region had already tagged. Together those two facts made `loop { body: [ parallel { branches } ] }` unreadable: every branch step recorded its branch index and **no** step of that branch recorded the row it ran for, so a per-row failure inside a branch was attributable to a branch and never to a row. That is the shape a fan-out inside a sweep has, and the one an operator most needs to read. + +- **`branch` is written, and only inside a parallel branch.** `parallel` tags its branch regions with `branch: i` instead of `iteration: i`. A step outside a parallel branch carries no `branch` at all. +- **`iteration` is single-valued and carried through nesting.** `runRegion`'s tagger now splits what "innermost wins" governs. The IDENTITY fields — `parentNodeId`, `regionKind`, `retryAttempt` — answer *which region ran this step* and still belong to the innermost region outright; an enclosing region never relabels them. The INDEX fields — `iteration` and `branch` — answer *which pass of which region*, and nested regions contribute different ones that are both true of the same step, so an enclosing region now fills the index the inner region left undefined instead of being turned away at the door. A branch step inside a loop body therefore carries **both**: the row on `iteration`, the branch on `branch`. +- **`try` / `catch` inside a loop is unchanged**, deliberately. Such a region has no index of its own, so its steps keep carrying the enclosing loop's `iteration` with `regionKind` still naming the region, and gain no `branch`. It is the control arm of this change, not a subject of it. +- **Nested loops are unchanged too.** "Fill only what is undefined" still holds in both halves, so for `loop { loop }` the inner loop's `iteration` stands. + +`StepLogEntry` (exported) gains `branch?: number`. It is not derived from the spec type, and is now held equal to it by a type-level pin rather than by a comment claiming they agree. + +**Reading a run recorded before this change.** `iteration` on a `regionKind: 'parallel-branch'` step written by an older engine is a BRANCH index, not a row — the same absent-versus-zero care the run summary's other counters need. Nothing is migrated and nothing is defaulted: a step with no `branch` key is either a pre-change record or a step that ran outside a parallel branch, and `regionKind` is what tells those apart. Bumped `minor` rather than `major` to match the contract half of the same ruling, which shipped its declaration change that way. diff --git a/docs/qa/platform-checklist/areas/automation.json b/docs/qa/platform-checklist/areas/automation.json index bf3d48db9f..cbc675d6c4 100644 --- a/docs/qa/platform-checklist/areas/automation.json +++ b/docs/qa/platform-checklist/areas/automation.json @@ -76,7 +76,7 @@ "title": "Flow Runs render loop/region iterations as a nested execution tree", "since": "v16", "status": "active", - "revision": 2, + "revision": 3, "priority": "P1", "surface": "mixed", "personas": [ @@ -85,7 +85,8 @@ "fixtures": { "app": "showcase", "requires": [ - "showcase_batch_reminders (examples/app-showcase/src/automation/flows/index.ts BatchRemindersFlow) — an autolaunched loop flow with a `tasks` list input, runnable on demand via the trigger route" + "showcase_batch_reminders (examples/app-showcase/src/automation/flows/index.ts BatchRemindersFlow) — an autolaunched loop flow with a `tasks` list input, runnable on demand via the trigger route", + "a `loop` whose body holds a two-branch `parallel` — the only shape that exercises both index keys on one step, needed by the loop { parallel } clause. ⚠️ NOT PRESENT in examples/app-showcase: it carries `loop` (showcase_batch_reminders) and `parallel` (showcase_fan_out_notify) as SEPARATE flows and nests neither, so that clause scores blocked(fixture) until such a flow lands (filed as #16356). The clause is written now because what it pins is settled — maintainer ruling 2026-09-03 — and a clause missing from this item is exactly what let the `iteration` overload sit unmeasured" ] }, "steps": [ @@ -95,6 +96,7 @@ "record every step's nodeId, nodeType, status, parentNodeId, iteration, regionKind (ExecutionStepLogSchema #1505 region tags)", "open the flow in the Studio flow-designer (metadata-admin) and its Runs panel (FlowRunsPanel) — NOT the developer Flow Runs page — and expand the newest run", "screenshot the expanded step tree showing the per-iteration children under the loop node", + "nesting run (needs the loop { parallel } fixture above): trigger that flow over at least two rows, GET its newest run detail, and record every branch step's parentNodeId, iteration, branch and regionKind — both index keys are read off the SAME step, which is the whole point of the clause", "contrast run: trigger again with {\"params\": {\"tasks\": []}} and capture the loop step of that run" ], "acceptance": [ @@ -110,6 +112,12 @@ "verify": "assert the three send_reminder steps each carry {parentNodeId:'loop_tasks', iteration: 0|1|2, regionKind:'loop-body'} and no two share an iteration; top-level steps (start/loop_tasks/end) carry NO parentNodeId", "evidence": "step-log excerpt with the tags" }, + { + "clause": "loop { parallel }: a branch step carries BOTH indices, one meaning each — the enclosing loop's row on `iteration`, its own branch position on `branch` — and `iteration` never carries a branch index. Maintainer ruling 2026-09-03: \"`iteration` is single-valued: the zero-based iteration of the enclosing loop, carried through any nesting … The branch index lives on a new optional `branch` key, present only on steps inside a `parallel` branch.\" Before it, one field held both meanings and the innermost region won outright, so every branch step of a loop { parallel } recorded its branch and NO step of that branch recorded the row — a per-row failure inside a branch was attributable to a branch and never to the row", + "oracle": "api", + "verify": "from the nesting run's detail take the steps with regionKind='parallel-branch' and assert THREE things, because any two of them still pass on a broken engine: (a) each carries a `branch` equal to its branch position, `branch: 0` INCLUDED — a falsy check anywhere on the way silently drops the first branch; (b) each carries an `iteration` equal to the ROW it ran for, so across N rows x M branches every (iteration, branch) pair appears exactly once — the pre-fix engine wrote a CONSTANT iteration per branch, which is why counting distinct values is not enough on its own; (c) the enclosing `parallel` container step itself reads regionKind='loop-body' with the row on `iteration` and NO `branch` of its own. Cross-check that every recorded step still parses under ExecutionStepLogSchema", + "evidence": "step-log excerpt for one full row showing both branch steps with their (iteration, branch) pairs, plus the enclosing parallel container step" + }, { "clause": "the designer Runs panel renders the iterations as a nested tree (per-iteration children folded under the loop node, labeled 1-based), not a flat list", "oracle": "screenshot", @@ -154,6 +162,12 @@ "date": "2026-08-07", "change": "expanded to deep-test contract: concrete steps, multi-clause acceptance, negatives, variants", "ref": "claude/platform-test-checklist-ocwugl" + }, + { + "revision": 3, + "date": "2026-09-06", + "change": "added the loop { parallel } clause. The item pinned parentNodeId/iteration/regionKind on a plain loop body and said nothing about the nested case — the one shape where two enclosing regions each have an index of their own. It was unwritable while `iteration` was overloaded, because there was no correct reading to assert: the branch index displaced the row. The maintainer ruling of 2026-09-03 made `iteration` single-valued and gave the branch index its own `branch` key, and the engine's runRegion tagger now carries an outer region's index through nesting instead of discarding it for a step an inner region already tagged. Also records, rather than hides, the fixture gap the clause exposes: showcase nests neither construct in the other", + "ref": "#15230" } ] }, diff --git a/packages/services/service-automation/src/builtin/contained-failure-visibility.test.ts b/packages/services/service-automation/src/builtin/contained-failure-visibility.test.ts index 2a94ecb64e..d1bd556973 100644 --- a/packages/services/service-automation/src/builtin/contained-failure-visibility.test.ts +++ b/packages/services/service-automation/src/builtin/contained-failure-visibility.test.ts @@ -331,9 +331,17 @@ describe('#14456 — a contained per-iteration failure is visible, attributed an } }); - // ── The fence: `parallel` is untouched ───────────────────────────────── - - it('leaves `parallel` branch tagging exactly as it was — the #14414 fence', async () => { + // ── `parallel` inside a loop: both indices, one meaning each ─────────── + // + // This case was the #14414 FENCE while the overload stood: it asserted that + // #14456 had left `parallel` alone, with the branch index still occupying + // `iteration`. #15230 is the card that retired the overload (maintainer + // ruling 2026-09-03), so the fence is spent and what it guarded has moved: + // the assertion below now reads the two indices apart. It stays in this + // suite because #14456's subject — attributing a contained per-row failure + // to its ROW — is exactly what a branch step could not do before. + + it('loop { parallel }: a branch step carries the row on `iteration` and the branch on `branch` (#15230)', async () => { setup(CASES); engine.registerFlow('par', { name: 'par', label: 'par', type: 'autolaunched', runAs: 'system', @@ -370,13 +378,23 @@ describe('#14456 — a contained per-iteration failure is visible, attributed an await engine.execute('par', { event: 'schedule' } as AutomationContext); const run = (await engine.listRuns('par'))[0]; - // A branch step still carries its BRANCH index on `iteration` and - // `regionKind: 'parallel-branch'`, five times over (once per row) — the - // pre-existing overload #14414 owns. Nothing here changed it. + // Five rows x two branches, `regionKind: 'parallel-branch'` throughout. const branchSteps = run.steps.filter((s) => s.regionKind === 'parallel-branch'); expect(branchSteps).toHaveLength(10); - expect(branchSteps.filter((s) => s.nodeId === 'flag').every((s) => s.iteration === 0)).toBe(true); - expect(branchSteps.filter((s) => s.nodeId === 'noop').every((s) => s.iteration === 1)).toBe(true); + + const flagSteps = branchSteps.filter((s) => s.nodeId === 'flag'); + const noopSteps = branchSteps.filter((s) => s.nodeId === 'noop'); + + // The BRANCH index is constant per branch and lives on `branch`. + expect(flagSteps.every((s) => s.branch === 0)).toBe(true); + expect(noopSteps.every((s) => s.branch === 1)).toBe(true); + + // The ROW comes through `iteration`, carried down from the enclosing + // loop — the reading that did not exist while the branch index sat + // there. Each branch ran once per row, so each sees every row exactly + // once. + expect(flagSteps.map((s) => s.iteration)).toEqual([0, 1, 2, 3, 4]); + expect(noopSteps.map((s) => s.iteration)).toEqual([0, 1, 2, 3, 4]); }); }); diff --git a/packages/services/service-automation/src/builtin/parallel-node.test.ts b/packages/services/service-automation/src/builtin/parallel-node.test.ts index ff3c6d7825..c17fc35931 100644 --- a/packages/services/service-automation/src/builtin/parallel-node.test.ts +++ b/packages/services/service-automation/src/builtin/parallel-node.test.ts @@ -96,11 +96,18 @@ describe('parallel block executor (ADR-0031)', () => { const stepA = runs[0].steps.find(s => s.nodeId === 'a'); const stepB = runs[0].steps.find(s => s.nodeId === 'b'); expect(stepA?.parentNodeId).toBe('par'); - expect(stepA?.iteration).toBe(0); + expect(stepA?.branch).toBe(0); expect(stepA?.regionKind).toBe('parallel-branch'); expect(stepB?.parentNodeId).toBe('par'); - expect(stepB?.iteration).toBe(1); + expect(stepB?.branch).toBe(1); expect(stepB?.regionKind).toBe('parallel-branch'); + // #15230: the branch index moved off `iteration`, which is now single-valued + // — the enclosing LOOP's row. This `parallel` sits in no loop, so there is + // nothing for it to report and it stays absent. Asserting the absence is the + // point: re-spelling the assertion alone would leave the retired overload + // free to come back as a second writer. + expect(stepA?.iteration).toBeUndefined(); + expect(stepB?.iteration).toBeUndefined(); }); it('joins only after the slowest branch completes', async () => { diff --git a/packages/services/service-automation/src/builtin/parallel-node.ts b/packages/services/service-automation/src/builtin/parallel-node.ts index 68c475075c..85bb8e732d 100644 --- a/packages/services/service-automation/src/builtin/parallel-node.ts +++ b/packages/services/service-automation/src/builtin/parallel-node.ts @@ -72,11 +72,22 @@ export function registerParallelNode(engine: AutomationEngine, ctx: PluginContex try { // Implicit join: continue once when ALL branches have completed. // #1479: each branch returns its body steps, tagged with the branch index. + // + // #15230: the branch index goes on `branch`, NOT on `iteration`. It + // used to share `iteration` with the loop's row index — one field, two + // meanings, told apart only by reading `regionKind` first — and because + // the tagger let the innermost region win outright, a branch step + // inside a loop body recorded the branch and lost the row entirely. + // `iteration` is now single-valued (the enclosing loop's, carried + // through nesting by `runRegion`'s tagger), so nothing here writes it: + // a branch step inside a loop body gets the row from the loop's own + // tagging pass, and a bare `parallel` leaves it absent because there is + // no enclosing loop to report. Maintainer ruling 2026-09-03. branchSteps = await Promise.all( - branches.map((branch, i) => - engine.runRegion(branch, variables, context ?? ({} as AutomationContext), { + branches.map((branchRegion, i) => + engine.runRegion(branchRegion, variables, context ?? ({} as AutomationContext), { parentNodeId: node.id, - iteration: i, + branch: i, regionKind: 'parallel-branch', }), ), diff --git a/packages/services/service-automation/src/builtin/region-index-keys.test.ts b/packages/services/service-automation/src/builtin/region-index-keys.test.ts new file mode 100644 index 0000000000..6f25fd8c03 --- /dev/null +++ b/packages/services/service-automation/src/builtin/region-index-keys.test.ts @@ -0,0 +1,296 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #15230 — the region index keys a step log entry carries, and what each one + * means. The engine half of #14414; the contract half is `ExecutionStepLogSchema` + * (`packages/spec/src/automation/execution.zod.ts`), already landed. + * + * Maintainer ruling, 2026-09-03 (recorded on #14414): + * + * `iteration` is single-valued: the zero-based iteration of the enclosing + * loop, carried through any nesting … The branch index lives on a new + * optional `branch` key, present only on steps inside a `parallel` branch. + * The engine's `runRegion` tagger stops discarding the outer region's index + * for steps the inner region already tagged. + * + * The three cases the ruling names are pinned here, plus the two guards that + * keep them honest: + * + * 1. `loop { parallel }` — a branch step carries BOTH the loop's `iteration` + * and its own `branch`. This is the shape that was unreadable before: the + * branch index won outright and no step of that branch recorded the row, + * so a per-row failure inside a branch was attributable to a branch and + * never to the row. + * 2. `loop { try_catch }` — UNCHANGED. A try/catch region has no index of its + * own, so its steps carry the enclosing loop's `iteration` and no + * `branch`, with `regionKind` still naming the region. Control arm: this + * must read the same before and after the tagger change. + * 3. `parallel` NOT inside a loop — `branch` is written and `iteration` is + * ABSENT. `iteration` no longer doubles as the branch index, so there is + * nothing for it to say here. + * + * Guards: + * - every record the engine produced parses under the spec schema at THIS + * head, and `branch` refuses negative / fractional values at the `branch` + * path (⛔ not a positives-only gate); + * - a TYPE-LEVEL pin holding the engine's local `StepLogEntry` region keys + * equal to the spec type's. The engine interface is NOT derived from the + * spec — the card asks for the two to be kept in step "by a pin, not by + * prose", and this is that pin. ⚠️ It is compiled by + * `pnpm --filter @objectstack/service-automation typecheck` (both + * `tsconfig.json` and `tsconfig.test.json` reach `src/**\/*.test.ts`) and + * is INVISIBLE to `vitest`, which strips types — a green `pnpm test` says + * nothing about it. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ExecutionStepLogSchema } from '@objectstack/spec/automation'; +import type { ExecutionStepLog } from '@objectstack/spec/automation'; +import { AutomationEngine } from '../engine.js'; +import type { NodeExecutor, StepLogEntry } from '../engine.js'; +import { registerLoopNode } from './loop-node.js'; +import { registerParallelNode } from './parallel-node.js'; +import { registerTryCatchNode } from './try-catch-node.js'; + +// ── Type-level pin: engine `StepLogEntry` ↔ spec `ExecutionStepLog` ───────── +// +// Region-grouping keys only. The engine's interface legitimately carries +// engine-local fields the contract does not (`nodeLabel`, `warnings`), and the +// contract carries fields the engine never writes (`input`, `output`), so +// pinning the WHOLE shape would pin noise. What must not drift is the set of +// keys this card is about and the type of each. +type RegionKeys = 'parentNodeId' | 'iteration' | 'branch' | 'regionKind' | 'retryAttempt'; + +// Both `Pick`s fail to compile if either side is missing a key — that is half +// the pin. `Eq` is the invariant (not merely assignable) comparison, so a +// widening on one side is caught too. +type Eq = (() => G extends A ? 1 : 2) extends () => G extends B ? 1 : 2 ? true : false; +type Assert = T; +export type _RegionKeysInStep = Assert< + Eq, Pick> +>; + +function silentLogger(): any { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } }; +} +function ctx(): any { + return { logger: silentLogger(), getService() { throw new Error('none'); } }; +} + +/** Assert every step the run produced is a legal `ExecutionStepLog` at this head. */ +function expectParsesUnderSpec(steps: StepLogEntry[]): void { + expect(steps.length).toBeGreaterThan(0); + for (const step of steps) { + const parsed = ExecutionStepLogSchema.safeParse(step); + if (!parsed.success) { + throw new Error( + `step ${step.nodeId} does not parse under ExecutionStepLogSchema: ` + + JSON.stringify(parsed.error.issues), + ); + } + } +} + +describe('#15230 — `iteration` is the loop\'s, `branch` is the parallel branch\'s', () => { + let engine: AutomationEngine; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + registerLoopNode(engine, ctx()); + registerParallelNode(engine, ctx()); + registerTryCatchNode(engine, ctx()); + engine.registerNodeExecutor({ + type: 'touch', + async execute() { return { success: true }; }, + } as NodeExecutor); + engine.registerNodeExecutor({ + type: 'boom', + async execute(node) { throw new Error(`boom from ${node.id}`); }, + } as NodeExecutor); + }); + + // ── Ruling pin 1: `loop { parallel }` ────────────────────────────────── + it('loop { parallel }: a branch step carries the loop\'s `iteration` AND its own `branch`', async () => { + engine.registerFlow('lp', { + name: 'lp', label: 'Loop over parallel', type: 'autolaunched', + variables: [{ name: 'rows', type: 'list', isInput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'each', type: 'loop', label: 'For each row', + config: { + collection: '{rows}', iteratorVariable: 'row', indexVariable: 'idx', + body: { + nodes: [{ + id: 'fan', type: 'parallel', label: 'Fan out', + config: { + branches: [ + { name: 'A', nodes: [{ id: 'leafA', type: 'touch', label: 'A' }], edges: [] }, + { name: 'B', nodes: [{ id: 'leafB', type: 'touch', label: 'B' }], edges: [] }, + ], + }, + }], + edges: [], + }, + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'each' }, + { id: 'e2', source: 'each', target: 'end' }, + ], + } as never); + + const result = await engine.execute('lp', { params: { rows: ['r0', 'r1', 'r2'] } }); + expect(result.success).toBe(true); + const steps = (await engine.listRuns('lp'))[0].steps as StepLogEntry[]; + + const branchSteps = steps.filter(s => s.regionKind === 'parallel-branch'); + // 3 rows x 2 branches. + expect(branchSteps).toHaveLength(6); + + // Each branch step names BOTH indices: which row, and which branch. + expect( + branchSteps.map(s => `${s.nodeId}@iteration=${s.iteration}/branch=${s.branch}`).sort(), + ).toEqual([ + 'leafA@iteration=0/branch=0', + 'leafA@iteration=1/branch=0', + 'leafA@iteration=2/branch=0', + 'leafB@iteration=0/branch=1', + 'leafB@iteration=1/branch=1', + 'leafB@iteration=2/branch=1', + ]); + + // The innermost container still wins the IDENTITY fields — carrying the + // outer index through must not relabel which region ran the step. + for (const s of branchSteps) expect(s.parentNodeId).toBe('fan'); + + // The parallel container step itself is a loop-body step: it has the + // row, and no branch of its own. + const fanSteps = steps.filter(s => s.nodeId === 'fan'); + expect(fanSteps).toHaveLength(3); + for (const s of fanSteps) { + expect(s.regionKind).toBe('loop-body'); + expect(s.parentNodeId).toBe('each'); + expect(s.branch).toBeUndefined(); + } + expect(fanSteps.map(s => s.iteration)).toEqual([0, 1, 2]); + + expectParsesUnderSpec(steps); + }); + + // ── Ruling pin 2: `loop { try_catch }` — the CONTROL ARM, unchanged ───── + it('loop { try_catch }: try/catch steps keep the loop\'s `iteration` and gain no `branch`', async () => { + engine.registerFlow('ltc', { + name: 'ltc', label: 'Loop over try_catch', type: 'autolaunched', + variables: [{ name: 'rows', type: 'list', isInput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'each', type: 'loop', label: 'For each row', + config: { + collection: '{rows}', iteratorVariable: 'row', + body: { + nodes: [{ + id: 'guard', type: 'try_catch', label: 'Guard', + config: { + try: { nodes: [{ id: 'risky', type: 'boom', label: 'Risky' }], edges: [] }, + catch: { nodes: [{ id: 'recover', type: 'touch', label: 'Recover' }], edges: [] }, + }, + }], + edges: [], + }, + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'each' }, + { id: 'e2', source: 'each', target: 'end' }, + ], + } as never); + + const result = await engine.execute('ltc', { params: { rows: ['r0', 'r1'] } }); + expect(result.success).toBe(true); + const steps = (await engine.listRuns('ltc'))[0].steps as StepLogEntry[]; + + const trySteps = steps.filter(s => s.regionKind === 'try'); + const catchSteps = steps.filter(s => s.regionKind === 'catch'); + expect(trySteps).toHaveLength(2); + expect(catchSteps).toHaveLength(2); + + // A try/catch region has no index of its own: the row comes through + // `iteration`, `regionKind` still names the region, `branch` is absent. + for (const s of [...trySteps, ...catchSteps]) { + expect(s.parentNodeId).toBe('guard'); + expect(s.branch).toBeUndefined(); + } + expect(trySteps.map(s => s.iteration)).toEqual([0, 1]); + expect(catchSteps.map(s => s.iteration)).toEqual([0, 1]); + + expectParsesUnderSpec(steps); + }); + + // ── Ruling pin 3: `parallel` NOT inside a loop ───────────────────────── + it('bare parallel: a branch step writes `branch` and NO `iteration`', async () => { + engine.registerFlow('bare', { + name: 'bare', label: 'Bare parallel', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'fan', type: 'parallel', label: 'Fan out', + config: { + branches: [ + { name: 'A', nodes: [{ id: 'leafA', type: 'touch', label: 'A' }], edges: [] }, + { name: 'B', nodes: [{ id: 'leafB', type: 'touch', label: 'B' }], edges: [] }, + ], + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'fan' }, + { id: 'e2', source: 'fan', target: 'end' }, + ], + } as never); + + const result = await engine.execute('bare'); + expect(result.success).toBe(true); + const steps = (await engine.listRuns('bare'))[0].steps as StepLogEntry[]; + + const leafA = steps.find(s => s.nodeId === 'leafA'); + const leafB = steps.find(s => s.nodeId === 'leafB'); + expect(leafA?.regionKind).toBe('parallel-branch'); + expect(leafB?.regionKind).toBe('parallel-branch'); + expect(leafA?.branch).toBe(0); + expect(leafB?.branch).toBe(1); + // No enclosing loop ⇒ nothing for `iteration` to say. Before #15230 it + // carried the branch index; that overload is what the ruling removed. + expect(leafA?.iteration).toBeUndefined(); + expect(leafB?.iteration).toBeUndefined(); + + expectParsesUnderSpec(steps); + }); + + // ── Guard: the contract refuses illegal `branch` values ───────────────── + it('the spec refuses a negative or fractional `branch`, at the `branch` path', () => { + const base = { + nodeId: 'leafA', nodeType: 'touch', status: 'success' as const, + startedAt: '2026-09-06T00:00:00.000Z', + parentNodeId: 'fan', regionKind: 'parallel-branch', + }; + + expect(ExecutionStepLogSchema.safeParse({ ...base, branch: 0 }).success).toBe(true); + expect(ExecutionStepLogSchema.safeParse({ ...base, branch: 7 }).success).toBe(true); + + for (const bad of [-1, 1.5]) { + const parsed = ExecutionStepLogSchema.safeParse({ ...base, branch: bad }); + expect(parsed.success).toBe(false); + // The refusal must land ON `branch` — a schema that rejected the + // whole record for some other reason would read the same at the + // `success` boolean. + expect(parsed.success === false && parsed.error.issues.some(i => i.path[0] === 'branch')).toBe(true); + } + }); +}); diff --git a/packages/services/service-automation/src/builtin/try-catch-node.ts b/packages/services/service-automation/src/builtin/try-catch-node.ts index 80021174d6..da39dac7ad 100644 --- a/packages/services/service-automation/src/builtin/try-catch-node.ts +++ b/packages/services/service-automation/src/builtin/try-catch-node.ts @@ -189,12 +189,17 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex regionKind: 'try', // #14456 — forward the ENCLOSING loop's iteration so a step this // region ran says which region ran it AND which row it ran for. - // `runRegion`'s tagger fills only fields the INNERMOST tagger - // left undefined, so a loop's own tagger can never reach past - // this one to a try/catch step; forwarding at this call site is - // what closes that, and it leaves both the tagger and `parallel` - // untouched (a branch step already carries its own `iteration`, - // and nothing here changes what `parallel` writes). + // A try/catch region has no index of its own, so `iteration` is + // free to carry the row. + // + // #15230 made `runRegion`'s tagger carry `iteration` THROUGH + // nesting, so an enclosing loop would now reach a try/catch step + // on its own and this forwarding is no longer the only route. + // It stays, and stays FIRST: the value is identical (both are the + // loop's row index), it is what `$error.iteration` is bound from + // twenty lines below, and it keeps the row on these steps even + // when the run unwinds through a path that never gives the loop's + // tagger a pass over them. ...(loopFrame ? { iteration: loopFrame.iteration } : {}), // Only tag the attempt index when a retry ladder is actually // declared: on a plain `try_catch` every step would carry a diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 8b8aeaf9ed..8603e9cca6 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -738,12 +738,39 @@ export interface StepLogEntry { * #1479: structured-region grouping. When a step ran inside a `loop` / * `parallel` / `try_catch` body region, these tag it with its **immediate** * container so run observability can distinguish per-iteration / per-branch - * body steps from top-level ones. Set by {@link AutomationEngine.runRegion} - * (innermost wins — never overwritten as steps bubble through nested regions). + * body steps from top-level ones. Set by {@link AutomationEngine.runRegion}. + * + * #15230: the innermost region wins the IDENTITY fields — `parentNodeId`, + * `regionKind`, `retryAttempt` name which region actually ran the step, and + * a step a nested region already claimed keeps them. The INDEX fields + * (`iteration`, `branch`) are instead carried THROUGH nesting: an enclosing + * region fills the one the inner region left undefined rather than having + * it discarded. + * + * ⚠️ This interface is NOT derived from the spec's `ExecutionStepLog` + * (`packages/spec/src/automation/execution.zod.ts`). The two are held in + * step by the type-level pin in `builtin/region-index-keys.test.ts` — by + * that assertion, not by this comment. */ parentNodeId?: string; - /** Zero-based loop iteration or parallel branch index of the enclosing region. */ + /** + * Zero-based iteration of the enclosing `loop`, carried through any + * nesting. **Single-valued** (#15230, maintainer ruling 2026-09-03): it no + * longer doubles as the parallel branch index — that lives on `branch`. + * A step inside a `try` / `catch` region nested in a loop body carries the + * enclosing loop's iteration here, because such a region has no index of + * its own. + */ iteration?: number; + /** + * #15230: zero-based index of the enclosing `parallel` branch. Present only + * on a step inside a parallel branch, absent everywhere else. When the + * parallel node is itself inside a loop body the step carries BOTH — the + * row through `iteration`, the branch through here — which is what the + * overload could not express: the branch index won outright and no step of + * that branch recorded which row it ran for. + */ + branch?: number; /** Which region kind the step ran in: `loop-body` | `parallel-branch` | `try` | `catch`. */ regionKind?: string; /** @@ -8276,9 +8303,17 @@ export class AutomationEngine implements IAutomationService { * * #1479: the executed body steps are **returned** (tagged with `grouping`) * so the calling container node can fold them into the parent run log via - * `NodeExecutionResult.childSteps`. Tagging only fills fields left undefined, - * so when regions nest, each step keeps its **innermost** container's - * `parentNodeId` / `iteration` / `regionKind` / `retryAttempt`. + * `NodeExecutionResult.childSteps`. Tagging only ever fills fields left + * undefined, so when regions nest each step keeps its **innermost** + * container's IDENTITY — `parentNodeId` / `regionKind` / `retryAttempt`. + * + * #15230: the INDEX fields are the exception, and the reason this card + * exists. `iteration` and `branch` are carried THROUGH nesting: an + * enclosing region fills the index the inner region left undefined instead + * of being discarded because the step already had a `parentNodeId`. Before + * this, `loop { parallel }` recorded the branch index and nothing else, so + * a per-row failure inside a branch was attributable to a branch and never + * to the row. * * #7546: a region that FAILS still throws — the `try_catch` retry/throw * semantics are untouched — but its partial steps are no longer discarded. @@ -8308,7 +8343,7 @@ export class AutomationEngine implements IAutomationService { region: FlowRegionParsed, variables: Map, context: AutomationContext, - grouping?: { parentNodeId: string; iteration?: number; regionKind?: string; retryAttempt?: number }, + grouping?: { parentNodeId: string; iteration?: number; branch?: number; regionKind?: string; retryAttempt?: number }, partialSteps?: StepLogEntry[], ): Promise { const entryId = findRegionEntry(region); @@ -8319,20 +8354,44 @@ export class AutomationEngine implements IAutomationService { // A synthetic flow view — executeNode/traverseNext only read `nodes`/`edges`. const subFlow = { nodes: region.nodes, edges: region.edges ?? [] } as unknown as FlowParsed; const regionSteps: StepLogEntry[] = []; - // Tag this region's steps with their immediate container. Innermost wins: - // a step that already carries a `parentNodeId` (set by a nested region) - // is left untouched. Shared by the success and failure paths (#7546) so - // a failed attempt's steps are indistinguishable in SHAPE from a - // successful one's — they differ only in their own `status`. + // Tag this region's steps with their container. Shared by the success + // and failure paths (#7546) so a failed attempt's steps are + // indistinguishable in SHAPE from a successful one's — they differ only + // in their own `status`. + // + // #15230 splits what "innermost wins" governs, because the two halves + // answer different questions: + // + // IDENTITY (`parentNodeId` / `regionKind` / `retryAttempt`) answers + // WHICH REGION RAN THIS STEP. Innermost wins outright: a step a + // nested region already claimed keeps naming that region, and an + // enclosing region must never relabel it. + // + // INDEX (`iteration` / `branch`) answers WHICH PASS OF WHICH REGION. + // Nested regions contribute DIFFERENT indices — the loop's row and + // the parallel's branch are both true of the same step — so an + // enclosing region fills the index the inner one left undefined + // instead of being skipped along with the identity fields. That skip + // is the defect: `loop { parallel }` used to record the branch and + // discard the row. + // + // Still "only fills what is undefined" in both halves, so an index a + // nested region DID set wins: for `loop { loop }` the inner loop's + // `iteration` stands, exactly as before. const tag = (): void => { if (!grouping) return; for (const step of regionSteps) { if (step.parentNodeId === undefined) { step.parentNodeId = grouping.parentNodeId; - if (grouping.iteration !== undefined) step.iteration = grouping.iteration; if (grouping.regionKind !== undefined) step.regionKind = grouping.regionKind; if (grouping.retryAttempt !== undefined) step.retryAttempt = grouping.retryAttempt; } + if (grouping.iteration !== undefined && step.iteration === undefined) { + step.iteration = grouping.iteration; + } + if (grouping.branch !== undefined && step.branch === undefined) { + step.branch = grouping.branch; + } } }; try {