Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,18 @@ GITTENSORY_REVIEW_ENRICHMENT=false
# Current analyzer names:
# dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol,redos
# provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig,nativeBuild
# history,docCommentDrift,duplication,churnHotspot,blameLink
# history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity
#
# Profile defaults:
# fast: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol
# redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild
# balanced (default): dependency,lockfileDrift,secret,license,installScript,heavyDependency
# actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature
# iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink
# approvalIntegrity
# deep: dependency,lockfileDrift,secret,license,installScript,heavyDependency,actionPin,eol
# redos,provenance,codeowners,secretLog,assetWeight,typosquat,commitSignature,iacMisconfig
# nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink
# nativeBuild,history,docCommentDrift,duplication,churnHotspot,blameLink,approvalIntegrity
# END GENERATED REES ANALYZERS

# Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep
Expand Down
24 changes: 24 additions & 0 deletions apps/gittensory-ui/src/lib/rees-analyzers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,30 @@ export const REES_ANALYZERS = [
"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.",
},
},
{
name: "approvalIntegrity",
title: "Review/approval integrity",
category: "history",
cost: "github-light",
defaultEnabled: true,
profiles: ["balanced", "deep"],
requires: ["github-token", "head-sha"],
limits: {
maxPages: 10,
reviewsPerPage: 100,
},
docs: {
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.",
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.",
reports:
"Reviewer login, the finding kind, and (for a stale approval) a short commit-SHA prefix — never review body text.",
network: "Calls the GitHub PR-reviews API, paginated and bounded to a fixed page cap.",
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.",
},
},
] as const satisfies readonly ReesAnalyzerDoc[];

export const REES_ANALYZER_NAMES = REES_ANALYZERS.map((analyzer) => analyzer.name);
26 changes: 26 additions & 0 deletions review-enrichment/analyzer-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,32 @@
"network": "Calls the GitHub commits API and the commit→PR association API, both bounded by a total lookup cap.",
"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."
}
},
{
"name": "approvalIntegrity",
"title": "Review/approval integrity",
"category": "history",
"cost": "github-light",
"defaultEnabled": true,
"profiles": [
"balanced",
"deep"
],
"requires": [
"github-token",
"head-sha"
],
"limits": {
"maxPages": 10,
"reviewsPerPage": 100
},
"docs": {
"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.",
"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.",
"reports": "Reviewer login, the finding kind, and (for a stale approval) a short commit-SHA prefix — never review body text.",
"network": "Calls the GitHub PR-reviews API, paginated and bounded to a fixed page cap.",
"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."
}
}
]
}
175 changes: 175 additions & 0 deletions review-enrichment/src/analyzers/approval-integrity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
// Review/approval integrity signals, read from structured PR-reviews data only — no diff/text/YAML parsing.
// Surfaces cases a PR's own page does not always make obvious without branch protection's "dismiss stale reviews"
// setting enabled: an APPROVED review that predates the current head commit (new pushes landed since the
// approval), the PR author approving their own PR, and a reviewer whose CURRENT (most recent) review is still
// CHANGES_REQUESTED. Reads only documented fields from the GitHub PR-reviews API (state, commit_id, user.login,
// submitted_at) and compares them — no ambiguous-syntax parsing, so it cannot suffer a patch scanner's edge cases.
// Pure GitHub-metadata read, no repo content. Fail-safe: no token, no head SHA, a bad repo slug, or a fetch error
// all yield no finding. Bounded to MAX_PAGES pages of reviews (REVIEWS_PER_PAGE each) — a pathological PR can't
// spin the analyzer, but any realistic PR's full review history is read, not just its oldest page.
import type {
AnalyzerDiagnostics,
ApprovalIntegrityFinding,
EnrichRequest,
} from "../types.js";
import type { AnalysisContext } from "../analysis-context.js";
import { boundedFetchJson } from "../external-fetch.js";

const GITHUB_API = "https://api.github.com";
const SLUG_RE = /^[A-Za-z0-9._-]+$/;
const REVIEWS_PER_PAGE = 100;
// GitHub returns PR reviews oldest-first with no reorder option, so a single `per_page=100` fetch would silently
// read only the OLDEST reviews on any PR with more — exactly backwards for "each reviewer's latest vote". Walk
// pages instead, bounded so a pathological PR can't spin (mirrors this repo's own PR_DETAIL_MAX_PAGES convention).
const MAX_PAGES = 10;
const SHA_PREFIX_LEN = 12;

interface ScanOptions {
signal?: AbortSignal;
analysis?: Pick<AnalysisContext, "fetchJson">;
diagnostics?: AnalyzerDiagnostics;
}

/** The slice of a GitHub PR-review list item this analyzer reads. */
interface ReviewListItem {
user?: { login?: string } | null;
state?: string;
commit_id?: string;
submitted_at?: string | null;
}

/** One reviewer's current (most recent submitted) vote. */
interface LatestReview {
login: string;
state: string;
commitId: string | undefined;
submittedAt: string;
}

function githubHeaders(token: string): Record<string, string> {
return {
Authorization: `Bearer ${token}`,
Accept: "application/vnd.github+json",
"X-GitHub-Api-Version": "2022-11-28",
};
}

async function fetchReviewsPage(
owner: string,
repo: string,
prNumber: number,
page: number,
headers: Record<string, string>,
fetchFn: typeof fetch,
signal: AbortSignal | undefined,
options: Pick<ScanOptions, "analysis" | "diagnostics">,
): Promise<ReviewListItem[] | null> {
const url =
`${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/pulls/` +
`${encodeURIComponent(String(prNumber))}/reviews?per_page=${REVIEWS_PER_PAGE}&page=${page}`;
const fetchOptions = {
endpointCategory: "github-pr-reviews",
headers,
signal,
fetchImpl: fetchFn,
diagnostics: options.diagnostics,
phase: "approval-integrity",
subcall: "github-pr-reviews",
maxBytes: 512 * 1024,
};
const response = options.analysis
? await options.analysis.fetchJson<ReviewListItem[]>(url, fetchOptions)
: await boundedFetchJson<ReviewListItem[]>(url, fetchOptions);
return response.ok && Array.isArray(response.data) ? response.data : null;
}

/** Walks review pages (oldest-first, as GitHub returns them) up to MAX_PAGES, so `latestReviewPerReviewer` sees
* every reviewer's true latest vote rather than just the oldest page. A short page (fewer than REVIEWS_PER_PAGE
* items) confirms there is nothing further — no Link-header parsing needed — and only THEN is the list returned.
* Unlike a purely additive signal (e.g. blame-link's "last touched by"), a PARTIAL oldest-first list here is not
* a smaller-but-still-correct result — it is actively WRONG: a reviewer's true latest vote could sit on a page
* never read, making a resolved CHANGES_REQUESTED or a fresh APPROVED look stuck/stale. So any page failure, OR
* exhausting MAX_PAGES while the last page fetched was still full (pagination not confirmed complete), fails
* the whole call closed (null) rather than treating incomplete history as authoritative. */
async function fetchReviews(
owner: string,
repo: string,
prNumber: number,
headers: Record<string, string>,
fetchFn: typeof fetch,
signal: AbortSignal | undefined,
options: Pick<ScanOptions, "analysis" | "diagnostics">,
): Promise<ReviewListItem[] | null> {
const items: ReviewListItem[] = [];
for (let page = 1; page <= MAX_PAGES; page += 1) {
const pageItems = await fetchReviewsPage(owner, repo, prNumber, page, headers, fetchFn, signal, options);
if (!pageItems) return null; // any page failing means we cannot trust what we have so far
items.push(...pageItems);
if (pageItems.length < REVIEWS_PER_PAGE) return items; // confirmed: this was the last page
}
return null; // MAX_PAGES exhausted with a still-full final page — completeness unconfirmed
}

/** Reduces a PR's review list to one entry per reviewer: the review with the latest `submitted_at`. This mirrors
* GitHub's own semantics for "a reviewer's current vote" — a later review of ANY state supersedes an earlier one
* from the same person, including a dismissal (the API reports a dismissed review back with `state: "DISMISSED"`,
* so a dismissed CHANGES_REQUESTED naturally stops counting as outstanding without any extra handling here).
* Reviews with no `submitted_at` (a still-open PENDING draft review) are excluded — not yet a submitted vote.
* Login comparison is case-insensitive (GitHub logins are case-insensitive), keyed on the lowercased login.
* `reviews` must be in GitHub's own (oldest-first) API order: on a `submitted_at` tie (same timestamp
* precision), the LATER item in that order wins, using API order itself as the tie-break. Pure. */
export function latestReviewPerReviewer(reviews: ReviewListItem[]): Map<string, LatestReview> {
const latest = new Map<string, LatestReview>();
for (const review of reviews) {
const login = review.user?.login;
const state = review.state;
const submittedAt = review.submitted_at;
if (!login || !state || !submittedAt) continue;
const key = login.toLowerCase();
const existing = latest.get(key);
if (!existing || submittedAt >= existing.submittedAt) {
latest.set(key, { login, state, commitId: review.commit_id, submittedAt });
}
}
return latest;
}

/** Analyzer entrypoint: a PR's reviews → stale/self/outstanding approval-integrity findings. Fail-safe — no token,
* no head SHA, a bad repo slug, or a fetch error all yield no finding rather than an error. */
export async function scanApprovalIntegrity(
req: EnrichRequest,
fetchFn: typeof fetch = fetch,
options: ScanOptions = {},
): Promise<ApprovalIntegrityFinding[]> {
const { repoFullName, githubToken, headSha, author, prNumber } = req;
if (!githubToken || !headSha) return [];
const parts = repoFullName.split("/");
const owner = parts[0];
const repo = parts[1];
if (parts.length !== 2 || !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return [];

const headers = githubHeaders(githubToken);
const reviews = await fetchReviews(owner, repo, prNumber, headers, fetchFn, options.signal, options);
if (!reviews) return [];

const findings: ApprovalIntegrityFinding[] = [];
const authorKey = author?.toLowerCase();
const headShaKey = headSha.toLowerCase();
for (const { login, state, commitId } of latestReviewPerReviewer(reviews).values()) {
if (state === "APPROVED") {
if (commitId && commitId.toLowerCase() !== headShaKey) {
findings.push({
reviewer: login,
kind: "stale-approval",
reviewedShaPrefix: commitId.slice(0, SHA_PREFIX_LEN),
});
}
if (authorKey && login.toLowerCase() === authorKey) {
findings.push({ reviewer: login, kind: "self-approval" });
}
} else if (state === "CHANGES_REQUESTED") {
findings.push({ reviewer: login, kind: "outstanding-changes-requested" });
}
}
return findings;
}
38 changes: 38 additions & 0 deletions review-enrichment/src/analyzers/registry.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { scanActionPins } from "./actions-pin.js";
import { scanApprovalIntegrity } from "./approval-integrity.js";
import { scanAssetWeight } from "./asset-weight.js";
import { scanChurnHotspot } from "./churn-hotspot.js";
import { scanBlameLink } from "./blame-link.js";
Expand Down Expand Up @@ -478,6 +479,43 @@ export const ANALYZER_DESCRIPTORS = [
run: (req, { signal, analysis, diagnostics }) =>
scanBlameLink(req, fetch, { signal, analysis, diagnostics }),
}),
descriptor({
name: "approvalIntegrity",
title: "Review/approval integrity",
category: "history",
cost: "github-light",
defaultEnabled: true,
requires: ["github-token", "head-sha"],
limits: { maxPages: 10, reviewsPerPage: 100 },
docs: {
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.",
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.",
reports: "Reviewer login, the finding kind, and (for a stale approval) a short commit-SHA prefix — never review body text.",
network: "Calls the GitHub PR-reviews API, paginated and bounded to a fixed page cap.",
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.",
},
render: (findings, helpers) => {
if (!findings.length) return [];
const lines = ["### Review/approval integrity"];
for (const item of findings) {
if (item.kind === "stale-approval") {
lines.push(
`- ${helpers.safeCodeSpan(item.reviewer)}'s approval predates the current head commit (reviewed ${helpers.safeCodeSpan(item.reviewedShaPrefix)})`,
);
} else if (item.kind === "self-approval") {
lines.push(`- ${helpers.safeCodeSpan(item.reviewer)} approved their own PR`);
} else {
lines.push(`- ${helpers.safeCodeSpan(item.reviewer)}'s current review is still requesting changes`);
}
}
return lines;
},
run: (req, { signal, analysis, diagnostics }) =>
scanApprovalIntegrity(req, fetch, { signal, analysis, diagnostics }),
}),
] as const satisfies readonly AnyAnalyzerDescriptor[];

export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map(
Expand Down
1 change: 1 addition & 0 deletions review-enrichment/src/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ export function renderBrief(
}

lines.push(...renderDescriptorSection("blameLink", findings.blameLink));
lines.push(...renderDescriptorSection("approvalIntegrity", findings.approvalIntegrity));

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

Expand Down
16 changes: 16 additions & 0 deletions review-enrichment/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,21 @@ export interface BlameLinkFinding {
lastTouchedByShaPrefix?: string;
}

/** A review/approval integrity signal, read from structured PR-reviews API fields only (state, commit_id,
* user.login, submitted_at) — never diff/file content. `stale-approval`: the reviewer's latest APPROVED review
* predates the PR's current head commit. `self-approval`: the PR author approved their own PR.
* `outstanding-changes-requested`: the reviewer's CURRENT (most recent) review is still CHANGES_REQUESTED, not
* yet superseded by a later review from the same person. */
export type ApprovalIntegrityFinding =
| {
reviewer: string;
kind: "stale-approval";
/** Short prefix of the stale review's commit SHA (prefix only — never the full SHA). */
reviewedShaPrefix: string;
}
| { reviewer: string; kind: "self-approval" }
| { reviewer: string; kind: "outstanding-changes-requested" };

/** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */
export interface BriefFindings {
dependency?: DependencyFinding[];
Expand All @@ -325,6 +340,7 @@ export interface BriefFindings {
duplication?: DuplicationFinding[];
churnHotspot?: ChurnHotspotFinding[];
blameLink?: BlameLinkFinding[];
approvalIntegrity?: ApprovalIntegrityFinding[];
}

/** A JSDoc/TSDoc block whose `@param` tags name parameters the adjacent function no longer declares — a
Expand Down
1 change: 1 addition & 0 deletions review-enrichment/test/analyzer-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const EXPECTED_ANALYZERS = [
"duplication",
"churnHotspot",
"blameLink",
"approvalIntegrity",
];

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