fix(agent): grant the linked worktree's git dirs to the OS sandbox (P7-SANDBOX-01)#142
Open
Reederey87 wants to merge 1 commit into
Open
fix(agent): grant the linked worktree's git dirs to the OS sandbox (P7-SANDBOX-01)#142Reederey87 wants to merge 1 commit into
Reederey87 wants to merge 1 commit into
Conversation
…7-SANDBOX-01)
A DevStrap agent worktree is a git *linked* worktree whose index/objects/refs
live in the parent clone's .git, outside the worktree write-allow. Under the
default guarded policy with --sandbox auto (the common Mac dev host), the write
confinement kernel-EPERM'd every `git add`/`git commit`, so `agent pr` had
nothing to push — the core agent loop silently broke, untested (the only
sandbox+git canary used a fresh nested repo).
New git.Runner.WorktreeSandboxWriteDirs resolves the linked worktree's
<git-common-dir>/{objects,refs,logs} + the per-worktree admin dir; all three
sandbox backends (Seatbelt, bubblewrap, Landlock) and the --read-confine
allow-list now grant them. The common dir's hooks/ and config are deliberately
excluded (per-subpath, never the whole .git) — granting them would let a
confined agent plant a hook/config that runs UNSANDBOXED on a later git op.
Proven by a live Seatbelt e2e (TestSeatbeltAllowsLinkedWorktreeCommit): the
commit FAILS without the grant and SUCCEEDS with it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds ChangesLinked worktree git sandbox write grants
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/git/sandbox_writedirs_test.go (1)
17-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for the main (non-linked) checkout branch.
Only the linked-worktree and non-repo-at-all cases are exercised. The
gitDirAbs == commonAbsbranch inWorktreeSandboxWriteDirs(main checkout returning just objects/refs/logs, no admin dir) has no direct test coverage here.🤖 Prompt for 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. In `@internal/git/sandbox_writedirs_test.go` around lines 17 - 58, Add a test case in TestWorktreeSandboxWriteDirs that exercises the main checkout path where gitDirAbs equals commonAbs in WorktreeSandboxWriteDirs; verify it returns only the expected common grants for objects, refs, and logs, and does not include a per-worktree admin dir. Use the existing WorktreeSandboxWriteDirs helper and compare against the current linked-worktree assertions so the non-linked branch is explicitly covered.internal/git/git.go (2)
601-613: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueBoundary check accepts
rel == ".", i.e.gitDirAbs == worktreesRoot.
filepath.Rel(worktreesRoot, gitDirAbs)returns"."(not an error) when the two paths are equal, andrel != ".." && !strings.HasPrefix(rel, "../")doesn't exclude that case — so a--git-dirresolving to exactly<common>/worktrees(rather than a subdirectory under it) would be granted wholesale instead of being refused. Given the explicit hardening intent of this function ("keeping the hooks/config exclusion robust against hostile setups"), tightening the check to also rejectrel == "."costs nothing and removes the ambiguity, even though ordinary git worktree layouts (confirmed via docs) never produce--git-dir == <common>/worktreesitself.🔒️ Suggested tightening
- if rel, rerr := filepath.Rel(worktreesRoot, gitDirAbs); rerr == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + if rel, rerr := filepath.Rel(worktreesRoot, gitDirAbs); rerr == nil && rel != "." && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { dirs = append(dirs, gitDirAbs) }Since git validates the target of a worktree gitdir file before returning it, I'm not fully certain this is reachable in practice — flagging for confirmation.
🤖 Prompt for 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. In `@internal/git/git.go` around lines 601 - 613, The boundary check in the git worktree admin-dir handling is too permissive because `filepath.Rel(worktreesRoot, gitDirAbs)` can return `"."` when `gitDirAbs` equals the `worktreesRoot` itself, causing `gitDirAbs` to be added even though it is not a child worktree. Update the `gitDirAbs != commonAbs` / `worktreesRoot` check in `internal/git/git.go` to explicitly reject `rel == "."` while still allowing only true descendants, preserving the hardening intent around `dirs` and `worktreesRoot`.
557-565: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAny
git rev-parsefailure is silently treated as "not a worktree."Both rev-parse calls collapse every error (missing binary, transient I/O, timeout, permission issue) into
return nil, nil. Since callers (e.g.agent.go) discard the error too, a genuine linked worktree could silently get zero write grants on a transient git failure, producing a confusing sandbox denial with no diagnosable cause.r.Runalready returns aCommandErrorwith aKindfield (seeclassifyGitError/ErrTimeoutusage above in this file) that could be used to distinguish a real "not a git repository" condition from other failures worth surfacing/logging.♻️ Sketch
common, err := r.Run(ctx, dir, "rev-parse", "--git-common-dir") if err != nil { - return nil, nil // not a git worktree — nothing to grant + var cmdErr CommandError + if errors.As(err, &cmdErr) && cmdErr.Kind == ErrNotARepo { + return nil, nil // not a git worktree — nothing to grant + } + return nil, err // surface unexpected failures instead of masking them }🤖 Prompt for 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. In `@internal/git/git.go` around lines 557 - 565, WorktreeSandboxWriteDirs currently treats every git rev-parse failure as “not a worktree,” which hides real failures. Update Runner.WorktreeSandboxWriteDirs to inspect the error returned by r.Run and only return nil, nil for the genuine non-repository case; for other CommandError kinds (for example timeout, I/O, permission, missing binary) surface or log the error instead. Use the existing CommandError.Kind handling and classifyGitError/ErrTimeout patterns in internal/git/git.go to keep the no-worktree path distinct from actionable failures.internal/cli/agent_sandbox_test.go (1)
101-109: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for
GitDirsthreading throughagentSandboxSpec.Both updated tests pass
nilfor the newgitDirsparam and neither assertsspec.GitDirs. Given this is the CLI-level wiring for the P7-SANDBOX-01 fix, a regression here (e.g.gitDirsparam dropped or ignored insideagentSandboxSpec) wouldn't be caught by any test in this cohort —sandbox_gitdirs_test.goonly covers the platform-backend consumption ofspec.GitDirs, not that the CLI actually populates it.✅ Suggested addition to `TestAgentSandboxSpecAnchorsRealUserHome`
func TestAgentSandboxSpecAnchorsRealUserHome(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) - spec, err := agentSandboxSpec("/wt", "/tmp/run", "/log", nil, agentSandboxLaunch{devstrapHome: "/dsh", denyNetwork: true}, "arun_test") + gitDirs := []string{"/common/objects", "/common/refs"} + spec, err := agentSandboxSpec("/wt", "/tmp/run", "/log", gitDirs, agentSandboxLaunch{devstrapHome: "/dsh", denyNetwork: true}, "arun_test") if err != nil { t.Fatalf("agentSandboxSpec: %v", err) } + if !slices.Equal(spec.GitDirs, gitDirs) { + t.Fatalf("GitDirs = %v, want %v", spec.GitDirs, gitDirs) + } if spec.UserHome != home {🤖 Prompt for 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. In `@internal/cli/agent_sandbox_test.go` around lines 101 - 109, The CLI wiring in agentSandboxSpec is not covered for the new GitDirs path because the current tests pass nil and never assert spec.GitDirs. Update TestAgentSandboxSpecAnchorsRealUserHome (and/or add a sibling test) to pass a non-nil gitDirs value into agentSandboxSpec and verify the returned spec populates GitDirs correctly, so regressions where the parameter is dropped or ignored are caught at the agentSandboxSpec boundary.
🤖 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 `@internal/git/sandbox_writedirs_test.go`:
- Around line 73-98: Add test coverage for the plain-checkout branch where
gitDirAbs equals commonAbs in a normal repository checkout; the current
`mustEval`, `contains`, and `gitBinOrSkip` helpers are fine, but the sandbox
write-dir tests need a new case that exercises the regular checkout path in
addition to the existing linked worktree and non-repo scenarios. Update
`internal/git/sandbox_writedirs_test.go` to set up or use a standard checkout
and assert the `gitDirAbs == commonAbs` branch is reached.
---
Nitpick comments:
In `@internal/cli/agent_sandbox_test.go`:
- Around line 101-109: The CLI wiring in agentSandboxSpec is not covered for the
new GitDirs path because the current tests pass nil and never assert
spec.GitDirs. Update TestAgentSandboxSpecAnchorsRealUserHome (and/or add a
sibling test) to pass a non-nil gitDirs value into agentSandboxSpec and verify
the returned spec populates GitDirs correctly, so regressions where the
parameter is dropped or ignored are caught at the agentSandboxSpec boundary.
In `@internal/git/git.go`:
- Around line 601-613: The boundary check in the git worktree admin-dir handling
is too permissive because `filepath.Rel(worktreesRoot, gitDirAbs)` can return
`"."` when `gitDirAbs` equals the `worktreesRoot` itself, causing `gitDirAbs` to
be added even though it is not a child worktree. Update the `gitDirAbs !=
commonAbs` / `worktreesRoot` check in `internal/git/git.go` to explicitly reject
`rel == "."` while still allowing only true descendants, preserving the
hardening intent around `dirs` and `worktreesRoot`.
- Around line 557-565: WorktreeSandboxWriteDirs currently treats every git
rev-parse failure as “not a worktree,” which hides real failures. Update
Runner.WorktreeSandboxWriteDirs to inspect the error returned by r.Run and only
return nil, nil for the genuine non-repository case; for other CommandError
kinds (for example timeout, I/O, permission, missing binary) surface or log the
error instead. Use the existing CommandError.Kind handling and
classifyGitError/ErrTimeout patterns in internal/git/git.go to keep the
no-worktree path distinct from actionable failures.
In `@internal/git/sandbox_writedirs_test.go`:
- Around line 17-58: Add a test case in TestWorktreeSandboxWriteDirs that
exercises the main checkout path where gitDirAbs equals commonAbs in
WorktreeSandboxWriteDirs; verify it returns only the expected common grants for
objects, refs, and logs, and does not include a per-worktree admin dir. Use the
existing WorktreeSandboxWriteDirs helper and compare against the current
linked-worktree assertions so the non-linked branch is explicitly covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c984efc-cc68-4f9a-925f-720bb861064e
📒 Files selected for processing (15)
internal/cli/agent.gointernal/cli/agent_sandbox_test.gointernal/git/git.gointernal/git/sandbox_writedirs_test.gointernal/platform/sandbox.gointernal/platform/sandbox_bwrap_args.gointernal/platform/sandbox_gitdirs_test.gointernal/platform/sandbox_landlock.gointernal/platform/sandbox_profile.gointernal/platform/sandbox_read_confine.gointernal/platform/sandbox_worktree_commit_darwin_test.gospec/05_MAC_FIRST_IMPLEMENTATION.mdspec/10_AGENT_WORKSPACES_AND_POLICIES.mdspec/15_SECURITY_THREAT_MODEL.mdspec/18_WORK_LOG.md
Comment on lines
+73
to
+98
| func mustEval(t *testing.T, p string) string { | ||
| t.Helper() | ||
| r, err := filepath.EvalSymlinks(p) | ||
| if err != nil { | ||
| return filepath.Clean(p) | ||
| } | ||
| return r | ||
| } | ||
|
|
||
| func contains(s []string, v string) bool { | ||
| for _, x := range s { | ||
| if x == v { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func gitBinOrSkip(t *testing.T) string { | ||
| t.Helper() | ||
| bin, err := exec.LookPath("git") | ||
| if err != nil { | ||
| t.Skip("git not installed") | ||
| } | ||
| return bin | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=go '^func (contains|mustEval|gitBinOrSkip)\(' internal/gitRepository: Reederey87/DevStrap
Length of output: 414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package files =="
git ls-files internal/git/*.go
echo
echo "== helper name search across internal/git =="
rg -n --type=go '^func (contains|mustEval|gitBinOrSkip)\(' internal/git
echo
echo "== tests around sandbox_writedirs =="
ast-grep outline internal/git/sandbox_writedirs_test.go --view expanded || true
echo
echo "== implementation branch references =="
rg -n 'gitDirAbs == commonAbs|commonAbs|linked-worktree|sandbox_writedirs' internal/gitRepository: Reederey87/DevStrap
Length of output: 1353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/git/sandbox_writedirs_test.go (relevant sections) =="
sed -n '1,170p' internal/git/sandbox_writedirs_test.go
echo
echo "== internal/git/git.go (relevant section) =="
sed -n '560,630p' internal/git/git.goRepository: Reederey87/DevStrap
Length of output: 5566
Cover the plain-checkout branch too Add a case for the gitDirAbs == commonAbs path in a regular checkout; the current tests only cover linked worktrees and non-repos.
🤖 Prompt for 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.
In `@internal/git/sandbox_writedirs_test.go` around lines 73 - 98, Add test
coverage for the plain-checkout branch where gitDirAbs equals commonAbs in a
normal repository checkout; the current `mustEval`, `contains`, and
`gitBinOrSkip` helpers are fine, but the sandbox write-dir tests need a new case
that exercises the regular checkout path in addition to the existing linked
worktree and non-repo scenarios. Update `internal/git/sandbox_writedirs_test.go`
to set up or use a standard checkout and assert the `gitDirAbs == commonAbs`
branch is reached.
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
1 participant
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.
Fixes P7-SANDBOX-01 (P1) from the Pass-7 audit (#141).
The bug
A DevStrap agent worktree is a git linked worktree whose index/objects/refs live in the parent clone's
.git, outside the sandbox write-allow (worktree + tmp only). Under the defaultguardedpolicy with--sandbox auto(the common Mac dev host), the kernel EPERM'd everygit add/git commit, soagent prhad nothing to push — the core "agent commits →agent prpushes" loop silently broke. It was untested: the only sandbox+git canary committed into a fresh nested repo, never exercising the linked-worktree path.The fix
New
git.Runner.WorktreeSandboxWriteDirsresolves the linked worktree's git storage a commit writes —<git-common-dir>/{objects,refs,logs}+ the per-worktree admin dir (--git-dir) — and all three sandbox backends (Seatbelt, bubblewrap, Landlock) plus the--read-confineallow-list now grant them.Security-critical design: the grant is per-subpath, never the common dir root —
hooks/andconfigare withheld, because granting write there would let a confined agent plant a hook/config that executes unsandboxed on a later git op (a sandbox escape). This per-subpath approach is also required because Landlock cannot carve a read-only hole out of an RW grant. The resolver additionally refuses a--git-dirthat resolves outside<common>/worktrees/(hardening against a malformed gitfile/commondir).Proof
Load-bearing e2e on the real Mac Seatbelt kernel sandbox (
TestSeatbeltAllowsLinkedWorktreeCommit, env-gated):git add && git commitin a real linked worktree fails without the grant and succeeds with it, landing on the branch. Plus unit coverage for the resolver's security invariants (never the common root/hooks/config), and the Seatbelt/bwrap/read-confine wiring.Review
Dual review applied (opus — clean/ship-it; Codex gpt-5.5 — two minors fixed): the
<common>/worktrees/validation, symlink-resolving the object/ref subpaths, gating resolution on sandbox-enabled, and a warning on an unexpectedly-empty grant. Deferred by agreement (latent —readonlyblocks commits): the--read-confinecommon-configread path (tracked in the work-log follow-ups, alongside the pre-existing, not-widened commondir-redirect note).Gates
gofmt·go vet· golangci-lint 0 issues ·go test -race ./...green · spec-drift passes (specs 05/10/15 + work-log).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
git addandgit commit, while still blocking sensitive Git hooks and config files.Bug Fixes