fix(stats): measure accuracy over auto-actioned PRs, not every reviewed PR (#9793)

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

Author: JSONbored

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<boolean> {
-  // 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 },

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<PublicAccuracyTrendWeek, "weekStart"> {
+ *  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<PublicAccuracyTrendWeek, "weekStart"> {
   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<Map<string, number>> {
+  const map = new Map<string, number>();
+  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);
 }

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 (?, ?, ?, ?, ?, ?)`,
     )