diff --git a/agent/delegation_context.py b/agent/delegation_context.py index 9b8bce97597a..9d30fe050eac 100644 --- a/agent/delegation_context.py +++ b/agent/delegation_context.py @@ -34,6 +34,17 @@ DELEGATED_CHILD_ENV_MARKER = "HERMES_DELEGATED_CHILD_CONTEXT" +# Caller contract for the two Kanban env helpers: +# * scrub_kanban_env — delegate_task children: strip + set the lineage +# marker so the child process (and ITS subprocesses) are recognized as +# delegated and keep the kanban tool fencing. +# * strip_kanban_env — plain nested spawns (terminal tool, execute_code) +# that must NOT inherit the parent worker's dispatcher identity, but are +# NOT delegated children: strip without the marker (#81508). + +# Historical keys remain public for callers/tests that need to enumerate the +# current contract. Enforcement below is deliberately prefix-based so a newly +# introduced dispatcher capability cannot leak before this tuple is updated. KANBAN_ENV_KEYS: tuple[str, ...] = ( "HERMES_KANBAN_TASK", "HERMES_KANBAN_RUN_ID", @@ -130,11 +141,24 @@ def is_delegated_child_process_context() -> bool: ) +def strip_kanban_env(env: Mapping[str, str] | MutableMapping[str, str]) -> dict[str, str]: + """Return *env* with dispatcher-only Kanban variables removed (no marker). + + Unlike :func:`scrub_kanban_env`, this does NOT set + ``HERMES_DELEGATED_CHILD_CONTEXT``: it is for nested spawns (terminal + tool, execute_code) that must simply not inherit the parent worker's + Kanban identity — they are not delegate_task children. + """ + return { + key: value + for key, value in env.items() + if not key.startswith("HERMES_KANBAN_") + } + + def scrub_kanban_env(env: Mapping[str, str] | MutableMapping[str, str]) -> dict[str, str]: """Return *env* with dispatcher-only Kanban variables removed.""" - cleaned = dict(env) - for key in KANBAN_ENV_KEYS: - cleaned.pop(key, None) + cleaned = strip_kanban_env(env) cleaned[DELEGATED_CHILD_ENV_MARKER] = "1" return cleaned diff --git a/tests/tools/test_code_execution_windows_env.py b/tests/tools/test_code_execution_windows_env.py index 2963e24fcf11..d80146c78c77 100644 --- a/tests/tools/test_code_execution_windows_env.py +++ b/tests/tools/test_code_execution_windows_env.py @@ -256,6 +256,14 @@ def _legacy_posix_scrubber(source_env, is_passthrough): _HERMES_CHILD_ALLOWED = frozenset({ "HERMES_HOME", "HERMES_PROFILE", "HERMES_CONFIG", "HERMES_ENV", }) + # Dispatcher-owned Kanban identity is stripped unconditionally from + # execute_code children (#81508) — even an explicit passthrough cannot + # re-grant a nested process the parent's board mutation capability. + _KANBAN_ENV_KEYS = frozenset({ + "HERMES_KANBAN_TASK", "HERMES_KANBAN_RUN_ID", "HERMES_KANBAN_WORKSPACE", + "HERMES_KANBAN_WORKSPACES_ROOT", "HERMES_KANBAN_CLAIM_LOCK", + "HERMES_KANBAN_BOARD", "HERMES_KANBAN_DB", + }) out = {} for k, v in source_env.items(): if is_passthrough(k): @@ -268,6 +276,8 @@ def _legacy_posix_scrubber(source_env, is_passthrough): continue if k in _HERMES_CHILD_ALLOWED: out[k] = v + for k in _KANBAN_ENV_KEYS: + out.pop(k, None) return out diff --git a/tests/tools/test_hermes_subprocess_env.py b/tests/tools/test_hermes_subprocess_env.py index 9838f10cb590..aec9e28cd55d 100644 --- a/tests/tools/test_hermes_subprocess_env.py +++ b/tests/tools/test_hermes_subprocess_env.py @@ -165,6 +165,30 @@ def test_delegated_child_context_scrubs_parent_kanban_keys_and_sets_marker(self) assert "HERMES_KANBAN_WORKSPACE" not in env assert env["MY_APP_VAR"] == "keep-me" + def test_non_delegated_worker_keeps_kanban_env_for_runtime(self): + """A worker's codex/ACP runtime subprocess keeps HERMES_KANBAN_*. + + ``hermes_subprocess_env`` feeds the codex app server and copilot ACP + runtimes, which are the worker's OWN execution surface — they must + retain ``HERMES_KANBAN_TASK`` so the runtime can write completion / + block back to the board (#81508 fixes the terminal-tool boundary; the + non-terminal runtime boundary is intentionally unchanged). + """ + env = _build( + { + "HERMES_KANBAN_TASK": "t_parent", + "HERMES_KANBAN_RUN_ID": "123", + "HERMES_KANBAN_DB": "/tmp/parent-kanban.db", + "HERMES_KANBAN_WORKSPACE": "/tmp/parent-workspace", + }, + inherit_credentials=True, + ) + assert env["HERMES_KANBAN_TASK"] == "t_parent" + assert env["HERMES_KANBAN_RUN_ID"] == "123" + assert env["HERMES_KANBAN_DB"] == "/tmp/parent-kanban.db" + # Plain (non-delegated) spawns must not receive the lineage marker. + assert env.get("HERMES_DELEGATED_CHILD_CONTEXT") is None + _INTERNAL_DYNAMIC_SAMPLE = { "AUXILIARY_VISION_API_KEY": "sk-vision", diff --git a/tests/tools/test_local_env_blocklist.py b/tests/tools/test_local_env_blocklist.py index 3431d63b4fb6..d1fe4db425da 100644 --- a/tests/tools/test_local_env_blocklist.py +++ b/tests/tools/test_local_env_blocklist.py @@ -722,3 +722,134 @@ def test_gateway_relay_static_names_in_blocklist(self): assert "GATEWAY_RELAY_SECRET" in _HERMES_PROVIDER_ENV_BLOCKLIST assert "GATEWAY_RELAY_DELIVERY_KEY" in _HERMES_PROVIDER_ENV_BLOCKLIST assert "GATEWAY_RELAY_ID" in _HERMES_PROVIDER_ENV_BLOCKLIST + + +class TestKanbanNestedSpawnScrub: + """Nested subprocesses must not inherit the parent worker's Kanban identity. + + A dispatcher-owned Kanban worker has HERMES_KANBAN_* in its own env, but a + nested ``hermes`` CLI it launches through the terminal tool inherits that + env and is accepted as the parent run owner — it can complete/block the + parent's card while the real worker is still running (#81508). The + terminal-tool spawn env must strip the dispatcher identity unconditionally, + not just for delegate_task children. + + See https://github.com/NousResearch/hermes-agent/issues/81508 + """ + + _WORKER_ENV = { + "HERMES_KANBAN_TASK": "t_6229de04", + "HERMES_KANBAN_RUN_ID": "71", + "HERMES_KANBAN_CLAIM_LOCK": "claim-lock-abc", + "HERMES_KANBAN_BOARD": "default", + "HERMES_KANBAN_DB": "/tmp/parent-kanban.db", + "HERMES_KANBAN_WORKSPACE": "/tmp/parent-workspace", + "HERMES_KANBAN_BRANCH": "wt/t_6229de04", + "HERMES_KANBAN_GOAL_MODE": "1", + "HERMES_KANBAN_GOAL_MAX_TURNS": "12", + "HERMES_KANBAN_WORKER_SCOPE": "lifecycle-only", + # Drift oracle: a future dispatcher key must be stripped without first + # being added to a hand-maintained allow/deny list. + "HERMES_KANBAN_FUTURE_CAPABILITY": "must-not-leak", + } + + def test_worker_terminal_foreground_spawn_strips_kanban_env(self): + """A worker's foreground terminal command must not see HERMES_KANBAN_*.""" + result_env = _run_with_env(extra_os_env=dict(self._WORKER_ENV)) + for key in self._WORKER_ENV: + assert key not in result_env, f"{key} leaked into nested subprocess env" + + def test_worker_terminal_foreground_spawn_keeps_other_env(self): + """Non-Kanban vars still flow to the nested subprocess.""" + result_env = _run_with_env(extra_os_env={**self._WORKER_ENV, "MY_APP_VAR": "keep-me"}) + assert result_env.get("MY_APP_VAR") == "keep-me" + + def test_worker_terminal_foreground_spawn_not_marked_delegated_child(self): + """A plain nested spawn is NOT a delegate_task child — no lineage marker.""" + result_env = _run_with_env(extra_os_env=dict(self._WORKER_ENV)) + assert result_env.get("HERMES_DELEGATED_CHILD_CONTEXT") is None + + def test_worker_terminal_background_spawn_strips_kanban_env(self): + """The process_registry (background/PTY) path strips too. + + ``process_registry.spawn_local`` builds its env via + ``_sanitize_subprocess_env``, which must scrub the dispatcher identity + for every caller — the nested-CLI attack is not foreground-only. + """ + from tools.environments.local import _sanitize_subprocess_env + + with patch.dict(os.environ, {"PATH": "/usr/bin:/bin", **self._WORKER_ENV}, clear=True): + env = _sanitize_subprocess_env(dict(os.environ)) + for key in self._WORKER_ENV: + assert key not in env, f"{key} leaked via background spawn env" + + def test_worker_terminal_spawn_real_subprocess_sees_no_kanban_env(self): + """End-to-end: a REAL spawned child must not see HERMES_KANBAN_*. + + Unlike the mocked-Popen tests above, this spawns an actual + subprocess through ``_make_run_env`` and asserts the dispatcher + identity never crosses the process boundary (#81508). + """ + import subprocess + import sys + + from tools.environments.local import _make_run_env + + with patch.dict( + os.environ, + {"PATH": "/usr/bin:/bin", "HOME": "/tmp", **self._WORKER_ENV}, + clear=True, + ): + run_env = _make_run_env(dict(os.environ)) + + probe = ( + "import os;" + "leaked=[k for k in os.environ if k.startswith('HERMES_KANBAN_')];" + "print('LEAK:' + ','.join(sorted(leaked)) if leaked else 'CLEAN')" + ) + result = subprocess.run( + [sys.executable, "-c", probe], + env=run_env, + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, result.stderr + assert "LEAK:" not in result.stdout + # Plain nested spawns are not delegated children — no false marker. + assert "HERMES_DELEGATED_CHILD_CONTEXT" not in run_env + + def test_worker_execute_code_spawn_strips_kanban_env(self): + """execute_code sandbox children must not inherit Kanban identity. + + Regression for the sibling subprocess boundary: a worker's + execute_code child could otherwise spawn a nested hermes with full + board mutation capability (#81508). + """ + from tools.code_execution_tool import _scrub_child_env + + with patch.dict(os.environ, {"PATH": "/usr/bin:/bin", **self._WORKER_ENV}, clear=True): + env = _scrub_child_env( + dict(os.environ), + is_passthrough=lambda k: k.startswith("HERMES_KANBAN_"), + is_windows=False, + ) + for key in self._WORKER_ENV: + assert key not in env, f"{key} leaked into execute_code sandbox env" + # Passthrough must not re-grant it either. + assert env.get("HERMES_DELEGATED_CHILD_CONTEXT") is None + + def test_worker_execute_code_spawn_still_marks_delegated_children(self): + """Delegated children keep the lineage marker (existing behavior).""" + from agent.delegation_context import delegated_child_context + from tools.code_execution_tool import _scrub_child_env + + with patch.dict(os.environ, {"PATH": "/usr/bin:/bin", **self._WORKER_ENV}, clear=True): + with delegated_child_context(): + env = _scrub_child_env( + dict(os.environ), + is_passthrough=lambda k: k.startswith("HERMES_KANBAN_"), + is_windows=False, + ) + assert env["HERMES_DELEGATED_CHILD_CONTEXT"] == "1" + assert "HERMES_KANBAN_TASK" not in env diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index 0abde608e4f3..ed1ccb909ab8 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -282,21 +282,30 @@ def _scrub_child_env(source_env, is_passthrough=None, is_windows=None): ", ".join(sorted(_dropped_hermes)), ) - # delegate_task children are marked with a ContextVar, not os.environ, while - # the execute_code sandbox crosses a process boundary. Bridge that context - # into the child env and strip dispatcher-owned Kanban variables after the - # normal secret/passthrough scrub so an explicit passthrough cannot re-grant - # a delegated child the parent's board mutation capability. + # The execute_code sandbox crosses a process boundary. Strip dispatcher- + # owned Kanban variables after the normal secret/passthrough scrub so an + # explicit passthrough cannot re-grant a nested process (delegated child + # OR plain worker-spawned sandbox) the parent's board mutation capability + # (#81508). Delegated children additionally keep the lineage marker. + # Fail closed: on import failure the prefix sweep still strips the + # dispatcher identity. try: from agent.delegation_context import ( is_delegated_child_process_context, scrub_kanban_env, + strip_kanban_env, ) if is_delegated_child_process_context(): scrubbed = scrub_kanban_env(scrubbed) + else: + scrubbed = strip_kanban_env(scrubbed) except Exception: - pass + scrubbed = { + key: value + for key, value in scrubbed.items() + if not key.startswith("HERMES_KANBAN_") + } return scrubbed diff --git a/tools/environments/local.py b/tools/environments/local.py index 330d46c53dd6..74305e95a4b7 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -508,13 +508,62 @@ def _sanitize_subprocess_env(base_env: dict | None, extra_env: dict | None = Non _apply_windows_msys_bash_env_defaults(sanitized) - sanitized = _scrub_delegated_child_kanban_env(sanitized) + # Any subprocess spawned from a Kanban worker (terminal tool foreground / + # background / PTY, watchers, cua-driver) must not inherit the worker's + # dispatcher identity (#81508). The codex/ACP runtime keeps its env via + # hermes_subprocess_env, which calls the delegated-child-only scrub. + sanitized = _scrub_terminal_spawn_kanban_env(sanitized) return sanitized +def _scrub_terminal_spawn_kanban_env(env: dict[str, str]) -> dict[str, str]: + """Strip dispatcher-owned Kanban env from terminal-spawned subprocesses. + + A dispatcher-owned worker legitimately carries ``HERMES_KANBAN_*`` in its + own env, but any subprocess it spawns through the terminal tool (or the + execute_code sandbox) must NOT inherit that identity: a nested ``hermes`` + CLI would otherwise be accepted as the parent run owner and could + complete/block the parent's card (#81508). The dispatcher's own worker + spawn (``kanban_db._default_spawn``) builds its env explicitly, so it is + unaffected by this scrub. + + Delegated children additionally keep the lineage marker (strip + marker); + plain nested spawns are stripped without the marker — they are not + delegate_task children. + + Fail closed: if the delegation module cannot be imported, the + ``HERMES_KANBAN_*`` prefix sweep still strips the dispatcher identity — + the env must never reach a terminal child unscrubbed. + """ + try: + from agent.delegation_context import ( + is_delegated_child_process_context, + scrub_kanban_env, + strip_kanban_env, + ) + + if is_delegated_child_process_context(): + return scrub_kanban_env(env) + return strip_kanban_env(env) + except Exception: + # Fail-closed fallback: prefix sweep is robust to any future + # HERMES_KANBAN_* key without duplicating the canonical key list. + return { + key: value + for key, value in env.items() + if not key.startswith("HERMES_KANBAN_") + } + + def _scrub_delegated_child_kanban_env(env: dict[str, str]) -> dict[str, str]: - """Strip dispatcher-owned Kanban env from delegate_task child subprocesses.""" + """Strip dispatcher-owned Kanban env from delegate_task child subprocesses. + + Non-terminal spawn surface (browser, lazy-deps, TUI/ACP hosts, codex + runtime): only delegated children lose the Kanban identity. A codex-app- + server / ACP runtime subprocess of a worker legitimately needs + ``HERMES_KANBAN_TASK`` to write completion back to the board. + """ try: from agent.delegation_context import ( is_delegated_child_process_context, @@ -1323,7 +1372,10 @@ def _make_run_env(env: dict) -> dict: _apply_windows_msys_bash_env_defaults(run_env) - run_env = _scrub_delegated_child_kanban_env(run_env) + # Foreground terminal spawns from a Kanban worker must not inherit the + # worker's dispatcher identity: a nested `hermes` CLI launched through the + # terminal would otherwise be accepted as the parent run owner (#81508). + run_env = _scrub_terminal_spawn_kanban_env(run_env) return run_env