From 55c66125f0d5813855246538c1d0010cc276bb64 Mon Sep 17 00:00:00 2001
From: Ali jawwad <33836051+jawwad-ali@users.noreply.github.com>
Date: Mon, 13 Jul 2026 20:14:55 +0500
Subject: [PATCH] fix(workflows): if-step validate accepts falsy non-list else
 (#3264)

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
---
 .../workflows/steps/if_then/__init__.py       |  4 +-
 tests/test_workflows.py                       | 40 +++++++++++++++++++
 2 files changed, 42 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 5b921a31a..e7179a418 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 @@ class IfThenStep(StepBase):
             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 f2f41bce7..2c94966f9 100644
--- a/tests/test_workflows.py
+++ b/tests/test_workflows.py
@@ -1988,6 +1988,46 @@ class TestIfThenStep:
         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):
+        """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()
+        errors = step.validate({"id": "i", "condition": "true", "then": []})
+        assert not any("'else'" in e for e in errors)
+
 
 class TestSwitchStep:
     """Test the switch step type."""