builder agents inside the sandbox + key broker #293

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-4303-groom-sandbox-wire

📝 Commits (3)

ad1036d feat(groom): run the finder/verifier/builder agents inside the sandbox + key broker
eff84d9 fix(groom): address cursor-review panel findings on the sandbox+broker wiring
4e80b8e Merge origin/main into matt/be-4303-groom-sandbox-wire

📊 Changes

4 files changed (+547 additions, -410 deletions)

View changed files

📝 .github/groom/README.md (+36 -13)
📝 .github/groom/agent-sandbox.sh (+10 -4)
📝 .github/groom/tests/test_environment_binding.py (+120 -0)
📝 .github/workflows/groom.yml (+381 -393)

📄 Description

ELI-5

Groom runs three AI agents (a finder, a verifier, and a builder) over other
repos' code. That code is untrusted — a malicious repo could try to trick an
agent into stealing the Anthropic API key. Until now each agent step held the
real key in its environment and relied on a hand-rolled chmod/env -u scrub.

This PR moves all three agents inside the bubblewrap jail that phases 1–2
already built (agent-sandbox.sh + broker.mjs + jail-shim.mjs). The real key
now lives only in a small "broker" step; the agent runs with a dummy key and
reaches Anthropic through the broker over a bind-mounted unix socket. The jail
also hides the runner's filesystem, clears the environment, and cuts off the
network — so a prompt-injected agent has nothing to steal and nowhere to send it.
This is the gate that had been blocking groom on untrusted-contributor repos.

What changed (per agent job: `audit_find`, `audit_verify`, each `build` cell)

New Start the key broker step — the only agent-facing step holding
secrets.ANTHROPIC_API_KEY. Starts broker.mjs on the host socket
$BROKER_SOCK, polls /healthz (fails the job after ~10s), records the pid.
The agent step (Run finder/Run verifier/Run builder) drops the key
and runs agent-sandbox.sh — clone ro for finder/verifier, rw-git-ro for
the builder. A bash -c wrapper starts the in-jail jail-shim.mjs forwarder,
waits for it, then execs the pinned claude CLI with a dummy key and
ANTHROPIC_BASE_URL pointed at the shim. The exec JSON is captured by a
host-side stdout redirect, out of the agent's reach.
Outputs move under one GROOM_OUT_DIR (/tmp/groom-out) — the jail's sole
rw --out-dir. Briefs and the builder's finding JSON stay at /tmp and are
passed --ro-file. The verifier's finder-candidates download follows into
GROOM_OUT_DIR so the rw bind makes it readable in the jail.
always() cleanup step kills the broker by recorded pid.
Removed as subsumed by the jail: the chmod -R a-w clone/.git lock+unlock
dance, the env -u GITHUB_*/RUNNER_TEMP prefix (--clearenv covers it), and
the rm -rf diag pre-delete (host /tmp is a shadowed tmpfs inside the jail).
Kept as regression tripwires: the literal-key pre-publish scans (now their
own finder/verifier steps that hold the key with no agent present; the builder's
stays in Capture patch), the type-guarded diag projection, the CLI pin/flags.
New regression test (test_environment_binding.py): asserts the real key is
in no agent step and each agent step runs inside agent-sandbox.sh with the
dummy key — mirroring the existing bot-App-key boundary test. (Falsified: it
fails if the key is re-added to an agent step.)

Deviation from the ticket's plan — worth a careful look

The ticket described a TCP broker (node broker.mjs 8082,
ANTHROPIC_BASE_URL=http://127.0.0.1:8082, no netns). Since that plan was
written, phase-2 hardening (BE-4421) evolved the merged assets to an isolated
network namespace with no egress, reaching the broker only over a
bind-mounted unix socket (/run/broker.sock) bridged by the in-jail
jail-shim.mjs. So the TCP recipe would not work against today's
agent-sandbox.sh.

I implemented the ticket's intent using the canonical composition the repo
already documents and CI-tests — sandbox-tests.sh §5 calls it "exactly the
composition the production groom caller will use" (host broker.mjs on a UDS +
agent-sandbox.sh --uds + in-jail jail-shim.mjs). The ANTHROPIC_BASE_URL
still points at http://127.0.0.1:8082, but 8082 is now the jail-local shim
port, not a host TCP port.

One gap the ticket's plan omitted: the builder also writes PR_BODY_OUT
(BE-4346) and reads FINDING_IN. PR_BODY_OUT moved under GROOM_OUT_DIR
(agent-written, needs the rw bind); FINDING_IN is passed --ro-file (read-only
input).

Acceptance proof

grep: secrets.ANTHROPIC_API_KEY appears in no Run finder/verifier/builder
step — only in the broker steps, the two scan steps, and Capture patch (all
no-agent). Verified by script and by the new unit test.
Subsumed mitigations deleted (chmod dance, env -u prefix, rm -rf pre-delete).
Full groom suite green (383 passed), check_workflow_pins.py OK,
check_agents_md.py OK, shellcheck -x agent-sandbox.sh sandbox-tests.sh OK,
and bash -n + shellcheck clean on every changed run: script.

Validation I could run locally

sandbox-tests.sh sections 1–4 & 7 pass here (env scrub, FS confinement + tmpfs
shadowing, rw-git-ro, out-dir/clone overlap rejection, pid isolation, egress
isolation).
I smoke-tested the exact wiring: broker.mjs on a UDS + /healthz poll +
agent-sandbox.sh --uds + in-jail jail-shim.mjs. Broker readiness over the
socket worked; the jail environ contained only the dummy key (env →
ANTHROPIC_API_KEY=groom-sandbox-placeholder plus HOME/PATH/PWD/SHLVL/TERM,
no real key) — the ticket's negative proof, satisfied.
- Note: the ticket's literal tr '\0' '\n' < /proc/self/environ returns
  empty inside the jail (a /proc/self self-resolution/exec-timing quirk),
  so the robust command is env or /proc/$$/environ.

Residual — not exercised here; please complete before merge

End-to-end canary NOT run. The ticket asks for a workflow_dispatch groom
with dry_run: true on the pilot caller (comfy-cloud-mcp-server), confirming
finder/verifier/builder complete within --max-turns, no new denials_by_tool
storm, finder JSON parses, and a builder cell produces a patch. That needs
GitHub Actions + the real ANTHROPIC_API_KEY + a pilot repo, which a one-shot
cannot do. Run it and paste the link before merging — it is an acceptance
criterion.
sandbox-tests.sh §5/§6 could not run on this dev host. This host has
node at a custom path outside /usr, so it is not inside the jail's read-only
/usr bind and the shim/broker sections fail with node: command not found.
This reproduces identically on origin/main's unmodified assets (pre-existing,
host-only). On ubuntu-latest node lives under /usr, so those sections —
and the same in-jail node+claude flow this PR relies on — pass in CI. Confirm
test-groom-scripts.yml is green on CI as the second half of §5's proof.
Node/claude-in-jail assumption. The wiring assumes node and the
npm -g-installed claude resolve inside the jail from /usr//usr/local
(true on ubuntu-latest; the CLI pin is untouched). If a self-hosted runner
put them elsewhere the jail would not see them — surfaced by the broker/shim
readiness poll failing the job loudly (fail-closed).

Provenance

Authored by: agent-work loop
Verified: groom unittest suite: 383 passed, 0 failed; check_workflow_pins.py: OK; check_agents_md.py: passed (2 pre-existing warnings); shellcheck agent-sandbox.sh+sandbox-tests.sh: clean; bash -n on all 45 workflow run: scripts: clean. NOT run: end-to-end groom canary (see Residual).
Deviations: implemented the ticket's intent via the current UDS + jail-shim composition rather than its stale TCP-8082 recipe (see "Deviation" above); also moved PR_BODY_OUT under the out-dir, which the ticket's file list omitted.

_{🔄 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/293 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/16/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4303-groom-sandbox-wire` --- ### 📝 Commits (3) - [`ad1036d`](https://github.com/Comfy-Org/github-workflows/commit/ad1036de6dc49263692337167dd2fe5a47842de5) feat(groom): run the finder/verifier/builder agents inside the sandbox + key broker - [`eff84d9`](https://github.com/Comfy-Org/github-workflows/commit/eff84d94cabda078d6cd5ba70dfaea2267d3dcc5) fix(groom): address cursor-review panel findings on the sandbox+broker wiring - [`4e80b8e`](https://github.com/Comfy-Org/github-workflows/commit/4e80b8ee0ebab388eaf608c9d197d9e21e1e9aab) Merge origin/main into matt/be-4303-groom-sandbox-wire ### 📊 Changes **4 files changed** (+547 additions, -410 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+36 -13) 📝 `.github/groom/agent-sandbox.sh` (+10 -4) 📝 `.github/groom/tests/test_environment_binding.py` (+120 -0) 📝 `.github/workflows/groom.yml` (+381 -393) </details> ### 📄 Description ## ELI-5 Groom runs three AI agents (a finder, a verifier, and a builder) over other repos' code. That code is **untrusted** — a malicious repo could try to trick an agent into stealing the Anthropic API key. Until now each agent step held the real key in its environment and relied on a hand-rolled `chmod`/`env -u` scrub. This PR moves all three agents **inside the bubblewrap jail** that phases 1–2 already built (`agent-sandbox.sh` + `broker.mjs` + `jail-shim.mjs`). The real key now lives only in a small "broker" step; the agent runs with a **dummy** key and reaches Anthropic through the broker over a bind-mounted unix socket. The jail also hides the runner's filesystem, clears the environment, and cuts off the network — so a prompt-injected agent has nothing to steal and nowhere to send it. This is the gate that had been blocking groom on untrusted-contributor repos. ## What changed (per agent job: `audit_find`, `audit_verify`, each `build` cell) - **New `Start the key broker` step** — the only agent-facing step holding `secrets.ANTHROPIC_API_KEY`. Starts `broker.mjs` on the host socket `$BROKER_SOCK`, polls `/healthz` (fails the job after ~10s), records the pid. - **The agent step (`Run finder`/`Run verifier`/`Run builder`) drops the key** and runs `agent-sandbox.sh` — clone `ro` for finder/verifier, `rw-git-ro` for the builder. A `bash -c` wrapper starts the in-jail `jail-shim.mjs` forwarder, waits for it, then `exec`s the pinned `claude` CLI with a dummy key and `ANTHROPIC_BASE_URL` pointed at the shim. The exec JSON is captured by a host-side stdout redirect, out of the agent's reach. - **Outputs move under one `GROOM_OUT_DIR` (`/tmp/groom-out`)** — the jail's sole rw `--out-dir`. Briefs and the builder's finding JSON stay at `/tmp` and are passed `--ro-file`. The verifier's finder-candidates download follows into `GROOM_OUT_DIR` so the rw bind makes it readable in the jail. - **`always()` cleanup step** kills the broker by recorded pid. - **Removed as subsumed by the jail:** the `chmod -R a-w` clone/`.git` lock+unlock dance, the `env -u GITHUB_*`/`RUNNER_TEMP` prefix (`--clearenv` covers it), and the `rm -rf` diag pre-delete (host `/tmp` is a shadowed tmpfs inside the jail). - **Kept as regression tripwires:** the literal-key pre-publish scans (now their own finder/verifier steps that hold the key with no agent present; the builder's stays in `Capture patch`), the type-guarded diag projection, the CLI pin/flags. - **New regression test** (`test_environment_binding.py`): asserts the real key is in no agent step and each agent step runs inside `agent-sandbox.sh` with the dummy key — mirroring the existing bot-App-key boundary test. (Falsified: it fails if the key is re-added to an agent step.) ## Deviation from the ticket's plan — worth a careful look The ticket described a **TCP** broker (`node broker.mjs 8082`, `ANTHROPIC_BASE_URL=http://127.0.0.1:8082`, no netns). Since that plan was written, phase-2 hardening (BE-4421) evolved the merged assets to an **isolated network namespace with no egress**, reaching the broker only over a bind-mounted **unix socket** (`/run/broker.sock`) bridged by the in-jail `jail-shim.mjs`. So the TCP recipe would not work against today's `agent-sandbox.sh`. I implemented the ticket's **intent** using the canonical composition the repo already documents and CI-tests — `sandbox-tests.sh` §5 calls it "exactly the composition the production groom caller will use" (host `broker.mjs` on a UDS + `agent-sandbox.sh --uds` + in-jail `jail-shim.mjs`). The `ANTHROPIC_BASE_URL` still points at `http://127.0.0.1:8082`, but 8082 is now the **jail-local shim port**, not a host TCP port. One gap the ticket's plan omitted: the builder also writes **`PR_BODY_OUT`** (BE-4346) and reads **`FINDING_IN`**. `PR_BODY_OUT` moved under `GROOM_OUT_DIR` (agent-written, needs the rw bind); `FINDING_IN` is passed `--ro-file` (read-only input). ## Acceptance proof - `grep`: `secrets.ANTHROPIC_API_KEY` appears in **no** `Run finder/verifier/builder` step — only in the broker steps, the two scan steps, and `Capture patch` (all no-agent). Verified by script and by the new unit test. - Subsumed mitigations deleted (chmod dance, `env -u` prefix, `rm -rf` pre-delete). - Full groom suite green (`383 passed`), `check_workflow_pins.py` OK, `check_agents_md.py` OK, `shellcheck -x agent-sandbox.sh sandbox-tests.sh` OK, and `bash -n` + `shellcheck` clean on every changed `run:` script. ## Validation I could run locally - `sandbox-tests.sh` sections 1–4 & 7 pass here (env scrub, FS confinement + tmpfs shadowing, `rw-git-ro`, out-dir/clone overlap rejection, pid isolation, egress isolation). - I smoke-tested the exact wiring: `broker.mjs` on a UDS + `/healthz` poll + `agent-sandbox.sh --uds` + in-jail `jail-shim.mjs`. Broker readiness over the socket worked; **the jail environ contained only the dummy key** (`env` → `ANTHROPIC_API_KEY=groom-sandbox-placeholder` plus `HOME/PATH/PWD/SHLVL/TERM`, no real key) — the ticket's negative proof, satisfied. - Note: the ticket's literal `tr '\0' '\n' < /proc/self/environ` returns **empty** inside the jail (a `/proc/self` self-resolution/exec-timing quirk), so the robust command is `env` or `/proc/$$/environ`. ## Residual — not exercised here; please complete before merge - **End-to-end canary NOT run.** The ticket asks for a `workflow_dispatch` groom with `dry_run: true` on the pilot caller (comfy-cloud-mcp-server), confirming finder/verifier/builder complete within `--max-turns`, no new `denials_by_tool` storm, finder JSON parses, and a builder cell produces a patch. That needs GitHub Actions + the real `ANTHROPIC_API_KEY` + a pilot repo, which a one-shot cannot do. **Run it and paste the link before merging** — it is an acceptance criterion. - **`sandbox-tests.sh` §5/§6 could not run on this dev host.** This host has `node` at a custom path outside `/usr`, so it is not inside the jail's read-only `/usr` bind and the shim/broker sections fail with `node: command not found`. This reproduces identically on `origin/main`'s unmodified assets (pre-existing, host-only). On `ubuntu-latest` `node` lives under `/usr`, so those sections — and the same in-jail node+claude flow this PR relies on — pass in CI. **Confirm `test-groom-scripts.yml` is green on CI** as the second half of §5's proof. - **Node/claude-in-jail assumption.** The wiring assumes `node` and the `npm -g`-installed `claude` resolve inside the jail from `/usr`/`/usr/local` (true on `ubuntu-latest`; the CLI pin is untouched). If a self-hosted runner put them elsewhere the jail would not see them — surfaced by the broker/shim readiness poll failing the job loudly (fail-closed). ## Provenance - **Authored by:** agent-work loop - **Verified:** groom unittest suite: 383 passed, 0 failed; check_workflow_pins.py: OK; check_agents_md.py: passed (2 pre-existing warnings); shellcheck agent-sandbox.sh+sandbox-tests.sh: clean; bash -n on all 45 workflow run: scripts: clean. NOT run: end-to-end groom canary (see Residual). - **Deviations:** implemented the ticket's intent via the current UDS + jail-shim composition rather than its stale TCP-8082 recipe (see "Deviation" above); also moved `PR_BODY_OUT` under the out-dir, which the ticket's file list omitted. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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