diff --git a/.changelog/next/fixed-agent-1e904405.md b/.changelog/next/fixed-agent-1e904405.md new file mode 100644 index 0000000000..d673554125 --- /dev/null +++ b/.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 diff --git a/server/services/agentWorkspacePrep.js b/server/services/agentWorkspacePrep.js index 385868323e..995f700746 100644 --- a/server/services/agentWorkspacePrep.js +++ b/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 diff --git a/server/services/agentWorkspacePrep.test.js b/server/services/agentWorkspacePrep.test.js index d7b8e9405c..4a9942eb03 100644 --- a/server/services/agentWorkspacePrep.test.js +++ b/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' })); + }); +}); diff --git a/server/services/worktreeManager.js b/server/services/worktreeManager.js index 7732b5bcf5..281baacf2d 100644 --- a/server/services/worktreeManager.js +++ b/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} [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. diff --git a/server/services/worktreeManager.test.js b/server/services/worktreeManager.test.js index 0f8e566e3c..3f56dd4479 100644 --- a/server/services/worktreeManager.test.js +++ b/server/services/worktreeManager.test.js @@ -39,6 +39,7 @@ const { isBranchCheckedOutElsewhereError, removeWorktree, adoptWorktree, + findAdoptableWorktreeForBranch, createWorktree, createPersistentWorktree, } = await import('./worktreeManager.js'); @@ -716,6 +717,84 @@ describe('isBranchCheckedOutElsewhereError (branch-busy pause gate)', () => { }); }); +describe('findAdoptableWorktreeForBranch (take over the tree that holds the branch)', () => { + const REPO = '/repo'; + const BRANCH = 'cos/task-x/agent-y'; + + // `git worktree list --porcelain`: the primary checkout first, then whatever + // entries a test names. + function scriptWorktrees(entries) { + execGitMock.mockReset(); + const stdout = [ + `worktree ${REPO}`, 'HEAD abc123', 'branch refs/heads/main', '', + ...entries.flatMap(e => [ + `worktree ${e.path}`, 'HEAD def456', + e.branch ? `branch ${e.branch}` : 'detached', + ...(e.locked ? ['locked'] : []), '' + ]) + ].join('\n'); + execGitMock.mockResolvedValue({ stdout, stderr: '' }); + } + + const cosTree = (agentId) => join(PATHS.worktrees, agentId); + + it('finds the CoS worktree holding the branch', async () => { + scriptWorktrees([{ path: cosTree('agent-y'), branch: `refs/heads/${BRANCH}` }]); + + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)) + .toEqual({ path: cosTree('agent-y'), agentId: 'agent-y' }); + }); + + it('returns null when nothing holds the branch', async () => { + scriptWorktrees([{ path: cosTree('agent-z'), branch: 'refs/heads/cos/other/agent-z' }]); + + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); + }); + + // Adoption MOVES the directory, so a holder PortOS doesn't own is never a + // candidate — taking the user's own checkout is the branch-jacking this + // codebase guards against everywhere else. + it('refuses the primary checkout, and any tree outside the managed root', async () => { + scriptWorktrees([{ path: '/repo/../elsewhere/tree', branch: `refs/heads/${BRANCH}` }]); + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); + + // The repo root itself, checked out on the branch. + execGitMock.mockResolvedValue({ + stdout: `worktree ${REPO}\nHEAD abc\nbranch refs/heads/${BRANCH}\n`, stderr: '' + }); + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); + }); + + it('refuses a human /claim worktree — the claim flow owns its cleanup', async () => { + scriptWorktrees([{ path: cosTree('claim-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}` }]); + + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH, { + activeAgentIds: new Set(['agent-y']) + })).toBeNull(); + }); + + it('refuses a locked worktree whatever else is true of it', async () => { + scriptWorktrees([{ path: cosTree('agent-y'), branch: `refs/heads/${BRANCH}`, locked: true }]); + + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); + }); + + it('returns null rather than throwing when the listing fails', async () => { + execGitMock.mockReset(); + execGitMock.mockRejectedValue(new Error('not a git repository')); + + expect(await findAdoptableWorktreeForBranch(REPO, BRANCH)).toBeNull(); + expect(await findAdoptableWorktreeForBranch(REPO, '')).toBeNull(); + expect(await findAdoptableWorktreeForBranch('', BRANCH)).toBeNull(); + }); +}); + describe('removeWorktree branch preservation for resume (#3167)', () => { // Routes each git invocation this path makes to a scripted answer, keyed on the // subcommand, so a test only has to state what it cares about instead of