fix(item): surface worktree creation failure reason in claim response - #318
Conversation
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
|
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:
📝 WalkthroughWalkthroughWorktree creation now returns ChangesWorktree error propagation
Shim PATH matching
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant item_claim
participant create_worktree
participant git_worktree
item_claim->>create_worktree: request worktree creation
create_worktree->>git_worktree: add worktree
git_worktree-->>create_worktree: path or creation error
create_worktree-->>item_claim: worktree_path or worktree_error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/flare-git-core/src/worktree.rs (1)
866-866: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the propagated worktree-error contract end to end.
crates/flare-git-core/src/worktree.rs#L866-L866: assert that the returned error contains the item-specific prefix and non-empty underlying git detail.src/mcp_server/item.rs#L410-L417: add anitem_claimregression test assertingworktree_erroris serialized andworktree_pathis omitted on failure.🤖 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` at line 866, The worktree failure coverage only checks that an error exists. Update the test around the assertion in the worktree error path to verify the returned error includes the item-specific prefix and non-empty underlying git details. In src/mcp_server/item.rs lines 410-417, add an item_claim regression test confirming worktree_error is serialized on failure and worktree_path is omitted.
🤖 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.
Nitpick comments:
In `@crates/flare-git-core/src/worktree.rs`:
- Line 866: The worktree failure coverage only checks that an error exists.
Update the test around the assertion in the worktree error path to verify the
returned error includes the item-specific prefix and non-empty underlying git
details. In src/mcp_server/item.rs lines 410-417, add an item_claim regression
test confirming worktree_error is serialized on failure and worktree_path is
omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d8c475e6-8070-4f46-b4fd-a22d773a907f
📒 Files selected for processing (3)
crates/flare-git-core/src/worktree.rssrc/mcp_server/item.rssrc/worktree.rs
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
|
Followed up on "is this actually circular anywhere else" by auditing every place in the codebase that excludes a shim's own directory from PATH before resolving the real binary (the class of bug PR #304 fixed for Found one more instance: Grepped for every PATH self-exclusion filter site in the repo — these two ( Verified: |
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
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
Summary
create_worktree()soft-failed on anygit worktree adderror and returnedNone, soitem_claimsilently omittedworktree_pathwith no indication why.git_binary()PATH-shim self-deny bug class (fix(flare-output): don't fail RealLlm on a benign stdin broken-pipe #234/PR feat(git): worktree orphan audit + fix git_binary shims-dir self-deny #304): a stale daemon resolvesgitback through its own shim, which unconditionally deniesgit worktree, and the denial never surfaced past stderr.create_worktreenow returnsResult<PathBuf, String>instead ofOption<PathBuf>;item_claim's response gets aworktree_errorfield when creation fails, instead of silently droppingworktree_path.Test plan
cargo build --workspace --all-featurescargo clippy -p flare-git-core --all-features -- -D warningscargo test -p flare-git-core -p agentflare worktree(27 passed)cargo test --bin agentflare item_claim(item_claim_response_includes_worktree_pathpassed)Summary by CodeRabbit