From aa283d1e4f45731087653bbf7c057761c517cf28 Mon Sep 17 00:00:00 2001 From: zapabob Date: Sat, 30 May 2026 02:38:11 +0900 Subject: [PATCH] fix(model): isolate custom provider picker credentials --- hermes_cli/model_switch.py | 82 ++++++++++--------- .../test_model_switch_custom_providers.py | 38 +++++++++ .../test_runtime_provider_resolution.py | 48 +++++++++++ 3 files changed, 131 insertions(+), 37 deletions(-) diff --git a/hermes_cli/model_switch.py b/hermes_cli/model_switch.py index e1acc564a..5fb786127 100644 --- a/hermes_cli/model_switch.py +++ b/hermes_cli/model_switch.py @@ -1556,24 +1556,21 @@ def list_authenticated_providers( # --- 4. Saved custom providers from config --- # Each ``custom_providers`` entry represents one model under a named - # provider. Entries sharing the same endpoint (``base_url`` + ``api_key``) - # are grouped into a single picker row, so e.g. four Ollama entries - # pointing at ``http://localhost:11434/v1`` with per-model display names - # ("Ollama — GLM 5.1", "Ollama — Qwen3-coder", ...) appear as one + # provider. Entries sharing the same endpoint, credential identity, and + # wire protocol are grouped into a single picker row, so e.g. four Ollama + # entries pointing at ``http://localhost:11434/v1`` with per-model display + # names ("Ollama — GLM 5.1", "Ollama — Qwen3-coder", ...) appear as one # "Ollama" row with four models inside instead of four near-duplicates - # that differ only by suffix. Entries with distinct endpoints still - # produce separate rows. - # - # When the grouped endpoint matches ``current_base_url`` the group's - # slug becomes ``current_provider`` so that selecting a model from the - # picker flows back through the runtime provider that already holds - # valid credentials — no re-resolution needed. + # that differ only by suffix. Same-host entries with different ``key_env`` + # or ``api_mode`` remain distinct providers. if custom_providers and isinstance(custom_providers, list): from collections import OrderedDict - # Key by (base_url, api_key) instead of slug: names frequently - # differ per model ("Ollama — X") while the endpoint stays the - # same. Slug-based grouping left them as separate rows. + # Key by endpoint + credential identity + wire protocol instead of + # slug: names frequently differ per model ("Ollama — X") while the + # endpoint stays the same. Keep same-host providers with distinct + # env-backed credentials or API protocols separate so picker selection + # cannot route through the wrong credential/mode pair. groups: "OrderedDict[tuple, dict]" = OrderedDict() for entry in custom_providers: if not isinstance(entry, dict): @@ -1588,9 +1585,23 @@ def list_authenticated_providers( ).strip().rstrip("/") if not raw_name or not api_url: continue - api_key = (entry.get("api_key") or "").strip() + inline_api_key = (entry.get("api_key") or "").strip() + key_env = (entry.get("key_env") or "").strip() + api_key = inline_api_key or ( + os.environ.get(key_env, "").strip() if key_env else "" + ) + api_mode = str( + entry.get("api_mode") + or entry.get("transport") + or "" + ).strip().lower() + credential_identity = ( + inline_api_key + if inline_api_key + else (f"env:{key_env}" if key_env else "") + ) - group_key = (api_url, api_key) + group_key = (api_url, credential_identity, api_mode) if group_key not in groups: # Strip per-model suffix so "Ollama — GLM 5.1" becomes # "Ollama" for the grouped row. Em dash is the convention @@ -1603,29 +1614,16 @@ def list_authenticated_providers( break if not display_name: display_name = raw_name - # If this endpoint matches the currently active one, use - # ``current_provider`` as the slug so picker-driven switches - # route through the live credential pipeline. - if ( - current_base_url - and api_url == current_base_url.strip().rstrip("/") - ): - # Guard against bare "custom" slug left by a prior - # failed switch — always resolve to the canonical - # custom: form. (GH #17478) - slug = ( - current_provider - if current_provider and current_provider != "custom" - else custom_provider_slug(display_name) - ) - else: - slug = custom_provider_slug(display_name) + slug = custom_provider_slug(display_name) groups[group_key] = { "slug": slug, "name": display_name, "api_url": api_url, + "api_key": api_key, "models": [], } + elif api_key and not groups[group_key].get("api_key"): + groups[group_key]["api_key"] = api_key # The singular ``model:`` field only holds the currently # active model. Hermes's own writer (main.py::_save_custom_provider) @@ -1647,8 +1645,16 @@ def list_authenticated_providers( groups[group_key]["models"].append(m) _section4_emitted_slugs: set = set() - for grp_key, grp in groups.items(): - api_url, api_key = grp_key + _current_base_url_norm = str(current_base_url or "").strip().rstrip("/").lower() + _current_base_url_group_count = sum( + 1 + for _grp in groups.values() + if _current_base_url_norm + and str(_grp["api_url"]).strip().rstrip("/").lower() == _current_base_url_norm + ) + for grp in groups.values(): + api_url = grp["api_url"] + api_key = grp.get("api_key", "") slug = grp["slug"] # If the slug is already claimed by a built-in / overlay / # user-provider row (sections 1-3), skip this custom group @@ -1721,8 +1727,10 @@ def list_authenticated_providers( "slug": slug, "name": grp["name"], "is_current": slug == current_provider or ( - bool(current_base_url) - and _grp_url_norm == current_base_url.strip().rstrip("/").lower() + current_provider == "custom" + and bool(_current_base_url_norm) + and _grp_url_norm == _current_base_url_norm + and _current_base_url_group_count == 1 ), "is_user_defined": True, "models": grp["models"], diff --git a/tests/hermes_cli/test_model_switch_custom_providers.py b/tests/hermes_cli/test_model_switch_custom_providers.py index 4d88942b3..8ef865ee3 100644 --- a/tests/hermes_cli/test_model_switch_custom_providers.py +++ b/tests/hermes_cli/test_model_switch_custom_providers.py @@ -403,6 +403,44 @@ def test_list_authenticated_providers_same_url_different_keys_disambiguated(monk assert models["custom:openai-2"] == ["gpt-4.6"] +def test_list_authenticated_providers_same_url_different_key_env_and_api_mode_stay_separate(monkeypatch): + """Same gateway host but different key_env/api_mode entries are distinct providers.""" + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr(providers_mod, "HERMES_OVERLAYS", {}) + + providers = list_authenticated_providers( + current_provider="custom:gpt", + current_base_url="https://gateway.example.com", + user_providers={}, + custom_providers=[ + { + "name": "gpt", + "base_url": "https://gateway.example.com", + "key_env": "GPT_KEY", + "api_mode": "codex_responses", + "model": "gpt-5.5", + }, + { + "name": "claude", + "base_url": "https://gateway.example.com", + "key_env": "CLAUDE_KEY", + "api_mode": "anthropic_messages", + "model": "claude-opus-4-8", + }, + ], + max_models=50, + ) + + custom = [p for p in providers if p.get("is_user_defined")] + by_slug = {p["slug"]: p for p in custom} + + assert set(by_slug) == {"custom:gpt", "custom:claude"} + assert by_slug["custom:gpt"]["models"] == ["gpt-5.5"] + assert by_slug["custom:claude"]["models"] == ["claude-opus-4-8"] + assert by_slug["custom:gpt"]["is_current"] is True + assert by_slug["custom:claude"]["is_current"] is False + + def test_list_authenticated_providers_total_models_reflects_grouped_count(monkeypatch): """After grouping six entries into one row, total_models must reflect the full count, and every grouped model appears in the list.""" diff --git a/tests/hermes_cli/test_runtime_provider_resolution.py b/tests/hermes_cli/test_runtime_provider_resolution.py index 129c21f04..2f89be933 100644 --- a/tests/hermes_cli/test_runtime_provider_resolution.py +++ b/tests/hermes_cli/test_runtime_provider_resolution.py @@ -793,6 +793,54 @@ def test_named_custom_provider_uses_key_env_from_providers_dict(monkeypatch): assert resolved["model"] == "acme-large" +def test_named_custom_provider_same_url_uses_matching_key_env_and_api_mode(monkeypatch): + """Named custom providers on one gateway must keep their own credentials and protocol.""" + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.delenv("OPENROUTER_API_KEY", raising=False) + monkeypatch.setenv("GPT_KEY", "gpt-secret") + monkeypatch.setenv("CLAUDE_KEY", "claude-secret") + monkeypatch.setattr( + rp, + "load_config", + lambda: { + "custom_providers": [ + { + "name": "gpt", + "base_url": "https://gateway.example.com", + "key_env": "GPT_KEY", + "api_mode": "codex_responses", + "model": "gpt-5.5", + }, + { + "name": "claude", + "base_url": "https://gateway.example.com", + "key_env": "CLAUDE_KEY", + "api_mode": "anthropic_messages", + "model": "claude-opus-4-8", + }, + ], + }, + ) + monkeypatch.setattr( + rp, + "resolve_provider", + lambda *a, **k: (_ for _ in ()).throw( + AssertionError( + "resolve_provider should not be called for named custom providers" + ) + ), + ) + + resolved = rp.resolve_runtime_provider(requested="custom:claude") + + assert resolved["provider"] == "custom" + assert resolved["base_url"] == "https://gateway.example.com" + assert resolved["api_key"] == "claude-secret" + assert resolved["api_mode"] == "anthropic_messages" + assert resolved["requested_provider"] == "custom:claude" + assert resolved["model"] == "claude-opus-4-8" + + def test_named_custom_provider_falls_back_to_openai_api_key(monkeypatch): monkeypatch.setenv("OPENAI_API_KEY", "env-openai-key") monkeypatch.delenv("OPENROUTER_API_KEY", raising=False)