diff --git a/src/review/risk-control-wire.ts b/src/review/risk-control-wire.ts index 89c581ea3..2af4a9f41 100644 --- a/src/review/risk-control-wire.ts +++ b/src/review/risk-control-wire.ts @@ -178,8 +178,8 @@ async function recalibrateArm(env: Env, arm: "close" | "merge", verdict: "close" * (#8835's "per-repo where sample size permits, global fallback otherwise"). A repo key certifies or is * retracted independently of the global one; the actuation read prefers the repo key. Returns per-arm * global statuses for the caller's log line. */ -export async function runRiskControlRecalibration(env: Env): Promise> { - const summary: Record = {}; +export async function runRiskControlRecalibration(env: Env): Promise> { + const summary: Record = {}; for (const { arm, verdict, alpha } of riskControlArms(env)) { try { summary[arm] = (await recalibrateArm(env, arm, verdict, alpha, null)).status; @@ -195,7 +195,10 @@ export async function runRiskControlRecalibration(env: Env): Promise= effectiveNeeded gate, falling through to + // `no_certifiable_threshold` (an error-rate shortfall) when the true shortfall is labels. candidates.length + // is always ≥ 1 here (the raw-δ floor check above already returned for an empty `pairs`), so this divides + // safely and, at K=1, effectiveNeeded === needed — identical to pre-#9637 behavior. + const effectiveNeeded = minimumCalibrationLabels(alpha, testDelta); + if (sorted.length < effectiveNeeded) { + return { status: "insufficient_labels", needed: effectiveNeeded, have: sorted.length, alpha, delta }; + } let dropped = 0; let droppedErrors = 0; let index = 0; // Tracks the closest-to-certifying candidate for `no_certifiable_threshold`'s message. The very first - // candidate always computes a bound (n = sorted.length ≥ needed, guaranteed by the floor check above), so - // these are always overwritten before any caller can observe the initial values. + // candidate always computes a bound (n = sorted.length ≥ effectiveNeeded, guaranteed by the floor check + // above), so these are always overwritten before any caller can observe the initial values. let bestLambda = candidates[0]!; let bestN = sorted.length; let bestUpperBound = Infinity; for (const lambda of candidates) { const n = sorted.length - dropped; const errors = totalErrors - droppedErrors; - if (n >= needed) { + if (n >= effectiveNeeded) { const bound = clopperPearsonUpperBound(errors, n, testDelta); if (bound < bestUpperBound) { bestUpperBound = bound; @@ -206,6 +220,11 @@ export function validateCalibrationPayload(value: unknown): { alpha: number; lam if (typeof v.coverageAtLambda !== "number" || !(v.coverageAtLambda >= 0 && v.coverageAtLambda <= 1)) return null; if (typeof v.delta !== "number" || !(v.delta > 0 && v.delta <= 1)) return null; if (typeof v.nAtLambda !== "number" || !Number.isFinite(v.nAtLambda)) return null; + // #9637: deliberately the UNSPLIT delta, not delta/K — the stored payload only ever records the originally- + // requested delta (candidates.length is not persisted), and nAtLambda is the coverage at the ALREADY- + // CERTIFIED lambda, not a candidate count mid-scan. A payload legitimately certified by the split-delta scan + // always clears this looser, unsplit-delta floor too (effectiveNeeded ≥ needed), so this stays the coarser, + // always-safe check calibrateActThreshold itself no longer uses for its own scan. if (v.nAtLambda < minimumCalibrationLabels(v.alpha, v.delta)) return null; return { alpha: v.alpha, lambda: v.lambda, coverageAtLambda: v.coverageAtLambda, nAtLambda: v.nAtLambda, delta: v.delta }; } diff --git a/test/unit/risk-control-wire.test.ts b/test/unit/risk-control-wire.test.ts index d732a4b06..3d43810cd 100644 --- a/test/unit/risk-control-wire.test.ts +++ b/test/unit/risk-control-wire.test.ts @@ -123,10 +123,11 @@ describe("runRiskControlRecalibration", () => { it("REGRESSION (#9048): NO CERTIFIABLE THRESHOLD — ample labels but no λ clears α reports the true total, not a residual stratum, under a distinct event_type", async () => { const env = createTestEnv({ LOOPOVER_RISK_CONTROL_CLOSE_ALPHA: "0.005" }); // fixed alpha; floor = 598 - // 600 dirty pairs (all wrong) at 0.5, then 50 clean pairs at 0.99 — 650 total, well over the 598 floor, - // but no candidate certifies: 0.5 has ruinous error, 0.99 falls below the floor once 0.5 is dropped. - for (let i = 1; i <= 600; i += 1) await seedLabeledDecision(env, i, "close", "incorrect", 0.5); - for (let i = 601; i <= 650; i += 1) await seedLabeledDecision(env, i, "close", "correct", 0.99); + // 700 dirty pairs (all wrong) at 0.5, then 100 clean pairs at 0.99 — 800 total, over the split-delta floor + // for these 2 candidates (#9637: 736, not the raw 598), but no candidate certifies: 0.5 has ruinous error, + // 0.99 falls below the split-delta floor once 0.5 is dropped. + for (let i = 1; i <= 700; i += 1) await seedLabeledDecision(env, i, "close", "incorrect", 0.5); + for (let i = 701; i <= 800; i += 1) await seedLabeledDecision(env, i, "close", "correct", 0.99); const summary = await runRiskControlRecalibration(env); expect(summary.close).toBe("no_certifiable_threshold"); const flag = await env.DB.prepare(`SELECT value FROM system_flags WHERE key = ?`).bind(riskControlFlagKey("close")).first(); @@ -134,7 +135,7 @@ describe("runRiskControlRecalibration", () => { const audit = await env.DB.prepare( `SELECT detail FROM audit_events WHERE event_type = 'risk_control_no_certifiable_threshold' AND target_key = 'riskcontrol:close'`, ).first<{ detail: string }>(); - expect(audit!.detail).toContain("650 labels available but no threshold achieves α=0.005"); + expect(audit!.detail).toContain("800 labels available but no threshold achieves α=0.005"); // The label-shortfall event must NOT also fire for this scope — it is a distinct outcome (#9048). const shortfall = await env.DB.prepare( `SELECT detail FROM audit_events WHERE event_type = 'risk_control_insufficient' AND target_key = 'riskcontrol:close'`, @@ -157,7 +158,7 @@ describe("fail-safe arms", () => { vi.restoreAllMocks(); }); - it("a throwing arm recalibration is contained: the OTHER arm still runs and the summary reports insufficient", async () => { + it("REGRESSION (#9637): a throwing arm recalibration reports 'error', never a statistical status — an infra failure is not a label shortfall", async () => { const env = createTestEnv(); const { vi } = await import("vitest"); const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); @@ -169,7 +170,7 @@ describe("fail-safe arms", () => { return realPrepare(sql); }); const summary = await runRiskControlRecalibration(env); - expect(summary).toEqual({ close: "insufficient_labels", merge: "insufficient_labels" }); + expect(summary).toEqual({ close: "error", merge: "error" }); expect(warn).toHaveBeenCalled(); vi.restoreAllMocks(); }); diff --git a/test/unit/risk-control.test.ts b/test/unit/risk-control.test.ts index ced85b892..dc32d5d62 100644 --- a/test/unit/risk-control.test.ts +++ b/test/unit/risk-control.test.ts @@ -71,17 +71,17 @@ describe("calibrateActThreshold", () => { }); it("REGRESSION (#9048): a repo with AMPLE labels that cannot certify reports its true total, not a residual stratum size", () => { - // 650 total labels (well over the 598-label floor for alpha=0.005) — but every candidate either has a - // ruinous error rate (the 600 pairs at 0.5, all wrong) or falls below the sample-size floor once that - // dirty stratum is dropped (only 50 remain at 0.99). Before #9048 this reported `have: 50` under - // `insufficient_labels` — exactly the "N usable labels of 59/598 needed" bug: a residual stratum size - // misreported as the repo's total label supply. - const pairs = [...Array.from({ length: 600 }, () => pair(0.5, false)), ...Array.from({ length: 50 }, () => pair(0.99, true))]; + // 800 total labels (well over the 736-label split-delta floor for these 2 candidates at alpha=0.005, + // #9637) — but every candidate either has a ruinous error rate (the 700 pairs at 0.5, all wrong) or falls + // below the split-delta sample-size floor once that dirty stratum is dropped (only 100 remain at 0.99). + // Before #9048 this reported `have: 100` under `insufficient_labels` — exactly the "N usable labels of + // 59/598 needed" bug: a residual stratum size misreported as the repo's total label supply. + const pairs = [...Array.from({ length: 700 }, () => pair(0.5, false)), ...Array.from({ length: 100 }, () => pair(0.99, true))]; const result = calibrateActThreshold(pairs, 0.005, 0.05); expect(result.status).toBe("no_certifiable_threshold"); if (result.status === "no_certifiable_threshold") { - expect(result.totalPairs).toBe(650); // the TRUE total, never a residual stratum size - expect(result.bestN).toBe(650); // the only candidate that cleared the sample-size floor + expect(result.totalPairs).toBe(800); // the TRUE total, never a residual stratum size + expect(result.bestN).toBe(800); // the only candidate that cleared the split-delta sample-size floor expect(result.bestLambda).toBe(0.5); expect(result.bestUpperBound).toBeGreaterThan(0.005); // could not certify alpha expect(result.bestUpperBound).toBeLessThanOrEqual(1); @@ -89,13 +89,49 @@ describe("calibrateActThreshold", () => { }); it("REGRESSION (#9066): does not over-certify across many observed confidence candidates — Bonferroni-splits delta across the K distinct thresholds actually tested", () => { - // Same shape the pre-fix code certified at lambda=0.9 (700 clean pairs, 10 distinct confidences, zero + // Same shape the pre-#9066 code certified at lambda=0.9 (700 clean pairs, 10 distinct confidences, zero // errors, alpha=0.005): the RAW zero-error bound at n=700, delta=0.05 is ≈0.0043 (<=0.005, would certify), - // but split across K=10 candidates (delta/10=0.005) the bound is ≈0.0075 (>0.005) — correctly refuses, - // because reporting whichever of 10 tested candidates passes first is a selection, not a single test. + // but split across K=10 candidates (delta/10=0.005) the bound is ≈0.0075 (>0.005) — over-certification is + // refused. #9637: the label floor at that split delta (1058) is now also enforced up front, so 700 pairs — + // genuinely short of what 10 candidates need — refuses as `insufficient_labels`, not the pre-#9637 + // `no_certifiable_threshold` (which would have wrongly implied the error rate, not the label count, was + // the shortfall). Either way `calibrated` must never be reached. const pairs = Array.from({ length: 700 }, (_, i) => pair(0.9 + (i % 10) / 100, true)); const result = calibrateActThreshold(pairs, 0.005, 0.05); + expect(result.status).toBe("insufficient_labels"); + expect(result.status).not.toBe("calibrated"); + if (result.status === "insufficient_labels") { + expect(result.needed).toBe(1058); + expect(result.have).toBe(700); + } + }); + + it("REGRESSION (#9637): the split-delta floor, not the raw floor, gates a zero-error set — 59 clean labels across 59 distinct confidences is insufficient, not uncertifiable", () => { + // Exactly the raw floor (minimumCalibrationLabels(0.05, 0.05) = 59), so the pre-scan check alone would + // let this through — but every pair is a distinct confidence (K=59 candidates), so the scan actually + // tests each candidate at delta/59. The effective floor at that split delta is 138: below it, a + // ZERO-ERROR set must still refuse as insufficient_labels, not fall through the whole scan into a + // misleading no_certifiable_threshold (#9048's status split, reintroduced by #9066's Bonferroni + // correction and the reason this issue exists). + const pairs = Array.from({ length: 59 }, (_, i) => pair(0.4 + i / 1000, true)); + const result = calibrateActThreshold(pairs, 0.05, 0.05); + expect(result).toMatchObject({ status: "insufficient_labels", needed: 138, have: 59 }); + }); + + it("REGRESSION (#9637): no_certifiable_threshold remains reachable once labels clear the split-delta floor", () => { + // A single confidence bucket (K=1 candidate, so the split delta equals the raw delta — this is also the + // `candidates.length === 1` edge the fix must leave behaviorally identical to before #9637) isolates a + // genuine error-rate shortfall from the label-count shortfall the tests above exercise: 1000 pairs, 12% + // wrong — comfortably over alpha=0.05's own floor (59) but nowhere near a certifiable error rate. + const pairs = [...Array.from({ length: 880 }, () => pair(0.9, true)), ...Array.from({ length: 120 }, () => pair(0.9, false))]; + const result = calibrateActThreshold(pairs, 0.05, 0.05); expect(result.status).toBe("no_certifiable_threshold"); + if (result.status === "no_certifiable_threshold") { + expect(result.totalPairs).toBe(1000); + expect(result.bestN).toBe(1000); + expect(result.bestLambda).toBe(0.9); + expect(result.bestUpperBound).toBeGreaterThan(0.05); + } }); it("is deterministic and input-order independent", () => {