fix(routing): impose a total order on the reviewer-vote read so latest-vote-wins is deterministic - #9885
Conversation
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 19:06:28 UTC
Review summary Nits — 3 non-blocking
Decision 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. 🟩 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. |
…t-vote-wins is deterministic computeProviderTrackRecords dedupes reviewer signals per (provider, targetKey) with last-write-wins, taking the last element of its input array. Its only production caller, loadLiveProviderTrackRecords, read the reviewer_vote rows with no ORDER BY, so under a re-review the surviving vote — and thus the provider's precision, agreementRate, consensusRate, and splitRate — depended on unspecified SQL row order, differing across the D1/SQLite and Postgres backends this repo targets. That record is what computeWouldHaveRouted writes into a durable routing-shadow audit event, so the nondeterminism reached recorded evidence. Add ORDER BY created_at ASC, id ASC to the vote query, mirroring risk-control-wire.ts's created_at+id tie-break. created_at alone is insufficient — votes for one PR are written in a single loop and can share a timestamp — so the id tie-break is mandatory. Document the ascending-chronological input-order contract on computeProviderTrackRecords. Closes JSONbored#9638
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9885 +/- ##
==========================================
- Coverage 91.75% 91.74% -0.01%
==========================================
Files 919 919
Lines 112987 112993 +6
Branches 27193 27193
==========================================
+ Hits 103667 103669 +2
Misses 8034 8034
- Partials 1286 1290 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
computeProviderTrackRecords(packages/loopover-engine/src/calibration/provider-track-record.ts) dedupes reviewer signals per(provider, targetKey)with last-write-wins, keeping the last element of its input array (the#8876re-vote-inflation guard). Its only production caller,loadLiveProviderTrackRecords(src/services/reviewer-routing.ts), read thereviewer_voterows with noORDER BY.SQL row order without
ORDER BYis unspecified in both backends this repo targets (D1/SQLite and the Postgres self-host), so when a provider re-reviews the same PR after a push, which vote survives — and therefore that provider'sprecision,agreementRate,consensusRate, andsplitRate— was decided by whichever row the engine happened to return last.computeWouldHaveRoutedsorts on those records to pick apreferredProvider, and the result is written to a durable routing-shadow audit event (recordRoutingShadow) that #8229 stage 2 reads as its evidence base — so the nondeterminism reached recorded evidence.Fix
loadLiveProviderTrackRecords's vote query now endsORDER BY created_at ASC, id ASC, mirroringsrc/review/risk-control-wire.ts'screated_at+idtie-break discipline.created_atalone is insufficient — votes for one PR are written in a single loop (src/queue/ai-review-orchestration.ts) and can share a timestamp — so theidtie-break is mandatory. TheSELECTlist and theProviderReviewSignalshape are unchanged.computeProviderTrackRecords's doc comment now states its input-order contract:signalsmust be ascending-chronological, "latest-vote-wins" means "last element of the input array wins", and the caller guarantees it via theORDER BY.Tests
Added to
test/unit/reviewer-routing.test.ts:ORDER BY created_at ASC, id ASC, plus that a re-reviewed PR's record reflects the chronologically later vote (fails against the unordered query).created_attest proving theidtie-break makes the surviving vote deterministic — this one fails against the current unordered query even with the in-memory backend.Full
reviewer-routingsuite passes (11/11).Closes #9638