diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index c97cda7a1..8a5128ba0 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -320,6 +320,62 @@ export const PUBLISHED_PR_KEYS = ` WHERE event_type = 'github_app.pr_public_surface_published' AND instr(target_key, '#') > 0 AND length(target_key) - length(replace(target_key, '#', '')) = 1`; +/** + * #9963: the deployment's OWN decision ledger as a disposition source, in the same shape the published-surface + * query above produces, so everything downstream (totals, byProject, the sort) folds it identically. + * + * Why this exists. `totals` had exactly two sources -- the allowlisted `audit_events` published-surface snapshot + * and the registered-installs fleet fold -- and BOTH are hosted-Worker concepts. On a self-hosted Orb + * `LOOPOVER_PUBLIC_STATS_REPOS` is unset (it is a frozen snapshot of the repos the old central App used to + * process, not a list any self-hoster has a reason to fill in), so the own-ledger queries are skipped entirely; + * and `orb_pr_outcomes` holds what OTHER registered installations reported, which on an Orb is nobody. Both + * sources are therefore structurally empty there, while `decision_records` -- the anchored ledger this + * deployment actually writes a row to per verdict -- held 2,123 rows. The result was a published + * `totals.handled: 0` beside a `reviewParity.verdicts: 2123` computed from that same ledger: not a rounding + * difference, an entire population missing. Every `totals.*` figure inherited it. + * + * DISJOINT BY CONSTRUCTION, so the three sources can be added rather than reconciled -- the same discipline the + * file header states for the first two. Both anti-joins below are exclusions of pairs another source already + * counts: + * • the published-surface set, but ONLY where that repo is allowlisted, because that is the exact condition + * under which the first query counts a pair. Excluding an un-allowlisted published pair would drop it from + * every source at once; + * • the registered-install outcomes the fleet fold counts (`getOrbGlobalStats`'s own population, matched on + * the same (repo, pr_number) key its anti-join uses). + * + * On the hosted Worker `decision_records` is empty by design (review execution is retired there), so this adds + * nothing and that deployment's numbers do not move at all. + */ +function ledgerDispositionsSql(allowlistPlaceholders: string): string { + // The allowlist arm is omitted entirely when there is no allowlist: `IN ()` is a syntax error in both + // dialects, and with no allowlist the published-surface query contributes nothing to exclude anyway. + const publishedExclusion = + allowlistPlaceholders === "" + ? "" + : `AND NOT EXISTS ( + SELECT 1 FROM audit_events ae + WHERE ae.event_type = 'github_app.pr_public_surface_published' + AND ae.target_key = dr.repo || '#' || dr.number + AND LOWER(dr.repo) IN (${allowlistPlaceholders}) + )`; + // `||` for concatenation and NOT EXISTS both mean the same thing in SQLite and Postgres and need no + // translation -- the same basis on which getOrbGlobalStats already builds its target_key anti-join. + return `SELECT dr.repo AS project, + COUNT(*) AS reviewed, + 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) dr + LEFT JOIN pull_requests pr ON pr.repo_full_name = dr.repo AND pr.number = dr.number + WHERE NOT EXISTS ( + SELECT 1 FROM orb_pr_outcomes o + JOIN orb_github_installations i ON i.installation_id = o.installation_id AND i.registered = 1 + WHERE o.repository_full_name = dr.repo AND o.pr_number = dr.number + ) + ${publishedExclusion} + GROUP BY dr.repo`; +} + /** 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. */ @@ -333,6 +389,9 @@ 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(", "); + // #9963: deliberately OUTSIDE the allowlist branch below. An empty allowlist is precisely the self-host case + // this source exists to cover, so gating it on the allowlist would reproduce the bug it fixes. + const ledgerDispositionsPromise = safeAll(env, ledgerDispositionsSql(inList), ...projects); const [dispositions, windowedDispositions, reversalRows, weeklyRows, effortRows] = projects.length === 0 ? await Promise.all([ Promise.resolve([]), @@ -463,6 +522,30 @@ export async function getPublicStats( ), ]); + // #9963: fold the own decision ledger in alongside the published-surface dispositions. Merged per project + // rather than concatenated so a repo appearing in both sources stays ONE row in `byProject` -- the two are + // already disjoint at the (repo, pr) level (see ledgerDispositionsSql), so the per-project counts add. + const ledgerDispositions = await ledgerDispositionsPromise; + const dispositionsByProject = new Map(); + for (const row of [...dispositions, ...ledgerDispositions]) { + const key = String(row.project).toLowerCase(); + const existing = dispositionsByProject.get(key); + if (!existing) { + dispositionsByProject.set(key, { ...row, project: String(row.project) }); + continue; + } + // `?? 0` on every arm: SUM()/COUNT() over zero rows is NULL, and DispositionRow types these as `number` + // only because the downstream fold already coalesces them. + dispositionsByProject.set(key, { + project: existing.project, + reviewed: (existing.reviewed ?? 0) + (row.reviewed ?? 0), + merged: (existing.merged ?? 0) + (row.merged ?? 0), + closed: (existing.closed ?? 0) + (row.closed ?? 0), + inReview: (existing.inReview ?? 0) + (row.inReview ?? 0), + }); + } + const mergedDispositions = [...dispositionsByProject.values()]; + const reversedByProject = new Map( reversalRows.map((r) => [String(r.project).toLowerCase(), r.reversed ?? 0]), ); @@ -482,7 +565,7 @@ export async function getPublicStats( const windowedByProject = new Map(windowedDispositions.map((row) => [String(row.project).toLowerCase(), row])); let windowedMerged = 0; let windowedClosed = 0; - const byProject = dispositions + const byProject = mergedDispositions .map((d) => { const merged = d.merged ?? 0; const closed = d.closed ?? 0; diff --git a/test/unit/public-stats-self-host-ledger.test.ts b/test/unit/public-stats-self-host-ledger.test.ts new file mode 100644 index 000000000..1d9d5079e --- /dev/null +++ b/test/unit/public-stats-self-host-ledger.test.ts @@ -0,0 +1,187 @@ +import { describe, expect, it } from "vitest"; + +import { checkStatsParity } from "../../packages/loopover-mcp/lib/verify-public-claims"; +import { getPublicStats } from "../../src/review/public-stats"; +import { loadReviewParityRollups } from "../../src/review/review-parity-rollups"; +import { recordAuditEvent, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// #9963: `totals.*` on a SELF-HOSTED Orb. +// +// The defect: an Orb published `totals.handled: 0` in the same payload as `reviewParity.verdicts: 2123`, both +// derived from its own ledger. `totals` had exactly two sources and both are hosted-Worker concepts -- +// the `LOOPOVER_PUBLIC_STATS_REPOS`-allowlisted `audit_events` snapshot (a frozen list of the repos the old +// central App used to process, which no self-hoster has a reason to set) and the registered-installs fleet fold +// (which on an Orb is nobody). `decision_records`, the ledger the Orb actually writes a row to per verdict, was +// not a source at all. So every `totals.*` figure was structurally zero on the deployment that does the work. +// +// These run against a REAL migrated D1 rather than a SQL-shape stub, because the bug was that a real query +// returned nothing for a real reason -- a stub asked the wrong question and would have answered it happily. +const NOW = Date.parse("2026-07-30T12:00:00.000Z"); +const REPO = "JSONbored/loopover"; + +/** Insert one ledger verdict. Direct SQL, not `persistDecisionRecord`: this fixes a READ, and going through the + * writer would drag in digesting and the hash-chain append without making the row under test any more real. */ +async function seedVerdict(env: Env, input: { repo?: string; pull: number; action?: string; at?: string }): Promise { + const repo = input.repo ?? REPO; + const at = input.at ?? new Date(NOW - 3_600_000).toISOString(); + await env.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ) + .bind(`record:${repo}#${input.pull}@sha${input.pull}`, repo, input.pull, `sha${input.pull}`, input.action ?? "merge", "gate_pass", "d".repeat(64), "{}", at) + .run(); +} + +async function seedMergedPr(env: Env, pull: number): Promise { + await upsertRepositoryFromGitHub(env, { name: "loopover", full_name: REPO, private: false, owner: { login: "JSONbored" } }, 1); + await upsertPullRequestFromGitHub(env, REPO, { + number: pull, + title: `pr ${pull}`, + state: "closed", + merged_at: new Date(NOW - 86_400_000).toISOString(), + user: { login: "a" }, + head: { sha: `sha${pull}` }, + labels: [], + }); +} + +describe("getPublicStats on a self-hosted Orb (#9963)", () => { + it("REGRESSION: counts the deployment's own decision ledger instead of publishing handled: 0", async () => { + // The Orb's exact configuration: public stats on, and NO own-ledger allowlist -- which is what silently + // skipped every own-ledger query and left the headline at zero. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "" }); + for (const pull of [1, 2, 3, 4, 5]) await seedVerdict(env, { pull }); + + const stats = await getPublicStats(env, NOW); + + expect(stats.totals.handled).toBe(5); + expect(stats.totals.reviewed).toBe(5); + // And the per-project table is no longer empty beside a non-zero headline. + expect(stats.byProject.map((row) => row.project)).toEqual([REPO]); + expect(stats.byProject[0]?.reviewed).toBe(5); + }); + + it("counts a PR ONCE however many verdicts the ledger holds for it", async () => { + // Re-evaluations append rows for the same (repo, pull). `handled` counts PRs, so a re-decided PR must not + // inflate it -- the DISTINCT is load-bearing, not incidental. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "" }); + await seedVerdict(env, { pull: 1 }); + await env.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ) + .bind(`record:${REPO}#1@sha1:rev2`, REPO, 1, "sha1", "merge", "gate_pass", "e".repeat(64), "{}", new Date(NOW - 1_000).toISOString()) + .run(); + + expect((await getPublicStats(env, NOW)).totals.handled).toBe(1); + }); + + it("reads the terminal disposition from the PR cache, so a merged verdict is not filed as still-in-review", async () => { + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "" }); + await seedMergedPr(env, 1); + await seedVerdict(env, { pull: 1 }); + await seedVerdict(env, { pull: 2 }); // no cached PR row -> still in review + + const stats = await getPublicStats(env, NOW); + expect(stats.totals.handled).toBe(2); + expect(stats.totals.merged).toBe(1); + expect(stats.totals.commented).toBe(1); + }); + + it("INVARIANT: does not double-count a PR the allowlisted published-surface query already counted", async () => { + // With an allowlist set, both sources see the same PR. They are added, not reconciled, so the ledger + // source must exclude exactly what the other one counts or the headline silently doubles. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: REPO }); + await seedMergedPr(env, 1); + await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: `${REPO}#1`, outcome: "completed" }); + await seedVerdict(env, { pull: 1 }); + + expect((await getPublicStats(env, NOW)).totals.handled).toBe(1); + }); + + it("still counts a ledger PR the allowlist EXCLUDES -- the exclusion is per counted pair, not per event", async () => { + // The subtle way to get the anti-join wrong: drop any PR that has a published-surface event, rather than + // only those the other query actually counts. An un-allowlisted repo publishes surfaces too, and those PRs + // would then be counted by nobody. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/other-repo" }); + await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: `${REPO}#1`, outcome: "completed" }); + await seedVerdict(env, { pull: 1 }); + + expect((await getPublicStats(env, NOW)).totals.handled).toBe(1); + }); + + it("INVARIANT: does not double-count a PR the registered-install fleet fold already counted", async () => { + // The second overlap, and the easier one to forget: `getOrbGlobalStats` adds every REGISTERED install's + // outcomes on top of the own-ledger totals. An operator running the central Orb App for telemetry beside + // their self-hosted engine (which is exactly what JSONbored's own repos do -- see the file header) has the + // same PR in both populations, so without this exclusion the headline counts it twice. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "" }); + await env.DB.prepare(`INSERT INTO orb_github_installations (installation_id, account_login, registered) VALUES (?, ?, 1)`).bind(77, "JSONbored").run(); + await env.DB.prepare(`INSERT INTO orb_pr_outcomes (repository_full_name, pr_number, installation_id, outcome, occurred_at) VALUES (?, ?, ?, ?, ?)`) + .bind(REPO, 1, 77, "merged", new Date(NOW - 86_400_000).toISOString()) + .run(); + await seedVerdict(env, { pull: 1 }); // the SAME PR, also in this deployment's own ledger + await seedVerdict(env, { pull: 2 }); // ledger-only, so the exclusion cannot pass by dropping everything + + const stats = await getPublicStats(env, NOW); + // 1 from the fleet fold + 1 ledger-only PR. The shared PR is counted once, not twice. + expect(stats.totals.handled).toBe(2); + }); + + it("INVARIANT: an unregistered install's outcome does NOT suppress a ledger PR", async () => { + // The exclusion has to mirror the fleet fold's own population exactly. That fold counts only REGISTERED + // installations, so excluding on an unregistered row would drop a PR that nothing else counts. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "" }); + await env.DB.prepare(`INSERT INTO orb_github_installations (installation_id, account_login, registered) VALUES (?, ?, 0)`).bind(78, "someone").run(); + await env.DB.prepare(`INSERT INTO orb_pr_outcomes (repository_full_name, pr_number, installation_id, outcome, occurred_at) VALUES (?, ?, ?, ?, ?)`) + .bind(REPO, 1, 78, "merged", new Date(NOW - 86_400_000).toISOString()) + .run(); + await seedVerdict(env, { pull: 1 }); + + expect((await getPublicStats(env, NOW)).totals.handled).toBe(1); + }); + + it("INVARIANT: an empty decision ledger leaves the hosted Worker's numbers exactly where they were", async () => { + // The hosted Worker has review execution retired, so its ledger is empty by design. This change must be a + // no-op there -- a fix for one deployment that moves another deployment's published figures is not a fix. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: REPO }); + await seedMergedPr(env, 1); + await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", targetKey: `${REPO}#1`, outcome: "completed" }); + + const stats = await getPublicStats(env, NOW); + expect(stats.totals.handled).toBe(1); + expect(stats.totals.merged).toBe(1); + }); +}); + +// The invariant the public verifier actually enforces, checked with the verifier's OWN function rather than a +// restatement of it -- so this cannot drift from the tool that decides whether production is publishing a +// contradiction. `checkStatsParity` is what printed the original FAIL against the Orb. +describe("published stats and parity rollups cannot contradict each other (#9963)", () => { + it("REGRESSION: the verifier's stats-parity claim PASSES for a self-hosted Orb", async () => { + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "" }); + for (let pull = 1; pull <= 12; pull += 1) await seedVerdict(env, { pull }); + + const [stats, parity] = await Promise.all([getPublicStats(env, NOW), loadReviewParityRollups(env, { nowMs: NOW })]); + + // Both surfaces see the same ledger. Before the fix: handled=0 beside verdicts=12. + expect(parity.verdicts).toBe(12); + expect(stats.totals.handled).toBe(12); + + const result = checkStatsParity(stats, parity); + expect(result.status).toBe("pass"); + }); + + it("MUTATION GUARD: the same claim FAILS when handled is zeroed beneath a populated rollup", async () => { + // Proves the assertion above is driven by the numbers rather than passing for any payload at all. This is + // the exact contradiction production published. + const env = createTestEnv({ LOOPOVER_PUBLIC_STATS: "true", LOOPOVER_PUBLIC_STATS_REPOS: "" }); + for (let pull = 1; pull <= 12; pull += 1) await seedVerdict(env, { pull }); + const parity = await loadReviewParityRollups(env, { nowMs: NOW }); + + const result = checkStatsParity({ totals: { handled: 0 } }, parity); + expect(result.status).toBe("fail"); + expect(result.detail).toContain("exceeding the all-time handled count of 0"); + }); +}); diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index f39a3d6af..8d5404457 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -44,15 +44,26 @@ function isEffort(sql: string): boolean { // getOrbGlobalStats's own-ledger anti-join also references `github_app.pr_public_surface_published` (to skip // PRs the disposition query already counted) — exclude it here the same way isWeekly/isEffort already are, or // every stub that doesn't special-case `orb_pr_outcomes` would wrongly route the orb read through here too. +// #9963: the own-decision-ledger disposition source. It is the ONLY read that touches `decision_records`, which +// is what makes this a precise discriminator -- and it needs one, because it names `orb_pr_outcomes` in an +// anti-join and would otherwise be routed to the Orb aggregate by any stub matching on that table alone. Same +// hazard, and the same fix, as the isOrbGlobal exclusion directly below. +function isLedgerDispositions(sql: string): boolean { + return sql.includes("decision_records"); +} function isOrbGlobal(sql: string): boolean { - return sql.includes("orb_pr_outcomes"); + return sql.includes("orb_pr_outcomes") && !isLedgerDispositions(sql); } function isDispositions(sql: string): boolean { return ( sql.includes("github_app.pr_public_surface_published") && !isWeekly(sql) && !isEffort(sql) && - !isOrbGlobal(sql) + !isOrbGlobal(sql) && + // #9963: the ledger source names this event type too, in the anti-join that keeps it disjoint from THIS + // query's population. Without the exclusion it would be served the published-surface fixture and every + // total would double. + !isLedgerDispositions(sql) ); } // The reversal read is the only one that reads the recorded reversal_reopened/reversal_reverted events. @@ -337,7 +348,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { it("folds Orb installs into the global totals on top of the own-ledger totals", async () => { const withOrb = (sql: string): Row[] => - sql.includes("orb_pr_outcomes") ? [{ merged: 50, closed: 30, total: 80 }] : ledger(sql); + isOrbGlobal(sql) ? [{ merged: 50, closed: 30, total: 80 }] : ledger(sql); const out = await getPublicStats(stubEnv(withOrb), NOW); expect(out.totals.merged).toBe(1392 + 50); // own-ledger + Orb expect(out.totals.closed).toBe(724 + 30); @@ -354,7 +365,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { 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 }]; + if (isOrbGlobal(sql)) return [{ merged: 6000, closed: 4000, total: 10000 }]; return []; }; const out = await getPublicStats(stubEnv(handler), NOW); @@ -376,7 +387,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { it("keeps own-ledger per-PR effort sum separate from Orb fleet flat credit", async () => { const withOrbAndEffort = (sql: string): Row[] => { - if (sql.includes("orb_pr_outcomes")) return [{ merged: 10, closed: 5, total: 15 }]; + if (isOrbGlobal(sql)) return [{ merged: 10, closed: 5, total: 15 }]; if (isEffort(sql)) return [{ totalMinutes: 100 }]; return ledger(sql); }; @@ -387,7 +398,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { it("does not exclude any account from the Orb aggregate (own-ledger side is a frozen snapshot, not live-overlapping)", async () => { let excludeBindArg: unknown; const captureExclude = (sql: string, args: unknown[]): Row[] => { - if (sql.includes("orb_pr_outcomes")) { + if (isOrbGlobal(sql)) { excludeBindArg = args[0]; return [{ merged: 0, closed: 0, total: 0 }]; } @@ -921,7 +932,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { 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")) { + if (isOrbGlobal(sql) || sql.includes("orb_outcome_rollups")) { return { bind: () => ({ first: async () => ({ merged: 0, closed: 0, total: 0 }) }), }; @@ -949,7 +960,7 @@ describe("getPublicStats — live aggregate over the review ledger", () => { bind: () => ({ first: async () => ({ merged: null, closed: null, total: null }) }), }; } - if (sql.includes("orb_pr_outcomes")) { + if (isOrbGlobal(sql)) { return { bind: () => ({ first: async () => ({ merged: 12, closed: 8, total: 20 }) }), };