Skip to content

Commit 2f9e455

Browse files
fix(workflows): fail gate step loudly on a malformed 'options' (#3595)
`GateStep.validate` rejects a non-list (or empty) `options` and requires every option to be a string, but the engine does not auto-validate before `execute`. On an unvalidated run a scalar/dict/None `options` reached `_prompt` and crashed the whole workflow with a raw `TypeError` (`enumerate`/`len` on a non-iterable) or `KeyError` (indexing a dict); an empty list spun `_prompt`'s input loop forever; a non-string option crashed the reject check at `choice.lower()` with `AttributeError`. Guard `execute` to FAIL the step cleanly instead, before the non-TTY PAUSE short-circuit so the error surfaces in CI too rather than pausing and only crashing later on interactive resume. Mirrors the switch 'cases' and command 'input' unvalidated-execute guards. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 69c8b64 commit 2f9e455

2 files changed

Lines changed: 95 additions & 0 deletions

File tree

src/specify_cli/workflows/steps/gate/__init__.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,35 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
4343
options = config.get("options", ["approve", "reject"])
4444
on_reject = config.get("on_reject", "abort")
4545

46+
# ``validate`` rejects a non-list (or empty) ``options``, and requires
47+
# every option to be a string, but the engine does not auto-validate
48+
# before ``execute``. An unvalidated run with a scalar/dict/None
49+
# ``options`` would otherwise reach ``_prompt`` and crash the whole run
50+
# with a raw ``TypeError`` (``enumerate``/``len`` on a non-iterable) or
51+
# ``KeyError`` (indexing a dict); a non-string option would crash at the
52+
# ``choice.lower()`` reject check with ``AttributeError``. Fail this step
53+
# loudly instead — mirroring the switch 'cases' and command 'input'
54+
# guards. Checked before the non-TTY short-circuit so the error surfaces
55+
# in CI too, rather than PAUSING and crashing later on interactive resume.
56+
if (
57+
not isinstance(options, list)
58+
or not options
59+
or not all(isinstance(o, str) for o in options)
60+
):
61+
return StepResult(
62+
status=StepStatus.FAILED,
63+
error=(
64+
f"Gate step {config.get('id', '?')!r}: 'options' must be a "
65+
f"non-empty list of strings, got {type(options).__name__}."
66+
),
67+
output={
68+
"message": message,
69+
"options": options,
70+
"on_reject": on_reject,
71+
"choice": None,
72+
},
73+
)
74+
4675
show_file = config.get("show_file")
4776
if isinstance(show_file, str) and "{{" in show_file:
4877
show_file = evaluate_expression(show_file, context)

tests/test_workflows.py

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2093,6 +2093,72 @@ def test_templated_show_file_resolving_to_non_string_is_coerced(self):
20932093
assert result.status == StepStatus.PAUSED
20942094
assert result.output["show_file"] == "123"
20952095

2096+
@pytest.mark.parametrize(
2097+
"bad_options",
2098+
[5, {"a": "approve"}, None, [], "approve"],
2099+
)
2100+
def test_execute_non_list_options_fails_cleanly(self, monkeypatch, bad_options):
2101+
"""A malformed ``options`` must FAIL the step, not crash the run.
2102+
2103+
``validate`` rejects a non-list/empty ``options``, but the engine does
2104+
not auto-validate before ``execute``. On an interactive run a scalar/
2105+
dict/None ``options`` would otherwise reach ``_prompt`` and raise a raw
2106+
``TypeError`` (``enumerate``/``len`` on a non-iterable) or ``KeyError``
2107+
(indexing a dict), crashing the whole workflow. Mirrors the switch
2108+
'cases' and command 'input' unvalidated-execute guards."""
2109+
from specify_cli.workflows.steps.gate import GateStep
2110+
from specify_cli.workflows.base import StepContext, StepStatus
2111+
2112+
# Force an interactive TTY so the crash-prone _prompt path is reached;
2113+
# input() is stubbed so a (buggy) fall-through can't block the suite.
2114+
_force_gate_stdin(monkeypatch, tty=True)
2115+
monkeypatch.setattr("builtins.input", lambda _prompt="": "1")
2116+
2117+
step = GateStep()
2118+
config = {"id": "review", "message": "Review.", "options": bad_options}
2119+
result = step.execute(config, StepContext())
2120+
2121+
assert result.status == StepStatus.FAILED
2122+
assert "options" in (result.error or "")
2123+
assert result.output["choice"] is None
2124+
2125+
def test_execute_non_string_options_element_fails_cleanly(self, monkeypatch):
2126+
"""A non-string option element must FAIL the step, not crash.
2127+
2128+
A non-empty list with a non-string element passes the shape check but
2129+
would reach the reject test ``choice.lower()`` and raise a raw
2130+
``AttributeError`` at run time. ``validate`` reports "must be strings";
2131+
``execute`` must fail cleanly on an unvalidated run too."""
2132+
from specify_cli.workflows.steps.gate import GateStep
2133+
from specify_cli.workflows.base import StepContext, StepStatus
2134+
2135+
_force_gate_stdin(monkeypatch, tty=True)
2136+
monkeypatch.setattr("builtins.input", lambda _prompt="": "1")
2137+
2138+
step = GateStep()
2139+
config = {"id": "review", "message": "Review.", "options": [123, 456]}
2140+
result = step.execute(config, StepContext())
2141+
2142+
assert result.status == StepStatus.FAILED
2143+
assert "options" in (result.error or "")
2144+
2145+
def test_execute_non_list_options_fails_in_non_tty_too(self):
2146+
"""The guard runs before the non-TTY PAUSE short-circuit.
2147+
2148+
A malformed ``options`` should surface as FAILED in CI (non-TTY) rather
2149+
than PAUSING and only crashing later when an operator resumes on a real
2150+
terminal."""
2151+
from specify_cli.workflows.steps.gate import GateStep
2152+
from specify_cli.workflows.base import StepContext, StepStatus
2153+
2154+
# Autouse fixture already forces non-TTY stdin.
2155+
step = GateStep()
2156+
config = {"id": "review", "message": "Review.", "options": 5}
2157+
result = step.execute(config, StepContext())
2158+
2159+
assert result.status == StepStatus.FAILED
2160+
assert "options" in (result.error or "")
2161+
20962162

20972163
class TestIfThenStep:
20982164
"""Test the if/then/else step type."""

0 commit comments

Comments
 (0)