Skip to content

Commit 73e30da

Browse files
authored
fix(stats): withhold reversal-grounded accuracy where a reversal cannot be recorded (#9718)
The fairness page showed 100% accuracy for every repository and all eight weeks of the trend while the headline read 94.6%. Those 100%s are structural zeros, not measurements. `recordReversalSignals` has one call site, inside `handlePullRequestWebhookEvent`, reached only via the `github-webhook` job — a member of REVIEW_EXECUTION_JOB_TYPES, which a runtime that does not execute reviews acks-and-drops. So `reversed` is pinned at 0 while merged/closed keep growing, and `1 - 0/N` renders as a flawless score over a numerator that can never move. The repo already recorded the outcome: reversal-superseded.ts notes "verified in production: zero reversal events ever". `accuracyPct` now returns null unless a reversal is OBSERVABLE on this deployment — a recorded reversal proves it outright (and short-circuits the probe), otherwise there must be at least one terminal auto-action in the retained window for a reversal to attach to. This is the discipline the surface already applies elsewhere, extended to the case where the numerator's WRITER, not just its sample, is absent: a sparse rule's precision is null, and gamingFlagsCaught is null below three instances. Also fixes the weekly trend's denominator asymmetry: `merged`/`closed` folded in registered-Orb rows while `reversed` stayed own-ledger-only, so the denominator grew with every new install while the numerator did not — trending every week toward 100% independent of real reversal behavior. #7449 fixed exactly this for the lifetime figure and it was never carried across. Accuracy now divides the own-ledger pairing; the displayed volume still includes the fleet. UI: the withheld cells say why rather than leaving a bare em dash next to healthy volume, and the headline now discloses `basis` — #9168 computes "single_instance_self_report" precisely so one operator's self-reported number is not read as fleet corroboration, and the page dropped the field entirely, along with the sample size and confidence interval. The methodology note also described the reversal formula while the headline rendered decisionAccuracy; it now explains both and why they differ. Refs #9676
1 parent 56961e0 commit 73e30da

6 files changed

Lines changed: 288 additions & 27 deletions

File tree

apps/loopover-ui/src/components/site/fairness-report-page.test.tsx

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,65 @@ describe("FairnessReportPage (#fairness-analytics)", () => {
220220
expect(screen.getByText("2 human-reversed, lifetime")).toBeTruthy();
221221
});
222222

223+
it("explains a withheld accuracy instead of letting it read as a dash-shaped mystery", async () => {
224+
// Backend publishes null when no auto-action was ever recorded for a reversal to attach to; the page must
225+
// say that rather than leaving a bare "—" next to a healthy-looking volume.
226+
apiFetch.mockResolvedValue({
227+
ok: true,
228+
data: {
229+
...FIXTURE,
230+
byProject: [
231+
{ project: "owner/repo", reviewed: 100, merged: 60, closed: 30, accuracyPct: null },
232+
],
233+
accuracyTrend: [
234+
{ weekStart: "2026-07-13", merged: 30, closed: 15, reversed: 0, accuracyPct: null },
235+
],
236+
},
237+
status: 200,
238+
durationMs: 10,
239+
});
240+
renderWithClient(<FairnessReportPage />);
241+
242+
await waitFor(() => expect(screen.getByText("By repository")).toBeTruthy());
243+
const notes = screen.getAllByText(/not measurable on this deployment, not 100%/);
244+
expect(notes.length).toBe(2); // one under each affected table
245+
});
246+
247+
it("does not show the unmeasurable-accuracy note when every accuracy is real", async () => {
248+
apiFetch.mockResolvedValue({ ok: true, data: FIXTURE, status: 200, durationMs: 10 });
249+
renderWithClient(<FairnessReportPage />);
250+
251+
await waitFor(() => expect(screen.getByText("By repository")).toBeTruthy());
252+
expect(screen.queryByText(/not measurable on this deployment/)).toBeNull();
253+
});
254+
255+
it("#9168: discloses a single-instance self-report rather than presenting it as fleet corroboration", async () => {
256+
apiFetch.mockResolvedValue({
257+
ok: true,
258+
data: {
259+
...FIXTURE,
260+
fleetAccuracy: {
261+
...FIXTURE.fleetAccuracy,
262+
instanceCount: 1,
263+
basis: "single_instance_self_report",
264+
decidedCount: 5225,
265+
accuracyCiPct: { lo: 93.9, hi: 95.2 },
266+
},
267+
},
268+
status: 200,
269+
durationMs: 10,
270+
});
271+
renderWithClient(<FairnessReportPage />);
272+
273+
await waitFor(() => expect(screen.getByText("Decision accuracy")).toBeTruthy());
274+
expect(screen.getByText(/Self-reported by that single instance/).textContent).toContain(
275+
"5,225 decided",
276+
);
277+
expect(screen.getByText(/Self-reported by that single instance/).textContent).toContain(
278+
"93.9–95.2%",
279+
);
280+
});
281+
223282
it("#9068: renders the insufficient-instances state (not a fabricated zero) when gamingFlagsCaught is null", async () => {
224283
apiFetch.mockResolvedValue({
225284
ok: true,

apps/loopover-ui/src/components/site/fairness-report-page.tsx

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,19 @@ import type { PublicStats } from "@/components/site/proof-of-power-stats-model";
1515
// count, with a short methodology note. Counts only; no PR content, contributor identities, or trust scores.
1616

1717
const pctFmt = new Intl.NumberFormat("en", { maximumFractionDigits: 1 });
18+
/** Why an accuracy cell reads "—". Reversal-grounded accuracy needs the deployment to have recorded the
19+
* terminal auto-actions a reversal attaches to; where it hasn't, `1 - 0/N` would render as a flawless 100%
20+
* over a numerator that can never move, so the backend publishes null and the page says so out loud. */
21+
function UnmeasurableAccuracyNote() {
22+
return (
23+
<p className="mt-3 text-token-xs text-muted-foreground">
24+
An accuracy of <span className="font-mono"></span> means not measurable on this deployment,
25+
not 100%: no auto-merge/auto-close was recorded here for a human reversal to be counted
26+
against. The volume columns beside it are measured directly and are unaffected.
27+
</p>
28+
);
29+
}
30+
1831
const intFmt = new Intl.NumberFormat("en");
1932

2033
async function fetchPublicStats(): Promise<PublicStats | null> {
@@ -119,6 +132,23 @@ export function FairnessReportPage() {
119132
? `${intFmt.format(data.totals.reversed)} human-reversed, lifetime`
120133
: "reversal-grounded, lifetime"}
121134
</p>
135+
{/* #9168 computes `basis` precisely so this number is not read as corroborated-across-operators
136+
when it is one operator's own disclosed outcomes; the page used to drop the field entirely. */}
137+
{fleetEligible && data.fleetAccuracy.basis === "single_instance_self_report" ? (
138+
<p className="mt-2 text-token-xs text-muted-foreground">
139+
Self-reported by that single instance, not corroborated across operators
140+
{data.fleetAccuracy.decidedCount != null
141+
? ` (${intFmt.format(data.fleetAccuracy.decidedCount)} decided`
142+
: ""}
143+
{data.fleetAccuracy.decidedCount != null &&
144+
data.fleetAccuracy.accuracyCiPct != null
145+
? `, 95% CI ${pctFmt.format(data.fleetAccuracy.accuracyCiPct.lo)}${pctFmt.format(data.fleetAccuracy.accuracyCiPct.hi)}%)`
146+
: data.fleetAccuracy.decidedCount != null
147+
? ")"
148+
: ""}
149+
.
150+
</p>
151+
) : null}
122152
</Card>
123153
<Card className="p-5">
124154
<div className="text-token-xs text-muted-foreground">Anti-gaming flags caught</div>
@@ -149,11 +179,15 @@ export function FairnessReportPage() {
149179

150180
<div className="mt-10 space-y-2 rounded-token border-hairline px-4 py-4 text-token-sm text-muted-foreground">
151181
<p>
152-
<span className="font-medium text-foreground">How accuracy is measured:</span> 1
153-
minus the share of auto-merged/auto-closed PRs a human later overturned — a
154-
bot-closed PR a contributor reopened, or a bot-merged PR undone by a separate revert
155-
PR. Nothing here is a prediction or a self-assessment; it's counted after the fact
156-
from what actually happened on GitHub.
182+
<span className="font-medium text-foreground">How accuracy is measured:</span> the
183+
headline scores the gate's own merge/close <em>decisions</em> — the share the
184+
realized outcome confirmed, with holds excluded because a deferral to a human is not
185+
a decision that can be right or wrong (#8820). The per-repository and weekly tables
186+
below are a different, stricter measure: 1 minus the share of
187+
auto-merged/auto-closed PRs a human later overturned — a bot-closed PR a contributor
188+
reopened, or a bot-merged PR undone by a separate revert PR. Neither is a prediction
189+
or a self-assessment; both are counted after the fact from what actually happened on
190+
GitHub, which is also why the two can differ.
157191
</p>
158192
<p>
159193
<span className="font-medium text-foreground">
@@ -206,6 +240,9 @@ export function FairnessReportPage() {
206240
</tbody>
207241
</table>
208242
</TableScroll>
243+
{data.byProject.some((row) => row.accuracyPct == null) ? (
244+
<UnmeasurableAccuracyNote />
245+
) : null}
209246
</div>
210247
) : null}
211248

@@ -256,6 +293,11 @@ export function FairnessReportPage() {
256293
</tbody>
257294
</table>
258295
</TableScroll>
296+
{data.accuracyTrend.some(
297+
(week) => week.merged != null && week.accuracyPct == null,
298+
) ? (
299+
<UnmeasurableAccuracyNote />
300+
) : null}
259301
</div>
260302

261303
{data.rulePrecision && data.rulePrecision.rules.length > 0 ? (

src/review/public-stats.ts

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ import { validateCalibrationPayload } from "./risk-control";
5555
import { loadRepoFocusManifest } from "../signals/focus-manifest-loader";
5656
import { resolveLoopOverSelfRepoFullName } from "../config/loopover-repo-focus-manifest";
5757
import { errorMessage } from "../utils/json";
58+
import { retentionCutoffIsoForTable } from "../db/retention";
5859

5960
/** FALLBACK estimate of maintainer review/triage time saved per reviewed PR, used ONLY when the real per-PR
6061
* average (`estimateReviewEffort`'s minutes, persisted at publish time — see `reviewEffortMinutes` in the
@@ -164,12 +165,47 @@ function filteredPct(reviewed: number, merged: number): number | null {
164165
return Math.round(((reviewed - merged) / reviewed) * 1000) / 10;
165166
}
166167

167-
/** Reversal-grounded accuracy over the irreversible auto-actions (merged + closed); null until there is signal. */
168+
/** The auto-actions a reversal is recorded against. `recordReversalSignals` only ever fires for a PR this
169+
* deployment itself auto-merged or auto-closed, so with zero of these rows in the retained window a
170+
* `reversal_*` event cannot exist — and `1 - 0/N` is then a structural zero, not a measurement. */
171+
export const AUTO_ACTION_EVENT_TYPES = ["agent.action.close", "agent.action.merge"] as const;
172+
173+
/**
174+
* Whether a reversal is OBSERVABLE on this deployment: did it record any terminal auto-action in the window a
175+
* reversal could still be attributed to? Reversal signals are written only by the review-execution pipeline
176+
* (`recordReversalSignals`, reached via the `github-webhook` job), which a runtime that does not execute
177+
* reviews acks-and-drops — so on such a runtime `reversed` is pinned at 0 forever while the merged/closed
178+
* denominator keeps growing, and every reversal-grounded percentage converges on a fake 100%.
179+
*
180+
* Publishing that as accuracy is the exact failure #8820 called out for the fleet number ("the old formula
181+
* overstated accuracy") and #7449 fixed for the global one. This is the same discipline the rest of this
182+
* surface already applies — a sparse rule's precision is null, `gamingFlagsCaught` is null below three
183+
* instances — extended to the case where the numerator's WRITER, not just its sample, is absent.
184+
*/
185+
export async function loadReversalObservability(env: Env, knownReversals = 0): Promise<boolean> {
186+
// A recorded reversal is itself proof the pipeline that records them runs here -- no probe needed, and no
187+
// query on the hot path for any deployment that has ever overturned an auto-action.
188+
if (knownReversals > 0) return true;
189+
const rows = await safeAll<{ n: number }>(
190+
env,
191+
`SELECT COUNT(*) AS n FROM audit_events
192+
WHERE event_type IN (${AUTO_ACTION_EVENT_TYPES.map((type) => `'${type}'`).join(", ")})
193+
AND created_at >= ?`,
194+
retentionCutoffIsoForTable("audit_events"),
195+
);
196+
return (rows[0]?.n ?? 0) > 0;
197+
}
198+
199+
/** Reversal-grounded accuracy over the irreversible auto-actions (merged + closed); null until there is signal,
200+
* and null when a reversal is not observable at all on this deployment (see {@link loadReversalObservability}) —
201+
* an unmeasurable quantity stays unknown rather than rendering as a perfect score. */
168202
function accuracyPct(
169203
merged: number,
170204
closed: number,
171205
reversed: number,
206+
reversalObservable: boolean,
172207
): number | null {
208+
if (!reversalObservable) return null;
173209
const decided = merged + closed;
174210
if (decided <= 0) return null;
175211
// `reversed` counts engine auto-actions regardless of a PR's CURRENT disposition, so a reopened
@@ -426,6 +462,10 @@ export async function getPublicStats(
426462
error: 0,
427463
reversed: 0,
428464
};
465+
// Resolved before byProject so every published accuracy figure -- per repo and global -- answers the same
466+
// question about the same deployment, rather than one of them silently disagreeing with the others.
467+
const totalReversals = [...reversedByProject.values()].reduce((sum, n) => sum + n, 0);
468+
const reversalObservable = await loadReversalObservability(env, totalReversals);
429469
const byProject = dispositions
430470
.map((d) => {
431471
const merged = d.merged ?? 0;
@@ -445,7 +485,7 @@ export async function getPublicStats(
445485
reviewed,
446486
merged,
447487
closed,
448-
accuracyPct: accuracyPct(merged, closed, reversed),
488+
accuracyPct: accuracyPct(merged, closed, reversed, reversalObservable),
449489
};
450490
})
451491
.filter((r) => r.reviewed > 0)
@@ -573,7 +613,7 @@ export async function getPublicStats(
573613
// Option 1 of #7449: compute the global accuracy from the OWN-LEDGER merged/closed snapshot (not the
574614
// fleet-folded totals.merged/closed), so its numerator (own-ledger reversed) and denominator are drawn
575615
// from the same population. See the ownLedgerMerged/ownLedgerClosed snapshot above the Orb fold for why.
576-
accuracyPct: accuracyPct(ownLedgerMerged, ownLedgerClosed, totals.reversed),
616+
accuracyPct: accuracyPct(ownLedgerMerged, ownLedgerClosed, totals.reversed, reversalObservable),
577617
minutesSaved,
578618
},
579619
weekly: { reviewed: w.reviewed ?? 0, merged: w.merged ?? 0 },

src/services/public-accuracy-trend.ts

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
// #2568 pattern for the sibling per-repo quality trend) can recompute any historical week correctly on every
99
// request -- no cron-miss gap risk, no second copy of the number to keep in sync, and the SAME formula as the
1010
// live figure by construction, so the two can never silently diverge or read as inconsistent to a public viewer.
11-
import { PUBLISHED_PR_KEYS, publicStatsProjects, safeAll } from "../review/public-stats";
11+
import { loadReversalObservability, PUBLISHED_PR_KEYS, publicStatsProjects, safeAll } from "../review/public-stats";
1212
import { isoWeekStart } from "./public-quality-metrics";
1313

1414
export const PUBLIC_ACCURACY_TREND_WEEKS = 8;
@@ -24,7 +24,13 @@ export type PublicAccuracyTrendWeek = {
2424
accuracyPct: number | null;
2525
};
2626

27-
type DayRow = { day: string; merged: number; closed: number; reversed: number };
27+
/** `merged`/`closed` are the DISPLAYED volume (own ledger + registered Orb fleet). `ownMerged`/`ownClosed` are
28+
* the own-ledger-only pairing for `reversed`, which is own-ledger-only by construction (the Orb aggregate has
29+
* no reversal concept). Accuracy divides the own-ledger numbers ONLY -- #7449 fixed exactly this asymmetry for
30+
* the lifetime figure in public-stats.ts and it was never carried across to this trend, so the denominator grew
31+
* with every newly registered install while the numerator stayed own-ledger-scoped, trending every week toward
32+
* 100% independent of real reversal behavior. */
33+
type DayRow = { day: string; merged: number; closed: number; ownMerged: number; ownClosed: number; reversed: number };
2834

2935
const MS_PER_WEEK = 7 * 86_400_000;
3036

@@ -33,20 +39,35 @@ function roundPct(value: number): number {
3339
}
3440

3541
/** Same formula as public-stats.ts's accuracyPct, reused so the trend and the live number can never drift
36-
* apart into two competing definitions of "accuracy". */
37-
function publicBucketOf(bucket: { merged: number; closed: number; reversed: number }): Omit<PublicAccuracyTrendWeek, "weekStart"> {
42+
* apart into two competing definitions of "accuracy". `reversalObservable` false means this deployment records
43+
* no terminal auto-actions for a reversal to be attributed to, so the week's accuracy is unknown, not perfect --
44+
* the volume columns still publish, since those ARE measured. */
45+
function publicBucketOf(
46+
bucket: { merged: number; closed: number; ownMerged: number; ownClosed: number; reversed: number },
47+
reversalObservable: boolean,
48+
): Omit<PublicAccuracyTrendWeek, "weekStart"> {
3849
const decided = bucket.merged + bucket.closed;
3950
if (decided < MIN_ACCURACY_TREND_SAMPLE) return { merged: null, closed: null, reversed: null, accuracyPct: null };
40-
const reversalRate = Math.min(1, bucket.reversed / decided);
41-
return { merged: bucket.merged, closed: bucket.closed, reversed: bucket.reversed, accuracyPct: roundPct(1 - reversalRate) };
51+
const observed = { merged: bucket.merged, closed: bucket.closed, reversed: bucket.reversed };
52+
if (!reversalObservable) return { ...observed, accuracyPct: null };
53+
// Own-ledger-only denominator: `reversed` can only ever be attributed to own-ledger PRs (see DayRow).
54+
const ownDecided = bucket.ownMerged + bucket.ownClosed;
55+
if (ownDecided <= 0) return { ...observed, accuracyPct: null };
56+
const reversalRate = Math.min(1, bucket.reversed / ownDecided);
57+
return { ...observed, accuracyPct: roundPct(1 - reversalRate) };
4258
}
4359

4460
/** Fold day-granularity rows into `weeks` trailing UTC-Monday buckets ending in the week containing `nowMs`.
4561
* Pure -- mirrors buildPublicQualityTrend's own bucketing shape (public-quality-metrics.ts). */
46-
export function buildPublicAccuracyTrend(dayRows: DayRow[], nowMs: number, weeks: number = PUBLIC_ACCURACY_TREND_WEEKS): PublicAccuracyTrendWeek[] {
62+
export function buildPublicAccuracyTrend(
63+
dayRows: DayRow[],
64+
nowMs: number,
65+
weeks: number = PUBLIC_ACCURACY_TREND_WEEKS,
66+
reversalObservable = true,
67+
): PublicAccuracyTrendWeek[] {
4768
const currentStartMs = Date.parse(isoWeekStart(nowMs));
4869
const oldestStartMs = currentStartMs - (weeks - 1) * MS_PER_WEEK;
49-
const buckets = Array.from({ length: weeks }, () => ({ merged: 0, closed: 0, reversed: 0 }));
70+
const buckets = Array.from({ length: weeks }, () => ({ merged: 0, closed: 0, ownMerged: 0, ownClosed: 0, reversed: 0 }));
5071

5172
for (const row of dayRows) {
5273
const dayMs = Date.parse(`${row.day}T00:00:00.000Z`);
@@ -56,12 +77,14 @@ export function buildPublicAccuracyTrend(dayRows: DayRow[], nowMs: number, weeks
5677
const bucket = buckets[weekOffset]!;
5778
bucket.merged += row.merged;
5879
bucket.closed += row.closed;
80+
bucket.ownMerged += row.ownMerged;
81+
bucket.ownClosed += row.ownClosed;
5982
bucket.reversed += row.reversed;
6083
}
6184

6285
return buckets.map((bucket, offset) => ({
6386
weekStart: isoWeekStart(oldestStartMs + offset * MS_PER_WEEK),
64-
...publicBucketOf(bucket),
87+
...publicBucketOf(bucket, reversalObservable),
6588
}));
6689
}
6790

@@ -169,19 +192,22 @@ export async function loadPublicAccuracyTrend(env: Env, nowMs: number = Date.now
169192
const projects = publicStatsProjects(env);
170193
const sinceIso = new Date(Date.parse(isoWeekStart(nowMs)) - (PUBLIC_ACCURACY_TREND_WEEKS - 1) * MS_PER_WEEK).toISOString();
171194

172-
const [ownLedger, reversals, orb] = await Promise.all([
195+
const [ownLedger, reversals, orb, reversalObservable] = await Promise.all([
173196
loadOwnLedgerDayRows(env, projects, sinceIso),
174197
loadReversalDayRows(env, projects, sinceIso),
175198
loadOrbDayRows(env, sinceIso),
199+
loadReversalObservability(env),
176200
]);
177201

178202
const days = new Set([...ownLedger.keys(), ...reversals.keys(), ...orb.keys()]);
179203
const dayRows: DayRow[] = [...days].map((day) => ({
180204
day,
181205
merged: (ownLedger.get(day)?.merged ?? 0) + (orb.get(day)?.merged ?? 0),
182206
closed: (ownLedger.get(day)?.closed ?? 0) + (orb.get(day)?.closed ?? 0),
207+
ownMerged: ownLedger.get(day)?.merged ?? 0,
208+
ownClosed: ownLedger.get(day)?.closed ?? 0,
183209
reversed: reversals.get(day) ?? 0,
184210
}));
185211

186-
return buildPublicAccuracyTrend(dayRows, nowMs);
212+
return buildPublicAccuracyTrend(dayRows, nowMs, PUBLIC_ACCURACY_TREND_WEEKS, reversalObservable);
187213
}

0 commit comments

Comments
 (0)