diff --git a/src/llm/providers/claude.py b/src/llm/providers/claude.py index 20b45aedc..d62e353fa 100644 --- a/src/llm/providers/claude.py +++ b/src/llm/providers/claude.py @@ -26,8 +26,14 @@ def _uses_adaptive_thinking_only(model: str) -> bool: class ClaudeProvider(LLMProvider): provider_type = ProviderType.CLAUDE - def __init__(self, api_key: str | None = None, base_url: str | None = None) -> None: + def __init__( + self, + api_key: str | None = None, + base_url: str | None = None, + default_model: str | None = None, + ) -> None: self.client = Anthropic(api_key=api_key or os.environ.get("ANTHROPIC_API_KEY"), base_url=base_url) + self.default_model = default_model or _DEFAULT_MODEL self._models = [ ModelInfo( name="claude-opus-4-8", @@ -57,7 +63,7 @@ class ClaudeProvider(LLMProvider): def generate(self, input: LLMInput) -> LLMOutput: try: - model = input.model or _DEFAULT_MODEL + model = input.model or self.default_model system_parts = [msg.content for msg in input.messages if msg.role == Role.SYSTEM] api_messages = [ msg.to_dict() for msg in input.messages if msg.role not in (Role.SYSTEM,) @@ -139,4 +145,4 @@ class ClaudeProvider(LLMProvider): return bool(self.client.api_key) def get_default_model(self) -> str: - return _DEFAULT_MODEL + return self.default_model diff --git a/src/llm/providers/openai.py b/src/llm/providers/openai.py index 7d00d333c..443bdc0c1 100644 --- a/src/llm/providers/openai.py +++ b/src/llm/providers/openai.py @@ -28,12 +28,18 @@ from llm.providers.openai_compat import to_openai_message_param class OpenAIProvider(LLMProvider): provider_type = ProviderType.OPENAI - def __init__(self, api_key: str | None = None, base_url: str | None = None) -> None: + def __init__( + self, + api_key: str | None = None, + base_url: str | None = None, + default_model: str | None = None, + ) -> None: self.client = OpenAI( api_key=api_key or os.environ.get("OPENAI_API_KEY"), base_url=base_url, _enforce_credentials=False, ) + self.default_model = default_model or "gpt-4o-mini" self._models = [ ModelInfo( name="gpt-4o", @@ -72,7 +78,7 @@ class OpenAIProvider(LLMProvider): def generate(self, input: LLMInput) -> LLMOutput: try: params: dict[str, Any] = { - "model": input.model or "gpt-4o-mini", + "model": input.model or self.default_model, "messages": [to_openai_message_param(msg) for msg in input.messages], "temperature": input.temperature, } @@ -129,4 +135,4 @@ class OpenAIProvider(LLMProvider): return bool(self.client.api_key) def get_default_model(self) -> str: - return "gpt-4o-mini" + return self.default_model diff --git a/src/llm/providers/resolver.py b/src/llm/providers/resolver.py index 4156e4fb7..a53ae3792 100644 --- a/src/llm/providers/resolver.py +++ b/src/llm/providers/resolver.py @@ -22,6 +22,16 @@ _PROVIDER_MAP: dict[ProviderType, type[LLMProvider]] = { ProviderType.OLLAMA: OllamaProvider, } +# Only built-in adapters have a known default_model constructor contract. +_MODEL_ENV_VARS: dict[type[LLMProvider], tuple[str, ...]] = { + ClaudeProvider: (), + OpenAIProvider: (), + OllamaProvider: ("OLLAMA_MODEL",), + AtlasProvider: (AtlasProvider.model_env,), + AstraflowProvider: (AstraflowProvider.model_env,), + AstraflowCNProvider: (AstraflowCNProvider.model_env, AstraflowProvider.model_env), +} + LLM_ENV_FILE = ".llm.env" @@ -47,20 +57,48 @@ def _read_saved_llm_config(env_path: str | Path = LLM_ENV_FILE) -> dict[str, str return config -def _resolve_provider_type(provider_type: ProviderType | str | None) -> ProviderType | str: +def _resolve_provider_config( + provider_type: ProviderType | str | None, +) -> tuple[ProviderType | str, str | None]: if provider_type is not None: - return provider_type + return provider_type, None env_provider = os.environ.get("LLM_PROVIDER") if env_provider: - return _strip_env_value(env_provider).lower() + return _strip_env_value(env_provider).lower(), None saved_config = _read_saved_llm_config() - return saved_config.get("LLM_PROVIDER", "claude").lower() + saved_provider = saved_config.get("LLM_PROVIDER", "claude").lower() + # A saved model belongs to its saved provider, never to an override. + saved_model = saved_config.get("LLM_MODEL") if saved_config.get("LLM_PROVIDER") else None + return saved_provider, saved_model + + +def _resolve_default_model( + provider_type: ProviderType, provider_cls: type[LLMProvider], saved_model: str | None, +) -> str | None: + for name in _MODEL_ENV_VARS[provider_cls]: + model = os.environ.get(name) + if model: + return model + + env_provider = _strip_env_value(os.environ.get("LLM_PROVIDER", "")).lower() + if not env_provider or env_provider == provider_type.value: + env_model = _strip_env_value(os.environ.get("LLM_MODEL", "")) + if env_model: + return env_model + return saved_model or None def get_provider(provider_type: ProviderType | str | None = None, **kwargs: str) -> LLMProvider: - provider_type = _resolve_provider_type(provider_type) + """Create a provider, honoring the model selected by llm-select. + + Built-in model defaults prefer explicit kwargs, then existing provider-specific + environment variables, LLM_MODEL, and the saved provider/model pair. Saved + config is only read when neither an explicit nor an environment provider wins. + Custom providers retain their own constructor and configuration semantics. + """ + provider_type, saved_model = _resolve_provider_config(provider_type) if isinstance(provider_type, str): try: @@ -72,6 +110,11 @@ def get_provider(provider_type: ProviderType | str | None = None, **kwargs: str) if not provider_cls: raise ValueError(f"No provider registered for type: {provider_type}") + if "default_model" not in kwargs and provider_cls in _MODEL_ENV_VARS: + model = _resolve_default_model(provider_type, provider_cls, saved_model) + if model: + kwargs = {**kwargs, "default_model": model} + return provider_cls(**kwargs) diff --git a/tests/test_claude_provider.py b/tests/test_claude_provider.py index 7a4d54f95..37e151619 100644 --- a/tests/test_claude_provider.py +++ b/tests/test_claude_provider.py @@ -38,6 +38,41 @@ def make_response(content: list[SimpleNamespace], stop_reason: str = "tool_use") ) +@pytest.mark.unit +@pytest.mark.parametrize( + ("default_model", "request_model", "expected_model"), + [ + (None, None, "claude-sonnet-4-6"), + (None, "claude-haiku-4-5", "claude-haiku-4-5"), + ("claude-haiku-4-5", None, "claude-haiku-4-5"), + ("claude-opus-4-8", None, "claude-opus-4-8"), + ("claude-haiku-4-5", "claude-opus-4-8", "claude-opus-4-8"), + ], +) +def test_generate_respects_configured_default_and_request_model( + default_model: str | None, request_model: str | None, expected_model: str +) -> None: + provider_options = {"api_key": "test-key"} + if default_model is not None: + provider_options["default_model"] = default_model + provider = ClaudeProvider(**provider_options) + client = FakeClient(make_response([SimpleNamespace(type="text", text="ok")])) + provider.client = client + expected_default = default_model or "claude-sonnet-4-6" + + assert provider.get_default_model() == expected_default + + provider.generate( + LLMInput(messages=[Message(role=Role.USER, content="Hi")], model=request_model) + ) + + assert client.messages.last_params["model"] == expected_model + if expected_model == "claude-opus-4-8": + assert client.messages.last_params["thinking"] == {"type": "adaptive"} + assert "temperature" not in client.messages.last_params + assert provider.get_default_model() == expected_default + + @pytest.mark.unit def test_generate_collects_text_and_tool_use_blocks() -> None: provider = make_provider( diff --git a/tests/test_provider_tools.py b/tests/test_provider_tools.py index 7155953f7..2d913ab29 100644 --- a/tests/test_provider_tools.py +++ b/tests/test_provider_tools.py @@ -73,6 +73,38 @@ def _openai_response(**overrides) -> SimpleNamespace: return SimpleNamespace(**defaults) +@pytest.mark.unit +@pytest.mark.parametrize( + ("default_model", "request_model", "expected_model"), + [ + (None, None, "gpt-4o-mini"), + (None, "gpt-4o", "gpt-4o"), + ("gpt-4o", None, "gpt-4o"), + ("gpt-4o", "gpt-4-turbo", "gpt-4-turbo"), + ], +) +def test_openai_generate_respects_configured_default_and_request_model( + default_model: str | None, request_model: str | None, expected_model: str +) -> None: + provider_options = {"api_key": "test"} + if default_model is not None: + provider_options["default_model"] = default_model + provider = OpenAIProvider(**provider_options) + client = _OpenAIClient() + provider.client = client + expected_default = default_model or "gpt-4o-mini" + + assert provider.get_default_model() == expected_default + + provider.generate( + LLMInput(messages=[Message(role=Role.USER, content="hi")], model=request_model) + ) + + assert client.completions.params is not None + assert client.completions.params["model"] == expected_model + assert provider.get_default_model() == expected_default + + def test_openai_provider_serializes_tools_for_chat_completions(): provider = OpenAIProvider(api_key="test") client = _OpenAIClient() diff --git a/tests/test_resolver.py b/tests/test_resolver.py index c2a65787f..31fac930d 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -1,6 +1,11 @@ +import io +import json +from unittest.mock import Mock + import pytest -from llm.core.types import ProviderType +from llm.cli.selector import save_config +from llm.core.types import LLMInput, Message, ProviderType, Role from llm.providers import ( AstraflowCNProvider, AstraflowProvider, @@ -9,6 +14,7 @@ from llm.providers import ( OllamaProvider, OpenAIProvider, get_provider, + resolver, ) @@ -36,6 +42,7 @@ class TestGetProvider: def test_get_astraflow_cn_provider(self): provider = get_provider("astraflow_cn") assert isinstance(provider, AstraflowCNProvider) + assert provider.provider_type == ProviderType.ASTRAFLOW_CN def test_get_atlas_provider(self): @@ -100,3 +107,176 @@ class TestGetProvider: provider = get_provider() assert isinstance(provider, AstraflowCNProvider) + + +class TestSelectedModel: + @pytest.fixture(autouse=True) + def isolated_config(self, monkeypatch, tmp_path): + monkeypatch.chdir(tmp_path) + for name in ( + "LLM_PROVIDER", "LLM_MODEL", "OLLAMA_MODEL", "ATLAS_MODEL", + "ASTRAFLOW_MODEL", "ASTRAFLOW_CN_MODEL", + ): + # save_config(persist=True) writes os.environ directly; restore it after each test. + monkeypatch.setenv(name, "") + monkeypatch.delenv(name) + monkeypatch.setenv("ANTHROPIC_API_KEY", "test-key") + monkeypatch.setenv("OPENAI_API_KEY", "test-key") + + @pytest.mark.parametrize("persist", [False, True]) + def test_selector_model_reaches_actual_request(self, monkeypatch, persist): + save_config("ollama", "mistral", persist=persist) + urlopen = Mock(side_effect=lambda *args, **kwargs: io.BytesIO( + b'{"message":{"content":"ok"},"model":"mistral"}' + )) + monkeypatch.setattr("urllib.request.urlopen", urlopen) + + provider = get_provider() + provider.generate(LLMInput(messages=[Message(Role.USER, "hello")])) + + assert provider.get_default_model() == "mistral" + assert json.loads(urlopen.call_args.args[0].data)["model"] == "mistral" + + provider.generate(LLMInput(messages=[], model="codellama")) + assert json.loads(urlopen.call_args.args[0].data)["model"] == "codellama" + + @pytest.mark.parametrize("provider_type", list(ProviderType)) + @pytest.mark.parametrize("source", ["saved", "environment"]) + def test_selected_model_applies_to_every_builtin(self, monkeypatch, provider_type, source): + save_config(provider_type.value, "Selected/MixedCase-model", persist=source == "environment") + + provider = get_provider() + + assert provider.provider_type == provider_type + assert provider.get_default_model() == "Selected/MixedCase-model" + + def test_environment_model_overrides_saved_model(self, monkeypatch): + save_config("ollama", "mistral") + monkeypatch.setenv("LLM_MODEL", "codellama") + + assert get_provider().get_default_model() == "codellama" + + @pytest.mark.parametrize("provider_type,model_env", [ + ("ollama", "OLLAMA_MODEL"), + ("atlas", "ATLAS_MODEL"), + ("astraflow", "ASTRAFLOW_MODEL"), + ("astraflow_cn", "ASTRAFLOW_CN_MODEL"), + ("astraflow_cn", "ASTRAFLOW_MODEL"), + ]) + def test_provider_environment_retains_precedence(self, monkeypatch, provider_type, model_env): + save_config(provider_type, "saved-model") + monkeypatch.setenv("LLM_MODEL", "global-model") + monkeypatch.setenv(model_env, "provider-model") + + assert get_provider().get_default_model() == "provider-model" + + def test_cn_specific_environment_overrides_shared_fallback(self, monkeypatch): + save_config("astraflow_cn", "saved-model") + monkeypatch.setenv("ASTRAFLOW_MODEL", "shared-model") + monkeypatch.setenv("ASTRAFLOW_CN_MODEL", "cn-model") + + assert get_provider().get_default_model() == "cn-model" + + @pytest.mark.parametrize("provider_type", list(ProviderType)) + def test_explicit_default_model_overrides_selection(self, monkeypatch, provider_type): + save_config(provider_type.value, "saved-model", persist=True) + monkeypatch.setenv("OLLAMA_MODEL", "provider-model") + + assert get_provider(default_model="explicit-model").get_default_model() == "explicit-model" + + @pytest.mark.parametrize("default_model", [None, ""]) + def test_explicit_empty_default_retains_constructor_fallback(self, monkeypatch, default_model): + save_config("ollama", "saved-model", persist=True) + monkeypatch.setenv("OLLAMA_MODEL", "provider-model") + + assert get_provider(default_model=default_model).get_default_model() == "provider-model" + + @pytest.mark.parametrize("explicit", [False, True]) + def test_provider_override_does_not_inherit_saved_model(self, monkeypatch, explicit): + save_config("openai", "gpt-selected") + if explicit: + provider = get_provider("ollama") + else: + monkeypatch.setenv("LLM_PROVIDER", "ollama") + provider = get_provider() + + assert provider.get_default_model() == "llama3.2" + + def test_explicit_provider_does_not_inherit_another_environment_model(self, monkeypatch): + monkeypatch.setenv("LLM_PROVIDER", "openai") + monkeypatch.setenv("LLM_MODEL", "gpt-selected") + + assert get_provider("ollama").get_default_model() == "llama3.2" + + @pytest.mark.parametrize("env_provider", [None, " 'OLLAMA' "]) + def test_environment_model_applies_to_matching_explicit_provider(self, monkeypatch, env_provider): + if env_provider is not None: + monkeypatch.setenv("LLM_PROVIDER", env_provider) + monkeypatch.setenv("LLM_MODEL", ' "MixedCase/Model" ') + + assert get_provider("ollama").get_default_model() == "MixedCase/Model" + + @pytest.mark.parametrize("source", ["saved", "environment"]) + @pytest.mark.parametrize("model", ["", " ", '""']) + def test_empty_selected_model_retains_default(self, monkeypatch, source, model): + save_config("ollama", model, persist=source == "environment") + + assert get_provider().get_default_model() == "llama3.2" + + def test_empty_environment_model_falls_back_to_saved_model(self, monkeypatch): + save_config("ollama", "mistral") + monkeypatch.setenv("LLM_MODEL", " ") + + assert get_provider().get_default_model() == "mistral" + + def test_quoted_saved_model_preserves_case(self, tmp_path): + tmp_path.joinpath(".llm.env").write_text( + "LLM_PROVIDER='OLLAMA'\nLLM_MODEL='MixedCase/Model'\n" + ) + + assert get_provider().get_default_model() == "MixedCase/Model" + + def test_unpaired_saved_model_does_not_override_default_provider(self, tmp_path): + tmp_path.joinpath(".llm.env").write_text("LLM_MODEL=orphan-model\n") + + assert get_provider().get_default_model() == "claude-sonnet-4-6" + + @pytest.mark.parametrize("explicit", [False, True]) + def test_provider_override_does_not_read_invalid_saved_file(self, monkeypatch, tmp_path, explicit): + tmp_path.joinpath(".llm.env").write_bytes(b"\xff\xfe") + monkeypatch.setenv("LLM_MODEL", "mistral") + if explicit: + provider = get_provider("ollama") + else: + monkeypatch.setenv("LLM_PROVIDER", "ollama") + provider = get_provider() + + assert provider.get_default_model() == "mistral" + + def test_registered_provider_keeps_its_constructor_contract(self, monkeypatch): + class CustomProvider(OllamaProvider): + def __init__(self, base_url=None): + super().__init__(base_url=base_url, default_model="custom-default") + + monkeypatch.setattr(resolver, "_PROVIDER_MAP", resolver._PROVIDER_MAP.copy()) + resolver.register_provider(ProviderType.OLLAMA, CustomProvider) + save_config("ollama", "mistral", persist=True) + + provider = get_provider(base_url="http://localhost:1234") + + assert isinstance(provider, CustomProvider) + assert provider.base_url == "http://localhost:1234" + assert provider.get_default_model() == "custom-default" + + def test_registered_provider_receives_explicit_default_model(self, monkeypatch): + class CustomProvider(OllamaProvider): + pass + + monkeypatch.setattr(resolver, "_PROVIDER_MAP", resolver._PROVIDER_MAP.copy()) + resolver.register_provider(ProviderType.OLLAMA, CustomProvider) + save_config("ollama", "mistral", persist=True) + + provider = get_provider(default_model="custom-explicit") + + assert isinstance(provider, CustomProvider) + assert provider.get_default_model() == "custom-explicit"