From 72f130b35690466bf87aecc5a53281ffa445fe64 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Mon, 5 Oct 2026 21:30:34 +0500 Subject: [PATCH 1/2] 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. Rebased onto current main (files moved in the workflow/bundler restructure). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../workflows/step/gate/__init__.py | 32 +++- tests/test_workflows.py | 157 ++++++++++++++++++ 2 files changed, 187 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/step/gate/__init__.py b/src/specify_cli/workflows/step/gate/__init__.py index 5aac060c0f..3968bde662 100644 --- a/src/specify_cli/workflows/step/gate/__init__.py +++ b/src/specify_cli/workflows/step/gate/__init__.py @@ -247,9 +247,37 @@ def _prompt(message: str, options: list[str]) -> str: while True: try: raw = input(f" Choose [1-{len(options)}]: ").strip() - except (EOFError, KeyboardInterrupt): + except KeyboardInterrupt: + # Ctrl+C is not a gate verdict at all. ``WorkflowEngine`` turns a + # propagated KeyboardInterrupt into ``RunStatus.PAUSED`` plus a + # ``workflow_interrupted`` log event (engine.py:1059-1063 and + # :1144-1148), so the operator can pick the run back up with + # ``specify workflow resume``. Swallowing it here produced a + # *decision* instead -- ``on_reject`` then fired, usually + # aborting the whole run -- which is the one outcome an + # interrupted reviewer did not choose. Re-raise so the gate + # behaves like every other step under Ctrl+C. print() - return options[-1] # default to last (usually reject) + raise + except EOFError: + # EOF is different: stdin is closed, so there is no operator to + # resume and pausing would strand the run. Fall back to a + # rejection, which is the safe verdict for an unanswered review. + # + # ``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]`` validated + # clean and EOF resolved to 'request-changes', which + # ``execute`` does not classify as a rejection: the gate + # reported COMPLETED and the run walked past the human review. + # Prefer the declared reject/abort option; for the documented + # default ``[approve, reject]`` this is the previous behaviour. + print() + 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 bb83f3eda3..a2b64e98a9 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -3633,6 +3633,163 @@ 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( + "options", + [ + ["approve", "reject"], + ["reject", "approve"], + ["approve", "reject", "request-changes"], + ], + ids=["reject_last", "reject_first", "reject_middle"], + ) + def test_eof_at_prompt_never_approves(self, monkeypatch, options): + """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 EOF 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.step.gate import GateStep + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise EOFError + + monkeypatch.setattr("builtins.input", _boom) + + assert GateStep._prompt("Approve the plan?", options) == "reject" + + def test_eof_without_a_reject_option_keeps_last(self, monkeypatch): + """With no reject/abort option declared, the last option is still used.""" + from specify_cli.workflows.step.gate import GateStep + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise EOFError + + monkeypatch.setattr("builtins.input", _boom) + + assert GateStep._prompt("Pick one.", ["yes", "no"]) == "no" + + def test_ctrl_c_propagates_rather_than_becoming_a_verdict(self, monkeypatch): + """Ctrl+C is not a gate decision — it must reach the engine. + + `WorkflowEngine` turns a propagated KeyboardInterrupt into + `RunStatus.PAUSED` plus a `workflow_interrupted` event, so the operator + can resume. Swallowing it here produced a *decision* instead: the reject + branch fired `on_reject`, usually aborting the whole run — the one + outcome an interrupted reviewer did not choose. + """ + from specify_cli.workflows.step.gate import GateStep + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise KeyboardInterrupt + + monkeypatch.setattr("builtins.input", _boom) + + with pytest.raises(KeyboardInterrupt): + GateStep._prompt("Approve the plan?", ["approve", "reject"]) + + def test_ctrl_c_at_a_gate_step_propagates(self, monkeypatch): + """The step level must not convert it either — `execute` lets it through.""" + from specify_cli.workflows.step.gate import GateStep + from specify_cli.workflows.base import StepContext + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise KeyboardInterrupt + + monkeypatch.setattr("builtins.input", _boom) + + with pytest.raises(KeyboardInterrupt): + GateStep().execute( + { + "id": "review", + "message": "Approve the plan?", + "options": ["approve", "reject", "request-changes"], + "on_reject": "abort", + }, + StepContext(), + ) + + def test_eof_at_a_gate_step_records_a_rejection(self, monkeypatch): + """EOF still yields a verdict: there is no operator left to resume.""" + from specify_cli.workflows.step.gate import GateStep + from specify_cli.workflows.base import StepContext, StepStatus + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise EOFError + + 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_ctrl_c_at_a_gate_pauses_the_run(self, tmp_path, monkeypatch): + """End to end: Ctrl+C at a gate pauses the run, it does not abort it. + + This is the contract the split exists to honour. Previously the reject + fallback fired `on_reject: abort`, so an interrupted reviewer lost the + run instead of being able to `specify workflow resume` it. + """ + import yaml + from specify_cli.workflows.engine import WorkflowEngine, RunStatus + + workflows = tmp_path / ".specify" / "workflows" / "demo" + workflows.mkdir(parents=True) + (workflows / "workflow.yml").write_text( + yaml.safe_dump( + { + "schema_version": "1.0", + "workflow": {"id": "demo", "name": "D", "version": "1.0.0"}, + "steps": [ + { + "id": "review", + "type": "gate", + "message": "Approve?", + "options": ["approve", "reject"], + "on_reject": "abort", + } + ], + } + ), + encoding="utf-8", + ) + + _force_gate_stdin(monkeypatch, tty=True) + + def _boom(_prompt=""): + raise KeyboardInterrupt + + monkeypatch.setattr("builtins.input", _boom) + + engine = WorkflowEngine(tmp_path) + state = engine.execute(engine.load_workflow("demo")) + + assert state.status is RunStatus.PAUSED + events = [e.get("event") for e in state.log_entries if isinstance(e, dict)] + assert "workflow_interrupted" in events, events + def test_interactive_prompt_missing_show_file_does_not_crash( self, tmp_path, monkeypatch, capsys ): From 4dd4a28cace311d0ee592d7861b4f2740ca0f9db Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 8 Oct 2026 00:17:26 +0500 Subject: [PATCH 2/2] docs(workflows): cite the engine's interrupt handlers by method, not line The gate's Ctrl+C comment pointed at `engine.py:1059-1063` and `:1144-1148`; after the restructure those lines are workflow-copy persistence and resume bounds validation, while the handlers now sit in `WorkflowEngine.execute` and `WorkflowEngine.resume`. Name the methods so the reference survives the next move. Comment-only change. Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/workflows/step/gate/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/workflows/step/gate/__init__.py b/src/specify_cli/workflows/step/gate/__init__.py index 3968bde662..825229168f 100644 --- a/src/specify_cli/workflows/step/gate/__init__.py +++ b/src/specify_cli/workflows/step/gate/__init__.py @@ -248,11 +248,11 @@ def _prompt(message: str, options: list[str]) -> str: try: raw = input(f" Choose [1-{len(options)}]: ").strip() except KeyboardInterrupt: - # Ctrl+C is not a gate verdict at all. ``WorkflowEngine`` turns a - # propagated KeyboardInterrupt into ``RunStatus.PAUSED`` plus a - # ``workflow_interrupted`` log event (engine.py:1059-1063 and - # :1144-1148), so the operator can pick the run back up with - # ``specify workflow resume``. Swallowing it here produced a + # Ctrl+C is not a gate verdict at all. ``WorkflowEngine.execute`` + # and ``WorkflowEngine.resume`` turn a propagated + # KeyboardInterrupt into ``RunStatus.PAUSED`` plus a + # ``workflow_interrupted`` log event, so the operator can pick + # the run back up with ``specify workflow resume``. Swallowing it here produced a # *decision* instead -- ``on_reject`` then fired, usually # aborting the whole run -- which is the one outcome an # interrupted reviewer did not choose. Re-raise so the gate