diff --git a/src/review/review-effort.ts b/src/review/review-effort.ts index 9c7c5243eb..8c2a1b3a94 100644 --- a/src/review/review-effort.ts +++ b/src/review/review-effort.ts @@ -46,6 +46,11 @@ function bandForEffort(effort: number): 1 | 2 | 3 | 4 | 5 { return 5; } +/** Map a persisted minutes estimate back to its complexity band (inverse of `estimateReviewEffort`'s minutes step). */ +export function bandFromMinutes(minutes: number): 1 | 2 | 3 | 4 | 5 { + return bandForEffort(Math.max(0, minutes) / MINUTES_PER_EFFORT); +} + /** Estimate the review effort of a change set. Pure and deterministic. */ export function estimateReviewEffort(files: ReviewEffortFile[]): ReviewEffort { let weighted = 0; diff --git a/src/review/stats.ts b/src/review/stats.ts index f8346fc5be..af3179b082 100644 --- a/src/review/stats.ts +++ b/src/review/stats.ts @@ -16,19 +16,11 @@ // deps so the core decision/reversal/gate-action aggregation is fully native here. The host wires its own // implementations (or the defaults below, which emit empty/no-signal reports, keeping the payload shape). // -// SCOPE NOTE (#1955 — deterministic review-effort score): this feed's tables (`review_targets`, `review_audit`, -// and the injected eval/parity engine's own source, `review_audit`'s `gate_decision`/`pr_outcome` rows) are the -// LEGACY reviewbot ledger — nothing writes new rows to `review_targets` since the self-host convergence cutover -// (see public-stats.ts's file header), and every write into `review_audit` (outcomes-wire.ts's `pr_outcome`, -// the gate's `gate_decision`) carries only decision/outcome metadata, never a PR's changed files or patches. -// There is therefore NO live source this module could aggregate a per-PR review-effort estimate FROM today — -// unlike public-stats.ts's `getPublicStats`, which reads the ACTIVE `audit_events` ledger the live review -// pipeline (src/queue/processors.ts) still writes to every publish, and where `estimateReviewEffort`'s minutes -// are now persisted (`reviewEffortMinutes` in `github_app.pr_public_surface_published` metadata) and averaged. -// Wiring a same-shaped aggregate here would only ever read back a permanently-null placeholder — scaffolding -// with no live behavior — so it is deliberately left out of this change rather than faked. A REAL maintainer- -// dashboard effort aggregate needs its own persisted source (e.g. this module reading `audit_events` the way -// public-stats.ts now does), which is a genuine follow-up, not a one-line addition to this file. +// REVIEW EFFORT (#2155): decision/reversal/gate-action aggregates still read the legacy `review_targets` / +// `review_audit` ledgers above, but the maintainer dashboard's complexity read comes from the ACTIVE `audit_events` +// ledger (same `github_app.pr_public_surface_published` rows + `reviewEffortMinutes` metadata public-stats.ts uses). +// Bearer-gated here only — never folded into the public homepage counter. +import { bandFromMinutes } from "./review-effort"; // ── Inlined report types (ported shapes from reviewbot src/core/{eval,tuning}.ts) ──────────────── @@ -176,6 +168,13 @@ const BUCKET_SQL: Record = { month: "strftime('%Y-%m', created_at)", }; +export interface ReviewEffortAggregate { + /** Rounded average complexity band across distinct reviewed PRs in the window; null when no samples. */ + avgBand: number | null; + /** Sum of per-PR estimated review minutes in the window; 0 when no samples. */ + totalEstimatedMinutes: number; +} + export interface StatsPayload { generatedAt: string; window: { fromIso: string; days: number; bucket: string }; @@ -187,6 +186,8 @@ export interface StatsPayload { reversals: Array<{ bucket: string; project: string; n: number }>; /** Non-content gate decisions (incl. SHADOW would-actions), per project+action. */ gateActions: Array<{ project: string; action: string; n: number }>; + /** Aggregate review-effort signal for maintainer triage (#2155); reads `audit_events`, not the legacy ledgers. */ + reviewEffort: ReviewEffortAggregate; /** Gate eval: prediction scored against the PR's real outcome — merge/close precision per project. */ gateEval: GateEvalReport; /** Ranked tuning recommendations derived from the eval (ready-to-flip / tighten / loosen). */ @@ -195,6 +196,18 @@ export interface StatsPayload { gateParity: GateParityReport & { cutoverReady: Array<{ project: string; ready: boolean }> }; } +/** Fold per-PR persisted minutes into the maintainer aggregate (avg band + total minutes). */ +export function aggregateReviewEffort(perPrMinutes: number[]): ReviewEffortAggregate { + if (perPrMinutes.length === 0) { + return { avgBand: null, totalEstimatedMinutes: 0 }; + } + const bands = perPrMinutes.map((minutes) => bandFromMinutes(minutes)); + return { + avgBand: Math.round(bands.reduce((sum, band) => sum + band, 0) / bands.length), + totalEstimatedMinutes: perPrMinutes.reduce((sum, minutes) => sum + minutes, 0), + }; +} + /** Aggregate the decision ledger for the dashboard. Pure-ish (reads D1 only); no GitHub I/O. */ export async function computeStats( env: Env, @@ -211,7 +224,7 @@ export async function computeStats( const bucketExpr = BUCKET_SQL[bucket] ?? BUCKET_SQL.day; const fromIso = new Date(opts.nowMs - days * 86_400_000).toISOString().slice(0, 10); // YYYY-MM-DD - const [decisionRows, reversalRows] = await Promise.all([ + const [decisionRows, reversalRows, effortRows] = await Promise.all([ storage(env).prepare( `SELECT ${bucketExpr} AS bucket, project, COALESCE(verdict, status) AS verdict, COUNT(*) AS n FROM review_targets @@ -226,6 +239,25 @@ export async function computeStats( GROUP BY bucket, project ORDER BY bucket ASC`, ).bind(fromIso).all<{ bucket: string; project: string; n: number }>(), + // review-effort (#2155): same persisted `reviewEffortMinutes` public-stats averages, scoped to this window. + // Repeated publish events for one PR collapse to one sample (per-PR AVG) before the global fold. + storage(env).prepare( + `SELECT minutes FROM ( + SELECT repo, number, AVG(minutes) AS minutes + FROM ( + SELECT LOWER(substr(target_key, 1, instr(target_key, '#') - 1)) AS repo, + CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS number, + json_extract(metadata_json, '$.reviewEffortMinutes') AS minutes + FROM audit_events + WHERE event_type = 'github_app.pr_public_surface_published' + AND created_at >= ? + AND instr(target_key, '#') > 0 + ) + WHERE minutes IS NOT NULL + GROUP BY repo, number + )`, + ).bind(fromIso).all<{ minutes: number }>() + .catch(() => ({ results: [] as Array<{ minutes: number }> })), ]); // Non-content gate decisions (incl. SHADOW would-actions) — recorded as `gate_decision` audit rows with @@ -247,6 +279,9 @@ export async function computeStats( const rows = decisionRows.results ?? []; const reversals = reversalRows.results ?? []; + const reviewEffort = aggregateReviewEffort( + (effortRows.results ?? []).map((row) => row.minutes ?? 0).filter((minutes) => minutes > 0), + ); return { generatedAt: new Date(opts.nowMs).toISOString(), window: { fromIso, days, bucket }, @@ -255,6 +290,7 @@ export async function computeStats( rows, reversals, gateActions: gateRows.results ?? [], + reviewEffort, gateEval, recommendations, gateParity: { ...parity, cutoverReady: parity.rows.map((r) => ({ project: r.project, ready: isParityCutoverReady(r) })) }, diff --git a/test/unit/review-effort.test.ts b/test/unit/review-effort.test.ts index 2bf21f3024..8f336435e9 100644 --- a/test/unit/review-effort.test.ts +++ b/test/unit/review-effort.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { estimateReviewEffort, type ReviewEffortFile } from "../../src/review/review-effort"; +import { bandFromMinutes, estimateReviewEffort, type ReviewEffortFile } from "../../src/review/review-effort"; // A patch with exactly `added` added lines (each `+`), so a test can dial the effort precisely. function srcPatch(added: number): string { @@ -56,3 +56,17 @@ describe("estimateReviewEffort (#2151)", () => { expect(estimateReviewEffort(files)).toEqual({ band: 5, minutes: 258 }); }); }); + +describe("bandFromMinutes (#2155)", () => { + it("maps persisted minutes back to the same band the estimator would have produced", () => { + const samples = [ + estimateReviewEffort([]), + estimateReviewEffort([file("src/a.ts", 20)]), + estimateReviewEffort([file("src/a.ts", 200)]), + estimateReviewEffort([file("src/a.ts", 400)]), + ]; + for (const sample of samples) { + expect(bandFromMinutes(sample.minutes)).toBe(sample.band); + } + }); +}); diff --git a/test/unit/stats.test.ts b/test/unit/stats.test.ts index ae17268146..30b1a70f7a 100644 --- a/test/unit/stats.test.ts +++ b/test/unit/stats.test.ts @@ -1,5 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; +import { createTestEnv } from "../helpers/d1"; import { + aggregateReviewEffort, computeStats, handleParity, handleStats, @@ -22,6 +24,10 @@ function stubEnv(extra: Record = {}): Env { { project: "metagraphed", action: "merge", n: 7 }, { project: "metagraphed", action: "hold", n: 2 }, ]; + const effortMinutes = [ + { minutes: 4 }, + { minutes: 96 }, + ]; let lastSql = ""; return { ...extra, @@ -31,9 +37,15 @@ function stubEnv(extra: Record = {}): Env { return { bind: () => ({ all: async () => ({ - // gate_decision breakdown → gateActions; other review_audit reads → reversals; + // review-effort read → effortMinutes; gate_decision → gateActions; other review_audit → reversals; // everything else → decision rows. (The eval/parity engine is the default no-op deps.) - results: lastSql.includes("gate_decision") ? gateActions : lastSql.includes("review_audit") ? reversals : decisions, + results: lastSql.includes("reviewEffortMinutes") + ? effortMinutes + : lastSql.includes("gate_decision") + ? gateActions + : lastSql.includes("review_audit") + ? reversals + : decisions, }), }), }; @@ -59,6 +71,7 @@ describe("computeStats — D1 aggregate for the dashboard", () => { expect(out.gateEval).toEqual({ rows: [], hasSignal: false }); expect(out.recommendations).toEqual([]); expect(out.gateParity.cutoverReady).toEqual([]); + expect(out.reviewEffort).toEqual({ avgBand: 3, totalEstimatedMinutes: 100 }); }); it("clamps an absurd window and falls back to a safe bucket", async () => { @@ -130,6 +143,96 @@ describe("handleStats — bearer-gated, CORS-open feed", () => { }); }); +describe("aggregateReviewEffort — maintainer complexity fold (#2155)", () => { + it("returns null avgBand and 0 total minutes for an empty sample", () => { + expect(aggregateReviewEffort([])).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); + }); + + it("averages bands and sums minutes across per-PR samples", () => { + // minutes 4 -> band 1; minutes 96 -> band 4 -> rounded avg 3; total 100. + expect(aggregateReviewEffort([4, 96])).toEqual({ avgBand: 3, totalEstimatedMinutes: 100 }); + }); +}); + +describe("computeStats — review-effort read is fail-safe", () => { + function effortThrowingEnv(): Env { + let lastSql = ""; + return { + DB: { + prepare: (s: string) => { + lastSql = s; + return { + bind: () => ({ + all: async () => { + if (lastSql.includes("reviewEffortMinutes")) throw new Error("effort read down"); + return { results: [] }; + }, + }), + }; + }, + }, + } as unknown as Env; + } + + it("falls back to reviewEffort null/0 when the audit_events effort query rejects", async () => { + const out = await computeStats(effortThrowingEnv(), { days: 30, bucket: "day", nowMs: NOW }); + expect(out.reviewEffort).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); + }); + + it("averages real reviewEffortMinutes out of audit_events via json_extract (real D1)", async () => { + const env = createTestEnv(); + const db = env.DB; + await db + .prepare( + `INSERT INTO audit_events (id, event_type, target_key, outcome, metadata_json, created_at) + VALUES (?, ?, ?, ?, ?, ?), (?, ?, ?, ?, ?, ?)`, + ) + .bind( + "published-a", + "github_app.pr_public_surface_published", + "JSONbored/gittensory#10", + "completed", + JSON.stringify({ reviewEffortMinutes: 4 }), + "2026-06-10T00:00:00.000Z", + "published-b", + "github_app.pr_public_surface_published", + "JSONbored/gittensory#11", + "completed", + JSON.stringify({ reviewEffortMinutes: 96 }), + "2026-06-11T00:00:00.000Z", + ) + .run(); + + const out = await computeStats(env, { days: 90, bucket: "day", nowMs: NOW }); + expect(out.reviewEffort).toEqual({ avgBand: 3, totalEstimatedMinutes: 100 }); + }); + + it("skips nullish/zero minute rows when folding reviewEffort (the ?? 0 + > 0 filter branches)", async () => { + function mixedEffortEnv(): Env { + let lastSql = ""; + return { + DB: { + prepare: (s: string) => { + lastSql = s; + return { + bind: () => ({ + all: async () => ({ + results: lastSql.includes("reviewEffortMinutes") + ? [{ minutes: null }, { minutes: 0 }, { minutes: 10 }] + : [], + }), + }), + }; + }, + }, + } as unknown as Env; + } + + const out = await computeStats(mixedEffortEnv(), { days: 30, bucket: "day", nowMs: NOW }); + expect(out.reviewEffort).toEqual({ avgBand: 2, totalEstimatedMinutes: 10 }); + }); +}); + describe("computeStats — gate-decision read is fail-safe", () => { // A stubEnv whose gate_decision query rejects: computeStats should still resolve with gateActions: []. function gateThrowingEnv(): Env { @@ -300,6 +403,7 @@ describe("computeStats — NaN window + null D1 results (the ?? [] fallbacks)", expect(out.gateActions).toEqual([]); expect(out.projects).toEqual([]); expect(out.verdicts).toEqual([]); + expect(out.reviewEffort).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); }); });