diff --git a/review-enrichment/src/analyzers/blame-link.ts b/review-enrichment/src/analyzers/blame-link.ts index c59e34497c..a47c17604b 100644 --- a/review-enrichment/src/analyzers/blame-link.ts +++ b/review-enrichment/src/analyzers/blame-link.ts @@ -12,15 +12,13 @@ import type { } from "../types.js"; import type { AnalysisContext } from "../analysis-context.js"; import { boundedFetchJson } from "../external-fetch.js"; +import { isHistoryUninformativePath } from "./history-path.js"; const GITHUB_API = "https://api.github.com"; const SLUG_RE = /^[A-Za-z0-9._-]+$/; const MAX_FILES_PROBED = 6; // bound the files we probe, matching the other history-class analyzers const MAX_LOOKUPS = 12; // hard cap on total GitHub round-trips (each file costs up to 2: commits + pulls) const SHA_PREFIX_LEN = 12; -// Files whose commit history is not a useful "who introduced this" signal — lockfiles, generated output, binaries. -const SKIP_RE = - /(?:^|\/)(?:package-lock\.json|yarn\.lock|pnpm-lock\.yaml|poetry\.lock|go\.sum)$|\.(?:lock|min\.js|map|snap|png|jpe?g|gif|svg|ico|pdf|zip|gz|woff2?)$|(?:^|\/)(?:dist|build|vendor)\//i; interface ScanOptions { signal?: AbortSignal; @@ -159,7 +157,7 @@ export async function scanBlameLink( // additions (a patch with no deletion line has no prior author to attribute). const candidates: Array<{ lookupPath: string; displayPath: string; line: number }> = []; for (const file of files) { - if (file.status === "added" || SKIP_RE.test(file.path)) continue; + if (file.status === "added" || isHistoryUninformativePath(file.path)) continue; let line = file.patch ? firstTouchedOldLine(file.patch) : null; // A removed OR renamed file resolves against the base tree even without a usable patch (binary/truncated, or a // pure rename with no content change). Anchor to line 1 as the representative point. diff --git a/review-enrichment/src/analyzers/churn-hotspot.ts b/review-enrichment/src/analyzers/churn-hotspot.ts index fe5878b75f..37608471f1 100644 --- a/review-enrichment/src/analyzers/churn-hotspot.ts +++ b/review-enrichment/src/analyzers/churn-hotspot.ts @@ -11,6 +11,7 @@ import type { } from "../types.js"; import type { AnalysisContext } from "../analysis-context.js"; import { boundedFetchJson } from "../external-fetch.js"; +import { isHistoryUninformativePath } from "./history-path.js"; const GITHUB_API = "https://api.github.com"; const SLUG_RE = /^[A-Za-z0-9._-]+$/; @@ -19,9 +20,6 @@ const PER_PAGE = 100; // one page; a file with a full page of commits in the win const MAX_FILES_PROBED = 8; // bound the GitHub round-trips, matching the other history-class analyzers const MIN_COMMITS = 8; // a hotspot must change frequently within the window const MIN_FIX_FRACTION = 0.3; // and a meaningful share of those changes must be fixes/reverts -// Files whose commit churn is not a useful code-fragility signal — lockfiles, generated output, and binaries. -const SKIP_RE = - /(?:^|\/)(?:package-lock\.json|yarn\.lock|pnpm-lock\.yaml|poetry\.lock|go\.sum)$|\.(?:lock|min\.js|map|snap|png|jpe?g|gif|svg|ico|pdf|zip|gz|woff2?)$|(?:^|\/)(?:dist|build|vendor)\//i; // Defect-correcting commit subjects: fix/bugfix/hotfix/revert/regression (conventional-commit `fix:` included). const FIX_RE = /\b(?:fix(?:e[ds]|ing)?|bug ?fix|hotfix|revert(?:ed|s)?|regression)\b/i; @@ -109,7 +107,7 @@ export async function scanChurnHotspot( const since = new Date(Date.now() - WINDOW_DAYS * 86_400_000).toISOString(); // A newly-added file has no prior history; skip it (and non-code/generated files) before spending a round-trip. const paths = files - .filter((file) => file.status !== "added" && !SKIP_RE.test(file.path)) + .filter((file) => file.status !== "added" && !isHistoryUninformativePath(file.path)) .map((file) => file.path) .slice(0, MAX_FILES_PROBED); diff --git a/review-enrichment/src/analyzers/history-path.ts b/review-enrichment/src/analyzers/history-path.ts new file mode 100644 index 0000000000..20b2d8d6d2 --- /dev/null +++ b/review-enrichment/src/analyzers/history-path.ts @@ -0,0 +1,20 @@ +// Shared skip predicate for the history-class analyzers (blame-link #2034, churn-hotspot #1513). A file whose +// commit history carries no useful "who introduced this" / code-fragility signal — a lockfile, generated +// output, or a binary blob — should be skipped by both, so the rule lives in one place instead of a duplicated +// per-analyzer regex. Binary and lockfile recognition delegate to the unified inventories (binary-extensions, +// lockfile-path) so this stays in sync as those grow, rather than a hand-maintained list. +import { BINARY_EXT_RE } from "./binary-extensions.js"; +import { isSupportedLockfile } from "../lockfile-path.js"; + +// Non-binary generated output (and the original narrow binary/lockfile set) — kept verbatim from the previous +// per-analyzer SKIP_RE. Broader binary and lockfile coverage is added by the two shared inventories below. +const SKIP_RE = + /(?:^|\/)(?:package-lock\.json|yarn\.lock|pnpm-lock\.yaml|poetry\.lock|go\.sum)$|\.(?:lock|min\.js|map|snap|png|jpe?g|gif|svg|ico|pdf|zip|gz|woff2?)$|(?:^|\/)(?:dist|build|vendor)\//i; + +/** True when a file's commit history is not a useful fragility/attribution signal — a lockfile, generated + * output, or a binary blob. Additive over the original rule: it now also skips the full shared binary + * inventory (e.g. webp/avif/heic/mp4/wasm/safetensors) and the full lockfile set (e.g. Cargo.lock, + * composer.lock, bun.lockb), not just the original narrow list. Pure. */ +export function isHistoryUninformativePath(path: string): boolean { + return SKIP_RE.test(path) || BINARY_EXT_RE.test(path) || isSupportedLockfile(path); +} diff --git a/review-enrichment/test/churn-hotspot.test.ts b/review-enrichment/test/churn-hotspot.test.ts index d18f1f6d5e..85c406261d 100644 --- a/review-enrichment/test/churn-hotspot.test.ts +++ b/review-enrichment/test/churn-hotspot.test.ts @@ -61,7 +61,14 @@ test("scanChurnHotspot: marks the count capped when the page is full", async () test("scanChurnHotspot: skips lockfiles, binaries, and newly-added files without fetching", async () => { let called = false; const out = await scanChurnHotspot( - req([{ path: "package-lock.json" }, { path: "assets/logo.png" }, { path: "src/new.ts", status: "added" }]), + req([ + { path: "package-lock.json" }, + { path: "assets/logo.png" }, + // Broader shared-inventory skips: a heavy binary and a Cargo lockfile the original narrow regex missed. + { path: "models/llama.safetensors" }, + { path: "crates/api/Cargo.lock" }, + { path: "src/new.ts", status: "added" }, + ]), async () => { called = true; return jsonResponse(commits(50, 2)); diff --git a/review-enrichment/test/history-path.test.ts b/review-enrichment/test/history-path.test.ts new file mode 100644 index 0000000000..c96bec424e --- /dev/null +++ b/review-enrichment/test/history-path.test.ts @@ -0,0 +1,67 @@ +// Units for the shared history-class skip predicate used by blame-link and churn-hotspot. +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { isHistoryUninformativePath } from "../dist/analyzers/history-path.js"; + +test("isHistoryUninformativePath skips the original lockfile / generated / dir set", () => { + // Lockfiles from the original rule. + for (const p of [ + "package-lock.json", + "app/yarn.lock", + "pkg/pnpm-lock.yaml", + "poetry.lock", + "svc/go.sum", + ]) { + assert.equal(isHistoryUninformativePath(p), true, p); + } + // Non-binary generated output and vendored/build directories. + for (const p of [ + "flake.lock", + "app.min.js", + "bundle.js.map", + "__snapshots__/x.snap", + "icons/logo.svg", + "dist/app.js", + "build/out.js", + "vendor/pkg/mod.go", + ]) { + assert.equal(isHistoryUninformativePath(p), true, p); + } +}); + +test("isHistoryUninformativePath skips the full binary inventory, not just the original narrow list", () => { + // Original narrow binaries still skip. + for (const p of ["ui/logo.png", "fonts/Inter.woff2", "docs/spec.pdf"]) { + assert.equal(isHistoryUninformativePath(p), true, p); + } + // Newly-covered shared-inventory binaries (were NOT in the original per-analyzer regex). + for (const p of [ + "media/demo.mp4", + "assets/hero.webp", + "assets/photo.heic", + "vendor/lib.wasm", + "models/llama.safetensors", + "models/model.gguf", + ]) { + assert.equal(isHistoryUninformativePath(p), true, p); + } +}); + +test("isHistoryUninformativePath skips the full lockfile inventory (Cargo/Composer/Bun)", () => { + for (const p of ["crates/api/Cargo.lock", "composer.lock", "bun.lockb"]) { + assert.equal(isHistoryUninformativePath(p), true, p); + } +}); + +test("isHistoryUninformativePath does not skip real source/doc files", () => { + for (const p of [ + "src/index.ts", + "packages/app/main.rs", + "README.md", + "Cargo.toml", + "notes/cargo.lock.md", + ]) { + assert.equal(isHistoryUninformativePath(p), false, p); + } +});