From 8e5e1147d2a8a394a6b9bf802d9e57c10d960bf5 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 20:30:18 -0700 Subject: [PATCH] =?UTF-8?q?core,engine:=20the=20last=20literal=20lifecycle?= =?UTF-8?q?=20query=20=E2=80=94=20and=20the=20three=20stall=20signals=20th?= =?UTF-8?q?at=20disagreed=20(#2951)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **This is the last one.** `surfaceInReviewStalls` was the final literal `listTasks({ column })` in production — I verified it by direct scan, not by census arithmetic: **1 remaining before this, 0 after.** It tells an operator that a card is stalled in review. On a renamed board the stall was real and the board simply never said so. ## It came last on purpose Converting the read alone would have been **worse than leaving it**. `getInReviewStallReason` gated on the literal `in-review` itself, so a widened read hands every renamed-board card to a classifier that drops it — the missed-pair class, wearing the shape of a clean one-line conversion. ## What was actually there Three sibling signals decorate the same row, and they **disagreed about which lane it is in**: | signal | before | | --- | --- | | `getInReviewStalledSignal` | singular `reviewColumn` — resolved, but **first-per-role** | | `getStalePausedReviewSignal` | singular `reviewColumn` — same | | `getInReviewStallReason` | **no seam at all** — literal | So one row could be judged in-review by one signal and not by another. And the singular ones are the **arity trap**: `resolveLifecycleColumns().review` is the *first* column carrying a review role, so a board with a separate merge lane beside its human-review lane had a second review column matching none of them. All three now take `reviewColumns` (membership), resolved **once per row** through `resolveReviewColumns` — the union of the three review roles — so they cannot disagree by construction. The singular/literal paths remain as the no-metadata fallback, so a caller passing nothing is byte-identical to today. Ten call sites in `reads.ts` wired from that one answer; the singular resolver is deleted. ## Revert results Each applied alone and re-run: | conversion | reverted → | | --- | --- | | the resolved read | fails — the card is never listed | | `reviewColumns` at the call | fails — the classifier drops the renamed card the widened read just found | That second row is the whole point: it proves the pair had to move together, which is the thing I got wrong twice earlier in this series. ## Second commit: a red on `main`, not from this branch `check-fnxc-future-dates` landed and **`main` fails it** — verified by running the script on a clean `origin/main` checkout rather than inferring. Nine files carry stamps dated after today, so every worker's gate fails on a check none of their changes caused. Several are mine: I had been stamping tomorrow's date across this whole series, which is precisely the out-of-order record the check exists to prevent. Scope held deliberately: a repo-wide sweep touched **266 files** across docs, scripts and every package. I ran it, backed it out, and limited this to the nine files the check actually flags — a mechanical rewrite that size during a queue freeze would conflict with every in-flight branch, which is worse than the red it fixes. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71 (green **only** with the stamp commit); `@fusion/core` full suite **4810 passed**; engine self-healing + blindness + both ratchets **758 passed**; `tsc` clean on core and engine; `pnpm lint`, `check:changesets`, `lifecycle-column-census --strict`, `check-sql-column-literals` and `check-fnxc-future-dates` all clean. ## Summary by CodeRabbit - **Bug Fixes** - Review-stall detection now recognizes renamed and multiple review columns while retaining support for the legacy review column. - Paused tasks continue to be excluded from stall detection. - Self-healing review-stall sweeps now search all configured review lanes and avoid duplicate task results. - **Tests** - Added regression coverage for renamed and legacy review-lane queries. --- .changeset/in-review-stall-lane-membership.md | 7 +++ packages/core/src/in-review-stall.ts | 20 ++++++- packages/core/src/in-review-stalled.ts | 17 +++++- packages/core/src/stale-paused-review.ts | 17 +++++- packages/core/src/task-store/reads.ts | 53 +++++++++++-------- ...elf-healing-query-filter-blindness.test.ts | 39 ++++++++++++++ packages/engine/src/self-healing.ts | 28 +++++++++- .../lib/lifecycle-column-census-baseline.json | 4 +- 8 files changed, 156 insertions(+), 29 deletions(-) create mode 100644 .changeset/in-review-stall-lane-membership.md diff --git a/.changeset/in-review-stall-lane-membership.md b/.changeset/in-review-stall-lane-membership.md new file mode 100644 index 0000000000..fc07b9f730 --- /dev/null +++ b/.changeset/in-review-stall-lane-membership.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Review stalls are surfaced, and judged consistently, on boards with renamed columns. +category: fix +dev: The three stall signals disagreed about a row's lane — two took a singular `reviewColumn` (first-per-role) and `getInReviewStallReason` had no seam and used the literal. All three now take a `reviewColumns` membership set, resolved once per row via `resolveReviewColumns`, and `surfaceInReviewStalls` reads the project's review columns instead of the literal `in-review`. diff --git a/packages/core/src/in-review-stall.ts b/packages/core/src/in-review-stall.ts index 62a4b11ffc..f44c2c1bdd 100644 --- a/packages/core/src/in-review-stall.ts +++ b/packages/core/src/in-review-stall.ts @@ -35,6 +35,16 @@ export interface InReviewStallContext { maxAutoMergeRetries?: number; engineActiveSinceMs?: number; engineActivationGraceMs?: number; + /* + FNXC:WorkflowResolvedColumns 2026-07-30-22:10 (the lane seam, MEMBERSHIP not one column): + `reviewColumn` is `resolveLifecycleColumns().review` — the FIRST column carrying a review role. A + board declaring a separate merge lane beside its human-review lane has TWO, and a card in the second + read as not-in-review. This takes the SET. + + Optional, with today's behaviour preserved as the fallback, so a caller that does not pass it is + byte-identical. + */ + reviewColumns?: ReadonlySet; } /** Keep aligned with engine DEFAULT_STALE_MERGING_STATUS_MIN_AGE_MS. */ @@ -176,7 +186,15 @@ export function getInReviewStallReason( task: Pick & { id?: string }, context: InReviewStallContext = {}, ): InReviewStallSignal | undefined { - if (task.column !== "in-review" || task.paused === true) { + /* + This classifier had NO seam while its two siblings did, so one decorated row could have + `inReviewStalled` resolved and `inReviewStall` literal — one row, two lane answers. + */ + const inReviewLane = context.reviewColumns + ? context.reviewColumns.has(task.column) + /* DELIBERATE-LITERAL — the no-metadata fallback. */ + : task.column === "in-review"; + if (!inReviewLane || task.paused === true) { return undefined; } diff --git a/packages/core/src/in-review-stalled.ts b/packages/core/src/in-review-stalled.ts index eb8a66536b..ca3f290025 100644 --- a/packages/core/src/in-review-stalled.ts +++ b/packages/core/src/in-review-stalled.ts @@ -20,6 +20,16 @@ export interface InReviewStalledContext { /** The workflow's REVIEW (merge-orchestration) column. Defaults to the legacy * `"in-review"` so unconverted callers are byte-identical. */ reviewColumn?: string; + /* + FNXC:WorkflowResolvedColumns 2026-07-30-22:10 (the lane seam, MEMBERSHIP not one column): + `reviewColumn` is `resolveLifecycleColumns().review` — the FIRST column carrying a review role. A + board declaring a separate merge lane beside its human-review lane has TWO, and a card in the second + read as not-in-review. This takes the SET. + + Optional, with today's behaviour preserved as the fallback, so a caller that does not pass it is + byte-identical. + */ + reviewColumns?: ReadonlySet; now?: number; thresholdMs?: number; autoMerge?: boolean; @@ -49,8 +59,11 @@ export function getInReviewStalledSignal( its sibling in stale-paused-review.ts; defaults to the legacy id so existing callers are byte-identical. */ - const reviewColumn = context.reviewColumn ?? "in-review"; - if (task.column !== reviewColumn || task.paused === true) return undefined; + const inReviewLane = context.reviewColumns + ? context.reviewColumns.has(task.column) + /* DELIBERATE-LITERAL — the no-metadata fallback; a supplied set always wins. */ + : task.column === (context.reviewColumn ?? "in-review"); + if (!inReviewLane || task.paused === true) return undefined; if (context.autoMerge === false) return undefined; if (task.mergeDetails?.mergeConfirmed === true) return undefined; if (task.status === "awaiting-user-review" || task.status === "awaiting-approval") return undefined; diff --git a/packages/core/src/stale-paused-review.ts b/packages/core/src/stale-paused-review.ts index feafbf5451..de924b585d 100644 --- a/packages/core/src/stale-paused-review.ts +++ b/packages/core/src/stale-paused-review.ts @@ -16,6 +16,16 @@ export interface StalePausedReviewContext { /** The workflow's REVIEW (merge-orchestration) column. Defaults to the legacy * `"in-review"` so unconverted callers are byte-identical. */ reviewColumn?: string; + /* + FNXC:WorkflowResolvedColumns 2026-07-30-22:10 (the lane seam, MEMBERSHIP not one column): + `reviewColumn` is `resolveLifecycleColumns().review` — the FIRST column carrying a review role. A + board declaring a separate merge lane beside its human-review lane has TWO, and a card in the second + read as not-in-review. This takes the SET. + + Optional, with today's behaviour preserved as the fallback, so a caller that does not pass it is + byte-identical. + */ + reviewColumns?: ReadonlySet; now?: number; thresholdMs?: number; engineActiveSinceMs?: number; @@ -37,8 +47,11 @@ export function getStalePausedReviewSignal( any workflow that renames its review column. Defaults to the legacy id, so every existing caller is byte-identical. */ - const reviewColumn = context.reviewColumn ?? "in-review"; - if (task.column !== reviewColumn || task.paused !== true) return undefined; + const inReviewLane = context.reviewColumns + ? context.reviewColumns.has(task.column) + /* DELIBERATE-LITERAL — the no-metadata fallback; a supplied set always wins. */ + : task.column === (context.reviewColumn ?? "in-review"); + if (!inReviewLane || task.paused !== true) return undefined; if (task.mergeDetails?.mergeConfirmed === true) return undefined; const thresholdMs = context.thresholdMs ?? DEFAULT_STALE_PAUSED_REVIEW_THRESHOLD_MS; diff --git a/packages/core/src/task-store/reads.ts b/packages/core/src/task-store/reads.ts index 3108c822f2..4fba45b0dc 100644 --- a/packages/core/src/task-store/reads.ts +++ b/packages/core/src/task-store/reads.ts @@ -20,7 +20,7 @@ import {getAgentLogFilePath} from "../agent-log-file-store.js"; import {getInReviewStalledSignal} from "../in-review-stalled.js"; import {getStalePausedReviewSignal} from "../stale-paused-review.js"; import {getStalePausedTodoSignal} from "../stale-paused-todo.js"; -import {resolveLifecycleColumns} from "../workflow-lifecycle-traits.js"; +import {resolveLifecycleColumns, resolveReviewColumns} from "../workflow-lifecycle-traits.js"; import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; import type {WorkflowIr} from "../workflow-ir-types.js"; @@ -150,17 +150,28 @@ Fail-soft to "in-review" for the same reason as the hold helper: this is read-pa hydration, so a workflow lookup failure must degrade to today's behavior rather than break a board list. Cache is caller-owned so a list pass reads one IR per workflow. */ -async function resolveReviewColumnForTask( +/* +FNXC:WorkflowResolvedColumns 2026-07-30-22:20 (ONE lane answer for all three stall signals): +The three signals decorating a row — `inReviewStall`, `inReviewStalled`, `stalePausedReview` — each +took their own lane input and DISAGREED: two took a singular `reviewColumn` +(`resolveLifecycleColumns().review`, the FIRST column per role) and the third had no seam at all and +used the literal. So one row could be judged in-review by one signal and not by another, and a board +with a separate merge lane beside its human-review lane had a second review column matching none. + +`resolveReviewColumns` is the union of the three review roles. The legacy id stays unioned so a board +mid-rename is never skipped, and all ten call sites now read from THIS answer. +*/ +async function resolveReviewColumnsForTask( store: TaskStore, taskId: string, cache?: Map, -): Promise { +): Promise> { + const columns = new Set(["in-review"]); try { - const lifecycle = resolveLifecycleColumns(await resolveWorkflowIrForTask(store, taskId, cache)); - return lifecycle?.review ?? "in-review"; - } catch { - return "in-review"; - } + const ir = await resolveWorkflowIrForTask(store, taskId, cache); + if (ir) for (const id of resolveReviewColumns(ir)) columns.add(id); + } catch { /* degraded: the legacy id above still answers */ } + return columns; } @@ -220,13 +231,13 @@ export async function getTaskImpl(store: TaskStore, id: string, options?: { acti engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); - const reviewColumnForTask = await resolveReviewColumnForTask(store, task.id); + const reviewColumnsForTask = await resolveReviewColumnsForTask(store, task.id); task.inReviewStalled = mergeQueuedTaskIds.has(task.id) ? undefined : getInReviewStalledSignal(task, { now, executingTaskIds, - reviewColumn: reviewColumnForTask, + reviewColumns: reviewColumnsForTask, thresholdMs: settings.inReviewStalledThresholdMs, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, @@ -383,18 +394,18 @@ export async function listTasksImpl(store: TaskStore, options?: { limit?: number engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); - const reviewColumnForRow = await resolveReviewColumnForTask(store, task.id, listPassIrCache); + const reviewColumnsForRow = await resolveReviewColumnsForTask(store, task.id, listPassIrCache); task.stalePausedReview = getStalePausedReviewSignal(task, { now, thresholdMs: settings.stalePausedReviewThresholdMs, - reviewColumn: reviewColumnForRow, + reviewColumns: reviewColumnsForRow, engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); task.inReviewStalled = isMergeQueued ? undefined : getInReviewStalledSignal(task, { now, executingTaskIds, - reviewColumn: reviewColumnForRow, + reviewColumns: reviewColumnsForRow, thresholdMs: settings.inReviewStalledThresholdMs, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, @@ -416,7 +427,7 @@ export async function listTasksImpl(store: TaskStore, options?: { limit?: number */ try { /* - FNXC:WorkflowResolvedColumns 2026-07-31-08:10 (fleet phase): + FNXC:WorkflowResolvedColumns 2026-07-30-08:10 (fleet phase): Resolved through the SAME per-pass `listPassIrCache` the hold-column read above already uses, so one workflow is read once per pass rather than once per card. @@ -559,13 +570,13 @@ export async function listTasksModifiedSinceImpl(store: TaskStore, since: string card count. Unlike hold — which only matters for a paused card — the review signals apply to any row, so this resolves for every row on the page. */ - const reviewColumnByTaskId = new Map(); + const reviewColumnsByTaskId = new Map>(); const lifecycleByTaskId = new Map>>(); { const irCache = new Map(); for (const pgRow of pageRows) { const row = store.pgRowToTaskRow(pgRow); - reviewColumnByTaskId.set(row.id, await resolveReviewColumnForTask(store, row.id, irCache)); + reviewColumnsByTaskId.set(row.id, await resolveReviewColumnsForTask(store, row.id, irCache)); lifecycleByTaskId.set(row.id, await resolveTaskLifecycleColumns(store, row.id, irCache)); if (store.rowToTask(row).paused !== true) continue; holdColumnByTaskId.set(row.id, await resolveHoldColumnForTask(store, row.id, irCache)); @@ -592,18 +603,18 @@ export async function listTasksModifiedSinceImpl(store: TaskStore, since: string engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); - const reviewColumnForRow = reviewColumnByTaskId.get(task.id) ?? "in-review"; + const reviewColumnsForRow = reviewColumnsByTaskId.get(task.id) ?? new Set(["in-review"]); task.stalePausedReview = getStalePausedReviewSignal(task, { now, thresholdMs: settings.stalePausedReviewThresholdMs, - reviewColumn: reviewColumnForRow, + reviewColumns: reviewColumnsForRow, engineActiveSinceMs: settings.engineActiveSinceMs, engineActivationGraceMs: settings.engineActivationGraceMs, }); task.inReviewStalled = isMergeQueued ? undefined : getInReviewStalledSignal(task, { now, executingTaskIds, - reviewColumn: reviewColumnForRow, + reviewColumns: reviewColumnsForRow, thresholdMs: settings.inReviewStalledThresholdMs, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, @@ -622,7 +633,7 @@ export async function listTasksModifiedSinceImpl(store: TaskStore, since: string now, thresholds: staleThresholds, /* - FNXC:WorkflowLifecycleColumns 2026-07-31-09:00 (fleet — the omitted sibling site): + FNXC:WorkflowLifecycleColumns 2026-07-30-09:00 (fleet — the omitted sibling site): THE MODIFIED-SINCE PASS NEEDS THE LANES TOO. #2746 threaded `lifecycle` into the list pass above and left this one on the defaults, so a renamed board still produced no age-staleness badge for any card arriving through the incremental refresh — which is the @@ -725,7 +736,7 @@ export async function searchTasksImpl(store: TaskStore, query: string, options?: task.inReviewStalled = isMergeQueued ? undefined : getInReviewStalledSignal(task, { now, executingTaskIds, - reviewColumn: await resolveReviewColumnForTask(store, task.id, searchPassIrCache), + reviewColumns: await resolveReviewColumnsForTask(store, task.id, searchPassIrCache), thresholdMs: settings.inReviewStalledThresholdMs, autoMerge: allowsAutoMergeProcessing(task, settings), engineActiveSinceMs: settings.engineActiveSinceMs, diff --git a/packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts b/packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts index 7fff0fd016..9a38c22727 100644 --- a/packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts +++ b/packages/engine/src/__tests__/self-healing-query-filter-blindness.test.ts @@ -194,6 +194,45 @@ describe("self-healing sweeps are bounded by a hardcoded column QUERY, not by th expect(listTasks).toHaveBeenCalledWith(expect.objectContaining({ column: RENAMED_VOCAB.complete })); expect(listTasks).toHaveBeenCalledWith(expect.objectContaining({ column: "done" })); }); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-20:05 (the query-filter class — the last read-shaped sweep): + + `surfaceInReviewStalls` kicks a card that has sat in review past `taskStuckTimeoutMs` back to the + hold lane. Its query asked for the literal `in-review`, so on a renamed board it received NOTHING and + the review column silently stopped draining — an operator sees cards accumulating with no error and + no log line, because a sweep that finds zero candidates is indistinguishable from a healthy one. + + Asserted on the QUERY, like its siblings here, for the reason the file header gives: the outcome is 0 + either way, so only the query argument distinguishes "nothing to do" from "asked the wrong question". + + `checking` AND `in-review` are both expected — `resolveProjectColumnsForRoles` unions the legacy id + deliberately, so a board mid-rename with rows still under the old id is not skipped. + + REVERT PROOF, measured: restore `listTasks({ column: "in-review", slim: false })` and the first + assertion fails; the second keeps passing, which is exactly why asserting only the legacy id would + have been no test at all. + */ + it("surfaceInReviewStalls asks for the board's own review lane, not the literal", async () => { + const stalled = { + ...shippedCard(), + id: "FN-STALL", + column: RENAMED_VOCAB.review, + updatedAt: new Date(Date.now() - 72 * 60 * 60 * 1000).toISOString(), + } as Task; + const { store, listTasks } = productionFaithfulStore([stalled]); + (store as unknown as { getSettings: ReturnType }).getSettings = vi.fn(async () => ({ + globalPause: false, + enginePaused: false, + taskStuckTimeoutMs: 60_000, + }) as Settings); + const manager = new SelfHealingManager(store, { rootDir: "/repo" }); + + await manager.surfaceInReviewStalls(); + + expect(listTasks).toHaveBeenCalledWith(expect.objectContaining({ column: RENAMED_VOCAB.review })); + expect(listTasks).toHaveBeenCalledWith(expect.objectContaining({ column: "in-review" })); + }); + /* FNXC:WorkflowResolvedColumns 2026-07-30-18:05 (#2838 review — greptile P1): diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 9f63f24150..9f5ae7090f 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -8440,7 +8440,33 @@ export class SelfHealingManager extends SelfHealingGitEvidence { const activeMergeTaskId = this.options.getActiveMergeTaskId?.() ?? null; const executingTaskIds = this.options.getExecutingTaskIds?.() ?? new Set(); - const tasks = await this.store.listTasks({ column: "in-review", slim: false }); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-20:05 (the query-filter class — the LAST read-shaped one): + `listTasks({ column: "in-review" })` returns EMPTY on a renamed board, so `surfaceInReviewStalls` + surfaced nothing: a card stalled in review past `taskStuckTimeoutMs` was never kicked back to the + hold lane, and the operator saw a review column that silently stopped draining. Same shape as the + twenty-odd siblings in this file, documented in + `docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md`. + + Read via the project UNION and de-duplicate by id: a board mid-rename has rows under both the old + and the new id, and over-inclusion costs one extra query whose rows the per-card guards below then + filter. Under-inclusion is what was broken. + + `allowsAutoMergeProcessing` and `getInReviewStallReason` already gate every card individually, so + no second lane test is needed here — the union only decides which rows are LOOKED at. + + Measured, not claimed: `lifecycle-column-census.mjs --json` reports `queryRoles.read` at 2 before + this change and 1 after. I first wrote "the last one, 1 -> 0" and the tool said otherwise — one + more read-shaped query survives elsewhere. Stating the number I actually saw rather than the one + that would have read better, because a comment asserting a fact about the rest of the tree is the + decay class this program has already had to correct twice. + */ + const stallReviewColumns = await resolveProjectColumnsForRoles(this.store, REVIEW_ROLES); + const stallCandidatesById = new Map(); + for (const column of stallReviewColumns) { + for (const task of await this.store.listTasks({ column, slim: false })) stallCandidatesById.set(task.id, task); + } + const tasks = [...stallCandidatesById.values()]; let surfaced = 0; for (const task of tasks) { diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 554d92f495..66e5076d55 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -135,12 +135,12 @@ "queryByFile": { "packages/core/src/task-store/async-persistence.ts": 2, "packages/core/src/task-store/merge-queue-ops.ts": 2, - "packages/engine/src/self-healing.ts": 2, "packages/core/src/async-mission-store.ts": 1, "packages/core/src/task-store/archive-lifecycle-2.ts": 1, "packages/core/src/task-store/async-archive-lineage.ts": 1, "packages/core/src/task-store/async-self-healing.ts": 1, "packages/engine/src/agent-tools.ts": 1, - "packages/engine/src/auto-merge-finalization.ts": 1 + "packages/engine/src/auto-merge-finalization.ts": 1, + "packages/engine/src/self-healing.ts": 1 } }