feat(git): worktree orphan audit + fix git_binary shims-dir self-deny - #304
Merged
Conversation
ensure_on_path prepends ~/.agentflare/shims to the user's persistent PATH. git_binary()'s exclusion filter only excluded this process's own dir and cargo target dirs, so agentflare's own worktree creation (create_worktree, called from the item claim flow) resolved git back to its own PATH shim -- which denies 'worktree' unconditionally, and the denial looked like an ordinary git error to the soft-fail-on-error caller, silently deadlocking the claim flow. Also ports mise's paths_eq comparator (case-insensitive, separator- normalized) instead of raw PathBuf equality, since a case or / vs mismatch on Windows/macOS would silently reintroduce the same bug. Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: feat/worktree-audit-294
…shot (#294) agentflare git audit preview: lists .worktrees/task/* dirs with a broken .git gitdir pointer, excluding any directory whose name matches a live claimed item's sequence_id. agentflare git audit prune [names...|--all]: removes the listed orphans, snapshotting each one first (same pre-destructive-snapshot rationale as reset --hard/clean -f), then runs 'git worktree prune' to clear git's own metadata. Automates the manual cleanup done in the 2026-07-21 branch/worktree sweep (37 branches -> 13 kept, .worktrees/ 942MB -> 316KB). Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: feat/worktree-audit-294
|
Warning Review limit reached
Next review available in: 21 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds orphan worktree auditing and pruning through the Git CLI, protects active claimed items, exports claim helpers, and filters Agentflare shim paths during git binary resolution. ChangesOrphan worktree audit
Git PATH filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant GitCLI
participant ClaimDatabase
participant WorktreeFilesystem
participant Git
Operator->>GitCLI: audit preview or prune
GitCLI->>ClaimDatabase: load active claimed sequence IDs
GitCLI->>WorktreeFilesystem: scan orphan task worktrees
WorktreeFilesystem-->>GitCLI: return orphan records
GitCLI->>WorktreeFilesystem: delete selected orphan directories
WorktreeFilesystem->>Git: run git worktree prune
GitCLI-->>Operator: print audit or pruning results
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…or cases The combined test hand-built a backslash Windows path and asserted it matches on macOS too, but macOS never treats '\' as a separator -- Path::components() splits the two inputs differently there, so the assertion was wrong for that platform, not the paths_eq logic. CI caught it on build (macos-latest). Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: feat/worktree-audit-294
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/agentflare-backend/src/claim.rs (1)
46-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTTL lookup duplicated across crates — risk of config drift.
This mirrors
src/claims.rs::ttl_secs(), which reads the sameAGENTFLARE_CLAIM_TTL_SECSenv var but falls back to a different-looking constant (DEFAULT_TTL_SECS) vs. the hardcoded1800here. Now thatdefault_ttl_secs()ispub, consider havingclaims::ttl_secs()delegate to this function (or vice versa) so the two claim subsystems can't silently diverge if one fallback is changed without the other.♻️ Suggested consolidation direction
-pub fn ttl_secs() -> i64 { - std::env::var("AGENTFLARE_CLAIM_TTL_SECS") - .ok() - .and_then(|s| s.parse::<u64>().ok()) - .unwrap_or(DEFAULT_TTL_SECS) as i64 -} +pub fn ttl_secs() -> i64 { + agentflare_backend::claim::default_ttl_secs() +}🤖 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-backend/src/claim.rs` around lines 46 - 51, Consolidate the duplicated AGENTFLARE_CLAIM_TTL_SECS lookup by making claims::ttl_secs() and default_ttl_secs() share a single implementation and fallback constant. Update the other function to delegate to the chosen canonical helper, preserving the existing parsed environment value and fallback behavior so future changes cannot diverge.
🤖 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/shell.rs`:
- Around line 209-236: Correct
paths_eq_matches_case_and_separator_variants_on_windows_and_macos by separating
platform-specific assertions: keep the drive-letter and separator-variant paths
under Windows, and use forward-slash paths differing only in case for macOS.
Preserve the existing case-sensitive-platform assertion and unrelated-path
check.
- Around line 41-53: Update agentflare_shims_dir() to resolve the home directory
through crate::paths::home() instead of calling dirs::home_dir() directly,
preserving the existing .agentflare/shims path construction and ensuring
AGENTFLARE_HOME_OVERRIDE is honored.
In `@crates/flare-git-core/src/worktree.rs`:
- Around line 491-498: Update the WalkDir traversal in audit_orphans to process
entries individually instead of collecting into Result<Vec<_>, _>. Skip
unreadable or errored entries while continuing to inspect valid entries,
preserving any orphaned worktrees discovered so partial results are returned
rather than immediately returning the current orphans.
- Around line 548-573: Update gc_orphans to check the Result from
snapshot_before before calling remove_dir_all. If snapshot creation fails,
report the failure for that orphan and continue without deleting its worktree;
only proceed with removal after a successful snapshot.
In `@src/cli/git.rs`:
- Around line 448-451: Move the repository-root doc comment so it directly
documents resolve_repo_root, and leave claimed_sequence_ids documented only by
its own “Build set of claimed item sequence_ids from the DB” description. Ensure
neither function retains a misleading or misplaced doc comment.
---
Nitpick comments:
In `@crates/agentflare-backend/src/claim.rs`:
- Around line 46-51: Consolidate the duplicated AGENTFLARE_CLAIM_TTL_SECS lookup
by making claims::ttl_secs() and default_ttl_secs() share a single
implementation and fallback constant. Update the other function to delegate to
the chosen canonical helper, preserving the existing parsed environment value
and fallback behavior so future changes cannot diverge.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e0fa14d-a50b-449e-9763-5de09ab66652
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/agentflare-backend/src/claim.rscrates/flare-git-core/Cargo.tomlcrates/flare-git-core/src/shell.rscrates/flare-git-core/src/worktree.rssrc/cli/git.rs
Comment on lines
+41
to
+53
| /// `~/.agentflare/shims` -- the PATH-shim install dir (mirrored here since | ||
| /// this crate can't depend on the main `agentflare` crate's `shim_install` | ||
| /// module). Must be excluded from `git_binary()`'s search the same way | ||
| /// `self_dir` is: `ensure_on_path` (`src/cli/git.rs`) prepends this dir to | ||
| /// the user's persistent PATH, so an unfiltered search resolves straight | ||
| /// back to the `git` PATH shim -- which classifies `worktree` as | ||
| /// always-deny (see `classify.rs`), making agentflare's OWN worktree | ||
| /// creation (`create_worktree`, called from the `item` claim flow) | ||
| /// self-deadlock silently: the shim's denial looks like an ordinary git | ||
| /// error to the soft-fail-on-error caller, so no error ever surfaces. | ||
| fn agentflare_shims_dir() -> Option<PathBuf> { | ||
| dirs::home_dir().map(|h| h.join(".agentflare").join("shims")) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the home() helper used by shims_dir() in src/cli/git.rs
rg -nP -C5 '\bfn\s+home\s*\(' src/cli/git.rs
ast-grep run --pattern 'fn home($$$) { $$$ }' --lang rust src/cli/git.rsRepository: getappz/agentflare
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'crates/flare-git-core/src/shell.rs' 'src/cli/git.rs'
printf '\n== shell.rs slice ==\n'
sed -n '1,260p' crates/flare-git-core/src/shell.rs | nl -ba | sed -n '1,260p'
printf '\n== git.rs slice around shims_dir/home ==\n'
rg -n -C 8 '\bfn\s+home\s*\(|\bfn\s+shims_dir\s*\(' src/cli/git.rs
printf '\n== git.rs around matching lines ==\n'
sed -n '170,240p' src/cli/git.rs | nl -ba | sed -n '1,120p'Repository: getappz/agentflare
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== shell.rs (lines 1-260 with numbers) ==\n'
awk 'NR>=1 && NR<=260 { printf "%4d %s\n", NR, $0 }' crates/flare-git-core/src/shell.rs
printf '\n== git.rs (lines 1-280 with numbers) ==\n'
awk 'NR>=1 && NR<=280 { printf "%4d %s\n", NR, $0 }' src/cli/git.rs
printf '\n== search for home helper usages ==\n'
rg -n -C 3 '\bhome\(\)' src/cli/git.rs crates/flare-git-core/src/shell.rsRepository: getappz/agentflare
Length of output: 24247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== home() definition search ==\n'
rg -n -C 6 '\bfn\s+home\s*\(' src crates
printf '\n== paths_eq test search ==\n'
rg -n -C 6 '\bpaths_eq\b|agentflare_shims_dir_is_excludable_from_git_binary_search' crates/flare-git-core/src/shell.rs src
printf '\n== home helper files ==\n'
git ls-files | rg '(^|/)(paths|home)\.rs$|src/paths\.rs|src/.*home.*\.rs'Repository: getappz/agentflare
Length of output: 6309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AGENTFLARE_HOME_OVERRIDE usages ==\n'
rg -n -C 4 'AGENTFLARE_HOME_OVERRIDE|home\(\)' src crates
printf '\n== call sites of git_binary / run_in / run_in_opt ==\n'
rg -n -C 4 '\bgit_binary\(\)|\brun_in\(|\brun_in_opt\(|\brun_in_ok\(' src cratesRepository: getappz/agentflare
Length of output: 50375
Mirror AGENTFLARE_HOME_OVERRIDE when building the shims path. agentflare_shims_dir() uses dirs::home_dir() directly, while the rest of the app resolves home through crate::paths::home(). That mismatch can make git_binary() miss ~/.agentflare/shims under temp-home/override-based runs and let the shim back into 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/shell.rs` around lines 41 - 53, Update
agentflare_shims_dir() to resolve the home directory through
crate::paths::home() instead of calling dirs::home_dir() directly, preserving
the existing .agentflare/shims path construction and ensuring
AGENTFLARE_HOME_OVERRIDE is honored.
- audit_orphans: filter_map instead of collect::<Result<Vec<_>,_>>() -- one unreadable subdirectory no longer aborts the whole scan and silently reports 'no orphans found'. - gc_orphans: abort deletion of an orphan if its pre-delete snapshot fails, instead of discarding the error and deleting anyway with no recovery point. - claimed_sequence_ids/resolve_repo_root: fix a doc comment that ended up describing the wrong function after an earlier insertion. Agentflare-Agent: claude-code_2-1-217_agent Agentflare-Branch: feat/worktree-audit-294
This was referenced Jul 22, 2026
getappz
added a commit
that referenced
this pull request
Jul 24, 2026
Audited every place the codebase excludes a shim's own directory from PATH before resolving the real binary, to confirm the git_binary() self-deny bug class (PR #304) isn't still reachable elsewhere. Found one more: path_without_shim_dir() (shared exec plumbing used by both the flare-git-shim and agentflare-shim binaries) filtered shim_dir with a byte-equal PathBuf comparison. On Windows/macOS a case or / vs mismatch between shim_dir and PATH would leave the shim's own directory in the filtered PATH, so run_real()'s which::which_in could resolve back to the shim itself -- the same self-deny/recursion risk git_binary() was fixed for, just in the shim's own passthrough path instead of agentflare's internal git calls. The existing FLARE_GIT_SHIM_DEPTH backstop would cap the blast radius, but this closes the actual gap rather than relying on the backstop. Ports the same paths_eq() normalization already proven in flare_git_core::shell::git_binary() (duplicated rather than cross-crate-depended on: agentflare-shim is deliberately dependency- light generic plumbing, not git-specific). No other self-exclusion filter sites exist in the codebase (grepped for every filter(...) over PATH entries). Agentflare-Agent: claude-code_2-1-218_agent Agentflare-Branch: fix/claim-worktree-error-visibility
getappz
added a commit
that referenced
this pull request
Jul 24, 2026
…#318) * fix(item): surface worktree creation failure reason in claim response create_worktree() soft-failed on any git error and returned None, so item_claim silently omitted worktree_path with no indication why. This reads to the caller as an unexplained claim/worktree deadlock (hit live in appz-cli, indistinguishable from a real circular dependency) instead of the one known cause: a stale daemon's git_binary() PATH-shim self-deny (PR #304's bug class recurring). create_worktree now returns Result<PathBuf, String>; item_claim adds a worktree_error field to the acquired response when creation fails, so the failure is visible instead of silent. Agentflare-Agent: claude-code_2-1-218_agent Agentflare-Branch: fix/claim-worktree-error-visibility * fix(agentflare-shim): case/separator-normalize shim-dir PATH exclusion Audited every place the codebase excludes a shim's own directory from PATH before resolving the real binary, to confirm the git_binary() self-deny bug class (PR #304) isn't still reachable elsewhere. Found one more: path_without_shim_dir() (shared exec plumbing used by both the flare-git-shim and agentflare-shim binaries) filtered shim_dir with a byte-equal PathBuf comparison. On Windows/macOS a case or / vs mismatch between shim_dir and PATH would leave the shim's own directory in the filtered PATH, so run_real()'s which::which_in could resolve back to the shim itself -- the same self-deny/recursion risk git_binary() was fixed for, just in the shim's own passthrough path instead of agentflare's internal git calls. The existing FLARE_GIT_SHIM_DEPTH backstop would cap the blast radius, but this closes the actual gap rather than relying on the backstop. Ports the same paths_eq() normalization already proven in flare_git_core::shell::git_binary() (duplicated rather than cross-crate-depended on: agentflare-shim is deliberately dependency- light generic plumbing, not git-specific). No other self-exclusion filter sites exist in the codebase (grepped for every filter(...) over PATH entries). Agentflare-Agent: claude-code_2-1-218_agent Agentflare-Branch: fix/claim-worktree-error-visibility * fix(agentflare-shim): satisfy fmt + move test mod to end of file cargo fmt wanted the multi-line closure/assert wrapped differently. clippy::items_after_test_module flagged tool_name_from_exe/run_real being defined after the #[cfg(test)] mod tests block -- moved the test module to the end of the file, which also matches convention everywhere else in this codebase. Agentflare-Agent: claude-code_2-1-218_agent Agentflare-Branch: fix/claim-worktree-error-visibility * test(item): cover the worktree_error contract end to end Addresses CodeRabbit nitpick on PR #318: the create_worktree failure test only asserted an error existed, not its content, and item_claim had no regression test for the failure path at all (only success). - create_worktree_soft_fails_on_bad_git now asserts the error names the failing item and carries the underlying git failure detail, not just the format-string prefix. - New item_claim_response_includes_worktree_error_instead_of_silently_omitting_it claims an item against a non-repo worktree_repo_root_override and asserts worktree_path is absent while worktree_error is present and non-empty. Agentflare-Agent: claude-code_2-1-218_agent Agentflare-Branch: fix/claim-worktree-error-visibility
This was referenced Jul 25, 2026
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
agentflare git audit preview|prune [--all]: previews/removes orphaned.worktrees/task/*dirs (broken.gitgitdir pointer), excluding anything matching a live claimed item. Prune snapshots before removing, then runsgit worktree prune. Closes item Feature: Embedded Anthropic→OpenAI proxy with multi-backend failover #294.git_binary()(crates/flare-git-core/src/shell.rs) resolving back to agentflare's owngitPATH shim once~/.agentflare/shimsis prepended to PATH (item feat(dev-install): build and install PATH shims alongside the main binary #301's dev-install shim bundling) — the shim unconditionally deniesgit worktree, so agentflare's owncreate_worktree(theitemclaim flow) was silently self-deadlocking. Also ports mise's case-insensitivepaths_eqcomparator for the exclusion check.Test plan
cargo fmt --checkcargo clippy -p flare-git-core -p agentflare-backend --all-features -- -D warningscargo test -p flare-git-core -p agentflare-backend— 84 passedSummary by CodeRabbit
New Features
agentflare git auditto preview orphaned worktrees and identify their locations, sizes, and status.Bug Fixes