fix(cli): migrate setup model/provider pickers off simple_term_menu to 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).
This commit is contained in:
@@ -45,7 +45,7 @@ class TestCustomProviderModelSwitch:
|
||||
}
|
||||
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=["model-A", "model-B"]) as mock_fetch, \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="2"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -70,7 +70,7 @@ class TestCustomProviderModelSwitch:
|
||||
}
|
||||
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=["model-A", "model-B"]), \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="2"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -116,7 +116,7 @@ class TestCustomProviderModelSwitch:
|
||||
}
|
||||
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=["model-X"]), \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -140,7 +140,7 @@ class TestCustomProviderModelSwitch:
|
||||
}
|
||||
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=["claude-3"]) as mock_fetch, \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -173,7 +173,7 @@ class TestCustomProviderModelSwitch:
|
||||
}
|
||||
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=["llama-3"]), \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -210,7 +210,7 @@ class TestCustomProviderModelSwitch:
|
||||
}
|
||||
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=["qwen3.6-35b-fast"]) as mock_fetch, \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -251,7 +251,7 @@ class TestCustomProviderModelSwitch:
|
||||
}
|
||||
|
||||
with patch("hermes_cli.models.fetch_api_models", return_value=["qwen3.6-35b-fast"]), \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -309,7 +309,7 @@ class TestCustomProviderModelSwitch:
|
||||
side_effect=_pick_neuralwatt), \
|
||||
patch("hermes_cli.models.fetch_api_models",
|
||||
return_value=["qwen3.6-35b-fast"]) as mock_fetch, \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
select_provider_and_model()
|
||||
@@ -422,7 +422,7 @@ class TestCustomProviderModelSwitch:
|
||||
side_effect=_pick_neuralwatt), \
|
||||
patch("hermes_cli.models.fetch_api_models",
|
||||
return_value=["qwen3.6-35b-fast"]) as mock_fetch, \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
select_provider_and_model()
|
||||
@@ -486,7 +486,7 @@ class TestCustomProviderModelSwitch:
|
||||
"hermes_cli.models.fetch_api_models",
|
||||
return_value=["claude-opus-4-7"],
|
||||
) as mock_fetch, \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
@@ -551,7 +551,7 @@ class TestCustomProviderModelSwitch:
|
||||
"hermes_cli.models.fetch_api_models",
|
||||
return_value=["claude-opus-4-7"],
|
||||
), \
|
||||
patch.dict("sys.modules", {"simple_term_menu": None}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=ImportError), \
|
||||
patch("builtins.input", return_value="1"), \
|
||||
patch("builtins.print"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
|
||||
@@ -191,14 +191,12 @@ class TestProviderPersistsAfterModelSave:
|
||||
}
|
||||
|
||||
# Patch fetch_api_models so the named custom flow returns one model;
|
||||
# patch simple_term_menu to force the input() fallback; patch input to
|
||||
# auto-select the first model from the fallback prompt.
|
||||
fake_menu_module = MagicMock()
|
||||
fake_menu_module.TerminalMenu.side_effect = OSError("no tty in test")
|
||||
# force the curses menu to error so the input() fallback runs; patch
|
||||
# input to auto-select the first model from the fallback prompt.
|
||||
with patch("hermes_cli.auth._save_model_choice"), \
|
||||
patch("hermes_cli.auth.deactivate_provider"), \
|
||||
patch("hermes_cli.models.fetch_api_models", return_value=["gpt-5.4"]), \
|
||||
patch.dict("sys.modules", {"simple_term_menu": fake_menu_module}), \
|
||||
patch("hermes_cli.curses_ui.curses_radiolist", side_effect=OSError("no tty in test")), \
|
||||
patch("builtins.input", return_value="1"):
|
||||
_model_flow_named_custom({}, provider_info)
|
||||
|
||||
|
||||
@@ -1,24 +1,15 @@
|
||||
import sys
|
||||
import types
|
||||
|
||||
|
||||
from hermes_cli.main import _prompt_reasoning_effort_selection
|
||||
|
||||
|
||||
class _FakeTerminalMenu:
|
||||
last_choices = None
|
||||
|
||||
def __init__(self, choices, **kwargs):
|
||||
_FakeTerminalMenu.last_choices = choices
|
||||
self._cursor_index = kwargs.get("cursor_index")
|
||||
|
||||
def show(self):
|
||||
return self._cursor_index
|
||||
|
||||
|
||||
def test_reasoning_menu_orders_minimal_before_low(monkeypatch):
|
||||
fake_module = types.SimpleNamespace(TerminalMenu=_FakeTerminalMenu)
|
||||
monkeypatch.setitem(sys.modules, "simple_term_menu", fake_module)
|
||||
captured = {}
|
||||
|
||||
def _fake_radiolist(title, items, *, selected=0, cancel_returns=None, description=None):
|
||||
captured["items"] = items
|
||||
captured["selected"] = selected
|
||||
return selected # pick the pre-selected (current) entry
|
||||
|
||||
monkeypatch.setattr("hermes_cli.curses_ui.curses_radiolist", _fake_radiolist)
|
||||
|
||||
selected = _prompt_reasoning_effort_selection(
|
||||
["low", "minimal", "medium", "high"],
|
||||
@@ -26,9 +17,9 @@ def test_reasoning_menu_orders_minimal_before_low(monkeypatch):
|
||||
)
|
||||
|
||||
assert selected == "medium"
|
||||
assert _FakeTerminalMenu.last_choices[:4] == [
|
||||
" minimal",
|
||||
" low",
|
||||
" medium ← currently in use",
|
||||
" high",
|
||||
assert captured["items"][:4] == [
|
||||
"minimal",
|
||||
"low",
|
||||
"medium ← currently in use",
|
||||
"high",
|
||||
]
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
"""Regression tests confirming the setup model/provider/reasoning pickers route
|
||||
through the shared curses radiolist (ESC + arrow-key handling that works across
|
||||
terminals, incl. Ghostty) instead of simple_term_menu.
|
||||
|
||||
Guards against silently regressing back to simple_term_menu, whose ESC/arrow
|
||||
handling was unreliable in `hermes setup` (the provider->model sub-menu).
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
|
||||
|
||||
def test_prompt_model_selection_uses_curses_radiolist():
|
||||
from hermes_cli.auth import _prompt_model_selection
|
||||
|
||||
seen = {}
|
||||
|
||||
def _fake(title, items, *, selected=0, cancel_returns=None, description=None):
|
||||
seen["title"] = title
|
||||
seen["items"] = items
|
||||
return 1 # pick second model
|
||||
|
||||
with patch("hermes_cli.curses_ui.curses_radiolist", side_effect=_fake), \
|
||||
patch("builtins.print"):
|
||||
result = _prompt_model_selection(["model-a", "model-b"])
|
||||
|
||||
assert result == "model-b"
|
||||
assert seen["title"] == "Select default model:"
|
||||
# Items are the models plus the custom/skip entries.
|
||||
assert seen["items"][:2] == ["model-a", "model-b"]
|
||||
assert "Skip (keep current)" in seen["items"]
|
||||
|
||||
|
||||
def test_prompt_model_selection_esc_cancels():
|
||||
from hermes_cli.auth import _prompt_model_selection
|
||||
|
||||
# curses_radiolist returns the cancel sentinel (-1) on ESC.
|
||||
with patch("hermes_cli.curses_ui.curses_radiolist", return_value=-1), \
|
||||
patch("builtins.print"):
|
||||
result = _prompt_model_selection(["model-a", "model-b"])
|
||||
|
||||
assert result is None
|
||||
|
||||
|
||||
def test_reasoning_effort_uses_curses_radiolist():
|
||||
from hermes_cli.main import _prompt_reasoning_effort_selection
|
||||
|
||||
with patch("hermes_cli.curses_ui.curses_radiolist", return_value=2), \
|
||||
patch("builtins.print"):
|
||||
result = _prompt_reasoning_effort_selection(["low", "medium", "high"], current_effort="")
|
||||
|
||||
assert result == "high"
|
||||
|
||||
|
||||
def test_reasoning_effort_esc_cancels():
|
||||
from hermes_cli.main import _prompt_reasoning_effort_selection
|
||||
|
||||
with patch("hermes_cli.curses_ui.curses_radiolist", return_value=-1), \
|
||||
patch("builtins.print"):
|
||||
result = _prompt_reasoning_effort_selection(["low", "medium", "high"], current_effort="")
|
||||
|
||||
assert result is None
|
||||
|
||||
|
||||
def test_model_selection_with_pricing_passes_description():
|
||||
"""When pricing is supplied, the aligned header is passed as the curses
|
||||
description (multi-line text above the list), not lost."""
|
||||
from hermes_cli.auth import _prompt_model_selection
|
||||
|
||||
seen = {}
|
||||
|
||||
def _fake(title, items, *, selected=0, cancel_returns=None, description=None):
|
||||
seen["description"] = description
|
||||
return len(items) - 1 # Skip
|
||||
|
||||
pricing = {
|
||||
"model-a": {"prompt": "0.000001", "completion": "0.000002"},
|
||||
"model-b": {"prompt": "0.000003", "completion": "0.000004"},
|
||||
}
|
||||
with patch("hermes_cli.curses_ui.curses_radiolist", side_effect=_fake), \
|
||||
patch("builtins.print"):
|
||||
_prompt_model_selection(["model-a", "model-b"], pricing=pricing)
|
||||
|
||||
# The description should carry the In/Out price header.
|
||||
assert seen["description"] is not None
|
||||
assert "In" in seen["description"] and "Out" in seen["description"]
|
||||
@@ -1,25 +1,21 @@
|
||||
"""Regression tests for numbered fallbacks when TerminalMenu cannot initialize."""
|
||||
"""Regression tests for numbered fallbacks when the interactive curses menu
|
||||
cannot initialize (e.g. non-TTY, curses unavailable, terminal error)."""
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
import types
|
||||
|
||||
from hermes_cli.config import load_config, save_config
|
||||
|
||||
|
||||
class _BrokenTerminalMenu:
|
||||
def __init__(self, *args, **kwargs):
|
||||
raise subprocess.CalledProcessError(2, ["tput", "clear"])
|
||||
def _raise_menu(*args, **kwargs):
|
||||
# Mimic curses_radiolist hitting an unrecoverable terminal error so the
|
||||
# caller's except clause routes to the numbered-input fallback.
|
||||
raise subprocess.CalledProcessError(2, ["tput", "clear"])
|
||||
|
||||
|
||||
def test_prompt_model_selection_falls_back_on_terminalmenu_runtime_error(monkeypatch):
|
||||
def test_prompt_model_selection_falls_back_on_menu_runtime_error(monkeypatch):
|
||||
from hermes_cli.auth import _prompt_model_selection
|
||||
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"simple_term_menu",
|
||||
types.SimpleNamespace(TerminalMenu=_BrokenTerminalMenu),
|
||||
)
|
||||
monkeypatch.setattr("hermes_cli.curses_ui.curses_radiolist", _raise_menu)
|
||||
responses = iter(["2"])
|
||||
monkeypatch.setattr("builtins.input", lambda _prompt="": next(responses))
|
||||
|
||||
@@ -28,14 +24,10 @@ def test_prompt_model_selection_falls_back_on_terminalmenu_runtime_error(monkeyp
|
||||
assert selected == "model-b"
|
||||
|
||||
|
||||
def test_prompt_reasoning_effort_falls_back_on_terminalmenu_runtime_error(monkeypatch):
|
||||
def test_prompt_reasoning_effort_falls_back_on_menu_runtime_error(monkeypatch):
|
||||
from hermes_cli.main import _prompt_reasoning_effort_selection
|
||||
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"simple_term_menu",
|
||||
types.SimpleNamespace(TerminalMenu=_BrokenTerminalMenu),
|
||||
)
|
||||
monkeypatch.setattr("hermes_cli.curses_ui.curses_radiolist", _raise_menu)
|
||||
responses = iter(["3"])
|
||||
monkeypatch.setattr("builtins.input", lambda _prompt="": next(responses))
|
||||
|
||||
@@ -44,15 +36,11 @@ def test_prompt_reasoning_effort_falls_back_on_terminalmenu_runtime_error(monkey
|
||||
assert selected == "high"
|
||||
|
||||
|
||||
def test_remove_custom_provider_falls_back_on_terminalmenu_runtime_error(tmp_path, monkeypatch):
|
||||
def test_remove_custom_provider_falls_back_on_menu_runtime_error(tmp_path, monkeypatch):
|
||||
from hermes_cli.main import _remove_custom_provider
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"simple_term_menu",
|
||||
types.SimpleNamespace(TerminalMenu=_BrokenTerminalMenu),
|
||||
)
|
||||
monkeypatch.setattr("hermes_cli.curses_ui.curses_radiolist", _raise_menu)
|
||||
|
||||
cfg = load_config()
|
||||
cfg["custom_providers"] = [
|
||||
@@ -72,15 +60,11 @@ def test_remove_custom_provider_falls_back_on_terminalmenu_runtime_error(tmp_pat
|
||||
]
|
||||
|
||||
|
||||
def test_named_custom_provider_model_picker_falls_back_on_terminalmenu_runtime_error(tmp_path, monkeypatch):
|
||||
def test_named_custom_provider_model_picker_falls_back_on_menu_runtime_error(tmp_path, monkeypatch):
|
||||
from hermes_cli.main import _model_flow_named_custom
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
monkeypatch.setitem(
|
||||
sys.modules,
|
||||
"simple_term_menu",
|
||||
types.SimpleNamespace(TerminalMenu=_BrokenTerminalMenu),
|
||||
)
|
||||
monkeypatch.setattr("hermes_cli.curses_ui.curses_radiolist", _raise_menu)
|
||||
monkeypatch.setattr("hermes_cli.models.fetch_api_models", lambda *args, **kwargs: ["model-a", "model-b"])
|
||||
monkeypatch.setattr("hermes_cli.auth.deactivate_provider", lambda: None)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user