From 10728ce6e98caca4f1fb8863000ad601c974d986 Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Wed, 17 Jun 2026 11:51:41 +0700 Subject: [PATCH] fix(scan): non-fatal batch errors + non-JSON response diagnostics (#316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add early non-JSON response guard in parse_scan_response: detect HTML error pages, rate-limit bodies, empty responses, and prose wrappers before attempting strict parse. Surface raw response prefix (first 512 bytes, whitespace-collapsed) in the error so users can diagnose truncation vs provider error vs prose. - Make per-batch parse failures non-fatal by default: skip the failing batch with a stderr warning and warn-level log (with file list), continue with remaining batches. --no-continue-on-batch-error restores old abort behavior. - Add --batch-files flag (default 20) to cap files per LLM batch — lower it to work around provider token limits on large scans. - Truncated-JSON repair path now also includes raw response prefix. - Add 14 unit tests covering looks_like_json_array, preview_raw, and parse_scan_response non-JSON rejection. - Bump version 0.6.0 → 0.6.1 (patch: bug fix, no API breaking changes). --- CHANGELOG.md | 14 ++++- Cargo.lock | 2 +- Cargo.toml | 2 +- src/commands/scan.rs | 77 ++++++++++++++++++++--- src/engine/llm.rs | 143 ++++++++++++++++++++++++++++++++++++++++++- src/main.rs | 18 ++++++ 6 files changed, 244 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b23e2e..b4f53a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.6.1] - 2026-06-17 + +### Fixed — Scan + +- **`cora scan` no longer aborts on non-JSON LLM responses (#316)** + - Detect non-JSON responses early (provider error pages, rate-limit bodies, empty responses, prose wrappers) and surface the raw response prefix (first 512 bytes) in the error message so users can diagnose the cause. + - Per-batch parse failures are now **non-fatal by default**: the failing batch is skipped with a `warn`-level log and a stderr warning listing the affected files, and the scan continues with the remaining batches. Set `--no-continue-on-batch-error` to restore the old abort behavior. + - Added `--batch-files ` flag (default: 20) to control the maximum number of files per LLM batch — lower it to work around provider token limits or rate-limit errors on large scans. + - Truncated-JSON and general parse errors now include the raw response prefix for easier debugging without `--verbose`. + ## [0.6.0] - 2026-06-14 ### Added — Code Intelligence @@ -502,7 +512,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Cross-platform** — Linux (x86_64, ARM64), macOS (Apple Silicon), Windows (x86_64) - **MIT License** — fully open source -[Unreleased]: https://github.com/codecoradev/cora-cli/compare/v0.5.0...develop +[Unreleased]: https://github.com/codecoradev/cora-cli/compare/v0.6.1...develop +[0.6.1]: https://github.com/codecoradev/cora-cli/compare/v0.6.0...v0.6.1 +[0.6.0]: https://github.com/codecoradev/cora-cli/compare/v0.5.0...v0.6.0 [0.5.0]: https://github.com/codecoradev/cora-cli/compare/v0.4.6...v0.5.0 [0.4.6]: https://github.com/codecoradev/cora-cli/compare/v0.4.5...v0.4.6 [0.4.5]: https://github.com/codecoradev/cora-cli/compare/v0.4.4...v0.4.5 diff --git a/Cargo.lock b/Cargo.lock index f1b00de..9295742 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -275,7 +275,7 @@ dependencies = [ [[package]] name = "cora-cli" -version = "0.6.0" +version = "0.6.1" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index fb3c6e9..44056cb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cora-cli" -version = "0.6.0" +version = "0.6.1" edition = "2024" description = "CLI-first AI code review — BYOK, diff/scan/branch, pre-commit hooks" license = "MIT" diff --git a/src/commands/scan.rs b/src/commands/scan.rs index c153538..95de3e8 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -22,6 +22,12 @@ pub struct ScanOptions { pub incremental: bool, /// Focus areas for review (overrides config). pub focus: Vec, + /// Maximum files per LLM batch (0 = use default 20). + pub batch_files: usize, + /// Whether to continue scanning when a batch fails to parse. + /// When true (default), a failed batch is skipped with a warning and the + /// rest of the scan continues. When false, a failed batch aborts the run. + pub continue_on_batch_error: bool, } /// Execute the scan command. @@ -99,12 +105,22 @@ pub async fn execute_scan( let total_lines: usize = files.iter().map(|f| f.lines).sum(); // 3. Batch files - let batches = batch_files(&files, 60_000, 20); - debug!(batches = batches.len(), "batched files"); + let max_files_per_batch = if opts.batch_files > 0 { + opts.batch_files + } else { + 20 + }; + let batches = batch_files(&files, 60_000, max_files_per_batch); + debug!( + batches = batches.len(), + max_files = max_files_per_batch, + "batched files" + ); // 4. Process batches and collect issues let mut all_issues = Vec::new(); let mut total_tokens = None; + let mut skipped_batches: Vec<(usize, Vec, String)> = Vec::new(); for (batch_idx, batch) in batches.iter().enumerate() { let files_content = format_batch_for_prompt(batch); @@ -116,7 +132,7 @@ pub async fn execute_scan( println!(" Reviewing{batch_label}…"); - let (issues, _summary, tokens) = crate::engine::llm::scan_files( + match crate::engine::llm::scan_files( llm_config, &files_content, &effective_focus, @@ -124,14 +140,61 @@ pub async fn execute_scan( &config.response_format, None, ) - .await?; + .await + { + Ok((issues, _summary, tokens)) => { + all_issues.extend(issues); + if tokens.is_some() { + total_tokens = tokens; + } + } + Err(err) => { + let file_list: Vec = + batch.iter().map(|f| f.path.clone()).collect::>(); + let err_string = err.to_string(); + + // Always log the failure at warn level so it shows even without --verbose. + tracing::warn!( + batch = batch_idx + 1, + total_batches = batches.len(), + files = ?file_list, + error = %err_string, + "batch scan failed" + ); - all_issues.extend(issues); - if tokens.is_some() { - total_tokens = tokens; + if !opts.continue_on_batch_error { + eprintln!( + " {} batch {}/{}: {}", + "failed".red().bold(), + batch_idx + 1, + batches.len(), + err_string + ); + return Err(err.into()); + } + + eprintln!( + " {} batch {}/{} — skipping ({} files): {}", + "warn".yellow().bold(), + batch_idx + 1, + batches.len(), + file_list.len(), + err_string + ); + skipped_batches.push((batch_idx + 1, file_list, err_string)); + } } } + if !skipped_batches.is_empty() { + eprintln!( + " {} {} of {} batches skipped due to parse failures.", + skipped_batches.len().to_string().yellow(), + skipped_batches.len(), + batches.len() + ); + } + // 5. Build response and format let issue_count = all_issues.len(); let min_severity = config.hook.min_severity_level(); diff --git a/src/engine/llm.rs b/src/engine/llm.rs index d2b1fa6..e6ec431 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -720,6 +720,14 @@ pub(crate) fn parse_review_response( pub(crate) fn parse_scan_response( raw: &str, ) -> std::result::Result<(Vec, Option, Option), CoraError> { + // Fast-fail when the response is clearly not JSON (e.g. provider error page, + // empty body, rate-limit message, or prose wrapper). Surfacing the raw + // prefix lets users diagnose whether it's truncation, a provider error, + // or HTML. + if !looks_like_json_array(raw) { + return Err(CoraError::LlmParse(non_json_error_message(raw))); + } + let (json_str, summary) = extract_json_and_summary(raw); let json_str = strip_code_fences(&json_str); @@ -741,12 +749,16 @@ pub(crate) fn parse_scan_response( } Err(repair_err) => { return Err(CoraError::LlmParse(format!( - "parse failed (original: {err_msg}, after repair: {repair_err})" + "parse failed (original: {err_msg}, after repair: {repair_err}). Raw response prefix: {}", + preview_raw(raw) ))); } } } else { - return Err(CoraError::LlmParse(e.to_string())); + return Err(CoraError::LlmParse(format!( + "{err_msg}. Raw response prefix: {}", + preview_raw(raw) + ))); } } }; @@ -760,6 +772,53 @@ pub(crate) fn parse_scan_response( Ok((issues, summary, None)) } +/// Check whether a raw LLM response plausibly contains a JSON payload. +/// +/// Accepts responses that (after trimming leading whitespace and optional +/// markdown fences) begin with `[` or `{`. Rejects obvious non-JSON bodies +/// such as HTML error pages, empty strings, or pure prose. +pub(crate) fn looks_like_json_array(raw: &str) -> bool { + let trimmed = raw.trim_start(); + if trimmed.is_empty() { + return false; + } + // Strip a leading ```json or ``` fence if present + let stripped = trimmed + .strip_prefix("```json") + .or_else(|| trimmed.strip_prefix("```")) + .map(str::trim_start) + .unwrap_or(trimmed); + matches!(stripped.chars().next(), Some('[') | Some('{')) +} + +/// Build a human-readable diagnostic for a non-JSON LLM response, including a +/// truncated preview of the raw body (first 512 bytes) so users can tell +/// whether the provider returned an error page, rate-limit message, or prose. +pub(crate) fn non_json_error_message(raw: &str) -> String { + let len = raw.len(); + format!( + "LLM response is not valid JSON (length={len}). This usually means the provider returned an error body, rate-limit page, or truncated output. Raw response prefix: {}", + preview_raw(raw) + ) +} + +/// Return a single-line, length-capped preview of a raw LLM response for logs +/// and error messages. Collapses whitespace and caps at 512 bytes. +pub(crate) fn preview_raw(raw: &str) -> String { + const MAX_BYTES: usize = 512; + let collapsed: String = raw.split_whitespace().collect::>().join(" "); + if collapsed.len() <= MAX_BYTES { + collapsed + } else { + // Split at a char boundary <= MAX_BYTES to avoid slicing mid-codepoint. + let mut end = MAX_BYTES; + while end > 0 && !collapsed.is_char_boundary(end) { + end -= 1; + } + format!("{}… [truncated]", &collapsed[..end]) + } +} + /// Extract JSON and optional summary (after ||| separator). fn extract_json_and_summary(raw: &str) -> (String, String) { if let Some(idx) = raw.find("|||") { @@ -1423,4 +1482,84 @@ mod tests { assert_eq!(result.0.len(), 1); assert_eq!(result.0[0].file, "config.rs"); } + + // ─── looks_like_json_array / non-JSON guard (#316) ─── + + #[test] + fn looks_like_json_array_accepts_plain_array() { + assert!(looks_like_json_array(EMPTY_ARRAY)); + assert!(looks_like_json_array(SINGLE_ISSUE_JSON)); + } + + #[test] + fn looks_like_json_array_accepts_fenced_json() { + let fenced = format!("```json\n{SINGLE_ISSUE_JSON}\n```"); + assert!(looks_like_json_array(&fenced)); + let plain_fence = format!("```\n{EMPTY_ARRAY}\n```"); + assert!(looks_like_json_array(&plain_fence)); + } + + #[test] + fn looks_like_json_array_accepts_leading_whitespace() { + let padded = format!("\n \t {SINGLE_ISSUE_JSON}"); + assert!(looks_like_json_array(&padded)); + } + + #[test] + fn looks_like_json_array_rejects_empty() { + assert!(!looks_like_json_array("")); + assert!(!looks_like_json_array(" \n\t\n")); + } + + #[test] + fn looks_like_json_array_rejects_html_error_page() { + let html = "

503 Service Unavailable

"; + assert!(!looks_like_json_array(html)); + } + + #[test] + fn looks_like_json_array_rejects_prose() { + let prose = "Sure, here are the issues I found in your code: first, ..."; + assert!(!looks_like_json_array(prose)); + } + + #[test] + fn parse_scan_response_rejects_non_json_with_preview() { + let html = "Rate limited"; + let err = parse_scan_response(html).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("not valid JSON"), "msg = {msg}"); + assert!(msg.contains("Rate limited"), "msg = {msg}"); + assert!(msg.contains("length="), "msg = {msg}"); + } + + #[test] + fn parse_scan_response_rejects_empty_body() { + let err = parse_scan_response("").unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("not valid JSON"), "msg = {msg}"); + assert!(msg.contains("length=0"), "msg = {msg}"); + } + + #[test] + fn preview_raw_is_truncated_to_max_bytes() { + // 2000-char prose should be collapsed and capped at 512 bytes. + let long = "word ".repeat(500); + let preview = preview_raw(&long); + assert!(preview.ends_with("… [truncated]")); + // Hard cap (512 + suffix length). + assert!(preview.len() < 600); + } + + #[test] + fn preview_raw_preserves_short_input() { + let short = "hello world"; + assert_eq!(preview_raw(short), short); + } + + #[test] + fn preview_raw_collapses_whitespace() { + let messy = "hello\n\t world\n\n"; + assert_eq!(preview_raw(messy), "hello world"); + } } diff --git a/src/main.rs b/src/main.rs index d7963df..3190f8e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -316,6 +316,16 @@ enum Command { /// Focus areas for review (overrides config) #[clap(long)] focus: Vec, + + /// Maximum files per LLM batch (default: 20). Lower this to work around + /// provider token limits or rate-limit errors on large scans. + #[clap(long, value_name = "N", default_value_t = 20)] + batch_files: usize, + + /// Abort the entire scan when a batch fails to parse instead of + /// skipping it and continuing (default: skip and continue). + #[clap(long)] + no_continue_on_batch_error: bool, }, /// Manage quality profiles (preset rule sets) @@ -991,6 +1001,8 @@ async fn main() -> Result<()> { extensions, incremental, focus, + batch_files, + no_continue_on_batch_error, } => { cmd_scan( &cli.global, @@ -1001,6 +1013,8 @@ async fn main() -> Result<()> { extensions, incremental, focus, + batch_files, + continue_on_batch_error: !no_continue_on_batch_error, }, ) .await? @@ -1147,6 +1161,8 @@ struct ScanOpts { extensions: Vec, incremental: bool, focus: Vec, + batch_files: usize, + continue_on_batch_error: bool, } /// Handle the `review` subcommand. @@ -1375,6 +1391,8 @@ async fn cmd_scan(globals: &GlobalOptions, opts: ScanOpts) -> Result { extensions: opts.extensions, incremental: opts.incremental, focus: opts.focus, + batch_files: opts.batch_files, + continue_on_batch_error: opts.continue_on_batch_error, }; scan::execute_scan(&config, &llm_config, &scan_opts, format).await