diff --git a/.changeset/reliability-lane-metrics.md b/.changeset/reliability-lane-metrics.md new file mode 100644 index 0000000000..50b14b8fa9 --- /dev/null +++ b/.changeset/reliability-lane-metrics.md @@ -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. diff --git a/packages/dashboard/src/__tests__/reliability-metrics.test.ts b/packages/dashboard/src/__tests__/reliability-metrics.test.ts index 782068295e..e003a7a89a 100644 --- a/packages/dashboard/src/__tests__/reliability-metrics.test.ts +++ b/packages/dashboard/src/__tests__/reliability-metrics.test.ts @@ -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>) => ({ + 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({}); + }); +}); diff --git a/packages/dashboard/src/reliability-metrics.ts b/packages/dashboard/src/reliability-metrics.ts index c7e85aa69e..59382a68a3 100644 --- a/packages/dashboard/src/reliability-metrics.ts +++ b/packages/dashboard/src/reliability-metrics.ts @@ -237,3 +237,99 @@ export function inReviewFailureRate7d(enteredByDay: Record, 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>; +} + +function mergeDayCounts(into: Record, from: Record): Record { + 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, +): Promise> { + const results = await Promise.all( + [...toColumns].map((toColumn) => store.getTaskMovedCountsByDay({ ...window, toColumn })), + ); + return results.reduce>((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, +): Promise> { + const [into, within] = await Promise.all([ + countMovesInto(store, window, lanes), + lanes.size > 1 ? countBouncesOut(store, window, lanes, lanes) : Promise.resolve>({}), + ]); + 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, + toColumns: ReadonlySet, +): Promise> { + 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>((acc, counts) => mergeDayCounts(acc, counts), {}); +} diff --git a/packages/dashboard/src/server.ts b/packages/dashboard/src/server.ts index 8cd229fb6f..e89c20e813 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, 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 }), ]);