From 08830efd96489f78f322caaa464abf61f1179f0a Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:10:12 -0700 Subject: [PATCH] fix(secrets): secret-source re-pull no longer latches an empty snapshot or wipes sibling profiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom (#102041): under a multiplex gateway the default profile's vault/1Password/ Bitwarden/plugin-sourced credentials vanished for the rest of the process after the first cron fire or the post-discovery plugin refresh; with the key already in the process env (systemd EnvironmentFile=) the scope was empty from boot. Every get_secret() read then failed closed ("No usable credentials", every Telegram sender rejected). Why: _apply_external_secret_sources marked the home applied after any real fetch, but only snapshotted names in report.provenance — the NEWLY applied ones. On a re-apply the previous apply's own write-back makes every key `skipped_existing`, so the snapshot latched to {} and _hydrate_profile_secret_sources returned that empty snapshot forever. Separately, reset_secret_source_cache() was process-wide, so one profile's cron re-pull dropped every sibling's hydrated snapshot (1aa62ceb458 isolated the routed reload but not the reset). Change: - env_loader: snapshot every name a source supplied (provenance + skipped_existing) from the home's effective environment, so a shadowed re-apply keeps the values it had. - reset_secret_source_cache(hermes_home=None): optional per-home reset; global clear kept for tests/config edits. - cron per-fire re-pull and plugins._refresh_secret_sources_after_discovery reset + reload only the home they resolve to. - Two invariant tests (red on base) in tests/test_env_loader_secret_sources.py; docs note in secret-source-plugin.md. Reported-by: luochen1990 Addresses #102041 --- cron/scheduler.py | 2 +- hermes_cli/env_loader.py | 45 +++++++---- hermes_cli/plugins.py | 9 ++- tests/cron/test_scheduler.py | 2 +- .../test_secret_source_bootstrap.py | 12 +-- tests/test_env_loader_secret_sources.py | 75 +++++++++++++++++++ .../developer-guide/secret-source-plugin.md | 2 +- 7 files changed, 122 insertions(+), 25 deletions(-) diff --git a/cron/scheduler.py b/cron/scheduler.py index 6d758c42dd..ebdd73f985 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -2089,7 +2089,7 @@ def _reload_dotenv_and_publish_delivery_target(job: dict) -> None: from hermes_cli.env_loader import load_hermes_dotenv, reset_secret_source_cache from gateway.session_context import _VAR_MAP - reset_secret_source_cache() + reset_secret_source_cache(_get_hermes_home()) load_hermes_dotenv(hermes_home=_get_hermes_home()) delivery_target = _resolve_delivery_target(job) diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index ba99297d5d..4b7e83bd2c 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -156,11 +156,21 @@ def _hydrate_profile_secret_sources(home: Path) -> dict[str, str]: return dict(values) -def reset_secret_source_cache() -> None: - """Forget applied homes so the next load re-pulls (tests, long-running processes after config edits).""" - _APPLIED_HOMES.clear() - _SECRET_SOURCES.clear() - _SECRET_SOURCE_VALUES_BY_HOME.clear() +def reset_secret_source_cache(hermes_home: str | os.PathLike | None = None) -> None: + """Forget applied homes so the next load re-pulls (tests, long-running processes after config edits). + + ``hermes_home`` limits the reset to ONE home: a multiplex gateway keeps every profile's snapshot in + this process, and a per-fire cron re-pull or a plugin-discovery refresh for one home must not wipe + a sibling's hydrated snapshot — the sibling's next scope build would run empty until it re-hydrated + (#102041).""" + if hermes_home is None: + _APPLIED_HOMES.clear() + _SECRET_SOURCES.clear() + _SECRET_SOURCE_VALUES_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) def format_secret_source_suffix(env_var: str) -> str: @@ -476,19 +486,26 @@ def _apply_external_secret_sources(home_path: Path) -> None: # Marking AFTER the attempt keeps the earlier failure paths retryable. _APPLIED_HOMES.add(home_key) - # 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. if report.applied_any: _sanitize_loaded_credentials() # vault values carry the same copy-paste corruption risk as .env - # Re-run the ASCII sanitization pass: vault values are user-supplied and might have the same - # copy-paste corruption as a manually edited .env (see #6843). - values: dict[str, str] = {} for name, applied in report.provenance.items(): _SECRET_SOURCES[name] = applied.source - if name in os.environ: - values[name] = os.environ[name] + + # Snapshot EVERY name a source supplied, not just the newly applied ones. A name the source supplied + # but the pre-existing process value won (``skipped_existing``) is still this home's effective value + # for it — and on the unscoped path os.environ IS this home's environment. Skipping those names + # latched an EMPTY snapshot whenever the key was already in the env: after the first cron/plugin + # re-pull (the previous apply's own write-back shadows every key) or from boot under systemd + # ``EnvironmentFile=``. Under multiplex the scope is the only credential source, so an empty + # snapshot failed every default-profile turn for the process lifetime (#102041). + values: dict[str, str] = {} + supplied = set(report.provenance) + for src in report.sources: + supplied.update(src.skipped_existing) + for name in supplied: + if name in os.environ: + values[name] = os.environ[name] + if values: _SECRET_SOURCE_VALUES_BY_HOME[home_key] = values for src in report.sources: diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index da8d7e2c68..bc45188e48 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1275,8 +1275,13 @@ class PluginManager(PluginLoaderMixin, PluginDispatchMixin, PluginLedgerMixin): if not enabled_names: return try: - reset_secret_source_cache() - load_hermes_dotenv() + # Reset and reload the SAME home the process (or routed turn) resolves to: under multiplex this + # runs at gateway boot after sibling profiles may already have hydrated, and a global clear + # wiped their snapshots; a routed discovery must rebuild the profile it just dropped. + from hermes_constants import get_hermes_home + home = get_hermes_home() + reset_secret_source_cache(home) + load_hermes_dotenv(hermes_home=home) logger.debug("Re-applied secret sources after plugin discovery for: %s", ", ".join(sorted(enabled_names))) except Exception as exc: diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 95fdcfd227..0947edb459 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -975,7 +975,7 @@ class TestRunJobSessionPersistence: fake_db = MagicMock() call_order = [] - def _record_reset(): + def _record_reset(*_args): call_order.append("reset") def _record_load(*args, **kwargs): diff --git a/tests/hermes_cli/test_secret_source_bootstrap.py b/tests/hermes_cli/test_secret_source_bootstrap.py index f7f1188a5a..481b275859 100644 --- a/tests/hermes_cli/test_secret_source_bootstrap.py +++ b/tests/hermes_cli/test_secret_source_bootstrap.py @@ -42,7 +42,7 @@ def test_refresh_secret_sources_noop_without_plugin_sources(monkeypatch): monkeypatch.setattr(reg, "list_plugin_sources", lambda: []) monkeypatch.setattr( "hermes_cli.env_loader.reset_secret_source_cache", - lambda: called.__setitem__("reset", called["reset"] + 1), + lambda *a, **kw: called.__setitem__("reset", called["reset"] + 1), ) monkeypatch.setattr( "hermes_cli.env_loader.load_hermes_dotenv", @@ -68,7 +68,7 @@ def test_refresh_secret_sources_noop_when_only_builtins(monkeypatch): ) monkeypatch.setattr( "hermes_cli.env_loader.reset_secret_source_cache", - lambda: called.__setitem__("reset", called["reset"] + 1), + lambda *a, **kw: called.__setitem__("reset", called["reset"] + 1), ) monkeypatch.setattr( "hermes_cli.env_loader.load_hermes_dotenv", @@ -92,7 +92,7 @@ def test_refresh_secret_sources_repulls_when_plugin_enabled(monkeypatch): ) monkeypatch.setattr( "hermes_cli.env_loader.reset_secret_source_cache", - lambda: called.__setitem__("reset", called["reset"] + 1), + lambda *a, **kw: called.__setitem__("reset", called["reset"] + 1), ) monkeypatch.setattr( "hermes_cli.env_loader.load_hermes_dotenv", @@ -120,7 +120,7 @@ def test_refresh_respects_custom_is_enabled(monkeypatch): ) monkeypatch.setattr( "hermes_cli.env_loader.reset_secret_source_cache", - lambda: called.__setitem__("reset", called["reset"] + 1), + lambda *a, **kw: called.__setitem__("reset", called["reset"] + 1), ) monkeypatch.setattr( "hermes_cli.env_loader.load_hermes_dotenv", @@ -147,7 +147,7 @@ def test_refresh_skips_custom_source_when_not_activated(monkeypatch): ) monkeypatch.setattr( "hermes_cli.env_loader.reset_secret_source_cache", - lambda: called.__setitem__("reset", called["reset"] + 1), + lambda *a, **kw: called.__setitem__("reset", called["reset"] + 1), ) monkeypatch.setattr( "hermes_cli.env_loader.load_hermes_dotenv", @@ -175,7 +175,7 @@ def test_refresh_skips_source_whose_is_enabled_raises(monkeypatch): ) monkeypatch.setattr( "hermes_cli.env_loader.reset_secret_source_cache", - lambda: called.__setitem__("reset", called["reset"] + 1), + lambda *a, **kw: called.__setitem__("reset", called["reset"] + 1), ) monkeypatch.setattr( "hermes_cli.env_loader.load_hermes_dotenv", diff --git a/tests/test_env_loader_secret_sources.py b/tests/test_env_loader_secret_sources.py index c2959144c9..ff4805064a 100644 --- a/tests/test_env_loader_secret_sources.py +++ b/tests/test_env_loader_secret_sources.py @@ -600,3 +600,78 @@ def test_apply_external_secret_sources_bad_ttl_does_not_crash(tmp_path, monkeypa # Coerced to the 300s default rather than raising ValueError. assert captured["cache_ttl_seconds"] == 300 + + +@pytest.fixture +def _fresh_registry(): + from agent.secret_sources import registry as reg_module + + reg_module._reset_registry_for_tests() + yield + reg_module._reset_registry_for_tests() + + +def _register_fake_bulk_source(value_for_home): + """One bulk source supplying GLM_API_KEY, resolved per home.""" + from agent.secret_sources import registry as reg_module + from agent.secret_sources.base import FetchResult, SecretSource + + class _Fake(SecretSource): + name = "fakebulk" + label = "Fake" + shape = "bulk" + + def fetch(self, cfg, home_path): + result = FetchResult() + result.secrets = {"GLM_API_KEY": value_for_home(Path(home_path))} + return result + + reg_module.register_source(_Fake(), replace=True) + + +def test_env_shadowed_reapply_keeps_home_snapshot(tmp_path, monkeypatch, _fresh_registry): + """#102041: a re-apply whose every key is ``skipped_existing`` (the previous apply's own write-back, + or a systemd ``EnvironmentFile=`` value) must still snapshot the home's effective values. Latching + an empty snapshot made ``build_profile_secret_scope`` drop every vault credential for the process + lifetime under multiplex.""" + from agent.secret_scope import build_profile_secret_scope + + home = tmp_path / ".hermes" + home.mkdir() + (home / "config.yaml").write_text("secrets:\n fakebulk:\n enabled: true\n", encoding="utf-8") + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.delenv("GLM_API_KEY", raising=False) + _register_fake_bulk_source(lambda _home: "vault-value") + + env_loader.load_hermes_dotenv(hermes_home=home) + assert env_loader.get_secret_source_values(home) == {"GLM_API_KEY": "vault-value"} + + # cron per-fire / plugin-discovery re-pull: reset + reload with the key now shadowing itself. + env_loader.reset_secret_source_cache() + env_loader.load_hermes_dotenv(hermes_home=home) + + assert str(home.resolve()) in env_loader._APPLIED_HOMES + assert env_loader.hydrate_profile_secret_sources(home) == {"GLM_API_KEY": "vault-value"} + assert build_profile_secret_scope(home)["GLM_API_KEY"] == "vault-value" + + +def test_home_scoped_reset_preserves_sibling_snapshot(tmp_path, monkeypatch, _fresh_registry): + """A cron fire / discovery refresh for one profile resets only THAT home: a multiplex sibling's + hydrated snapshot stays intact instead of running empty until it re-hydrates.""" + home = tmp_path / ".hermes" + sibling = home / "profiles" / "b" + sibling.mkdir(parents=True) + for h in (home, sibling): + (h / "config.yaml").write_text("secrets:\n fakebulk:\n enabled: true\n", encoding="utf-8") + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.delenv("GLM_API_KEY", raising=False) + _register_fake_bulk_source(lambda h: f"vault-{h.name}") + + env_loader.load_hermes_dotenv(hermes_home=home) + assert env_loader.hydrate_profile_secret_sources(sibling) == {"GLM_API_KEY": "vault-b"} + + env_loader.reset_secret_source_cache(home) + + assert env_loader.get_secret_source_values(home) == {} + assert env_loader.get_secret_source_values(sibling) == {"GLM_API_KEY": "vault-b"} + assert str(sibling.resolve()) in env_loader._APPLIED_HOMES diff --git a/website/docs/developer-guide/secret-source-plugin.md b/website/docs/developer-guide/secret-source-plugin.md index 3c0dd465e2..b6e5f93da7 100644 --- a/website/docs/developer-guide/secret-source-plugin.md +++ b/website/docs/developer-guide/secret-source-plugin.md @@ -143,7 +143,7 @@ def register(ctx): Registration is rejected (with a log warning, never a crash) for: non-`SecretSource` instances, invalid/duplicate names, a `scheme` another source owns, wrong `api_version`, or a `shape` outside `mapped`/`bulk`. :::note Timing -Plugin discovery runs later in startup than the first `load_hermes_dotenv()` call. Immediately after discovery, Hermes re-pulls enabled plugin secret sources (`reset_secret_source_cache()` + `load_hermes_dotenv()`), so the discovering process *does* pick them up — see [First-process bootstrap timing](#first-process-bootstrap-timing) above (#64177). The re-pull is fail-open and skipped when no plugin source is enabled. Any code that reads `os.environ` during the plugin module's import or `register(ctx)` still runs before the re-pull and cannot depend on credentials supplied by that same source; keep credentialed work inside `fetch()`. Gateway, cron, and subagent processes perform the same discovery/re-pull sequence. +Plugin discovery runs later in startup than the first `load_hermes_dotenv()` call. Immediately after discovery, Hermes re-pulls enabled plugin secret sources (`reset_secret_source_cache()` + `load_hermes_dotenv()`), so the discovering process *does* pick them up — see [First-process bootstrap timing](#first-process-bootstrap-timing) above (#64177). The re-pull is fail-open and skipped when no plugin source is enabled. Any code that reads `os.environ` during the plugin module's import or `register(ctx)` still runs before the re-pull and cannot depend on credentials supplied by that same source; keep credentialed work inside `fetch()`. Gateway, cron, and subagent processes perform the same discovery/re-pull sequence. The re-pull (and the per-fire cron re-pull) resets only the resolving home's cache, so under a multiplex gateway sibling profiles keep their hydrated snapshots; and a re-pull whose keys already sit in the process environment (`skipped_existing`, e.g. the previous apply's own write-back) still records the home's effective values, so `override_existing` is never required just to survive a re-pull. ::: ## Users configure it like any other source