From 27ec3dc6e79065e9ff9c52c99d1189a2be36d749 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 18:02:09 +0900 Subject: [PATCH] fix(review): gate the tuning advisor on wouldMerge and weighted precision, matching the breaker computeTuningRecommendations disagreed with its sibling circuit breaker on both rules the breaker documents. It gated the sample on decided (holds included) instead of wouldMerge, so 9 holds + 1 wrong would-merge cleared decided>=10, produced mergePrecision 0, and emitted a warn carrying an auto-applicable overridePayload -- queuing a live confidence-floor raise off a single prediction the breaker already refuses. And it read the RAW mergePrecision/closePrecision while the breaker gates on the reversal-WEIGHTED fields, so a project whose merges are systematically reverted (weighted ~0, raw healthy) got no tightening recommended -- exactly the project that needs one -- and evaluateShadowPromotion dropped its queued tightening on the raw number the breaker itself distrusts. Gate the advisor's sample on wouldMerge < MIN_DECIDED, its risk/ready tests on weightedMergePrecision/weightedClosePrecision, and its close-side warn on the weighted close precision against AUTOTUNE_CLOSE_PRECISION_FLOOR (closeFalse stays in the message, no longer the condition). Thread the weighted merge precision into evaluateShadowPromotion by renaming AutoApplyContext.mergePrecision to weightedMergePrecision. Reorder planCloseAutoTune's guard to null-check first, matching planAutoTune, so its formerly-dead null arm is reachable. Every threshold constant, severity ordering, and the tightening-only direction are unchanged. Closes #10014 --- src/review/auto-apply.ts | 13 ++++---- src/review/auto-tune.ts | 31 ++++++++++++------- test/unit/auto-apply.test.ts | 4 +-- test/unit/auto-tune.test.ts | 60 ++++++++++++++++++++++++++++++------ 4 files changed, 80 insertions(+), 28 deletions(-) diff --git a/src/review/auto-apply.ts b/src/review/auto-apply.ts index d1776aea67..33e196659b 100644 --- a/src/review/auto-apply.ts +++ b/src/review/auto-apply.ts @@ -382,11 +382,12 @@ export interface AutoApplyContext { baseScopeCap?: { files: number; lines: number }; /** This project's decided-sample count from the gate eval (drives the promotion evidence gate). */ decided: number; - /** This project's freshly-computed merge precision from THIS tick's gate eval (the same field - * computeTuningRecommendations reads). Threaded into evaluateShadowPromotion so a shadow-queued tightening - * cannot be promoted once the precision that originally warranted it has since recovered. Optional/nullable - * because a project can have no would-merge samples yet (GateEvalRow.mergePrecision is null in that case). */ - mergePrecision?: number | null; + /** This project's freshly-computed reversal-WEIGHTED merge precision from THIS tick's gate eval (the same + * field computeTuningRecommendations gates on, #10014). Threaded into evaluateShadowPromotion so a + * shadow-queued tightening cannot be promoted once the weighted precision that originally warranted it has + * since recovered -- feeding the RAW number here would clear a hold the reversal-weighted breaker still + * holds. Optional/nullable because a project can have no would-merge samples yet (it is null in that case). */ + weightedMergePrecision?: number | null; /** The tuning advisor's recommendations for this project (only ones with an overridePayload are applied). */ recs: TuningRec[]; /** Current wall-clock (ms) — injected for determinism in tests. */ @@ -429,7 +430,7 @@ export async function runAutoApplyRecommendations(env: StorageEnv, ctx: AutoAppl decided: ctx.decided, validatedUntilIso: shadow.validatedUntil, nowIso, - ...(ctx.mergePrecision !== undefined ? { currentMergePrecision: ctx.mergePrecision } : {}), + ...(ctx.weightedMergePrecision !== undefined ? { currentMergePrecision: ctx.weightedMergePrecision } : {}), }); if (gate.promote) { // Audit BEFORE the mutation — see applyOverrideRecommendation's force branch for why this ordering diff --git a/src/review/auto-tune.ts b/src/review/auto-tune.ts index d6cf730dd4..8e27658b83 100644 --- a/src/review/auto-tune.ts +++ b/src/review/auto-tune.ts @@ -196,7 +196,9 @@ export interface CloseAutoTuneAction { export function planCloseAutoTune(report: GateEvalReport): CloseAutoTuneAction[] { const actions: CloseAutoTuneAction[] = []; for (const r of report.rows) { - if (r.wouldClose < AUTOTUNE_MIN_DECIDED || r.weightedClosePrecision == null) continue; + // #10014: null-check FIRST, mirroring planAutoTune -- weightedClosePrecision is non-null iff wouldClose > 0, + // so once wouldClose >= 10 the old trailing `== null` disjunct was unreachable (a dead branch arm). + if (r.weightedClosePrecision == null || r.wouldClose < AUTOTUNE_MIN_DECIDED) continue; if (r.weightedClosePrecision < AUTOTUNE_CLOSE_PRECISION_FLOOR) { actions.push({ project: r.project, @@ -298,37 +300,44 @@ const pct = (x: number | null): string => (x == null ? "—" : `${Math.round(x * export function computeTuningRecommendations(report: GateEvalReport): TuningRec[] { const recs: TuningRec[] = []; for (const r of report.rows) { - if (r.decided < MIN_DECIDED) { - recs.push({ project: r.project, severity: "info", message: `Only ${r.decided} decided PR(s) — collect more shadow data before judging accuracy or flipping live.` }); + // #10014: gate on wouldMerge, not decided, matching planAutoTune's documented rule -- precision is measured + // over WOULD-MERGE predictions, so a project of many holds + one wrong would-merge (9 holds + 1) is a + // statistically meaningless sample the breaker already refuses; gating on `decided` let it clear the floor. + if (r.wouldMerge < MIN_DECIDED) { + recs.push({ project: r.project, severity: "info", message: `Only ${r.wouldMerge} would-merge PR(s) — collect more shadow data before judging accuracy or flipping live.` }); continue; } let flagged = false; - // The dangerous error: would auto-merge something the human closed. - if (r.mergePrecision != null && r.mergePrecision < RISK_MERGE_PRECISION) { + // The dangerous error: would auto-merge something the human closed. #10014: read the reversal-WEIGHTED + // precision the breaker gates on (weightedMergePrecision is non-null iff wouldMerge > 0), so a project whose + // merges are systematically reverted -- weighted at ~0 while raw stays healthy -- is flagged, not silent. + if (r.weightedMergePrecision != null && r.weightedMergePrecision < RISK_MERGE_PRECISION) { recs.push({ project: r.project, severity: "warn", - message: `Would have auto-merged ${r.mergeFalse} PR(s) the human CLOSED (merge precision ${pct(r.mergePrecision)} over ${r.wouldMerge}). Tighten guardrails / raise the confidence floor — do NOT flip live yet.`, + message: `Would have auto-merged ${r.mergeFalse} PR(s) the human CLOSED (weighted merge precision ${pct(r.weightedMergePrecision)} over ${r.wouldMerge}). Tighten guardrails / raise the confidence floor — do NOT flip live yet.`, // Auto-applicable TIGHTENING: raise the floor to the ready bar. Strictly safe-ward (a higher floor can // only HOLD more would-merges, never add a bad one), so the apply path can promote it. (#275) overridePayload: { confidenceFloor: TIGHTEN_FLOOR_TARGET }, }); flagged = true; } - // The other error: would auto-close something the human merged. - if (r.closeFalse > 0) { + // The other error: would auto-close something the human merged. #10014: gate on the weighted close precision + // against the same floor the close breaker uses, not the raw closeFalse count (which the weighted number + // discounts). closeFalse is still named in the message, but no longer the condition. + if (r.weightedClosePrecision != null && r.weightedClosePrecision < AUTOTUNE_CLOSE_PRECISION_FLOOR) { recs.push({ project: r.project, severity: "warn", - message: `Would have auto-closed ${r.closeFalse} PR(s) the human MERGED (close precision ${pct(r.closePrecision)}). Loosen the area/scope rules before going live.`, + message: `Would have auto-closed ${r.closeFalse} PR(s) the human MERGED (weighted close precision ${pct(r.weightedClosePrecision)}). Loosen the area/scope rules before going live.`, }); flagged = true; } - if (!flagged && r.mergePrecision != null && r.mergePrecision >= READY_MERGE_PRECISION && (r.closePrecision == null || r.closePrecision >= READY_CLOSE_PRECISION)) { + if (!flagged && r.weightedMergePrecision != null && r.weightedMergePrecision >= READY_MERGE_PRECISION && (r.weightedClosePrecision == null || r.weightedClosePrecision >= READY_CLOSE_PRECISION)) { recs.push({ project: r.project, severity: "good", - message: `Merge precision ${pct(r.mergePrecision)} over ${r.decided} decided PR(s) with no false closes — looks ready to flip live (shadow:false).`, + message: `Merge precision ${pct(r.weightedMergePrecision)} over ${r.decided} decided PR(s) with no false closes — looks ready to flip live (shadow:false).`, }); } } diff --git a/test/unit/auto-apply.test.ts b/test/unit/auto-apply.test.ts index 131995c5eb..1328582fbc 100644 --- a/test/unit/auto-apply.test.ts +++ b/test/unit/auto-apply.test.ts @@ -381,7 +381,7 @@ describe("runAutoApplyRecommendations (#278 — closes the loop: queue tightenin it("refuses to promote a stale shadow tightening once the project's precision has since recovered", async () => { const { env, tables } = fakeEnv(); tables.shadow.set("g", { confidence_floor: 0.95, scope_cap_files: null, scope_cap_lines: null, validated_until: "2026-06-19T00:00:00Z" }); - await runAutoApplyRecommendations(env, ctx({ recs: [], mergePrecision: 0.92 })); + await runAutoApplyRecommendations(env, ctx({ recs: [], weightedMergePrecision: 0.92 })); expect(tables.live.has("g")).toBe(false); // NOT promoted expect(tables.shadow.has("g")).toBe(true); // stays queued rather than being silently dropped }); @@ -389,7 +389,7 @@ describe("runAutoApplyRecommendations (#278 — closes the loop: queue tightenin it("still promotes a soaked shadow override when the fresh precision has NOT recovered", async () => { const { env, tables } = fakeEnv(); tables.shadow.set("g", { confidence_floor: 0.95, scope_cap_files: null, scope_cap_lines: null, validated_until: "2026-06-19T00:00:00Z" }); - await runAutoApplyRecommendations(env, ctx({ recs: [], mergePrecision: 0.5 })); + await runAutoApplyRecommendations(env, ctx({ recs: [], weightedMergePrecision: 0.5 })); expect(tables.live.get("g")?.confidence_floor).toBe(0.95); expect(tables.shadow.has("g")).toBe(false); }); diff --git a/test/unit/auto-tune.test.ts b/test/unit/auto-tune.test.ts index 8df920124b..df43b6c10d 100644 --- a/test/unit/auto-tune.test.ts +++ b/test/unit/auto-tune.test.ts @@ -237,6 +237,12 @@ describe("planCloseAutoTune (#close-precision-breaker) — tightening-only, clos it("does NOT engage when close precision is null (no would-close predictions with a known outcome)", () => { expect(planCloseAutoTune(report([row({ project: "p", decided: 30, wouldClose: 0, closeConfirmed: 0, closePrecision: null })]))).toHaveLength(0); }); + it("#10014: the reordered guard's null arm is reachable — null weighted precision with wouldClose >= 10 skips (not a crash on the <-comparison)", () => { + // Before the reorder the `wouldClose < 10` arm short-circuited first, so once wouldClose >= 10 the trailing + // `== null` disjunct was dead. Null-checking FIRST (mirroring planAutoTune) makes this arm reachable: a + // null weighted precision at wouldClose=20 continues (no action), never reaching the `< FLOOR` comparison. + expect(planCloseAutoTune(report([row({ project: "p", decided: 30, wouldClose: 20, closeConfirmed: 0, closePrecision: 0.5, weightedClosePrecision: null })]))).toHaveLength(0); + }); it("does NOT engage when close precision is healthy (it only ever tightens, never loosens)", () => { expect(planCloseAutoTune(report([row({ project: "p", decided: 30, wouldClose: 30, closeConfirmed: 29, closePrecision: 0.97 })]))).toHaveLength(0); }); @@ -421,15 +427,25 @@ describe("computeTuningRecommendations (#self-improve)", () => { expect(recs[0]?.severity).toBe("warn"); }); - it("renders an em-dash for a NULL close precision in the loosen-warn message (pct null branch)", () => { - // closeFalse > 0 but closePrecision is null (e.g. no would-close predictions had a known outcome): the - // loosen-warn message interpolates pct(null) → "—" rather than a percentage. Exercises pct's null side. + it("#10014: the loosen-warn fires on a weighted close precision below the floor, not on a raw closeFalse count", () => { + // The close-side gate now reads weightedClosePrecision against AUTOTUNE_CLOSE_PRECISION_FLOOR (0.85), the + // same reversal-discounted evidence the close breaker uses -- not the raw closeFalse count the weighted + // number discounts. A below-floor weighted precision emits a loosen-warn naming that weighted percentage. const recs = computeTuningRecommendations( - report([row({ project: "p", decided: 15, wouldMerge: 10, mergeConfirmed: 10, mergePrecision: 1.0, closeFalse: 2, closePrecision: null })]), + report([row({ project: "p", decided: 15, wouldMerge: 10, mergeConfirmed: 10, mergePrecision: 1.0, weightedMergePrecision: 1.0, closeFalse: 2, wouldClose: 6, closePrecision: 0.9, weightedClosePrecision: 0.2 })]), ); const loosen = recs.find((r) => r.severity === "warn" && /loosen/i.test(r.message)); expect(loosen).toBeDefined(); - expect(loosen?.message).toContain("close precision —"); + expect(loosen?.message).toContain("weighted close precision 20%"); + }); + + it("#10014: a NULL weighted close precision no longer triggers a loosen-warn (the raw closeFalse path is gone)", () => { + // Previously closeFalse > 0 with a null closePrecision emitted a loosen-warn; now a null weighted close + // precision (no would-close outcomes) fires nothing, so a merge-clean project reads as ready, not held. + const recs = computeTuningRecommendations( + report([row({ project: "p", decided: 15, wouldMerge: 12, mergeConfirmed: 12, mergePrecision: 1.0, weightedMergePrecision: 1.0, closeFalse: 2, closePrecision: null, weightedClosePrecision: null })]), + ); + expect(recs.find((r) => r.severity === "warn" && /loosen/i.test(r.message))).toBeUndefined(); }); it("says READY when close precision is also high and non-null (ready close-precision threshold branch)", () => { @@ -444,11 +460,12 @@ describe("computeTuningRecommendations (#self-improve)", () => { expect(recs[0]?.message).toMatch(/ready to flip live/i); }); - it("does NOT say ready when a non-null close precision is below the ready bar (close arm fails)", () => { - // Same ready conditions but closePrecision below READY_CLOSE_PRECISION (0.9) and no false closes → neither - // warn nor good fires, so the project yields no recommendation. Asserts the close arm gates 'good'. + it("#10014: does NOT say ready when the weighted close precision is below the ready bar (close arm gates 'good' on the weighted field)", () => { + // High merge precision but weightedClosePrecision at 0.88 — below the 0.9 ready bar yet at/above the 0.85 + // loosen floor, so NEITHER the good arm nor the loosen-warn fires: the project yields no recommendation. + // Proves the ready guard's close arm now reads weightedClosePrecision, not the raw closePrecision. const recs = computeTuningRecommendations( - report([row({ project: "borderline", decided: 20, wouldMerge: 18, mergeConfirmed: 18, mergePrecision: 1.0, wouldClose: 4, closeConfirmed: 3, closeFalse: 0, closePrecision: 0.8 })]), + report([row({ project: "borderline", decided: 20, wouldMerge: 18, mergeConfirmed: 18, mergePrecision: 1.0, weightedMergePrecision: 1.0, wouldClose: 4, closeConfirmed: 3, closeFalse: 0, closePrecision: 0.95, weightedClosePrecision: 0.88 })]), ); expect(recs).toHaveLength(0); }); @@ -464,6 +481,31 @@ describe("computeTuningRecommendations (#self-improve)", () => { ); expect(recs.map((r) => r.project)).toEqual(["alpha", "zeta"]); }); + + it("REGRESSION #10014: the sample gate reads wouldMerge, not decided — 9 holds + 1 wrong would-merge is info-only, no overridePayload", () => { + // The exact shape planAutoTune's comment names: decided=10 clears the old `decided < MIN_DECIDED` gate, but + // it is 9 holds + 1 would-merge — a statistically meaningless sample the breaker already refuses. Under the + // fix it gates on wouldMerge (1 < 10) and emits an INFO rec with NO auto-applicable overridePayload, rather + // than a warn that queued a live confidence-floor raise off a single prediction. + const recs = computeTuningRecommendations( + report([row({ project: "o/r", decided: 10, wouldMerge: 1, mergeFalse: 1, mergePrecision: 0, weightedMergePrecision: 0, hold: 9 })]), + ); + expect(recs).toHaveLength(1); + expect(recs[0]?.severity).toBe("info"); + expect(recs[0]?.overridePayload).toBeUndefined(); + expect(recs[0]?.message).toContain("would-merge"); + }); + + it("REGRESSION #10014: a healthy RAW merge precision with a failing WEIGHTED one still warns with a tightening overridePayload", () => { + // A project whose merges are systematically reverted: mergePrecision 0.98 looks healthy, but the + // reversal-weighted precision the breaker gates on is 0.2. The advisor must read the weighted field and flag + // it (with the auto-applicable tightening payload), not stay silent on the raw number the breaker distrusts. + const recs = computeTuningRecommendations( + report([row({ project: "o/r", decided: 20, wouldMerge: 20, mergeConfirmed: 20, mergeFalse: 0, mergePrecision: 0.98, weightedMergePrecision: 0.2 })]), + ); + const warn = recs.find((r) => r.severity === "warn"); + expect(warn?.overridePayload).toEqual({ confidenceFloor: 0.95 }); + }); }); describe("T3 byte-stability pins (#8225 migration map)", () => {