Point the differential gate at any released baseline - #332
Open
derek73 wants to merge 14 commits into
Open
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.
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