From b92a62b5f62788cbb46a904986bee3ea83d1c390 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 05:03:34 -0700 Subject: [PATCH] fix(orb): stop double-counting PRs already tracked by the own-ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getOrbGlobalStats folded in every registered Orb installation's outcomes unconditionally, re-counting PRs the frozen own-ledger (pull_requests + audit_events, frozen since the self-host cutover) already counted. Quantified: 243 PRs (173 merged + 70 closed, 96% in one repo), inflating the public homepage's "PRs reviewed" and "Decision accuracy" tiles. Adds a NOT EXISTS anti-join so orb_pr_outcomes rows whose (repo, pr_number) already has a github_app.pr_public_surface_published audit event are excluded — the two sums are now over disjoint PR sets. No data loss: the own-ledger side is untouched, and no backfill is needed since the two sources' actual PR-number ranges barely overlap outside the double-counted set. --- src/orb/outcomes.ts | 17 ++++++++++++++--- src/review/public-stats.ts | 17 +++++++++-------- test/integration/orb-outcomes.test.ts | 15 +++++++++++++++ test/unit/public-stats.test.ts | 9 ++++++++- 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/orb/outcomes.ts b/src/orb/outcomes.ts index aa953ea3a..2f57fbd8c 100644 --- a/src/orb/outcomes.ts +++ b/src/orb/outcomes.ts @@ -41,10 +41,16 @@ export interface OrbGlobalStats { * The public global aggregate: merged / closed / total terminal PR outcomes across REGISTERED installations * only (registered = 1) — an install that hasn't been opted in never contributes to the public counter. SUM over * no matching rows is NULL, so each total is nullish-guarded to 0 (fail-safe on an empty/cold table). + * + * The NOT EXISTS anti-join skips any (repo, pr_number) that already has a `github_app.pr_public_surface_published` + * audit event — i.e. a PR the own-ledger disposition query in public-stats.ts already counted. Without it, a PR + * reviewed before the self-host cutover (own-ledger) that also has a terminal outcome recorded here (Orb) gets + * counted twice. Quantified 2026-07-12: 243 PRs (173 merged + 70 closed), 96% in one repo, inflating the public + * "PRs reviewed" counter. That event_type's target_key only ever references the own-ledger's own repos, so this + * is a no-op for every other registered installation's outcomes. */ export async function getOrbGlobalStats(env: Env, opts: { excludeAccount?: string } = {}): Promise { - // excludeAccount de-dups an account already counted by another source — the homepage counts JSONbored's own - // repos via cloud review_audit, so it excludes that account here to avoid double-counting. "" = include all. + // excludeAccount de-dups an account already counted by another source. "" = include all. const exclude = (opts.excludeAccount ?? "").toLowerCase(); const row = await env.DB.prepare( `SELECT @@ -53,7 +59,12 @@ export async function getOrbGlobalStats(env: Env, opts: { excludeAccount?: strin COUNT(*) AS total FROM orb_pr_outcomes o JOIN orb_github_installations i ON i.installation_id = o.installation_id AND i.registered = 1 - WHERE ? = '' OR LOWER(COALESCE(i.account_login, '')) <> ?`, + WHERE (? = '' OR LOWER(COALESCE(i.account_login, '')) <> ?) + AND NOT EXISTS ( + SELECT 1 FROM audit_events ae + WHERE ae.event_type = 'github_app.pr_public_surface_published' + AND LOWER(ae.target_key) = LOWER(o.repository_full_name) || '#' || o.pr_number + )`, ) .bind(exclude, exclude) .first<{ merged: number | null; closed: number | null; total: number | null }>(); diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index f133224c4..efeff3867 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -25,14 +25,15 @@ // GLOBAL: the homepage total folds in every REGISTERED Orb installation's outcomes (getOrbGlobalStats) on top of // the own-ledger side, so the counter reflects the whole fleet, not just gittensory's own repos. The own-ledger // side (audit_events) is a FROZEN snapshot as of the self-host cutover -- it stops growing the day each repo's -// live processing moved off this worker -- while orb_pr_outcomes keeps growing in realtime for any repo with the -// central Orb App installed (including JSONbored's own, which still runs the Orb App for telemetry alongside its -// self-hosted review engine). The two sources are NOT mutually exclusive by account: JSONbored appears in both, -// but only overlaps for the few-day window before the cutover froze the own-ledger side, so no excludeAccount is -// applied here -- a small, bounded double-count for that historical window is preferable to silently dropping -// all of JSONbored's live post-cutover Orb data (which excluding "jsonbored" used to do, since every REGISTERED -// installation currently happens to be a JSONbored account). See getOrbGlobalStats for the general-purpose -// excludeAccount dedup this call deliberately does not use. +// live processing moved off this worker, and can never grow again now that the old App has been fully deleted -- +// while orb_pr_outcomes keeps growing in realtime for any repo with the central Orb App installed (including +// JSONbored's own, which still runs the Orb App for telemetry alongside its self-hosted review engine). The two +// sources overlap by (repo, pr_number), not just by account: earlier reasoning here called that overlap "a small, +// bounded double-count," but it was measured directly (2026-07-12) at 243 PRs (173 merged + 70 closed, 96% in one +// repo) -- large enough to move accuracyPct and mislead visitors, not a rounding error. getOrbGlobalStats now +// excludes exactly those overlapping (repo, pr_number) pairs via a NOT EXISTS anti-join against the same +// `github_app.pr_public_surface_published` audit events this file's own disposition query reads, so the two sums +// are over disjoint PR sets and can be added directly. import { getOrbGlobalStats } from "../orb/outcomes"; /** FALLBACK estimate of maintainer review/triage time saved per reviewed PR, used ONLY when the real per-PR diff --git a/test/integration/orb-outcomes.test.ts b/test/integration/orb-outcomes.test.ts index bd94a5467..33b3a15fc 100644 --- a/test/integration/orb-outcomes.test.ts +++ b/test/integration/orb-outcomes.test.ts @@ -96,4 +96,19 @@ describe("getOrbGlobalStats", () => { expect(await getOrbGlobalStats(e, { excludeAccount: "jsonbored" })).toEqual({ merged: 0, closed: 1, total: 1 }); // JSONbored dropped expect(await getOrbGlobalStats(e)).toEqual({ merged: 1, closed: 1, total: 2 }); // no exclude → both }); + + it("excludes a PR already counted by the own-ledger (published-surface audit event), but still counts an unrelated one", async () => { + const e = createTestEnv(); + const db = e.DB as unknown as TestD1Database; + await registerInstall(e, 100, 1); + // acme/dup#1 was already published (and therefore counted) by the own-ledger disposition query. + await db + .prepare( + "INSERT INTO audit_events (id, event_type, target_key, outcome, created_at) VALUES ('evt-1', 'github_app.pr_public_surface_published', 'acme/dup#1', 'success', '2026-06-24T00:00:00Z')", + ) + .run(); + await recordOrbPrOutcome(e, "pull_request", closedPr("acme/dup", 1, "2026-06-24T00:00:00Z", 100)); // merged, already own-ledger-counted → must be excluded + await recordOrbPrOutcome(e, "pull_request", closedPr("acme/new", 2, null, 100)); // closed, no own-ledger counterpart → must still count + expect(await getOrbGlobalStats(e)).toEqual({ merged: 0, closed: 1, total: 1 }); + }); }); diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index 179c2b5aa..e63fd5131 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -34,11 +34,18 @@ function isWeekly(sql: string): boolean { function isEffort(sql: string): boolean { return sql.includes("reviewEffortMinutes"); } +// 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. +function isOrbGlobal(sql: string): boolean { + return sql.includes("orb_pr_outcomes"); +} function isDispositions(sql: string): boolean { return ( sql.includes("github_app.pr_public_surface_published") && !isWeekly(sql) && - !isEffort(sql) + !isEffort(sql) && + !isOrbGlobal(sql) ); } // The reversal read is the only one that inspects engine auto-actions (close/merge) against pull_requests state.