fix(tools): keep invalid UTF-8 boundaries binary

[Christopher-Schulze]

What does this PR do?

read_file and read_file_raw sample the first 1000 bytes through a terminal stream decoded with errors="replace". When that boundary splits a valid UTF-8 character, the lossy sample contains U+FFFD and the file is incorrectly rejected as binary. This is reproducible with ordinary UTF-8 notes containing Turkish, CJK, or other multibyte text.

The shared binary-detection sampler now performs a small boundary-only extension when the first sample ends with U+FFFD. It accepts valid UTF-8 at the boundary only after a bounded raw-byte probe proves that the replacement is an incomplete sequence; genuine undecodable bytes and truncated content remain fail-closed.

Two contemporaneous direct fixes remain open as #76924 and #76925; the later duplicate #81711 was closed in favor of this PR. This branch is materially stronger for the shared backend surface: it keeps the existing shell abstraction, performs one bounded optional od probe only for the boundary signal, fails closed when that probe is unavailable, and tests the adversarial case where a real invalid byte sits exactly at the extended boundary. That guard is absent from #76925's unconditional trailing-character exemption, while #76924 pays for repeated raw-byte shell probes instead of this single bounded validation.

Related Issue

Fixes #76886; fixes #80308

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✅ Regression tests included with the bug fix

Changes Made

• tools/file_operations.py: centralize the 1000-byte binary sample, retry only when the decoded sample ends at a possible UTF-8 boundary, strictly validate the bounded raw bytes before removing a replacement, and retain the original boundary signal on every unproven path.
• tests/tools/test_file_operations.py: exercise paginated and raw reads with valid multibyte characters at the initial and extended boundaries, add a dense CJK boundary regression for both public paths, and verify invalid bytes at both boundaries remain blocked.

How to Test

1. On the selected base (4983c576b), a real errors="replace" subprocess probe classified 999 ASCII bytes followed by a valid UTF-8 ç as is_binary=True.
2. scripts/run_tests.sh tests/tools/test_file_operations.py -q -> 56 passed.
3. .venv/bin/ruff check tools/file_operations.py tests/tools/test_file_operations.py -> passed.
4. Local final head 9179af050 is rebased onto current origin/main 3d7dda4cf; the canonical changed-file gate passes with 56 tests, Ruff, uv lock --check, and git diff --check. Existing repository-wide ty diagnostics remain advisory only.
5. A post-fix real subprocess probe accepted both paginated and raw reads, while invalid 0xff bytes at the initial and extended sample boundaries remained binary for both public read paths.

Checklist

Code

• Read the current Contributing Guide and project policy files.
• Commit message follows Conventional Commits (fix(tools): ...).
• Searched and reviewed semantic PR candidates; PDF-only and unrelated voice-stack PRs are recorded as non-competing in the worklog.
• The branch contains only the two files required for this fix.
• Ran the focused file-operations suite and all canonical local blocking gates; release verification passed before publication.
• Added regression tests for both public read paths and for fail-closed invalid bytes.
• Tested on macOS arm64 with Python 3.11.15.

Documentation & Housekeeping

• No README or standalone documentation change is needed; this is an internal sampling correction.
• No configuration keys, schemas, or dependencies were added or changed.
• No architecture or workflow policy changed.
• Considered cross-platform terminal decoding: the fix uses the existing shell sampling path, invokes one bounded optional raw-byte probe only on the replacement-character boundary signal, and falls back fail-closed when od is unavailable or cannot prove the boundary.
• No tool descriptions or generated schemas changed.

[teknium1]

Thanks for addressing the confirmed boundary regression; current main still samples with head -c 1000 in both reads (tools/file_operations.py:1168, :1286) and rejects its U+FFFD output at :896.

Problems

• Blocking: tools/file_operations.py:907 removes a lone trailing U+FFFD from the 1004-byte retry without knowing whether it is a valid UTF-8 boundary artifact. For 999 ASCII bytes, a valid ç spanning bytes 1000-1001, two ASCII bytes, then an invalid 0xff at byte 1004 followed by content, the initial sample triggers the retry and the extended output has exactly one trailing U+FFFD. This line removes the invalid-byte signal, so _is_likely_binary() accepts malformed content instead of preserving its existing fail-closed behavior.

Suggested changes

• Retain the fail-closed signal unless the implementation can distinguish raw valid UTF-8 continuation bytes from an invalid byte at the extended boundary.
• Add this extended-boundary invalid-byte case for both read_file and read_file_raw; the current adversarial case at tests/tools/test_file_operations.py:717 only covers an invalid byte in the initial window.

Automated hermes-sweeper review.

[tneemo]

Independent verification — Windows native (WinGet rg + Git Bash)

Verified the current diff on a native Windows host (Git Bash/MSYS terminal backend, %LOCALAPPDATA% Hermes install tree, ripgrep 15.1.0 MSVC via WinGet). Applied the diff manually against local main and ran the suite.

Result: 9/9 pass — all 8 tests in TestReadUtf8SampleBoundary plus the existing test_plain_utf8_text_not_flagged regression:

test_read_file_accepts_multibyte_character_at_sample_boundary PASSED
test_read_file_raw_accepts_multibyte_character_at_sample_boundary PASSED
test_invalid_bytes_at_sample_boundary_remain_binary PASSED
test_invalid_boundary_byte_is_not_hidden_by_a_following_split_character PASSED
test_read_file_rejects_invalid_byte_at_extended_sample_boundary PASSED
test_read_file_raw_rejects_invalid_byte_at_extended_sample_boundary PASSED
test_read_file_accepts_valid_character_split_by_extended_sample_boundary PASSED
test_read_file_raw_accepts_valid_character_split_by_extended_sample_boundary PASSED
test_plain_utf8_text_not_flagged PASSED

This covers the exact matrix the #76886 triage asked for: the original Turkish/CJK boundary repro ('ç' starting at byte 1000), repeated valid boundary splits, invalid bytes at byte 1000 and at the extended boundary, EOF-truncated sequences, and both read_file / read_file_raw paths — with fail-closed behavior preserved (invalid bytes still report binary).

Before/after on the original repro (same file, same content, one byte of offset):

file	unpatched main	with this diff
fails.md (ç at byte 1000)	is_binary=True (bug)	is_binary=False ✅
ok.md (ç inside sample)	is_binary=False	is_binary=False ✅

One portability note (non-blocking): the new tests use make_real_subprocess_env with subprocess.run(..., shell=True), which on a Windows host wraps with cmd.exe and can't find head/wc/od (exit 127). To run the suite natively on Windows I had to invoke bash explicitly ([bash.exe, "-c", command]) in the test harness. The fix itself needed no changes. Worth a small harness tweak if Windows-native test execution is desired, but it doesn't affect the fix's correctness.

This PR is technically ready: MERGEABLE + CLEAN, CI green, and now independently verified on the platform where the bug reproduces.

[Christopher-Schulze]

Maintenance update: rebased onto current main at e6f1d613b683802c6fe7a7d5155fa07724dd4f02 and pushed final head 05fe260b679933f2ad065726cb32a8550059a601. The extended-boundary case from review is now explicitly fail-closed in both read_file and read_file_raw: a raw-byte probe removes the trailing replacement character only when it proves an incomplete valid UTF-8 sequence, while a genuine invalid byte at that boundary remains binary. The regression matrix covers both public paths, initial and extended boundaries, valid split characters, invalid bytes, and truncated input. Final local validation is clean: 54/54 canonical tests, Ruff, uv lock --check, git diff --check, and all blocking gates passed. The independently reported Windows matrix is 9/9. CI has restarted on the rebased head.

[Christopher-Schulze]

Maintenance update

Rebased and published the fail-closed UTF-8 boundary fix on current main.

The extended-boundary retry now removes a replacement character only after a bounded raw-byte probe proves an incomplete valid sequence; genuine invalid bytes remain binary in both read_file and read_file_raw.

• Focused suite: 54/54 passed; independent native Windows verification: 9/9 passed.
• All contributor blocking gates passed.
• Exact new head: cf64b6f11.

GitHub CI is running on this head.

[futureCreator]

Confirming this affects Korean UTF-8 Markdown in real use.

In Hermes Desktop, long Korean Obsidian .md files can be reported as binary when the 1000-byte sample ends inside a multibyte character.

This blocks both reading and editing otherwise valid notes, so the workaround is not practical for a Korean knowledge-base workflow.

#76934 looks like the safest consolidation candidate because it preserves rejection of genuinely invalid UTF-8 while covering both read paths. I can test the final merged build on Korean Markdown files if useful.

[falling-streamlet]

Hi — just closed our own competing duplicate (#81711) in favor of this PR per the contributing guide. Your raw-byte probe approach is strictly stronger than the trailing-char exemption; thanks for the fail-closed design.

One gap worth closing: this PR references #76886 but not #80308 — the same bug reported by a CJK user (valid UTF-8 CJK files misflagged as binary on Windows/git-bash). This fix covers that report too; adding Fixes #80308 to the description would close both issues on merge.

FWIW, our closed PR included a CJK-specific regression test (test_cjk_byte_cut_tail_replacement_not_flagged) that mirrors the dense-CJK boundary case — you may want to add an equivalent to this PR's coverage.

[Christopher-Schulze]

Maintenance update: added a dense-CJK boundary regression covering both read_file and read_file_raw, and the PR description now records coverage of #80308. The fail-closed invalid-byte matrix remains intact. Rebased onto current main; 56/56 focused tests and the canonical checker pass on head 9179af0. #81711 was closed in favor of this PR; the remaining linked lanes were reviewed during the authorized existing-PR update.

[teknium1]

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.