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

fix(engine): harden smart-prefer-main against silent rebase skips

Browse Source 
The smart-prefer-main strategy depends on a successful pre-merge rebase
to honor main's deletions. Previously, three failure modes silently fell
through to the -X ours merge, which would re-introduce code main had
just removed (because branch additions vs main deletions don't textually
conflict and -X ours only resolves content conflicts, not modify/delete).

- Hard-fail when prefer-main is paired with worktreeRebaseBeforeMerge=false
  (semantically incoherent combination)
- Hard-fail when the pre-merge rebase starts and aborts (any of the three
  rebase paths: remote, nested local-base, or fallback local-only)
- Warn (not throw) on environmental silent skips — no remote resolvable
  or no worktreePath — so the gap is observable in logs without breaking
  common test/setup environments

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-04-28 16:57:17 -07:00
parent cc9181db47
commit d6d5aa570f
2 changed files with 143 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

66
packages/engine/src/__tests__/merger.test.ts
View File

@@ -578,6 +578,11 @@ describe("aiMergeTask — pre-merge rebase abort observability", () => {
{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050" },
[{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050", column: "in-review" } as Task],
);
// Fall-through is only allowed for prefer-branch; prefer-main hard-fails (see test below).
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
...DEFAULT_SETTINGS,
mergeConflictStrategy: "smart-prefer-branch",
});
mockedExecSync.mockImplementation((cmd: any) => {
const cmdStr = String(cmd);
@@ -624,6 +629,10 @@ describe("aiMergeTask — pre-merge rebase abort observability", () => {
{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050" },
[{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050", column: "in-review" } as Task],
);
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
...DEFAULT_SETTINGS,
mergeConflictStrategy: "smart-prefer-branch",
});
const warnSpy = vi.spyOn(mergerLog, "warn");
const abortFailureMessage = "fatal: no rebase in progress";
@@ -667,6 +676,63 @@ describe("aiMergeTask — pre-merge rebase abort observability", () => {
warnSpy.mockRestore();
});
it("hard-fails when prefer-main is paired with worktreeRebaseBeforeMerge=false", async () => {
const store = createMockStore(
{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050" },
[{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050", column: "in-review" } as Task],
);
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
...DEFAULT_SETTINGS,
mergeConflictStrategy: "smart-prefer-main",
worktreeRebaseBeforeMerge: false,
});
mockedExecSync.mockImplementation((cmd: any) => {
const cmdStr = String(cmd);
if (cmdStr.includes("rev-parse --verify")) return Buffer.from("abc123");
if (cmdStr.includes("git rev-parse --abbrev-ref")) return "main" as any;
return Buffer.from("");
});
await expect(aiMergeTask(store, "/tmp/root", "FN-050")).rejects.toThrow(
/Incompatible settings.*smart-prefer-main.*worktreeRebaseBeforeMerge/i,
);
});
it("hard-fails when smart-prefer-main rebase aborts (no silent fall-through)", async () => {
const store = createMockStore(
{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050" },
[{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050", column: "in-review" } as Task],
);
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
...DEFAULT_SETTINGS,
mergeConflictStrategy: "smart-prefer-main",
});
mockedExecSync.mockImplementation((cmd: any) => {
const cmdStr = String(cmd);
if (cmdStr.includes("rev-parse --verify")) return Buffer.from("abc123");
if (cmdStr.includes("git symbolic-ref --short HEAD")) return "main" as any;
if (cmdStr.includes("git rev-parse --abbrev-ref origin/HEAD")) return "origin/main" as any;
if (cmdStr === "git rev-parse --abbrev-ref HEAD") return "main" as any;
if (cmdStr.includes("git config --get branch.main.remote")) return "origin" as any;
if (cmdStr === 'git fetch "origin"') return Buffer.from("");
if (cmdStr === 'git rebase "origin/main"') {
throw new Error("pre-merge rebase conflict");
}
if (cmdStr === "git rebase --abort") return Buffer.from("");
return Buffer.from("");
});
await expect(aiMergeTask(store, "/tmp/root", "FN-050")).rejects.toThrow(
/smart-prefer-main.*rebase/i,
);
// Critical: the unsafe -X ours fallback must not have run.
expect(
mockedExec.mock.calls.some(([command]) => String(command).includes("merge -X ours")),
).toBe(false);
});
});
describe("aiMergeTask — task.branch field", () => {

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

@@ -2266,6 +2266,32 @@ export async function aiMergeTask(
//
// Controlled by `settings.worktreeRebaseBeforeMerge` (default true) and
// `settings.worktreeRebaseRemote` (empty → use repo's default remote).
//
// For "smart-prefer-main" we treat a rebase abort as a hard error: a stale
// branch base means the -X ours fallback can silently re-add code that main
// recently deleted (the merge sees branch additions vs main deletions as
// non-conflicting). Track here and throw outside the catch wrapper.
//
// We also surface silent-skip cases (no remote, no worktreePath) as
// warnings so they're observable in logs even when prefer-main can't
// strictly enforce its semantics.
let rebaseHappened = false;
let preferMainRebaseFailureMessage: string | undefined;
// Semantic guard: prefer-main + rebase explicitly disabled is incoherent —
// the strategy depends on the rebase to honor main's deletions. Fail fast
// before we waste work attempting a merge that can't deliver its promise.
if (
settings.worktreeRebaseBeforeMerge === false
&& mergeConflictStrategy === "smart-prefer-main"
) {
throw new Error(
`Incompatible settings for ${taskId}: mergeConflictStrategy="smart-prefer-main" ` +
`requires worktreeRebaseBeforeMerge to remain enabled. The strategy relies on ` +
`rebasing the branch onto current main to preserve main's deletions; with rebase ` +
`disabled it can silently re-introduce branch-only content. Re-enable ` +
`worktreeRebaseBeforeMerge or switch to "smart-prefer-branch" / "ai-only".`,
);
}
if (settings.worktreeRebaseBeforeMerge !== false) {
try {
// Resolve which remote to fetch. An explicit setting wins; otherwise
@@ -2330,6 +2356,7 @@ export async function aiMergeTask(
if (worktreePath) {
throwIfAborted(options.signal, taskId);
await execAsync(`git rebase "${remoteRef}"`, { cwd: worktreePath });
rebaseHappened = true;
mergerLog.log(`${taskId}: rebased ${branch} onto ${remoteRef}`);
// Stage 2: also rebase onto rootDir's local HEAD when enabled.
@@ -2356,7 +2383,13 @@ export async function aiMergeTask(
if (!alreadyContains) {
throwIfAborted(options.signal, taskId);
await execAsync(`git rebase "${localHead}"`, { cwd: worktreePath });
rebaseHappened = true;
mergerLog.log(`${taskId}: rebased ${branch} onto local HEAD ${localHead.slice(0, 8)}`);
} else {
// Already contains current main — branch is up-to-date,
// so prefer-main semantics are satisfied even without a
// fresh rebase command running.
rebaseHappened = true;
}
}
} catch (localRebaseErr) {
@@ -2368,6 +2401,13 @@ export async function aiMergeTask(
} catch (abortError: unknown) {
mergerLog.warn(`${taskId}: failed to abort local-HEAD rebase: ${getCommandErrorMessage(abortError)}`);
}
// Strict prefer-main semantics: a stale branch base means main's
// deletions can be silently re-added by the -X ours fallback.
// Record so we can throw after the outer catch.
if (mergeConflictStrategy === "smart-prefer-main") {
preferMainRebaseFailureMessage =
`Pre-merge rebase onto local HEAD aborted (${lmsg})`;
}
}
}
} else {
@@ -2384,6 +2424,11 @@ export async function aiMergeTask(
mergerLog.warn(`${taskId}: failed to abort pre-merge rebase: ${getCommandErrorMessage(abortError)}`);
}
}
// See above: prefer-main semantics require a successful rebase.
if (mergeConflictStrategy === "smart-prefer-main") {
preferMainRebaseFailureMessage =
`Pre-merge rebase onto remote main aborted (${msg})`;
}
}
}
} catch (err) {
@@ -2412,7 +2457,10 @@ export async function aiMergeTask(
if (!alreadyContains) {
throwIfAborted(options.signal, taskId);
await execAsync(`git rebase "${localHead}"`, { cwd: worktreePath });
rebaseHappened = true;
mergerLog.log(`${taskId}: rebased ${branch} onto local HEAD ${localHead.slice(0, 8)} (remote rebase disabled)`);
} else {
rebaseHappened = true;
}
}
} catch (localOnlyErr) {
@@ -2424,9 +2472,38 @@ export async function aiMergeTask(
} catch (abortError: unknown) {
mergerLog.warn(`${taskId}: failed to abort local-HEAD rebase: ${getCommandErrorMessage(abortError)}`);
}
if (mergeConflictStrategy === "smart-prefer-main") {
preferMainRebaseFailureMessage =
`Pre-merge local-HEAD rebase aborted (${msg})`;
}
}
}
// Hard-fail prefer-main when a rebase started and aborted: a stale branch
// base means the -X ours fallback can silently re-introduce branch-only
// content that main recently deleted.
if (preferMainRebaseFailureMessage) {
throw new Error(
`${preferMainRebaseFailureMessage} for ${taskId}. ` +
`Strategy "smart-prefer-main" requires a successful rebase to preserve main's deletions; ` +
`falling through to a -X ours merge would silently re-introduce branch-only content. ` +
`Resolve the rebase conflict manually, or switch mergeConflictStrategy to ` +
`"smart-prefer-branch" / "ai-only".`,
);
}
// Silent-skip observability: when prefer-main couldn't run a rebase at all
// (no remote resolvable, no worktreePath), warn loudly so the gap is visible
// in logs. Not a hard fail — environmental skips are common in tests and
// some setups, and would cause too much breakage to enforce here. Production
// monitoring can alert on this warning.
if (mergeConflictStrategy === "smart-prefer-main" && !rebaseHappened) {
mergerLog.warn(
`${taskId}: smart-prefer-main ran without a successful pre-merge rebase ` +
`(${worktreePath ? "no remote resolvable or rebase disabled" : "no worktreePath"}). ` +
`Main's deletions may not be preserved if the branch re-introduces them.`,
);
}
// 4. Gather context for the agent (used in all attempts)
// Keep this range strategy aligned with dashboard changed-files endpoints.
const diffBaseRef = await resolveTaskDiffBaseRef({