From effa074f7dc24cedf93e8cf8b9e4ebe0b9f58d6e Mon Sep 17 00:00:00 2001 From: Yann Roberto <1922498827@qq.com> Date: Sun, 11 Oct 2026 14:31:45 +0800 Subject: [PATCH] fix(llm): normalize model environment values --- src/llm/providers/astraflow.py | 5 +- src/llm/providers/atlas.py | 3 +- src/llm/providers/model_config.py | 11 +++ src/llm/providers/ollama.py | 3 +- src/llm/providers/resolver.py | 7 +- tests/test_model_config.py | 119 ++++++++++++++++++++++++++++++ tests/test_resolver.py | 57 +++++++++----- 7 files changed, 179 insertions(+), 26 deletions(-) create mode 100644 src/llm/providers/model_config.py create mode 100644 tests/test_model_config.py diff --git a/src/llm/providers/astraflow.py b/src/llm/providers/astraflow.py index 2bcc6d6b2..270c4951b 100644 --- a/src/llm/providers/astraflow.py +++ b/src/llm/providers/astraflow.py @@ -16,6 +16,7 @@ from llm.core.interface import ( ) from llm.core.types import LLMInput, LLMOutput, ModelInfo, ProviderType, ToolCall from llm.providers.constants import EMPTY_FILTERED_RESPONSE_ERROR +from llm.providers.model_config import read_model_env from llm.providers.openai_compat import to_openai_message_param ASTRAFLOW_BASE_URL = "https://api.umodelverse.ai/v1" @@ -54,8 +55,8 @@ class _AstraflowBaseProvider(LLMProvider): ) -> None: self.api_key = api_key or os.environ.get(self.api_key_env) or "" self.base_url = base_url or os.environ.get(self.base_url_env, self.default_base_url) - env_model = os.environ.get(self.model_env) - fallback_model = os.environ.get(self.fallback_model_env) if self.fallback_model_env else None + env_model = read_model_env(self.model_env) + fallback_model = read_model_env(self.fallback_model_env) if self.fallback_model_env else None self.default_model = default_model or env_model or fallback_model or DEFAULT_ASTRAFLOW_MODEL self.client = OpenAI(api_key=self.api_key, base_url=self.base_url, _enforce_credentials=False) self._models = [ diff --git a/src/llm/providers/atlas.py b/src/llm/providers/atlas.py index d626509cc..d5a93889e 100644 --- a/src/llm/providers/atlas.py +++ b/src/llm/providers/atlas.py @@ -16,6 +16,7 @@ from llm.core.interface import ( ) from llm.core.types import LLMInput, LLMOutput, ModelInfo, ProviderType, ToolCall from llm.providers.constants import EMPTY_FILTERED_RESPONSE_ERROR +from llm.providers.model_config import read_model_env from llm.providers.openai_compat import to_openai_message_param ATLAS_BASE_URL = "https://api.atlascloud.ai/v1" @@ -68,7 +69,7 @@ class AtlasProvider(LLMProvider): or "" ) self.base_url = base_url or os.environ.get(self.base_url_env, self.default_base_url) - env_model = os.environ.get(self.model_env) + env_model = read_model_env(self.model_env) self.default_model = default_model or env_model or DEFAULT_ATLAS_MODEL self.client = OpenAI(api_key=self.api_key, base_url=self.base_url, _enforce_credentials=False) self._models = [ diff --git a/src/llm/providers/model_config.py b/src/llm/providers/model_config.py new file mode 100644 index 000000000..bb14917e5 --- /dev/null +++ b/src/llm/providers/model_config.py @@ -0,0 +1,11 @@ +"""Model environment parsing shared by the resolver and provider adapters.""" + +import os + + +def read_model_env(name: str) -> str | None: + """Read a model ID, treating surrounding whitespace and quotes as config syntax.""" + value = os.environ.get(name, "").strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in {"'", '"'}: + value = value[1:-1].strip() + return value or None diff --git a/src/llm/providers/ollama.py b/src/llm/providers/ollama.py index 58505ac7b..c7a0c7121 100644 --- a/src/llm/providers/ollama.py +++ b/src/llm/providers/ollama.py @@ -19,6 +19,7 @@ from llm.core.types import ( ProviderType, ToolCall, ) +from llm.providers.model_config import read_model_env class OllamaProvider(LLMProvider): @@ -30,7 +31,7 @@ class OllamaProvider(LLMProvider): default_model: str | None = None, ) -> None: self.base_url = base_url or os.environ.get("OLLAMA_BASE_URL", "http://localhost:11434") - self.default_model = default_model or os.environ.get("OLLAMA_MODEL", "llama3.2") + self.default_model = default_model or read_model_env("OLLAMA_MODEL") or "llama3.2" self._models = [ ModelInfo( name="llama3.2", diff --git a/src/llm/providers/resolver.py b/src/llm/providers/resolver.py index a53ae3792..3b73a1c99 100644 --- a/src/llm/providers/resolver.py +++ b/src/llm/providers/resolver.py @@ -10,6 +10,7 @@ from llm.core.types import ProviderType from llm.providers.astraflow import AstraflowCNProvider, AstraflowProvider from llm.providers.atlas import AtlasProvider from llm.providers.claude import ClaudeProvider +from llm.providers.model_config import read_model_env from llm.providers.ollama import OllamaProvider from llm.providers.openai import OpenAIProvider @@ -78,19 +79,19 @@ 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) + model = read_model_env(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", "")) + env_model = read_model_env("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: +def get_provider(provider_type: ProviderType | str | None = None, **kwargs: str | None) -> LLMProvider: """Create a provider, honoring the model selected by llm-select. Built-in model defaults prefer explicit kwargs, then existing provider-specific diff --git a/tests/test_model_config.py b/tests/test_model_config.py new file mode 100644 index 000000000..86596936b --- /dev/null +++ b/tests/test_model_config.py @@ -0,0 +1,119 @@ +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path +from typing import Literal + +import pytest + +from llm.cli.selector import save_config +from llm.core.interface import LLMProvider +from llm.core.types import ProviderType +from llm.providers.astraflow import AstraflowCNProvider, AstraflowProvider +from llm.providers.atlas import AtlasProvider +from llm.providers.ollama import OllamaProvider +from llm.providers.resolver import get_provider + +pytestmark = pytest.mark.unit + + +@dataclass(frozen=True) +class ProviderCase: + provider_type: ProviderType + constructor: Callable[..., LLMProvider] + model_env: str + builtin_model: str + + +PROVIDERS = ( + ProviderCase(ProviderType.OLLAMA, OllamaProvider, "OLLAMA_MODEL", "llama3.2"), + ProviderCase(ProviderType.ATLAS, AtlasProvider, "ATLAS_MODEL", "deepseek-ai/deepseek-v4-pro"), + ProviderCase(ProviderType.ASTRAFLOW, AstraflowProvider, "ASTRAFLOW_MODEL", "gpt-4o-mini"), + ProviderCase(ProviderType.ASTRAFLOW_CN, AstraflowCNProvider, "ASTRAFLOW_CN_MODEL", "gpt-4o-mini"), +) +PROVIDER_IDS = tuple(case.provider_type.value for case in PROVIDERS) +BLANK_MODELS = (" \t ", ' " \t " ') + + +@pytest.fixture(autouse=True) +def isolated_model_config(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + monkeypatch.chdir(tmp_path) + for name in ( + "LLM_PROVIDER", "LLM_MODEL", "OLLAMA_MODEL", "ATLAS_MODEL", + "ASTRAFLOW_MODEL", "ASTRAFLOW_CN_MODEL", "OLLAMA_BASE_URL", + "ATLAS_BASE_URL", "ASTRAFLOW_BASE_URL", "ASTRAFLOW_CN_BASE_URL", + ): + # Record even absent variables: save_config(persist=True) writes them directly. + monkeypatch.setenv(name, "") + monkeypatch.delenv(name) + for name in ("ATLAS_API_KEY", "ASTRAFLOW_API_KEY", "ASTRAFLOW_CN_API_KEY"): + monkeypatch.setenv(name, "test-key") + + +def set_provider_model( + monkeypatch: pytest.MonkeyPatch, case: ProviderCase, value: str +) -> None: + monkeypatch.setenv(case.model_env, value) + if case.provider_type == ProviderType.ASTRAFLOW_CN: + monkeypatch.setenv("ASTRAFLOW_MODEL", value) + + +@pytest.mark.parametrize("case", PROVIDERS, ids=PROVIDER_IDS) +@pytest.mark.parametrize("blank_model", BLANK_MODELS, ids=("whitespace", "quoted-whitespace")) +@pytest.mark.parametrize("source", ("environment", "saved")) +def test_blank_provider_model_falls_back_to_selected_model( + monkeypatch: pytest.MonkeyPatch, + case: ProviderCase, + blank_model: str, + source: Literal["environment", "saved"], +) -> None: + save_config(case.provider_type.value, "Selected/MixedCase-model", persist=source == "environment") + set_provider_model(monkeypatch, case, blank_model) + + assert get_provider().get_default_model() == "Selected/MixedCase-model" + + +@pytest.mark.parametrize("case", PROVIDERS, ids=PROVIDER_IDS) +@pytest.mark.parametrize("blank_model", BLANK_MODELS, ids=("whitespace", "quoted-whitespace")) +def test_blank_provider_model_without_selection_retains_builtin_default( + monkeypatch: pytest.MonkeyPatch, case: ProviderCase, blank_model: str +) -> None: + set_provider_model(monkeypatch, case, blank_model) + + assert get_provider(case.provider_type).get_default_model() == case.builtin_model + assert case.constructor().get_default_model() == case.builtin_model + + +@pytest.mark.parametrize("case", PROVIDERS, ids=PROVIDER_IDS) +@pytest.mark.parametrize("model", (' "Provider/MixedCase-model" ', " 'Provider/MixedCase-model' ")) +def test_quoted_provider_model_is_normalized_without_changing_case( + monkeypatch: pytest.MonkeyPatch, case: ProviderCase, model: str +) -> None: + monkeypatch.setenv("LLM_MODEL", "selected-model") + monkeypatch.setenv(case.model_env, model) + + assert get_provider(case.provider_type).get_default_model() == "Provider/MixedCase-model" + assert case.constructor().get_default_model() == "Provider/MixedCase-model" + + +@pytest.mark.parametrize("case", PROVIDERS, ids=PROVIDER_IDS) +def test_explicit_default_model_keeps_precedence_and_original_value( + monkeypatch: pytest.MonkeyPatch, case: ProviderCase +) -> None: + save_config(case.provider_type.value, "selected-model", persist=True) + set_provider_model(monkeypatch, case, ' "Provider/MixedCase-model" ') + explicit_model = ' "Explicit/MixedCase-model" ' + + assert get_provider(default_model=explicit_model).get_default_model() == explicit_model + assert case.constructor(default_model=explicit_model).get_default_model() == explicit_model + + +@pytest.mark.parametrize("blank_model", BLANK_MODELS, ids=("whitespace", "quoted-whitespace")) +def test_cn_blank_specific_model_uses_normalized_shared_model( + monkeypatch: pytest.MonkeyPatch, blank_model: str +) -> None: + monkeypatch.setenv("ASTRAFLOW_CN_MODEL", blank_model) + monkeypatch.setenv("ASTRAFLOW_MODEL", ' "Shared/MixedCase-model" ') + monkeypatch.setenv("LLM_MODEL", "selected-model") + + assert get_provider(ProviderType.ASTRAFLOW_CN).get_default_model() == "Shared/MixedCase-model" + assert AstraflowCNProvider().get_default_model() == "Shared/MixedCase-model" diff --git a/tests/test_resolver.py b/tests/test_resolver.py index 31fac930d..24c2d80ae 100644 --- a/tests/test_resolver.py +++ b/tests/test_resolver.py @@ -1,5 +1,6 @@ import io import json +from pathlib import Path from unittest.mock import Mock import pytest @@ -111,7 +112,7 @@ class TestGetProvider: class TestSelectedModel: @pytest.fixture(autouse=True) - def isolated_config(self, monkeypatch, tmp_path): + def isolated_config(self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: monkeypatch.chdir(tmp_path) for name in ( "LLM_PROVIDER", "LLM_MODEL", "OLLAMA_MODEL", "ATLAS_MODEL", @@ -124,7 +125,7 @@ class TestSelectedModel: monkeypatch.setenv("OPENAI_API_KEY", "test-key") @pytest.mark.parametrize("persist", [False, True]) - def test_selector_model_reaches_actual_request(self, monkeypatch, persist): + def test_selector_model_reaches_actual_request(self, monkeypatch: pytest.MonkeyPatch, persist: bool) -> None: save_config("ollama", "mistral", persist=persist) urlopen = Mock(side_effect=lambda *args, **kwargs: io.BytesIO( b'{"message":{"content":"ok"},"model":"mistral"}' @@ -142,7 +143,9 @@ class TestSelectedModel: @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): + def test_selected_model_applies_to_every_builtin( + self, monkeypatch: pytest.MonkeyPatch, provider_type: ProviderType, source: str, + ) -> None: save_config(provider_type.value, "Selected/MixedCase-model", persist=source == "environment") provider = get_provider() @@ -150,7 +153,7 @@ class TestSelectedModel: assert provider.provider_type == provider_type assert provider.get_default_model() == "Selected/MixedCase-model" - def test_environment_model_overrides_saved_model(self, monkeypatch): + def test_environment_model_overrides_saved_model(self, monkeypatch: pytest.MonkeyPatch) -> None: save_config("ollama", "mistral") monkeypatch.setenv("LLM_MODEL", "codellama") @@ -163,14 +166,16 @@ class TestSelectedModel: ("astraflow_cn", "ASTRAFLOW_CN_MODEL"), ("astraflow_cn", "ASTRAFLOW_MODEL"), ]) - def test_provider_environment_retains_precedence(self, monkeypatch, provider_type, model_env): + def test_provider_environment_retains_precedence( + self, monkeypatch: pytest.MonkeyPatch, provider_type: str, model_env: str, + ) -> None: 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): + def test_cn_specific_environment_overrides_shared_fallback(self, monkeypatch: pytest.MonkeyPatch) -> None: save_config("astraflow_cn", "saved-model") monkeypatch.setenv("ASTRAFLOW_MODEL", "shared-model") monkeypatch.setenv("ASTRAFLOW_CN_MODEL", "cn-model") @@ -178,21 +183,27 @@ class TestSelectedModel: 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): + def test_explicit_default_model_overrides_selection( + self, monkeypatch: pytest.MonkeyPatch, provider_type: ProviderType, + ) -> None: 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): + def test_explicit_empty_default_retains_constructor_fallback( + self, monkeypatch: pytest.MonkeyPatch, default_model: str | None, + ) -> None: 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): + def test_provider_override_does_not_inherit_saved_model( + self, monkeypatch: pytest.MonkeyPatch, explicit: bool, + ) -> None: save_config("openai", "gpt-selected") if explicit: provider = get_provider("ollama") @@ -202,14 +213,18 @@ class TestSelectedModel: assert provider.get_default_model() == "llama3.2" - def test_explicit_provider_does_not_inherit_another_environment_model(self, monkeypatch): + def test_explicit_provider_does_not_inherit_another_environment_model( + self, monkeypatch: pytest.MonkeyPatch, + ) -> None: 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): + def test_environment_model_applies_to_matching_explicit_provider( + self, monkeypatch: pytest.MonkeyPatch, env_provider: str | None, + ) -> None: if env_provider is not None: monkeypatch.setenv("LLM_PROVIDER", env_provider) monkeypatch.setenv("LLM_MODEL", ' "MixedCase/Model" ') @@ -218,31 +233,35 @@ class TestSelectedModel: @pytest.mark.parametrize("source", ["saved", "environment"]) @pytest.mark.parametrize("model", ["", " ", '""']) - def test_empty_selected_model_retains_default(self, monkeypatch, source, model): + def test_empty_selected_model_retains_default( + self, monkeypatch: pytest.MonkeyPatch, source: str, model: str, + ) -> None: 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): + def test_empty_environment_model_falls_back_to_saved_model(self, monkeypatch: pytest.MonkeyPatch) -> None: 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): + def test_quoted_saved_model_preserves_case(self, tmp_path: Path) -> None: 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): + def test_unpaired_saved_model_does_not_override_default_provider(self, tmp_path: Path) -> None: 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): + def test_provider_override_does_not_read_invalid_saved_file( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, explicit: bool, + ) -> None: tmp_path.joinpath(".llm.env").write_bytes(b"\xff\xfe") monkeypatch.setenv("LLM_MODEL", "mistral") if explicit: @@ -253,9 +272,9 @@ class TestSelectedModel: assert provider.get_default_model() == "mistral" - def test_registered_provider_keeps_its_constructor_contract(self, monkeypatch): + def test_registered_provider_keeps_its_constructor_contract(self, monkeypatch: pytest.MonkeyPatch) -> None: class CustomProvider(OllamaProvider): - def __init__(self, base_url=None): + def __init__(self, base_url: str | None = None) -> None: super().__init__(base_url=base_url, default_model="custom-default") monkeypatch.setattr(resolver, "_PROVIDER_MAP", resolver._PROVIDER_MAP.copy()) @@ -268,7 +287,7 @@ class TestSelectedModel: assert provider.base_url == "http://localhost:1234" assert provider.get_default_model() == "custom-default" - def test_registered_provider_receives_explicit_default_model(self, monkeypatch): + def test_registered_provider_receives_explicit_default_model(self, monkeypatch: pytest.MonkeyPatch) -> None: class CustomProvider(OllamaProvider): pass