From 1c47ca278fabd1127f2dc59a0986f63941e7e577 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 30 Jun 2026 21:21:38 +0500 Subject: [PATCH 1/2] fix(workflows): if-step validate accepts falsy non-list else IfThenStep.validate() guarded the 'else' branch with 'if else_branch and not isinstance(else_branch, list)'. The leading truthiness check short-circuits for falsy non-list values (False, 0, '', {}), so a malformed else-branch passes validation and is then silently skipped at runtime. The sibling 'then' branch is validated strictly; 'else' now matches by switching to an 'is not None' guard. Co-Authored-By: Claude Opus 4.8 --- .../workflows/steps/if_then/__init__.py | 4 +-- tests/test_workflows.py | 29 +++++++++++++++++++ 2 files changed, 31 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 5b921a31a5..e7179a418a 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -47,8 +47,8 @@ def validate(self, config: dict[str, Any]) -> list[str]: errors.append( f"If step {config.get('id', '?')!r}: 'then' must be a list of steps." ) - else_branch = config.get("else", []) - if else_branch and not isinstance(else_branch, list): + else_branch = config.get("else") + if else_branch is not None and not isinstance(else_branch, list): errors.append( f"If step {config.get('id', '?')!r}: 'else' must be a list of steps." ) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index c2ec4acb4e..f8b351ab68 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1684,6 +1684,35 @@ 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_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. + + The original guard used ``if else_branch and ...`` which + short-circuits for falsy non-list values (False/0/''/{}), letting a + malformed else-branch pass validation only to be silently skipped at + runtime. ``then`` is already strictly validated; ``else`` must match. + """ + from specify_cli.workflows.steps.if_then import IfThenStep + + step = IfThenStep() + errors = step.validate( + {"id": "i", "condition": "true", "then": [], "else": bad_else} + ) + assert any("'else' must be a list of steps" in e for e in errors) + + @pytest.mark.parametrize("ok_else", [None, [], [{"id": "x", "command": "/y"}]]) + def test_validate_accepts_valid_else(self, ok_else): + """A missing/None/list 'else' stays valid.""" + from specify_cli.workflows.steps.if_then import IfThenStep + + step = IfThenStep() + config = {"id": "i", "condition": "true", "then": []} + if ok_else is not None: + config["else"] = ok_else + errors = step.validate(config) + assert not any("'else'" in e for e in errors) + class TestSwitchStep: """Test the switch step type.""" From cb24673b6436eedb18ca80f42a783b59d9c413fb Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Wed, 1 Jul 2026 01:31:37 +0500 Subject: [PATCH 2/2] test(workflows): cover explicit else:None and missing-else separately Per Copilot feedback: the parametrized valid-else test omitted the 'else' key when the value was None, so it covered only the missing-else case, not an explicit 'else: None'. Set 'else' explicitly (including None) in the parametrized test and add a dedicated missing-else test, so both accepted shapes are pinned. Co-Authored-By: Claude Opus 4.8 --- tests/test_workflows.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index f8b351ab68..bd5cf6d30f 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1703,14 +1703,25 @@ def test_validate_rejects_non_list_else(self, bad_else): @pytest.mark.parametrize("ok_else", [None, [], [{"id": "x", "command": "/y"}]]) def test_validate_accepts_valid_else(self, ok_else): - """A missing/None/list 'else' stays valid.""" + """An explicit 'else' of None or a list stays valid. + + ``else`` is set explicitly here (including ``else: None``) so the + explicit-None case is exercised, not just the missing-key case. + """ + from specify_cli.workflows.steps.if_then import IfThenStep + + step = IfThenStep() + errors = step.validate( + {"id": "i", "condition": "true", "then": [], "else": ok_else} + ) + assert not any("'else'" in e for e in errors) + + def test_validate_accepts_missing_else(self): + """A missing 'else' key stays valid (no else branch).""" from specify_cli.workflows.steps.if_then import IfThenStep step = IfThenStep() - config = {"id": "i", "condition": "true", "then": []} - if ok_else is not None: - config["else"] = ok_else - errors = step.validate(config) + errors = step.validate({"id": "i", "condition": "true", "then": []}) assert not any("'else'" in e for e in errors)