root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(core): resolveReviewColumns is the BROAD set — and one of its consumers must NOT migrate onto it (#2750)

Browse Source 
## A flaw in the helper I merged in #2730

Found by trying to do the migration I had been advocating for three
rounds. One **name** was answering two questions:

| | question | answer |
|---|---|---|
| **broad** | "is this card in a lane where review happens?" | every
`mergeOrchestration` lane + every `mergeBlocker`/`humanReview` lane —
**this function** |
| **narrow** | "is this card in *the* review lane the engine acts on?" |
`resolveLifecycleColumns().review` = `columnsWithFlag(ir,
"mergeOrchestration")[0]` — **one** lane |

The narrow answer is what the executor, the scheduler and
`project-engine` act on. **A caller that admits on the broad set and
then MOVES the card moves cards the engine does not consider in
review.**

## The correction I owe

I have been arguing across #2722, #2723 and #2728 that the inline review
unions should converge on this helper. For the notifier that is right —
over-admission there just means an extra notification.

For `register-task-workflow-routes.ts` it is **wrong**. That resolver is
deliberately narrower (#2723): its re-engagement *moves* the card, so
admitting a second merge lane is a state change the engine will not
agree with. Its local copy is **not drift from this helper — it is the
other question.** Migrating it would reintroduce precisely the
over-admission that PR's review round reasoned away.

I was about to make that change. Reading both implementations side by
side is the only thing that stopped me, and "consolidate the duplicates"
would have looked like an obvious cleanup to the next person too.

## What this PR does

Nothing to behaviour. It writes the distinction down **at the helper**,
where a consumer reaching for "the review columns" will see it, and pins
the difference with a test.

The test needs a board declaring `mergeOrchestration` **twice** — no
default lineage does, which is exactly why the two answers look
identical everywhere else and why the conflation survived review.

**Mutation: narrowing this helper to the first merge lane — the
consolidation someone would reasonably attempt — fails the test.**

## Verification

30 trait tests green · core `tsc` clean · lint clean (0 errors) · gate
green (487 + 158 + 10 + 71). No census movement.

## Not done here

Migrating the notifier and the CLI copies onto this helper. Those
genuinely should converge, but both live in open PRs (#2722,
#2728/#2736) with live review threads; switching them under their
authors mid-flight is worse than letting them adopt it once this
distinction is documented.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Clarified the distinction between broad review-capable lanes and the
workflow’s primary review lane.
* Documented how review lane selection affects workflow state handling.

* **Tests**
* Added coverage confirming that review detection includes all matching
merge lanes.
* Verified lifecycle review selection continues to use only the primary
merge lane.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
gsxdsm
2026-07-30 06:35:27 -07:00
committed by GitHub
parent 8e50967279
commit b9f3fb0ff2
2 changed files with 51 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
packages/core/src/__tests__/workflow-lifecycle-traits.test.ts
View File

@@ -405,6 +405,32 @@ describe("resolveReviewColumns", () => {
expect(resolveReviewColumns(ir([{ id: "building", traits: [{ trait: "wip" }] }]))).toEqual([]);
});
it("is BROADER than `.review` on a board with two merge lanes — the distinction callers must choose between", () => {
/*
FNXC:WorkflowLifecycleColumns 2026-07-30-11:30:
Pinned because one NAME was answering two questions, and the difference only appears on a board that
declares `mergeOrchestration` twice — which no default lineage does.
BROAD (this helper) every merge lane, plus mergeBlocker/humanReview lanes.
Safe where over-admission is harmless: notifications, badges.
NARROW (`resolveLifecycleColumns`) the FIRST merge lane only — what the executor, scheduler and
project-engine act on.
A caller that admits on the broad set and then MOVES the card moves cards the engine does not consider
in review. `register-task-workflow-routes.ts` keeps its own narrower resolver for that reason (#2723);
this test is what stops someone "consolidating" the two and silently re-admitting the second lane.
*/
const twoMergeLanes = ir([
{ id: "building", traits: [{ trait: "wip" }] },
{ id: "merge-gate", traits: [{ trait: "merge" }] },
{ id: "second-gate", traits: [{ trait: "merge" }] },
]);
expect(resolveReviewColumns(twoMergeLanes)).toEqual(["merge-gate", "second-gate"]);
// The engine's answer is ONE lane, and it is the first.
expect(resolveLifecycleColumns(twoMergeLanes)?.review).toBe("merge-gate");
});
it("agrees with the shipped coding workflow", () => {
expect(resolveReviewColumns(BUILTIN_CODING_WORKFLOW_IR)).toContain("in-review");
});

25
packages/core/src/workflow-lifecycle-traits.ts
View File

@@ -84,6 +84,31 @@ MONOTONIC, which the #2723 review round argued about: a column carrying BOTH `hu
`mergeOrchestration` is included. Adding a trait must never remove a lane from this set, or a card
stops counting as in review because its column gained an unrelated capability.
*/
/*
FNXC:WorkflowLifecycleColumns 2026-07-30-11:30 (a flaw in this helper as merged, found by trying to
migrate its consumers onto it):
THIS IS THE BROAD SET, AND IT IS THE WRONG ANSWER FOR STATE-CHANGING ADMISSION.
Two different questions were being answered by one name:
BROAD "is this card in a lane where review happens?" -> every mergeOrchestration lane, plus every
mergeBlocker and humanReview lane. Safe when over-admission is harmless: notifications, badges,
read-only surfaces. This function.
NARROW "is this card in THE review lane the engine acts on?" -> `resolveLifecycleColumns().review` is
`columnsWithFlag(ir, "mergeOrchestration")[0]`, ONE lane, and that is what the executor, the
scheduler and project-engine act on. A caller that ADMITS on the broad set and then MOVES the
card will move cards the engine does not consider in review.
`register-task-workflow-routes.ts` keeps its own narrower resolver for exactly that reason (#2723): its
re-engagement moves the card, so admitting a SECOND merge lane is a state change the engine will not
agree with. That local copy is not drift from this helper — it is the other question, and migrating it
onto this one would reintroduce the over-admission its review round reasoned away.
Stated here because the name does not carry the distinction: a future consumer reaching for "the review
columns" on a state-changing path wants the narrow form. The pair below is pinned in
`workflow-lifecycle-traits.test.ts`.
*/
export function resolveReviewColumns(ir: WorkflowIr): string[] {
return [...new Set([
...columnsWithFlag(ir, "mergeOrchestration"),