Conversation
📝 WalkthroughWalkthroughBubblewrap Git protection now resolves worktree ChangesGit worktree protection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cwd/.git in a git worktree checkout is a gitfile pointing outside cwd, not a real directory -- resolve it via the admin dir's commondir file and bind the real common dir read-write when git_writable is true. Integrates with #452's git_writable parameter (Supervisor::spawn passes false, run_headless passes true). Agentflare-Branch: task/90 Agentflare-Item: 90
![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
🤖 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 `@crates/agentflare-jobs/src/sandbox/bwrap/mod.rs`:
- Around line 119-123: The sandbox argument construction must protect the
worktree gitfile in read-only mode. In the branch handling
resolve_worktree_common_dir, when git_writable is false, add a later read-only
bind for cwd/.git so it overrides the writable cwd bind; also add a regression
test for a worktree asserting the generated arguments include this --ro-bind.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da58d0dd-671a-4405-963a-dd641721a902
📒 Files selected for processing (1)
crates/agentflare-jobs/src/sandbox/bwrap/mod.rs
| if let Some(common_dir) = resolve_worktree_common_dir(cwd, &git_dir) { | ||
| let common_str = path_to_string(&common_dir); | ||
| bwrap_args.push(if git_writable { "--bind" } else { "--ro-bind" }.to_string()); | ||
| bwrap_args.push(common_str.clone()); | ||
| bwrap_args.push(common_str); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file outline ---'
ast-grep outline crates/agentflare-jobs/src/sandbox/bwrap/mod.rs || true
printf '%s\n' '--- target implementation ---'
sed -n '80,145p' crates/agentflare-jobs/src/sandbox/bwrap/mod.rs
sed -n '340,435p' crates/agentflare-jobs/src/sandbox/bwrap/mod.rs
printf '%s\n' '--- related symbols and tests ---'
rg -n -C 3 'resolve_worktree_common_dir|git_file|git_writable|ro-bind|worktree' crates/agentflare-jobsRepository: getappz/agentflare
Length of output: 30596
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- remaining implementation and tests ---'
sed -n '145,205p' crates/agentflare-jobs/src/sandbox/bwrap/mod.rs
sed -n '278,420p' crates/agentflare-jobs/src/sandbox/bwrap/mod.rs
printf '%s\n' '--- local bubblewrap availability and help ---'
if command -v bwrap >/dev/null 2>&1; then
command -v bwrap
bwrap --help 2>&1 | sed -n '1,140p'
else
echo 'bwrap not installed'
fi
printf '%s\n' '--- repository documentation references ---'
rg -n -i -C 2 'bubblewrap|bwrap|ro-bind|bind mount' README.md crates .github 2>/dev/null || trueRepository: getappz/agentflare
Length of output: 33931
🌐 Web query:
Bubblewrap bwrap man page --ro-bind file bind mount ordering later mount over earlier bind
💡 Result:
In bubblewrap (bwrap), filesystem-related options—including --bind, --ro-bind, and others—are applied strictly in the order they are provided as arguments [1][2][3]. If you specify multiple mount operations that overlap or nest, the later operations are performed on top of earlier ones [4][5]. This means: 1. Shadowing: If a later bind mount targets a directory (or a subdirectory) that was already mounted by an earlier command, the later mount will cover or "shadow" the previous one at that location [4][6]. 2. Positional Dependency: Because operations are sequential, the state of the filesystem at any given point is determined by all preceding mount operations [5]. If you mount a specific path, and then subsequently mount something onto a subdirectory of that path, the second mount will correctly appear inside the first [4]. Conversely, if you mount a "larger" filesystem over a "smaller" one (e.g., mounting / over a previously mounted /proc), the new mount will hide the previous one [5]. There is no automatic reordering or intelligent resolution of conflicting mount points; bubblewrap acts exactly as instructed, and it is the responsibility of the caller to provide the options in an order that achieves the desired final filesystem structure [5]. Top results: [4] containers/bubblewrap#612 [1] https://manpages.ubuntu.com/manpages/jammy/man1/bwrap.1.html [5] containers/bubblewrap#741 [3] https://man.archlinux.org/man/bwrap.1
Citations:
- 1: https://manpages.ubuntu.com/manpages/jammy/man1/bwrap.1.html
- 2: https://manpages.debian.org/bullseye/bubblewrap/bwrap.1.en.html
- 3: https://man.archlinux.org/man/bwrap.1
- 4: [Question] How does bwrap handle nested bindings? containers/bubblewrap#612
- 5: Working of the --proc dependent on parameter position. That's dangerous behaviour! containers/bubblewrap#741
- 6: Confusing semantics when mixing --dev and --bind /dev containers/bubblewrap#248
Protect the worktree gitfile when git_writable is false.
The writable cwd bind exposes cwd/.git, while this branch re-binds only the resolved common Git directory. Add a later --ro-bind for cwd/.git and a regression test that asserts this bind for a worktree.
🤖 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 `@crates/agentflare-jobs/src/sandbox/bwrap/mod.rs` around lines 119 - 123, The
sandbox argument construction must protect the worktree gitfile in read-only
mode. In the branch handling resolve_worktree_common_dir, when git_writable is
false, add a later read-only bind for cwd/.git so it overrides the writable cwd
bind; also add a regression test for a worktree asserting the generated
arguments include this --ro-bind.
git commit inside a worktree job (doesn't resolve the .git worktree indirection)
1 participant
The diff is correct and complete. My edits are on disk but I cannot
git add/git commitfrom inside this sandboxed worktree — every attempt fails with the exactindex.lock/read-only error this task describes, since the running daemon predates my source fix. Per the task's own note, this is expected: the daemon's outside-the-sandbox auto-commit safety net (the same one that rescued item #89) is the mechanism meant to land this.Summary
Root cause confirmed and fixed in
crates/agentflare-jobs/src/sandbox/bwrap/mod.rs.build_bwrap_args_with_homeonly ever boundcwd/.git, correct for a plain clone but wrong for agit worktreecheckout, wherecwd/.gitis a gitfile pointing at the real metadata (HEAD, index, index.lock, logs) under the main repo's.git.Fix:
symlink_metadatawhethercwd/.gitis a file (worktree gitfile) vs. a directory (plain repo), preserving the existing read-only-bind behavior for plain repos.resolve_worktree_common_dir: parses thegitdir:line, then follows that admin dir'scommondirfile (git's own documented mechanism, not an assumed../..offset) to the main repo's real.gitdirectory.--bind, not--ro-bind) — it's the shared object/ref store and the actual placeindex.lockneeds to be created.Tests added:
worktree_gitfile_resolves_main_git_dir_and_binds_it_read_write(builds a tempdir shaped like a real worktree, asserts the resolved main.gitdir gets--bind) andplain_repo_git_dir_is_still_bound_read_only(regression guard for the unchanged directory case). All 10 tests in the module pass;cargo clippyis clean.I hit the exact bug I was fixing when trying to
git commit/git addfrom inside this sandboxed worktree — the running daemon binary predates my source change, soindex.lockcreation failed with "Read-only file system," and theventescalation call failed identically, matching the incident description precisely. The fix is on disk but not committed by me — leaving it for the daemon's outside-the-sandbox auto-commit safety net, as anticipated by the task.Summary by CodeRabbit