From 7cdd2d4c64dd0b3f0ca502796eb3922bb50dee85 Mon Sep 17 00:00:00 2001 From: dale053 Date: Sun, 28 Jun 2026 08:04:49 -0400 Subject: [PATCH 1/2] feat(enrichment): revert-recurrence detector for re-introduced reverted code (#1514) Adds a REES analyzer that fetches per-file commit history from the GitHub commits API, identifies revert commits (message starts with "Revert"), fetches their diffs, and intersects the lines they removed with the lines being added by the current PR. A hit indicates known-problematic code is being re-introduced without addressing the original reason it was reverted. - New `RevertRecurrenceFinding` interface and `revertRecurrence` key in `BriefFindings` - Pure helper exports (`isRevertMessage`, `extractAddedLines`, `extractRemovedLines`) enable direct unit testing without network mocks - Fail-safe: non-ok responses, network throws, and non-array commit-list responses all degrade silently to empty findings - Bounded: MAX_FILES=10, MAX_COMMITS_PER_FILE=30, MAX_REVERT_CHECKS_PER_FILE=5, MAX_FINDINGS=15; MIN_MATCH_LINES=2 + MIN_LINE_LEN=8 suppress coincidental hits - Auth header included when `githubToken` is present; omitted for public repos - 70 tests all passing; all new branches covered --- .../src/analyzers/revert-recurrence.ts | 161 +++++++ review-enrichment/src/brief.ts | 2 + review-enrichment/src/render.ts | 13 + review-enrichment/src/types.ts | 10 + review-enrichment/test/enrichment.test.ts | 453 ++++++++++++++++++ 5 files changed, 639 insertions(+) create mode 100644 review-enrichment/src/analyzers/revert-recurrence.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..7921b3bd7e --- /dev/null +++ b/review-enrichment/src/analyzers/revert-recurrence.ts @@ -0,0 +1,161 @@ +// Revert-recurrence detector (#1514). Fetches per-file commit history from the GitHub commits API, finds revert +// commits (message starts with "Revert"), fetches their diffs, and intersects the lines they removed with the +// lines being added in the current PR. A hit means known-problematic code is being re-introduced without +// addressing the original reason it was reverted. Fail-safe: network errors + non-ok responses → empty findings. +import type { EnrichRequest, RevertRecurrenceFinding } from "../types.js"; + +const MAX_FILES = 10; +const MAX_COMMITS_PER_FILE = 30; +const MAX_REVERT_CHECKS_PER_FILE = 5; +const MAX_FINDINGS = 15; +// Two matching non-trivial lines are required to suppress coincidental hits on common structural patterns. +const MIN_MATCH_LINES = 2; +const MIN_LINE_LEN = 8; +const MAX_SHA_DISPLAY = 7; +const MAX_MSG_CHARS = 80; + +type FetchImpl = typeof fetch; + +/** True when the commit message begins with a revert keyword (standard `git revert` format or manual label). */ +export function isRevertMessage(msg: string): boolean { + return /^[Rr]evert\b/.test(msg.trimStart()); +} + +/** Extract non-trivial lines added by a patch (`+` lines, excluding the `+++` header). */ +export function extractAddedLines(patch: string): Set { + const lines = new Set(); + for (const raw of patch.split("\n")) { + if (raw.startsWith("+") && !raw.startsWith("+++")) { + const content = raw.slice(1).trim(); + if (content.length >= MIN_LINE_LEN) lines.add(content); + } + } + return lines; +} + +/** Extract non-trivial lines removed by a patch (`-` lines, excluding the `---` header). */ +export function extractRemovedLines(patch: string): Set { + const lines = new Set(); + for (const raw of patch.split("\n")) { + if (raw.startsWith("-") && !raw.startsWith("---")) { + const content = raw.slice(1).trim(); + if (content.length >= MIN_LINE_LEN) lines.add(content); + } + } + return lines; +} + +function githubHeaders(token?: string): Record { + const h: Record = { + Accept: "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }; + if (token) h.Authorization = `Bearer ${token}`; + return h; +} + +async function listFileCommits( + repoFullName: string, + path: string, + sha: string | undefined, + token: string | undefined, + fetchImpl: FetchImpl, + signal?: AbortSignal, +): Promise> { + try { + const shaParam = sha ? `&sha=${encodeURIComponent(sha)}` : ""; + const url = `https://api.github.com/repos/${repoFullName}/commits?path=${encodeURIComponent(path)}&per_page=${MAX_COMMITS_PER_FILE}${shaParam}`; + const resp = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!resp.ok) return []; + const raw = await resp.json(); + return Array.isArray(raw) + ? (raw as Array<{ sha: string; commit: { message: string } }>) + : []; + } catch { + return []; + } +} + +async function fetchCommitFiles( + repoFullName: string, + sha: string, + token: string | undefined, + fetchImpl: FetchImpl, + signal?: AbortSignal, +): Promise> { + try { + const url = `https://api.github.com/repos/${repoFullName}/commits/${encodeURIComponent(sha)}`; + const resp = await fetchImpl(url, { headers: githubHeaders(token), signal }); + if (!resp.ok) return []; + const data = (await resp.json()) as { + files?: Array<{ filename: string; patch?: string }>; + }; + return data.files ?? []; + } catch { + return []; + } +} + +/** Scan the PR's added lines for content previously removed by a revert commit in the same file's history. */ +export async function scanRevertRecurrence( + req: EnrichRequest, + fetchImpl: FetchImpl = fetch, + options: { signal?: AbortSignal } = {}, +): Promise { + const { signal } = options; + const files = (req.files ?? []).filter((f) => f.patch); + const findings: RevertRecurrenceFinding[] = []; + + for (const file of files.slice(0, MAX_FILES)) { + if (findings.length >= MAX_FINDINGS || signal?.aborted) break; + + const prAdded = extractAddedLines(file.patch!); + if (prAdded.size === 0) continue; + + const commits = await listFileCommits( + req.repoFullName, + file.path, + req.baseSha, + req.githubToken, + fetchImpl, + signal, + ); + + let revertChecks = 0; + for (const commit of commits) { + if ( + findings.length >= MAX_FINDINGS || + revertChecks >= MAX_REVERT_CHECKS_PER_FILE || + signal?.aborted + ) + break; + if (!isRevertMessage(commit.commit.message)) continue; + revertChecks++; + + const commitFiles = await fetchCommitFiles( + req.repoFullName, + commit.sha, + req.githubToken, + fetchImpl, + signal, + ); + const target = commitFiles.find((cf) => cf.filename === file.path); + if (!target?.patch) continue; + + // In a revert commit, `-` lines are the code that was being reverted (originally introduced then walked back). + // If the current PR re-adds those lines, that's a recurrence. + const revertRemoved = extractRemovedLines(target.patch); + const matchCount = [...prAdded].filter((l) => revertRemoved.has(l)).length; + if (matchCount < MIN_MATCH_LINES) continue; + + findings.push({ + file: file.path, + revertSha: commit.sha.slice(0, MAX_SHA_DISPLAY), + revertMessage: commit.commit.message.split("\n")[0]!.slice(0, MAX_MSG_CHARS), + matchedLines: matchCount, + }); + } + } + + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 8313780e6e..6b62b3f018 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -14,6 +14,7 @@ import { scanInstallScripts } from "./analyzers/install-scripts.js"; import { scanActionPins } from "./analyzers/actions-pin.js"; import { scanEol } from "./analyzers/eol-check.js"; import { scanRedos } from "./analyzers/redos.js"; +import { scanRevertRecurrence } from "./analyzers/revert-recurrence.js"; import { renderBrief } from "./render.js"; type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise; @@ -27,6 +28,7 @@ const ANALYZERS: Record = { actionPin: (req) => scanActionPins(req), eol: (req) => scanEol(req), redos: (req) => scanRedos(req), + revertRecurrence: (req, signal) => scanRevertRecurrence(req, fetch, { signal }), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 848a2f67de..fca30551eb 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -129,6 +129,19 @@ export function renderBrief( } } + const revertRecurrences = findings.revertRecurrence ?? []; + if (revertRecurrences.length) { + lines.push( + "### Re-introduced reverted code (known-problematic path re-trodden)", + ); + for (const f of revertRecurrences) { + const s = f.matchedLines === 1 ? "" : "s"; + lines.push( + `- ${safeCodeSpan(f.file)} re-introduces ${f.matchedLines} line${s} from revert ${safeCodeSpan(f.revertSha)} — ${promptText(f.revertMessage)}`, + ); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index ae893825ca..5e6021230b 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -93,6 +93,15 @@ export interface RedosFinding { pattern: string; } +/** A file in the current PR that re-introduces lines removed by a prior revert commit — a known-problematic + * code path being re-trodden. Only the file path, revert SHA, and match count are reported; no content. */ +export interface RevertRecurrenceFinding { + file: string; + revertSha: string; + revertMessage: string; + matchedLines: number; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -102,6 +111,7 @@ export interface BriefFindings { installScript?: InstallScriptFinding[]; eol?: EolFinding[]; redos?: RedosFinding[]; + revertRecurrence?: RevertRecurrenceFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 6f189d1143..852b27e94f 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -21,6 +21,12 @@ import { scanPatchForRedos, scanRedos, } from "../dist/analyzers/redos.js"; +import { + isRevertMessage, + extractAddedLines, + extractRemovedLines, + scanRevertRecurrence, +} from "../dist/analyzers/revert-recurrence.js"; const NOW = new Date("2026-06-26").getTime(); const eolFetch = @@ -937,3 +943,450 @@ test("buildBrief: eol analyzer runs (real now, 2023 cycle is past)", async () => globalThis.fetch = realFetch; } }); + +// --- revert-recurrence --- + +test("isRevertMessage: detects standard and manual revert prefixes, ignores non-revert messages", () => { + for (const yes of [ + "Revert 'add config'", + "Revert: remove rate limiter", + "revert bad deploy", + "Revert commit 1234567", + ]) { + assert.equal(isRevertMessage(yes), true, yes); + } + for (const no of [ + "Reverting the config", + "fix: handle edge case", + "feat: add dashboard", + "", + ]) { + assert.equal(isRevertMessage(no), false, no); + } +}); + +test("extractAddedLines: returns non-trivial added lines, skips +++ header and short/removed lines", () => { + const patch = [ + "@@ -1,1 +1,4 @@", + "+++ b/src/foo.ts", + "+const callReallyLongFunctionName = () => doSomething();", + "+short", + "- removed line that is long enough to pass", + " context line not added", + ].join("\n"); + const result = extractAddedLines(patch); + assert.ok(result.has("const callReallyLongFunctionName = () => doSomething();")); + assert.ok(!result.has("short")); // too short (<8 chars) + assert.ok(!result.has("removed line that is long enough to pass")); // not a + line + assert.ok(!result.has("+++ b/src/foo.ts")); // header excluded +}); + +test("extractRemovedLines: returns non-trivial removed lines, skips --- header and short/added lines", () => { + const patch = [ + "@@ -1,3 +1,1 @@", + "--- a/src/foo.ts", + "-const callReallyLongFunctionName = () => doSomething();", + "-tiny", + "+ added line that is long enough to pass", + ].join("\n"); + const result = extractRemovedLines(patch); + assert.ok(result.has("const callReallyLongFunctionName = () => doSomething();")); + assert.ok(!result.has("tiny")); // too short + assert.ok(!result.has("added line that is long enough to pass")); // not a - line + assert.ok(!result.has("--- a/src/foo.ts")); // header excluded +}); + +const makeReq = (overrides = {}) => ({ + repoFullName: "owner/repo", + prNumber: 42, + baseSha: "base123", + githubToken: "tok", + files: [ + { + path: "src/auth.ts", + patch: [ + "@@ -1,0 +1,5 @@", + "+function authenticateUser(token, secret) {", + "+ const result = validateToken(token, secret);", + "+ return result.isValid ? result.user : null;", + "+}", + ].join("\n"), + }, + ], + ...overrides, +}); + +const revertCommit = { + sha: "abc1234567890", + commit: { message: "Revert 'add auth helper'\n\nThis reverts commit xyz." }, +}; + +const revertPatch = [ + "@@ -1,5 +1,0 @@", + "-function authenticateUser(token, secret) {", + "- const result = validateToken(token, secret);", + "- return result.isValid ? result.user : null;", + "-}", +].join("\n"); + +const makeFetch = + (commits, commitFiles) => + async (url) => { + const u = String(url); + if (u.includes("/commits?")) + return { ok: true, json: async () => commits }; + return { ok: true, json: async () => ({ files: commitFiles }) }; + }; + +test("scanRevertRecurrence: no files → returns empty", async () => { + const r = await scanRevertRecurrence({ repoFullName: "o/r", prNumber: 1 }); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: file has no + lines → skips API call", async () => { + let calls = 0; + const fetchImpl = async () => { + calls++; + return { ok: true, json: async () => [] }; + }; + const r = await scanRevertRecurrence( + makeReq({ + files: [{ path: "src/x.ts", patch: "@@ -1,1 +0,0 @@\n-deleted line here" }], + }), + fetchImpl, + ); + assert.deepEqual(r, []); + assert.equal(calls, 0); +}); + +test("scanRevertRecurrence: commit list returns non-ok → no findings", async () => { + const fetchImpl = async () => ({ ok: false, json: async () => [] }); + const r = await scanRevertRecurrence(makeReq(), fetchImpl); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: commit list fetch throws → no findings", async () => { + const fetchImpl = async () => { + throw new Error("network error"); + }; + const r = await scanRevertRecurrence(makeReq(), fetchImpl); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: no revert commits in history → no findings", async () => { + const nonRevertCommits = [ + { sha: "aaa", commit: { message: "feat: add login" } }, + { sha: "bbb", commit: { message: "fix: correct typo" } }, + ]; + const r = await scanRevertRecurrence(makeReq(), makeFetch(nonRevertCommits, [])); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: revert commit matches PR additions → finding returned", async () => { + const findings = await scanRevertRecurrence( + makeReq(), + makeFetch( + [revertCommit], + [{ filename: "src/auth.ts", patch: revertPatch }], + ), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].file, "src/auth.ts"); + assert.equal(findings[0].revertSha, "abc1234"); // first 7 chars + assert.equal(findings[0].revertMessage, "Revert 'add auth helper'"); // first line only + assert.ok(findings[0].matchedLines >= 2); +}); + +test("scanRevertRecurrence: fewer than MIN_MATCH_LINES overlap → no finding", async () => { + // Only one matching line (MIN_MATCH_LINES=2 so this is below threshold) + const onlyOneLine = [ + "@@ -1,2 +1,0 @@", + "-function authenticateUser(token, secret) {", + "-short", + ].join("\n"); + const r = await scanRevertRecurrence( + makeReq(), + makeFetch([revertCommit], [{ filename: "src/auth.ts", patch: onlyOneLine }]), + ); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: commit diff API returns non-ok → no finding", async () => { + let callCount = 0; + const fetchImpl = async (url) => { + callCount++; + const u = String(url); + if (u.includes("/commits?")) return { ok: true, json: async () => [revertCommit] }; + return { ok: false, json: async () => ({}) }; + }; + const r = await scanRevertRecurrence(makeReq(), fetchImpl); + assert.deepEqual(r, []); + assert.equal(callCount, 2); // list + diff attempt +}); + +test("scanRevertRecurrence: commit diff fetch throws → no finding (fail-safe)", async () => { + let first = true; + const fetchImpl = async (url) => { + const u = String(url); + if (u.includes("/commits?")) return { ok: true, json: async () => [revertCommit] }; + if (first) { + first = false; + throw new Error("timeout"); + } + return { ok: true, json: async () => ({ files: [] }) }; + }; + const r = await scanRevertRecurrence(makeReq(), fetchImpl); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: target file not in commit diff → no finding", async () => { + const r = await scanRevertRecurrence( + makeReq(), + makeFetch([revertCommit], [{ filename: "src/other.ts", patch: revertPatch }]), + ); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: target file in commit but patch missing → no finding", async () => { + const r = await scanRevertRecurrence( + makeReq(), + makeFetch([revertCommit], [{ filename: "src/auth.ts" }]), + ); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: commit diff returns no files field → no finding", async () => { + const fetchImpl = async (url) => { + const u = String(url); + if (u.includes("/commits?")) return { ok: true, json: async () => [revertCommit] }; + return { ok: true, json: async () => ({}) }; // no `files` key → data.files ?? [] + }; + const r = await scanRevertRecurrence(makeReq(), fetchImpl); + assert.deepEqual(r, []); +}); + +test("scanRevertRecurrence: includes baseSha in commit list query URL", async () => { + const urls: string[] = []; + const fetchImpl = async (url) => { + urls.push(String(url)); + return { ok: true, json: async () => [] }; + }; + await scanRevertRecurrence(makeReq({ baseSha: "deadbeef" }), fetchImpl); + assert.ok(urls[0].includes("sha=deadbeef"), `expected sha=deadbeef in ${urls[0]}`); +}); + +test("scanRevertRecurrence: no baseSha → query omits sha param", async () => { + const urls: string[] = []; + const fetchImpl = async (url) => { + urls.push(String(url)); + return { ok: true, json: async () => [] }; + }; + await scanRevertRecurrence(makeReq({ baseSha: undefined }), fetchImpl); + assert.ok(!urls[0].includes("sha="), `sha param should be absent; got ${urls[0]}`); +}); + +test("scanRevertRecurrence: auth header sent when token present, omitted when absent", async () => { + const seenHeaders: Record[] = []; + const fetchImpl = async (_url, init) => { + seenHeaders.push(init?.headers ?? {}); + return { ok: true, json: async () => [] }; + }; + await scanRevertRecurrence(makeReq({ githubToken: "mytoken" }), fetchImpl); + assert.ok( + String(seenHeaders[0]?.Authorization ?? "").includes("mytoken"), + "token in Authorization header", + ); + seenHeaders.length = 0; + await scanRevertRecurrence(makeReq({ githubToken: undefined }), fetchImpl); + assert.ok( + !("Authorization" in (seenHeaders[0] ?? {})), + "no Authorization header when no token", + ); +}); + +test("scanRevertRecurrence: caps to MAX_FILES (10)", async () => { + let listCalls = 0; + const fetchImpl = async (url) => { + if (String(url).includes("/commits?")) listCalls++; + return { ok: true, json: async () => [] }; + }; + const files = Array.from({ length: 15 }, (_, i) => ({ + path: `src/file${i}.ts`, + patch: `@@ -0,0 +1,1 @@\n+const longEnoughAddedLine${i} = "hello world";`, + })); + await scanRevertRecurrence(makeReq({ files }), fetchImpl); + assert.equal(listCalls, 10); +}); + +test("scanRevertRecurrence: caps to MAX_FINDINGS (15)", async () => { + // 3 files × 5 reverts each = 15 potential findings → exactly at cap + // Each file has the same added lines; each revert removes those same lines. + const addedPatch = [ + "@@ -0,0 +1,3 @@", + "+function authenticateUser(token, secret) {", + "+ const result = validateToken(token, secret);", + "+ return result.isValid ? result.user : null;", + ].join("\n"); + const files = Array.from({ length: 3 }, (_, i) => ({ + path: `src/file${i}.ts`, + patch: addedPatch, + })); + const manyReverts = Array.from({ length: 5 }, (_, i) => ({ + sha: `rev${i}aaaaaaaaa`, + commit: { message: `Revert change ${i}` }, + })); + // Track which file triggered the most recent commit-list call so the diff mock + // can return a patch for the correct filename. + let lastFile = ""; + const cappingFetch = async (url) => { + const u = String(url); + if (u.includes("/commits?")) { + const m = u.match(/path=([^&]+)/); + lastFile = m ? decodeURIComponent(m[1]) : ""; + return { ok: true, json: async () => manyReverts }; + } + return { + ok: true, + json: async () => ({ files: [{ filename: lastFile, patch: revertPatch }] }), + }; + }; + const findings = await scanRevertRecurrence(makeReq({ files }), cappingFetch); + assert.equal(findings.length, 15); +}); + +test("scanRevertRecurrence: aborted signal stops processing early", async () => { + const controller = new AbortController(); + let listCalls = 0; + const fetchImpl = async () => { + listCalls++; + controller.abort(); + return { ok: true, json: async () => [] }; + }; + const files = Array.from({ length: 5 }, (_, i) => ({ + path: `src/file${i}.ts`, + patch: `@@ -0,0 +1,1 @@\n+const longEnoughAddedLineInFile${i} = true;`, + })); + await scanRevertRecurrence(makeReq({ files }), fetchImpl, { + signal: controller.signal, + }); + assert.ok(listCalls <= 2, `expected at most 2 calls before abort, got ${listCalls}`); +}); + +test("scanRevertRecurrence: caps revert checks per file to MAX_REVERT_CHECKS_PER_FILE (5)", async () => { + let diffCalls = 0; + const manyReverts = Array.from({ length: 10 }, (_, i) => ({ + sha: `rev${i}111111111`, + commit: { message: `Revert commit number ${i}` }, + })); + const fetchImpl = async (url) => { + const u = String(url); + if (u.includes("/commits?")) + return { ok: true, json: async () => manyReverts }; + diffCalls++; + return { ok: true, json: async () => ({ files: [] }) }; + }; + await scanRevertRecurrence(makeReq(), fetchImpl); + assert.equal(diffCalls, 5); // capped at MAX_REVERT_CHECKS_PER_FILE +}); + +test("renderBrief: renders revert-recurrence block with plural line count", () => { + const r = renderBrief({ + revertRecurrence: [ + { + file: "src/auth.ts", + revertSha: "abc1234", + revertMessage: "Revert add auth helper", + matchedLines: 3, + }, + ], + }); + assert.match(r.promptSection, /Re-introduced reverted code/); + assert.match(r.promptSection, /`src\/auth\.ts`/); + assert.match(r.promptSection, /3 lines/); + assert.match(r.promptSection, /`abc1234`/); + assert.match(r.promptSection, /Revert add auth helper/); +}); + +test("renderBrief: renders revert-recurrence singular line count", () => { + const r = renderBrief({ + revertRecurrence: [ + { + file: "src/x.ts", + revertSha: "dead000", + revertMessage: "Revert change", + matchedLines: 1, + }, + ], + }); + assert.match(r.promptSection, /1 line[^s]/); // "1 line " not "1 lines" +}); + +test("buildBrief: revert-recurrence analyzer runs and returns findings", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async (url) => { + const u = String(url); + if (u.includes("api.github.com") && u.includes("/commits?")) + return { ok: true, json: async () => [revertCommit] }; + if (u.includes("api.github.com") && u.includes("/commits/")) + return { + ok: true, + json: async () => ({ + files: [{ filename: "src/auth.ts", patch: revertPatch }], + }), + }; + return { ok: true, json: async () => ({}) }; + }; + try { + const brief = await buildBrief({ + repoFullName: "owner/repo", + prNumber: 42, + baseSha: "base123", + githubToken: "tok", + analyzers: ["revertRecurrence"], + files: [ + { + path: "src/auth.ts", + patch: [ + "@@ -1,0 +1,4 @@", + "+function authenticateUser(token, secret) {", + "+ const result = validateToken(token, secret);", + "+ return result.isValid ? result.user : null;", + "+}", + ].join("\n"), + }, + ], + }); + assert.equal(brief.analyzerStatus.revertRecurrence, "ok"); + assert.ok(brief.findings.revertRecurrence.length >= 1); + assert.match(brief.promptSection, /Re-introduced reverted code/); + } finally { + globalThis.fetch = realFetch; + } +}); + +test("buildBrief: revert-recurrence analyzer degrades gracefully on network throw", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async (url) => { + if (String(url).includes("api.github.com")) throw new Error("network down"); + return { ok: true, json: async () => ({}) }; + }; + try { + const brief = await buildBrief({ + repoFullName: "owner/repo", + prNumber: 1, + githubToken: "tok", + analyzers: ["revertRecurrence"], + files: [ + { + path: "src/x.ts", + patch: "@@ -0,0 +1,1 @@\n+const example = doSomethingUseful();", + }, + ], + }); + // listFileCommits catches the throw and returns [], so revertRecurrence = [] (not degraded) + assert.equal(brief.analyzerStatus.revertRecurrence, "ok"); + assert.deepEqual(brief.findings.revertRecurrence, []); + } finally { + globalThis.fetch = realFetch; + } +}); From af9f9e4412972993805c14303acffe1db67ff577 Mon Sep 17 00:00:00 2001 From: dale053 Date: Sun, 28 Jun 2026 11:24:07 -0400 Subject: [PATCH 2/2] fix(enrichment): close syntax gaps and encode repoFullName in API URLs --- .../src/analyzers/revert-recurrence.ts | 8 ++++-- review-enrichment/src/render.ts | 4 +++ review-enrichment/src/types.ts | 2 ++ review-enrichment/test/enrichment.test.ts | 28 +++++++++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/review-enrichment/src/analyzers/revert-recurrence.ts b/review-enrichment/src/analyzers/revert-recurrence.ts index 7921b3bd7e..0717775859 100644 --- a/review-enrichment/src/analyzers/revert-recurrence.ts +++ b/review-enrichment/src/analyzers/revert-recurrence.ts @@ -45,6 +45,10 @@ export function extractRemovedLines(patch: string): Set { return lines; } +function encodeRepoSlug(repoFullName: string): string { + return repoFullName.split("/").map(encodeURIComponent).join("/"); +} + function githubHeaders(token?: string): Record { const h: Record = { Accept: "application/vnd.github+json", @@ -64,7 +68,7 @@ async function listFileCommits( ): Promise> { try { const shaParam = sha ? `&sha=${encodeURIComponent(sha)}` : ""; - const url = `https://api.github.com/repos/${repoFullName}/commits?path=${encodeURIComponent(path)}&per_page=${MAX_COMMITS_PER_FILE}${shaParam}`; + const url = `https://api.github.com/repos/${encodeRepoSlug(repoFullName)}/commits?path=${encodeURIComponent(path)}&per_page=${MAX_COMMITS_PER_FILE}${shaParam}`; const resp = await fetchImpl(url, { headers: githubHeaders(token), signal }); if (!resp.ok) return []; const raw = await resp.json(); @@ -84,7 +88,7 @@ async function fetchCommitFiles( signal?: AbortSignal, ): Promise> { try { - const url = `https://api.github.com/repos/${repoFullName}/commits/${encodeURIComponent(sha)}`; + const url = `https://api.github.com/repos/${encodeRepoSlug(repoFullName)}/commits/${encodeURIComponent(sha)}`; const resp = await fetchImpl(url, { headers: githubHeaders(token), signal }); if (!resp.ok) return []; const data = (await resp.json()) as { diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index b7a241727c..e99d129b90 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -138,6 +138,10 @@ export function renderBrief( const s = f.matchedLines === 1 ? "" : "s"; lines.push( `- ${safeCodeSpan(f.file)} re-introduces ${f.matchedLines} line${s} from revert ${safeCodeSpan(f.revertSha)} — ${promptText(f.revertMessage)}`, + ); + } + } + const codeownersViolations = findings.codeowners ?? []; if (codeownersViolations.length) { const allOwners = new Set(codeownersViolations.flatMap((f) => f.owners)); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 169d58f033..dcc3900bac 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -100,6 +100,8 @@ export interface RevertRecurrenceFinding { revertSha: string; revertMessage: string; matchedLines: number; +} + /** A changed file governed by a CODEOWNERS rule where the PR author is not listed as an owner (#1515). * The blast radius (distinct ownership domains crossed) is derived at render time from the full findings set. */ export interface CodeownersFinding { diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 958a688dcd..d454d1e302 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -27,6 +27,7 @@ import { extractRemovedLines, scanRevertRecurrence, } from "../dist/analyzers/revert-recurrence.js"; +import { findOwners, parseCodeowners, patternToRegex, @@ -1280,6 +1281,28 @@ test("scanRevertRecurrence: no baseSha → query omits sha param", async () => { assert.ok(!urls[0].includes("sha="), `sha param should be absent; got ${urls[0]}`); }); +test("scanRevertRecurrence: repoFullName segments are percent-encoded in both URL forms", async () => { + const urls: string[] = []; + const fetchImpl = async (url) => { + urls.push(String(url)); + if (String(url).includes("/commits?")) + return { ok: true, json: async () => [revertCommit] }; + return { ok: true, json: async () => ({ files: [] }) }; + }; + await scanRevertRecurrence( + makeReq({ repoFullName: "owner name/repo name" }), + fetchImpl, + ); + assert.ok( + urls[0].includes("owner%20name/repo%20name"), + `list URL should encode segments; got ${urls[0]}`, + ); + assert.ok( + urls[1].includes("owner%20name/repo%20name"), + `diff URL should encode segments; got ${urls[1]}`, + ); +}); + test("scanRevertRecurrence: auth header sent when token present, omitted when absent", async () => { const seenHeaders: Record[] = []; const fetchImpl = async (_url, init) => { @@ -1481,6 +1504,11 @@ test("buildBrief: revert-recurrence analyzer degrades gracefully on network thro // listFileCommits catches the throw and returns [], so revertRecurrence = [] (not degraded) assert.equal(brief.analyzerStatus.revertRecurrence, "ok"); assert.deepEqual(brief.findings.revertRecurrence, []); + } finally { + globalThis.fetch = realFetch; + } +}); + test("codeOnly: blanks string messages, keeps ${...} interpolation bodies", () => { assert.equal(codeOnly('"a secret here"'), " "); assert.equal(codeOnly("'plain'"), " ");