diff --git a/.github/workflows/diffscope.yml b/.github/workflows/diffscope.yml index 4cae662..a01608a 100644 --- a/.github/workflows/diffscope.yml +++ b/.github/workflows/diffscope.yml @@ -27,14 +27,26 @@ jobs: git fetch origin ${{ github.base_ref }} --depth=1 git diff origin/${{ github.base_ref }}...HEAD > pr.diff + - name: Check API key + id: check_key + run: | + if [ -z "${{ secrets.OPENAI_API_KEY }}" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "::notice::DiffScope review skipped — OPENAI_API_KEY secret not configured" + else + echo "skip=false" >> "$GITHUB_OUTPUT" + fi + - name: Run DiffScope + if: steps.check_key.outputs.skip != 'true' uses: docker://ghcr.io/haasonsaas/diffscope:latest env: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} with: args: review --diff pr.diff --output-format json --output comments.json - + - name: Post comments + if: steps.check_key.outputs.skip != 'true' uses: actions/github-script@v7 with: script: | diff --git a/Cargo.lock b/Cargo.lock index a3716e9..bd61b85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1934,9 +1934,9 @@ dependencies = [ [[package]] name = "quinn-proto" -version = "0.11.13" +version = "0.11.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f1906b49b0c3bc04b5fe5d86a77925ae6524a19b816ae38ce1e426255f1d8a31" +checksum = "434b42fec591c96ef50e21e886936e66d3cc3f737104fdb9b737c40ffb94c098" dependencies = [ "bytes", "getrandom 0.3.3", diff --git a/src/config.rs b/src/config.rs index dd9aad2..5fce667 100644 --- a/src/config.rs +++ b/src/config.rs @@ -4,6 +4,24 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use tracing::warn; +/// Identifies the role a model plays in the review pipeline. +/// +/// Different tasks benefit from different model tiers: cheap/fast models +/// for triage and summarization, frontier models for deep review, and +/// specialised models for reasoning or embeddings. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ModelRole { + /// The main review model (default). + Primary, + /// Cheap/fast model for triage, summarization, NL translation. + Weak, + /// Reasoning-capable model for complex analysis and self-reflection. + Reasoning, + /// Embedding model for RAG indexing. + Embedding, +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ProviderConfig { #[serde(default)] @@ -29,6 +47,22 @@ pub struct Config { #[serde(default = "default_model")] pub model: String, + /// Cheap/fast model for triage, summarization, NL translation. + #[serde(default)] + pub model_weak: Option, + + /// Reasoning-capable model for complex analysis and self-reflection. + #[serde(default)] + pub model_reasoning: Option, + + /// Embedding model for RAG indexing. + #[serde(default)] + pub model_embedding: Option, + + /// Fallback models tried in order when the primary model fails. + #[serde(default)] + pub fallback_models: Vec, + #[serde(default = "default_temperature")] pub temperature: f32, @@ -303,6 +337,10 @@ impl Default for Config { fn default() -> Self { Self { model: default_model(), + model_weak: None, + model_reasoning: None, + model_embedding: None, + fallback_models: Vec::new(), temperature: default_temperature(), max_tokens: default_max_tokens(), max_context_chars: default_max_context_chars(), @@ -869,6 +907,23 @@ impl Config { } } + /// Get the model name for a specific role, falling back to the primary model. + pub fn model_for_role(&self, role: ModelRole) -> &str { + match role { + ModelRole::Primary => &self.model, + ModelRole::Weak => self.model_weak.as_deref().unwrap_or(&self.model), + ModelRole::Reasoning => self.model_reasoning.as_deref().unwrap_or(&self.model), + ModelRole::Embedding => self.model_embedding.as_deref().unwrap_or(&self.model), + } + } + + /// Build a ModelConfig for a specific role. + pub fn to_model_config_for_role(&self, role: ModelRole) -> crate::adapters::llm::ModelConfig { + let mut config = self.to_model_config(); + config.model_name = self.model_for_role(role).to_string(); + config + } + /// Resolve which provider to use based on configuration. /// /// Returns `(api_key, base_url, adapter)` by checking: @@ -1509,4 +1564,146 @@ mod tests { Some("rust-analyzer") ); } + + #[test] + fn test_model_role_primary_returns_model() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + ..Config::default() + }; + assert_eq!( + config.model_for_role(ModelRole::Primary), + "claude-sonnet-4-6" + ); + } + + #[test] + fn test_model_role_weak_fallback_to_primary() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + model_weak: None, + ..Config::default() + }; + assert_eq!(config.model_for_role(ModelRole::Weak), "claude-sonnet-4-6"); + } + + #[test] + fn test_model_role_weak_explicit() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + model_weak: Some("claude-haiku-4-5".to_string()), + ..Config::default() + }; + assert_eq!(config.model_for_role(ModelRole::Weak), "claude-haiku-4-5"); + } + + #[test] + fn test_model_role_reasoning_fallback() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + model_reasoning: None, + ..Config::default() + }; + assert_eq!( + config.model_for_role(ModelRole::Reasoning), + "claude-sonnet-4-6" + ); + } + + #[test] + fn test_model_role_reasoning_explicit() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + model_reasoning: Some("claude-opus-4-6".to_string()), + ..Config::default() + }; + assert_eq!( + config.model_for_role(ModelRole::Reasoning), + "claude-opus-4-6" + ); + } + + #[test] + fn test_model_role_embedding_default() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + model_embedding: None, + ..Config::default() + }; + // Falls back to primary model when no embedding model configured + assert_eq!( + config.model_for_role(ModelRole::Embedding), + "claude-sonnet-4-6" + ); + } + + #[test] + fn test_model_role_embedding_explicit() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + model_embedding: Some("custom-embedding-model".to_string()), + ..Config::default() + }; + assert_eq!( + config.model_for_role(ModelRole::Embedding), + "custom-embedding-model" + ); + } + + #[test] + fn test_to_model_config_for_role_uses_correct_model() { + let config = Config { + model: "claude-sonnet-4-6".to_string(), + model_weak: Some("claude-haiku-4-5".to_string()), + ..Config::default() + }; + let primary_config = config.to_model_config_for_role(ModelRole::Primary); + assert_eq!(primary_config.model_name, "claude-sonnet-4-6"); + + let weak_config = config.to_model_config_for_role(ModelRole::Weak); + assert_eq!(weak_config.model_name, "claude-haiku-4-5"); + } + + #[test] + fn test_fallback_models_default_empty() { + let config = Config::default(); + assert!(config.fallback_models.is_empty()); + } + + #[test] + fn test_config_deserialization_with_model_roles() { + let yaml = r#" +model: claude-sonnet-4-6 +model_weak: claude-haiku-4-5 +model_reasoning: claude-opus-4-6 +model_embedding: text-embedding-3-small +fallback_models: + - gpt-4o + - claude-sonnet-4-6 +"#; + let config: Config = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.model, "claude-sonnet-4-6"); + assert_eq!(config.model_weak, Some("claude-haiku-4-5".to_string())); + assert_eq!(config.model_reasoning, Some("claude-opus-4-6".to_string())); + assert_eq!( + config.model_embedding, + Some("text-embedding-3-small".to_string()) + ); + assert_eq!(config.fallback_models.len(), 2); + } + + #[test] + fn test_config_deserialization_without_model_roles() { + // Existing configs without new fields should still work + let yaml = r#" +model: claude-sonnet-4-6 +temperature: 0.3 +"#; + let config: Config = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(config.model, "claude-sonnet-4-6"); + assert!(config.model_weak.is_none()); + assert!(config.model_reasoning.is_none()); + assert!(config.model_embedding.is_none()); + assert!(config.fallback_models.is_empty()); + } } diff --git a/src/core/context.rs b/src/core/context.rs index 5a4103e..faea834 100644 --- a/src/core/context.rs +++ b/src/core/context.rs @@ -5,6 +5,7 @@ use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; +use crate::core::function_chunker::find_enclosing_boundary_line; use crate::core::SymbolIndex; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct LLMContextChunk { @@ -50,8 +51,18 @@ impl ContextFetcher { } let start = start.max(1); let end = end.max(start); - let start_idx = start.saturating_sub(1); - let end_idx = end.min(file_lines.len()); + + // Dynamic context: expand start to enclosing function boundary + let expanded_start = find_enclosing_boundary_line(&content, file_path, start, 10) + .filter(|&boundary| boundary >= start.saturating_sub(10)) + .unwrap_or_else(|| start.saturating_sub(5)); // fallback: 5 lines before + let expanded_start = expanded_start.max(1); + + // Asymmetric: less context after (1 extra line) + let expanded_end = (end + 1).min(file_lines.len()); + + let start_idx = expanded_start.saturating_sub(1); + let end_idx = expanded_end.min(file_lines.len()); if start_idx < file_lines.len() { let chunk_content = truncate_with_notice( @@ -62,7 +73,7 @@ impl ContextFetcher { file_path: file_path.clone(), content: chunk_content, context_type: ContextType::FileContent, - line_range: Some((start, end)), + line_range: Some((expanded_start, expanded_end)), }); } } @@ -330,6 +341,28 @@ mod tests { let merged = merge_ranges(&ranges); assert_eq!(merged, vec![(1, 10)]); } + + #[tokio::test] + async fn test_fetch_context_expands_to_function_boundary() { + // Create a temp file with a function + let dir = tempfile::tempdir().unwrap(); + let file_path = dir.path().join("test.rs"); + let content = "use std::io;\n\npub fn process(x: i32) -> bool {\n let y = x + 1;\n y > 0\n}\n\npub fn other() {\n println!(\"hi\");\n}\n"; + std::fs::write(&file_path, content).unwrap(); + + let fetcher = ContextFetcher::new(dir.path().to_path_buf()); + let relative = PathBuf::from("test.rs"); + // Request context for line 4-5 (inside process function) + let chunks = fetcher + .fetch_context_for_file(&relative, &[(4, 5)]) + .await + .unwrap(); + + assert!(!chunks.is_empty()); + // Should expand to include the function signature (line 3) + let chunk = &chunks[0]; + assert!(chunk.content.contains("pub fn process")); + } } async fn read_file_lossy(path: &Path) -> Result { diff --git a/src/core/function_chunker.rs b/src/core/function_chunker.rs index 774bc2f..7f72a86 100644 --- a/src/core/function_chunker.rs +++ b/src/core/function_chunker.rs @@ -282,6 +282,21 @@ fn find_function_end(lines: &[&str], start: usize, language: &str) -> usize { } } +/// Given file content and a line number, find the start line of the enclosing +/// function/class boundary. Searches upward from `line` up to `max_search_lines`. +/// Returns the boundary start line, or None if no boundary found. +pub fn find_enclosing_boundary_line( + content: &str, + file_path: &Path, + line: usize, + _max_search_lines: usize, +) -> Option { + let language = detect_language(file_path); + let boundaries = detect_function_boundaries(content, &language); + // Find the innermost function containing this line + find_enclosing_function(&boundaries, line).map(|b| b.start_line) +} + fn detect_language(file_path: &Path) -> String { file_path .extension() @@ -851,6 +866,51 @@ fn next_func() { assert!(result.is_some()); } + #[test] + fn test_find_enclosing_boundary_line_rust() { + let content = + "use std::io;\n\npub fn process(x: i32) -> bool {\n let y = x + 1;\n y > 0\n}\n"; + let path = Path::new("test.rs"); + // Line 4 is inside process() which starts at line 3 + let result = find_enclosing_boundary_line(content, path, 4, 10); + assert_eq!(result, Some(3)); + } + + #[test] + fn test_find_enclosing_boundary_line_no_function() { + let content = "let x = 1;\nlet y = 2;\n"; + let path = Path::new("test.rs"); + let result = find_enclosing_boundary_line(content, path, 1, 10); + assert!(result.is_none()); + } + + #[test] + fn test_find_enclosing_boundary_line_python() { + let content = + "import os\n\ndef handle_request(req):\n data = req.json()\n return data\n"; + let path = Path::new("handler.py"); + // Line 4 is inside handle_request() which starts at line 3 + let result = find_enclosing_boundary_line(content, path, 4, 10); + assert_eq!(result, Some(3)); + } + + #[test] + fn test_find_enclosing_boundary_line_unsupported_language() { + let content = "some text\nmore text\n"; + let path = Path::new("readme.txt"); + let result = find_enclosing_boundary_line(content, path, 1, 10); + assert!(result.is_none()); + } + + #[test] + fn test_find_enclosing_boundary_nested_functions() { + let content = "pub fn outer() {\n fn inner() {\n let x = 1;\n }\n}\n"; + let path = Path::new("test.rs"); + // Line 3 is inside inner() which starts at line 2 + let result = find_enclosing_boundary_line(content, path, 3, 10); + assert_eq!(result, Some(2)); + } + #[test] fn test_find_function_end_escaped_backslash_in_string() { // A string containing "\\" (escaped backslash) on the same line as braces. diff --git a/src/parsing/llm_response.rs b/src/parsing/llm_response.rs index 737755b..434f42a 100644 --- a/src/parsing/llm_response.rs +++ b/src/parsing/llm_response.rs @@ -9,6 +9,42 @@ pub fn parse_llm_response( content: &str, file_path: &Path, ) -> Result> { + // Strategy 1: Primary parser (existing regex + code suggestion blocks) + let comments = parse_primary(content, file_path)?; + if !comments.is_empty() { + return Ok(comments); + } + + // Strategy 2: Numbered list format (e.g. "1. **src/lib.rs:42** - Issue text") + let comments = parse_numbered_list(content, file_path); + if !comments.is_empty() { + return Ok(comments); + } + + // Strategy 3: Markdown bullet format (e.g. "- Line 42: Issue text") + let comments = parse_markdown_bullets(content, file_path); + if !comments.is_empty() { + return Ok(comments); + } + + // Strategy 4: file:line format (e.g. "src/lib.rs:42 - Issue text") + let comments = parse_file_line_format(content, file_path); + if !comments.is_empty() { + return Ok(comments); + } + + // Strategy 5: JSON extraction + let comments = parse_json_format(content, file_path); + if !comments.is_empty() { + return Ok(comments); + } + + // All strategies failed — return empty + Ok(Vec::new()) +} + +/// Strategy 1: Primary parser — `Line : ` with code suggestion blocks. +fn parse_primary(content: &str, file_path: &Path) -> Result> { let mut comments = Vec::new(); static LINE_PATTERN: Lazy = Lazy::new(|| { Regex::new(r"(?i)line\s+(\d+)((?:\s*(?:\[[^\]]+\]|\([^)]+\)))*)\s*:\s*(.+)").unwrap() @@ -135,6 +171,163 @@ pub fn parse_llm_response( Ok(comments) } +/// Strategy 2: Numbered list format. +/// Matches patterns like: +/// 1. **src/lib.rs:42** - Missing null check +/// 2. src/lib.rs:15 - SQL injection +fn parse_numbered_list(content: &str, file_path: &Path) -> Vec { + static NUMBERED_PATTERN: Lazy = Lazy::new(|| { + Regex::new(r"^\s*\d+\.\s*\*{0,2}(?:.*?):?(\d+)\*{0,2}\s*[-\u{2013}\u{2014}:]\s*(.+)") + .unwrap() + }); + + let mut comments = Vec::new(); + for line in content.lines() { + if let Some(caps) = NUMBERED_PATTERN.captures(line) { + if let Ok(line_number) = caps.get(1).unwrap().as_str().parse::() { + let text = caps.get(2).unwrap().as_str().trim().to_string(); + comments.push(make_raw_comment(file_path, line_number, text)); + } + } + } + comments +} + +/// Strategy 3: Markdown bullet format. +/// Matches patterns like: +/// - Line 42: Missing null check +/// * **Line 42**: Missing null check +fn parse_markdown_bullets(content: &str, file_path: &Path) -> Vec { + static BULLET_PATTERN: Lazy = Lazy::new(|| { + Regex::new(r"^\s*[-*]\s*\*{0,2}[Ll]ine\s+(\d+)\*{0,2}\s*[:–—-]\s*(.+)").unwrap() + }); + + let mut comments = Vec::new(); + for line in content.lines() { + if let Some(caps) = BULLET_PATTERN.captures(line) { + if let Ok(line_number) = caps.get(1).unwrap().as_str().parse::() { + let text = caps.get(2).unwrap().as_str().trim().to_string(); + comments.push(make_raw_comment(file_path, line_number, text)); + } + } + } + comments +} + +/// Strategy 4: file:line format. +/// Matches patterns like: +/// src/lib.rs:42 - Missing null check +/// file.py:15: SQL injection vulnerability +fn parse_file_line_format(content: &str, file_path: &Path) -> Vec { + static FILE_LINE_PATTERN: Lazy = Lazy::new(|| { + Regex::new(r"^\s*\*{0,2}(?:[\w./]+):(\d+)\*{0,2}\s*[-\u{2013}\u{2014}:]\s*(.+)").unwrap() + }); + + let mut comments = Vec::new(); + for line in content.lines() { + if let Some(caps) = FILE_LINE_PATTERN.captures(line) { + if let Ok(line_number) = caps.get(1).unwrap().as_str().parse::() { + let text = caps.get(2).unwrap().as_str().trim().to_string(); + comments.push(make_raw_comment(file_path, line_number, text)); + } + } + } + comments +} + +/// Strategy 5: JSON extraction. +/// Tries to find and parse JSON arrays from the response content. +/// Handles JSON in code blocks or bare JSON arrays. +fn parse_json_format(content: &str, file_path: &Path) -> Vec { + let json_str = extract_json_from_code_block(content).or_else(|| find_json_array(content)); + + if let Some(json_str) = json_str { + if let Ok(items) = serde_json::from_str::>(&json_str) { + return items + .iter() + .filter_map(|item| { + let line = item + .get("line") + .or_else(|| item.get("line_number")) + .or_else(|| item.get("lineNumber")) + .and_then(|v| v.as_u64()) + .map(|v| v as usize)?; + let text = item + .get("issue") + .or_else(|| item.get("description")) + .or_else(|| item.get("message")) + .or_else(|| item.get("content")) + .or_else(|| item.get("text")) + .and_then(|v| v.as_str()) + .unwrap_or("Issue found") + .to_string(); + Some(make_raw_comment(file_path, line, text)) + }) + .collect(); + } + } + Vec::new() +} + +/// Extract JSON array content from markdown code blocks (```json ... ``` or ``` ... ```). +fn extract_json_from_code_block(content: &str) -> Option { + static CODE_BLOCK: Lazy = + Lazy::new(|| Regex::new(r"(?s)```(?:json)?\s*\n(.*?)```").unwrap()); + + for caps in CODE_BLOCK.captures_iter(content) { + let block = caps.get(1).unwrap().as_str().trim(); + if block.starts_with('[') { + return Some(block.to_string()); + } + } + None +} + +/// Find a bare JSON array in the content (not in a code block). +fn find_json_array(content: &str) -> Option { + // Find the first '[' and try to parse from there + let trimmed = content.trim(); + if trimmed.starts_with('[') { + // The whole content might be a JSON array + return Some(trimmed.to_string()); + } + + // Look for a JSON array somewhere in the content + if let Some(start) = content.find('[') { + if let Some(end) = content.rfind(']') { + if end > start { + let candidate = &content[start..=end]; + // Quick validation: try to parse it + if serde_json::from_str::>(candidate).is_ok() { + return Some(candidate.to_string()); + } + } + } + } + None +} + +/// Helper to construct a RawComment with default fields. +fn make_raw_comment( + file_path: &Path, + line_number: usize, + content: String, +) -> core::comment::RawComment { + core::comment::RawComment { + file_path: file_path.to_path_buf(), + line_number, + content, + rule_id: None, + suggestion: None, + severity: None, + category: None, + confidence: None, + fix_effort: None, + tags: Vec::new(), + code_suggestion: None, + } +} + /// Build a unified-diff-style string from original and suggested code. fn build_suggestion_diff(original: &str, suggested: &str) -> String { let mut diff = String::new(); @@ -399,4 +592,115 @@ let data = &input; let diff = build_suggestion_diff("line1\nline2", "line1\nline2_fixed\nline3"); assert_eq!(diff, "- line1\n- line2\n+ line1\n+ line2_fixed\n+ line3"); } + + // === Fallback strategy tests === + + #[test] + fn parse_fallback_numbered_list() { + let input = "Here are the issues found:\n\n1. **src/lib.rs:42** - Missing null check on user input\n2. **src/lib.rs:15** - SQL injection vulnerability in query builder"; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 2); + assert_eq!(comments[0].line_number, 42); + assert_eq!(comments[1].line_number, 15); + } + + #[test] + fn parse_fallback_numbered_list_no_bold() { + let input = "1. src/lib.rs:42 - Missing null check\n2. src/lib.rs:15 - SQL injection"; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 2); + } + + #[test] + fn parse_fallback_markdown_bullets() { + let input = + "- Line 42: Missing null check on user input\n- Line 15: SQL injection vulnerability"; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 2); + assert_eq!(comments[0].line_number, 42); + assert_eq!(comments[1].line_number, 15); + } + + #[test] + fn parse_fallback_markdown_bullets_bold() { + let input = "* **Line 42**: Missing null check\n* **Line 15**: SQL injection"; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 2); + } + + #[test] + fn parse_fallback_file_line_format() { + let input = "src/lib.rs:42 - Missing null check\nsrc/lib.rs:15: SQL injection"; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 2); + assert_eq!(comments[0].line_number, 42); + assert_eq!(comments[1].line_number, 15); + } + + #[test] + fn parse_fallback_json_array() { + let input = r#"Here are the issues: +```json +[ + {"line": 42, "issue": "Missing null check", "severity": "warning"}, + {"line": 15, "issue": "SQL injection", "severity": "error"} +] +```"#; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 2); + assert_eq!(comments[0].line_number, 42); + assert_eq!(comments[1].line_number, 15); + } + + #[test] + fn parse_fallback_json_with_different_keys() { + // LLMs use various key names + let input = r#"[{"line_number": 10, "description": "Bug here", "type": "bug"}]"#; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 1); + assert_eq!(comments[0].line_number, 10); + } + + #[test] + fn parse_primary_still_takes_priority() { + // If primary format works, fallbacks should NOT run + let input = "Line 10: This is a basic issue."; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 1); + assert_eq!(comments[0].line_number, 10); + } + + #[test] + fn parse_empty_input_returns_empty() { + let input = ""; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert!(comments.is_empty()); + } + + #[test] + fn parse_no_issues_response() { + let input = "No issues found. The code looks good."; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert!(comments.is_empty()); + } + + #[test] + fn parse_fallback_preserves_code_suggestion_in_primary() { + // Code suggestions should still work with primary parser + let input = "Line 42: Bug - Off by one.\n<<>>SUGGESTED"; + let file_path = PathBuf::from("src/lib.rs"); + let comments = parse_llm_response(input, &file_path).unwrap(); + assert_eq!(comments.len(), 1); + assert!(comments[0].code_suggestion.is_some()); + } } diff --git a/src/review/compression.rs b/src/review/compression.rs new file mode 100644 index 0000000..dcaf604 --- /dev/null +++ b/src/review/compression.rs @@ -0,0 +1,701 @@ +//! Adaptive patch compression for large PRs. +//! +//! Implements a 4-stage progressive degradation strategy (inspired by Qodo/CodeRabbit): +//! Stage 1 – Full: all diffs fit within token budget → use as-is. +//! Stage 2 – Compressed: remove deletion-only hunks, sort by size, drop tail files. +//! Stage 3 – Clipped: truncate remaining large files at clean line boundaries. +//! Stage 4 – MultiCall: split into multiple LLM call batches. + +use crate::core::diff_parser::{ChangeType, DiffHunk, UnifiedDiff}; +use serde::{Deserialize, Serialize}; + +/// Rough token estimation: ~4 chars per token (industry standard fallback). +const CHARS_PER_TOKEN: usize = 4; + +/// Strategy selected by the compressor. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub enum CompressionStrategy { + Full, + Compressed, + Clipped, + MultiCall, +} + +/// A single batch of diffs that fits within the token budget. +#[derive(Debug, Clone)] +pub struct DiffBatch { + /// Indices into the original diffs vec. + pub diff_indices: Vec, + /// Estimated token count for this batch. + pub estimated_tokens: usize, +} + +/// Result of running adaptive compression. +#[derive(Debug, Clone)] +pub struct CompressionResult { + pub strategy: CompressionStrategy, + /// Batches of diffs to review (1 batch for stages 1-3, N for stage 4). + pub batches: Vec, + /// Indices of diffs that were dropped entirely. + pub skipped_indices: Vec, + /// Human-readable summary of what was skipped. + pub skipped_summary: String, +} + +/// Estimate the token cost of a single diff. +pub fn estimate_diff_tokens(diff: &UnifiedDiff) -> usize { + let chars: usize = diff + .hunks + .iter() + .map(|h| { + h.changes + .iter() + .map(|c| c.content.len() + 10) // +10 for line prefix overhead + .sum::() + + h.context.len() + + 20 // hunk header + }) + .sum(); + // Add file header overhead + let file_overhead = diff.file_path.to_string_lossy().len() + 40; + (chars + file_overhead) / CHARS_PER_TOKEN +} + +/// Check if a hunk is deletion-only (all changes are removals or context). +pub fn is_deletion_only_hunk(hunk: &DiffHunk) -> bool { + hunk.changes + .iter() + .all(|c| c.change_type == ChangeType::Removed || c.change_type == ChangeType::Context) +} + +/// Remove deletion-only hunks from a diff. Returns a new diff (or None if all hunks removed). +pub fn compress_diff(diff: &UnifiedDiff) -> Option { + let kept_hunks: Vec = diff + .hunks + .iter() + .filter(|h| !is_deletion_only_hunk(h)) + .cloned() + .collect(); + + if kept_hunks.is_empty() { + return None; + } + + Some(UnifiedDiff { + file_path: diff.file_path.clone(), + old_content: diff.old_content.clone(), + new_content: diff.new_content.clone(), + hunks: kept_hunks, + is_binary: diff.is_binary, + is_deleted: diff.is_deleted, + is_new: diff.is_new, + }) +} + +/// Clip a diff to fit within a character budget by keeping only leading hunks. +pub fn clip_diff(diff: &UnifiedDiff, max_chars: usize) -> Option { + let mut kept_hunks = Vec::new(); + let mut chars_used = 0; + + for hunk in &diff.hunks { + let hunk_chars: usize = hunk + .changes + .iter() + .map(|c| c.content.len() + 10) + .sum::() + + hunk.context.len() + + 20; + + if chars_used + hunk_chars > max_chars && !kept_hunks.is_empty() { + break; + } + kept_hunks.push(hunk.clone()); + chars_used += hunk_chars; + } + + if kept_hunks.is_empty() { + return None; + } + + Some(UnifiedDiff { + file_path: diff.file_path.clone(), + old_content: diff.old_content.clone(), + new_content: diff.new_content.clone(), + hunks: kept_hunks, + is_binary: diff.is_binary, + is_deleted: diff.is_deleted, + is_new: diff.is_new, + }) +} + +/// Build a human-readable summary of skipped files. +pub fn build_skipped_summary(diffs: &[UnifiedDiff], skipped_indices: &[usize]) -> String { + if skipped_indices.is_empty() { + return String::new(); + } + + let mut deleted = Vec::new(); + let mut modified = Vec::new(); + + for &idx in skipped_indices { + if idx < diffs.len() { + let diff = &diffs[idx]; + let path = diff.file_path.display().to_string(); + if diff.is_deleted { + deleted.push(path); + } else { + modified.push(path); + } + } + } + + let mut summary = String::new(); + if !deleted.is_empty() { + summary.push_str("Deleted files (not reviewed):\n"); + for f in &deleted { + summary.push_str(&format!(" - {}\n", f)); + } + } + if !modified.is_empty() { + if !summary.is_empty() { + summary.push('\n'); + } + summary.push_str("Additional modified files (not reviewed due to context budget):\n"); + for f in &modified { + summary.push_str(&format!(" - {}\n", f)); + } + } + summary +} + +/// Run adaptive compression on a set of diffs. +/// +/// `token_budget` is the maximum tokens available for diff content. +/// `max_calls` is the maximum number of LLM calls for multi-call splitting. +pub fn compress_diffs( + diffs: &[UnifiedDiff], + token_budget: usize, + max_calls: usize, +) -> CompressionResult { + if diffs.is_empty() { + return CompressionResult { + strategy: CompressionStrategy::Full, + batches: vec![], + skipped_indices: vec![], + skipped_summary: String::new(), + }; + } + + // Estimate per-file tokens + let mut file_tokens: Vec<(usize, usize)> = diffs + .iter() + .enumerate() + .map(|(i, d)| (i, estimate_diff_tokens(d))) + .collect(); + + let total_tokens: usize = file_tokens.iter().map(|(_, t)| *t).sum(); + + // Stage 1: Full — everything fits + if total_tokens <= token_budget { + return CompressionResult { + strategy: CompressionStrategy::Full, + batches: vec![DiffBatch { + diff_indices: (0..diffs.len()).collect(), + estimated_tokens: total_tokens, + }], + skipped_indices: vec![], + skipped_summary: String::new(), + }; + } + + // Stage 2: Compressed — remove deletion-only hunks, drop tail files + // Sort by token count (smallest first) to maximize files reviewed. + file_tokens.sort_by(|a, b| a.1.cmp(&b.1)); + + let mut included = Vec::new(); + let mut skipped = Vec::new(); + let mut budget_used = 0; + + for &(idx, _tokens) in &file_tokens { + // Re-estimate with compression + let compressed_tokens = if let Some(compressed) = compress_diff(&diffs[idx]) { + estimate_diff_tokens(&compressed) + } else { + // All hunks deletion-only — skip + skipped.push(idx); + continue; + }; + + if budget_used + compressed_tokens <= token_budget { + included.push(idx); + budget_used += compressed_tokens; + } else { + skipped.push(idx); + } + } + + if skipped.is_empty() && !included.is_empty() { + // Everything fit after compression + included.sort(); + return CompressionResult { + strategy: CompressionStrategy::Compressed, + batches: vec![DiffBatch { + diff_indices: included, + estimated_tokens: budget_used, + }], + skipped_indices: vec![], + skipped_summary: String::new(), + }; + } + + if !skipped.is_empty() && !included.is_empty() && max_calls <= 1 { + // Can't split further — return compressed result with skipped files + included.sort(); + skipped.sort(); + let summary = build_skipped_summary(diffs, &skipped); + return CompressionResult { + strategy: CompressionStrategy::Compressed, + batches: vec![DiffBatch { + diff_indices: included, + estimated_tokens: budget_used, + }], + skipped_indices: skipped, + skipped_summary: summary, + }; + } + + // Stage 3: Clipped — truncate large files to fit + let per_file_char_budget = (token_budget * CHARS_PER_TOKEN) / diffs.len().max(1); + let mut clipped_included = Vec::new(); + let mut clipped_skipped = Vec::new(); + let mut clipped_budget = 0; + + // Reset and try with clipping + for (idx, diff) in diffs.iter().enumerate() { + let clipped = clip_diff(diff, per_file_char_budget); + if let Some(ref clipped_diff) = clipped { + let tokens = estimate_diff_tokens(clipped_diff); + if clipped_budget + tokens <= token_budget { + clipped_included.push(idx); + clipped_budget += tokens; + continue; + } + } + clipped_skipped.push(idx); + } + + if !clipped_included.is_empty() && clipped_included.len() > included.len() { + let summary = build_skipped_summary(diffs, &clipped_skipped); + return CompressionResult { + strategy: CompressionStrategy::Clipped, + batches: vec![DiffBatch { + diff_indices: clipped_included, + estimated_tokens: clipped_budget, + }], + skipped_indices: clipped_skipped, + skipped_summary: summary, + }; + } + + // Stage 4: MultiCall — split into multiple batches + let max_calls = max_calls.max(1); + let per_batch_budget = token_budget; + let mut batches: Vec = Vec::new(); + let mut multi_skipped = Vec::new(); + + // Sort files by size (smallest first for better bin packing) + let mut sorted_files: Vec<(usize, usize)> = diffs + .iter() + .enumerate() + .map(|(i, d)| (i, estimate_diff_tokens(d))) + .collect(); + sorted_files.sort_by_key(|(_, t)| *t); + + for (idx, tokens) in sorted_files { + if tokens > per_batch_budget { + // Even alone this file exceeds the budget — try clipping + if let Some(clipped) = clip_diff(&diffs[idx], per_batch_budget * CHARS_PER_TOKEN) { + let clipped_tokens = estimate_diff_tokens(&clipped); + // Try to fit in existing batch or create new one + let placed = batches + .iter_mut() + .find(|b| b.estimated_tokens + clipped_tokens <= per_batch_budget); + if let Some(batch) = placed { + batch.diff_indices.push(idx); + batch.estimated_tokens += clipped_tokens; + } else if batches.len() < max_calls { + batches.push(DiffBatch { + diff_indices: vec![idx], + estimated_tokens: clipped_tokens, + }); + } else { + multi_skipped.push(idx); + } + } else { + multi_skipped.push(idx); + } + continue; + } + + // Try to fit in existing batch + let placed = batches + .iter_mut() + .find(|b| b.estimated_tokens + tokens <= per_batch_budget); + if let Some(batch) = placed { + batch.diff_indices.push(idx); + batch.estimated_tokens += tokens; + } else if batches.len() < max_calls { + batches.push(DiffBatch { + diff_indices: vec![idx], + estimated_tokens: tokens, + }); + } else { + multi_skipped.push(idx); + } + } + + // Sort indices within each batch + for batch in &mut batches { + batch.diff_indices.sort(); + } + multi_skipped.sort(); + + let summary = build_skipped_summary(diffs, &multi_skipped); + CompressionResult { + strategy: CompressionStrategy::MultiCall, + batches, + skipped_indices: multi_skipped, + skipped_summary: summary, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::core::diff_parser::{ChangeType, DiffHunk, DiffLine, UnifiedDiff}; + use std::path::PathBuf; + + fn make_line(change_type: ChangeType, content: &str) -> DiffLine { + DiffLine { + old_line_no: Some(1), + new_line_no: Some(1), + change_type, + content: content.to_string(), + } + } + + fn make_hunk(changes: Vec) -> DiffHunk { + DiffHunk { + old_start: 1, + old_lines: 10, + new_start: 1, + new_lines: 10, + context: String::new(), + changes, + } + } + + fn make_diff(path: &str, hunks: Vec) -> UnifiedDiff { + UnifiedDiff { + file_path: PathBuf::from(path), + old_content: None, + new_content: None, + hunks, + is_binary: false, + is_deleted: false, + is_new: false, + } + } + + fn make_simple_diff(path: &str, content_size: usize) -> UnifiedDiff { + let content = "x".repeat(content_size); + make_diff( + path, + vec![make_hunk(vec![make_line(ChangeType::Added, &content)])], + ) + } + + // --- estimate_diff_tokens --- + + #[test] + fn test_estimate_tokens_empty_diff() { + let diff = make_diff("file.rs", vec![]); + let tokens = estimate_diff_tokens(&diff); + // Only file overhead + assert!(tokens > 0); + assert!(tokens < 50); + } + + #[test] + fn test_estimate_tokens_scales_with_content() { + let small = make_simple_diff("a.rs", 100); + let large = make_simple_diff("b.rs", 1000); + assert!(estimate_diff_tokens(&large) > estimate_diff_tokens(&small)); + } + + #[test] + fn test_estimate_tokens_roughly_correct() { + // 400 chars of content ~ 100 tokens + let diff = make_simple_diff("file.rs", 400); + let tokens = estimate_diff_tokens(&diff); + // Should be approximately 100 + overhead + assert!(tokens >= 80, "tokens={} too low", tokens); + assert!(tokens <= 200, "tokens={} too high", tokens); + } + + // --- is_deletion_only_hunk --- + + #[test] + fn test_deletion_only_hunk_all_removed() { + let hunk = make_hunk(vec![ + make_line(ChangeType::Removed, "old line 1"), + make_line(ChangeType::Removed, "old line 2"), + ]); + assert!(is_deletion_only_hunk(&hunk)); + } + + #[test] + fn test_deletion_only_hunk_with_context() { + let hunk = make_hunk(vec![ + make_line(ChangeType::Context, "context"), + make_line(ChangeType::Removed, "deleted"), + make_line(ChangeType::Context, "more context"), + ]); + assert!(is_deletion_only_hunk(&hunk)); + } + + #[test] + fn test_not_deletion_only_with_additions() { + let hunk = make_hunk(vec![ + make_line(ChangeType::Removed, "old"), + make_line(ChangeType::Added, "new"), + ]); + assert!(!is_deletion_only_hunk(&hunk)); + } + + #[test] + fn test_not_deletion_only_additions_only() { + let hunk = make_hunk(vec![make_line(ChangeType::Added, "new code")]); + assert!(!is_deletion_only_hunk(&hunk)); + } + + // --- compress_diff --- + + #[test] + fn test_compress_diff_keeps_mixed_hunks() { + let diff = make_diff( + "file.rs", + vec![make_hunk(vec![ + make_line(ChangeType::Removed, "old"), + make_line(ChangeType::Added, "new"), + ])], + ); + let compressed = compress_diff(&diff); + assert!(compressed.is_some()); + assert_eq!(compressed.unwrap().hunks.len(), 1); + } + + #[test] + fn test_compress_diff_removes_deletion_only_hunks() { + let diff = make_diff( + "file.rs", + vec![ + make_hunk(vec![make_line(ChangeType::Removed, "deleted")]), + make_hunk(vec![make_line(ChangeType::Added, "added")]), + ], + ); + let compressed = compress_diff(&diff).unwrap(); + assert_eq!(compressed.hunks.len(), 1); + } + + #[test] + fn test_compress_diff_returns_none_when_all_deletion() { + let diff = make_diff( + "file.rs", + vec![make_hunk(vec![make_line(ChangeType::Removed, "deleted")])], + ); + assert!(compress_diff(&diff).is_none()); + } + + // --- clip_diff --- + + #[test] + fn test_clip_diff_keeps_hunks_within_budget() { + let diff = make_diff( + "file.rs", + vec![ + make_hunk(vec![make_line(ChangeType::Added, &"x".repeat(100))]), + make_hunk(vec![make_line(ChangeType::Added, &"y".repeat(100))]), + make_hunk(vec![make_line(ChangeType::Added, &"z".repeat(100))]), + ], + ); + let clipped = clip_diff(&diff, 200).unwrap(); + assert!(clipped.hunks.len() < diff.hunks.len()); + } + + #[test] + fn test_clip_diff_keeps_at_least_one_hunk() { + let diff = make_diff( + "file.rs", + vec![make_hunk(vec![make_line( + ChangeType::Added, + &"x".repeat(1000), + )])], + ); + // Even with tiny budget, should keep at least one hunk + let clipped = clip_diff(&diff, 10).unwrap(); + assert_eq!(clipped.hunks.len(), 1); + } + + #[test] + fn test_clip_diff_empty_diff() { + let diff = make_diff("file.rs", vec![]); + assert!(clip_diff(&diff, 1000).is_none()); + } + + // --- build_skipped_summary --- + + #[test] + fn test_skipped_summary_empty() { + let diffs = vec![make_simple_diff("a.rs", 100)]; + assert!(build_skipped_summary(&diffs, &[]).is_empty()); + } + + #[test] + fn test_skipped_summary_includes_modified_files() { + let diffs = vec![make_simple_diff("a.rs", 100), make_simple_diff("b.rs", 100)]; + let summary = build_skipped_summary(&diffs, &[1]); + assert!(summary.contains("b.rs")); + assert!(summary.contains("not reviewed")); + } + + #[test] + fn test_skipped_summary_separates_deleted_files() { + let mut deleted = make_simple_diff("old.rs", 100); + deleted.is_deleted = true; + let diffs = vec![make_simple_diff("a.rs", 100), deleted]; + let summary = build_skipped_summary(&diffs, &[1]); + assert!(summary.contains("Deleted files")); + assert!(summary.contains("old.rs")); + } + + // --- compress_diffs (full pipeline) --- + + #[test] + fn test_stage1_full_when_fits() { + let diffs = vec![make_simple_diff("a.rs", 100), make_simple_diff("b.rs", 100)]; + let result = compress_diffs(&diffs, 10000, 5); + assert_eq!(result.strategy, CompressionStrategy::Full); + assert_eq!(result.batches.len(), 1); + assert_eq!(result.batches[0].diff_indices.len(), 2); + assert!(result.skipped_indices.is_empty()); + } + + #[test] + fn test_stage2_compressed_drops_tail() { + // Create diffs where total exceeds budget but some fit + let diffs = vec![ + make_simple_diff("small.rs", 100), + make_simple_diff("huge.rs", 50000), + ]; + let small_tokens = estimate_diff_tokens(&diffs[0]); + // Budget: fits small but not huge + let result = compress_diffs(&diffs, small_tokens + 50, 1); + assert!( + result.strategy == CompressionStrategy::Compressed + || result.strategy == CompressionStrategy::Clipped + ); + assert!(!result.skipped_indices.is_empty()); + } + + #[test] + fn test_stage4_multicall_splits() { + // Multiple files that individually fit but not together. + // Use large enough files that compression can't rescue them. + let diffs = vec![ + make_simple_diff("a.rs", 4000), + make_simple_diff("b.rs", 4000), + make_simple_diff("c.rs", 4000), + ]; + let single_tokens = estimate_diff_tokens(&diffs[0]); + // Budget fits exactly 1 file, allow 3 calls + let result = compress_diffs(&diffs, single_tokens, 3); + assert!( + result.batches.len() >= 2, + "Expected multiple batches, got {} (single_tokens={})", + result.batches.len(), + single_tokens, + ); + } + + #[test] + fn test_multicall_respects_max_calls() { + let diffs: Vec<_> = (0..10) + .map(|i| make_simple_diff(&format!("file{}.rs", i), 2000)) + .collect(); + let single_tokens = estimate_diff_tokens(&diffs[0]); + let result = compress_diffs(&diffs, single_tokens + 10, 3); + assert!( + result.batches.len() <= 3, + "Got {} batches, max was 3", + result.batches.len() + ); + } + + #[test] + fn test_empty_diffs() { + let result = compress_diffs(&[], 10000, 5); + assert_eq!(result.strategy, CompressionStrategy::Full); + assert!(result.batches.is_empty()); + } + + #[test] + fn test_all_indices_accounted_for() { + let diffs: Vec<_> = (0..5) + .map(|i| make_simple_diff(&format!("f{}.rs", i), 1000)) + .collect(); + let single_tokens = estimate_diff_tokens(&diffs[0]); + let result = compress_diffs(&diffs, single_tokens * 2, 3); + + let mut all_indices: Vec = result + .batches + .iter() + .flat_map(|b| b.diff_indices.iter().copied()) + .collect(); + all_indices.extend(result.skipped_indices.iter().copied()); + all_indices.sort(); + all_indices.dedup(); + // Every file should be either in a batch or skipped + assert_eq!(all_indices.len(), 5); + } + + #[test] + fn test_batch_indices_sorted() { + let diffs: Vec<_> = (0..5) + .map(|i| make_simple_diff(&format!("f{}.rs", i), 1000)) + .collect(); + let result = compress_diffs(&diffs, 500, 5); + for batch in &result.batches { + let sorted = { + let mut v = batch.diff_indices.clone(); + v.sort(); + v + }; + assert_eq!(batch.diff_indices, sorted); + } + } + + #[test] + fn test_single_huge_file_clipped_in_multicall() { + // One file so big it doesn't fit even alone + let diff = make_simple_diff("massive.rs", 100_000); + let tokens = estimate_diff_tokens(&diff); + // Budget is half the file + let result = compress_diffs(&[diff], tokens / 2, 1); + // Should still produce a batch (clipped) + assert!( + !result.batches.is_empty() || !result.skipped_indices.is_empty(), + "File should be either clipped or skipped" + ); + } +} diff --git a/src/review/mod.rs b/src/review/mod.rs index 91ff8a8..b7bd9cd 100644 --- a/src/review/mod.rs +++ b/src/review/mod.rs @@ -1,8 +1,11 @@ +pub(crate) mod compression; mod context_helpers; mod feedback; mod filters; mod pipeline; mod rule_helpers; +pub mod triage; +pub(crate) mod verification; pub use context_helpers::{ inject_custom_context, inject_pattern_repository_context, rank_and_trim_context_chunks, diff --git a/src/review/pipeline.rs b/src/review/pipeline.rs index bdcf2dc..0e71769 100644 --- a/src/review/pipeline.rs +++ b/src/review/pipeline.rs @@ -20,6 +20,9 @@ use crate::output::OutputFormat; use crate::parsing::parse_llm_response; use crate::plugins; +use super::compression; +use super::triage; + /// Rich result from the review pipeline, carrying comments plus telemetry metadata. #[derive(Debug, Clone, Default)] pub struct ReviewResult { @@ -176,6 +179,22 @@ async fn review_diff_content_raw_inner( } } + // Adaptive compression: decide strategy based on token budget + let token_budget = config.max_diff_chars / 4; // rough char→token conversion + let compression_result = compression::compress_diffs(&diffs, token_budget.max(2000), 5); + info!( + "Compression strategy: {:?} ({} batches, {} skipped)", + compression_result.strategy, + compression_result.batches.len(), + compression_result.skipped_indices.len(), + ); + if !compression_result.skipped_summary.is_empty() { + info!( + "Compression analysis skipped files (advisory in per-file mode):\n{}", + compression_result.skipped_summary + ); + } + // Gather git history for enhanced context let git_log_output = gather_git_log(repo_path); @@ -232,6 +251,20 @@ async fn review_diff_content_raw_inner( let adapter: Arc = Arc::from(adapters::llm::create_adapter(&model_config)?); + + // Use weak model for verification pass (cheaper, faster) + let verification_adapter: Arc = { + let weak_config = config.to_model_config_for_role(config::ModelRole::Weak); + if weak_config.model_name != model_config.model_name { + info!( + "Using weak model '{}' for verification pass", + weak_config.model_name + ); + Arc::from(adapters::llm::create_adapter(&weak_config)?) + } else { + adapter.clone() + } + }; let base_prompt_config = core::prompt::PromptConfig { max_context_chars: config.max_context_chars, max_diff_chars: config.max_diff_chars, @@ -274,6 +307,18 @@ async fn review_diff_content_raw_inner( continue; } + // Triage: skip files that don't need expensive LLM review + let triage_result = triage::triage_diff(diff); + if triage_result.should_skip() { + info!( + "Skipping {} (triage: {})", + diff.file_path.display(), + triage_result.reason() + ); + files_skipped += 1; + continue; + } + // Emit progress: preparing this file if let Some(ref cb) = on_progress { cb(ProgressUpdate { @@ -699,6 +744,37 @@ async fn review_diff_content_raw_inner( let processed_comments = plugin_manager .run_post_processors(all_comments, &repo_path_str) .await?; + + // Verification pass: ask the LLM to validate findings against actual code. + // Skip when there are no comments or too many (cost control: max 20). + let processed_comments = if !processed_comments.is_empty() && processed_comments.len() <= 20 { + let comment_count_before = processed_comments.len(); + let fallback_comments = processed_comments.clone(); + match super::verification::verify_comments( + processed_comments, + diff_content, + verification_adapter.as_ref(), + 5, // min_score threshold + ) + .await + { + Ok(verified) => { + info!( + "Verification pass: {}/{} comments passed", + verified.len(), + comment_count_before + ); + verified + } + Err(e) => { + warn!("Verification pass failed, keeping all comments: {}", e); + fallback_comments + } + } + } else { + processed_comments + }; + let processed_comments = apply_review_filters(processed_comments, &config, &feedback); // Apply enhanced filters from convention learning and composable pipeline diff --git a/src/review/triage.rs b/src/review/triage.rs new file mode 100644 index 0000000..605c7d3 --- /dev/null +++ b/src/review/triage.rs @@ -0,0 +1,815 @@ +use crate::core::diff_parser::UnifiedDiff; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TriageResult { + NeedsReview, + SkipLockFile, + SkipWhitespaceOnly, + SkipDeletionOnly, + SkipGenerated, + SkipCommentOnly, +} + +impl TriageResult { + pub fn should_skip(&self) -> bool { + !matches!(self, TriageResult::NeedsReview) + } + + pub fn reason(&self) -> &'static str { + match self { + TriageResult::NeedsReview => "needs review", + TriageResult::SkipLockFile => "lock file", + TriageResult::SkipWhitespaceOnly => "whitespace-only changes", + TriageResult::SkipDeletionOnly => "deletion-only changes", + TriageResult::SkipGenerated => "generated file", + TriageResult::SkipCommentOnly => "comment-only changes", + } + } +} + +/// Lock file names that should be auto-skipped. +const LOCK_FILES: &[&str] = &[ + "Cargo.lock", + "package-lock.json", + "yarn.lock", + "pnpm-lock.yaml", + "Gemfile.lock", + "poetry.lock", + "composer.lock", + "go.sum", + "Pipfile.lock", +]; + +/// Comment line prefixes (after trimming leading whitespace). +const COMMENT_PREFIXES: &[&str] = &["//", "# ", "/*", "*/", "* ", "--", "")], + ); + assert_eq!(triage_diff(&diff), TriageResult::SkipCommentOnly); + } + + #[test] + fn test_comment_only_python_docstring() { + let diff = make_diff( + "src/app.py", + vec![ + make_line(1, ChangeType::Added, "\"\"\""), + make_line(2, ChangeType::Added, "New docstring"), + // Note: the middle line is not a comment prefix, so this should NOT be SkipCommentOnly. + ], + ); + // The middle line "New docstring" doesn't start with a comment prefix, + // so this should be NeedsReview. + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_comment_only_mixed_with_code() { + let diff = make_diff( + "src/lib.rs", + vec![ + make_line(1, ChangeType::Removed, "// old comment"), + make_line(1, ChangeType::Added, "let x = 42;"), + ], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_comment_only_does_not_match_pointer_dereference_code() { + let diff = make_diff( + "src/lib.rs", + vec![ + make_line(1, ChangeType::Added, "*ptr = compute();"), + make_line(2, ChangeType::Added, "*buffer = data;"), + ], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_comment_only_does_not_match_rust_attributes() { + let diff = make_diff( + "src/lib.rs", + vec![ + make_line(1, ChangeType::Added, "#[derive(Debug, Clone, Serialize)]"), + make_line( + 2, + ChangeType::Added, + "#[serde(rename_all = \"snake_case\")]", + ), + ], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_comment_only_does_not_match_c_preprocessor() { + let diff = make_diff( + "src/main.c", + vec![ + make_line(1, ChangeType::Added, "#include "), + make_line(2, ChangeType::Added, "#define MAX_SIZE 100"), + ], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_comment_only_does_not_match_rust_inner_attributes() { + let diff = make_diff( + "src/lib.rs", + vec![make_line(1, ChangeType::Added, "#![allow(unused)]")], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_generated_does_not_match_user_generated_dir() { + let diff = make_diff( + "src/user_generated/profile.rs", + vec![make_line(1, ChangeType::Added, "pub struct Profile {}")], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + // ── Edge cases ─────────────────────────────────────────────────────── + + #[test] + fn test_empty_hunks_needs_review() { + let diff = make_diff("src/empty.rs", vec![]); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_normal_code_change() { + let diff = make_diff( + "src/main.rs", + vec![ + make_line(1, ChangeType::Context, "fn main() {"), + make_line(2, ChangeType::Removed, " println!(\"hello\");"), + make_line(2, ChangeType::Added, " println!(\"world\");"), + make_line(3, ChangeType::Context, "}"), + ], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_lock_file_in_subdirectory() { + // Lock files should be detected even in subdirectories + let diff = make_diff( + "packages/web/package-lock.json", + vec![make_line(1, ChangeType::Added, "dep change")], + ); + assert_eq!(triage_diff(&diff), TriageResult::SkipLockFile); + } + + #[test] + fn test_priority_lock_file_over_deletion() { + // Lock file check should take priority over deletion-only check + let diff = make_diff( + "Cargo.lock", + vec![make_line(1, ChangeType::Removed, "old dep")], + ); + assert_eq!(triage_diff(&diff), TriageResult::SkipLockFile); + } + + #[test] + fn test_priority_generated_over_deletion() { + // Generated file check should take priority over deletion-only check + let diff = make_diff( + "vendor/lib/code.go", + vec![make_line(1, ChangeType::Removed, "old code")], + ); + assert_eq!(triage_diff(&diff), TriageResult::SkipGenerated); + } + + #[test] + fn test_context_only_lines_needs_review() { + // A diff with only context lines (no actual changes) should be NeedsReview + let diff = make_diff( + "src/lib.rs", + vec![ + make_line(1, ChangeType::Context, "fn main() {"), + make_line(2, ChangeType::Context, "}"), + ], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } + + #[test] + fn test_whitespace_only_with_context_lines() { + let diff = make_diff( + "src/fmt.rs", + vec![ + make_line(1, ChangeType::Context, "fn foo() {"), + make_line(2, ChangeType::Removed, " let x = 1;"), + make_line(2, ChangeType::Added, "\tlet x = 1;"), + make_line(3, ChangeType::Context, "}"), + ], + ); + assert_eq!(triage_diff(&diff), TriageResult::SkipWhitespaceOnly); + } + + #[test] + fn test_multiple_hunks_deletion_only() { + let mut diff = make_diff( + "src/cleanup.rs", + vec![make_line(1, ChangeType::Removed, "fn old1() {}")], + ); + diff.hunks.push(DiffHunk { + old_start: 10, + old_lines: 1, + new_start: 10, + new_lines: 0, + context: String::new(), + changes: vec![make_line(10, ChangeType::Removed, "fn old2() {}")], + }); + assert_eq!(triage_diff(&diff), TriageResult::SkipDeletionOnly); + } + + #[test] + fn test_multiple_hunks_mixed_not_deletion_only() { + let mut diff = make_diff( + "src/mixed.rs", + vec![make_line(1, ChangeType::Removed, "fn old() {}")], + ); + diff.hunks.push(DiffHunk { + old_start: 10, + old_lines: 0, + new_start: 10, + new_lines: 1, + context: String::new(), + changes: vec![make_line(10, ChangeType::Added, "fn new_thing() {}")], + }); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + } +} diff --git a/src/review/verification.rs b/src/review/verification.rs new file mode 100644 index 0000000..bdaaad9 --- /dev/null +++ b/src/review/verification.rs @@ -0,0 +1,393 @@ +use anyhow::Result; +use once_cell::sync::Lazy; +use regex::Regex; +use serde::{Deserialize, Serialize}; +use tracing::info; + +use crate::adapters::llm::{LLMAdapter, LLMRequest}; +use crate::core::Comment; + +/// Result of verifying a single review comment +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct VerificationResult { + pub comment_id: String, + pub accurate: bool, + pub score: u8, // 0-10 + pub reason: String, +} + +/// Categories that should be auto-scored 0 (noise) +const AUTO_ZERO_PATTERNS: &[&str] = &[ + "docstring", + "doc comment", + "documentation comment", + "type hint", + "type annotation", + "import order", + "import sorting", + "unused import", + "trailing whitespace", + "trailing newline", +]; + +const VERIFICATION_SYSTEM_PROMPT: &str = r#"You are a code review verifier. Your job is to validate review findings against actual code. + +For each finding, assess: +1. Does the referenced issue actually exist in the code? +2. Is the description accurate? +3. Would the suggested fix (if any) work correctly? + +Score each finding 0-10: +- 8-10: Critical bugs or security issues that are clearly present +- 5-7: Valid issues that exist but may be minor +- 1-4: Questionable issues, possibly hallucinated or too trivial +- 0: Noise (docstrings, type hints, import ordering, trailing whitespace) + +Respond with one line per finding in this exact format: +FINDING : score=<0-10> accurate= reason= +"#; + +/// Verify a batch of review comments by asking the LLM to validate each one. +/// Returns only comments that pass verification (score >= min_score). +pub async fn verify_comments( + comments: Vec, + diff_content: &str, + adapter: &dyn LLMAdapter, + min_score: u8, +) -> Result> { + if comments.is_empty() { + return Ok(comments); + } + + // Build verification prompt + let prompt = build_verification_prompt(&comments, diff_content); + + let request = LLMRequest { + system_prompt: VERIFICATION_SYSTEM_PROMPT.to_string(), + user_prompt: prompt, + temperature: Some(0.0), + max_tokens: Some(2000), + }; + + let response = adapter.complete(request).await?; + let results = parse_verification_response(&response.content, &comments); + + // Filter comments based on verification results + let total_count = comments.len(); + let mut verified = Vec::new(); + for comment in comments { + let result = results.iter().find(|r| r.comment_id == comment.id); + match result { + Some(r) if r.score >= min_score => { + let mut comment = comment; + // Update confidence based on verification score + comment.confidence = (r.score as f32 / 10.0).min(1.0); + verified.push(comment); + } + Some(r) => { + // Score too low, skip + info!( + "Verification filtered comment {} (score: {})", + comment.id, r.score + ); + } + None => { + // No verification result found, keep with original confidence + verified.push(comment); + } + } + } + + info!( + "Verification: {}/{} comments passed", + verified.len(), + total_count + ); + + Ok(verified) +} + +/// Check if a comment's content matches auto-zero patterns. +pub fn is_auto_zero(content: &str) -> bool { + let lower = content.to_lowercase(); + AUTO_ZERO_PATTERNS.iter().any(|p| lower.contains(p)) +} + +fn build_verification_prompt(comments: &[Comment], diff_content: &str) -> String { + let mut prompt = String::from("## Code Diff\n```\n"); + // Include a truncated version of the diff + let truncated_diff = safe_utf8_prefix(diff_content, 8000); + prompt.push_str(truncated_diff); + prompt.push_str("\n```\n\n## Findings to Verify\n\n"); + + for (i, comment) in comments.iter().enumerate() { + prompt.push_str(&format!( + "### Finding {}\n- File: {}:{}\n- Issue: {}\n", + i + 1, + comment.file_path.display(), + comment.line_number, + comment.content, + )); + if let Some(ref suggestion) = comment.suggestion { + prompt.push_str(&format!("- Suggestion: {}\n", suggestion)); + } + prompt.push('\n'); + } + + prompt.push_str("Verify each finding against the actual code diff above.\n"); + prompt +} + +fn safe_utf8_prefix(content: &str, max_bytes: usize) -> &str { + if content.len() <= max_bytes { + return content; + } + + let mut end = max_bytes; + while end > 0 && !content.is_char_boundary(end) { + end -= 1; + } + &content[..end] +} + +fn parse_verification_response(content: &str, comments: &[Comment]) -> Vec { + static FINDING_PATTERN: Lazy = Lazy::new(|| { + Regex::new(r"(?i)FINDING\s+(\d+)\s*:\s*score\s*=\s*(\d+)\s+accurate\s*=\s*(true|false)\s+reason\s*=\s*(.+)") + .unwrap() + }); + + let mut results = Vec::new(); + + for line in content.lines() { + if let Some(caps) = FINDING_PATTERN.captures(line) { + let index: usize = caps.get(1).unwrap().as_str().parse().unwrap_or(0); + let score: u8 = caps.get(2).unwrap().as_str().parse().unwrap_or(0); + let accurate = caps.get(3).unwrap().as_str().to_lowercase() == "true"; + let reason = caps.get(4).unwrap().as_str().trim().to_string(); + + // Map 1-based index to comment + if index > 0 && index <= comments.len() { + results.push(VerificationResult { + comment_id: comments[index - 1].id.clone(), + accurate, + score: score.min(10), + reason, + }); + } + } + } + + // Apply auto-zero for noise categories + for comment in comments { + if is_auto_zero(&comment.content) { + // Override or add a zero score + if let Some(existing) = results.iter_mut().find(|r| r.comment_id == comment.id) { + existing.score = 0; + existing.reason = "Auto-zero: noise category".to_string(); + } else { + results.push(VerificationResult { + comment_id: comment.id.clone(), + accurate: false, + score: 0, + reason: "Auto-zero: noise category".to_string(), + }); + } + } + } + + results +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::core::comment::{Category, Comment, FixEffort, Severity}; + use std::path::PathBuf; + + fn make_comment(id: &str, content: &str, line: usize) -> Comment { + Comment { + id: id.to_string(), + file_path: PathBuf::from("src/lib.rs"), + line_number: line, + content: content.to_string(), + rule_id: None, + severity: Severity::Warning, + category: Category::Bug, + suggestion: None, + confidence: 0.7, + code_suggestion: None, + tags: Vec::new(), + fix_effort: FixEffort::Low, + feedback: None, + } + } + + #[test] + fn test_is_auto_zero_docstring() { + assert!(is_auto_zero("Missing docstring for public function")); + assert!(is_auto_zero("Add a documentation comment here")); + } + + #[test] + fn test_is_auto_zero_type_hint() { + assert!(is_auto_zero("Missing type annotation on parameter")); + assert!(is_auto_zero("Add type hint for return value")); + } + + #[test] + fn test_is_auto_zero_imports() { + assert!(is_auto_zero("Unused import: std::io")); + assert!(is_auto_zero("Import sorting is inconsistent")); + } + + #[test] + fn test_is_auto_zero_false_for_real_issues() { + assert!(!is_auto_zero("SQL injection vulnerability")); + assert!(!is_auto_zero("Missing null check on user input")); + assert!(!is_auto_zero("Buffer overflow in array access")); + } + + #[test] + fn test_build_verification_prompt_includes_all_findings() { + let comments = vec![ + make_comment("c1", "SQL injection risk", 10), + make_comment("c2", "Missing null check", 20), + ]; + let prompt = build_verification_prompt(&comments, "diff content here"); + assert!(prompt.contains("Finding 1")); + assert!(prompt.contains("Finding 2")); + assert!(prompt.contains("SQL injection risk")); + assert!(prompt.contains("Missing null check")); + assert!(prompt.contains("diff content here")); + } + + #[test] + fn test_build_verification_prompt_truncates_long_diff() { + let long_diff = "x".repeat(10000); + let comments = vec![make_comment("c1", "issue", 10)]; + let prompt = build_verification_prompt(&comments, &long_diff); + assert!(prompt.len() < long_diff.len() + 1000); // truncated + } + + #[test] + fn test_build_verification_prompt_utf8_safe_truncation() { + // 3000 emojis => 12k bytes, and byte 8000 is not a char boundary. + let long_diff = "😀".repeat(3000); + let comments = vec![make_comment("c1", "issue", 10)]; + let prompt = build_verification_prompt(&comments, &long_diff); + assert!(prompt.contains("## Code Diff")); + } + + #[test] + fn test_build_verification_prompt_includes_suggestion() { + let mut comment = make_comment("c1", "Use parameterized queries", 10); + comment.suggestion = Some("Use prepared statements instead".to_string()); + let prompt = build_verification_prompt(&[comment], "some diff"); + assert!(prompt.contains("Suggestion: Use prepared statements instead")); + } + + #[test] + fn test_parse_verification_response_basic() { + let comments = vec![ + make_comment("c1", "SQL injection", 10), + make_comment("c2", "Missing check", 20), + ]; + let response = "FINDING 1: score=9 accurate=true reason=SQL injection is present\nFINDING 2: score=3 accurate=false reason=Check exists on line 18"; + let results = parse_verification_response(response, &comments); + assert_eq!(results.len(), 2); + assert_eq!(results[0].score, 9); + assert!(results[0].accurate); + assert_eq!(results[1].score, 3); + assert!(!results[1].accurate); + } + + #[test] + fn test_parse_verification_response_case_insensitive() { + let comments = vec![make_comment("c1", "issue", 10)]; + let response = "finding 1: score=7 accurate=true reason=Valid issue"; + let results = parse_verification_response(response, &comments); + assert_eq!(results.len(), 1); + assert_eq!(results[0].score, 7); + } + + #[test] + fn test_parse_verification_response_auto_zero_applied() { + let comments = vec![ + make_comment("c1", "Missing docstring for function", 10), + make_comment("c2", "SQL injection risk", 20), + ]; + let response = "FINDING 1: score=5 accurate=true reason=Valid\nFINDING 2: score=9 accurate=true reason=Real issue"; + let results = parse_verification_response(response, &comments); + // c1 should be auto-zeroed despite LLM giving it score=5 + let c1_result = results.iter().find(|r| r.comment_id == "c1").unwrap(); + assert_eq!(c1_result.score, 0); + // c2 should keep its score + let c2_result = results.iter().find(|r| r.comment_id == "c2").unwrap(); + assert_eq!(c2_result.score, 9); + } + + #[test] + fn test_parse_verification_response_empty() { + let comments = vec![make_comment("c1", "issue", 10)]; + let response = "No issues to report."; + let results = parse_verification_response(response, &comments); + // Should only have auto-zero results if applicable + assert!(results.is_empty() || results.iter().all(|r| r.score == 0)); + } + + #[test] + fn test_parse_verification_response_score_clamped() { + let comments = vec![make_comment("c1", "issue", 10)]; + let response = "FINDING 1: score=15 accurate=true reason=Very important"; + let results = parse_verification_response(response, &comments); + assert_eq!(results[0].score, 10); // clamped to 10 + } + + #[test] + fn test_parse_verification_response_invalid_index() { + let comments = vec![make_comment("c1", "issue", 10)]; + let response = "FINDING 0: score=5 accurate=true reason=bad index\nFINDING 99: score=5 accurate=true reason=out of range"; + let results = parse_verification_response(response, &comments); + assert!(results.is_empty()); // both indices invalid + } + + #[test] + fn test_parse_verification_response_preserves_reason() { + let comments = vec![make_comment("c1", "issue", 10)]; + let response = + "FINDING 1: score=8 accurate=true reason=The buffer overflow is clearly present"; + let results = parse_verification_response(response, &comments); + assert_eq!(results[0].reason, "The buffer overflow is clearly present"); + } + + #[test] + fn test_parse_verification_response_multiple_auto_zero() { + let comments = vec![ + make_comment("c1", "Missing docstring for function", 10), + make_comment("c2", "Trailing whitespace on line 5", 20), + make_comment("c3", "Real security bug", 30), + ]; + // LLM only responds about c3 + let response = "FINDING 3: score=9 accurate=true reason=Valid security issue"; + let results = parse_verification_response(response, &comments); + // c1 and c2 should get auto-zero results + let c1_result = results.iter().find(|r| r.comment_id == "c1").unwrap(); + assert_eq!(c1_result.score, 0); + let c2_result = results.iter().find(|r| r.comment_id == "c2").unwrap(); + assert_eq!(c2_result.score, 0); + // c3 should keep its score + let c3_result = results.iter().find(|r| r.comment_id == "c3").unwrap(); + assert_eq!(c3_result.score, 9); + } + + #[test] + fn test_is_auto_zero_whitespace() { + assert!(is_auto_zero("trailing whitespace detected")); + assert!(is_auto_zero("Missing trailing newline at end of file")); + } + + #[test] + fn test_is_auto_zero_import_order() { + assert!(is_auto_zero("import order should be alphabetical")); + } +} diff --git a/src/server/mod.rs b/src/server/mod.rs index cd67a18..5c93e4d 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -13,14 +13,14 @@ use axum::{ routing::{delete, get, post, put}, Router, }; -use rust_embed::Embed; +use rust_embed::RustEmbed; use std::net::SocketAddr; use std::sync::Arc; use tower_http::cors::CorsLayer; use crate::config::Config; -#[derive(Embed)] +#[derive(RustEmbed)] #[folder = "web/dist"] struct WebAssets;