diff --git a/src/review/maintainer-recap-wire.ts b/src/review/maintainer-recap-wire.ts index 20651e283e..08f61e9387 100644 --- a/src/review/maintainer-recap-wire.ts +++ b/src/review/maintainer-recap-wire.ts @@ -176,7 +176,10 @@ export async function runMaintainerRecapJob( for (const repoFullName of repoNames) { try { const [gatePrecision, calibration] = await Promise.all([ - loadGatePrecisionReport(env, repoFullName, { windowDays: resolvedWindowDays }), + // #4521: a periodic digest is exactly the "occasional aggregate view" includeCohorts was designed + // for -- unlike a hot webhook path, one extra Gittensor API call per repo per recap run is a small, + // bounded cost, so this call site opts in by default rather than needing its own separate flag. + loadGatePrecisionReport(env, repoFullName, { windowDays: resolvedWindowDays, includeCohorts: true }), buildRepoOutcomeCalibration(env, repoFullName, resolvedWindowDays), ]); repos.push({ gatePrecision, calibration }); diff --git a/src/services/maintainer-recap.ts b/src/services/maintainer-recap.ts index da0381bec1..1dd12bcda5 100644 --- a/src/services/maintainer-recap.ts +++ b/src/services/maintainer-recap.ts @@ -14,7 +14,7 @@ import { PUBLIC_LOCAL_PATH_SCRUB_PATTERN, PUBLIC_UNSAFE_PATTERN } from "../signa import { deliverRecapToDiscord, deliverRecapToSlack } from "./notify-discord"; import type { GatePrecisionReport } from "./gate-precision"; import type { OutcomeCalibration } from "./outcome-calibration"; -import type { MaintainerRecapRepo, RecapReport } from "../types"; +import type { MaintainerRecapCohortCounts, MaintainerRecapRepo, RecapReport } from "../types"; import { nowIso } from "../utils/json"; const DEFAULT_WINDOW_DAYS = 7; @@ -45,6 +45,12 @@ export type MaintainerRecapInputs = { repos: MaintainerRecapRepoInput[]; }; +/** #4521: convert one GatePrecisionCohortReport's `overall` bucket into the recap's own cohort-counts shape + * (renamed fields to match this file's gateFalsePositives/gateFalsePositiveRate convention). Pure. */ +function toRecapCohortCounts(overall: { blocked: number; blockedThenMerged: number; falsePositiveRate: number | null }): MaintainerRecapCohortCounts { + return { blocked: overall.blocked, gateFalsePositives: overall.blockedThenMerged, gateFalsePositiveRate: overall.falsePositiveRate }; +} + /** PURE recap builder: fold each repo's gate-precision + outcome-calibration reports into a {@link RecapReport} * with per-repo counts and top-line gate/reversal totals. Never throws; an empty repo list yields a zeroed * report with a null false-positive rate (nothing blocked ⇒ nothing to divide by). */ @@ -61,6 +67,14 @@ export function buildMaintainerRecap(args: MaintainerRecapInputs): RecapReport { reversals: 0, gateFalsePositiveRate: null as number | null, }; + // #4521: accumulated only across repos whose GatePrecisionReport actually carried `cohorts` (loadGate + // PrecisionReport's includeCohorts option) -- a repo without one simply doesn't contribute, so a window + // mixing cohort-aware and legacy call sites still degrades gracefully rather than half-reporting zeros. + let cohortBlockedRepos = 0; + const cohortTotals = { + miner: { blocked: 0, gateFalsePositives: 0 }, + human: { blocked: 0, gateFalsePositives: 0 }, + }; for (const { gatePrecision, calibration } of args.repos) { let merged = 0; let closed = 0; @@ -78,6 +92,9 @@ export function buildMaintainerRecap(args: MaintainerRecapInputs): RecapReport { gateFalsePositives: gatePrecision.overall.blockedThenMerged, gateOverrides, reversals: calibration.recommendations.negative, + ...(gatePrecision.cohorts + ? { cohorts: { miner: toRecapCohortCounts(gatePrecision.cohorts.miner.overall), human: toRecapCohortCounts(gatePrecision.cohorts.human.overall) } } + : {}), }; repos.push(repo); totals.reviewed += repo.reviewed; @@ -87,19 +104,54 @@ export function buildMaintainerRecap(args: MaintainerRecapInputs): RecapReport { totals.gateFalsePositives += repo.gateFalsePositives; totals.gateOverrides += repo.gateOverrides; totals.reversals += repo.reversals; + if (gatePrecision.cohorts) { + cohortBlockedRepos += 1; + cohortTotals.miner.blocked += gatePrecision.cohorts.miner.overall.blocked; + cohortTotals.miner.gateFalsePositives += gatePrecision.cohorts.miner.overall.blockedThenMerged; + cohortTotals.human.blocked += gatePrecision.cohorts.human.overall.blocked; + cohortTotals.human.gateFalsePositives += gatePrecision.cohorts.human.overall.blockedThenMerged; + } } totals.gateFalsePositiveRate = totals.blocked > 0 ? 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, + }, + 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, + }, + } + : undefined; const rateLine = totals.gateFalsePositiveRate !== null ? `Gate false-positive rate: ${Math.round(totals.gateFalsePositiveRate * 100)}% (${totals.gateFalsePositives}/${totals.blocked} block(s) later merged).` : `Gate false-positive rate: not enough blocked PRs in the window to report.`; + // #4521: an additional summary line ONLY when the cohort split was actually requested this run — omitted + // (not "N/A") when absent, so a legacy call site's summary output is byte-identical to before this existed. + const cohortLine = cohorts ? [formatCohortSummaryLine(cohorts)] : []; const summary = [ `Maintainer recap over the last ${windowDays} day(s): ${repos.length} repo(s), ${totals.reviewed} reviewed, ${totals.merged} merged, ${totals.closed} closed.`, rateLine, `${totals.gateOverrides} maintainer override(s), ${totals.reversals} recommendation reversal(s).`, + ...cohortLine, ].map(sanitizeRecapText); - return { generatedAt: args.generatedAt, windowDays, repos, totals, summary }; + return { generatedAt: args.generatedAt, windowDays, repos, totals: { ...totals, ...(cohorts ? { cohorts } : {}) }, summary }; +} + +/** #4521: "N of M blocked PRs were miner-originated, precision X% vs human Y%" — mirrors rateLine's own + * null-below-sample handling per cohort (a cohort's own falsePositiveRate is already null when its blocked + * count is 0, from GatePrecisionCohortReport's MIN_SAMPLE floor at the source). */ +function formatCohortSummaryLine(cohorts: { miner: MaintainerRecapCohortCounts; human: MaintainerRecapCohortCounts }): string { + const rate = (counts: MaintainerRecapCohortCounts): string => + counts.gateFalsePositiveRate !== null ? `${Math.round(counts.gateFalsePositiveRate * 100)}%` : "n/a"; + return `Miner-originated: ${cohorts.miner.blocked} blocked (${rate(cohorts.miner)} false-positive) — Human-originated: ${cohorts.human.blocked} blocked (${rate(cohorts.human)} false-positive).`; } /** Redact one free-text line bound for the public digest body. Two arms mirroring weekly-value-report.ts's @@ -147,12 +199,25 @@ export function formatMaintainerRecap(report: RecapReport): string { `- Overrides: ${totals.gateOverrides}`, `- Reversals: ${totals.reversals}`, "", + // #4521: an entire section, only when the cohort split was requested this run -- omitted (not an empty + // header) when absent, so the digest degrades gracefully to exactly today's output. + ...(totals.cohorts ? ["## Cohorts", ...formatCohortLines(totals.cohorts), ""] : []), "## Per-repo", ...recapSectionLines(perRepoLines, "_No repositories in this window._"), ]; return `${lines.join("\n").replace(/\n{3,}/g, "\n\n").trimEnd()}\n`; } +/** #4521: render the aggregate miner-vs-human split as two bullet lines, mirroring the Totals section's own + * "gate false positives: N/M (rate)" phrasing per cohort. */ +function formatCohortLines(cohorts: { miner: MaintainerRecapCohortCounts; human: MaintainerRecapCohortCounts }): string[] { + const line = (label: string, counts: MaintainerRecapCohortCounts): string => { + const cohortRate = counts.gateFalsePositiveRate !== null ? `${Math.round(counts.gateFalsePositiveRate * 100)}%` : "n/a"; + return `- ${label}: ${counts.gateFalsePositives}/${counts.blocked} gate false positives (${cohortRate})`; + }; + return [line("Miner-originated", cohorts.miner), line("Human-originated", cohorts.human)]; +} + export type RunMaintainerRecapResult = | { skipped: true; reason: "disabled" } | { diff --git a/src/types.ts b/src/types.ts index 6692e73cb8..b8a22429ad 100644 --- a/src/types.ts +++ b/src/types.ts @@ -2444,6 +2444,15 @@ export type ReviewRecap = { summary: string[]; }; +/** #4521: one cohort's blocked/false-positive counts within a maintainer recap window — the SAME shape for + * both the per-repo and aggregate-totals cohort splits. Mirrors GatePrecisionCohortReport's overall shape, + * renamed to match this file's own gateFalsePositives/gateFalsePositiveRate naming convention. */ +export type MaintainerRecapCohortCounts = { + blocked: number; + gateFalsePositives: number; + gateFalsePositiveRate: number | null; +}; + /** One repo's realized review-outcome roll-up inside a maintainer recap window (#2239, foundation for #1963). * Counts are ground-truth PR outcomes + gate/recommendation calibration totals — never predictions. */ export type MaintainerRecapRepo = { @@ -2458,6 +2467,10 @@ export type MaintainerRecapRepo = { gateOverrides: number; /** Recommendations that resolved NEGATIVELY (a reversal) over the window, from the outcome calibration. */ reversals: number; + /** #4521: miner-vs-human split of this repo's gate-block outcomes, present only when the caller's + * GatePrecisionReport carried a `cohorts` field (loadGatePrecisionReport's `includeCohorts` option). + * Absent means the split wasn't requested for this recap run — never a signal that it doesn't apply. */ + cohorts?: { miner: MaintainerRecapCohortCounts; human: MaintainerRecapCohortCounts } | undefined; }; /** A serializable maintainer recap: a window of gittensory's OWN review-outcome data folded across repos. @@ -2479,6 +2492,10 @@ export type RecapReport = { reversals: number; /** Aggregate false-positive rate (gateFalsePositives / blocked), null when nothing was blocked. */ gateFalsePositiveRate: number | null; + /** #4521: aggregate miner-vs-human split across every repo that carried one — present only when at + * least one repo's GatePrecisionReport included `cohorts`. A repo without one simply doesn't + * contribute to these sums, so a partial-adoption window still degrades gracefully. */ + cohorts?: { miner: MaintainerRecapCohortCounts; human: MaintainerRecapCohortCounts } | undefined; }; summary: string[]; }; diff --git a/test/unit/maintainer-recap-format.test.ts b/test/unit/maintainer-recap-format.test.ts index 289df589bf..e2c7a566ed 100644 --- a/test/unit/maintainer-recap-format.test.ts +++ b/test/unit/maintainer-recap-format.test.ts @@ -90,4 +90,33 @@ describe("formatMaintainerRecap (#2240)", () => { expect(body).toContain("- "); expect(body).not.toContain("payout"); }); + + // #4521: the whole "## Cohorts" section is additive -- absent when totals.cohorts is, present (with both + // cohort lines) when it's supplied. + it("omits the Cohorts section entirely when totals.cohorts is absent (byte-identical to before the split existed)", () => { + const body = formatMaintainerRecap(emptyReport()); + expect(body).not.toContain("## Cohorts"); + expect(body).not.toContain("Miner-originated"); + }); + + it("renders the Cohorts section with both lines when totals.cohorts is present", () => { + const report: RecapReport = { + ...emptyReport(), + totals: { + ...emptyReport().totals, + cohorts: { + miner: { blocked: 3, gateFalsePositives: 1, gateFalsePositiveRate: 0.333 }, + human: { blocked: 5, gateFalsePositives: 0, gateFalsePositiveRate: 0 }, + }, + }, + }; + const body = formatMaintainerRecap(report); + expect(body).toContain("## Cohorts"); + expect(body).toContain("- Miner-originated: 1/3 gate false positives (33%)"); + expect(body).toContain("- Human-originated: 0/5 gate false positives (0%)"); + // The section sits between Totals and Per-repo, and the trailing-blank-line collapse still holds. + expect(body.indexOf("## Totals")).toBeLessThan(body.indexOf("## Cohorts")); + expect(body.indexOf("## Cohorts")).toBeLessThan(body.indexOf("## Per-repo")); + expect(body).not.toMatch(/\n{3,}/); + }); }); diff --git a/test/unit/maintainer-recap-wire.test.ts b/test/unit/maintainer-recap-wire.test.ts index dca741b9b2..5631a3a6db 100644 --- a/test/unit/maintainer-recap-wire.test.ts +++ b/test/unit/maintainer-recap-wire.test.ts @@ -2,7 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { isRecapEnabled, resolveMaintainerRecapManifestOverride, runMaintainerRecapJob, shouldFireMaintainerRecap } from "../../src/review/maintainer-recap-wire"; import type { MaintainerRecapJobSkipped } from "../../src/review/maintainer-recap-wire"; import type { RunMaintainerRecapResult } from "../../src/services/maintainer-recap"; -import { updatePullRequestSlopAssessment, upsertPullRequestFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; +import { recordGateBlockOutcome, updatePullRequestSlopAssessment, upsertPullRequestFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { createTestEnv } from "../helpers/d1"; @@ -189,6 +189,34 @@ describe("runMaintainerRecapJob — cross-repo digest (#1963, #2248)", () => { expect(posted).toHaveLength(1); }); + // #4521: runMaintainerRecapJob always opts loadGatePrecisionReport into includeCohorts -- proves the split + // actually reaches the finished report/formatted digest, not just that the wiring doesn't crash (every + // OTHER test in this file also exercises includeCohorts implicitly since it's now unconditional, but none + // of them seed a gate block or a miner author, so none would catch a real miner-vs-human misclassification). + it("populates totals.cohorts end-to-end when a blocked PR's author is a confirmed miner", async () => { + const env = createTestEnv({ DISCORD_WEBHOOK_URL: HOOK }); + await seedRegisteredRepo(env, "owner/alpha"); + await upsertPullRequestFromGitHub(env, "owner/alpha", { number: 1, title: "miner PR", state: "closed", user: { login: "miner-alice" } }); + await recordGateBlockOutcome(env, { repoFullName: "owner/alpha", pullNumber: 1, blockerCodes: ["slop_risk"] }); + await upsertPullRequestFromGitHub(env, "owner/alpha", { number: 2, title: "human PR", state: "closed", user: { login: "human-bob" } }); + await recordGateBlockOutcome(env, { repoFullName: "owner/alpha", pullNumber: 2, blockerCodes: ["slop_risk"] }); + vi.stubGlobal("fetch", async (url: RequestInfo | URL, init?: RequestInit) => { + if (String(url) === HOOK) return new Response(null, { status: 204 }); + if (String(url) === "https://api.gittensor.io/miners") return Response.json([{ uid: 1, githubUsername: "miner-alice", githubId: "1" }]); + return new Response(null, { status: 204 }); + }); + + const { report, formatted } = ranRecap(await runMaintainerRecapJob(env)); + + expect(report.totals.cohorts).toMatchObject({ miner: { blocked: 1 }, human: { blocked: 1 } }); + expect(report.repos[0]?.cohorts).toMatchObject({ miner: { blocked: 1 }, human: { blocked: 1 } }); + expect(formatted).toContain("## Cohorts"); + // Neither PR merged (both stay "closed"), so blockedThenMerged is 0 for both cohorts -- only `blocked` + // differs from zero here. + expect(formatted).toContain("Miner-originated: 0/1 gate false positives"); + expect(formatted).toContain("Human-originated: 0/1 gate false positives"); + }); + it("threads a custom windowDays through to the report and the per-repo aggregators", async () => { const env = createTestEnv({ DISCORD_WEBHOOK_URL: HOOK }); await seedRegisteredRepo(env, "owner/alpha"); diff --git a/test/unit/maintainer-recap.test.ts b/test/unit/maintainer-recap.test.ts index e30ec86511..92d3878704 100644 --- a/test/unit/maintainer-recap.test.ts +++ b/test/unit/maintainer-recap.test.ts @@ -8,7 +8,8 @@ const GEN = "2026-07-08T00:00:00.000Z"; const DISCORD_HOOK = "https://discord.com/api/webhooks/123/abc"; const SLACK_HOOK = "https://hooks.slack.com/services/T00/B00/xxxyyyzzz"; -/** Build one repo's injected inputs from the handful of counts this builder actually reads. */ +/** Build one repo's injected inputs from the handful of counts this builder actually reads. `cohorts` (#4521) + * is omitted by default -- every pre-existing call site keeps exercising the no-cohorts (legacy) arm. */ function repoInput( repoFullName: string, c: { @@ -20,6 +21,7 @@ function repoInput( closed?: number; reversals?: number; emptyBands?: boolean; + cohorts?: { miner: { blocked: number; blockedThenMerged: number }; human: { blocked: number; blockedThenMerged: number } }; } = {}, ): MaintainerRecapRepoInput { const blocked = c.blocked ?? 0; @@ -27,6 +29,10 @@ function repoInput( const bands: OutcomeCalibration["slop"]["bands"] = c.emptyBands ? [] : [{ band: "clean", sampleSize: 0, merged: c.merged ?? 0, closed: c.closed ?? 0, mergeRate: 0 }]; + 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 }, + }); return { gatePrecision: { repoFullName, @@ -35,6 +41,7 @@ function repoInput( perGateType: [{ gateType: "missing_linked_issue", blocked, blockedThenMerged, overridden: c.overridden ?? 0, falsePositiveRate: null }], overall: { blocked, blockedThenMerged, falsePositiveRate: null }, signals: [], + ...(c.cohorts ? { cohorts: { miner: cohortReport(c.cohorts.miner), human: cohortReport(c.cohorts.human) } } : {}), }, calibration: { repoFullName, @@ -101,6 +108,69 @@ describe("buildMaintainerRecap (#2239)", () => { }); }); +// #4521: additive miner-vs-human cohort split, computed only for repos whose injected GatePrecisionReport +// actually carried `cohorts` (loadGatePrecisionReport's includeCohorts option, #4520). +describe("buildMaintainerRecap cohort split (#4521)", () => { + it("omits totals.cohorts and the per-repo cohorts field when no repo's report carried one (byte-identical to before the split existed)", () => { + const report = buildMaintainerRecap({ generatedAt: GEN, repos: [repoInput("owner/repo-a", { blocked: 4, blockedThenMerged: 1 })] }); + expect(report.totals.cohorts).toBeUndefined(); + expect(report.repos[0]?.cohorts).toBeUndefined(); + expect(report.summary).toHaveLength(3); // no 4th cohort summary line either + }); + + it("attaches the per-repo split verbatim and aggregates it into totals.cohorts", () => { + const report = buildMaintainerRecap({ + generatedAt: GEN, + repos: [ + repoInput("owner/repo-a", { + blocked: 6, blockedThenMerged: 2, + cohorts: { miner: { blocked: 2, blockedThenMerged: 1 }, human: { blocked: 4, blockedThenMerged: 1 } }, + }), + ], + }); + expect(report.repos[0]?.cohorts).toMatchObject({ + miner: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: 0.5 }, + human: { blocked: 4, gateFalsePositives: 1, gateFalsePositiveRate: 0.25 }, + }); + expect(report.totals.cohorts).toMatchObject({ + miner: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: 0.5 }, + human: { blocked: 4, gateFalsePositives: 1, gateFalsePositiveRate: 0.25 }, + }); + expect(report.summary[3]).toContain("Miner-originated: 2 blocked (50% false-positive)"); + expect(report.summary[3]).toContain("Human-originated: 4 blocked (25% false-positive)"); + }); + + it("sums cohorts ACROSS repos, correctly reporting n/a for a zero-blocked cohort", () => { + const report = buildMaintainerRecap({ + generatedAt: GEN, + repos: [ + repoInput("owner/repo-a", { blocked: 2, blockedThenMerged: 0, cohorts: { miner: { blocked: 2, blockedThenMerged: 0 }, human: { blocked: 0, blockedThenMerged: 0 } } }), + repoInput("owner/repo-b", { blocked: 3, blockedThenMerged: 1, cohorts: { miner: { blocked: 1, blockedThenMerged: 0 }, human: { blocked: 2, blockedThenMerged: 1 } } }), + ], + }); + expect(report.totals.cohorts).toMatchObject({ + miner: { blocked: 3, gateFalsePositives: 0, gateFalsePositiveRate: 0 }, + human: { blocked: 2, gateFalsePositives: 1, gateFalsePositiveRate: 0.5 }, + }); + }); + + it("degrades gracefully when only SOME repos in the window carried a cohort split (partial adoption)", () => { + const report = buildMaintainerRecap({ + generatedAt: GEN, + repos: [ + repoInput("owner/repo-a", { blocked: 4, blockedThenMerged: 1 }), // no cohorts -- doesn't contribute + repoInput("owner/repo-b", { blocked: 2, blockedThenMerged: 1, cohorts: { miner: { blocked: 1, blockedThenMerged: 1 }, human: { blocked: 1, blockedThenMerged: 0 } } }), + ], + }); + expect(report.repos[0]?.cohorts).toBeUndefined(); + expect(report.repos[1]?.cohorts).toBeDefined(); + // Only repo-b's counts feed the aggregate -- repo-a's 4 blocked never appear here. + expect(report.totals.cohorts).toMatchObject({ miner: { blocked: 1 }, human: { blocked: 1 } }); + // But repo-a's counts STILL feed the ordinary (non-cohort) totals, unaffected by the split. + expect(report.totals.blocked).toBe(6); + }); +}); + function envWithBothWebhooks(): Env { return createTestEnv({ DISCORD_WEBHOOK_URL: DISCORD_HOOK, SLACK_WEBHOOK_URL: SLACK_HOOK }) as Env; }