From 85c2d47cb4b6200807c7bd98f4f4e7e5117fdb22 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Mon, 20 Jul 2026 22:19:06 +0200 Subject: [PATCH 1/5] Reap leaked issue worktrees safely --- src/git/agent-worktree.test.ts | 74 ++++++++++++++- src/git/agent-worktree.ts | 150 ++++++++++++++++++++++++++++--- src/orchestrator/factory.test.ts | 128 +++++++++++++++++++++++++- src/orchestrator/factory.ts | 143 ++++++++++++++++++++++++++++- src/ports/index.ts | 2 + src/ports/worktree.ts | 16 ++++ 6 files changed, 498 insertions(+), 15 deletions(-) diff --git a/src/git/agent-worktree.test.ts b/src/git/agent-worktree.test.ts index 1c1699b..9b99f43 100644 --- a/src/git/agent-worktree.test.ts +++ b/src/git/agent-worktree.test.ts @@ -52,13 +52,83 @@ 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('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 () => { diff --git a/src/git/agent-worktree.ts b/src/git/agent-worktree.ts index c406c45..7d71e8c 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) @@ -124,6 +129,88 @@ 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) + 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 root = await realpath((await this.#git(worktree.worktreePath, ['rev-parse', '--show-toplevel'])).trim()) @@ -194,22 +281,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,9 +317,43 @@ 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 removeEmptyWorktreeParents = async (worktreePath: string): Promise => { @@ -239,11 +364,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 4b8496d..d44fa61 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' @@ -1365,6 +1365,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 @@ -1382,6 +1384,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 { @@ -14807,6 +14820,119 @@ 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('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 8813f46..0fe179d 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -37,8 +37,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' @@ -664,6 +664,7 @@ export class FactoryLoop implements Factory { this.#wireFleetEvents() await this.#adoptInFlightAgents(legacyRegistry) this.#startupAgentAdoptionActive = false + if (opts.mode !== 'dispatch-owner') await this.#reapOrphanedWorktreesOnStartup() await this.#restoreBabysitterOwnership() } catch (error) { this.#startupAgentAdoptionActive = false @@ -4890,9 +4891,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) { @@ -4911,6 +4944,99 @@ 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(): Promise { + if (!this.#worktrees) return + const activeIssueSlugs = new Set((await this.#batch()).inFlight.map((record) => + factoryWorktreeIssueSlug(record.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, @@ -12512,6 +12638,19 @@ 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 isOwnSlackBotReply = (payload: Record, botUserId: string): boolean => payload.user_is_bot === true || stringValue(payload.user) === botUserId 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 } From 537c75c65946f878236c2f59b723f06cd6b87106 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 21 Jul 2026 05:06:48 +0200 Subject: [PATCH 2/5] Prune stale registrations before cleanup inspection --- src/git/agent-worktree.test.ts | 3 +++ src/git/agent-worktree.ts | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/src/git/agent-worktree.test.ts b/src/git/agent-worktree.test.ts index 1d3c013..b68bfe8 100644 --- a/src/git/agent-worktree.test.ts +++ b/src/git/agent-worktree.test.ts @@ -278,6 +278,9 @@ describe('GitAgentWorktreeManager', () => { 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 a9f4fe0..eaec0a8 100644 --- a/src/git/agent-worktree.ts +++ b/src/git/agent-worktree.ts @@ -178,6 +178,11 @@ export class GitAgentWorktreeManager implements AgentWorktreeManager { async inspectForCleanup(worktree: AgentWorktree): Promise { assertSafeWorktree(worktree) + // 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) From 5d741335dcb2cf1c11bc4a675ef151e2d911ad8f Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 21 Jul 2026 05:52:26 +0200 Subject: [PATCH 3/5] Harden worktree cleanup safety --- src/git/agent-worktree.test.ts | 71 +++++++++++++++++++++++++++++++++- src/git/agent-worktree.ts | 23 ++++++++++- 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/src/git/agent-worktree.test.ts b/src/git/agent-worktree.test.ts index b68bfe8..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, symlink, writeFile } from 'node:fs/promises' -import { dirname, join } from 'node:path' +import { basename, dirname, join } from 'node:path' import { tmpdir } from 'node:os' import { promisify } from 'node:util' import { describe, expect, it } from 'vitest' @@ -96,6 +96,72 @@ describe('GitAgentWorktreeManager', () => { } }) + 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') @@ -250,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 { @@ -275,6 +341,7 @@ 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() diff --git a/src/git/agent-worktree.ts b/src/git/agent-worktree.ts index eaec0a8..a6c43f0 100644 --- a/src/git/agent-worktree.ts +++ b/src/git/agent-worktree.ts @@ -118,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) @@ -178,6 +179,7 @@ export class GitAgentWorktreeManager implements AgentWorktreeManager { 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 @@ -218,7 +220,9 @@ export class GitAgentWorktreeManager implements AgentWorktreeManager { async #assertRegisteredCheckout(worktree: AgentWorktree): Promise { const wanted = await realpath(worktree.worktreePath) - const expectedRoot = await realpath(factoryWorktreeRoot(worktree.baseClonePath)) + 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}`) } @@ -230,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}`) } @@ -365,6 +371,19 @@ const directorySize = async (root: string): Promise => { 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 + } + } +} + const removeEmptyWorktreeParents = async (worktreePath: string): Promise => { const repoRoot = dirname(worktreePath) const factoryRoot = dirname(repoRoot) From da4d96fc40429796f66ddeb053a8bee94c2e6dca Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 21 Jul 2026 07:10:49 +0200 Subject: [PATCH 4/5] Reap worktrees from stale legacy registries --- src/orchestrator/factory.test.ts | 77 ++++++++++++++++++++++++++++++++ src/orchestrator/factory.ts | 16 +++++-- 2 files changed, 89 insertions(+), 4 deletions(-) diff --git a/src/orchestrator/factory.test.ts b/src/orchestrator/factory.test.ts index 753d36b..071b316 100644 --- a/src/orchestrator/factory.test.ts +++ b/src/orchestrator/factory.test.ts @@ -16302,6 +16302,83 @@ describe('FactoryLoop PR babysitter', () => { 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 3ae39f0..5a03771 100644 --- a/src/orchestrator/factory.ts +++ b/src/orchestrator/factory.ts @@ -5730,16 +5730,22 @@ export class FactoryLoop implements Factory { 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 { - [durableLifecycles, waitingClarifications] = await Promise.all([ + 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 state could not be loaded', { + this.#logger.warn?.('[factory] startup worktree reaper skipped because active lifecycle or roster state could not be loaded', { error: describeError(error).errorMessage, }) return @@ -5754,8 +5760,10 @@ export class FactoryLoop implements Factory { for (const [, waiting] of waitingClarifications) { activeIssueSlugs.add(factoryWorktreeIssueSlug(waiting.issue.key)) } - for (const agent of legacyRegistry?.agents ?? []) { - if (agent.issue) activeIssueSlugs.add(factoryWorktreeIssueSlug(agent.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 From 4324ec1761ffa5c05662b408d431cb3fe3ee5b0b Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 21 Jul 2026 07:22:32 +0200 Subject: [PATCH 5/5] test: isolate broker PID protection from relay env --- src/fleet/internal-fleet-client.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/fleet/internal-fleet-client.test.ts b/src/fleet/internal-fleet-client.test.ts index 2e4a026..e01909c 100644 --- a/src/fleet/internal-fleet-client.test.ts +++ b/src/fleet/internal-fleet-client.test.ts @@ -704,7 +704,11 @@ describe('InternalFleetClient', () => { it('surfaces the broker pid as protected process state', async () => { const harness = new FakeHarnessDriverClient() harness.brokerPid = 68009 - const fleet = new InternalFleetClient({ client: harness, cwd: '/worktree' }) + const fleet = new InternalFleetClient({ + client: harness, + cwd: '/worktree', + connectionPath: '/nonexistent/factory-test-relay-connection.json', + }) await expect(fleet.protectedPids()).resolves.toEqual([68009]) })