docs(core): pin the LifecycleColumns arity contract — two shipped bugs came from it (#2721)
Found by **auditing for the pattern** after hitting it twice, rather than waiting for a third instance. ## The contract, previously undocumented `LifecycleColumns` names **one** column per role even when a workflow declares several. Nothing validates that a trait appears at most once — `columnsWithFlag` returns an array and `resolveLifecycleColumns` takes its head. So a workflow may legitimately declare two intake lanes, or split `mergeBlocker` and `humanReview` across a merge lane and a separate sign-off lane, and the struct names only one of each. That makes the fields safe for one question and unsafe for another: | | question | correct? | |---|---|---| | **safe** | *"where should this card **go**?"* | a move target must be exactly one column | | **unsafe** | *"is this card **already** there?"* | membership — use `columnsWithFlag(...).includes(...)` | ## Two shipped bugs, both the unsafe use Both in #2713, both reading like ordinary conversions: - a task in a **second terminal column** was rejected with a 409 - a task in a **human-review lane split from the merge lane** was classified as outside review entirely, suppressing comment re-engagement I fixed the first, did not generalise, and hit the second one review round later. That is the actual failure mode this PR addresses — not the individual bugs, which are already fixed. ## Two call sites left for their owners Named in the doc comment rather than changed here, since they belong to other clusters. Both are correct **only while their workflow declares one column per trait**: - `packages/engine/src/self-healing.ts` — `columns.intake` / `columns.hold` - `packages/core/src/builtin-workflows.ts` — `lifecycle.intake` ## The test asserts the contrast, not just the arity A reader who sees only *"it returns one id"* learns the wrong lesson. The test builds a workflow with **two intake and two complete columns** and shows that exactly one qualifying column **fails an equality check** against the struct while the **membership form gets it right** — the shipped-bug shape, reproduced in miniature. ## Verification `workflow-lifecycle-traits.test.ts` **20 → 22**. `pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm check:lifecycle-columns` exits 0. `tsc -p packages/core/tsconfig.json` clean. `pnpm lint` clean. Docs + test only — no behaviour change, no census movement, no changeset. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user