Skip to content

fix(file): classify UTF-8 samples before lossy decoding - #81807

Closed
milnerrad wants to merge 1 commit into
NousResearch:mainfrom
milnerrad:fix/utf8-byte-safe-sampling
Closed

milnerrad wants to merge 1 commit into
NousResearch:mainfrom
milnerrad:fix/utf8-byte-safe-sampling

Conversation

@milnerrad

Copy link
Copy Markdown
Contributor

Summary

Fix false binary classification when the 1,000-byte content sample splits a valid multibyte UTF-8 sequence.

ShellFileOperations currently sends head -c 1000 output through environment APIs that decode stdout with errors="replace". A valid character beginning before byte 1,000 and ending after it therefore arrives as U+FFFD and is classified as binary.

This change:

  • transports a bounded 1,003-byte sample as validated ASCII hex before backend text decoding;
  • strictly validates the 1,000-byte logical UTF-8 prefix with up to three completion bytes;
  • accepts valid 2-, 3-, and 4-byte characters split at every boundary position;
  • treats literal valid U+FFFD as text;
  • rejects malformed and incomplete UTF-8 samples;
  • preserves existing extension and control-character heuristics;
  • consolidates read_file() and read_file_raw() through the byte-safe sampler;
  • fails closed on malformed, truncated, or incomplete sample transport.

patch_replace() is intentionally unchanged. Prefix classification does not prove an entire editable file is valid UTF-8, and the existing display-read helper performs lossy terminal-fence cleanup; edit-path hardening requires a separate lossless full-file snapshot design.

The size probe, bounded sample, and final display read remain separate opens. Exact-length validation fails closed on many growth/shrink races, but it cannot detect same-size replacement or mutation after sampling. This PR fixes the byte-boundary classification defect; it does not claim stable-snapshot or full-file edit validation.

Relationship to existing PRs

Several open PRs address the same symptom, but this implementation differs at the safety boundary:

The regression matrix covers every split position for valid 2-, 3-, and 4-byte UTF-8 characters, malformed continuations at those positions, literal U+FFFD, true-EOF incompleteness, and invalid bytes beginning immediately after the logical boundary.

Reproduction

A valid 4,109-byte UTF-8 file with E2 at byte 1,000 and 80 9D immediately after it:

  • current main: is_binary=True with “Binary file” error;
  • this branch: is_binary=False, no error, and the closing quote is preserved.

Verification

scripts/run_tests.sh tests/tools/test_file*.py tests/tools/test_read_extract.py -q --tb=short
# 346 passed, 0 failed

scripts/run_tests.sh \
  tests/tools/test_file_operations.py \
  tests/tools/test_file_operations_edge_cases.py \
  tests/tools/test_patch_parser.py \
  tests/tools/test_file_ops_cwd_tracking.py \
  -q --tb=short
# 121 passed, 0 failed

ruff check \
  tools/file_operations.py \
  tests/tools/test_file_operations.py \
  tests/tools/test_file_operations_edge_cases.py
# All checks passed

python -m py_compile \
  tools/file_operations.py \
  tests/tools/test_file_operations.py \
  tests/tools/test_file_operations_edge_cases.py

git diff --check

The exact od -An -v -tx1 -N form was also exercised with GNU coreutils and BusyBox. CI will provide the repository-wide test result.

@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 sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 8, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related: this is another implementation for #76886 alongside #76924 and #76925. It uses byte-safe hex transport with continuation lookahead rather than lossy sample handling; maintainer choice is needed on the desired boundary semantics.

@teknium1

teknium1 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Closing — superseded by #81961 (salvaged from #80440). Classify-before-lossy-decode was the right instinct; the merged fix does the same at the byte layer with a lighter transport. Thanks.

@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

comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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