feat(groom): bwrap sandbox harness + key-broker with deterministic CI proofs (BE-4302)#66
Open
mattmillerai wants to merge 5 commits into
Open
feat(groom): bwrap sandbox harness + key-broker with deterministic CI proofs (BE-4302)#66mattmillerai wants to merge 5 commits into
mattmillerai wants to merge 5 commits into
Conversation
… proofs (BE-4302) Phase 1 of the groom auto-builder sandbox: trusted assets that confine the untrusted agent step, plus a no-API-spend CI job proving every confinement property. No changes to groom.yml's agent steps (that is phase 2), so this is mergeable independently of PR #65. - .github/groom/agent-sandbox.sh — bwrap wrapper (fail-loud preflight; never runs the command unsandboxed) that jails an arbitrary command with a read-only /usr+/etc, tmpfs /tmp+HOME, the clone bound ro or rw-git-ro, explicit ro-files, one writable out-dir, a cleared env, and its own pid namespace. - .github/groom/broker.mjs — zero-dep node reverse proxy that holds the real ANTHROPIC_API_KEY on the host, strips inbound credentials, injects the real key, forwards only /v1/* to api.anthropic.com, and streams responses (SSE). - sandbox-tests job in test-groom-scripts.yml (the PR-gating test workflow) + tests/fake-upstream.mjs: asserts env scrub, FS confinement + tmpfs shadowing, both clone modes, pid isolation, and the broker contract — no claude, no key. - README: sandbox contract + loud-preflight guarantee under the groom docs.
mattmillerai
added
agent-coded
mattmillerai
marked this pull request as ready for review
July 24, 2026 05:52
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a Bubblewrap-based agent sandbox with mandatory preflight checks, a loopback credential broker for Anthropic requests, deterministic confinement and proxy tests, documentation, and CI validation. ChangesAgent sandbox and broker
Sequence Diagram(s)sequenceDiagram
participant Agent
participant agent-sandbox.sh
participant broker.mjs
participant AnthropicEndpoint
Agent->>agent-sandbox.sh: submit command
agent-sandbox.sh->>agent-sandbox.sh: verify and create Bubblewrap jail
agent-sandbox.sh->>broker.mjs: send API request from jail
broker.mjs->>AnthropicEndpoint: forward /v1/* with host credential
AnthropicEndpoint-->>broker.mjs: stream response
broker.mjs-->>Agent: return streamed response
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
… jail) The `< /proc/self/environ` redirect resolves /proc/self in the pre-exec forked shell and reads empty inside the bwrap jail, so the env-scrub assertion saw no FOO. Reading it with `cat` (which opens post-exec, /proc/self = cat) returns the real environ. Verified end-to-end in a privileged ubuntu:24.04 container: all sandbox assertions + broker proxy pass, shellcheck clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/groom/agent-sandbox.sh:
- Around line 150-157: Update the rw-git-ro branch in the sandbox argument
construction to detect whether $clone/.git is a directory or a gitdir pointer
file. Preserve the read-only protection for directory-based metadata, and
explicitly handle worktree checkouts by exposing the pointed-to metadata inside
the jail with read-only access so git operations continue working without
permitting history or configuration writes.
- Around line 104-116: Validate that --clone, --out-dir, and every --ro-file
value are absolute paths before preflight or any bubblewrap bind construction.
Add the checks alongside the existing argument validation in the script’s main
validation flow, rejecting relative values with clear errors while preserving
the current directory and mode checks.
In @.github/groom/broker.mjs:
- Around line 60-76: Update the proxy handler around the upstream request and
response piping to handle upstream response errors, client request abort/close
events, and response cleanup without crashing or leaving streams active. Add an
appropriate upstream timeout and ensure all failure or disconnect paths
destroy/close the corresponding request and response streams, while preserving
normal streaming through upRes.pipe(res) and the existing 502 response behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb9f256e-9e2d-4815-8f5f-a24d80877aa6
📒 Files selected for processing (6)
.github/groom/README.md.github/groom/agent-sandbox.sh.github/groom/broker.mjs.github/groom/tests/fake-upstream.mjs.github/groom/tests/sandbox-tests.sh.github/workflows/test-groom-scripts.yml
… (BE-4302) Address CodeRabbit review on PR #66: - agent-sandbox.sh: require --clone/--out-dir/--ro-file to be absolute paths (bwrap binds them at their real path; a relative value would resolve against an unexpected CWD instead of failing loud, per the script's fail-closed design). - agent-sandbox.sh: rw-git-ro now fails loud when .git is a gitdir pointer file (git-worktree checkout) rather than a directory — the pointed-to metadata isn't mounted, which would silently break git instead of protecting it. - broker.mjs: handle upstream-response stream errors (previously unhandled → crashed the broker mid-stream), tear down on client abort/close, and add a 120s upstream idle timeout so a hung upstream can't pin a request open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 4 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
Addresses the still-valid cursor-review panel findings on top of 3ed4386: - broker: reject dot-segment paths (`/v1/../v2/...`) that pass a bare `/v1/` prefix check but normalize off /v1 upstream — fail closed. - broker: on an upstream error AFTER the response began streaming, tear the response down instead of splicing 'upstream error' bytes into the SSE body. - broker: add a `res.on('error')` handler — `.pipe()` installs none, so a client socket error mid-stream would crash the whole broker via uncaughtException. - agent-sandbox: reject an `--out-dir` that overlaps `--clone` (equal / nested / ancestor). It is bound rw LAST, so bwrap's last-wins ordering would otherwise shadow the read-only clone/.git mounts and silently un-protect them. Tests: overlap rejection (3 cases), dot-segment 404, and broker crash-resilience on a mid-stream client disconnect added to sandbox-tests.sh. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
agent-sandbox.sh): the agent gets a shell where it can only seea 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.
broker.mjs): the agent has no API key. It talks to a tinyproxy 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
bashcommands — noclaude,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
.github/groom/agent-sandbox.shbwrapwrapper — jails an arbitrary command; loud fail-closed preflight.github/groom/broker.mjs.github/groom/tests/sandbox-tests.sh.github/groom/tests/fake-upstream.mjsapi.anthropic.comin the broker test.github/workflows/test-groom-scripts.ymlsandbox-testsjob (ubuntu-latest).github/groom/README.mdThe wrapper matches the ticket's interface and bwrap invocation exactly
(
--unshare-all --share-net --die-with-parent --new-session --clearenv, the basemounts,
HOME/PATH/TERM+ each--env, the clone at its real path ro orrw-git-ro, ro-files, the rw out-dir,
--chdir <clone>). The broker holds the keyhost-side, deletes inbound
x-api-key/authorization, forwards only/v1/*,answers
/healthzlocally, 404s everything else, streams responses unbuffered,and logs method + path + status only.
Acceptance criteria
sandbox-testsjob onubuntu-latest— asserts env scrub (FOO/HOME/PATHpresent, a host
HOSTSECRETcanary absent), FS confinement (host$HOME/$RUNNER_TEMP/$GITHUB_WORKSPACEcanaries unreadable; writes fail except theout-dir; host
/tmp/canaryshadowed by tmpfs), both clone modes (rowritefails;
rw-git-roworktree write succeeds and is visible on the host while.git/configwrite fails), pid isolation (a hostsleep's pid absent from/proc), and the full broker contract (real key forwarded +dummystripped,/healthz200, non-/v1404, SSE streams intact).shellcheckclean onagent-sandbox.sh(verified locally; also gated inthe new job, alongside
sandbox-tests.sh).loud-preflight guarantee.
groom.ymlagent steps —groom.ymlis untouched.Judgment calls (please read)
1. The
sandbox-testsjob lives intest-groom-scripts.yml, notci-groom.yml.The ticket said extend
ci-groom.yml, butci-groom.ymlis the scheduled groomcaller 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.ymlis the repo's PR-gating, path-filtered(
.github/groom/**) home for groom's deterministic script tests, so the job runson this very change. The job is named
sandbox-testsexactly as specified. Thisfelt 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
bwrapon the dev host (macOS), so I validated the full driverend-to-end in a privileged
ubuntu:24.04container (all five property groups +the broker proxy pass, shellcheck clean) and the
sandbox-testsCI job is nowgreen on
ubuntu-latest— everyPASS:line plusALL SANDBOX TESTS PASSED.The broker layer was additionally smoke-tested standalone on the host (key
injection +
dummystrip,/healthz200, non-/v1404, 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 hasexhausted 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 referencesthe runner image's own userns support, and the
sandbox-testsjob empiricallyconfirms the sandbox comes up on
ubuntu-latest. This is a deliberatesecurity-control fail-closed, not a denial of a reachable capability.
4.
NODE_TLS_REJECT_UNAUTHORIZED=0is test-only. It's set on the test brokerprocess's env so it accepts the fake upstream's self-signed cert —
broker.mjsitself has no TLS bypass; production talks real TLS to
api.anthropic.com. Thesk-ant-TESTFAKE-…key passed via--envin the broker test is a throwawayliteral 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 bubblewrapwith no
apt-get update). bubblewrap is present on the ubuntu-latest image and thefast-path skips the install when the sandbox already works, so this is a no-op in
practice; kept literal to match the spec.