CLI approval modal blocked by invalidate throttle

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #41155 fix(cli): paint approval/clarify/sudo/secret modal prompts directly, not via the throttle (#41098)

  merged to main

  files: cli.py, tests/cli/test_cli_approval_ui.py

  Show the diff

  • cli.py

    @@ -3479,7 +3479,22 @@ def __init__(
             self._background_task_counter = 0
     
         def _invalidate(self, min_interval: float = 0.25) -> None:
    -        """Throttled UI repaint — prevents terminal blinking on slow/SSH connections."""
    +        """Throttled UI repaint for high-frequency background updates.
    +
    +        Use this for spinner frames, streaming token flushes, and other
    +        repaints that can fire many times per second — the throttle prevents
    +        terminal blinking on slow/SSH connections, and the resize-recovery
    +        guard avoids stamping footer/status-bar chrome into scrollback while a
    +        SIGWINCH reflow is in flight.
    +
    +        Do NOT use this for user-blocking modal prompts (approval / clarify /
    +        sudo). Those are rare, one-shot, user-blocking events that must paint
    +        immediately; route them through ``self._app.invalidate()`` directly, the
    +        same way the modal key-binding handlers already do. Sending a modal's
    +        entry paint through this throttle lets an unrelated background repaint
    +        within the 250ms window — or an in-flight resize — silently drop it, so
    +        the prompt never renders and times out unseen (#41098).
    +        """
             if getattr(self, "_resize_recovery_pending", False):
                 return
             now = time.monotonic()
    @@ -3487,6 +3502,24 @@ def _invalidate(self, min_interval: float = 0.25) -> None:
                 self._last_invalidate = now
                 self._app.invalidate()
     
    +    def _paint_now(self) -> None:
    +        """Immediate, unthrottled repaint for user-blocking modal prompts.
    +
    +        Background-thread callbacks (approval / clarify / sudo) set their modal
    +        state then call this to make the panel visible at once. It deliberately
    +        bypasses the ``_invalidate`` throttle and resize-recovery guard — a
    +        modal the user is actively waiting on must never be dropped — mirroring
    +        the direct ``event.app.invalidate()`` the modal key-binding handlers
    +        already use. See ``_invalidate`` for why the throttle must not gate
    +        these paints (#41098).
    +        """
    +        app = getattr(self, "_app", None)
    +        if app is not None:
    +            try:
    +                app.invalidate()
    +            except Exception:
    +                pass
    +
         def _force_full_redraw(self) -> None:
             """Force a clean full-screen repaint of the prompt_toolkit UI.
     
    @@ -11801,18 +11834,15 @@ def _clarify_callback(self, question, choices):
             # Open-ended questions skip straight to freetext input
             self._clarify_freetext = is_open_ended
     
    -        # Trigger prompt_toolkit repaint from this (non-main) thread
    -        self._invalidate()
    +        # Trigger an immediate prompt_toolkit repaint from this (non-main)
    +        # thread. Modal prompts must paint at once and must not be gated by the
    +        # _invalidate throttle / resize guard — see _paint_now / _invalidate (#41098).
    +        self._paint_now()
     
    -        # Poll for the user's response.  The countdown in the hint line
    -        # updates on each invalidate — but frequent repaints cause visible
    -        # flicker in some terminals (Kitty, ghostty).  We only refresh the
    -        # countdown every 5 s; selection changes (↑/↓) trigger instant
    -        # Poll for the user's response.  The countdown in the hint line
    -        # updates on each invalidate — but frequent repaints cause visible
    -        # flicker in some terminals (Kitty, ghostty).  We only refresh the
    -        # countdown every 5 s; selection changes (↑/↓) trigger instant
    -        # repaints via the key bindings.
    +        # Poll for the user's response. The countdown in the hint line updates
    +        # on each repaint; refresh it once a second so the timer stays visible
    +        # while we wait. Selection changes (↑/↓) trigger instant repaints via
    +        # the key bindings.
             _last_countdown_refresh = _time.monotonic()
             while True:
                 try:
    @@ -11823,20 +11853,16 @@ def _clarify_callback(self, question, choices):
                     remaining = self._clarify_deadline - _time.monotonic()
                     if remaining <= 0:
                         break
    -                # Only repaint every 5 s for the countdown — avoids flicker
                     now = _time.monotonic()
    -                if now - _last_countdown_refresh >= 5.0:
    -                    _last_countdown_refresh = now
    -                    self._invalidate()
    -                if now - _last_countdown_refresh >= 5.0:
    +                if now - _last_countdown_refresh >= 1.0:
                         _last_countdown_refresh = now
    -                    self._invalidate()
    +                    self._paint_now()
     
             # Timed out — tear down the UI and let the agent decide
             self._clarify_state = None
             self._clarify_freetext = False
             self._clarify_deadline = 0
    -        self._invalidate()
    +        self._paint_now()
             _cprint(f"\n{_DIM}(clarify timed out after {timeout}s — agent will decide){_RST}")
             return (
                 "The user did not provide a response within the time limit. "
    @@ -11862,7 +11888,9 @@ def _sudo_password_callback(self) -> str:
             }
             self._sudo_deadline = _time.monotonic() + timeout
     
    -        self._invalidate()
    +        # Modal prompt — paint immediately, bypassing the throttle/resize guard
    +        # so the prompt can't be dropped and time out unseen (#41098).
    +        self._paint_now()
     
             while True:
                 try:
    @@ -11870,7 +11898,7 @@ def _sudo_password_callback(self) -> str:
                     self._sudo_state = None
                     self._sudo_deadline = 0
                     self._restore_modal_input_snapshot()
    -                self._invalidate()
    +                self._paint_now()
                     if result:
                         _cprint(f"\n{_DIM}  ✓ Password received (cached for session){_RST}")
                     else:
    @@ -11880,12 +11908,12 @@ def _sudo_password_callback(self) -> str:
                     remaining = self._sudo_deadline - _time.monotonic()
                     if remaining <= 0:
                         break
    -                self._invalidate()
    +                self._paint_now()
     
             self._sudo_state = None
             self._sudo_deadline = 0
             self._restore_modal_input_snapshot()
    -        self._invalidate()
    +        self._paint_now()
             _cprint(f"\n{_DIM}  ⏱ Timeout — continuing without sudo{_RST}")
             return ""
     
    @@ -11919,7 +11947,12 @@ def _approval_callback(self, command: str, description: str,
                 }
                 self._approval_deadline = _time.monotonic() + timeout
     
    -            self._invalidate()
    +            # Modal prompt — paint immediately, bypassing the throttle/resize
    +            # guard. A throttled paint here can be silently dropped (250ms
    +            # window collision or in-flight resize), leaving the panel unseen so
    +            # the command is denied on timeout without the user ever seeing it
    +            # (#41098). The countdown refreshes below paint the same way.
    +            self._paint_now()
     
                 _last_countdown_refresh = _time.monotonic()
                 while True:
    @@ -11927,20 +11960,20 @@ def _approval_callback(self, command: str, description: str,
                         result = response_queue.get(timeout=1)
                         self._approval_state = None
                         self._approval_deadline = 0
    -                    self._invalidate()
    +                    self._paint_now()
                         return result
                     except queue.Empty:
                         remaining = self._approval_deadline - _time.monotonic()
                         if remaining <= 0:
                             break
                         now = _time.monotonic()
    -                    if now - _last_countdown_refresh >= 5.0:
    +                    if now - _last_countdown_refresh >= 1.0:
                             _last_countdown_refresh = now
    -                        self._invalidate()
    +                        self._paint_now()
     
                 self._approval_state = None
                 self._approval_deadline = 0
    -            self._invalidate()
    +            self._paint_now()
                 _cprint(f"\n{_DIM}  ⏱ Timeout — denying command{_RST}")
                 return "deny"
     
    @@ -12198,7 +12231,9 @@ def _submit_secret_response(self, value: str) -> None:
             self._secret_state["response_queue"].put(value)
             self._secret_state = None
             self._secret_deadline = 0
    -        self._invalidate()
    +        # Modal teardown — paint directly so the secret panel clears at once and
    +        # isn't held by the _invalidate throttle/resize guard (#41098).
    +        self._paint_now()
     
         def _cancel_secret_capture(self) -> None:
             self._submit_secret_response("")

  • tests/cli/test_cli_approval_ui.py

    @@ -339,6 +339,123 @@ def run_conversation(self, **kwargs):
             assert not cli._background_tasks
     
     
    +def _make_real_paint_cli_stub():
    +    """A stub whose modal repaint path runs the REAL _paint_now / _invalidate.
    +
    +    Both gates are set adversarially: _resize_recovery_pending=True and a recent
    +    _last_invalidate inside the throttle window. A throttled _invalidate() would
    +    be dropped under these conditions — _paint_now must paint regardless.
    +    """
    +    cli = HermesCLI.__new__(HermesCLI)
    +    cli._approval_state = None
    +    cli._approval_deadline = 0
    +    cli._approval_lock = threading.Lock()
    +    cli._sudo_state = None
    +    cli._sudo_deadline = 0
    +    cli._clarify_state = None
    +    cli._clarify_freetext = False
    +    cli._clarify_deadline = 0
    +    cli._modal_input_snapshot = None
    +    # Real methods, not mocks.
    +    cli._paint_now = HermesCLI._paint_now.__get__(cli, HermesCLI)
    +    cli._invalidate = HermesCLI._invalidate.__get__(cli, HermesCLI)
    +    cli._resize_recovery_pending = True       # gate 1: resize in flight
    +    cli._last_invalidate = time.monotonic()   # gate 2: inside throttle window
    +    cli._app = SimpleNamespace(invalidate=MagicMock(), current_buffer=_FakeBuffer())
    +    return cli
    +
    +
    +class TestModalPaintNow:
    +    """Regression for #41098 — modal prompts must paint immediately.
    +
    +    The dangerous-command approval, clarify, and sudo prompts run their wait
    +    loop on a background thread, set modal state a ConditionalContainer reads,
    +    then must repaint so the panel becomes visible. They used the throttled
    +    _invalidate(), whose paint is silently dropped on a 250ms window collision
    +    or while a resize is pending — so the prompt timed out unseen. They now use
    +    _paint_now(), which paints directly like the modal key-binding handlers.
    +    """
    +
    +    def test_paint_now_bypasses_throttle_and_resize_guard(self):
    +        cli = _make_real_paint_cli_stub()
    +        # A bare _invalidate() is suppressed under both gates...
    +        cli._invalidate()
    +        assert not cli._app.invalidate.called
    +        # ...but _paint_now() always paints.
    +        cli._paint_now()
    +        assert cli._app.invalidate.called
    +
    +    def test_paint_now_no_app_is_safe(self):
    +        cli = HermesCLI.__new__(HermesCLI)
    +        cli._app = None
    +        cli._paint_now()  # must not raise
    +
    +    def _drive(self, cli, target, state_attr):
    +        result = {}
    +
    +        def _run():
    +            result["value"] = target()
    +
    +        with patch.object(cli_module, "_cprint"):
    +            thread = threading.Thread(target=_run, daemon=True)
    +            thread.start()
    +            deadline = time.time() + 2
    +            while getattr(cli, state_attr) is None and time.time() < deadline:
    +                time.sleep(0.01)
    +            assert getattr(cli, state_attr) is not None
    +            assert cli._app.invalidate.called, (
    +                f"{state_attr} panel was not painted despite throttle + resize gates"
    +            )
    +            # Reset so we can prove the response-received teardown also repaints
    +            # (the panel must clear at once, not be held by the throttle).
    +            cli._app.invalidate.reset_mock()
    +            getattr(cli, state_attr)["response_queue"].put(
    +                "deny" if state_attr == "_approval_state" else
    +                ("a" if state_attr == "_clarify_state" else "pw")
    +            )
    +            thread.join(timeout=2)
    +            # clarify returns immediately on a response (no teardown repaint);
    +            # approval and sudo repaint to tear the panel down.
    +            if state_attr != "_clarify_state":
    +                assert cli._app.invalidate.called, (
    +                    f"{state_attr} panel was not repainted on teardown"
    +                )
    +        assert not thread.is_alive()
    +        return result["value"]
    +
    +    def test_approval_prompt_paints_under_both_gates(self):
    +        cli = _make_real_paint_cli_stub()
    +        value = self._drive(
    +            cli, lambda: cli._approval_callback("rm -rf /tmp/scratch", "danger"),
    +            "_approval_state",
    +        )
    +        assert value == "deny"
    +
    +    def test_clarify_prompt_paints_under_both_gates(self):
    +        cli = _make_real_paint_cli_stub()
    +        value = self._drive(
    +            cli, lambda: cli._clarify_callback("Pick one", ["a", "b"]),
    +            "_clarify_state",
    +        )
    +        assert value == "a"
    +
    +    def test_sudo_prompt_paints_under_both_gates(self):
    +        cli = _make_real_paint_cli_stub()
    +        value = self._drive(cli, cli._sudo_password_callback, "_sudo_state")
    +        assert value == "pw"
    +
    +    def test_secret_response_teardown_paints(self):
    +        """_submit_secret_response tears the secret panel down via _paint_now,
    +        so the panel clears immediately rather than being held by the throttle."""
    +        cli = _make_real_paint_cli_stub()
    +        cli._secret_state = {"response_queue": queue.Queue()}
    +        cli._secret_deadline = 0
    +        cli._submit_secret_response("hunter2")
    +        assert cli._secret_state is None
    +        assert cli._app.invalidate.called
    +        assert cli._secret_state is None  # cleared
    +
    +
     class TestApprovalCallbackThreadLocalWiring:
         """Regression guard for the thread-local callback freeze (#13617 / #13618).