Fix crash on supplementary-plane characters (emoji, astral CJK) in release/catalog titles - #117
Open
jbob06 wants to merge 1 commit into
Conversation
UnicodeComparisonNormalizer.NormalizeWordsWithSourceSpans iterated a string one UTF-16 code unit at a time and normalized each unit alone. Any character outside the Basic Multilingual Plane (an emoji, or a character like a CJK Extension B ideograph) is a surrogate pair in .NET strings, so normalizing one half in isolation threw "String contains invalid Unicode code points" - a lone surrogate isn't well-formed UTF-16 on its own. This crashed ReleaseTitleMatchScorer.TokenizeWithSpans, which DownloadDecisionMaker calls per report while matching RSS releases. The per-report catch turns that into a permanent, non-bypassable rejection, so any release whose title (or an author-catalog title compared against it) contains such a character can never be grabbed. Fix: walk the input by Unicode scalar value (consuming full surrogate pairs) instead of by code unit, and switch character classification from char/CharUnicodeInfo to System.Text.Rune so a supplementary-plane letter is recognized as a letter rather than silently dropped. A genuinely unpaired surrogate (real ill-formed input, not a valid character) is sanitized to U+FFFD - reusing the existing replacement-character handling - instead of crashing. NormalizeInternal (backing NormalizeKey/NormalizeWords) had the same latent bug in its whole-string Normalize() call and gets the same sanitize-and-retry treatment. Fixes Chaptarr#116. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNX9Y1DFRRTsVFXG6aYoHH
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
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.
What
Fixes #116.
UnicodeComparisonNormalizer.NormalizeWordsWithSourceSpansiterated a string one UTF-16 code unit at a time and called.Normalize(NormalizationForm.FormD)on each unit alone. Any character outside the Basic Multilingual Plane (an emoji, or a character like a CJK Extension B ideograph) is a surrogate pair in .NET strings — normalizing one half in isolation throwsArgumentException: String contains invalid Unicode code points, since a lone surrogate isn't well-formed UTF-16 on its own. This is a real, valid character being torn in half by the per-code-unit loop, not malformed input.This crashes
ReleaseTitleMatchScorer.TokenizeWithSpans, whichDownloadDecisionMakercalls per report while matching RSS releases (BuildContradictoryVariantstokenizes both the release title and the matched author's full catalog titles). The per-reportcatchinGetBookDecisionsturns the crash into a permanent, non-bypassable rejection ("Unexpected error processing release"), so any release whose title — or a title in the author's catalog it gets compared against — contains such a character can never be grabbed, silently. (I initially thought this could abort matching for the whole RSS batch; that was wrong, see the correction comment on #116 — it's scoped to the one report.)Fix
char.IsHighSurrogate/char.IsLowSurrogate) instead of by UTF-16 code unit, so a supplementary-plane character normalizes as a whole instead of being split. Source-span tracking (used to map normalized tokens back to original release-title offsets) merges the two spans of a pair into one.char/CharUnicodeInfotoSystem.Text.Rune(Rune.IsLetterOrDigit,Rune.GetUnicodeCategory,Rune.ToLowerInvariant) so a supplementary-plane letter is recognized as a letter instead of being silently dropped (a lone surrogate half is neverIsLetterOrDigit). One side effect worth calling out: a supplementary-plane symbol (e.g. an emoji) sitting directly between two letters with no surrounding whitespace is now correctly treated as a word boundary — previously it vanished without inserting one ("ab🔥cd"→"abcd"before,"ab cd"now). Covered by a new test.U+FFFDvia a newSanitizeUnpairedSurrogateshelper — reusing the method's existing replacement-character handling — instead of crashing.NormalizeWordsWithSourceSpanssanitizes up front (it's length-preserving, so the source-span indices stay valid);NormalizeInternal(backingNormalizeKey/NormalizeWords) had the same latent whole-string-Normalize()bug and gets a try/sanitize-and-retry fallback instead, since that path isn't on the RSS-matching hot loop and the common case shouldn't pay for a sanitize scan it doesn't need.Known limitation, not fixed here
ReleaseTitleMatchScorer'sTokenRegex([\p{L}\p{Nd}]+) matches per UTF-16 code unit, and .NET regex doesn't recognize a surrogate pair as\p{L}even when it's a real letter — so a supplementary-plane letter embedded in a release title still won't survive tokenization as part of a token (e.g.Tokenize("abc𠀀def")still splits into two tokens rather than one). It no longer crashes, which is the actual reported bug, but full round-trip fidelity for supplementary-plane letters through the regex tokenizer would need a separate change to the tokenizer itself. Flagging so it isn't mistaken for solved by this PR.Testing
Added regression tests in
UnicodeComparisonNormalizerFixtureandReleaseTitleMatchScorerFixturecovering: an emoji no longer crashing (NormalizeWords,NormalizeWordsWithSourceSpans, and end-to-endReleaseTitleMatchScorer.Tokenize), a genuinely unpaired surrogate no longer crashing, a real supplementary-plane letter (U+20000) being preserved throughNormalizeWords, the word-boundary behavior change for an adjacent symbol called out above, and the source-span merging for a surrogate pair (asserted by value, not just "doesn't throw").Full suite:
dotnet test src/Chaptarr.Core.Test/Chaptarr.Core.Test.csproj→ 2859 passed, 0 failed, 1 skipped (pre-existing, unrelated).Independently reviewed (fresh-context Opus pass) before opening this PR — findings incorporated: removed the per-scalar try/catch on the RSS-matching hot path in favor of sanitizing once up front (same fix, one code path, no exception-as-control-flow); added value-asserting tests in place of
DoesNotThrow-only ones; corrected the issue's understanding of blast radius (see comment on #116).🤖 Generated with Claude Code
https://claude.ai/code/session_01XNX9Y1DFRRTsVFXG6aYoHH