[PR #300] [MERGED] test(groom): prove name resolution is dead in the sandbox, non-vacuously #302

New issue

Closed

opened 2026-10-26 22:38:14 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:14 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/300
Author: @mattmillerai
Created: 9/16/2026
Status: ✅ Merged
Merged: 9/17/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4304-groom-sandbox-net-bridge

📝 Commits (4)

c6f9049 test(groom): prove name resolution is dead in the sandbox, non-vacuously
977b7cc test(groom): key the 7d netns proof on the jail's own /proc, not a curl exit code
c5a8ceb test(groom): key 7d part (ii) on netns IDENTITY, and close the round-2 gaps
ae3c6fd Merge branch 'main' into matt/be-4304-groom-sandbox-net-bridge

📊 Changes

2 files changed (+188 additions, -10 deletions)

View changed files

📝 .github/groom/README.md (+40 -8)
📝 .github/groom/tests/sandbox-tests.sh (+148 -2)

📄 Description

ELI-5

The groom agent runs in a locked box with no way onto the network — the only thing it can reach is a unix socket that proxies to the API. We already had tests proving it can't dial any IP address. But every one of those tests dials a raw IP on purpose, so none of them ever asked the box to look up a name. This adds that test. The interesting part is that the obvious version of it is a lie: it passes even when the box is fully connected to the network, so it would never catch a regression. This adds the version that actually goes red.

What this is

The confinement work (isolated netns, unix-socket broker, in-jail TCP→UDS bridge, all three groom agent steps wired to it) already landed. Reading the tree against the acceptance criteria, everything was in place except one leg of the proof, which is what this PR adds.

Criterion-by-criterion, against the current default branch:

Criterion	State
broker listens on a unix socket, running on the host	already shipped — `broker.mjs` takes a socket path as `argv[2]`
sandbox drops `--share-net` for a full isolated netns, broker socket bind-mounted in, TCP→UDS bridge so `ANTHROPIC_BASE_URL` keeps working	already shipped — `agent-sandbox.sh --uds` + `--unshare-all`, bridged by `jail-shim.mjs`
sandbox-tests: broker round-trip through the unix socket from inside	already shipped — §5
sandbox-tests: direct TCP to a host-listening port and to an external IP both fail	already shipped — §7a, §7c (§7b adds cloud metadata)
sandbox-tests: DNS resolution fails	missing — added here as §7d
all three groom agent steps run with the netns isolated	already shipped
canary groom dry-run wall-time comparable to the prior phase	not exercised — see Residual

The finding: the obvious DNS check is vacuous

My first draft was the one-liner — resolve a name in the jail, assert it fails. I ran it against a shared-netns control before trusting it, and it passed there too.

The reason is that /etc/resolv.conf is a symlink into /run, and the sandbox mounts /etc read-only but deliberately does not mount /run (mounting it would be a confinement regression in its own right). So the resolver has no nameserver configured no matter what the network namespace looks like. A bare "DNS fails" assertion is therefore green whether the jail is isolated or wide open — it could never catch someone putting the sandbox back on a shared network, which is precisely the regression it would exist to catch.

So §7d asserts both halves:

Resolution fails, on resolver-specific exit codes rather than a bare non-zero: getent 2 is "key not found" (a missing binary is 127) and curl 6 is CURLE_COULDNT_RESOLVE_HOST (a connect failure is 7, a timeout 28, a missing binary 127). This is the same false-pass class the existing §7a/7b/7c tool-presence guard was written for, closed here by construction instead. Part (i) is only evidence about the sandbox if the name is live off-jail, so a host-side control (bounded by timeout 5) gates it: when this host cannot resolve the name either, the assertions still run and still have to hold, but part (i) is reported SKIP rather than counted as proof.
The netns is empty — read out of the jail's own /proc. The wrapper passes --proc /proc, a fresh procfs mount inside the new netns, so /proc/net/dev and /proc/net/route describe the jail's network: the first must list lo and nothing else, the second must carry no default route. This is the half only an isolated netns can produce, and it is what makes the section a real proof.

Review's first pass at this keyed part 2 on an exact curl 7 to a hardcoded nameserver address instead, and that was wrong — see the thread on §7d. curl 7 is CURLE_COULDNT_CONNECT, which equally covers ECONNREFUSED, ENETUNREACH and a firewall REJECT, so a fully shared netns on a filtered or offline host returns 7 too and the assertion went green on exactly the regression it exists to catch; it was TCP-only besides. The /proc facts cannot be faked by host network state, need no egress of any kind, and cover a UDP/53 path as well as TCP — no route is no route, for any protocol. The 1.1.1.1:53 probe is kept as behavioral confirmation but relaxed to "did not succeed"; its exact code is no longer load-bearing.

Verification

Run on a Linux host with unprivileged user namespaces, against the real agent-sandbox.sh:

shellcheck -x .github/groom/*.sh .github/groom/tests/*.sh — clean.
python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — 398 tests, OK.
python3 .github/workflow-pins/check_workflow_pins.py — 11 workflows, 0 defaults, all ref checkouts guarded.
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (2 pre-existing warnings: AGENTS.md is 155 lines vs the 150 aspirational target, and no CODEOWNERS; both predate this branch).
sandbox-tests.sh §1–§4, §7a–§7e — all PASS, including §7d.

Empirical falsification of the new negative assertion. This diff asserts a capability is absent, which is exactly the claim a self-authored test cannot establish on its own, so I went and attempted the capability rather than assuming it:

Attempted name resolution from inside the real jail through every path available to the agent: the NSS/getent path, curl's own resolver, and a direct connect to a nameserver address. All three fail; none of them is reachable by any route. The jail's own /proc/net/dev lists lo alone and its /proc/net/route is empty, so there is no route for any protocol to take.
Negative control: ran the identical assertion block against a jail patched to --share-net. It correctly goes red: jail netns has non-loopback interface(s): ens4 docker0 tailscale0 — the netns is SHARED with the host, so nothing here proves isolation.
The superseded exact-curl-7 assertion was separately shown false-passable, which is why part 2 was rekeyed: in a fully shared, fully routable netns, a connect to any closed port returns 7 (ECONNREFUSED). Exit 7 therefore does not imply "no route", and an assertion demanding it cannot distinguish an isolated netns from a shared one.
Positive control: the same block under --unshare-all exits 0.
Separately measured the vacuity described above: the resolution half alone exits 0 under a shared netns. That measurement is the reason part 2 exists.

Judgment calls

The ticket's plan named socat for the in-jail bridge and --net-bridge / /run/groom/broker.sock for the flag and mount path. What shipped uses jail-shim.mjs (node, already present — no apt-get install), the flag --uds, and /run/broker.sock. Those choices are not mine and are not changed here; I mention them only so the plan's wording isn't read as unimplemented.
The ticket says to add the assertions to ci-groom.yml. That reference is stale — the sandbox-tests job lives in test-groom-scripts.yml, path-filtered on .github/groom/**, so the new section is picked up with no workflow edit.
No Skip-caller-bump: true trailer. This PR is a test file plus a README, neither of which a consumer ever loads at run time, so the caller bump it will trigger is a no-op SHA churn. But the trailer is documented for comment/docs-only edits and a test file is not literally that, and the guidance is to leave it off when in doubt. Left off deliberately; a reviewer who reads it as docs-only can add it.
--max-time on the in-jail probes is a backstop, not margin. The original comment oversold it; corrected in review. It is not sized to outlast a retrying resolver — glibc's defaults (timeout:5, attempts:2) burn exactly 10s on one blackholing nameserver — but every failure here is immediate in practice, and a clipped lookup exits 28 and fails loud with the code printed rather than passing silently.

Residual

The canary dry-run wall-time criterion is not exercised. The acceptance text asks for a canary groom dry-run completing in wall-time comparable to the prior phase, with the specific watch item being multi-second per-turn stalls from residual non-API traffic timing out against the closed netns (and a stop-ship if they appear). I did not run it: it needs a dispatched workflow run with a live API key, which is outside what this branch can do and is a spending action besides. What I can report from reading the tree is that all three agent steps already set CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC=1 — the suppression the criterion names as the expected mitigation — and that the netns has been isolated on the default branch since the phase that shipped it, so any such stall would already be observable in existing groom run history rather than being introduced by this PR. Someone with access to a groom run should still time one dry-run against a recent pre-isolation run and confirm per-turn latency is unchanged; if it is not, the mitigation to investigate is residual non-API traffic from the CLI timing out with no route, and the documented fallback is to stop-ship the isolation and keep the earlier phases as the baseline.
sandbox-tests.sh §5 and §6 were not run on my host and are unchanged by this diff. They need node resolvable under /usr (the jail's PATH is /usr/local/bin:/usr/bin:/bin); on my host node lives outside /usr, so both fail there. I confirmed this is pre-existing and not caused by this branch by running the unmodified suite from the base commit and getting the identical failure at the identical point. CI runners have node under /usr/local/bin, which is inside the read-only /usr bind, so both sections exercise normally there — but I have not observed them pass on this branch, only on CI's behalf.
~~Part 2 of §7d can itself become vacuous on a host with no egress at all.~~ Resolved in review. This was real and worse than stated — it applied to any filtered host, not just a fully offline one. Part 2 no longer depends on host reachability at all: it reads the jail's interface list and route table out of /proc, which no host network state can fake.
The parent epic was named to me but not readable from here, so I could not check this leg against whatever else it tracks. Scope was taken from this ticket's own text alone.

Provenance

Authored by: agent-work loop
Verified: shellcheck (groom scripts + tests): clean; groom python suite: 398 tests, OK; check_workflow_pins.py: 11 workflows, 0 defaults, all ref checkouts guarded; check_agents_md.py: passed (2 pre-existing warnings); sandbox-tests.sh §1-§4 + §7a-§7e: all PASS. §7d validated by a positive control (real isolated netns, exit 0) and a negative control (identical block against a --share-net jail, exit 1, naming ens4/docker0/tailscale0); the superseded exact-curl-7 form was separately demonstrated false-passable in a shared, routable netns (exit 7 on any closed port), which is the measurement behind the review rekey.
Deviations: the canary dry-run wall-time criterion was not exercised (no workflow dispatch / API spend available from this branch) — see Residual; sandbox-tests.sh §5/§6 unrunnable on this host for a pre-existing, environment-only reason also recorded there (§7 was therefore exercised via a driver copy with §5/§6 elided, against the real agent-sandbox.sh)

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/300 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/17/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4304-groom-sandbox-net-bridge` --- ### 📝 Commits (4) - [`c6f9049`](https://github.com/Comfy-Org/github-workflows/commit/c6f904947890e3216d8288a2bdae15d38eff6ab8) test(groom): prove name resolution is dead in the sandbox, non-vacuously - [`977b7cc`](https://github.com/Comfy-Org/github-workflows/commit/977b7cc11b3e0e73ba5d2580a4d59f8222a19469) test(groom): key the 7d netns proof on the jail's own /proc, not a curl exit code - [`c5a8ceb`](https://github.com/Comfy-Org/github-workflows/commit/c5a8cebd459a94222e9b6a6be44db2f024f8b490) test(groom): key 7d part (ii) on netns IDENTITY, and close the round-2 gaps - [`ae3c6fd`](https://github.com/Comfy-Org/github-workflows/commit/ae3c6fd953f85fcb03b2d14900d15e37a2afd8a7) Merge branch 'main' into matt/be-4304-groom-sandbox-net-bridge ### 📊 Changes **2 files changed** (+188 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+40 -8) 📝 `.github/groom/tests/sandbox-tests.sh` (+148 -2) </details> ### 📄 Description ## ELI-5 The groom agent runs in a locked box with no way onto the network — the only thing it can reach is a unix socket that proxies to the API. We already had tests proving it can't dial any IP address. But every one of those tests dials a raw IP *on purpose*, so none of them ever asked the box to look up a *name*. This adds that test. The interesting part is that the obvious version of it is a lie: it passes even when the box is fully connected to the network, so it would never catch a regression. This adds the version that actually goes red. ## What this is The confinement work (isolated netns, unix-socket broker, in-jail TCP→UDS bridge, all three groom agent steps wired to it) already landed. Reading the tree against the acceptance criteria, everything was in place except one leg of the proof, which is what this PR adds. Criterion-by-criterion, against the current default branch: | Criterion | State | |---|---| | broker listens on a unix socket, running on the host | already shipped — `broker.mjs` takes a socket path as `argv[2]` | | sandbox drops `--share-net` for a full isolated netns, broker socket bind-mounted in, TCP→UDS bridge so `ANTHROPIC_BASE_URL` keeps working | already shipped — `agent-sandbox.sh --uds` + `--unshare-all`, bridged by `jail-shim.mjs` | | sandbox-tests: broker round-trip through the unix socket from inside | already shipped — §5 | | sandbox-tests: direct TCP to a host-listening port and to an external IP both fail | already shipped — §7a, §7c (§7b adds cloud metadata) | | **sandbox-tests: DNS resolution fails** | **missing — added here as §7d** | | all three groom agent steps run with the netns isolated | already shipped | | canary groom dry-run wall-time comparable to the prior phase | **not exercised — see Residual** | ## The finding: the obvious DNS check is vacuous My first draft was the one-liner — resolve a name in the jail, assert it fails. I ran it against a shared-netns control before trusting it, and it passed there too. The reason is that `/etc/resolv.conf` is a symlink into `/run`, and the sandbox mounts `/etc` read-only but deliberately does *not* mount `/run` (mounting it would be a confinement regression in its own right). So the resolver has no nameserver configured no matter what the network namespace looks like. A bare "DNS fails" assertion is therefore green whether the jail is isolated or wide open — it could never catch someone putting the sandbox back on a shared network, which is precisely the regression it would exist to catch. So §7d asserts both halves: 1. **Resolution fails**, on resolver-*specific* exit codes rather than a bare non-zero: `getent` 2 is "key not found" (a missing binary is 127) and `curl` 6 is `CURLE_COULDNT_RESOLVE_HOST` (a connect failure is 7, a timeout 28, a missing binary 127). This is the same false-pass class the existing §7a/7b/7c tool-presence guard was written for, closed here by construction instead. Part (i) is only *evidence about the sandbox* if the name is live off-jail, so a host-side control (bounded by `timeout 5`) **gates** it: when this host cannot resolve the name either, the assertions still run and still have to hold, but part (i) is reported `SKIP` rather than counted as proof. 2. **The netns is empty** — read out of the jail's own `/proc`. The wrapper passes `--proc /proc`, a fresh procfs mount inside the new netns, so `/proc/net/dev` and `/proc/net/route` describe the *jail's* network: the first must list `lo` and nothing else, the second must carry no default route. This is the half only an isolated netns can produce, and it is what makes the section a real proof. Review's first pass at this keyed part 2 on an exact `curl` 7 to a hardcoded nameserver address instead, and **that was wrong** — see the thread on §7d. `curl` 7 is `CURLE_COULDNT_CONNECT`, which equally covers ECONNREFUSED, ENETUNREACH and a firewall REJECT, so a *fully shared* netns on a filtered or offline host returns 7 too and the assertion went green on exactly the regression it exists to catch; it was TCP-only besides. The `/proc` facts cannot be faked by host network state, need no egress of any kind, and cover a UDP/53 path as well as TCP — no route is no route, for any protocol. The 1.1.1.1:53 probe is kept as behavioral confirmation but relaxed to "did not succeed"; its exact code is no longer load-bearing. ## Verification Run on a Linux host with unprivileged user namespaces, against the real `agent-sandbox.sh`: - `shellcheck -x .github/groom/*.sh .github/groom/tests/*.sh` — clean. - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — 398 tests, OK. - `python3 .github/workflow-pins/check_workflow_pins.py` — 11 workflows, 0 defaults, all ref checkouts guarded. - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed (2 pre-existing warnings: AGENTS.md is 155 lines vs the 150 aspirational target, and no CODEOWNERS; both predate this branch). - `sandbox-tests.sh` §1–§4, §7a–§7e — all PASS, including §7d. **Empirical falsification of the new negative assertion.** This diff asserts a capability is *absent*, which is exactly the claim a self-authored test cannot establish on its own, so I went and attempted the capability rather than assuming it: - Attempted name resolution from inside the real jail through every path available to the agent: the NSS/`getent` path, `curl`'s own resolver, and a direct connect to a nameserver address. All three fail; none of them is reachable by any route. The jail's own `/proc/net/dev` lists `lo` alone and its `/proc/net/route` is empty, so there is no route for any protocol to take. - **Negative control:** ran the identical assertion block against a jail patched to `--share-net`. It correctly goes red: `jail netns has non-loopback interface(s): ens4 docker0 tailscale0 — the netns is SHARED with the host, so nothing here proves isolation`. - **The superseded exact-`curl`-7 assertion was separately shown false-passable**, which is why part 2 was rekeyed: in a fully shared, fully *routable* netns, a connect to any closed port returns 7 (ECONNREFUSED). Exit 7 therefore does not imply "no route", and an assertion demanding it cannot distinguish an isolated netns from a shared one. - **Positive control:** the same block under `--unshare-all` exits 0. - Separately measured the vacuity described above: the resolution half *alone* exits 0 under a shared netns. That measurement is the reason part 2 exists. ## Judgment calls - **The ticket's plan named `socat` for the in-jail bridge and `--net-bridge` / `/run/groom/broker.sock` for the flag and mount path.** What shipped uses `jail-shim.mjs` (node, already present — no `apt-get install`), the flag `--uds`, and `/run/broker.sock`. Those choices are not mine and are not changed here; I mention them only so the plan's wording isn't read as unimplemented. - **The ticket says to add the assertions to `ci-groom.yml`.** That reference is stale — the `sandbox-tests` job lives in `test-groom-scripts.yml`, path-filtered on `.github/groom/**`, so the new section is picked up with no workflow edit. - **No `Skip-caller-bump: true` trailer.** This PR is a test file plus a README, neither of which a consumer ever loads at run time, so the caller bump it will trigger is a no-op SHA churn. But the trailer is documented for *comment/docs-only* edits and a test file is not literally that, and the guidance is to leave it off when in doubt. Left off deliberately; a reviewer who reads it as docs-only can add it. - **`--max-time` on the in-jail probes is a backstop, not margin.** The original comment oversold it; corrected in review. It is not sized to outlast a retrying resolver — glibc's defaults (timeout:5, attempts:2) burn exactly 10s on one blackholing nameserver — but every failure here is immediate in practice, and a clipped lookup exits 28 and fails loud with the code printed rather than passing silently. ## Residual - **The canary dry-run wall-time criterion is not exercised.** The acceptance text asks for a canary groom dry-run completing in wall-time comparable to the prior phase, with the specific watch item being multi-second per-turn stalls from residual non-API traffic timing out against the closed netns (and a stop-ship if they appear). I did not run it: it needs a dispatched workflow run with a live API key, which is outside what this branch can do and is a spending action besides. What I *can* report from reading the tree is that all three agent steps already set `CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC=1` — the suppression the criterion names as the expected mitigation — and that the netns has been isolated on the default branch since the phase that shipped it, so any such stall would already be observable in existing groom run history rather than being introduced by this PR. **Someone with access to a groom run should still time one dry-run against a recent pre-isolation run and confirm per-turn latency is unchanged**; if it is not, the mitigation to investigate is residual non-API traffic from the CLI timing out with no route, and the documented fallback is to stop-ship the isolation and keep the earlier phases as the baseline. - **`sandbox-tests.sh` §5 and §6 were not run on my host** and are unchanged by this diff. They need `node` resolvable under `/usr` (the jail's `PATH` is `/usr/local/bin:/usr/bin:/bin`); on my host `node` lives outside `/usr`, so both fail there. I confirmed this is pre-existing and not caused by this branch by running the *unmodified* suite from the base commit and getting the identical failure at the identical point. CI runners have `node` under `/usr/local/bin`, which is inside the read-only `/usr` bind, so both sections exercise normally there — but I have not observed them pass on this branch, only on CI's behalf. - ~~**Part 2 of §7d can itself become vacuous on a host with no egress at all.**~~ **Resolved in review.** This was real and worse than stated — it applied to any *filtered* host, not just a fully offline one. Part 2 no longer depends on host reachability at all: it reads the jail's interface list and route table out of `/proc`, which no host network state can fake. - **The parent epic was named to me but not readable from here**, so I could not check this leg against whatever else it tracks. Scope was taken from this ticket's own text alone. ## Provenance - **Authored by:** agent-work loop - **Verified:** shellcheck (groom scripts + tests): clean; groom python suite: 398 tests, OK; check_workflow_pins.py: 11 workflows, 0 defaults, all ref checkouts guarded; check_agents_md.py: passed (2 pre-existing warnings); sandbox-tests.sh §1-§4 + §7a-§7e: all PASS. §7d validated by a positive control (real isolated netns, exit 0) and a negative control (identical block against a `--share-net` jail, exit 1, naming ens4/docker0/tailscale0); the superseded exact-`curl`-7 form was separately demonstrated false-passable in a shared, routable netns (exit 7 on any closed port), which is the measurement behind the review rekey. - **Deviations:** the canary dry-run wall-time criterion was not exercised (no workflow dispatch / API spend available from this branch) — see Residual; sandbox-tests.sh §5/§6 unrunnable on this host for a pre-existing, environment-only reason also recorded there (§7 was therefore exercised via a driver copy with §5/§6 elided, against the real `agent-sandbox.sh`) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-26 22:38:14 +00:00

bsakaguchi referenced this issue

2026-10-26 22:38:14 +00:00

[PR #302] [MERGED] ci(bump-callers): add WATCHED_EXEC to the five fleets whose asset directory outlives the scripts inside it #303

bsakaguchi closed this issue

2026-10-26 22:38:15 +00:00

bsakaguchi referenced this issue

2026-10-26 22:38:27 +00:00

[PR #318] [MERGED] test(bump-callers): assert the suite's own trigger covers every fleet paths: positive #320