fix(coding): reject text writes to binary documents

crates/extensions/ironclaw_extension_support/src/coding/file.rs

@@ -26,8 +26,8 @@ use super::{
         virtual_to_relative,
     },
     state::{
-        CodingReadScopeKey, SharedCodingEditLocks, SharedCodingReadStates, content_fingerprint,
-        read_scope_key,
+        CodingReadScopeKey, ReadRepresentation, SharedCodingEditLocks, SharedCodingReadStates,
+        content_fingerprint, read_scope_key,
     },
     text::{
         TextEdit, decode_text, decode_text_lossy, encode_text, previous_char_boundary,
@@ -74,22 +74,26 @@ pub(super) async fn read_file(
             filesystem_error_with_summary("read_file", resolved.scoped_path.as_str(), error)
         })?;
 
-    let content = if should_extract_document_before_text(&bytes, resolved.scoped_path.as_str()) {
-        match extract_document_text_for_read_file(&bytes, resolved.scoped_path.as_str())? {
-            Some(content) => content,
-            None => decode_read_file_text(&bytes)?,
-        }
-    } else {
-        match decode_read_file_text(&bytes) {
-            Ok(content) => content,
-            Err(text_error) => {
-                match extract_document_text_for_read_file(&bytes, resolved.scoped_path.as_str())? {
-                    Some(content) => content,
-                    None => return Err(text_error),
+    let (content, representation) =
+        if should_extract_document_before_text(&bytes, resolved.scoped_path.as_str()) {
+            match extract_document_text_for_read_file(&bytes, resolved.scoped_path.as_str())? {
+                Some(content) => (content, ReadRepresentation::ExtractedText),
+                None => (decode_read_file_text(&bytes)?, ReadRepresentation::RawText),
+            }
+        } else {
+            match decode_read_file_text(&bytes) {
+                Ok(content) => (content, ReadRepresentation::RawText),
+                Err(text_error) => {
+                    match extract_document_text_for_read_file(
+                        &bytes,
+                        resolved.scoped_path.as_str(),
+                    )? {
+                        Some(content) => (content, ReadRepresentation::ExtractedText),
+                        None => return Err(text_error),
+                    }
                 }
             }
-        }
-    };
+        };
 
     let output = read_file_text_output(
         &content,
@@ -108,6 +112,7 @@ pub(super) async fn read_file(
             &read_scope_key(request),
             resolved.virtual_path.as_str(),
             content_fingerprint(&bytes),
+            representation,
         );
     }
 
@@ -353,6 +358,12 @@ pub(super) async fn write_file(
         return Err(input_error());
     }
     let resolved = resolve_required_path(request, "path", FilesystemOperation::WriteFile)?;
+    if is_opaque_binary_document_path(resolved.scoped_path.as_str()) {
+        return Err(binary_document_write_error(
+            "write_file",
+            resolved.scoped_path.as_str(),
+        ));
+    }
     let content = required_str(request.input, "content")?;
     if content.len() > MAX_WRITE_SIZE {
         return Err(input_error());
@@ -369,6 +380,20 @@ pub(super) async fn write_file(
             RuntimeDispatchErrorKind::FilesystemDenied,
         ));
     }
+    // PDF is a text-authorable format (new-file creation is legitimate), but
+    // an existing PDF cannot be safely overwritten via text tools once it has
+    // been read as extracted text — the fingerprint bypass in issue #6898
+    // applies to overwrites only. Block existing-PDF overwrites explicitly;
+    // new PDF creation falls through to the normal write path.
+    if let Some(stat) = &existing_stat
+        && stat.file_type == FileType::File
+        && is_pdf_document_path(resolved.scoped_path.as_str())
+    {
+        return Err(binary_document_write_error(
+            "write_file",
+            resolved.scoped_path.as_str(),
+        ));
+    }
     let can_read = operation_allowed(&resolved.grant.permissions, FilesystemOperation::ReadFile);
     // Overwriting an existing regular file requires a prior full read_file
     // whose fingerprint still matches the file's current bytes (blind
@@ -405,6 +430,7 @@ pub(super) async fn write_file(
         &scope,
         resolved.virtual_path.as_str(),
         content_fingerprint(content.as_bytes()),
+        ReadRepresentation::RawText,
     );
     let output = json!({
         "path": resolved.scoped_path.as_str(),
@@ -436,6 +462,12 @@ async fn verify_read_before_edit(
             resolved.scoped_path.as_str(),
         ));
     };
+    if recorded.representation != ReadRepresentation::RawText {
+        return Err(binary_document_write_error(
+            operation,
+            resolved.scoped_path.as_str(),
+        ));
+    }
     // A file grown past what read_file can return cannot match any recorded
     // read; report it as changed instead of fingerprinting unbounded bytes.
     if stat.len > MAX_READ_SIZE {
@@ -448,12 +480,36 @@ async fn verify_read_before_edit(
         .map_err(|error| {
             filesystem_error_with_summary(operation, resolved.scoped_path.as_str(), error)
         })?;
-    if content_fingerprint(&bytes) != recorded {
+    if content_fingerprint(&bytes) != recorded.fingerprint {
         return Err(stale_read_error(operation, resolved.scoped_path.as_str()));
     }
+    reject_binary_probe(&bytes)
+        .map_err(|_| binary_document_write_error(operation, resolved.scoped_path.as_str()))?;
     Ok(bytes)
 }
 
+fn lower_path_extension(scoped_path: &str) -> Option<String> {
+    scoped_path.rsplit('.').next().map(str::to_ascii_lowercase)
+}
+
+fn is_opaque_binary_document_path(scoped_path: &str) -> bool {
+    matches!(
+        lower_path_extension(scoped_path).as_deref(),
+        Some("doc" | "docx" | "xls" | "xlsx" | "ppt" | "pptx")
+    )
+}
+
+fn is_pdf_document_path(scoped_path: &str) -> bool {
+    matches!(lower_path_extension(scoped_path).as_deref(), Some("pdf"))
+}
+
+fn binary_document_write_error(operation: &str, scoped_path: &str) -> CodingCapabilityError {
+    operation_error_with_summary(format!(
+        "{operation} failed for {}: binary documents cannot be edited with text tools; use a document editing capability that preserves the original format",
+        safe_summary_path(scoped_path)
+    ))
+}
+
 fn read_before_edit_error(operation: &str, scoped_path: &str) -> CodingCapabilityError {
     operation_error_with_summary(format!(
         "{operation} failed for {}: read it in full with read_file before editing it. Ranged reads (offset or limit) and default reads truncated at the line or byte cap do not count as having seen the whole file; a file too large to read in full cannot be edited with this tool",
@@ -664,6 +720,7 @@ pub(super) async fn apply_patch(
         &scope,
         resolved.virtual_path.as_str(),
         content_fingerprint(&output),
+        ReadRepresentation::RawText,
     );
     let mut result = json!({
         "path": resolved.scoped_path.as_str(),

crates/extensions/ironclaw_extension_support/src/coding/mod.rs

@@ -533,6 +533,136 @@ mod tests {
         );
     }
 
+    fn assert_binary_document_rejection(err: &super::CodingCapabilityError, file_hint: &str) {
+        assert_eq!(err.kind(), RuntimeDispatchErrorKind::OperationFailed);
+        let summary = err
+            .safe_summary()
+            .expect("binary-document rejection must carry a model-visible reason");
+        assert!(
+            summary.contains(file_hint),
+            "summary should name the file, got: {summary}"
+        );
+        assert!(
+            summary.contains("binary documents cannot be edited with text tools"),
+            "summary should explain why text editing is unsafe, got: {summary}"
+        );
+    }
+
+    #[tokio::test]
+    async fn write_file_rejects_opaque_document_target_without_touching_bytes() {
+        let fixture = CodingFixture::new("opaque-document-user");
+        let file = fixture.workspace_dir.join("review.docx");
+        let original = b"opaque document bytes";
+        std::fs::write(&file, original).expect("seed document");
+
+        let err = fixture
+            .dispatch(
+                super::CodingCapabilityKind::WriteFile,
+                json!({"path": "/workspace/review.docx", "content": "replacement"}),
+            )
+            .await
+            .expect_err("text write to DOCX must be rejected");
+
+        assert_binary_document_rejection(&err, "review.docx");
+        assert_eq!(
+            std::fs::read(file).expect("document after rejected write"),
+            original
+        );
+    }
+
+    #[tokio::test]
+    async fn extracted_rtf_read_does_not_authorize_raw_overwrite() {
+        let fixture = CodingFixture::new("extracted-rtf-user");
+        let file = fixture.workspace_dir.join("review.rtf");
+        let original = br"{\rtf1\ansi Original RTF text}";
+        std::fs::write(&file, original).expect("seed RTF document");
+
+        let read = fixture
+            .dispatch(
+                super::CodingCapabilityKind::ReadFile,
+                json!({"path": "/workspace/review.rtf"}),
+            )
+            .await
+            .expect("read extracted RTF text");
+        assert!(
+            read.output["content"]
+                .as_str()
+                .expect("read content")
+                .contains("Original RTF text")
+        );
+
+        let err = fixture
+            .dispatch(
+                super::CodingCapabilityKind::WriteFile,
+                json!({"path": "/workspace/review.rtf", "content": "replacement"}),
+            )
+            .await
+            .expect_err("extracted text must not authorize a raw overwrite");
+
+        assert_binary_document_rejection(&err, "review.rtf");
+        assert_eq!(
+            std::fs::read(file).expect("RTF after rejected write"),
+            original
+        );
+    }
+
+    #[tokio::test]
+    async fn write_file_rejects_existing_pdf_but_allows_new_pdf() {
+        let fixture = CodingFixture::new("pdf-write-user");
+        let existing = fixture.workspace_dir.join("existing.pdf");
+        let original = b"%PDF-1.4 existing\n";
+        std::fs::write(&existing, original).expect("seed PDF");
+
+        let err = fixture
+            .dispatch(
+                super::CodingCapabilityKind::WriteFile,
+                json!({"path": "/workspace/existing.pdf", "content": "replacement"}),
+            )
+            .await
+            .expect_err("existing PDF overwrite must be rejected");
+        assert_binary_document_rejection(&err, "existing.pdf");
+        assert_eq!(
+            std::fs::read(existing).expect("PDF after rejected write"),
+            original
+        );
+
+        fixture
+            .dispatch(
+                super::CodingCapabilityKind::WriteFile,
+                json!({"path": "/workspace/new.pdf", "content": "%PDF-1.4 new\n"}),
+            )
+            .await
+            .expect("new PDF creation must remain supported");
+        assert_eq!(
+            std::fs::read(fixture.workspace_dir.join("new.pdf")).expect("new PDF"),
+            b"%PDF-1.4 new\n"
+        );
+    }
+
+    #[tokio::test]
+    async fn read_file_falls_back_to_legacy_document_extraction() {
+        let fixture = CodingFixture::new("legacy-document-user");
+        let file = fixture.workspace_dir.join("legacy.doc");
+        let mut original = b"Legacy document text".to_vec();
+        original.extend_from_slice(&[0; 32]);
+        std::fs::write(file, original).expect("seed legacy document");
+
+        let read = fixture
+            .dispatch(
+                super::CodingCapabilityKind::ReadFile,
+                json!({"path": "/workspace/legacy.doc"}),
+            )
+            .await
+            .expect("legacy document extraction fallback succeeds");
+
+        assert!(
+            read.output["content"]
+                .as_str()
+                .expect("read content")
+                .contains("Legacy document text")
+        );
+    }
+
     #[tokio::test]
     async fn write_file_requires_reading_existing_files_first() {
         let fixture = CodingFixture::new("read-before-write-user");

crates/extensions/ironclaw_extension_support/src/coding/state.rs

@@ -56,13 +56,31 @@ const MAX_READ_STATE_ENTRIES: usize = 8192;
 
 type ReadStateKey = (CodingReadScopeKey, String);
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(super) enum ReadRepresentation {
+    RawText,
+    ExtractedText,
+}
+
+#[derive(Debug, Clone, Copy)]
+pub(super) struct ReadState {
+    pub(super) fingerprint: u64,
+    pub(super) representation: ReadRepresentation,
+}
+
 #[derive(Debug, Default)]
 pub(crate) struct CodingReadStates {
-    entries: std::sync::Mutex<HashMap<ReadStateKey, u64>>,
+    entries: std::sync::Mutex<HashMap<ReadStateKey, ReadState>>,
 }
 
 impl CodingReadStates {
-    pub(super) fn record(&self, scope: &CodingReadScopeKey, path: &str, fingerprint: u64) {
+    pub(super) fn record(
+        &self,
+        scope: &CodingReadScopeKey,
+        path: &str,
+        fingerprint: u64,
+        representation: ReadRepresentation,
+    ) {
         let mut entries = self.lock_entries();
         let key = (scope.clone(), path.to_string());
         if entries.len() >= MAX_READ_STATE_ENTRIES && !entries.contains_key(&key) {
@@ -72,16 +90,22 @@ impl CodingReadStates {
                 entries.remove(&evicted);
             }
         }
-        entries.insert(key, fingerprint);
+        entries.insert(
+            key,
+            ReadState {
+                fingerprint,
+                representation,
+            },
+        );
     }
 
-    pub(super) fn recorded(&self, scope: &CodingReadScopeKey, path: &str) -> Option<u64> {
+    pub(super) fn recorded(&self, scope: &CodingReadScopeKey, path: &str) -> Option<ReadState> {
         self.lock_entries()
             .get(&(scope.clone(), path.to_string()))
             .copied()
     }
 
-    fn lock_entries(&self) -> std::sync::MutexGuard<'_, HashMap<ReadStateKey, u64>> {
+    fn lock_entries(&self) -> std::sync::MutexGuard<'_, HashMap<ReadStateKey, ReadState>> {
         // A poisoned lock means another thread panicked mid-update; the map
         // itself stays coherent (single insert/remove ops), so keep serving.
         match self.entries.lock() {