diff --git a/CHANGELOG.md b/CHANGELOG.md index 706be67..3c04ab6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **`rules_engine.max_findings` now defaults to 50, and Critical/Major findings are never hidden.** The old default of 5 could hide serious findings (a module with 7 critical findings showed 5) and made agents and users misread a capped list as the full picture. The cap now limits only the noise: it keeps every Critical and Major finding and fills the remaining slots with the most severe Minor/Info ones, still reporting how many were not shown. On a real Rust codebase the unfiltered list was up to 2400 findings, 88% `bug-unwrap`, so unlimited is not the default. Set `max_findings: 0` (or `null`) for no cap. + - **Review is now prepare / judge / finalize with the LLM as the seam (#649).** `review::prepare` turns a diff into a `judge::Packet` (judge-visible text, assembled context, system prompt override, `valid_files`, diff chunks, deterministic findings); a `judge::Judge` returns findings (`HttpJudge` = today's LLM call, `NoJudge` = deterministic only); `review::review_with` merges, guards file paths, post-processes and decides `should_block`. Output of `cora review` is unchanged. The review pipeline now has an end-to-end test with a fake judge (no network), and the `llm` module no longer prints: spinners moved to `progress::Spinner` and `llm::run_review_prompts` / `llm::scan_files` take an events observer (the old `llm::review_diff` / `llm::review_diff_stream` are replaced by `run_review_prompts`). - **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"`). diff --git a/docs/configuration.md b/docs/configuration.md index 70621a6..09d23e7 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -445,7 +445,7 @@ Control how index-based scanners (unused imports, dead code, breaking changes) b ```yaml rules_engine: enabled: true - max_findings: 5 + max_findings: 50 index_skip_files: - "*.config.ts" - "vite.config.*" @@ -461,7 +461,7 @@ rules_engine: | Field | Type | Default | Description | |-------|------|---------|-------------| | `enabled` | `bool` | `true` | Enable/disable the rule engine | -| `max_findings` | `int` | `5` | Max deterministic findings shown per run (`0` or `null` = unlimited). See below | +| `max_findings` | `int` | `50` | Max deterministic findings shown per run; Critical and Major are never hidden (`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 @@ -469,8 +469,8 @@ rules_engine: `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. +- Findings are kept highest severity first (Critical, Major, Minor, Info); ties keep their original order. **Critical and Major findings are never hidden:** the cap limits the noise (Minor, Info) and never drops below the number of serious findings, so a project with 60 Major findings sees all 60 (and no Minor ones). Because the most severe findings always survive, the cap never changes the blocking decision. +- When findings are cut, `cora scan` and `cora review` print one line on **stderr** (`⚠ N more deterministic findings not shown (rules.max_findings = 50; critical and major are never hidden; 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): diff --git a/src/config/schema.rs b/src/config/schema.rs index b9cb6da..69ac767 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -614,7 +614,7 @@ fn default_rules_enabled() -> bool { } fn default_max_findings() -> usize { - 5 + 50 } /// `rules_engine.max_findings`: an explicit `null` means unlimited (stored as 0). diff --git a/src/engine/postprocess.rs b/src/engine/postprocess.rs index b9c652a..9de3e5b 100644 --- a/src/engine/postprocess.rs +++ b/src/engine/postprocess.rs @@ -105,7 +105,7 @@ pub fn postprocess_report( } /// Keep at most `cap` deterministic findings (those carrying a `rule_id`), -/// highest severity first; ties keep their existing order (stable). LLM issues +/// highest severity first, but never fewer than every Critical and Major one; 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 @@ -118,6 +118,14 @@ fn cap_deterministic(issues: Vec, cap: usize) -> (Vec, .filter(|(_, i)| i.rule_id.is_some()) .map(|(idx, _)| idx) .collect(); + // Critical and Major findings are never hidden: the cap is a limit on the + // noise (Minor, Info), so it never drops below the number of serious + // findings. A project with 60 Majors sees all 60 and no Minors. + let serious = det + .iter() + .filter(|&&idx| issues[idx].severity <= Severity::Major) + .count(); + let cap = cap.max(serious); if det.len() <= cap { return (issues, 0); } @@ -139,7 +147,7 @@ fn cap_deterministic(issues: Vec, cap: usize) -> (Vec, 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)", + "⚠ {dropped} more deterministic findings not shown (rules.max_findings = {}; critical and major are never hidden; raise it or set 0/null to show all)", config.rules_config.max_findings ) }) @@ -443,7 +451,7 @@ mod tests { #[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.extend((10..16).map(|l| det("a", l, Severity::Minor))); issues.push(llm(3, Severity::Minor)); let (kept, dropped) = cap_deterministic(issues, 2); assert_eq!(dropped, 4); @@ -451,6 +459,47 @@ mod tests { assert_eq!(kept.iter().filter(|i| i.rule_id.is_some()).count(), 2); } + #[test] + fn cap_never_hides_critical_or_major() { + // 4 serious findings, cap 2: all 4 stay; the minors fill no slot. + let mut issues: Vec = (1..=4) + .map(|l| { + det( + "a", + l, + if l % 2 == 0 { + Severity::Critical + } else { + Severity::Major + }, + ) + }) + .collect(); + issues.extend((10..20).map(|l| det("a", l, Severity::Minor))); + let (kept, dropped) = cap_deterministic(issues, 2); + assert_eq!(kept.len(), 4); + assert_eq!(dropped, 10); + assert!(kept.iter().all(|i| i.severity <= Severity::Major)); + } + + #[test] + fn cap_fills_the_remaining_slots_with_the_most_severe_noise() { + // 2 serious + 6 noise, cap 5: 2 serious + 3 noise (Minor before Info). + let mut issues = vec![ + det("a", 1, Severity::Critical), + det("a", 2, Severity::Major), + ]; + issues.extend((10..13).map(|l| det("a", l, Severity::Info))); + issues.extend((20..23).map(|l| det("a", l, Severity::Minor))); + let (kept, dropped) = cap_deterministic(issues, 5); + assert_eq!((kept.len(), dropped), (5, 3)); + assert_eq!( + kept.iter().filter(|i| i.severity == Severity::Info).count(), + 0, + "Info is cut before Minor" + ); + } + #[test] fn cap_under_limit_is_a_no_op() { let issues = vec![det("a", 1, Severity::Info), llm(2, Severity::Info)]; @@ -461,7 +510,7 @@ mod tests { #[test] fn zero_and_unlimited_semantics() { let mut cfg = Config::default(); - assert_eq!(cfg.rules_config.finding_cap(), 5, "default cap"); + assert_eq!(cfg.rules_config.finding_cap(), 50, "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 = @@ -469,7 +518,7 @@ mod tests { 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"); + assert_eq!(parsed.max_findings, 50, "absent = default"); } #[test] @@ -514,12 +563,47 @@ mod tests { issues.iter().filter(|i| i.rule_id.is_some()).count() } + /// `n` files, each with one Info-level rule finding (`bug-todo`). + fn noise_files(n: usize) -> Vec { + (0..n) + .map(|i| { + let content = format!("fn f{i}() {{}}\n// TODO: tidy {i}\n"); + FileEntry { + path: format!("src/n{i}.rs"), + lines: content.lines().count(), + content, + } + }) + .collect() + } + + #[test] + fn files_source_reports_truncation_of_noise_with_a_small_cap() { + let files = noise_files(8); + let mut cfg = Config::default(); + cfg.rules_config.max_findings = 5; + let r = postprocess_report(vec![], &Source::Files(&files), &cfg); + assert_eq!(r.issues.len(), 5); + assert_eq!(r.dropped, 3); + } + #[test] - fn files_source_reports_truncation_with_default_cap() { + fn files_source_never_hides_critical_findings_even_below_the_cap() { + // 8 critical secrets, cap 5: all 8 stay and nothing is "not shown". let files = secret_files(8, 0); + let mut cfg = Config::default(); + cfg.rules_config.max_findings = 5; + let r = postprocess_report(vec![], &Source::Files(&files), &cfg); + assert_eq!(secret_count(&r.issues), 8); + assert_eq!(r.dropped, 0); + } + + #[test] + fn default_cap_is_50_and_shows_a_typical_noisy_scan() { + let files = noise_files(8); let r = postprocess_report(vec![], &Source::Files(&files), &Config::default()); - assert_eq!(secret_count(&r.issues), 5); - assert_eq!(r.dropped, 3); + assert_eq!(r.issues.len(), 8); + assert_eq!(r.dropped, 0); } #[test] @@ -546,10 +630,16 @@ mod tests { 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()); + let noise = noise_files(8); + let mut cfg = Config::default(); + cfg.rules_config.max_findings = 5; + let r = postprocess_report(llm_issues, &Source::Files(&noise), &cfg); 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); + // Serious findings are exempt from the cap as well. + let r = postprocess_report(vec![], &Source::Files(&files), &cfg); + assert_eq!(secret_count(&r.issues), 8); } fn secret_diff(n: usize, ignored: usize) -> String { @@ -586,9 +676,12 @@ mod tests { 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); + // Critical secrets are never hidden, whatever the cap. + let mut cfg = Config::default(); + cfg.rules_config.max_findings = 5; + let r = postprocess_report(issues, &Source::Diff(&chunks), &cfg); + assert!(secret_count(&r.issues) >= 8); + assert_eq!(r.dropped, 0); // 5 suppressed + 1 real: nothing dropped, the real one is reported. let (issues, chunks) = diff_findings(&secret_diff(6, 5)); diff --git a/src/engine/rules/types.rs b/src/engine/rules/types.rs index 8602326..1ccae81 100644 --- a/src/engine/rules/types.rs +++ b/src/engine/rules/types.rs @@ -64,7 +64,7 @@ impl Default for RulesConfig { fn default() -> Self { Self { enabled: true, - max_findings: 5, + max_findings: 50, custom_rules: Vec::new(), index_skip_files: default_index_skip_files(), } diff --git a/src/mcp/agent_tools.rs b/src/mcp/agent_tools.rs index 1384c1b..dbb4a2c 100644 --- a/src/mcp/agent_tools.rs +++ b/src/mcp/agent_tools.rs @@ -796,19 +796,20 @@ mod tests { #[test] fn capped_deterministic_findings_are_reported_not_silently_hidden() { - // 7 TODO comments, default cap 5: prepare lists all, finalize says 2 are hidden. + // 60 Info-level TODO findings, default cap 50: prepare lists all, finalize + // says 10 are hidden. let mut diff = String::from(concat!( "diff --git a/lib.rs b/lib.rs\n", "new file mode 100644\n", "--- /dev/null\n", "+++ b/lib.rs\n", - "@@ -0,0 +1,7 @@\n", + "@@ -0,0 +1,60 @@\n", )); - for n in 0..7 { + for n in 0..60 { diff.push_str(&format!("+// TODO item {n}\n")); } let prep = as_json(&review_prepare(&json!({ "diff": diff }))); - assert_eq!(prep["deterministic_findings_total"], 7); + assert_eq!(prep["deterministic_findings_total"], 60); assert!( prep["deterministic_findings_note"] .as_str() @@ -816,8 +817,8 @@ mod tests { .contains("not_shown") ); let out = as_json(&review_finalize(&json!({ "diff": diff, "issues": [] }))); - assert_eq!(out["total_issues"], 5); - assert_eq!(out["not_shown"], 2); + assert_eq!(out["total_issues"], 50); + assert_eq!(out["not_shown"], 10); assert!( out["not_shown_note"] .as_str() diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index a56e1f1..03ce345 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -396,7 +396,7 @@ fn check_snippet_with( } if report.dropped > 0 { lines.push(format!( - "\n{} more deterministic finding(s) not shown (rules.max_findings = {}; raise it or set 0/null to show all)", + "\n{} more deterministic finding(s) not shown (rules.max_findings = {}; critical and major are never hidden; raise it or set 0/null to show all)", report.dropped, config.rules_config.max_findings )); } @@ -1292,8 +1292,9 @@ mod tests { fn snippet_respects_max_findings_and_reports_dropped() { let mut cfg = crate::config::schema::Config::default(); cfg.rules_config.max_findings = 1; - let code = "a = 'AKIAIOSFODNN7EXAMPLE'\nb = 'AKIAIOSFODNN7EXAMPLF'"; - let text = snippet_text(code, "py", &cfg); + // Two Info-level findings: only noise is capped. + let code = "// TODO: first\n// TODO: second"; + let text = snippet_text(code, "rs", &cfg); assert!(text.contains("Found 1 issue(s)"), "{text}"); assert!( text.contains("1 more deterministic finding(s) not shown"), @@ -1301,7 +1302,14 @@ mod tests { ); cfg.rules_config.max_findings = 0; // unlimited - let text = snippet_text(code, "py", &cfg); + let text = snippet_text(code, "rs", &cfg); + assert!(text.contains("Found 2 issue(s)"), "{text}"); + assert!(!text.contains("not shown"), "{text}"); + + // Critical findings are never hidden, whatever the cap. + cfg.rules_config.max_findings = 1; + let secrets = "a = 'AKIAIOSFODNN7EXAMPLE'\nb = 'AKIAIOSFODNN7EXAMPLF'"; + let text = snippet_text(secrets, "py", &cfg); assert!(text.contains("Found 2 issue(s)"), "{text}"); assert!(!text.contains("not shown"), "{text}"); }