diff --git a/strix/config/loader.py b/strix/config/loader.py index fbcde8982..9a69394f1 100644 --- a/strix/config/loader.py +++ b/strix/config/loader.py @@ -54,26 +54,59 @@ def apply_config_override(path: Path) -> None: def persist_current() -> None: - """Write currently-set env vars to the active config file (0o600).""" + """Write currently-set env vars to the active config file (0o600). + + The existing ``"env"`` block on disk is merged in first so keys that are + not present in the current process environment (e.g. loaded only from the + file itself, or written by a newer version) are preserved. Current env + vars still win on conflicts. If the file is missing or contains invalid + JSON, behavior is unchanged: only the env-derived block is written. + """ s = load_settings() target = _override or _DEFAULT_PATH target.parent.mkdir(parents=True, exist_ok=True) - env_block: dict[str, str] = {} + env_block: dict[str, Any] = _read_existing_env_block(target) for sub_name in s.model_fields: sub_model = getattr(s, sub_name) if not isinstance(sub_model, BaseModel): continue for finfo in type(sub_model).model_fields.values(): - for alias in _aliases_for(finfo): - value = os.environ.get(alias.upper()) + aliases = [alias.upper() for alias in _aliases_for(finfo)] + for alias in aliases: + value = os.environ.get(alias) if value: - env_block[alias.upper()] = value + # Drop sibling aliases of this field so a stale stored + # value (e.g. LLM_API_KEY) can't shadow the fresh one on + # the next load. + for key in [k for k in env_block if k.upper() in aliases]: + del env_block[key] + env_block[alias] = value break write_secret_text(target, json.dumps({"env": env_block}, indent=2)) +def _read_existing_env_block(path: Path) -> dict[str, Any]: + """Return the ``"env"`` block already stored in ``path``. + + Values are preserved verbatim so structured entries written by newer + versions survive the rewrite. Missing files, invalid JSON, and non-dict + ``"env"`` values all yield an empty dict so persistence degrades to the + previous overwrite behavior. + """ + if not path.exists(): + return {} + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + return {} + env_block = data.get("env", {}) if isinstance(data, dict) else {} + if not isinstance(env_block, dict): + return {} + return {str(k): v for k, v in env_block.items()} + + def _aliases_for(finfo: FieldInfo) -> list[str]: """Collect every env-var name that should populate ``finfo``.""" aliases: list[str] = [] diff --git a/tests/test_config_loader.py b/tests/test_config_loader.py index e83ab1192..f90035f7e 100644 --- a/tests/test_config_loader.py +++ b/tests/test_config_loader.py @@ -216,3 +216,90 @@ def test_persist_current_sets_0600_mode(tmp_path: Path, monkeypatch: pytest.Monk loader.persist_current() assert target.stat().st_mode & 0o777 == 0o600 + + +def test_persist_current_preserves_file_only_keys(tmp_path: Path) -> None: + # Keys stored in the file but absent from the current environment (e.g. + # STRIX_LLM in a bare shell/cron/CI run) must survive a persist. + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"STRIX_LLM": "file-model", "PERPLEXITY_API_KEY": "pk"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + + loader.persist_current() + + stored = json.loads(target.read_text(encoding="utf-8"))["env"] + assert stored["STRIX_LLM"] == "file-model" + assert stored["PERPLEXITY_API_KEY"] == "pk" + + +def test_persist_current_env_var_overrides_stored_value( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("STRIX_LLM", "env-model") + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"STRIX_LLM": "file-model"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + + loader.persist_current() + + stored = json.loads(target.read_text(encoding="utf-8"))["env"] + assert stored["STRIX_LLM"] == "env-model" + + +def test_persist_current_keeps_unknown_stored_keys(tmp_path: Path) -> None: + # Keys outside the current schema (written by a newer version) must not + # be dropped by an older binary rewriting the file. + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"STRIX_LLM": "file-model", "FUTURE_OPTION": "on"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + + loader.persist_current() + + stored = json.loads(target.read_text(encoding="utf-8"))["env"] + assert stored["FUTURE_OPTION"] == "on" + + +def test_persist_current_removes_stale_sibling_alias( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # api_key resolves from AliasChoices("LLM_API_KEY", "OPENAI_API_KEY"). If the + # file holds the first alias while the env sets the second, keeping both would + # let the stale LLM_API_KEY win on the next load. It must be dropped. + monkeypatch.setenv("OPENAI_API_KEY", "sk-new") + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"LLM_API_KEY": "sk-old"}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + + loader.persist_current() + + stored = json.loads(target.read_text(encoding="utf-8"))["env"] + assert stored == {"OPENAI_API_KEY": "sk-new"} + + +def test_persist_current_preserves_non_string_stored_values(tmp_path: Path) -> None: + # Structured values written by newer versions must survive verbatim instead + # of being flattened through str(). + target = tmp_path / "cli-config.json" + target.write_text( + json.dumps({"env": {"FUTURE_HEADERS": {"X-Team": "sec"}, "FUTURE_RETRIES": 3}}), + encoding="utf-8", + ) + loader.apply_config_override(target) + + loader.persist_current() + + stored = json.loads(target.read_text(encoding="utf-8"))["env"] + assert stored["FUTURE_HEADERS"] == {"X-Team": "sec"} + assert stored["FUTURE_RETRIES"] == 3