diff --git a/api/routes.py b/api/routes.py index 5f5893ba..86a6769e 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21882,6 +21882,49 @@ def _handle_live_models(handler, parsed): _env = str(_cp.get("key_env") or "").strip() return os.getenv(_env, "").strip() if _env else "" + def _custom_provider_allowlist_ids(_cp): + """Plural ``models`` list only — the explicit allowlist signal. + + The singular ``model`` field is sticky/default metadata, NOT an + allowlist: it must not gate live-catalog filtering, otherwise a + provider configured with only ``model: assistant`` (no ``models`` + list) would be collapsed to a single model. Only an explicit + ``models`` allowlist expresses "show exactly these models". + + The plural value is decoded through ``_parse_config_string_list()`` + because ``hermes config set`` / JSON-mode editor saves persist + lists as quoted JSON-array strings (``'["chat-a","chat-b"]'``) or + Python literals (``"['chat-a']"``). Handling only ``dict``/``list`` + made a serialized allowlist fall through to ``[]``, which the + caller reads as "no allowlist configured" and floods the picker + with the full upstream catalog — the same class of bug as the + ``skills.disabled`` regression (#7120 / #7134). + """ + _ids = [] + _models = _cp.get("models") + # Serialized shapes only: ``hermes config set`` / JSON-mode + # editor saves persist lists as quoted JSON-array strings + # (``'["chat-a","chat-b"]'``) or Python literals + # (``"['chat-a']"``). Decode those through the shared helper so + # they are honored; native dict/list values are walked below so + # dict *entries* keep their id|model|name metadata (the decoder + # str()-ifies list members, which would mangle them). + if isinstance(_models, str): + _models = _parse_config_string_list(_models) + if isinstance(_models, dict): + for _mid in _models: + if isinstance(_mid, str) and _mid.strip(): + _ids.append(_mid.strip()) + elif isinstance(_models, (list, tuple)): + for _item in _models: + if isinstance(_item, str) and _item.strip(): + _ids.append(_item.strip()) + elif isinstance(_item, dict): + _mid = _item.get("id") or _item.get("model") or _item.get("name") + if _mid and str(_mid).strip(): + _ids.append(str(_mid).strip()) + return _ids + # For 'custom' and 'custom:*' providers, provider_model_ids() # returns [] because they aren't real hermes_cli endpoints. # Fall back to the custom_providers entries from config.yaml so @@ -21890,11 +21933,13 @@ def _handle_live_models(handler, parsed): # Collect config-specified model IDs separately so they don't # prevent the live fetch below from running (#3718). _config_ids = [] + _allowlist_ids = [] if provider == "custom" or provider.startswith("custom:"): for _cp in _custom_provider_entries_for_request(): if custom_provider_entry is None: custom_provider_entry = _cp _config_ids.extend(_custom_provider_model_ids(_cp)) + _allowlist_ids.extend(_custom_provider_allowlist_ids(_cp)) # Always try live fetch for custom providers — config entries are a # fallback, not a replacement. The live endpoint should return ALL @@ -21965,13 +22010,25 @@ def _handle_live_models(handler, parsed): except Exception as _fetch_err: logger.debug("Live fetch from custom provider failed: %s", _fetch_err) - # If live fetch succeeded, merge with config entries (live takes - # priority). If live fetch failed, fall back to config-only list. + # If live fetch succeeded, filter the live catalog down to the + # config-declared models allowlist first, then append any + # allowlisted models the live endpoint didn't return. Custom + # providers (esp. New-API-style gateways) expose their ENTIRE + # catalog via /v1/models — including image/audio models that + # are not chat models. The picker must not surface models the + # user never declared in config.yaml. Only an explicit + # ``models`` allowlist gates the filter — a provider configured + # with just a singular ``model`` (no ``models`` list) keeps the + # unfiltered live catalog. When no allowlist is configured, + # return the live list as-is (preserves the pre-filter + # discovery behaviour). if ids: - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) + if _allowlist_ids: + _allowlist_set = set(_allowlist_ids) + ids = [m for m in ids if m in _allowlist_set] or [] + for _aid in _allowlist_ids: + if _aid not in ids: + ids.append(_aid) else: ids = list(_config_ids) diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 8939523b..d5cab25e 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -1,193 +1,194 @@ -"""Regression test for #3718 -- /api/models/live skips probe for custom providers. +"""Handler-level regression tests for /api/models/live custom-provider probing. -When a custom provider (``custom_providers`` entry in config.yaml) has a -``model:`` field but no ``models:`` allowlist, the live endpoint previously -populated ``ids`` from the config entry and then skipped the live ``/v1/models`` -probe (guarded by ``if not ids``). The fix collects config-specified model IDs -in a separate ``_config_ids`` list so the live fetch always runs, and merges -config entries as a fallback after the fetch. +These tests invoke the real ``routes._handle_live_models`` and inspect the +emitted ``models`` payload. Earlier revisions of this file re-implemented the +filtering loop locally, which meant they passed unchanged even with the +production filter deleted (a vacuous oracle). Every case here must fail if the +allowlist gating in ``api/routes.py`` regresses. + +Covered shapes: + * native ``models`` list → live catalog filtered + * JSON-array-string ``models`` → live catalog filtered (#7120 class) + * Python-literal-string ``models`` → live catalog filtered + * singular ``model`` only, no ``models`` → live catalog NOT gated + * malformed serialized ``models`` → safe, treated as scalar name + * allowlisted model absent from live catalog → appended + * live probe failure → falls back to config ids """ + +import io import json -import pathlib -import unittest -from unittest import mock +import sys +import types +from urllib.parse import urlparse -REPO = pathlib.Path(__file__).parent.parent -ROUTES_PY = REPO / "api" / "routes.py" +import pytest -class TestLiveModelsCustomProviderProbe(unittest.TestCase): - """Live endpoint must probe the upstream /v1/models even when config has entries.""" - - def test_custom_provider_with_model_field_probes_upstream(self): - """When a custom provider has model: in config, live fetch must still run. - - Before the fix, _custom_provider_model_ids() populated ids=["assistant"], - and the `if not ids` guard prevented the upstream /v1/models probe from - running. The endpoint returned only ["assistant"] instead of the full - upstream catalog. - - We test the fix by simulating the logic directly: config provides - model IDs, the upstream probe returns a different set, and the result - must contain BOTH the live models and the config fallback. - """ - # Simulate config-specified model IDs - _config_ids = ["assistant"] - - # Simulate upstream /v1/models response - live_models = [ - "assistant", - "assistant-pro", - "assistant-zdr", - "gpt-5.5:pt", - "claude-sonnet-4.6:pt", - ] - - # Simulate the merge logic from the fix: - # if ids: merge config entries not in live set - # else: fall back to config-only list - ids = list(live_models) # live fetch succeeded - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) - - # "assistant" from config should not duplicate the live result - self.assertEqual(ids, live_models) - # All live models must be present - for m in live_models: - self.assertIn(m, ids) - - def test_custom_provider_falls_back_to_config_when_probe_fails(self): - """When the upstream probe fails, config entries must be used as fallback.""" - _config_ids = ["assistant", "custom-only-model"] - - # Simulate failed live fetch: ids stays empty - ids = [] - - # Fallback logic from the fix - if not ids: - ids = list(_config_ids) - - self.assertEqual(ids, ["assistant", "custom-only-model"]) - - def test_config_only_model_appended_when_not_in_live_results(self): - """Config-specified models not in the live results must be appended.""" - _config_ids = ["assistant", "local-finetune"] - live_models = ["assistant", "assistant-pro", "gpt-5.5:pt"] - - ids = list(live_models) - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) - - # "assistant" is in both, "local-finetune" is config-only - self.assertIn("local-finetune", ids) - self.assertIn("assistant-pro", ids) - self.assertEqual(ids.count("assistant"), 1, "assistant must not be duplicated") - - def test_live_fetch_guard_no_longer_checks_ids(self): - """The live fetch block must not be guarded by `if not ids`. - - This is a structural regression guard: the fix replaced - `if not ids and (provider == "custom" ...)` with - `if provider == "custom" ...` so the probe always runs regardless - of whether config entries populated ids. - """ - source = ROUTES_PY.read_text(encoding="utf-8") - - # Find the "Always try live fetch" comment (added by the fix) - marker = "Always try live fetch for custom providers" - self.assertIn(marker, source, ( - "routes.py must contain the 'Always try live fetch' comment (#3718)" - )) - - # Extract the block between the marker and the next major section - marker_pos = source.find(marker) - next_section = source.find("OpenAI-compat live fetch fallback", marker_pos) - self.assertNotEqual(next_section, -1, "could not find next section marker") - block = source[marker_pos:next_section] - - # The old `if not ids and` guard must NOT appear in this block - self.assertNotIn("if not ids and", block, ( - "Live fetch for custom providers must not be guarded by 'if not ids' (#3718)" - )) - - def test_mocked_live_fetch_returns_full_catalog_plus_config_entry(self): - """Integration test: mock urlopen and exercise the real fetch/parse/merge path. - - This test patches urllib.request.urlopen to return a fake /v1/models - response, then runs the same fetch+merge logic that the live handler - uses, verifying that config entries are merged and live models are - included. - """ - fake_models = {"data": [ - {"id": "assistant"}, - {"id": "assistant-pro"}, - {"id": "gpt-5.5:pt"}, - ]} - fake_body = json.dumps(fake_models).encode("utf-8") - fake_resp = mock.MagicMock() - fake_resp.read.return_value = fake_body - fake_resp.__enter__ = mock.MagicMock(return_value=fake_resp) - fake_resp.__exit__ = mock.MagicMock(return_value=False) - - _config_ids = ["assistant", "local-only"] - - with mock.patch("urllib.request.urlopen", return_value=fake_resp): - # Replicate the fetch+parse logic from routes.py - import urllib.request - ids = [] - _req = urllib.request.Request( - "http://localhost:4000/v1/models", - headers={"Authorization": "Bearer test-key"}, - ) - with urllib.request.urlopen(_req, timeout=5) as _resp: - _body = json.loads(_resp.read()) - - if isinstance(_body, dict): - _data = _body.get("data", []) - if isinstance(_data, list): - ids = [m.get("id", "") for m in _data if m.get("id")] - - # Apply the merge logic from the fix - if ids: - _live_set = set(ids) - for _cid in _config_ids: - if _cid not in _live_set: - ids.append(_cid) - else: - ids = list(_config_ids) - - # Live models must all be present - self.assertIn("assistant", ids) - self.assertIn("assistant-pro", ids) - self.assertIn("gpt-5.5:pt", ids) - # Config-only entry must be appended - self.assertIn("local-only", ids) - # No duplicates - self.assertEqual(ids.count("assistant"), 1) - - def test_timeout_uses_config_constant(self): - """The live fetch must use CUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS, not a hardcoded value.""" - source = ROUTES_PY.read_text(encoding="utf-8") - - # Find the custom-provider live fetch block - marker = "Always try live fetch for custom providers" - marker_pos = source.find(marker) - next_section = source.find("OpenAI-compat live fetch fallback", marker_pos) - block = source[marker_pos:next_section] - - # Must use the constant, not a bare number - self.assertIn("CUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS", block, ( - "Live fetch timeout must use CUSTOM_MODELS_ENDPOINT_TIMEOUT_SECONDS " - "instead of a hardcoded value (#3718 review feedback)" - )) - self.assertNotIn("timeout=8", block, ( - "Live fetch must not use hardcoded timeout=8 (#3718 review feedback)" - )) +def _install_provider_model_ids(monkeypatch, fn): + """Stub hermes_cli.models.provider_model_ids (returns [] for custom:*).""" + hermes_cli = types.ModuleType("hermes_cli") + hermes_cli.__path__ = [] + models = types.ModuleType("hermes_cli.models") + models.provider_model_ids = fn + monkeypatch.setitem(sys.modules, "hermes_cli", hermes_cli) + monkeypatch.setitem(sys.modules, "hermes_cli.models", models) -if __name__ == "__main__": - unittest.main() \ No newline at end of file +class _FakeResponse(io.BytesIO): + def __enter__(self): + return self + + def __exit__(self, *_exc): + self.close() + return False + + +def _install_live_catalog(monkeypatch, routes, catalog, *, fail=False): + """Stub the upstream /v1/models probe used by the custom-provider branch.""" + import urllib.request + + requested: list[str] = [] + + def _fake_urlopen(req, timeout=None): + requested.append(getattr(req, "full_url", str(req))) + if fail: + raise OSError("probe failed") + body = {"data": [{"id": mid} for mid in catalog]} + return _FakeResponse(json.dumps(body).encode()) + + monkeypatch.setattr(urllib.request, "urlopen", _fake_urlopen) + return requested + + +def _run_live_models(monkeypatch, routes, custom_provider, *, catalog, fail=False): + """Invoke the real handler for provider=custom:test-gateway.""" + import api.config as config + import api.profiles as profiles + + routes._clear_live_models_cache() + monkeypatch.setattr( + routes, "j", lambda _handler, payload, status=200, extra_headers=None: payload + ) + monkeypatch.setattr(config, "_resolve_provider_alias", lambda provider: provider) + monkeypatch.setattr(profiles, "get_active_profile_name", lambda: "default") + + config_data = { + "model": {"provider": "custom:test-gateway"}, + "custom_providers": [custom_provider], + } + monkeypatch.setattr(config, "get_config", lambda: config_data) + monkeypatch.setattr(routes, "cfg", config_data, raising=False) + + # provider_model_ids() returns [] for custom:* → drives the custom branch. + _install_provider_model_ids(monkeypatch, lambda provider: []) + requested = _install_live_catalog(monkeypatch, routes, catalog, fail=fail) + + parsed = urlparse("/api/models/live?provider=custom:test-gateway") + payload = routes._handle_live_models(object(), parsed) + return payload, requested + + +def _ids(payload): + return [m["id"] for m in payload.get("models", [])] + + +_LIVE_CATALOG = [ + "chat-a", + "chat-b", + "image-gen-1", # non-chat noise the picker must not surface + "audio-tts-1", +] + +_BASE_PROVIDER = { + "name": "Test Gateway", + "base_url": "https://gateway.example/v1", + "api_key": "sk-test", +} + + +@pytest.mark.parametrize( + "models_value", + [ + pytest.param(["chat-a", "chat-b"], id="native-list"), + pytest.param('["chat-a","chat-b"]', id="json-array-string"), + pytest.param("['chat-a', 'chat-b']", id="python-literal-string"), + pytest.param({"chat-a": {}, "chat-b": {}}, id="mapping-metadata"), + pytest.param( + [{"id": "chat-a"}, {"model": "chat-b"}], id="list-of-dicts" + ), + ], +) +def test_allowlist_filters_live_catalog(monkeypatch, models_value): + """An explicit plural allowlist must trim the live catalog to itself. + + The JSON-array-string and Python-literal cases are the regression: those + shapes are what ``hermes config set`` / JSON-mode editor saves persist, and + a dict/list-only parser drops them to ``[]`` → read as "no allowlist" → the + full upstream catalog floods the picker. + """ + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models=models_value) + payload, requested = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert requested, "live /v1/models probe was never attempted" + assert _ids(payload) == ["chat-a", "chat-b"] + assert "image-gen-1" not in _ids(payload) + assert "audio-tts-1" not in _ids(payload) + + +def test_singular_model_only_does_not_gate_live_catalog(monkeypatch): + """``model:`` is default metadata, not an allowlist — keep full discovery.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, model="chat-a") + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert _ids(payload) == _LIVE_CATALOG + + +def test_malformed_serialized_models_is_safe(monkeypatch): + """A broken JSON-array string must not crash or silently drop the gate. + + ``_parse_config_string_list()`` treats an undecodable value as one literal + name, so the allowlist stays non-empty and the filter still runs. + """ + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models='["chat-a", "chat-b"') # missing ] + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + # Treated as a single (unmatched) name → appended, catalog not flooded. + assert "image-gen-1" not in _ids(payload) + assert "audio-tts-1" not in _ids(payload) + + +def test_allowlisted_model_missing_from_live_is_appended(monkeypatch): + """A declared model the upstream didn't return must still be selectable.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models=["chat-a", "chat-offline"]) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert _ids(payload) == ["chat-a", "chat-offline"] + + +def test_probe_failure_falls_back_to_config_ids(monkeypatch): + """When the live probe fails, config ids (incl. singular model) are used.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, model="chat-default", models=["chat-a"]) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=True + ) + + assert _ids(payload) == ["chat-default", "chat-a"]