Skip to content

fix(forge_fs): unify is_binary detector in read_range_utf8 - #3749

Open
MeGaurav4 wants to merge 1 commit into
tailcallhq:mainfrom
MeGaurav4:fix/unify-is-binary-detector-3633
Open

MeGaurav4 wants to merge 1 commit into
tailcallhq:mainfrom
MeGaurav4:fix/unify-is-binary-detector-3633

Conversation

@MeGaurav4

Copy link
Copy Markdown

read_range_utf8 used the infer-based ForgeFS::is_binary (MIME sniff over 8192 bytes, assumes text when the type is unknown), while fs_search, fs_write, plan_create, and image_read all used the BOM + zero-byte detector in binary_detection.rs. A file with a stray 0x00 in the first 512 bytes was skipped as binary by search but read as text by read_range_utf8.

This PR routes read_range_utf8 through the same is_binary(path) detector used by every other caller, and removes the now-unused infer-based implementation, its tests, and the infer dependency.

Files changed:

  • crates/forge_fs/src/read_range.rs: use is_binary(path_ref) instead of Self::is_binary(&mut file); add regression test
  • crates/forge_fs/src/is_binary.rs: deleted (no remaining callers)
  • crates/forge_fs/src/lib.rs: remove mod is_binary;
  • crates/forge_fs/Cargo.toml: remove infer dependency

Fixes #3633

Verification

  • cargo test -p forge_fs --lib passes (10 tests, including new test_stray_zero_byte_classified_as_binary)
  • cargo clippy -p forge_fs --lib is clean
  • cargo check -p forge_infra and cargo check -p forge_app pass (no downstream breakage)

read_range_utf8 used the infer-based ForgeFS::is_binary (MIME sniff,
8192 bytes, assumes text when type is unknown) while fs_search and
all other callers used the BOM + zero-byte detector in
binary_detection.rs. The same file could be skipped as binary by
search but read as text by read_range.

Route read_range_utf8 through the same is_binary(path) detector and
remove the now-unused infer-based implementation and its dependency.

Fixes tailcallhq#3633
@github-actions github-actions Bot added the type: fix Iterations on existing features or infrastructure. label Jul 23, 2026
@CLAassistant

CLAassistant commented Jul 23, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

Copy link
Copy Markdown

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

@github-actions github-actions Bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Jul 29, 2026
@MeGaurav4

Copy link
Copy Markdown
Author

The fix is complete and the branch is up to date. This PR has been open for 5 days awaiting review, happy to rebase or adjust if anything needs changing.

@github-actions github-actions Bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 2, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Action required: PR inactive for 5 days.
Status update or closure in 10 days.

@github-actions github-actions Bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Aug 7, 2026
@amitksingh1490

Copy link
Copy Markdown
Contributor

Independent review of head 116f976f0 against refreshed main 6ed5d37b6 for #3633. The inactive label was applied with the inactivity-bot notice; main still has both detectors, and I found no merged/superseding detector fix. This PR correctly removes the duplicate low-level detector; I have not created a duplicate PR or modified your branch.

Remaining acceptance gap: the current public read tool does not call read_range_utf8. See /home/forge/worktrees/3633/crates/forge_services/src/tool_services/fs_read.rs:125-166: it reads raw bytes, routes visual MIME, then uses String::from_utf8. Thus KEY=value\0trailing text remains readable through that service while search skips it via metadata detection (/home/forge/worktrees/3633/crates/forge_services/src/tool_services/fs_search.rs:217-219). The PR fixes the reported low-level pair, but not the broader user-facing read/search symptom. Also, current write no longer invokes is_binary: its existing-content read is lossy UTF-8 (/home/forge/worktrees/3633/crates/forge_services/src/tool_services/fs_write.rs:80-88, /home/forge/worktrees/3633/crates/forge_fs/src/read.rs:7-10). Please narrow the all-callers claim or extend this existing PR with service-level coverage/policy; preserve intended image/PDF reads rather than blanket-rejecting visual content.

#3848 interaction: its known-source-extension precedence is a separate MIME-routing fix and does not supersede this consolidation. It leaves the nonvisual strict-UTF8 read path intact, so both changes can coexist but neither closes that service-level NUL gap by itself. No changes made to that work.

Coverage/validation: an isolated rustc probe using the exact pure detector functions passed 15 cases: empty/plain, isolated NUL, UTF-8 BOM with/without NUL, UTF-16 with/without BOM, short PNG headers, unknown non-UTF8 bytes, and NUL at indices 511/512. This confirms existing heuristic limitations (UTF-16 classification is not decoding; short no-NUL binary headers and NUL beyond the sample remain nonbinary), NOT crate or service test success. The added regression should assert BinaryFileNotSupported, not just any error, and a shared corpus should exercise the real operation paths.

git diff origin/main...HEAD --check passed; git merge-tree --write-tree HEAD origin/main was conflict-free. rustfmt +1.97 --check --edition 2024 crates/forge_fs/src/read_range.rs exited 1 with formatting differences at the new error construction and assertion (also warned about nightly-only config). Only Release Drafter ran on this head; there is no passing test/Clippy CI evidence. Crate tests, Clippy and debug-cli build/--help/-p verification were NOT run: /data is 95% full with only 1.6 GB available and this isolated worktree has no target cache. No large build or cleanup of others' artifacts attempted. Not claiming PR readiness; validation needs additional build capacity and current-head CI.

Co-Authored-By: ForgeCode noreply@forgecode.dev

@github-actions github-actions Bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: fix Iterations on existing features or infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: two divergent is_binary detectors in forge_fs — fs_search/fs_write vs read_range disagree on same file

3 participants