fix(cli): force approval panel redraw past the _invalidate throttle (#41098)#41116
Closed
sanidhyasin wants to merge 1 commit into
Closed
fix(cli): force approval panel redraw past the _invalidate throttle (#41098)#41116sanidhyasin wants to merge 1 commit into
sanidhyasin wants to merge 1 commit into
Conversation
…ousResearch#41098) The dangerous-command approval prompt never rendered in classic CLI mode. `_approval_callback` set `_approval_state` and called the throttled `_invalidate()` (250ms min interval). When any other UI event — a spinner frame, an output flush — had invalidated within the previous window, this redraw was silently dropped, the `ConditionalContainer` wrapping the panel never re-evaluated, and the user saw nothing until the 60s timeout denied the command. The retry refresh only fired every 5s, far too slow to recover. The approval prompt is a user-blocking modal: it must paint immediately regardless of throttle state. Pass `min_interval=0.0` on every redraw in the loop — the same throttle-bypass already used elsewhere in the file — and tighten the countdown refresh to 1s so the timer stays visible while waiting. Adds a regression test driving the real throttled `_invalidate` with a recent invalidation in the window; it fails without the bypass and passes with it.
Collaborator
|
Superseded by #41155. Your root-cause diagnosis (the throttled |
kshitijk4poor
added a commit
to kshitijk4poor/hermes-agent
that referenced
this pull request
Jun 7, 2026
…not via the throttle (NousResearch#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 NousResearch#41116; @islam666 independently reached the same direct-invalidate approach in NousResearch#41166; original report NousResearch#41098 by @jodonnel.
a249169329-cpu
pushed a commit
to a249169329-cpu/hermes-agent
that referenced
this pull request
Jun 9, 2026
…not via the throttle (NousResearch#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 NousResearch#41116; @islam666 independently reached the same direct-invalidate approach in NousResearch#41166; original report NousResearch#41098 by @jodonnel.
changman
pushed a commit
to changman/hermes-agent
that referenced
this pull request
Jun 10, 2026
…not via the throttle (NousResearch#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 NousResearch#41116; @islam666 independently reached the same direct-invalidate approach in NousResearch#41166; original report NousResearch#41098 by @jodonnel.
alt-glitch
pushed a commit
that referenced
this pull request
Jun 14, 2026
…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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the dangerous-command approval prompt never rendering in classic CLI mode (
hermes chat, not--tui). Today the user just sees⏱ Timeout — denying commandafter 60s and never gets a chance to respond, which makesapprovals.mode: manualeffectively unusable — commands are silently denied.The root cause is the 250ms throttle in
_invalidate(). When_approval_callbacksetsself._approval_stateand callsself._invalidate(), any other UI event (spinner frame, output flush, completion) that invalidated within the previous 250ms causes the approval redraw to be silently dropped. TheConditionalContainerwrapping the approval widget never re-evaluates, so the panel never appears. The in-loop retry only refreshed every 5s — far too slow to recover.The approval prompt is a user-blocking modal: it must paint immediately regardless of throttle state. The fix passes
min_interval=0.0on every redraw inside_approval_callback— the same throttle-bypass idiom already used in ~9 other call sites incli.py— and tightens the in-wait countdown refresh from 5s to 1s so the timer stays visible while waiting.Related Issue
Fixes #41098
Type of Change
Changes Made
cli.py— inHermesCLI._approval_callback, the initial paint, the response-received paint, the periodic countdown refresh, and the timeout paint now callself._invalidate(min_interval=0.0)to bypass the redraw throttle. Countdown refresh cadence tightened from 5s → 1s.tests/cli/test_cli_approval_ui.py— addstest_approval_callback_renders_despite_recent_invalidate, a regression test that drives the real throttled_invalidatewith a recent invalidation inside the throttle window and asserts the panel is still painted.How to Test
pytest tests/cli/test_cli_approval_ui.py -q→ all pass.min_interval=0.0back toself._invalidate()and re-run —test_approval_callback_renders_despite_recent_invalidatefails with "approval panel redraw was dropped by the throttle".approvals.mode: manual, run a command that triggers the dangerous-command prompt while the spinner is active — the approval panel now renders immediately instead of timing out.Checklist
Code
fix(scope):)pytest tests/ -qand all tests pass — ran the affected suitetests/cli/test_cli_approval_ui.py -q(12 passed); did not run the full suite locallyDocumentation & Housekeeping