From 0be2805270fb49c50208f230ca803eb46eb2d7f3 Mon Sep 17 00:00:00 2001 From: jeffrey701 Date: Mon, 29 Jun 2026 13:07:49 -0400 Subject: [PATCH] feat(enrichment): add revert-recurrence detector for review brief MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a REES analyzer that flags when a PR is reverting or re-introducing previously reverted work — a common regression / review-churn source the no-checkout reviewer cannot spot at a glance. Two signals, from the request the engine already has (no repo checkout): - explicit-revert: revert/rollback/re-introduce language in the PR title or body, including GitHub's `Revert "…"` titles (captures the subject) and `reverts commit ` (captures the sha). - symmetric-churn: a file whose diff removes lines that re-appear as additions (overlap >= 3 and >= half the larger side) — the textual fingerprint of a revert / re-introduce, while ordinary mostly-new or mostly-deleted edits are not flagged. Wires the analyzer into the registry (brief.ts), the structured findings (types.ts), and a dedicated "Revert / re-introduce recurrence" brief section (render.ts). Fail-safe: returns [] on empty input. Tests cover explicit titles/bodies, the commit-sha body line, free-text rollback language, symmetric churn (incl. ignoring +++/--- headers), non-revert edits, empty input, and the rendered section. Closes #1696 --- .../src/analyzers/revert-recurrence.ts | 98 +++++++++++++++++ review-enrichment/src/brief.ts | 2 + review-enrichment/src/render.ts | 14 +++ review-enrichment/src/types.ts | 17 +++ .../test/revert-recurrence.test.ts | 100 ++++++++++++++++++ 5 files changed, 231 insertions(+) create mode 100644 review-enrichment/src/analyzers/revert-recurrence.ts create mode 100644 review-enrichment/test/revert-recurrence.test.ts diff --git a/review-enrichment/src/analyzers/revert-recurrence.ts b/review-enrichment/src/analyzers/revert-recurrence.ts new file mode 100644 index 0000000000..bee51635a7 --- /dev/null +++ b/review-enrichment/src/analyzers/revert-recurrence.ts @@ -0,0 +1,98 @@ +// Revert-recurrence analyzer (#1696). Flags when a PR is reverting or re-introducing previously reverted +// work — a common source of regressions and review churn the no-checkout `claude --print` reviewer cannot +// see at a glance. Two signals: (1) explicit revert/rollback language in the PR title or body (including +// GitHub's `Revert "…"` titles and `reverts commit `); (2) symmetric churn in a file's diff, where the +// lines removed re-appear as additions — the textual fingerprint of a revert / re-introduce. Pure + offline: +// it reads only the request the engine already has (title, body, file patches), so it needs no repo checkout. +import type { EnrichRequest, RevertRecurrenceFinding } from "../types.js"; + +const MAX_FILES = 50; +// A symmetric-churn flag needs enough removed-then-re-added lines to be meaningful (not a one-line tweak) and +// that overlap must dominate the change, so an ordinary edit (mostly new or mostly deleted lines) is not flagged. +const MIN_CHURN_OVERLAP = 3; +const MIN_CHURN_RATIO = 0.5; + +// GitHub's auto-generated revert title, e.g. `Revert "Add feature X"`. +const GITHUB_REVERT_TITLE = /Revert\s+"(.+?)"/i; +// `This reverts commit 0a1b2c3…` (git's revert body line; 7–40 hex). +const REVERTS_COMMIT = /\breverts?\s+commit\s+([0-9a-f]{7,40})\b/i; +// Free-text revert/rollback/re-introduce language anywhere in the title or body. +const REVERT_WORDS = /\b(reverts?|reverting|reverted|rollbacks?|roll\s+back|re-?introduc\w*|re-?appl(?:y|ies|ied))\b/i; + +function detectExplicit(source: "title" | "body", text: string | undefined): RevertRecurrenceFinding | null { + const value = typeof text === "string" ? text : ""; + if (!value.trim()) return null; + + const titleMatch = value.match(GITHUB_REVERT_TITLE); + if (titleMatch) { + const subject = titleMatch[1]!.trim(); + return { kind: "explicit-revert", source, revertedSubject: subject, reason: `PR ${source} reverts "${subject}"` }; + } + + const commitMatch = value.match(REVERTS_COMMIT); + if (commitMatch) { + const sha = commitMatch[1]!; + return { kind: "explicit-revert", source, revertedSubject: sha, reason: `PR ${source} reverts commit ${sha}` }; + } + + if (REVERT_WORDS.test(value)) { + return { kind: "explicit-revert", source, reason: `PR ${source} contains revert/rollback/re-introduce language` }; + } + + return null; +} + +// Count distinct non-empty added vs removed line bodies in a unified-diff patch (ignoring the +++/--- file headers). +function patchLineSets(patch: string): { added: Set; removed: Set } { + const added = new Set(); + const removed = new Set(); + for (const raw of patch.split("\n")) { + if (raw.startsWith("+++") || raw.startsWith("---")) continue; + if (raw.startsWith("+")) { + const line = raw.slice(1).trim(); + if (line) added.add(line); + } else if (raw.startsWith("-")) { + const line = raw.slice(1).trim(); + if (line) removed.add(line); + } + } + return { added, removed }; +} + +function detectSymmetricChurn(path: string, patch: string): RevertRecurrenceFinding | null { + const { added, removed } = patchLineSets(patch); + if (added.size === 0 || removed.size === 0) return null; + let overlap = 0; + for (const line of removed) { + if (added.has(line)) overlap += 1; + } + const larger = Math.max(added.size, removed.size); + if (overlap >= MIN_CHURN_OVERLAP && overlap / larger >= MIN_CHURN_RATIO) { + return { + kind: "symmetric-churn", + path, + churnedLines: overlap, + reason: `${overlap} line${overlap === 1 ? "" : "s"} removed and re-added in the same file — symmetric churn typical of reverting or re-introducing work`, + }; + } + return null; +} + +/** Analyzer entrypoint: explicit revert language (title/body) + symmetric revert churn (per file). Pure; [] on empty input. */ +export async function scanRevertRecurrence(req: EnrichRequest): Promise { + const findings: RevertRecurrenceFinding[] = []; + + const titleFinding = detectExplicit("title", req?.title); + if (titleFinding) findings.push(titleFinding); + const bodyFinding = detectExplicit("body", req?.body); + if (bodyFinding) findings.push(bodyFinding); + + const files = Array.isArray(req?.files) ? req.files.slice(0, MAX_FILES) : []; + for (const file of files) { + if (!file || typeof file.patch !== "string" || typeof file.path !== "string" || !file.path) continue; + const churn = detectSymmetricChurn(file.path, file.patch); + if (churn) findings.push(churn); + } + + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 7550f7db89..ac1d30384a 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -20,6 +20,7 @@ import { scanCodeowners } from "./analyzers/codeowners.js"; import { scanSecretLog } from "./analyzers/secret-log.js"; import { scanAssetWeight } from "./analyzers/asset-weight.js"; import { scanTyposquat } from "./analyzers/typosquat.js"; +import { scanRevertRecurrence } from "./analyzers/revert-recurrence.js"; import { renderBrief } from "./render.js"; import { captureAnalyzerDegradation } from "./sentry.js"; @@ -41,6 +42,7 @@ const ANALYZERS: Record = { secretLog: (req, signal) => scanSecretLog(req, signal), assetWeight: (req, signal) => scanAssetWeight(req, fetch, { signal }), typosquat: (req, signal) => scanTyposquat(req, fetch, { signal }), + revertRecurrence: (req) => scanRevertRecurrence(req), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index e9fc157841..8a0b732e32 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -254,6 +254,20 @@ export function renderBrief( } } + const reverts = findings.revertRecurrence ?? []; + if (reverts.length) { + lines.push( + "### Revert / re-introduce recurrence (watch for regressions)", + ); + for (const item of reverts) { + const where = + item.kind === "explicit-revert" + ? `PR ${item.source ?? "text"}` + : safeCodeSpan(item.path ?? ""); + lines.push(`- ${where}: ${item.reason}`); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 70dcf66619..a20079f2da 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -162,6 +162,22 @@ export interface TyposquatFinding { reason: string; } +export interface RevertRecurrenceFinding { + /** `explicit-revert`: revert/rollback language in the PR title or body. `symmetric-churn`: a file + * whose diff removes lines that re-appear as additions — the churn pattern of reverting/re-introducing work. */ + kind: "explicit-revert" | "symmetric-churn"; + /** Where an explicit revert signal was found (set for `explicit-revert`). */ + source?: "title" | "body"; + /** File path carrying revert-like symmetric churn (set for `symmetric-churn`). */ + path?: string; + /** The subject or commit an explicit revert points at, when parseable. */ + revertedSubject?: string; + /** Count of distinct lines removed and re-added in the file (set for `symmetric-churn`). */ + churnedLines?: number; + /** Short, public-safe explanation of why the change was flagged. */ + reason: string; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -177,6 +193,7 @@ export interface BriefFindings { secretLog?: SecretLogFinding[]; assetWeight?: AssetWeightFinding[]; typosquat?: TyposquatFinding[]; + 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..e592731b22 --- /dev/null +++ b/review-enrichment/test/revert-recurrence.test.ts @@ -0,0 +1,100 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { scanRevertRecurrence } from "../dist/analyzers/revert-recurrence.js"; +import { renderBrief } from "../dist/render.js"; +import type { EnrichRequest } from "../dist/types.js"; + +function req(overrides: Partial): EnrichRequest { + return { repoFullName: "acme/app", prNumber: 1, ...overrides }; +} + +test("flags a GitHub-style Revert title with the reverted subject", async () => { + const findings = await scanRevertRecurrence(req({ title: 'Revert "Add streaming upload (#412)"' })); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.kind, "explicit-revert"); + assert.equal(findings[0]!.source, "title"); + assert.equal(findings[0]!.revertedSubject, "Add streaming upload (#412)"); +}); + +test("flags a 'reverts commit ' body line with the sha", async () => { + const findings = await scanRevertRecurrence(req({ title: "Roll back upload", body: "This reverts commit 0a1b2c3d4e5f6a7b." })); + // Title carries rollback language AND the body carries an explicit commit revert. + assert.equal(findings.length, 2); + const body = findings.find((f) => f.source === "body")!; + assert.equal(body.kind, "explicit-revert"); + assert.equal(body.revertedSubject, "0a1b2c3d4e5f6a7b"); +}); + +test("flags free-text rollback / re-introduce language", async () => { + assert.equal((await scanRevertRecurrence(req({ body: "Re-introduces the cache layer we removed last week." })))[0]?.kind, "explicit-revert"); + assert.equal((await scanRevertRecurrence(req({ title: "Rollback the flaky retry change" })))[0]?.kind, "explicit-revert"); +}); + +test("does not flag ordinary titles/bodies that merely mention nearby words", async () => { + // "reverted" etc. absent; "diversion"/"convert" must not trip the word boundary. + const findings = await scanRevertRecurrence(req({ title: "Convert config to a diversion-free loader", body: "A normal change." })); + assert.deepEqual(findings, []); +}); + +test("flags symmetric churn: lines removed then re-added in the same file", async () => { + const patch = [ + "@@ -1,4 +1,4 @@", + "-const a = computeA();", + "-const b = computeB();", + "-const c = computeC();", + "+const a = computeA();", + "+const b = computeB();", + "+const c = computeC();", + ].join("\n"); + const findings = await scanRevertRecurrence(req({ files: [{ path: "src/x.ts", patch }] })); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.kind, "symmetric-churn"); + assert.equal(findings[0]!.path, "src/x.ts"); + assert.equal(findings[0]!.churnedLines, 3); +}); + +test("does not flag an ordinary edit (mostly new lines, little overlap)", async () => { + const patch = ["@@ -1,1 +1,4 @@", "-const a = 1;", "+const a = 1;", "+const b = 2;", "+const c = 3;", "+const d = 4;"].join("\n"); + // overlap = 1 (only `const a = 1;`), below MIN_CHURN_OVERLAP and ratio. + assert.deepEqual(await scanRevertRecurrence(req({ files: [{ path: "src/x.ts", patch }] })), []); +}); + +test("ignores +++/--- file headers and blank lines when measuring churn", async () => { + const patch = [ + "--- a/src/y.ts", + "+++ b/src/y.ts", + "@@ -1,3 +1,3 @@", + "-keep one", + "-keep two", + "-keep three", + "+keep one", + "+keep two", + "+keep three", + ].join("\n"); + const findings = await scanRevertRecurrence(req({ files: [{ path: "src/y.ts", patch }] })); + assert.equal(findings.length, 1); + assert.equal(findings[0]!.churnedLines, 3); +}); + +test("fail-safe: empty input and missing fields yield []", async () => { + assert.deepEqual(await scanRevertRecurrence(req({})), []); + assert.deepEqual(await scanRevertRecurrence(req({ title: "", body: "", files: [] })), []); + assert.deepEqual(await scanRevertRecurrence(req({ files: [{ path: "src/z.ts" }] })), []); // no patch +}); + +test("renderBrief emits a dedicated revert-recurrence section when findings exist", () => { + const { promptSection } = renderBrief({ + revertRecurrence: [ + { kind: "explicit-revert", source: "title", revertedSubject: "Add X", reason: 'PR title reverts "Add X"' }, + { kind: "symmetric-churn", path: "src/x.ts", churnedLines: 4, reason: "4 lines removed and re-added in the same file — symmetric churn typical of reverting or re-introducing work" }, + ], + }); + assert.match(promptSection, /### Revert \/ re-introduce recurrence/); + assert.match(promptSection, /PR title: PR title reverts "Add X"/); + assert.match(promptSection, /`src\/x\.ts`: 4 lines removed and re-added/); +}); + +test("renderBrief omits the section when there are no revert findings", () => { + const { promptSection } = renderBrief({ revertRecurrence: [] }); + assert.doesNotMatch(promptSection, /Revert \/ re-introduce/); +});