From 8f7f52784dfbef9536c6966c60fd68a9c136b7da Mon Sep 17 00:00:00 2001 From: flexi767 <96955327+flexi767@users.noreply.github.com> Date: Wed, 22 Jul 2026 02:08:17 +0200 Subject: [PATCH] Fix merge blockers lost during concurrent rebuilds (#2346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What changed - Preserve blocking merge-review reasons when `main` advances and the clean-room squash must be rebuilt. - Recover the latest unresolved blocking reason from task history when a later merge retry starts. - Require reviewers to validate prior blockers against the complete resulting tree, not only a smaller residual diff. - Add regression coverage for both concurrent-main rebuilds and durable retry recovery. ## Why A corrective clean-room squash can be approved and then discarded when `main` advances before landing. The rebuild previously reset the reviewer context, allowing a later, smaller squash to be approved and the task to be finalized as Done without rechecking the original correctness blocker. ## Impact Tasks with unresolved blocking review findings can no longer become Done merely because a concurrent rebuild or later retry loses that review context. ## Validation - `pnpm --filter @fusion/engine exec vitest run src/__tests__/merger-ai.test.ts` — 45 passed - `pnpm --filter @fusion/engine typecheck` - ESLint on the changed merger source files - Changeset format check ## Summary by CodeRabbit * **Bug Fixes** * Merge and review blockers now remain active across rebuilds and retry attempts. * Previous blocking reasons are preserved alongside newly identified issues. * Empty corrective rebuilds are reviewed before being accepted as complete. * Tasks can no longer be finalized solely because a rebuilt diff is smaller when unresolved blockers remain. * **Documentation** * Updated release notes to describe the improved blocker behavior. --------- Co-authored-by: v --- .../merge-review-blockers-survive-rebuilds.md | 7 + .../engine/src/__tests__/merger-ai.test.ts | 140 ++++++++++++++++++ packages/engine/src/merger-ai-prompts.ts | 3 +- packages/engine/src/merger-ai.ts | 76 ++++++++-- 4 files changed, 209 insertions(+), 17 deletions(-) create mode 100644 .changeset/merge-review-blockers-survive-rebuilds.md diff --git a/.changeset/merge-review-blockers-survive-rebuilds.md b/.changeset/merge-review-blockers-survive-rebuilds.md new file mode 100644 index 0000000000..d087a24125 --- /dev/null +++ b/.changeset/merge-review-blockers-survive-rebuilds.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Keep unresolved merge-review blockers active across concurrent-main rebuilds and later retries. +category: fix +dev: Carries prior blocking reasons into rebuilt merge and review prompts so a smaller residual diff cannot incorrectly finalize a task as done. diff --git a/packages/engine/src/__tests__/merger-ai.test.ts b/packages/engine/src/__tests__/merger-ai.test.ts index 0a4767903e..cf52f512fc 100644 --- a/packages/engine/src/__tests__/merger-ai.test.ts +++ b/packages/engine/src/__tests__/merger-ai.test.ts @@ -243,6 +243,146 @@ describe("parseReviewVerdict", () => { }); describe("runAiMerge", () => { + it("carries blocking review reasons across a concurrent-main rebuild", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + const blocker = "server pages still bypass the live authorization guard"; + const { store } = makeStore(dir); + const mergeAgent = realMergeAgent("fusion/fn-1"); + const reviewPrompts: string[] = []; + let reviewCount = 0; + const reviewAgent = vi.fn(async (_cwd: string, prompt: string) => { + reviewPrompts.push(prompt); + reviewCount++; + if (reviewCount === 1) { + return `${blocker}\nSEVERITY: blocking\nREVIEW_VERDICT: reject`; + } + if (reviewCount === 2) { + writeFileSync(join(dir, "concurrent.txt"), "main advanced\n"); + git(dir, "add concurrent.txt"); + git(dir, "commit -q -m 'main: concurrent advance'"); + } + return "REVIEW_VERDICT: approve"; + }); + + const result = await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent, + reviewAgent, + }); + + expect(result.merged).toBe(true); + expect(reviewPrompts).toHaveLength(3); + expect(reviewPrompts[2]).toContain(blocker); + expect(reviewPrompts[2]).toContain("complete resulting tree"); + }); + + it("rechecks a durable blocker when a later merge retry starts", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + const blocker = "server pages still bypass the live authorization guard"; + const { store } = makeStore(dir, { + log: [{ + action: `AI merge BLOCKED after 3 corrective pass(es) — unresolved correctness concern: ${blocker}`, + timestamp: new Date().toISOString(), + }], + }); + const reviewAgent = vi.fn(async () => "REVIEW_VERDICT: approve"); + + await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent: realMergeAgent("fusion/fn-1"), + reviewAgent, + }); + + expect(reviewAgent.mock.calls[0]?.[1]).toContain(blocker); + expect(reviewAgent.mock.calls[0]?.[1]).toContain("complete resulting tree"); + }); + + it("reviews a durable blocker even when the retried branch has zero commits ahead", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + git(dir, "merge -q fusion/fn-1"); + const blocker = "the integrated tree still bypasses authorization"; + const { store } = makeStore(dir, { + log: [{ + action: `AI merge BLOCKED after 1 corrective pass(es) — unresolved correctness concern: ${blocker}`, + timestamp: new Date().toISOString(), + }], + }); + const reviewAgent = vi.fn(async () => "REVIEW_VERDICT: approve"); + + await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent: vi.fn(async () => { /* zero-ahead corrective review */ }), + reviewAgent, + }); + + expect(reviewAgent).toHaveBeenCalledOnce(); + expect(reviewAgent.mock.calls[0]?.[1]).toContain(blocker); + }); + + it("reviews an empty corrective rebuild before accepting it as a no-op", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + const blocker = "the merged tree still bypasses authorization"; + const { store } = makeStore(dir); + const integrationTipBefore = git(dir, "rev-parse main"); + let mergeCount = 0; + const mergeAgent = vi.fn(async (cwd: string) => { + mergeCount++; + if (mergeCount === 1) await realMergeAgent("fusion/fn-1")(cwd, ""); + /* + FNXC:MergeReviewBlockers 2026-07-21-21:50: + The corrective pass deliberately leaves the clean-room tree at the integration tip so the regression proves an empty rebuild still receives review and cannot advance the integration ref. + */ + }); + const reviewAgent = vi.fn() + .mockResolvedValueOnce(`${blocker}\nSEVERITY: blocking\nREVIEW_VERDICT: reject`) + .mockResolvedValueOnce("REVIEW_VERDICT: approve"); + + const result = await runAiMerge(store, dir, "FN-1", { manual: true }, { mergeAgent, reviewAgent }); + + expect(mergeAgent).toHaveBeenCalledTimes(2); + expect(reviewAgent).toHaveBeenCalledTimes(2); + expect(reviewAgent.mock.calls[1]?.[1]).toContain(blocker); + expect(result.merged).toBe(false); + expect(git(dir, "rev-parse main")).toBe(integrationTipBefore); + }); + + it("keeps earlier blockers when later reviews discover different failures", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + const blockerX = "authorization is bypassed"; + const blockerY = "audit metadata is missing"; + const { store } = makeStore(dir, {}, { merger: { mode: "ai", maxReviewPasses: 2 } }); + const reviewAgent = vi.fn() + .mockResolvedValueOnce(`${blockerX}\nREVIEW_VERDICT: reject`) + .mockResolvedValueOnce(`${blockerY}\nREVIEW_VERDICT: reject`) + .mockResolvedValueOnce("REVIEW_VERDICT: approve"); + + await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent: realMergeAgent("fusion/fn-1"), + reviewAgent, + }); + + expect(reviewAgent.mock.calls[2]?.[1]).toContain(blockerX); + expect(reviewAgent.mock.calls[2]?.[1]).toContain(blockerY); + }); + + it("recovers every blocker from interrupted per-pass rejection 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}` }, + ], + }); + const reviewAgent = vi.fn(async () => "REVIEW_VERDICT: approve"); + + await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent: realMergeAgent("fusion/fn-1"), + reviewAgent, + }); + + expect(reviewAgent.mock.calls[0]?.[1]).toContain(blockerX); + expect(reviewAgent.mock.calls[0]?.[1]).toContain(blockerY); + }); + it("merges a clean branch, advances main, and finalizes the task", async () => { const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); const { store, emitted } = makeStore(dir); diff --git a/packages/engine/src/merger-ai-prompts.ts b/packages/engine/src/merger-ai-prompts.ts index 727b418a80..af5bcc52d3 100644 --- a/packages/engine/src/merger-ai-prompts.ts +++ b/packages/engine/src/merger-ai-prompts.ts @@ -350,7 +350,8 @@ export function buildReviewPrompt(input: { lines.push( "", "A prior pass rejected an earlier attempt for these reasons — confirm they", - "are now resolved:", + "are now resolved in the complete resulting tree, including code outside", + "this squash diff. Do not approve merely because the rebuilt diff became smaller:", ...input.priorReasons.map((r) => ` - ${r}`) ); } diff --git a/packages/engine/src/merger-ai.ts b/packages/engine/src/merger-ai.ts index 9006837982..9f8225ef56 100644 --- a/packages/engine/src/merger-ai.ts +++ b/packages/engine/src/merger-ai.ts @@ -161,6 +161,29 @@ 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]); + } + return reasons; +} + function matchesApprovedAiMergeSha(squashSha: string, approvedShas: Set): boolean { if (approvedShas.size === 0) return true; const normalized = squashSha.toLowerCase(); @@ -795,6 +818,8 @@ export async function landOneRepo( await log(`AI merge: pre-merge prune failed: ${getErrorMessage(err)}`); } let advanceRetries = 0; + const taskAtStart = await store.getTask(taskId); + let outstandingReviewReasons = getOutstandingBlockingMergeReasons(taskAtStart); while (true) { throwIfAborted(signal, taskId); const tipSha = await git(["rev-parse", "--verify", `refs/heads/${integrationBranch}`], repoRootDir); @@ -807,7 +832,11 @@ export async function landOneRepo( // exhaustion and the card is parked failed. Only short-circuit on a CONFIDENT // 0: a git failure yields "" → parseInt → NaN (≠ 0) and falls through. const aheadRaw = await git(["rev-list", "--count", `${integrationBranch}..${branch}`], repoRootDir).catch(() => ""); - if (Number.parseInt(aheadRaw.trim(), 10) === 0) { + /* + FNXC:MergeReviewBlockers 2026-07-21-21:45: + Zero commits ahead is only an unconditional no-op when no durable blocker remains. A retry after reset, rebase, or prior integration must still review the complete integration tree before clearing previously rejected correctness concerns. + */ + if (Number.parseInt(aheadRaw.trim(), 10) === 0 && outstandingReviewReasons.length === 0) { await audit.git({ type: "merge:ai-empty", target: integrationBranch, metadata: { taskId, tipSha } }); return { outcome: "empty", tipSha, integrationBranch }; } @@ -906,10 +935,13 @@ export async function landOneRepo( await log(`[timing] AI merge dependency sync completed in ${Date.now() - depsSyncStartedAt}ms${depsSyncResult ? (depsSyncResult.installCommand ? ` (${depsSyncResult.skipped ? "skipped" : "ran"}: ${depsSyncResult.installCommand})` : " (no command)") : " (failed — non-fatal, deps unavailable)"}`); // 2 + 3. Merge + review loop (corrective passes). - const squashSha = await mergeAndReview({ + const reviewResult = await mergeAndReview({ mergeRoot, branch, integrationBranch, tipSha, taskTitle, includeTaskId, trailers, taskId, maxPasses, mergeAgent, reviewAgent, audit, log, setStatus, store, signal, + initialPriorReasons: outstandingReviewReasons, }); + const squashSha = reviewResult.squashSha; + outstandingReviewReasons = reviewResult.priorReasons; if (!squashSha) { // Branch had no net changes vs the tip — nothing to land. The caller @@ -1993,9 +2025,10 @@ async function mergeAndReview(input: { setStatus: (status: string | null) => Promise; store: TaskStore; signal?: AbortSignal; -}): Promise { + 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: string[] = []; + let priorReasons = [...(input.initialPriorReasons ?? [])]; for (let attempt = 0; ; attempt++) { throwIfAborted(signal, taskId); @@ -2017,13 +2050,16 @@ async function mergeAndReview(input: { })); let head = await git(["rev-parse", "HEAD"], mergeRoot); - if (head === tipSha) return null; // empty merge — nothing landed + const emptyMerge = head === tipSha; + if (emptyMerge && priorReasons.length === 0) return { squashSha: null, priorReasons }; // empty initial merge — nothing landed // Guarantee the squash's task metadata (task-id subject prefix + board // association trailers) even if the agent omitted it — this amends HEAD, so // re-read the sha afterwards. - await ensureCommitTaskMetadata(mergeRoot, taskId, includeTaskId, trailers); - head = await git(["rev-parse", "HEAD"], mergeRoot); + if (!emptyMerge) { + await ensureCommitTaskMetadata(mergeRoot, taskId, includeTaskId, trailers); + head = await git(["rev-parse", "HEAD"], mergeRoot); + } await setStatus("reviewing"); const diffStat = await git(["diff", "--stat", `${tipSha}..${head}`], mergeRoot); @@ -2041,24 +2077,32 @@ async function mergeAndReview(input: { if (verdict.verdict === "approve") { await log(`AI merge review (pass ${attempt + 1}): approved squash ${head}`); - return 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) { if (verdict.severity === "blocking") { - await audit.git({ type: "merge:ai-review-blocked", target: integrationBranch, metadata: { taskId, attempt, reasons: verdict.reasons } }); - await log(`AI merge BLOCKED after ${attempt} corrective pass(es) — unresolved correctness concern: ${verdict.reasons.join("; ")}`); - throw new AiMergeBlockedError(taskId, verdict.reasons); + 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: verdict.reasons, squashSha: head } }); - await log(`AI merge: landing with unresolved advisory concern(s): ${verdict.reasons.join("; ")}`); - return head; + 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 }; } - priorReasons = verdict.reasons; - await log(`AI merge review (pass ${attempt + 1}): rejected (${verdict.severity}) — ${verdict.reasons.join("; ")}`); + priorReasons = unresolvedReasons; + await log(`AI merge review (pass ${attempt + 1}): rejected (${verdict.severity}) — ${unresolvedReasons.join("; ")}`); } }