fix(read): prioritize known text extensions

[original4422]

Summary

• Treat known text extensions as authoritative before content sniffing, preventing embedded magic-byte literals from turning source files into image/PDF payloads.
• Preserve infer detection for unknown and extensionless files and preserve the existing extension fallback.
• Exercise the real ForgeFsRead path and MIME compatibility matrix with same-file regression tests.

Root cause

infer::get scans the entire supplied buffer and recognizes the %PDF sequence at byte offset 449. Limiting the buffer to the issue's suggested 1 KiB would still include that sequence and therefore would not fix this reproduction. The minimal reliable fix is to trust the existing known-text extension allowlist before content sniffing.

Validation

• cargo test -p forge_services tool_services::fs_read::tests -- --nocapture — 28 passed
• cargo test -p forge_services — 216 passed; doc tests passed
• cargo check -p forge_services — passed
• cargo clippy -p forge_services --all-targets --all-features -- -D warnings — passed
• cargo fmt -p forge_services -- --check — passed
• git diff --check — passed

cargo insta test was unavailable because the local cargo-insta command is not installed, so the complete crate suite was run directly with cargo test. A workspace-wide clippy attempt stopped while building the untouched forge_repo crate because local protoc is absent; CI installs protoc, and the touched crate's all-target/all-feature clippy is green.

Fixes #3812

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[CLAassistant]

All committers have signed the CLA.

[original4422]

Hi @tusharmath, when you have a chance, would you mind reviewing this MIME-detection fix and letting me know if any changes are needed? Thank you!

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

[github-actions]

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

[original4422]

Status update: this PR is still active and ready for review. The current head remains c8f0320e721b4adaf0bd889c1f7f60456f6f295a and GitHub reports it mergeable.

Validation recorded for this unchanged head includes 28/28 focused fs_read tests, 216/216 forge_services tests, plus passing crate cargo check, all-target/all-feature clippy, and formatting. CLA Assistant confirms all committers have signed. The ci, Bounty Management, and autofix.ci runs remain at the external-contributor action_required gate with zero jobs, so there is no actionable test failure log; Release Drafter passed.

Maintainers, could you please review this when available and approve the gated workflows if appropriate? Thank you.

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

[github-actions]

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

[original4422]

Status update: this PR is still active and ready for review at the unchanged head c8f0320e721b4adaf0bd889c1f7f60456f6f295a. The recorded focused and crate validation remains green; the contributor workflows are still awaiting approval and have no job logs. I’m happy to make any requested changes. Could a maintainer please approve the workflows and review when convenient? Thank you.

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

[github-actions]

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

[amitksingh1490]

Review of the unchanged head c8f0320e7 for #3812: the contributor's production fix is sufficient for the reported known-source-extension false positive. I have not changed production code, pushed this branch, or opened a competing PR.

Independent verification on Linux:

• Original head: cargo test --locked -p forge_services — 216 passed.
• With optional test-only additions below: same command — 221 passed (including 33 fs_read tests); cargo insta test --test-runner cargo-test -p forge_services — 221 passed, no snapshots to review.
• cargo clippy --locked -p forge_services --all-targets --all-features -- -D warnings — passed.
• cargo fmt -p forge_services -- --check and git diff --check — passed.
• cargo build --locked -p forge_main (debug) and forge --help — passed. Build used installed protoc and a local OpenSSL SDK; debug symbols disabled and two build jobs for container resources.
• Negative control: temporarily restoring content-first sniffing makes test_fs_read_magic_source_matrix fail; restoring this PR's implementation returns the complete suite to 221 passed.

The additions exercise ForgeFsRead::read itself with mock raw-byte infrastructure: all 12 existing text/notebook extensions in lower/uppercase with PDF at offset 449, escaped/Unicode PNG literals and a GIF-signature source identifier; a complete one-pixel PNG under normal/uppercase/unknown/extensionless/mismatched media extensions; invalid UTF-8 renamed to .ts; and unknown/extensionless text and PDF-magic fallback. The 96 source/case combinations remain File content, not Image content. This is service-path verification, not a successful live-provider run.

One root-cause clarification: infer 0.22's PDF matcher already searches only the first 1024 bytes, rather than the entire buffer. The reported offset 449 is within that window, so the suggested 1 KiB cap cannot fix the reproduction. Unknown/extensionless files intentionally retain sniffing, including embedded PDF magic. Related #3633 / #3749 changes read_range_utf8; this service uses raw read, so no detector-unification changes were made here.

Remaining verification blockers (not ready-to-merge evidence):

• The actual noninteractive forge -p smoke attempt used an isolated config without credentials. It stopped before any read/model call with No such device or address (os error 6) during startup/provider selection, despite exit code 0. Live verification needs an authenticated provider configured interactively; no credentials are included here.
• Latest-head ci, autofix.ci, and Bounty Management are still action_required; Release Drafter is the only passing workflow. No workflows were approved or automation changed.

@original4422, please consider folding in the optional test-only patch below, or equivalent coverage. It applies on your current head and preserves all your existing code/tests. This comment shares the additions without pushing your branch or duplicating your PR. The existing non-draft PR has been left as-is.

Optional supplemental regression patch (test code only)

diff --git a/crates/forge_services/src/attachment.rs b/crates/forge_services/src/attachment.rs
index 7b3da13d7..04ea8d208 100644
--- a/crates/forge_services/src/attachment.rs
+++ b/crates/forge_services/src/attachment.rs
@@ -475,6 +475,15 @@ pub mod tests {
         pub fn add_file(&self, path: PathBuf, content: String) {
             self.file_service.add_file(path, content);
         }
+
+        /// Adds raw bytes without requiring binary fixtures to be valid UTF-8.
+        pub fn add_bytes(&self, path: PathBuf, content: Vec<u8>) {
+            self.file_service
+                .files
+                .lock()
+                .unwrap()
+                .push((path, Bytes::from(content)));
+        }
     }
 
     #[async_trait::async_trait]
diff --git a/crates/forge_services/src/tool_services/fs_read.rs b/crates/forge_services/src/tool_services/fs_read.rs
index 6a71d0fd8..0f379b9c9 100644
--- a/crates/forge_services/src/tool_services/fs_read.rs
+++ b/crates/forge_services/src/tool_services/fs_read.rs
@@ -394,6 +394,136 @@ mod tests {
         assert_eq!(actual, expected);
     }
 
+    fn read_fixture(path: &str, content: &[u8]) -> ForgeFsRead<MockCompositeService> {
+        let infra = Arc::new(MockCompositeService::new());
+        infra.add_bytes(PathBuf::from(path), content.to_vec());
+        ForgeFsRead::new(infra)
+    }
+
+    fn png_fixture() -> anyhow::Result<Vec<u8>> {
+        use base64::Engine;
+
+        // Complete one-pixel RGB PNG, including IHDR, IDAT and IEND chunks.
+        base64::engine::general_purpose::STANDARD
+            .decode(concat!(
+                "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAIAAACQd1Pe",
+                "AAAADElEQVR4nGP4z8AAAAMBAQDJ/pLvAAAAAElFTkSuQmCC"
+            ))
+            .context("Failed to decode PNG fixture")
+    }
+
+    #[tokio::test]
+    async fn test_fs_read_magic_source_matrix() {
+        let sources = [
+            typescript_with_embedded_pdf_magic(),
+            r#"const magic = "\x89PNG";"#.to_string(),
+            "// PNG signature: ‰PNG".to_string(),
+            "GIF89a; // A source identifier that also matches image magic".to_string(),
+        ];
+        let extensions = [
+            "txt", "md", "rs", "toml", "yaml", "yml", "json", "js", "ts", "py", "sh", "ipynb",
+        ];
+        for source in sources {
+            for extension in extensions {
+                for extension in [extension.to_string(), extension.to_ascii_uppercase()] {
+                    let path = format!("/test/source.{extension}");
+                    let fixture = read_fixture(&path, source.as_bytes());
+
+                    let output = fixture.read(path, None, None).await.unwrap();
+                    let actual = (output.content.file_content().to_string(), output.info);
+
+                    let expected = (
+                        source.clone(),
+                        FileInfo::new(1, 2000, 1, compute_hash(&source)),
+                    );
+                    assert_eq!(actual, expected);
+                    assert!(output.content.as_image().is_none());
+                }
+            }
+        }
+    }
+
+    #[tokio::test]
+    async fn test_fs_read_binary_png_preserves_visual_payload() {
+        let content = png_fixture().unwrap();
+        for path in [
+            "/test/pixel.png",
+            "/test/pixel.PNG",
+            "/test/pixel.unknown",
+            "/test/pixel",
+            "/test/pixel.pdf",
+        ] {
+            let fixture = read_fixture(path, &content);
+
+            let output = fixture.read(path.to_string(), None, None).await.unwrap();
+            let actual = (output.content.as_image().cloned(), output.info);
+
+            let image = Image::new_bytes(content.clone(), "image/png");
+            let info = FileInfo::new(0, 0, 0, compute_hash(image.url()));
+            let expected = (Some(image), info);
+            assert_eq!(actual, expected);
+        }
+    }
+
+    #[tokio::test]
+    async fn test_fs_read_invalid_utf8_with_text_extension_is_not_visual() {
+        let content = png_fixture().unwrap();
+        let path = "/test/not_source.ts";
+        let fixture = read_fixture(path, &content);
+
+        let actual = fixture
+            .read(path.to_string(), None, None)
+            .await
+            .unwrap_err();
+
+        let expected = format!("Failed to read file as UTF-8 from {path}");
+        assert_eq!(actual.to_string(), expected);
+    }
+
+    #[tokio::test]
+    async fn test_fs_read_unknown_and_extensionless_text_fallback() {
+        for path in ["/test/source.unknown", "/test/source"] {
+            for content in ["plain text", "", r#"const magic = "\x89PNG";"#] {
+                let fixture = read_fixture(path, content.as_bytes());
+
+                let output = fixture.read(path.to_string(), None, None).await.unwrap();
+                let actual = (output.content.file_content().to_string(), output.info);
+
+                let expected = (
+                    content.to_string(),
+                    FileInfo::new(
+                        1,
+                        2000,
+                        u64::from(!content.is_empty()),
+                        compute_hash(content),
+                    ),
+                );
+                assert_eq!(actual, expected);
+                assert!(output.content.as_image().is_none());
+            }
+        }
+    }
+
+    #[tokio::test]
+    async fn test_fs_read_unknown_and_extensionless_pdf_magic_remains_visual() {
+        let content = typescript_with_embedded_pdf_magic();
+        for path in [
+            "/test/document.pdf",
+            "/test/document.unknown",
+            "/test/document",
+        ] {
+            let fixture = read_fixture(path, content.as_bytes());
+
+            let output = fixture.read(path.to_string(), None, None).await.unwrap();
+            let actual = (output.content.as_image().cloned(), output.info);
+
+            let image = Image::new_bytes(content.as_bytes().to_vec(), "application/pdf");
+            let info = FileInfo::new(0, 0, 0, compute_hash(image.url()));
+            let expected = (Some(image), info);
+            assert_eq!(actual, expected);
+        }
+    }
+
     #[test]
     fn test_detect_mime_type_for_ipynb() {
         let fixture = typescript_with_embedded_pdf_magic();

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

[original4422]

Thanks for the independent verification and supplemental patch. I folded in a focused subset as commit 600e237: the real ForgeFsRead path now verifies that .pdf, unknown-extension, and extensionless inputs with embedded PDF magic remain visual content. I left out the broader raw-byte helper and 96-case matrix because the existing tests already cover the full known-text allowlist and the reported source-file path, so this keeps the PR regression-focused.

Validation on the submitted head:

• cargo test --locked -p forge_services test_fs_read_unknown_and_extensionless_pdf_magic_remains_visual -- --nocapture — 1 passed
• cargo test --locked -p forge_services — 217 passed; doc tests passed
• cargo clippy --locked -p forge_services --all-targets --all-features -- -D warnings — passed
• cargo fmt -p forge_services -- --check — passed
• git diff --check — passed

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