fix(engine): split pre-merge rebase into independent remote + local-base stages
The pre-merge rebase had the local-base rebase nested inside the remote-rebase success path, so when no remote resolved (or worktreePath was missing), the entire block exited without running local-base. That left smart-prefer-main exposed: with no rebase, the -X ours fallback would silently re-introduce code main had recently deleted, which is exactly the case the strategy is meant to prevent. Restructure so remote rebase (Stage 1) and local-base rebase (Stage 2) run as independent gates. Local-base rebase still picks up sibling-task merges that landed locally even when the remote stage was skipped or disabled, so prefer-main always gets at least one defense. Also relax the semantic-incompatibility guard: prefer-main now requires EITHER stage to remain enabled (was: required worktreeRebaseBeforeMerge). Extracted runLocalBaseRebase() helper to remove duplication between the two entry points (after Stage 1 vs. standalone) and centralize the ancestor-check + abort handling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
@@ -677,7 +677,11 @@ describe("aiMergeTask — pre-merge rebase abort observability", () => {
|
||||
warnSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("hard-fails when prefer-main is paired with worktreeRebaseBeforeMerge=false", async () => {
|
||||
it("hard-fails when prefer-main is paired with both rebase stages disabled", async () => {
|
||||
// After the rebase block was split into two independent stages
|
||||
// (remote + local-base), prefer-main is satisfied by either stage running
|
||||
// successfully. Only when BOTH are explicitly disabled is the
|
||||
// configuration incoherent.
|
||||
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],
|
||||
@@ -686,6 +690,7 @@ describe("aiMergeTask — pre-merge rebase abort observability", () => {
|
||||
...DEFAULT_SETTINGS,
|
||||
mergeConflictStrategy: "smart-prefer-main",
|
||||
worktreeRebaseBeforeMerge: false,
|
||||
worktreeRebaseLocalBase: false,
|
||||
});
|
||||
|
||||
mockedExecSync.mockImplementation((cmd: any) => {
|
||||
@@ -696,10 +701,58 @@ describe("aiMergeTask — pre-merge rebase abort observability", () => {
|
||||
});
|
||||
|
||||
await expect(aiMergeTask(store, "/tmp/root", "FN-050")).rejects.toThrow(
|
||||
/Incompatible settings.*smart-prefer-main.*worktreeRebaseBeforeMerge/i,
|
||||
/Incompatible settings.*smart-prefer-main.*worktreeRebase/i,
|
||||
);
|
||||
});
|
||||
|
||||
it("local-base rebase still runs when remote is unresolvable (independent stages)", async () => {
|
||||
// Regression: previously the local-base rebase was nested inside the
|
||||
// remote-rebase success path, so a missing remote would skip BOTH. Now
|
||||
// the stages are independent — local-base runs even when remote can't
|
||||
// resolve, providing prefer-main with a usable safety net.
|
||||
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",
|
||||
});
|
||||
|
||||
let localBaseRebaseRan = false;
|
||||
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 === "git rev-parse --abbrev-ref HEAD") return "main" as any;
|
||||
// No remote configured: empty config + empty remote list
|
||||
if (cmdStr.includes("git config --get branch.")) return "" as any;
|
||||
if (cmdStr === "git remote") return "" as any;
|
||||
// Local HEAD lookup for Stage 2
|
||||
if (cmdStr === "git rev-parse HEAD") return "localhead123";
|
||||
// Stage 2 ancestor check fails (not contained), so rebase runs
|
||||
if (cmdStr.includes("merge-base --is-ancestor")) {
|
||||
const err: any = new Error("not an ancestor");
|
||||
err.status = 1;
|
||||
throw err;
|
||||
}
|
||||
// Stage 2 rebase command
|
||||
if (cmdStr.includes('git rebase "localhead123"')) {
|
||||
localBaseRebaseRan = true;
|
||||
return Buffer.from("");
|
||||
}
|
||||
if (cmdStr.includes("merge --squash")) return Buffer.from("");
|
||||
if (cmdStr.includes("diff --name-only --diff-filter=U")) return "";
|
||||
if (cmdStr.includes("diff --cached --quiet")) return "1";
|
||||
if (cmdStr.includes("show --shortstat")) return "1 file changed, 1 insertion(+), 0 deletions(-)";
|
||||
return Buffer.from("");
|
||||
});
|
||||
|
||||
await aiMergeTask(store, "/tmp/root", "FN-050");
|
||||
|
||||
expect(localBaseRebaseRan).toBe(true);
|
||||
});
|
||||
|
||||
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" },
|
||||
@@ -4938,6 +4991,14 @@ describe("aiMergeTask — merge details collection", () => {
|
||||
{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050" },
|
||||
[{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050", column: "in-review" } as Task],
|
||||
);
|
||||
// Use prefer-branch: this test simulates `git rev-parse HEAD` failing to
|
||||
// exercise merge-details fallback. The local-base rebase stage hits the
|
||||
// same command first; under prefer-main its failure would hard-fail. This
|
||||
// test isn't about prefer-main semantics, so opt out.
|
||||
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||
...DEFAULT_SETTINGS,
|
||||
mergeConflictStrategy: "smart-prefer-branch",
|
||||
});
|
||||
|
||||
let revParseHeadCalled = false;
|
||||
mockedExecSync.mockImplementation((cmd: any) => {
|
||||
|
||||
@@ -2272,26 +2272,86 @@ export async function aiMergeTask(
|
||||
// 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.
|
||||
// The block runs as TWO INDEPENDENT STAGES so that prefer-main always gets
|
||||
// the strongest available rebase coverage:
|
||||
// Stage 1 (remote): rebase onto remote/main when enabled + remote resolves.
|
||||
// Picks up upstream pushes from collaborators / other
|
||||
// workers.
|
||||
// Stage 2 (local-base): rebase onto rootDir's HEAD when enabled. Picks up
|
||||
// sibling-task merges that landed locally but
|
||||
// haven't pushed yet, AND covers the no-remote case
|
||||
// where Stage 1 silently skipped.
|
||||
// Either stage failing under prefer-main is a hard error.
|
||||
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
|
||||
|
||||
// Semantic guards: prefer-main with no rebase available is incoherent —
|
||||
// the strategy depends on 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
|
||||
&& settings.worktreeRebaseLocalBase === 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".`,
|
||||
`requires at least one of worktreeRebaseBeforeMerge or worktreeRebaseLocalBase ` +
|
||||
`to remain enabled. The strategy relies on rebasing the branch onto current main ` +
|
||||
`to preserve main's deletions; with both disabled it can silently re-introduce ` +
|
||||
`branch-only content. Re-enable a rebase stage or switch to "smart-prefer-branch" ` +
|
||||
`/ "ai-only".`,
|
||||
);
|
||||
}
|
||||
|
||||
// Helper: run the local-base rebase (Stage 2). Centralized so both
|
||||
// entry points (after Stage 1, or standalone when Stage 1 is disabled)
|
||||
// share the same logic for ancestor check, rebase, and abort handling.
|
||||
async function runLocalBaseRebase(label: string): Promise<void> {
|
||||
if (!worktreePath) return;
|
||||
try {
|
||||
const { stdout: localHeadOut } = await execAsync("git rev-parse HEAD", {
|
||||
cwd: rootDir,
|
||||
encoding: "utf-8",
|
||||
});
|
||||
const localHead = localHeadOut.trim();
|
||||
if (!localHead) return;
|
||||
|
||||
// Skip if worktree branch already contains local HEAD.
|
||||
let alreadyContains = false;
|
||||
try {
|
||||
await execAsync(`git merge-base --is-ancestor "${localHead}" HEAD`, { cwd: worktreePath });
|
||||
alreadyContains = true;
|
||||
} catch {
|
||||
// not an ancestor — rebase needed
|
||||
}
|
||||
|
||||
if (alreadyContains) {
|
||||
// Branch is already up-to-date with current main; prefer-main is
|
||||
// satisfied without re-running git rebase.
|
||||
rebaseHappened = true;
|
||||
return;
|
||||
}
|
||||
|
||||
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)}${label ? ` (${label})` : ""}`);
|
||||
} catch (localRebaseErr) {
|
||||
rethrowIfMergeAborted(localRebaseErr);
|
||||
const lmsg = localRebaseErr instanceof Error ? localRebaseErr.message : String(localRebaseErr);
|
||||
mergerLog.warn(`${taskId}: pre-merge rebase onto local HEAD failed (${lmsg}) — aborting and falling through`);
|
||||
try {
|
||||
await execAsync("git rebase --abort", { cwd: worktreePath });
|
||||
} catch (abortError: unknown) {
|
||||
mergerLog.warn(`${taskId}: failed to abort local-HEAD rebase: ${getCommandErrorMessage(abortError)}`);
|
||||
}
|
||||
if (mergeConflictStrategy === "smart-prefer-main" && !preferMainRebaseFailureMessage) {
|
||||
preferMainRebaseFailureMessage = `Pre-merge rebase onto local HEAD aborted (${lmsg})`;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ── Stage 1: remote rebase ────────────────────────────────────────────
|
||||
if (settings.worktreeRebaseBeforeMerge !== false) {
|
||||
try {
|
||||
// Resolve which remote to fetch. An explicit setting wins; otherwise
|
||||
@@ -2332,17 +2392,14 @@ export async function aiMergeTask(
|
||||
}
|
||||
|
||||
if (!remote) {
|
||||
mergerLog.log(`${taskId}: no remote resolvable — skipping pre-merge rebase`);
|
||||
mergerLog.log(`${taskId}: no remote resolvable — skipping remote rebase stage (local-base stage may still run)`);
|
||||
} else if (!worktreePath) {
|
||||
mergerLog.warn(`${taskId}: no worktreePath — skipping remote rebase stage`);
|
||||
} else {
|
||||
throwIfAborted(options.signal, taskId);
|
||||
mergerLog.log(`${taskId}: fetching ${remote} before merge`);
|
||||
await execAsync(`git fetch "${remote}"`, { cwd: rootDir });
|
||||
|
||||
// Rebase the task branch onto the freshly-fetched remote main.
|
||||
// Use a worktree-scoped checkout of the task branch, rebase, then
|
||||
// return rootDir to the main branch so the subsequent merge starts
|
||||
// from the expected state. If rebase fails we log and fall through
|
||||
// — aiMergeTask's attempt cascade still has a chance to succeed.
|
||||
try {
|
||||
const { stdout: mainBranchOut } = await execAsync(
|
||||
"git rev-parse --abbrev-ref HEAD",
|
||||
@@ -2350,73 +2407,14 @@ export async function aiMergeTask(
|
||||
);
|
||||
const mainBranch = mainBranchOut.trim();
|
||||
const remoteRef = `${remote}/${mainBranch}`;
|
||||
|
||||
// Rebase in the task's worktree (so the rootDir's HEAD isn't
|
||||
// disturbed). This is the same worktree the executor used.
|
||||
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.
|
||||
// Catches sibling-task merges that landed locally but haven't
|
||||
// been pushed to remote yet. Without this, two tasks based on a
|
||||
// common ancestor where one deletes code can have the other
|
||||
// silently resurrect that code via a -X theirs fallback.
|
||||
if (settings.worktreeRebaseLocalBase !== false) {
|
||||
try {
|
||||
const { stdout: localHeadOut } = await execAsync("git rev-parse HEAD", {
|
||||
cwd: rootDir,
|
||||
encoding: "utf-8",
|
||||
});
|
||||
const localHead = localHeadOut.trim();
|
||||
if (localHead) {
|
||||
// Skip if worktree branch already contains local HEAD.
|
||||
let alreadyContains = false;
|
||||
try {
|
||||
await execAsync(`git merge-base --is-ancestor "${localHead}" HEAD`, { cwd: worktreePath });
|
||||
alreadyContains = true;
|
||||
} catch {
|
||||
// not an ancestor — rebase needed
|
||||
}
|
||||
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) {
|
||||
rethrowIfMergeAborted(localRebaseErr);
|
||||
const lmsg = localRebaseErr instanceof Error ? localRebaseErr.message : String(localRebaseErr);
|
||||
mergerLog.warn(`${taskId}: pre-merge rebase onto local HEAD failed (${lmsg}) — aborting and falling through`);
|
||||
try {
|
||||
await execAsync("git rebase --abort", { cwd: worktreePath });
|
||||
} 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 {
|
||||
mergerLog.warn(`${taskId}: no worktreePath — skipping task branch rebase`);
|
||||
}
|
||||
throwIfAborted(options.signal, taskId);
|
||||
await execAsync(`git rebase "${remoteRef}"`, { cwd: worktreePath });
|
||||
rebaseHappened = true;
|
||||
mergerLog.log(`${taskId}: rebased ${branch} onto ${remoteRef}`);
|
||||
} catch (rebaseErr) {
|
||||
rethrowIfMergeAborted(rebaseErr);
|
||||
const msg = rebaseErr instanceof Error ? rebaseErr.message : String(rebaseErr);
|
||||
mergerLog.warn(`${taskId}: pre-merge rebase failed (${msg}) — aborting rebase and falling through to smart/AI merge`);
|
||||
mergerLog.warn(`${taskId}: pre-merge rebase failed (${msg}) — aborting rebase and falling through`);
|
||||
if (worktreePath) {
|
||||
try {
|
||||
await execAsync("git rebase --abort", { cwd: worktreePath });
|
||||
@@ -2424,61 +2422,31 @@ 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})`;
|
||||
preferMainRebaseFailureMessage = `Pre-merge rebase onto remote main aborted (${msg})`;
|
||||
}
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
rethrowIfMergeAborted(err);
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
mergerLog.warn(`${taskId}: pre-merge rebase pipeline failed (${msg}) — proceeding without rebase`);
|
||||
}
|
||||
} else if (settings.worktreeRebaseLocalBase !== false && worktreePath) {
|
||||
// Remote rebase is disabled but the user still wants the local-base
|
||||
// rebase. This catches sibling-task merges that landed locally even when
|
||||
// the project doesn't have a remote configured.
|
||||
try {
|
||||
const { stdout: localHeadOut } = await execAsync("git rev-parse HEAD", {
|
||||
cwd: rootDir,
|
||||
encoding: "utf-8",
|
||||
});
|
||||
const localHead = localHeadOut.trim();
|
||||
if (localHead) {
|
||||
let alreadyContains = false;
|
||||
try {
|
||||
await execAsync(`git merge-base --is-ancestor "${localHead}" HEAD`, { cwd: worktreePath });
|
||||
alreadyContains = true;
|
||||
} catch {
|
||||
// not an ancestor — rebase needed
|
||||
}
|
||||
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) {
|
||||
rethrowIfMergeAborted(localOnlyErr);
|
||||
const msg = localOnlyErr instanceof Error ? localOnlyErr.message : String(localOnlyErr);
|
||||
mergerLog.warn(`${taskId}: pre-merge local-HEAD rebase failed (${msg}) — falling through to smart/AI merge`);
|
||||
try {
|
||||
await execAsync("git rebase --abort", { cwd: worktreePath });
|
||||
} 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})`;
|
||||
}
|
||||
mergerLog.warn(`${taskId}: pre-merge remote rebase pipeline failed (${msg}) — proceeding without remote rebase`);
|
||||
}
|
||||
}
|
||||
|
||||
// ── Stage 2: local-base rebase ─────────────────────────────────────────
|
||||
// Runs independently of Stage 1, so the no-remote case still gets coverage.
|
||||
// Skipped if Stage 1 already aborted under prefer-main (we'll throw below
|
||||
// anyway, and a second attempt just adds noise).
|
||||
if (
|
||||
settings.worktreeRebaseLocalBase !== false
|
||||
&& !preferMainRebaseFailureMessage
|
||||
) {
|
||||
await runLocalBaseRebase(
|
||||
settings.worktreeRebaseBeforeMerge === false ? "remote rebase disabled" : "",
|
||||
);
|
||||
}
|
||||
|
||||
// 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.
|
||||
|
||||
Reference in New Issue
Block a user