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

fix(FN-4417): stop false-positive branch contamination from stale baseCommitSha

Browse Source 
The contamination check at executor.ts was reusing task.baseCommitSha as
its reference SHA. That field is intentionally preserved across resumed
sessions for stable diff math, which means it can lag behind main by
many commits. Passing it to assertCleanBranchAtBase caused every
legitimately-merged commit on main since the stale SHA to be reported
as a foreign task-attributed contamination commit, pausing the task
with pausedReason=branch-cross-contamination.

FN-4403 was the trigger case: a pooled worktree was force-reset to
current main by WorktreePool.prepareForTask (correctly), then the
executor immediately ran assertCleanBranchAtBase(rootDir, branch,
staleBaseCommitSha, taskId) and flagged 157 commits across ~39
unrelated FN-* tasks as contamination. FN-4417 itself then hit the
same bug when it tried to start, blocking the board.

Two fixes, both in packages/engine/src/executor.ts:

1. New resolveContaminationBaseRef(worktreePath) computes a fresh
   merge-base against origin/main or main and is used in place of
   resolveDiffBaseRef for the contamination check. It never reads
   task.baseCommitSha and never falls back to HEAD~1 (which on a
   force-reset pooled branch would be a main commit and re-introduce
   the same false positive at smaller scale). Returns undefined on
   git failure so the caller treats it as check skipped.

2. captureBaseCommitSha gains an explicit { isResume: boolean }
   parameter and only preserves an existing baseCommitSha when
   isResume is true. On fresh/pool acquisitions the branch was just
   force-reset to current main, so the stored value is stale by
   definition. Always recapture in that case. Diff-base stability
   across resumed sessions (FN-4309/FN-4383) is preserved by passing
   isResume: true on resume; the existing executor call site is
   already gated on non-resume and passes false.

Tests:
- executor-base-commit-capture.test.ts: updated to thread isResume
  through assertions and added a FN-4417 regression case that verifies
  a stale-but-ancestor baseCommitSha is recaptured (not preserved) on
  non-resume.
- executor-base-commit-capture.real-git.test.ts: FN-4309/FN-4383
  multi-session test now explicitly passes isResume: true on the
  second capture, matching the real resume code path.
- executor-contamination-base.test.ts (new): three focused tests for
  resolveContaminationBaseRef covering fresh-merge-base resolution,
  graceful failure when neither origin/main nor main resolves, and a
  structural guard that the function arity is 1 (no baseCommitSha
  parameter, so the bug cannot regress through that surface).

Verified: 4265 engine tests pass; tsc clean.

Fusion-Task-Id: FN-4417
This commit is contained in:
gsxdsm
2026-05-13 18:44:41 -07:00
parent ef34f0ab97
commit a2b494ddc0
5 changed files with 190 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/fix-fn-4417-contamination-false-positive.md Normal file
View File

@@ -0,0 +1,5 @@
---
"@runfusion/fusion": patch
---
Fix false-positive `BranchCrossContaminationError` that paused tasks at start when their stored `baseCommitSha` was stale relative to `main`. The contamination check now computes a fresh merge-base against the integration branch instead of reusing the diff-stable `task.baseCommitSha`, and `captureBaseCommitSha` only preserves a prior stored value when resuming an existing worktree. Diff-base stability across resumed sessions is preserved (FN-4309/FN-4383 behavior unchanged).

6
packages/engine/src/__tests__/executor-base-commit-capture.real-git.test.ts
View File

@@ -65,14 +65,16 @@ describeIfGit("captureBaseCommitSha (real git)", () => {
const executor = new TaskExecutor(store, repo);
const audit = { git: vi.fn().mockResolvedValue(undefined) };
await (executor as any).captureBaseCommitSha(makeTask(), repo, audit);
await (executor as any).captureBaseCommitSha(makeTask(), repo, audit, { isResume: false });
const firstBase = (store.updateTask as any).mock.calls[0][1].baseCommitSha as string;
expect(firstBase).toBeTruthy();
writeFileSync(path.join(repo, "branch-18.txt"), "branch 18\n", "utf-8");
git(repo, "git add branch-18.txt && git commit -m 'branch 18'");
await (executor as any).captureBaseCommitSha(makeTask(firstBase), repo, audit);
// Resume of the same task: baseCommitSha must be preserved so diff math
// stays stable across sessions (FN-4309/FN-4383).
await (executor as any).captureBaseCommitSha(makeTask(firstBase), repo, audit, { isResume: true });
expect((store.updateTask as any).mock.calls).toHaveLength(1);

47
packages/engine/src/__tests__/executor-base-commit-capture.test.ts
View File

@@ -41,18 +41,52 @@ describe("captureBaseCommitSha", () => {
expect(audit.git).toHaveBeenCalledWith(expect.objectContaining({ metadata: { purpose: "base", preserved: false } }));
});
it("preserves existing valid baseCommitSha across sessions", async () => {
it("preserves existing valid baseCommitSha across resumed sessions", async () => {
mockedExecSync.mockReturnValue("");
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
const audit = { git: vi.fn().mockResolvedValue(undefined) };
await (executor as any).captureBaseCommitSha(makeTask({ baseCommitSha: "old123" }), "/tmp/test/.worktrees/fn-4383", audit);
await (executor as any).captureBaseCommitSha(
makeTask({ baseCommitSha: "old123" }),
"/tmp/test/.worktrees/fn-4383",
audit,
{ isResume: true },
);
expect(store.updateTask).not.toHaveBeenCalled();
expect(audit.git).toHaveBeenCalledWith(expect.objectContaining({ metadata: { purpose: "base", preserved: true } }));
});
it("recaptures baseCommitSha on non-resume acquisitions even when stored value is ancestor (FN-4417)", async () => {
// FN-4417 regression: on a fresh pool acquisition the branch was just
// force-reset to current main, so any stored baseCommitSha is stale
// relative to the new merge-base. Preserving it would re-introduce the
// false-positive contamination cascade.
mockedExecSync.mockReturnValue(""); // is-ancestor would succeed if asked
mockedExec.mockImplementation(((cmd: any, _opts: any, cb: any) => {
cb(null, cmd.includes("merge-base") ? "freshmainSHA\n" : "");
return {} as any;
}) as any);
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
const audit = { git: vi.fn().mockResolvedValue(undefined) };
await (executor as any).captureBaseCommitSha(
makeTask({ baseCommitSha: "stale_main_sha" }),
"/tmp/test/.worktrees/fn-4383",
audit,
{ isResume: false },
);
expect(store.updateTask).toHaveBeenCalledWith("FN-4383", { baseCommitSha: "freshmainSHA" });
expect(audit.git).toHaveBeenCalledWith(
expect.objectContaining({ metadata: { purpose: "base", preserved: false } }),
);
// Critically: is-ancestor must NOT have been the deciding factor.
// Even if it would have passed, non-resume always recaptures.
});
it("recaptures when existing baseCommitSha is not ancestor", async () => {
mockedExecSync.mockImplementation(() => {
throw new Error("not ancestor");
@@ -70,13 +104,18 @@ describe("captureBaseCommitSha", () => {
expect(store.updateTask).toHaveBeenCalledWith("FN-4383", { baseCommitSha: "new456" });
});
it("preserves prior merge base for FN-4309/FN-4383 multi-session regression", async () => {
it("preserves prior merge base on resume for FN-4309/FN-4383 multi-session regression", async () => {
mockedExecSync.mockReturnValue("");
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
const audit = { git: vi.fn().mockResolvedValue(undefined) };
await (executor as any).captureBaseCommitSha(makeTask({ baseCommitSha: "merge_base_sha" }), "/tmp/test/.worktrees/fn-4383", audit);
await (executor as any).captureBaseCommitSha(
makeTask({ baseCommitSha: "merge_base_sha" }),
"/tmp/test/.worktrees/fn-4383",
audit,
{ isResume: true },
);
expect(store.updateTask).not.toHaveBeenCalled();
expect(audit.git).toHaveBeenCalledWith(expect.objectContaining({ metadata: { purpose: "base", preserved: true } }));

90
packages/engine/src/__tests__/executor-contamination-base.test.ts Normal file
View File

@@ -0,0 +1,90 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
import "./executor-test-helpers.js";
import { TaskExecutor } from "../executor.js";
import { createMockStore, mockedExec, resetExecutorMocks } from "./executor-test-helpers.js";
/**
* FN-4417 regression: the contamination check must compute its own fresh
* merge-base against the integration branch, not reuse `task.baseCommitSha`.
*
* Before FN-4417, on a freshly pool-acquired worktree (branch force-reset to
* current main) the executor passed `task.baseCommitSha` to the contamination
* `git log <base>..<branch>` query. When that stored SHA was stale, every
* legitimately-merged commit on main since the stale SHA appeared as a
* "foreign task-attributed commit" and the task was paused with
* `pausedReason: "branch-cross-contamination"`. FN-4403 hit this with 157
* false-positive foreign commits across ~39 unrelated FN-* tasks.
*/
describe("resolveContaminationBaseRef (FN-4417)", () => {
beforeEach(() => {
resetExecutorMocks();
});
it("returns the current merge-base with origin/main, ignoring task.baseCommitSha", async () => {
const calls: string[] = [];
mockedExec.mockImplementation(((cmd: any, _opts: any, cb: any) => {
calls.push(String(cmd));
if (String(cmd).includes("merge-base")) {
cb(null, "fresh_main_sha\n");
} else {
cb(null, "");
}
return {} as any;
}) as any);
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
const result = await (executor as any).resolveContaminationBaseRef(
"/tmp/test/.worktrees/swift-delta",
);
expect(result).toBe("fresh_main_sha");
// Must have asked for merge-base against origin/main || main, never
// fallen back to HEAD~1 or read task.baseCommitSha.
expect(calls.some((c) => c.includes("merge-base HEAD origin/main"))).toBe(true);
expect(calls.some((c) => c.includes("HEAD~1"))).toBe(false);
});
it("returns undefined when neither origin/main nor main resolves", async () => {
mockedExec.mockImplementation(((_cmd: any, _opts: any, cb: any) => {
cb(new Error("fatal: no main"), "", "fatal: no main");
return {} as any;
}) as any);
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
const result = await (executor as any).resolveContaminationBaseRef(
"/tmp/test/.worktrees/swift-delta",
);
// Caller (execute()) treats undefined as "skip contamination check"
// rather than crash the run on git failure.
expect(result).toBeUndefined();
});
it("does NOT fall back to task.baseCommitSha (FN-4417 false-positive guard)", async () => {
// Simulate the exact FN-4403 condition: merge-base succeeds with a fresh
// SHA. Even if a stale baseCommitSha is on the task, the contamination
// base resolver must use the fresh merge-base output.
mockedExec.mockImplementation(((cmd: any, _opts: any, cb: any) => {
cb(null, String(cmd).includes("merge-base") ? "currentMainSHA\n" : "");
return {} as any;
}) as any);
const store = createMockStore();
const executor = new TaskExecutor(store, "/tmp/test");
// resolveContaminationBaseRef takes only worktreePath — it has no API
// surface that accepts task.baseCommitSha. That is the structural
// guarantee of the fix.
const result = await (executor as any).resolveContaminationBaseRef(
"/tmp/test/.worktrees/swift-delta",
);
expect(result).toBe("currentMainSHA");
// Sanity: function arity is 1, not 2 (no baseCommitSha parameter).
expect((executor as any).resolveContaminationBaseRef.length).toBe(1);
});
});

53
packages/engine/src/executor.ts
View File

@@ -2510,11 +2510,18 @@ export class TaskExecutor {
// Capture the base commit SHA for diff computation whenever a task
// starts with a newly assigned worktree.
if (!acquisition.isResume) {
await this.captureBaseCommitSha(task, worktreePath, audit);
await this.captureBaseCommitSha(task, worktreePath, audit, { isResume: false });
}
const latestTaskForBase = await this.store.getTask(task.id);
const contaminationBaseRef = await this.resolveDiffBaseRef(worktreePath, latestTaskForBase.baseCommitSha);
// Contamination check must use a FRESH merge-base with the integration
// branch — NOT task.baseCommitSha. baseCommitSha is intentionally
// preserved across sessions for stable diff math, which makes it
// potentially stale relative to main. Using it here would falsely flag
// every legitimately-merged commit on main since that stale SHA as
// "foreign contamination" (see FN-4417). The real signal we want is:
// does the branch contain commits past its current merge-base with main
// that are attributed to OTHER tasks? Compute the merge-base fresh.
const contaminationBaseRef = await this.resolveContaminationBaseRef(worktreePath);
if (contaminationBaseRef) {
await assertCleanBranchAtBase(this.rootDir, acquisition.branch, contaminationBaseRef, task.id);
}
@@ -5689,15 +5696,23 @@ ${failureFeedback}
task: Task,
worktreePath: string,
audit: { git: (event: { type: "commit:create"; target: string; metadata: Record<string, unknown> }) => Promise<void> },
options: { isResume: boolean } = { isResume: false },
): Promise<void> {
try {
if (task.baseCommitSha) {
// Preserve an existing baseCommitSha only on RESUME of the same
// worktree, where diff-base stability across sessions of the same task
// matters. On fresh/pooled acquisitions the branch was just
// force-reset to current main, so any stored baseCommitSha is by
// definition behind the new merge-base — preserving it would yield
// stale diff math and (when reused as a contamination reference) the
// FN-4417 false-positive cascade. Always recapture on non-resume.
if (options.isResume && task.baseCommitSha) {
try {
execSync(`git merge-base --is-ancestor ${task.baseCommitSha} HEAD`, {
cwd: worktreePath,
stdio: "pipe",
});
executorLog.log(`${task.id}: preserved baseCommitSha ${task.baseCommitSha.slice(0, 7)}`);
executorLog.log(`${task.id}: preserved baseCommitSha ${task.baseCommitSha.slice(0, 7)} (resume)`);
await audit.git({
type: "commit:create",
target: task.baseCommitSha,
@@ -5739,6 +5754,34 @@ ${failureFeedback}
}
}
/**
* Resolve a fresh merge-base against the integration branch for use as a
* contamination check reference. Unlike {@link resolveDiffBaseRef}, this
* NEVER falls back to `task.baseCommitSha`, because a stale stored base
* would make the contamination check flag every legitimately-merged commit
* since that snapshot as "foreign" (FN-4417). It also never falls back to
* `HEAD~1`, because for a newly force-reset pooled branch HEAD~1 is a
* commit on main itself, which would yield the same false positive on a
* smaller scale.
*
* Returns `undefined` when neither `origin/main` nor `main` is resolvable;
* the caller is expected to treat that as "contamination check skipped".
*/
private async resolveContaminationBaseRef(worktreePath: string): Promise<string | undefined> {
try {
const { stdout } = await execAsync(
"git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main",
{ cwd: worktreePath, encoding: "utf-8" },
);
const ref = stdout.trim();
return ref || undefined;
} catch (err: unknown) {
const errorMessage = err instanceof Error ? err.message : String(err);
executorLog.warn(`Failed merge-base lookup for contamination check in ${worktreePath}: ${errorMessage}`);
return undefined;
}
}
/**
* Capture the list of files modified during agent execution.
* Uses git diff against the stored baseCommitSha to determine what changed.