fix(core): the in-review stall signal never got the board's review lanes — 0 of 4 call sites (#2956)
## Main is red, and the red is pointing at a real defect `unwired-lane-parameter-guard` fails on `origin/main` after #2951. This is not a stale allow-list — the parameter genuinely never reaches the function. #2951 added `reviewColumns?: ReadonlySet<string>` to three signal modules and wired two of them completely. **`getInReviewStallReason` was wired at none of its four call sites.** Measured by brace-matching each call's option literal: ``` getInReviewStallReason L227=NO L390=NO L599=NO L729=NO getInReviewStalledSignal all 4 wired getStalePausedReviewSignal both wired ``` ## The user-visible consequence `reads.ts` computes two adjacent signals for the same card. On a board declaring a **separate merge lane beside its human-review lane**, `inReviewStall` read the *first* review column only, while `inReviewStalled` — three lines below — read the *set*. **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 exactly one element. At three of the four sites the resolve sat *below* the call, which is why the parameter could not be passed. Those are hoisted. ## I have to correct my own earlier report On #2951 I said *"3 of 4 call sites wired, `reads.ts:227` is the gap."* **That was wrong.** I had measured with a 12-line proximity grep, which bled into the adjacent `getInReviewStalledSignal` call and counted its `reviewColumns:` as the first call's. Brace-matching the literal shows 0 of 4. The defect was four times larger than I reported, and the cause was exactly the anti-pattern I have spent this session filing against other people's guards — a proximity window standing in for structure. ## Naming the context types The guard keys an interface member to its **owner symbol** and only counts a mention from a file that also names that owner, so passing the property inline reads as unwired even when every site supplies it. `satisfies InReviewStalledContext` / `satisfies StalePausedReviewContext` on the option literals is real type-checking, not a decorative import — lint rejected the decorative version, correctly. ## New test, because the existing guard cannot see this Measured: **deleting the `reviewColumns:` line from a fixed call site leaves `unwired-lane-parameter-guard` at 9/9 green**, because the file still names the type. So the wiring I just fixed had no coverage at all. The new ratchet brace-matches each call site's option literal: | mutation | result | |---|---| | remove lanes from one call site | **1 failed** — *"1 of 4 getInReviewStallReason call sites omit reviewColumns"* | It also asserts it **found** call sites before checking them — a parse that matched nothing would be vacuous, which is the failure mode this guard family keeps producing. (It caught me mid-change too: an earlier scripted edit left the file syntactically invalid and the source-text test still passed 3/3. It is a wiring ratchet, not a substitute for `tsc`.) ## Verification Core **4861 passed / 0 failed** · guard **9/9** with `KNOWN_UNWIRED` **unchanged** · `pnpm test:gate` **exit 0** · lint clean · core `tsc` **0 errors**. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
@@ -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);
|
||||
});
|
||||
}
|
||||
});
|
||||
@@ -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<string>([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<string>([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<string>([task.id]) : undefined;
|
||||
const reviewColumnsForRow = reviewColumnsByTaskId.get(task.id) ?? new Set<string>(["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<string>(["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<string>([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) {
|
||||
|
||||
Reference in New Issue
Block a user