Skip to content

fix(file-ops): byte-layer binary detection — stop flagging boundary-cut UTF-8 as binary (#80308 class) - #80440

Closed
ayushnangia wants to merge 1 commit into
NousResearch:mainfrom
ayushnangia:fix/80308-byte-layer-binary-detection
Closed

fix(file-ops): byte-layer binary detection — stop flagging boundary-cut UTF-8 as binary (#80308 class)#80440
ayushnangia wants to merge 1 commit into
NousResearch:mainfrom
ayushnangia:fix/80308-byte-layer-binary-detection

Conversation

@ayushnangia

Copy link
Copy Markdown
Contributor

Summary

Boundary fix for the read_file binary-misclassification class reported in #80308 (Bug 1; Bug 2 is Windows path translation, out of scope here). Binary detection now happens at the byte layer, where "is this UTF-8?" is a well-defined question — instead of on text that the terminal transport has already lossily decoded.

Root cause (the class, not the instance)

read_file samples via head -c 1000 through the terminal transport, which decodes stdout with errors="replace". A multibyte character cut at byte 1000 arrives as U+FFFD, and _is_likely_binary (tools/file_operations.py:906) treats any U+FFFD as binary — so valid CJK/emoji text reads as "Binary file". At the text layer, a stored U+FFFD (legit — logs of lossy output) and a transport-manufactured one are indistinguishable, which is why each open fragment fix — #80261 (@luntion), #80250 (@0809android), #80188 (@shihuaiya), #80349 (@JonthanaHanh), #79834 (@cgordoncarroll), #79534 (@diesdaas), #79408 (@LShang001) — caught a real member of the class while siblings stayed open. Same pattern #80258 closed for the lifecycle-guard class.

Changes

  • _sample_file_bytes(): sample as head -c 1000 … | base64 so raw bytes survive the transport; fail-open to the legacy text heuristic when the transport can't produce clean base64 (nonzero exit, non-base64 output).
  • _is_likely_binary_bytes(): NUL ⇒ binary; valid UTF-8 allowing one incomplete multibyte sequence at the sample's end (an artifact of the byte-boundary cut, UTF-8 sequences ≤ 4 bytes) ⇒ text; mid-stream invalid UTF-8 (latin-1, true binaries) ⇒ read-only — preserving the anti-mojibake guarantee the old U+FFFD check existed for: a read→edit→write round-trip must never rewrite undecodable bytes with replacement characters.
  • Both read paths (read_file, read_file_raw) use the byte layer; extension fast-path unchanged; legacy heuristic kept intact as the fallback.

Validation

  • New TestByteLayerBinaryDetection (16 tests, all fail on main): CJK cut at byte 1000, pure-CJK sample, emoji cut at boundary, UTF-8 BOM, stored-U+FFFD log file, NUL/ELF binaries, latin-1 stays read-only, empty/short samples, invalid-prefix truncation, base64 transport round-trip, non-base64 and nonzero-exit fallbacks, end-to-end read_file returning CJK content, end-to-end NUL binary still blocked.
  • scripts/run_tests.sh tests/tools/test_file_operations.py: 75 passed. Neighbor suites (test_file_operations_edge_cases, test_terminal_task_cwd): green. Pre-existing failures on clean main (test_image_source, test_vision_tools, test_url_safety, test_website_policy, 3 MCP collection errors) are unchanged by this diff — verified by stash-runs.
  • One existing test updated: test_read_file_uses_bash_safe_windows_paths now expects the | base64 sample command (mock answers with base64 payload).

Scope notes

…ort-lossy text

Fixes the read_file half of NousResearch#80308 and the class behind NousResearch#80261, NousResearch#80250,
NousResearch#80188, NousResearch#80349, NousResearch#79834, NousResearch#79534, NousResearch#79408.

The binary sniff sampled files via 'head -c 1000' through the terminal
transport, which decodes stdout with errors="replace". A multibyte
character cut at byte 1000 therefore arrived as U+FFFD, and
_is_likely_binary treated any U+FFFD as binary — flagging valid CJK and
emoji text as unreadable. At the text layer a stored replacement char
and a transport-manufactured one are indistinguishable, which is why
per-callsite adjustments kept leaving siblings open.

Sample as 'head -c 1000 | base64' so raw bytes survive the transport
(fail-open to the legacy heuristic when the transport cannot produce
clean base64), then classify bytes: NUL => binary; valid UTF-8 allowing
one incomplete multibyte sequence at the sample end => text; mid-stream
invalid UTF-8 (latin-1, true binaries) => read-only, preserving the
anti-mojibake guarantee the old check existed for. Files legitimately
containing U+FFFD become readable.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/file File tools (read, write, patch, search) needs-decision Awaiting maintainer decision before any implementation labels Aug 6, 2026
@teknium1

teknium1 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Merged in #81961 — your byte-layer detection (base64 sample transport + _is_likely_binary_bytes) won the bake-off across ~24 competing fixes: it's the only approach that correctly handles truncated multibyte, BOM, genuine binary, NUL-in-text, AND the latin-1 anti-mojibake case, at both the read_file and read_file_raw (patch) sites. Your commit is on main with authorship preserved. Excellent work.

@teknium1 teknium1 closed this Aug 8, 2026
This was referenced Aug 8, 2026
@ayushnangia

Copy link
Copy Markdown
Contributor Author

Thank you — and credit where due: the anti-mojibake case only survived because the original U+FFFD check's comment explained why it existed, so the byte-layer contract was designed to keep its guarantee rather than delete it. The seven fragment PRs mapped the class member by member first; this was a team effort across ~8 people who each caught a real case.

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