diff --git a/src/review/auto-tune.ts b/src/review/auto-tune.ts index 4e83a288d..f183a80ab 100644 --- a/src/review/auto-tune.ts +++ b/src/review/auto-tune.ts @@ -34,6 +34,15 @@ export interface GateEvalRow { decided: number; // predictions that have a known outcome mergePrecision: number | null; closePrecision: number | null; + // #2348: reversal-discounted variants (src/review/parity.ts's own REVERSAL_DISCOUNT_WEIGHT — a merge/close + // later marked reversal_reverted/reversal_reopened has its confirmed-credit discounted). Denominators + // (wouldMerge/wouldClose) are unchanged, so these are always <= the raw precisions above. The circuit- + // breaker below gates on THESE, not the raw fields, so a high volume of later-reverted merges cannot keep + // the raw number artificially healthy while gaming the breaker into staying disengaged. + weightedMergeConfirmed: number; + weightedCloseConfirmed: number; + weightedMergePrecision: number | null; + weightedClosePrecision: number | null; } /** The gate eval report: per-project rows + a coarse "enough data to read" flag. */ @@ -78,28 +87,35 @@ export const AUTOCLEAR_AFTER_MS = 24 * 60 * 60 * 1000; export interface AutoTuneAction { project: string; + /** Raw (unweighted) merge precision, preserved for continuity with any existing log/dashboard consumer of + * this field's historical meaning. NOT what gated this action -- see weightedMergePrecision (#2348). */ mergePrecision: number; + /** #2348: the reversal-discounted precision that actually gated this action. Always <= mergePrecision. */ + weightedMergePrecision: number; decided: number; wouldMerge: number; message: string; } -/** PURE: which projects' merge precision has dropped enough to warrant engaging the circuit-breaker? */ +/** PURE: which projects' merge precision has dropped enough to warrant engaging the circuit-breaker? + * Gates on the reversal-WEIGHTED precision (#2348), not the raw one -- see GateEvalRow's own doc comment + * for why: a high volume of later-reverted merges must not keep the raw number artificially healthy. */ export function planAutoTune(report: GateEvalReport): AutoTuneAction[] { const actions: AutoTuneAction[] = []; for (const r of report.rows) { // Gate on wouldMerge, NOT decided: precision is measured over WOULD-MERGE predictions, so a project with many // holds/closes but few would-merges (e.g. 9 holds + 1 wrong would-merge) must not trip the breaker on a - // statistically meaningless sample. mergePrecision is non-null iff wouldMerge > 0, so check it FIRST to keep - // both arms of the guard reachable. - if (r.mergePrecision == null || r.wouldMerge < AUTOTUNE_MIN_DECIDED) continue; - if (r.mergePrecision < AUTOTUNE_MERGE_PRECISION_FLOOR) { + // statistically meaningless sample. weightedMergePrecision is non-null iff wouldMerge > 0 (same nullability + // as the raw field it discounts), so check it FIRST to keep both arms of the guard reachable. + if (r.weightedMergePrecision == null || r.wouldMerge < AUTOTUNE_MIN_DECIDED) continue; + if (r.weightedMergePrecision < AUTOTUNE_MERGE_PRECISION_FLOOR) { actions.push({ project: r.project, - mergePrecision: r.mergePrecision, + mergePrecision: r.mergePrecision ?? r.weightedMergePrecision, + weightedMergePrecision: r.weightedMergePrecision, decided: r.decided, wouldMerge: r.wouldMerge, - message: `Auto-merge DISABLED for ${r.project}: merge precision ${Math.round(r.mergePrecision * 100)}% over ${r.wouldMerge} would-merge PR(s) (< ${Math.round(AUTOTUNE_MERGE_PRECISION_FLOOR * 100)}%). Would-merges now HOLD for review. Investigate, then clear holdonly:${r.project}.`, + message: `Auto-merge DISABLED for ${r.project}: weighted merge precision ${Math.round(r.weightedMergePrecision * 100)}% (raw ${Math.round((r.mergePrecision ?? r.weightedMergePrecision) * 100)}%) over ${r.wouldMerge} would-merge PR(s) (< ${Math.round(AUTOTUNE_MERGE_PRECISION_FLOOR * 100)}%). Would-merges now HOLD for review. Investigate, then clear holdonly:${r.project}.`, }); } } @@ -124,7 +140,9 @@ export async function applyAutoTune(flags: FlagStore, report: GateEvalReport): P /** PURE: should an auto-engaged breaker for `project` be cleared now? True when the per-project holdonly flag * was set ≥ AUTOCLEAR_AFTER_MS ago AND merge precision is no longer failing (recovered, or no recent merge - * predictions to judge). Never considers a human-set global holdonly (no per-project row → setAt is null). */ + * predictions to judge). Never considers a human-set global holdonly (no per-project row → setAt is null). + * Reads the same weighted precision the breaker engaged on (#2348) -- a project can never clear on a raw + * number recovering while the reversal-discounted one it was actually held for is still failing. */ export function shouldAutoClear(report: GateEvalReport, project: string, setAtIso: string | null, nowMs: number): boolean { if (!setAtIso) return false; // not auto-engaged for THIS project (global breaker is human-only) // SQLite CURRENT_TIMESTAMP is "YYYY-MM-DD HH:MM:SS" in UTC (no zone). Normalize to ISO and FORCE a UTC zone @@ -135,7 +153,7 @@ export function shouldAutoClear(report: GateEvalReport, project: string, setAtIs const setMs = Date.parse(hasZone ? t : `${t}Z`); if (!Number.isFinite(setMs) || nowMs - setMs < AUTOCLEAR_AFTER_MS) return false; // still in cooldown const row = report.rows.find((r) => r.project === project); - const stillFailing = !!row && row.mergePrecision != null && row.wouldMerge >= AUTOTUNE_MIN_DECIDED && row.mergePrecision < AUTOTUNE_MERGE_PRECISION_FLOOR; + const stillFailing = !!row && row.weightedMergePrecision != null && row.wouldMerge >= AUTOTUNE_MIN_DECIDED && row.weightedMergePrecision < AUTOTUNE_MERGE_PRECISION_FLOOR; return !stillFailing; // cooldown elapsed + precision recovered (or no signal) → clear and let it retry } @@ -162,25 +180,31 @@ export async function maybeAutoClearHoldOnly(flags: FlagStore, report: GateEvalR export interface CloseAutoTuneAction { project: string; + /** Raw (unweighted) close precision, preserved for continuity with any existing log/dashboard consumer of + * this field's historical meaning. NOT what gated this action -- see weightedClosePrecision (#2348). */ closePrecision: number; + /** #2348: the reversal-discounted precision that actually gated this action. Always <= closePrecision. */ + weightedClosePrecision: number; decided: number; wouldClose: number; message: string; } /** PURE: which projects' CLOSE precision has dropped enough to warrant engaging the close-side breaker? - * Mirrors planAutoTune, testing closePrecision against AUTOTUNE_CLOSE_PRECISION_FLOOR. */ + * Mirrors planAutoTune, testing the reversal-WEIGHTED closePrecision (#2348) against + * AUTOTUNE_CLOSE_PRECISION_FLOOR -- not the raw one, for the same anti-gaming reason as the merge breaker. */ export function planCloseAutoTune(report: GateEvalReport): CloseAutoTuneAction[] { const actions: CloseAutoTuneAction[] = []; for (const r of report.rows) { - if (r.wouldClose < AUTOTUNE_MIN_DECIDED || r.closePrecision == null) continue; - if (r.closePrecision < AUTOTUNE_CLOSE_PRECISION_FLOOR) { + if (r.wouldClose < AUTOTUNE_MIN_DECIDED || r.weightedClosePrecision == null) continue; + if (r.weightedClosePrecision < AUTOTUNE_CLOSE_PRECISION_FLOOR) { actions.push({ project: r.project, - closePrecision: r.closePrecision, + closePrecision: r.closePrecision ?? r.weightedClosePrecision, + weightedClosePrecision: r.weightedClosePrecision, decided: r.decided, wouldClose: r.wouldClose, - message: `Auto-CLOSE DISABLED for ${r.project}: close precision ${Math.round(r.closePrecision * 100)}% over ${r.wouldClose} would-close PR(s) (< ${Math.round(AUTOTUNE_CLOSE_PRECISION_FLOOR * 100)}%). Would-closes now HOLD for review. Investigate, then clear closehold:${r.project}.`, + message: `Auto-CLOSE DISABLED for ${r.project}: weighted close precision ${Math.round(r.weightedClosePrecision * 100)}% (raw ${Math.round((r.closePrecision ?? r.weightedClosePrecision) * 100)}%) over ${r.wouldClose} would-close PR(s) (< ${Math.round(AUTOTUNE_CLOSE_PRECISION_FLOOR * 100)}%). Would-closes now HOLD for review. Investigate, then clear closehold:${r.project}.`, }); } } @@ -205,9 +229,9 @@ export async function applyCloseAutoTune(flags: FlagStore, report: GateEvalRepor } /** PURE: should an auto-engaged CLOSE breaker for `project` be cleared now? Mirrors shouldAutoClear but tests - * closePrecision. True when the per-project closehold flag was set ≥ AUTOCLEAR_AFTER_MS ago AND close precision - * is no longer failing (recovered, or no recent close predictions to judge). Never considers a human-set global - * closehold (no per-project row → setAt is null). */ + * the weighted closePrecision (#2348). True when the per-project closehold flag was set ≥ AUTOCLEAR_AFTER_MS + * ago AND close precision is no longer failing (recovered, or no recent close predictions to judge). Never + * considers a human-set global closehold (no per-project row → setAt is null). */ export function shouldAutoClearClose(report: GateEvalReport, project: string, setAtIso: string | null, nowMs: number): boolean { if (!setAtIso) return false; // not auto-engaged for THIS project (global breaker is human-only) const t = setAtIso.includes("T") ? setAtIso : setAtIso.replace(" ", "T"); @@ -215,7 +239,7 @@ export function shouldAutoClearClose(report: GateEvalReport, project: string, se const setMs = Date.parse(hasZone ? t : `${t}Z`); if (!Number.isFinite(setMs) || nowMs - setMs < AUTOCLEAR_AFTER_MS) return false; // still in cooldown const row = report.rows.find((r) => r.project === project); - const stillFailing = !!row && row.closePrecision != null && row.wouldClose >= AUTOTUNE_MIN_DECIDED && row.closePrecision < AUTOTUNE_CLOSE_PRECISION_FLOOR; + const stillFailing = !!row && row.weightedClosePrecision != null && row.wouldClose >= AUTOTUNE_MIN_DECIDED && row.weightedClosePrecision < AUTOTUNE_CLOSE_PRECISION_FLOOR; return !stillFailing; // cooldown elapsed + precision recovered (or no signal) → clear and let it retry } diff --git a/src/review/selftune-wire.ts b/src/review/selftune-wire.ts index c57bf6c7d..281cf9390 100644 --- a/src/review/selftune-wire.ts +++ b/src/review/selftune-wire.ts @@ -67,6 +67,7 @@ export const SELFTUNE_BASE_CONFIDENCE_FLOOR = 0; export function evalRowFromCalibration(project: string, positive: number, negative: number): GateEvalRow { const wouldMerge = positive + negative; // resolved recommendation outcomes graded against the human's call const decided = wouldMerge; + const mergePrecision = wouldMerge > 0 ? positive / wouldMerge : null; return { project, wouldMerge, @@ -77,8 +78,16 @@ export function evalRowFromCalibration(project: string, positive: number, negati closeFalse: 0, // held at 0 by construction: the only loosening branch of the advisor is unreachable hold: 0, decided, - mergePrecision: wouldMerge > 0 ? positive / wouldMerge : null, + mergePrecision, closePrecision: null, + // #2348: recommendation-outcome calibration (agent_recommendation_outcomes) carries no reversal signal at + // all — it is not derived from review_audit, so there is nothing for parity.ts's reversal-discount formula + // to discount BY. weighted === raw here by construction (no data to distinguish them), mirroring how a + // review_audit row with zero reversals also naturally produces weighted === raw. + weightedMergeConfirmed: positive, + weightedCloseConfirmed: 0, + weightedMergePrecision: mergePrecision, + weightedClosePrecision: null, }; } diff --git a/test/unit/auto-tune.test.ts b/test/unit/auto-tune.test.ts index 7b17d7f65..926d35a62 100644 --- a/test/unit/auto-tune.test.ts +++ b/test/unit/auto-tune.test.ts @@ -15,20 +15,33 @@ import { shouldAutoClearClose, } from "../../src/review/auto-tune"; -const row = (over: Partial): GateEvalRow => ({ - project: "p", - wouldMerge: 0, - mergeConfirmed: 0, - mergeFalse: 0, - wouldClose: 0, - closeConfirmed: 0, - closeFalse: 0, - hold: 0, - decided: 0, - mergePrecision: null, - closePrecision: null, - ...over, -}); +const row = (over: Partial): GateEvalRow => { + const mergeConfirmed = over.mergeConfirmed ?? 0; + const closeConfirmed = over.closeConfirmed ?? 0; + const mergePrecision = over.mergePrecision ?? null; + const closePrecision = over.closePrecision ?? null; + return { + project: "p", + wouldMerge: 0, + mergeConfirmed, + mergeFalse: 0, + wouldClose: 0, + closeConfirmed, + closeFalse: 0, + hold: 0, + decided: 0, + mergePrecision, + closePrecision, + // #2348: default weighted === raw (no reversal signal in a bare fixture), so every pre-#2348 test that + // only sets the raw fields keeps exercising the breaker meaningfully unchanged. A test proving the + // anti-gaming property overrides weightedMergePrecision/weightedClosePrecision explicitly to diverge. + weightedMergeConfirmed: mergeConfirmed, + weightedCloseConfirmed: closeConfirmed, + weightedMergePrecision: mergePrecision, + weightedClosePrecision: closePrecision, + ...over, + }; +}; const report = (rows: GateEvalRow[]): GateEvalReport => ({ rows, hasSignal: rows.some((r) => r.decided >= 10) }); /** A stub FlagStore (the injected seam — the live D1-backed store is deferred infra). */ @@ -69,6 +82,42 @@ describe("planAutoTune (#self-improve) — circuit-breaker is one-directional", }); }); +describe("planAutoTune — #2348 weighted anti-gaming cutover", () => { + it("engages on a WEIGHTED precision failure even though RAW precision is healthy (reversal-discounted merges cannot keep the raw number artificially healthy to dodge the breaker)", () => { + const a = planAutoTune( + report([ + row({ + project: "gamed", + decided: 20, + wouldMerge: 20, + mergeConfirmed: 19, + mergePrecision: 0.95, // raw looks healthy... + weightedMergePrecision: 0.5, // ...but most of those "confirmed" merges were later reverted + }), + ]), + ); + expect(a).toHaveLength(1); + expect(a[0]?.mergePrecision).toBe(0.95); // raw preserved for log continuity, NOT what gated this + expect(a[0]?.weightedMergePrecision).toBe(0.5); // weighted is what actually gated it + expect(a[0]?.message).toContain("weighted merge precision 50%"); + expect(a[0]?.message).toContain("raw 95%"); + }); + it("does NOT engage when weighted precision is healthy, proving the gate reads weightedMergePrecision (not mergePrecision) — these fields are structurally independent on GateEvalRow even though weighted <= raw always holds in real parity.ts data", () => { + expect( + planAutoTune( + report([row({ project: "p", decided: 20, wouldMerge: 20, mergeConfirmed: 10, mergePrecision: 0.5, weightedMergePrecision: 0.9 })]), + ), + ).toHaveLength(0); + }); + it("falls back to the weighted precision for the action's raw mergePrecision field when raw is null (defensive fallback; not expected from live parity.ts data, whose weighted/raw nullability always match)", () => { + const a = planAutoTune( + report([row({ project: "p", decided: 20, wouldMerge: 20, mergeConfirmed: 18, mergePrecision: null, weightedMergePrecision: 0.5 })]), + ); + expect(a).toHaveLength(1); + expect(a[0]?.mergePrecision).toBe(0.5); + }); +}); + describe("applyAutoTune", () => { it("sets the holdonly flag for a newly-flagged project", async () => { const { flags, setFlag } = stubFlags(); @@ -151,6 +200,23 @@ describe("shouldAutoClear (#272 recovery-gated breaker auto-clear)", () => { }); }); +describe("shouldAutoClear — #2348 weighted anti-gaming cutover", () => { + const now = Date.parse("2026-06-20T12:00:00Z"); + const past = new Date(now - AUTOCLEAR_AFTER_MS - 3_600_000).toISOString(); // >24h ago + it("stays engaged after cooldown when RAW precision recovered but WEIGHTED precision is still failing (reversals keep it held; raw recovery alone cannot clear it)", () => { + const stillGamed = report([ + row({ project: "g", decided: 20, wouldMerge: 20, mergeConfirmed: 20, mergePrecision: 1.0, weightedMergePrecision: 0.5 }), + ]); + expect(shouldAutoClear(stillGamed, "g", past, now)).toBe(false); + }); + it("clears after cooldown once WEIGHTED precision recovers, proving the recovery check reads weightedMergePrecision", () => { + const recoveredWeighted = report([ + row({ project: "g", decided: 20, wouldMerge: 20, mergeConfirmed: 20, mergePrecision: 1.0, weightedMergePrecision: 0.9 }), + ]); + expect(shouldAutoClear(recoveredWeighted, "g", past, now)).toBe(true); + }); +}); + // ── CLOSE-precision circuit-breaker (symmetric mirror of the merge breaker) ───────────────────────────────── describe("planCloseAutoTune (#close-precision-breaker) — tightening-only, close direction", () => { @@ -175,6 +241,42 @@ describe("planCloseAutoTune (#close-precision-breaker) — tightening-only, clos }); }); +describe("planCloseAutoTune — #2348 weighted anti-gaming cutover", () => { + it("engages on a WEIGHTED close-precision failure even though RAW close precision is healthy", () => { + const a = planCloseAutoTune( + report([ + row({ + project: "gamed", + decided: 20, + wouldClose: 20, + closeConfirmed: 19, + closePrecision: 0.95, + weightedClosePrecision: 0.5, + }), + ]), + ); + expect(a).toHaveLength(1); + expect(a[0]?.closePrecision).toBe(0.95); // raw preserved for log continuity, NOT what gated this + expect(a[0]?.weightedClosePrecision).toBe(0.5); // weighted is what actually gated it + expect(a[0]?.message).toContain("weighted close precision 50%"); + expect(a[0]?.message).toContain("raw 95%"); + }); + it("does NOT engage when weighted close precision is healthy, proving the gate reads weightedClosePrecision (not closePrecision)", () => { + expect( + planCloseAutoTune( + report([row({ project: "p", decided: 20, wouldClose: 20, closeConfirmed: 10, closePrecision: 0.5, weightedClosePrecision: 0.9 })]), + ), + ).toHaveLength(0); + }); + it("falls back to the weighted close precision for the action's raw closePrecision field when raw is null (defensive fallback)", () => { + const a = planCloseAutoTune( + report([row({ project: "p", decided: 20, wouldClose: 20, closeConfirmed: 18, closePrecision: null, weightedClosePrecision: 0.5 })]), + ); + expect(a).toHaveLength(1); + expect(a[0]?.closePrecision).toBe(0.5); + }); +}); + describe("applyCloseAutoTune", () => { it("sets the closehold flag for a newly-flagged project", async () => { const { flags, setFlag } = stubFlags(); @@ -258,6 +360,23 @@ describe("shouldAutoClearClose (#close-precision-breaker recovery-gated auto-cle }); }); +describe("shouldAutoClearClose — #2348 weighted anti-gaming cutover", () => { + const now = Date.parse("2026-06-20T12:00:00Z"); + const past = new Date(now - AUTOCLEAR_AFTER_MS - 3_600_000).toISOString(); // >24h ago + it("stays engaged after cooldown when RAW close precision recovered but WEIGHTED close precision is still failing", () => { + const stillGamed = report([ + row({ project: "g", decided: 20, wouldClose: 20, closeConfirmed: 20, closePrecision: 1.0, weightedClosePrecision: 0.5 }), + ]); + expect(shouldAutoClearClose(stillGamed, "g", past, now)).toBe(false); + }); + it("clears after cooldown once WEIGHTED close precision recovers, proving the recovery check reads weightedClosePrecision", () => { + const recoveredWeighted = report([ + row({ project: "g", decided: 20, wouldClose: 20, closeConfirmed: 20, closePrecision: 1.0, weightedClosePrecision: 0.9 }), + ]); + expect(shouldAutoClearClose(recoveredWeighted, "g", past, now)).toBe(true); + }); +}); + describe("computeTuningRecommendations (#self-improve)", () => { it("says READY when merge precision is high over a real sample with no false closes", () => { const recs = computeTuningRecommendations(report([row({ project: "gittensory", decided: 20, wouldMerge: 18, mergeConfirmed: 18, mergePrecision: 1.0 })])); diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 02bd03215..63153e0e5 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -635,6 +635,11 @@ describe("applyAutoTune over the live FlagStore — engages holdonly on low merg decided: 12, mergePrecision: 5 / 12, closePrecision: null, + // #2348: no reversal scenario in this fixture — weighted mirrors raw. + weightedMergeConfirmed: 5, + weightedCloseConfirmed: 0, + weightedMergePrecision: 5 / 12, + weightedClosePrecision: null, }, ], hasSignal: true, diff --git a/test/unit/selftune-wiring.test.ts b/test/unit/selftune-wiring.test.ts index 1b0c14882..97f507736 100644 --- a/test/unit/selftune-wiring.test.ts +++ b/test/unit/selftune-wiring.test.ts @@ -92,6 +92,14 @@ describe("evalRowFromCalibration — gittensory outcome data → ported GateEval expect(row.closePrecision).toBeNull(); }); + it("#2348: the weighted fields mirror the raw ones — agent_recommendation_outcomes carries no reversal signal, so there is nothing for the breaker's reversal-discount read to discount BY", () => { + const row = evalRowFromCalibration("owner/repo", 7, 5); + expect(row.weightedMergeConfirmed).toBe(row.mergeConfirmed); + expect(row.weightedCloseConfirmed).toBe(0); + expect(row.weightedMergePrecision).toBe(row.mergePrecision); + expect(row.weightedClosePrecision).toBeNull(); + }); + it("a low merge precision yields ONLY a TIGHTENING recommendation (raise the floor), never a loosening one", () => { // 5 confirmed / 15 would-merge = 33% precision over 15 decided → well under the risk floor. const row = evalRowFromCalibration("owner/repo", 5, 10);