fix(rules): max_findings defaults to 50 and never hides critical/major findings

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"`).

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,16 +461,16 @@ 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
 
 `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):

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).

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<ReviewIssue>, cap: usize) -> (Vec<ReviewIssue>,
         .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<ReviewIssue>, cap: usize) -> (Vec<ReviewIssue>,
 pub fn dropped_warning(dropped: usize, config: &Config) -> Option<String> {
     (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,14 +451,55 @@ 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);
         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_never_hides_critical_or_major() {
+        // 4 serious findings, cap 2: all 4 stay; the minors fill no slot.
+        let mut issues: Vec<ReviewIssue> = (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,15 +510,15 @@ 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 =
             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");
+        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<FileEntry> {
+        (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));

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(),
         }

src/mcp/agent_tools.rs

@@ -796,28 +796,29 @@ 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()
                 .unwrap()
                 .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()

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,16 +1292,24 @@ 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"),
             "{text}"
         );
 
         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}");
     }