Stop offering a live spelling imports will refuse - #405
Merged
Merged
Conversation
#400 let a catalogue carry a ledger name holding a control character -- a ledger genuinely named across two lines -- because a catalogue name is a fact about the book. The proposal side still refuses one, because a caller's name is input. Both rules are right, and the gap between them is a spelling the tool can report but the caller cannot send back. `master_recovery_guidance` told them to send it anyway: "For identifier-bound entries, copy exact_live_spelling from this fresh result." Following that does not fail one row. `source_entities` collects into a single Result and refuses on the first bad name, so one such ledger fails the **entire** batch with `master_name_unsafe` -- every voucher in the request, including those naming no unusual ledger at all. The report now carries `importable`, and the guidance offers the copy only for entries where copying can work. Where it cannot, it says so and names the one remedy that exists: have an operator rename the ledger in Tally. `importable` is answered by calling `SourceEntity::new` rather than restating what it accepts, so the report and the import path cannot drift apart. No corruption was possible -- the failure was closed, not silent. What it cost was a caller following the tool's own instructions into a dead end it could not diagnose. Found by an independent review of #294 that was asked to trace this exact path. Both tests confirmed failing before the fix and passing after. Both workspaces green; 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. |
lamemustafa
pushed a commit
that referenced
this pull request
Sep 15, 2026
#405 resealed pinned files without changing the pin set, so both sides moved the two manifests and they conflicted. Pin lists compared by path: master's 216 is a strict subset of this branch's 217, the difference being this branch's own addition, and #405 changed hashes only. Resolved by taking this branch's list and running scripts/reseal.sh, which recomputes every hash from the merged tree rather than hand-merging one. Exactly one hash moved, and no path is missing versus either side. reseal.sh --verify exit 0, read directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lamemustafa
added a commit
that referenced
this pull request
Sep 15, 2026
* Refuse a proposed name that reads as a different master `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> * Stop treating Indic orthography as deception `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> * Reseal after merging master 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> * Name the trade-off, and cover the crate that actually reads books 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> * Reseal after merging master Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: t <dev@example.invalid>
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.
#400 let a catalogue carry a ledger name holding a control character — a ledger genuinely named across two lines — because a catalogue name is a fact about the book. The proposal side still refuses one, because a caller's name is input. Both rules are right. The gap between them is a spelling the tool can report but the caller cannot send back.
master_recovery_guidancetold them to send it anyway:Following that does not fail one row.
source_entitiescollects into a singleResultand refuses on the first bad name, so one such ledger fails the entire batch withmaster_name_unsafe— every voucher in the request, including those naming no unusual ledger at all.The fix
The report now carries
importable, and the guidance offers the copy only where copying can work. Where it cannot, it says so and names the one remedy that exists: have an operator rename the ledger in Tally.importableis answered by callingSourceEntity::newrather than restating what it accepts, so the report and the import path cannot drift apart.Severity
No corruption was possible — the failure was closed, not silent. What it cost was a caller following the tool's own instructions into a dead end it could not diagnose, with a batch-wide failure pointing at a name it had just been handed as correct.
Provenance
Found by an independent review of #294 that was asked to trace how catalogue names flow into
agent_import.rs. It is a consequence of #400 meeting pre-existing code inagent_import.rsthat neither #400 nor #294 touched, which is why neither PR's own tests could have caught it.Verification
importable: trueand is still offered for copying.src-tauriandtools.reseal.sh --verifyexit 0; pin set unchanged at 216 (agent_import.rsis pinned).🤖 Generated with Claude Code