feat: port noise-filtered diff parsing engine
Add src/diff.ts: a reusable unified-diff engine that parses diffs into per-file +/− stats and filters noise (lockfiles, minified/generated assets, source maps, snapshots, build output, node_modules/vendor, binary/media) so review prompts feed only clean, review-relevant changes. Exports DiffSummary/FileDiff shapes, EXCLUDED_PATTERNS, isExcluded, parseDiff, plus filterNoise and configurable extra-pattern/ignore-path overrides. Malformed diff chunks are skipped without crashing, and excluded files are never double-counted into totals. Add tests/diff.test.ts covering the included/excluded split, per-file +/− counts, totals excluding noise, malformed-chunk guard, and the noise-filter override rules.
This commit is contained in:
274
src/diff.ts
Normal file
274
src/diff.ts
Normal file
@@ -0,0 +1,274 @@
|
||||
/**
|
||||
* Reusable unified-diff engine: parses a diff into per-file +/− stats and
|
||||
* filters out noise files (locks, build output, vendor, generated, media
|
||||
* binaries) so review prompts feed the model only clean, review-relevant
|
||||
* changes.
|
||||
*
|
||||
* Ported from @piex-dev/review's `EXCLUDED_PATTERNS` + `parseDiff` (MIT).
|
||||
* Kept the excluded-files-not-totaled behavior that fixed the upstream
|
||||
* double-count bug.
|
||||
*/
|
||||
|
||||
// ─── Types ──────────────────────────────────────────────────────────────────
|
||||
|
||||
/** Per-file diff stats. */
|
||||
export interface FileDiff {
|
||||
/** File path as it appears in the diff (`a/` path). */
|
||||
path: string;
|
||||
/** Number of added lines (excluding the `+++` header). */
|
||||
linesAdded: number;
|
||||
/** Number of removed lines (excluding the `---` header). */
|
||||
linesRemoved: number;
|
||||
/** File extension (empty when the path has none). */
|
||||
ext: string;
|
||||
}
|
||||
|
||||
/** An excluded (noise) file with the reason it was filtered. */
|
||||
export interface ExcludedFile extends FileDiff {
|
||||
/** Why the file was excluded (e.g. "lockfile"). */
|
||||
reason: string;
|
||||
}
|
||||
|
||||
/** Result of parsing a unified diff. */
|
||||
export interface DiffSummary {
|
||||
/** Files kept in scope (review-relevant). */
|
||||
files: FileDiff[];
|
||||
/** Files filtered out as noise. */
|
||||
excluded: ExcludedFile[];
|
||||
/** Sum of added lines over included files only. */
|
||||
totalAdded: number;
|
||||
/** Sum of removed lines over included files only. */
|
||||
totalRemoved: number;
|
||||
}
|
||||
|
||||
/** Caller-supplied overrides for the noise filter. */
|
||||
export interface DiffOptions {
|
||||
/** Additional exclusion regexes merged into EXCLUDED_PATTERNS. */
|
||||
extraPatterns?: RegExp[];
|
||||
/** Pathspec allowlist — files matching these stay in scope even if a
|
||||
* default rule would exclude them. */
|
||||
ignorePaths?: string[];
|
||||
}
|
||||
|
||||
// ─── Noise-Filter Rules ─────────────────────────────────────────────────────
|
||||
|
||||
/** Default noise-exclusion rules, ported from @piex-dev/review (MIT).
|
||||
* Each entry is a regex tested against the file path plus a human-readable
|
||||
* reason surfaced in the "Excluded Files" prompt section. */
|
||||
export const EXCLUDED_PATTERNS: { pattern: RegExp; reason: string }[] = [
|
||||
// Lockfiles
|
||||
{ pattern: /(^|\/)package-lock\.json$/i, reason: "lockfile" },
|
||||
{ pattern: /(^|\/)yarn\.lock$/i, reason: "lockfile" },
|
||||
{ pattern: /(^|\/)pnpm-lock\.yaml$/i, reason: "lockfile" },
|
||||
{ pattern: /(^|\/)Cargo\.lock$/i, reason: "lockfile" },
|
||||
{ pattern: /(^|\/)Gemfile\.lock$/i, reason: "lockfile" },
|
||||
{ pattern: /\.lock$/i, reason: "lockfile" },
|
||||
// Minified assets
|
||||
{ pattern: /\.min\.(js|css)$/i, reason: "minified asset" },
|
||||
// Generated / tooling output
|
||||
{ pattern: /\.generated\./i, reason: "generated file" },
|
||||
{ pattern: /\.snap$/i, reason: "snapshot" },
|
||||
{ pattern: /\.map$/i, reason: "source map" },
|
||||
// Build output directories
|
||||
{ pattern: /(^|\/)(dist|build|out|coverage)\//i, reason: "build output" },
|
||||
// Dependency trees
|
||||
{ pattern: /(^|\/)node_modules\//i, reason: "dependency" },
|
||||
{ pattern: /(^|\/)vendor\//i, reason: "vendored dependency" },
|
||||
// Image / font / binary extensions
|
||||
{
|
||||
pattern:
|
||||
/\.(png|jpe?g|gif|webp|svg|ico|avif|bmp|woff2?|ttf|otf|eot|pdf|zip|tar|gz|mp[34]|wav|ogg|flac|wasm|bin|exe|dll|so|a|o|class|jar|pyc)$/i,
|
||||
reason: "binary/media asset",
|
||||
},
|
||||
];
|
||||
|
||||
/**
|
||||
* Return the exclusion reason for a file path, or undefined when the file is
|
||||
* review-relevant. Extra caller-supplied patterns are merged into the default
|
||||
* rule set.
|
||||
*/
|
||||
export function isExcluded(
|
||||
fp: string,
|
||||
extraPatterns?: RegExp[],
|
||||
): string | undefined {
|
||||
for (const rule of EXCLUDED_PATTERNS) {
|
||||
if (rule.pattern.test(fp)) return rule.reason;
|
||||
}
|
||||
if (extraPatterns) {
|
||||
for (const p of extraPatterns) {
|
||||
if (p.test(fp)) return "extra ignore pattern";
|
||||
}
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Safely compile user-supplied regex strings into RegExp objects. Invalid
|
||||
* patterns (that don't compile) are skipped so a bad config value never
|
||||
* crashes review prompt building.
|
||||
*/
|
||||
export function compileIgnorePatterns(patterns: string[]): RegExp[] {
|
||||
const out: RegExp[] = [];
|
||||
for (const p of patterns) {
|
||||
if (!p) continue;
|
||||
try {
|
||||
out.push(new RegExp(p));
|
||||
} catch {
|
||||
// Skip malformed patterns silently
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
// ─── Chunking + Counting Helpers ────────────────────────────────────────────
|
||||
|
||||
/** Split a raw diff into per-file chunks, each starting at a `diff --git`
|
||||
* line. The leading non-diff preamble (e.g. a `--stat` block) is dropped —
|
||||
* per-file stats are derived from the patch chunks themselves. */
|
||||
function chunkDiff(raw: string): string[] {
|
||||
if (!raw) return [];
|
||||
const lines = raw.split("\n");
|
||||
const chunks: string[] = [];
|
||||
let current: string[] = [];
|
||||
let started = false;
|
||||
for (const line of lines) {
|
||||
if (line.startsWith("diff --git ")) {
|
||||
if (started && current.length > 0) chunks.push(current.join("\n"));
|
||||
current = [line];
|
||||
started = true;
|
||||
} else if (started) {
|
||||
current.push(line);
|
||||
}
|
||||
}
|
||||
if (started && current.length > 0) chunks.push(current.join("\n"));
|
||||
return chunks;
|
||||
}
|
||||
|
||||
/** Parse the `a/<path>` from a `diff --git a/… b/…` header. Returns null for
|
||||
* malformed chunks that lack the a/… b/… header (guarded, never crashes). */
|
||||
function chunkPath(chunk: string): string | null {
|
||||
const m = chunk.match(/^diff --git a\/(.+?) b\//);
|
||||
return m ? m[1] : null;
|
||||
}
|
||||
|
||||
/** Count added/removed lines in a chunk, excluding the `+++`/`---` headers. */
|
||||
function countLines(chunk: string): { added: number; removed: number } {
|
||||
let added = 0;
|
||||
let removed = 0;
|
||||
for (const line of chunk.split("\n")) {
|
||||
if (line.startsWith("+") && !line.startsWith("+++")) added++;
|
||||
else if (line.startsWith("-") && !line.startsWith("---")) removed++;
|
||||
}
|
||||
return { added, removed };
|
||||
}
|
||||
|
||||
/** Extract the file extension from a path (no ext → empty string). */
|
||||
function getExt(fp: string): string {
|
||||
const base = fp.split("/").pop() ?? "";
|
||||
const idx = base.lastIndexOf(".");
|
||||
return idx > 0 ? base.slice(idx + 1) : "";
|
||||
}
|
||||
|
||||
/** Convert a git pathspec glob into a regex (supports `*`, `**`, `?`). */
|
||||
function globToRegExp(glob: string): RegExp {
|
||||
let re = "";
|
||||
for (let i = 0; i < glob.length; i++) {
|
||||
const c = glob[i];
|
||||
if (c === "*") {
|
||||
if (glob[i + 1] === "*") {
|
||||
re += ".*";
|
||||
i++;
|
||||
} else {
|
||||
re += "[^/]*";
|
||||
}
|
||||
} else if (c === "?") {
|
||||
re += "[^/]";
|
||||
} else if (c === ".") {
|
||||
re += "\\.";
|
||||
} else {
|
||||
re += c;
|
||||
}
|
||||
}
|
||||
return new RegExp(`^${re}$`);
|
||||
}
|
||||
|
||||
/** Whether a file path matches a pathspec allowlist entry. */
|
||||
function matchesPathspec(pathspec: string, fp: string): boolean {
|
||||
const ps = pathspec.trim();
|
||||
if (!ps) return false;
|
||||
// Directory prefix: "tests/" or a bare dir name matches everything under it.
|
||||
if (ps.endsWith("/") && fp.startsWith(ps)) return true;
|
||||
if (ps.includes("*") || ps.includes("?")) return globToRegExp(ps).test(fp);
|
||||
// Plain path — exact file or prefix directory.
|
||||
if (fp === ps) return true;
|
||||
if (fp.startsWith(ps + "/")) return true;
|
||||
return false;
|
||||
}
|
||||
|
||||
/** Decide whether a file path is kept in scope or noise-excluded. */
|
||||
function classify(
|
||||
path: string,
|
||||
opts?: DiffOptions,
|
||||
): { kept: boolean; reason?: string } {
|
||||
const reason = isExcluded(path, opts?.extraPatterns);
|
||||
if (reason === undefined) return { kept: true };
|
||||
// Excluded by a rule, but an ignorePaths allowlist can keep it in scope.
|
||||
const keptByPathspec =
|
||||
opts?.ignorePaths?.some((ps) => matchesPathspec(ps, path)) ?? false;
|
||||
return keptByPathspec ? { kept: true } : { kept: false, reason };
|
||||
}
|
||||
|
||||
// ─── Public API ─────────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Parse a unified diff into per-file +/− stats, splitting excluded (noise)
|
||||
* files from included files. Totals are summed over included files only.
|
||||
* Malformed chunks (no a/… b/… header) are skipped without crashing.
|
||||
*/
|
||||
export function parseDiff(raw: string, opts?: DiffOptions): DiffSummary {
|
||||
const files: FileDiff[] = [];
|
||||
const excluded: ExcludedFile[] = [];
|
||||
let totalAdded = 0;
|
||||
let totalRemoved = 0;
|
||||
|
||||
for (const chunk of chunkDiff(raw)) {
|
||||
if (!chunk) continue;
|
||||
const path = chunkPath(chunk);
|
||||
if (path === null) continue; // malformed chunk — skip
|
||||
const { added, removed } = countLines(chunk);
|
||||
const base: FileDiff = {
|
||||
path,
|
||||
linesAdded: added,
|
||||
linesRemoved: removed,
|
||||
ext: getExt(path),
|
||||
};
|
||||
const decision = classify(path, opts);
|
||||
if (decision.kept) {
|
||||
files.push(base);
|
||||
totalAdded += added;
|
||||
totalRemoved += removed;
|
||||
} else if (decision.reason) {
|
||||
excluded.push({ ...base, reason: decision.reason });
|
||||
}
|
||||
}
|
||||
|
||||
return { files, excluded, totalAdded, totalRemoved };
|
||||
}
|
||||
|
||||
/**
|
||||
* Return the diff re-emitted with excluded (noise) file chunks removed, so an
|
||||
* inlined review diff never contains filtered content. The stat preamble is
|
||||
* dropped — the per-file summary table carries that information. Empty string
|
||||
* when every changed file is noise.
|
||||
*/
|
||||
export function filterNoise(raw: string, opts?: DiffOptions): string {
|
||||
const kept: string[] = [];
|
||||
for (const chunk of chunkDiff(raw)) {
|
||||
if (!chunk) continue;
|
||||
const path = chunkPath(chunk);
|
||||
if (path === null) continue;
|
||||
const decision = classify(path, opts);
|
||||
if (decision.kept) kept.push(chunk);
|
||||
}
|
||||
return kept.join("\n");
|
||||
}
|
||||
237
tests/diff.test.ts
Normal file
237
tests/diff.test.ts
Normal file
@@ -0,0 +1,237 @@
|
||||
/**
|
||||
* Tests for the noise-filtered diff engine (src/diff.ts).
|
||||
* Covers: per-file +/− parsing, excluded-file split, totals excluding noise,
|
||||
* malformed-chunk guard, isExcluded reasons, and configurable overrides.
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from "bun:test";
|
||||
import {
|
||||
parseDiff,
|
||||
filterNoise,
|
||||
isExcluded,
|
||||
compileIgnorePatterns,
|
||||
EXCLUDED_PATTERNS,
|
||||
} from "../src/diff";
|
||||
|
||||
/** A synthetic unified diff mixing code, a lockfile, a minified file, and a binary. */
|
||||
const SYNTH_DIFF = [
|
||||
"diff --git a/src/index.ts b/src/index.ts",
|
||||
"index 111..222 100644",
|
||||
"--- a/src/index.ts",
|
||||
"+++ b/src/index.ts",
|
||||
"@@ -1,2 +1,4 @@",
|
||||
' import { foo } from "./foo";',
|
||||
"+export const baz = 1;",
|
||||
"+export const qux = 2;",
|
||||
"-foo();",
|
||||
"+bar();",
|
||||
"",
|
||||
"diff --git a/package-lock.json b/package-lock.json",
|
||||
"index 000..111 100644",
|
||||
"--- a/package-lock.json",
|
||||
"+++ b/package-lock.json",
|
||||
"@@ -0,0 +1,3 @@",
|
||||
"+{",
|
||||
'+ "name": "x"',
|
||||
"+}",
|
||||
"",
|
||||
"diff --git a/dist/foo.min.js b/dist/foo.min.js",
|
||||
"index 111..222 100644",
|
||||
"--- a/dist/foo.min.js",
|
||||
"+++ b/dist/foo.min.js",
|
||||
"@@ -1 +1 @@",
|
||||
"-var a=1;",
|
||||
"+var a=2;",
|
||||
"",
|
||||
"diff --git a/assets/logo.png b/assets/logo.png",
|
||||
"index 111..222 100644",
|
||||
"Binary files differ",
|
||||
].join("\n");
|
||||
|
||||
describe("parseDiff", () => {
|
||||
test("splits included vs excluded files and totals only included", () => {
|
||||
const summary = parseDiff(SYNTH_DIFF);
|
||||
|
||||
// Included: only src/index.ts (code). Lockfile, minified, binary excluded.
|
||||
expect(summary.files).toHaveLength(1);
|
||||
expect(summary.files[0]).toEqual({
|
||||
path: "src/index.ts",
|
||||
linesAdded: 3,
|
||||
linesRemoved: 1,
|
||||
ext: "ts",
|
||||
});
|
||||
|
||||
expect(summary.excluded).toHaveLength(3);
|
||||
const byPath = new Map(
|
||||
summary.excluded.map((f) => [f.path, f]),
|
||||
);
|
||||
expect(byPath.get("package-lock.json")).toMatchObject({
|
||||
linesAdded: 3,
|
||||
linesRemoved: 0,
|
||||
reason: "lockfile",
|
||||
});
|
||||
expect(byPath.get("dist/foo.min.js")).toMatchObject({
|
||||
linesAdded: 1,
|
||||
linesRemoved: 1,
|
||||
reason: "minified asset",
|
||||
});
|
||||
expect(byPath.get("assets/logo.png")).toMatchObject({
|
||||
linesAdded: 0,
|
||||
linesRemoved: 0,
|
||||
reason: "binary/media asset",
|
||||
});
|
||||
|
||||
// Totals exclude the noise files.
|
||||
expect(summary.totalAdded).toBe(3);
|
||||
expect(summary.totalRemoved).toBe(1);
|
||||
});
|
||||
|
||||
test("returns empty summary for an empty diff", () => {
|
||||
const summary = parseDiff("");
|
||||
expect(summary.files).toHaveLength(0);
|
||||
expect(summary.excluded).toHaveLength(0);
|
||||
expect(summary.totalAdded).toBe(0);
|
||||
expect(summary.totalRemoved).toBe(0);
|
||||
});
|
||||
|
||||
test("skips malformed chunks without a/… b/ header without crashing", () => {
|
||||
const malformed =
|
||||
"diff --git weird-line\nindex 111..222\n--- a/x\n+++ b/x\n+x\n" +
|
||||
"\n" +
|
||||
"diff --git a/src/ok.ts b/src/ok.ts\n--- a/src/ok.ts\n+++ b/src/ok.ts\n+ok\n";
|
||||
const summary = parseDiff(malformed);
|
||||
// Only the well-formed chunk is counted.
|
||||
expect(summary.files).toHaveLength(1);
|
||||
expect(summary.files[0].path).toBe("src/ok.ts");
|
||||
expect(summary.totalAdded).toBe(1);
|
||||
});
|
||||
|
||||
test("does not count +++/--- header lines as additions/removals", () => {
|
||||
const diff = [
|
||||
"diff --git a/src/a.ts b/src/a.ts",
|
||||
"--- a/src/a.ts",
|
||||
"+++ b/src/a.ts",
|
||||
"@@ -0,0 +1,2 @@",
|
||||
"+one",
|
||||
"+two",
|
||||
].join("\n");
|
||||
const summary = parseDiff(diff);
|
||||
expect(summary.files[0].linesAdded).toBe(2);
|
||||
expect(summary.files[0].linesRemoved).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("isExcluded", () => {
|
||||
test("returns the right reason per pattern", () => {
|
||||
expect(isExcluded("package-lock.json")).toBe("lockfile");
|
||||
expect(isExcluded("yarn.lock")).toBe("lockfile");
|
||||
expect(isExcluded("src/app.min.js")).toBe("minified asset");
|
||||
expect(isExcluded("src/styles.min.css")).toBe("minified asset");
|
||||
expect(isExcluded("build/out.js")).toBe("build output");
|
||||
expect(isExcluded("node_modules/foo/index.js")).toBe("dependency");
|
||||
expect(isExcluded("vendor/lib.bundle.js")).toBe("vendored dependency");
|
||||
expect(isExcluded("assets/icon.svg")).toBe("binary/media asset");
|
||||
expect(isExcluded("src/api.generated.ts")).toBe("generated file");
|
||||
expect(isExcluded("test/__snapshots__/x.snap")).toBe("snapshot");
|
||||
expect(isExcluded("dist/x.js.map")).toBe("source map");
|
||||
});
|
||||
|
||||
test("returns undefined for review-relevant files", () => {
|
||||
expect(isExcluded("src/foo.ts")).toBeUndefined();
|
||||
expect(isExcluded("src/index.ts")).toBeUndefined();
|
||||
});
|
||||
|
||||
test("merges caller-supplied extra patterns", () => {
|
||||
expect(isExcluded("src/data.foo", [/\.foo$/])).toBe("extra ignore pattern");
|
||||
expect(isExcluded("src/data.foo")).toBeUndefined();
|
||||
});
|
||||
|
||||
test("EXCLUDED_PATTERNS covers lockfiles, min, generated, snap, map, build, vendor, binaries", () => {
|
||||
for (const pat of [
|
||||
"package-lock.json",
|
||||
"src/app.min.js",
|
||||
"src/thing.generated.ts",
|
||||
"x.snap",
|
||||
"x.js.map",
|
||||
"dist/bundle.js",
|
||||
"node_modules/a/b.js",
|
||||
"vendor/x",
|
||||
"a.png",
|
||||
"f.woff2",
|
||||
]) {
|
||||
const hit = EXCLUDED_PATTERNS.some((r) => r.pattern.test(pat));
|
||||
expect(hit, `${pat} should be covered by a default rule`).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("filterNoise", () => {
|
||||
test("re-emits only included-file chunks", () => {
|
||||
const filtered = filterNoise(SYNTH_DIFF);
|
||||
expect(filtered).toContain("diff --git a/src/index.ts");
|
||||
expect(filtered).not.toContain("package-lock.json");
|
||||
expect(filtered).not.toContain("foo.min.js");
|
||||
expect(filtered).not.toContain("logo.png");
|
||||
});
|
||||
|
||||
test("returns empty when every file is noise", () => {
|
||||
const onlyNoise = [
|
||||
"diff --git a/package-lock.json b/package-lock.json",
|
||||
"--- a/package-lock.json",
|
||||
"+++ b/package-lock.json",
|
||||
"+x",
|
||||
].join("\n");
|
||||
expect(filterNoise(onlyNoise)).toBe("");
|
||||
});
|
||||
});
|
||||
|
||||
describe("configurable noise rules", () => {
|
||||
test("extraPatterns excludes a matching file from the review diff", () => {
|
||||
const diff = [
|
||||
"diff --git a/src/foo.ts b/src/foo.ts",
|
||||
"--- a/src/foo.ts",
|
||||
"+++ b/src/foo.ts",
|
||||
"+keep",
|
||||
"diff --git a/src/data.foo b/src/data.foo",
|
||||
"--- a/src/data.foo",
|
||||
"+++ b/src/data.foo",
|
||||
"+drop",
|
||||
].join("\n");
|
||||
const opts = { extraPatterns: compileIgnorePatterns(["\\.foo$"]) };
|
||||
const summary = parseDiff(diff, opts);
|
||||
expect(summary.files.map((f) => f.path)).toEqual(["src/foo.ts"]);
|
||||
expect(summary.excluded.map((f) => [f.path, f.reason])).toEqual([
|
||||
["src/data.foo", "extra ignore pattern"],
|
||||
]);
|
||||
expect(filterNoise(diff, opts)).not.toContain("data.foo");
|
||||
});
|
||||
|
||||
test("ignorePaths keeps an excluded-by-default file in scope", () => {
|
||||
const diff = [
|
||||
"diff --git a/package-lock.json b/package-lock.json",
|
||||
"--- a/package-lock.json",
|
||||
"+++ b/package-lock.json",
|
||||
"+a",
|
||||
"+b",
|
||||
"+c",
|
||||
].join("\n");
|
||||
const opts = { ignorePaths: ["package-lock.json"] };
|
||||
const summary = parseDiff(diff, opts);
|
||||
expect(summary.files).toHaveLength(1);
|
||||
expect(summary.files[0].path).toBe("package-lock.json");
|
||||
expect(summary.excluded).toHaveLength(0);
|
||||
expect(summary.totalAdded).toBe(3);
|
||||
expect(filterNoise(diff, opts)).toContain("package-lock.json");
|
||||
});
|
||||
|
||||
test("default behavior unchanged when overrides are unset", () => {
|
||||
const summary = parseDiff(SYNTH_DIFF);
|
||||
expect(summary.files[0].path).toBe("src/index.ts");
|
||||
expect(summary.totalAdded).toBe(3);
|
||||
});
|
||||
|
||||
test("compileIgnorePatterns skips invalid regexes", () => {
|
||||
const compiled = compileIgnorePatterns(["\\.foo$", "(", "ok$"]);
|
||||
expect(compiled.length).toBe(2);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user