diff --git a/.changelog/next/changed-issue-4241.md b/.changelog/next/changed-issue-4241.md new file mode 100644 index 0000000000..1327cfe7d8 --- /dev/null +++ b/.changelog/next/changed-issue-4241.md @@ -0,0 +1 @@ +- CoS task recovery now finds and reuses its existing worktree more reliably. diff --git a/server/lib/README.md b/server/lib/README.md index 3bf542c1e3..f8913bfd29 100644 --- a/server/lib/README.md +++ b/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 diff --git a/server/lib/index.js b/server/lib/index.js index 7b5cc7ba3e..559d741963 100644 --- a/server/lib/index.js +++ b/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 === diff --git a/server/lib/taskTargetBranch.js b/server/lib/taskTargetBranch.js new file mode 100644 index 0000000000..7dbe83be0b --- /dev/null +++ b/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; +} diff --git a/server/lib/taskTargetBranch.test.js b/server/lib/taskTargetBranch.test.js new file mode 100644 index 0000000000..68e11e43b0 --- /dev/null +++ b/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); + }); +}); diff --git a/server/lib/worktreeOwnership.js b/server/lib/worktreeOwnership.js new file mode 100644 index 0000000000..f2ab756b79 --- /dev/null +++ b/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, + * 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; +} diff --git a/server/lib/worktreeOwnership.test.js b/server/lib/worktreeOwnership.test.js new file mode 100644 index 0000000000..8558baee6e --- /dev/null +++ b/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); + }); +}); diff --git a/server/services/agentManagement.js b/server/services/agentManagement.js index 0ddfd0ca02..14d33c8779 100644 --- a/server/services/agentManagement.js +++ b/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', diff --git a/server/services/agentManagement.test.js b/server/services/agentManagement.test.js index 726cdea856..b910657cfe 100644 --- a/server/services/agentManagement.test.js +++ b/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({ diff --git a/server/services/agentWorkspacePrep.js b/server/services/agentWorkspacePrep.js index 995f700746..285f763627 100644 --- a/server/services/agentWorkspacePrep.js +++ b/server/services/agentWorkspacePrep.js @@ -37,6 +37,7 @@ import { detectConflicts } from './taskConflict.js'; import { createWorktree, adoptWorktree, findAdoptableWorktreeForBranch, isBranchCheckedOutElsewhereError } from './worktreeManager.js'; import { resolveSpawnCwd } from '../lib/spawnCwd.js'; import { enforceSafeBranchUpstream } from '../lib/branchUpstreamGuard.js'; +import { resolveTaskTargetBranch } from '../lib/taskTargetBranch.js'; import { getAppWorkspace, getAppDataForTask, createJiraTicketForTask } from './agentPromptBuilder.js'; const ROOT_DIR = PATHS.root; @@ -75,30 +76,9 @@ 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; -} +// Compatibility export for callers that reached for the accessor from this +// service before the shared task-target-branch contract existed. +export { resolveTaskTargetBranch as resolveTaskExistingBranch } from '../lib/taskTargetBranch.js'; /** * Agent ids whose worktree must not be taken out from under them, using the same @@ -125,19 +105,19 @@ async function getProtectedAgentIds() { * 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. + * Resolved before `createWorktree`, so both a resume and a review-loop follow-up + * share one answer to "which tree holds this branch?". Routinely that holder is a + * cleanup's tree seconds from teardown, but a tree `removeWorktree` REFUSED to + * delete (uncommitted changes) holds the branch until a human intervenes. 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 }) { +async function adoptWorktreeHoldingBranch({ agentId, workspacePath, branchName, preferredPath = null, 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. @@ -147,7 +127,7 @@ async function adoptWorktreeHoldingBranch({ agentId, workspacePath, branchName, }); if (!activeAgentIds) return null; - const holder = await findAdoptableWorktreeForBranch(workspacePath, branchName, { activeAgentIds }); + const holder = await findAdoptableWorktreeForBranch(workspacePath, branchName, { activeAgentIds, preferredPath }); if (!holder) return null; const worktreeInfo = await adoptWorktree(agentId, workspacePath, holder.path, branchName).catch(err => { @@ -223,7 +203,7 @@ 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 existingBranch = resolveTaskExistingBranch(task.metadata); + const existingBranch = resolveTaskTargetBranch(task.metadata); const wantsWorktree = explicitWorktree || !!existingBranch; if (!isReadOnly) { @@ -349,7 +329,28 @@ export async function prepareAgentWorkspace({ agentId, task }) { } if (wantsWorktree && !jiraBranchName) { - const { baseBranch: detectedBase } = await git.getRepoBranches(workspacePath).catch(() => ({ baseBranch: null })); + // Detecting the base branch and resolving the branch holder are independent + // reads (a git-branches lookup vs. an agent-liveness + worktree-list check) — + // kick both off before awaiting either so their I/O overlaps instead of + // serializing on the spawn hot path. + const detectedBasePromise = git.getRepoBranches(workspacePath).catch(() => ({ baseBranch: null })); + // Resolve the branch holder ONCE before creation. `resumeWorktreePath` is a + // cache of that answer, not a separate ownership rule: if it is gone or + // stale, discovery finds the actual holder. This gives resume retries the + // 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; + const takeoverPromise = existingBranch + ? adoptWorktreeHoldingBranch({ + agentId, + workspacePath, + branchName: existingBranch, + preferredPath: resumeWorktreePath, + taskId: task.id, + }) + : Promise.resolve(null); + + const { baseBranch: detectedBase } = await detectedBasePromise; if (existingBranch) { emitLog('info', `🌳 Worktree requested for task ${task.id} on existing branch ${existingBranch}`, { taskId: task.id, app: task.metadata?.app, branch: existingBranch @@ -360,27 +361,7 @@ export async function prepareAgentWorkspace({ agentId, task }) { }); } - // Resume path: the run this task is retrying left a worktree behind (its - // process died mid-edit, so `removeWorktree` refused to delete the dirty - // tree — see recordTaskResumePointer). Adopt it, which carries the - // uncommitted edits and untracked files no branch pointer can, instead of - // building a fresh tree and redoing that work. - const resumeWorktreePath = existingBranch ? task.metadata?.resumeWorktreePath : null; - const adopted = resumeWorktreePath - ? await adoptWorktree(agentId, workspacePath, resumeWorktreePath, existingBranch).catch(err => { - emitLog('warn', `🌳 Could not adopt worktree ${resumeWorktreePath} for task ${task.id}: ${err.message}`, { taskId: task.id }); - return null; - }) - : null; - - // Adoption failing while the stale tree is STILL on disk means the branch is - // checked out there, so attaching a second worktree to it would fail with - // "already checked out" and block the task outright. Fail open — start clean, - // the same polarity resolveResumePointer uses — rather than not spawning. - const branchStillClaimed = !adopted && resumeWorktreePath && existsSync(resumeWorktreePath); - if (branchStillClaimed) { - emitLog('warn', `🌳 Worktree ${resumeWorktreePath} could not be adopted and still holds ${existingBranch} — task ${task.id} starts from a clean branch`, { taskId: task.id }); - } + const takeover = await takeoverPromise; // Both read only by the block/pause decision below: the failure REASON // decides whether the task is unrunnable or merely early, and `attempt` is @@ -389,9 +370,9 @@ export async function prepareAgentWorkspace({ agentId, task }) { // task's whole patience budget rather than a per-attempt one). let worktreeError = null; const attempt = (Number(task.metadata?.worktreeBusyAttempts) || 0) + 1; - worktreeInfo = adopted || await createWorktree(agentId, workspacePath, task.id, { + worktreeInfo = takeover?.worktreeInfo || await createWorktree(agentId, workspacePath, task.id, { baseBranch: detectedBase || undefined, - existingBranch: branchStillClaimed ? undefined : (existingBranch || undefined), + existingBranch: existingBranch || undefined, planId: task.metadata?.planId || undefined }).catch(err => { worktreeError = err; @@ -399,15 +380,6 @@ 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 diff --git a/server/services/agentWorkspacePrep.test.js b/server/services/agentWorkspacePrep.test.js index 4a9942eb03..496500f686 100644 --- a/server/services/agentWorkspacePrep.test.js +++ b/server/services/agentWorkspacePrep.test.js @@ -180,9 +180,14 @@ describe('prepareAgentWorkspace — resuming an interrupted run', () => { } }); - beforeEach(() => { ensureLatest.mockResolvedValue({ success: true, upToDate: true }); }); + beforeEach(() => { + ensureLatest.mockResolvedValue({ success: true, upToDate: true }); + getAgents.mockResolvedValue([]); + findAdoptableWorktreeForBranch.mockResolvedValue(null); + }); it('adopts the interrupted run’s worktree instead of creating a new one', async () => { + findAdoptableWorktreeForBranch.mockResolvedValue({ path: DEAD_TREE, agentId: 'agent-dead' }); adoptWorktree.mockResolvedValue({ worktreePath: '/mock/worktrees/agent-new', branchName: 'cos/t-resume/agent-dead', baseBranch: null, existingBranch: true, adopted: true @@ -213,24 +218,22 @@ describe('prepareAgentWorkspace — resuming an interrupted run', () => { expect(r.outcome).toBe('ready'); }); - // Git allows a branch in only ONE worktree. If adoption failed while the stale - // tree is still on disk holding the branch, attaching a second worktree to it - // errors out and the task is blocked entirely — worse than starting clean. - it('starts clean when adoption fails and the stale tree still holds the branch', async () => { - // Any real directory stands in for the stale tree — the production check is a - // bare `existsSync`, so `cwd` is enough and leaves nothing behind to clean up. - const stillThere = process.cwd(); - adoptWorktree.mockResolvedValue(null); - createWorktree.mockResolvedValue({ - worktreePath: '/mock/worktrees/agent-new', branchName: 'cos/t-resume/agent-new', baseBranch: 'main' + it('discovers and adopts the branch holder before cutting a fresh resume branch', async () => { + const stalePointer = '/mock/worktrees/agent-moved'; + findAdoptableWorktreeForBranch.mockResolvedValue({ path: DEAD_TREE, agentId: 'agent-dead' }); + adoptWorktree.mockResolvedValue({ + worktreePath: '/mock/worktrees/agent-new', branchName: 'cos/t-resume/agent-dead', + baseBranch: null, existingBranch: true, adopted: true }); const r = await prepareAgentWorkspace({ - agentId: 'agent-new', task: resumeTask({ resumeWorktreePath: stillThere }) + agentId: 'agent-new', task: resumeTask({ resumeWorktreePath: stalePointer }) }); - expect(createWorktree).toHaveBeenCalledWith('agent-new', expect.any(String), 't-resume', - expect.objectContaining({ existingBranch: undefined })); + expect(findAdoptableWorktreeForBranch).toHaveBeenCalledWith(expect.any(String), 'cos/t-resume/agent-dead', + expect.objectContaining({ preferredPath: stalePointer })); + expect(adoptWorktree).toHaveBeenCalledWith('agent-new', expect.any(String), DEAD_TREE, 'cos/t-resume/agent-dead'); + expect(createWorktree).not.toHaveBeenCalled(); expect(r.outcome).toBe('ready'); }); @@ -239,6 +242,7 @@ describe('prepareAgentWorkspace — resuming an interrupted run', () => { // detection returns `proceed` once the dead agent is gone, so the old gate sent // the retry into the shared workspace and abandoned the work on disk. it('takes the worktree path for a resume even when the task never asked for isolation', async () => { + findAdoptableWorktreeForBranch.mockResolvedValue({ path: DEAD_TREE, agentId: 'agent-dead' }); adoptWorktree.mockResolvedValue({ worktreePath: '/mock/worktrees/agent-new', branchName: 'cos/t-resume/agent-dead', baseBranch: null, existingBranch: true, adopted: true @@ -301,8 +305,8 @@ describe('prepareAgentWorkspace — the branch is checked out in another worktre id: 'sys-rl-1', taskType: 'internal', metadata: { useWorktree: true, - existingBranch: 'cos/task-x/agent-y', reviewLoopFollowUp: true, + reviewLoopPRBranch: 'cos/task-x/agent-y', reviewLoopPRUrl: 'https://github.com/o/r/pull/1', ...extra } @@ -395,9 +399,9 @@ describe('prepareAgentWorkspace — the branch is checked out in another worktre const [, patch] = updateTask.mock.calls.at(-1); expect(patch.status).toBe('blocked'); expect(patch.metadata.blockedCategory).toBe('worktree-busy'); - // `worktree-busy` is a PAUSE category, so updateTask keeps `existingBranch` — + // `worktree-busy` is a PAUSE category, so its canonical PR branch survives — // the revived attempt must still attach to the PR branch, not cut a new one. - expect(patch.metadata.existingBranch).toBe('cos/task-x/agent-y'); + expect(patch.metadata.reviewLoopPRBranch).toBe('cos/task-x/agent-y'); // The cooldown stamp is what the sweeper in cosTaskGenerator revives on. expect(new Date(patch.metadata.cooldownUntil).getTime()).toBeGreaterThan(Date.now()); expect(patch.metadata.worktreeBusyAttempts).toBe(1); diff --git a/server/services/agentWorktreeCleanup.js b/server/services/agentWorktreeCleanup.js index 02ea380f12..4c228641b1 100644 --- a/server/services/agentWorktreeCleanup.js +++ b/server/services/agentWorktreeCleanup.js @@ -23,6 +23,7 @@ import { removeWorktree, classifyWorktreeDirt } from './worktreeManager.js'; import { isTruthyMeta } from './agentState.js'; import { PATHS } from '../lib/fileUtils.js'; import { isRetryHoldOwner, clearedRetryHoldMetadata } from '../lib/taskRetryHold.js'; +import { resolveTaskTargetBranch, shouldStripTaskTargetBranch } from '../lib/taskTargetBranch.js'; import { RECOVERY_TASK_PREFIX } from './recoveryTasks.js'; import { detectForgeCli } from '../lib/gitForge.js'; import { PR_COMPLETIONS, PR_COMPLETION_VALUES, PR_CREATION, leavesPrForHuman, prClaimWasVerified } from '../lib/prDisposition.js'; @@ -584,18 +585,18 @@ export async function resolveTaskResumePatch({ task, agentId, agentMetadata }) { /** * The task-metadata patch that makes a retry resume (or stop resuming). * - * `existingBranch` is the flag agentWorkspacePrep already honors to attach a - * worktree to a pre-existing branch (the review-loop follow-up uses it), and - * `resumeWorktreePath` is honored there too, so resuming needs no new spawn - * plumbing. `resumedFromAgentId` records whose run is being continued — it drives - * the prompt's resume banner and is the marker that distinguishes a resume from - * the follow-up (see `isPrBranchWorktree` in agentPromptBuilder.js). + * `existingBranch` is the retry-owned target that `agentWorkspacePrep` resolves + * before attaching a worktree, and `resumeWorktreePath` is honored there too, so + * resuming needs no new spawn plumbing. `resumedFromAgentId` records whose run is + * being continued — it drives the prompt's resume banner and is the marker that + * distinguishes a resume from a review-loop follow-up (see `isPrBranchWorktree` + * in agentPromptBuilder.js). * * With no pointer, a previously-stamped resume is CLEARED: its branch may since * have been merged or deleted, and leaving the pointer would attach the next * attempt to landed work. Keyed on `resumedFromAgentId` so it only ever clears a - * pointer this mechanism wrote — the review-loop follow-up's own `existingBranch` - * is its whole reason for existing and must survive being orphaned. + * pointer this mechanism wrote. Review-loop follow-ups keep their canonical target + * in `reviewLoopPRBranch`, so clearing a legacy duplicate cannot strand the PR. * * @param {{branchName: string, worktreePath: string|null}|null} pointer * @param {string} agentId - the run being resumed from @@ -610,7 +611,7 @@ export function resumePointerMetadata(pointer, agentId, task) { resumeWorktreePath: pointer.worktreePath }; } - if (!task?.metadata?.resumedFromAgentId) return {}; + if (!shouldStripTaskTargetBranch(task?.metadata)) return {}; // `undefined`, not `null`: `updateTask` DELETES undefined keys from the merged // metadata, while a null survives the merge and TASKS.md serializes it as the // literal string `"null"` — which reads back as a truthy `existingBranch` and @@ -634,7 +635,7 @@ export async function recordTaskResumePointer({ task, agentId, agentMetadata }) await updateTask(task.id, { metadata }, task.taskType || 'user').catch(err => { emitLog('warn', `Failed to record resume pointer for task ${task.id}: ${err.message}`, { taskId: task.id, agentId }); }); - if (!metadata.existingBranch) { + if (!resolveTaskTargetBranch(metadata)) { emitLog('info', `🧹 Cleared spent resume pointer on task ${task.id} — nothing left to resume`, { taskId: task.id, agentId }); } return metadata; @@ -734,9 +735,10 @@ export async function releaseRetryHold({ agentId, task, success, agentMetadata } emitLog('warn', `⏳ Task ${task.id} still held after a failed release — the orphan sweep will requeue it`, { taskId: task.id, agentId }); return {}; } - emitLog('info', patch.existingBranch - ? `🔁 Task ${task.id} requeued pointing at ${patch.existingBranch}` - : `🔓 Task ${task.id} requeued for retry`, { taskId: task.id, agentId, branchName: patch.existingBranch || null }); + const targetBranch = resolveTaskTargetBranch(patch); + emitLog('info', targetBranch + ? `🔁 Task ${task.id} requeued pointing at ${targetBranch}` + : `🔓 Task ${task.id} requeued for retry`, { taskId: task.id, agentId, branchName: targetBranch }); return patch; } @@ -759,9 +761,9 @@ export async function releaseRetryHold({ agentId, task, success, agentMetadata } * then merge versus merge on green directly; `leave-open` is intentional and * returns without creating a follow-up. * - * The follow-up task uses an isolated worktree attached to the existing PR - * branch (via createWorktree's `existingBranch` option) so it can fix-and-push - * without trampling concurrent agents. + * The follow-up task uses an isolated worktree attached to the existing PR branch + * through its canonical `reviewLoopPRBranch`, so it can fix-and-push without + * trampling concurrent agents. */ export async function spawnReviewLoopFollowUp({ originalAgentId, originalTask, prUrl, prBranch, sourceWorkspace, prCompletion = PR_COMPLETIONS.REVIEW_THEN_MERGE, reviewers = DEFAULT_REVIEWERS, usernames = [], optionalReviewers = [], reviewerMaxRounds = {}, reviewStopMode = DEFAULT_REVIEW_STOP_MODE, reviewerApplies = false, reviewerModels = null, reviewerEfforts = null, leaveOpen = false }) { if (!prUrl || !prBranch) return null; @@ -869,10 +871,9 @@ export async function spawnReviewLoopFollowUp({ originalAgentId, originalTask, p metadata: { app: appId, ...providerPins, - // useWorktree is required so the follow-up runs in isolation; existingBranch - // tells createWorktree to attach to the PR branch instead of cutting a new one. + // useWorktree is required so the follow-up runs in isolation. Its canonical + // reviewLoopPRBranch below tells the shared resolver which PR branch to attach. useWorktree: true, - existingBranch: prBranch, // openPR/reviewLoop must stay false so cleanup doesn't try to create another PR // or request another initial review (the agent itself drives the loop) openPR: false, diff --git a/server/services/branchReconcile.js b/server/services/branchReconcile.js index ab0279ee57..e2f1b29cc3 100644 --- a/server/services/branchReconcile.js +++ b/server/services/branchReconcile.js @@ -25,7 +25,8 @@ import { stat } from 'node:fs/promises'; import { getBranches, getDefaultBranch, isBranchMergedInto, deleteBranch } from './git.js'; import { execGit } from '../lib/execGit.js'; -import { listWorktrees, forceRemoveWorktreeDir, classifyWorktreeDirt, isHumanClaimWorktree } from './worktreeManager.js'; +import { listWorktrees, forceRemoveWorktreeDir, classifyWorktreeDirt } from './worktreeManager.js'; +import { isAgentWorktreeId, worktreeOwnershipReason } from '../lib/worktreeOwnership.js'; import { execGh, ensureForgeReachable } from './github.js'; import { getOriginInfo } from '../lib/gitRemote.js'; import { githubRepoSpec, githubApiHost } from '../lib/workTracker.js'; @@ -234,22 +235,15 @@ async function getOpenPrsByHead(repoPath) { * @returns {string|null} */ export function worktreeProtectionReason({ path, locked, activeAgentIds, ageMs, staleClaimIdleMs = STALE_CLAIM_IDLE_MS }) { - if (locked) return 'worktree-locked'; - const basename = (path || '').split('/').pop() || ''; - if (isHumanClaimWorktree(basename)) { - // Reap an abandoned claim (age known AND past the idle window); keep protecting - // a recent one so a live human /claim session — or its Phase-7 self-clean — wins. - // Unknown age (ageMs omitted) stays protected: fail safe toward not-deleting. - if (typeof ageMs === 'number' && ageMs >= staleClaimIdleMs) return null; - return 'worktree-human-claim'; - } - // `instanceof Set`, not truthiness: `getActiveAgentIds()` returns an ARRAY, and a - // caller passing it raw would otherwise throw `.has is not a function` mid-cleanup. - // Non-Set ⇒ liveness unknown ⇒ not protected here; see isAbandonedAgentWorktree's - // JSDoc below for the sentinel rationale, and resolveLiveOwnerReason for the - // dispatch side, which fails the other way on purpose. - if (activeAgentIds instanceof Set && activeAgentIds.has(basename)) return 'worktree-active-agent'; - return null; + if (!path) return null; + return worktreeOwnershipReason({ + path, + locked, + activeAgentIds, + allowStaleClaim: true, + ageMs, + staleClaimIdleMs, + }); } /** @@ -275,10 +269,13 @@ export function worktreeProtectionReason({ path, locked, activeAgentIds, ageMs, * @returns {boolean} */ export function isAbandonedAgentWorktree({ path, locked, activeAgentIds }) { - if (!path || locked || !(activeAgentIds instanceof Set)) return false; - const basename = path.split('/').pop() || ''; - if (!basename.startsWith('agent-') || isHumanClaimWorktree(basename)) return false; - return !activeAgentIds.has(basename); + return worktreeOwnershipReason({ + path, + locked, + activeAgentIds, + requireAgentId: true, + requireKnownLiveness: true, + }) === null; } /** @@ -308,15 +305,19 @@ export function resolveLiveOwnerReason({ branch, path, locked, activeAgentIds, a // The branch's own trailing segment is an agent id for CoS branches — checked // FIRST because it holds even after the worktree is gone. const owner = (branch || '').split('/').pop() || ''; - if (owner.startsWith('agent-') && activeAgentIds instanceof Set && activeAgentIds.has(owner)) { + if (isAgentWorktreeId(owner) && activeAgentIds instanceof Set && activeAgentIds.has(owner)) { return 'branch-active-agent'; } if (!path) return null; - const reason = worktreeProtectionReason({ path, locked, activeAgentIds, ageMs }); - if (reason) return reason; - const basename = path.split('/').pop() || ''; - if (basename.startsWith('agent-') && !(activeAgentIds instanceof Set)) return 'worktree-agent-liveness-unknown'; - return null; + return worktreeOwnershipReason({ + path, + locked, + activeAgentIds, + allowStaleClaim: true, + ageMs, + staleClaimIdleMs: STALE_CLAIM_IDLE_MS, + requireKnownLiveness: true, + }); } /** diff --git a/server/services/branchReconcile.test.js b/server/services/branchReconcile.test.js index d9387ca64a..f67bac7fe9 100644 --- a/server/services/branchReconcile.test.js +++ b/server/services/branchReconcile.test.js @@ -33,7 +33,6 @@ vi.mock('./worktreeManager.js', () => ({ realChangePaths: lines.map((l) => l.replace(/^\s*\S+\s+/, '')) }; }), - isHumanClaimWorktree: vi.fn((id) => typeof id === 'string' && id.startsWith('claim-')) })); const ensureForgeReachableMock = vi.fn(async () => ({ ok: true, status: 'ok', detail: null, remedy: null })); vi.mock('./github.js', () => ({ @@ -52,6 +51,8 @@ const tryReadFileMock = vi.fn(async () => null); vi.mock('../lib/fileUtils.js', () => ({ PATHS: { root: '/repo', cos: '/repo/data/cos' }, safeJSONParse: (raw, fallback) => { try { return JSON.parse(raw); } catch { return fallback; } }, + isPathInsideDir: (dir, candidate) => typeof dir === 'string' && typeof candidate === 'string' + && candidate.startsWith(`${dir}/`), tryReadFile: (...args) => tryReadFileMock(...args), atomicWrite: vi.fn(async () => {}) })); diff --git a/server/services/cleanupAgentWorktree.test.js b/server/services/cleanupAgentWorktree.test.js index 6c72a97644..6a35a16c6a 100644 --- a/server/services/cleanupAgentWorktree.test.js +++ b/server/services/cleanupAgentWorktree.test.js @@ -664,7 +664,7 @@ describe('cleanupAgentWorktree - PR-creation path', () => { expect(followUp.metadata.reviewLoopPRNumber).toBe(42); expect(followUp.metadata.reviewLoopPROwner).toBe('test'); expect(followUp.metadata.reviewLoopPRRepo).toBe('repo'); - expect(followUp.metadata.existingBranch).toBe('cos/task-abc123'); + expect(followUp.metadata.existingBranch).toBeUndefined(); expect(followUp.metadata.useWorktree).toBe(true); expect(followUp.metadata.openPR).toBe(false); // must not chain another PR expect(followUp.metadata.reviewLoop).toBe(false); // must not chain another loop @@ -848,7 +848,8 @@ describe('cleanupAgentWorktree - PR-creation path', () => { expect(followUp.metadata.reviewLoopReviewerUsernames).toEqual([]); expect(followUp.description).toMatch(/^\[Merge\]/); // Still attaches to the PR branch so it can fix a failing check before merging. - expect(followUp.metadata.existingBranch).toBe('cos/task-abc123'); + expect(followUp.metadata.reviewLoopPRBranch).toBe('cos/task-abc123'); + expect(followUp.metadata.existingBranch).toBeUndefined(); expect(removeWorktree).toHaveBeenCalled(); }); diff --git a/server/services/cosTaskStore.js b/server/services/cosTaskStore.js index 28e6ff2930..f819497e4f 100644 --- a/server/services/cosTaskStore.js +++ b/server/services/cosTaskStore.js @@ -20,6 +20,7 @@ import { REVIEW_STOP_MODES, normalizeReviewers, normalizeReviewUsernames, normal import { isPlainObject } from '../lib/objects.js'; import { PR_COMPLETIONS, PR_COMPLETION_VALUES } from '../lib/prDisposition.js'; import { RETRY_HOLD_KEY, RETRY_HOLD_SINCE_KEY } from '../lib/taskRetryHold.js'; +import { resolveTaskTargetBranch, shouldStripTaskTargetBranch } from '../lib/taskTargetBranch.js'; import { AGENT_PAUSED_CATEGORY, PAUSE_METADATA_KEYS, isAgentPausedTask, resolvePausedTaskResume, retirePausedAgent } from '../lib/taskPauseHold.js'; import { REQUEUED_AT_KEY } from '../lib/taskRequeue.js'; import { isInvestigationTask } from '../lib/investigationTasks.js'; @@ -557,7 +558,7 @@ export async function updateTask(taskId, updates, taskType = 'user', { now = Dat const release = await preparePauseRelease(taskId, updates); const result = await writeTaskUpdate(taskId, release ? { ...updates, metadata: release.metadata } : updates, taskType, { now }); if (release && !result?.error) { - await retirePausedAgent(release.agentId, taskId, result?.metadata?.existingBranch || null); + await retirePausedAgent(release.agentId, taskId, resolveTaskTargetBranch(result?.metadata)); } return result; } @@ -660,17 +661,10 @@ async function writeTaskUpdate(taskId, updates, taskType, { now }) { // starts clean and abandons the worktree its dead agent left behind — which is // exactly the recovery this mechanism exists for. if (isTerminalTaskStatus(updates.status) && !PAUSED_BLOCKED_CATEGORIES.has(updatedMetadata.blockedCategory)) { - // ...but only the RESUME mechanism's `existingBranch`, which is keyed by the - // `resumedFromAgentId` it is always written with — the same discriminator - // `resumePointerMetadata` (agentWorktreeCleanup.js) uses to avoid clearing a - // pointer it didn't write. A review-loop / merge follow-up sets - // `existingBranch` as its own CONFIGURATION: it exists to land the PR on that - // branch. Stripping that copy meant re-running a blocked merge follow-up got - // a worktree cut fresh off the default branch — the merge still targets the - // right PR by url, but any fix-and-push lands on a branch the PR has never - // heard of. Keyed on the mechanism rather than on the task kind so the next - // producer of a self-configured `existingBranch` doesn't re-hit this. - if (updatedMetadata.resumedFromAgentId) delete updatedMetadata.existingBranch; + // The shared predicate identifies only retry-owned `existingBranch` pointers. + // Review-loop follow-ups own `reviewLoopPRBranch`, so their canonical target + // remains intact even when a legacy duplicate is removed here. + if (shouldStripTaskTargetBranch(updatedMetadata)) delete updatedMetadata.existingBranch; delete updatedMetadata.resumedFromAgentId; delete updatedMetadata.resumeWorktreePath; } diff --git a/server/services/cosTaskStore.test.js b/server/services/cosTaskStore.test.js index 26b1655016..9745f30d94 100644 --- a/server/services/cosTaskStore.test.js +++ b/server/services/cosTaskStore.test.js @@ -994,6 +994,22 @@ describe('cosTaskStore.updateTask', () => { expect(done.metadata.resumeWorktreePath).toBeUndefined(); }); + it('keeps a review-loop target after clearing its legacy retry duplicate', async () => { + await addTask({ description: 'merge PR canonical', id: 'sys-rl-canonical' }, 'internal'); + await updateTask('sys-rl-canonical', { + metadata: { + existingBranch: 'cos/task-4/agent-old', + resumedFromAgentId: 'agent-old', + resumeWorktreePath: '/w/agent-old', + reviewLoopFollowUp: true, + reviewLoopPRBranch: 'cos/task-4/agent-pr', + } + }, 'internal'); + const done = await updateTask('sys-rl-canonical', { status: 'completed' }, 'internal'); + expect(done.metadata.existingBranch).toBeUndefined(); + expect(done.metadata.reviewLoopPRBranch).toBe('cos/task-4/agent-pr'); + }); + // `worktree-busy` joins the pause categories: the cooldown sweeper revives it, // and the revived attempt must still attach to the PR branch. it('keeps the pointer through a worktree-busy cooldown block', async () => { diff --git a/server/services/worktreeManager.js b/server/services/worktreeManager.js index 281baacf2d..91c5990792 100644 --- a/server/services/worktreeManager.js +++ b/server/services/worktreeManager.js @@ -11,25 +11,27 @@ import { existsSync, realpathSync } from 'fs'; import { readdir, rm, stat } from 'fs/promises'; -import { join, win32 } from 'path'; +import { join } from 'path'; import { ensureDir, isPathInsideDir, PATHS, sleep, tryReadFile } from '../lib/fileUtils.js'; import { DONE_SENTINEL_NAME, doneSentinelName } from '../lib/agentSentinel.js'; import { execGit } from '../lib/execGit.js'; import { createKeyCachedQueue } from '../lib/createKeyCachedQueue.js'; import { enforceSafeBranchUpstream } from '../lib/branchUpstreamGuard.js'; +import { isHumanClaimWorktree, worktreeAgentId, worktreeOwnershipReason } from '../lib/worktreeOwnership.js'; import { ensureInstanceId } from './instances.js'; +export { isHumanClaimWorktree } from '../lib/worktreeOwnership.js'; + const WORKTREES_DIR = PATHS.worktrees; // `git worktree list --porcelain` reports POSIX separators on every platform — // `H:/repo/data/cos/worktrees/agent-x` — while WORKTREES_DIR is built with // `join` and is backslash-separated on Windows. So a git-reported path is never // compared with a bare `startsWith`/`split('/')`: `isPathInsideDir` resolves // both sides first (and rejects a sibling like `worktrees-old`), and -// `win32.basename` treats either separator as one. Before this, every CoS +// `worktreeAgentId()` treats either separator as one. Before this, every CoS // worktree failed the containment check on Windows — `cleanupOrphanedWorktrees` // skipped them all and `reapMergedWorktrees` filed them as `unmanaged-location`, // so the daily line read "reaped 0 merged + 0 orphaned" with orphans on disk. -const worktreeAgentId = (worktreePath) => win32.basename(worktreePath || ''); // Lockfiles that npm/yarn/pnpm modify as a side-effect — safe to discard during worktree cleanup const AUTO_GENERATED_LOCKFILES = ['package-lock.json', 'yarn.lock', 'pnpm-lock.yaml']; // Cap the dirty paths named in a "worktree preserved" warning — the message ends @@ -317,24 +319,6 @@ function pathsEqual(a, b) { return resolved(a) === resolved(b); } -/** - * True when a worktree directory belongs to a human-driven `/claim` TUI - * session, not a CoS agent. - * - * The `/claim` command creates its worktree at `data/cos/worktrees/claim-` - * — the SAME directory CoS uses for agent worktrees (`agent-`). CoS - * agent IDs are always `agent-<8-char-uuid>` (see `agentLifecycle.js`), so the - * `claim-` prefix is unambiguous. These worktrees are owned by the `/claim` - * command's own Phase 7 cleanup; CoS orphan-cleanup MUST skip them. Otherwise - * every cleanup cycle (boot + each evaluation) sees a `claim-` dir with - * no matching active agent, treats it as orphaned, and removes it — pruning a - * human's in-flight claim mid-review (and, with `{ merge: true }`, even - * fast-forwarding the `claim/` branch into the default branch). - */ -export function isHumanClaimWorktree(agentId) { - return typeof agentId === 'string' && agentId.startsWith('claim-'); -} - /** * Decide whether an auto-merge into `currentBranch` should be refused. * @@ -515,20 +499,34 @@ async function createWorktreeUnlocked(agentId, sourceWorkspace, taskId, options * @param {string} branchName - branch to find a holder for (no `refs/heads/`) * @param {object} [options] * @param {Set} [options.activeAgentIds] - agents currently running + * @param {string} [options.preferredPath] - cached holder path to validate first * @returns {Promise<{ path: string, agentId: string }|null>} */ -export async function findAdoptableWorktreeForBranch(sourceWorkspace, branchName, { activeAgentIds = new Set() } = {}) { +export async function findAdoptableWorktreeForBranch(sourceWorkspace, branchName, { + activeAgentIds = new Set(), + preferredPath = null, +} = {}) { 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; + // Git permits one holder per branch. A resume pointer is merely a cache of that + // answer, so validate it against the current worktree list first and then fall + // back to discovery when the cached path went stale or was moved. + const holders = worktrees.filter(wt => wt.branch?.replace('refs/heads/', '') === branchName); + const holder = preferredPath + ? holders.find(wt => pathsEqual(wt.path, preferredPath)) || holders[0] + : holders[0]; + if (!holder?.path) return null; const agentId = worktreeAgentId(holder.path); - if (!agentId || isHumanClaimWorktree(agentId) || activeAgentIds.has(agentId)) return null; + const ownershipReason = worktreeOwnershipReason({ + path: holder.path, + locked: holder.locked, + activeAgentIds, + roots: [{ path: WORKTREES_DIR, requireAgentId: true }], + requireKnownLiveness: true, + }); + if (ownershipReason) return null; return { path: holder.path, agentId }; } @@ -1005,25 +1003,27 @@ export async function cleanupOrphanedWorktrees(sourceWorkspace, activeAgentIds) const handledAgentIds = new Set(); for (const wt of worktrees) { - // Only clean up worktrees under our managed directory - if (!isPathInsideDir(WORKTREES_DIR, wt.path)) continue; - + const ownershipReason = worktreeOwnershipReason({ + path: wt.path, + locked: wt.locked, + activeAgentIds, + roots: [{ path: WORKTREES_DIR, requireAgentId: true }], + requireKnownLiveness: true, + }); + if (ownershipReason === 'worktree-unmanaged-location') continue; const agentId = worktreeAgentId(wt.path); handledAgentIds.add(agentId); - // Never reap human-driven `/claim` worktrees (`claim-`) — they belong - // to the `/claim` command's own Phase 7 cleanup, not CoS. See isHumanClaimWorktree. - if (isHumanClaimWorktree(agentId)) continue; - if (!activeAgentIds.has(agentId)) { - const branchName = wt.branch?.replace('refs/heads/', '') || ''; - // Attempt merge so committed work from preserved worktrees (e.g., PR/push failures) isn't lost. - // If merge fails, the branch is preserved for manual recovery. - const result = await removeWorktree(agentId, sourceWorkspace, branchName, { merge: true }) - .catch(err => { - console.log(`⚠️ Failed to clean orphaned worktree ${agentId}: ${err.message}`); - return { removed: false }; - }); - if (result?.removed) cleaned++; - } + if (ownershipReason) continue; + + const branchName = wt.branch?.replace('refs/heads/', '') || ''; + // Attempt merge so committed work from preserved worktrees (e.g., PR/push failures) isn't lost. + // If merge fails, the branch is preserved for manual recovery. + const result = await removeWorktree(agentId, sourceWorkspace, branchName, { merge: true }) + .catch(err => { + console.log(`⚠️ Failed to clean orphaned worktree ${agentId}: ${err.message}`); + return { removed: false }; + }); + if (result?.removed) cleaned++; } // Scan for external-repo worktrees (directories whose .git points to a different repo). @@ -1087,6 +1087,10 @@ export async function reapMergedWorktrees(sourceWorkspace, { const protectedBranches = new Set(['main', 'master', 'dev', 'develop', 'release', defaultBranch]); const claudeTreesRoot = join(sourceWorkspace, '.claude', 'worktrees'); + const managedRoots = [ + { path: WORKTREES_DIR, requireAgentId: true }, + { path: claudeTreesRoot, requireAgentId: false }, + ]; const worktrees = await listWorktrees(sourceWorkspace).catch(() => []); const reaped = []; @@ -1101,16 +1105,25 @@ export async function reapMergedWorktrees(sourceWorkspace, { if (!branchName) { skipped.push({ path: wt.path, reason: 'no-branch' }); continue; } if (protectedBranches.has(branchName) || branchName === currentBranch) { skipped.push({ path: wt.path, reason: 'protected' }); continue; } - const agentId = worktreeAgentId(wt.path); - // Human `/claim` worktrees self-clean in the claim flow's Phase 7 — never reap them here. - if (isHumanClaimWorktree(agentId)) { skipped.push({ path: wt.path, reason: 'human-claim' }); continue; } - if (activeAgentIds.has(agentId)) { skipped.push({ path: wt.path, reason: 'active-agent' }); continue; } - - const isCosTree = isPathInsideDir(WORKTREES_DIR, wt.path); const isClaudeTree = isPathInsideDir(claudeTreesRoot, wt.path); - if (!isCosTree && !isClaudeTree) { skipped.push({ path: wt.path, reason: 'unmanaged-location' }); continue; } + const ownershipReason = worktreeOwnershipReason({ + path: wt.path, + locked: wt.locked, + activeAgentIds, + roots: managedRoots, + requireKnownLiveness: true, + }); + if (ownershipReason) { + const reason = { + 'worktree-human-claim': 'human-claim', + 'worktree-active-agent': 'active-agent', + 'worktree-locked': 'locked', + 'worktree-unmanaged-location': 'unmanaged-location', + }[ownershipReason] || ownershipReason; + skipped.push({ path: wt.path, reason }); + continue; + } if (isClaudeTree && !includeClaudeTrees) { skipped.push({ path: wt.path, reason: 'claude-tree-excluded' }); continue; } - if (wt.locked) { skipped.push({ path: wt.path, reason: 'locked' }); continue; } // Gate 1: working tree must be completely clean. Unlike removeWorktree(), // the background reaper does not discard even lockfile-only edits: an diff --git a/server/services/worktreeManager.test.js b/server/services/worktreeManager.test.js index 3f56dd4479..150e971a73 100644 --- a/server/services/worktreeManager.test.js +++ b/server/services/worktreeManager.test.js @@ -44,6 +44,7 @@ const { createPersistentWorktree, } = await import('./worktreeManager.js'); const { isPathInsideDir } = await import('../lib/fileUtils.js'); +const { worktreeOwnershipReason } = await import('../lib/worktreeOwnership.js'); const { win32 } = await import('path'); const { existsSync } = await import('fs'); const { PATHS } = await import('../lib/fileUtils.js'); @@ -429,17 +430,13 @@ describe('git-vs-PortOS path comparison', () => { describe('Orphaned Worktree Detection', () => { function findOrphanedWorktrees(worktrees, worktreesDir, activeAgentIds) { - return worktrees.filter(wt => { - // The real helpers, not a local re-implementation: a mirrored copy here is - // exactly what let the Windows separator bug live in the shipped path - // while this suite stayed green. - if (!isPathInsideDir(worktreesDir, wt.path)) return false; - const agentId = win32.basename(wt.path); - // Mirror the real cleanup guard: human-driven `/claim` worktrees are - // never CoS orphans. - if (isHumanClaimWorktree(agentId)) return false; - return !activeAgentIds.has(agentId); - }); + return worktrees.filter((wt) => worktreeOwnershipReason({ + path: wt.path, + locked: wt.locked, + activeAgentIds, + roots: [{ path: worktreesDir, requireAgentId: true }], + requireKnownLiveness: true, + }) === null); } it('should identify worktrees without active agents', () => { @@ -771,6 +768,12 @@ describe('findAdoptableWorktreeForBranch (take over the tree that holds the bran expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); }); + it('refuses a non-agent directory in the managed root', async () => { + scriptWorktrees([{ path: cosTree('next-issue-42'), branch: `refs/heads/${BRANCH}` }]); + + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); + }); + it('refuses a tree whose agent is still running — it is mid-edit in there', async () => { scriptWorktrees([{ path: cosTree('agent-y'), branch: `refs/heads/${BRANCH}` }]);