Skip to content

Commit 7767ba2

Browse files
committed
Prove the tree side too, and reject rules that silently widen
The review found a demonstrated false-parity path the design missed. Run as a script, sys.path[0] is tools/differential/ -- which holds no nameparser -- so PYTHONPATH outranks the editable install, and compare.py imported a released wheel while believing it read the checkout. PEP 723 does not save the worker either: PYTHONPATH precedes site-packages inside uv's own environment. Measured: with a released 2.0.0 on PYTHONPATH the run reported 'intentional diffs: 0' and exited 0, BOTH halves of the baseline tell passing -- the version matched and the path was outside REPO_ROOT because it was outside the repo. The design proved which library answered as the BASELINE and took the tree on faith. Now the tree is checked against REPO_ROOT and printed beside the baseline, and the worker's env has PYTHONPATH/PYTHONHOME stripped. validate_rules only checked key PRESENCE, so five shapes that widen a rule passed it: an empty or empty-matching name_regex (which sorts FIRST and shadows the whole ledger), a fields list naming every role, a wrong-typed or misspelled key (classify skips the bad half and the rule matches on the other alone), a fields entry that is not a role, and an uncompilable pattern -- which raised mid-run, after the worker pass, in a traceback naming neither file nor rule. Tests: main() had no composition coverage at all. Mutating its verdict to a bare 0, or deleting its validate_rules/sorted_rules/tree-check calls, left every test passing. A faked-worker fixture pins all four. Also corrects seven claims measured false: a hangul name labelled katakana, a README statement that file order does not decide (the new ledger depends on it doing so), an ordering justification naming a string only one rule matches, a rule claiming a name the rule above takes first, 'fully exercised' for vocabulary only 3/17 covered, and longest-first cited as a mechanism the trailing anchor provides.
1 parent b30f174 commit 7767ba2

4 files changed

Lines changed: 364 additions & 31 deletions

File tree

tests/v2/test_differential.py

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,3 +243,194 @@ def test_v2_fields_matches_the_Role_enum() -> None:
243243
is invisible on the v2 surface -- silent under-coverage, exit 0."""
244244
from nameparser import Role
245245
assert compare.V2_FIELDS == tuple(str(r) for r in Role)
246+
247+
248+
# The malformed-rule family. Every row is a way a rule can silently
249+
# match MORE than its author meant, which is how a real regression
250+
# becomes a classified diff and a green run. Parametrized rather than
251+
# written one-by-one because a guard added to one member of this family
252+
# belongs on all of it.
253+
@pytest.mark.parametrize("rule,expect", [
254+
({}, "no string 'issue'"),
255+
({"issue": ""}, "no string 'issue'"),
256+
({"issue": "x"}, "neither 'name_regex' nor 'fields'"),
257+
# a misspelled key is not ignored -- it deletes that half of the
258+
# narrowing and the rule matches on the other half alone
259+
({"issue": "x", "name_regex": ",", "field": ["given"]}, "unknown key"),
260+
# wrong types: classify skips them, so the rule silently widens
261+
({"issue": "x", "name_regex": ["a"], "fields": ["given"]},
262+
"non-string 'name_regex'"),
263+
({"issue": "x", "name_regex": "a", "fields": "given"},
264+
"not a list of strings"),
265+
# an empty pattern matches every name, and name_regex rules sort
266+
# FIRST, so it would shadow the whole ledger
267+
({"issue": "x", "name_regex": ""}, "matches the empty string"),
268+
({"issue": "x", "name_regex": "(?:)"}, "matches the empty string"),
269+
# uncompilable: without this it raises mid-run, after the worker
270+
({"issue": "x", "name_regex": "Smith("}, "invalid 'name_regex'"),
271+
({"issue": "x", "fields": []}, "empty 'fields'"),
272+
({"issue": "x", "fields": ["famly"]}, "not roles"),
273+
# facade vocabulary is not role vocabulary; it would never match
274+
({"issue": "x", "fields": ["first"]}, "not roles"),
275+
({"issue": "x", "fields": ["title", "given", "middle", "family",
276+
"suffix", "nickname", "maiden",
277+
"_ambiguities"]}, "every role"),
278+
])
279+
def test_validate_rules_rejects_a_rule_that_would_silently_widen(
280+
rule: dict, expect: str) -> None:
281+
with pytest.raises(SystemExit, match=expect):
282+
compare.validate_rules([rule], "expected_since_2.0.0.toml")
283+
284+
285+
def test_validate_rules_accepts_the_shipped_ledgers() -> None:
286+
"""The guards above must not be so strict they reject real rules."""
287+
import tomllib
288+
ledgers = sorted(_TOOLS.glob("expected_since_*.toml"))
289+
assert ledgers, "no ledgers found; this test would pass vacuously"
290+
for ledger in ledgers:
291+
rules = tomllib.loads(
292+
ledger.read_text(encoding="utf-8")).get("change", [])
293+
assert rules, f"{ledger.name} has no [[change]] rules"
294+
compare.validate_rules(rules, ledger.name)
295+
296+
297+
def test_ambiguities_is_a_legal_field_name() -> None:
298+
"""A SEGMENTATION-only diff is facade-identical by construction, so
299+
this pseudo-field is the only name that can classify it -- and the
300+
2.0 ledger's first rule depends on it."""
301+
compare.validate_rules(
302+
[{"issue": "x", "fields": ["_ambiguities"]}], "ledger.toml")
303+
304+
305+
def _run_main(tmp_path, monkeypatch, ledger_body: str,
306+
baseline_facade: dict) -> tuple[int, str]:
307+
"""Drive main() end to end with a faked baseline worker.
308+
309+
No uv, no network. The helper exists because every unit test above
310+
proves a helper WORKS while none proves main() calls it -- and in a
311+
gate, the composition is the part that can go silently permissive.
312+
"""
313+
import sys
314+
corpus = tmp_path / "corpus_x.jsonl"
315+
corpus.write_text('"John Smith"\n', encoding="utf-8")
316+
(tmp_path / "expected_since_1.4.0.toml").write_text(
317+
ledger_body, encoding="utf-8")
318+
monkeypatch.setattr(compare, "HERE", tmp_path)
319+
monkeypatch.setattr(
320+
compare, "_run_worker",
321+
lambda v, w, n: ({"__version__": v,
322+
"__file__": "/wheel/nameparser/__init__.py"},
323+
[{"facade": baseline_facade}]))
324+
monkeypatch.setattr(sys, "argv", ["compare.py", "--baseline", "1.4.0",
325+
"--corpus", str(corpus)])
326+
import io
327+
import contextlib
328+
buf = io.StringIO()
329+
with contextlib.redirect_stdout(buf):
330+
code = compare.main()
331+
return code, buf.getvalue()
332+
333+
334+
#: 'John Smith' with the family name altered, so the tree disagrees on
335+
#: exactly one role. The facade calls it `last`; the report and any rule
336+
#: must call it `family`.
337+
_DIFFERS = {"title": "", "first": "John", "middle": "", "last": "SMYTHE",
338+
"suffix": "", "nickname": "", "maiden": ""}
339+
340+
341+
def test_main_exits_1_and_reports_an_unclassified_diff(
342+
tmp_path, monkeypatch) -> None:
343+
"""The gate's entire verdict. Nothing else pins it: mutating the
344+
return to a bare 0 leaves every other test in this file passing,
345+
and the harness would report unexplained diffs on stdout while
346+
exiting 0 forever -- read by exit code, that is silence."""
347+
code, out = _run_main(
348+
tmp_path, monkeypatch,
349+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n', _DIFFERS)
350+
assert code == 1
351+
assert "UNEXPLAINED 'John Smith'" in out
352+
353+
354+
def test_main_reports_the_unexplained_field_under_its_role_name(
355+
tmp_path, monkeypatch) -> None:
356+
"""The block exists to be copy-pasted into a ledger rule, so the
357+
label it prints must be the label a rule needs. The facade calls
358+
this role `last`; a rule saying `last` never matches."""
359+
_, out = _run_main(
360+
tmp_path, monkeypatch,
361+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n', _DIFFERS)
362+
assert "family:" in out and "last:" not in out
363+
364+
365+
def test_main_exits_0_when_every_diff_is_claimed(
366+
tmp_path, monkeypatch) -> None:
367+
code, out = _run_main(
368+
tmp_path, monkeypatch,
369+
'[[change]]\nissue = "claimed"\nfields = ["family"]\n', _DIFFERS)
370+
assert code == 0
371+
assert "UNEXPLAINED" not in out
372+
assert "## claimed (1)" in out
373+
374+
375+
def test_main_validates_the_ledger_before_running_anything(
376+
tmp_path, monkeypatch) -> None:
377+
"""validate_rules has its own tests; this pins that main CALLS it.
378+
Deleting the call leaves those tests passing while a match-anything
379+
rule shadows the ledger."""
380+
with pytest.raises(SystemExit, match="matches the empty string"):
381+
_run_main(tmp_path, monkeypatch,
382+
'[[change]]\nissue = "wide"\nname_regex = ""\n', _DIFFERS)
383+
384+
385+
def test_main_sorts_rules_so_file_order_is_not_load_bearing(
386+
tmp_path, monkeypatch) -> None:
387+
"""A broad fields-only rule written FIRST must not claim a diff the
388+
specific name_regex rule below it owns. Deleting main's
389+
_sorted_rules call leaves _sorted_rules' own test passing."""
390+
_, out = _run_main(
391+
tmp_path, monkeypatch,
392+
'[[change]]\nissue = "broad"\nfields = ["family"]\n'
393+
'[[change]]\nissue = "specific"\nname_regex = "Smith"\n', _DIFFERS)
394+
assert "## specific (1)" in out and "broad" not in out
395+
396+
397+
def test_check_tree_accepts_the_checkout_and_rejects_anything_else(
398+
tmp_path) -> None:
399+
"""The tree side is the half that had no proof at all: the baseline
400+
gets a pinned wheel, a temp dir and a version tell, while the tree
401+
was a bare import trusted on sight."""
402+
inside = _TOOLS.parents[1] / "nameparser" / "__init__.py"
403+
assert compare._check_tree(str(inside)) == inside.resolve()
404+
with pytest.raises(SystemExit, match="outside this checkout"):
405+
compare._check_tree(str(tmp_path / "nameparser" / "__init__.py"))
406+
407+
408+
def test_main_aborts_when_the_tree_side_is_not_the_checkout(
409+
tmp_path, monkeypatch) -> None:
410+
"""Pins that main CALLS the tree check, not merely that the check
411+
works. Measured 2026-08-05: with a released 2.0.0 on PYTHONPATH,
412+
compare.py imported THAT and reported `intentional diffs: 0`,
413+
exit 0 -- both halves of the baseline tell passing. Run as a
414+
script, sys.path[0] is tools/differential/, which holds no
415+
nameparser, so PYTHONPATH outranks the editable install.
416+
417+
REPO_ROOT is moved rather than the module, because relocating the
418+
import is what the trap does and this reproduces its EFFECT: the
419+
tree's nameparser is no longer under the root it must be under.
420+
"""
421+
monkeypatch.setattr(compare, "REPO_ROOT", tmp_path)
422+
with pytest.raises(SystemExit, match="outside this checkout"):
423+
_run_main(tmp_path, monkeypatch,
424+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n', _DIFFERS)
425+
426+
427+
def test_worker_env_strips_the_import_path_overrides(monkeypatch) -> None:
428+
"""PEP 723 isolation does not survive PYTHONPATH -- it precedes
429+
site-packages, so a directory named there shadows the pinned wheel
430+
inside uv's own environment."""
431+
monkeypatch.setenv("PYTHONPATH", "/somewhere/else")
432+
monkeypatch.setenv("PYTHONHOME", "/elsewhere")
433+
monkeypatch.setenv("PATH", "/usr/bin")
434+
env = compare._worker_env()
435+
assert "PYTHONPATH" not in env and "PYTHONHOME" not in env
436+
assert env["PATH"] == "/usr/bin", "the rest of the env must survive"

tools/differential/README.md

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,11 @@ directory outside the worktree. Its first output line is a version
5757
tell, and `compare.py` aborts before comparing anything if the wrong
5858
version answered or if the module resolved inside the checkout.
5959

60-
A rule's `fields` names roles the way `Role` does — `title`, `given`,
61-
`middle`, `family`, `suffix`, `nickname`, `maiden` — whichever surface
62-
the diff came from. The facade reports `first`/`last`; those are
60+
A rule's `fields` names roles the way `Role` does, whichever surface
61+
the diff came from, plus the pseudo-field `_ambiguities` for a change
62+
in reported `AmbiguityKind`s. The roster is not restated here: it is
63+
`Role`'s members, `validate_rules` rejects anything outside them, and
64+
a copy in prose is a copy that goes stale when a role is added. The facade reports `first`/`last`; those are
6365
canonicalized on the way in, and the `UNEXPLAINED` block prints the
6466
canonical name so what you read is what you write.
6567

@@ -215,10 +217,14 @@ matches only if the observed diff fields are a subset of this list).
215217
Keep both as tight as the actual diff allows -- a loose rule can mask
216218
a real regression.
217219

218-
Rules are sorted most-specific-first before matching -- a `name_regex`
220+
Rules are sorted most-specific-first before matching: a `name_regex`
219221
rule outranks a `fields`-only one (which is broad by construction)
220-
wherever both match -- so file order does not decide which rule claims
221-
a diff.
222+
wherever both match. **Within a tier, file order decides.** That is
223+
not a detail -- every rule in `expected_since_2.0.0.toml` carries a
224+
`name_regex`, so they all sit in one tier and the order they are
225+
written in settles every tie between them. Append a rule to the bottom
226+
of a file only after checking that nothing above it already claims the
227+
diff you meant it for.
222228

223229
Some entries in the seed list are for behavior families that a
224230
particular corpus happens not to contain any example of (e.g. custom
@@ -231,7 +237,12 @@ ready the moment a matching string is added to the corpus.
231237

232238
The corpora run under the **default policy**, so any behavior gated
233239
behind a non-default `Policy` field is invisible here. Default
234-
*vocabulary*, by contrast, is fully exercised.
240+
*vocabulary* is a different matter: it is fully in EFFECT, never
241+
gated off the way a `Policy` field is, so a change to it can show up
242+
here. That is not the same as coverage -- only 3 of the 17 shipped
243+
`maiden_markers` and 8 of the 15 `honorific_tails` appear anywhere in
244+
the corpora (measured 2026-08-05), so an entry no corpus name
245+
exercises is as invisible as an opt-in policy.
235246

236247
Two independent mechanisms put a birth surname in `maiden`, and only
237248
one is opt-in (measured 2026-08-05):

0 commit comments

Comments
 (0)