From c7b0c0d35fd6b2864d0772c60b3648dda170ee94 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 22 Jul 2026 03:20:25 -0700 Subject: [PATCH] fix(secrets): mark _APPLIED_HOMES only after a real fetch attempt (#40597) (#69056) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _apply_external_secret_sources() added the home to _APPLIED_HOMES before loading config, so a malformed config.yaml, a missing secrets section, or all-sources-disabled permanently disabled secret loading for the process — even after the user fixed the config. Long-lived processes (gateway) never recovered without a restart. Now the home is marked only after apply_all() actually ran with at least one enabled source. Fetch errors still mark the home (so import-time load_hermes_dotenv() calls don't re-fetch and re-print the same failure 3-5x per startup); the cheap early-exit paths stay retryable. Fixes #40597. --- hermes_cli/env_loader.py | 21 +++- tests/test_env_loader_applied_homes.py | 135 +++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 tests/test_env_loader_applied_homes.py diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index d89ce643784..cb234afc36a 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -396,13 +396,19 @@ def _apply_external_secret_sources(home_path: Path) -> None: home_key = str(Path(home_path).resolve()) if home_key in _APPLIED_HOMES: return - _APPLIED_HOMES.add(home_key) try: cfg = _load_secrets_config(home_path) except Exception: # noqa: BLE001 — config errors must not block startup + # Deliberately NOT marked applied: a malformed config.yaml would + # otherwise permanently disable secret loading for this process + # even after the user fixes the file (#40597). return if not cfg: + # No secrets section (or everything disabled at parse level). Not + # marked applied either — the re-parse is a cheap fast_safe_load and + # leaving the home unmarked lets a process pick up a config change + # on its next load_hermes_dotenv() call instead of never. return try: @@ -415,6 +421,19 @@ def _apply_external_secret_sources(home_path: Path) -> None: except Exception: # noqa: BLE001 — belt-and-braces; apply_all shouldn't raise return + if not report.sources: + # Config parsed but no source is enabled: keep retrying cheaply + # (no fetch happens for disabled sources) so flipping a source on + # mid-process takes effect on the next call. + return + + # A real fetch attempt happened (success OR error). Mark the home now + # so the 3-5 import-time load_hermes_dotenv() calls per startup don't + # re-fetch / re-print — error retries within one process are opt-in via + # reset_secret_source_cache(). Marking AFTER the attempt (not before, + # see #40597) is what lets the earlier failure paths stay retryable. + _APPLIED_HOMES.add(home_key) + if report.applied_any: # Re-run the ASCII sanitization pass: vault values are # user-supplied and might have the same copy-paste corruption as diff --git a/tests/test_env_loader_applied_homes.py b/tests/test_env_loader_applied_homes.py new file mode 100644 index 00000000000..434767ea2a7 --- /dev/null +++ b/tests/test_env_loader_applied_homes.py @@ -0,0 +1,135 @@ +"""Regression tests for #40597: _APPLIED_HOMES must be marked AFTER a real +fetch attempt, so early failures (malformed config, disabled sources) stay +retryable within the process instead of being permanently skipped.""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from hermes_cli import env_loader + + +@pytest.fixture(autouse=True) +def _reset(): + env_loader.reset_secret_source_cache() + yield + env_loader.reset_secret_source_cache() + from agent.secret_sources import registry + registry._reset_registry_for_tests() + + +def _write_enabled_config(home: Path): + (home / "config.yaml").write_text( + "secrets:\n" + " bitwarden:\n" + " enabled: true\n" + " project_id: proj\n" + ) + + +def test_malformed_config_does_not_permanently_skip(tmp_path, monkeypatch): + """Config error on first call → fixed config on second call must apply.""" + home = tmp_path / ".hermes" + home.mkdir() + (home / "config.yaml").write_text("secrets: [unclosed") # malformed YAML + + env_loader._apply_external_secret_sources(home) + assert str(home.resolve()) not in env_loader._APPLIED_HOMES + + # User fixes the config; same process must now attempt the fetch. + _write_enabled_config(home) + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.t") + + import agent.secret_sources.bitwarden as bw + calls = {"n": 0} + + def fake_fetch(**kwargs): + calls["n"] += 1 + return {"NEW_KEY_40597": "val"}, [] + + monkeypatch.setattr(bw, "find_bws", lambda install_if_missing=True: home / "bws") + monkeypatch.setattr(bw, "fetch_bitwarden_secrets", fake_fetch) + monkeypatch.delenv("NEW_KEY_40597", raising=False) + + from agent.secret_sources import registry + registry._reset_registry_for_tests() + + env_loader._apply_external_secret_sources(home) + assert calls["n"] == 1 + assert str(home.resolve()) in env_loader._APPLIED_HOMES + monkeypatch.delenv("NEW_KEY_40597", raising=False) + + +def test_no_secrets_section_does_not_mark_applied(tmp_path): + home = tmp_path / ".hermes" + home.mkdir() + (home / "config.yaml").write_text("model:\n provider: openrouter\n") + env_loader._apply_external_secret_sources(home) + assert str(home.resolve()) not in env_loader._APPLIED_HOMES + + +def test_disabled_sources_do_not_mark_applied(tmp_path): + home = tmp_path / ".hermes" + home.mkdir() + (home / "config.yaml").write_text( + "secrets:\n bitwarden:\n enabled: false\n project_id: p\n" + ) + from agent.secret_sources import registry + registry._reset_registry_for_tests() + env_loader._apply_external_secret_sources(home) + assert str(home.resolve()) not in env_loader._APPLIED_HOMES + + +def test_fetch_error_still_marks_applied(tmp_path, monkeypatch): + """A real fetch attempt that FAILS still marks the home — otherwise every + import-time load_hermes_dotenv() would re-fetch and re-print the same + error 3-5x per startup.""" + home = tmp_path / ".hermes" + home.mkdir() + _write_enabled_config(home) + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.dead") + + import agent.secret_sources.bitwarden as bw + calls = {"n": 0} + + def boom(**kwargs): + calls["n"] += 1 + raise RuntimeError("bws exited 1: network unreachable") + + monkeypatch.setattr(bw, "find_bws", lambda install_if_missing=True: home / "bws") + monkeypatch.setattr(bw, "fetch_bitwarden_secrets", boom) + + from agent.secret_sources import registry + registry._reset_registry_for_tests() + + env_loader._apply_external_secret_sources(home) + env_loader._apply_external_secret_sources(home) # second call = no-op + assert calls["n"] == 1 + assert str(home.resolve()) in env_loader._APPLIED_HOMES + + +def test_success_marks_applied_and_second_call_noop(tmp_path, monkeypatch): + home = tmp_path / ".hermes" + home.mkdir() + _write_enabled_config(home) + monkeypatch.setenv("BWS_ACCESS_TOKEN", "0.t") + monkeypatch.delenv("KEY_OK_40597", raising=False) + + import agent.secret_sources.bitwarden as bw + calls = {"n": 0} + + def fake_fetch(**kwargs): + calls["n"] += 1 + return {"KEY_OK_40597": "v"}, [] + + monkeypatch.setattr(bw, "find_bws", lambda install_if_missing=True: home / "bws") + monkeypatch.setattr(bw, "fetch_bitwarden_secrets", fake_fetch) + + from agent.secret_sources import registry + registry._reset_registry_for_tests() + + env_loader._apply_external_secret_sources(home) + env_loader._apply_external_secret_sources(home) + assert calls["n"] == 1 + monkeypatch.delenv("KEY_OK_40597", raising=False)