From 817f56900a297b8024f5e4de26563330b0fba6f7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:52:01 -0700 Subject: [PATCH 1/6] metrics(ledger): re-evaluation counts and per-author-class review-parity rollups (#9743) Two questions this deployment could only answer with an ad-hoc query now have standing numbers on /v1/public/stats: how often a verdict is re-evaluated and why, and whether a PR faces the same scrutiny regardless of who wrote it. Everything is computed from decision_records -- the anchored ledger -- and nothing else, so an outsider holding the export can recompute every figure. That constraint is why findingsCount is now recorded ON the decision record (v6, migration 0205): findings existed only in ai_review_cache, which is keyed for reuse rather than anchored and is pruned independently, so a fairness number counted from it would have been unverifiable and quietly wrong once the cache turned over. Author class is GitHub's own author_association, not a list this project maintains. That predicate was spelled out at four independent call sites -- one lower-cased, one under a different name -- so it now lives once in github/author-association.ts and they all read it. The definitions that are easy to get wrong are stated beside the code and in the verifier walkthrough: verdicts counts EVALUATIONS not PRs; findingsPerPr excludes verdicts that recorded no count rather than averaging them in as zero, and publishes the coverage it was earned at; an unrecorded association is its own `unknown` bucket rather than folded into either side, because folding it would bias the exact comparison being published; and a rate over an empty window is null, never 0. --- .../content/docs/verify-this-review.mdx | 27 ++ .../0205_decision_record_findings_count.sql | 16 ++ src/api/routes.ts | 9 +- src/github/author-association.ts | 47 +++ src/github/commands.ts | 3 +- src/queue/processors.ts | 4 + src/review/decision-record.ts | 22 +- src/review/review-parity-rollups.ts | 256 +++++++++++++++++ src/rules/advisory.ts | 3 +- src/signals/local-branch.ts | 4 +- src/signals/settings-preview.ts | 3 +- test/integration/public-stats-route.test.ts | 9 + .../backfill-decision-labels-core.test.ts | 2 +- test/unit/decision-record.test.ts | 1 + test/unit/queue-2.test.ts | 6 +- test/unit/review-parity-rollups.test.ts | 272 ++++++++++++++++++ 16 files changed, 668 insertions(+), 16 deletions(-) create mode 100644 migrations/0205_decision_record_findings_count.sql create mode 100644 src/github/author-association.ts create mode 100644 src/review/review-parity-rollups.ts create mode 100644 test/unit/review-parity-rollups.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 333bb6ce02..d9a2296831 100644 --- a/apps/loopover-ui/content/docs/verify-this-review.mdx +++ b/apps/loopover-ui/content/docs/verify-this-review.mdx @@ -130,6 +130,33 @@ this site, so fetch it absolutely — a bare `/v1/public/stats` resolves against curl -s "https://api.loopover.ai/v1/public/stats" | jq '.rulePrecision' ``` +### Re-evaluation counts and author-class parity + +The same endpoint publishes `reviewParity` (#9743): how often a verdict was re-evaluated and why, and +whether a pull request faced the same scrutiny regardless of who wrote it. + +```bash +curl -s "https://api.loopover.ai/v1/public/stats" | jq '.reviewParity' +``` + +Every figure in that block is computed from `decision_records` alone — the anchored ledger — so you can +recompute all of it from an export without trusting this endpoint. The definitions are published beside +the code that implements them, in `src/review/review-parity-rollups.ts`. The three that are easy to get +wrong, and that are therefore stated explicitly: + +- **`verdicts` counts evaluations, not pull requests.** A repeat evaluation of the same head SHA is its + own ledger row, which is exactly what `reviewsPerPr` divides out. +- **`findingsPerPr` excludes verdicts that recorded no finding count**, rather than averaging them in as + zero. `findingsBasis` beside it is the coverage the mean was earned at — a mean over 3 of 400 verdicts + is not the same claim as a mean over 400. +- **Author class is GitHub's own `author_association`**, not a list this project maintains. A PR whose + association was never recorded is reported as `unknown` in its own bucket rather than folded into + either side, because folding it would bias the exact comparison being published. + +A rate over an empty window is `null`, never `0` — "nothing was held" and "nothing was measured" are +different claims, and the fairness report renders the second as *measured zero* with its window rather +than as a reassuring number. + 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/migrations/0205_decision_record_findings_count.sql b/migrations/0205_decision_record_findings_count.sql new file mode 100644 index 0000000000..a5d9f7f55a --- /dev/null +++ b/migrations/0205_decision_record_findings_count.sql @@ -0,0 +1,16 @@ +-- Findings raised per evaluation (#9743). +-- +-- The per-author-class parity rollups publish "findings raised per PR" beside the block/close and hold +-- rates. Those numbers are a fairness claim, so they have to be reproducible by an outsider from the +-- anchored ledger alone -- and findings were only ever stored in `ai_review_cache.findings_json`, which is +-- keyed for REUSE rather than anchored and is pruned independently. Counting a published fairness figure +-- out of a cache would make it unverifiable and, worse, quietly wrong once the cache turned over. +-- +-- NULLABLE, and null for every row written before this: a caller with no findings to report (a policy +-- close, an update_branch) is genuinely different from an evaluation that raised zero, and the rollups +-- treat the two differently rather than averaging a guess. +ALTER TABLE decision_records ADD COLUMN findings_count INTEGER; + +-- Answers "findings per PR, per author class, per window" without scanning the whole table. +CREATE INDEX IF NOT EXISTS decision_records_findings + ON decision_records (created_at, findings_count); diff --git a/src/api/routes.ts b/src/api/routes.ts index c67839c292..13b4c71562 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -13,6 +13,7 @@ import { isScreenshotsEnabled } from "../review/visual-wire"; 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 { GITHUB_OAUTH_STATE_COOKIE, authenticateInternalToken, @@ -671,7 +672,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] = await Promise.all([ + const [stats, accuracyTrend, fleetAccuracyTrend, reuseRateTrend, reviewVolumeTrend, rulePrecision, reviewParity] = 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 @@ -683,9 +684,13 @@ export function createApp() { // #8230: measured per-rule precision + the reproducibility freeze point. Same flag, same cache, // same one-surface posture as the sibling trends. loadPublicRulePrecision(c.env), + // #9743: re-evaluation counts by declared reason, and the per-author-class parity rollups. Computed + // 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), ]); c.header("Cache-Control", "public, max-age=60, stale-while-revalidate=300"); - return c.json({ ...stats, accuracyTrend, fleetAccuracyTrend, reuseRateTrend, reviewVolumeTrend, rulePrecision }); + return c.json({ ...stats, accuracyTrend, fleetAccuracyTrend, reuseRateTrend, reviewVolumeTrend, rulePrecision, reviewParity }); } catch { return c.json({ error: "public_stats_unavailable" }, 503); } diff --git a/src/github/author-association.ts b/src/github/author-association.ts new file mode 100644 index 0000000000..f29ffb6cf2 --- /dev/null +++ b/src/github/author-association.ts @@ -0,0 +1,47 @@ +// GitHub's `author_association` vocabulary, and the one place this repo decides what it means. +// +// The value comes straight from GitHub on every issue/PR/comment payload and reflects the author's real +// standing in the repository, so it is a MECHANICAL classification: nothing here is a maintained list of +// people, and adding or removing a maintainer upstream changes the answer with no code change. That is +// what makes the per-author-class parity rollups (#9743) reproducible by an outsider -- they can recompute +// the same split from the same public field. +// +// This set was previously spelled out at four independent call sites (the mention-command gate, the +// settings preview, the advisory rules, and local-branch's owner check), one of them lower-cased and one +// of them under a different name. Four copies of a security-relevant predicate is four chances to drift, +// so they now all read this. + +/** + * The associations that mean the author has standing in the repo rather than being a drive-by contributor. + * + * `OWNER`/`MEMBER`/`COLLABORATOR` only. Deliberately NOT `CONTRIBUTOR`, which GitHub gives to anyone with + * a previously-merged PR -- that is a history of contribution, not authority over the repo. + * + * Note this is GitHub's *display* association and is not by itself proof of push access (see the note in + * mcp/server.ts): any surface that will MUTATE something must verify live permissions instead. It is + * exactly right for classification and reporting, which is what it is used for here. + */ +export const MAINTAINER_AUTHOR_ASSOCIATIONS: readonly string[] = ["OWNER", "MEMBER", "COLLABORATOR"]; + +/** + * How a PR is counted in the parity rollups. + * + * `unknown` is its own class rather than being folded into `contributor`: a PR whose association we never + * recorded is not evidence of anything, and silently counting it as contributor-authored would bias the + * very comparison the rollups exist to make. + */ +export type AuthorClass = "maintainer" | "contributor" | "unknown"; + +/** True when the association is one this repo treats as having standing. Case-insensitive: GitHub sends + * upper-case, but one existing call site stored it lower-cased. */ +export function isMaintainerAuthorAssociation(association: string | null | undefined): boolean { + if (typeof association !== "string") return false; + const normalized = association.trim().toUpperCase(); + return MAINTAINER_AUTHOR_ASSOCIATIONS.includes(normalized); +} + +/** The author class for a recorded association. Absent/blank is `unknown`, never a guess. */ +export function classifyAuthorAssociation(association: string | null | undefined): AuthorClass { + if (typeof association !== "string" || association.trim() === "") return "unknown"; + return isMaintainerAuthorAssociation(association) ? "maintainer" : "contributor"; +} diff --git a/src/github/commands.ts b/src/github/commands.ts index 0dd3f63f81..5db591c8a7 100644 --- a/src/github/commands.ts +++ b/src/github/commands.ts @@ -25,6 +25,7 @@ import { } from "../signals/engine"; import { isFailingCheckSummary } from "../signals/check-summary"; import { buildMaintainerNoiseReport, type MaintainerNoiseReport } from "../signals/reward-risk"; +import { MAINTAINER_AUTHOR_ASSOCIATIONS } from "./author-association"; const PUBLIC_MENTION_COMMAND_CATALOG = [ { id: "help", title: "LoopOver command help", description: "Show public-safe @loopover command help." }, @@ -175,7 +176,7 @@ export type AgentCommandFeedbackContext = { const COMMANDS = new Set(LOOPOVER_MENTION_COMMAND_CATALOG.map((command) => command.id)); const ACTION_COMMANDS = new Set(LOOPOVER_ACTION_COMMANDS); const MAINTAINER_QUEUE_DIGEST_COMMANDS = new Set(MAINTAINER_QUEUE_DIGEST_COMMAND_CATALOG.map((command) => command.id)); -const MAINTAINER_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); +const MAINTAINER_ASSOCIATIONS = new Set(MAINTAINER_AUTHOR_ASSOCIATIONS); const AGENT_COMMAND_FEEDBACK_MARKER = "gittensory-agent-command-answer"; const COMMAND_TITLES = Object.fromEntries(LOOPOVER_MENTION_COMMAND_CATALOG.map((command) => [command.id, command.title])) as Record; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index dc8d374be7..2eaa1d55f8 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3894,6 +3894,10 @@ async function runAgentMaintenancePlanAndExecute( salvageability, // #9135: legible on the record's own face — see maybeApplyCloseAuditHoldout's doc comment. divertedByHoldout: closeAuditHoldout?.diverted ?? false, + // #9743: what this evaluation actually raised, anchored on the record so "findings per PR" in the + // parity rollups is reproducible from the ledger rather than from the (unanchored, prunable) AI + // review cache. Blockers and warnings together -- both are findings a contributor had to read. + findingsCount: gate.blockers.length + gate.warnings.length, }); // #9742: a repeat verdict for the SAME head SHA must declare why. The cause is derived from this // job's own delivery id -- the scheduled sweep, a repair fan-out, an operator's manual re-gate, diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index 6855079a48..c7699bafe9 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -50,7 +50,7 @@ import { errorMessage, nowIso } from "../utils/json"; import { retentionCutoffIsoForTable } from "../db/retention"; /** Bump when the record's FIELD SET changes meaning — consumers compare records only within a version. */ -export const DECISION_RECORD_SCHEMA_VERSION = "5"; // v5 (#8834): + aiAgreement (inter-run agreement folded with the verbalized confidence); v4 (#9124/#9135): configDigest digests the resolved policy (+ settingsDigest split out), promptDigest digests the actual sent prompt, modelId -> modelIds (real identities), ciState populated, + divertedByHoldout; v3 (#8962): + salvageability {score, factors}; v2 (#8834): + aiConfidence, model/prompt commitments +export const DECISION_RECORD_SCHEMA_VERSION = "6"; // v6 (#9743): + findingsCount, so "findings raised per PR" is derivable from the LEDGER rather than from the AI review cache (which is keyed for reuse, not anchored, and so cannot back a reproducibility claim); v5 (#8834): + aiAgreement (inter-run agreement folded with the verbalized confidence); v4 (#9124/#9135): configDigest digests the resolved policy (+ settingsDigest split out), promptDigest digests the actual sent prompt, modelId -> modelIds (real identities), ciState populated, + divertedByHoldout; v3 (#8962): + salvageability {score, factors}; v2 (#8834): + aiConfidence, model/prompt commitments /** * Canonical JSON: recursively key-sorted, no insignificant whitespace — the ONE serialization every digest @@ -146,13 +146,19 @@ export type DecisionRecord = { * records disagree on outcome. false for every decision the holdout never touched, including every * decision recorded before #9135 shipped (via `buildDecisionRecord`'s normalization below). */ divertedByHoldout: boolean; + /** #9743: how many findings this evaluation actually raised (blockers + warnings). Recorded HERE so the + * per-author-class parity rollups are reproducible from the anchored ledger alone -- the AI review cache + * also holds findings, but it is keyed for reuse rather than anchored, so counting from it would make a + * published fairness number unverifiable. Null for a caller that has no findings to report (a policy + * close, an update_branch), which is distinct from a genuine zero. */ + findingsCount: number | null; decidedAt: string; }; /** Assemble the record and its own content digest. PURE given pre-computed digests. Normalizes the * optional-shaped caller fields (undefined -> null) HERE so call sites carry no fallback arms of their own. */ export async function buildDecisionRecord( - input: Omit & { + input: Omit & { decidedAt?: string; gatePack?: string | null | undefined; ciState?: string | null | undefined; @@ -162,6 +168,7 @@ export async function buildDecisionRecord( salvageability?: { score: number; factors: string[] } | null | undefined; settingsDigest?: string | null | undefined; divertedByHoldout?: boolean | undefined; + findingsCount?: number | null | undefined; }, ): Promise<{ record: DecisionRecord; recordDigest: string }> { const record: DecisionRecord = { @@ -176,6 +183,11 @@ export async function buildDecisionRecord( salvageability: input.salvageability ?? null, settingsDigest: input.settingsDigest ?? null, divertedByHoldout: input.divertedByHoldout ?? false, + // Non-negative integer or null; a fractional or negative count is a caller bug, not a fact to publish. + findingsCount: + typeof input.findingsCount === "number" && Number.isInteger(input.findingsCount) && input.findingsCount >= 0 + ? input.findingsCount + : null, }; return { record, recordDigest: await contentDigest(record) }; } @@ -328,10 +340,10 @@ export async function persistDecisionRecord( const id = priorCount === 0 ? baseId : `${baseId}:rev${priorCount + 1}`; try { await env.DB.prepare( - `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at, reevaluation_reason, supersedes_record_id, reevaluation_actor) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at, reevaluation_reason, supersedes_record_id, reevaluation_actor, findings_count) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) - .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt, reevaluated?.reason ?? null, reevaluated?.supersedesRecordId ?? null, reevaluated?.actor ?? null) + .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt, reevaluated?.reason ?? null, reevaluated?.supersedesRecordId ?? null, reevaluated?.actor ?? null, record.findingsCount) .run(); } catch (error) { if (attempt >= attempts) throw error; diff --git a/src/review/review-parity-rollups.ts b/src/review/review-parity-rollups.ts new file mode 100644 index 0000000000..b82555e89c --- /dev/null +++ b/src/review/review-parity-rollups.ts @@ -0,0 +1,256 @@ +// Re-evaluation counts and per-author-class review-parity rollups (#9743). +// +// Two questions this deployment should be able to answer with standing numbers rather than an ad-hoc query: +// +// 1. How often is a verdict re-evaluated, and why? (#9742 made every repeat verdict declare a reason; this +// counts them by that reason, as a share of all verdicts.) +// 2. Does a PR face the same scrutiny regardless of who wrote it? +// +// PUBLISHED DEFINITIONS. Everything below is computed from `decision_records` -- the anchored ledger -- and +// nothing else, so an outsider holding the same export can recompute every figure here and get the same +// answer. That is the whole point of the exercise; a number nobody can check is not evidence of fairness. +// +// verdicts One row in `decision_records`. A repeat evaluation of the same head SHA is its own +// row (#9123), so this counts EVALUATIONS, not pull requests. +// reevaluations Verdicts carrying a `reevaluation_reason` -- i.e. every verdict past the first for +// a head SHA. A first evaluation has no reason by construction and is never counted. +// reviewsPerPr verdicts / distinct (repo, pull). >1 means heads were re-evaluated. +// findingsPerPr Mean `findings_count` over verdicts that RECORDED one. Verdicts with a null count +// (a policy close, an update_branch, or any row written before migration 0205) are +// excluded from both halves of the mean rather than counted as zero -- see +// `findingsBasis` for the coverage that mean was earned at. +// closeRate Share of verdicts whose action is `close`. +// holdRate Share of verdicts whose action is `hold`. +// +// AUTHOR CLASS is `pull_requests.author_association`, GitHub's own field, mapped by +// `classifyAuthorAssociation` (github/author-association.ts). Mechanical: no maintained list of people, and +// a permissions change upstream changes the class with no code change here. A PR whose association was +// never recorded is `unknown` and is reported as its own class rather than folded into either side -- the +// comparison is the entire product, so quietly bucketing unknowns would bias exactly the thing being +// measured. + +import { classifyAuthorAssociation, type AuthorClass } from "../github/author-association"; +import { safeAll } from "./public-stats"; + +/** Every author class, in report order. Exported so a caller cannot invent a fifth bucket. */ +export const AUTHOR_CLASSES: readonly AuthorClass[] = ["maintainer", "contributor", "unknown"]; + +/** One verdict, reduced to just the fields the rollups read. */ +export type ParityVerdictRow = { + authorClass: AuthorClass; + repoFullName: string; + pullNumber: number; + action: string; + /** Null when this verdict recorded no finding count -- NOT the same as zero findings. */ + findingsCount: number | null; + /** Null for a first evaluation of a head SHA. */ + reevaluationReason: string | null; +}; + +export type ParityRollup = { + authorClass: AuthorClass; + /** Evaluations, not pull requests. */ + verdicts: number; + /** Distinct (repo, pull) behind those verdicts. */ + pullRequests: number; + reviewsPerPr: number | null; + findingsPerPr: number | null; + /** How many verdicts actually carried a finding count -- the coverage `findingsPerPr` was earned at. + * A mean over 3 of 400 verdicts is not the same claim as a mean over 400, and publishing the two + * identically is how an honest-looking number misleads. */ + findingsBasis: number; + closeRate: number | null; + holdRate: number | null; +}; + +export type ReevaluationReasonCount = { + reason: string; + count: number; + /** Share of ALL verdicts in the window, not of re-evaluations -- "3% of verdicts were re-run for a + * pipeline error" is the readable claim; a share of re-evaluations would always sum to 100% and say + * nothing about how often re-evaluation happens at all. */ + shareOfVerdictsPct: number | null; +}; + +export type ReviewParityRollups = { + /** Inclusive-exclusive ISO bounds the whole payload was computed over. */ + windowStart: string; + windowEnd: string; + verdicts: number; + reevaluations: number; + /** Share of verdicts that were a re-evaluation. Null when the window holds no verdicts -- an undefined + * ratio, reported as such rather than as a reassuring 0%. */ + reevaluationRatePct: number | null; + /** Descending by count, then reason for a stable order. Reasons with zero occurrences are omitted. */ + byReason: ReevaluationReasonCount[]; + /** One entry per class that has at least one verdict, in AUTHOR_CLASSES order. */ + byAuthorClass: ParityRollup[]; + /** Per-repo granularity, busiest first. Same shape, scoped to one repo. */ + byProject: Array<{ project: string; byAuthorClass: ParityRollup[] }>; +}; + +/** Percent to one decimal, or null when the denominator is zero. A rate with no denominator is unknown, + * never 0 -- the distinction is the difference between "nothing was held" and "nothing was measured". */ +export function ratePct(numerator: number, denominator: number): number | null { + if (denominator <= 0) return null; + return Math.round((numerator / denominator) * 1000) / 10; +} + +/** Mean to two decimals, or null when nothing contributed. */ +function mean(total: number, count: number): number | null { + if (count <= 0) return null; + return Math.round((total / count) * 100) / 100; +} + +/** Roll one class's verdicts into the published shape. PURE, so the definitions above are testable + * directly against a table of rows rather than only through a database. */ +export function rollUpVerdicts(authorClass: AuthorClass, rows: readonly ParityVerdictRow[]): ParityRollup { + const pulls = new Set(); + let closes = 0; + let holds = 0; + let findingsTotal = 0; + let findingsBasis = 0; + for (const row of rows) { + pulls.add(`${row.repoFullName}#${row.pullNumber}`); + if (row.action === "close") closes += 1; + if (row.action === "hold") holds += 1; + if (row.findingsCount !== null) { + findingsTotal += row.findingsCount; + findingsBasis += 1; + } + } + return { + authorClass, + verdicts: rows.length, + pullRequests: pulls.size, + reviewsPerPr: mean(rows.length, pulls.size), + findingsPerPr: mean(findingsTotal, findingsBasis), + findingsBasis, + closeRate: ratePct(closes, rows.length), + holdRate: ratePct(holds, rows.length), + }; +} + +/** Group rows by author class, in report order, omitting classes with no verdicts. */ +export function rollUpByAuthorClass(rows: readonly ParityVerdictRow[]): ParityRollup[] { + return AUTHOR_CLASSES.map((authorClass) => rollUpVerdicts(authorClass, rows.filter((row) => row.authorClass === authorClass))).filter( + (rollup) => rollup.verdicts > 0, + ); +} + +/** + * The whole published payload, from a window's verdict rows. PURE. + * + * Kept separate from the query so every definition above can be tested against a hand-written table -- + * which is also what lets an outsider check our arithmetic without reproducing our database. + */ +export function buildReviewParityRollups(input: { + windowStart: string; + windowEnd: string; + rows: readonly ParityVerdictRow[]; +}): ReviewParityRollups { + const { rows } = input; + // A type PREDICATE, not a plain filter: it narrows `reevaluationReason` to `string` for the tally below, + // which is what removes the re-check the tally would otherwise need and could never fail. + const reevaluated = rows.filter((row): row is ParityVerdictRow & { reevaluationReason: string } => row.reevaluationReason !== null); + + const counts = new Map(); + for (const row of reevaluated) { + counts.set(row.reevaluationReason, (counts.get(row.reevaluationReason) ?? 0) + 1); + } + + const byProject = new Map(); + for (const row of rows) { + const bucket = byProject.get(row.repoFullName); + if (bucket) bucket.push(row); + else byProject.set(row.repoFullName, [row]); + } + + return { + windowStart: input.windowStart, + windowEnd: input.windowEnd, + verdicts: rows.length, + reevaluations: reevaluated.length, + reevaluationRatePct: ratePct(reevaluated.length, rows.length), + byReason: [...counts.entries()] + .map(([reason, count]) => ({ reason, count, shareOfVerdictsPct: ratePct(count, rows.length) })) + .sort((a, b) => (b.count === a.count ? a.reason.localeCompare(b.reason) : b.count - a.count)), + byAuthorClass: rollUpByAuthorClass(rows), + byProject: [...byProject.entries()] + .map(([project, projectRows]) => ({ project, byAuthorClass: rollUpByAuthorClass(projectRows) })) + .sort((a, b) => { + const aVerdicts = a.byAuthorClass.reduce((sum, rollup) => sum + rollup.verdicts, 0); + const bVerdicts = b.byAuthorClass.reduce((sum, rollup) => sum + rollup.verdicts, 0); + return bVerdicts === aVerdicts ? a.project.localeCompare(b.project) : bVerdicts - aVerdicts; + }), + }; +} + +/** + * Read one window's verdicts and roll them up (#9743). + * + * The join is `decision_records` LEFT JOIN `pull_requests` on (repo, number): LEFT so a verdict whose PR + * row has since been pruned still counts toward the totals as `unknown` rather than vanishing -- dropping + * it would quietly shrink the denominator of a fairness figure. + */ +export async function loadReviewParityRollups( + env: unknown, + options: { + windowDays?: number | undefined; + nowMs?: number | undefined; + /** Injected so the whole path is testable without a database; defaults to the real read. */ + fetchRows?: ((sinceIso: string, untilIso: string) => Promise) | undefined; + } = {}, +): Promise { + const nowMs = options.nowMs ?? Date.now(); + // Narrowed on the local rather than re-reading the option, so there is no `?? 0` arm that cannot fire + // and no cast: a non-number, non-finite, or non-positive window all fall back to the 7-day default. + const requested = options.windowDays; + const windowDays = typeof requested === "number" && Number.isFinite(requested) && requested > 0 ? Math.trunc(requested) : 7; + const windowEnd = new Date(nowMs).toISOString(); + const windowStart = new Date(nowMs - windowDays * 86_400_000).toISOString(); + + const fetchRows = options.fetchRows ?? ((since: string, until: string) => queryParityRows(env, since, until)); + const raw = await fetchRows(windowStart, windowEnd).catch(() => [] as RawParityRow[]); + + return buildReviewParityRollups({ + windowStart, + windowEnd, + rows: raw.map((row) => ({ + authorClass: classifyAuthorAssociation(row.authorAssociation), + repoFullName: row.repoFullName, + pullNumber: row.pullNumber, + action: row.action, + findingsCount: typeof row.findingsCount === "number" ? row.findingsCount : null, + reevaluationReason: typeof row.reevaluationReason === "string" && row.reevaluationReason !== "" ? row.reevaluationReason : null, + })), + }); +} + +/** The row shape the query returns, before classification. */ +export type RawParityRow = { + repoFullName: string; + pullNumber: number; + action: string; + findingsCount: number | null; + reevaluationReason: string | null; + authorAssociation: string | null; +}; + +async function queryParityRows(env: unknown, sinceIso: string, untilIso: string): Promise { + return safeAll( + env as never, + `SELECT dr.repo_full_name AS repoFullName, + dr.pull_number AS pullNumber, + dr.action AS action, + dr.findings_count AS findingsCount, + dr.reevaluation_reason AS reevaluationReason, + pr.author_association AS authorAssociation + FROM decision_records dr + LEFT JOIN pull_requests pr + ON pr.repo_full_name = dr.repo_full_name AND pr.number = dr.pull_number + WHERE dr.created_at >= ? AND dr.created_at < ?`, + sinceIso, + untilIso, + ); +} diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 923b6a7fb0..eb3049988e 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -49,6 +49,7 @@ import { CLA_CHECK_UNRESOLVED_CODE, CLA_CONSENT_MISSING_CODE } from "../review/c import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings"; import { createSignalStore } from "../review/signal-tracking-wire"; import { labelMatchesPattern } from "../scoring/preview"; +import { isMaintainerAuthorAssociation } from "../github/author-association"; export type GateCheckConclusion = "success" | "failure" | "action_required" | "neutral" | "skipped"; @@ -1170,7 +1171,7 @@ function addPullRequestFindings( detail: `Matched configured labels: ${matchedLabels.join(", ")}.`, }); } - if (pr.authorAssociation && ["OWNER", "MEMBER", "COLLABORATOR"].includes(pr.authorAssociation)) { + if (isMaintainerAuthorAssociation(pr.authorAssociation)) { findings.push({ code: "maintainer_authored_pr", severity: "info", diff --git a/src/signals/local-branch.ts b/src/signals/local-branch.ts index 1da0fc0ed5..2391b6ee5a 100644 --- a/src/signals/local-branch.ts +++ b/src/signals/local-branch.ts @@ -30,6 +30,7 @@ import { scenarioInputFromLocalBranchMetadata } from "../scenarios/input-model"; import { renderPublicScenarioSummary, type PublicScenarioSummary, type ScenarioSummaryInput } from "../scenarios/scenario-summary"; import { simulateOpenPrPressure } from "../services/open-pr-pressure-scenarios"; import { isCodeFile, isTestFile } from "./path-matchers"; +import { isMaintainerAuthorAssociation } from "../github/author-association"; export type LocalBranchChangedFile = { path: string; @@ -768,8 +769,7 @@ function hasPendingCheck(checks: CheckSummaryRecord[]): boolean { } function isMaintainerAuthoredPr(pr: PullRequestRecord, repo: RepositoryRecord | undefined, login: string): boolean { - /* v8 ignore next -- Missing association is a defensive GitHub row fallback; observed association behavior is covered above. */ - return sameLogin(repo?.owner, login) || ["owner", "member", "collaborator"].includes((pr.authorAssociation ?? "").toLowerCase()); + return sameLogin(repo?.owner, login) || isMaintainerAuthorAssociation(pr.authorAssociation); } function isStaleOpenPr(pr: PullRequestRecord, nowMs: number | undefined): boolean { diff --git a/src/signals/settings-preview.ts b/src/signals/settings-preview.ts index ba962c65a8..bdaad0df9e 100644 --- a/src/signals/settings-preview.ts +++ b/src/signals/settings-preview.ts @@ -23,6 +23,7 @@ import { decideReviewEligibility } from "../review/review-eligibility"; import { buildUnifiedCommentBody } from "../review/unified-comment-bridge"; import { requiredAgentActionPermissions } from "../settings/agent-execution"; import { isAgentConfigured } from "../settings/autonomy"; +import { isMaintainerAuthorAssociation } from "../github/author-association"; function hasVisiblePrSurface(settings: RepositorySettings): boolean { return settings.publicSurface !== "off" || settings.checkRunMode === "enabled" || shouldPublishReviewCheck(settings.reviewCheckMode); @@ -135,7 +136,7 @@ export function decidePublicSurface(input: PublicSurfaceDecisionInput): PublicSu if (!input.authorLogin) return skipDecision("missing_author"); if (input.authorType === "Bot" || /\[bot\]$/i.test(input.authorLogin)) return skipDecision("bot_author"); if (!decideReviewEligibility({ authorLogin: input.authorLogin, ignoreAuthors: input.ignoredAuthorPatterns }).eligible) return skipDecision("ignored_author"); - if (!settings.includeMaintainerAuthors && input.authorAssociation && ["OWNER", "MEMBER", "COLLABORATOR"].includes(input.authorAssociation)) { + if (!settings.includeMaintainerAuthors && isMaintainerAuthorAssociation(input.authorAssociation)) { return skipDecision("maintainer_author"); } if (settings.publicAudienceMode === "gittensor_only") { diff --git a/test/integration/public-stats-route.test.ts b/test/integration/public-stats-route.test.ts index 2060fef730..b67e2117ad 100644 --- a/test/integration/public-stats-route.test.ts +++ b/test/integration/public-stats-route.test.ts @@ -102,6 +102,15 @@ describe("GET /v1/public/stats (#1059)", () => { expect(withPrecision.rulePrecision.reversals).toEqual({ reopened: 1, reverted: 0, superseded: 0 }); expect(withPrecision.rulePrecision.latestBacktestRun).toBeNull(); + // #9743: the re-evaluation counts + per-author-class parity rollups ride the same surface. An empty + // ledger publishes a MEASURED-ZERO window with its own bounds, never a missing key -- /fairness renders + // the difference, and a reader cannot tell "nothing happened" from "nothing was computed" without it. + const withParity = (await res.clone().json()) as { + reviewParity: { windowStart: string; windowEnd: string; verdicts: number; reevaluations: number; reevaluationRatePct: number | null; byReason: unknown[]; byAuthorClass: unknown[]; byProject: unknown[] }; + }; + expect(withParity.reviewParity).toMatchObject({ verdicts: 0, reevaluations: 0, reevaluationRatePct: null, byReason: [], byAuthorClass: [], byProject: [] }); + expect(Date.parse(withParity.reviewParity.windowStart)).toBeLessThan(Date.parse(withParity.reviewParity.windowEnd)); + const body = (await res.json()) as { totals: Record; weekly: { reviewed: number; merged: number }; diff --git a/test/unit/backfill-decision-labels-core.test.ts b/test/unit/backfill-decision-labels-core.test.ts index 83520a8639..fbe3d1e975 100644 --- a/test/unit/backfill-decision-labels-core.test.ts +++ b/test/unit/backfill-decision-labels-core.test.ts @@ -115,7 +115,7 @@ describe("buildBundle", () => { expect(await contentDigest(record)).toBe(row.record_digest); expect(canonicalJson(record)).toBe(row.record_json); expect(record.configDigest).toBe("backfill:unavailable"); - expect(record.schemaVersion).toBe("5"); // v5 (#8834) — bumped past the v3 salvageability shape this test targets + expect(record.schemaVersion).toBe("6"); // v6 (#9743) — bumped past the v3 salvageability shape this test targets } expect(records[0]).toMatchObject({ id: "record:o/r#1@sha1", action: "close", created_at: "2026-07-01T00:00:00.000Z" }); expect(records[1]).toMatchObject({ action: "hold" }); diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index 654e7d9564..2fb78c2d16 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -72,6 +72,7 @@ function recordInput(over: Partial = {}): Omit { // The decision record carries the boundary evidence. const record = await env.DB.prepare("select record_json from decision_records where repo_full_name = ? and pull_number = ?").bind("owner/agent-repo", 18).first<{ record_json: string }>(); const parsedRecord = JSON.parse(record!.record_json) as { schemaVersion: string; salvageability: { score: number; factors: string[] } | null }; - expect(parsedRecord.schemaVersion).toBe("5"); // v5 (#8834) — bumped past the v3 salvageability shape this test targets + expect(parsedRecord.schemaVersion).toBe("6"); // v6 (#9743) — bumped past the v3 salvageability shape this test targets expect(parsedRecord.salvageability?.score).toBe(70); expect(parsedRecord.salvageability?.factors.join(" ")).toContain("mechanical defect class"); // #8838: the replay input persisted beside the record, and the decision re-derives bit-exactly from it. @@ -357,7 +357,7 @@ describe("queue processors", () => { settingsDigest: string | null; divertedByHoldout: boolean; }; - expect(parsed.schemaVersion).toBe("5"); + expect(parsed.schemaVersion).toBe("6"); expect(parsed.action).toBe("close"); // above-floor confidence, no salvageability config -> a one-shot close DECISION // The exact requirement: modelId is non-null whenever an AI judgment shaped the decision. expect(parsed.modelIds).toEqual(["claude-code", "codex"]); @@ -694,7 +694,7 @@ describe("queue processors", () => { // decision record — the row every future calibration read keys on. const record = await env.DB.prepare("select record_json from decision_records where repo_full_name = 'owner/agent-repo' and pull_number = 9 order by created_at desc limit 1").first<{ record_json: string }>(); const parsedRecord = JSON.parse(record!.record_json) as { aiConfidence: number | null; promptDigest: string | null; modelIds: string[] | null; schemaVersion: string }; - expect(parsedRecord.schemaVersion).toBe("5"); // v4 (#9124/#9135): configDigest/promptDigest/modelIds/ciState + divertedByHoldout + expect(parsedRecord.schemaVersion).toBe("6"); // v6 (#9743) + findingsCount; v4 (#9124/#9135): configDigest/promptDigest/modelIds/ciState + divertedByHoldout expect(parsedRecord.aiConfidence).toBe(0.3); // the cached sub-floor defect's calibrated confidence expect(parsedRecord.promptDigest).toBe("f".repeat(64)); expect(parsedRecord.modelIds).toEqual(["claude-code"]); diff --git a/test/unit/review-parity-rollups.test.ts b/test/unit/review-parity-rollups.test.ts new file mode 100644 index 0000000000..f55746be89 --- /dev/null +++ b/test/unit/review-parity-rollups.test.ts @@ -0,0 +1,272 @@ +// The published fairness definitions (#9743). +// +// Every figure here is a claim an outsider is invited to recompute from the ledger export, so these tests +// pin the DEFINITIONS -- especially the places where a plausible-looking shortcut would quietly bias the +// comparison the rollups exist to make. +import { describe, expect, it } from "vitest"; +import { + AUTHOR_CLASSES, + buildReviewParityRollups, + loadReviewParityRollups, + ratePct, + rollUpByAuthorClass, + rollUpVerdicts, + type ParityVerdictRow, + type RawParityRow, +} from "../../src/review/review-parity-rollups"; +import { classifyAuthorAssociation, isMaintainerAuthorAssociation, MAINTAINER_AUTHOR_ASSOCIATIONS } from "../../src/github/author-association"; + +function row(over: Partial = {}): ParityVerdictRow { + return { + authorClass: "contributor", + repoFullName: "o/r", + pullNumber: 1, + action: "merge", + findingsCount: 0, + reevaluationReason: null, + ...over, + }; +} + +describe("author class derivation (#9743)", () => { + it("maps GitHub's own association, case-insensitively", () => { + for (const association of MAINTAINER_AUTHOR_ASSOCIATIONS) { + expect(classifyAuthorAssociation(association), association).toBe("maintainer"); + expect(classifyAuthorAssociation(association.toLowerCase()), association).toBe("maintainer"); + } + expect(classifyAuthorAssociation("CONTRIBUTOR")).toBe("contributor"); + expect(classifyAuthorAssociation("FIRST_TIME_CONTRIBUTOR")).toBe("contributor"); + expect(classifyAuthorAssociation("NONE")).toBe("contributor"); + }); + + it("treats CONTRIBUTOR as a contributor — a merged PR in the past is not authority over the repo", () => { + expect(isMaintainerAuthorAssociation("CONTRIBUTOR")).toBe(false); + }); + + it("reports an unrecorded association as UNKNOWN rather than folding it into either side", () => { + // Folding unknowns into `contributor` would bias exactly the comparison being published. + for (const value of [null, undefined, "", " "]) { + expect(classifyAuthorAssociation(value), JSON.stringify(value)).toBe("unknown"); + } + expect(AUTHOR_CLASSES).toEqual(["maintainer", "contributor", "unknown"]); + }); +}); + +describe("ratePct", () => { + it("is null on a zero denominator — unknown is not 0%", () => { + // "nothing was held" and "nothing was measured" are different claims; a 0 here would assert the first. + expect(ratePct(0, 0)).toBeNull(); + expect(ratePct(1, 2)).toBe(50); + expect(ratePct(1, 3)).toBe(33.3); + }); +}); + +describe("rollUpVerdicts", () => { + it("counts EVALUATIONS but divides by distinct pull requests", () => { + const rollup = rollUpVerdicts("contributor", [ + row({ pullNumber: 1 }), + row({ pullNumber: 1 }), + row({ pullNumber: 2 }), + ]); + expect(rollup.verdicts).toBe(3); + expect(rollup.pullRequests).toBe(2); + expect(rollup.reviewsPerPr).toBe(1.5); + }); + + it("separates pull numbers by REPO, so two repos' #1 are not one PR", () => { + const rollup = rollUpVerdicts("contributor", [row({ repoFullName: "a/b", pullNumber: 1 }), row({ repoFullName: "c/d", pullNumber: 1 })]); + expect(rollup.pullRequests).toBe(2); + }); + + it("EXCLUDES null finding counts from the mean instead of counting them as zero", () => { + // A policy close records no findings. Averaging it in as 0 would understate how many findings a real + // review raised, which is the number being compared across author classes. + const rollup = rollUpVerdicts("contributor", [row({ findingsCount: 4 }), row({ findingsCount: null }), row({ findingsCount: 2 })]); + expect(rollup.findingsPerPr).toBe(3); + expect(rollup.findingsBasis, "the coverage the mean was earned at is published beside it").toBe(2); + }); + + it("reports a mean of null, not 0, when no verdict recorded a count", () => { + const rollup = rollUpVerdicts("maintainer", [row({ findingsCount: null })]); + expect(rollup.findingsPerPr).toBeNull(); + expect(rollup.findingsBasis).toBe(0); + }); + + it("computes close and hold rates over ALL verdicts, including merges", () => { + const rollup = rollUpVerdicts("contributor", [row({ action: "close" }), row({ action: "hold" }), row({ action: "merge" }), row({ action: "merge" })]); + expect(rollup.closeRate).toBe(25); + expect(rollup.holdRate).toBe(25); + }); + + it("reports every rate as null for an empty class rather than a reassuring zero", () => { + const rollup = rollUpVerdicts("maintainer", []); + expect(rollup).toMatchObject({ verdicts: 0, pullRequests: 0, reviewsPerPr: null, findingsPerPr: null, closeRate: null, holdRate: null }); + }); +}); + +describe("rollUpByAuthorClass", () => { + it("keeps report order and omits classes with no verdicts", () => { + const rollups = rollUpByAuthorClass([row({ authorClass: "unknown" }), row({ authorClass: "maintainer" })]); + expect(rollups.map((r) => r.authorClass)).toEqual(["maintainer", "unknown"]); + }); +}); + +describe("buildReviewParityRollups", () => { + const window = { windowStart: "2026-07-22T00:00:00.000Z", windowEnd: "2026-07-29T00:00:00.000Z" }; + + it("counts re-evaluations by declared reason, as a share of ALL verdicts", () => { + // Share-of-verdicts, not share-of-re-evaluations: the latter always sums to 100% and says nothing + // about how often re-evaluation happens at all. + const result = buildReviewParityRollups({ + ...window, + rows: [ + row({ reevaluationReason: "scheduled_recheck" }), + row({ reevaluationReason: "scheduled_recheck" }), + row({ reevaluationReason: "pipeline_error" }), + row(), + ], + }); + expect(result.verdicts).toBe(4); + expect(result.reevaluations).toBe(3); + expect(result.reevaluationRatePct).toBe(75); + expect(result.byReason).toEqual([ + { reason: "scheduled_recheck", count: 2, shareOfVerdictsPct: 50 }, + { reason: "pipeline_error", count: 1, shareOfVerdictsPct: 25 }, + ]); + }); + + it("orders equal reason counts by name, so the series is stable between runs", () => { + const result = buildReviewParityRollups({ + ...window, + rows: [row({ reevaluationReason: "pipeline_error" }), row({ reevaluationReason: "config_change" })], + }); + expect(result.byReason.map((entry) => entry.reason)).toEqual(["config_change", "pipeline_error"]); + }); + + it("reports an EMPTY window as measured-zero with its bounds, never as a missing series", () => { + const result = buildReviewParityRollups({ ...window, rows: [] }); + expect(result).toMatchObject({ verdicts: 0, reevaluations: 0, reevaluationRatePct: null, byReason: [], byAuthorClass: [], byProject: [] }); + expect(result.windowStart).toBe(window.windowStart); + expect(result.windowEnd).toBe(window.windowEnd); + }); + + it("splits per project, busiest first, each with its own class breakdown", () => { + const result = buildReviewParityRollups({ + ...window, + rows: [ + row({ repoFullName: "busy/repo", pullNumber: 1 }), + row({ repoFullName: "busy/repo", pullNumber: 2 }), + row({ repoFullName: "quiet/repo", pullNumber: 1, authorClass: "maintainer" }), + ], + }); + expect(result.byProject.map((entry) => entry.project)).toEqual(["busy/repo", "quiet/repo"]); + expect(result.byProject[1]?.byAuthorClass.map((r) => r.authorClass)).toEqual(["maintainer"]); + }); + + it("breaks ties between equally busy projects by name", () => { + const result = buildReviewParityRollups({ ...window, rows: [row({ repoFullName: "b/b" }), row({ repoFullName: "a/a" })] }); + expect(result.byProject.map((entry) => entry.project)).toEqual(["a/a", "b/b"]); + }); +}); + +describe("loadReviewParityRollups", () => { + const raw = (over: Partial = {}): RawParityRow => ({ + repoFullName: "o/r", + pullNumber: 1, + action: "merge", + findingsCount: 1, + reevaluationReason: null, + authorAssociation: "CONTRIBUTOR", + ...over, + }); + + it("windows on the supplied clock and classifies each row's association", async () => { + let asked: [string, string] | null = null; + const result = await loadReviewParityRollups( + {}, + { + nowMs: Date.parse("2026-07-29T00:00:00.000Z"), + windowDays: 7, + fetchRows: async (since, until) => { + asked = [since, until]; + return [raw(), raw({ authorAssociation: "OWNER" })]; + }, + }, + ); + expect(asked).toEqual(["2026-07-22T00:00:00.000Z", "2026-07-29T00:00:00.000Z"]); + expect(result.byAuthorClass.map((r) => r.authorClass)).toEqual(["maintainer", "contributor"]); + }); + + it("defaults to a 7-day window and rejects a non-positive or fractional one", async () => { + for (const windowDays of [undefined, 0, -3, Number.NaN]) { + const result = await loadReviewParityRollups({}, { nowMs: Date.parse("2026-07-29T00:00:00.000Z"), windowDays, fetchRows: async () => [] }); + expect(result.windowStart, String(windowDays)).toBe("2026-07-22T00:00:00.000Z"); + } + const truncated = await loadReviewParityRollups({}, { nowMs: Date.parse("2026-07-29T00:00:00.000Z"), windowDays: 2.9, fetchRows: async () => [] }); + expect(truncated.windowStart).toBe("2026-07-27T00:00:00.000Z"); + }); + + it("normalizes a blank re-evaluation reason to null, so it is not counted as a cause", async () => { + const result = await loadReviewParityRollups({}, { fetchRows: async () => [raw({ reevaluationReason: "" })] }); + expect(result.reevaluations).toBe(0); + }); + + it("normalizes a non-numeric finding count to null rather than trusting it", async () => { + const result = await loadReviewParityRollups({}, { fetchRows: async () => [raw({ findingsCount: null })] }); + expect(result.byAuthorClass[0]?.findingsPerPr).toBeNull(); + }); + + it("degrades to a measured-zero window when the read throws, never failing the stats endpoint", async () => { + // /v1/public/stats composes several series; one unavailable table must not take the whole payload down. + const result = await loadReviewParityRollups({}, { + fetchRows: async () => { + throw new Error("no such table: decision_records"); + }, + }); + expect(result).toMatchObject({ verdicts: 0, reevaluations: 0, byAuthorClass: [] }); + }); +}); + +describe("loadReviewParityRollups default paths", () => { + it("reads through the real query when no fetcher is injected", async () => { + // The production caller passes only `env`. If the default were missing, every injected-fetcher test + // above would still pass while the endpoint published an empty series forever. + let asked = ""; + const env = { + DB: { + prepare(sql: string) { + asked = sql; + return { + bind: () => ({ all: async () => ({ results: [{ repoFullName: "o/r", pullNumber: 1, action: "hold", findingsCount: 2, reevaluationReason: "scheduled_recheck", authorAssociation: "OWNER" }] }) }), + }; + }, + }, + }; + + const result = await loadReviewParityRollups(env, { nowMs: Date.parse("2026-07-29T00:00:00.000Z") }); + + expect(asked, "it really queries the anchored ledger").toContain("FROM decision_records"); + expect(asked, "LEFT JOIN so a pruned PR row still counts rather than vanishing").toContain("LEFT JOIN pull_requests"); + expect(result.byAuthorClass).toEqual([ + expect.objectContaining({ authorClass: "maintainer", verdicts: 1, holdRate: 100, findingsPerPr: 2, findingsBasis: 1 }), + ]); + }); + + it("degrades to an empty window when the real query throws", async () => { + const env = { + DB: { + prepare() { + throw new Error("no such column: findings_count"); + }, + }, + }; + await expect(loadReviewParityRollups(env)).resolves.toMatchObject({ verdicts: 0, byAuthorClass: [] }); + }); + + it("defaults the clock to now when no nowMs is supplied", async () => { + const before = Date.now(); + const result = await loadReviewParityRollups({}, { fetchRows: async () => [] }); + expect(Date.parse(result.windowEnd)).toBeGreaterThanOrEqual(before); + expect(Date.parse(result.windowEnd)).toBeLessThanOrEqual(Date.now()); + }); +}); From a5dee18699947f4c1d31cd1764d546226a64ee7c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:58:07 -0700 Subject: [PATCH 2/6] ui(fairness): publish the re-evaluation and review-parity series (#9744) Both series #9743 computes now render on /fairness, with the methodology stated where the numbers are rather than only in the docs. The zero states are the point. A window with no verdicts renders as a MEASURED zero with its own dates -- "not missing data" in as many words -- and "no re-evaluations recorded" is a separate, separately-worded state from "no verdicts recorded", because they are different facts and a reader cannot tell them apart otherwise. A mean with no basis reads as insufficient data, never as 0, and every published mean carries the count it was earned at beside it. The payload shape is added to the contract's zod schema, so the UI type is DERIVED rather than hand-kept: adding the block made both existing fixtures fail to compile, which is exactly the guard #9282/#9521 put there. --- apps/loopover-ui/public/openapi.json | 172 ++++++++++++++++++ .../site/fairness-report-page.test.tsx | 113 +++++++++++- .../components/site/fairness-report-page.tsx | 163 +++++++++++++++++ .../site/proof-of-power-stats.test.tsx | 10 + packages/loopover-contract/src/public-api.ts | 47 +++++ 5 files changed, 504 insertions(+), 1 deletion(-) diff --git a/apps/loopover-ui/public/openapi.json b/apps/loopover-ui/public/openapi.json index e0157d1e49..28e8d2aed6 100644 --- a/apps/loopover-ui/public/openapi.json +++ b/apps/loopover-ui/public/openapi.json @@ -828,6 +828,177 @@ "accuracyPct" ] } + }, + "reviewParity": { + "type": "object", + "properties": { + "windowStart": { + "type": "string" + }, + "windowEnd": { + "type": "string" + }, + "verdicts": { + "type": "number" + }, + "reevaluations": { + "type": "number" + }, + "reevaluationRatePct": { + "type": "number", + "nullable": true + }, + "byReason": { + "type": "array", + "items": { + "type": "object", + "properties": { + "reason": { + "type": "string" + }, + "count": { + "type": "number" + }, + "shareOfVerdictsPct": { + "type": "number", + "nullable": true + } + }, + "required": [ + "reason", + "count", + "shareOfVerdictsPct" + ] + } + }, + "byAuthorClass": { + "type": "array", + "items": { + "type": "object", + "properties": { + "authorClass": { + "type": "string", + "enum": [ + "maintainer", + "contributor", + "unknown" + ] + }, + "verdicts": { + "type": "number" + }, + "pullRequests": { + "type": "number" + }, + "reviewsPerPr": { + "type": "number", + "nullable": true + }, + "findingsPerPr": { + "type": "number", + "nullable": true + }, + "findingsBasis": { + "type": "number" + }, + "closeRate": { + "type": "number", + "nullable": true + }, + "holdRate": { + "type": "number", + "nullable": true + } + }, + "required": [ + "authorClass", + "verdicts", + "pullRequests", + "reviewsPerPr", + "findingsPerPr", + "findingsBasis", + "closeRate", + "holdRate" + ] + } + }, + "byProject": { + "type": "array", + "items": { + "type": "object", + "properties": { + "project": { + "type": "string" + }, + "byAuthorClass": { + "type": "array", + "items": { + "type": "object", + "properties": { + "authorClass": { + "type": "string", + "enum": [ + "maintainer", + "contributor", + "unknown" + ] + }, + "verdicts": { + "type": "number" + }, + "pullRequests": { + "type": "number" + }, + "reviewsPerPr": { + "type": "number", + "nullable": true + }, + "findingsPerPr": { + "type": "number", + "nullable": true + }, + "findingsBasis": { + "type": "number" + }, + "closeRate": { + "type": "number", + "nullable": true + }, + "holdRate": { + "type": "number", + "nullable": true + } + }, + "required": [ + "authorClass", + "verdicts", + "pullRequests", + "reviewsPerPr", + "findingsPerPr", + "findingsBasis", + "closeRate", + "holdRate" + ] + } + } + }, + "required": [ + "project", + "byAuthorClass" + ] + } + } + }, + "required": [ + "windowStart", + "windowEnd", + "verdicts", + "reevaluations", + "reevaluationRatePct", + "byReason", + "byAuthorClass", + "byProject" + ] } }, "required": [ @@ -836,6 +1007,7 @@ "totals", "weekly", "rulePrecision", + "reviewParity", "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 a53611c884..0b7e744f56 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,16 @@ 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. + reviewParity: { + windowStart: "2026-07-22T00:00:00.000Z", + windowEnd: "2026-07-29T00:00:00.000Z", + verdicts: 0, + reevaluations: 0, + reevaluationRatePct: null, + byReason: [], + byAuthorClass: [], + byProject: [], + }, rulePrecision: { windowDays: 90, rules: [], @@ -123,7 +133,9 @@ describe("FairnessReportPage (#fairness-analytics)", () => { expect(screen.getByText(/Reproducibility freeze point/)).toBeTruthy(); expect(screen.getByText(/aaaaaaaaaaaaaaaa…/)).toBeTruthy(); // And the walkthrough link points at the docs page. - expect(screen.getByRole("link", { name: /verify this review/i })).toBeTruthy(); + // Both the per-rule precision block and the review-parity block (#9744) invite the reader to + // reproduce their numbers, so there is deliberately more than one of these links. + expect(screen.getAllByRole("link", { name: /verify this review/i }).length).toBeGreaterThan(0); }); it("hides the per-rule section entirely when the API response predates rulePrecision (deployment skew) or has no rules (#8231)", async () => { @@ -398,4 +410,103 @@ describe("FairnessReportPage (#fairness-analytics)", () => { ).toContain("—"); expect(screen.queryByRole("alert")).toBeNull(); }); + + // #9744: the two series #9743 computes, and the zero-state conventions they must honour. + function parityFixture(over: Record) { + return { + ok: true, + durationMs: 10, + data: { + ...FIXTURE, + reviewParity: { + windowStart: "2026-07-22T00:00:00.000Z", + windowEnd: "2026-07-29T00:00:00.000Z", + verdicts: 0, + reevaluations: 0, + reevaluationRatePct: null, + byReason: [], + byAuthorClass: [], + byProject: [], + ...over, + }, + }, + }; + } + + it("renders the re-evaluation reason table and the author-class parity table", async () => { + apiFetch.mockResolvedValue( + parityFixture({ + verdicts: 10, + reevaluations: 3, + reevaluationRatePct: 30, + byReason: [{ reason: "scheduled_recheck", count: 3, shareOfVerdictsPct: 30 }], + byAuthorClass: [ + { + authorClass: "maintainer", + verdicts: 4, + pullRequests: 4, + reviewsPerPr: 1, + findingsPerPr: 2, + findingsBasis: 4, + closeRate: 0, + holdRate: 25, + }, + { + authorClass: "contributor", + verdicts: 6, + pullRequests: 3, + reviewsPerPr: 2, + findingsPerPr: null, + findingsBasis: 0, + closeRate: 50, + holdRate: 0, + }, + ], + }), + ); + renderWithClient(); + + expect(await screen.findByText(/Re-evaluation and review parity/i)).toBeTruthy(); + expect(screen.getByText("scheduled_recheck")).toBeTruthy(); + expect(screen.getByText(/3 of 10 verdicts were re-evaluations/i)).toBeTruthy(); + expect(screen.getByText("maintainer")).toBeTruthy(); + expect(screen.getByText("contributor")).toBeTruthy(); + // The coverage a mean was earned at is published beside it, and an absent mean reads as + // insufficient data rather than as 0. + expect(screen.getByText(/n=4/)).toBeTruthy(); + expect(screen.getAllByText(/insufficient data/i).length).toBeGreaterThan(0); + }); + + it("renders an EMPTY window as a measured zero with its dates, not as missing data", async () => { + apiFetch.mockResolvedValue(parityFixture({})); + renderWithClient(); + + expect(await screen.findByText(/No verdicts recorded/i)).toBeTruthy(); + expect(screen.getByText(/measured zero over the dates above, not missing data/i)).toBeTruthy(); + }); + + it("distinguishes 'no re-evaluations' from 'no verdicts' — both are measured zeros, not the same one", async () => { + apiFetch.mockResolvedValue( + parityFixture({ + verdicts: 5, + reevaluationRatePct: 0, + byAuthorClass: [ + { + authorClass: "contributor", + verdicts: 5, + pullRequests: 5, + reviewsPerPr: 1, + findingsPerPr: 1, + findingsBasis: 5, + closeRate: 20, + holdRate: 0, + }, + ], + }), + ); + renderWithClient(); + + expect(await screen.findByText(/No re-evaluations recorded/i)).toBeTruthy(); + expect(screen.queryByText(/No verdicts recorded/i)).toBeNull(); + }); }); 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 e8eb3a6597..d025c6cd74 100644 --- a/apps/loopover-ui/src/components/site/fairness-report-page.tsx +++ b/apps/loopover-ui/src/components/site/fairness-report-page.tsx @@ -423,6 +423,169 @@ export function FairnessReportPage() { ) : null} ) : 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. */} + {data.reviewParity ? ( +
+

Re-evaluation and review parity

+

+ Every verdict is written to an append-only ledger, and a repeat verdict for the + same commit has to declare why. This is how often that happened, and whether a + pull request faced the same scrutiny regardless of who wrote it. Computed from the + ledger alone over {new Date(data.reviewParity.windowStart).toLocaleDateString()} –{" "} + {new Date(data.reviewParity.windowEnd).toLocaleDateString()}, so you can recompute + all of it yourself:{" "} + + verify this review + + . +

+ + {data.reviewParity.verdicts === 0 ? ( +

+ No verdicts recorded in + this window — a measured zero over the dates above, not missing data. +

+ ) : ( + <> +

+ {intFmt.format(data.reviewParity.reevaluations)} of{" "} + {intFmt.format(data.reviewParity.verdicts)} verdicts were re-evaluations + {data.reviewParity.reevaluationRatePct != null + ? ` (${pctFmt.format(data.reviewParity.reevaluationRatePct)}%)` + : ""} + . +

+ {data.reviewParity.byReason.length > 0 ? ( + + + + + + + + + + + + {data.reviewParity.byReason.map((entry) => ( + + + + + + ))} + +
+ Re-evaluations by declared reason, and each reason's share of all + verdicts. +
+ Reason + + Count + + Share of verdicts +
+ {entry.reason} + {intFmt.format(entry.count)} + {entry.shareOfVerdictsPct != null + ? `${pctFmt.format(entry.shareOfVerdictsPct)}%` + : "—"} +
+ + ) : ( +

+ + No re-evaluations recorded + {" "} + in this window — every verdict was a first evaluation. +

+ )} + +

By author class

+

+ Author class is GitHub's own{" "} + author_association, not a list this project + maintains. Reviews counts evaluations, not pull requests. Findings per PR is a + mean over the verdicts that recorded a count — the count behind it is shown, + because a mean over three cases is not the same claim as a mean over four + hundred. An unrecorded association is its own row rather than folded into + either side. +

+ + + + + + + + + + + + + + + {data.reviewParity.byAuthorClass.map((rollup) => ( + + + + + + + + + ))} + +
+ Reviews, findings, close rate and hold rate per author class. +
+ Author + + PRs + + Reviews / PR + + Findings / PR + + Close rate + + Hold rate +
{rollup.authorClass}{intFmt.format(rollup.pullRequests)} + {rollup.reviewsPerPr != null + ? pctFmt.format(rollup.reviewsPerPr) + : "—"} + + {rollup.findingsPerPr != null ? ( + <> + {pctFmt.format(rollup.findingsPerPr)}{" "} + + (n={intFmt.format(rollup.findingsBasis)}) + + + ) : ( + insufficient data + )} + + {rollup.closeRate != null + ? `${pctFmt.format(rollup.closeRate)}%` + : "—"} + + {rollup.holdRate != null + ? `${pctFmt.format(rollup.holdRate)}%` + : "—"} +
+ + + )} +
+ ) : null} ) : null} 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 77d80653e0..2c763ca733 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,16 @@ 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. + reviewParity: { + windowStart: "2026-07-22T00:00:00.000Z", + windowEnd: "2026-07-29T00:00:00.000Z", + verdicts: 0, + reevaluations: 0, + reevaluationRatePct: null, + byReason: [], + byAuthorClass: [], + byProject: [], + }, rulePrecision: { windowDays: 90, rules: [], diff --git a/packages/loopover-contract/src/public-api.ts b/packages/loopover-contract/src/public-api.ts index cd5410f8d2..8be0164a08 100644 --- a/packages/loopover-contract/src/public-api.ts +++ b/packages/loopover-contract/src/public-api.ts @@ -30,6 +30,50 @@ export const PublicRulePrecisionSchema = z.object({ export type PublicRulePrecision = z.infer; +/** + * Re-evaluation counts and per-author-class review-parity rollups (#9743). + * + * Every figure is computed from `decision_records` alone -- the anchored ledger -- so a reader can + * recompute all of it from an export. The definitions live beside the implementation in + * src/review/review-parity-rollups.ts and are restated in the verifier walkthrough; the three that are easy + * to misread are called out on the fields themselves below. + */ +export const ParityRollupSchema = z.object({ + authorClass: z.enum(["maintainer", "contributor", "unknown"]), + /** EVALUATIONS, not pull requests: a repeat evaluation of one head SHA is its own ledger row. */ + verdicts: z.number(), + pullRequests: z.number(), + reviewsPerPr: z.number().nullable(), + /** Mean over verdicts that RECORDED a count; verdicts with none are excluded rather than averaged in + * as zero. Read it together with `findingsBasis`. */ + findingsPerPr: z.number().nullable(), + /** The coverage `findingsPerPr` was earned at -- a mean over 3 of 400 verdicts is not the same claim + * as a mean over 400. */ + findingsBasis: z.number(), + closeRate: z.number().nullable(), + holdRate: z.number().nullable(), +}); + +export const ReviewParitySchema = z.object({ + windowStart: z.string(), + windowEnd: z.string(), + verdicts: z.number(), + reevaluations: z.number(), + /** Null when the window holds no verdicts -- an undefined ratio, never a reassuring 0%. */ + reevaluationRatePct: z.number().nullable(), + byReason: z.array( + z.object({ + reason: z.string(), + count: z.number(), + /** Share of ALL verdicts, not of re-evaluations: the latter always sums to 100% and says nothing + * about how often re-evaluation happens at all. */ + shareOfVerdictsPct: z.number().nullable(), + }), + ), + byAuthorClass: z.array(ParityRollupSchema), + byProject: z.array(z.object({ project: z.string(), byAuthorClass: z.array(ParityRollupSchema) })), +}); + export const PublicStatsSchema = z.object({ generatedAt: z.string(), updatedAt: z.string(), @@ -49,6 +93,7 @@ export const PublicStatsSchema = z.object({ }), weekly: z.object({ reviewed: z.number(), merged: z.number() }), rulePrecision: PublicRulePrecisionSchema, + reviewParity: ReviewParitySchema, byProject: z.array( z.object({ project: z.string(), @@ -140,3 +185,5 @@ export const PublicStatsSchema = z.object({ }); export type PublicStats = z.infer; +export type ReviewParity = z.infer; +export type ParityRollup = z.infer; From 72e41fb8b18af515b6fe589d90d195123d5134da Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:22:52 -0700 Subject: [PATCH 3/6] refactor(engine): move the author-association vocabulary into the engine package The gate-advisory TWIN carried a FIFTH copy of the OWNER/MEMBER/COLLABORATOR predicate, and the engine is standalone -- it cannot import out of src/. So the definition lives in @loopover/engine and src/github/author-association.ts re-exports it, the same shape settings/pr-type-label.ts already uses for exactly this reason. Both twins now read one definition. --- .../src/advisory/gate-advisory.ts | 3 +- .../src/settings/author-association.ts | 47 +++++++++++++++++ src/github/author-association.ts | 52 +++---------------- 3 files changed, 55 insertions(+), 47 deletions(-) create mode 100644 packages/loopover-engine/src/settings/author-association.ts diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index dd47b4adb4..cb76e758c5 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -24,6 +24,7 @@ import { LOOPOVER_GATE_CHECK_NAME } from "../review/check-names.js"; import { CLA_CHECK_UNRESOLVED_CODE, CLA_CONSENT_MISSING_CODE } from "../review/cla-check.js"; import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings.js"; import { labelMatchesPattern } from "../scoring/label-match.js"; +import { isMaintainerAuthorAssociation } from "../settings/author-association.js"; // Kept byte-identical with the GATE_DECISION_TWIN_PAIR copy in src/rules/advisory.ts // (checkGateDecisionForbiddenTermsParity now diffs the two regex bodies byte-for-byte, #8697). The @@ -394,7 +395,7 @@ function addPullRequestFindings( detail: `Matched configured labels: ${matchedLabels.join(", ")}.`, }); } - if (pr.authorAssociation && ["OWNER", "MEMBER", "COLLABORATOR"].includes(pr.authorAssociation)) { + if (isMaintainerAuthorAssociation(pr.authorAssociation)) { findings.push({ code: "maintainer_authored_pr", severity: "info", diff --git a/packages/loopover-engine/src/settings/author-association.ts b/packages/loopover-engine/src/settings/author-association.ts new file mode 100644 index 0000000000..f29ffb6cf2 --- /dev/null +++ b/packages/loopover-engine/src/settings/author-association.ts @@ -0,0 +1,47 @@ +// GitHub's `author_association` vocabulary, and the one place this repo decides what it means. +// +// The value comes straight from GitHub on every issue/PR/comment payload and reflects the author's real +// standing in the repository, so it is a MECHANICAL classification: nothing here is a maintained list of +// people, and adding or removing a maintainer upstream changes the answer with no code change. That is +// what makes the per-author-class parity rollups (#9743) reproducible by an outsider -- they can recompute +// the same split from the same public field. +// +// This set was previously spelled out at four independent call sites (the mention-command gate, the +// settings preview, the advisory rules, and local-branch's owner check), one of them lower-cased and one +// of them under a different name. Four copies of a security-relevant predicate is four chances to drift, +// so they now all read this. + +/** + * The associations that mean the author has standing in the repo rather than being a drive-by contributor. + * + * `OWNER`/`MEMBER`/`COLLABORATOR` only. Deliberately NOT `CONTRIBUTOR`, which GitHub gives to anyone with + * a previously-merged PR -- that is a history of contribution, not authority over the repo. + * + * Note this is GitHub's *display* association and is not by itself proof of push access (see the note in + * mcp/server.ts): any surface that will MUTATE something must verify live permissions instead. It is + * exactly right for classification and reporting, which is what it is used for here. + */ +export const MAINTAINER_AUTHOR_ASSOCIATIONS: readonly string[] = ["OWNER", "MEMBER", "COLLABORATOR"]; + +/** + * How a PR is counted in the parity rollups. + * + * `unknown` is its own class rather than being folded into `contributor`: a PR whose association we never + * recorded is not evidence of anything, and silently counting it as contributor-authored would bias the + * very comparison the rollups exist to make. + */ +export type AuthorClass = "maintainer" | "contributor" | "unknown"; + +/** True when the association is one this repo treats as having standing. Case-insensitive: GitHub sends + * upper-case, but one existing call site stored it lower-cased. */ +export function isMaintainerAuthorAssociation(association: string | null | undefined): boolean { + if (typeof association !== "string") return false; + const normalized = association.trim().toUpperCase(); + return MAINTAINER_AUTHOR_ASSOCIATIONS.includes(normalized); +} + +/** The author class for a recorded association. Absent/blank is `unknown`, never a guess. */ +export function classifyAuthorAssociation(association: string | null | undefined): AuthorClass { + if (typeof association !== "string" || association.trim() === "") return "unknown"; + return isMaintainerAuthorAssociation(association) ? "maintainer" : "contributor"; +} diff --git a/src/github/author-association.ts b/src/github/author-association.ts index f29ffb6cf2..6dfe3eb3ea 100644 --- a/src/github/author-association.ts +++ b/src/github/author-association.ts @@ -1,47 +1,7 @@ -// GitHub's `author_association` vocabulary, and the one place this repo decides what it means. +// Re-export of the engine's author-association vocabulary. // -// The value comes straight from GitHub on every issue/PR/comment payload and reflects the author's real -// standing in the repository, so it is a MECHANICAL classification: nothing here is a maintained list of -// people, and adding or removing a maintainer upstream changes the answer with no code change. That is -// what makes the per-author-class parity rollups (#9743) reproducible by an outsider -- they can recompute -// the same split from the same public field. -// -// This set was previously spelled out at four independent call sites (the mention-command gate, the -// settings preview, the advisory rules, and local-branch's owner check), one of them lower-cased and one -// of them under a different name. Four copies of a security-relevant predicate is four chances to drift, -// so they now all read this. - -/** - * The associations that mean the author has standing in the repo rather than being a drive-by contributor. - * - * `OWNER`/`MEMBER`/`COLLABORATOR` only. Deliberately NOT `CONTRIBUTOR`, which GitHub gives to anyone with - * a previously-merged PR -- that is a history of contribution, not authority over the repo. - * - * Note this is GitHub's *display* association and is not by itself proof of push access (see the note in - * mcp/server.ts): any surface that will MUTATE something must verify live permissions instead. It is - * exactly right for classification and reporting, which is what it is used for here. - */ -export const MAINTAINER_AUTHOR_ASSOCIATIONS: readonly string[] = ["OWNER", "MEMBER", "COLLABORATOR"]; - -/** - * How a PR is counted in the parity rollups. - * - * `unknown` is its own class rather than being folded into `contributor`: a PR whose association we never - * recorded is not evidence of anything, and silently counting it as contributor-authored would bias the - * very comparison the rollups exist to make. - */ -export type AuthorClass = "maintainer" | "contributor" | "unknown"; - -/** True when the association is one this repo treats as having standing. Case-insensitive: GitHub sends - * upper-case, but one existing call site stored it lower-cased. */ -export function isMaintainerAuthorAssociation(association: string | null | undefined): boolean { - if (typeof association !== "string") return false; - const normalized = association.trim().toUpperCase(); - return MAINTAINER_AUTHOR_ASSOCIATIONS.includes(normalized); -} - -/** The author class for a recorded association. Absent/blank is `unknown`, never a guess. */ -export function classifyAuthorAssociation(association: string | null | undefined): AuthorClass { - if (typeof association !== "string" || association.trim() === "") return "unknown"; - return isMaintainerAuthorAssociation(association) ? "maintainer" : "contributor"; -} +// The definition lives in @loopover/engine because the gate-advisory TWIN +// (packages/loopover-engine/src/advisory/gate-advisory.ts) needs it too, and the engine is a standalone +// published package that cannot import back out of src/. Same shape as settings/pr-type-label.ts, which +// re-exports its engine counterpart for exactly this reason. +export * from "../../packages/loopover-engine/src/settings/author-association"; From 9f086268d601a9d87b141377a28435dd7fda0690 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:48:58 -0700 Subject: [PATCH 4/6] fix(ledger): never bind an undefined findings count persistDecisionRecord bound record.findingsCount raw. A caller that assembles a DecisionRecord literal rather than going through buildDecisionRecord leaves it undefined, D1 refuses undefined as a bind, and the insert threw straight into the outer catch -- so the row silently never appeared and the ledger simply had fewer decisions in it, with nothing surfaced anywhere. Caught by proof-summary's own fixtures, which build records that way. The bind coalesces to null now (the column is nullable and undefined is never a valid bind), with a regression test that fails if the coalesce is removed. --- src/review/decision-record.ts | 2 +- test/unit/decision-record.test.ts | 36 +++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index c7699bafe9..4208351316 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -343,7 +343,7 @@ export async function persistDecisionRecord( `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at, reevaluation_reason, supersedes_record_id, reevaluation_actor, findings_count) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) - .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt, reevaluated?.reason ?? null, reevaluated?.supersedesRecordId ?? null, reevaluated?.actor ?? null, record.findingsCount) + .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt, reevaluated?.reason ?? null, reevaluated?.supersedesRecordId ?? null, reevaluated?.actor ?? null, record.findingsCount ?? null) .run(); } catch (error) { if (attempt >= attempts) throw error; diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index 2fb78c2d16..f2dcde217c 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -940,6 +940,42 @@ describe("verdict immutability per head SHA (#9742)", () => { expect(isReevaluationReason(undefined)).toBe(false); }); + it("REGRESSION: a record built WITHOUT buildDecisionRecord still writes (#9743)", async () => { + // findingsCount used to be bound raw. A caller that assembles a DecisionRecord literal rather than + // going through buildDecisionRecord leaves it `undefined`, D1 refuses `undefined` as a bind, and the + // insert threw straight into the outer catch -- so the row silently never appeared and the ledger + // just... had fewer decisions in it. Nothing surfaced. Caught live by proof-summary's fixtures. + const env = createTestEnv(); + const literal = { + schemaVersion: DECISION_RECORD_SCHEMA_VERSION, + repoFullName: "o/r", + pullNumber: 9, + headSha: "abc1234def", + baseSha: null, + action: "merge", + reasonCode: "clean", + configDigest: "c", + settingsDigest: "s", + gatePack: "oss-anti-slop", + ciState: "success", + modelIds: null, + promptDigest: null, + aiConfidence: null, + aiAgreement: null, + salvageability: null, + divertedByHoldout: false, + decidedAt: "2026-07-29T00:00:00.000Z", + } as never; + + const id = await persistDecisionRecord(env, literal, "d".repeat(64)); + + expect(id, "the write must succeed, not swallow an undefined bind").not.toBeNull(); + const row = await env.DB.prepare("SELECT findings_count AS findingsCount FROM decision_records WHERE id = ?") + .bind(id) + .first<{ findingsCount: number | null }>(); + expect(row?.findingsCount, "an unstated count is NULL, which is not the same as zero findings").toBeNull(); + }); + it("records WHO asked, when a person did, and nothing when a schedule did", async () => { const env = createTestEnv(); const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); From 2c445d34db137e2ec8e393bfbbe0655525ed8ecc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:15:44 -0700 Subject: [PATCH 5/6] test(engine): behaviour-test the author-association vocabulary in the ENGINE's own suite Same cause as the sibling fix on #9847. packages/loopover-engine/src/** is credited by two Codecov uploads, and the "engine" flag is fed by the package's own node:test suite -- so a module that only the root vitest suite exercises reads as uncovered there. author-association.ts moved INTO the engine in this PR, which makes every one of its lines new to that flag. The predicate is now behaviour-tested in the engine suite, including through buildPullRequestAdvisory, since the gate-advisory twin is one of its callers and is where the fifth duplicate copy lived. --- packages/loopover-engine/src/index.ts | 9 +++ .../test/author-association.test.ts | 65 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 packages/loopover-engine/test/author-association.test.ts diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index 8172f57715..fcb802c00d 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -957,3 +957,12 @@ export { isDone as isPlanStepDone, nextReadySteps } from "./plan-step-readiness. // #9743: the priority label's single resolution, exported so the engine's OWN behaviour suite can test // it -- the `engine` Codecov flag credits engine source from that suite, not from the root vitest run. export { DEFAULT_PRIORITY_LABEL, resolvePriorityTypeLabel } from "./settings/pr-type-label.js"; +// #9743: the author-association vocabulary, exported so the engine's OWN behaviour suite can reach it -- +// the "engine" Codecov flag credits engine source from that suite, and gate-advisory (an engine twin) +// is one of its callers. +export { + MAINTAINER_AUTHOR_ASSOCIATIONS, + classifyAuthorAssociation, + isMaintainerAuthorAssociation, + type AuthorClass, +} from "./settings/author-association.js"; diff --git a/packages/loopover-engine/test/author-association.test.ts b/packages/loopover-engine/test/author-association.test.ts new file mode 100644 index 0000000000..158510602f --- /dev/null +++ b/packages/loopover-engine/test/author-association.test.ts @@ -0,0 +1,65 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { MAINTAINER_AUTHOR_ASSOCIATIONS, classifyAuthorAssociation, isMaintainerAuthorAssociation } from "../dist/index.js"; + +// The engine's own behaviour suite for the author-association vocabulary (#9743). gate-advisory -- an +// engine twin -- is one of its callers, and the per-author-class parity rollups publish this split, so +// these are engine semantics rather than app glue. + +test("every maintainer association classifies as maintainer, case-insensitively", () => { + for (const association of MAINTAINER_AUTHOR_ASSOCIATIONS) { + assert.equal(classifyAuthorAssociation(association), "maintainer", association); + assert.equal(classifyAuthorAssociation(association.toLowerCase()), "maintainer", association); + assert.equal(isMaintainerAuthorAssociation(association), true, association); + } +}); + +test("CONTRIBUTOR is a contributor -- a merged PR in the past is not authority over the repo", () => { + assert.equal(isMaintainerAuthorAssociation("CONTRIBUTOR"), false); + assert.equal(classifyAuthorAssociation("CONTRIBUTOR"), "contributor"); + assert.equal(classifyAuthorAssociation("FIRST_TIME_CONTRIBUTOR"), "contributor"); + assert.equal(classifyAuthorAssociation("NONE"), "contributor"); +}); + +test("an unrecorded association is UNKNOWN, never folded into either side", () => { + // Folding unknowns into `contributor` would bias the exact comparison the rollups publish. + for (const value of [null, undefined, "", " "]) { + assert.equal(classifyAuthorAssociation(value as never), "unknown", JSON.stringify(value)); + assert.equal(isMaintainerAuthorAssociation(value as never), false, JSON.stringify(value)); + } +}); + +test("a non-string is not treated as an association", () => { + assert.equal(isMaintainerAuthorAssociation(42 as never), false); + assert.equal(classifyAuthorAssociation(42 as never), "unknown"); +}); + +// The gate-advisory TWIN is one of this predicate's callers (#9743 consolidated a fifth copy out of it), +// so the branch it feeds is exercised here too rather than only through the app's own suite. +test("buildPullRequestAdvisory raises maintainer_authored_pr for a maintainer association", async () => { + const { buildPullRequestAdvisory } = await import("../dist/advisory/gate-advisory.js"); + const repo = { fullName: "o/r", defaultBranch: "main" } as never; + const basePr = { + repoFullName: "o/r", + number: 1, + title: "fix: a thing", + state: "open", + authorLogin: "someone", + labels: [], + linkedIssues: [], + bodyObservedAt: null, + }; + + const maintainer = buildPullRequestAdvisory(repo, { ...basePr, authorAssociation: "MEMBER" } as never, {}); + assert.ok( + maintainer.findings.some((f) => f.code === "maintainer_authored_pr"), + "a MEMBER-authored PR is flagged as maintainer-authored", + ); + + const contributor = buildPullRequestAdvisory(repo, { ...basePr, authorAssociation: "CONTRIBUTOR" } as never, {}); + assert.equal( + contributor.findings.some((f) => f.code === "maintainer_authored_pr"), + false, + "a CONTRIBUTOR-authored PR is not", + ); +}); From 781ee037fcdfe757bc722d98846712f3e5335775 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 09:06:18 -0700 Subject: [PATCH 6/6] fix(db): renumber the findings-count migration to 0206 0205 was taken on main by the visual-capture retry-latch migration (#9876). Two PRs grabbed the same number; the newest renumbers, per check-migrations. --- ...findings_count.sql => 0206_decision_record_findings_count.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename migrations/{0205_decision_record_findings_count.sql => 0206_decision_record_findings_count.sql} (100%) diff --git a/migrations/0205_decision_record_findings_count.sql b/migrations/0206_decision_record_findings_count.sql similarity index 100% rename from migrations/0205_decision_record_findings_count.sql rename to migrations/0206_decision_record_findings_count.sql