From 6e0b0bebd2623a095a26785a398f575372f47614 Mon Sep 17 00:00:00 2001 From: carlh171112 Date: Sat, 11 Jul 2026 14:02:44 -0700 Subject: [PATCH] feat(review): add finding acceptance rate calculation and related types (#1967) Introduces the FindingAcceptanceAggregate interface to track the acceptance rate of PRs flagged with blocking findings (hold/close). Implements aggregateFindingAcceptance function to compute the acceptance metrics from flagged PR outcomes. Updates computeStats to include finding acceptance data and adds corresponding tests to validate the new functionality. --- src/review/stats.ts | 84 ++++++++++++++++++++++++++++- test/unit/stats.test.ts | 113 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 192 insertions(+), 5 deletions(-) diff --git a/src/review/stats.ts b/src/review/stats.ts index f10955aa1..cf37e67e1 100644 --- a/src/review/stats.ts +++ b/src/review/stats.ts @@ -195,6 +195,28 @@ export const EMPTY_CYCLE_TIME: CycleTimeAggregate = { sampleSize: 0, }; +/** Finding acceptance rate (#1967): of PRs whose gate raised a BLOCKING finding (a `gate_decision` of `hold` + * or `close` — the gate did NOT clear the PR to merge), how often the contributor acted on it and the PR + * shipped anyway. The realized `pr_outcome` (merged vs closed) is the answer key, so acceptance is measurable + * with zero manual labeling — the maintainer-ROI "our review feedback gets acted on X% of the time" signal. */ +export interface FindingAcceptanceAggregate { + /** PRs whose gate raised a blocking finding (hold|close) that reached a realized outcome in the window. */ + flagged: number; + /** Of `flagged`, those later merged — the contributor acted on the finding and the PR shipped. */ + addressed: number; + /** Of `flagged`, those closed without merging — the finding stood; the PR did not ship. */ + unaddressed: number; + /** addressed / flagged (3 dp); null when `flagged` is 0. */ + acceptanceRate: number | null; +} + +export const EMPTY_FINDING_ACCEPTANCE: FindingAcceptanceAggregate = { + flagged: 0, + addressed: 0, + unaddressed: 0, + acceptanceRate: null, +}; + export interface StatsPayload { generatedAt: string; window: { fromIso: string; days: number; bucket: string }; @@ -216,6 +238,9 @@ export interface StatsPayload { gateParity: GateParityReport & { cutoverReady: Array<{ project: string; ready: boolean }> }; /** PR review cycle-time percentiles (gate decision → outcome) from review_audit (#2194). */ cycleTime: CycleTimeAggregate; + /** Finding acceptance rate (#1967): of PRs the gate flagged with a blocking finding (hold|close), the + * fraction later merged — i.e. the contributor acted on the finding and the PR shipped. */ + findingAcceptance: FindingAcceptanceAggregate; } /** ms between the gate decision and the resolution; null if implausible (NaN or negative). */ @@ -296,6 +321,61 @@ export async function computeCycleTimeAggregate( } } +/** Pure fold: flagged-PR outcome samples → the acceptance aggregate (#1967). Each sample is a PR whose gate + * raised a blocking finding (hold|close); `merged` is its realized pr_outcome (true = merged, false = closed). */ +export function aggregateFindingAcceptance( + samples: ReadonlyArray<{ merged: boolean }>, +): FindingAcceptanceAggregate { + const flagged = samples.length; + if (flagged === 0) return EMPTY_FINDING_ACCEPTANCE; + const addressed = samples.filter((sample) => sample.merged).length; + return { + flagged, + addressed, + unaddressed: flagged - addressed, + acceptanceRate: Number((addressed / flagged).toFixed(3)), + }; +} + +// A PR is "flagged" if it EVER received a hold/close gate decision in the window (DISTINCT target_id), so a +// PR that was held, fixed, then merge-cleared still counts as flagged-then-addressed. Joined to the LATEST +// pr_outcome per target (rn = 1) so a reopened+reclosed PR keeps its final realized state. +const FINDING_ACCEPTANCE_SQL = `WITH flagged AS ( + SELECT DISTINCT target_id + FROM review_audit + WHERE event_type = 'gate_decision' AND decision IN ('hold', 'close') AND created_at >= ? +), +po AS ( + SELECT target_id, decision AS truth, + ROW_NUMBER() OVER (PARTITION BY target_id ORDER BY created_at DESC) AS rn + FROM review_audit + WHERE event_type = 'pr_outcome' AND decision IS NOT NULL AND created_at >= ? +) +SELECT po.truth AS truth +FROM flagged +JOIN po ON flagged.target_id = po.target_id +WHERE po.rn = 1`; + +/** Load flagged-PR (blocking gate finding) → realized-outcome samples for the window and fold them into the + * acceptance rate. Fail-safe → empty aggregate (mirrors computeCycleTimeAggregate). (#1967) */ +export async function computeFindingAcceptance( + env: Env, + opts: { days: number; nowMs: number }, +): Promise { + const days = Number.isFinite(opts.days) && opts.days > 0 ? Math.min(opts.days, 730) : 90; + const fromIso = new Date(opts.nowMs - days * 86_400_000).toISOString().slice(0, 10); + try { + const rows = await storage(env) + .prepare(FINDING_ACCEPTANCE_SQL) + .bind(fromIso, fromIso) + .all<{ truth: string }>(); + const samples = (rows.results ?? []).map((row) => ({ merged: row.truth === "merged" })); + return aggregateFindingAcceptance(samples); + } catch { + return EMPTY_FINDING_ACCEPTANCE; + } +} + /** Fold per-PR persisted minutes into the maintainer aggregate (avg band + total minutes). */ export function aggregateReviewEffort(perPrMinutes: number[]): ReviewEffortAggregate { if (perPrMinutes.length === 0) { @@ -324,7 +404,7 @@ export async function computeStats( const bucketExpr = BUCKET_SQL[bucket] ?? BUCKET_SQL.day; const fromIso = new Date(opts.nowMs - days * 86_400_000).toISOString().slice(0, 10); // YYYY-MM-DD - const [decisionRows, reversalRows, effortRows, cycleTime] = await Promise.all([ + const [decisionRows, reversalRows, effortRows, cycleTime, findingAcceptance] = await Promise.all([ storage(env).prepare( `SELECT ${bucketExpr} AS bucket, project, COALESCE(verdict, status) AS verdict, COUNT(*) AS n FROM review_targets @@ -359,6 +439,7 @@ export async function computeStats( ).bind(fromIso).all<{ minutes: number }>() .catch(() => ({ results: [] as Array<{ minutes: number }> })), computeCycleTimeAggregate(env, { days, nowMs: opts.nowMs }), + computeFindingAcceptance(env, { days, nowMs: opts.nowMs }), ]); // Non-content gate decisions (incl. SHADOW would-actions) — recorded as `gate_decision` audit rows with @@ -396,6 +477,7 @@ export async function computeStats( recommendations, gateParity: { ...parity, cutoverReady: parity.rows.map((r) => ({ project: r.project, ready: isParityCutoverReady(r) })) }, cycleTime, + findingAcceptance, }; } diff --git a/test/unit/stats.test.ts b/test/unit/stats.test.ts index 5007f507c..9501e482e 100644 --- a/test/unit/stats.test.ts +++ b/test/unit/stats.test.ts @@ -2,11 +2,14 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createTestEnv } from "../helpers/d1"; import { aggregateCycleTimePercentiles, + aggregateFindingAcceptance, aggregateReviewEffort, buildCycleTimeDistribution, + computeFindingAcceptance, computeStats, cycleTimeMs, EMPTY_CYCLE_TIME, + EMPTY_FINDING_ACCEPTANCE, handleParity, handleStats, isParityCutoverReady, @@ -37,6 +40,8 @@ function stubEnv(extra: Record = {}): Env { { decided_at: "2026-06-01T10:00:00Z", outcome_at: "2026-06-01T10:05:00Z" }, { decided_at: "2026-06-01T11:00:00Z", outcome_at: "2026-06-01T11:30:00Z" }, ]; + // flagged-PR (hold|close) realized outcomes → 2 merged (addressed) + 1 closed (unaddressed). + const acceptanceRows = [{ truth: "merged" }, { truth: "merged" }, { truth: "closed" }]; let lastSql = ""; return { ...extra, @@ -47,16 +52,18 @@ function stubEnv(extra: Record = {}): Env { bind: () => ({ all: async () => ({ // review-effort read → effortMinutes; cycle-time pairs → cyclePairs; gate action counts → gateActions; - // other review_audit → reversals; everything else → decision rows. + // finding-acceptance read → acceptanceRows; other review_audit → reversals; everything else → decisions. results: lastSql.includes("reviewEffortMinutes") ? effortMinutes : lastSql.includes("decided_at") && lastSql.includes("outcome_at") ? cyclePairs : lastSql.includes("decision AS action") ? gateActions - : lastSql.includes("review_audit") - ? reversals - : decisions, + : lastSql.includes("flagged") + ? acceptanceRows + : lastSql.includes("review_audit") + ? reversals + : decisions, }), }), }; @@ -86,6 +93,7 @@ describe("computeStats — D1 aggregate for the dashboard", () => { expect(out.cycleTime.sampleSize).toBe(2); expect(out.cycleTime.p50Ms).toBe(300_000); expect(out.cycleTime.distribution.length).toBeGreaterThan(0); + expect(out.findingAcceptance).toEqual({ flagged: 3, addressed: 2, unaddressed: 1, acceptanceRate: 0.667 }); }); it("clamps an absurd window and falls back to a safe bucket", async () => { @@ -423,6 +431,7 @@ describe("computeStats — NaN window + null D1 results (the ?? [] fallbacks)", expect(out.verdicts).toEqual([]); expect(out.reviewEffort).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); expect(out.cycleTime).toEqual(EMPTY_CYCLE_TIME); + expect(out.findingAcceptance).toEqual(EMPTY_FINDING_ACCEPTANCE); }); }); @@ -561,6 +570,102 @@ describe("cycle-time aggregation (#2194)", () => { }); }); +describe("finding acceptance rate (#1967)", () => { + it("aggregateFindingAcceptance folds flagged-PR outcomes into the acceptance rate", () => { + const agg = aggregateFindingAcceptance([{ merged: true }, { merged: true }, { merged: false }]); + expect(agg).toEqual({ flagged: 3, addressed: 2, unaddressed: 1, acceptanceRate: 0.667 }); + }); + + it("aggregateFindingAcceptance reports 1 / 0 acceptance at the extremes (both filter branches)", () => { + expect(aggregateFindingAcceptance([{ merged: true }, { merged: true }])).toEqual({ + flagged: 2, + addressed: 2, + unaddressed: 0, + acceptanceRate: 1, + }); + expect(aggregateFindingAcceptance([{ merged: false }, { merged: false }])).toEqual({ + flagged: 2, + addressed: 0, + unaddressed: 2, + acceptanceRate: 0, + }); + }); + + it("aggregateFindingAcceptance returns EMPTY_FINDING_ACCEPTANCE for no samples", () => { + expect(aggregateFindingAcceptance([])).toEqual(EMPTY_FINDING_ACCEPTANCE); + }); + + it("computeFindingAcceptance joins EVER-flagged (hold|close) PRs to their latest outcome from real D1", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO review_audit (id, project, target_id, event_type, decision, source, created_at) VALUES + ('gd1', 'owner/repo', 'owner/repo#1', 'gate_decision', 'close', 'test', '2026-06-10T10:00:00Z'), + ('po1', 'owner/repo', 'owner/repo#1', 'pr_outcome', 'merged', 'test', '2026-06-10T12:00:00Z'), + ('gd2', 'owner/repo', 'owner/repo#2', 'gate_decision', 'hold', 'test', '2026-06-11T10:00:00Z'), + ('po2', 'owner/repo', 'owner/repo#2', 'pr_outcome', 'closed', 'test', '2026-06-11T12:00:00Z'), + ('gd3a', 'owner/repo', 'owner/repo#3', 'gate_decision', 'hold', 'test', '2026-06-12T09:00:00Z'), + ('gd3b', 'owner/repo', 'owner/repo#3', 'gate_decision', 'hold', 'test', '2026-06-12T10:00:00Z'), + ('po3a', 'owner/repo', 'owner/repo#3', 'pr_outcome', 'closed', 'test', '2026-06-12T11:00:00Z'), + ('po3b', 'owner/repo', 'owner/repo#3', 'pr_outcome', 'merged', 'test', '2026-06-12T12:00:00Z'), + ('gd4', 'owner/repo', 'owner/repo#4', 'gate_decision', 'merge', 'test', '2026-06-13T10:00:00Z'), + ('po4', 'owner/repo', 'owner/repo#4', 'pr_outcome', 'merged', 'test', '2026-06-13T12:00:00Z'), + ('gd5', 'owner/repo', 'owner/repo#5', 'gate_decision', 'close', 'test', '2026-06-13T10:00:00Z')`, + ).run(); + const agg = await computeFindingAcceptance(env, { days: 90, nowMs: NOW }); + // #1 close→merged (addressed) and #3 hold→(closed then MERGED, rn=1 latest) (addressed); #2 hold→closed + // (unaddressed); #4 merge→merged is a CLEAN merge (not flagged); #5 close has no outcome yet (not counted). + expect(agg).toEqual({ flagged: 3, addressed: 2, unaddressed: 1, acceptanceRate: 0.667 }); + }); + + it("computeFindingAcceptance fails safe to EMPTY_FINDING_ACCEPTANCE when the query rejects", async () => { + const env = { + DB: { + prepare: () => ({ + bind: () => ({ + all: async () => { + throw new Error("d1 down"); + }, + }), + }), + }, + } as unknown as Env; + expect(await computeFindingAcceptance(env, { days: 30, nowMs: NOW })).toEqual(EMPTY_FINDING_ACCEPTANCE); + }); + + it("computeFindingAcceptance tolerates missing D1 results (the ?? [] fallback)", async () => { + const env = { + DB: { + prepare: () => ({ + bind: () => ({ + all: async () => ({ results: undefined }), + }), + }), + }, + } as unknown as Env; + expect(await computeFindingAcceptance(env, { days: 30, nowMs: NOW })).toEqual(EMPTY_FINDING_ACCEPTANCE); + }); + + it("computeFindingAcceptance defaults a non-finite / non-positive window to 90 days and clamps to 730", async () => { + let boundFrom: string | undefined; + const env = { + DB: { + prepare: () => ({ + bind: (fromIso: string) => { + boundFrom = fromIso; + return { all: async () => ({ results: [] as Array<{ truth: string }> }) }; + }, + }), + }, + } as unknown as Env; + await computeFindingAcceptance(env, { days: Number.NaN, nowMs: NOW }); + expect(boundFrom).toBe(new Date(NOW - 90 * 86_400_000).toISOString().slice(0, 10)); + await computeFindingAcceptance(env, { days: 0, nowMs: NOW }); + expect(boundFrom).toBe(new Date(NOW - 90 * 86_400_000).toISOString().slice(0, 10)); + await computeFindingAcceptance(env, { days: 99_999, nowMs: NOW }); + expect(boundFrom).toBe(new Date(NOW - 730 * 86_400_000).toISOString().slice(0, 10)); + }); +}); + describe("isParityCutoverReady — every gate condition", () => { const base: GateParityRow = { project: "p",