From 35da6a90d6f92a14abdb048e9829db4731976840 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Fri, 9 Oct 2026 13:11:23 +0700 Subject: [PATCH 1/2] fix(rules): apply max_findings after suppression and report dropped findings (#624) Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 4 + docs/configuration.md | 11 +- src/commands/review.rs | 18 +++ src/commands/scan.rs | 34 +++-- src/config/schema.rs | 14 +- src/engine/cache.rs | 1 + src/engine/deterministic.rs | 11 +- src/engine/llm/mod.rs | 1 + src/engine/postprocess.rs | 264 +++++++++++++++++++++++++++++++++++- src/engine/review.rs | 12 +- src/engine/rules/mod.rs | 2 +- src/engine/rules/types.rs | 14 +- src/engine/types.rs | 5 + src/formatters/compact.rs | 2 + src/formatters/json_fmt.rs | 2 + src/formatters/pretty.rs | 3 + src/formatters/sarif.rs | 3 + 17 files changed, 376 insertions(+), 25 deletions(-) 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..d659bcd 100644 --- a/src/engine/deterministic.rs +++ b/src/engine/deterministic.rs @@ -45,11 +45,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), 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(); From b06c5d36bc117bac71515f2ab5e9003c875e2d7a Mon Sep 17 00:00:00 2001 From: ajianaz Date: Fri, 9 Oct 2026 13:14:58 +0700 Subject: [PATCH 2/2] fix(review): bound the deterministic findings listed in the LLM prompt (#624) Scanners now run uncapped (the cap is applied after suppression), so the prompt context keeps only the worst 20 findings per family and says how many are omitted, instead of growing with the diff. Co-Authored-By: Claude Sonnet 5.5 --- src/engine/deterministic.rs | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/engine/deterministic.rs b/src/engine/deterministic.rs index d659bcd..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 { @@ -102,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); } } @@ -214,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);