Reject a ledger rule that narrows by name and not by role - #457
Merged
Conversation
The symmetric twin of #451's ban. Such a rule narrows by name and by nothing else, so on any name its regex reaches it claims every diff shape there is -- measured, 127 of 127. #452 is what made it urgent rather than untidy. over_declared_rules skips a rule with no `fields`, correctly, since one declaring no roles cannot over-declare them -- so deleting the line is the cheapest way to silence an OVER-DECLARED failure and the most permissive thing that can be done to the rule at the same time. The #372 failure mode, reached by following a gate error message. The new message says so, and says not to reach for #451's banned shape instead. Free on #451's own terms: measured, 0 of 179 rules across the three ledgers have it, and all three still load. Placed at the END of the `fields` family beside #451's, so neither pre-empts the precise messages for a MALFORMED `fields` -- that placement was itself a #453 review finding, and this check has the same hazard in reverse. Twenty test fixtures gained a `fields`; intent is preserved in every one -- they are `ZZZ` rules that match nothing whatever their roles, a `""` rule that hits the sentinel check first, and `Smith` rules whose {family} diff the added roles still admit. Nine of the twenty already carried a `fields` on the next concatenated line and my first pass duplicated the key; caught by TOMLDecodeError and removed. test_a_rule_with_a_regex_and_no_fields_or_both_stays_legal pinned this shape as LEGAL when #451 landed. It is inverted rather than deleted, so the change of status is visible in the history rather than silent. Proved able to fail: disabling the check fails the new test. 5350 passed; gates 229/0, 194/0, 102/0. Closes #456 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #457 +/- ##
=======================================
Coverage 98.64% 98.64%
=======================================
Files 45 45
Lines 3177 3177
=======================================
Hits 3134 3134
Misses 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
One real coverage loss and two falsified sentences, all from the focused review of #457. test_main_compares_the_v2_surface_from_baseline_2_0 stopped catching its own mutation. Its diff IS the ambiguity-only one, and the blanket `fields = ["family"]` my fixture sweep gave it refuses that shape, so the ZZZ rule stayed inert even with name narrowing disabled: measured, that mutant failed 11 tests on master and 10 here. The fixture now declares `_ambiguities`, the role its diff actually moves, and the mutant is back to 11. This is exactly the weakening the sweep's own commit message claimed not to have caused, in the one fixture whose diff is not {family}. "127 of 127" is wrong at both 2.x baselines, including the default. _RULE_FIELDS has eight members, and from 2.0 on main unions `_ambiguities` into the same diff set, so the banned shape claims 255 there and 127 below. A live count with no date, quoted in an error message a contributor reads while a 2.1.0 gate is failing. Both figures now, in the message, the README and decisions.md. validate_rules' docstring said the fields-without-name_regex check was "the family's sharpest example: no other malformed shape can widen invisibly". #456 is the counterexample, and worse -- over_declared_rules skips a fieldless rule, so nothing narrows it either. The paragraph names both now. Also: `dormant` gains a pin, since disabling this check otherwise failed exactly one test where #451's fails three; the README's "It may narrow further with `fields`" is folded into the required list rather than left contradicting the sentence eight lines below it; over_declared_rules' and classify's docstrings stop presenting the banned shape as legal; the decisions.md entry moves after the #452 entry it cites, restoring date order; and the inversion note moves to the assertion that was actually inverted, where a `git log -L` finds it. 5350 passed; gates 229/0, 194/0, 102/0. Refs #456 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #456. Found by the #455 review; the symmetric twin of #451's ban.
Measured: such a rule claims 127 of 127 possible diff shapes on any name its regex reaches. It narrows by name and by nothing else.
Why #452 made this urgent rather than untidy
over_declared_rulesskips a rule with nofields— correctly, since one declaring no roles cannot over-declare them. But that turns deleting the line into an opt-out: it is the cheapest way to silence anOVER-DECLAREDfailure and the most permissive thing that can be done to the rule, at the same time. The #372 failure mode, reached by following a gate error message.The new message says exactly that, and warns against the obvious wrong correction — reaching for
fieldswhile dropping the regex lands on the shape #451 banned. The two bans are each other's trap.Free to enforce
All three still load. Same argument
validate_rulesalready makes for #451's ban in its own docstring.The symmetry is the point
validate_rulesnow rejects a rule carrying neither key, one carryingfieldswithoutname_regex(#451), and one carryingname_regexwithoutfields(#456). After this, every ledger rule carries both, and the three rejections read as one rule with one reason: a rule narrows by name and by role, or it is not a rule.Placement
At the END of the
fieldsfamily, beside #451's. That matters: written under the neither-key check, #451's version pre-empted the three checks that buy precise messages for a malformedfields— an empty one reported the #451 message instead of"empty 'fields'". That was a #453 review finding, and this check has the same hazard in reverse.The test fixture ripple, and one thing worth flagging
Twenty fixtures gained a
fields. Intent is preserved in every one — they areZZZrules that match nothing whatever their roles, a""rule that hits the sentinel check first, andSmithrules whose{family}diff the added roles still admit.Nine of the twenty already carried a
fieldson the next concatenated line, and my first pass duplicated the key.tomllibcaught it withCannot overwrite a value; removed.test_a_rule_with_a_regex_and_no_fields_or_both_stays_legalpinned this shape as legal when #451 landed. It is inverted rather than deleted, so the change of status is visible in the history rather than silent — a test quietly disappearing is how a decision stops being reviewable.Verification
Proved able to fail: disabling the check fails the new test.
tests/v2/5350 passed, 156 skipped, 1 xfailed. Gates 1.4.0229/0, 2.0.0194/0, 2.1.0102/0, noOVER-DECLARED.Decided, not deferred
No escape hatch for a "genuinely unbounded" rule — declined until one appears, the same call #452 made.
dormantcovers "explains nothing"; nothing covers "explains everything", and no such rule exists. If one ever does it should argue for a key the waydormantwas argued for in #373, rather than finding one already built.🤖 Generated with Claude Code