Skip to content

fix(worktree): never let doctor reclaim delete the main worktree - #327

Merged
getappz merged 1 commit into
masterfrom
task/348
Jul 25, 2026
Merged

fix(worktree): never let doctor reclaim delete the main worktree#327
getappz merged 1 commit into
masterfrom
task/348

Conversation

@getappz

@getappz getappz commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • Incident 2026-07-25: agentflare git doctor --reclaim deleted an entire canonical checkout (.git, all source) plus every linked worktree nested under it (.worktrees/task/*), including two with real uncommitted work. Root cause: reclaim() in crates/flare-git-core/src/doctor.rs treated the main/canonical worktree exactly like any linked worktree -- any HealthFlag it happened to pick up (MissingUpstream, Stale, etc.) made it a std::fs::remove_dir_all target. Because linked worktrees live nested under the main one (.worktrees/<name>), deleting the main worktree cascaded and destroyed them too, bypassing their own individually-correct dirty-check protection.
  • git worktree list always lists the main worktree first (documented, stable git behavior) -- tags that lane (LaneHealth::is_main_worktree) and hard-skips it in reclaim() unconditionally, even under --force, matching how real git worktree remove already refuses to ever remove the main worktree.
  • Regression test reproduces the exact incident shape in a temp repo: a main worktree naturally flagged MissingUpstream (but clean) plus a dirty linked worktree nested under it; asserts the main worktree, its .git, and the linked worktree all survive reclaim(..., force=false). Verified the test actually catches the bug by temporarily reverting the guard and confirming it fails with exactly the pre-fix symptom.

Follow-ups (filed as item #348, not addressed here)

  • is_dirty() did not flag the actual main worktree as dirty in the live incident despite real uncommitted modifications -- couldn't reproduce post-incident since the tree was gone; needs its own investigation.
  • snapshot_before(repo_root, ...) (crates/flare-git-core/src/snapshot.rs) writes its pre-destructive safety-net commit as a ref inside repo_root/.git -- for the main-worktree case, that's the exact tree being deleted, so the safety net gives zero protection against the one failure mode it most needs to cover. Worth relocating snapshot storage outside any single lane's own directory.

Test plan

  • cargo test -p flare-git-core -- 116 passed, 0 failed
  • cargo clippy -p flare-git-core --all-targets --all-features -- -D warnings -- clean
  • cargo fmt -- clean
  • Confirmed the new regression test fails (reproducing the incident) when the is_main_worktree guard is temporarily removed, and passes with it restored

Summary by CodeRabbit

  • Bug Fixes
    • Protected the main worktree from accidental reclamation, even when forced.
    • Preserved nested linked worktrees during cleanup.
    • Improved worktree health reporting by identifying the main worktree explicitly.

reclaim() treated the main/canonical worktree exactly like any other
lane -- any HealthFlag it happened to pick up (MissingUpstream, Stale,
etc.) made it std::fs::remove_dir_all-eligible. Since linked worktrees
live nested under it (.worktrees/<name>), deleting the main worktree
cascaded and destroyed every linked worktree too, bypassing their own
per-lane dirty-check protection entirely.

git worktree list always lists the main worktree first; tag that lane
and hard-skip it in reclaim(), unconditionally, even under --force --
matching how real git worktree remove already refuses to ever remove
the main worktree.

Regression test reproduces the exact incident shape: a main worktree
flagged MissingUpstream (but clean) plus a dirty linked worktree
nested under it; asserts the main worktree, its .git, and the linked
worktree all survive reclaim(..., false).
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The doctor scan now marks the canonical worktree in each LaneHealth entry. Reclaim skips that worktree even with force enabled, and a regression test verifies nested linked worktrees are preserved.

Changes

Main worktree protection

Layer / File(s) Summary
Worktree health marker
crates/flare-git-core/src/doctor.rs
LaneHealth records whether a lane is the canonical worktree, and scan populates the field for existing and missing paths.
Reclaim guard and regression test
crates/flare-git-core/src/doctor.rs
reclaim skips the canonical worktree, while the regression test verifies that it and a nested linked worktree remain present.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: preventing doctor reclaim from deleting the main worktree.
Description check ✅ Passed The description covers the required Summary and Test plan sections and is sufficiently complete, though Notes for reviewers are not explicitly formatted.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch task/348

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
crates/flare-git-core/src/doctor.rs (2)

572-668: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Also cover --force in the regression test.

The doc comment on is_main_worktree and the PR intent both emphasize the main worktree must be protected "regardless of flags or --force", but this test only calls reclaim(&repo.path, &report, false). Since the guard sits before the force/dirty checks, a force=true call would currently pass too, but the explicit "even under --force" guarantee isn't actually exercised by any test.

✅ Suggested additional assertion
         assert!(
             !reclaimed.contains(&main_name),
             "main worktree must never be reported as reclaimed"
         );
+
+        let reclaimed_forced = reclaim(&repo.path, &report, true);
+        assert!(
+            repo.path.exists() && repo.path.join(".git").exists(),
+            "main worktree must survive reclaim even with --force"
+        );
+        assert!(
+            !reclaimed_forced.contains(&main_name),
+            "main worktree must never be reclaimed, even with --force"
+        );
     }
🤖 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/doctor.rs` around lines 572 - 668, Extend
reclaim_never_deletes_the_main_worktree_even_when_flagged to exercise reclaim
with force=true, while preserving the existing setup and survival assertions.
Ensure the test explicitly verifies that the main worktree remains present and
is not included in the reclaimed results under --force.

271-294: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider git worktree remove as defense-in-depth instead of raw remove_dir_all.

Reclaim still deletes linked worktrees via std::fs::remove_dir_all + a follow-up worktree prune, bypassing git's own built-in refusal to remove the main worktree. The new is_main_worktree boolean is now the only safeguard against a repeat of this incident; using git worktree remove [--force] here would give a second, git-native line of defense (and drop the need for the separate prune call), for future lane types or flag combinations that might not correctly set is_main_worktree.

♻️ Sketch of using git's own removal
         let path = Path::new(&lane.path);
         if path.exists() {
             if let Err(e) = crate::snapshot::snapshot_before(
                 repo_root,
                 &format!("doctor reclaim {}", lane.name),
             ) {
                 eprintln!(
                     "doctor: snapshot failed before reclaiming {}: {}",
                     lane.name, e
                 );
                 continue;
             }
-            match std::fs::remove_dir_all(path) {
-                Ok(()) => {
-                    let _ = crate::shell::run_in(repo_root, &["worktree", "prune"]);
-                    reclaimed.push(lane.name.clone());
-                }
-                Err(e) => {
-                    eprintln!("doctor: failed to reclaim '{}': {}", lane.name, e);
-                }
-            }
+            let mut args = vec!["worktree", "remove"];
+            if force {
+                args.push("--force");
+            }
+            args.push(&lane.path);
+            match crate::shell::run_in(repo_root, &args) {
+                Ok(_) => reclaimed.push(lane.name.clone()),
+                Err(e) => eprintln!("doctor: failed to reclaim '{}': {}", lane.name, e),
+            }
         } else {
🤖 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/doctor.rs` around lines 271 - 294, Replace the raw
std::fs::remove_dir_all and follow-up worktree prune in the reclaim flow with
Git’s worktree removal command, using the appropriate force behavior for
reclaiming lanes. Preserve the existing success handling that records reclaimed
lanes and the failure logging, while relying on git worktree remove to protect
the main worktree; update the surrounding match to handle the command result.
🤖 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/doctor.rs`:
- Around line 572-668: Extend
reclaim_never_deletes_the_main_worktree_even_when_flagged to exercise reclaim
with force=true, while preserving the existing setup and survival assertions.
Ensure the test explicitly verifies that the main worktree remains present and
is not included in the reclaimed results under --force.
- Around line 271-294: Replace the raw std::fs::remove_dir_all and follow-up
worktree prune in the reclaim flow with Git’s worktree removal command, using
the appropriate force behavior for reclaiming lanes. Preserve the existing
success handling that records reclaimed lanes and the failure logging, while
relying on git worktree remove to protect the main worktree; update the
surrounding match to handle the command result.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6845df32-fb09-46f0-a15c-40aefde183f3

📥 Commits

Reviewing files that changed from the base of the PR and between 7fadab2 and 76c97ad.

📒 Files selected for processing (1)
  • crates/flare-git-core/src/doctor.rs

@getappz
getappz merged commit e88cccd into master Jul 25, 2026
17 checks passed
@getappz
getappz deleted the task/348 branch July 25, 2026 07:10
@getappz
getappz restored the task/348 branch July 25, 2026 07:10
getappz added a commit that referenced this pull request Jul 25, 2026
… already merged (#329)

push_and_open_pr() unconditionally called pulls::create() for the
item's branch. GitHub's API only rejects a duplicate while an existing
PR is still open -- once it's merged (or manually closed), a second PR
against the same branch is perfectly legal, which is exactly how
item done re-running on an already-merged item opened a redundant PR
(2026-07-25: PR #328 duplicating already-merged #327).

Add pulls::find_existing() to check for any PR (open/merged/closed)
matching the branch before creating a new one, and use it in
push_and_open_pr -- if found, reuse its url instead of opening a
duplicate.
getappz added a commit that referenced this pull request Jul 26, 2026
…on-blocking-fetch test (#344)

* fix(flare-docs): caller-vs-service error mapping, search limit cap, non-blocking-fetch test

Addresses the actionable subset of the PR #316 whole-branch review follow-ups
(item #327):

- 404/bad package name now returns invalid_params instead of internal_error.
  FetchError gains a structured Status(u16) variant (ureq::Error::Status was
  previously flattened into a string), and a ClientError trait lets
  blocking_fetch discriminate 4xx (caller's fault) from 5xx/transport/timeout
  (ours). npm's NoTypes counts as caller-caused too.
- search limit is capped at 50, enforced inside DocsStore::search so the MCP
  tool and the CLI both inherit it rather than each guarding separately.
- the non-2xx re-check in UreqFetcher::fetch is documented as deliberate
  rather than dead: ureq only auto-errors on >= 400, so 1xx/3xx still arrive
  as Ok. It now returns the same structured Status variant.
- committed regression test for the spawn_blocking fix (83f76ad), which was
  previously only proven by an ad-hoc uncommitted script. Verified it fails
  ("only 0 ticks elapsed") when the fetch is made inline.

Two of the six findings needed no change: the zstd output cap already landed
(MAX_DECOMPRESSED_BYTES + read_capped), and CLI `get` now has its own
cache-checking arm, so its "or read from cache" help text is accurate.

* fix(flare-docs): don't map retryable 4xx to invalid_params; honour list limit

Self-review follow-ups on this branch:

- 408 and 429 are 4xx but retryable — the request was well-formed and the
  caller needs to back off, not fix its arguments. Mapping them to
  invalid_params told an agent to correct a request that was never wrong.
  They now stay internal_error.
- the `limit` schema documents a ceiling for both search and list, but the
  list action ignored the field entirely, so the documented cap was a promise
  the tool did not keep. list still returns every cached document by default;
  an explicit limit is now honoured and capped. Description reworded to state
  both behaviours exactly.

* fix(flare-docs): only attach the other-ecosystem hint to a genuine miss

CodeRabbit finding on PR #344, and broader than reported: the hint reads
"\"X\" was not found on docs.rs/npm", but blocking_fetch appended it to every
failure. A 503, a corrupt tarball, a store error, or a package that exists and
simply ships no types all produced a message asserting the package does not
exist. In the NoTypes case it directly contradicted the sentence it was
appended to.

ClientError gains is_package_missing(), kept separate from is_client_error()
because they answer different questions -- a package with no types is the
caller's problem yet is not missing. Only a 404 now earns the hint.

* docs(flare-docs): note retryable 4xx in the blocking_fetch classification contract
@getappz
getappz deleted the task/348 branch August 5, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant