diff --git a/hermes_cli/inventory.py b/hermes_cli/inventory.py index 9eae39abbfc..e1b9a661666 100644 --- a/hermes_cli/inventory.py +++ b/hermes_cli/inventory.py @@ -124,6 +124,7 @@ def build_models_payload( refresh: bool = False, probe_custom_providers: bool = True, probe_current_custom_provider: bool = False, + for_picker: bool = False, max_models: int | None = None, ) -> dict: """Build the ``{providers, model, provider}`` shape every consumer @@ -168,6 +169,11 @@ def build_models_payload( false, still live-probe the current custom endpoint. This keeps normal GUI/TUI picker opens fast while making the active custom provider's model list match the classic CLI picker. + - ``for_picker``: interactive-picker visibility. Keeps providers whose + credential pool exists but is entirely rate-limited (exhausted) in the + list. Rate limits are per-model, so a different model under the same + provider may still work; hiding the provider strands the user. Set for + any surface a human is choosing from, not for programmatic resolution. """ from hermes_cli.model_switch import list_authenticated_providers @@ -182,6 +188,7 @@ def build_models_payload( refresh=refresh, probe_custom_providers=probe_custom_providers, probe_current_custom_provider=probe_current_custom_provider, + for_picker=for_picker, excluded_providers=ctx.excluded_providers or [], ) @@ -295,6 +302,94 @@ def build_model_options_payload( ) +# ─── Public: auxiliary-task pickers ───────────────────────────────────── + + +def build_aux_picker_rows( + *, + current_provider: str = "", + current_model: str = "", + current_base_url: str = "", + max_models: int | None = None, +) -> list[dict]: + """Provider rows for any auxiliary-task picker (vision, compression, …). + + THE entry point for every aux picker — present and future. Call this + instead of ``list_authenticated_providers()`` directly. + + Aux pickers kept re-deriving their own kwargs and each one silently + dropped a different slice of the user's configuration. Two independent + contributor PRs landed against the same two call sites for exactly this: + #52642 (user ``providers:`` / ``custom_providers:`` entries never + appeared) and #66624 (providers with an exhausted credential pool were + hidden). Both were per-site kwarg patches, so the next aux picker would + have reintroduced the same gap. Routing through one function makes the + correct behaviour the default that a new caller cannot forget: + + - user-defined ``providers:`` and saved ``custom_providers:`` entries + - ``model_catalog.excluded_providers`` honoured, matching ``/model`` + - exhausted-credential-pool providers stay visible (``for_picker``) + - the active custom endpoint is probed, offline saved ones are not, so + the picker never blocks on a dead local server + + The virtual ``moa`` row is excluded: auxiliary tasks must not run the + MoA reference fan-out, and ``auxiliary_client`` unwraps a ``moa`` + provider to its aggregator slot anyway (see ``_resolve_auto``), so + offering it here would be a choice silently rewritten behind the user's + back. Mirrors the same filter in ``hermes_cli/moa_cmd.py``. + + Rows are the standard ``list_authenticated_providers`` shape. Pair with + :func:`format_aux_picker_entries` to render them. + """ + ctx = load_picker_context().with_overrides( + current_provider=current_provider, + current_model=current_model, + current_base_url=current_base_url, + ) + rows = build_models_payload( + ctx, + for_picker=True, + probe_custom_providers=False, + probe_current_custom_provider=True, + max_models=max_models, + )["providers"] + return [r for r in rows if str(r.get("slug") or "").strip().lower() != "moa"] + + +def format_aux_picker_entries( + rows: list[dict], + *, + current_provider: str = "", + current_base_url: str = "", +) -> list[tuple[str, str, list[str]]]: + """Render aux-picker rows as ``(slug, label, models)`` menu entries. + + Owns the label text and the ``← current`` marker so every aux picker + presents providers identically. Callers add their own leading/trailing + entries (``auto``, ``Custom endpoint``, ``Back``) around this list. + + A custom endpoint set via a raw ``base_url`` is "current" only through + that URL — never through a provider slug — so when ``current_base_url`` + is set no provider row is marked, matching the pre-existing behaviour of + both call sites. + """ + entries: list[tuple[str, str, list[str]]] = [] + current_slug = str(current_provider or "").strip().lower() + has_base_url = bool(str(current_base_url or "").strip()) + for row in rows: + slug = str(row.get("slug") or "") + name = row.get("name") or slug + total = row.get("total_models") or len(row.get("models") or []) + model_hint = f" — {total} models" if total else "" + marker = ( + " ← current" + if slug.lower() == current_slug and current_slug and not has_base_url + else "" + ) + entries.append((slug, f"{name}{model_hint}{marker}", list(row.get("models") or []))) + return entries + + def _apply_capabilities(rows: list[dict]) -> None: """Attach a ``{model: {fast, reasoning}}`` map to each provider row. diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 42e058d9cd1..e3241b05632 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -3626,18 +3626,19 @@ def _aux_config_menu() -> None: def _aux_select_for_task(task: str) -> None: """Pick a provider + model for a single auxiliary task and persist it. - Uses ``list_authenticated_providers()`` to only show providers the user - has already configured. This avoids re-running OAuth/credential flows - inside the aux picker — users set up new providers through the normal - ``hermes model`` flow, then route aux tasks to them here. + Provider rows come from ``build_aux_picker_rows()`` — the shared aux-picker + substrate — so this surface shows exactly what every other aux picker + shows: authenticated built-ins, the user's own ``providers:`` / + ``custom_providers:`` endpoints, and providers whose credential pool is + temporarily exhausted. Only already-configured providers appear; users set + up new ones through the normal ``hermes model`` flow, then route aux tasks + to them here. """ - from hermes_cli.config import get_compatible_custom_providers, load_config - from hermes_cli.model_switch import list_authenticated_providers + from hermes_cli.config import load_config + from hermes_cli.inventory import build_aux_picker_rows, format_aux_picker_entries cfg = load_config() aux = cfg.get("auxiliary", {}) if isinstance(cfg.get("auxiliary"), dict) else {} - user_providers = cfg.get("providers") if isinstance(cfg.get("providers"), dict) else {} - custom_providers = get_compatible_custom_providers(cfg) task_cfg = aux.get(task, {}) if isinstance(aux.get(task), dict) else {} current_provider = str(task_cfg.get("provider") or "auto").strip() or "auto" current_model = str(task_cfg.get("model") or "").strip() @@ -3647,18 +3648,10 @@ def _aux_select_for_task(task: str) -> None: # Gather authenticated providers (has credentials + curated model list) try: - providers = list_authenticated_providers( + providers = build_aux_picker_rows( current_provider=current_provider, current_model=current_model, current_base_url=current_base_url, - user_providers=user_providers, - custom_providers=custom_providers, - # Interactive picker: also show providers whose credential pool is - # entirely rate-limited (exhausted). Rate limits are per-model, and - # this persists an aux-task config the user will use later once the - # cooldown clears, so hiding the provider here is wrong — same - # rationale as the /model picker (#66584). - for_picker=True, ) except Exception as exc: print(f"Could not detect authenticated providers: {exc}") @@ -3671,16 +3664,13 @@ def _aux_select_for_task(task: str) -> None: ) entries.append(("__auto__", f"auto (recommended){auto_marker}", [])) - for p in providers: - slug = p.get("slug", "") - name = p.get("name") or slug - total = p.get("total_models", 0) - models = p.get("models") or [] - model_hint = f" — {total} models" if total else "" - marker = ( - " ← current" if slug == current_provider and not current_base_url else "" + entries.extend( + format_aux_picker_entries( + providers, + current_provider=current_provider, + current_base_url=current_base_url, ) - entries.append((slug, f"{name}{model_hint}{marker}", list(models))) + ) # Custom endpoint (raw base_url) custom_marker = " ← current" if current_base_url else "" diff --git a/hermes_cli/tools_config.py b/hermes_cli/tools_config.py index 457e31601f7..ace4dc3a727 100644 --- a/hermes_cli/tools_config.py +++ b/hermes_cli/tools_config.py @@ -3810,22 +3810,33 @@ def _configure_vision_backend() -> None: def _configure_vision_provider_model(config: dict, vision_cfg: dict) -> None: """Provider + model picker for vision, mirroring the ``/model`` surface. - Lists authenticated providers (same data source as the model switcher), - lets the user pick one and then a model from its curated list (or type a - custom id), and persists ``auxiliary.vision.provider`` + ``.model``. + Provider rows come from ``build_aux_picker_rows()`` — the shared aux-picker + substrate — so this picker lists exactly what the ``hermes model`` aux-task + picker lists, including the user's own ``providers:`` / ``custom_providers:`` + endpoints. Lets the user pick a provider and then a model from its curated + list (or type a custom id), and persists ``auxiliary.vision.provider`` + + ``.model``. """ try: - from hermes_cli.model_switch import list_authenticated_providers + from hermes_cli.inventory import ( + build_aux_picker_rows, + format_aux_picker_entries, + ) except Exception as exc: # pragma: no cover - import guard _print_warning(f" Could not load provider list: {exc}") return + current_provider = str(vision_cfg.get("provider") or "").strip() + current_model = str(vision_cfg.get("model") or "").strip() + current_base_url = str(vision_cfg.get("base_url") or "").strip() + try: - # Interactive picker: include providers whose credential pool is - # entirely rate-limited (exhausted) so the user can still route vision - # here — rate limits are per-model and this persists a config used - # later. Same rationale as the /model picker (#66584). - providers = list_authenticated_providers(max_models=40, for_picker=True) + providers = build_aux_picker_rows( + current_provider=current_provider, + current_model=current_model, + current_base_url=current_base_url, + max_models=40, + ) except Exception as exc: _print_warning(f" Could not detect providers: {exc}") providers = [] @@ -3837,11 +3848,14 @@ def _configure_vision_provider_model(config: dict, vision_cfg: dict) -> None: ) return - provider_labels = [] - for p in providers: - name = p.get("name") or p.get("slug") - total = p.get("total_models", len(p.get("models", []))) - provider_labels.append(f"{name} ({total} models)" if total else str(name)) + provider_labels = [ + label + for _slug, label, _models in format_aux_picker_entries( + providers, + current_provider=current_provider, + current_base_url=current_base_url, + ) + ] provider_labels.append("Cancel") pidx = _prompt_choice(" Choose vision provider:", provider_labels, 0) diff --git a/tests/hermes_cli/test_aux_picker_inventory.py b/tests/hermes_cli/test_aux_picker_inventory.py new file mode 100644 index 00000000000..73ac3628a68 --- /dev/null +++ b/tests/hermes_cli/test_aux_picker_inventory.py @@ -0,0 +1,239 @@ +"""Auxiliary-task pickers share one provider-inventory substrate. + +Every aux picker (``hermes model`` → Configure auxiliary models, the +``hermes tools`` vision picker, and any future one) must route through +``hermes_cli.inventory.build_aux_picker_rows()`` so it shows the same +provider universe as ``/model``. + +Two independent contributor PRs fixed the same two call sites for exactly +this reason: + +- #52642 (@deepjia) — user ``providers:`` / ``custom_providers:`` entries + were invisible because the aux picker never forwarded them. +- #66624 (@Drexuxux) — providers with a fully rate-limited credential pool + were hidden because the aux picker never forwarded ``for_picker``. + +Both were per-call-site kwarg patches, so the next aux picker would have +reintroduced the gap. These tests pin the *shared substrate* behaviour and +guard the seam itself, not the kwargs at any one site. +""" + +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml + + +CONFIG = { + "model": {"provider": "openrouter", "default": "anthropic/claude-opus-4.6"}, + "model_catalog": {"excluded_providers": ["copilot"]}, + "providers": { + "my-llm": { + "name": "My LLM", + "base_url": "https://myllm.example.com/v1", + "key_env": "MYLLM_KEY", + "discover_models": False, + "models": {"big-model": {}, "small-model": {}}, + } + }, + "custom_providers": [ + { + "name": "Legacy Box", + "base_url": "https://legacy.example.com/v1", + "key_env": "LEGACY_KEY", + "model": "legacy-1", + "discover_models": False, + } + ], +} + + +@pytest.fixture +def configured_home(tmp_path, monkeypatch): + """A HERMES_HOME with one ``providers:`` entry and one legacy + ``custom_providers:`` entry, both credentialled via env.""" + home = tmp_path / ".hermes" + home.mkdir() + (home / "config.yaml").write_text(yaml.safe_dump(CONFIG)) + monkeypatch.setattr(Path, "home", lambda: tmp_path) + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-or-test") + monkeypatch.setenv("MYLLM_KEY", "sk-mine") + monkeypatch.setenv("LEGACY_KEY", "sk-legacy") + return home + + +# ─── The substrate contract ───────────────────────────────────────────── + + +def test_aux_picker_surfaces_user_defined_providers(configured_home): + """Both config schemas for a user's own endpoint reach an aux picker. + + This is #52642's bug: the aux picker built its own kwargs and passed + neither ``user_providers`` nor ``custom_providers``, so a user who had + configured their own endpoint could not route any auxiliary task to it. + """ + from hermes_cli.inventory import build_aux_picker_rows + + slugs = {r["slug"] for r in build_aux_picker_rows()} + + assert "my-llm" in slugs, ( + "a keyed providers: entry must be selectable for auxiliary tasks" + ) + assert "custom:legacy-box" in slugs, ( + "a legacy custom_providers: entry must be selectable for auxiliary tasks" + ) + + +def test_aux_picker_carries_configured_models_for_user_provider(configured_home): + """A user provider arrives with its configured model list, so the + follow-up model prompt has something to offer.""" + from hermes_cli.inventory import build_aux_picker_rows + + row = next(r for r in build_aux_picker_rows() if r["slug"] == "my-llm") + + assert set(row["models"]) == {"big-model", "small-model"} + + +def test_aux_picker_honors_excluded_providers(configured_home): + """``model_catalog.excluded_providers`` applies to aux pickers too. + + A provider the user hid from ``/model`` must not reappear in an aux + picker — the exclusion is about the provider, not about one surface. + """ + from hermes_cli.inventory import build_aux_picker_rows + + slugs = {str(r["slug"]).lower() for r in build_aux_picker_rows()} + + assert "copilot" not in slugs + + +def test_aux_picker_omits_virtual_moa_row(configured_home): + """MoA is not a real endpoint and auxiliary_client unwraps it to the + aggregator slot, so offering it in an aux picker would be a selection + silently rewritten behind the user's back.""" + from hermes_cli.inventory import build_aux_picker_rows + + cfg = dict(CONFIG) + cfg["moa"] = {"presets": {"opus-gpt": {}}} + (configured_home / "config.yaml").write_text(yaml.safe_dump(cfg)) + + slugs = {str(r["slug"]).lower() for r in build_aux_picker_rows()} + + assert "moa" not in slugs + + +def test_aux_picker_requests_exhausted_pool_visibility(configured_home): + """#66624: a provider whose credential pool is entirely rate-limited + must stay visible. Rate limits are per-model and the aux picker writes a + config the user runs later, once the cooldown has cleared.""" + from hermes_cli import inventory + + seen = {} + + def _capture(**kwargs): + seen.update(kwargs) + return [] + + with patch("hermes_cli.model_switch.list_authenticated_providers", _capture): + inventory.build_aux_picker_rows() + + assert seen.get("for_picker") is True + + +def test_aux_picker_does_not_block_on_offline_saved_endpoints(configured_home): + """Saved custom endpoints are not live-probed on open (a dead local + server would hang the picker); only the active one is.""" + from hermes_cli import inventory + + seen = {} + + def _capture(**kwargs): + seen.update(kwargs) + return [] + + with patch("hermes_cli.model_switch.list_authenticated_providers", _capture): + inventory.build_aux_picker_rows() + + assert seen.get("probe_custom_providers") is False + assert seen.get("probe_current_custom_provider") is True + + +# ─── Shared rendering ─────────────────────────────────────────────────── + + +def test_format_entries_marks_current_provider(): + from hermes_cli.inventory import format_aux_picker_entries + + rows = [ + {"slug": "my-llm", "name": "My LLM", "models": ["a", "b"], "total_models": 2}, + {"slug": "openrouter", "name": "OpenRouter", "models": ["x"], "total_models": 1}, + ] + + entries = format_aux_picker_entries(rows, current_provider="my-llm") + + assert entries[0] == ("my-llm", "My LLM — 2 models ← current", ["a", "b"]) + assert entries[1] == ("openrouter", "OpenRouter — 1 models", ["x"]) + + +def test_format_entries_base_url_owns_current_marker(): + """When the task points at a raw base_url, the current selection is the + URL — no provider row may claim the marker.""" + from hermes_cli.inventory import format_aux_picker_entries + + rows = [{"slug": "my-llm", "name": "My LLM", "models": ["a"], "total_models": 1}] + + entries = format_aux_picker_entries( + rows, current_provider="my-llm", current_base_url="https://elsewhere/v1" + ) + + assert "← current" not in entries[0][1] + + +# ─── Seam guard ───────────────────────────────────────────────────────── + + +def test_aux_pickers_route_through_the_shared_substrate(configured_home): + """Neither aux picker may call ``list_authenticated_providers`` directly. + + This is the regression that produced #52642 and #66624 as two separate + contributor PRs against the same two call sites: a picker calling the + low-level function re-derives its own kwargs and drops whichever slice + of user config the author didn't think about. Both pickers must reach + the provider list only through ``build_aux_picker_rows``. + """ + import hermes_cli.main as main + import hermes_cli.tools_config as tools_config + + direct_calls = [] + substrate_calls = [] + + def _direct(**kwargs): + direct_calls.append(kwargs) + return [] + + def _substrate(**kwargs): + substrate_calls.append(kwargs) + return [] + + # Abort each picker at its first prompt, right after it has asked for its + # provider list. The vision picker returns early on empty rows; the + # aux-task picker still renders auto/custom/back, so cancel its menu. + with ( + patch("hermes_cli.model_switch.list_authenticated_providers", _direct), + patch("hermes_cli.inventory.build_aux_picker_rows", _substrate), + patch("hermes_cli.main._prompt_provider_choice", return_value=None), + ): + main._aux_select_for_task("compression") + tools_config._configure_vision_provider_model({}, {}) + + assert len(substrate_calls) == 2, ( + "both the aux-task picker and the vision picker must source providers " + f"from build_aux_picker_rows (got {len(substrate_calls)} calls)" + ) + assert direct_calls == [], ( + "aux pickers must not call list_authenticated_providers directly — " + "route through hermes_cli.inventory.build_aux_picker_rows so custom " + "providers, exclusions, and picker visibility stay consistent" + )