diff --git a/src/config/schema.rs b/src/config/schema.rs index f4a11ffa..f6daea87 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -36,6 +36,8 @@ pub struct Config { pub timeout: u64, /// Cache TTL in minutes for review caching. pub cache_ttl: u64, + /// Static analysis context injection for reviews. + pub static_analysis: StaticAnalysisConfig, } /// Provider configuration. @@ -110,6 +112,7 @@ impl Default for Config { max_tokens: 4096, timeout: 120, cache_ttl: 1440, // 24h in minutes + static_analysis: StaticAnalysisConfig::default(), } } } @@ -189,6 +192,26 @@ pub struct ReviewSection { pub system_prompt: Option, #[serde(skip_serializing_if = "Option::is_none")] pub system_prompt_file: Option, + /// Static analysis context injection (e.g., clippy output). + #[serde(skip_serializing_if = "Option::is_none")] + pub static_analysis: Option, +} + +/// Static analysis configuration — inject linter/compiler output as review context. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct StaticAnalysisConfig { + /// Automatically run `cargo clippy` and inject output as context. + /// Adds ~2-5 seconds to review time. + #[serde(default, skip_serializing_if = "is_default")] + pub auto_clippy: bool, + /// Path to a file containing pre-computed static analysis output (e.g., clippy JSON). + /// If set, this file's content is injected instead of running auto_clippy. + #[serde(skip_serializing_if = "Option::is_none")] + pub clippy_output_file: Option, +} + +fn is_default(val: &T) -> bool { + *val == T::default() } /// Scan-specific configuration section. @@ -275,6 +298,9 @@ impl CoraFile { if let Some(v) = &r.system_prompt_file { config.review_system_prompt_file = Some(v.clone()); } + if let Some(sa) = &r.static_analysis { + config.static_analysis.clone_from(sa); + } } if let Some(s) = &self.scan { if let Some(v) = &s.system_prompt { @@ -629,6 +655,7 @@ review: response_format: Some("json_object".to_string()), system_prompt: None, system_prompt_file: None, + static_analysis: None, }), ..Default::default() }; @@ -644,6 +671,7 @@ review: response_format: None, system_prompt: Some("Custom prompt here.".to_string()), system_prompt_file: None, + static_analysis: None, }), ..Default::default() }; @@ -662,6 +690,7 @@ review: response_format: None, system_prompt: None, system_prompt_file: Some("prompts/review.md".to_string()), + static_analysis: None, }), ..Default::default() }; diff --git a/src/engine/llm.rs b/src/engine/llm.rs index b3ab8a0d..9e700150 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -257,6 +257,7 @@ fn create_spinner(message: &str) -> ProgressBar { } /// Review a diff using the LLM. Returns a `ReviewResponse`. +#[allow(clippy::too_many_arguments)] pub async fn review_diff( llm_config: &LLMConfig, diff: &str, @@ -265,6 +266,7 @@ pub async fn review_diff( response_format: &str, system_prompt_override: Option<&str>, quiet: bool, + static_context: Option<&str>, ) -> std::result::Result { let spinner = if quiet { None @@ -272,7 +274,7 @@ pub async fn review_diff( Some(create_spinner("Reviewing diff…")) }; - let user_prompt = build_review_prompt(diff, focus, rules); + let user_prompt = build_review_prompt(diff, focus, rules, static_context); let system_prompt = system_prompt_override.unwrap_or(REVIEW_SYSTEM_PROMPT); @@ -336,6 +338,7 @@ pub async fn review_diff( /// /// Streams tokens from the LLM and prints them to stdout in real-time, /// then collects the full response for parsing. +#[allow(clippy::too_many_arguments)] pub async fn review_diff_stream( llm_config: &LLMConfig, diff: &str, @@ -343,8 +346,9 @@ pub async fn review_diff_stream( rules: &[String], response_format: &str, system_prompt_override: Option<&str>, + static_context: Option<&str>, ) -> std::result::Result { - let user_prompt = build_review_prompt(diff, focus, rules); + let user_prompt = build_review_prompt(diff, focus, rules, static_context); let system_prompt = system_prompt_override.unwrap_or(REVIEW_SYSTEM_PROMPT); @@ -585,7 +589,12 @@ pub(crate) fn extract_file_paths_from_diff(diff: &str) -> Vec { /// Build the user prompt for diff review. #[allow(clippy::format_push_string)] -fn build_review_prompt(diff: &str, focus: &[String], rules: &[String]) -> String { +fn build_review_prompt( + diff: &str, + focus: &[String], + rules: &[String], + static_context: Option<&str>, +) -> String { let mut prompt = String::new(); // Inject valid file paths to reduce hallucination @@ -598,6 +607,16 @@ fn build_review_prompt(diff: &str, focus: &[String], rules: &[String]) -> String prompt.push('\n'); } + // Inject static analysis context (clippy output, etc.) + if let Some(ctx) = static_context { + if !ctx.is_empty() { + prompt.push_str("Static analysis context (pre-verified by compiler/linter):\n"); + prompt.push_str("---\n"); + prompt.push_str(ctx); + prompt.push_str("\n---\n\n"); + } + } + if !focus.is_empty() { prompt.push_str(&format!("Focus areas: {}\n\n", focus.join(", "))); } @@ -1066,34 +1085,34 @@ mod tests { #[test] fn build_prompt_basic() { - let prompt = build_review_prompt("diff content", &[], &[]); + let prompt = build_review_prompt("diff content", &[], &[], None); assert!(prompt.contains("diff content")); assert!(prompt.contains("```diff")); } #[test] fn build_prompt_with_focus() { - let prompt = build_review_prompt("d", &["security".to_string()], &[]); + let prompt = build_review_prompt("d", &["security".to_string()], &[], None); assert!(prompt.contains("Focus areas: security")); } #[test] fn build_prompt_with_rules() { - let prompt = build_review_prompt("d", &[], &["no unwrap".to_string()]); + let prompt = build_review_prompt("d", &[], &["no unwrap".to_string()], None); assert!(prompt.contains("no unwrap")); } #[test] fn build_prompt_contains_file_paths() { let diff = "diff --git a/src/main.rs b/src/main.rs\n--- a/src/main.rs\n+++ b/src/main.rs\n@@ -1 +1 @@\n- old\n+ new"; - let prompt = build_review_prompt(diff, &[], &[]); + let prompt = build_review_prompt(diff, &[], &[], None); assert!(prompt.contains("Valid files in this diff:")); assert!(prompt.contains("src/main.rs")); } #[test] fn build_prompt_no_file_paths_for_empty_diff() { - let prompt = build_review_prompt("no diff headers here", &[], &[]); + let prompt = build_review_prompt("no diff headers here", &[], &[], None); assert!(!prompt.contains("Valid files in this diff:")); } diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 99ec7fbb..d4b01d03 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -2,6 +2,7 @@ pub mod cache; pub mod llm; pub mod review; pub mod scanner; +pub mod static_analysis; pub mod types; // Re-export commonly used types from other modules for convenience diff --git a/src/engine/review.rs b/src/engine/review.rs index c3507bdf..0f064215 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -109,6 +109,10 @@ async fn review_diff_inner( config.review_system_prompt_file.as_deref(), ); + // Collect static analysis context (clippy output, etc.) + let static_context = + crate::engine::static_analysis::collect_static_context(diff, &config.static_analysis); + let mut response = if stream { llm::review_diff_stream( llm_config, @@ -117,6 +121,7 @@ async fn review_diff_inner( &config.rules, &config.response_format, review_prompt.as_deref(), + static_context.as_deref(), ) .await? } else { @@ -128,6 +133,7 @@ async fn review_diff_inner( &config.response_format, review_prompt.as_deref(), quiet, + static_context.as_deref(), ) .await? }; diff --git a/src/engine/static_analysis.rs b/src/engine/static_analysis.rs new file mode 100644 index 00000000..7c8d5cc4 --- /dev/null +++ b/src/engine/static_analysis.rs @@ -0,0 +1,251 @@ +#![allow(clippy::question_mark, unused_mut)] + +/// Static analysis integration — run clippy and extract relevant output. +/// +/// Used to inject compiler/linter context into the review prompt, +/// reducing false positives on verified-intentional changes. +use std::time::Duration; +use tracing::debug; + +use crate::config::schema::StaticAnalysisConfig; + +/// Maximum clippy output to inject (in characters). +/// Keeps prompt tokens reasonable — clippy output for a focused diff +/// should rarely exceed this. +const MAX_CLIPPY_OUTPUT_CHARS: usize = 4000; + +/// Run static analysis and return formatted context string, or None. +/// +pub fn collect_static_context(diff: &str, config: &StaticAnalysisConfig) -> Option { + // Wrap in block_in_place — called from async review pipeline + tokio::task::block_in_place(|| collect_static_context_inner(diff, config)) +} + +fn collect_static_context_inner(diff: &str, config: &StaticAnalysisConfig) -> Option { + if let Some(file_path) = &config.clippy_output_file { + return read_clippy_file(file_path); + } + + if config.auto_clippy { + return run_clippy_for_diff(diff); + } + + None +} + +/// Read clippy output from a pre-computed file with path traversal guard. +fn read_clippy_file(path: &str) -> Option { + let Ok(canonical) = std::fs::canonicalize(path) else { + debug!(path = path, "clippy output file does not exist"); + return None; + }; + + let project_root = std::env::current_dir().ok()?; + let project_root = std::fs::canonicalize(&project_root).ok()?; + + if !canonical.starts_with(&project_root) { + debug!( + path = path, + "clippy output file is outside project root, ignoring (path traversal guard)" + ); + return None; + } + + let content = std::fs::read_to_string(&canonical).ok()?; + let trimmed = content.trim(); + + if trimmed.is_empty() { + debug!("clippy output file is empty, skipping"); + return None; + } + + Some(trimmed.to_string()) +} + +/// Run `cargo clippy` and filter output to lines mentioning files in the diff. +/// Uses `block_in_place` to avoid starving the async runtime. +/// Has a 30-second timeout on clippy execution. +fn run_clippy_for_diff(diff: &str) -> Option { + let changed_files = extract_changed_rust_files(diff); + if changed_files.is_empty() { + debug!("no Rust files changed in diff, skipping clippy"); + return None; + } + + debug!( + files = changed_files.len(), + "running clippy for changed Rust files" + ); + + let result = tokio::task::block_in_place(|| { + let mut child = std::process::Command::new("cargo") + .args([ + "clippy", + "--message-format=short", + "--", + "-W", + "clippy::all", + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn(); + + let child = match child { + Ok(c) => c, + Err(e) => { + debug!("clippy spawn failed: {}", e); + return None; + } + }; + + // Spawn thread to wait + collect output with 30-second timeout. + // Child is moved into thread — on timeout we rely on the thread + // completing and the OS reaping the process. In practice clippy + // rarely hangs; the timeout is a safety net. + let (tx, rx) = std::sync::mpsc::channel(); + let child_pid = child.id(); + std::thread::spawn(move || match child.wait_with_output() { + Ok(output) => { + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + let _ = tx.send((stdout, stderr)); + } + Err(e) => { + debug!("clippy wait_with_output failed: {}", e); + } + }); + + match rx.recv_timeout(Duration::from_secs(30)) { + Ok((stdout, stderr)) => Some((stdout, stderr)), + Err(_) => { + debug!(pid = child_pid, "clippy timed out after 30 seconds"); + None + } + } + }); + let (stdout, stderr) = match result { + Some(r) => r, + None => return None, + }; + + // Combine stdout + stderr, clippy sometimes outputs to stderr + let combined = format!("{}\n{}", stdout, stderr); + + // Filter to lines mentioning changed files + let mut relevant_lines = Vec::new(); + for line in combined.lines() { + let trimmed = line.trim(); + // Skip cargo build messages, empty lines, progress output + if trimmed.is_empty() + || trimmed.starts_with("Compiling") + || trimmed.starts_with("Downloading") + || trimmed.starts_with("Fresh") + || trimmed.starts_with("Finished") + || trimmed.starts_with("Updating") + || trimmed.starts_with("Locking") + { + continue; + } + + // Check if line mentions any changed file + if changed_files.iter().any(|f| line.contains(f.as_str())) { + relevant_lines.push(trimmed.to_string()); + } + } + + if relevant_lines.is_empty() { + debug!("no clippy warnings for changed files"); + return None; + } + + let result = relevant_lines.join("\n"); + + // Truncate at char boundary (safe for UTF-8) + if result.len() > MAX_CLIPPY_OUTPUT_CHARS { + let mut end = MAX_CLIPPY_OUTPUT_CHARS; + // floor_char_boundary is available on Rust 1.85+ + while !result.is_char_boundary(end) { + end -= 1; + } + debug!( + original_len = result.len(), + truncated_len = end, + "clippy output truncated" + ); + Some(format!("{}\n... (truncated)", &result[..end])) + } else { + Some(result) + } +} + +/// Extract changed Rust file paths from a diff. +/// Returns base paths like `src/engine/llm.rs` (no `a/` or `b/` prefix). +fn extract_changed_rust_files(diff: &str) -> Vec { + let mut files = Vec::new(); + + for line in diff.lines() { + let trimmed = line.trim(); + + // Match diff headers: --- a/path.rs or +++ b/path.rs + if let Some(path) = trimmed + .strip_prefix("--- a/") + .or_else(|| trimmed.strip_prefix("+++ b/")) + { + if path.ends_with(".rs") && !path.contains("target/") { + files.push(path.to_string()); + } + } + } + + files.sort(); + files.dedup(); + files +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn extract_rust_files_from_diff() { + let diff = "--- a/src/main.rs\n+++ b/src/main.rs\n@@ -1,1 +1,2 @@\n use anyhow::Result;\n--- a/src/engine/llm.rs\n+++ b/src/engine/llm.rs\n@@ -10,1 +10,2 @@\n pub async fn review_diff()"; + + let files = extract_changed_rust_files(diff); + assert_eq!( + files, + vec!["src/engine/llm.rs".to_string(), "src/main.rs".to_string()] + ); + } + + #[test] + fn extract_skips_non_rust() { + let diff = "--- a/src/main.rs\n+++ b/src/main.rs\n--- a/package.json\n+++ b/package.json"; + + let files = extract_changed_rust_files(diff); + assert_eq!(files, vec!["src/main.rs".to_string()]); + } + + #[test] + fn extract_skips_target_dir() { + let diff = "--- a/target/debug/build.rs\n+++ b/target/debug/build.rs"; + let files = extract_changed_rust_files(diff); + assert!(files.is_empty()); + } + + #[test] + fn empty_config_returns_none() { + let config = StaticAnalysisConfig::default(); + let result = collect_static_context("some diff", &config); + assert!(result.is_none()); + } + + #[test] + fn empty_diff_returns_none_for_auto_clippy() { + let config = StaticAnalysisConfig { + auto_clippy: true, + clippy_output_file: None, + }; + let result = collect_static_context("", &config); + assert!(result.is_none()); + } +}