Point the differential gate at any released baseline - #332
Merged
Conversation
The spaced-honorific rule matched a LEADING 양/군, which are ordinary surnames in that position. Nothing is misclassified today, but a future change routing the leading 양 to suffix would have fallen through to that rule and been reported as an intended honorific -- the regression class the gate exists to catch. Head match dropped; all 89 still classify. UNEXPLAINED kept only the facade's before/after while the diff set unions both surfaces, so an _ambiguities-only diff -- the case _surfaces_for names as the whole reason to compare v2 -- would have printed a name with no field lines under it. Also: pin classify's subset check and V2_FIELDS against Role, both untested and both silent when wrong; drop an unreachable middle from the #298 rule; and correct a claim that this ledger's hand copies are pinned by test_regex_sync, which pins the 1.4 file by name.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #332 +/- ##
=======================================
Coverage 98.48% 98.48%
=======================================
Files 41 41
Lines 2832 2832
=======================================
Hits 2789 2789
Misses 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
ruff enforces ANN001 repo-wide and the new fixtures were bare, so CI failed at the lint step before it reached mypy or pytest. Matches the existing convention (pytest.MonkeyPatch, pathlib.Path).
_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.
The empty-file guard only catches a corpus that lost EVERY name. One truncated to a handful sails past it: the run compares 298 names instead of 751, prints those counts, and exits 0 -- green while having compared a fraction of what its own summary reports. Floors rather than exact counts, because corpus_issues.jsonl grows whenever it is regenerated from the tracker and an exact pin would fail on every legitimate harvest. A corpus with no floor is a hard error rather than an unguarded default, so adding one forces a decision the way the Script tables do -- and the test binds both directions, so neither a floor without a file nor a file without a floor can rot.
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.
Summary
tools/differential/answered one question — did the tree keep 1.4 parity — because it was built for the 2.0 migration. This generalizes it:--baseline VERSIONcompares the tree against any released version, so "what changes for a user already on the previous minor who upgrades and opts into nothing" is answerable at every release.For 2.1 that answer is 89 diffs, every one on a name bearing a CJK character, zero on Latin-only input.
Changes
--baseline VERSION(default2.0.0). The version selects the ledger (expected_since_<VERSION>.toml, a hard error if absent — not an empty rule set, which would classify nothing and read as a catastrophic regression) and the compared surfaces: facade alone below 2.0, facade + v2 API +AmbiguityKinds from 2.0 on.uvreads genuine inline metadata, andsys.path[0]becomes a directory holding nonameparser, so the checkout cannot shadow the pinned wheel.worker_v1.pyis deleted.fieldsname roles the wayRoledoes. The facade'sfirst/lastcanonicalize on the way in, and theUNEXPLAINEDblock prints canonical names so what a reader sees is what they write in a rule. AGENTS.md already makesRolecanonical "defined once and derived everywhere"; the facade's vocabulary expires at 3.0.expected_changes.toml→expected_since_1.4.0.toml(rename kept 100% similarity, sogit log --followstill reaches the file's origin), plus the newexpected_since_2.0.0.toml.tests/v2/test_differential.py— 38 tests over the logic whose silent misbehavior produces false confidence.corpus_cjk.jsonl, the corpus covering the whole 2.1 CJK surface.Known ceiling, documented in the README
The corpora run under the default policy, so policy-gated behavior is invisible here. #329 is the worked example: under
Policy(maiden_delimiters={("(", ")")})the tree gives maidenJoneswhere 2.0.0 givesnée Jones, and this gate cannot see it. Default vocabulary is fully exercised —Lexicon.maiden_markersships 17 entries and the barenée Jonesform needs no configuration. What is opt-in is the delimited path, not the marker words.tests/v2/cases.pycovers opt-in behavior via its per-rowpolicy=.Review found one hole worth naming
The spaced-honorific rule matched a leading 양/군 — ordinary surnames in that position (Yang, Gun). Nothing was misclassified today, but a future change routing the leading 양 of
양 미선tosuffixwould have fallen through to that rule, whosefieldsincludesuffix, and been reported as an intended honorific. A surname silently reclassified as a suffix is precisely the regression class this gate exists to catch. Head match dropped; all 89 still classify.Verification
uv run pytest -q→ 3013 passed, 20 skipped, 11 xfaileduv run mypy→ clean, 97 source filesUNEXPLAINEDpath exercised against a stub ledger, confirming it prints canonical field namesFollow-up left open
tests/v2/test_regex_sync.pypins the 1.4 ledger's hand-copied character classes against their sources by hardcoded filename, so the 2.0 ledger's copies are unpinned. Both were verified equal to their sources today, and the gap is recorded in the ledger's own comments. Parametrizing those pins over everyexpected_since_*.tomlneeds the#271/#272selector reworked first, since it asserts uniqueness and the new file has two such rules.🤖 Generated with Claude Code