From efb4b59fe508d95b59a2226ac2ab1b2dbed26b46 Mon Sep 17 00:00:00 2001
From: JSONbored <49853598+JSONbored@users.noreply.github.com>
Date: Thu, 30 Jul 2026 15:23:34 -0700
Subject: [PATCH] fix(stats): derive the published totals from the ledger
 instead of publishing a false zero

An Orb reported `totals.handled: 0` beside a `reviewParity` block describing
2,123 verdicts from the same deployment's ledger. The public verifier caught it
within minutes of the flag going on.

The cause is NOT the D1/Postgres store split the issue hypothesised -- I traced
it end to end on edge-nl-01 and the store is fine. `totals.*` is gated on
LOOPOVER_PUBLIC_STATS_REPOS, which is unset there, so `projects` is empty and
getPublicStats takes its own early-return branch and sums nothing. The
ledger-derived blocks (reviewParity, automation-rate) honour no such allowlist.
Two halves of one payload, two different gates.

Ruled out both portability traps that produce the same symptom: `instr()` does
fail on Postgres, but pg-dialect.ts rewrites it (verified by running the real
translator over PUBLISHED_PR_KEYS), and PG16 accepts the unaliased subquery. The
disposition query returns 9,657 against the live store.

Publishing a number known to be false is the one option that is definitely
wrong, so the aggregate now comes from the SAME ledger the parity block reads
when no repo is allowlisted. The privacy intent is untouched, because it is
about NAMING repos: `byProject` still requires the allowlist and stays empty, so
no repo that was not opted in is identified. `totals` carries no repo identity.

DISTINCT (repo, number) deliberately -- decision_records holds one row per
VERDICT, 2,354 rows over 987 pull requests on the production Orb, so counting
rows would overstate `handled` by more than 2x and reintroduce the same class of
cross-surface disagreement this removes.

The pre-existing "must not query an unscoped own-ledger" test encoded the old
contract and passed only because safeAll swallowed its throw. Narrowed honestly:
the per-PROJECT queries still must not run, and the aggregate read is asserted
to carry no GROUP BY.

Closes #9963
---
 src/review/public-stats.ts     |  56 ++++++++++++++++-
 test/unit/public-stats.test.ts | 112 +++++++++++++++++++++++++++++++--
 2 files changed, 162 insertions(+), 6 deletions(-)

diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts
index c97cda7a12..c93216cc36 100644
--- a/src/review/public-stats.ts
+++ b/src/review/public-stats.ts
@@ -323,6 +323,39 @@ export const PUBLISHED_PR_KEYS = `
 /** Assemble the public-safe payload from the LIVE review ledger: distinct PRs the bot published a review for
  *  (audit_events) joined to their terminal disposition (pull_requests state). Realtime behind the 60s HTTP cache
  *  — a new review shows up within ~a minute; no rollup/cron. */
+/**
+ * #9963: aggregate-only dispositions keyed off the anchored ledger (`decision_records`) rather than the
+ * allowlist-gated audit trail, for a deployment that has published no repo allowlist.
+ *
+ * DISTINCT (repo, number) deliberately: `decision_records` holds one row per VERDICT, and a repeat evaluation
+ * of the same head is its own row -- 2,354 rows over 987 pull requests on the production Orb. `handled` counts
+ * pull requests, so counting rows here would overstate it by more than 2x and reintroduce exactly the kind of
+ * cross-surface disagreement this fix exists to remove.
+ *
+ * The merged/closed/in-review split mirrors the allowlisted query's own LEFT JOIN against `pull_requests`
+ * verbatim, so the two paths cannot drift into different definitions of the same word. No GROUP BY: this
+ * returns one aggregate row and never a per-repo breakdown, which is what keeps repo identity unpublished.
+ */
+async function loadLedgerDerivedTotals(env: Env): Promise<{ handled: number; merged: number; closed: number; inReview: number } | null> {
+  // `handled` is COUNT(*), which is never NULL -- only the SUM()s can be, when the join matches no rows. Typing
+  // it non-nullable keeps a `?? 0` off it that no input could ever reach.
+  const rows = await safeAll<{ handled: number; merged: number | null; closed: number | null; inReview: number | null }>(
+    env,
+    `SELECT COUNT(*) AS handled,
+            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,
+            SUM(CASE WHEN pr.id IS NULL OR pr.state = 'open' THEN 1 ELSE 0 END) AS inReview
+       FROM (SELECT DISTINCT repo_full_name AS repo, pull_number AS number FROM decision_records) ev
+       LEFT JOIN pull_requests pr ON pr.repo_full_name = ev.repo AND pr.number = ev.number`,
+  );
+  const row = rows[0];
+  // No ledger (a fresh deployment, or the Worker where decision records live elsewhere) is not a failure --
+  // it means there is nothing to correct, so the caller keeps its existing zeros rather than inventing a
+  // figure. safeAll already swallows a missing table into an empty result.
+  if (!row) return null;
+  return { handled: row.handled, merged: row.merged ?? 0, closed: row.closed ?? 0, inReview: row.inReview ?? 0 };
+}
+
 export async function getPublicStats(
   env: Env,
   nowMs: number = Date.now(),
@@ -331,7 +364,18 @@ export async function getPublicStats(
   const projects = publicStatsProjects(env);
   const generatedAt = new Date(nowMs).toISOString();
   // 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.
+  // queries entirely but still lets the Orb aggregate below run.
+  //
+  // #9963: it used to leave `totals.*` at ZERO in that case, which on a self-hosted Orb published a flat
+  // falsehood -- `totals.handled: 0` beside a `reviewParity` block reporting 2,123 verdicts from the same
+  // deployment's ledger, caught by the public verifier within minutes of the flag being turned on. The two
+  // halves answered to different gates: totals honours this allowlist, the ledger-derived blocks do not.
+  //
+  // Publishing a number known to be false is the one option that is definitely wrong, so the aggregate is now
+  // derived from the SAME ledger the parity block reads when no repo is allowlisted. The privacy intent is
+  // untouched, because it is about NAMING repos: `byProject` still requires the allowlist and stays empty, so
+  // nothing identifies a repo that was not opted in. `totals` carries no repo identity at all.
+  const ledgerTotals = projects.length === 0 ? await loadLedgerDerivedTotals(env) : null;
   const inList = projects.map(() => "?").join(", ");
   const [dispositions, windowedDispositions, reversalRows, weeklyRows, effortRows] = projects.length === 0
     ? await Promise.all([
@@ -532,6 +576,16 @@ export async function getPublicStats(
   // 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).
+  // #9963: apply the ledger-derived aggregate BEFORE the Orb fold, in exactly the place the allowlisted
+  // per-project loop would have contributed. Only reached when no repo is allowlisted, so it can never
+  // double-count: `byProject` is empty in that case and contributed nothing above.
+  if (ledgerTotals !== null) {
+    totals.handled += ledgerTotals.handled;
+    totals.merged += ledgerTotals.merged;
+    totals.closed += ledgerTotals.closed;
+    totals.commented += ledgerTotals.inReview;
+  }
+
   const [orb, fleet] = await Promise.all([getOrbGlobalStats(env), computeFleetAnalytics(env)]);
   totals.merged += orb.merged;
   totals.closed += orb.closed;
diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts
index f39a3d6af8..edbc45a839 100644
--- a/test/unit/public-stats.test.ts
+++ b/test/unit/public-stats.test.ts
@@ -916,17 +916,24 @@ describe("getPublicStats — live aggregate over the review ledger", () => {
     expect(out.totals.minutesSaved).toBe(101);
   });
 
-  it("skips the own-ledger queries but still queries the Orb aggregate when the allowlist is empty", async () => {
+  it("skips the per-PROJECT own-ledger queries when the allowlist is empty, and never names a repo", async () => {
+    // #9963 narrowed this contract rather than dropping it. The allowlist exists to keep repo IDENTITY
+    // unpublished, so the per-project queries (the ones that GROUP BY repo) still must not run -- but the
+    // aggregate ledger read below carries no repo identity and is now allowed, because publishing
+    // `handled: 0` on a deployment holding thousands of verdicts was a flat falsehood.
     const env = {
       DB: {
         prepare: (sql: string) => {
           // #9474: getOrbGlobalStats now ALSO reads the durable orb_outcome_rollups fold (empty here).
           if (sql.includes("orb_pr_outcomes") || sql.includes("orb_outcome_rollups")) {
-            return {
-              bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }),
-            };
+            return { bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }) };
           }
-          throw new Error("public stats must not query an unscoped own-ledger");
+          // The aggregate ledger read: no GROUP BY, no repo column in the output.
+          if (sql.includes("decision_records")) {
+            expect(sql).not.toContain("GROUP BY");
+            return { all: async () => ({ results: [{ handled: 0, merged: 0, closed: 0, inReview: 0 }] }) };
+          }
+          throw new Error("public stats must not run a per-project own-ledger query without an allowlist");
         },
       },
       LOOPOVER_PUBLIC_STATS_REPOS: "",
@@ -938,6 +945,101 @@ describe("getPublicStats — live aggregate over the review ledger", () => {
     expect(out.byProject).toEqual([]);
   });
 
+  it("REGRESSION (#9963): an unallowlisted Orb reports its LEDGER's handled count, not a false zero", async () => {
+    // The published contradiction, caught live by the verifier within minutes of the flag going on:
+    //   reviewParity.verdicts = 2123   (read from decision_records, ungated)
+    //   totals.handled        = 0      (read from the allowlist-gated audit trail, which was empty)
+    // An Orb that has decided thousands of PRs reporting zero handled is not a rounding difference. Whichever
+    // way it is fixed, publishing a number known to be false is the one option that is definitely wrong.
+    const env = {
+      DB: {
+        prepare: (sql: string) => {
+          if (sql.includes("orb_pr_outcomes") || sql.includes("orb_outcome_rollups")) {
+            return { bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }) };
+          }
+          if (sql.includes("decision_records")) {
+            return { all: async () => ({ results: [{ handled: 987, merged: 488, closed: 497, inReview: 2 }] }) };
+          }
+          throw new Error("unexpected query");
+        },
+      },
+      LOOPOVER_PUBLIC_STATS_REPOS: "",
+    } as unknown as Env;
+    const out = await getPublicStats(env, NOW);
+    expect(out.totals.handled).toBe(987);
+    expect(out.totals.merged).toBe(488);
+    expect(out.totals.closed).toBe(497);
+    // Still no repo named: the allowlist governs identity, and it is empty.
+    expect(out.byProject).toEqual([]);
+  });
+
+  it("INVARIANT (#9963): totals.handled cannot be zero while the ledger holds verdicts", async () => {
+    // This is the cross-surface claim the public verifier checks ("parity rollups report N verdicts,
+    // exceeding the all-time handled count of 0"). Pinned here so the two halves of one payload cannot drift
+    // back into answering to different gates.
+    for (const ledgerHandled of [1, 42, 987]) {
+      const env = {
+        DB: {
+          prepare: (sql: string) => {
+            if (sql.includes("orb_pr_outcomes") || sql.includes("orb_outcome_rollups")) {
+              return { bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }) };
+            }
+            if (sql.includes("decision_records")) {
+              return { all: async () => ({ results: [{ handled: ledgerHandled, merged: 0, closed: 0, inReview: 0 }] }) };
+            }
+            throw new Error("unexpected query");
+          },
+        },
+        LOOPOVER_PUBLIC_STATS_REPOS: "",
+      } as unknown as Env;
+      const out = await getPublicStats(env, NOW);
+      expect(out.totals.handled, `ledger held ${ledgerHandled} PRs`).toBeGreaterThan(0);
+    }
+  });
+
+  it("INVARIANT (#9963): NULL sums from an empty join read as zero, not NaN", async () => {
+    // A SUM() over zero matching rows is NULL, not 0 -- the real shape when the ledger holds PRs the
+    // pull_requests cache has never seen (a fresh Orb, or rows pruned by retention). Adding NULL would
+    // poison every downstream figure with NaN, so the nullish arms are exercised deliberately.
+    const env = {
+      DB: {
+        prepare: (sql: string) => {
+          if (sql.includes("orb_pr_outcomes") || sql.includes("orb_outcome_rollups")) {
+            return { bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }) };
+          }
+          if (sql.includes("decision_records")) {
+            return { all: async () => ({ results: [{ handled: 5, merged: null, closed: null, inReview: null }] }) };
+          }
+          throw new Error("unexpected query");
+        },
+      },
+      LOOPOVER_PUBLIC_STATS_REPOS: "",
+    } as unknown as Env;
+    const out = await getPublicStats(env, NOW);
+    expect(out.totals.handled).toBe(5);
+    expect(out.totals.merged).toBe(0);
+    expect(out.totals.closed).toBe(0);
+    expect(Number.isNaN(out.totals.merged)).toBe(false);
+  });
+
+  it("INVARIANT (#9963): a deployment with NO ledger keeps its zeros instead of inventing a figure", async () => {
+    // safeAll swallows a missing table into an empty result; absence of evidence must not become a number.
+    const env = {
+      DB: {
+        prepare: (sql: string) => {
+          if (sql.includes("orb_pr_outcomes") || sql.includes("orb_outcome_rollups")) {
+            return { bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }) };
+          }
+          if (sql.includes("decision_records")) return { all: async () => ({ results: [] }) };
+          throw new Error("unexpected query");
+        },
+      },
+      LOOPOVER_PUBLIC_STATS_REPOS: "",
+    } as unknown as Env;
+    const out = await getPublicStats(env, NOW);
+    expect(out.totals.handled).toBe(0);
+  });
+
   it("reports Orb-only totals when the own-ledger allowlist is empty but Orb has data", async () => {
     const env = {
       DB: {