fix(stats): derive the published totals from the ledger instead of publishing a false zero

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;

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: {