From 7d4c09584409322b08f31dfbdb97f4a5f9a7bc52 Mon Sep 17 00:00:00 2001 From: Asterios Raptis Date: Wed, 22 Jul 2026 11:48:38 +0200 Subject: [PATCH 1/2] 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) --- src/invisible-chars.test.ts | 28 +++++++++++++++++++- src/invisible-chars.ts | 53 ++++++++++++++++++++++++++----------- 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/src/invisible-chars.test.ts b/src/invisible-chars.test.ts index 2110825..066f1d7 100644 --- a/src/invisible-chars.test.ts +++ b/src/invisible-chars.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from "vitest"; -import { findInvisibleChars } from "./invisible-chars.js"; +import { describeInvisibleChars, findInvisibleChars } from "./invisible-chars.js"; import { validateLesson } from "./validate.js"; /** @@ -212,6 +212,32 @@ describe("W-INVISIBLE-CHAR: control characters", () => { expect(messageFor(lesson)).toContain("U+009B"); }); + it("flags the bidi isolates, which modern editors emit instead of the embeddings", () => { + for (const codepoint of [0x2066, 0x2067, 0x2068, 0x2069]) { + const lesson = lessonWith({ title: `A${String.fromCharCode(codepoint)}B` }); + expect(warningsFor(lesson), `U+${codepoint.toString(16)}`).toHaveLength(1); + } + }); + + it("flags the invisible math operators a formula editor pastes", () => { + for (const codepoint of [0x2061, 0x2062, 0x2063, 0x2064]) { + const lesson = lessonWith({ title: `A${String.fromCharCode(codepoint)}B` }); + expect(warningsFor(lesson), `U+${codepoint.toString(16)}`).toHaveLength(1); + } + }); + + it("sorts reported codepoints numerically, not as text", () => { + // Once a codepoint needs five hex digits the two orders disagree: + // "U+10000" precedes "U+FEFF" as text and follows it as a number. + const described = describeInvisibleChars([ + { path: "/a", codepoint: "U+10000", name: "SUPPLEMENTARY" }, + { path: "/a", codepoint: "U+FEFF", name: "BYTE ORDER MARK" }, + { path: "/a", codepoint: "U+00AD", name: "SOFT HYPHEN" }, + ])!; + const order = ["U+00AD", "U+FEFF", "U+10000"].map((label) => described.indexOf(label)); + expect(order).toEqual([...order].sort((left, right) => left - right)); + }); + it("survives a circular reference instead of blowing the stack", () => { // validateLesson takes `unknown`, so a caller can hand it a hand-built // object rather than JSON.parse output. Before the guard this threw diff --git a/src/invisible-chars.ts b/src/invisible-chars.ts index c178ad8..794ba92 100644 --- a/src/invisible-chars.ts +++ b/src/invisible-chars.ts @@ -21,6 +21,12 @@ * characters themselves made the source unreadable: a reviewer saw an empty * string and had to take the label on trust, in the one file where that is * least acceptable. + * + * Codepoints and names follow the Unicode Character Database; the format + * ranges are those of general category Cf. The starting set was taken from + * the sanitizer of `manuscript-tools` + * (https://github.com/astrapi69/manuscript-tools), whose checker had the same + * gap this module closed. */ /** A codepoint, or an inclusive range of them, that renders as nothing. */ @@ -56,7 +62,11 @@ const INVISIBLE_RANGES: readonly InvisibleRange[] = [ { from: 0x202c, to: 0x202c, name: "POP DIRECTIONAL FORMATTING" }, { from: 0x202d, to: 0x202d, name: "LEFT-TO-RIGHT OVERRIDE" }, { from: 0x202e, to: 0x202e, name: "RIGHT-TO-LEFT OVERRIDE" }, - { from: 0x2060, to: 0x2060, name: "WORD JOINER" }, + // The whole U+2060-206F format block: word joiner, the invisible math + // operators a formula editor pastes, the bidi isolates that modern editors + // emit in place of the older embeddings, and the deprecated format + // characters. All are Unicode general category Cf and all render as nothing. + { from: 0x2060, to: 0x206f, name: "INVISIBLE FORMAT CHARACTER" }, { from: 0xfeff, to: 0xfeff, name: "BYTE ORDER MARK" }, ]; @@ -123,27 +133,40 @@ function findingsInString(value: string, path: string): InvisibleCharFinding[] { * what lets this cover ``ext_payload`` (whose shape the engine does not know) * and any field a later schema version adds. */ -export function findInvisibleChars( - value: unknown, - path = "", - ancestors: ReadonlySet = new Set(), -): InvisibleCharFinding[] { +export function findInvisibleChars(value: unknown, path = ""): InvisibleCharFinding[] { + return walk(value, path, new Set()); +} + +/** + * The recursion behind {@link findInvisibleChars}. + * + * ``ancestors`` holds the current PATH, not everything seen, so a node reached + * twice without a cycle is still reported at each path it appears at. It is + * one mutable set that is extended on the way down and restored on the way + * back up: copying it per node would allocate one set per object in the + * lesson, which for a pasted chapter is thousands of short-lived sets. The + * ``finally`` keeps the set honest even if a callee throws. + */ +function walk(value: unknown, path: string, ancestors: Set): InvisibleCharFinding[] { if (typeof value === "string") return findingsInString(value, path); if (typeof value !== "object" || value === null) return []; // A lesson from JSON.parse cannot be cyclic, but this API takes `unknown`, // so a hand-built object can be. Unguarded that is a stack overflow rather - // than a diagnosis. Tracking the ANCESTORS rather than everything seen keeps - // a shared (but acyclic) node reported at each path it appears at. + // than a diagnosis. if (ancestors.has(value)) return []; - const nested = new Set(ancestors).add(value); - if (Array.isArray(value)) { - return value.flatMap((entry, index) => - findInvisibleChars(entry, `${path}/${index}`, nested), + ancestors.add(value); + try { + if (Array.isArray(value)) { + return value.flatMap((entry, index) => walk(entry, `${path}/${index}`, ancestors)); + } + // Object.entries, not for..in: only own enumerable keys, so nothing + // inherited from a polluted prototype is ever walked. + return Object.entries(value).flatMap(([key, entry]) => + walk(entry, `${path}/${key}`, ancestors), ); + } finally { + ancestors.delete(value); } - return Object.entries(value).flatMap(([key, entry]) => - findInvisibleChars(entry, `${path}/${key}`, nested), - ); } /** How many distinct paths to name before trailing off. Enough to start From f819936b1605df1a9556deca741b5f2b15f4eef1 Mon Sep 17 00:00:00 2001 From: Asterios Raptis Date: Wed, 22 Jul 2026 12:15:56 +0200 Subject: [PATCH 2/2] 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) --- src/invisible-chars.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/invisible-chars.ts b/src/invisible-chars.ts index 794ba92..bb2dc5e 100644 --- a/src/invisible-chars.ts +++ b/src/invisible-chars.ts @@ -27,6 +27,14 @@ * the sanitizer of `manuscript-tools` * (https://github.com/astrapi69/manuscript-tools), whose checker had the same * gap this module closed. + * + * Scope note: the walker below reads plain JSON shapes only. ``Map`` and + * ``Set`` values would be walked as empty objects, which sounds like a silent + * false negative but cannot happen here. ``validateLesson`` returns early when + * the structural check fails, so the lint only ever sees a schema-valid + * lesson, and this module is not exported from the package entry. Both would + * have to change before the gap is reachable; if this is ever made public, + * handle those two types before doing so. */ /** A codepoint, or an inclusive range of them, that renders as nothing. */