fix(validate): W-INVISIBLE-CHAR misses control characters; make the table readable (Closes #77)#78
Merged
Merged
Conversation
…able readable Three defects in the rule shipped in 0.13.2, all found by comparing it against the reference implementation it was modelled on rather than assuming the first cut was complete. 1. Control characters were not covered. A JSON source escapes them, so JSON.parse hands back a real control character that every structural check accepts. Verified before fixing: a lesson carrying U+0007 in its title validated clean and produced no warning at all. Now covers C0 (minus tab, newline, carriage return), U+007F DELETE and the C1 range. The harness proved the point while this was being written: two of my own shell commands were refused for containing a control character that "would be hidden in the approval dialog". 2. The codepoint table was keyed by the characters themselves, which made the source unreadable in the one file where that is least acceptable. A reviewer saw ["", "SOFT HYPHEN"] and had to take the label on trust. Now keyed by numeric codepoint with explicit ranges. 3. Matching walked every character of every string through a map lookup. Almost all content is clean, so a single regex test now rejects a string in one pass; only a string that actually matches is walked to locate the offenders. Tab, newline and carriage return stay excluded deliberately: theory bodies are full of newlines, and flagging them would warn on nearly every knowledge lesson in the ecosystem. A boundary test pins that, and it was the first test written for this change. Re-measured on the same 528 real lessons across seven content repositories: still 4 findings, still 0 false positives, with control characters and C1 newly in scope. The newline boundary held. TDD RED-first, 19 tests (3 red before the fix). Release 0.13.3 (patch), verified free on npm. Warning tier, additive, no schema change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
External review (Qwen) raised five points. Four are adopted, one is
declined with reasoning.
Adopted:
- matchAll instead of walking every character. The pattern is global now,
so only the actual matches are visited; a pasted chapter is long and
almost entirely clean, which is exactly the case that was being walked
in full. The non-null assertion the old loop needed is gone with it.
- A cycle guard. Verified as a real crash first, not assumed:
findInvisibleChars on a self-referencing object threw RangeError
(maximum call stack size exceeded). The API takes `unknown`, so a
hand-built object can reach it even though JSON.parse output cannot.
Tracks ANCESTORS rather than everything seen, so a shared but acyclic
node is still reported at each path it appears at; a second test pins
that distinction.
- The `u` flag with `\u{...}` syntax. Without it the pattern matches
UTF-16 code units, so any future codepoint above the BMP would be
compared against half a surrogate pair.
- Numeric sort of the codepoint labels. Lexicographic order is correct
only while every label has four hex digits; "U+10000" sorts before
"U+FEFF" as text and after it as a number.
Declined: precomputing a Map<codepoint, name> for O(1) name lookup. With
matchAll in place that lookup runs once per FOUND character, which is
four times across 528 real lessons, against a 20-entry static array.
Expanding the ranges would trade the readable range table for a
~70-entry generated map to optimise a path that barely executes.
Also considered and rejected on evidence: adding the Unicode TAG range
(U+E0000-E007F). Those characters are genuinely invisible and are the
vector for hidden-text tricks, but they also make up legitimate flag
emoji sequences. The current content carries none, though a
language-learning set with country flags is entirely plausible, so
adding them would build in a false positive.
Same 528 lessons, same 4 findings, still 0 false positives. 21 tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
astrapi69
added a commit
that referenced
this pull request
Jul 22, 2026
…#78 squash (#79) * refactor(validate): second review round on the invisible-char walker Three of Qwen's four points adopted, one extended well past what it asked for. - Cycle tracking allocated a fresh Set per object node. One mutable set is now extended on the way down and restored in a finally on the way back up, so a pasted chapter no longer produces thousands of short-lived sets. The public signature loses its internal third parameter; the recursion moved into a private walk(). - The format block was covered only at U+2060. Verified against the build: U+2061-2064 (the invisible math operators a formula editor pastes), U+2066-2069 (the bidi isolates modern editors emit in place of the older embeddings) and U+206A-206F (deprecated format characters) all slipped through. The review named the four isolates; the actual hole was the whole U+2060-206F range, which is general category Cf in full, so that is what the rule now carries. - The numeric sort of codepoint labels had no test. It turned out to be correct, but untested logic is only accidentally correct; a test now pins that U+00AD, U+FEFF and a five-digit U+10000 come out in numeric rather than lexicographic order. Declined again: nothing. The remaining point was praise for using Object.entries over for..in, which is now stated as a reason in the code so it survives a future edit. Provenance recorded in the header: the starting set came from the sanitizer of manuscript-tools, whose own checker had the very gap this module closed (fixed there in manuscript-tools#1). Same 528 lessons, still 4 findings, still 0 false positives with the wider range in scope. 24 tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(validate): record why Map/Set need no handling in the walker Third review round raised two points; neither needs code. The Map/Set observation is correct in isolation and unreachable in practice, verified rather than argued: validateLesson returns early when the structural check fails, so the lint only ever sees a schema-valid lesson, and a Map is rejected with E-SCHEMA before it. On top of that this module is not exported from the package entry. Both would have to change before the gap opens, so the reasoning is recorded in the header instead of speculative handling being added for a path that cannot execute. The second point asked for a test of the numeric codepoint sort. That test was added in the previous round and passes; the review was reading the earlier version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Asterios Raptis <asteri.raptis@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Three defects in the rule shipped in 0.13.2 (#75 / #76), found by comparing it against the reference implementation it was modelled on rather than assuming the first cut was complete.
1. Control characters slip through
A JSON source escapes a control character (``), so
JSON.parsehands back a real one and every structural check accepts it. Verified before fixing:The reference tool's sanitizer covers the whole C0 range plus
U+007F; only its checker is limited to the six zero-width codepoints, and the first cut mirrored the checker rather than the sanitizer.The boundary that matters more: tab, newline and carriage return must stay excluded. Theory bodies are full of newlines, so flagging them would warn on nearly every knowledge lesson in the ecosystem.
2. The codepoint table is unreadable in source
It was keyed by the characters themselves, so a reviewer sees this:
The key is invisible, in the one file where taking a label on trust is least acceptable. Reviewing it required
cat -A. It should be keyed by numeric codepoint.3. Matching is slower than it needs to be
Every character of every string goes through a map lookup, though almost all content is clean. A single regex test can reject a string in one pass, walking it only when it actually matches.
Fix
Ranges instead of single characters, numeric keys, a regex fast path, control characters and C1 in scope, and the newline boundary pinned by a test.
Re-measured on the same 528 real lessons across seven content repositories: still 4 findings, still 0 false positives, with control characters newly in scope.
Footnote
While this was being written, the agent harness twice refused a shell command of mine for containing a control character that "would be hidden in the approval dialog". The failure mode this rule exists for is real enough to have interrupted its own fix.