Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/review/maintainer-recap-wire.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
69 changes: 67 additions & 2 deletions src/services/maintainer-recap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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). */
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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" }
| {
Expand Down
17 changes: 17 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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.
Expand All @@ -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[];
};
29 changes: 29 additions & 0 deletions test/unit/maintainer-recap-format.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,33 @@ describe("formatMaintainerRecap (#2240)", () => {
expect(body).toContain("- <redacted>");
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,}/);
});
});
30 changes: 29 additions & 1 deletion test/unit/maintainer-recap-wire.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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");
Expand Down
Loading