diff --git a/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts b/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts index 1e90523e97..af1f933645 100644 --- a/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts +++ b/packages/core/src/__tests__/workflow-lifecycle-traits.test.ts @@ -11,6 +11,7 @@ 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 { 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"; describe("columnsWithFlag — builtin:coding trait→columnIds (R8)", () => { const ir = BUILTIN_CODING_WORKFLOW_IR; @@ -273,3 +274,79 @@ describe("resolveTaskLifecycleColumns — U1 store-aware form", () => { await expect(resolveTaskLifecycleColumns(store, "T-1")).resolves.toBeUndefined(); }); }); + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-07:10 (the arity contract, pinned): +`LifecycleColumns` names ONE column per role even when the workflow declares several — nothing +validates that a trait appears at most once, and `resolveLifecycleColumns` takes the head of +`columnsWithFlag`. + +This is asserted rather than left in the doc comment because two production bugs came from assuming +otherwise (PR #2713): a task in a SECOND terminal column was rejected with a 409, and a task in a +human-review lane split from the merge lane was classified as outside review entirely. Both read +like ordinary conversions. + +The point of the pair below is the CONTRAST: the struct is safe for "where should this card go" and +unsafe for "is this card already there". A reader who only sees the first assertion learns the wrong +lesson. +*/ +describe("LifecycleColumns arity — one id per role, even when several qualify", () => { + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-07:40 (PR #2721 review — greptile, and the premise was + wrong): + Uses `complete`, NOT `intake`. My first version demonstrated the arity gap with two intake lanes + and bypassed typing with `as never` to build it — but `validateColumnTraits` raises + `multiple-intake-columns`, so that workflow shape is REJECTED by the product. The test would have + stayed green while documenting something that cannot exist, which is worse than not testing it. + + `complete` genuinely repeats: there is no uniqueness rule for it, nor for `archived`, `hold`, + `countsTowardWip`, `mergeBlocker` or `humanReview`. Only `intake` is validated unique. That is the + real boundary, and it means `intake` comparisons are safe by equality while every other role's are + not — which narrows the call sites at risk rather than widening them. + */ + const twoTerminalsIr: WorkflowIr = { + version: "v2", + name: "two-terminals", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + { id: "released", name: "Released", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }, { id: "end", kind: "end", column: "shipped" }], + edges: [{ from: "start", to: "end" }], + } as WorkflowIr; + + it("is a workflow the product actually ACCEPTS — the premise this rests on", () => { + /* + Asserted, not assumed. My first version of these cases used two INTAKE columns, which + `validateColumnTraits` rejects with `multiple-intake-columns` — so it documented a shape that + cannot exist while staying green. Proving the fixture is valid is what makes the arity gap below + a real hazard rather than a hypothetical one. + */ + const violations = getTraitRegistry().validateColumnTraits(twoTerminalsIr.columns as never); + expect(violations.filter((v) => v.severity === "error")).toEqual([]); + }); + + it("reports only ONE complete column — the second is invisible to the struct", () => { + const lifecycle = resolveLifecycleColumns(twoTerminalsIr); + expect(lifecycle).toBeDefined(); + expect(["shipped", "released"]).toContain(lifecycle!.complete); + }); + + it("so a MEMBERSHIP test against it misses the second column — use columnsWithFlag instead", () => { + const lifecycle = resolveLifecycleColumns(twoTerminalsIr)!; + const bothTerminals = columnsWithFlag(twoTerminalsIr, "complete"); + + // Both are genuinely terminal columns. + expect(bothTerminals).toHaveLength(2); + expect(bothTerminals).toEqual(expect.arrayContaining(["shipped", "released"])); + + // Exactly one fails an equality check against the struct — the shipped-bug shape from PR #2713. + const missed = bothTerminals.find((id) => id !== lifecycle.complete)!; + expect(missed).toBeDefined(); + expect(missed === lifecycle.complete).toBe(false); + // The membership form gets it right. + expect(bothTerminals.includes(missed)).toBe(true); + }); +}); diff --git a/packages/core/src/workflow-lifecycle-traits.ts b/packages/core/src/workflow-lifecycle-traits.ts index d42e0e4ae1..dd9399891c 100644 --- a/packages/core/src/workflow-lifecycle-traits.ts +++ b/packages/core/src/workflow-lifecycle-traits.ts @@ -140,6 +140,34 @@ struct present) apart from "this workflow has no column vocabulary at all" (unde The first is a real workflow shape to honor; the second means the caller has no basis to decide and must skip-and-log rather than guess a legacy literal. */ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-07:00 (arity contract, after two production bugs): +EACH FIELD IS **ONE** COLUMN, EVEN WHEN THE WORKFLOW DECLARES SEVERAL. + +Uniqueness is validated for exactly ONE trait. `TraitRegistry.validateColumnTraits` raises +`multiple-intake-columns` when more than one column carries `intake` — and raises nothing for +`hold`, `countsTowardWip`, `mergeBlocker`, `humanReview`, `complete` or `archived`. Those may +legitimately repeat: a workflow can split `mergeBlocker` and `humanReview` across a merge lane and a +separate sign-off lane, or declare two terminal columns. `columnsWithFlag` returns an array and +`first()` below picks its head, so this struct names only one of each. + +So `intake` is safe to compare by equality; every other field is not. + +That makes these fields safe for ONE question and unsafe for another: + + SAFE "where should this card GO" — a move target must be exactly one column + UNSAFE "is this card ALREADY there" — that is membership; use `columnsWithFlag(ir, flag)` + and test `.includes(task.column)` + +Two shipped bugs came from the unsafe use, both in PR #2713: a task in a second terminal column was +rejected with a 409, and a task in a human-review lane split from the merge lane was classified as +outside review entirely, suppressing comment re-engagement. Both read like ordinary conversions. + +Known call sites comparing `task.column` against these fields: + packages/engine/src/self-healing.ts `columns.intake` SAFE (validated unique); + `columns.hold` AT RISK — hold has no uniqueness rule + packages/core/src/builtin-workflows.ts `lifecycle.intake` SAFE (validated unique) +*/ export interface LifecycleColumns { /** Where new cards land. */ intake: string | undefined;