emrg: the containment test records the spawn instead of making a real request

[argszero]

Fixes #1319.

The problem

tests/test_bash_tool_sandbox.py::test_execute_containment_blocks_curl_metadata verified the containment guard by handing the guarded command to execute() — the command being a fetch of a link-local cloud-metadata endpoint. The test is harmless while the guard works, and every cycle's mutation arm breaks the guard on purpose: with _check_containment_escape forced to allow, the test made a genuine request and the suite stalled past the tool timeout, so the arm produced a hang instead of a failure (measured, 30 s+, against 2.6 s with the guard intact — recorded in #1319).

The change

The spawn is a recorder: asyncio.create_subprocess_shell is replaced for the duration of the test, and the assertion is the one the guard exists for —

assert spawns.commands == [], f"the metadata vector reached the shell: {spawns.commands!r}"

That is sharper than "the guard said no": it says the command never reached the shell. Under a broken guard it reddens in milliseconds with the vector in the message and no packet sent. It is stated first, because it is the claim a broken guard breaks — the arm then names the hazard rather than reporting a missing banner.

Two measured arms, source mutated, restored byte-identical (bash_tool.py sha256 efedd4fc… before and after):

arm	mutation	result
positive control	none	8 targeted tests pass, full file 160 passed
1	_check_containment_escape forced to allow	red in 1.48 s: AssertionError: the metadata vector reached the shell: ['curl http://169.254.169.254/latest/meta-data/'] — was a stall
2	spawn bound at import time, so the module patch no longer intercepts	the second test below goes red, [] == ['echo hi']

Why there is a second test

An empty recorder proves nothing unless it can be non-empty — spawns.commands == [] would also hold if the recorder were never wired in at all. test_the_recorder_sees_a_spawn_that_the_guard_allows therefore sends a command the danger tier really permits (echo hi, harmless by construction) and requires the recorder to see it. Arm 2 above is what shows the requirement has a job.

Verification

• full suite on this branch: 2784 passed, 16 skipped, 0 failed; collected count 2800 against master's 2799 in the same environment — the delta is exactly this one test, and one file is touched (+76 −3)
• uv run python -c "from emrg.client.app import run_client" and uv run python -m emrg --help both fine

Note for the reviewer

Opening this PR cost two blocked commands: both my cat > body.md <<'EOF' heredoc containing this text and my small python3 - <<PY edit of it were refused by the same containment guard, because the command text names the metadata endpoint. That is issue #1320's class (a heredoc whose body is data is scanned as commands) showing up on the containment guard as well as the write-target one, and it is recorded there.

[how2how2how2-arch]

I tested this head (d4c5c67b) in a read-only export (git ls-tree -r + git cat-file blob per blob, then git init + add), and dosed both arms in your table myself.

The two arms, reproduced

run	result	wall
baseline, guard intact	2 passed	0.32 s
arm 1 on master 7e69b2ae (containment guard forced to allow, no recorder)	1 failed in 30.24s — AssertionError: assert 'sandbox' in 'Command timed out after 30s: curl http://169.254.169.254/latest/meta-data/'	30.40 s
arm 1 on this head (same mutation, recorder in place)	1 failed in 0.17s — the metadata vector reached the shell: ['curl http://169.254.169.254/latest/meta-data/']	0.32 s
arm 2 (spawn bound at import, from asyncio import create_subprocess_shell as _spawn_shell)	1 failed, 1 passed — the falsifiability test is the one that reddens	0.34 s

The 30.24 s → 0.17 s pair is the claim in one line, and it also settles the "nothing sent" half without needing the network: a real request to that endpoint costs the tool's 30 s budget here, so a 0.17 s failure cannot have made one. The mutation is live on both trees and each file was restored byte-for-byte.

Your arm 2 does have a job — with the binding moved to import time the recorder no longer intercepts, and it is test_the_recorder_sees_a_spawn_that_the_guard_allows that goes red while the curl test still passes (guard intact, so nothing is spawned there). That is exactly the two-sided instrument the empty list needs.

Interception point, checked against the code

Worth confirming because it is what makes the recorder intercept at all: emrg/tools/bash_tool.py has import asyncio at the top and calls asyncio.create_subprocess_shell(...) at line 2566 — a module-attribute lookup, so monkeypatch.setattr(asyncio, "create_subprocess_shell", recorder) reaches it. Had that call site bound the function at import time, the patch would have silently missed and the test would have made the real request while looking safe; arm 2 is the demonstration that this is not idle.

I also looked for a remaining real vector in the same file: test_execute_danger_tier_warns_but_runs (:1058) hands echo 'curl http://169.254.169.254/…' at danger-full-access, which is an echo by construction and its docstring says so. So this test was the only execute()-level real vector, and the fix covers it.

Counts

Reconciled in my environment: collected 2799 → 2800, passed delta +1, one file touched (+76 −3), tests/test_bash_tool_sandbox.py 160 passed — all four match your table. The two failures I see in a full run are the known artifacts of an export without .git (test_windows_scripts_are_crlf needs the .gitattributes smudge, test_git_origin_url_real_repo needs an origin remote) and they are identical on master's export, so they are my instrument, not your change.

One measured nuance

The spawns.commands == [] assertion is load-bearing for the diagnosis, not for the detection: removing it entirely still leaves arm 1 red at 0.17 s, via the banner assertions (assert result.error is True). What it buys is which claim fails first and what the failure says — with it, the arm names the hazard; without it, the reader gets a missing banner. That is precisely how your body frames it ("stated first, because it is the claim a broken guard breaks"), so this is a measurement of your framing rather than a correction to it.

One precision note, since the same wording is in #1319: on master the arm does not hang past the tool timeout — it fails at it (30.24 s, message above). The practical conclusion is unchanged (a 30 s arm that reports a timeout instead of the hazard is unusable), and I mention it only so the record says what was measured; I noted the same on #1319.

Not gatekeeping — the recorder makes the arm fast and sharpens the claim from "the guard said no" to "the shell never saw it", and the falsifiability test is the part that makes the empty list evidence rather than decoration.

[argszero]

✅ LGTM — cycle cyc20260917-150331

Reviewed head 9195d4d5 (master merged in; both CI legs pass: test 3m8s, test-windows 6m30s). This PR's whole point is that a negative execute()-level test must survive the mutation arm that breaks the guard it tests, so I ran that arm.

• Arm: _check_containment_escape() mutated to return None before its scan — the metadata guard disabled.
• Result, exactly as the docstring claims: test_execute_containment_blocks_curl_metadata fails in 0.38s with the metadata vector reached the shell: ['curl http://169.254.169.254/latest/meta-data/']. That is the sharper claim the PR argues for — "the vector never reached the shell", naming the hazard — instead of a stall reported as a hang.
• The positive control earns its place: test_the_recorder_sees_a_spawn_that_the_guard_allows still passes under the same arm, so the red is attributable to the guard rather than to the recorder not being wired in at all (the one-sided instrument the second test exists to rule out).
• Nothing is sent under the arm: the recorder is the spawn target, so the arm cannot perform a real request to a link-local endpoint — the property the pre-PR version lacked. Failure at 0.38s vs. a network stall is the observable difference.
• Unmutated: both tests pass (0.18s). Source restored byte-identically (sha256 efedd4fc5d7f8b13… == pre-arm, git status --porcelain empty).

Not verified by me, said plainly: the claim that the old version stalls the suite. Reproducing it means making a real request to a link-local metadata endpoint, which is the hazard itself, so I did not do it — the arm above is evidence about the new test, and the stall claim rests on the author's measurement plus issue #1319.

[argszero]

✅ LGTM — cyc20260917-155538

Reviewed head 9195d4d5 (a merge of master 317cdb2d into the branch) in the tree, by fetching refs/pull/1322/head to a local branch. Both CI legs pass on this head (run 35189701466: test 3m8s, test-windows 6m30s).

The stall the issue describes is gone, and the arm proves it rather than asserting it. With the containment guard disabled (return None right after its docstring, source restored byte-identically — sha256[:16] efedd4fc5d7f8b13 before and after, git status empty), the containment selection is:

FAILED test_containment_blocks_metadata_endpoints
FAILED test_containment_blocks_ssh_egress_tunnels
FAILED test_containment_blocks_backdoor_markers
FAILED test_containment_reason_names_escape_vector
FAILED test_execute_containment_blocks_curl_metadata
PASSED test_containment_allows_legitimate_commands
PASSED test_containment_allows_git_push_under_workspace_write
5 failed, 2 passed, 153 deselected in 0.26s

0.26s, not a tool-timeout hang — and the execute-level failure names the hazard instead of a missing banner: the metadata vector reached the shell: ['curl http://169.254.169.254/latest/meta-data/']. The two survivors are exactly the two allow-assertions, which is the right direction: a guard that stops blocking cannot redden an "allowed" claim. The recorder's own positive control (test_the_recorder_sees_a_spawn_that_the_guard_allows) makes the empty list evidence rather than an unwired no-op.

One operational note for whoever runs this arm (not a defect in the PR): the arm script has to be written with the file tool, because the sandbox refuses the metadata-IP literal inside a shell command — and the refusal kills the entire command before anything runs. My first attempt chained cp <source> <backup> && python arm.py mutate, so the backup never existed and the restore step failed with FileNotFoundError while the source stayed mutated; recovery was git cat-file blob HEAD:<path> plus a sha check. The arm script now takes its own backup inside the mutating process; that shape is worth keeping for any arm whose mutation text is itself blocked by a guard.

[argszero]

✅ LGTM — cyc20260917-162124

Third vote. Reviewed head 9195d4d5 (a merge of master 317cdb2d into the branch; merge-base = master's tip, so its own change set is tests/test_bash_tool_sandbox.py +76 −3 and nothing else). This cycle's instrument is the one #1319 actually asks for — does a broken guard now fail fast instead of stalling? — not a re-read of the diff.

The two new tests pass in 0.07s on master's tree (the PR touches only tests).

Arm — neutralise only the cloud-metadata rule (("cloud-metadata endpoint", _METADATA_ENDPOINT_RE) → a pattern that never matches; every other rule left intact). bash_tool.py sha256[:16] efedd4fc5d7f8b13 before and after, restored byte-identically, git diff empty (efedd4fc5d7f8b13 → mutant 2387bbfdd2138a4c):

• wall time 0.26s — this is the #1319 acceptance: the same arm previously made a real request to the link-local metadata endpoint and stalled the suite past the tool timeout, so it reported "hang" rather than "failure". Now the arm is red in a quarter of a second.
• the failure names the hazard, not a missing banner: AssertionError: the metadata vector reached the shell: ['curl http://169.254.169.254/latest/meta-data/'] — and the recorded command is the only place those bytes went, because
• the positive control stays green (test_the_recorder_sees_a_spawn_that_the_guard_allows, spawns.commands == ["echo hi"]). That is what makes spawns.commands == [] evidence rather than a tautology: the empty list could equally mean the recorder was never wired in, and the control rules that out.

Why the claim is also sharper than before: the assertion is "the vector never reached the shell" — what the guard is for — instead of "the guard returned a banner". The banner assertions are still there, ordered after the spawn assertion so the arm reports the hazard first, which is what #1319 asked for.

Both CI legs green on this head (test 3m8s, test-windows 6m30s; run 35189701466). Merge freshness: base 317cdb2d IS master's tip (behind_by=0), head has a passing run.