Skip to content

fix: stop misclassifying UTF-8 files cut at the 1000-byte sample boundary as binary - #81810

Closed
tianliuyun wants to merge 1 commit into
NousResearch:mainfrom
tianliuyun:fix/read-file-utf8-truncation-false-binary
Closed

fix: stop misclassifying UTF-8 files cut at the 1000-byte sample boundary as binary#81810
tianliuyun wants to merge 1 commit into
NousResearch:mainfrom
tianliuyun:fix/read-file-utf8-truncation-false-binary

Conversation

@tianliuyun

Copy link
Copy Markdown

Problem

read_file samples the first 1000 bytes with head -c 1000 and decodes with errors=replace. When byte 1000 lands mid-character in a multi-byte UTF-8 sequence, the truncated tail decodes to a single trailing U+FFFD, and _is_likely_binary() treated any U+FFFD as binary evidence. Result: perfectly valid UTF-8 text files whose 1000th byte lands mid-character are refused with Binary file - cannot display as text.

Reproduction

data = b"a" * 999 + "汉字测试\n".encode("utf-8")
open("/tmp/utf8_boundary.md", "wb").write(data)
  • head -c 1000 cuts the 汉字 at byte 1000
  • errors=replace decode yields a...a\ufffd
  • _is_likely_binary returns True → file refused, despite being 100% valid UTF-8 (file says Unicode text, UTF-8 text)

Fix

Distinguish the truncation artifact from real undecodable bytes:

  • Real binary: U+FFFD appears in the middle of the sample, and/or more than once
  • Truncation artifact: exactly one U+FFFD squeezed against the sample tail (UTF-8 sequences are ≤4 bytes, so a boundary cut loses ≤3 bytes)

A lone trailing U+FFFD now falls through to the existing non-printable ratio check, which still catches NUL-heavy binary content. The original mojibake-corruption guard is preserved: files with genuine undecodable bytes anywhere in the sample are still treated as binary (read-only).

Tests

Added 5 regression tests to tests/tools/test_file_operations_edge_cases.py:

  • test_truncation_artifact_trailing_replacement_char — the bug case (artifact ≠ binary)
  • test_replacement_char_in_middle_is_binary — mid-sample U+FFFD still binary
  • test_multiple_replacement_chars_is_binary — two U+FFFDs still binary
  • test_trailing_replacement_char_with_binary_ratio — ratio check still catches NUL-heavy content

All tests/tools/ file_operations tests pass (73 passed). Full tests/tools/ run: 5443 passed; remaining failures are pre-existing environment issues (missing parallel-web lazy dep, external modal/fal/video services).

Verification

End-to-end repro: a valid UTF-8 file with a boundary-cut character now reads correctly via read_file (content intact), while a real binary file (NUL + non-UTF-8 bytes) is still rejected as binary.

…dary as binary

read_file samples the first 1000 bytes with head -c 1000 and decodes with
errors=replace. When byte 1000 lands mid-character in a multi-byte UTF-8
sequence, the truncated tail decodes to a single trailing U+FFFD, and
_is_likely_binary() treated any U+FFFD as binary evidence — so perfectly
valid text files were refused with 'Binary file - cannot display as text'.

Distinguish the truncation artifact from real undecodable bytes: a true
binary sample carries U+FFFD in the middle of the sample and/or more than
once, whereas a boundary cut produces at most one replacement char
squeezed against the tail (UTF-8 sequences are ≤4 bytes, so a cut loses
≤3 bytes). A lone trailing U+FFFD now falls through to the existing
non-printable ratio check, which still catches NUL-heavy binary content.

Adds regression tests covering the artifact case, mid-sample U+FFFD,
multiple U+FFFDs, and the ratio check still firing with a trailing
artifact char.
@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation labels Aug 8, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this fixes the #76886 UTF-8 sample-boundary false-binary family, but competes with #80250 and #79408 on trailing-U+FFFD safety semantics. This PR permits exactly one replacement character near the sample tail; the other open PRs use broader trailing-run carve-outs.

@tianliuyun

Copy link
Copy Markdown
Author

Thanks for the triage note. Since the trailing-U+FFFD safety semantics are the point of comparison here, let me spell out why this PR deliberately chose the narrower carve-out, and what the trade-off is.

The three approaches side by side

PR Carve-out What gets exempted
#79408 rstrip("\ufffd") (unbounded) any trailing run of U+FFFD
#80250 rstrip then keep if run ≤ 3 trailing run of ≤3 U+FFFD
#81810 (this) count == 1 AND position in last 3 chars exactly one U+FFFD squeezed against the tail

Why "exactly one" is the physically correct bound

head -c 1000 cuts at a byte boundary. A multi-byte UTF-8 sequence (≤4 bytes) split by that cut yields at most one incomplete trailing sequence, which errors="replace" decodes to exactly one U+FFFD. So:

  • A legitimate truncation artifact can never produce 2+ U+FFFDs at the tail.
  • Therefore, 2+ trailing U+FFFDs in a sample means the file genuinely contains replacement chars (or real undecodable bytes) — and that should stay binary.

The unbounded rstrip in #79408 and the run-of-≤3 in #80250 both exempt cases that the truncation model says cannot happen from a clean cut. Those extra exemptions are not "handling more edge cases" — they are windows where a genuinely non-UTF-8 file (e.g. one whose tail happens to decode to a few U+FFFDs) gets let through, which is exactly the mojibake-corruption risk the original guard exists to prevent.

The safety direction

The original code's comment is explicit: the U+FFFD check exists so a read→edit→write round-trip cannot silently overwrite real bytes with replacement chars. A fix for a false-positive (valid text flagged binary) should not weaken that false-negative guard. This PR keeps the guard intact for every case except the single provable artifact: one U+FFFD in the final 3 chars, which is the maximum damage a 4-byte UTF-8 cut can do.

I've added a regression test (test_trailing_replacement_char_with_binary_ratio) asserting that a trailing artifact char does not mask NUL-heavy binary content — the ratio check still fires. Happy to adjust if maintainers prefer the broader carve-out, but wanted the safety trade-off on the record.

@teknium1

teknium1 commented Aug 8, 2026

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists tool/file File tools (read, write, patch, search) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants