M4: bookmark tree round-trips what real files hold; review fixes (#6018)

- organize: a /FitR destination missing one of its four numbers is copied
  with its page and no view, instead of failing the whole copy (the
  rectangle can't be kept, and import refuses a partial one).
- organize: `visit` keeps the visited node's path in step with its stack
  instead of rebuilding it per node (O(depth) per node before).
- automation: an empty title is accepted when present, so a bookmark a
  file left untitled exports and imports back; a missing title is still
  refused.
- automation: "no bookmark at" names the path 1-based, as it was given.
- automation: `bookmark_copy` and `bookmark_import` report `truncated`;
  an exported file carries `"truncated": true` when the export left
  bookmarks out, and importing it says so.
- automation: `bookmark_export` is marked destructive (it overwrites
  `out`) and idempotent for MCP.

Tests: a_fit_r_missing_a_value_copies_with_its_page_and_no_view (organize),
odd_bookmarks_round_trip_and_a_cut_tree_says_so (automation). Each fix
reverted alone fails one of them; the visit change is behaviour-neutral
and covered by the existing outline tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqmHYx8FiWM5PcsoiErgp9

crates/automation/src/bookmark_io.rs

@@ -3,7 +3,8 @@
 //! The portable form is JSON: `{"bookmarks": [node, …]}`, where a node is `title`, `page`
 //! (1-based, or `null` for one that goes nowhere), `view` (`{"type", "values"}`: a destination
 //! type and its values in PDF user space, as the file stores them), `color`, `bold`, `italic`,
-//! `open` and `children`. What export writes, import takes back unchanged.
+//! `open` and `children`. What export writes, import takes back unchanged. A file also carries
+//! `"truncated": true` when the export left out bookmarks nested past the listing limit.
 use crate::{Args, Automation, Result, ToolError, failed};
 use pdfcraft_engine::{DocId, Edit, OutlineNode};
 use serde_json::{Value, json};
@@ -19,7 +20,11 @@ impl Automation {
         let mut out = json!({ "bookmarks": tree.nodes.iter().map(node_json).collect::<Vec<_>>(), "truncated": tree.truncated });
         if let Some(file) = a.opt_str("out")? {
             let target = self.resolve(file, true)?;
-            let text = serde_json::to_vec_pretty(&json!({ "bookmarks": out["bookmarks"] })).map_err(failed)?;
+            let mut saved = json!({ "bookmarks": out["bookmarks"] });
+            if tree.truncated {
+                saved["truncated"] = json!(true);
+            }
+            let text = serde_json::to_vec_pretty(&saved).map_err(failed)?;
             crate::write_atomic(&target, &text)?;
             out["path"] = json!(target);
         }
@@ -27,16 +32,19 @@ impl Automation {
     }
 
     pub(crate) fn bookmark_import(&mut self, a: &Args) -> Result<Value> {
-        let nodes = match (a.get("bookmarks"), a.opt_str("file")?) {
-            (Some(list), None) => nodes_of(list)?,
+        let (nodes, truncated) = match (a.get("bookmarks"), a.opt_str("file")?) {
+            (Some(list), None) => (nodes_of(list)?, false),
             (None, Some(file)) => {
                 let path = self.resolve(file, false)?;
                 let value: Value = serde_json::from_slice(&crate::read_input(&path)?).map_err(|e| failed(format!("not a bookmark file: {e}")))?;
-                nodes_of(value.get("bookmarks").ok_or_else(|| failed("not a bookmark file: it has no \"bookmarks\" list"))?)?
+                let list = value.get("bookmarks").ok_or_else(|| failed("not a bookmark file: it has no \"bookmarks\" list"))?;
+                (nodes_of(list)?, value.get("truncated") == Some(&Value::Bool(true)))
             }
             _ => return Err(ToolError::InvalidArgs("give either bookmarks or file".into())),
         };
-        self.write_tree(a, nodes)
+        let mut out = self.write_tree(a, nodes)?;
+        out["truncated"] = json!(truncated);
+        Ok(out)
     }
 
     pub(crate) fn bookmark_copy(&mut self, a: &Args) -> Result<Value> {
@@ -44,12 +52,18 @@ impl Automation {
         let from = DocId(u64::try_from(from).map_err(|_| ToolError::InvalidArgs("from must be positive".into()))?);
         let path = a.opt_path("path")?.unwrap_or_default();
         let tree = self.outline_of(from, &path)?;
-        self.write_tree(a, tree.nodes)
+        let mut out = self.write_tree(a, tree.nodes)?;
+        out["truncated"] = json!(tree.truncated);
+        Ok(out)
     }
 
     fn outline_of(&self, id: DocId, path: &[usize]) -> Result<pdfcraft_engine::OutlineTree> {
         match self.session.outline_tree(id, path) {
-            Some(tree) => tree.map_err(failed),
+            // The engine names the path 0-based; the caller gave it 1-based.
+            Some(tree) => tree.map_err(|e| match e {
+                pdfcraft_organize::OutlineError::NoSuchBookmark(_) => failed(format!("there is no bookmark at {:?}", one_based(path))),
+                e => failed(e),
+            }),
             None => Err(failed(format!("document {} is not open, or its bookmarks can't be read for editing", id.0))),
         }
     }
@@ -121,7 +135,8 @@ fn list_of(list: &Value, at: &[usize], count: &mut usize) -> Result<Vec<OutlineN
 fn node_of(v: &Value, at: &[usize], count: &mut usize) -> Result<OutlineNode> {
     let o = v.as_object().ok_or_else(|| bad(at, "must be an object"))?;
     let field = |k: &str| o.get(k).filter(|x| !x.is_null());
-    let title = field("title").and_then(Value::as_str).filter(|t| !t.trim().is_empty()).ok_or_else(|| bad(at, "needs a title"))?.to_string();
+    // An empty title is kept: files have them, and export writes them as "".
+    let title = field("title").and_then(Value::as_str).ok_or_else(|| bad(at, "needs a title"))?.to_string();
     let page = match field("page") {
         None => None,
         Some(p) => Some(p.as_u64().filter(|p| *p >= 1).and_then(|p| usize::try_from(p - 1).ok()).ok_or_else(|| bad(at, "page must be 1 or more"))?),

crates/automation/src/mcp.rs

@@ -557,7 +557,7 @@ fn tool_json(t: &crate::ToolDef) -> Value {
         "title": t.title,
         "description": t.description,
         "inputSchema": t.input_schema,
-        "annotations": { "title": t.title, "readOnlyHint": t.read_only, "destructiveHint": t.destructive, "idempotentHint": t.read_only || matches!(t.name, "doc_save" | "doc_optimize" | "doc_export_data" | "measure_export" | "doc_export_images" | "doc_export_office" | "doc_export_all_images" | "doc_export_text" | "image_save"), "openWorldHint": false },
+        "annotations": { "title": t.title, "readOnlyHint": t.read_only, "destructiveHint": t.destructive, "idempotentHint": t.read_only || matches!(t.name, "doc_save" | "doc_optimize" | "doc_export_data" | "measure_export" | "doc_export_images" | "doc_export_office" | "doc_export_all_images" | "doc_export_text" | "image_save" | "bookmark_export"), "openWorldHint": false },
     })
 }
 

crates/automation/src/tools.rs

@@ -326,8 +326,9 @@ pub fn tools() -> Vec<ToolDef> {
         t(
             "bookmark_export",
             "Export bookmarks",
-            "The bookmark tree, or the subtree at path, as a list of nodes bookmark_import takes back unchanged. A node has title, page (1-based, null when it goes nowhere), open, and when set view ({type, values}: a destination type from ISO 32000-2 §12.3.2.2 Table 149 and its values in PDF user space, as the file stores them), color ([r, g, b], 0-1), bold, italic and children. truncated is true when bookmarks nested deeper than 33 levels were left out. With out, the tree is also written there as a JSON file {\"bookmarks\": [...]}.",
+            "The bookmark tree, or the subtree at path, as a list of nodes bookmark_import takes back unchanged. A node has title, page (1-based, null when it goes nowhere), open, and when set view ({type, values}: a destination type from ISO 32000-2 §12.3.2.2 Table 149 and its values in PDF user space, as the file stores them), color ([r, g, b], 0-1), bold, italic and children. truncated is true when bookmarks nested deeper than 33 levels were left out. With out, the tree is also written there as a JSON file {\"bookmarks\": [...]}, with \"truncated\": true when that is so; out is overwritten.",
         )
+        .destructive()
         .with(schema(
             json!({ "doc": doc(), "path": path("The bookmark whose subtree to export (omit for the whole outline)"), "out": path_arg() }),
             &["doc"],
@@ -335,7 +336,7 @@ pub fn tools() -> Vec<ToolDef> {
         t(
             "bookmark_import",
             "Import bookmarks",
-            "Add a tree of bookmarks, as bookmark_export gives it (bookmarks) or as a JSON file it wrote (file), under a parent bookmark (or at the top level) at a position, or with replace in place of the whole outline. A bookmark whose page this document lacks is kept without a destination and listed in unmapped (1-based paths into the given tree). A view takes exactly its type's values (XYZ 3, FitR 4, FitH/FitV/FitBH/FitBV 1, Fit/FitB none). Give exactly one of bookmarks and file. Every node is checked before anything is written: at most 100000 bookmarks, nested at most 33 levels counting the parent's. Undoable.",
+            "Add a tree of bookmarks, as bookmark_export gives it (bookmarks) or as a JSON file it wrote (file), under a parent bookmark (or at the top level) at a position, or with replace in place of the whole outline. A bookmark whose page this document lacks is kept without a destination and listed in unmapped (1-based paths into the given tree). A view takes exactly its type's values (XYZ 3, FitR 4, FitH/FitV/FitBH/FitBV 1, Fit/FitB none). Give exactly one of bookmarks and file. A title may be empty but not missing. Every node is checked before anything is written: at most 100000 bookmarks, nested at most 33 levels counting the parent's. truncated is true when the file says its export left bookmarks out. Undoable.",
         )
         .destructive()
         .with(schema(
@@ -352,7 +353,7 @@ pub fn tools() -> Vec<ToolDef> {
         t(
             "bookmark_copy",
             "Copy bookmarks from another document",
-            "Copy the bookmarks of document from (or the subtree at path) into doc, by page number: under a parent bookmark (or at the top level) at a position, or with replace in place of doc's outline. A bookmark whose page doc lacks is kept without a destination and listed in unmapped (1-based paths into the copied tree). Undoable.",
+            "Copy the bookmarks of document from (or the subtree at path) into doc, by page number: under a parent bookmark (or at the top level) at a position, or with replace in place of doc's outline. A bookmark whose page doc lacks is kept without a destination and listed in unmapped (1-based paths into the copied tree). truncated is true when bookmarks nested deeper than 33 levels were left out of the copy. Undoable.",
         )
         .destructive()
         .with(schema(

crates/automation/tests/it/automation.rs

@@ -10683,6 +10683,65 @@ fn bookmarks_export_import_and_copy_between_documents() {
     assert_eq!(ok(&mut a, "bookmark_list", json!({ "doc": same }))["bookmarks"][1]["children"][0]["title"], "Detail");
 }
 
+/// A one-page document whose outline has an untitled item, a /FitR destination with three of
+/// its four numbers, and a chain nested `deep` levels below them.
+fn odd_outline(deep: usize) -> Vec<u8> {
+    let first_deep = 7;
+    let mut objs = vec![
+        "<< /Type /Catalog /Pages 2 0 R /Outlines 4 0 R >>".to_string(),
+        "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 300] >>".into(),
+        "<< /Type /Page /Parent 2 0 R >>".into(),
+        "<< /Type /Outlines /First 5 0 R /Last 6 0 R >>".into(),
+        "<< /Title () /Parent 4 0 R /Next 6 0 R /Dest [3 0 R /Fit] >>".into(),
+        format!(
+            "<< /Title (Short) /Parent 4 0 R /Prev 5 0 R /Dest [3 0 R /FitR 0 0 10]{} >>",
+            if deep > 0 { format!(" /First {first_deep} 0 R /Last {first_deep} 0 R") } else { String::new() }
+        ),
+    ];
+    for i in 0..deep {
+        let (me, parent) = (first_deep + i, if i == 0 { 6 } else { first_deep + i - 1 });
+        let child = if i + 1 < deep { format!(" /First {0} 0 R /Last {0} 0 R", me + 1) } else { String::new() };
+        objs.push(format!("<< /Title (Level) /Parent {parent} 0 R /Dest [3 0 R /Fit]{child} >>"));
+    }
+    pdf_of(&objs.iter().map(String::as_str).collect::<Vec<_>>())
+}
+
+/// What a real file holds and the JSON form can't say exactly still copies and round-trips: an
+/// empty title stays empty, a short /FitR keeps its page and drops the view, and bookmarks past
+/// the listing depth are reported as left out at every step.
+#[test]
+fn odd_bookmarks_round_trip_and_a_cut_tree_says_so() {
+    let dir = workdir("bookmark-io-odd");
+    std::fs::write(dir.join("odd.pdf"), odd_outline(0)).unwrap();
+    std::fs::write(dir.join("deep.pdf"), odd_outline(40)).unwrap();
+    let mut a = auto(&dir);
+    let src = ok(&mut a, "doc_open", json!({ "path": "odd.pdf" }))["doc"].as_u64().unwrap();
+    let tree = ok(&mut a, "bookmark_export", json!({ "doc": src, "out": "odd.json" }))["bookmarks"].clone();
+    assert_eq!((tree[0]["title"].clone(), tree[1]["page"].clone(), tree[1].get("view")), (json!(""), json!(1), None));
+    let dst = ok(&mut a, "doc_open", json!({ "path": "b.pdf" }))["doc"].as_u64().unwrap();
+    assert_eq!(ok(&mut a, "bookmark_import", json!({ "doc": dst, "file": "odd.json", "replace": true }))["truncated"], false);
+    // A destination with no view is written as XYZ with nulls: the same page, the reader's view.
+    let back = ok(&mut a, "bookmark_export", json!({ "doc": dst }))["bookmarks"].clone();
+    assert_eq!((back[0].clone(), back[1]["title"].clone(), back[1]["page"].clone()), (tree[0].clone(), json!("Short"), json!(1)));
+    assert_eq!(back[1]["view"], json!({ "type": "XYZ", "values": [null, null, null] }));
+    let copied = ok(&mut a, "bookmark_copy", json!({ "doc": dst, "from": src, "replace": true }));
+    assert_eq!((copied["unmapped"].clone(), copied["truncated"].clone()), (json!([]), json!(false)));
+    match a.call("bookmark_export", &json!({ "doc": src, "path": [3] })) {
+        Err(ToolError::Failed(m)) => assert!(m.contains("no bookmark at [3]"), "{m}"),
+        other => panic!("expected no bookmark, got {other:?}"),
+    }
+
+    let deep = ok(&mut a, "doc_open", json!({ "path": "deep.pdf" }))["doc"].as_u64().unwrap();
+    assert_eq!(ok(&mut a, "bookmark_export", json!({ "doc": deep, "out": "deep.json" }))["truncated"], true);
+    let saved: Value = serde_json::from_slice(&std::fs::read(dir.join("deep.json")).unwrap()).unwrap();
+    assert_eq!(saved["truncated"], true);
+    assert_eq!(ok(&mut a, "bookmark_import", json!({ "doc": dst, "file": "deep.json", "replace": true }))["truncated"], true);
+    assert_eq!(ok(&mut a, "bookmark_copy", json!({ "doc": dst, "from": deep, "replace": true }))["truncated"], true);
+    let export = tools().into_iter().find(|t| t.name == "bookmark_export").unwrap();
+    assert!(export.destructive && !export.read_only, "bookmark_export overwrites out");
+    std::fs::remove_dir_all(dir).unwrap();
+}
+
 #[test]
 fn a_bad_bookmark_tree_is_refused_whole_and_names_the_node() {
     let dir = workdir("bookmark-io-bad");
@@ -10695,7 +10754,7 @@ fn a_bad_bookmark_tree_is_refused_whole_and_names_the_node() {
     let fine = json!({ "title": "Fine", "page": 1 });
     let m = refused(&mut a, json!([fine, { "title": "Parent", "children": [{ "page": 1 }] }]));
     assert!(m.contains("[2, 1]") && m.contains("title"), "{m}");
-    let m = refused(&mut a, json!([{ "title": "  " }]));
+    let m = refused(&mut a, json!([{ "title": 5 }]));
     assert!(m.contains("[1]") && m.contains("title"), "{m}");
     let m = refused(&mut a, json!([fine, { "title": "Bad", "page": 0 }]));
     assert!(m.contains("[2]") && m.contains("page"), "{m}");

crates/organize/src/outline_io.rs

@@ -90,10 +90,14 @@ fn has_children(doc: &Document, r: ObjRef) -> bool {
 }
 
 /// A destination's view as [`import_outline`] takes it: a known type with exactly its number of
-/// values (extra ones dropped, missing ones `None`). An unknown type gives `None`.
+/// values (extra ones dropped, missing ones `None`). An unknown type, or a FitR missing one of
+/// its four values (the rectangle can't be kept), gives `None`.
 fn portable_view(kind: Vec<u8>, mut values: Vec<Option<f64>>) -> Option<(Vec<u8>, Vec<Option<f64>>)> {
     let (_, arity) = VIEWS.iter().find(|(k, _)| *k == kind.as_slice())?;
     values.resize(*arity, None);
+    if kind.as_slice() == b"FitR" && values.iter().any(Option::is_none) {
+        return None;
+    }
     Some((kind, values))
 }
 
@@ -119,14 +123,18 @@ pub fn view_arity(kind: &[u8]) -> Option<usize> {
 fn visit<'a>(nodes: &'a [OutlineNode], mut f: impl FnMut(&[usize], &'a OutlineNode)) {
     // One entry per open level: its nodes and how many of them have been visited.
     let mut stack: Vec<(&'a [OutlineNode], usize)> = vec![(nodes, 0)];
+    // The visited node's path, kept in step with the stack rather than rebuilt per node.
+    let mut path: Vec<usize> = Vec::new();
     while let Some(level) = stack.last_mut() {
         let list = level.0;
-        let Some(node) = list.get(level.1) else {
+        let index = level.1;
+        let Some(node) = list.get(index) else {
             stack.pop();
             continue;
         };
-        level.1 = level.1.saturating_add(1);
-        let path: Vec<usize> = stack.iter().map(|(_, visited)| visited.saturating_sub(1)).collect();
+        level.1 = index.saturating_add(1);
+        path.truncate(stack.len().saturating_sub(1));
+        path.push(index);
         f(&path, node);
         if !node.children.is_empty() {
             stack.push((&node.children, 0));

crates/organize/src/tests.rs

@@ -3904,6 +3904,21 @@ fn a_page_the_target_lacks_keeps_its_bookmark_without_a_destination() {
     assert!(target.get(listed[2].obj).as_dict().unwrap().get(b"Dest").is_none());
 }
 
+#[test]
+fn a_fit_r_missing_a_value_copies_with_its_page_and_no_view() {
+    let mut src = outline_source();
+    let pages = crate::walk(&src).unwrap();
+    let figure = crate::bookmarks(&src)[2].obj;
+    let short = vec![Object::Ref(pages[2].0), Object::name("FitR"), Object::Int(10), Object::Int(20), Object::Int(110)];
+    src.update_dict(figure, |x| x.set(b"Dest".to_vec(), Object::Array(short))).unwrap();
+    let tree = crate::outline_tree(&src, &[]).unwrap();
+    let copied = &tree.nodes[0].children[1];
+    assert_eq!((copied.page, copied.view.clone()), (Some(2), None));
+    let mut target = doc_a();
+    crate::import_outline(&mut target, &[], 0, &tree.nodes, false).unwrap();
+    assert_eq!(titles(&crate::bookmarks(&target)), ["Intro[Part,Figure]", "Results"]);
+}
+
 #[test]
 fn an_import_appends_inserts_or_replaces() {
     let mut d = doc_a();