mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 16:18:55 +00:00
fix(model_switch): filter /model picker for unregistered providers (#57503)
list_authenticated_providers() emits picker rows for every slug in PROVIDER_TO_MODELS_DEV that has any credential env-var set. Several of those slugs (notably 'mistral') have no PROVIDER_REGISTRY entry, so resolve_provider() rejects them as 'Unknown provider' once the user selects a model — leaving the picker showing rows that cannot actually be selected. Add a resolve-gate in section 1: if PROVIDER_REGISTRY.get(hermes_id) is None, skip the slug. The picker now only lists providers that can actually be switched to at runtime. This automatically resolves the duplicate-Mistral dedup symptom too: once the broken-from-models.dev row is filtered, the conflict between PROVIDER_TO_MODELS_DEV['mistral'] and a custom_providers 'Mistral' row is moot. Composes with #50289 (which promotes mistral to first-class via the provider-plugin path): when that lands, PROVIDER_REGISTRY gains a 'mistral' entry and the gate becomes a no-op for it. No conflict. Tests (regression suite): - tests/hermes_cli/test_model_switch_filter_unresolved.py (new, 4 tests): Picker excludes 'mistral' when MISTRAL_API_KEY is set; 'deepseek' and 'xai' (PROVIDER_REGISTRY-backed) still appear; 'mistral' stays excluded when no key is set. Confirmed by reverting the fix and seeing the test fail with 'mistral leaked into /model picker'. Cross-checked against the existing 51 test_model_switch_* and test_custom_provider_* cases — 55/55 PASS, no regressions.
This commit is contained in:
parent
4a4a0c2fc7
commit
938c2622f6
2 changed files with 129 additions and 1 deletions
|
|
@ -1702,7 +1702,15 @@ def list_authenticated_providers(
|
|||
# section 2 (HERMES_OVERLAYS) with proper auth store checking.
|
||||
if pconfig and pconfig.auth_type != "api_key":
|
||||
continue
|
||||
if pconfig and pconfig.api_key_env_vars:
|
||||
# Skip providers that have no PROVIDER_REGISTRY entry. They appear
|
||||
# in PROVIDER_TO_MODELS_DEV (so the picker sees their models.dev
|
||||
# catalog and emits a row), but runtime resolution in
|
||||
# resolve_provider() rejects them as "Unknown provider". Filter
|
||||
# them here so the /model picker only ever lists providers that
|
||||
# can actually be switched to. Fixes #57503 (mistral).
|
||||
if not pconfig:
|
||||
continue
|
||||
if pconfig.api_key_env_vars:
|
||||
env_vars = list(pconfig.api_key_env_vars)
|
||||
else:
|
||||
env_vars = pdata.get("env", [])
|
||||
|
|
|
|||
120
tests/hermes_cli/test_model_switch_filter_unresolved.py
Normal file
120
tests/hermes_cli/test_model_switch_filter_unresolved.py
Normal file
|
|
@ -0,0 +1,120 @@
|
|||
"""Regression tests for picker filtering of unresolved providers.
|
||||
|
||||
Bug — ``list_authenticated_providers()`` would emit ``/model`` picker rows for
|
||||
providers that ``PROVIDER_TO_MODELS_DEV`` knows about but that the runtime
|
||||
``resolve_provider()`` rejects as ``Unknown provider``. The reporter set
|
||||
``MISTRAL_API_KEY`` and saw a ``Mistral (72)`` row in the Telegram picker;
|
||||
selecting any model under it produced
|
||||
``Could not resolve credentials for provider 'Mistral': Unknown provider 'mistral'``
|
||||
(#57503).
|
||||
|
||||
The fix adds a resolve-gate in section 1 of ``list_authenticated_providers``:
|
||||
if a slug has no ``PROVIDER_REGISTRY`` entry, skip it. The picker now only
|
||||
shows providers that can actually be selected at runtime.
|
||||
|
||||
These tests pin that behavior in two directions:
|
||||
|
||||
1. With ``MISTRAL_API_KEY`` set, ``mistral`` MUST NOT appear in the picker
|
||||
(it has no ``PROVIDER_REGISTRY`` entry at the time of this fix).
|
||||
2. A provider that IS in ``PROVIDER_REGISTRY`` (e.g. ``deepseek``) and has
|
||||
its API key set MUST still appear, so the fix does not regress existing
|
||||
picker coverage.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
from unittest.mock import patch
|
||||
|
||||
|
||||
def _reload():
|
||||
"""Force a fresh import of model_switch so module-level caches reset.
|
||||
|
||||
The picker caches ``PROVIDER_REGISTRY`` at import time. Each test must
|
||||
see a fresh module to avoid bleeding env vars between cases.
|
||||
"""
|
||||
for mod_name in [m for m in list(sys.modules) if m.startswith("hermes_cli.model_switch")]:
|
||||
sys.modules.pop(mod_name, None)
|
||||
return importlib.import_module("hermes_cli.model_switch")
|
||||
|
||||
|
||||
def _slug(rows):
|
||||
"""Return the set of provider slugs from picker rows."""
|
||||
return {row.get("slug") for row in rows}
|
||||
|
||||
|
||||
def test_mistral_filtered_when_unregistered_but_api_key_set(monkeypatch):
|
||||
"""#57503 — MISTRAL_API_KEY set, mistral has no PROVIDER_REGISTRY entry.
|
||||
|
||||
Picker must NOT emit a 'mistral' row. Users with a working
|
||||
custom_providers entry for Mistral no longer see the duplicate
|
||||
broken-from-models.dev entry cluttering the list.
|
||||
"""
|
||||
monkeypatch.setenv("MISTRAL_API_KEY", "test-mistral-key-shouldnt-crash")
|
||||
|
||||
model_switch = _reload()
|
||||
rows = model_switch.list_authenticated_providers(max_models=5)
|
||||
|
||||
slugs = _slug(rows)
|
||||
assert "mistral" not in slugs, (
|
||||
f"mistral leaked into /model picker despite being unregistered: {sorted(slugs)}"
|
||||
)
|
||||
|
||||
|
||||
def test_resolveable_provider_still_appears(monkeypatch):
|
||||
"""Sanity — a registered provider with a key must still surface.
|
||||
|
||||
Guards against the new resolve-gate being too broad (e.g. nuking the
|
||||
happy path of every other provider while filtering mistral).
|
||||
"""
|
||||
monkeypatch.setenv("DEEPSEEK_API_KEY", "test-deepseek-key-shouldnt-crash")
|
||||
|
||||
model_switch = _reload()
|
||||
rows = model_switch.list_authenticated_providers(max_models=5)
|
||||
|
||||
slugs = _slug(rows)
|
||||
assert "deepseek" in slugs, (
|
||||
"deepseek disappeared from /model picker — resolve-gate is too broad. "
|
||||
f"slugs seen: {sorted(slugs)}"
|
||||
)
|
||||
|
||||
|
||||
def test_resolve_gate_skips_models_dev_only_provider_without_creds(monkeypatch):
|
||||
"""A models-dev provider with no key and no PROVIDER_REGISTRY entry has
|
||||
no way to surface in the picker — verify we never emit such a row.
|
||||
|
||||
Keeps the fix idempotent: the previous ``if not has_creds: continue``
|
||||
gate already handled the no-credential case; we add a second gate on
|
||||
top that catches the WITH-credentials-but-no-registry case.
|
||||
"""
|
||||
# No MISTRAL_API_KEY, no other mistral-shaped env vars set
|
||||
for k in ("MISTRAL_API_KEY", "MISTRAL_BASE_URL"):
|
||||
monkeypatch.delenv(k, raising=False)
|
||||
|
||||
model_switch = _reload()
|
||||
rows = model_switch.list_authenticated_providers(max_models=5)
|
||||
|
||||
slugs = _slug(rows)
|
||||
assert "mistral" not in slugs
|
||||
|
||||
|
||||
def test_picker_skips_pconfig_none_does_not_break_other_section1_providers(monkeypatch, tmp_path):
|
||||
"""Coverage guard — providers that share a models.dev id with their
|
||||
canonical Hermes slug (e.g. ``gemini``, ``xai``, ``cohere``) must keep
|
||||
appearing. These ARE in ``PROVIDER_REGISTRY`` so the new gate should
|
||||
not touch them.
|
||||
"""
|
||||
# Pick a registry-backed provider that exists in models.dev and is
|
||||
# unlikely to be subject to the env-var duck-typing the picker uses
|
||||
# for unnamespaced keys.
|
||||
monkeypatch.setenv("XAI_API_KEY", "test-xai-key-shouldnt-crash")
|
||||
|
||||
model_switch = _reload()
|
||||
rows = model_switch.list_authenticated_providers(max_models=5)
|
||||
|
||||
slugs = _slug(rows)
|
||||
assert "xai" in slugs, (
|
||||
"xai disappeared — the resolve-gate ended up filtering registry-backed "
|
||||
"providers too. slugs seen: " + repr(sorted(slugs))
|
||||
)
|
||||
Loading…
Add table
Add a link
Reference in a new issue