diff --git a/.env.example b/.env.example index 327637fef9..d64398bd7b 100644 --- a/.env.example +++ b/.env.example @@ -69,7 +69,7 @@ GITTENSORY_REVIEW_ENRICHMENT=false # blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y -# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep +# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence # # Profile defaults: # fast: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency @@ -84,14 +84,14 @@ GITTENSORY_REVIEW_ENRICHMENT=false # commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology # todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting # errorSwallow,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak -# deprecatedDep +# deprecatedDep,revertRecurrence # deep: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat # commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot # blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber # conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y -# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep +# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence # 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 2f93f5a570..7d5074ca41 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -1268,6 +1268,33 @@ export const REES_ANALYZERS = [ "Conservative: only an exact match against the bundled list is flagged, so a package it does not name is never reported. Bounded by manifest, patch-line, and finding caps; fail-safe on absent patches or an aborted signal.", }, }, + { + name: "revertRecurrence", + title: "Revert recurrence", + category: "history", + cost: "github-heavy", + defaultEnabled: true, + profiles: ["balanced", "deep"], + requires: ["files", "github-token"], + limits: { + maxFilesProbed: 5, + commitsPerFile: 15, + maxRevertLookups: 10, + maxFindings: 25, + }, + docs: { + summary: + "Flags a changed file where the PR re-introduces added lines in a region a prior revert commit removed — a signal it may be re-treading a path that was already reverted or hot-fixed out.", + looksAt: + "For each changed file's added line-ranges, the file's recent commit history and the patch of any revert commit in it, intersecting the reverted (removed) old-file line-ranges with this PR's added new-file line-ranges.", + reports: + "File, a re-introduced line, a short revert commit-SHA prefix, and the reverted PR number when the revert message names one — never file contents.", + network: + "Calls the GitHub commits API per probed file and the single-commit API per revert commit, both bounded by fixed fanout caps. Requires GitHub token forwarding for private repos.", + notes: + "Conservative: only a message-confirmed revert commit whose removed range overlaps an added range is reported (one finding per file); line-range overlap is a heuristic across history, and lockfiles/generated/binary paths are skipped. Fail-safe on missing token/invalid slug/fetch error or an aborted signal.", + }, + }, ] 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 8fad1b54e0..86b48de311 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -1427,6 +1427,34 @@ "network": "Pure local analyzer. No external network call; the curated list is bundled.", "notes": "Conservative: only an exact match against the bundled list is flagged, so a package it does not name is never reported. Bounded by manifest, patch-line, and finding caps; fail-safe on absent patches or an aborted signal." } + }, + { + "name": "revertRecurrence", + "title": "Revert recurrence", + "category": "history", + "cost": "github-heavy", + "defaultEnabled": true, + "profiles": [ + "balanced", + "deep" + ], + "requires": [ + "files", + "github-token" + ], + "limits": { + "maxFilesProbed": 5, + "commitsPerFile": 15, + "maxRevertLookups": 10, + "maxFindings": 25 + }, + "docs": { + "summary": "Flags a changed file where the PR re-introduces added lines in a region a prior revert commit removed — a signal it may be re-treading a path that was already reverted or hot-fixed out.", + "looksAt": "For each changed file's added line-ranges, the file's recent commit history and the patch of any revert commit in it, intersecting the reverted (removed) old-file line-ranges with this PR's added new-file line-ranges.", + "reports": "File, a re-introduced line, a short revert commit-SHA prefix, and the reverted PR number when the revert message names one — never file contents.", + "network": "Calls the GitHub commits API per probed file and the single-commit API per revert commit, both bounded by fixed fanout caps. Requires GitHub token forwarding for private repos.", + "notes": "Conservative: only a message-confirmed revert commit whose removed range overlaps an added range is reported (one finding per file); line-range overlap is a heuristic across history, and lockfiles/generated/binary paths are skipped. Fail-safe on missing token/invalid slug/fetch error or an aborted signal." + } } ] } diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 21960cd780..e26dea04e1 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -49,6 +49,7 @@ import { scanExhaustivenessDrift } from "./exhaustiveness-drift.js"; import { scanFlakyTest } from "./flaky-test.js"; import { scanApiBreak } from "./api-break.js"; import { scanDeprecatedDependencies } from "./deprecated-dep.js"; +import { scanRevertRecurrence } from "./revert-recurrence.js"; import type { AnalyzerDescriptor, AnalyzerFn, @@ -1446,6 +1447,48 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req, { signal }) => scanDeprecatedDependencies(req, signal), }), + descriptor({ + name: "revertRecurrence", + title: "Revert recurrence", + category: "history", + cost: "github-heavy", + defaultEnabled: true, + requires: ["files", "github-token"], + limits: { + maxFilesProbed: 5, + commitsPerFile: 15, + maxRevertLookups: 10, + maxFindings: 25, + }, + docs: { + summary: + "Flags a changed file where the PR re-introduces added lines in a region a prior revert commit removed — a signal it may be re-treading a path that was already reverted or hot-fixed out.", + looksAt: + "For each changed file's added line-ranges, the file's recent commit history and the patch of any revert commit in it, intersecting the reverted (removed) old-file line-ranges with this PR's added new-file line-ranges.", + reports: + "File, a re-introduced line, a short revert commit-SHA prefix, and the reverted PR number when the revert message names one — never file contents.", + network: + "Calls the GitHub commits API per probed file and the single-commit API per revert commit, both bounded by fixed fanout caps. Requires GitHub token forwarding for private repos.", + notes: + "Conservative: only a message-confirmed revert commit whose removed range overlaps an added range is reported (one finding per file); line-range overlap is a heuristic across history, and lockfiles/generated/binary paths are skipped. Fail-safe on missing token/invalid slug/fetch error or an aborted signal.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = [ + "### Revert recurrence (re-introduces code from a previously reverted region)", + ]; + for (const item of findings) { + const pr = + item.revertedPr !== undefined ? ` (revert of #${item.revertedPr})` : ""; + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} overlaps a region removed by revert commit ${helpers.safeCodeSpan(item.revertShaPrefix)}${pr}`, + ); + } + return lines; + }, + run: (req, { signal, analysis, diagnostics }) => + scanRevertRecurrence(req, fetch, { signal, analysis, diagnostics }), + }), ] as const satisfies readonly AnyAnalyzerDescriptor[]; export const ANALYZER_NAMES = ANALYZER_DESCRIPTORS.map( diff --git a/review-enrichment/src/analyzers/revert-recurrence.ts b/review-enrichment/src/analyzers/revert-recurrence.ts new file mode 100644 index 0000000000..de2e549639 --- /dev/null +++ b/review-enrichment/src/analyzers/revert-recurrence.ts @@ -0,0 +1,257 @@ +// Revert-recurrence analyzer (#1514, part of #1499). For the files a PR changes, reads each file's recent commit +// history from the GitHub API, finds the revert commits in it, fetches each revert commit's patch, and flags the +// file when the PR RE-INTRODUCES added lines in a region that revert previously REMOVED — a signal the change may +// be re-treading a known-bad path that was already reverted/hot-fixed out. This is heavy/external/historical +// analysis the no-checkout `claude --print` reviewer cannot do. Surfaces only the file, a re-introduced line, and +// a short revert-commit SHA prefix (plus the reverted PR number when the revert names one) — never file contents. +// Distinct from the churn-hotspot analyzer (#1513, defect DENSITY of the area) and the history analyzer (#1478, +// the AUTHOR's track record); this intersects the PR's added lines with a specific past revert's removed lines. +import type { + AnalyzerDiagnostics, + EnrichRequest, + RevertRecurrenceFinding, +} from "../types.js"; +import type { AnalysisContext } from "../analysis-context.js"; +import { boundedFetchJson } from "../external-fetch.js"; +import { isHistoryUninformativePath } from "./history-path.js"; + +const GITHUB_API = "https://api.github.com"; +const SLUG_RE = /^[A-Za-z0-9._-]+$/; +const MAX_FILES_PROBED = 5; // bound the per-file commit-history fan-out, matching the other history-class analyzers +const COMMITS_PER_FILE = 15; // recent commits to inspect per probed file when looking for a revert +const MAX_REVERT_LOOKUPS = 10; // global cap on revert-commit detail fetches across all probed files +const MAX_FINDINGS = 25; +// GitHub's auto-generated revert commit has a `Revert " (#N)"` subject; a hand-written revert body +// carries the `This reverts commit ` trailer. Either shape confirms a revert without diff-classifying a patch. +const REVERT_SUBJECT_RE = /^revert\b/i; +const REVERT_BODY_RE = /this reverts commit [0-9a-f]{7,40}/i; +const REVERT_PR_RE = /\brevert\s+"[^"]*\(#(\d+)\)"/i; + +/** An inclusive 1-based line range within a file. */ +export interface Range { + start: number; + end: number; +} + +interface ScanOptions { + signal?: AbortSignal; + analysis?: Pick; + diagnostics?: AnalyzerDiagnostics; +} + +/** The slice of a GitHub commit-list item this analyzer reads. */ +interface CommitListItem { + sha?: string; + commit?: { message?: string }; +} + +/** The slice of a GitHub single-commit response this analyzer reads. */ +interface CommitDetail { + files?: Array<{ filename?: string; patch?: string }>; +} + +/** True when a commit message describes a revert — a `Revert "…"` subject or a `This reverts commit ` body. Pure. */ +export function isRevertCommit(message: string): boolean { + if (!message) return false; + const subject = message.split("\n", 1)[0]?.trim() ?? ""; + return REVERT_SUBJECT_RE.test(subject) || REVERT_BODY_RE.test(message); +} + +/** The PR number a revert undid, from GitHub's `Revert " (#N)"` subject shape; undefined otherwise. Pure. */ +export function revertedPrNumber(message: string): number | undefined { + const match = REVERT_PR_RE.exec(message ?? ""); + if (!match) return undefined; + const number = Number(match[1]); + return Number.isInteger(number) && number > 0 ? number : undefined; +} + +/** Contiguous line-ranges on one side of a unified-diff patch: `"+"` = added new-file lines, `"-"` = removed + * old-file lines. Operates on GitHub's per-file `patch` (which has no `+++`/`---` file headers). Pure — returns + * [] for an empty patch or one with no valid hunk header. */ +export function diffLineRanges(patch: string, side: "+" | "-"): Range[] { + const ranges: Range[] = []; + if (!patch) return ranges; + let oldLine = 0; + let newLine = 0; + let active = false; + let runStart = 0; + let runEnd = 0; + let inRun = false; + const flush = (): void => { + if (inRun) { + ranges.push({ start: runStart, end: runEnd }); + inRun = false; + } + }; + for (const line of patch.split("\n")) { + if (line.startsWith("@@")) { + flush(); + const header = /^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (!header) { + active = false; + continue; + } + oldLine = Number(header[1]); + newLine = Number(header[2]); + active = true; + continue; + } + if (!active) continue; + if (line.startsWith("\\")) continue; // "\ No newline at end of file" + const marker = line[0]; + if (marker === "+") { + if (side === "+") { + if (!inRun) { + inRun = true; + runStart = newLine; + } + runEnd = newLine; + } else { + flush(); + } + newLine += 1; + } else if (marker === "-") { + if (side === "-") { + if (!inRun) { + inRun = true; + runStart = oldLine; + } + runEnd = oldLine; + } else { + flush(); + } + oldLine += 1; + } else { + flush(); + oldLine += 1; + newLine += 1; + } + } + flush(); + return ranges; +} + +/** True when two inclusive integer ranges overlap. Pure. */ +export function rangesOverlap(a: Range, b: Range): boolean { + return a.start <= b.end && b.start <= a.end; +} + +/** The first `left` range that overlaps any `right` range, or null when none do. Pure. */ +export function firstOverlap(left: Range[], right: Range[]): Range | null { + for (const a of left) { + for (const b of right) { + if (rangesOverlap(a, b)) return a; + } + } + return null; +} + +function githubHeaders(token: string): Record<string, string> { + return { + Authorization: `Bearer ${token}`, + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; +} + +/** Fetch + parse JSON with the shared bounded-fetch guard rails. Returns the parsed body, or null on any + * error / non-200 so the caller degrades that one lookup rather than throwing. */ +async function fetchGithubJson<T>( + url: string, + headers: Record<string, string>, + fetchFn: typeof fetch, + category: string, + maxCalls: number, + options: ScanOptions, +): Promise<T | null> { + const fetchOptions = { + endpointCategory: category, + headers, + signal: options.signal, + fetchImpl: fetchFn, + diagnostics: options.diagnostics, + phase: "revert-recurrence", + subcall: category, + maxBytes: 512 * 1024, + maxCallsPerCategory: maxCalls, + }; + const response = options.analysis + ? await options.analysis.fetchJson<T>(url, fetchOptions) + : await boundedFetchJson<T>(url, fetchOptions); + return response.ok ? response.data : null; +} + +/** Analyzer entrypoint: changed files → per-file recent commit history → revert commits → line-range overlap with + * this PR's added lines. Fail-safe: any missing token/invalid slug/failed fetch/aborted signal degrades to []. */ +export async function scanRevertRecurrence( + req: EnrichRequest, + fetchFn: typeof fetch = fetch, + options: ScanOptions = {}, +): Promise<RevertRecurrenceFinding[]> { + const { repoFullName, githubToken, files = [] } = req; + if (!githubToken) return []; + const parts = repoFullName.split("/"); + if (parts.length !== 2) return []; + const [owner, repo] = parts; + if (!owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo)) return []; + + const headers = githubHeaders(githubToken); + // Only a file with re-introduced (added) lines can re-tread a reverted region; skip lockfiles/generated/binaries + // (no informative history) and any file whose patch adds nothing, before spending a round-trip. + const probes = files + .filter((file) => !isHistoryUninformativePath(file.path)) + .map((file) => ({ path: file.path, added: diffLineRanges(file.patch ?? "", "+") })) + .filter((probe) => probe.added.length > 0) + .slice(0, MAX_FILES_PROBED); + + const findings: RevertRecurrenceFinding[] = []; + let revertLookups = 0; + + for (const probe of probes) { + if (options.signal?.aborted) break; + const commitsUrl = + `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits` + + `?path=${encodeURIComponent(probe.path)}&per_page=${COMMITS_PER_FILE}`; + const commits = await fetchGithubJson<CommitListItem[]>( + commitsUrl, + headers, + fetchFn, + "github-commits", + MAX_FILES_PROBED, + options, + ); + if (!Array.isArray(commits)) continue; + + for (const commit of commits) { + if (revertLookups >= MAX_REVERT_LOOKUPS) break; + const sha = commit.sha; + const message = commit.commit?.message ?? ""; + if (typeof sha !== "string" || !isRevertCommit(message)) continue; + revertLookups += 1; + const detailUrl = `${GITHUB_API}/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/commits/${encodeURIComponent(sha)}`; + const detail = await fetchGithubJson<CommitDetail>( + detailUrl, + headers, + fetchFn, + "github-commit-detail", + MAX_REVERT_LOOKUPS, + options, + ); + const filePatch = detail?.files?.find((file) => file.filename === probe.path)?.patch; + if (!filePatch) continue; + const overlap = firstOverlap(probe.added, diffLineRanges(filePatch, "-")); + if (!overlap) continue; + const finding: RevertRecurrenceFinding = { + file: probe.path, + line: overlap.start, + revertShaPrefix: sha.slice(0, 7), + }; + const pr = revertedPrNumber(message); + if (pr !== undefined) finding.revertedPr = pr; + findings.push(finding); + break; // one finding per file — the most recent revert whose removed region this PR re-introduces + } + } + + return findings.slice(0, MAX_FINDINGS); +} diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index be451fa054..66a4dada67 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -500,6 +500,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("commitLint", findings.commitLint)); lines.push(...renderDescriptorSection("apiBreak", findings.apiBreak)); lines.push(...renderDescriptorSection("deprecatedDep", findings.deprecatedDep)); + lines.push(...renderDescriptorSection("revertRecurrence", findings.revertRecurrence)); if (!lines.length) return { promptSection: "", systemSuffix: "" }; diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 39bb3ab5da..743dc842fd 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -613,6 +613,22 @@ export interface ApiBreakFinding { symbol: string; } +/** A changed file where the PR re-introduces added lines in a region a PRIOR revert commit removed — a signal the + * change may be re-treading a known-bad path that was already reverted/hot-fixed out (#1514, part of #1499). + * Derived from the public commit history and the revert commit's own patch: the PR's added new-file line ranges + * are intersected with the old-file line ranges the revert deleted in the same file. Reports the file, a + * re-introduced line, and a short revert commit-SHA prefix (with the reverted PR number when the revert message + * names one) — never file contents. */ +export interface RevertRecurrenceFinding { + file: string; + /** New-file line where the PR's re-introduced range that overlaps the reverted region begins. */ + line: number; + /** Short prefix of the prior revert commit's SHA (prefix only — never the full SHA). */ + revertShaPrefix: string; + /** The PR the revert undid, when the revert message names it (`Revert "<title> (#N)"`). */ + revertedPr?: number; +} + export interface BriefFindings { dependency?: DependencyFinding[]; dependencyDiff?: DependencyDiffFinding[]; @@ -665,6 +681,7 @@ export interface BriefFindings { commitLint?: CommitLintFinding[]; apiBreak?: ApiBreakFinding[]; deprecatedDep?: DeprecatedDependencyFinding[]; + revertRecurrence?: RevertRecurrenceFinding[]; } /** 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 23da7aafbf..26b5a558c5 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -61,6 +61,7 @@ const EXPECTED_ANALYZERS = [ "commitLint", "apiBreak", "deprecatedDep", + "revertRecurrence", ]; test("analyzer descriptors cover the runtime registry in stable order", () => { diff --git a/review-enrichment/test/revert-recurrence.test.ts b/review-enrichment/test/revert-recurrence.test.ts new file mode 100644 index 0000000000..c493316512 --- /dev/null +++ b/review-enrichment/test/revert-recurrence.test.ts @@ -0,0 +1,270 @@ +// Units for the revert-recurrence analyzer (#1514). 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 { + isRevertCommit, + revertedPrNumber, + diffLineRanges, + rangesOverlap, + firstOverlap, + scanRevertRecurrence, +} from "../dist/analyzers/revert-recurrence.js"; +import { renderBrief } from "../dist/render.js"; +import { resetExternalFetchCircuitBreakerForTest } from "../dist/external-fetch.js"; + +const jsonResponse = (body, code = 200) => new Response(JSON.stringify(body), { status: code }); + +// A PR patch that adds new-file lines 11-12 to src/a.ts. +const PR_PATCH = "@@ -10,3 +10,4 @@\n ctx1\n+new11\n+new12\n ctx2"; +const REVERT_SHA = "abcdef1234567"; +// A revert commit whose patch removed old-file lines 11-12 of src/a.ts — overlaps the PR's added 11-12. +const OVERLAP_LIST = [ + { sha: REVERT_SHA, commit: { message: 'Revert "feat: thing (#42)"\n\nThis reverts commit deadbeef1234567.' } }, +]; +const OVERLAP_DETAIL = { + [REVERT_SHA]: { files: [{ filename: "src/a.ts", patch: "@@ -11,2 +11,0 @@\n-old11\n-old12" }] }, +}; + +const req = (files) => ({ repoFullName: "octo/repo", prNumber: 1, githubToken: "ghp_test", files }); + +// Route by URL: the single-commit detail endpoint is `/commits/<sha>`; the commits-by-path list is `/commits?…`. +const routed = (list, detailBySha) => async (url) => { + if (url.includes("/commits/")) { + const sha = url.split("/commits/")[1]; + return jsonResponse(detailBySha[sha] ?? { files: [] }); + } + return jsonResponse(list); +}; + +test("isRevertCommit: true for a revert subject or a 'This reverts commit' body; false otherwise", () => { + assert.equal(isRevertCommit('Revert "feat: x (#42)"'), true); + assert.equal(isRevertCommit("feat: x\n\nThis reverts commit deadbeef123456."), true); + assert.equal(isRevertCommit("revert bad change"), true); + assert.equal(isRevertCommit("feat: reverting the plan later"), false); // 'reverting' isn't a whole word, no body trailer + assert.equal(isRevertCommit("fix: normal change"), false); + assert.equal(isRevertCommit(""), false); +}); + +test("revertedPrNumber: extracts the reverted PR number, else undefined", () => { + assert.equal(revertedPrNumber('Revert "feat: thing (#42)"'), 42); + assert.equal(revertedPrNumber("This reverts commit deadbeef."), undefined); + assert.equal(revertedPrNumber('Revert "weird (#0)"'), undefined); // #0 rejected (must be > 0) +}); + +test("diffLineRanges: extracts added (new-file) and removed (old-file) contiguous ranges", () => { + const patch = "@@ -10,3 +10,4 @@\n ctx\n+a\n+b\n ctx2\n-gone"; + assert.deepEqual(diffLineRanges(patch, "+"), [{ start: 11, end: 12 }]); + assert.deepEqual(diffLineRanges(patch, "-"), [{ start: 12, end: 12 }]); +}); + +test("diffLineRanges: empty, no-hunk, and malformed-header patches yield []", () => { + assert.deepEqual(diffLineRanges("", "+"), []); + assert.deepEqual(diffLineRanges("no hunk here\n+notcounted", "+"), []); // no @@ header ⇒ inactive + assert.deepEqual(diffLineRanges("@@ bad header @@\n+x", "+"), []); // header regex fails ⇒ inactive +}); + +test("diffLineRanges: spans multiple hunks and skips 'No newline' markers", () => { + const patch = "@@ -1,1 +1,2 @@\n+a\n b\n@@ -10,1 +11,2 @@\n+c\n\\ No newline at end of file\n+d"; + assert.deepEqual(diffLineRanges(patch, "+"), [{ start: 1, end: 1 }, { start: 11, end: 12 }]); +}); + +test("rangesOverlap + firstOverlap: inclusive overlap detection", () => { + assert.equal(rangesOverlap({ start: 1, end: 3 }, { start: 3, end: 5 }), true); // touch at 3 + assert.equal(rangesOverlap({ start: 1, end: 2 }, { start: 3, end: 4 }), false); + assert.deepEqual( + firstOverlap([{ start: 1, end: 2 }, { start: 10, end: 12 }], [{ start: 11, end: 11 }]), + { start: 10, end: 12 }, + ); + assert.equal(firstOverlap([{ start: 1, end: 2 }], [{ start: 5, end: 6 }]), null); +}); + +test("scanRevertRecurrence: flags a re-introduced range overlapping a reverted region", async () => { + const findings = await scanRevertRecurrence( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed(OVERLAP_LIST, OVERLAP_DETAIL), + ); + assert.equal(findings.length, 1); + assert.deepEqual(findings[0], { + file: "src/a.ts", + line: 11, + revertShaPrefix: "abcdef1", + revertedPr: 42, + }); +}); + +test("scanRevertRecurrence: a body-style revert with no PR number omits revertedPr", async () => { + const list = [{ sha: REVERT_SHA, commit: { message: "This reverts commit deadbeef1234567." } }]; + const findings = await scanRevertRecurrence( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed(list, OVERLAP_DETAIL), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].revertShaPrefix, "abcdef1"); + assert.equal("revertedPr" in findings[0], false); +}); + +test("scanRevertRecurrence: no revert commit in history ⇒ no finding and no detail fetch", async () => { + let detailCalled = false; + const out = await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), async (url) => { + if (url.includes("/commits/")) { + detailCalled = true; + return jsonResponse({ files: [] }); + } + return jsonResponse([{ sha: "1111111", commit: { message: "feat: add thing" } }]); + }); + assert.deepEqual(out, []); + assert.equal(detailCalled, false); +}); + +test("scanRevertRecurrence: a revert that doesn't overlap the PR's added lines is not flagged", async () => { + const detail = { [REVERT_SHA]: { files: [{ filename: "src/a.ts", patch: "@@ -100,2 +100,0 @@\n-x\n-y" }] } }; + assert.deepEqual( + await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), routed(OVERLAP_LIST, detail)), + [], + ); +}); + +test("scanRevertRecurrence: a revert whose patch doesn't touch the probed file is not flagged", async () => { + const detail = { [REVERT_SHA]: { files: [{ filename: "src/other.ts", patch: "@@ -11,2 +11,0 @@\n-a\n-b" }] } }; + assert.deepEqual( + await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), routed(OVERLAP_LIST, detail)), + [], + ); +}); + +test("scanRevertRecurrence: a commit missing a sha is skipped, a later valid revert still flags", async () => { + const list = [ + { commit: { message: 'Revert "no sha (#7)"' } }, + { sha: REVERT_SHA, commit: { message: 'Revert "feat (#42)"' } }, + ]; + const findings = await scanRevertRecurrence( + req([{ path: "src/a.ts", patch: PR_PATCH }]), + routed(list, OVERLAP_DETAIL), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].revertedPr, 42); +}); + +test("scanRevertRecurrence: a file whose patch adds nothing is not probed", async () => { + let called = false; + const out = await scanRevertRecurrence( + req([{ path: "src/a.ts", patch: "@@ -5,2 +5,0 @@\n-gone1\n-gone2" }]), + async () => { + called = true; + return jsonResponse([]); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanRevertRecurrence: skips lockfiles and binaries without fetching", async () => { + let called = false; + const out = await scanRevertRecurrence( + req([ + { path: "package-lock.json", patch: PR_PATCH }, + { path: "assets/logo.png", patch: PR_PATCH }, + ]), + async () => { + called = true; + return jsonResponse([]); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanRevertRecurrence: requires a github token and a single valid repo slug", async () => { + assert.deepEqual( + await scanRevertRecurrence( + { repoFullName: "octo/repo", prNumber: 1, files: [{ path: "src/a.ts", patch: PR_PATCH }] }, + routed(OVERLAP_LIST, OVERLAP_DETAIL), + ), + [], + ); + assert.deepEqual( + await scanRevertRecurrence( + { repoFullName: "bad slug/x!", prNumber: 1, githubToken: "t", files: [{ path: "src/a.ts", patch: PR_PATCH }] }, + routed(OVERLAP_LIST, OVERLAP_DETAIL), + ), + [], + ); +}); + +test("scanRevertRecurrence: rejects multi-segment repo slugs without fetching", async () => { + let called = false; + const out = await scanRevertRecurrence( + { repoFullName: "octo/repo/extra", prNumber: 1, githubToken: "ghp_test", files: [{ path: "src/a.ts", patch: PR_PATCH }] }, + async () => { + called = true; + return jsonResponse(OVERLAP_LIST); + }, + ); + assert.deepEqual(out, []); + assert.equal(called, false); +}); + +test("scanRevertRecurrence: caps the number of revert-commit detail lookups", async () => { + // 12 revert commits, each detail non-overlapping ⇒ no finding; the global lookup cap (10) stops the scan. + const many = Array.from({ length: 12 }, (_, i) => ({ + sha: `abc10${10 + i}`, + commit: { message: 'Revert "x (#1)"' }, + })); + const capMock = async (url) => { + if (url.includes("/commits/")) { + return jsonResponse({ files: [{ filename: "src/a.ts", patch: "@@ -100,1 +100,0 @@\n-x" }] }); + } + return jsonResponse(many); + }; + assert.deepEqual(await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), capMock), []); +}); + +test("scanRevertRecurrence: fails safe on a non-ok or throwing list fetch", async () => { + resetExternalFetchCircuitBreakerForTest(); + assert.deepEqual( + await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), async () => jsonResponse({}, 500)), + [], + ); + assert.deepEqual( + await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), async () => { + throw new Error("network"); + }), + [], + ); +}); + +test("scanRevertRecurrence: fails safe when the revert-detail fetch fails", async () => { + resetExternalFetchCircuitBreakerForTest(); + const out = await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), async (url) => + url.includes("/commits/") ? jsonResponse({}, 500) : jsonResponse(OVERLAP_LIST), + ); + assert.deepEqual(out, []); +}); + +test("scanRevertRecurrence: stops on an already-aborted signal", async () => { + assert.deepEqual( + await scanRevertRecurrence(req([{ path: "src/a.ts", patch: PR_PATCH }]), routed(OVERLAP_LIST, OVERLAP_DETAIL), { + signal: AbortSignal.abort(), + }), + [], + ); +}); + +test("renderBrief emits a public-safe revert-recurrence block", () => { + const { promptSection } = renderBrief({ + revertRecurrence: [ + { file: "src/a.ts", line: 11, revertShaPrefix: "abcdef1", revertedPr: 42 }, + { file: "src/b.ts", line: 5, revertShaPrefix: "1234567" }, + ], + }); + assert.match(promptSection, /Revert recurrence/); + assert.match(promptSection, /src\/a\.ts:11/); + assert.match(promptSection, /abcdef1/); + assert.match(promptSection, /revert of #42/); + assert.match(promptSection, /src\/b\.ts:5/); +}); + +test("renderBrief omits the revert-recurrence block when there are no findings", () => { + assert.equal(renderBrief({ revertRecurrence: [] }).promptSection, ""); +}); diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index a0d39486e8..ffdb9b8608 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -55,6 +55,7 @@ export const REES_ANALYZER_NAMES = [ "commitLint", "apiBreak", "deprecatedDep", + "revertRecurrence", ] as const; export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number];