From b47fb70b817690a45f1aa4d776983783685780bb Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sat, 22 Aug 2026 19:23:15 -0700 Subject: [PATCH] fix: defer merge on unrun pre-merge gates instead of failing the task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An enabled pre-merge gate that has not reported yet is a not-yet condition, not a failure. FN-9191 proved the difference is load-bearing: the in-review auto-merge sweep enqueued the card ~2s after fn_task_done and ~18s before the graph started its own Code Review node, the merge door correctly refused, and the auto-merge error path parked it status="failed". Code Review APPROVED two minutes later, but every subsequent merge — including the graph's own merge node — then died on "task is marked 'failed'". - Merge doors throw the typed PreMergeStepsNotRunError for that blocker. - The auto-merge error path treats it as a deferral: no status write, no mergeRetries burn, no operator handoff. - enqueueEligibleInReviewTasks holds a card out of the merge queue until every enabled pre-merge group has a result, so the race stops at admission. Co-Authored-By: Claude Opus 5 --- .changeset/pre-merge-gate-deferral.md | 7 +++ .../required-pre-merge-steps.test.ts | 45 ++++++++++++++- .../core/src/__tests__/task-merge.test.ts | 15 +++++ packages/core/src/index.gate.ts | 3 + packages/core/src/index.ts | 5 +- .../src/merge/required-pre-merge-steps.ts | 21 +++++++ packages/core/src/merge/task-merge.ts | 40 +++++++++++++- .../core/src/task-store/merge-queue-ops.ts | 4 +- .../__tests__/merge-error-recovery.test.ts | 55 ++++++++++++++++++- packages/engine/src/merge/merger-ai.ts | 4 ++ packages/engine/src/merger.ts | 4 ++ packages/engine/src/project-engine.ts | 27 ++++++++- 12 files changed, 223 insertions(+), 7 deletions(-) create mode 100644 .changeset/pre-merge-gate-deferral.md diff --git a/.changeset/pre-merge-gate-deferral.md b/.changeset/pre-merge-gate-deferral.md new file mode 100644 index 0000000000..1d490510c2 --- /dev/null +++ b/.changeset/pre-merge-gate-deferral.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: A task no longer fails permanently when auto-merge runs before its Code Review gate. +category: fix +dev: Merge doors throw the typed `PreMergeStepsNotRunError` for the unrun-enabled-gate blocker; the auto-merge error path treats it as a deferral (no `status:"failed"` park, no retry burn), and `enqueueEligibleInReviewTasks` holds in-review cards out of the merge queue until every enabled pre-merge group has a result (`findUnrunRequiredPreMergeStepIds`). diff --git a/packages/core/src/__tests__/required-pre-merge-steps.test.ts b/packages/core/src/__tests__/required-pre-merge-steps.test.ts index 120cc033d2..4e3b35f9b0 100644 --- a/packages/core/src/__tests__/required-pre-merge-steps.test.ts +++ b/packages/core/src/__tests__/required-pre-merge-steps.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { BUILTIN_CODING_WORKFLOW_IR } from "../workflows/builtin-coding-workflow-ir.js"; -import { resolveRequiredPreMergeStepIds } from "../merge/required-pre-merge-steps.js"; +import { findUnrunRequiredPreMergeStepIds, resolveRequiredPreMergeStepIds } from "../merge/required-pre-merge-steps.js"; describe("resolveRequiredPreMergeStepIds", () => { it("includes default-on pre-merge groups when no explicit selection exists", () => { @@ -23,3 +23,46 @@ describe("resolveRequiredPreMergeStepIds", () => { )).toEqual(new Set(["browser-verification"])); }); }); + +/* +FNXC:RequiredPreMergeSteps 2026-08-22-22:40 (FN-9191 wedge): +The auto-merge sweep's admission uses this to hold a card out of the merge queue until every +enabled pre-merge gate has reported. FN-9191's exact shape — both gates enabled, Plan Review +already passed, Code Review not yet started — must read as "unrun", and the same task after +Code Review lands must read as ready. +*/ +describe("findUnrunRequiredPreMergeStepIds", () => { + const planReviewPassed = { + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "passed" as const, + phase: "pre-merge" as const, + }; + const codeReviewPassed = { + workflowStepId: "code-review", + workflowStepName: "Code Review", + status: "passed" as const, + phase: "pre-merge" as const, + }; + + it("reports the FN-9191 window: Code Review enabled but not yet started", () => { + expect(findUnrunRequiredPreMergeStepIds(BUILTIN_CODING_WORKFLOW_IR, { + enabledWorkflowSteps: ["plan-review", "code-review"], + workflowStepResults: [planReviewPassed], + })).toEqual(["code-review"]); + }); + + it("reports nothing once every enabled gate has a result", () => { + expect(findUnrunRequiredPreMergeStepIds(BUILTIN_CODING_WORKFLOW_IR, { + enabledWorkflowSteps: ["plan-review", "code-review"], + workflowStepResults: [planReviewPassed, codeReviewPassed], + })).toEqual([]); + }); + + it("reports nothing when the gates are disabled for the task", () => { + expect(findUnrunRequiredPreMergeStepIds(BUILTIN_CODING_WORKFLOW_IR, { + enabledWorkflowSteps: [], + workflowStepResults: [], + })).toEqual([]); + }); +}); diff --git a/packages/core/src/__tests__/task-merge.test.ts b/packages/core/src/__tests__/task-merge.test.ts index a691ae96bf..59f360e174 100644 --- a/packages/core/src/__tests__/task-merge.test.ts +++ b/packages/core/src/__tests__/task-merge.test.ts @@ -1,6 +1,9 @@ import { describe, it, expect } from "vitest"; import type { PrInfo, StepStatus } from "../types.js"; import { + isPreMergeStepsNotRunBlocker, + PreMergeStepsNotRunError, + PRE_MERGE_STEPS_NOT_RUN_BLOCKER, BLOCKING_TASK_STATUSES, collectLandedMemberReviewAdvisories, HARD_BLOCKING_TASK_STATUSES, @@ -518,6 +521,18 @@ describe("getTaskMergeBlocker", () => { expect(getTaskMergeBlocker(baseTask)).toBeUndefined(); }); + /* FNXC:RequiredPreMergeSteps 2026-08-22-22:40: the blocker text is a shared contract — the + auto-merge error path classifies on the typed error built from it, so it may not drift. */ + it("exposes the unrun-gate reason as a classifiable constant", () => { + expect(getTaskMergeBlocker(baseTask, { requiredPreMergeStepIds: new Set(["code-review"]) })) + .toBe(PRE_MERGE_STEPS_NOT_RUN_BLOCKER); + expect(isPreMergeStepsNotRunBlocker(PRE_MERGE_STEPS_NOT_RUN_BLOCKER)).toBe(true); + expect(isPreMergeStepsNotRunBlocker("task has failed pre-merge workflow steps")).toBe(false); + expect(isPreMergeStepsNotRunBlocker(undefined)).toBe(false); + expect(new PreMergeStepsNotRunError("FN-9191").message) + .toBe(`Cannot merge FN-9191: ${PRE_MERGE_STEPS_NOT_RUN_BLOCKER}`); + }); + it("accepts a skipped result for a required pre-merge group", () => { expect(getTaskMergeBlocker({ ...baseTask, diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index 35c0b5c388..06bce54fca 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -1086,6 +1086,9 @@ export { getPrimaryPrInfo, taskHasManualOpenPullRequest } from "./tasks/task-hel export { collectLandedMemberReviewAdvisories, getTaskMergeBlocker, + isPreMergeStepsNotRunBlocker, + PreMergeStepsNotRunError, + PRE_MERGE_STEPS_NOT_RUN_BLOCKER, getTaskHardMergeBlocker, REVIEW_ELIGIBLE_SENTINEL_COLUMN, MERGE_CONFIRMED_TRANSIENT_STATUSES, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d287a3c4c9..8cfce7f820 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -350,7 +350,7 @@ export { isWorkflowOptionalGroupEnabled, } from "./workflows/workflow-optional-steps.js"; export type { ResolvedWorkflowOptionalStep } from "./workflows/workflow-optional-steps.js"; -export { resolveRequiredPreMergeStepIds } from "./merge/required-pre-merge-steps.js"; +export { resolveRequiredPreMergeStepIds, findUnrunRequiredPreMergeStepIds } from "./merge/required-pre-merge-steps.js"; export { applyPromptOverridesToIr, enumeratePromptBearingWorkflowNodes, @@ -1252,6 +1252,9 @@ export { getPrimaryPrInfo, taskHasManualOpenPullRequest } from "./tasks/task-hel export { collectLandedMemberReviewAdvisories, getTaskMergeBlocker, + isPreMergeStepsNotRunBlocker, + PreMergeStepsNotRunError, + PRE_MERGE_STEPS_NOT_RUN_BLOCKER, getTaskHardMergeBlocker, REVIEW_ELIGIBLE_SENTINEL_COLUMN, MERGE_CONFIRMED_TRANSIENT_STATUSES, diff --git a/packages/core/src/merge/required-pre-merge-steps.ts b/packages/core/src/merge/required-pre-merge-steps.ts index a9ab28d1fd..329d5edfa2 100644 --- a/packages/core/src/merge/required-pre-merge-steps.ts +++ b/packages/core/src/merge/required-pre-merge-steps.ts @@ -24,3 +24,24 @@ export function resolveRequiredPreMergeStepIds( .map((step) => step.templateId), ); } + +/* +FNXC:RequiredPreMergeSteps 2026-08-22-22:40 (FN-9191 wedge): +Admission-side twin of the door's check. The in-review auto-merge sweep must be able to ask +"has every enabled pre-merge gate reported yet?" BEFORE it queues a card, because its sync +`canMergeTask` admission sees result rows only — and a gate that has not started has no row. +FN-9191 was queued ~2s after `fn_task_done` and ~18s before its own Code Review node started. +Shared with the door so the two can never answer differently. +*/ +/** Enabled pre-merge group ids that have produced no result row on this task yet. */ +export function findUnrunRequiredPreMergeStepIds( + ir: WorkflowIr, + task: { + enabledWorkflowSteps?: readonly string[]; + workflowStepResults?: ReadonlyArray<{ workflowStepId?: string }>; + }, +): string[] { + const results = task.workflowStepResults ?? []; + return [...resolveRequiredPreMergeStepIds(ir, task.enabledWorkflowSteps)] + .filter((workflowStepId) => !results.some((result) => result.workflowStepId === workflowStepId)); +} diff --git a/packages/core/src/merge/task-merge.ts b/packages/core/src/merge/task-merge.ts index ebfcd60107..071c3a27f8 100644 --- a/packages/core/src/merge/task-merge.ts +++ b/packages/core/src/merge/task-merge.ts @@ -339,6 +339,44 @@ const NON_TERMINAL_WORKFLOW_STATUSES = new Set([ "pending", ]); +/* +FNXC:RequiredPreMergeSteps 2026-08-22-22:40 (FN-9191 wedge): +"An enabled pre-merge gate has not produced a result yet" is a NOT-YET condition, not a +failure. FN-9191 proved the difference is load-bearing: the in-review auto-merge sweep +enqueued the card ~2s after `fn_task_done`, BEFORE the graph started its own Code Review +node, so the door correctly refused — and the auto-merge error path then parked the card +`status:"failed"` as a non-conflict failure. Code Review completed and APPROVED 2 minutes +later, but every subsequent merge (including the graph's own merge node) then failed with +`task is marked 'failed'`. A correct refusal became a permanent wedge. + +The blocker message is exported so merge doors can throw the typed error below instead of a +bare `Error`, and so the auto-merge error path can classify it as a deferral rather than a +terminal park. +*/ +export const PRE_MERGE_STEPS_NOT_RUN_BLOCKER = + "task has enabled pre-merge workflow steps that never ran"; + +/** + * Thrown by merge doors when the ONLY thing standing between a card and merge is an + * enabled pre-merge gate that has not run yet. Callers must treat it as "retry after the + * gate reports", never as a terminal failure: no `status:"failed"` park, no retry-budget + * burn, no operator handoff. + */ +export class PreMergeStepsNotRunError extends Error { + readonly code = "pre-merge-steps-not-run" as const; + readonly taskId: string; + constructor(taskId: string, message = `Cannot merge ${taskId}: ${PRE_MERGE_STEPS_NOT_RUN_BLOCKER}`) { + super(message); + this.name = "PreMergeStepsNotRunError"; + this.taskId = taskId; + } +} + +/** True when a `getTaskMergeBlocker` reason is the deferrable unrun-gate reason. */ +export function isPreMergeStepsNotRunBlocker(blocker: string | undefined): boolean { + return blocker === PRE_MERGE_STEPS_NOT_RUN_BLOCKER; +} + export const TASK_DONE_BYPASS_BLOCKER_MESSAGE = "done bypass requires merge confirmation or explicit no-commits policy"; @@ -426,7 +464,7 @@ export function getTaskMergeBlocker( (workflowStepId) => !(task.workflowStepResults ?? []).some((result) => result.workflowStepId === workflowStepId), ) ) { - return "task has enabled pre-merge workflow steps that never ran"; + return PRE_MERGE_STEPS_NOT_RUN_BLOCKER; } // Only pre-merge workflow step failures block merge. diff --git a/packages/core/src/task-store/merge-queue-ops.ts b/packages/core/src/task-store/merge-queue-ops.ts index 909a8cac15..374a8beb21 100644 --- a/packages/core/src/task-store/merge-queue-ops.ts +++ b/packages/core/src/task-store/merge-queue-ops.ts @@ -11,7 +11,7 @@ import {existsSync} from "node:fs"; import type {Task, MergeResult, MergeQueueEntry, MergeQueueAcquireOptions} from "../types.js"; import {assertNotWorkspaceTaskMerge} from "../types.js"; import "../builtin-traits.js"; -import {getTaskMergeBlocker, resolveTaskMergeTarget} from "../merge/task-merge.js"; +import {getTaskMergeBlocker, isPreMergeStepsNotRunBlocker, PreMergeStepsNotRunError, resolveTaskMergeTarget} from "../merge/task-merge.js"; import {resolveRequiredPreMergeStepIds} from "../merge/required-pre-merge-steps.js"; import {resolveWorkflowIrForTask} from "../workflows/workflow-ir-resolver.js"; import {resolveReviewColumns, resolveTaskLifecycleColumns} from "../workflows/workflow-lifecycle-traits.js"; @@ -440,6 +440,8 @@ export async function mergeTaskImpl(store: TaskStore, id: string): Promise { class MockVerificationError extends Error { @@ -390,6 +396,53 @@ describe("ProjectEngine merge error recovery", () => { vi.useRealTimers(); }); + /* + FNXC:RequiredPreMergeSteps 2026-08-22-22:40 (FN-9191 wedge): + SYMPTOM: FN-9191 sat `in-review` with `status:"failed"` and + `error: "Cannot merge FN-9191: task has enabled pre-merge workflow steps that never ran"`, + even though BOTH enabled gates (Plan Review, Code Review) later ran and APPROVED. The sweep + enqueued the card ~2s after `fn_task_done`, ~18s before the graph started its own Code Review + node; the door refused correctly, and THIS error path turned a not-yet answer into a terminal + park. Every later merge — including the graph's own merge node at 02:04:38 — then died on + `task is marked 'failed'`. + ASSERTION: a `PreMergeStepsNotRunError` writes no status, burns no retry, and moves nothing. + */ + it("defers (does not park) when a merge door refuses only because a pre-merge gate has not run", async () => { + vi.useFakeTimers(); + const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout"); + const store = makeStore({ tasks: [makeTask({ mergeRetries: 0 }), makeTask({ mergeRetries: 0 })] }); + vi.mocked(runAiMerge).mockRejectedValueOnce(new PreMergeStepsNotRunError(TASK_ID)); + + const engine = createEngine(store); + await runMergeCycle(engine); + + expect(store.updateTask).not.toHaveBeenCalled(); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.addTaskComment).not.toHaveBeenCalled(); + expect(store.logEntry).toHaveBeenCalledWith( + TASK_ID, + expect.stringContaining(PRE_MERGE_STEPS_NOT_RUN_BLOCKER), + "MergeDeferredPendingPreMergeSteps", + ); + expect(store.logEntry).not.toHaveBeenCalledWith(TASK_ID, expect.any(String), "MergeNonConflictFailure"); + expect(setTimeoutSpy).not.toHaveBeenCalledWith(expect.any(Function), expect.any(Number)); + vi.useRealTimers(); + }); + + it("still parks other non-conflict merge failures as failed", async () => { + const store = makeStore({ tasks: [makeTask({ mergeRetries: 0 }), makeTask({ mergeRetries: 0 })] }); + vi.mocked(runAiMerge).mockRejectedValueOnce(new Error("remote rejected the push")); + + const engine = createEngine(store); + await runMergeCycle(engine); + + expect(store.updateTask).toHaveBeenCalledWith( + TASK_ID, + expect.objectContaining({ status: "failed", error: expect.stringContaining("remote rejected the push") }), + ); + expect(store.logEntry).toHaveBeenCalledWith(TASK_ID, expect.any(String), "MergeNonConflictFailure"); + }); + 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/merge/merger-ai.ts b/packages/engine/src/merge/merger-ai.ts index b4c4646eb0..74891beb27 100644 --- a/packages/engine/src/merge/merger-ai.ts +++ b/packages/engine/src/merge/merger-ai.ts @@ -48,6 +48,8 @@ import { getPlannerInterventionTimeline, getPrimaryPrInfo, getTaskMergeBlocker, + isPreMergeStepsNotRunBlocker, + PreMergeStepsNotRunError, normalizeMergeAdvanceAutoSyncMode, resolvePersistAgentThinkingLog, resolveTaskMergeTarget, @@ -1432,6 +1434,8 @@ export async function runAiMerge( reviewColumns: aiReviewColumns, requiredPreMergeStepIds, }); + /* FNXC:RequiredPreMergeSteps 2026-08-22-22:40: an unrun enabled gate is a deferral (typed), not a failure. */ + if (blocker && isPreMergeStepsNotRunBlocker(blocker)) throw new PreMergeStepsNotRunError(taskId); if (blocker) throw new Error(`Cannot merge ${taskId}: ${blocker}`); const settings = await store.getSettings(); diff --git a/packages/engine/src/merger.ts b/packages/engine/src/merger.ts index b36b639bec..c6ae83f083 100644 --- a/packages/engine/src/merger.ts +++ b/packages/engine/src/merger.ts @@ -91,6 +91,8 @@ import { buildTaskLineageTrailer, evaluateNoCommitsNoOpFinalize, getTaskMergeBlocker, + isPreMergeStepsNotRunBlocker, + PreMergeStepsNotRunError, normalizeMergeConflictStrategy, normalizeMergeStrategyOverlapBehavior, normalizePostMergeAuditMode, @@ -6817,6 +6819,8 @@ export async function aiMergeTask( requiredPreMergeStepIds, }); if (mergeBlocker) { + /* FNXC:RequiredPreMergeSteps 2026-08-22-22:40: an unrun enabled gate is a deferral (typed), not a failure — see PreMergeStepsNotRunError. */ + if (isPreMergeStepsNotRunBlocker(mergeBlocker)) throw new PreMergeStepsNotRunError(taskId); throw new Error(`Cannot merge ${taskId}: ${mergeBlocker}`); } diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index 9622a4692a..6a4cc6faa6 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -35,7 +35,7 @@ import { getTaskHardMergeBlocker, PreMergeStepsNotRunError, PRE_MERGE_STEPS_NOT_RUN_BLOCKER, - resolveRequiredPreMergeStepIds, + findUnrunRequiredPreMergeStepIds, isLiveSharedBranchGroupMemberIntegration, isSharedBranchGroupMemberIntegration, isWorkspaceTask, @@ -3435,8 +3435,31 @@ export class ProjectEngine { ); }) as Task[]; const allowFlags = await Promise.all(candidates.map((t) => this.allowInReviewMergeProcessing(t, settings, this.runtime.getTaskStore()))); + /* + FNXC:RequiredPreMergeSteps 2026-08-22-22:40 (FN-9191 wedge): + Admission cannot see unrun pre-merge gates: `canMergeTask` is sync and the injected + `getTaskMergeBlocker` has no workflow IR, so it answers on RESULT ROWS only — and a gate that + has not started yet has no row. FN-9191 was enqueued ~2s after `fn_task_done`, ~18s before its + Code Review node started, and the door then had to refuse it. + + This sweep already resolves each card's IR, so ask the same question the door asks and hold the + card out of the queue until every enabled pre-merge group has a result. Failure to resolve the + IR admits the card (the door remains the authority); this filter exists to stop the race, not + to become a second gate. + */ + const unrunGateFlags = await Promise.all( + candidates.map(async (t) => { + try { + const ir = await resolveWorkflowIrForTask(this.runtime.getTaskStore(), t.id, reviewLaneIrCache); + if (!ir) return false; + return findUnrunRequiredPreMergeStepIds(ir, t).length > 0; + } catch { + return false; + } + }), + ); const eligible = sortTasksByPriorityThenAgeAndId( - candidates.filter((_, i) => allowFlags[i]), + candidates.filter((_, i) => allowFlags[i] && !unrunGateFlags[i]), ); for (const t of eligible) { this.internalEnqueueMerge(t.id);