[PR #66] [MERGED] feat(groom): bwrap sandbox harness + key-broker with deterministic CI proofs (BE-4302) #78

New issue

Closed

opened 2026-10-26 22:35:59 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:35:59 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/66
Author: @mattmillerai
Created: 7/24/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4302-groom-sandbox

📝 Commits (6)

469df38 feat(groom): bwrap sandbox harness + key-broker with deterministic CI proofs (BE-4302)
b59cd67 fix(groom): make sandbox-tests cleanup SC2015-clean (explicit if over A && B || C)
8ca79f3 fix(groom): read /proc/self/environ via cat, not < redirect (empty in jail)
3ed4386 fix(groom): harden sandbox path validation + broker stream resilience (BE-4302)
7942e58 fix(groom): close residual sandbox+broker review gaps (BE-4302)
42340b3 feat(groom): isolated netns sandbox — drop --share-net, broker over a bind-mounted unix socket (BE-4421) (#73)

📊 Changes

7 files changed (+916 additions, -0 deletions)

View changed files

📝 .github/groom/README.md (+95 -0)
➕ .github/groom/agent-sandbox.sh (+240 -0)
➕ .github/groom/broker.mjs (+165 -0)
➕ .github/groom/jail-shim.mjs (+44 -0)
➕ .github/groom/tests/fake-upstream.mjs (+48 -0)
➕ .github/groom/tests/sandbox-tests.sh (+301 -0)
📝 .github/workflows/test-groom-scripts.yml (+23 -0)

📄 Description

ELI-5

The groom auto-builder lets an AI agent write code changes. We don't fully trust that agent, so before phase 2 wires it up, this PR builds the cage it will run in and a safe mail slot for its one allowed outside call:

The cage (agent-sandbox.sh): the agent gets a shell where it can only see a read-only copy of the system, a scratch tmpfs, its own repo checkout, and one folder it's allowed to write to. It cannot see the runner's other files, secrets, or even the other running processes. If the cage can't be built, we refuse to run the agent at all — we never quietly let it out.
The mail slot (broker.mjs): the agent has no API key. It talks to a tiny proxy on localhost that holds the real key, swaps it in, and forwards only to Anthropic. The key lives on the host; the agent can spend it but never read it.

Plus a CI job that proves all of this with plain bash commands — no claude, no API key, no spend. Nothing about groom's actual agent steps changes here, so it can merge on its own.

What's in this PR

File	What it is
`.github/groom/agent-sandbox.sh`	`bwrap` wrapper — jails an arbitrary command; loud fail-closed preflight
`.github/groom/broker.mjs`	zero-dep node reverse proxy that injects the real key and strips inbound ones
`.github/groom/tests/sandbox-tests.sh`	deterministic confinement + broker proofs (no spend)
`.github/groom/tests/fake-upstream.mjs`	local HTTPS stand-in for `api.anthropic.com` in the broker test
`.github/workflows/test-groom-scripts.yml`	new `sandbox-tests` job (ubuntu-latest)
`.github/groom/README.md`	sandbox contract + loud-preflight guarantee

The wrapper matches the ticket's interface and bwrap invocation exactly (--unshare-all --share-net --die-with-parent --new-session --clearenv, the base mounts, HOME/PATH/TERM + each --env, the clone at its real path ro or rw-git-ro, ro-files, the rw out-dir, --chdir <clone>). The broker holds the key host-side, deletes inbound x-api-key/authorization, forwards only /v1/*, answers /healthz locally, 404s everything else, streams responses unbuffered, and logs method + path + status only.

Acceptance criteria

sandbox-tests job on ubuntu-latest — asserts env scrub (FOO/HOME/PATH present, a host HOSTSECRET canary absent), FS confinement (host $HOME / $RUNNER_TEMP / $GITHUB_WORKSPACE canaries unreadable; writes fail except the out-dir; host /tmp/canary shadowed by tmpfs), both clone modes (ro write fails; rw-git-ro worktree write succeeds and is visible on the host while .git/config write fails), pid isolation (a host sleep's pid absent from /proc), and the full broker contract (real key forwarded + dummy stripped, /healthz 200, non-/v1 404, SSE streams intact).
shellcheck clean on agent-sandbox.sh (verified locally; also gated in the new job, alongside sandbox-tests.sh).
README section under the groom docs — the sandbox contract table + the loud-preflight guarantee.
No changes to groom.yml agent steps — groom.yml is untouched.

Judgment calls (please read)

1. The sandbox-tests job lives in test-groom-scripts.yml, not ci-groom.yml. The ticket said extend ci-groom.yml, but ci-groom.yml is the scheduled groom caller that deliberately never fires on a PR (its header + AGENTS.md: "runs weekly on a schedule … never on-PR") and spends API. A deterministic, no-spend job placed there could never be seen green on this PR — which is the acceptance criterion. test-groom-scripts.yml is the repo's PR-gating, path-filtered (.github/groom/**) home for groom's deterministic script tests, so the job runs on this very change. The job is named sandbox-tests exactly as specified. This felt clearly right, but flagging it since it deviates from the literal wording.

2. Verification status — proven both locally and in CI. I can't run unprivileged-userns bwrap on the dev host (macOS), so I validated the full driver end-to-end in a privileged ubuntu:24.04 container (all five property groups + the broker proxy pass, shellcheck clean) and the sandbox-tests CI job is now green on ubuntu-latest — every PASS: line plus ALL SANDBOX TESTS PASSED. The broker layer was additionally smoke-tested standalone on the host (key injection + dummy strip, /healthz 200, non-/v1 404, SSE pass-through).

3. Fail-closed refusal is intentional, not a capability dead-end. The preflight ends in ::error::bwrap sandbox unavailable … refusing to run the agent unsandboxed + non-zero exit. That refusal fires only after the preflight has exhausted every path to establish the sandbox (install bubblewrap, install the AppArmor unconfined profile mirroring the runner image's podman workaround, drop kernel.apparmor_restrict_unprivileged_userns, and retest). The ticket references the runner image's own userns support, and the sandbox-tests job empirically confirms the sandbox comes up on ubuntu-latest. This is a deliberate security-control fail-closed, not a denial of a reachable capability.

4. NODE_TLS_REJECT_UNAUTHORIZED=0 is test-only. It's set on the test broker process's env so it accepts the fake upstream's self-signed cert — broker.mjs itself has no TLS bypass; production talks real TLS to api.anthropic.com. The sk-ant-TESTFAKE-… key passed via --env in the broker test is a throwaway literal used to prove forwarding; the real-key-never-in-jail property is a production concern the test doesn't (and shouldn't) touch.

5. Preflight matches the ticket verbatim (sudo apt-get install -y bubblewrap with no apt-get update). bubblewrap is present on the ubuntu-latest image and the fast-path skips the install when the sandbox already works, so this is a no-op in practice; kept literal to match the spec.

_{🔄 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/66 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/24/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4302-groom-sandbox` --- ### 📝 Commits (6) - [`469df38`](https://github.com/Comfy-Org/github-workflows/commit/469df382b2a95cbda144080da9c278c785f6f2a2) feat(groom): bwrap sandbox harness + key-broker with deterministic CI proofs (BE-4302) - [`b59cd67`](https://github.com/Comfy-Org/github-workflows/commit/b59cd67365892b4cf3e9c3c43f3e733127941179) fix(groom): make sandbox-tests cleanup SC2015-clean (explicit if over A && B || C) - [`8ca79f3`](https://github.com/Comfy-Org/github-workflows/commit/8ca79f332900f3c32a91e8d58cb791bc30d0e8e3) fix(groom): read /proc/self/environ via cat, not < redirect (empty in jail) - [`3ed4386`](https://github.com/Comfy-Org/github-workflows/commit/3ed4386052a6e4e4f06aee505559aa6f9629de66) fix(groom): harden sandbox path validation + broker stream resilience (BE-4302) - [`7942e58`](https://github.com/Comfy-Org/github-workflows/commit/7942e58a18672a421ab36c9a45a14309fdced825) fix(groom): close residual sandbox+broker review gaps (BE-4302) - [`42340b3`](https://github.com/Comfy-Org/github-workflows/commit/42340b3dcb5f12fa5747071f7730ee3901728298) feat(groom): isolated netns sandbox — drop --share-net, broker over a bind-mounted unix socket (BE-4421) (#73) ### 📊 Changes **7 files changed** (+916 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+95 -0) ➕ `.github/groom/agent-sandbox.sh` (+240 -0) ➕ `.github/groom/broker.mjs` (+165 -0) ➕ `.github/groom/jail-shim.mjs` (+44 -0) ➕ `.github/groom/tests/fake-upstream.mjs` (+48 -0) ➕ `.github/groom/tests/sandbox-tests.sh` (+301 -0) 📝 `.github/workflows/test-groom-scripts.yml` (+23 -0) </details> ### 📄 Description ## ELI-5 The groom auto-builder lets an AI agent write code changes. We don't fully trust that agent, so before phase 2 wires it up, this PR builds the **cage** it will run in and a **safe mail slot** for its one allowed outside call: - **The cage** (`agent-sandbox.sh`): the agent gets a shell where it can only see a read-only copy of the system, a scratch tmpfs, its own repo checkout, and one folder it's allowed to write to. It cannot see the runner's other files, secrets, or even the other running processes. If the cage can't be built, we **refuse to run the agent at all** — we never quietly let it out. - **The mail slot** (`broker.mjs`): the agent has no API key. It talks to a tiny proxy on localhost that holds the real key, swaps it in, and forwards only to Anthropic. The key lives on the host; the agent can spend it but never read it. Plus a CI job that **proves** all of this with plain `bash` commands — no `claude`, no API key, **no spend**. Nothing about groom's actual agent steps changes here, so it can merge on its own. ## What's in this PR | File | What it is | |---|---| | `.github/groom/agent-sandbox.sh` | `bwrap` wrapper — jails an arbitrary command; loud fail-closed preflight | | `.github/groom/broker.mjs` | zero-dep node reverse proxy that injects the real key and strips inbound ones | | `.github/groom/tests/sandbox-tests.sh` | deterministic confinement + broker proofs (no spend) | | `.github/groom/tests/fake-upstream.mjs` | local HTTPS stand-in for `api.anthropic.com` in the broker test | | `.github/workflows/test-groom-scripts.yml` | new `sandbox-tests` job (ubuntu-latest) | | `.github/groom/README.md` | sandbox contract + loud-preflight guarantee | The wrapper matches the ticket's interface and bwrap invocation exactly (`--unshare-all --share-net --die-with-parent --new-session --clearenv`, the base mounts, `HOME`/`PATH`/`TERM` + each `--env`, the clone at its real path ro or rw-git-ro, ro-files, the rw out-dir, `--chdir <clone>`). The broker holds the key host-side, deletes inbound `x-api-key`/`authorization`, forwards only `/v1/*`, answers `/healthz` locally, 404s everything else, streams responses unbuffered, and logs method + path + status only. ## Acceptance criteria - [x] **`sandbox-tests` job on `ubuntu-latest`** — asserts env scrub (FOO/HOME/PATH present, a host `HOSTSECRET` canary absent), FS confinement (host `$HOME` / `$RUNNER_TEMP` / `$GITHUB_WORKSPACE` canaries unreadable; writes fail except the out-dir; host `/tmp/canary` shadowed by tmpfs), both clone modes (`ro` write fails; `rw-git-ro` worktree write **succeeds and is visible on the host** while `.git/config` write fails), pid isolation (a host `sleep`'s pid absent from `/proc`), and the full broker contract (real key forwarded + `dummy` stripped, `/healthz` 200, non-`/v1` 404, SSE streams intact). - [x] **`shellcheck` clean** on `agent-sandbox.sh` (verified locally; also gated in the new job, alongside `sandbox-tests.sh`). - [x] **README section** under the groom docs — the sandbox contract table + the loud-preflight guarantee. - [x] **No changes to `groom.yml` agent steps** — `groom.yml` is untouched. ## Judgment calls (please read) **1. The `sandbox-tests` job lives in `test-groom-scripts.yml`, not `ci-groom.yml`.** The ticket said extend `ci-groom.yml`, but `ci-groom.yml` is the scheduled groom *caller* that **deliberately never fires on a PR** (its header + AGENTS.md: "runs weekly on a schedule … never on-PR") and spends API. A deterministic, no-spend job placed there could never be *seen green on this PR* — which is the acceptance criterion. `test-groom-scripts.yml` is the repo's PR-gating, path-filtered (`.github/groom/**`) home for groom's deterministic script tests, so the job runs on this very change. The job is named `sandbox-tests` exactly as specified. This felt clearly right, but flagging it since it deviates from the literal wording. **2. Verification status — proven both locally and in CI.** I can't run unprivileged-userns `bwrap` on the dev host (macOS), so I validated the full driver end-to-end in a privileged `ubuntu:24.04` container (all five property groups + the broker proxy pass, shellcheck clean) and the `sandbox-tests` CI job is now **green on `ubuntu-latest`** — every `PASS:` line plus `ALL SANDBOX TESTS PASSED`. The broker layer was additionally smoke-tested standalone on the host (key injection + `dummy` strip, `/healthz` 200, non-`/v1` 404, SSE pass-through). **3. Fail-closed refusal is intentional, not a capability dead-end.** The preflight ends in `::error::bwrap sandbox unavailable … refusing to run the agent unsandboxed` + non-zero exit. That refusal fires **only after** the preflight has exhausted every path to establish the sandbox (install bubblewrap, install the AppArmor unconfined profile mirroring the runner image's podman workaround, drop `kernel.apparmor_restrict_unprivileged_userns`, and retest). The ticket references the runner image's own userns support, and the `sandbox-tests` job empirically confirms the sandbox comes up on `ubuntu-latest`. This is a deliberate security-control fail-closed, not a denial of a reachable capability. **4. `NODE_TLS_REJECT_UNAUTHORIZED=0` is test-only.** It's set on the *test* broker process's env so it accepts the fake upstream's self-signed cert — `broker.mjs` itself has no TLS bypass; production talks real TLS to `api.anthropic.com`. The `sk-ant-TESTFAKE-…` key passed via `--env` in the broker test is a throwaway literal used to prove *forwarding*; the real-key-never-in-jail property is a production concern the test doesn't (and shouldn't) touch. **5. Preflight matches the ticket verbatim** (`sudo apt-get install -y bubblewrap` with no `apt-get update`). bubblewrap is present on the ubuntu-latest image and the fast-path skips the install when the sandbox already works, so this is a no-op in practice; kept literal to match the spec. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:35:59 +00:00