feat(core): resolveReviewColumns — the shared answer four consumers each invented separately (#2730)
## The pattern `resolveLifecycleColumns().review` is a **single id derived from one flag** (`mergeOrchestration`). The domain is not that shape — a lane can host human review without orchestrating a merge, and a board may declare more than one review lane. So every consumer asking *"is this card in review"* re-derived its own answer, and they drifted: | PR | surface | how it diverged | |---|---|---| | #2713 | routes | terminal columns needed membership; fixed there only | | #2722 | notifier | a `humanReview`-only lane resolved to nothing — the operator's review notification **never fired**, silently | | #2723 | routes | the union was broader than core's single id | | #2728 | CLI | `fn task retry` refused a card `POST /tasks/:id/retry` accepted — the same operator action answering differently per surface | Four files, four patches, **two of them mine** — plus a fifth site inside #2722 that my own first pass missed, which is exactly the enumeration failure I had been pointing out in other people's PRs. Each recurrence cost a review round, and the next consumer would have invented a fifth answer. ## The change ```ts export function resolveReviewColumns(ir: WorkflowIr): string[] ``` **Additive on purpose.** `.review` is untouched, so nothing that reads it changes behaviour. This is the *missing* helper, not a reshaping of the existing one — `.review` stays correct for its own question ("which single lane hosts the merge gate"); this answers the other one ("is this card **already** in a review lane"). **Monotonic**, which #2723's review round argued about at length: a lane carrying **both** `humanReview` and `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. **Returns empty rather than defaulting to `in-review`.** The legacy fallback belongs to the caller, which knows whether refusing or admitting is the safe direction for its own guard — the notifier and the retry gate fail in opposite directions, and baking one choice in here would make one of them wrong. ## Verification Six tests, both directions: human-review-only lane, multiple lanes, monotonicity, de-duplication, empty result, and agreement with the shipped coding workflow. One asserts the divergence directly — `resolveReviewColumns` finds the lane while `.review` returns `undefined`. **Mutation: dropping the two extra flags fails 2 of 26.** 26 core trait tests green · core `tsc` clean · lint clean · gate green (487 + 158 + 10 + 71). **No census movement** — this adds capability and converts nothing. ## Follow-up, not done here Migrating the four consumers onto it. Each is an open PR with its own review thread, and switching them under their authors mid-flight would be worse than letting them adopt it. The helper is the prerequisite; adoption is theirs.
This commit is contained in:
@@ -8,7 +8,7 @@ byte-identical on the default workflow. The custom cases prove KTD-10 fallback.
|
||||
import { describe, expect, it } from "vitest";
|
||||
import "../builtin-traits.js"; // register built-in traits
|
||||
import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js";
|
||||
import { columnsWithFlag, columnHasFlag, resolveReboundTarget, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveLifecycleColumns, resolveTaskLifecycleColumns } from "../workflow-lifecycle-traits.js";
|
||||
import { columnsWithFlag, columnHasFlag, resolveReboundTarget, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveLifecycleColumns, resolveReviewColumns, resolveTaskLifecycleColumns } from "../workflow-lifecycle-traits.js";
|
||||
import { BUILTIN_CODING_IDEAS_WORKFLOW_IR } from "../builtin-coding-ideas-workflow-ir.js";
|
||||
import type { WorkflowIr } from "../workflow-ir-types.js";
|
||||
import { getTraitRegistry } from "../trait-registry.js";
|
||||
@@ -350,3 +350,62 @@ describe("LifecycleColumns arity — one id per role, even when several qualify"
|
||||
expect(bothTerminals.includes(missed)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-05:20:
|
||||
The set-shaped answer to "is this card ALREADY in a review lane", which four consumers each invented
|
||||
separately before this existed (#2713, #2722, #2723, #2728). Both directions are asserted, because the
|
||||
whole reason it exists is that the single-id `.review` silently answers a different question.
|
||||
*/
|
||||
describe("resolveReviewColumns", () => {
|
||||
const ir = (columns: Array<{ id: string; traits: Array<{ trait: string }> }>) =>
|
||||
({ version: "v2", id: "wf", name: "wf", columns: columns.map((c) => ({ ...c, name: c.id })), nodes: [], edges: [] }) as never;
|
||||
|
||||
it("includes a lane carrying human-review WITHOUT the merge trait", () => {
|
||||
/* The #2722 defect: `.review` reads mergeOrchestration only, so this lane resolved to nothing and
|
||||
the review notification never fired on a renamed board. */
|
||||
const columns = ir([
|
||||
{ id: "building", traits: [{ trait: "wip" }] },
|
||||
{ id: "signoff", traits: [{ trait: "human-review" }] },
|
||||
]);
|
||||
|
||||
expect(resolveReviewColumns(columns)).toEqual(["signoff"]);
|
||||
expect(resolveLifecycleColumns(columns)?.review).toBeUndefined();
|
||||
});
|
||||
|
||||
it("includes EVERY review lane, not just the first", () => {
|
||||
const columns = ir([
|
||||
{ id: "merge-gate", traits: [{ trait: "merge" }] },
|
||||
{ id: "signoff", traits: [{ trait: "human-review" }] },
|
||||
]);
|
||||
|
||||
expect(new Set(resolveReviewColumns(columns))).toEqual(new Set(["merge-gate", "signoff"]));
|
||||
});
|
||||
|
||||
it("is MONOTONIC: a lane with both traits stays in the set", () => {
|
||||
/* The #2723 review round argued for excluding this. Adding a trait must never REMOVE a lane —
|
||||
otherwise a card stops counting as in review because its column gained an unrelated capability. */
|
||||
const columns = ir([
|
||||
{ id: "merge-gate", traits: [{ trait: "merge" }] },
|
||||
{ id: "signoff", traits: [{ trait: "merge" }, { trait: "human-review" }] },
|
||||
]);
|
||||
|
||||
expect(resolveReviewColumns(columns)).toContain("signoff");
|
||||
});
|
||||
|
||||
it("does not duplicate a lane that carries several review traits", () => {
|
||||
const columns = ir([{ id: "signoff", traits: [{ trait: "merge" }, { trait: "human-review" }] }]);
|
||||
|
||||
expect(resolveReviewColumns(columns)).toEqual(["signoff"]);
|
||||
});
|
||||
|
||||
it("returns EMPTY when no lane reviews, so callers keep their own fallback", () => {
|
||||
/* Deliberately not defaulting to `in-review` here: the fallback belongs to the caller, which knows
|
||||
whether refusing or admitting is the safe direction for its own guard. */
|
||||
expect(resolveReviewColumns(ir([{ id: "building", traits: [{ trait: "wip" }] }]))).toEqual([]);
|
||||
});
|
||||
|
||||
it("agrees with the shipped coding workflow", () => {
|
||||
expect(resolveReviewColumns(BUILTIN_CODING_WORKFLOW_IR)).toContain("in-review");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -2254,7 +2254,7 @@ export { createWorkflowEventBus, getWorkflowEventBus, emitWorkflowLifecycleEvent
|
||||
export type { WorkflowEventBus, WorkflowEventSubscriber, WorkflowEventSubscription } from "./workflow-events.js";
|
||||
export { findWorkflowEventShapeViolations, isIdsOnlyWorkflowEvent, MAX_ID_VALUE_LENGTH, IMPLEMENTATION_EXITS } from "./types/workflow-events.js";
|
||||
export type { WorkflowLifecycleEvent, WorkflowLifecycleEventType, WorkflowLifecycleEventBase, TaskTransitionedEvent, NodeEnteredEvent, NodeCompletedEvent, RunSuspendedEvent, RunResumedEvent, WorkflowEventShapeViolation, ImplementationExit } from "./types/workflow-events.js";
|
||||
export { columnsWithFlag, columnHasFlag, resolveReboundTarget, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveLifecycleColumns, resolveTaskLifecycleColumns, resolveTerminalColumns } from "./workflow-lifecycle-traits.js";
|
||||
export { columnsWithFlag, columnHasFlag, resolveReboundTarget, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveLifecycleColumns, resolveTaskLifecycleColumns, resolveTerminalColumns, resolveReviewColumns } from "./workflow-lifecycle-traits.js";
|
||||
export type { LifecycleColumns } from "./workflow-lifecycle-traits.js";
|
||||
export { resolveReviewLevelSteps, applyReviewLevelPreset } from "./review-level-preset.js";
|
||||
export { LEGACY_STATUS_ADOPTION, resolveLegacyStatusAdoption, resolveReviewLevelBackfill, planLegacyAdoption, resolveOrphanedPendingStepResults, type LegacyAdoptionPlan, type LegacyAdoptionCandidate, type LegacyAdoptionAction, type LegacyAdoptionKind } from "./legacy-adoption.js";
|
||||
|
||||
@@ -467,7 +467,7 @@ export { createWorkflowEventBus, getWorkflowEventBus, emitWorkflowLifecycleEvent
|
||||
export type { WorkflowEventBus, WorkflowEventSubscriber, WorkflowEventSubscription } from "./workflow-events.js";
|
||||
export { findWorkflowEventShapeViolations, isIdsOnlyWorkflowEvent, MAX_ID_VALUE_LENGTH, IMPLEMENTATION_EXITS } from "./types/workflow-events.js";
|
||||
export type { WorkflowLifecycleEvent, WorkflowLifecycleEventType, WorkflowLifecycleEventBase, TaskTransitionedEvent, NodeEnteredEvent, NodeCompletedEvent, RunSuspendedEvent, RunResumedEvent, WorkflowEventShapeViolation, ImplementationExit } from "./types/workflow-events.js";
|
||||
export { columnsWithFlag, columnHasFlag, resolveReboundTarget, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveLifecycleColumns, resolveTaskLifecycleColumns, resolveTerminalColumns } from "./workflow-lifecycle-traits.js";
|
||||
export { columnsWithFlag, columnHasFlag, resolveReboundTarget, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveLifecycleColumns, resolveTaskLifecycleColumns, resolveTerminalColumns, resolveReviewColumns } from "./workflow-lifecycle-traits.js";
|
||||
export type { LifecycleColumns } from "./workflow-lifecycle-traits.js";
|
||||
export { resolveReviewLevelSteps, applyReviewLevelPreset } from "./review-level-preset.js";
|
||||
export {
|
||||
|
||||
@@ -58,6 +58,40 @@ export function resolveCompleteColumn(ir: WorkflowIr): string | undefined {
|
||||
return columnsWithFlag(ir, "complete")[0];
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:WorkflowLifecycleColumns 2026-07-31-05:20 (the divergence four consumers each solved differently):
|
||||
REVIEW IS A SET, AND `humanReview` COUNTS.
|
||||
|
||||
`resolveLifecycleColumns().review` is a SINGLE id derived from ONE flag (`mergeOrchestration`). The
|
||||
domain is not that shape: a lane can host human review without orchestrating a merge, and a board may
|
||||
declare more than one review lane. So every consumer asking "is this card in review" re-derived its own
|
||||
answer, and they drifted:
|
||||
|
||||
#2713 routes terminal columns needed membership; fixed there only
|
||||
#2722 notifier a `humanReview`-only lane resolved to nothing — review notifications never fired
|
||||
#2723 routes the union was broader than core's single id
|
||||
#2728 CLI `fn task retry` refused a card `POST /tasks/:id/retry` accepted
|
||||
|
||||
Four files, four patches, and a fifth site inside #2722 itself that the first pass missed. The shared
|
||||
answer belongs here.
|
||||
|
||||
ADDITIVE ON PURPOSE. `resolveLifecycleColumns().review` is untouched, so nothing that reads it changes
|
||||
behaviour — this is the missing helper, not a reshaping of the existing one. `.review` remains correct
|
||||
for its own question ("which single lane does the merge gate live in"); this answers the other one
|
||||
("is this card ALREADY in a review lane"), which is the question every drifting consumer was asking.
|
||||
|
||||
MONOTONIC, which the #2723 review round argued about: a column carrying BOTH `humanReview` and
|
||||
`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.
|
||||
*/
|
||||
export function resolveReviewColumns(ir: WorkflowIr): string[] {
|
||||
return [...new Set([
|
||||
...columnsWithFlag(ir, "mergeOrchestration"),
|
||||
...columnsWithFlag(ir, "mergeBlocker"),
|
||||
...columnsWithFlag(ir, "humanReview"),
|
||||
])];
|
||||
}
|
||||
|
||||
/**
|
||||
* U7 — the workflow's MERGE-ORCHESTRATION column: the first column carrying the
|
||||
* `mergeOrchestration` trait (where the merge-gate node lives). Merge-failure
|
||||
|
||||
Reference in New Issue
Block a user