From ac67b8d585546631278d9d0464823adeceb4dbb8 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 23:49:05 -0700 Subject: [PATCH] fix(scripts): the FN-4000 consistency reconciler failed in BOTH directions on a renamed board (#2994) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## The FN-4000 consistency reconciler failed in *both* directions `findTaskStateInconsistencies` keyed both checks on legacy lane literals, and they break in opposite ways: ```js const hasDoneTransient = task.column === "done" && (status failed || error || worktree || blockedBy || …); if (task.status === "failed" && task.column !== "in-review") { … } ``` | check | on a renamed board | effect | | --- | --- | --- | | `hasDoneTransient` | **never fires** | a finished card still holding `status:"failed"`, a worktree, a blockedBy or live recovery counters is never reported and never normalized — precisely the stale state FN-4000 exists to clear | | `failed-status-outside-in-review` | **fires for every failed card** | no column equals the literal, so the report lists the whole board | The second is the more dangerous of the two: a tool that reports nothing looks broken, but a tool that reports everything looks like it is working. ## Wiring, and why the resolver is injected rather than built inline Lanes are resolved **per task** (a board can span workflows) and passed in. Resolving inside the loop would drag `importCore()` — and therefore a built `packages/core/dist` — into every unit test of a pure reconciliation loop. `main` wires the real resolver whenever it opened a real backend, so this is **not** the inert optional-parameter shape this migration keeps finding. A caller injecting its own store (tests) has no staged dist and falls back to the documented legacy literals, which is exactly today's behaviour. `importCore` is now exported from `scripts/lib/backend-db.mjs` so operator scripts reach core helpers through the **same staged-dist seam `openBackend` already uses**, rather than each growing its own dist path — `@fusion/core` is not resolvable from repo-root `scripts/`, which is what made the obvious import fail. The normalization move now targets the card's **own** column: naming `"done"` was only ever a way of spelling *"where it already is"*, since the move exists to trigger the store's done-normalization. ## One of my test expectations was wrong before the code was My first version asserted that a card in a renamed complete lane with `status:"failed"` yields only the transient-state finding. It yields **both** — and that is correct, because a failed card outside the review lane genuinely is flagged. I isolated the case (dropping `status:"failed"`, keeping the worktree) so it pins one behaviour instead of blurring two, rather than "fixing" the expectation to match whatever came out. ## Revert proof Restoring the four literals: ``` ✖ reports stale transient state in a RENAMED complete lane ✖ does NOT flag a failed card that is sitting in the board's own review lane ✖ runReconciliation normalizes a renamed complete lane by moving the card to its OWN column ℹ pass 5 ℹ fail 3 ``` The remaining two new cases pass both ways by design — "still flags a failed card outside the resolved review lane" and "unresolved lanes keep exactly the legacy behaviour" guard against over-correction, so I am not counting them as coverage of the defect. ## Verification (measured) - `node --test` — **8 passed / 0 failed** (3 pre-existing + 5 new) - sibling script suites (`recover-stale-blocked-by`, `reconcile-leaked-soft-deletes`) — **7 passed**, unaffected by the shared-lib export - `node --check`, `eslint` — clean - `lifecycle-column-census --strict`, `check-sql-column-literals`, `check-lane-wiring`, `check-fnxc-future-dates` — green No changeset: root `scripts/` is repo tooling, not part of the published package. ## Still not addressed in this territory `reconcile-leaked-soft-deletes.mjs` carries a raw `UPDATE project."tasks" SET "column" = 'archived'` — on a renamed board that writes a column the workflow does not declare, creating the undeclared-column state this migration keeps repairing elsewhere. It holds a raw backend rather than a store, so it needs the same `importCore` seam this PR exports; left for a follow-up rather than bundled here. --- .../reconcile-task-state-consistency.test.mjs | 62 +++++++++++++++++++ scripts/lib/backend-db.mjs | 5 +- scripts/reconcile-task-state-consistency.mjs | 61 +++++++++++++++--- 3 files changed, 118 insertions(+), 10 deletions(-) diff --git a/scripts/__tests__/reconcile-task-state-consistency.test.mjs b/scripts/__tests__/reconcile-task-state-consistency.test.mjs index b133b2b87f..4404a2037c 100644 --- a/scripts/__tests__/reconcile-task-state-consistency.test.mjs +++ b/scripts/__tests__/reconcile-task-state-consistency.test.mjs @@ -104,3 +104,65 @@ test("apply reconciles done task and emits exactly one note", async () => { assert.equal(task.recoveryRetryCount, undefined); assert.equal(task.nextRecoveryAt, undefined); }); + +/* +FNXC:OperatorScriptLaneAssumptions 2026-07-30-26:10: +THE INVARIANT: both consistency checks ask the task's OWN lanes, and they failed in OPPOSITE directions. + +Keyed on the literals, `hasDoneTransient` (`column === "done"`) NEVER fires on a renamed board, so a +finished card still holding a worktree and `status:"failed"` goes unreported and unnormalized. Meanwhile +`failed-status-outside-in-review` (`column !== "in-review"`) fires for EVERY failed card, because no +column equals the literal — a report listing the whole board, which looks like the tool working. + +Reverted, the first case returns [] (the miss) and the second returns the spurious flag (the flood). +*/ +test("reports stale transient state in a RENAMED complete lane", () => { + /* No `status:"failed"` here on purpose: it would ALSO trip the second check (a failed card outside + the review lane is genuinely flagged), which would blur which of the two this case is pinning. */ + const task = { id: "FN-R1", column: "shipped", worktree: "/tmp/wt" }; + + assert.deepEqual( + findTaskStateInconsistencies(task, { complete: "shipped", review: "checking" }), + ["done-task-has-transient-failure-state"], + ); +}); + +test("does NOT flag a failed card that is sitting in the board's own review lane", () => { + const task = { id: "FN-R2", column: "checking", status: "failed" }; + + assert.deepEqual(findTaskStateInconsistencies(task, { complete: "shipped", review: "checking" }), []); +}); + +test("still flags a failed card outside the resolved review lane", () => { + const task = { id: "FN-R3", column: "building", status: "failed" }; + + assert.deepEqual( + findTaskStateInconsistencies(task, { complete: "shipped", review: "checking" }), + ["failed-status-outside-in-review"], + ); +}); + +test("unresolved lanes keep exactly the legacy behaviour", () => { + assert.deepEqual( + findTaskStateInconsistencies({ id: "FN-L", column: "done", status: "failed" }), + ["done-task-has-transient-failure-state", "failed-status-outside-in-review"], + ); +}); + +test("runReconciliation normalizes a renamed complete lane by moving the card to its OWN column", async () => { + const moves = []; + const store = { + async listTasks() { return [{ id: "FN-R4", column: "shipped", status: "failed", worktree: "/tmp/w" }]; }, + async moveTask(id, toColumn) { moves.push([id, toColumn]); return { id }; }, + async logEntry() { return { id: "FN-R4" }; }, + }; + + const result = await runReconciliation({ + store, + dryRun: false, + resolveLanes: async () => ({ complete: "shipped", review: "checking" }), + }); + + assert.deepEqual(moves, [["FN-R4", "shipped"]]); + assert.equal(result.actions[0].action, "reconciled"); +}); diff --git a/scripts/lib/backend-db.mjs b/scripts/lib/backend-db.mjs index 631fa11bee..370864155a 100644 --- a/scripts/lib/backend-db.mjs +++ b/scripts/lib/backend-db.mjs @@ -38,7 +38,10 @@ function ensureMigrationsStaged() { } } -async function importCore() { +/* FNXC:OperatorScriptLaneAssumptions 2026-07-30-26:10: exported so operator scripts can reach core + helpers (lane resolution) through the SAME staged-dist seam `openBackend` already uses, rather than + each growing its own dist path — `@fusion/core` is not resolvable from the repo-root `scripts/`. */ +export async function importCore() { ensureMigrationsStaged(); try { return await import(pathToFileURL(resolve(repoRoot, "packages/core/dist/index.js")).href); diff --git a/scripts/reconcile-task-state-consistency.mjs b/scripts/reconcile-task-state-consistency.mjs index 8aba15a57b..a9db9c0a86 100644 --- a/scripts/reconcile-task-state-consistency.mjs +++ b/scripts/reconcile-task-state-consistency.mjs @@ -1,12 +1,33 @@ #!/usr/bin/env node import process from "node:process"; -import { openBackend } from "./lib/backend-db.mjs"; +import { openBackend, importCore } from "./lib/backend-db.mjs"; const DEFAULT_NOTE = "FN-4000 reconciliation: cleared stale transient failure state using TaskStore done-normalization so database and task JSON remain synchronized."; -export function findTaskStateInconsistencies(task) { +/* +FNXC:OperatorScriptLaneAssumptions 2026-07-30-26:10: +Both checks ask this task's OWN lanes, because keyed on the literals they fail in BOTH directions. + + `hasDoneTransient` gated on `column === "done"`. On a board whose complete lane is named anything + else it NEVER fires, so a finished card still carrying `status:"failed"`, a worktree, a blockedBy or + live recovery counters is never reported and never normalized — the exact stale state FN-4000 exists + to clear. + + `failed-status-outside-in-review` gated on `column !== "in-review"`, and fails the OPPOSITE way: on a + renamed board no column equals the literal, so EVERY failed task is flagged. A reconciliation report + listing the whole board is as useless as one listing nothing, and it is the more dangerous of the + two because it looks like the tool is working. + +Lanes arrive resolved from the caller rather than being resolved here, so this stays pure and testable +without a database or a built dist. +*/ +export function findTaskStateInconsistencies(task, lanes = {}) { + /* DELIBERATE-LITERAL — the degraded default when the caller resolved no lanes. */ + const completeColumn = lanes.complete ?? "done"; + /* DELIBERATE-LITERAL — as above. */ + const reviewColumn = lanes.review ?? "in-review"; const findings = []; - const hasDoneTransient = task.column === "done" && ( + const hasDoneTransient = task.column === completeColumn && ( task.status === "failed" || Boolean(task.error) || Boolean(task.worktree) @@ -19,20 +40,30 @@ export function findTaskStateInconsistencies(task) { findings.push("done-task-has-transient-failure-state"); } - if (task.status === "failed" && task.column !== "in-review") { + if (task.status === "failed" && task.column !== reviewColumn) { findings.push("failed-status-outside-in-review"); } return findings; } -export async function runReconciliation({ store, dryRun = true, noteByTaskId = {} }) { +/* +FNXC:OperatorScriptLaneAssumptions 2026-07-30-26:10: +`resolveLanes` is INJECTED, not built here, and `main` below supplies the real one. + +Resolving inside this function would drag `importCore()` — and therefore a built `packages/core/dist` +— into every unit test of a pure reconciliation loop. Injection keeps the tests database-free and +build-free while the production entry point still wires a real resolver, which is the wiring that +matters: an optional lane parameter no caller fills is the inert shape this migration keeps finding. +*/ +export async function runReconciliation({ store, dryRun = true, noteByTaskId = {}, resolveLanes = null }) { const tasks = await store.listTasks({ includeArchived: false }); const findings = []; const actions = []; for (const task of tasks) { - const issues = findTaskStateInconsistencies(task); + const lanes = resolveLanes ? ((await resolveLanes(task.id)) ?? {}) : {}; + const issues = findTaskStateInconsistencies(task, lanes); if (issues.length === 0) continue; findings.push({ taskId: task.id, column: task.column, status: task.status ?? null, issues }); @@ -42,8 +73,10 @@ export async function runReconciliation({ store, dryRun = true, noteByTaskId = { continue; } - if (task.column === "done") { - await store.moveTask(task.id, "done"); + if (task.column === (lanes.complete ?? "done")) { + /* Its OWN column: this move exists to trigger the store's done-normalization, so naming the + destination by literal was only ever a way of spelling "where it already is". */ + await store.moveTask(task.id, task.column); const note = noteByTaskId[task.id] ?? DEFAULT_NOTE; await store.logEntry(task.id, "FN-4000 reconciliation", note); actions.push({ taskId: task.id, action: "reconciled", issues }); @@ -62,6 +95,13 @@ function readFlagValue(argv, flag) { return argv[index + 1]; } +async function buildLaneResolver(store) { + const { resolveTaskLifecycleColumns } = await importCore(); + /* One resolution per WORKFLOW, not per task — the cache is what keeps this loop cheap on a big board. */ + const irCache = new Map(); + return (taskId) => resolveTaskLifecycleColumns(store, taskId, irCache); +} + export async function main(argv = process.argv.slice(2), deps = {}) { const dryRun = !argv.includes("--apply"); const projectDir = readFlagValue(argv, "--project-dir") ?? process.cwd(); @@ -74,7 +114,10 @@ export async function main(argv = process.argv.slice(2), deps = {}) { try { /* FNXC:PostgresOperationalScripts 2026-07-14-18:18: Consistency reconciliation must inspect and repair the authoritative PostgreSQL rows. */ - const result = await runReconciliation({ store, dryRun, noteByTaskId }); + /* Wired only when we opened a real backend: a caller injecting its own store (tests) has no + staged dist to import, and falls back to the documented legacy literals. */ + const resolveLanes = deps.resolveLanes ?? (backend ? await buildLaneResolver(store) : null); + const result = await runReconciliation({ store, dryRun, noteByTaskId, resolveLanes }); console.log(JSON.stringify({ dryRun, ...result }, null, 2)); return 0; } finally {