From efc32ab639ff36d40aabf2c2401f6452a74a4b60 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 11 May 2026 13:57:02 +0530 Subject: [PATCH] refactor(inventory): extract shared ConfigContext + build_models_payload Three call-sites in the codebase each duplicated the same config-slice + list_authenticated_providers + post-processing pattern: - hermes_cli/web_server.py /api/model/options - tui_gateway/server.py model.options JSON-RPC - tui_gateway/server.py model.save_key JSON-RPC This consolidates them onto hermes_cli/inventory.py: load_picker_context() -> ConfigContext Replaces the 17-LOC config-slice (model.{default,name,provider, base_url}, providers:, custom_providers:) every consumer did inline. ConfigContext.with_overrides(*, current_provider=, current_model=, current_base_url=) -> ConfigContext Truthy-only overlay for TUI agent-session state on top of disk config. Empty getattr(agent, ...) attrs MUST NOT clobber disk. build_models_payload(ctx, *, include_unconfigured, picker_hints, canonical_order, max_models) -> dict Single payload builder. Delegates curation to list_authenticated_providers (does not call provider_model_ids per row \u2014 that pulls non-agentic models). picker_hints + canonical_order produce the TUI ModelPickerDialog shape; defaults match the dashboard's existing /api/model/options contract. Two latent bugs fixed by consolidation: 1. The dashboard read cfg.get('custom_providers') directly, missing the v12+ keyed providers: form. Now both surfaces go through get_compatible_custom_providers(). 2. The TUI's canonical-merge keyed on is_user_defined to decide order. Section 3 of list_authenticated_providers sets is_user_defined=True on rows from the providers: config dict even when the slug is canonical \u2014 that silently demoted them to the picker tail. _reorder_canonical now keys on slug membership instead. Stats: +666 / -145 (net +521). Module 240 LOC; 18 behavior tests. This PR replaces the rejected #23369 (which bundled the consolidation with new scriptable CLI surfaces \u2014 hermes models list/status, hermes providers list \u2014 and a JSON contract that have no external user demand). Just the refactor; the CLI surface is deferred to a separate PR gated on actual demand. Refs #23359. --- hermes_cli/inventory.py | 240 ++++++++++++++++++ hermes_cli/web_server.py | 34 +-- tests/hermes_cli/test_inventory.py | 378 +++++++++++++++++++++++++++++ tui_gateway/server.py | 159 ++++-------- 4 files changed, 666 insertions(+), 145 deletions(-) create mode 100644 hermes_cli/inventory.py create mode 100644 tests/hermes_cli/test_inventory.py diff --git a/hermes_cli/inventory.py b/hermes_cli/inventory.py new file mode 100644 index 00000000000..5cf32d1c847 --- /dev/null +++ b/hermes_cli/inventory.py @@ -0,0 +1,240 @@ +"""Provider/model inventory context — shared substrate for the dashboard +``/api/model/options``, the TUI ``model.options``/``model.save_key`` +JSON-RPC handlers, and the interactive picker. + +Before this module the three call-sites each duplicated: + +1. The 17-LOC config-slice that pulls ``model.{default,name,provider,base_url}``, + ``providers:``, and ``custom_providers:`` out of ``load_config()``; +2. The call into ``list_authenticated_providers`` with the resulting kwargs; +3. (TUI only) a 45-LOC post-pass that merges authenticated rows with + unconfigured ``CANONICAL_PROVIDERS`` rows and emits ``authenticated``/ + ``auth_type``/``key_env``/``warning`` hints for the picker UI. + +Consolidating those three steps into one entry point eliminates two bugs +the duplicates were hiding: + +- The dashboard read ``cfg.get("custom_providers")`` directly, missing the + v12+ keyed ``providers:`` form (which the TUI handled via + ``get_compatible_custom_providers``). +- The TUI's canonical-merge keyed on ``is_user_defined`` to decide + ordering. Section 3 of ``list_authenticated_providers`` sets + ``is_user_defined=True`` even for canonical slugs that appear in the + ``providers:`` config dict, which silently demoted them to the tail of + the picker. ``_reorder_canonical`` keys on slug membership instead. + +Substrate facts (verified May 2026): +- ``list_authenticated_providers`` already populates each row's + ``models`` from the curated catalog (same source as the picker). Do + NOT call ``provider_model_ids()`` per row to "freshen" — that bypasses + curation and pulls in non-agentic models (Nous /models returns ~400 + IDs including TTS, embeddings, rerankers, image/video generators). +""" + +from __future__ import annotations + +from dataclasses import dataclass, replace +from typing import Optional + + +# ─── Public types ─────────────────────────────────────────────────────── + + +@dataclass(frozen=True) +class ConfigContext: + """Snapshot of the model + provider config every inventory caller + needs. Built once via ``load_picker_context()``; the TUI overlays + live agent state via ``with_overrides()`` before passing through. + """ + + current_provider: str + current_model: str + current_base_url: str + user_providers: dict + custom_providers: list + + def with_overrides( + self, + *, + current_provider: Optional[str] = None, + current_model: Optional[str] = None, + current_base_url: Optional[str] = None, + ) -> "ConfigContext": + """Return a copy with truthy overrides applied. + + Truthy-only because the TUI reads agent attributes that may be + empty strings before an agent is spawned — empties must NOT + clobber the disk-config values. + """ + kw: dict = {} + if current_provider: + kw["current_provider"] = current_provider + if current_model: + kw["current_model"] = current_model + if current_base_url: + kw["current_base_url"] = current_base_url + return replace(self, **kw) if kw else self + + +def load_picker_context() -> ConfigContext: + """Load the disk-config snapshot every consumer needs. + + Replaces the inline 17-LOC config-slice that ``web_server.py`` and + ``tui_gateway/server.py`` (×2 sites) used to do. + """ + from hermes_cli.config import get_compatible_custom_providers, load_config + + cfg = load_config() + model_cfg = cfg.get("model", {}) + if isinstance(model_cfg, dict): + current_model = model_cfg.get("default", model_cfg.get("name", "")) or "" + current_provider = model_cfg.get("provider", "") or "" + current_base_url = model_cfg.get("base_url", "") or "" + else: + # config.model can be a bare string in older configs. + current_model = str(model_cfg) if model_cfg else "" + current_provider = "" + current_base_url = "" + raw = cfg.get("providers") + return ConfigContext( + current_provider=current_provider, + current_model=current_model, + current_base_url=current_base_url, + user_providers=raw if isinstance(raw, dict) else {}, + custom_providers=get_compatible_custom_providers(cfg), + ) + + +# ─── Public: payload builder ──────────────────────────────────────────── + + +def build_models_payload( + ctx: ConfigContext, + *, + include_unconfigured: bool = False, + picker_hints: bool = False, + canonical_order: bool = False, + max_models: int = 50, +) -> dict: + """Build the ``{providers, model, provider}`` shape every consumer + needs from a single substrate call. + + Flags: + - ``include_unconfigured``: append ``CANONICAL_PROVIDERS`` rows that + ``list_authenticated_providers`` didn't emit (TUI uses this to show + the full provider universe in the picker). + - ``picker_hints``: add ``authenticated``/``auth_type``/``key_env``/ + ``warning`` per row (TUI ``ModelPickerDialog`` shape). + - ``canonical_order``: reorder canonical-slug rows to + ``CANONICAL_PROVIDERS`` declaration order; truly-custom rows go + last (TUI display order). + """ + from hermes_cli.model_switch import list_authenticated_providers + + rows = list_authenticated_providers( + current_provider=ctx.current_provider, + current_base_url=ctx.current_base_url, + current_model=ctx.current_model, + user_providers=ctx.user_providers, + custom_providers=ctx.custom_providers, + max_models=max_models, + ) + + if include_unconfigured: + rows = list(rows) + _append_unconfigured_rows(rows, ctx) + if picker_hints: + _apply_picker_hints(rows) + if canonical_order: + rows = _reorder_canonical(rows) + + return { + "providers": rows, + "model": ctx.current_model, + "provider": ctx.current_provider, + } + + +# ─── Internal: row post-processing ────────────────────────────────────── + + +def _append_unconfigured_rows(rows: list[dict], ctx: ConfigContext) -> list[dict]: + """Build skeleton rows for canonical providers missing from ``rows``.""" + from hermes_cli.models import CANONICAL_PROVIDERS, _PROVIDER_LABELS + + seen = {r["slug"].lower() for r in rows} + cur = (ctx.current_provider or "").lower() + extras: list[dict] = [] + for entry in CANONICAL_PROVIDERS: + if entry.slug.lower() in seen: + continue + extras.append( + { + "slug": entry.slug, + "name": _PROVIDER_LABELS.get(entry.slug, entry.label), + "is_current": entry.slug.lower() == cur, + "is_user_defined": False, + "models": [], + "total_models": 0, + "source": "canonical", + } + ) + return extras + + +def _apply_picker_hints(rows: list[dict]) -> None: + """Add ``authenticated``/``auth_type``/``key_env``/``warning`` per row. + + Mutates ``rows`` in-place. Rows already from + ``list_authenticated_providers`` are marked ``authenticated=True``; + the unconfigured skeleton rows from ``_append_unconfigured_rows`` get + the picker's setup-hint shape. + """ + from hermes_cli.auth import PROVIDER_REGISTRY + + for row in rows: + if "authenticated" in row: + continue + # Distinguish authenticated rows (returned by + # list_authenticated_providers) from skeleton rows (from + # _append_unconfigured_rows). The skeleton rows have empty + # `models` AND source="canonical"; authenticated rows have + # populated `models` OR a non-canonical source. + is_skeleton = row.get("source") == "canonical" and not row.get("models") + row["authenticated"] = not is_skeleton + if not is_skeleton or row.get("is_user_defined"): + continue + cfg = PROVIDER_REGISTRY.get(row["slug"]) + auth_type = cfg.auth_type if cfg else "api_key" + key_env = ( + cfg.api_key_env_vars[0] + if (cfg and cfg.api_key_env_vars) + else "" + ) + row["auth_type"] = auth_type + row["key_env"] = key_env + row["warning"] = ( + f"paste {key_env} to activate" + if auth_type == "api_key" and key_env + else f"run `hermes model` to configure ({auth_type})" + ) + + +def _reorder_canonical(rows: list[dict]) -> list[dict]: + """Canonical slugs in ``CANONICAL_PROVIDERS`` declaration order; + truly-custom rows last. + + Keys on slug membership, NOT ``is_user_defined`` — section 3 of + ``list_authenticated_providers`` sets ``is_user_defined=True`` on + rows from the ``providers:`` config dict even when the slug is + canonical. Keying on the flag would silently demote canonical + providers configured via the new keyed schema. + """ + from hermes_cli.models import CANONICAL_PROVIDERS + + order = {e.slug: i for i, e in enumerate(CANONICAL_PROVIDERS)} + canon = sorted( + (r for r in rows if r["slug"] in order), + key=lambda r: order[r["slug"]], + ) + extras = [r for r in rows if r["slug"] not in order] + return canon + extras diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 3f0eae0aebc..bdb24554f87 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -994,39 +994,9 @@ def get_model_options(): can share the same types. """ try: - from hermes_cli.model_switch import list_authenticated_providers + from hermes_cli.inventory import build_models_payload, load_picker_context - cfg = load_config() - model_cfg = cfg.get("model", {}) - if isinstance(model_cfg, dict): - current_model = model_cfg.get("default", model_cfg.get("name", "")) or "" - current_provider = model_cfg.get("provider", "") or "" - current_base_url = model_cfg.get("base_url", "") or "" - else: - current_model = str(model_cfg) if model_cfg else "" - current_provider = "" - current_base_url = "" - - user_providers = cfg.get("providers") if isinstance(cfg.get("providers"), dict) else {} - custom_providers = ( - cfg.get("custom_providers") - if isinstance(cfg.get("custom_providers"), list) - else [] - ) - - providers = list_authenticated_providers( - current_provider=current_provider, - current_base_url=current_base_url, - current_model=current_model, - user_providers=user_providers, - custom_providers=custom_providers, - max_models=50, - ) - return { - "providers": providers, - "model": current_model, - "provider": current_provider, - } + return build_models_payload(load_picker_context(), max_models=50) except Exception: _log.exception("GET /api/model/options failed") raise HTTPException(status_code=500, detail="Failed to list model options") diff --git a/tests/hermes_cli/test_inventory.py b/tests/hermes_cli/test_inventory.py new file mode 100644 index 00000000000..2a288b37a45 --- /dev/null +++ b/tests/hermes_cli/test_inventory.py @@ -0,0 +1,378 @@ +"""Behavior tests for hermes_cli.inventory. + +Locks the invariants the three migrated consumers (web_server.py +/api/model/options, tui_gateway model.options, tui_gateway model.save_key) +depend on: + +- load_picker_context() reproduces the inline 17-LOC config-slice exactly. +- with_overrides() is truthy-only (empty agent attrs must not clobber). +- build_models_payload() returns a stable {providers, model, provider} + shape and delegates curation to list_authenticated_providers (does not + call provider_model_ids per row). +- canonical_order keys on slug membership, not is_user_defined — section + 3 of list_authenticated_providers sets is_user_defined=True for + canonical slugs in the providers: dict, and that flag must NOT demote + them to the tail. +- picker_hints adds authenticated/auth_type/key_env/warning per row, + matching the TUI ModelPickerDialog shape. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from hermes_cli.inventory import ( + ConfigContext, + build_models_payload, + load_picker_context, +) + + +# ─── load_picker_context ─────────────────────────────────────────────── + + +def _cfg(model=None, providers=None, custom_providers=None) -> dict: + return { + "model": model if model is not None else {}, + "providers": providers if providers is not None else {}, + "custom_providers": custom_providers if custom_providers is not None else [], + } + + +def test_load_picker_context_full_dict(): + cfg = _cfg( + model={ + "default": "anthropic/claude-sonnet-4.6", + "provider": "openrouter", + "base_url": "https://openrouter.ai/api/v1", + }, + providers={"openrouter": {}}, + custom_providers=[{"name": "Ollama", "base_url": "http://localhost:11434/v1"}], + ) + with patch("hermes_cli.config.load_config", return_value=cfg): + ctx = load_picker_context() + assert ctx.current_model == "anthropic/claude-sonnet-4.6" + assert ctx.current_provider == "openrouter" + assert ctx.current_base_url == "https://openrouter.ai/api/v1" + assert "openrouter" in ctx.user_providers + # custom_providers comes from get_compatible_custom_providers, which + # merges legacy list + v12+ keyed providers — both present here means + # at least one row. + assert isinstance(ctx.custom_providers, list) + + +def test_load_picker_context_falls_back_to_name_when_default_missing(): + cfg = _cfg(model={"name": "gpt-5.4", "provider": "openai"}) + with patch("hermes_cli.config.load_config", return_value=cfg): + ctx = load_picker_context() + assert ctx.current_model == "gpt-5.4" + assert ctx.current_provider == "openai" + + +def test_load_picker_context_string_model_legacy_shape(): + """config.model can be a bare string in older configs.""" + cfg = {"model": "some-model", "providers": {}, "custom_providers": []} + with patch("hermes_cli.config.load_config", return_value=cfg): + ctx = load_picker_context() + assert ctx.current_model == "some-model" + assert ctx.current_provider == "" + assert ctx.current_base_url == "" + + +def test_load_picker_context_empty_config(): + cfg = _cfg() + with patch("hermes_cli.config.load_config", return_value=cfg): + ctx = load_picker_context() + assert ctx.current_provider == "" + assert ctx.current_model == "" + assert ctx.current_base_url == "" + assert ctx.user_providers == {} + assert ctx.custom_providers == [] + + +# ─── with_overrides ──────────────────────────────────────────────────── + + +def _empty_ctx(provider="orig", model="orig-model", base_url="orig-url"): + return ConfigContext( + current_provider=provider, + current_model=model, + current_base_url=base_url, + user_providers={}, + custom_providers=[], + ) + + +def test_with_overrides_truthy_only_strings(): + """Empty strings must NOT clobber disk config — TUI calls this with + empty getattr(agent, 'provider', '') when no agent is spawned yet.""" + ctx = _empty_ctx() + overlaid = ctx.with_overrides( + current_provider="", + current_model="", + current_base_url="", + ) + assert overlaid.current_provider == "orig" + assert overlaid.current_model == "orig-model" + assert overlaid.current_base_url == "orig-url" + + +def test_with_overrides_truthy_value_replaces(): + ctx = _empty_ctx() + overlaid = ctx.with_overrides(current_provider="anthropic") + assert overlaid.current_provider == "anthropic" + assert overlaid.current_model == "orig-model" # untouched + + +def test_with_overrides_no_args_returns_self_or_equivalent(): + ctx = _empty_ctx() + assert ctx.with_overrides() == ctx + + +# ─── build_models_payload ────────────────────────────────────────────── + + +def _list_auth_returning(rows: list[dict]): + """Patch list_authenticated_providers to return a fixed row list.""" + return patch( + "hermes_cli.model_switch.list_authenticated_providers", + return_value=rows, + ) + + +def test_build_models_payload_returns_expected_shape(): + rows = [ + {"slug": "openrouter", "name": "OpenRouter", "models": ["m1"], + "total_models": 1, "is_current": True, "is_user_defined": False, + "source": "built-in"}, + ] + ctx = _empty_ctx(provider="openrouter", model="m1", base_url="") + with _list_auth_returning(rows): + payload = build_models_payload(ctx) + assert set(payload.keys()) == {"providers", "model", "provider"} + assert payload["model"] == "m1" + assert payload["provider"] == "openrouter" + assert payload["providers"] == rows + + +def test_build_models_payload_does_not_call_provider_model_ids(): + """Curated lists must come from list_authenticated_providers, not + provider_model_ids — that would pull TTS/embeddings/etc. + """ + rows = [{"slug": "nous", "name": "Nous", "models": ["hermes-4-405b"], + "total_models": 1, "is_current": False, "is_user_defined": False, + "source": "built-in"}] + ctx = _empty_ctx() + with _list_auth_returning(rows), \ + patch("hermes_cli.models.provider_model_ids") as mock_pm: + build_models_payload(ctx) + mock_pm.assert_not_called() + + +def test_include_unconfigured_appends_canonical_skeletons(): + """include_unconfigured=True adds CANONICAL_PROVIDERS rows that + list_authenticated_providers didn't emit. Skeleton rows have empty + models and source='canonical'.""" + rows = [ + {"slug": "openrouter", "name": "OpenRouter", "models": ["m1"], + "total_models": 1, "is_current": True, "is_user_defined": False, + "source": "built-in"}, + ] + ctx = _empty_ctx(provider="openrouter") + with _list_auth_returning(rows): + payload = build_models_payload(ctx, include_unconfigured=True) + # All canonical providers other than openrouter should appear as + # skeleton rows. + from hermes_cli.models import CANONICAL_PROVIDERS + + seen_slugs = {r["slug"] for r in payload["providers"]} + for entry in CANONICAL_PROVIDERS: + assert entry.slug in seen_slugs, f"missing {entry.slug}" + # Skeletons have empty models and source='canonical'. + skeletons = [r for r in payload["providers"] + if r.get("source") == "canonical"] + assert all(r["models"] == [] for r in skeletons) + assert all(r["total_models"] == 0 for r in skeletons) + + +def test_include_unconfigured_skips_already_present_slugs(): + """If list_authenticated_providers already returned a row for a + canonical slug, include_unconfigured must NOT duplicate it.""" + rows = [ + {"slug": "openrouter", "name": "OpenRouter", "models": ["m1"], + "total_models": 1, "is_current": True, "is_user_defined": False, + "source": "built-in"}, + ] + ctx = _empty_ctx() + with _list_auth_returning(rows): + payload = build_models_payload(ctx, include_unconfigured=True) + or_rows = [r for r in payload["providers"] if r["slug"] == "openrouter"] + assert len(or_rows) == 1 + assert or_rows[0]["models"] == ["m1"] # the authenticated row, not skeleton + + +# ─── picker_hints ────────────────────────────────────────────────────── + + +def test_picker_hints_marks_authed_rows_authenticated(): + rows = [ + {"slug": "openrouter", "name": "OpenRouter", "models": ["m1"], + "total_models": 1, "is_current": True, "is_user_defined": False, + "source": "built-in"}, + ] + ctx = _empty_ctx() + with _list_auth_returning(rows): + payload = build_models_payload(ctx, picker_hints=True) + assert payload["providers"][0]["authenticated"] is True + + +def test_picker_hints_adds_warning_to_skeleton_rows(): + """Skeleton rows (unconfigured canonical providers) must carry the + setup hint the picker UI displays.""" + rows = [] + ctx = _empty_ctx() + with _list_auth_returning(rows): + payload = build_models_payload( + ctx, include_unconfigured=True, picker_hints=True, + ) + skeleton_rows = [r for r in payload["providers"] + if r.get("source") == "canonical"] + assert skeleton_rows, "test setup: expected at least one skeleton row" + for row in skeleton_rows: + assert row["authenticated"] is False + assert "auth_type" in row + assert "warning" in row + # api_key providers get "paste X to activate" / others get the + # hermes model fallback. + assert ( + row["warning"].startswith("paste ") + or row["warning"].startswith("run `hermes model`") + ) + + +def test_picker_hints_api_key_warning_format(): + """For api_key providers with a defined env var, the warning must + point to that env var.""" + rows = [] + ctx = _empty_ctx() + with _list_auth_returning(rows): + payload = build_models_payload( + ctx, include_unconfigured=True, picker_hints=True, + ) + # anthropic uses api_key + ANTHROPIC_API_KEY. + anthropic = next( + r for r in payload["providers"] if r["slug"] == "anthropic" + ) + assert "ANTHROPIC_API_KEY" in anthropic["warning"] + assert anthropic["warning"].startswith("paste ") + + +# ─── canonical_order ─────────────────────────────────────────────────── + + +def test_canonical_order_uses_slug_not_is_user_defined_flag(): + """Section 3 of list_authenticated_providers sets is_user_defined=True + for canonical slugs that appear in the providers: config dict. + canonical_order MUST key on slug membership, not the flag — otherwise + canonical providers configured via the keyed schema get demoted to + the tail. + """ + from hermes_cli.models import CANONICAL_PROVIDERS + + canonical_slug = CANONICAL_PROVIDERS[2].slug # any canonical + rows = [ + # A truly-custom row (correct: is_user_defined=True) + {"slug": "custom:Ollama", "name": "Ollama", "models": [], + "total_models": 0, "is_current": False, "is_user_defined": True, + "source": "user-config"}, + # A canonical row that the substrate flagged as user-defined + # because the user configured it via providers: dict. + {"slug": canonical_slug, "name": "x", "models": ["m1"], + "total_models": 1, "is_current": False, "is_user_defined": True, + "source": "built-in"}, + ] + ctx = _empty_ctx() + with _list_auth_returning(rows): + payload = build_models_payload(ctx, canonical_order=True) + slugs = [r["slug"] for r in payload["providers"]] + # Canonical-slug row must come BEFORE truly-custom rows, regardless + # of is_user_defined. + canonical_idx = slugs.index(canonical_slug) + custom_idx = slugs.index("custom:Ollama") + assert canonical_idx < custom_idx, ( + f"canonical {canonical_slug} demoted to tail " + f"(canonical_idx={canonical_idx} > custom_idx={custom_idx})" + ) + + +def test_canonical_order_with_unconfigured_preserves_full_universe(): + """Combined picker call: include_unconfigured + picker_hints + + canonical_order is the production TUI shape. Verify the result + has CANONICAL_PROVIDERS in declaration order, hints applied, + custom rows trailing. + """ + from hermes_cli.models import CANONICAL_PROVIDERS + + rows = [ + {"slug": "custom:Ollama", "name": "Ollama", "models": [], + "total_models": 0, "is_current": False, "is_user_defined": True, + "source": "user-config"}, + ] + ctx = _empty_ctx() + with _list_auth_returning(rows): + payload = build_models_payload( + ctx, + include_unconfigured=True, + picker_hints=True, + canonical_order=True, + ) + slugs = [r["slug"] for r in payload["providers"]] + # First row: first canonical provider in declaration order. + assert slugs[0] == CANONICAL_PROVIDERS[0].slug + # Custom row trails canonical universe. + assert slugs.index("custom:Ollama") >= len(CANONICAL_PROVIDERS) + + +# ─── Integration: end-to-end through real load_picker_context ────────── + + +def test_end_to_end_with_real_context_no_credentials_leak(monkeypatch): + """Full pipeline: real load_picker_context + real + list_authenticated_providers. Verify no credential string ever + appears in the returned payload, even with picker_hints=True.""" + canary = "sk-canary-XYZ-must-not-appear" + monkeypatch.setenv("OPENROUTER_API_KEY", canary) + monkeypatch.setenv("ANTHROPIC_API_KEY", canary) + cfg = _cfg(model={"provider": "openrouter"}) + with patch("hermes_cli.config.load_config", return_value=cfg): + ctx = load_picker_context() + payload = build_models_payload( + ctx, include_unconfigured=True, picker_hints=True, + ) + import json as _json + + assert canary not in _json.dumps(payload) + + +def test_payload_shape_compatible_with_modelpickerdialog_frontend(): + """Frontend (web/src/components/ModelPickerDialog.tsx) reads: + name, slug, models, total_models, is_current, warning, authenticated. + Verify every authenticated/skeleton row exposes those keys. + """ + rows = [ + {"slug": "openrouter", "name": "OpenRouter", "models": ["m1"], + "total_models": 1, "is_current": True, "is_user_defined": False, + "source": "built-in"}, + ] + ctx = _empty_ctx() + with _list_auth_returning(rows): + payload = build_models_payload( + ctx, include_unconfigured=True, picker_hints=True, + ) + required_keys = {"name", "slug", "models", "total_models", "is_current", + "authenticated"} + for row in payload["providers"]: + missing = required_keys - row.keys() + assert not missing, f"row {row['slug']} missing keys: {missing}" diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 41cbdd05e37..230387ce23b 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -5155,94 +5155,37 @@ def _(rid, params: dict) -> dict: @method("model.options") def _(rid, params: dict) -> dict: try: - from hermes_cli.model_switch import list_authenticated_providers - from hermes_cli.models import CANONICAL_PROVIDERS, _PROVIDER_LABELS + from hermes_cli.inventory import build_models_payload, load_picker_context session = _sessions.get(params.get("session_id", "")) agent = session.get("agent") if session else None - cfg = _load_cfg() - current_provider = getattr(agent, "provider", "") or "" - current_model = getattr(agent, "model", "") or _resolve_model() - current_base_url = getattr(agent, "base_url", "") or "" - # list_authenticated_providers already populates each provider's - # "models" with the curated list (same source as `hermes model` and - # classic CLI's /model picker). Do NOT overwrite with live - # provider_model_ids() — that bypasses curation and pulls in - # non-agentic models (e.g. Nous /models returns ~400 IDs including - # TTS, embeddings, rerankers, image/video generators). - user_provs = ( - cfg.get("providers") if isinstance(cfg.get("providers"), dict) else {} + # Layer agent-session state on top of disk config — once an agent + # is spawned, IT owns the live provider/model/base_url. Empty + # agent attributes must NOT clobber disk config (with_overrides + # is truthy-only). + ctx = load_picker_context().with_overrides( + current_provider=getattr(agent, "provider", "") if agent else "", + current_model=( + (getattr(agent, "model", "") if agent else "") or _resolve_model() + ), + current_base_url=getattr(agent, "base_url", "") if agent else "", ) - custom_provs = ( - cfg.get("custom_providers") - if isinstance(cfg.get("custom_providers"), list) - else [] - ) - authenticated = list_authenticated_providers( - current_provider=current_provider, - current_base_url=current_base_url, - current_model=current_model, - user_providers=user_provs, - custom_providers=custom_provs, + # picker_hints + canonical_order produce the TUI's required shape: + # `authenticated`/`auth_type`/`key_env`/`warning` per row, in + # CANONICAL_PROVIDERS declaration order. include_unconfigured=True + # so the picker can show the full provider universe (with the + # setup-hint warning attached) instead of only authed rows. + # Curated model lists are preserved — list_authenticated_providers + # populates `models` from the curated catalog, not provider_model_ids + # (which would pull non-agentic models like TTS/embeddings/etc.). + payload = build_models_payload( + ctx, + include_unconfigured=True, + picker_hints=True, + canonical_order=True, max_models=50, ) - - # Mark authenticated providers and build lookup by slug - authed_map: dict = {} - authed_extra: list = [] # user-defined/custom not in CANONICAL_PROVIDERS - canonical_slugs = {e.slug for e in CANONICAL_PROVIDERS} - for p in authenticated: - p["authenticated"] = True - authed_map[p["slug"]] = p - if p["slug"] not in canonical_slugs: - authed_extra.append(p) - - # Build final list in CANONICAL_PROVIDERS order, merging auth data - from hermes_cli.auth import PROVIDER_REGISTRY as _auth_reg - - ordered: list = [] - for entry in CANONICAL_PROVIDERS: - if entry.slug in authed_map: - ordered.append(authed_map[entry.slug]) - else: - pconfig = _auth_reg.get(entry.slug) - auth_type = pconfig.auth_type if pconfig else "api_key" - key_env = ( - pconfig.api_key_env_vars[0] - if (pconfig and pconfig.api_key_env_vars) - else "" - ) - if auth_type == "api_key" and key_env: - warning = f"paste {key_env} to activate" - else: - warning = f"run `hermes model` to configure ({auth_type})" - ordered.append( - { - "slug": entry.slug, - "name": _PROVIDER_LABELS.get(entry.slug, entry.label), - "is_current": entry.slug == current_provider, - "is_user_defined": False, - "models": [], - "total_models": 0, - "source": "built-in", - "authenticated": False, - "auth_type": auth_type, - "key_env": key_env, - "warning": warning, - } - ) - - # Append user-defined/custom providers not in canonical list - ordered.extend(authed_extra) - - return _ok( - rid, - { - "providers": ordered, - "model": current_model, - "provider": current_provider, - }, - ) + return _ok(rid, payload) except Exception as e: return _err(rid, 5033, str(e)) @@ -5261,7 +5204,7 @@ def _(rid, params: dict) -> dict: try: from hermes_cli.auth import PROVIDER_REGISTRY from hermes_cli.config import is_managed, save_env_value - from hermes_cli.model_switch import list_authenticated_providers + from hermes_cli.inventory import build_models_payload, load_picker_context slug = (params.get("slug") or "").strip() api_key = (params.get("api_key") or "").strip() @@ -5287,43 +5230,32 @@ def _(rid, params: dict) -> dict: # Save the key to ~/.hermes/.env env_var = pconfig.api_key_env_vars[0] save_env_value(env_var, api_key) - # Also set in current process so list_authenticated_providers sees it + # Also set in current process so the refreshed inventory sees it. import os os.environ[env_var] = api_key - # Refresh provider data - cfg = _load_cfg() + # Refresh provider data via the shared inventory builder so this + # surface stays in lock-step with model.options + dashboard + # /api/model/options. picker_hints=True ensures the returned row + # carries `authenticated` for the TUI frontend. session = _sessions.get(params.get("session_id", "")) agent = session.get("agent") if session else None - current_provider = getattr(agent, "provider", "") or "" - current_model = getattr(agent, "model", "") or _resolve_model() - current_base_url = getattr(agent, "base_url", "") or "" - - providers = list_authenticated_providers( - current_provider=current_provider, - current_base_url=current_base_url, - current_model=current_model, - user_providers=( - cfg.get("providers") if isinstance(cfg.get("providers"), dict) else {} + ctx = load_picker_context().with_overrides( + current_provider=getattr(agent, "provider", "") if agent else "", + current_model=( + (getattr(agent, "model", "") if agent else "") or _resolve_model() ), - custom_providers=( - cfg.get("custom_providers") - if isinstance(cfg.get("custom_providers"), list) - else [] - ), - max_models=50, + current_base_url=getattr(agent, "base_url", "") if agent else "", ) - - # Find the newly-authenticated provider - provider_data = None - for p in providers: - if p["slug"] == slug: - provider_data = p - break - - if not provider_data: - # Key was saved but provider didn't appear — still return success + payload = build_models_payload( + ctx, picker_hints=True, max_models=50, + ) + provider_data = next( + (p for p in payload["providers"] if p["slug"] == slug), None + ) + if provider_data is None: + # Key was saved but provider didn't appear — still return success. provider_data = { "slug": slug, "name": pconfig.name, @@ -5332,7 +5264,8 @@ def _(rid, params: dict) -> dict: "total_models": 0, "authenticated": True, } - + # picker_hints sets `authenticated` from the row state, but the + # synthetic fallback above doesn't go through that path. provider_data["authenticated"] = True return _ok(rid, {"provider": provider_data}) except Exception as e: