Skip to content
Closed
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
98 changes: 98 additions & 0 deletions review-enrichment/src/analyzers/revert-recurrence.ts
Original file line number Diff line number Diff line change
@@ -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 <sha>`); (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<string>; removed: Set<string> } {
const added = new Set<string>();
const removed = new Set<string>();
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<RevertRecurrenceFinding[]> {
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;
}
2 changes: 2 additions & 0 deletions review-enrichment/src/brief.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -41,6 +42,7 @@ const ANALYZERS: Record<keyof BriefFindings, AnalyzerFn> = {
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<T>(
Expand Down
14 changes: 14 additions & 0 deletions review-enrichment/src/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
17 changes: 17 additions & 0 deletions review-enrichment/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand All @@ -177,6 +193,7 @@ export interface BriefFindings {
secretLog?: SecretLogFinding[];
assetWeight?: AssetWeightFinding[];
typosquat?: TyposquatFinding[];
revertRecurrence?: RevertRecurrenceFinding[];
}

export type AnalyzerStatus = "ok" | "degraded" | "skipped";
Expand Down
100 changes: 100 additions & 0 deletions review-enrichment/test/revert-recurrence.test.ts
Original file line number Diff line number Diff line change
@@ -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>): 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 <sha>' 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/);
});