From 4a1fb47ce5ed1fea7e15fdf9cbcc682431310837 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Thu, 20 Aug 2026 12:39:25 +0800 Subject: [PATCH 1/7] fix(models): respect config model allowlist in live custom-provider catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Custom providers (esp. New-API-style gateways) expose their ENTIRE catalog via /v1/models, including image/audio models that aren't chat models. The live-catalog enrichment surfaced all of them in the model picker regardless of the `models` allowlist declared in config.yaml. Filter the live catalog down to the explicit plural `models` allowlist, then append any allowlisted models the upstream didn't return. Only the plural `models` key gates the filter — a provider configured with just a singular `model` (sticky/default metadata) keeps the unfiltered live catalog so discovery still works. Decode the plural value through `_parse_config_string_list()` so serialized shapes are honored: `hermes config set` and JSON-mode editor saves persist lists as quoted JSON-array strings ('["chat-a","chat-b"]') or Python literals ("['chat-a']"). A dict/list-only parser dropped those to [], which the caller reads as "no allowlist configured" → full catalog floods the picker (same class as the skills.disabled regression, #7120/#7134). Native dict/list values are still walked directly so dict entries keep their id|model|name metadata, which the decoder would str()-ify. Tests invoke the real `_handle_live_models` and assert on the emitted `models` payload, covering: native list, JSON-array string, Python-literal string, mapping metadata, list-of-dicts, singular-model-only (not gated), malformed serialized input (safe), allowlisted model missing from live (appended), and probe failure (config fallback). Verified non-vacuous: removing the production filter block fails 7 of 9 cases; reverting only the string decode fails exactly the 3 serialized cases with the documented flood symptom (image/audio models reappear). --- api/routes.py | 69 +++- ...test_issue3718_live_models_custom_probe.py | 371 +++++++++--------- 2 files changed, 249 insertions(+), 191 deletions(-) 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"] From 88236dbc26249ed2f6830bc92c43bf62fa414350 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Fri, 21 Aug 2026 00:15:44 +0800 Subject: [PATCH 2/7] docs(models): pin empty-allowlist semantics with regression coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile flagged that `models: []` / `models: "[]"` falls through the non-empty allowlist gate and leaves the full live catalog selectable. The symptom is real, but implementing the ask (gate on declared-ness instead) measurably makes things worse: it emits an EMPTY picker and leaves the provider unselectable. Measured both branches against the real handler before deciding: models key absent declared=False -> [chat-a, chat-b, image-gen-1, audio-tts-1] models: [] declared=True -> [] EMPTY PICKER models: "[]" declared=True -> [] EMPTY PICKER models: [chat-a] declared=True -> [chat-a] An unselectable provider is a harder failure than a few extra visible models, and it is unrecoverable from the UI: `custom_providers` has no WebUI write path, so it is hand-edited in config.yaml. There is also no deny-all use case — a provider the user wants hidden is removed from `custom_providers` outright, not left behind with an empty allowlist. No production behavior change. Records the trade-off in the filter block and adds `test_empty_allowlist_is_treated_as_not_configured` (native empty list, serialized "[]", empty mapping, blank scalar) so a future "fix" of this edge case fails loudly instead of silently emptying the picker. Verified non-vacuous: applying the declared-ness gate fails all 4 new cases with `assert [] == [...]`. --- api/routes.py | 14 ++++++- ...test_issue3718_live_models_custom_probe.py | 37 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/api/routes.py b/api/routes.py index 86a6769e..fc0c6f2f 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22016,12 +22016,24 @@ def _handle_live_models(handler, parsed): # 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 + # user never declared in config.yaml. Only a NON-EMPTY 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). + # + # An empty allowlist (``models: []``, ``models: "[]"``, or a + # value that decodes to no usable ids) is deliberately treated + # as "not configured", NOT as "allow nothing". Gating on + # declared-ness instead would emit an EMPTY picker and make the + # provider unselectable — a harder failure than surfacing a few + # extra models, and unrecoverable from the UI because + # ``custom_providers`` is hand-edited in config.yaml (the WebUI + # has no write path for it). ``[]`` in practice means a + # leftover/placeholder key, not an intentional deny-all, and no + # deny-all use case exists: a provider the user wants hidden is + # removed from ``custom_providers`` outright. if ids: if _allowlist_ids: _allowlist_set = set(_allowlist_ids) diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index d5cab25e..39e5682d 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -192,3 +192,40 @@ def test_probe_failure_falls_back_to_config_ids(monkeypatch): ) assert _ids(payload) == ["chat-default", "chat-a"] + + +@pytest.mark.parametrize( + "models_value", + [ + pytest.param([], id="native-empty-list"), + pytest.param("[]", id="serialized-empty-list"), + pytest.param({}, id="empty-mapping"), + pytest.param(" ", id="blank-scalar"), + ], +) +def test_empty_allowlist_is_treated_as_not_configured(monkeypatch, models_value): + """An empty ``models`` value must NOT be read as "allow nothing". + + Pins a deliberate semantic decision. Gating the filter on *declared-ness* + rather than on a non-empty allowlist would make ``models: []`` emit an + EMPTY picker, leaving the provider unselectable — a harder failure than + surfacing a few extra models, and unrecoverable from the UI because + ``custom_providers`` has no WebUI write path (hand-edited config.yaml only). + There is also no deny-all use case: a provider the user wants hidden is + removed from ``custom_providers`` outright. + + If someone later "fixes" the empty case into a deny-all gate, this test + fails and forces the trade-off to be re-argued rather than silently shipped. + """ + import api.routes as routes + + provider = dict(_BASE_PROVIDER, models=models_value) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert _ids(payload) == _LIVE_CATALOG, ( + "empty allowlist must fall through to full live discovery, " + "never collapse the picker to zero models" + ) + assert _ids(payload), "picker must never be emptied by an empty allowlist" From 4b689ecbce03b8e6e963d9485edcdcac729af8c4 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Sat, 22 Aug 2026 02:20:31 +0800 Subject: [PATCH 3/7] fix(models): enforce plural allowlist contract on failed live probes --- api/routes.py | 2 +- tests/test_byok_model_dropdown.py | 2 +- ...test_issue3718_live_models_custom_probe.py | 30 ++++++++++++++++--- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/api/routes.py b/api/routes.py index fc0c6f2f..19f1feb5 100644 --- a/api/routes.py +++ b/api/routes.py @@ -22042,7 +22042,7 @@ def _handle_live_models(handler, parsed): if _aid not in ids: ids.append(_aid) else: - ids = list(_config_ids) + ids = list(_allowlist_ids or _config_ids) # ── OpenAI-compat live fetch fallback ────────────────────────────────── # When provider_model_ids() is unavailable or returns [] for a provider diff --git a/tests/test_byok_model_dropdown.py b/tests/test_byok_model_dropdown.py index 6a8329aa..44fbe96d 100644 --- a/tests/test_byok_model_dropdown.py +++ b/tests/test_byok_model_dropdown.py @@ -308,7 +308,7 @@ class TestLiveModelsCustomProviderFallback: resp = self._call_live_models(monkeypatch, cfg, "custom:rightcode-codex") assert resp["provider"] == "custom:rightcode-codex" - assert [m["id"] for m in resp["models"]] == ["gpt-5.5", "gpt-5.5-mini"] + assert [m["id"] for m in resp["models"]] == ["gpt-5.5-mini"] def test_bare_custom_fallback_ignores_named_custom_provider_models(self, monkeypatch): """Bare custom only represents unnamed custom entries, not named siblings.""" diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 39e5682d..22043c82 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -182,16 +182,38 @@ def test_allowlisted_model_missing_from_live_is_appended(monkeypatch): 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.""" +def test_contradictory_singular_model_outside_plural_allowlist_is_ignored(monkeypatch): + """A singular default outside the plural allowlist must not leak into the picker. + + Paired test: whether the live probe succeeds or fails, only the explicit + plural allowlist is exposed. + """ import api.routes as routes provider = dict(_BASE_PROVIDER, model="chat-default", models=["chat-a"]) + + # 1. Successful live fetch -> only plural allowlist + payload_success, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=False + ) + assert _ids(payload_success) == ["chat-a"] + + # 2. Failed live fetch -> still only plural allowlist (not chat-default) + payload_fail, _ = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=True + ) + assert _ids(payload_fail) == ["chat-a"] + + +def test_probe_failure_without_allowlist_falls_back_to_singular_model(monkeypatch): + """When no plural allowlist is configured and probe fails, singular model is used.""" + import api.routes as routes + + provider = dict(_BASE_PROVIDER, model="chat-default") payload, _ = _run_live_models( monkeypatch, routes, provider, catalog=_LIVE_CATALOG, fail=True ) - - assert _ids(payload) == ["chat-default", "chat-a"] + assert _ids(payload) == ["chat-default"] @pytest.mark.parametrize( From 39017f60132bd8ae36aa1c8486067824b7aedba5 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Sat, 22 Aug 2026 03:56:37 +0800 Subject: [PATCH 4/7] chore: retrigger CI matrix (shard 0 flake, green locally on 3 bases) From 18842cc707c4473a560fb9340fd0aa847721bfd7 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 11:25:23 +0800 Subject: [PATCH 5/7] fix(models): discovered catalogs are discovery metadata, not allowlists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hermes persists discovery results as models: {...} + models_discovered: true. _custom_provider_allowlist_ids() treated any plural models value as an explicit allowlist, so after one discovery /api/models/live was permanently pinned to the first-discovery set and newly available upstream models were silently dropped (gate repro: new-live-c missing vs merge-base). Gate the helper on api.config._provider_models_are_discovered_catalog() — the same predicate the /api/models path uses — and return no allowlist when it holds. discover_models: false still pins via the predicate; a hand-written models: list without the discovered marker still filters; probe-failure fallback still serves saved ids. Real-handler tests: discovered catalog preserves full live catalog (RED on the previous head), discover_models: false control stays pinned, failed-probe control still returns saved models. --- api/routes.py | 17 ++++ ...test_issue3718_live_models_custom_probe.py | 91 +++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/api/routes.py b/api/routes.py index 19f1feb5..0f634796 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21891,6 +21891,19 @@ def _handle_live_models(handler, parsed): list) would be collapsed to a single model. Only an explicit ``models`` allowlist expresses "show exactly these models". + An auto-discovered catalog is NOT an allowlist: when Hermes + persisted discovery results back into config (``models: {...}`` + plus ``models_discovered: true``), that mapping is a snapshot + of what the gateway exposed at discovery time. Gating on it + would permanently pin the live catalog to the first-discovery + set, silently dropping any model the user pulls in later + (LM Studio / Ollama). ``_provider_models_are_discovered_catalog()`` + is the shared predicate the ``/api/models`` path already uses; + when it says "discovered", return no allowlist and let the + live probe win. An explicit ``discover_models: false`` opt-out + re-pins the catalog (the predicate accounts for it), so a + hand-pinned discovered mapping still filters. + 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 @@ -21900,6 +21913,10 @@ def _handle_live_models(handler, parsed): with the full upstream catalog — the same class of bug as the ``skills.disabled`` regression (#7120 / #7134). """ + from api.config import _provider_models_are_discovered_catalog + + if _provider_models_are_discovered_catalog(_cp): + return [] _ids = [] _models = _cp.get("models") # Serialized shapes only: ``hermes config set`` / JSON-mode diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 22043c82..8a4570be 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -14,6 +14,9 @@ Covered shapes: * malformed serialized ``models`` → safe, treated as scalar name * allowlisted model absent from live catalog → appended * live probe failure → falls back to config ids + * ``models_discovered: true`` catalog → live catalog NOT gated (discovery + metadata is not an allowlist) + * discovered + ``discover_models: false`` → still pinned (explicit opt-out) """ import io @@ -251,3 +254,91 @@ def test_empty_allowlist_is_treated_as_not_configured(monkeypatch, models_value) "never collapse the picker to zero models" ) assert _ids(payload), "picker must never be emptied by an empty allowlist" + + +def test_models_discovered_catalog_is_not_an_allowlist(monkeypatch): + """``models_discovered: true`` marks ``models:`` as discovery metadata (#7165). + + Hermes persists *discovery results* back into config as ``models: {...}`` + plus ``models_discovered: true``. That mapping is a snapshot of what the + gateway exposed at discovery time — NOT a hand-curated allowlist. Treating + it as one permanently pins the live catalog to the first-discovery set, so + a model the user later pulls into LM Studio / Ollama is silently dropped + from ``/api/models/live`` (the exact regression the gate reproduced: merge + base returned ``saved-a``, ``saved-b`` AND ``new-live-c``; the pinned head + dropped ``new-live-c``). + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={ + "saved-a": {"context_length": 128000}, + "saved-b": {"context_length": 32000}, + }, + models_discovered=True, + ) + payload, requested = _run_live_models( + monkeypatch, + routes, + provider, + catalog=["saved-a", "saved-b", "new-live-c"], + ) + + assert requested, "live /v1/models probe was never attempted" + assert _ids(payload) == ["saved-a", "saved-b", "new-live-c"], ( + "a discovered catalog must not gate the live probe — models that " + "appear upstream after discovery (new-live-c) must reach the picker" + ) + + +def test_discovered_catalog_with_discover_models_false_still_pins(monkeypatch): + """``discover_models: false`` re-pins a discovered catalog (control). + + The explicit opt-out wins over ``models_discovered: true`` — same rule as + the ``/api/models`` catalog path (`api.config._provider_discover_allowed`). + Filtering to the configured set must survive the discovered-catalog fix. + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={ + "saved-a": {"context_length": 128000}, + "saved-b": {"context_length": 32000}, + }, + models_discovered=True, + discover_models=False, + ) + payload, _ = _run_live_models( + monkeypatch, + routes, + provider, + catalog=["saved-a", "saved-b", "new-live-c"], + ) + + assert _ids(payload) == ["saved-a", "saved-b"] + + +def test_discovered_catalog_probe_failure_falls_back_to_saved_models(monkeypatch): + """Discovered + failed probe → saved models, not an empty picker (control). + + Releasing the allowlist must not empty the fallback: when the live probe + fails, the previously discovered ``models:`` mapping is still the best + answer and must be served from ``_config_ids``. + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={ + "saved-a": {"context_length": 128000}, + "saved-b": {"context_length": 32000}, + }, + models_discovered=True, + ) + payload, _ = _run_live_models( + monkeypatch, routes, provider, catalog=[], fail=True + ) + + assert _ids(payload) == ["saved-a", "saved-b"] From ff8e75c0bed6dd3683e6f43d7b7f985d210a7aae Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 11:25:23 +0800 Subject: [PATCH 6/7] docs(models): document the /api/models/live allowlist runtime contract Addresses the standing documentation finding: records the four evaluation signals (discovered catalog defers, plural allowlist filters, singular model never gates, no allowlist shows full catalog), serialized-list shapes, the empty-allowlist-is-not-configured rule, and the probe-failure fallback. Registered in docs/CONTRACTS.md per the #7556 precedent. --- docs/CONTRACTS.md | 9 ++ docs/architecture/live-models-allowlist.md | 96 ++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 docs/architecture/live-models-allowlist.md diff --git a/docs/CONTRACTS.md b/docs/CONTRACTS.md index 2989b587..0b81b9bb 100644 --- a/docs/CONTRACTS.md +++ b/docs/CONTRACTS.md @@ -89,6 +89,15 @@ contributor guidance; it does not change runtime behavior or CI gates. `models_cache.json` refresh timestamps), the stat-identity fallbacks, and the schema/version stamps. Start here before changing model-catalog caching, the `/api/models` cache keys, or the Codex catalog dependency (#2443, #7540). +- [`docs/architecture/live-models-allowlist.md`](architecture/live-models-allowlist.md): + current contract for how `/api/models/live` filters a custom provider's + upstream catalog: the four signals in evaluation order (discovered catalog + defers to the live probe, explicit plural `models:` allowlist filters, + singular `model:` never gates, no allowlist shows the full catalog), the + serialized-list shapes `hermes config set` persists, the deliberate + empty-allowlist-is-not-configured rule, and the probe-failure fallback. + Start here before changing custom-provider model filtering or + discovery-vs-allowlist semantics (#7165, #7404). - [`docs/architecture/profile-home-resolve-cache.md`](architecture/profile-home-resolve-cache.md): current contract for the call-scoped memoization around `_resolve_profile_home_param()` in `api/workspace.py`: what it caches, the diff --git a/docs/architecture/live-models-allowlist.md b/docs/architecture/live-models-allowlist.md new file mode 100644 index 00000000..6d05dd43 --- /dev/null +++ b/docs/architecture/live-models-allowlist.md @@ -0,0 +1,96 @@ +# Live model picker allowlist contract (`/api/models/live`) + +This document records the runtime contract for how `/api/models/live` decides +which models a `custom` / `custom:*` provider contributes to the chat model +picker. It describes shipped behavior and changes no runtime behavior. It was +added because the #7165 review flagged the contract as undocumented: the +filtering rules live in `_handle_live_models` (`api/routes.py`) and had only +code comments backing them. + +Start here before changing how the live catalog of a custom provider is +filtered, how discovery snapshots are distinguished from allowlists, or what +the endpoint returns when the upstream probe fails. + +## Why the endpoint filters at all + +OpenAI-compatible gateways (New-API-style aggregators, LM Studio, Ollama +proxies) expose their **entire** catalog from `/v1/models`, including +image-generation, audio, TTS, and embedding models that are not chat models. +The background enrichment path must not surface models the user never opted +into — but it also must not hide models the gateway gained since config was +last edited. The contract below is how those two goals are reconciled. + +## The four signals, in evaluation order + +For a custom provider the handler consults these signals: + +1. **Discovered catalog → no allowlist (live probe wins).** + When the provider entry carries `models_discovered: true` and discovery is + allowed (`discover_models` is not an explicit opt-out), the `models:` + mapping is a *snapshot Hermes persisted from a past discovery*, not a + hand-curated list. It must not gate the live probe: a model the user pulls + into LM Studio / Ollama after discovery has to appear in the picker. + Detected by the shared predicate + `api.config._provider_models_are_discovered_catalog()` — the same + predicate the `/api/models` catalog path uses, so both endpoints agree. + With this signal, no allowlist is derived and the full live catalog is + returned. + +2. **Explicit plural `models:` allowlist → filter.** + A non-empty plural `models:` value on an entry that is *not* a discovered + catalog expresses "show exactly these models". The live catalog is + intersected with it, then any allowlisted model missing from the live + response is appended (so an allowlisted-but-offline model stays + selectable). This is the #7165 filter. + +3. **Singular `model:` → never gates.** + The singular field is sticky/default metadata (the model to preselect), + not an allowlist. A provider configured only with `model: assistant` keeps + the full unfiltered live catalog; gating on it would collapse discovery to + a single model. + +4. **No allowlist → full live catalog.** + Preserves the pre-#7165 discovery behavior for setups that rely on it. + +### Serialized shapes + +`hermes config set` and the JSON-mode editor persist lists as quoted +JSON-array strings (`'["chat-a","chat-b"]'`) or Python literals +(`"['chat-a']"`). The plural value is decoded through the shared +`_parse_config_string_list()` before id extraction — but only when the value +is a `str`; native `dict` / `list` values are walked directly so dict *entries* +keep their `id|model|name` metadata (the decoder stringifies members). A +dict-only parser drops serialized allowlists to "no allowlist" and floods the +picker — the same bug class as `skills.disabled` (#7120 / #7134). + +### Empty allowlist semantics + +`models: []`, `models: "[]"`, an empty mapping, or a blank scalar are +deliberately treated as **not configured**, never as "allow nothing". The +filter runs only on a non-empty allowlist because gating on declared-ness +would emit an empty picker and make the provider unselectable — a harder +failure than showing a few extra models, and unrecoverable from the UI +(`custom_providers` has no WebUI write path; it is hand-edited in +config.yaml). There is no deny-all use case: a provider the user wants hidden +is removed from `custom_providers` outright. + +### Probe failure / empty probe + +When the upstream probe fails or returns no models, the endpoint falls back +to `allowlist or config ids` — the non-empty plural allowlist first, so a +singular `model:` outside the allowlist cannot leak in precisely when the +probe is unavailable; with no allowlist, the configured ids (singular + +plural) are served. A discovered catalog therefore still serves its saved +models on a transient probe failure instead of emptying the picker. + +## Tests + +- `tests/test_issue3718_live_models_custom_probe.py` — real-handler + (`_handle_live_models`) coverage of every signal above: plural list / + JSON-array-string / Python-literal / mapping / list-of-dicts filtering, + singular passthrough, malformed safety, discovered-catalog passthrough, + `discover_models: false` pinning, and both fallback branches. +- `tests/test_issue7404_models_discovered_not_allowlist.py` — the + `/api/models` side of the discovered-catalog contract. +- `tests/test_byok_model_dropdown.py` — picker rendering, profile-scoped + caching, and current-session model preservation around this endpoint. From 0fcd584274eb74a3a9fb5898cb22c240e713bfb1 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 17:48:34 +0800 Subject: [PATCH 7/7] fix(models): dict-shaped models is per-model metadata, not an allowlist (re-gate 9/23) Re-review #7165 (nesquena-hermes 2026-09-23T06:21:57Z, CHANGES_REQUESTED). CORE: a dict-shaped custom_providers[].models mapping (what Hermes Agent's setup flow writes, e.g. {chat-a: {context_length: 128000}}) was treated as an allowlist by iterating its keys, collapsing the live /v1/models catalog to a single saved default while the CLI live-probed the full list. The Agent defines exactly this contract in hermes_cli/model_switch.py::_models_config_is_allowlist: a mapping is per-model metadata, NOT a catalog narrow. Fix (all 3 spec points): 1. api/routes.py::_custom_provider_allowlist_ids: added _custom_provider_models_discover_is_false(_cp) (mirrors api.config._provider_discover_allowed / Agent model_switch_providers.py:557: bool False or string false/no/0, case-insensitive). In the dict branch, when discovery is NOT explicitly false, return no allowlist so the live probe returns the full catalog. Only discover_models: false opts into treating dict keys as a pinned allowlist. List / JSON-array-string / Python-literal shapes stay plain allowlists. 2. tests/test_issue3718_live_models_custom_probe.py: removed the mapping-metadata param that asserted the regression; added two real-handler tests: test_dict_models_without_discover_false_does_not_gate (dict -> full _LIVE_CATALOG) and test_dict_models_with_discover_false_pins_to_keys (bool False / "false" / "no" / "0" -> pinned to keys). 3. docs/architecture/live-models-allowlist.md: documented that list and string shapes filter, and a dict filters only with discover_models: false. Companion test update (blocker reported by the fix itself): tests/test_byok_model_dropdown.py rightcode-codex entry encoded the removed dict-as-allowlist behavior (models: {"gpt-5.5-mini": {}} asserting the response collapsed to just that key). Added discover_models: False to preserve the provider-scoping intent (no sibling leak) under the new contract. Red/green verified: removing the dict-branch guard turns test_dict_models_without_discover_false_does_not_gate red (collapsed catalog); restoring it turns it green. Verification: 3718 18 passed; byok 24 passed; neighbour sweep (46 files) 520 passed (2 pre-existing order-dependent flakes reproduce identically on clean HEAD, verified via git stash); ruff curated gate OK. Co-Authored-By: Claude Opus 4.8 (1M context) --- api/routes.py | 35 ++++++++++- docs/architecture/live-models-allowlist.md | 31 +++++++++- tests/test_byok_model_dropdown.py | 7 +++ ...test_issue3718_live_models_custom_probe.py | 58 ++++++++++++++++++- 4 files changed, 126 insertions(+), 5 deletions(-) diff --git a/api/routes.py b/api/routes.py index 0f634796..391d3583 100644 --- a/api/routes.py +++ b/api/routes.py @@ -21882,6 +21882,21 @@ 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_models_discover_is_false(_cp): + """True when ``discover_models`` is an explicit ``false`` opt-out. + + Mirrors ``api.config._provider_discover_allowed`` / Hermes + Agent ``model_switch_providers._discover_flag``: ``discover_models`` + defaults to True, and the string forms ``"false"``/``"no"``/``"0"`` + (case-insensitive) mean False. Used to decide whether a + dict-shaped ``models`` mapping is a hand-pinned allowlist (only + when discovery is off) rather than per-model metadata. + """ + _discover = _cp.get("discover_models", True) if isinstance(_cp, dict) else True + if isinstance(_discover, str): + return _discover.strip().lower() in {"false", "no", "0"} + return not bool(_discover) + def _custom_provider_allowlist_ids(_cp): """Plural ``models`` list only — the explicit allowlist signal. @@ -21902,7 +21917,18 @@ def _handle_live_models(handler, parsed): when it says "discovered", return no allowlist and let the live probe win. An explicit ``discover_models: false`` opt-out re-pins the catalog (the predicate accounts for it), so a - hand-pinned discovered mapping still filters. + hand-pinned discovered catalog still filters. + + A dict-shaped ``models`` mapping that is NOT marked discovered is + *per-model metadata* written by the Hermes Agent setup flow + (``hermes_cli/model_switch.py::_save_custom_provider`` and the + setup wizard) — e.g. ``{chat-a: {context_length: 128000}}``. It is + not a catalog narrow: treating its keys as an allowlist would + collapse the live picker to the single saved default (keyless + Ollama) while the CLI live-probe shows the full catalog. Only an + explicit ``discover_models: false`` opts into treating the dict + keys as a pinned allowlist. List/JSON-array-string/Python-literal + shapes remain plain allowlists. The plural value is decoded through ``_parse_config_string_list()`` because ``hermes config set`` / JSON-mode editor saves persist @@ -21929,6 +21955,13 @@ def _handle_live_models(handler, parsed): if isinstance(_models, str): _models = _parse_config_string_list(_models) if isinstance(_models, dict): + # A dict-shaped ``models`` is per-model metadata written by + # the Hermes Agent setup flow, NOT a catalog narrow. Only a + # ``discover_models: false`` opt-out treats the dict keys as a + # pinned allowlist; otherwise return the empty allowlist so the + # live probe returns the full catalog. + if not _custom_provider_models_discover_is_false(_cp): + return [] for _mid in _models: if isinstance(_mid, str) and _mid.strip(): _ids.append(_mid.strip()) diff --git a/docs/architecture/live-models-allowlist.md b/docs/architecture/live-models-allowlist.md index 6d05dd43..83c7363a 100644 --- a/docs/architecture/live-models-allowlist.md +++ b/docs/architecture/live-models-allowlist.md @@ -41,7 +41,9 @@ For a custom provider the handler consults these signals: catalog expresses "show exactly these models". The live catalog is intersected with it, then any allowlisted model missing from the live response is appended (so an allowlisted-but-offline model stays - selectable). This is the #7165 filter. + selectable). This is the #7165 filter. Only **list** and **string** + (serialized) shapes are plain allowlists — see "Dict-shaped `models:`" + below. 3. **Singular `model:` → never gates.** The singular field is sticky/default metadata (the model to preselect), @@ -63,6 +65,30 @@ keep their `id|model|name` metadata (the decoder stringifies members). A dict-only parser drops serialized allowlists to "no allowlist" and floods the picker — the same bug class as `skills.disabled` (#7120 / #7134). +### Dict-shaped `models:` + +A dict-shaped `models:` mapping (e.g. `{chat-a: {context_length: 128000}}`) +is *per-model metadata* written by the Hermes Agent setup flow — +`hermes_cli/model_switch.py::_save_custom_provider` and the setup wizard — +**not** a catalog narrow. Treating its keys as an allowlist would collapse the +live picker to the single saved default (keyless Ollama) while the CLI +live-probe shows the full catalog. So a dict is **never** a plain allowlist: + +- without an explicit `discover_models: false`, a dict-shaped `models:` + contributes **no allowlist** and the full live catalog is returned; +- with `discover_models: false` (bool `False` or the string forms + `"false"`/`"no"`/`"0"`, case-insensitive — mirroring + `model_switch_providers.py:557`), the dict **keys** are honored as a pinned + allowlist. + +This complements the discovered-catalog rule: `models_discovered: true` +without the opt-out is already ignored (signal 1). The dict guard covers the +Agent-setup shape that is *not* flagged discovered. + +If someone later "fixes" the dict shape back into an always-allowlist, the +regression test `test_dict_models_without_discover_false_does_not_gate` fails +and forces the trade-off to be re-argued. + ### Empty allowlist semantics `models: []`, `models: "[]"`, an empty mapping, or a blank scalar are @@ -87,7 +113,8 @@ models on a transient probe failure instead of emptying the picker. - `tests/test_issue3718_live_models_custom_probe.py` — real-handler (`_handle_live_models`) coverage of every signal above: plural list / - JSON-array-string / Python-literal / mapping / list-of-dicts filtering, + JSON-array-string / Python-literal / list-of-dicts filtering, dict-shaped + `models:` (not gated) and dict + `discover_models: false` (pinned), singular passthrough, malformed safety, discovered-catalog passthrough, `discover_models: false` pinning, and both fallback branches. - `tests/test_issue7404_models_discovered_not_allowlist.py` — the diff --git a/tests/test_byok_model_dropdown.py b/tests/test_byok_model_dropdown.py index 44fbe96d..976dfdd6 100644 --- a/tests/test_byok_model_dropdown.py +++ b/tests/test_byok_model_dropdown.py @@ -291,6 +291,13 @@ class TestLiveModelsCustomProviderFallback: "model": "gpt-5.5", "models": {"gpt-5.5-mini": {}}, "base_url": "https://right.codes/codex/v1", + # dict-shaped ``models`` is per-model metadata (Agent setup + # flow) and only narrows the live catalog when discovery is + # explicitly off — the new #7165 contract. This test still + # wants provider-scoping (no sibling leak), so pin the + # catalog via ``discover_models: false`` instead of relying + # on the removed dict-as-allowlist behavior. + "discover_models": False, }, { "name": "infini-ai", diff --git a/tests/test_issue3718_live_models_custom_probe.py b/tests/test_issue3718_live_models_custom_probe.py index 8a4570be..ca6b31c1 100644 --- a/tests/test_issue3718_live_models_custom_probe.py +++ b/tests/test_issue3718_live_models_custom_probe.py @@ -14,8 +14,12 @@ Covered shapes: * malformed serialized ``models`` → safe, treated as scalar name * allowlisted model absent from live catalog → appended * live probe failure → falls back to config ids + * dict-shaped ``models`` (Agent setup) → live catalog NOT gated (per-model + metadata, not an allowlist) + * dict-shaped ``models`` + ``discover_models: + false`` → pinned to dict keys (explicit opt-out) * ``models_discovered: true`` catalog → live catalog NOT gated (discovery - metadata is not an allowlist) + metadata is not an allowlist) * discovered + ``discover_models: false`` → still pinned (explicit opt-out) """ @@ -116,7 +120,6 @@ _BASE_PROVIDER = { 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" ), @@ -143,6 +146,57 @@ def test_allowlist_filters_live_catalog(monkeypatch, models_value): assert "audio-tts-1" not in _ids(payload) +def test_dict_models_without_discover_false_does_not_gate(monkeypatch): + """A dict-shaped ``models`` is per-model metadata — NOT an allowlist (#7165). + + Hermes Agent's setup flow and ``hermes_cli/model_switch.py::_save_custom_provider`` + persist per-model metadata as a mapping, e.g. ``{chat-a: {context_length: 128000}}``. + Treating its keys as an allowlist would collapse the live picker to the saved + default while the CLI live-probe shows the full catalog. Without an explicit + ``discover_models: false`` opt-out the dict must NOT gate — full live catalog wins. + """ + import api.routes as routes + + provider = dict( + _BASE_PROVIDER, + models={"chat-a": {"context_length": 128000}, "chat-b": {"context_length": 32000}}, + ) + payload, requested = _run_live_models( + monkeypatch, routes, provider, catalog=_LIVE_CATALOG + ) + + assert requested, "live /v1/models probe was never attempted" + assert _ids(payload) == _LIVE_CATALOG, ( + "dict-shaped models is per-model metadata, not an allowlist — the " + "full live catalog must be surfaced" + ) + + +def test_dict_models_with_discover_false_pins_to_keys(monkeypatch): + """``discover_models: false`` opts a dict-shaped ``models`` into an allowlist. + + The explicit opt-out (bool ``False`` or the string forms ``"false"``/``"no"``/``"0"``) + means the dict keys are a hand-pinned allowlist and must filter the live catalog. + """ + import api.routes as routes + + for discover in (False, "false", "no", "0"): + provider = dict( + _BASE_PROVIDER, + models={"chat-a": {"context_length": 128000}, "chat-b": {"context_length": 32000}}, + discover_models=discover, + ) + 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"], ( + f"discover_models={discover!r} must pin the dict keys as the allowlist" + ) + 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