fix(stats): derive the published totals from the ledger instead of publishing a false zero - #9967
Merged
Merged
Conversation
…blishing 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
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9967 +/- ##
==========================================
- Coverage 91.87% 91.00% -0.88%
==========================================
Files 928 928
Lines 113697 113707 +10
Branches 27421 27425 +4
==========================================
- Hits 104460 103476 -984
- Misses 7936 9125 +1189
+ Partials 1301 1106 -195
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This was referenced Jul 30, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
#9963 was an Orb publishing `totals.handled: 0` in the same payload as `reviewParity.verdicts: 2123`, both derived from the same ledger. #9967 fixed the aggregate and covered it well -- but per surface, which is the blind spot the defect lived in. Both halves were individually correct and individually green while production served the contradiction, and the verifier's own source says why per-surface testing cannot catch this class: "the two are computed by different code over the same ledger, which is the divergence no amount of per-surface testing catches, because each surface is individually self-consistent." `getPublicStats` and `loadReviewParityRollups` remain separate reads with separate gating, so nothing stopped the next change to either from reintroducing the disagreement with every existing test still green. These build BOTH halves over one seeded ledger and compare them through the verifier's own `checkStatsParity` -- the function that printed the original FAIL against the Orb -- rather than restating its rule, so the test cannot drift from the tool that decides whether production is publishing a contradiction. Covers the case a hand-written equality assertion would get wrong: a re-decided pull request appends a verdict row, so parity counts 3 while `handled` counts 2 pull requests. That is consistent, not a contradiction, and only parity EXCEEDING all-time handled is the accounting error. Mutation-tested against #9967's fix: reverting the ledger fallback fails exactly the two behavioural assertions, while the empty-ledger case and the failing-direction guard correctly stay green. Closes #9971
loopover-orb Bot
pushed a commit
that referenced
this pull request
Jul 31, 2026
… it (#9972) #9963 was an Orb publishing `totals.handled: 0` in the same payload as `reviewParity.verdicts: 2123`, both derived from the same ledger. #9967 fixed the aggregate and covered it well -- but per surface, which is the blind spot the defect lived in. Both halves were individually correct and individually green while production served the contradiction, and the verifier's own source says why per-surface testing cannot catch this class: "the two are computed by different code over the same ledger, which is the divergence no amount of per-surface testing catches, because each surface is individually self-consistent." `getPublicStats` and `loadReviewParityRollups` remain separate reads with separate gating, so nothing stopped the next change to either from reintroducing the disagreement with every existing test still green. These build BOTH halves over one seeded ledger and compare them through the verifier's own `checkStatsParity` -- the function that printed the original FAIL against the Orb -- rather than restating its rule, so the test cannot drift from the tool that decides whether production is publishing a contradiction. Covers the case a hand-written equality assertion would get wrong: a re-decided pull request appends a verdict row, so parity counts 3 while `handled` counts 2 pull requests. That is consistent, not a contradiction, and only parity EXCEEDING all-time handled is the accounting error. Mutation-tested against #9967's fix: reverting the ledger fallback fails exactly the two behavioural assertions, while the empty-ledger case and the failing-direction guard correctly stay green. Closes #9971
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9963
The root cause is not what the issue assumed
The issue attributed this to a D1/Postgres store split. It isn’t — I traced it end to end on
edge-nl-01and the store is fine.totals.*is gated onLOOPOVER_PUBLIC_STATS_REPOS, which is unset on the Orb, soprojectsis[]andgetPublicStatstakes 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.I ruled out both portability traps that produce exactly this symptom:
instr()genuinely does not exist in Postgres (ERROR: function instr(text, unknown) does not exist) — butpg-dialect.tsrewrites it. Verified by running the realtranslateInstr/translateFunctionsoverPUBLISHED_PR_KEYS.FROM (...)is accepted by PG16.The disposition query returns 9,657 against the live store. Nothing was broken about the query, the dialect layer, or the database.
The fix
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:
byProjectstill requires the allowlist and stays empty, so no repo that was not opted in is identified.totalscarries no repo identity at all.DISTINCT (repo, number)deliberately —decision_recordsholds one row per verdict, 2,354 rows over 987 pull requests on the production Orb. Counting rows would overstatehandledby more than 2× and reintroduce the same class of cross-surface disagreement this removes. Live figures: 987 handled / 488 merged / 497 closed.An existing test that was passing dishonestly
"skips the own-ledger queries ... when the allowlist is empty"threw on any non-Orb query. My change deliberately makes one — and the test still passed, becausesafeAllswallowed the throw and the result degraded to the same zeros.Narrowed to the contract that actually matters: the per-project queries still must not run without an allowlist, and the aggregate read is asserted to carry no
GROUP BY, which is what keeps repo identity unpublished.Tests
The regression (ledger-backed
handled,byProjectstill empty), the verifier’s own invariant across several ledger sizes (handledcan never be 0 while the ledger holds verdicts), no-ledger keeps its zeros rather than inventing a figure, andSUM()→NULLfrom an empty join reading as 0 rather than NaN.handledis typed non-nullable becauseCOUNT(*)cannot be NULL — that removed a?? 0no input could reach.Full suite: 25,894 passing, zero failures. Changed-line coverage: zero uncovered statements, zero uncovered branches.