fix(ignore): warn about cora-ignore markers that match nothing and add a reason separator

CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - **Custom rules are validated and the docs match the schema (#684, #678).** `cora config validate` and config load now reject an invalid regex, an empty or duplicate id, an id that shadows a built-in rule, an empty message and an unknown `languages` value, naming the rule. `languages` accepts language names (`"rust"`) as well as extensions (`"rs"`); `languages` and `exclude` are optional. `cora config validate --dry-run [--path DIR]` runs each custom rule and prints how many lines it matched. `docs/configuration.md` now shows the real schema (`rules_engine.custom`); the 0.19.0 entry no longer says `custom_rules`.
+- **`cora-ignore` no longer fails silently (#679).** A marker on a line with findings that names none of them (a typo, an unknown rule, or explanation text that became part of the rule name) now prints a warning on stderr naming the marker and the rule ids on that line. ` -- ` ends the rule list, so `cora-ignore: sec-hardcoded-secret -- env var name` suppresses the finding and keeps the reason in the source.
 - The `Cora Review` workflow now fails with an error naming each empty secret (`CORA_API_KEY`, `CORA_BASE_URL`, `CORA_MODEL`) instead of running without them.
 - The `Cora Review` job no longer gets cancelled on larger pull requests: its time limit is 30 minutes (the review action allows up to three LLM attempts of 10 minutes each).
 

docs/configuration.md

@@ -562,6 +562,8 @@ const next = compute();
 - If an LLM finding and a scanner finding on the same line describe the same problem (they share a topic word, e.g. both are about a hardcoded password/secret), only the LLM finding is reported, and a marker (or `ignore.rules` entry) naming the scanner rule's id or title still suppresses it. If the LLM finding is about something else (say SQL injection next to a hardcoded secret), both are reported and a marker only suppresses the one it names.
 - Scope: the same line; plus the next line when the marker line contains only a comment.
 - A bare `cora-ignore` (no `:` or empty rule list) suppresses nothing, so every suppression names its rule.
+- The rule list runs to the end of the line (or the comment terminator). Put an explanation after ` -- `: `// cora-ignore: sec-hardcoded-secret -- env var name, not a value`. Text without the separator becomes part of the rule name.
+- A marker on a line that has findings but names none of them (a typo, an unknown rule, trailing text) prints `warning: cora-ignore at FILE:LINE matched no finding ...` on stderr, listing the rule ids that are on that line.
 - Applies to both deterministic scanner and LLM findings, in both `cora review` and `cora scan`. Other rules on the same line and the rest of the project stay visible. Works alongside `ignore.rules` and `ignore.files`.
 - `cora review` reads markers from the diff (added and context lines); a marker outside the diff's context window is not seen. `cora scan` reads markers from the full file contents.
 - `cora scan` also runs the deterministic secrets and security scanners on every scanned file and applies `ignore.rules`, so those findings are reported even when the LLM call fails.

src/engine/inline_suppress.rs

@@ -13,6 +13,11 @@
 //! * The marker is matched as the substring `cora-ignore:` anywhere in a line,
 //!   so it works after `//`, `#`, `--`, `/* */`, `<!-- -->`, and so on.
 //! * A bare `cora-ignore` with no `:` and rule list never suppresses anything.
+//! * The rule list runs to the end of the line (or the comment terminator), so
+//!   explanatory text goes after a ` -- ` separator:
+//!   `cora-ignore: sec-hardcoded-secret -- env var name, not a value`.
+//! * A marker on a line that has findings but names none of them is reported
+//!   (see [`Unmatched`]) instead of silently doing nothing (#679).
 //!
 //! One filter ([`apply`]) is applied to the merged deterministic + LLM findings.
 //! It reads the post-change lines present in the parsed diff (added and
@@ -42,6 +47,13 @@ fn parse_rules(line: &str) -> Vec<String> {
             rest = &rest[..i];
         }
     }
+    // `-- reason text` ends the rule list; the reason is documentation only.
+    if let Some(i) = rest
+        .find(" -- ")
+        .or_else(|| rest.strip_suffix(" --").map(str::len))
+    {
+        rest = &rest[..i];
+    }
     rest.split(',')
         .map(|r| r.trim().to_lowercase())
         .filter(|r| !r.is_empty())
@@ -73,18 +85,51 @@ fn matches_any(issue: &ReviewIssue, rules: &HashSet<String>) -> bool {
         || issue.also_matches.iter().any(|a| hit(a))
 }
 
+/// A marker that sits on a line with findings but names none of them (a typo,
+/// an unknown rule, or trailing text that became part of the rule name).
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Unmatched {
+    pub file: String,
+    pub line: u32,
+    /// What the marker named (lowercased, as parsed).
+    pub named: Vec<String>,
+    /// Rule ids (or titles) of the findings on the marker's target line(s).
+    pub findings: Vec<String>,
+}
+
+impl Unmatched {
+    pub fn message(&self) -> String {
+        format!(
+            "cora-ignore at {}:{} matched no finding (named: {}); findings there: {}. Name a rule id exactly and put any explanation after ` -- `.",
+            self.file,
+            self.line,
+            self.named
+                .iter()
+                .map(|n| format!("\"{n}\""))
+                .collect::<Vec<_>>()
+                .join(", "),
+            self.findings.join(", ")
+        )
+    }
+}
+
 /// Drop findings suppressed by an inline `cora-ignore:` marker, reading the
 /// post-change lines of a diff (diff adapter over [`apply_lines`]).
-pub fn apply(issues: Vec<ReviewIssue>, chunks: &[FileChunk]) -> Vec<ReviewIssue> {
+pub fn apply(issues: Vec<ReviewIssue>, chunks: &[FileChunk]) -> (Vec<ReviewIssue>, Vec<Unmatched>) {
     let files: Vec<ScanFile<'_>> = chunks.iter().map(ScanFile::post_image).collect();
     apply_lines(issues, &files)
 }
 
 /// Drop findings suppressed by an inline `cora-ignore:` marker found in
 /// `files`' lines.
-pub fn apply_lines(mut issues: Vec<ReviewIssue>, files: &[ScanFile<'_>]) -> Vec<ReviewIssue> {
+pub fn apply_lines(
+    mut issues: Vec<ReviewIssue>,
+    files: &[ScanFile<'_>],
+) -> (Vec<ReviewIssue>, Vec<Unmatched>) {
     // (file, line) -> lowercased rules suppressed there.
     let mut suppressed: HashMap<(String, u32), HashSet<String>> = HashMap::new();
+    // Every marker: (file, marker line, target lines, rules).
+    let mut markers: Vec<(String, u32, Vec<u32>, Vec<String>)> = Vec::new();
     for file in files {
         let path = file.path;
         for &(ln, content) in &file.lines {
@@ -103,16 +148,40 @@ pub fn apply_lines(mut issues: Vec<ReviewIssue>, files: &[ScanFile<'_>]) -> Vec<
                 .entry((path.to_string(), ln))
                 .or_default()
                 .extend(rules.iter().cloned());
+            let mut targets = vec![ln];
             if is_comment_only(content) {
                 suppressed
                     .entry((path.to_string(), ln + 1))
                     .or_default()
-                    .extend(rules);
+                    .extend(rules.iter().cloned());
+                targets.push(ln + 1);
             }
+            markers.push((path.to_string(), ln, targets, rules));
         }
     }
     if suppressed.is_empty() {
-        return issues;
+        return (issues, Vec::new());
+    }
+
+    // Markers that sit on findings yet name none of them.
+    let mut unmatched = Vec::new();
+    for (file, ln, targets, rules) in &markers {
+        let set: HashSet<String> = rules.iter().cloned().collect();
+        let on_target: Vec<&ReviewIssue> = issues
+            .iter()
+            .filter(|i| i.file == *file && i.line.is_some_and(|l| targets.contains(&l)))
+            .collect();
+        if !on_target.is_empty() && !on_target.iter().any(|i| matches_any(i, &set)) {
+            unmatched.push(Unmatched {
+                file: file.clone(),
+                line: *ln,
+                named: rules.clone(),
+                findings: on_target
+                    .iter()
+                    .map(|i| i.rule_id.clone().unwrap_or_else(|| i.title.clone()))
+                    .collect(),
+            });
+        }
     }
 
     let before = issues.len();
@@ -127,7 +196,7 @@ pub fn apply_lines(mut issues: Vec<ReviewIssue>, files: &[ScanFile<'_>]) -> Vec<
     if dropped > 0 {
         debug!(dropped, "suppressed findings via inline cora-ignore");
     }
-    issues
+    (issues, unmatched)
 }
 
 #[cfg(test)]
@@ -136,6 +205,14 @@ mod tests {
     use crate::engine::Severity;
     use crate::engine::diff_parser::parse_diff;
 
+    fn apply(issues: Vec<ReviewIssue>, chunks: &[FileChunk]) -> Vec<ReviewIssue> {
+        super::apply(issues, chunks).0
+    }
+
+    fn unmatched(issues: Vec<ReviewIssue>, chunks: &[FileChunk]) -> Vec<Unmatched> {
+        super::apply(issues, chunks).1
+    }
+
     const RULE: &str = "Hardcoded password or secret in variable";
 
     fn issue(file: &str, line: u32, title: &str) -> ReviewIssue {
@@ -380,4 +457,38 @@ mod tests {
         // Without the marker the finding stays.
         assert!(!apply(issues, &plain).is_empty());
     }
+
+    #[test]
+    fn a_reason_after_the_separator_is_not_part_of_the_rule_list() {
+        let c = diff_of(&["x(); // cora-ignore: Rule A -- env var name, not a value"]);
+        assert!(apply(vec![issue("f.rs", 1, "Rule A")], &c).is_empty());
+        let c = diff_of(&["x(); /* cora-ignore: Rule A, Rule B -- why */"]);
+        assert!(apply(vec![issue("f.rs", 1, "Rule B")], &c).is_empty());
+    }
+
+    #[test]
+    fn trailing_text_without_a_separator_is_reported_not_silent() {
+        let c = diff_of(&["x(); // cora-ignore: Rule A (an env var name, not a value)"]);
+        let issues = vec![issue("f.rs", 1, "Rule A")];
+        assert_eq!(apply(issues.clone(), &c).len(), 1, "still reported");
+        let u = unmatched(issues, &c);
+        assert_eq!(u.len(), 1);
+        assert_eq!((u[0].file.as_str(), u[0].line), ("f.rs", 1));
+        assert!(u[0].message().contains("--"), "{}", u[0].message());
+    }
+
+    #[test]
+    fn an_unknown_rule_name_is_reported() {
+        let c = diff_of(&["x(); // cora-ignore: no-such-rule"]);
+        let u = unmatched(vec![issue("f.rs", 1, "Rule A")], &c);
+        assert_eq!(u.len(), 1);
+        assert_eq!(u[0].named, vec!["no-such-rule".to_string()]);
+        assert_eq!(u[0].findings, vec!["Rule A".to_string()]);
+    }
+
+    #[test]
+    fn matching_markers_and_markers_on_clean_lines_are_not_reported() {
+        let c = diff_of(&["x(); // cora-ignore: Rule A", "y(); // cora-ignore: stale"]);
+        assert!(unmatched(vec![issue("f.rs", 1, "Rule A")], &c).is_empty());
+    }
 }

src/engine/postprocess.rs

@@ -87,7 +87,8 @@ pub fn postprocess_report(
             let issues = apply_ignore_rules(issues, &config.ignore.rules);
             // Rule-scoped inline suppression (`cora-ignore: <rule>`, #554),
             // shared by deterministic and LLM findings.
-            let issues = inline_suppress::apply(issues, chunks);
+            let (issues, unmatched) = inline_suppress::apply(issues, chunks);
+            warn_unmatched(&unmatched);
             // Drop low-severity findings on unchanged (context) lines (#507).
             apply_context_line_filter(issues, chunks)
         }
@@ -97,13 +98,23 @@ pub fn postprocess_report(
             let merged = deterministic::run_files(files, config).merge_into(issues);
             let scan_files = scan_input::from_entries(files);
             let merged = apply_ignore_rules(merged, &config.ignore.rules);
-            inline_suppress::apply_lines(merged, &scan_files)
+            let (kept, unmatched) = inline_suppress::apply_lines(merged, &scan_files);
+            warn_unmatched(&unmatched);
+            kept
         }
     };
     let (issues, dropped) = cap_deterministic(issues, config.rules_config.finding_cap());
     PostprocessReport { issues, dropped }
 }
 
+/// Tell the user about `cora-ignore` markers that suppressed nothing (#679).
+/// Goes to stderr, so JSON / SARIF on stdout stays clean.
+fn warn_unmatched(unmatched: &[inline_suppress::Unmatched]) {
+    for u in unmatched {
+        eprintln!("warning: {}", u.message());
+    }
+}
+
 /// Keep at most `cap` deterministic findings (those carrying a `rule_id`),
 /// 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.

tests/inline_ignore.rs

@@ -0,0 +1,45 @@
+//! `cora-ignore` markers must not fail silently (#679).
+use std::process::Command;
+
+fn scan(src: &str) -> String {
+    let d = tempfile::tempdir().unwrap();
+    std::fs::create_dir_all(d.path().join("src")).unwrap();
+    std::fs::write(d.path().join("src/a.rs"), src).unwrap();
+    let out = Command::new(env!("CARGO_BIN_EXE_cora"))
+        .args(["scan", "--no-llm", "--format", "compact"])
+        .current_dir(d.path())
+        .env_clear()
+        .env("PATH", "/usr/bin:/bin")
+        .env("HOME", d.path())
+        .output()
+        .unwrap();
+    String::from_utf8_lossy(&out.stdout).to_string() + &String::from_utf8_lossy(&out.stderr)
+}
+
+const LINE: &str =
+    "pub const ENV_API_KEY: &str = \"TOLE_API_KEY\"; // cora-ignore: sec-hardcoded-secret";
+
+#[test]
+fn trailing_text_without_a_separator_warns_and_keeps_the_finding() {
+    let text = scan(&format!("{LINE} (an env var name)\n"));
+    assert!(text.contains("matched no finding"), "{text}");
+    assert!(text.contains("sec-hardcoded-secret"), "{text}");
+    assert!(text.contains("Found 1 issue"), "{text}");
+}
+
+#[test]
+fn an_unknown_rule_warns() {
+    let text =
+        scan("pub const ENV_API_KEY: &str = \"TOLE_API_KEY\"; // cora-ignore: no-such-rule\n");
+    assert!(
+        text.contains("no-such-rule") && text.contains("matched no finding"),
+        "{text}"
+    );
+}
+
+#[test]
+fn the_separator_suppresses_without_a_warning() {
+    let text = scan(&format!("{LINE} -- env var name\n"));
+    assert!(text.contains("Found 0 issues"), "{text}");
+    assert!(!text.contains("matched no finding"), "{text}");
+}