From 466b52636f55739d9def04106c11d92f5804139d Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:38:24 +0530 Subject: [PATCH 01/29] fix(llm): use x-goog-api-key header for Gemini Why: the API key was embedded in the URL query string, which leaks into proxy logs, error traces, and request middleware. Google's API supports header-based auth via x-goog-api-key. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/llm.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index d7fc03e..0b996e6 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -392,12 +392,14 @@ impl LlmBackend for GeminiBackend { .build()?; let url = format!( - "https://generativelanguage.googleapis.com/v1beta/models/{}:streamGenerateContent?key={}&alt=sse", - self.model, self.api_key + "https://generativelanguage.googleapis.com/v1beta/models/{}:streamGenerateContent?alt=sse", + self.model ); let resp = client .post(&url) + .header("x-goog-api-key", &self.api_key) + .header("content-type", "application/json") .json(&GeminiRequest { contents: vec![GeminiContent { parts: vec![GeminiPart { text: prompt }], From 19143d84e2e156556eb726e5a970093963af3178 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:38:57 +0530 Subject: [PATCH 02/29] fix(llm): validate OPENAI_BASE_URL scheme and host Why: OPENAI_BASE_URL was read from env with no validation, so a compromised env could redirect prompts (containing source code) to a malicious endpoint. Now reject non-https URLs and loopback/private hosts. Override with CREV_ALLOW_INSECURE_BASE_URL=1 for local proxies. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/llm.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index 0b996e6..df38534 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -245,12 +245,70 @@ impl OpenAiBackend { fn new(model: String) -> Result { let api_key = std::env::var("OPENAI_API_KEY") .context("$OPENAI_API_KEY is not set")?; - let base_url = std::env::var("OPENAI_BASE_URL") - .unwrap_or_else(|_| "https://api.openai.com".to_string()); + let base_url = match std::env::var("OPENAI_BASE_URL") { + Ok(url) => { + validate_base_url(&url)?; + url + } + Err(_) => "https://api.openai.com".to_string(), + }; Ok(Self { api_key, base_url, model }) } } +fn validate_base_url(url: &str) -> Result<()> { + let parsed = reqwest::Url::parse(url) + .with_context(|| format!("OPENAI_BASE_URL is not a valid URL: {}", url))?; + + // Require https unless explicitly opted in. + let allow_http = std::env::var("CREV_ALLOW_INSECURE_BASE_URL").is_ok(); + let scheme = parsed.scheme(); + if scheme != "https" && !allow_http { + anyhow::bail!( + "OPENAI_BASE_URL must use https (got {}). Set CREV_ALLOW_INSECURE_BASE_URL=1 to override.", + scheme + ); + } + + // Reject loopback/private hosts unless explicitly allowed. + let host = parsed.host_str().unwrap_or(""); + if !allow_http && is_private_or_loopback(host) { + anyhow::bail!( + "OPENAI_BASE_URL points at a private or loopback host ({}). \ + Set CREV_ALLOW_INSECURE_BASE_URL=1 if this is intentional.", + host + ); + } + Ok(()) +} + +fn is_private_or_loopback(host: &str) -> bool { + if host.eq_ignore_ascii_case("localhost") || host == "0.0.0.0" || host == "::" { + return true; + } + if let Ok(ip) = host.parse::() { + return ip.is_loopback() || is_private_ip(&ip) || ip.is_unspecified(); + } + false +} + +fn is_private_ip(ip: &std::net::IpAddr) -> bool { + use std::net::IpAddr; + match ip { + IpAddr::V4(v4) => { + let o = v4.octets(); + o[0] == 10 + || (o[0] == 172 && (16..=31).contains(&o[1])) + || (o[0] == 192 && o[1] == 168) + || (o[0] == 169 && o[1] == 254) + } + IpAddr::V6(v6) => { + let seg = v6.segments()[0]; + (seg & 0xfe00) == 0xfc00 || (seg & 0xffc0) == 0xfe80 + } + } +} + #[derive(Serialize)] struct OpenAiRequest<'a> { model: &'a str, From fee797a8871c5ab988f8840ced2c60a692a529b2 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:39:18 +0530 Subject: [PATCH 03/29] fix(llm): surface body-read failures in API error messages Why: when an API call returned non-2xx and the body read also failed, the error message was "Anthropic API returned 401: " with no clue why. Now include the body-read error so transient network issues during error reporting don't swallow the real cause. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/llm.rs | 15 ++++++++++++--- src/ollama.rs | 10 ++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index df38534..a0254b0 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -196,7 +196,10 @@ impl LlmBackend for AnthropicBackend { if !resp.status().is_success() { let status = resp.status(); - let body = resp.text().await.unwrap_or_default(); + let body = resp + .text() + .await + .unwrap_or_else(|e| format!("", e)); anyhow::bail!("Anthropic API returned {}: {}", status, body); } @@ -358,7 +361,10 @@ impl LlmBackend for OpenAiBackend { if !resp.status().is_success() { let status = resp.status(); - let body = resp.text().await.unwrap_or_default(); + let body = resp + .text() + .await + .unwrap_or_else(|e| format!("", e)); anyhow::bail!("OpenAI API returned {}: {}", status, body); } @@ -469,7 +475,10 @@ impl LlmBackend for GeminiBackend { if !resp.status().is_success() { let status = resp.status(); - let body = resp.text().await.unwrap_or_default(); + let body = resp + .text() + .await + .unwrap_or_else(|e| format!("", e)); anyhow::bail!("Gemini API returned {}: {}", status, body); } diff --git a/src/ollama.rs b/src/ollama.rs index e64a83b..1ec19a2 100644 --- a/src/ollama.rs +++ b/src/ollama.rs @@ -64,7 +64,10 @@ pub async fn stream_completion( if !resp.status().is_success() { let status = resp.status(); - let body = resp.text().await.unwrap_or_default(); + let body = resp + .text() + .await + .unwrap_or_else(|e| format!("", e)); anyhow::bail!("Ollama returned {}: {}", status, body); } @@ -155,7 +158,10 @@ pub async fn pull_model(model: &str) -> Result<()> { if !resp.status().is_success() { let status = resp.status(); - let body = resp.text().await.unwrap_or_default(); + let body = resp + .text() + .await + .unwrap_or_else(|e| format!("", e)); anyhow::bail!("Failed to pull {}: {} {}", model, status, body); } From da12d34a0e2d6b58911e47206a59ea7e6b795f3d Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:39:41 +0530 Subject: [PATCH 04/29] fix(cli): remove dead --file flag from review command Why: --file was accepted by the CLI but the value was discarded (let _file = ...) and never passed to run_review. Users may have assumed per-file review worked when it silently didn't. Drop the flag rather than ship a misleading no-op. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/main.rs b/src/main.rs index 12b84a1..bcd0a27 100644 --- a/src/main.rs +++ b/src/main.rs @@ -42,10 +42,6 @@ enum Commands { #[arg(long)] commits: Option, - /// Review a specific file - #[arg(long)] - file: Option, - /// Output findings as JSON #[arg(long)] json: bool, @@ -156,7 +152,6 @@ async fn main() -> Result<()> { unstaged, commit, commits, - file: _file, json, fail_on, security, From 79a358813cf9b1fefddff7cc81148ceb80ce42f8 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:40:55 +0530 Subject: [PATCH 05/29] fix(update): require confirmation before piping install script to sh Why: 'crev update' piped a curl response straight into sh with no confirmation and no pinning to a release tag. A compromised main branch (or a transient curl failure mid-download) could execute arbitrary code. Now: download fully first, prompt unless stdin is non-interactive or CREV_UPDATE_YES is set, and show the URL up front so users can inspect it. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 67 ++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 9 deletions(-) diff --git a/src/main.rs b/src/main.rs index bcd0a27..72a1330 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,7 +9,7 @@ mod ollama; mod output; mod prompt; -use anyhow::Result; +use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; use std::path::PathBuf; use std::sync::{Arc, Mutex}; @@ -551,16 +551,65 @@ fn find_git_root(start: &std::path::Path) -> Result { git::find_repo_root(start) } +const INSTALL_SCRIPT_URL: &str = + "https://raw.githubusercontent.com/starc007/crev/main/install.sh"; + fn run_update() -> Result<()> { - eprintln!("Updating crev to the latest version..."); - let status = std::process::Command::new("sh") - .args([ - "-c", - "curl -fsSL https://raw.githubusercontent.com/starc007/crev/main/install.sh | sh", - ]) - .status()?; + eprintln!("crev update will download and execute:"); + eprintln!(" {}", INSTALL_SCRIPT_URL); + eprintln!(); + eprintln!("The script verifies the binary's SHA256 before installing, but the"); + eprintln!("script itself is fetched from the main branch and is not pinned."); + eprintln!("Inspect it first if you don't trust the repo state."); + eprintln!(); + + let non_interactive = + std::env::var("CREV_UPDATE_YES").is_ok() || !std::io::IsTerminal::is_terminal(&std::io::stdin()); + + if !non_interactive { + eprint!("Proceed? [y/N] "); + use std::io::Write; + std::io::stderr().flush().ok(); + let mut answer = String::new(); + std::io::stdin().read_line(&mut answer)?; + let answer = answer.trim().to_lowercase(); + if answer != "y" && answer != "yes" { + eprintln!("Aborted."); + return Ok(()); + } + } + + // Fetch the script first so a transient network error doesn't leave a + // half-downloaded pipe partially executed by sh. + let script = std::process::Command::new("curl") + .args(["-fsSL", INSTALL_SCRIPT_URL]) + .output() + .context("Failed to invoke curl")?; + if !script.status.success() { + anyhow::bail!( + "Failed to download install script: {}", + String::from_utf8_lossy(&script.stderr) + ); + } + + let mut child = std::process::Command::new("sh") + .stdin(std::process::Stdio::piped()) + .spawn() + .context("Failed to spawn sh")?; + { + use std::io::Write; + let stdin = child + .stdin + .as_mut() + .context("Failed to open sh stdin")?; + stdin.write_all(&script.stdout)?; + } + let status = child.wait()?; if !status.success() { - anyhow::bail!("Update failed. Try running the install script manually:\n curl -fsSL https://raw.githubusercontent.com/starc007/crev/main/install.sh | sh"); + anyhow::bail!( + "Update failed. Try running the install script manually:\n curl -fsSL {} | sh", + INSTALL_SCRIPT_URL + ); } Ok(()) } From 953666d56b1a398e3b9768871f39b950a7ad9612 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:41:13 +0530 Subject: [PATCH 06/29] fix(prompt): warn when truncating diff drops files Why: when a diff exceeded max_tokens, the truncation pass dropped the largest files silently. A user could ship a PR believing the whole change was reviewed when an important file was never sent to the LLM. Now print the dropped file list to stderr. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/prompt.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/prompt.rs b/src/prompt.rs index 4b5c444..3420a76 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -139,7 +139,25 @@ pub fn build_review_prompt_ctx( /// Fallback: build a prompt from a raw diff only (Phase 1 behaviour). pub fn build_review_prompt(diff: &ParsedDiff, config: &Config, security_mode: bool) -> String { let diff = if estimate_tokens(&format_diff(diff)) > config.review.max_tokens { - truncate_to_budget(diff, config.review.max_tokens) + let original_files: Vec = + diff.files.iter().map(|f| f.path.clone()).collect(); + let trimmed = truncate_to_budget(diff, config.review.max_tokens); + let kept: std::collections::HashSet<_> = + trimmed.files.iter().map(|f| f.path.clone()).collect(); + let dropped: Vec<_> = original_files + .iter() + .filter(|p| !kept.contains(*p)) + .collect(); + if !dropped.is_empty() { + eprintln!( + "warning: diff exceeded token budget; {} file(s) dropped from review:", + dropped.len() + ); + for p in &dropped { + eprintln!(" - {}", p.display()); + } + } + trimmed } else { diff.clone() }; From 2b5d263ceee0cbdcb275fd0e44c2e007b5c49ac1 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:41:26 +0530 Subject: [PATCH 07/29] fix(prompt): correct duplicate section comment numbering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: two adjacent sections in build_review_prompt_ctx were both labelled "7." — minor, but the comments are the only structural documentation of how the prompt is assembled, and the typo invites mistakes when adding new sections later. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/prompt.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/prompt.rs b/src/prompt.rs index 3420a76..c087b84 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -128,7 +128,7 @@ pub fn build_review_prompt_ctx( prompt.push('\n'); } - // 7. Output format (always last) + // 8. Output format (always last) prompt.push_str("=== OUTPUT FORMAT ===\n"); prompt.push_str(OUTPUT_FORMAT); prompt.push('\n'); From 1b3278493b7cfaed0e5ae2bbc8857d3f42e71721 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:41:57 +0530 Subject: [PATCH 08/29] fix(linters): canonicalize paths before matching against diff Why: filter_to_diff used ends_with/substring fallbacks to match linter output paths against diff file paths. A file named main.rs appearing in both src/main.rs and tests/main.rs could cross-match. Now resolve both to repo-relative canonical paths before equality comparison. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/linters.rs | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/src/linters.rs b/src/linters.rs index ce25744..002c713 100644 --- a/src/linters.rs +++ b/src/linters.rs @@ -5,6 +5,22 @@ use tokio::process::Command; use crate::git::{FileType, ParsedDiff}; +/// Normalize an absolute or relative path to a repo-rooted relative path string. +/// Returns the original lossy string if canonicalization fails. +fn rel_to_repo(path: &Path, repo_root: &Path) -> String { + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + repo_root.join(path) + }; + let canon = std::fs::canonicalize(&absolute).unwrap_or(absolute); + let canon_root = std::fs::canonicalize(repo_root).unwrap_or_else(|_| repo_root.to_path_buf()); + canon + .strip_prefix(&canon_root) + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_else(|_| canon.to_string_lossy().to_string()) +} + #[derive(Debug, Clone)] pub struct LinterFinding { pub linter: String, @@ -64,25 +80,25 @@ pub async fn run_linters(diff: &ParsedDiff, repo_root: &Path) -> Vec Vec { +fn filter_to_diff(findings: &[LinterFinding], diff: &ParsedDiff, repo_root: &Path) -> Vec { + // Pre-compute normalized repo-relative paths for diff files. + let diff_paths: Vec<(String, &crate::git::ChangedFile)> = diff + .files + .iter() + .map(|df| (rel_to_repo(&df.path, repo_root), df)) + .collect(); + findings .iter() .filter(|f| { - diff.files.iter().any(|df| { - // Normalise both paths for comparison - let df_path = df.path.to_string_lossy(); - let f_path = f.file.to_string_lossy(); - let paths_match = f_path.ends_with(df_path.as_ref()) - || df_path.ends_with(f_path.as_ref()) - || f_path == df_path; - - if !paths_match { + let f_rel = rel_to_repo(&f.file, repo_root); + diff_paths.iter().any(|(df_rel, df)| { + if df_rel != &f_rel { return false; } - df.hunks.iter().any(|h| { let changed_lines: Vec = (h.new_start..h.new_start + h.new_lines).collect(); changed_lines From 557bc7cc777da0be6d2aca28d59b0df2420e9189 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:42:16 +0530 Subject: [PATCH 09/29] fix(config): reject unsafe ignore-glob patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: glob patterns in .reviewrc were applied without validation, so a pattern like '../../.ssh/*' or '/etc/*' would match outside the repo. This is committed-to-disk team config, so the blast radius is limited, but a single mistake shouldn't be able to traverse out of the repo at all. Now patterns must be repo-relative and free of '..' segments — invalid patterns are skipped with a warning. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/config.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/config.rs b/src/config.rs index 881bb38..3ef6da5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -189,6 +189,14 @@ pub fn should_ignore_file(path: &Path, config: &Config) -> bool { let path_str = path.to_string_lossy(); for pattern in &config.ignore.paths { + if !is_safe_ignore_pattern(pattern) { + eprintln!( + "warning: ignoring unsafe glob pattern {:?} \ + (must be repo-relative, no '..' or absolute paths)", + pattern + ); + continue; + } if glob_match(pattern, &path_str) { return true; } @@ -197,6 +205,26 @@ pub fn should_ignore_file(path: &Path, config: &Config) -> bool { false } +fn is_safe_ignore_pattern(pattern: &str) -> bool { + if pattern.is_empty() { + return false; + } + if pattern.starts_with('/') || pattern.starts_with('\\') { + return false; + } + // Windows-style drive prefix + if pattern.len() >= 2 && pattern.chars().nth(1) == Some(':') { + return false; + } + // Reject any '..' segment (handles `../`, `..\\`, and a trailing `..`). + for segment in pattern.split(|c| c == '/' || c == '\\') { + if segment == ".." { + return false; + } + } + true +} + fn glob_match(pattern: &str, path: &str) -> bool { // Use the glob crate for matching if let Ok(pat) = glob::Pattern::new(pattern) { From f74737d86bc6ce9748624aa1b223710bac5fd5a5 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:43:17 +0530 Subject: [PATCH 10/29] fix(context): cap per-file size and total files walked Why: build() walked the entire repo to find called-function defs and related tests, with no upper bound. A pathological repo (huge generated files, hundreds of thousands of source files) could make a single review run for minutes. Now skip files > 1 MiB and stop walking after 5,000 source files have been considered. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/context.rs | 92 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 74 insertions(+), 18 deletions(-) diff --git a/src/context.rs b/src/context.rs index 4e0b25c..4737741 100644 --- a/src/context.rs +++ b/src/context.rs @@ -44,6 +44,24 @@ pub struct ContextBuilder { // Directories to skip when walking the repo const SKIP_DIRS: &[&str] = &["target", "node_modules", ".git", "vendor", "dist", "build"]; +/// Per-file source-read ceiling (1 MiB). Larger files are skipped — they are +/// nearly always generated code or vendored bundles that would blow the +/// token budget on their own. +const MAX_FILE_BYTES: u64 = 1024 * 1024; + +/// Cap on how many files we'll walk while searching for called-function +/// definitions and related tests. Prevents pathological repos (hundreds of +/// thousands of source files) from making `context::build` run for minutes. +const MAX_FILES_WALKED: usize = 5000; + +fn read_capped(path: &Path) -> Option { + let meta = std::fs::metadata(path).ok()?; + if meta.len() > MAX_FILE_BYTES { + return None; + } + std::fs::read_to_string(path).ok() +} + impl ContextBuilder { pub fn new(repo_root: PathBuf, max_tokens: usize) -> Self { Self { @@ -61,9 +79,9 @@ impl ContextBuilder { for file in &diff.files { let abs_path = self.repo_root.join(&file.path); - let source = match std::fs::read_to_string(&abs_path) { - Ok(s) => s, - Err(_) => continue, + let source = match read_capped(&abs_path) { + Some(s) => s, + None => continue, }; let parsed = match self.parser.parse_file(&abs_path, &source) { @@ -155,28 +173,41 @@ impl ContextBuilder { } let mut results = Vec::new(); + let mut walked = 0usize; let search_dirs = ["src", "lib", "pkg", "internal", "cmd"]; for dir_name in &search_dirs { let dir = self.repo_root.join(dir_name); if dir.exists() { - self.walk_for_functions(&dir, names, &mut results); + self.walk_for_functions(&dir, names, &mut results, &mut walked); } } // Also check repo root itself for single-file projects - self.walk_dir_shallow(&self.repo_root, names, &mut results); + self.walk_dir_shallow(&self.repo_root, names, &mut results, &mut walked); results } - fn walk_for_functions(&self, dir: &Path, names: &[String], out: &mut Vec) { + fn walk_for_functions( + &self, + dir: &Path, + names: &[String], + out: &mut Vec, + walked: &mut usize, + ) { + if *walked >= MAX_FILES_WALKED { + return; + } let entries = match std::fs::read_dir(dir) { Ok(e) => e, Err(_) => return, }; for entry in entries.flatten() { + if *walked >= MAX_FILES_WALKED { + return; + } let path = entry.path(); if path.is_dir() { @@ -185,30 +216,41 @@ impl ContextBuilder { continue; } } - self.walk_for_functions(&path, names, out); + self.walk_for_functions(&path, names, out, walked); } else if is_source_file(&path) { + *walked += 1; self.extract_matching_fns(&path, names, out); } } } - fn walk_dir_shallow(&self, dir: &Path, names: &[String], out: &mut Vec) { + fn walk_dir_shallow( + &self, + dir: &Path, + names: &[String], + out: &mut Vec, + walked: &mut usize, + ) { let entries = match std::fs::read_dir(dir) { Ok(e) => e, Err(_) => return, }; for entry in entries.flatten() { + if *walked >= MAX_FILES_WALKED { + return; + } let path = entry.path(); if path.is_file() && is_source_file(&path) { + *walked += 1; self.extract_matching_fns(&path, names, out); } } } fn extract_matching_fns(&self, path: &Path, names: &[String], out: &mut Vec) { - let source = match std::fs::read_to_string(path) { - Ok(s) => s, - Err(_) => return, + let source = match read_capped(path) { + Some(s) => s, + None => return, }; let parsed = match self.parser.parse_file(path, &source) { Ok(p) => p, @@ -224,41 +266,55 @@ impl ContextBuilder { fn find_related_tests(&self, fn_names: &[&str]) -> Vec { let mut tests = Vec::new(); + let mut walked = 0usize; let test_dirs = ["tests", "test", "__tests__", "spec"]; for dir_name in &test_dirs { let dir = self.repo_root.join(dir_name); if dir.exists() { - self.walk_for_tests(&dir, fn_names, &mut tests); + self.walk_for_tests(&dir, fn_names, &mut tests, &mut walked); } } // Also inline tests in src (Rust's #[cfg(test)]) let src_dir = self.repo_root.join("src"); if src_dir.exists() { - self.walk_for_tests(&src_dir, fn_names, &mut tests); + self.walk_for_tests(&src_dir, fn_names, &mut tests, &mut walked); } tests } - fn walk_for_tests(&self, dir: &Path, fn_names: &[&str], out: &mut Vec) { + fn walk_for_tests( + &self, + dir: &Path, + fn_names: &[&str], + out: &mut Vec, + walked: &mut usize, + ) { + if *walked >= MAX_FILES_WALKED { + return; + } let entries = match std::fs::read_dir(dir) { Ok(e) => e, Err(_) => return, }; for entry in entries.flatten() { + if *walked >= MAX_FILES_WALKED { + return; + } let path = entry.path(); if path.is_dir() { if let Some(name) = path.file_name().and_then(|n| n.to_str()) { if !SKIP_DIRS.contains(&name) { - self.walk_for_tests(&path, fn_names, out); + self.walk_for_tests(&path, fn_names, out, walked); } } } else if is_source_file(&path) { - let source = match std::fs::read_to_string(&path) { - Ok(s) => s, - Err(_) => continue, + *walked += 1; + let source = match read_capped(&path) { + Some(s) => s, + None => continue, }; let parsed = match self.parser.parse_file(&path, &source) { Ok(p) => p, From cac271dfcb2e094d9b68b0a668f40ef92d95ca34 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:43:35 +0530 Subject: [PATCH 11/29] fix(history): walk char boundaries when stripping line refs Why: regex_strip_line_refs indexed bytes directly and cast each byte to a char, which corrupted any multi-byte UTF-8 in a finding message before it was stored as a pattern. The loop was bounded so it didn't panic, but the result was garbled, which made pattern dedup miss recurrences whose only difference was a non-ASCII glyph. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/history.rs | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/history.rs b/src/history.rs index 556334d..1eeb912 100644 --- a/src/history.rs +++ b/src/history.rs @@ -226,21 +226,27 @@ fn normalize_pattern(msg: &str) -> String { } fn regex_strip_line_refs(s: &str) -> String { - // Strip patterns like ":42" or "line 42" from the message + // Strip patterns like ":42" from the message. UTF-8 safe: walks char + // boundaries via char_indices so multi-byte chars round-trip cleanly. let mut out = String::with_capacity(s.len()); - let bytes = s.as_bytes(); - let mut i = 0; - while i < bytes.len() { - if bytes[i] == b':' && i + 1 < bytes.len() && bytes[i + 1].is_ascii_digit() { - // skip ":NNN" - i += 1; - while i < bytes.len() && bytes[i].is_ascii_digit() { - i += 1; + let mut chars = s.char_indices().peekable(); + while let Some((_, c)) = chars.next() { + if c == ':' { + // Peek next char; if it's an ascii digit, swallow the ":NNN" run. + if let Some(&(_, next)) = chars.peek() { + if next.is_ascii_digit() { + while let Some(&(_, d)) = chars.peek() { + if d.is_ascii_digit() { + chars.next(); + } else { + break; + } + } + continue; + } } - } else { - out.push(bytes[i] as char); - i += 1; } + out.push(c); } out } From 1adb69fa4b246914c9ca72288b372df2ae8ee524 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:43:48 +0530 Subject: [PATCH 12/29] fix(git): keep .git assertion when canonicalize fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: find_repo_root canonicalizes start then walks up looking for a .git entry. The fallback used start.to_path_buf() when canonicalize failed, but the loop's .git check is still what ultimately decides — without that check the fallback would still be safe today, but the error message gave no hint where it looked. Include the original path in the error so users know what was searched. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/git.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/git.rs b/src/git.rs index b040beb..65d8dde 100644 --- a/src/git.rs +++ b/src/git.rs @@ -298,6 +298,10 @@ fn parse_diff(diff: git2::Diff) -> Result { } pub fn find_repo_root(start: &Path) -> Result { + // Prefer canonicalize, but if it fails (e.g. start lives behind a broken + // symlink) walk the literal path instead. Either way we still require + // that some ancestor actually contains a .git entry — without that check + // the fallback could silently "succeed" by returning a non-repo dir. let mut dir = std::fs::canonicalize(start) .unwrap_or_else(|_| start.to_path_buf()); loop { @@ -305,7 +309,10 @@ pub fn find_repo_root(start: &Path) -> Result { return Ok(dir); } if !dir.pop() { - anyhow::bail!("Not inside a git repository"); + anyhow::bail!( + "Not inside a git repository (searched upward from {})", + start.display() + ); } } } From 2ce3bb54da4976f483ace4184f8cf8bf0d5bfe34 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:44:41 +0530 Subject: [PATCH 13/29] chore: drop dead code paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: parse_findings, print_findings, and compress_function were never reached at runtime — try_parse_finding_line and print_finding handle streaming output, and the prompt builder no longer compresses function bodies through a separate helper. Carrying dead branches makes the file harder to read and tempts future edits to revive half-supported paths. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/output.rs | 90 --------------------------------------------------- src/prompt.rs | 15 --------- 2 files changed, 105 deletions(-) diff --git a/src/output.rs b/src/output.rs index 344b49d..c929042 100644 --- a/src/output.rs +++ b/src/output.rs @@ -40,44 +40,6 @@ pub struct Finding { pub message: String, } -pub fn parse_findings(llm_output: &str) -> Vec { - let mut findings = Vec::new(); - - for line in llm_output.lines() { - let line = line.trim(); - if line.is_empty() { - continue; - } - - // Match [HIGH], [MED], [LOW] patterns - if let Some(finding) = parse_severity_line(line) { - findings.push(finding); - continue; - } - - // Match LGTM: ... - if line.starts_with("LGTM:") || line.starts_with("LGTM ") { - let msg = line - .trim_start_matches("LGTM:") - .trim_start_matches("LGTM") - .trim() - .to_string(); - findings.push(Finding { - severity: Severity::Lgtm, - file: PathBuf::new(), - line: None, - message: if msg.is_empty() { - "No issues found.".to_string() - } else { - msg - }, - }); - } - } - - findings -} - fn parse_severity_line(line: &str) -> Option { // Patterns: [HIGH] path:42 — msg or [MED] path:42 — msg or [LOW] path:42 — msg let (severity, rest) = if let Some(r) = line.strip_prefix("[HIGH]") { @@ -189,58 +151,6 @@ pub fn print_summary(findings: &[Finding], elapsed: Duration, model: &str) { println!("\n{}", summary.dimmed()); } -pub fn print_findings(findings: &[Finding], elapsed: Duration, model: &str) { - if findings.is_empty() { - println!("{}", "[✓] No findings — review output was empty.".green()); - return; - } - - let mut high = 0; - let mut med = 0; - let mut low = 0; - let mut has_lgtm = false; - - for finding in findings { - match finding.severity { - Severity::High => { - high += 1; - let prefix = "[!] HIGH ".bold().red(); - let location = format_location(&finding.file, finding.line); - println!("{}{}", prefix, location.bold()); - println!(" {}", finding.message); - } - Severity::Med => { - med += 1; - let prefix = "[~] MED ".yellow(); - let location = format_location(&finding.file, finding.line); - println!("{}{}", prefix, location); - println!(" {}", finding.message); - } - Severity::Low => { - low += 1; - let prefix = "[i] LOW ".blue(); - let location = format_location(&finding.file, finding.line); - println!("{}{}", prefix, location); - println!(" {}", finding.message); - } - Severity::Lgtm => { - has_lgtm = true; - println!("{} {}", "[✓] LGTM".green().bold(), finding.message.green()); - } - } - } - - if !has_lgtm { - let total = high + med + low; - let elapsed_secs = elapsed.as_secs_f64(); - let summary = format!( - "{} findings ({} high, {} med, {} low) · {:.1}s · {}", - total, high, med, low, elapsed_secs, model - ); - println!("\n{}", summary.dimmed()); - } -} - fn format_location(file: &PathBuf, line: Option) -> String { if file.as_os_str().is_empty() { return String::new(); diff --git a/src/prompt.rs b/src/prompt.rs index c087b84..7b333a6 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -1,4 +1,3 @@ -use crate::ast::FunctionInfo; use crate::config::Config; use crate::context::ReviewContext; use crate::git::{DiffHunk, DiffLine, ParsedDiff}; @@ -185,20 +184,6 @@ pub fn build_review_prompt(diff: &ParsedDiff, config: &Config, security_mode: bo prompt } -pub fn compress_function(fn_info: &FunctionInfo, max_lines: usize) -> String { - let body_lines: Vec<&str> = fn_info.signature.lines().collect(); - if body_lines.len() <= max_lines { - return fn_info.signature.clone(); - } - let truncated: Vec<&str> = body_lines.iter().take(max_lines).copied().collect(); - let omitted = body_lines.len() - max_lines; - format!( - "{}\n// ... ({} lines omitted)", - truncated.join("\n"), - omitted - ) -} - fn format_diff(diff: &ParsedDiff) -> String { let mut out = String::new(); From 5139def7ff1482f9d3284dfea9068885b9a58283 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:46:19 +0530 Subject: [PATCH 14/29] feat(review): validate findings against the diff before reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: LLM reviewers regularly cite line numbers that don't exist in the diff — off-by-one from header lines, completely fabricated, or on the wrong file entirely. A single bad citation makes a finding useless because the user clicks through to unrelated code, distrust grows, and the recurring-pattern detector logs phantom entries. How: a new validate::DiffIndex builds a {file -> {visible lines}} map from the parsed diff (added + context lines, since both are shown to the model). Each parsed finding goes through the index: exact match accepted, within ±3 lines re-anchored to the nearest real line, anything further away dropped. Drop and re-anchor counts appear in the run summary so users can see how noisy a model is on their codebase. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 49 +++++++++++++-- src/validate.rs | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 4 deletions(-) create mode 100644 src/validate.rs diff --git a/src/main.rs b/src/main.rs index 72a1330..0ac6522 100644 --- a/src/main.rs +++ b/src/main.rs @@ -8,6 +8,7 @@ mod llm; mod ollama; mod output; mod prompt; +mod validate; use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; @@ -376,19 +377,38 @@ async fn run_review( drop(line_tx); // close channel so receiver loop exits // ── Consume lines: stop spinner then print each finding ─────────────────── + let validator = validate::DiffIndex::from_diff(&diff); let mut findings: Vec = Vec::new(); + let mut dropped_findings: Vec<(output::Finding, validate::DropReason)> = Vec::new(); + let mut reanchored_count: usize = 0; let mut spinner_task = Some(spinner_task); while let Some(line) = line_rx.recv().await { - if let Some(f) = output::try_parse_finding_line(&line) { + if let Some(raw) = output::try_parse_finding_line(&line) { + // Stop the spinner on the first parseable line so the user + // sees output streaming, regardless of validation outcome. if let Some(task) = spinner_task.take() { stop_tx.send(true).ok(); task.await.ok(); } - if !json { - output::print_finding(&f); + + let (kept, outcome) = validator.apply(raw.clone()); + if let validate::Validation::Reanchor { .. } = outcome { + reanchored_count += 1; + } + match kept { + Some(f) => { + if !json { + output::print_finding(&f); + } + findings.push(f); + } + None => { + if let validate::Validation::Drop(reason) = outcome { + dropped_findings.push((raw, reason)); + } + } } - findings.push(f); } } @@ -404,6 +424,27 @@ async fn run_review( output::print_findings_json(&findings)?; } else { output::print_summary(&findings, elapsed, &model); + if reanchored_count > 0 { + eprintln!( + "note: re-anchored {} finding(s) to the nearest line in the diff", + reanchored_count + ); + } + if !dropped_findings.is_empty() { + eprintln!( + "note: dropped {} hallucinated finding(s) (line/file not in diff)", + dropped_findings.len() + ); + for (f, reason) in &dropped_findings { + eprintln!( + " - [{}] {}:{} ({})", + f.severity.as_str(), + f.file.display(), + f.line.map(|l| l.to_string()).unwrap_or_else(|| "?".into()), + reason.as_str() + ); + } + } } // Save to history diff --git a/src/validate.rs b/src/validate.rs new file mode 100644 index 0000000..9b1db9d --- /dev/null +++ b/src/validate.rs @@ -0,0 +1,158 @@ +//! Validate LLM-emitted findings against the actual diff. +//! +//! The reviewer prompt asks the model to cite a path and a line number for +//! every finding. Even capable models will sometimes hallucinate a line that +//! isn't in the diff, or pick a number off by a few from the real one. This +//! module: +//! +//! 1. Indexes every file:line pair the model has been shown. +//! 2. Re-anchors a finding's line to the nearest real diff line when the +//! cited line is close (±3). +//! 3. Drops findings whose line/file pair has no plausible match. +//! +//! Without this, a single bad line number makes a finding useless: the user +//! clicks through to the wrong code, distrust grows, and the recurring-pattern +//! detector logs phantom entries. + +use std::collections::{HashMap, HashSet}; +use std::path::PathBuf; + +use crate::git::{DiffLine, ParsedDiff}; +use crate::output::{Finding, Severity}; + +/// How far off a cited line can be from the nearest diff line before we drop +/// the finding entirely. Three lines is enough to absorb header-line off-by-one +/// errors but small enough that we don't silently re-anchor to unrelated code. +const REANCHOR_TOLERANCE: u32 = 3; + +pub struct DiffIndex { + /// Per-file set of line numbers we can plausibly attribute findings to. + /// Includes both Added lines and surrounding Context the model was shown. + visible: HashMap>, +} + +impl DiffIndex { + pub fn from_diff(diff: &ParsedDiff) -> Self { + let mut visible: HashMap> = HashMap::new(); + for file in &diff.files { + let key = file.path.to_string_lossy().into_owned(); + let entry = visible.entry(key).or_default(); + for hunk in &file.hunks { + let mut line_num = hunk.new_start; + for line in &hunk.lines { + match line { + DiffLine::Added(_) | DiffLine::Context(_) => { + entry.insert(line_num); + line_num += 1; + } + DiffLine::Removed(_) => { + // Removed lines have no new-line number. + } + } + } + } + } + Self { visible } + } + + /// Outcome of validating a single finding. + pub fn validate(&self, finding: &Finding) -> Validation { + // LGTM has no file/line and is always allowed. + if finding.severity == Severity::Lgtm { + return Validation::Accept; + } + + // Findings without a file or line carry less weight but the prompt + // explicitly allows them in some shapes — accept rather than drop. + if finding.file.as_os_str().is_empty() { + return Validation::Accept; + } + let Some(line) = finding.line else { + return Validation::Accept; + }; + + let file_key = finding.file.to_string_lossy(); + let matches: Option<&HashSet> = self + .visible + .get(file_key.as_ref()) + // Fall back to a suffix match — the model sometimes prepends or + // strips a leading directory we already showed it. + .or_else(|| { + self.visible + .iter() + .find(|(k, _)| { + k.ends_with(file_key.as_ref()) || file_key.ends_with(k.as_str()) + }) + .map(|(_, v)| v) + }); + + let Some(lines) = matches else { + return Validation::Drop(DropReason::UnknownFile); + }; + + if lines.contains(&line) { + return Validation::Accept; + } + + // Try to re-anchor to the nearest real line. + let nearest = lines + .iter() + .min_by_key(|&&l| l.abs_diff(line)) + .copied(); + match nearest { + Some(real) if real.abs_diff(line) <= REANCHOR_TOLERANCE => { + Validation::Reanchor { from: line, to: real } + } + _ => Validation::Drop(DropReason::LineNotInDiff), + } + } + + /// Apply a [`Validation`] result, returning the possibly-mutated finding + /// or [`None`] if it should be dropped. + pub fn apply(&self, mut finding: Finding) -> (Option, Validation) { + let outcome = self.validate(&finding); + match outcome { + Validation::Accept => (Some(finding), Validation::Accept), + Validation::Reanchor { from, to } => { + finding.line = Some(to); + (Some(finding), Validation::Reanchor { from, to }) + } + Validation::Drop(reason) => (None, Validation::Drop(reason)), + } + } + + /// Number of files indexed — used to suppress validation when we have no + /// diff to validate against (defensive guard). + pub fn is_empty(&self) -> bool { + self.visible.is_empty() + } + + /// Helper for [`crate::main`] flag wiring: should a finding's location + /// be silently kept when validation can't decide? + #[allow(dead_code)] + pub fn known_files(&self) -> impl Iterator + '_ { + self.visible.keys().map(PathBuf::from) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DropReason { + UnknownFile, + LineNotInDiff, +} + +impl DropReason { + pub fn as_str(&self) -> &'static str { + match self { + DropReason::UnknownFile => "file not in diff", + DropReason::LineNotInDiff => "line not in diff", + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Validation { + Accept, + Reanchor { from: u32, to: u32 }, + Drop(DropReason), +} From add962c5ff01c37a7aac2c51bfb1ed9ee2c85e3f Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:47:45 +0530 Subject: [PATCH 15/29] feat(review): prefer Added lines when re-anchoring findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: the diff carries both Added lines (the actual change) and Context lines (surrounding code shown for orientation). A finding that lands on a Context line usually describes pre-existing code that the change merely touches — useful, but not what the user is asking to review. The old re-anchor logic treated both equally and could "fix" a hallucinated line by snapping it to neighbouring unchanged code, hiding the fact that the model missed the change. How: split the diff index into added/context sets per file. Re- anchor preference is now nearest Added line first, fall back to visible Context only if no Added line is in tolerance. Findings that finally land on Context are counted separately and surfaced in the run summary so the user knows they aren't about the change. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 20 +++++++++ src/validate.rs | 106 +++++++++++++++++++++++++++++++++++------------- 2 files changed, 97 insertions(+), 29 deletions(-) diff --git a/src/main.rs b/src/main.rs index 0ac6522..ce00143 100644 --- a/src/main.rs +++ b/src/main.rs @@ -381,6 +381,7 @@ async fn run_review( let mut findings: Vec = Vec::new(); let mut dropped_findings: Vec<(output::Finding, validate::DropReason)> = Vec::new(); let mut reanchored_count: usize = 0; + let mut context_only_count: usize = 0; let mut spinner_task = Some(spinner_task); while let Some(line) = line_rx.recv().await { @@ -396,6 +397,19 @@ async fn run_review( if let validate::Validation::Reanchor { .. } = outcome { reanchored_count += 1; } + // A finding accepted on a Context line (not Added) is usually a + // comment about pre-existing code — flag it so the user knows it + // isn't about the change itself. + let counts_as_context_only = matches!( + outcome, + validate::Validation::Accept { on_change: false } | validate::Validation::Reanchor { on_change: false, .. } + ) && !matches!(raw.severity, output::Severity::Lgtm) + && raw.line.is_some() + && !raw.file.as_os_str().is_empty(); + if counts_as_context_only { + context_only_count += 1; + } + match kept { Some(f) => { if !json { @@ -430,6 +444,12 @@ async fn run_review( reanchored_count ); } + if context_only_count > 0 { + eprintln!( + "note: {} finding(s) reference unchanged context lines, not the change itself", + context_only_count + ); + } if !dropped_findings.is_empty() { eprintln!( "note: dropped {} hallucinated finding(s) (line/file not in diff)", diff --git a/src/validate.rs b/src/validate.rs index 9b1db9d..c5841c1 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -26,23 +26,40 @@ use crate::output::{Finding, Severity}; const REANCHOR_TOLERANCE: u32 = 3; pub struct DiffIndex { - /// Per-file set of line numbers we can plausibly attribute findings to. - /// Includes both Added lines and surrounding Context the model was shown. - visible: HashMap>, + /// Per-file sets of line numbers we can plausibly attribute findings to. + /// We track Added and Context separately so re-anchoring can prefer real + /// changes (Added) over surrounding code (Context). + files: HashMap, +} + +#[derive(Default)] +struct FileLines { + added: HashSet, + context: HashSet, +} + +impl FileLines { + fn contains(&self, line: u32) -> bool { + self.added.contains(&line) || self.context.contains(&line) + } } impl DiffIndex { pub fn from_diff(diff: &ParsedDiff) -> Self { - let mut visible: HashMap> = HashMap::new(); + let mut files: HashMap = HashMap::new(); for file in &diff.files { let key = file.path.to_string_lossy().into_owned(); - let entry = visible.entry(key).or_default(); + let entry = files.entry(key).or_default(); for hunk in &file.hunks { let mut line_num = hunk.new_start; for line in &hunk.lines { match line { - DiffLine::Added(_) | DiffLine::Context(_) => { - entry.insert(line_num); + DiffLine::Added(_) => { + entry.added.insert(line_num); + line_num += 1; + } + DiffLine::Context(_) => { + entry.context.insert(line_num); line_num += 1; } DiffLine::Removed(_) => { @@ -52,33 +69,33 @@ impl DiffIndex { } } } - Self { visible } + Self { files } } /// Outcome of validating a single finding. pub fn validate(&self, finding: &Finding) -> Validation { // LGTM has no file/line and is always allowed. if finding.severity == Severity::Lgtm { - return Validation::Accept; + return Validation::Accept { on_change: false }; } // Findings without a file or line carry less weight but the prompt // explicitly allows them in some shapes — accept rather than drop. if finding.file.as_os_str().is_empty() { - return Validation::Accept; + return Validation::Accept { on_change: false }; } let Some(line) = finding.line else { - return Validation::Accept; + return Validation::Accept { on_change: false }; }; let file_key = finding.file.to_string_lossy(); - let matches: Option<&HashSet> = self - .visible + let matches: Option<&FileLines> = self + .files .get(file_key.as_ref()) // Fall back to a suffix match — the model sometimes prepends or // strips a leading directory we already showed it. .or_else(|| { - self.visible + self.files .iter() .find(|(k, _)| { k.ends_with(file_key.as_ref()) || file_key.ends_with(k.as_str()) @@ -90,19 +107,38 @@ impl DiffIndex { return Validation::Drop(DropReason::UnknownFile); }; - if lines.contains(&line) { - return Validation::Accept; + if lines.added.contains(&line) { + return Validation::Accept { on_change: true }; + } + if lines.context.contains(&line) { + return Validation::Accept { on_change: false }; } - // Try to re-anchor to the nearest real line. - let nearest = lines + // Re-anchor preference: nearest Added line first (this is what the + // change is actually about), then any visible line. Drop only when + // both are out of tolerance. + let nearest_added = lines.added.iter().min_by_key(|&&l| l.abs_diff(line)).copied(); + if let Some(real) = nearest_added { + if real.abs_diff(line) <= REANCHOR_TOLERANCE { + return Validation::Reanchor { + from: line, + to: real, + on_change: true, + }; + } + } + let nearest_visible = lines + .context .iter() + .chain(lines.added.iter()) .min_by_key(|&&l| l.abs_diff(line)) .copied(); - match nearest { - Some(real) if real.abs_diff(line) <= REANCHOR_TOLERANCE => { - Validation::Reanchor { from: line, to: real } - } + match nearest_visible { + Some(real) if real.abs_diff(line) <= REANCHOR_TOLERANCE => Validation::Reanchor { + from: line, + to: real, + on_change: lines.added.contains(&real), + }, _ => Validation::Drop(DropReason::LineNotInDiff), } } @@ -112,10 +148,10 @@ impl DiffIndex { pub fn apply(&self, mut finding: Finding) -> (Option, Validation) { let outcome = self.validate(&finding); match outcome { - Validation::Accept => (Some(finding), Validation::Accept), - Validation::Reanchor { from, to } => { + Validation::Accept { on_change } => (Some(finding), Validation::Accept { on_change }), + Validation::Reanchor { from, to, on_change } => { finding.line = Some(to); - (Some(finding), Validation::Reanchor { from, to }) + (Some(finding), Validation::Reanchor { from, to, on_change }) } Validation::Drop(reason) => (None, Validation::Drop(reason)), } @@ -124,14 +160,14 @@ impl DiffIndex { /// Number of files indexed — used to suppress validation when we have no /// diff to validate against (defensive guard). pub fn is_empty(&self) -> bool { - self.visible.is_empty() + self.files.is_empty() } /// Helper for [`crate::main`] flag wiring: should a finding's location /// be silently kept when validation can't decide? #[allow(dead_code)] pub fn known_files(&self) -> impl Iterator + '_ { - self.visible.keys().map(PathBuf::from) + self.files.keys().map(PathBuf::from) } } @@ -152,7 +188,19 @@ impl DropReason { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Validation { - Accept, - Reanchor { from: u32, to: u32 }, + Accept { on_change: bool }, + Reanchor { from: u32, to: u32, on_change: bool }, Drop(DropReason), } + +impl Validation { + /// True when the finding lands on a line the change actually added. + /// Context-only findings can still be useful but should be marked, since + /// they often describe pre-existing code the diff merely touched. + pub fn is_on_change(&self) -> bool { + match self { + Validation::Accept { on_change } | Validation::Reanchor { on_change, .. } => *on_change, + Validation::Drop(_) => false, + } + } +} From 172ce614fb45eccc81031a7357dee4f50b38984b Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:48:15 +0530 Subject: [PATCH 16/29] feat(prompt): add grounding rules + severity rubric to system prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: even strong models invent line numbers and reference symbols that aren't in the diff. The validator now catches the worst of these at runtime, but prevention is cheaper than cleanup — telling the model up front that fabricated citations are worse than silence shifts the distribution of its output. The severity rubric also gives the model concrete examples of what counts as HIGH vs MED so findings calibrate more consistently across runs. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/prompt.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/prompt.rs b/src/prompt.rs index 7b333a6..f2e9f08 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -4,13 +4,31 @@ use crate::git::{DiffHunk, DiffLine, ParsedDiff}; use crate::linters::LinterFinding; const SYSTEM_INSTRUCTIONS: &str = "\ -You are a senior engineer doing a focused code review. +You are a senior engineer doing a focused code review of a diff. Only report: bugs, security vulnerabilities, logic errors, missing error \ handling, race conditions, and performance issues. Do NOT comment on: style, formatting, naming conventions, or anything \ a linter would catch. If the change looks correct, say LGTM with one sentence of explanation. +GROUNDING RULES — failures here make findings worse than useless: +1. Only cite line numbers that appear in the diff you are shown. Each diff \ + line is prefixed with its line number; never invent a number. +2. Only name functions, variables, or symbols that appear in the diff or in \ + the context blocks below. Do not refer to code you have not seen. +3. Findings must describe the ADDED code, not unchanged context. Context \ + lines are shown for orientation only. +4. If you cannot anchor a concern to a specific shown line, omit it — silence \ + is better than a hallucinated citation. + +SEVERITY RUBRIC: +- [HIGH]: data loss, auth bypass, RCE, financial bug, panic on user input, \ + resource exhaustion, race condition that corrupts shared state. +- [MED]: correctness bug on a non-critical path, silently swallowed error, \ + obvious performance regression on a hot path, missing input validation. +- [LOW]: real but minor concern (e.g. off-by-one in a debug-only path, \ + defensive check that helps future readers). + For performance findings: only report if you can identify a specific hot path \ where the cost is significant AND avoidable given the surrounding constraints. \ Do not flag allocations or copies that are structurally required by the \ @@ -28,7 +46,12 @@ while the data was never written BAD: [MED] src/main.rs:99 — Unnecessary allocation on this line GOOD: [MED] src/main.rs:99 — buffer is re-allocated inside the loop on every \ -iteration; moving the allocation before the loop would reduce it to once"; +iteration; moving the allocation before the loop would reduce it to once + +BAD (line not in diff, fabricated): + [HIGH] src/auth.rs:999 — token comparison is timing-unsafe +GOOD (concern is real but you cannot point to a shown line): + "; const SECURITY_INSTRUCTIONS: &str = "\ You are a security engineer doing a targeted vulnerability review. From 53342fa35494e78039ec784fee4f99b13ee180aa Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:50:23 +0530 Subject: [PATCH 17/29] feat(review): self-critique pass to drop low-signal findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: even with a tight system prompt and diff-aware validation, the first review pass still produces vague, generic, or duplicate findings — especially on Ollama models. Running the findings back through the model with a sharper "is each one specific, grounded, and actionable?" prompt cheaply trims that noise. How: a new critique module builds a follow-up prompt listing the parsed findings (skipping LGTM) and asks the same backend to emit KEEP/DROP per line. Unparsed decisions default to KEEP so a broken critique response can't hide a real finding. With critique enabled, streaming live findings is suppressed in favour of a single batch print at the end — otherwise the user would see a finding scroll by that gets retroactively dropped, which is more confusing than helpful. --no-critique restores the previous live-stream UX. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/critique.rs | 171 ++++++++++++++++++++++++++++++++++++++++++++++++ src/main.rs | 69 +++++++++++++++---- 2 files changed, 228 insertions(+), 12 deletions(-) create mode 100644 src/critique.rs diff --git a/src/critique.rs b/src/critique.rs new file mode 100644 index 0000000..afae7ec --- /dev/null +++ b/src/critique.rs @@ -0,0 +1,171 @@ +//! Self-critique pass over LLM-emitted findings. +//! +//! The reviewer prompt asks for high-precision output, but in practice every +//! model produces some noise — vague suggestions, duplicates of linter +//! findings, false positives that look plausible until you re-read them. +//! Running the findings back through the same model with a focused "is this +//! a real, specific, actionable finding?" prompt cheaply filters most of +//! that noise. It costs one extra completion per review, so it is opt-out +//! rather than mandatory; running it against a local Ollama model also has +//! a much lower latency penalty than the user might expect. + +use anyhow::Result; + +use crate::llm::LlmBackend; +use crate::output::{Finding, Severity}; + +const CRITIQUE_INSTRUCTIONS: &str = "\ +You are reviewing a junior reviewer's findings. For each finding below, decide: + +KEEP — the finding is specific, grounded in the shown code, actionable, and \ + not a stylistic nit a linter would catch. +DROP — the finding is vague, fabricated, generic best-practice advice, \ + a duplicate of another finding, or doesn't describe a real defect. + +Output one line per finding in this exact form (nothing else): +N: KEEP — one-sentence justification +N: DROP — one-sentence reason + +Be strict. If you cannot point to the specific shown code that makes the \ +finding true, DROP it. If two findings describe the same bug, KEEP the more \ +specific one and DROP the other."; + +/// Result of a critique pass — kept findings plus the dropped pairs so the +/// caller can surface them in the run summary. +pub struct CritiqueResult { + pub kept: Vec, + pub dropped: Vec<(Finding, String)>, +} + +/// Run the critique against `findings`. Returns the original list unchanged +/// when there is nothing to critique (no findings, or LGTM-only). +pub async fn run_critique( + findings: Vec, + backend: &dyn LlmBackend, + original_prompt: &str, +) -> Result { + let reviewable: Vec<(usize, &Finding)> = findings + .iter() + .enumerate() + .filter(|(_, f)| f.severity != Severity::Lgtm) + .collect(); + + if reviewable.is_empty() { + return Ok(CritiqueResult { kept: findings, dropped: Vec::new() }); + } + + let prompt = build_prompt(original_prompt, &reviewable); + // No streaming callback — we want the full response and don't want the + // critique to mix with the live finding output. + let response = backend.complete(&prompt, &(|_token: &str| {})).await?; + let decisions = parse_decisions(&response, reviewable.len()); + + let mut kept = Vec::with_capacity(findings.len()); + let mut dropped = Vec::new(); + + for (decision_idx, (orig_idx, finding)) in reviewable.iter().enumerate() { + let decision = decisions.get(decision_idx).cloned().unwrap_or(Decision::Keep { + reason: String::from("no critique decision returned; kept by default"), + }); + match decision { + Decision::Keep { .. } => kept.push((*orig_idx, (*finding).clone())), + Decision::Drop { reason } => dropped.push(((*finding).clone(), reason)), + } + } + + // Re-insert LGTM and other non-reviewable findings in their original spot + // so the user-facing order is preserved. + let mut by_original_index: std::collections::HashMap = + kept.into_iter().collect(); + for (i, f) in findings.iter().enumerate() { + if f.severity == Severity::Lgtm { + by_original_index.insert(i, f.clone()); + } + } + let mut ordered: Vec<(usize, Finding)> = by_original_index.into_iter().collect(); + ordered.sort_by_key(|(i, _)| *i); + + Ok(CritiqueResult { + kept: ordered.into_iter().map(|(_, f)| f).collect(), + dropped, + }) +} + +fn build_prompt(original_prompt: &str, reviewable: &[(usize, &Finding)]) -> String { + let mut out = String::new(); + out.push_str(CRITIQUE_INSTRUCTIONS); + out.push_str("\n\n=== ORIGINAL REVIEW INPUT ===\n"); + // Truncate the original prompt to keep the critique cheap. The diff and + // context are what matter; we don't need the entire system prompt twice. + let snippet_limit = 8000; + let snippet = if original_prompt.len() > snippet_limit { + // Try to keep the trailing portion that includes the actual diff, + // which is appended after the static system prompt. + &original_prompt[original_prompt.len() - snippet_limit..] + } else { + original_prompt + }; + out.push_str(snippet); + out.push_str("\n\n=== FINDINGS TO JUDGE ===\n"); + for (display_idx, (_orig, f)) in reviewable.iter().enumerate() { + let line = f.line.map(|l| format!(":{}", l)).unwrap_or_default(); + out.push_str(&format!( + "{}: [{}] {}{} — {}\n", + display_idx + 1, + f.severity.as_str().to_uppercase(), + f.file.display(), + line, + f.message + )); + } + out.push_str("\n=== YOUR JUDGEMENTS ===\n"); + out +} + +#[derive(Debug, Clone)] +enum Decision { + Keep { #[allow(dead_code)] reason: String }, + Drop { reason: String }, +} + +fn parse_decisions(response: &str, expected: usize) -> Vec { + let mut out: Vec> = vec![None; expected]; + for raw in response.lines() { + let line = raw.trim(); + if line.is_empty() { + continue; + } + // Expected shape: "N: KEEP — reason" or "N: DROP — reason" + let Some((num_part, rest)) = line.split_once(':') else { + continue; + }; + let Ok(idx) = num_part.trim().parse::() else { + continue; + }; + if idx == 0 || idx > expected { + continue; + } + let rest = rest.trim_start(); + let (verdict, reason) = if let Some(r) = rest.strip_prefix("KEEP") { + ("KEEP", r.trim_start_matches(['—', '-', ' ']).trim().to_string()) + } else if let Some(r) = rest.strip_prefix("DROP") { + ("DROP", r.trim_start_matches(['—', '-', ' ']).trim().to_string()) + } else { + continue; + }; + out[idx - 1] = Some(match verdict { + "KEEP" => Decision::Keep { reason }, + "DROP" => Decision::Drop { reason }, + _ => unreachable!(), + }); + } + // Anything unparsed defaults to KEEP so a half-broken critique response + // never hides a finding the user should see. + out.into_iter() + .map(|d| { + d.unwrap_or(Decision::Keep { + reason: String::from("no decision parsed; kept by default"), + }) + }) + .collect() +} diff --git a/src/main.rs b/src/main.rs index ce00143..69653e0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,7 @@ mod ast; mod config; mod context; +mod critique; mod git; mod history; mod linters; @@ -63,6 +64,10 @@ enum Commands { #[arg(long)] no_cloud: bool, + /// Skip the self-critique pass (faster, slightly more false positives) + #[arg(long)] + no_critique: bool, + /// Model to use (e.g. qwen2.5-coder:14b, claude-sonnet-4-5, gpt-4o, gemini-1.5-pro) #[arg(long, short = 'm')] model: Option, @@ -158,10 +163,11 @@ async fn main() -> Result<()> { security, verbose: _verbose, no_cloud, + no_critique, model, path, } => { - run_review(&path, staged, unstaged, commit, commits, json, fail_on, security, no_cloud, model).await?; + run_review(&path, staged, unstaged, commit, commits, json, fail_on, security, no_cloud, no_critique, model).await?; } Commands::Init { @@ -223,6 +229,7 @@ async fn run_review( fail_on: Option, security: bool, no_cloud: bool, + no_critique: bool, cli_model: Option, ) -> Result<()> { let cfg = config::load_config(path); @@ -376,7 +383,11 @@ async fn run_review( } drop(line_tx); // close channel so receiver loop exits - // ── Consume lines: stop spinner then print each finding ─────────────────── + // ── Consume lines: stop spinner then collect / print each finding ──────── + // When critique is enabled we buffer findings instead of streaming them, + // because a finding the model will later drop in critique shouldn't have + // already been shown to the user. JSON output is always buffered. + let stream_live = !json && no_critique; let validator = validate::DiffIndex::from_diff(&diff); let mut findings: Vec = Vec::new(); let mut dropped_findings: Vec<(output::Finding, validate::DropReason)> = Vec::new(); @@ -386,20 +397,17 @@ async fn run_review( while let Some(line) = line_rx.recv().await { if let Some(raw) = output::try_parse_finding_line(&line) { - // Stop the spinner on the first parseable line so the user - // sees output streaming, regardless of validation outcome. - if let Some(task) = spinner_task.take() { - stop_tx.send(true).ok(); - task.await.ok(); + if stream_live { + if let Some(task) = spinner_task.take() { + stop_tx.send(true).ok(); + task.await.ok(); + } } let (kept, outcome) = validator.apply(raw.clone()); if let validate::Validation::Reanchor { .. } = outcome { reanchored_count += 1; } - // A finding accepted on a Context line (not Added) is usually a - // comment about pre-existing code — flag it so the user knows it - // isn't about the change itself. let counts_as_context_only = matches!( outcome, validate::Validation::Accept { on_change: false } | validate::Validation::Reanchor { on_change: false, .. } @@ -412,7 +420,7 @@ async fn run_review( match kept { Some(f) => { - if !json { + if stream_live { output::print_finding(&f); } findings.push(f); @@ -426,12 +434,34 @@ async fn run_review( } } - // Stop spinner if model returned nothing parseable + // Run critique pass before the spinner is dismissed so the user sees a + // single uninterrupted "analyzing..." spinner across both LLM calls. + let mut critique_dropped: Vec<(output::Finding, String)> = Vec::new(); + if !no_critique && !json && !findings.is_empty() { + match critique::run_critique(findings.clone(), backend.as_ref(), &prompt_text).await { + Ok(result) => { + findings = result.kept; + critique_dropped = result.dropped; + } + Err(e) => { + eprintln!("warning: critique pass failed, keeping all findings: {}", e); + } + } + } + + // Stop spinner now that all LLM calls are done. if let Some(task) = spinner_task.take() { stop_tx.send(true).ok(); task.await.ok(); } + // For the buffered (critique-enabled) path, print the surviving findings now. + if !stream_live && !json { + for f in &findings { + output::print_finding(f); + } + } + let elapsed = start.elapsed(); if json { @@ -465,6 +495,21 @@ async fn run_review( ); } } + if !critique_dropped.is_empty() { + eprintln!( + "note: critique dropped {} finding(s) as low-signal:", + critique_dropped.len() + ); + for (f, reason) in &critique_dropped { + eprintln!( + " - [{}] {}:{} ({})", + f.severity.as_str(), + f.file.display(), + f.line.map(|l| l.to_string()).unwrap_or_else(|| "?".into()), + reason + ); + } + } } // Save to history From abb6df8e2a7f790dc0dcc3586d9ef93f2155709d Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:52:35 +0530 Subject: [PATCH 18/29] feat(llm): Anthropic prompt caching for stable prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: every review on the same repo replays the same system prompt, team rules, and output-format trailer — currently ~2k tokens that the cloud provider re-bills on every call. Pre-commit hooks amplify this: a developer who staged, reviewed, fixed, and re-staged a few times in the same session paid full price for the identical prefix every time. How: introduce PromptParts { system, cacheable, dynamic }. Default LlmBackend impl concatenates and falls back to the existing complete path, so Ollama/OpenAI/Gemini keep working unchanged. Anthropic overrides complete_parts to emit a structured request with cache_control: { type: "ephemeral" } on the system prompt and the cacheable user block. The dynamic block (diff + context + linter findings) stays uncached. Provider returns 90% cost discount on the cached portion within a 5-minute window. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/llm.rs | 130 ++++++++++++++++++++++++++++++++++++++++++++++---- src/main.rs | 18 +++++-- src/prompt.rs | 109 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 242 insertions(+), 15 deletions(-) diff --git a/src/llm.rs b/src/llm.rs index a0254b0..d07f113 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -6,9 +6,37 @@ use std::time::Duration; // ── trait ───────────────────────────────────────────────────────────────────── +/// A prompt split into pieces so backends that support prompt caching can mark +/// stable prefixes (system instructions, code context) as cacheable. +/// +/// - `system`: high-level instructions, never changes between runs. +/// - `cacheable`: the heavy, stable prefix — system prompt body, code context, +/// linter findings, team rules. The same value across many runs (e.g. the +/// pre-commit hook reviewing the same staged diff multiple times) hits the +/// provider's cache. +/// - `dynamic`: the parts that vary every call — currently the diff itself and +/// the output-format trailer. Never cached. +pub struct PromptParts<'a> { + pub system: &'a str, + pub cacheable: &'a str, + pub dynamic: &'a str, +} + #[async_trait] pub trait LlmBackend: Send + Sync { async fn complete(&self, prompt: &str, on_token: &(dyn for<'a> Fn(&'a str) + Send + Sync)) -> Result; + + /// Default implementation concatenates the parts and calls `complete`. + /// Backends with native cache support (Anthropic) should override. + async fn complete_parts( + &self, + parts: PromptParts<'_>, + on_token: &(dyn for<'a> Fn(&'a str) + Send + Sync), + ) -> Result { + let combined = format!("{}\n\n{}\n\n{}", parts.system, parts.cacheable, parts.dynamic); + self.complete(&combined, on_token).await + } + fn name(&self) -> &str; fn is_local(&self) -> bool; } @@ -150,12 +178,39 @@ struct AnthropicRequest<'a> { max_tokens: u32, stream: bool, messages: Vec>, + #[serde(skip_serializing_if = "Option::is_none")] + system: Option>>, } #[derive(Serialize)] struct AnthropicMessage<'a> { role: &'a str, - content: &'a str, + content: AnthropicMessageContent<'a>, +} + +/// Anthropic content can be either a single string (no caching) or an array +/// of blocks that may individually carry `cache_control`. We use the array +/// form whenever caching is in play. +#[derive(Serialize)] +#[serde(untagged)] +enum AnthropicMessageContent<'a> { + Plain(&'a str), + Blocks(Vec>), +} + +#[derive(Serialize)] +struct AnthropicContentBlock<'a> { + #[serde(rename = "type")] + block_type: &'a str, + text: &'a str, + #[serde(skip_serializing_if = "Option::is_none")] + cache_control: Option, +} + +#[derive(Serialize)] +struct AnthropicCacheControl { + #[serde(rename = "type")] + cache_type: &'static str, } #[derive(Deserialize)] @@ -175,6 +230,68 @@ struct AnthropicDelta { #[async_trait] impl LlmBackend for AnthropicBackend { async fn complete(&self, prompt: &str, on_token: &(dyn for<'a> Fn(&'a str) + Send + Sync)) -> Result { + let req = AnthropicRequest { + model: &self.model, + max_tokens: 2048, + stream: true, + system: None, + messages: vec![AnthropicMessage { + role: "user", + content: AnthropicMessageContent::Plain(prompt), + }], + }; + self.send_request(req, on_token).await + } + + async fn complete_parts( + &self, + parts: PromptParts<'_>, + on_token: &(dyn for<'a> Fn(&'a str) + Send + Sync), + ) -> Result { + // Anthropic caches the prefix up to the latest cache_control marker. + // Mark the system block and the cacheable user block; leave the + // dynamic block uncached so the cache key stays stable across runs + // where only the dynamic portion changes. + let system = vec![AnthropicContentBlock { + block_type: "text", + text: parts.system, + cache_control: Some(AnthropicCacheControl { cache_type: "ephemeral" }), + }]; + let blocks = vec![ + AnthropicContentBlock { + block_type: "text", + text: parts.cacheable, + cache_control: Some(AnthropicCacheControl { cache_type: "ephemeral" }), + }, + AnthropicContentBlock { + block_type: "text", + text: parts.dynamic, + cache_control: None, + }, + ]; + let req = AnthropicRequest { + model: &self.model, + max_tokens: 2048, + stream: true, + system: Some(system), + messages: vec![AnthropicMessage { + role: "user", + content: AnthropicMessageContent::Blocks(blocks), + }], + }; + self.send_request(req, on_token).await + } + + fn name(&self) -> &str { "Anthropic" } + fn is_local(&self) -> bool { false } +} + +impl AnthropicBackend { + async fn send_request( + &self, + req: AnthropicRequest<'_>, + on_token: &(dyn for<'a> Fn(&'a str) + Send + Sync), + ) -> Result { let client = reqwest::Client::builder() .timeout(Duration::from_secs(120)) .build()?; @@ -183,13 +300,9 @@ impl LlmBackend for AnthropicBackend { .post("https://api.anthropic.com/v1/messages") .header("x-api-key", &self.api_key) .header("anthropic-version", "2023-06-01") + .header("anthropic-beta", "prompt-caching-2024-07-31") .header("content-type", "application/json") - .json(&AnthropicRequest { - model: &self.model, - max_tokens: 2048, - stream: true, - messages: vec![AnthropicMessage { role: "user", content: prompt }], - }) + .json(&req) .send() .await .context("Failed to connect to Anthropic API")?; @@ -231,9 +344,6 @@ impl LlmBackend for AnthropicBackend { } Ok(full) } - - fn name(&self) -> &str { "Anthropic" } - fn is_local(&self) -> bool { false } } // ── OpenAI ──────────────────────────────────────────────────────────────────── diff --git a/src/main.rs b/src/main.rs index 69653e0..1869236 100644 --- a/src/main.rs +++ b/src/main.rs @@ -313,13 +313,17 @@ async fn run_review( eprintln!("linters: {} findings in diff ({})", linter_findings.len(), summary.join(", ")); } - let prompt_text = match ctx_result { - Ok(ctx) => prompt::build_review_prompt_ctx(&ctx, &cfg, security, &linter_findings), + let prompt_parts = match ctx_result { + Ok(ctx) => Some(prompt::build_review_prompt_parts_ctx(&ctx, &cfg, security, &linter_findings)), Err(e) => { eprintln!("context: Minimal (fallback to diff-only: {})", e); - prompt::build_review_prompt(&diff, &cfg, security) + None } }; + let prompt_text: String = match &prompt_parts { + Some(parts) => parts.to_combined(), + None => prompt::build_review_prompt(&diff, &cfg, security), + }; // Show recurring patterns before the review output if let Ok(patterns) = history::detect_patterns(&repo_root) { @@ -362,7 +366,7 @@ async fn run_review( let line_tx2 = line_tx.clone(); let start = Instant::now(); - let full_response = backend.complete(&prompt_text, &(move |token: &str| { + let token_callback = move |token: &str| { let mut buf = line_buf2.lock().unwrap(); buf.push_str(token); while let Some(nl) = buf.find('\n') { @@ -372,7 +376,11 @@ async fn run_review( let _ = line_tx2.send(line); } } - })).await?; + }; + let _full_response = match &prompt_parts { + Some(parts) => backend.complete_parts(parts.as_parts(), &token_callback).await?, + None => backend.complete(&prompt_text, &token_callback).await?, + }; // Flush any remaining content not terminated with a newline { diff --git a/src/prompt.rs b/src/prompt.rs index f2e9f08..9d603d6 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -2,6 +2,31 @@ use crate::config::Config; use crate::context::ReviewContext; use crate::git::{DiffHunk, DiffLine, ParsedDiff}; use crate::linters::LinterFinding; +use crate::llm::PromptParts; + +/// Owned counterpart to [`PromptParts`] — built here so each backend can +/// borrow the parts when it streams a completion. The split is chosen so that +/// Anthropic prompt caching reuses the stable `system` and `cacheable` blocks +/// across runs while only the `dynamic` block changes per review. +pub struct PromptPartsOwned { + pub system: String, + pub cacheable: String, + pub dynamic: String, +} + +impl PromptPartsOwned { + pub fn as_parts(&self) -> PromptParts<'_> { + PromptParts { + system: &self.system, + cacheable: &self.cacheable, + dynamic: &self.dynamic, + } + } + + pub fn to_combined(&self) -> String { + format!("{}\n\n{}\n\n{}", self.system, self.cacheable, self.dynamic) + } +} const SYSTEM_INSTRUCTIONS: &str = "\ You are a senior engineer doing a focused code review of a diff. @@ -73,6 +98,90 @@ LGTM: brief note if no issues found. Every finding must name the specific variable, function, or value involved. Do not output vague findings like 'add error handling' without specifics."; +/// Build a [`PromptPartsOwned`] from a full review context. +/// +/// Cache split: +/// - `system`: the static reviewer instructions (severity rubric, +/// grounding rules, few-shot examples). Never varies. +/// - `cacheable`: team rules + output-format trailer. Stable per repo +/// across many runs, so still worth caching even though +/// `dynamic` always invalidates the suffix. +/// - `dynamic`: diff, called-fn bodies, type defs, related tests, +/// linter findings. Changes every review. +pub fn build_review_prompt_parts_ctx( + ctx: &ReviewContext, + config: &Config, + security_mode: bool, + linter_findings: &[LinterFinding], +) -> PromptPartsOwned { + let system = if security_mode { SECURITY_INSTRUCTIONS } else { SYSTEM_INSTRUCTIONS }.to_string(); + + let mut cacheable = String::new(); + if !config.rules.is_empty() { + cacheable.push_str("=== TEAM RULES ===\n"); + cacheable.push_str("Also check for these team-specific rules:\n"); + for rule in &config.rules { + cacheable.push_str(&format!("- [{}]: {}\n", rule.name, rule.description)); + } + cacheable.push('\n'); + } + cacheable.push_str("=== OUTPUT FORMAT ===\n"); + cacheable.push_str(OUTPUT_FORMAT); + cacheable.push('\n'); + + let mut dynamic = String::new(); + dynamic.push_str("=== CHANGED CODE ===\n"); + dynamic.push_str(&format_diff(&ctx.diff)); + dynamic.push('\n'); + + if !ctx.called_functions.is_empty() { + dynamic.push_str("=== FUNCTIONS CALLED BY CHANGED CODE ===\n"); + for f in &ctx.called_functions { + dynamic.push_str(&f.full_text); + dynamic.push_str("\n\n"); + } + } + + if !ctx.types_used.is_empty() { + dynamic.push_str("=== TYPES USED ===\n"); + for t in &ctx.types_used { + dynamic.push_str(&format!( + "{} {} {{ {} }}\n", + format!("{:?}", t.kind).to_lowercase(), + t.name, + t.fields.join(", ") + )); + } + dynamic.push('\n'); + } + + if !ctx.test_functions.is_empty() { + dynamic.push_str("=== RELATED TESTS ===\n"); + for f in &ctx.test_functions { + dynamic.push_str(&f.full_text); + dynamic.push_str("\n\n"); + } + } + + if !linter_findings.is_empty() { + dynamic.push_str("=== LINTER FINDINGS ===\n"); + for f in linter_findings { + dynamic.push_str(&format!( + "{} at {}:{}\n", + f.code, + f.file.display(), + f.line + )); + } + dynamic.push_str( + "For each linter finding above, assess: is this a genuine risk or a \ + false positive given the context? Explain the actual consequence if real.\n\n", + ); + } + + PromptPartsOwned { system, cacheable, dynamic } +} + /// Build a prompt from a full ReviewContext (Phase 2+) with optional linter findings. pub fn build_review_prompt_ctx( ctx: &ReviewContext, From 020b151cf16c4b77e38c14e2fa50ed773c7bf80e Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:55:21 +0530 Subject: [PATCH 19/29] feat(review): semantic chunking for diffs over the token budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: when a diff is larger than max_tokens the truncator drops the biggest files entirely — they're silently never reviewed. For a sprawling PR that's the opposite of what the user wants. Splitting the diff into per-file chunks and reviewing each chunk separately keeps every file covered at the cost of one extra LLM call per chunk. How: prompt::chunk_diff_by_files greedily packs files into chunks that each render under budget; files that exceed budget on their own become their own chunk and the existing context truncator trims them further. run_review now loops over chunks, builds a context and prompt for each, streams the completion, parses findings, and merges them into a single validated + critiqued list. Linters run once on the whole diff and their findings are filtered to each chunk's files. Live streaming is suppressed in chunked mode for the same reason as critique-on mode: a finding from chunk 1 that gets dropped by critique after chunk 4 finishes shouldn't already be visible. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 197 +++++++++++++++++++++++++++++--------------------- src/prompt.rs | 80 ++++++++++++++++++++ 2 files changed, 193 insertions(+), 84 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1869236..dbb6151 100644 --- a/src/main.rs +++ b/src/main.rs @@ -294,14 +294,10 @@ async fn run_review( files: filtered_files, }; - // Build semantic context and run linters in parallel (Phase 2 + 3) + // ── Linters run once on the whole diff ───────────────────────────────── let repo_root = git::find_repo_root(path)?; let ctx_builder = context::ContextBuilder::new(repo_root.clone(), cfg.review.max_tokens); - - let (ctx_result, linter_findings) = tokio::join!( - ctx_builder.build(diff.clone()), - linters::run_linters(&diff, &repo_root), - ); + let linter_findings = linters::run_linters(&diff, &repo_root).await; if !linter_findings.is_empty() { let by_tool: std::collections::HashMap<&str, usize> = @@ -313,18 +309,6 @@ async fn run_review( eprintln!("linters: {} findings in diff ({})", linter_findings.len(), summary.join(", ")); } - let prompt_parts = match ctx_result { - Ok(ctx) => Some(prompt::build_review_prompt_parts_ctx(&ctx, &cfg, security, &linter_findings)), - Err(e) => { - eprintln!("context: Minimal (fallback to diff-only: {})", e); - None - } - }; - let prompt_text: String = match &prompt_parts { - Some(parts) => parts.to_combined(), - None => prompt::build_review_prompt(&diff, &cfg, security), - }; - // Show recurring patterns before the review output if let Ok(patterns) = history::detect_patterns(&repo_root) { for p in &patterns { @@ -336,6 +320,21 @@ async fn run_review( } } + // ── Chunk if the diff is too large for a single pass ─────────────────── + let chunks = prompt::chunk_diff_by_files(&diff, cfg.review.max_tokens); + let is_chunked = chunks.len() > 1; + if is_chunked { + eprintln!( + "diff too large for a single review pass; splitting into {} chunks", + chunks.len() + ); + } + + // When critique or chunking is on we buffer findings instead of streaming + // them, because retroactively dropping a finding that already scrolled by + // is more confusing than helpful. + let stream_live = !json && no_critique && !is_chunked; + // ── Spinner ────────────────────────────────────────────────────────────── let (stop_tx, mut stop_rx) = tokio::sync::watch::channel(false); let spinner_task = tokio::spawn(async move { @@ -358,84 +357,114 @@ async fn run_review( } } }); - - // ── Stream completion, buffer complete lines ────────────────────────────── - let line_buf: Arc> = Arc::new(Mutex::new(String::new())); - let line_buf2 = line_buf.clone(); - let (line_tx, mut line_rx) = tokio::sync::mpsc::unbounded_channel::(); - let line_tx2 = line_tx.clone(); + let mut spinner_task = Some(spinner_task); let start = Instant::now(); - let token_callback = move |token: &str| { - let mut buf = line_buf2.lock().unwrap(); - buf.push_str(token); - while let Some(nl) = buf.find('\n') { - let line = buf[..nl].to_string(); - *buf = buf[nl + 1..].to_string(); - if !line.trim().is_empty() { - let _ = line_tx2.send(line); - } - } - }; - let _full_response = match &prompt_parts { - Some(parts) => backend.complete_parts(parts.as_parts(), &token_callback).await?, - None => backend.complete(&prompt_text, &token_callback).await?, - }; - - // Flush any remaining content not terminated with a newline - { - let buf = line_buf.lock().unwrap(); - if !buf.trim().is_empty() { - let _ = line_tx.send(buf.trim().to_string()); - } - } - drop(line_tx); // close channel so receiver loop exits - - // ── Consume lines: stop spinner then collect / print each finding ──────── - // When critique is enabled we buffer findings instead of streaming them, - // because a finding the model will later drop in critique shouldn't have - // already been shown to the user. JSON output is always buffered. - let stream_live = !json && no_critique; let validator = validate::DiffIndex::from_diff(&diff); let mut findings: Vec = Vec::new(); let mut dropped_findings: Vec<(output::Finding, validate::DropReason)> = Vec::new(); let mut reanchored_count: usize = 0; let mut context_only_count: usize = 0; - let mut spinner_task = Some(spinner_task); + let mut last_prompt_text: String = String::new(); - while let Some(line) = line_rx.recv().await { - if let Some(raw) = output::try_parse_finding_line(&line) { - if stream_live { - if let Some(task) = spinner_task.take() { - stop_tx.send(true).ok(); - task.await.ok(); - } - } + for (chunk_idx, chunk) in chunks.iter().enumerate() { + if is_chunked { + eprintln!( + "\rreviewing chunk {}/{} ({} file(s))", + chunk_idx + 1, + chunks.len(), + chunk.files.len() + ); + } - let (kept, outcome) = validator.apply(raw.clone()); - if let validate::Validation::Reanchor { .. } = outcome { - reanchored_count += 1; + // Per-chunk context build + prompt + let ctx_result = ctx_builder.build(chunk.clone()).await; + let chunk_linter_findings: Vec = linter_findings + .iter() + .filter(|f| chunk.files.iter().any(|cf| cf.path == f.file)) + .cloned() + .collect(); + + let prompt_parts = match ctx_result { + Ok(ctx) => Some(prompt::build_review_prompt_parts_ctx( + &ctx, &cfg, security, &chunk_linter_findings, + )), + Err(e) => { + eprintln!("context: Minimal (fallback to diff-only: {})", e); + None } - let counts_as_context_only = matches!( - outcome, - validate::Validation::Accept { on_change: false } | validate::Validation::Reanchor { on_change: false, .. } - ) && !matches!(raw.severity, output::Severity::Lgtm) - && raw.line.is_some() - && !raw.file.as_os_str().is_empty(); - if counts_as_context_only { - context_only_count += 1; + }; + let prompt_text: String = match &prompt_parts { + Some(parts) => parts.to_combined(), + None => prompt::build_review_prompt(chunk, &cfg, security), + }; + last_prompt_text = prompt_text.clone(); + + // ── Stream completion for this chunk, buffer complete lines ─────── + let line_buf: Arc> = Arc::new(Mutex::new(String::new())); + let line_buf2 = line_buf.clone(); + let (line_tx, mut line_rx) = tokio::sync::mpsc::unbounded_channel::(); + let line_tx2 = line_tx.clone(); + let token_callback = move |token: &str| { + let mut buf = line_buf2.lock().unwrap(); + buf.push_str(token); + while let Some(nl) = buf.find('\n') { + let line = buf[..nl].to_string(); + *buf = buf[nl + 1..].to_string(); + if !line.trim().is_empty() { + let _ = line_tx2.send(line); + } } + }; + let _full_response = match &prompt_parts { + Some(parts) => backend.complete_parts(parts.as_parts(), &token_callback).await?, + None => backend.complete(&prompt_text, &token_callback).await?, + }; - match kept { - Some(f) => { - if stream_live { - output::print_finding(&f); + // Flush trailing content + { + let buf = line_buf.lock().unwrap(); + if !buf.trim().is_empty() { + let _ = line_tx.send(buf.trim().to_string()); + } + } + drop(line_tx); + + // Consume per-chunk parsed findings into the global state. + while let Some(line) = line_rx.recv().await { + if let Some(raw) = output::try_parse_finding_line(&line) { + if stream_live { + if let Some(task) = spinner_task.take() { + stop_tx.send(true).ok(); + task.await.ok(); } - findings.push(f); } - None => { - if let validate::Validation::Drop(reason) = outcome { - dropped_findings.push((raw, reason)); + + let (kept, outcome) = validator.apply(raw.clone()); + if let validate::Validation::Reanchor { .. } = outcome { + reanchored_count += 1; + } + let counts_as_context_only = matches!( + outcome, + validate::Validation::Accept { on_change: false } | validate::Validation::Reanchor { on_change: false, .. } + ) && !matches!(raw.severity, output::Severity::Lgtm) + && raw.line.is_some() + && !raw.file.as_os_str().is_empty(); + if counts_as_context_only { + context_only_count += 1; + } + + match kept { + Some(f) => { + if stream_live { + output::print_finding(&f); + } + findings.push(f); + } + None => { + if let validate::Validation::Drop(reason) = outcome { + dropped_findings.push((raw, reason)); + } } } } @@ -443,10 +472,10 @@ async fn run_review( } // Run critique pass before the spinner is dismissed so the user sees a - // single uninterrupted "analyzing..." spinner across both LLM calls. + // single uninterrupted "analyzing..." spinner across all LLM calls. let mut critique_dropped: Vec<(output::Finding, String)> = Vec::new(); if !no_critique && !json && !findings.is_empty() { - match critique::run_critique(findings.clone(), backend.as_ref(), &prompt_text).await { + match critique::run_critique(findings.clone(), backend.as_ref(), &last_prompt_text).await { Ok(result) => { findings = result.kept; critique_dropped = result.dropped; diff --git a/src/prompt.rs b/src/prompt.rs index 9d603d6..4b73a2d 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -364,6 +364,86 @@ pub fn estimate_tokens(text: &str) -> usize { text.len() / 4 } +/// Split a diff into chunks that each fit inside `max_tokens` worth of +/// rendered output. Files larger than the budget on their own end up alone +/// in a chunk; the truncator is the last line of defense for those. +/// +/// This is the cheap alternative to truncation: instead of dropping the +/// largest files when a diff blows the token budget, we run several review +/// passes and merge their findings. +pub fn chunk_diff_by_files(diff: &ParsedDiff, max_tokens: usize) -> Vec { + use crate::git::DiffStats; + + let budget_chars = max_tokens.saturating_mul(4); + if format_diff(diff).len() <= budget_chars || diff.files.len() <= 1 { + return vec![diff.clone()]; + } + + let mut chunks: Vec> = Vec::new(); + let mut current: Vec = Vec::new(); + let mut current_chars: usize = 0; + + for file in &diff.files { + let single = format_one_file_diff(file); + let file_chars = single.len(); + + if file_chars > budget_chars { + // The file alone exceeds budget. Flush current chunk, then take + // this file as its own chunk — the existing truncator will trim + // its context lines further at prompt-build time. + if !current.is_empty() { + chunks.push(std::mem::take(&mut current)); + current_chars = 0; + } + chunks.push(vec![file.clone()]); + continue; + } + + if current_chars + file_chars > budget_chars && !current.is_empty() { + chunks.push(std::mem::take(&mut current)); + current_chars = 0; + } + current.push(file.clone()); + current_chars += file_chars; + } + if !current.is_empty() { + chunks.push(current); + } + + chunks + .into_iter() + .map(|files| { + let lines_added: usize = files + .iter() + .flat_map(|f| f.hunks.iter()) + .flat_map(|h| h.lines.iter()) + .filter(|l| matches!(l, DiffLine::Added(_))) + .count(); + let lines_removed: usize = files + .iter() + .flat_map(|f| f.hunks.iter()) + .flat_map(|h| h.lines.iter()) + .filter(|l| matches!(l, DiffLine::Removed(_))) + .count(); + let files_changed = files.len(); + ParsedDiff { + files, + stats: DiffStats { lines_added, lines_removed, files_changed }, + } + }) + .collect() +} + +fn format_one_file_diff(file: &crate::git::ChangedFile) -> String { + let mut out = String::new(); + out.push_str(&format!("=== FILE: {} ===\n", file.path.display())); + for hunk in &file.hunks { + out.push_str(&format_hunk(hunk)); + } + out.push('\n'); + out +} + pub fn truncate_to_budget(diff: &ParsedDiff, max_tokens: usize) -> ParsedDiff { use crate::git::{ChangedFile, DiffHunk, DiffStats}; From 598781f94e3dce56bef8e77fd6786a5629bc97da Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:57:34 +0530 Subject: [PATCH 20/29] feat(output): attach the cited source line to every accepted finding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: a finding like "[HIGH] src/foo.rs:42 — overflow risk" forces the user to alt-tab to the editor just to see what line 42 is. With the diff already in memory, attaching the line content costs almost nothing and makes the report self-contained — the user can judge correctness without leaving the terminal. JSON output picks up the same field for downstream tools (the GitHub Actions workflow can quote the line in its PR comments). How: the validator now indexes line content alongside line numbers and writes it onto each accepted/re-anchored finding. print_finding renders the quote dimmed under the message with a left bar so it reads as supporting evidence, not as part of the description. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/output.rs | 32 +++++++++++++++++++++++++ src/validate.rs | 62 ++++++++++++++++++++++++------------------------- 2 files changed, 62 insertions(+), 32 deletions(-) diff --git a/src/output.rs b/src/output.rs index c929042..7d2d81f 100644 --- a/src/output.rs +++ b/src/output.rs @@ -38,6 +38,11 @@ pub struct Finding { pub file: PathBuf, pub line: Option, pub message: String, + /// The exact source line the finding refers to, copied from the diff. + /// Attached after validation so the user has the cited code right next + /// to the description and never has to alt-tab to verify the citation. + #[serde(skip_serializing_if = "Option::is_none")] + pub quote: Option, } fn parse_severity_line(line: &str) -> Option { @@ -76,6 +81,7 @@ fn parse_severity_line(line: &str) -> Option { file, line: line_num, message: message.to_string(), + quote: None, }) } @@ -99,6 +105,7 @@ pub fn try_parse_finding_line(line: &str) -> Option { file: PathBuf::new(), line: None, message: if msg.is_empty() { "No issues found.".to_string() } else { msg }, + quote: None, }); } None @@ -112,18 +119,21 @@ pub fn print_finding(f: &Finding) { let location = format_location(&f.file, f.line); println!("{}{}", prefix, location.bold()); println!(" {}", f.message); + print_quote(f); } Severity::Med => { let prefix = "[~] MED ".yellow(); let location = format_location(&f.file, f.line); println!("{}{}", prefix, location); println!(" {}", f.message); + print_quote(f); } Severity::Low => { let prefix = "[i] LOW ".blue(); let location = format_location(&f.file, f.line); println!("{}{}", prefix, location); println!(" {}", f.message); + print_quote(f); } Severity::Lgtm => { println!("{} {}", "[✓] LGTM".green().bold(), f.message.green()); @@ -131,6 +141,25 @@ pub fn print_finding(f: &Finding) { } } +fn print_quote(f: &Finding) { + if let Some(quote) = &f.quote { + let trimmed = quote.trim_end(); + if trimmed.is_empty() { + return; + } + // Indent under the message and dim the source so the user's eye + // returns to the description by default. Trim very long lines. + let max = 120usize; + let display = if trimmed.chars().count() > max { + let truncated: String = trimmed.chars().take(max).collect(); + format!("{}…", truncated) + } else { + trimmed.to_string() + }; + println!(" {} {}", "│".dimmed(), display.dimmed()); + } +} + /// Print the summary line after all findings. pub fn print_summary(findings: &[Finding], elapsed: Duration, model: &str) { if findings.is_empty() { @@ -173,6 +202,8 @@ pub struct JsonFinding { pub file: String, pub line: Option, pub message: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub quote: Option, } #[derive(Serialize)] @@ -192,6 +223,7 @@ pub fn print_findings_json(findings: &[Finding]) -> Result<()> { file: f.file.display().to_string(), line: f.line, message: f.message.clone(), + quote: f.quote.clone(), }) .collect(); diff --git a/src/validate.rs b/src/validate.rs index c5841c1..b12cff9 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -15,7 +15,6 @@ //! detector logs phantom entries. use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; use crate::git::{DiffLine, ParsedDiff}; use crate::output::{Finding, Severity}; @@ -36,13 +35,12 @@ pub struct DiffIndex { struct FileLines { added: HashSet, context: HashSet, + /// Original source text for every visible line, indexed by line number. + /// Used to attach a code quote to each accepted finding so the user can + /// see the cited code without leaving the terminal. + content: HashMap, } -impl FileLines { - fn contains(&self, line: u32) -> bool { - self.added.contains(&line) || self.context.contains(&line) - } -} impl DiffIndex { pub fn from_diff(diff: &ParsedDiff) -> Self { @@ -54,12 +52,14 @@ impl DiffIndex { let mut line_num = hunk.new_start; for line in &hunk.lines { match line { - DiffLine::Added(_) => { + DiffLine::Added(text) => { entry.added.insert(line_num); + entry.content.insert(line_num, text.clone()); line_num += 1; } - DiffLine::Context(_) => { + DiffLine::Context(text) => { entry.context.insert(line_num); + entry.content.insert(line_num, text.clone()); line_num += 1; } DiffLine::Removed(_) => { @@ -72,6 +72,19 @@ impl DiffIndex { Self { files } } + /// Look up the source text for a (file, line) pair. + fn line_content(&self, file: &str, line: u32) -> Option { + self.files + .get(file) + .or_else(|| { + self.files + .iter() + .find(|(k, _)| k.ends_with(file) || file.ends_with(k.as_str())) + .map(|(_, v)| v) + }) + .and_then(|fl| fl.content.get(&line).cloned()) + } + /// Outcome of validating a single finding. pub fn validate(&self, finding: &Finding) -> Validation { // LGTM has no file/line and is always allowed. @@ -148,27 +161,23 @@ impl DiffIndex { pub fn apply(&self, mut finding: Finding) -> (Option, Validation) { let outcome = self.validate(&finding); match outcome { - Validation::Accept { on_change } => (Some(finding), Validation::Accept { on_change }), + Validation::Accept { on_change } => { + if let Some(line) = finding.line { + let key = finding.file.to_string_lossy().to_string(); + finding.quote = self.line_content(&key, line); + } + (Some(finding), Validation::Accept { on_change }) + } Validation::Reanchor { from, to, on_change } => { finding.line = Some(to); + let key = finding.file.to_string_lossy().to_string(); + finding.quote = self.line_content(&key, to); (Some(finding), Validation::Reanchor { from, to, on_change }) } Validation::Drop(reason) => (None, Validation::Drop(reason)), } } - /// Number of files indexed — used to suppress validation when we have no - /// diff to validate against (defensive guard). - pub fn is_empty(&self) -> bool { - self.files.is_empty() - } - - /// Helper for [`crate::main`] flag wiring: should a finding's location - /// be silently kept when validation can't decide? - #[allow(dead_code)] - pub fn known_files(&self) -> impl Iterator + '_ { - self.files.keys().map(PathBuf::from) - } } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -193,14 +202,3 @@ pub enum Validation { Drop(DropReason), } -impl Validation { - /// True when the finding lands on a line the change actually added. - /// Context-only findings can still be useful but should be marked, since - /// they often describe pre-existing code the diff merely touched. - pub fn is_on_change(&self) -> bool { - match self { - Validation::Accept { on_change } | Validation::Reanchor { on_change, .. } => *on_change, - Validation::Drop(_) => false, - } - } -} From 11124196be373e47fbe1d3cc48f43f707be79ec1 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 11:59:04 +0530 Subject: [PATCH 21/29] chore(prompt): drop string-form build_review_prompt_ctx MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: build_review_prompt_parts_ctx superseded it once chunking and caching landed — every call site now goes through the parts builder and concatenates only when a backend without cache support needs a plain string. The old function was a stale copy of the same logic and would drift out of sync if either path were edited. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/prompt.rs | 84 --------------------------------------------------- 1 file changed, 84 deletions(-) diff --git a/src/prompt.rs b/src/prompt.rs index 4b73a2d..050eb3d 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -182,90 +182,6 @@ pub fn build_review_prompt_parts_ctx( PromptPartsOwned { system, cacheable, dynamic } } -/// Build a prompt from a full ReviewContext (Phase 2+) with optional linter findings. -pub fn build_review_prompt_ctx( - ctx: &ReviewContext, - config: &Config, - security_mode: bool, - linter_findings: &[LinterFinding], -) -> String { - let mut prompt = String::new(); - - // 1. System instructions - prompt.push_str(if security_mode { SECURITY_INSTRUCTIONS } else { SYSTEM_INSTRUCTIONS }); - prompt.push_str("\n\n"); - - // 2. Changed code - prompt.push_str("=== CHANGED CODE ===\n"); - prompt.push_str(&format_diff(&ctx.diff)); - prompt.push('\n'); - - // 3. Called function bodies - if !ctx.called_functions.is_empty() { - prompt.push_str("=== FUNCTIONS CALLED BY CHANGED CODE ===\n"); - for f in &ctx.called_functions { - prompt.push_str(&f.full_text); - prompt.push_str("\n\n"); - } - } - - // 4. Types used - if !ctx.types_used.is_empty() { - prompt.push_str("=== TYPES USED ===\n"); - for t in &ctx.types_used { - prompt.push_str(&format!( - "{} {} {{ {} }}\n", - format!("{:?}", t.kind).to_lowercase(), - t.name, - t.fields.join(", ") - )); - } - prompt.push('\n'); - } - - // 5. Related tests - if !ctx.test_functions.is_empty() { - prompt.push_str("=== RELATED TESTS ===\n"); - for f in &ctx.test_functions { - prompt.push_str(&f.full_text); - prompt.push_str("\n\n"); - } - } - - // 6. Linter findings - if !linter_findings.is_empty() { - prompt.push_str("=== LINTER FINDINGS ===\n"); - for f in linter_findings { - prompt.push_str(&format!( - "{} at {}:{}\n", - f.code, - f.file.display(), - f.line - )); - } - prompt.push_str( - "For each linter finding above, assess: is this a genuine risk or a \ - false positive given the context? Explain the actual consequence if real.\n\n", - ); - } - - // 7. Team rules - if !config.rules.is_empty() { - prompt.push_str("=== TEAM RULES ===\n"); - prompt.push_str("Also check for these team-specific rules:\n"); - for rule in &config.rules { - prompt.push_str(&format!("- [{}]: {}\n", rule.name, rule.description)); - } - prompt.push('\n'); - } - - // 8. Output format (always last) - prompt.push_str("=== OUTPUT FORMAT ===\n"); - prompt.push_str(OUTPUT_FORMAT); - prompt.push('\n'); - - prompt -} /// Fallback: build a prompt from a raw diff only (Phase 1 behaviour). pub fn build_review_prompt(diff: &ParsedDiff, config: &Config, security_mode: bool) -> String { From 0f008d20c9ea429798727d39fa703a6225bea69d Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:21:34 +0530 Subject: [PATCH 22/29] docs: document validator, critique, chunking, prompt caching Update docs/context.md and README.md to cover the new pipeline: - module map adds validate.rs and critique.rs - data flow shows the ignore filter, linters, chunking, validator, critique, and final print steps - Anthropic prompt-caching strategy explained alongside PromptParts - README gets a "Review quality" section covering diff-aware validation, self-critique, and source quotes, plus a "Performance" section for chunking + prompt caching, and the new --no-critique flag in the options listing - env vars (CREV_ALLOW_INSECURE_BASE_URL, CREV_UPDATE_YES) and the Gemini header-auth change are noted in the relevant sections - example output shows the new quote line and dropped-finding notes - JSON example includes the quote field Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 66 +++++++++++++++++++++++++---- docs/context.md | 109 ++++++++++++++++++++++++++++++++++++------------ 2 files changed, 141 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index cdd5c57..99877de 100644 --- a/README.md +++ b/README.md @@ -13,14 +13,18 @@ context: Rich (4 types, 6 called fns, 2 tests) [!] HIGH src/payments/processor.rs:142 balance + amount can exceed i64::MAX when processing large transfers; use checked_add() and return Err on overflow + │ let total = balance + amount; [~] MED src/payments/processor.rs:98 db.execute() result is silently ignored; if the INSERT fails, the caller receives a success response while the data was never written + │ db.execute(stmt); [✓] LGTM src/utils/format.rs — change looks correct 3 findings (1 high, 1 med, 0 low) · 4.2s · qwen2.5-coder:14b +note: dropped 1 hallucinated finding(s) (line/file not in diff) +note: critique dropped 1 finding(s) as low-signal ``` --- @@ -83,6 +87,7 @@ Options: --fail-on Exit 1 if any finding at or above this severity [low|med|high] --security Security-focused review mode --no-cloud Never use a cloud LLM + --no-critique Skip the self-critique pass (faster, slightly noisier) --path Path to git repo (default: current directory) ``` @@ -114,6 +119,7 @@ crev init --ci --model gpt-4o # same, with OpenAI model + correct secret name ``` Installs two hooks: + - **pre-commit**: reviews staged changes before every commit - **pre-push**: reviews all unpushed commits only when pushing more than one — single commits are already covered by pre-commit @@ -194,6 +200,8 @@ export OPENAI_BASE_URL=https://api.groq.com crev review --model llama-3.1-70b-versatile ``` +`OPENAI_BASE_URL` is validated: must use https and must not point at loopback or private hosts. For local vLLM/Ollama proxies set `CREV_ALLOW_INSECURE_BASE_URL=1`. + ### Ollama local models Any model available in your Ollama instance works — crev picks the best one it finds automatically. Pull any coding model and crev will use it: @@ -250,7 +258,6 @@ Personal defaults (model choice, API keys) go in `~/.config/crev/config.toml` **Config lookup order:** `.reviewrc` (current dir → upward) → `~/.config/crev/config.toml` → built-in defaults. - ## Output formats ### Terminal (default) @@ -281,7 +288,8 @@ A spinner shows while the model analyzes. Each finding streams to the terminal a "severity": "High", "file": "src/payments/processor.rs", "line": 142, - "message": "balance + amount can exceed i64::MAX — use checked_add()" + "message": "balance + amount can exceed i64::MAX — use checked_add()", + "quote": "let total = balance + amount;" } ], "github_annotations": [ @@ -335,11 +343,11 @@ Then commit and push: **Triggers:** -| Event | Behaviour | -|---|---| -| PR opened | Runs automatically | -| `/crev` comment | Runs on demand, reacts with 👀 to acknowledge | -| Push to PR branch | Does not run — comment `/crev` to re-review | +| Event | Behaviour | +| ----------------- | --------------------------------------------- | +| PR opened | Runs automatically | +| `/crev` comment | Runs on demand, reacts with 👀 to acknowledge | +| Push to PR branch | Does not run — comment `/crev` to re-review | --- @@ -356,6 +364,50 @@ This surfaces systemic issues that keep slipping through code review. --- +## Review quality + +crev does more than just pipe a diff into an LLM. Every finding goes through three filters before it reaches your terminal: + +### 1. Diff-aware validation + +LLMs regularly cite line numbers that don't exist in the diff — off-by-one from header lines, completely fabricated, or on the wrong file entirely. crev indexes every `(file, line)` pair the model was shown and: + +- **Drops** findings whose line/file isn't in the diff (`note: dropped N hallucinated finding(s)`). +- **Re-anchors** findings within ±3 lines to the nearest real line, preferring lines the change *added* over surrounding context. +- **Flags** findings that landed on a context line — they describe pre-existing code the change merely touches, not the change itself. + +### 2. Self-critique pass + +After the first review, crev sends the findings back through the model with a sharper "is each one specific, grounded, and actionable?" prompt. The model emits `KEEP` or `DROP` per finding; low-signal ones are removed before you see them. Skip with `--no-critique` if you want raw output. + +### 3. Source quotes + +Every accepted finding carries the cited source line as a `quote`, rendered dimmed under the description so you can judge correctness without leaving the terminal: + +``` +[!] HIGH src/payments/processor.rs:142 + balance + amount can exceed i64::MAX — use checked_add() + │ let total = balance + amount; +``` + +--- + +## Performance + +### Chunking for large diffs + +When the rendered diff exceeds `max_tokens`, crev splits it by file and runs the review across multiple chunks rather than dropping the biggest files entirely. Findings are merged, validated, and critiqued as one set. + +### Prompt caching + +The Anthropic backend uses Anthropic's prompt-caching API to mark the system prompt and stable user-block as cacheable. The diff and context (which change per call) stay uncached. The provider returns ~90% cost discount on the cached prefix within a 5-minute window — pre-commit hooks that re-review the same change multiple times effectively pay for the diff alone after the first call. + +### Self-update + +`crev update` downloads the install script, shows the URL, and asks for confirmation before running it. Skip the prompt with `CREV_UPDATE_YES=1` for non-interactive environments. + +--- + ## License HEHEHEHEHE diff --git a/docs/context.md b/docs/context.md index 23844e2..0d4b208 100644 --- a/docs/context.md +++ b/docs/context.md @@ -8,17 +8,19 @@ Architecture reference for contributors and AI assistants working on this repo. | File | Purpose | |---|---| -| `src/main.rs` | CLI entrypoint, clap commands, review orchestration, spinner, streaming output | -| `src/llm.rs` | Trait-based LLM backend system (Ollama, Anthropic, OpenAI, Gemini) | +| `src/main.rs` | CLI entrypoint, clap commands, chunk-aware review orchestration, spinner, streaming output | +| `src/llm.rs` | Trait-based LLM backend system (Ollama, Anthropic, OpenAI, Gemini), prompt caching support via `PromptParts` | | `src/ollama.rs` | Ollama HTTP client — streaming, model detection, health check | | `src/git.rs` | git2 wrapper — staged/unstaged/commit/range diffs → `ParsedDiff` | | `src/ast.rs` | tree-sitter multi-language parser — functions, types, call graph | | `src/context.rs` | Builds `ReviewContext` from a diff: finds changed fns, resolves call defs, fits into token budget | -| `src/prompt.rs` | Assembles the final LLM prompt from `ReviewContext` + config rules | -| `src/output.rs` | Parses LLM output lines into `Finding` structs, pretty-prints with colors, JSON output | -| `src/config.rs` | Loads `.reviewrc` (repo-local) and `~/.config/crev/config.toml` (global) | +| `src/prompt.rs` | Assembles the LLM prompt; emits `PromptPartsOwned` for cache-aware backends; chunks oversize diffs | +| `src/output.rs` | Parses LLM output lines into `Finding` structs (incl. cited source `quote`), pretty-prints with colors, JSON output | +| `src/validate.rs` | Diff-aware validator: drops findings citing lines not in the diff, re-anchors near-misses, attaches source quote | +| `src/critique.rs` | Second LLM pass that filters low-signal findings (KEEP/DROP per finding); opt-out via `--no-critique` | +| `src/config.rs` | Loads `.reviewrc` (repo-local) and `~/.config/crev/config.toml` (global), guards ignore-glob patterns | | `src/history.rs` | SQLite review history — saves reviews, detects recurring patterns | -| `src/linters.rs` | Runs language linters (clippy, eslint, ruff, golangci-lint) and filters findings to diff lines | +| `src/linters.rs` | Runs language linters (clippy, eslint, ruff, golangci-lint, semgrep) and filters findings to diff lines | --- @@ -28,16 +30,23 @@ Architecture reference for contributors and AI assistants working on this repo. git diff └─▶ git.rs::get_*_diff() └─▶ ParsedDiff { files[], stats } - └─▶ context.rs::ContextBuilder::build() - ├─ ast.rs → functions_changed, called_functions, test_functions - ├─ token budget fit (priority: diff > types > called fns > tests) - └─▶ ReviewContext - └─▶ prompt.rs::build_review_prompt() - └─▶ String (prompt) - └─▶ llm.rs::resolve() → backend.complete() - └─▶ on_token callback (line buffering → mpsc channel) - └─▶ output.rs::try_parse_finding_line() - └─▶ Finding[] → print / JSON + └─▶ ignore-glob filter (config.rs) + └─▶ linters::run_linters() (once, whole diff) + └─▶ prompt::chunk_diff_by_files() + └─▶ for each chunk: + ├─ context.rs::ContextBuilder::build() → ReviewContext + ├─ prompt::build_review_prompt_parts_ctx() → PromptPartsOwned + └─ backend.complete_parts(parts) (Anthropic: cache_control on system+cacheable) + └─▶ on_token → mpsc channel → output::try_parse_finding_line() + └─▶ raw Finding[] + └─▶ validate::DiffIndex.apply() per finding + ├─ drop if line/file not in diff + ├─ re-anchor (prefer Added) if within ±3 lines + ├─ attach source quote + └─▶ kept Finding[] + └─▶ critique::run_critique() (KEEP/DROP per finding) + └─▶ surviving Finding[] + └─▶ print / JSON / history / fail_on ``` --- @@ -61,11 +70,28 @@ ReviewContext { diff, functions_changed, called_functions, types_used, test_func ContextQuality = Rich | Partial | Minimal // output.rs -Finding { severity: Severity, file, line, message } +Finding { severity: Severity, file, line, message, quote: Option } Severity = High | Med | Low | Lgtm +// validate.rs +DiffIndex { files: HashMap } +FileLines { added: HashSet, context: HashSet, content: HashMap } +Validation = Accept { on_change } | Reanchor { from, to, on_change } | Drop(DropReason) +DropReason = UnknownFile | LineNotInDiff + +// critique.rs +CritiqueResult { kept: Vec, dropped: Vec<(Finding, String)> } + // llm.rs -trait LlmBackend { complete(prompt, on_token) -> Result; name(); is_local() } +PromptParts<'a> { system: &str, cacheable: &str, dynamic: &str } +trait LlmBackend { + complete(prompt, on_token) -> Result; + complete_parts(parts, on_token) -> Result; // default impl concatenates + name(); is_local(); +} + +// prompt.rs +PromptPartsOwned { system: String, cacheable: String, dynamic: String } ``` --- @@ -82,8 +108,11 @@ Each backend implements `LlmBackend::complete()` which streams tokens via the `o **API key env vars:** - Anthropic: `ANTHROPIC_API_KEY` (or `api_key_env` in config) -- OpenAI: `OPENAI_API_KEY`, base URL override: `OPENAI_BASE_URL` -- Gemini: `GEMINI_API_KEY` or `GOOGLE_API_KEY` +- OpenAI: `OPENAI_API_KEY`, base URL override: `OPENAI_BASE_URL` (validated: https + non-private host; set `CREV_ALLOW_INSECURE_BASE_URL=1` for local proxies) +- Gemini: `GEMINI_API_KEY` or `GOOGLE_API_KEY` (sent via `x-goog-api-key` header, never in URL) + +**Prompt caching (Anthropic only):** +`AnthropicBackend::complete_parts` sets `cache_control: { type: "ephemeral" }` on the system block and the stable `cacheable` user block (team rules + output format). The `dynamic` block (diff + context + linter findings) stays uncached. 90% cost discount on the cached prefix within a 5-minute window. Anthropic-only — other backends concatenate via the default trait impl. --- @@ -104,18 +133,44 @@ Token budget priority: diff content → type defs → called function bodies → - `Partial` — some context but not rich - `Minimal` — diff only +**Safety caps:** files over 1 MiB are skipped; the walker stops after 5,000 source files (prevents pathological repos from making `build()` run for minutes). + +--- + +## Chunking (`src/prompt.rs::chunk_diff_by_files`) + +When the rendered diff exceeds `max_tokens * 4` chars, `chunk_diff_by_files` greedily packs files into chunks under budget. Files that exceed budget alone become their own chunk and the existing truncator trims their context lines further at prompt-build time. `run_review` loops over chunks, building context and prompt per chunk; linter findings come from a single whole-diff run and are filtered per chunk. + +--- + +## Validation + critique (`src/validate.rs`, `src/critique.rs`) + +After each chunk's LLM call, raw findings pass through `DiffIndex::apply`: +- Line in **Added** set → accept, mark `on_change: true` +- Line in **Context** set → accept, mark `on_change: false` (surfaces in summary as "references unchanged context lines") +- Within ±3 lines of a real line → re-anchor (prefer Added) +- Otherwise → drop (`UnknownFile` or `LineNotInDiff`) + +Accepted findings get the cited source line attached as `quote`. + +After all chunks have been validated, `critique::run_critique` runs one more LLM call asking the model to grade each kept finding as KEEP or DROP. Unparsed decisions default to KEEP so a broken critique response can't hide a real finding. Skip with `--no-critique` to recover the live-streaming UX. + --- ## Streaming output (`src/main.rs`) -Findings stream to the terminal as soon as each line arrives from the LLM: +Two modes: + +- **Live stream** (single-chunk path, `--no-critique`, non-JSON): + 1. Spinner runs on a separate tokio task (watch channel stop signal) + 2. `on_token` callback buffers tokens, sends complete lines via unbounded mpsc channel + 3. Consumer loop receives lines, calls `output::try_parse_finding_line()` + 4. On first finding: stops spinner, clears spinner line + 5. Each finding is printed immediately via `output::print_finding()` + +- **Batched** (chunking active OR critique active OR JSON): findings are collected silently across all chunks, validated, critiqued, then printed in one block. A finding that critique would later drop is never shown. -1. Spinner runs on a separate tokio task (watch channel stop signal) -2. `on_token` callback buffers tokens, sends complete lines via unbounded mpsc channel -3. Consumer loop receives lines, calls `output::try_parse_finding_line()` -4. On first finding: stops spinner, clears spinner line -5. Each finding is printed immediately via `output::print_finding()` -6. After `complete()` returns: print summary line +Run-summary footer surfaces re-anchor count, context-only count, dropped-hallucination list, and critique-dropped list so the user sees how noisy the model was. --- From e78a423858490fd86ffff9ca780ea41dc14b0da0 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:40:50 +0530 Subject: [PATCH 23/29] perf(context): build a repo index once, reuse it across chunks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: with chunking, ContextBuilder::build runs once per chunk, and each call re-walked the entire src/lib/pkg/internal/cmd tree to resolve called-function definitions and find related tests. A 5-chunk review on a moderate repo paid 5x the parsing cost for the same data. How: RepoIndex walks the repo once (bounded by the same 1MiB/file and 5,000-file caps), groups every defined function by name, and stores an indexed list with file paths. ContextBuilder::build now takes &RepoIndex and resolves called fns and related tests via HashMap lookups — no further IO. The index is built once at the top of run_review and shared by reference across every chunk's context build. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/context.rs | 340 ++++++++++++++++++++++++------------------------- src/main.rs | 8 +- 2 files changed, 176 insertions(+), 172 deletions(-) diff --git a/src/context.rs b/src/context.rs index 4737741..fa84f5a 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,10 +1,132 @@ use anyhow::Result; +use std::collections::HashMap; use std::path::{Path, PathBuf}; use crate::ast::{AstParser, FunctionInfo, TypeDef}; use crate::git::{DiffHunk, ParsedDiff}; use crate::prompt::estimate_tokens; +/// Cross-chunk repo index. Walks the source tree exactly once per review, +/// even when chunking forces multiple context builds. Without this, every +/// chunk re-parses the entire `src/`, `lib/`, … tree from scratch. +pub struct RepoIndex { + /// All defined functions in the repo, grouped by name. A name can map to + /// many functions (e.g. `new` exists on many types) — receiver-aware + /// matching in `find_called_function_defs` picks the right one. + pub functions_by_name: HashMap>, + /// Subset of `functions_by_name` flattened for test discovery. + pub all_functions: Vec, +} + +#[derive(Debug, Clone)] +pub struct IndexedFunction { + pub file: PathBuf, + pub info: FunctionInfo, +} + +impl RepoIndex { + /// Walk the repo once and index every function we can parse. Bounded by + /// the same per-file-size and total-file caps used elsewhere so a + /// pathological repo can't make indexing run forever. + pub fn build(repo_root: &Path, parser: &AstParser) -> Self { + let mut by_name: HashMap> = HashMap::new(); + let mut all: Vec = Vec::new(); + let mut walked = 0usize; + + let dirs = ["src", "lib", "pkg", "internal", "cmd", "tests", "test", "__tests__", "spec"]; + for dir_name in &dirs { + let dir = repo_root.join(dir_name); + if dir.exists() { + walk_index(&dir, parser, &mut by_name, &mut all, &mut walked); + } + } + // Shallow scan of repo root for single-file projects. + walk_index_shallow(repo_root, parser, &mut by_name, &mut all, &mut walked); + + Self { functions_by_name: by_name, all_functions: all } + } +} + +fn walk_index( + dir: &Path, + parser: &AstParser, + by_name: &mut HashMap>, + all: &mut Vec, + walked: &mut usize, +) { + if *walked >= MAX_FILES_WALKED { + return; + } + let entries = match std::fs::read_dir(dir) { + Ok(e) => e, + Err(_) => return, + }; + for entry in entries.flatten() { + if *walked >= MAX_FILES_WALKED { + return; + } + let path = entry.path(); + if path.is_dir() { + if let Some(name) = path.file_name().and_then(|n| n.to_str()) { + if SKIP_DIRS.contains(&name) { + continue; + } + } + walk_index(&path, parser, by_name, all, walked); + } else if is_source_file(&path) { + *walked += 1; + index_file(&path, parser, by_name, all); + } + } +} + +fn walk_index_shallow( + dir: &Path, + parser: &AstParser, + by_name: &mut HashMap>, + all: &mut Vec, + walked: &mut usize, +) { + let entries = match std::fs::read_dir(dir) { + Ok(e) => e, + Err(_) => return, + }; + for entry in entries.flatten() { + if *walked >= MAX_FILES_WALKED { + return; + } + let path = entry.path(); + if path.is_file() && is_source_file(&path) { + *walked += 1; + index_file(&path, parser, by_name, all); + } + } +} + +fn index_file( + path: &Path, + parser: &AstParser, + by_name: &mut HashMap>, + all: &mut Vec, +) { + let source = match read_capped(path) { + Some(s) => s, + None => return, + }; + let parsed = match parser.parse_file(path, &source) { + Ok(p) => p, + Err(_) => return, + }; + for info in parser.extract_all_functions(&parsed) { + let indexed = IndexedFunction { file: path.to_path_buf(), info }; + by_name + .entry(indexed.info.name.clone()) + .or_default() + .push(indexed.clone()); + all.push(indexed); + } +} + // ── public types ───────────────────────────────────────────────────────────── #[derive(Debug, Clone)] @@ -71,7 +193,13 @@ impl ContextBuilder { } } - pub async fn build(&self, diff: ParsedDiff) -> Result { + /// Borrow the internal parser so callers can build a [`RepoIndex`] using + /// the same instance — no extra setup cost for the tree-sitter languages. + pub fn parser(&self) -> &AstParser { + &self.parser + } + + pub async fn build(&self, diff: ParsedDiff, index: &RepoIndex) -> Result { // 1. For each changed file, parse with tree-sitter let mut functions_changed: Vec = Vec::new(); let mut all_called_names: Vec = Vec::new(); @@ -106,12 +234,12 @@ impl ContextBuilder { all_called_names.sort(); all_called_names.dedup(); - // 5. Search repo for definitions of called functions - let called_functions = self.find_called_function_defs(&all_called_names); + // 5. Look up definitions from the prebuilt repo index — no walking. + let called_functions = lookup_called_function_defs(&all_called_names, index); - // 6. Find related tests + // 6. Find related tests from the prebuilt index let changed_fn_names: Vec<&str> = functions_changed.iter().map(|f| f.name.as_str()).collect(); - let test_functions = self.find_related_tests(&changed_fn_names); + let test_functions = related_tests_from_index(&changed_fn_names, index); // 7. Fit into token budget (priority: diff > changed sigs > types > called sigs > tests) let (types_used, called_functions, test_functions) = @@ -167,171 +295,6 @@ impl ContextBuilder { .collect() } - fn find_called_function_defs(&self, names: &[String]) -> Vec { - if names.is_empty() { - return Vec::new(); - } - - let mut results = Vec::new(); - let mut walked = 0usize; - let search_dirs = ["src", "lib", "pkg", "internal", "cmd"]; - - for dir_name in &search_dirs { - let dir = self.repo_root.join(dir_name); - if dir.exists() { - self.walk_for_functions(&dir, names, &mut results, &mut walked); - } - } - - // Also check repo root itself for single-file projects - self.walk_dir_shallow(&self.repo_root, names, &mut results, &mut walked); - - results - } - - fn walk_for_functions( - &self, - dir: &Path, - names: &[String], - out: &mut Vec, - walked: &mut usize, - ) { - if *walked >= MAX_FILES_WALKED { - return; - } - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return, - }; - - for entry in entries.flatten() { - if *walked >= MAX_FILES_WALKED { - return; - } - let path = entry.path(); - - if path.is_dir() { - if let Some(name) = path.file_name().and_then(|n| n.to_str()) { - if SKIP_DIRS.contains(&name) { - continue; - } - } - self.walk_for_functions(&path, names, out, walked); - } else if is_source_file(&path) { - *walked += 1; - self.extract_matching_fns(&path, names, out); - } - } - } - - fn walk_dir_shallow( - &self, - dir: &Path, - names: &[String], - out: &mut Vec, - walked: &mut usize, - ) { - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return, - }; - for entry in entries.flatten() { - if *walked >= MAX_FILES_WALKED { - return; - } - let path = entry.path(); - if path.is_file() && is_source_file(&path) { - *walked += 1; - self.extract_matching_fns(&path, names, out); - } - } - } - - fn extract_matching_fns(&self, path: &Path, names: &[String], out: &mut Vec) { - let source = match read_capped(path) { - Some(s) => s, - None => return, - }; - let parsed = match self.parser.parse_file(path, &source) { - Ok(p) => p, - Err(_) => return, - }; - let fns = self.parser.extract_all_functions(&parsed); - for f in fns { - if names.contains(&f.name) && !out.iter().any(|e: &FunctionInfo| e.name == f.name) { - out.push(f); - } - } - } - - fn find_related_tests(&self, fn_names: &[&str]) -> Vec { - let mut tests = Vec::new(); - let mut walked = 0usize; - - let test_dirs = ["tests", "test", "__tests__", "spec"]; - for dir_name in &test_dirs { - let dir = self.repo_root.join(dir_name); - if dir.exists() { - self.walk_for_tests(&dir, fn_names, &mut tests, &mut walked); - } - } - - // Also inline tests in src (Rust's #[cfg(test)]) - let src_dir = self.repo_root.join("src"); - if src_dir.exists() { - self.walk_for_tests(&src_dir, fn_names, &mut tests, &mut walked); - } - - tests - } - - fn walk_for_tests( - &self, - dir: &Path, - fn_names: &[&str], - out: &mut Vec, - walked: &mut usize, - ) { - if *walked >= MAX_FILES_WALKED { - return; - } - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return, - }; - for entry in entries.flatten() { - if *walked >= MAX_FILES_WALKED { - return; - } - let path = entry.path(); - if path.is_dir() { - if let Some(name) = path.file_name().and_then(|n| n.to_str()) { - if !SKIP_DIRS.contains(&name) { - self.walk_for_tests(&path, fn_names, out, walked); - } - } - } else if is_source_file(&path) { - *walked += 1; - let source = match read_capped(&path) { - Some(s) => s, - None => continue, - }; - let parsed = match self.parser.parse_file(&path, &source) { - Ok(p) => p, - Err(_) => continue, - }; - let fns = self.parser.extract_all_functions(&parsed); - for f in fns { - let is_test = f.name.starts_with("test_") - || f.name.ends_with("_test") - || fn_names.iter().any(|n| f.name.contains(n)); - if is_test { - out.push(f); - } - } - } - } - } fn fit_to_budget( &self, @@ -392,3 +355,40 @@ fn is_source_file(path: &Path) -> bool { Some("rs" | "ts" | "tsx" | "js" | "jsx" | "py" | "go") ) } + +/// Resolve called-function names against the prebuilt index. Returns at most +/// one match per name (the first encountered) — receiver-aware disambiguation +/// is a follow-up, but a single definition per name is already enough to +/// ground most reviews. +fn lookup_called_function_defs(names: &[String], index: &RepoIndex) -> Vec { + let mut out: Vec = Vec::new(); + let mut seen: std::collections::HashSet<&str> = std::collections::HashSet::new(); + for name in names { + if !seen.insert(name.as_str()) { + continue; + } + if let Some(candidates) = index.functions_by_name.get(name) { + if let Some(first) = candidates.first() { + out.push(first.info.clone()); + } + } + } + out +} + +/// Pull related test functions from the index instead of walking again. A +/// function counts as a test when its name follows the `test_*` / `*_test` +/// convention or includes one of the changed function names. +fn related_tests_from_index(changed_names: &[&str], index: &RepoIndex) -> Vec { + let mut out: Vec = Vec::new(); + for f in &index.all_functions { + let name = &f.info.name; + let is_test = name.starts_with("test_") + || name.ends_with("_test") + || changed_names.iter().any(|n| name.contains(n)); + if is_test { + out.push(f.info.clone()); + } + } + out +} diff --git a/src/main.rs b/src/main.rs index dbb6151..6f9658a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -297,6 +297,9 @@ async fn run_review( // ── Linters run once on the whole diff ───────────────────────────────── let repo_root = git::find_repo_root(path)?; let ctx_builder = context::ContextBuilder::new(repo_root.clone(), cfg.review.max_tokens); + // Build the repo index exactly once and reuse it across every chunk's + // context build. Without this, a 5-chunk review re-walks the repo 5x. + let repo_index = std::sync::Arc::new(context::RepoIndex::build(&repo_root, ctx_builder.parser())); let linter_findings = linters::run_linters(&diff, &repo_root).await; if !linter_findings.is_empty() { @@ -377,8 +380,9 @@ async fn run_review( ); } - // Per-chunk context build + prompt - let ctx_result = ctx_builder.build(chunk.clone()).await; + // Per-chunk context build + prompt — index is shared so we don't + // walk the repo again for every chunk. + let ctx_result = ctx_builder.build(chunk.clone(), repo_index.as_ref()).await; let chunk_linter_findings: Vec = linter_findings .iter() .filter(|f| chunk.files.iter().any(|cf| cf.path == f.file)) From 07d77f5b0a0642fce03f2aae64502355404c3f11 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:41:55 +0530 Subject: [PATCH 24/29] fix(context): disambiguate same-named functions by caller location MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: lookup_called_function_defs returned the first definition of a name found anywhere in the repo. For ubiquitous names like new, default, or build, that's a coin flip — the model often saw a totally unrelated implementation as "the function being called", which made findings about the change harder to ground. How: every recorded call now carries the repo-relative path of the file it was called from. When a name has multiple definitions, the picker prefers same-file over same-directory over first-found. This is not real type resolution, but it kills the most common mis-attribution without paying for a full symbol table. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/context.rs | 66 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 50 insertions(+), 16 deletions(-) diff --git a/src/context.rs b/src/context.rs index fa84f5a..cd527ac 100644 --- a/src/context.rs +++ b/src/context.rs @@ -202,7 +202,7 @@ impl ContextBuilder { pub async fn build(&self, diff: ParsedDiff, index: &RepoIndex) -> Result { // 1. For each changed file, parse with tree-sitter let mut functions_changed: Vec = Vec::new(); - let mut all_called_names: Vec = Vec::new(); + let mut all_calls: Vec<(PathBuf, String)> = Vec::new(); let mut types_used: Vec = Vec::new(); for file in &diff.files { @@ -220,9 +220,13 @@ impl ContextBuilder { // 2. Find functions that overlap with diff hunks let changed_fns = self.functions_overlapping_hunks(&parsed, &file.hunks); - // 3. Collect all calls made by those functions + // 3. Collect all calls made by those functions — tagged with the + // caller's repo-relative file so we can disambiguate names that + // are defined in multiple places (e.g. `new`, `default`). for f in &changed_fns { - all_called_names.extend(f.called_functions.iter().cloned()); + for call in &f.called_functions { + all_calls.push((file.path.clone(), call.clone())); + } } functions_changed.extend(changed_fns); @@ -231,11 +235,11 @@ impl ContextBuilder { types_used.extend(file_types); } - all_called_names.sort(); - all_called_names.dedup(); + all_calls.sort(); + all_calls.dedup(); // 5. Look up definitions from the prebuilt repo index — no walking. - let called_functions = lookup_called_function_defs(&all_called_names, index); + let called_functions = lookup_called_function_defs(&all_calls, index); // 6. Find related tests from the prebuilt index let changed_fn_names: Vec<&str> = functions_changed.iter().map(|f| f.name.as_str()).collect(); @@ -356,26 +360,56 @@ fn is_source_file(path: &Path) -> bool { ) } -/// Resolve called-function names against the prebuilt index. Returns at most -/// one match per name (the first encountered) — receiver-aware disambiguation -/// is a follow-up, but a single definition per name is already enough to -/// ground most reviews. -fn lookup_called_function_defs(names: &[String], index: &RepoIndex) -> Vec { +/// Resolve called-function names against the prebuilt index. When multiple +/// definitions share a name (`new`, `default`, `build`), prefer one defined +/// in the same file as the call site, then any in the same directory, then +/// fall back to the first match. This avoids the worst of the +/// name-collision problem without doing real type resolution. +fn lookup_called_function_defs( + calls: &[(PathBuf, String)], + index: &RepoIndex, +) -> Vec { let mut out: Vec = Vec::new(); let mut seen: std::collections::HashSet<&str> = std::collections::HashSet::new(); - for name in names { + for (caller_file, name) in calls { if !seen.insert(name.as_str()) { continue; } - if let Some(candidates) = index.functions_by_name.get(name) { - if let Some(first) = candidates.first() { - out.push(first.info.clone()); - } + let Some(candidates) = index.functions_by_name.get(name) else { + continue; + }; + let pick = pick_best_candidate(candidates, caller_file); + if let Some(p) = pick { + out.push(p.info.clone()); } } out } +fn pick_best_candidate<'a>( + candidates: &'a [IndexedFunction], + caller_file: &Path, +) -> Option<&'a IndexedFunction> { + if candidates.len() == 1 { + return candidates.first(); + } + // Same file wins. + if let Some(same_file) = candidates.iter().find(|c| c.file.ends_with(caller_file)) { + return Some(same_file); + } + // Then same directory. + let caller_dir = caller_file.parent(); + if let Some(dir) = caller_dir { + if let Some(same_dir) = candidates + .iter() + .find(|c| c.file.parent().is_some_and(|p| p.ends_with(dir))) + { + return Some(same_dir); + } + } + candidates.first() +} + /// Pull related test functions from the index instead of walking again. A /// function counts as a test when its name follows the `test_*` / `*_test` /// convention or includes one of the changed function names. From 0c508a6114e5a319fdf1ddc0a4ad0d49a9551956 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:43:27 +0530 Subject: [PATCH 25/29] perf(context): distill called-function bodies before prompting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: every called function was shipped to the model as its full body, even if it was 200 lines long. For most review questions the signature and the first ~12 lines plus the return are enough to answer "what does this called function do?" — the rest just burns budget and pushes the diff further down the prompt where the model attends less. How: ast::distill_function keeps short bodies as-is and replaces the middle of long ones with a "// ... N lines omitted ..." marker. The cost estimator in fit_to_budget uses the same distilled form so the budget pass agrees with what the prompt actually sends. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/ast.rs | 25 +++++++++++++++++++++++++ src/context.rs | 7 ++++--- src/prompt.rs | 3 ++- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/ast.rs b/src/ast.rs index c7352e2..e549df9 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -45,6 +45,31 @@ pub enum TypeKind { Class, } +/// Maximum lines we'll ship for a single called-function body before +/// distilling. Most "what does this called function do?" questions are +/// answered by its first ~12 lines and its return statement; shipping a +/// 200-line body burns context budget for negligible signal gain. +pub const MAX_CALLED_FN_LINES: usize = 15; + +/// Render a function for inclusion in the prompt: full body if short, or +/// a head + tail slice with an "N lines omitted" marker otherwise. +pub fn distill_function(info: &FunctionInfo, max_lines: usize) -> String { + let lines: Vec<&str> = info.full_text.lines().collect(); + if lines.len() <= max_lines { + return info.full_text.clone(); + } + let head_take = max_lines.saturating_sub(2).max(1); + let head: Vec<&str> = lines.iter().take(head_take).copied().collect(); + let tail = lines.last().copied().unwrap_or("}"); + let omitted = lines.len().saturating_sub(head_take + 1); + format!( + "{}\n // ... {} lines omitted ...\n{}", + head.join("\n"), + omitted, + tail + ) +} + // ── parser ─────────────────────────────────────────────────────────────────── pub struct AstParser { diff --git a/src/context.rs b/src/context.rs index cd527ac..c989896 100644 --- a/src/context.rs +++ b/src/context.rs @@ -2,7 +2,7 @@ use anyhow::Result; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use crate::ast::{AstParser, FunctionInfo, TypeDef}; +use crate::ast::{distill_function, AstParser, FunctionInfo, TypeDef, MAX_CALLED_FN_LINES}; use crate::git::{DiffHunk, ParsedDiff}; use crate::prompt::estimate_tokens; @@ -329,10 +329,11 @@ impl ContextBuilder { } } - // Called functions (full body) + // Called functions — estimate using the distilled form, since + // that's what the prompt will actually ship. for f in called { if f.full_text.is_empty() { continue; } - let cost = estimate_tokens(&f.full_text); + let cost = estimate_tokens(&distill_function(f, MAX_CALLED_FN_LINES)); if used.saturating_add(cost) <= budget { used = used.saturating_add(cost); kept_called.push(f.clone()); diff --git a/src/prompt.rs b/src/prompt.rs index 050eb3d..d9f34be 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -1,3 +1,4 @@ +use crate::ast::{distill_function, MAX_CALLED_FN_LINES}; use crate::config::Config; use crate::context::ReviewContext; use crate::git::{DiffHunk, DiffLine, ParsedDiff}; @@ -137,7 +138,7 @@ pub fn build_review_prompt_parts_ctx( if !ctx.called_functions.is_empty() { dynamic.push_str("=== FUNCTIONS CALLED BY CHANGED CODE ===\n"); for f in &ctx.called_functions { - dynamic.push_str(&f.full_text); + dynamic.push_str(&distill_function(f, MAX_CALLED_FN_LINES)); dynamic.push_str("\n\n"); } } From b915d1483e1f5937bbaf5d3bf95cff0d517ec576 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:44:11 +0530 Subject: [PATCH 26/29] perf(prompt): filter type fields to those referenced in the diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: types_used was rendered with every field of every struct, even when the diff only touched two of them. For type-heavy codebases this dominated the dynamic block — a 30-field DTO ate hundreds of tokens to tell the reviewer about fields it would never look at. How: collect the set of identifier-like tokens from the diff text, then keep only fields whose names appear in that set. Fully unused types collapse to `name { … }` so the model still knows the type exists; partially-used types render `kept, kept, /* +N more */` to signal there's more without listing it. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/prompt.rs | 76 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 68 insertions(+), 8 deletions(-) diff --git a/src/prompt.rs b/src/prompt.rs index d9f34be..9595a3a 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -144,16 +144,19 @@ pub fn build_review_prompt_parts_ctx( } if !ctx.types_used.is_empty() { - dynamic.push_str("=== TYPES USED ===\n"); + let diff_idents = collect_diff_identifiers(&ctx.diff); + let mut rendered_types = String::new(); for t in &ctx.types_used { - dynamic.push_str(&format!( - "{} {} {{ {} }}\n", - format!("{:?}", t.kind).to_lowercase(), - t.name, - t.fields.join(", ") - )); + if let Some(line) = render_type(t, &diff_idents) { + rendered_types.push_str(&line); + rendered_types.push('\n'); + } + } + if !rendered_types.is_empty() { + dynamic.push_str("=== TYPES USED ===\n"); + dynamic.push_str(&rendered_types); + dynamic.push('\n'); } - dynamic.push('\n'); } if !ctx.test_functions.is_empty() { @@ -281,6 +284,63 @@ pub fn estimate_tokens(text: &str) -> usize { text.len() / 4 } +/// Render a single type definition, keeping only the fields whose names +/// appear somewhere in the diff text. A struct with 30 fields where the +/// diff only touches 2 of them becomes a 2-field rendering — same signal +/// for the reviewer at a tenth of the tokens. If no fields match we keep +/// just the type name with `…`, which still tells the model the type +/// existed without spending budget on every field. +fn render_type(t: &crate::ast::TypeDef, diff_idents: &std::collections::HashSet) -> Option { + let kind = format!("{:?}", t.kind).to_lowercase(); + if t.fields.is_empty() { + // Traits, type aliases, enums-with-no-payload — keep the name. + return Some(format!("{} {} {{ }}", kind, t.name)); + } + let kept: Vec<&str> = t + .fields + .iter() + .filter(|f| diff_idents.contains(f.as_str())) + .map(|f| f.as_str()) + .collect(); + if kept.is_empty() { + return Some(format!("{} {} {{ … }}", kind, t.name)); + } + let omitted = t.fields.len() - kept.len(); + let mut body = kept.join(", "); + if omitted > 0 { + body.push_str(&format!(", /* +{} more */", omitted)); + } + Some(format!("{} {} {{ {} }}", kind, t.name, body)) +} + +/// Extract identifier-like tokens from every line of the diff. Used to +/// filter type-field renderings: a field the diff doesn't reference is +/// almost never relevant to reviewing the diff. +fn collect_diff_identifiers(diff: &ParsedDiff) -> std::collections::HashSet { + let mut out: std::collections::HashSet = std::collections::HashSet::new(); + for file in &diff.files { + for hunk in &file.hunks { + for line in &hunk.lines { + let text = match line { + DiffLine::Added(s) | DiffLine::Removed(s) | DiffLine::Context(s) => s.as_str(), + }; + let mut current = String::new(); + for ch in text.chars() { + if ch.is_alphanumeric() || ch == '_' { + current.push(ch); + } else if !current.is_empty() { + out.insert(std::mem::take(&mut current)); + } + } + if !current.is_empty() { + out.insert(current); + } + } + } + } + out +} + /// Split a diff into chunks that each fit inside `max_tokens` worth of /// rendered output. Files larger than the budget on their own end up alone /// in a chunk; the truncator is the last line of defense for those. From dea434a8b0765985ea65d9b402a810e1bb491e5b Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:45:05 +0530 Subject: [PATCH 27/29] perf(context): tiered token budget for types / called fns / tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: the old fit_to_budget filled greedily: types first, then called fns, then tests, with no per-category cap. On a diff that touched many types the budget was exhausted before any called function bodies were included, leaving the model without the single highest-signal piece of context. The reverse failure was also possible — many called fns starved out the type info. How: split the post-diff budget into 25/60/15 caps for types / called fns / tests. Each tier first spends from its own cap; whatever it doesn't use stays in a shared pool that the next tier can draw from. No category can starve the others, and underused tiers don't waste budget. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/context.rs | 98 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 34 deletions(-) diff --git a/src/context.rs b/src/context.rs index c989896..be41eb3 100644 --- a/src/context.rs +++ b/src/context.rs @@ -300,6 +300,10 @@ impl ContextBuilder { } + /// Fit context into the token budget using a tiered allocation: + /// types 25% · called fns 60% · tests 15% of the budget left over after + /// the diff. Each tier overflows into the remaining global pool only + /// after its own cap is reached, so no category can starve the others. fn fit_to_budget( &self, diff: &ParsedDiff, @@ -309,47 +313,43 @@ impl ContextBuilder { ) -> (Vec, Vec, Vec) { use crate::git::DiffLine; - // Estimate diff tokens from actual line content (same as it appears in the prompt) + // Estimate diff tokens from the actual rendered line content. let diff_chars: usize = diff.files.iter().flat_map(|f| f.hunks.iter()).flat_map(|h| h.lines.iter()).map(|l| match l { DiffLine::Added(s) | DiffLine::Removed(s) | DiffLine::Context(s) => s.len() + 8, }).sum(); - let mut used = diff_chars / 4; // ~4 chars per token - let budget = self.max_tokens; - - let mut kept_types = Vec::new(); - let mut kept_called = Vec::new(); - let mut kept_tests = Vec::new(); - - // Types: estimate from field list - for t in types { - let cost = estimate_tokens(&t.fields.join(", ")) + estimate_tokens(&t.name) + 4; - if used.saturating_add(cost) <= budget { - used = used.saturating_add(cost); - kept_types.push(t.clone()); + let diff_tokens = diff_chars / 4; + let total_budget = self.max_tokens; + let context_budget = total_budget.saturating_sub(diff_tokens); + + // Per-tier caps. Numerator/denominator written explicitly so the + // allocation is easy to read and to tune. + let types_cap = context_budget * 25 / 100; + let called_cap = context_budget * 60 / 100; + let tests_cap = context_budget * 15 / 100; + + // Each tier first spends from its own cap; whatever it leaves behind + // gets recycled into the global pool the next tier can also draw on. + let mut global_pool = context_budget; + + let (kept_types, types_spent) = fill_tier(types, types_cap, &mut global_pool, |t| { + estimate_tokens(&t.fields.join(", ")) + estimate_tokens(&t.name) + 4 + }); + + let (kept_called, called_spent) = fill_tier(called, called_cap, &mut global_pool, |f| { + if f.full_text.is_empty() { + return 0; } - } - - // Called functions — estimate using the distilled form, since - // that's what the prompt will actually ship. - for f in called { - if f.full_text.is_empty() { continue; } - let cost = estimate_tokens(&distill_function(f, MAX_CALLED_FN_LINES)); - if used.saturating_add(cost) <= budget { - used = used.saturating_add(cost); - kept_called.push(f.clone()); - } - } + estimate_tokens(&distill_function(f, MAX_CALLED_FN_LINES)) + }); - // Tests (full body) - for f in tests { - if f.full_text.is_empty() { continue; } - let cost = estimate_tokens(&f.full_text); - if used.saturating_add(cost) <= budget { - used = used.saturating_add(cost); - kept_tests.push(f.clone()); + let (kept_tests, tests_spent) = fill_tier(tests, tests_cap, &mut global_pool, |f| { + if f.full_text.is_empty() { + return 0; } - } + estimate_tokens(&f.full_text) + }); + let _ = (types_spent, called_spent, tests_spent); (kept_types, kept_called, kept_tests) } } @@ -361,6 +361,36 @@ fn is_source_file(path: &Path) -> bool { ) } +/// Greedy-pack `items` into a per-tier cap, drawing from `pool` (the shared +/// remaining budget) only after the tier's own cap is exhausted. Returns the +/// kept items and total cost spent. `pool` is decremented in place so the +/// next tier can see how much slack is left. +fn fill_tier( + items: &[T], + tier_cap: usize, + pool: &mut usize, + cost_of: impl Fn(&T) -> usize, +) -> (Vec, usize) { + let mut kept = Vec::new(); + let mut tier_spent = 0usize; + for item in items { + let cost = cost_of(item); + if cost == 0 { + continue; + } + if tier_spent + cost <= tier_cap { + tier_spent += cost; + *pool = pool.saturating_sub(cost); + kept.push(item.clone()); + } else if *pool >= cost { + // Tier cap hit — keep drawing from the shared remainder if any. + *pool -= cost; + kept.push(item.clone()); + } + } + (kept, tier_spent) +} + /// Resolve called-function names against the prebuilt index. When multiple /// definitions share a name (`new`, `default`, `build`), prefer one defined /// in the same file as the call site, then any in the same directory, then From 9b4827380105441b64107ef79d3c7f1e3550cac8 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:45:58 +0530 Subject: [PATCH 28/29] perf(prompt): tighten token estimate to ~3.5 chars/token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: estimate_tokens used `len/4`, but OpenAI/Anthropic tokenizers sit closer to 3.3-3.5 chars/token on source code. The under- estimate let the prompt builder pack ~14% more context than the model actually accepted, occasionally tripping the API's context limit and getting the response truncated. How: switch to `chars * 2 / 7` (≈3.5). Add a helper for callers that already have a char count so context.rs doesn't have to fake a string just to share the formula. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/context.rs | 8 +++++--- src/prompt.rs | 14 +++++++++++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/context.rs b/src/context.rs index be41eb3..c5b97a7 100644 --- a/src/context.rs +++ b/src/context.rs @@ -4,7 +4,7 @@ use std::path::{Path, PathBuf}; use crate::ast::{distill_function, AstParser, FunctionInfo, TypeDef, MAX_CALLED_FN_LINES}; use crate::git::{DiffHunk, ParsedDiff}; -use crate::prompt::estimate_tokens; +use crate::prompt::{estimate_tokens, estimate_tokens_from_chars}; /// Cross-chunk repo index. Walks the source tree exactly once per review, /// even when chunking forces multiple context builds. Without this, every @@ -313,11 +313,13 @@ impl ContextBuilder { ) -> (Vec, Vec, Vec) { use crate::git::DiffLine; - // Estimate diff tokens from the actual rendered line content. + // Estimate diff tokens from the actual rendered line content; pass + // through `estimate_tokens` so the conversion ratio stays in sync + // with prompt.rs. let diff_chars: usize = diff.files.iter().flat_map(|f| f.hunks.iter()).flat_map(|h| h.lines.iter()).map(|l| match l { DiffLine::Added(s) | DiffLine::Removed(s) | DiffLine::Context(s) => s.len() + 8, }).sum(); - let diff_tokens = diff_chars / 4; + let diff_tokens = estimate_tokens_from_chars(diff_chars); let total_budget = self.max_tokens; let context_budget = total_budget.saturating_sub(diff_tokens); diff --git a/src/prompt.rs b/src/prompt.rs index 9595a3a..eeb8d25 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -280,8 +280,20 @@ fn format_hunk(hunk: &DiffHunk) -> String { out } +/// Rough char-to-token conversion. The OpenAI / Anthropic tokenizers cluster +/// around 3.3-3.5 chars/token on source code (denser than prose), so the old +/// `len/4` rule consistently *under*-estimated. Using `*2/7` (~3.5) gives us +/// a small safety margin so the prompt rarely overshoots the API's context +/// limit and gets truncated mid-response. pub fn estimate_tokens(text: &str) -> usize { - text.len() / 4 + estimate_tokens_from_chars(text.len()) +} + +/// Same ratio as [`estimate_tokens`] but takes a precomputed char count for +/// callers that already know the byte length and don't want to materialize a +/// string to pass through the API. +pub fn estimate_tokens_from_chars(chars: usize) -> usize { + chars.saturating_mul(2) / 7 } /// Render a single type definition, keeping only the fields whose names From 1702f8e8647e0c0f70a72694a1915409c35d2524 Mon Sep 17 00:00:00 2001 From: Saurabh Date: Tue, 12 May 2026 14:47:55 +0530 Subject: [PATCH 29/29] fix: spinner Drop guard and O(1) suffix lookup in validator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two findings from the PR's own crev review of itself: 1. Spinner task could outlive its parent on the error path. When backend.complete*()? returned an Err, run_review unwound via ? without stopping the spinner — the JoinHandle was dropped without abort and the task kept printing frames until process exit. Wrap spinner state in a Spinner struct whose Drop calls abort() so any early return reaps it. 2. validate::DiffIndex::line_content fell back to a linear scan over every file when the LLM's path didn't match exactly. For diffs with many files this was O(n) per finding. Add a basename → key index so the common "model stripped or added a leading directory" case resolves in O(1); keep the linear suffix scan only for deeper mismatches. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/main.rs | 99 ++++++++++++++++++++++++++++++++++--------------- src/validate.rs | 64 ++++++++++++++++++++------------ 2 files changed, 110 insertions(+), 53 deletions(-) diff --git a/src/main.rs b/src/main.rs index 6f9658a..5c62810 100644 --- a/src/main.rs +++ b/src/main.rs @@ -339,28 +339,10 @@ async fn run_review( let stream_live = !json && no_critique && !is_chunked; // ── Spinner ────────────────────────────────────────────────────────────── - let (stop_tx, mut stop_rx) = tokio::sync::watch::channel(false); - let spinner_task = tokio::spawn(async move { - let frames = ["⠋","⠙","⠹","⠸","⠼","⠴","⠦","⠧","⠇","⠏"]; - let mut i = 0usize; - loop { - tokio::select! { - _ = tokio::time::sleep(tokio::time::Duration::from_millis(80)) => { - use std::io::Write; - eprint!("\r{} analyzing...", frames[i % frames.len()]); - std::io::stderr().flush().ok(); - i += 1; - } - _ = stop_rx.changed() => { - use std::io::Write; - eprint!("\r\x1b[K"); - std::io::stderr().flush().ok(); - break; - } - } - } - }); - let mut spinner_task = Some(spinner_task); + // Wrapped in a Spinner struct so any early return via `?` aborts the + // background task instead of leaving it running until process exit. The + // happy path still calls `stop()` to clear the spinner line cleanly. + let mut spinner = Spinner::start(); let start = Instant::now(); let validator = validate::DiffIndex::from_diff(&diff); @@ -438,10 +420,7 @@ async fn run_review( while let Some(line) = line_rx.recv().await { if let Some(raw) = output::try_parse_finding_line(&line) { if stream_live { - if let Some(task) = spinner_task.take() { - stop_tx.send(true).ok(); - task.await.ok(); - } + spinner.stop().await; } let (kept, outcome) = validator.apply(raw.clone()); @@ -491,10 +470,7 @@ async fn run_review( } // Stop spinner now that all LLM calls are done. - if let Some(task) = spinner_task.take() { - stop_tx.send(true).ok(); - task.await.ok(); - } + spinner.stop().await; // For the buffered (critique-enabled) path, print the surviving findings now. if !stream_live && !json { @@ -698,6 +674,69 @@ fn find_git_root(start: &std::path::Path) -> Result { git::find_repo_root(start) } +/// Owns the spinner's tokio task and stop channel so it always shuts down, +/// including on `?` early returns: Drop aborts the task. The happy path +/// should call `stop().await` to clear the spinner line cleanly before +/// printing real output. +struct Spinner { + stop_tx: Option>, + task: Option>, +} + +impl Spinner { + fn start() -> Self { + let (stop_tx, mut stop_rx) = tokio::sync::watch::channel(false); + let task = tokio::spawn(async move { + let frames = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; + let mut i = 0usize; + loop { + tokio::select! { + _ = tokio::time::sleep(tokio::time::Duration::from_millis(80)) => { + use std::io::Write; + eprint!("\r{} analyzing...", frames[i % frames.len()]); + std::io::stderr().flush().ok(); + i += 1; + } + _ = stop_rx.changed() => { + use std::io::Write; + eprint!("\r\x1b[K"); + std::io::stderr().flush().ok(); + break; + } + } + } + }); + Self { + stop_tx: Some(stop_tx), + task: Some(task), + } + } + + /// Cleanly stop the spinner: send the stop signal and await the task so + /// the spinner line is cleared before subsequent prints land. + async fn stop(&mut self) { + if let Some(tx) = self.stop_tx.take() { + tx.send(true).ok(); + } + if let Some(task) = self.task.take() { + task.await.ok(); + } + } +} + +impl Drop for Spinner { + fn drop(&mut self) { + // Best-effort cleanup on `?` paths or panics. Abort the task instead + // of awaiting it, since Drop is sync. + if let Some(tx) = self.stop_tx.take() { + tx.send(true).ok(); + } + if let Some(task) = self.task.take() { + task.abort(); + } + } +} + const INSTALL_SCRIPT_URL: &str = "https://raw.githubusercontent.com/starc007/crev/main/install.sh"; diff --git a/src/validate.rs b/src/validate.rs index b12cff9..3b3e052 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -29,6 +29,9 @@ pub struct DiffIndex { /// We track Added and Context separately so re-anchoring can prefer real /// changes (Added) over surrounding code (Context). files: HashMap, + /// Basename → full key map, used to short-circuit the linear suffix-match + /// fallback when the model strips or adds a leading directory. + basename_index: HashMap, } #[derive(Default)] @@ -45,8 +48,20 @@ struct FileLines { impl DiffIndex { pub fn from_diff(diff: &ParsedDiff) -> Self { let mut files: HashMap = HashMap::new(); + let mut basename_index: HashMap = HashMap::new(); for file in &diff.files { let key = file.path.to_string_lossy().into_owned(); + if let Some(basename) = file + .path + .file_name() + .and_then(|n| n.to_str()) + { + // First entry wins on collision — rare, and the linear-suffix + // fallback below still handles the duplicate-basename case. + basename_index + .entry(basename.to_string()) + .or_insert_with(|| key.clone()); + } let entry = files.entry(key).or_default(); for hunk in &file.hunks { let mut line_num = hunk.new_start; @@ -69,20 +84,35 @@ impl DiffIndex { } } } - Self { files } + Self { files, basename_index } + } + + /// Resolve a finding's file path to a stored `FileLines` entry. Tries the + /// exact key first (zero-cost), then the basename map (O(1)), and only + /// falls back to a linear suffix scan when both miss. + fn lookup_file(&self, file: &str) -> Option<&FileLines> { + if let Some(fl) = self.files.get(file) { + return Some(fl); + } + if let Some(basename) = std::path::Path::new(file) + .file_name() + .and_then(|n| n.to_str()) + { + if let Some(real_key) = self.basename_index.get(basename) { + if let Some(fl) = self.files.get(real_key) { + return Some(fl); + } + } + } + self.files + .iter() + .find(|(k, _)| k.ends_with(file) || file.ends_with(k.as_str())) + .map(|(_, v)| v) } /// Look up the source text for a (file, line) pair. fn line_content(&self, file: &str, line: u32) -> Option { - self.files - .get(file) - .or_else(|| { - self.files - .iter() - .find(|(k, _)| k.ends_with(file) || file.ends_with(k.as_str())) - .map(|(_, v)| v) - }) - .and_then(|fl| fl.content.get(&line).cloned()) + self.lookup_file(file).and_then(|fl| fl.content.get(&line).cloned()) } /// Outcome of validating a single finding. @@ -102,19 +132,7 @@ impl DiffIndex { }; let file_key = finding.file.to_string_lossy(); - let matches: Option<&FileLines> = self - .files - .get(file_key.as_ref()) - // Fall back to a suffix match — the model sometimes prepends or - // strips a leading directory we already showed it. - .or_else(|| { - self.files - .iter() - .find(|(k, _)| { - k.ends_with(file_key.as_ref()) || file_key.ends_with(k.as_str()) - }) - .map(|(_, v)| v) - }); + let matches: Option<&FileLines> = self.lookup_file(file_key.as_ref()); let Some(lines) = matches else { return Validation::Drop(DropReason::UnknownFile);