fix: let a merge follow-up take over the worktree that already holds its PR branch

.changelog/next/fixed-agent-1e904405.md

@@ -0,0 +1 @@
+- A merge follow-up now takes over the worktree that already holds its PR branch instead of blocking on it

server/services/agentWorkspacePrep.js

@@ -30,11 +30,11 @@ import { execGit } from '../lib/execGit.js';
 import { emitLog } from './cosEvents.js';
 import { updateTask, addTask } from './cos.js';
 import { getAppById } from './apps.js';
-import { isTruthyMeta, isFalsyMeta } from './agentState.js';
+import { isTruthyMeta, isFalsyMeta, getActiveAgentIds } from './agentState.js';
 import { PATHS } from '../lib/fileUtils.js';
 import * as git from './git.js';
 import { detectConflicts } from './taskConflict.js';
-import { createWorktree, adoptWorktree, isBranchCheckedOutElsewhereError } from './worktreeManager.js';
+import { createWorktree, adoptWorktree, findAdoptableWorktreeForBranch, isBranchCheckedOutElsewhereError } from './worktreeManager.js';
 import { resolveSpawnCwd } from '../lib/spawnCwd.js';
 import { enforceSafeBranchUpstream } from '../lib/branchUpstreamGuard.js';
 import { getAppWorkspace, getAppDataForTask, createJiraTicketForTask } from './agentPromptBuilder.js';
@@ -75,6 +75,88 @@ async function blockTask(task, reason, blockedCategory, extraMetadata = {}) {
 const WORKTREE_BUSY_COOLDOWN_MS = 2 * 60 * 1000;
 const WORKTREE_BUSY_MAX_ATTEMPTS = 5;
 
+/**
+ * The branch this task's worktree must attach to, or null to cut a fresh one.
+ *
+ * `existingBranch` is the pointer both producers write — a resume, and a
+ * review-loop/merge follow-up. It is also the key `updateTask` DROPS when a task
+ * reaches a terminal state, since a stale resume pointer would silently attach an
+ * unrelated re-run to a dead branch. That strip is keyed on `resumedFromAgentId`
+ * so a follow-up's copy — its CONFIGURATION, not a resume pointer — normally
+ * survives, but the exemption does not hold once the follow-up's OWN run fails:
+ * `resumePointerMetadata` stamps `resumedFromAgentId` onto any task with work left
+ * behind, and from then on the follow-up looks exactly like a resume and loses its
+ * branch. It then cut a worktree fresh off the default branch, so the fix it
+ * pushed for the PR went to a branch the PR never heard of.
+ *
+ * `reviewLoopPRBranch` is the same branch under the follow-up's own namespace,
+ * written by `spawnReviewLoopFollowUp` and never treated as a resume pointer, so
+ * it survives both the strip and the older records written before the exemption
+ * existed. Falling back to it makes the two copies impossible to disagree about.
+ */
+export function resolveTaskExistingBranch(metadata) {
+  if (metadata?.existingBranch) return metadata.existingBranch;
+  if (isTruthyMeta(metadata?.reviewLoopFollowUp) && metadata?.reviewLoopPRBranch) return metadata.reviewLoopPRBranch;
+  return null;
+}
+
+/**
+ * Agent ids whose worktree must not be taken out from under them, using the same
+ * definition the daily worktree reap protects on (`autonomousJobs/scriptHandlers.js`):
+ *
+ *   - the in-process maps, which are authoritative for THIS process;
+ *   - every persisted `running` agent, which is how a run that survived a server
+ *     restart (the maps are empty then) still counts;
+ *   - every persisted `paused` agent — pausing deliberately preserves the tree as
+ *     resume context, and a paused agent is absent from the maps.
+ *
+ * Under-counting here would move a directory another run is mid-edit in.
+ */
+async function getProtectedAgentIds() {
+  const { getAgents } = await import('./cos.js');
+  const ids = new Set(getActiveAgentIds());
+  for (const agent of await getAgents()) {
+    if (agent?.status === 'running' || agent?.status === 'paused') ids.add(agent.id);
+  }
+  return ids;
+}
+
+/**
+ * Take over the worktree that already holds `branchName`, for a task whose whole
+ * purpose is to run ON that branch (a merge/review-loop follow-up, a resume).
+ *
+ * Called only after `git worktree add` refused because the branch is checked out
+ * elsewhere. Routinely that holder is a cleanup's tree seconds from teardown — the
+ * caller's timed pause covers that — but a tree `removeWorktree` REFUSED to delete
+ * (uncommitted changes) holds the branch until a human intervenes, and waiting it
+ * out just strands the pull request the follow-up exists to land. Adoption is the
+ * shorter path in both cases and preserves whatever the previous run left behind.
+ *
+ * `findAdoptableWorktreeForBranch` refuses every holder PortOS doesn't own
+ * outright, so this can never move the user's checkout or a live agent's tree.
+ *
+ * @returns {Promise<{ worktreeInfo: object, adoptedFrom: string }|null>}
+ */
+async function adoptWorktreeHoldingBranch({ agentId, workspacePath, branchName, taskId }) {
+  // 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.
+  const activeAgentIds = await getProtectedAgentIds().catch(err => {
+    emitLog('warn', `🌳 Skipping worktree adoption for task ${taskId} — could not read the agent list: ${err.message}`, { taskId });
+    return null;
+  });
+  if (!activeAgentIds) return null;
+
+  const holder = await findAdoptableWorktreeForBranch(workspacePath, branchName, { activeAgentIds });
+  if (!holder) return null;
+
+  const worktreeInfo = await adoptWorktree(agentId, workspacePath, holder.path, branchName).catch(err => {
+    emitLog('warn', `🌳 Could not adopt ${holder.path} holding ${branchName} for task ${taskId}: ${err.message}`, { taskId });
+    return null;
+  });
+  return worktreeInfo ? { worktreeInfo, adoptedFrom: holder.agentId } : null;
+}
+
 /**
  * Prepare the workspace (and any worktree/JIRA branch) for an agent task.
  *
@@ -141,7 +223,8 @@ export async function prepareAgentWorkspace({ agentId, task }) {
   // conflict AUTO-detection resumes into the shared workspace on retry (conflict
   // detection returns `proceed` once the dead agent is gone) and silently
   // abandons the work the pointer was recorded to save.
-  const wantsWorktree = explicitWorktree || !!task.metadata?.existingBranch;
+  const existingBranch = resolveTaskExistingBranch(task.metadata);
+  const wantsWorktree = explicitWorktree || !!existingBranch;
 
   if (!isReadOnly) {
     // Pull latest from git before starting work
@@ -266,7 +349,6 @@ export async function prepareAgentWorkspace({ agentId, task }) {
     }
 
     if (wantsWorktree && !jiraBranchName) {
-      const existingBranch = task.metadata?.existingBranch || null;
       const { baseBranch: detectedBase } = await git.getRepoBranches(workspacePath).catch(() => ({ baseBranch: null }));
       if (existingBranch) {
         emitLog('info', `🌳 Worktree requested for task ${task.id} on existing branch ${existingBranch}`, {
@@ -317,10 +399,19 @@ export async function prepareAgentWorkspace({ agentId, task }) {
         return null;
       });
 
+      // The branch this task exists to work on is checked out somewhere else. If
+      // that somewhere is a tree PortOS owns, take it over rather than burning
+      // cooldowns below on a teardown that may never come (see
+      // `adoptWorktreeHoldingBranch`).
+      const takeover = !worktreeInfo && existingBranch && isBranchCheckedOutElsewhereError(worktreeError?.message)
+        ? await adoptWorktreeHoldingBranch({ agentId, workspacePath, branchName: existingBranch, taskId: task.id })
+        : null;
+      if (takeover) worktreeInfo = takeover.worktreeInfo;
+
       if (worktreeInfo) {
         workspacePath = worktreeInfo.worktreePath;
         const origin = worktreeInfo.adopted
-          ? `adopted from ${task.metadata?.resumedFromAgentId || 'the interrupted run'}`
+          ? `adopted from ${takeover?.adoptedFrom || task.metadata?.resumedFromAgentId || 'the interrupted run'}`
           : `base: ${worktreeInfo.baseBranch}`;
         emitLog('success', `🌳 Agent ${agentId} will work in worktree: ${worktreeInfo.branchName} (${origin})`, {
           agentId, worktreePath: worktreeInfo.worktreePath, branchName: worktreeInfo.branchName, baseBranch: worktreeInfo.baseBranch

server/services/agentWorkspacePrep.test.js

@@ -35,6 +35,7 @@ vi.mock('./worktreeManager.js', async (importOriginal) => ({
   ...(await importOriginal()),
   createWorktree: vi.fn(),
   adoptWorktree: vi.fn(),
+  findAdoptableWorktreeForBranch: vi.fn().mockResolvedValue(null),
   mergeBaseIntoFeatureWorktree: vi.fn(),
 }));
 vi.mock('./agentPromptBuilder.js', () => ({
@@ -43,12 +44,12 @@ vi.mock('./agentPromptBuilder.js', () => ({
   createJiraTicketForTask: vi.fn(),
 }));
 
-import { prepareAgentWorkspace } from './agentWorkspacePrep.js';
-import { updateTask } from './cos.js';
+import { prepareAgentWorkspace, resolveTaskExistingBranch } from './agentWorkspacePrep.js';
+import { updateTask, getAgents } from './cos.js';
 import { ensureLatest } from './git.js';
 import { detectConflicts } from './taskConflict.js';
 import { getAppWorkspace } from './agentPromptBuilder.js';
-import { createWorktree, adoptWorktree } from './worktreeManager.js';
+import { createWorktree, adoptWorktree, findAdoptableWorktreeForBranch } from './worktreeManager.js';
 
 beforeEach(() => { vi.clearAllMocks(); });
 
@@ -311,6 +312,80 @@ describe('prepareAgentWorkspace — the branch is checked out in another worktre
     ensureLatest.mockResolvedValue({ success: true, upToDate: true });
     adoptWorktree.mockResolvedValue(null);
     createWorktree.mockRejectedValue(BUSY);
+    getAgents.mockResolvedValue([]);
+    // Default: nobody adoptable holds the branch, so the pause path below is
+    // reached. The adoption tests opt in explicitly.
+    findAdoptableWorktreeForBranch.mockResolvedValue(null);
+  });
+
+  // The whole point of a merge follow-up's worktree is to be attached to the PR
+  // branch. When a tree PortOS owns already has it — the finished run's own,
+  // preserved because it was dirty — that tree IS the workspace being asked for,
+  // and no cooldown was ever going to free it.
+  it('adopts the worktree that already holds the branch instead of pausing', async () => {
+    findAdoptableWorktreeForBranch.mockResolvedValue({ path: '/mock/worktrees/agent-y', agentId: 'agent-y' });
+    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() });
+
+    expect(adoptWorktree).toHaveBeenCalledWith('agent-new', expect.any(String), '/mock/worktrees/agent-y', 'cos/task-x/agent-y');
+    expect(r.outcome).toBe('ready');
+    expect(r.workspacePath).toBe('/mock/worktrees/agent-new');
+    // Not blocked, not paused — the task never reaches updateTask at all.
+    expect(updateTask).not.toHaveBeenCalled();
+  });
+
+  // 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
+  // in-process maps entirely.
+  it('protects running AND paused agents from having their tree moved', async () => {
+    getAgents.mockResolvedValue([
+      { id: 'agent-running', status: 'running' },
+      { id: 'agent-paused', status: 'paused' },
+      { id: 'agent-done', status: 'completed' },
+    ]);
+
+    await prepareAgentWorkspace({ agentId: 'agent-new', task: followUpTask() });
+
+    const [, , opts] = findAdoptableWorktreeForBranch.mock.calls.at(-1);
+    expect([...opts.activeAgentIds].sort()).toEqual(['agent-paused', 'agent-running']);
+  });
+
+  // An unreadable agent list must not read as "nothing is running" — that is the
+  // one wrong answer, since it would move a live run's directory.
+  it('refuses to adopt at all when the agent list cannot be read', async () => {
+    getAgents.mockRejectedValue(new Error('state.json unreadable'));
+
+    const r = await prepareAgentWorkspace({ agentId: 'agent-new', task: followUpTask() });
+
+    expect(findAdoptableWorktreeForBranch).not.toHaveBeenCalled();
+    expect(adoptWorktree).toHaveBeenCalledTimes(0);
+    expect(r.outcome).toBe('blocked');
+  });
+
+  it('falls back to the cooldown pause when the adoption is refused', async () => {
+    findAdoptableWorktreeForBranch.mockResolvedValue({ path: '/mock/worktrees/agent-y', agentId: 'agent-y' });
+    adoptWorktree.mockResolvedValue(null);
+
+    const r = await prepareAgentWorkspace({ agentId: 'agent-new', task: followUpTask() });
+
+    expect(r.outcome).toBe('blocked');
+    expect(updateTask.mock.calls.at(-1)[1].metadata.blockedCategory).toBe('worktree-busy');
+  });
+
+  // Only a task that KNOWS which branch it wants can take over the tree holding
+  // it. A plain isolated task's add failure names a branch it just tried to
+  // create, and adopting some other tree would hand it unrelated work.
+  it('does not go looking for a holder when the task has no branch to attach to', async () => {
+    const task = { id: 't-plain', taskType: 'user', metadata: { useWorktree: true } };
+
+    await prepareAgentWorkspace({ agentId: 'agent-new', task });
+
+    expect(findAdoptableWorktreeForBranch).not.toHaveBeenCalled();
   });
 
   it('pauses with a cooldown instead of blocking, and keeps the branch pointer', async () => {
@@ -358,3 +433,54 @@ describe('prepareAgentWorkspace — the branch is checked out in another worktre
     expect(patch.metadata.blockedCategory).toBe('worktree-failed');
   });
 });
+
+// Which branch a task's worktree attaches to when `existingBranch` didn't survive
+// `updateTask`'s resume-pointer strip — see the docblock on the function.
+describe('resolveTaskExistingBranch', () => {
+  it('prefers the explicit pointer', () => {
+    expect(resolveTaskExistingBranch({ existingBranch: 'cos/a/agent-1' })).toBe('cos/a/agent-1');
+  });
+
+  it('falls back to a follow-up’s own record of its PR branch', () => {
+    expect(resolveTaskExistingBranch({
+      reviewLoopFollowUp: true, reviewLoopPRBranch: 'cos/a/agent-1'
+    })).toBe('cos/a/agent-1');
+    // TASKS.md round-trips metadata as strings.
+    expect(resolveTaskExistingBranch({
+      reviewLoopFollowUp: 'true', reviewLoopPRBranch: 'cos/a/agent-1'
+    })).toBe('cos/a/agent-1');
+  });
+
+  // The ORIGINAL task also carries PR metadata once its review loop starts; it
+  // owns that branch through its own worktree and must not be re-pointed at it.
+  it('ignores a PR branch on anything that is not a follow-up', () => {
+    expect(resolveTaskExistingBranch({ reviewLoopPRBranch: 'cos/a/agent-1' })).toBeNull();
+    expect(resolveTaskExistingBranch({ reviewLoopFollowUp: false, reviewLoopPRBranch: 'cos/a/agent-1' })).toBeNull();
+    expect(resolveTaskExistingBranch({})).toBeNull();
+    expect(resolveTaskExistingBranch(undefined)).toBeNull();
+  });
+
+  it('attaches a stripped follow-up to its PR branch rather than cutting a new one', async () => {
+    ensureLatest.mockResolvedValue({ success: true, upToDate: true });
+    adoptWorktree.mockResolvedValue(null);
+    createWorktree.mockResolvedValue({
+      worktreePath: '/mock/worktrees/agent-new', branchName: 'cos/task-x/agent-y',
+      baseBranch: null, existingBranch: true
+    });
+
+    await prepareAgentWorkspace({
+      agentId: 'agent-new',
+      task: {
+        id: 'sys-rl-1', taskType: 'internal',
+        metadata: {
+          useWorktree: true, reviewLoopFollowUp: true,
+          reviewLoopPRBranch: 'cos/task-x/agent-y',
+          reviewLoopPRUrl: 'https://github.com/o/r/pull/1'
+        }
+      }
+    });
+
+    expect(createWorktree).toHaveBeenCalledWith('agent-new', expect.any(String), 'sys-rl-1',
+      expect.objectContaining({ existingBranch: 'cos/task-x/agent-y' }));
+  });
+});

server/services/worktreeManager.js

@@ -490,6 +490,49 @@ async function createWorktreeUnlocked(agentId, sourceWorkspace, taskId, options
   return { worktreePath, branchName, baseBranch, instanceId };
 }
 
+/**
+ * Locate the worktree that currently holds `branchName`, when it is one PortOS
+ * may take over — i.e. a tree `adoptWorktree` could legitimately move.
+ *
+ * `git worktree add` refuses to attach a second tree to a checked-out branch, so
+ * a task pointed at an existing branch (a review-loop/merge follow-up, a resume)
+ * cannot start while another tree holds it. Usually that holder is the finished
+ * run's own worktree, still there because `removeWorktree` won't delete a dirty
+ * tree — and it IS that branch's workspace, so adopting it is both the fastest
+ * path and the one that preserves the leftover work. Waiting for a teardown that
+ * is never coming just strands the pull request the follow-up exists to land.
+ *
+ * Refuses (returns null) for every holder PortOS doesn't own outright, because
+ * adoption MOVES the directory:
+ *   - 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 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.
+ *
+ * @param {string} sourceWorkspace - the parent git repository
+ * @param {string} branchName - branch to find a holder for (no `refs/heads/`)
+ * @param {object} [options]
+ * @param {Set<string>} [options.activeAgentIds] - agents currently running
+ * @returns {Promise<{ path: string, agentId: string }|null>}
+ */
+export async function findAdoptableWorktreeForBranch(sourceWorkspace, branchName, { activeAgentIds = new Set() } = {}) {
+  if (!sourceWorkspace || !branchName) return null;
+
+  const worktrees = await listWorktrees(sourceWorkspace).catch(() => []);
+  // Only one worktree can hold a branch, so the first match is the only match —
+  // a guard failing means "nobody may adopt this", not "keep looking".
+  const holder = worktrees.find(wt => wt.branch?.replace('refs/heads/', '') === branchName);
+  if (!holder?.path || holder.locked) return null;
+  if (!isPathInsideDir(WORKTREES_DIR, holder.path)) return null;
+
+  const agentId = worktreeAgentId(holder.path);
+  if (!agentId || isHumanClaimWorktree(agentId) || activeAgentIds.has(agentId)) return null;
+
+  return { path: holder.path, agentId };
+}
+
 /**
  * Adopt an INTERRUPTED agent's surviving worktree on behalf of the agent that is
  * retrying its task, instead of building a fresh one from the default branch.