From e0c3caf3b8d62adc9925ff4362c2a895decc2582 Mon Sep 17 00:00:00 2001 From: Austin Pickett Date: Sat, 8 Aug 2026 16:07:03 -0400 Subject: [PATCH] fix(model-picker): serve cached custom-provider catalog on no-probe opens (supersedes #81665, #81556) (#81973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(model-picker): serve cached custom-provider catalog on no-probe opens #58183 stopped GUI picker opens from live-probing saved custom OpenAI-compatible endpoints so a stopped local server could not stall the picker. It gated the whole discovery block, not just the network call, so `cached_fetch_api_models()` was skipped too — and with it the catalog an earlier probe had already written to `provider_models_cache.json`. A custom endpoint that is not the current provider therefore renders only the models named in its config entry. A local server with 8 models loaded shows the 1 model that was saved when the provider was first added, on every picker open, while an explicit Refresh shows all 8. Add `cache_only` to `cached_fetch_api_models()`: answer from disk within the existing stale-serve window, never fetch, never revalidate off-thread, return None on a miss. Split the three call sites in `list_authenticated_providers()` into what the user's config permits (`discover_models`, an explicit `models:` allowlist) and how we may obtain it, so suppressing the probe now downgrades to a cached read instead of skipping discovery outright. `discover_models: false` still pins, and a cache hit no longer writes back to config since the probe that populated it already did. The latency win stands: a cold cache is a miss, so picker opens against offline endpoints still make zero network calls. * test(model-picker): pin the cached-catalog contract for no-probe opens Cover both halves of the invariant, since fixing either one alone reintroduces a bug the other guards against. `cache_only` on `cached_fetch_api_models()`: a fresh entry and an entry past its TTL but inside the stale-serve window both serve; an entry beyond that window, an empty cache, rotated credentials, `force_refresh`, and a missing base_url are all misses — and none of them fetch or spawn a background revalidation. `list_authenticated_providers()` on the GUI path: a non-current endpoint with a warm cache reports its full catalog across all three provider shapes (`custom_providers`, `providers:`, bare `provider: custom`) with no live fetch attempted. A cold cache keeps the configured list and still makes no network call, which is the #58183 guarantee. `discover_models: false` keeps pinning, and a cache hit does not write back to config. * fix: persist discovered custom-provider models in the hermes model flow The `hermes model` named-custom-provider flow (_model_flow_named_custom) probes the endpoint and shows the full catalog, but never persists it to the entry's `models:` list. No-probe surfaces (dashboard, desktop, ACP) call build_models_payload(..., probe_custom_providers=False) and only render the configured `models:` list, so a provider added via `hermes model` collapses to the single `model:` default everywhere except the CLI. OpenAI-compatible providers added via a probing picker already benefit from _save_discovered_models_to_config; the CLI flow did not. Persist the live catalog after a successful probe, mirroring the picker path in model_switch.py. A failed save is non-fatal. * fix(model-picker): stop an auto-saved catalog pinning a keyless endpoint The cached-catalog read added for no-probe picker opens still sat behind the no-key discovery gate, so it never reached the shape that motivated it: a keyless local model server. `bool(api_key) or not has_explicit_models` is a network-cost gate. It exists so Hermes does not probe an endpoint it cannot authenticate to when that endpoint already declares its catalog (5f00f36ba, 1039e90b5). Reading a catalog an earlier probe already paid for costs nothing, so the gate belongs on the probe, not on discovery as a whole. Left on the discovery side it re-pins the endpoint it was meant to spare. A successful probe calls `_save_discovered_models_to_config()`, which writes a plain list into `models:` — exactly the shape `_models_config_is_allowlist()` reads back as an explicit user allowlist. A keyless server therefore froze on the catalog of its first probe and could never widen again, which is the "lineup changes after config was written" case. f66319097 already carved the dict shape out of this trap for the same reason; the list shape is the other door into it. Move the clause to `_probe_live` at both custom-endpoint sites. Probe suppression is unchanged — verified byte-identical to main across the keyed/keyless x declared/undeclared matrix — and `discover_models: false` remains the documented way to pin a catalog. * test(model-picker): cover the keyless auto-save pinning trap Three tests around the gate move, each failing on the code before it: - a keyless endpoint carrying an auto-saved `models:` list still reads its full cached catalog - the same row, cold cache and probing enabled, still makes zero live fetches — the network-cost gate the clause exists for - an end-to-end round trip: persist a probe result via `_save_discovered_models_to_config()`, reload it, and assert the shape we wrote does not read back as a user pin The round-trip test guards the whole chain rather than one branch, so a future change that makes the saved shape look like an intentional allowlist fails here even if the gate logic is refactored. * fix(model-picker): key the custom-endpoint model cache by api_mode `cached_fetch_api_models()` fingerprints entries with `api_mode`, but no call site in `list_authenticated_providers()` passed it, so every custom row resolved to the `api_mode=None` fingerprint. Two rows sharing a base_url and credential but differing by `api_mode` are deliberately distinct picker rows — it is part of `group_key` at both sites — yet they collapsed onto one cache entry. That was latent while probing was the only way to fill a row: a mismatched entry was overwritten by the row's own live fetch. Serving that entry without a probe makes it visible, so an `anthropic_messages` row could render the catalog an OpenAI-mode row cached against the same URL. The wire protocols differ (`x-api-key` + `anthropic-version` vs `Authorization: Bearer`), so those catalogs are not interchangeable. Persist `api_mode` on the group at both grouping sites — it is already part of `group_key`, so it is constant across the group — and pass it into the cache read. Section 3b (bare `provider: custom`) has no `api_mode` in scope and already reads with the empty-credential fingerprint, so it is unchanged. Reported by Copilot review on #81973. --------- Co-authored-by: xxxigm Co-authored-by: Navlem <114683850+Navlem@users.noreply.github.com> --- hermes_cli/model_setup_flows.py | 19 +- hermes_cli/model_switch.py | 100 +++- hermes_cli/models.py | 21 + .../test_cached_fetch_api_models.py | 67 +++ .../test_model_switch_custom_providers.py | 473 +++++++++++++++++- 5 files changed, 653 insertions(+), 27 deletions(-) diff --git a/hermes_cli/model_setup_flows.py b/hermes_cli/model_setup_flows.py index fd5a0265df..f62b78faac 100644 --- a/hermes_cli/model_setup_flows.py +++ b/hermes_cli/model_setup_flows.py @@ -1561,11 +1561,24 @@ def _model_flow_named_custom(config, provider_info): fetch_kwargs = {"timeout": 8.0} if api_mode: fetch_kwargs["api_mode"] = api_mode - models = fetch_api_models(api_key, base_url, **fetch_kwargs) + live_models = fetch_api_models(api_key, base_url, **fetch_kwargs) # If the probe came back empty but the operator configured an explicit # list, fall back to it rather than forcing manual entry. - if not models and configured_models: - models = configured_models + models = live_models or configured_models + # Persist the live catalog back to the custom_providers entry so that + # no-probe surfaces (dashboard, desktop, ACP) show the full model list + # instead of collapsing to the single ``model:`` default. Mirrors the + # picker path in model_switch.py::_save_discovered_models_to_config; a + # failed save is non-fatal. + if live_models: + try: + from hermes_cli.model_switch import ( + _save_discovered_models_to_config, + ) + + _save_discovered_models_to_config(base_url, live_models) + except Exception: + pass if models: default_idx = 0 diff --git a/hermes_cli/model_switch.py b/hermes_cli/model_switch.py index c530daf8bf..a5d7273d33 100644 --- a/hermes_cli/model_switch.py +++ b/hermes_cli/model_switch.py @@ -2648,6 +2648,12 @@ def list_authenticated_providers( "models": [], "has_explicit_models": False, "ep_cfg": ep_cfg, # used below for discover_models / api_key + # Part of group_key, so it is constant across the group. + # The render loop below needs it to key the model cache: + # api_mode changes the wire protocol (``x-api-key`` vs + # ``Authorization: Bearer``), so two rows that differ only + # by it must not share a cached catalog. + "api_mode": api_mode, "raw_names": [], "aliases": set(), } @@ -2720,17 +2726,32 @@ def list_authenticated_providers( and _ep_url_norm == _current_base_url_norm ) ) - should_probe = _can_probe_custom_provider(row_is_current=_ep_is_current) and bool(api_url) and discover and ( - bool(api_key) or not has_explicit_models + # See section 4: when live probing is suppressed for latency, a + # warm same-fingerprint cache entry still serves the full catalog + # with no network round-trip. + # + # ``has_explicit_models`` gates the *probe*, not the cache read: + # it exists so a keyless endpoint with a declared catalog is not + # hammered over the network (5f00f36ba, 1039e90b5). Reading a + # catalog an earlier probe already paid for costs nothing, and + # applying the probe gate to it re-pins the endpoint — see + # ``_discovery_allowed`` in section 4 for the full rationale. + _discovery_allowed = bool(api_url) and discover + _probe_live = ( + _discovery_allowed + and (bool(api_key) or not has_explicit_models) + and _can_probe_custom_provider(row_is_current=_ep_is_current) ) - if should_probe: + if _discovery_allowed: try: from hermes_cli.models import cached_fetch_api_models live_models = cached_fetch_api_models( api_key, api_url, timeout=1.5 if for_picker else 5.0, # picker: fail fast so a slow custom endpoint doesn't block /model + api_mode=grp.get("api_mode") or None, headers=_extra_headers_from_config(ep_cfg) or None, + cache_only=not _probe_live, ) if live_models: models_list = live_models @@ -2793,19 +2814,22 @@ def list_authenticated_providers( ) ): _models = [current_model] if current_model else [] - if refresh or probe_current_custom_provider: - try: - from hermes_cli.models import cached_fetch_api_models + # As in sections 3 and 4: with live probing suppressed, fall back to + # the cached catalog rather than to the single active model. + _probe_live = bool(refresh or probe_current_custom_provider) + try: + from hermes_cli.models import cached_fetch_api_models - _live_models = cached_fetch_api_models( - "", - str(current_base_url).strip().rstrip("/"), - timeout=1.5 if for_picker else 5.0, # picker: fail fast on a slow current endpoint - ) - if _live_models: - _models = _live_models - except Exception: - pass + _live_models = cached_fetch_api_models( + "", + str(current_base_url).strip().rstrip("/"), + timeout=1.5 if for_picker else 5.0, # picker: fail fast on a slow current endpoint + cache_only=not _probe_live, + ) + if _live_models: + _models = _live_models + except Exception: + pass results.append({ "slug": "custom", "name": "Custom endpoint", @@ -2911,6 +2935,11 @@ def list_authenticated_providers( "has_explicit_models": False, "discover_models": discover, "extra_headers": entry_extra_headers, + # Part of group_key, so constant across the group. Needed + # in the render loop to key the model cache — api_mode + # selects the wire protocol, so rows differing only by it + # must not share a cached catalog. + "api_mode": api_mode, "aliases": set(), } else: @@ -3031,13 +3060,33 @@ def list_authenticated_providers( and _grp_url_norm == _current_base_url_norm and _current_base_url_group_count == 1 ) - should_probe = ( - _can_probe_custom_provider(row_is_current=_grp_is_current) - and bool(api_url) + # Discovery is what the user's config asks for; probing is how we + # get it. When the caller suppresses live probing for latency, the + # already-discovered catalog on disk still answers the question + # without a round-trip — skipping it too is what collapsed a + # multi-model endpoint to its config-declared subset. + # + # ``has_explicit_models`` belongs on the probe side of that line. + # It is a network-cost gate: don't hammer a keyless endpoint that + # already declares its catalog (5f00f36ba, 1039e90b5). It is not a + # user pin — ``discover_models: false`` is the documented way to + # pin, and it is honored above. + # + # Keeping it on the discovery side re-pins the endpoint it was + # meant to spare, because a successful probe calls + # ``_save_discovered_models_to_config()``, which writes a plain + # list — the exact shape ``_models_config_is_allowlist()`` reads + # back as an explicit allowlist. A keyless local server therefore + # self-pins on its first probe and can never widen again. f66319097 + # already carved the dict shape out of that trap for the same + # reason; the list shape is the other door into it. + _discovery_allowed = bool(api_url) and grp.get("discover_models", True) + _probe_live = ( + _discovery_allowed and (bool(api_key) or not grp.get("has_explicit_models")) - and grp.get("discover_models", True) + and _can_probe_custom_provider(row_is_current=_grp_is_current) ) - if should_probe: + if _discovery_allowed: try: from hermes_cli.models import cached_fetch_api_models @@ -3045,7 +3094,9 @@ def list_authenticated_providers( api_key, api_url, timeout=1.5 if for_picker else 5.0, # picker: fail fast so a slow custom endpoint doesn't block /model + api_mode=grp.get("api_mode") or None, headers=grp.get("extra_headers") or None, + cache_only=not _probe_live, ) if live_models: grp["models"] = live_models @@ -3053,9 +3104,12 @@ def list_authenticated_providers( # Auto-save discovered models back to config so # ``discover_models: false`` has a populated cache # on the next read. A failed save is non-fatal. - _save_discovered_models_to_config( - api_url, live_models - ) + # Only after a real probe: a cache hit is already the + # product of an earlier probe that saved it. + if _probe_live: + _save_discovered_models_to_config( + api_url, live_models + ) except Exception: pass results.append({ diff --git a/hermes_cli/models.py b/hermes_cli/models.py index b8ca965c02..f9714bc307 100644 --- a/hermes_cli/models.py +++ b/hermes_cli/models.py @@ -4704,6 +4704,7 @@ def cached_fetch_api_models( api_mode: Optional[str] = None, headers: Optional[dict[str, str]] = None, force_refresh: bool = False, + cache_only: bool = False, ttl_seconds: int = _PROVIDER_MODELS_CACHE_TTL, ) -> Optional[list[str]]: """Disk-cached wrapper around :func:`fetch_api_models` for custom endpoints. @@ -4717,9 +4718,18 @@ def cached_fetch_api_models( last same-fingerprint result rather than an empty list. Returns whatever :func:`fetch_api_models` would (a list or ``None``); corrupt cache rows degrade to a live fetch instead of raising. + + ``cache_only`` serves a previously-discovered catalog without touching + the network at all — no live fetch, no background revalidation — and + returns ``None`` when nothing usable is cached. Callers that deliberately + skip live probing for latency reasons (GUI picker opens, which must not + block on a stopped local endpoint) use this so a warm catalog still + reaches the picker instead of collapsing to the config-declared subset. """ normalized_url = str(base_url or "").strip().rstrip("/").lower() if not normalized_url: + if cache_only: + return None # No base_url means nothing to key the cache on — fall through to a # live call so callers keep getting fetch_api_models' own behavior. return fetch_api_models( @@ -4732,6 +4742,17 @@ def cached_fetch_api_models( entry = cache.get(cache_key) now = time.time() + if cache_only: + # Same trust window as the stale-while-revalidate tier below, minus + # the revalidation: an entry this side of the bound is good enough to + # render, and anything older is treated as a miss so the caller falls + # back to its configured list rather than showing a stale catalog. + if force_refresh or not _cache_entry_valid(entry, fp): + return None + if now - entry["at"] >= _PROVIDER_MODELS_STALE_SERVE_MAX: + return None + return list(entry["models"]) + if not force_refresh and _cache_entry_valid(entry, fp): age = now - entry["at"] if age < ttl_seconds: diff --git a/tests/hermes_cli/test_cached_fetch_api_models.py b/tests/hermes_cli/test_cached_fetch_api_models.py index 346a0e4f76..24246fd0ea 100644 --- a/tests/hermes_cli/test_cached_fetch_api_models.py +++ b/tests/hermes_cli/test_cached_fetch_api_models.py @@ -128,6 +128,73 @@ class TestCachedFetchApiModels: assert out is None save.assert_not_called() + +class TestCacheOnly: + """``cache_only=True`` is the no-network read used by picker opens that + deliberately skip live probing. It must answer from disk or not at all — + never a live fetch, never a background revalidation.""" + + def _entry(self, models, age_seconds, fp="fp"): + return {"fp": fp, "at": time.time() - age_seconds, "models": list(models)} + + def _call(self, cache, *, fp="fp", **kwargs): + import hermes_cli.models as mod + + with patch.object(mod, "_load_provider_models_cache", return_value=cache), \ + patch.object(mod, "_custom_endpoint_fingerprint", return_value=fp), \ + patch.object(mod, "_save_provider_models_cache") as save, \ + patch.object(mod, "_spawn_swr_refresh") as swr, \ + patch.object(mod, "fetch_api_models") as live: + out = mod.cached_fetch_api_models( + "sk-key", "https://gw.example.com/v1", cache_only=True, **kwargs + ) + live.assert_not_called() + swr.assert_not_called() + save.assert_not_called() + return out + + def test_fresh_entry_is_served(self): + cache = {"custom:https://gw.example.com/v1": self._entry(["m1", "m2"], 10)} + assert self._call(cache) == ["m1", "m2"] + + def test_entry_past_ttl_is_still_served_within_the_stale_window(self): + """The TTL governs when to *revalidate*, and cache_only cannot. Inside + the stale-serve bound the entry is still the best answer available — + collapsing to the config subset an hour in would reintroduce the bug.""" + import hermes_cli.models as mod + + age = mod._PROVIDER_MODELS_CACHE_TTL + 60 + cache = {"custom:https://gw.example.com/v1": self._entry(["m1", "m2"], age)} + assert self._call(cache) == ["m1", "m2"] + + def test_entry_beyond_the_stale_window_is_a_miss(self): + import hermes_cli.models as mod + + age = mod._PROVIDER_MODELS_STALE_SERVE_MAX + 60 + cache = {"custom:https://gw.example.com/v1": self._entry(["ancient"], age)} + assert self._call(cache) is None + + def test_empty_cache_is_a_miss(self): + assert self._call({}) is None + + def test_rotated_credentials_are_a_miss(self): + cache = {"custom:https://gw.example.com/v1": self._entry(["old"], 10, fp="old-fp")} + assert self._call(cache, fp="new-fp") is None + + def test_force_refresh_is_a_miss_rather_than_a_live_fetch(self): + """cache_only outranks force_refresh: the caller has said no network, + so an un-revalidatable entry is withheld instead of fetched.""" + cache = {"custom:https://gw.example.com/v1": self._entry(["m1"], 10)} + assert self._call(cache, force_refresh=True) is None + + def test_missing_base_url_is_a_miss_rather_than_a_live_fetch(self): + import hermes_cli.models as mod + + with patch.object(mod, "fetch_api_models") as live: + out = mod.cached_fetch_api_models("sk-key", "", cache_only=True) + assert out is None + live.assert_not_called() + def test_empty_live_result_is_not_persisted(self): """An empty list from a transient error must never pin an empty cache entry over real data on the next open.""" diff --git a/tests/hermes_cli/test_model_switch_custom_providers.py b/tests/hermes_cli/test_model_switch_custom_providers.py index e7139e7dac..381c5870c9 100644 --- a/tests/hermes_cli/test_model_switch_custom_providers.py +++ b/tests/hermes_cli/test_model_switch_custom_providers.py @@ -5,9 +5,16 @@ shared slash-command pipeline (`/model` in CLI/gateway/Telegram) historically only looked at `providers:`. """ +import time + import hermes_cli.providers as providers_mod import pytest -from hermes_cli.model_switch import list_authenticated_providers, switch_model +import yaml +from hermes_cli.model_switch import ( + _save_discovered_models_to_config, + list_authenticated_providers, + switch_model, +) from hermes_cli.providers import resolve_provider_full @@ -830,6 +837,74 @@ def test_save_discovered_models_preserves_dict_form(monkeypatch): ) +def test_model_flow_named_custom_persists_discovered_models(monkeypatch): + """The ``hermes model`` named-custom-provider flow persists the discovered + catalog back to the entry's ``models:`` list. + + No-probe surfaces (dashboard, desktop, ACP) call + ``build_models_payload(..., probe_custom_providers=False)`` and only show + the configured ``models:`` list. The CLI flow probes and shows the full + catalog but (before this fix) never saved it, so a provider added via + ``hermes model`` collapsed to the single ``model:`` default everywhere but + the CLI. It must persist discovered models the same way the picker path in + ``_save_discovered_models_to_config`` does. + """ + monkeypatch.setattr( + "hermes_cli.models.fetch_api_models", + lambda api_key, base_url, **kw: [ + "discovered-a", + "discovered-b", + "discovered-c", + ], + ) + # Non-interactive model selection. + monkeypatch.setattr( + "hermes_cli.curses_ui.curses_radiolist", lambda *a, **k: 0 + ) + # No-op downstream writes so the test never touches a real config. + monkeypatch.setattr("hermes_cli.main._save_custom_provider", lambda *a, **k: None) + monkeypatch.setattr("hermes_cli.auth._save_model_choice", lambda *a, **k: None) + monkeypatch.setattr("hermes_cli.auth.deactivate_provider", lambda *a, **k: None) + monkeypatch.setattr( + "hermes_cli.config.load_config", + lambda: {"model": {}, "providers": {}, "custom_providers": []}, + ) + monkeypatch.setattr("hermes_cli.config.save_config", lambda cfg: None) + + save_calls = [] + monkeypatch.setattr( + "hermes_cli.model_switch._save_discovered_models_to_config", + lambda api_url, model_ids: save_calls.append((api_url, model_ids)), + ) + + from hermes_cli.model_setup_flows import _model_flow_named_custom + + _model_flow_named_custom( + {}, + { + "name": "Dragomes", + "base_url": "http://example.com/v1", + "api_mode": "anthropic_messages", + "api_key": "sk-test", + "key_env": "", + "model": "MiniMax-M3", + "provider_key": "", + "discover_models": True, + "models": {}, + }, + ) + + assert save_calls == [ + ( + "http://example.com/v1", + ["discovered-a", "discovered-b", "discovered-c"], + ) + ], ( + "_model_flow_named_custom must persist discovered models " + "(base_url, model_ids) after a successful probe" + ) + + def test_shared_url_different_display_names_are_separate_rows(monkeypatch): """Multiple custom_providers entries sharing base_url + api_key + api_mode but with *different* display-name prefixes (e.g. a proxy fronting @@ -972,3 +1047,399 @@ def test_custom_provider_dict_models_pin_requires_discover_false(monkeypatch): row = next(p for p in providers if p["name"] == "Local Ollama") assert calls == [] assert row["models"] == ["llama3"] + + +# ─── No-probe picker opens still serve the cached catalog ─────────────── +# +# #58183 stopped GUI picker opens from live-probing saved custom endpoints so +# a stopped local server could not stall the picker. It skipped the cached +# read along with the network one, so a non-current endpoint collapsed to the +# one model named in config even with a full catalog already on disk. These +# pin both halves: the cache is served, the network is not touched. + + +_LOCAL_ENDPOINT = "http://127.0.0.1:8000/v1" +_LOCAL_CATALOG = [f"omlx-model-{i}" for i in range(1, 9)] +_SHARED_PROXY_URL = "https://proxy.example.com/v1" + + +def _seed_custom_model_cache(monkeypatch, models, *, age_seconds=10): + """Put *models* on disk for ``_LOCAL_ENDPOINT`` under the no-credential + fingerprint the picker probes local endpoints with.""" + import hermes_cli.models as models_mod + + fp = models_mod._custom_endpoint_fingerprint("", None, None) + cache = { + f"custom:{_LOCAL_ENDPOINT}": { + "fp": fp, + "at": time.time() - age_seconds, + "models": list(models), + } + } + monkeypatch.setattr(models_mod, "_load_provider_models_cache", lambda: cache) + + +def _no_probe_local_row(monkeypatch, *, custom_providers=None, user_providers=None, + current_provider="nous", **kwargs): + """Run the GUI picker path (no live probing) and return the local row + plus every base_url a live fetch was attempted against.""" + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr(providers_mod, "HERMES_OVERLAYS", {}) + fetched = [] + + def fetch(_api_key, base_url, **_kwargs): + fetched.append(base_url) + return ["should-not-be-reached"] + + monkeypatch.setattr("hermes_cli.models.fetch_api_models", fetch) + + providers = list_authenticated_providers( + current_provider=current_provider, + user_providers=user_providers or {}, + custom_providers=custom_providers or [], + for_picker=True, + refresh=False, + probe_custom_providers=False, + probe_current_custom_provider=True, + **kwargs, + ) + row = next( + (p for p in providers if _LOCAL_ENDPOINT in str(p.get("api_url", ""))), None + ) + return row, fetched + + +def test_no_probe_open_serves_cached_catalog_for_custom_provider(monkeypatch): + """A ``custom_providers`` endpoint that is not the current provider still + shows its full discovered catalog, from cache, with no network call.""" + _seed_custom_model_cache(monkeypatch, _LOCAL_CATALOG) + + row, fetched = _no_probe_local_row( + monkeypatch, + custom_providers=[ + { + "name": "Local (127.0.0.1:8000)", + "base_url": _LOCAL_ENDPOINT, + "model": "omlx-model-1", + } + ], + ) + + assert row is not None + assert row["is_current"] is False + assert row["models"] == _LOCAL_CATALOG + assert row["total_models"] == len(_LOCAL_CATALOG) + assert fetched == [] + + +def test_no_probe_open_serves_cached_catalog_for_user_provider(monkeypatch): + """Same contract for a ``providers:`` entry (section 3).""" + _seed_custom_model_cache(monkeypatch, _LOCAL_CATALOG) + + row, fetched = _no_probe_local_row( + monkeypatch, + user_providers={ + "local-8000": { + "name": "Local (127.0.0.1:8000)", + "base_url": _LOCAL_ENDPOINT, + "default_model": "omlx-model-1", + } + }, + ) + + assert row is not None + assert row["models"] == _LOCAL_CATALOG + assert fetched == [] + + +def test_no_probe_open_serves_cached_catalog_for_bare_custom_endpoint(monkeypatch): + """Same contract for the bare ``provider: custom`` shape (section 3b), + where the fallback would otherwise be the single active model.""" + _seed_custom_model_cache(monkeypatch, _LOCAL_CATALOG) + + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr(providers_mod, "HERMES_OVERLAYS", {}) + fetched = [] + monkeypatch.setattr( + "hermes_cli.models.fetch_api_models", + lambda _k, base_url, **_kw: (fetched.append(base_url), None)[1], + ) + + providers = list_authenticated_providers( + current_provider="custom", + current_base_url=_LOCAL_ENDPOINT, + current_model="omlx-model-1", + user_providers={}, + custom_providers=[], + for_picker=True, + refresh=False, + probe_custom_providers=False, + probe_current_custom_provider=False, + ) + + row = next(p for p in providers if p["slug"] == "custom") + assert row["models"] == _LOCAL_CATALOG + assert fetched == [] + + +def test_no_probe_open_without_cache_keeps_configured_models_and_stays_offline( + monkeypatch, +): + """The #58183 guarantee: a cold cache must not trigger a live probe. The + row degrades to its configured list rather than stalling on a dead port.""" + _seed_custom_model_cache(monkeypatch, [], age_seconds=10) + + row, fetched = _no_probe_local_row( + monkeypatch, + custom_providers=[ + { + "name": "Local (127.0.0.1:8000)", + "base_url": _LOCAL_ENDPOINT, + "model": "omlx-model-1", + } + ], + ) + + assert row is not None + assert row["models"] == ["omlx-model-1"] + assert fetched == [] + + +def test_no_probe_open_respects_discover_models_false(monkeypatch): + """A user who pinned their catalog must not have it replaced from cache.""" + _seed_custom_model_cache(monkeypatch, _LOCAL_CATALOG) + + row, fetched = _no_probe_local_row( + monkeypatch, + custom_providers=[ + { + "name": "Local (127.0.0.1:8000)", + "base_url": _LOCAL_ENDPOINT, + "model": "pinned-model", + "models": ["pinned-model"], + "discover_models": False, + } + ], + ) + + assert row is not None + assert row["models"] == ["pinned-model"] + assert fetched == [] + + +def test_cached_catalog_is_not_written_back_to_config(monkeypatch): + """Only a real probe persists discovered models; a cache hit is already + the product of the probe that saved it.""" + _seed_custom_model_cache(monkeypatch, _LOCAL_CATALOG) + saves = [] + monkeypatch.setattr( + "hermes_cli.model_switch._save_discovered_models_to_config", + lambda api_url, model_ids: saves.append((api_url, model_ids)), + ) + + row, _ = _no_probe_local_row( + monkeypatch, + custom_providers=[ + { + "name": "Local (127.0.0.1:8000)", + "base_url": _LOCAL_ENDPOINT, + "model": "omlx-model-1", + } + ], + ) + + assert row["models"] == _LOCAL_CATALOG + assert saves == [] + + +def test_keyless_endpoint_with_saved_catalog_still_reads_cache(monkeypatch): + """A keyless local server must not be pinned by Hermes' own auto-save. + + ``_save_discovered_models_to_config()`` writes a plain list into + ``models:``, which ``_models_config_is_allowlist()`` reads back as an + explicit allowlist. Combined with the no-key discovery gate, a keyless + endpoint (the common local-model-server shape) froze on the catalog of + its first probe and could never widen again — the exact "lineup changes + after config was written" case. The cache read must not be subject to the + probe's network-cost gate. + """ + _seed_custom_model_cache(monkeypatch, _LOCAL_CATALOG) + + row, fetched = _no_probe_local_row( + monkeypatch, + custom_providers=[ + { + "name": "Local (127.0.0.1:8000)", + "base_url": _LOCAL_ENDPOINT, + "model": "omlx-model-1", + # No api_key, and a models: list of the shape our own + # auto-save writes after a successful probe. + "models": ["omlx-model-1"], + } + ], + ) + + assert row is not None + assert row["models"] == _LOCAL_CATALOG + assert fetched == [] + + +def test_keyless_endpoint_with_saved_catalog_is_still_not_probed(monkeypatch): + """...but the network-cost gate it rides on must survive intact. + + The no-key + declared-catalog combination exists to keep Hermes from + probing an endpoint it cannot authenticate to. Serving that endpoint from + a warm cache is free; hitting the network is not. With a cold cache and + live probing fully enabled, this row must still make zero fetches. + """ + _seed_custom_model_cache(monkeypatch, []) # cold: only a probe could answer + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr(providers_mod, "HERMES_OVERLAYS", {}) + + fetched = [] + + def fetch(_api_key, base_url, **_kwargs): + fetched.append(base_url) + return ["should-not-be-reached"] + + monkeypatch.setattr("hermes_cli.models.fetch_api_models", fetch) + + providers = list_authenticated_providers( + current_provider="nous", + user_providers={}, + custom_providers=[ + { + "name": "Local (127.0.0.1:8000)", + "base_url": _LOCAL_ENDPOINT, + "model": "omlx-model-1", + "models": ["omlx-model-1"], + } + ], + for_picker=True, + refresh=False, + probe_custom_providers=True, # live probing fully enabled + ) + row = next( + (p for p in providers if _LOCAL_ENDPOINT in str(p.get("api_url", ""))), None + ) + + assert row is not None + assert row["models"] == ["omlx-model-1"] + assert fetched == [] + + +def test_api_mode_rows_do_not_share_a_cached_catalog(monkeypatch): + """Two rows differing only by ``api_mode`` must not share a cache entry. + + ``api_mode`` selects the wire protocol — ``x-api-key`` + + ``anthropic-version`` versus ``Authorization: Bearer`` — so it is part of + both the picker's group identity and + ``_custom_endpoint_fingerprint()``. The cache read has to pass it through + or an ``anthropic_messages`` row renders whatever the OpenAI-mode row + cached against the same base_url. + """ + import hermes_cli.models as models_mod + + openai_catalog = ["gpt-oss-a", "gpt-oss-b"] + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr(providers_mod, "HERMES_OVERLAYS", {}) + + fetched = [] + + def fetch(_api_key, base_url, **_kwargs): + fetched.append(base_url) + return ["should-not-be-reached"] + + monkeypatch.setattr("hermes_cli.models.fetch_api_models", fetch) + + # Only the OpenAI-mode probe (api_mode=None) is on disk. + fp = models_mod._custom_endpoint_fingerprint("sk-shared", None, None) + cache = { + f"custom:{_SHARED_PROXY_URL}": { + "fp": fp, + "at": time.time() - 10, + "models": list(openai_catalog), + } + } + monkeypatch.setattr(models_mod, "_load_provider_models_cache", lambda: cache) + + def _row(entry): + providers = list_authenticated_providers( + current_provider="nous", + user_providers={}, + custom_providers=[entry], + for_picker=True, + refresh=False, + probe_custom_providers=False, + probe_current_custom_provider=True, + ) + return next( + (p for p in providers if _SHARED_PROXY_URL in str(p.get("api_url", ""))), + None, + ) + + anthropic_row = _row( + { + "name": "Proxy Anthropic", + "base_url": _SHARED_PROXY_URL, + "api_key": "sk-shared", + "api_mode": "anthropic_messages", + "model": "claude-via-proxy", + } + ) + openai_row = _row( + { + "name": "Proxy OpenAI", + "base_url": _SHARED_PROXY_URL, + "api_key": "sk-shared", + "model": "gpt-via-proxy", + } + ) + + assert anthropic_row is not None and openai_row is not None + assert anthropic_row["models"] == ["claude-via-proxy"], ( + "an anthropic_messages row must not render the OpenAI-mode catalog " + "cached against the same base_url" + ) + # ...while the row the entry actually belongs to still resolves. + assert openai_row["models"] == openai_catalog + assert fetched == [] + + +def test_auto_saved_catalog_round_trips_without_pinning(tmp_path, monkeypatch): + """End-to-end: the shape we persist must not read back as a user pin. + + Guards the whole chain rather than one branch — probe saves a catalog, + config is reloaded, and the endpoint must still be discoverable. If a + future change makes the saved shape look like an intentional allowlist + again, this fails even if the gate logic above is refactored away. + """ + import hermes_cli.config as config_mod + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + cfg_path = tmp_path / "config.yaml" + cfg_path.write_text( + "custom_providers:\n" + f" - name: Local MLX\n base_url: {_LOCAL_ENDPOINT}\n" + " model: omlx-model-1\n" + ) + monkeypatch.setattr(config_mod, "CONFIG_PATH", str(cfg_path), raising=False) + + _save_discovered_models_to_config(_LOCAL_ENDPOINT, list(_LOCAL_CATALOG)) + + saved = yaml.safe_load(cfg_path.read_text())["custom_providers"][0] + assert saved["models"] == _LOCAL_CATALOG, "probe result should be persisted" + + # The persisted shape is what the picker will read on the next open. It + # must not, on a keyless entry, suppress discovery of a wider catalog. + _seed_custom_model_cache(monkeypatch, [*_LOCAL_CATALOG, "omlx-model-9"]) + row, fetched = _no_probe_local_row( + monkeypatch, custom_providers=[saved] + ) + + assert row is not None + assert row["models"] == [*_LOCAL_CATALOG, "omlx-model-9"], ( + "an auto-saved catalog must not pin the endpoint against a newer " + "cached lineup" + ) + assert fetched == []