From 2fe82cb6441f371ebf135ecb7e1eb8606ad48ea5 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 05:40:54 -0700 Subject: [PATCH] =?UTF-8?q?feat(core):=20resolveReviewColumns=20=E2=80=94?= =?UTF-8?q?=20the=20shared=20answer=20four=20consumers=20each=20invented?= =?UTF-8?q?=20separately=20(#2730)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- .../workflow-lifecycle-traits.test.ts | 61 ++++++++++++++++++- packages/core/src/index.gate.ts | 2 +- packages/core/src/index.ts | 2 +- .../core/src/workflow-lifecycle-traits.ts | 34 +++++++++++ 4 files changed, 96 insertions(+), 3 deletions(-) diff --git a/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts b/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts index af1f933645..df10271fe3 100644 --- a/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts +++ b/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts @@ -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"); + }); +}); diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index 7c6fc8594d..935a5254f2 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -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"; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 732e66da1e..92b2f77646 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -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 { diff --git a/packages/core/src/workflow-lifecycle-traits.ts b/packages/core/src/workflow-lifecycle-traits.ts index dd9399891c..50fd9d3598 100644 --- a/packages/core/src/workflow-lifecycle-traits.ts +++ b/packages/core/src/workflow-lifecycle-traits.ts @@ -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