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
109 changes: 109 additions & 0 deletions review-enrichment/src/analyzers/revert-recurrence.ts
Original file line number Diff line number Diff line change
@@ -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<EnrichRequest["files"]>,
): 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<RevertRecurrenceFinding[]> {
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);
}
2 changes: 2 additions & 0 deletions review-enrichment/src/brief.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<unknown>;
Expand All @@ -31,6 +32,7 @@ const ANALYZERS: Record<keyof BriefFindings, AnalyzerFn> = {
redos: (req) => scanRedos(req),
codeowners: (req, signal) => scanCodeowners(req, fetch, { signal }),
secretLog: (req, signal) => scanSecretLog(req, 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 @@ -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 =
Expand Down
9 changes: 9 additions & 0 deletions review-enrichment/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand All @@ -120,6 +128,7 @@ export interface BriefFindings {
redos?: RedosFinding[];
codeowners?: CodeownersFinding[];
secretLog?: SecretLogFinding[];
revertRecurrence?: RevertRecurrenceFinding[];
}

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