fix(workflows): namespace nested descendant step ids in loops/fan-out by Noor-ul-ain001 · Pull Request #4338 · github/spec-kit

Noor-ul-ain001 · 2026-08-26T10:36:19Z

Summary

while/do-while loop bodies and fan-out templates namespace nested step ids per iteration/item so logs and state.step_results entries stay unique — but the namespacing only rewrote the id of the immediate child step, not any descendant nested deeper (e.g. a shell step inside an if inside a while body, or inside a fan-out template's if/switch branch). That grandchild kept its bare, unnamespaced id across every iteration/item, so each iteration/item silently overwrote the previous one's entry in state.step_results under that same key — only the last iteration's or item's result for that nested step ever survived, and no per-iteration/per-item record of it ever existed.
This is also a correctness gap beyond bookkeeping: nested/template step ids are deliberately exempted from the workflow's global id-uniqueness validation, on the assumption that runtime namespacing makes any collision safe. Since only the top-level child was actually namespaced, a step nested one level deeper could collide with an unrelated step of the same id elsewhere in the workflow and silently overwrite its result.
Fix: add _rename_step_tree_ids, which recursively rewrites every id in a step's subtree (walking then/else/steps/default/cases.* — the same nesting keys overlays/merge.py walks for step-tree attribution) and returns a {new_id: original_id} map. Both the while/do-while loop body and fan-out's run_item now use this helper instead of renaming only the top-level id, and alias every renamed descendant's result back to its original id (mirroring the existing single-level aliasing) so sibling steps within the same iteration/item and code reading steps.<id>.output after the loop/fan-out still see that iteration's/item's value.

Test plan

Added test_while_loop_namespaces_nested_descendant_steps and test_fan_out_namespaces_nested_descendant_steps to tests/test_workflows.py::TestWorkflowEngine: a shell step nested inside an if inside a while body (and inside a fan-out template) gets a distinct namespaced state.step_results entry per iteration/item, while the unprefixed key still holds the latest value.
Verified both fail without the fix (test-the-test): the namespaced keys (retry-loop:leaf:1, fan:leaf:0, etc.) were simply absent, and step_results only ever held the last iteration's/item's bare-keyed entry — reproducing the exact bug.
Ran the full tests/test_workflows.py suite: 926 passed, 20 pre-existing Windows symlink-elevation failures (need admin rights, unrelated to this change), 7 skipped. All While/DoWhile/FanOut/FanOutConcurrency tests pass, including the concurrent-execution and per-thread context isolation tests.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

https://claude.ai/code/session_01PJHJ2dHP2RVCNncHqN8Qm9

`while`/`do-while` loop bodies and `fan-out` templates namespace nested step ids per iteration/item so logs and `state.step_results` entries stay unique — but the namespacing only rewrote the id of the *immediate* child step, not any descendant nested deeper (e.g. a `shell` step inside an `if` inside a `while` body, or inside a `fan-out` template's `if`/`switch` branch). That grandchild kept its bare, unnamespaced id across every iteration/item, so each iteration/item silently overwrote the previous one's entry in `state.step_results` under that same key — only the last iteration's or item's result for that nested step ever survived, and no per-iteration/per-item record of it ever existed. This is also a correctness gap beyond bookkeeping: nested/template step ids are deliberately exempted from the workflow's global id-uniqueness validation, on the assumption that runtime namespacing makes any collision safe. Since only the top-level child was actually namespaced, a step nested one level deeper could collide with an unrelated step of the same id elsewhere in the workflow and silently overwrite its result. Fix: add `_rename_step_tree_ids`, which recursively rewrites every id in a step's subtree (walking `then`/`else`/`steps`/`default`/`cases.*` — the same nesting keys `overlays/merge.py` walks for step-tree attribution) and returns a `{new_id: original_id}` map. Both the while/do-while loop body and fan-out's `run_item` now use this helper instead of renaming only the top-level id, and alias every renamed descendant's result back to its original id (mirroring the existing single-level aliasing) so sibling steps within the same iteration/item and code reading `steps.<id>.output` after the loop/fan-out still see that iteration's/item's value. ## Test plan - Added `test_while_loop_namespaces_nested_descendant_steps` and `test_fan_out_namespaces_nested_descendant_steps` to `tests/test_workflows.py::TestWorkflowEngine`: a `shell` step nested inside an `if` inside a `while` body (and inside a `fan-out` template) gets a distinct namespaced `state.step_results` entry per iteration/item, while the unprefixed key still holds the latest value. - Verified both fail without the fix (test-the-test): the namespaced keys (`retry-loop:leaf:1`, `fan:leaf:0`, etc.) were simply absent, and `step_results` only ever held the last iteration's/item's bare-keyed entry — reproducing the exact bug. - Ran the full `tests/test_workflows.py` suite: 926 passed, 20 pre-existing Windows symlink-elevation failures (need admin rights, unrelated to this change), 7 skipped. All `While`/`DoWhile`/`FanOut`/`FanOutConcurrency` tests pass, including the concurrent-execution and per-thread context isolation tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PJHJ2dHP2RVCNncHqN8Qm9

Copilot

🟡 Changes recommended

Initial loop iterations remain unnamespaced, while delayed and shared aliases break sibling references and concurrent fan-out isolation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds recursive runtime namespacing for nested workflow steps in loops and fan-out templates.

Changes:

Adds recursive descendant ID rewriting and aliasing.
Adds while and fan-out regression tests.
Preserves namespaced execution results.

File summaries

File	Description
`src/specify_cli/workflows/engine.py`	Recursively namespaces nested step IDs.
`tests/test_workflows.py`	Tests nested loop and fan-out results.

Review details

Suppressed comments (1)

src/specify_cli/workflows/engine.py:1447

These aliases are created only after the entire renamed subtree finishes. If a branch contains step a followed by step b that references steps.a, b executes before a is aliased and reads the previous iteration's value (or no value), whereas both steps previously used their bare IDs. Alias each descendant immediately after that descendant completes so intra-branch references retain their existing semantics.

                            for new_id, orig_id in id_map.items():
                                if new_id in context.steps:
                                    self._record_result(
                                        context, state, orig_id,
                                        context.steps[new_id],
                                    )

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

mnriem · 2026-09-01T22:55:13Z

Please address Copilot feedback

… post-subtree Addresses Copilot review feedback on PR github#4338: - while/do-while loop iteration 0 ran through a separate, unnamespaced code path before the loop-specific namespacing logic was reached, so it had no dedicated state.step_results entry and was immediately overwritten the moment iteration 1's aliasing ran. Every iteration, including the first, now goes through the same _rename_step_tree_ids + alias_map path. - Bare-id aliasing for a namespaced descendant happened only after its entire renamed subtree finished executing, so a later sibling step in the same iteration/item that referenced an earlier sibling by its original id ran before that alias existed and read a stale (or absent) value. _execute_steps now threads an alias_map through so each descendant is aliased immediately after it completes, not in bulk afterward. - For a concurrent fan-out (max_concurrency > 1), that same bare-id alias write raced across worker threads sharing context.steps, so one item's sibling read could observe another item's value. Each concurrent item now runs against a private ChainMap overlay for its bare-id aliases; only the namespaced (disjoint-key) result is published to shared state during execution. Once every item has finished (back on the single thread), the last item's aliases are applied to shared state once, deterministically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhR6g8xT8at5pPMhkrC3e2

mnriem · 2026-09-08T13:29:33Z

Strong direction — this is a real correctness bug (silent per-iteration result overwrite + the uniqueness-exemption collision risk), disclosed and tested. Two review findings to resolve before merge: the first loop iteration still isn't namespaced (it goes through the generic result.next_steps path before _rename_step_tree_ids runs), and the post-subtree aliasing breaks sibling references and concurrent fan-out isolation — a later child can execute before an earlier renamed child is aliased back. Please address those and re-request review.

Separately: you currently have 7 open PRs. Per CONTRIBUTING, that's past the point where further submissions may be deprioritized in the queue. Several are small workflow/config validation fixes (#4319, #4323, #4324, #4325) — could you consolidate the related ones so review can focus? This PR can stay standalone; it's the smaller validation fixes I'd group.

Copilot

🟡 Changes recommended

Nested-loop aliasing, expression resolution, and bare-ID collisions remain incorrect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/specify_cli/workflows/engine.py:1457

When this loop is itself inside another loop or a fan-out template, its result.next_steps were already recursively renamed by the outer _rename_step_tree_ids. Renaming them again treats the first generated ID as the original (for example fan:leaf:0 becomes fan:while:0:fan:leaf:0:0) and aliases only back to fan:leaf:0, so a later inner-loop sibling reading steps.leaf cannot see the current value. Preserve canonical original IDs/compose the outer alias map, or avoid eagerly renaming bodies that receive their own runtime namespace.

                            ns_copy, id_map = _rename_step_tree_ids(
                                ns, step_id, str(_loop_iter),
                                default_id=f"step-{ns_idx}",

src/specify_cli/workflows/engine.py:1752

The concurrent path has the same bare-ID collision as the sequential alias path: because each fan-out template is validated in its own ID set, orig_id can already belong to an unrelated workflow step or another fan-out, and this call deterministically overwrites that result after the pool joins. Namespaced entries survive, but the unrelated bare-key result still does not, so runtime namespacing has not made collisions safe.

            for orig_id, data in alias_slots[last_idx].items():
                self._record_result(context, state, orig_id, data)

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Balanced

Noor-ul-ain001 · 2026-09-13T15:23:47Z

+                    if alias_local_only:
+                        context.steps[orig_id] = step_data
+                    else:
+                        self._record_result(context, state, orig_id, step_data)


Fixed both this (sequential) and the concurrent post-join write at line 1751/now further down. Added _collect_reserved_step_ids, which walks the workflow's regular control-flow nesting keys (then/else/steps/default/cases) -- the same shape _validate_steps walks into its global seen_ids -- but deliberately skips a fan-out's own step template, mirroring the fresh-set() exemption at engine.py:569-580. That gives a static, run-wide set of every id declared OUTSIDE a fan-out template. Both alias-write sites (this one and the concurrent post-join loop) now skip writing the bare-id alias when orig_id is a member of that set -- the namespaced entry is written unconditionally either way, so nothing is lost, only the unsafe convenience alias is suppressed. Kept it scoped to fan-out specifically (a new alias_may_collide flag, set only for fan-out template execution) rather than checking every alias write, since a while/do-while loop body's ids ARE globally unique by validation and gating those too would have broken the loop-aliasing this PR adds. Added tests for both the sequential and concurrent paths, each confirmed via test-the-test to fail pre-fix (the unrelated step's result gets clobbered with the fan-out's last item's data) and pass now (7365250).

Noor-ul-ain001 · 2026-09-13T15:23:34Z

+            original_steps = item_ctx.steps
+            local_overlay: dict[str, dict[str, Any]] = {}
+            if local_only:
+                item_ctx.steps = ChainMap(local_overlay, original_steps)


Confirmed and fixed. Verified directly: isinstance(ChainMap(...), dict) is False, and _build_namespace's ns["steps"] = context.steps or {} put that ChainMap straight into the expression namespace, so _resolve_dot_path's isinstance(current, dict) check failed on the very first steps.<id> hop -- every such expression inside a concurrent fan-out item silently resolved to None. Replaced the ChainMap with a plain dict(original_steps) snapshot taken once per item: still isolates this item's own writes from concurrently-running siblings (a fresh copy, never shared), but is a real dict so every expression path works. Added a test that goes through evaluate_expression("{{ steps.first.output.marker }}", context) (the real templating path) rather than context.steps.get(), and confirmed via test-the-test that it fails against the ChainMap version (None == 0) and passes now (a8188b6).

…liasing Two follow-up bugs in the nested step-id namespacing added for loops and fan-out templates: 1. A while/do-while step nested inside an outer loop iteration or fan-out item had its OWN 'steps' body eagerly renamed by the outer _rename_step_tree_ids pass (since 'steps' is a walked nesting key). When that while step then ran its own per-iteration rename, it treated the already-namespaced id (e.g. "fan:leaf:0") as the original and aliased back to that synthetic id instead of the workflow author's real bare id ("leaf") -- so a doubly-prefixed id like "fan:while:0:fan:leaf:0:0" was the only place the value ever landed, and state.step_results["leaf"] was never populated at all. _rename_step_tree_ids now still renames a nested while/do-while step's own id, but leaves its 'steps' body untouched for the loop's own runtime namespacing to rename exactly once, against the real ids. 2. A fan-out template's bare-id convenience alias (writing the current item's result under its unprefixed id, so `steps.<id>` sees the latest value) could silently clobber an unrelated, distinctly-authored step's result if the two happened to share an id -- fan-out templates are deliberately exempt from the workflow's global id-uniqueness check, so nothing prevents that collision. This applied to both the sequential path's immediate alias write and the concurrent path's deferred post-join write. Added _collect_reserved_step_ids, which computes the set of step ids declared outside any fan-out template (mirroring _validate_steps' global id set), and gate both alias-write sites on it via a new alias_may_collide flag -- set only for fan-out's own template execution, so ordinary while/do-while loop-body aliasing (always globally unique by validation) is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U74yBbvVQCPwB7Ed8Dzeu6

…n-out The concurrent fan-out item isolation gave each item's context.steps a ChainMap(local_overlay, original_steps) so a sibling step could resolve an earlier one by its bare id without racing other concurrently-running items. But _resolve_dot_path (the function every {{ steps.x.output... }} expression goes through) only descends when isinstance(current, dict) is true, and ChainMap is not a dict subclass -- so _build_namespace's `ns["steps"] = context.steps or {}` put a non-dict at "steps", and every steps.* expression evaluated inside a concurrent fan-out item silently resolved to None. The existing test only exercised context.steps.get() directly, which IS supported by ChainMap, so it never caught this. Replaced the ChainMap with a plain dict snapshot of the shared steps dict, taken once when the item starts. It still isolates this item's own writes from concurrently-running siblings (a fresh copy per item, never shared), it's a real dict so isinstance(..., dict) and every expression path work normally, and the snapshot copy is safe under the GIL against a concurrent sibling's writes to the source dict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U74yBbvVQCPwB7Ed8Dzeu6

Noor-ul-ain001 · 2026-09-13T15:24:15Z

Addressed everything in this review — both the two posted inline comments and the two suppressed ones summarized in the review body.

1. Double-renaming when a while/do-while loop is nested inside an outer loop iteration or fan-out item (the suppressed comment quoting engine.py:1457): confirmed exactly as described — a nested while step's own steps body was being eagerly renamed by the outer _rename_step_tree_ids pass (since steps is a walked nesting key), so the loop's own per-iteration rename then treated that already-namespaced id as "original" and aliased back to a synthetic id like fan:leaf:0 instead of the real leaf. state.step_results["leaf"] was never populated at all. Fixed by having _rename_step_tree_ids still rename a nested while/do-while step's own id, but leave its steps body untouched — the loop renames it exactly once, against the real original ids, when it actually runs.

2 & 3. Fan-out bare-id alias clobbering an unrelated step (both the suppressed concurrent-path comment quoting engine.py:1752, and the posted sequential-path comment): fan-out template ids are exempt from the workflow's global id-uniqueness check, so a template's bare id can coincide with a real step elsewhere. Added _collect_reserved_step_ids (mirrors _validate_steps's global seen_ids, minus the fan-out-template exemption) and gated both the sequential path's immediate alias write and the concurrent path's deferred post-join write on membership in that set — scoped via a new alias_may_collide flag so ordinary loop-body aliasing (always globally unique) is untouched.

4. ChainMap breaking steps.* expressions in concurrent fan-out (posted comment): confirmed — isinstance(ChainMap(...), dict) is False, and _resolve_dot_path only descends through actual dicts, so every {{ steps.x.output... }} expression inside a concurrent fan-out item silently resolved to None. Replaced the ChainMap overlay with a plain dict snapshot per item, which keeps the same isolation property while being a real dict.

Each fix has a dedicated regression test, and each was confirmed via test-the-test (temporarily reverting just that fix) to fail against the prior code and pass now. Full test_workflows.py run clean except the pre-existing Windows-symlink-elevation failures unrelated to this change (20 failures, same set as baseline on main). Pushed as 7365250 and a8188b6.

Noor-ul-ain001 requested a review from mnriem as a code owner August 26, 2026 10:36

mnriem requested a balanced review from Copilot September 1, 2026 21:11

Copilot started reviewing on behalf of mnriem September 1, 2026 21:11 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py

Comment thread src/specify_cli/workflows/engine.py Outdated

mnriem requested a balanced review from Copilot and removed request for Copilot September 1, 2026 22:54

Copilot started reviewing on behalf of mnriem September 1, 2026 22:54 View session

mnriem requested a balanced review from Copilot September 8, 2026 13:27

Copilot started reviewing on behalf of mnriem September 8, 2026 13:28 View session

mnriem added author-awaiting Waiting on author response author-over-cap Over the 3-open-PR cap or repetitive batch submissions — please consolidate triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review labels Sep 8, 2026

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Noor-ul-ain001 and others added 2 commits September 13, 2026 20:15

mnriem removed the author-over-cap Over the 3-open-PR cap or repetitive batch submissions — please consolidate label Sep 14, 2026

Navigation Menu

Uh oh!

fix(workflows): namespace nested descendant step ids in loops/fan-out - #4338

fix(workflows): namespace nested descendant step ids in loops/fan-out#4338
Noor-ul-ain001 wants to merge 4 commits into
github:mainfrom
Noor-ul-ain001:fix/nested-step-id-namespace-collision

Noor-ul-ain001 commented Aug 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

mnriem commented Sep 1, 2026

Uh oh!

mnriem commented Sep 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Noor-ul-ain001 Sep 13, 2026

Uh oh!

Noor-ul-ain001 Sep 13, 2026

Uh oh!

Noor-ul-ain001 commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Noor-ul-ain001 commented Aug 26, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

mnriem commented Sep 1, 2026

Uh oh!

mnriem commented Sep 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Noor-ul-ain001 Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

Noor-ul-ain001 Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

Noor-ul-ain001 commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants