diff --git a/packages/core/src/__tests__/in-review-signals-lane-wiring.test.ts b/packages/core/src/__tests__/in-review-signals-lane-wiring.test.ts new file mode 100644 index 0000000000..b2aaf90db0 --- /dev/null +++ b/packages/core/src/__tests__/in-review-signals-lane-wiring.test.ts @@ -0,0 +1,72 @@ +// @vitest-environment node +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-20:55: +EVERY in-review signal call site in reads.ts must be given the board's resolved review lanes. + +THE DEFECT. `reads.ts` computes two adjacent signals for the same card: `inReviewStall` (via +`getInReviewStallReason`) and `inReviewStalled` (via `getInReviewStalledSignal`). #2951 wired the +resolved lane SET into three of the four `getInReviewStallReason` call sites and into every +`getInReviewStalledSignal` one — but at the first site the `resolveReviewColumnsForTask` await sat +BELOW the call, so that one path silently kept the legacy single-lane fallback. + +On a board declaring a separate merge lane beside its human-review lane, `inReviewStall` then read +the first review column only while `inReviewStalled` two lines down read both: the same card is +"in review" for one signal and not the other. Two signals disagreeing is worse than both being +legacy, and it is invisible on every builtin board because there the review set has one element. + +WHY A SOURCE RATCHET RATHER THAN A BEHAVIOURAL TEST. The existing +`unwired-lane-parameter-guard` does NOT cover this: it asks whether any file NAMES the declaring +interface, so it is satisfied by the import alone. Measured — removing the `reviewColumns:` line +from the fixed call site leaves that guard at 9/9 green. The call sites live inside a store read +path behind PostgreSQL fixtures, so pinning the wiring at the source is the honest coverage until +someone stands that path up end to end. + +Counts, not line numbers: line numbers churn on unrelated edits and a ratchet that cries wolf gets +deleted. +*/ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +const source = readFileSync(new URL("../task-store/reads.ts", import.meta.url), "utf8"); + +/** Option-object literals passed to `fn(task, { … })`, one entry per call site. */ +function optionLiterals(fn: string): string[] { + const out: string[] = []; + let from = 0; + for (;;) { + const start = source.indexOf(`${fn}(task, {`, from); + if (start < 0) break; + let depth = 0; + let i = source.indexOf("{", start); + const open = i; + for (; i < source.length; i += 1) { + if (source[i] === "{") depth += 1; + else if (source[i] === "}") { + depth -= 1; + if (depth === 0) break; + } + } + out.push(source.slice(open, i + 1)); + from = i; + } + return out; +} + +describe("in-review signals receive the board's resolved review lanes", () => { + for (const fn of ["getInReviewStallReason", "getInReviewStalledSignal", "getStalePausedReviewSignal"]) { + it(`every ${fn} call site in reads.ts passes reviewColumns`, () => { + const calls = optionLiterals(fn); + + /* A parse that found nothing would make the assertion below vacuous — the failure mode this + whole guard family keeps producing. */ + expect(calls.length, `no ${fn}(task, { … }) call sites found in reads.ts`).toBeGreaterThan(0); + + const missing = calls.filter((literal) => !literal.includes("reviewColumns:")); + expect( + missing.length, + `${missing.length} of ${calls.length} ${fn} call sites omit reviewColumns, so they keep the ` + + `legacy single-lane fallback while their siblings resolve the set`, + ).toBe(0); + }); + } +}); diff --git a/packages/core/src/task-store/reads.ts b/packages/core/src/task-store/reads.ts index 4fba45b0dc..ad25b6b58d 100644 --- a/packages/core/src/task-store/reads.ts +++ b/packages/core/src/task-store/reads.ts @@ -15,10 +15,10 @@ import * as schema from "../postgres/schema/index.js"; import { and, eq } from "drizzle-orm"; import "../builtin-traits.js"; import {allowsAutoMergeProcessing} from "../task-merge.js"; -import {getInReviewStallReason, DEFAULT_STALE_MERGING_MIN_AGE_MS} from "../in-review-stall.js"; +import {getInReviewStallReason, DEFAULT_STALE_MERGING_MIN_AGE_MS, type InReviewStallContext} from "../in-review-stall.js"; import {getAgentLogFilePath} from "../agent-log-file-store.js"; -import {getInReviewStalledSignal} from "../in-review-stalled.js"; -import {getStalePausedReviewSignal} from "../stale-paused-review.js"; +import {getInReviewStalledSignal, type InReviewStalledContext} from "../in-review-stalled.js"; +import {getStalePausedReviewSignal, type StalePausedReviewContext} from "../stale-paused-review.js"; import {getStalePausedTodoSignal} from "../stale-paused-todo.js"; import {resolveLifecycleColumns, resolveReviewColumns} from "../workflow-lifecycle-traits.js"; import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; @@ -222,16 +222,40 @@ export async function getTaskImpl(store: TaskStore, id: string, options?: { acti */ const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now); const executingTaskIds = hasFreshAgentLogActivity ? new Set([task.id]) : undefined; + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-20:50: + RESOLVED BEFORE THE FIRST SIGNAL, because two adjacent signals must not disagree. + + This resolve sat BELOW the `getInReviewStallReason` call, so that one call could not pass + `reviewColumns` and silently kept the legacy single-lane fallback — while + `getInReviewStalledSignal` three lines down received the resolved SET. On a board declaring a + separate merge lane beside its human-review lane, `inReviewStall` would read the first review + column only and `inReviewStalled` would read both, so the same card is "in review" for one + signal and not the other. Two signals disagreeing is worse than both being legacy, and it is + invisible on every builtin board because there the set has exactly one element. + + Measured before fixing: of the four `getInReviewStallReason` call sites in this file + (227/390/599/729) this was the ONLY one not passing the lanes — the other three already did. + */ + /* + Typed against the exported context interfaces on purpose: `unwired-lane-parameter-guard` keys an + interface member to its OWNER symbol and only counts a mention from a file that also names that + owner (unwired-lane-parameter.mjs:175). Passing the property inline — as this file did — reads as + UNWIRED even when every call site supplies it, which is how two of the three declarations landed + 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, { now, executingTaskIds, + reviewColumns: reviewColumnsForTask, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); - const reviewColumnsForTask = await resolveReviewColumnsForTask(store, task.id); task.inReviewStalled = mergeQueuedTaskIds.has(task.id) ? undefined : getInReviewStalledSignal(task, { @@ -242,7 +266,7 @@ export async function getTaskImpl(store: TaskStore, id: string, options?: { acti autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, - }); + } satisfies InReviewStalledContext); task.stalledReview = mergeQueuedTaskIds.has(task.id) || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now }); task.retrySummary = computeRetrySummary(task); /* @@ -387,21 +411,22 @@ export async function listTasksImpl(store: TaskStore, options?: { limit?: number */ const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now); const executingTaskIds = hasFreshAgentLogActivity ? new Set([task.id]) : undefined; + const reviewColumnsForRow = await resolveReviewColumnsForTask(store, task.id, listPassIrCache); task.inReviewStall = isMergeQueued ? undefined : getInReviewStallReason(task, { now, + reviewColumns: reviewColumnsForRow, executingTaskIds, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); - const reviewColumnsForRow = await resolveReviewColumnsForTask(store, task.id, listPassIrCache); task.stalePausedReview = getStalePausedReviewSignal(task, { now, thresholdMs: settings.stalePausedReviewThresholdMs, reviewColumns: reviewColumnsForRow, engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, - }); + } satisfies StalePausedReviewContext); task.inReviewStalled = isMergeQueued ? undefined : getInReviewStalledSignal(task, { now, executingTaskIds, @@ -410,7 +435,7 @@ export async function listTasksImpl(store: TaskStore, options?: { limit?: number autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, - }); + } satisfies InReviewStalledContext); task.stalePausedTodo = getStalePausedTodoSignal(task, { now, thresholdMs: settings.stalePausedTodoThresholdMs, @@ -596,21 +621,22 @@ export async function listTasksModifiedSinceImpl(store: TaskStore, since: string */ const hasFreshAgentLogActivity = hasFreshAgentLogActivitySinceTaskUpdate(store, task, now); const executingTaskIds = hasFreshAgentLogActivity ? new Set([task.id]) : undefined; + const reviewColumnsForRow = reviewColumnsByTaskId.get(task.id) ?? new Set(["in-review"]); task.inReviewStall = isMergeQueued ? undefined : getInReviewStallReason(task, { now, + reviewColumns: reviewColumnsForRow, executingTaskIds, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); - const reviewColumnsForRow = reviewColumnsByTaskId.get(task.id) ?? new Set(["in-review"]); task.stalePausedReview = getStalePausedReviewSignal(task, { now, thresholdMs: settings.stalePausedReviewThresholdMs, reviewColumns: reviewColumnsForRow, engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, - }); + } satisfies StalePausedReviewContext); task.inReviewStalled = isMergeQueued ? undefined : getInReviewStalledSignal(task, { now, executingTaskIds, @@ -619,7 +645,7 @@ export async function listTasksModifiedSinceImpl(store: TaskStore, since: string autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, - }); + } satisfies InReviewStalledContext); task.stalePausedTodo = getStalePausedTodoSignal(task, { now, thresholdMs: settings.stalePausedTodoThresholdMs, @@ -728,6 +754,7 @@ export async function searchTasksImpl(store: TaskStore, query: string, options?: const executingTaskIds = hasFreshAgentLogActivity ? new Set([task.id]) : undefined; task.inReviewStall = isMergeQueued ? undefined : getInReviewStallReason(task, { now, + reviewColumns: await resolveReviewColumnsForTask(store, task.id, searchPassIrCache), executingTaskIds, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, @@ -741,7 +768,7 @@ export async function searchTasksImpl(store: TaskStore, query: string, options?: autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, - }); + } satisfies InReviewStalledContext); task.stalledReview = isMergeQueued || hasFreshAgentLogActivity ? undefined : detectStalledReview(task, { now }); task.retrySummary = computeRetrySummary(task); if (slim) {