fix(core): the review lane was resolved for two stall signals and literal for the other two (#3053)

## Claim Largest **unclaimed** census cluster. `self-healing.ts` (56) is the capacity worker's file and `scheduler.ts` (12) is blocked (below), so I took **@fusion/core** — 16 bare guards across 12 files, untouched by any open PR. ## Census before / after ``` before: COLUMN guards (the backlog): 126 after: COLUMN guards (the backlog): 126 ``` **Unchanged, and that is the honest result — not a failed conversion.** The repo's sanctioned device is an optional *resolved* parameter whose default stays the legacy literal (the exemplar is `restart-recovery-coordinator.ts`, watched by the unwired-lane-parameter guard). The literal survives as the default arm, so the counter cannot see the conversion. **This matters for the fleet phase.** The census is not a progress meter for this pattern. A worker driving the number down has only two ways to move it, and both are wrong: 1. **Delete the fallback** (make the parameter required) — prior review explicitly argued against this; `cli-active-count-lanes.test.ts` deliberately covers the no-argument path. 2. **"Convert" with `resolveTaskWorkflowIrSync`** — that reader returns `undefined` unconditionally under PostgreSQL, the shipped backend. It drops the count while behaving *exactly* like the literal. That is the inert-conversion class, and `merge-queue-ops-2.ts:53` already carries a flag note saying so. I measured the split across all 126: **18 are fallback arms of already-converted seams; 108 are bare guards.** The headline number conflates them. ## What changed `reads.ts` states the invariant in its own words — > RESOLVED BEFORE THE FIRST SIGNAL, because two adjacent signals must not disagree. — and then called two of the four stall signals with the literal: | signal | before | |---|---| | `getInReviewStallReason` | resolved (`reviewColumns`) | | `getInReviewStalledSignal` | resolved (`reviewColumns`) | | `detectStalledReview` | **literal `"in-review"`** | | `hasFreshAgentLogActivitySinceTaskUpdate` | **literal `"in-review"`** | On a renamed board `stalledReview` returned `undefined` for every card, and the fresh-activity gate answered `false` — so `executingTaskIds` stayed empty and the board showed Stalled / Merge stalled *while a merger was visibly streaming*, the precise regression that function's own FNXC note says it was restored to prevent. Both now take an optional resolved `reviewColumns`. All four hydration passes pass the set **they already had in scope one line away**; two needed only a hoist, one reused the per-row map, one was resolving the same set inline twice. ## Mutation evidence | Mutant | Result | |---|---| | baseline | 11 passed | | revert the detector guard to the literal | **2 failed** | | make the parameter a widening (`reviewColumns ? true`) | **2 failed** | The second matters: it proves the new parameter is a real gate and not a change that merely makes every card eligible. Both arms are asserted, since the literal default is load-bearing for every caller outside `reads.ts`. ## Flagged — do not guess - **`scheduler.ts` (12 guards).** All 12 sit inside *synchronous* listeners (`task:moved`'s sync prologue; `task:updated` is sync outright). The only sync resolver available, `resolveTaskParkedColumnsSync`, is already used at lines 929/1130/1157 and is **inert under PostgreSQL** — my own live-PG E2E proves it always returns the default board. "Converting" these with it would drop the census by 12 and change nothing. The existing note at 908–926 names the real unblock: carry resolved lanes on the event payload so no listener resolves at all. Left alone. - **`restart-recovery-coordinator.ts` (4)** — already the optional-parameter device with all three production callers passing resolved answers. Not backlog. - **`reads.ts:358`, `audit-ops.ts:208`, `task-id-integrity.ts:444`** — `"archived"` here is the *cold-storage tier*, not the board column. Trait resolution would be wrong. ## Verification `test:gate` exit 0 · full `@fusion/core` unit suite **4880 passed** · typecheck exit 0 · `pnpm lint` clean · lifecycle-column census exit 0 · FNXC date ratchet exit 0 · lane-wiring census exit 0. **One unrelated failure to report, not appeased:** `src/__tests__/postgres/pg-test-harness-template-concurrency.pg.test.ts` fails under the full suite and **passes in isolation on both my tree and the untouched baseline** — a pre-existing full-suite concurrency flake in the PG harness. Not mine, not in the merge gate. I did not quarantine it: it is another worker's harness, and AGENTS.md warns that quarantining a concurrency test can mask a real product race. Flagging for its owner.
2026-07-31 02:39:33 -07:00
parent 24f5ffaffa
commit 107a1e790a
3 changed files with 93 additions and 16 deletions
--- a/packages/core/src/tests/stalled-review-detector.test.ts
+++ b/packages/core/src/tests/stalled-review-detector.test.ts
@@ -111,4 +111,44 @@ describe("detectStalledReview", () => {

    expect(signal?.heuristic).toBe("reenqueue-churn");
  });
+
+  /*
+  FNXC:WorkflowLifecycleColumns 2026-07-31-01:20 (fleet — the review lane, resolved):
+  The renamed-board arm. Before the `reviewColumns` parameter this detector compared against the
+  literal `"in-review"`, so on a board whose review lane is called anything else it returned
+  `undefined` for EVERY card and the stall was unreportable — while the adjacent
+  `getInReviewStallReason` on the very next line in `reads.ts` resolved the real lane. Two stall
+  signals for one card, disagreeing by construction.
+
+  Both arms are asserted, because the literal default is load-bearing: every caller outside
+  `reads.ts` still omits the parameter and must keep today's behaviour exactly.
+  */
+  const churnLog = [
+    entry("2026-05-12T11:30:00.000Z", STALLED_REVIEW_REENQUEUE_PATTERN),
+    entry("2026-05-12T11:40:00.000Z", `noise ${STALLED_REVIEW_REENQUEUE_PATTERN}`),
+    entry("2026-05-12T11:50:00.000Z", STALLED_REVIEW_REENQUEUE_PATTERN),
+  ];
+
+  it("RENAMED BOARD — detects the stall when the resolved review lane is passed", () => {
+    const task = { column: "checking", paused: false, log: churnLog };
+
+    // The unconverted call shape: silent on this board, which is the defect.
+    expect(detectStalledReview(task, { now })).toBeUndefined();
+
+    // The resolved answer the reads.ts hydration passes.
+    const signal = detectStalledReview(task, { now, reviewColumns: new Set(["in-review", "checking"]) });
+    expect(signal?.heuristic).toBe("reenqueue-churn");
+    expect(signal?.matchCount).toBe(STALLED_REVIEW_REENQUEUE_THRESHOLD);
+  });
+
+  it("DEFAULT BOARD — the literal default is unchanged, and a resolved set still gates", () => {
+    const task = { column: "in-review", paused: false, log: churnLog };
+
+    // No parameter: exactly today's behaviour, which is what every caller outside reads.ts relies on.
+    expect(detectStalledReview(task, { now })?.heuristic).toBe("reenqueue-churn");
+
+    // A resolved set that does NOT contain the card's column must still say "not in review" — the
+    // parameter is a real gate, not a widening that makes every card eligible.
+    expect(detectStalledReview(task, { now, reviewColumns: new Set(["checking"]) })).toBeUndefined();
+  });
 });
--- a/packages/core/src/stalled-review-detector.ts
+++ b/packages/core/src/stalled-review-detector.ts
@@ -38,11 +38,32 @@ export interface StalledReviewSignal {
  lastMatchAt: string;
 }

+/*
+FNXC:WorkflowLifecycleColumns 2026-07-31-01:20 (fleet — the review lane, resolved):
+`reviewColumns` is an optional RESOLVED answer; omitted, this is byte-for-byte today's behaviour, so
+no caller or test outside `reads.ts` changes.
+
+Why it mattered. All four production call sites are stall-badge hydration passes in
+`task-store/reads.ts`, and every one of them ALREADY resolves the review lane one line earlier and
+hands it to the adjacent `getInReviewStallReason`/`getInReviewStalledSignal`. That file states the
+invariant in its own words — "RESOLVED BEFORE THE FIRST SIGNAL, because two adjacent signals must not
+disagree" — and then called THIS detector with the literal. So on a renamed board the two stall
+signals for one card disagreed by construction: the in-review stall reason resolved the board's real
+review lane while `stalledReview` compared against `"in-review"`, a column that board does not
+contain, and silently returned `undefined` for every card. The stall this detector exists to surface
+was unreportable on any renamed board.
+
+The set is the union of the three review roles (`resolveReviewColumnsForTask`), which unions the
+legacy id too, so a board mid-rename is never skipped.
+*/
 export function detectStalledReview(
  task: Pick<Task, "column" | "paused" | "log">,
-  options?: { now?: number; windowMs?: number },
+  options?: { now?: number; windowMs?: number; reviewColumns?: ReadonlySet<string> },
 ): StalledReviewSignal | undefined {
-  if (task.column !== "in-review" || task.paused === true || task.log.length === 0) {
+  const inReview = options?.reviewColumns
+    ? options.reviewColumns.has(task.column)
+    : task.column === "in-review";
+  if (!inReview || task.paused === true || task.log.length === 0) {
    return undefined;
  }

--- a/packages/core/src/task-store/reads.ts
+++ b/packages/core/src/task-store/reads.ts
@@ -85,12 +85,23 @@ function getLatestAgentLogActivityMs(store: TaskStore, taskId: string): number |
 * TaskStore.hasFreshAgentLogActivitySinceTaskUpdate, which the PostgreSQL
 * cutover's store split predated.
 */
+/*
+FNXC:WorkflowLifecycleColumns 2026-07-31-01:20 (fleet — the review lane, resolved):
+`reviewColumns` is an optional RESOLVED answer; omitted, this is exactly today's behaviour.
+
+Same defect as `detectStalledReview` and the same blast radius: this gate decides whether a streaming
+merge/review agent SUPPRESSES the stall badges. Against the literal it answered `false` for every card
+on a renamed board, so `executingTaskIds` stayed empty and the board showed "Stalled"/"Merge stalled"
+while a merger was visibly making progress — the precise regression the FNXC note below says this
+function was restored to prevent.
+*/
 function hasFreshAgentLogActivitySinceTaskUpdate(
  store: TaskStore,
  task: Pick<Task, "id" | "column" | "updatedAt">,
  now: number,
+  reviewColumns?: ReadonlySet<string>,
 ): boolean {
-  if (task.column !== "in-review") return false;
+  if (!(reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review")) return false;
  const latestAgentLogMs = getLatestAgentLogActivityMs(store, task.id);
  if (latestAgentLogMs == null) return false;

@@ -221,7 +232,10 @@ export async function getTaskImpl(store: TaskStore, id: string, options?: { acti
      main's FNXC:WorkflowLifecycle 2026-07-01-23:27 behavior, which the
      PostgreSQL cutover's store split predated.
      */
-      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now);
+      /* FNXC:WorkflowLifecycleColumns 2026-07-31-01:20 (fleet): hoisted ABOVE the fresh-activity gate
+         so that gate can resolve too — it is now the FIRST signal, and the note below is the rule. */
+      const reviewColumnsForTask: InReviewStallContext["reviewColumns"] = await resolveReviewColumnsForTask(store, task.id);
+      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now, reviewColumnsForTask);
      const executingTaskIds = hasFreshAgentLogActivity ? new Set<string>([task.id]) : undefined;
      /*
      FNXC:WorkflowLifecycleColumns 2026-07-30-20:50:
@@ -246,7 +260,6 @@ export async function getTaskImpl(store: TaskStore, id: string, options?: { acti
      on that ratchet while genuinely wired. Naming the types is the smaller fix than appending to a
      list the guard says may only ever shorten.
      */
-      const reviewColumnsForTask: InReviewStallContext["reviewColumns"] = await resolveReviewColumnsForTask(store, task.id);
      task.inReviewStall = mergeQueuedTaskIds.has(task.id)
        ? undefined
        : getInReviewStallReason(task, {
@@ -268,7 +281,7 @@ export async function getTaskImpl(store: TaskStore, id: string, options?: { acti
          engineActiveSinceMs: settings.engineActiveSinceMs,
          engineActivationGraceMs: settings.engineActivationGraceMs,
        } satisfies InReviewStalledContext);
-      task.stalledReview = mergeQueuedTaskIds.has(task.id) || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now });
+      task.stalledReview = mergeQueuedTaskIds.has(task.id) || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now, reviewColumns: reviewColumnsForTask });
      task.retrySummary = computeRetrySummary(task);
      /*
      FNXC:TaskDetailPromptResilience 2026-07-10-15:00 (merge port from main):
@@ -410,9 +423,9 @@ export async function listTasksImpl(store: TaskStore, options?: { limit?: number
      main's FNXC:WorkflowLifecycle 2026-07-01-23:27 behavior, which the
      PostgreSQL cutover's store split predated.
      */
-      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now);
-      const executingTaskIds = hasFreshAgentLogActivity ? new Set<string>([task.id]) : undefined;
      const reviewColumnsForRow = await resolveReviewColumnsForTask(store, task.id, listPassIrCache);
+      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now, reviewColumnsForRow);
+      const executingTaskIds = hasFreshAgentLogActivity ? new Set<string>([task.id]) : undefined;
      task.inReviewStall = isMergeQueued ? undefined : getInReviewStallReason(task, {
        now,
        reviewColumns: reviewColumnsForRow,
@@ -472,7 +485,7 @@ export async function listTasksImpl(store: TaskStore, options?: { limit?: number
        if (!(err instanceof RangeError)) throw err;
        task.ageStaleness = undefined;
      }
-      task.stalledReview = isMergeQueued || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now });
+      task.stalledReview = isMergeQueued || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now, reviewColumns: reviewColumnsForRow });
      task.retrySummary = computeRetrySummary(task);
      if (slim) {
        task.timedExecutionMs = store.computeTimedExecutionMs(task.log);
@@ -641,9 +654,9 @@ export async function listTasksModifiedSinceImpl(store: TaskStore, since: string
      main's FNXC:WorkflowLifecycle 2026-07-01-23:27 behavior, which the
      PostgreSQL cutover's store split predated.
      */
-      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now);
-      const executingTaskIds = hasFreshAgentLogActivity ? new Set<string>([task.id]) : undefined;
      const reviewColumnsForRow = reviewColumnsByTaskId.get(task.id) ?? new Set<string>(["in-review"]);
+      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now, reviewColumnsForRow);
+      const executingTaskIds = hasFreshAgentLogActivity ? new Set<string>([task.id]) : undefined;
      task.inReviewStall = isMergeQueued ? undefined : getInReviewStallReason(task, {
        now,
        reviewColumns: reviewColumnsForRow,
@@ -709,7 +722,7 @@ export async function listTasksModifiedSinceImpl(store: TaskStore, since: string
        }
      }
      task.timedExecutionMs = store.computeTimedExecutionMs(task.log);
-      task.stalledReview = isMergeQueued || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now });
+      task.stalledReview = isMergeQueued || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now, reviewColumns: reviewColumnsForRow });
      task.retrySummary = computeRetrySummary(task);
      task.log = [];
      return task;
@@ -772,11 +785,14 @@ export async function searchTasksImpl(store: TaskStore, query: string, options?:
      main's FNXC:WorkflowLifecycle 2026-07-01-23:27 behavior, which the
      PostgreSQL cutover's store split predated.
      */
-      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now);
+      /* FNXC:WorkflowLifecycleColumns 2026-07-31-01:20 (fleet): resolved ONCE for this row — it was
+         resolved inline twice below, and the fresh-activity gate could not see it at all. */
+      const reviewColumnsForRow = await resolveReviewColumnsForTask(store, task.id, searchPassIrCache);
+      const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now, reviewColumnsForRow);
      const executingTaskIds = hasFreshAgentLogActivity ? new Set<string>([task.id]) : undefined;
      task.inReviewStall = isMergeQueued ? undefined : getInReviewStallReason(task, {
        now,
-        reviewColumns: await resolveReviewColumnsForTask(store, task.id, searchPassIrCache),
+        reviewColumns: reviewColumnsForRow,
        executingTaskIds,
        autoMerge: allowsAutoMergeProcessing(task, settings),
        engineActiveSinceMs: settings.engineActiveSinceMs,
@@ -785,13 +801,13 @@ export async function searchTasksImpl(store: TaskStore, query: string, options?:
      task.inReviewStalled = isMergeQueued ? undefined : getInReviewStalledSignal(task, {
        now,
        executingTaskIds,
-        reviewColumns: await resolveReviewColumnsForTask(store, task.id, searchPassIrCache),
+        reviewColumns: reviewColumnsForRow,
        thresholdMs: settings.inReviewStalledThresholdMs,
        autoMerge: allowsAutoMergeProcessing(task, settings),
        engineActiveSinceMs: settings.engineActiveSinceMs,
        engineActivationGraceMs: settings.engineActivationGraceMs,
      } satisfies InReviewStalledContext);
-      task.stalledReview = isMergeQueued || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now });
+      task.stalledReview = isMergeQueued || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now, reviewColumns: reviewColumnsForRow });
      task.retrySummary = computeRetrySummary(task);
      if (slim) {
        task.timedExecutionMs = store.computeTimedExecutionMs(task.log);