diff --git a/cron/scheduler.py b/cron/scheduler.py index 2b07b2cc03..33635215c2 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -3155,7 +3155,25 @@ def _launch_external_cron_worker(job: dict) -> bool: ownership handoff: in a transient user scope, or — when no user D-Bus session exists and ``cron.require_restart_safe_scope`` is false — as a direct subprocess (process separation kept, cgroup isolation lost). + + A fire routed to a profile other than the process's own is multiplexed at THIS boundary too. + ``run_one_job`` switches the context on in ``_install_fire_secret_scope``, which runs AFTER + this handoff, so a routed desktop fire on the managed path serialized ``multiplex_active=False`` + and built the worker environment with the launch profile's residue and no scrub (review on + f5f88d5058). Enable it for exactly this span; the worker then re-establishes it from the payload. """ + from agent.secret_scope import is_multiplex_active, reset_multiplex_context, set_multiplex_context + from cron.scheduler_provider import routed_profile_fire + + context_token = set_multiplex_context(True) if routed_profile_fire() and not is_multiplex_active() else None + try: + return _launch_external_cron_worker_inner(job) + finally: + if context_token is not None: + reset_multiplex_context(context_token) + + +def _launch_external_cron_worker_inner(job: dict) -> bool: execution_id = str(job["execution_id"]) job_id = str(job["id"]) handoff_dir = _get_hermes_home() / "cron" / "external-workers" @@ -3178,7 +3196,7 @@ def _launch_external_cron_worker(job: dict) -> bool: set_secret_scope, ) from hermes_cli.env_loader import hydrate_profile_secret_sources - from tools.environments.local import build_subprocess_env, strip_launch_profile_env + from tools.environments.local import build_subprocess_env, restore_managed_env, strip_launch_profile_env from tools.process_registry import ( restart_safe_gateway_child_argv, systemd_user_bus_env, @@ -3230,11 +3248,11 @@ def _launch_external_cron_worker(job: dict) -> bool: hydrate_profile_secret_sources(profile_home) secret_token = set_secret_scope(build_profile_secret_scope(profile_home)) try: - worker_env = strip_launch_profile_env(build_subprocess_env( + worker_env = restore_managed_env(strip_launch_profile_env(build_subprocess_env( scrub_secrets=multiplex_active, inherit_profile_home=True, extra={"HERMES_HOME": str(profile_home)}, - )) + ))) finally: reset_secret_scope(secret_token) worker_env = systemd_user_bus_env(worker_env) diff --git a/cron/scheduler_script.py b/cron/scheduler_script.py index 93e83a012e..2d8a7e6bfe 100644 --- a/cron/scheduler_script.py +++ b/cron/scheduler_script.py @@ -358,7 +358,7 @@ def _run_job_script( # fires; the parent process is never mutated. from agent.secret_scope import _is_global_env, current_secret_scope, is_multiplex_active from hermes_cli.env_loader import secret_source_names - from tools.environments.local import strip_launch_profile_env + from tools.environments.local import restore_managed_env, strip_launch_profile_env base = strip_launch_profile_env(dict(os.environ)) # strip_launch_profile_env only knows dotenv- and terminal-config-owned names. External # secret sources (vault, 1Password, ...) also write their names into the shared os.environ, @@ -375,6 +375,9 @@ def _run_job_script( scope = current_secret_scope() if scope: base.update(scope) + # Administrator-managed values keep their precedence over the routed profile's own .env, exactly + # as they do in the launch process (``_apply_managed_env`` applies them last, with override). + restore_managed_env(base) env = build_subprocess_env(base=base) env.update(env_overlay) # Subprocess cwd only (default: scripts-dir parent). NEVER os.chdir() the process. diff --git a/hermes_cli/env_loader.py b/hermes_cli/env_loader.py index 9ef1d45aa8..a5b0cbfe32 100644 --- a/hermes_cli/env_loader.py +++ b/hermes_cli/env_loader.py @@ -38,6 +38,10 @@ _SOURCE_SUPPLIED_NAMES: set[str] = set() # 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() +# KEY names loaded from the administrator-managed ``.env`` (``_apply_managed_env``). Kept OUT of the launch +# residue: those values are policy that beats the user's own ``.env`` for every profile, so a routed child +# must keep them — and keep them LAST, over the routed profile's scope (review on f5f88d5058). +_MANAGED_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 @@ -94,11 +98,17 @@ def secret_source_names() -> tuple[str, ...]: def launch_dotenv_keys() -> frozenset[str]: - """KEY names any dotenv file loaded into this process's ``os.environ`` so far (see + """KEY names any NON-managed 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 managed_dotenv_keys() -> frozenset[str]: + """KEY names the administrator-managed ``.env`` loaded (see ``_MANAGED_DOTENV_KEYS``). Policy for + every profile: never stripped from a routed child, and re-applied over the routed scope.""" + return frozenset(_MANAGED_DOTENV_KEYS) + + def get_secret_source_values(hermes_home: str | os.PathLike) -> dict[str, str]: """Return the external-secret value snapshot for ``hermes_home``.""" return dict(_SECRET_SOURCE_VALUES_BY_HOME.get(str(Path(hermes_home).resolve()), {})) @@ -157,6 +167,13 @@ def _hydrate_profile_secret_sources(home: Path) -> dict[str, str]: # mixed report are still snapshotted below and can be used while the failed source recovers. if all(src.result.ok for src in report.sources): _APPLIED_HOMES.add(home_key) + # Same ownership bookkeeping as the process-global path: a name this profile's source supplied — applied, + # or skipped because the private mapping already had it — is a source-owned name the routed-child scrub + # must know about, or a sibling still inherits the launch value for it (review on f5f88d5058). + supplied = set(report.provenance) + for src in report.sources: + supplied.update(src.skipped_existing) + _SOURCE_SUPPLIED_NAMES.update(supplied) values: dict[str, str] = {} for name, applied in report.provenance.items(): value = local_env.get(name) @@ -253,7 +270,7 @@ def _sanitize_loaded_credentials() -> None: ) -def _load_dotenv_with_fallback(path: Path, *, override: bool) -> None: +def _load_dotenv_with_fallback(path: Path, *, override: bool, managed: bool = False) -> None: try: # utf-8-sig strips a leading BOM (PowerShell 5.1 / Notepad); plain utf-8 would keep U+FEFF on the # first key name and silently drop it from os.environ under its canonical name. @@ -263,8 +280,9 @@ 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)) + # Same scanner both branches: it re-reads the file with the same latin-1 fallback. Managed keys are + # recorded separately: they are administrator policy, not launch-profile residue. + (_MANAGED_DOTENV_KEYS if managed else _LOADED_DOTENV_KEYS).update(_env_keys_defined_in_dotenv(path)) _sanitize_loaded_credentials() # httpx encodes headers as ASCII @@ -458,7 +476,7 @@ def _apply_managed_env() -> None: if not managed_env.exists(): return _sanitize_env_file_if_needed(managed_env) - _load_dotenv_with_fallback(managed_env, override=True) + _load_dotenv_with_fallback(managed_env, override=True, managed=True) def _apply_external_secret_sources(home_path: Path) -> None: diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index 2b280cbe03..553f80a73b 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -1295,7 +1295,9 @@ class PluginManager(PluginLoaderMixin, PluginDispatchMixin, PluginLedgerMixin): # 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 + # The marker is cleared only AFTER the cleanup below succeeds: reset/reload/refresh are + # fallible, and clearing first left the stale credential active with no retry on the next + # discovery (review on f5f88d5058). else: self._plugin_secret_sources_reconciled = True try: @@ -1312,6 +1314,8 @@ class PluginManager(PluginLoaderMixin, PluginDispatchMixin, PluginLedgerMixin): # into the installed scope or THIS fire never sees the plugin credential. from agent.secret_scope import refresh_installed_secret_scope refresh_installed_secret_scope(Path(home)) + if not enabled_names: + self._plugin_secret_sources_reconciled = False # cleanup succeeded; nothing left to drop logger.debug("Re-applied secret sources after plugin discovery for: %s", ", ".join(sorted(enabled_names)) or "") except Exception as exc: diff --git a/tests/agent/test_env_loader_secret_sources.py b/tests/agent/test_env_loader_secret_sources.py index c1984db591..31befe867f 100644 --- a/tests/agent/test_env_loader_secret_sources.py +++ b/tests/agent/test_env_loader_secret_sources.py @@ -543,6 +543,46 @@ def test_apply_external_secret_sources_status_line_suppresses_secret_names( assert "LEAK_THIS_TOKEN" not in err +def test_private_hydration_records_skipped_existing_names_for_the_routed_scrub(tmp_path, monkeypatch): + """The private (routed-profile) hydration path must feed ``secret_source_names()`` like the + process-global path does — including ``skipped_existing`` — or a name first observed through + ``hydrate_profile_secret_sources()`` is invisible to the routed-child scrub and a sibling inherits + the ambient launch value for it (#107695 review on f5f88d5058).""" + from agent.secret_sources import registry as reg_module + from agent.secret_sources.base import FetchResult + from agent.secret_sources.registry import AppliedVar, ApplyReport, SourceReport + + home = tmp_path / "profile-b" + home.mkdir() + (home / "config.yaml").write_text("secrets:\n test-source:\n enabled: true\n", encoding="utf-8") + 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", {}) + + report = ApplyReport( + sources=[SourceReport(name="test-source", label="Test Source", result=FetchResult(), + applied=["APPLIED_SECRET"], skipped_existing=["CUSTOM_SOURCE_SECRET"])], + provenance={"APPLIED_SECRET": AppliedVar(name="APPLIED_SECRET", source="test-source", + shape="mapped", overrode_env=False)}, + ) + + def _fake_apply_all(_cfg, home_path, environ=None): + if environ is not None: + environ["APPLIED_SECRET"] = "applied-b" + return report + + monkeypatch.setattr(reg_module, "apply_all", _fake_apply_all) + + env_loader.hydrate_profile_secret_sources(home) + + names = set(env_loader.secret_source_names()) + assert {"APPLIED_SECRET", "CUSTOM_SOURCE_SECRET"} <= names + # provenance stays honest: only the APPLIED name carries a source label + assert env_loader.get_secret_source("APPLIED_SECRET") == "test-source" + assert env_loader.get_secret_source("CUSTOM_SOURCE_SECRET") is None + + def test_external_secret_values_are_isolated_between_homes(tmp_path, monkeypatch): """A later apply for the same key must not mutate an earlier home snapshot.""" from agent.secret_scope import build_profile_secret_scope diff --git a/tests/cron/test_cron_no_agent.py b/tests/cron/test_cron_no_agent.py index 5d017a6831..fd64bd9d1e 100644 --- a/tests/cron/test_cron_no_agent.py +++ b/tests/cron/test_cron_no_agent.py @@ -559,6 +559,86 @@ def test_a_routed_profile_script_never_receives_a_launch_source_value_that_lost_ assert os.environ["CUSTOM_VAULT_SECRET"] == "launch-value" # parent untouched +def test_a_routed_profile_script_keeps_administrator_managed_values_over_its_own(hermes_env, monkeypatch): + """Managed-scope precedence (#107695 review on f5f88d5058): the administrator's managed ``.env`` is + applied LAST with override in the launch process, so it beats the user's own ``.env``. Recording its + keys as launch residue stripped ``ORG_POLICY_FLAG`` before the routed overlay, and the routed + profile's own value replaced policy. Managed keys are not residue, and they are re-applied over the + routed scope so the child sees the same precedence the launch process does.""" + import os + + from agent import secret_scope + from cron.scheduler_script import _run_job_script + from hermes_cli import env_loader, managed_scope + from hermes_constants import get_process_hermes_home, reset_hermes_home_override, set_hermes_home_override + + launch = get_process_hermes_home() + managed = launch / "managed" + managed.mkdir() + (managed / ".env").write_text("ORG_POLICY_FLAG=managed-value\n", encoding="utf-8") + monkeypatch.setattr(env_loader, "_LOADED_DOTENV_KEYS", set(env_loader._LOADED_DOTENV_KEYS)) + monkeypatch.setattr(env_loader, "_MANAGED_DOTENV_KEYS", set()) + monkeypatch.setattr(managed_scope, "get_managed_dir", lambda: managed) + monkeypatch.setenv("ORG_POLICY_FLAG", "placeholder") + env_loader._apply_managed_env() # the boot-time managed load + assert os.environ["ORG_POLICY_FLAG"] == "managed-value" + assert "ORG_POLICY_FLAG" in env_loader.managed_dotenv_keys() + assert "ORG_POLICY_FLAG" not in env_loader.launch_dotenv_keys() + + routed = launch / "profiles" / "ops" + (routed / "scripts").mkdir(parents=True, exist_ok=True) + script = routed / "scripts" / "probe_policy.sh" + script.write_text('#!/bin/bash\necho "${ORG_POLICY_FLAG:-}"\n') + + home_token = set_hermes_home_override(str(routed)) + context_token = secret_scope.set_multiplex_context(True) + # The routed user's own .env carries a competing value for the managed key. + scope_token = secret_scope.set_secret_scope({"ORG_POLICY_FLAG": "user-value"}) + try: + ok, output = _run_job_script("probe_policy.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() == "managed-value" + assert os.environ["ORG_POLICY_FLAG"] == "managed-value" # parent untouched + + +def test_strip_launch_profile_env_never_treats_managed_keys_as_residue(hermes_env, monkeypatch): + """The exclusion stands on its own (#107695 review on f5f88d5058): ``kanban_db_dispatch`` and + ``scheduler_delivery`` strip and spawn ``hermes -p `` with NO scope overlay and no managed + re-apply afterwards, so for them the strip itself must leave administrator-managed keys in place. + + The case that matters is a key defined in BOTH the user's launch ``.env`` and the managed ``.env`` — + the precedence conflict managed override exists for. That key IS launch residue by every other rule + (it is in the launch file and was recorded as loaded), and only the managed exclusion keeps the + policy value in the child. A launch-only recorded key is still removed.""" + from agent import secret_scope + from hermes_cli import env_loader + from hermes_constants import get_process_hermes_home, reset_hermes_home_override, set_hermes_home_override + from tools.environments.local import strip_launch_profile_env + + launch = get_process_hermes_home() + routed = launch / "profiles" / "ops" + routed.mkdir(parents=True, exist_ok=True) + # The user's own .env ALSO sets ORG_POLICY_FLAG; the managed .env overrode it at boot. + (launch / ".env").write_text("ORG_POLICY_FLAG=user-value\n", encoding="utf-8") + monkeypatch.setattr(env_loader, "_LOADED_DOTENV_KEYS", {"LAUNCH_ONLY_RECORDED", "ORG_POLICY_FLAG"}) + monkeypatch.setattr(env_loader, "_MANAGED_DOTENV_KEYS", {"ORG_POLICY_FLAG"}) + + home_token = set_hermes_home_override(str(routed)) + context_token = secret_scope.set_multiplex_context(True) + try: + env = strip_launch_profile_env({"ORG_POLICY_FLAG": "managed-value", "LAUNCH_ONLY_RECORDED": "stale"}) + finally: + secret_scope.reset_multiplex_context(context_token) + reset_hermes_home_override(home_token) + + assert env == {"ORG_POLICY_FLAG": "managed-value"} + + 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/cron/test_restart_safe_worker.py b/tests/cron/test_restart_safe_worker.py index b7e55bbf14..8b9032d1da 100644 --- a/tests/cron/test_restart_safe_worker.py +++ b/tests/cron/test_restart_safe_worker.py @@ -258,6 +258,49 @@ def _stub_external_worker_launch(scheduler, monkeypatch): return spawned, payloads, handoff, get +def test_launch_external_worker_treats_a_routed_fire_as_multiplexed(tmp_path, monkeypatch): + """A fire routed to another profile is multiplexed at the handoff boundary (#107695 review on + f5f88d5058). ``run_one_job`` only enables the context in ``_install_fire_secret_scope``, which runs + AFTER this handoff, so a routed desktop fire on the managed path serialized ``multiplex_active=False`` + and the worker inherited the launch profile's residue. The payload must carry ``True`` and the + worker env must not carry a launch-only value — and the context must not outlive the handoff.""" + import cron.scheduler as scheduler + import hermes_constants + from agent import secret_scope + from hermes_constants import reset_hermes_home_override, set_hermes_home_override + from tools.process_registry import GatewayChildDispatch + + launch = tmp_path / "launch" + routed = tmp_path / "routed" + launch.mkdir() + routed.mkdir() + (launch / ".env").write_text("LAUNCH_ONLY_SECRET=launch-secret\n", encoding="utf-8") + (routed / ".env").write_text("", encoding="utf-8") + monkeypatch.setenv("LAUNCH_ONLY_SECRET", "launch-secret") + monkeypatch.setattr(scheduler, "_get_hermes_home", lambda: routed) + monkeypatch.setattr(hermes_constants, "get_process_hermes_home", lambda: launch) + monkeypatch.setattr("cron.scheduler_provider.routed_profile_fire", lambda: True) + monkeypatch.setattr( + "tools.process_registry.restart_safe_gateway_child_argv", + lambda command, *, unit_suffix, require_restart_safe_scope=False: GatewayChildDispatch( + "scoped", ["scope", "--", *command]), + ) + spawned, payloads, _handoff, _get = _stub_external_worker_launch(scheduler, monkeypatch) + + assert not secret_scope.is_multiplex_active() # the desktop tick itself is NOT a multiplexer + home_token = set_hermes_home_override(str(routed)) + try: + assert scheduler._launch_external_cron_worker( + {"id": "job-r", "execution_id": "exec-1", "prompt": "work"}) is True + finally: + reset_hermes_home_override(home_token) + + assert payloads[0]["multiplex_active"] is True + assert "LAUNCH_ONLY_SECRET" not in spawned[0][1]["env"] + assert not secret_scope.is_multiplex_active() # enabled for the handoff span only + assert os.environ["LAUNCH_ONLY_SECRET"] == "launch-secret" # parent untouched + + def test_launch_external_worker_uses_restart_safe_scope_and_acknowledges( tmp_path, monkeypatch ): diff --git a/tests/hermes_cli/test_secret_source_bootstrap.py b/tests/hermes_cli/test_secret_source_bootstrap.py index 71d5337544..efb8e8a127 100644 --- a/tests/hermes_cli/test_secret_source_bootstrap.py +++ b/tests/hermes_cli/test_secret_source_bootstrap.py @@ -135,6 +135,48 @@ def test_refresh_reconciles_once_when_the_last_plugin_source_is_removed(monkeypa assert called == {"reset": 2, "load": 2, "scope": 2} # and not again: nothing left to reconcile +def test_refresh_retries_removal_cleanup_after_a_failed_attempt(monkeypatch): + """The reconcile marker must survive a failed cleanup (#107695 review on f5f88d5058): clearing it + before the fallible reset/reload/refresh left the removed plugin's credential active while every + later no-source discovery returned early. It clears only once cleanup succeeds.""" + mgr = PluginManager() + calls = {"load": 0} + fail = {"on": True} + + 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: None) + monkeypatch.setattr("agent.secret_scope.refresh_installed_secret_scope", lambda *a, **kw: True) + + def _load(**kw): + calls["load"] += 1 + if fail["on"]: + raise RuntimeError("reload blew up") + + monkeypatch.setattr("hermes_cli.env_loader.load_hermes_dotenv", _load) + + fail["on"] = False + mgr._refresh_secret_sources_after_discovery() # enabled: marker set + assert calls["load"] == 1 + + sources.clear() + fail["on"] = True + mgr._refresh_secret_sources_after_discovery() # removal cleanup attempt fails + assert calls["load"] == 2 + assert mgr._plugin_secret_sources_reconciled is True # NOT cleared by a failed attempt + + fail["on"] = False + mgr._refresh_secret_sources_after_discovery() # retried, succeeds + assert calls["load"] == 3 + assert mgr._plugin_secret_sources_reconciled is False + + mgr._refresh_secret_sources_after_discovery() # nothing left to reconcile + assert calls["load"] == 3 + + 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 f8b0005ec7..dfedeaf782 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -354,17 +354,32 @@ 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 - from hermes_cli.env_loader import launch_dotenv_keys + from hermes_cli.env_loader import launch_dotenv_keys, managed_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). + # a re-parse of the file alone no longer names it (#107695 review). The administrator-managed .env + # is NOT residue: its values are policy for every profile (``_apply_managed_env`` applies it last, + # with override, so it beats the user's own .env) — leave them in place. residue = set(load_env_file(launch_home / ".env")) | set(launch_dotenv_keys()) | set(TERMINAL_CONFIG_ENV_MAP.values()) + residue -= set(managed_dotenv_keys()) for key in residue: if not _is_global_env(key) or key.startswith("TERMINAL_"): env.pop(key, None) return env +def restore_managed_env(env: dict) -> dict: + """Re-apply the administrator-managed ``.env`` values over *env* — call AFTER a routed profile's scope + has been overlaid. ``_apply_managed_env`` gives those keys precedence over the user's own ``.env`` in + the launch process; a routed child must see the same precedence, or the routed user's value for a + managed key (``ORG_POLICY_FLAG=user-value``) silently wins over policy.""" + from hermes_cli.env_loader import managed_dotenv_keys + for key in managed_dotenv_keys(): + if key in os.environ: + env[key] = os.environ[key] + return env + + # --- Shell discovery --- def _windows_bash_candidates(custom: "str | None") -> list[str]: """Ordered bash.exe candidates on Windows: HERMES_GIT_BASH_PATH, our portable Git