bullpen run --bg --worktree: give a background session its own tree - #14
Merged
Conversation
Two background runs dispatched from the same directory edited the same files. spawn_detached sets no current_dir, so a detached child inherits the dispatcher's cwd; nothing in the store recorded where a run had been placed, and run -r derived its directory from std::env::current_dir() rather than from the session, so resuming from elsewhere silently ran somewhere else. With --worktree the session gets a git worktree at $BULLPEN_HOME/worktrees/<session_id> on a run-unique `bullpen/<id8>` branch, and both the path and the branch are persisted (schema v6). Resume goes back there without the flag. Concurrent dispatches no longer collide. Without the flag nothing about --bg changes: same cwd inheritance, same store writes, same output. The issue named four questions and left them open. Answers, as implemented: - Cleanup is fail-closed by omission. No code path here removes a worktree or deletes a branch — not on completion, failure, resume, or a timer. Nothing in this change can prove an agent's work was published, and losing the only copy of that work is far worse than leaving a directory behind. Reclaiming one stays a deliberate `git worktree remove`. ARCHITECTURE.md states the posture so a later prune command has to earn its proof-of-publish rule rather than inherit an optimistic one. - run -r on a session whose worktree is gone recreates it at the recorded path from the recorded branch, and says so on stderr. If the branch is gone, the repo is gone, or git disagrees, it fails naming both path and branch. It never falls back to the caller's cwd — that is the bug being closed. The decision is a pure `decide` over two booleans, so all four cases are tested without git. - --worktree outside a git repository is a hard error, as are a missing git binary and a failing `git worktree add`. Degrading to the shared tree would silently reintroduce exactly what the flag exists to prevent. The dispatch aborts before a session row is written, so nothing is left half-created pointing at a directory that does not exist. - The location surfaces in all three views: the `sessions` table, `sessions --json` as additive `worktree_path` / `worktree_branch` keys (null when absent — the wire contract in crates/cli/src/json.rs is a compatibility surface), and the `agents` peek panel. Dispatch also prints the path once on stderr, since otherwise the dispatching user needs a second command to learn where their run went. Notes: - The sandbox had to learn about linked worktrees. A linked worktree's admin dir and the shared object store both live outside the worktree, so under the generated Seatbelt profile an agent could edit files but never commit — `git add` died on index.lock with EPERM. Sandbox::allowing_writes takes the extra roots and worktree::git_write_roots derives them, keeping the sandbox crate ignorant of git. Under the retention rule this was load-bearing, not cosmetic: work that cannot be committed can never become the evidence that would justify reclaiming the directory. - Worktrees live under BULLPEN_HOME rather than beside the repo, so an isolated run never dirties the tree it was dispatched from. - session.cwd keeps recording the dispatch directory; the worktree goes in new columns because recreating one needs both the origin repo and the branch. - --worktree without --bg, and --worktree with --resume, are both rejected rather than ignored. A session's location is fixed at creation. Verified: 125 tests (up from 108), fmt and clippy clean. Beyond the suite, the built binary was exercised against a temp BULLPEN_HOME and a temp repo — both guards bail, dispatch outside a repo leaves no session row and no process, two concurrent dispatches land on distinct branches, `lsof -d cwd` confirms the detached child's real directory is its worktree, resume from / uses the recorded worktree and recreates it from the branch when deleted, and plain --bg produces byte-identical output with NULL columns and no worktrees/ directory. The sandbox test is mutation-checked: removing the allowing_writes call reproduces the index.lock denial. Refs #13 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3
📝 WalkthroughWalkthroughThe PR adds ChangesBackground worktree sessions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Git
participant Store
participant Sandbox
User->>CLI: run --bg --worktree
CLI->>Git: resolve repository and create worktree
CLI->>Store: persist worktree path and branch
CLI->>Sandbox: configure worktree and Git write roots
CLI-->>User: report worktree location
User->>CLI: resume session
CLI->>Store: load worktree metadata
CLI->>Git: reuse or recreate recorded worktree
CLI->>Sandbox: configure resolved run directory
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cli/src/main.rs (1)
376-401: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not retain a shared-CWD session when worktree provisioning fails.
Lines 376-382 insert the session before Line 394 creates its worktree. If
git worktree addfails, the row remains with NULL worktree fields. A laterbullpen run --resumethen runs in the shared caller directory.Delete the unconfigured session row on provisioning failure, or provision a session ID before inserting durable session state. Keep the worktree retention policy for worktrees that were created successfully.
🤖 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/cli/src/main.rs` around lines 376 - 401, Ensure the session created in the match around store.create_session is removed or otherwise not persisted if worktree::create or store.set_worktree fails, preventing resume from using a shared-CWD session. Preserve the existing session and worktree retention behavior when provisioning succeeds.
🤖 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/cli/src/worktree.rs`:
- Around line 36-40: Update branch_for to generate collision-safe branch names
by using the full session ID or an equivalent unique suffix instead of
truncating it to eight characters. Revise the function documentation to describe
the new uniqueness guarantee, and update or add tests covering distinct session
IDs and the resulting branch names.
- Around line 123-133: Update git_write_roots to return only session-specific
writable Git paths: the linked-worktree administrative directory, shared object
storage, and the recorded session branch ref and reflog, excluding broad
common_dir access to refs, configuration, and other worktree metadata. Reuse the
existing worktree/session metadata symbols and adjust tests to cover the
restricted roots, staging, and commit behavior.
- Around line 211-222: Update locate to validate the recorded worktree’s Git
common directory against anchor before decide can select Location::Use; do not
treat a plain is_dir() result as sufficient, and reject ordinary directories or
different repositories with the existing fail-closed error behavior. Preserve
the current branch_exists fallback for missing candidate directories.
In `@README.md`:
- Around line 99-100: Update the README command reference from sessions --json
to bullpen sessions --json, preserving the surrounding documentation.
---
Outside diff comments:
In `@crates/cli/src/main.rs`:
- Around line 376-401: Ensure the session created in the match around
store.create_session is removed or otherwise not persisted if worktree::create
or store.set_worktree fails, preventing resume from using a shared-CWD session.
Preserve the existing session and worktree retention behavior when provisioning
succeeds.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: beae8318-3f3a-411c-bf21-36bc54ffd6cc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
ARCHITECTURE.mdREADME.mdcrates/cli/Cargo.tomlcrates/cli/src/agents.rscrates/cli/src/main.rscrates/cli/src/worktree.rscrates/sandbox/src/lib.rscrates/store/src/lib.rs
Three fail-closed gaps in the --worktree path, all of which could place a run somewhere the session had never been: - branch_for truncated the session id to eight hex characters. Two sessions colliding on 32 bits would name one branch, which git refuses to check out in two worktrees, failing the second dispatch. Use the whole id. - locate accepted any directory at the recorded path. An ordinary directory restored there, or a worktree of a different repository, became Location::Use. The candidate is now checked against the session's own repository (shared git dir and top level both), and a foreign one is a third error, Location::Occupied, rather than a fourth place to run. - the session row was written before the worktree but the worktree fields after it, so a failed `git worktree add` left a row naming no worktree at all — which resume reads as a plain shared-cwd session. Record path and branch first: a row naming a directory that is not there is refused, which is the answer this flag exists to give. git_write_roots keeps the whole common directory, and now says why: `git gc` prunes refs/heads/bullpen/ and packs the refs into a packed-refs file at the top of the common dir, so no path allowlist can grant a worktree its own branch and nothing else. The residual risk — a sandboxed agent writing the main repository's config and hooks — is documented rather than papered over. Refs #13 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3
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/cli/src/worktree.rs`:
- Around line 249-271: Update the worktree lookup flow and inspect to accept
recorded_branch, requiring the inspected worktree’s HEAD branch to match before
returning Candidate::Live; otherwise return Candidate::Foreign. Classify any
existing non-directory path, including regular files, as Candidate::Foreign
rather than Candidate::Gone, while preserving Gone for absent paths. Add
Git-backed tests covering a same-repository worktree on a different branch and a
regular file at the recorded path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 255d0800-afc7-40a8-b682-053cb5776e99
📒 Files selected for processing (5)
ARCHITECTURE.mdREADME.mdcrates/cli/src/main.rscrates/cli/src/worktree.rscrates/store/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- ARCHITECTURE.md
- README.md
- crates/store/src/lib.rs
- crates/cli/src/main.rs
Comment on lines
+249
to
+271
| let candidate = inspect(anchor, Path::new(path)); | ||
| let branch_lives = candidate == Candidate::Gone | ||
| && repo_root(anchor).is_ok_and(|root| branch_exists(&root, branch)); | ||
| decide(Some((path, branch)), candidate, branch_lives) | ||
| } | ||
|
|
||
| /// Whether `path` really is a worktree of the repository `anchor` sits in. | ||
| /// `is_dir` alone would accept an ordinary directory left at the recorded | ||
| /// path — running there is the silent misplacement the recording exists to | ||
| /// prevent — so both the shared git directory and the worktree's own top | ||
| /// level have to agree. | ||
| fn inspect(anchor: &Path, path: &Path) -> Candidate { | ||
| if !path.is_dir() { | ||
| return Candidate::Gone; | ||
| } | ||
| let same_repo = rev_parse_dir(path, "--git-common-dir") | ||
| .zip(rev_parse_dir(anchor, "--git-common-dir")) | ||
| .is_some_and(|(candidate, anchor)| candidate == anchor); | ||
| let is_top_level = repo_root(path).is_ok_and(|top| canonical(&top) == canonical(path)); | ||
| if same_repo && is_top_level { | ||
| Candidate::Live | ||
| } else { | ||
| Candidate::Foreign |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the recorded branch and non-directory path entries.
At Line 249, inspect accepts any top-level worktree from the anchor repository. It does not verify that the worktree checks out recorded_branch.
If a user removes the session worktree and creates another worktree at the same path on a different branch, locate returns Location::Use. A resumed session can then commit to that other branch.
Also, Lines 261-263 classify a regular file at the recorded path as Candidate::Gone. If the branch exists, resume attempts recreation and fails with a generic Git error instead of returning Location::Occupied.
Pass recorded_branch into inspect. Require the worktree HEAD branch to equal it before returning Candidate::Live. Classify every existing non-directory entry as Candidate::Foreign. Add Git-backed tests for a same-repository worktree on another branch and for a regular file at the recorded path.
Also applies to: 402-445
🤖 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/cli/src/worktree.rs` around lines 249 - 271, Update the worktree
lookup flow and inspect to accept recorded_branch, requiring the inspected
worktree’s HEAD branch to match before returning Candidate::Live; otherwise
return Candidate::Foreign. Classify any existing non-directory path, including
regular files, as Candidate::Foreign rather than Candidate::Gone, while
preserving Gone for absent paths. Add Git-backed tests covering a
same-repository worktree on a different branch and a regular file at the
recorded path.
Steel-tech
added a commit
that referenced
this pull request
Aug 8, 2026
#14 shipped --bg --worktree with a review finding unresolved, and the same weak check was hiding three more. All four are one bug class: something at the recorded path that is not this session's worktree being treated as if it were. In the feature whose whole purpose is isolation, that means a resumed agent commits its work somewhere it does not belong. locate accepted recorded_branch but never passed it to inspect, and inspect asked only whether the path was a directory of the same repository. worktree of the same repo on another branch Live -> Occupied regular file at the path Gone -> Occupied dangling symlink at the path Gone -> Occupied detached worktree at the path Live -> Occupied The detached case is worth stating because the obvious implementation gets it backwards: `git symbolic-ref -q HEAD` exits nonzero when detached, so a check shaped `!status.success() || name == branch` reads command failure as agreement and accepts precisely the case it should refuse. Verified against real git rather than reasoned about. Every one of the four guards was mutation-tested — fix reverted, test confirmed failing, fix restored. That is not ceremony here: this repo has already shipped a regression guard that passed with its bug still present, which is how the branch-identity bug reached main. Workspace 131 tests, clippy clean under -D warnings, fmt clean. Refs #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3
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.
Refs #13.
What was broken
spawn_detachedsets nocurrent_dir, so a detached child inherits the dispatcher's cwd. Twobullpen run --bgdispatches from the same directory edited the same files, and nothing recorded where a run had been placed.run -r <id>compounded it:run()derived its directory fromstd::env::current_dir()and never readsession.cwd, so resuming from a different directory silently ran somewhere else.What is now possible
--worktree, with--bg, gives a session a git worktree at$BULLPEN_HOME/worktrees/<session_id>on a run-uniquebullpen/<id8>branch, persisted in the store (schema v6:worktree_path,worktree_branch). Resume goes back to that worktree without repeating the flag. Concurrent dispatches no longer collide.Plain
--bgis byte-for-byte unchanged — same cwd inheritance, same store writes, same stdout/stderr. The new columns are NULL and noworktrees/directory is created.Decisions a reviewer would otherwise have to reverse-engineer
The issue named four questions and deliberately left them open. These are the answers this PR commits to.
Cleanup is fail-closed by omission. There is no code path in this change that removes a worktree or deletes a branch — not on completion, not on failure, not on resume, not on a timer. The reasoning is asymmetric cost: nothing here can prove an agent's work was published, and losing the only copy of that work is far worse than leaving a directory behind. Reclaiming a worktree stays a deliberate human
git worktree remove. ARCHITECTURE.md records the posture explicitly so a future prune command has to earn a proof-of-publish rule rather than inherit an optimistic one from a codebase that already deletes things. Agrep -rnforremove_dir_all/remove_fileacross the workspace returns 3 hits, all inside#[cfg(test)].Resume across a missing worktree recreates when provably safe, otherwise fails.
run -ron a session with a recorded worktree runs in that worktree. If the directory is gone but the recorded branch still exists in the recorded repo, it is recreated at the same path from that branch and stderr says so. If the branch is gone, the repo is gone, or git disagrees, the run fails naming both the path and the branch. It never falls back to the caller's cwd — that fallback is the bug being closed, and it fails silently, which is the worst shape of it. The choice is a puredecide()over two booleans (directory exists, branch exists), so all four cases are covered without shelling out to git.--worktreeoutside a git repository is a hard error, as are a missinggitbinary and a failinggit worktree add. Degrading to the shared tree would silently reintroduce exactly what the flag exists to prevent. The dispatch aborts before a session row is written, so a failed--worktreeleaves no session pointing at a directory that does not exist.The location surfaces in all three views — the
sessionstable,sessions --json, and theagentspeek panel — because an isolated session whose output cannot be found just trades one problem for another. The JSON keys are additive andnullwhen absent;crates/cli/src/json.rsdocuments that stream as a compatibility surface. Dispatch also prints the path once on stderr, since otherwise the person dispatching needs a second command to learn where their own run went.The sandbox change, which is not incidental
A linked worktree's admin dir (
.git/worktrees/<name>) and the shared object store both live outside the worktree. Under the generated Seatbelt profile, whose write roots were the worktree plus temp, an agent could edit files but never commit —git adddied withUnable to create '<repo>/.git/worktrees/s1/index.lock': Operation not permitted. This was reproduced by hand before any code changed.Sandbox::allowing_writes(roots)takes the extra roots;worktree::git_write_roots(cwd)derives them from--absolute-git-dirand--git-common-dir, returning empty for an ordinary checkout and for a non-repo. Derivation lives in the CLI so the sandbox crate stays ignorant of git.Under the retention rule above this is load-bearing rather than cosmetic: work that cannot be committed can never become the evidence that would justify reclaiming its directory.
Smaller calls
BULLPEN_HOME, not beside the repo, so an isolated run never dirties the tree it was dispatched from.session.cwdkeeps recording the dispatch directory; the worktree gets new columns because recreating one needs both the origin repo and the branch.--worktreewithout--bg, and--worktreewith--resume, are rejected rather than ignored. A session's location is fixed at creation, and resuming already goes to the right place.Verification
125 tests (up from 108),
cargo fmt --all --checkclean,cargo clippy --workspace --all-targets -- -D warningsclean. New: 8 inworktree.rs, 3 for the sandbox interaction, 2 inmain.rs, 2 inagents.rs, 2 in the store (including a hand-built v5 schema that must migrate to v6).Two tests shell out to real
git. Both pass-c user.email/-c user.nameinline so they do not depend on a global identity, and assert againstcanonicalize()so macOS's/private/varsymlink does not break them.The sandbox test is mutation-checked: deleting the
.allowing_writes(...)call reproduces the exactindex.lock: Operation not permittedfailure. It uses a directory under$HOMErather than a tempdir, because tempdirs sit under the sandbox's intentionally-writable temp roots and the test would pass regardless.Beyond the suite, the built binary was driven against a temp
BULLPEN_HOMEand a temp repo:--worktreewithout--bg, and with--resume--worktreeoutside a repobullpen/<id8>brancheslsof -d cwd)sessions,sessions --json, peekrun -rfrom/[recreated worktree …]--bgworktrees/🤖 Generated with Claude Code
https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3
Summary by CodeRabbit
New Features
--worktreefor background runs, creating a dedicated Git worktree and branch per session.Bug Fixes
Documentation