feat(core,engine): split smart merge strategy into prefer-main / prefer-branch

The single "smart" strategy is now two flavors with the new default flipped to prefer-main. Both share a pre-cascade `git fetch origin <currentBranch>` + best-effort fast-forward so a freshly-pushed sibling commit doesn't get clobbered when the fallback resolves a conflict against a stale base. - "smart-prefer-main" (new default): -X ours fallback. Protects just-merged sibling work from being regressed by a concurrent task branch. - "smart-prefer-branch": -X theirs fallback. Equivalent to legacy "smart". Legacy "smart" / "prefer-main" enum values are accepted and normalized via `normalizeMergeConflictStrategy()` so existing settings.json files migrate seamlessly. The fast-forward step gracefully degrades on fetch failure or divergent local main (logs and continues). Updates settings UI dropdown, test helpers, and adds 5 fetch+ff regression tests + 7 normalize-helper tests. Lint cleanup of two empty catch blocks in scripts/release.mjs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-28 15:44:38 -07:00
parent e4e75a28c4
commit ad45c0b1b4
9 changed files with 347 additions and 45 deletions
--- a/packages/engine/src/tests/merger.test.ts
+++ b/packages/engine/src/tests/merger.test.ts
@@ -164,8 +164,12 @@ function createMockStore(taskOverrides: Partial<Task> = {}, allTasks: Task[] = [
 * rev-parse, log, diff, merge --squash, diff --cached --quiet (squash check),
 * diff --cached (post-agent verify), branch -d
 *
- * For tests that want the merge to fail after 3 AI attempts (before -X theirs succeeds),
- * call setupFailingTheirsStrategy() instead.
+ * Both `-X ours` and `-X theirs` final-fallback merges return success — the
+ * default settings strategy is "smart-prefer-main" (-X ours), but a few tests
+ * still exercise -X theirs explicitly via `mergeConflictStrategy: "smart-prefer-branch"`.
+ *
+ * For tests that want the merge to fail after 3 attempts, call
+ * setupFailingFallbackStrategy() instead.
 */
 function setupHappyPathExecSync() {
  mockedExecSync.mockImplementation((cmd: any) => {
@@ -176,7 +180,9 @@ function setupHappyPathExecSync() {
    if (cmdStr.includes("merge-base")) return Buffer.from("abc123");
    if (cmdStr.includes("git diff") && cmdStr.includes("--stat")) return "1 file changed" as any;
    if (cmdStr.includes("merge --squash")) return Buffer.from("");
-    if (cmdStr.includes("merge -X theirs --squash")) return Buffer.from("");
+    if (cmdStr.includes("merge -X theirs --squash") || cmdStr.includes("merge -X ours --squash")) {
+      return Buffer.from("");
+    }
    // Post-squash check: --quiet means "did squash stage anything?" → "1" = yes
    if (cmdStr.includes("diff --cached --quiet")) return "1" as any;
    // Post-agent check: "did agent commit?" → "0" = yes
@@ -189,10 +195,11 @@ function setupHappyPathExecSync() {
 }

 /**
- * Same as setupHappyPathExecSync but makes -X theirs merge fail.
- * Use this for tests that expect the merge to throw after 3 AI attempts fail.
+ * Same as setupHappyPathExecSync but makes the final fallback merge fail
+ * (both `-X theirs` and `-X ours`). Use this for tests that expect the merge
+ * to throw after 3 attempts fail.
 */
-function setupFailingTheirsStrategy() {
+function setupFailingFallbackStrategy() {
  mockedExecSync.mockImplementation((cmd: any) => {
    const cmdStr = String(cmd);
    if (cmdStr.includes("rev-parse --verify")) return Buffer.from("abc123");
@@ -201,9 +208,9 @@ function setupFailingTheirsStrategy() {
    if (cmdStr.includes("merge-base")) return Buffer.from("abc123");
    if (cmdStr.includes("git diff") && cmdStr.includes("--stat")) return "1 file changed" as any;
    if (cmdStr.includes("merge --squash")) return Buffer.from("");
-    // -X theirs should fail for these tests (they expect merge to throw)
-    if (cmdStr.includes("merge -X theirs --squash")) {
-      const err = new Error("fatal: git merge -X theirs failed with unresolved conflicts");
+    // -X theirs / -X ours should fail for these tests (they expect merge to throw)
+    if (cmdStr.includes("merge -X theirs --squash") || cmdStr.includes("merge -X ours --squash")) {
+      const err = new Error("fatal: git merge -X fallback failed with unresolved conflicts");
      err.name = "ExecSyncError";
      throw err;
    }
@@ -218,6 +225,9 @@ function setupFailingTheirsStrategy() {
  });
 }

+/** @deprecated Renamed to setupFailingFallbackStrategy. */
+const setupFailingTheirsStrategy = setupFailingFallbackStrategy;
+
 describe("findWorktreeUser", () => {
  it("returns null when no other task uses the worktree", async () => {
    const store = createMockStore({}, [
@@ -246,6 +256,140 @@ describe("findWorktreeUser", () => {
  });
 });

+describe("aiMergeTask pre-merge fetch + fast-forward (smart strategies)", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockedExistsSync.mockReturnValue(true);
+    mockedCreateFnAgent.mockResolvedValue({
+      session: {
+        prompt: vi.fn().mockResolvedValue(undefined),
+        dispose: vi.fn(),
+      },
+    } as any);
+  });
+
+  function setupSyncMock({
+    behind,
+    ahead,
+    fetchFails = false,
+  }: {
+    behind: number;
+    ahead: number;
+    fetchFails?: boolean;
+  }) {
+    let fetchCalled = false;
+    let ffCalled = false;
+    mockedExecSync.mockImplementation((cmd: any) => {
+      const cmdStr = String(cmd);
+      if (cmdStr.includes("rev-parse --abbrev-ref HEAD")) return "main" as any;
+      if (cmdStr.includes("rev-parse --verify")) return Buffer.from("abc123");
+      if (cmdStr === "git rev-parse HEAD" || cmdStr.startsWith("git rev-parse HEAD ")) return "mergedcommit123";
+      if (cmdStr.includes("rev-list --left-right --count")) {
+        return `${behind}\t${ahead}` as any;
+      }
+      if (cmdStr.includes("git fetch origin")) {
+        fetchCalled = true;
+        if (fetchFails) throw new Error("fatal: unable to access remote");
+        return Buffer.from("");
+      }
+      if (cmdStr.includes("merge --ff-only")) {
+        ffCalled = true;
+        return Buffer.from("");
+      }
+      if (cmdStr.includes("git log")) return "- feat: something" as any;
+      if (cmdStr.includes("merge-base")) return Buffer.from("abc123");
+      if (cmdStr.includes("git diff") && cmdStr.includes("--stat")) return "1 file changed" as any;
+      if (cmdStr.includes("merge --squash")) return Buffer.from("");
+      if (cmdStr.includes("merge -X theirs --squash") || cmdStr.includes("merge -X ours --squash")) {
+        return Buffer.from("");
+      }
+      if (cmdStr.includes("diff --cached --quiet")) return "1" as any;
+      if (cmdStr.includes("diff --cached")) return "0" as any;
+      if (cmdStr.includes("show --shortstat")) return "3 files changed, 10 insertions(+), 2 deletions(-)" as any;
+      if (cmdStr.includes("branch -d") || cmdStr.includes("branch -D")) return Buffer.from("");
+      if (cmdStr.includes("worktree remove")) return Buffer.from("");
+      return Buffer.from("");
+    });
+    return {
+      get fetchCalled() { return fetchCalled; },
+      get ffCalled() { return ffCalled; },
+    };
+  }
+
+  it("fast-forwards local main when origin is strictly ahead (default smart-prefer-main)", 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],
+    );
+    const probe = setupSyncMock({ behind: 2, ahead: 0 });
+
+    await aiMergeTask(store, "/tmp/root", "FN-050");
+
+    expect(probe.fetchCalled).toBe(true);
+    expect(probe.ffCalled).toBe(true);
+  });
+
+  it("skips fast-forward when local main has unpushed commits (divergent)", 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],
+    );
+    const probe = setupSyncMock({ behind: 1, ahead: 1 });
+
+    await aiMergeTask(store, "/tmp/root", "FN-050");
+
+    expect(probe.fetchCalled).toBe(true);
+    expect(probe.ffCalled).toBe(false);
+  });
+
+  it("continues merge when fetch fails (graceful degrade)", 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],
+    );
+    const probe = setupSyncMock({ behind: 0, ahead: 0, fetchFails: true });
+
+    const result = await aiMergeTask(store, "/tmp/root", "FN-050");
+
+    expect(probe.fetchCalled).toBe(true);
+    expect(probe.ffCalled).toBe(false);
+    expect(result.merged).toBe(true);
+  });
+
+  it("does not fetch for ai-only strategy", 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: "ai-only",
+    });
+    const probe = setupSyncMock({ behind: 5, ahead: 0 });
+
+    await aiMergeTask(store, "/tmp/root", "FN-050");
+
+    expect(probe.fetchCalled).toBe(false);
+  });
+
+  it("normalizes legacy 'smart' setting and still fetches", 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" as any,
+    });
+    const probe = setupSyncMock({ behind: 1, ahead: 0 });
+
+    await aiMergeTask(store, "/tmp/root", "FN-050");
+
+    expect(probe.fetchCalled).toBe(true);
+    expect(probe.ffCalled).toBe(true);
+  });
+});
+
 describe("aiMergeTask abort handling", () => {
  beforeEach(() => {
    vi.clearAllMocks();
@@ -1945,6 +2089,12 @@ describe("aiMergeTask — retry logic with escalating strategies", () => {
      { id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050" },
      [{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050", column: "in-review" } as Task],
    );
+    // Pin the strategy: default is now "smart-prefer-main" (-X ours), but
+    // this test specifically exercises the -X theirs fallback path.
+    (store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
+      ...DEFAULT_SETTINGS,
+      mergeConflictStrategy: "smart-prefer-branch",
+    });

    let squashCallCount = 0;
    let theirsCallCount = 0;
@@ -2297,7 +2447,7 @@ describe("aiMergeTask — retry logic with escalating strategies", () => {
    const resetFailureMessage = "retry cleanup reset failed";
    let mergeSquashCalls = 0;
    let resetCalls = 0;
-    let usedTheirsStrategy = false;
+    let usedFallbackStrategy = false;

    mockedExecSync.mockImplementation((cmd: any) => {
      const cmdStr = String(cmd);
@@ -2316,13 +2466,13 @@ describe("aiMergeTask — retry logic with escalating strategies", () => {
        return Buffer.from("");
      }

-      if (cmdStr.includes("merge -X theirs --squash")) {
-        usedTheirsStrategy = true;
+      if (cmdStr.includes("merge -X theirs --squash") || cmdStr.includes("merge -X ours --squash")) {
+        usedFallbackStrategy = true;
        return Buffer.from("");
      }

      if (cmdStr.includes("diff --name-only --diff-filter=U")) {
-        if (!usedTheirsStrategy && mergeSquashCalls === 2) {
+        if (!usedFallbackStrategy && mergeSquashCalls === 2) {
          return "src/complex.ts\n";
        }
        return "";
@@ -2519,7 +2669,7 @@ describe("aiMergeTask — reset cleanup failure diagnostics", () => {
        return Buffer.from("");
      }

-      if (cmdStr.includes("merge -X theirs --squash")) {
+      if (cmdStr.includes("merge -X theirs --squash") || cmdStr.includes("merge -X ours --squash")) {
        throw new Error("Merge conflict");
      }

--- a/packages/engine/src/merger.ts
+++ b/packages/engine/src/merger.ts
@@ -133,7 +133,7 @@ async function execWithProcessGroup(
 }
 import { existsSync } from "node:fs";
 import { join } from "node:path";
-import { getTaskMergeBlocker, type TaskStore, type MergeResult, type MergeDetails, type WorkflowStep, type WorkflowStepResult, type Settings, type AgentPromptsConfig, type MergeConflictStrategy } from "@fusion/core";
+import { getTaskMergeBlocker, normalizeMergeConflictStrategy, type TaskStore, type MergeResult, type MergeDetails, type WorkflowStep, type WorkflowStepResult, type Settings, type AgentPromptsConfig, type CanonicalMergeConflictStrategy } from "@fusion/core";
 import { resolveAgentPrompt } from "@fusion/core";
 import { describeModel, promptWithFallback } from "./pi.js";
 import { accumulateSessionTokenUsage } from "./session-token-usage.js";
@@ -2170,7 +2170,17 @@ export async function aiMergeTask(
  const includeTaskId = settings.includeTaskIdInCommit !== false;
  // Support both setting names: smartConflictResolution (new) and autoResolveConflicts (legacy)
  const smartConflictResolution = (settings.smartConflictResolution ?? settings.autoResolveConflicts) !== false;
-  const mergeConflictStrategy: NonNullable<MergeConflictStrategy> = settings.mergeConflictStrategy ?? "smart";
+  const mergeConflictStrategy: CanonicalMergeConflictStrategy = normalizeMergeConflictStrategy(
+    settings.mergeConflictStrategy,
+  );
+
+  // Pre-merge sync: for the smart strategies, opportunistically fast-forward
+  // local main from origin so a freshly-pushed sibling commit isn't clobbered
+  // by `-X ours`/`-X theirs` falling back to a stale base. Best-effort: any
+  // failure (no remote, network down, divergent local) logs and continues.
+  if (mergeConflictStrategy === "smart-prefer-main" || mergeConflictStrategy === "smart-prefer-branch") {
+    await tryFastForwardFromOrigin(rootDir, taskId);
+  }

  // 3. Check branch exists
  try {
@@ -2779,7 +2789,7 @@ export async function aiMergeTask(
    merged = await mergeAttempt(2);
  }

-  // Attempt 3: -X theirs (smart) or -X ours (prefer-main) fallback.
+  // Attempt 3: -X theirs (smart-prefer-branch) or -X ours (smart-prefer-main) fallback.
  // Skipped for "ai-only" (no silent side-pick) and "abort" (one shot only).
  if (
    !merged
@@ -3012,6 +3022,61 @@ export async function aiMergeTask(
  return result;
 }

+/** Best-effort `git fetch origin <currentBranch>` + fast-forward of local
+ *  HEAD when origin is strictly ahead. Returns silently on any failure
+ *  (no remote configured, network down, divergent local commits, etc.).
+ *  Only called for the smart strategies, which want to avoid resolving a
+ *  conflict against a stale local base. */
+async function tryFastForwardFromOrigin(rootDir: string, taskId: string): Promise<void> {
+  let currentBranch: string;
+  try {
+    currentBranch = execSync("git rev-parse --abbrev-ref HEAD", {
+      cwd: rootDir,
+      encoding: "utf-8",
+      stdio: "pipe",
+    }).trim();
+  } catch {
+    return;
+  }
+  if (!currentBranch || currentBranch === "HEAD") return;
+
+  try {
+    await execAsync(`git fetch origin "${currentBranch}"`, { cwd: rootDir });
+  } catch (err) {
+    mergerLog.log(`${taskId}: pre-merge fetch failed (continuing): ${err instanceof Error ? err.message : String(err)}`);
+    return;
+  }
+
+  // Detect divergence: local must be strictly behind remote (no local-only commits).
+  let behind = 0;
+  let ahead = 0;
+  try {
+    const counts = execSync(`git rev-list --left-right --count "origin/${currentBranch}...HEAD"`, {
+      cwd: rootDir,
+      encoding: "utf-8",
+      stdio: "pipe",
+    }).trim();
+    const [b, a] = counts.split(/\s+/).map((n) => Number.parseInt(n, 10) || 0);
+    behind = b;
+    ahead = a;
+  } catch {
+    return;
+  }
+
+  if (behind === 0) return; // already up to date
+  if (ahead > 0) {
+    mergerLog.log(`${taskId}: local ${currentBranch} has ${ahead} unpushed commit(s); skipping fast-forward`);
+    return;
+  }
+
+  try {
+    await execAsync(`git merge --ff-only "origin/${currentBranch}"`, { cwd: rootDir });
+    mergerLog.log(`${taskId}: fast-forwarded ${currentBranch} by ${behind} commit(s) from origin`);
+  } catch (err) {
+    mergerLog.log(`${taskId}: fast-forward failed (continuing): ${err instanceof Error ? err.message : String(err)}`);
+  }
+}
+
 /** Get the resolution strategy based on attempt number and settings.
 *  `mergeConflictStrategy` controls the FALLBACK on attempt 3 (and gates the
 *  whole cascade on "abort"); attempts 1–2 always try AI then auto-resolve so
@@ -3019,7 +3084,7 @@ export async function aiMergeTask(
 function getResolutionStrategy(
  attemptNum: 1 | 2 | 3,
  smartConflictResolution: boolean,
-  mergeConflictStrategy: NonNullable<MergeConflictStrategy> = "smart",
+  mergeConflictStrategy: CanonicalMergeConflictStrategy = "smart-prefer-main",
 ): MergeResult["resolutionStrategy"] {
  if (!smartConflictResolution || attemptNum === 1) {
    return "ai";
@@ -3031,11 +3096,11 @@ function getResolutionStrategy(
  switch (mergeConflictStrategy) {
    case "ai-only":
      return "ai";
-    case "prefer-main":
+    case "smart-prefer-main":
      return "ours";
    case "abort":
      return "abort";
-    case "smart":
+    case "smart-prefer-branch":
    default:
      return "theirs";
  }
@@ -3071,7 +3136,7 @@ interface MergeAttemptParams {
  diffStat: string;
  includeTaskId: boolean;
  smartConflictResolution: boolean;
-  mergeConflictStrategy: NonNullable<MergeConflictStrategy>;
+  mergeConflictStrategy: CanonicalMergeConflictStrategy;
  attemptNum: 1 | 2 | 3;
  options: MergerOptions;
  result: MergeResult;
@@ -3123,10 +3188,9 @@ async function executeMergeAttempt(

  // Attempt 3: dispatch on the configured fallback strategy.
  // Note: "ai-only" and "abort" are filtered out by the mergeAttempt cascade
-  // before reaching here — only "smart" (theirs) and "prefer-main" (ours)
-  // legitimately run attempt 3.
+  // before reaching here — only the two smart variants legitimately run attempt 3.
  if (attemptNum === 3) {
-    if (params.mergeConflictStrategy === "prefer-main") {
+    if (params.mergeConflictStrategy === "smart-prefer-main") {
      return attemptWithSideStrategy(params, "ours");
    }
    return attemptWithSideStrategy(params, "theirs");
@@ -3404,8 +3468,8 @@ async function executeMergeAttempt(
 /**
 * Attempt 3: Use git merge -X{theirs,ours} --squash strategy.
 * Side controls which version wins on conflicts:
- *   - "theirs" — the task branch wins (default fallback)
- *   - "ours" — the main branch wins (used by mergeConflictStrategy="prefer-main")
+ *   - "theirs" — the task branch wins (mergeConflictStrategy="smart-prefer-branch")
+ *   - "ours" — the main branch wins (mergeConflictStrategy="smart-prefer-main", default)
 */
 async function attemptWithSideStrategy(
  params: MergeAttemptParams,