From 476c5c360cfa23bbd4a9e7ec8aaa9198acfca27f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 13:23:20 -0700 Subject: [PATCH] test(dashboard): pin the Reliability endpoint's three lane reads (extract seam + pin) (#3237) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Pins the **Reliability endpoint's three lane reads** — the last uncovered resolver cluster the repo-wide audit found. Two commits, deliberately separate: 1. **refactor** — extract the three resolves behind `resolveReliabilityLanes(store)`. Behaviour-preserving, no test changes. 2. **test** — pin all three through that seam. ## Why a seam was needed The three resolves lived inline in the `/api/health/reliability` route closure. Blinding any of them left the **entire dashboard suite green — 21,582 tests** — and the only way to reach them was booting `createServer` behind a mock-the-world shell the slow-test rule forbids. **And the obvious test would not have helped.** `reliability-metrics.test.ts` exercises `countEntriesInto`, `countBouncesOut` and `inReviewDurationMetrics` with lane sets **passed in by hand**. That proves the collaborators honour a resolved set; it says nothing about whether the caller passes one. *A unit test of the collaborator can never fail when the caller's resolve is blinded* — the same trap the audit note records for `reads.ts`, where a suite written for the exact conversion still could not see it. The seam is the caller. It resolves, so blinding a resolve fails a test of it. ## Measured — each blind fails exactly its own case | blinded | fails | |---|---| | `REVIEW_ROLES` | "resolves the board's OWN review lane" | | `["countsTowardWip"]` | "resolves the board's OWN wip lane" | | `["complete"]` | "resolves the board's OWN complete lane" | ``` converted: Tests 6 passed (6) each blind: Tests 1 failed (its own case only) reliability-metrics.test.ts + this file: 28 passed typecheck clean; lint clean; fnxc-future-dates: none added ``` That isolation is the point: **three resolves in one function invite a copy-paste that hands the same set to all three**, and every positive assertion would still pass. There is a paired negative asserting each renamed lane appears in *its* bucket and nowhere else — without it the duration metric could silently measure review → review. Also pinned: the degrade path. An unreadable workflow list must not fail the endpoint, so the legacy ids still answer. ## What breaks without the conversion On a board that renames either lane, every underlying query returns `{}` — so `tasksEnteredInReview` and `tasksBouncedToInProgress` are zero for every day, and `inReviewFailureRate7d` divides one zero by another and reports a **healthy** rate. It produces a NUMBER, not an error, and the number says everything is fine. An operator reading 0% review failures beside a populated audit list has no reason to suspect the metric is blind. ## The one observable difference in the refactor, stated not buried The complete-lane read moves from *after* the counting `Promise.all` into the same phase as the review/wip pair. These are pure reads of workflow definitions — no writes, no ordering dependency — so the resolved values are identical; only the concurrency shape changes (three parallel reads instead of two-then-one). Flagging it because "behaviour-preserving" should be a claim someone can check, not an assertion. ## Audit status With this, **3 of the 4 flagged sites are closed**. Remaining: `cli/commands/task.ts:660`, where the glyph decision is inline in `runTaskList` and the same seam argument applies — but its sibling test file already documents that driving that function needs the forbidden shell, and extracting a helper there would produce a test that *looks* like coverage while leaving the resolve unpinned. Left flagged rather than faked. ## Summary by CodeRabbit - **New Features** - Reliability health metrics now recognize configured review, work-in-progress, and completion lanes, including renamed workflow lanes. - **Bug Fixes** - Improved fallback behavior when workflow definitions are unavailable, preserving compatibility with legacy lane configurations. - Ensured lane resolution remains isolated by role for more accurate reliability metrics. --- .../reliability-lanes-resolved.test.ts | 112 ++++++++++++++++++ packages/dashboard/src/reliability-metrics.ts | 31 +++++ packages/dashboard/src/server.ts | 12 +- 3 files changed, 147 insertions(+), 8 deletions(-) create mode 100644 packages/dashboard/src/__tests__/reliability-lanes-resolved.test.ts diff --git a/packages/dashboard/src/__tests__/reliability-lanes-resolved.test.ts b/packages/dashboard/src/__tests__/reliability-lanes-resolved.test.ts new file mode 100644 index 0000000000..e68ada7d90 --- /dev/null +++ b/packages/dashboard/src/__tests__/reliability-lanes-resolved.test.ts @@ -0,0 +1,112 @@ +// @vitest-environment node + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-23:58: +THE RELIABILITY ENDPOINT'S THREE LANE READS, on a RENAMED board. + +`/api/health/reliability` asks three lane questions: which lanes are REVIEW, which count toward WIP, +and which are COMPLETE. They feed the entered/bounced counts and the review -> done duration metric. + +WHY THIS FILE EXISTS. All three were UNCOVERED. Blinding any of them left the entire dashboard suite +green — 21,582 tests — because they sat inline in the route closure and nothing drives that route. + +AND WHY THE OBVIOUS TEST WOULD NOT HAVE HELPED. `reliability-metrics.test.ts` exercises +`countEntriesInto`, `countBouncesOut` and `inReviewDurationMetrics` with lane sets passed in BY HAND. +That proves the collaborators honour a resolved set; it says nothing about whether the caller passes +one. A unit test of the collaborator can never fail when the caller's resolve is blinded. The seam +under test here is the CALLER: `resolveReliabilityLanes` resolves, so blinding a resolve fails it. + +WHAT BREAKS WITHOUT THE CONVERSION. On a board that renames either lane, every underlying query +returns {} — so `tasksEnteredInReview` and `tasksBouncedToInProgress` are zero for every day, and +`inReviewFailureRate7d` divides one zero by another and reports a healthy rate. That is the worst +shape a lifecycle defect takes: it produces a NUMBER, not an error, and the number says everything is +fine. An operator reading 0% review failures beside a populated audit list has no reason to suspect +the metric is blind. + +DIFFERENTIAL. The same workflow SHAPE under two vocabularies with identical traits; only the ids +differ, and no renamed id collides with a legacy one. The default-vocabulary cases are controls. +*/ + +import { describe, expect, it, vi } from "vitest"; +import { resolveReliabilityLanes } from "../reliability-metrics.js"; + +const RENAMED = { review: "signoff", wip: "building", complete: "shipped" }; + +function ir(names: { review: string; wip: string; complete: string }) { + return { + version: "v2", + id: "custom:renamed-reliability", + nodes: [], + edges: [], + columns: [ + { id: "todo", label: "Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: names.wip, label: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: names.review, label: "Review", traits: [{ trait: "merge" }, { trait: "human-review" }] }, + { id: names.complete, label: "Complete", traits: [{ trait: "complete" }] }, + ], + }; +} + +/** A store that can answer differently from the legacy floor — i.e. one with workflow definitions. */ +function storeWith(names: { review: string; wip: string; complete: string }) { + return { + listWorkflowDefinitions: vi.fn(async () => [{ ir: ir(names) }]), + getWorkflowDefinition: vi.fn(async () => ({ ir: ir(names) })), + } as unknown as Parameters[0]; +} + +describe("resolveReliabilityLanes", () => { + it("default vocabulary: resolves the built-in review, wip and complete lanes", async () => { + const lanes = await resolveReliabilityLanes( + storeWith({ review: "in-review", wip: "in-progress", complete: "done" }), + ); + + expect([...lanes.review]).toContain("in-review"); + expect([...lanes.wip]).toContain("in-progress"); + expect([...lanes.complete]).toContain("done"); + }); + + it("renamed vocabulary: resolves the board's OWN review lane", async () => { + const lanes = await resolveReliabilityLanes(storeWith(RENAMED)); + expect([...lanes.review]).toContain(RENAMED.review); + }); + + it("renamed vocabulary: resolves the board's OWN wip lane", async () => { + const lanes = await resolveReliabilityLanes(storeWith(RENAMED)); + expect([...lanes.wip]).toContain(RENAMED.wip); + }); + + it("renamed vocabulary: resolves the board's OWN complete lane", async () => { + const lanes = await resolveReliabilityLanes(storeWith(RENAMED)); + expect([...lanes.complete]).toContain(RENAMED.complete); + }); + + it("keeps the three roles distinct — a renamed lane does not leak across buckets", async () => { + /* + The paired negative. Three resolves in one function invite a copy-paste that hands the same set + to all three; every positive above would still pass, and the duration metric would then measure + review -> review. Each renamed lane must appear in ITS bucket and nowhere else. + */ + const lanes = await resolveReliabilityLanes(storeWith(RENAMED)); + + expect([...lanes.review]).not.toContain(RENAMED.wip); + expect([...lanes.review]).not.toContain(RENAMED.complete); + expect([...lanes.wip]).not.toContain(RENAMED.review); + expect([...lanes.wip]).not.toContain(RENAMED.complete); + expect([...lanes.complete]).not.toContain(RENAMED.review); + expect([...lanes.complete]).not.toContain(RENAMED.wip); + }); + + it("degrades to the legacy floor when the board cannot be read", async () => { + /* An unreadable workflow list must not fail the endpoint; the legacy ids still answer. */ + const lanes = await resolveReliabilityLanes({ + listWorkflowDefinitions: vi.fn(async () => { + throw new Error("unreadable"); + }), + } as unknown as Parameters[0]); + + expect([...lanes.review]).toContain("in-review"); + expect([...lanes.wip]).toContain("in-progress"); + expect([...lanes.complete]).toContain("done"); + }); +}); diff --git a/packages/dashboard/src/reliability-metrics.ts b/packages/dashboard/src/reliability-metrics.ts index 832c916fb2..52c2288a8d 100644 --- a/packages/dashboard/src/reliability-metrics.ts +++ b/packages/dashboard/src/reliability-metrics.ts @@ -1,4 +1,5 @@ import type { ActivityLogEntry, RunAuditEvent } from "@fusion/core"; +import { resolveProjectColumnsForRoles, REVIEW_ROLES } from "@fusion/core"; /** * Discovery notes (FN-4360): @@ -370,3 +371,33 @@ export async function countBouncesOut( ); return results.reduce>((acc, counts) => mergeDayCounts(acc, counts), {}); } + +/** + * FNXC:WorkflowResolvedColumns 2026-07-31-23:55: + * The Reliability endpoint's three lane reads, behind one seam. + * + * WHY THIS EXISTS. These resolves lived inline in the `/api/health/reliability` route closure, which + * has no route-level test — the only way in was booting `createServer` behind a mock-the-world + * shell, which the slow-test rule forbids. So all three were UNCOVERED and unpinnable: blinding any + * of them left the whole dashboard suite green. `reliability-metrics.test.ts` looks like it covers + * them and does not — it exercises the collaborators (`countEntriesInto` and friends) with lane sets + * passed in by hand, which can never fail when the CALLER's resolve is blinded. + * + * This function is that missing caller-side seam: it resolves, so a test of it fails when a resolve + * is blinded. + * + * WHAT THE LANES MEAN. `review` and `wip` are the two sides of the entered/bounced counts; + * `complete` is the other half of the review -> done transition the duration metric measures. + * Keyed on literals, a renamed board returned {} from every query, so the headline divided one zero + * by another and reported a healthy rate — a NUMBER, not an error, saying everything is fine. + */ +export async function resolveReliabilityLanes( + store: Parameters[0], +): Promise<{ review: ReadonlySet; wip: ReadonlySet; complete: ReadonlySet }> { + const [review, wip, complete] = await Promise.all([ + resolveProjectColumnsForRoles(store, REVIEW_ROLES), + resolveProjectColumnsForRoles(store, ["countsTowardWip"]), + resolveProjectColumnsForRoles(store, ["complete"]), + ]); + return { review, wip, complete }; +} diff --git a/packages/dashboard/src/server.ts b/packages/dashboard/src/server.ts index 45b30bdd2d..39c135becc 100644 --- a/packages/dashboard/src/server.ts +++ b/packages/dashboard/src/server.ts @@ -20,7 +20,7 @@ import type { AgentLogEntry, RunAuditEvent, } from "@fusion/core"; -import { AgentStore, ChatStore, queryRunAuditEvents, resolveGlobalDir, resolveProjectColumnsForRoles, resolveReboundTargetForTask, REVIEW_ROLES, setRunningAgentCountSource } from "@fusion/core"; +import { AgentStore, ChatStore, queryRunAuditEvents, resolveGlobalDir, resolveReboundTargetForTask, setRunningAgentCountSource } from "@fusion/core"; import type { AuthStorageLike, ModelRegistryLike } from "./routes.js"; import { createApiRoutes } from "./routes.js"; import { createSSE, disconnectSSEClient, markSSEClientAlive } from "./sse.js"; @@ -88,6 +88,7 @@ import { recoverAlreadyMergedReviewTasksRecoveriesPerDay, countEntriesInto, countBouncesOut, + resolveReliabilityLanes, } from "./reliability-metrics.js"; import { loadViewChunkManifest, type ViewChunkManifestEntry } from "./view-chunk-manifest.js"; import { maybeStartOtelExporter, type OtelExporterHandle } from "./otel-exporter.js"; @@ -1918,10 +1919,8 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT summing across disjoint pairs cannot double-count. On the built-in board this is 1x1 — exactly the two queries that were here before — and on a renamed board it is a handful. */ - const [reviewLanes, wipLanes] = await Promise.all([ - resolveProjectColumnsForRoles(scopedStore, REVIEW_ROLES), - resolveProjectColumnsForRoles(scopedStore, ["countsTowardWip"]), - ]); + const { review: reviewLanes, wip: wipLanes, complete: durationCompleteLanes } = + await resolveReliabilityLanes(scopedStore); const [runAuditEvents, enteredByDay, bouncedByDay, durationEvents, mergedTaskIds] = await Promise.all([ runAuditEventsPromise, countEntriesInto(scopedStore, { since: startIso, until: endIso }, reviewLanes), @@ -1933,9 +1932,6 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT const postMergeByDay = postMergeAuditFailuresPerDay(runAuditEvents, effectiveStartMs, nowMs); const fileScopeByDay = fileScopeInvariantFailuresPerDay(runAuditEvents, effectiveStartMs, nowMs); const recoveriesByDay = recoverAlreadyMergedReviewTasksRecoveriesPerDay(runAuditEvents, effectiveStartMs, nowMs); - /* `reviewLanes` is already resolved above for the entry/bounce counts; the complete lanes are the - other half of the review -> done transition this metric measures. */ - const durationCompleteLanes = await resolveProjectColumnsForRoles(scopedStore, ["complete"]); const duration = inReviewDurationMetrics(durationEvents, effectiveStartMs, nowMs, { review: reviewLanes, complete: durationCompleteLanes,