From 2fd798cb36f1cb1b3a7c2b501c43b8f49f7b9764 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 22:25:24 -0700 Subject: [PATCH] core: every review card reported a false stall on a renamed board (#2970) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The failure mode worth distinguishing: the rest of this family went quiet on a renamed board. This one shouted.** `getInReviewStallReason` satisfied its **own** lane check from `context.reviewColumns` — then called `getTaskMergeBlocker` **without** them. That helper re-ran its column-identity check against the literal `in-review` and returned, for a perfectly healthy card: ``` task is in 'signoff', must be in 'in-review' ``` …which was surfaced as `{ code: "merge-blocker" }`. **Every in-review card on a renamed board was flagged as stalled**, each citing a lane the board does not have. That is how a signal stops being read at all. ## A second symptom, found by the revert rather than by reading On a **genuinely failed** card, the identity message wins over the real one. The operator saw the bogus column complaint instead of `task is marked 'failed': merge verification failed`. So it did not only invent stalls — it **masked the true reason for real ones**. I would not have noticed that from the diff; it showed up because the revert run asserted on the reason text. ## Same shape, last one in the family The outer question was resolved and the inner one was not — the half-conversion the helper's own comment records for `moves.ts`, and #2963/#2964 fixed for the merge entry points. This is the last site the audit turned up where the lane answer was already in scope and simply not forwarded. ## Revert results | | reverted → | | --- | --- | | the unforwarded call (what ships today) | **2 of 3 fail** — healthy card reports a merge-blocker stall; failed card reports the wrong reason | **Fixture note worth keeping:** `paused` is deliberately *not* the genuine-stall case. An earlier guard returns `undefined` for a paused card before the merge blocker is ever consulted, so that case would pass whether or not the lanes are forwarded — the vacuous shape this series has produced eight times. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `@fusion/core` full suite **4878 passed** (457 files); `tsc` core clean; lint, lifecycle census `--strict`, FNXC gate, changesets all clean. --- .changeset/stall-signal-forwards-lanes.md | 7 ++ .../in-review-stall-forwards-lanes.test.ts | 80 +++++++++++++++++++ packages/core/src/in-review-stall.ts | 16 +++- scripts/lib/lane-wiring-baseline.json | 1 - 4 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 .changeset/stall-signal-forwards-lanes.md create mode 100644 packages/core/src/__tests__/in-review-stall-forwards-lanes.test.ts diff --git a/.changeset/stall-signal-forwards-lanes.md b/.changeset/stall-signal-forwards-lanes.md new file mode 100644 index 0000000000..e0af10095a --- /dev/null +++ b/.changeset/stall-signal-forwards-lanes.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Review cards no longer all report a false stall on boards with renamed columns. +category: fix +dev: `getInReviewStallReason` satisfied its own lane check from `context.reviewColumns` but called `getTaskMergeBlocker` without them, so the helper re-checked against the literal `in-review` and returned an identity message for every healthy card — surfaced as a merge-blocker stall, and masking the real reason on genuinely failed ones. diff --git a/packages/core/src/__tests__/in-review-stall-forwards-lanes.test.ts b/packages/core/src/__tests__/in-review-stall-forwards-lanes.test.ts new file mode 100644 index 0000000000..dae29c69d9 --- /dev/null +++ b/packages/core/src/__tests__/in-review-stall-forwards-lanes.test.ts @@ -0,0 +1,80 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-21:10 (a false-alarm generator, not a silent miss): + +`getInReviewStallReason` satisfied its OWN lane check from `context.reviewColumns`, then called +`getTaskMergeBlocker` WITHOUT them. That helper re-ran its column-identity check against the literal +`in-review`, so on a renamed board a perfectly healthy review card came back with + + task is in 'signoff', must be in 'in-review' + +which was surfaced as `{ code: "merge-blocker" }` — a stall reason naming a column the board does not +have, for EVERY card in review. + +This is the failure mode worth distinguishing from the rest of the family: the others went quiet on a +renamed board, this one shouted. An operator would see every in-review card flagged as stalled, each +citing a lane that does not exist, which is how a signal stops being read at all. + +The outer question was resolved and the inner one was not — the same half-conversion the helper's own +comment records for `moves.ts`, and #2963/#2964 fixed for the merge paths. +*/ +import { describe, expect, it } from "vitest"; +import { getInReviewStallReason } from "../in-review-stall.js"; +import { getTaskMergeBlocker } from "../task-merge.js"; +import type { Task } from "../types.js"; + +/** A healthy card sitting in a renamed review lane: nothing wrong with it. */ +function healthyReviewCard(column: string): Task { + return { + id: "FN-HEALTHY", + column, + paused: false, + status: null, + error: null, + steps: [{ id: "s1", status: "done" }], + workflowStepResults: [], + worktree: "/tmp/wt", + mergeDetails: {}, + mergeRetries: 0, + updatedAt: new Date().toISOString(), + } as unknown as Task; +} + +describe("the stall signal forwards its resolved lanes to the merge blocker", () => { + it("reports NO stall for a healthy card on a RENAMED review lane", () => { + const signal = getInReviewStallReason(healthyReviewCard("signoff"), { + reviewColumns: new Set(["signoff"]), + now: Date.now(), + }); + + expect(signal).toBeUndefined(); + }); + + it("reproduces the shipped false alarm when the lanes are not forwarded", () => { + /* + Drives the helper the way the unfixed code did — lanes known to the caller, not passed on. The + reason string is the operator-visible text, so a regression reports what they would actually see. + */ + const blocker = getTaskMergeBlocker(healthyReviewCard("signoff")); + + expect(blocker).toBe("task is in 'signoff', must be in 'in-review'"); + }); + + it("still reports a genuine stall on a renamed lane", () => { + /* + Non-vacuous companion: forwarding lanes must not silence REAL blockers, or the fix would trade a + wall of false alarms for silence. A failed card in the board's own review lane is genuinely stalled + and must still be reported. + + Note `paused` is deliberately not the case used here — an earlier guard returns undefined for a + paused card before the merge blocker is ever consulted, so it would pass whether or not the lanes + are forwarded. That is the vacuous shape this file exists to avoid. + */ + const signal = getInReviewStallReason( + { ...healthyReviewCard("signoff"), status: "failed", error: "merge verification failed" } as Task, + { reviewColumns: new Set(["signoff"]), now: Date.now() }, + ); + + expect(signal?.code).toBe("merge-blocker"); + expect(signal?.reason).toContain("failed"); + }); +}); diff --git a/packages/core/src/in-review-stall.ts b/packages/core/src/in-review-stall.ts index f44c2c1bdd..57c71db2de 100644 --- a/packages/core/src/in-review-stall.ts +++ b/packages/core/src/in-review-stall.ts @@ -252,7 +252,21 @@ export function getInReviewStallReason( }; } - const mergeBlocker = getTaskMergeBlocker(task); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-21:05 (this reported EVERY healthy review card as stalled): + Forward the resolved lanes. This function had already satisfied its OWN lane check above using + `context.reviewColumns`, then called getTaskMergeBlocker without them — so the helper re-ran its + identity check against the literal `in-review` and, on a renamed board, returned + `task is in 'signoff', must be in 'in-review'` for a perfectly healthy card. + + That string was then surfaced as `{ code: "merge-blocker" }`, i.e. a stall reason naming a column + the board does not have — for every card in review. Worse than silence: the operator gets a wall of + false stalls, each citing a lane that does not exist. + + The outer question was resolved and the inner one was not — the same half-conversion recorded at the + helper itself for moves.ts, and fixed in #2963/#2964 for the merge paths. + */ + const mergeBlocker = getTaskMergeBlocker(task, { reviewColumns: context.reviewColumns }); if (mergeBlocker) { if (mergeBlocker.startsWith(FAILED_TASK_MERGE_BLOCKER_PREFIX)) { const error = mergeBlocker.slice(FAILED_TASK_MERGE_BLOCKER_PREFIX.length).trim(); diff --git a/scripts/lib/lane-wiring-baseline.json b/scripts/lib/lane-wiring-baseline.json index e2193e5251..95652dffe6 100644 --- a/scripts/lib/lane-wiring-baseline.json +++ b/scripts/lib/lane-wiring-baseline.json @@ -1,6 +1,5 @@ { "counts": { - "packages/core/src/in-review-stall.ts": 1, "packages/core/src/task-merge.ts": 1, "packages/core/src/task-store/moves.ts": 2, "packages/engine/src/auto-merge-finalization.ts": 1,