diff --git a/src/git/agent-worktree.test.ts b/src/git/agent-worktree.test.ts index 1c1699b..756da6d 100644 --- a/src/git/agent-worktree.test.ts +++ b/src/git/agent-worktree.test.ts @@ -1,6 +1,6 @@ import { execFile } from 'node:child_process' -import { mkdtemp, mkdir, rm, stat, writeFile } from 'node:fs/promises' -import { join } from 'node:path' +import { mkdtemp, mkdir, rm, stat, symlink, writeFile } from 'node:fs/promises' +import { basename, dirname, join } from 'node:path' import { tmpdir } from 'node:os' import { promisify } from 'node:util' import { describe, expect, it } from 'vitest' @@ -52,13 +52,182 @@ describe('GitAgentWorktreeManager', () => { it('refuses cleanup outside the Factory worktree root', async () => { const manager = new GitAgentWorktreeManager() - await expect(manager.cleanup({ + const unsafe = { repo: 'AgentWorkforce/pear', issueKey: 'AR-33', baseClonePath: '/work/pear', worktreePath: '/work/pear', branch: 'factory/ar-33-pear', - })).rejects.toThrow(/unsafe Factory worktree path/u) + } + await expect(manager.cleanup(unsafe)).rejects.toThrow(/unsafe Factory worktree path/u) + await expect(manager.inspectForCleanup(unsafe)).rejects.toThrow(/unsafe Factory worktree path/u) + }) + + it('refuses a Factory-root symlink that resolves to a registered checkout outside the root', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-agent-worktree-symlink-safety-')) + const base = join(root, 'PearCheckout') + const outside = join(root, 'outside-worktree') + try { + await mkdir(base) + await git(base, ['init', '-b', 'main']) + await git(base, ['config', 'user.email', 'factory@example.test']) + await git(base, ['config', 'user.name', 'Factory Test']) + await writeFile(join(base, 'README.md'), '# pear\n', 'utf8') + await git(base, ['add', 'README.md']) + await git(base, ['commit', '-m', 'initial']) + const worktreePath = factoryWorktreePath(base, 'AR-34', 'AgentWorkforce/pear', 'escape01') + await git(base, ['worktree', 'add', '-b', 'factory/ar-34-pear-escape01', outside]) + await mkdir(dirname(worktreePath), { recursive: true }) + await symlink(outside, worktreePath, 'dir') + const worktree = { + repo: 'AgentWorkforce/pear', + issueKey: 'AR-34', + baseClonePath: base, + worktreePath, + branch: 'factory/ar-34-pear-escape01', + } + const manager = new GitAgentWorktreeManager() + + await expect(manager.inspectForCleanup(worktree)).rejects.toThrow(/resolved target is/u) + await expect(manager.cleanup(worktree)).rejects.toThrow(/resolved target is/u) + await expect(stat(outside)).resolves.toMatchObject({}) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + + it('refuses cleanup when the Factory checkout root itself redirects outside the owned tree', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-agent-worktree-root-symlink-safety-')) + const base = join(root, 'PearCheckout') + const outsideRoot = join(root, 'outside-root') + try { + await mkdir(base) + await git(base, ['init', '-b', 'main']) + await git(base, ['config', 'user.email', 'factory@example.test']) + await git(base, ['config', 'user.name', 'Factory Test']) + await writeFile(join(base, 'README.md'), '# pear\n', 'utf8') + await git(base, ['add', 'README.md']) + await git(base, ['commit', '-m', 'initial']) + const worktreePath = factoryWorktreePath(base, 'AR-35', 'AgentWorkforce/pear', 'escape02') + const expectedRoot = dirname(worktreePath) + const outside = join(outsideRoot, basename(worktreePath)) + await mkdir(outsideRoot) + await git(base, ['worktree', 'add', '-b', 'factory/ar-35-pear-escape02', outside]) + await mkdir(dirname(expectedRoot), { recursive: true }) + await symlink(outsideRoot, expectedRoot, 'dir') + const worktree = { + repo: 'AgentWorkforce/pear', + issueKey: 'AR-35', + baseClonePath: base, + worktreePath, + branch: 'factory/ar-35-pear-escape02', + } + const manager = new GitAgentWorktreeManager() + + await expect(manager.inspectForCleanup(worktree)).rejects.toThrow(/symbolic-link root/u) + await expect(manager.cleanup(worktree)).rejects.toThrow(/symbolic-link root/u) + await expect(stat(outside)).resolves.toMatchObject({}) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + + it('refuses parent cleanup through a symbolic-link Factory container when the checkout is missing', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-agent-worktree-container-symlink-safety-')) + const base = join(root, 'PearCheckout') + const outsideFactory = join(root, 'outside-factory') + try { + await mkdir(base) + await git(base, ['init', '-b', 'main']) + const worktreePath = factoryWorktreePath(base, 'AR-36', 'AgentWorkforce/pear', 'escape03') + const expectedRoot = dirname(worktreePath) + const factoryContainer = dirname(expectedRoot) + const outsideCheckout = join(outsideFactory, basename(expectedRoot)) + await mkdir(outsideCheckout, { recursive: true }) + await symlink(outsideFactory, factoryContainer, 'dir') + const worktree = { + repo: 'AgentWorkforce/pear', + issueKey: 'AR-36', + baseClonePath: base, + worktreePath, + branch: 'factory/ar-36-pear-escape03', + } + const manager = new GitAgentWorktreeManager() + + await expect(manager.inspectForCleanup(worktree)).rejects.toThrow(/symbolic-link root/u) + await expect(manager.cleanup(worktree)).rejects.toThrow(/symbolic-link root/u) + await expect(stat(outsideCheckout)).resolves.toMatchObject({}) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + + it('discovers every run and detects dirty, unpushed, and locked worktrees before cleanup', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-agent-worktree-safety-')) + const remote = join(root, 'remote.git') + const base = join(root, 'PearCheckout') + try { + await git(root, ['init', '--bare', remote]) + await mkdir(base) + await git(base, ['init', '-b', 'main']) + await git(base, ['config', 'user.email', 'factory@example.test']) + await git(base, ['config', 'user.name', 'Factory Test']) + await writeFile(join(base, 'README.md'), '# pear\n', 'utf8') + await git(base, ['add', 'README.md']) + await git(base, ['commit', '-m', 'initial']) + await git(base, ['remote', 'add', 'origin', remote]) + await git(base, ['push', '-u', 'origin', 'main']) + + const manager = new GitAgentWorktreeManager() + const create = async (runId: string) => { + const worktreePath = factoryWorktreePath(base, 'AR-123', 'AgentWorkforce/pear', runId) + const worktree = { + repo: 'AgentWorkforce/pear', + issueKey: 'AR-123', + baseClonePath: base, + worktreePath, + branch: `factory/ar-123-pear-${runId}`, + } + await manager.prepare(worktree) + return worktree + } + const clean = await create('11111111') + const dirty = await create('22222222') + const unpushed = await create('33333333') + const locked = await create('44444444') + await writeFile(join(dirty.worktreePath, 'dirty.txt'), 'local work\n', 'utf8') + await writeFile(join(unpushed.worktreePath, 'commit.txt'), 'not pushed\n', 'utf8') + await git(unpushed.worktreePath, ['add', 'commit.txt']) + await git(unpushed.worktreePath, ['commit', '-m', 'local only']) + await git(base, ['worktree', 'lock', locked.worktreePath]) + + const discovered = await manager.listWorktrees({ repo: 'AgentWorkforce/pear', baseClonePath: base }) + expect(discovered.map((worktree) => worktree.worktreePath)).toEqual([ + clean.worktreePath, + dirty.worktreePath, + unpushed.worktreePath, + locked.worktreePath, + ]) + expect(discovered.every((worktree) => worktree.issueKey === 'ar-123')).toBe(true) + + await expect(manager.inspectForCleanup(clean)).resolves.toMatchObject({ retentionReasons: [] }) + await expect(manager.inspectForCleanup(dirty)).resolves.toMatchObject({ + retentionReasons: expect.arrayContaining(['uncommitted changes']), + }) + await expect(manager.inspectForCleanup(unpushed)).resolves.toMatchObject({ + retentionReasons: expect.arrayContaining(['1 unpushed commit']), + }) + const lockedInspection = await manager.inspectForCleanup(locked) + expect(lockedInspection.retentionReasons.some((reason) => reason.startsWith('git lock present'))).toBe(true) + + await manager.cleanup(clean) + await expect(stat(clean.worktreePath)).rejects.toMatchObject({ code: 'ENOENT' }) + await expect(stat(dirty.worktreePath)).resolves.toMatchObject({}) + await expect(stat(unpushed.worktreePath)).resolves.toMatchObject({}) + await expect(stat(locked.worktreePath)).resolves.toMatchObject({}) + } finally { + await rm(root, { recursive: true, force: true }) + } }) it('checks out a verified numeric legacy PR head from origin without synthesizing it from base', async () => { @@ -147,7 +316,7 @@ describe('GitAgentWorktreeManager', () => { })).rejects.toThrow(/unsafe Factory worktree branch/u) }) - it('prunes an unrelated deleted registration before validating an existing checkout', async () => { + it('ignores an unrelated deleted locked registration when validating an existing checkout', async () => { const root = await mkdtemp(join(tmpdir(), 'factory-agent-worktree-prune-')) const base = join(root, 'pear') try { @@ -172,9 +341,13 @@ describe('GitAgentWorktreeManager', () => { const stalePath = join(root, 'deleted-unrelated-worktree') await git(base, ['worktree', 'add', '-b', 'unrelated-stale', stalePath, 'main']) + await git(base, ['worktree', 'lock', stalePath]) await rm(stalePath, { recursive: true, force: true }) await expect(manager.prepare(worktree)).resolves.toBeUndefined() + await expect(manager.inspectForCleanup(worktree)).resolves.toMatchObject({ + retentionReasons: ['1 unpushed commit'], + }) expect(await git(worktreePath, ['branch', '--show-current'])) .toBe('factory/ar-34-pear-abcdefgh\n') } finally { diff --git a/src/git/agent-worktree.ts b/src/git/agent-worktree.ts index c406c45..a6c43f0 100644 --- a/src/git/agent-worktree.ts +++ b/src/git/agent-worktree.ts @@ -1,9 +1,14 @@ import { execFile } from 'node:child_process' -import { mkdir, realpath, rmdir, stat } from 'node:fs/promises' +import { lstat, mkdir, readdir, realpath, rmdir, stat } from 'node:fs/promises' import { basename, dirname, join, resolve } from 'node:path' import { promisify } from 'node:util' -import type { AgentWorktree, AgentWorktreeManager } from '../ports/worktree' +import type { + AgentWorktree, + AgentWorktreeCleanupInspection, + AgentWorktreeManager, + AgentWorktreeRepository, +} from '../ports/worktree' const execFileAsync = promisify(execFile) @@ -113,6 +118,7 @@ export class GitAgentWorktreeManager implements AgentWorktreeManager { async cleanup(worktree: AgentWorktree): Promise { await this.#prepares.get(resolve(worktree.worktreePath)) assertSafeWorktree(worktree) + await assertNonSymlinkFactoryRoot(factoryWorktreeRoot(worktree.baseClonePath)) await this.#git(worktree.baseClonePath, ['worktree', 'prune']) if (!await pathExists(worktree.worktreePath)) { await removeEmptyWorktreeParents(worktree.worktreePath) @@ -124,8 +130,102 @@ export class GitAgentWorktreeManager implements AgentWorktreeManager { await removeEmptyWorktreeParents(worktree.worktreePath) } + async listWorktrees(repository: AgentWorktreeRepository): Promise { + const root = factoryWorktreeRoot(repository.baseClonePath) + let entries + try { + entries = await readdir(root, { withFileTypes: true }) + } catch (error) { + if (errorCode(error) === 'ENOENT') return [] + throw error + } + + const repoSlug = sanitizeSegment(repository.repo.split('/').at(-1) ?? repository.repo) + const pattern = new RegExp(`^(.+)-${escapeRegExp(repoSlug)}-(.+)$`, 'u') + const worktrees: AgentWorktree[] = [] + for (const entry of entries.sort((a, b) => a.name.localeCompare(b.name))) { + if (!entry.isDirectory()) continue + const match = entry.name.match(pattern) + if (!match?.[1]) continue + const worktreePath = join(root, entry.name) + // Keep malformed or detached matching directories visible to the caller. + // Their synthetic unsafe branch makes inspection fail closed and ensures + // Factory logs the retained path without one bad entry hiding siblings. + let branch = '__factory-unreadable__' + try { + branch = (await this.#git(worktreePath, ['symbolic-ref', '--short', 'HEAD'])).trim() + } catch { + // Inspection will reject this candidate before deletion. + } + const issueKey = match[1] + worktrees.push({ + repo: repository.repo, + issueKey, + baseClonePath: repository.baseClonePath, + worktreePath, + branch, + ...(isAuthorizedExistingPrBranch({ + repo: repository.repo, + issueKey, + baseClonePath: repository.baseClonePath, + worktreePath, + branch, + existingPullRequestBranch: true, + }) ? { existingPullRequestBranch: true } : {}), + }) + } + return worktrees + } + + async inspectForCleanup(worktree: AgentWorktree): Promise { + assertSafeWorktree(worktree) + await assertNonSymlinkFactoryRoot(factoryWorktreeRoot(worktree.baseClonePath)) + // Inspection runs before cleanup, so it must clear unrelated deleted + // registrations itself. Otherwise #assertRegisteredCheckout can fail on + // realpath() for a stale sibling and retain every healthy candidate in the + // repository without ever reaching cleanup's existing prune step. + await this.#git(worktree.baseClonePath, ['worktree', 'prune']) + if (!await pathExists(worktree.worktreePath)) return { bytes: 0, retentionReasons: [] } + await this.#assertRegisteredCheckout(worktree) + + const retentionReasons: string[] = [] + const gitDir = resolveGitPath( + worktree.worktreePath, + (await this.#git(worktree.worktreePath, ['rev-parse', '--git-dir'])).trim(), + ) + const commonDir = resolveGitPath( + worktree.worktreePath, + (await this.#git(worktree.worktreePath, ['rev-parse', '--git-common-dir'])).trim(), + ) + const locks = await relevantGitLocks(gitDir, commonDir, worktree.branch) + if (locks.length > 0) retentionReasons.push(`git lock present (${locks.join(', ')})`) + + const status = await this.#git(worktree.worktreePath, ['status', '--porcelain', '--untracked-files=all']) + if (status.trim()) retentionReasons.push('uncommitted changes') + + const unpushed = Number.parseInt( + (await this.#git(worktree.worktreePath, ['rev-list', '--count', 'HEAD', '--not', '--remotes'])).trim(), + 10, + ) + if (!Number.isFinite(unpushed)) { + throw new Error(`Unable to determine unpushed commits for ${worktree.worktreePath}`) + } + if (unpushed > 0) retentionReasons.push(`${unpushed} unpushed commit${unpushed === 1 ? '' : 's'}`) + + return { + bytes: await directorySize(worktree.worktreePath), + retentionReasons, + } + } + async #assertRegisteredCheckout(worktree: AgentWorktree): Promise { const wanted = await realpath(worktree.worktreePath) + const expectedRootPath = factoryWorktreeRoot(worktree.baseClonePath) + await assertNonSymlinkFactoryRoot(expectedRootPath) + const expectedRoot = await realpath(expectedRootPath) + if (wanted === expectedRoot || !wanted.startsWith(`${expectedRoot}/`)) { + throw new Error(`Refusing Factory worktree operation outside ${expectedRoot}: resolved target is ${wanted}`) + } const root = await realpath((await this.#git(worktree.worktreePath, ['rev-parse', '--show-toplevel'])).trim()) if (root !== wanted) { throw new Error(`Refusing Factory worktree operation outside ${wanted}: git root is ${root}`) @@ -134,7 +234,9 @@ export class GitAgentWorktreeManager implements AgentWorktreeManager { .split(/\r?\n/u) .filter((line) => line.startsWith('worktree ')) .map((line) => line.slice('worktree '.length).trim()) - const registered = await Promise.all(registeredPaths.map(async (path) => await realpath(path))) + const registered = (await Promise.allSettled( + registeredPaths.map(async (path) => await realpath(path)), + )).flatMap((result) => result.status === 'fulfilled' ? [result.value] : []) if (!registered.includes(wanted)) { throw new Error(`Refusing Factory worktree operation for unregistered checkout ${wanted}`) } @@ -194,22 +296,26 @@ export function factoryWorktreePath( ): string { const repoSlug = sanitizeSegment(repo.split('/').at(-1) ?? repo) const issueSlug = sanitizeSegment(issueKey) - const checkoutSlug = sanitizeSegment(basename(resolve(baseClonePath))) return join( - dirname(resolve(baseClonePath)), - '.factory-worktrees', - checkoutSlug, + factoryWorktreeRoot(baseClonePath), `${issueSlug}-${repoSlug}-${runId.slice(0, 8)}`, ) } +export const factoryWorktreeIssueSlug = (issueKey: string): string => sanitizeSegment(issueKey) + +const factoryWorktreeRoot = (baseClonePath: string): string => { + const base = resolve(baseClonePath) + return resolve(dirname(base), '.factory-worktrees', sanitizeSegment(basename(base))) +} + const sanitizeSegment = (value: string): string => value.toLowerCase().replace(/[^a-z0-9._-]+/gu, '-').replace(/^-+|-+$/gu, '') || 'worktree' const assertSafeWorktree = (worktree: AgentWorktree): void => { const base = resolve(worktree.baseClonePath) const target = resolve(worktree.worktreePath) - const expectedRoot = resolve(dirname(base), '.factory-worktrees', sanitizeSegment(basename(base))) + const expectedRoot = factoryWorktreeRoot(base) if (target === base || !target.startsWith(`${expectedRoot}/`)) { throw new Error(`Refusing unsafe Factory worktree path ${target}; expected a child of ${expectedRoot}`) } @@ -226,8 +332,55 @@ const isAuthorizedExistingPrBranch = (worktree: AgentWorktree): boolean => const pathExists = async (path: string): Promise => { try { return (await stat(path)).isDirectory() - } catch { - return false + } catch (error) { + if (errorCode(error) === 'ENOENT') return false + throw error + } +} + +const resolveGitPath = (cwd: string, path: string): string => resolve(cwd, path) + +const relevantGitLocks = async (gitDir: string, commonDir: string, branch: string): Promise => { + const candidates = new Set([ + join(gitDir, 'locked'), + join(gitDir, 'index.lock'), + join(gitDir, 'HEAD.lock'), + join(commonDir, 'packed-refs.lock'), + join(commonDir, 'config.lock'), + join(commonDir, 'refs', 'heads', `${branch}.lock`), + ]) + const locks: string[] = [] + for (const path of candidates) { + try { + await lstat(path) + locks.push(path) + } catch (error) { + if (errorCode(error) !== 'ENOENT') throw error + } + } + return locks +} + +const directorySize = async (root: string): Promise => { + const entry = await lstat(root) + if (!entry.isDirectory()) return entry.size + let bytes = entry.size + for (const child of await readdir(root)) { + bytes += await directorySize(join(root, child)) + } + return bytes +} + +const assertNonSymlinkFactoryRoot = async (expectedRoot: string): Promise => { + for (const path of [dirname(expectedRoot), expectedRoot]) { + try { + if ((await lstat(path)).isSymbolicLink()) { + throw new Error(`Refusing Factory worktree operation through symbolic-link root ${path}`) + } + } catch (error) { + if (errorCode(error) === 'ENOENT') return + throw error + } } } @@ -239,11 +392,16 @@ const removeEmptyWorktreeParents = async (worktreePath: string): Promise = } const ignoreNonEmptyOrMissing = (error: unknown): void => { - const code = typeof error === 'object' && error !== null && 'code' in error ? String(error.code) : undefined + const code = errorCode(error) if (code === 'ENOENT' || code === 'ENOTEMPTY' || code === 'EEXIST') return throw error } +const errorCode = (error: unknown): string | undefined => + typeof error === 'object' && error !== null && 'code' in error ? String(error.code) : undefined + +const escapeRegExp = (value: string): string => value.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&') + const runGit: AgentWorktreeGitRunner = async (cwd, args) => { const { stdout } = await execFileAsync('git', ['-C', cwd, ...args], { encoding: 'utf8', diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index a4a1223..620fdf1 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -27,7 +27,7 @@ import { type WorkflowRunnerInput, } from '../index' import { changeEventPath } from './factory' -import type { AgentWorktree, AgentWorktreeManager, ChangeEvent, EventPage, GithubConnectionWrite, GithubIssueStatus, GithubPublishPullRequestInput, GithubWriteback, LinearWriteback, ProviderSyncStatus, SlackWriteback, SpawnInput, SpawnResult } from '../ports' +import type { AgentWorktree, AgentWorktreeCleanupInspection, AgentWorktreeManager, AgentWorktreeRepository, ChangeEvent, EventPage, GithubConnectionWrite, GithubIssueStatus, GithubPublishPullRequestInput, GithubWriteback, LinearWriteback, ProviderSyncStatus, SlackWriteback, SpawnInput, SpawnResult } from '../ports' import { FakeFleetClient, FakeMountClient } from '../testing' import type { CloseProbePrInput, GithubMergeGatePort, GithubMergeGateVerdict, GithubMergeInput, LinearIssue } from '../index' import { BatchTracker, issueKey } from './batch-tracker' @@ -1523,6 +1523,8 @@ class CloudWritebackFakeMountClient extends FakeMountClient { class RecordingWorktreeManager implements AgentWorktreeManager { readonly prepared: AgentWorktree[] = [] readonly cleaned: AgentWorktree[] = [] + readonly listed: AgentWorktree[] = [] + readonly inspections = new Map() cleanupAttempts = 0 failCleanups = 0 onCleanup?: () => void @@ -1540,6 +1542,17 @@ class RecordingWorktreeManager implements AgentWorktreeManager { } this.cleaned.push(structuredClone(worktree)) } + + async listWorktrees(repository: AgentWorktreeRepository): Promise { + return this.listed + .filter((worktree) => + worktree.repo === repository.repo && worktree.baseClonePath === repository.baseClonePath) + .map((worktree) => structuredClone(worktree)) + } + + async inspectForCleanup(worktree: AgentWorktree): Promise { + return structuredClone(this.inspections.get(worktree.worktreePath) ?? { bytes: 0, retentionReasons: [] }) + } } class HumanReplyDuringQuestionMountClient extends CloudWritebackFakeMountClient { @@ -16445,6 +16458,241 @@ describe('FactoryLoop PR babysitter', () => { expect(factory.status().inFlight).toEqual([]) }) + it('removes every clean worktree run for an issue when completion is fenced', async () => { + const issue = realIssueFile(412, ready, { title: 'Real multi-run worktree cleanup' }) + const mount = new FakeMountClient({ [issuePath(412)]: issue }) + seedPrMeta(mount, 'AgentWorkforce/pear', 412, { state: 'open', draft: false }) + const fleet = new FakeFleetClient() + const worktrees = new RecordingWorktreeManager() + const factory = createFactory(babysitterConfig(), { + mount, + fleet, + triage: new StaticTriage(), + worktrees, + probePrResolver: async () => ({ repo: 'AgentWorkforce/pear', prNumber: 412 }), + }) + + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(412), issue))) + const current = worktrees.prepared[0] + expect(current).toBeDefined() + const priorRun: AgentWorktree = { + ...current!, + worktreePath: '/work/.factory-worktrees/pear/ar-412-pear-11111111', + branch: 'factory/ar-412-pear-11111111', + } + worktrees.listed.push(priorRun) + fleet.emitAgentExit('ar-412-impl-pear', 'worker_exited') + await vi.waitFor(() => expect(fleet.spawns.map((spawn) => spawn.name)).toContain('ar-412-babysit')) + fleet.emitAgentMessage({ from: 'ar-412-babysit', target: 'factory', body: '[factory-pr-ready] AR-412' }) + + await vi.waitFor(() => expect(factory.status().inFlight).toEqual([])) + expect(new Set(worktrees.cleaned.map((worktree) => worktree.worktreePath))).toEqual(new Set([ + current!.worktreePath, + priorRun.worktreePath, + ])) + expect(factory.status().counters.agentWorktreesCleaned).toBe(2) + }) + + it('retains and logs a dirty extra run while removing clean completion worktrees', async () => { + const issue = realIssueFile(413, ready, { title: 'Real dirty multi-run worktree cleanup' }) + const mount = new FakeMountClient({ [issuePath(413)]: issue }) + seedPrMeta(mount, 'AgentWorkforce/pear', 413, { state: 'open', draft: false }) + const fleet = new FakeFleetClient() + const worktrees = new RecordingWorktreeManager() + const logger = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() } + const factory = createFactory(babysitterConfig(), { + mount, + fleet, + logger, + triage: new StaticTriage(), + worktrees, + probePrResolver: async () => ({ repo: 'AgentWorkforce/pear', prNumber: 413 }), + }) + + await factory.dispatch(await factory.triageIssue(parseLinearIssue(issuePath(413), issue))) + const current = worktrees.prepared[0] + expect(current).toBeDefined() + const dirtyRun: AgentWorktree = { + ...current!, + worktreePath: '/work/.factory-worktrees/pear/ar-413-pear-22222222', + branch: 'factory/ar-413-pear-22222222', + } + worktrees.listed.push(dirtyRun) + worktrees.inspections.set(dirtyRun.worktreePath, { bytes: 512, retentionReasons: ['uncommitted changes'] }) + fleet.emitAgentExit('ar-413-impl-pear', 'worker_exited') + await vi.waitFor(() => expect(fleet.spawns.map((spawn) => spawn.name)).toContain('ar-413-babysit')) + fleet.emitAgentMessage({ from: 'ar-413-babysit', target: 'factory', body: '[factory-pr-ready] AR-413' }) + + await vi.waitFor(() => expect(factory.status().inFlight).toEqual([])) + expect(worktrees.cleaned.map((worktree) => worktree.worktreePath)).toEqual([current!.worktreePath]) + expect(factory.status().counters.agentWorktreeCleanupRetained).toBe(1) + expect(logger.warn).toHaveBeenCalledWith( + '[factory] retained completed issue worktree with local state', + expect.objectContaining({ worktreePath: dirtyRun.worktreePath, retentionReasons: ['uncommitted changes'] }), + ) + }) + + it('reaps clean startup orphans and retains dirty ones with a reclaimed-size summary', async () => { + const mount = new FakeMountClient() + const fleet = new FakeFleetClient() + const worktrees = new RecordingWorktreeManager() + const clean: AgentWorktree = { + repo: 'AgentWorkforce/pear', + issueKey: 'ar-900', + baseClonePath: '/work/pear', + worktreePath: '/work/.factory-worktrees/pear/ar-900-pear-11111111', + branch: 'factory/ar-900-pear-11111111', + } + const dirty: AgentWorktree = { + ...clean, + issueKey: 'ar-901', + worktreePath: '/work/.factory-worktrees/pear/ar-901-pear-22222222', + branch: 'factory/ar-901-pear-22222222', + } + worktrees.listed.push(clean, dirty) + worktrees.inspections.set(clean.worktreePath, { bytes: 4096, retentionReasons: [] }) + worktrees.inspections.set(dirty.worktreePath, { bytes: 1024, retentionReasons: ['uncommitted changes'] }) + const logger = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() } + const factory = createFactory(config(), { mount, fleet, logger, worktrees, triage: new StaticTriage() }) + + await factory.start({ mode: 'backfill-and-subscribe' }) + + expect(worktrees.cleaned.map((worktree) => worktree.worktreePath)).toEqual([clean.worktreePath]) + expect(factory.status().counters.agentWorktreesReapedOnStartup).toBe(1) + expect(factory.status().counters.agentWorktreesCleaned).toBe(1) + expect(logger.warn).toHaveBeenCalledWith( + '[factory] retained startup orphan worktree with local state', + expect.objectContaining({ worktreePath: dirty.worktreePath, retentionReasons: ['uncommitted changes'] }), + ) + expect(logger.info).toHaveBeenCalledWith( + '[factory] startup worktree reaper completed', + { reaped: 1, reclaimedBytes: 4096, reclaimed: '4.00 KiB', retained: 1, failures: 0 }, + ) + await factory.stop() + }) + + it('retains a clean worktree for a durable issue waiting for human input on startup', async () => { + const issue = parseLinearIssue(issuePath(902), issueFile(902)) + const decision = await new StaticTriage().triage(issue) + const stateStore = new InMemoryStateStore({ batchSize: 2 }) + await stateStore.claimDispatchLifecycle( + 'factory-test', + issueKey(decision.issue), + { + runId: 'durable-waiting-run', + issue: decision.issue, + decision, + dryRun: false, + phase: 'waiting-for-human', + agents: [], + invocationIds: [], + updatedAtMs: 0, + }, + 'previous-owner', + 0, + 60_000, + ) + const worktrees = new RecordingWorktreeManager() + const active: AgentWorktree = { + repo: 'AgentWorkforce/pear', + issueKey: 'ar-902', + baseClonePath: '/work/pear', + worktreePath: '/work/.factory-worktrees/pear/ar-902-pear-11111111', + branch: 'factory/ar-902-pear-11111111', + } + worktrees.listed.push(active) + const factory = createFactory(config(), { + mount: new FakeMountClient(), + fleet: new FakeFleetClient(), + stateStore, + worktrees, + triage: new StaticTriage(), + }) + + await factory.start({ mode: 'backfill-and-subscribe' }) + + expect(worktrees.cleaned).toEqual([]) + expect(factory.status().counters.agentWorktreesReapedOnStartup).toBeUndefined() + await factory.stop() + }) + + it('reaps a clean startup orphan referenced only by a stale legacy registry', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-stale-legacy-worktree-')) + const registryPath = join(root, 'registry.json') + const heartbeatPath = join(root, 'heartbeat.json') + const issue = { uuid: 'uuid-903', key: 'AR-903', path: issuePath(903) } + await writeFile(registryPath, JSON.stringify({ + pid: 99_903, + updatedAt: new Date(0).toISOString(), + updatedAtMs: 0, + agents: [{ name: 'ar-903-impl-pear', role: 'implementer', issue, pids: [] }], + })) + const worktrees = new RecordingWorktreeManager() + const orphan: AgentWorktree = { + repo: 'AgentWorkforce/pear', + issueKey: issue.key, + baseClonePath: '/work/pear', + worktreePath: '/work/.factory-worktrees/pear/ar-903-pear-11111111', + branch: 'factory/ar-903-pear-11111111', + } + worktrees.listed.push(orphan) + const factory = createFactory(config({ loop: { registryPath, heartbeatPath } }), { + mount: new FakeMountClient(), + fleet: new FakeFleetClient(), + worktrees, + triage: new StaticTriage(), + }) + try { + await factory.start({ mode: 'live', liveSubscription: { transport: 'subscribe' } }) + + expect(worktrees.cleaned).toEqual([orphan]) + expect(factory.status().counters.agentWorktreesReapedOnStartup).toBe(1) + } finally { + await factory.stop() + await rm(root, { recursive: true, force: true }) + } + }) + + it('retains a clean startup worktree for an online legacy-registry agent', async () => { + const root = await mkdtemp(join(tmpdir(), 'factory-online-legacy-worktree-')) + const registryPath = join(root, 'registry.json') + const heartbeatPath = join(root, 'heartbeat.json') + const issue = { uuid: 'uuid-904', key: 'AR-904', path: issuePath(904) } + const agentName = 'ar-904-impl-pear' + await writeFile(registryPath, JSON.stringify({ + pid: 99_904, + updatedAt: new Date().toISOString(), + updatedAtMs: Date.now(), + agents: [{ name: agentName, role: 'implementer', issue, pids: [] }], + })) + const fleet = new FakeFleetClient() + fleet.hydrateTracked([{ name: agentName }]) + const worktrees = new RecordingWorktreeManager() + const active: AgentWorktree = { + repo: 'AgentWorkforce/pear', + issueKey: issue.key, + baseClonePath: '/work/pear', + worktreePath: '/work/.factory-worktrees/pear/ar-904-pear-11111111', + branch: 'factory/ar-904-pear-11111111', + } + worktrees.listed.push(active) + const factory = createFactory(config({ loop: { registryPath, heartbeatPath } }), { + mount: new FakeMountClient(), + fleet, + worktrees, + triage: new StaticTriage(), + }) + try { + await factory.start({ mode: 'live', liveSubscription: { transport: 'subscribe' } }) + + expect(worktrees.cleaned).toEqual([]) + expect(factory.status().counters.agentWorktreesReapedOnStartup).toBeUndefined() + } finally { + await factory.stop() + await rm(root, { recursive: true, force: true }) + } + }) + it('releases failed-dispatch agents before cleaning their isolated worktree', async () => { const issue = realIssueFile(408, ready, { title: '[factory-e2e] Failed dispatch worktree cleanup' }) const mount = new FakeMountClient({ [issuePath(408)]: issue }) diff --git a/src/orchestrator/factory.ts b/src/orchestrator/factory.ts index 4276a32..3d75493 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -40,8 +40,8 @@ import type { WaitingClarification, } from '../ports/state' import type { Clock, Logger } from '../ports/system' -import type { AgentWorktree, AgentWorktreeManager } from '../ports/worktree' -import { factoryWorktreePath } from '../git/agent-worktree' +import type { AgentWorktree, AgentWorktreeManager, AgentWorktreeRepository } from '../ports/worktree' +import { factoryWorktreeIssueSlug, factoryWorktreePath } from '../git/agent-worktree' import { InMemoryStateStore } from '../state/in-memory-state-store' import { containsExplicitIssueReference, containsIssueKey } from '../issue-key-match' import { normalizeLogger, normalizeLogValue, setSafeErrorStack, stringifyLogValue } from '../logging' @@ -726,6 +726,7 @@ export class FactoryLoop implements Factory { this.#wireFleetEvents() await this.#adoptInFlightAgents(legacyRegistry) this.#startupAgentAdoptionActive = false + if (opts.mode !== 'dispatch-owner') await this.#reapOrphanedWorktreesOnStartup(legacyRegistry) if (this.#config.babysitter.enabled) { // Re-run the idempotent receipt fold after adoption returns. This // catches records restored by lifecycle work that completed while the @@ -5710,9 +5711,41 @@ export class FactoryLoop implements Factory { const worktree = this.#agentWorktree(record, tracked.spec) if (worktree) unique.set(worktree.worktreePath, worktree) } + const issueSlug = factoryWorktreeIssueSlug(record.issue.key) const failures: string[] = [] + for (const repository of this.#worktreeRepositories(record)) { + try { + const candidates = await this.#worktrees.listWorktrees(repository) + for (const candidate of candidates) { + if (factoryWorktreeIssueSlug(candidate.issueKey) === issueSlug) { + unique.set(candidate.worktreePath, candidate) + } + } + } catch (error) { + const message = `${repository.baseClonePath}: ${describeError(error).errorMessage}` + failures.push(message) + this.#increment('agentWorktreeCleanupFailures') + this.#logger.warn?.('[factory] failed to enumerate completed issue worktrees', { + issue: record.issue.key, + repo: repository.repo, + baseClonePath: repository.baseClonePath, + error: describeError(error).errorMessage, + }) + } + } for (const worktree of unique.values()) { try { + const inspection = await this.#worktrees.inspectForCleanup(worktree) + if (inspection.retentionReasons.length > 0) { + this.#increment('agentWorktreeCleanupRetained') + this.#logger.warn?.('[factory] retained completed issue worktree with local state', { + issue: record.issue.key, + repo: worktree.repo, + worktreePath: worktree.worktreePath, + retentionReasons: inspection.retentionReasons, + }) + continue + } await this.#worktrees.cleanup(worktree) this.#increment('agentWorktreesCleaned') } catch (error) { @@ -5731,6 +5764,132 @@ export class FactoryLoop implements Factory { } } + #worktreeRepositories(record?: InFlightIssue): AgentWorktreeRepository[] { + const repositories = new Map() + const add = (repo: string, baseClonePath: string | undefined): void => { + if (!baseClonePath) return + const key = `${repo}\u0000${resolve(baseClonePath)}` + repositories.set(key, { repo, baseClonePath }) + } + + if (record) { + for (const route of record.decision.routes) { + add(route.repo, this.#config.repos.clonePaths[route.repo] ?? route.clonePath) + } + for (const tracked of record.agents.values()) { + add(tracked.spec.repo, tracked.spec.baseClonePath) + } + for (const spec of record.decision.implementers) { + add(spec.repo, spec.baseClonePath) + } + } else { + for (const [repo, baseClonePath] of Object.entries(this.#config.repos.clonePaths)) { + add(repo, baseClonePath) + } + } + return [...repositories.values()] + } + + async #reapOrphanedWorktreesOnStartup(legacyRegistry?: FactoryInFlightRegistry): Promise { + if (!this.#worktrees) return + const legacyAgents = (legacyRegistry?.agents ?? []).filter((agent) => agent.issue) + let durableLifecycles: Array<[string, DispatchLifecycle]> + let waitingClarifications: Array<[string, WaitingClarification]> + let onlineAgentNames: Set + try { + const [lifecycles, clarifications, roster] = await Promise.all([ + this.#state.listDispatchLifecycles(this.#workspaceId), + this.#state.listWaitingClarifications(this.#workspaceId), + legacyAgents.length > 0 ? this.#fleet.roster() : undefined, + ]) + durableLifecycles = lifecycles + waitingClarifications = clarifications + onlineAgentNames = new Set((roster?.agents ?? []).map((agent) => agent.name)) + } catch (error) { + this.#increment('agentWorktreeCleanupFailures') + this.#logger.warn?.('[factory] startup worktree reaper skipped because active lifecycle or roster state could not be loaded', { + error: describeError(error).errorMessage, + }) + return + } + const activeIssueSlugs = new Set((await this.#batch()).inFlight.map((record) => + factoryWorktreeIssueSlug(record.issue.key))) + for (const [, lifecycle] of durableLifecycles) { + if (!isTerminalDispatchLifecycle(lifecycle)) { + activeIssueSlugs.add(factoryWorktreeIssueSlug(lifecycle.issue.key)) + } + } + for (const [, waiting] of waitingClarifications) { + activeIssueSlugs.add(factoryWorktreeIssueSlug(waiting.issue.key)) + } + for (const agent of legacyAgents) { + if (agent.issue && onlineAgentNames.has(agent.name)) { + activeIssueSlugs.add(factoryWorktreeIssueSlug(agent.issue.key)) + } + } + const candidates = new Map() + let reaped = 0 + let reclaimedBytes = 0 + let retained = 0 + let failures = 0 + + for (const repository of this.#worktreeRepositories()) { + try { + for (const candidate of await this.#worktrees.listWorktrees(repository)) { + candidates.set(candidate.worktreePath, candidate) + } + } catch (error) { + failures += 1 + this.#increment('agentWorktreeCleanupFailures') + this.#logger.warn?.('[factory] startup worktree reaper failed to enumerate repository', { + repo: repository.repo, + baseClonePath: repository.baseClonePath, + error: describeError(error).errorMessage, + }) + } + } + + for (const worktree of candidates.values()) { + if (activeIssueSlugs.has(factoryWorktreeIssueSlug(worktree.issueKey))) continue + try { + const inspection = await this.#worktrees.inspectForCleanup(worktree) + if (inspection.retentionReasons.length > 0) { + retained += 1 + this.#increment('agentWorktreeCleanupRetained') + this.#logger.warn?.('[factory] retained startup orphan worktree with local state', { + issue: worktree.issueKey, + repo: worktree.repo, + worktreePath: worktree.worktreePath, + retentionReasons: inspection.retentionReasons, + }) + continue + } + await this.#worktrees.cleanup(worktree) + reaped += 1 + reclaimedBytes += inspection.bytes + this.#increment('agentWorktreesCleaned') + this.#increment('agentWorktreesReapedOnStartup') + } catch (error) { + failures += 1 + this.#increment('agentWorktreeCleanupFailures') + this.#logger.warn?.('[factory] startup worktree reaper retained checkout after cleanup failure', { + issue: worktree.issueKey, + repo: worktree.repo, + worktreePath: worktree.worktreePath, + error: describeError(error).errorMessage, + }) + } + } + + this.#logger.info?.('[factory] startup worktree reaper completed', { + reaped, + reclaimedBytes, + reclaimed: formatByteCount(reclaimedBytes), + retained, + failures, + }) + } + async #confirmPublishedRemotePullRequest( repo: string, result: GithubPublishPullRequestResult, @@ -14167,6 +14326,18 @@ const clarificationStaleSlackText = ( const escapeRegExp = (value: string): string => value.replace(/[.*+?^${}()|[\]\\]/gu, '\\$&') +const formatByteCount = (bytes: number): string => { + if (bytes < 1024) return `${bytes} B` + const units = ['KiB', 'MiB', 'GiB', 'TiB'] + let value = bytes + let unit = 'B' + for (const candidate of units) { + value /= 1024 + unit = candidate + if (value < 1024) break + } + return `${value.toFixed(value >= 10 ? 1 : 2)} ${unit}` +} const unrefDelay = (ms: number): Promise => new Promise((resolve) => { const timer = setTimeout(resolve, ms) timer.unref?.() diff --git a/src/ports/index.ts b/src/ports/index.ts index 834eb26..a460b2b 100644 --- a/src/ports/index.ts +++ b/src/ports/index.ts @@ -45,5 +45,7 @@ export type { } from './observability' export type { AgentWorktree, + AgentWorktreeCleanupInspection, AgentWorktreeManager, + AgentWorktreeRepository, } from './worktree' diff --git a/src/ports/worktree.ts b/src/ports/worktree.ts index 07e75a8..aae86da 100644 --- a/src/ports/worktree.ts +++ b/src/ports/worktree.ts @@ -8,7 +8,23 @@ export interface AgentWorktree { existingPullRequestBranch?: boolean } +export interface AgentWorktreeRepository { + repo: string + baseClonePath: string +} + +export interface AgentWorktreeCleanupInspection { + /** Approximate on-disk bytes that would be reclaimed by removing the checkout. */ + bytes: number + /** Empty only when the checkout is safe for an automated sweep to remove. */ + retentionReasons: string[] +} + export interface AgentWorktreeManager { prepare(worktree: AgentWorktree): Promise cleanup(worktree: AgentWorktree): Promise + /** Enumerate Factory-owned linked checkouts for one configured repository. */ + listWorktrees(repository: AgentWorktreeRepository): Promise + /** Fail closed before cleanup when local work, unpublished commits, or locks exist. */ + inspectForCleanup(worktree: AgentWorktree): Promise }