diff --git a/review-enrichment/src/analyzers/revert-recurrence.ts b/review-enrichment/src/analyzers/revert-recurrence.ts new file mode 100644 index 0000000000..6f143e1f72 --- /dev/null +++ b/review-enrichment/src/analyzers/revert-recurrence.ts @@ -0,0 +1,109 @@ +// Revert-recurrence analyzer (#1696). Flags PRs that revert prior work or re-introduce churn patterns +// associated with regression risk — explicit revert titles, rollback language, and symmetric file churn +// where a change mostly deletes what a prior PR added (or vice versa). Pure text/diff analysis, no network. +import type { EnrichRequest, RevertRecurrenceFinding } from "../types.js"; + +const MAX_FINDINGS = 15; +const MAX_TITLE_CHARS = 500; +const MAX_BODY_CHARS = 4000; + +const EXPLICIT_REVERT_TITLE = /^revert[\s:]/i; +const EXPLICIT_REVERT_BODY = + /\b(reverts commit|this reverts|reverted in|revert of)\b/i; +const ROLLBACK_LANGUAGE = + /\b(rollback|roll back|rolled back|undo(?:ing)?|revert(?:ing|ed)?)\b/i; + +interface FileChurn { + path: string; + additions: number; + deletions: number; +} + +/** Count `+`/`-` hunk lines per file from unified diff patches (bounded). */ +export function summarizeFileChurn( + files: NonNullable, +): FileChurn[] { + const churn: FileChurn[] = []; + for (const file of files) { + if (!file.patch) continue; + let additions = 0; + let deletions = 0; + for (const line of file.patch.split("\n", 2000)) { + if (line.startsWith("+++") || line.startsWith("---") || line.startsWith("@@")) + continue; + if (line.startsWith("+")) additions += 1; + else if (line.startsWith("-")) deletions += 1; + } + if (additions + deletions > 0) { + churn.push({ path: file.path, additions, deletions }); + } + } + return churn; +} + +/** True when one side dominates — classic revert/re-apply shape (large deletions + few additions or vice versa). */ +export function isSymmetricChurn(entry: FileChurn): boolean { + const total = entry.additions + entry.deletions; + if (total < 12) return false; + const dominant = Math.max(entry.additions, entry.deletions); + const minor = Math.min(entry.additions, entry.deletions); + if (dominant < 8) return false; + if (minor === 0) return true; + return dominant / minor >= 4; +} + +/** Scan title/body for explicit revert or rollback language. */ +export function detectRevertLanguage( + title: string | undefined, + body: string | undefined, +): RevertRecurrenceFinding[] { + const findings: RevertRecurrenceFinding[] = []; + const safeTitle = (title ?? "").slice(0, MAX_TITLE_CHARS); + const safeBody = (body ?? "").slice(0, MAX_BODY_CHARS); + + if (EXPLICIT_REVERT_TITLE.test(safeTitle.trim())) { + findings.push({ + kind: "explicit-revert", + detail: `PR title signals an explicit revert: ${safeTitle.trim().slice(0, 120)}`, + confidence: "high", + }); + } else if (EXPLICIT_REVERT_BODY.test(safeBody)) { + findings.push({ + kind: "explicit-revert", + detail: "PR body references reverting a prior commit or change set", + confidence: "high", + }); + } + + if ( + ROLLBACK_LANGUAGE.test(safeTitle) || + ROLLBACK_LANGUAGE.test(safeBody) + ) { + findings.push({ + kind: "rollback-language", + detail: + "PR title or body uses rollback/undo language — verify this is intentional and covered by tests", + confidence: "medium", + }); + } + + return findings; +} + +/** Analyzer entrypoint: language signals + symmetric churn on changed files. */ +export async function scanRevertRecurrence( + req: EnrichRequest, +): Promise { + const findings = detectRevertLanguage(req.title, req.body); + const churn = summarizeFileChurn(req.files ?? []); + const churnFiles = churn.filter(isSymmetricChurn).map((c) => c.path); + if (churnFiles.length) { + findings.push({ + kind: "symmetric-churn", + detail: `${churnFiles.length} changed file(s) show revert-shaped symmetric churn (large deletions with few additions or vice versa)`, + files: churnFiles.slice(0, 10), + confidence: churnFiles.length >= 3 ? "high" : "medium", + }); + } + return findings.slice(0, MAX_FINDINGS); +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index ccb30f107b..399b37855c 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -16,6 +16,7 @@ import { scanEol } from "./analyzers/eol-check.js"; import { scanRedos } from "./analyzers/redos.js"; import { scanCodeowners } from "./analyzers/codeowners.js"; import { scanSecretLog } from "./analyzers/secret-log.js"; +import { scanRevertRecurrence } from "./analyzers/revert-recurrence.js"; import { renderBrief } from "./render.js"; type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise; @@ -31,6 +32,7 @@ const ANALYZERS: Record = { redos: (req) => scanRedos(req), codeowners: (req, signal) => scanCodeowners(req, fetch, { signal }), secretLog: (req, signal) => scanSecretLog(req, signal), + revertRecurrence: (req) => scanRevertRecurrence(req), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 5270f795e0..571fad86ac 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -160,6 +160,20 @@ export function renderBrief( } } + const revertSignals = findings.revertRecurrence ?? []; + if (revertSignals.length) { + lines.push( + "### Revert / regression risk (verify intent and add regression coverage)", + ); + for (const item of revertSignals) { + const files = + item.files?.length ? ` — files: ${item.files.map((f) => safeCodeSpan(f)).join(", ")}` : ""; + lines.push( + `- **${item.confidence}** (${item.kind}): ${promptText(item.detail)}${files}`, + ); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 0136d320af..e062c2135f 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -109,6 +109,14 @@ export interface SecretLogFinding { category: "secret" | "pii" | "request-object"; } +/** A revert/regression signal: explicit revert language or symmetric churn that mirrors undoing prior work. */ +export interface RevertRecurrenceFinding { + kind: "explicit-revert" | "rollback-language" | "symmetric-churn"; + detail: string; + files?: string[]; + confidence: "high" | "medium"; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -120,6 +128,7 @@ export interface BriefFindings { redos?: RedosFinding[]; codeowners?: CodeownersFinding[]; secretLog?: SecretLogFinding[]; + revertRecurrence?: RevertRecurrenceFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/revert-recurrence.test.ts b/review-enrichment/test/revert-recurrence.test.ts new file mode 100644 index 0000000000..7395db9e33 --- /dev/null +++ b/review-enrichment/test/revert-recurrence.test.ts @@ -0,0 +1,107 @@ +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { + detectRevertLanguage, + isSymmetricChurn, + scanRevertRecurrence, + summarizeFileChurn, +} from "../src/analyzers/revert-recurrence.ts"; + +describe("revert-recurrence analyzer", () => { + it("detects explicit revert titles and body references", () => { + const titleFindings = detectRevertLanguage( + 'Revert "feat: add cache layer"', + undefined, + ); + assert.equal(titleFindings[0]?.kind, "explicit-revert"); + assert.equal(titleFindings[0]?.confidence, "high"); + assert.match(titleFindings[0]!.detail, /Revert/i); + + const bodyFindings = detectRevertLanguage( + "fix: patch", + "This reverts commit abc123 from PR #42", + ); + assert.equal(bodyFindings[0]?.kind, "explicit-revert"); + }); + + it("flags rollback language separately from explicit revert titles", () => { + const findings = detectRevertLanguage("Rollback auth middleware change", ""); + assert.ok(findings.some((f) => f.kind === "rollback-language")); + }); + + it("summarizes per-file churn from patches", () => { + const churn = summarizeFileChurn([ + { + path: "src/a.ts", + patch: [ + "@@", + "+line1", + "+line2", + "+line3", + "-old1", + "-old2", + "-old3", + "-old4", + "-old5", + "-old6", + "-old7", + "-old8", + ].join("\n"), + }, + ]); + assert.equal(churn.length, 1); + assert.deepEqual(churn[0], { + path: "src/a.ts", + additions: 3, + deletions: 8, + }); + }); + + it("recognizes symmetric churn shapes", () => { + assert.equal( + isSymmetricChurn({ path: "a.ts", additions: 2, deletions: 20 }), + true, + ); + assert.equal( + isSymmetricChurn({ path: "a.ts", additions: 3, deletions: 3 }), + false, + ); + assert.equal( + isSymmetricChurn({ path: "a.ts", additions: 1, deletions: 1 }), + false, + ); + }); + + it("scanRevertRecurrence combines language and churn signals", async () => { + const findings = await scanRevertRecurrence({ + repoFullName: "o/r", + prNumber: 1, + title: "Revert risky refactor", + body: "", + files: [ + { + path: "src/core.ts", + patch: [ + "@@ -1,12 +1,2 @@", + "-export function old() {}", + "-export function old2() {}", + "-export function old3() {}", + "-export function old4() {}", + "-export function old5() {}", + "-export function old6() {}", + "-export function old7() {}", + "-export function old8() {}", + "-export function old9() {}", + "-export function old10() {}", + "-export function old11() {}", + "+export function newOnly() {}", + ].join("\n"), + }, + ], + }); + assert.ok(findings.some((f) => f.kind === "explicit-revert")); + assert.ok(findings.some((f) => f.kind === "symmetric-churn")); + const churn = findings.find((f) => f.kind === "symmetric-churn"); + assert.ok(churn?.files?.includes("src/core.ts")); + }); +});