Repository navigation
Merged
Conversation
…s up its own worktree Two bugs from a cluster of vents (item #420): - item_done unconditionally pushed and opened a PR whenever a target branch resolved, with no way to just mark an item done locally. Add push: Option<bool> to ItemRequest (default true, back-compat), and skip push_and_open_pr when false. - claim provisions a worktree but nothing ever removed it, leaving orphaned .worktrees/task/<id> directories behind after every done -- and the git shim blocks removing one by hand, a dead end. done now calls cleanup_item_worktree, which only removes a CLEAN worktree (checked via git status --porcelain, not inferred from push/PR success) so uncommitted work is never at risk; a dirty one is left in place with a log line. - ensure_worktrees_ignored also excludes .cargo/ now, the per-worktree cargo config isolate_worktree_target_dir writes into every worktree it creates -- left unignored, it made every freshly-created worktree look dirty to the new cleanup check before any real work happened.
|
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:
📝 WalkthroughWalkthrough
ChangesItem review lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant item_done
participant GitHub
participant Backend
participant WorktreeCleanup
Client->>item_done: complete item with optional push
item_done->>GitHub: publish branch and create PR when enabled
item_done->>Backend: retain item in review when a PR exists
Client->>Backend: request merge check
Backend->>GitHub: check PR merged_at
Backend->>WorktreeCleanup: complete merged item and clean worktree
WorktreeCleanup-->>Client: return lifecycle status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
getappz
changed the title
![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 633-669: The cleanup_item_worktree function must verify that the
target path is registered by repo_root as the expected task/<sequence_id>
worktree before invoking gc_orphans. Reject and preserve any unregistered or
mismatched directory, and add a regression test covering an unrelated clean Git
repository at that path remaining intact.
🪄 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: f1312e2b-8843-472b-b0af-14b1766ebaf5
📒 Files selected for processing (5)
crates/flare-git-core/src/worktree.rssrc/mcp_server/item.rssrc/mcp_server/tests/action_tests.rssrc/mcp_server/types.rssrc/worktree.rs
Comment on lines
+633
to
+669
| /// Removes `item`'s own worktree once its work is done, if it's safe to. | ||
| /// | ||
| /// `create_worktree` provisions a directory per item but nothing ever | ||
| /// removed it (item #420) — every completed item left an orphaned | ||
| /// `.worktrees/task/<id>` behind, and the git shim blocks removing one by | ||
| /// hand. Deleting a linked worktree's directory only drops the checkout, | ||
| /// never the branch or its commits (those live in the shared object store), | ||
| /// so the one real risk is uncommitted changes that exist only in that | ||
| /// checkout. Refuses (logs, returns `false`) on a dirty tree or a failed | ||
| /// status check rather than guessing from push/PR outcome — a caller can | ||
| /// mark an item done without ever pushing, and deleting then would be | ||
| /// destructive. | ||
| pub fn cleanup_item_worktree(item: &Item, repo_root: &Path) -> bool { | ||
| let name = item.sequence_id.to_string(); | ||
| let worktree_path = repo_root.join(".worktrees").join("task").join(&name); | ||
| if !worktree_path.exists() { | ||
| return false; | ||
| } | ||
| match run_git_in(&worktree_path, &["status", "--porcelain"]) { | ||
| Ok(out) if out.trim().is_empty() => {} | ||
| Ok(_) => { | ||
| eprintln!( | ||
| "worktree: leaving {} in place — it has uncommitted changes", | ||
| worktree_path.display() | ||
| ); | ||
| return false; | ||
| } | ||
| Err(e) => { | ||
| eprintln!( | ||
| "worktree: leaving {} in place — could not check its status: {e}", | ||
| worktree_path.display() | ||
| ); | ||
| return false; | ||
| } | ||
| } | ||
| !gc_orphans(repo_root, &[name]).is_empty() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify worktree identity before deletion.
Line 648 accepts any existing directory at .worktrees/task/<sequence_id>. Lines 651-668 delete it when its own Git status is clean. An unrelated Git repository at that path passes this check and is recursively deleted when item_done runs.
Confirm that the path is registered by this repository as the expected task/<sequence_id> worktree before calling gc_orphans. Add a regression test that preserves an unrelated clean repository at this 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 633 - 669, The
cleanup_item_worktree function must verify that the target path is registered by
repo_root as the expected task/<sequence_id> worktree before invoking
gc_orphans. Reject and preserve any unregistered or mismatched directory, and
add a regression test covering an unrelated clean Git repository at that path
remaining intact.
…d merged item_done previously marked an item Completed and deleted its worktree the instant a PR was opened, before it was reviewed or merged -- we nearly lost the ability to push CodeRabbit-requested follow-up commits to PR #381/#382 fixing exactly this. done now decides its state transition AFTER attempting the push: - a PR resulting (new or already-open) moves the item to a new in_review state instead of completed, keeps the claim lease held so nobody else can claim it mid-review, and leaves the worktree in place for follow-up commits - nothing resulting (push: false, or no new commits) goes straight to completed as before, releases the lease, and cleans up the worktree item(action=check_merge) promotes in_review -> completed once the PR is confirmed merged via the GitHub API, and only then cleans up the worktree. Soft-fails like push_and_open_pr when GitHub can't be reached, so it's safe to call speculatively. in_review is backfilled automatically for projects seeded before this state existed (mark_in_review self-heals via state::create on NotFound), so no migration is needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/github/models.rs (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
merged_atdeserialization.
check_mergedecides promotion from this field only. The existing test at Lines 252-267 omitsmerged_at, so only thedefaultpath is covered. Add both a null case and a timestamp case.💚 Proposed test
#[test] fn pull_request_reads_merged_at() { let base = |merged_at: serde_json::Value| { serde_json::json!({ "number": 5, "html_url": "u", "state": "closed", "title": "t", "merged_at": merged_at, }) }; let unmerged: PullRequest = serde_json::from_value(base(serde_json::Value::Null)).unwrap(); assert_eq!(unmerged.merged_at, None); let merged: PullRequest = serde_json::from_value(base("2026-07-19T00:00:00Z".into())).unwrap(); assert_eq!(merged.merged_at.as_deref(), Some("2026-07-19T00:00:00Z")); }🤖 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 `@src/github/models.rs` around lines 24 - 29, Add a test near the existing PullRequest deserialization tests that constructs JSON with merged_at set to null and to a timestamp, deserializes each into PullRequest, and asserts None for the null case and the exact timestamp for the populated case. Keep the existing default-path coverage unchanged.src/mcp_server/tests/action_tests.rs (1)
640-643: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest the public lifecycle success paths.
item_done_with_a_resulting_pr_moves_to_in_review_not_completedcallsmark_in_reviewdirectly. It does not testitem_done, itspushbehavior, itspr_urlresult, or its response.The merge tests only cover no-op paths. They do not prove that
check_mergepromotes a merged item, releases its lease, and cleans its worktree.Inject controllable publication and merge-status dependencies. Then test both success paths through
s.item(...).
src/mcp_server/tests/action_tests.rs#L640-L643: simulate a resulting PR and assert the publicdoneresponse,in_reviewstate, unsetcompleted_at, and retained lease.src/mcp_server/tests/action_tests.rs#L738-L759: simulate a merged PR and assert completion, lease release, and worktree cleanup.🤖 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 `@src/mcp_server/tests/action_tests.rs` around lines 640 - 643, The tests in src/mcp_server/tests/action_tests.rs:640-643 must exercise the public s.item(...) done flow instead of calling item::mark_in_review directly: inject controllable publication dependencies to simulate a resulting PR, then assert the done response, in_review state, unset completed_at, and retained lease. At src/mcp_server/tests/action_tests.rs:738-759, inject controllable merge-status dependencies to simulate a merged PR and exercise s.item(...) check_merge, asserting completion, lease release, and worktree cleanup.
🤖 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 `@src/mcp_server/item.rs`:
- Around line 585-626: Update the transition handling around mark_in_review and
mark_completed so done is true only when completion succeeds, never when an item
merely enters review. Derive status from the actual transition result, reporting
in_review only when mark_in_review returns true and unchanged when it returns
false; preserve completed behavior for successful mark_completed transitions.
Add an item_done test covering the review path and asserting both done and
status response fields.
---
Nitpick comments:
In `@src/github/models.rs`:
- Around line 24-29: Add a test near the existing PullRequest deserialization
tests that constructs JSON with merged_at set to null and to a timestamp,
deserializes each into PullRequest, and asserts None for the null case and the
exact timestamp for the populated case. Keep the existing default-path coverage
unchanged.
In `@src/mcp_server/tests/action_tests.rs`:
- Around line 640-643: The tests in src/mcp_server/tests/action_tests.rs:640-643
must exercise the public s.item(...) done flow instead of calling
item::mark_in_review directly: inject controllable publication dependencies to
simulate a resulting PR, then assert the done response, in_review state, unset
completed_at, and retained lease. At
src/mcp_server/tests/action_tests.rs:738-759, inject controllable merge-status
dependencies to simulate a merged PR and exercise s.item(...) check_merge,
asserting completion, lease release, and worktree cleanup.
🪄 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: ebc3ca17-909c-4c29-a7d4-483a055ac8fa
📒 Files selected for processing (10)
crates/agentflare-backend/src/item.rscrates/agentflare-backend/src/project.rscrates/agentflare-backend/src/state.rssrc/github/models.rssrc/mcp_server.rssrc/mcp_server/builtin_tools.rssrc/mcp_server/item.rssrc/mcp_server/tests/action_tests.rssrc/mcp_server/types.rssrc/worktree.rs
Comment on lines
+585
to
+626
| let in_review = owns_claim && pr_url.is_some(); | ||
| let done = if !owns_claim { | ||
| false | ||
| } else if in_review { | ||
| self.with_backend_db(|conn| { | ||
| agentflare_backend::item::mark_in_review(conn, &item_id, &owner) | ||
| .map_err(map_backend_err) | ||
| })?? | ||
| } else { | ||
| let moved = self.with_backend_db(|conn| { | ||
| agentflare_backend::item::mark_completed(conn, &item_id, &owner) | ||
| .map_err(map_backend_err) | ||
| })??; | ||
| if moved { | ||
| if let Some(item) = &item { | ||
| crate::worktree::cleanup_worktree(item, &repo_root); | ||
| } | ||
| Err(e) => { | ||
| eprintln!("worktree: failed to release claim for item {item_id}: {e:?}") | ||
| match self.with_backend_db(|conn| { | ||
| agentflare_backend::claim::done(conn, &item_id, &owner, now) | ||
| }) { | ||
| Ok(Ok(true)) => {} | ||
| Ok(Ok(false)) => eprintln!( | ||
| "worktree: releasing claim for item {item_id} affected no rows (owner mismatch or already released)" | ||
| ), | ||
| Ok(Err(e)) => { | ||
| eprintln!("worktree: failed to release claim for item {item_id}: {e}") | ||
| } | ||
| Err(e) => { | ||
| eprintln!("worktree: failed to release claim for item {item_id}: {e:?}") | ||
| } | ||
| } | ||
| } | ||
| } | ||
| let mut resp = serde_json::json!({"done": done, "item_id": item_id}); | ||
| moved | ||
| }; | ||
| let status = if in_review { | ||
| "in_review" | ||
| } else if done { | ||
| "completed" | ||
| } else { | ||
| "unchanged" | ||
| }; | ||
| let mut resp = serde_json::json!({"done": done, "item_id": item_id, "status": status}); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Derive done and status from the transition that actually happened.
Two response-shape problems come from the same root cause: done and status are computed from in_review instead of from the transition result.
- In the review path,
donereceives the result ofmark_in_review, so a successful transition returns"done": truewith"status": "in_review". The item is not completed. A caller that keys ondone— the pre-existing field — treats an item with an open, unreviewed PR as finished. That is the regression this PR removes at the state level, but the response still reports it. - If
mark_in_reviewreturnsfalse(the lease changed owner between the read and the write),in_reviewis stilltrue, sostatusreports"in_review"while nothing changed anddoneisfalse.
Bind done to completion only, and bind status to the observed transition.
🐛 Proposed fix
- let in_review = owns_claim && pr_url.is_some();
- let done = if !owns_claim {
- false
- } else if in_review {
- self.with_backend_db(|conn| {
- agentflare_backend::item::mark_in_review(conn, &item_id, &owner)
- .map_err(map_backend_err)
- })??
- } else {
+ let wants_review = owns_claim && pr_url.is_some();
+ let mut moved_to_review = false;
+ let done = if !owns_claim {
+ false
+ } else if wants_review {
+ moved_to_review = self.with_backend_db(|conn| {
+ agentflare_backend::item::mark_in_review(conn, &item_id, &owner)
+ .map_err(map_backend_err)
+ })??;
+ false
+ } else {
@@
moved
};
- let status = if in_review {
+ let status = if moved_to_review {
"in_review"
} else if done {
"completed"
} else {
"unchanged"
};Note: src/mcp_server/tests/action_tests.rs drives mark_in_review directly rather than through item_done, so no current test pins the done value on the review path. Add a case that asserts the response fields for that path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let in_review = owns_claim && pr_url.is_some(); | |
| let done = if !owns_claim { | |
| false | |
| } else if in_review { | |
| self.with_backend_db(|conn| { | |
| agentflare_backend::item::mark_in_review(conn, &item_id, &owner) | |
| .map_err(map_backend_err) | |
| })?? | |
| } else { | |
| let moved = self.with_backend_db(|conn| { | |
| agentflare_backend::item::mark_completed(conn, &item_id, &owner) | |
| .map_err(map_backend_err) | |
| })??; | |
| if moved { | |
| if let Some(item) = &item { | |
| crate::worktree::cleanup_worktree(item, &repo_root); | |
| } | |
| Err(e) => { | |
| eprintln!("worktree: failed to release claim for item {item_id}: {e:?}") | |
| match self.with_backend_db(|conn| { | |
| agentflare_backend::claim::done(conn, &item_id, &owner, now) | |
| }) { | |
| Ok(Ok(true)) => {} | |
| Ok(Ok(false)) => eprintln!( | |
| "worktree: releasing claim for item {item_id} affected no rows (owner mismatch or already released)" | |
| ), | |
| Ok(Err(e)) => { | |
| eprintln!("worktree: failed to release claim for item {item_id}: {e}") | |
| } | |
| Err(e) => { | |
| eprintln!("worktree: failed to release claim for item {item_id}: {e:?}") | |
| } | |
| } | |
| } | |
| } | |
| let mut resp = serde_json::json!({"done": done, "item_id": item_id}); | |
| moved | |
| }; | |
| let status = if in_review { | |
| "in_review" | |
| } else if done { | |
| "completed" | |
| } else { | |
| "unchanged" | |
| }; | |
| let mut resp = serde_json::json!({"done": done, "item_id": item_id, "status": status}); | |
| let wants_review = owns_claim && pr_url.is_some(); | |
| let mut moved_to_review = false; | |
| let done = if !owns_claim { | |
| false | |
| } else if wants_review { | |
| moved_to_review = self.with_backend_db(|conn| { | |
| agentflare_backend::item::mark_in_review(conn, &item_id, &owner) | |
| .map_err(map_backend_err) | |
| })??; | |
| false | |
| } else { | |
| let moved = self.with_backend_db(|conn| { | |
| agentflare_backend::item::mark_completed(conn, &item_id, &owner) | |
| .map_err(map_backend_err) | |
| })??; | |
| if moved { | |
| if let Some(item) = &item { | |
| crate::worktree::cleanup_worktree(item, &repo_root); | |
| } | |
| match self.with_backend_db(|conn| { | |
| agentflare_backend::claim::done(conn, &item_id, &owner, now) | |
| }) { | |
| Ok(Ok(true)) => {} | |
| Ok(Ok(false)) => eprintln!( | |
| "worktree: releasing claim for item {item_id} affected no rows (owner mismatch or already released)" | |
| ), | |
| Ok(Err(e)) => { | |
| eprintln!("worktree: failed to release claim for item {item_id}: {e}") | |
| } | |
| Err(e) => { | |
| eprintln!("worktree: failed to release claim for item {item_id}: {e:?}") | |
| } | |
| } | |
| } | |
| moved | |
| }; | |
| let status = if moved_to_review { | |
| "in_review" | |
| } else if done { | |
| "completed" | |
| } else { | |
| "unchanged" | |
| }; | |
| let mut resp = serde_json::json!({"done": done, "item_id": item_id, "status": status}); |
🤖 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 `@src/mcp_server/item.rs` around lines 585 - 626, Update the transition
handling around mark_in_review and mark_completed so done is true only when
completion succeeds, never when an item merely enters review. Derive status from
the actual transition result, reporting in_review only when mark_in_review
returns true and unchanged when it returns false; preserve completed behavior
for successful mark_completed transitions. Add an item_done test covering the
review path and asserting both done and status response fields.
This was referenced Aug 4, 2026
3 tasks done
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
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.
Auto-opened on
item donefor N3V4uVO6zs5F2_rMuGPdo.Summary by CodeRabbit
New Features
Bug Fixes