fix(validate): restore the Cf block and walk() refactor dropped by the #78 squash#79
Merged
Merged
Conversation
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>
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>
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.
#78was squash-merged while I was still pushing to its branch. The squash captured the branch as it stood at that moment, which was the first review round; the two commits that landed afterwards never reachedmain.What is missing from main
47ffc46walk()refactor and the U+2060-206F Cf block7b7892fMap/Setneed no handlingVerified rather than assumed:
mainstill carries the per-nodenew Set(ancestors)copy and covers onlyU+2060of the format block.Why this matters right now
mainis already at version0.13.3. Publishing from it would have put a0.13.3on npm without the Cf block, while the changelog and the PR discussion describe it as included. Caught by checking the merged state before publishing rather than after.This PR
The two commits cherry-picked onto current
main. No new work, no version change:0.13.3is correct once this lands, and the release then matches what#78was reviewed as.Verification
make release-checkgreenNote for the sequence
The two re-pin PRs (adaptive-learner-content#156, alc-psychology#4) should move from
0.13.2to0.13.3after this lands and0.13.3is published, so the content repos pick up the full range in one step instead of two.