Skip to content

fix(cli): decode arrow keys in curses model menus - #8335

Closed
ESKRiPO wants to merge 7 commits into
NousResearch:mainfrom
ESKRiPO:codex/fix-curses-model-selection-menus
Closed

fix(cli): decode arrow keys in curses model menus#8335
ESKRiPO wants to merge 7 commits into
NousResearch:mainfrom
ESKRiPO:codex/fix-curses-model-selection-menus

Conversation

@ESKRiPO

@ESKRiPO ESKRiPO commented Apr 12, 2026

Copy link
Copy Markdown

What changed

  • made curses menus translate raw arrow-key escape sequences in addition to KEY_UP/KEY_DOWN
  • switched nested model-selection menus off simple_term_menu onto the shared curses menu helpers
  • added regression coverage for raw arrow escape sequences and session-browser navigation

Why

Some terminals were sending arrow keys as raw escape sequences such as ESC [ B, so the initial provider picker could appear to work while later model-selection menus treated navigation input incorrectly. This made hermes model and related setup flows unreliable.

Impact

  • provider and model selection menus now behave consistently across more terminals
  • nested selection flows reuse one input stack instead of mixing curses and simple_term_menu
  • users can navigate the affected menus with arrow keys without the input being misread as cancel or confirm

Root cause

The shared menu code only handled curses.KEY_* values, and some later pickers still used simple_term_menu. In affected terminals that split arrows into escape sequences, the menus interpreted the input incorrectly.

Validation

  • ./.venv/bin/python -m pytest tests/hermes_cli/test_curses_ui_navigation.py tests/hermes_cli/test_session_browse.py tests/hermes_cli/test_setup_prompt_menus.py tests/hermes_cli/test_plugins_cmd.py -q

@ESKRiPO
ESKRiPO marked this pull request as ready for review April 12, 2026 19:13
@ESKRiPO ESKRiPO changed the title [codex] Fix curses navigation in model selection menus fix(cli): decode arrow keys in curses model menus Apr 12, 2026
@ESKRiPO
ESKRiPO force-pushed the codex/fix-curses-model-selection-menus branch from 612af9f to 59e466a Compare April 19, 2026 14:42
@ESKRiPO

ESKRiPO commented Apr 19, 2026

Copy link
Copy Markdown
Author

Rebased on latest main — conflicts resolved and PR is now mergeable again. Both the new navigation tests (tests/hermes_cli/test_curses_ui_navigation.py) and test_session_browse.py pass locally.

Quick summary of what this fixes: on some terminals the curses model/provider pickers would eat arrow-key CSI/SS3 escape sequences and jump to odd rows (or drop input entirely). This PR decodes those sequences explicitly so up/down behave consistently, and adds a non-TTY fallback for the single-select path.

@teknium1 would appreciate a look when you get a moment — this touches the same area as #7167 (flush_stdin after terminal menus), so it felt like the natural follow-up. Happy to iterate on anything.

Two regressions surfaced when reviewing the switch from simple_term_menu
to curses_single_select for model/provider pickers:

1. _prompt_model_selection built a multi-line title containing a priced
   column header ("In  Out  /Mtok"), but curses_single_select rendered
   the whole title on row 0 and immediately overwrote the second line
   with the navigation hint. Priced menus thus lost their column legend.

2. The fallback path displayed unavailable paid-tier models plus an
   upgrade URL, but the curses path silently dropped both and shortened
   the title to "(free models)".

curses_single_select now splits the title by newlines and renders each
line on its own row, and accepts an optional footer_lines list rendered
dimly below the items. _prompt_model_selection passes the unavailable
models and upgrade hint through footer_lines so the information is
preserved in the curses menu.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ESKRiPO

ESKRiPO commented Apr 19, 2026

Copy link
Copy Markdown
Author

Follow-up: pushed a third commit addressing two regressions caught in self-review of the curses switch:

  1. Multi-line title rendering_prompt_model_selection builds a multi-line title that embeds the In / Out / Cache /Mtok column header. The previous curses_single_select drew the whole title on row 0 and then overwrote row 1 with the navigation hint, so priced menus lost their legend. The helper now splits the title by newlines and renders each line on its own row before the hint.
  2. Unavailable-model disclosure preserved — the fallback path printed a dim "Unavailable models (requires paid tier — upgrade at …)" block, but the curses path silently dropped it and truncated the title to (free models). Added an optional footer_lines argument to curses_single_select (rendered dimly below the items) and pipe the unavailable models + upgrade URL through it.

Added two unit tests covering both behaviors in tests/hermes_cli/test_curses_ui_navigation.py. All 38 curses-UI / session-browse tests pass locally.

Two follow-up regressions surfaced in review:

1. read_curses_key queued the tail bytes of unrecognized CSI/SS3
   sequences (Delete = ESC [ 3 ~, Home = ESC [ H, End = ESC [ F, etc.)
   back into _PENDING_KEYS and returned the leading ESC. The session-
   browse picker then read ESC (clearing an active search filter) and
   injected the leftover bytes as printable characters. Now read the
   sequence through to its terminator (any byte in 0x40–0x7E for CSI,
   one byte for SS3) and return a neutral 0 when the key is not an
   arrow, so the menu/filter loop ignores it.

2. curses_single_select previously reserved one row per footer line,
   so a provider reporting ~20 unavailable paid models shrank the
   selectable list to a single row on a standard 24-row terminal.
   Cap the footer at one-third of the rows below the hint, clip the
   overflow, and annotate the last shown footer line with a
   "(+N more)" hint so users know more information is available in
   the numbered fallback.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ESKRiPO

ESKRiPO commented Apr 19, 2026

Copy link
Copy Markdown
Author

Follow-up round 2 — self-review caught two more curses regressions:

  1. Non-arrow escape sequences were leaking into input buffers. When keypad(True) doesn't translate keys like Delete/Home/End, the terminal delivers their raw CSI form (ESC [ 3 ~, ESC [ H, ESC [ F). read_curses_key() previously recognized only arrows and requeued the tail bytes back into _PENDING_KEYS — so pressing Delete while the session-browse filter was active first cleared the search (via the ESC branch) and then injected [3~ as printable chars into it. Now the decoder reads through to the CSI terminator (any byte in 0x40–0x7E) / the single SS3 function byte and returns a neutral 0 for unrecognized sequences, so menu/filter loops simply ignore them.

  2. Long unavailable-model footers were crushing the selectable list. visible_rows subtracted the full len(footer) up front, so a provider with ~20 unavailable paid models left only one selectable row on a 24-row terminal. The footer is now capped at one-third of the rows below the hint, overflow is clipped, and the last shown footer line is annotated with (+N more) so users know extra info is available in the numbered fallback.

Added two more unit tests: one verifies that a Delete-style ESC [ 3 ~ sequence neither returns ESC nor leaves bytes in the pending buffer; the other checks that a 40-line footer still leaves at least 10 items visible on a 24-row terminal. All 41 curses-UI / session-browse tests pass locally.

Three more regressions surfaced in review:

1. curses_single_select fell back to _numbered_single_fallback when
   stdin was not a TTY, which calls input() — so piped/headless
   invocations would block on or consume piped bytes. Now returns None
   directly, matching curses_checklist's cancel-on-headless behavior.

2. The arrow-key decoder polled with nodelay(True), which returns -1
   immediately when the continuation byte has not yet arrived. On
   slower SSH/tmux PTYs that delivered ESC, [, A across separate
   reads the decoder misread the whole sequence as a bare Escape and
   cancelled the picker. Switched to stdscr.timeout(50) so we wait up
   to 50 ms for each continuation byte before giving up.

3. _prompt_model_selection's priced-model header still used pad=5
   from the old simple_term_menu layout (3-char cursor + 2-char item
   prefix). curses_single_select renders rows as " {arrow} {label}"
   (3-char prefix), so the header's In/Out labels were shifted past
   their values. Pad is now 3, and a regression test verifies header
   right-edge alignment with the first priced column.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ESKRiPO

ESKRiPO commented Apr 19, 2026

Copy link
Copy Markdown
Author

Follow-up round 3 — self-review caught three more regressions:

  1. [P2] Headless stdin now returns cancel instead of blocking. curses_single_select previously fell back to _numbered_single_fallback when stdin wasn't a TTY, which internally calls input() — so CI / piped invocations could block or swallow piped bytes. It now returns None directly, matching curses_checklist's long-standing cancel-on-headless behavior.

  2. [P2] Split ESC sequences on slow PTYs are no longer misread as cancel. The arrow-key decoder was polling with nodelay(True), which returns -1 immediately when the continuation byte hadn't arrived yet. On slower SSH/tmux PTYs where ESC, [, A arrive across three separate reads, the decoder saw just ESC and cancelled the picker. Switched to stdscr.timeout(50) so we wait up to 50 ms for each continuation byte before giving up — enough for SSH jitter but not perceptible to users pressing ESC alone.

  3. [P3] Priced-model column header re-aligned. The header pad was still 5 (old simple_term_menu layout = 3-char cursor + 2-char label prefix), but curses_single_select renders rows as " {arrow} {label}" (3-char prefix). That shifted the In/Out/Cache labels past their values. Pad is now 3; added a regression test that verifies the header's In right edge aligns with the first priced column in an item row.

All 45 curses-UI / session-browse / reasoning-effort-menu tests pass locally.

The previous commit short-circuited curses_single_select() to return
None when stdin is not a TTY, on the theory that matching
curses_checklist's cancel-on-headless behavior was safer. It is not:
several selection flows are exercised in tests and scripted wrappers
by piping a numeric choice through stdin (test_terminal_menu_fallbacks
and test_custom_provider_model_switch drive _prompt_model_selection(),
_prompt_reasoning_effort_selection(), _remove_custom_provider(), and
_model_flow_named_custom() this way). Returning None surfaced as a
silent "Cancelled." for every one of those paths.

Go back to dispatching into _numbered_single_fallback(): it prints the
numbered prompt, reads one line via input(), and already catches
EOFError so a truly detached stdin still yields a clean cancel
without blocking. Added two regression tests — one asserts a piped
"2" selects index 1, the other asserts EOFError surfaces as None.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ESKRiPO

ESKRiPO commented Apr 19, 2026

Copy link
Copy Markdown
Author

Follow-up round 4 — self-review caught a P1 regression that the previous round introduced:

The previous "make curses_single_select return None on non-TTY" change (meant to match curses_checklist's cancel-on-headless behavior) broke scripted callers. Four Hermes CLI test suites pipe a numeric choice through stdin into _prompt_model_selection / _prompt_reasoning_effort_selection / _remove_custom_provider / _model_flow_named_custom and expect that number to resolve the menu. Returning None turned every one of those flows into a silent Cancelled. / No change., and test_terminal_menu_fallbacks + test_custom_provider_model_switch failed accordingly.

Reverted to dispatching into _numbered_single_fallback() when stdin isn't a TTY. That helper prints the numbered prompt, reads one line via input(), and already catches EOFError — so scripted callers with buffered input keep working, and truly detached contexts (closed stdin, /dev/null) still cancel cleanly without blocking.

Added two regression tests: one asserts a piped "2" resolves to index 1 via the numbered fallback, the other asserts EOFError surfaces as None. All 56 curses-UI / session-browse / reasoning-effort / terminal-menu-fallback / custom-provider-model-switch tests pass locally.

Honest lesson from this round: the "simple" arrow-key fix is really a simple_term_menu → home-grown curses_single_select migration, and each round of review has surfaced cases the old library silently handled — headless input routing being the latest. Keeping the iteration transparent here rather than squashing so the history shows the correctness trajectory.

1. _prompt_model_selection wrapped the entire curses flow (including
   the custom-model input() call) in a broad except-Exception.
   Ctrl-D / pipe-close during "Enter model name" raised EOFError,
   which was swallowed and the function fell through to the numbered
   fallback, redrawing the whole menu. EOF now cancels cleanly by
   catching EOFError/KeyboardInterrupt at the input call.

2. curses_single_select subtracted footer_shown from visible_rows but
   rendered the footer starting at last_item_row + 2, which costs a
   blank separator row that was not budgeted. On a 6-row tmux split
   the reserved-but-never-rendered footer slot shrank the selectable
   list for no visible gain. Now add the separator to the budget and
   skip the footer entirely when the terminal is too short to fit
   even one footer row after the separator.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ESKRiPO

ESKRiPO commented Apr 19, 2026

Copy link
Copy Markdown
Author

Follow-up round 5 — two P3 fixes:

  1. EOF from custom-model entry now cancels cleanly. _prompt_model_selection wrapped the whole curses flow (including the input("Enter model name: ") call) in a broad except Exception. Ctrl-D / pipe-close during that input raised EOFError, which got swallowed and the function fell through to the numbered fallback — redrawing the menu a second time. Now catches EOFError/KeyboardInterrupt at the input() call so EOF surfaces as an immediate cancel.

  2. Footer separator row now budgeted correctly. curses_single_select subtracted footer_shown from visible_rows but rendered the footer starting at last_item_row + 2 — spending a blank separator row that wasn't accounted for. On a 6-row tmux split the reserved-but-never-rendered footer slot shrank the selectable list for no visible gain. Now the separator is included in the budget, and the footer is skipped entirely when the terminal is too short for even one footer row.

Added two regression tests: one asserts that an EOF during custom-entry returns None without re-entering the picker; the other asserts that on a 6-row terminal the 3 items stay visible and no footer-* line is drawn. All 58 relevant tests pass locally.

@ESKRiPO ESKRiPO left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok

@ESKRiPO ESKRiPO closed this Apr 24, 2026
@ESKRiPO
ESKRiPO deleted the codex/fix-curses-model-selection-menus branch April 24, 2026 16:58
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Apr 24, 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants