From bc82d8e0e15bbbf938fc7cbba551cc0f89ec9f73 Mon Sep 17 00:00:00 2001 From: Phil Larson Date: Sun, 23 Aug 2026 15:31:17 -0700 Subject: [PATCH] fix(core): thread review lanes through merge readiness (#3514) ## Summary - thread resolved review lanes through `isTaskReadyForMerge` - preserve required pre-merge step filtering - add coverage for a renamed review lane ## Test plan - `pnpm --filter @fusion/core exec vitest run --silent=passed-only --reporter=dot src/__tests__/task-merge.test.ts` - `pnpm --filter @fusion/core typecheck` - `pnpm check:lane-wiring` - `pnpm check:changesets` - `pnpm exec eslint packages/core/src/merge/task-merge.ts packages/core/src/__tests__/task-merge.test.ts` ## Summary by CodeRabbit * **Bug Fixes** * Custom review lanes are now honored during merge-readiness checks and auto-merge processing. * Renamed workflow lanes correctly determine whether tasks can merge. * Tasks resumed from a paused state are routed and evaluated using the appropriate review lane. * The default `in-review` lane remains supported when no custom review lanes are configured. --------- Co-authored-by: gsxdsm --- .changeset/task-ready-lane-wiring.md | 7 + .../core/src/__tests__/task-merge.test.ts | 21 +++ packages/core/src/merge/task-merge.ts | 22 ++- ...project-engine-merge-lane-resolved.test.ts | 129 +++++++++++++++++- packages/engine/src/project-engine.ts | 36 +++-- 5 files changed, 198 insertions(+), 17 deletions(-) create mode 100644 .changeset/task-ready-lane-wiring.md diff --git a/.changeset/task-ready-lane-wiring.md b/.changeset/task-ready-lane-wiring.md new file mode 100644 index 0000000000..8439210ca9 --- /dev/null +++ b/.changeset/task-ready-lane-wiring.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Honor custom review lanes across live merge-readiness checks. +category: fix +dev: Threads resolved review lanes through ProjectEngine admission and preserves the empty-set legacy fallback. diff --git a/packages/core/src/__tests__/task-merge.test.ts b/packages/core/src/__tests__/task-merge.test.ts index d9ff875e46..788e7006ab 100644 --- a/packages/core/src/__tests__/task-merge.test.ts +++ b/packages/core/src/__tests__/task-merge.test.ts @@ -862,6 +862,27 @@ describe("isTaskReadyForMerge", () => { expect(isTaskReadyForMerge(baseTask)).toBe(true); }); + it("uses the caller's resolved review lanes", () => { + expect(isTaskReadyForMerge( + { ...baseTask, column: "signoff" }, + { reviewColumns: new Set(["signoff"]) }, + )).toBe(true); + }); + + /* + FNXC:MergeReadiness 2026-08-23-18:49: + A resolved workflow can return an empty review-lane set when it has no usable trait answer. Empty is + therefore "unresolved", not an authoritative board with no review lane; keep the legacy `in-review` + identity fallback until the caller can supply at least one resolved lane. + */ + it("preserves the legacy review lane fallback for an empty resolved set", () => { + expect(isTaskReadyForMerge(baseTask, { reviewColumns: new Set() })).toBe(true); + expect(isTaskReadyForMerge( + { ...baseTask, column: "signoff" }, + { reviewColumns: new Set() }, + )).toBe(false); + }); + it("returns false when pre-merge step failed", () => { expect(isTaskReadyForMerge({ ...baseTask, diff --git a/packages/core/src/merge/task-merge.ts b/packages/core/src/merge/task-merge.ts index 41b5c496d2..f21cad0ae1 100644 --- a/packages/core/src/merge/task-merge.ts +++ b/packages/core/src/merge/task-merge.ts @@ -430,12 +430,19 @@ export function getTaskMergeBlocker( the resolved lanes when they are known, so it can never point at a column the board does not have. */ if (!options.skipColumnIdentityCheck) { - const inReviewLane = options.reviewColumns - ? options.reviewColumns.has(task.column) + /* + FNXC:MergeReadiness 2026-08-23-18:49: + An empty resolved lane set means the workflow supplied no usable trait answer. Preserve the + documented legacy identity fallback until at least one resolved review lane is available; treating + an empty Set as authoritative would make every column fail while the error still names `in-review`. + */ + const reviewColumns = options.reviewColumns?.size ? options.reviewColumns : undefined; + const inReviewLane = reviewColumns + ? reviewColumns.has(task.column) : task.column === "in-review"; if (!inReviewLane) { - const expected = options.reviewColumns && options.reviewColumns.size > 0 - ? [...options.reviewColumns].map((c) => `'${c}'`).join(" or ") + const expected = reviewColumns + ? [...reviewColumns].map((c) => `'${c}'`).join(" or ") : "'in-review'"; return `task is in '${task.column}', must be in ${expected}`; } @@ -665,9 +672,12 @@ export function getTaskDoneBypassBlocker( export function isTaskReadyForMerge( task: Pick, - options: { requiredPreMergeStepIds?: ReadonlySet } = {}, + options: { reviewColumns?: ReadonlySet; requiredPreMergeStepIds?: ReadonlySet } = {}, ): boolean { - return getTaskMergeBlocker(task, options) === undefined; + return getTaskMergeBlocker(task, { + reviewColumns: options.reviewColumns, + requiredPreMergeStepIds: options.requiredPreMergeStepIds, + }) === undefined; } export interface TaskCompletionBlockerOptions { diff --git a/packages/engine/src/__tests__/project-engine-merge-lane-resolved.test.ts b/packages/engine/src/__tests__/project-engine-merge-lane-resolved.test.ts index 34831acce9..cba791144e 100644 --- a/packages/engine/src/__tests__/project-engine-merge-lane-resolved.test.ts +++ b/packages/engine/src/__tests__/project-engine-merge-lane-resolved.test.ts @@ -26,7 +26,7 @@ REVERT PROOF, measured: restore the literal and the renamed-board case returns ` routing to `onMerge`. */ import { describe, expect, it, vi } from "vitest"; -import type { Settings, Task, TaskStore, WorkflowIr } from "@fusion/core"; +import { getTaskMergeBlocker, type Settings, type Task, type TaskStore, type WorkflowIr } from "@fusion/core"; import { ProjectEngine } from "../project-engine.js"; @@ -191,4 +191,131 @@ describe("the other merge-lane surfaces on a renamed board", () => { expect([...self.pausedReviewTaskIds]).toEqual([]); }); + + /* + FNXC:MergeReadiness 2026-08-23-18:49: + ProjectEngine's live admission callbacks must receive the same resolved merge lane used by their + surrounding column guard. Passing only the task makes the injected core blocker silently restore the + `in-review` literal and prevents a renamed-lane card from re-entering the production merge queue. + */ + it("re-enqueues an unpaused renamed-lane card through the live merge blocker", async () => { + const { store, task } = storeFor("signoff", RENAMED_IR); + const self = { + options: { getTaskMergeBlocker }, + pausedReviewTaskIds: new Set([task.id]), + mergeQueue: [] as string[], + mergeActive: new Set(), + activeMergeTaskId: null as string | null, + taskUpdatedHandler: undefined as unknown, + abortActiveMerge: vi.fn(), + allowInReviewMergeProcessing: vi.fn(async () => true), + classifyMergeSweepCandidate: vi.fn(async () => ({ admit: true })), + internalEnqueueMerge: vi.fn(), + }; + + (ProjectEngine.prototype as unknown as { + wireTaskPauseMergeInterruption: (this: unknown, s: TaskStore) => void; + }).wireTaskPauseMergeInterruption.call(self, store); + + await (self.taskUpdatedHandler as (t: Task) => Promise)({ ...task, paused: false } as Task); + + expect(self.internalEnqueueMerge).toHaveBeenCalledWith(task.id); + expect(self.options.getTaskMergeBlocker(task, { reviewColumns: new Set(["signoff"]) })).toBeUndefined(); + }); + + /* + FNXC:MergeReadiness 2026-08-23-20:25: + Lane forwarding is required at every live ProjectEngine merge-blocker door, not only unpause. The + periodic sweep, final dequeue, and both sides of the handoff grace window must give the blocker the + resolved `signoff` lane; otherwise any one of those callers can silently reject a renamed-lane card. + */ + it("passes the resolved lane from the periodic sweep into the merge blocker", async () => { + const { store, task } = storeFor("signoff", RENAMED_IR); + const blocker = vi.fn(() => "blocked for assertion"); + const self = { + options: { getTaskMergeBlocker: blocker, getMergeStrategy: vi.fn(() => "direct") }, + runtime: { getTaskStore: () => store }, + canMergeTask: (ProjectEngine.prototype as unknown as { canMergeTask: (...args: unknown[]) => boolean }).canMergeTask, + allowInReviewMergeProcessing: vi.fn(async () => true), + loadMergeSweepBatch: vi.fn(async () => ({})), + classifyMergeSweepCandidate: vi.fn(async () => ({ admit: true })), + mergeSweepHoldReasons: new Map(), + internalEnqueueMerge: vi.fn(), + }; + + const admitted = await (ProjectEngine.prototype as unknown as { + enqueueEligibleInReviewTasks: (this: unknown, tasks: readonly Task[], settings: Pick) => Promise; + }).enqueueEligibleInReviewTasks.call(self, [task], { autoMerge: true, maxAutoMergeRetries: 3 }); + + expect(admitted).toBe(0); + expect(blocker).toHaveBeenCalledWith(task, { reviewColumns: new Set(["signoff"]) }); + }); + + it("passes the resolved lane from the final dequeue into the merge blocker", async () => { + const { store, task } = storeFor("signoff", RENAMED_IR); + const blocker = vi.fn(() => "blocked for assertion"); + const mergeQueue = [task.id]; + const self = { + options: { getTaskMergeBlocker: blocker, getMergeStrategy: vi.fn(() => "direct") }, + runtime: { getTaskStore: () => store }, + config: { workingDirectory: "/unused" }, + mergeQueue, + coordinatorAdmittedMergeTaskIds: new Set(), + mergeRunning: false, + mergeRunningSince: 0, + mergeAbortController: null, + shuttingDown: false, + reconcileStaleMergeActive: vi.fn(), + getShadowMergeRequestCandidateId: vi.fn(async () => null), + pickNextMergeTaskId: vi.fn(async () => mergeQueue.shift()), + hasMergeResolvers: vi.fn(() => false), + allowInReviewMergeProcessing: vi.fn(async () => true), + canMergeTask: (ProjectEngine.prototype as unknown as { canMergeTask: (...args: unknown[]) => boolean }).canMergeTask, + schedulePrMergeRetry: vi.fn(), + clearActiveMergeClaim: vi.fn(), + clearMergeActive: vi.fn(), + }; + + await (ProjectEngine.prototype as unknown as { + drainMergeQueue: (this: unknown) => Promise; + }).drainMergeQueue.call(self); + + expect(blocker).toHaveBeenCalledWith(task, { reviewColumns: new Set(["signoff"]) }); + }); + + it("passes the resolved lane to both immediate and post-grace handoff blockers", async () => { + vi.useFakeTimers(); + try { + const { store, task } = storeFor("signoff", RENAMED_IR); + const blocker = vi.fn(() => undefined); + const self = { + options: { getTaskMergeBlocker: blocker }, + taskMovedHandler: undefined as unknown, + mergeActive: new Set(), + mergeQueue: [] as string[], + activeMergeTaskId: null as string | null, + clearMergeActive: vi.fn(), + allowInReviewMergeProcessing: vi.fn(async () => true), + classifyMergeSweepCandidate: vi.fn(async () => ({ admit: true })), + internalEnqueueMerge: vi.fn(), + }; + + (ProjectEngine.prototype as unknown as { + wireAutoMerge: (this: unknown, s: TaskStore, cwd: string) => void; + }).wireAutoMerge.call(self, store, "/unused"); + + await (self.taskMovedHandler as (event: { task: Task; to: string }) => Promise)({ task, to: "signoff" }); + + expect(blocker).toHaveBeenCalledTimes(1); + expect(blocker).toHaveBeenLastCalledWith(task, { reviewColumns: new Set(["signoff"]) }); + + await vi.runOnlyPendingTimersAsync(); + + expect(blocker).toHaveBeenCalledTimes(2); + expect(blocker).toHaveBeenLastCalledWith(task, { reviewColumns: new Set(["signoff"]) }); + expect(self.internalEnqueueMerge).toHaveBeenCalledWith(task.id); + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index ca795be035..54da46a5ea 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -447,7 +447,10 @@ export interface ProjectEngineOptions { * Returns the merge blocker reason for a task, or null/undefined if * the task is eligible for merge. Imported from @fusion/core. */ - getTaskMergeBlocker?: (task: Task) => string | null | undefined; + getTaskMergeBlocker?: ( + task: Task, + options?: { reviewColumns?: ReadonlySet }, + ) => string | null | undefined; /** * Callback for insight extraction run processing. * Invoked after CronRunner completes a memory insight extraction schedule. @@ -3160,14 +3163,20 @@ export class ProjectEngine { log?: Array<{ action?: string }>; updatedAt?: string | null; mergeDetails?: { mergeConfirmed?: boolean } | null; - }, maxAutoMergeRetries: number, isReviewColumn?: boolean, enforcePrRetryBackoff = false): boolean { + }, maxAutoMergeRetries: number, reviewColumns?: ReadonlySet, enforcePrRetryBackoff = false): boolean { // Merge-confirmed tasks use the fast-path finalizer, which applies blocker // checks after clearing transient status/error state. Once that path parks // a blocked task as failed, skip future auto-merge retries. if (task.mergeDetails?.mergeConfirmed) { return true; } - if (this.options.getTaskMergeBlocker?.(task as Task)) return false; + /* + FNXC:MergeReadiness 2026-08-23-18:49: + This shared admission predicate serves both the periodic sweep and the final queue dispatch. Forward + their resolved lane set into the injected core blocker so neither production path reverts to the + legacy `in-review` literal after its surrounding column check accepted a renamed merge lane. + */ + if (this.options.getTaskMergeBlocker?.(task as Task, { reviewColumns })) return false; // Terminal failure: don't let the cooldown sweep re-attempt a merge that // already gave up (verification cap, conflict-bounce cap, or non-conflict // error). The task is parked for human/follow-up intervention. @@ -3185,7 +3194,11 @@ export class ProjectEngine { } return ( (task.mergeRetries ?? 0) < maxAutoMergeRetries || - this.hasAutoHealableVerificationBufferFailure(task, maxAutoMergeRetries, isReviewColumn) || + this.hasAutoHealableVerificationBufferFailure( + task, + maxAutoMergeRetries, + reviewColumns === undefined ? undefined : reviewColumns.has(task.column), + ) || this.isRetryCooldownElapsed(task) ); } @@ -3459,7 +3472,7 @@ export class ProjectEngine { return this.canMergeTask( t as any, maxAutoMergeRetries, - reviewLane === undefined ? undefined : t.column === reviewLane, + reviewLane === undefined ? undefined : new Set([reviewLane]), enforcePrRetryBackoff, ); }) as Task[]; @@ -4143,7 +4156,7 @@ export class ProjectEngine { if (!this.canMergeTask( task as any, maxAutoMergeRetries, - mergeLoopReviewLane === undefined ? undefined : task.column === mergeLoopReviewLane, + mergeLoopReviewLane === undefined ? undefined : new Set([mergeLoopReviewLane]), pullRequestMerge, )) { // A queued retry can be rejected after an engine restart or a racing @@ -6106,7 +6119,7 @@ export class ProjectEngine { const handoffReviewColumn = (await resolveTaskLifecycleColumns(store, task.id))?.review ?? "in-review"; if (to !== handoffReviewColumn) return; if (task.paused) return; - if (this.options.getTaskMergeBlocker?.(task)) return; + if (this.options.getTaskMergeBlocker?.(task, { reviewColumns: new Set([handoffReviewColumn]) })) return; // Grace period before handing off to the merger. The executor's finally // block (session disposal, child-agent termination, in-flight reviewer @@ -6133,7 +6146,9 @@ export class ProjectEngine { runtimeLog.log(`Auto-merge handoff (${task.id}) skipped: task paused`); return; } - const blockerReason = this.options.getTaskMergeBlocker?.(latestTask); + const blockerReason = this.options.getTaskMergeBlocker?.(latestTask, { + reviewColumns: new Set([handoffReviewColumn]), + }); if (blockerReason) { runtimeLog.log(`Auto-merge handoff (${task.id}) skipped: ${blockerReason}`); return; @@ -6252,7 +6267,8 @@ export class ProjectEngine { this.taskUpdatedHandler = async (task: Task) => { /* FNXC:WorkflowLifecycleColumns 2026-08-01-19:25 (fleet): on a renamed board this dropped EVERY card from the paused-review set on its next update, so a merge paused mid-flight was never interrupted. */ - if (task.column !== ((await resolveTaskLifecycleColumns(store, task.id))?.review ?? "in-review")) { + const taskReviewColumn = (await resolveTaskLifecycleColumns(store, task.id))?.review ?? "in-review"; + if (task.column !== taskReviewColumn) { this.pausedReviewTaskIds.delete(task.id); return; } @@ -6291,7 +6307,7 @@ export class ProjectEngine { if (settings.globalPause || settings.enginePaused || !(await this.allowInReviewMergeProcessing(task, settings, store))) { return; } - if (this.options.getTaskMergeBlocker?.(task)) { + if (this.options.getTaskMergeBlocker?.(task, { reviewColumns: new Set([taskReviewColumn]) })) { return; }