Attachments: report the added file's own key; drop an emptied indirect /Names

attachment_add compared every attachment name, including files attached to pages as comments, so a page comment with the same file name made the reported key empty. It now compares only the document-level name-tree keys. Deleting the last document attachment when the catalog's /Names is an indirect dictionary left an empty /Names behind; it is now removed from the catalog, as the direct case already did.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

crates/automation/src/lib.rs

@@ -761,9 +761,10 @@ impl Automation {
                 };
                 let mime = pdfcraft_engine::mime_for_name(&name).map(str::to_string);
                 let description = a.opt_str("description")?.map(str::to_string);
-                let before: Vec<String> = self.doc(&a)?.info.attachments.iter().map(|x| x.name.clone()).collect();
+                // Only the document-level keys: an annotation attachment of the same name is not the new one.
+                let before = document_attachment_keys(self.doc(&a)?);
                 let out = self.apply(&a, Edit::AddAttachment { name, bytes, description, mime })?;
-                let key = self.doc(&a)?.info.attachments.iter().map(|x| x.name.clone()).find(|n| !before.contains(n)).unwrap_or_default();
+                let key = document_attachment_keys(self.doc(&a)?).into_iter().find(|n| !before.contains(n)).unwrap_or_default();
                 json!({ "name": key, "document": out })
             }
             "attachment_delete" => {
@@ -2314,6 +2315,18 @@ impl Args<'_> {
 
 /// Bookmark rows in listing order as JSON: a row holds the rows that follow it on the page at the
 /// next level down in `children`, and paths and pages are 1-based.
+/// The name-tree keys of a document's own attachments, leaving out the files attached to pages as comments.
+fn document_attachment_keys(doc: &Document) -> Vec<String> {
+    doc.info
+        .attachments
+        .iter()
+        .filter_map(|x| match &x.source {
+            pdfcraft_render::AttachmentSource::Document { key } => Some(key.clone()),
+            pdfcraft_render::AttachmentSource::Annotation { .. } => None,
+        })
+        .collect()
+}
+
 fn nest_rows(rows: &[pdfcraft_engine::BookmarkRow], pages: &[pdfcraft_render::PageInfo]) -> Vec<Value> {
     // The open rows, outermost first, with their depth and the children gathered so far.
     let mut open: Vec<(usize, Value, Vec<Value>)> = Vec::new();

crates/automation/tests/attachment_key.rs

@@ -0,0 +1,34 @@
+//! `attachment_add` reports the name-tree key of the file it added, even when a file attached to
+//! a page as a comment already has the same name.
+
+use pdfcraft_automation::{Automation, Content};
+use pdfcraft_engine::{AttachIcon, Edit, NewAnnotation, Session, Shape, Style};
+use serde_json::{Value, json};
+
+fn tool(a: &mut Automation, name: &str, args: Value) -> Value {
+    match a.call(name, &args).unwrap().remove(0) {
+        Content::Json(value) => value,
+        other => panic!("expected JSON: {other:?}"),
+    }
+}
+
+#[test]
+fn attachment_add_reports_its_own_key_when_a_page_comment_has_the_same_file_name() {
+    let dir = tempfile::tempdir().unwrap();
+    let mut session = Session::new();
+    let bytes = session.create_blank(300.0, 400.0, 1).unwrap();
+    let id = session.open("a.pdf", None, bytes, None).unwrap();
+    let shape = Shape::Attachment { at: [20.0, 300.0], icon: AttachIcon::Paperclip, file: "notes.txt".into(), data: b"on the page".to_vec() };
+    session
+        .apply(id, Edit::AddAnnotation(NewAnnotation { page: 0, shape, style: Style::default(), contents: String::new(), author: "Ada".into() }))
+        .unwrap();
+    std::fs::write(dir.path().join("a.pdf"), session.save_bytes(id).unwrap().as_slice()).unwrap();
+    std::fs::write(dir.path().join("notes.txt"), b"for the document").unwrap();
+
+    let mut auto = Automation::new().with_root(dir.path()).unwrap();
+    let doc = tool(&mut auto, "doc_open", json!({ "path": "a.pdf" }))["doc"].as_u64().unwrap();
+    let added = tool(&mut auto, "attachment_add", json!({ "doc": doc, "path": "notes.txt" }));
+    assert_eq!(added["name"], "notes.txt", "{added}");
+    let again = tool(&mut auto, "attachment_add", json!({ "doc": doc, "path": "notes.txt" }));
+    assert_eq!(again["name"], "notes.txt (2)", "{again}");
+}

crates/organize/src/attachments.rs

@@ -103,9 +103,16 @@ fn clear_embedded_files(doc: &mut Document) -> Result<(), OrganizeError> {
     let names = doc.get(root).as_dict().and_then(|c| c.get(b"Names").cloned());
     match names {
         Some(Object::Ref(r)) => {
+            let mut empty = false;
             doc.update_dict(r, |n| {
                 n.remove(b"EmbeddedFiles");
+                empty = n.is_empty();
             })?;
+            if empty {
+                doc.update_dict(root, |c| {
+                    c.remove(b"Names");
+                })?;
+            }
         }
         Some(Object::Dict(mut n)) => {
             n.remove(b"EmbeddedFiles");

crates/organize/tests/attachment_names_cleanup.rs

@@ -0,0 +1,48 @@
+//! Deleting the last document attachment when the catalog's `/Names` is an indirect dictionary
+//! (as some producers write it) must not leave an empty `/Names` behind.
+
+use std::sync::Arc;
+
+use pdfcraft_cos::{Document, Object};
+use pdfcraft_organize::{add_attachment, attachment_keys, delete_attachment};
+
+/// One page, and a catalog whose `/Names` is the indirect (empty) dictionary 4 0 R.
+fn indirect_names_pdf() -> Vec<u8> {
+    let objs: [&[u8]; 4] = [
+        b"<< /Type /Catalog /Pages 2 0 R /Names 4 0 R >>",
+        b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>",
+        b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 100 100] >>",
+        b"<< >>",
+    ];
+    let mut out = b"%PDF-1.7\n".to_vec();
+    let mut offsets = Vec::new();
+    for (i, o) in objs.iter().enumerate() {
+        offsets.push(out.len());
+        out.extend_from_slice(format!("{} 0 obj\n", i + 1).as_bytes());
+        out.extend_from_slice(o);
+        out.extend_from_slice(b"\nendobj\n");
+    }
+    let xref = out.len();
+    out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes());
+    for o in offsets {
+        out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+    }
+    out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes());
+    out
+}
+
+#[test]
+fn deleting_the_last_attachment_removes_an_indirect_empty_names_dictionary() {
+    let mut doc = Document::open(Arc::new(indirect_names_pdf())).unwrap();
+    let key = add_attachment(&mut doc, "notes.txt", b"hello", None, None, None).unwrap();
+    assert_eq!(attachment_keys(&doc), ["notes.txt"]);
+    delete_attachment(&mut doc, &key).unwrap();
+    assert!(attachment_keys(&doc).is_empty());
+    let root = doc.root().unwrap();
+    let catalog = doc.get(root).as_dict().cloned().unwrap();
+    assert!(
+        catalog.get(b"Names").is_none(),
+        "the catalog keeps no empty /Names: {:?}",
+        catalog.get(b"Names").map(|n| matches!(n, Object::Ref(_)))
+    );
+}