Skip to content

fix(review): sum per-PR review-effort minutes with fallback in public-stats (#2070) #4060

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 3 commits into from
Jul 7, 2026
Merged

fix(review): sum per-PR review-effort minutes with fallback in public-stats (#2070) #4060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64800da
fix(review): sum per-PR review-effort minutes with fallback in public…
RealDiligent Jul 7, 2026
15c6195
fix(review): keep Orb fleet minutes separate from own-ledger effort sum
RealDiligent Jul 7, 2026
d3d8874
docs(review): clarify ownLedgerReviewed is captured pre-Orb merge
RealDiligent Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 22 additions & 20 deletions src/review/public-stats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down Expand Up @@ -190,7 +191,7 @@ export async function getPublicStats(
Promise.resolve<DispositionRow[]>([]),
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<DispositionRow>(
Expand Down Expand Up @@ -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 (
Expand All @@ -267,6 +263,7 @@ export async function getPublicStats(
)
GROUP BY repo, number
)`,
MINUTES_SAVED_PER_PR,
...projects,
),
]);
Expand Down Expand Up @@ -316,18 +313,23 @@ 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;
totals.handled += orb.total;

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,
Expand All @@ -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,
Expand Down
91 changes: 75 additions & 16 deletions test/unit/public-stats.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
});
Expand Down