refactor: consolidate CoS worktree ownership - #4286
Merged
Merged
Conversation
…prep I/O /simplify pass on #4241's worktree-ownership consolidation: - worktreeOwnershipReason() drops allowLocked (no caller ever passed true) and the unexercised bare-string root form in normalizedRoots(). - prepareAgentWorkspace() now resolves the base-branch lookup and the branch-holder takeover concurrently instead of serially — they're independent reads and neither depends on the other's result. Deferred: the legacy skip-reason remap in reapMergedWorktrees (filed as #4284, out of scope here — it touches an externally-asserted vocabulary).
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.
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, andcosTaskStore.jsinto 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/claimworktrees 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'sexistingBranch(retry) vsreviewLoopPRBranch(review-loop follow-up) pointer, closing a gap where three call sites readmetadata.existingBranchraw and so weren't healed the same wayagentWorkspacePrep.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 thebranchStillClaimedfail-open.Follow-up from #4239's review; addresses #4241.
Also includes a
/simplifypass: droppedworktreeOwnershipReason's unusedallowLockedoption and the unexercised bare-string root form (no caller ever used either), and parallelizedprepareAgentWorkspace'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 canonicalworktree-*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 passingisAbandonedAgentWorktree,worktreeProtectionReason, andresolveLiveOwnerReasonagainst 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)spawnReviewLoopFollowUpto confirmreviewLoopFollowUp/reviewLoopPRBranchare still both stamped soresolveTaskTargetBranch's fallback still resolves a follow-up's branch after theexistingBranchwrite was removed