From 552cc8873509f82d070282590e9247f67b3f906e Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 15 Jul 2026 10:28:27 +0500 Subject: [PATCH 1/3] fix(workflows): if step execute() fails cleanly on a non-list branch IfThenStep.execute returns config['then']/['else'] directly as StepResult.next_steps with no check that it is a list. The engine does not auto-validate step config before execute(), so an unvalidated run with a non-list branch (a scalar or mapping authoring mistake) passes a non-iterable as next_steps and crashes the whole run. validate() catches this, but execute() should fail the step loudly instead. Add an isinstance(list) guard returning a FAILED StepResult, completing the same guard #3519 added to the while/do-while steps (and that its comment already references for the 'if' step) and mirroring the switch step's 'cases' guard. Parametrized test feeds a str/dict/int branch and asserts FAILED (fails before: the non-iterable next_steps crashed execution). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflows/steps/if_then/__init__.py | 22 +++++++++++++++++ tests/test_workflows.py | 24 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index b2ed880678..8f13f8479f 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -24,9 +24,31 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: if result: branch_name = "then" branch = config.get("then", []) + branch_name = "then" else: branch_name = "else" branch = config.get("else", []) + branch_name = "else" + + if not isinstance(branch, list): + # The engine does not auto-validate step config and feeds + # ``next_steps`` straight into ``_execute_steps``, which iterates them + # as step mappings. A non-list ``then``/``else`` (a single mapping or + # scalar authoring mistake) would otherwise be iterated element-wise + # — a dict yields its keys, a str its characters — and crash the whole + # run with AttributeError on ``.get()``. ``validate`` already rejects + # a non-list branch; fail this step loudly on an unvalidated run + # instead, mirroring the while/do-while (#3519) / switch / fan-out + # steps. The selected branch is always returned as next_steps, so the + # guard is unconditional. + return StepResult( + status=StepStatus.FAILED, + error=( + f"If step {config.get('id', '?')!r}: {branch_name!r} must be a " + f"list of steps, got {type(branch).__name__}." + ), + output={"condition_result": result}, + ) # The engine does not auto-validate step config (see # ``WorkflowEngine.load_workflow``), and it feeds ``next_steps`` straight diff --git a/tests/test_workflows.py b/tests/test_workflows.py index b1d9841535..8a1acaa038 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2130,6 +2130,30 @@ def test_execute_else_branch(self): assert result.output["condition_result"] is False assert result.next_steps[0]["id"] == "b" + @pytest.mark.parametrize("bad_branch", ["oops", {"a": 1}, 42]) + def test_execute_rejects_non_list_branch(self, bad_branch): + """execute() fails cleanly on a non-list then/else branch. The engine + doesn't auto-validate step config, so a non-iterable next_steps would + otherwise crash the run — completing the while/do-while guard (#3519) for + the if step (mirrors the switch step's 'cases' guard).""" + from specify_cli.workflows.steps.if_then import IfThenStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = IfThenStep() + cond = "{{ inputs.scope == 'full' }}" + res_then = step.execute( + {"id": "i", "condition": cond, "then": bad_branch}, + StepContext(inputs={"scope": "full"}), + ) + assert res_then.status is StepStatus.FAILED + assert "'then' must be a list" in (res_then.error or "") + res_else = step.execute( + {"id": "i", "condition": cond, "then": [], "else": bad_branch}, + StepContext(inputs={"scope": "backend"}), + ) + assert res_else.status is StepStatus.FAILED + assert "'else' must be a list" in (res_else.error or "") + def test_validate_missing_condition(self): from specify_cli.workflows.steps.if_then import IfThenStep From c8a37b0530e4b1d6d18a0a6cd6f7fe1e02a8d371 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 17 Jul 2026 15:29:01 +0500 Subject: [PATCH 2/3] fix(workflows): if step honors else: null (no else branch), not FAILED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The non-list guard turned a valid 'else: null' into a FAILED result on a false condition — but validate() deliberately accepts None as 'no else branch' (test_validate_accepts_valid_else). Normalize a selected None branch to [] before the guard so validation and execution stay consistent; the guard still rejects genuine non-list values (str/dict/int). Test: false condition + else: null now COMPLETES with no next steps (fails before: FAILED 'else must be a list ... NoneType'). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflows/steps/if_then/__init__.py | 8 ++++++-- tests/test_workflows.py | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index 8f13f8479f..9c889a9eb8 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -22,14 +22,18 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: result = evaluate_condition(condition, context) if result: - branch_name = "then" branch = config.get("then", []) branch_name = "then" else: - branch_name = "else" branch = config.get("else", []) branch_name = "else" + # ``else: null`` is the supported "no else branch" form (validate() + # accepts None here); normalize a selected None branch to [] so a false + # condition on such a config runs cleanly instead of hitting the guard. + if branch is None: + branch = [] + if not isinstance(branch, list): # The engine does not auto-validate step config and feeds # ``next_steps`` straight into ``_execute_steps``, which iterates them diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 8a1acaa038..b7ace0b3d3 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2154,6 +2154,23 @@ def test_execute_rejects_non_list_branch(self, bad_branch): assert res_else.status is StepStatus.FAILED assert "'else' must be a list" in (res_else.error or "") + def test_execute_allows_null_else_branch(self): + """`else: null` is the supported 'no else branch' form (validate accepts + it); a false condition must run cleanly (COMPLETED, no next steps), not + be turned into FAILED by the non-list guard.""" + from specify_cli.workflows.steps.if_then import IfThenStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = IfThenStep() + result = step.execute( + {"id": "i", "condition": "{{ inputs.scope == 'full' }}", + "then": [{"id": "a", "command": "speckit.tasks"}], "else": None}, + StepContext(inputs={"scope": "backend"}), + ) + assert result.status is StepStatus.COMPLETED + assert result.output["condition_result"] is False + assert result.next_steps == [] + def test_validate_missing_condition(self): from specify_cli.workflows.steps.if_then import IfThenStep From e84793106a08ed08d302367bea15852c7133273a Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sun, 19 Jul 2026 01:47:43 +0500 Subject: [PATCH 3/3] fix(workflows): de-duplicate if-step guard; normalize only else: null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rebase left two copies of the non-list branch guard in IfThenStep.execute: an earlier block that normalized ANY selected None branch to [] (so a true condition with then: null silently completed instead of failing — validate() rejects a null then) plus a redundant non-list return, followed by the correct branch-specific block. Remove the duplicate earlier block and keep the single guard that normalizes None only for the else branch and rejects every other non-list value. Tests: else: null still COMPLETES (no next steps); then: null now FAILS (not silently normalized). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflows/steps/if_then/__init__.py | 26 ------------------- tests/test_workflows.py | 15 +++++++++++ 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index 9c889a9eb8..d457e1d953 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -28,32 +28,6 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: branch = config.get("else", []) branch_name = "else" - # ``else: null`` is the supported "no else branch" form (validate() - # accepts None here); normalize a selected None branch to [] so a false - # condition on such a config runs cleanly instead of hitting the guard. - if branch is None: - branch = [] - - if not isinstance(branch, list): - # The engine does not auto-validate step config and feeds - # ``next_steps`` straight into ``_execute_steps``, which iterates them - # as step mappings. A non-list ``then``/``else`` (a single mapping or - # scalar authoring mistake) would otherwise be iterated element-wise - # — a dict yields its keys, a str its characters — and crash the whole - # run with AttributeError on ``.get()``. ``validate`` already rejects - # a non-list branch; fail this step loudly on an unvalidated run - # instead, mirroring the while/do-while (#3519) / switch / fan-out - # steps. The selected branch is always returned as next_steps, so the - # guard is unconditional. - return StepResult( - status=StepStatus.FAILED, - error=( - f"If step {config.get('id', '?')!r}: {branch_name!r} must be a " - f"list of steps, got {type(branch).__name__}." - ), - output={"condition_result": result}, - ) - # The engine does not auto-validate step config (see # ``WorkflowEngine.load_workflow``), and it feeds ``next_steps`` straight # into ``_execute_steps`` which iterates them as step mappings. A diff --git a/tests/test_workflows.py b/tests/test_workflows.py index b7ace0b3d3..75db91458d 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2171,6 +2171,21 @@ def test_execute_allows_null_else_branch(self): assert result.output["condition_result"] is False assert result.next_steps == [] + def test_execute_null_then_is_failed_not_normalized(self): + """Only `else: null` is normalized to []; `then: null` is invalid + (validate() rejects it), so a true condition must FAIL, not silently + complete an empty branch.""" + from specify_cli.workflows.steps.if_then import IfThenStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = IfThenStep() + result = step.execute( + {"id": "i", "condition": "{{ inputs.scope == 'full' }}", "then": None}, + StepContext(inputs={"scope": "full"}), + ) + assert result.status is StepStatus.FAILED + assert "'then' must be" in (result.error or "") + def test_validate_missing_condition(self): from specify_cli.workflows.steps.if_then import IfThenStep