From 126cee7e6d7a70fee2c11b775b5d2171772691ee Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 22:06:24 -0700 Subject: [PATCH] engine: finalization parked ALREADY-MERGED work as failed on a renamed board (#2964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The worst symptom in this family: the branch landed, and the board says the task failed.** `project-engine`'s merge-confirmed finalization spread the task's **real** column into `getTaskHardMergeBlocker` with no `reviewColumns`, so the identity check ran against the literal `in-review`. On a renamed board it returned `task is in 'signoff', must be in 'in-review'`, and the caller parked the card: ``` status: "failed" error: "Merge confirmed but finalization blocked: task is in 'signoff', must be in 'in-review'" ``` For work that had already merged. ## Its sibling had already solved this `auto-merge-finalization.ts` passes the **review-eligible sentinel** instead of the card's own column, with the reasoning recorded at that site: `getTaskHardMergeBlocker` asks *"is this card blocked by anything other than where it sits?"*, and its callers are recovery paths for landed work that a graph crash can leave resting in any column. `project-engine` simply never got the same treatment. ## One name instead of two spellings Rather than write the sentinel a second time, it is exported once as `REVIEW_ELIGIBLE_SENTINEL_COLUMN` next to the helper whose contract gives it meaning, and both recovery paths use it. **Two sites independently spelling a magic value is how one of them came to be missing it** — that is the actual root cause here, not the literal itself. This also answers the census, which flagged the new literal — correctly. Its guidance (which I wrote, in #2909) is to hoist a deliberate literal into a *declaration*, where a `DELIBERATE-LITERAL` marker actually attaches, instead of leaving it mid-expression where the marker is silently ignored. The shared constant is exactly that, and it lowers `auto-merge-finalization`'s literal count too. ## Revert result | | reverted → | | --- | --- | | sentinel replaced by the card's own renamed column | reproduces the shipped string | The middle test asserts that string deliberately — it is what landed in `task.error`, so a regression reports what the operator would actually have seen. A third case checks the sentinel does **not** suppress genuine blockers: incomplete steps still block finalization in any lane. These drive the helper directly; reaching `project-engine`'s finalization end to end needs a live engine, a merge run and a real repo, while the defect is entirely in *what the blocker is asked*. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `project-engine` + `auto-merge-finalization` + the new suite, 207; `tsc` clean on core and engine; lint, census `--strict`, FNXC gate, changesets all clean. ## Summary by CodeRabbit * **Bug Fixes** * Fixed merge-confirmed tasks being finalized correctly when boards use renamed workflow columns. * Prevented already-merged tasks from being incorrectly marked as failed due to custom review-column names. * Preserved enforcement of genuine incomplete-step blockers. * **Tests** * Added coverage for finalization on renamed lanes and legitimate merge blockers. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../finalize-parks-merged-work-failed.md | 7 ++ packages/core/src/index.gate.ts | 3 + packages/core/src/index.ts | 3 + packages/core/src/task-merge.ts | 43 +++++++ ...alize-merge-confirmed-renamed-lane.test.ts | 108 ++++++++++++++++++ .../engine/src/auto-merge-finalization.ts | 8 +- packages/engine/src/project-engine.ts | 20 +++- .../lib/lifecycle-column-census-baseline.json | 1 - 8 files changed, 187 insertions(+), 6 deletions(-) create mode 100644 .changeset/finalize-parks-merged-work-failed.md create mode 100644 packages/engine/src/__tests__/finalize-merge-confirmed-renamed-lane.test.ts diff --git a/.changeset/finalize-parks-merged-work-failed.md b/.changeset/finalize-parks-merged-work-failed.md new file mode 100644 index 0000000000..929746fe3b --- /dev/null +++ b/.changeset/finalize-parks-merged-work-failed.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Merge-confirmed tasks finalize instead of being parked failed on renamed boards. +category: fix +dev: `project-engine`'s merge-confirmed finalization passed the card's real column to `getTaskHardMergeBlocker` with no `reviewColumns`, so on a renamed board the identity check returned a blocker and already-landed work was parked `failed`. Both recovery paths now share an exported `REVIEW_ELIGIBLE_SENTINEL_COLUMN` instead of spelling the sentinel independently. diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index 61de81b32c..234c2562f5 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -1004,6 +1004,9 @@ export { getPrimaryPrInfo, taskHasManualOpenPullRequest } from "./task-helpers.j export { getTaskMergeBlocker, getTaskHardMergeBlocker, + REVIEW_ELIGIBLE_SENTINEL_COLUMN, + MERGE_CONFIRMED_TRANSIENT_STATUSES, + clearMergeConfirmedTransientStatus, getTaskCompletionBlocker, isTaskReadyForMerge, allowsAutoMergeProcessing, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 58757013e2..6605686879 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1130,6 +1130,9 @@ export { getPrimaryPrInfo, taskHasManualOpenPullRequest } from "./task-helpers.j export { getTaskMergeBlocker, getTaskHardMergeBlocker, + REVIEW_ELIGIBLE_SENTINEL_COLUMN, + MERGE_CONFIRMED_TRANSIENT_STATUSES, + clearMergeConfirmedTransientStatus, getTaskCompletionBlocker, getLatestFailedPreMergeReviewStep, isTaskReadyForMerge, diff --git a/packages/core/src/task-merge.ts b/packages/core/src/task-merge.ts index 3ad58c3adc..1317b499eb 100644 --- a/packages/core/src/task-merge.ts +++ b/packages/core/src/task-merge.ts @@ -360,6 +360,49 @@ QUERY was also a literal, so the unwired check was unreachable and therefore unn sweep's query ACTIVATES it — the sweep starts finding renamed-board cards and this then declines every one. Optional, so no caller changes behaviour until it passes the set. */ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-21:10: +DELIBERATE-LITERAL — the review-eligible SENTINEL, reviewed 2026-07-30-18:20. + +NOT a lifecycle column. `getTaskHardMergeBlocker` answers "is this card blocked by anything other than +where it sits?", and its callers are recovery paths for work that has ALREADY LANDED — a merge-confirmed +card whose graph crashed can be resting in any column. They pass this sentinel so the identity check is +satisfied by construction and the real blockers (paused / blocking status / incomplete steps / failed +pre-merge steps) remain the sole deciders. + +Named and exported because two recovery paths were spelling it independently, and one of them +(`project-engine.ts`) forgot to and instead passed the card's own column — which on a renamed board +parked already-merged work as `failed` with "Merge confirmed but finalization blocked: task is in +'signoff', must be in 'in-review'". One name, one meaning, one place to find it. +*/ +export const REVIEW_ELIGIBLE_SENTINEL_COLUMN = "in-review"; + +/* +FNXC:WorkflowMerge 2026-07-30-21:25 (#2964 review — coderabbitai, "normalize `queued` before the +sentinel blocker check"): ONE SPELLING OF "TRANSIENT ON ALREADY-LANDED WORK", NOT TWO. + +`mergeConfirmed` means the branch HAS landed. The statuses below are in-flight bookkeeping the graph +never got to clear, so on a merge-confirmed card they are soft state to drop — not hard blockers that +park finished work `failed`. + +Extracted because the two finalization paths were spelling the set independently and had already +DIVERGED: `auto-merge-finalization.ts` cleared `queued`, `project-engine.ts` cleared only the two +`merging*` values. `queued` is a BLOCKING status (`SCHEDULER_TRANSIENT_STATUSES`), so a merge-confirmed +card the scheduler had queued reached the blocker check with it intact and got parked `failed` — the +same "already-landed work parked failed" bug this change fixes for renamed columns, surviving one layer +down. Same failure shape as the sentinel above, same remedy: one name, one meaning. +*/ +export const MERGE_CONFIRMED_TRANSIENT_STATUSES: ReadonlySet = new Set([ + "merging", + "merging-pr", + "queued", +]); + +/** Status a merge-confirmed card should be judged on: transient in-flight bookkeeping cleared. */ +export function clearMergeConfirmedTransientStatus(status: string | undefined): string | undefined { + return status !== undefined && MERGE_CONFIRMED_TRANSIENT_STATUSES.has(status) ? undefined : status; +} + export function getTaskHardMergeBlocker( task: Pick, options: { reviewColumns?: ReadonlySet } = {}, diff --git a/packages/engine/src/__tests__/finalize-merge-confirmed-renamed-lane.test.ts b/packages/engine/src/__tests__/finalize-merge-confirmed-renamed-lane.test.ts new file mode 100644 index 0000000000..4a63d27d8a --- /dev/null +++ b/packages/engine/src/__tests__/finalize-merge-confirmed-renamed-lane.test.ts @@ -0,0 +1,108 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-18:10 (finalization parked ALREADY-MERGED work as failed): + +`project-engine.ts`'s merge-confirmed finalization spread the task's REAL column into +`getTaskHardMergeBlocker` and supplied no `reviewColumns`, so the identity check ran against the literal +`in-review`. On a board whose review lane is renamed it returned + + task is in 'signoff', must be in 'in-review' + +and the caller parked the card `failed` with "Merge confirmed but finalization blocked" — for a branch +that had ALREADY LANDED. The worst symptom in this family: the work is merged and the board says it +failed. + +The sibling recovery path in `auto-merge-finalization.ts` had already solved this by passing the +review-eligible SENTINEL column rather than the card's own, with the rationale recorded at that site. +This pins the shared contract both paths now rely on. + +WHY THE SEAM. Driving `project-engine`'s finalization end to end needs a live engine, a merge run and a +real repo. The defect is entirely in what the blocker is asked, so the helper is where it is decidable — +and the assertion below is the exact string the operator saw in `task.error`. + +REVERT CHECK, measured: pass the renamed column instead of the sentinel and the first case fails with +that string. +*/ +import { describe, expect, it } from "vitest"; +import { getTaskHardMergeBlocker, clearMergeConfirmedTransientStatus } from "@fusion/core"; +import type { Task } from "@fusion/core"; + +/** A merge-confirmed card as finalization sees it: landed, steps done, transient status cleared. */ +function mergeConfirmedCard(column: string): Task { + return { + id: "FN-LANDED", + column, + paused: false, + status: undefined, + error: undefined, + steps: [{ id: "s1", status: "done" }], + workflowStepResults: [], + } as unknown as Task; +} + +describe("merge-confirmed finalization does not park landed work as failed", () => { + it("reports no blocker for a landed card evaluated as review-eligible", () => { + /* What both finalization paths now do: judge the blocker set, not the card's column identity. */ + const blocker = getTaskHardMergeBlocker({ ...mergeConfirmedCard("signoff"), column: "in-review" }); + + expect(blocker).toBeUndefined(); + }); + + it("reproduces the shipped failure when the card's own renamed column is used", () => { + /* This string went into `task.error` behind "Merge confirmed but finalization blocked: …". */ + const blocker = getTaskHardMergeBlocker(mergeConfirmedCard("signoff")); + + expect(blocker).toBe("task is in 'signoff', must be in 'in-review'"); + }); + + /* + FNXC:WorkflowMerge 2026-07-30-21:35 (#2964 review — coderabbitai): THE SECOND HALF OF THE SAME BUG. + + The two finalization paths normalized transient status independently and diverged: + auto-merge-finalization cleared `queued`, project-engine did not. `queued` BLOCKS + (`SCHEDULER_TRANSIENT_STATUSES`), so a merge-confirmed card the scheduler had queued reached the + blocker check with it intact and was parked `failed` — already-landed work, same as the column bug, + one layer down. Asserted through the shared helper both paths now call, so a future third caller + cannot re-diverge silently. + */ + it.each(["merging", "merging-pr", "queued"])( + "clears transient status %s on a merge-confirmed card, so finalization is not blocked", + (status) => { + expect(clearMergeConfirmedTransientStatus(status)).toBeUndefined(); + + const blocker = getTaskHardMergeBlocker({ + ...mergeConfirmedCard("signoff"), + column: "in-review", + status: clearMergeConfirmedTransientStatus(status), + } as unknown as Task); + + expect(blocker).toBeUndefined(); + }, + ); + + it("does NOT clear a genuinely blocking status — the paired negative", () => { + /* Without this, a helper that returned undefined unconditionally would pass the cases above. */ + expect(clearMergeConfirmedTransientStatus("stuck-killed")).toBe("stuck-killed"); + + const blocker = getTaskHardMergeBlocker({ + ...mergeConfirmedCard("signoff"), + column: "in-review", + status: clearMergeConfirmedTransientStatus("stuck-killed"), + } as unknown as Task); + + expect(blocker).toBeDefined(); + }); + + it("still reports real blockers on a landed card", () => { + /* + Non-vacuous companion: evaluating as review-eligible must not suppress genuine blockers. Incomplete + steps still block finalization regardless of which lane the card sits in. + */ + const blocker = getTaskHardMergeBlocker({ + ...mergeConfirmedCard("signoff"), + column: "in-review", + steps: [{ id: "s1", status: "in-progress" }], + } as unknown as Task); + + expect(blocker).toBe("task has incomplete steps"); + }); +}); diff --git a/packages/engine/src/auto-merge-finalization.ts b/packages/engine/src/auto-merge-finalization.ts index ebf770726e..5e0ca79523 100644 --- a/packages/engine/src/auto-merge-finalization.ts +++ b/packages/engine/src/auto-merge-finalization.ts @@ -1,4 +1,6 @@ -import { getTaskHardMergeBlocker, resolveWorkflowIrForTask, resolveCompleteColumn, resolveMergeOrchestrationColumn, columnHasFlag, type MergeResult, type Task, type TaskStore } from "@fusion/core"; +import { getTaskHardMergeBlocker, resolveWorkflowIrForTask, resolveCompleteColumn, resolveMergeOrchestrationColumn, columnHasFlag, type MergeResult, type Task, type TaskStore, REVIEW_ELIGIBLE_SENTINEL_COLUMN, + clearMergeConfirmedTransientStatus, +} from "@fusion/core"; import { createRunAuditor, generateSyntheticRunId, type DatabaseMutationType, type RunAuditor } from "./run-audit.js"; /* @@ -252,9 +254,9 @@ export async function finalizeProvenAutoMergeTask({ NOT re-keyed to the merge-orchestration column so custom workflows evaluate the same review-eligible blocker set as builtin. */ - column: "in-review", + column: REVIEW_ELIGIBLE_SENTINEL_COLUMN, paused: false, - status: latest.status === "merging" || latest.status === "merging-pr" || latest.status === "queued" ? undefined : latest.status, + status: clearMergeConfirmedTransientStatus(latest.status), error: undefined, }); if (hardBlocker) { diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index de7d25013b..c9b1da5600 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -43,7 +43,8 @@ import { resolveTaskSessionAdvisorEnabled, sortTasksByPriorityThenAgeAndId, resolveWipTargetForTask, - resolveReboundTargetForTask, + resolveReboundTargetForTask, REVIEW_ELIGIBLE_SENTINEL_COLUMN, + clearMergeConfirmedTransientStatus, } from "@fusion/core"; import { assemblePlannerOverseerRuntimeSnapshot } from "./planner-overseer-runtime-snapshot.js"; import { execFile } from "node:child_process"; @@ -3585,11 +3586,26 @@ export class ProjectEngine { } // end !isWorkspaceTask reachability gate (B2): workspace tasks skip the root-cwd commitSha check const blockerReason = getTaskHardMergeBlocker({ ...(task as Task), + /* + FNXC:WorkflowResolvedColumns 2026-07-30-18:05 (this parked ALREADY-MERGED work as failed): + The spread carries the task's REAL column, and no `reviewColumns` was supplied, so + getTaskHardMergeBlocker's identity check ran against the literal `in-review`. On a board + whose review lane is renamed that returned `task is in 'signoff', must be in 'in-review'` + and the branch below parked the card FAILED with "Merge confirmed but finalization + blocked" — for work that had already landed. + + Fixed the way the sibling recovery path in auto-merge-finalization.ts already does it, + and for the reason recorded there: `"in-review"` is the review-eligible SENTINEL for this + helper, not a lifecycle column, so a merge-confirmed card evaluates the same blocker set + on a custom workflow as on the builtin one. The column identity of an already-landed card + is not what this check is for — paused / error / incomplete steps still apply. + */ + column: REVIEW_ELIGIBLE_SENTINEL_COLUMN, // Merge-confirmed tasks have already landed. Treat stale merge // in-flight statuses as soft state to clear during finalization, // not hard blockers that park an otherwise confirmed merge as failed. paused: false, - status: task.status === "merging" || task.status === "merging-pr" ? undefined : task.status, + status: clearMergeConfirmedTransientStatus(task.status), error: undefined, }); if (blockerReason) { diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 66e5076d55..9e6a449014 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -140,7 +140,6 @@ "packages/core/src/task-store/async-archive-lineage.ts": 1, "packages/core/src/task-store/async-self-healing.ts": 1, "packages/engine/src/agent-tools.ts": 1, - "packages/engine/src/auto-merge-finalization.ts": 1, "packages/engine/src/self-healing.ts": 1 } }