fix: restore #847, which #851 reverted in full (#831) - #870
Merged
Conversation
I broke this. #851 was collapsed to one commit with `git reset --soft origin/main` at a moment when origin/main had moved ahead of the working tree, so the commit recorded the DIFFERENCE -- which included deleting every file #847 had added that the branch had never seen. All fourteen files: 140 lines of build.rs, data/confusables_lgr.tsv, tests/test_lgr_pairs.py, the seventeen ICANN LGR rows in confusables_to_latin.tsv, load_lgr in the generator, and the documented count in four places. The consequence was behavioural, not cosmetic: canonicalize("ż") stopped equalling canonicalize("ź"), so the same-script Latin homoglyph pairs #831 closed had reopened and the four hostname rows that issue names were live again. Nothing caught it, and that is the part worth fixing rather than the files. tests/test_lgr_pairs.py covers this class properly and was deleted by the same commit, so it could not fail. The doc-count gates could not fire either, because the counts were reverted along with the table they check. A 40-row fixture diff was the only trace and it was applied rather than questioned -- by me. Restored file by file rather than by reverting the revert, which was not available: #851 carried the legitimate has_bidi_control work in the same squashed commit. Ten of the fourteen had no later commits other than the revert and were taken verbatim from 79c522f; docs/limitations.md and python/disarm/_api.py had later work and took only #847's own hunks. The key fixture is regenerated, and the 40 rows it moves are the LGR collisions coming back. tests/test_no_silent_revert.py checks the ARTIFACTS rather than the behaviour: a bundled data file must exist and something must reference it, and build.rs must still carry #831's two safety asserts. Behaviour tests travel with the feature and vanish with it; a data file's absence is a build-level fact a deleted test cannot hide. Mutation-checked by moving the file aside. Refs #831, #847, #851 Signed-off-by: Richard Quinn <quinn.richard@gmail.com>
|
📄 Docs preview: https://6ad13039.disarm-docs.pages.dev |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A few newly introduced parsing/doc details are internally inconsistent or less robust than adjacent code and should be tightened before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Restores the ICANN Latin second-level LGR confusable-pair support that was unintentionally removed, re-establishing the intended canonicalize collisions for same-script Latin homoglyph pairs and adding a build-level gate to prevent silent loss of bundled artifacts.
Changes:
- Reintroduce the LGR-driven to-Latin overrides and merge them into the confusable generation pipeline.
- Reinstate/extend build-time safety assertions for non-ASCII to-Latin targets and update documented mapping counts.
- Add regression tests for LGR collision behavior plus an “artifact presence + referenced” gate to catch silent reverts.
File summaries
| File | Description |
|---|---|
| tests/test_no_silent_revert.py | Adds an artifact-level gate ensuring key bundled data files still exist and are referenced; also asserts a few LGR collisions and build.rs markers. |
| tests/test_lgr_pairs.py | Restores detailed regression coverage for LGR homoglyph pairs (collide) and Common-LGR-only transitivity pairs (do not collide). |
| tests/test_confusable_residue_docs.py | Updates narrative around residue counts to reflect the restored mapping behavior. |
| src/tables/data/confusables_to_latin.tsv | Restores the ICANN LGR-related to-Latin mappings. |
| src/tables/confusables_data.rs | Updates the documented count for the Latin confusables table. |
| scripts/gen_confusables.py | Restores LGR loading/merging into the generated override set and updates reporting. |
| python/disarm/_api.py | Updates the documented Latin mapping count for normalize_confusables. |
| docs/user-guide/confusables.md | Updates Latin mapping counts and the residue count. |
| docs/limitations.md | Documents the restored LGR source and updates mapping counts. |
| docs/architecture/data-tables.md | Updates the confusables(Latin) table entry count and provenance summary. |
| data/confusables_lgr.tsv | Restores the ICANN LGR override source data with provenance and selection rationale. |
| CHANGELOG.md | Records the silent revert incident and the new artifact-level gate rationale. |
| build.rs | Restores/extends build-time assertions around non-ASCII to-Latin targets and adds supporting helpers/allowlist. |
Review details
- Files reviewed: 13/14 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three findings, all correct, all pre-existing in #847 rather than introduced by the restore -- which is a fair reading of what a restore is for: the code got a second review it would not otherwise have had. load_lgr did not strip the target column while stripping the other two, and while load_supplement and load_attested strip theirs. A trailing space in the TSV would have become part of the fold target. Worth noting the asymmetry is deliberate elsewhere: build.rs's read_char_str_tsv explicitly does NOT trim, because in confusables_to_latin.tsv a trailing space can be the whole value (U+30FB folds to one). An LGR target is always a Latin letter, so stripping is right here and wrong there. The data file's header said "the 19 qualifying pairs" nine lines after saying 23 qualify. Both numbers are true of different things: 23 pairs meet the admission criterion, 19 of those can be expressed by a single code point, and the file holds 17 rows because the 19 collapse into equivalence classes. The header now says which is which. limitations.md hard-coded "the 23 pairs ... are imported", which is a count the page has to keep in step with data it does not own -- and was already imprecise, since 19 are expressible rather than 23. It now describes the criterion, which is the durable statement: the Latin Generation Panel's own comment, a judgement by the people who wrote the registry. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Richard Quinn <quinn.richard@gmail.com>
…' into fix/multichar-target-is-a-source
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.
What happened
I broke this. #851 was collapsed to a single commit with
git reset --soft origin/mainat a moment whenorigin/mainhad moved ahead of the working tree. The reset recorded the difference, which included deleting every file #847 had added that the branch had never seen.All fourteen files: 140 lines of
build.rs,data/confusables_lgr.tsv,tests/test_lgr_pairs.py, the seventeen ICANN LGR rows inconfusables_to_latin.tsv,load_lgrin the generator, and the documented count in four places.The consequence was behavioural
The same-script Latin homoglyph pairs #831 closed had reopened, and the four hostname rows that issue names —
ważne.pl/waźne.pland the rest — were live again.Nothing caught it, and that is the part worth fixing
tests/test_lgr_pairs.pycovers this class properly. It was deleted by the same commit, so it could not fail.How it was restored
Not by reverting the revert, which was not available: #851 carried the legitimate
has_bidi_controlwork in the same squashed commit.File by file, after checking what else had touched each:
79c522fdocs/limitations.md,python/disarm/_api.pyThe key fixture is regenerated; the 40 rows it moves are the LGR collisions coming back.
The gate
tests/test_no_silent_revert.pychecks the artifacts, not the behaviour: a bundled data file must exist and something must reference it, andbuild.rsmust still carry #831's two safety asserts.That distinction is the lesson. Behaviour tests travel with the feature and vanish with it — a data file's absence is a build-level fact a deleted test cannot hide. Mutation-checked by moving the file aside.
Refs #831, #847, #851
🤖 Generated with Claude Code