From a98cf01d605fe82450126a6034c5d68ca3631f44 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Tue, 14 Jul 2026 20:53:10 +0500 Subject: [PATCH 1/5] fix(workflows): fail if/switch steps on non-list branch instead of crashing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `IfThenStep.validate()` and `SwitchStep.validate()` already reject a non-list branch (`then`/`else`, and `case`/`default`), but the engine's `execute()` path does not auto-validate (see `WorkflowEngine.load_workflow`, whose docstring notes the definition is "not yet validated"). On an unvalidated run, the selected branch is fed straight into `next_steps`, which `_execute_steps` iterates as step mappings. A non-list branch — a single mapping or scalar authoring mistake — was iterated element-wise (a dict yields its string keys, a str its characters) and raised `AttributeError` on `.get()`, taking down the whole run; the engine invokes `step_impl.execute()` with no surrounding try/except. Guard both `execute` paths to return a FAILED StepResult naming the type error instead, mirroring the switch non-mapping `cases` and fan-out non-list `items` handling. The switch guard is factored into a shared `_non_list_branch_failure` helper covering both `case` and `default` branches. A missing `else`/`default` still defaults to an empty list (COMPLETED), unchanged; the guard fires only on an explicit non-list value. The condition/expression is still evaluated first, so its result is surfaced in the step output for downstream context. Co-authored-by: Claude Opus 4.8 (1M context) --- .../workflows/steps/if_then/__init__.py | 21 +++++++++ .../workflows/steps/switch/__init__.py | 31 +++++++++++++ tests/test_workflows.py | 45 +++++++++++++++++++ 3 files changed, 97 insertions(+) diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index e7179a418a..c7c150e171 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -22,10 +22,31 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: result = evaluate_condition(condition, context) if result: + branch_name = "then" branch = config.get("then", []) else: + branch_name = "else" branch = config.get("else", []) + # 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 + # non-list branch (a single mapping or scalar authoring mistake) would + # otherwise be iterated element-wise — a dict yields its string 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 switch/fan-out + # steps. A missing ``else`` defaults to ``[]`` and stays valid. + if not isinstance(branch, list): + return StepResult( + status=StepStatus.FAILED, + output={"condition_result": result}, + error=( + f"If step {config.get('id', '?')!r}: {branch_name!r} must be " + f"a list of steps, got {type(branch).__name__}." + ), + ) + return StepResult( status=StepStatus.COMPLETED, output={"condition_result": result}, diff --git a/src/specify_cli/workflows/steps/switch/__init__.py b/src/specify_cli/workflows/steps/switch/__init__.py index a63b283432..8f05a0d086 100644 --- a/src/specify_cli/workflows/steps/switch/__init__.py +++ b/src/specify_cli/workflows/steps/switch/__init__.py @@ -42,6 +42,10 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ) for case_key, case_steps in cases.items(): if str(case_key) == str_value: + if not isinstance(case_steps, list): + return self._non_list_branch_failure( + config, f"case {str(case_key)!r}", case_steps, value + ) return StepResult( status=StepStatus.COMPLETED, output={"matched_case": str(case_key), "expression_value": value}, @@ -50,12 +54,39 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # Default fallback default_steps = config.get("default", []) + if not isinstance(default_steps, list): + return self._non_list_branch_failure( + config, "'default'", default_steps, value + ) return StepResult( status=StepStatus.COMPLETED, output={"matched_case": "__default__", "expression_value": value}, next_steps=default_steps, ) + @staticmethod + def _non_list_branch_failure( + config: dict[str, Any], branch_label: str, branch: Any, value: Any + ) -> StepResult: + """Fail the step for a non-list branch instead of crashing the run. + + ``validate`` rejects a non-list case/default branch, but the engine does + not auto-validate and feeds ``next_steps`` straight into + ``_execute_steps``, which iterates them as step mappings. A non-list + branch would be iterated element-wise (a dict yields its keys, a str its + characters) and crash the whole run with AttributeError on ``.get()``. + Fail this step loudly on an unvalidated run instead, mirroring the + non-mapping ``cases`` guard above. + """ + return StepResult( + status=StepStatus.FAILED, + output={"matched_case": None, "expression_value": value}, + error=( + f"Switch step {config.get('id', '?')!r}: {branch_label} must be " + f"a list of steps, got {type(branch).__name__}." + ), + ) + def validate(self, config: dict[str, Any]) -> list[str]: errors = super().validate(config) if "expression" not in config: diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 81aec96d3e..d2651a4b1f 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2066,6 +2066,51 @@ def test_validate_missing_condition(self): errors = step.validate({"id": "test", "then": []}) assert any("missing 'condition'" in e for e in errors) + @pytest.mark.parametrize("bad_branch", [{"id": "x"}, "oops", 5]) + def test_execute_non_list_then_fails_loudly(self, bad_branch): + """A non-list ``then`` must fail the step, not crash the run. + + ``validate`` rejects a non-list ``then``, but the engine does not + auto-validate (see ``WorkflowEngine.load_workflow``) and feeds + ``next_steps`` straight into ``_execute_steps``, which iterates them as + step mappings. Before the guard, a non-list ``then`` (a single mapping + or scalar authoring mistake) was iterated element-wise and raised + AttributeError on ``.get()``, taking down the whole run. Mirrors the + switch/fan-out non-list handling. + """ + from specify_cli.workflows.steps.if_then import IfThenStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = IfThenStep() + ctx = StepContext(inputs={}) + result = step.execute( + {"id": "branch", "condition": "true", "then": bad_branch}, ctx + ) + assert result.status == StepStatus.FAILED + assert "'then' must be a list of steps" in (result.error or "") + assert result.next_steps == [] + + @pytest.mark.parametrize("bad_branch", [{"id": "x"}, "oops", 5]) + def test_execute_non_list_else_fails_loudly(self, bad_branch): + """A non-list ``else`` selected at runtime must fail the step, not crash. + + Same asymmetry as ``then``: the ``else`` branch is only reached when the + condition is false, so a non-list ``else`` reaches ``next_steps`` and + would crash the engine's step iteration on an unvalidated run. + """ + from specify_cli.workflows.steps.if_then import IfThenStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = IfThenStep() + ctx = StepContext(inputs={}) + result = step.execute( + {"id": "branch", "condition": "false", "then": [], "else": bad_branch}, + ctx, + ) + assert result.status == StepStatus.FAILED + assert "'else' must be a list of steps" in (result.error or "") + assert result.next_steps == [] + @pytest.mark.parametrize("bad_else", [False, 0, "", {}, 42]) def test_validate_rejects_non_list_else(self, bad_else): """A non-list 'else' must be rejected even when it is falsy. From 3b14f8555fb077d589691535f8fe6c7ba3ca07d1 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Tue, 14 Jul 2026 21:56:14 +0500 Subject: [PATCH 2/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/steps/switch/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/steps/switch/__init__.py b/src/specify_cli/workflows/steps/switch/__init__.py index 8f05a0d086..690df0f19a 100644 --- a/src/specify_cli/workflows/steps/switch/__init__.py +++ b/src/specify_cli/workflows/steps/switch/__init__.py @@ -54,7 +54,9 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # Default fallback default_steps = config.get("default", []) - if not isinstance(default_steps, list): + if default_steps is None: + default_steps = [] + elif not isinstance(default_steps, list): return self._non_list_branch_failure( config, "'default'", default_steps, value ) From b8bc3a90000beec600575bac47b7b3497f9f2ff6 Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Wed, 15 Jul 2026 16:40:49 +0500 Subject: [PATCH 3/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/steps/if_then/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index c7c150e171..39b7800d39 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -37,7 +37,9 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # ``.get()``. ``validate`` already rejects a non-list branch; fail this # step loudly on an unvalidated run instead, mirroring the switch/fan-out # steps. A missing ``else`` defaults to ``[]`` and stays valid. - if not isinstance(branch, list): +if branch is None and branch_name == "else": + branch = [] + elif not isinstance(branch, list): return StepResult( status=StepStatus.FAILED, output={"condition_result": result}, From 3edf77d557659da39b46572ea34485ad61fd20ba Mon Sep 17 00:00:00 2001 From: Noor ul ain Date: Wed, 15 Jul 2026 18:11:11 +0500 Subject: [PATCH 4/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/steps/if_then/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index 39b7800d39..b2ed880678 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -37,7 +37,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # ``.get()``. ``validate`` already rejects a non-list branch; fail this # step loudly on an unvalidated run instead, mirroring the switch/fan-out # steps. A missing ``else`` defaults to ``[]`` and stays valid. -if branch is None and branch_name == "else": + if branch is None and branch_name == "else": branch = [] elif not isinstance(branch, list): return StepResult( From 1048686dfab65b28f4afc09f0240797a9680189f Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Wed, 15 Jul 2026 21:25:09 +0500 Subject: [PATCH 5/5] test(workflows): cover switch non-list branch execute paths Copilot flagged the new switch branch guards as untested: coverage stopped at a non-mapping `cases` container. Add SwitchStep.execute tests for a matched case with a non-list body and a non-list default (dict/str/int), asserting FAILED, the branch-specific error, empty next_steps, and preserved expression_value. Also add explicit `default: null` / `else: null` normalization tests so the validator-approved empty-branch contract cannot regress. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_workflows.py | 103 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index d2651a4b1f..6a9a9eed58 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2111,6 +2111,24 @@ def test_execute_non_list_else_fails_loudly(self, bad_branch): assert "'else' must be a list of steps" in (result.error or "") assert result.next_steps == [] + def test_execute_none_else_stays_empty(self): + """An explicit ``else: null`` selected at runtime stays an empty branch. + + ``validate`` deliberately accepts ``else: None``; the execute guard must + normalize it to an empty branch (COMPLETED) rather than failing a + validator-approved workflow when the condition is false. + """ + from specify_cli.workflows.steps.if_then import IfThenStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = IfThenStep() + ctx = StepContext(inputs={}) + result = step.execute( + {"id": "branch", "condition": "false", "then": [], "else": None}, ctx + ) + assert result.status == StepStatus.COMPLETED + assert result.next_steps == [] + @pytest.mark.parametrize("bad_else", [False, 0, "", {}, 42]) def test_validate_rejects_non_list_else(self, bad_else): """A non-list 'else' must be rejected even when it is falsy. @@ -2244,6 +2262,91 @@ def test_execute_non_dict_cases_fails_loudly(self): # expression is still evaluated, so its value is surfaced for context. assert result.output["expression_value"] == "approve" + @pytest.mark.parametrize("bad_branch", [{"id": "x"}, "oops", 5]) + def test_execute_non_list_matched_case_fails_loudly(self, bad_branch): + """A matched case with a non-list body must fail the step, not crash. + + ``validate`` rejects a non-list case body, but the engine does not + auto-validate (see ``WorkflowEngine.load_workflow``) and feeds the + selected branch straight into ``_execute_steps``, which iterates it as + step mappings. A non-list body (a single mapping or scalar authoring + mistake) would be iterated element-wise and raise AttributeError on + ``.get()``, taking down the whole run. Mirrors the non-mapping + ``cases`` guard. + """ + from specify_cli.workflows.steps.switch import SwitchStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = SwitchStep() + ctx = StepContext(steps={"review": {"output": {"choice": "approve"}}}) + result = step.execute( + { + "id": "route", + "expression": "{{ steps.review.output.choice }}", + "cases": {"approve": bad_branch}, + }, + ctx, + ) + assert result.status == StepStatus.FAILED + assert "case 'approve' must be a list of steps" in (result.error or "") + assert result.next_steps == [] + # expression is still evaluated, so its value is surfaced for context. + assert result.output["expression_value"] == "approve" + + @pytest.mark.parametrize("bad_branch", [{"id": "x"}, "oops", 5]) + def test_execute_non_list_default_fails_loudly(self, bad_branch): + """A non-list ``default`` reached at runtime must fail, not crash. + + Same asymmetry as the case body: ``default`` is only selected when no + case matches, so a non-list ``default`` reaches ``next_steps`` and would + crash the engine's step iteration on an unvalidated run. + """ + from specify_cli.workflows.steps.switch import SwitchStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = SwitchStep() + ctx = StepContext(steps={"review": {"output": {"choice": "other"}}}) + result = step.execute( + { + "id": "route", + "expression": "{{ steps.review.output.choice }}", + "cases": {"approve": [{"id": "plan", "command": "speckit.plan"}]}, + "default": bad_branch, + }, + ctx, + ) + assert result.status == StepStatus.FAILED + assert "'default' must be a list of steps" in (result.error or "") + assert result.next_steps == [] + # expression is still evaluated, so its value is surfaced for context. + assert result.output["expression_value"] == "other" + + @pytest.mark.parametrize("ok_default", [None, [], [{"id": "x", "command": "/y"}]]) + def test_execute_none_default_stays_empty(self, ok_default): + """An explicit ``default: null`` or a list default stays valid. + + ``validate`` deliberately accepts ``default: None``; the execute guard + must normalize it to an empty branch (COMPLETED) rather than failing a + validator-approved workflow. + """ + from specify_cli.workflows.steps.switch import SwitchStep + from specify_cli.workflows.base import StepContext, StepStatus + + step = SwitchStep() + ctx = StepContext(steps={"review": {"output": {"choice": "other"}}}) + result = step.execute( + { + "id": "route", + "expression": "{{ steps.review.output.choice }}", + "cases": {"approve": [{"id": "plan", "command": "speckit.plan"}]}, + "default": ok_default, + }, + ctx, + ) + assert result.status == StepStatus.COMPLETED + assert result.output["matched_case"] == "__default__" + assert result.next_steps == (ok_default or []) + def test_validate_missing_expression(self): from specify_cli.workflows.steps.switch import SwitchStep