From 0ca7ed07fa56300677c85c10f672825f6c5c06bc Mon Sep 17 00:00:00 2001 From: hcchung1 <131962610+hcchung1@users.noreply.github.com> Date: Wed, 21 Oct 2026 00:22:07 +0800 Subject: [PATCH] fix(automation): a page whose paragraph listing was dropped at the cap is refused, not read by position (#2516) The listings kept for paragraph numbers are capped at 256 pages, and the 257th cleared them all. A page that lost its listing fell back to reading a number as a position in the page's current paragraphs, so after an edit regrouped it a number could rewrite another paragraph. Documents that lose a listing to the cap are remembered, and a page of one with no listing is refused with a message to list it again. doc_close drops a document's listings and that mark. Co-Authored-By: Claude Sonnet 5.5 --- crates/automation/src/lib.rs | 27 ++++++++++++++++++++--- crates/automation/tests/automation.rs | 31 +++++++++++++++++++++++++++ docs/editing-existing-text.md | 4 +++- 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index e33b3242e5..753ca1a68c 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -32,7 +32,7 @@ mod signing; mod text_cache; mod tools; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -121,6 +121,9 @@ pub struct Automation { /// The paragraphs of the last `text_paragraphs` listing of each page, so a paragraph number /// keeps naming that paragraph after edits regroup the page (#2516). listed: HashMap<(DocId, usize), Vec>, + /// Documents that lost a listing to the cap. A page of one with no listing is refused rather + /// than read as "a number is a position", which the page may no longer match. + evicted: HashSet, } impl Default for Automation { @@ -131,7 +134,15 @@ impl Default for Automation { impl Automation { pub fn new() -> Self { - Self { session: Session::new(), root: None, roots: None, renderers: HashMap::new(), texts: TextCache::default(), listed: HashMap::new() } + Self { + session: Session::new(), + root: None, + roots: None, + renderers: HashMap::new(), + texts: TextCache::default(), + listed: HashMap::new(), + evicted: HashSet::new(), + } } /// Confine every path the tools read or write to `root` (relative paths resolve inside it). @@ -908,6 +919,7 @@ impl Automation { /// resolved against that listing. fn record_listing(&mut self, doc: DocId, page: usize, blocks: &[TextBlock]) { if self.listed.len() >= Self::MAX_LISTINGS && !self.listed.contains_key(&(doc, page)) { + self.evicted.extend(self.listed.keys().map(|(d, _)| *d)); self.listed.clear(); } self.listed.insert((doc, page), blocks.iter().map(BlockKey::of).collect()); @@ -918,9 +930,16 @@ impl Automation { /// After `text_paragraphs` a number names the paragraph that listing showed, wherever it is /// now. Without a listing it is a position in `blocks`. A paragraph an edit merged into a /// neighbour (or changed) is refused: the number would otherwise rewrite another paragraph - /// and lose its text (#2516). + /// and lose its text (#2516). So is any number on a page of a document whose listings were + /// dropped at the cap, until the page is listed again. fn paragraph_index(&self, doc: DocId, page: usize, k: i64, blocks: &[TextBlock]) -> Result { let keys = self.listed.get(&(doc, page)); + if keys.is_none() && self.evicted.contains(&doc) { + return Err(ToolError::InvalidArgs(format!( + "page {} has no paragraph listing any more: too many pages were listed and the oldest listings were dropped. List the page again with text_paragraphs", + page + 1 + ))); + } let n = keys.map_or(blocks.len(), Vec::len); let i = usize::try_from(k) .ok() @@ -972,6 +991,8 @@ impl Automation { self.session.close(id); self.renderers.remove(&id); self.texts.remove(&id); + self.listed.retain(|(d, _), _| *d != id); + self.evicted.remove(&id); Ok(json!({ "closed": id.0 })) } diff --git a/crates/automation/tests/automation.rs b/crates/automation/tests/automation.rs index ef16e77d40..fe638d63f5 100644 --- a/crates/automation/tests/automation.rs +++ b/crates/automation/tests/automation.rs @@ -6991,6 +6991,37 @@ fn page_render_hide_text_block_refuses_a_paragraph_number_an_earlier_edit_regrou std::fs::remove_dir_all(dir).unwrap(); } +/// The listings kept for paragraph numbers are capped. A page whose listing was dropped must not +/// fall back to reading its numbers as positions, which an edit may have regrouped: it is refused +/// until the page is listed again (#2516). +#[test] +fn text_edit_refuses_a_paragraph_number_whose_listing_was_dropped_at_the_cap() { + let dir = workdir("text-edit-evicted-listing"); + std::fs::write(dir.join("p.pdf"), three_paragraph_pdf()).unwrap(); + let mut a = auto(&dir); + let open = |a: &mut Automation| ok(a, "doc_open", json!({"path":"p.pdf"}))["doc"].as_u64().unwrap(); + let doc = open(&mut a); + ok(&mut a, "text_paragraphs", json!({"doc":doc,"page":1})); + // 256 more pages fill the cap, so the next new page drops every earlier listing. + let others: Vec = (0..256).map(|_| open(&mut a)).collect(); + for &other in &others { + ok(&mut a, "text_paragraphs", json!({"doc":other,"page":1})); + } + + let dropped = a.call("text_edit", &json!({"doc":doc,"page":1,"paragraph":2,"text":"Bravo paragraph, edited."})); + assert!(matches!(&dropped, Err(ToolError::InvalidArgs(m)) if m.contains("text_paragraphs")), "{dropped:?}"); + assert!(!page_text(&mut a, doc).join(" ").contains("Bravo paragraph, edited."), "nothing was rewritten"); + let hidden = a.call("page_render", &json!({"doc":doc,"page":1,"dpi":72,"hide_text_block":2})); + assert!(matches!(&hidden, Err(ToolError::InvalidArgs(m)) if m.contains("text_paragraphs")), "{:?}", hidden.map(|_| ())); + + // A page listed after the drop keeps its numbers, and so does the page once it is listed again. + ok(&mut a, "text_edit", json!({"doc":others[255],"page":1,"paragraph":2,"text":"Bravo paragraph, edited."})); + ok(&mut a, "text_paragraphs", json!({"doc":doc,"page":1})); + ok(&mut a, "text_edit", json!({"doc":doc,"page":1,"paragraph":2,"text":"Bravo paragraph, edited."})); + assert!(page_text(&mut a, doc).join(" ").contains("Bravo paragraph, edited.")); + std::fs::remove_dir_all(dir).unwrap(); +} + /// A number from the listing keeps naming its paragraph while other paragraphs are edited, and /// a paragraph can be edited twice in a row by the same number. #[test] diff --git a/docs/editing-existing-text.md b/docs/editing-existing-text.md index b1f810b717..e2a89fa8ea 100644 --- a/docs/editing-existing-text.md +++ b/docs/editing-existing-text.md @@ -105,6 +105,8 @@ two the listing showed. A paragraph an edit merged into a neighbour (or changed) itself when the edit merged it into one, with a message to list the page again, instead of rewriting whichever paragraph took its number (#2516). The result of an edit names the numbers that are gone in `stale_paragraphs`. Listing -a page again renumbers it. Without a listing, a number is a position in the page's current +a page again renumbers it. The listings kept are capped (256 pages); once the cap drops +them, a page of an affected document is refused until it is listed again, instead of reading its +numbers as positions. Without a listing, a number is a position in the page's current paragraphs, as before. The inline editor holds the paragraph a switching click asked for the same way. What is still missing is an identity that survives save and reopen (design: #2508).