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

fix(FN-5627): close merger TOCTOU + add fast-path reachability gate

Browse Source 
The merger persisted `mergeConfirmed: true` + `commitSha` to the task row
as soon as the local squash commit was built, BEFORE running
`git update-ref refs/heads/<integration>` to actually advance the
integration branch. If the ref-advance then failed for any reason (lock
contention, hook rejection, packed-refs race, or a misclassified non-CAS
error via the merger-ref-update-advance.ts string heuristic), the task row
was poisoned: the auto-merge scheduler's mergeConfirmed fast-path would
silently promote the never-landed work to 'done' on the next tick,
including emitting task:merged and closing the linked GitHub tracking
issue. The 'expected SHA == observed SHA' log signature on FN-5625 was a
red herring — the ref-advance had failed for non-race reasons but the
string heuristic in merger-ref-update-advance.ts classified it as
'concurrent-advance', and the downstream IntegrationBranchConcurrentAdvanceError
routed through the unsafe 'merge already confirmed' recovery path.

This silently dropped real work on at least 9 tasks across 2026-05-27/28
(FN-5596, FN-5597, FN-5599, FN-5612, FN-5613, FN-5614, FN-5616, FN-5623,
FN-5625) and likely affected older now-archived tasks for which evidence
has been pruned.

Three-layer fix:

1. merger.ts (~9752): in reuseTaskWorktreeMerge mode, persist
   `mergeConfirmed: false` initially. After advanceIntegrationBranchRef
   returns advanced=true, do a follow-up updateTask to flip the flag.
   Other merge paths (legacy in-place, verified no-op fast-paths,
   owned-commit recovery) advance the ref BEFORE the mergeDetails write
   and remain unchanged.

2. project-engine.ts (~1378): defense-in-depth reachability gate on the
   auto-merge 'merge already confirmed' fast-path. Before moveTask to
   'done', verify `git merge-base --is-ancestor <commitSha>
   refs/heads/<integration>` succeeds. On failure, clear mergeConfirmed,
   set status='failed' with descriptive error, leave task in 'in-review',
   and emit `merger:fast-path-blocked-foreign-commit` run-audit event.
   Legitimate no-op merges (no commitSha) bypass the gate; ancient tasks
   missing mergeTargetBranch also bypass to avoid false-positive parks.

3. merger-ref-update-advance.ts (~189): replace fragile string heuristic
   ('is at' / 'expected' / 'cannot lock ref' in stderr) with structured
   detection. After update-ref fails, re-read the ref: if observed ==
   expected, classify as `ref-update-refused` (no race occurred); only
   classify as `concurrent-advance` when ref actually moved. Eliminates
   the misleading 'expected X observed X' same-SHA pair.

Tests: 3 new regression tests covering all three layers. Full engine
suite: 6150 tests pass.

Fixes:
- FN-5625 (autopilot validator trigger fix lost)
- FN-5623 (`fn goals` CLI lost)
- FN-5616 (source-issue close handlers lost)
- FN-5614 (`fn update` collision retry lost)
- FN-5613 (dashboard reload banner lost)
- FN-5612 (bundled-plugin-install lost)
- FN-5599 (tablet modal width lost)
- FN-5597 (ntfy notifier priority lost)
- FN-5596 (PR tab spacing test lost)

Fusion-Task-Id: FN-5627
This commit is contained in:
gsxdsm
2026-05-28 13:02:30 -07:00
parent 200dda95dc
commit b2d547eae5
7 changed files with 434 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

78
packages/engine/src/__tests__/merger-ref-update-advance.test.ts
View File

@@ -257,6 +257,84 @@ describe("advanceIntegrationBranchRef", () => {
}
});
it("FN-5627: classifies update-ref refusal as ref-update-refused (NOT concurrent-advance) when ref did not move", async () => {
// Repro for the FN-5625/FN-5623 false-positive done class: an update-ref
// failure where the ref did NOT actually move (e.g., lock contention,
// hook rejection, packed-refs write race) was previously classified as
// `concurrent-advance` purely by string-matching the git error message
// ("is at" / "expected" / "cannot lock ref"). That heuristic
// misreported "expected X, observed X" with the SAME sha for both
// values, and the downstream IntegrationBranchConcurrentAdvanceError
// routed through the unsafe "merge already confirmed" recovery path.
//
// The fix: after update-ref fails, re-read the ref. If observed ==
// expected, classify as `ref-update-refused`. Only classify as
// `concurrent-advance` when the ref actually moved.
const dir = setupRepo("main");
const events: Array<{ type: string; metadata?: Record<string, unknown> }> = [];
try {
const expectedCurrentSha = git(dir, "git rev-parse refs/heads/main");
git(dir, "git checkout -b feat");
writeFileSync(join(dir, "feature.txt"), "feature\n");
git(dir, "git add feature.txt");
git(dir, "git commit -m feat");
const newSha = git(dir, "git rev-parse HEAD");
// Simulate update-ref refusal that did NOT move the ref. The cleanest
// way to force this is to inject a runGit stub that fails the
// update-ref call but leaves the ref untouched (re-read returns the
// same expected sha). We monkey-patch the internal test hooks.
const { __test__ } = await import("../merger-ref-update-advance.js");
const originalRunGit = __test__.runGit;
let updateRefSeen = false;
__test__.runGit = (async (args: string[], cwd: string) => {
if (args[0] === "update-ref") {
updateRefSeen = true;
// Mimic git's CAS-failure-shaped error message but without
// actually moving the ref. This matches the FN-5625 log exactly.
throw new Error(
`fatal: update_ref failed for ref 'refs/heads/main': cannot lock ref 'refs/heads/main': is at ${expectedCurrentSha} but expected ${expectedCurrentSha}`,
);
}
return originalRunGit(args, cwd);
}) as typeof originalRunGit;
try {
const result = await advanceIntegrationBranchRef({
rootDir: dir,
projectRootDir: dir,
integrationBranch: "main",
newSha,
expectedCurrentSha,
taskId: "FN-5627",
audit: {
git: async (event: any) => events.push(event),
} as any,
});
expect(updateRefSeen).toBe(true);
expect(result.advanced).toBe(false);
if (result.advanced) throw new Error("expected refusal");
// The critical assertion: ref-update-refused, NOT concurrent-advance.
expect(result.reason).toBe("ref-update-refused");
// Observed should equal expected because the ref did not move.
expect(result.observedCurrentSha).toBe(expectedCurrentSha);
expect(git(dir, "git rev-parse refs/heads/main")).toBe(expectedCurrentSha);
// The emitted audit event should also use the correct classification.
expect(String(events[0]?.metadata?.error ?? "")).toContain(
"ref-update-refused",
);
expect(String(events[0]?.metadata?.error ?? "")).not.toContain(
"concurrent-advance",
);
} finally {
__test__.runGit = originalRunGit;
}
} finally {
removeTmpDirSync(dir);
}
});
it("throws on missing precondition shas", async () => {
const dir = setupRepo("main");
try {

134
packages/engine/src/__tests__/project-engine.test.ts
View File

@@ -1499,6 +1499,140 @@ describe("ProjectEngine paused in-review auto-merge behavior", () => {
await engine.stop();
});
it("FN-5627: refuses fast-path and parks task when mergeConfirmed commitSha is not ancestor of integration branch", async () => {
// Repro for the FN-5625/FN-5623 false-positive done class: the merger
// has a TOCTOU between writing `mergeConfirmed: true` and `git update-ref`
// succeeding. When the ref-advance fails after the optimistic write, the
// task row is poisoned. Without this gate, the auto-merge fast-path
// would silently promote the poisoned row to `done`. With the gate, the
// fast-path verifies reachability and refuses, parking the task in
// in-review with status=failed for manual review.
const mockStore = createMockStore({ ...baseSettings, autoMerge: true });
mockStore.store.getTask.mockResolvedValueOnce({
id: "FN-poisoned",
column: "in-review",
paused: false,
mergeRetries: 0,
status: null,
branch: "fusion/fn-poisoned",
baseBranch: "main",
mergeDetails: {
mergeConfirmed: true,
commitSha: "abc123abc123abc123abc123abc123abc1234567",
mergeTargetBranch: "main",
mergedAt: "2026-05-28T19:34:17.022Z",
},
});
mocks.currentStore = mockStore.store;
// Simulate the gate: `git cat-file -e` succeeds (commit exists locally
// on the orphan task branch), but `git merge-base --is-ancestor` fails
// with exit code 1 (commit is NOT reachable from main).
mocks.execFile.mockImplementation((
_file: string,
args: string[],
_options: unknown,
callback?: (error: (Error & { code?: number }) | null, result: { stdout: string; stderr: string }) => void,
) => {
const cb = (typeof _options === "function" ? _options : callback) as (
error: (Error & { code?: number }) | null,
result: { stdout: string; stderr: string },
) => void;
if (args[0] === "cat-file") {
cb(null, { stdout: "", stderr: "" });
return {} as never;
}
if (args[0] === "merge-base" && args[1] === "--is-ancestor") {
const err = new Error("Command failed: git merge-base --is-ancestor") as Error & { code?: number };
err.code = 1;
cb(err, { stdout: "", stderr: "" });
return {} as never;
}
// Default success for any other git call.
cb(null, { stdout: "/usr/bin/mock\n", stderr: "" });
return {} as never;
});
const engine = createEngine();
await engine.start();
engine.enqueueMerge("FN-poisoned");
// Gate refuses the fast-path: the task row is updated with status=failed,
// mergeConfirmed is cleared, and the task is parked in in-review.
await vi.waitFor(() => {
const calls = (mockStore.store.updateTask as ReturnType<typeof vi.fn>).mock.calls as unknown as Array<[string, Record<string, unknown>]>;
const refusalCall = calls.find((call) =>
call[0] === "FN-poisoned"
&& call[1]?.status === "failed"
&& typeof call[1]?.error === "string"
&& /not reachable/.test(call[1].error as string),
);
expect(refusalCall).toBeDefined();
const updates = refusalCall![1] as { mergeDetails?: { mergeConfirmed?: boolean } };
expect(updates.mergeDetails?.mergeConfirmed).toBe(false);
});
// moveTask("done") was NOT called.
expect(mockStore.store.moveTask).not.toHaveBeenCalledWith("FN-poisoned", "done");
// task:merged was NOT emitted for the poisoned task.
const emitCalls = (mockStore.store.emit as ReturnType<typeof vi.fn>).mock.calls as unknown as Array<[string, { task?: { id: string } }]>;
const mergedCalls = emitCalls.filter((call) => call[0] === "task:merged");
const poisonedEmit = mergedCalls.find((call) => call[1]?.task?.id === "FN-poisoned");
expect(poisonedEmit).toBeUndefined();
await engine.stop();
});
it("FN-5627: fast-path still works when mergeConfirmed has no commitSha (verified-no-op path)", async () => {
// Legitimate no-op merges have mergeConfirmed=true with no commitSha
// (verified-short-circuit / proven-no-op / already-on-main paths). The
// reachability gate must not break those.
const mockStore = createMockStore({ ...baseSettings, autoMerge: true });
mockStore.store.getTask.mockResolvedValueOnce({
id: "FN-noop",
column: "in-review",
paused: false,
mergeRetries: 0,
status: null,
branch: "fusion/fn-noop",
mergeDetails: {
mergeConfirmed: true,
noOpMerge: true,
mergedAt: "2026-05-28T19:34:17.022Z",
mergeTargetBranch: "main",
// No commitSha — verified no-op.
},
});
mockStore.store.moveTask.mockResolvedValueOnce({
id: "FN-noop",
column: "done",
branch: "fusion/fn-noop",
mergeDetails: {
mergeConfirmed: true,
noOpMerge: true,
mergedAt: "2026-05-28T19:34:17.022Z",
mergeTargetBranch: "main",
},
} as any);
mocks.currentStore = mockStore.store;
const engine = createEngine();
await engine.start();
engine.enqueueMerge("FN-noop");
await vi.waitFor(() => {
expect(mockStore.store.emit).toHaveBeenCalledWith(
"task:merged",
expect.objectContaining({
merged: true,
task: expect.objectContaining({ id: "FN-noop", column: "done" }),
}),
);
});
await engine.stop();
});
it("emits task:merged when PR merge strategy returns merged", async () => {
const mockStore = createMockStore({ ...baseSettings, autoMerge: true });
mockStore.store.getTask