fix: CJK UTF-8 source files misclassified as binary when head -c 1000 truncates mid-character - #81098
Closed
MISAKIGA wants to merge 1 commit into
Closed
Conversation
… truncates mid-character
The _is_likely_binary() content-analysis path rejects any sample
containing U+FFFD (replacement char) under the assumption that
'legitimate UTF-8 text effectively never contains U+FFFD'. This
assumption holds for a complete file read, but not for the head -c
1000 sample that read_file uses: when byte 1000 lands inside a
multi-byte CJK character, the truncated sequence decodes as exactly
one trailing U+FFFD -- a sampling artifact, not binary content.
Fix: strip at most one U+FFFD from the tail of the sample before
judging. A real non-UTF-8 file has replacement chars throughout
the sample (not just at the tail), so the binary-detection safety
net is preserved.
Before: chat/store.ts (24892 bytes of valid UTF-8 with CJK comments)
→ head -c 1000 cuts '会' (E4 BC 9A) → '\ufffd' in sample
→ is_binary=True → 'Binary file - cannot display as text'
After: the trailing artifact is stripped; the rest of the sample is
clean → is_binary=False → file is readable as text.
Collaborator
Duplicate of #79408: both implement the same trailing-U+FFFD exemption for a valid UTF-8 byte-sample boundary cut. Please consolidate review and tests there. |
Contributor
|
Closing — superseded by #81961, which fixed this bug class (1000-byte sample cutting a multibyte char → binary false positive) at the byte layer across read_file, patch, and search_files, with regression tests for truncated-CJK/BOM/UTF-16/NUL cases. ~24 PRs raced on this one; earliest diagnosis credit to @webtecnica (#76924), merged implementation from @ayushnangia (#80440). Thanks for jumping on it. |
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.
Summary
read_filemisclassifies valid UTF-8 source files containing CJK (or any multi-byte) characters as binary, refusing to display them.Root cause
read_filesamples a file withhead -c 1000and passes the decoded output to_is_likely_binary(). The content-analysis path treats any occurrence ofU+FFFD(replacement char) as binary — a guard against lossy read→edit→write round-trips on genuinely non-UTF-8 files.The guard's assumption ("legitimate UTF-8 text effectively never contains U+FFFD") holds for a complete file, but not for a truncated sample: when byte 1000 lands inside a 3- or 4-byte CJK character, the incomplete sequence decodes as exactly one trailing
U+FFFD— a sampling artifact.Impact
A real-world case:
hermes-shine/front/src/pages/chat/store.ts(24,892 bytes, valid UTF-8 with Chinese comments) became unreadable viaread_filebecause its byte 1000 splits the 3-byte character 会 (E4 BC 9A).Fix
Strip at most one
U+FFFDfrom the tail of the sample before judging. This precisely targets the truncation artifact:U+FFFD→ artifact removed → readable as text.Tests
Added 4 unit tests in
tests/tools/test_file_operations_edge_cases.py:test_truncated_multibyte_utf8_not_flaggedtest_trailing_fffd_from_real_cjk_filetest_internal_replacement_chars_still_binarytest_only_trailing_replacement_char_strippedVerification
tests/tools/test_file_operations.py tests/tools/test_file_operations_edge_cases.py: 71 passed (incl. the 4 new + pre-existing U+FFFD guard tests)tests/tools/suite: 5112 passed, 228 pre-existing failures in unrelated modules (approval/delegation/daytona/browser — environment-dependent, zero in file ops)LocalEnvironment: the previously-unreadablechat/store.tsnow returnsis_binary: Falseand full content