feat: add per-file +/− summary and noise-filtered scope to review prompts
Emit a ### Changed Files Markdown table (| File | +/− | Type | rows plus total added/removed) ahead of the raw diff in both committed and uncommitted review prompts, parsed from the diff via the shared parseDiff engine. Add an ### Excluded Files (n) section listing filtered noise (path, +/− counts, reason), and replace byte-truncation of oversized diffs with a file-list + read instruction when the cleaned diff exceeds 50KB or touches more than 20 files. Also: distinguish diff-computation failure from genuinely no changes in the review loop (tri-state result, never treats a broken base ref as a clean verified task), map critical→blocker in finding severity parsing, inject a per-review custom focus/instructions section from config, and make the noise-filter ignore rules project-configurable via review.extraIgnorePatterns and review.ignorePaths. Add same-model retry before cycling to the next model in task/follow-up/fix sessions.
This commit is contained in:
259
tests/review-prompt.test.ts
Normal file
259
tests/review-prompt.test.ts
Normal file
@@ -0,0 +1,259 @@
|
||||
/**
|
||||
* Tests for the review prompt builders (src/prompts.ts).
|
||||
* Covers: per-file summary table, excluded-files section, oversized-diff
|
||||
* read-instruction (never byte-truncates), custom review focus, and the
|
||||
* configurable noise-filter overrides surfacing in the prompt.
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from "bun:test";
|
||||
import { buildReviewPrompt, buildReviewPromptUncommitted } from "../src/prompts";
|
||||
import { compileIgnorePatterns } from "../src/diff";
|
||||
import type { Task, Project } from "../src/types";
|
||||
|
||||
const task: Task = {
|
||||
id: "01",
|
||||
title: "Implement auth",
|
||||
description: "Add a login flow",
|
||||
status: "completed",
|
||||
dependencies: [],
|
||||
};
|
||||
|
||||
const project: Project = {
|
||||
objective: "Build the app",
|
||||
sourcePath: "README.md",
|
||||
sourceDir: "/tmp",
|
||||
tasks: [task],
|
||||
dependencies: {},
|
||||
};
|
||||
|
||||
/** A diff mixing one code file plus lockfile/minified/binary noise. */
|
||||
const MIXED_DIFF = [
|
||||
"diff --git a/src/auth.ts b/src/auth.ts",
|
||||
"index 111..222 100644",
|
||||
"--- a/src/auth.ts",
|
||||
"+++ b/src/auth.ts",
|
||||
"@@ -1,3 +1,5 @@",
|
||||
" import { hash } from \"./hash\";",
|
||||
"+export function login() {",
|
||||
"+ return hash(secret);",
|
||||
"- return legacy();",
|
||||
"+}",
|
||||
"",
|
||||
"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/assets/logo.png b/assets/logo.png",
|
||||
"index 111..222 100644",
|
||||
"Binary files differ",
|
||||
"",
|
||||
"diff --git a/dist/app.min.js b/dist/app.min.js",
|
||||
"index 111..222 100644",
|
||||
"--- a/dist/app.min.js",
|
||||
"+++ b/dist/app.min.js",
|
||||
"@@ -1 +1 @@",
|
||||
"-var a=1;",
|
||||
"+var a=2;",
|
||||
].join("\n");
|
||||
|
||||
function manyFileDiff(n: number): string {
|
||||
const chunks: string[] = [];
|
||||
for (let i = 0; i < n; i++) {
|
||||
chunks.push(
|
||||
`diff --git a/src/f${String(i).padStart(2, "0")}.ts b/src/f${String(i).padStart(2, "0")}.ts`,
|
||||
"--- a/src/f.ts",
|
||||
"+++ b/src/f.ts",
|
||||
`+line ${i}`,
|
||||
);
|
||||
}
|
||||
return chunks.join("\n");
|
||||
}
|
||||
|
||||
describe("buildReviewPrompt", () => {
|
||||
test("emits a per-file +/− summary table with totals, excluding noise", () => {
|
||||
const prompt = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: auth",
|
||||
MIXED_DIFF,
|
||||
);
|
||||
|
||||
expect(prompt).toContain("### Changed Files");
|
||||
expect(prompt).toContain("| `src/auth.ts` | +3/-1 | ts |");
|
||||
expect(prompt).toContain("| **Total** | **+3/-1** | |");
|
||||
});
|
||||
|
||||
test("surfaces an excluded-files section with path, counts, and reason", () => {
|
||||
const prompt = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: auth",
|
||||
MIXED_DIFF,
|
||||
);
|
||||
|
||||
expect(prompt).toContain("### Excluded Files (3)");
|
||||
expect(prompt).toContain("- `package-lock.json` (+3/-0) — lockfile");
|
||||
expect(prompt).toContain("- `assets/logo.png` (+0/-0) — binary/media asset");
|
||||
expect(prompt).toContain("- `dist/app.min.js` (+1/-1) — minified asset");
|
||||
});
|
||||
|
||||
test("never inlines excluded (noise) chunks into the diff block", () => {
|
||||
const prompt = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: auth",
|
||||
MIXED_DIFF,
|
||||
);
|
||||
|
||||
// The noise chunks themselves are never inlined — only the excluded-files
|
||||
// section names them (as `- path (+x/-y) — reason`, no `diff --git` header).
|
||||
expect(prompt).not.toContain("diff --git a/package-lock.json");
|
||||
expect(prompt).not.toContain("diff --git a/assets/logo.png");
|
||||
expect(prompt).not.toContain("diff --git a/dist/app.min.js");
|
||||
// The cleaned diff block is present with the code file.
|
||||
expect(prompt).toContain("```diff");
|
||||
expect(prompt).toContain("diff --git a/src/auth.ts");
|
||||
});
|
||||
|
||||
test("omits the excluded section entirely when nothing is excluded", () => {
|
||||
const clean = [
|
||||
"diff --git a/src/auth.ts b/src/auth.ts",
|
||||
"--- a/src/auth.ts",
|
||||
"+++ b/src/auth.ts",
|
||||
"+export const x = 1;",
|
||||
].join("\n");
|
||||
const prompt = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: auth",
|
||||
clean,
|
||||
);
|
||||
expect(prompt).not.toContain("### Excluded Files");
|
||||
expect(prompt).toContain("| `src/auth.ts` | +1/-0 | ts |");
|
||||
});
|
||||
|
||||
test("switches to a file-list + read instruction for >20 files, no truncation", () => {
|
||||
const prompt = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: many",
|
||||
manyFileDiff(21),
|
||||
);
|
||||
|
||||
expect(prompt).toContain("Diff too large");
|
||||
expect(prompt).toContain("Use `read` to inspect the changed files");
|
||||
// No byte-truncated inline diff for oversized inputs.
|
||||
expect(prompt).not.toContain("```diff");
|
||||
});
|
||||
|
||||
test("inlines a small diff normally (no read-instruction)", () => {
|
||||
const prompt = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: auth",
|
||||
MIXED_DIFF,
|
||||
);
|
||||
expect(prompt).not.toContain("Diff too large");
|
||||
});
|
||||
|
||||
test("emits a Custom Review Focus section only when focus is set", () => {
|
||||
const withFocus = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: auth",
|
||||
MIXED_DIFF,
|
||||
{ focus: "check security only" },
|
||||
);
|
||||
expect(withFocus).toContain("## Custom Review Focus");
|
||||
expect(withFocus).toContain("check security only");
|
||||
|
||||
const withoutFocus = buildReviewPrompt(
|
||||
task,
|
||||
project,
|
||||
"abc1234",
|
||||
"feat: auth",
|
||||
MIXED_DIFF,
|
||||
);
|
||||
expect(withoutFocus).not.toContain("## Custom Review Focus");
|
||||
});
|
||||
|
||||
test("surfaces extra ignore patterns and ignorePaths overrides in the prompt", () => {
|
||||
const diff = [
|
||||
"diff --git a/src/keep.ts b/src/keep.ts",
|
||||
"--- a/src/keep.ts",
|
||||
"+++ b/src/keep.ts",
|
||||
"+keep",
|
||||
"diff --git a/package-lock.json b/package-lock.json",
|
||||
"--- a/package-lock.json",
|
||||
"+++ b/package-lock.json",
|
||||
"+a",
|
||||
"+b",
|
||||
"+c",
|
||||
"+d",
|
||||
].join("\n");
|
||||
|
||||
// ignorePaths keeps the lockfile in scope → it shows in the table,
|
||||
// and no excluded section is emitted.
|
||||
const kept = buildReviewPrompt(task, project, "abc1234", "x", diff, {
|
||||
diffOptions: { ignorePaths: ["package-lock.json"] },
|
||||
});
|
||||
expect(kept).toContain("| `package-lock.json` | +4/-0 | json |");
|
||||
expect(kept).not.toContain("### Excluded Files");
|
||||
|
||||
// Without ignorePaths, the lockfile is excluded.
|
||||
const excluded = buildReviewPrompt(task, project, "abc1234", "x", diff);
|
||||
expect(excluded).not.toContain("| `package-lock.json` |");
|
||||
expect(excluded).toContain("### Excluded Files (1)");
|
||||
|
||||
// extraPatterns drops a matching file from scope.
|
||||
const dropped = buildReviewPrompt(task, project, "abc1234", "x", diff, {
|
||||
diffOptions: {
|
||||
extraPatterns: compileIgnorePatterns(["\\.ts$"]),
|
||||
ignorePaths: [],
|
||||
},
|
||||
});
|
||||
expect(dropped).not.toContain("| `src/keep.ts` |");
|
||||
expect(dropped).toContain("### Excluded Files (2)");
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildReviewPromptUncommitted", () => {
|
||||
test("emits summary table, excluded section, and cleaned diff", () => {
|
||||
const prompt = buildReviewPromptUncommitted(
|
||||
task,
|
||||
project,
|
||||
"M src/auth.ts",
|
||||
MIXED_DIFF,
|
||||
);
|
||||
|
||||
expect(prompt).toContain("### Changed Files");
|
||||
expect(prompt).toContain("| `src/auth.ts` | +3/-1 | ts |");
|
||||
expect(prompt).toContain("### Excluded Files (3)");
|
||||
expect(prompt).not.toContain("diff --git a/package-lock.json");
|
||||
expect(prompt).toContain("### Current Tracked Diff (git diff)");
|
||||
});
|
||||
|
||||
test("supports custom focus", () => {
|
||||
const prompt = buildReviewPromptUncommitted(
|
||||
task,
|
||||
project,
|
||||
"M src/auth.ts",
|
||||
MIXED_DIFF,
|
||||
{ focus: "review performance" },
|
||||
);
|
||||
expect(prompt).toContain("## Custom Review Focus");
|
||||
expect(prompt).toContain("review performance");
|
||||
});
|
||||
});
|
||||
53
tests/review-severity.test.ts
Normal file
53
tests/review-severity.test.ts
Normal file
@@ -0,0 +1,53 @@
|
||||
/**
|
||||
* Tests for the severity taxonomy alignment in review verdict parsing
|
||||
* (src/review.ts): the `critical` token is accepted and normalized to
|
||||
* ralpi's `blocker` severity, mirroring @piex-dev/review's grading.
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from "bun:test";
|
||||
import { extractReview } from "../src/review";
|
||||
|
||||
/** Build a full review-agent output ending in a REVIEW VERDICT block. */
|
||||
function reviewOutput(findings: string[]): string {
|
||||
return [
|
||||
"Prose: looks mostly fine, a few issues to fix.",
|
||||
"## REVIEW VERDICT",
|
||||
"VERDICT: fail",
|
||||
"SUMMARY: Needs fixes.",
|
||||
"FINDINGS:",
|
||||
...findings,
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
describe("extractReview severity normalization", () => {
|
||||
test("maps critical → blocker, keeps warning/nit/info", () => {
|
||||
const out = reviewOutput([
|
||||
"- [critical] src/auth.ts:12 hardcoded secret",
|
||||
"- [warning] src/auth.ts:30 unused import",
|
||||
"- [nit] src/auth.ts:5 style",
|
||||
"- [info] src/auth.ts:1 note",
|
||||
]);
|
||||
const review = extractReview(out, "01", "abc1234");
|
||||
expect(review).not.toBeNull();
|
||||
const severities = review!.findings.map((f) => f.severity);
|
||||
expect(severities).toEqual(["blocker", "warning", "nit", "info"]);
|
||||
});
|
||||
|
||||
test("normalizes the warn synonym to warning", () => {
|
||||
const out = reviewOutput(["- [warn] src/a.ts:2 thing"]);
|
||||
const review = extractReview(out, "01", "abc1234");
|
||||
expect(review!.findings[0].severity).toBe("warning");
|
||||
});
|
||||
|
||||
test("uppercase CRITICAL token also maps to blocker", () => {
|
||||
const out = reviewOutput(["- [CRITICAL] src/a.ts:2 thing"]);
|
||||
const review = extractReview(out, "01", "abc1234");
|
||||
expect(review!.findings[0].severity).toBe("blocker");
|
||||
});
|
||||
|
||||
test("findings without a severity are still parsed", () => {
|
||||
const out = reviewOutput(["- src/a.ts:2 plain line"]);
|
||||
const review = extractReview(out, "01", "abc1234");
|
||||
expect(review!.findings[0].severity).toBe("info");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user