Skip to content

fix(read_file): stop misjudging UTF-8 CJK files as binary - #80188

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

fix(read_file): stop misjudging UTF-8 CJK files as binary#80188
shihuaiya wants to merge 1 commit into
NousResearch:mainfrom
shihuaiya:fix/read-file-utf8-binary

Conversation

@shihuaiya

Copy link
Copy Markdown

read_file/read_file_raw sampled the file head with 'head -c 1000', which truncates at an arbitrary byte boundary. For UTF-8 files whose byte-1000 boundary lands inside a multi-byte char (CJK text, emoji), the terminal env decodes stdout with errors="replace", turning the dangling tail into U+FFFD — and _is_likely_binary's U+FFFD guard then misclassified a perfectly valid text file as binary.

Replace the byte-truncating head sample with _sample_text_head(): a python -c snippet that reads the first 1000 bytes and decodes them as UTF-8, backing off up to 3 bytes on a truncated sequence so the sample always ends on a character boundary. Genuinely non-UTF-8 files (GBK etc.) still fail decode and yield U+FFFD, so the existing binary protection is preserved. Sampling failures err toward binary (safe).

Windows details: paths are normalized to forward slashes and the fallback marker uses chr(0xFFFD) because _escape_shell_arg runs the snippet through _bash_safe_path, which rewrites backslashes (breaking repr'd paths and the '\ufffd' escape).

Verification: character-boundary scan across 0..1001 byte prefixes, CJK/GBK/ASCII/empty/binary fixtures, read_file + read_file_raw.

What does this PR do?

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

How to Test

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform:

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

  • This skill is broadly useful to most users (if bundled) — see Contributing Guide
  • SKILL.md follows the standard format (frontmatter, trigger conditions, steps, pitfalls)
  • No external dependencies that aren't already available (prefer stdlib, curl, existing Hermes tools)
  • I've tested the skill end-to-end: hermes --toolsets skills -q "Use the X skill to do Y"

Screenshots / Logs

read_file/read_file_raw sampled the file head with 'head -c 1000',
which truncates at an arbitrary byte boundary. For UTF-8 files whose
byte-1000 boundary lands inside a multi-byte char (CJK text, emoji),
the terminal env decodes stdout with errors="replace", turning the
dangling tail into U+FFFD — and _is_likely_binary's U+FFFD guard then
misclassified a perfectly valid text file as binary.

Replace the byte-truncating head sample with _sample_text_head(): a
python -c snippet that reads the first 1000 bytes and decodes them as
UTF-8, backing off up to 3 bytes on a truncated sequence so the sample
always ends on a character boundary. Genuinely non-UTF-8 files (GBK
etc.) still fail decode and yield U+FFFD, so the existing binary
protection is preserved. Sampling failures err toward binary (safe).

Windows details: paths are normalized to forward slashes and the
fallback marker uses chr(0xFFFD) because _escape_shell_arg runs the
snippet through _bash_safe_path, which rewrites backslashes (breaking
repr'd paths and the '\ufffd' escape).

Verification: character-boundary scan across 0..1001 byte prefixes,
CJK/GBK/ASCII/empty/binary fixtures, read_file + read_file_raw.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/file File tools (read, write, patch, search) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 6, 2026
@kyssta-exe

Copy link
Copy Markdown
Contributor

Triage note: this appears to duplicate three other PRs that fix the same issue — UTF-8 files whose 1000-byte read_file sample lands mid-multibyte-char (trailing U+FFFD) get misclassified as binary. Same files (tools/file_operations.py + tests), same fix. The cluster: #80186, #80250, #80261. Recommend picking one to champion and closing the rest as duplicates.

@shirahoshi-kanata

Copy link
Copy Markdown

I reviewed all 5 open PRs for this issue (#76934, #79534, #79641, #80188, #80957) and wanted to share a cross-comparison, since they all target the same root cause.

Root cause (all agree): head -c 1000 samples by byte count. When byte 1000 lands inside a multi-byte UTF-8 char, the terminal's errors="replace" decode produces a trailing U+FFFD, which _is_likely_binary's U+FFFD guard correctly-but-falsely flags as binary.

The 5 approaches:

  1. fix(tools): keep invalid UTF-8 boundaries binary #76934 — prove the cut via od hex dump + strict decode. Conceptually sound, but depends on od being present; it fails closed without it, so the original bug would resurface on minimal systems.
  2. fix(file-ops): stop reading truncated UTF-8 samples as binary #79534 — rstrip the trailing U+FFFD, then read past the boundary to distinguish a truncation artifact from a real bad byte. Elegant; handles the exact-1000-byte-file edge case well.
  3. fix(read): stop misclassifying UTF-8 text files as binary #79641 — replace head -c 1000 with a python -c "...read(1000).decode('utf-8','ignore')" snippet. Simple, but errors='ignore' silently drops undecodable bytes — a GBK file's sample becomes garbled-but-printable text and may slip past the binary guard entirely.
  4. fix(read_file): stop misjudging UTF-8 CJK files as binary #80188 — back off up to 3 bytes to a character boundary when sampling. Fixes the sampling itself (root cause), handles MSYS/Windows paths, and genuine non-UTF-8 files (GBK etc.) still produce U+FFFD → binary protection preserved.
  5. fix(file_ops): Korean/CJK UTF-8 files falsely detected as binary #80957 — on U+FFFD, re-validate the real file with errors='strict' via python3. Reliable, but spawns a subprocess on every U+FFFD. fix(file_ops): Korean/CJK UTF-8 files falsely detected as binary #80957's tests (Korean 400-char boundary + real latin-1 file) are excellent.

My assessment: #80188 is the strongest — it addresses the root cause at the sampling layer, is cross-platform aware, and preserves the existing binary guard. Two suggestions for it:

Happy to be corrected on any of this — just sharing the cross-comparison to help the merge decision.

— Airi (IRIS), Hermes Agent

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

@teknium1 teknium1 closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows 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.

5 participants