fix(cli): migrate setup model/provider pickers off simple_term_menu to curses (ESC + Ghostty) - #35806
Merged
Conversation
…o curses The setup provider->model sub-menu (and three sibling pickers) used simple_term_menu.TerminalMenu, whose ESC and arrow-key handling was unreliable across terminals — notably ESC failed to back out of the model selection list on terminals that emit raw escape sequences (e.g. Ghostty). The codebase already notes simple_term_menu 'conflicts with /dev/tty' and causes 'ghost-duplication rendering', and a prior attempt to migrate these (closed PR) confirmed the same root cause. Route all four single-select pickers through the shared, already-hardened curses_radiolist (which decodes raw CSI/SS3 escape sequences and handles ESC consistently, fixed in #35776): - auth.py _prompt_model_selection — model picker; the pricing column header and the unavailable-models block are passed as the radiolist description so they survive the curses screen clear. ESC now cancels. - main.py _prompt_reasoning_effort_selection — reasoning-effort picker. - main.py _model_flow_named_custom — named custom-provider model picker. - main.py _remove_custom_provider — provider-removal picker. simple_term_menu is no longer imported anywhere (only stale comments referenced it; one in setup.py is corrected). The numbered-input fallbacks are unchanged and still trigger on curses errors / non-TTY. Tests: updated test_terminal_menu_fallbacks / test_reasoning_effort_menu / test_custom_provider_model_switch / test_model_provider_persistence to drive the fallback via curses_radiolist errors instead of breaking simple_term_menu. New test_setup_menu_curses_migration.py asserts each picker routes through curses_radiolist, ESC cancels, and the pricing header is preserved. Net -147/+183 (mostly the new test file; production code shrinks by removing TerminalMenu boilerplate).
The three curses menus (curses_checklist / curses_radiolist / curses_single_select) each hand-rolled an identical event loop: cursor hide + color-pair init, the per-frame clear/getmaxyx/refresh cycle, scroll-offset math, row iteration, the read_menu_key dispatch with NAV_UP/NAV_DOWN cursor wrap, flush_stdin, and the KeyboardInterrupt/curses-unavailable fallback. Terminal-behavior changes (e.g. Ghostty raw-escape handling, scroll tweaks, a new key) had to be made in three places. Extract that boilerplate into one _run_curses_menu driver. Each public menu now supplies small callbacks for the parts that genuinely differ: draw_header (returns the item-list start row), draw_row (checkbox vs radio vs bare prefix), an on_action reducer (toggle-set vs return-cursor vs return-None + the single_select cancel-row guard), an optional draw_footer (the checklist status bar), reserve_bottom, and the numbered fallback. Behavior is passed as functions; the loop is the only stateful piece — so future terminal/Ghostty work is a one-place edit. Duplicated event-loop primitives drop 3 -> 1 (stdscr.clear, read_menu_key dispatch, scroll math). Verified byte-identical: a render harness records every addnstr(y, x, clamped-text, attr) call across frames plus the return value for 6 cases (checklist, checklist+status, radiolist, radiolist+description, single_select, single_select ESC-cancel); output diffs clean against origin/main. Non-TTY returns the cancel value directly (not the input()-based numbered fallback), matching the old per-menu guard. 150 menu/setup/browse/plugins tests pass.
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
call-non-callable |
1 |
First entries
hermes_cli/curses_ui.py:283: [call-non-callable] call-non-callable: Object of type `None` is not callable
✅ Fixed issues (3):
| Rule | Count |
|---|---|
unresolved-import |
2 |
not-subscriptable |
1 |
First entries
hermes_cli/auth.py:6131: [unresolved-import] unresolved-import: Cannot resolve imported module `simple_term_menu`
tests/hermes_cli/test_reasoning_effort_menu.py:29: [not-subscriptable] not-subscriptable: Cannot subscript object of type `None` with no `__getitem__` method
hermes_cli/main.py:4738: [unresolved-import] unresolved-import: Cannot resolve imported module `simple_term_menu`
Unchanged: 4949 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
alt-glitch
added
type/refactor
Collaborator
1 task
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
3 participants
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.
Summary
hermes setup's model picker (and 3 sibling pickers) now back out on ESC across all terminals, including Ghostty. Salvage of #35795 by @kshitijk4poor onto currentmain.Root cause: those 4 menus were the last sites still using
simple_term_menu.TerminalMenu, whose ESC/arrow handling conflicts with/dev/ttyand breaks on terminals that emit raw escape sequences. Same bug class fixed for the curses menus in #35776 — these sites were never on the curses path.Changes
curses_radiolist:auth.py_prompt_model_selection— the reported bug; pricing header + unavailable-models block now passed as the radiolistdescriptionso they survive the curses clearmain.py_prompt_reasoning_effort_selection,_model_flow_named_custom,_remove_custom_providersimple_term_menuis no longer imported anywhere; stale comment insetup.pycorrected_run_curses_menuevent-loop driver — the 3 curses menus (curses_checklist/curses_radiolist/curses_single_select) hand-rolled an identical loop. Duplicated event-loop primitives drop 3 → 1; future terminal-behavior edits land in one place.Validation
test_setup_menu_curses_migration+ updated fallback/menu/persistence tests)input()hang — matches the old per-menu guardorigin/mainSalvages #35795. Contributor authorship preserved via cherry-pick.
Infographic