From 107a1e790afebcb073298f3838f7f75606124ef8 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 02:39:33 -0700 Subject: [PATCH] fix(core): the review lane was resolved for two stall signals and literal for the other two (#3053) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- .../__tests__/stalled-review-detector.test.ts | 40 +++++++++++++++++ packages/core/src/stalled-review-detector.ts | 25 ++++++++++- packages/core/src/task-store/reads.ts | 44 +++++++++++++------ 3 files changed, 93 insertions(+), 16 deletions(-) diff --git a/packages/core/src/__tests__/stalled-review-detector.test.ts b/packages/core/src/__tests__/stalled-review-detector.test.ts index 4b578f68d6..0f3185fff0 100644 --- 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(); + }); }); diff --git a/packages/core/src/stalled-review-detector.ts b/packages/core/src/stalled-review-detector.ts index 8bbb7e5043..52312e67c5 100644 --- 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, - options?: { now?: number; windowMs?: number }, + options?: { now?: number; windowMs?: number; reviewColumns?: ReadonlySet }, ): 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; } diff --git a/packages/core/src/task-store/reads.ts b/packages/core/src/task-store/reads.ts index 35200ef09e..6058ca8f43 100644 --- 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, now: number, + reviewColumns?: ReadonlySet, ): 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([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([task.id]) : undefined; const reviewColumnsForRow = await resolveReviewColumnsForTask(store, task.id, listPassIrCache); + const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now, reviewColumnsForRow); + const executingTaskIds = hasFreshAgentLogActivity ? new Set([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([task.id]) : undefined; const reviewColumnsForRow = reviewColumnsByTaskId.get(task.id) ?? new Set(["in-review"]); + const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now, reviewColumnsForRow); + const executingTaskIds = hasFreshAgentLogActivity ? new Set([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([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);