feat(review): add finding acceptance rate calculation and related types (#1967) (#5127)

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.

Author: carlh7777

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<FindingAcceptanceAggregate> {
+  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,
   };
 }
 

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<string, unknown> = {}): 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<string, unknown> = {}): 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",