fix(cli): drain stdin on every curses picker exit, and stop raw arrow keys clearing the search query - #83045
Open
briandevans wants to merge 3 commits into
Open
Conversation
`_run_curses_menu` called `flush_stdin()` only on the normal return path. Both abnormal exits — `except KeyboardInterrupt` and the `except Exception` branch that hands off to the numbered fallback — skipped it. `flush_stdin`'s own docstring states the invariant this violates: it "must be called after `curses.wrapper()` ... before the next `input()` / `getpass.getpass()` call", because `curses.endwin()` restores terminal modes but does not drain the OS input buffer. Leftover CSI bytes from arrow keys or rapid keypresses survive into the next `input()` and are silently consumed as if the user had typed them. The fallback path is where this bites hardest: `_radio_numbered_fallback`, `_numbered_single_fallback` and `_numbered_fallback` all call `input()` as their next real statement. So the one code path whose entire job is "curses died, let the user type a number instead" goes straight from curses teardown to `input()` with the buffer undrained — `int(val)` then raises `ValueError` and the picker resolves to its cancel value without the user having answered. Wrapping the `curses.wrapper()` call in `try/finally` drains on all three paths. `import curses` deliberately stays outside the inner `try` so an ImportError on Windows never triggers a pointless flush, and `flush_stdin` already no-ops on non-TTY stdin and swallows its own errors, so it cannot mask the exception that is unwinding.
…s picker While the type-to-filter prompt is open, the driver hands the raw `getch()` byte straight to `_handle_active_search_key`, which treats any `27` as a lone ESC and clears `search.query`. But `27` is ambiguous. `_decode_menu_key` exists in this same module precisely because some terminals/terminfo entries deliver cursor keys as raw CSI/SS3 sequences even with `keypad(True)` — `getch()` returns `27`, then `[`, then `A`/`B` — and it disambiguates with a 60 ms continuation probe. The search branch did no such probe, so on those terminals an arrow key inside an active search was read as "stop searching". Symptom: in `hermes model`, press `/`, type `sonnet`, press the down arrow. The query vanishes, the full unfiltered list snaps back, and the cursor does not move — the `[` and `A`/`B` tail bytes then decode to NAV_NONE and are swallowed. Fix: when search is active and the key is `27`, run `_decode_menu_key` first. Only a genuine `NAV_CANCEL` (the probe timed out, i.e. a real lone ESC) reaches `_handle_active_search_key`; a decoded arrow becomes the loop's action so the cursor moves through the filtered list with the query intact. The already consumed sequence is never decoded twice. `_handle_active_search_key` keeps its signature and its lone-ESC semantics.
New file rather than an edit to the existing curses test modules, so the regression coverage does not collide with unrelated in-flight work on them. Covers both invariants and both directions: - `test_flush_stdin_runs_when_picker_is_interrupted` — `curses.wrapper` raises KeyboardInterrupt; asserts the cancel value AND exactly one drain. - `test_flush_stdin_runs_before_the_numbered_fallback` — `curses.wrapper` raises `curses.error`; the fallback spy records how many drains it can see at the moment it is invoked, so this asserts ORDERING, not just a count. This is the load-bearing case: the fallback's next statement is `input()`. - `test_flush_stdin_still_runs_on_the_normal_path` — guards against a regression that MOVES the drain into the exception handlers instead of widening it to all paths. - `test_raw_arrow_escape_does_not_wipe_the_active_search_query` — replays the raw three-byte arrow-down `ESC [ B` during an active search and asserts the cursor moved within the filtered list rather than the query being cleared. - `test_lone_escape_still_stops_the_search_and_restores_the_full_list` — a genuine lone ESC (continuation probe reads -1) keeps its existing meaning. Reuses the module-level `sys.platform == "win32"` skip and the `FakeStdscr` replay pattern already established in `tests/hermes_cli/test_curses_arrow_keys.py`.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the shared curses picker driver (_run_curses_menu) against raw escape-byte leakage and mis-decoding during active search, ensuring terminal input remains clean and cursor-key navigation works reliably across terminals that emit raw CSI/SS3 sequences.
Changes:
- Move
flush_stdin()into afinallyso stdin is drained on normal return,KeyboardInterrupt, and curses-error fallback paths. - When search is active, decode a raw
ESC(27) via_decode_menu_keybefore treating it as “lone ESC clears search,” preventing arrow keys from wiping the query. - Add a new regression test module covering teardown draining and search + raw-arrow behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
hermes_cli/curses_ui.py |
Ensures stdin is flushed on every wrapper exit and fixes raw-ESC handling during active search by routing through _decode_menu_key. |
tests/hermes_cli/test_curses_ui_teardown.py |
Adds regression tests proving flush ordering on abnormal exits and preserving search query on raw CSI arrow sequences. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alt-glitch
added
type/bug
19 tasks
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.
This is a sibling follow-up to #7167 (merged 2026-04-10)
flush_stdin()and wired it intohermes_cli/curses_ui.pyso stray escape bytes left in the OS input bufferby a curses menu can't leak into the next
input().path only.
_run_curses_menu's two abnormal exits —except KeyboardInterruptand theexcept Exceptionbranch that hands off to thenumbered fallback — still return without draining.
finally, so all three exitsdrain. Plus a second escape-byte defect in the same driver: a raw
27duringan active search is now decoded before it is treated as a lone ESC.
What does this PR do?
Two bugs in
hermes_cli/curses_ui.py's shared picker driver,_run_curses_menu,both about raw escape bytes.
1.
flush_stdin()only ran on the normal return path.The invariant is stated in this file, by
flush_stdin's own docstring: it"must be called after
curses.wrapper()… before the nextinput()/getpass.getpass()call", becausecurses.endwin()restores the terminal butdoes not drain the OS input buffer, so leftover escape-sequence bytes
"silently get consumed by the next
input()call, corrupting user data".To be precise about the blast radius:
curses.wrapper's teardown iskeypad(0); echo(); nocbreak(); endwin(), so terminal modes are restoredcorrectly. This is not a "terminal left unusable" bug. The claim is
narrower and concrete: buffered bytes leak into the next prompt.
The fallback path is where that bites hardest. All three numbered fallbacks
call
input()as their next real statement —_radio_numbered_fallback,_numbered_single_fallback,_numbered_fallback— so the one code path whoseentire job is "curses died, let the user type a number instead" goes from
curses teardown straight to
input()with the buffer undrained. Stray bytesare read as the answer,
int(val)raisesValueError, and the picker silentlyresolves to its cancel value without the user having answered. On the
Ctrl+C path the bytes survive into whatever the caller prompts for next.
Fixed by wrapping the wrapper call in
try/finally.import cursesstaysoutside the inner
tryso an ImportError on Windows never triggers apointless flush, and
flush_stdinalready no-ops on non-TTY stdin and swallowsits own exceptions, so it cannot mask the error that is unwinding.
2. A raw arrow key during an active search was read as "cancel the search".
The driver reads
key = stdscr.getch()and, when the type-to-filter prompt isopen, hands that raw byte to
_handle_active_search_key, which treats any27as a lone ESC and wipes
search.query._decode_menu_keyexists in this same module precisely because someterminals/terminfo entries deliver cursor keys as raw CSI/SS3 sequences even
with
keypad(True)—getch()returns27, then[, thenA/B— and itdisambiguates with a 60 ms continuation probe. The search branch ran no such
probe. Repro on an affected terminal:
hermes model→/→ typesonnet→press ↓. The query vanishes, the full list snaps back, and the cursor does not
move (the
[andBtail bytes decode toNAV_NONEand are swallowed).Now the driver decodes a
27first and only routes a genuineNAV_CANCEL(probe timed out ⇒ real lone ESC) to
_handle_active_search_key. A decodedarrow becomes the loop's action, so the cursor moves through the filtered list
with the query intact.
_handle_active_search_keykeeps its signature and itslone-ESC semantics, which are still covered.
Related Issue
No filed issue. Direct sibling completion of merged #7167; the interrupt-path
variant of the same drain already merged in #54058 (
flush_stdin()on theturn-interrupt path in
cli.py). The raw-CSI terminal population isrepo-acknowledged — merged #35806 ("ESC + Ghostty") and the dedicated
tests/hermes_cli/test_curses_arrow_keys.py.Type of Change
Changes Made
hermes_cli/curses_ui.py—_run_curses_menu:curses.wrapper(_draw)nowruns under
try/finallywithflush_stdin()in thefinally, so theKeyboardInterrupt and curses-error paths drain too.
hermes_cli/curses_ui.py—_run_curses_menu: while search is active, a raw27is decoded via_decode_menu_keybefore_handle_active_search_keyisallowed to treat it as a lone ESC; the already-consumed sequence is never
decoded twice.
tests/hermes_cli/test_curses_ui_teardown.py(new) — five regression tests,three of which fail on
maintoday.Sibling sweep — deliberately not changed
There are three production
curses.wrapper(...)sites. This PR fixes theshared driver only; the other two are left alone on purpose because I already
have open PRs holding those files, and stacking an unrelated change into them
would make both harder to review:
hermes_cli/curses_ui.py_run_curses_menuhermes_cli/plugins_cmd.py(curses.wrapper+flush_stdinon the normal path only)hermes_cli/main.py_session_browse_picker(never callsflush_stdinon any path)Both have the same defect and are worth a follow-up; I did not want to bundle
them.
Related / Positioning
_handle_active_search_keyand thekey = stdscr.getch()line, but for a different bug — acceptingget_wch()wide characters for CJK input. It adds no
finallyand no ESC continuationprobe. Neither PR is a subset of the other; if both land the textual overlap
is small and mechanical.
_decode_menu_key" idea,but in
hermes_cli/main.py's_session_browse_picker— a different functionin a different file. It does not touch
curses_ui.py.flush_stdinacross open PRs: no results.How to Test
Automated:
Before/after on clean
main(56dc01d904d), three of the five are red:maintest_flush_stdin_runs_when_picker_is_interruptedassert [] == ['flush']test_flush_stdin_runs_before_the_numbered_fallbackassert [0] == [1](fallback sees zero drains)test_raw_arrow_escape_does_not_wipe_the_active_search_queryassert 1 == 2(query wiped, cursor never moved)test_flush_stdin_still_runs_on_the_normal_pathtest_lone_escape_still_stops_the_search_and_restores_the_full_listThe two guards are deliberately green in both directions: one catches a
regression that moves the drain into the exception handlers instead of
widening it to every path, the other pins that a genuine lone ESC still stops
the search and restores the full list.
Manual (bug 2), on a terminal that emits raw CSI cursor keys (Ghostty, some
tmux/SSH terminfo combinations):
hermes model/, typesonnet.moves to the next match and
Search: sonnetstays on the hint line.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passI ran the curses/picker-adjacent suites rather than the full tree:
test_curses_ui_teardown.py,test_curses_ui_search.py,test_curses_arrow_keys.py,test_curses_ui_fuzzy_rank.py,test_curses_color_compat.py,test_setup_menu_curses_migration.py,test_reasoning_effort_menu.py,test_custom_provider_model_switch.py,test_model_switch_custom_providers.py,test_plugins_cmd.py,test_session_browse.py— 126 passed.Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AWindows:
import cursesstays outside the new innertry, so the ImportErrorpath is unchanged and no flush is attempted;
flush_stdinis already a no-opthere. The new test module carries the same module-level
sys.platform == "win32"skip astest_curses_arrow_keys.py.