diff --git a/src/specify_cli/workflows/steps/gate/__init__.py b/src/specify_cli/workflows/steps/gate/__init__.py index 5aac060c0f..20e1a0b569 100644 --- a/src/specify_cli/workflows/steps/gate/__init__.py +++ b/src/specify_cli/workflows/steps/gate/__init__.py @@ -248,8 +248,23 @@ def _prompt(message: str, options: list[str]) -> str: try: raw = input(f" Choose [1-{len(options)}]: ").strip() except (EOFError, KeyboardInterrupt): + # An interrupted prompt is not a choice. Returning ``options[-1]`` + # assumed the reject option is last, but ``validate`` only + # requires that *some* option is 'reject'/'abort' -- never that + # it is last. So ``options: [approve, reject, request-changes]`` + # validates clean and an interrupt resolved to + # 'request-changes', which ``execute`` does not classify as a + # rejection: the gate reported COMPLETED and the run walked past + # the human review it exists to enforce. + # + # Prefer the declared reject/abort option. For the documented + # default ``[approve, reject]`` this is byte-for-byte the old + # behaviour, since 'reject' is both last and the reject option. print() - return options[-1] # default to last (usually reject) + return next( + (o for o in options if o.lower() in ("reject", "abort")), + options[-1], + ) # isdecimal() (not isdigit()): int() accepts exactly the decimal-digit # set, whereas isdigit() also returns True for superscripts/subscripts # (e.g. "²") that int() then rejects with ValueError — crashing diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2c7141e954..2c82856741 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -3003,6 +3003,79 @@ def test_interactive_prompt_rejects_non_decimal_digit(self, monkeypatch, capsys) choice = GateStep._prompt("Review the spec.", ["approve", "reject"]) assert choice == "approve" + @pytest.mark.parametrize("interrupt", [KeyboardInterrupt, EOFError]) + @pytest.mark.parametrize( + "options", + [ + ["approve", "reject"], + ["reject", "approve"], + ["approve", "reject", "request-changes"], + ], + ids=["reject_last", "reject_first", "reject_middle"], + ) + def test_interrupted_prompt_never_approves( + self, monkeypatch, options, interrupt + ): + """Ctrl+C / Ctrl+D at a gate must not resolve to an approving option. + + `_prompt` returned `options[-1]`, assuming the reject option is last. + `validate` only requires that *some* option is 'reject'/'abort', never + that it is last, so `options: [approve, reject, request-changes]` + validates clean and an interrupt returned 'request-changes' — which + `execute` does not classify as a rejection, so the gate reported + COMPLETED and the run walked past the human review. + """ + from specify_cli.workflows.steps.gate import GateStep + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise interrupt + + monkeypatch.setattr("builtins.input", _boom) + + assert GateStep._prompt("Approve the plan?", options) == "reject" + + def test_interrupted_prompt_without_a_reject_option_keeps_last( + self, monkeypatch + ): + """With no reject/abort option declared, the last option is still used.""" + from specify_cli.workflows.steps.gate import GateStep + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise KeyboardInterrupt + + monkeypatch.setattr("builtins.input", _boom) + + assert GateStep._prompt("Pick one.", ["yes", "no"]) == "no" + + def test_interrupted_gate_step_does_not_complete(self, monkeypatch): + """The step-level consequence: the gate must not report COMPLETED.""" + from specify_cli.workflows.steps.gate import GateStep + from specify_cli.workflows.base import StepContext, StepStatus + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise KeyboardInterrupt + + monkeypatch.setattr("builtins.input", _boom) + + result = GateStep().execute( + { + "id": "review", + "message": "Approve the plan?", + "options": ["approve", "reject", "request-changes"], + "on_reject": "abort", + }, + StepContext(), + ) + + assert result.status is StepStatus.FAILED + assert result.output["choice"] == "reject" + def test_interactive_prompt_missing_show_file_does_not_crash( self, tmp_path, monkeypatch, capsys ):