diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 3d4836e4d8..5ce085479b 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -14,10 +14,11 @@ // 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) (reversed = engine auto-actions a human overturned, live) -// minutesSaved = reviewed * avgReviewEffortMinutes (estimated maintainer review time saved -- #1955: the -// real per-PR average of `estimateReviewEffort`'s minutes, -// persisted at publish time; MINUTES_SAVED_PER_PR only -// backstops an empty/all-historical ledger) +// minutesSaved = SUM(per-PR COALESCE(reviewEffortMinutes, MINUTES_SAVED_PER_PR)) (estimated maintainer +// review time saved -- #1955/#2070: each distinct +// published PR contributes its persisted estimate, +// with MINUTES_SAVED_PER_PR only backstopping PRs +// that lack a stored estimate) // // PRIVACY: counts only — no PR content, authors, scores, or reward internals. Safe to serve publicly. // @@ -190,7 +191,7 @@ export async function getPublicStats( Promise.resolve([]), Promise.resolve<{ project: string; reversed: number }[]>([]), Promise.resolve<{ reviewed: number; merged: number }[]>([]), - Promise.resolve<{ avgMinutes: number | null }[]>([]), + Promise.resolve<{ totalMinutes: number | null }[]>([]), ]) : await Promise.all([ safeAll( @@ -244,16 +245,11 @@ export async function getPublicStats( sinceIso, ...projects, ), - // review-effort minutes (#1955): a deterministic, no-AI per-PR estimate persisted at publish time - // (processors.ts's pr_public_surface_published metadata.reviewEffortMinutes). Fold repeated publish events - // down to one sample per distinct PR before the global AVG, matching the distinct-PR reviewed denominator - // below; a published row that predates this feature (or a files-fetch failure at publish time) simply has no - // `reviewEffortMinutes` key, so json_extract returns SQL NULL for that row and SQLite's AVG silently skips it - // — an all-historical ledger degrades to a NULL average (handled below via `?? MINUTES_SAVED_PER_PR`), never a - // crash or a skewed zero. - safeAll<{ avgMinutes: number | null }>( + // review-effort minutes (#1955/#2070): sum each distinct published PR's persisted estimate, using + // MINUTES_SAVED_PER_PR only for PRs whose metadata lacks reviewEffortMinutes (mixed-rollout safe). + safeAll<{ totalMinutes: number | null }>( env, - `SELECT AVG(minutes) AS avgMinutes + `SELECT SUM(COALESCE(minutes, ?)) AS totalMinutes FROM ( SELECT repo, number, AVG(minutes) AS minutes FROM ( @@ -267,6 +263,7 @@ export async function getPublicStats( ) GROUP BY repo, number )`, + MINUTES_SAVED_PER_PR, ...projects, ), ]); @@ -316,6 +313,9 @@ export async function getPublicStats( // homepage reflects the whole fleet. No excludeAccount here (see the file header) -- reversals/weekly stay // own-ledger-only (the Orb aggregate only captures merged/closed, not reversals or a trailing-7-day split). The // total grows automatically as more installations register, self-hosted or otherwise. + // Snapshot before Orb merge: effort SQL only covers allowlisted own-ledger publishes, while `reviewed` + // below includes Orb fleet outcomes folded into totals.merged/closed. + const ownLedgerReviewed = reviewedOf(totals); const orb = await getOrbGlobalStats(env); totals.merged += orb.merged; totals.closed += orb.closed; @@ -323,11 +323,13 @@ export async function getPublicStats( const reviewed = reviewedOf(totals); const w = weeklyRows[0] ?? { reviewed: 0, merged: 0 }; - // review-effort minutes (#1955): prefer the REAL average per-review estimate (persisted at publish time from - // estimateReviewEffort); an empty/all-null ledger (no allowlisted project, or every published row predates - // this feature) falls back to the flat MINUTES_SAVED_PER_PR constant, exactly like every other nullish-SUM - // fallback in this module (`?? 0`) — never a crash, never a skewed zero. - const avgReviewEffortMinutes = effortRows[0]?.avgMinutes ?? MINUTES_SAVED_PER_PR; + // review-effort minutes (#1955/#2070): own-ledger publishes sum per-PR estimates (COALESCE fallback); Orb fleet + // outcomes have no persisted effort metadata here, so they still credit the flat MINUTES_SAVED_PER_PR constant. + const minutesSavedTotal = effortRows[0]?.totalMinutes; + const ownLedgerMinutes = + minutesSavedTotal != null ? minutesSavedTotal : ownLedgerReviewed * MINUTES_SAVED_PER_PR; + const minutesSaved = + reviewed === 0 ? 0 : Math.round(ownLedgerMinutes + orb.total * MINUTES_SAVED_PER_PR); return { generatedAt, updatedAt: generatedAt, @@ -336,7 +338,7 @@ export async function getPublicStats( reviewed, filteredPct: filteredPct(reviewed, totals.merged), accuracyPct: accuracyPct(totals.merged, totals.closed, totals.reversed), - minutesSaved: Math.round(reviewed * avgReviewEffortMinutes), + minutesSaved, }, weekly: { reviewed: w.reviewed ?? 0, merged: w.merged ?? 0 }, byProject, diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index d2c99875b7..179c2b5aa0 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -123,32 +123,80 @@ describe("getPublicStats — live aggregate over the review ledger", () => { expect(out.updatedAt).toBe(out.generatedAt); }); - // #1955: minutesSaved now averages the REAL per-PR estimate (estimateReviewEffort's minutes, persisted at - // publish time) instead of unconditionally multiplying by the flat MINUTES_SAVED_PER_PR constant. Proves the - // new estimate is actually used when the ledger has it — the regression case for the flat-constant replacement. - it("uses the real average review-effort minutes when the ledger has them, instead of the flat constant", async () => { + // #1955/#2070: minutesSaved sums per-PR estimates (with MINUTES_SAVED_PER_PR fallback for missing rows) + // instead of multiplying reviewed by a global average. + it("sums the real per-PR review-effort minutes when the ledger has them, instead of the flat constant", async () => { const withEffort = (sql: string): Row[] => { - if (isEffort(sql)) return [{ avgMinutes: 7.4 }]; + if (isEffort(sql)) return [{ totalMinutes: 2742 * 7.4 }]; return ledger(sql); }; const out = await getPublicStats(stubEnv(withEffort), NOW); - // reviewed = 2742 (same ledger as the base test) * 7.4 = 20290.8 -> rounded. expect(out.totals.minutesSaved).toBe(Math.round(2742 * 7.4)); expect(out.totals.minutesSaved).not.toBe(2742 * MINUTES_SAVED_PER_PR); }); - // The nullish arm of `effortRows[0]?.avgMinutes ?? MINUTES_SAVED_PER_PR`: a ledger whose published rows all - // predate this feature (or an empty allowlist) yields a NULL average (SQLite's AVG skips missing json_extract - // keys entirely) rather than a row missing outright — both must degrade to the flat constant, not NaN/0. - it("falls back to the flat MINUTES_SAVED_PER_PR constant when the effort average is SQL NULL", async () => { + // The nullish arm when the effort subquery returns SQL NULL (no published rows in scope). + it("falls back to the flat MINUTES_SAVED_PER_PR constant when the effort sum is SQL NULL", async () => { const nullEffort = (sql: string): Row[] => { - if (isEffort(sql)) return [{ avgMinutes: null }]; + if (isEffort(sql)) return [{ totalMinutes: null }]; return ledger(sql); }; const out = await getPublicStats(stubEnv(nullEffort), NOW); expect(out.totals.minutesSaved).toBe(2742 * MINUTES_SAVED_PER_PR); }); + // #2070: mixed ledgers must COALESCE missing per-PR estimates to MINUTES_SAVED_PER_PR, not AVG-skip them. + it("sums mixed per-PR effort with fallback when one published PR lacks reviewEffortMinutes", async () => { + const env = createTestEnv({ GITTENSORY_PUBLIC_STATS_REPOS: "JSONbored/gittensory" }); + const db = env.DB; + + await db + .prepare( + `INSERT INTO pull_requests (id, repo_full_name, number, title, state, merged_at) + VALUES (?, ?, ?, ?, ?, ?), (?, ?, ?, ?, ?, ?)`, + ) + .bind( + "pr-a", + "JSONbored/gittensory", + 10, + "small fix", + "closed", + "2026-06-01T00:00:00.000Z", + "pr-b", + "JSONbored/gittensory", + 11, + "legacy publish", + "closed", + "2026-06-01T00:00:00.000Z", + ) + .run(); + await db + .prepare( + `INSERT INTO audit_events (id, event_type, target_key, outcome, metadata_json) + VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)`, + ) + .bind( + "published-a", + "github_app.pr_public_surface_published", + "JSONbored/gittensory#10", + "completed", + JSON.stringify({ reviewEffortMinutes: 4 }), + "published-b", + "github_app.pr_public_surface_published", + "JSONbored/gittensory#11", + "completed", + "{}", + ) + .run(); + + const out = await getPublicStats(env, NOW); + + expect(out.totals.reviewed).toBe(2); + expect(out.totals.minutesSaved).toBe(4 + MINUTES_SAVED_PER_PR); + // Old AVG-based path skipped the missing row and under-reported: reviewed * avg(4) = 8. + expect(out.totals.minutesSaved).not.toBe(8); + }); + it("breaks byProject ties on project name so equal-reviewed repos keep a deterministic order", async () => { // Two repos share reviewed=10, fed in reverse-alphabetical input order; the busier repo // still leads and the tied pair must come out alphabetically, not in arbitrary SQL order. @@ -174,6 +222,18 @@ describe("getPublicStats — live aggregate over the review ledger", () => { expect(out.totals.closed).toBe(724 + 30); expect(out.totals.handled).toBe(2742 + 80); expect(out.totals.reviewed).toBe(1442 + 754 + 626); // reviewedOf = merged + closed + commented + manual + // Own-ledger flat fallback + Orb fleet flat credit (Orb has no per-PR effort metadata in this module). + expect(out.totals.minutesSaved).toBe(2742 * MINUTES_SAVED_PER_PR + 80 * MINUTES_SAVED_PER_PR); + }); + + it("keeps own-ledger per-PR effort sum separate from Orb fleet flat credit", async () => { + const withOrbAndEffort = (sql: string): Row[] => { + if (sql.includes("orb_pr_outcomes")) return [{ merged: 10, closed: 5, total: 15 }]; + if (isEffort(sql)) return [{ totalMinutes: 100 }]; + return ledger(sql); + }; + const out = await getPublicStats(stubEnv(withOrbAndEffort), NOW); + expect(out.totals.minutesSaved).toBe(100 + 15 * MINUTES_SAVED_PER_PR); }); it("does not exclude any account from the Orb aggregate (own-ledger side is a frozen snapshot, not live-overlapping)", async () => { @@ -322,9 +382,8 @@ describe("getPublicStats — live aggregate over the review ledger", () => { expect(out.totals.accuracyPct).toBe(100); }); - // #1955: end-to-end over REAL D1/SQLite (not the stub) — a published row's `metadata_json.reviewEffortMinutes` - // (the exact shape processors.ts writes at publish time) round-trips through json_extract/AVG into - // minutesSaved, proving the SQL itself (not just the mocked shape) computes the real per-PR average. + // #1955/#2070: end-to-end over REAL D1/SQLite — published reviewEffortMinutes round-trip through + // json_extract/SUM(COALESCE(...)) into minutesSaved. it("averages a real reviewEffortMinutes value out of metadata_json via json_extract (real D1)", async () => { const env = createTestEnv({ GITTENSORY_PUBLIC_STATS_REPOS: "JSONbored/gittensory" }); const db = env.DB; @@ -370,7 +429,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { const out = await getPublicStats(env, NOW); - // avg(4, 96) = 50; reviewed = 2 -> minutesSaved = 100 (not 2 * MINUTES_SAVED_PER_PR = 40). + // sum(4, 96) = 100; reviewed = 2 -> minutesSaved = 100 (not 2 * MINUTES_SAVED_PER_PR = 40). expect(out.totals.reviewed).toBe(2); expect(out.totals.minutesSaved).toBe(100); expect(out.totals.minutesSaved).not.toBe(2 * MINUTES_SAVED_PER_PR); @@ -432,7 +491,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { const out = await getPublicStats(env, NOW); expect(out.totals.reviewed).toBe(2); - // Per-PR average: avg(avg(100, 100, 100), 1) = 50.5; reviewed = 2 -> 101. + // Per-PR sum: 100 + 1 = 101 (deduped republish events averaged per PR first). // A raw event-level average would skew this to round(2 * avg(100, 100, 100, 1)) = 151. expect(out.totals.minutesSaved).toBe(101); });