Refuse a rule that matches its own declaration and selects the file it is in - #73
Conversation
…t is in A policy file is a tracked file, so a rule's own `regexp` sits inside the corpus that rule scans. An unanchored literal therefore matches the line it is written on, and the run reports the policy file as violating the rule the policy file defines -- a real path, a real line number, and nothing in the tree. Reading that finding means working out that the rule is describing itself, and nothing in the message says so. WHAT THE MEASUREMENT CHANGED ABOUT THE CHECK. Over 82 policy files and 178 `regexp` rules in one workspace, 54 rules matched their own declaration textually and NONE of them selected the file it was written in: an `include` of `["cmd", "internal"]` with a `glob` of `["*.go"]` cannot reach `policy/`. Refusing on the text alone would have failed 54 rules that work. So the scope test comes first, and it is the same test `from_index` applies to every tracked path rather than a second reader of `files.*` -- see `selection::selects`, which is that test asked about one path. With the scope test, the count of rules in those 82 trees that this refuses is zero. Nothing that passes today starts failing. WHY IT IS A LOAD-TIME REFUSAL AND NOT A REPORT. At run time this is a violation like any other, and the tier that produces it cannot say what it is. `validate_shims` gives the argument for its own pair and it holds unchanged here: a load-time refusal is the only place either can be seen at all. The message names three cures, because a refusal that names none is a wall: exclude the policy file, narrow the include, or anchor the pattern. Ordered last, after `rule.validate`. A rule naming two checks is not yet a rule whose pattern means anything, and reporting a self-match on one answers a question the reader has not reached. Own rules only. A bundled set's rule is declared inside this binary and an `inherit.paths` rule in a file this rule may not select; neither has a declaration in the policy file to match. WHAT THE FLEET WAS DOING INSTEAD, WHICH IS WHY THIS IS WORTH A REFUSAL. Three workarounds have spread with nothing recording any of them as the answer: a narrow `include` that happens to miss the policy dir, an explicit `exclude = ["**/policy/principles.toml"]`, and breaking the literal with a one-character class -- `\bYubi[K]ey\b`, `Yubi[c]o`, `Windows Hell[o]`, `Touch I[D]`, `Bitward[e]n`, nine of them in a single pattern that is unreadable because of the workaround rather than because of the problem. The third is also applied where it does nothing. `^Sta[t]us:` appears in three repositories and the dodge is unnecessary in all three: the pattern is anchored, so it cannot match `regexp = '^Status:...'` -- that line begins with `regexp`. A defensive edit that changes nothing, transcribed three times, is what "the engine gives no signal either way" looks like from outside. EIGHT TEST FIXTURES CHANGED, AND EACH ONE IS AN INSTANCE. Five wrote an unanchored literal over the whole tree -- `regexp = 'SHOUT'`, `regexp = 'hunter2'`, `regexp = "local"` -- which is exactly the shape this refuses. Their subjects are provenance, shadowing, redaction and the shim seam, none of which the added `files.exclude` or narrowed `include` touches. Three more in `config.rs` needed nothing once the check moved after `rule.validate`. 549 tests pass, clippy and fmt clean, and `uphold scan` and `uphold check` pass on this repository.
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (96.74%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
+ Coverage 90.27% 90.34% +0.07%
==========================================
Files 35 35
Lines 11025 11147 +122
==========================================
+ Hits 9953 10071 +118
- Misses 1072 1076 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… other eight `tests/test_review.py` builds a policy carrying `regexp = 'TODO'` over the whole tree. That is the shape the new load-time refusal catches: the literal appears on its own `regexp` line, so the rule reports the file it is declared in. The test is about a claim naming a rule that does not exist, and it asserted exit 1 -- a policy violation. A load refusal is a `Fatal` and exits 2, the same as every other validator in `load`, so the assertion failed on the code rather than on anything it was testing. Found by the three hook jobs in CI, which run this repository's own lefthook commands and were the only jobs that reached this suite. Every consumer job passed, which is the half that matters: nothing outside this repository changed behaviour.
2 participants
Closes #72, taking option B from that issue: refuse at load, name the cause, leave what a rule scans unchanged.
The defect
A policy file is a tracked file, so a rule's own
regexpsits inside the corpus it scans. An unanchored literal matches the line it is written on, and the run reports the policy file as violating the rule that file defines:A real path, a real line, and nothing in the tree. Now:
The scope test is the whole design, and it came from measuring
Across 82 policy files and 178
regexprules, 54 matched their own declaration textually and none selected the file it was written in. Anincludeof["cmd", "internal"]withglob = ["*.go"]cannot reachpolicy/. Refusing on the text alone would have failed 54 working rules.So the check asks scope first, and asks it through
selection::selects-- the same two testsfrom_indexapplies to every tracked path (under an include prefix, not matched by an exclusion), asked about one path. Not a second reader offiles.*that could disagree with the scanner.include_prefixesis split out ofsearch_rootsso both use one definition;search_rootskeeps the side effects that belong to a real search (the warning about an include that is not there, the refusal of one outside the tree), which are not true of a caller that only wants to know whether a path is in scope.Measured blast radius: zero. 82 policy trees scanned with the built binary, none newly refused.
Ordering
After
rule.validate. A rule naming two checks is not yet a rule whose pattern means anything, and reporting a self-match on one answers a question the reader has not reached -- that surfaced as 7 test failures when it ran first, and 5 of them went away on the reorder alone.Own rules only
A bundled set's rule is declared inside this binary; an
inherit.pathsrule lives in a file this rule may not select. Neither has a declaration in the policy file to match. Thethe_effective_rules_are_what_inheritance_resolved_tofixture keeps itspolicy/extra.tomlrule untouched for exactly that reason.Eight fixtures changed, each one an instance
Five wrote an unanchored literal over the whole tree --
regexp = 'SHOUT',regexp = 'hunter2',regexp = "local"-- which is the shape this refuses. Their subjects are provenance, shadowing, redaction and the shim seam; the addedfiles.exclude(the same line the bundledcredentialsset carries) touches none of them.That the fixtures reproduced the defect while testing something else is part of the argument for the check.
What this makes deletable
^Sta[t]us:appears in three repositories and the dodge is unnecessary in all three -- the pattern is anchored, so it cannot matchregexp = '^Status:...', because that line begins withregexp. Verified. The nine dodges in\bYubi[K]ey\b|\bYubi[c]o\b|...are load-bearing today and become anexcludeline instead, which is readable.This does not suppress anything, so it does not make any of them go away on its own. It makes the moment you need one discoverable rather than silent, which is what option B is.
Verification
cargo test-- 549 pass, 0 fail (545 before, 4 added)cargo clippy --all-targetsandcargo fmt --check-- cleanuphold scananduphold checkpass on this repositoryNew tests: the refusal fires and names all three cures; an anchored pattern loads; a self-matching pattern that cannot reach the policy file still loads (three narrowings --
include,glob,exclude); anddeclaration_ofstops at the next table that is not its own sub-table.lefthook is not on PATH here, so its hooks did not run on the commit; the gates they invoke were run directly and are listed above.