281 lines
9.7 KiB
Rust
281 lines
9.7 KiB
Rust
|
//! Auto-review engine — automatically checks git diff after file edits
|
||
|
|
//! using an LLM subagent.
|
||
|
|
//!
|
||
|
|
//! Flow: after each write/edit tool execution in the agent turn, the runner
|
||
|
|
//! calls `trigger_auto_review` which:
|
||
|
|
//! 1. Runs `git diff --cached` and `git diff` to get working-tree changes
|
||
|
|
//! 2. Sends the diff to a lightweight LLM call for quick review
|
||
|
|
//! 3. Emits findings as `TurnEvent::SystemNote` on the event queue
|
||
|
|
|
||
|
|
use std::collections::VecDeque;
|
||
|
|
use std::path::Path;
|
||
|
|
use std::process::Command;
|
||
|
|
use std::sync::{Arc, Mutex};
|
||
|
|
|
||
|
|
use anyhow::Result;
|
||
|
|
use tracing::{debug, info, instrument, warn};
|
||
|
|
|
||
|
|
use crate::llm::provider::LlmClient;
|
||
|
|
use crate::subagent::gating::should_review;
|
||
|
|
use crate::TurnEvent;
|
||
|
|
|
||
|
|
/// Trigger an auto-review of recent git changes.
|
||
|
|
///
|
||
|
|
/// Flow:
|
||
|
|
/// 1. Check gating conditions (edit count, consecutive empty reviews)
|
||
|
|
/// 2. Run `git diff --cached` to get staged changes
|
||
|
|
/// 3. Run `git diff` to get unstaged changes
|
||
|
|
/// 4. If there are changes, call LLM for a quick review
|
||
|
|
/// 5. Emit findings as TurnEvent::SystemNote
|
||
|
|
///
|
||
|
|
/// Returns `(had_findings, total_findings)` tuple.
|
||
|
|
#[instrument(skip(turn_events, llm_client))]
|
||
|
|
pub fn trigger_auto_review(
|
||
|
|
workspace_root: &Path,
|
||
|
|
edit_count: u32,
|
||
|
|
consecutive_empty_reviews: &mut u32,
|
||
|
|
max_skip: u32,
|
||
|
|
turn_events: &Arc<Mutex<VecDeque<TurnEvent>>>,
|
||
|
|
llm_client: Option<&LlmClient>,
|
||
|
|
) -> Result<(bool, usize)> {
|
||
|
|
// Gating check
|
||
|
|
if !should_review(edit_count, *consecutive_empty_reviews, max_skip) {
|
||
|
|
debug!("auto-review skipped by gating");
|
||
|
|
return Ok((false, 0));
|
||
|
|
}
|
||
|
|
|
||
|
|
info!("triggering auto-review");
|
||
|
|
|
||
|
|
// Run git diff to get changes
|
||
|
|
let diff = match get_git_diff(workspace_root) {
|
||
|
|
Ok(d) => d,
|
||
|
|
Err(e) => {
|
||
|
|
debug!(error = %e, "auto-review: git diff failed (not a git repo?)");
|
||
|
|
return Ok((false, 0));
|
||
|
|
}
|
||
|
|
};
|
||
|
|
|
||
|
|
if diff.is_empty() {
|
||
|
|
debug!("auto-review: no changes to review");
|
||
|
|
*consecutive_empty_reviews = consecutive_empty_reviews.saturating_add(1);
|
||
|
|
return Ok((false, 0));
|
||
|
|
}
|
||
|
|
|
||
|
|
// If we have an LLM client, do a real review
|
||
|
|
let review_result = if let Some(client) = llm_client {
|
||
|
|
perform_llm_review(client, &diff)?
|
||
|
|
} else {
|
||
|
|
// Fallback: simple heuristic review without LLM
|
||
|
|
perform_heuristic_review(&diff)
|
||
|
|
};
|
||
|
|
|
||
|
|
let had_findings = !review_result.is_empty();
|
||
|
|
let finding_count = review_result.len();
|
||
|
|
|
||
|
|
if had_findings {
|
||
|
|
*consecutive_empty_reviews = 0;
|
||
|
|
info!(finding_count, "auto-review produced findings");
|
||
|
|
|
||
|
|
// Emit findings as SystemNote events
|
||
|
|
for finding in &review_result {
|
||
|
|
let note = TurnEvent::SystemNote {
|
||
|
|
kind: "info".to_string(),
|
||
|
|
message: format!("🔍 Auto-Review: {finding}"),
|
||
|
|
};
|
||
|
|
if let Ok(mut q) = turn_events.lock() {
|
||
|
|
q.push_back(note);
|
||
|
|
}
|
||
|
|
}
|
||
|
|
} else {
|
||
|
|
*consecutive_empty_reviews = consecutive_empty_reviews.saturating_add(1);
|
||
|
|
info!("auto-review: no issues found");
|
||
|
|
}
|
||
|
|
|
||
|
|
Ok((had_findings, finding_count))
|
||
|
|
}
|
||
|
|
|
||
|
|
/// Run `git diff` to get workspace changes (both staged and unstaged).
|
||
|
|
fn get_git_diff(workspace_root: &Path) -> Result<String> {
|
||
|
|
// Check if this is a git repo
|
||
|
|
let git_dir = workspace_root.join(".git");
|
||
|
|
if !git_dir.exists() {
|
||
|
|
return Ok(String::new());
|
||
|
|
}
|
||
|
|
|
||
|
|
// Get unstaged diff
|
||
|
|
let unstaged = Command::new("git")
|
||
|
|
.arg("diff")
|
||
|
|
.current_dir(workspace_root)
|
||
|
|
.output()
|
||
|
|
.map_err(|e| anyhow::anyhow!("git diff failed: {e}"))?;
|
||
|
|
|
||
|
|
// Get staged diff
|
||
|
|
let staged = Command::new("git")
|
||
|
|
.arg("diff")
|
||
|
|
.arg("--cached")
|
||
|
|
.current_dir(workspace_root)
|
||
|
|
.output()
|
||
|
|
.map_err(|e| anyhow::anyhow!("git diff --cached failed: {e}"))?;
|
||
|
|
|
||
|
|
let mut combined = String::new();
|
||
|
|
|
||
|
|
let staged_out = String::from_utf8_lossy(&staged.stdout).trim().to_string();
|
||
|
|
if !staged_out.is_empty() {
|
||
|
|
combined.push_str("=== Staged Changes ===\n");
|
||
|
|
combined.push_str(&staged_out);
|
||
|
|
combined.push('\n');
|
||
|
|
}
|
||
|
|
|
||
|
|
let unstaged_out = String::from_utf8_lossy(&unstaged.stdout).trim().to_string();
|
||
|
|
if !unstaged_out.is_empty() {
|
||
|
|
combined.push_str("=== Unstaged Changes ===\n");
|
||
|
|
combined.push_str(&unstaged_out);
|
||
|
|
combined.push('\n');
|
||
|
|
}
|
||
|
|
|
||
|
|
// Run `git diff --stat` for summary
|
||
|
|
let stat = Command::new("git")
|
||
|
|
.arg("diff")
|
||
|
|
.arg("--stat")
|
||
|
|
.current_dir(workspace_root)
|
||
|
|
.output()
|
||
|
|
.map_err(|e| anyhow::anyhow!("git diff --stat failed: {e}"))?;
|
||
|
|
let stat_out = String::from_utf8_lossy(&stat.stdout).trim().to_string();
|
||
|
|
if !stat_out.is_empty() {
|
||
|
|
combined.push_str("=== Summary ===\n");
|
||
|
|
combined.push_str(&stat_out);
|
||
|
|
combined.push('\n');
|
||
|
|
}
|
||
|
|
|
||
|
|
Ok(combined)
|
||
|
|
}
|
||
|
|
|
||
|
|
/// Perform an LLM-based review of the git diff.
|
||
|
|
///
|
||
|
|
/// Sends the diff to the model with a focused prompt asking it to find
|
||
|
|
/// typos, missing imports, syntax errors, and other issues.
|
||
|
|
fn perform_llm_review(client: &LlmClient, diff: &str) -> Result<Vec<String>> {
|
||
|
|
// Truncate very large diffs to avoid token limits
|
||
|
|
const MAX_DIFF_CHARS: usize = 8000;
|
||
|
|
let truncated_diff = if diff.len() > MAX_DIFF_CHARS {
|
||
|
|
format!("{}...\n[diff truncated at {} characters]",
|
||
|
|
&diff[..MAX_DIFF_CHARS], MAX_DIFF_CHARS)
|
||
|
|
} else {
|
||
|
|
diff.to_string()
|
||
|
|
};
|
||
|
|
|
||
|
|
let messages = vec![
|
||
|
|
zesdex_domain::core::ChatMessage::system(
|
||
|
|
"You are a focused code reviewer. Review the following git diff for:\n\
|
||
|
|
1. Typos and spelling errors\n\
|
||
|
|
2. Missing imports or undefined references\n\
|
||
|
|
3. Syntax errors or type mismatches\n\
|
||
|
|
4. Logic bugs or off-by-one errors\n\
|
||
|
|
5. Missing error handling\n\
|
||
|
|
6. Security issues\n\n\
|
||
|
|
Be concise. List each issue on a new line with severity [HIGH], [MEDIUM], or [LOW].\n\
|
||
|
|
If no issues are found, reply with exactly: NO_ISSUES_FOUND"
|
||
|
|
.to_string(),
|
||
|
|
),
|
||
|
|
zesdex_domain::core::ChatMessage::user(format!(
|
||
|
|
"Review this git diff:\n\n```diff\n{truncated_diff}\n```"
|
||
|
|
)),
|
||
|
|
];
|
||
|
|
|
||
|
|
match client.chat_with_tools_non_streaming(&messages, None, Some(1024), Some(0.3), None) {
|
||
|
|
Ok((response, _usage)) => {
|
||
|
|
let text = response.content.unwrap_or_default().trim().to_string();
|
||
|
|
|
||
|
|
if text.contains("NO_ISSUES_FOUND") {
|
||
|
|
return Ok(Vec::new());
|
||
|
|
}
|
||
|
|
|
||
|
|
// Parse findings line by line
|
||
|
|
let findings: Vec<String> = text
|
||
|
|
.lines()
|
||
|
|
.map(|l| l.trim().to_string())
|
||
|
|
.filter(|l| {
|
||
|
|
!l.is_empty()
|
||
|
|
&& !l.starts_with("Here")
|
||
|
|
&& !l.starts_with("Let me")
|
||
|
|
&& !l.starts_with("I've")
|
||
|
|
&& !l.starts_with("The diff")
|
||
|
|
})
|
||
|
|
.collect();
|
||
|
|
|
||
|
|
Ok(findings)
|
||
|
|
}
|
||
|
|
Err(e) => {
|
||
|
|
warn!(error = %e, "auto-review LLM call failed");
|
||
|
|
Ok(Vec::new())
|
||
|
|
}
|
||
|
|
}
|
||
|
|
}
|
||
|
|
|
||
|
|
/// Perform a simple heuristic-based review without an LLM call.
|
||
|
|
///
|
||
|
|
/// This is a fallback when no LLM client is available. It checks for:
|
||
|
|
/// - Missing semicolons
|
||
|
|
/// - Unclosed brackets
|
||
|
|
/// - `todo!()` or `unimplemented!()` macros left in code
|
||
|
|
/// - Debug print statements
|
||
|
|
/// - Extremely long functions
|
||
|
|
fn perform_heuristic_review(diff: &str) -> Vec<String> {
|
||
|
|
let mut findings = Vec::new();
|
||
|
|
|
||
|
|
// Check for added lines (lines starting with +)
|
||
|
|
let added_lines: Vec<&str> = diff
|
||
|
|
.lines()
|
||
|
|
.filter(|l| l.starts_with('+') && !l.starts_with("+++"))
|
||
|
|
.collect();
|
||
|
|
|
||
|
|
let added_content: String = added_lines
|
||
|
|
.iter()
|
||
|
|
.map(|l| &l[1..]) // Strip leading +
|
||
|
|
.collect::<Vec<&str>>()
|
||
|
|
.join("\n");
|
||
|
|
|
||
|
|
// Check for todo! and unimplemented!
|
||
|
|
if added_content.contains("todo!()") {
|
||
|
|
findings.push("[MEDIUM] `todo!()` found in new code — replace with implementation".to_string());
|
||
|
|
}
|
||
|
|
if added_content.contains("unimplemented!()") {
|
||
|
|
findings.push("[MEDIUM] `unimplemented!()` found in new code — replace with implementation".to_string());
|
||
|
|
}
|
||
|
|
|
||
|
|
// Check for debug print statements
|
||
|
|
if added_content.contains("println!") || added_content.contains("dbg!") {
|
||
|
|
findings.push("[LOW] Debug print statements (println!/dbg!) found — consider removing before finalizing".to_string());
|
||
|
|
}
|
||
|
|
if added_content.contains("eprintln!") {
|
||
|
|
findings.push("[LOW] Debug eprintln! found — consider removing before finalizing".to_string());
|
||
|
|
}
|
||
|
|
|
||
|
|
// Check for unreachable or panic statements
|
||
|
|
if added_content.contains("panic!(\"reached") || added_content.contains("panic!(\"not implemented") {
|
||
|
|
findings.push("[HIGH] Unreachable code / panic found — implement the missing logic".to_string());
|
||
|
|
}
|
||
|
|
|
||
|
|
// Check for very long lines (>120 chars)
|
||
|
|
for (i, line) in added_lines.iter().enumerate() {
|
||
|
|
let content = &line[1..]; // Strip leading +
|
||
|
|
if content.len() > 120 && !content.trim_start().starts_with("//") {
|
||
|
|
let preview: String = content.chars().take(80).collect();
|
||
|
|
findings.push(format!(
|
||
|
|
"[LOW] Very long line ({} chars, line {} in diff) — consider breaking up:\n `{}…`",
|
||
|
|
content.len(),
|
||
|
|
i + 1,
|
||
|
|
preview
|
||
|
|
));
|
||
|
|
}
|
||
|
|
}
|
||
|
|
|
||
|
|
// Count new functions to detect very long additions
|
||
|
|
let fn_count = added_content.matches("fn ").count();
|
||
|
|
if fn_count > 5 {
|
||
|
|
findings.push("[INFO] Large number of new functions ({fn_count}) — consider whether this should be split into separate modules".to_string());
|
||
|
|
}
|
||
|
|
|
||
|
|
findings
|
||
|
|
}
|