diff --git a/.changeset/preserve-dirty-worktrees-minimal.md b/.changeset/preserve-dirty-worktrees-minimal.md new file mode 100644 index 0000000000..550682dcf3 --- /dev/null +++ b/.changeset/preserve-dirty-worktrees-minimal.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Preserve dirty or unverifiable worktrees during automatic cleanup. +category: fix +dev: Automatic cleanup now fails closed for unverified content and revalidates cleanliness without force at removal time. diff --git a/packages/engine/src/__tests__/reliability-interactions/worktree-defensive-removal-preservation.real-git.test.ts b/packages/engine/src/__tests__/reliability-interactions/worktree-defensive-removal-preservation.real-git.test.ts new file mode 100644 index 0000000000..0228e3a958 --- /dev/null +++ b/packages/engine/src/__tests__/reliability-interactions/worktree-defensive-removal-preservation.real-git.test.ts @@ -0,0 +1,153 @@ +import { access, constants as fsConstants, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { + RemovalReason, + removeWorktree, +} from "../../worktree/worktree-backend.js"; +import { reapOrphanWorktrees } from "../../worktree/worktree-pool.js"; +import { git, hasGit } from "./_helpers.js"; + +async function pathExists(path: string): Promise { + try { + await access(path, fsConstants.F_OK); + return true; + } catch { + return false; + } +} + +describe.skipIf(!hasGit)("reliability interactions: defensive removal preserves unverifiable content", () => { + const roots: string[] = []; + + afterEach(async () => { + await Promise.all(roots.map((root) => rm(root, { recursive: true, force: true }))); + roots.length = 0; + }); + + async function setupRepo(): Promise { + const root = await mkdtemp(join(tmpdir(), "fusion-defensive-remove-")); + roots.push(root); + git(root, "git init -b main"); + git(root, 'git config user.email "test@example.com"'); + git(root, 'git config user.name "Test User"'); + await writeFile(join(root, "README.md"), "# repo\n", "utf-8"); + await writeFile(join(root, ".gitignore"), "dist/\n", "utf-8"); + git(root, "git add README.md .gitignore"); + git(root, 'git commit -m "init"'); + await mkdir(join(root, ".worktrees"), { recursive: true }); + return root; + } + + async function createWorktree(root: string, name: string): Promise { + const worktreePath = join(root, ".worktrees", name); + git(root, `git worktree add -b ${JSON.stringify(`fusion/${name}`)} ${JSON.stringify(worktreePath)}`); + return worktreePath; + } + + it("pool-prune refuses and preserves a dirty registered worktree", async () => { + const root = await setupRepo(); + const worktreePath = await createWorktree(root, "dirty-prune"); + await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8"); + + await expect( + removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune }), + ).rejects.toThrow(/preserving/); + + expect(await readFile(join(worktreePath, "wip.txt"), "utf-8")).toBe("uncommitted\n"); + }); + + it("idle-sweep refuses and preserves a dirty registered worktree", async () => { + const root = await setupRepo(); + const worktreePath = await createWorktree(root, "dirty-idle"); + await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8"); + + await expect( + removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.SelfHealingIdleSweep }), + ).rejects.toThrow(/preserving/); + + expect(await pathExists(worktreePath)).toBe(true); + }); + + it("pool-prune preserves user content under an ignored generated-looking path", async () => { + const root = await setupRepo(); + const worktreePath = await createWorktree(root, "ignored-prune"); + await mkdir(join(worktreePath, "dist"), { recursive: true }); + await writeFile(join(worktreePath, "dist", "manual.txt"), "precious\n", "utf-8"); + + await expect( + removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune }), + ).rejects.toThrow(/preserving/); + + expect(await readFile(join(worktreePath, "dist", "manual.txt"), "utf-8")).toBe("precious\n"); + }); + + it.each([ + RemovalReason.MergerCleanup, + RemovalReason.MergerPostMerge, + RemovalReason.SelfHealingBranchConflict, + RemovalReason.SelfHealingReclaim, + RemovalReason.SelfHealingStaleActiveBranch, + RemovalReason.StepSessionCleanup, + ])("%s preserves a dirty automatically managed worktree", async (reason) => { + const root = await setupRepo(); + const worktreePath = await createWorktree(root, reason); + await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8"); + + await expect(removeWorktree({ rootDir: root, worktreePath, settings: {}, reason })).rejects.toThrow(/preserving/); + + expect(await readFile(join(worktreePath, "wip.txt"), "utf-8")).toBe("uncommitted\n"); + }); + + it("a failing status probe preserves the checkout instead of enabling deletion", async () => { + const root = await setupRepo(); + const worktreePath = await createWorktree(root, "corrupt-probe"); + // Corrupt the registration so the cleanliness probe cannot run at all. + await rm(join(root, ".git", "worktrees", "corrupt-probe"), { recursive: true, force: true }); + + await expect( + removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune }), + ).rejects.toThrow(/preserving/); + + expect(await pathExists(join(worktreePath, "README.md"))).toBe(true); + }); + + it("clean registered worktrees are still removed by pool-prune", async () => { + const root = await setupRepo(); + const worktreePath = await createWorktree(root, "clean-prune"); + + await removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.PoolPrune }); + + expect(await pathExists(worktreePath)).toBe(false); + }); + + it("addressed teardown reasons keep their legacy forced semantics on dirty worktrees", async () => { + const root = await setupRepo(); + const worktreePath = await createWorktree(root, "task-reset-dirty"); + await writeFile(join(worktreePath, "wip.txt"), "uncommitted\n", "utf-8"); + + await removeWorktree({ rootDir: root, worktreePath, settings: {}, reason: RemovalReason.TaskReset }); + + expect(await pathExists(worktreePath)).toBe(false); + }); + + it("startup reaper preserves dangling or content orphans", async () => { + const root = await setupRepo(); + + const danglingOrphan = join(root, ".worktrees", "dangling-orphan"); + await mkdir(danglingOrphan, { recursive: true }); + await writeFile(join(danglingOrphan, ".git"), "gitdir: /nonexistent/admin\n", "utf-8"); + + const contentOrphan = join(root, ".worktrees", "content-orphan"); + await mkdir(contentOrphan, { recursive: true }); + await writeFile(join(contentOrphan, ".git"), "gitdir: /nonexistent/admin\n", "utf-8"); + await writeFile(join(contentOrphan, "wip.txt"), "precious\n", "utf-8"); + + await reapOrphanWorktrees(root); + + expect(await pathExists(join(danglingOrphan, ".git"))).toBe(true); + expect(await readFile(join(contentOrphan, "wip.txt"), "utf-8")).toBe("precious\n"); + expect(dirname(contentOrphan)).toBe(join(root, ".worktrees")); + }); +}); diff --git a/packages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts b/packages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts index bf820feb29..a4e30b37f8 100644 --- a/packages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts @@ -5,16 +5,21 @@ import { cleanupOrphanedWorktrees } from "../../worktree/worktree-pool.js"; import { SelfHealingManager } from "../../self-healing.js"; import { NativeWorktreeBackend, WorktrunkWorktreeBackend } from "../../worktree/worktree-backend.js"; -const { execSpy, existsSpy, readdirSpy, readFileSpy } = vi.hoisted(() => ({ - execSpy: vi.fn(), - existsSpy: vi.fn(() => true), - readdirSpy: vi.fn(() => []), - readFileSpy: vi.fn(() => ""), -})); +const { execSpy, execFileSpy, existsSpy, readdirSpy, readFileSpy } = vi.hoisted(() => { + const execFileSpy = vi.fn().mockResolvedValue({ stdout: "", stderr: "" }); + (execFileSpy as any)[Symbol.for("nodejs.util.promisify.custom")] = execFileSpy; + return { + execSpy: vi.fn(), + execFileSpy, + existsSpy: vi.fn(() => true), + readdirSpy: vi.fn(() => []), + readFileSpy: vi.fn(() => ""), + }; +}); vi.mock("node:child_process", async (importOriginal) => { const actual = await importOriginal(); - return { ...actual, exec: execSpy }; + return { ...actual, exec: execSpy, execFile: execFileSpy }; }); vi.mock("node:fs", async (importOriginal) => { @@ -39,6 +44,8 @@ describe("reliability interactions: worktrunk worktree removal routing", () => { beforeEach(() => { vi.clearAllMocks(); execSpy.mockImplementation((_cmd: string, _opts: unknown, cb: (err: unknown, stdout: string, stderr: string) => void) => cb(null, "", "")); + execFileSpy.mockReset(); + execFileSpy.mockResolvedValue({ stdout: "", stderr: "" }); // A workspace-group marker is an explicit delete veto in the ownership proof; these fixtures // are ordinary single-project worktrees, so the marker must be absent. existsSpy.mockImplementation(((path: string) => !String(path).endsWith("/.fusion-workspace-root")) as never); diff --git a/packages/engine/src/__tests__/self-healing-tempdir-sweep.test.ts b/packages/engine/src/__tests__/self-healing-tempdir-sweep.test.ts index 978dbf174f..1d64523d45 100644 --- a/packages/engine/src/__tests__/self-healing-tempdir-sweep.test.ts +++ b/packages/engine/src/__tests__/self-healing-tempdir-sweep.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { existsSync, mkdirSync, mkdtempSync, realpathSync, rmSync, utimesSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { execFileSync, execSync } from "node:child_process"; const osState = vi.hoisted(() => ({ tempRoot: "" })); const fsState = vi.hoisted(() => ({ @@ -174,6 +175,19 @@ function makeReclaimableWorktree(path: string, name: string): void { writeFileSync(join(path, ".git"), `gitdir: ${join(projectRoot, ".git", "worktrees", name)}\n`); } +function makeRealIdleWorktree(root: string, name: string): string { + // Create a genuine git worktree with admin entry so the status probe succeeds. + execSync("git init -b main", { cwd: root }); + execSync('git config user.email "test@example.com"', { cwd: root }); + execSync('git config user.name "Test"', { cwd: root }); + writeFileSync(join(root, "README.md"), "# fixture\n"); + execSync("git add README.md", { cwd: root }); + execSync('git commit -m init', { cwd: root }); + const worktreeDir = join(root, ".worktrees", name); + execFileSync("git", ["worktree", "add", "-b", `fusion/${name}`, worktreeDir], { cwd: root }); + return worktreeDir; +} + async function sweep(manager: SelfHealingManager): Promise { return await (manager as any).cleanupStaleTempMergeWorktrees(); } @@ -183,7 +197,7 @@ function sweepAudits(audits: any[]) { } describe("SelfHealingManager worktrees-dir sweeps", () => { - it("excludes internal containers from unregistered-orphan reap while removing genuine orphans", async () => { + it("excludes internal containers and preserves unverifiable unregistered orphans", async () => { const worktreesDir = join(projectRoot, ".worktrees"); const aiMergeContainer = join(worktreesDir, ".ai-merge"); const recoveryContainer = join(worktreesDir, ".fusion-recovery"); @@ -193,12 +207,12 @@ describe("SelfHealingManager worktrees-dir sweeps", () => { makeReclaimableWorktree(orphan, "half-built"); const { manager } = makeManager({ recycleWorktrees: true }); - await expect((manager as any).reapUnregisteredOrphans()).resolves.toBe(1); + await expect((manager as any).reapUnregisteredOrphans()).resolves.toBe(0); expect(existsSync(aiMergeContainer)).toBe(true); expect(existsSync(recoveryContainer)).toBe(true); - expect(existsSync(orphan)).toBe(false); - expect(fsState.rmCalls).toContain(orphan); + expect(existsSync(orphan)).toBe(true); + expect(fsState.rmCalls).not.toContain(orphan); expect(fsState.rmCalls).not.toContain(aiMergeContainer); expect(fsState.rmCalls).not.toContain(recoveryContainer); }); @@ -207,10 +221,9 @@ describe("SelfHealingManager worktrees-dir sweeps", () => { const worktreesDir = join(projectRoot, ".worktrees"); const aiMergeContainer = join(worktreesDir, ".ai-merge"); const recoveryContainer = join(worktreesDir, ".fusion-recovery"); - const idle = join(worktreesDir, "idle-wt"); mkdirSync(aiMergeContainer, { recursive: true }); mkdirSync(recoveryContainer, { recursive: true }); - makeReclaimableWorktree(idle, "idle-wt"); + makeRealIdleWorktree(projectRoot, "idle-wt"); childState.execStdout = gitWorktreeList(["idle-wt"]); const { manager } = makeManager({ maxWorktrees: 0 }); diff --git a/packages/engine/src/__tests__/worktree-backend.test.ts b/packages/engine/src/__tests__/worktree-backend.test.ts index 2c7fab8ca5..2650a03b3c 100644 --- a/packages/engine/src/__tests__/worktree-backend.test.ts +++ b/packages/engine/src/__tests__/worktree-backend.test.ts @@ -12,6 +12,7 @@ import { activeSessionRegistry } from "../agents/active-session-registry.js"; const { execMock, + execFileMock, accessMock, rmMock, chmodMock, @@ -26,8 +27,11 @@ const { } = vi.hoisted(() => { const mock = vi.fn(); (mock as any)[Symbol.for("nodejs.util.promisify.custom")] = mock; + const execFileMock = vi.fn().mockResolvedValue({ stdout: "", stderr: "" }); + (execFileMock as any)[Symbol.for("nodejs.util.promisify.custom")] = execFileMock; return { execMock: mock, + execFileMock, accessMock: vi.fn(), rmMock: vi.fn(), chmodMock: vi.fn(), @@ -42,7 +46,7 @@ const { }; }); -vi.mock("node:child_process", () => ({ exec: execMock, execFile: vi.fn() })); +vi.mock("node:child_process", () => ({ exec: execMock, execFile: execFileMock })); vi.mock("node:fs", () => ({ existsSync: existsSyncMock })); vi.mock("node:fs/promises", () => ({ access: accessMock, chmod: chmodMock, rm: rmMock })); vi.mock("../execution/branch-conflicts.js", () => ({ @@ -79,6 +83,8 @@ vi.mock("../worktree/worktree-prune.js", () => ({ beforeEach(() => { execMock.mockReset(); + execFileMock.mockReset(); + execFileMock.mockResolvedValue({ stdout: "", stderr: "" }); accessMock.mockReset(); rmMock.mockReset(); rmMock.mockResolvedValue(undefined as never); @@ -278,6 +284,24 @@ describe("NativeWorktreeBackend", () => { expect(pruneWorktreeAdminEntriesMock).not.toHaveBeenCalled(); }); + it("prunes a missing defensive worktree registration without recursive fallback", async () => { + execMock.mockRejectedValueOnce({ stderr: "fatal: '/repo/.worktrees/fn-1' is not a working tree" }); + existsSyncMock.mockReturnValue(false); + + await new NativeWorktreeBackend().remove({ + rootDir: "/repo", + worktreePath: "/repo/.worktrees/fn-1", + force: false, + }); + + expect(rmMock).not.toHaveBeenCalled(); + expect(pruneWorktreeAdminEntriesMock).toHaveBeenCalledWith(expect.objectContaining({ + rootDir: "/repo", + reason: "remove-missing-fallback", + target: "/repo/.worktrees/fn-1", + })); + }); + it("retries errno-only recoverable cleanup failures before pruning once", async () => { const busy = Object.assign(new Error("busy"), { code: "EBUSY" }); execMock.mockRejectedValueOnce(busy); @@ -954,7 +978,7 @@ describe("removeWorktree", () => { }); expect(execMock).toHaveBeenCalledWith( - 'git worktree remove --force "/repo/.worktrees/fn-1"', + 'git worktree remove "/repo/.worktrees/fn-1"', expect.objectContaining({ cwd: "/repo", timeout: 60000 }), ); expect(audit.git).toHaveBeenCalledWith({ type: "worktree:remove", target: "/repo/.worktrees/fn-1" }); diff --git a/packages/engine/src/__tests__/worktree-pool-secrets-env-cleanup.test.ts b/packages/engine/src/__tests__/worktree-pool-secrets-env-cleanup.test.ts index d05ec12601..f7b0987070 100644 --- a/packages/engine/src/__tests__/worktree-pool-secrets-env-cleanup.test.ts +++ b/packages/engine/src/__tests__/worktree-pool-secrets-env-cleanup.test.ts @@ -36,8 +36,8 @@ afterEach(async () => { await Promise.all(dirs.splice(0).map((d) => rm(d, { recursive: true, force: true }))); }); -describe("worktree-pool secrets cleanup hooks", () => { - it("reapOrphanWorktrees invokes cleanup before removal", async () => { +describe("worktree-pool secrets preservation", () => { + it("preserves an unverifiable orphan instead of deleting its environment file", async () => { cleanupSecretsEnvFile.mockResolvedValue({ outcome: "cleaned", reason: "fingerprint-match" }); const root = tmpRoot(); const worktrees = join(root, ".worktrees"); @@ -48,15 +48,12 @@ describe("worktree-pool secrets cleanup hooks", () => { const mod = await import("../worktree/worktree-pool.js"); const removed = await mod.reapOrphanWorktrees(root); - expect(removed).toBe(1); - expect(cleanupSecretsEnvFile).toHaveBeenCalledWith(expect.objectContaining({ - worktreePath: orphan, - taskId: "orphan:orphan-1", - })); - expect(existsSync(orphan)).toBe(false); + expect(removed).toBe(0); + expect(cleanupSecretsEnvFile).not.toHaveBeenCalled(); + expect(existsSync(orphan)).toBe(true); }); - it("cleanup failures do not block orphan removal", async () => { + it("does not invoke secrets cleanup before preserving dangling metadata", async () => { cleanupSecretsEnvFile.mockRejectedValueOnce(new Error("cleanup failed")); const root = tmpRoot(); const orphan = join(root, ".worktrees", "orphan-2"); @@ -65,7 +62,8 @@ describe("worktree-pool secrets cleanup hooks", () => { const mod = await import("../worktree/worktree-pool.js"); const removed = await mod.reapOrphanWorktrees(root); - expect(removed).toBe(1); - expect(existsSync(orphan)).toBe(false); + expect(removed).toBe(0); + expect(cleanupSecretsEnvFile).not.toHaveBeenCalled(); + expect(existsSync(orphan)).toBe(true); }); }); diff --git a/packages/engine/src/__tests__/worktree-pool.test.ts b/packages/engine/src/__tests__/worktree-pool.test.ts index b4e467fcd5..c4974460fd 100644 --- a/packages/engine/src/__tests__/worktree-pool.test.ts +++ b/packages/engine/src/__tests__/worktree-pool.test.ts @@ -1140,7 +1140,7 @@ describe("cleanupOrphanedWorktrees", () => { expect(removeCalls).toHaveLength(0); }); - it("excludes internal containers while still removing genuine unregistered orphans", async () => { + it("excludes internal containers and preserves unregistered orphans", async () => { mockedReaddirSync.mockReturnValue([ makeDirEntry(".ai-merge"), makeDirEntry(".fusion-recovery"), @@ -1152,16 +1152,13 @@ describe("cleanupOrphanedWorktrees", () => { const cleaned = await cleanupOrphanedWorktrees("/root", store); - expect(cleaned).toBe(1); - expect(mockedRmSync).toHaveBeenCalledWith("/root/.worktrees/broken-wt", { - recursive: true, - force: true, - }); + expect(cleaned).toBe(0); + expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/broken-wt", expect.anything()); expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/.ai-merge", expect.anything()); expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/.fusion-recovery", expect.anything()); }); - it("removes unregistered directories even when stale active task metadata references them", async () => { + it("preserves unregistered directories referenced by stale active task metadata", async () => { mockedReaddirSync.mockReturnValue([ makeDirEntry("broken-wt"), ] as any); @@ -1173,14 +1170,9 @@ describe("cleanupOrphanedWorktrees", () => { const cleaned = await cleanupOrphanedWorktrees("/root", store); - expect(cleaned).toBe(1); - expect(mockedRmSync).toHaveBeenCalledWith("/root/.worktrees/broken-wt", { - recursive: true, - force: true, - }); - expect(mockedPruneWorktreeAdminEntries).toHaveBeenCalledWith( - expect.objectContaining({ reason: "pool-cleanup-orphan", target: "/root/.worktrees/broken-wt" }), - ); + expect(cleaned).toBe(0); + expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/broken-wt", expect.anything()); + expect(mockedPruneWorktreeAdminEntries).not.toHaveBeenCalled(); }); }); @@ -1207,13 +1199,8 @@ describe("reapOrphanWorktrees", () => { expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/.fusion-recovery", expect.anything()); }); - // FN-6782 follow-up: a directory whose `.git` points to a missing admin entry is leak - // residue (invisible to `git worktree list`/`prune`), not "partially registered". It - // must be reaped — otherwise it collides with freshly generated worktree names and - // breaks `execute`. Previously the reaper skipped on mere `.git` presence. - it("reaps a dir with a dangling .git pointer (admin gitdir missing)", async () => { + it("preserves a dir with a dangling .git pointer", async () => { mockedReaddirSync.mockReturnValue([makeDirEntry("leaked-wt")] as any); - // `.git` is a link FILE (not a dir); the worktree dir itself is a dir. mockedLstatSync.mockImplementation((p: any) => (String(p).endsWith("/.git") ? { isDirectory: () => false, isSymbolicLink: () => false } @@ -1222,14 +1209,13 @@ describe("reapOrphanWorktrees", () => { mockedReadFileSync.mockReturnValue("gitdir: /root/.git/worktrees/leaked-wt\n" as any); mockedExistsSync.mockImplementation((p) => { const s = String(p); - // .worktrees root exists; the .git link file exists; the gitdir target does NOT. return s === "/root/.worktrees" || s === "/root/.worktrees/leaked-wt/.git"; }); const removed = await reapOrphanWorktrees("/root"); - expect(removed).toBe(1); - expect(mockedRmSync).toHaveBeenCalledWith("/root/.worktrees/leaked-wt", { recursive: true, force: true }); + expect(removed).toBe(0); + expect(mockedRmSync).not.toHaveBeenCalledWith("/root/.worktrees/leaked-wt", expect.anything()); }); it("skips a dir with a valid .git pointer (admin gitdir exists)", async () => { diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index f0d0f15738..fcfa3edaab 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -26,7 +26,7 @@ import { execSync } from "node:child_process"; import { setImmediate as setImmediateCb } from "node:timers"; -import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmdirSync, rmSync, statSync, writeFileSync } from "node:fs"; import { readFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { basename, dirname, isAbsolute, join, relative, resolve } from "node:path"; @@ -16617,7 +16617,9 @@ const movedTask = await this.store.moveTask(task.id, completeLane); continue; } try { - rmSync(path, { recursive: true, force: true }); + // FNXC:WorktreeCleanup: rmdir is deliberately non-recursive. Any content + // makes it fail closed and preserves the unregistered checkout. + rmdirSync(path); log.log(`Cleaned unregistered worktree dir: ${path}`); cleaned++; } catch (err: unknown) { diff --git a/packages/engine/src/worktree/worktree-backend.ts b/packages/engine/src/worktree/worktree-backend.ts index 4ab6abee70..f6505a1a5a 100644 --- a/packages/engine/src/worktree/worktree-backend.ts +++ b/packages/engine/src/worktree/worktree-backend.ts @@ -1,4 +1,4 @@ -import { exec } from "node:child_process"; +import { exec, execFile } from "node:child_process"; import { existsSync } from "node:fs"; import { access, rm } from "node:fs/promises"; import { basename, resolve } from "node:path"; @@ -27,6 +27,7 @@ import { import { parseStaleRegistrationPath, recoverStaleRegistration } from "./worktree-stale-registration.js"; const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); const NATIVE_TIMEOUT_MS = 120_000; const REMOVE_TIMEOUT_MS = 60_000; const MAX_BUFFER = 10 * 1024 * 1024; @@ -216,6 +217,7 @@ export interface WorktreeRemoveInput { worktreePath: string; branch?: string; taskId?: string; + force?: boolean; } export interface WorktreeSyncInput { @@ -679,7 +681,7 @@ export class NativeWorktreeBackend implements WorktreeBackend { async remove(input: WorktreeRemoveInput): Promise { try { - await execAsync(`git worktree remove --force ${quoteShellArg(input.worktreePath)}`, { + await execAsync(`git worktree remove${input.force === false ? "" : " --force"} ${quoteShellArg(input.worktreePath)}`, { cwd: input.rootDir, encoding: "utf-8", timeout: REMOVE_TIMEOUT_MS, @@ -687,6 +689,22 @@ export class NativeWorktreeBackend implements WorktreeBackend { }); return; } catch (error) { + // Defensive callers rely on Git's deletion-boundary dirty check. Never turn + // that refusal into the recursive filesystem fallback below. + if (input.force === false) { + const missingPathError = /is not a working tree|no such file or directory|does not exist/i.test(getErrorMessageWithStderr(error)); + if (!existsSync(input.worktreePath) && missingPathError) { + await pruneWorktreeAdminEntries({ + rootDir: input.rootDir, + auditor: this.deps.audit, + reason: "remove-missing-fallback", + target: input.worktreePath, + logger: this.deps.logger, + }); + return; + } + throw error; + } if (!isRecoverableNativeWorktreeRemoveError(error)) { throw error; } @@ -951,7 +969,7 @@ export class WorktrunkWorktreeBackend implements WorktreeBackend { async remove(input: WorktreeRemoveInput): Promise { const target = input.branch ?? input.worktreePath; try { - await this.runWorktrunk(["remove", "--foreground", target], { + await this.runWorktrunk(["remove", "--foreground", ...(input.force === true ? ["--force"] : []), target], { cwd: input.rootDir, operation: "remove", }); @@ -1072,6 +1090,17 @@ const ALLOWED_FORCE_REASONS = new Set([ RemovalReason.WorkspaceAcquireRollback, ]); +const DEFENSIVE_REMOVAL_REASONS = new Set([ + RemovalReason.MergerCleanup, + RemovalReason.MergerPostMerge, + RemovalReason.PoolPrune, + RemovalReason.SelfHealingBranchConflict, + RemovalReason.SelfHealingIdleSweep, + RemovalReason.SelfHealingReclaim, + RemovalReason.SelfHealingStaleActiveBranch, + RemovalReason.StepSessionCleanup, +]); + export class InvalidForceUsageError extends Error { constructor(reason: RemovalReason) { super(`force=true is not allowed for removal reason '${reason}'`); @@ -1092,6 +1121,28 @@ export class ActiveSessionWorktreeRemovalError extends Error { } } +/** Fail closed when an automatic sweep cannot prove the checkout is empty of user content. */ +async function assertCleanForDefensiveRemoval(worktreePath: string): Promise { + // Nothing on disk means nothing to preserve — stale registrations prune normally below. + if (!existsSync(worktreePath)) { + return; + } + let stdout: string; + try { + ({ stdout } = await execFileAsync("git", ["status", "--porcelain", "--ignored", "--untracked-files=all"], { + cwd: worktreePath, + encoding: "utf-8", + timeout: 15_000, + maxBuffer: MAX_BUFFER, + })); + } catch (error) { + throw new Error(`preserving ${worktreePath}: status probe failed (${error instanceof Error ? error.message : String(error)})`); + } + if (stdout.trim().length > 0) { + throw new Error(`preserving ${worktreePath}: uncommitted or ignored content present`); + } +} + /** * FNXC:WorkspaceWorktree 2026-08-20-07:08: * Force removal is reserved for explicit executor teardown paths and workspace-acquisition rollback @@ -1120,6 +1171,15 @@ export async function removeWorktree(input: { throw new InvalidForceUsageError(input.reason); } + const requiresCleanWorktree = DEFENSIVE_REMOVAL_REASONS.has(input.reason); + + // FNXC:WorktreeCleanup: + // Defensive sweeps must prove cleanliness before destroying anything. Dirty or + // unverifiable content is preserved (fail closed) — callers treat the throw as "kept". + if (requiresCleanWorktree) { + await assertCleanForDefensiveRemoval(input.worktreePath); + } + if (input.expectedOwnerTaskId && input.liveOwnerProbe) { const reconciled = reconcileSelfOwnedActiveSessionForRemoval( activeSessionRegistry, @@ -1169,6 +1229,7 @@ export async function removeWorktree(input: { rootDir: input.rootDir, worktreePath: input.worktreePath, taskId: input.taskId, + force: requiresCleanWorktree ? false : input.force, }; if (input.force === false || typeof input.timeout === "number") { diff --git a/packages/engine/src/worktree/worktree-pool.ts b/packages/engine/src/worktree/worktree-pool.ts index 2cb48331f8..4194bd2a63 100644 --- a/packages/engine/src/worktree/worktree-pool.ts +++ b/packages/engine/src/worktree/worktree-pool.ts @@ -1,6 +1,6 @@ import { exec, execFile } from "node:child_process"; import { promisify } from "node:util"; -import { existsSync, lstatSync, readdirSync, readFileSync, rmSync, realpathSync } from "node:fs"; +import { existsSync, lstatSync, readdirSync, readFileSync, rmdirSync, realpathSync } from "node:fs"; import { mkdir } from "node:fs/promises"; import { basename, dirname, join, relative, resolve, isAbsolute } from "node:path"; import type { SecretsStore, Settings, TaskStore, WorktrunkSettings, WorkspaceWorktreeContext } from "@fusion/core"; @@ -18,7 +18,6 @@ import { removeWorktree as removeWorktreeViaBackend, resolveWorktreeBackend as resolveWorktreeBackendViaSettings, } from "./worktree-backend.js"; -import { cleanupSecretsEnvFile } from "./secrets-env-writer.js"; import { removeDesktopBuildArtifacts } from "./worktree-desktop-artifacts.js"; import { resolveIntegrationBranch } from "../merge/integration-branch.js"; import type { RunAuditor } from "../util/run-audit.js"; @@ -1026,22 +1025,6 @@ export async function cleanupOrphanedWorktrees( for (const worktreePath of candidates) { try { if (registeredWorktrees.has(resolve(worktreePath))) { - const orphanTaskId = `orphan:${basename(worktreePath)}`; - try { - await cleanupSecretsEnvFile({ - worktreePath, - taskId: orphanTaskId, - expectedFingerprint: null, - filename: ".env", - audit: undefined, - logger: worktreePoolLog, - }); - } catch (error) { - worktreePoolLog.warn( - `secrets-env cleanup failed for registered orphan ${worktreePath}: ${error instanceof Error ? error.message : String(error)}`, - ); - } - await removeWorktreeViaBackend({ rootDir, worktreePath, @@ -1052,7 +1035,9 @@ export async function cleanupOrphanedWorktrees( if (!isInsideWorktreesDir(rootDir, worktreePath, settings)) { throw new Error(`Refusing to remove path outside .worktrees: ${worktreePath}`); } - rmSync(worktreePath, { recursive: true, force: true }); + // FNXC:WorktreeCleanup: rmdir is deliberately non-recursive. Any content + // makes it fail closed and preserves the unregistered checkout. + rmdirSync(worktreePath); await pruneWorktreeAdminEntries({ rootDir, reason: "pool-cleanup-orphan", @@ -1199,36 +1184,26 @@ export async function reapOrphanWorktrees( continue; } worktreePoolLog.debug(`reapOrphanWorktrees: ${name} has a dangling .git pointer (admin entry missing) — treating as orphan`); - // fall through to removal + // fall through to the non-recursive removal below; `.git` makes it fail closed. } // This directory is on disk but has no valid .git entry and is not a registered // worktree — it is a half-initialized / leaked orphan. Remove it. try { - try { - await cleanupSecretsEnvFile({ - worktreePath: resolvedFull, - taskId: `orphan:${name}`, - expectedFingerprint: null, - filename: ".env", - logger: worktreePoolLog, - }); - } catch (error) { - worktreePoolLog.warn(`secrets-env cleanup failed for orphan ${name}: ${error instanceof Error ? error.message : String(error)}`); - } - rmSync(resolvedFull, { recursive: true, force: true }); - await pruneWorktreeAdminEntries({ - rootDir: projectRoot, - reason: "pool-reap-orphan", - target: resolvedFull, - logger: worktreePoolLog, - }).catch(() => undefined); - worktreePoolLog.log(`reapOrphanWorktrees: removed half-initialized orphan ${name}`); - removed++; + rmdirSync(resolvedFull); } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); worktreePoolLog.warn(`reapOrphanWorktrees: failed to remove ${name} — ${msg}`); + continue; } + await pruneWorktreeAdminEntries({ + rootDir: projectRoot, + reason: "pool-reap-orphan", + target: resolvedFull, + logger: worktreePoolLog, + }).catch(() => undefined); + worktreePoolLog.log(`reapOrphanWorktrees: removed half-initialized orphan ${name}`); + removed++; } return removed;