From 9d7de6c1401dcadc031bf0c8a655f0c64c3256c1 Mon Sep 17 00:00:00 2001 From: John Paul Soliva Date: Mon, 14 Sep 2026 02:50:20 +0900 Subject: [PATCH] fix(cron): close three launch-residue leaks into a routed no_agent child MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on d8c467f223, each reproduced through its production path. Stale launch key. `strip_launch_profile_env` built its residue set from a re-parse of the launch `.env`. A key removed or renamed in that file after boot is still in `os.environ` with the old value (dotenv never unsets), and the current file no longer names it, so it survived into the routed child. `_load_dotenv_with_fallback` — the one chokepoint every dotenv load goes through — now records the KEY names it put into the process env, additive for the process lifetime (`launch_dotenv_keys()`), and the strip unions that record with the current file. Source name that lost to the process env. `_apply_external_secret_sources` snapshots every name a source SUPPLIED (`provenance` + `skipped_existing`), but `secret_source_names()` only exposed `_SECRET_SOURCES`, which is provenance metadata and names applied values alone. A launch-profile source that supplied `CUSTOM_VAULT_SECRET` while the process already had it was therefore invisible to the scrub, and a routed child with an empty scope got the launch value. Supplied names are tracked separately (`_SOURCE_SUPPLIED_NAMES`) so the provenance labels stay honest, and `secret_source_names()` returns the union. Last plugin source removed. `_refresh_secret_sources_after_discovery` returned before the cache reset and the installed-scope refresh whenever no plugin source was enabled — and `discover_and_load(force=True)` unloads the old registration first, so removing the final plugin source hit exactly that return with the removed plugin's names still in the per-home snapshot and the current scope. The manager now remembers that a discovery re-applied plugin sources and, on the next discovery that finds none, reconciles once. A home that never had a plugin source is still a no-op (pinned by the existing tests). Regressions: the stale-key lifecycle and the skipped-existing case through `_run_job_script` against a real routed child, and the removal case through the manager. Each checked by reverting its fix and confirming the test fails. (cherry picked from commit d464f5f6126a394cfb47937f683d3a5e2f141840) --- hermes_cli/env_loader.py | 26 +++++- hermes_cli/plugins.py | 42 +++++---- tests/cron/test_cron_no_agent.py | 90 +++++++++++++++++++ .../test_secret_source_bootstrap.py | 32 +++++++ tools/environments/local.py | 7 +- 5 files changed, 179 insertions(+), 18 deletions(-) diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index 7c65fc68f2..9ef1d45aa8 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -28,6 +28,16 @@ _SCOPED_SKIP_LOGGED: set[str] = set() # routed profile homes whose multiplex d # env-var name → source label ("bitwarden", …) for externally injected credentials; setup / `hermes # model` tell users WHERE a key came from when .env lacks it. _SECRET_SOURCES: dict[str, str] = {} +# Every env-var name an external source SUPPLIED for some home, whether it was applied or lost to a +# pre-existing process value (``skipped_existing``). ``_SECRET_SOURCES`` is provenance metadata and only +# names applied values; the scrub that keeps a launch profile's source-supplied names out of a routed +# child must see the skipped ones too, or a name already in the process env leaks with the launch value. +_SOURCE_SUPPLIED_NAMES: set[str] = set() +# Every KEY name a dotenv file loaded into ``os.environ`` during this process's lifetime. A key removed +# or renamed in the launch ``.env`` after boot stays in ``os.environ`` (dotenv never unsets), but a +# re-parse of the current file no longer names it — so the launch-residue strip for a routed child must +# work from what was LOADED, not from what the file says now. Additive for the process lifetime. +_LOADED_DOTENV_KEYS: set[str] = set() # 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]] = {} # HERMES_HOME paths already pulled external secrets for: load_hermes_dotenv() runs at import time from @@ -77,8 +87,16 @@ def get_secret_source(env_var: str) -> str | None: def secret_source_names() -> tuple[str, ...]: """Every env-var name some profile's external secret source supplied (names only — the map is - process-wide, so a value must be resolved through the active profile's secret scope).""" - return tuple(_SECRET_SOURCES) + process-wide, so a value must be resolved through the active profile's secret scope). Includes names + the source supplied but a pre-existing process value won (``skipped_existing``): the launch value + in ``os.environ`` is still not a routed profile's to inherit.""" + return tuple(dict.fromkeys((*_SECRET_SOURCES, *sorted(_SOURCE_SUPPLIED_NAMES)))) + + +def launch_dotenv_keys() -> frozenset[str]: + """KEY names any dotenv file loaded into this process's ``os.environ`` so far (see + ``_LOADED_DOTENV_KEYS``); the launch profile's residue set for routed children.""" + return frozenset(_LOADED_DOTENV_KEYS) def get_secret_source_values(hermes_home: str | os.PathLike) -> dict[str, str]: @@ -160,6 +178,7 @@ def reset_secret_source_cache(hermes_home: str | os.PathLike | None = None) -> N if hermes_home is None: _APPLIED_HOMES.clear() _SECRET_SOURCES.clear() + _SOURCE_SUPPLIED_NAMES.clear() _SECRET_SOURCE_VALUES_BY_HOME.clear() return home_key = str(Path(hermes_home).resolve()) @@ -244,6 +263,8 @@ def _load_dotenv_with_fallback(path: Path, *, override: bool) -> None: if raw.startswith(codecs.BOM_UTF8): raw = raw[len(codecs.BOM_UTF8) :] load_dotenv(stream=io.StringIO(raw.decode("latin-1")), override=override) + # Same scanner both branches: it re-reads the file with the same latin-1 fallback. + _LOADED_DOTENV_KEYS.update(_env_keys_defined_in_dotenv(path)) _sanitize_loaded_credentials() # httpx encodes headers as ASCII @@ -501,6 +522,7 @@ def _apply_external_secret_sources(home_path: Path) -> None: supplied = set(report.provenance) for src in report.sources: supplied.update(src.skipped_existing) + _SOURCE_SUPPLIED_NAMES.update(supplied) for name in supplied: if name in os.environ: values[name] = os.environ[name] diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 58325e9843..2b280cbe03 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1129,6 +1129,10 @@ class PluginManager(PluginLoaderMixin, PluginDispatchMixin, PluginLedgerMixin): self.home_path = Path(self.scope_key) self._discovery_lock = threading.RLock() self._discovered: bool = False + # True once a discovery re-applied plugin secret sources for this home: the per-home snapshot and + # the installed scope may then hold plugin-supplied names, and a later discovery that finds NO + # enabled plugin source (plugin removed / disabled) must still reconcile once to drop them. + self._plugin_secret_sources_reconciled: bool = False self._cli_ref = None # Set by CLI after plugin discovery self._gateway_message_injector: tuple[object, Callable] | None = None self._context_engine = None # Set by a plugin via register_context_engine() @@ -1268,24 +1272,32 @@ class PluginManager(PluginLoaderMixin, PluginDispatchMixin, PluginLedgerMixin): plugin_sources = list_plugin_sources() except Exception: return - if not plugin_sources: - return - try: - from hermes_cli.config import load_config - secrets = (load_config() or {}).get("secrets") or {} - except Exception: - secrets = {} - - def _enabled(source) -> bool: - section = secrets.get(getattr(source, "name", "")) + enabled_names: list[str] = [] + if plugin_sources: try: - return bool(source.is_enabled(section if isinstance(section, dict) else {})) + from hermes_cli.config import load_config + secrets = (load_config() or {}).get("secrets") or {} except Exception: - return False # mirrors the orchestrator: a raising is_enabled() is skipped + secrets = {} - enabled_names = [getattr(s, "name", "") for s in plugin_sources if _enabled(s)] + def _enabled(source) -> bool: + section = secrets.get(getattr(source, "name", "")) + try: + return bool(source.is_enabled(section if isinstance(section, dict) else {})) + except Exception: + return False # mirrors the orchestrator: a raising is_enabled() is skipped + + enabled_names = [getattr(s, "name", "") for s in plugin_sources if _enabled(s)] if not enabled_names: - return + # Nothing enabled now. If an earlier discovery re-applied plugin sources for this home, the + # snapshot and installed scope still carry that plugin's names (force-reload unloads the + # registration first, so this is exactly the "last plugin source removed" path) — reconcile + # once so they drop out. A home that never had one stays a no-op: no re-pull, no re-load. + if not self._plugin_secret_sources_reconciled: + return + self._plugin_secret_sources_reconciled = False + else: + self._plugin_secret_sources_reconciled = True try: # 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 @@ -1301,7 +1313,7 @@ class PluginManager(PluginLoaderMixin, PluginDispatchMixin, PluginLedgerMixin): from agent.secret_scope import refresh_installed_secret_scope refresh_installed_secret_scope(Path(home)) logger.debug("Re-applied secret sources after plugin discovery for: %s", - ", ".join(sorted(enabled_names))) + ", ".join(sorted(enabled_names)) or "") except Exception as exc: logger.debug("secret source re-apply after discovery failed: %s", exc) diff --git a/tests/cron/test_cron_no_agent.py b/tests/cron/test_cron_no_agent.py index 39127cdf06..5d017a6831 100644 --- a/tests/cron/test_cron_no_agent.py +++ b/tests/cron/test_cron_no_agent.py @@ -469,6 +469,96 @@ def test_a_routed_profile_script_never_receives_a_launch_external_source_value(h assert os.environ["LAUNCH_VAULT_ONLY"] == "launch-vault-value" # parent untouched +def test_a_routed_profile_script_never_receives_a_launch_key_removed_from_dotenv_after_boot(hermes_env, monkeypatch): + """Lifecycle negative control (#107695 review): the launch profile's .env loaded ``STALE_LAUNCH_KEY`` + at boot, the operator then removed the key from the file, and the long-running process still holds + the old value in ``os.environ`` (dotenv never unsets). A re-parse of the CURRENT file no longer names + it, so a strip built from the file alone let the stale value reach a routed child. The strip must + work from every key any dotenv load put into the process env during its lifetime.""" + import os + + from agent import secret_scope + from cron.scheduler_script import _run_job_script + from hermes_cli import env_loader + from hermes_constants import get_process_hermes_home, reset_hermes_home_override, set_hermes_home_override + + launch = get_process_hermes_home() + monkeypatch.setattr(env_loader, "_LOADED_DOTENV_KEYS", set(env_loader._LOADED_DOTENV_KEYS)) + monkeypatch.setenv("STALE_LAUNCH_KEY", "placeholder") # so monkeypatch restores the parent env afterwards + (launch / ".env").write_text("STALE_LAUNCH_KEY=stale-launch-value\n", encoding="utf-8") + env_loader._load_dotenv_with_fallback(launch / ".env", override=True) # the boot-time load + assert os.environ["STALE_LAUNCH_KEY"] == "stale-launch-value" + (launch / ".env").write_text("# key removed after boot\n", encoding="utf-8") + + routed = launch / "profiles" / "ops" + (routed / "scripts").mkdir(parents=True, exist_ok=True) + script = routed / "scripts" / "probe_stale.sh" + script.write_text('#!/bin/bash\necho "${STALE_LAUNCH_KEY:-}"\n') + + home_token = set_hermes_home_override(str(routed)) + context_token = secret_scope.set_multiplex_context(True) + scope_token = secret_scope.set_secret_scope({}) + try: + ok, output = _run_job_script("probe_stale.sh") + finally: + secret_scope.reset_secret_scope(scope_token) + secret_scope.reset_multiplex_context(context_token) + reset_hermes_home_override(home_token) + + assert ok, output + assert output.strip() == "" + assert os.environ["STALE_LAUNCH_KEY"] == "stale-launch-value" # the parent process was not mutated + + +def test_a_routed_profile_script_never_receives_a_launch_source_value_that_lost_to_the_process_env(hermes_env, monkeypatch): + """A launch-profile source SUPPLIED ``CUSTOM_VAULT_SECRET`` but a pre-existing process value won + (``skipped_existing``), so it never entered the provenance map ``secret_source_names()`` used to be + built from — and the launch value reached a routed child with an empty scope (#107695 review). The + ownership set must include every source-supplied name, applied or skipped.""" + import os + + from agent import secret_scope + from agent.secret_sources import registry as reg_module + from agent.secret_sources.base import FetchResult + from agent.secret_sources.registry import ApplyReport, SourceReport + from cron.scheduler_script import _run_job_script + from hermes_cli import env_loader + from hermes_constants import get_process_hermes_home, reset_hermes_home_override, set_hermes_home_override + + launch = get_process_hermes_home() + (launch / "config.yaml").write_text("secrets:\n test-source:\n enabled: true\n", encoding="utf-8") + monkeypatch.setenv("CUSTOM_VAULT_SECRET", "launch-value") + monkeypatch.setattr(env_loader, "_SOURCE_SUPPLIED_NAMES", set()) + monkeypatch.setattr(env_loader, "_SECRET_SOURCES", {}) + monkeypatch.setattr(env_loader, "_APPLIED_HOMES", set()) + monkeypatch.setattr(env_loader, "_SECRET_SOURCE_VALUES_BY_HOME", {}) + monkeypatch.setattr(reg_module, "apply_all", lambda _cfg, home_path, **_kw: ApplyReport( + sources=[SourceReport(name="test-source", label="Test Source", result=FetchResult(), + applied=[], skipped_existing=["CUSTOM_VAULT_SECRET"])], + provenance={})) + env_loader._apply_external_secret_sources(launch) # the real registry path, source loses to the env + assert "CUSTOM_VAULT_SECRET" in env_loader.secret_source_names() + + routed = launch / "profiles" / "ops" + (routed / "scripts").mkdir(parents=True, exist_ok=True) + script = routed / "scripts" / "probe_skipped.sh" + script.write_text('#!/bin/bash\necho "${CUSTOM_VAULT_SECRET:-}"\n') + + home_token = set_hermes_home_override(str(routed)) + context_token = secret_scope.set_multiplex_context(True) + scope_token = secret_scope.set_secret_scope({}) + try: + ok, output = _run_job_script("probe_skipped.sh") + finally: + secret_scope.reset_secret_scope(scope_token) + secret_scope.reset_multiplex_context(context_token) + reset_hermes_home_override(home_token) + + assert ok, output + assert output.strip() == "" + assert os.environ["CUSTOM_VAULT_SECRET"] == "launch-value" # parent untouched + + def test_single_profile_child_keeps_its_own_external_source_value(hermes_env, monkeypatch): """No multiplexing: os.environ IS this profile's environment, so the source-name strip must not run at all — the child keeps its own vault value even if the per-home snapshot were missing.""" diff --git a/tests/hermes_cli/test_secret_source_bootstrap.py b/tests/hermes_cli/test_secret_source_bootstrap.py index 481b275859..71d5337544 100644 --- a/tests/hermes_cli/test_secret_source_bootstrap.py +++ b/tests/hermes_cli/test_secret_source_bootstrap.py @@ -103,6 +103,38 @@ def test_refresh_secret_sources_repulls_when_plugin_enabled(monkeypatch): assert called == {"reset": 1, "load": 1} +def test_refresh_reconciles_once_when_the_last_plugin_source_is_removed(monkeypatch): + """Removal regression (#107695 review): ``discover_and_load(force=True)`` unloads the old + registration first, so a discovery that finds no enabled plugin source used to return before the + cache reset and the installed-scope refresh — the per-home snapshot and the current scope kept the + removed plugin's names. After a discovery that DID re-apply plugin sources, the next one that finds + none must reconcile exactly once; a home that never had a plugin source stays a no-op.""" + mgr = PluginManager() + called = {"reset": 0, "load": 0, "scope": 0} + + import agent.secret_sources.registry as reg + + sources = [_StubSource()] + monkeypatch.setattr(reg, "list_plugin_sources", lambda: list(sources)) + monkeypatch.setattr("hermes_cli.config.load_config", lambda: {"secrets": {"myvault": {"enabled": True}}}) + monkeypatch.setattr("hermes_cli.env_loader.reset_secret_source_cache", + lambda *a, **kw: called.__setitem__("reset", called["reset"] + 1)) + monkeypatch.setattr("hermes_cli.env_loader.load_hermes_dotenv", + lambda **kw: called.__setitem__("load", called["load"] + 1)) + monkeypatch.setattr("agent.secret_scope.refresh_installed_secret_scope", + lambda *a, **kw: called.__setitem__("scope", called["scope"] + 1) or True) + + mgr._refresh_secret_sources_after_discovery() # plugin source present and enabled + assert called == {"reset": 1, "load": 1, "scope": 1} + + sources.clear() # the plugin is gone (force-reload unloaded it) + mgr._refresh_secret_sources_after_discovery() + assert called == {"reset": 2, "load": 2, "scope": 2} # reconciled once so its names drop out + + mgr._refresh_secret_sources_after_discovery() + assert called == {"reset": 2, "load": 2, "scope": 2} # and not again: nothing left to reconcile + + def test_refresh_respects_custom_is_enabled(monkeypatch): """A source with custom activation (no ``enabled`` key) is re-pulled.""" mgr = PluginManager() diff --git a/tools/environments/local.py b/tools/environments/local.py index b9ffb48aa5..f8b0005ec7 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -354,7 +354,12 @@ def strip_launch_profile_env(env: dict, target_home: "str | Path | None" = None) if Path(target).resolve() == launch_home.resolve(): return env from hermes_cli.config import TERMINAL_CONFIG_ENV_MAP - for key in set(load_env_file(launch_home / ".env")) | set(TERMINAL_CONFIG_ENV_MAP.values()): + from hermes_cli.env_loader import launch_dotenv_keys + # Current file AND every key any dotenv load put into os.environ this process lifetime: a key + # removed or renamed in the launch .env after boot is still in os.environ with the old value, and + # a re-parse of the file alone no longer names it (#107695 review). + residue = set(load_env_file(launch_home / ".env")) | set(launch_dotenv_keys()) | set(TERMINAL_CONFIG_ENV_MAP.values()) + for key in residue: if not _is_global_env(key) or key.startswith("TERMINAL_"): env.pop(key, None) return env