fix(cron): close three launch-residue leaks into a routed no_agent child
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)
This commit is contained in:
committed by
kshitij
parent
3dedff6a12
commit
9d7de6c140
@@ -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]
|
||||
|
||||
+27
-15
@@ -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 "<none — reconciled removed plugin sources>")
|
||||
except Exception as exc:
|
||||
logger.debug("secret source re-apply after discovery failed: %s", exc)
|
||||
|
||||
|
||||
@@ -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:-<unset>}"\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() == "<unset>"
|
||||
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:-<unset>}"\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() == "<unset>"
|
||||
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."""
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user