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
36 changes: 31 additions & 5 deletions src/services/maintainer-recap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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",
"",
Expand All @@ -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
Expand Down Expand Up @@ -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<RunMaintainerRecapResult> {
if (options.enabled === false) return { skipped: true, reason: "disabled" };
Expand All @@ -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),
Expand Down
14 changes: 11 additions & 3 deletions test/unit/maintainer-recap-format.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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.
Expand Down
68 changes: 67 additions & 1 deletion test/unit/maintainer-recap.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -229,10 +230,75 @@ 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);
});

// #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")] });
Expand Down