From be6069296995278f2abf4d264716aafd2c72894a Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 24 Jul 2026 22:17:18 +0800 Subject: [PATCH] fix: wire the recap calibration/gate-outcomes/per-repo builders into the digest The three section builders shipped fully implemented and unit-tested but were never composed into the delivered digest, and formatMaintainerRecap's per-repo body was a second, drifted inline copy of buildPerRepoRecapSection. Co-Authored-By: Claude Opus 4.8 --- src/services/maintainer-recap.ts | 34 +++++++++++++++++++---- test/unit/maintainer-recap-format.test.ts | 14 ++++++++-- test/unit/maintainer-recap.test.ts | 15 +++++++++- 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/services/maintainer-recap.ts b/src/services/maintainer-recap.ts index 111c99c101..77b9225c2b 100644 --- a/src/services/maintainer-recap.ts +++ b/src/services/maintainer-recap.ts @@ -14,6 +14,11 @@ 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 { 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. +import { buildCalibrationRecapSection } from "./maintainer-recap-calibration"; +import { buildGateOutcomesRecapSection } from "./maintainer-recap-gate-outcomes"; +import { buildPerRepoRecapSection } from "./maintainer-recap-per-repo"; import type { OutcomeCalibration } from "./outcome-calibration"; import type { MaintainerRecapCohortCounts, MaintainerRecapRepo, RecapReport } from "../types"; import { nowIso } from "../utils/json"; @@ -165,10 +170,12 @@ function recapSectionLines(items: string[], fallback: string): string[] { export function formatMaintainerRecap(report: RecapReport, options: { configDrift?: DriftRecapSection } = {}): string { const { totals } = report; const rate = totals.gateFalsePositiveRate !== null ? `${Math.round(totals.gateFalsePositiveRate * 100)}%` : "n/a"; - const perRepoLines = report.repos.map( - (repo) => - `${redactRecapLine(repo.repoFullName)} — ${repo.reviewed} reviewed, ${repo.merged} merged, ${repo.closed} closed, ${repo.gateFalsePositives} gate false-positive(s), ${repo.gateOverrides} override(s), ${repo.reversals} reversal(s)`, - ); + // #8372: the dedicated builder replaces the inline map that duplicated it -- unlike this file's old copy, + // it sorts, caps the list, and emits a "(+N more)" remainder line. + const perRepoSection = buildPerRepoRecapSection({ windowDays: report.windowDays, repos: report.repos }); + const perRepoLines = perRepoSection.lines.map(redactRecapLine); + const calibrationSection = buildCalibrationRecapSection({ windowDays: report.windowDays, totals: report.totals }); + const gateOutcomesSection = buildGateOutcomesRecapSection({ windowDays: report.windowDays, totals: report.totals }); const lines = [ "# Maintainer recap", "", @@ -189,6 +196,14 @@ export function formatMaintainerRecap(report: RecapReport, options: { configDrif "", "## Per-repo", ...recapSectionLines(perRepoLines, "_No repositories in this window._"), + "", + // #8372: unconditional (not behind an options flag) -- both sections read only report.totals/windowDays, + // which every RecapReport always carries, so there is nothing for a caller to opt into. + `## ${redactRecapLine(calibrationSection.title)}`, + ...recapSectionLines(calibrationSection.lines.map(redactRecapLine), "_No calibration lines for this window._"), + "", + `## ${redactRecapLine(gateOutcomesSection.title)}`, + ...recapSectionLines(gateOutcomesSection.lines.map(redactRecapLine), "_No gate-outcome lines for this window._"), // #8214: optional config-drift section (maintainer-recap-drift.ts) — appended only when the caller has a // sentinel projection to render, so every existing digest stays byte-identical until the sentinel wires in. ...(options.configDrift @@ -227,6 +242,11 @@ export async function runMaintainerRecap( report?: RecapReport; /** When explicitly false, short-circuits before build/format/delivery. Default: run. */ enabled?: boolean; + /** #8372: forwarded to {@link formatMaintainerRecap} so a caller holding a drift projection can have it + * rendered. Deliberately NOT sourced here -- reading the knob-loosening sentinel state is its own + * data-sourcing concern; this is only the plumbing, so the section stays absent until a caller passes it + * and every existing digest is unaffected. */ + configDrift?: DriftRecapSection; } = {}, ): Promise { if (options.enabled === false) return { skipped: true, reason: "disabled" }; @@ -238,7 +258,11 @@ export async function runMaintainerRecap( windowDays: options.windowDays, repos: options.repos ?? [], }); - const formatted = formatMaintainerRecap(report); + // Two call arms rather than a spread of a conditional object: exactOptionalPropertyTypes forbids passing + // `configDrift: undefined`, and the spread form trips unicorn/no-useless-spread. + const formatted = options.configDrift + ? formatMaintainerRecap(report, { configDrift: options.configDrift }) + : formatMaintainerRecap(report); const [discord, slack] = await Promise.all([ deliverRecapToDiscord(env, report, formatted), deliverRecapToSlack(env, report, formatted), diff --git a/test/unit/maintainer-recap-format.test.ts b/test/unit/maintainer-recap-format.test.ts index 68800f993e..aba1edf41f 100644 --- a/test/unit/maintainer-recap-format.test.ts +++ b/test/unit/maintainer-recap-format.test.ts @@ -33,12 +33,18 @@ describe("formatMaintainerRecap (#2240)", () => { expect(body).toContain("## Summary"); expect(body).toContain("## Totals"); expect(body).toContain("## Per-repo"); + // #8372: both builders read only totals/windowDays, so their sections are unconditional. + expect(body).toContain("## Calibration"); + expect(body).toContain("## Gate outcomes"); // #8214: without a sentinel projection the drift section is entirely absent — the digest stays // byte-identical to the pre-drift shape, not a dangling empty header. expect(body).not.toContain("## Config drift"); // Empty sections show a single fallback line instead of dangling under the header. expect(body).toContain("_No summary lines for this window._"); - expect(body).toContain("_No repositories in this window._"); + // #8372: the ## Per-repo body now comes from buildPerRepoRecapSection, which emits its own + // windowed empty-state line, so the section is never empty and the generic fallback never fires. + expect(body).toContain("No repo activity in the last 7 day(s)."); + expect(body).not.toContain("_No repositories in this window._"); // Null rate ⇒ the "n/a" arm. expect(body).toContain("- Gate false positives: 0/0 (n/a)"); expect(body).toContain("- Repos: 0"); @@ -98,8 +104,10 @@ describe("formatMaintainerRecap (#2240)", () => { // Numeric / non-null rate arm. expect(body).toContain("- Gate false positives: 1/4 (25%)"); expect(body).toContain("- Repos: 1"); - // Per-repo row rendered (non-empty section arm). - expect(body).toContain("acme/widgets — 5 reviewed, 3 merged, 2 closed, 1 gate false-positive(s), 1 override(s), 0 reversal(s)"); + // Per-repo row rendered (non-empty section arm), now in buildPerRepoRecapSection's row format (#8372). + // The gate/override/reversal counts this row used to carry are unchanged in ## Totals above, and are + // broken out per-dimension by the ## Gate outcomes section this digest now composes. + expect(body).toContain("acme/widgets: reviewed 5, merged 3, closed 2"); // Clean summary line survives verbatim (redaction no-op arm). expect(body).toContain("- Normal recap line about resolved reviews."); // Arm 1: local path scrubbed to the placeholder, raw path gone. diff --git a/test/unit/maintainer-recap.test.ts b/test/unit/maintainer-recap.test.ts index d4dbd79fc7..4f46d1cec2 100644 --- a/test/unit/maintainer-recap.test.ts +++ b/test/unit/maintainer-recap.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { buildMaintainerRecap, runMaintainerRecap, type MaintainerRecapRepoInput } from "../../src/services/maintainer-recap"; +import { buildDriftRecapSection } from "../../src/services/maintainer-recap-drift"; import type { OutcomeCalibration } from "../../src/services/outcome-calibration"; import type { RecapReport } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; @@ -229,10 +230,22 @@ describe("runMaintainerRecap (#2252 end-to-end orchestration)", () => { expect(result.skipped).toBe(false); if (result.skipped) return; expect(result.report.repos).toEqual([]); - expect(result.formatted).toContain("_No repositories in this window._"); + // #8372: the ## Per-repo body is buildPerRepoRecapSection's, which carries its own empty-state line. + expect(result.formatted).toContain("No repo activity in the last 7 day(s)."); expect(result.formatted).toContain("(n/a)"); }); + it("forwards a caller-supplied configDrift projection into the delivered digest (#8372 present arm)", async () => { + const calls = stubRecapChannelFetch(); + const configDrift = buildDriftRecapSection({ generatedAt: GEN, sentinelEnabled: false, drifting: [], cleanKnobs: 0 }); + const result = await runMaintainerRecap(envWithBothWebhooks(), { configDrift }); + expect(result.skipped).toBe(false); + if (result.skipped) return; + expect(result.formatted).toContain("## Config drift"); + // Reaches the actual delivered payload, not just the returned string. + expect(calls.some((c) => c.body.includes("## Config drift"))).toBe(true); + }); + it("short-circuits when enabled is false — no build/format/fetch (flag-OFF arm)", async () => { const calls = stubRecapChannelFetch(); const result = await runMaintainerRecap(envWithBothWebhooks(), { enabled: false, repos: [repoInput("owner/repo")] });