diff --git a/.changeset/reliability-duration-lanes.md b/.changeset/reliability-duration-lanes.md new file mode 100644 index 0000000000..bd91305322 --- /dev/null +++ b/.changeset/reliability-duration-lanes.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: The Reliability panel's in-review duration metric now works on a board with renamed lanes. +category: fix +dev: `getInReviewDurationEvents` had `in-review` and `done` baked into a raw `sql` predicate — invisible to both the lifecycle census and the unwired-lane-parameter guard — so it stayed blind after #2861 fixed the panel's other two inputs. The lanes are now resolved once per call via `resolveProjectColumnsForRoles` and passed in as parameterised equality fragments, defaulting to the legacy ids. diff --git a/packages/core/src/__tests__/postgres/activity-log-parity.pg.test.ts b/packages/core/src/__tests__/postgres/activity-log-parity.pg.test.ts index 087e1e0352..f656b71806 100644 --- a/packages/core/src/__tests__/postgres/activity-log-parity.pg.test.ts +++ b/packages/core/src/__tests__/postgres/activity-log-parity.pg.test.ts @@ -194,4 +194,88 @@ pgDescribe("activity log parity (PostgreSQL)", () => { expect(durationEvents.map((event) => event.id)).toEqual(["reliability-entered", "reliability-done"]); expect(await store.getTaskMergedTaskIds(window)).toEqual(new Set(["FN-REL-1"])); }); + + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-22:15: + THE INVARIANT: the duration query reads the board's OWN review and complete lanes. + + The predicate had `in-review` and `done` baked into a `sql` template — the one place neither the + lifecycle census (which scans comparisons) nor the unwired-lane-parameter guard (which scans + declarations) can see them. So this was the Reliability panel's LAST blind input after #2861 fixed + the two counts beside it, and the panel read as partially healthy: entries and bounces populated, + duration reporting `no-in-review-entries` forever. Partial blindness is harder to spot than total. + + A REAL PostgreSQL row set on purpose. This is a SQL predicate change; a mocked store would assert + the arguments and prove nothing about the query that actually runs, which is the whole risk when + the literal lives inside `sql`. + + The legacy-lane case above stays green in the same file, which is the compatibility half: the union + covers move records written under the OLD id as well as the new one, and a past move recorded the + name as it was at the time. + + REVERT PROOF, measured: restore the hardcoded `= 'in-review'` / `= 'done'` fragments and this fails + with `expected [] to deeply equal [ 'renamed-entered', 'renamed-done' ]`. + */ + it("finds duration events on a board whose review and complete lanes are renamed", async () => { + const store = h.store(); + /* Same derivation as the rest of this file: the tasks/activity tables partition on the + `__legacy_unscoped__` sentinel when the layer carries no project id, so asserting `!` here + would write `projectId: undefined` on an unscoped harness and the query would match nothing. + I hit exactly that in #2886 and fixed it there; this one happened to work, which is worse. */ + const projectId = h.layer().projectId ?? "__legacy_unscoped__"; + await store.createWorkflowDefinition({ + name: "Renamed review and complete", + ir: { + version: "v2", + name: "Renamed review and complete", + columns: [ + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "signoff", name: "Sign-off", traits: [{ trait: "merge" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [ + { id: "start", kind: "start", column: "building" }, + { id: "end", kind: "end", column: "shipped" }, + ], + edges: [{ from: "start", to: "end", condition: "success" }], + } as never, + }); + + await h.adminDb().insert(schema.project.activityLog).values([ + { + projectId, + id: "renamed-entered", + timestamp: "2026-07-15T20:01:00.000Z", + type: "task:moved", + taskId: "FN-REN-1", + details: "Entered sign-off", + metadata: { from: "building", to: "signoff" }, + }, + { + projectId, + id: "renamed-done", + timestamp: "2026-07-15T20:02:00.000Z", + type: "task:moved", + taskId: "FN-REN-1", + details: "Shipped", + metadata: { from: "signoff", to: "shipped" }, + }, + { + projectId, + id: "renamed-unrelated", + timestamp: "2026-07-15T20:03:00.000Z", + type: "task:moved", + taskId: "FN-REN-1", + details: "Not a review transition", + metadata: { from: "building", to: "building" }, + }, + ]); + + const events = await store.getInReviewDurationEvents({ + since: "2026-07-15T20:00:00.000Z", + until: "2026-07-15T20:05:00.000Z", + }); + + expect(events.map((event) => event.id)).toEqual(["renamed-entered", "renamed-done"]); + }); }); diff --git a/packages/core/src/task-store/async-audit.ts b/packages/core/src/task-store/async-audit.ts index 206e460c94..58f99552ed 100644 --- a/packages/core/src/task-store/async-audit.ts +++ b/packages/core/src/task-store/async-audit.ts @@ -334,11 +334,55 @@ export async function getTaskMovedCountsByDay( FNXC:ReliabilityHealth 2026-07-14-16:13: Reliability metrics must query PostgreSQL activity rows through the async data layer. Keep the bounded duration-event shape and project scope used by the dashboard without falling through to the unavailable SQLite TaskStore database. */ +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-22:15: +The lane ids were baked into the SQL, which is the one place nothing could see them. + +This is the Reliability panel's third input. #2861 converted the other two — they were call arguments +— and this one stayed blind, so the panel went from uniformly wrong to partially wrong: the entry and +bounce counts started working while `inReviewDurationMetrics` kept reporting `no-in-review-entries`. +Partial blindness is harder to notice than total, which is why it is worth finishing rather than +leaving as a known gap. + +INVISIBLE TO EVERY CHECK WE HAVE, and that is the general lesson: the lifecycle census scans +`===`/`!==` comparisons and the unwired-lane-parameter guard scans declarations, so a lane id inside a +`sql` template is in neither total. The backlog number is a floor for this reason as well as the +usual one. (`scripts/check-sql-column-literals.mjs`, in flight, is the detector for this class; it +freezes the surface rather than converting it.) + +The predicate is built from the caller's resolved lanes as parameterised equality fragments rather +than an interpolated list — same shape, one branch per id, no string building. Lanes default to the +legacy pair so a caller that cannot resolve keeps exactly today's query. + +WHY A UNION IS CORRECT HERE, not a widening hack: these are MOVE RECORDS, and a past move recorded the +column name as it was at the time. A board renamed last month has old rows under the old id and new +rows under the new one, so the honest query covers both — which is precisely what +`resolveProjectColumnsForRoles` returns, legacy id always unioned in. +*/ +const LEGACY_REVIEW_LANES = ["in-review"] as const; +const LEGACY_COMPLETE_LANES = ["done"] as const; + +/** Lane ids for the two roles this query reads; omit to keep the built-in board's ids. */ +export interface InReviewDurationLanes { + reviewColumns?: ReadonlyArray; + completeColumns?: ReadonlyArray; +} + +/** `expr = ANY(values)` as an OR of parameterised equalities — never string-built. */ +function metadataColumnIn(field: "from" | "to", values: ReadonlyArray) { + const key = field === "to" ? sql`'to'` : sql`'from'`; + const parts = values.map((value) => sql`${schema.project.activityLog.metadata}->>${key} = ${value}`); + return parts.length === 1 ? parts[0] : or(...parts); +} + export async function getInReviewDurationEvents( db: AsyncDataLayer["db"] | DbTransaction, projectId: string, options: { since: string; until: string }, + lanes?: InReviewDurationLanes, ): Promise { + const reviewColumns = lanes?.reviewColumns?.length ? lanes.reviewColumns : LEGACY_REVIEW_LANES; + const completeColumns = lanes?.completeColumns?.length ? lanes.completeColumns : LEGACY_COMPLETE_LANES; const rows = await db .select() .from(schema.project.activityLog) @@ -348,10 +392,10 @@ export async function getInReviewDurationEvents( gt(schema.project.activityLog.timestamp, options.since), lte(schema.project.activityLog.timestamp, options.until), or( - sql`${schema.project.activityLog.metadata}->>'to' = 'in-review'`, + metadataColumnIn("to", reviewColumns), and( - sql`${schema.project.activityLog.metadata}->>'from' = 'in-review'`, - sql`${schema.project.activityLog.metadata}->>'to' = 'done'`, + metadataColumnIn("from", reviewColumns), + metadataColumnIn("to", completeColumns), ), ), )) diff --git a/packages/core/src/task-store/workflow-definitions.ts b/packages/core/src/task-store/workflow-definitions.ts index e629193e14..b3f26e15e2 100644 --- a/packages/core/src/task-store/workflow-definitions.ts +++ b/packages/core/src/task-store/workflow-definitions.ts @@ -43,6 +43,7 @@ import { resolveDefaultOnOptionalGroupIds } from "../workflow-optional-steps.js" import { resolveSwitchReconciliation } from "../workflow-reconciliation.js"; import { WORKFLOW_COMPILED_STEP_TEMPLATE_PREFIX } from "../store.js"; import { resolveWorkflowIrForTask } from "../workflow-ir-resolver.js"; +import { resolveProjectColumnsForRoles, REVIEW_ROLES } from "../project-lane-vocabulary.js"; export async function getAgentLogsByTimeRangeImpl(store: TaskStore, taskId: string, @@ -993,9 +994,31 @@ export function getSettingsSyncImpl(store: TaskStore): Settings { return store.settingsSyncCache ?? DEFAULT_SETTINGS; } +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-22:15: +Resolve the two roles the duration query reads, ONCE per call, and hand them to the SQL. + +The predicate had `in-review` and `done` baked into a `sql` template — the one place neither the +lifecycle census nor the unwired-lane-parameter guard can see them — so the Reliability panel's +duration metric stayed blind on a renamed board after #2861 fixed the two counts beside it. + +This impl is where the store is available, which is why the resolution lives here rather than in +`async-audit.ts`: that function takes a `db` handle and cannot resolve anything. Best-effort — a +failed resolve leaves the query on its documented legacy lanes rather than failing the panel. +*/ export async function getInReviewDurationEventsImpl(store: TaskStore, options: { since: string; until: string }): Promise { const layer = store.asyncLayer!; - return getInReviewDurationEventsAsync(layer.db, layer.projectId ?? "", options); + let lanes: { reviewColumns?: string[]; completeColumns?: string[] } | undefined; + try { + const [review, complete] = await Promise.all([ + resolveProjectColumnsForRoles(store, REVIEW_ROLES), + resolveProjectColumnsForRoles(store, ["complete"]), + ]); + lanes = { reviewColumns: [...review], completeColumns: [...complete] }; + } catch { + lanes = undefined; + } + return getInReviewDurationEventsAsync(layer.db, layer.projectId ?? "", options, lanes); } export async function getTaskMergedTaskIdsImpl(store: TaskStore, options: { since: string; until: string }): Promise> { diff --git a/packages/dashboard/src/__tests__/reliability-metrics.test.ts b/packages/dashboard/src/__tests__/reliability-metrics.test.ts index e003a7a89a..c39abda9e4 100644 --- a/packages/dashboard/src/__tests__/reliability-metrics.test.ts +++ b/packages/dashboard/src/__tests__/reliability-metrics.test.ts @@ -304,3 +304,90 @@ describe("reliability move counts span every lane carrying the role", () => { expect(await countMovesInto(store({}) as never, WINDOW, new Set())).toEqual({}); }); }); + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-00:05 (#2875 review — greptile P1): + +THE PRODUCER WAS CONVERTED AND THE CONSUMER DISCARDED THE ANSWER. + +`getInReviewDurationEvents` fetches moves using the project's RESOLVED review lanes; this function +then matched `to === "in-review"` / `to === "done"` against them. On a renamed board every fetched +event was thrown away, the sample count never reached three, and the panel reported +`insufficient-samples` — silently absent rather than visibly wrong, which is why it survived. + +The negative matters as much: widening to "any move counts" would satisfy the renamed case and start +measuring wip -> wip transitions as review durations. +*/ +describe("inReviewDurationMetrics keys on the board's resolved lanes", () => { + const lanes = { review: new Set(["checking"]), complete: new Set(["shipped"]) }; + const move = (taskId: string, from: string, to: string, at: string) => ({ + type: "task:moved", + taskId, + timestamp: at, + metadata: { from, to }, + }) as unknown as ActivityLogEntry; + + /* Three samples: the metric requires at least that many before it reports anything. */ + const renamedActivity = ["A", "B", "C"].flatMap((id, i) => [ + move(id, "building", "checking", `2026-05-10T0${i}:00:00.000Z`), + move(id, "checking", "shipped", `2026-05-10T0${i}:30:00.000Z`), + ]); + const from = Date.parse("2026-05-10T00:00:00.000Z"); + const to = Date.parse("2026-05-11T00:00:00.000Z"); + + it("measures review duration on a RENAMED board when the caller supplies its lanes", () => { + const metric = inReviewDurationMetrics(renamedActivity, from, to, lanes); + + expect(metric.sampleCount).toBe(3); + expect(metric.p50Ms).toBe(30 * 60_000); + }); + + it("reports insufficient-samples for the same activity without the lanes — the defect", () => { + const metric = inReviewDurationMetrics(renamedActivity, from, to); + + expect(metric.sampleCount).toBe(0); + expect(metric.reason).toBe("insufficient-samples"); + }); + + it("keeps the ORIGINAL review-entry time when a card moves between two review lanes", () => { + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-21:35 (#2875 review — greptile P1): + A board may declare several review-role lanes, and a card moving between them has NOT re-entered + review. Overwriting the start on every move into a review lane made the metric measure only the + LAST lane — so the number shrank exactly on the boards that review most carefully, and it looked + plausible the whole time. + + Each card here spends 30 minutes in `checking`, hops to a second review lane, then completes 30 + minutes later. The honest duration is 60; the defect reports 30. + */ + const twoReviewLanes = { review: new Set(["checking", "signoff"]), complete: new Set(["shipped"]) }; + const activity = ["A", "B", "C"].flatMap((id, i) => [ + move(id, "building", "checking", `2026-05-10T0${i}:00:00.000Z`), + move(id, "checking", "signoff", `2026-05-10T0${i}:30:00.000Z`), + move(id, "signoff", "shipped", `2026-05-10T0${i}:59:59.999Z`), + ]); + + const metric = inReviewDurationMetrics(activity, from, to, twoReviewLanes); + + expect(metric.sampleCount).toBe(3); + expect(metric.p50Ms).toBe(59 * 60_000 + 59_999); + }); + + it("does NOT count a card that ENTERED review and then bounced out to WIP", () => { + /* + The paired negative, and it has to look like this to bite. A card that never enters review records + no start, so ANY exit predicate — including "count every move" — yields zero and the test passes + against a broken implementation. Measured: my first version used drafting -> building noise and + stayed green when the exit condition was widened to `ms in range`. + + Entering `checking` and leaving to `building` is the shape that distinguishes them: a start IS + recorded, so only a correct COMPLETE-lane check keeps it out of the durations. + */ + const bounced = ["A", "B", "C"].flatMap((id, i) => [ + move(id, "building", "checking", `2026-05-10T0${i}:00:00.000Z`), + move(id, "checking", "building", `2026-05-10T0${i}:30:00.000Z`), + ]); + + expect(inReviewDurationMetrics(bounced, from, to, lanes).sampleCount).toBe(0); + }); +}); diff --git a/packages/dashboard/src/reliability-metrics.ts b/packages/dashboard/src/reliability-metrics.ts index 59382a68a3..832c916fb2 100644 --- a/packages/dashboard/src/reliability-metrics.ts +++ b/packages/dashboard/src/reliability-metrics.ts @@ -119,10 +119,29 @@ function percentile(sortedValues: number[], p: number): number { return sortedValues[Math.min(sortedValues.length - 1, Math.max(0, index))] ?? 0; } -/* FNXC:ReliabilityMetrics 2026-07-30-03:10 DELIBERATE-LITERAL: historical log values — `from`/`to` - as RECORDED on a past move event, matched as recorded. Full reasoning above - `tasksEnteredInReviewPerDay`. */ -export function inReviewDurationMetrics(activity: ActivityLogEntry[], startMs: number, endMs: number): InReviewDurationMetric { +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-23:55 (#2875 review — greptile P1, "resolved lanes discarded +downstream"): THE PRODUCER WAS CONVERTED AND THIS CONSUMER THREW THE ANSWER AWAY. + +`getInReviewDurationEvents` now fetches moves using the project's RESOLVED review lanes, and this +function then matched `to === "in-review"` and `to === "done"` against them. On a renamed board every +fetched event was discarded, the sample count stayed under three, and the Reliability panel reported +`insufficient-samples` forever — a metric that is silently absent rather than visibly wrong, which is +why nothing surfaced it. + +The lane sets are OPTIONAL and the production caller supplies them: `server.ts` already resolves +`reviewLanes` for `countEntriesInto`/`countBouncesOut` two statements above this call, so wiring costs +no extra read. Omitted, the legacy ids answer — the documented degraded path for the pure function's +own tests, not a floor anything in production takes. +*/ +export function inReviewDurationMetrics( + activity: ActivityLogEntry[], + startMs: number, + endMs: number, + lanes?: { review?: ReadonlySet; complete?: ReadonlySet }, +): InReviewDurationMetric { + const reviewLanes = lanes?.review ?? new Set(["in-review"]); + const completeLanes = lanes?.complete ?? new Set(["done"]); const moved = activity .filter((entry) => entry.type === "task:moved") .map((entry) => ({ entry, ms: new Date(entry.timestamp).getTime() })) @@ -141,12 +160,30 @@ export function inReviewDurationMetrics(activity: ActivityLogEntry[], startMs: n const from = metadataColumn(entry, "from"); const to = metadataColumn(entry, "to"); - if (to === "in-review") { - latestInReviewEntryByTask.set(taskId, ms); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-21:30 (#2875 review — greptile P1, "internal review moves + reset duration"): ENTERING REVIEW IS A CROSSING, NOT AN ARRIVAL. + + A board may declare several review-role lanes — merge orchestration beside a human sign-off lane — + and a card moving BETWEEN them has not re-entered review. Overwriting the timestamp on every move + whose destination is a review lane made the metric measure only the LAST lane, so the number shrank + exactly on the boards that review most carefully. It read plausible, which is why it needed the + review to find. + + The start is therefore recorded only when the card was NOT already in a review lane. An unknown + `from` (absent metadata) still records, because a first observation with no prior lane is an entry + as far as this data can tell — dropping it would lose the sample entirely, which is worse than + dating it slightly late. + */ + if (to !== undefined && reviewLanes.has(to)) { + const alreadyInReview = from !== undefined && reviewLanes.has(from); + if (!alreadyInReview) latestInReviewEntryByTask.set(taskId, ms); continue; } - if (from === "in-review" && to === "done" && ms >= startMs && ms <= endMs) { + if (from !== undefined && to !== undefined + && reviewLanes.has(from) && completeLanes.has(to) + && ms >= startMs && ms <= endMs) { const start = latestInReviewEntryByTask.get(taskId); if (typeof start === "number" && ms >= start) { durations.push(ms - start); diff --git a/packages/dashboard/src/server.ts b/packages/dashboard/src/server.ts index e89c20e813..19e98fc75c 100644 --- a/packages/dashboard/src/server.ts +++ b/packages/dashboard/src/server.ts @@ -1916,7 +1916,13 @@ 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); - const duration = inReviewDurationMetrics(durationEvents, 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, + }); const mergeAttempts = mergeAttemptsPerMergedTask(runAuditEvents, mergedTaskIds, effectiveStartMs, nowMs); const headline = inReviewFailureRate7d(enteredByDay, bouncedByDay, nowMs); diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index ea28b9119b..43c22c1fdf 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -39,7 +39,6 @@ "packages/engine/src/triage.ts": 1 }, "deliberateByFile": { - "packages/dashboard/src/reliability-metrics.ts\u0000in-review": 4, "packages/dashboard/app/components/TaskContextMenu.tsx\u0000in-review": 3, "packages/core/src/live-agent-count.ts\u0000in-progress": 2, "packages/core/src/live-agent-count.ts\u0000in-review": 2, @@ -53,6 +52,7 @@ "packages/dashboard/app/components/TaskContextMenu.tsx\u0000done": 2, "packages/dashboard/app/components/TaskDetailModal.tsx\u0000triage": 2, "packages/dashboard/app/hooks/useTaskDiffStats.ts\u0000done": 2, + "packages/dashboard/src/reliability-metrics.ts\u0000in-review": 2, "packages/engine/src/cli-agent/state-machine.ts\u0000done": 2, "packages/engine/src/scheduler.ts\u0000archived": 2, "packages/engine/src/scheduler.ts\u0000done": 2, @@ -105,7 +105,6 @@ "packages/dashboard/src/github-tracking-state.ts\u0000archived": 1, "packages/dashboard/src/github-tracking-state.ts\u0000done": 1, "packages/dashboard/src/gitlab-tracking-comments.ts\u0000in-progress": 1, - "packages/dashboard/src/reliability-metrics.ts\u0000done": 1, "packages/dashboard/src/reliability-metrics.ts\u0000in-progress": 1, "packages/dashboard/src/routes/register-task-workflow-routes.ts\u0000todo": 1, "packages/dashboard/src/routes/register-task-workflow-routes.ts\u0000triage": 1,