Skip to content

Commit 016fead

Browse files
committed
Close the holes the re-review found in the previous fix
_check_tree asked "is this under the repo" when the invariant is "is this the source package". The checkout also contains .venv/, build/lib/ and dist/, any of which can hold a released wheel -- so PYTHONPATH=<repo>/build/lib was the same trap with the shadowing directory moved one level left, and uv never touches build/ to self-heal it. Two reviewers found it independently. Verified: both the original attack and the build/lib variant now abort. validate_rules had two guards that did not guard. The empty-string probe tested the wrong property: '.', '.+', r'\b' and '[\s\S]' all decline "" and still match every name in every corpus, and name_regex rules sort first, so one of them shadows the whole ledger. Replaced with a sentinel set sharing no script, vocabulary or punctuation. The "lists every role" check compared against the seven roles PLUS _ambiguities, so a seven-role list passed -- and below baseline 2.0 _ambiguities cannot enter a diff at all, making those seven the entire vocabulary. Demonstrated end to end: a total regression across every field reported as intentional, exit 0. Now checked against the seven. Tests: deleting env=_worker_env() left all 61 green, the same proved-helper/unproved-call-site shape as last round. main's entire v2 half was unreached -- _run_main hardcoded 1.4.0, so want_v2 forced False, the v2 diff union deleted, and the [v2 surface only] tag all survived, including a SEGMENTATION-only regression exiting 0. That diff shape is the whole stated reason to compare v2. All now pinned, plus _run_worker's four subprocess guards via a fake Popen. The tree check also moves ahead of the worker: it depends on nothing the worker produces, and validate_rules' own reasoning about not deferring a failure past the multi-minute pass applies to it. Prose: the file-order correction reached the README last round but not _sorted_rules' docstring or a test name, which both still asserted the retracted claim. "Parses fine, validates fine, and never matches" is now false in three places -- validate_rules rejects it. "Every check below is a way a rule can silently widen" was false for three checks whose own messages say they make a rule dead, not wider. The README documented two invocation traps while the code cross-referenced a third; it now documents three, including why the first predicate was wrong.
1 parent 516bd67 commit 016fead

4 files changed

Lines changed: 369 additions & 78 deletions

File tree

tests/v2/test_differential.py

Lines changed: 225 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,8 @@ def test_allowlist_for_a_baseline_with_no_ledger_is_a_hard_error() -> None:
8282

8383

8484
def test_name_regex_rules_sort_ahead_of_fields_only_rules() -> None:
85-
"""Most-specific-first, so file order stops being load-bearing."""
85+
"""Most-specific-first BETWEEN tiers. Within a tier the stable
86+
sort leaves file order deciding, which the 2.0 ledger relies on."""
8687
rules = [{"issue": "broad", "fields": ["first"]},
8788
{"issue": "specific", "name_regex": "Smith"}]
8889
assert [r["issue"] for r in compare._sorted_rules(rules)] \
@@ -180,10 +181,11 @@ def test_canonical_field_is_idempotent_on_role_names() -> None:
180181

181182

182183
def test_every_ledger_rule_names_roles_canonically() -> None:
183-
"""The trap this guards: a rule written in facade vocabulary parses
184-
fine, validates fine, and simply never matches -- the ledger grows
185-
an entry that does nothing, classification silently loosens, and
186-
nothing anywhere says so. Sweeps every ledger, so a new baseline's
184+
"""A rule written in facade vocabulary parses, and validate_rules
185+
now rejects it at startup ("not roles"). Before that guard it
186+
validated and then silently never matched -- the ledger growing an
187+
entry that did nothing. This keeps a sharper message than the
188+
generic role check, and sweeps every ledger, so a new baseline's
187189
file is covered the day it is added."""
188190
import tomllib
189191
ledgers = sorted(_TOOLS.glob("expected_since_*.toml"))
@@ -245,11 +247,14 @@ def test_v2_fields_matches_the_Role_enum() -> None:
245247
assert compare.V2_FIELDS == tuple(str(r) for r in Role)
246248

247249

248-
# The malformed-rule family. Every row is a way a rule can silently
250+
# The malformed-rule family. Most rows are a way a rule can silently
249251
# 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.
252+
# becomes a classified diff and a green run. Three rows are the
253+
# opposite -- an empty `fields`, a non-role name, a facade name -- and
254+
# make a rule that can never match; those fail loudly (the diff
255+
# surfaces as UNEXPLAINED) so their rows buy a precise message rather
256+
# than safety. Parametrized rather than written one-by-one because a
257+
# guard added to one member of this family belongs on all of it.
253258
@pytest.mark.parametrize("rule,expect", [
254259
({}, "no string 'issue'"),
255260
({"issue": ""}, "no string 'issue'"),
@@ -264,8 +269,19 @@ def test_v2_fields_matches_the_Role_enum() -> None:
264269
"not a list of strings"),
265270
# an empty pattern matches every name, and name_regex rules sort
266271
# 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"),
272+
({"issue": "x", "name_regex": ""}, "matches every one of"),
273+
({"issue": "x", "name_regex": "(?:)"}, "matches every one of"),
274+
# the shapes the empty-string probe let through: each declines ""
275+
# and still matches every name in every corpus
276+
({"issue": "x", "name_regex": "."}, "matches every one of"),
277+
({"issue": "x", "name_regex": ".+"}, "matches every one of"),
278+
({"issue": "x", "name_regex": r"\b"}, "matches every one of"),
279+
({"issue": "x", "name_regex": r"[\s\S]"}, "matches every one of"),
280+
# seven roles without _ambiguities: below baseline 2.0 that IS the
281+
# whole vocabulary, so it claims every diff
282+
({"issue": "x", "fields": ["title", "given", "middle", "family",
283+
"suffix", "nickname", "maiden"]},
284+
"all seven roles"),
269285
# uncompilable: without this it raises mid-run, after the worker
270286
({"issue": "x", "name_regex": "Smith("}, "invalid 'name_regex'"),
271287
({"issue": "x", "fields": []}, "empty 'fields'"),
@@ -274,7 +290,7 @@ def test_v2_fields_matches_the_Role_enum() -> None:
274290
({"issue": "x", "fields": ["first"]}, "not roles"),
275291
({"issue": "x", "fields": ["title", "given", "middle", "family",
276292
"suffix", "nickname", "maiden",
277-
"_ambiguities"]}, "every role"),
293+
"_ambiguities"]}, "all seven roles"),
278294
])
279295
def test_validate_rules_rejects_a_rule_that_would_silently_widen(
280296
rule: dict, expect: str) -> None:
@@ -302,26 +318,42 @@ def test_ambiguities_is_a_legal_field_name() -> None:
302318
[{"issue": "x", "fields": ["_ambiguities"]}], "ledger.toml")
303319

304320

321+
#: What _run_worker was asked for, so a test can prove main forwarded
322+
#: the baseline and the corpus rather than defaults of its own.
323+
_WORKER_CALL: dict = {}
324+
325+
305326
def _run_main(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ledger_body: str,
306-
baseline_facade: dict) -> tuple[int, str]:
327+
baseline_facade: dict, baseline: str = "1.4.0",
328+
baseline_v2: dict | None = None) -> tuple[int, str]:
307329
"""Drive main() end to end with a faked baseline worker.
308330
309331
No uv, no network. The helper exists because every unit test above
310332
proves a helper WORKS while none proves main() calls it -- and in a
311333
gate, the composition is the part that can go silently permissive.
334+
335+
`baseline` defaults to 1.4.0 (facade only). Pass 2.0.0 with
336+
`baseline_v2` to exercise the v2 surface, including the
337+
ambiguity-only diff that is the stated reason to compare it.
312338
"""
313339
import sys
314340
corpus = tmp_path / "corpus_x.jsonl"
315341
corpus.write_text('"John Smith"\n', encoding="utf-8")
316-
(tmp_path / "expected_since_1.4.0.toml").write_text(
342+
(tmp_path / f"expected_since_{baseline}.toml").write_text(
317343
ledger_body, encoding="utf-8")
344+
row: dict = {"facade": baseline_facade}
345+
if baseline_v2 is not None:
346+
row["v2"] = baseline_v2
347+
_WORKER_CALL.clear()
348+
349+
def _fake(v: str, w: bool, n: list[str]) -> tuple[dict, list[dict]]:
350+
_WORKER_CALL.update(version=v, want_v2=w, names=list(n))
351+
return ({"__version__": v,
352+
"__file__": "/wheel/nameparser/__init__.py"}, [row])
353+
318354
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",
355+
monkeypatch.setattr(compare, "_run_worker", _fake)
356+
monkeypatch.setattr(sys, "argv", ["compare.py", "--baseline", baseline,
325357
"--corpus", str(corpus)])
326358
import io
327359
import contextlib
@@ -377,12 +409,12 @@ def test_main_validates_the_ledger_before_running_anything(
377409
"""validate_rules has its own tests; this pins that main CALLS it.
378410
Deleting the call leaves those tests passing while a match-anything
379411
rule shadows the ledger."""
380-
with pytest.raises(SystemExit, match="matches the empty string"):
412+
with pytest.raises(SystemExit, match="matches every one of"):
381413
_run_main(tmp_path, monkeypatch,
382414
'[[change]]\nissue = "wide"\nname_regex = ""\n', _DIFFERS)
383415

384416

385-
def test_main_sorts_rules_so_file_order_is_not_load_bearing(
417+
def test_main_sorts_a_name_regex_rule_ahead_of_a_fields_only_one(
386418
tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
387419
"""A broad fields-only rule written FIRST must not claim a diff the
388420
specific name_regex rule below it owns. Deleting main's
@@ -401,7 +433,7 @@ def test_check_tree_accepts_the_checkout_and_rejects_anything_else(
401433
was a bare import trusted on sight."""
402434
inside = _TOOLS.parents[1] / "nameparser" / "__init__.py"
403435
assert compare._check_tree(str(inside)) == inside.resolve()
404-
with pytest.raises(SystemExit, match="outside this checkout"):
436+
with pytest.raises(SystemExit, match="not from this checkout's source"):
405437
compare._check_tree(str(tmp_path / "nameparser" / "__init__.py"))
406438

407439

@@ -419,7 +451,7 @@ def test_main_aborts_when_the_tree_side_is_not_the_checkout(
419451
tree's nameparser is no longer under the root it must be under.
420452
"""
421453
monkeypatch.setattr(compare, "REPO_ROOT", tmp_path)
422-
with pytest.raises(SystemExit, match="outside this checkout"):
454+
with pytest.raises(SystemExit, match="not from this checkout's source"):
423455
_run_main(tmp_path, monkeypatch,
424456
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n', _DIFFERS)
425457

@@ -435,3 +467,172 @@ def test_worker_env_strips_the_import_path_overrides(
435467
env = compare._worker_env()
436468
assert "PYTHONPATH" not in env and "PYTHONHOME" not in env
437469
assert env["PATH"] == "/usr/bin", "the rest of the env must survive"
470+
471+
472+
class _FakePopen:
473+
"""Records how _run_worker spawned the child, and replays a canned
474+
stdout. Lets the subprocess-facing guards be tested without uv."""
475+
476+
last: dict = {}
477+
out: str = ""
478+
rc: int = 0
479+
480+
def __init__(self, argv: list[str], **kw: object) -> None:
481+
_FakePopen.last = {"argv": argv, **kw}
482+
self.returncode = _FakePopen.rc
483+
484+
def communicate(self, payload: str) -> tuple[str, str]:
485+
_FakePopen.last["stdin"] = payload
486+
return _FakePopen.out, ""
487+
488+
489+
def _fake_popen(monkeypatch: pytest.MonkeyPatch, out: str,
490+
rc: int = 0) -> type[_FakePopen]:
491+
_FakePopen.out, _FakePopen.rc = out, rc
492+
monkeypatch.setattr(compare.subprocess, "Popen", _FakePopen)
493+
return _FakePopen
494+
495+
496+
_TELL = ('{"__version__": "1.4.0", '
497+
'"__file__": "/wheel/nameparser/__init__.py"}')
498+
_ROW = '{"facade": {"first": "John"}}'
499+
500+
501+
def test_run_worker_strips_the_import_path_overrides_from_the_child(
502+
monkeypatch: pytest.MonkeyPatch) -> None:
503+
"""_worker_env has its own test; this pins that _run_worker USES
504+
it. Deleting `env=_worker_env()` left all 61 tests green -- the
505+
same shape as the bug the previous review found, a proved helper
506+
with an unproved call site."""
507+
monkeypatch.setenv("PYTHONPATH", "/shadow")
508+
_fake_popen(monkeypatch, f"{_TELL}\n{_ROW}\n")
509+
compare._run_worker("1.4.0", False, ["John Smith"])
510+
env = _FakePopen.last["env"]
511+
assert "PYTHONPATH" not in env and "PYTHONHOME" not in env
512+
513+
514+
def test_run_worker_aborts_on_a_nonzero_exit(
515+
monkeypatch: pytest.MonkeyPatch) -> None:
516+
_fake_popen(monkeypatch, "", rc=3)
517+
with pytest.raises(SystemExit, match="exited 3"):
518+
compare._run_worker("1.4.0", False, ["John Smith"])
519+
520+
521+
def test_run_worker_aborts_on_empty_output(
522+
monkeypatch: pytest.MonkeyPatch) -> None:
523+
_fake_popen(monkeypatch, "")
524+
with pytest.raises(SystemExit, match="not even a version tell"):
525+
compare._run_worker("1.4.0", False, ["John Smith"])
526+
527+
528+
def test_run_worker_aborts_when_fewer_results_than_names(
529+
monkeypatch: pytest.MonkeyPatch) -> None:
530+
"""The guard behind main's zip(), which truncates silently. This is
531+
the comparing-fewer-names-than-you-think failure."""
532+
_fake_popen(monkeypatch, f"{_TELL}\n{_ROW}\n")
533+
with pytest.raises(SystemExit, match="1 results for 2 corpus names"):
534+
compare._run_worker("1.4.0", False, ["John Smith", "Jane Doe"])
535+
536+
537+
def test_run_worker_checks_the_tell_before_returning_results(
538+
monkeypatch: pytest.MonkeyPatch) -> None:
539+
wrong = ('{"__version__": "9.9.9", '
540+
'"__file__": "/wheel/nameparser/__init__.py"}')
541+
_fake_popen(monkeypatch, f"{wrong}\n{_ROW}\n")
542+
with pytest.raises(SystemExit, match="not the requested"):
543+
compare._run_worker("1.4.0", False, ["John Smith"])
544+
545+
546+
@pytest.mark.parametrize("rel", [
547+
".venv/lib/python3.11/site-packages/nameparser/__init__.py",
548+
"build/lib/nameparser/__init__.py",
549+
"dist/unpacked/nameparser/__init__.py",
550+
])
551+
def test_check_tree_rejects_a_wheel_sitting_inside_the_checkout(
552+
rel: str) -> None:
553+
"""The hole in the first version of this guard. It asked "is this
554+
under the repo", but the repo contains .venv/, build/ and dist/,
555+
any of which can hold a released wheel -- so
556+
PYTHONPATH=<repo>/build/lib was the same trap one directory to the
557+
left, and uv never touches build/ to self-heal it."""
558+
with pytest.raises(SystemExit, match="not from this checkout's source"):
559+
compare._check_tree(str(compare.REPO_ROOT / rel))
560+
561+
562+
def test_check_tree_resolves_before_comparing() -> None:
563+
"""Without .resolve(), a path escaping via .. reads as inside."""
564+
escaped = compare.REPO_ROOT / "nameparser" / ".." / ".." / "x" \
565+
/ "nameparser" / "__init__.py"
566+
with pytest.raises(SystemExit, match="not from this checkout's source"):
567+
compare._check_tree(str(escaped))
568+
569+
570+
#: The tree's own reading of the fixture name, on both surfaces. A fake
571+
#: baseline row built from these differs from the tree in exactly the
572+
#: one field a test chooses to alter.
573+
_SAME_FACADE = {"title": "", "first": "John", "middle": "", "last": "Smith",
574+
"suffix": "", "nickname": "", "maiden": ""}
575+
_SAME_V2 = {"title": "", "given": "John", "middle": "", "family": "Smith",
576+
"suffix": "", "nickname": "", "maiden": "", "_ambiguities": []}
577+
578+
579+
def test_main_compares_the_v2_surface_from_baseline_2_0(
580+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
581+
"""A SEGMENTATION-only diff is facade-identical by construction, so
582+
it is invisible unless main actually unions the v2 surface into the
583+
diff set. That diff shape is the whole stated reason _surfaces_for
584+
compares v2 from 2.0 on -- and every mutation that disabled it
585+
(want_v2 forced False, the v2 union deleted, `|=` changed to `=`)
586+
passed the suite before this test existed.
587+
"""
588+
v2 = {**_SAME_V2, "_ambiguities": ["SEGMENTATION"]}
589+
code, out = _run_main(
590+
tmp_path, monkeypatch,
591+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n',
592+
_SAME_FACADE, baseline="2.0.0", baseline_v2=v2)
593+
assert code == 1, "an ambiguity-only regression must not exit 0"
594+
assert "UNEXPLAINED 'John Smith'" in out
595+
assert "_ambiguities:" in out
596+
assert "[v2 surface only]" in out, (
597+
"the tag distinguishes an ambiguity-kind change from a field "
598+
"change; without it the row reads as a field diff")
599+
600+
601+
def test_main_claims_an_ambiguity_only_diff_when_a_rule_names_it(
602+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
603+
v2 = {**_SAME_V2, "_ambiguities": ["SEGMENTATION"]}
604+
code, out = _run_main(
605+
tmp_path, monkeypatch,
606+
'[[change]]\nissue = "seg"\nfields = ["_ambiguities"]\n',
607+
_SAME_FACADE, baseline="2.0.0", baseline_v2=v2)
608+
assert code == 0 and "## seg (1)" in out
609+
610+
611+
def test_main_reports_a_role_once_when_both_surfaces_moved(
612+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
613+
"""The `seen` set. Both surfaces name the same role, so a family
614+
change shows on each; printing it twice would read as two findings."""
615+
_, out = _run_main(
616+
tmp_path, monkeypatch,
617+
'[[change]]\nissue = "unrelated"\nname_regex = "ZZZ"\n',
618+
{**_SAME_FACADE, "last": "SMYTHE"}, baseline="2.0.0",
619+
baseline_v2={**_SAME_V2, "family": "SMYTHE"})
620+
assert out.count("family:") == 1
621+
622+
623+
def test_main_forwards_the_baseline_and_corpus_to_the_worker(
624+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
625+
"""Otherwise main could read the 2.0 ledger while comparing against
626+
1.4, or compare a truncated corpus, and every other test would pass."""
627+
_run_main(tmp_path, monkeypatch,
628+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n',
629+
_SAME_FACADE, baseline="2.0.0", baseline_v2=_SAME_V2)
630+
assert _WORKER_CALL == {"version": "2.0.0", "want_v2": True,
631+
"names": ["John Smith"]}
632+
633+
634+
def test_main_asks_for_the_facade_alone_below_2_0(
635+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
636+
_run_main(tmp_path, monkeypatch,
637+
'[[change]]\nissue = "x"\nname_regex = "ZZZ"\n', _SAME_FACADE)
638+
assert _WORKER_CALL["want_v2"] is False

0 commit comments

Comments
 (0)