Skip to content

feat(review-stats): add finding acceptance-rate signal to the maintainer stats feed (#1967) #5127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 12, 2026
Merged

feat(review-stats): add finding acceptance-rate signal to the maintainer stats feed (#1967) #5127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
84 changes: 83 additions & 1 deletion src/review/stats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand All @@ -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). */
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -396,6 +477,7 @@ export async function computeStats(
recommendations,
gateParity: { ...parity, cutoverReady: parity.rows.map((r) => ({ project: r.project, ready: isParityCutoverReady(r) })) },
cycleTime,
findingAcceptance,
};
}

Expand Down
113 changes: 109 additions & 4 deletions test/unit/stats.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
}),
}),
};
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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);
});
});

Expand Down Expand Up @@ -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",
Expand Down