From fc2739a8cdeeaa0f0fd5153a6108da5ab2db3605 Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Fri, 9 Oct 2026 23:13:29 -0600 Subject: [PATCH] #61: honor MCP typing, story and page contracts (#118) --- crates/engine/src/cmd/text.rs | 18 ++++- crates/mcp/src/headless.rs | 18 ++++- crates/mcp/src/tests.rs | 84 +++++++++++++++++++++++ crates/mcp/src/tools.rs | 19 ++++-- crates/ui-egui/src/control.rs | 125 ++++++++++++++++++++++++++++++++-- crates/ui-egui/src/menus.rs | 2 +- 6 files changed, 250 insertions(+), 16 deletions(-) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index a75d6d4..552ce51 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -114,8 +114,8 @@ pub fn specs() -> Vec { cmd!(query "story.get", "Get Story", [], None, "{story? | frame?} → text, frames, paragraphs, vertical, overset", has_doc, |s, p| { let st = s.doc()?; let sid = story_of(s, p).ok_or_else(|| bad("story.get", "no story"))?; + let mut v = st.doc.story_summary(sid).ok_or(designcraft_doc::DocError::NoStory(sid))?; let cs = s.cache.get(&st.doc, sid, None); - let mut v = st.doc.story_summary(sid).unwrap_or_default(); v["overset"] = json!(cs.overset_at); v["lines"] = json!(cs.line_count()); Ok(v) @@ -1638,3 +1638,19 @@ mod vertical_caret_tests { assert_eq!(mv(&mut s, "up", false, false), 3 * c); } } + +#[cfg(test)] +mod story_query_contract_tests { + use serde_json::json; + #[test] + fn missing_story_errors_but_valid_empty_and_selected_stories_succeed() { + let mut s = crate::Session::new(); + s.execute("file.new", &json!({})).unwrap(); + assert!(s.execute("story.get", &json!({"story":999})).unwrap_err().to_string().contains("no such story")); + let f = s.execute("frame.create", &json!({"rect":[36,36,300,200],"content":"text"})).unwrap(); + assert_eq!(s.execute("story.get", &json!({"story":f["story"]})).unwrap()["text"], ""); + assert_eq!(s.execute("story.get", &json!({"frame":f["id"]})).unwrap()["text"], ""); + s.execute("text.select", &json!({"story":f["story"],"anchor":0})).unwrap(); + assert_eq!(s.execute("story.get", &json!({})).unwrap()["text"], ""); + } +} diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index ee9d11d..aea3e87 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -180,7 +180,7 @@ impl Headless { fn text(&mut self, p: &Value) -> Result { let t = s(p, "text").ok_or("missing `text`")?; - if !self.session.wants_text() { + if self.session.active().is_none_or(|st| st.selection.text.is_none_or(|t| st.doc.text_story(t.story, t.cell).is_none())) { return Err("no text insertion point: click into a text frame with the type tool first (pointer with tool \"type\"), \ or select text with execute text.select, or use set_story_text" .into()); @@ -191,7 +191,7 @@ impl Headless { fn render(&mut self, p: &Value) -> Result { let st = self.session.active().ok_or("no document open")?; - let page = p.get("page").and_then(Value::as_u64).unwrap_or(0) as usize; + let page = page_index(p, st.doc.page_count())?; let scale = p.get("scale").and_then(Value::as_f64).unwrap_or(1.0).clamp(0.05, 16.0); let bleed = p.get("bleed").and_then(Value::as_bool).unwrap_or(false); let img = self @@ -213,7 +213,7 @@ impl Headless { fn export(&mut self, p: &Value) -> Result { let path = s(p, "path").ok_or("missing `path`")?; let st = self.session.active().ok_or("no document open")?; - let page = p.get("page").and_then(Value::as_u64).unwrap_or(0) as usize; + let page = page_index(p, st.doc.page_count())?; let scale = p.get("scale").and_then(Value::as_f64).unwrap_or(2.0).clamp(0.05, 16.0); let img = self .renderer @@ -263,3 +263,15 @@ impl Backend for Headless { "headless (in-process engine, no window)".into() } } + +/// A supplied page must be a representable unsigned index; only omission selects page zero. +fn page_index(p: &Value, count: usize) -> Result { + let page = match p.get("page") { + None => 0, + Some(v) => v.as_u64().and_then(|n| usize::try_from(n).ok()).ok_or("`page` must be a nonnegative integer representable as a page index")?, + }; + if page >= count { + return Err(format!("no page {page} (the document has {count} pages, indices are 0-based)")); + } + Ok(page) +} diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 2804d13..3051a5a 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -416,3 +416,87 @@ fn batch_steps_use_earlier_results() { ); assert_eq!(v["completed"], 2); } + +#[test] +fn explicit_typing_honors_caret_range_and_undo_without_changing_shortcuts() { + let mut s = server(); + assert_eq!(call(&mut s, "type_text", json!({"text":"X"}))["isError"], true); + let f = ok(&mut s, "execute", json!({"command":"frame.create","params":{"rect":[36,36,300,200],"content":"text","text":"Hello"}})); + let story = f["story"].as_u64().unwrap(); + for (anchor, focus, expected) in [(5, 5, "HelloX"), (0, 5, "X")] { + ok(&mut s, "set_story_text", json!({"story":story,"text":"Hello"})); + ok(&mut s, "execute", json!({"command":"text.select","params":{"story":story,"anchor":anchor,"focus":focus}})); + ok(&mut s, "type_text", json!({"text":"X"})); + assert_eq!(ok(&mut s, "get_story", json!({"story":story}))["text"], expected); + ok(&mut s, "execute", json!({"command":"edit.undo"})); + assert_eq!(ok(&mut s, "get_story", json!({"story":story}))["text"], "Hello"); + } + ok(&mut s, "execute", json!({"command":"text.placeCaret","params":{"frame":f["id"],"point":[100,50]}})); + ok(&mut s, "type_text", json!({"text":"X"})); + assert_eq!(ok(&mut s, "get_story", json!({"story":story}))["text"], "HelloX"); + assert_eq!(ok(&mut s, "key", json!({"key":"T"}))["handledBy"], "tool.select"); + assert_eq!(ok(&mut s, "get_story", json!({"story":story}))["text"], "HelloX"); +} + +#[test] +fn story_queries_distinguish_missing_from_empty() { + let mut s = server(); + assert_eq!(call(&mut s, "get_story", json!({"story":999}))["isError"], true); + let f = ok(&mut s, "execute", json!({"command":"frame.create","params":{"rect":[36,36,300,200],"content":"text"}})); + assert_eq!(ok(&mut s, "get_story", json!({"story":f["story"]}))["text"], ""); + assert_eq!(ok(&mut s, "get_story", json!({"frame":f["id"]}))["text"], ""); + ok(&mut s, "execute", json!({"command":"text.select","params":{"story":f["story"],"anchor":0}})); + assert_eq!(ok(&mut s, "get_story", json!({}))["text"], ""); +} + +#[test] +fn page_contract_rejects_supplied_invalid_values_without_writes() { + let mut s = server(); + ok(&mut s, "new_document", json!({"sample":true})); + let path = tmp("page-contract.png"); + for tool in ["export_png", "render_page"] { + for page in + [json!(-1), json!(-2), json!(1.5), Value::Null, json!("0"), json!(true), json!(u64::MAX), json!(18446744073709551616_f64), json!(99)] + { + std::fs::write(&path, b"KEEP").unwrap(); + let args = json!({"page":page,"path":path,"scale":0.1}); + assert_eq!(call(&mut s, tool, args)["isError"], true, "{tool}: {page}"); + assert_eq!(std::fs::read(&path).unwrap(), b"KEEP"); + } + } + let first = image_of(&call(&mut s, "render_page", json!({"page":0,"scale":0.1}))); + let next = image_of(&call(&mut s, "render_page", json!({"page":1,"scale":0.1}))); + assert_ne!(first, next); + assert_eq!(first, image_of(&call(&mut s, "render_page", json!({"scale":0.1})))); + ok(&mut s, "export_png", json!({"page":1,"path":path,"scale":0.1})); + assert_eq!(std::fs::read(&path).unwrap(), next); + ok(&mut s, "export_png", json!({"path":path,"scale":0.1})); + assert_eq!(std::fs::read(&path).unwrap(), first); + for tool in ["render_page", "export_png"] { + let definition = tool_definitions().into_iter().find(|v| v["name"] == tool).unwrap(); + assert_eq!(definition["inputSchema"]["properties"]["page"]["minimum"], 0); + } + let mut b = Headless::with_document(); + for method in ["ui.render", "app.export"] { + for page in [json!(-1), Value::Null, json!("0"), json!(99)] { + std::fs::write(&path, b"KEEP").unwrap(); + assert!(b.call(method, json!({"page":page,"path":path,"scale":0.1})).is_err()); + assert_eq!(std::fs::read(&path).unwrap(), b"KEEP"); + } + std::fs::remove_file(&path).unwrap(); + assert!(b.call(method, json!({"page":-1,"path":path,"scale":0.1})).is_err()); + assert!(!path.exists()); + } +} + +#[test] +fn explicit_typing_rejects_stale_selection_and_closed_document() { + let mut b = Headless::with_document(); + let f = b.session.execute("frame.create", &json!({"rect":[36,36,300,200],"content":"text","text":"Hello"})).unwrap(); + b.session.execute("text.select", &json!({"story":f["story"],"anchor":5})).unwrap(); + b.session.active_mut().unwrap().selection.text.as_mut().unwrap().story.0 = 999; + assert!(b.call("ui.text", json!({"text":"X"})).is_err()); + assert_eq!(b.session.execute("story.get", &json!({"story":f["story"]})).unwrap()["text"], "Hello"); + b.session.execute("file.close", &json!({})).unwrap(); + assert!(b.call("ui.text", json!({"text":"X"})).is_err()); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index dc2604c..0ad9962 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -223,7 +223,7 @@ pub fn tool_definitions() -> Vec { also write it to `path`.", obj( json!({ - "page": int("Page index, 0-based (default 0; in the desktop app default = the current page)"), + "page": {"type": "integer", "minimum": 0, "description": "Page index, 0-based (default 0; in the desktop app default = the current page)"}, "scale": num("Pixels per point (default 1 = 72 ppi)"), "bleed": boolean("Include the bleed area"), "path": string("Also write the PNG here"), @@ -237,7 +237,7 @@ pub fn tool_definitions() -> Vec { "Export PNG", "Export one page as a PNG (or JPEG if the path ends in .jpg) file. Returns the path and pixel size.", obj( - json!({"path": string("Destination file"), "page": int("Page index, 0-based (default 0)"), "scale": num("Pixels per point (default 2)")}), + json!({"path": string("Destination file"), "page": {"type": "integer", "minimum": 0, "description": "Page index, 0-based (default 0; in the desktop app default = the current page)"}, "scale": num("Pixels per point (default 2)")}), &["path"], ), false, @@ -303,7 +303,7 @@ pub fn tool_definitions() -> Vec { tool( "type_text", "Type text", - "Type text at the text insertion point (as the keyboard would with the type tool), or into the focused dialog field in \ + "Type text at the text insertion point, independently of the active tool, or into the focused dialog field in \ the desktop app. Put a caret first with pointer (tool \"type\") or execute text.placeCaret / text.select.", obj(json!({"text": string("Text to type (\\n = new paragraph)")}), &["text"]), false, @@ -409,6 +409,15 @@ fn pick(a: &Args, keys: &[&str]) -> Value { Value::Object(keys.iter().filter_map(|k| a.get(*k).filter(|v| !v.is_null()).map(|v| (k.to_string(), v.clone()))).collect()) } +/// Page omission selects a default, but an explicitly supplied null must reach validation. +fn page_params(a: &Args, keys: &[&str]) -> Value { + let mut p = pick(a, keys); + if let Some(page) = a.get("page") { + p["page"] = page.clone(); + } + p +} + fn command_params(v: Option<&Value>) -> Result { match v { None | Some(Value::Null) => Ok(json!({})), @@ -463,7 +472,7 @@ fn story_id(b: &mut dyn Backend, a: &Args) -> Result { } fn render_page(b: &mut dyn Backend, a: &Args) -> Result { - let r = b.call("ui.render", pick(a, &["page", "scale", "bleed"]))?; + let r = b.call("ui.render", page_params(a, &["page", "scale", "bleed"]))?; let b64 = r.get("pngBase64").and_then(Value::as_str).ok_or("renderer returned no image")?.to_string(); let path = a.get("path").and_then(Value::as_str); if let Some(path) = path { @@ -550,7 +559,7 @@ fn dispatch(b: &mut dyn Backend, name: &str, a: &Args) -> Result render_page(b, a), "export_png" => { req_str(a, "path")?; - j(b.call("app.export", pick(a, &["path", "page", "scale"]))?) + j(b.call("app.export", page_params(a, &["path", "page", "scale"]))?) } "screenshot" => screenshot(b, a), "select_tool" => j(b.call("ui.tool.select", json!({"tool": req_str(a, "tool")?}))?), diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 3245025..e81e4c1 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -8,7 +8,8 @@ //! - `ui.tool.select {tool}`, `ui.tool.list` //! - `ui.pointer {events:[{kind: down|drag|up|move|doubleclick, x, y, space?: "doc"|"screen"}], mods?}`: //! drive the active tool through the same path as the mouse -//! - `ui.key {key, shift?, alt?, cmd?}` / `ui.text {text}`: synthetic keyboard input +//! - `ui.key {key, shift?, alt?, cmd?}`: synthetic keyboard input +//! - `ui.text {text}`: insert at the explicit caret, or type into a focused UI field //! - `ui.move {x, y}` / `ui.click {x, y, button?, count?, shift?…}` / `ui.drag {x, y, toX, toY, steps?}`: //! real egui pointer input in screen points (reaches every widget: panels, flyouts, dialogs) //! - `ui.set {brightness?, panel?, dockTab?, rulers?, outline?, …}` @@ -216,9 +217,19 @@ pub fn handle(app: &mut DesignApp, ctx: &egui::Context, req: &ControlRequest) -> ok(Value::Null) } "ui.text" => { - app.synthetic.push(egui::Event::Text(s("text").unwrap_or("").to_string())); - ctx.request_repaint(); - ok(Value::Null) + let Some(text) = s("text") else { return err("missing `text`") }; + // Focused widgets retain the synthetic keyboard route. Document automation uses + // the explicit caret independently of the active tool, unlike ordinary typing. + if ctx.text_edit_focused() || app.ui.dialog.is_some() || app.ui.palette.is_some() { + app.synthetic.push(egui::Event::Text(text.to_string())); + ctx.request_repaint(); + ok(Value::Null) + } else { + if app.session.active().is_none_or(|st| st.selection.text.is_none_or(|t| st.doc.text_story(t.story, t.cell).is_none())) { + return err("no valid text insertion point: use text.select or text.placeCaret first"); + } + wrap(app.run("text.insert", json!({"text": text}))) + } } "ui.set" => { let mut r = Ok(Value::Null); @@ -313,7 +324,10 @@ pub fn handle(app: &mut DesignApp, ctx: &egui::Context, req: &ControlRequest) -> } "ui.render" => { let Some(st) = app.session.active() else { return err("no document") }; - let page = p.get("page").and_then(Value::as_u64).map(|v| v as usize).or_else(|| crate::canvas::current_page(app)).unwrap_or(0); + let page = match page_index(p, crate::canvas::current_page(app).unwrap_or(0), st.doc.page_count()) { + Ok(page) => page, + Err(e) => return err(e), + }; let scale = p.get("scale").and_then(Value::as_f64).unwrap_or(1.0); let Some(img) = app.canvas.renderer.render_page( &st.doc, @@ -336,7 +350,7 @@ pub fn handle(app: &mut DesignApp, ctx: &egui::Context, req: &ControlRequest) -> } "app.open" => wrap(app.run("file.open", json!({"path": s("path")}))), "app.save" => wrap(app.run("file.save", json!({"path": s("path")}))), - "app.export" => wrap(app.run("app.exportPng", json!({"path": s("path"), "page": p.get("page"), "scale": p.get("scale")}))), + "app.export" => wrap(app.run("app.exportPng", p.clone())), "app.quit" => { ctx.send_viewport_cmd(egui::ViewportCommand::Close); ok(Value::Null) @@ -361,3 +375,102 @@ pub fn save_screenshot(app: &mut DesignApp, image: &egui::ColorImage, path: Opti None => json!({"ok": false, "error": "no writer configured"}), } } + +/// Parse explicit page indices without treating invalid values as omission. +pub(crate) fn page_index(p: &Value, default: usize, count: usize) -> Result { + let page = match p.get("page") { + None => default, + Some(v) => v.as_u64().and_then(|n| usize::try_from(n).ok()).ok_or("`page` must be a nonnegative integer representable as a page index")?, + }; + if page >= count { + return Err(format!("no page {page} (the document has {count} pages, indices are 0-based)")); + } + Ok(page) +} + +#[cfg(test)] +mod automation_contract_tests { + use super::*; + fn app() -> DesignApp { + let mut app = DesignApp::new(designcraft_engine::Session::new(), crate::Services::default()); + app.session.execute("file.new", &json!({"pages":2})).unwrap(); + app + } + fn call(app: &mut DesignApp, ctx: &egui::Context, method: &str, params: Value) -> Value { + let (req, _) = ControlRequest::new(method, params); + match handle(app, ctx, &req) { + Outcome::Done(v) => v, + Outcome::Screenshot { .. } => panic!("unexpected screenshot"), + } + } + fn frame(app: &mut DesignApp, ctx: &egui::Context, events: Vec) { + let mut output = ctx.run_ui( + egui::RawInput { + screen_rect: Some(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(1400.0, 900.0))), + events, + ..Default::default() + }, + |ui| { + app.logic(ui.ctx()); + app.ui(ui); + }, + ); + output.textures_delta.clear(); + } + #[test] + fn control_typing_uses_explicit_caret_but_retains_widget_events() { + let mut app = app(); + let ctx = egui::Context::default(); + assert_eq!(call(&mut app, &ctx, "ui.text", json!({"text":"X"}))["ok"], false); + let f = app.session.execute("frame.create", &json!({"rect":[36,36,300,200],"content":"text","text":"Hello"})).unwrap(); + app.session.execute("text.select", &json!({"story":f["story"],"anchor":5})).unwrap(); + assert_eq!(app.session.tool_id(), "selection"); + assert_eq!(call(&mut app, &ctx, "ui.text", json!({"text":"X"}))["ok"], true); + assert_eq!(app.session.execute("story.get", &json!({"story":f["story"]})).unwrap()["text"], "HelloX"); + app.session.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(app.session.execute("story.get", &json!({"story":f["story"]})).unwrap()["text"], "Hello"); + app.select_tool("type"); + assert!(app.session.wants_text(), "the document would receive ordinary typing without widget focus"); + app.ui.palette = Some(String::new()); + for _ in 0..3 { + frame(&mut app, &ctx, vec![]); + } + assert!(ctx.text_edit_focused()); + assert_eq!(call(&mut app, &ctx, "ui.text", json!({"text":"field"}))["ok"], true); + let events = std::mem::take(&mut app.synthetic); + frame(&mut app, &ctx, events); + assert_eq!(app.ui.palette.as_deref(), Some("field")); + assert_eq!(app.session.execute("story.get", &json!({"story":f["story"]})).unwrap()["text"], "Hello"); + } + #[test] + fn control_pages_validate_before_writer_and_preserve_omission() { + let mut app = app(); + let ctx = egui::Context::default(); + let writes = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let sink = writes.clone(); + app.services.write = Some(Box::new(move |_, bytes| { + sink.lock().unwrap().push(bytes.to_vec()); + Ok(()) + })); + app.session.execute("frame.create", &json!({"rect":[36,36,300,200]})).unwrap(); + app.canvas_rect = Some(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(1400.0, 900.0))); + crate::canvas::go_to_page(&mut app, 1); + assert_eq!(crate::canvas::current_page(&app), Some(1)); + for method in ["ui.render", "app.export"] { + for page in + [json!(-1), json!(-2), json!(1.5), Value::Null, json!("0"), json!(true), json!(u64::MAX), json!(18446744073709551616_f64), json!(99)] + { + assert_eq!(call(&mut app, &ctx, method, json!({"page":page,"path":"unchanged.png","scale":0.1}))["ok"], false, "{method}: {page}"); + assert!(writes.lock().unwrap().is_empty()); + } + assert_eq!(call(&mut app, &ctx, method, json!({"path":"omitted.png","scale":0.1}))["ok"], true); + assert_eq!(call(&mut app, &ctx, method, json!({"page":1,"path":"explicit.png","scale":0.1}))["ok"], true); + let mut bytes = writes.lock().unwrap(); + assert_eq!(bytes.len(), 2); + assert_eq!(bytes[0], bytes[1]); + bytes.clear(); + } + assert!(app.run("app.exportPng", json!({"page":Value::Null,"path":"unchanged.png"})).is_err()); + assert!(writes.lock().unwrap().is_empty()); + } +} diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index be0b030..2a04e7c 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -1393,7 +1393,7 @@ fn download_document(app: &mut DesignApp) -> Result { fn export_png(app: &mut DesignApp, p: &Value) -> Result { let st = app.session.active().ok_or("no document")?; - let page = p.get("page").and_then(Value::as_u64).map(|v| v as usize).or_else(|| crate::canvas::current_page(app)).unwrap_or(0); + let page = crate::control::page_index(p, crate::canvas::current_page(app).unwrap_or(0), st.doc.page_count())?; let scale = p.get("scale").and_then(Value::as_f64).unwrap_or(2.0); let mut r = designcraft_render::Renderer::new(); let img = r