Skip to content

feat: apply reviewed worktree cleanup plans - #2655

Merged
nicobailon merged 8 commits into
nicobailon:mainfrom
quifox:feat/worktree-retention
Oct 3, 2026
Merged

nicobailon merged 8 commits into
nicobailon:mainfrom
quifox:feat/worktree-retention

Conversation

@quifox

@quifox quifox commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Operators can now apply the named worktree cleanup plan they reviewed. Existing discard authorization governs application; the plan is claimed once, current ownership and Git facts are rechecked under shared cleanup/resume locks, and a durable receipt records each outcome. Removal retains the local branch and protects active, resumed, session-dependent, dirty, ignored-file and Git-locked trees. Interrupted journaled removals can repair matching handoff facts without deleting a recreated path.

Validation: typecheck, package build, focused cleanup and handoff tests, real executor authorization and external-job resume tests, and independent review passed on Linux Node 24. Disposable Git fixtures cover two-process contention, interrupted receipts, cancellation, drift and retained branches. Focused validation was repeated after rebasing onto 0.75.0. Required exact-head CI and macOS validation remain pending.

Related: #1622.

CI follow-up: the first Linux/Windows runs exposed a resume-validation ordering regression and a Windows path-identity mismatch when recording removals. Commit 7ac3537a restores the original baseRef rejection before lock admission and reuses the planner's existing filesystem-aware path comparison for handoff bookkeeping, including relative paths. The interrupted-receipt fixture now uses the canonical plan repository root; receipt/hash validation remains unchanged. Typecheck, package build and 36 focused cleanup/resume tests pass on Linux. Updated exact-head CI is running.

Review follow-up: record each removing intent atomically before invoking Git, then update that same receipt entry on success. An interrupted attempt can repair removal facts only after both directory and Git registration are absent; replay preserves recreated trees and newer handoff ownership and does not execute remaining candidates. Recover proven-dead locks with a token-specific atomic directory rename, retaining the nonempty old owner record so an interrupted or delayed reclaimer cannot block maintenance or move a new live lease. Unknown and live owners are still preserved.

Validation: typecheck, package build and 54 focused cleanup/handoff/lock tests pass on Linux, including real child-process interruption at the removal and recovery boundaries. Add a focused macOS cleanup/recovery job; the existing Windows matrix runs the portable regression tests. Checks on this appended commit are pending.

Follow-up head: 06d53c59.

CI update for 06d53c59: 15 of 16 configured checks passed, including the new macOS cleanup/recovery job and the portable unit regressions in the Windows matrix. The remaining failure is background keeps a terminal answer authoritative over an earlier tool timeout in the existing Windows integration suite (job); that test and execution path are unchanged by this follow-up. It passes when run alone on Linux, but the failed Windows check needs a maintainer rerun before merge readiness can be confirmed. The full Linux unit suite passed (3,691 passed, 15 existing skips). The latest code re-review confirms both original findings are addressed.

Maintainer review follow-up (befdd380): record the canonical worktree parent at allocation and carry that proof through handoff and cleanup records. Planning and apply retain paths outside that saved creation directory, and older handoffs without the proof remain kept. Use the full handoff validator during planning and under the handoff write lock before admitting a removal or invoking Git; malformed records stay on disk. Remove docs/design/worktree-retention.md and document the shipped checks in the tool reference.

Validation: typecheck, package build, 116 cleanup/handoff/worktree/lock tests, 9 focused cases through a symlinked temporary-root alias, and both tool-activation smoke tests passed. Disposable Git regressions cover ancestor redirection before and after planning, invalid child indexes and missing patch objects, missing creation-base proof, and propagation through the real handoff/cleanup writers. The previously failing timeout integration test also passed in an isolated Linux run. Fresh checks on this appended head must establish the Windows result.

Integration follow-up (3a429886): merge current main (8fb89814) without rewriting published history, preserving both changelog contributions. Update the two existing integration handoffs to carry creation-time base proof: the external-job follow-up fixture records it immediately after allocation, and the public cleanup-action fixture uses the proof returned by createWorktrees. Both previously failing scenarios now pass and still exercise resume protection and authorized removal. Typecheck, package build and all 116 focused cleanup tests were repeated after integrating main. Fresh exact-head Linux/Windows/macOS checks are pending.

CI update for 3a429886: all 16 configured checks passed, including every Linux/Windows shard, macOS cleanup/recovery and native nested-wait coverage, loader checks, and standalone/clean-install jobs (run). The formerly failing windows-3 shard passed. An independent before/after replay in disposable Git repositories confirmed that both reported cases delete trees on the old head and preserve them after the fix. The latest review reports no outstanding code findings, and all inline review threads are resolved.

@greptile-apps

greptile-apps Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[High risk] Adds file-system cleanup execution with locking and plan validation.

No outstanding code finding was identified, but merge readiness requires confirmation of required CI on the exact PR head.

Summary

The PR adds authorized application of reviewed worktree cleanup plans, with locked revalidation, single-use receipts, and conservative recovery of interrupted removals.

  • Records creation-directory proof and retains trees when ownership, path, session, or Git safety checks fail.
  • Adds focused cleanup and lock-recovery tests, updates the tool reference, and credits @quifox in the changelog.
  • Exact-head CI for 8db86e4c was not verified; the supplied passing CI result is for the earlier 3a429886 head.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Plan[Save reviewed plan] --> Auth[Authorize apply]
  Auth --> Lock[Acquire repository and handoff locks]
  Lock --> Check[Revalidate ownership and Git facts]
  Check -->|unsafe or changed| Keep[Record kept outcome]
  Check -->|eligible| Intent[Persist removal intent]
  Intent --> Git[Remove worktree; retain branch]
  Git --> Receipt[Update receipt and handoff]
  Intent -->|interrupted| Replay[Reconcile without repeating deletion]
Loading

Reviews (6) · Last reviewed commit: "docs(changelog): fold the cleanup safety..."

Comment thread src/runs/shared/worktree-lock.ts Resolved
@nicobailon

Copy link
Copy Markdown
Owner

Reviewed at 06d53c59 against #1622. Authority checks, single-use claims, journaling before Git, recovery that preserves recreated paths, and the lock fencing all hold up, and the recovery tests cover the real destructive boundaries. Two keep rules from #1622 still fail, though, and both were reproduced with disposable Git repos on this head. Apply turns these planner gaps, which were harmless while cleanup was plan-only, into deletions:

  1. A symlinked ancestor lets apply delete a tree outside its creation base (worktree-cleanup-apply.ts:114-124, worktree-cleanup-plan.ts:537, 619-636). recordedBaseDir is derived from the candidate's current resolved path, and only the leaf is checked with lstat. Repro: create a managed worktree at base/repo/pi-worktree-review-0 with a valid terminal preserved handoff, move base/repo to outside, replace it with a directory symlink to outside, then plan and apply with the original base config. The plan marks outside/pi-worktree-review-0 safe with recordedBaseDir: outside, and apply removes it (receipt complete). The base needs to come from creation-time metadata that apply compares against, and entries without that proof should be kept.
  2. Malformed handoff metadata is detected only after Git has removed the tree (worktree-cleanup-apply.ts:114-129; full validator in parallel-handoff.ts:95-104 runs only in recordRemoval). Repro: plan a removable entry, then change groups[0].children[0].index to -1 in the handoff and apply the unchanged plan. The worktree is removed, and the receipt then reports partial with the validation error. A child patch object that's missing before planning behaves the same way. The full manifest validation (readParallelHandoffManifest) needs to run under the handoff lock before journaling and Git, and the planner should keep entries it can't validate.
  3. Please drop docs/design/worktree-retention.md. It's a draft (publication pending, the count-retention proposal, future touchpoints), and docs/design/ doesn't exist on main. docs/tool-reference.md already documents the shipped interface.

Please add a regression for each of 1 and 2 using your disposable-Git fixtures. On CI, test (windows-3) failed background keeps a terminal answer authoritative over an earlier tool timeout. I found no causal link to this diff, and it passes locally, but I also didn't find it failing elsewhere, so the next push needs a green Windows run. #2657 is closed separately (see the comment there).

@nicobailon
nicobailon merged commit 3ae772c into nicobailon:main Oct 3, 2026
16 checks passed
@nicobailon

Copy link
Copy Markdown
Owner

Merged, thanks @quifox. Both deletion fixes held up in a re-review that reran the original reproductions. I folded the follow-up changelog line into the feature entry, since the feature has not been released yet.

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.

2 participants