Skip to content

Commit fe816af

Browse files
kai392RealDiligentclaude
authored
fix(notifications): wire the calibration/gate-outcomes/per-repo builders into the recap digest (#8511)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: RealDiligent <brave.challenge007@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 048aeb7 commit fe816af

3 files changed

Lines changed: 109 additions & 9 deletions

File tree

src/services/maintainer-recap.ts

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@ import { PUBLIC_LOCAL_PATH_SCRUB_PATTERN, PUBLIC_UNSAFE_PATTERN } from "../signa
1414
import { deliverRecapToDiscord, deliverRecapToSlack } from "./notify-discord";
1515
import type { GatePrecisionReport } from "./gate-precision";
1616
import type { DriftRecapSection } from "./maintainer-recap-drift";
17+
// #8372: these three section builders shipped fully implemented + unit-tested but were never composed into
18+
// the delivered digest -- the same "built, tested, never called from production" shape as #6636.
19+
import { buildCalibrationRecapSection } from "./maintainer-recap-calibration";
20+
import { buildGateOutcomesRecapSection } from "./maintainer-recap-gate-outcomes";
21+
import { buildPerRepoRecapSection } from "./maintainer-recap-per-repo";
1722
import { buildRoutingRecapSection } from "./maintainer-recap-routing";
1823
import { REVIEWER_ROUTING_SHADOW_EVENT_TYPE, type RoutingShadowDecision } from "./reviewer-routing";
1924
import type { OutcomeCalibration } from "./outcome-calibration";
@@ -167,10 +172,12 @@ function recapSectionLines(items: string[], fallback: string): string[] {
167172
export function formatMaintainerRecap(report: RecapReport, options: { configDrift?: DriftRecapSection; routingShadow?: { title: string; lines: string[] } } = {}): string {
168173
const { totals } = report;
169174
const rate = totals.gateFalsePositiveRate !== null ? `${Math.round(totals.gateFalsePositiveRate * 100)}%` : "n/a";
170-
const perRepoLines = report.repos.map(
171-
(repo) =>
172-
`${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)`,
173-
);
175+
// #8372: the dedicated builder replaces the inline map that duplicated it -- unlike this file's old copy,
176+
// it sorts, caps the list, and emits a "(+N more)" remainder line.
177+
const perRepoSection = buildPerRepoRecapSection({ windowDays: report.windowDays, repos: report.repos });
178+
const perRepoLines = perRepoSection.lines.map(redactRecapLine);
179+
const calibrationSection = buildCalibrationRecapSection({ windowDays: report.windowDays, totals: report.totals });
180+
const gateOutcomesSection = buildGateOutcomesRecapSection({ windowDays: report.windowDays, totals: report.totals });
174181
const lines = [
175182
"# Maintainer recap",
176183
"",
@@ -191,6 +198,14 @@ export function formatMaintainerRecap(report: RecapReport, options: { configDrif
191198
"",
192199
"## Per-repo",
193200
...recapSectionLines(perRepoLines, "_No repositories in this window._"),
201+
"",
202+
// #8372: unconditional (not behind an options flag) -- both sections read only report.totals/windowDays,
203+
// which every RecapReport always carries, so there is nothing for a caller to opt into.
204+
`## ${redactRecapLine(calibrationSection.title)}`,
205+
...recapSectionLines(calibrationSection.lines.map(redactRecapLine), "_No calibration lines for this window._"),
206+
"",
207+
`## ${redactRecapLine(gateOutcomesSection.title)}`,
208+
...recapSectionLines(gateOutcomesSection.lines.map(redactRecapLine), "_No gate-outcome lines for this window._"),
194209
// #8214: optional config-drift section (maintainer-recap-drift.ts) — appended only when the caller has a
195210
// sentinel projection to render, so every existing digest stays byte-identical until the sentinel wires in.
196211
...(options.configDrift
@@ -257,6 +272,11 @@ export async function runMaintainerRecap(
257272
report?: RecapReport;
258273
/** When explicitly false, short-circuits before build/format/delivery. Default: run. */
259274
enabled?: boolean;
275+
/** #8372: forwarded to {@link formatMaintainerRecap} so a caller holding a drift projection can have it
276+
* rendered. Deliberately NOT sourced here -- reading the knob-loosening sentinel state is its own
277+
* data-sourcing concern; this is only the plumbing, so the section stays absent until a caller passes it
278+
* and every existing digest is unaffected. */
279+
configDrift?: DriftRecapSection;
260280
} = {},
261281
): Promise<RunMaintainerRecapResult> {
262282
if (options.enabled === false) return { skipped: true, reason: "disabled" };
@@ -271,7 +291,13 @@ export async function runMaintainerRecap(
271291
// #8229 stage 1: the routing-shadow section reads the window's recorded decisions straight from the
272292
// audit trail — fail-safe to an absent section (the recap must never break on a read blip).
273293
const routingShadow = await loadRoutingRecapSection(env, report.windowDays, options.generatedAt ?? nowIso());
274-
const formatted = formatMaintainerRecap(report, routingShadow ? { routingShadow } : {});
294+
// Built up key-by-key rather than passed as a conditional-spread literal: exactOptionalPropertyTypes
295+
// forbids handing either key an explicit `undefined`, and #8229's routingShadow and #8372's configDrift
296+
// are independent — each is present or absent on its own, so a single ternary can't express all four cases.
297+
const recapOptions: { routingShadow?: { title: string; lines: string[] }; configDrift?: DriftRecapSection } = {};
298+
if (routingShadow) recapOptions.routingShadow = routingShadow;
299+
if (options.configDrift) recapOptions.configDrift = options.configDrift;
300+
const formatted = formatMaintainerRecap(report, recapOptions);
275301
const [discord, slack] = await Promise.all([
276302
deliverRecapToDiscord(env, report, formatted),
277303
deliverRecapToSlack(env, report, formatted),

test/unit/maintainer-recap-format.test.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,18 @@ describe("formatMaintainerRecap (#2240)", () => {
3333
expect(body).toContain("## Summary");
3434
expect(body).toContain("## Totals");
3535
expect(body).toContain("## Per-repo");
36+
// #8372: both builders read only totals/windowDays, so their sections are unconditional.
37+
expect(body).toContain("## Calibration");
38+
expect(body).toContain("## Gate outcomes");
3639
// #8214: without a sentinel projection the drift section is entirely absent — the digest stays
3740
// byte-identical to the pre-drift shape, not a dangling empty header.
3841
expect(body).not.toContain("## Config drift");
3942
// Empty sections show a single fallback line instead of dangling under the header.
4043
expect(body).toContain("_No summary lines for this window._");
41-
expect(body).toContain("_No repositories in this window._");
44+
// #8372: the ## Per-repo body now comes from buildPerRepoRecapSection, which emits its own
45+
// windowed empty-state line, so the section is never empty and the generic fallback never fires.
46+
expect(body).toContain("No repo activity in the last 7 day(s).");
47+
expect(body).not.toContain("_No repositories in this window._");
4248
// Null rate ⇒ the "n/a" arm.
4349
expect(body).toContain("- Gate false positives: 0/0 (n/a)");
4450
expect(body).toContain("- Repos: 0");
@@ -98,8 +104,10 @@ describe("formatMaintainerRecap (#2240)", () => {
98104
// Numeric / non-null rate arm.
99105
expect(body).toContain("- Gate false positives: 1/4 (25%)");
100106
expect(body).toContain("- Repos: 1");
101-
// Per-repo row rendered (non-empty section arm).
102-
expect(body).toContain("acme/widgets — 5 reviewed, 3 merged, 2 closed, 1 gate false-positive(s), 1 override(s), 0 reversal(s)");
107+
// Per-repo row rendered (non-empty section arm), now in buildPerRepoRecapSection's row format (#8372).
108+
// The gate/override/reversal counts this row used to carry are unchanged in ## Totals above, and are
109+
// broken out per-dimension by the ## Gate outcomes section this digest now composes.
110+
expect(body).toContain("acme/widgets: reviewed 5, merged 3, closed 2");
103111
// Clean summary line survives verbatim (redaction no-op arm).
104112
expect(body).toContain("- Normal recap line about resolved reviews.");
105113
// Arm 1: local path scrubbed to the placeholder, raw path gone.

test/unit/maintainer-recap.test.ts

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { afterEach, describe, expect, it, vi } from "vitest";
22
import { buildMaintainerRecap, runMaintainerRecap, type MaintainerRecapRepoInput } from "../../src/services/maintainer-recap";
3+
import { buildDriftRecapSection } from "../../src/services/maintainer-recap-drift";
34
import type { OutcomeCalibration } from "../../src/services/outcome-calibration";
45
import type { RecapReport } from "../../src/types";
56
import { createTestEnv } from "../helpers/d1";
@@ -229,10 +230,75 @@ describe("runMaintainerRecap (#2252 end-to-end orchestration)", () => {
229230
expect(result.skipped).toBe(false);
230231
if (result.skipped) return;
231232
expect(result.report.repos).toEqual([]);
232-
expect(result.formatted).toContain("_No repositories in this window._");
233+
// #8372: the ## Per-repo body is buildPerRepoRecapSection's, which carries its own empty-state line.
234+
expect(result.formatted).toContain("No repo activity in the last 7 day(s).");
233235
expect(result.formatted).toContain("(n/a)");
234236
});
235237

238+
it("forwards a caller-supplied configDrift projection into the delivered digest (#8372 present arm)", async () => {
239+
const calls = stubRecapChannelFetch();
240+
const configDrift = buildDriftRecapSection({ generatedAt: GEN, sentinelEnabled: false, drifting: [], cleanKnobs: 0 });
241+
const result = await runMaintainerRecap(envWithBothWebhooks(), { configDrift });
242+
expect(result.skipped).toBe(false);
243+
if (result.skipped) return;
244+
expect(result.formatted).toContain("## Config drift");
245+
// Reaches the actual delivered payload, not just the returned string.
246+
expect(calls.some((c) => c.body.includes("## Config drift"))).toBe(true);
247+
});
248+
249+
// #8372: runMaintainerRecap now assembles its formatter options key-by-key, so the routingShadow-present
250+
// arm needs a real recorded decision. #8229 shipped that path with no test that produced one.
251+
it("includes the #8229 routing-shadow section when the window has recorded decisions (routingShadow present arm)", async () => {
252+
stubRecapChannelFetch();
253+
const env = envWithBothWebhooks();
254+
await env.DB.prepare(
255+
"INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)",
256+
)
257+
.bind(
258+
"ae-routing-1",
259+
"reviewer_routing_shadow",
260+
"loopover",
261+
"acme/widgets#1",
262+
"completed",
263+
"shadow",
264+
JSON.stringify({ repoFullName: "acme/widgets", preferredProvider: "claude-code", basis: ["evidence"] }),
265+
GEN, // pinned to the same instant as generatedAt below, so the since-filter keeps it deterministically
266+
)
267+
.run();
268+
const result = await runMaintainerRecap(env, { generatedAt: GEN });
269+
expect(result.skipped).toBe(false);
270+
if (result.skipped) return;
271+
expect(result.formatted).toContain("Reviewer routing shadow");
272+
});
273+
274+
// The absent arm: loadRoutingRecapSection is fail-safe (returns null on any read error), so a routing
275+
// read blip must leave the digest intact minus that one section rather than breaking the whole recap.
276+
it("omits the routing-shadow section when its audit read fails (routingShadow absent arm)", async () => {
277+
stubRecapChannelFetch();
278+
const base = envWithBothWebhooks();
279+
const env = new Proxy(base, {
280+
get(target, prop, receiver) {
281+
if (prop !== "DB") return Reflect.get(target, prop, receiver);
282+
return new Proxy(target.DB, {
283+
get(dbTarget, dbProp, dbReceiver) {
284+
if (dbProp !== "prepare") return Reflect.get(dbTarget, dbProp, dbReceiver);
285+
return (sql: string) => {
286+
if (sql.includes("SELECT metadata_json FROM audit_events")) throw new Error("routing_read_blip");
287+
return dbTarget.prepare(sql);
288+
};
289+
},
290+
});
291+
},
292+
}) as Env;
293+
294+
const result = await runMaintainerRecap(env, { generatedAt: GEN });
295+
expect(result.skipped).toBe(false);
296+
if (result.skipped) return;
297+
expect(result.formatted).not.toContain("Reviewer routing shadow");
298+
// The rest of the digest is unaffected — the failure costs one section, not the recap.
299+
expect(result.formatted).toContain("## Totals");
300+
});
301+
236302
it("short-circuits when enabled is false — no build/format/fetch (flag-OFF arm)", async () => {
237303
const calls = stubRecapChannelFetch();
238304
const result = await runMaintainerRecap(envWithBothWebhooks(), { enabled: false, repos: [repoInput("owner/repo")] });

0 commit comments

Comments
 (0)