From 8e71b5136be81741277b17550f53a6d6937e26a7 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 7 Jun 2026 15:38:36 +0530 Subject: [PATCH] fix(cli): paint approval/clarify/sudo/secret modal prompts directly, not via the throttle (#41098) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In classic CLI mode the dangerous-command approval prompt (and the clarify, sudo, and secret-capture prompts) could fail to render: the user saw '⏱ Timeout — denying command' after 60s without ever seeing the panel, making approvals.mode: manual unusable. Root cause. These prompts run their wait loop on the agent/background thread: they set modal state that a ConditionalContainer's filter reads, then call self._invalidate() to repaint so the panel appears. _invalidate() is a THROTTLED wrapper built for high-frequency background repaints (spinner frames, streaming) — it (a) returns early while a SIGWINCH resize-recovery is pending, and (b) otherwise only repaints if 250ms elapsed since the last paint. Under either condition the modal's entry paint is silently dropped, the ConditionalContainer never re-evaluates, and the prompt times out unseen. The throttle never belonged on these paths. Originally the callbacks painted with a direct self._app.invalidate() and worked; a throttle PR blanket-replaced every invalidate (including these rare, one-shot, user-blocking modal paints) with the throttled _invalidate(); a later commit removed an idle 1Hz repaint that had been masking dropped modal paints, surfacing the bug. Notably the modal KEY-BINDING handlers (↑/↓/Enter) already paint with a direct event.app.invalidate(), never the throttle — the background-thread callbacks were the inconsistent ones. Fix. Add a small _paint_now() helper that paints directly (guarded for a missing _app, exception-safe) and route the four modal paths' entry, response, countdown, and teardown paints through it — matching the key-handler idiom. This covers approval, clarify, sudo, and the secret-capture teardown (_submit_secret_response, which previously used the throttled _invalidate() so its panel could linger after submit). _invalidate() is left untouched and its docstring now states it is for high-frequency background repaints only; modal/interactive paints must use _paint_now()/_app.invalidate() directly. This also fixes the resize-recovery edge case for free (a direct paint never consults the resize guard) without a throttle-bypass flag that could be cargo-culted onto hot paths. Countdown refresh cadence tightened 5s->1s so the timer stays visible while waiting, and a copy-pasted duplicate countdown block in _clarify_callback is removed. Tests: TestModalPaintNow drives all three wait-loop callbacks on a background thread with BOTH gates active (_resize_recovery_pending=True + a recent _last_invalidate in the throttle window) and asserts the panel paints on entry AND repaints on teardown; plus a secret-teardown test, a direct _paint_now-vs-_invalidate gate test, and a no-_app safety test. Each modal test fails if its paint is reverted to _invalidate(). 17 in-file tests pass; full tests/cli suite green (900). Diagnosis credit: the throttle-drop root cause was identified by @sanidhyasin in #41116; @islam666 independently reached the same direct-invalidate approach in #41166; original report #41098 by @jodonnel. --- cli.py | 93 ++++++++++++++++-------- tests/cli/test_cli_approval_ui.py | 117 ++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+), 29 deletions(-) diff --git a/cli.py b/cli.py index 000778b750f..9d51059af7b 100644 --- a/cli.py +++ b/cli.py @@ -3479,7 +3479,22 @@ class HermesCLI: 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 @@ class HermesCLI: 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 @@ class HermesCLI: # 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 @@ class HermesCLI: 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: + if now - _last_countdown_refresh >= 1.0: _last_countdown_refresh = now - self._invalidate() - if now - _last_countdown_refresh >= 5.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 @@ class HermesCLI: } 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 @@ class HermesCLI: 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 @@ class HermesCLI: 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 @@ class HermesCLI: } 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 @@ class HermesCLI: 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 @@ class HermesCLI: 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("") diff --git a/tests/cli/test_cli_approval_ui.py b/tests/cli/test_cli_approval_ui.py index f086f27a9b6..df7c06a2d00 100644 --- a/tests/cli/test_cli_approval_ui.py +++ b/tests/cli/test_cli_approval_ui.py @@ -339,6 +339,123 @@ class TestCliApprovalUi: 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).