diff --git a/CHANGELOG.md b/CHANGELOG.md index c30cede..4ab4579 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`rules_engine.max_findings` is applied after suppression and truncation is reported (#624).** The cap (default 5) used to be applied inside each scanner, before `ignore.rules` / `cora-ignore`, so suppressed findings consumed slots and pushed real findings out, and the rest were dropped silently. Scanners now run uncapped and the cap is enforced once in post-processing on deterministic findings only (LLM issues are never capped), highest severity first. `cora scan` and `cora review` print `⚠ N more deterministic findings not shown (...)` on stderr when findings are cut, and the `cora scan` summary mentions it; stdout formats are unchanged. `max_findings: 0` or `null` now means unlimited (previously `0` hid every finding). + ## [0.17.2] - 2026-10-09 ### Fixed diff --git a/docs/configuration.md b/docs/configuration.md index 4a314b6..4f1eba7 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -453,9 +453,18 @@ rules_engine: | Field | Type | Default | Description | |-------|------|---------|-------------| | `enabled` | `bool` | `true` | Enable/disable the rule engine | -| `max_findings` | `int` | `5` | Max findings per scan | +| `max_findings` | `int` | `5` | Max deterministic findings shown per run (`0` or `null` = unlimited). See below | | `index_skip_files` | `[string]` | *(see below)* | Glob patterns for files to skip during index scanning | +#### What `max_findings` caps + +`max_findings` caps the **deterministic** findings only (rule engine, secrets, security, and index scanners; the findings that carry a `rule_id`). LLM issues are never capped. + +- The cap is applied **after** suppression (`ignore.rules` and inline `cora-ignore:` markers), so suppressed findings never use up slots and hide real ones. +- Findings are kept highest severity first (Critical, Major, Minor, Info); ties keep their original order. Because the most severe findings survive, the cap never changes the blocking decision for a cap of 1 or more. +- When findings are cut, `cora scan` and `cora review` print one line on **stderr** (`⚠ N more deterministic findings not shown (rules.max_findings = 5; raise it or set 0/null to show all)`), and the `cora scan` summary mentions it. JSON, SARIF and compact output on stdout are unchanged. +- `0` or `null` means unlimited. For large diffs that `cora review` splits into chunks, the cap applies per chunk. + Default `index_skip_files` patterns (bundled with cora): - `*.config.ts`, `*.config.js`, `*.config.mjs` — bundler/build config files diff --git a/src/commands/review.rs b/src/commands/review.rs index 7a76bbe..f8572d1 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -254,6 +254,8 @@ pub async fn execute_review( } }; + warn_dropped(response.dropped_findings, config, opts.quiet); + // 6. Filter by severity if specified let min_severity = if let Some(ref sev) = opts.severity { Severity::from_str_lossy(sev) @@ -505,6 +507,7 @@ async fn execute_chunked_review( let mut any_error = false; let mut any_success = false; let mut should_block = false; + let mut dropped_findings = 0usize; for chunk in &chunks { if !opts.quiet { @@ -565,6 +568,7 @@ async fn execute_chunked_review( )); } + dropped_findings += resp.dropped_findings; all_issues.extend(resp.issues); if resp.should_block { should_block = true; @@ -629,11 +633,14 @@ async fn execute_chunked_review( // Build merged response let merged_summary = merged_chunk_summary(&summaries, all_issues.len(), any_error); + warn_dropped(dropped_findings, config, opts.quiet); + let merged_response = ReviewResponse { issues: all_issues, summary: merged_summary, tokens_used: Some(total_tokens), should_block, + dropped_findings, }; // 6. Filter by severity if specified @@ -784,6 +791,16 @@ fn compute_exit_code( } } +/// Print the one-line "findings not shown" warning to stderr (#624). +fn warn_dropped(dropped: usize, config: &Config, quiet: bool) { + if quiet { + return; + } + if let Some(w) = crate::engine::postprocess::dropped_warning(dropped, config) { + eprintln!("{}", w.yellow()); + } +} + #[cfg(test)] mod tests { use super::*; @@ -803,6 +820,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block, + dropped_findings: 0, } } diff --git a/src/commands/scan.rs b/src/commands/scan.rs index b0e5d49..de17856 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -6,7 +6,7 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::ReviewIssue; -use crate::engine::postprocess::{Source, postprocess}; +use crate::engine::postprocess::{Source, dropped_warning, postprocess_report}; use crate::engine::review_store; use crate::engine::scanner::{FileEntry, batch_files, format_batch_for_prompt, walk_project}; use crate::engine::types::TokenUsage; @@ -127,7 +127,8 @@ pub async fn execute_scan( let index_findings = crate::engine::index_scanner::scan_project_index( &index_bridge, &files, - config.rules_config.max_findings, + // Uncapped: the cap is enforced after suppression in `postprocess` (#624). + usize::MAX, &index_skip, ); if !index_findings.is_empty() { @@ -253,7 +254,10 @@ pub async fn execute_scan( // apply ignore.rules and inline cora-ignore markers. Deterministic // findings survive an LLM failure (#595). all_issues.extend(index_findings); - let all_issues = finalize_issues(config, &files, all_issues); + let (all_issues, dropped) = finalize_issues(config, &files, all_issues); + if let Some(w) = dropped_warning(dropped, config) { + eprintln!("{}", w.yellow()); + } let issue_count = all_issues.len(); let min_severity = config.hook.min_severity_level(); // Ord order is Critical(0) < Major(1) < Minor(2) < Info(3), so "at or above @@ -265,10 +269,17 @@ pub async fn execute_scan( files_scanned: files.len(), lines_scanned: total_lines, summary: format!( - "Scanned {} files ({} lines), found {} issues.", + "Scanned {} files ({} lines), found {} issues{}.", files.len(), total_lines, - issue_count + issue_count, + if dropped > 0 { + format!( + " ({dropped} more deterministic findings not shown, rules.max_findings cap)" + ) + } else { + String::new() + } ), tokens_used: total_tokens, should_block, @@ -342,8 +353,9 @@ fn finalize_issues( config: &Config, files: &[FileEntry], issues: Vec, -) -> Vec { - postprocess(issues, &Source::Files(files), config) +) -> (Vec, usize) { + let report = postprocess_report(issues, &Source::Files(files), config); + (report.issues, report.dropped) } /// Compute a short SHA256 hash of a file's content for incremental scanning. @@ -428,7 +440,7 @@ mod tests { } fn secret_title() -> String { - let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()); + let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()).0; out.iter() .find(|i| i.line == Some(2)) .unwrap_or_else(|| panic!("no finding on line 2: {:?}", titles(&out))) @@ -438,7 +450,7 @@ mod tests { #[test] fn finds_hardcoded_secret_without_llm() { - let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()); + let out = finalize_issues(&Config::default(), &[secret_file("")], Vec::new()).0; assert!( out.iter() .any(|i| i.file == "src/app.py" && i.line == Some(2)), @@ -451,7 +463,7 @@ mod tests { fn inline_marker_suppresses_secret() { let title = secret_title(); let file = secret_file(&format!(" # cora-ignore: {title}")); - let out = finalize_issues(&Config::default(), &[file], Vec::new()); + let out = finalize_issues(&Config::default(), &[file], Vec::new()).0; assert!( !out.iter().any(|i| i.line == Some(2)), "got {:?}", @@ -463,7 +475,7 @@ mod tests { fn ignore_rules_suppress_secret() { let mut config = Config::default(); config.ignore.rules = vec![secret_title()]; - let out = finalize_issues(&config, &[secret_file("")], Vec::new()); + let out = finalize_issues(&config, &[secret_file("")], Vec::new()).0; assert!( !out.iter().any(|i| i.line == Some(2)), "got {:?}", diff --git a/src/config/schema.rs b/src/config/schema.rs index 40c9d40..11f7395 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -613,13 +613,25 @@ fn default_max_findings() -> usize { 5 } +/// `rules_engine.max_findings`: an explicit `null` means unlimited (stored as 0). +fn deserialize_max_findings<'de, D>(d: D) -> std::result::Result +where + D: serde::Deserializer<'de>, +{ + Ok(Option::::deserialize(d)?.unwrap_or(0)) +} + /// Rule engine configuration section for `.cora.yaml`. #[derive(Debug, Clone, Serialize, Deserialize, Default)] #[serde(deny_unknown_fields)] pub struct RulesSection { #[serde(default, skip_serializing_if = "is_default")] pub enabled: bool, - #[serde(default = "default_max_findings")] + /// Cap on deterministic findings shown per run. `0` or `null` = unlimited (#624). + #[serde( + default = "default_max_findings", + deserialize_with = "deserialize_max_findings" + )] pub max_findings: usize, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub custom: Vec, diff --git a/src/engine/cache.rs b/src/engine/cache.rs index 042e6cd..61a061d 100644 --- a/src/engine/cache.rs +++ b/src/engine/cache.rs @@ -195,6 +195,7 @@ mod tests { summary: "Found 1 issue.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } diff --git a/src/engine/deterministic.rs b/src/engine/deterministic.rs index d75bf4e..90bfab1 100644 --- a/src/engine/deterministic.rs +++ b/src/engine/deterministic.rs @@ -23,6 +23,9 @@ use crate::engine::index_bridge::IndexBridge; use crate::engine::rules::{self, types::RuleFinding}; use crate::engine::{index_scanner, secrets_scanner, security_scanner}; +/// Max findings per family listed in the LLM prompt context. +const PROMPT_FINDINGS_PER_FAMILY: usize = 20; + /// Findings of every deterministic scanner family, in contract order. #[derive(Debug, Default)] pub struct DeterministicReport { @@ -45,11 +48,18 @@ pub fn skip_patterns(config: &Config) -> Vec { /// Run every deterministic check on a parsed diff. pub fn run(chunks: &[FileChunk], config: &Config, bridge: &IndexBridge) -> DeterministicReport { - let max = config.rules_config.max_findings; + // Scanners run uncapped: `rules_engine.max_findings` is enforced by + // `postprocess` AFTER suppression, so suppressed findings never consume + // slots and truncation can be reported (#624). + let max = usize::MAX; let skip = skip_patterns(config); + let uncapped_rules = crate::engine::rules::types::RulesConfig { + max_findings: 0, + ..config.rules_config.clone() + }; DeterministicReport { - rules: rules::run_rules(chunks, &config.rules_config), + rules: rules::run_rules(chunks, &uncapped_rules), secrets: secrets_scanner::scan_secrets(chunks, max), security: security_scanner::scan_security(chunks, max), index_unused: index_scanner::scan_unused_imports(bridge, chunks, max, &skip), @@ -95,8 +105,18 @@ impl DeterministicReport { parts.push(warning); } for family in self.families() { - let text = rules::format_rule_context(family); + // Scanners run uncapped (the user-facing cap is applied after + // suppression in `postprocess`), so bound what is sent to the model: + // families are severity-sorted, keep the worst PROMPT_FINDINGS_PER_FAMILY. + let shown = &family[..family.len().min(PROMPT_FINDINGS_PER_FAMILY)]; + let mut text = rules::format_rule_context(shown); if !text.is_empty() { + if family.len() > shown.len() { + text.push_str(&format!( + "({} more findings of this kind not listed here)\n", + family.len() - shown.len() + )); + } parts.push(text); } } @@ -207,6 +227,26 @@ diff --git a/src/api.js b/src/api.js ); } + #[test] + fn prompt_context_is_bounded_per_family() { + let report = DeterministicReport { + secrets: (1..=(PROMPT_FINDINGS_PER_FAMILY as u32 + 7)) + .map(|line| RuleFinding { + rule_id: "secrets/x".to_string(), + file: "a.rs".to_string(), + line, + severity: crate::engine::Severity::Critical, + title: "t".to_string(), + body: "b".to_string(), + }) + .collect(), + ..Default::default() + }; + let ctx = report.context(None).expect("context"); + assert_eq!(ctx.matches("secrets/x").count(), PROMPT_FINDINGS_PER_FAMILY); + assert!(ctx.contains("(7 more findings of this kind not listed here)")); + } + #[test] fn context_is_ordered_and_optional() { let chunks = parse_diff(DIFF); diff --git a/src/engine/llm/mod.rs b/src/engine/llm/mod.rs index 5e2921c..7dd4797 100644 --- a/src/engine/llm/mod.rs +++ b/src/engine/llm/mod.rs @@ -145,6 +145,7 @@ fn into_review_response(findings: Findings, usage: Option<&transport::Usage>) -> summary: findings.summary.unwrap_or_default(), tokens_used: usage.map(usage_to_token_usage), should_block: false, + dropped_findings: 0, } } diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs index 85d44d7..b09c02e 100644 --- a/src/engine/postprocess.rs +++ b/src/engine/postprocess.rs @@ -25,6 +25,11 @@ //! | 5 | inline `cora-ignore:` markers | yes | yes | //! | 6 | context-line severity filter | yes | no | //! +//! After the filters, `rules_engine.max_findings` caps the deterministic +//! findings (those with a `rule_id`; LLM issues are never capped), highest +//! severity first. `0` or `null` = unlimited. The dropped count is returned by +//! [`postprocess_report`] so the CLI can warn on stderr (#624). +//! //! Filters 2, 3 and 6 reason about added vs unchanged diff lines (or LLM //! mistakes about added lines), which a full-file scan does not have: every //! line of a scanned file is "added", and scan findings come from the @@ -46,14 +51,35 @@ pub enum Source<'a> { Files(&'a [FileEntry]), } +/// Result of [`postprocess_report`]. +#[derive(Debug, Default)] +pub struct PostprocessReport { + /// Final, user-visible findings. + pub issues: Vec, + /// Deterministic findings cut by `rules_engine.max_findings` (0 = none). + pub dropped: usize, +} + /// Run the shared post-processing pipeline over `issues`. See the module docs -/// for the per-source filter table. +/// for the per-source filter table. Drops the truncation count; use +/// [`postprocess_report`] when the caller should surface it. +#[cfg_attr(not(test), allow(dead_code))] // callers use postprocess_report; kept for tests/API pub fn postprocess( issues: Vec, source: &Source<'_>, config: &Config, ) -> Vec { - match source { + postprocess_report(issues, source, config).issues +} + +/// Like [`postprocess`], but also reports how many deterministic findings the +/// `rules_engine.max_findings` cap removed (#624). +pub fn postprocess_report( + issues: Vec, + source: &Source<'_>, + config: &Config, +) -> PostprocessReport { + let issues = match source { Source::Diff(chunks) => { let issues = apply_llm_secret_fp_filter(issues, chunks); let issues = apply_markdown_code_block_filter(issues, chunks); @@ -66,11 +92,12 @@ pub fn postprocess( } Source::Files(files) => { let chunks = files_as_chunks(files); - let max = config.rules_config.max_findings; + // Scanners run uncapped; the cap is enforced below, after + // suppression (#624). let mut merged = issues; for family in [ - secrets_scanner::scan_secrets(&chunks, max), - security_scanner::scan_security(&chunks, max), + secrets_scanner::scan_secrets(&chunks, usize::MAX), + security_scanner::scan_security(&chunks, usize::MAX), ] { if !family.is_empty() { merged = rules::merge_rule_findings(merged, family); @@ -79,7 +106,50 @@ pub fn postprocess( let merged = apply_ignore_rules(merged, &config.ignore.rules); inline_suppress::apply(merged, &chunks) } + }; + let (issues, dropped) = cap_deterministic(issues, config.rules_config.finding_cap()); + PostprocessReport { issues, dropped } +} + +/// Keep at most `cap` deterministic findings (those carrying a `rule_id`), +/// highest severity first; ties keep their existing order (stable). LLM issues +/// are never counted or dropped, and every kept issue keeps its position. +/// +/// Because the most severe findings survive, the blocking decision +/// (`any(severity <= min_severity)`) is unchanged by the cap for any `cap >= 1`. +/// Returns the kept issues and the number of deterministic findings dropped. +fn cap_deterministic(issues: Vec, cap: usize) -> (Vec, usize) { + let mut det: Vec = issues + .iter() + .enumerate() + .filter(|(_, i)| i.rule_id.is_some()) + .map(|(idx, _)| idx) + .collect(); + if det.len() <= cap { + return (issues, 0); } + // Stable: equal severities stay in their original relative order. + det.sort_by_key(|&idx| issues[idx].severity); + let drop: std::collections::HashSet = det[cap..].iter().copied().collect(); + let dropped = drop.len(); + let kept = issues + .into_iter() + .enumerate() + .filter(|(idx, _)| !drop.contains(idx)) + .map(|(_, i)| i) + .collect(); + (kept, dropped) +} + +/// One-line stderr warning for truncated deterministic findings, or `None` +/// when nothing was dropped. +pub fn dropped_warning(dropped: usize, config: &Config) -> Option { + (dropped > 0).then(|| { + format!( + "⚠ {dropped} more deterministic findings not shown (rules.max_findings = {}; raise it or set 0/null to show all)", + config.rules_config.max_findings + ) + }) } /// Filter out LLM findings about hardcoded secrets/passwords that point to @@ -345,6 +415,190 @@ fn apply_context_line_filter( mod tests { use super::*; + // ─── max_findings cap (#624) ─── + + fn det(file: &str, line: u32, sev: Severity) -> ReviewIssue { + ReviewIssue::new(file, Some(line), sev, format!("t{line}")).with_rule_id("r") + } + + fn llm(line: u32, sev: Severity) -> ReviewIssue { + ReviewIssue::new("a.rs", Some(line), sev, format!("llm{line}")) + } + + #[test] + fn cap_keeps_highest_severity_stably_and_counts_dropped() { + let issues = vec![ + det("a", 1, Severity::Info), + det("a", 2, Severity::Major), + det("a", 3, Severity::Critical), + det("a", 4, Severity::Major), + det("a", 5, Severity::Info), + ]; + let (kept, dropped) = cap_deterministic(issues, 3); + assert_eq!(dropped, 2); + // Critical + both Majors kept, original relative order preserved. + let lines: Vec<_> = kept.iter().map(|i| i.line.unwrap()).collect(); + assert_eq!(lines, vec![2, 3, 4]); + } + + #[test] + fn cap_never_counts_or_drops_llm_issues() { + let mut issues = vec![llm(1, Severity::Info), llm(2, Severity::Info)]; + issues.extend((10..16).map(|l| det("a", l, Severity::Major))); + issues.push(llm(3, Severity::Minor)); + let (kept, dropped) = cap_deterministic(issues, 2); + assert_eq!(dropped, 4); + assert_eq!(kept.iter().filter(|i| i.rule_id.is_none()).count(), 3); + assert_eq!(kept.iter().filter(|i| i.rule_id.is_some()).count(), 2); + } + + #[test] + fn cap_under_limit_is_a_no_op() { + let issues = vec![det("a", 1, Severity::Info), llm(2, Severity::Info)]; + let (kept, dropped) = cap_deterministic(issues, 5); + assert_eq!((kept.len(), dropped), (2, 0)); + } + + #[test] + fn zero_and_unlimited_semantics() { + let mut cfg = Config::default(); + assert_eq!(cfg.rules_config.finding_cap(), 5, "default cap"); + cfg.rules_config.max_findings = 0; + assert_eq!(cfg.rules_config.finding_cap(), usize::MAX, "0 = unlimited"); + let parsed: crate::config::schema::RulesSection = + serde_yaml_ng::from_str("enabled: true\nmax_findings: null\n").unwrap(); + assert_eq!(parsed.max_findings, 0, "null = 0"); + let parsed: crate::config::schema::RulesSection = + serde_yaml_ng::from_str("enabled: true\n").unwrap(); + assert_eq!(parsed.max_findings, 5, "absent = default"); + } + + #[test] + fn dropped_warning_text() { + let cfg = Config::default(); + assert!(dropped_warning(0, &cfg).is_none()); + let w = dropped_warning(3, &cfg).unwrap(); + assert!(w.contains("3 more deterministic findings")); + assert!(w.contains("rules.max_findings = 5")); + } + + /// Inline marker naming every finding a probe secret line produces, so + /// the marked lines are suppressed regardless of how many rules fire. + fn ignore_marker() -> String { + let files = secret_files_with(1, 0, ""); + let r = postprocess_report(vec![], &Source::Files(&files), &Config::default()); + let ids: Vec<_> = r.issues.iter().filter_map(|i| i.rule_id.clone()).collect(); + assert!(!ids.is_empty(), "probe must produce a finding"); + format!(" # cora-ignore: {}", ids.join(", ")) + } + + fn secret_files(n: usize, extra_on_first: usize) -> Vec { + secret_files_with(n, extra_on_first, &ignore_marker()) + } + + fn secret_files_with(n: usize, extra_on_first: usize, marker: &str) -> Vec { + (0..n) + .map(|i| { + let marker = if i < extra_on_first { marker } else { "" }; + let content = + format!("import os\npassword = \"hunter2hunter{i}\"{marker}\nprint(1)\n"); + FileEntry { + path: format!("src/app{i}.py"), + lines: content.lines().count(), + content, + } + }) + .collect() + } + + fn secret_count(issues: &[ReviewIssue]) -> usize { + issues.iter().filter(|i| i.rule_id.is_some()).count() + } + + #[test] + fn files_source_reports_truncation_with_default_cap() { + let files = secret_files(8, 0); + let r = postprocess_report(vec![], &Source::Files(&files), &Config::default()); + assert_eq!(secret_count(&r.issues), 5); + assert_eq!(r.dropped, 3); + } + + #[test] + fn files_source_unlimited_when_cap_is_zero() { + let files = secret_files(8, 0); + let mut cfg = Config::default(); + cfg.rules_config.max_findings = 0; + let r = postprocess_report(vec![], &Source::Files(&files), &cfg); + assert_eq!(secret_count(&r.issues), 8); + assert_eq!(r.dropped, 0); + } + + #[test] + fn files_source_suppressed_findings_do_not_consume_cap_slots() { + // 5 inline-suppressed secrets + 1 real one: the real one must survive. + let files = secret_files(6, 5); + let r = postprocess_report(vec![], &Source::Files(&files), &Config::default()); + assert_eq!(r.dropped, 0); + assert_eq!(secret_count(&r.issues), 1); + assert_eq!(r.issues[0].file, "src/app5.py"); + } + + #[test] + fn files_source_llm_issues_are_never_capped() { + let files = secret_files(8, 0); + let llm_issues: Vec<_> = (1..=7).map(|l| llm(l, Severity::Info)).collect(); + let r = postprocess_report(llm_issues, &Source::Files(&files), &Config::default()); + assert_eq!(r.issues.iter().filter(|i| i.rule_id.is_none()).count(), 7); + assert_eq!(secret_count(&r.issues), 5); + assert_eq!(r.dropped, 3); + } + + fn secret_diff(n: usize, ignored: usize) -> String { + let marker_text = ignore_marker(); + let mut d = String::new(); + for i in 0..n { + let marker = if i < ignored { + marker_text.as_str() + } else { + "" + }; + d.push_str(&format!( + "diff --git a/src/f{i}.py b/src/f{i}.py\n--- a/src/f{i}.py\n+++ b/src/f{i}.py\n@@ -1,1 +1,2 @@\n import os\n+password = \"hunter2hunter{i}\"{marker}\n" + )); + } + d + } + + fn diff_findings(diff: &str) -> (Vec, Vec) { + let chunks = crate::engine::diff_parser::parse_diff(diff); + let cfg = Config::default(); + // Isolated: a db path inside a fresh temp dir that does not exist, so + // the real data dir is never read. + let tmp = tempfile::tempdir().unwrap(); + let bridge = crate::engine::index_bridge::IndexBridge::open_tolerant_at( + &tmp.path().join("none.db"), + tmp.path(), + ); + let det = crate::engine::deterministic::run(&chunks, &cfg, &bridge); + (det.merge_into(vec![]), chunks) + } + + #[test] + fn diff_source_caps_after_suppression_and_reports_dropped() { + let (issues, chunks) = diff_findings(&secret_diff(8, 0)); + assert!(secret_count(&issues) >= 8, "scanners must run uncapped"); + let r = postprocess_report(issues, &Source::Diff(&chunks), &Config::default()); + assert_eq!(secret_count(&r.issues), 5); + assert_eq!(r.dropped, 3); + + // 5 suppressed + 1 real: nothing dropped, the real one is reported. + let (issues, chunks) = diff_findings(&secret_diff(6, 5)); + let r = postprocess_report(issues, &Source::Diff(&chunks), &Config::default()); + assert_eq!(r.dropped, 0); + assert!(r.issues.iter().all(|i| i.file == "src/f5.py")); + assert!(!r.issues.is_empty()); + } + #[test] fn secret_fp_filter_removes_struct_field_declarations() { use crate::engine::diff_parser::*; diff --git a/src/engine/review.rs b/src/engine/review.rs index d6f4045..6b76a88 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -3,7 +3,7 @@ use tracing::{debug, instrument}; use crate::config::schema::Config; use crate::engine::llm; -use crate::engine::postprocess::{Source, postprocess}; +use crate::engine::postprocess::{Source, postprocess_report}; use crate::engine::types::{LLMConfig, ReviewResponse}; /// Load a custom system prompt from a file path. @@ -97,6 +97,7 @@ async fn review_diff_inner( summary: "No changes to review.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, }); } @@ -308,8 +309,11 @@ async fn review_diff_inner( ), tokens_used: None, should_block: false, + dropped_findings: 0, }; - fallback.issues = postprocess(fallback.issues, &Source::Diff(&diff_chunks), config); + let pp = postprocess_report(fallback.issues, &Source::Diff(&diff_chunks), config); + fallback.issues = pp.issues; + fallback.dropped_findings = pp.dropped; let min_sev = config.hook.min_severity_level(); fallback.should_block = fallback .issues @@ -343,7 +347,9 @@ async fn review_diff_inner( // Shared post-processing (LLM secret FP cross-check, Markdown code blocks, // ignore.rules, inline `cora-ignore:`, context-line filter), see // `engine::postprocess`. - response.issues = postprocess(response.issues, &Source::Diff(&diff_chunks), config); + let pp = postprocess_report(response.issues, &Source::Diff(&diff_chunks), config); + response.issues = pp.issues; + response.dropped_findings = pp.dropped; // Calculate should_block based on min_severity let min_severity = config.hook.min_severity_level(); diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index d2898d6..900bc46 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -108,7 +108,7 @@ pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec findings.dedup_by(|a, b| a.rule_id == b.rule_id && a.file == b.file && a.line == b.line); // Cap at max_findings - let capped = findings.len().min(config.max_findings); + let capped = findings.len().min(config.finding_cap()); if findings.len() > capped { debug!(total = findings.len(), capped, "capping rule findings"); findings.truncate(capped); diff --git a/src/engine/rules/types.rs b/src/engine/rules/types.rs index 7567479..8602326 100644 --- a/src/engine/rules/types.rs +++ b/src/engine/rules/types.rs @@ -8,7 +8,8 @@ use crate::engine::Severity; pub struct RulesConfig { /// Whether the rule engine is enabled. pub enabled: bool, - /// Maximum number of findings to report per review (prevents noisy output). + /// Maximum number of deterministic findings shown per run, applied after + /// suppression (`ignore.rules`, `cora-ignore`). `0` = unlimited (#624). pub max_findings: usize, /// User-defined custom rules, merged with built-in rules. pub custom_rules: Vec, @@ -48,6 +49,17 @@ pub(crate) fn default_index_skip_files() -> Vec { ] } +impl RulesConfig { + /// Effective cap: `0` means unlimited. + pub fn finding_cap(&self) -> usize { + if self.max_findings == 0 { + usize::MAX + } else { + self.max_findings + } + } +} + impl Default for RulesConfig { fn default() -> Self { Self { diff --git a/src/engine/types.rs b/src/engine/types.rs index db9e299..bce81a8 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -485,6 +485,11 @@ pub struct ReviewResponse { pub summary: String, pub tokens_used: Option, pub should_block: bool, + /// Deterministic findings cut by `rules_engine.max_findings` after + /// suppression (#624). Never serialized, so JSON/SARIF output is + /// unchanged; the CLI reports it on stderr. + #[serde(skip)] + pub dropped_findings: usize, } /// Response from a full project scan diff --git a/src/formatters/compact.rs b/src/formatters/compact.rs index ccacb59..d7f9014 100644 --- a/src/formatters/compact.rs +++ b/src/formatters/compact.rs @@ -99,6 +99,7 @@ mod tests { summary: "Summary".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -108,6 +109,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } diff --git a/src/formatters/json_fmt.rs b/src/formatters/json_fmt.rs index d5c3546..88798f0 100644 --- a/src/formatters/json_fmt.rs +++ b/src/formatters/json_fmt.rs @@ -65,6 +65,7 @@ mod tests { estimated_cost_usd: 0.005, }), should_block: false, + dropped_findings: 0, } } @@ -74,6 +75,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } diff --git a/src/formatters/pretty.rs b/src/formatters/pretty.rs index a3a5734..87e5cba 100644 --- a/src/formatters/pretty.rs +++ b/src/formatters/pretty.rs @@ -254,6 +254,7 @@ mod tests { summary: "Found 1 critical issue.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -263,6 +264,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -272,6 +274,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: true, + dropped_findings: 0, } } diff --git a/src/formatters/sarif.rs b/src/formatters/sarif.rs index 0fa7570..898d902 100644 --- a/src/formatters/sarif.rs +++ b/src/formatters/sarif.rs @@ -166,6 +166,7 @@ mod tests { summary: "Found 1 critical issue.".to_string(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -175,6 +176,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, } } @@ -327,6 +329,7 @@ mod tests { summary: String::new(), tokens_used: None, should_block: false, + dropped_findings: 0, }; let fmt = SarifFormatter; let output = fmt.format_review(&response).unwrap();