engine: finalization parked ALREADY-MERGED work as failed on a renamed board (#2964)
**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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/finalize-parks-merged-work-failed.md
Normal file
7
.changeset/finalize-parks-merged-work-failed.md
Normal file
@@ -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.
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string> = 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<Task, "column" | "paused" | "status" | "error" | "steps" | "workflowStepResults">,
|
||||
options: { reviewColumns?: ReadonlySet<string> } = {},
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user