diff --git a/agentdescent/agents.py b/agentdescent/agents.py index ba5603b5..acfe23b5 100644 --- a/agentdescent/agents.py +++ b/agentdescent/agents.py @@ -28,10 +28,14 @@ import urllib.request from dataclasses import dataclass, field from typing import (Any, Callable, Dict, List, Mapping, Optional, Protocol, - Sequence, Tuple, runtime_checkable) + Sequence, runtime_checkable) from .evolvable import ContractError from ._subprocess import decode_output +# The host-session denylist is shared with the detached-run scrub in +# ``surface_common``: it used to be two lists, and the shorter one let +# OpenCode/OpenAI-Agents markers through (#1082). +from .surface_common import SESSION_MARKERS, is_session_marker Completion = Callable[[str], str] @@ -270,28 +274,6 @@ def __call__(self, prompt: str) -> str: ... def in_workspace(self, path: str) -> Completion: ... -#: Environment variables a host agent sets in its own session and its children -#: inherit. A worker that sees them believes it is *inside* that session: -#: Claude Code refuses to start, DSH loads the parent's profile, and a worker -#: whose host runs this package's MCP server would call it recursively. -SESSION_MARKERS: Tuple[str, ...] = ( - "CLAUDECODE", "CLAUDE_CODE_", "CLAUDE_CONFIG_DIR", "CODEX_", "DSH_", - "OPENAI_AGENT_", "MCP_", - # OpenCode reads three of these, and redirecting only the directory is not - # isolation: `OPENCODE_CONFIG` (a file) and `OPENCODE_CONFIG_CONTENT` - # (inline JSON) both win over `OPENCODE_CONFIG_DIR` -- measured, a config - # named by `OPENCODE_CONFIG` still supplied its MCP servers with the dir - # pointed at an empty directory. They are dropped by name rather than by an - # "OPENCODE_" prefix on purpose: `OPENCODE_API_KEY` is a provider - # credential, and a worker needs its keys. - "OPENCODE_CONFIG_DIR", "OPENCODE_CONFIG", "OPENCODE_CONFIG_CONTENT", - # Dropped so the redirect below can take: `setdefault` cannot override a - # value the parent already exported, and this one usually is exported. - # It is broader than the others -- every XDG-respecting tool the worker - # runs sees the workspace copy -- which is the point of an isolated worker. - "XDG_CONFIG_HOME", -) - #: Set for every worker so a tool the worker reaches (this package's own MCP #: server, when the plugin that hosts it is being evolved) can tell it is inside #: a run and refuse to start another. See ``agentdescent.mcp``. @@ -338,8 +320,7 @@ def worker_env(workspace: Optional[str], extra: Optional[Mapping[str, str]] = No the user's setup. """ env = {k: v for k, v in os.environ.items() - if not (isolate and any(k == m.rstrip("_") or k.startswith(m) - for m in SESSION_MARKERS))} + if not (isolate and is_session_marker(k))} env[NESTED_MARKER] = "1" if workspace: # `cwd=` moves the process, not `$PWD`, and a runtime that resolves its diff --git a/agentdescent/runstore.py b/agentdescent/runstore.py index 5c107b92..c8984950 100644 --- a/agentdescent/runstore.py +++ b/agentdescent/runstore.py @@ -51,6 +51,7 @@ from typing import Any, Dict, Iterator, List, Optional, Sequence from .evolution import EvolutionResult, RoundInfo +from .surface_common import scrub_host_env __all__ = [ "STATES", @@ -678,7 +679,12 @@ def launch(rd: RunDir, *, budget_usd: Optional[float] = None, argv += ["--budget", str(budget_usd)] if usd_per_call is not None: argv += ["--usd-per-call", str(usd_per_call)] - child_env = {**os.environ, "PYTHONPATH": _child_pythonpath(), **(env or {})} + # Scrub the merged dict, not just the ambient one: a caller-supplied + # ``env`` is spread last and could otherwise put ``MCP_*``/``DSH_*`` back + # into the run after the scrub (#1082). ``PYTHONPATH`` carries no marker + # prefix, so it survives either way. + child_env = scrub_host_env({**os.environ, "PYTHONPATH": _child_pythonpath(), + **(env or {})}) boot = subprocess.run([_python(), "-c", _BOOTSTRAP, rd.log_path, *argv], capture_output=True, text=True, cwd=rd.path, env=child_env) if boot.returncode != 0 or not boot.stdout.strip().isdigit(): diff --git a/agentdescent/surface_common.py b/agentdescent/surface_common.py index 3b3e82da..ec934c61 100644 --- a/agentdescent/surface_common.py +++ b/agentdescent/surface_common.py @@ -17,7 +17,8 @@ import json import os import tempfile -from typing import Any, Dict, Iterator, Mapping, Optional, Sequence, Type, TypeVar +from typing import (Any, Dict, Iterator, Mapping, Optional, Sequence, Tuple, + Type, TypeVar) __all__ = [ "SURFACE_SCHEMA_VERSION", @@ -38,6 +39,10 @@ "resolve_ref", "surface_allow_from_env", "surface_write_dirs_from_env", + "scrub_host_env", + "SESSION_MARKERS", + "DETACHED_KEEP_VARS", + "is_session_marker", ] SURFACE_SCHEMA_VERSION = 1 @@ -200,6 +205,89 @@ def surface_write_dirs_from_env(environ: Optional[Mapping[str, str]] = None return tuple(part.strip() for part in raw.split(",") if part.strip()) +#: Environment variables a host agent sets in its own session and its children +#: inherit. A worker that sees them believes it is *inside* that session: Claude +#: Code refuses to start, DSH loads the parent's profile, and a worker whose +#: host runs this package's MCP server would call it recursively (#1082). +#: +#: One tuple, in this leaf module, because there used to be two lists: the +#: candidate worker (:func:`agentdescent.agents.worker_env`) dropped these +#: eleven names while :func:`scrub_host_env` dropped the four prefixes +#: ``DSH_``/``CLAUDE``/``CODEX``/``MCP_``. Measured on that pair, five of the +#: eleven crossed the scrub -- ``OPENAI_AGENT_*``, ``OPENCODE_CONFIG``, +#: ``OPENCODE_CONFIG_CONTENT``, ``OPENCODE_CONFIG_DIR`` and ``XDG_CONFIG_HOME`` +#: -- and the two OpenCode config carriers are the ones measured to hand a +#: nested process the parent's MCP servers. The tuple lives here rather than in +#: ``agents`` because ``agents`` imports ``socket``/``threading``/``urllib`` +#: and every surface module imports this one. +SESSION_MARKERS: Tuple[str, ...] = ( + "CLAUDECODE", "CLAUDE_CODE_", "CLAUDE_CONFIG_DIR", "CODEX_", "DSH_", + "OPENAI_AGENT_", "MCP_", + # OpenCode reads three of these, and redirecting only the directory is not + # isolation: `OPENCODE_CONFIG` (a file) and `OPENCODE_CONFIG_CONTENT` + # (inline JSON) both win over `OPENCODE_CONFIG_DIR` -- measured, a config + # named by `OPENCODE_CONFIG` still supplied its MCP servers with the dir + # pointed at an empty directory. They are dropped by name rather than by an + # "OPENCODE_" prefix on purpose: `OPENCODE_API_KEY` is a provider + # credential, and a worker needs its keys. + "OPENCODE_CONFIG_DIR", "OPENCODE_CONFIG", "OPENCODE_CONFIG_CONTENT", + # Dropped so a redirect can take: `setdefault` cannot override a value the + # parent already exported, and this one usually is exported. It is broader + # than the others -- every XDG-respecting tool the child runs sees the + # workspace copy -- which is the point of an isolated worker. + "XDG_CONFIG_HOME", +) + + +#: The host config/home directories a *detached run* keeps (#1082 review). +#: +#: The candidate worker redirects these into its own workspace +#: (:func:`agentdescent.agents.worker_env`), but the detached run is this +#: package: ``integrations._host_root`` reads all four so a worker spec with +#: ``isolate: false`` -- and every ``agentdescent start`` from a plain terminal +#: -- resolves the user's configured home. Dropping them sent such a run to +#: ``~/.claude`` / ``~/.codex`` / ``~/.config``, which for a custom directory is +#: the wrong, often unauthenticated, login. ``XDG_CONFIG_HOME`` is the one +#: OpenCode actually follows, which is why it is here too. The names are listed +#: here, not imported from ``agents``, because this module stays a leaf; the test +#: ``test_the_keep_set_covers_every_variable_host_root_reads`` reads the names +#: out of ``integrations``' own source, so a fifth host root fails it. +DETACHED_KEEP_VARS: Tuple[str, ...] = ( + "CLAUDE_CONFIG_DIR", "CODEX_HOME", "DSH_HOME", "XDG_CONFIG_HOME", +) + + +def is_session_marker(name: str) -> bool: + """Whether ``name`` belongs to a host agent's session rather than to the run. + + ``CLAUDECODE`` is a bare name, not a prefix, so a marker matches when the + variable is the marker without its trailing underscore, or starts with the + marker itself. + """ + upper = name.upper() + return any(upper == marker.rstrip("_") or upper.startswith(marker) + for marker in SESSION_MARKERS) + + +def scrub_host_env(environ: Optional[Mapping[str, str]] = None) -> Dict[str, str]: + """Drop the host session's markers and MCP config from a child environment. + + The candidate's gates run under an allowlist (:func:`runners._child_env`), + but a detached run is our own process: it needs the API keys and ``PATH`` + that an allowlist would drop. A denylist of the host-session variables keeps + the credentials while the worker's identity -- and any MCP config that would + hand the child a second set of tools -- stays with the host (#1082). + + The list is :data:`SESSION_MARKERS`, the same one the candidate worker + drops, so the two cannot drift apart again. The exception is + :data:`DETACHED_KEEP_VARS`: those three name the host's *configuration*, not + its session, and this run resolves a host's real home from them. + """ + env = dict(os.environ if environ is None else environ) + return {name: value for name, value in env.items() + if name.upper() in DETACHED_KEEP_VARS or not is_session_marker(name)} + + def _allowed_module(module_name: str, allow: Optional[Sequence[str]]) -> bool: if allow is None: return True diff --git a/changelog.d/1082-host-env-scrub.md b/changelog.d/1082-host-env-scrub.md new file mode 100644 index 00000000..a15f05e1 --- /dev/null +++ b/changelog.d/1082-host-env-scrub.md @@ -0,0 +1,12 @@ +Fixed +A detached run no longer inherits the worker's own session: `runstore.launch` +drops every `SESSION_MARKERS` variable -- `DSH_*`, `CLAUDE*`, `CODEX*`, `MCP_*`, +the OpenCode config carriers and `XDG_CONFIG_HOME` -- before it starts the +grandchild, so the host worker's session identity and MCP server configuration +stay with the host while the credentials and `PATH` the run needs still pass +through (#1082). The list is the one the candidate worker already dropped +(`agentdescent.agents.worker_env`); it used to be a second, shorter copy, and +five of its eleven entries crossed the scrub. The host's config/home +directories (`CLAUDE_CONFIG_DIR`, `CODEX_HOME`, `DSH_HOME`, `XDG_CONFIG_HOME`) +are kept, because they name configuration rather than session and the run +resolves a host's real home from them (#1082). diff --git a/docs/dsh-super-agent.md b/docs/dsh-super-agent.md index d1ba8066..b149906f 100644 --- a/docs/dsh-super-agent.md +++ b/docs/dsh-super-agent.md @@ -106,6 +106,17 @@ your tree, under the operator's write allowlist). the Ticket. - `plan` is the budget line: it estimates cost without executing. Read it before `start`, and put the number in the Ticket. +- A detached run no longer inherits the worker session: `launch` drops every + host-session marker (`SESSION_MARKERS`: `DSH_*`, `CLAUDE*`, `CODEX*`, `MCP_*`, + the OpenCode config carriers, `XDG_CONFIG_HOME`) before it starts the + grandchild, so the worker's session identity and MCP config stay with the + host while the API keys the run needs are passed through (`#1082`). The list + is the one the candidate worker drops, not a second copy of it. The host's + config/home directories (`DETACHED_KEEP_VARS`: `CLAUDE_CONFIG_DIR`, + `CODEX_HOME`, `DSH_HOME`, `XDG_CONFIG_HOME`) are kept: they name + configuration, not session, and this run resolves a host's real home from + them for `isolate: false` workers and for `agentdescent start` from a + terminal. ## 6. Long runs, Reports and Attention diff --git a/tests/test_host_env_scrub.py b/tests/test_host_env_scrub.py new file mode 100644 index 00000000..30b646c9 --- /dev/null +++ b/tests/test_host_env_scrub.py @@ -0,0 +1,272 @@ +"""A detached run does not inherit the host session's identity or MCP config (#1082). + +The worker's own gates run under an allowlist (:func:`runners._child_env`), but +the run `launch` detaches is our process, so it needs the API keys and PATH the +allowlist would drop. The cure is a denylist -- ``SESSION_MARKERS``, the same +tuple the candidate worker drops -- so `DSH_*`, `CLAUDE*`, `CODEX*`, `MCP_*`, +the OpenCode config carriers and `XDG_CONFIG_HOME` stay with the host, and +everything else, including the credentials the run needs, is passed through. + +The parametrised row is the one that cannot go stale: it walks the tuple itself, +so a marker added for a new host is covered here the day it is added. +""" + +from __future__ import annotations + +import ast +import inspect +import os +import subprocess +from unittest import mock + +import pytest + +from agentdescent import integrations, runstore +from agentdescent.agents import SESSION_MARKERS as AGENTS_SESSION_MARKERS +from agentdescent.surface_common import DETACHED_KEEP_VARS, SESSION_MARKERS, scrub_host_env + +HOST_MARKERS = { + "DSH_SESSION_ID": "host-session", + "DSH_WORKSPACE": "/host/workspace", + "CLAUDE_CODE_ENTRYPOINT": "cli", + "CLAUDECODE": "1", + "CODEX_SANDBOX": "1", + "MCP_SERVERS": '{"filesystem": {}}', +} + +#: The host config/home directories the run itself reads +#: (``integrations._host_root``), so a worker spec with ``isolate: false`` -- and +#: every ``agentdescent start`` from a plain terminal -- resolves the user's real +#: setup instead of the default home (#1082 review). The fourth is +#: ``XDG_CONFIG_HOME``: OpenCode resolves its config under XDG, which is the +#: variable that actually moves it. +HOST_CONFIG_DIRS = { + "CLAUDE_CONFIG_DIR": "/home/someone/.claude-work", + "CODEX_HOME": "/home/someone/.codex-work", + "DSH_HOME": "/home/someone/.dsh-work", + "XDG_CONFIG_HOME": "/home/someone/.config-work", +} + +#: One variable per marker, spelled the way a host would set it: the bare name +#: when the marker ends in ``_`` (``CLAUDECODE``), a suffixed one otherwise. +def _host_variable(marker: str) -> str: + return marker if marker.endswith("_") else marker + "_PROBE" + + +def test_scrub_drops_host_markers_and_keeps_what_the_run_needs(monkeypatch): + for name, value in HOST_MARKERS.items(): + monkeypatch.setenv(name, value) + monkeypatch.setenv("ARK_API_KEY", "not-a-real-key") + + env = scrub_host_env(os.environ) + + for name in HOST_MARKERS: + assert name not in env + assert env["ARK_API_KEY"] == "not-a-real-key" + assert env["PATH"] == os.environ["PATH"] + + +def _host_root_variables() -> set: + """The names ``integrations._host_root`` is actually called with. + + Read from the module's own source, so a fifth host root added there fails the + drift row below instead of silently resolving to its default directory. + """ + tree = ast.parse(inspect.getsource(integrations)) + names = set() + for node in ast.walk(tree): + if (isinstance(node, ast.Call) and isinstance(node.func, ast.Name) + and node.func.id == "_host_root" and node.args + and isinstance(node.args[0], ast.Constant) + and isinstance(node.args[0].value, str)): + names.add(node.args[0].value) + return names + + +def test_the_keep_set_covers_every_variable_host_root_reads(): + """The keep list is in the leaf module, so a test binds it to `integrations`. + + Both directions: a fifth host root fails the first assert, and a name the + worker's own map does not know fails the second -- so neither list can move + alone. + """ + from agentdescent.agents import WORKER_CONFIG_DIRS + + roots = _host_root_variables() + assert roots == {"DSH_HOME", "CODEX_HOME", "XDG_CONFIG_HOME", "CLAUDE_CONFIG_DIR"} + assert roots <= set(DETACHED_KEEP_VARS) + assert set(DETACHED_KEEP_VARS) <= set(WORKER_CONFIG_DIRS) + + +def test_every_host_root_resolves_under_the_scrubbed_environment(monkeypatch): + """After the scrub each host still finds the directory it was configured with.""" + for name, value in HOST_CONFIG_DIRS.items(): + monkeypatch.setenv(name, value) + + env = scrub_host_env(os.environ) + + with mock.patch.dict(os.environ, env, clear=True): + assert integrations._dsh_root("/home/someone", True) == HOST_CONFIG_DIRS["DSH_HOME"] + assert integrations._codex_root("/home/someone", True) == HOST_CONFIG_DIRS["CODEX_HOME"] + assert (integrations._claude_root("/home/someone", True) + == HOST_CONFIG_DIRS["CLAUDE_CONFIG_DIR"]) + assert (integrations._opencode_root("/home/someone", True) + == os.path.join(HOST_CONFIG_DIRS["XDG_CONFIG_HOME"], "opencode")) + + +def test_an_isolated_worker_still_loses_and_redirects_the_config_dirs(tmp_path, monkeypatch): + """The keep set is the detached run's; a candidate worker keeps none of it. + + Asserted as where each value points, not merely as absence: an absence-only + row would pass while the worker still pointed at the host's directory if the + redirect ever stopped taking (#1082 review). + """ + from agentdescent.agents import worker_env + + for name, value in HOST_CONFIG_DIRS.items(): + monkeypatch.setenv(name, value) + + env = worker_env(str(tmp_path), isolate=True) + expected = { + "CLAUDE_CONFIG_DIR": "claude", + "CODEX_HOME": "codex", + "DSH_HOME": "dsh", + "XDG_CONFIG_HOME": "xdg", + } + + for name, leaf in expected.items(): + assert env[name] == os.path.join(str(tmp_path), ".agentdescent-worker", leaf), name + + # `isolate=False` is the opposite contract: the worker sees the user's setup, + # which is why the detached run keeps these four in the first place. + loose = worker_env(str(tmp_path), isolate=False) + for name, value in HOST_CONFIG_DIRS.items(): + assert loose[name] == value, name + + +def test_the_host_config_dirs_survive_and_the_session_does_not(monkeypatch): + """The row the review named: the host's config/home dirs survive `launch`. + + `CLAUDE_CONFIG_DIR`, `CODEX_HOME` and `DSH_HOME` are host *configuration*, + which this package reads through `integrations._host_root`, not session + identity: dropping them sent a worker spec with `isolate: false` -- and every + `agentdescent start` from a terminal -- to `~/.claude` / `~/.codex`, which for + a custom directory is the wrong, often unauthenticated, login (#1082 review). + """ + for name, value in HOST_CONFIG_DIRS.items(): + monkeypatch.setenv(name, value) + for name, value in HOST_MARKERS.items(): + monkeypatch.setenv(name, value) + + env = scrub_host_env(os.environ) + + for name, value in HOST_CONFIG_DIRS.items(): + assert env[name] == value, name + for name in HOST_MARKERS: + assert name not in env, name + + +def test_launch_keeps_the_host_config_dirs_and_drops_the_session(tmp_path, monkeypatch): + for name, value in HOST_CONFIG_DIRS.items(): + monkeypatch.setenv(name, value) + monkeypatch.setenv("CLAUDECODE", "1") + monkeypatch.setenv("DSH_SESSION_ID", "host-session") + monkeypatch.setenv("MCP_SERVERS", '{"filesystem": {}}') + rd = runstore.create({"kind": "text", "target": str(tmp_path / "out.md")}, + store=str(tmp_path / "store")) + seen = {} + + def fake_run(argv, **kwargs): + seen["env"] = kwargs.get("env") + return subprocess.CompletedProcess(argv, 0, stdout="4242\n", stderr="") + + monkeypatch.setattr(runstore.subprocess, "run", fake_run) + + st = runstore.launch(rd) + + assert st.pid == 4242 + env = seen["env"] + for name, value in HOST_CONFIG_DIRS.items(): + assert env[name] == value, name + assert "CLAUDECODE" not in env + assert "DSH_SESSION_ID" not in env + assert "MCP_SERVERS" not in env + + +def test_scrub_does_not_mutate_the_caller(monkeypatch): + monkeypatch.setenv("DSH_SESSION_ID", "host-session") + + env = scrub_host_env(os.environ) + + assert env is not os.environ + assert os.environ["DSH_SESSION_ID"] == "host-session" + + +def test_launch_hands_the_child_a_scrubbed_environment(tmp_path, monkeypatch): + monkeypatch.setenv("DSH_SESSION_ID", "host-session") + monkeypatch.setenv("MCP_SERVERS", '{"filesystem": {}}') + monkeypatch.setenv("ARK_API_KEY", "not-a-real-key") + rd = runstore.create({"kind": "text", "target": str(tmp_path / "out.md")}, + store=str(tmp_path / "store")) + seen = {} + + def fake_run(argv, **kwargs): + seen["env"] = kwargs.get("env") + return subprocess.CompletedProcess(argv, 0, stdout="4242\n", stderr="") + + monkeypatch.setattr(runstore.subprocess, "run", fake_run) + + st = runstore.launch(rd) + + assert st.pid == 4242 + env = seen["env"] + assert "DSH_SESSION_ID" not in env and "MCP_SERVERS" not in env + assert env["ARK_API_KEY"] == "not-a-real-key" + assert env["PYTHONPATH"] # the launcher still makes the package importable + + +@pytest.mark.parametrize("marker", SESSION_MARKERS) +def test_every_session_marker_is_dropped(marker): + """A marker added for a new host is covered without a second list (#1082). + + The scrub used to carry its own four prefixes, so five of these eleven + crossed it; this row walks the tuple instead of naming them. + """ + env = scrub_host_env({_host_variable(marker): "host-session", "PATH": "/bin"}) + + assert [name for name in env if name != "PATH"] == [] + assert env["PATH"] == "/bin" + + +def test_the_worker_and_the_detached_run_share_one_list(): + """The two consumers of the denylist cannot drift apart again (#1082).""" + assert AGENTS_SESSION_MARKERS is SESSION_MARKERS + + +def test_a_provider_key_that_merely_looks_like_a_marker_is_kept(): + """``OPENCODE_API_KEY`` is a credential, not a config pointer (#1082).""" + env = scrub_host_env({"OPENCODE_API_KEY": "not-a-real-key", "OPENCODE_CONFIG": "/x"}) + + assert env == {"OPENCODE_API_KEY": "not-a-real-key"} + + +def test_launch_re_scrubs_a_caller_supplied_env(tmp_path, monkeypatch): + """A caller cannot put the host session back through ``env=`` (#1082).""" + rd = runstore.create({"kind": "text", "target": str(tmp_path / "out.md")}, + store=str(tmp_path / "store")) + seen = {} + + def fake_run(argv, **kwargs): + seen["env"] = kwargs.get("env") + return subprocess.CompletedProcess(argv, 0, stdout="4242\n", stderr="") + + monkeypatch.setattr(runstore.subprocess, "run", fake_run) + + runstore.launch(rd, env={"MCP_SERVERS": '{"filesystem": {}}', + "DSH_SESSION_ID": "host-session", + "RUN_TAG": "kept"}) + + env = seen["env"] + assert "MCP_SERVERS" not in env and "DSH_SESSION_ID" not in env + assert env["RUN_TAG"] == "kept" + assert env["PYTHONPATH"]