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. --- 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/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) { diff --git a/src/context.rs b/src/context.rs index 4e0b25c..c5b97a7 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,9 +1,131 @@ 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; +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 +/// 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 ───────────────────────────────────────────────────────────── @@ -44,6 +166,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 { @@ -53,17 +193,23 @@ 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(); + let mut all_calls: Vec<(PathBuf, String)> = Vec::new(); let mut types_used: Vec = Vec::new(); 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) { @@ -74,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); @@ -85,15 +235,15 @@ impl ContextBuilder { types_used.extend(file_types); } - all_called_names.sort(); - all_called_names.dedup(); + all_calls.sort(); + all_calls.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_calls, 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) = @@ -149,134 +299,11 @@ 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 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); - } - } - - // Also check repo root itself for single-file projects - self.walk_dir_shallow(&self.repo_root, names, &mut results); - - results - } - - fn walk_for_functions(&self, dir: &Path, names: &[String], out: &mut Vec) { - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return, - }; - - for entry in entries.flatten() { - 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); - } else if is_source_file(&path) { - self.extract_matching_fns(&path, names, out); - } - } - } - - fn walk_dir_shallow(&self, dir: &Path, names: &[String], out: &mut Vec) { - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return, - }; - for entry in entries.flatten() { - let path = entry.path(); - if path.is_file() && is_source_file(&path) { - 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 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 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); - } - } - - // 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); - } - - tests - } - - fn walk_for_tests(&self, dir: &Path, fn_names: &[&str], out: &mut Vec) { - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return, - }; - for entry in entries.flatten() { - 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); - } - } - } else if is_source_file(&path) { - let source = match std::fs::read_to_string(&path) { - Ok(s) => s, - Err(_) => 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); - } - } - } - } - } + /// 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, @@ -286,46 +313,45 @@ 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; 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 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 = estimate_tokens_from_chars(diff_chars); + 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; } - } + estimate_tokens(&distill_function(f, MAX_CALLED_FN_LINES)) + }); - // Called functions (full body) - for f in called { - 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_called.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; } - } - - // 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()); - } - } + estimate_tokens(&f.full_text) + }); + let _ = (types_spent, called_spent, tests_spent); (kept_types, kept_called, kept_tests) } } @@ -336,3 +362,100 @@ fn is_source_file(path: &Path) -> bool { Some("rs" | "ts" | "tsx" | "js" | "jsx" | "py" | "go") ) } + +/// 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 +/// 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 (caller_file, name) in calls { + if !seen.insert(name.as_str()) { + continue; + } + 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. +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/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/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() + ); } } } 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 } 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 diff --git a/src/llm.rs b/src/llm.rs index d7fc03e..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,20 +300,19 @@ 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")?; 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); } @@ -228,9 +344,6 @@ impl LlmBackend for AnthropicBackend { } Ok(full) } - - fn name(&self) -> &str { "Anthropic" } - fn is_local(&self) -> bool { false } } // ── OpenAI ──────────────────────────────────────────────────────────────────── @@ -245,12 +358,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, @@ -300,7 +471,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); } @@ -392,12 +566,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 }], @@ -409,7 +585,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/main.rs b/src/main.rs index 12b84a1..5c62810 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; @@ -8,8 +9,9 @@ mod llm; mod ollama; mod output; mod prompt; +mod validate; -use anyhow::Result; +use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; use std::path::PathBuf; use std::sync::{Arc, Mutex}; @@ -42,10 +44,6 @@ enum Commands { #[arg(long)] commits: Option, - /// Review a specific file - #[arg(long)] - file: Option, - /// Output findings as JSON #[arg(long)] json: bool, @@ -66,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, @@ -156,16 +158,16 @@ async fn main() -> Result<()> { unstaged, commit, commits, - file: _file, json, fail_on, 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 { @@ -227,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); @@ -291,14 +294,13 @@ 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), - ); + // 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() { let by_tool: std::collections::HashMap<&str, usize> = @@ -310,14 +312,6 @@ 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), - Err(e) => { - eprintln!("context: Minimal (fallback to diff-only: {})", e); - 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 { @@ -329,78 +323,160 @@ 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 { - 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; - } - } + // 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); + 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 last_prompt_text: String = String::new(); + + for (chunk_idx, chunk) in chunks.iter().enumerate() { + if is_chunked { + eprintln!( + "\rreviewing chunk {}/{} ({} file(s))", + chunk_idx + 1, + chunks.len(), + chunk.files.len() + ); } - }); - // ── 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(); + // 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)) + .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 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?, + }; - let start = Instant::now(); - let full_response = backend.complete(&prompt_text, &(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); + // Flush trailing content + { + let buf = line_buf.lock().unwrap(); + if !buf.trim().is_empty() { + let _ = line_tx.send(buf.trim().to_string()); } } - })).await?; + drop(line_tx); - // 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()); + // 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 { + spinner.stop().await; + } + + 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)); + } + } + } + } } } - drop(line_tx); // close channel so receiver loop exits - // ── Consume lines: stop spinner then print each finding ─────────────────── - let mut findings: Vec = Vec::new(); - 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(task) = spinner_task.take() { - stop_tx.send(true).ok(); - task.await.ok(); + // Run critique pass before the spinner is dismissed so the user sees a + // 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(), &last_prompt_text).await { + Ok(result) => { + findings = result.kept; + critique_dropped = result.dropped; } - if !json { - output::print_finding(&f); + Err(e) => { + eprintln!("warning: critique pass failed, keeping all findings: {}", e); } - findings.push(f); } } - // Stop spinner if model returned nothing parseable - if let Some(task) = spinner_task.take() { - stop_tx.send(true).ok(); - task.await.ok(); + // Stop spinner now that all LLM calls are done. + spinner.stop().await; + + // 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(); @@ -409,6 +485,48 @@ 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 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)", + 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() + ); + } + } + 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 @@ -556,16 +674,128 @@ 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"; + 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(()) } 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); } diff --git a/src/output.rs b/src/output.rs index 344b49d..7d2d81f 100644 --- a/src/output.rs +++ b/src/output.rs @@ -38,44 +38,11 @@ pub struct Finding { pub file: PathBuf, pub line: Option, 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 + /// 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 { @@ -114,6 +81,7 @@ fn parse_severity_line(line: &str) -> Option { file, line: line_num, message: message.to_string(), + quote: None, }) } @@ -137,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 @@ -150,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()); @@ -169,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() { @@ -189,58 +180,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(); @@ -263,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)] @@ -282,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/prompt.rs b/src/prompt.rs index 4b5c444..eeb8d25 100644 --- a/src/prompt.rs +++ b/src/prompt.rs @@ -1,17 +1,60 @@ -use crate::ast::FunctionInfo; +use crate::ast::{distill_function, MAX_CALLED_FN_LINES}; 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. +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 \ @@ -29,7 +72,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. @@ -51,95 +99,116 @@ 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 prompt from a full ReviewContext (Phase 2+) with optional linter findings. -pub fn build_review_prompt_ctx( +/// 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], -) -> String { - let mut prompt = String::new(); +) -> PromptPartsOwned { + let system = if security_mode { SECURITY_INSTRUCTIONS } else { SYSTEM_INSTRUCTIONS }.to_string(); - // 1. System instructions - prompt.push_str(if security_mode { SECURITY_INSTRUCTIONS } else { SYSTEM_INSTRUCTIONS }); - prompt.push_str("\n\n"); + 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'); - // 2. Changed code - prompt.push_str("=== CHANGED CODE ===\n"); - prompt.push_str(&format_diff(&ctx.diff)); - prompt.push('\n'); + let mut dynamic = String::new(); + dynamic.push_str("=== CHANGED CODE ===\n"); + dynamic.push_str(&format_diff(&ctx.diff)); + dynamic.push('\n'); - // 3. Called function bodies if !ctx.called_functions.is_empty() { - prompt.push_str("=== FUNCTIONS CALLED BY CHANGED CODE ===\n"); + dynamic.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"); + dynamic.push_str(&distill_function(f, MAX_CALLED_FN_LINES)); + dynamic.push_str("\n\n"); } } - // 4. Types used if !ctx.types_used.is_empty() { - prompt.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 { - prompt.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'); } - prompt.push('\n'); } - // 5. Related tests if !ctx.test_functions.is_empty() { - prompt.push_str("=== RELATED TESTS ===\n"); + dynamic.push_str("=== RELATED TESTS ===\n"); for f in &ctx.test_functions { - prompt.push_str(&f.full_text); - prompt.push_str("\n\n"); + dynamic.push_str(&f.full_text); + dynamic.push_str("\n\n"); } } - // 6. Linter findings if !linter_findings.is_empty() { - prompt.push_str("=== LINTER FINDINGS ===\n"); + dynamic.push_str("=== LINTER FINDINGS ===\n"); for f in linter_findings { - prompt.push_str(&format!( + dynamic.push_str(&format!( "{} at {}:{}\n", f.code, f.file.display(), f.line )); } - prompt.push_str( + 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", ); } - // 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'); - } - - // 7. Output format (always last) - prompt.push_str("=== OUTPUT FORMAT ===\n"); - prompt.push_str(OUTPUT_FORMAT); - prompt.push('\n'); - - prompt + PromptPartsOwned { system, cacheable, dynamic } } + /// 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() }; @@ -167,20 +236,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(); @@ -225,8 +280,157 @@ 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 +/// 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. +/// +/// 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 { diff --git a/src/validate.rs b/src/validate.rs new file mode 100644 index 0000000..3b3e052 --- /dev/null +++ b/src/validate.rs @@ -0,0 +1,222 @@ +//! 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 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 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, + /// 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)] +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 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; + for line in &hunk.lines { + match line { + DiffLine::Added(text) => { + entry.added.insert(line_num); + entry.content.insert(line_num, text.clone()); + line_num += 1; + } + DiffLine::Context(text) => { + entry.context.insert(line_num); + entry.content.insert(line_num, text.clone()); + line_num += 1; + } + DiffLine::Removed(_) => { + // Removed lines have no new-line number. + } + } + } + } + } + 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.lookup_file(file).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. + if finding.severity == Severity::Lgtm { + 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 { on_change: false }; + } + let Some(line) = finding.line else { + return Validation::Accept { on_change: false }; + }; + + let file_key = finding.file.to_string_lossy(); + let matches: Option<&FileLines> = self.lookup_file(file_key.as_ref()); + + let Some(lines) = matches else { + return Validation::Drop(DropReason::UnknownFile); + }; + + if lines.added.contains(&line) { + return Validation::Accept { on_change: true }; + } + if lines.context.contains(&line) { + return Validation::Accept { on_change: false }; + } + + // 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_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), + } + } + + /// 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 { 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)), + } + } + +} + +#[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 { on_change: bool }, + Reanchor { from: u32, to: u32, on_change: bool }, + Drop(DropReason), +} +