From 6e51fa30cfc13f9747b63e19d9cf761edce432fe Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Fri, 24 Jul 2026 22:17:18 +0800 Subject: [PATCH 1/2] 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 | 36 +++++++++++++++++++---- test/unit/maintainer-recap-format.test.ts | 14 +++++++-- test/unit/maintainer-recap.test.ts | 15 +++++++++- 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/services/maintainer-recap.ts b/src/services/maintainer-recap.ts index e0d000943c..8da8c4f187 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 { buildRoutingRecapSection } from "./maintainer-recap-routing"; import { REVIEWER_ROUTING_SHADOW_EVENT_TYPE, type RoutingShadowDecision } from "./reviewer-routing"; import type { OutcomeCalibration } from "./outcome-calibration"; @@ -167,10 +172,12 @@ function recapSectionLines(items: string[], fallback: string): string[] { export function formatMaintainerRecap(report: RecapReport, options: { configDrift?: DriftRecapSection; routingShadow?: { title: string; lines: string[] } } = {}): 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", "", @@ -191,6 +198,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 @@ -257,6 +272,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" }; @@ -271,7 +291,13 @@ export async function runMaintainerRecap( // #8229 stage 1: the routing-shadow section reads the window's recorded decisions straight from the // audit trail — fail-safe to an absent section (the recap must never break on a read blip). const routingShadow = await loadRoutingRecapSection(env, report.windowDays, options.generatedAt ?? nowIso()); - const formatted = formatMaintainerRecap(report, routingShadow ? { routingShadow } : {}); + // Built up key-by-key rather than passed as a conditional-spread literal: exactOptionalPropertyTypes + // forbids handing either key an explicit `undefined`, and #8229's routingShadow and #8372's configDrift + // are independent — each is present or absent on its own, so a single ternary can't express all four cases. + const recapOptions: { routingShadow?: { title: string; lines: string[] }; configDrift?: DriftRecapSection } = {}; + if (routingShadow) recapOptions.routingShadow = routingShadow; + if (options.configDrift) recapOptions.configDrift = options.configDrift; + const formatted = formatMaintainerRecap(report, recapOptions); 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")] }); From 63712fb4d6a3d2002e1c38f991a331efcb7dd11b Mon Sep 17 00:00:00 2001 From: RealDiligent Date: Sat, 25 Jul 2026 00:10:20 +0800 Subject: [PATCH 2/2] test(recap): cover both routing-shadow arms after the options merge runMaintainerRecap now assembles formatMaintainerRecap's options key-by-key, so the routingShadow present/absent arms are both changed lines. #8229 shipped that path with no test producing a decision, and none covering the fail-safe null return. Co-Authored-By: Claude Opus 4.8 --- test/unit/maintainer-recap.test.ts | 53 ++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/test/unit/maintainer-recap.test.ts b/test/unit/maintainer-recap.test.ts index 4f46d1cec2..ef64700003 100644 --- a/test/unit/maintainer-recap.test.ts +++ b/test/unit/maintainer-recap.test.ts @@ -246,6 +246,59 @@ describe("runMaintainerRecap (#2252 end-to-end orchestration)", () => { expect(calls.some((c) => c.body.includes("## Config drift"))).toBe(true); }); + // #8372: runMaintainerRecap now assembles its formatter options key-by-key, so the routingShadow-present + // arm needs a real recorded decision. #8229 shipped that path with no test that produced one. + it("includes the #8229 routing-shadow section when the window has recorded decisions (routingShadow present arm)", async () => { + stubRecapChannelFetch(); + const env = envWithBothWebhooks(); + await env.DB.prepare( + "INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + ) + .bind( + "ae-routing-1", + "reviewer_routing_shadow", + "loopover", + "acme/widgets#1", + "completed", + "shadow", + JSON.stringify({ repoFullName: "acme/widgets", preferredProvider: "claude-code", basis: ["evidence"] }), + GEN, // pinned to the same instant as generatedAt below, so the since-filter keeps it deterministically + ) + .run(); + const result = await runMaintainerRecap(env, { generatedAt: GEN }); + expect(result.skipped).toBe(false); + if (result.skipped) return; + expect(result.formatted).toContain("Reviewer routing shadow"); + }); + + // The absent arm: loadRoutingRecapSection is fail-safe (returns null on any read error), so a routing + // read blip must leave the digest intact minus that one section rather than breaking the whole recap. + it("omits the routing-shadow section when its audit read fails (routingShadow absent arm)", async () => { + stubRecapChannelFetch(); + const base = envWithBothWebhooks(); + const env = new Proxy(base, { + get(target, prop, receiver) { + if (prop !== "DB") return Reflect.get(target, prop, receiver); + return new Proxy(target.DB, { + get(dbTarget, dbProp, dbReceiver) { + if (dbProp !== "prepare") return Reflect.get(dbTarget, dbProp, dbReceiver); + return (sql: string) => { + if (sql.includes("SELECT metadata_json FROM audit_events")) throw new Error("routing_read_blip"); + return dbTarget.prepare(sql); + }; + }, + }); + }, + }) as Env; + + const result = await runMaintainerRecap(env, { generatedAt: GEN }); + expect(result.skipped).toBe(false); + if (result.skipped) return; + expect(result.formatted).not.toContain("Reviewer routing shadow"); + // The rest of the digest is unaffected — the failure costs one section, not the recap. + expect(result.formatted).toContain("## Totals"); + }); + 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")] });