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
143 lines
5.6 KiB
TypeScript
143 lines
5.6 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from "vitest";
|
|
import "./executor-test-helpers.js";
|
|
import { TaskExecutor } from "../executor.js";
|
|
import { executorLog } from "../logger.js";
|
|
import type { Task } from "@fusion/core";
|
|
import { createMockStore, mockedExec, mockedExecSync, resetExecutorMocks } from "./executor-test-helpers.js";
|
|
|
|
function makeTask(overrides: Partial<Task> = {}): Task {
|
|
return {
|
|
id: "FN-4383",
|
|
title: "Test",
|
|
description: "Test",
|
|
column: "in-progress",
|
|
dependencies: [],
|
|
steps: [],
|
|
currentStep: 0,
|
|
log: [],
|
|
createdAt: new Date().toISOString(),
|
|
updatedAt: new Date().toISOString(),
|
|
...overrides,
|
|
} as Task;
|
|
}
|
|
|
|
describe("captureBaseCommitSha", () => {
|
|
beforeEach(() => {
|
|
resetExecutorMocks();
|
|
});
|
|
|
|
it("captures merge-base for fresh worktree", async () => {
|
|
mockedExec.mockImplementation(((cmd: any, _opts: any, cb: any) => {
|
|
cb(null, cmd.includes("merge-base") ? "abc1234\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(), "/tmp/test/.worktrees/fn-4383", audit);
|
|
|
|
expect(store.updateTask).toHaveBeenCalledWith("FN-4383", { baseCommitSha: "abc1234" });
|
|
expect(audit.git).toHaveBeenCalledWith(expect.objectContaining({ metadata: { purpose: "base", preserved: false } }));
|
|
});
|
|
|
|
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,
|
|
{ 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");
|
|
});
|
|
mockedExec.mockImplementation(((cmd: any, _opts: any, cb: any) => {
|
|
cb(null, cmd.includes("merge-base") ? "new456\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: "stale999" }), "/tmp/test/.worktrees/fn-4383", audit);
|
|
|
|
expect(store.updateTask).toHaveBeenCalledWith("FN-4383", { baseCommitSha: "new456" });
|
|
});
|
|
|
|
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,
|
|
{ isResume: true },
|
|
);
|
|
|
|
expect(store.updateTask).not.toHaveBeenCalled();
|
|
expect(audit.git).toHaveBeenCalledWith(expect.objectContaining({ metadata: { purpose: "base", preserved: true } }));
|
|
});
|
|
|
|
it("falls back to HEAD when merge-base fails", async () => {
|
|
mockedExec.mockImplementation(((cmd: any, _opts: any, cb: any) => {
|
|
if (String(cmd).includes("merge-base")) {
|
|
cb(new Error("merge-base failed"), "", "merge-base failed");
|
|
return {} as any;
|
|
}
|
|
cb(null, "head777\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(), "/tmp/test/.worktrees/fn-4383", audit);
|
|
|
|
expect(store.updateTask).toHaveBeenCalledWith("FN-4383", { baseCommitSha: "head777" });
|
|
expect(vi.mocked(executorLog.warn)).toHaveBeenCalledWith(expect.stringContaining("falling back to HEAD"));
|
|
});
|
|
});
|