Skip to content

fix(cli): keep approval prompts visible in scrollback - #64475

Open
visualfox-ch wants to merge 1 commit into
NousResearch:mainfrom
visualfox-ch:fix/approval-prompt-scrollback
Open

fix(cli): keep approval prompts visible in scrollback#64475
visualfox-ch wants to merge 1 commit into
NousResearch:mainfrom
visualfox-ch:fix/approval-prompt-scrollback

Conversation

@visualfox-ch

Copy link
Copy Markdown

Summary

  • keep terminal approval prompts visible in scrollback while the modal is active
  • route approval rendering through a bounded callback instead of transient-only output
  • add focused UI regression coverage

Test plan

  • python -m pytest tests/cli/test_cli_approval_ui.py -q -o 'addopts='
  • ruff check cli.py hermes_cli/callbacks.py tests/cli/test_cli_approval_ui.py

Preserve a plaintext pending-approval notice when prompt_toolkit repaints race with background-thread approvals.

Assisted-by: Hermes:gpt-5.6-sol
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have labels Jul 14, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Fix to keep approval prompts visible in scrollback. Small change (49 additions, 1 deletion). Clean UX fix.


Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real classic-CLI visibility gap: current HermesCLI._approval_callback sets its modal state and then waits, while the existing scrollback summary is only emitted after a choice is received (cli.py:11785-11822). The new _cprint call uses the established output-history and cross-thread prompt_toolkit path (cli.py:2593-2667).

Problems

  • The new test has a scheduling race: it waits for _approval_state, which is set before the new _cprint, then immediately reads the mock call list (tests/cli/test_cli_approval_ui.py:119 in this diff). The callback thread can yield in between.
  • The hermes_cli/callbacks.py change targets approval_callback, but repository-wide search found no in-tree caller; the active CLI registers self._approval_callback (cli.py:6091, cli.py:12418). That unused path also bypasses CLI _cprint's output-history handling.

Suggested changes

  • Wait for the mock pending-notice call with a bounded deadline before asserting its content.
  • Remove the unwired callback-module edit unless it is intentionally being wired into a supported path.

Automated hermes-sweeper review.

time.sleep(0.01)

assert cli._approval_state is not None
rendered = "\n".join(str(call.args[0]) for call in cprint.call_args_list)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This waits only for _approval_state, but the callback assigns that state before it calls _cprint. Please wait (with the same bounded deadline) for cprint.call_args_list to become non-empty before reading it; otherwise a scheduler switch between those operations makes this test flaky.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 16, 2026
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 P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants