diff --git a/src/review/parity.ts b/src/review/parity.ts index 5c7e9926cd..4b88c2fef7 100644 --- a/src/review/parity.ts +++ b/src/review/parity.ts @@ -42,6 +42,19 @@ export interface GateEvalRow { decided: number; // predictions that have a known outcome mergePrecision: number | null; closePrecision: number | null; + /** #2348: mergeConfirmed, discounted by REVERSAL_DISCOUNT_WEIGHT for any target later marked + * reversal_reverted (a merge a human subsequently undid) -- see the constant's own doc comment for the + * formula and rationale. wouldMerge (the denominator) is UNCHANGED -- only the credit for a merge that + * didn't hold up is discounted, not whether a merge was predicted at all. */ + weightedMergeConfirmed: number; + /** #2348: closeConfirmed, discounted by REVERSAL_DISCOUNT_WEIGHT for any target later marked + * reversal_reopened (a bot-closed PR a contributor disputed by reopening it). wouldClose is UNCHANGED, + * same rationale as weightedMergeConfirmed. */ + weightedCloseConfirmed: number; + /** weightedMergeConfirmed / wouldMerge, or null when wouldMerge is 0. Always <= mergePrecision. */ + weightedMergePrecision: number | null; + /** weightedCloseConfirmed / wouldClose, or null when wouldClose is 0. Always <= closePrecision. */ + weightedClosePrecision: number | null; } export interface GateEvalReport { @@ -52,12 +65,26 @@ export interface GateEvalReport { const MIN_DECIDED_FOR_SIGNAL = 10; +/** #2348: the value-weighting formula's ONE tunable knob, deliberately hardcoded (not read from config/env) + * so the objective function itself stays auditable — changing what "accuracy" measures is a code change + + * review, not a runtime toggle. 0 = a merge/close later reversed earns ZERO credit toward + * weightedMergeConfirmed/weightedCloseConfirmed (full discount): a miner or the fleet cannot game the + * accuracy number by producing high volumes of barely-passing, later-reverted PRs, because a reverted merge + * contributes nothing to the weighted-correct bucket regardless of volume. This is a DISCOUNT on credit, not + * a change to the denominator — wouldMerge/wouldClose (how many merge/close predictions were made) is + * unchanged, so weightedMergePrecision/weightedClosePrecision can only ever be <= the raw precision, never + * higher. Bump this constant's value (and this comment) if the maintainer later wants partial credit + * instead of a hard zero — never make it runtime-configurable. */ +export const REVERSAL_DISCOUNT_WEIGHT = 0; + /** Join the latest prediction (gate_decision) and the latest ground truth (pr_outcome) per target, then * fold into a per-project confusion matrix + precisions. Pure read; fail-safe → empty report. * `source` scopes the predictions to ONE writer (#preconv-parity standalone accuracy) — default the * authoritative 'reviewbot' rows when set; omit to score ALL writers' predictions as before. The * pr_outcome (ground truth) is the human's realized merge/close, so it is NOT source-scoped — both - * systems are graded against the same answer key. */ + * systems are graded against the same answer key. Also LEFT JOINs a reversal existence check (#2348) so the + * fold below can additionally compute weightedMergeConfirmed/weightedCloseConfirmed alongside the existing + * raw counts — see REVERSAL_DISCOUNT_WEIGHT's doc comment for the formula. */ export async function computeGateEval(env: Env, opts: { days: number; nowMs: number; source?: string }): Promise { const days = Number.isFinite(opts.days) && opts.days > 0 ? Math.min(opts.days, 730) : 90; const fromIso = new Date(opts.nowMs - days * 86_400_000).toISOString().slice(0, 10); @@ -74,16 +101,21 @@ export async function computeGateEval(env: Env, opts: { days: number; nowMs: num SELECT target_id, decision AS truth, MAX(created_at) AS t FROM review_audit WHERE event_type = 'pr_outcome' AND decision IS NOT NULL GROUP BY target_id + ), + rev AS ( + SELECT DISTINCT target_id FROM review_audit WHERE event_type IN ('reversal_reverted', 'reversal_reopened') ) - SELECT gd.project AS project, gd.pred AS pred, po.truth AS truth, COUNT(*) AS n + SELECT gd.project AS project, gd.pred AS pred, po.truth AS truth, + CASE WHEN rev.target_id IS NOT NULL THEN 1 ELSE 0 END AS reversed, COUNT(*) AS n FROM gd JOIN po ON gd.target_id = po.target_id - GROUP BY gd.project, gd.pred, po.truth`; + LEFT JOIN rev ON gd.target_id = rev.target_id + GROUP BY gd.project, gd.pred, po.truth, reversed`; - let cells: Array<{ project: string; pred: string; truth: string; n: number }> = []; + let cells: Array<{ project: string; pred: string; truth: string; reversed: number; n: number }> = []; try { const stmt = storage(env).prepare(sql); const bound = opts.source ? stmt.bind(fromIso, opts.source) : stmt.bind(fromIso); - const res = await bound.all<{ project: string; pred: string; truth: string; n: number }>(); + const res = await bound.all<{ project: string; pred: string; truth: string; reversed: number; n: number }>(); cells = res.results ?? []; } catch { return { rows: [], hasSignal: false }; @@ -93,7 +125,10 @@ export async function computeGateEval(env: Env, opts: { days: number; nowMs: num const row = (p: string): GateEvalRow => { let r = byProject.get(p); if (!r) { - r = { project: p, wouldMerge: 0, mergeConfirmed: 0, mergeFalse: 0, wouldClose: 0, closeConfirmed: 0, closeFalse: 0, hold: 0, decided: 0, mergePrecision: null, closePrecision: null }; + r = { + project: p, wouldMerge: 0, mergeConfirmed: 0, mergeFalse: 0, wouldClose: 0, closeConfirmed: 0, closeFalse: 0, hold: 0, decided: 0, + mergePrecision: null, closePrecision: null, weightedMergeConfirmed: 0, weightedCloseConfirmed: 0, weightedMergePrecision: null, weightedClosePrecision: null, + }; byProject.set(p, r); } return r; @@ -102,14 +137,21 @@ export async function computeGateEval(env: Env, opts: { days: number; nowMs: num for (const c of cells) { const r = row(c.project); r.decided += c.n; + // A reversed cell's credit toward the CONFIRMED (weighted) bucket is discounted by REVERSAL_DISCOUNT_WEIGHT; + // the raw (unweighted) buckets below are always the full count, byte-identical to pre-#2348 behavior. + const weightedN = c.reversed ? c.n * REVERSAL_DISCOUNT_WEIGHT : c.n; if (c.pred === "merge") { r.wouldMerge += c.n; - if (c.truth === "merged") r.mergeConfirmed += c.n; - else if (c.truth === "closed") r.mergeFalse += c.n; + if (c.truth === "merged") { + r.mergeConfirmed += c.n; + r.weightedMergeConfirmed += weightedN; + } else if (c.truth === "closed") r.mergeFalse += c.n; } else if (c.pred === "close") { r.wouldClose += c.n; - if (c.truth === "closed") r.closeConfirmed += c.n; - else if (c.truth === "merged") r.closeFalse += c.n; + if (c.truth === "closed") { + r.closeConfirmed += c.n; + r.weightedCloseConfirmed += weightedN; + } else if (c.truth === "merged") r.closeFalse += c.n; } else if (c.pred === "hold") { r.hold += c.n; } @@ -119,6 +161,8 @@ export async function computeGateEval(env: Env, opts: { days: number; nowMs: num ...r, mergePrecision: r.wouldMerge > 0 ? r.mergeConfirmed / r.wouldMerge : null, closePrecision: r.wouldClose > 0 ? r.closeConfirmed / r.wouldClose : null, + weightedMergePrecision: r.wouldMerge > 0 ? r.weightedMergeConfirmed / r.wouldMerge : null, + weightedClosePrecision: r.wouldClose > 0 ? r.weightedCloseConfirmed / r.wouldClose : null, })); rows.sort((a, b) => a.project.localeCompare(b.project)); return { rows, hasSignal: rows.some((r) => r.decided >= MIN_DECIDED_FOR_SIGNAL) }; diff --git a/test/unit/parity.test.ts b/test/unit/parity.test.ts index 08d676b747..e523265c74 100644 --- a/test/unit/parity.test.ts +++ b/test/unit/parity.test.ts @@ -6,6 +6,7 @@ import { isParityCutoverReady, MIN_PARITY_SAMPLE, PARITY_AGREEMENT_FLOOR, + REVERSAL_DISCOUNT_WEIGHT, } from "../../src/review/parity"; // NOTE: this is the SELF-CONTAINED native port of reviewbot's parity test (eval.test.ts). The reviewbot @@ -449,3 +450,95 @@ describe("computeGateEval — source scoping for per-system standalone accuracy expect(nan.binds?.[0]).toBe(new Date(NOW - 90 * 86_400_000).toISOString().slice(0, 10)); }); }); + +describe("computeGateEval — value-weighted precision (#2348, discounts a later-reversed merge/close)", () => { + it("backward-compat: a zero-reversal fixture (matching pre-#2348 cells, no `reversed` field at all) produces weighted precision identical to raw precision", async () => { + const cells = [ + { project: "p", pred: "merge", truth: "merged", n: 8 }, + { project: "p", pred: "merge", truth: "closed", n: 2 }, + { project: "p", pred: "close", truth: "closed", n: 5 }, + { project: "p", pred: "close", truth: "merged", n: 1 }, + ]; + const env = { DB: { prepare: () => ({ bind: () => ({ all: async () => ({ results: cells }) }) }) } } as unknown as Env; + const out = await computeGateEval(env, { days: 90, nowMs: NOW }); + const r = out.rows[0]; + expect(r).toBeDefined(); + if (!r) return; + expect(r.weightedMergeConfirmed).toBe(r.mergeConfirmed); + expect(r.weightedCloseConfirmed).toBe(r.closeConfirmed); + expect(r.weightedMergePrecision).toBe(r.mergePrecision); + expect(r.weightedClosePrecision).toBe(r.closePrecision); + }); + + it("discounts a reversed merge's credit toward weightedMergeConfirmed, while raw mergeConfirmed and wouldMerge (the denominator) stay unchanged", async () => { + const cells = [ + { project: "p", pred: "merge", truth: "merged", reversed: 0, n: 6 }, // held up + { project: "p", pred: "merge", truth: "merged", reversed: 1, n: 4 }, // later reverted + ]; + const env = { DB: { prepare: () => ({ bind: () => ({ all: async () => ({ results: cells }) }) }) } } as unknown as Env; + const out = await computeGateEval(env, { days: 90, nowMs: NOW }); + const r = out.rows[0]; + expect(r).toBeDefined(); + if (!r) return; + expect(r.wouldMerge).toBe(10); // denominator unaffected by reversal + expect(r.mergeConfirmed).toBe(10); // raw bucket: both count as "predicted merge, human merged" + expect(r.mergePrecision).toBeCloseTo(1); // raw precision unaffected — byte-identical to pre-#2348 + expect(r.weightedMergeConfirmed).toBe(6 + 4 * REVERSAL_DISCOUNT_WEIGHT); + expect(r.weightedMergePrecision).toBeCloseTo((6 + 4 * REVERSAL_DISCOUNT_WEIGHT) / 10); + // REVERSAL_DISCOUNT_WEIGHT is documented as 0 (full discount) — assert the current formula's real effect, + // not just the generic shape, so a silent formula change is caught here. + expect(r.weightedMergePrecision).toBeCloseTo(0.6); + }); + + it("discounts a reversed (reopened) close's credit toward weightedCloseConfirmed the same way", async () => { + const cells = [ + { project: "p", pred: "close", truth: "closed", reversed: 0, n: 3 }, // stayed closed + { project: "p", pred: "close", truth: "closed", reversed: 1, n: 2 }, // reopened by a contributor + ]; + const env = { DB: { prepare: () => ({ bind: () => ({ all: async () => ({ results: cells }) }) }) } } as unknown as Env; + const out = await computeGateEval(env, { days: 90, nowMs: NOW }); + const r = out.rows[0]; + expect(r).toBeDefined(); + if (!r) return; + expect(r.wouldClose).toBe(5); + expect(r.closeConfirmed).toBe(5); + expect(r.closePrecision).toBeCloseTo(1); + expect(r.weightedCloseConfirmed).toBe(3 + 2 * REVERSAL_DISCOUNT_WEIGHT); + expect(r.weightedClosePrecision).toBeCloseTo(0.6); + }); + + it("a reversed mergeFalse/closeFalse cell (the dangerous-error buckets) never contributes to either weighted CONFIRMED bucket", async () => { + // Reversal only ever applies to the CONFIRMED (correct-and-later-undone) buckets; a cell that was already + // a mismatch (mergeFalse/closeFalse) has no "confirmed" credit to discount in the first place. + const cells = [ + { project: "p", pred: "merge", truth: "closed", reversed: 1, n: 3 }, // mergeFalse, marked reversed + { project: "p", pred: "close", truth: "merged", reversed: 1, n: 2 }, // closeFalse, marked reversed + ]; + const env = { DB: { prepare: () => ({ bind: () => ({ all: async () => ({ results: cells }) }) }) } } as unknown as Env; + const out = await computeGateEval(env, { days: 90, nowMs: NOW }); + const r = out.rows[0]; + expect(r).toBeDefined(); + if (!r) return; + expect(r.mergeFalse).toBe(3); + expect(r.closeFalse).toBe(2); + expect(r.weightedMergeConfirmed).toBe(0); + expect(r.weightedCloseConfirmed).toBe(0); + expect(r.weightedMergePrecision).toBe(0); + expect(r.weightedClosePrecision).toBe(0); + }); + + it("weighted precisions are null (not 0/0) when there is no would-merge/would-close prediction at all", async () => { + const cells = [{ project: "p", pred: "hold", truth: "merged", n: 4 }]; + const env = { DB: { prepare: () => ({ bind: () => ({ all: async () => ({ results: cells }) }) }) } } as unknown as Env; + const out = await computeGateEval(env, { days: 90, nowMs: NOW }); + const r = out.rows[0]; + expect(r?.weightedMergePrecision).toBeNull(); + expect(r?.weightedClosePrecision).toBeNull(); + }); + + it("REVERSAL_DISCOUNT_WEIGHT is a hardcoded module constant, not read from env/config (auditability requirement)", () => { + // #2348 explicitly requires this NOT be silently runtime-tunable. Pin its current documented value so a + // change to the objective function is a visible, reviewed diff here, not a silent behavior shift. + expect(REVERSAL_DISCOUNT_WEIGHT).toBe(0); + }); +});