From e8820455b6c2a00d005e176092443651a1ebbc3b Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 09:14:25 -0500 Subject: [PATCH 1/8] fix: while/do-while loop condition reads stale iteration-0 step output After executing namespaced loop body steps, copy each iteration's results back to the original unprefixed step key so that evaluate_condition() sees the latest values instead of stale iteration-0 data. Fixes #2592 --- src/specify_cli/workflows/engine.py | 11 +- tests/test_workflows.py | 177 ++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 934cfbe5ee..0e5223ad17 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -674,15 +674,24 @@ def _execute_steps( break # Namespace nested step IDs per iteration iter_steps = [] + original_ids = {} for ns in result.next_steps: ns_copy = dict(ns) if "id" in ns_copy: - ns_copy["id"] = f"{step_id}:{ns_copy['id']}:{_loop_iter + 1}" + orig = ns_copy["id"] + ns_copy["id"] = f"{step_id}:{orig}:{_loop_iter + 1}" + original_ids[ns_copy["id"]] = orig iter_steps.append(ns_copy) self._execute_steps( iter_steps, context, state, registry, step_offset=-1, ) + # Copy namespaced results back to unprefixed + # keys so loop conditions see updated values. + for namespaced, orig in original_ids.items(): + if namespaced in context.steps: + context.steps[orig] = context.steps[namespaced] + state.step_results[orig] = context.steps[namespaced] if state.status in ( RunStatus.PAUSED, RunStatus.FAILED, diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 3fa71f3404..616a681ee0 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1889,6 +1889,183 @@ def test_validate_workflow_rejects_non_string_default_for_string_type(self): errors = validate_workflow(definition) assert any("invalid default" in e for e in errors), errors + def test_while_loop_condition_reads_latest_iteration(self, project_dir): + """Regression: while-loop condition must see updated step output + from the most recent iteration, not stale iteration-0 data. + + See https://github.com/github/spec-kit/issues/2592 + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + from specify_cli.workflows.base import RunStatus + + # Shell step echoes a counter via a file. + # Condition: exit_code != 0 means "keep looping" — but a non-zero + # exit code would mark the step FAILED and abort the run, so we + # use stdout-based comparison instead. + # + # Iteration 0: counter=1, echoes "1" → not "done" → loop continues + # Iteration 1: counter=2, echoes "done" → condition false → stop + # Without the fix, condition always reads iteration-0 stdout, + # so the loop runs all max_iterations. + counter_file = project_dir / ".counter" + counter_file.write_text("0", encoding="utf-8") + + yaml_str = f""" +schema_version: "1.0" +workflow: + id: "while-condition-update" + name: "While Condition Update" + version: "1.0.0" +steps: + - id: retry-loop + type: while + condition: "{{{{ 'done' not in steps.attempt.output.stdout }}}}" + max_iterations: 5 + steps: + - id: attempt + type: shell + run: | + n=$(cat {counter_file}) + n=$((n + 1)) + printf '%s' $n > {counter_file} + if [ "$n" -ge 2 ]; then printf done; else printf $n; fi +""" + definition = WorkflowDefinition.from_string(yaml_str) + engine = WorkflowEngine(project_dir) + state = engine.execute(definition) + + assert state.status == RunStatus.COMPLETED + # The unprefixed key should reflect the latest iteration's result. + assert state.step_results["attempt"]["output"]["stdout"] == "done" + # Namespaced iteration-1 result should also exist. + assert "retry-loop:attempt:1" in state.step_results + # Counter should be 2 (iteration 0 + iteration 1), not 5. + assert counter_file.read_text(encoding="utf-8").strip() == "2" + + def test_do_while_loop_condition_reads_latest_iteration(self, project_dir): + """Regression: do-while loop condition must also see updated output. + + See https://github.com/github/spec-kit/issues/2592 + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + from specify_cli.workflows.base import RunStatus + + counter_file = project_dir / ".counter" + counter_file.write_text("0", encoding="utf-8") + + yaml_str = f""" +schema_version: "1.0" +workflow: + id: "do-while-condition-update" + name: "Do While Condition Update" + version: "1.0.0" +steps: + - id: retry-loop + type: do-while + condition: "{{{{ 'done' not in steps.attempt.output.stdout }}}}" + max_iterations: 5 + steps: + - id: attempt + type: shell + run: | + n=$(cat {counter_file}) + n=$((n + 1)) + printf '%s' $n > {counter_file} + if [ "$n" -ge 2 ]; then printf done; else printf $n; fi +""" + definition = WorkflowDefinition.from_string(yaml_str) + engine = WorkflowEngine(project_dir) + state = engine.execute(definition) + + assert state.status == RunStatus.COMPLETED + assert state.step_results["attempt"]["output"]["stdout"] == "done" + assert counter_file.read_text(encoding="utf-8").strip() == "2" + + def test_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): + """While loop must still run to max_iterations when the condition + never becomes false — copy-back must not break this path. + + See https://github.com/github/spec-kit/issues/2592 + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + from specify_cli.workflows.base import RunStatus + + counter_file = project_dir / ".counter" + counter_file.write_text("0", encoding="utf-8") + + yaml_str = f""" +schema_version: "1.0" +workflow: + id: "while-max-iterations" + name: "While Max Iterations" + version: "1.0.0" +steps: + - id: retry-loop + type: while + condition: "{{{{ 'done' not in steps.tick.output.stdout }}}}" + max_iterations: 3 + steps: + - id: tick + type: shell + run: | + n=$(cat {counter_file}) + n=$((n + 1)) + printf '%s' $n > {counter_file} + printf 'pending' +""" + definition = WorkflowDefinition.from_string(yaml_str) + engine = WorkflowEngine(project_dir) + state = engine.execute(definition) + + assert state.status == RunStatus.COMPLETED + # All 3 iterations ran (iteration 0 + 2 loop iterations). + assert counter_file.read_text(encoding="utf-8").strip() == "3" + # Unprefixed key holds the last iteration's result. + assert state.step_results["tick"]["output"]["stdout"] == "pending" + # Namespaced keys for loop iterations 1 and 2 exist. + assert "retry-loop:tick:1" in state.step_results + assert "retry-loop:tick:2" in state.step_results + + def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): + """Do-while loop must still run to max_iterations when the condition + never becomes false. + + See https://github.com/github/spec-kit/issues/2592 + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + from specify_cli.workflows.base import RunStatus + + counter_file = project_dir / ".counter" + counter_file.write_text("0", encoding="utf-8") + + yaml_str = f""" +schema_version: "1.0" +workflow: + id: "do-while-max-iterations" + name: "Do While Max Iterations" + version: "1.0.0" +steps: + - id: retry-loop + type: do-while + condition: "{{{{ 'done' not in steps.tick.output.stdout }}}}" + max_iterations: 3 + steps: + - id: tick + type: shell + run: | + n=$(cat {counter_file}) + n=$((n + 1)) + printf '%s' $n > {counter_file} + printf 'pending' +""" + definition = WorkflowDefinition.from_string(yaml_str) + engine = WorkflowEngine(project_dir) + state = engine.execute(definition) + + assert state.status == RunStatus.COMPLETED + assert counter_file.read_text(encoding="utf-8").strip() == "3" + assert state.step_results["tick"]["output"]["stdout"] == "pending" + # ===== State Persistence Tests ===== From 093d248e4bdbb7db05efbca14bc4c884de88494d Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 09:25:22 -0500 Subject: [PATCH 2/8] address review: cross-platform tests, preserve iteration-0 history - Rewrite shell scripts in tests to use Python via script files instead of POSIX syntax, so they pass on Windows CI. - Snapshot iteration-0 nested-step results under a namespaced key (parent:child:0) before the first copy-back overwrite, preserving complete per-iteration history for debugging. --- src/specify_cli/workflows/engine.py | 9 ++++ tests/test_workflows.py | 69 ++++++++++++++++++++--------- 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 0e5223ad17..2feaaef704 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -672,6 +672,15 @@ def _execute_steps( for _loop_iter in range(max_iters - 1): if not evaluate_condition(condition, context): break + # Snapshot iteration-0 results under a + # namespaced key before the first overwrite. + if _loop_iter == 0: + for ns in result.next_steps: + orig = ns.get("id") + if orig and orig in context.steps: + ns_key = f"{step_id}:{orig}:0" + context.steps[ns_key] = context.steps[orig] + state.step_results[ns_key] = context.steps[orig] # Namespace nested step IDs per iteration iter_steps = [] original_ids = {} diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 616a681ee0..d8b2304ae4 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1907,8 +1907,18 @@ def test_while_loop_condition_reads_latest_iteration(self, project_dir): # Iteration 1: counter=2, echoes "done" → condition false → stop # Without the fix, condition always reads iteration-0 stdout, # so the loop runs all max_iterations. + import sys + counter_file = project_dir / ".counter" counter_file.write_text("0", encoding="utf-8") + py = sys.executable + script_file = project_dir / "_tick.py" + script_file.write_text( + f"import pathlib; p = pathlib.Path(r'{counter_file}')\n" + "n = int(p.read_text()) + 1; p.write_text(str(n))\n" + "print('done' if n >= 2 else str(n), end='')\n", + encoding="utf-8", + ) yaml_str = f""" schema_version: "1.0" @@ -1924,11 +1934,7 @@ def test_while_loop_condition_reads_latest_iteration(self, project_dir): steps: - id: attempt type: shell - run: | - n=$(cat {counter_file}) - n=$((n + 1)) - printf '%s' $n > {counter_file} - if [ "$n" -ge 2 ]; then printf done; else printf $n; fi + run: {py} {script_file} """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) @@ -1939,6 +1945,8 @@ def test_while_loop_condition_reads_latest_iteration(self, project_dir): assert state.step_results["attempt"]["output"]["stdout"] == "done" # Namespaced iteration-1 result should also exist. assert "retry-loop:attempt:1" in state.step_results + # Iteration-0 history preserved under namespaced key. + assert "retry-loop:attempt:0" in state.step_results # Counter should be 2 (iteration 0 + iteration 1), not 5. assert counter_file.read_text(encoding="utf-8").strip() == "2" @@ -1950,8 +1958,18 @@ def test_do_while_loop_condition_reads_latest_iteration(self, project_dir): from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition from specify_cli.workflows.base import RunStatus + import sys + counter_file = project_dir / ".counter" counter_file.write_text("0", encoding="utf-8") + py = sys.executable + script_file = project_dir / "_tick.py" + script_file.write_text( + f"import pathlib; p = pathlib.Path(r'{counter_file}')\n" + "n = int(p.read_text()) + 1; p.write_text(str(n))\n" + "print('done' if n >= 2 else str(n), end='')\n", + encoding="utf-8", + ) yaml_str = f""" schema_version: "1.0" @@ -1967,11 +1985,7 @@ def test_do_while_loop_condition_reads_latest_iteration(self, project_dir): steps: - id: attempt type: shell - run: | - n=$(cat {counter_file}) - n=$((n + 1)) - printf '%s' $n > {counter_file} - if [ "$n" -ge 2 ]; then printf done; else printf $n; fi + run: {py} {script_file} """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) @@ -1990,8 +2004,18 @@ def test_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition from specify_cli.workflows.base import RunStatus + import sys + counter_file = project_dir / ".counter" counter_file.write_text("0", encoding="utf-8") + py = sys.executable + script_file = project_dir / "_tick.py" + script_file.write_text( + f"import pathlib; p = pathlib.Path(r'{counter_file}')\n" + "n = int(p.read_text()) + 1; p.write_text(str(n))\n" + "print('pending', end='')\n", + encoding="utf-8", + ) yaml_str = f""" schema_version: "1.0" @@ -2007,11 +2031,7 @@ def test_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): steps: - id: tick type: shell - run: | - n=$(cat {counter_file}) - n=$((n + 1)) - printf '%s' $n > {counter_file} - printf 'pending' + run: {py} {script_file} """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) @@ -2022,7 +2042,8 @@ def test_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): assert counter_file.read_text(encoding="utf-8").strip() == "3" # Unprefixed key holds the last iteration's result. assert state.step_results["tick"]["output"]["stdout"] == "pending" - # Namespaced keys for loop iterations 1 and 2 exist. + # Namespaced keys for all iterations exist. + assert "retry-loop:tick:0" in state.step_results assert "retry-loop:tick:1" in state.step_results assert "retry-loop:tick:2" in state.step_results @@ -2035,8 +2056,18 @@ def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition from specify_cli.workflows.base import RunStatus + import sys + counter_file = project_dir / ".counter" counter_file.write_text("0", encoding="utf-8") + py = sys.executable + script_file = project_dir / "_tick.py" + script_file.write_text( + f"import pathlib; p = pathlib.Path(r'{counter_file}')\n" + "n = int(p.read_text()) + 1; p.write_text(str(n))\n" + "print('pending', end='')\n", + encoding="utf-8", + ) yaml_str = f""" schema_version: "1.0" @@ -2052,11 +2083,7 @@ def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): steps: - id: tick type: shell - run: | - n=$(cat {counter_file}) - n=$((n + 1)) - printf '%s' $n > {counter_file} - printf 'pending' + run: {py} {script_file} """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) From f8019eba84082689eaef81be50ede1fd8d559797 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 09:36:36 -0500 Subject: [PATCH 3/8] address review: skip copy-back on paused/failed iterations Move the status check before the copy-back so that partial results from paused or failed nested steps (e.g., a gate awaiting input) do not overwrite the unprefixed key. This preserves correct resume behavior. --- src/specify_cli/workflows/engine.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 2feaaef704..facbbc708f 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -695,18 +695,21 @@ def _execute_steps( iter_steps, context, state, registry, step_offset=-1, ) - # Copy namespaced results back to unprefixed - # keys so loop conditions see updated values. - for namespaced, orig in original_ids.items(): - if namespaced in context.steps: - context.steps[orig] = context.steps[namespaced] - state.step_results[orig] = context.steps[namespaced] if state.status in ( RunStatus.PAUSED, RunStatus.FAILED, RunStatus.ABORTED, ): return + # Copy namespaced results back to unprefixed + # keys so loop conditions see updated values. + # Only after a fully completed iteration — + # partial results from paused/failed steps + # must not overwrite the unprefixed key. + for namespaced, orig in original_ids.items(): + if namespaced in context.steps: + context.steps[orig] = context.steps[namespaced] + state.step_results[orig] = context.steps[namespaced] # Fan-out: execute nested step template per item with unique IDs if step_type == "fan-out": From 8b24011fd8d0d5061be3ff662d60c843f81e5925 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 09:46:18 -0500 Subject: [PATCH 4/8] address review: quote paths in test shell commands Quote both the Python executable and script file paths in the run: commands to handle spaces in paths on Windows. --- tests/test_workflows.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d8b2304ae4..6fae039c46 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1934,7 +1934,7 @@ def test_while_loop_condition_reads_latest_iteration(self, project_dir): steps: - id: attempt type: shell - run: {py} {script_file} + run: '"{py}" "{script_file}"' """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) @@ -1985,7 +1985,7 @@ def test_do_while_loop_condition_reads_latest_iteration(self, project_dir): steps: - id: attempt type: shell - run: {py} {script_file} + run: '"{py}" "{script_file}"' """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) @@ -2031,7 +2031,7 @@ def test_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): steps: - id: tick type: shell - run: {py} {script_file} + run: '"{py}" "{script_file}"' """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) @@ -2083,7 +2083,7 @@ def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): steps: - id: tick type: shell - run: {py} {script_file} + run: '"{py}" "{script_file}"' """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) From 0df91825789c9242374229b388f7f8f8bbabbcc9 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 09:58:11 -0500 Subject: [PATCH 5/8] address review: execute loop body with original IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of namespacing step IDs for execution and copying results back, execute the loop body with original (unprefixed) step IDs so results naturally land at the right keys. Snapshot previous iteration results to namespaced keys (parent:child:N) for history only. This fixes multi-step loop bodies where step B references step A's output within the same iteration — previously step B would see stale data until the copy-back ran after the entire iteration. --- src/specify_cli/workflows/engine.py | 42 ++++++++++------------------- tests/test_workflows.py | 6 ++--- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index facbbc708f..24a0a37702 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -672,27 +672,22 @@ def _execute_steps( for _loop_iter in range(max_iters - 1): if not evaluate_condition(condition, context): break - # Snapshot iteration-0 results under a - # namespaced key before the first overwrite. - if _loop_iter == 0: - for ns in result.next_steps: - orig = ns.get("id") - if orig and orig in context.steps: - ns_key = f"{step_id}:{orig}:0" - context.steps[ns_key] = context.steps[orig] - state.step_results[ns_key] = context.steps[orig] - # Namespace nested step IDs per iteration - iter_steps = [] - original_ids = {} + # Snapshot current results under namespaced + # keys for per-iteration history before they + # are overwritten by the next iteration. for ns in result.next_steps: - ns_copy = dict(ns) - if "id" in ns_copy: - orig = ns_copy["id"] - ns_copy["id"] = f"{step_id}:{orig}:{_loop_iter + 1}" - original_ids[ns_copy["id"]] = orig - iter_steps.append(ns_copy) + orig = ns.get("id") + if orig and orig in context.steps: + ns_key = f"{step_id}:{orig}:{_loop_iter}" + context.steps[ns_key] = context.steps[orig] + state.step_results[ns_key] = context.steps[orig] + # Execute body with original step IDs so + # results land at the unprefixed keys. Both + # inter-step references within the body and + # the loop condition naturally see the latest + # values without a copy-back. self._execute_steps( - iter_steps, context, state, registry, + result.next_steps, context, state, registry, step_offset=-1, ) if state.status in ( @@ -701,15 +696,6 @@ def _execute_steps( RunStatus.ABORTED, ): return - # Copy namespaced results back to unprefixed - # keys so loop conditions see updated values. - # Only after a fully completed iteration — - # partial results from paused/failed steps - # must not overwrite the unprefixed key. - for namespaced, orig in original_ids.items(): - if namespaced in context.steps: - context.steps[orig] = context.steps[namespaced] - state.step_results[orig] = context.steps[namespaced] # Fan-out: execute nested step template per item with unique IDs if step_type == "fan-out": diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 6fae039c46..9ea063f058 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1943,10 +1943,9 @@ def test_while_loop_condition_reads_latest_iteration(self, project_dir): assert state.status == RunStatus.COMPLETED # The unprefixed key should reflect the latest iteration's result. assert state.step_results["attempt"]["output"]["stdout"] == "done" - # Namespaced iteration-1 result should also exist. - assert "retry-loop:attempt:1" in state.step_results # Iteration-0 history preserved under namespaced key. assert "retry-loop:attempt:0" in state.step_results + assert state.step_results["retry-loop:attempt:0"]["output"]["stdout"] != "done" # Counter should be 2 (iteration 0 + iteration 1), not 5. assert counter_file.read_text(encoding="utf-8").strip() == "2" @@ -2042,10 +2041,9 @@ def test_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): assert counter_file.read_text(encoding="utf-8").strip() == "3" # Unprefixed key holds the last iteration's result. assert state.step_results["tick"]["output"]["stdout"] == "pending" - # Namespaced keys for all iterations exist. + # Namespaced history keys for previous iterations exist. assert "retry-loop:tick:0" in state.step_results assert "retry-loop:tick:1" in state.step_results - assert "retry-loop:tick:2" in state.step_results def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): """Do-while loop must still run to max_iterations when the condition From 9d0a2226bb144a807abf69238677d8b72e0730a5 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 10:11:19 -0500 Subject: [PATCH 6/8] address review: namespaced execution with per-step copy-back Revert to namespaced step IDs for execution (preserving unique log entries and state keys per iteration) but copy each step's result back to the unprefixed key immediately after it completes. This preserves backward compatibility (same namespaced key format, same log IDs) while fixing both the condition evaluation bug and inter-step references within multi-step loop bodies. --- src/specify_cli/workflows/engine.py | 46 ++++++++++++++--------------- tests/test_workflows.py | 9 +++--- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 24a0a37702..2192993f99 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -672,30 +672,30 @@ def _execute_steps( for _loop_iter in range(max_iters - 1): if not evaluate_condition(condition, context): break - # Snapshot current results under namespaced - # keys for per-iteration history before they - # are overwritten by the next iteration. + # Namespace nested step IDs per iteration + # so logs and state keys are unique. + # Execute one step at a time and alias each + # result back to the unprefixed key so that + # later steps in the same body and the loop + # condition see the latest values. for ns in result.next_steps: - orig = ns.get("id") - if orig and orig in context.steps: - ns_key = f"{step_id}:{orig}:{_loop_iter}" - context.steps[ns_key] = context.steps[orig] - state.step_results[ns_key] = context.steps[orig] - # Execute body with original step IDs so - # results land at the unprefixed keys. Both - # inter-step references within the body and - # the loop condition naturally see the latest - # values without a copy-back. - self._execute_steps( - result.next_steps, context, state, registry, - step_offset=-1, - ) - if state.status in ( - RunStatus.PAUSED, - RunStatus.FAILED, - RunStatus.ABORTED, - ): - return + ns_copy = dict(ns) + orig = ns_copy.get("id") + if orig: + ns_copy["id"] = f"{step_id}:{orig}:{_loop_iter + 1}" + self._execute_steps( + [ns_copy], context, state, registry, + step_offset=-1, + ) + if orig and ns_copy["id"] in context.steps: + context.steps[orig] = context.steps[ns_copy["id"]] + state.step_results[orig] = context.steps[ns_copy["id"]] + if state.status in ( + RunStatus.PAUSED, + RunStatus.FAILED, + RunStatus.ABORTED, + ): + return # Fan-out: execute nested step template per item with unique IDs if step_type == "fan-out": diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 9ea063f058..6f68556ffc 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1943,9 +1943,8 @@ def test_while_loop_condition_reads_latest_iteration(self, project_dir): assert state.status == RunStatus.COMPLETED # The unprefixed key should reflect the latest iteration's result. assert state.step_results["attempt"]["output"]["stdout"] == "done" - # Iteration-0 history preserved under namespaced key. - assert "retry-loop:attempt:0" in state.step_results - assert state.step_results["retry-loop:attempt:0"]["output"]["stdout"] != "done" + # Namespaced iteration-1 result should also exist. + assert "retry-loop:attempt:1" in state.step_results # Counter should be 2 (iteration 0 + iteration 1), not 5. assert counter_file.read_text(encoding="utf-8").strip() == "2" @@ -2041,9 +2040,9 @@ def test_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): assert counter_file.read_text(encoding="utf-8").strip() == "3" # Unprefixed key holds the last iteration's result. assert state.step_results["tick"]["output"]["stdout"] == "pending" - # Namespaced history keys for previous iterations exist. - assert "retry-loop:tick:0" in state.step_results + # Namespaced keys for loop iterations exist. assert "retry-loop:tick:1" in state.step_results + assert "retry-loop:tick:2" in state.step_results def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): """Do-while loop must still run to max_iterations when the condition From d4af331d53f9f6fb590907a7b76c00855ee4618b Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 11:43:59 -0500 Subject: [PATCH 7/8] address review: alias after status check, add multi-step body test - Move per-step aliasing below the PAUSED/FAILED/ABORTED status check so partial results from incomplete steps are not aliased back to the unprefixed key. - Add test_while_loop_multi_step_body_inter_step_refs to exercise a multi-step loop body where step B reads step A's output within the same iteration, verifying per-step aliasing works correctly. Addresses feedback from @doquanghuy (items 2 & 4) and Copilot review on commit 9d0a222. --- src/specify_cli/workflows/engine.py | 6 +-- tests/test_workflows.py | 68 +++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 2192993f99..8477567b3f 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -687,15 +687,15 @@ def _execute_steps( [ns_copy], context, state, registry, step_offset=-1, ) - if orig and ns_copy["id"] in context.steps: - context.steps[orig] = context.steps[ns_copy["id"]] - state.step_results[orig] = context.steps[ns_copy["id"]] if state.status in ( RunStatus.PAUSED, RunStatus.FAILED, RunStatus.ABORTED, ): return + if orig and ns_copy["id"] in context.steps: + context.steps[orig] = context.steps[ns_copy["id"]] + state.step_results[orig] = context.steps[ns_copy["id"]] # Fan-out: execute nested step template per item with unique IDs if step_type == "fan-out": diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 6f68556ffc..a65b431877 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2090,6 +2090,74 @@ def test_do_while_loop_runs_to_max_when_condition_stays_true(self, project_dir): assert counter_file.read_text(encoding="utf-8").strip() == "3" assert state.step_results["tick"]["output"]["stdout"] == "pending" + def test_while_loop_multi_step_body_inter_step_refs(self, project_dir): + """Multi-step loop body: step B must see step A's output from the + current iteration, not a stale previous one. + + See https://github.com/github/spec-kit/issues/2592 + """ + from specify_cli.workflows.engine import WorkflowEngine, WorkflowDefinition + from specify_cli.workflows.base import RunStatus + + import sys + + counter_file = project_dir / ".counter" + counter_file.write_text("0", encoding="utf-8") + py = sys.executable + + # Step A: increments counter file, echoes the value. + step_a_file = project_dir / "_step_a.py" + step_a_file.write_text( + f"import pathlib; p = pathlib.Path(r'{counter_file}')\n" + "n = int(p.read_text()) + 1; p.write_text(str(n))\n" + "print(str(n), end='')\n", + encoding="utf-8", + ) + + # Step B: reads step A's stdout from a marker file written by + # the test harness (since shell steps can't read context). + # Instead, step B just echoes its own value — we verify via + # the engine that step B's namespaced result was aliased back. + step_b_file = project_dir / "_step_b.py" + step_b_file.write_text( + f"import pathlib; p = pathlib.Path(r'{counter_file}')\n" + "print('b-saw-' + p.read_text().strip(), end='')\n", + encoding="utf-8", + ) + + yaml_str = f""" +schema_version: "1.0" +workflow: + id: "while-multi-step" + name: "While Multi Step" + version: "1.0.0" +steps: + - id: retry-loop + type: while + condition: "{{{{ 'done' not in steps.step-a.output.stdout }}}}" + max_iterations: 3 + steps: + - id: step-a + type: shell + run: '"{py}" "{step_a_file}"' + - id: step-b + type: shell + run: '"{py}" "{step_b_file}"' +""" + definition = WorkflowDefinition.from_string(yaml_str) + engine = WorkflowEngine(project_dir) + state = engine.execute(definition) + + assert state.status == RunStatus.COMPLETED + # Both unprefixed keys reflect the latest iteration's results. + assert state.step_results["step-a"]["output"]["stdout"] == "3" + assert state.step_results["step-b"]["output"]["stdout"] == "b-saw-3" + # Namespaced keys exist for loop iterations. + assert "retry-loop:step-a:1" in state.step_results + assert "retry-loop:step-b:1" in state.step_results + assert "retry-loop:step-a:2" in state.step_results + assert "retry-loop:step-b:2" in state.step_results + # ===== State Persistence Tests ===== From 5f0692f0ab5aa8d269d8ec4c30a64e763ccff151 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 21 May 2026 11:51:56 -0500 Subject: [PATCH 8/8] address review: stable fallback IDs, expression-based inter-step test - Use enumerate() for stable fallback IDs when loop body steps lack an explicit id (step-0, step-1, etc. instead of always step-0). - Rewrite multi-step body test so step B uses expression substitution ({{ steps.step-a.output.stdout }}) instead of reading the counter file directly, making it a true regression test for per-step aliasing. --- src/specify_cli/workflows/engine.py | 6 +++--- tests/test_workflows.py | 19 ++++++------------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 8477567b3f..65d9b3a272 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -678,11 +678,11 @@ def _execute_steps( # result back to the unprefixed key so that # later steps in the same body and the loop # condition see the latest values. - for ns in result.next_steps: + for ns_idx, ns in enumerate(result.next_steps): ns_copy = dict(ns) orig = ns_copy.get("id") - if orig: - ns_copy["id"] = f"{step_id}:{orig}:{_loop_iter + 1}" + base_id = orig or f"step-{ns_idx}" + ns_copy["id"] = f"{step_id}:{base_id}:{_loop_iter + 1}" self._execute_steps( [ns_copy], context, state, registry, step_offset=-1, diff --git a/tests/test_workflows.py b/tests/test_workflows.py index a65b431877..f340e9c0dc 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2114,17 +2114,9 @@ def test_while_loop_multi_step_body_inter_step_refs(self, project_dir): encoding="utf-8", ) - # Step B: reads step A's stdout from a marker file written by - # the test harness (since shell steps can't read context). - # Instead, step B just echoes its own value — we verify via - # the engine that step B's namespaced result was aliased back. - step_b_file = project_dir / "_step_b.py" - step_b_file.write_text( - f"import pathlib; p = pathlib.Path(r'{counter_file}')\n" - "print('b-saw-' + p.read_text().strip(), end='')\n", - encoding="utf-8", - ) - + # Step B uses {{ steps.step-a.output.stdout }} expression + # substitution in its run command so the engine resolves the + # aliased unprefixed key — this is the real inter-step test. yaml_str = f""" schema_version: "1.0" workflow: @@ -2142,7 +2134,7 @@ def test_while_loop_multi_step_body_inter_step_refs(self, project_dir): run: '"{py}" "{step_a_file}"' - id: step-b type: shell - run: '"{py}" "{step_b_file}"' + run: "echo b-saw-{{{{ steps.step-a.output.stdout }}}}" """ definition = WorkflowDefinition.from_string(yaml_str) engine = WorkflowEngine(project_dir) @@ -2151,7 +2143,8 @@ def test_while_loop_multi_step_body_inter_step_refs(self, project_dir): assert state.status == RunStatus.COMPLETED # Both unprefixed keys reflect the latest iteration's results. assert state.step_results["step-a"]["output"]["stdout"] == "3" - assert state.step_results["step-b"]["output"]["stdout"] == "b-saw-3" + # Step B saw step A's output via expression substitution. + assert "b-saw-3" in state.step_results["step-b"]["output"]["stdout"] # Namespaced keys exist for loop iterations. assert "retry-loop:step-a:1" in state.step_results assert "retry-loop:step-b:1" in state.step_results