From 3463c97a362cc99f30174519f21ec15b98e835e9 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 31 May 2026 13:59:56 +0530 Subject: [PATCH] fix(cli): decode raw arrow-key escape sequences in curses menus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The setup wizard's provider/model pickers (curses_radiolist via prompt_choice) bailed to the numbered "Select [1-N]" fallback the moment a user pressed up or down. Root cause: even with keypad(True) — which curses.wrapper sets — many terminals/terminfo entries deliver cursor keys to getch() as raw CSI/SS3 byte sequences (e.g. 27, 91, 66 for arrow-down) rather than the translated curses.KEY_DOWN. The menus matched only curses.KEY_UP/KEY_DOWN and treated the leading 27 (ESC) as cancel, so navigation dropped into the text fallback and the trailing bytes leaked into the next input(). Add a shared read_menu_key() helper that decodes CSI/SS3 escape sequences into normalized NAV_* actions (only a lone ESC, with no continuation byte within a short timeout, still cancels) and consumes the tail of unhandled sequences so stray bytes can't corrupt later input(). Route all three curses menus (checklist, radiolist, single_select) through it. Add regression tests covering raw CSI/SS3 arrows, translated KEY_* constants, vim keys, lone-ESC cancel, and full consumption of unhandled sequences (Delete/Home/End). --- hermes_cli/curses_ui.py | 104 +++++++++++++++++---- tests/hermes_cli/test_curses_arrow_keys.py | 102 ++++++++++++++++++++ 2 files changed, 190 insertions(+), 16 deletions(-) create mode 100644 tests/hermes_cli/test_curses_arrow_keys.py diff --git a/hermes_cli/curses_ui.py b/hermes_cli/curses_ui.py index f0e991c0ae2..e2c2af62647 100644 --- a/hermes_cli/curses_ui.py +++ b/hermes_cli/curses_ui.py @@ -32,6 +32,78 @@ def flush_stdin() -> None: pass +# Normalized menu actions returned by ``read_menu_key``. Using sentinels keeps +# every menu's key-handling branch identical and free of raw escape-byte logic. +NAV_UP = "up" +NAV_DOWN = "down" +NAV_SELECT = "select" +NAV_TOGGLE = "toggle" +NAV_CANCEL = "cancel" +NAV_NONE = "none" + + +def read_menu_key(stdscr) -> str: + """Read one keypress and normalize it to a menu action. + + Decodes raw arrow-key escape sequences in addition to the translated + ``curses.KEY_*`` values. Even with ``keypad(True)`` (which + ``curses.wrapper`` sets), some terminals/terminfo entries deliver cursor + keys as raw CSI/SS3 byte sequences — ``getch()`` then returns ``27`` (ESC) + followed by e.g. ``[`` ``A``. Treating that leading ``27`` as a cancel is + what made the setup wizard's provider/model pickers bail to the numbered + fallback the moment a user pressed up/down. + + Returns one of the ``NAV_*`` constants. A lone ESC (no continuation byte + within a short window) is the only thing that maps to ``NAV_CANCEL`` via + the escape path; ``q`` also cancels. Unknown sequences map to + ``NAV_NONE`` so the caller simply ignores them rather than misfiring. + """ + import curses + + key = stdscr.getch() + + if key in (curses.KEY_UP, ord("k")): + return NAV_UP + if key in (curses.KEY_DOWN, ord("j")): + return NAV_DOWN + if key in (curses.KEY_ENTER, 10, 13): + return NAV_SELECT + if key == ord(" "): + return NAV_TOGGLE + if key == ord("q"): + return NAV_CANCEL + + if key == 27: # ESC — could be a lone ESC (cancel) or an escape sequence. + # Wait briefly for a continuation byte. On slow PTYs (SSH/tmux) the + # bytes of an arrow key can arrive across separate reads, so a tiny + # timeout avoids misreading a split sequence as a bare ESC. + try: + stdscr.timeout(60) + nxt = stdscr.getch() + finally: + stdscr.timeout(-1) # restore blocking mode + + if nxt == -1: + return NAV_CANCEL # genuine lone ESC + + if nxt in (ord("["), ord("O")): # CSI / SS3 introducer + final = stdscr.getch() + if final in (ord("A"), ord("k")): + return NAV_UP + if final in (ord("B"), ord("j")): + return NAV_DOWN + # Consume the tail of any other CSI sequence (e.g. ``[3~`` Delete, + # ``[H`` Home) up to its terminator so stray bytes don't leak into + # the next input() and corrupt it. + while 0x20 <= final <= 0x3F: # CSI parameter/intermediate bytes + final = stdscr.getch() + return NAV_NONE + # ESC followed by some other byte we don't handle — swallow it. + return NAV_NONE + + return NAV_NONE + + def curses_checklist( title: str, items: List[str], @@ -137,18 +209,18 @@ def curses_checklist( pass stdscr.refresh() - key = stdscr.getch() + action = read_menu_key(stdscr) - if key in {curses.KEY_UP, ord("k")}: + if action == NAV_UP: cursor = (cursor - 1) % len(items) - elif key in {curses.KEY_DOWN, ord("j")}: + elif action == NAV_DOWN: cursor = (cursor + 1) % len(items) - elif key == ord(" "): + elif action == NAV_TOGGLE: chosen.symmetric_difference_update({cursor}) - elif key in {curses.KEY_ENTER, 10, 13}: + elif action == NAV_SELECT: result_holder[0] = set(chosen) return - elif key in {27, ord("q")}: + elif action == NAV_CANCEL: result_holder[0] = cancel_returns return @@ -263,16 +335,16 @@ def curses_radiolist( pass stdscr.refresh() - key = stdscr.getch() + action = read_menu_key(stdscr) - if key in {curses.KEY_UP, ord("k")}: + if action == NAV_UP: cursor = (cursor - 1) % len(items) - elif key in {curses.KEY_DOWN, ord("j")}: + elif action == NAV_DOWN: cursor = (cursor + 1) % len(items) - elif key in {ord(" "), curses.KEY_ENTER, 10, 13}: + elif action in (NAV_SELECT, NAV_TOGGLE): result_holder[0] = cursor return - elif key in {27, ord("q")}: + elif action == NAV_CANCEL: result_holder[0] = cancel_returns return @@ -386,16 +458,16 @@ def curses_single_select( pass stdscr.refresh() - key = stdscr.getch() + action = read_menu_key(stdscr) - if key in {curses.KEY_UP, ord("k")}: + if action == NAV_UP: cursor = (cursor - 1) % len(all_items) - elif key in {curses.KEY_DOWN, ord("j")}: + elif action == NAV_DOWN: cursor = (cursor + 1) % len(all_items) - elif key in {curses.KEY_ENTER, 10, 13}: + elif action == NAV_SELECT: result_holder[0] = cursor return - elif key in {27, ord("q")}: + elif action == NAV_CANCEL: result_holder[0] = None return diff --git a/tests/hermes_cli/test_curses_arrow_keys.py b/tests/hermes_cli/test_curses_arrow_keys.py new file mode 100644 index 00000000000..c1bafbd8c3d --- /dev/null +++ b/tests/hermes_cli/test_curses_arrow_keys.py @@ -0,0 +1,102 @@ +"""Regression tests for arrow-key decoding in the curses menus. + +Root cause these guard against: on many terminals/terminfo entries, cursor +keys are delivered to ``getch()`` as raw CSI/SS3 escape byte sequences +(``27, 91, 66`` for arrow-down) even when ``keypad(True)`` is set. The menus +used to treat the leading ``27`` as ESC/cancel, which dumped the setup wizard's +provider/model picker into its numbered "Select [1-N]" fallback the instant a +user pressed up or down. +""" +import curses + +from hermes_cli.curses_ui import ( + NAV_CANCEL, + NAV_DOWN, + NAV_NONE, + NAV_SELECT, + NAV_UP, + read_menu_key, +) + + +class FakeStdscr: + """Minimal stdscr stand-in that replays a queue of getch() byte returns. + + ``getch`` pops from ``keys``; an empty queue yields ``-1`` (matching curses + non-blocking behavior). ``timeout`` is recorded but otherwise inert. + """ + + def __init__(self, keys): + self.keys = list(keys) + self.timeouts = [] + + def getch(self): + return self.keys.pop(0) if self.keys else -1 + + def timeout(self, ms): + self.timeouts.append(ms) + + +def test_raw_csi_arrow_down_decodes_to_down(): + # ESC [ B -> down, NOT cancel + assert read_menu_key(FakeStdscr([27, ord("["), ord("B")])) == NAV_DOWN + + +def test_raw_csi_arrow_up_decodes_to_up(): + # ESC [ A -> up + assert read_menu_key(FakeStdscr([27, ord("["), ord("A")])) == NAV_UP + + +def test_raw_ss3_arrow_keys_decode(): + # Application cursor mode: ESC O B / ESC O A + assert read_menu_key(FakeStdscr([27, ord("O"), ord("B")])) == NAV_DOWN + assert read_menu_key(FakeStdscr([27, ord("O"), ord("A")])) == NAV_UP + + +def test_translated_key_constants_still_work(): + assert read_menu_key(FakeStdscr([curses.KEY_DOWN])) == NAV_DOWN + assert read_menu_key(FakeStdscr([curses.KEY_UP])) == NAV_UP + + +def test_vim_keys(): + assert read_menu_key(FakeStdscr([ord("j")])) == NAV_DOWN + assert read_menu_key(FakeStdscr([ord("k")])) == NAV_UP + + +def test_lone_escape_is_cancel(): + # ESC with no continuation byte (getch returns -1) -> genuine cancel. + assert read_menu_key(FakeStdscr([27])) == NAV_CANCEL + + +def test_q_is_cancel(): + assert read_menu_key(FakeStdscr([ord("q")])) == NAV_CANCEL + + +def test_enter_variants_select(): + assert read_menu_key(FakeStdscr([10])) == NAV_SELECT + assert read_menu_key(FakeStdscr([13])) == NAV_SELECT + assert read_menu_key(FakeStdscr([curses.KEY_ENTER])) == NAV_SELECT + + +def test_unhandled_csi_sequence_is_consumed_and_ignored(): + # Delete key (ESC [ 3 ~): must be swallowed whole and map to NAV_NONE so + # its tail bytes don't leak into a subsequent input() call. + fake = FakeStdscr([27, ord("["), ord("3"), ord("~"), ord("X")]) + assert read_menu_key(fake) == NAV_NONE + # The trailing 'X' (a genuinely separate keypress) must remain unconsumed. + assert fake.keys == [ord("X")] + + +def test_home_end_csi_sequences_ignored(): + # ESC [ H (Home) and ESC [ F (End) -> NAV_NONE, fully consumed. + assert read_menu_key(FakeStdscr([27, ord("["), ord("H")])) == NAV_NONE + assert read_menu_key(FakeStdscr([27, ord("["), ord("F")])) == NAV_NONE + + +def test_escape_uses_short_timeout_then_restores_blocking(): + fake = FakeStdscr([27, ord("["), ord("B")]) + read_menu_key(fake) + # A short positive timeout is set to wait for the continuation byte, then + # blocking mode (-1) is restored. + assert fake.timeouts[0] > 0 + assert fake.timeouts[-1] == -1