From d448ab6951f37ed76b94e9837dc18034d14ef4a5 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 04:22:33 -0700 Subject: [PATCH] fix(engine): the merge-refusal reason was classified by a column id, and it lands in run-audit (#3098) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claimed `auto-merge-finalization.ts` — and this one is a **reversal of an earlier audit in the same file**, which is the interesting part. ## The earlier note said "diagnostic only". It was wrong about the consequence `validateWorkflowDoneMergeProof` picks between two refusal reasons with `task.column === "done"`. Both arms return `{ ok: false }`, so this never changed which branch ran — and on that basis a prior pass recorded it as *"REAL but DIAGNOSTIC-ONLY"* and declined it, reasoning that widening a signature to improve an error string is a poor trade. **The reason is not an error string.** It is written to run-audit metadata alongside `previousColumn` — `merger-merge-lifecycle.test.ts` asserts exactly that — and that row is what an operator reads to find out why a merge was refused. So on a board whose complete lane is not called `done`, a card resting 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 recorded the opposite of what happened. A wrong record is worse than a vague one, because it gets acted on. ## The trade was also cheaper than the note claimed The function is **already async** and **already takes an options bag**. `resolveFinalizationColumns`, two functions up in the same file, **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 — the half-conversion shape this program keeps finding, here inside a single file, one line apart: the caller guards on the resolved `isCompleteColumn(latest.column)`, then calls a validator that re-asked the same question with the literal. `isCompleteColumn` is **optional with the legacy literal as its default** — the same default-to-legacy contract the lane-parameter vocabulary uses elsewhere — and `check-lane-wiring` watches the parameter, so the two call sites cannot silently stop passing it. ## Measured - New `merge-proof-reason-renamed-complete-lane.test.ts` — **2 pass**. - **MUTATION**: dropping the parameter fails the renamed case and leaves the legacy **control** green. The control earns its place: a failure now means *"renamed board"*, not *"the refusal stopped working"*. - **Driven through `finalizeProvenAutoMergeTask`**, not by calling the validator with the new argument. The contract under test is the **wiring** — a test that passed the argument directly would assert my own parameter works and prove nothing about the seam that was broken. - **The audit row is asserted, not just the return value.** The return value alone is not the contract that failed here. - merger / auto-merge suites — **5 files / 159 tests pass**. - `tsc --noEmit -p packages/engine` clean; census `--strict`, `check-lane-wiring`, `check-inert-sync-lane-conversions`, `check-fnxc-future-dates` clean. ## Census `auto-merge-finalization.ts` stays at **2**, deliberately. Both remaining entries are now documented **degraded-fallback arms** — the resolver's `catch` and this parameter's default — which is the right kind of literal rather than a missed conversion. Converting a fallback to a resolution would defeat its purpose. ## A note on the FNXC gate My first stamps were dated `2026-08-01` while local today is `2026-07-31`. `check-fnxc-future-dates` caught it and I re-stamped. Worth mentioning because it is the second time this session that a date-only local-calendar comparison has caught a stamp written near midnight — the gate is doing real work, not ceremony. Co-authored-by: Claude Opus 5 (1M context) --- ...proof-reason-renamed-complete-lane.test.ts | 113 ++++++++++++++++++ .../engine/src/auto-merge-finalization.ts | 40 +++++-- 2 files changed, 142 insertions(+), 11 deletions(-) create mode 100644 packages/engine/src/__tests__/merge-proof-reason-renamed-complete-lane.test.ts 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({