Skip to content

fix(stats): measure accuracy over auto-actioned PRs, not every reviewed PR #9793

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 29, 2026
Merged

fix(stats): measure accuracy over auto-actioned PRs, not every reviewed PR #9793

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
91 changes: 39 additions & 52 deletions src/review/public-stats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,47 +165,19 @@ function filteredPct(reviewed: number, merged: number): number | null {
return Math.round(((reviewed - merged) / reviewed) * 1000) / 10;
}

/** The auto-actions a reversal is recorded against. `recordReversalSignals` only ever fires for a PR this
* deployment itself auto-merged or auto-closed, so with zero of these rows in the retained window a
* `reversal_*` event cannot exist — and `1 - 0/N` is then a structural zero, not a measurement. */
export const AUTO_ACTION_EVENT_TYPES = ["agent.action.close", "agent.action.merge"] as const;

/**
* Whether a reversal is OBSERVABLE on this deployment: did it record any terminal auto-action in the window a
* reversal could still be attributed to? Reversal signals are written only by the review-execution pipeline
* (`recordReversalSignals`, reached via the `github-webhook` job), which a runtime that does not execute
* reviews acks-and-drops — so on such a runtime `reversed` is pinned at 0 forever while the merged/closed
* denominator keeps growing, and every reversal-grounded percentage converges on a fake 100%.
/** Reversal-grounded accuracy over the AUTO-ACTIONED merged + closed; null until there is signal.
*
* Publishing that as accuracy is the exact failure #8820 called out for the fleet number ("the old formula
* overstated accuracy") and #7449 fixed for the global one. This is the same discipline the rest of this
* surface already applies — a sparse rule's precision is null, `gamingFlagsCaught` is null below three
* instances — extended to the case where the numerator's WRITER, not just its sample, is absent.
*/
export async function loadReversalObservability(env: Env, knownReversals = 0): Promise<boolean> {
// A recorded reversal is itself proof the pipeline that records them runs here -- no probe needed, and no
// query on the hot path for any deployment that has ever overturned an auto-action.
if (knownReversals > 0) return true;
const rows = await safeAll<{ n: number }>(
env,
`SELECT COUNT(*) AS n FROM audit_events
WHERE event_type IN (${AUTO_ACTION_EVENT_TYPES.map((type) => `'${type}'`).join(", ")})
AND created_at >= ?`,
retentionCutoffIsoForTable("audit_events"),
);
return (rows[0]?.n ?? 0) > 0;
}

/** Reversal-grounded accuracy over the irreversible auto-actions (merged + closed); null until there is signal,
* and null when a reversal is not observable at all on this deployment (see {@link loadReversalObservability}) —
* an unmeasurable quantity stays unknown rather than rendering as a perfect score. */
* `merged`/`closed` must come from the auto-action denominator (#9792), never the published-surface counts:
* a reversal only ever exists for a PR the engine itself merged or closed, so a wider denominator counts PRs
* whose reversal was never possible and drifts the ratio toward 100%. That also makes the separate
* observability probe #9718 added redundant and it has been removed — no auto-actions means `decided` is 0
* and the answer is null by construction, which is a structural guarantee rather than a heuristic that
* could disagree with the denominator beside it. */
function accuracyPct(
merged: number,
closed: number,
reversed: number,
reversalObservable: boolean,
): number | null {
if (!reversalObservable) return null;
const decided = merged + closed;
if (decided <= 0) return null;
// `reversed` counts engine auto-actions regardless of a PR's CURRENT disposition, so a reopened
Expand Down Expand Up @@ -378,22 +350,39 @@ export async function getPublicStats(
GROUP BY ev.repo`,
...projects,
),
// The ACCURACY denominator, bounded to the same window its numerator can survive in. `reviewed`/
// `merged`/`closed` above are lifetime: `github_app.pr_public_surface_published` is the single
// retention-EXEMPT audit event type (src/db/retention.ts's DURABLE_AUDIT_EVENT_TYPES), so those
// counts never shrink. `reversal_*` rows are pruned with the rest of audit_events at 90 days. Pairing
// an immortal denominator with a 90-day numerator makes `1 - reversed/decided` drift toward 100% as
// the ledger ages, independent of real reversal behavior -- the same shape #7449 fixed for the
// Orb-folded denominator, and confirmed live (2377/602/508 reviewed, 0 reversals, 100% each).
// The ACCURACY denominator: the PRs this deployment AUTO-ACTIONED in the window, not the PRs it
// published a surface for.
//
// A reversal is by definition a human overturning an engine auto-action -- outcomes-wire.ts only records
// one against a PR the engine merged or closed. So the population the numerator can draw from is
// auto-actions, and any denominator wider than that counts PRs whose reversal was never possible.
// `github_app.pr_public_surface_published` is wider in two independent ways: it is the single
// retention-EXEMPT event type (retention.ts's DURABLE_AUDIT_EVENT_TYPES) so it never shrinks, and it
// covers every reviewed PR including the ones the engine only commented on. #9768 fixed the first by
// windowing it; this fixes the second, which was why the published figure was STILL 100% for all three
// repos on 2377/602/508 reviewed with 0 reversals after that shipped.
//
// This is the pairing public-accuracy-trend.ts's loadReversalDayRows already uses for the weekly series
// -- it anchors reversals on `agent.action.*` rows, dry-runs excluded -- so the two surfaces now agree on
// what "decided" means instead of each choosing its own denominator.
safeAll<{ project: string; merged: number; closed: number }>(
env,
`SELECT ev.repo AS project,
`SELECT act.project AS project,
SUM(CASE WHEN pr.merged_at IS NOT NULL THEN 1 ELSE 0 END) AS merged,
SUM(CASE WHEN pr.state = 'closed' AND pr.merged_at IS NULL THEN 1 ELSE 0 END) AS closed
FROM (SELECT DISTINCT repo, number FROM (${PUBLISHED_PR_KEYS}) WHERE created_at >= ?) ev
LEFT JOIN pull_requests pr ON pr.repo_full_name = ev.repo AND pr.number = ev.number
WHERE LOWER(ev.repo) IN (${inList})
GROUP BY ev.repo`,
FROM (
SELECT DISTINCT substr(target_key, 1, instr(target_key, '#') - 1) AS project,
CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS pr_number
FROM audit_events
WHERE event_type IN ('agent.action.close', 'agent.action.merge')
AND outcome = 'completed' AND instr(target_key, '#') > 0
AND length(target_key) - length(replace(target_key, '#', '')) = 1
AND COALESCE(json_extract(metadata_json, '$.mode'), 'live') <> 'dry_run'
AND created_at >= ?
) act
LEFT JOIN pull_requests pr ON pr.repo_full_name = act.project AND pr.number = act.pr_number
WHERE LOWER(act.project) IN (${inList})
GROUP BY act.project`,
retentionCutoffIsoForTable("audit_events"),
...projects,
),
Expand Down Expand Up @@ -484,9 +473,7 @@ export async function getPublicStats(
};
// Resolved before byProject so every published accuracy figure -- per repo and global -- answers the same
// question about the same deployment, rather than one of them silently disagreeing with the others.
const totalReversals = [...reversedByProject.values()].reduce((sum, n) => sum + n, 0);
const reversalObservable = await loadReversalObservability(env, totalReversals);
// project -> the retention-windowed merged/closed pairing for `reversed`. See the query's own comment.
// project -> the auto-actioned merged/closed pairing for `reversed`. See the query's own comment.
const windowedByProject = new Map(windowedDispositions.map((row) => [String(row.project).toLowerCase(), row]));
let windowedMerged = 0;
let windowedClosed = 0;
Expand Down Expand Up @@ -514,7 +501,7 @@ export async function getPublicStats(
reviewed,
merged,
closed,
accuracyPct: accuracyPct(windowedRepoMerged, windowedRepoClosed, reversed, reversalObservable),
accuracyPct: accuracyPct(windowedRepoMerged, windowedRepoClosed, reversed),
};
})
.filter((r) => r.reviewed > 0)
Expand Down Expand Up @@ -643,7 +630,7 @@ export async function getPublicStats(
// byProject fold above), never the fleet-folded lifetime totals.merged/closed, so the numerator
// (own-ledger `reversed`) and the denominator are drawn from the same population AND the same
// retention window. See the windowed disposition query's own comment for both halves of that pairing.
accuracyPct: accuracyPct(windowedMerged, windowedClosed, totals.reversed, reversalObservable),
accuracyPct: accuracyPct(windowedMerged, windowedClosed, totals.reversed),
minutesSaved,
},
weekly: { reviewed: w.reviewed ?? 0, merged: w.merged ?? 0 },
Expand Down
86 changes: 55 additions & 31 deletions src/services/public-accuracy-trend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// #2568 pattern for the sibling per-repo quality trend) can recompute any historical week correctly on every
// request -- no cron-miss gap risk, no second copy of the number to keep in sync, and the SAME formula as the
// live figure by construction, so the two can never silently diverge or read as inconsistent to a public viewer.
import { loadReversalObservability, PUBLISHED_PR_KEYS, publicStatsProjects, safeAll } from "../review/public-stats";
import { PUBLISHED_PR_KEYS, publicStatsProjects, safeAll } from "../review/public-stats";
import { isoWeekStart } from "./public-quality-metrics";

export const PUBLIC_ACCURACY_TREND_WEEKS = 8;
Expand All @@ -24,13 +24,16 @@ export type PublicAccuracyTrendWeek = {
accuracyPct: number | null;
};

/** `merged`/`closed` are the DISPLAYED volume (own ledger + registered Orb fleet). `ownMerged`/`ownClosed` are
* the own-ledger-only pairing for `reversed`, which is own-ledger-only by construction (the Orb aggregate has
* no reversal concept). Accuracy divides the own-ledger numbers ONLY -- #7449 fixed exactly this asymmetry for
* the lifetime figure in public-stats.ts and it was never carried across to this trend, so the denominator grew
* with every newly registered install while the numerator stayed own-ledger-scoped, trending every week toward
* 100% independent of real reversal behavior. */
type DayRow = { day: string; merged: number; closed: number; ownMerged: number; ownClosed: number; reversed: number };
/** `merged`/`closed` are the DISPLAYED volume (own ledger + registered Orb fleet). `autoActioned` is the
* ACCURACY denominator: the PRs this deployment actually merged or closed itself that day.
*
* Two separate reasons it cannot be the displayed volume. #7449: the Orb aggregate has no reversal concept, so
* folding it in grows the denominator with every newly registered install while `reversed` stays own-ledger
* scoped. And #9792: `reversed` is only ever recorded against an engine AUTO-ACTION (loadReversalDayRows
* anchors on `agent.action.*`), so even the own-ledger published-PR count is wider than the population the
* numerator can draw from -- it includes every PR the engine merely commented on. Both make the ratio drift
* toward 100% for reasons that have nothing to do with reversal behavior. */
type DayRow = { day: string; merged: number; closed: number; autoActioned: number; reversed: number };

const MS_PER_WEEK = 7 * 86_400_000;

Expand All @@ -39,21 +42,17 @@ function roundPct(value: number): number {
}

/** Same formula as public-stats.ts's accuracyPct, reused so the trend and the live number can never drift
* apart into two competing definitions of "accuracy". `reversalObservable` false means this deployment records
* no terminal auto-actions for a reversal to be attributed to, so the week's accuracy is unknown, not perfect --
* the volume columns still publish, since those ARE measured. */
function publicBucketOf(
bucket: { merged: number; closed: number; ownMerged: number; ownClosed: number; reversed: number },
reversalObservable: boolean,
): Omit<PublicAccuracyTrendWeek, "weekStart"> {
* apart into two competing definitions of "accuracy". The volume columns still publish when accuracy cannot:
* those ARE measured. */
function publicBucketOf(bucket: { merged: number; closed: number; autoActioned: number; reversed: number }): Omit<PublicAccuracyTrendWeek, "weekStart"> {
const decided = bucket.merged + bucket.closed;
if (decided < MIN_ACCURACY_TREND_SAMPLE) return { merged: null, closed: null, reversed: null, accuracyPct: null };
const observed = { merged: bucket.merged, closed: bucket.closed, reversed: bucket.reversed };
if (!reversalObservable) return { ...observed, accuracyPct: null };
// Own-ledger-only denominator: `reversed` can only ever be attributed to own-ledger PRs (see DayRow).
const ownDecided = bucket.ownMerged + bucket.ownClosed;
if (ownDecided <= 0) return { ...observed, accuracyPct: null };
const reversalRate = Math.min(1, bucket.reversed / ownDecided);
// No auto-actions that week means no PR a reversal could have been recorded against, so accuracy is
// unknown rather than perfect. This replaces the separate observability probe (#9718): the denominator IS
// the signal now, and one structural guarantee beats a heuristic that could disagree with it.
if (bucket.autoActioned <= 0) return { ...observed, accuracyPct: null };
const reversalRate = Math.min(1, bucket.reversed / bucket.autoActioned);
return { ...observed, accuracyPct: roundPct(1 - reversalRate) };
}

Expand All @@ -63,11 +62,10 @@ export function buildPublicAccuracyTrend(
dayRows: DayRow[],
nowMs: number,
weeks: number = PUBLIC_ACCURACY_TREND_WEEKS,
reversalObservable = true,
): PublicAccuracyTrendWeek[] {
const currentStartMs = Date.parse(isoWeekStart(nowMs));
const oldestStartMs = currentStartMs - (weeks - 1) * MS_PER_WEEK;
const buckets = Array.from({ length: weeks }, () => ({ merged: 0, closed: 0, ownMerged: 0, ownClosed: 0, reversed: 0 }));
const buckets = Array.from({ length: weeks }, () => ({ merged: 0, closed: 0, autoActioned: 0, reversed: 0 }));

for (const row of dayRows) {
const dayMs = Date.parse(`${row.day}T00:00:00.000Z`);
Expand All @@ -77,14 +75,13 @@ export function buildPublicAccuracyTrend(
const bucket = buckets[weekOffset]!;
bucket.merged += row.merged;
bucket.closed += row.closed;
bucket.ownMerged += row.ownMerged;
bucket.ownClosed += row.ownClosed;
bucket.autoActioned += row.autoActioned;
bucket.reversed += row.reversed;
}

return buckets.map((bucket, offset) => ({
weekStart: isoWeekStart(oldestStartMs + offset * MS_PER_WEEK),
...publicBucketOf(bucket, reversalObservable),
...publicBucketOf(bucket),
}));
}

Expand Down Expand Up @@ -122,6 +119,34 @@ async function loadOwnLedgerDayRows(env: Env, projects: string[], sinceIso: stri
return map;
}

/** Distinct PRs this deployment AUTO-ACTIONED per day -- the accuracy denominator. Deliberately the same
* population, filters and dry-run exclusion as loadReversalDayRows' inner `orig` query below, because the
* numerator is drawn from exactly these rows; any divergence between the two would silently reintroduce a
* ratio over mismatched populations. */
async function loadAutoActionDayRows(env: Env, projects: string[], sinceIso: string): Promise<Map<string, number>> {
const map = new Map<string, number>();
if (projects.length === 0) return map;
const inList = projects.map(() => "?").join(", ");
const rows = await safeAll<{ day: string; n: number }>(
env,
`SELECT date(act.created_at) AS day, COUNT(DISTINCT act.target_key) AS n FROM (
SELECT substr(target_key, 1, instr(target_key, '#') - 1) AS project, target_key, created_at
FROM audit_events
WHERE event_type IN ('agent.action.close', 'agent.action.merge')
AND outcome = 'completed' AND instr(target_key, '#') > 0
AND length(target_key) - length(replace(target_key, '#', '')) = 1
AND COALESCE(json_extract(metadata_json, '$.mode'), 'live') <> 'dry_run'
AND created_at >= ?
) act
WHERE LOWER(act.project) IN (${inList})
GROUP BY day`,
sinceIso,
...projects,
);
for (const row of rows) map.set(row.day, row.n);
return map;
}

/** Day-bucketed reversal count, bucketed by the ORIGINAL auto-action's own created_at (not the later reversal's
* timestamp) so a reversal always credits the week the decision was actually made, and never retroactively
* shifts a past week's published trend. Detection matches public-stats.ts's `reversalRows` fix (#fairness-
Expand Down Expand Up @@ -192,22 +217,21 @@ export async function loadPublicAccuracyTrend(env: Env, nowMs: number = Date.now
const projects = publicStatsProjects(env);
const sinceIso = new Date(Date.parse(isoWeekStart(nowMs)) - (PUBLIC_ACCURACY_TREND_WEEKS - 1) * MS_PER_WEEK).toISOString();

const [ownLedger, reversals, orb, reversalObservable] = await Promise.all([
const [ownLedger, reversals, orb, autoActions] = await Promise.all([
loadOwnLedgerDayRows(env, projects, sinceIso),
loadReversalDayRows(env, projects, sinceIso),
loadOrbDayRows(env, sinceIso),
loadReversalObservability(env),
loadAutoActionDayRows(env, projects, sinceIso),
]);

const days = new Set([...ownLedger.keys(), ...reversals.keys(), ...orb.keys()]);
const days = new Set([...ownLedger.keys(), ...reversals.keys(), ...orb.keys(), ...autoActions.keys()]);
const dayRows: DayRow[] = [...days].map((day) => ({
day,
merged: (ownLedger.get(day)?.merged ?? 0) + (orb.get(day)?.merged ?? 0),
closed: (ownLedger.get(day)?.closed ?? 0) + (orb.get(day)?.closed ?? 0),
ownMerged: ownLedger.get(day)?.merged ?? 0,
ownClosed: ownLedger.get(day)?.closed ?? 0,
autoActioned: autoActions.get(day) ?? 0,
reversed: reversals.get(day) ?? 0,
}));

return buildPublicAccuracyTrend(dayRows, nowMs, PUBLIC_ACCURACY_TREND_WEEKS, reversalObservable);
return buildPublicAccuracyTrend(dayRows, nowMs);
}
10 changes: 10 additions & 0 deletions test/integration/public-stats-route.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@ async function seed(env: Env) {
)
.bind(`ae-${repo}-${number}`, `${repo}#${number}`)
.run();
// #9792: the accuracy denominator reads engine AUTO-ACTIONS, not published surfaces, so a decided PR
// needs the action that decided it. The still-open one (loopover#3) gets its own agent.action.close
// below, since it was auto-closed and then reopened -- that is what makes it a reversal.
if (state === "closed") {
await env.DB.prepare(
`INSERT INTO audit_events (id, event_type, target_key, outcome) VALUES (?, ?, ?, 'completed')`,
)
.bind(`act-${repo}-${number}`, mergedAt ? "agent.action.merge" : "agent.action.close", `${repo}#${number}`)
.run();
}
await env.DB.prepare(
`INSERT INTO pull_requests (id, repo_full_name, number, title, state, merged_at) VALUES (?, ?, ?, ?, ?, ?)`,
)
Expand Down
Loading