From d6f2c42685d7c08496c7553ea334efe10363afaa Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 21 Aug 2026 00:45:34 +0900 Subject: [PATCH] Refuse a require_regexp that satisfies itself, which is the same accident going the other way `validate_no_self_match` covered `regexp` and stopped there. Both are content searches over a corpus that contains the rule's own declaration, and the same accident reaches them in opposite directions: regexp must find nothing, so a self-match is a finding that is always there require_regexp must find something, so a self-match is a PASS that is always there The second is the quieter one and the worse one. Reproduced: a rule requiring `Copyright` in every file reports `bad.txt` and never reports `policy/principles.toml`, because that file carries the word on its own `require_regexp` line. It meets the requirement by naming it. It is the one file in the corpus that cannot fail the rule, whatever else it does or stops doing, and nothing says so -- a loud finding about the wrong file gets read, and a file that has exempted itself is never mentioned again. The message is not shared. The three cures are the same, and the sentence before them is the opposite: a reader told that the file will be REPORTED, when what actually happens is that it is exempt, goes looking for a finding that is not there. Measured over the same 82 policy files: 11 `require_regexp` rules, none of them self-satisfying with the policy file in scope. Nothing that passes today starts failing, and the fleet sweep with the release binary confirms it. 551 tests pass, clippy and fmt clean, and every lefthook pre-commit command was run directly -- including the Python suite, which is what caught the exit-code assumption in the last change. --- src/config.rs | 98 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 85 insertions(+), 13 deletions(-) diff --git a/src/config.rs b/src/config.rs index bbd5059..46e75d7 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2264,8 +2264,17 @@ fn validate_no_self_match(root: &Path, policy_path: &Path, rules: &[Rule]) -> Re if rule.origin != Origin::Own { continue; } - let Some(pattern) = rule.regexp.as_deref() else { - continue; + // Both content searches, and the same accident reaches them in opposite + // directions. `regexp` must find nothing, so matching its own + // declaration is a finding that is always there. `require_regexp` must + // find something, so matching its own declaration is a pass that is + // always there. The second is the quieter of the two and the worse: a + // loud finding about the wrong file gets read, and a file that has + // exempted itself from a requirement is never mentioned again. + let subject = match (rule.regexp.as_deref(), rule.require_regexp.as_deref()) { + (Some(pattern), _) => SelfMatch::Refuses(pattern), + (None, Some(pattern)) => SelfMatch::Requires(pattern), + (None, None) => continue, }; if !crate::selection::selects(root, rule, relative)? { continue; @@ -2273,27 +2282,57 @@ fn validate_no_self_match(root: &Path, policy_path: &Path, rules: &[Rule]) -> Re let Some(section) = declaration_of(&text, &rule.id) else { continue; }; - let query = crate::engine::Query::from_files(pattern, rule.files()); + let query = crate::engine::Query::from_files(subject.pattern(), rule.files()); let hits = crate::engine::search_text(§ion, &query, &rule.id)?; if let Some(hit) = hits.first() { return Err(Fatal::at( policy_path, - format!( - "rule {:?} matches its own declaration ({:?}), and it selects the file that \ - declaration is in. Every run will report this file as violating this rule, \ - naming a line that is the rule rather than anything in the tree. Exclude \ - the policy file from this rule with `files.exclude`, narrow `files.include` \ - to what the rule is about, or anchor the pattern so it cannot match the key \ - it is written under.", - rule.id, - hit.text.trim() - ), + subject.message(&rule.id, hit.text.trim()), )); } } Ok(()) } +/// Which way a rule's own declaration reaches its pattern. +enum SelfMatch<'a> { + /// `regexp`: the match is a finding, so a self-match is a standing false one. + Refuses(&'a str), + /// `require_regexp`: the match is the pass, so a self-match is a standing + /// exemption for the one file that granted it. + Requires(&'a str), +} + +impl SelfMatch<'_> { + const fn pattern(&self) -> &str { + match self { + Self::Refuses(pattern) | Self::Requires(pattern) => pattern, + } + } + + /// The cures are the same three in both directions, and the sentence before + /// them is not: a reader who is told the wrong thing about what goes wrong + /// fixes the wrong rule. + fn message(&self, id: &str, matched: &str) -> String { + let cures = "Exclude the policy file from this rule with `files.exclude`, narrow \ + `files.include` to what the rule is about, or anchor the pattern so it \ + cannot match the key it is written under."; + match self { + Self::Refuses(_) => format!( + "rule {id:?} matches its own declaration ({matched:?}), and it selects the file \ + that declaration is in. Every run will report this file as violating this rule, \ + naming a line that is the rule rather than anything in the tree. {cures}" + ), + Self::Requires(_) => format!( + "rule {id:?} has a `require_regexp` that matches its own declaration \ + ({matched:?}), and it selects the file that declaration is in. That file \ + satisfies the requirement by naming it, so it is exempt from this rule forever \ + -- whatever else it does or stops doing. {cures}" + ), + } + } +} + /// The lines of one rule's own `[rule.]` table. /// /// Its sub-tables belong to it -- `[rule..files]` is where `exclude` is @@ -3414,6 +3453,39 @@ mod tests { assert!(text.contains("anchor"), "{text}"); } + #[test] + fn a_require_regexp_that_satisfies_itself_is_refused() { + // The inverted case, and the quieter one. `regexp` matching its own + // declaration is a finding that is always there and gets read. + // `require_regexp` matching it is a PASS that is always there: the + // policy file meets the requirement by naming it, so it is the one file + // in the corpus that can never fail the rule, and nothing ever says so. + let error = policy_from( + "[rule.every-file]\nrequire_regexp = 'Copyright'\nmessage = \"m\"\n[rule.every-file.files]\ninclude = [\".\"]\n", + ) + .unwrap_err(); + let text = error.to_string(); + assert!(text.contains("every-file"), "{text}"); + assert!(text.contains("require_regexp"), "{text}"); + // The message says what goes wrong HERE, which is the opposite of what + // goes wrong for `regexp`: a reader told the wrong one fixes nothing. + assert!(text.contains("exempt"), "{text}"); + assert!(!text.contains("violating this rule"), "{text}"); + // And the same three cures. + assert!(text.contains("files.exclude"), "{text}"); + } + + #[test] + fn a_requirement_the_policy_file_does_not_declare_still_loads() { + // The narrowing that fixes it, and the case that must keep working: a + // requirement whose pattern is nowhere in its own section has nothing + // to exempt. + policy_from( + "[rule.every-file]\nrequire_regexp = 'Copyright'\nmessage = \"m\"\n[rule.every-file.files]\ninclude = [\".\"]\nexclude = [\"**/rg-policy.toml\"]\n", + ) + .unwrap(); + } + #[test] fn an_anchored_pattern_cannot_match_the_key_it_is_written_under() { // `^Status:` does not match `regexp = '^Status:...'` -- that line begins