From b86174782f077d377291c8076fc1628b51d25a7c Mon Sep 17 00:00:00 2001 From: Michael Freno Date: Sat, 8 Aug 2026 18:10:08 -0400 Subject: [PATCH] =?UTF-8?q?feat:=20add=20per-file=20+/=E2=88=92=20summary?= =?UTF-8?q?=20and=20noise-filtered=20scope=20to=20review=20prompts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 13 +- README.md | 17 +++ src/executor.ts | 171 +++++++++++++++------- src/prompts.ts | 182 +++++++++++++++++++----- src/review.ts | 5 +- src/types.ts | 28 ++++ src/utils.ts | 65 +++++++-- tests/review-prompt.test.ts | 259 ++++++++++++++++++++++++++++++++++ tests/review-severity.test.ts | 53 +++++++ 9 files changed, 698 insertions(+), 95 deletions(-) create mode 100644 tests/review-prompt.test.ts create mode 100644 tests/review-severity.test.ts diff --git a/AGENTS.md b/AGENTS.md index 26ddd80..82d1e0a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -126,9 +126,20 @@ Key config fields in `execution`: loop startup via `selectLoopOptions`; review is asked FIRST, commit is mandated when review is on) - `models` — slot-aware round-robin model list for parallel mode, with - automatic failover to the next model per task + failover to the next model per task (only after exhausting same-model + retries, see `maxSameModelAttempts`) +- `maxSameModelAttempts` — max attempts on the SAME model before cycling to + the next model on failure (default 5, matching pi's normal retry count). + Applies to task execution, commit/review follow-up sessions, and + review-fix re-execution alike - `implModel` / `commitModel` / `reviewModel` — `/` strings resolved via `resolveModelSpec` in `utils.ts` +- `prompts.reviewFocus` — per-review custom focus/instructions, injected as a + `## Custom Review Focus` section in review prompts +- `review.extraIgnorePatterns` — extra noise-filter exclusion regexes (file + paths) merged into the default rules +- `review.ignorePaths` — pathspec allowlist keeping matching files in review + scope even when a default noise rule would exclude them - `maxReviewRetries` / `reviewBlockOnFail` — review-gated loop retry behavior - `worktrees` — `"never" | "parallel" | "always"` git worktree isolation (default `"parallel"`; see `shouldUseWorktrees` in `src/executor.ts`) diff --git a/README.md b/README.md index 8df97a2..b05e5f8 100644 --- a/README.md +++ b/README.md @@ -221,8 +221,25 @@ execution: prompts: projectContext: "Additional context for all tasks" reflectionPrompt: "" # custom suffix for reflection extraction + reviewFocus: "" # per-review custom focus/instructions (e.g. "check security only") +review: + extraIgnorePatterns: [] # extra noise-filter exclusion regexes (merged into the default rules) + ignorePaths: [] # pathspec allowlist — files matching these stay in review scope ``` +Review prompts (committed + uncommitted) run the diff through a noise filter +before inlining: lockfiles, minified/generated assets, source maps, +snapshots, build output, `node_modules`/`vendor`, and binary/media files are +excluded by default. The prompt gets a per-file `+/−` summary table, an +`### Excluded Files (n)` section listing what was filtered (path, counts, +reason), and — when a diff is oversized or touches >20 files — a +file-list + "use `read`" instruction instead of a byte-truncated diff. +`prompts.reviewFocus` injects a `### Custom Review Focus` section into each +review prompt. `review.extraIgnorePatterns` adds exclusion regexes (matched +against file paths), and `review.ignorePaths` is a pathspec allowlist that +keeps matching files in review scope even when a default rule would exclude +them. + > `execution.models` uses slot-aware round-robin: with 3 models and 2 concurrent > tasks, only the first two models are used. The third model is only touched when > a third concurrent task starts. Freed model slots are reused before new ones diff --git a/src/executor.ts b/src/executor.ts index 84a23cd..f148560 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -19,6 +19,10 @@ import { buildConflictResolutionPrompt, MAX_DIFF_BYTES, } from "./prompts"; +import { + compileIgnorePatterns, + type DiffOptions, +} from "./diff"; import { extractReflection } from "./reflection"; import { extractReview, @@ -756,10 +760,20 @@ async function executeTask( conflicts?: BatchConflict[], ): Promise { // Model failover: when a provider/API is down, cycle through available models. - // Pi's built-in retry (via SettingsManager) handles transient errors with - // exponential backoff within each model. Ralpi only handles model cycling. + // Pi's built-in retry (via SettingsManager) handles transient HTTP errors + // with exponential backoff WITHIN a single prompt. Ralpi adds two layers on + // top: (1) reattempt the SAME model up to `maxSameModelAttempts` times — a + // sustained provider hiccup can exhaust pi's in-call retries mid-session, + // and flapping to a different model on the first hard failure throws away + // model-specific context; (2) once same-model retries are exhausted, cycle + // to the next model in the round-robin pool. const maxModelAttempts = roundRobin ? roundRobin.length : 1; + const maxSameModelAttempts = Math.max( + 1, + config.execution.maxSameModelAttempts, + ); let modelAttempt = 0; + let sameModelAttempt = 0; // Resolve implModel from config (used in sequential mode when no round-robin assignment). // In parallel mode, the round-robin assignedModel takes precedence. const implModel = resolveModelSpec( @@ -787,12 +801,9 @@ async function executeTask( const worktreeDir = wt?.dir ?? projectDir; while (modelAttempt < maxModelAttempts) { - // On subsequent model attempts, advance to the next model. - // Uses advance() instead of assign() so we don't get stuck on - // the same freed slot when the current model is down. - if (modelAttempt > 0 && roundRobin) { - currentModel = roundRobin.advance(task.id); - } + // Model advancement happens in the cycling branch below (not here) so a + // same-model retry `continue` doesn't re-advance and accidentally swap + // models mid-retry. The first model uses `currentModel` set above. try { // Mark as in progress @@ -897,19 +908,30 @@ async function executeTask( // baseRef was captured before runTask (above). Each review iteration // diffs the range baseRef..HEAD — the complete task output including // all fix attempts. On re-execution the same baseRef is reused. + // A FAILED range computation (broken/stale base ref, git error) is + // logged as a distinct warning and is never treated as a clean, + // verified task — only a GENUINE "no changes" skips review. while (true) { - const reviewInfo = baseRef - ? getCommitRangeDiff(worktreeDir, baseRef) - : null; - if (!reviewInfo || !reviewInfo.diff) { - const reason = !baseRef - ? "could not capture base ref before execution" - : "no changes found between base and HEAD"; + if (!baseRef) { sendChatMessage?.( - `~ review for ${task.id} · ${task.title} — skipping review (${reason})`, + `~ review for ${task.id} · ${task.title} — diff could not be computed (could not capture base ref before execution)`, ); break; } + const rangeDiff = getCommitRangeDiff(worktreeDir, baseRef); + if (rangeDiff.kind === "error") { + sendChatMessage?.( + `~ review for ${task.id} · ${task.title} — diff could not be computed (${rangeDiff.error})`, + ); + break; + } + if (rangeDiff.kind === "no-changes") { + sendChatMessage?.( + `~ review for ${task.id} · ${task.title} — skipping review (no changes found between base and HEAD)`, + ); + break; + } + const reviewInfo = rangeDiff; const reviewPrompt = buildReviewPrompt( task, @@ -917,7 +939,16 @@ async function executeTask( reviewInfo.hash, reviewInfo.subject, reviewInfo.diff, - config.prompts.projectContext, + { + projectContext: config.prompts.projectContext, + focus: config.prompts.reviewFocus, + diffOptions: { + extraPatterns: compileIgnorePatterns( + config.review.extraIgnorePatterns, + ), + ignorePaths: config.review.ignorePaths, + }, + }, ); const reviewModel = resolveFollowUpModel( @@ -1049,19 +1080,33 @@ async function executeTask( fixAttempt++ ) { const fixModel = fixModels[fixAttempt]; - fixResult = await runTask( - task, - project, - config, - depReflections, - ctx, - sendChatMessage, - worktreeDir, - parallelState, - fixModel, - batchRender, - review ?? undefined, - ); + let fixSameAttempt = 0; + // Reattempt on the same model before cycling, matching the main + // task loop's behavior. + for (;;) { + fixResult = await runTask( + task, + project, + config, + depReflections, + ctx, + sendChatMessage, + worktreeDir, + parallelState, + fixModel, + batchRender, + review ?? undefined, + ); + if (fixResult.success) break; + if (fixSameAttempt < maxSameModelAttempts - 1) { + fixSameAttempt++; + sendChatMessage?.( + `~ re-execution for ${task.id} · ${task.title} — reattempting model ${fixAttempt + 1}/${fixModels.length} (${fixSameAttempt + 1}/${maxSameModelAttempts}, previous: ${fixResult.error})`, + ); + continue; + } + break; // same-model retries exhausted + } if (fixResult.success) break; // Connection/error failover — try the next model. if (fixAttempt < fixModels.length - 1) { @@ -1226,9 +1271,22 @@ async function executeTask( } // Agent session failed (provider error). - // Pi's built-in retry already exhausted for this model. Cycle to the next. + // Pi's built-in in-call retry already exhausted for this attempt. + // Reattempt on the SAME model a few more times before cycling — a + // transient outage can outlast pi's per-prompt backoff window. + sameModelAttempt++; + if (sameModelAttempt < maxSameModelAttempts) { + sendChatMessage?.( + `~ ${task.id} · ${task.title} — reattempting model ${modelAttempt + 1}/${maxModelAttempts} (${sameModelAttempt + 1}/${maxSameModelAttempts}, previous: ${result.error})`, + ); + continue; // same model, fresh session + } + + // Same-model retries exhausted — cycle to the next model (if any). if (roundRobin && modelAttempt < maxModelAttempts - 1) { modelAttempt++; + sameModelAttempt = 0; + currentModel = roundRobin.advance(task.id); sendChatMessage?.( `~ ${task.id} · ${task.title} — cycling to model ${modelAttempt + 1}/${maxModelAttempts} (previous: ${result.error})`, ); @@ -1417,28 +1475,43 @@ async function runFollowUpSession( }, 100); let result: Awaited> | undefined; + const maxSameModelAttempts = Math.max( + 1, + config.execution.maxSameModelAttempts, + ); try { for (let attempt = 0; attempt < models.length; attempt++) { const model = models[attempt]; - result = await runAgentSession( - prompt, - projectDir, - timeoutMs, - (event) => { - if (event.type === "tool_execution_start") { - const label = formatToolArg(event.toolName, event.args); - toolCalls.push({ name: event.toolName, label }); - requestRender(); - } - }, - undefined, - model, - config.thinkingLevel, - false, // noSkills=false — follow-up sessions load skills too - (ctx.modelRegistry as any).runtime as ModelRuntime, - ); + // Reattempt on the same model before cycling — matches the main task + // loop. Clear partial tool calls between failed attempts so the + // widget reflects only the successful (or final) attempt. + for (let same = 0; same < maxSameModelAttempts; same++) { + result = await runAgentSession( + prompt, + projectDir, + timeoutMs, + (event) => { + if (event.type === "tool_execution_start") { + const label = formatToolArg(event.toolName, event.args); + toolCalls.push({ name: event.toolName, label }); + requestRender(); + } + }, + undefined, + model, + config.thinkingLevel, + false, // noSkills=false — follow-up sessions load skills too + (ctx.modelRegistry as any).runtime as ModelRuntime, + ); - if (result.success) break; + if (result.success) break; + if (same < maxSameModelAttempts - 1) { + toolCalls.length = 0; + requestRender(); + } + } + + if (result!.success) break; // If there's a next model to try, cycle; otherwise give up. if (attempt < models.length - 1) { diff --git a/src/prompts.ts b/src/prompts.ts index 05c8804..79088a8 100644 --- a/src/prompts.ts +++ b/src/prompts.ts @@ -1,27 +1,34 @@ import type { Task, Project, Reflection, ReviewResult } from "./types"; import { readTaskSpec } from "./parser"; +import { + parseDiff, + filterNoise, + type DiffSummary, + type DiffOptions, +} from "./diff"; -/** Maximum bytes of a commit diff embedded in a review/commit prompt. - * Diffs larger than this are truncated to avoid blowing past the model's - * context window. The agent can always run `git show HEAD` itself to - * inspect the full diff when it needs more detail. +/** Maximum bytes of an inlined review diff before we stop inlining it and + * instead list the changed files + tell the model to `read` them. + * Diffs larger than this are never byte-truncated into a review prompt — + * truncation loses the middle of a large diff, so the file-list + read + * instruction is strictly better. * * ~50 KB ≈ 12.5K tokens — comfortably fits even on models with a 128K * context window once system-prompt overhead is accounted for. */ export const MAX_DIFF_BYTES = 50_000; -/** - * Truncate a diff to MAX_DIFF_BYTES, appending a clear notice when truncated. - */ -function truncateDiff(diff: string): string { - if (diff.length <= MAX_DIFF_BYTES) return diff; - const omitted = diff.length - MAX_DIFF_BYTES; - return ( - diff.slice(0, MAX_DIFF_BYTES) + - "\n\n... (diff truncated: omitted " + - omitted.toLocaleString() + - " bytes; run `git show HEAD` to view the full diff)" - ); +/** Max included files before an oversized diff is replaced by a read + * instruction rather than inlined. */ +const MAX_REVIEW_FILES = 20; + +/** Optional knobs for the review prompt builders. */ +export interface ReviewPromptOptions { + /** Extra context injected into the prompt (config.prompts.projectContext). */ + projectContext?: string; + /** Per-review custom focus/instructions (config.prompts.reviewFocus). */ + focus?: string; + /** Noise-filter overrides (config.review.*). */ + diffOptions?: DiffOptions; } // ─── Task Prompt ───────────────────────────────────────────────────────────── @@ -201,7 +208,7 @@ export function buildReviewPrompt( commitHash: string, commitSubject: string, commitDiff: string, - projectContext?: string, + opts: ReviewPromptOptions = {}, ): string { const parts: string[] = []; @@ -236,17 +243,34 @@ export function buildReviewPrompt( parts.push("## Commit Under Review"); parts.push(`Commit: ${commitHash} — ${commitSubject}`); parts.push(""); - parts.push("### Diff"); - parts.push("```diff"); - parts.push(truncateDiff(commitDiff)); - parts.push("```"); + + // ── Changed-Files Summary + Exclusions (noise-filtered scope) ── + + const summary = parseDiff(commitDiff, opts.diffOptions); + const filtered = filterNoise(commitDiff, opts.diffOptions); + parts.push(buildFileSummaryTable(summary)); + const excluded = renderExcludedFiles(summary); + if (excluded) parts.push(excluded); parts.push(""); + // ── Diff (inline, or file-list + read instruction when oversized) ── + + parts.push(renderDiffSection(summary, filtered, "### Diff")); + parts.push(""); + + // ── Custom Review Focus ── + + if (opts.focus) { + parts.push("## Custom Review Focus"); + parts.push(opts.focus); + parts.push(""); + } + // ── Project Context ── - if (projectContext) { + if (opts.projectContext) { parts.push("## Additional Context"); - parts.push(projectContext); + parts.push(opts.projectContext); parts.push(""); } @@ -254,7 +278,7 @@ export function buildReviewPrompt( parts.push("## Review Instructions"); parts.push( - "Review the commit above against the task description. Check for:", + "Review the changes above against the task description. Check for:", ); parts.push(...reviewInstructions()); parts.push(""); @@ -279,7 +303,7 @@ export function buildReviewPromptUncommitted( project: Project, status: string, diff: string, - projectContext?: string, + opts: ReviewPromptOptions = {}, ): string { const parts: string[] = []; @@ -321,17 +345,36 @@ export function buildReviewPromptUncommitted( parts.push(status || "(no status output)"); parts.push("```"); parts.push(""); - parts.push("### Current Tracked Diff (git diff)"); - parts.push("```diff"); - parts.push(truncateDiff(diff) || "(no tracked diff output)"); - parts.push("```"); + + // ── Changed-Files Summary + Exclusions (noise-filtered scope) ── + + const summary = parseDiff(diff, opts.diffOptions); + const filtered = filterNoise(diff, opts.diffOptions); + parts.push(buildFileSummaryTable(summary)); + const excluded = renderExcludedFiles(summary); + if (excluded) parts.push(excluded); parts.push(""); + // ── Diff (inline, or file-list + read instruction when oversized) ── + + parts.push( + renderDiffSection(summary, filtered, "### Current Tracked Diff (git diff)"), + ); + parts.push(""); + + // ── Custom Review Focus ── + + if (opts.focus) { + parts.push("## Custom Review Focus"); + parts.push(opts.focus); + parts.push(""); + } + // ── Project Context ── - if (projectContext) { + if (opts.projectContext) { parts.push("## Additional Context"); - parts.push(projectContext); + parts.push(opts.projectContext); parts.push(""); } @@ -354,6 +397,78 @@ export function buildReviewPromptUncommitted( // ─── Shared Review Prompt Helpers ─────────────────────────────────────────── +/** Whether an oversized/wide diff should be replaced by a file-list + read + * instruction instead of being inlined. Thresholds: cleaned diff over + * MAX_DIFF_BYTES, or more than MAX_REVIEW_FILES included files. */ +function shouldSkipInline(summary: DiffSummary, filteredLength: number): boolean { + return ( + filteredLength > MAX_DIFF_BYTES || summary.files.length > MAX_REVIEW_FILES + ); +} + +/** + * Render a per-file +/− summary Markdown table (with type column and a total + * line) from a parsed diff. Handles the empty/all-noise diff gracefully — an + * empty table with zero totals, no crash. + */ +function buildFileSummaryTable(summary: DiffSummary): string { + const lines: string[] = []; + lines.push("### Changed Files"); + lines.push(""); + lines.push("| File | +/− | Type |"); + lines.push("|------|-----|------|"); + if (summary.files.length === 0) { + lines.push("| _(no included changes)_ | — | — |"); + } else { + for (const f of summary.files) { + lines.push( + `| \`${f.path}\` | +${f.linesAdded}/-${f.linesRemoved} | ${f.ext || "—"} |`, + ); + } + } + lines.push(`| **Total** | **+${summary.totalAdded}/-${summary.totalRemoved}** | |`); + return lines.join("\n"); +} + +/** + * Render the `### Excluded Files (n)` bullet list (path, +/− counts, reason). + * Returns an empty string when there are no exclusions so callers omit the + * section entirely (no empty heading). + */ +function renderExcludedFiles(summary: DiffSummary): string { + if (summary.excluded.length === 0) return ""; + const lines: string[] = []; + lines.push(`### Excluded Files (${summary.excluded.length})`); + lines.push(""); + for (const f of summary.excluded) { + lines.push( + `- \`${f.path}\` (+${f.linesAdded}/-${f.linesRemoved}) — ${f.reason}`, + ); + } + return lines.join("\n"); +} + +/** + * Render the diff section of a review prompt. Under the threshold, inline the + * noise-filtered diff. Over the threshold (size or file count), emit a + * file-list + read-instruction notice and never byte-truncate the diff. + */ +function renderDiffSection( + summary: DiffSummary, + filtered: string, + heading: string, +): string { + if (shouldSkipInline(summary, filtered.length)) { + return `${heading} — _Diff too large (${filtered.length.toLocaleString()} chars, ${summary.files.length} files). Use \`read\` to inspect the changed files._`; + } + const lines: string[] = []; + lines.push(heading); + lines.push("```diff"); + lines.push(filtered || "(no included changes)"); + lines.push("```"); + return lines.join("\n"); +} + function reviewInstructions(): string[] { return [ "- **Correctness**: Does the implementation fulfill the task requirements?", @@ -373,7 +488,7 @@ function reviewVerdictBlock(): string[] { "VERDICT: [pass | warn | fail]", "SUMMARY: [1-2 sentence overall assessment]", "FINDINGS:", - "- [blocker] file:line description (use severity: blocker|warning|nit|info)", + "- [blocker] file:line description (use severity: blocker|warning|nit|info; `critical` is accepted as a blocker synonym)", "- [warning] file:line description", "```", "", @@ -387,7 +502,8 @@ function reviewVerdictBlock(): string[] { "", "Each FINDINGS line uses the form `- [severity] [file:line] message`.", "The `file:line` part is optional. Severity must be one of:", - "`blocker`, `warning`, `nit`, `info`.", + "`blocker`, `warning`, `nit`, `info`. The `critical` token is accepted", + "and treated as `blocker`.", ]; } diff --git a/src/review.ts b/src/review.ts index c5a84f1..2f45d20 100644 --- a/src/review.ts +++ b/src/review.ts @@ -88,13 +88,16 @@ function extractFindings(block: string): ReviewFinding[] { .filter(Boolean); const findings: ReviewFinding[] = []; - const severityRe = /^\[(blocker|warning|warn|nit|info)\]\s*(.*)$/i; + // `critical` is accepted and normalized to ralpi's `blocker` severity, + // providing parity with @piex-dev/review's critical/warning/info grading. + const severityRe = /^\[(blocker|critical|warning|warn|nit|info)\]\s*(.*)$/i; for (const line of lines) { const sm = line.match(severityRe); if (sm) { let sev = sm[1].toLowerCase(); if (sev === "warn") sev = "warning"; + else if (sev === "critical") sev = "blocker"; const rest = sm[2].trim(); const { file, line: lineNum, message } = parseFileRef(rest); findings.push({ diff --git a/src/types.ts b/src/types.ts index 0bed376..5b6f9e0 100644 --- a/src/types.ts +++ b/src/types.ts @@ -246,6 +246,16 @@ export interface RalpiConfig { reviewBlockOnFail: boolean; /** Maximum total duration for the entire loop execution in milliseconds (0 = no limit). Checked between batches — in-progress tasks finish naturally. */ loopTimeoutMs: number; + /** Max attempts on the SAME model before cycling to the next model on + * failure. Pi retries transient HTTP errors within a single prompt, + * but a sustained provider hiccup can still exhaust those in-call + * retries mid-session. Re-running the whole session a few times on + * the same model avoids flapping to a different model (and losing + * model-specific context) on the first hard failure. Applies to task + * execution, commit/review follow-up sessions, and review-fix + * re-execution alike. After this many attempts on one model, ralpi + * advances to the next model in the round-robin pool. */ + maxSameModelAttempts: number; /** Isolate each task in a separate git worktree so parallel tasks can't * stomp each other's files, and review/commit see a clean single-task diff. * - "never": all tasks run in the shared working tree (default, backward compat) @@ -258,6 +268,18 @@ export interface RalpiConfig { projectContext: string; /** Custom prompt suffix for reflection extraction */ reflectionPrompt: string; + /** Per-review custom focus/instructions (e.g. "check security only"). + * Injected as a `### Custom Review Focus` section in committed and + * uncommitted review prompts when non-empty. */ + reviewFocus: string; + }; + review: { + /** Extra noise-filter exclusion regexes (strings compiled to RegExp), + * merged into EXCLUDED_PATTERNS for review diffs. */ + extraIgnorePatterns: string[]; + /** Pathspec allowlist — files matching these stay in scope even when a + * default noise rule would exclude them. */ + ignorePaths: string[]; }; /** Parent session model to inherit in child agent sessions */ model?: unknown; @@ -287,9 +309,15 @@ export const DEFAULT_CONFIG: RalpiConfig = { reviewBlockOnFail: false, // false = commit anyway after retries exhausted loopTimeoutMs: 0, // 0 = no limit worktrees: "parallel", // worktree isolation for parallel tasks by default + maxSameModelAttempts: 5, // retry the same model up to 5 times before cycling to the next }, prompts: { projectContext: "", reflectionPrompt: "", + reviewFocus: "", + }, + review: { + extraIgnorePatterns: [], + ignorePaths: [], }, }; diff --git a/src/utils.ts b/src/utils.ts index ec3c3fe..06ca176 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -910,17 +910,46 @@ export function captureGitHead(projectDir: string): string | undefined { * made since the base reference. Used by the review-gated loop so the reviewer * sees the full task diff (all commits, not just the latest) across execution * attempts and re-execution fixes. `baseRef` must be a validated hex SHA from - * captureGitHead(). Returns the short HEAD hash, HEAD subject, and range diff, - * or null when git is unavailable / baseRef is invalid / no changes exist. + * captureGitHead(). + * + * Returns a tri-state so the review loop can tell a FAILED range computation + * (invalid/stale base ref, git error) apart from a GENUINELY EMPTY range — a + * broken base must never be silently treated as a clean, verified task. */ +export type CommitRangeDiffResult = + | { kind: "ok"; hash: string; subject: string; diff: string } + | { kind: "no-changes" } + | { kind: "error"; error: string }; + +/** + * Whether the `baseRef..HEAD` range can be computed — i.e. the base ref is a + * resolvable commit in this repo (mirrors @piex-dev/review's canCompareToBase). + * Only validated hex SHAs are passed to the shell. + */ +export function canComputeRange(projectDir: string, baseRef: string): boolean { + const { execSync } = require("node:child_process"); + if (!/^[0-9a-f]{7,40}$/i.test(baseRef)) return false; + try { + execSync(`git rev-parse --verify ${baseRef}`, { + cwd: projectDir, + stdio: "pipe", + }); + return true; + } catch { + return false; + } +} + export function getCommitRangeDiff( projectDir: string, baseRef: string, -): { hash: string; subject: string; diff: string } | null { +): CommitRangeDiffResult { const { execSync } = require("node:child_process"); // Only pass validated hex SHAs to the shell. - if (!/^[0-9a-f]{7,40}$/i.test(baseRef)) return null; + if (!/^[0-9a-f]{7,40}$/i.test(baseRef)) { + return { kind: "error", error: "invalid or stale base ref" }; + } try { execSync("git rev-parse --git-dir", { @@ -928,7 +957,18 @@ export function getCommitRangeDiff( stdio: "pipe", }); } catch { - return null; + return { kind: "error", error: "not a git repository" }; + } + + // Verify the base ref resolves before diffing — a stale/unfetched ref is a + // computation failure, not a clean "no changes" signal. + try { + execSync(`git rev-parse --verify ${baseRef}`, { + cwd: projectDir, + stdio: "pipe", + }); + } catch { + return { kind: "error", error: `base ref ${baseRef} cannot be resolved` }; } try { @@ -946,17 +986,20 @@ export function getCommitRangeDiff( // the snapshot. Includes stat overview + full patch. // // maxBuffer is set high (10 MB) so larger tasks don't cause execSync to - // throw. The review prompt builder truncates to MAX_DIFF_BYTES (50 KB) - // before sending to the model, so the full diff in memory is fine. + // throw. The review prompt builder filters noise and inlines only under + // MAX_DIFF_BYTES, so the full diff in memory is fine. const diff = execSync(`git diff ${baseRef} HEAD --stat --patch`, { cwd: projectDir, encoding: "utf-8", maxBuffer: 10 * 1024 * 1024, }).trim(); - if (!diff) return null; // no changes since baseRef - return { hash, subject, diff }; - } catch { - return null; + if (!diff) return { kind: "no-changes" }; // genuinely no changes since baseRef + return { kind: "ok", hash, subject, diff }; + } catch (error) { + return { + kind: "error", + error: error instanceof Error ? error.message : String(error), + }; } } diff --git a/tests/review-prompt.test.ts b/tests/review-prompt.test.ts new file mode 100644 index 0000000..ffcb494 --- /dev/null +++ b/tests/review-prompt.test.ts @@ -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"); + }); +}); diff --git a/tests/review-severity.test.ts b/tests/review-severity.test.ts new file mode 100644 index 0000000..0621531 --- /dev/null +++ b/tests/review-severity.test.ts @@ -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"); + }); +});