fix(file-ops): don't judge a byte-sliced sample tail as binary - #81066
Closed
weskerqu-stack wants to merge 1 commit into
Closed
fix(file-ops): don't judge a byte-sliced sample tail as binary#81066weskerqu-stack wants to merge 1 commit into
weskerqu-stack wants to merge 1 commit into
Conversation
_is_likely_binary treats any U+FFFD in the sample as proof the file is non-UTF-8 (021a076, guarding read->write mojibake corruption). But the sample comes from `head -c 1000`, which cuts on a BYTE boundary: the last character is routinely a multibyte one (CJK, emoji) sliced in half, and errors="replace" renders that truncated sequence as exactly one trailing U+FFFD. Ordinary UTF-8 text then reads as binary. Measured on one host: 130 of 349 real Chinese .md files (37.2%) were unreadable through read_file, including scheduled-job reports three days running -- each plain UTF-8 with zero NUL bytes. Fix: ignore a single U+FFFD at the sample's tail -- an artifact of how we sample, not evidence about the file. A genuinely non-UTF-8 file carries U+FFFD inside the sample too and still trips the check. Python emits exactly one U+FFFD per truncated multibyte sequence (verified for 2-, 3- and 4-byte characters cut at every offset), the same property process_registry.py documents at its EOF flush. Verified: both 021a076 regression tests still pass; latin-1 and true-binary fixtures still classified binary; 349 real Chinese text files went from 130 misjudged to 0; tests/tools/ full run has an identical failure set before and after (125 pre-existing, unrelated).
Collaborator
Duplicate of #76925: both suppress the synthetic trailing replacement character created when the byte-limited UTF-8 sample cuts a multibyte character. |
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.
What & why
ShellFileOperations._is_likely_binarytreats any U+FFFD in the sample as proof the file is non-UTF-8. That guard (021a076) is right in intent, but the sample it inspects comes fromhead -c 1000, which cuts on a byte boundary. When the 1000th byte lands mid-character — routine for CJK (3 bytes/char) and emoji (4 bytes) — the trailing partial sequence is decoded by the terminal env'serrors="replace"into exactly one U+FFFD, and the file is reported as binary.read_fileandread_file_rawthen refuse ordinary UTF-8 text files.Measured on my machine: 130 of 349 real-world Chinese
.mdfiles (37.2%) were unreadable throughread_file— each plain UTF-8 with zero NUL bytes,file(1)reportingUTF-8 Unicode text. Among them, the same scheduled-job report three days running.The fix
Ignore a single U+FFFD at the tail of the sample — an artifact of how we sample, not evidence about the file. A genuinely non-UTF-8 file carries U+FFFD inside the sample too and still trips the check.
Python's
errors="replace"emits exactly one U+FFFD for a truncated multibyte sequence (verified for 2-, 3- and 4-byte characters cut at every offset), which is what makes a one-character tail window sufficient — the same propertytools/process_registry.pyalready documents at its EOF flush.How to test
Regression coverage that must stay green: both tests from 021a076 (
TestReadNonUtf8IsBinary), plus latin-1 and true-binary fixtures still classified binary.Test results
scripts/run_tests.sh tests/tools/(upstream CI runner, per-file isolation): identical failure set before and after this patch — 110 failures across 29 files, diffed line by line, all pre-existing in this environment (optional deps: vision / voice / web / MCP-OAuth).test_file_operations.py+ 7 adjacent file-tool suites: 142 passed.Platforms tested
macOS 26.5.2 (arm64), Python 3.11.15. Not tested on Linux or WSL2 — the change is pure Python string handling with no platform-specific paths.
Known limitation
A file whose first non-UTF-8 byte lands exactly at byte 1000, with 999 clean bytes before it, is now read as text. This sits strictly inside the guard's existing blind spot — it only ever inspects the first 1000 bytes, so any non-UTF-8 byte past that offset already goes undetected. A byte-exact alternative (base64 the sample so raw bytes survive transport) closes that hole but changes the exec sequence enough to break three existing mock-based tests; happy to pursue it in a follow-up if you'd prefer that trade.
🤖 Generated with Claude Code