feat(retention): bound orb_signals by folding it into per-day rollups - #9796
Conversation
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 11:24:29 UTC
Review summary Blockers
Nits — 5 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed 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.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
orb_signals had no retention rule and grew without bound -- one row per PR
per self-host instance, forever. It could not simply be added to
RETENTION_POLICY, because two live surfaces read history the raw rows carry:
* listFleetInstances reports a LIFETIME signalCount per instance, which a
plain prune would silently redefine as "signals in the retention window"
with no change of label;
* /v1/internal/fleet/analytics takes an arbitrary ?days=, and the public
weekly fleet trend (#9676) looks back 8 weeks -- both would report windows
they no longer had data for.
So this is the #9474 pattern: fold the aging rows into a durable aggregate in
the SAME transaction that deletes them. orb_signal_rollups keeps the whole
confusion matrix per instance per day rather than a scalar count, because the
fleet numbers are not a total: foldInstance derives decisionAccuracy from
(verdict, outcome, reversal_flag, gate_reasoncode_bucket) cells, so a scalar
rollup would preserve signalCount and destroy every accuracy figure past the
window. Per-day cells preserve both -- a historical week reconstructs to the
same number it showed while the raw rows existed.
Reads union the two sources: computeFleetAnalytics, loadPublicFleetAccuracyTrend
and listFleetInstances each add the rollups back in, with the registered-instance
trust gate applied at READ time on both halves. The fold deliberately keeps
unregistered instances' history, since an instance can be registered after its
signals arrive and discarding it at prune time would make that later
registration silently lossy.
What is genuinely dropped is per-PR identity (repo_hash/pr_hash) -- per-instance
HMACs that cannot be reversed or joined across instances, whose only role is the
ingest dedup key, which matters while a row can still be re-exported and not
after it has aged out.
Closes #9783
c02ad6a to
a2eb304
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9796 +/- ##
=======================================
Coverage 90.46% 90.46%
=======================================
Files 918 918
Lines 113940 113957 +17
Branches 27053 27060 +7
=======================================
+ Hits 103079 103096 +17
Misses 9533 9533
Partials 1328 1328
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…upport Review catch on the orb_signals rollup: the confusion matrix folds into per-day cells and sums back exactly, but cycleP50Ms/cycleP95Ms are percentiles over individual time_to_close_ms values. Percentiles are not summable, so no per-day count can reconstruct them -- and orb_signal_rollups deliberately stores counts rather than a sample of durations, since an unbounded sample is the growth problem retention exists to solve. So computeFleetAnalytics' cycle query, left reading raw orb_signals, would compute percentiles from only the rows that survived the prune and report them as describing the whole requested window. windowDays is clamped to 365 while orb_signals now prunes at 90, so ?days= beyond 90 silently narrows -- the exact failure this change prevents for the matrix. The percentiles now go null past the retention horizon, with cycleTimeObservable saying why: the posture gamingDetectionEligible and fleetFramingEligible already established, where a surface labels a gap instead of being handed a number that quietly means something narrower than its window. The horizon is read from RETENTION_POLICY rather than restated, so the window this module trusts cannot drift from the one the prune enforces. The default 90-day window -- the public headline -- is unaffected: it equals the retention window, so cycle time stays fully observable there. Existing consumers (federated-bundle/federated-import) already type these as number | null.
|
Good catch — that was a real gap, now fixed in efb886a. Why the fold genuinely can't carry it. The confusion matrix survives because per-day cells sum. What it does instead. Past the retention horizon the percentiles go The horizon comes from The default window is unaffected. Four tests added, including your exact scenario (
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | f31fdeb | Commit Preview URL Branch Preview URL |
Jul 29 2026, 10:55 AM |
Closes #9783
The problem
orb_signalshad noRETENTION_POLICYrule and grew without bound — one row per PR per self-host instance, forever.It could not simply be added to the policy, because two live surfaces read history the raw rows carry:
listFleetInstances→signalCount/v1/internal/fleet/analytics?days=and the public weekly fleet trend (#9676, 8-week lookback)nullas their rows age outThat is the same window/denominator mismatch #9676 and #9793 have been correcting, so shipping it here would have been a regression, not a cleanup.
The fix
The #9474 pattern (
orb_pr_outcomes→orb_outcome_rollups), which exists for exactly this shape: fold the aging rows into a durable aggregate in the same transaction that deletes them.orb_signal_rollupskeeps the whole confusion matrix per instance per day, not a scalar count. The fleet numbers are not a total —foldInstancederivesdecisionAccuracyfrom(verdict, outcome, reversal_flag, gate_reasoncode_bucket)cells, and #9775's trend buckets the same cells by day. A scalar rollup would preservesignalCountand destroy every accuracy figure past the window; per-day cells preserve both, so a historical week reconstructs to the same number it showed while the raw rows existed.Reads union the two sources —
computeFleetAnalytics,loadPublicFleetAccuracyTrend,listFleetInstances— with the registered-instance trust gate applied at read time on both halves.Decisions worth reviewing
gate_verdict/gate_reasoncode_bucketareNOT NULL DEFAULT '', unlike the nullable source columns. APRIMARY KEYover nullable columns does not work: SQLite does not enforceNOT NULLon PK columns of a rowid table and compares everyNULLas distinct, so two folds of the same day would insert duplicate cells instead of accumulating.''is safe for both readers —foldInstanceonly tests=== "merge"/=== "close"/=== "policy_action", which''fails exactly asNULLdid.repo_hash/pr_hash). Those are per-instance HMACs that cannot be reversed or joined across instances; their only role is the ingest dedup key, which matters while a row can still be re-exported and not after it has aged out.substr(?1, 1, 10)on the trend's rollup half. That query bounds on a full ISO instant whiledayisYYYY-MM-DD, and a string prefix sorts before the string it prefixes — a bareday >= ?1drops the window's first day.computeFleetAnalyticsdoes not need this, because its cutoff is already date-only. Both are commented, and the trend has a regression test that fails without the fix (verified by reverting it).Coverage
src/db/retention.ts,src/orb/analytics.ts,src/services/public-fleet-accuracy-trend.tsall at 100% lines / 100% branches. Thefleet-admin.tschange is a SQL edit inside an already-covered function.Reaching 100% branches required the multi-slice arms, so the fold gets the same three guards the
orb_pr_outcomesfold has: bounded slices, a run of tied timestamps still making progress (an exclusive boundary selects zero rows and spins forever), andmaxPerTablecapping one run — the last of which also asserts the unioned read is exact at the intermediate point, with half the cohort folded and half still raw.Validation
db:migrations:check(contiguous 0001..0202) ·db:migrations:immutable:check·db:schema-drift:check·validate:no-hand-written-js·dead-source-files:check·import-specifiers:check·tsc --noEmit— all clean. 138 tests pass across the three touched suites.No visual output touched.