fix(stats): bound the accuracy denominator to the window its numerator survives in - #9768
Merged
Merged
Conversation
…r survives in #9718 gated accuracy on whether a reversal is observable at all, and fixed the weekly trend's Orb-folded denominator. It did not fix the third asymmetry in #9676: `merged`/`closed` come from `github_app.pr_public_surface_published`, the single retention-EXEMPT audit event type, so they are lifetime and never shrink -- while `reversal_*` rows prune with the rest of audit_events at 90 days. Pairing an immortal denominator with a 90-day numerator makes `1 - reversed/decided` drift toward 100% as the ledger ages, independent of real reversal behavior. Confirmed live after #9718 deployed: byProject still published 100% for all three repos on 2377/602/508 reviewed with 0 reversals, because the reversal signal IS observable on that deployment -- the gate passed and the ratio was still meaningless. The accuracy denominator is now a second, retention-windowed disposition query that shares BOTH of the numerator's bounds: own-ledger only, and inside audit_events' retention window. `reviewed`/`merged`/`closed` keep publishing lifetime volume, which is measured and correct. That display-vs-denominator split is the same shape #7449 established for the Orb fold and #9718 carried into the weekly trend; the #7449 snapshot it supersedes is removed rather than left dead, with its reasoning folded into the new comment. Boundary case, stated rather than hidden: a PR published before the window but reversed inside it contributes to the numerator and not the denominator. accuracyPct's existing clamp already handles that (it is the same shape as the reopened-auto-close case its comment describes), and the alternative -- an unbounded denominator -- is the bug being fixed. Refs #9676
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 08:07:33 UTC
Review summary Nits — 6 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9768 +/- ##
==========================================
- Coverage 90.28% 89.46% -0.82%
==========================================
Files 907 907
Lines 113363 113369 +6
Branches 26892 26894 +2
==========================================
- Hits 102349 101431 -918
- Misses 9683 10849 +1166
+ Partials 1331 1089 -242
Flags with carried forward coverage won't be shown. Click here to find out more.
|
… that still exist Review catch on #9768. The comment above the global accuracyPct call site still pointed at "the ownLedgerMerged/ownLedgerClosed snapshot above the Orb fold" -- variables this PR removed and replaced with windowedMerged/windowedClosed. A comment referring a reader to a symbol that no longer exists is worse than none: it sends them looking for context that was deliberately relocated. Retargeted at the windowed disposition query, and widened while there. The old text described ONE of the two bounds the pairing needs (same population, so the fleet fold cannot inflate it); it now names both, since this PR added the second (same retention window, so an immortal denominator cannot outlive its numerator). `ownLedgerReviewed`/`ownLedgerMinutes` are untouched -- different variables, both still live, feeding the minutes-saved calculation.
Owner
Author
|
Good catch — fixed in the latest push. You're right that the comment above the global Retargeted at the windowed disposition query, and widened while there. The old text described only one of the two bounds the pairing needs — same population, so the fleet fold can't inflate it. It now names both, since this PR adds the second: same retention window, so an immortal denominator can't outlive its numerator. That second bound is the actual subject of the PR, so the comment was under-describing the code even after the rename was corrected. For the record,
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 03ac9e1 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 07:51 AM |
Bundle ReportBundle size has no change ✅ |
11 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
…s something again (#9775) After #9718 and #9768 the own-ledger weekly trend is correct and, for recent weeks, correctly null: the hosted Worker does not execute reviews, so that ledger is frozen while Orb volume keeps growing. A column of nulls beside a rising volume column is honest and tells a reader nothing about how the gate behaves today. orb_signals already carries everything needed, with no new ingest, no new column and no new secret: self-host runtimes export gate_verdict/outcome/reversal_flag per PR, the hosted side validates and stores them, and computeFleetAnalytics already turns them into the 90-day headline. This buckets the SAME rows weekly. Two deliberate non-choices, both of which would have reintroduced the bug class this surface is being corrected for: - It is a SEPARATE series, never blended with the own-ledger one. orb_signals is keyed by per-instance HMACs of repo/PR; audit_events and orb_pr_outcomes use raw owner/repo#number. The populations cannot be joined, so a reversal from one can never be attributed to a decided row in the other, and one number over a mixed denominator would be exactly the defect #9676 reports. - It does NOT use 1 - reversalRate. #8820 established decisionAccuracy as the fleet estimand because the reversal formula divides by holds -- deferrals that cannot be right or wrong -- and misses mispredictions carrying no reversal marker. A weekly series on a different estimand than the headline directly above it would make the page disagree with itself. The weekly fold calls foldInstance rather than reimplementing the confusion matrix. That function is already exported for the federated bundle with a doc comment warning that a second copy makes comparisons apples-to-oranges, and the accounting is subtle: policy_action rows are excluded from scoring but still count as activity, and a superseded close is disconfirmed exactly like a reopen. Only registered instances count, matching computeFleetAnalytics' own trust gate -- the ingest is open, so a stranger's signals must not move a published number until a human opts them in. There is a test pinning that. Refs #9676
This was referenced Jul 29, 2026
orb_signals grows unbounded, and adding retention would silently corrupt two existing surfaces
#9783
Closed
JSONbored
added a commit
that referenced
this pull request
Jul 29, 2026
…ed PR (#9793) Live after both #9718 and #9768 deployed: byProject still published 100% for all three repos, on 2377/602/508 reviewed PRs with zero reversals. Neither earlier fix could have caught it. Both addressed the WINDOW the denominator covers -- #9718 withheld the ratio where a reversal was unobservable, #9768 bounded an immortal denominator to its numerator's retention -- and the defect is WHICH PRs belong in it. A reversal is by definition a human overturning an engine auto-action; recordReversalSignals only records one against a PR the engine merged or closed. The denominator was `github_app.pr_public_surface_published`: every PR that got a review surface, including the many the engine only commented on, held, or advised. None of those could produce a reversal, so each one pushes 1 - reversed/decided toward 100% for reasons unrelated to gate quality. Both surfaces now divide by the distinct PRs this deployment auto-actioned in the window -- same event types, same outcome filter, same dry-run exclusion that loadReversalDayRows already applies when anchoring the numerator, so the two halves of the ratio are finally drawn from one population. The weekly trend had the same mismatch, masked only because its own-ledger series currently reports null for recent weeks; fixing one and not the other would have left two definitions of "decided" on one page. Volume columns are untouched: reviewed/merged/closed keep publishing lifetime and fleet-folded counts, which are measured and correct. Only the ratio's denominator narrows. #9718's observability probe is REMOVED rather than left beside the thing that supersedes it. No auto-actions now means decided is 0 and the answer is null by construction -- a structural guarantee, where the probe was a heuristic that could disagree with the denominator sitting next to it. Six fixtures were made faithful rather than the assertion relaxed: each modelled a decided PR with no auto-action, which is not a reachable state. The clearest is the revert-PR regression, which recorded a reversal_reverted with no agent.action.merge -- a PR cannot be reverted unless the engine merged it. Closes #9792
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.
Summary
#9718 gated accuracy on whether a reversal is observable at all, and fixed the weekly trend's Orb-folded denominator. It did not fix the third asymmetry in #9676:
merged/closedcome fromgithub_app.pr_public_surface_published— the single retention-exempt audit event type (DURABLE_AUDIT_EVENT_TYPES,src/db/retention.ts:14) — so they are lifetime and never shrink.reversal_*rows prune with the rest ofaudit_eventsat 90 days. Pairing an immortal denominator with a 90-day numerator makes1 - reversed/decideddrift toward 100% as the ledger ages, independent of real reversal behavior.Confirmed live after #9718 deployed:
The observability gate #9718 added passed here — the reversal signal genuinely is observable on that deployment — and the ratio was still meaningless. Two different bugs; the first fix couldn't have caught this one.
The fix
A second, retention-windowed disposition query supplies the accuracy denominator. It shares both of the numerator's bounds: own-ledger only, and inside
audit_events' retention window.reviewed/merged/closedkeep publishing lifetime volume, which is measured and correct.That display-vs-denominator split is the same shape #7449 established for the Orb fold and #9718 carried into the weekly trend. The #7449 snapshot this supersedes (
ownLedgerMerged/ownLedgerClosed) is removed rather than left dead, with its reasoning folded into the new comment.Boundary case, stated rather than hidden: a PR published before the window but reversed inside it contributes to the numerator and not the denominator.
accuracyPct's existing clamp already handles that — it's the same shape as the reopened-auto-close case its comment already describes — and the alternative, an unbounded denominator, is the bug being fixed.Refs #9676
Validation
The stub-based tests route both disposition queries to the same rows, so they pass either way and prove nothing about the windowing. The regression test is therefore real-D1: four merged PRs published 200 days ago plus two recent ones (one auto-closed and reversed, one merged and standing).
totals.reviewedtotals.mergedtotals.reversedbyProject[0].accuracyPcttotals.accuracyPctUnder the old pairing that same fixture published
1 - 1/5 = 80%.public-stats.ts: 0 uncovered, 0 partial branches.public-stats+public-stats-route, including the existingREGRESSION (#7449)test, which still passes — the property it pins is preserved, not replaced.typecheck,ui:openapi:check,git diff --checkall clean.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.No auth/CORS/session change, and no UI file is touched — the fairness page already renders whatever the block returns, and #9718 shipped the copy explaining a withheld or lowered accuracy.
ui:openapi:checkconfirms the response shape is unchanged:accuracyPctwas already nullable, and this narrows what it is computed from, not what it can be.