fix: let a merge follow-up take over the worktree that already holds its PR branch - #4239
Merged
Merged
Conversation
…its PR branch A review-loop/merge follow-up must run attached to its PR branch. When some other worktree already has that branch checked out, `git worktree add` refuses and the task pauses on a cooldown — correct when the holder is a cleanup seconds from teardown, useless when it is a tree `removeWorktree` REFUSED to delete (uncommitted changes). Nothing is coming to release that branch, so the follow-up burns its five waits, blocks as `worktree-failed`, and the pull request it exists to land sits open forever (sys-rl-msto5qab, PR #4216). Adopt the holder instead. `findAdoptableWorktreeForBranch` locates the tree holding the branch and refuses every holder PortOS doesn't own outright — adoption MOVES the directory, so the primary checkout, anything outside data/cos/worktrees/, a human `claim-*` tree, a locked worktree, and any tree belonging to a running OR paused agent are all rejected (paused agents keep their tree as resume context and are absent from the in-process maps). An unreadable agent list fails closed to the existing pause rather than reading as "nothing is running". Also fixes the follow-up losing the branch it is pointed at. `updateTask` strips `existingBranch` on a terminal status unless the task is a follow-up, keyed on `resumedFromAgentId` — but `resumePointerMetadata` stamps that key onto ANY task whose run left work behind, so a follow-up that fails once looks exactly like a resume from then on and gets stripped. It would then cut a worktree fresh off main and push its fix to a branch the PR never heard of. `resolveTaskExistingBranch` falls back to `reviewLoopPRBranch`, the same value under the follow-up's own namespace, which is never treated as a resume pointer.
This was referenced Aug 15, 2026
atomantic
added a commit
that referenced
this pull request
Aug 15, 2026
## Summary Consolidates the "which worktrees may PortOS touch" and "what branch does this task target" logic that was independently duplicated across `worktreeManager.js`, `branchReconcile.js`, `agentWorkspacePrep.js`, `agentWorktreeCleanup.js`, `agentManagement.js`, and `cosTaskStore.js` into two pure, unit-tested modules: - `server/lib/worktreeOwnership.js` — `worktreeOwnershipReason()` is the single ownership gate for destructive worktree operations (adopt/reap/cleanup), taking explicit options for the differences that are intentional (a reaper may include `.claude/worktrees/`; stale `/claim` worktrees may be reclaimed only by branch reconciliation; liveness requirements differ by caller). - `server/lib/taskTargetBranch.js` — `resolveTaskTargetBranch()` / `shouldStripTaskTargetBranch()` is the single reader/writer contract for a task's `existingBranch` (retry) vs `reviewLoopPRBranch` (review-loop follow-up) pointer, closing a gap where three call sites read `metadata.existingBranch` raw and so weren't healed the same way `agentWorkspacePrep.js`'s resolver already was. Also collapses `prepareAgentWorkspace`'s two-step "try create, then discover the holder on failure" flow into resolving the branch holder once up front (pointer if still valid, else discovery), giving a resume retry the same safe worktree-takeover behavior a review-loop follow-up already had, and retiring the `branchStillClaimed` fail-open. Follow-up from #4239's review; addresses #4241. Also includes a `/simplify` pass: dropped `worktreeOwnershipReason`'s unused `allowLocked` option and the unexercised bare-string root form (no caller ever used either), and parallelized `prepareAgentWorkspace`'s independent base-branch-detection and branch-holder-resolution I/O instead of awaiting them serially. One deferred cleanup filed as #4284 (out of scope here — touches an externally-asserted test vocabulary): `reapMergedWorktrees`'s inline remap from the shared module's canonical `worktree-*` skip-reason codes back to a private legacy vocabulary. ## Test plan - `cd server && NODE_ENV=test npx vitest run lib/worktreeOwnership.test.js lib/taskTargetBranch.test.js services/agentManagement.test.js services/agentWorkspacePrep.test.js services/branchReconcile.test.js services/cleanupAgentWorktree.test.js services/cosTaskStore.test.js services/worktreeManager.test.js` — 624 tests passing - Manually traced `isAbandonedAgentWorktree`, `worktreeProtectionReason`, and `resolveLiveOwnerReason` against their pre-refactor implementations to confirm the shared predicate preserves each of their previously-intentional differences (locked handling, stale-claim reclaim, liveness-unknown fail-closed behavior) - Manually traced `spawnReviewLoopFollowUp` to confirm `reviewLoopFollowUp`/`reviewLoopPRBranch` are still both stamped so `resolveTaskTargetBranch`'s fallback still resolves a follow-up's branch after the `existingBranch` write was removed
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
A review-loop/merge follow-up must run in a worktree attached to its PR branch. When another worktree already has that branch checked out,
git worktree addrefuses and the task takes the timedworktree-busypause added in #4227 — correct when the holder is a cleanup seconds from teardown, useless when it is a treeremoveWorktreeREFUSED to delete (uncommitted changes). Nothing is coming to release that branch, so the follow-up burns its five waits, blocks asworktree-failed, and the PR it exists to land sits open forever. That is what stranded PR #4216.Two fixes:
Adopt the holder instead of waiting it out.
findAdoptableWorktreeForBranch(new,worktreeManager.js) locates the worktree holding a branch and refuses every holder PortOS doesn't own outright — adoption MOVES the directory, so it rejects the primary checkout, anything outsidedata/cos/worktrees/, a humanclaim-*tree, a locked worktree, and any tree belonging to a running or paused agent (a paused agent's tree is deliberately preserved as resume context and is absent from the in-process maps). The protected-id set unions the in-process maps with persistedrunning/pausedstatus, the same definition the daily worktree reap uses, so a run that survived a server restart still counts. An unreadable agent list fails closed to the existing pause rather than reading as "nothing is running".Stop the follow-up losing the branch it is pointed at.
updateTaskstripsexistingBranchon a terminal status unless the task is a follow-up — keyed onresumedFromAgentId. ButresumePointerMetadatastamps that key onto any task whose run left work behind, so a follow-up that fails once looks exactly like a resume from then on and gets stripped. It would then cut a worktree fresh offmainand push its fix to a branch the PR never heard of.resolveTaskExistingBranchfalls back toreviewLoopPRBranch— the same value under the follow-up's own namespace, never treated as a resume pointer.Test plan
server/services/worktreeManager.test.js— 7 new tests forfindAdoptableWorktreeForBranch: finds the CoS tree holding the branch; returns null when nothing holds it; refuses the primary checkout and any tree outside the managed root; refuses aclaim-*tree; refuses a running agent's tree; refuses a locked worktree; returns null rather than throwing when the listing fails.server/services/agentWorkspacePrep.test.js— adoption instead of pausing; fallback to the pause when adoption is refused; no holder lookup when the task has no branch to attach to; running and paused agents both protected; refuses to adopt at all when the agent list can't be read; plus 4 tests forresolveTaskExistingBranchincluding a stripped follow-up attaching to its PR branch.services/run clean end to end).