From d5db4f0c5c9cbb1b1e523b2deba3ed73ef15e965 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 11 Sep 2026 20:48:10 +0500 Subject: [PATCH] fix(workflows): an interrupted gate prompt must not approve the gate `GateStep._prompt` treated Ctrl+C / Ctrl+D as a choice: except (EOFError, KeyboardInterrupt): print() return options[-1] # default to last (usually reject) "usually reject" is an assumption `validate` never enforces. It requires only that *some* option is 'reject'/'abort': reject_choices = {"reject", "abort"} if not any(o.lower() in reject_choices for o in options): so a hand-written `options` list whose reject choice is not last validates clean, and `options[-1]` is then an approving option. `execute` classifies the result with `if choice.lower() in ("reject", "abort")`, so the gate reported COMPLETED and the run walked straight past the human review: options=['approve', 'reject'] -> failed choice='reject' options=['reject', 'approve'] -> completed choice='approve' options=['approve', 'reject', 'request-changes'] -> completed choice='request-changes' All three validate with zero errors. This also contradicted the engine's own interrupt contract: `WorkflowEngine` catches KeyboardInterrupt and sets `RunStatus.PAUSED` with a `workflow_interrupted` log event, so Ctrl+C anywhere else pauses the run for `specify workflow resume`. Only at a gate did it silently make an approval decision. Now prefers the declared reject/abort option, falling back to the last option when none is declared. For the documented default `[approve, reject]` this is byte-for-byte the previous behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/steps/gate/__init__.py | 17 ++++- tests/test_workflows.py | 73 +++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) 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 ):