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/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/migrations/0206_decision_record_findings_count.sql b/migrations/0206_decision_record_findings_count.sql new file mode 100644 index 0000000000..a5d9f7f55a --- /dev/null +++ b/migrations/0206_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/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; 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/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/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/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", + ); +}); 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..6dfe3eb3ea --- /dev/null +++ b/src/github/author-association.ts @@ -0,0 +1,7 @@ +// Re-export of the engine's author-association vocabulary. +// +// 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"; 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..4208351316 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 ?? null) .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..f2dcde217c 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -72,6 +72,7 @@ function recordInput(over: Partial = {}): Omit { 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" })); diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index c594589a5e..9c74357642 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -290,7 +290,7 @@ describe("queue processors", () => { // 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()); + }); +});