fix(cli): migrate setup model/provider pickers off simple_term_menu to curses (ESC + Ghostty) - #35795
Closed
kshitijk4poor wants to merge 2 commits into
Closed
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 NousResearch#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
|
Both your commits were cherry-picked onto current
CI was fully green (all 6 test shards + builds + e2e + lint + nix). Thanks for the clean fix and the byte-identical render-harness proof on the refactor. Heads up: #35786 also rewrites these |
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.
What
hermes setup→ select a provider → enter the model selection sub-menu → ESC doesn't back out. Same problem affects three sibling pickers. Root cause: these four menus usesimple_term_menu.TerminalMenu, whose ESC/arrow-key handling is unreliable across terminals — it conflicts with/dev/ttyand, on terminals that emit raw escape sequences (e.g. Ghostty), ESC and arrows misbehave.This is the same class of bug fixed for the curses menus in #35776 / #35786, but these four sites were never on the curses path.
This PR has two commits
1.
fix(cli):migrate the 4simple_term_menupickers tocurses_radiolist(the hardened helper from #35776, decodes raw CSI/SS3 escapes, consistent ESC):auth.py_prompt_model_selectionmain.py_prompt_reasoning_effort_selectionmain.py_model_flow_named_custommain.py_remove_custom_providerThe model picker's pricing column header and unavailable-models block are passed as the radiolist
descriptionso they survive the curses clear.simple_term_menuis no longer imported anywhere.2.
refactor(cli):extract a shared curses menu event-loop driver. The three curses menus (curses_checklist/curses_radiolist/curses_single_select) each hand-rolled an identical loop: color-pair init, clear/getmaxyx/refresh, scroll-offset math, theread_menu_keydispatch + NAV_UP/DOWN wrap,flush_stdin, and the fallback. A terminal-behavior change (Ghostty escapes, scroll, a new key) had to be made in three places._run_curses_menunow owns that boilerplate. Each menu supplies small callbacks for what genuinely differs:draw_header(returns the item-list start row),draw_row([✓]vs(●)vs bare), anon_actionreducer, an optionaldraw_footer(the checklist status bar),reserve_bottom, and the numbered fallback. Duplicated event-loop primitives drop 3 → 1. Future Ghostty/terminal work is a one-place edit.Validation
addnstr(y, x, clamped-text, attr)call across frames + the return value for 6 cases (checklist, checklist+status, radiolist, radiolist+description, single_select, single_select ESC-cancel). Output diffs clean againstorigin/main.input()-based numbered fallback), matching the old per-menu guard — fixing this also cleared a test-isolation failure where an errantinput()polluted later setup tests.test_setup_menu_curses_migration.pyasserts each picker routes throughcurses_radiolist, ESC cancels, and the pricing header is preserved.test_terminal_menu_fallbacks/test_reasoning_effort_menu/test_custom_provider_model_switch/test_model_provider_persistenceto drive the numbered fallback via acurses_radiolisterror (the mock target moved offsimple_term_menu).Net production code shrinks the duplicated surface (3→1 event loops); raw line count is roughly flat because the new driver + docstrings offset the removed inline loops — the win is maintainability, not LOC.