fix(cli): paint approval/clarify/sudo/secret modal prompts directly, not via the throttle (#41098)
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.
This commit is contained in:
@@ -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).
|
||||
|
||||
|
||||
Reference in New Issue
Block a user