From d56c635c7782b0be7e1e2b2df76da8cb1abe3751 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 13:18:08 -0700 Subject: [PATCH] test(dashboard): cover the three lane resolvers nobody was testing (#2826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why this exists #2821's review found a bug that lived entirely in a lane **builder** while every test drove the **guard** that consumed it. That is a structural blind spot, not a one-off: injecting a resolved value into a synchronous guard makes the guard testable and the resolver invisible. So I audited every lane helper I added this session. Three had **no direct coverage at all** — `archivedColumnsForTask`, `wipColumnsForTask`, `preWipColumnsForTask`. Their callers were tested; the functions were not. ## They shared the defect that review named Each read `resolved.length > 0 ? resolved : legacyId`, which conflates two different boards: - a **v1 upgrade** — `synthesizeDefaultColumns` emits `traits: []` on every column, so the legacy id is the only vocabulary that exists, and falling back is correct; - a **v2 board that expresses traits** and declares no lane of that role — where the legacy id names a column the board may still *have* and deliberately did not give the role. Falling back there widens the guard onto a role the board explicitly withheld. `declaresAnyLifecycleTrait` separates them, matching the shape #2821's review established for `resolveNodeOverrideLanes`. ## The fixture trap, which is the part worth reading **My first fixture could not see the bug.** It traited the role under test — and where the role *is* traited, the two shapes agree: both return the traited lane. Mutating a helper back to the old shape left all 15 cases green. The shapes diverge only when the resolved set is **empty while traits are expressed**. Each helper now has that case explicitly, with a fixture that traits something *other* than the role under test. **Mutation-verified per helper:** all three reverted independently now fail. Before the extra case, none did. This is the second time this session a fixture built with the production path normalised away the very thing under test. Worth stating as a rule: a renamed-lane fixture proves the resolver reads traits; only a *traits-expressed-but-role-absent* fixture proves what it does when the answer is legitimately nothing. ## Verification - `task-lifecycle-lanes.test.ts` → 18 passed (was 15, none covering these three) - consumer suites (`github-issue-comment`, `planning-board-tools`, `register-git-github.review-lanes`) → 64 passed together - `pnpm test:gate` → 161 + 487 + 13 + 71 - `--strict` → 0; `tsc --noEmit` and `pnpm lint` → 0 errors Co-authored-by: Claude Opus 5 (1M context) --- .../__tests__/task-lifecycle-lanes.test.ts | 98 ++++++++++++++++++- .../dashboard/src/task-lifecycle-lanes.ts | 28 ++++-- 2 files changed, 118 insertions(+), 8 deletions(-) diff --git a/packages/dashboard/src/__tests__/task-lifecycle-lanes.test.ts b/packages/dashboard/src/__tests__/task-lifecycle-lanes.test.ts index 00f07ee06d..47c0024b06 100644 --- a/packages/dashboard/src/__tests__/task-lifecycle-lanes.test.ts +++ b/packages/dashboard/src/__tests__/task-lifecycle-lanes.test.ts @@ -23,7 +23,7 @@ resolved set is EMPTY even though the columns exist) must BOTH keep the legacy p */ import { describe, expect, it, vi } from "vitest"; import "@fusion/core"; // registers the built-in column traits so flags resolve -import { landedColumnsForTask, completeColumnsForTask } from "../task-lifecycle-lanes.js"; +import { landedColumnsForTask, completeColumnsForTask, archivedColumnsForTask, wipColumnsForTask, preWipColumnsForTask } from "../task-lifecycle-lanes.js"; function storeWith(ir: unknown, workflowId = "wf") { const selection = { workflowId, stepIds: [] as string[] }; @@ -113,3 +113,99 @@ describe("completeColumnsForTask is narrower than the landed set", () => { expect([...(await completeColumnsForTask(store, "FN-1"))]).toEqual(["done"]); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-00:05 (self-audit after #2821's review): + +THE RESOLVERS NOBODY WAS TESTING. + +#2821's review found a bug that lived entirely in a lane BUILDER while every test drove the guard +that consumed it — injecting a value makes the guard testable and the resolver invisible. Auditing +the helpers I added this session for the same shape found three with no direct coverage at all: +`archivedColumnsForTask`, `wipColumnsForTask`, `preWipColumnsForTask`. Their callers are tested; the +functions themselves were not. + +They also shared the defect that review named. Each read `resolved.length > 0 ? resolved : legacyId`, +which conflates a v1 upgrade (traits synthesised empty, so the legacy id is the only vocabulary) with +a v2 board that expresses traits and declares no lane of that role — where falling back onto the +legacy id widens the guard onto a role the board deliberately did not assign. + +Each helper is now pinned on all four outcomes: the renamed lane, an untraited legacy NAME, the v1 +fallback, and the unresolvable fallback. +*/ +describe("the lane resolvers themselves, not just their callers", () => { + const storeFor = (ir: unknown) => { + const selection = { workflowId: "wf", stepIds: [] as string[] }; + return { + getTaskWorkflowSelection: () => selection, + getTaskWorkflowSelectionAsync: async () => selection, + getWorkflowDefinition: async () => (ir === undefined ? undefined : { id: "wf", ir }), + } as never; + }; + + /* Traits ARE expressed here, and `done`/`archived`/`in-progress`/`todo` appear WITHOUT them. */ + const TRAITED_WITH_LEGACY_NAMES = { + version: "v2", id: "wf", name: "wf", nodes: [], edges: [], + columns: [ + { id: "todo", name: "Not intake", traits: [] }, + { id: "in-progress", name: "Not wip", traits: [] }, + { id: "done", name: "Not complete", traits: [] }, + { id: "archived", name: "Not archived", traits: [] }, + { id: "backlog", name: "Backlog", traits: [{ trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "attic", name: "Attic", traits: [{ trait: "archived" }] }, + ], + }; + + const V1 = { + version: "v2", id: "wf", name: "wf", nodes: [], edges: [], + columns: ["todo", "in-progress", "in-review", "done", "archived"].map((id) => ({ id, name: id, traits: [] })), + }; + + const cases: Array<[string, (s: never, id: string) => Promise>, string, string]> = [ + ["archivedColumnsForTask", archivedColumnsForTask, "attic", "archived"], + ["wipColumnsForTask", wipColumnsForTask, "building", "in-progress"], + ["preWipColumnsForTask", preWipColumnsForTask, "backlog", "todo"], + ]; + + for (const [name, resolve, renamed, legacy] of cases) { + it(`${name} returns the traited lane and EXCLUDES the untraited legacy name`, async () => { + const lanes = await resolve(storeFor(TRAITED_WITH_LEGACY_NAMES), "FN-1"); + expect(lanes.has(renamed)).toBe(true); + /* The board declares this column and deliberately did not give it the role. */ + expect(lanes.has(legacy)).toBe(false); + }); + + it(`${name} returns EMPTY when traits are expressed but no column carries this role`, async () => { + /* + The case that actually separates the two shapes, and the reason the assertion above could not. + Where the role IS traited, `resolved.length > 0 ? resolved : legacy` and "trust the resolved + set" agree — both return the traited lane. They diverge only when the resolved set is EMPTY + while traits ARE expressed: the old shape falls back onto the legacy NAME, which this board + declares and deliberately did not give the role; the new one takes the board at its word. + + Caught by mutation: reverting the helper left every other case in this suite green. + */ + const traitedElsewhere = { + version: "v2", id: "wf", name: "wf", nodes: [], edges: [], + columns: [ + { id: "todo", name: "plain", traits: [] }, + { id: "in-progress", name: "plain", traits: [] }, + { id: "done", name: "plain", traits: [] }, + { id: "archived", name: "plain", traits: [] }, + /* One trait, and deliberately never the role under test. */ + { id: "signoff", name: "Signoff", traits: [{ trait: "merge" }] }, + ], + }; + expect([...(await resolve(storeFor(traitedElsewhere), "FN-1"))]).toEqual([]); + }); + + it(`${name} falls back to the legacy id on a V1-UPGRADED board`, async () => { + expect([...(await resolve(storeFor(V1), "FN-1"))]).toEqual([legacy]); + }); + + it(`${name} falls back to the legacy id when the workflow cannot be resolved`, async () => { + expect([...(await resolve(storeFor(undefined), "FN-1"))]).toEqual([legacy]); + }); + } +}); diff --git a/packages/dashboard/src/task-lifecycle-lanes.ts b/packages/dashboard/src/task-lifecycle-lanes.ts index 2f9d5d8334..c190c7c7f9 100644 --- a/packages/dashboard/src/task-lifecycle-lanes.ts +++ b/packages/dashboard/src/task-lifecycle-lanes.ts @@ -1,4 +1,4 @@ -import { columnsWithFlag, resolveWorkflowIrForTask, type WorkflowIr } from "@fusion/core"; +import { columnsWithFlag, declaresAnyLifecycleTrait, resolveWorkflowIrForTask, type WorkflowIr } from "@fusion/core"; /* FNXC:WorkflowResolvedColumns 2026-07-30-08:45 (#2783 review — coderabbit): @@ -95,6 +95,20 @@ which a complete-but-not-archived card is not. The two roles resolve independently and have failed independently before, so they get independent helpers rather than one flag argument — a caller that wants both asks `landedColumnsForTask`. */ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-23:55 (batch-core, following #2821's review): +THE EMPTY SET AND THE UNEXPRESSED ONE ARE DIFFERENT, and these three conflated them. + +Each read `resolved.length > 0 ? resolved : legacyId`, which is right for a v1 upgrade — every column +emitted with `traits: []`, so the legacy id is the only vocabulary that exists — and wrong for a v2 +board that expresses traits and simply declares no lane of that role. There the legacy id is a column +the board may still HAVE without meaning it: a `done` or `archived` column left untraited on purpose. +Falling back onto it widens the guard onto a role the board explicitly did not assign. + +`declaresAnyLifecycleTrait` separates the two, matching the shape #2821's review established for +`resolveNodeOverrideLanes`. A board that traits nothing keeps the legacy id; a board that traits +something is taken at its word, including when the answer is "no such lane". +*/ export async function archivedColumnsForTask( store: LaneResolverStore, taskId: string, @@ -102,8 +116,8 @@ export async function archivedColumnsForTask( ): Promise> { try { const ir = await resolveWorkflowIrForTask(store, taskId, irCache); - const archived = columnsWithFlag(ir, "archived"); - return new Set(archived.length > 0 ? archived : ["archived"]); + if (!declaresAnyLifecycleTrait(ir)) return new Set(["archived"]); + return new Set(columnsWithFlag(ir, "archived")); } catch { return new Set(["archived"]); } @@ -122,8 +136,8 @@ export async function wipColumnsForTask( ): Promise> { try { const ir = await resolveWorkflowIrForTask(store, taskId, irCache); - const wip = columnsWithFlag(ir, "countsTowardWip"); - return new Set(wip.length > 0 ? wip : ["in-progress"]); + if (!declaresAnyLifecycleTrait(ir)) return new Set(["in-progress"]); + return new Set(columnsWithFlag(ir, "countsTowardWip")); } catch { return new Set(["in-progress"]); } @@ -142,8 +156,8 @@ export async function preWipColumnsForTask( ): Promise> { try { const ir = await resolveWorkflowIrForTask(store, taskId, irCache); - const preWip = [...columnsWithFlag(ir, "intake"), ...columnsWithFlag(ir, "hold")]; - return new Set(preWip.length > 0 ? preWip : ["todo"]); + if (!declaresAnyLifecycleTrait(ir)) return new Set(["todo"]); + return new Set([...columnsWithFlag(ir, "intake"), ...columnsWithFlag(ir, "hold")]); } catch { return new Set(["todo"]); }