From d04a6136419a59771517cc67b6cbaa9a9ecb84a9 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:05:01 +0900 Subject: [PATCH] fix(recap): apply the gate-precision noise floor to the recap's aggregate false-positive rates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildMaintainerRecap` re-derived `totals.gateFalsePositiveRate` (and the miner/ human cohort aggregates) with a bare `blocked > 0` guard, while the per-repo path and the Gate-outcomes section both null the rate below MIN_SAMPLE (5) as gate-precision does. So one digest could read "Gate false-positive rate: 100% (2/2)" in its summary and Totals while its own Gate-outcomes section said "n/a (fewer than 5 blocks)" — the maintainer sees "the gate is 100% wrong" and "not enough data" about the same window, from the same totals struct. Export the floor once as `MIN_GATE_PRECISION_SAMPLE` from gate-precision.ts (replacing its private `MIN_SAMPLE`), import it in maintainer-recap-gate-outcomes.ts (deleting the duplicate) and maintainer-recap.ts, and gate the blended rate and each cohort aggregate on `blocked >= MIN_GATE_PRECISION_SAMPLE` against their own denominator. The below-floor case subsumes the old divide-by-zero guard; the rate-line arms and the `number | null` type are unchanged. The 5 now lives in one place, so the next consumer of RecapReport.totals inherits it. Closes #9691 --- src/services/gate-precision.ts | 15 ++-- .../maintainer-recap-gate-outcomes.ts | 18 ++--- src/services/maintainer-recap.ts | 10 ++- test/unit/maintainer-recap-format.test.ts | 22 +++++ test/unit/maintainer-recap.test.ts | 80 +++++++++++++++++-- 5 files changed, 117 insertions(+), 28 deletions(-) diff --git a/src/services/gate-precision.ts b/src/services/gate-precision.ts index 8492f81511..a44e4730fb 100644 --- a/src/services/gate-precision.ts +++ b/src/services/gate-precision.ts @@ -19,8 +19,9 @@ import { fetchOfficialGittensorMinerLogins } from "../gittensor/api"; import type { GateOutcomeRecord, PullRequestRecord } from "../types"; import { nowIso } from "../utils/json"; -// Below this per-gate-type blocked sample the false-positive rate is too noisy to judge. -const MIN_SAMPLE = 5; +// Below this per-gate-type blocked sample the false-positive rate is too noisy to judge. Exported (#9691) so the +// recap's aggregate rates share the one floor instead of each consumer re-deriving it and drifting. +export const MIN_GATE_PRECISION_SAMPLE = 5; export type GatePrecisionPerType = { gateType: string; @@ -69,7 +70,7 @@ function sameRepo(a: string | null | undefined, b: string): boolean { /** #4520: the fold core, extracted so buildGatePrecisionReport can run it up to three times (blended, miner, * human) over disjoint outcome subsets without duplicating the accumulation logic. Pure -- the same - * MIN_SAMPLE floor is applied independently per call, so a small cohort correctly reads null rather than a + * MIN_GATE_PRECISION_SAMPLE floor is applied independently per call, so a small cohort correctly reads null rather than a * noisy rate. */ function foldGateOutcomes(outcomes: GateOutcomeRecord[], prByNumber: Map): GatePrecisionCohortReport { const perType = new Map(); @@ -97,7 +98,7 @@ function foldGateOutcomes(outcomes: GateOutcomeRecord[], prByNumber: Map= MIN_SAMPLE ? round(entry.blockedThenMerged / entry.blocked) : null, + falsePositiveRate: entry.blocked >= MIN_GATE_PRECISION_SAMPLE ? round(entry.blockedThenMerged / entry.blocked) : null, })) .sort((a, b) => b.blocked - a.blocked || a.gateType.localeCompare(b.gateType)); @@ -106,7 +107,7 @@ function foldGateOutcomes(outcomes: GateOutcomeRecord[], prByNumber: Map= MIN_SAMPLE ? round(overallMerged / overallBlocked) : null, + falsePositiveRate: overallBlocked >= MIN_GATE_PRECISION_SAMPLE ? round(overallMerged / overallBlocked) : null, }, }; } @@ -124,7 +125,7 @@ function isMinerAuthoredOutcome(outcome: GateOutcomeRecord, prByNumber: Map= MIN_SAMPLE ? round(gateFalsePositives / blocked) : null; + // Null the rate below MIN_GATE_PRECISION_SAMPLE (gate-precision.ts:103) — a 1-of-1 "false positive" is noise. This also + // covers the divide-by-zero arm (blocked === 0 < MIN_GATE_PRECISION_SAMPLE), so the ratio is never evaluated at 0. + const falsePositiveRate = blocked >= MIN_GATE_PRECISION_SAMPLE ? round(gateFalsePositives / blocked) : null; const rateLine = falsePositiveRate === null - ? `False-positive rate: n/a (fewer than ${MIN_SAMPLE} blocks in the last ${report.windowDays} day(s))` + ? `False-positive rate: n/a (fewer than ${MIN_GATE_PRECISION_SAMPLE} blocks in the last ${report.windowDays} day(s))` : `False-positive rate: ${Math.round(falsePositiveRate * 100)}% (${gateFalsePositives} of ${blocked} blocks merged anyway)`; const title = "Gate outcomes"; diff --git a/src/services/maintainer-recap.ts b/src/services/maintainer-recap.ts index 1ab681f87e..af7a3e5e4e 100644 --- a/src/services/maintainer-recap.ts +++ b/src/services/maintainer-recap.ts @@ -13,6 +13,7 @@ import { PUBLIC_LOCAL_PATH_SCRUB_PATTERN, PUBLIC_UNSAFE_PATTERN } from "../signals/redaction"; import { deliverRecapToDiscord, deliverRecapToSlack } from "./notify-discord"; import type { GatePrecisionReport } from "./gate-precision"; +import { MIN_GATE_PRECISION_SAMPLE } from "./gate-precision"; import type { DriftRecapSection } from "./maintainer-recap-drift"; // #8372: these three section builders shipped fully implemented + unit-tested but were never composed into // the delivered digest -- the same "built, tested, never called from production" shape as #6636. @@ -142,20 +143,23 @@ export function buildMaintainerRecap(args: MaintainerRecapInputs): RecapReport { cohortTotals.human.gateFalsePositives += gatePrecision.cohorts.human.overall.blockedThenMerged; } } + // Null below the shared noise floor (#9691), matching gate-precision.ts and the gate-outcomes section — a + // digest must not print a false-positive percentage in its summary/Totals while its own Gate-outcomes section + // says the sample is too small to judge. Subsumes the old `blocked === 0` divide-by-zero guard. totals.gateFalsePositiveRate = - totals.blocked > 0 ? Math.round((totals.gateFalsePositives / totals.blocked) * 100) / 100 : null; + totals.blocked >= MIN_GATE_PRECISION_SAMPLE ? Math.round((totals.gateFalsePositives / totals.blocked) * 100) / 100 : null; const cohorts = cohortBlockedRepos > 0 ? { miner: { blocked: cohortTotals.miner.blocked, gateFalsePositives: cohortTotals.miner.gateFalsePositives, - gateFalsePositiveRate: cohortTotals.miner.blocked > 0 ? Math.round((cohortTotals.miner.gateFalsePositives / cohortTotals.miner.blocked) * 100) / 100 : null, + gateFalsePositiveRate: cohortTotals.miner.blocked >= MIN_GATE_PRECISION_SAMPLE ? Math.round((cohortTotals.miner.gateFalsePositives / cohortTotals.miner.blocked) * 100) / 100 : null, }, human: { blocked: cohortTotals.human.blocked, gateFalsePositives: cohortTotals.human.gateFalsePositives, - gateFalsePositiveRate: cohortTotals.human.blocked > 0 ? Math.round((cohortTotals.human.gateFalsePositives / cohortTotals.human.blocked) * 100) / 100 : null, + gateFalsePositiveRate: cohortTotals.human.blocked >= MIN_GATE_PRECISION_SAMPLE ? Math.round((cohortTotals.human.gateFalsePositives / cohortTotals.human.blocked) * 100) / 100 : null, }, } : undefined; diff --git a/test/unit/maintainer-recap-format.test.ts b/test/unit/maintainer-recap-format.test.ts index 767056b90d..820106c150 100644 --- a/test/unit/maintainer-recap-format.test.ts +++ b/test/unit/maintainer-recap-format.test.ts @@ -130,6 +130,28 @@ describe("formatMaintainerRecap (#2240)", () => { expect(body).not.toContain("payout"); }); + it("renders n/a (not a percentage) for a below-floor report — Totals and Gate-outcomes agree (#9691)", () => { + const report: RecapReport = { + ...emptyReport(), + totals: { + ...emptyReport().totals, + reviewed: 2, + blocked: 2, + gateFalsePositives: 2, + gateFalsePositiveRate: null, + }, + summary: ["Gate false-positive rate: not enough blocked PRs in the window to report."], + }; + const body = formatMaintainerRecap(report); + // Totals still reports the raw counts, but the rate reads n/a — no contradictory percentage anywhere. + expect(body).toContain("- Gate false positives: 2/2 (n/a)"); + expect(body).not.toMatch(/Gate false positives: 2\/2 \(\d+%\)/); + // The Gate-outcomes section's own below-floor copy is present and consistent. + expect(body).toContain("False-positive rate: n/a (fewer than 5 blocks in the last 7 day(s))"); + // The summary's not-enough-blocked arm survives verbatim. + expect(body).toContain("- Gate false-positive rate: not enough blocked PRs in the window to report."); + }); + it("omits cohort diagnostics from the public recap even when totals.cohorts is present", () => { const report: RecapReport = { ...emptyReport(), diff --git a/test/unit/maintainer-recap.test.ts b/test/unit/maintainer-recap.test.ts index 7e41672cf4..bba1933dbd 100644 --- a/test/unit/maintainer-recap.test.ts +++ b/test/unit/maintainer-recap.test.ts @@ -30,9 +30,12 @@ function repoInput( const bands: OutcomeCalibration["slop"]["bands"] = c.emptyBands ? [] : [{ band: "clean", sampleSize: 0, merged: c.merged ?? 0, closed: c.closed ?? 0, mergeRate: 0 }]; + // Mirror gate-precision.ts's real per-cohort floor (#9691): a GatePrecisionReport nulls the rate below + // MIN_GATE_PRECISION_SAMPLE (5) blocks, so a fixture simulating that report must too — otherwise it feeds the + // recap a per-repo rate production would never produce. const cohortReport = (counts: { blocked: number; blockedThenMerged: number }) => ({ perGateType: [], - overall: { blocked: counts.blocked, blockedThenMerged: counts.blockedThenMerged, falsePositiveRate: counts.blocked > 0 ? Math.round((counts.blockedThenMerged / counts.blocked) * 1000) / 1000 : null }, + overall: { blocked: counts.blocked, blockedThenMerged: counts.blockedThenMerged, falsePositiveRate: counts.blocked >= 5 ? Math.round((counts.blockedThenMerged / counts.blocked) * 1000) / 1000 : null }, }); return { gatePrecision: { @@ -145,13 +148,15 @@ describe("buildMaintainerRecap cohort split (#4521)", () => { }), ], }); + // Both cohorts are below the 5-block floor, so their rate reads null (#9691) — per-repo (fed from the + // already-floored GatePrecisionReport) and the cross-repo aggregate alike. Raw counts are still reported. expect(report.repos[0]?.cohorts).toMatchObject({ - miner: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: 0.5 }, - human: { blocked: 4, gateFalsePositives: 1, gateFalsePositiveRate: 0.25 }, + miner: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: null }, + human: { blocked: 4, gateFalsePositives: 1, gateFalsePositiveRate: null }, }); expect(report.totals.cohorts).toMatchObject({ - miner: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: 0.5 }, - human: { blocked: 4, gateFalsePositives: 1, gateFalsePositiveRate: 0.25 }, + miner: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: null }, + human: { blocked: 4, gateFalsePositives: 1, gateFalsePositiveRate: null }, }); expect(report.summary).toHaveLength(3); expect(report.summary.join("\n")).not.toContain("Miner-originated"); @@ -166,9 +171,66 @@ describe("buildMaintainerRecap cohort split (#4521)", () => { repoInput("owner/repo-b", { blocked: 3, blockedThenMerged: 1, cohorts: { miner: { blocked: 1, blockedThenMerged: 0 }, human: { blocked: 2, blockedThenMerged: 1 } } }), ], }); + // Both aggregated cohorts are below the 5-block floor → null rate, not 0 or 0.5 (#9691). expect(report.totals.cohorts).toMatchObject({ - miner: { blocked: 3, gateFalsePositives: 0, gateFalsePositiveRate: 0 }, - human: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: 0.5 }, + miner: { blocked: 3, gateFalsePositives: 0, gateFalsePositiveRate: null }, + human: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: null }, + }); + }); + + describe("aggregate gate false-positive rate honors the shared MIN_GATE_PRECISION_SAMPLE floor (#9691)", () => { + it("nulls the blended rate when two repos each block only once (2/2 total, below the 5-floor)", () => { + const report = buildMaintainerRecap({ + generatedAt: GEN, + repos: [ + repoInput("owner/repo-a", { blocked: 1, blockedThenMerged: 1 }), + repoInput("owner/repo-b", { blocked: 1, blockedThenMerged: 1 }), + ], + }); + // Raw counts still reported; the rate reads null instead of a contradictory 100%. + expect(report.totals).toMatchObject({ blocked: 2, gateFalsePositives: 2, gateFalsePositiveRate: null }); + }); + + it("reports the rate once the blended sample reaches the floor (blocked 5, fp 1 → 0.2)", () => { + const report = buildMaintainerRecap({ + generatedAt: GEN, + repos: [ + repoInput("owner/repo-a", { blocked: 3, blockedThenMerged: 1 }), + repoInput("owner/repo-b", { blocked: 2, blockedThenMerged: 0 }), + ], + }); + expect(report.totals).toMatchObject({ blocked: 5, gateFalsePositives: 1, gateFalsePositiveRate: 0.2 }); + }); + + it("floors each cohort against its OWN blocked denominator, not the blended one", () => { + const report = buildMaintainerRecap({ + generatedAt: GEN, + repos: [ + // Blended blocked = 8 (≥5), but the human cohort's own blocked is 2 and the miner's is 6. + repoInput("owner/repo-a", { blocked: 6, blockedThenMerged: 3, cohorts: { miner: { blocked: 6, blockedThenMerged: 3 }, human: { blocked: 0, blockedThenMerged: 0 } } }), + repoInput("owner/repo-b", { blocked: 2, blockedThenMerged: 1, cohorts: { miner: { blocked: 0, blockedThenMerged: 0 }, human: { blocked: 2, blockedThenMerged: 1 } } }), + ], + }); + expect(report.totals.cohorts).toMatchObject({ + miner: { blocked: 6, gateFalsePositives: 3, gateFalsePositiveRate: 0.5 }, // ≥5 → reported + human: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: null }, // <5 → null despite blended ≥5 + }); + // The blended rate itself is reported (total blocked 8 ≥ 5). + expect(report.totals.gateFalsePositiveRate).toBe(0.5); + }); + + it("reports each cohort rate when BOTH cohorts are at or above their own floor", () => { + const report = buildMaintainerRecap({ + generatedAt: GEN, + repos: [ + repoInput("owner/repo-a", { blocked: 10, blockedThenMerged: 5, cohorts: { miner: { blocked: 5, blockedThenMerged: 1 }, human: { blocked: 5, blockedThenMerged: 2 } } }), + ], + }); + // Both cohorts' own blocked ≥ 5 → both report a number (the ≥-floor truthy arm on each). + expect(report.totals.cohorts).toMatchObject({ + miner: { blocked: 5, gateFalsePositives: 1, gateFalsePositiveRate: 0.2 }, + human: { blocked: 5, gateFalsePositives: 2, gateFalsePositiveRate: 0.4 }, + }); }); }); @@ -351,7 +413,9 @@ describe("runMaintainerRecap (#2252 end-to-end orchestration)", () => { it("builds, formats, and fans out to BOTH channels when both webhooks are configured", async () => { const calls = stubRecapChannelFetch(); - const result = await runMaintainerRecap(envWithBothWebhooks(), { repos: [repoInput("owner/repo-a", { blocked: 4, blockedThenMerged: 1, totalResolved: 2, merged: 1, closed: 1 })] }); + // blocked: 5 keeps this fan-out check at or above the false-positive floor (#9691) so a rendered percentage + // still exists to assert against; the digest content itself is exercised in dedicated builder/format tests. + const result = await runMaintainerRecap(envWithBothWebhooks(), { repos: [repoInput("owner/repo-a", { blocked: 5, blockedThenMerged: 1, totalResolved: 2, merged: 1, closed: 1 })] }); expect(result.skipped).toBe(false); if (result.skipped) return; expect(result.formatted).toContain("# Maintainer recap");