diff --git a/packages/engine/src/__tests__/merge-proof-reason-renamed-complete-lane.test.ts b/packages/engine/src/__tests__/merge-proof-reason-renamed-complete-lane.test.ts new file mode 100644 index 0000000000..d13589f539 --- /dev/null +++ b/packages/engine/src/__tests__/merge-proof-reason-renamed-complete-lane.test.ts @@ -0,0 +1,113 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-31-23:40: +THE MERGE-REFUSAL REASON WAS CLASSIFIED BY A COLUMN ID. + +`validateWorkflowDoneMergeProof` picks between two refusal reasons with +`task.column === "done"`. Both arms return `{ ok: false }`, so this never changed WHICH branch ran — +which is why an earlier audit recorded it as "diagnostic only" and declined it. + +That undersold it. The reason is not a log line: it is written to run-audit metadata alongside +`previousColumn`, and that record is what an operator reads to find out why a merge was refused. On a +board whose complete lane is not called `done`, a card sitting in that lane was refused with the +generic `missing-merge-confirmation` — the classification for a card that is NOT in the complete lane +at all. The audit trail said the opposite of what happened. + +Driven through `finalizeProvenAutoMergeTask` rather than by calling the validator with the new +argument, because the contract this pins is the WIRING: the caller already resolves +`isCompleteColumn` for its own guard one line earlier, and the defect was that it did not hand that +answer down. A test that passed the argument directly would assert my own parameter works and prove +nothing about the seam. +*/ + +import { describe, expect, it, vi } from "vitest"; +import type { MergeResult, Task, TaskStore, WorkflowIr } from "@fusion/core"; +import { finalizeProvenAutoMergeTask } from "../auto-merge-finalization.js"; + +/** Complete lane is `shipped`; the board declares no column called `done`. */ +const RENAMED_IR = { + version: "v2", id: "wf-renamed", name: "renamed", nodes: [], edges: [], + columns: [ + { id: "drafting", name: "Drafting", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "checking", name: "Checking", traits: [{ trait: "merge" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], +} as unknown as WorkflowIr; + +/** A card resting in a completion lane with NO durable merge proof — the refused case. */ +function completedWithoutProof(column: string): Task { + return { + id: "FN-NOPROOF", + title: "landed without proof", + description: "t", + column, + dependencies: [], + steps: [{ status: "done" }], + currentStep: 0, + log: [], + createdAt: "2026-06-01T00:00:00.000Z", + updatedAt: "2026-06-01T00:00:00.000Z", + mergeDetails: { mergeConfirmed: false, noOpMerge: true }, + } as unknown as Task; +} + +function storeFor(task: Task, ir?: WorkflowIr) { + const recordRunAuditEvent = vi.fn(async () => undefined); + const store = { + getTask: vi.fn(async () => task), + updateTask: vi.fn(async () => task), + moveTask: vi.fn(async () => task), + logEntry: vi.fn(async () => undefined), + getSettings: vi.fn(async () => ({})), + recordRunAuditEvent, + /* Absent → the resolver's documented degraded fallback, i.e. the legacy `done` answer. */ + ...(ir + ? { + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "wf-renamed", stepIds: [] })), + getWorkflowDefinition: vi.fn(async () => ({ id: "wf-renamed", ir })), + } + : {}), + } as unknown as TaskStore; + return { store, recordRunAuditEvent }; +} + +async function finalize(task: Task, ir?: WorkflowIr) { + const { store, recordRunAuditEvent } = storeFor(task, ir); + const result = await finalizeProvenAutoMergeTask({ + store, + taskId: task.id, + result: { task, ok: true, merged: true, noOp: true } as unknown as MergeResult, + source: "workflow-graph-merge-finalize", + }); + return { result, recordRunAuditEvent }; +} + +describe("the merge-proof refusal reason names the board's own complete lane", () => { + /* + CONTROL. A default board answers `column === "done"` either way, so this passes with or without + the fix — it is here so a failure below means "renamed board", not "the refusal stopped working". + */ + it("classifies a proof-less card in the legacy `done` column (control)", async () => { + const { result } = await finalize(completedWithoutProof("done")); + + expect(result).toEqual(expect.objectContaining({ + outcome: "blocked", reason: "done-without-merge-confirmation", + })); + }); + + it("classifies a proof-less card in a RENAMED complete lane the same way", async () => { + const { result, recordRunAuditEvent } = await finalize(completedWithoutProof("shipped"), RENAMED_IR); + + /* Against the literal this was `missing-merge-confirmation` — the classification for a card that + is not in the complete lane at all, which is the opposite of what happened. */ + expect(result).toEqual(expect.objectContaining({ + outcome: "blocked", reason: "done-without-merge-confirmation", + })); + + /* The audit row is the artifact an operator actually reads; the return value alone is not the + contract that was broken. */ + expect(recordRunAuditEvent).toHaveBeenCalledWith(expect.objectContaining({ + metadata: expect.objectContaining({ previousColumn: "shipped", reason: "done-without-merge-confirmation" }), + })); + }); +}); diff --git a/packages/engine/src/auto-merge-finalization.ts b/packages/engine/src/auto-merge-finalization.ts index 5e0ca79523..827cad87ee 100644 --- a/packages/engine/src/auto-merge-finalization.ts +++ b/packages/engine/src/auto-merge-finalization.ts @@ -80,23 +80,40 @@ function hasIncompleteWorkflowSteps(task: Task): boolean { export async function validateWorkflowDoneMergeProof( task: Task, - options: { result?: MergeResult; checkWorkflowSteps?: boolean } = {}, + options: { + result?: MergeResult; + checkWorkflowSteps?: boolean; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:20: + The RESOLVED complete test, supplied by the caller. Omitted → the `done` literal, i.e. today's + behaviour, which is the same default-to-legacy contract the lane-parameter vocabulary uses + elsewhere. `resolveFinalizationColumns` in this file already builds exactly this predicate for + its own guard; the two callers below now hand it down instead of re-asking with an id. + */ + isCompleteColumn?: (columnId: string) => boolean; + } = {}, ): Promise { const hasProof = hasDurableMergeProof(task, options.result); /* - FNXC:WorkflowLifecycleColumns 2026-07-31-02:45 (audited — REAL but DIAGNOSTIC-ONLY): + FNXC:WorkflowResolvedColumns 2026-07-31-23:25 (the deferral is now paid — see the note above): This literal selects which REASON STRING is reported, not which branch runs. Both arms return - `{ ok: false }`, so on a renamed board a card sitting in the complete lane is refused with the + `{ ok: false }`, so on a renamed board a card sitting in the complete lane was refused with the generic `missing-merge-confirmation` instead of the specific `done-without-merge-confirmation`. - Worth recording rather than converting from here: the resolver two functions up already computes - `isCompleteColumn` for exactly this workflow, and threading it in is the right fix — but this - function does not receive it, and widening the signature to improve an error string is a change - whose cost outweighs the diagnosis it sharpens. The other two census entries in this file are NOT - defects: the `columnId === "done"` at the top is the resolver's documented degraded fallback (the - live arm calls `columnHasFlag`), and the `step.status` comparison is a STEP status, not a column. + The earlier note recorded this as "REAL but DIAGNOSTIC-ONLY" and declined it on the grounds that + widening a signature to improve an error string is a poor trade. That undersold the consequence: + this reason is not a log line. It is asserted as run-audit metadata alongside `previousColumn` + (`merger-merge-lifecycle.test.ts`), so the audit trail — the record an operator reads to find out + why a merge was refused — carried the wrong classification for every renamed board. + + The trade is also cheaper than it looked. This function is ALREADY async and ALREADY takes an + options bag, and `resolveFinalizationColumns` two functions up ALREADY builds this exact predicate + for its own guard. Nothing new is resolved; the answer that existed is handed down instead of + being re-asked with an id — which is the half-conversion shape this program keeps finding, here + within one file. */ - if (!hasProof) return { ok: false, reason: task.column === "done" ? "done-without-merge-confirmation" : "missing-merge-confirmation" }; + const isCompleteLane = options.isCompleteColumn ? options.isCompleteColumn(task.column) : task.column === "done"; + if (!hasProof) return { ok: false, reason: isCompleteLane ? "done-without-merge-confirmation" : "missing-merge-confirmation" }; if (options.checkWorkflowSteps !== false && hasIncompleteWorkflowSteps(task)) { return { ok: false, reason: "incomplete-workflow-steps" }; } @@ -210,7 +227,7 @@ export async function finalizeProvenAutoMergeTask({ * Workflow-owned completion requires current merge proof, not just a stale `mergeConfirmed` flag. A task cannot reach or remain accepted as `done` when workflow steps are still pending or a no-op claims landed files. Branch-only residue is ignored because squash landing validates the task patch, not branch-history cleanliness. */ if (isCompleteColumn(latest.column)) { - const proofVerdict = await validateWorkflowDoneMergeProof({ ...latest, mergeDetails: validationMergeDetails } as Task, { result }); + const proofVerdict = await validateWorkflowDoneMergeProof({ ...latest, mergeDetails: validationMergeDetails } as Task, { result, isCompleteColumn }); if (!proofVerdict.ok) { await recordFinalizationAudit({ store, @@ -279,6 +296,7 @@ export async function finalizeProvenAutoMergeTask({ const proofVerdict = await validateWorkflowDoneMergeProof({ ...latest, mergeDetails } as Task, { result, checkWorkflowSteps: false, + isCompleteColumn, }); if (!proofVerdict.ok) { await recordFinalizationAudit({