From ea9cd1685066199d86a916f3c786903c99b48f0c Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Tue, 16 Jun 2026 21:45:43 +0000 Subject: [PATCH] fix(providers): gate credential_pool detection to known model providers (#4324) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the Photon phantom-providers regression from #4247 (v0.51.461): the credential-pool detection loop added every auth.json credential_pool key to detected_providers without checking it names a model provider, so the Photon iMessage plugin's photon/photon_project/photon_user messaging credentials each rendered as a phantom provider carrying the full auto-detected model catalog (251 models on the reporter's machine). Gated on a new _is_known_model_provider() check (custom:* slug, static _PROVIDER_DISPLAY/_PROVIDER_MODELS entry, or registered model-provider plugin) on both the load_pool and raw-dict fallback paths; the group-builder's generic fallback no longer paints an unrecognized id with the global catalog. Real pool-keyed providers (copilot, aliased google->gemini) still surface. Integrates the stronger of two independent same-fix PRs — #4330's implementation (thorough docstring, try/except-hardened plugin check, preserved #1881 comment, defensive else branch) + its 8-case test that mocks at the WebUI boundary (stubs hermes_cli, never imports the bundled agent package that's absent in CI, exercises both the load_pool and ImportError raw-dict paths) — over #4329's 2-case test. Both rodboev (#4329) and the maintainer agent (#4330) independently found and fixed the same regression. Reported by Nic. Co-authored-by: rodboev --- CHANGELOG.md | 8 + api/config.py | 54 ++- ...test_issue4324_photon_phantom_providers.py | 307 ++++++++++++++++++ 3 files changed, 366 insertions(+), 3 deletions(-) create mode 100644 tests/test_issue4324_photon_phantom_providers.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 06e64811..e6b1e5ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,14 @@ ## [Unreleased] +### Added + +- **Model picker provider groups are now collapsible and open scannable (#4279).** Provider groups in the model picker start collapsed except the group that owns your currently-selected model, so a picker with many providers/models opens as a tidy list of provider headings rather than one long flat scroll. Searching auto-expands matching groups (and searches hidden overflow models); clearing the search restores the collapsed-except-selected view. The "Show all N models" affordance is restyled as a distinct "+ Show all N models" expander (muted italic, indented, not mistakable for a selectable model) and now reveals the overflow in place — the group stays open and the view scrolls to the newly-revealed models instead of resetting to the top. The redundant per-row provider chip is suppressed for rows already under their own provider heading (kept only for hoisted/search rows). Builds on @akrhin's collapsible-groups work. + +### Fixed + +- **Non-model credential-pool keys no longer appear as phantom providers in the model picker (#4324).** A regression from #4247: the credential-pool detection loop added *every* `auth.json` → `credential_pool` key to the detected providers without checking it names a model provider. The Photon iMessage plugin writes three such keys (`photon`, `photon_project`, `photon_user`) that are messaging-platform credentials, not LLM API keys — so the picker showed three bogus "providers" (`photon`, `photon project`, `photon user`) each listing the full auto-detected model catalog. Pool detection is now gated on a new `_is_known_model_provider()` check (a `custom:*` slug, a static `_PROVIDER_DISPLAY`/`_PROVIDER_MODELS` entry, or a registered model-provider plugin) on both the `load_pool` and raw-dict fallback paths, and the group-builder's generic fallback no longer paints an unrecognized id with the global catalog. Real pool-keyed providers (`copilot`, aliased `google`→`gemini`) still surface. Reported by Nic; fix by @nesquena-hermes and independently by @rodboev (#4329/#4330). + ## [v0.51.463] — 2026-06-16 — Release PX (tighter remote workspace boundaries) ### Fixed diff --git a/api/config.py b/api/config.py index e4a0b719..03183f71 100644 --- a/api/config.py +++ b/api/config.py @@ -981,6 +981,41 @@ def _resolve_provider_alias(name: str) -> str: return _PROVIDER_ALIASES.get(raw, name) +def _is_known_model_provider(provider_id: str) -> bool: + """True when *provider_id* names a model provider WebUI can render. + + The credential pool (``auth.json`` → ``credential_pool``) stores keys for + BOTH model providers (whose API keys belong in the model picker) and + non-model platform plugins. The Photon iMessage plugin, for example, + writes ``photon`` / ``photon_project`` / ``photon_user`` pool entries that + are messaging-platform credentials, not LLM API keys. Only the former + should surface as provider groups. + + Without this gate, #4247's pool-detection loop added *every* pool key to + ``detected_providers``; unknown ids then fell through to the global + auto-detected catalog and each phantom provider was painted with the full + model list (#4324). ``provider_id`` is expected to be the canonical slug + (post ``_resolve_provider_alias``); the lookup is case-insensitive. + + A provider is "known" when it is a configured custom-provider slug + (``custom:*``), appears in WebUI's static ``_PROVIDER_DISPLAY`` / + ``_PROVIDER_MODELS`` tables, or is a registered model-provider plugin. + """ + pid = (provider_id or "").strip().lower() + if not pid: + return False + if pid.startswith("custom:"): + return True + if pid in _PROVIDER_DISPLAY or pid in _PROVIDER_MODELS: + return True + try: + if _is_plugin_model_provider(pid): + return True + except Exception: + logger.debug("plugin model-provider check failed for %s", pid) + return False + + def _custom_provider_slug_from_name(name: object) -> str: raw = str(name or "").strip().lower() if not raw: @@ -5029,7 +5064,7 @@ def get_available_models(*, prefer_cache: bool = False) -> dict: str(getattr(e, "key_source", "") or ""), ) ] - if _explicit: + if _explicit and _is_known_model_provider(_canonical_pid): detected_providers.add(_canonical_pid) except Exception: logger.debug("credential_pool.load_pool(%s) failed", _pid) @@ -5047,7 +5082,9 @@ def get_available_models(*, prefer_cache: bool = False) -> dict: for _entry in _entries ) if _has_explicit_cred: - detected_providers.add(_resolve_provider_alias(str(_pid))) + _canonical_pid = _resolve_provider_alias(str(_pid)) + if _is_known_model_provider(_canonical_pid): + detected_providers.add(_canonical_pid) except Exception: logger.debug("Failed to inspect credential_pool from auth store") @@ -6034,7 +6071,7 @@ def get_available_models(*, prefer_cache: bool = False) -> dict: detected_models = auto_detected_models_by_provider.get(pid) if detected_models: models_for_group = copy.deepcopy(detected_models) - elif auto_detected_models: + elif auto_detected_models and (pid == "custom" or _is_known_model_provider(pid)): # Don't fall back to the global auto_detected_models # list for the bare "custom" PID when the active # provider is something concrete (e.g. ai-gateway, @@ -6047,6 +6084,17 @@ def get_available_models(*, prefer_cache: bool = False) -> dict: else: models_for_group = copy.deepcopy(auto_detected_models) else: + # An unrecognized provider id with no catalog of its + # own must NOT be painted with the global + # auto_detected_models list. Otherwise a non-model + # credential_pool key (the Photon plugin's + # photon/photon_project/photon_user entries, #4324) or + # any future unknown id renders as a phantom provider + # carrying the active endpoint's entire model catalog. + # Such ids are dropped upstream by + # _is_known_model_provider() in the pool-detection + # loop; this omission is belt-and-braces matching the + # #1572/#7372 "omit rather than misattribute" posture. models_for_group = [] if models_for_group: # Per-group deep copy so subsequent mutation by diff --git a/tests/test_issue4324_photon_phantom_providers.py b/tests/test_issue4324_photon_phantom_providers.py new file mode 100644 index 00000000..3a73cb75 --- /dev/null +++ b/tests/test_issue4324_photon_phantom_providers.py @@ -0,0 +1,307 @@ +"""Regression tests for issue #4324. + +The Photon iMessage plugin writes three keys into ``auth.json`` → +``credential_pool``: ``photon``, ``photon_project`` and ``photon_user``. +These are messaging-platform credentials, NOT model providers. + +Regression from #4247: the credential-pool provider-detection loop in +``api/config.py::_build_available_models_uncached`` added *every* non-ambient +pool key to ``detected_providers`` without checking that the key actually +names a model provider. Unknown ids (``photon``, ``photon-project``, +``photon-user``) then fell through to the generic ``else`` branch of the +group builder, which copied the global ``auto_detected_models`` catalog (the +reporter's 251 base-url-probed models) into each phantom group — so the model +picker showed three bogus "providers" each listing the full catalog. + +The fix gates pool detection behind ``_is_known_model_provider()`` (both the +``load_pool`` and the raw-dict ``ImportError`` branches) and, defensively, +stops the ``else`` branch from painting an unknown id with the global catalog. + +These tests mock at the WebUI boundary (stub ``hermes_cli`` + a fake +``load_pool`` + a fake ``urlopen`` for the base-url probe) and never import +the bundled ``agent`` package, which is not installed in CI. +""" + +import json +import sys +import types + +import api.config as config +import api.profiles as profiles + + +def _install_fake_hermes_cli(monkeypatch, *, with_load_pool: bool = False, pool_data: dict | None = None): + """Stub hermes_cli so detection is deterministic and offline. + + list_available_providers() returns [] so the ONLY way a provider can be + detected is via the credential_pool path under test (mirrors the + test_credential_pool_providers.py helper). + """ + fake_pkg = types.ModuleType("hermes_cli") + fake_pkg.__path__ = [] + + fake_models = types.ModuleType("hermes_cli.models") + fake_models.list_available_providers = lambda: [] + fake_models.provider_model_ids = lambda pid: [] + + fake_auth = types.ModuleType("hermes_cli.auth") + fake_auth.get_auth_status = lambda _pid: {} + + monkeypatch.setitem(sys.modules, "hermes_cli", fake_pkg) + monkeypatch.setitem(sys.modules, "hermes_cli.models", fake_models) + monkeypatch.setitem(sys.modules, "hermes_cli.auth", fake_auth) + + # Remove the real agent.credential_pool. When with_load_pool is False this + # forces the ImportError raw-dict fallback path; when True we install a + # fake load_pool so the primary path is exercised. + monkeypatch.delitem(sys.modules, "agent.credential_pool", raising=False) + monkeypatch.delitem(sys.modules, "agent", raising=False) + + if with_load_pool: + _pool_data = pool_data or {} + + class _FakeEntry: + def __init__(self, d): + self.source = d.get("source", "manual") + self.label = d.get("label", "") + self.key_source = d.get("key_source", "") + self.id = d.get("id", "") + self.runtime_api_key = (d.get("runtime_api_key") or d.get("access_token") or "") + self.access_token = (d.get("access_token") or "") + self.base_url = d.get("base_url", "") + + class _FakePool: + def __init__(self, entries_list): + self._entries = entries_list + + def entries(self): + return self._entries + + def select(self): + return self._entries[0] if self._entries else None + + def _fake_load_pool(pid): + raw = _pool_data.get(pid, []) + return _FakePool([_FakeEntry(e) for e in raw]) + + fake_cp = types.ModuleType("agent.credential_pool") + fake_cp.load_pool = _fake_load_pool + monkeypatch.setitem(sys.modules, "agent.credential_pool", fake_cp) + + +def _install_fake_base_url_probe(monkeypatch, model_ids): + """Make the /v1/models base-url probe return *model_ids*. + + This is what populates the global ``auto_detected_models`` catalog — the + list the phantom-provider bug copied into every unknown group. + """ + payload = {"data": [{"id": mid, "name": mid} for mid in model_ids]} + + class _Resp: + def read(self): + return json.dumps(payload).encode("utf-8") + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + monkeypatch.setattr("urllib.request.urlopen", lambda req, timeout=10: _Resp()) + # Avoid the SSRF private-IP guard rejecting the probe host. + monkeypatch.setattr("socket.getaddrinfo", lambda *a, **k: []) + + +def _call_get_available_models(monkeypatch, tmp_path, auth_payload, *, + with_load_pool=False, base_url=None, probe_models=None): + _install_fake_hermes_cli( + monkeypatch, + with_load_pool=with_load_pool, + pool_data=auth_payload.get("credential_pool", {}), + ) + if base_url is not None: + _install_fake_base_url_probe(monkeypatch, probe_models or []) + + (tmp_path / "auth.json").write_text(json.dumps(auth_payload), encoding="utf-8") + monkeypatch.setattr(profiles, "get_active_hermes_home", lambda: tmp_path) + + for var in ("OPENAI_API_KEY", "HERMES_API_KEY", "HERMES_OPENAI_API_KEY", + "LOCAL_API_KEY", "OPENROUTER_API_KEY", "API_KEY"): + monkeypatch.delenv(var, raising=False) + + old_cfg = dict(config.cfg) + old_mtime = config._cfg_mtime + config.cfg.clear() + if base_url is not None: + config.cfg["model"] = {"base_url": base_url, "api_key": "sk-test"} + else: + config.cfg["model"] = {} + try: + config._cfg_mtime = config.Path(config._get_config_path()).stat().st_mtime + except Exception: + config._cfg_mtime = 0.0 + + config.invalidate_models_cache() + try: + return config.get_available_models() + finally: + config.cfg.clear() + config.cfg.update(old_cfg) + config._cfg_mtime = old_mtime + config.invalidate_models_cache() + + +def _group_names(result): + return [g["provider"] for g in result.get("groups", [])] + + +_PHOTON_POOL = { + "photon": [ + {"id": "ph1", "label": "photon-device", "source": "manual", "auth_type": "api_key"} + ], + "photon_project": [ + {"id": "ph2", "label": "photon-project", "source": "manual", "auth_type": "api_key"} + ], + "photon_user": [ + {"id": "ph3", "label": "photon-user", "source": "manual", "auth_type": "api_key"} + ], +} + + +def _photon_auth_payload(extra_pool=None): + pool = dict(_PHOTON_POOL) + if extra_pool: + pool.update(extra_pool) + return { + "version": 1, + "providers": {}, + "active_provider": "openai-codex", + "credential_pool": pool, + } + + +# ── load_pool path (primary) ───────────────────────────────────────────────── + + +def test_photon_pool_keys_do_not_appear_load_pool(monkeypatch, tmp_path): + """The three Photon pool keys must NOT render as provider groups.""" + result = _call_get_available_models( + monkeypatch, tmp_path, _photon_auth_payload(), with_load_pool=True, + ) + names = [n.lower() for n in _group_names(result)] + assert not any("photon" in n for n in names), ( + f"Photon messaging credentials must not appear as model providers; got {_group_names(result)}" + ) + + +def test_photon_pool_keys_hidden_even_with_base_url_catalog(monkeypatch, tmp_path): + """Reproduces the reporter's exact symptom: a base-url probe populates the + global catalog, and pre-fix each photon* group was painted with all of it. + + Post-fix: no photon group exists at all, regardless of the catalog size. + """ + catalog = [f"model-{i}" for i in range(40)] # stand-in for the 251 models + result = _call_get_available_models( + monkeypatch, tmp_path, _photon_auth_payload(), + with_load_pool=True, base_url="http://my-llm.local:11434/v1", probe_models=catalog, + ) + groups = {g["provider"]: g["models"] for g in result.get("groups", [])} + assert not any("photon" in n.lower() for n in groups), ( + f"No photon* group should exist; got {list(groups)}" + ) + # And none of the catalog models leaked into a phantom photon group: + for name, models in groups.items(): + if "photon" in name.lower(): + raise AssertionError(f"phantom photon group {name!r} carries {len(models)} models") + + +def test_real_provider_still_detected_alongside_photon(monkeypatch, tmp_path): + """The #4247 intent is preserved: a real model provider whose key lives in + the pool (copilot) is still detected even when photon* keys are present. + """ + extra = { + "copilot": [ + {"id": "cp1", "label": "explicit-copilot", "source": "manual", + "auth_type": "api_key", "base_url": "https://api.githubcopilot.com"} + ] + } + result = _call_get_available_models( + monkeypatch, tmp_path, _photon_auth_payload(extra), with_load_pool=True, + ) + names = _group_names(result) + assert "GitHub Copilot" in names, f"Real pool provider must still appear; got {names}" + assert not any("photon" in n.lower() for n in names), ( + f"Photon keys must still be filtered out; got {names}" + ) + + +# ── ImportError raw-dict fallback path ─────────────────────────────────────── + + +def test_photon_pool_keys_do_not_appear_importerror_path(monkeypatch, tmp_path): + """Same guard must hold on the raw-dict fallback (agent.credential_pool absent).""" + result = _call_get_available_models( + monkeypatch, tmp_path, _photon_auth_payload(), with_load_pool=False, + ) + names = [n.lower() for n in _group_names(result)] + assert not any("photon" in n for n in names), ( + f"Photon keys must be filtered on the ImportError path too; got {_group_names(result)}" + ) + + +def test_real_provider_still_detected_importerror_path(monkeypatch, tmp_path): + extra = { + "copilot": [ + {"id": "cp1", "label": "explicit-copilot", "source": "manual", + "auth_type": "api_key", "base_url": "https://api.githubcopilot.com"} + ] + } + result = _call_get_available_models( + monkeypatch, tmp_path, _photon_auth_payload(extra), with_load_pool=False, + ) + names = _group_names(result) + assert "GitHub Copilot" in names, f"Real pool provider must still appear; got {names}" + assert not any("photon" in n.lower() for n in names), ( + f"Photon keys must still be filtered out; got {names}" + ) + + +def test_aliased_pool_key_still_detected_alongside_photon(monkeypatch, tmp_path): + """#4247 intent (aliased pool keys) is preserved end-to-end. + + A pool key of ``google`` resolves to canonical ``gemini`` (in + _PROVIDER_MODELS, not _PROVIDER_DISPLAY) and must still surface as the + Gemini group even when non-model photon* keys are filtered out. Drives a + real pool key through detection rather than asserting on the helper alone. + """ + extra = { + "google": [ + {"id": "gp1", "label": "explicit-gemini", "source": "manual", + "auth_type": "api_key", "base_url": "https://generativelanguage.googleapis.com"} + ] + } + result = _call_get_available_models( + monkeypatch, tmp_path, _photon_auth_payload(extra), with_load_pool=True, + ) + names = _group_names(result) + assert "Gemini" in names, f"Aliased google→gemini pool key must surface as Gemini; got {names}" + assert not any("photon" in n.lower() for n in names), ( + f"Photon keys must still be filtered out; got {names}" + ) + + +# ── helper unit coverage ───────────────────────────────────────────────────── + + +def test_is_known_model_provider_rejects_photon(): + assert config._is_known_model_provider("photon") is False + assert config._is_known_model_provider("photon-project") is False + assert config._is_known_model_provider("photon-user") is False + assert config._is_known_model_provider("") is False + + +def test_is_known_model_provider_accepts_static_and_custom(): + assert config._is_known_model_provider("copilot") is True + assert config._is_known_model_provider("anthropic") is True + assert config._is_known_model_provider("gemini") is True + assert config._is_known_model_provider("custom:my-endpoint") is True