⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.
Context
computeProviderTrackRecords (packages/loopover-engine/src/calibration/provider-track-record.ts:85) depends
on input ORDER for two of its maps:
// Dedupe to one signal per (provider, targetKey) pair, latest-vote-wins — matching stancesByTarget's
// last-write semantics above. loadLiveProviderTrackRecords reads raw audit rows with no dedup, so a provider
// re-reviewing the same PR (e.g. after a new push) would otherwise inflate its signals/decided/shared/consensus
// counters proportionally to its revote count while its stance reflects only the last vote (#8876).
const dedupedByProviderTarget = new Map<string, ProviderReviewSignal>();
for (const signal of signals) dedupedByProviderTarget.set(`${signal.provider} ${signal.targetKey}`, signal);
(lines 103-108), plus stancesByTarget.set(signal.provider, signal.vote === "fail") at line 100. Both are
last-write-wins, and the comment names loadLiveProviderTrackRecords as the exact reason the dedup exists.
loadLiveProviderTrackRecords (src/services/reviewer-routing.ts:71-100) is the only production caller. Its
query has no ORDER BY (lines 73-77). SQL row order without an ORDER BY is unspecified in both backends
this repo targets (D1/SQLite and the Postgres self-host), so "latest-vote-wins" is really "whichever row the
engine returned last". When a provider re-reviews the same PR after a push, which of its votes survives is not
determined by the code — and neither is that provider's precision, agreementRate, consensusRate, or
splitRate.
Those records are what computeWouldHaveRouted (src/services/reviewer-routing.ts:44) sorts on to pick a
preferredProvider, and the result is written to a durable audit event (recordRoutingShadow, line 118) that
#8229 stage 2 is specified to read as its evidence base. A nondeterministic input to a recorded "what routing
would have chosen" decision is the same unrecorded-nondeterminism class the decision-replay work
(src/review/decision-replay.ts, #9028/#9135/#9492) has been systematically eliminating everywhere else in
this subsystem — unstable iteration order rather than a clock or an RNG, but with the same consequence: the
same inputs do not reproducibly yield the same output.
Requirements
- The vote query in
loadLiveProviderTrackRecords must impose a total order: ORDER BY created_at ASC, id ASC.
created_at alone is insufficient — votes for the same PR are written in one loop
(src/queue/ai-review-orchestration.ts:905-914) and can share a timestamp — so the id tie-break is
mandatory, not optional.
- The
SELECT list must include the columns the ORDER BY needs if the backend requires it, but the
ProviderReviewSignal objects pushed at lines 88-93 must be unchanged in shape.
computeProviderTrackRecords's doc must state explicitly that it requires its signals argument to be in
ascending chronological order and that "latest-vote-wins" means "last element of the input array wins" —
the contract the caller now guarantees.
⚠️ Required pattern: mirror src/review/risk-control-wire.ts:105's
ORDER BY dr2.created_at DESC, dr2.id DESC LIMIT 1 — the same "created_at plus id tie-break" discipline
already used elsewhere in this subsystem for exactly this reason. What does NOT satisfy this issue: sorting
the signals array in JavaScript after the read (the rows carry no timestamp field once mapped, so this
would require widening ProviderReviewSignal, which is a public engine type); adding a second dedup pass
inside loadLiveProviderTrackRecords; or adding ORDER BY created_at without the id tie-break.
Deliverables
All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example adding
the ORDER BY without the test that fails on the current code — does not resolve this issue.
Test Coverage Requirements
This repo enforces 99%+ Codecov patch coverage, branch-counted. coverage.include covers BOTH src/**/*.ts
and packages/loopover-engine/src/**/*.ts, so src/services/reviewer-routing.ts and the engine module are
both measured and gated — do not assume the engine side is exempt. Both arms of every changed conditional need
a test, and the two ordering tests named above are mandatory (one is a named regression test for the
reversed-order case). The engine-side change here is doc-comment-only and adds no executable lines, so it
introduces no uncovered patch lines of its own.
Expected Outcome
Two runs of loadLiveProviderTrackRecords over the same audit_events rows produce byte-identical
ProviderTrackRecord[], so the recorded routing shadow — the evidence #8229 stage 2 is built on — is a
function of the data rather than of query-plan row order.
Links & Resources
src/services/reviewer-routing.ts:71-100, packages/loopover-engine/src/calibration/provider-track-record.ts:92-115,
the vote writer at src/queue/ai-review-orchestration.ts:905-914, the ordering precedent at
src/review/risk-control-wire.ts:99-108. Related open epic: #8229.
Context
computeProviderTrackRecords(packages/loopover-engine/src/calibration/provider-track-record.ts:85) dependson input ORDER for two of its maps:
(lines 103-108), plus
stancesByTarget.set(signal.provider, signal.vote === "fail")at line 100. Both arelast-write-wins, and the comment names
loadLiveProviderTrackRecordsas the exact reason the dedup exists.loadLiveProviderTrackRecords(src/services/reviewer-routing.ts:71-100) is the only production caller. Itsquery has no
ORDER BY(lines 73-77). SQL row order without anORDER BYis unspecified in both backendsthis repo targets (D1/SQLite and the Postgres self-host), so "latest-vote-wins" is really "whichever row the
engine returned last". When a provider re-reviews the same PR after a push, which of its votes survives is not
determined by the code — and neither is that provider's
precision,agreementRate,consensusRate, orsplitRate.Those records are what
computeWouldHaveRouted(src/services/reviewer-routing.ts:44) sorts on to pick apreferredProvider, and the result is written to a durable audit event (recordRoutingShadow, line 118) that#8229 stage 2 is specified to read as its evidence base. A nondeterministic input to a recorded "what routing
would have chosen" decision is the same unrecorded-nondeterminism class the decision-replay work
(
src/review/decision-replay.ts, #9028/#9135/#9492) has been systematically eliminating everywhere else inthis subsystem — unstable iteration order rather than a clock or an RNG, but with the same consequence: the
same inputs do not reproducibly yield the same output.
Requirements
loadLiveProviderTrackRecordsmust impose a total order:ORDER BY created_at ASC, id ASC.created_atalone is insufficient — votes for the same PR are written in one loop(
src/queue/ai-review-orchestration.ts:905-914) and can share a timestamp — so theidtie-break ismandatory, not optional.
SELECTlist must include the columns theORDER BYneeds if the backend requires it, but theProviderReviewSignalobjects pushed at lines 88-93 must be unchanged in shape.computeProviderTrackRecords's doc must state explicitly that it requires itssignalsargument to be inascending chronological order and that "latest-vote-wins" means "last element of the input array wins" —
the contract the caller now guarantees.
Deliverables
loadLiveProviderTrackRecordsinsrc/services/reviewer-routing.tsissues its vote query withORDER BY created_at ASC, id ASC.reviewer_voterows for the same(actor, target_key)inserted in reverse chronological order, the resultingProviderTrackRecordreflects the CHRONOLOGICALLY LATER vote — a test that fails against the current unordered query when the
stub returns rows newest-first.
created_atcase, asserting theidtie-break makes the outcome deterministic.computeProviderTrackRecordsdoc comment inpackages/loopover-engine/src/calibration/provider-track-record.tsstates the ascending-chronologicalinput-order requirement.
All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example adding
the
ORDER BYwithout the test that fails on the current code — does not resolve this issue.Test Coverage Requirements
This repo enforces 99%+ Codecov patch coverage, branch-counted.
coverage.includecovers BOTHsrc/**/*.tsand
packages/loopover-engine/src/**/*.ts, sosrc/services/reviewer-routing.tsand the engine module areboth measured and gated — do not assume the engine side is exempt. Both arms of every changed conditional need
a test, and the two ordering tests named above are mandatory (one is a named regression test for the
reversed-order case). The engine-side change here is doc-comment-only and adds no executable lines, so it
introduces no uncovered patch lines of its own.
Expected Outcome
Two runs of
loadLiveProviderTrackRecordsover the sameaudit_eventsrows produce byte-identicalProviderTrackRecord[], so the recorded routing shadow — the evidence #8229 stage 2 is built on — is afunction of the data rather than of query-plan row order.
Links & Resources
src/services/reviewer-routing.ts:71-100,packages/loopover-engine/src/calibration/provider-track-record.ts:92-115,the vote writer at
src/queue/ai-review-orchestration.ts:905-914, the ordering precedent atsrc/review/risk-control-wire.ts:99-108. Related open epic: #8229.