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);