Skip to content

fix(file_operations): U+FFFD density threshold in binary detection - #82494

Open
zhuyuhao0612 wants to merge 2 commits into
NousResearch:mainfrom
zhuyuhao0612:fix/read-file-fffd-density
Open

zhuyuhao0612 wants to merge 2 commits into
NousResearch:mainfrom
zhuyuhao0612:fix/read-file-fffd-density

Conversation

@zhuyuhao0612

Copy link
Copy Markdown

Problem

_is_likely_binary samples the first 1000 bytes (via head -c 1000). When the 1000th byte lands inside a multi-byte UTF-8 character (e.g. a 3-byte CJK char), the truncated tail decodes to U+FFFD replacement chars. The old logic flagged any U+FFFD as binary, so ordinary Chinese-language text files crossing the 1000-byte boundary were refused as "Binary file" and became unreadable via read_file.

Fix

Flag binary only when U+FFFD density exceeds 2%. Real binary / GBK-encoded content produces >10% density, so the protection against garbled writes is preserved; a boundary cut produces at most ~0.3-0.5%, so legitimate text files pass.

Tests

Adds TestUtf8BoundaryCutNotBinary (3 cases) in tests/tools/test_file_operations.py:

  • multi-byte UTF-8 boundary cut at byte 1000 → NOT binary (regression)
  • UTF-16 bytes with high U+FFFD density → still binary
  • density threshold boundary behavior
uvx pytest tests/tools/test_file_operations.py -q -k 'binary or Binary or likely'
7 passed, 42 deselected

Impact

Reads previously failing on Chinese/CJK files between 1000 and ~50000 bytes now work. No behavior change for genuine binary content.

_is_likely_binary sampled the first 1000 bytes via head -c 1000; when the
1000th byte landed inside a multi-byte UTF-8 char (e.g. CJK), the truncated
tail decoded to U+FFFD. Old logic flagged ANY U+FFFD as binary, so normal
Chinese text files crossing the 1000-byte boundary were refused as binary.

Fix: only flag binary when U+FFFD density > 2% (real binary/GBK >10%).
@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 labels Aug 9, 2026

@szzhoujiarui szzhoujiarui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now superseded by main commit e40315d, which fixes the same boundary-cut issue at the byte layer by transporting the sample through base64.

The 2% replacement-character threshold still classifies sparse invalid UTF-8 as text. For example, a 1000-byte mostly-ASCII file with one invalid byte in the middle decodes to one U+FFFD (0.1% density), so this patch allows a lossy read and a later write can silently corrupt the original byte.

The byte-layer implementation on main correctly accepts an incomplete UTF-8 sequence at the sample boundary while rejecting mid-stream invalid UTF-8. Given the existing conflict and broader main coverage, I recommend closing this PR as superseded.

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 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