diff --git a/hermes_cli/model_switch.py b/hermes_cli/model_switch.py index 66dbdc09aaaca..c530daf8bf1ba 100644 --- a/hermes_cli/model_switch.py +++ b/hermes_cli/model_switch.py @@ -2725,8 +2725,8 @@ def list_authenticated_providers( ) if should_probe: try: - from hermes_cli.models import fetch_api_models - live_models = fetch_api_models( + 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 @@ -2795,9 +2795,9 @@ def list_authenticated_providers( _models = [current_model] if current_model else [] if refresh or probe_current_custom_provider: try: - from hermes_cli.models import fetch_api_models + from hermes_cli.models import cached_fetch_api_models - _live_models = 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 @@ -3039,9 +3039,9 @@ def list_authenticated_providers( ) if should_probe: try: - from hermes_cli.models import fetch_api_models + from hermes_cli.models import cached_fetch_api_models - live_models = 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 diff --git a/hermes_cli/models.py b/hermes_cli/models.py index 2a4e342b51bcf..5c92c20ca2eb2 100644 --- a/hermes_cli/models.py +++ b/hermes_cli/models.py @@ -4643,6 +4643,101 @@ def fetch_api_models( ).get("models") +def _custom_endpoint_fingerprint( + api_key: Optional[str], + api_mode: Optional[str], + headers: Optional[dict[str, str]], +) -> str: + """Fingerprint the credentials/wire-shape used to probe a custom endpoint. + + Custom OpenAI-compatible endpoints have no ``PROVIDER_REGISTRY`` slug to + key off (unlike ``_credential_fingerprint``), so this hashes exactly the + values callers pass to :func:`fetch_api_models`: a rotated ``api_key``, a + changed ``api_mode``, or an edited ``extra_headers`` block each bust the + cache entry on their own. + """ + import hashlib + + blob = "|".join(( + api_key or "", + api_mode or "", + json.dumps(headers or {}, sort_keys=True), + )).encode("utf-8", errors="replace") + # blake2b for cache-key fingerprinting only, same rationale as + # _credential_fingerprint (avoids CodeQL's sha256-over-secrets rule). + return hashlib.blake2b(blob, digest_size=8).hexdigest() + + +def cached_fetch_api_models( + api_key: Optional[str], + base_url: Optional[str], + *, + timeout: float = 5.0, + api_mode: Optional[str] = None, + headers: Optional[dict[str, str]] = None, + force_refresh: bool = False, + ttl_seconds: int = _PROVIDER_MODELS_CACHE_TTL, +) -> Optional[list[str]]: + """Disk-cached wrapper around :func:`fetch_api_models` for custom endpoints. + + Mirrors :func:`cached_provider_model_ids` but keys + ``provider_models_cache.json`` off ``custom:`` instead of a + ``PROVIDER_REGISTRY`` slug, since custom endpoints (named + ``custom_providers`` rows, bare ``provider: custom``, and per-endpoint-map + entries) have none. Same stale-beats-nothing fallback policy as + ``cached_provider_model_ids``: a live-fetch failure serves the last + same-fingerprint result rather than an empty list. Always returns + whatever :func:`fetch_api_models` would (a list or ``None``), never + raises. + """ + # Only forward api_mode when the caller actually set it — none of the + # current probe call sites do, and omitting it (rather than passing + # api_mode=None) keeps the live-call signature identical to a direct + # fetch_api_models() call. + live_kwargs: dict[str, Any] = {"timeout": timeout, "headers": headers} + if api_mode is not None: + live_kwargs["api_mode"] = api_mode + + normalized_url = str(base_url or "").strip().rstrip("/").lower() + if not normalized_url: + # 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(api_key, base_url, **live_kwargs) + + cache_key = f"custom:{normalized_url}" + fp = _custom_endpoint_fingerprint(api_key, api_mode, headers) + cache = _load_provider_models_cache() + entry = cache.get(cache_key) + now = time.time() + + if ( + not force_refresh + and isinstance(entry, dict) + and entry.get("fp") == fp + and isinstance(entry.get("models"), list) + and entry["models"] + and (now - float(entry.get("at", 0))) < ttl_seconds + ): + return list(entry["models"]) + + live = fetch_api_models(api_key, base_url, **live_kwargs) + if live: + cache[cache_key] = {"fp": fp, "at": now, "models": list(live)} + _save_provider_models_cache(cache) + return list(live) + + # Live fetch returned nothing (offline endpoint, timeout, auth hiccup). + # A stale same-fingerprint entry beats an empty result. + if ( + isinstance(entry, dict) + and entry.get("fp") == fp + and isinstance(entry.get("models"), list) + and entry["models"] + ): + return list(entry["models"]) + return live + + # --------------------------------------------------------------------------- # Ollama Cloud — merged model discovery with disk cache # --------------------------------------------------------------------------- diff --git a/tests/hermes_cli/test_cached_fetch_api_models.py b/tests/hermes_cli/test_cached_fetch_api_models.py new file mode 100644 index 0000000000000..05cc27d0cd816 --- /dev/null +++ b/tests/hermes_cli/test_cached_fetch_api_models.py @@ -0,0 +1,207 @@ +"""Cache-contract tests for ``cached_fetch_api_models()``. + +Custom OpenAI-compatible endpoints (named ``custom_providers`` rows, bare +``provider: custom``, and per-endpoint-map entries) previously called +``fetch_api_models()`` directly with no disk cache, so the current custom +endpoint's ``/v1/models`` got a live HTTP round-trip on literally every +``/model`` open (#72762). ``cached_fetch_api_models()`` gives custom +endpoints the same ``provider_models_cache.json`` TTL cache first-class +providers already get via ``cached_provider_model_ids()``. + +These pin the cache contract directly (hit / stale / rotation / refresh / +fallback), separate from ``test_model_switch_custom_providers.py``'s +higher-level picker-shape tests. +""" + +from __future__ import annotations + +import time +from unittest.mock import patch + +import pytest + + +class TestCachedFetchApiModels: + def _entry(self, models, age_seconds, fp="fp"): + return {"fp": fp, "at": time.time() - age_seconds, "models": list(models)} + + def test_fresh_entry_served_without_live_fetch(self): + import hermes_cli.models as mod + + cache = {"custom:https://gw.example.com/v1": self._entry(["m1", "m2"], age_seconds=10)} + 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, "fetch_api_models") as live: + out = mod.cached_fetch_api_models("sk-key", "https://gw.example.com/v1") + assert out == ["m1", "m2"] + live.assert_not_called() + save.assert_not_called() + + def test_cache_key_normalizes_trailing_slash_and_case(self): + """A saved entry for the lowercased/rstripped URL must be hit even + when the caller passes a differently-cased URL with a trailing + slash — config.yaml entries are not guaranteed to be normalized.""" + import hermes_cli.models as mod + + cache = {"custom:https://gw.example.com/v1": self._entry(["m1"], age_seconds=10)} + with patch.object(mod, "_load_provider_models_cache", return_value=cache), \ + patch.object(mod, "_custom_endpoint_fingerprint", return_value="fp"), \ + patch.object(mod, "fetch_api_models") as live: + out = mod.cached_fetch_api_models("sk-key", "HTTPS://GW.example.com/v1/") + assert out == ["m1"] + live.assert_not_called() + + def test_expired_entry_triggers_live_fetch_and_is_persisted(self): + import hermes_cli.models as mod + + cache = {"custom:https://gw.example.com/v1": self._entry(["old"], age_seconds=99999)} + saved = {} + 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", side_effect=saved.update), \ + patch.object(mod, "fetch_api_models", return_value=["fresh-a", "fresh-b"]) as live: + out = mod.cached_fetch_api_models( + "sk-key", "https://gw.example.com/v1", ttl_seconds=3600 + ) + assert out == ["fresh-a", "fresh-b"] + live.assert_called_once() + assert saved["custom:https://gw.example.com/v1"]["models"] == ["fresh-a", "fresh-b"] + assert saved["custom:https://gw.example.com/v1"]["fp"] == "fp" + + def test_rotated_api_key_busts_cache_even_when_fresh(self): + """A same-age entry with a DIFFERENT fingerprint (key rotated, or + extra_headers edited) must not be served — it reflects the old + credentials' catalog.""" + import hermes_cli.models as mod + + cache = {"custom:https://gw.example.com/v1": self._entry(["old-key-models"], 10, fp="old-fp")} + with patch.object(mod, "_load_provider_models_cache", return_value=cache), \ + patch.object(mod, "_custom_endpoint_fingerprint", return_value="new-fp"), \ + patch.object(mod, "_save_provider_models_cache"), \ + patch.object(mod, "fetch_api_models", return_value=["new-key-models"]) as live: + out = mod.cached_fetch_api_models("sk-new-key", "https://gw.example.com/v1") + assert out == ["new-key-models"] + live.assert_called_once() + + def test_force_refresh_bypasses_fresh_cache(self): + import hermes_cli.models as mod + + cache = {"custom:https://gw.example.com/v1": self._entry(["stale-but-fresh"], age_seconds=5)} + 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"), \ + patch.object(mod, "fetch_api_models", return_value=["forced-live"]) as live: + out = mod.cached_fetch_api_models( + "sk-key", "https://gw.example.com/v1", force_refresh=True + ) + assert out == ["forced-live"] + live.assert_called_once() + + def test_live_failure_falls_back_to_stale_same_fingerprint_entry(self): + """Stale data beats no data when the endpoint is flaky (#72762 + proposed-fix: 'same stale-beats-nothing fallback as + cached_provider_model_ids').""" + import hermes_cli.models as mod + + cache = {"custom:https://gw.example.com/v1": self._entry(["last-known-good"], age_seconds=99999, fp="fp")} + 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, "fetch_api_models", return_value=None): + out = mod.cached_fetch_api_models("sk-key", "https://gw.example.com/v1") + assert out == ["last-known-good"] + save.assert_not_called() # nothing new to persist + + def test_live_failure_with_no_matching_entry_returns_live_value(self): + import hermes_cli.models as mod + + with patch.object(mod, "_load_provider_models_cache", return_value={}), \ + patch.object(mod, "_custom_endpoint_fingerprint", return_value="fp"), \ + patch.object(mod, "_save_provider_models_cache") as save, \ + patch.object(mod, "fetch_api_models", return_value=None): + out = mod.cached_fetch_api_models("sk-key", "https://gw.example.com/v1") + assert out is None + save.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.""" + import hermes_cli.models as mod + + with patch.object(mod, "_load_provider_models_cache", return_value={}), \ + patch.object(mod, "_custom_endpoint_fingerprint", return_value="fp"), \ + patch.object(mod, "_save_provider_models_cache") as save, \ + patch.object(mod, "fetch_api_models", return_value=[]): + out = mod.cached_fetch_api_models("sk-key", "https://gw.example.com/v1") + assert out == [] + save.assert_not_called() + + def test_blank_base_url_skips_cache_entirely(self): + """No base_url means nothing to key the cache on — call straight + through to fetch_api_models rather than caching under an empty key.""" + import hermes_cli.models as mod + + with patch.object(mod, "_load_provider_models_cache") as load, \ + patch.object(mod, "fetch_api_models", return_value=["x"]) as live: + out = mod.cached_fetch_api_models("sk-key", "") + assert out == ["x"] + live.assert_called_once() + load.assert_not_called() + + def test_fingerprint_ignores_timeout_but_reacts_to_headers(self): + """Sanity check on the real (non-mocked) fingerprint helper: it must + not vary with call-only params like timeout, but must vary with the + actual credential/header inputs.""" + import hermes_cli.models as mod + + fp_a = mod._custom_endpoint_fingerprint("sk-key", None, {"X-Tenant": "a"}) + fp_b = mod._custom_endpoint_fingerprint("sk-key", None, {"X-Tenant": "b"}) + fp_a_again = mod._custom_endpoint_fingerprint("sk-key", None, {"X-Tenant": "a"}) + assert fp_a != fp_b + assert fp_a == fp_a_again + + +class TestCachedFetchApiModelsDiskRoundTrip: + """End-to-end through the real (per-test-isolated) provider_models_cache.json + disk file rather than mocked load/save, so a regression in the on-disk + schema (e.g. a key collision with provider-slug entries) would show up + here even if the mocked unit tests above stayed green.""" + + def test_second_call_within_ttl_hits_disk_cache_no_live_fetch(self, monkeypatch): + import hermes_cli.models as mod + + calls = [] + + def fake_fetch(api_key, base_url, **kwargs): + calls.append((api_key, base_url)) + return ["disk-cached-model"] + + monkeypatch.setattr(mod, "fetch_api_models", fake_fetch) + + first = mod.cached_fetch_api_models("sk-key", "https://gw.example.com/v1") + second = mod.cached_fetch_api_models("sk-key", "https://gw.example.com/v1") + + assert first == ["disk-cached-model"] + assert second == ["disk-cached-model"] + assert len(calls) == 1, "second open must be served from disk, not a fresh live fetch" + + def test_custom_key_does_not_collide_with_provider_slug_cache(self, monkeypatch): + """A custom endpoint literally named e.g. 'openrouter' in its + base_url must not read/write the same cache slot as the first-class + 'openrouter' provider slug used by cached_provider_model_ids().""" + import hermes_cli.models as mod + + monkeypatch.setattr( + mod, "fetch_api_models", lambda *a, **k: ["custom-endpoint-model"] + ) + monkeypatch.setattr( + mod, "provider_model_ids", lambda *a, **k: ["openrouter-curated-model"] + ) + + mod.cached_fetch_api_models("sk-key", "https://openrouter.ai/v1") + mod.cached_provider_model_ids("openrouter") + + cache = mod._load_provider_models_cache() + assert cache["custom:https://openrouter.ai/v1"]["models"] == ["custom-endpoint-model"] + assert cache["openrouter"]["models"] == ["openrouter-curated-model"] diff --git a/tests/hermes_cli/test_picker_prewarm.py b/tests/hermes_cli/test_picker_prewarm.py index 12dc14ffbc742..a83064ea8751e 100644 --- a/tests/hermes_cli/test_picker_prewarm.py +++ b/tests/hermes_cli/test_picker_prewarm.py @@ -45,3 +45,98 @@ def test_prewarm_guard_is_once_per_process(): _reset_guard() +def test_prewarm_warms_the_active_custom_endpoint_for_the_next_open(monkeypatch): + """End-to-end regression for #72762: the active custom endpoint must be + warm by the time the user opens ``/model``, not just first-class + ``PROVIDER_REGISTRY`` providers. + + The cache is keyed purely on ``base_url`` (see ``cached_fetch_api_models`` + in ``hermes_cli/models.py``), so this is not specific to any named + provider — the fixture below stands in for any OpenAI-compatible custom + endpoint a user might configure (an LLM gateway, Kilo Code, Together AI, + a self-hosted vLLM/SGLang server, ...). + + Runs the real ``list_authenticated_providers()`` (not mocked, unlike the + two tests above) through the prewarm thread against a fake + ``load_picker_context()`` config with one active custom provider, then + replays the exact kwargs the plain CLI ``/model`` handler passes twice in + a row (``probe_custom_providers=False, probe_current_custom_provider=True``), + simulating two ``/model`` opens in one session. + + We deliberately do NOT assert on *which* of (prewarm thread, first + foreground open) wins the race to perform the live fetch — that's a + thread-scheduling detail, not the contract. What must hold regardless of + scheduling: across the warm-up plus two foreground opens, the endpoint is + ever probed live at most once, and every open after that first probe is + served from the disk cache with zero additional network calls. + """ + import hermes_cli.inventory as inventory_mod + import hermes_cli.models as models_mod + + _reset_guard() + + base_url = "https://api.example-gateway.test/v1" + ctx = inventory_mod.ConfigContext( + current_provider="custom:example-gateway", + current_model="", # avoid the unrelated current-model-always-shown guarantee (line ~3104) + current_base_url=base_url, + user_providers={}, + custom_providers=[ + { + "name": "example-gateway", + "base_url": base_url, + "api_key": "sk-gateway-key", + } + ], + excluded_providers=[], + ) + monkeypatch.setattr(inventory_mod, "load_picker_context", lambda: ctx) + + calls = [] + + def fake_fetch_api_models(api_key, url, **kwargs): + calls.append((api_key, url)) + return ["gateway-model-a", "gateway-model-b"] + + monkeypatch.setattr(models_mod, "fetch_api_models", fake_fetch_api_models) + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + + def open_picker(): + return ms.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, + excluded_providers=ctx.excluded_providers, + # Exact kwargs cli.py's plain (no-args, no --refresh) /model + # handler passes: probe only the active custom endpoint, + # everything else from the warm disk cache. + probe_custom_providers=False, + probe_current_custom_provider=True, + ) + + t = ms.prewarm_picker_cache_async() + assert t is not None + t.join(timeout=10) + + first_open = open_picker() + assert len(calls) == 1, ( + "the endpoint must be live-probed at most once across boot prewarm " + "plus the first /model open, however the race between them resolves" + ) + row = next(p for p in first_open if p.get("api_url") == base_url) + assert row["models"] == ["gateway-model-a", "gateway-model-b"] + + second_open = open_picker() + assert len(calls) == 1, ( + "a second /model open in the same session must be served entirely " + "from the disk cache — this is the #72762 regression: previously " + "every open re-probed the endpoint live" + ) + row2 = next(p for p in second_open if p.get("api_url") == base_url) + assert row2["models"] == ["gateway-model-a", "gateway-model-b"] + + _reset_guard() + +