From 0600a752e486536f1efbd4de5a13fbd314efe1c0 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:29:30 +0900 Subject: [PATCH] fix(services): bound the reviewer_vote track-record read to the newest N votes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadLiveProviderTrackRecords ran an unbounded SELECT over a 90-day slice of audit_events (raw rows, a metadata_json blob each) on every block-mode dual review, and swallowed any failure into []. An oversized result set throws, lands in that catch, and becomes 'no track records' — which computeWouldHaveRouted reads as 'below the decided floor' and indistinguishable from the legitimate no-signal case the module's invariants require. So the stage-1 shadow silently stops recording exactly as the ledger grows large enough for stage 2 to be worth shipping against, with nothing anywhere saying so. Bound the scan to the newest REVIEWER_VOTE_SCAN_LIMIT rows via a newest-first subquery re-ordered ascending in the outer projection (a naive ASC ... LIMIT would keep the OLDEST rows and invert the latest-vote-wins dedup #9638 established). Warn reviewer_vote_scan_truncated at a full page and reviewer_vote_scan_failed in the catch before failing safe, so a truncated or failed read is observable rather than silently under-counting. computeWouldHaveRouted, the corpus lookback window, the corrupt-row skip, and the fail-safe [] posture are all unchanged. Closes #10023 --- src/services/reviewer-routing.ts | 30 ++++++++++--- test/unit/reviewer-routing.test.ts | 67 ++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/src/services/reviewer-routing.ts b/src/services/reviewer-routing.ts index fc7793feaa..15d224a597 100644 --- a/src/services/reviewer-routing.ts +++ b/src/services/reviewer-routing.ts @@ -15,6 +15,7 @@ import { buildBacktestCorpus, computeProviderTrackRecords, type ProviderReviewSignal, type ProviderTrackRecord } from "@loopover/engine"; import { createSignalStore } from "../review/signal-tracking-wire"; import { recordAuditEvent } from "../db/repositories"; +import { errorMessage } from "../utils/json"; /** Audit event type a shadow decision writes — ONE stable type forever (the #8159 event discipline). */ export const REVIEWER_ROUTING_SHADOW_EVENT_TYPE = "reviewer_routing_shadow"; @@ -26,6 +27,13 @@ export const ROUTING_MIN_DECIDED = 10; /** The trailing window the track-record read replays — mirrors the calibration corpus lookback. */ const CORPUS_LOOKBACK_MS = 90 * 24 * 60 * 60 * 1000; +/** Max reviewer_vote rows the track-record read scans in one pass (#10023). The read runs on every block-mode + * dual review and pulls RAW rows (a metadata_json blob each), so an unbounded scan over a 90-day slice of an + * ever-growing table would eventually throw and be swallowed into "no evidence". Bounds the worst-case cost to + * the NEWEST this many votes, matching the bounded-read precedent in knob-loosening-run.ts. The newest 2,000 + * votes are an ample latest-vote-wins corpus; older ones fall outside the window the shadow acts on anyway. */ +export const REVIEWER_VOTE_SCAN_LIMIT = 2_000; + export type RoutingShadowDecision = { repoFullName: string; preferredProvider: string; @@ -75,13 +83,22 @@ export async function loadLiveProviderTrackRecords(env: Env, nowMs: number = Dat // 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", + // #10023: bound the scan to the NEWEST REVIEWER_VOTE_SCAN_LIMIT rows. A naive `ASC ... LIMIT n` would + // keep the OLDEST rows and invert the latest-vote-wins dedup, so select newest-first in a subquery and + // re-order the outer projection ASC, so computeProviderTrackRecords still receives ascending order. + "SELECT actor, target_key, metadata_json FROM (SELECT actor, target_key, metadata_json, created_at, id FROM audit_events WHERE event_type = ? AND created_at >= ? ORDER BY created_at DESC, id DESC LIMIT ?) ORDER BY created_at ASC, id ASC", ) - .bind(REVIEWER_VOTE_EVENT_TYPE, new Date(nowMs - CORPUS_LOOKBACK_MS).toISOString()) + .bind(REVIEWER_VOTE_EVENT_TYPE, new Date(nowMs - CORPUS_LOOKBACK_MS).toISOString(), REVIEWER_VOTE_SCAN_LIMIT) .all<{ actor: string; target_key: string; metadata_json: string }>(); - const signals: ProviderReviewSignal[] = []; /* v8 ignore next -- defined-results guard, the loadKnobStatus convention */ - for (const row of votes.results ?? []) { + const rows = votes.results ?? []; + if (rows.length === REVIEWER_VOTE_SCAN_LIMIT) { + // A full page means the corpus was truncated: the read under-counts, so say so rather than let the + // shadow silently under-record as the ledger grows — the distinction stage 2's rollout depends on. + console.warn(JSON.stringify({ event: "reviewer_vote_scan_truncated", rows: rows.length })); + } + const signals: ProviderReviewSignal[] = []; + for (const row of rows) { let metadata: { repoFullName?: unknown; vote?: unknown } = {}; try { metadata = JSON.parse(row.metadata_json) as { repoFullName?: unknown; vote?: unknown }; @@ -98,7 +115,10 @@ export async function loadLiveProviderTrackRecords(env: Env, nowMs: number = Dat } const { fired, overrides } = await createSignalStore(env).queryRuleHistory("ai_consensus_defect", nowMs - CORPUS_LOOKBACK_MS); return computeProviderTrackRecords(signals, buildBacktestCorpus("ai_consensus_defect", fired, overrides)); - } catch { + } catch (error) { + // #10023: say the read FAILED before failing safe, so "the shadow could not read its evidence" is + // distinguishable in the logs from a genuinely empty ledger — the two the stage-2 decision must tell apart. + console.warn(JSON.stringify({ event: "reviewer_vote_scan_failed", message: errorMessage(error).slice(0, 200) })); return []; // fail-safe: no records ⇒ downstream records nothing ⇒ byte-identical behavior } } diff --git a/test/unit/reviewer-routing.test.ts b/test/unit/reviewer-routing.test.ts index e58b2cf645..6c167671af 100644 --- a/test/unit/reviewer-routing.test.ts +++ b/test/unit/reviewer-routing.test.ts @@ -8,6 +8,7 @@ import { recordRoutingShadow, REVIEWER_ROUTING_SHADOW_EVENT_TYPE, REVIEWER_VOTE_EVENT_TYPE, + REVIEWER_VOTE_SCAN_LIMIT, ROUTING_MIN_DECIDED, } from "../../src/services/reviewer-routing"; import { buildRoutingRecapSection } from "../../src/services/maintainer-recap-routing"; @@ -105,6 +106,72 @@ describe("loadLiveProviderTrackRecords (#8229 stage 1 read path)", () => { expect(await loadLiveProviderTrackRecords(env)).toEqual([]); }); + it("#10023 REGRESSION: a thrown read warns reviewer_vote_scan_failed AND returns [] (distinguishable from an empty ledger)", async () => { + const env = createTestEnv(); + env.DB = { prepare: () => { throw new Error("boom-read"); } } as never; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect(await loadLiveProviderTrackRecords(env)).toEqual([]); + const failWarns = warn.mock.calls.map((c) => String(c[0])).filter((m) => m.includes("reviewer_vote_scan_failed")); + expect(failWarns).toHaveLength(1); + expect(failWarns[0]).toContain("boom-read"); + // recordRoutingShadow still resolves null on the failed read, never touching the review path. + expect(await recordRoutingShadow(env, { repoFullName: REPO, prNumber: 9, actualProviders: ["claude-code", "codex"] })).toBeNull(); + }); + + it("#10023: the read keeps the NEWEST REVIEWER_VOTE_SCAN_LIMIT votes, dropping the oldest overflow", async () => { + const env = createTestEnv(); + const store = createSignalStore(env); + const base = Date.parse("2026-06-01T00:00:00Z"); + const targetKey = `${REPO}#newest`; + // A labeled corpus entry so the newest provider's votes have something to join to. + await store.recordRuleFired({ ruleId: "ai_consensus_defect", targetKey, outcome: "close", occurredAt: new Date(base).toISOString(), metadata: { confidence: 0.97 } }); + await store.recordHumanOverride({ ruleId: "ai_consensus_defect", targetKey, verdict: "confirmed", occurredAt: new Date(base + 1).toISOString() }); + // Seed LIMIT + 5 votes newest-last by created_at: the OLDEST 5 (provider "stale") must fall outside the + // newest-LIMIT window, and the newest vote (provider "fresh") must survive. + const total = REVIEWER_VOTE_SCAN_LIMIT + 5; + const stmts = Array.from({ length: total }, (_, i) => { + const provider = i < 5 ? "stale" : i === total - 1 ? "fresh" : "filler"; + return env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?, ?, ?, ?, 'completed', 'v', ?, ?)") + .bind(`vote-${String(i).padStart(6, "0")}`, REVIEWER_VOTE_EVENT_TYPE, provider, targetKey, JSON.stringify({ repoFullName: REPO, vote: "fail" }), new Date(base + 1_000 + i).toISOString()); + }); + await env.DB.batch(stmts); + + const records = await loadLiveProviderTrackRecords(env, base + 10_000_000); + const providers = new Set(records.map((r) => r.provider)); + expect(providers.has("fresh")).toBe(true); // the newest vote survived the bound + expect(providers.has("stale")).toBe(false); // the oldest 5 fell outside the newest-LIMIT window + }, 60_000); + + it("#10023: a full page (rows === REVIEWER_VOTE_SCAN_LIMIT) warns reviewer_vote_scan_truncated; a shorter read does not", async () => { + const env = createTestEnv(); + const base = Date.parse("2026-06-01T00:00:00Z"); + const seed = async (count: number): Promise => { + const stmts = Array.from({ length: count }, (_, i) => + env.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?, ?, 'p', ?, 'completed', 'v', ?, ?)") + .bind(`t-${String(i).padStart(6, "0")}`, REVIEWER_VOTE_EVENT_TYPE, `${REPO}#${i}`, JSON.stringify({ repoFullName: REPO, vote: "fail" }), new Date(base + i).toISOString()), + ); + await env.DB.batch(stmts); + }; + // Exactly at the limit → truncation warn. + await seed(REVIEWER_VOTE_SCAN_LIMIT); + const warnAt = vi.spyOn(console, "warn").mockImplementation(() => {}); + await loadLiveProviderTrackRecords(env, base + 10_000_000); + expect(warnAt.mock.calls.map((c) => String(c[0])).some((m) => m.includes("reviewer_vote_scan_truncated"))).toBe(true); + warnAt.mockRestore(); + + // A fresh env below the limit → no truncation warn. + const env2 = createTestEnv(); + const stmts = Array.from({ length: 3 }, (_, i) => + env2.DB.prepare("INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?, ?, 'p', ?, 'completed', 'v', ?, ?)") + .bind(`u-${i}`, REVIEWER_VOTE_EVENT_TYPE, `${REPO}#${i}`, JSON.stringify({ repoFullName: REPO, vote: "fail" }), new Date(base + i).toISOString()), + ); + await env2.DB.batch(stmts); + const warnBelow = vi.spyOn(console, "warn").mockImplementation(() => {}); + await loadLiveProviderTrackRecords(env2, base + 10_000_000); + expect(warnBelow.mock.calls.map((c) => String(c[0])).some((m) => m.includes("reviewer_vote_scan_truncated"))).toBe(false); + warnBelow.mockRestore(); + }, 60_000); + 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