Skip to content

fix(cli): force approval panel redraw past the _invalidate throttle (#41098)#41116

Closed
sanidhyasin wants to merge 1 commit into
NousResearch:mainfrom
sanidhyasin:fix/cli-approval-panel-render-throttle
Closed

fix(cli): force approval panel redraw past the _invalidate throttle (#41098)#41116
sanidhyasin wants to merge 1 commit into
NousResearch:mainfrom
sanidhyasin:fix/cli-approval-panel-render-throttle

Conversation

@sanidhyasin

Copy link
Copy Markdown
Contributor

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 command after 60s and never gets a chance to respond, which makes approvals.mode: manual effectively unusable — commands are silently denied.

The root cause is the 250ms throttle in _invalidate(). When _approval_callback sets self._approval_state and calls self._invalidate(), any other UI event (spinner frame, output flush, completion) that invalidated within the previous 250ms causes the approval redraw to be silently dropped. The ConditionalContainer wrapping 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.0 on every redraw inside _approval_callback — the same throttle-bypass idiom already used in ~9 other call sites in cli.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

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cli.py — in HermesCLI._approval_callback, the initial paint, the response-received paint, the periodic countdown refresh, and the timeout paint now call self._invalidate(min_interval=0.0) to bypass the redraw throttle. Countdown refresh cadence tightened from 5s → 1s.
  • tests/cli/test_cli_approval_ui.py — adds test_approval_callback_renders_despite_recent_invalidate, a regression test that drives the real throttled _invalidate with a recent invalidation inside the throttle window and asserts the panel is still painted.

How to Test

  1. pytest tests/cli/test_cli_approval_ui.py -q → all pass.
  2. To confirm the regression test guards the fix: revert the initial min_interval=0.0 back to self._invalidate() and re-run — test_approval_callback_renders_despite_recent_invalidate fails with "approval panel redraw was dropped by the throttle".
  3. Manual: with 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

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass — ran the affected suite tests/cli/test_cli_approval_ui.py -q (12 passed); did not run the full suite locally
  • I've added tests for my changes
  • I've tested on my platform: macOS (Apple Silicon), Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings) — added an inline rationale comment; no user-facing docs affected, N/A
  • N/A — no config keys changed
  • N/A — no architecture/workflow changes
  • Cross-platform: pure prompt_toolkit redraw path, no platform-specific code
  • N/A — no tool behavior/schema changes

…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.
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Superseded by #41155. Your root-cause diagnosis (the throttled _invalidate silently dropping the approval panel's repaint) was spot on and is credited in #41155 — thank you! #41155 takes a slightly different implementation route (paint modal prompts directly via a _paint_now() helper, matching the existing key-handler idiom, rather than a throttle-bypass flag) and extends the same fix to the clarify and sudo prompts which regressed identically. Crediting you for the diagnosis.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Approval panel never renders — _invalidate() throttle drops redraw

3 participants