fix(stats): bound the accuracy denominator to the window its numerator survives in (#9768)

* fix(stats): bound the accuracy denominator to the window its numerator survives in

#9718 gated accuracy on whether a reversal is observable at all, and fixed the
weekly trend's Orb-folded denominator. It did not fix the third asymmetry in
#9676: `merged`/`closed` come from `github_app.pr_public_surface_published`,
the single retention-EXEMPT audit event type, so they are lifetime and never
shrink -- while `reversal_*` rows prune 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.

Confirmed live after #9718 deployed: byProject still published 100% for all
three repos on 2377/602/508 reviewed with 0 reversals, because the reversal
signal IS observable on that deployment -- the gate passed and the ratio was
still meaningless.

The accuracy denominator is now a second, retention-windowed disposition query
that shares BOTH of the numerator's bounds: own-ledger only, and inside
audit_events' retention window. `reviewed`/`merged`/`closed` keep publishing
lifetime volume, which is measured and correct. That display-vs-denominator
split is the same shape #7449 established for the Orb fold and #9718 carried
into the weekly trend; the #7449 snapshot it supersedes is removed rather than
left dead, with its reasoning folded into the new comment.

Boundary case, stated rather than hidden: a PR published before the window but
reversed inside it contributes to the numerator and not the denominator.
accuracyPct's existing clamp already handles that (it is the same shape as the
reopened-auto-close case its comment describes), and the alternative -- an
unbounded denominator -- is the bug being fixed.

Refs #9676

* docs(stats): retarget the global accuracyPct comment at the variables that still exist

Review catch on #9768. The comment above the global accuracyPct call site still
pointed at "the ownLedgerMerged/ownLedgerClosed snapshot above the Orb fold" --
variables this PR removed and replaced with windowedMerged/windowedClosed. A
comment referring a reader to a symbol that no longer exists is worse than none:
it sends them looking for context that was deliberately relocated.

Retargeted at the windowed disposition query, and widened while there. The old
text described ONE of the two bounds the pairing needs (same population, so the
fleet fold cannot inflate it); it now names both, since this PR added the second
(same retention window, so an immortal denominator cannot outlive its numerator).

`ownLedgerReviewed`/`ownLedgerMinutes` are untouched -- different variables, both
still live, feeding the minutes-saved calculation.

Author: JSONbored

src/review/public-stats.ts

@@ -356,9 +356,10 @@ export async function getPublicStats(
   // The own-ledger side needs at least one allowlisted project to query; an empty allowlist skips these three
   // queries entirely (own-ledger totals stay zero) but still lets the Orb aggregate below run.
   const inList = projects.map(() => "?").join(", ");
-  const [dispositions, reversalRows, weeklyRows, effortRows] = projects.length === 0
+  const [dispositions, windowedDispositions, reversalRows, weeklyRows, effortRows] = projects.length === 0
     ? await Promise.all([
         Promise.resolve<DispositionRow[]>([]),
+        Promise.resolve<{ project: string; merged: number; closed: number }[]>([]),
         Promise.resolve<{ project: string; reversed: number }[]>([]),
         Promise.resolve<{ reviewed: number; merged: number }[]>([]),
         Promise.resolve<{ totalMinutes: number | null }[]>([]),
@@ -377,6 +378,25 @@ 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).
+    safeAll<{ project: string; merged: number; closed: number }>(
+      env,
+      `SELECT ev.repo 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`,
+      retentionCutoffIsoForTable("audit_events"),
+      ...projects,
+    ),
     safeAll<{ project: string; reversed: number }>(
       env,
       // A "reversal" = a human overturning a terminal engine auto-action, already detected and recorded by
@@ -466,6 +486,10 @@ export async function getPublicStats(
   // 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.
+  const windowedByProject = new Map(windowedDispositions.map((row) => [String(row.project).toLowerCase(), row]));
+  let windowedMerged = 0;
+  let windowedClosed = 0;
   const byProject = dispositions
     .map((d) => {
       const merged = d.merged ?? 0;
@@ -474,6 +498,11 @@ export async function getPublicStats(
       const reversed =
         reversedByProject.get(String(d.project).toLowerCase()) ?? 0;
       const reviewed = merged + closed + inReview;
+      const windowed = windowedByProject.get(String(d.project).toLowerCase());
+      const windowedRepoMerged = windowed?.merged ?? 0;
+      const windowedRepoClosed = windowed?.closed ?? 0;
+      windowedMerged += windowedRepoMerged;
+      windowedClosed += windowedRepoClosed;
       totals.handled += reviewed;
       totals.merged += merged;
       totals.closed += closed;
@@ -485,7 +514,7 @@ export async function getPublicStats(
         reviewed,
         merged,
         closed,
-        accuracyPct: accuracyPct(merged, closed, reversed, reversalObservable),
+        accuracyPct: accuracyPct(windowedRepoMerged, windowedRepoClosed, reversed, reversalObservable),
       };
     })
     .filter((r) => r.reviewed > 0)
@@ -501,13 +530,13 @@ export async function getPublicStats(
   // Snapshot before Orb merge: effort SQL only covers allowlisted own-ledger publishes, while `reviewed`
   // below includes Orb fleet outcomes folded into totals.merged/closed.
   const ownLedgerReviewed = reviewedOf(totals);
-  // #7449: also snapshot the pre-fold own-ledger merged/closed. totals.reversed stays own-ledger-only (the Orb
-  // aggregate has no reversal concept), so the published global accuracyPct below is computed from THESE, not the
-  // fleet-folded totals.merged/closed -- otherwise the denominator would grow with every newly registered install
-  // while the numerator stayed own-ledger-scoped, trending the percentage toward 100 independent of real reversal
-  // behavior. The fleet fold still (correctly) inflates reviewed/handled/minutesSaved, which have no such pairing.
-  const ownLedgerMerged = totals.merged;
-  const ownLedgerClosed = totals.closed;
+  // #7449 (superseded, same reasoning): the published global accuracyPct is NOT computed from
+  // totals.merged/closed. Those are lifetime AND fleet-folded, while totals.reversed is own-ledger-only and
+  // 90-day-pruned, so pairing them lets the denominator grow -- with every newly registered install, and with
+  // every additional day of immortal publish events -- while the numerator cannot, trending the percentage
+  // toward 100 independent of real reversal behavior. windowedMerged/windowedClosed (accumulated above) is the
+  // pairing that shares BOTH of the numerator's bounds: own-ledger only, and inside audit_events' retention
+  // window. The fleet fold still correctly inflates reviewed/handled/minutesSaved, which have no such pairing.
   // Fleet accuracy (bugfix, #fairness-analytics): independent of the own-ledger allowlist above, so it's fetched
   // unconditionally alongside the Orb global fold, matching that fold's own unscoped-regardless-of-allowlist
   // behavior (see the "skips the own-ledger queries but still queries the Orb aggregate" test).
@@ -610,10 +639,11 @@ export async function getPublicStats(
       ...totals,
       reviewed,
       filteredPct: filteredPct(reviewed, totals.merged),
-      // Option 1 of #7449: compute the global accuracy from the OWN-LEDGER merged/closed snapshot (not the
-      // fleet-folded totals.merged/closed), so its numerator (own-ledger reversed) and denominator are drawn
-      // from the same population. See the ownLedgerMerged/ownLedgerClosed snapshot above the Orb fold for why.
-      accuracyPct: accuracyPct(ownLedgerMerged, ownLedgerClosed, totals.reversed, reversalObservable),
+      // #7449 extended: compute the global accuracy from windowedMerged/windowedClosed (accumulated in the
+      // 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),
       minutesSaved,
     },
     weekly: { reviewed: w.reviewed ?? 0, merged: w.merged ?? 0 },

test/unit/public-stats.test.ts

@@ -737,6 +737,42 @@ describe("getPublicStats — live aggregate over the review ledger", () => {
     expect(out.byProject.map((row) => row.accuracyPct)).toEqual([null]);
   });
 
+  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
+    // drift toward 100% as the ledger aged -- live: 2377/602/508 reviewed, 0 reversals, 100% each.
+    const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/loopover" });
+    const old = new Date(NOW - 200 * 86_400_000).toISOString(); // outside the 90-day window
+    const recent = new Date(NOW - 5 * 86_400_000).toISOString();
+    await upsertRepositoryFromGitHub(env, { name: "loopover", full_name: "JSONbored/loopover", private: false, owner: { login: "JSONbored" } }, 1);
+
+    // Four ancient merged PRs -- they keep counting toward `reviewed`, but must NOT pad the accuracy
+    // denominator, because a reversal of any of them would long since have been pruned.
+    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 });
+    }
+    // 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: [] });
+    await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: "JSONbored/loopover#5", outcome: "completed", createdAt: recent });
+    await recordAuditEvent(env, { eventType: "agent.action.close", targetKey: "JSONbored/loopover#5", outcome: "completed", createdAt: recent });
+    await recordAuditEvent(env, { eventType: "reversal_reopened", targetKey: "JSONbored/loopover#5", outcome: "completed", createdAt: recent });
+    // ...and one recent merged PR that stands, so the windowed denominator is 1 merged + 0 closed... plus
+    // 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 });
+
+    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.
+    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 () => {
     const env = createTestEnv({ LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/loopover" });
     await upsertRepositoryFromGitHub(env, { name: "loopover", full_name: "JSONbored/loopover", private: false, owner: { login: "JSONbored" } }, 1);