core: every review card reported a false stall on a renamed board (#2970)

**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.
This commit is contained in:
gsxdsm
2026-07-30 22:25:24 -07:00
committed by GitHub
parent df73bbc14a
commit 2fd798cb36
4 changed files with 102 additions and 2 deletions

View File

@@ -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.

View File

@@ -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");
});
});

View File

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

View File

@@ -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,