diff --git a/crates/engine/src/datafill.rs b/crates/engine/src/datafill.rs index 6b8376ef..b93e5f59 100644 --- a/crates/engine/src/datafill.rs +++ b/crates/engine/src/datafill.rs @@ -8,7 +8,7 @@ use serde_json::{Value, json}; use crate::cmd::CommandSpec; use crate::params::{get, id, id_of, num_or, string}; -use crate::{Result, Session, bad, spec}; +use crate::{EngineError, Result, Session, bad, spec}; const CMD: &str = "data.fill"; const MAX_ROWS: usize = 500; @@ -31,6 +31,10 @@ fn file_name(name: &str) -> String { } fn fill(s: &mut Session, p: &Value) -> Result { + // Refused before any copy or file is made: a rollback afterwards wouldn't take files back. + if let Some(why) = s.live_read_only(s.ds()?.uid) { + return Err(EngineError::Disabled(CMD.into(), why.into())); + } let src = id(p, "id").ok_or_else(|| bad(CMD, "`id` (the frame to repeat) required"))?; let rows = match get(p, "rows") { Some(Value::Array(a)) if a.iter().all(Value::is_object) && a.len() <= MAX_ROWS => a.clone(), diff --git a/crates/engine/src/live_access_tests.rs b/crates/engine/src/live_access_tests.rs index c2c75bb5..52a08cfd 100644 --- a/crates/engine/src/live_access_tests.rs +++ b/crates/engine/src/live_access_tests.rs @@ -105,6 +105,13 @@ fn a_view_link_makes_the_file_read_only() { refused(&mut s, "shape.rectangle", json!({"x": 0, "y": 0, "width": 5, "height": 5}), VIEW_ONLY); refused(&mut s, "node.set", json!({"ids": [card.0], "props": {"name": "Mine"}}), VIEW_ONLY); refused(&mut s, "edit.delete", json!({"ids": [card.0]}), VIEW_ONLY); + // Refused before it makes copies or writes files, which no rollback takes back. + let dir = std::env::temp_dir().join(format!("figcraft-view-fill-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + refused(&mut s, "data.fill", json!({"id": card.0, "rows": [{"name": "Copy"}], "dir": dir.to_string_lossy()}), VIEW_ONLY); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0, "no file was written"); + let _ = std::fs::remove_dir_all(&dir); refused(&mut s, "tool.set", json!({"tool": "text"}), VIEW_ONLY); assert!(matches!(s.begin_interaction("Move"), Err(EngineError::Disabled(..)))); assert_eq!(*s.doc().unwrap(), before); diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 8cceb910..dbfb1908 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -754,7 +754,10 @@ fn get_node(b: &mut dyn Backend, a: &Map) -> Result bool { if fits(value, rest.len()) { return rest.len() != all.len(); } - // The largest window that fits, but at least one child, so paging moves on past a child too - // large to fit (the marker then says so). - let (mut lo, mut hi) = (usize::from(!rest.is_empty()), rest.len()); + // The largest window that fits, between none and all of the rest. + let (mut lo, mut hi) = (0usize, rest.len()); while lo < hi { let mid = lo + (hi - lo).div_ceil(2); if fits(value, mid) { lo = mid } else { hi = mid - 1 } diff --git a/crates/mcp/tests/it/inspect_response_limit.rs b/crates/mcp/tests/it/inspect_response_limit.rs index d3bd70a0..45aa2729 100644 --- a/crates/mcp/tests/it/inspect_response_limit.rs +++ b/crates/mcp/tests/it/inspect_response_limit.rs @@ -153,8 +153,8 @@ fn get_node(c: &mut Client, args: Value) -> (Value, usize) { (serde_json::from_str(text).expect("the block is JSON"), wire) } -/// A child too large for the limit on its own is still shown, one per page, so paging moves on, -/// and the marker says the answer was cut. +/// A child too large for the limit on its own is passed over, one per page of just the marker, +/// so paging moves on and every page stays within the limit. #[test] fn children_too_large_to_fit_page_one_at_a_time() { let mut c = Client::new(); @@ -165,8 +165,9 @@ fn children_too_large_to_fit_page_one_at_a_time() { let mut offset = Some(0u64); let mut pages = 0; while let Some(at) = offset { - let (page, _) = get_node(&mut c, json!({"id": frame, "depth": 1, "maxBytes": 1024, "offset": at})); - assert_eq!(page["children"].as_array().map(Vec::len), Some(1), "{}", page["truncated"]); + let (page, wire) = get_node(&mut c, json!({"id": frame, "depth": 1, "maxBytes": 2500, "offset": at})); + assert!(wire <= 2500, "page at {at} exceeds maxBytes: {wire}"); + assert_eq!(page["children"].as_array().map_or(0, Vec::len), 0, "{}", page["truncated"]); assert!(!page["truncated"].is_null(), "the marker stays: page at {at}"); offset = page["truncated"]["children"]["nextOffset"].as_u64(); assert!(offset.is_none_or(|o| o > at), "paging moves on from {at}");