fix: verify NUL bytes before flagging U+FFFD samples as binary (#80308) - #80349
Closed
JonthanaHanh wants to merge 1 commit into
Closed
fix: verify NUL bytes before flagging U+FFFD samples as binary (#80308)#80349JonthanaHanh wants to merge 1 commit into
JonthanaHanh wants to merge 1 commit into
Conversation
The _is_likely_binary() function checked for U+FFFD in the first 1000 bytes and immediately returned True (binary). But head -c 1000 can cut UTF-8 multibyte sequences (CJK = 3 bytes, emoji = 4 bytes) at the byte boundary, causing the terminal decoder to produce false U+FFFD from perfectly valid text files. When U+FFFD is detected, now verifies by checking raw bytes for NUL (0x00) via Python before declaring binary. NUL is the reliable binary indicator: genuine binary files almost always contain NUL bytes, while text files effectively never do. Fixes NousResearch#80308
1 task
Collaborator
Duplicate of #76925: this addresses the same UTF-8 sample-boundary false-binary mechanism. Please consolidate the raw-NUL validation idea and tests with the canonical PR. |
Contributor
|
Closing — the NUL-only verification here would pass non-UTF-8 latin-1 files as text, which turns a read→edit→write cycle into mojibake corruption; it also shells out to |
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 files containing CJK characters (Chinese, Japanese, Korean, emoji) as binary and refuses to display them. The root cause is in_is_likely_binary()intools/file_operations.py.Root Cause
The function reads the first 1000 bytes via
head -c 1000and checks for U+FFFD (replacement character). However, CJK characters are 3 bytes each in UTF-8 (emoji = 4 bytes), so the 1000-byte boundary frequently cuts a character in half. The terminal decoder (usingerrors="replace") produces U+FFFD from the truncated sequence, which the function incorrectly treats as evidence of binary content.Key insight:
head -c 1000is a byte-level operation that doesn't respect UTF-8 character boundaries. A file of 1002 CJK bytes will always trigger this false positive.Fix
When U+FFFD is detected in the sample, instead of immediately returning
True(binary), verify by checking the raw file bytes for NUL (0x00) via Python. NUL is the reliable binary indicator:If no NUL is found, the U+FFFD is a truncation artifact from
head -csplitting a multibyte character, and the file should be treated as text.Changes
tools/file_operations.py:_is_likely_binary()— add NUL byte verification when U+FFFD is detected before flagging as binaryTest Plan
ast.parse)test_replacement_char_sample_flagged_binarystill passes (non-printable ratio check catches genuine encoding errors)test_plain_utf8_text_not_flaggedstill passesFixes #80308