test(dashboard): pin the Reliability endpoint's three lane reads (extract seam + pin) (#3237)

## 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.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## 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.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
gsxdsm
2026-07-31 13:23:20 -07:00
committed by GitHub
parent f0a13745b2
commit 476c5c360c
3 changed files with 147 additions and 8 deletions

View File

@@ -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<typeof resolveReliabilityLanes>[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<typeof resolveReliabilityLanes>[0]);
expect([...lanes.review]).toContain("in-review");
expect([...lanes.wip]).toContain("in-progress");
expect([...lanes.complete]).toContain("done");
});
});

View File

@@ -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<Record<string, number>>((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<typeof resolveProjectColumnsForRoles>[0],
): Promise<{ review: ReadonlySet<string>; wip: ReadonlySet<string>; complete: ReadonlySet<string> }> {
const [review, wip, complete] = await Promise.all([
resolveProjectColumnsForRoles(store, REVIEW_ROLES),
resolveProjectColumnsForRoles(store, ["countsTowardWip"]),
resolveProjectColumnsForRoles(store, ["complete"]),
]);
return { review, wip, complete };
}

View File

@@ -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,