fix: provider selection not persisting when switching via hermes model
Two related bugs prevented users from reliably switching providers: 1. OPENAI_BASE_URL poisoning OpenRouter resolution: When a user with a custom endpoint ran /model openrouter:model, _resolve_openrouter_runtime picked up OPENAI_BASE_URL instead of the OpenRouter URL, causing model validation to probe the wrong API and reject valid models. Fix: skip OPENAI_BASE_URL when requested_provider is explicitly 'openrouter'. 2. Provider never saved to config: _save_model_choice() could save config.model as a plain string. All five _model_flow_* functions then checked isinstance(model, dict) before writing the provider — which silently failed on strings. With no provider in config, auto-detection would pick up stale credentials (e.g. Codex desktop app) instead of the user's explicit choice. Fix: _save_model_choice() now always saves as dict format. All flow functions also normalize string->dict as a safety net before writing provider. Adds 4 regression tests. 2873 tests pass.
This commit is contained in:
99
tests/test_model_provider_persistence.py
Normal file
99
tests/test_model_provider_persistence.py
Normal file
@@ -0,0 +1,99 @@
|
||||
"""Tests that provider selection via `hermes model` always persists correctly.
|
||||
|
||||
Regression tests for the bug where _save_model_choice could save config.model
|
||||
as a plain string, causing subsequent provider writes (which check
|
||||
isinstance(model, dict)) to silently fail — leaving the provider unset and
|
||||
falling back to auto-detection.
|
||||
"""
|
||||
|
||||
import os
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def config_home(tmp_path, monkeypatch):
|
||||
"""Isolated HERMES_HOME with a minimal string-format config."""
|
||||
home = tmp_path / "hermes"
|
||||
home.mkdir()
|
||||
config_yaml = home / "config.yaml"
|
||||
# Start with model as a plain string — the format that triggered the bug
|
||||
config_yaml.write_text("model: some-old-model\n")
|
||||
env_file = home / ".env"
|
||||
env_file.write_text("")
|
||||
monkeypatch.setenv("HERMES_HOME", str(home))
|
||||
# Clear env vars that could interfere
|
||||
monkeypatch.delenv("HERMES_MODEL", raising=False)
|
||||
monkeypatch.delenv("LLM_MODEL", raising=False)
|
||||
monkeypatch.delenv("HERMES_INFERENCE_PROVIDER", raising=False)
|
||||
monkeypatch.delenv("OPENAI_BASE_URL", raising=False)
|
||||
monkeypatch.delenv("OPENAI_API_KEY", raising=False)
|
||||
monkeypatch.delenv("OPENROUTER_API_KEY", raising=False)
|
||||
return home
|
||||
|
||||
|
||||
class TestSaveModelChoiceAlwaysDict:
|
||||
def test_string_model_becomes_dict(self, config_home):
|
||||
"""When config.model is a plain string, _save_model_choice must
|
||||
convert it to a dict so provider can be set afterwards."""
|
||||
from hermes_cli.auth import _save_model_choice
|
||||
|
||||
_save_model_choice("kimi-k2.5")
|
||||
|
||||
import yaml
|
||||
config = yaml.safe_load((config_home / "config.yaml").read_text()) or {}
|
||||
model = config.get("model")
|
||||
assert isinstance(model, dict), (
|
||||
f"Expected model to be a dict after save, got {type(model)}: {model}"
|
||||
)
|
||||
assert model["default"] == "kimi-k2.5"
|
||||
|
||||
def test_dict_model_stays_dict(self, config_home):
|
||||
"""When config.model is already a dict, _save_model_choice preserves it."""
|
||||
import yaml
|
||||
(config_home / "config.yaml").write_text(
|
||||
"model:\n default: old-model\n provider: openrouter\n"
|
||||
)
|
||||
from hermes_cli.auth import _save_model_choice
|
||||
|
||||
_save_model_choice("new-model")
|
||||
|
||||
config = yaml.safe_load((config_home / "config.yaml").read_text()) or {}
|
||||
model = config.get("model")
|
||||
assert isinstance(model, dict)
|
||||
assert model["default"] == "new-model"
|
||||
assert model["provider"] == "openrouter" # preserved
|
||||
|
||||
|
||||
class TestProviderPersistsAfterModelSave:
|
||||
def test_api_key_provider_saved_when_model_was_string(self, config_home, monkeypatch):
|
||||
"""_model_flow_api_key_provider must persist the provider even when
|
||||
config.model started as a plain string."""
|
||||
from hermes_cli.auth import PROVIDER_REGISTRY
|
||||
|
||||
pconfig = PROVIDER_REGISTRY.get("kimi-coding")
|
||||
if not pconfig:
|
||||
pytest.skip("kimi-coding not in PROVIDER_REGISTRY")
|
||||
|
||||
# Simulate: user has a Kimi API key, model was a string
|
||||
monkeypatch.setenv("KIMI_API_KEY", "sk-kimi-test-key")
|
||||
|
||||
from hermes_cli.main import _model_flow_api_key_provider
|
||||
from hermes_cli.config import load_config
|
||||
|
||||
# Mock the model selection prompt to return "kimi-k2.5"
|
||||
# Also mock input() for the base URL prompt and builtins.input
|
||||
with patch("hermes_cli.auth._prompt_model_selection", return_value="kimi-k2.5"), \
|
||||
patch("hermes_cli.auth.deactivate_provider"), \
|
||||
patch("builtins.input", return_value=""):
|
||||
_model_flow_api_key_provider(load_config(), "kimi-coding", "old-model")
|
||||
|
||||
import yaml
|
||||
config = yaml.safe_load((config_home / "config.yaml").read_text()) or {}
|
||||
model = config.get("model")
|
||||
assert isinstance(model, dict), f"model should be dict, got {type(model)}"
|
||||
assert model.get("provider") == "kimi-coding", (
|
||||
f"provider should be 'kimi-coding', got {model.get('provider')}"
|
||||
)
|
||||
assert model.get("default") == "kimi-k2.5"
|
||||
@@ -181,6 +181,25 @@ def test_resolve_runtime_provider_nous_api(monkeypatch):
|
||||
assert resolved["requested_provider"] == "nous-api"
|
||||
|
||||
|
||||
def test_explicit_openrouter_skips_openai_base_url(monkeypatch):
|
||||
"""When the user explicitly requests openrouter, OPENAI_BASE_URL
|
||||
(which may point to a custom endpoint) must not override the
|
||||
OpenRouter base URL. Regression test for #874."""
|
||||
monkeypatch.setattr(rp, "resolve_provider", lambda *a, **k: "openrouter")
|
||||
monkeypatch.setattr(rp, "_get_model_config", lambda: {})
|
||||
monkeypatch.setenv("OPENAI_BASE_URL", "https://my-custom-llm.example.com/v1")
|
||||
monkeypatch.setenv("OPENROUTER_API_KEY", "or-test-key")
|
||||
monkeypatch.delenv("OPENROUTER_BASE_URL", raising=False)
|
||||
monkeypatch.delenv("OPENAI_API_KEY", raising=False)
|
||||
|
||||
resolved = rp.resolve_runtime_provider(requested="openrouter")
|
||||
|
||||
assert resolved["provider"] == "openrouter"
|
||||
assert "openrouter.ai" in resolved["base_url"]
|
||||
assert "my-custom-llm" not in resolved["base_url"]
|
||||
assert resolved["api_key"] == "or-test-key"
|
||||
|
||||
|
||||
def test_resolve_requested_provider_precedence(monkeypatch):
|
||||
monkeypatch.setenv("HERMES_INFERENCE_PROVIDER", "nous")
|
||||
monkeypatch.setattr(rp, "_get_model_config", lambda: {"provider": "openai-codex"})
|
||||
|
||||
Reference in New Issue
Block a user