Skip to content

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

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

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
@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 Sep 22, 2026
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

PR closed after 10 days of inactivity.

@github-actions github-actions Bot closed this Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. 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