Review follow-ups from #4239 (merge follow-up adopting the worktree that holds its PR branch). All three are structural consolidations, not bugs — the shipped behavior is correct, the knowledge is just spelled out in several places that must stay in sync by hand.
1. One predicate for "which worktrees may PortOS touch"
The chain inside data/cos/worktrees/, has an agentId, not claim-*, not an active agent, not locked is now written four times:
The four are deliberately not identical today (the reapers differ on locked and on whether .claude/worktrees/ is in scope), which is exactly why unifying them is its own change rather than a drive-by: each difference has to be confirmed intentional or fixed. Adoption moves the directory, so a future ownership rule added to one copy and missed in another is a data-loss bug, not a style drift.
Suggested shape: extract one predicate in worktreeManager.js taking explicit options for the parts that genuinely differ ({ allowLocked, roots }), have all three call sites use it, and decide whether branchReconcile.js's pair moves to server/lib/ so it can be shared without the import cycle (branchReconcile already imports worktreeManager).
2. One accessor for a task's target branch
resolveTaskExistingBranch (in server/services/agentWorkspacePrep.js) resolves existingBranch, falling back to reviewLoopPRBranch for a review-loop follow-up. Three other readers still read metadata.existingBranch raw:
server/services/agentWorktreeCleanup.js (resume-pointer logging)server/services/agentManagement.js (requeue reporting)server/services/cosTaskStore.js (paused-agent retire)
So a follow-up whose pointer was stripped is healed for worktree attach but not for the other three. Pair this with the writer side: cosTaskStore.js's strip rule and this resolver are one contract split across two files, synchronized only by prose. Both belong in one small pure module under server/lib/ (next to taskPauseHold.js / taskBlockCategories.js), exporting the strip predicate and the resolver, with the barrel + README rows the module-organization rule requires.
Deeper option worth evaluating first: spawnReviewLoopFollowUp writes the same string to both existingBranch and reviewLoopPRBranch. If follow-ups stopped writing existingBranch at all and derived it through the resolver, the strip exemption in cosTaskStore.js deletes itself and there is one key with one owner.
3. Resolve the branch holder once, instead of pointer-then-discovery
prepareAgentWorkspace now has two answers to "the branch is checked out elsewhere":
- the resume path abandons the branch (
branchStillClaimed → existingBranch: undefined → cut a fresh branch), and because that create succeeds, the new discovery path never runs for a resume at all;
- the follow-up path discovers the holder and adopts it.
resumeWorktreePath is really just a cached answer to "which tree holds this branch". Resolving the holder once up front — pointer if still valid, else discovery — and only then calling createWorktree collapses both blocks into one step and retires the branchStillClaimed fail-open. It would also give a resume the same take-over behavior a follow-up now gets.
Also considered, deliberately not done
Folding the adoption into createWorktree as an opt-in ({ adoptHolder: true }) handled inside queueWorktreeCreate, so discover-and-decide is serialized with the add rather than sitting between two queued operations. Architecturally cleaner and worth revisiting alongside item 3; skipped in #4239 to keep a heavily-tested primitive untouched.
Review follow-ups from #4239 (merge follow-up adopting the worktree that holds its PR branch). All three are structural consolidations, not bugs — the shipped behavior is correct, the knowledge is just spelled out in several places that must stay in sync by hand.
1. One predicate for "which worktrees may PortOS touch"
The chain inside
data/cos/worktrees/, has an agentId, notclaim-*, not an active agent, not locked is now written four times:server/services/worktreeManager.js—findAdoptableWorktreeForBranch(new in fix: let a merge follow-up take over the worktree that already holds its PR branch #4239)server/services/worktreeManager.js—cleanupOrphanedWorktreesserver/services/worktreeManager.js—reapMergedWorktreesserver/services/branchReconcile.js—worktreeProtectionReason/isAbandonedAgentWorktree(already pure, exported, and unit-tested)The four are deliberately not identical today (the reapers differ on
lockedand on whether.claude/worktrees/is in scope), which is exactly why unifying them is its own change rather than a drive-by: each difference has to be confirmed intentional or fixed. Adoption moves the directory, so a future ownership rule added to one copy and missed in another is a data-loss bug, not a style drift.Suggested shape: extract one predicate in
worktreeManager.jstaking explicit options for the parts that genuinely differ ({ allowLocked, roots }), have all three call sites use it, and decide whetherbranchReconcile.js's pair moves toserver/lib/so it can be shared without the import cycle (branchReconcilealready importsworktreeManager).2. One accessor for a task's target branch
resolveTaskExistingBranch(inserver/services/agentWorkspacePrep.js) resolvesexistingBranch, falling back toreviewLoopPRBranchfor a review-loop follow-up. Three other readers still readmetadata.existingBranchraw:server/services/agentWorktreeCleanup.js(resume-pointer logging)server/services/agentManagement.js(requeue reporting)server/services/cosTaskStore.js(paused-agent retire)So a follow-up whose pointer was stripped is healed for worktree attach but not for the other three. Pair this with the writer side:
cosTaskStore.js's strip rule and this resolver are one contract split across two files, synchronized only by prose. Both belong in one small pure module underserver/lib/(next totaskPauseHold.js/taskBlockCategories.js), exporting the strip predicate and the resolver, with the barrel + README rows the module-organization rule requires.Deeper option worth evaluating first:
spawnReviewLoopFollowUpwrites the same string to bothexistingBranchandreviewLoopPRBranch. If follow-ups stopped writingexistingBranchat all and derived it through the resolver, the strip exemption incosTaskStore.jsdeletes itself and there is one key with one owner.3. Resolve the branch holder once, instead of pointer-then-discovery
prepareAgentWorkspacenow has two answers to "the branch is checked out elsewhere":branchStillClaimed→existingBranch: undefined→ cut a fresh branch), and because that create succeeds, the new discovery path never runs for a resume at all;resumeWorktreePathis really just a cached answer to "which tree holds this branch". Resolving the holder once up front — pointer if still valid, else discovery — and only then callingcreateWorktreecollapses both blocks into one step and retires thebranchStillClaimedfail-open. It would also give a resume the same take-over behavior a follow-up now gets.Also considered, deliberately not done
Folding the adoption into
createWorktreeas an opt-in ({ adoptHolder: true }) handled insidequeueWorktreeCreate, so discover-and-decide is serialized with the add rather than sitting between two queued operations. Architecturally cleaner and worth revisiting alongside item 3; skipped in #4239 to keep a heavily-tested primitive untouched.