diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 74a5724c8..c86674403 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 9ba57b15d..753e6571f 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 868572899..2060fef73 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 9fed99559..df707d667 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 053ad1f49..217c05263 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 = {}) => ({