Fix merge blockers lost during concurrent rebuilds (#2346)
## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: v <v@v.speedport.ip>
This commit is contained in:
flexi767
7
.changeset/merge-review-blockers-survive-rebuilds.md
Normal file
7
.changeset/merge-review-blockers-survive-rebuilds.md
Normal file
@@ -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.
|
||||
@@ -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);
|
||||
|
||||
@@ -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}`)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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<string>): 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<unknown>;
|
||||
store: TaskStore;
|
||||
signal?: AbortSignal;
|
||||
}): Promise<string | null> {
|
||||
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("; ")}`);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user