From e429fd94bed0d9d4b5bbd97a0a5d76a46d983da4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:03:49 -0700 Subject: [PATCH 1/2] metrics(ledger): weekly automation-rate series, published on /fairness (#9727, #9728) "How much of this is actually automated?" underwrites every other claim the gate makes, and it was only answerable by ad-hoc query. It is a standing series now, computed from decision_records alone so an outsider holding the export recomputes the same numbers. The definition is the load-bearing part. A PR is automated only if EVERY verdict for it was a merge or close with no human in the path -- a hold, a named reevaluation actor, or a maintainer_request all make it manual. So a PR that was held and later merged is manual, not automated: counting the final disposition would let the rate be inflated by holding everything and merging it by hand, which is precisely the failure this number exists to expose. Weeks are bucketed by a PR's FIRST verdict, so a PR does not migrate between weeks as it accrues re-evaluations, and it is counted once however many verdicts it has. BACKFILL HORIZON, stated rather than hidden: the re-evaluation provenance fields arrived with migration 0204, so earlier weeks can only detect manual work that took the form of a hold and are published as basis "holds_only". A week straddling that date is labelled holds_only too -- understating confidence rather than overstating it, which is the direction of error a fairness figure has to take. /fairness renders the rate, the weekly table and the definition, with the same zero-state conventions as the sibling series: an empty window is a MEASURED zero with its dates, never a blank. --- .../content/docs/verify-this-review.mdx | 24 +++ apps/loopover-ui/public/openapi.json | 65 ++++++ .../site/fairness-report-page.test.tsx | 89 ++++++++ .../components/site/fairness-report-page.tsx | 97 +++++++++ .../site/proof-of-power-stats.test.tsx | 7 + packages/loopover-contract/src/public-api.ts | 30 +++ src/api/routes.ts | 8 +- src/review/automation-rate.ts | 191 ++++++++++++++++++ test/unit/automation-rate.test.ts | 179 ++++++++++++++++ 9 files changed, 688 insertions(+), 2 deletions(-) create mode 100644 src/review/automation-rate.ts create mode 100644 test/unit/automation-rate.test.ts diff --git a/apps/loopover-ui/content/docs/verify-this-review.mdx b/apps/loopover-ui/content/docs/verify-this-review.mdx index d9a2296831..6612d9e6b7 100644 --- a/apps/loopover-ui/content/docs/verify-this-review.mdx +++ b/apps/loopover-ui/content/docs/verify-this-review.mdx @@ -157,6 +157,30 @@ A rate over an empty window is `null`, never `0` — "nothing was held" and "not different claims, and the fairness report renders the second as *measured zero* with its window rather than as a reassuring number. +### Automation rate + +`reviewParity`'s sibling, from the same ledger (#9727): + +```bash +curl -s "https://api.loopover.ai/v1/public/stats" | jq '.automationRate' +``` + +A pull request counts as **automated** only if every verdict for it was a merge or close with no human in +the path. Three things mark a human: + +- `action = 'hold'` — the gate declined to decide and handed it to a person, +- a `reevaluation_actor` — a named person caused a re-evaluation, +- `reevaluation_reason = 'maintainer_request'` — a human asked for the re-run. + +So a PR that was **held and later merged is manual**, not automated. Counting the final disposition instead +would let the rate be inflated by holding everything and then merging it by hand — which is exactly the +failure mode this number exists to make visible. + +Each week carries a `basis`. `holds_only` weeks start before `provenanceHorizon`, the date the bot began +recording who caused a re-evaluation; those weeks can only detect manual work that took the form of a hold, +so they **under-count** it. A week straddling that date is labelled `holds_only` too — understating +confidence rather than overstating it. + The same per-rule numbers are also published as digest-committed [EvalScoreRecords](/docs/what-you-can-verify), each independently re-derivable without trusting the transport — recompute `recordDigest` over the record's own remaining fields and compare: diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index 28e8d2aed6..61c6a7ca09 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -999,6 +999,70 @@ "byAuthorClass", "byProject" ] + }, + "automationRate": { + "type": "object", + "properties": { + "weeks": { + "type": "array", + "items": { + "type": "object", + "properties": { + "weekStart": { + "type": "string" + }, + "decided": { + "type": "number" + }, + "automated": { + "type": "number" + }, + "manual": { + "type": "number" + }, + "automationRatePct": { + "type": "number", + "nullable": true + }, + "basis": { + "type": "string", + "enum": [ + "full", + "holds_only" + ] + } + }, + "required": [ + "weekStart", + "decided", + "automated", + "manual", + "automationRatePct", + "basis" + ] + } + }, + "decided": { + "type": "number" + }, + "automated": { + "type": "number" + }, + "automationRatePct": { + "type": "number", + "nullable": true + }, + "provenanceHorizon": { + "type": "string" + } + }, + "required": [ + "weeks", + "decided", + "automated", + "automationRatePct", + "provenanceHorizon" + ] } }, "required": [ @@ -1008,6 +1072,7 @@ "weekly", "rulePrecision", "reviewParity", + "automationRate", "byProject", "fleetAccuracy", "accuracyTrend", diff --git a/apps/loopover-ui/src/components/site/fairness-report-page.test.tsx b/apps/loopover-ui/src/components/site/fairness-report-page.test.tsx index 0b7e744f56..bbb6727e08 100644 --- a/apps/loopover-ui/src/components/site/fairness-report-page.test.tsx +++ b/apps/loopover-ui/src/components/site/fairness-report-page.test.tsx @@ -40,6 +40,13 @@ function renderWithClient(ui: ReactNode) { const FIXTURE: PublicStats = { // The wire always carries rulePrecision (#8230/#8231). A fixture without it is not a payload the // current backend can produce -- the one test that needs that shape strips it explicitly. + automationRate: { + weeks: [], + decided: 0, + automated: 0, + automationRatePct: null, + provenanceHorizon: "2026-07-29T00:00:00.000Z", + }, reviewParity: { windowStart: "2026-07-22T00:00:00.000Z", windowEnd: "2026-07-29T00:00:00.000Z", @@ -509,4 +516,86 @@ describe("FairnessReportPage (#fairness-analytics)", () => { expect(await screen.findByText(/No re-evaluations recorded/i)).toBeTruthy(); expect(screen.queryByText(/No verdicts recorded/i)).toBeNull(); }); + + // #9728: the automation-rate surface and its zero/reduced-basis states. + function automationFixture(over: Record) { + return { + ok: true, + durationMs: 10, + data: { + ...FIXTURE, + automationRate: { + weeks: [], + decided: 0, + automated: 0, + automationRatePct: null, + provenanceHorizon: "2026-07-29T00:00:00.000Z", + ...over, + }, + }, + }; + } + + it("renders the headline rate, the weekly table, and the definition", async () => { + apiFetch.mockResolvedValue( + automationFixture({ + decided: 10, + automated: 7, + automationRatePct: 70, + weeks: [ + { + weekStart: "2026-07-27T00:00:00.000Z", + decided: 10, + automated: 7, + manual: 3, + automationRatePct: 70, + basis: "full", + }, + ], + }), + ); + renderWithClient(); + + expect(await screen.findByText(/Automation rate/i)).toBeTruthy(); + expect(screen.getByText(/70% automated/)).toBeTruthy(); + expect(screen.getByText(/7 of 10 pull requests/)).toBeTruthy(); + // The definition must be readable without opening source -- that is #9728's acceptance. + expect(screen.getByText(/no human action/i)).toBeTruthy(); + expect(screen.getByText(/counts as manual even if it later merged/i)).toBeTruthy(); + }); + + it("labels reduced-basis weeks and explains that they UNDER-count manual work", async () => { + apiFetch.mockResolvedValue( + automationFixture({ + decided: 4, + automated: 4, + automationRatePct: 100, + weeks: [ + { + weekStart: "2026-07-06T00:00:00.000Z", + decided: 4, + automated: 4, + manual: 0, + automationRatePct: 100, + basis: "holds_only", + }, + ], + }), + ); + renderWithClient(); + + expect( + (await screen.findAllByText(/reduced basis/i)).length, + "the row badge and the footnote both say it", + ).toBeGreaterThanOrEqual(2); + expect(screen.getByText(/under-count it rather than over-count it/i)).toBeTruthy(); + }); + + it("renders an empty window as a measured zero, not as missing data", async () => { + apiFetch.mockResolvedValue(automationFixture({})); + renderWithClient(); + + expect(await screen.findByText(/No pull requests decided/i)).toBeTruthy(); + expect(screen.getByText(/measured zero, not missing data/i)).toBeTruthy(); + }); }); diff --git a/apps/loopover-ui/src/components/site/fairness-report-page.tsx b/apps/loopover-ui/src/components/site/fairness-report-page.tsx index d025c6cd74..91c28f643d 100644 --- a/apps/loopover-ui/src/components/site/fairness-report-page.tsx +++ b/apps/loopover-ui/src/components/site/fairness-report-page.tsx @@ -424,6 +424,103 @@ export function FairnessReportPage() { ) : null} + {/* #9728: the automation rate — the share of PRs decided with no human in the path. Rendered + unconditionally when present: a window that decided nothing is a MEASURED zero and says so, + which is a different claim from "we did not compute this". */} + {data.automationRate ? ( +
+

Automation rate

+

+ The share of pull requests decided and enacted with{" "} + no human action between open + and disposition. A PR that was held for a person — or that a maintainer asked to + be re-run — counts as manual even if it later merged: the question is whether + someone had to act, not how it ended. Computed from the ledger alone, so you can + recompute it:{" "} + + verify this review + + . +

+ + {data.automationRate.decided === 0 ? ( +

+ No pull requests decided in + this window — a measured zero, not missing data. +

+ ) : ( + <> +

+ {data.automationRate.automationRatePct != null + ? `${pctFmt.format(data.automationRate.automationRatePct)}% automated` + : "Rate unavailable"}{" "} + + ({intFmt.format(data.automationRate.automated)} of{" "} + {intFmt.format(data.automationRate.decided)} pull requests) + +

+ + + + + + + + + + + + + {data.automationRate.weeks.map((week) => ( + + + + + + + ))} + +
+ Automated and manual pull requests per week, with each week's + measurement basis. +
+ Week + + Decided + + Automated + + Rate +
+ {new Date(week.weekStart).toLocaleDateString()} + {week.basis === "holds_only" ? ( + + reduced basis + + ) : null} + {intFmt.format(week.decided)}{intFmt.format(week.automated)} + {week.automationRatePct != null + ? `${pctFmt.format(week.automationRatePct)}%` + : "—"} +
+ +

+ Weeks marked{" "} + reduced basis start + before {new Date(data.automationRate.provenanceHorizon).toLocaleDateString()}, + when the bot began recording who caused a re-evaluation. Those weeks can only + detect manual work that took the form of a hold, so they under-count it rather + than over-count it. +

+ + )} +
+ ) : null} + {/* #9744: re-evaluation rate + author-class parity. Rendered UNCONDITIONALLY when the block is present -- a window with no verdicts is a MEASURED zero and says so with its own bounds, which is a different claim from "we did not compute this" and must not look identical. */} diff --git a/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx b/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx index 2c763ca733..898ae5e6c3 100644 --- a/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx +++ b/apps/loopover-ui/src/components/site/proof-of-power-stats.test.tsx @@ -40,6 +40,13 @@ import { const PAYLOAD: PublicStats = { // The wire always carries rulePrecision (#8230/#8231). A fixture without it is not a payload the // current backend can produce -- the one test that needs that shape strips it explicitly. + automationRate: { + weeks: [], + decided: 0, + automated: 0, + automationRatePct: null, + provenanceHorizon: "2026-07-29T00:00:00.000Z", + }, reviewParity: { windowStart: "2026-07-22T00:00:00.000Z", windowEnd: "2026-07-29T00:00:00.000Z", diff --git a/packages/loopover-contract/src/public-api.ts b/packages/loopover-contract/src/public-api.ts index 8be0164a08..932a55db75 100644 --- a/packages/loopover-contract/src/public-api.ts +++ b/packages/loopover-contract/src/public-api.ts @@ -74,6 +74,34 @@ export const ReviewParitySchema = z.object({ byProject: z.array(z.object({ project: z.string(), byAuthorClass: z.array(ParityRollupSchema) })), }); +/** + * Weekly automation-rate series (#9727): the share of pull requests decided with no human in the path. + * Computed from `decision_records` alone; definitions live beside the implementation in + * src/review/automation-rate.ts and are restated in the verifier walkthrough. + */ +export const AutomationRateWeekSchema = z.object({ + /** ISO date-time of the week's UTC Monday. */ + weekStart: z.string(), + /** Distinct pull requests with at least one verdict that week, bucketed by their FIRST verdict. */ + decided: z.number(), + automated: z.number(), + manual: z.number(), + /** Null when the week decided nothing -- an undefined ratio, never a reassuring 100%. */ + automationRatePct: z.number().nullable(), + /** `holds_only` weeks predate the re-evaluation provenance fields and can only UNDER-count manual work. + * A week straddling the horizon is labelled `holds_only` too: understating confidence, not overstating. */ + basis: z.enum(["full", "holds_only"]), +}); + +export const AutomationRateSchema = z.object({ + weeks: z.array(AutomationRateWeekSchema), + decided: z.number(), + automated: z.number(), + automationRatePct: z.number().nullable(), + /** The date the provenance fields began being written, so a reader can see which weeks are reduced-basis. */ + provenanceHorizon: z.string(), +}); + export const PublicStatsSchema = z.object({ generatedAt: z.string(), updatedAt: z.string(), @@ -94,6 +122,7 @@ export const PublicStatsSchema = z.object({ weekly: z.object({ reviewed: z.number(), merged: z.number() }), rulePrecision: PublicRulePrecisionSchema, reviewParity: ReviewParitySchema, + automationRate: AutomationRateSchema, byProject: z.array( z.object({ project: z.string(), @@ -186,4 +215,5 @@ export const PublicStatsSchema = z.object({ export type PublicStats = z.infer; export type ReviewParity = z.infer; +export type AutomationRate = z.infer; export type ParityRollup = z.infer; diff --git a/src/api/routes.ts b/src/api/routes.ts index 13b4c71562..91950ffe05 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -14,6 +14,7 @@ import { buildFindingTaxonomyDocument } from "../review/finding-taxonomy"; import { buildEnrichmentAnalyzersTaxonomyDocument } from "../review/enrichment-analyzers-taxonomy"; import { deliveryIdFor } from "../queue/delivery-id"; import { loadReviewParityRollups } from "../review/review-parity-rollups"; +import { loadAutomationRateSeries } from "../review/automation-rate"; import { GITHUB_OAUTH_STATE_COOKIE, authenticateInternalToken, @@ -672,7 +673,7 @@ export function createApp() { const publicStatsManifestOverride = await resolvePublicStatsManifestOverride(c.env); if (!isPublicStatsEnabled(c.env, publicStatsManifestOverride)) return c.json({ error: "not_found" }, 404); try { - const [stats, accuracyTrend, fleetAccuracyTrend, reuseRateTrend, reviewVolumeTrend, rulePrecision, reviewParity] = await Promise.all([ + const [stats, accuracyTrend, fleetAccuracyTrend, reuseRateTrend, reviewVolumeTrend, rulePrecision, reviewParity, automationRate] = await Promise.all([ getPublicStats(c.env), loadPublicAccuracyTrend(c.env), // #9676: the fleet-population sibling of the series above. Deliberately a SECOND series rather than a @@ -688,9 +689,12 @@ export function createApp() { // from `decision_records` alone so an outsider holding the ledger export can recompute every figure // -- the definitions live beside the code in review-parity-rollups.ts. loadReviewParityRollups(c.env), + // #9727: the weekly automation rate -- share of PRs decided with no human in the path. Same ledger, + // same reproducibility contract; definitions live beside the code in review/automation-rate.ts. + loadAutomationRateSeries(c.env), ]); c.header("Cache-Control", "public, max-age=60, stale-while-revalidate=300"); - return c.json({ ...stats, accuracyTrend, fleetAccuracyTrend, reuseRateTrend, reviewVolumeTrend, rulePrecision, reviewParity }); + return c.json({ ...stats, accuracyTrend, fleetAccuracyTrend, reuseRateTrend, reviewVolumeTrend, rulePrecision, reviewParity, automationRate }); } catch { return c.json({ error: "public_stats_unavailable" }, 503); } diff --git a/src/review/automation-rate.ts b/src/review/automation-rate.ts new file mode 100644 index 0000000000..81f5927750 --- /dev/null +++ b/src/review/automation-rate.ts @@ -0,0 +1,191 @@ +// Weekly automation-rate series (#9727). +// +// "How much of this is actually automated?" is the question every claim about the gate rests on, and it was +// only answerable by ad-hoc query. This publishes it as a standing series, computed from `decision_records` +// -- the anchored ledger -- and nothing else, so an outsider holding the export recomputes the same numbers. +// +// PUBLISHED DEFINITION. A pull request is counted once per week, by the week of its FIRST verdict. +// +// manual ANY verdict for that PR shows a human in the decision path: +// - `action = 'hold'` -- the gate declined to decide and handed it to a person +// - `reevaluation_actor` -- a named person caused a re-evaluation (#9742) +// - `reevaluation_reason = 'maintainer_request'` -- a human asked for the re-run +// automated Every verdict was a merge or close with none of the above: opened, decided, enacted, with +// no human action in between. +// +// A PR that was held and LATER merged is manual, not automated. The question is whether a human had to act, +// not what the end state was -- counting the final disposition would let the rate be inflated by holding +// everything and then merging it by hand. +// +// BACKFILL HORIZON. The two re-evaluation fields arrived with migration 0204. Before that, only `hold` is +// observable, so a week earlier than the horizon can only UNDER-count manual work. Those weeks are published +// with `basis: "holds_only"` rather than silently mixed in with complete ones -- a series whose definition +// quietly changes partway along is worse than one that says where it changes. + +import { safeAll } from "./public-stats"; + +/** + * When the re-evaluation provenance fields (`reevaluation_reason`, `reevaluation_actor`) began being + * written -- migration 0204's ship date. Weeks starting before this can only detect manual work via `hold`. + * + * A single dated constant rather than something inferred: "the column did not exist yet" and "this was a + * first evaluation" both read as NULL, so the data genuinely cannot tell them apart. Stating the date is the + * honest option; guessing it from the rows would be a fabrication dressed as a derivation. + */ +export const AUTOMATION_RATE_PROVENANCE_HORIZON_ISO = "2026-07-29T00:00:00.000Z"; + +/** How completely a week could be measured. `full` weeks see every manual signal; `holds_only` weeks predate + * the provenance fields and can only under-count manual work. */ +export type AutomationWeekBasis = "full" | "holds_only"; + +export type AutomationRateWeek = { + /** ISO date of the week's Monday, UTC. */ + weekStart: string; + /** Distinct pull requests with at least one verdict that week. */ + decided: number; + automated: number; + manual: number; + /** Null when the week decided nothing -- an undefined ratio, not a reassuring 100%. */ + automationRatePct: number | null; + basis: AutomationWeekBasis; +}; + +export type AutomationRateSeries = { + weeks: AutomationRateWeek[]; + /** Totals over every week in the series, on the same definitions. */ + decided: number; + automated: number; + automationRatePct: number | null; + /** Published so a reader can see which weeks are `holds_only` without inspecting each. */ + provenanceHorizon: string; +}; + +/** One verdict, reduced to the fields the series reads. */ +export type AutomationVerdictRow = { + repoFullName: string; + pullNumber: number; + action: string; + createdAt: string; + reevaluationReason: string | null; + reevaluationActor: string | null; +}; + +/** The UTC Monday of the week containing `iso`, as an ISO date-time. Null when unparseable. */ +export function weekStartIso(iso: string): string | null { + const parsed = Date.parse(iso); + if (!Number.isFinite(parsed)) return null; + const date = new Date(parsed); + // getUTCDay: 0=Sunday. Shift so Monday starts the week, matching the other published weekly series. + const dayOffset = (date.getUTCDay() + 6) % 7; + const monday = Date.UTC(date.getUTCFullYear(), date.getUTCMonth(), date.getUTCDate() - dayOffset); + return new Date(monday).toISOString(); +} + +/** Percent to one decimal, or null when nothing was decided. */ +function ratePct(numerator: number, denominator: number): number | null { + if (denominator <= 0) return null; + return Math.round((numerator / denominator) * 1000) / 10; +} + +/** True when this verdict shows a human in the decision path. See the published definition above. */ +export function verdictShowsHumanAction(row: Pick): boolean { + if (row.action === "hold") return true; + if (typeof row.reevaluationActor === "string" && row.reevaluationActor.trim() !== "") return true; + return row.reevaluationReason === "maintainer_request"; +} + +/** + * Build the weekly series from a window's verdict rows. PURE, so every definition above is testable against + * a hand-written table -- which is also what lets an outsider check the arithmetic without our database. + */ +export function buildAutomationRateSeries(rows: readonly AutomationVerdictRow[]): AutomationRateSeries { + // Fold verdicts to PULL REQUESTS first: the unit of the question is "did a human have to act on this PR", + // and a PR with five verdicts is still one PR. Its week is the week of its FIRST verdict, so a PR does not + // migrate between weeks as it accrues re-evaluations. + const byPull = new Map(); + for (const row of rows) { + const key = `${row.repoFullName}#${row.pullNumber}`; + const existing = byPull.get(key); + const human = verdictShowsHumanAction(row); + if (!existing) { + byPull.set(key, { firstSeen: row.createdAt, human }); + continue; + } + existing.human = existing.human || human; + if (Date.parse(row.createdAt) < Date.parse(existing.firstSeen)) existing.firstSeen = row.createdAt; + } + + const horizon = Date.parse(AUTOMATION_RATE_PROVENANCE_HORIZON_ISO); + const weeks = new Map(); + for (const entry of byPull.values()) { + const week = weekStartIso(entry.firstSeen); + if (week === null) continue; + const bucket = weeks.get(week) ?? { decided: 0, automated: 0, manual: 0 }; + bucket.decided += 1; + if (entry.human) bucket.manual += 1; + else bucket.automated += 1; + weeks.set(week, bucket); + } + + const ordered = [...weeks.entries()] + .sort(([a], [b]) => a.localeCompare(b)) + .map(([weekStart, bucket]) => ({ + weekStart, + decided: bucket.decided, + automated: bucket.automated, + manual: bucket.manual, + automationRatePct: ratePct(bucket.automated, bucket.decided), + basis: (Date.parse(weekStart) >= horizon ? "full" : "holds_only") as AutomationWeekBasis, + })); + + const decided = ordered.reduce((sum, week) => sum + week.decided, 0); + const automated = ordered.reduce((sum, week) => sum + week.automated, 0); + return { + weeks: ordered, + decided, + automated, + automationRatePct: ratePct(automated, decided), + provenanceHorizon: AUTOMATION_RATE_PROVENANCE_HORIZON_ISO, + }; +} + +/** Trailing weeks the series covers, per #9727's "at least the trailing 12 weeks where data permits". */ +const AUTOMATION_RATE_WEEKS = 12; + +/** + * Read the trailing window's verdicts and build the series (#9727). + * + * `fetchRows` is overridable so the whole path is testable without a database; it defaults to the real read, + * so the production caller passes only `env` and cannot forget to wire it. + */ +export async function loadAutomationRateSeries( + env: unknown, + options: { + weeks?: number | undefined; + nowMs?: number | undefined; + fetchRows?: ((sinceIso: string) => Promise) | undefined; + } = {}, +): Promise { + const nowMs = options.nowMs ?? Date.now(); + const requested = options.weeks; + const weeks = typeof requested === "number" && Number.isFinite(requested) && requested > 0 ? Math.trunc(requested) : AUTOMATION_RATE_WEEKS; + const sinceIso = new Date(nowMs - weeks * 7 * 86_400_000).toISOString(); + const fetchRows = options.fetchRows ?? ((since: string) => queryAutomationRows(env, since)); + const rows = await fetchRows(sinceIso).catch(() => [] as AutomationVerdictRow[]); + return buildAutomationRateSeries(rows); +} + +async function queryAutomationRows(env: unknown, sinceIso: string): Promise { + return safeAll( + env as never, + `SELECT repo_full_name AS repoFullName, + pull_number AS pullNumber, + action AS action, + created_at AS createdAt, + reevaluation_reason AS reevaluationReason, + reevaluation_actor AS reevaluationActor + FROM decision_records + WHERE created_at >= ?`, + sinceIso, + ); +} diff --git a/test/unit/automation-rate.test.ts b/test/unit/automation-rate.test.ts new file mode 100644 index 0000000000..4ee316ecf0 --- /dev/null +++ b/test/unit/automation-rate.test.ts @@ -0,0 +1,179 @@ +// The published automation-rate definition (#9727). Every figure here is a claim an outsider is invited to +// recompute from the ledger, so these pin the DEFINITION -- especially the places where a plausible shortcut +// would let the rate be inflated. +import { describe, expect, it } from "vitest"; +import { + AUTOMATION_RATE_PROVENANCE_HORIZON_ISO, + buildAutomationRateSeries, + loadAutomationRateSeries, + verdictShowsHumanAction, + weekStartIso, + type AutomationVerdictRow, +} from "../../src/review/automation-rate"; + +function row(over: Partial = {}): AutomationVerdictRow { + return { + repoFullName: "o/r", + pullNumber: 1, + action: "merge", + createdAt: "2026-07-29T12:00:00.000Z", + reevaluationReason: null, + reevaluationActor: null, + ...over, + }; +} + +describe("verdictShowsHumanAction", () => { + it("counts a HOLD as human action — the gate declined to decide", () => { + expect(verdictShowsHumanAction({ action: "hold", reevaluationReason: null, reevaluationActor: null })).toBe(true); + }); + + it("counts a named re-evaluation actor, and a maintainer_request", () => { + expect(verdictShowsHumanAction({ action: "merge", reevaluationReason: null, reevaluationActor: "JSONbored" })).toBe(true); + expect(verdictShowsHumanAction({ action: "merge", reevaluationReason: "maintainer_request", reevaluationActor: null })).toBe(true); + }); + + it("does NOT count machine-paced re-evaluation causes", () => { + // A scheduled sweep or a repair is the automation working, not a human intervening. + for (const reason of ["scheduled_recheck", "pipeline_error", "upstream_state_change", "config_change"]) { + expect(verdictShowsHumanAction({ action: "merge", reevaluationReason: reason, reevaluationActor: null }), reason).toBe(false); + } + }); + + it("treats a blank actor as no actor", () => { + expect(verdictShowsHumanAction({ action: "merge", reevaluationReason: null, reevaluationActor: " " })).toBe(false); + }); +}); + +describe("weekStartIso", () => { + it("returns the UTC MONDAY of the containing week", () => { + expect(weekStartIso("2026-07-29T12:00:00.000Z")).toBe("2026-07-27T00:00:00.000Z"); // Wed -> Mon + expect(weekStartIso("2026-07-27T00:00:00.000Z")).toBe("2026-07-27T00:00:00.000Z"); // Mon -> itself + }); + + it("puts SUNDAY in the week that started six days earlier, not the next one", () => { + // getUTCDay()===0 is the off-by-one every week-bucketing bug is made of. + expect(weekStartIso("2026-08-02T23:59:59.000Z")).toBe("2026-07-27T00:00:00.000Z"); + }); + + it("is null on an unparseable timestamp rather than bucketing it somewhere wrong", () => { + expect(weekStartIso("not-a-date")).toBeNull(); + }); +}); + +describe("buildAutomationRateSeries", () => { + it("counts PULL REQUESTS once, not verdicts", () => { + const series = buildAutomationRateSeries([row({ pullNumber: 1 }), row({ pullNumber: 1 }), row({ pullNumber: 2 })]); + expect(series.decided).toBe(2); + expect(series.automated).toBe(2); + expect(series.automationRatePct).toBe(100); + }); + + it("a PR that was HELD and later merged is MANUAL — the end state is not the question", () => { + // Counting the final disposition would let the rate be inflated by holding everything and merging by hand. + const series = buildAutomationRateSeries([ + row({ pullNumber: 1, action: "hold", createdAt: "2026-07-29T10:00:00.000Z" }), + row({ pullNumber: 1, action: "merge", createdAt: "2026-07-29T11:00:00.000Z" }), + ]); + expect(series.decided).toBe(1); + expect(series.weeks[0]?.manual).toBe(1); + expect(series.automated).toBe(0); + expect(series.automationRatePct).toBe(0); + }); + + it("buckets a PR by its FIRST verdict, so re-evaluations do not migrate it between weeks", () => { + const series = buildAutomationRateSeries([ + row({ pullNumber: 1, createdAt: "2026-08-03T09:00:00.000Z" }), // a later Monday + row({ pullNumber: 1, createdAt: "2026-07-29T09:00:00.000Z" }), // the earlier week + ]); + expect(series.weeks.map((w) => w.weekStart)).toEqual(["2026-07-27T00:00:00.000Z"]); + }); + + it("separates pull numbers by REPO", () => { + const series = buildAutomationRateSeries([row({ repoFullName: "a/b", pullNumber: 1 }), row({ repoFullName: "c/d", pullNumber: 1 })]); + expect(series.decided).toBe(2); + }); + + it("orders weeks chronologically and reports a null rate for a week that decided nothing", () => { + const series = buildAutomationRateSeries([ + row({ pullNumber: 2, createdAt: "2026-08-03T09:00:00.000Z" }), + row({ pullNumber: 1, createdAt: "2026-07-29T09:00:00.000Z" }), + ]); + expect(series.weeks.map((w) => w.weekStart)).toEqual(["2026-07-27T00:00:00.000Z", "2026-08-03T00:00:00.000Z"]); + expect(buildAutomationRateSeries([]).automationRatePct).toBeNull(); + }); + + it("marks weeks before the provenance horizon as holds_only, not silently mixed in", () => { + // Before migration 0204 only `hold` was observable, so those weeks can only UNDER-count manual work. + const before = new Date(Date.parse(AUTOMATION_RATE_PROVENANCE_HORIZON_ISO) - 21 * 86_400_000).toISOString(); + const after = new Date(Date.parse(AUTOMATION_RATE_PROVENANCE_HORIZON_ISO) + 14 * 86_400_000).toISOString(); + const series = buildAutomationRateSeries([row({ createdAt: before }), row({ pullNumber: 2, createdAt: after })]); + expect(series.weeks[0]?.basis).toBe("holds_only"); + expect(series.weeks.at(-1)?.basis).toBe("full"); + expect(series.provenanceHorizon).toBe(AUTOMATION_RATE_PROVENANCE_HORIZON_ISO); + }); + + it("labels a week STRADDLING the horizon holds_only — understating confidence, not overstating it", () => { + // The horizon falls mid-week, so part of that week predates the provenance fields. Calling it `full` + // would claim completeness the data does not have; for a fairness figure the error must run the safe way. + const straddling = new Date(Date.parse(AUTOMATION_RATE_PROVENANCE_HORIZON_ISO) + 6 * 3_600_000).toISOString(); + const series = buildAutomationRateSeries([row({ createdAt: straddling })]); + expect(Date.parse(series.weeks[0]!.weekStart)).toBeLessThan(Date.parse(AUTOMATION_RATE_PROVENANCE_HORIZON_ISO)); + expect(series.weeks[0]?.basis).toBe("holds_only"); + }); + + it("drops a row with an unparseable timestamp rather than bucketing it wrong", () => { + expect(buildAutomationRateSeries([row({ createdAt: "nope" })]).decided).toBe(0); + }); +}); + +describe("loadAutomationRateSeries", () => { + it("windows on the supplied clock and defaults to 12 weeks", async () => { + let asked = ""; + await loadAutomationRateSeries({}, { + nowMs: Date.parse("2026-07-29T00:00:00.000Z"), + fetchRows: async (since) => { + asked = since; + return []; + }, + }); + expect(asked).toBe("2026-05-06T00:00:00.000Z"); // 12 weeks back + }); + + it("rejects a non-positive or fractional window rather than trusting it", async () => { + for (const weeks of [0, -3, Number.NaN, undefined]) { + let asked = ""; + await loadAutomationRateSeries({}, { nowMs: Date.parse("2026-07-29T00:00:00.000Z"), weeks, fetchRows: async (s) => { asked = s; return []; } }); + expect(asked, String(weeks)).toBe("2026-05-06T00:00:00.000Z"); + } + }); + + it("truncates a fractional window rather than rounding it up", async () => { + let asked = ""; + await loadAutomationRateSeries({}, { nowMs: Date.parse("2026-07-29T00:00:00.000Z"), weeks: 2.9, fetchRows: async (s) => { asked = s; return []; } }); + expect(asked, "2.9 weeks is a 2-week window -- never longer than the number asked for").toBe("2026-07-15T00:00:00.000Z"); + }); + + it("degrades when an INJECTED fetcher rejects, not just when the real query does", async () => { + // safeAll swallows the real query's own failures, so this outer guard is what protects a caller that + // supplies its own reader -- untested, it would look fine while being the only unprotected path. + await expect( + loadAutomationRateSeries({}, { fetchRows: async () => { throw new Error("upstream gone"); } }), + ).resolves.toMatchObject({ weeks: [], decided: 0, automationRatePct: null }); + }); + + it("reads through the real query when no fetcher is injected", async () => { + // The production caller passes only `env`; without a default the endpoint would publish an empty series + // forever while every injected-fetcher test above still passed. + let sql = ""; + const env = { DB: { prepare(q: string) { sql = q; return { bind: () => ({ all: async () => ({ results: [] }) }) }; } } }; + await loadAutomationRateSeries(env); + expect(sql).toContain("FROM decision_records"); + expect(sql).toContain("reevaluation_actor"); + }); + + it("degrades to an empty series when the read throws, never failing the stats endpoint", async () => { + const env = { DB: { prepare() { throw new Error("no such column"); } } }; + await expect(loadAutomationRateSeries(env)).resolves.toMatchObject({ weeks: [], decided: 0, automationRatePct: null }); + }); +}); From be8dbbe9f1c7bf3c59c798949fd3129ae3df07d3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:38:27 -0700 Subject: [PATCH 2/2] fix(fairness): exclude never-decided PRs from the automation-rate series The series folded EVERY decision_records row in the window, with no filter on action. `action` also carries non-deciding classes -- `label`, `update_branch`, `approve`, and the error/no-op classes -- none of which is `hold`, so a pull request that only ever drew those carried no human signal and was counted as an AUTOMATED decision. That inflated the published rate with pull requests the gate never decided at all, contradicting the definition published alongside it. A PR now enters the series only when it reached an enacting disposition (`merge`/`close`) or a hold, and is automated only when it enacted one with no human signal. A non-deciding verdict alongside a real one does not flip the PR to manual -- that is the bot acting, not a person, and treating it as manual would under-count automation just as wrongly. The read filters on the same exported constant the fold classifies on, so the WHERE clause and the predicate cannot drift apart. --- src/review/automation-rate.ts | 47 +++++++++++++++++++++----- test/unit/automation-rate.test.ts | 55 ++++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 10 deletions(-) diff --git a/src/review/automation-rate.ts b/src/review/automation-rate.ts index 81f5927750..fc6f8a023e 100644 --- a/src/review/automation-rate.ts +++ b/src/review/automation-rate.ts @@ -6,12 +6,18 @@ // // PUBLISHED DEFINITION. A pull request is counted once per week, by the week of its FIRST verdict. // -// manual ANY verdict for that PR shows a human in the decision path: +// decided The PR reached a disposition at all: at least one verdict whose action ENACTS one +// (`merge` / `close`) or one that HOLDS. `action` also carries non-deciding classes -- +// `label`, `update_branch`, `approve`, and the error/no-op classes -- and a PR that only ever +// drew those was never decided, so it is excluded from BOTH halves rather than counted. +// Counting it would have made a PR the gate never decided read as an automated decision. +// manual A decided PR where ANY verdict shows a human in the decision path: // - `action = 'hold'` -- the gate declined to decide and handed it to a person // - `reevaluation_actor` -- a named person caused a re-evaluation (#9742) // - `reevaluation_reason = 'maintainer_request'` -- a human asked for the re-run -// automated Every verdict was a merge or close with none of the above: opened, decided, enacted, with -// no human action in between. +// automated A PR that reached `merge` or `close` with none of the above: opened, decided, enacted, with +// no human action in between. Non-deciding verdicts alongside (a `label`, an `update_branch`) +// do NOT make it manual -- those are the bot acting, not a person. // // A PR that was held and LATER merged is manual, not automated. The question is whether a human had to act, // not what the end state was -- counting the final disposition would let the rate be inflated by holding @@ -34,6 +40,17 @@ import { safeAll } from "./public-stats"; */ export const AUTOMATION_RATE_PROVENANCE_HORIZON_ISO = "2026-07-29T00:00:00.000Z"; +/** The action classes that ENACT a decision. A PR reaching one of these, with no human signal, is what + * "automated" means. Exported so the read below filters on exactly the set the fold classifies on -- one + * definition, rather than a WHERE clause and a predicate that can drift apart. */ +export const AUTOMATION_ENACTING_ACTIONS = ["merge", "close"] as const; + +/** The action recording that the gate declined to decide and handed the PR to a person. */ +export const AUTOMATION_HOLD_ACTION = "hold"; + +/** Every action that puts a PR in the series at all -- an enacted decision, or a hold. */ +export const AUTOMATION_COUNTED_ACTIONS: readonly string[] = [...AUTOMATION_ENACTING_ACTIONS, AUTOMATION_HOLD_ACTION]; + /** How completely a week could be measured. `full` weeks see every manual signal; `holds_only` weeks predate * the provenance fields and can only under-count manual work. */ export type AutomationWeekBasis = "full" | "holds_only"; @@ -102,28 +119,38 @@ export function buildAutomationRateSeries(rows: readonly AutomationVerdictRow[]) // Fold verdicts to PULL REQUESTS first: the unit of the question is "did a human have to act on this PR", // and a PR with five verdicts is still one PR. Its week is the week of its FIRST verdict, so a PR does not // migrate between weeks as it accrues re-evaluations. - const byPull = new Map(); + const byPull = new Map(); for (const row of rows) { const key = `${row.repoFullName}#${row.pullNumber}`; - const existing = byPull.get(key); const human = verdictShowsHumanAction(row); + const enacted = (AUTOMATION_ENACTING_ACTIONS as readonly string[]).includes(row.action); + const held = row.action === AUTOMATION_HOLD_ACTION; + const existing = byPull.get(key); if (!existing) { - byPull.set(key, { firstSeen: row.createdAt, human }); + byPull.set(key, { firstSeen: row.createdAt, human, enacted, held }); continue; } existing.human = existing.human || human; + existing.enacted = existing.enacted || enacted; + existing.held = existing.held || held; if (Date.parse(row.createdAt) < Date.parse(existing.firstSeen)) existing.firstSeen = row.createdAt; } const horizon = Date.parse(AUTOMATION_RATE_PROVENANCE_HORIZON_ISO); const weeks = new Map(); for (const entry of byPull.values()) { + // Never decided -- only non-deciding verdicts (a label, an update_branch, an error). Excluded from both + // halves: this PR is not evidence for OR against automation, and folding it into `automated` because it + // happens to carry no human signal is exactly the over-count this guard exists to prevent. + if (!entry.enacted && !entry.held) continue; const week = weekStartIso(entry.firstSeen); if (week === null) continue; const bucket = weeks.get(week) ?? { decided: 0, automated: 0, manual: 0 }; bucket.decided += 1; - if (entry.human) bucket.manual += 1; - else bucket.automated += 1; + // Enacted AND clean of every human signal. A held PR later merged by hand fails the second test, and a + // PR that only ever held fails the first -- both are manual, which is the whole point of the metric. + if (entry.enacted && !entry.human) bucket.automated += 1; + else bucket.manual += 1; weeks.set(week, bucket); } @@ -185,7 +212,9 @@ async function queryAutomationRows(env: unknown, sinceIso: string): Promise= ?`, + WHERE created_at >= ? + AND action IN (${AUTOMATION_COUNTED_ACTIONS.map(() => "?").join(", ")})`, sinceIso, + ...AUTOMATION_COUNTED_ACTIONS, ); } diff --git a/test/unit/automation-rate.test.ts b/test/unit/automation-rate.test.ts index 4ee316ecf0..a8cbcdadba 100644 --- a/test/unit/automation-rate.test.ts +++ b/test/unit/automation-rate.test.ts @@ -3,6 +3,7 @@ // would let the rate be inflated. import { describe, expect, it } from "vitest"; import { + AUTOMATION_COUNTED_ACTIONS, AUTOMATION_RATE_PROVENANCE_HORIZON_ISO, buildAutomationRateSeries, loadAutomationRateSeries, @@ -81,6 +82,39 @@ describe("buildAutomationRateSeries", () => { expect(series.automationRatePct).toBe(0); }); + it("EXCLUDES a PR that was never actually decided, rather than counting it automated", () => { + // The regression (#9938 review): `action` also carries non-deciding classes -- `label`, `update_branch`, + // `approve`, and the error/no-op classes. None of them is `hold`, so a PR that only ever drew those + // carried no human signal and was folded in as an AUTOMATED decision, inflating the published rate with + // pull requests the gate never decided at all. + const series = buildAutomationRateSeries([ + row({ pullNumber: 1, action: "label" }), + row({ pullNumber: 2, action: "update_branch" }), + row({ pullNumber: 3, action: "approve" }), + ]); + expect(series.decided).toBe(0); + expect(series.automated).toBe(0); + expect(series.automationRatePct).toBeNull(); + }); + + it("still counts a PR that drew a non-deciding verdict AND then a real one", () => { + // The bot labelling a PR before merging it is the bot acting, not a person -- it must not flip the PR + // to manual, which would under-count automation just as wrongly as the bug over-counted it. + const series = buildAutomationRateSeries([ + row({ pullNumber: 1, action: "label", createdAt: "2026-07-29T10:00:00.000Z" }), + row({ pullNumber: 1, action: "merge", createdAt: "2026-07-29T11:00:00.000Z" }), + ]); + expect(series.decided).toBe(1); + expect(series.automated).toBe(1); + }); + + it("counts a hold-only PR as decided-and-manual, never as undecided", () => { + const series = buildAutomationRateSeries([row({ pullNumber: 1, action: "hold" })]); + expect(series.decided).toBe(1); + expect(series.automated).toBe(0); + expect(series.weeks[0]?.manual).toBe(1); + }); + it("buckets a PR by its FIRST verdict, so re-evaluations do not migrate it between weeks", () => { const series = buildAutomationRateSeries([ row({ pullNumber: 1, createdAt: "2026-08-03T09:00:00.000Z" }), // a later Monday @@ -166,10 +200,29 @@ describe("loadAutomationRateSeries", () => { // The production caller passes only `env`; without a default the endpoint would publish an empty series // forever while every injected-fetcher test above still passed. let sql = ""; - const env = { DB: { prepare(q: string) { sql = q; return { bind: () => ({ all: async () => ({ results: [] }) }) }; } } }; + let binds: unknown[] = []; + const env = { + DB: { + prepare(q: string) { + sql = q; + return { + bind: (...values: unknown[]) => { + binds = values; + return { all: async () => ({ results: [] }) }; + }, + }; + }, + }, + }; await loadAutomationRateSeries(env); expect(sql).toContain("FROM decision_records"); expect(sql).toContain("reevaluation_actor"); + // The action filter is built FROM the same constant the fold classifies on, so the placeholder count and + // the bind count cannot drift apart -- a mismatch is a D1 error at runtime that no injected-rows test + // above would ever reach. + expect(sql).toContain("action IN ("); + expect((sql.match(/\?/g) ?? []).length).toBe(binds.length); + expect(binds.slice(1)).toEqual([...AUTOMATION_COUNTED_ACTIONS]); }); it("degrades to an empty series when the read throws, never failing the stats endpoint", async () => {