Skip to content

Commit 0847ecf

Browse files
authored
Merge pull request #457 from derek73/claude/456-require-fields
Reject a ledger rule that narrows by name and not by role
2 parents 6ac31f4 + c856c33 commit 0847ecf

4 files changed

Lines changed: 112 additions & 32 deletions

File tree

docs/design/decisions.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,7 @@ Decisions that landed:
636636
- 2026-08-28 #452 — a rule's declared `fields` must EQUAL the union of the diffs it explains, checked by `compare.py` at the end of every run and failing it like an unexplained diff. The statement is exact rather than heuristic, which is what makes it cheap: `classify()` already requires `declared >= union` for the rule to match the names it matches, so the only possible error is the other direction, and the union is simultaneously the check and the repair. Narrowing to it cannot orphan a name, since every name a rule explains contributed to it. Measured before landing: 3 of 67 explaining rules over-declared at 1.4.0, 5 of 58 at 2.0.0, 6 of 51 at 2.1.0 — all fourteen narrowed first, so the check was silent the day it arrived.
637637
- 2026-08-28 #452 — over-declaration is BASELINE-RELATIVE, and each ledger is measured on its own run rather than copied. `fix(#296) a lone post-comma credential is a suffix` declared `{family, given, suffix, title}` in all three ledgers: exactly exercised at 1.4.0, where v1 reads the pre-comma word as `first` and all four roles move, and over-declared at both 2.x baselines, where the same behaviour moves only `{suffix, title}` — which is all it declares in those two files now. A reader comparing the three sees one rule with TWO different field lists and should read that as correct rather than as drift. Two and not three, measured: the 2.x pair narrowed to the same set, so the split is 1.4.0 against both 2.x ledgers, not one list per file.
638638
- 2026-08-28 #452 — NO escape hatch, decided rather than deferred. `dormant` already covers the explains-nothing case in both directions, a rule with no `fields` has nothing to over-declare, and the ledger's own doctrine — "a rule that pre-claims shapes it has never seen is the #372 failure mode" — makes strictness the existing principle. Accepted cost, stated so it is not rediscovered as a surprise: the first rule that genuinely needs a wider declaration has to argue for a key the way `dormant` was argued for in #373, rather than reaching for one that already exists.
639+
- 2026-08-29 #456 — a rule carrying `name_regex` and no `fields` is REJECTED, the symmetric twin of #451's ban. It narrows by name and by nothing else, so on any name its regex reaches it claims every diff shape there is — measured, 255 shapes from baseline 2.0 on and 127 below it. #452 is what made it urgent rather than merely 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, which is the #372 failure mode reached by following a gate error message. Free to enforce on #451's own terms: measured, 0 of 179 rules across the three ledgers had the shape. After it, every rule carries both keys, and the three rejections in `validate_rules` read as one rule — narrow by name and by role, or it is not a rule. NO escape hatch for a "genuinely unbounded" rule, declined until one appears, same call and same reason as #452's.
639640

640641
Found rather than decided, and worth as much:
641642

tests/v2/test_differential.py

Lines changed: 65 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -262,13 +262,54 @@ def test_dormant_does_not_buy_an_exemption_from_the_ban() -> None:
262262
"test_ledger.toml")
263263

264264

265-
def test_a_rule_with_a_regex_and_no_fields_or_both_stays_legal() -> None:
266-
"""The neighbouring shapes #451 did NOT retire. `name_regex` alone
267-
still narrows by name; `name_regex` plus `fields` narrows by both.
268-
Only the fields-only shape -- no name narrowing at all -- is new
269-
to reject."""
270-
compare.validate_rules(
271-
[{"issue": "x", "name_regex": "Smith"}], "test_ledger.toml")
265+
def test_a_rule_with_a_regex_and_no_fields_is_rejected() -> None:
266+
"""#451's ban in mirror image (#456).
267+
268+
A rule with no `fields` narrows by name and by nothing else, so on
269+
any name its regex reaches it claims every diff shape there is --
270+
measured, every one of the 255 shapes
271+
eight roles allow at a 2.x baseline. #452 made that worse than it looks by giving
272+
the shape a second job: over_declared_rules skips a rule with no
273+
`fields`, correctly, since one declaring no roles cannot
274+
over-declare them. So deleting the `fields` line is the response to
275+
an OVER-DECLARED failure that takes the least thought, and it both
276+
silences the check and makes the rule maximally permissive.
277+
278+
Free to enforce, on the same terms as #451's: no rule in any
279+
shipped ledger has the shape, so the ban costs no migration.
280+
281+
This assertion is an INVERSION. Its other half pinned the
282+
regex-only shape as LEGAL when #451 landed -- "the neighbouring
283+
shapes #451 did NOT retire" -- and #456 retired it. Inverted rather
284+
than deleted, so the change of status is visible to a `git log -L`
285+
on the assertion rather than vanishing with the test.
286+
"""
287+
with pytest.raises(SystemExit, match="no 'fields'"):
288+
compare.validate_rules(
289+
[{"issue": "fix(x) a rule with no role narrowing",
290+
"name_regex": "Smith"}],
291+
"test_ledger.toml")
292+
# `dormant` buys no exemption here either, for the reason it buys
293+
# none from #451's ban: it is a claim about today's corpus, not a
294+
# bound on reach, and a regex-only rule sits at every diff shape
295+
# the moment one matching name arrives -- at which point its
296+
# dormancy claim is false too. Pinned because disabling the check
297+
# otherwise fails exactly one test (#457 review).
298+
with pytest.raises(SystemExit, match="no 'fields'"):
299+
compare.validate_rules(
300+
[{"issue": "fix(x) idle and unbounded",
301+
"name_regex": "Smith", "dormant": "a reason nobody faults"}],
302+
"test_ledger.toml")
303+
304+
305+
def test_a_rule_carrying_both_keys_is_the_only_legal_shape() -> None:
306+
"""What is left after the three bans, and there is exactly one.
307+
308+
`validate_rules` rejects neither key (it would match every diff),
309+
`fields` without `name_regex` (#451, no name narrowing), and
310+
`name_regex` without `fields` (#456, no role narrowing). One
311+
shape survives all three, and this is it.
312+
"""
272313
compare.validate_rules(
273314
[{"issue": "x", "name_regex": "Smith", "fields": ["given"]}],
274315
"test_ledger.toml")
@@ -502,7 +543,7 @@ def test_main_exits_1_and_reports_an_unclassified_diff(
502543
exiting 0 forever -- read by exit code, that is silence."""
503544
code, out = _run_main(
504545
tmp_path, monkeypatch,
505-
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n', _DIFFERS)
546+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\nfields = ["family"]\n', _DIFFERS)
506547
assert code == 1
507548
assert "UNEXPLAINED 'John Smith'" in out
508549

@@ -514,7 +555,7 @@ def test_main_reports_the_unexplained_field_under_its_role_name(
514555
this role `last`; a rule saying `last` never matches."""
515556
_, out = _run_main(
516557
tmp_path, monkeypatch,
517-
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n', _DIFFERS)
558+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\nfields = ["family"]\n', _DIFFERS)
518559
assert "family:" in out and "last:" not in out
519560

520561

@@ -536,7 +577,7 @@ def test_main_validates_the_ledger_before_running_anything(
536577
rule shadows the ledger."""
537578
with pytest.raises(SystemExit, match="matches every one of"):
538579
_run_main(tmp_path, monkeypatch,
539-
'[[change]]\nissue = "wide"\nname_regex = ""\n', _DIFFERS)
580+
'[[change]]\nissue = "wide"\nname_regex = ""\nfields = ["family"]\n', _DIFFERS)
540581

541582

542583
def test_main_rejects_a_broad_fields_only_rule_before_running_anything(
@@ -559,7 +600,7 @@ def test_main_rejects_a_broad_fields_only_rule_before_running_anything(
559600
_run_main(
560601
tmp_path, monkeypatch,
561602
'[[change]]\nissue = "broad"\nfields = ["family"]\n'
562-
'[[change]]\nissue = "specific"\nname_regex = "Smith"\n',
603+
'[[change]]\nissue = "specific"\nname_regex = "Smith"\nfields = ["family"]\n',
563604
_DIFFERS)
564605

565606

@@ -705,7 +746,7 @@ def test_main_aborts_when_the_tree_side_is_not_the_checkout(
705746
monkeypatch.setattr(compare, "REPO_ROOT", tmp_path)
706747
with pytest.raises(SystemExit, match="not from this checkout's source"):
707748
_run_main(tmp_path, monkeypatch,
708-
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n', _DIFFERS)
749+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\nfields = ["family"]\n', _DIFFERS)
709750

710751

711752
def test_worker_env_strips_the_import_path_overrides(
@@ -840,7 +881,13 @@ def test_main_compares_the_v2_surface_from_baseline_2_0(
840881
v2 = {**_SAME_V2, "_ambiguities": ["SEGMENTATION"]}
841882
code, out = _run_main(
842883
tmp_path, monkeypatch,
843-
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n',
884+
# `_ambiguities`, not `family`: this test's diff IS the
885+
# ambiguity-only one, and a `family` declaration refuses that
886+
# shape -- which left the rule inert even with name narrowing
887+
# disabled, costing this test the mutation it was written to
888+
# catch (#457 review). Declare the role the diff moves.
889+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n'
890+
'fields = ["_ambiguities"]\n',
844891
_SAME_FACADE, baseline="2.0.0", baseline_v2=v2)
845892
assert code == 1, "an ambiguity-only regression must not exit 0"
846893
assert "UNEXPLAINED 'John Smith'" in out
@@ -867,7 +914,7 @@ def test_main_reports_a_role_once_when_both_surfaces_moved(
867914
change shows on each; printing it twice would read as two findings."""
868915
_, out = _run_main(
869916
tmp_path, monkeypatch,
870-
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n',
917+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\nfields = ["family"]\n',
871918
{**_SAME_FACADE, "last": "SMYTHE"}, baseline="2.0.0",
872919
baseline_v2={**_SAME_V2, "family": "SMYTHE"})
873920
assert out.count("family:") == 1
@@ -878,7 +925,7 @@ def test_main_forwards_the_baseline_and_corpus_to_the_worker(
878925
"""Otherwise main could read the 2.0 ledger while comparing against
879926
1.4, or compare a truncated corpus, and every other test would pass."""
880927
_run_main(tmp_path, monkeypatch,
881-
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n',
928+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\nfields = ["family"]\n',
882929
_SAME_FACADE, baseline="2.0.0", baseline_v2=_SAME_V2)
883930
assert _WORKER_CALL == {"version": "2.0.0", "want_v2": True,
884931
"names": ["John Smith"]}
@@ -887,7 +934,7 @@ def test_main_forwards_the_baseline_and_corpus_to_the_worker(
887934
def test_main_asks_for_the_facade_alone_below_2_0(
888935
tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
889936
_run_main(tmp_path, monkeypatch,
890-
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n', _SAME_FACADE)
937+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\nfields = ["family"]\n', _SAME_FACADE)
891938
assert _WORKER_CALL["want_v2"] is False
892939

893940

@@ -928,7 +975,7 @@ def test_main_aborts_on_a_truncated_corpus(
928975
"""A corpus below its floor must stop the run, not shrink it."""
929976
with pytest.raises(SystemExit, match="below its floor"):
930977
_run_main(tmp_path, monkeypatch,
931-
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n',
978+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\nfields = ["family"]\n',
932979
_SAME_FACADE, floor=50)
933980

934981

@@ -939,7 +986,7 @@ def test_main_aborts_on_a_corpus_with_no_floor(
939986
tables use."""
940987
with pytest.raises(SystemExit, match="no entry in _CORPUS_FLOORS"):
941988
_run_main(tmp_path, monkeypatch,
942-
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n',
989+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\nfields = ["family"]\n',
943990
_SAME_FACADE, floor=None)
944991

945992

tools/differential/README.md

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -300,13 +300,20 @@ history rather than being edited into the next one's.
300300
Each `[[change]]` entry needs `issue` (a short label, ideally an
301301
issue number or `fix(<slug>)` matching a `tests/v2/cases.py`
302302
classification) and `name_regex` (searched against the raw input
303-
string). It may narrow further with `fields` (the diffing rule matches
304-
only if the observed diff fields are a subset of this list), which
305-
since #452 must also name EXACTLY the roles that rule's own diffs move
306-
-- see below. Keep both as tight as the actual diff allows -- a loose
307-
rule can mask a real regression. `name_regex` is REQUIRED since #451:
308-
`validate_rules` rejects a rule carrying `fields` and no `name_regex`,
309-
as it already rejected one carrying neither.
303+
string) AND `fields` (the diffing rule matches only if the observed
304+
diff fields are a subset of this list), which since #452 must also
305+
name EXACTLY the roles that rule's own diffs move -- see below. Keep both as tight as the actual diff allows -- a loose
306+
rule can mask a real regression. **Both keys are REQUIRED**, and each
307+
ban has its own issue: `validate_rules` rejects a rule carrying
308+
neither (it would match every diff), one carrying `fields` and no
309+
`name_regex` (#451 -- no name narrowing, so it claims every name whose
310+
diff fits its roles), and one carrying `name_regex` and no `fields`
311+
(#456 -- no role narrowing, so on any name its regex reaches it claims
312+
every diff shape there is, measured, 255 of them from baseline 2.0 on and 127 below it). The three are one
313+
rule with one reason: a rule narrows by name AND by role, or it is not
314+
a rule. Note the two bans are each other's obvious wrong answer --
315+
deleting `fields` to silence an over-declaration failure lands on
316+
#456's, and adding `fields` while dropping the regex lands on #451's.
310317

311318
**That closes the SHAPE, not the property.** A required `name_regex`
312319
is not a bound on how much a rule reaches: the only width check is the

tools/differential/compare.py

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -390,9 +390,13 @@ def validate_rules(rules: list[dict[str, object]], ledger: str) -> None:
390390
buy a precise message rather than safety; they are here because the
391391
family is easier to reason about whole than split by direction.
392392
393-
The `fields`-without-`name_regex` check belongs to the dangerous
394-
direction, and it is the family's sharpest example: no other
395-
malformed shape can widen invisibly. #451 is the rule that lived
393+
Two checks belong to the dangerous direction and are the family's
394+
sharpest examples, one per missing key -- #451 for a `fields` with
395+
no `name_regex`, #456 for a `name_regex` with no `fields`. An
396+
earlier version of this paragraph said the first was the only shape
397+
that could widen invisibly; #456 falsified that, and worse, since
398+
over_declared_rules skips a fieldless rule so nothing narrows it
399+
either. #451 is the rule that lived
396400
it -- no name narrowing, so it claimed every name whose diff fit
397401
its `fields`, and _CORPUS_CLAIMS (the guard tracking each rule's
398402
reach) recorded that reach as the whole corpus from the start, so
@@ -536,6 +540,24 @@ def validate_rules(rules: list[dict[str, object]], ledger: str) -> None:
536540
f"unrelated behavior families, with every guard green "
537541
f"the whole time. Narrow by name instead, or split this "
538542
f"into the rules the diffs actually need")
543+
if has_regex and not has_fields:
544+
raise SystemExit(
545+
f"{where} has 'name_regex' but no 'fields' (#456). It "
546+
f"narrows by name and by nothing else, so on any name "
547+
f"its regex reaches it claims EVERY diff shape there is -- "
548+
f"255 of them from baseline 2.0 on, where `_ambiguities` "
549+
f"joins the seven roles, and 127 below it. #452 makes "
550+
f"that worse than it looks: "
551+
f"over_declared_rules skips a rule with no 'fields', "
552+
f"correctly, since one declaring no roles cannot "
553+
f"over-declare them -- so deleting the line is the "
554+
f"cheapest way to silence an OVER-DECLARED failure and "
555+
f"the most permissive thing you can do to the rule at "
556+
f"the same time. Name the roles the diffs actually "
557+
f"move. Do NOT reach for the other shape instead: a "
558+
f"'fields' with no 'name_regex' is banned by #451 for "
559+
f"the mirror-image reason, and the two bans are each "
560+
f"other's obvious wrong answer")
539561

540562

541563
def validate_exclusions(entries: list[dict[str, object]],
@@ -675,8 +697,9 @@ def classify(name: str, diff_fields: set[str],
675697
rules -- so a new entry's blast radius is exactly the set of names
676698
it captures, independent of rule order.
677699
678-
An exclusion narrows by `name_regex` and optionally by `fields`,
679-
exactly as a rule does. Without `fields` it refuses any diff on a
700+
An exclusion narrows by `name_regex` and optionally by `fields`.
701+
The optionality is the exclusion's alone since #456: a RULE must
702+
now carry both. Without `fields` it refuses any diff on a
680703
matching name; with them it refuses only the reading it names, so a
681704
name whose parens mark a nickname to one rule and a suffix to
682705
another stays classifiable on the reading the exclusion is not
@@ -824,8 +847,10 @@ def over_declared_rules(
824847
can have one, and the test that pins this skip uses exactly that
825848
input, because the empty-union input cannot discriminate.) A rule
826849
with no `fields` declares no roles and so has nothing to
827-
over-declare; one with `fields` and no `name_regex` cannot exist
828-
since #451. And a rule that explained nothing is dormant_rules'
850+
over-declare -- and cannot exist since #456, which banned that
851+
shape precisely because this skip made deleting `fields` the
852+
cheapest way to silence the check. One with `fields` and no
853+
`name_regex` cannot exist since #451. And a rule that explained nothing is dormant_rules'
829854
too, which is the third `continue` below.
830855
831856
What this does NOT bound is a diff shape no single name produced.

0 commit comments

Comments
 (0)