From c5723140658da2211afcf9d0d06184dd7ab43104 Mon Sep 17 00:00:00 2001 From: Aji Anaz Date: Tue, 9 Jun 2026 22:40:11 +0700 Subject: [PATCH 1/3] feat(review): integrate auto-chunker into review command (#228) - Add --no-auto-chunk flag (auto-chunk enabled by default for large diffs) - Add execute_chunked_review() that splits diffs via chunker::chunk_diff() - Review each chunk independently with full pipeline (cache, stream, rules) - Merge findings and deduplicate by (file, line, title) tuple - Accumulate token usage across chunks for accurate reporting - Graceful partial results on chunk failure (warns, continues) - Progress events emitted per chunk when --progress is set - Quality gate evaluated on merged results - Fix pre-existing borrow lifetime issues in chunker tests - Register chunker module in engine/mod.rs Closes #228 --- src/commands/review.rs | 249 ++++++++++++++++++++++++++++++++++++++++- src/engine/chunker.rs | 28 ++--- src/engine/mod.rs | 1 + src/main.rs | 8 ++ 4 files changed, 270 insertions(+), 16 deletions(-) diff --git a/src/commands/review.rs b/src/commands/review.rs index 7f55b76..89e6c72 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -4,7 +4,9 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::Severity; +use crate::engine::chunker; use crate::engine::quality_gate; +use crate::engine::types::ReviewResponse; use crate::formatters::{OutputFormat, formatter_for}; use crate::git; use crate::progress::{ProgressReporter, TokenInfo, diff_stats}; @@ -40,6 +42,8 @@ pub struct ReviewOptions { pub no_cache: bool, /// CI mode: skip diff size limit, hard gate on any findings. pub ci: bool, + /// Auto-chunk large diffs into review-sized pieces. + pub auto_chunk: bool, } /// Result of a review command execution. @@ -88,6 +92,13 @@ pub async fn execute_review( // 3. Validate size (skip in CI mode) let max_size = opts.max_diff_size.unwrap_or(config.hook.max_diff_size); if !opts.ci && diff.len() > max_size { + if opts.auto_chunk { + // Auto-chunk mode: split diff and review each chunk + return execute_chunked_review( + config, llm_config, opts, format, progress, &diff, max_size, + ) + .await; + } if config.hook.on_violation == "disallow" { // Return blocked result instead of error return Ok(ReviewResult { @@ -96,7 +107,7 @@ pub async fn execute_review( "{}\n", format!( "❌ Diff too large ({} chars, max {}). Commit blocked. \ - Use --base to review a specific branch, increase \ + Use --auto-chunk to review in pieces, --base to review a specific branch, increase \ hook.max_diff_size, or run: git commit --no-verify", diff.len(), max_size @@ -107,7 +118,7 @@ pub async fn execute_review( }); } anyhow::bail!( - "Diff too large ({} chars, max {}). Use --base to review a specific branch, or increase hook.max_diff_size.", + "Diff too large ({} chars, max {}). Use --auto-chunk to review in pieces, --base to review a specific branch, or increase hook.max_diff_size.", diff.len(), max_size ); @@ -280,3 +291,237 @@ fn get_diff(opts: &ReviewOptions, _config: &Config) -> Result { } } } + +/// Execute a chunked review for large diffs. +/// +/// Splits the diff into chunks, reviews each independently, then +/// merges and deduplicates findings. +#[allow(clippy::cast_possible_truncation)] +async fn execute_chunked_review( + config: &Config, + llm_config: &crate::engine::LLMConfig, + opts: &ReviewOptions, + format: OutputFormat, + progress: &ProgressReporter, + diff: &str, + max_size: usize, +) -> Result { + let chunks = chunker::chunk_diff(diff, max_size); + + if chunks.is_empty() { + let output = format!("{}\n", "No changes to review.".yellow()); + return Ok(ReviewResult { + exit_code: EXIT_OK, + output, + }); + } + + let total_chunks = chunks.len(); + if !opts.quiet { + eprintln!( + "{}", + format!( + "📦 Diff too large ({} chars), splitting into {} chunks…", + diff.len(), + total_chunks + ) + .cyan() + ); + } + + debug!( + total_chunks, + diff_len = diff.len(), + "auto-chunking large diff" + ); + + let mut all_issues: Vec = Vec::new(); + let mut summaries: Vec = Vec::new(); + let mut total_tokens = crate::engine::types::TokenUsage::default(); + let mut any_error = false; + + for chunk in &chunks { + if !opts.quiet { + eprintln!( + "{}", + format!( + " → Reviewing chunk {}/{} ({}, {} files)…", + chunk.index + 1, + chunk.total, + chunk.label, + chunk.file_count + ) + .dimmed() + ); + } + + if progress.is_enabled() { + progress.calling_llm(&llm_config.provider, &llm_config.model); + } + + let chunk_start = std::time::Instant::now(); + + match crate::engine::review::review_diff_with_cache( + config, + llm_config, + &chunk.diff, + opts.stream, + !opts.no_cache, + opts.quiet, + ) + .await + { + Ok(resp) => { + if progress.is_enabled() { + let duration_ms = chunk_start.elapsed().as_millis() as u64; + let tokens = resp + .tokens_used + .as_ref() + .map_or_else(TokenInfo::zero, TokenInfo::from_usage); + progress.llm_response(&tokens, duration_ms); + } + + // Accumulate tokens + if let Some(usage) = &resp.tokens_used { + total_tokens.input_tokens += usage.input_tokens; + total_tokens.output_tokens += usage.output_tokens; + total_tokens.estimated_cost_usd += usage.estimated_cost_usd; + } + + if !resp.summary.is_empty() { + summaries.push(format!( + "[Chunk {}/{} — {}] {}", + chunk.index + 1, + chunk.total, + chunk.label, + resp.summary + )); + } + + all_issues.extend(resp.issues); + } + Err(e) => { + if progress.is_enabled() { + progress.error(&e.to_string(), "chunked_review"); + } + any_error = true; + if !opts.quiet { + eprintln!( + " {}", + format!( + "⚠️ Chunk {}/{} failed: {}", + chunk.index + 1, + chunk.total, + e + ) + .yellow() + ); + } + } + } + } + + // Deduplicate findings by (file, line, title) + let before_dedup = all_issues.len(); + all_issues.sort_by(|a, b| { + a.file + .cmp(&b.file) + .then_with(|| a.line.cmp(&b.line)) + .then_with(|| a.title.cmp(&b.title)) + }); + all_issues.dedup_by(|a, b| a.file == b.file && a.line == b.line && a.title == b.title); + let deduped = before_dedup - all_issues.len(); + + if deduped > 0 && !opts.quiet { + eprintln!( + "{}", + format!(" ℹ️ Deduplicated {deduped} overlapping findings").dimmed() + ); + } + + // Build merged response + let merged_summary = if summaries.is_empty() { + if any_error { + "Review completed with partial results (some chunks failed).".to_string() + } else { + "No issues found across all chunks.".to_string() + } + } else { + summaries.join("\n\n") + }; + + let merged_response = ReviewResponse { + issues: all_issues, + summary: merged_summary, + tokens_used: Some(total_tokens), + should_block: false, + }; + + // 6. Filter by severity if specified + let min_severity = if let Some(ref sev) = opts.severity { + Severity::from_str_lossy(sev) + } else { + config.hook.min_severity_level() + }; + + let mut filtered_response = merged_response.clone(); + filtered_response + .issues + .retain(|i| i.severity <= min_severity); + + // 7. Format output + let formatter = formatter_for(format); + let mut output = formatter.format_review(&filtered_response)?; + + // Add chunking summary header if not quiet and format is pretty + if !opts.quiet && format == OutputFormat::Pretty { + let header = format!( + "📦 Reviewed in {} chunks ({} files total)\n", + total_chunks, + chunks.iter().map(|c| c.file_count).sum::() + ); + output = format!("{header}{output}"); + } + + // 8. Quality gate evaluation + let gate_result = if config.quality_gate.enabled { + let result = quality_gate::evaluate(&filtered_response.issues, &config.quality_gate); + output.push_str(&quality_gate::format_gate_output(&result)); + Some(result) + } else { + None + }; + + // 9. Return exit code + let exit_code = if gate_result + .as_ref() + .is_some_and(|g| g.status == quality_gate::GateStatus::Fail) + { + EXIT_BLOCKED + } else if opts.ci { + if !filtered_response.issues.is_empty() { + EXIT_BLOCKED + } else { + EXIT_OK + } + } else if merged_response.should_block && config.hook.mode == "block" { + EXIT_BLOCKED + } else { + EXIT_OK + }; + + // 10. Emit complete event + if progress.is_enabled() { + let tokens = filtered_response + .tokens_used + .as_ref() + .map_or_else(TokenInfo::zero, TokenInfo::from_usage); + progress.complete( + filtered_response.issues.len(), + exit_code == EXIT_BLOCKED, + &tokens, + ); + } + + Ok(ReviewResult { exit_code, output }) +} diff --git a/src/engine/chunker.rs b/src/engine/chunker.rs index ed9bab7..b0f6298 100644 --- a/src/engine/chunker.rs +++ b/src/engine/chunker.rs @@ -315,28 +315,28 @@ diff --git a/docs/README.md b/docs/README.md #[test] fn derive_label_single_file() { - let files = vec![&FileDiff { + let fd = FileDiff { dir_prefix: "src".to_string(), diff: String::new(), size: 0, - }]; + }; + let files = vec![&fd]; assert_eq!(derive_label(&files), "src/*"); } #[test] fn derive_label_multiple_dirs() { - let files = vec![ - &FileDiff { - dir_prefix: "src".to_string(), - diff: String::new(), - size: 0, - }, - &FileDiff { - dir_prefix: "docs".to_string(), - diff: String::new(), - size: 0, - }, - ]; + let fd1 = FileDiff { + dir_prefix: "src".to_string(), + diff: String::new(), + size: 0, + }; + let fd2 = FileDiff { + dir_prefix: "docs".to_string(), + diff: String::new(), + size: 0, + }; + let files = vec![&fd1, &fd2]; assert_eq!(derive_label(&files), "docs, src"); } } diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 6030edb..6a76365 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -1,5 +1,6 @@ pub mod bundling; pub mod cache; +pub mod chunker; pub mod context; pub mod diff_parser; pub mod llm; diff --git a/src/main.rs b/src/main.rs index d854ddf..e57ba1d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -109,6 +109,10 @@ enum Command { #[clap(long)] stream: bool, + /// Disable auto-chunking for large diffs (auto-chunk is enabled by default) + #[clap(long)] + no_auto_chunk: bool, + /// Output structured NDJSON progress events to stderr #[clap(long)] progress: bool, @@ -333,6 +337,7 @@ async fn main() -> Result<()> { unstaged, max_diff_size, stream, + no_auto_chunk, progress, quiet, output_file, @@ -355,6 +360,7 @@ async fn main() -> Result<()> { unstaged, max_diff_size, stream, + no_auto_chunk, progress, quiet, output_file, @@ -479,6 +485,7 @@ struct ReviewOpts { unstaged: bool, max_diff_size: Option, stream: bool, + no_auto_chunk: bool, progress: bool, quiet: bool, output_file: Option, @@ -542,6 +549,7 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { severity: opts.severity.clone(), no_cache: opts.no_cache, ci: opts.ci, + auto_chunk: !opts.no_auto_chunk, }; // When streaming and not quiet/progress, show a simpler message From abbe2ae3e330ab776a0644761d9aa65becda3e99 Mon Sep 17 00:00:00 2001 From: Aji Anaz Date: Tue, 9 Jun 2026 22:41:27 +0700 Subject: [PATCH 2/3] fix(review): track should_block across chunks and fail on all-chunk failure - Propagate should_block=true from any chunk to merged response - Return EXIT_BLOCKED when all chunks fail instead of silent pass - Addresses Cora self-review findings on chunked review flow --- src/commands/review.rs | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/commands/review.rs b/src/commands/review.rs index 89e6c72..307a846 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -339,6 +339,8 @@ async fn execute_chunked_review( let mut summaries: Vec = Vec::new(); let mut total_tokens = crate::engine::types::TokenUsage::default(); let mut any_error = false; + let mut any_success = false; + let mut should_block = false; for chunk in &chunks { if !opts.quiet { @@ -399,6 +401,10 @@ async fn execute_chunked_review( } all_issues.extend(resp.issues); + if resp.should_block { + should_block = true; + } + any_success = true; } Err(e) => { if progress.is_enabled() { @@ -439,6 +445,22 @@ async fn execute_chunked_review( ); } + // If all chunks failed, return an error instead of silently passing + if !any_success { + if progress.is_enabled() { + progress.error("All chunks failed during chunked review", "chunked_review"); + } + return Ok(ReviewResult { + exit_code: EXIT_BLOCKED, + output: format!( + "{}\n", + "❌ All review chunks failed. Check your API key and try again." + .red() + .bold() + ), + }); + } + // Build merged response let merged_summary = if summaries.is_empty() { if any_error { @@ -454,7 +476,7 @@ async fn execute_chunked_review( issues: all_issues, summary: merged_summary, tokens_used: Some(total_tokens), - should_block: false, + should_block, }; // 6. Filter by severity if specified From b66aa401d54c80c17529401363a7b383afa8574c Mon Sep 17 00:00:00 2001 From: Aji Anaz Date: Tue, 9 Jun 2026 22:50:24 +0700 Subject: [PATCH 3/3] style: apply cargo fmt --- src/engine/chunker.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/engine/chunker.rs b/src/engine/chunker.rs index b0f6298..7ccf15f 100644 --- a/src/engine/chunker.rs +++ b/src/engine/chunker.rs @@ -149,8 +149,7 @@ fn find_file_start(lines: &[&str], start_from: usize, path: &str) -> Option usize { for i in start_from..lines.len() { // A new file starts with "--- " followed by "+++ " - if lines[i].starts_with("--- ") && i + 1 < lines.len() && lines[i + 1].starts_with("+++ ") - { + if lines[i].starts_with("--- ") && i + 1 < lines.len() && lines[i + 1].starts_with("+++ ") { // Check it's not a hunk line that starts with "---" // File headers have "--- a/" pattern, hunks have "---1,5" pattern if lines[i].starts_with("--- a/") || lines[i].starts_with("--- /dev/null") {