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.

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

[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