From 876dca865946557beb0545efab5d136e87b63096 Mon Sep 17 00:00:00 2001 From: Ali jawwad <33836051+jawwad-ali@users.noreply.github.com> Date: Tue, 30 Jun 2026 01:07:50 +0500 Subject: [PATCH] fix(workflows): gate validate() must not crash on non-string options (#3233) GateStep.validate() reports non-string options as an error, but then -- when on_reject is 'abort'/'retry' -- still runs the reject-choice check 'any(o.lower() in ... for o in options)'. For a non-string option (e.g. options: [123]) o.lower() raised AttributeError, which escaped validate() and broke validate_workflow's documented 'return a list of errors, never raise' contract. Guard the check so it only runs when every option is a string (the non-string case is already reported above). Co-authored-by: Claude Opus 4.8 (1M context) --- .../workflows/steps/gate/__init__.py | 9 ++++++++- tests/test_workflows.py | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/steps/gate/__init__.py b/src/specify_cli/workflows/steps/gate/__init__.py index a2e473244..e07b6ebd6 100644 --- a/src/specify_cli/workflows/steps/gate/__init__.py +++ b/src/specify_cli/workflows/steps/gate/__init__.py @@ -194,7 +194,14 @@ class GateStep(StepBase): f"Gate step {config.get('id', '?')!r}: 'on_reject' must be " f"'abort', 'skip', or 'retry'." ) - if on_reject in ("abort", "retry") and isinstance(options, list): + # Only inspect option text when every option is a string; otherwise the + # `o.lower()` below would raise AttributeError on a non-string option + # (already reported above) and break validate_workflow's never-raise contract. + if ( + on_reject in ("abort", "retry") + and isinstance(options, list) + and all(isinstance(o, str) for o in options) + ): reject_choices = {"reject", "abort"} if not any(o.lower() in reject_choices for o in options): errors.append( diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 8bf572439..eebc89fad 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -1455,6 +1455,23 @@ class TestGateStep: }) assert any("on_reject" in e for e in errors) + def test_validate_non_string_options_does_not_raise(self): + """Non-string options with on_reject=abort/retry must be REPORTED as an + error, not crash: the reject-choice check calls o.lower() on each option, + which previously raised AttributeError on a non-string option and broke + validate_workflow's 'return errors, never raise' contract.""" + from specify_cli.workflows.steps.gate import GateStep + + step = GateStep() + # on_reject defaults to "abort", which triggers the option-text check. + errors = step.validate({"id": "test", "message": "Review", "options": [123]}) + assert any("must be strings" in e for e in errors) + # also with an explicit retry on_reject + errors = step.validate( + {"id": "test", "message": "Review", "options": [True], "on_reject": "retry"} + ) + assert any("must be strings" in e for e in errors) + def test_interactive_prompt_renders_show_file(self, tmp_path, monkeypatch, capsys): from specify_cli.workflows.steps.gate import GateStep from specify_cli.workflows.base import StepContext, StepStatus