diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 1eee1e187d6..9129becd603 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -224,9 +224,11 @@ _LAST_EXPANDED_CONFIG_BY_PATH: Dict[str, Any] = {} # produces a fresh inode, so stat() sees a new mtime_ns and the next # load repopulates automatically — no explicit invalidation hook. # Cached tuple is (user_mtime_ns, user_size, managed_mtime_ns, managed_size, -# merged_value) — the managed-file signature is folded in so editing the -# managed-scope config.yaml invalidates the cache (see managed_scope). -_LOAD_CONFIG_CACHE: Dict[str, Tuple[int, int, int, int, Dict[str, Any]]] = {} +# merged_value, env_ref_snapshot) — the managed-file signature is folded in so +# editing the managed-scope config.yaml invalidates the cache (see +# managed_scope), and the env snapshot invalidates it when a referenced ${VAR} +# changes value (late .env load, in-process rotation — #58514). +_LOAD_CONFIG_CACHE: Dict[str, Tuple[int, int, int, int, Dict[str, Any], Dict[str, Optional[str]]]] = {} # (path, mtime_ns, size) -> cached raw yaml dict. Same pattern as # _LOAD_CONFIG_CACHE but for read_raw_config() — used when callers want # the user's on-disk values without defaults merged in. @@ -6147,6 +6149,31 @@ def _expand_env_vars(obj): return obj +def _env_ref_snapshot(obj, snapshot=None): + """Map every ``${VAR}`` name referenced in config values to its current + ``os.environ`` value (``None`` when unset). + + Stored alongside cached ``load_config()`` results so a cache hit can + detect that the cached expansion was made against a *different* + environment — e.g. a ``load_config()`` that ran before + ``load_hermes_dotenv()`` populated the process env, or an env var + rotated in-process after the first load. File mtime/size alone cannot + see either case (#58514). + """ + if snapshot is None: + snapshot = {} + if isinstance(obj, str): + for name in re.findall(r"\${([^}]+)}", obj): + snapshot[name] = os.environ.get(name) + elif isinstance(obj, dict): + for value in obj.values(): + _env_ref_snapshot(value, snapshot) + elif isinstance(obj, list): + for item in obj: + _env_ref_snapshot(item, snapshot) + return snapshot + + def _items_by_unique_name(items): """Return a name-indexed dict only when all items have unique string names.""" if not isinstance(items, list): @@ -6746,7 +6773,14 @@ def _load_config_impl(*, want_deepcopy: bool) -> Dict[str, Any]: cached = _LOAD_CONFIG_CACHE.get(path_key) if cached is not None and cache_sig is not None and cached[:4] == cache_sig: - return copy.deepcopy(cached[4]) if want_deepcopy else cached[4] + # File signatures match, but the cached expansion is only valid if + # every ${VAR} it was expanded against still has the same value. + # Without this, a load_config() that ran before load_hermes_dotenv() + # pins unexpanded literals (e.g. auxiliary..api_key) for the + # life of the process (#58514). + env_snapshot = cached[5] if len(cached) > 5 else {} + if all(os.environ.get(k) == v for k, v in env_snapshot.items()): + return copy.deepcopy(cached[4]) if want_deepcopy else cached[4] config = copy.deepcopy(DEFAULT_CONFIG) @@ -6783,9 +6817,15 @@ def _load_config_impl(*, want_deepcopy: bool) -> Dict[str, Any]: # (deepcopy=True) callers can mutate freely without affecting the # cached value, and ``load_config_readonly()`` (deepcopy=False) # callers all see the same stable cached object. The cached tuple is - # (user_mtime, user_size, managed_mtime, managed_size, value). + # (user_mtime, user_size, managed_mtime, managed_size, value, + # env_ref_snapshot). The snapshot records the environment values + # this expansion was made against so later loads can detect env + # drift (late .env load, in-process rotation) — see cache hit above. cached_copy = copy.deepcopy(expanded) - _LOAD_CONFIG_CACHE[path_key] = (*cache_sig, cached_copy) + env_snapshot = _env_ref_snapshot(normalized) + if managed_config: + _env_ref_snapshot(managed_config, env_snapshot) + _LOAD_CONFIG_CACHE[path_key] = (*cache_sig, cached_copy, env_snapshot) # On the readonly path return the same cached object subsequent # calls will see — keeps "two readonly calls return the same # object" invariant that callers may rely on for identity checks. diff --git a/tests/hermes_cli/test_config_env_expansion.py b/tests/hermes_cli/test_config_env_expansion.py index 435a4766878..acc41da7c46 100644 --- a/tests/hermes_cli/test_config_env_expansion.py +++ b/tests/hermes_cli/test_config_env_expansion.py @@ -94,6 +94,59 @@ class TestLoadConfigExpansion: assert config["model"]["api_key"] == "${NOT_SET_XYZ_123}" +class TestLoadConfigCacheEnvStaleness: + """The load_config() cache must not pin expansions made against a stale + environment (#58514): a load before load_hermes_dotenv() runs, or an env + var rotated in-process, must not keep serving the old expansion.""" + + def test_env_var_appearing_after_first_load_invalidates_cache(self, tmp_path, monkeypatch): + config_yaml = "auxiliary:\n vision:\n api_key: ${LATE_DOTENV_KEY_58514}\n" + config_file = tmp_path / "config.yaml" + config_file.write_text(config_yaml) + + monkeypatch.delenv("LATE_DOTENV_KEY_58514", raising=False) + monkeypatch.setitem(load_config.__globals__, "get_config_path", lambda: config_file) + + # First load happens before the var exists (pre-dotenv): literal kept. + assert load_config()["auxiliary"]["vision"]["api_key"] == "${LATE_DOTENV_KEY_58514}" + + # .env load brings the var in — same file mtime/size, env changed. + monkeypatch.setenv("LATE_DOTENV_KEY_58514", "nvapi-real") + assert load_config()["auxiliary"]["vision"]["api_key"] == "nvapi-real" + + def test_env_var_rotation_invalidates_cache(self, tmp_path, monkeypatch): + config_yaml = "providers:\n mistral:\n api_key: ${ROTATED_KEY_58514}\n" + config_file = tmp_path / "config.yaml" + config_file.write_text(config_yaml) + + monkeypatch.setenv("ROTATED_KEY_58514", "key-v1") + monkeypatch.setitem(load_config.__globals__, "get_config_path", lambda: config_file) + + assert load_config()["providers"]["mistral"]["api_key"] == "key-v1" + + monkeypatch.setenv("ROTATED_KEY_58514", "key-v2") + assert load_config()["providers"]["mistral"]["api_key"] == "key-v2" + + def test_unchanged_env_still_serves_cache(self, tmp_path, monkeypatch): + config_yaml = "providers:\n mistral:\n api_key: ${STABLE_KEY_58514}\n" + config_file = tmp_path / "config.yaml" + config_file.write_text(config_yaml) + + monkeypatch.setenv("STABLE_KEY_58514", "key-stable") + monkeypatch.setitem(load_config.__globals__, "get_config_path", lambda: config_file) + + load_config() + # load_config_readonly() returns the cached object itself, so object + # identity across calls proves the cache-hit path was taken (a rebuild + # would produce a fresh dict). + readonly = load_config.__globals__["load_config_readonly"] + first = readonly() + second = readonly() + + assert first is second + assert first["providers"]["mistral"]["api_key"] == "key-stable" + + class TestLoadCliConfigExpansion: """Verify that load_cli_config() also expands ${VAR} references."""