diff --git a/.env.example b/.env.example index ea34ae6363..be30a8ff5b 100644 --- a/.env.example +++ b/.env.example @@ -65,7 +65,7 @@ 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 @@ -73,9 +73,10 @@ GITTENSORY_REVIEW_ENRICHMENT=false # 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 diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index 867dfd2607..6f59d0fe27 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -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); diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index dc5ca73fc9..cb41d50328 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -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." + } } ] } diff --git a/review-enrichment/src/analyzers/approval-integrity.ts b/review-enrichment/src/analyzers/approval-integrity.ts new file mode 100644 index 0000000000..af3837c308 --- /dev/null +++ b/review-enrichment/src/analyzers/approval-integrity.ts @@ -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; + 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 { + 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, + fetchFn: typeof fetch, + signal: AbortSignal | undefined, + options: Pick, +): Promise { + 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(url, fetchOptions) + : await boundedFetchJson(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, + fetchFn: typeof fetch, + signal: AbortSignal | undefined, + options: Pick, +): Promise { + 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 { + const latest = new Map(); + 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 { + 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; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 47a272b5d8..e6bb7dff93 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -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"; @@ -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( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 40f6489698..9546fdbace 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -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: "" }; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 7221ce3e61..2d147201e7 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -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[]; @@ -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 diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 483cfb23d0..de194c56d9 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -32,6 +32,7 @@ const EXPECTED_ANALYZERS = [ "duplication", "churnHotspot", "blameLink", + "approvalIntegrity", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/approval-integrity.test.ts b/review-enrichment/test/approval-integrity.test.ts new file mode 100644 index 0000000000..8a6ae46bd4 --- /dev/null +++ b/review-enrichment/test/approval-integrity.test.ts @@ -0,0 +1,281 @@ +// Units for the review/approval integrity analyzer. Own file (not enrichment.test.ts) so concurrent analyzer PRs +// don't collide. All network is mocked. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + latestReviewPerReviewer, + scanApprovalIntegrity, +} from "../dist/analyzers/approval-integrity.js"; +import { renderBrief } from "../dist/render.js"; + +const jsonResponse = (body, code = 200) => new Response(JSON.stringify(body), { status: code }); + +const req = (extra = {}) => ({ + repoFullName: "octo/repo", + prNumber: 7, + githubToken: "ghp_test", + headSha: "head0000000000000000000000000000000000", + ...extra, +}); + +const reviewsFetch = (reviews) => async () => jsonResponse(reviews); + +const review = (login, state, commitId, submittedAt) => ({ + user: { login }, + state, + commit_id: commitId, + submitted_at: submittedAt, +}); + +test("latestReviewPerReviewer: keeps only the latest-submitted review per login", () => { + const latest = latestReviewPerReviewer([ + review("alice", "CHANGES_REQUESTED", "sha1", "2026-01-01T00:00:00Z"), + review("alice", "APPROVED", "sha2", "2026-01-02T00:00:00Z"), + review("bob", "APPROVED", "sha1", "2026-01-01T00:00:00Z"), + ]); + assert.equal(latest.size, 2); + assert.equal(latest.get("alice").state, "APPROVED"); + assert.equal(latest.get("alice").commitId, "sha2"); + assert.equal(latest.get("bob").state, "APPROVED"); +}); + +test("latestReviewPerReviewer: excludes a PENDING draft review (no submitted_at)", () => { + const latest = latestReviewPerReviewer([ + { user: { login: "alice" }, state: "PENDING", commit_id: "sha1", submitted_at: null }, + ]); + assert.equal(latest.size, 0); +}); + +test("latestReviewPerReviewer: groups logins case-insensitively, keeping the latest", () => { + const latest = latestReviewPerReviewer([ + review("Alice", "CHANGES_REQUESTED", "sha1", "2026-01-01T00:00:00Z"), + review("alice", "APPROVED", "sha2", "2026-01-02T00:00:00Z"), + ]); + assert.equal(latest.size, 1); + assert.equal(latest.get("alice").state, "APPROVED"); + assert.equal(latest.get("alice").login, "alice"); // the winning (latest) review's own-case login is kept +}); + +test("latestReviewPerReviewer: skips a malformed entry missing user/state/submitted_at", () => { + const latest = latestReviewPerReviewer([ + { state: "APPROVED", commit_id: "sha1", submitted_at: "2026-01-01T00:00:00Z" }, // no user + { user: { login: "bob" }, commit_id: "sha1", submitted_at: "2026-01-01T00:00:00Z" }, // no state + { user: { login: "carol" }, state: "APPROVED", submitted_at: undefined }, // no submitted_at + ]); + assert.equal(latest.size, 0); +}); + +test("latestReviewPerReviewer: on a submitted_at tie, the LATER item in API order wins", () => { + const latest = latestReviewPerReviewer([ + review("alice", "CHANGES_REQUESTED", "sha1", "2026-01-01T00:00:00Z"), + review("alice", "APPROVED", "sha2", "2026-01-01T00:00:00Z"), // same timestamp, later in API (oldest-first) order + ]); + assert.equal(latest.get("alice").state, "APPROVED"); + assert.equal(latest.get("alice").commitId, "sha2"); +}); + +test("scanApprovalIntegrity: flags an approval that predates the current head commit", async () => { + const findings = await scanApprovalIntegrity( + req(), + reviewsFetch([review("alice", "APPROVED", "old-sha-1234567890", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, [ + { reviewer: "alice", kind: "stale-approval", reviewedShaPrefix: "old-sha-1234" }, + ]); + const brief = renderBrief({ approvalIntegrity: findings }).promptSection; + assert.match(brief, /predates the current head commit/); +}); + +test("scanApprovalIntegrity: an approval on the current head commit is fresh (no finding)", async () => { + const findings = await scanApprovalIntegrity( + req(), + reviewsFetch([review("alice", "APPROVED", req().headSha, "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: flags the author approving their own PR", async () => { + const findings = await scanApprovalIntegrity( + req({ author: "octocat" }), + reviewsFetch([review("octocat", "APPROVED", req().headSha, "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, [{ reviewer: "octocat", kind: "self-approval" }]); + const brief = renderBrief({ approvalIntegrity: findings }).promptSection; + assert.match(brief, /approved their own PR/); +}); + +test("scanApprovalIntegrity: self-approval comparison is case-insensitive", async () => { + const findings = await scanApprovalIntegrity( + req({ author: "Octocat" }), + reviewsFetch([review("octocat", "APPROVED", req().headSha, "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, [{ reviewer: "octocat", kind: "self-approval" }]); +}); + +test("scanApprovalIntegrity: a stale AND self approval on the same review yields both findings", async () => { + const findings = await scanApprovalIntegrity( + req({ author: "octocat" }), + reviewsFetch([review("octocat", "APPROVED", "old-sha-1234567890", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, [ + { reviewer: "octocat", kind: "stale-approval", reviewedShaPrefix: "old-sha-1234" }, + { reviewer: "octocat", kind: "self-approval" }, + ]); +}); + +test("scanApprovalIntegrity: no self-approval finding when the PR has no known author", async () => { + const findings = await scanApprovalIntegrity( + req(), // no `author` + reviewsFetch([review("octocat", "APPROVED", req().headSha, "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: flags a reviewer whose current review is still CHANGES_REQUESTED", async () => { + const findings = await scanApprovalIntegrity( + req(), + reviewsFetch([review("bob", "CHANGES_REQUESTED", "sha1", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, [{ reviewer: "bob", kind: "outstanding-changes-requested" }]); + const brief = renderBrief({ approvalIntegrity: findings }).promptSection; + assert.match(brief, /still requesting changes/); +}); + +test("scanApprovalIntegrity: a later APPROVED supersedes an earlier CHANGES_REQUESTED (no longer outstanding)", async () => { + const findings = await scanApprovalIntegrity( + req(), + reviewsFetch([ + review("bob", "CHANGES_REQUESTED", "sha1", "2026-01-01T00:00:00Z"), + review("bob", "APPROVED", req().headSha, "2026-01-02T00:00:00Z"), + ]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: a dismissed review (state DISMISSED) is not outstanding", async () => { + const findings = await scanApprovalIntegrity( + req(), + reviewsFetch([review("bob", "DISMISSED", "sha1", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: a COMMENTED review yields no finding", async () => { + const findings = await scanApprovalIntegrity( + req(), + reviewsFetch([review("bob", "COMMENTED", "sha1", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: no GitHub token → skipped (no finding, no throw)", async () => { + const findings = await scanApprovalIntegrity( + req({ githubToken: undefined }), + reviewsFetch([review("alice", "APPROVED", "old-sha", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: no head SHA → skipped (no finding, no throw)", async () => { + const findings = await scanApprovalIntegrity( + req({ headSha: undefined }), + reviewsFetch([review("alice", "APPROVED", "old-sha", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: a malformed repoFullName is skipped, not thrown", async () => { + const findings = await scanApprovalIntegrity( + req({ repoFullName: "not-a-valid-slug" }), + reviewsFetch([review("alice", "APPROVED", "old-sha", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: a fetch failure yields no finding", async () => { + const findings = await scanApprovalIntegrity(req(), async () => jsonResponse({ message: "bad" }, 500)); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: no reviews yields no finding", async () => { + const findings = await scanApprovalIntegrity(req(), reviewsFetch([])); + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: requests the first page of reviews with the expected URL shape", async () => { + let requestedUrl; + const findings = await scanApprovalIntegrity(req(), async (url) => { + requestedUrl = url; + return jsonResponse([]); + }); + assert.deepEqual(findings, []); + assert.match(requestedUrl, /\/pulls\/7\/reviews\?per_page=100&page=1$/); + assert.match(requestedUrl, /^https:\/\/api\.github\.com\/repos\/octo\/repo\//); +}); + +test("scanApprovalIntegrity: a short page (< per_page) stops pagination without a second request", async () => { + let calls = 0; + const findings = await scanApprovalIntegrity(req(), async () => { + calls += 1; + return jsonResponse([review("alice", "APPROVED", req().headSha, "2026-01-01T00:00:00Z")]); + }); + assert.deepEqual(findings, []); + assert.equal(calls, 1); +}); + +test("scanApprovalIntegrity: walks a second page to find a reviewer's true latest vote (>100 reviews)", async () => { + // Page 1 (oldest-first, as GitHub returns them) is a full page ending on alice's CHANGES_REQUESTED; her real + // latest vote — an APPROVED on the current head commit — is on page 2. Reading only page 1 would wrongly report + // her as still requesting changes. + const page1 = Array.from({ length: 100 }, (_, i) => + i === 99 + ? review("alice", "CHANGES_REQUESTED", "sha-old", "2026-01-01T00:00:00Z") + : review(`bystander${i}`, "COMMENTED", "sha-old", "2026-01-01T00:00:00Z"), + ); + const page2 = [review("alice", "APPROVED", req().headSha, "2026-01-02T00:00:00Z")]; + let calls = 0; + const findings = await scanApprovalIntegrity(req(), async (url) => { + calls += 1; + return jsonResponse(url.includes("page=2") ? page2 : page1); + }); + assert.equal(calls, 2); + assert.deepEqual(findings, []); // alice's true latest vote is a fresh approval, not outstanding +}); + +test("scanApprovalIntegrity: pagination is bounded, and an unconfirmed-complete history (still-full last page) fails closed", async () => { + let calls = 0; + // Every page is full, including page 10 (MAX_PAGES) — so we can never confirm this is the reviewer's true + // latest vote (an 11th page might exist). A naive "return what we have" would wrongly report bob as still + // requesting changes; failing closed (no finding) is correct here. + const fullPage = (marker) => + Array.from({ length: 100 }, (_, i) => (i === 99 ? review("bob", "CHANGES_REQUESTED", marker, "2026-01-01T00:00:00Z") : review(`user${i}`, "COMMENTED", marker, "2026-01-01T00:00:00Z"))); + const findings = await scanApprovalIntegrity(req(), async () => { + calls += 1; + return jsonResponse(fullPage(`sha-${calls}`)); + }); + assert.equal(calls, 10); // MAX_PAGES, not unbounded + assert.deepEqual(findings, []); // completeness unconfirmed → fails closed, not a false "outstanding" finding +}); + +test("scanApprovalIntegrity: a later-page fetch failure fails the whole call closed (no false findings from partial history)", async () => { + const page1 = Array.from({ length: 100 }, (_, i) => + i === 99 + ? review("bob", "CHANGES_REQUESTED", "sha-old", "2026-01-01T00:00:00Z") + : review(`bystander${i}`, "COMMENTED", "sha-old", "2026-01-01T00:00:00Z"), + ); + const findings = await scanApprovalIntegrity(req(), async (url) => { + if (url.includes("page=2")) return jsonResponse({ message: "boom" }, 500); + return jsonResponse(page1); + }); + // page 2 (which might hold bob's true latest vote — e.g. a superseding APPROVED) failed to fetch, so treating + // page 1's CHANGES_REQUESTED as his current state could be wrong. Fail closed: no finding, not a stale one. + assert.deepEqual(findings, []); +}); + +test("scanApprovalIntegrity: stale-approval commit SHA comparison is case-insensitive", async () => { + const findings = await scanApprovalIntegrity( + req({ headSha: "AbCdEf0000000000000000000000000000000000".toLowerCase() }), + reviewsFetch([review("alice", "APPROVED", "ABCDEF0000000000000000000000000000000000", "2026-01-01T00:00:00Z")]), + ); + assert.deepEqual(findings, []); // same SHA, different case → NOT stale +});