Files
hermes-webui/tests/test_issue3718_live_models_custom_probe.py
T
happy5318andClaude Opus 4.8 0fcd584274 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) <noreply@anthropic.com>
2026-09-23 17:48:34 +08:00

399 lines
15 KiB
Python

"""Handler-level regression tests for /api/models/live custom-provider probing.
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
* 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)
* discovered + ``discover_models: false`` → still pinned (explicit opt-out)
"""
import io
import json
import sys
import types
from urllib.parse import urlparse
import pytest
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)
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(
[{"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_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
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_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"]
@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"
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"]