From 9abf7d73e63146d4e49887e37a9c2731ac1b9740 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 24 Jul 2026 13:22:35 +0500 Subject: [PATCH] fix(workflows): reject non-string 'condition' in if/while/do-while steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `if_then`, `while_loop`, and `do_while` validate() confirm `condition` is present but never that it is a string. execute() feeds it to `evaluate_condition()`, which returns a non-string as-is and takes `bool()` of it -- so `condition: [1, 2]` (a list authoring mistake) silently resolves to `True`, branching wrongly / spinning the loop to `max_iterations`, with no error reported. Reject a present-but-non-string `condition` at validation, mirroring the existing prompt/shell/command 'must be a string' guards. `"true"`/`"false"` and expressions like `"{{ ... }}"` are strings, so they stay valid. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflows/steps/do_while/__init__.py | 12 +++++++ .../workflows/steps/if_then/__init__.py | 12 +++++++ .../workflows/steps/while_loop/__init__.py | 12 +++++++ tests/test_workflows.py | 35 +++++++++++++++++++ 4 files changed, 71 insertions(+) diff --git a/src/specify_cli/workflows/steps/do_while/__init__.py b/src/specify_cli/workflows/steps/do_while/__init__.py index ca6047a57a..d8ae79b324 100644 --- a/src/specify_cli/workflows/steps/do_while/__init__.py +++ b/src/specify_cli/workflows/steps/do_while/__init__.py @@ -70,6 +70,18 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"Do-while step {config.get('id', '?')!r} is missing " f"'condition' field." ) + elif not isinstance(config["condition"], str): + # The engine re-evaluates 'condition' via evaluate_condition() after + # each iteration; it returns a non-string as-is and takes bool() of + # it -- so a list/dict/number condition silently resolves to a + # truthiness (e.g. condition: [1, 2] is always truthy, looping to + # max_iterations) with no error. Reject non-strings at validation, + # mirroring the prompt/shell/command 'must be a string' checks. + # "true"/"false" and an expression like "{{ ... }}" stay valid. + errors.append( + f"Do-while step {config.get('id', '?')!r}: 'condition' must be a " + f"string, got {type(config['condition']).__name__}." + ) max_iter = config.get("max_iterations") if max_iter is not None: # bool is a subclass of int, so isinstance(True, int) is True and diff --git a/src/specify_cli/workflows/steps/if_then/__init__.py b/src/specify_cli/workflows/steps/if_then/__init__.py index b2ed880678..48691285c3 100644 --- a/src/specify_cli/workflows/steps/if_then/__init__.py +++ b/src/specify_cli/workflows/steps/if_then/__init__.py @@ -61,6 +61,18 @@ def validate(self, config: dict[str, Any]) -> list[str]: errors.append( f"If step {config.get('id', '?')!r} is missing 'condition' field." ) + elif not isinstance(config["condition"], str): + # execute() feeds 'condition' to evaluate_condition(), which returns + # a non-string as-is and takes bool() of it -- so a list/dict/number + # condition silently resolves to a truthiness (e.g. condition: [1, 2] + # is always True) with no error, branching wrongly on an authoring + # mistake. Reject non-strings at validation, mirroring the prompt/ + # shell/command 'must be a string' checks. "true"/"false" and an + # expression like "{{ ... }}" are strings, so they stay valid. + errors.append( + f"If step {config.get('id', '?')!r}: 'condition' must be a " + f"string, got {type(config['condition']).__name__}." + ) if "then" not in config: errors.append( f"If step {config.get('id', '?')!r} is missing 'then' field." diff --git a/src/specify_cli/workflows/steps/while_loop/__init__.py b/src/specify_cli/workflows/steps/while_loop/__init__.py index e2dbb19305..0bb592ca1f 100644 --- a/src/specify_cli/workflows/steps/while_loop/__init__.py +++ b/src/specify_cli/workflows/steps/while_loop/__init__.py @@ -79,6 +79,18 @@ def validate(self, config: dict[str, Any]) -> list[str]: f"While step {config.get('id', '?')!r} is missing " f"'condition' field." ) + elif not isinstance(config["condition"], str): + # execute() feeds 'condition' to evaluate_condition(), which returns + # a non-string as-is and takes bool() of it -- so a list/dict/number + # condition silently resolves to a truthiness (e.g. condition: [1, 2] + # is always truthy, spinning the loop to max_iterations) with no + # error. Reject non-strings at validation, mirroring the prompt/ + # shell/command 'must be a string' checks. "true"/"false" and an + # expression like "{{ ... }}" are strings, so they stay valid. + errors.append( + f"While step {config.get('id', '?')!r}: 'condition' must be a " + f"string, got {type(config['condition']).__name__}." + ) max_iter = config.get("max_iterations") if max_iter is not None: # bool is a subclass of int, so isinstance(True, int) is True and diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 1c29ab56e6..e6e110309c 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2490,6 +2490,25 @@ 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", [["a", "b"], {"k": "v"}, 5, True]) + def test_validate_rejects_non_string_condition(self, bad): + # A non-string condition is returned as-is by evaluate_condition and + # bool()-coerced, so it silently resolves to a truthiness (e.g. [1, 2] + # is always True) instead of erroring on the authoring mistake. + from specify_cli.workflows.steps.if_then import IfThenStep + + step = IfThenStep() + errors = step.validate({"id": "test", "condition": bad, "then": []}) + assert any("'condition' must be a string" in e for e in errors), bad + + @pytest.mark.parametrize("good", ["true", "false", "{{ inputs.flag }}"]) + def test_validate_accepts_string_condition(self, good): + from specify_cli.workflows.steps.if_then import IfThenStep + + step = IfThenStep() + errors = step.validate({"id": "test", "condition": good, "then": []}) + assert not any("'condition' must be a string" in e for e in errors), good + @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. @@ -2880,6 +2899,14 @@ def test_validate_missing_fields(self): assert any("missing 'condition'" in e for e in errors) # max_iterations is optional (defaults to 10) + @pytest.mark.parametrize("bad", [["a", "b"], {"k": "v"}, 5, True]) + def test_validate_rejects_non_string_condition(self, bad): + from specify_cli.workflows.steps.while_loop import WhileStep + + step = WhileStep() + errors = step.validate({"id": "test", "condition": bad, "steps": []}) + assert any("'condition' must be a string" in e for e in errors), bad + def test_validate_invalid_max_iterations(self): from specify_cli.workflows.steps.while_loop import WhileStep @@ -2994,6 +3021,14 @@ def test_validate_missing_fields(self): assert any("missing 'condition'" in e for e in errors) # max_iterations is optional (defaults to 10) + @pytest.mark.parametrize("bad", [["a", "b"], {"k": "v"}, 5, True]) + def test_validate_rejects_non_string_condition(self, bad): + from specify_cli.workflows.steps.do_while import DoWhileStep + + step = DoWhileStep() + errors = step.validate({"id": "test", "condition": bad, "steps": []}) + assert any("'condition' must be a string" in e for e in errors), bad + def test_validate_steps_not_list(self): from specify_cli.workflows.steps.do_while import DoWhileStep