From d2182813df9082b75be69f3a005d33a9d71d5463 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 15 Jul 2026 10:28:27 +0500 Subject: [PATCH 1/2] 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 eab4bef540..1d124c2655 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2105,6 +2105,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 5f90f8950c42238a1c9ac93054049208edebf893 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 17 Jul 2026 15:29:01 +0500 Subject: [PATCH 2/2] 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 1d124c2655..ed86fcf352 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2129,6 +2129,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