From d34cd41216fd9d39c7c2f27dfa0ee61a3f53273c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 01:54:55 -0700 Subject: [PATCH] fix(stats): measure accuracy over auto-actioned PRs, not every reviewed PR Live after both #9718 and #9768 deployed: byProject still published 100% for all three repos, on 2377/602/508 reviewed PRs with zero reversals. Neither earlier fix could have caught it. Both addressed the WINDOW the denominator covers -- #9718 withheld the ratio where a reversal was unobservable, #9768 bounded an immortal denominator to its numerator's retention -- and the defect is WHICH PRs belong in it. A reversal is by definition a human overturning an engine auto-action; recordReversalSignals only records one against a PR the engine merged or closed. The denominator was `github_app.pr_public_surface_published`: every PR that got a review surface, including the many the engine only commented on, held, or advised. None of those could produce a reversal, so each one pushes 1 - reversed/decided toward 100% for reasons unrelated to gate quality. Both surfaces now divide by the distinct PRs this deployment auto-actioned in the window -- same event types, same outcome filter, same dry-run exclusion that loadReversalDayRows already applies when anchoring the numerator, so the two halves of the ratio are finally drawn from one population. The weekly trend had the same mismatch, masked only because its own-ledger series currently reports null for recent weeks; fixing one and not the other would have left two definitions of "decided" on one page. Volume columns are untouched: reviewed/merged/closed keep publishing lifetime and fleet-folded counts, which are measured and correct. Only the ratio's denominator narrows. #9718's observability probe is REMOVED rather than left beside the thing that supersedes it. No auto-actions now means decided is 0 and the answer is null by construction -- a structural guarantee, where the probe was a heuristic that could disagree with the denominator sitting next to it. Six fixtures were made faithful rather than the assertion relaxed: each modelled a decided PR with no auto-action, which is not a reachable state. The clearest is the revert-PR regression, which recorded a reversal_reverted with no agent.action.merge -- a PR cannot be reverted unless the engine merged it. Closes #9792 --- src/review/public-stats.ts | 91 +++++++++---------- src/services/public-accuracy-trend.ts | 86 +++++++++++------- test/integration/public-stats-route.test.ts | 10 +++ test/unit/public-accuracy-trend.test.ts | 32 ++++--- test/unit/public-stats.test.ts | 97 +++++++++++++++------ 5 files changed, 195 insertions(+), 121 deletions(-) diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 74a5724c88..c866744034 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -165,47 +165,19 @@ function filteredPct(reviewed: number, merged: number): number | null { return Math.round(((reviewed - merged) / reviewed) * 1000) / 10; } -/** The auto-actions a reversal is recorded against. `recordReversalSignals` only ever fires for a PR this - * deployment itself auto-merged or auto-closed, so with zero of these rows in the retained window a - * `reversal_*` event cannot exist — and `1 - 0/N` is then a structural zero, not a measurement. */ -export const AUTO_ACTION_EVENT_TYPES = ["agent.action.close", "agent.action.merge"] as const; - -/** - * Whether a reversal is OBSERVABLE on this deployment: did it record any terminal auto-action in the window a - * reversal could still be attributed to? Reversal signals are written only by the review-execution pipeline - * (`recordReversalSignals`, reached via the `github-webhook` job), which a runtime that does not execute - * reviews acks-and-drops — so on such a runtime `reversed` is pinned at 0 forever while the merged/closed - * denominator keeps growing, and every reversal-grounded percentage converges on a fake 100%. +/** Reversal-grounded accuracy over the AUTO-ACTIONED merged + closed; null until there is signal. * - * Publishing that as accuracy is the exact failure #8820 called out for the fleet number ("the old formula - * overstated accuracy") and #7449 fixed for the global one. This is the same discipline the rest of this - * surface already applies — a sparse rule's precision is null, `gamingFlagsCaught` is null below three - * instances — extended to the case where the numerator's WRITER, not just its sample, is absent. - */ -export async function loadReversalObservability(env: Env, knownReversals = 0): Promise { - // A recorded reversal is itself proof the pipeline that records them runs here -- no probe needed, and no - // query on the hot path for any deployment that has ever overturned an auto-action. - if (knownReversals > 0) return true; - const rows = await safeAll<{ n: number }>( - env, - `SELECT COUNT(*) AS n FROM audit_events - WHERE event_type IN (${AUTO_ACTION_EVENT_TYPES.map((type) => `'${type}'`).join(", ")}) - AND created_at >= ?`, - retentionCutoffIsoForTable("audit_events"), - ); - return (rows[0]?.n ?? 0) > 0; -} - -/** Reversal-grounded accuracy over the irreversible auto-actions (merged + closed); null until there is signal, - * and null when a reversal is not observable at all on this deployment (see {@link loadReversalObservability}) — - * an unmeasurable quantity stays unknown rather than rendering as a perfect score. */ + * `merged`/`closed` must come from the auto-action denominator (#9792), never the published-surface counts: + * a reversal only ever exists for a PR the engine itself merged or closed, so a wider denominator counts PRs + * whose reversal was never possible and drifts the ratio toward 100%. That also makes the separate + * observability probe #9718 added redundant and it has been removed — no auto-actions means `decided` is 0 + * and the answer is null by construction, which is a structural guarantee rather than a heuristic that + * could disagree with the denominator beside it. */ function accuracyPct( merged: number, closed: number, reversed: number, - reversalObservable: boolean, ): number | null { - if (!reversalObservable) return null; const decided = merged + closed; if (decided <= 0) return null; // `reversed` counts engine auto-actions regardless of a PR's CURRENT disposition, so a reopened @@ -378,22 +350,39 @@ export async function getPublicStats( GROUP BY ev.repo`, ...projects, ), - // The ACCURACY denominator, bounded to the same window its numerator can survive in. `reviewed`/ - // `merged`/`closed` above are lifetime: `github_app.pr_public_surface_published` is the single - // retention-EXEMPT audit event type (src/db/retention.ts's DURABLE_AUDIT_EVENT_TYPES), so those - // counts never shrink. `reversal_*` rows are pruned with the rest of audit_events at 90 days. Pairing - // an immortal denominator with a 90-day numerator makes `1 - reversed/decided` drift toward 100% as - // the ledger ages, independent of real reversal behavior -- the same shape #7449 fixed for the - // Orb-folded denominator, and confirmed live (2377/602/508 reviewed, 0 reversals, 100% each). + // The ACCURACY denominator: the PRs this deployment AUTO-ACTIONED in the window, not the PRs it + // published a surface for. + // + // A reversal is by definition a human overturning an engine auto-action -- outcomes-wire.ts only records + // one against a PR the engine merged or closed. So the population the numerator can draw from is + // auto-actions, and any denominator wider than that counts PRs whose reversal was never possible. + // `github_app.pr_public_surface_published` is wider in two independent ways: it is the single + // retention-EXEMPT event type (retention.ts's DURABLE_AUDIT_EVENT_TYPES) so it never shrinks, and it + // covers every reviewed PR including the ones the engine only commented on. #9768 fixed the first by + // windowing it; this fixes the second, which was why the published figure was STILL 100% for all three + // repos on 2377/602/508 reviewed with 0 reversals after that shipped. + // + // This is the pairing public-accuracy-trend.ts's loadReversalDayRows already uses for the weekly series + // -- it anchors reversals on `agent.action.*` rows, dry-runs excluded -- so the two surfaces now agree on + // what "decided" means instead of each choosing its own denominator. safeAll<{ project: string; merged: number; closed: number }>( env, - `SELECT ev.repo AS project, + `SELECT act.project AS project, 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 - FROM (SELECT DISTINCT repo, number FROM (${PUBLISHED_PR_KEYS}) WHERE created_at >= ?) 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`, + FROM ( + SELECT DISTINCT substr(target_key, 1, instr(target_key, '#') - 1) AS project, + CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS pr_number + FROM audit_events + WHERE event_type IN ('agent.action.close', 'agent.action.merge') + AND outcome = 'completed' AND instr(target_key, '#') > 0 + AND length(target_key) - length(replace(target_key, '#', '')) = 1 + AND COALESCE(json_extract(metadata_json, '$.mode'), 'live') <> 'dry_run' + AND created_at >= ? + ) act + LEFT JOIN pull_requests pr ON pr.repo_full_name = act.project AND pr.number = act.pr_number + WHERE LOWER(act.project) IN (${inList}) + GROUP BY act.project`, retentionCutoffIsoForTable("audit_events"), ...projects, ), @@ -484,9 +473,7 @@ export async function getPublicStats( }; // Resolved before byProject so every published accuracy figure -- per repo and global -- answers the same // question about the same deployment, rather than one of them silently disagreeing with the others. - const totalReversals = [...reversedByProject.values()].reduce((sum, n) => sum + n, 0); - const reversalObservable = await loadReversalObservability(env, totalReversals); - // project -> the retention-windowed merged/closed pairing for `reversed`. See the query's own comment. + // project -> the auto-actioned merged/closed pairing for `reversed`. See the query's own comment. const windowedByProject = new Map(windowedDispositions.map((row) => [String(row.project).toLowerCase(), row])); let windowedMerged = 0; let windowedClosed = 0; @@ -514,7 +501,7 @@ export async function getPublicStats( reviewed, merged, closed, - accuracyPct: accuracyPct(windowedRepoMerged, windowedRepoClosed, reversed, reversalObservable), + accuracyPct: accuracyPct(windowedRepoMerged, windowedRepoClosed, reversed), }; }) .filter((r) => r.reviewed > 0) @@ -643,7 +630,7 @@ export async function getPublicStats( // byProject fold above), never the fleet-folded lifetime totals.merged/closed, so the numerator // (own-ledger `reversed`) and the denominator are drawn from the same population AND the same // retention window. See the windowed disposition query's own comment for both halves of that pairing. - accuracyPct: accuracyPct(windowedMerged, windowedClosed, totals.reversed, reversalObservable), + accuracyPct: accuracyPct(windowedMerged, windowedClosed, totals.reversed), minutesSaved, }, weekly: { reviewed: w.reviewed ?? 0, merged: w.merged ?? 0 }, diff --git a/src/services/public-accuracy-trend.ts b/src/services/public-accuracy-trend.ts index 9ba57b15d8..753e6571fc 100644 --- a/src/services/public-accuracy-trend.ts +++ b/src/services/public-accuracy-trend.ts @@ -8,7 +8,7 @@ // #2568 pattern for the sibling per-repo quality trend) can recompute any historical week correctly on every // request -- no cron-miss gap risk, no second copy of the number to keep in sync, and the SAME formula as the // live figure by construction, so the two can never silently diverge or read as inconsistent to a public viewer. -import { loadReversalObservability, PUBLISHED_PR_KEYS, publicStatsProjects, safeAll } from "../review/public-stats"; +import { PUBLISHED_PR_KEYS, publicStatsProjects, safeAll } from "../review/public-stats"; import { isoWeekStart } from "./public-quality-metrics"; export const PUBLIC_ACCURACY_TREND_WEEKS = 8; @@ -24,13 +24,16 @@ export type PublicAccuracyTrendWeek = { accuracyPct: number | null; }; -/** `merged`/`closed` are the DISPLAYED volume (own ledger + registered Orb fleet). `ownMerged`/`ownClosed` are - * the own-ledger-only pairing for `reversed`, which is own-ledger-only by construction (the Orb aggregate has - * no reversal concept). Accuracy divides the own-ledger numbers ONLY -- #7449 fixed exactly this asymmetry for - * the lifetime figure in public-stats.ts and it was never carried across to this trend, so the denominator grew - * with every newly registered install while the numerator stayed own-ledger-scoped, trending every week toward - * 100% independent of real reversal behavior. */ -type DayRow = { day: string; merged: number; closed: number; ownMerged: number; ownClosed: number; reversed: number }; +/** `merged`/`closed` are the DISPLAYED volume (own ledger + registered Orb fleet). `autoActioned` is the + * ACCURACY denominator: the PRs this deployment actually merged or closed itself that day. + * + * Two separate reasons it cannot be the displayed volume. #7449: the Orb aggregate has no reversal concept, so + * folding it in grows the denominator with every newly registered install while `reversed` stays own-ledger + * scoped. And #9792: `reversed` is only ever recorded against an engine AUTO-ACTION (loadReversalDayRows + * anchors on `agent.action.*`), so even the own-ledger published-PR count is wider than the population the + * numerator can draw from -- it includes every PR the engine merely commented on. Both make the ratio drift + * toward 100% for reasons that have nothing to do with reversal behavior. */ +type DayRow = { day: string; merged: number; closed: number; autoActioned: number; reversed: number }; const MS_PER_WEEK = 7 * 86_400_000; @@ -39,21 +42,17 @@ function roundPct(value: number): number { } /** Same formula as public-stats.ts's accuracyPct, reused so the trend and the live number can never drift - * apart into two competing definitions of "accuracy". `reversalObservable` false means this deployment records - * no terminal auto-actions for a reversal to be attributed to, so the week's accuracy is unknown, not perfect -- - * the volume columns still publish, since those ARE measured. */ -function publicBucketOf( - bucket: { merged: number; closed: number; ownMerged: number; ownClosed: number; reversed: number }, - reversalObservable: boolean, -): Omit { + * apart into two competing definitions of "accuracy". The volume columns still publish when accuracy cannot: + * those ARE measured. */ +function publicBucketOf(bucket: { merged: number; closed: number; autoActioned: number; reversed: number }): Omit { const decided = bucket.merged + bucket.closed; if (decided < MIN_ACCURACY_TREND_SAMPLE) return { merged: null, closed: null, reversed: null, accuracyPct: null }; const observed = { merged: bucket.merged, closed: bucket.closed, reversed: bucket.reversed }; - if (!reversalObservable) return { ...observed, accuracyPct: null }; - // Own-ledger-only denominator: `reversed` can only ever be attributed to own-ledger PRs (see DayRow). - const ownDecided = bucket.ownMerged + bucket.ownClosed; - if (ownDecided <= 0) return { ...observed, accuracyPct: null }; - const reversalRate = Math.min(1, bucket.reversed / ownDecided); + // No auto-actions that week means no PR a reversal could have been recorded against, so accuracy is + // unknown rather than perfect. This replaces the separate observability probe (#9718): the denominator IS + // the signal now, and one structural guarantee beats a heuristic that could disagree with it. + if (bucket.autoActioned <= 0) return { ...observed, accuracyPct: null }; + const reversalRate = Math.min(1, bucket.reversed / bucket.autoActioned); return { ...observed, accuracyPct: roundPct(1 - reversalRate) }; } @@ -63,11 +62,10 @@ export function buildPublicAccuracyTrend( dayRows: DayRow[], nowMs: number, weeks: number = PUBLIC_ACCURACY_TREND_WEEKS, - reversalObservable = true, ): PublicAccuracyTrendWeek[] { const currentStartMs = Date.parse(isoWeekStart(nowMs)); const oldestStartMs = currentStartMs - (weeks - 1) * MS_PER_WEEK; - const buckets = Array.from({ length: weeks }, () => ({ merged: 0, closed: 0, ownMerged: 0, ownClosed: 0, reversed: 0 })); + const buckets = Array.from({ length: weeks }, () => ({ merged: 0, closed: 0, autoActioned: 0, reversed: 0 })); for (const row of dayRows) { const dayMs = Date.parse(`${row.day}T00:00:00.000Z`); @@ -77,14 +75,13 @@ export function buildPublicAccuracyTrend( const bucket = buckets[weekOffset]!; bucket.merged += row.merged; bucket.closed += row.closed; - bucket.ownMerged += row.ownMerged; - bucket.ownClosed += row.ownClosed; + bucket.autoActioned += row.autoActioned; bucket.reversed += row.reversed; } return buckets.map((bucket, offset) => ({ weekStart: isoWeekStart(oldestStartMs + offset * MS_PER_WEEK), - ...publicBucketOf(bucket, reversalObservable), + ...publicBucketOf(bucket), })); } @@ -122,6 +119,34 @@ async function loadOwnLedgerDayRows(env: Env, projects: string[], sinceIso: stri return map; } +/** Distinct PRs this deployment AUTO-ACTIONED per day -- the accuracy denominator. Deliberately the same + * population, filters and dry-run exclusion as loadReversalDayRows' inner `orig` query below, because the + * numerator is drawn from exactly these rows; any divergence between the two would silently reintroduce a + * ratio over mismatched populations. */ +async function loadAutoActionDayRows(env: Env, projects: string[], sinceIso: string): Promise> { + const map = new Map(); + if (projects.length === 0) return map; + const inList = projects.map(() => "?").join(", "); + const rows = await safeAll<{ day: string; n: number }>( + env, + `SELECT date(act.created_at) AS day, COUNT(DISTINCT act.target_key) AS n FROM ( + SELECT substr(target_key, 1, instr(target_key, '#') - 1) AS project, target_key, created_at + FROM audit_events + WHERE event_type IN ('agent.action.close', 'agent.action.merge') + AND outcome = 'completed' AND instr(target_key, '#') > 0 + AND length(target_key) - length(replace(target_key, '#', '')) = 1 + AND COALESCE(json_extract(metadata_json, '$.mode'), 'live') <> 'dry_run' + AND created_at >= ? + ) act + WHERE LOWER(act.project) IN (${inList}) + GROUP BY day`, + sinceIso, + ...projects, + ); + for (const row of rows) map.set(row.day, row.n); + return map; +} + /** Day-bucketed reversal count, bucketed by the ORIGINAL auto-action's own created_at (not the later reversal's * timestamp) so a reversal always credits the week the decision was actually made, and never retroactively * shifts a past week's published trend. Detection matches public-stats.ts's `reversalRows` fix (#fairness- @@ -192,22 +217,21 @@ export async function loadPublicAccuracyTrend(env: Env, nowMs: number = Date.now const projects = publicStatsProjects(env); const sinceIso = new Date(Date.parse(isoWeekStart(nowMs)) - (PUBLIC_ACCURACY_TREND_WEEKS - 1) * MS_PER_WEEK).toISOString(); - const [ownLedger, reversals, orb, reversalObservable] = await Promise.all([ + const [ownLedger, reversals, orb, autoActions] = await Promise.all([ loadOwnLedgerDayRows(env, projects, sinceIso), loadReversalDayRows(env, projects, sinceIso), loadOrbDayRows(env, sinceIso), - loadReversalObservability(env), + loadAutoActionDayRows(env, projects, sinceIso), ]); - const days = new Set([...ownLedger.keys(), ...reversals.keys(), ...orb.keys()]); + const days = new Set([...ownLedger.keys(), ...reversals.keys(), ...orb.keys(), ...autoActions.keys()]); const dayRows: DayRow[] = [...days].map((day) => ({ day, merged: (ownLedger.get(day)?.merged ?? 0) + (orb.get(day)?.merged ?? 0), closed: (ownLedger.get(day)?.closed ?? 0) + (orb.get(day)?.closed ?? 0), - ownMerged: ownLedger.get(day)?.merged ?? 0, - ownClosed: ownLedger.get(day)?.closed ?? 0, + autoActioned: autoActions.get(day) ?? 0, reversed: reversals.get(day) ?? 0, })); - return buildPublicAccuracyTrend(dayRows, nowMs, PUBLIC_ACCURACY_TREND_WEEKS, reversalObservable); + return buildPublicAccuracyTrend(dayRows, nowMs); } diff --git a/test/integration/public-stats-route.test.ts b/test/integration/public-stats-route.test.ts index 8685728993..2060fef730 100644 --- a/test/integration/public-stats-route.test.ts +++ b/test/integration/public-stats-route.test.ts @@ -24,6 +24,16 @@ async function seed(env: Env) { ) .bind(`ae-${repo}-${number}`, `${repo}#${number}`) .run(); + // #9792: the accuracy denominator reads engine AUTO-ACTIONS, not published surfaces, so a decided PR + // needs the action that decided it. The still-open one (loopover#3) gets its own agent.action.close + // below, since it was auto-closed and then reopened -- that is what makes it a reversal. + if (state === "closed") { + await env.DB.prepare( + `INSERT INTO audit_events (id, event_type, target_key, outcome) VALUES (?, ?, ?, 'completed')`, + ) + .bind(`act-${repo}-${number}`, mergedAt ? "agent.action.merge" : "agent.action.close", `${repo}#${number}`) + .run(); + } await env.DB.prepare( `INSERT INTO pull_requests (id, repo_full_name, number, title, state, merged_at) VALUES (?, ?, ?, ?, ?, ?)`, ) diff --git a/test/unit/public-accuracy-trend.test.ts b/test/unit/public-accuracy-trend.test.ts index 9fed995593..df707d6676 100644 --- a/test/unit/public-accuracy-trend.test.ts +++ b/test/unit/public-accuracy-trend.test.ts @@ -11,11 +11,12 @@ import { createTestEnv } from "../helpers/d1"; const NOW = Date.parse("2026-06-22T12:00:00.000Z"); -/** A day row whose volume is entirely own-ledger (no registered-Orb fold), which is what every case below - * models -- so `ownMerged`/`ownClosed` mirror `merged`/`closed` and the accuracy denominator is unchanged. - * The Orb-fold asymmetry gets its own explicit cases further down. */ +/** A day row whose volume was entirely auto-actioned by this deployment (no registered-Orb fold, nothing + * merely commented on), which is what most cases below model -- so `autoActioned` equals the decided volume + * and the accuracy denominator is the whole of it. The cases where the denominator is deliberately NARROWER + * than the displayed volume set `autoActioned` explicitly. */ function row(day: string, merged: number, closed: number, reversed: number) { - return { day, merged, closed, ownMerged: merged, ownClosed: closed, reversed }; + return { day, merged, closed, autoActioned: merged + closed, reversed }; } describe("buildPublicAccuracyTrend", () => { @@ -99,7 +100,7 @@ describe("buildPublicAccuracyTrend", () => { // stays own-ledger-only trends every week toward 100% as more installs register. const week = isoWeekStart(NOW); const trend = buildPublicAccuracyTrend( - [{ day: week, merged: 100, closed: 0, ownMerged: 4, ownClosed: 0, reversed: 1 }], + [{ day: week, merged: 100, closed: 0, autoActioned: 4, reversed: 1 }], NOW, 1, ); @@ -107,18 +108,29 @@ describe("buildPublicAccuracyTrend", () => { expect(trend[0]).toEqual({ weekStart: week, merged: 100, closed: 0, reversed: 1, accuracyPct: 75 }); }); - it("reports null accuracy — never 100% — when a reversal is not observable on this deployment", () => { - // Regression: on a runtime that does not execute reviews, `reversed` is pinned at 0 forever, so every week - // rendered as a perfect 100% over rising volume. The volume itself IS measured and still publishes. + it("REGRESSION: reports null accuracy — never 100% — for a week with volume but no auto-actions", () => { + // On a runtime that does not execute reviews, `reversed` is pinned at 0 forever, so every week rendered + // as a perfect 100% over rising volume. This is now structural rather than a separate observability + // probe (#9792): no auto-action means no PR a reversal could attach to, so the denominator is 0 and the + // ratio is unknown. The volume itself IS measured and still publishes. const week = isoWeekStart(NOW); - const trend = buildPublicAccuracyTrend([row(week, 40, 10, 0)], NOW, 1, false); + const trend = buildPublicAccuracyTrend([{ day: week, merged: 40, closed: 10, autoActioned: 0, reversed: 0 }], NOW, 1); expect(trend[0]).toEqual({ weekStart: week, merged: 40, closed: 10, reversed: 0, accuracyPct: null }); }); + it("REGRESSION: divides by AUTO-ACTIONS, not by every reviewed PR", () => { + // A week where the engine published a surface for 100 PRs but only auto-actioned 4 of them, one of which + // was reversed. 1 - 1/4 = 75%, not the 1 - 1/100 = 99% a published-PR denominator would have reported -- + // the defect that kept byProject at 100% across 2377/602/508 reviewed PRs with zero reversals. + const week = isoWeekStart(NOW); + const trend = buildPublicAccuracyTrend([{ day: week, merged: 100, closed: 0, autoActioned: 4, reversed: 1 }], NOW, 1); + expect(trend[0]?.accuracyPct).toBe(75); + }); + it("reports null accuracy when a week's volume is entirely Orb-folded (no own-ledger PRs to attribute a reversal to)", () => { const week = isoWeekStart(NOW); const trend = buildPublicAccuracyTrend( - [{ day: week, merged: 50, closed: 10, ownMerged: 0, ownClosed: 0, reversed: 0 }], + [{ day: week, merged: 50, closed: 10, autoActioned: 0, reversed: 0 }], NOW, 1, ); diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index 053ad1f494..217c052636 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -6,7 +6,6 @@ import { getPublicStats, isPublicStatsEnabled, MINUTES_SAVED_PER_PR, - loadReversalObservability, resolvePublicStatsManifestOverride, } from "../../src/review/public-stats"; import { recordAuditEvent, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; @@ -59,6 +58,14 @@ function isDispositions(sql: string): boolean { function isReversal(sql: string): boolean { return sql.includes("reversal_reopened"); } +// #9792: the ACCURACY denominator reads engine auto-actions (agent.action.close/merge), a strictly narrower +// population than the published surfaces isDispositions serves -- a reversal can only exist for a PR the +// engine actually merged or closed. Every fixture below returns the same merged/closed here as it does for +// dispositions, i.e. it models a world where every decided PR was auto-actioned; the tests that need those +// two populations to DIFFER say so explicitly. +function isAutoAction(sql: string): boolean { + return sql.includes("agent.action.close") && !isReversal(sql); +} describe("isPublicStatsEnabled", () => { it("is truthy only for 1/true/yes/on (case-insensitive)", () => { @@ -174,6 +181,13 @@ describe("getPublicStats — live aggregate over the review ledger", () => { }, ]; } + if (isAutoAction(sql)) { + return [ + { project: "JSONbored/awesome-claude", merged: 1231, closed: 524 }, + { project: "JSONbored/metagraphed", merged: 137, closed: 176 }, + { project: "JSONbored/loopover", merged: 24, closed: 24 }, + ]; + } if (isReversal(sql)) { return [ { project: "JSONbored/awesome-claude", reversed: 20 }, @@ -337,6 +351,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { // (6000 merged + 4000 closed, with no reversal data at all) must NOT dilute the denominator toward 100. const handler = (sql: string): Row[] => { if (isDispositions(sql)) return [{ project: "JSONbored/loopover", reviewed: 100, merged: 100, closed: 0, inReview: 0 }]; + if (isAutoAction(sql)) return [{ project: "JSONbored/loopover", merged: 100, closed: 0 }]; if (isReversal(sql)) return [{ project: "JSONbored/loopover", reversed: 10 }]; if (sql.includes("orb_pr_outcomes")) return [{ merged: 6000, closed: 4000, total: 10000 }]; return []; @@ -382,6 +397,9 @@ describe("getPublicStats — live aggregate over the review ledger", () => { // of merged+closed but still counted as reversals). decided=1, reversed=2 → an unclamped 1 - 2/1 = -100%. const handler = (sql: string): Row[] => { if (isDispositions(sql)) return [{ project: "JSONbored/loopover", reviewed: 3, merged: 1, closed: 0, inReview: 2 }]; + // All three were auto-actioned; the two reopened ones are now `open`, so they fall out of merged+closed + // while still counting as reversals -- which is exactly what makes the ratio exceed 1 and need clamping. + if (isAutoAction(sql)) return [{ project: "JSONbored/loopover", merged: 1, closed: 0 }]; if (isReversal(sql)) return [{ project: "JSONbored/loopover", reversed: 2 }]; return []; }; @@ -467,7 +485,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { await db .prepare( `INSERT INTO audit_events (id, event_type, target_key, outcome, metadata_json) - VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)`, ) .bind( "published", @@ -475,6 +493,14 @@ describe("getPublicStats — live aggregate over the review ledger", () => { "JSONbored/loopover#1", "completed", "{}", + // The engine's own merge. A PR cannot be REVERTED unless the engine merged it, so a fixture with a + // reversal and no auto-action does not describe a reachable state -- and #9792 made the accuracy + // denominator read exactly these rows. + "auto-merged", + "agent.action.merge", + "JSONbored/loopover#1", + "completed", + JSON.stringify({ repoFullName: "JSONbored/loopover", actionClass: "merge" }), "reverted", "reversal_reverted", "JSONbored/loopover#1", @@ -737,6 +763,39 @@ describe("getPublicStats — live aggregate over the review ledger", () => { expect(out.byProject.map((row) => row.accuracyPct)).toEqual([null]); }); + it("REGRESSION: a repo with reviewed PRs but no AUTO-ACTIONS publishes null accuracy, not 100% (real D1)", async () => { + // The live defect #9792 fixes. After #9718's observability gate and #9768's retention window both + // shipped, production still published 100% for all three repos on 2377/602/508 reviewed PRs with zero + // reversals -- because the denominator counted every PR that got a review surface, including the many + // the engine only commented on. A reversal can only exist for a PR the engine merged or closed, so a + // repo the engine never auto-actioned has no measurable accuracy at all. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/loopover" }); + await upsertRepositoryFromGitHub(env, { name: "loopover", full_name: "JSONbored/loopover", private: false, owner: { login: "JSONbored" } }, 1); + for (const number of [1, 2, 3]) { + await upsertPullRequestFromGitHub(env, "JSONbored/loopover", { number, title: `pr ${number}`, state: "closed", merged_at: new Date(NOW - 86_400_000).toISOString(), user: { login: "a" }, head: { sha: `s${number}` }, labels: [] }); + await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: `JSONbored/loopover#${number}`, outcome: "completed" }); + } + + const out = await getPublicStats(env, NOW); + // The volume is real and still published -- only the ratio is withheld. + expect(out.totals.reviewed).toBe(3); + expect(out.totals.merged).toBe(3); + expect(out.byProject[0]?.accuracyPct).toBeNull(); + expect(out.totals.accuracyPct).toBeNull(); + }); + + it("REGRESSION: a dry-run auto-action never counts toward the denominator (real D1)", async () => { + // loadReversalDayRows already excludes dry-runs from the numerator's anchor; the denominator has to + // agree or a dry-run would inflate it and drag accuracy toward 100%. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/loopover" }); + await upsertRepositoryFromGitHub(env, { name: "loopover", full_name: "JSONbored/loopover", private: false, owner: { login: "JSONbored" } }, 1); + await upsertPullRequestFromGitHub(env, "JSONbored/loopover", { number: 1, title: "dry", state: "closed", merged_at: new Date(NOW - 86_400_000).toISOString(), user: { login: "a" }, head: { sha: "s1" }, labels: [] }); + await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: "JSONbored/loopover#1", outcome: "completed" }); + await recordAuditEvent(env, { eventType: "agent.action.merge", targetKey: "JSONbored/loopover#1", outcome: "completed", metadata: { mode: "dry_run" } }); + + expect((await getPublicStats(env, NOW)).byProject[0]?.accuracyPct).toBeNull(); + }); + it("REGRESSION: the accuracy denominator is bounded to audit_events' retention window, not lifetime (real D1)", async () => { // `github_app.pr_public_surface_published` is the ONE retention-exempt event type, so reviewed/merged/ // closed are immortal, while `reversal_*` rows prune at 90 days. Pairing them made 1 - reversed/decided @@ -751,6 +810,9 @@ describe("getPublicStats — live aggregate over the review ledger", () => { for (const number of [1, 2, 3, 4]) { await upsertPullRequestFromGitHub(env, "JSONbored/loopover", { number, title: `old ${number}`, state: "closed", merged_at: old, user: { login: "a" }, head: { sha: `s${number}` }, labels: [] }); await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: `JSONbored/loopover#${number}`, outcome: "completed", createdAt: old }); + // Auto-actioned back then too -- so this test proves the WINDOW excludes them, not merely that they + // were never auto-actioned. + await recordAuditEvent(env, { eventType: "agent.action.merge", targetKey: `JSONbored/loopover#${number}`, outcome: "completed", createdAt: old }); } // One recent auto-closed PR, reversed by a human. await upsertPullRequestFromGitHub(env, "JSONbored/loopover", { number: 5, title: "recent", state: "open", user: { login: "b" }, head: { sha: "s5" }, labels: [] }); @@ -761,19 +823,21 @@ describe("getPublicStats — live aggregate over the review ledger", () => { // PR#5 which is currently `open` and so counts as inReview, not decided. await upsertPullRequestFromGitHub(env, "JSONbored/loopover", { number: 6, title: "recent ok", state: "closed", merged_at: recent, user: { login: "c" }, head: { sha: "s6" }, labels: [] }); await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: "JSONbored/loopover#6", outcome: "completed", createdAt: recent }); + await recordAuditEvent(env, { eventType: "agent.action.merge", targetKey: "JSONbored/loopover#6", outcome: "completed", createdAt: recent }); const out = await getPublicStats(env, NOW); // Lifetime volume still reports every PR ever published -- that part is measured and unaffected. expect(out.totals.reviewed).toBe(6); expect(out.totals.merged).toBe(5); expect(out.totals.reversed).toBe(1); - // Accuracy divides by the WINDOWED denominator (PR#6 merged inside the window), not the lifetime 5. - // 1 - 1/1 = 0%, not the 1 - 1/5 = 80% the old lifetime pairing would have published. + // Accuracy divides by the windowed AUTO-ACTION denominator: PR#6 is the only auto-action inside the + // window that still reads as merged/closed (PR#5 was reopened, so it is `open` and falls out), against + // PR#5's reversal. 1 - 1/1 = 0%, not the 1 - 1/5 = 80% a lifetime published-PR pairing would publish. expect(out.byProject[0]?.accuracyPct).toBe(0); expect(out.totals.accuracyPct).toBe(0); }); - it("publishes a real accuracy once the deployment records the auto-actions a reversal attaches to (real D1)", async () => { + it("publishes a real accuracy once the deployment records the auto-action a reversal attaches to (real D1)", async () => { const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/loopover" }); await upsertRepositoryFromGitHub(env, { name: "loopover", full_name: "JSONbored/loopover", private: false, owner: { login: "JSONbored" } }, 1); await upsertPullRequestFromGitHub(env, "JSONbored/loopover", { number: 1, title: "PR 1", state: "closed", merged_at: "2026-06-20T09:00:00.000Z", user: { login: "a" }, head: { sha: "s1" }, labels: [] }); @@ -981,29 +1045,6 @@ describe("getPublicStats — live aggregate over the review ledger", () => { }); }); -describe("loadReversalObservability", () => { - it("short-circuits to true on a known reversal without querying at all", async () => { - const env = { DB: { prepare: () => { throw new Error("must not query"); } } } as unknown as Env; - expect(await loadReversalObservability(env, 1)).toBe(true); - }); - - it("is false on a deployment that has recorded no terminal auto-action", async () => { - expect(await loadReversalObservability(createTestEnv(), 0)).toBe(false); - }); - - it("is true once a terminal auto-action exists in the retained window", async () => { - const env = createTestEnv(); - await recordAuditEvent(env, { eventType: "agent.action.close", targetKey: "JSONbored/loopover#3", outcome: "completed" }); - expect(await loadReversalObservability(env, 0)).toBe(true); - }); - - it("degrades to false (never throws) when the probe query itself fails", async () => { - const broken = createTestEnv(); - broken.DB = { prepare: () => { throw new Error("boom"); } } as never; - expect(await loadReversalObservability(broken, 0)).toBe(false); - }); -}); - describe("fleetAccuracy.guaranteed (#8835/#9121/#9050/#9068)", () => { // minimumCalibrationLabels(0.015, 0.05) = 199 -- every valid fixture below clears it with margin. const calibrated = (overrides: Record = {}) => ({