From 1c8bccb583d311bc3be24ffc44b2c1f149f1879c Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 11:44:38 +0700 Subject: [PATCH] fix(scan): run the same deterministic family as review; scan judge seam (#645) deterministic::run_files serves whole files (scan, MCP snippet check) the way run serves diffs, so built-in and custom rules now fire in scan. ScanJudge (HttpScanJudge, NoJudge) makes the batch loop testable without a network. DEFAULT_BATCH_TOKEN_BUDGET is renamed to CHAR_BUDGET and the docs state which review flags scan lacks. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 4 + docs/cli-reference.md | 2 + src/commands/scan.rs | 318 +++++++++++++++++++++++++----------- src/engine/deterministic.rs | 22 +++ src/engine/judge.rs | 63 ++++++- src/engine/postprocess.rs | 18 +- src/mcp/tools.rs | 2 +- 7 files changed, 317 insertions(+), 112 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a18c236..4161d52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **One `finalize` module owns the result tail of review, scan, commit and MCP (#648).** `engine::finalize` (`shape`, `shape_issues`, `blocks`, `exit_code`, `save_debt`, `save_history`) replaces six hand-copied versions of severity filtering, `should_block`, quality gate, exit code, debt snapshot and review history. Behavior of `cora review` (single and chunked) and `cora scan` is unchanged. Fixed along the way: the MCP `cora.review_diff` tool now honors its `min_severity` parameter and evaluates the quality gate on the reported findings; `cora commit` now records its review in the review history (`command = "commit"`). +### Fixed + +- **`cora scan` now runs the rules engine (#645).** Built-in rules (`bug-todo`, `sec-sql-concat`, ...) and custom `rules_engine.custom_rules` were only run by `cora review`; scan silently skipped them. One set of deterministic families (`deterministic::run_files` for whole files, `deterministic::run` for diffs) now serves both, and the MCP `cora.check_snippet` tool picks up rules too. Scan also has a judge seam (`judge::ScanJudge`, with `HttpScanJudge` and `NoJudge`) so the batch loop is tested without a network, and `DEFAULT_BATCH_TOKEN_BUDGET` is renamed `DEFAULT_BATCH_CHAR_BUDGET` (it was always a character count). `docs/cli-reference.md` now states which review flags scan does not have. + ## [0.18.0] - 2026-10-09 ### Security diff --git a/docs/cli-reference.md b/docs/cli-reference.md index a731a2a..322cc9a 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -90,6 +90,8 @@ Complete command reference for the cora CLI. | `cora scan --batch-files` `N` | Max files per LLM batch (default: 20) | | `cora scan --no-continue-on-batch-error` | Abort on batch failure (default: skip and continue) | +`cora scan` runs the same deterministic checks as `cora review` (built-in and custom rules, secrets, security patterns, plus unused imports and dead code from the index) on every file, then asks the LLM about each batch. Findings are filtered by `ignore.rules` and inline `cora-ignore:` markers and capped by `rules_engine.max_findings`. Unlike `review`, scan has no `--ci`, `--severity`, `--quiet`, `--stream`, `--output-file` or `--no-cache`, does not apply the quality gate, and keeps every finding in the output (`hook.min_severity` only decides the exit code in `block` mode). + ### Code Intelligence See [Code Intelligence](./code-intelligence) for detailed usage. diff --git a/src/commands/scan.rs b/src/commands/scan.rs index 3bfb492..5239b1a 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -7,7 +7,7 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::ReviewIssue; use crate::engine::finalize::{self, HistoryEntry, ShapeOptions}; -use crate::engine::judge::admit; +use crate::engine::judge::{HttpScanJudge, ScanBatch, ScanJudge, admit}; use crate::engine::postprocess::{Source, dropped_warning, postprocess_report}; use crate::engine::scanner::{FileEntry, batch_files, format_batch_for_prompt, walk_project}; use crate::engine::types::TokenUsage; @@ -17,9 +17,10 @@ use crate::formatters::{OutputFormat, formatter_for}; /// Lower this to work around provider token limits or rate-limit errors. const DEFAULT_MAX_FILES_PER_BATCH: usize = 20; -/// Approximate token budget per batch. Used by the scanner to split large file -/// sets into review-sized chunks that fit within typical model context windows. -const DEFAULT_BATCH_TOKEN_BUDGET: usize = 60_000; +/// Character budget per batch (roughly 4 characters per token). Used by the +/// scanner to split large file sets into review-sized chunks that fit within +/// typical model context windows. +const DEFAULT_BATCH_CHAR_BUDGET: usize = 60_000; /// Scan command options. pub struct ScanOptions { @@ -145,7 +146,7 @@ pub async fn execute_scan( } else { DEFAULT_MAX_FILES_PER_BATCH }; - let batches = batch_files(&files, DEFAULT_BATCH_TOKEN_BUDGET, max_files_per_batch); + let batches = batch_files(&files, DEFAULT_BATCH_CHAR_BUDGET, max_files_per_batch); debug!( batches = batches.len(), max_files = max_files_per_batch, @@ -165,97 +166,20 @@ pub async fn execute_scan( None }; - // 5. Process batches and collect issues - let mut all_issues = Vec::new(); - let mut total_tokens: Option = 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); - let batch_label = if batches.len() > 1 { - format!(" (batch {}/{})", batch_idx + 1, batches.len()) - } else { - String::new() - }; - - eprintln!(" Reviewing{batch_label}…"); - - let spinner = crate::progress::Spinner::new("Scanning files…", false); - let scanned = crate::engine::llm::scan_files( - llm_config, - &files_content, - &effective_focus, - &config.rules, - &config.response_format, - None, - brain_ctx.as_deref(), - &spinner, - ) - .await; - spinner.finish(); - match scanned { - Ok((issues, _summary, tokens)) => { - // The model's answer is untrusted: only files of this batch, - // bounded text, no deterministic markers (#644). - let batch_paths: Vec = batch.iter().map(|f| f.path.clone()).collect(); - all_issues.extend(admit(issues, &batch_paths, &Source::Files(batch))); - total_tokens = match (total_tokens, tokens) { - (Some(mut acc), Some(t)) => { - acc.input_tokens += t.input_tokens; - acc.output_tokens += t.output_tokens; - acc.estimated_cost_usd += t.estimated_cost_usd; - Some(acc) - } - (None, Some(t)) => Some(t), - (acc, None) => acc, - }; - } - 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" - ); - - 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. Judge the batches and collect issues + let judge = HttpScanJudge { llm_config }; + let BatchRun { + issues: mut all_issues, + tokens: total_tokens, + } = judge_batches( + &judge, + config, + &batches, + &effective_focus, + brain_ctx.as_deref(), + opts.continue_on_batch_error, + ) + .await?; // 5. Merge deterministic + index findings with the LLM findings, then // apply ignore.rules and inline cora-ignore markers. Deterministic @@ -335,7 +259,109 @@ pub async fn execute_scan( Ok(shaped.exit_code) } -/// Run the deterministic scanners (secrets, security) over whole files and +/// Findings and token usage gathered from all scan batches. +struct BatchRun { + issues: Vec, + tokens: Option, +} + +/// Ask `judge` about every batch. A batch that fails is skipped with a warning +/// (`continue_on_error`) or aborts the scan. Every batch's answer passes +/// [`admit`] first: it is untrusted (#644). +async fn judge_batches( + judge: &J, + config: &Config, + batches: &[Vec], + focus: &[String], + brain_context: Option<&str>, + continue_on_error: bool, +) -> Result { + let mut all_issues = Vec::new(); + let mut total_tokens: Option = 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); + let batch_label = if batches.len() > 1 { + format!(" (batch {}/{})", batch_idx + 1, batches.len()) + } else { + String::new() + }; + + eprintln!(" Reviewing{batch_label}…"); + + match judge + .judge_batch(config, &files_content, focus, brain_context) + .await + { + Ok(ScanBatch { issues, tokens }) => { + // Only files of this batch, bounded text, no deterministic markers. + let batch_paths: Vec = batch.iter().map(|f| f.path.clone()).collect(); + all_issues.extend(admit(issues, &batch_paths, &Source::Files(batch))); + total_tokens = match (total_tokens, tokens) { + (Some(mut acc), Some(t)) => { + acc.input_tokens += t.input_tokens; + acc.output_tokens += t.output_tokens; + acc.estimated_cost_usd += t.estimated_cost_usd; + Some(acc) + } + (None, Some(t)) => Some(t), + (acc, None) => acc, + }; + } + 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" + ); + + if !continue_on_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() + ); + } + + Ok(BatchRun { + issues: all_issues, + tokens: total_tokens, + }) +} + +/// Run the deterministic scanners (rules, secrets, security) over whole files and /// merge them with `issues` (LLM + index findings); then drop findings matched /// by `ignore.rules` and by inline `cora-ignore:` markers. Reuses the review /// pipeline's scanners, merge, and filters (#595). @@ -501,4 +527,102 @@ mod tests { .unwrap(); assert_eq!(code, 2, "deterministic finding must survive LLM failure"); } + + // ─── #645: scan runs the rules engine, and has a judge seam ─── + + #[test] + fn builtin_rules_fire_in_scan() { + let file = entry("src/lib.rs", "fn main() {\n // TODO: handle this\n}\n"); + let out = finalize_issues(&Config::default(), &[file], Vec::new()).0; + assert!( + out.iter().any(|i| i.rule_id.as_deref() == Some("bug-todo")), + "rules engine must run in scan, got {:?}", + titles(&out) + ); + } + + #[test] + fn disabled_rules_engine_stays_off_in_scan() { + let mut config = Config::default(); + config.rules_config.enabled = false; + let file = entry("src/lib.rs", "// TODO: later\n"); + let out = finalize_issues(&config, &[file], Vec::new()).0; + assert!(!out.iter().any(|i| i.rule_id.as_deref() == Some("bug-todo"))); + } + + struct FakeScanJudge(Result, String>); + + impl ScanJudge for FakeScanJudge { + async fn judge_batch( + &self, + _config: &Config, + _files_content: &str, + _focus: &[String], + _brain_context: Option<&str>, + ) -> std::result::Result { + match &self.0 { + Ok(issues) => Ok(ScanBatch { + issues: issues.clone(), + tokens: None, + }), + Err(msg) => Err(crate::error::CoraError::LlmParse(msg.clone())), + } + } + } + + fn judged(file: &str, line: u32) -> ReviewIssue { + ReviewIssue::new( + file, + Some(line), + crate::engine::Severity::Major, + "judge finding", + ) + .with_type("bug") + .with_body("body") + } + + fn one_batch() -> Vec> { + vec![vec![entry("a.rs", "fn a() {}\nfn b() {}\n")]] + } + + #[tokio::test] + async fn judge_findings_for_the_batch_are_admitted() { + let judge = FakeScanJudge(Ok(vec![judged("a.rs", 1), judged("elsewhere.rs", 1)])); + let run = judge_batches(&judge, &Config::default(), &one_batch(), &[], None, true) + .await + .unwrap(); + assert_eq!(run.issues.len(), 1, "foreign file must be dropped"); + assert_eq!(run.issues[0].file, "a.rs"); + } + + #[tokio::test] + async fn failed_batch_is_skipped_when_continuing() { + let judge = FakeScanJudge(Err("bad json".to_string())); + let run = judge_batches(&judge, &Config::default(), &one_batch(), &[], None, true) + .await + .unwrap(); + assert!(run.issues.is_empty()); + } + + #[tokio::test] + async fn failed_batch_aborts_without_continue() { + let judge = FakeScanJudge(Err("bad json".to_string())); + let err = judge_batches(&judge, &Config::default(), &one_batch(), &[], None, false).await; + assert!(err.is_err()); + } + + #[tokio::test] + async fn no_judge_scans_deterministically_only() { + let run = judge_batches( + &crate::engine::judge::NoJudge, + &Config::default(), + &one_batch(), + &[], + None, + true, + ) + .await + .unwrap(); + assert!(run.issues.is_empty()); + } } diff --git a/src/engine/deterministic.rs b/src/engine/deterministic.rs index 90bfab1..ecccae6 100644 --- a/src/engine/deterministic.rs +++ b/src/engine/deterministic.rs @@ -69,6 +69,28 @@ pub fn run(chunks: &[FileChunk], config: &Config, bridge: &IndexBridge) -> Deter } } +/// Run the deterministic checks that need no index over whole files (`cora +/// scan`, MCP snippet check): rules, secrets, security. Every line counts as +/// added. The index-backed families (unused imports, dead code) come from +/// `index_scanner::scan_project_index`, which needs a project bridge and is +/// merged by the caller. +pub fn run_files( + files: &[crate::engine::scanner::FileEntry], + config: &Config, +) -> DeterministicReport { + let scan_files = crate::engine::scan_input::from_entries(files); + let uncapped_rules = crate::engine::rules::types::RulesConfig { + max_findings: 0, + ..config.rules_config.clone() + }; + DeterministicReport { + rules: rules::run_rules_in(&scan_files, &uncapped_rules), + secrets: secrets_scanner::scan_secrets_in(&scan_files, usize::MAX), + security: security_scanner::scan_security_in(&scan_files, usize::MAX), + ..DeterministicReport::default() + } +} + impl DeterministicReport { fn families(&self) -> [&Vec; 6] { [ diff --git a/src/engine/judge.rs b/src/engine/judge.rs index d22694a..c55a8cd 100644 --- a/src/engine/judge.rs +++ b/src/engine/judge.rs @@ -31,7 +31,7 @@ use crate::engine::deterministic::DeterministicReport; use crate::engine::diff_parser::{DiffLineType, FileChunk}; use crate::engine::llm::{self, ReviewPrompts}; use crate::engine::postprocess::Source; -use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse}; +use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, TokenUsage}; use crate::error::CoraError; /// Everything about one review that does not depend on who judges it. @@ -135,6 +135,55 @@ impl Judge for HttpJudge<'_> { } } +/// What a judge returned for one scan batch. +#[derive(Debug, Default)] +pub struct ScanBatch { + pub issues: Vec, + pub tokens: Option, +} + +/// Something that turns a batch of numbered file contents into findings +/// (`cora scan`). The same seam as [`Judge`], for whole files. +pub trait ScanJudge { + async fn judge_batch( + &self, + config: &Config, + files_content: &str, + focus: &[String], + brain_context: Option<&str>, + ) -> Result; +} + +/// Scan judge backed by an OpenAI-compatible LLM. +pub struct HttpScanJudge<'a> { + pub llm_config: &'a LLMConfig, +} + +impl ScanJudge for HttpScanJudge<'_> { + async fn judge_batch( + &self, + config: &Config, + files_content: &str, + focus: &[String], + brain_context: Option<&str>, + ) -> Result { + let spinner = crate::progress::Spinner::new("Scanning files…", false); + let result = llm::scan_files( + self.llm_config, + files_content, + focus, + &config.rules, + &config.response_format, + None, + brain_context, + &spinner, + ) + .await; + spinner.finish(); + result.map(|(issues, _summary, tokens)| ScanBatch { issues, tokens }) + } +} + /// Judge that finds nothing: the review is the deterministic findings only. // Selected by `cora review --no-llm` / a missing API key (#646). #[allow(dead_code)] @@ -152,6 +201,18 @@ impl Judge for NoJudge { } } +impl ScanJudge for NoJudge { + async fn judge_batch( + &self, + _config: &Config, + _files_content: &str, + _focus: &[String], + _brain_context: Option<&str>, + ) -> Result { + Ok(ScanBatch::default()) + } +} + // ─── Admission of untrusted judge output ─── /// Most findings admitted from one judge answer; the most severe are kept. diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs index 22e620b..bd38149 100644 --- a/src/engine/postprocess.rs +++ b/src/engine/postprocess.rs @@ -18,7 +18,7 @@ //! //! | # | Filter | Diff | Files | //! |---|-----------------------------------|--------------------|-------| -//! | 1 | scanner merge (secrets, security) | no (caller merges) | yes | +//! | 1 | scanner merge (rules, secrets, security) | no (caller merges) | yes | //! | 2 | LLM hardcoded-secret FP check | yes | no | //! | 3 | Markdown fenced code block | yes | no | //! | 4 | `ignore.rules` | yes | yes | @@ -42,7 +42,7 @@ use crate::engine::diff_parser::FileChunk; use crate::engine::scan_input; use crate::engine::scanner::FileEntry; use crate::engine::types::{ReviewIssue, Severity}; -use crate::engine::{inline_suppress, rules, secrets_scanner, security_scanner}; +use crate::engine::{deterministic, inline_suppress}; /// What the findings being post-processed were produced from. pub enum Source<'a> { @@ -92,18 +92,10 @@ pub fn postprocess_report( apply_context_line_filter(issues, chunks) } Source::Files(files) => { + // Rules, secrets and security run uncapped; the cap is enforced + // below, after suppression (#624). + let merged = deterministic::run_files(files, config).merge_into(issues); let scan_files = scan_input::from_entries(files); - // Scanners run uncapped; the cap is enforced below, after - // suppression (#624). - let mut merged = issues; - for family in [ - secrets_scanner::scan_secrets_in(&scan_files, usize::MAX), - security_scanner::scan_security_in(&scan_files, usize::MAX), - ] { - if !family.is_empty() { - merged = rules::merge_rule_findings(merged, family); - } - } let merged = apply_ignore_rules(merged, &config.ignore.rules); inline_suppress::apply_lines(merged, &scan_files) } diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 9a1ef3c..0c2e787 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -31,7 +31,7 @@ pub fn list_tools() -> Vec { }, Tool { name: "cora.check_snippet".to_string(), - description: "Check a code snippet against cora's deterministic rules (secrets, security patterns). No LLM call.".to_string(), + description: "Check a code snippet against cora's deterministic checks (built-in and custom rules, secrets, security patterns). No LLM call.".to_string(), input_schema: serde_json::json!({ "type": "object", "properties": {