Skip to content

Commit fde99b1

Browse files
committed
fix(service-automation): runRegion carries the outer region's index through nesting; the parallel branch index moves to branch (#15230)
The engine half of the maintainer ruling of 2026-09-03; the contract half (`ExecutionStepLogSchema.branch`) already landed in `@objectstack/spec`. `runRegion`'s tagger let the innermost region win outright, skipping any step a nested region had already tagged, and `parallel` wrote its branch index into `iteration`. Together those made `loop { parallel }` unreadable: every branch step recorded its branch and no step of that branch recorded its row. The tagger now splits what "innermost wins" governs. IDENTITY fields (`parentNodeId` / `regionKind` / `retryAttempt`) answer WHICH REGION ran the step and still belong to the innermost region outright. INDEX fields (`iteration` / `branch`) answer WHICH PASS of which region — nested regions contribute different ones, 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. Still "fill only what is undefined", so `loop { loop }` keeps the inner loop's `iteration`. `try` / `catch` inside a loop is unchanged (the control arm): such a region has no index of its own, so its steps keep the loop's `iteration` and gain no `branch`. Two fixtures pinned the retired branch and are replaced, not re-spelled: parallel-node.test.ts asserted the branch index ON `iteration` for a bare parallel, and contained-failure-visibility.test.ts carried the "#14414 fence" asserting that #14456 had left the overload standing. #15230 is the card that retires it, so the fence is spent and now reads the two indices apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
1 parent 60f48e6 commit fde99b1

8 files changed

Lines changed: 165 additions & 35 deletions

File tree

‎.changeset/contained-failure-visibility.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ Four changes populate the contract `@objectstack/spec` already declares:
1010

1111
- **`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.
1212
- **`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.
13-
- **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.
13+
- **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.)
1414
- **`$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`.
1515

1616
**`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.
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
"@objectstack/service-automation": minor
3+
---
4+
5+
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`.
6+
7+
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.
8+
9+
- **`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.
10+
- **`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`.
11+
- **`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.
12+
- **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.
13+
14+
`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.
15+
16+
**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.

‎docs/qa/platform-checklist/areas/automation.json‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@
7676
"title": "Flow Runs render loop/region iterations as a nested execution tree",
7777
"since": "v16",
7878
"status": "active",
79-
"revision": 2,
79+
"revision": 3,
8080
"priority": "P1",
8181
"surface": "mixed",
8282
"personas": [
@@ -85,7 +85,8 @@
8585
"fixtures": {
8686
"app": "showcase",
8787
"requires": [
88-
"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"
88+
"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",
89+
"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 separately). 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 through two releases"
8990
]
9091
},
9192
"steps": [
@@ -95,6 +96,7 @@
9596
"record every step's nodeId, nodeType, status, parentNodeId, iteration, regionKind (ExecutionStepLogSchema #1505 region tags)",
9697
"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",
9798
"screenshot the expanded step tree showing the per-iteration children under the loop node",
99+
"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",
98100
"contrast run: trigger again with {\"params\": {\"tasks\": []}} and capture the loop step of that run"
99101
],
100102
"acceptance": [
@@ -110,6 +112,12 @@
110112
"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",
111113
"evidence": "step-log excerpt with the tags"
112114
},
115+
{
116+
"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",
117+
"oracle": "api",
118+
"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",
119+
"evidence": "step-log excerpt for one full row showing both branch steps with their (iteration, branch) pairs, plus the enclosing parallel container step"
120+
},
113121
{
114122
"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",
115123
"oracle": "screenshot",
@@ -154,6 +162,12 @@
154162
"date": "2026-08-07",
155163
"change": "expanded to deep-test contract: concrete steps, multi-clause acceptance, negatives, variants",
156164
"ref": "claude/platform-test-checklist-ocwugl"
165+
},
166+
{
167+
"revision": 3,
168+
"date": "2026-09-06",
169+
"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",
170+
"ref": "#15230"
157171
}
158172
]
159173
},

‎packages/services/service-automation/src/builtin/contained-failure-visibility.test.ts‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -331,9 +331,17 @@ describe('#14456 — a contained per-iteration failure is visible, attributed an
331331
}
332332
});
333333

334-
// ── The fence: `parallel` is untouched ─────────────────────────────────
335-
336-
it('leaves `parallel` branch tagging exactly as it was — the #14414 fence', async () => {
334+
// ── `parallel` inside a loop: both indices, one meaning each ───────────
335+
//
336+
// This case was the #14414 FENCE while the overload stood: it asserted that
337+
// #14456 had left `parallel` alone, with the branch index still occupying
338+
// `iteration`. #15230 is the card that retired the overload (maintainer
339+
// ruling 2026-09-03), so the fence is spent and what it guarded has moved:
340+
// the assertion below now reads the two indices apart. It stays in this
341+
// suite because #14456's subject — attributing a contained per-row failure
342+
// to its ROW — is exactly what a branch step could not do before.
343+
344+
it('loop { parallel }: a branch step carries the row on `iteration` and the branch on `branch` (#15230)', async () => {
337345
setup(CASES);
338346
engine.registerFlow('par', {
339347
name: 'par', label: 'par', type: 'autolaunched', runAs: 'system',
@@ -370,13 +378,23 @@ describe('#14456 — a contained per-iteration failure is visible, attributed an
370378
await engine.execute('par', { event: 'schedule' } as AutomationContext);
371379
const run = (await engine.listRuns('par'))[0];
372380

373-
// A branch step still carries its BRANCH index on `iteration` and
374-
// `regionKind: 'parallel-branch'`, five times over (once per row) — the
375-
// pre-existing overload #14414 owns. Nothing here changed it.
381+
// Five rows x two branches, `regionKind: 'parallel-branch'` throughout.
376382
const branchSteps = run.steps.filter((s) => s.regionKind === 'parallel-branch');
377383
expect(branchSteps).toHaveLength(10);
378-
expect(branchSteps.filter((s) => s.nodeId === 'flag').every((s) => s.iteration === 0)).toBe(true);
379-
expect(branchSteps.filter((s) => s.nodeId === 'noop').every((s) => s.iteration === 1)).toBe(true);
384+
385+
const flagSteps = branchSteps.filter((s) => s.nodeId === 'flag');
386+
const noopSteps = branchSteps.filter((s) => s.nodeId === 'noop');
387+
388+
// The BRANCH index is constant per branch and lives on `branch`.
389+
expect(flagSteps.every((s) => s.branch === 0)).toBe(true);
390+
expect(noopSteps.every((s) => s.branch === 1)).toBe(true);
391+
392+
// The ROW comes through `iteration`, carried down from the enclosing
393+
// loop — the reading that did not exist while the branch index sat
394+
// there. Each branch ran once per row, so each sees every row exactly
395+
// once.
396+
expect(flagSteps.map((s) => s.iteration)).toEqual([0, 1, 2, 3, 4]);
397+
expect(noopSteps.map((s) => s.iteration)).toEqual([0, 1, 2, 3, 4]);
380398
});
381399
});
382400

‎packages/services/service-automation/src/builtin/parallel-node.test.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,18 @@ describe('parallel block executor (ADR-0031)', () => {
9696
const stepA = runs[0].steps.find(s => s.nodeId === 'a');
9797
const stepB = runs[0].steps.find(s => s.nodeId === 'b');
9898
expect(stepA?.parentNodeId).toBe('par');
99-
expect(stepA?.iteration).toBe(0);
99+
expect(stepA?.branch).toBe(0);
100100
expect(stepA?.regionKind).toBe('parallel-branch');
101101
expect(stepB?.parentNodeId).toBe('par');
102-
expect(stepB?.iteration).toBe(1);
102+
expect(stepB?.branch).toBe(1);
103103
expect(stepB?.regionKind).toBe('parallel-branch');
104+
// #15230: the branch index moved off `iteration`, which is now single-valued
105+
// — the enclosing LOOP's row. This `parallel` sits in no loop, so there is
106+
// nothing for it to report and it stays absent. Asserting the absence is the
107+
// point: re-spelling the assertion alone would leave the retired overload
108+
// free to come back as a second writer.
109+
expect(stepA?.iteration).toBeUndefined();
110+
expect(stepB?.iteration).toBeUndefined();
104111
});
105112

106113
it('joins only after the slowest branch completes', async () => {

‎packages/services/service-automation/src/builtin/parallel-node.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,11 +72,22 @@ export function registerParallelNode(engine: AutomationEngine, ctx: PluginContex
7272
try {
7373
// Implicit join: continue once when ALL branches have completed.
7474
// #1479: each branch returns its body steps, tagged with the branch index.
75+
//
76+
// #15230: the branch index goes on `branch`, NOT on `iteration`. It
77+
// used to share `iteration` with the loop's row index — one field, two
78+
// meanings, told apart only by reading `regionKind` first — and because
79+
// the tagger let the innermost region win outright, a branch step
80+
// inside a loop body recorded the branch and lost the row entirely.
81+
// `iteration` is now single-valued (the enclosing loop's, carried
82+
// through nesting by `runRegion`'s tagger), so nothing here writes it:
83+
// a branch step inside a loop body gets the row from the loop's own
84+
// tagging pass, and a bare `parallel` leaves it absent because there is
85+
// no enclosing loop to report. Maintainer ruling 2026-09-03.
7586
branchSteps = await Promise.all(
76-
branches.map((branch, i) =>
77-
engine.runRegion(branch, variables, context ?? ({} as AutomationContext), {
87+
branches.map((branchRegion, i) =>
88+
engine.runRegion(branchRegion, variables, context ?? ({} as AutomationContext), {
7889
parentNodeId: node.id,
79-
iteration: i,
90+
branch: i,
8091
regionKind: 'parallel-branch',
8192
}),
8293
),

‎packages/services/service-automation/src/builtin/try-catch-node.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,12 +189,17 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex
189189
regionKind: 'try',
190190
// #14456 — forward the ENCLOSING loop's iteration so a step this
191191
// region ran says which region ran it AND which row it ran for.
192-
// `runRegion`'s tagger fills only fields the INNERMOST tagger
193-
// left undefined, so a loop's own tagger can never reach past
194-
// this one to a try/catch step; forwarding at this call site is
195-
// what closes that, and it leaves both the tagger and `parallel`
196-
// untouched (a branch step already carries its own `iteration`,
197-
// and nothing here changes what `parallel` writes).
192+
// A try/catch region has no index of its own, so `iteration` is
193+
// free to carry the row.
194+
//
195+
// #15230 made `runRegion`'s tagger carry `iteration` THROUGH
196+
// nesting, so an enclosing loop would now reach a try/catch step
197+
// on its own and this forwarding is no longer the only route.
198+
// It stays, and stays FIRST: the value is identical (both are the
199+
// loop's row index), it is what `$error.iteration` is bound from
200+
// twenty lines below, and it keeps the row on these steps even
201+
// when the run unwinds through a path that never gives the loop's
202+
// tagger a pass over them.
198203
...(loopFrame ? { iteration: loopFrame.iteration } : {}),
199204
// Only tag the attempt index when a retry ladder is actually
200205
// declared: on a plain `try_catch` every step would carry a

0 commit comments

Comments
 (0)