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