diff --git a/strix/config/loader.py b/strix/config/loader.py index 969d67f8..250790c6 100644 --- a/strix/config/loader.py +++ b/strix/config/loader.py @@ -10,11 +10,13 @@ from typing import TYPE_CHECKING, Any from pydantic import AliasChoices, BaseModel -from strix.config.settings import Settings +from strix.config.settings import LlmSettings, Settings from strix.utils.secret_files import write_secret_text if TYPE_CHECKING: + from collections.abc import Mapping + from pydantic.fields import FieldInfo @@ -25,6 +27,11 @@ _DEFAULT_PATH: Path = Path.home() / ".strix" / "cli-config.json" _override: Path | None = None _cached: Settings | None = None +# Model, API key, and API base describe one provider connection. When the shell +# changes any of them, the stored values of the others no longer belong together +# and are dropped rather than mixed with the new value. +_LINKED_LLM_FIELDS = ("model", "api_key", "api_base") + def load_settings() -> Settings: """Resolve settings from env + JSON file + defaults. Memoized. @@ -58,13 +65,14 @@ def persist_current() -> None: Values already in the file survive when their env var is unset, so a run that gets its settings from the file does not erase them. An env - var set to the empty string clears the field from the file. + var set to the empty string clears the field from the file. A change to + any linked LLM connection var drops the whole stored connection first. """ s = load_settings() target = _override or _DEFAULT_PATH target.parent.mkdir(parents=True, exist_ok=True) - env_block = _read_env_block(target) + env_block = _drop_stale_llm_connection(_read_env_block(target)) for sub_name in type(s).model_fields: sub_model = getattr(s, sub_name) if not isinstance(sub_model, BaseModel): @@ -104,7 +112,7 @@ def _read_json_overrides(path: Path) -> dict[str, dict[str, Any]]: Only includes keys whose env var is NOT already set, so env always wins over the persisted file. """ - env_block_upper = _read_env_block(path) + env_block_upper = _drop_stale_llm_connection(_read_env_block(path)) if not env_block_upper: return {} env_present = {k.upper() for k in os.environ} @@ -128,6 +136,27 @@ def _read_json_overrides(path: Path) -> dict[str, dict[str, Any]]: return nested +def _first_alias_value(aliases: list[str], source: Mapping[str, Any]) -> Any | None: + return next((source[alias] for alias in aliases if alias in source), None) + + +def _drop_stale_llm_connection(env_block: dict[str, Any]) -> dict[str, Any]: + """Remove every linked LLM var from ``env_block`` if the shell changed any of them.""" + linked_aliases = [ + [alias.upper() for alias in _aliases_for(LlmSettings.model_fields[name])] + for name in _LINKED_LLM_FIELDS + ] + changed = any( + (env_value := _first_alias_value(aliases, os.environ)) is not None + and env_value != _first_alias_value(aliases, env_block) + for aliases in linked_aliases + ) + if not changed: + return env_block + stale = {alias for aliases in linked_aliases for alias in aliases} + return {k: v for k, v in env_block.items() if k not in stale} + + def _read_env_block(path: Path) -> dict[str, Any]: """Return the ``env`` block stored in ``path`` with upper-cased keys, or ``{}``.""" if not path.exists(): diff --git a/tests/test_config_loader.py b/tests/test_config_loader.py index be975f0e..c370f8eb 100644 --- a/tests/test_config_loader.py +++ b/tests/test_config_loader.py @@ -229,16 +229,109 @@ def test_persist_current_env_overrides_file_value( ) -> None: target = tmp_path / "cli-config.json" target.write_text( - json.dumps({"env": {"STRIX_LLM": "file-model", "LLM_API_KEY": "file-key"}}), + json.dumps({"env": {"STRIX_LLM": "file-model", "PERPLEXITY_API_KEY": "file-pplx"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + monkeypatch.setenv("PERPLEXITY_API_KEY", "env-pplx") + + loader.persist_current() + + assert json.loads(target.read_text(encoding="utf-8")) == { + "env": {"STRIX_LLM": "file-model", "PERPLEXITY_API_KEY": "env-pplx"} + } + + +def test_linked_llm_model_change_drops_stored_key_and_base( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps( + { + "env": { + "STRIX_LLM": "file-model", + "LLM_API_KEY": "file-key", + "LLM_API_BASE": "http://file-base", + "PERPLEXITY_API_KEY": "pplx", + } + } + ), encoding="utf-8", ) loader.apply_config_override(target) monkeypatch.setenv("STRIX_LLM", "env-model") + llm = loader.load_settings().llm + assert llm.model == "env-model" + assert llm.api_key is None + assert llm.api_base is None + loader.persist_current() assert json.loads(target.read_text(encoding="utf-8")) == { - "env": {"STRIX_LLM": "env-model", "LLM_API_KEY": "file-key"} + "env": {"STRIX_LLM": "env-model", "PERPLEXITY_API_KEY": "pplx"} + } + + +def test_linked_llm_key_change_drops_stored_model( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"STRIX_LLM": "file-model", "LLM_API_KEY": "file-key"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + monkeypatch.setenv("LLM_API_KEY", "new-key") + + assert loader.load_settings().llm.model is None + + loader.persist_current() + + assert json.loads(target.read_text(encoding="utf-8")) == {"env": {"LLM_API_KEY": "new-key"}} + + +def test_linked_llm_secondary_alias_in_env_is_not_a_change( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"STRIX_LLM": "file-model", "LLM_API_KEY": "file-key"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + monkeypatch.setenv("LLM_API_KEY", "file-key") + monkeypatch.setenv("OPENAI_API_KEY", "unrelated-global-key") + + llm = loader.load_settings().llm + assert llm.model == "file-model" + assert llm.api_key == "file-key" + + loader.persist_current() + + assert json.loads(target.read_text(encoding="utf-8")) == { + "env": {"STRIX_LLM": "file-model", "LLM_API_KEY": "file-key"} + } + + +def test_linked_llm_unchanged_env_keeps_stored_key( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"STRIX_LLM": "file-model", "LLM_API_KEY": "file-key"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + monkeypatch.setenv("STRIX_LLM", "file-model") + + assert loader.load_settings().llm.api_key == "file-key" + + loader.persist_current() + + assert json.loads(target.read_text(encoding="utf-8")) == { + "env": {"STRIX_LLM": "file-model", "LLM_API_KEY": "file-key"} } diff --git a/tests/test_dedupe_model.py b/tests/test_dedupe_model.py index 547f434d..809867b1 100644 --- a/tests/test_dedupe_model.py +++ b/tests/test_dedupe_model.py @@ -114,7 +114,14 @@ def test_config_file_loads_dedupe_model( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - for key in ("STRIX_LLM", "STRIX_DEDUPE_MODEL", "STRIX_DEDUPE_REASONING_EFFORT"): + for key in ( + "STRIX_LLM", + "LLM_API_KEY", + "OPENAI_API_KEY", + "LLM_API_BASE", + "STRIX_DEDUPE_MODEL", + "STRIX_DEDUPE_REASONING_EFFORT", + ): monkeypatch.delenv(key, raising=False) path = tmp_path / "config.json" path.write_text(