diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 9f589d7a16..4487db1742 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -2,16 +2,18 @@ // done, powering the above-the-fold homepage counter. Flag-gated by GITTENSORY_PUBLIC_STATS (default OFF): when // off the public endpoint 404s, so the deploy is byte-identical to today until the flag is deliberately set. // -// REALTIME: queries the live tables directly (no rollup/cron) so a new review shows up within the 60s HTTP cache -// window — single source of truth, always current. The source is review_targets (one row per PR, scoped to the -// repos the review system handles: gittensory, awesome-claude, metagraphed) with the terminal review DISPOSITION -// in `status`. This is NOT the broader pull_requests / recent_merged_pull_requests mining caches. +// REALTIME: queries the live ledger directly (no rollup/cron) so a new review shows up within the 60s HTTP cache +// window. "reviewed" = a distinct PR for which the review system published a public review surface (audit_events +// `github_app.pr_public_surface_published`, scoped to the repos it handles: gittensory, awesome-claude, +// metagraphed); each PR's terminal DISPOSITION is read from the pull_requests cache. (The legacy review_targets +// ledger this used to read was orphaned by the convergence cutover — nothing writes it anymore.) // -// DISPOSITIONS (terminal): merged / closed = gittensory auto-actioned; commented = reviewed + advised, deferred -// to a maintainer; manual = escalated to a maintainer; ignored = skipped (drafts/bots/excluded); error = failed. -// reviewed = merged + closed + commented + manual (PRs it actually reviewed; excludes ignored + error) +// DISPOSITIONS: merged (merged_at set) / closed (closed without a merge) = the review system auto-actioned; +// commented = still-open reviewed PRs (reviewed + advised, awaiting a maintainer / CI). Reviewed PRs that never +// got a published surface (skipped drafts/bots, errors) simply don't appear — there is no ignored/manual/error. +// reviewed = merged + closed + commented (every distinct PR a review surface was published for) // filteredPct = (reviewed - merged) / reviewed (share resolved WITHOUT a merge — noise kept off humans) -// accuracyPct = 1 - reversed / (merged + closed) (reversal-grounded; reversed from review_audit) +// accuracyPct = 1 - reversed / (merged + closed) (reversal-grounded; reversed from review_audit history) // minutesSaved = reviewed * MINUTES_SAVED_PER_PR (estimated maintainer review time saved) // // PRIVACY: counts only — no PR content, authors, scores, or reward internals. Safe to serve publicly. @@ -92,13 +94,10 @@ function publicStatsProjects(env: { interface DispositionRow { project: string; - handled: number; + reviewed: number; merged: number; closed: number; - commented: number; - ignored: number; - manual: number; - error: number; + inReview: number; } export interface PublicStatsPayload { @@ -130,23 +129,28 @@ export interface PublicStatsPayload { }>; } -const DISPOSITION_SELECT = ` - SUM(CASE WHEN status = 'merged' THEN 1 ELSE 0 END) AS merged, - SUM(CASE WHEN status = 'closed' THEN 1 ELSE 0 END) AS closed, - SUM(CASE WHEN status = 'commented' THEN 1 ELSE 0 END) AS commented, - SUM(CASE WHEN status = 'ignored' THEN 1 ELSE 0 END) AS ignored, - SUM(CASE WHEN status = 'manual' THEN 1 ELSE 0 END) AS manual, - SUM(CASE WHEN status = 'error' THEN 1 ELSE 0 END) AS error`; +// Live "reviewed" = a distinct PR for which the bot published a review surface (audit_events +// `github_app.pr_public_surface_published`, target_key "owner/repo#number"). Its terminal DISPOSITION +// (merged / closed-without-merge / still-open-in-review) comes from the pull_requests cache. This replaces the +// legacy review_targets ledger, which the convergence cutover orphaned (nothing writes it anymore). `reversed` +// (the accuracy denominator) still comes from review_audit's historical reversal events — there is no live +// reversal signal yet, so it acts as a floor. All reads are public-safe COUNTs and degrade to 0 via safeAll. +const PUBLISHED_PR_KEYS = ` + SELECT + substr(target_key, 1, instr(target_key, '#') - 1) AS repo, + CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS number, + created_at + FROM audit_events + WHERE event_type = 'github_app.pr_public_surface_published' AND instr(target_key, '#') > 0`; -/** Assemble the public-safe payload from the LIVE review ledger (cheap: review_targets is one row per PR). */ +/** Assemble the public-safe payload from the LIVE review ledger: distinct PRs the bot published a review for + * (audit_events) joined to their terminal disposition (pull_requests state). Realtime behind the 60s HTTP cache + * — a new review shows up within ~a minute; no rollup/cron. */ export async function getPublicStats( env: Env, nowMs: number = Date.now(), ): Promise { - const sinceIso = new Date(nowMs - 7 * 86_400_000) - .toISOString() - .slice(0, 19) - .replace("T", " "); + const sinceIso = new Date(nowMs - 7 * 86_400_000).toISOString(); const projects = publicStatsProjects(env); const generatedAt = new Date(nowMs).toISOString(); const empty = (): PublicStatsPayload => ({ @@ -171,34 +175,48 @@ export async function getPublicStats( }); if (projects.length === 0) return empty(); - const projectFilter = `LOWER(project) IN (${projects.map(() => "?").join(", ")})`; - const [dispositions, reversalRows, weekly] = await Promise.all([ + const inList = projects.map(() => "?").join(", "); + const [dispositions, reversalRows, weeklyRows] = await Promise.all([ safeAll( env, - `SELECT project, COUNT(*) AS handled,${DISPOSITION_SELECT} FROM review_targets WHERE ${projectFilter} GROUP BY project`, + `SELECT ev.repo AS project, + COUNT(*) AS reviewed, + SUM(CASE WHEN pr.merged_at IS NOT NULL THEN 1 ELSE 0 END) AS merged, + SUM(CASE WHEN pr.state = 'closed' AND pr.merged_at IS NULL THEN 1 ELSE 0 END) AS closed, + SUM(CASE WHEN pr.id IS NULL OR pr.state = 'open' THEN 1 ELSE 0 END) AS inReview + FROM (SELECT DISTINCT repo, number FROM (${PUBLISHED_PR_KEYS})) ev + LEFT JOIN pull_requests pr ON pr.repo_full_name = ev.repo AND pr.number = ev.number + WHERE LOWER(ev.repo) IN (${inList}) + GROUP BY ev.repo`, ...projects, ), safeAll<{ project: string; reversed: number }>( env, `SELECT project, COUNT(*) AS reversed FROM review_audit - WHERE event_type IN ('reversal_reverted', 'reversal_reopened') AND ${projectFilter} GROUP BY project`, + WHERE event_type IN ('reversal_reverted', 'reversal_reopened') AND LOWER(project) IN (${inList}) + GROUP BY project`, ...projects, ), - safeAll<{ - merged: number; - closed: number; - commented: number; - manual: number; - }>( + safeAll<{ reviewed: number; merged: number }>( env, - `SELECT${DISPOSITION_SELECT.replace(/, $/, "")} FROM review_targets WHERE created_at >= ? AND ${projectFilter}`, + `SELECT + SUM(CASE WHEN first_seen >= ? THEN 1 ELSE 0 END) AS reviewed, + SUM(CASE WHEN merged_at IS NOT NULL AND merged_at >= ? THEN 1 ELSE 0 END) AS merged + FROM ( + SELECT ev.repo, ev.number, MIN(ev.created_at) AS first_seen, MAX(pr.merged_at) AS merged_at + FROM (${PUBLISHED_PR_KEYS}) ev + LEFT JOIN pull_requests pr ON pr.repo_full_name = ev.repo AND pr.number = ev.number + WHERE LOWER(ev.repo) IN (${inList}) + GROUP BY ev.repo, ev.number + )`, + sinceIso, sinceIso, ...projects, ), ]); const reversedByProject = new Map( - reversalRows.map((r) => [r.project, r.reversed ?? 0]), + reversalRows.map((r) => [String(r.project).toLowerCase(), r.reversed ?? 0]), ); const totals = { handled: 0, @@ -214,20 +232,16 @@ export async function getPublicStats( .map((d) => { const merged = d.merged ?? 0; const closed = d.closed ?? 0; - const commented = d.commented ?? 0; - const manual = d.manual ?? 0; - const ignored = d.ignored ?? 0; - const error = d.error ?? 0; - const reversed = reversedByProject.get(d.project) ?? 0; - totals.handled += d.handled ?? 0; + const inReview = d.inReview ?? 0; + const reversed = + reversedByProject.get(String(d.project).toLowerCase()) ?? 0; + const reviewed = merged + closed + inReview; + totals.handled += reviewed; totals.merged += merged; totals.closed += closed; - totals.commented += commented; - totals.ignored += ignored; - totals.manual += manual; - totals.error += error; + // "commented" carries the still-open reviewed PRs (reviewed + advised, awaiting a maintainer / CI). + totals.commented += inReview; totals.reversed += reversed; - const reviewed = reviewedOf({ merged, closed, commented, manual }); return { project: d.project, reviewed, @@ -240,7 +254,7 @@ export async function getPublicStats( .sort((a, b) => b.reviewed - a.reviewed); const reviewed = reviewedOf(totals); - const w = weekly[0] ?? { merged: 0, closed: 0, commented: 0, manual: 0 }; + const w = weeklyRows[0] ?? { reviewed: 0, merged: 0 }; return { generatedAt, updatedAt: generatedAt, @@ -251,7 +265,7 @@ export async function getPublicStats( accuracyPct: accuracyPct(totals.merged, totals.closed, totals.reversed), minutesSaved: reviewed * MINUTES_SAVED_PER_PR, }, - weekly: { reviewed: reviewedOf(w), merged: w.merged ?? 0 }, + weekly: { reviewed: w.reviewed ?? 0, merged: w.merged ?? 0 }, byProject, }; } diff --git a/test/integration/public-stats-route.test.ts b/test/integration/public-stats-route.test.ts index e4b0264c39..9c083bd6a9 100644 --- a/test/integration/public-stats-route.test.ts +++ b/test/integration/public-stats-route.test.ts @@ -2,26 +2,39 @@ import { describe, expect, it } from "vitest"; import { createApp } from "../../src/api/routes"; import { createTestEnv } from "../helpers/d1"; -/** Seed a handful of terminal review dispositions (+ one reversal) into review_targets / review_audit. */ +/** Seed the LIVE ledger: a published-review surface per reviewed PR (audit_events) + each PR's terminal + * disposition (pull_requests state/merged_at), plus one reversal (review_audit). */ async function seed(env: Env) { - const rows: Array<[string, string, number, string]> = [ - ["t-m1", "JSONbored/gittensory", 1, "merged"], - ["t-c1", "JSONbored/gittensory", 2, "closed"], - ["t-cm1", "JSONbored/gittensory", 3, "commented"], - ["t-ig1", "JSONbored/gittensory", 4, "ignored"], // excluded from "reviewed" - ["t-m2", "JSONbored/awesome-claude", 5, "merged"], - ["t-m3", "JSONbored/awesome-claude", 6, "merged"], + // [repo, number, state, mergedAt] — merged (merged_at set) / closed (state closed, no merge) / open (in review). + const prs: Array<[string, number, string, string | null]> = [ + ["JSONbored/gittensory", 1, "closed", "2026-06-20T00:00:00Z"], // merged + ["JSONbored/gittensory", 2, "closed", null], // closed without merge + ["JSONbored/gittensory", 3, "open", null], // still in review + ["JSONbored/awesome-claude", 5, "closed", "2026-06-20T00:00:00Z"], // merged + ["JSONbored/awesome-claude", 6, "closed", "2026-06-20T00:00:00Z"], // merged ]; - for (const [id, project, number, status] of rows) { + for (const [repo, number, state, mergedAt] of prs) { await env.DB.prepare( - `INSERT INTO review_targets (id, project, kind, repo, number, status) VALUES (?, ?, 'pr', ?, ?, ?)`, + `INSERT INTO audit_events (id, event_type, target_key, outcome) VALUES (?, 'github_app.pr_public_surface_published', ?, 'completed')`, ) - .bind(id, project, project, number, status) + .bind(`ae-${repo}-${number}`, `${repo}#${number}`) + .run(); + await env.DB.prepare( + `INSERT INTO pull_requests (id, repo_full_name, number, title, state, merged_at) VALUES (?, ?, ?, ?, ?, ?)`, + ) + .bind( + `pr-${repo}-${number}`, + repo, + number, + `PR ${number}`, + state, + mergedAt, + ) .run(); } // One human reversal of a gittensory auto-merge (awesome-claude has none → exercises the per-project ?? 0). await env.DB.prepare( - `INSERT INTO review_audit (id, project, target_id, event_type, decision) VALUES ('rev1', 'JSONbored/gittensory', 't-m1', 'reversal_reverted', 'merge')`, + `INSERT INTO review_audit (id, project, target_id, event_type, decision) VALUES ('rev1', 'JSONbored/gittensory', 'JSONbored/gittensory#1', 'reversal_reverted', 'merge')`, ).run(); } @@ -47,14 +60,14 @@ describe("GET /v1/public/stats (#1059)", () => { weekly: { reviewed: number; merged: number }; byProject: Array<{ project: string; reviewed: number }>; }; - expect(body.totals.handled).toBe(6); + expect(body.totals.handled).toBe(5); // distinct reviewed PRs expect(body.totals.merged).toBe(3); expect(body.totals.closed).toBe(1); - expect(body.totals.commented).toBe(1); - expect(body.totals.ignored).toBe(1); + expect(body.totals.commented).toBe(1); // the still-open reviewed PR + expect(body.totals.ignored).toBe(0); expect(body.totals.manual).toBe(0); expect(body.totals.error).toBe(0); - expect(body.totals.reviewed).toBe(5); // merged 3 + closed 1 + commented 1 (ignored excluded) + expect(body.totals.reviewed).toBe(5); // merged 3 + closed 1 + in-review 1 expect(body.totals.reversed).toBe(1); expect(body.totals.accuracyPct).toBe(75); // 1 - 1 / (3 + 1) // busiest repo first: gittensory reviewed 3 (m1+c1+cm1) > awesome-claude 2 (m2+m3) diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index eabd83e5a7..dd9d449f0d 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -7,7 +7,10 @@ import { type Row = Record; -// Stub D1: route reads by SQL (FROM table + clause). Supports prepare(sql).all() and prepare(sql).bind(...).all(). +// Stub D1: route reads by SQL signature. The three reads are distinguished by: +// - weekly: contains `first_seen` +// - dispositions: contains `github_app.pr_public_surface_published` (and is NOT the weekly read) +// - reversals: contains `FROM review_audit` function stubEnv(handler: (sql: string, args: unknown[]) => Row[]): Env { const make = (sql: string, args: unknown[]) => ({ bind: (...a: unknown[]) => make(sql, a), @@ -22,6 +25,15 @@ function stubEnv(handler: (sql: string, args: unknown[]) => Row[]): Env { const NOW = Date.parse("2026-06-22T00:00:00Z"); +function isWeekly(sql: string): boolean { + return sql.includes("first_seen"); +} +function isDispositions(sql: string): boolean { + return ( + sql.includes("github_app.pr_public_surface_published") && !isWeekly(sql) + ); +} + describe("isPublicStatsEnabled", () => { it("is truthy only for 1/true/yes/on (case-insensitive)", () => { for (const v of ["1", "true", "TRUE", "yes", "on"]) @@ -32,43 +44,34 @@ describe("isPublicStatsEnabled", () => { }); describe("getPublicStats — live aggregate over the review ledger", () => { - // Real prod proportions: merged 1392 + closed 724 + commented 514 + ignored 491 + manual 78 + error 34 = 3233; - // reviewed = 1392+724+514+78 = 2708; reversed 33 over 2116 auto-actions. + // Live shape: distinct reviewed PRs (audit_events) per repo, split by terminal disposition from pull_requests + // (merged / closed-without-merge / still-open-in-review). reviewed = merged + closed + inReview. function ledger(sql: string): Row[] { - if ( - sql.includes("FROM review_targets") && - sql.includes("GROUP BY project") - ) { + if (isWeekly(sql)) { + return [{ reviewed: 1420, merged: 900 }]; + } + if (isDispositions(sql)) { return [ { project: "JSONbored/awesome-claude", - handled: 2066, + reviewed: 2034, merged: 1231, closed: 524, - commented: 200, - ignored: 80, - manual: 31, - error: 0, + inReview: 279, }, { project: "JSONbored/metagraphed", - handled: 829, + reviewed: 393, merged: 137, closed: 176, - commented: 200, - ignored: 300, - manual: 16, - error: 0, + inReview: 80, }, { project: "JSONbored/gittensory", - handled: 338, + reviewed: 315, merged: 24, closed: 24, - commented: 114, - ignored: 111, - manual: 31, - error: 34, + inReview: 267, }, ]; } @@ -79,31 +82,26 @@ describe("getPublicStats — live aggregate over the review ledger", () => { { project: "JSONbored/gittensory", reversed: 3 }, ]; } - if (sql.includes("created_at >= ?")) { - return [{ merged: 900, closed: 300, commented: 200, manual: 20 }]; - } return []; } it("derives reviewed / filtered% / accuracy / time-saved from real-shaped data", async () => { const out = await getPublicStats(stubEnv(ledger), NOW); - expect(out.totals.handled).toBe(3233); - expect(out.totals.merged).toBe(1392); - expect(out.totals.closed).toBe(724); - expect(out.totals.commented).toBe(514); - expect(out.totals.ignored).toBe(491); - expect(out.totals.manual).toBe(78); - expect(out.totals.error).toBe(34); - expect(out.totals.reversed).toBe(33); - // reviewed = merged + closed + commented + manual = 2708 - expect(out.totals.reviewed).toBe(2708); - // filtered = (2708 - 1392) / 2708 = 48.6% - expect(out.totals.filteredPct).toBe(48.6); + // handled = reviewed = 2034 + 393 + 315 = 2742 + expect(out.totals.handled).toBe(2742); + expect(out.totals.merged).toBe(1392); // 1231 + 137 + 24 + expect(out.totals.closed).toBe(724); // 524 + 176 + 24 + expect(out.totals.commented).toBe(626); // still-open reviewed PRs: 279 + 80 + 267 + expect(out.totals.ignored).toBe(0); + expect(out.totals.manual).toBe(0); + expect(out.totals.error).toBe(0); + expect(out.totals.reversed).toBe(33); // 20 + 10 + 3 + expect(out.totals.reviewed).toBe(2742); + // filtered = (2742 - 1392) / 2742 = 49.2% + expect(out.totals.filteredPct).toBe(49.2); // accuracy = 1 - 33 / (1392 + 724) = 98.4% expect(out.totals.accuracyPct).toBe(98.4); - // time saved = 2708 * 15 min - expect(out.totals.minutesSaved).toBe(2708 * MINUTES_SAVED_PER_PR); - // weekly reviewed = 900 + 300 + 200 + 20 = 1420 + expect(out.totals.minutesSaved).toBe(2742 * MINUTES_SAVED_PER_PR); expect(out.weekly).toEqual({ reviewed: 1420, merged: 900 }); expect(out.byProject.map((p) => p.project)).toEqual([ "JSONbored/awesome-claude", @@ -122,59 +120,39 @@ describe("getPublicStats — live aggregate over the review ledger", () => { { project: "CustomerCo/stealth-product", reversed: 1 }, ].filter((row) => args.includes(String(row.project).toLowerCase())); } - if (sql.includes("created_at >= ?")) { - const allowed = args.slice(1); + if (isWeekly(sql)) { + const allowed = args.slice(2); // [sinceIso, sinceIso, ...projects] const weeklyRows = [ - { - project: "JSONbored/gittensory", - merged: 1, - closed: 1, - commented: 0, - manual: 0, - }, - { - project: "CustomerCo/stealth-product", - merged: 3, - closed: 0, - commented: 0, - manual: 0, - }, + { project: "JSONbored/gittensory", reviewed: 2, merged: 1 }, + { project: "CustomerCo/stealth-product", reviewed: 3, merged: 3 }, ].filter((row) => allowed.includes(String(row.project).toLowerCase()), ); return [ weeklyRows.reduce( (acc, row) => ({ + reviewed: acc.reviewed + row.reviewed, merged: acc.merged + row.merged, - closed: acc.closed + row.closed, - commented: acc.commented + row.commented, - manual: acc.manual + row.manual, }), - { merged: 0, closed: 0, commented: 0, manual: 0 }, + { reviewed: 0, merged: 0 }, ), ]; } - if (sql.includes("GROUP BY project")) { + if (isDispositions(sql)) { return [ { project: "JSONbored/gittensory", - handled: 2, + reviewed: 2, merged: 1, closed: 1, - commented: 0, - ignored: 0, - manual: 0, - error: 0, + inReview: 0, }, { project: "CustomerCo/stealth-product", - handled: 3, + reviewed: 3, merged: 3, closed: 0, - commented: 0, - ignored: 0, - manual: 0, - error: 0, + inReview: 0, }, ].filter((row) => args.includes(String(row.project).toLowerCase())); } @@ -224,8 +202,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { it("is fail-safe: a throwing read degrades to zeros, not an error", async () => { const env = stubEnv((sql) => { - if (sql.includes("GROUP BY project")) - throw new Error("review_targets down"); + if (isDispositions(sql)) throw new Error("audit_events down"); return []; }); const out = await getPublicStats(env, NOW); @@ -235,36 +212,27 @@ describe("getPublicStats — live aggregate over the review ledger", () => { it("coerces null SUM/reversal/weekly fields to 0 (SUM over an empty set returns NULL in SQLite)", async () => { // Every numeric column comes back null (the nullish arm of each `?? 0`); p2 has no reversal row, exercising - // the `reversedByProject.get(...) ?? 0` fallback; weekly[0] is present but its fields are null. + // the `reversedByProject.get(...) ?? 0` fallback; the weekly row is present but its fields are null. const out = await getPublicStats( stubEnv((sql) => { if (sql.includes("FROM review_audit")) return [{ project: "p1", reversed: null }]; - if (sql.includes("created_at >= ?")) - return [ - { merged: null, closed: null, commented: null, manual: null }, - ]; - if (sql.includes("GROUP BY project")) { + if (isWeekly(sql)) return [{ reviewed: null, merged: null }]; + if (isDispositions(sql)) { return [ { project: "p1", - handled: null, + reviewed: null, merged: null, closed: null, - commented: null, - ignored: null, - manual: null, - error: null, + inReview: null, }, { project: "p2", - handled: null, + reviewed: null, merged: null, closed: null, - commented: null, - ignored: null, - manual: null, - error: null, + inReview: null, }, ]; }