diff --git a/.changeset/fn-062-ai-merge-review-livelock.md b/.changeset/fn-062-ai-merge-review-livelock.md new file mode 100644 index 0000000000..168fd71936 --- /dev/null +++ b/.changeset/fn-062-ai-merge-review-livelock.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Prevent blocked AI merge reviews from retrying as git conflicts. +category: fix +dev: Reconciles and bounds durable squash-review findings across corrective passes. diff --git a/packages/engine/src/__tests__/merge-error-recovery.test.ts b/packages/engine/src/__tests__/merge-error-recovery.test.ts index e16e9c1159..2537fff336 100644 --- a/packages/engine/src/__tests__/merge-error-recovery.test.ts +++ b/packages/engine/src/__tests__/merge-error-recovery.test.ts @@ -70,7 +70,7 @@ vi.mock("../runtimes/in-process-runtime.js", () => ({ import { ProjectEngine } from "../project-engine.js"; import { runtimeLog } from "../logger.js"; import { VerificationError } from "../merger.js"; -import { runAiMerge } from "../merge/merger-ai.js"; +import { AiMergeBlockedError, runAiMerge } from "../merge/merger-ai.js"; type MockTask = { id: string; @@ -362,6 +362,33 @@ describe("ProjectEngine merge error recovery", () => { expect(hasErrorLog(errorSpy, "failed to bounce")).toBe(false); }); + it("parks typed AI review blocks containing conflicts without retrying or bouncing", async () => { + vi.useFakeTimers(); + const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout"); + const store = makeStore({ + tasks: [makeTask({ mergeRetries: 0 }), makeTask({ mergeRetries: 3 })], + }); + vi.mocked(runAiMerge).mockRejectedValueOnce( + new AiMergeBlockedError(TASK_ID, ["review assertions conflicts with builtin settings"]), + ); + + const engine = createEngine(store); + await runMergeCycle(engine); + + expect(store.updateTask).toHaveBeenCalledWith(TASK_ID, { + status: "failed", + mergeRetries: 3, + error: "AI merge review blocked landing; operator intervention is required.", + }); + expect(store.updateTask).not.toHaveBeenCalledWith(TASK_ID, expect.objectContaining({ status: null })); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.logEntry).not.toHaveBeenCalledWith(TASK_ID, expect.any(String), "MergeConflictBounce"); + expect(setTimeoutSpy).not.toHaveBeenCalledWith(expect.any(Function), expect.any(Number)); + const canMergeTask = (engine as unknown as { canMergeTask: (task: MockTask, retries: number) => boolean }).canMergeTask.bind(engine); + expect(canMergeTask(makeTask({ status: "failed", updatedAt: new Date(0).toISOString() }), 3)).toBe(false); + vi.useRealTimers(); + }); + it("logs when bouncing fails after conflict retries are exhausted", async () => { const store = makeStore({ tasks: [makeTask({ mergeRetries: 2 }), makeTask({ mergeRetries: 3 })], diff --git a/packages/engine/src/__tests__/merger-ai-prompts.test.ts b/packages/engine/src/__tests__/merger-ai-prompts.test.ts index 7c0defa2fd..e1974fbd21 100644 --- a/packages/engine/src/__tests__/merger-ai-prompts.test.ts +++ b/packages/engine/src/__tests__/merger-ai-prompts.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { REVIEW_VERDICT_MARKER, + RESOLVED_PRIOR_FINDINGS_MARKER, buildMergeSystemPrompt, buildReviewSystemPrompt, parseReviewVerdict, @@ -13,6 +14,7 @@ describe("merger-ai prompt/verdict re-exports", () => { verdict: "reject", reasons: ["reviewer produced no output"], severity: "blocking", + resolvedPriorReasons: [], }); }); @@ -23,6 +25,7 @@ describe("merger-ai prompt/verdict re-exports", () => { `reviewer did not emit a "${REVIEW_VERDICT_MARKER} approve|reject" line`, ], severity: "blocking", + resolvedPriorReasons: [], }); }); @@ -35,6 +38,7 @@ describe("merger-ai prompt/verdict re-exports", () => { verdict: "reject", reasons: ["dropped a conflict hunk"], severity: "blocking", + resolvedPriorReasons: [], }); }); @@ -47,6 +51,7 @@ describe("merger-ai prompt/verdict re-exports", () => { verdict: "reject", reasons: ["commit message is vague"], severity: "advisory", + resolvedPriorReasons: [], }); }); @@ -56,6 +61,7 @@ describe("merger-ai prompt/verdict re-exports", () => { ).toEqual({ verdict: "approve", reasons: [], + resolvedPriorReasons: [], }); }); @@ -72,6 +78,21 @@ describe("merger-ai prompt/verdict re-exports", () => { "skipped docs update", ], severity: "blocking", + resolvedPriorReasons: [], + }); + }); + + it("parses explicitly acknowledged resolved prior findings", () => { + expect(parseReviewVerdict([ + `${RESOLVED_PRIOR_FINDINGS_MARKER}`, + "- Missing generated types.", + "- Drops task export", + "SEVERITY: blocking", + "- newly found defect", + `${REVIEW_VERDICT_MARKER} reject`, + ].join("\n"))).toMatchObject({ + resolvedPriorReasons: ["Missing generated types.", "Drops task export"], + reasons: ["newly found defect"], }); }); @@ -82,6 +103,8 @@ describe("merger-ai prompt/verdict re-exports", () => { ); expect(buildReviewSystemPrompt()).toContain(REVIEW_VERDICT_MARKER); expect(buildReviewSystemPrompt()).toContain("Do NOT edit, stage, commit"); + expect(buildReviewSystemPrompt()).toContain("Whole-tree semantic"); + expect(buildReviewSystemPrompt()).toContain("never a blocking veto"); }); }); @@ -191,7 +214,7 @@ describe("parseReviewVerdict — reason recovery (FN-8004)", () => { const result = parseReviewVerdict( `Everything checks out.\n${REVIEW_VERDICT_MARKER} approve` ); - expect(result).toEqual({ verdict: "approve", reasons: [] }); + expect(result).toEqual({ verdict: "approve", reasons: [], resolvedPriorReasons: [] }); }); it("gives the reviewer an unambiguous, self-consistent ordering instruction", () => { diff --git a/packages/engine/src/__tests__/merger-ai.test.ts b/packages/engine/src/__tests__/merger-ai.test.ts index 4da4466d5d..e7e6ccac30 100644 --- a/packages/engine/src/__tests__/merger-ai.test.ts +++ b/packages/engine/src/__tests__/merger-ai.test.ts @@ -31,6 +31,7 @@ import { buildReviewPrompt, buildReviewSystemPrompt, REVIEW_VERDICT_MARKER, + RESOLVED_PRIOR_FINDINGS_MARKER, AiMergeBlockedError, } from "../merge/merger-ai.js"; import { EXECUTOR_FAILED_INCOMPLETE_REASON } from "../overseer/planner-overseer.js"; @@ -134,16 +135,16 @@ function realMergeAgent(branch: string) { describe("parseReviewVerdict", () => { it("approves cleanly", () => { - expect(parseReviewVerdict("ok\nREVIEW_VERDICT: approve")).toEqual({ verdict: "approve", reasons: [] }); + expect(parseReviewVerdict("ok\nREVIEW_VERDICT: approve")).toEqual({ verdict: "approve", reasons: [], resolvedPriorReasons: [] }); }); it("rejects with blocking severity by default", () => { expect(parseReviewVerdict("REVIEW_VERDICT: reject\n- dropped a hunk")).toEqual({ - verdict: "reject", severity: "blocking", reasons: ["dropped a hunk"], + verdict: "reject", severity: "blocking", reasons: ["dropped a hunk"], resolvedPriorReasons: [], }); }); it("parses advisory severity and drops the SEVERITY line from reasons", () => { expect(parseReviewVerdict("REVIEW_VERDICT: reject\nSEVERITY: advisory\n- nit")).toEqual({ - verdict: "reject", severity: "advisory", reasons: ["nit"], + verdict: "reject", severity: "advisory", reasons: ["nit"], resolvedPriorReasons: [], }); }); it("fails safe to blocking on empty/garbled output", () => { @@ -262,7 +263,7 @@ describe("runAiMerge", () => { git(dir, "add concurrent.txt"); git(dir, "commit -q -m 'main: concurrent advance'"); } - return "REVIEW_VERDICT: approve"; + return `${RESOLVED_PRIOR_FINDINGS_MARKER} ${blocker}\nREVIEW_VERDICT: approve`; }); const result = await runAiMerge(store, dir, "FN-1", { manual: true }, { @@ -273,7 +274,7 @@ describe("runAiMerge", () => { expect(result.merged).toBe(true); expect(reviewPrompts).toHaveLength(3); expect(reviewPrompts[2]).toContain(blocker); - expect(reviewPrompts[2]).toContain("complete resulting tree"); + expect(reviewPrompts[2]).toContain("squash's fidelity"); }); it("rechecks a durable blocker when a later merge retry starts", async () => { @@ -285,7 +286,7 @@ describe("runAiMerge", () => { timestamp: new Date().toISOString(), }], }); - const reviewAgent = vi.fn(async () => "REVIEW_VERDICT: approve"); + const reviewAgent = vi.fn(async () => `${RESOLVED_PRIOR_FINDINGS_MARKER} ${blocker}\nREVIEW_VERDICT: approve`); await runAiMerge(store, dir, "FN-1", { manual: true }, { mergeAgent: realMergeAgent("fusion/fn-1"), @@ -293,7 +294,60 @@ describe("runAiMerge", () => { }); expect(reviewAgent.mock.calls[0]?.[1]).toContain(blocker); - expect(reviewAgent.mock.calls[0]?.[1]).toContain("complete resulting tree"); + expect(reviewAgent.mock.calls[0]?.[1]).toContain("squash's fidelity"); + }); + + it("reconciles resolved findings and gives corrective merges only new findings", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + const { store } = makeStore(dir, {}, { merger: { mode: "ai", maxReviewPasses: 2 } }); + const prior = "Missing generated types."; + const introduced = "Drops the task branch export."; + const mergePrompts: string[] = []; + const reviewPrompts: string[] = []; + let reviewCount = 0; + + await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent: async (cwd, prompt) => { + mergePrompts.push(prompt); + await realMergeAgent("fusion/fn-1")(cwd, prompt); + }, + reviewAgent: async (_cwd, prompt) => { + reviewPrompts.push(prompt); + reviewCount++; + if (reviewCount === 1) return `${prior}\nSEVERITY: blocking\nREVIEW_VERDICT: reject`; + if (reviewCount === 2) return `${RESOLVED_PRIOR_FINDINGS_MARKER} missing generated types\n\n- ${introduced}\nSEVERITY: blocking\nREVIEW_VERDICT: reject`; + return `${RESOLVED_PRIOR_FINDINGS_MARKER} ${introduced}\nREVIEW_VERDICT: approve`; + }, + }); + + expect(reviewPrompts[1]).toContain(prior); + expect(reviewPrompts[2]).toContain(introduced); + expect(reviewPrompts[2]).not.toContain(prior); + expect(mergePrompts[1]).toContain(prior); + expect(mergePrompts[2]).toContain(introduced); + expect(mergePrompts[2]).not.toContain(prior); + }); + + it("bounds and normalizes recovered blocking finding contracts", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + const reasons = Array.from({ length: 12 }, (_, index) => `finding ${index}`); + const { store } = makeStore(dir, { + log: [{ + action: `AI merge BLOCKED after 2 corrective pass(es) — unresolved correctness concern: Finding 0; finding 0!!!; ${reasons.slice(1).join("; ")}`, + timestamp: new Date().toISOString(), + }], + }); + const reviewAgent = vi.fn(async () => `${RESOLVED_PRIOR_FINDINGS_MARKER}\n${reasons.slice(0, 8).map((reason) => `- ${reason}`).join("\n")}\nREVIEW_VERDICT: approve`); + + await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent: realMergeAgent("fusion/fn-1"), + reviewAgent, + }); + + const recoveredPrompt = reviewAgent.mock.calls[0]?.[1] ?? ""; + expect(recoveredPrompt).toContain("Finding 0"); + expect(recoveredPrompt).not.toContain("finding 8"); + expect((recoveredPrompt.match(/^ - finding/igm) ?? []).length).toBeLessThanOrEqual(8); }); it("reviews a durable blocker even when the retried branch has zero commits ahead", async () => { @@ -306,7 +360,7 @@ describe("runAiMerge", () => { timestamp: new Date().toISOString(), }], }); - const reviewAgent = vi.fn(async () => "REVIEW_VERDICT: approve"); + const reviewAgent = vi.fn(async () => `${RESOLVED_PRIOR_FINDINGS_MARKER} ${blocker}\nREVIEW_VERDICT: approve`); await runAiMerge(store, dir, "FN-1", { manual: true }, { mergeAgent: vi.fn(async () => { /* zero-ahead corrective review */ }), @@ -333,7 +387,7 @@ describe("runAiMerge", () => { }); const reviewAgent = vi.fn() .mockResolvedValueOnce(`${blocker}\nSEVERITY: blocking\nREVIEW_VERDICT: reject`) - .mockResolvedValueOnce("REVIEW_VERDICT: approve"); + .mockResolvedValueOnce(`${RESOLVED_PRIOR_FINDINGS_MARKER} ${blocker}\nREVIEW_VERDICT: approve`); const result = await runAiMerge(store, dir, "FN-1", { manual: true }, { mergeAgent, reviewAgent }); @@ -352,7 +406,7 @@ describe("runAiMerge", () => { const reviewAgent = vi.fn() .mockResolvedValueOnce(`${blockerX}\nREVIEW_VERDICT: reject`) .mockResolvedValueOnce(`${blockerY}\nREVIEW_VERDICT: reject`) - .mockResolvedValueOnce("REVIEW_VERDICT: approve"); + .mockResolvedValueOnce(`${RESOLVED_PRIOR_FINDINGS_MARKER} ${blockerX}\n- ${blockerY}\nREVIEW_VERDICT: approve`); await runAiMerge(store, dir, "FN-1", { manual: true }, { mergeAgent: realMergeAgent("fusion/fn-1"), @@ -363,17 +417,17 @@ describe("runAiMerge", () => { expect(reviewAgent.mock.calls[2]?.[1]).toContain(blockerY); }); - it("recovers every blocker from interrupted per-pass rejection logs", async () => { + it("recovers the latest reconciled blocker contract from interrupted logs", async () => { const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); const blockerX = "authorization is bypassed"; const blockerY = "audit metadata is missing"; const { store } = makeStore(dir, { log: [ { action: `AI merge review (pass 1): rejected (blocking) — ${blockerX}` }, - { action: `AI merge review (pass 2): rejected (blocking) — ${blockerY}` }, + { action: `AI merge review (pass 2): rejected (blocking) — ${blockerX}; ${blockerY}` }, ], }); - const reviewAgent = vi.fn(async () => "REVIEW_VERDICT: approve"); + const reviewAgent = vi.fn(async () => `${RESOLVED_PRIOR_FINDINGS_MARKER} ${blockerX}\n- ${blockerY}\nREVIEW_VERDICT: approve`); await runAiMerge(store, dir, "FN-1", { manual: true }, { mergeAgent: realMergeAgent("fusion/fn-1"), diff --git a/packages/engine/src/__tests__/workspace-merger.test.ts b/packages/engine/src/__tests__/workspace-merger.test.ts index 60d4f5c38b..cf707a781d 100644 --- a/packages/engine/src/__tests__/workspace-merger.test.ts +++ b/packages/engine/src/__tests__/workspace-merger.test.ts @@ -30,7 +30,7 @@ import { writeFileSync } from "node:fs"; import path from "node:path"; import type { Task, TaskStore } from "@fusion/core"; import { assertNotWorkspaceTaskMerge } from "@fusion/core"; -import { landWorkspaceTask, runAiMerge } from "../merge/merger-ai.js"; +import { RESOLVED_PRIOR_FINDINGS_MARKER, landWorkspaceTask, runAiMerge } from "../merge/merger-ai.js"; import { createWorkspaceFixture, hasGit, type WorkspaceFixture } from "./_workspace-fixture.js"; const describeIfGit = hasGit ? describe : describe.skip; @@ -217,6 +217,31 @@ describeIfGit("landWorkspaceTask — per-repo merge loop (Phase C U1)", () => { expect(store.emitted.filter((e) => e.event === "task:merged")).toHaveLength(1); }); + it("reuses reconciled review findings for a workspace sub-repository", async () => { + fx = await createWorkspaceFixture(["repo-a"]); + addRepoBranchWithEdit(fx, "repo-a", "a feature\n"); + const store = createStore({ merger: { mode: "ai", maxReviewPasses: 1 } }); + const task = makeTask({ "repo-a": { worktreePath: fx.repoPath("repo-a"), branch: BRANCH } }); + const finding = "drops the workspace task export"; + const reviewPrompts: string[] = []; + let reviews = 0; + + const result = await landWorkspaceTask(store, task, fx.rootDir, {}, { + mergeAgent: squashMergeAgent(BRANCH), + reviewAgent: async (_cwd, prompt) => { + reviewPrompts.push(prompt); + reviews++; + return reviews === 1 + ? `${finding}\nSEVERITY: blocking\nREVIEW_VERDICT: reject` + : `${RESOLVED_PRIOR_FINDINGS_MARKER} ${finding}\nREVIEW_VERDICT: approve`; + }, + }); + + expect(result.allLanded).toBe(true); + expect(reviewPrompts).toHaveLength(2); + expect(reviewPrompts[1]).toContain(finding); + }); + it("per-repo resolution: each repo lands on its OWN origin/HEAD branch (override-stripping)", async () => { fx = await createWorkspaceFixture(["repo-a", "repo-b"]); // Give each repo a different default integration branch via a bare origin whose diff --git a/packages/engine/src/merge/merger-ai-prompts.ts b/packages/engine/src/merge/merger-ai-prompts.ts index 52e49bcf30..91de940352 100644 --- a/packages/engine/src/merge/merger-ai-prompts.ts +++ b/packages/engine/src/merge/merger-ai-prompts.ts @@ -20,11 +20,15 @@ export interface AiMergeReviewVerdict { verdict: "approve" | "reject"; reasons: string[]; severity?: AiMergeReviewSeverity; + /** Prior findings the reviewer explicitly confirmed are fixed in this squash. */ + resolvedPriorReasons: string[]; } export const REVIEW_VERDICT_MARKER = "REVIEW_VERDICT:"; +export const RESOLVED_PRIOR_FINDINGS_MARKER = "RESOLVED_PRIOR_FINDINGS:"; const VERDICT_LINE_RE = /REVIEW_VERDICT:\s*(approve|reject)\b/i; const SEVERITY_LINE_RE = /SEVERITY:\s*(blocking|advisory)\b/i; +const RESOLVED_PRIOR_FINDINGS_LINE_RE = /RESOLVED_PRIOR_FINDINGS:\s*(.*)$/i; /* FNXC:MergerAiReview 2026-07-15-21:30: @@ -49,6 +53,7 @@ export function parseReviewVerdict( verdict: "reject", reasons: ["reviewer produced no output"], severity: "blocking", + resolvedPriorReasons: [], }; const lines = text.split(/\r?\n/); @@ -69,31 +74,62 @@ export function parseReviewVerdict( `reviewer did not emit a "${REVIEW_VERDICT_MARKER} approve|reject" line`, ], severity: "blocking", + resolvedPriorReasons: [], }; } - if (decision === "approve") return { verdict: "approve", reasons: [] }; + const resolvedPriorReasons = extractResolvedPriorReasons(lines); + if (decision === "approve") return { verdict: "approve", reasons: [], resolvedPriorReasons }; const severity: AiMergeReviewSeverity = SEVERITY_LINE_RE.test(text) ? (text.match(SEVERITY_LINE_RE)![1].toLowerCase() as AiMergeReviewSeverity) : "blocking"; + const resolvedKeys = new Set(resolvedPriorReasons.map(normalizeReasonIdentity)); return { verdict: "reject", - reasons: extractRejectReasons(lines, verdictLineIndex), + // Marker bullets are acknowledgements; reconciliation separately promotes + // unknown acknowledgements to new findings instead of dropping them. + reasons: extractRejectReasons(lines, verdictLineIndex) + .filter((reason) => !resolvedKeys.has(normalizeReasonIdentity(reason))), severity, + resolvedPriorReasons, }; } +/** + * Extract only a clearly delimited, bullet-form resolution acknowledgement. + * A missing or malformed marker intentionally yields no resolutions: callers + * retain every prior blocker rather than guessing that reviewer prose cleared it. + */ +function extractResolvedPriorReasons(lines: string[]): string[] { + const markerIndex = lines.findIndex((line) => RESOLVED_PRIOR_FINDINGS_LINE_RE.test(line)); + if (markerIndex === -1) return []; + const inline = lines[markerIndex].match(RESOLVED_PRIOR_FINDINGS_LINE_RE)?.[1].trim(); + const resolved = inline && !/^none\b/i.test(inline) ? [inline] : []; + for (let index = markerIndex + 1; index < lines.length; index++) { + const line = lines[index]; + if (VERDICT_LINE_RE.test(line) || SEVERITY_LINE_RE.test(line) || !line.trim()) break; + if (!/^\s*(?:[-*•]|\d+[.)])\s+/.test(line)) return []; + resolved.push(cleanReasonLine(line)); + } + return resolved; +} + /** Strip bullet/numeric list markers and surrounding whitespace from one line. */ function cleanReasonLine(line: string): string { return line.replace(/^\s*(?:[-*•]|\d+[.)])\s+/, "").trim(); } +function normalizeReasonIdentity(reason: string): string { + return reason.trim().toLocaleLowerCase().replace(/[\s\p{P}]+/gu, " ").trim(); +} + /** Lines that carry no reviewer reasoning and must never be reported as a reason. */ function isNonReasonLine(line: string): boolean { const t = line.trim(); if (!t) return true; if (SEVERITY_LINE_RE.test(t)) return true; if (VERDICT_LINE_RE.test(t)) return true; + if (RESOLVED_PRIOR_FINDINGS_LINE_RE.test(t)) return true; // Markdown scaffolding the reviewer may emit around its analysis. if (/^#{1,6}\s/.test(t)) return true; if (/^(?:-{3,}|={3,}|`{3,})/.test(t)) return true; @@ -277,19 +313,24 @@ export function buildReviewSystemPrompt(): string { "merged into the integration branch and decide whether it is safe to land.", "", "Investigate with read-only commands (git show, git diff, git log, cat, grep).", - "Judge on four axes:", - " 1. Completeness — does the squash contain ALL of the task branch's intended", + "Judge the squash's fidelity on four axes:", + " 1. Branch preservation — does the squash preserve the task branch's", " changes? Flag any hunk silently dropped during conflict resolution.", - " 2. No collateral — does it touch only files within the task's footprint?", - " 3. Conflict soundness — were conflicts resolved coherently (both sides'", - " intent preserved), not by blindly discarding one side?", + " 2. No collateral — does the squash add unrelated changes outside its", + " branch/squash footprint?", + " 3. Conflict soundness — were conflicts resolved coherently (both relevant", + " branch intents retained), not by blindly discarding one side?", " 4. Commit message — read `git show`'s message: the subject must concisely", " and ACCURATELY summarize the actual changes (not vague, not a mere", " restatement of the task title, not misleading). A poor/inaccurate", " message is an ADVISORY concern (it should be rewritten on retry, but", " must not block the merge).", "", - "Bias toward rejection when uncertain.", + "Whole-tree semantic or intent concerns in files untouched by this squash are", + "ADVISORY or follow-up material, never a blocking veto: the merger is not", + "authorized to edit outside branch reconciliation.", + "", + "Bias toward rejection when uncertain about squash fidelity.", "", /* FNXC:MergerAiReview 2026-07-15-21:30: @@ -306,9 +347,9 @@ export function buildReviewSystemPrompt(): string { "vague reason produces a blind retry rather than a fix.", "", `Then add a "SEVERITY:" line:`, - " - SEVERITY: blocking — a correctness problem (dropped/lost task changes,", - " incomplete squash, or a conflict resolution that discards intent). The", - " merge must NOT land if this is unfixable.", + " - SEVERITY: blocking — a squash-fidelity defect: dropped/lost branch", + " changes, unrelated collateral in the squash, or a conflict resolution", + " that discards relevant branch intent. The merge must NOT land if unfixable.", " - SEVERITY: advisory — a quality/style concern that does not risk", " correctness; acceptable to land if unresolved.", "", @@ -349,9 +390,14 @@ export function buildReviewPrompt(input: { if (input.priorReasons && input.priorReasons.length > 0) { lines.push( "", - "A prior pass rejected an earlier attempt for these reasons — confirm they", - "are now resolved in the complete resulting tree, including code outside", - "this squash diff. Do not approve merely because the rebuilt diff became smaller:", + "A prior pass rejected an earlier squash for these findings. Inspect this", + "squash's fidelity, then explicitly identify only findings it resolves:", + ` ${RESOLVED_PRIOR_FINDINGS_MARKER}`, + " - ", + "If none are resolved, write `RESOLVED_PRIOR_FINDINGS: none`. Omit this", + "marker only when you cannot determine resolution; omitted or malformed", + "evidence retains every prior blocker.", + "Do not use whole-tree concerns outside this squash's footprint as blockers:", ...input.priorReasons.map((r) => ` - ${r}`) ); } diff --git a/packages/engine/src/merge/merger-ai.ts b/packages/engine/src/merge/merger-ai.ts index d81e4e06fc..fb8dd23905 100644 --- a/packages/engine/src/merge/merger-ai.ts +++ b/packages/engine/src/merge/merger-ai.ts @@ -206,27 +206,41 @@ function taskHasApprovedAiMergeReview(task: Task | undefined): boolean { ); } -function getOutstandingBlockingMergeReasons(task: Task | undefined): string[] { - const actions = task?.log?.map((entry) => entry.action).filter((action): action is string => typeof action === "string") ?? []; - const reasons: string[] = []; - const addReasons = (value: string): void => { - for (const reason of value.split(/;\s*/).map((part) => part.trim()).filter(Boolean)) { - if (!reasons.includes(reason)) reasons.push(reason); - } - }; - for (let index = actions.length - 1; index >= 0; index--) { - const action = actions[index]; - if (/AI merge: (?:landed|finalized).*task → done/i.test(action)) return []; - if (/AI merge review \(pass \d+\): approved/i.test(action)) return []; - const blocked = action.match(/AI merge BLOCKED .*?unresolved correctness concern:\s*(.+)$/i); - if (blocked?.[1]) { - addReasons(blocked[1]); - return reasons; - } - const rejected = action.match(/AI merge review \(pass \d+\): rejected \(blocking\) —\s*(.+)$/i); - if (rejected?.[1]) addReasons(rejected[1]); +const MAX_BLOCKING_REVIEW_REASONS = 8; +const MAX_BLOCKING_REVIEW_REASON_AGE_MS = 7 * 24 * 60 * 60 * 1000; + +function normalizeBlockingReviewReason(reason: string): string { + return reason.trim().toLocaleLowerCase().replace(/[\s\p{P}]+/gu, " ").trim(); +} + +function boundBlockingReviewReasons(reasons: readonly string[]): string[] { + const seen = new Set(); + const bounded: string[] = []; + for (const reason of reasons) { + const display = reason.trim().replace(/\s+/g, " "); + const key = normalizeBlockingReviewReason(display); + if (!key || seen.has(key)) continue; + seen.add(key); + bounded.push(display); + if (bounded.length === MAX_BLOCKING_REVIEW_REASONS) break; } - return reasons; + return bounded; +} + +function getOutstandingBlockingMergeReasons(task: Task | undefined): string[] { + const entries = task?.log ?? []; + for (let index = entries.length - 1; index >= 0; index--) { + const entry = entries[index]; + const action = entry.action; + if (/AI merge: (?:landed|finalized).*task → done/i.test(action) || /AI merge review \(pass \d+\): approved/i.test(action)) return []; + const current = action.match(/AI merge (?:BLOCKED .*?unresolved correctness concern:\s*|review \(pass \d+\): rejected \(blocking\) —\s*)(.+)$/i); + if (!current?.[1]) continue; + const ageMs = Date.now() - Date.parse(entry.timestamp); + // Invalid legacy timestamps remain recoverable; only provably stale contracts expire. + if (Number.isFinite(ageMs) && ageMs > MAX_BLOCKING_REVIEW_REASON_AGE_MS) return []; + return boundBlockingReviewReasons(current[1].split(/;\s*/)); + } + return []; } function matchesApprovedAiMergeSha(squashSha: string, approvedShas: Set): boolean { @@ -418,6 +432,7 @@ async function ensureCommitTaskMetadata( export { REVIEW_VERDICT_MARKER, + RESOLVED_PRIOR_FINDINGS_MARKER, buildMergePrompt, buildMergeSystemPrompt, buildReviewPrompt, @@ -2618,7 +2633,10 @@ async function mergeAndReview(input: { initialPriorReasons?: string[]; }): Promise<{ squashSha: string | null; priorReasons: string[] }> { const { mergeRoot, branch, integrationBranch, tipSha, taskTitle, includeTaskId, trailers, taskId, maxPasses, mergeAgent, reviewAgent, audit, log, setStatus, store, signal } = input; - let priorReasons = [...(input.initialPriorReasons ?? [])]; + let priorReasons = boundBlockingReviewReasons(input.initialPriorReasons ?? []); + // Only findings introduced by the immediately preceding review are actionable + // merger instructions; older findings remain reviewer-verification evidence. + let correctiveReasons: string[] = []; for (let attempt = 0; ; attempt++) { throwIfAborted(signal, taskId); @@ -2629,13 +2647,13 @@ async function mergeAndReview(input: { if (attempt > 0) { await setStatus("merging"); - await log(`AI merge: corrective re-merge (pass ${attempt}/${maxPasses}) addressing: ${priorReasons.join("; ")}`); + await log(`AI merge: corrective re-merge (pass ${attempt}/${maxPasses}) addressing new findings: ${correctiveReasons.join("; ") || "reviewer reconciliation"}`); } const latestTaskForMergePrompt = await store.getTask(taskId); const mergeUserComments = selectUserCommentsForAgentContext(latestTaskForMergePrompt); await mergeAgent(mergeRoot, buildMergePrompt({ taskId, branch, integrationBranch, tipSha, taskTitle, includeTaskId, trailers, - correctiveReasons: priorReasons.length ? priorReasons : undefined, + correctiveReasons: correctiveReasons.length ? correctiveReasons : undefined, userComments: mergeUserComments, })); @@ -2665,34 +2683,59 @@ async function mergeAndReview(input: { metadata: { taskId, attempt, verdict: verdict.verdict, severity: verdict.severity, reasons: verdict.reasons, squashSha: head }, }); - if (verdict.verdict === "approve") { + /* + FNXC:MergeReviewReconciliation 2026-08-20-02:02: + Review findings are a current squash-fidelity contract, not a permanent transcript. + A reviewer must explicitly acknowledge every supplied blocking finding before approval; + unknown acknowledgements are new findings, never silently discarded. Advisory findings + remain landable and are never carried into the next approval gate. + */ + const priorKeys = new Set(priorReasons.map(normalizeBlockingReviewReason)); + const acknowledgedKeys = new Set( + verdict.resolvedPriorReasons + .map(normalizeBlockingReviewReason) + .filter((key) => priorKeys.has(key)), + ); + const unknownAcknowledgements = verdict.resolvedPriorReasons.filter( + (reason) => !priorKeys.has(normalizeBlockingReviewReason(reason)), + ); + const retainedPriorReasons = priorReasons.filter( + (reason) => !acknowledgedKeys.has(normalizeBlockingReviewReason(reason)), + ); + const acknowledgedReasonKeys = new Set( + verdict.resolvedPriorReasons + .filter((reason) => acknowledgedKeys.has(normalizeBlockingReviewReason(reason))) + .map(normalizeBlockingReviewReason), + ); + const reportedNewReasons = verdict.reasons.filter( + (reason) => !acknowledgedReasonKeys.has(normalizeBlockingReviewReason(reason)), + ); + const newReasons = verdict.verdict === "approve" || verdict.severity === "blocking" + ? [...reportedNewReasons, ...unknownAcknowledgements] + : []; + const unresolvedReasons = boundBlockingReviewReasons([...retainedPriorReasons, ...newReasons]); + const budgetExhausted = attempt >= maxPasses; + + if (verdict.verdict === "approve" && unresolvedReasons.length === 0) { await log(`AI merge review (pass ${attempt + 1}): approved squash ${head}`); return { squashSha: emptyMerge ? null : head, priorReasons }; } - /* - FNXC:MergeReviewBlockers 2026-07-21-21:30: - Every rejected blocker remains part of the corrective contract until a reviewer approves the complete result. Review an empty corrective rebuild instead of treating it as an unreviewed no-op, and accumulate newly discovered blockers so a later pass cannot regress an earlier concern. - - FNXC:MergeReviewBlockers 2026-07-21-21:45: - Persist the accumulated set in every rejection log so crash recovery restores all outstanding concerns rather than only the latest pass. - */ - const unresolvedReasons = [...new Set([...priorReasons, ...verdict.reasons])]; - const budgetExhausted = attempt >= maxPasses; + if (budgetExhausted && (verdict.severity === "blocking" || unresolvedReasons.length > 0)) { + await audit.git({ type: "merge:ai-review-blocked", target: integrationBranch, metadata: { taskId, attempt, reasons: unresolvedReasons } }); + await log(`AI merge BLOCKED after ${attempt} corrective pass(es) — unresolved correctness concern: ${unresolvedReasons.join("; ")}`); + throw new AiMergeBlockedError(taskId, unresolvedReasons); + } if (budgetExhausted) { - if (verdict.severity === "blocking") { - await audit.git({ type: "merge:ai-review-blocked", target: integrationBranch, metadata: { taskId, attempt, reasons: unresolvedReasons } }); - await log(`AI merge BLOCKED after ${attempt} corrective pass(es) — unresolved correctness concern: ${unresolvedReasons.join("; ")}`); - throw new AiMergeBlockedError(taskId, unresolvedReasons); - } - // Advisory: land the squash with the concern logged. - await audit.git({ type: "merge:ai-review-landed-with-concerns", target: integrationBranch, metadata: { taskId, attempt, reasons: unresolvedReasons, squashSha: head } }); - await log(`AI merge: landing with unresolved advisory concern(s): ${unresolvedReasons.join("; ")}`); - return { squashSha: emptyMerge ? null : head, priorReasons: unresolvedReasons }; + // Advisory: land the squash with the concern logged, without poisoning the next pass. + await audit.git({ type: "merge:ai-review-landed-with-concerns", target: integrationBranch, metadata: { taskId, attempt, reasons: reportedNewReasons, squashSha: head } }); + await log(`AI merge: landing with unresolved advisory concern(s): ${reportedNewReasons.join("; ")}`); + return { squashSha: emptyMerge ? null : head, priorReasons: [] }; } priorReasons = unresolvedReasons; - await log(`AI merge review (pass ${attempt + 1}): rejected (${verdict.severity}) — ${unresolvedReasons.join("; ")}`); + correctiveReasons = boundBlockingReviewReasons(newReasons); + await log(`AI merge review (pass ${attempt + 1}): rejected (${verdict.severity ?? "blocking"}) — ${unresolvedReasons.join("; ")}`); } } diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index 06cc31204f..f5ec7b510b 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -84,6 +84,7 @@ import type { RoutineRunner } from "./scheduling/routine-runner.js"; import { sweepStaleAutostashes, VerificationError } from "./merger.js"; import { runAiMerge, + AiMergeBlockedError, landWorkspaceTask, WorkspaceFinalizeBlockedError, WorkspaceMergeDispatchSupersededError, @@ -5141,6 +5142,32 @@ export class ProjectEngine { } if (mergeStrategyOnErr === "direct") { + /* + FNXC:AIMergeReviewRecovery 2026-08-20-02:02: + An exhausted AI review is terminal operator work, not a git conflict. + Its message includes reviewer prose, so classify the typed error before + any text sniffing can turn a natural-language "conflicts" finding into + a retry/bounce/cooldown livelock. + */ + if (err instanceof AiMergeBlockedError) { + try { + await store.updateTask(taskId, { + status: "failed", + mergeRetries: maxAutoMergeRetriesOnErr, + error: "AI merge review blocked landing; operator intervention is required.", + }); + await store.logEntry( + taskId, + `AI merge review exhausted its corrective budget; task parked for operator intervention (${Math.min(err.reasons.length, 8)} current blocking finding(s))`, + "AiMergeReviewBlocked", + ); + } catch (recoveryErr) { + runtimeLog.error( + `Auto-merge: failed to park ${taskId} after AI review block: ${recoveryErr instanceof Error ? recoveryErr.message : String(recoveryErr)}`, + ); + } + continue; + } const isConflictError = errorMsg.includes("conflict") || errorMsg.includes("Conflict");