fix(scripts): the FN-4000 consistency reconciler failed in BOTH directions on a renamed board (#2994)

## 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.
2026-07-30 23:49:05 -07:00
parent 65f4e8533e
commit ac67b8d585
3 changed files with 118 additions and 10 deletions
--- 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");
+});
--- 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);
--- 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 {