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

[Noor-ul-ain001]

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.

Follow-up fixes (review rounds)

• Namespace loop iteration 0 too and alias immediately (not after the whole subtree finishes), so the first iteration gets its own recorded entry and a same-iteration sibling sees an earlier sibling's aliased value right away.
• Stop double-renaming loop bodies nested inside an outer loop/fan-out, and stop an unsafe fan-out bare-id alias write from racing across concurrently-running items (each concurrent item now gets a private context.steps overlay instead of writing the shared one directly).
• Stop using a ChainMap overlay for that per-item isolation: _resolve_dot_path (what every {{ steps.x... }} expression goes through) only descends through isinstance(current, dict), and ChainMap isn't a dict subclass, so every such expression inside a concurrent fan-out item silently resolved to None. Replaced with a plain dict copy.
• Latest round — two further edge cases combining previously-separate test scenarios:
  1. Reserved-id collision + item-local sibling reference: when a fan-out template id collides with a real, globally-unique step elsewhere in the workflow, the guard that protects that unrelated step's persisted result also suppressed the write needed for a later sibling step within the same item to resolve the colliding id locally — the sequential path (which runs directly against the real shared context.steps, unlike the concurrent path's private copy) read the outside value instead of the item's own. Fixed by always updating context.steps for the local read while still skipping the persisted-state write for the collision case; the sequential path now snapshots and restores that entry so the transient item-local write never leaks past the item that made it.
  2. Nested while/do-while aliasing dropped under concurrency: a while/do-while body nested inside a fan-out template renames itself dynamically at runtime, so it has no entry in the fan-out's own static id map. The concurrent path's per-item alias publishing was reconstructed from that static map after the item finished, which silently missed the while body's dynamic alias — the namespaced entries (e.g. fan:item:0:leaf:0) were still recorded correctly, but the convenience bare-id entry (leaf) never was, unlike the sequential path (which publishes immediately during execution instead of reconstructing afterward). Fixed by threading a mutable accumulator through _execute_steps that captures every alias write at the point it happens, regardless of nesting depth or whether the rename was static or dynamic.

Test plan

• test_while_loop_namespaces_nested_descendant_steps, test_fan_out_namespaces_nested_descendant_steps (original fix)
• test_fan_out_concurrent_sibling_step_isolated_per_item, test_fan_out_concurrent_sibling_step_resolves_via_expression, test_fan_out_concurrent_alias_never_clobbers_unrelated_step, test_fan_out_sequential_alias_never_clobbers_unrelated_step (prior review rounds)
• New this round: test_fan_out_reserved_id_collision_still_resolves_item_local_sibling, parametrized over max_concurrency 1 and 2 — a template's second step reads the first's output via {{ steps.first.output... }} where first collides with an outside reserved step; asserts the item-local value resolves under both paths, and the outside step's entry survives untouched
• New this round: test_while_loop_nested_in_fan_out_aliases_to_true_original_id is now parametrized over max_concurrency 1 and 2 (previously sequential-only)
• Verified both new/updated tests fail without this round's fix (test-the-test, git stash on engine.py only): test_fan_out_reserved_id_collision_still_resolves_item_local_sibling[1] failed with the outside value ('unrelated' == 'a') leaking into the item-local read; test_while_loop_nested_in_fan_out_aliases_to_true_original_id[2] failed with KeyError: 'leaf' — both reproducing exactly the two symptoms from review, then passed once restored
• Ran the full tests/test_workflows.py suite: 935 passed, 20 pre-existing Windows symlink-elevation failures (need admin rights, unrelated to this change), 7 skipped — no regressions

AI Disclosure

• I did use AI assistance (describe below)

This PR, across all rounds including this one, was authored with Claude Code (model: Claude Sonnet 5), used in its interactive, agentic CLI mode with full local tool access (file read/edit, running pytest, git). For this round specifically: the AI agent traced both combined edge cases back to their root cause in _execute_steps/_run_fan_out, wrote the source fix and the two regression tests above, and ran the test-the-test verification and full suite described above, under my direction. I reviewed the diff and test output myself before pushing and understand the change being made.

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

[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]

Please address Copilot feedback

[mnriem]

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]

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]

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).

[Noor-ul-ain001]

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.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[mnriem]

The earlier review fixes are present, but two combined cases still need correction.

First, combine the reserved-ID collision case with a sibling reference inside the fan-out item. With an outside first result and an item-local first, sequential execution reads the outside value while concurrent execution correctly reads the item’s value. Protecting shared state must not suppress item-local lookup.

Second, run the nested-while test with both one and two workers. Sequential execution publishes the latest bare leaf result; concurrent execution leaves it absent, despite preserving the namespaced records. Please carry those nested aliases through deterministic publication.

Please cover both cases across worker counts and complete the plain AI disclosure with tool, mode/settings, and extent of assistance; the model attribution is already present.

Drafted for @mnriem with assistance from GitHub Copilot (model: GPT-6 Astra; interactive comment drafting).

[Noor-ul-ain001]

Thanks for combining those two cases — both were real, and each was masked by the other's path working correctly:

1. Reserved-id collision + item-local sibling reference: the guard that protects an outside colliding step's persisted result was also suppressing the context.steps write a same-item sibling needs for local resolution — only on the sequential path, since it runs directly against the real shared context.steps (the concurrent path already has a private per-item copy, which is why it was unaffected). Fixed by always writing context.steps for the local read while still skipping the persisted-state write for the collision case, with a snapshot/restore in run_item so that transient write never leaks past the item that made it.
2. Nested while/do-while aliasing dropped under concurrency: a nested while body renames itself dynamically at runtime, so the concurrent path's post-hoc reconstruction of alias records from the fan-out's static id map missed it — the namespaced entries were fine, only the bare-id convenience alias (leaf) was silently dropped. Fixed by threading a mutable accumulator through _execute_steps that records every alias at write time, at any nesting depth, instead of reconstructing afterward.

Added test_fan_out_reserved_id_collision_still_resolves_item_local_sibling and parametrized the existing test_while_loop_nested_in_fan_out_aliases_to_true_original_id over max_concurrency 1 and 2, as requested. Verified both fail on the pre-fix code with the exact symptoms described (outside value leaking into the item-local read; KeyError: 'leaf'), then pass with the fix. Full suite: 935 passed, same 20 pre-existing Windows symlink failures, 7 skipped.

Also added a plain AI Disclosure section to the PR body (tool/model, mode, extent of assistance) covering this round and the prior ones.