fix(engine): refuse to treat non-conflict squash failures as no-op merges
Attempt 2 of the merge cascade caught any git merge --squash failure into mergeExitedWithConflicts=true. If the failure was non-conflict (pre-commit hook rejection, IO error, locked repo) and produced no U files, the code fell into the "all conflicts auto-resolved" branch with empty classified arrays, ran deterministic verification on pre-merge HEAD, and returned true — recording merge metadata for a merge that never happened. Distinguish "exit code 1 with U files" (recoverable) from "any other failure" (real). When a real failure surfaces with no conflicts, raise a sentinel MergeNonConflictError that the outer mergeAttempt catch propagates without retrying — retrying just re-runs the same broken command. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2103,6 +2103,42 @@ describe("aiMergeTask — retry logic with escalating strategies", () => {
|
||||
expect(agentCallCount).toBe(1);
|
||||
});
|
||||
|
||||
it("attempt 2 throws when squash fails for a non-conflict reason (no U files)", async () => {
|
||||
// Regression: previously any squash error was treated as conflicts. If
|
||||
// the failure was non-conflict (hook, IO, lock) and no U files existed,
|
||||
// the cascade fell into "all conflicts auto-resolved" and returned true,
|
||||
// recording merge metadata for a merge that never happened.
|
||||
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],
|
||||
);
|
||||
|
||||
let mergeCallCount = 0;
|
||||
mockedExecSync.mockImplementation((cmd: any) => {
|
||||
const cmdStr = String(cmd);
|
||||
if (cmdStr.includes("rev-parse")) return Buffer.from("abc123");
|
||||
if (cmdStr.includes("git log")) return "- feat: something";
|
||||
if (cmdStr.includes("merge-base")) return Buffer.from("abc123");
|
||||
if (cmdStr.includes("--stat")) return "1 file changed";
|
||||
if (cmdStr.includes("merge --squash")) {
|
||||
mergeCallCount++;
|
||||
// Simulate a non-conflict failure on every squash attempt
|
||||
// (e.g. pre-commit hook rejected, repo locked).
|
||||
throw new Error("fatal: pre-commit hook returned non-zero status");
|
||||
}
|
||||
// Critical: no conflicted files surface
|
||||
if (cmdStr.includes("diff --name-only --diff-filter=U")) return "";
|
||||
if (cmdStr.includes("diff --cached --quiet")) return "1";
|
||||
if (cmdStr.includes("reset --merge")) return Buffer.from("");
|
||||
return Buffer.from("");
|
||||
});
|
||||
|
||||
await expect(aiMergeTask(store, "/tmp/root", "FN-050")).rejects.toThrow(
|
||||
/failed without producing conflicts/i,
|
||||
);
|
||||
expect(mergeCallCount).toBeGreaterThanOrEqual(1);
|
||||
});
|
||||
|
||||
it("attempt 1 fails, attempt 2 auto-resolves lock files: sets resolutionStrategy to 'auto-resolve'", async () => {
|
||||
const store = createMockStore(
|
||||
{ id: "FN-050", worktree: "/tmp/root/.worktrees/KB-050" },
|
||||
|
||||
@@ -2834,6 +2834,15 @@ export async function aiMergeTask(
|
||||
throw error; // No retries left — fatal
|
||||
}
|
||||
|
||||
// Non-conflict squash failure: don't retry — the underlying cause
|
||||
// (broken hook, IO error, locked repo) won't fix itself by retrying.
|
||||
if (error.name === "MergeNonConflictError") {
|
||||
try {
|
||||
execSync("git reset --merge", { cwd: rootDir, stdio: "pipe" });
|
||||
} catch { /* best-effort */ }
|
||||
throw error;
|
||||
}
|
||||
|
||||
// Clean up on error before potentially rethrowing or retrying
|
||||
if (attemptNum < 3 && smartConflictResolution) {
|
||||
mergerLog.log(`${taskId}: attempt ${attemptNum} error, cleaning up for retry...`);
|
||||
@@ -3281,7 +3290,7 @@ async function executeMergeAttempt(
|
||||
// First, do a standard merge to get conflicts
|
||||
// Note: git merge --squash exits with code 1 when conflicts exist
|
||||
// This is expected - we catch it and proceed with auto-resolution
|
||||
let mergeExitedWithConflicts = false;
|
||||
let mergeError: unknown;
|
||||
try {
|
||||
await execAsync(`git merge --squash "${branch}"`, {
|
||||
cwd: rootDir,
|
||||
@@ -3289,12 +3298,30 @@ async function executeMergeAttempt(
|
||||
throwIfAborted(options.signal, taskId);
|
||||
} catch (error: unknown) {
|
||||
rethrowIfMergeAborted(error);
|
||||
// Merge exits with code 1 when conflicts exist - this is expected
|
||||
mergeExitedWithConflicts = true;
|
||||
// Capture the error so we can distinguish "exit code 1 with conflicts"
|
||||
// (expected, recoverable) from "any other failure" (hooks, IO, locks).
|
||||
mergeError = error;
|
||||
}
|
||||
|
||||
// Use new API: get conflicted files and classify them
|
||||
const conflictedFiles = await getConflictedFiles(rootDir);
|
||||
|
||||
// Don't paper over non-conflict failures: if the merge errored AND no
|
||||
// U files exist, the failure was something other than a merge conflict
|
||||
// (pre-commit hook, disk error, repo lock, etc.). Returning success
|
||||
// here would store merge metadata for a merge that never happened.
|
||||
// The outer mergeAttempt catch propagates this sentinel name without
|
||||
// retrying (retrying would just re-run the same broken command).
|
||||
if (mergeError && conflictedFiles.length === 0) {
|
||||
const cause = mergeError instanceof Error ? mergeError.message : String(mergeError);
|
||||
const fatal = new Error(
|
||||
`${taskId}: git merge --squash failed without producing conflicts ` +
|
||||
`(${cause}) — refusing to treat as a no-op merge.`,
|
||||
);
|
||||
fatal.name = "MergeNonConflictError";
|
||||
throw fatal;
|
||||
}
|
||||
const mergeExitedWithConflicts = mergeError !== undefined;
|
||||
if (conflictedFiles.length > 0 || mergeExitedWithConflicts) {
|
||||
// Classify each conflicted file
|
||||
const classified: { file: string; type: ConflictType }[] = [];
|
||||
|
||||
Reference in New Issue
Block a user