Skip to content

Commit 3a95eae

Browse files
committed
feat(enrichment): flag review/approval integrity signals
1 parent 96d2980 commit 3a95eae

9 files changed

Lines changed: 546 additions & 2 deletions

File tree

.env.example

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,17 +65,18 @@ GITTENSORY_REVIEW_ENRICHMENT=false
6565
# Current analyzer names:
6666
# dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos
6767
# provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild
68-
# history,docCommentDrift,duplication,churnHotspot,blameLink
68+
# history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity
6969
#
7070
# Profile defaults:
7171
# fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol
7272
# redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild
7373
# balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency
7474
# actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature
7575
# iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink
76+
# approvalIntegrity
7677
# deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol
7778
# redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig
78-
# nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink
79+
# nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity
7980
# END GENERATED REES ANALYZERS
8081

8182
# Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep

apps/gittensory-ui/src/lib/rees-analyzers.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -584,6 +584,30 @@ export const REES_ANALYZERS = [
584584
"File-level, not per-line: it reports each file's most recent prior toucher, never claiming a specific line's origin. Fail-safe and partial on cap.",
585585
},
586586
},
587+
{
588+
name: "approvalIntegrity",
589+
title: "Review/approval integrity",
590+
category: "history",
591+
cost: "github-light",
592+
defaultEnabled: true,
593+
profiles: ["balanced", "deep"],
594+
requires: ["github-token", "head-sha"],
595+
limits: {
596+
maxPages: 10,
597+
reviewsPerPage: 100,
598+
},
599+
docs: {
600+
summary:
601+
"Flags review/approval integrity signals: an APPROVED review that predates the current head commit, the author approving their own PR, and a reviewer whose current review is still CHANGES_REQUESTED.",
602+
looksAt:
603+
"The PR's reviews (walked page by page, bounded), reduced to each reviewer's most recent submitted review — GitHub's own semantics for a reviewer's current vote.",
604+
reports:
605+
"Reviewer login, the finding kind, and (for a stale approval) a short commit-SHA prefix — never review body text.",
606+
network: "Calls the GitHub PR-reviews API, paginated and bounded to a fixed page cap.",
607+
notes:
608+
"Structured-fields-only: reads state/commit_id/user.login/submitted_at, never diff or review-body text. Fail-safe on missing token/head SHA/fetch error.",
609+
},
610+
},
587611
] as const satisfies readonly ReesAnalyzerDoc[];
588612

589613
export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name);

review-enrichment/analyzer-metadata.json

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -662,6 +662,32 @@
662662
"network": "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.",
663663
"notes": "File-level, not per-line: it reports each file's most recent prior toucher, never claiming a specific line's origin. Fail-safe and partial on cap."
664664
}
665+
},
666+
{
667+
"name": "approvalIntegrity",
668+
"title": "Review/approval integrity",
669+
"category": "history",
670+
"cost": "github-light",
671+
"defaultEnabled": true,
672+
"profiles": [
673+
"balanced",
674+
"deep"
675+
],
676+
"requires": [
677+
"github-token",
678+
"head-sha"
679+
],
680+
"limits": {
681+
"maxPages": 10,
682+
"reviewsPerPage": 100
683+
},
684+
"docs": {
685+
"summary": "Flags review/approval integrity signals: an APPROVED review that predates the current head commit, the author approving their own PR, and a reviewer whose current review is still CHANGES_REQUESTED.",
686+
"looksAt": "The PR's reviews (walked page by page, bounded), reduced to each reviewer's most recent submitted review — GitHub's own semantics for a reviewer's current vote.",
687+
"reports": "Reviewer login, the finding kind, and (for a stale approval) a short commit-SHA prefix — never review body text.",
688+
"network": "Calls the GitHub PR-reviews API, paginated and bounded to a fixed page cap.",
689+
"notes": "Structured-fields-only: reads state/commit_id/user.login/submitted_at, never diff or review-body text. Fail-safe on missing token/head SHA/fetch error."
690+
}
665691
}
666692
]
667693
}
Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
// Review/approval integrity signals, read from structured PR-reviews data only — no diff/text/YAML parsing.
2+
// Surfaces cases a PR's own page does not always make obvious without branch protection's "dismiss stale reviews"
3+
// setting enabled: an APPROVED review that predates the current head commit (new pushes landed since the
4+
// approval), the PR author approving their own PR, and a reviewer whose CURRENT (most recent) review is still
5+
// CHANGES_REQUESTED. Reads only documented fields from the GitHub PR-reviews API (state, commit_id, user.login,
6+
// submitted_at) and compares them — no ambiguous-syntax parsing, so it cannot suffer a patch scanner's edge cases.
7+
// Pure GitHub-metadata read, no repo content. Fail-safe: no token, no head SHA, a bad repo slug, or a fetch error
8+
// all yield no finding. Bounded to MAX_PAGES pages of reviews (REVIEWS_PER_PAGE each) — a pathological PR can't
9+
// spin the analyzer, but any realistic PR's full review history is read, not just its oldest page.
10+
import type {
11+
AnalyzerDiagnostics,
12+
ApprovalIntegrityFinding,
13+
EnrichRequest,
14+
} from "../types.js";
15+
import type { AnalysisContext } from "../analysis-context.js";
16+
import { boundedFetchJson } from "../external-fetch.js";
17+
18+
const GITHUB_API = "https://api.github.com";
19+
const SLUG_RE = /^[A-Za-z0-9._-]+$/;
20+
const REVIEWS_PER_PAGE = 100;
21+
// GitHub returns PR reviews oldest-first with no reorder option, so a single `per_page=100` fetch would silently
22+
// read only the OLDEST reviews on any PR with more — exactly backwards for "each reviewer's latest vote". Walk
23+
// pages instead, bounded so a pathological PR can't spin (mirrors this repo's own PR_DETAIL_MAX_PAGES convention).
24+
const MAX_PAGES = 10;
25+
const SHA_PREFIX_LEN = 12;
26+
27+
interface ScanOptions {
28+
signal?: AbortSignal;
29+
analysis?: Pick<AnalysisContext, "fetchJson">;
30+
diagnostics?: AnalyzerDiagnostics;
31+
}
32+
33+
/** The slice of a GitHub PR-review list item this analyzer reads. */
34+
interface ReviewListItem {
35+
user?: { login?: string } | null;
36+
state?: string;
37+
commit_id?: string;
38+
submitted_at?: string | null;
39+
}
40+
41+
/** One reviewer's current (most recent submitted) vote. */
42+
interface LatestReview {
43+
login: string;
44+
state: string;
45+
commitId: string | undefined;
46+
submittedAt: string;
47+
}
48+
49+
function githubHeaders(token: string): Record<string, string> {
50+
return {
51+
Authorization: `Bearer ${token}`,
52+
Accept: "application/vnd.github+json",
53+
"X-GitHub-Api-Version": "2022-11-28",
54+
};
55+
}
56+
57+
async function fetchReviewsPage(
58+
owner: string,
59+
repo: string,
60+
prNumber: number,
61+
page: number,
62+
headers: Record<string, string>,
63+
fetchFn: typeof fetch,
64+
signal: AbortSignal | undefined,
65+
options: Pick<ScanOptions, "analysis" | "diagnostics">,
66+
): Promise<ReviewListItem[] | null> {
67+
const url =
68+
`${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/pulls/` +
69+
`${encodeURIComponent(String(prNumber))}/reviews?per_page=${REVIEWS_PER_PAGE}&page=${page}`;
70+
const fetchOptions = {
71+
endpointCategory: "github-pr-reviews",
72+
headers,
73+
signal,
74+
fetchImpl: fetchFn,
75+
diagnostics: options.diagnostics,
76+
phase: "approval-integrity",
77+
subcall: "github-pr-reviews",
78+
maxBytes: 512 * 1024,
79+
};
80+
const response = options.analysis
81+
? await options.analysis.fetchJson<ReviewListItem[]>(url, fetchOptions)
82+
: await boundedFetchJson<ReviewListItem[]>(url, fetchOptions);
83+
return response.ok && Array.isArray(response.data) ? response.data : null;
84+
}
85+
86+
/** Walks review pages (oldest-first, as GitHub returns them) up to MAX_PAGES, so `latestReviewPerReviewer` sees
87+
* every reviewer's true latest vote rather than just the oldest page. A short page (fewer than REVIEWS_PER_PAGE
88+
* items) means there is nothing further — no Link-header parsing needed. A page-1 failure yields null (same
89+
* fail-safe contract as before); a later-page failure keeps the pages already fetched rather than discarding a
90+
* successful start, mirroring this repo's own githubPaginatedList convention. */
91+
async function fetchReviews(
92+
owner: string,
93+
repo: string,
94+
prNumber: number,
95+
headers: Record<string, string>,
96+
fetchFn: typeof fetch,
97+
signal: AbortSignal | undefined,
98+
options: Pick<ScanOptions, "analysis" | "diagnostics">,
99+
): Promise<ReviewListItem[] | null> {
100+
const items: ReviewListItem[] = [];
101+
for (let page = 1; page <= MAX_PAGES; page += 1) {
102+
const pageItems = await fetchReviewsPage(owner, repo, prNumber, page, headers, fetchFn, signal, options);
103+
if (!pageItems) return page === 1 ? null : items;
104+
items.push(...pageItems);
105+
if (pageItems.length < REVIEWS_PER_PAGE) break;
106+
}
107+
return items;
108+
}
109+
110+
/** Reduces a PR's review list to one entry per reviewer: the review with the latest `submitted_at`. This mirrors
111+
* GitHub's own semantics for "a reviewer's current vote" — a later review of ANY state supersedes an earlier one
112+
* from the same person, including a dismissal (the API reports a dismissed review back with `state: "DISMISSED"`,
113+
* so a dismissed CHANGES_REQUESTED naturally stops counting as outstanding without any extra handling here).
114+
* Reviews with no `submitted_at` (a still-open PENDING draft review) are excluded — not yet a submitted vote.
115+
* Login comparison is case-insensitive (GitHub logins are case-insensitive), keyed on the lowercased login. Pure. */
116+
export function latestReviewPerReviewer(reviews: ReviewListItem[]): Map<string, LatestReview> {
117+
const latest = new Map<string, LatestReview>();
118+
for (const review of reviews) {
119+
const login = review.user?.login;
120+
const state = review.state;
121+
const submittedAt = review.submitted_at;
122+
if (!login || !state || !submittedAt) continue;
123+
const key = login.toLowerCase();
124+
const existing = latest.get(key);
125+
if (!existing || submittedAt > existing.submittedAt) {
126+
latest.set(key, { login, state, commitId: review.commit_id, submittedAt });
127+
}
128+
}
129+
return latest;
130+
}
131+
132+
/** Analyzer entrypoint: a PR's reviews → stale/self/outstanding approval-integrity findings. Fail-safe — no token,
133+
* no head SHA, a bad repo slug, or a fetch error all yield no finding rather than an error. */
134+
export async function scanApprovalIntegrity(
135+
req: EnrichRequest,
136+
fetchFn: typeof fetch = fetch,
137+
options: ScanOptions = {},
138+
): Promise<ApprovalIntegrityFinding[]> {
139+
const { repoFullName, githubToken, headSha, author, prNumber } = req;
140+
if (!githubToken || !headSha) return [];
141+
const parts = repoFullName.split("/");
142+
const owner = parts[0];
143+
const repo = parts[1];
144+
if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return [];
145+
146+
const headers = githubHeaders(githubToken);
147+
const reviews = await fetchReviews(owner, repo, prNumber, headers, fetchFn, options.signal, options);
148+
if (!reviews) return [];
149+
150+
const findings: ApprovalIntegrityFinding[] = [];
151+
const authorKey = author?.toLowerCase();
152+
const headShaKey = headSha.toLowerCase();
153+
for (const { login, state, commitId } of latestReviewPerReviewer(reviews).values()) {
154+
if (state === "APPROVED") {
155+
if (commitId && commitId.toLowerCase() !== headShaKey) {
156+
findings.push({
157+
reviewer: login,
158+
kind: "stale-approval",
159+
reviewedShaPrefix: commitId.slice(0, SHA_PREFIX_LEN),
160+
});
161+
}
162+
if (authorKey && login.toLowerCase() === authorKey) {
163+
findings.push({ reviewer: login, kind: "self-approval" });
164+
}
165+
} else if (state === "CHANGES_REQUESTED") {
166+
findings.push({ reviewer: login, kind: "outstanding-changes-requested" });
167+
}
168+
}
169+
return findings;
170+
}

review-enrichment/src/analyzers/registry.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { scanActionPins } from "./actions-pin.js";
2+
import { scanApprovalIntegrity } from "./approval-integrity.js";
23
import { scanAssetWeight } from "./asset-weight.js";
34
import { scanChurnHotspot } from "./churn-hotspot.js";
45
import { scanBlameLink } from "./blame-link.js";
@@ -478,6 +479,43 @@ export const ANALYZER_DESCRIPTORS = [
478479
run: (req, { signal, analysis, diagnostics }) =>
479480
scanBlameLink(req, fetch, { signal, analysis, diagnostics }),
480481
}),
482+
descriptor({
483+
name: "approvalIntegrity",
484+
title: "Review/approval integrity",
485+
category: "history",
486+
cost: "github-light",
487+
defaultEnabled: true,
488+
requires: ["github-token", "head-sha"],
489+
limits: { maxPages: 10, reviewsPerPage: 100 },
490+
docs: {
491+
summary:
492+
"Flags review/approval integrity signals: an APPROVED review that predates the current head commit, the author approving their own PR, and a reviewer whose current review is still CHANGES_REQUESTED.",
493+
looksAt:
494+
"The PR's reviews (walked page by page, bounded), reduced to each reviewer's most recent submitted review — GitHub's own semantics for a reviewer's current vote.",
495+
reports: "Reviewer login, the finding kind, and (for a stale approval) a short commit-SHA prefix — never review body text.",
496+
network: "Calls the GitHub PR-reviews API, paginated and bounded to a fixed page cap.",
497+
notes:
498+
"Structured-fields-only: reads state/commit_id/user.login/submitted_at, never diff or review-body text. Fail-safe on missing token/head SHA/fetch error.",
499+
},
500+
render: (findings, helpers) => {
501+
if (!findings.length) return [];
502+
const lines = ["### Review/approval integrity"];
503+
for (const item of findings) {
504+
if (item.kind === "stale-approval") {
505+
lines.push(
506+
`- ${helpers.safeCodeSpan(item.reviewer)}'s approval predates the current head commit (reviewed ${helpers.safeCodeSpan(item.reviewedShaPrefix)})`,
507+
);
508+
} else if (item.kind === "self-approval") {
509+
lines.push(`- ${helpers.safeCodeSpan(item.reviewer)} approved their own PR`);
510+
} else {
511+
lines.push(`- ${helpers.safeCodeSpan(item.reviewer)}'s current review is still requesting changes`);
512+
}
513+
}
514+
return lines;
515+
},
516+
run: (req, { signal, analysis, diagnostics }) =>
517+
scanApprovalIntegrity(req, fetch, { signal, analysis, diagnostics }),
518+
}),
481519
] as const satisfies readonly AnyAnalyzerDescriptor[];
482520

483521
export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map(

review-enrichment/src/render.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,7 @@ export function renderBrief(
380380
}
381381

382382
lines.push(...renderDescriptorSection("blameLink", findings.blameLink));
383+
lines.push(...renderDescriptorSection("approvalIntegrity", findings.approvalIntegrity));
383384

384385
if (!lines.length) return { promptSection: "", systemSuffix: "" };
385386

review-enrichment/src/types.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,21 @@ export interface BlameLinkFinding {
301301
lastTouchedByShaPrefix?: string;
302302
}
303303

304+
/** A review/approval integrity signal, read from structured PR-reviews API fields only (state, commit_id,
305+
* user.login, submitted_at) — never diff/file content. `stale-approval`: the reviewer's latest APPROVED review
306+
* predates the PR's current head commit. `self-approval`: the PR author approved their own PR.
307+
* `outstanding-changes-requested`: the reviewer's CURRENT (most recent) review is still CHANGES_REQUESTED, not
308+
* yet superseded by a later review from the same person. */
309+
export type ApprovalIntegrityFinding =
310+
| {
311+
reviewer: string;
312+
kind: "stale-approval";
313+
/** Short prefix of the stale review's commit SHA (prefix only — never the full SHA). */
314+
reviewedShaPrefix: string;
315+
}
316+
| { reviewer: string; kind: "self-approval" }
317+
| { reviewer: string; kind: "outstanding-changes-requested" };
318+
304319
/** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */
305320
export interface BriefFindings {
306321
dependency?: DependencyFinding[];
@@ -325,6 +340,7 @@ export interface BriefFindings {
325340
duplication?: DuplicationFinding[];
326341
churnHotspot?: ChurnHotspotFinding[];
327342
blameLink?: BlameLinkFinding[];
343+
approvalIntegrity?: ApprovalIntegrityFinding[];
328344
}
329345

330346
/** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a

review-enrichment/test/analyzer-registry.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ const EXPECTED_ANALYZERS = [
3232
"duplication",
3333
"churnHotspot",
3434
"blameLink",
35+
"approvalIntegrity",
3536
];
3637

3738
test("analyzer descriptors cover the runtime registry in stable order", () => {

0 commit comments

Comments
 (0)