fix(file-ops): stop reading truncated UTF-8 samples as binary - #79534
Closed
diesdaas wants to merge 1 commit into
Closed
fix(file-ops): stop reading truncated UTF-8 samples as binary#79534diesdaas wants to merge 1 commit into
diesdaas wants to merge 1 commit into
Conversation
read_file and read_file_raw probe a file with `head -c 1000` before deciding whether it is text. That is a BYTE cut, so a multibyte codepoint straddling the limit decodes (errors="replace") to a trailing U+FFFD -- an artifact the probe itself created. _is_likely_binary treated any U+FFFD in the sample as proof of undecodable bytes, so those files came back as "Binary file - cannot display as text" from both read_file and patch. The file was then neither readable nor editable, with no way for the agent to recover. Non-ASCII text hits this constantly: byte 1000 only has to land inside an umlaut. A German screenplay in the reporter's working directory reproduces it 5 runs out of 5. A marker on the cut itself is ambiguous, and the decoded sample cannot resolve it: a split codepoint and a genuinely undecodable byte sitting on the boundary both leave exactly one trailing U+FFFD. So the check now looks past the boundary instead of guessing. Four more bytes is the most a UTF-8 codepoint can need -- a split one completes and its marker vanishes, a bad byte moves into the body where it is unmistakable, and a read that returns nothing new means the file ended there, so nothing was cut off and the marker was never an artifact. Only that second read can clear a file; anything unresolved stays read-only. The corruption guard this check exists for is therefore intact: lossy content still makes a file read-only, so a read/edit/write round-trip cannot replace the original bytes with mojibake. The >30% non-printable ratio that catches real binaries is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
23 tasks
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.
The bug
read_fileandread_file_rawprobe a file withhead -c 1000before deciding whether it is text. That is a byte cut, so a multibyte codepoint straddling the limit decodes (errors="replace") to a trailing U+FFFD — an artifact the probe itself created._is_likely_binarytreated any U+FFFD in the sample as proof of undecodable bytes, so those files came back asBinary file — cannot display as textfrom bothread_fileandpatch. The file was then neither readable nor editable, with no way for the agent to recover.Non-ASCII text hits this constantly — byte 1000 only has to land inside an umlaut. It surfaced on a German screenplay, where the agent worked around the block by stripping every en dash from the prose.
Reproducer against the current code, no fixtures needed:
The fix
A marker on the cut itself is ambiguous, and the decoded sample cannot resolve it: a split codepoint and a genuinely undecodable byte sitting on the boundary both leave exactly one trailing U+FFFD. Stripping the tail unconditionally would let real corruption through, so the check looks past the boundary instead of guessing.
Four more bytes is the most a UTF-8 codepoint can need:
Only that second read can clear a file; anything unresolved stays read-only. The extra
headruns solely in the ambiguous case.The corruption guard this check exists for is therefore intact — lossy content still makes a file read-only, so a read/edit/write round-trip cannot replace the original bytes with mojibake. The
>30%non-printable ratio that catches real binaries is unchanged.The other
head -cprobes were checked and are unaffected:_detect_file_line_endingonly looks for\r\nvs\n,_file_has_bomcompares an exact 3-byte prefix, andfile_tools.py's guard is extension-only.Tests
tests/tools/test_file_operations.pygains coverage for the split-codepoint case and for both ways a marker on the cut can be genuine (a bad byte followed by more content, and a file that ends on one).make_real_subprocess_envgained alossy_utf8flag so the fixture decodes the wayLocalEnvironmentactually does (encoding="utf-8", errors="replace").Green: every test file that touches
file_operations— 207 tests.Pre-existing failures in
tests/tools/are unrelated and reproduce on a clean tree (test_approval.py,test_clipboard.py—prompt_toolkitis not installed in this environment).🤖 Generated with Claude Code