Refuse a proposed name that reads as a different master - #408
Merged
lamemustafa merged 7 commits intoSep 15, 2026
Merged
Conversation
`char::is_control` is Unicode category Cc. Bidi overrides and zero-width characters are Cf, so every one of them walked straight through the proposal side's control-character check: `U+202E`, `U+200B`, `U+061C` and `U+FEFF` all answer `false`. The catalog side refuses them and this side did not, which is the wrong way round. A name read from a book is a fact about the book. A name a caller supplies is the one that can be *chosen* so that what a reviewer sees is not the master it binds. #400 added the catalog-side refusal and its doc comment claimed these characters "stay refused on both sides". That was false when I wrote it -- I asserted a property of a function I had not changed. This makes the claim true rather than deleting it. The test asserts the premise before the conclusion: for each character it first checks `char::is_control` is false, so the test fails loudly if a future Rust release reclassifies one and the coverage silently becomes vacuous. It also pins what must keep working -- an ordinary name, and the two-line catalog name #400 deliberately admits. Found by an independent review of #405, raised as suspected and unverified; confirmed by running `char::is_control` over the set. Gate: both workspaces green, clippy silent on both, reseal --verify exits 0 with the pin set unchanged at 216. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
`U+200B..=U+200F` is not a set of deceptive characters. It contains ZWNJ (U+200C) and ZWJ (U+200D), which Devanagari and other Indic scripts use to prevent or force a conjunct. They are ordinary spelling, and this repository's own fixtures are full of Indic ledger names. Refusing them failed the **whole** catalog on a legitimately spelled Hindi or Marathi ledger -- the exact failure the newline fix existed to remove, on a book type Indian CA practice is full of. **This was a regression I introduced.** Before #400, `MasterCatalog::new` checked only `char::is_control`, which is category Cc and admits ZWNJ. #400 added the deceptive-set check to the catalog side and took the whole `U+200B..=U+200F` span with it, so a Devanagari name that worked before stopped working. The protocol crate refused it before and after; only the core is a regression. The set is narrowed rather than abandoned: `U+200B`, `U+200E` and `U+200F` carry no orthographic role and stay refused, alongside the override, isolate and BOM ranges. The test pins both directions -- ZWNJ and ZWJ admissible on both sides, their neighbours in the same span still refused -- so this cannot be widened back by accident. Prompted by asking an independent reviewer to check exactly this before merging, rather than after. Gate: both workspaces green, clippy silent on both, reseal --verify exits 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…refuse-deceptive-characters # Conflicts: # docs/tally/compatibility/compatibility-matrix.json # docs/tally/compatibility/compatibility-surface.json
Took master's pin list, which is a superset of this branch's at 217 to 216 -- it adds the scheduled audit workflow and this branch adds nothing. Sets compared as paths against both parents; none dropped from either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two gaps an independent review found, both fair. The protocol crate carries its own copy of the deceptive set and it is the one on the real read path -- `observed_standard_ledger_name` runs on every name a book returns -- yet the narrowing landed there with no test at all. It has one now, driven to failure by restoring the wide range in that crate alone. And the predicates now say what admitting ZWNJ costs. A codepoint filter cannot tell a joiner between two Devanagari consonants from one injected into ASCII, so `Alpha<ZWJ> Traders` is byte-distinct from `Alpha Traders` and renders the same. The guarantee is bounded rather than closed: the fold and the token index keep the joiner verbatim, so such a name tends to fail exact and token matching rather than quietly aliasing a real master. Worse than refusal, far better than breaking every Indic book. One claim withdrawn: the `char::is_control` premise assertion does not guard against the refusal going vacuous -- `unwrap_err()` already panics on an unexpected `Ok`. It guards against Unicode reclassifying a codepoint out from under the comment, which is narrower than I said. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…refuse-deceptive-characters # Conflicts: # docs/tally/compatibility/compatibility-matrix.json # docs/tally/compatibility/compatibility-surface.json
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lamemustafa
deleted the
fix/proposal-names-refuse-deceptive-characters
branch
September 15, 2026 20:37
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
1 participant
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.
char::is_controlis Unicode category Cc. Bidi overrides and zero-width characters are Cf, so every one of them walked straight through the proposal side's control-character check:is_controlU+202EfalseU+200BfalseU+061CfalseU+FEFFfalseThe catalogue side refuses them;
SourceEntity::newdid not. That is the wrong way round. A name read from a book is a fact about the book. A name a caller supplies is the one that can be chosen so that what a reviewer sees is not the master it binds.A correction to #400
#400 added the catalogue-side refusal, and its doc comment claimed these characters "stay refused on both sides". That was false when I wrote it — I asserted a property of a function I had not changed and did not check. This PR makes the claim true rather than deleting it.
The test asserts its own premise
For each character it first asserts
char::is_controlisfalse, then asserts the name is refused. So if a future Rust release reclassifies one of them, the test fails loudly instead of silently becoming vacuous — the coverage cannot quietly stop measuring the thing it exists for.It also pins what must keep working: an ordinary name, and the two-line catalogue name #400 deliberately admits.
Provenance
Raised by an independent review of #405 as suspected and unverified, explicitly flagged as out of scope for that PR. Confirmed by running
char::is_controlover the set rather than reasoning about the category.Verification
src-tauriandtools; clippy silent on both.reseal.sh --verifyexit 0; pin set unchanged at 216.🤖 Generated with Claude Code