root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(reliability): the review-failure headline read 0% because both inputs were zero (#2861)

Browse Source 
The Reliability panel's headline metric is computed from two queries
that name lanes:

```ts
scopedStore.getTaskMovedCountsByDay({ …, toColumn: "in-review" }),
scopedStore.getTaskMovedCountsByDay({ …, fromColumn: "in-review", toColumn: "in-progress" }),
…
const headline = inReviewFailureRate7d(enteredByDay, bouncedByDay, nowMs);
```

On a board that renamed either lane, **both return `{}`**. Every per-day
count is zero, and the headline then divides one zero by another and
reports a healthy rate.

**This is the worst shape a lifecycle defect takes.** It produces a
*number*, not an error, and the number is reassuring. An operator
reading a 0% review-failure rate beside a populated audit event list has
no reason to suspect the metric is blind — the same failure mode as the
analytics `tasksInProgress: 0` sitting next to correct cost totals.

## Why a union is correct here, not a compromise

`getTaskMovedCountsByDay` takes **one** column per side, so the lanes
are resolved to sets and the query is issued per `(from, to)` pair and
summed.

The important part is *why* summing over a union is the right answer
rather than a widening hack. These read **move history**, and a past
move recorded the column name as it was at the time — the same reasoning
that keeps `tasksEnteredInReviewPerDay` in this module matching recorded
values verbatim, marked DELIBERATE-LITERAL. A board renamed last month
therefore has old rows under the old id and new rows under the new one,
so the correct query covers **both**. That is exactly the set
`resolveProjectColumnsForRoles` returns, with the legacy id always
unioned in.

Asking for either name alone is the bug — not a choice between them.

No double-counting: a move event has exactly one `(from, to)` pair, so
the queries partition the events rather than overlapping.

**The common path does not get more expensive.** On the built-in board
this issues the same two queries as before, and there is a test
asserting the call count so a future change cannot quietly turn one
query into N.

## Revert proof (measured)

Collapse the sets back to the single literals:

```
AssertionError: expected { '2026-07-01': 1 } to deeply equal { '2026-07-01': 2, '2026-07-02': 3 }
AssertionError: expected { '2026-07-01': 1 } to deeply equal { '2026-07-01': 2, '2026-07-03': 5 }
```

The helpers live in `reliability-metrics.ts` rather than inline in
`server.ts` specifically so they are testable without booting an express
app.

## Verification

- `pnpm test:gate` — 161 / 487 / 13 / 71 passed
- `pnpm smoke:boot` — PASS (`fn --help`, real `serve` `/api/health` 200,
clean shutdown)
- `pnpm lint` — clean
- `tsc --noEmit` (`@fusion/dashboard`) — clean
- `reliability-metrics.test.ts` — 15 passed
- census `--strict` — exit 0 (unchanged: query filters are not
comparisons)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-30 15:57:51 -07:00
committed by GitHub
parent 5792452f0a
commit 9e1deffa72
4 changed files with 249 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.changeset/reliability-lane-metrics.md Normal file
View File

@@ -0,0 +1,7 @@
---
"@runfusion/fusion": patch
---
summary: The Reliability health panel no longer reports a perfect review-failure rate on a renamed board.
category: fix
dev: `/api/health/reliability` counted review entries and bounces with two `getTaskMovedCountsByDay` queries naming `in-review` and `in-progress`. On a renamed board both returned `{}`, so every per-day count was zero and `inReviewFailureRate7d` divided one zero by another and reported healthy. The lanes are now resolved via `resolveProjectColumnsForRoles` and the query is issued per (from, to) pair and summed.

123
packages/dashboard/src/__tests__/reliability-metrics.test.ts
View File

@@ -1,9 +1,12 @@
import { describe, expect, it } from "vitest";
import { describe, expect, it, vi } from "vitest";
import type { ActivityLogEntry, RunAuditEvent } from "@fusion/core";
import {
bucketByDay,
countBouncesOut,
countEntriesInto,
countMovesInto,
dayHasSamples,
fileScopeInvariantFailuresPerDay,
inReviewDurationMetrics,
@@ -183,3 +186,121 @@ describe("reliability-metrics", () => {
})).toBe(true);
});
});
/*
FNXC:WorkflowLifecycleColumns 2026-07-30-17:05:
THE INVARIANT: the Reliability move counts cover every lane that carries the role, old name and new.
The route issued two queries naming `in-review` and `in-progress`. On a board that renamed either,
both return `{}`, every per-day count is zero, and `inReviewFailureRate7d` divides one zero by
another and reports a healthy rate. A metric that answers with a REASSURING NUMBER instead of an
error is the worst shape this class takes — an operator has no reason to suspect it is blind.
The union covers history as well as the present, which matters because these read MOVE RECORDS: a
board renamed last month has old rows under the old id and new rows under the new one. Asking for
either name alone is what is broken today, not a trade-off between them.
REVERT PROOF, measured: replace the sets with the single literals and the renamed-lane cases fail
with `expected {} to deeply equal { '2026-07-01': 2 }`.
*/
describe("reliability move counts span every lane carrying the role", () => {
const store = (rows: Record<string, Record<string, number>>) => ({
getTaskMovedCountsByDay: vi.fn(async (o: { fromColumn?: string; toColumn?: string }) =>
rows[`${o.fromColumn ?? ""}->${o.toColumn ?? ""}`] ?? {}),
});
const WINDOW = { since: "2026-07-01T00:00:00.000Z", until: "2026-07-08T00:00:00.000Z" };
it("sums entries across a renamed review lane and the legacy one", async () => {
// A board mid-rename: old move rows under `in-review`, new ones under `signoff`.
const counts = await countMovesInto(
store({ "->in-review": { "2026-07-01": 1 }, "->signoff": { "2026-07-01": 1, "2026-07-02": 3 } }) as never,
WINDOW,
new Set(["in-review", "signoff"]),
);
expect(counts).toEqual({ "2026-07-01": 2, "2026-07-02": 3 });
});
it("sums bounces across every (review, wip) pair without double-counting", async () => {
// Each move event has exactly one (from, to) pair, so the queries partition rather than overlap.
const counts = await countBouncesOut(
store({
"in-review->in-progress": { "2026-07-01": 1 },
"signoff->building": { "2026-07-01": 1 },
"signoff->in-progress": { "2026-07-03": 5 },
}) as never,
WINDOW,
new Set(["in-review", "signoff"]),
new Set(["in-progress", "building"]),
);
expect(counts).toEqual({ "2026-07-01": 2, "2026-07-03": 5 });
});
/*
FNXC:WorkflowLifecycleColumns 2026-07-30-18:20 (#2861 review — greptile P1):
A MOVE BETWEEN TWO REVIEW LANES IS NOT AN ENTRY INTO REVIEW.
A defect the single-lane version could not have. A board with `signoff` and `waiting` both carrying
review roles has moves between them, and counting by destination alone scores `signoff -> waiting`
as another entry — inflating the denominator while the bounce count is unchanged, so the headline
UNDERSTATES the review-failure rate. Wrong in the reassuring direction, which is the failure mode
this whole change is about; generalising a one-lane query to a set introduced a question one lane
never had to answer.
REVERT PROOF, measured: drop the subtraction and this fails with
`expected { '2026-07-01': 3 } to deeply equal { '2026-07-01': 2 }`.
*/
it("does not count a move BETWEEN two review lanes as an entry into review", async () => {
const counts = await countEntriesInto(
store({
"->signoff": { "2026-07-01": 2 },
"->waiting": { "2026-07-01": 1 },
/* One of those was `signoff -> waiting`: already in review, not a new entry. */
"signoff->waiting": { "2026-07-01": 1 },
}) as never,
WINDOW,
new Set(["signoff", "waiting"]),
);
expect(counts).toEqual({ "2026-07-01": 2 });
});
it("drops a day entirely when every move into the set was internal", async () => {
// Guards the subtraction's edge: 0 must not be reported as a day with zero entries, and a
// negative must never surface.
const counts = await countEntriesInto(
store({ "->waiting": { "2026-07-01": 1 }, "signoff->waiting": { "2026-07-01": 1 } }) as never,
WINDOW,
new Set(["signoff", "waiting"]),
);
expect(counts).toEqual({});
});
it("skips the intra-set subtraction for a single-lane board", async () => {
// A move requires the column to change, so a one-lane set has no internal moves to remove and
// must not pay for a query asking about them.
const single = store({ "->in-review": { "2026-07-01": 4 } });
expect(await countEntriesInto(single as never, WINDOW, new Set(["in-review"]))).toEqual({ "2026-07-01": 4 });
expect(single.getTaskMovedCountsByDay).toHaveBeenCalledTimes(1);
});
it("issues exactly the two legacy queries on the built-in board", async () => {
// The common path must not get more expensive to fix the uncommon one.
const entered = store({ "->in-review": { "2026-07-01": 4 } });
const bounced = store({ "in-review->in-progress": { "2026-07-01": 1 } });
expect(await countMovesInto(entered as never, WINDOW, new Set(["in-review"]))).toEqual({ "2026-07-01": 4 });
expect(await countBouncesOut(bounced as never, WINDOW, new Set(["in-review"]), new Set(["in-progress"]))).toEqual({ "2026-07-01": 1 });
expect(entered.getTaskMovedCountsByDay).toHaveBeenCalledTimes(1);
expect(bounced.getTaskMovedCountsByDay).toHaveBeenCalledTimes(1);
});
it("returns an empty map rather than throwing when no lane is supplied", async () => {
expect(await countMovesInto(store({}) as never, WINDOW, new Set())).toEqual({});
});
});

96
packages/dashboard/src/reliability-metrics.ts
View File

@@ -237,3 +237,99 @@ export function inReviewFailureRate7d(enteredByDay: Record<string, number>, boun
return { value: bounced / entered };
}
/*
FNXC:WorkflowLifecycleColumns 2026-07-30-17:05:
Per-day move counts across a SET of lanes, because `getTaskMovedCountsByDay` takes one column a side.
The Reliability headline was built from two queries naming `in-review` and `in-progress`. On a board
that renamed either, both return `{}` — so `tasksEnteredInReview` and `tasksBouncedToInProgress` are
zero for every day and `inReviewFailureRate7d` divides one zero by another and reports healthy. That
is the worst shape this class takes: it produces a NUMBER, not an error, and the number is reassuring.
WHY A UNION IS RIGHT HERE AND NOT A COMPROMISE. These read MOVE HISTORY, and a past move recorded the
column name as it was at the time — the same reasoning that keeps `tasksEnteredInReviewPerDay` above
matching recorded values verbatim. A board renamed last month therefore has old rows under the old id
and new rows under the new one, so the correct query covers BOTH. `resolveProjectColumnsForRoles`
always unions the legacy id in, which is exactly that set. Asking for either name alone is what is
broken today.
Summing across pairs cannot double-count: a move event has exactly one (from, to) pair, so the
queries partition the events rather than overlapping. On the built-in board this issues the same two
queries as before.
*/
export interface MovedCountsStore {
getTaskMovedCountsByDay(options: { since: string; until: string; fromColumn?: string; toColumn?: string }): Promise<Record<string, number>>;
}
function mergeDayCounts(into: Record<string, number>, from: Record<string, number>): Record<string, number> {
for (const [day, count] of Object.entries(from)) into[day] = (into[day] ?? 0) + count;
return into;
}
/*
FNXC:WorkflowLifecycleColumns 2026-07-30-18:20 (#2861 review — greptile P1 and P2, both right):
ENTRIES INTO THE SET, not moves into its members; and the reads run concurrently.
P1, and it is a defect the single-lane version could not have: a board with two review-role lanes
(`signoff` and `waiting`, say) has moves BETWEEN them, and counting only by destination scores
`signoff -> waiting` as another entry into review. That inflates the denominator while the bounce
count is unchanged, so the headline UNDERSTATES the review-failure rate — a metric that is wrong in
the reassuring direction, which is the same failure this whole change set is about. Generalising a
one-lane query to a set introduced a question one lane never had to answer.
The subtraction is skipped when there is only one lane, because a move requires the column to change
so there are no intra-set moves to remove. That keeps the built-in board at exactly the queries it
had before rather than paying for a case it cannot have.
P2: the per-pair reads are independent, so they run under `Promise.all` rather than sequentially.
Latency is now one round trip deep instead of N + NxM.
*/
/** Moves into any of `toColumns`, summed per day. */
export async function countMovesInto(
store: MovedCountsStore,
window: { since: string; until: string },
toColumns: ReadonlySet<string>,
): Promise<Record<string, number>> {
const results = await Promise.all(
[...toColumns].map((toColumn) => store.getTaskMovedCountsByDay({ ...window, toColumn })),
);
return results.reduce<Record<string, number>>((acc, counts) => mergeDayCounts(acc, counts), {});
}
/**
* Moves into the lane SET from outside it — the "entered review" shape.
*
* Subtracts moves BETWEEN members, which are not entries. No-op for a single-lane set.
*/
export async function countEntriesInto(
store: MovedCountsStore,
window: { since: string; until: string },
lanes: ReadonlySet<string>,
): Promise<Record<string, number>> {
const [into, within] = await Promise.all([
countMovesInto(store, window, lanes),
lanes.size > 1 ? countBouncesOut(store, window, lanes, lanes) : Promise.resolve<Record<string, number>>({}),
]);
for (const [day, count] of Object.entries(within)) {
const remaining = (into[day] ?? 0) - count;
if (remaining > 0) into[day] = remaining;
else delete into[day];
}
return into;
}
/** Moves OUT of any `fromColumns` into any `toColumns` — the review-bounce shape — summed per day. */
export async function countBouncesOut(
store: MovedCountsStore,
window: { since: string; until: string },
fromColumns: ReadonlySet<string>,
toColumns: ReadonlySet<string>,
): Promise<Record<string, number>> {
const pairs = [...fromColumns].flatMap((fromColumn) => [...toColumns].map((toColumn) => ({ fromColumn, toColumn })));
const results = await Promise.all(
pairs.map((pair) => store.getTaskMovedCountsByDay({ ...window, ...pair })),
);
return results.reduce<Record<string, number>>((acc, counts) => mergeDayCounts(acc, counts), {});
}

27
packages/dashboard/src/server.ts
View File

@@ -20,7 +20,7 @@ import type {
AgentLogEntry,
RunAuditEvent,
} from "@fusion/core";
import { AgentStore, ChatStore, queryRunAuditEvents, resolveGlobalDir, setRunningAgentCountSource } from "@fusion/core";
import { AgentStore, ChatStore, queryRunAuditEvents, resolveGlobalDir, resolveProjectColumnsForRoles, REVIEW_ROLES, setRunningAgentCountSource } from "@fusion/core";
import type { AuthStorageLike, ModelRegistryLike } from "./routes.js";
import { createApiRoutes } from "./routes.js";
import { createSSE, disconnectSSEClient, markSSEClientAlive } from "./sse.js";
@@ -86,6 +86,8 @@ import {
mergeAttemptsPerMergedTask,
postMergeAuditFailuresPerDay,
recoverAlreadyMergedReviewTasksRecoveriesPerDay,
countEntriesInto,
countBouncesOut,
} from "./reliability-metrics.js";
import { loadViewChunkManifest, type ViewChunkManifestEntry } from "./view-chunk-manifest.js";
import { maybeStartOtelExporter, type OtelExporterHandle } from "./otel-exporter.js";
@@ -1884,10 +1886,29 @@ export function createServer(store: TaskStore, options?: ServerOptions): ReturnT
metadata: event.metadata ?? undefined,
})))
: scopedStore.getRunAuditEventsAsync(auditFilter);
/*
FNXC:WorkflowLifecycleColumns 2026-07-30-17:05:
The Reliability headline was computed from two queries that name `in-review` and `in-progress`.
On a board that renamed either lane both return {}, so `tasksEnteredInReview` and
`tasksBouncedToInProgress` are zero for every day — and `inReviewFailureRate7d` then 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 a 0%
review-failure rate beside a populated audit list has no reason to suspect the metric is blind.
`getTaskMovedCountsByDay` takes ONE column per side, so the lanes are resolved to sets and the
query is issued per pair, then summed. Move events are keyed on a single (from, to) pair, so
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 [runAuditEvents, enteredByDay, bouncedByDay, durationEvents, mergedTaskIds] = await Promise.all([
runAuditEventsPromise,
scopedStore.getTaskMovedCountsByDay({ since: startIso, until: endIso, toColumn: "in-review" }),
scopedStore.getTaskMovedCountsByDay({ since: startIso, until: endIso, fromColumn: "in-review", toColumn: "in-progress" }),
countEntriesInto(scopedStore, { since: startIso, until: endIso }, reviewLanes),
countBouncesOut(scopedStore, { since: startIso, until: endIso }, reviewLanes, wipLanes),
scopedStore.getInReviewDurationEvents({ since: startIso, until: endIso }),
scopedStore.getTaskMergedTaskIds({ since: startIso, until: endIso }),
]);