From 952fd8a189940e8f82006cf063ef9d4f348433a6 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Fri, 4 Sep 2026 17:37:12 +0000 Subject: [PATCH] allow resolve-and-merge follow-ups to adopt a live /claim worktree A "resolve and merge" trigger on a PR whose branch was still checked out in a /do:next claim-* worktree retried git worktree add against it for 5 cooldown cycles and then blocked permanently, because findAdoptableWorktreeForBranch's requireAgentId root gate rejected the claim-shaped directory name before the existing allowLiveClaim carve-out (used by branchReconcile's dispatch side) ever got a chance to apply. Fix the root-gate check in worktreeOwnershipReason to let a claim-shaped id through when the caller opts in, and thread allowLiveClaim from findAdoptableWorktreeForBranch through agentWorkspacePrep's review-loop and PR-remediation follow-up paths (isNonCommittingCoordinatorTask), so these tasks take over the branch's existing worktree and land the merge instead of stalling on it. --- server/lib/worktreeOwnership.js | 13 +++++- server/lib/worktreeOwnership.test.js | 10 ++++ server/services/agentWorkspacePrep.js | 21 ++++++++- server/services/agentWorkspacePrep.test.js | 53 ++++++++++++++++++++++ server/services/worktreeManager.js | 14 +++++- server/services/worktreeManager.test.js | 11 +++++ 6 files changed, 117 insertions(+), 5 deletions(-) diff --git a/server/lib/worktreeOwnership.js b/server/lib/worktreeOwnership.js index b97f1ea193..9a52640aba 100644 --- a/server/lib/worktreeOwnership.js +++ b/server/lib/worktreeOwnership.js @@ -9,7 +9,10 @@ * see `worktreeOwnershipReason` for why the claim comes last. Callers can * explicitly opt into the differences that are intentional: a reaper may * include `.claude/worktrees/`, stale claims may be reclaimed only by branch - * reconciliation, and only the dispatch side reads a live claim as unowned. + * reconciliation, and a live claim reads as unowned only for branch-reconcile's + * dispatch side and a non-committing coordinator follow-up (review-loop, + * PR-remediation) adopting the exact branch it exists to land — all three name + * the branch, not merely the directory. */ import { win32 } from 'path'; @@ -86,7 +89,13 @@ export function worktreeOwnershipReason({ const agentId = worktreeAgentId(path); const mustBeAgentWorktree = root?.requireAgentId ?? requireAgentId; - if (mustBeAgentWorktree && !isAgentWorktreeId(agentId)) return 'worktree-missing-agent-id'; + // A claim-shaped id inside an agent-only root would otherwise fail here before + // ever reaching the human-claim check below — which is exactly the id shape + // `allowLiveClaim` exists to admit. Let it through to that check instead of + // being turned away one gate early. + if (mustBeAgentWorktree && !isAgentWorktreeId(agentId) && !(allowLiveClaim && isHumanClaimWorktree(agentId))) { + return 'worktree-missing-agent-id'; + } if (locked) return 'worktree-locked'; if (activeAgentIds instanceof Set && activeAgentIds.has(agentId)) return 'worktree-active-agent'; if (requireKnownLiveness && isAgentWorktreeId(agentId) && !(activeAgentIds instanceof Set)) { diff --git a/server/lib/worktreeOwnership.test.js b/server/lib/worktreeOwnership.test.js index 3a660dd92d..b5b8fb6251 100644 --- a/server/lib/worktreeOwnership.test.js +++ b/server/lib/worktreeOwnership.test.js @@ -43,6 +43,16 @@ describe('worktree ownership', () => { // A root that demands an agent id refuses the claim basename on that ground. expect(worktreeOwnershipReason({ path: claim, roots: [{ path: COS_ROOT, requireAgentId: true }] })) .toBe('worktree-missing-agent-id'); + // …unless the caller opted into live-claim adoption, which lets a claim + // basename through the agent-id root gate so the claim test below it (not + // this one) decides the verdict. + expect(worktreeOwnershipReason({ + path: claim, roots: [{ path: COS_ROOT, requireAgentId: true }], allowLiveClaim: true + })).toBeNull(); + // A non-claim, non-agent basename still can't ride that carve-out through. + expect(worktreeOwnershipReason({ + path: `${COS_ROOT}/next-issue-42`, roots: [{ path: COS_ROOT, requireAgentId: true }], allowLiveClaim: true + })).toBe('worktree-missing-agent-id'); }); it('fails closed when agent liveness is unknown and permits an explicitly non-agent root', () => { diff --git a/server/services/agentWorkspacePrep.js b/server/services/agentWorkspacePrep.js index 2be4ec94a0..65b0474099 100644 --- a/server/services/agentWorkspacePrep.js +++ b/server/services/agentWorkspacePrep.js @@ -41,6 +41,7 @@ import { resolveTaskTargetBranch } from '../lib/taskTargetBranch.js'; import { resolveTaskForkHead } from '../lib/forkHead.js'; import { getAppWorkspace, getAppDataForTask, createJiraTicketForTask } from './agentPromptBuilder.js'; import { INVESTIGATION_TASK_DELIVERY, isInvestigationTask } from '../lib/investigationTasks.js'; +import { isNonCommittingCoordinatorTask } from './taskTypeHooks.js'; const ROOT_DIR = PATHS.root; @@ -101,10 +102,15 @@ async function getProtectedAgentIds() { * * `findAdoptableWorktreeForBranch` refuses every holder PortOS doesn't own * outright, so this can never move the user's checkout or a live agent's tree. + * The one caller-gated exception is `allowLiveClaim`: a non-committing + * coordinator follow-up (`isNonCommittingCoordinatorTask` — a review-loop + * resolve-and-merge or a PR-remediation follow-up) may take over a `claim-*` + * holder too, because its whole deliverable names that exact branch as the one + * to finish and land. * * @returns {Promise<{ worktreeInfo: object, adoptedFrom: string }|null>} */ -async function adoptWorktreeHoldingBranch({ agentId, workspacePath, branchName, preferredPath = null, taskId }) { +async function adoptWorktreeHoldingBranch({ agentId, workspacePath, branchName, preferredPath = null, taskId, allowLiveClaim = false }) { // Fail CLOSED on an unreadable agent list: an empty protected set would read as // "nothing is running", which is the one wrong answer here — it would move a // live run's directory. The caller's timed pause is the safe outcome instead. @@ -114,7 +120,7 @@ async function adoptWorktreeHoldingBranch({ agentId, workspacePath, branchName, }); if (!activeAgentIds) return null; - const holder = await findAdoptableWorktreeForBranch(workspacePath, branchName, { activeAgentIds, preferredPath }); + const holder = await findAdoptableWorktreeForBranch(workspacePath, branchName, { activeAgentIds, preferredPath, allowLiveClaim }); if (!holder) return null; const worktreeInfo = await adoptWorktree(agentId, workspacePath, holder.path, branchName).catch(err => { @@ -150,6 +156,16 @@ async function prepareRequestedWorktree({ // same safe adoption path review-loop follow-ups use, rather than cutting a // fresh branch merely because a cached path could not be moved. const resumeWorktreePath = existingBranch ? task.metadata?.resumeWorktreePath : null; + // A review-loop / PR-remediation follow-up's whole purpose is landing THIS + // branch, and the user's own "resolve and merge" trigger (or pr-reviewer's own + // dispatch) is the signal to finish whatever a `/do:next` claim left on it — so + // these are the callers allowed to take over a `claim-*` holder instead of + // retrying against it until the task gives up (#6243). `isNonCommittingCoordinatorTask` + // is the shared predicate for exactly this "same shape" set (taskTypeHooks.js) — + // reused here rather than re-listing the flags, so a future follow-up type of the + // same shape inherits the carve-out too. A plain resume never targets a claim tree + // (its pointer names a CoS `agent-*` worktree) and doesn't set either follow-up + // flag, so the predicate is a no-op there. const takeoverPromise = existingBranch ? adoptWorktreeHoldingBranch({ agentId, @@ -157,6 +173,7 @@ async function prepareRequestedWorktree({ branchName: existingBranch, preferredPath: resumeWorktreePath, taskId: task.id, + allowLiveClaim: isNonCommittingCoordinatorTask(task), }) : Promise.resolve(null); diff --git a/server/services/agentWorkspacePrep.test.js b/server/services/agentWorkspacePrep.test.js index b954fbbc18..c8a44f481a 100644 --- a/server/services/agentWorkspacePrep.test.js +++ b/server/services/agentWorkspacePrep.test.js @@ -480,6 +480,59 @@ describe('prepareAgentWorkspace — the branch is checked out in another worktre expect(updateTask).not.toHaveBeenCalled(); }); + // A resolve-and-merge follow-up's whole purpose is landing THIS branch, so the + // user's own trigger is the signal to take over whatever a `/do:next` claim + // left checked out on it — instead of retrying against it until the task gives + // up and strands the PR the follow-up exists to land (#6243). + it('opts into adopting a live /claim worktree for a review-loop follow-up', async () => { + findAdoptableWorktreeForBranch.mockResolvedValue({ path: '/mock/worktrees/claim-issue-42', agentId: 'claim-issue-42' }); + adoptWorktree.mockResolvedValue({ + worktreePath: '/mock/worktrees/agent-new', branchName: 'cos/task-x/agent-y', + baseBranch: null, existingBranch: true, adopted: true + }); + + const r = await prepareAgentWorkspace({ agentId: 'agent-new', task: followUpTask() }); + + const [, , opts] = findAdoptableWorktreeForBranch.mock.calls.at(-1); + expect(opts.allowLiveClaim).toBe(true); + expect(r.outcome).toBe('ready'); + expect(updateTask).not.toHaveBeenCalled(); + }); + + // A PR-remediation follow-up is the same shape one step further out (its + // commits land on the contributor's own branch) and hits the identical #6243 + // bug when that branch is checked out in a claim tree, so it carries the same + // carve-out via the shared isNonCommittingCoordinatorTask predicate. + it('opts into adopting a live /claim worktree for a PR-remediation follow-up', async () => { + findAdoptableWorktreeForBranch.mockResolvedValue({ path: '/mock/worktrees/claim-issue-99', agentId: 'claim-issue-99' }); + adoptWorktree.mockResolvedValue({ + worktreePath: '/mock/worktrees/agent-new', branchName: 'contributor/fix-thing', + baseBranch: null, existingBranch: true, adopted: true + }); + const task = { + id: 't-remediation', taskType: 'internal', + metadata: { useWorktree: true, prRemediationFollowUp: true, existingBranch: 'contributor/fix-thing' } + }; + + const r = await prepareAgentWorkspace({ agentId: 'agent-new', task }); + + const [, , opts] = findAdoptableWorktreeForBranch.mock.calls.at(-1); + expect(opts.allowLiveClaim).toBe(true); + expect(r.outcome).toBe('ready'); + }); + + // A plain resume never targets a claim tree — its pointer names a CoS + // `agent-*` worktree — so it must not carry the same carve-out. + it('does not opt into live-claim adoption for a plain resume', async () => { + findAdoptableWorktreeForBranch.mockResolvedValue(null); + const task = { id: 't-resume', taskType: 'user', metadata: { useWorktree: true, existingBranch: 'cos/t-resume/agent-dead' } }; + + await prepareAgentWorkspace({ agentId: 'agent-new', task }); + + const [, , opts] = findAdoptableWorktreeForBranch.mock.calls.at(-1); + expect(opts.allowLiveClaim).toBe(false); + }); + // Adoption MOVES the directory, so the protected set has to cover every agent // that still needs its tree — including a PAUSED one, whose worktree is // deliberately preserved as resume context and which is absent from the diff --git a/server/services/worktreeManager.js b/server/services/worktreeManager.js index b2a374367f..ddea294caf 100644 --- a/server/services/worktreeManager.js +++ b/server/services/worktreeManager.js @@ -584,7 +584,8 @@ async function createWorktreeUnlocked(agentId, sourceWorkspace, taskId, options * - the primary checkout, or any tree outside `data/cos/worktrees/` — moving * the user's own checkout out from under them is exactly the branch-jacking * guarded against everywhere else; - * - a human `/claim` tree (`claim-*`), owned by the claim flow's cleanup; + * - a human `/claim` tree (`claim-*`), owned by the claim flow's cleanup — + * unless the caller passes `allowLiveClaim` (see below); * - a tree whose agent is still running — it is mid-edit in that directory; * - a locked worktree, whose lock means "don't touch" regardless of owner. * @@ -593,11 +594,21 @@ async function createWorktreeUnlocked(agentId, sourceWorkspace, taskId, options * @param {object} [options] * @param {Set} [options.activeAgentIds] - agents currently running * @param {string} [options.preferredPath] - cached holder path to validate first + * @param {boolean} [options.allowLiveClaim=false] - treat a `claim-*` holder as + * adoptable rather than off-limits. The claim flow keeps no durable agent id + * for its branch, so this can't distinguish an idle claim tree from a live + * `/do:next` session in it — pass it only for a task whose whole purpose IS + * that exact branch (a review-loop resolve-and-merge follow-up, or a PR- + * remediation follow-up — `isNonCommittingCoordinatorTask` in taskTypeHooks.js), + * where the task's own deliverable is the signal that the claim's work should + * be finished and landed. Mirrors `branchReconcile.resolveLiveOwnerReason`'s + * dispatch-side carve-out for the same directory shape (#6243). * @returns {Promise<{ path: string, agentId: string }|null>} */ export async function findAdoptableWorktreeForBranch(sourceWorkspace, branchName, { activeAgentIds = new Set(), preferredPath = null, + allowLiveClaim = false, } = {}) { if (!sourceWorkspace || !branchName) return null; @@ -618,6 +629,7 @@ export async function findAdoptableWorktreeForBranch(sourceWorkspace, branchName activeAgentIds, roots: [{ path: WORKTREES_DIR, requireAgentId: true }], requireKnownLiveness: true, + allowLiveClaim, }); if (ownershipReason) return null; diff --git a/server/services/worktreeManager.test.js b/server/services/worktreeManager.test.js index 377ee378ea..e3c7a02a76 100644 --- a/server/services/worktreeManager.test.js +++ b/server/services/worktreeManager.test.js @@ -832,6 +832,17 @@ describe('findAdoptableWorktreeForBranch (take over the tree that holds the bran expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); }); + // A review-loop resolve-and-merge follow-up is the one caller that names this + // exact branch as the thing it exists to finish and land, so it opts in via + // `allowLiveClaim` instead of retrying `git worktree add` against a branch a + // `/do:next` claim already holds (#6243). + it('adopts a human /claim worktree when the caller opts into allowLiveClaim', async () => { + scriptWorktrees([{ path: cosTree('claim-issue-42'), branch: `refs/heads/${BRANCH}` }]); + + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH, { allowLiveClaim: true })) + .toEqual({ path: cosTree('claim-issue-42'), agentId: 'claim-issue-42' }); + }); + it('refuses a non-agent directory in the managed root', async () => { scriptWorktrees([{ path: cosTree('next-issue-42'), branch: `refs/heads/${BRANCH}` }]);