root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(FN-5345): address third-pass review findings

Browse Source 
Follow-up to c64884c24 addressing three review findings, including one
real interaction bug caught by a new test.

MEDIUM

- Re-indented and rewrote 'if (directReuseEligible) try { ... } catch'
  as 'if (directReuseEligible) { try { ... } catch { ... } }' with the
  whole body at one consistent indent level. No behavior change \u2014 fixes
  the mismatched indentation from c64884c24 where the body sat one level
  deeper than its containing block.

LOW

- Two new backstop tests in merge-reuse-task-worktree.test.ts:
  * 'preserves worktrees with uncommitted tracked changes' \u2014 asserts the
    fast-path leaves a tracked-dirty worktree alone (result.worktreeRemoved
    is false, dir still exists). Without this, a future refactor could
    silently re-enable destructive cleanup.
  * 'cleans up worktrees with only untracked noise' \u2014 asserts untracked
    junk (.DS_Store, editor swap files) does NOT block cleanup. Also caught
    a real interaction bug: 'git worktree remove' without --force refuses
    on untracked files, so the LOW finding's intent (drop noise, preserve
    tracked dirt) needs --force on the removal call. Restored --force with
    a comment explaining why it's safe (the tracked-only dirty check above
    already refused if there was real work to preserve).

- Switched 'git status' check from '--untracked-files=normal' to
  '--untracked-files=no'. Tracked modifications and staged changes still
  block cleanup; untracked junk is correctly ignored. Dirty-skip warn log
  now includes the first 5 dirty paths for operator diagnosability.

Tests
  - Full @fusion/engine suite: 448 files / 5883 tests / 9 skipped, all green
  - pnpm lint green, pnpm build green
This commit is contained in:
Fusion (runfusion.ai)
2026-05-20 18:21:14 -07:00
parent c64884c24a
commit e814ba9508
2 changed files with 145 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

119
packages/engine/src/__tests__/reliability-interactions/merge-reuse-task-worktree.test.ts
View File

@@ -1,4 +1,5 @@
import { mkdir } from "node:fs/promises";
import { mkdir, writeFile } from "node:fs/promises";
import { existsSync } from "node:fs";
import { join } from "node:path";
import { describe, expect, it, vi } from "vitest";
@@ -653,4 +654,120 @@ describe("FN-5279 reliability interactions: merge reuse task worktree", () => {
},
30_000,
);
// FN-5345/FN-5377 cleanup-safety backstop: the fast-path's worktree removal
// MUST preserve a worktree that has uncommitted tracked changes. We run
// `git status --porcelain --untracked-files=no` and skip the removal when
// tracked dirt is present. Untracked junk does not count (operator noise
// like .DS_Store should not block cleanup).
it.skipIf(!hasGit)(
"FN-5345: empty-own-diff fast-path preserves worktrees with uncommitted tracked changes",
async () => {
const fixture = await makeReliabilityFixture({
taskId: "FN-5279-RI-DIRTY-PRESERVE",
settings: {
baseBranch: "master",
mergeIntegrationWorktree: "reuse-task-worktree",
} as any,
});
try {
const { rootDir, store, task } = fixture;
const actualTask = await store.getTask(task.id);
const branch = `fusion/${actualTask!.id.toLowerCase()}`;
const worktreeRoot = `${rootDir}-worktrees`;
const worktreePath = join(worktreeRoot, actualTask!.id.toLowerCase());
git(rootDir, "git branch -m main master");
const completedSteps = (actualTask?.steps ?? []).map((step) => ({ ...step, status: "done" as const }));
await store.updateTask(task.id, {
baseBranch: "master",
branch,
steps: completedSteps,
currentStep: completedSteps.length,
} as any);
await fixture.createBranch(branch);
// Empty-own-diff handoff commit on the branch.
git(rootDir, `git commit --allow-empty -m 'test(${actualTask!.id}): verification-only handoff'`);
await fixture.checkout("master");
// Create the worktree at branch tip, then introduce a tracked
// modification (not committed) to simulate agent scratch.
await mkdir(worktreeRoot, { recursive: true });
git(rootDir, `git worktree add ${JSON.stringify(worktreePath)} ${JSON.stringify(branch)}`);
// README.md is created by the fixture as a tracked file. Modify it to
// produce tracked-dirty status.
await writeFile(join(worktreePath, "README.md"), "agent scratch: uncommitted edits\n");
await store.updateTask(task.id, { worktree: worktreePath, branch } as any);
store.enqueueMergeQueue(task.id);
const result = await aiMergeTask(store, rootDir, task.id);
expect(result.merged).toBe(true);
expect(result.noOp).toBe(true);
// Critical: the worktree must NOT be removed because it has tracked
// uncommitted changes. result.worktreeRemoved reflects that.
expect(result.worktreeRemoved).toBe(false);
expect(existsSync(worktreePath)).toBe(true);
expect(existsSync(join(worktreePath, "README.md"))).toBe(true);
} finally {
await fixture.cleanup();
}
},
30_000,
);
// FN-5345/FN-5377 cleanup-noise backstop: untracked junk (e.g. .DS_Store,
// editor swap files, build artifacts that are not gitignored at the task
// worktree level) must NOT block fast-path cleanup. Only tracked dirt does.
it.skipIf(!hasGit)(
"FN-5345: empty-own-diff fast-path cleans up worktrees with only untracked noise",
async () => {
const fixture = await makeReliabilityFixture({
taskId: "FN-5279-RI-UNTRACKED-OK",
settings: {
baseBranch: "master",
mergeIntegrationWorktree: "reuse-task-worktree",
} as any,
});
try {
const { rootDir, store, task } = fixture;
const actualTask = await store.getTask(task.id);
const branch = `fusion/${actualTask!.id.toLowerCase()}`;
const worktreeRoot = `${rootDir}-worktrees`;
const worktreePath = join(worktreeRoot, actualTask!.id.toLowerCase());
git(rootDir, "git branch -m main master");
const completedSteps = (actualTask?.steps ?? []).map((step) => ({ ...step, status: "done" as const }));
await store.updateTask(task.id, {
baseBranch: "master",
branch,
steps: completedSteps,
currentStep: completedSteps.length,
} as any);
await fixture.createBranch(branch);
git(rootDir, `git commit --allow-empty -m 'test(${actualTask!.id}): verification-only handoff'`);
await fixture.checkout("master");
await mkdir(worktreeRoot, { recursive: true });
git(rootDir, `git worktree add ${JSON.stringify(worktreePath)} ${JSON.stringify(branch)}`);
// Sprinkle untracked-only noise into the worktree.
await writeFile(join(worktreePath, ".DS_Store"), "binary junk\n");
await writeFile(join(worktreePath, "editor.swp"), "swap file\n");
await store.updateTask(task.id, { worktree: worktreePath, branch } as any);
store.enqueueMergeQueue(task.id);
const result = await aiMergeTask(store, rootDir, task.id);
expect(result.merged).toBe(true);
expect(result.noOp).toBe(true);
// Untracked-only is treated as clean — cleanup proceeds.
expect(result.worktreeRemoved).toBe(true);
expect(existsSync(worktreePath)).toBe(false);
} finally {
await fixture.cleanup();
}
},
30_000,
);
});

40
packages/engine/src/merger.ts
View File

@@ -6510,9 +6510,11 @@ async function tryEarlyEmptyOwnDiffFinalize(input: {
//
// Safety rules:
// - FN-4811: never touch a worktree owned by a different task.
// - Dirty worktrees are left alone (no --force) so we never silently
// discard uncommitted scratch; self-healing's worktree sweep handles
// them later.
// - Dirty worktrees (tracked modifications or staged changes) are left
// alone (no --force) so we never silently discard uncommitted work.
// Untracked junk (.DS_Store, editor swap files, build artifacts) does
// NOT block cleanup — we use `--untracked-files=no` and only respect
// tracked dirt as a signal of agent work in progress.
// - Branch deletion only fires when task.branch was non-null on entry
// (i.e. the task explicitly owned a branch). If task.branch was null,
// `cleanupOrphanedBranches` handles any orphan ref later.
@@ -6528,12 +6530,18 @@ async function tryEarlyEmptyOwnDiffFinalize(input: {
);
} else {
let dirty = false;
let dirtyDiagnostic: string | undefined;
try {
const { stdout } = await execAsync(
`git status --porcelain --untracked-files=normal`,
`git status --porcelain --untracked-files=no`,
{ cwd: stranded, encoding: "utf-8", timeout: 15_000 },
);
dirty = stdout.trim().length > 0;
const trimmed = stdout.trim();
dirty = trimmed.length > 0;
if (dirty) {
// First few dirty paths, for operator diagnosis.
dirtyDiagnostic = trimmed.split("\n").slice(0, 5).join("; ");
}
} catch (statusErr) {
// Treat status failure as "unknown" — fail safe by skipping removal.
dirty = true;
@@ -6543,12 +6551,16 @@ async function tryEarlyEmptyOwnDiffFinalize(input: {
}
if (dirty) {
log.warn(
`${taskId}: skipping early-fast-path worktree cleanup — ${stranded} has uncommitted changes; self-healing sweep will reconcile later`,
`${taskId}: skipping early-fast-path worktree cleanup — ${stranded} has uncommitted tracked changes${dirtyDiagnostic ? ` (${dirtyDiagnostic})` : ""}; self-healing sweep will reconcile later`,
);
} else {
try {
// --force here is safe: the tracked-only dirty check above already
// refused if there were uncommitted tracked changes. --force lets
// us discard untracked junk (.DS_Store, editor swap files, etc.)
// that would otherwise block `git worktree remove`.
await execAsync(
`git worktree remove ${quoteArg(stranded)}`,
`git worktree remove --force ${quoteArg(stranded)}`,
{ cwd: projectRootDir, timeout: 30_000 },
);
worktreeRemoved = true;
@@ -6773,8 +6785,9 @@ export async function aiMergeTask(
// lease bookkeeping stays consistent. Skip the direct-reuse shortcut here
// and fall through to the existing acquisition path.
const directReuseEligible = !(options.pool && settings.recycleWorktrees);
if (directReuseEligible) try {
const { stdout: porcelain } = await execAsync(
if (directReuseEligible) {
try {
const { stdout: porcelain } = await execAsync(
`git worktree list --porcelain`,
{ cwd: projectRootDir, encoding: "utf-8", timeout: 30_000 },
);
@@ -6885,10 +6898,11 @@ export async function aiMergeTask(
});
return;
}
} catch (listErr) {
mergerLog.warn(
`${taskId}: git worktree list consult failed before reacquire; proceeding with fresh creation: ${listErr instanceof Error ? listErr.message : String(listErr)}`,
);
} catch (listErr) {
mergerLog.warn(
`${taskId}: git worktree list consult failed before reacquire; proceeding with fresh creation: ${listErr instanceof Error ? listErr.message : String(listErr)}`,
);
}
}
const acquisition = await acquireTaskWorktree({