From dfd79cef48f2cb6f9f0f2b839eea68069b365d9d Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 12 Aug 2026 19:25:18 -0700 Subject: [PATCH] fix: GAP-020 - bound the first-run confirm prompt to 3 attempts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rich.prompt.Confirm.ask() runs a bare `while True:` internally with no attempt limit: any non-y/n response just re-prompts, forever. That loop sits at the first-run "Run setup now?" gate a brand-new user hits before anything else works, so a stray keypress, a pasted line, or an automation sending the wrong thing leaves the process stuck with no signal it will ever end. Adds _bounded_confirm(), a drop-in replacement built on Confirm's own input/validation primitives (get_input / process_response / on_validate_error) that caps retries at max_attempts (default 3). On the third invalid response it prints "No valid y/n response after 3 attempts. Skipping setup." and returns False -- the same fall-through the "setup skipped" path already takes for an explicit "n". EOF/Ctrl-C are deliberately left untouched: Click's BaseCommand.main() already converts those to a clean "Aborted!" exit, and catching them here would silently turn "the user asked to stop" into "skip setup and keep going." Test evidence (tests/test_gap020_bounded_confirm.py, 5 tests): terminates after exactly max_attempts invalid responses; honours a configurable limit (1, 2, 5); a valid answer short-circuits on the first attempt without consuming the retry budget; one invalid answer followed by a valid one recovers correctly; bare Enter returns the default. Reverting _bounded_confirm() to the old unbounded Confirm.ask() call makes the first test fail with "_bounded_confirm asked for input 51 times -- the loop is unbounded. This is the GAP-020 hang." (see PR description for the before/after run). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- amplifier_app_cli/commands/init.py | 56 +++++++++++- tests/test_gap020_bounded_confirm.py | 127 +++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 tests/test_gap020_bounded_confirm.py diff --git a/amplifier_app_cli/commands/init.py b/amplifier_app_cli/commands/init.py index 67efeedb..33884e86 100644 --- a/amplifier_app_cli/commands/init.py +++ b/amplifier_app_cli/commands/init.py @@ -5,6 +5,7 @@ import click from rich.console import Console from rich.prompt import Confirm +from rich.prompt import InvalidResponse from rich.prompt import Prompt from rich.table import Table @@ -220,6 +221,59 @@ def check_first_run() -> bool: return False +def _bounded_confirm( + console_arg: Console, + prompt: str, + default: bool = True, + max_attempts: int = 3, +) -> bool: + """Attempt-bounded replacement for ``rich.prompt.Confirm.ask()``. + + GAP-020: ``Confirm.ask()`` re-prompts on invalid (non-y/n) input with + **no bound** -- forever, if the user never types a recognized response. + Reproduced live: two non-y/n inputs in a row just re-prompt a third + time, with nothing indicating there's any limit at all. That's a real + gap at a first-run gate a brand-new user hits before anything else + works: anyone who doesn't type exactly "y" or "n" (a stray keypress, a + pasted line, an automation sending the wrong thing) is stuck with no + signal that it will ever end. + + EOF and Ctrl-C were investigated too and are deliberately left alone: + Click's own ``BaseCommand.main()`` already catches ``(EOFError, + KeyboardInterrupt)`` globally and converts them to a clean "Aborted!" + exit (verified from Click's source and confirmed live on native + Windows for both Ctrl-C and Ctrl-Z -- Windows' real console EOF key; + Ctrl-D, POSIX's EOF key, has no special meaning to the Windows console + subsystem at all and is simply inert there, not a bug). Catching EOF + here too would just relitigate something Click already gets right, and + would silently change "the user asked to stop" into "skip setup and + keep going" -- exactly the kind of quiet fallback the far more + predictable existing "Aborted!" exit avoids. + + This bounds only the retry count, falling through to the same "setup + skipped" messaging an explicit "n" answer already produces once + ``max_attempts`` is exhausted, so a run that can't get a valid answer + still terminates the prompt loudly and predictably instead of hanging. + """ + prompt_obj = Confirm(prompt, console=console_arg) + for _ in range(max_attempts): + value = prompt_obj.get_input( + console_arg, prompt_obj.make_prompt(default), False + ) + if value == "": + return default + try: + return prompt_obj.process_response(value) + except InvalidResponse as error: + prompt_obj.on_validate_error(value, error) + + console_arg.print( + f"[yellow]No valid y/n response after {max_attempts} attempts. " + "Skipping setup.[/yellow]" + ) + return False + + def prompt_first_run_init(console_arg: Console) -> bool: """Prompt user to run init on first run. Returns True if provider was added. @@ -238,7 +292,7 @@ def prompt_first_run_init(console_arg: Console) -> bool: ) console_arg.print() - if Confirm.ask("Run setup now?", default=True): + if _bounded_confirm(console_arg, "Run setup now?", default=True): from .provider import provider_manage_loop settings = _get_settings() diff --git a/tests/test_gap020_bounded_confirm.py b/tests/test_gap020_bounded_confirm.py new file mode 100644 index 00000000..36eb5cf0 --- /dev/null +++ b/tests/test_gap020_bounded_confirm.py @@ -0,0 +1,127 @@ +"""Regression tests for GAP-020: the first-run confirm prompt must terminate. + +This fix had **zero** test coverage. It was found by auditing every GAP claimed +in source comments against the GAPs referenced in tests -- the same audit that +would have caught GAP-021, whose untested fix turned out to silently corrupt +user input on every platform. + +`rich.prompt.Confirm.ask()` is a bare `while True:` with no bound. On invalid +(non-y/n) input it re-prompts forever, with nothing indicating a limit exists. +That sits at a first-run gate a brand-new user hits before anything else works, +so anyone who doesn't type exactly "y" or "n" -- a stray keypress, a pasted +line, an automation sending the wrong thing -- is stuck with no signal that it +will ever end. + +The contract these tests pin is exactly the one the fix exists to add: after +`max_attempts` invalid responses it **stops**, loudly, and falls through to the +same "setup skipped" state an explicit "n" produces. Every test here is bounded +by `pytest-timeout`-free construction -- a hang shows up as a failed assertion +on call count, not as a wedged suite. +""" + +from __future__ import annotations + +from unittest.mock import patch + +from amplifier_app_cli.commands.init import _bounded_confirm +from rich.console import Console + + +class _ScriptedInput: + """Feeds a fixed script of responses, then refuses to be asked again. + + If the loop is unbounded it will ask past the end of the script; raising + there converts an infinite hang into an immediate, legible failure rather + than a suite that never finishes. + """ + + def __init__(self, responses: list[str], hard_limit: int = 50) -> None: + self.responses = responses + self.calls = 0 + self.hard_limit = hard_limit + + def __call__(self, *_args: object, **_kwargs: object) -> str: + self.calls += 1 + if self.calls > self.hard_limit: + raise AssertionError( + f"_bounded_confirm asked for input {self.calls} times -- the " + "loop is unbounded. This is the GAP-020 hang." + ) + idx = min(self.calls - 1, len(self.responses) - 1) + return self.responses[idx] + + +def test_invalid_input_terminates_after_max_attempts() -> None: + """Three invalid answers must end the prompt, not re-ask forever.""" + console = Console(quiet=True) + scripted = _ScriptedInput(["banana", "banana", "banana"]) + + with patch("rich.prompt.PromptBase.get_input", scripted): + result = _bounded_confirm(console, "Proceed?", default=True, max_attempts=3) + + assert scripted.calls == 3, ( + f"expected exactly 3 prompts, got {scripted.calls}. Fewer means the " + "bound is too tight; more means it is not being honoured." + ) + assert result is False, ( + "after exhausting attempts the result must be the conservative " + "'skip setup' answer, matching an explicit 'n'" + ) + + +def test_max_attempts_is_actually_honoured() -> None: + """The bound must track max_attempts, not be hardcoded.""" + console = Console(quiet=True) + for limit in (1, 2, 5): + scripted = _ScriptedInput(["nonsense"]) + with patch("rich.prompt.PromptBase.get_input", scripted): + _bounded_confirm(console, "Proceed?", default=True, max_attempts=limit) + assert scripted.calls == limit, ( + f"max_attempts={limit} produced {scripted.calls} prompts" + ) + + +def test_valid_answer_short_circuits_immediately() -> None: + """A good answer must not consume the retry budget. + + Guards against a "fix" that bounds the loop by always running it to + exhaustion. + """ + console = Console(quiet=True) + + for answer, expected in (("y", True), ("n", False)): + scripted = _ScriptedInput([answer]) + with patch("rich.prompt.PromptBase.get_input", scripted): + result = _bounded_confirm( + console, "Proceed?", default=False, max_attempts=3 + ) + assert result is expected, f"answer {answer!r} produced {result!r}" + assert scripted.calls == 1, ( + f"a valid answer took {scripted.calls} prompts; should take 1" + ) + + +def test_recovery_after_invalid_input() -> None: + """An invalid answer followed by a valid one must accept the valid one.""" + console = Console(quiet=True) + scripted = _ScriptedInput(["what", "y"]) + + with patch("rich.prompt.PromptBase.get_input", scripted): + result = _bounded_confirm(console, "Proceed?", default=False, max_attempts=3) + + assert result is True + assert scripted.calls == 2 + + +def test_empty_input_returns_the_default() -> None: + """Bare Enter means "accept the default", not "invalid".""" + console = Console(quiet=True) + + for default in (True, False): + scripted = _ScriptedInput([""]) + with patch("rich.prompt.PromptBase.get_input", scripted): + result = _bounded_confirm( + console, "Proceed?", default=default, max_attempts=3 + ) + assert result is default + assert scripted.calls == 1