fix(file-ops): tolerate truncated UTF-8 sample boundary - #80497
Closed
loker174 wants to merge 1 commit into
Closed
Conversation
Collaborator
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.
Fix truncated UTF-8 sample boundary in file operations
Bug Description
read_filecan misclassify valid UTF-8 text as binary when its binary-detection sample is produced withhead -c 1000and the 1000-byte boundary falls inside a multi-byte UTF-8 code point. The terminal output layer decodes the truncated sample witherrors="replace", producing a synthetic trailingU+FFFD; the detector then treats that replacement character as proof of binary content.This is especially visible with long CJK Markdown files. The path encoding is not the cause: a Chinese path containing long ASCII content does not reproduce the failure.
Related upstream issue/PRs previously observed during investigation (their
current state should be rechecked by maintainers; this change is a potentially
overlapping alternative, not a claim that the upstream maintainers have
accepted this particular implementation):
These reports and PRs appear to concern the same broad sample-boundary/U+FFFD
failure mode. Their proposed implementations differ from this commit: this
draft keeps the existing non-UTF-8 safety guard and makes the sample-truncated
condition explicit before ignoring a single trailing replacement character.
The commit should therefore be evaluated as a potentially complementary or
alternative implementation, not as an unrelated new bug or a claim that the
other proposals are insufficient.
Root Cause
ShellFileOperations.read_file()andread_file_raw()sample the first 1000 bytes before reading the requested content. The terminal environment decodes command output as UTF-8 witherrors="replace". A byte-truncated multi-byte character therefore becomes a trailingU+FFFD, and_is_likely_binary()previously rejected any sample containing that character.Fix
sample_truncatedfact to_is_likely_binary()based on the measured file size.U+FFFDin the middle of the sample, or in an untruncated sample, as binary evidence. This preserves the existing protection against lossy reads of non-UTF-8 files.read_file()and full-contentread_file_raw().How to Verify
read_fileand confirm it returns text rather thanis_binary=true.read_file_rawon the same file and confirm it returns text.U+FFFDin the middle remains classified as binary..binfile containing NUL bytes remains classified as binary; this exercises the known-binary-extension guard as well as the real-file path, not a universal claim about every binary format.Test Plan
U+FFFD.U+FFFDremains binary.tests/tools/test_file_operations.pypasses:48 passed(run on the clean contribution worktree).python3 -m py_compile tools/file_operations.py tests/tools/test_file_operations.pypasses (clean contribution worktree).git diff --checkpasses (clean contribution worktree).read_fileandread_file_rawpasses; the files were copied from the Hermes repository/docs into Chinese-named paths before testing.Final exact-HEAD verification (
a1fd18985, clean contribution worktree):pytest -q tests/tools/test_file_operations.py→48 passed.python3 -m py_compile tools/file_operations.py tests/tools/test_file_operations.py→ exit 0.git diff --check origin/main...HEAD→ exit 0.read_fileandread_file_rawreturned text; the negativeU+FFFDand known.binchecks retained binary classification.Risk Assessment
Low to medium — the change is limited to binary-sample classification in the two file-reading paths. The main trade-off is a narrow residual ambiguity for a genuinely invalid byte sequence that appears only as the final replacement character of a truncated sample. Replacement characters in the sample body remain binary evidence, and the change does not remove extension-based binary detection or the existing non-printable-byte heuristic.
Notes for Maintainers
This draft uses a small local fix rather than changing all terminal backends or redesigning sampling around raw bytes/base64. If maintainers prefer the more rigorous raw-byte approach, this commit can be adapted or superseded by that design.