From 89c3d6b1483b899f1d25ab025cc2de3524b7f053 Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Thu, 30 Jul 2026 02:47:48 +0800 Subject: [PATCH] fix(routing): impose a total order on the reviewer-vote read so latest-vote-wins is deterministic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #9638 --- .../src/calibration/provider-track-record.ts | 6 ++ src/services/reviewer-routing.ts | 6 +- test/unit/reviewer-routing.test.ts | 58 +++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/packages/loopover-engine/src/calibration/provider-track-record.ts b/packages/loopover-engine/src/calibration/provider-track-record.ts index 167d6c4704..6d214e66f1 100644 --- a/packages/loopover-engine/src/calibration/provider-track-record.ts +++ b/packages/loopover-engine/src/calibration/provider-track-record.ts @@ -81,6 +81,12 @@ function toRecord(provider: string, repoFullName: string | null, stats: MutableS * a labeled corpus, per the join semantics documented in this module's header. Deterministic ordering: * providers ascending, and within each provider the overall rollup (repoFullName null) first, then repos * ascending. Aggregates only — provider ids, repo names, and numbers; never target keys or vote payloads. + * + * INPUT-ORDER CONTRACT: `signals` MUST already be in ascending chronological order. The per-(provider, + * targetKey) dedup below is last-write-wins, which here means the LAST element of the input array wins — so a + * provider that re-reviews the same PR is represented by its latest vote ONLY IF the caller feeds rows + * oldest-first. The sole production caller, `loadLiveProviderTrackRecords`, guarantees this with + * `ORDER BY created_at ASC, id ASC` (#9638); an unordered feed makes the surviving stance nondeterministic. */ export function computeProviderTrackRecords( signals: readonly ProviderReviewSignal[], diff --git a/src/services/reviewer-routing.ts b/src/services/reviewer-routing.ts index ce29a88d88..fc7793feaa 100644 --- a/src/services/reviewer-routing.ts +++ b/src/services/reviewer-routing.ts @@ -71,7 +71,11 @@ export function computeWouldHaveRouted( export async function loadLiveProviderTrackRecords(env: Env, nowMs: number = Date.now()): Promise { try { const votes = await env.DB.prepare( - "SELECT actor, target_key, metadata_json FROM audit_events WHERE event_type = ? AND created_at >= ?", + // ORDER BY is mandatory, not cosmetic: computeProviderTrackRecords dedupes latest-vote-wins by taking + // the LAST array element, so an unordered scan makes a re-voted PR's surviving stance backend-dependent + // (#9638). created_at alone is insufficient — same-loop votes share a timestamp — so the id tie-break + // is required, mirroring risk-control-wire.ts's `created_at, id` discipline. + "SELECT actor, target_key, metadata_json FROM audit_events WHERE event_type = ? AND created_at >= ? ORDER BY created_at ASC, id ASC", ) .bind(REVIEWER_VOTE_EVENT_TYPE, new Date(nowMs - CORPUS_LOOKBACK_MS).toISOString()) .all<{ actor: string; target_key: string; metadata_json: string }>(); diff --git a/test/unit/reviewer-routing.test.ts b/test/unit/reviewer-routing.test.ts index da6461dd8a..e58b2cf645 100644 --- a/test/unit/reviewer-routing.test.ts +++ b/test/unit/reviewer-routing.test.ts @@ -104,6 +104,64 @@ describe("loadLiveProviderTrackRecords (#8229 stage 1 read path)", () => { env.DB = { prepare: () => { throw new Error("store down"); } } as never; expect(await loadLiveProviderTrackRecords(env)).toEqual([]); }); + + it("REGRESSION: the vote read is totally ordered (ORDER BY created_at ASC, id ASC) so the LATER vote wins a re-review (#9638)", async () => { + const env = createTestEnv(); + // Capture the vote-read SQL to pin the total order in the query itself — the tie-break survives even a + // backend/index whose default scan order happens to already be chronological, which a behavioural + // assertion alone cannot distinguish from a load-bearing ORDER BY. + const realPrepare = env.DB.prepare.bind(env.DB); + let voteSql = ""; + env.DB.prepare = ((sql: string) => { + if (sql.includes("FROM audit_events WHERE event_type")) voteSql = sql; + return realPrepare(sql); + }) as never; + + const store = createSignalStore(env); + const now = Date.now(); + const targetKey = `${REPO}#1`; + // A confirmed target: a fail vote scores precision 1; a non_fail vote leaves failDecided 0 ⇒ precision null. + await store.recordRuleFired({ ruleId: "ai_consensus_defect", targetKey, outcome: "close", occurredAt: new Date(now - 10_000).toISOString(), metadata: { confidence: 0.97 } }); + await store.recordHumanOverride({ ruleId: "ai_consensus_defect", targetKey, verdict: "confirmed", occurredAt: new Date(now - 5_000).toISOString() }); + // Stored NEWEST-FIRST (later "fail" vote gets the lower rowid); the ascending order must still surface the + // later vote last so the last-write-wins dedup keeps it. + await env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES ('v-late', ?, 'claude-code', ?, 'completed', 'v', ?, ?)") + .bind(REVIEWER_VOTE_EVENT_TYPE, targetKey, JSON.stringify({ repoFullName: REPO, vote: "fail" }), new Date(now - 1_000).toISOString()) + .run(); + await env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES ('v-early', ?, 'claude-code', ?, 'completed', 'v', ?, ?)") + .bind(REVIEWER_VOTE_EVENT_TYPE, targetKey, JSON.stringify({ repoFullName: REPO, vote: "non_fail" }), new Date(now - 3_000).toISOString()) + .run(); + + const records = await loadLiveProviderTrackRecords(env, now); + const repoRow = records.find((row) => row.provider === "claude-code" && row.repoFullName === REPO)!; + expect(voteSql).toContain("ORDER BY created_at ASC, id ASC"); // the total order is in the query, not left to the backend + expect(repoRow.signals).toBe(1); // the two votes dedupe to a single signal + expect(repoRow.precision).toBe(1); // the LATER fail vote wins; null would mean the earlier vote leaked through + }); + + it("REGRESSION: the id tie-break makes the winner deterministic when two votes share a created_at (#9638)", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const now = Date.now(); + const targetKey = `${REPO}#2`; + await store.recordRuleFired({ ruleId: "ai_consensus_defect", targetKey, outcome: "close", occurredAt: new Date(now - 10_000).toISOString(), metadata: { confidence: 0.97 } }); + await store.recordHumanOverride({ ruleId: "ai_consensus_defect", targetKey, verdict: "confirmed", occurredAt: new Date(now - 5_000).toISOString() }); + // Same created_at, so ordering falls entirely to the id tie-break: 'z-fail' > 'a-nonfail' lexicographically, + // so the fail vote sorts LAST and wins deterministically. Inserted higher-id-first so an unordered rowid + // scan would instead keep 'a-nonfail' ⇒ precision null; the ORDER BY id ASC is what makes it reproducible. + const shared = new Date(now - 2_000).toISOString(); + await env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES ('z-fail', ?, 'claude-code', ?, 'completed', 'v', ?, ?)") + .bind(REVIEWER_VOTE_EVENT_TYPE, targetKey, JSON.stringify({ repoFullName: REPO, vote: "fail" }), shared) + .run(); + await env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES ('a-nonfail', ?, 'claude-code', ?, 'completed', 'v', ?, ?)") + .bind(REVIEWER_VOTE_EVENT_TYPE, targetKey, JSON.stringify({ repoFullName: REPO, vote: "non_fail" }), shared) + .run(); + + const records = await loadLiveProviderTrackRecords(env, now); + const repoRow = records.find((row) => row.provider === "claude-code" && row.repoFullName === REPO)!; + expect(repoRow.signals).toBe(1); + expect(repoRow.precision).toBe(1); // 'z-fail' sorts last on the id tie-break and wins deterministically + }); }); describe("recordRoutingShadow (#8229 stage 1 write path)", () => {