refactor: consolidate CoS worktree ownership

.changelog/next/changed-issue-4241.md

@@ -0,0 +1 @@
+- CoS task recovery now finds and reuses its existing worktree more reliably.

server/lib/README.md

@@ -256,7 +256,9 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub
 | `taskBlockCategories.js` | The `blockedCategory` vocabulary — which blocks are the system's to clear and which are a person's, read by the pause logic, the failure reaper, and the investigation auto-retry so they can't drift into three literal sets. `PAUSED_BLOCKED_CATEGORIES` (paused until something outside the task changes — keeps the resume pointer), `USER_DECISION_BLOCKED_CATEGORIES` (user intent / open decision — the reaper's exemption), `NON_AUTO_RETRY_BLOCK_CATEGORIES` (their union — a completed investigation must never revive these), `TIMED_COOLDOWN_BLOCKED_CATEGORIES` (a timer clears these — the cooldown sweeper revives them, so block reporters stay quiet). Pure. |
 | `taskRequeue.js` | The REQUEUE stamp (#3376) — pure metadata helpers for the one BACKWARD lifecycle step (`in_progress → pending`, performed by the orphan sweep and the retry-hold release). `REQUEUED_AT_KEY` / `LAST_SPAWNED_AT_KEY` name the two stamps; `isPostSpawnRequeue(pendingTask, inProgressTask)` answers whether the requeue happened strictly AFTER that spawn, which is how the federated merge tells a real requeue apart from an ordinary edit landing on a peer's stale `pending` copy. Returns false when either stamp is missing, so callers fall back to the lifecycle rank. |
 | `taskRetryHold.js` | The failed-task RETRY HOLD state (#3373) — pure metadata helpers shared by the failure verdict, the post-cleanup release, the spawn guard, and the orphan sweep. `retryHoldMetadata(agentId, now)` arms it (task stays `in_progress`, so no dequeue tier can claim the retry before its resume pointer is resolved); `clearedRetryHoldMetadata()` releases it in the same write that flips the task to `pending`; `isRetryHeld(metadata)` / `isRetryHoldOwner(metadata, agentId)` gate the spawn and the owner-scoped release; `isStaleRetryHold(metadata, now, graceMs)` + `RETRY_HOLD_GRACE_MS` let the orphan sweep finish a transition whose process died. |
+| `taskTargetBranch.js` | Pure task-branch contract: `resolveTaskTargetBranch(metadata)` reads a retry's legacy `existingBranch` or a review-loop follow-up's canonical `reviewLoopPRBranch`; `shouldStripTaskTargetBranch(metadata)` identifies only retry-owned pointers for terminal cleanup. |
 | `taxonomyTally.js` | Generic taxonomy tally + top-N-line render engine shared by the two Layered Intelligence leaf taxonomies (`services/layeredIntelligenceRejections.js`, `layeredIntelligenceExecutionFailures.js`). `createTaxonomyTally({predicate, select, field, vocabulary, sentinel, glossFn, gapWording})` is the single composed seam — it binds a taxonomy config into `{summarize, format}`, where `summarize(records)` yields the three-bucket `{entries, unknown, unclassified, diagnosed, total}` tally (commonest-first + taxonomy-order tie-break) and `format(records, limit)` renders one prompt line naming every non-zero gap. Also exports the two leaf utilities the classifiers use directly: `normalizeToken(value)` (lowercase + separator-collapse for label/category matching) and `formatTaxonomyToken(token, labels)` (gloss-map render, nullish→'', unglossed passthrough). Pure leaf — imports nothing from the LI graph. |
+| `worktreeOwnership.js` | Pure ownership gate for destructive worktree operations. `worktreeOwnershipReason()` applies explicit root, agent-id, claim, lock, and active-agent policies; `worktreeAgentId()` is separator-safe; `isHumanClaimWorktree()` and `isAgentWorktreeId()` make the protected namespaces explicit. |
 | `xmlEntities.js` | Shared dependency-free XML/HTML entity decoder. `decodeXmlEntities(str, extraEntities?)` — single-pass (double-decode-safe) decode of the five predefined named entities + decimal/hex numeric refs, with an optional caller-supplied extra-entity map (e.g. `{ nbsp: ' ', zwnj: '' }`). Unknown/out-of-range refs left untouched. Used by the Apple Health XML parser, Claude changelog feed, Pinterest RSS, generic feeds, and Gmail HTML-to-text. |
 
 ## Curated static data

server/lib/index.js

@@ -241,7 +241,9 @@ export * from './taskPauseHold.js';
 export * from './taskBlockCategories.js';
 export * from './taskRequeue.js';
 export * from './taskRetryHold.js';
+export * from './taskTargetBranch.js';
 export * from './taxonomyTally.js';
+export * from './worktreeOwnership.js';
 export * from './xmlEntities.js';
 
 // === Curated static data ===

server/lib/taskTargetBranch.js

@@ -0,0 +1,33 @@
+/**
+ * Task target-branch metadata — one reader and one terminal-strip rule.
+ *
+ * A retry owns `existingBranch` as a short-lived resume pointer. A review-loop
+ * follow-up instead owns `reviewLoopPRBranch`, which survives terminal cleanup so
+ * it can continue to repair and merge the same PR. Older follow-ups may contain
+ * both fields, so resolution remains backward compatible while new writers use
+ * the single canonical review-loop key.
+ */
+
+const isTruthyMetadataFlag = (value) => value === true || value === 'true';
+
+/**
+ * Resolve the branch a task must work on, or null when it should cut a fresh one.
+ * The legacy `existingBranch` wins when present; review-loop follow-ups fall back
+ * to their canonical PR-branch field.
+ */
+export function resolveTaskTargetBranch(metadata) {
+  if (metadata?.existingBranch) return metadata.existingBranch;
+  if (isTruthyMetadataFlag(metadata?.reviewLoopFollowUp) && metadata?.reviewLoopPRBranch) {
+    return metadata.reviewLoopPRBranch;
+  }
+  return null;
+}
+
+/**
+ * Does this metadata carry a retry-owned branch pointer that terminal cleanup
+ * must clear? Review-loop follow-ups use `reviewLoopPRBranch` instead, so an
+ * older duplicate `existingBranch` can be removed safely once a retry owns it.
+ */
+export function shouldStripTaskTargetBranch(metadata) {
+  return !!metadata?.resumedFromAgentId;
+}

server/lib/taskTargetBranch.test.js

@@ -0,0 +1,34 @@
+import { describe, expect, it } from 'vitest';
+import { resolveTaskTargetBranch, shouldStripTaskTargetBranch } from './taskTargetBranch.js';
+
+describe('task target branch', () => {
+  it('prefers a retry or legacy explicit pointer', () => {
+    expect(resolveTaskTargetBranch({
+      existingBranch: 'cos/task-1/agent-1',
+      reviewLoopFollowUp: true,
+      reviewLoopPRBranch: 'cos/task-2/agent-2',
+    })).toBe('cos/task-1/agent-1');
+  });
+
+  it('uses the canonical review-loop branch when no legacy duplicate exists', () => {
+    expect(resolveTaskTargetBranch({
+      reviewLoopFollowUp: true,
+      reviewLoopPRBranch: 'cos/task-1/agent-1',
+    })).toBe('cos/task-1/agent-1');
+    expect(resolveTaskTargetBranch({
+      reviewLoopFollowUp: 'true',
+      reviewLoopPRBranch: 'cos/task-1/agent-1',
+    })).toBe('cos/task-1/agent-1');
+  });
+
+  it('does not attach an original task merely because it carries PR metadata', () => {
+    expect(resolveTaskTargetBranch({ reviewLoopPRBranch: 'cos/task-1/agent-1' })).toBeNull();
+    expect(resolveTaskTargetBranch({ reviewLoopFollowUp: false, reviewLoopPRBranch: 'cos/task-1/agent-1' })).toBeNull();
+  });
+
+  it('strips only a retry-owned pointer at a terminal transition', () => {
+    expect(shouldStripTaskTargetBranch({ resumedFromAgentId: 'agent-1' })).toBe(true);
+    expect(shouldStripTaskTargetBranch({ reviewLoopFollowUp: true, reviewLoopPRBranch: 'cos/task-1/agent-1' })).toBe(false);
+    expect(shouldStripTaskTargetBranch({ existingBranch: 'feature/x' })).toBe(false);
+  });
+});

server/lib/worktreeOwnership.js

@@ -0,0 +1,92 @@
+/**
+ * Worktree ownership — the one policy for whether PortOS may move or remove a
+ * worktree.
+ *
+ * Worktree operations are destructive: adoption moves a directory and reapers
+ * remove one. The callers therefore share this pure gate instead of carrying
+ * slightly different copies of "managed root, agent id, claim, liveness, lock".
+ * Callers can explicitly opt into the differences that are intentional: a
+ * reaper may include `.claude/worktrees/`, and stale claims may be reclaimed
+ * only by branch reconciliation.
+ */
+
+import { win32 } from 'path';
+import { isPathInsideDir } from './fileUtils.js';
+
+/** Directory basename from either POSIX or Windows git worktree output. */
+export function worktreeAgentId(worktreePath) {
+  return win32.basename(worktreePath || '');
+}
+
+/** True for a worktree owned by the human `/claim` lifecycle. */
+export function isHumanClaimWorktree(agentId) {
+  return typeof agentId === 'string' && agentId.startsWith('claim-');
+}
+
+/** True for the directory naming convention exclusively owned by CoS agents. */
+export function isAgentWorktreeId(agentId) {
+  return typeof agentId === 'string' && agentId.startsWith('agent-');
+}
+
+function normalizedRoots(roots) {
+  return (Array.isArray(roots) ? roots : [])
+    .filter((root) => typeof root?.path === 'string' && root.path);
+}
+
+/**
+ * Why PortOS must leave a worktree alone, or null when this caller may handle it.
+ *
+ * `roots` is an explicit allowlist. Each root may opt into arbitrary directory
+ * names with `{ path, requireAgentId: false }`, which is how the safe merged-tree
+ * reaper can include `.claude/worktrees/` without weakening the CoS-agent root.
+ * `requireKnownLiveness` fails closed for `agent-*` trees when an authoritative
+ * `Set` of live agents is unavailable.
+ *
+ * @param {{
+ *   path?: string,
+ *   locked?: boolean,
+ *   activeAgentIds?: Set<string>,
+ *   roots?: Array<{path:string, requireAgentId?:boolean}>,
+ *   requireAgentId?: boolean,
+ *   allowStaleClaim?: boolean,
+ *   ageMs?: number|null,
+ *   staleClaimIdleMs?: number,
+ *   requireKnownLiveness?: boolean,
+ * }} options
+ * @returns {string|null}
+ */
+export function worktreeOwnershipReason({
+  path,
+  locked = false,
+  activeAgentIds,
+  roots = [],
+  requireAgentId = false,
+  allowStaleClaim = false,
+  ageMs = null,
+  staleClaimIdleMs,
+  requireKnownLiveness = false,
+} = {}) {
+  if (!path) return 'worktree-missing-path';
+
+  const configuredRoots = normalizedRoots(roots);
+  const root = configuredRoots.find((candidate) => isPathInsideDir(candidate.path, path));
+  if (configuredRoots.length > 0 && !root) return 'worktree-unmanaged-location';
+
+  const agentId = worktreeAgentId(path);
+  if (isHumanClaimWorktree(agentId)) {
+    const stale = allowStaleClaim
+      && typeof ageMs === 'number'
+      && typeof staleClaimIdleMs === 'number'
+      && ageMs >= staleClaimIdleMs;
+    if (!stale) return 'worktree-human-claim';
+  }
+
+  const mustBeAgentWorktree = root?.requireAgentId ?? requireAgentId;
+  if (mustBeAgentWorktree && !isAgentWorktreeId(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)) {
+    return 'worktree-agent-liveness-unknown';
+  }
+  return null;
+}

server/lib/worktreeOwnership.test.js

@@ -0,0 +1,48 @@
+import { describe, expect, it } from 'vitest';
+import { isAgentWorktreeId, isHumanClaimWorktree, worktreeAgentId, worktreeOwnershipReason } from './worktreeOwnership.js';
+
+describe('worktree ownership', () => {
+  const COS_ROOT = '/repo/data/cos/worktrees';
+
+  it('permits only an inactive, unlocked CoS agent tree under the configured root', () => {
+    const options = {
+      roots: [{ path: COS_ROOT, requireAgentId: true }],
+      activeAgentIds: new Set(),
+      requireKnownLiveness: true,
+    };
+    expect(worktreeOwnershipReason({ ...options, path: `${COS_ROOT}/agent-dead` })).toBeNull();
+    expect(worktreeOwnershipReason({ ...options, path: '/repo/elsewhere/agent-dead' })).toBe('worktree-unmanaged-location');
+    expect(worktreeOwnershipReason({ ...options, path: `${COS_ROOT}/next-issue-42` })).toBe('worktree-missing-agent-id');
+    expect(worktreeOwnershipReason({ ...options, path: `${COS_ROOT}/agent-live`, activeAgentIds: new Set(['agent-live']) }))
+      .toBe('worktree-active-agent');
+    expect(worktreeOwnershipReason({ ...options, path: `${COS_ROOT}/agent-locked`, locked: true })).toBe('worktree-locked');
+  });
+
+  it('keeps human claims unless the stale-claim caller explicitly permits reclamation', () => {
+    const input = { path: `${COS_ROOT}/claim-issue-42`, ageMs: 8_000, staleClaimIdleMs: 7_000 };
+    expect(worktreeOwnershipReason(input)).toBe('worktree-human-claim');
+    expect(worktreeOwnershipReason({ ...input, allowStaleClaim: true })).toBeNull();
+  });
+
+  it('fails closed when agent liveness is unknown and permits an explicitly non-agent root', () => {
+    expect(worktreeOwnershipReason({
+      path: `${COS_ROOT}/agent-unknown`,
+      requireAgentId: true,
+      requireKnownLiveness: true,
+    })).toBe('worktree-agent-liveness-unknown');
+    expect(worktreeOwnershipReason({
+      path: '/repo/.claude/worktrees/review-fix',
+      roots: [{ path: '/repo/.claude/worktrees', requireAgentId: false }],
+      activeAgentIds: new Set(),
+      requireKnownLiveness: true,
+    })).toBeNull();
+  });
+
+  it('uses one separator-safe namespace definition', () => {
+    expect(worktreeAgentId('H:/repo/data/cos/worktrees/agent-abc')).toBe('agent-abc');
+    expect(worktreeAgentId('H:\\repo\\data\\cos\\worktrees\\claim-issue-42')).toBe('claim-issue-42');
+    expect(isAgentWorktreeId('agent-abc')).toBe(true);
+    expect(isAgentWorktreeId('next-issue-42')).toBe(false);
+    expect(isHumanClaimWorktree('claim-issue-42')).toBe(true);
+  });
+});

server/services/agentManagement.js

@@ -27,6 +27,7 @@ import { activeAgents, runnerAgents, userTerminatedAgents, pausedAgents, useRunn
 // lets that edge be a plain static import instead of a dynamic-import dodge.
 import { cleanupAgentWorktree, resolveTaskResumePatch } from './agentWorktreeCleanup.js';
 import { isRetryHeld, clearedRetryHoldMetadata } from '../lib/taskRetryHold.js';
+import { resolveTaskTargetBranch } from '../lib/taskTargetBranch.js';
 import { syncRunnerAgents } from './agentRunnerSync.js';
 import { flushRunnerOutputBatcher } from './agentRunnerOutputBatchers.js';
 import { completeAgentRun } from './agentRunTracking.js';
@@ -408,7 +409,7 @@ async function requeuePausedTask({ task, taskType, overrides }) {
       status: 500, code: 'AGENT_RESUME_FAILED'
     });
   }
-  return { taskId: task.id, mode: 'requeued', branchName: result?.metadata?.existingBranch || null };
+  return { taskId: task.id, mode: 'requeued', branchName: resolveTaskTargetBranch(result?.metadata) };
 }
 
 /**
@@ -1145,8 +1146,9 @@ export async function handleOrphanedTask(taskId, agentId, getTaskByIdFn, { agent
       emitLog('warn', `Resume pointer for held task ${taskId} could not be resolved: ${err.message}`, { taskId, agentId });
       return {};
     });
-    emitLog('info', `🔓 Completing interrupted retry transition for task ${taskId}${resumePatch.existingBranch ? ` — resuming ${resumePatch.existingBranch}` : ''}`, {
-      taskId, agentId, branchName: resumePatch.existingBranch || null
+    const targetBranch = resolveTaskTargetBranch(resumePatch);
+    emitLog('info', `🔓 Completing interrupted retry transition for task ${taskId}${targetBranch ? ` — resuming ${targetBranch}` : ''}`, {
+      taskId, agentId, branchName: targetBranch
     });
     await updateTask(taskId, {
       status: 'pending',

server/services/agentManagement.test.js

@@ -752,6 +752,16 @@ describe('resumeAgent — requeues the paused agent\'s own task', () => {
     });
   });
 
+  it('reports a review-loop task’s canonical PR branch when no legacy duplicate exists', async () => {
+    reviveBlockedTask.mockResolvedValueOnce({ metadata: {
+      reviewLoopFollowUp: true,
+      reviewLoopPRBranch: 'cos/task-abc/agent-pr',
+    } });
+    await expect(resumeAgent('agent-paused-1')).resolves.toMatchObject({
+      mode: 'requeued', branchName: 'cos/task-abc/agent-pr',
+    });
+  });
+
   it('retires the paused agent record so it stops showing as paused', async () => {
     await resumeAgent('agent-paused-1');
     expect(markAgentComplete).toHaveBeenCalledWith('agent-paused-1', expect.objectContaining({