fix(worktree): reuse an existing worktree found on disk, not just via cwd - #414
Merged
Conversation
… cwd already_isolated_for(branch, repo_root) only recognizes a re-claim when the calling process is itself already running from inside that worktree (compares --git-dir vs --git-common-dir on repo_root). The daemon's normal dispatch always calls create_worktree from the main repo root, so this check never fires there, even when .worktrees/task/N already exists on disk as a valid worktree for the branch. create_worktree then falls through to `git worktree add`, which refuses to re-add an already-checked-out branch, surfacing as "claim succeeded but no worktree was created (bad git state?)" on every redispatch of an item whose worktree survived a prior claim. worktree_already_checked_out(worktree_path, branch) adds the on-disk counterpart: does a worktree already exist at this path, checked out to this branch, regardless of which process is asking. Reuses the same branch --show-current check already_isolated_for uses internally. Agentflare-Agent: claude-code Agentflare-Branch: fix-worktree-reuse-from-repo-root
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesWorktree reuse
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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: 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/flare-git-core/src/worktree.rs`:
- Around line 69-83: Update worktree_already_checked_out to validate ownership
using git worktree list --porcelain executed from repo_root, matching both the
canonical worktree_path and branch before returning true; do not rely solely on
git branch --show-current. Adjust callers and related symbols as needed to
provide repo_root, and add a negative test covering an unrelated repository at
the target 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86540761-f40d-48f9-b503-b1625fc4573f
📒 Files selected for processing (1)
crates/flare-git-core/src/worktree.rs
Comment on lines
+69
to
+83
| /// `true` if `worktree_path` already exists on disk and is itself a git | ||
| /// worktree checked out to `branch` -- the on-disk counterpart to | ||
| /// `already_isolated_for` above, for callers running from outside the | ||
| /// worktree (the daemon's normal case) rather than from inside it. | ||
| #[must_use] | ||
| fn worktree_already_checked_out(worktree_path: &Path, branch: &str) -> bool { | ||
| if !worktree_path.is_dir() { | ||
| return false; | ||
| } | ||
| match run_git_in(worktree_path, &["branch", "--show-current"]) { | ||
| Ok(b) => b == branch, | ||
| Err(_) => false, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
git -C "$tmp" init -q
git -C "$tmp" config user.email test@example.com
git -C "$tmp" config user.name Test
printf root > "$tmp/root.txt"
git -C "$tmp" add root.txt
git -C "$tmp" commit -qm init
candidate="$tmp/.worktrees/task/1"
mkdir -p "$candidate"
git -C "$candidate" init -q
git -C "$candidate" config user.email test@example.com
git -C "$candidate" config user.name Test
printf nested > "$candidate/nested.txt"
git -C "$candidate" add nested.txt
git -C "$candidate" commit -qm init
git -C "$candidate" branch -M task/1
test "$(git -C "$candidate" branch --show-current)" = "task/1"
! git -C "$tmp" worktree list --porcelain | grep -Fq "$candidate"Repository: getappz/agentflare
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper and caller ---'
sed -n '1,110p' crates/flare-git-core/src/worktree.rs
sed -n '220,270p' crates/flare-git-core/src/worktree.rs
printf '%s\n' '--- run_git_in and ownership-related calls ---'
rg -n -C 4 'fn run_git_in|worktree_already_checked_out|worktree list|worktree add|already_isolated_for' crates/flare-git-core/src/worktree.rsRepository: getappz/agentflare
Length of output: 11950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- shell module ---'
fd -i -t f 'shell' .
rg -n -C 8 'pub fn run_in|fn run_in|run_in_ok' crates
printf '%s\n' '--- relevant tests ---'
sed -n '1035,1140p' crates/flare-git-core/src/worktree.rsRepository: getappz/agentflare
Length of output: 30758
Validate worktree ownership before reusing the path.
git branch --show-current only proves that Git reports the requested branch from worktree_path. It does not prove that worktree_path is registered under repo_root. An unrelated repository at that path can cause the caller to skip worktree creation and modify the wrong checkout.
Match the path and branch against git worktree list --porcelain from repo_root. Add a negative test for an unrelated repository at the target path.
🤖 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/flare-git-core/src/worktree.rs` around lines 69 - 83, Update
worktree_already_checked_out to validate ownership using git worktree list
--porcelain executed from repo_root, matching both the canonical worktree_path
and branch before returning true; do not rely solely on git branch
--show-current. Adjust callers and related symbols as needed to provide
repo_root, and add a negative test covering an unrelated repository at the
target path.
7 tasks
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.
Summary
already_isolated_for(branch, repo_root)only recognizes a re-claim when the calling process is itself already running from inside that worktree (compares--git-dirvs--git-common-dironrepo_root). The daemon's normal dispatch always callscreate_worktreefrom the main repo root, so this check never fires there — even when.worktrees/task/Nalready exists on disk as a valid worktree for the branch.create_worktreethen falls through togit worktree add, which refuses to re-add an already-checked-out branch, surfacing as"claim succeeded but no worktree was created (bad git state?)"on every redispatch of an item whose worktree survived a prior claim attempt.Root-caused while dogfooding item #43's autonomous redispatch (repeatedly hit this exact failure); very likely the real cause behind item #30's "database is locked" report too, not a two-item concurrency collision as originally guessed there.
worktree_already_checked_out(worktree_path, branch)adds the on-disk counterpart toalready_isolated_for: does a worktree already exist at this path, checked out to this branch, regardless of which process is asking. Reuses the samebranch --show-currentcheckalready_isolated_foralready does internally.Test plan
create_worktree_reuses_an_existing_worktree_when_called_from_the_repo_root— reproduces the daemon's exact calling pattern (twocreate_worktreecalls from the repo root, never from inside the worktree); confirmed it fails first withfatal: '.../.worktrees/task/1' already exists, then passes after the fixcargo test -p flare-git-core worktree::— 23 passed, 0 failedcargo build --workspacecleancargo clippy -p flare-git-core --all-targets— no new warningscargo fmt --checkcleanSummary by CodeRabbit