From 0fcd584274eb74a3a9fb5898cb22c240e713bfb1 Mon Sep 17 00:00:00 2001 From: happy5318 Date: Wed, 23 Sep 2026 17:48:34 +0800 Subject: [PATCH] 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