diff --git a/agent/secret_sources/registry.py b/agent/secret_sources/registry.py index 9294e771b5..0bd36ba991 100644 --- a/agent/secret_sources/registry.py +++ b/agent/secret_sources/registry.py @@ -52,6 +52,9 @@ class AppliedVar: source: str # SecretSource.name shape: str # "mapped" | "bulk" overrode_env: bool # replaced a pre-existing .env/shell value + # The source may beat .env/shell for this var (``override_existing`` and not ``preserve_existing``), so a + # dotenv reload may re-assert it; a gap-fill or preserved name must keep following .env edits (#74265). + authoritative: bool = False @dataclass @@ -359,7 +362,8 @@ class _Applier: self.env[var] = value self.claimed[var] = source.name sr.applied.append(var) - self.report.provenance[var] = AppliedVar(var, source.name, source.shape, overrode_env=existed) + self.report.provenance[var] = AppliedVar(var, source.name, source.shape, overrode_env=existed, + authoritative=override and var not in self.preserve) return True diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index ac8b0ba72e..1addf55758 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -34,6 +34,8 @@ _SCOPED_SKIP_LOGGED: set[str] = set() # routed profile homes whose multiplex d _SECRET_SOURCES: dict[str, str] = {} # Immutable per-home snapshots: os.environ is shared across profiles and a later home's apply may overwrite it. _SECRET_SOURCE_VALUES_BY_HOME: dict[str, dict[str, str]] = {} +# Per home: the subset of the snapshot a dotenv reload may re-assert — see ``AppliedVar.authoritative`` (#74265). +_SECRET_SOURCE_RESTORE_BY_HOME: dict[str, dict[str, str]] = {} # HERMES_HOME paths already pulled external secrets for: load_hermes_dotenv() runs at import time from # several hot modules, so without this the Bitwarden status line prints 3-5x per startup and the config # re-parse + ASCII sweep re-run each time (Bitwarden's own cache only saves the network call). @@ -120,6 +122,7 @@ def _hydrate_profile_secret_sources(home: Path) -> dict[str, str]: # A retry must not keep serving a partial result after the source is removed, disabled, or can no # longer be evaluated. Publish only the snapshot established by this attempt. _SECRET_SOURCE_VALUES_BY_HOME.pop(home_key, None) + _SECRET_SOURCE_RESTORE_BY_HOME.pop(home_key, None) try: cfg = _load_secrets_config(home) @@ -177,10 +180,12 @@ def reset_secret_source_cache(hermes_home: str | os.PathLike | None = None) -> N _APPLIED_HOMES.clear() _SECRET_SOURCES.clear() _SECRET_SOURCE_VALUES_BY_HOME.clear() + _SECRET_SOURCE_RESTORE_BY_HOME.clear() return home_key = str(Path(hermes_home).resolve()) _APPLIED_HOMES.discard(home_key) _SECRET_SOURCE_VALUES_BY_HOME.pop(home_key, None) + _SECRET_SOURCE_RESTORE_BY_HOME.pop(home_key, None) def format_secret_source_suffix(env_var: str) -> str: @@ -410,13 +415,6 @@ def load_hermes_dotenv( project_env_path = Path(project_env) if project_env else None load_pass = next(_DOTENV_PASSES) # one pass: later layers below see the earlier layers' output - # Snapshot the values an external secret source (Bitwarden, 1Password, ...) already resolved for THIS - # home on an earlier load. load_dotenv(override=True) below would write the raw .env placeholder - # (``__BITWARDEN_MANAGED__``) or a stale token back over them, and _apply_external_secret_sources() is a - # once-per-home no-op (``_APPLIED_HOMES``), so the clobber would stick for the life of the process - # (#74265). Per-home on purpose: a process-global snapshot would leak profile A's secrets into B. - protected_secret_values = get_secret_source_values(home_path) - if user_env.exists(): # normalize formatting / strip NULs before parsing _sanitize_env_file_if_needed(user_env) if project_env_path and project_env_path.exists(): @@ -438,11 +436,14 @@ def load_hermes_dotenv( _load_dotenv_with_fallback(project_env_path, override=not loaded, load_pass=load_pass) loaded.append(project_env_path) - # Undo the dotenv clobber of external-source secrets before the (no-op on reload) source pass. Managed - # scope, applied last with override=True, still beats a source value on purpose. - for name, value in protected_secret_values.items(): - if os.environ.get(name) != value: - os.environ[name] = value + # The override=True loads above wrote the raw .env line (``__BITWARDEN_MANAGED__`` placeholder, stale + # token) back over a value an external source resolved on an earlier call, and the source pass below is a + # once-per-home no-op — so the clobber stuck for the life of the process (#74265). Re-assert only what the + # source is authoritative for; managed scope, applied last with override=True, still wins on purpose. + if _SECRET_SOURCE_RESTORE_BY_HOME: + for name, value in _SECRET_SOURCE_RESTORE_BY_HOME.get(str(home_path.resolve()), {}).items(): + if os.environ.get(name) != value: + os.environ[name] = value # External sources are skipped for the updater (dotenv + managed env still load): ``update`` must not # import optional secret-manager libs (Bitwarden → cryptography → _rust.pyd) into the process replacing @@ -573,6 +574,8 @@ def _apply_external_secret_sources(home_path: Path) -> None: values[name] = os.environ[name] if values: _SECRET_SOURCE_VALUES_BY_HOME[home_key] = values + _SECRET_SOURCE_RESTORE_BY_HOME[home_key] = { + n: values[n] for n, a in report.provenance.items() if a.authoritative and n in values} for src in report.sources: if src.applied: diff --git a/tests/agent/test_env_loader_reload_restore.py b/tests/agent/test_env_loader_reload_restore.py index 5efec89c10..9ebe715c33 100644 --- a/tests/agent/test_env_loader_reload_restore.py +++ b/tests/agent/test_env_loader_reload_restore.py @@ -26,11 +26,9 @@ def _fresh_state(): from agent.secret_sources import registry as reg_module reg_module._reset_registry_for_tests() - env_loader._SECRET_SOURCES.clear() env_loader.reset_secret_source_cache() yield reg_module._reset_registry_for_tests() - env_loader._SECRET_SOURCES.clear() env_loader.reset_secret_source_cache() @@ -54,10 +52,13 @@ def _register_fake_source(values: dict[str, str], *, override_existing: bool): reg_module.register_source(_Fake(), replace=True) -def _make_home(tmp_path: Path, monkeypatch, env_text: str) -> Path: +def _make_home(tmp_path: Path, monkeypatch, env_text: str, *, preserve: str = "") -> Path: home = tmp_path / ".hermes" home.mkdir() - (home / "config.yaml").write_text("secrets:\n fakebulk:\n enabled: true\n", encoding="utf-8") + secrets = "secrets:\n fakebulk:\n enabled: true\n" + if preserve: + secrets += f" preserve_existing: [{preserve}]\n" + (home / "config.yaml").write_text(secrets, encoding="utf-8") (home / ".env").write_text(env_text, encoding="utf-8") monkeypatch.setenv("HERMES_HOME", str(home)) monkeypatch.delenv("GLM_API_KEY", raising=False) @@ -69,6 +70,7 @@ def test_source_secret_survives_second_load_hermes_dotenv(tmp_path, monkeypatch) placeholder on load 1; load 2 (gateway import / per-turn reload / cron fire) must keep the resolved value instead of writing the placeholder back.""" home = _make_home(tmp_path, monkeypatch, "GLM_API_KEY=__BITWARDEN_MANAGED__\n") + # override_existing=True: the source is authoritative over .env, which is what makes the restore legal. _register_fake_source({"GLM_API_KEY": "vault-value"}, override_existing=True) env_loader.load_hermes_dotenv(hermes_home=home) @@ -78,3 +80,26 @@ def test_source_secret_survives_second_load_hermes_dotenv(tmp_path, monkeypatch) assert os.environ["GLM_API_KEY"] == "vault-value", ( "second load_hermes_dotenv() wrote the .env placeholder back over the source-resolved value" ) + + +@pytest.mark.parametrize( + ("override_existing", "preserve"), + [(False, ""), (True, "GLM_API_KEY")], + ids=["gap-fill-source", "preserve_existing-name"], +) +def test_reload_restore_keeps_dotenv_precedence(tmp_path, monkeypatch, override_existing, preserve): + """Names .env must win are never restored over a later .env edit: an ``override_existing: false`` + source only fills gaps, and a ``secrets.preserve_existing`` name keeps .env's value even against an + overriding source. Both land in the per-home snapshot on load 1 (no .env value yet), so restoring + the whole snapshot froze them at the source value; a cold start would have yielded ``local``.""" + home = _make_home(tmp_path, monkeypatch, "", preserve=preserve) + _register_fake_source({"GLM_API_KEY": "vault-value"}, override_existing=override_existing) + + env_loader.load_hermes_dotenv(hermes_home=home) + assert os.environ["GLM_API_KEY"] == "vault-value" + + (home / ".env").write_text("GLM_API_KEY=local\n", encoding="utf-8") + env_loader.load_hermes_dotenv(hermes_home=home) + assert os.environ["GLM_API_KEY"] == "local", ( + "reload restore re-asserted a source value over the user's .env edit" + )