From e12bc4f0bc6670f44e66e454f2ef7d64933075db Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 21:31:12 +0800 Subject: [PATCH] =?UTF-8?q?feat(mcp):=20replace=5Fnode=20write=20tool=20?= =?UTF-8?q?=E2=80=94=20atomic=20swap=20at=20same=20slot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eighth write tool in the catalog. Required args: node_id, kind, name, x, y, width, height. Optional: fill_hex. Builds a fresh node with a non-colliding id and swaps it into the same parent slot the target node currently occupies, preserving sibling order. Bounded scope: only leaf-style fields land on the replacement. Full subtree replacement requires a JSON Node parser that doesn't live on this side yet. The current contract matches TS `replace_node` for primitives, minus children. Apply path follows the same pre-validate-then-mutate discipline as `update_node` and `move_node`: kind / geometry / fill_hex / target existence / id space — every check before any mutation. A bad fill_hex never leaves the document half-touched (covered by `apply_mcp_command_replace_node_atomic_on_invalid_fill_hex`). Tests live in `mcp/replace_node_tests.rs` (matches the `copy_node_tests.rs` sibling pattern; keeps `write_tools_tests.rs` under cap). --- .../src/document/mcp_apply.rs | 82 ++++++++++ .../src/document/variables.rs | 3 +- crates/openpencil-shell-core/src/mcp.rs | 32 +++- .../src/mcp/replace_node_tests.rs | 148 ++++++++++++++++++ .../src/mcp/write_tools.rs | 113 +++++++++++++ 5 files changed, 374 insertions(+), 4 deletions(-) create mode 100644 crates/openpencil-shell-core/src/mcp/replace_node_tests.rs diff --git a/crates/openpencil-shell-core/src/document/mcp_apply.rs b/crates/openpencil-shell-core/src/document/mcp_apply.rs index a6e83b624..f858733d3 100644 --- a/crates/openpencil-shell-core/src/document/mcp_apply.rs +++ b/crates/openpencil-shell-core/src/document/mcp_apply.rs @@ -130,6 +130,43 @@ fn clone_subtree(node: &Node, next_id: &mut u64) -> Option { Some(clone) } +/// Replace the node with `target` id with `replacement` at its +/// current slot. Walks every page; on the first match, +/// swaps in place (`children[idx] = replacement`) so the +/// sibling order is preserved. Returns true on success. +fn replace_node_in_doc(doc: &mut Document, target: NodeId, replacement: Node) -> bool { + // Wrap the replacement in an Option so we can take() it inside + // the recursive walk without giving up the Some-on-failure + // contract (replacement returned untouched is irrelevant here + // — we either consumed it or replace_node already returned). + let mut slot = Some(replacement); + for page in doc.pages.iter_mut() { + if replace_in_subtree(&mut page.children, target, &mut slot) { + return true; + } + } + false +} + +fn replace_in_subtree( + children: &mut [Node], + target: NodeId, + slot: &mut Option, +) -> bool { + if let Some(idx) = children.iter().position(|n| n.id == target) { + if let Some(replacement) = slot.take() { + children[idx] = replacement; + return true; + } + } + for node in children.iter_mut() { + if replace_in_subtree(&mut node.children, target, slot) { + return true; + } + } + false +} + /// Resolve an MCP `kind` arg into a NodeKind. Accepts the same /// lowercase strings the read-side tools emit (`frame`, `group`, /// `rect`, `ellipse`, `polygon`, `line`, `text`, `path`). @@ -323,6 +360,51 @@ impl Document { } true } + crate::mcp::McpCommand::ReplaceNode { + node_id, + kind, + name, + x, + y, + width, + height, + fill_hex, + } => { + let Some(target) = NodeId::new_opt(*node_id) else { + return false; + }; + let Some(node_kind) = parse_node_kind(kind) else { + return false; + }; + // Pre-validate EVERYTHING before mutating (same + // discipline as update_node + move_node — no + // half-mutated state on a bad arg). + if *width < 0 || *height < 0 { + return false; + } + let fill = match fill_hex { + None => None, + Some(hex) => match parse_hex_color(hex) { + Some(c) => Some(c), + None => return false, + }, + }; + if find_node_in_doc(self, target).is_none() { + return false; + } + let Some(next_id) = self.next_node_id_seed() else { + return false; + }; + let mut replacement = Node::leaf(next_id, node_kind, name.clone()); + replacement.bounds = crate::Rect::xywh( + *x as f32, + *y as f32, + *width as f32, + *height as f32, + ); + replacement.fill = fill; + replace_node_in_doc(self, target, replacement) + } crate::mcp::McpCommand::MoveNode { node_id, target_parent_id, diff --git a/crates/openpencil-shell-core/src/document/variables.rs b/crates/openpencil-shell-core/src/document/variables.rs index c745297e6..451729d04 100644 --- a/crates/openpencil-shell-core/src/document/variables.rs +++ b/crates/openpencil-shell-core/src/document/variables.rs @@ -181,7 +181,8 @@ impl VariableTable { | crate::mcp::McpCommand::UpdateNode { .. } | crate::mcp::McpCommand::DeleteNode { .. } | crate::mcp::McpCommand::MoveNode { .. } - | crate::mcp::McpCommand::CopyNode { .. } => { + | crate::mcp::McpCommand::CopyNode { .. } + | crate::mcp::McpCommand::ReplaceNode { .. } => { // Not VariableTable mutations — Pages-level commands // live on `Document::apply_mcp_command`. Return false // so callers with only a VariableTable handle know diff --git a/crates/openpencil-shell-core/src/mcp.rs b/crates/openpencil-shell-core/src/mcp.rs index cb6eeefa6..ae24a2fd0 100644 --- a/crates/openpencil-shell-core/src/mcp.rs +++ b/crates/openpencil-shell-core/src/mcp.rs @@ -14,6 +14,7 @@ pub mod tools; pub mod write_tools; #[cfg(test)] mod write_tools_tests; #[cfg(test)] mod copy_node_tests; +#[cfg(test)] mod replace_node_tests; // Re-export the public surface of submodules so callers can keep // using `mcp::parse_tool_call` / `mcp::GetDocumentInfo` after the @@ -26,9 +27,9 @@ pub use tools::{ }; pub use write_tools::{ copy_node_snapshot, delete_node_snapshot, insert_node_snapshot, move_node_snapshot, - set_active_axis_value_snapshot, set_variable_color_snapshot, update_node_snapshot, - CopyNode, DeleteNode, InsertNode, MoveNode, SetActiveAxisValue, SetVariableColor, - UpdateNode, + replace_node_snapshot, set_active_axis_value_snapshot, set_variable_color_snapshot, + update_node_snapshot, CopyNode, DeleteNode, InsertNode, MoveNode, ReplaceNode, + SetActiveAxisValue, SetVariableColor, UpdateNode, }; /// JSON-RPC-style request id. Strings + integers both supported by @@ -180,6 +181,31 @@ pub enum McpCommand { node_id: u64, target_parent_id: u64, }, + /// Replace an existing node with a freshly-built one at the + /// same parent slot + same index. Captures the same shape as + /// `InsertNode` (kind / name / bounds / fill_hex) plus the + /// target `node_id` to swap. Useful when the LLM wants to + /// change kind (rect → ellipse) or radically alter the node + /// in one atomic op rather than via incremental `UpdateNode` + /// patches. The new node gets a fresh id past `max_node_id()` + /// so the wire response is still fire-and-forget (callers + /// re-query to learn the new id). + /// + /// Bounded scope: today only the leaf-style fields land on + /// the replacement node. A future patch may grow this to + /// carry children / a full subtree once a JSON Node parser + /// lives on the host. The current contract matches what TS + /// `replace_node` accepts for primitives, minus children. + ReplaceNode { + node_id: u64, + kind: String, + name: String, + x: i32, + y: i32, + width: i32, + height: i32, + fill_hex: Option, + }, } /// Trait every MCP tool implements. The MCP server walks its diff --git a/crates/openpencil-shell-core/src/mcp/replace_node_tests.rs b/crates/openpencil-shell-core/src/mcp/replace_node_tests.rs new file mode 100644 index 000000000..596bf8a9f --- /dev/null +++ b/crates/openpencil-shell-core/src/mcp/replace_node_tests.rs @@ -0,0 +1,148 @@ +//! Tests for `mcp::write_tools::ReplaceNode` + the matching +//! `Document::apply_mcp_command(ReplaceNode)` branch. Sibling +//! file (same rationale as `copy_node_tests.rs`) — keeps +//! `write_tools_tests.rs` under the 800-line cap as the write +//! surface grows. + +use super::write_tools::*; +use super::{McpCommand, McpTool, ToolErrorCode, ToolOutcome}; +use std::collections::BTreeMap; + +#[test] +fn replace_node_validates_required_args() { + let tool = replace_node_snapshot(); + match tool.call(&BTreeMap::new()) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::MissingArgument), + _ => panic!(), + } + let mut args = BTreeMap::new(); + args.insert("node_id".into(), "11".into()); + match tool.call(&args) { + ToolOutcome::Err(code, msg) => { + assert_eq!(code, ToolErrorCode::MissingArgument); + assert!(msg.contains("kind")); + } + _ => panic!(), + } + args.insert("kind".into(), "rect".into()); + args.insert("name".into(), "Replacement".into()); + args.insert("x".into(), "10".into()); + args.insert("y".into(), "20".into()); + args.insert("width".into(), "100".into()); + args.insert("height".into(), "30".into()); + match tool.call(&args) { + ToolOutcome::OkWithCommand(_, McpCommand::ReplaceNode { node_id, kind, name, x, y, width, height, fill_hex }) => { + assert_eq!(node_id, 11); + assert_eq!(kind, "rect"); + assert_eq!(name, "Replacement"); + assert_eq!(x, 10); + assert_eq!(y, 20); + assert_eq!(width, 100); + assert_eq!(height, 30); + assert!(fill_hex.is_none()); + } + other => panic!("expected ReplaceNode, got {other:?}"), + } +} + +#[test] +fn replace_node_rejects_bad_kind_and_geometry_and_fill() { + let tool = replace_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("node_id".into(), "11".into()); + args.insert("kind".into(), "blob".into()); // not in ALLOWED_KINDS + args.insert("name".into(), "X".into()); + args.insert("x".into(), "0".into()); + args.insert("y".into(), "0".into()); + args.insert("width".into(), "10".into()); + args.insert("height".into(), "10".into()); + match tool.call(&args) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::InvalidArgument), + _ => panic!(), + } + args.insert("kind".into(), "rect".into()); + args.insert("width".into(), "-5".into()); + match tool.call(&args) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::InvalidArgument), + _ => panic!(), + } + args.insert("width".into(), "5".into()); + args.insert("fill_hex".into(), "not-a-hex".into()); + match tool.call(&args) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::InvalidArgument), + _ => panic!(), + } +} + +#[test] +fn apply_mcp_command_replace_node_swaps_at_same_slot() { + use crate::document::Document; + let mut doc = Document::sample(); + let pre_max = doc.max_node_id(); + // Title (id 11) is at frame.children[0]. Replace it with a Rect. + let cmd = McpCommand::ReplaceNode { + node_id: 11, + kind: "rect".into(), + name: "Swapped".into(), + x: 0, + y: 0, + width: 50, + height: 50, + fill_hex: Some("#ff0000".into()), + }; + assert!(doc.apply_mcp_command(&cmd)); + let frame = doc.pages[0].children.iter().find(|n| n.id.raw() == 10).unwrap(); + // Slot 0 now holds the replacement; old id is gone. + assert!(frame.children.iter().all(|n| n.id.raw() != 11)); + let swapped = &frame.children[0]; + assert_eq!(swapped.name, "Swapped"); + assert!(swapped.id.raw() > pre_max, "fresh id past pre_max"); + // Sibling (Button group, id 12) still at slot 1. + assert_eq!(frame.children[1].id.raw(), 12); +} + +#[test] +fn apply_mcp_command_replace_node_rejects_unknown_id() { + use crate::document::Document; + let mut doc = Document::sample(); + let pre_root_len = doc.pages[0].children.len(); + let cmd = McpCommand::ReplaceNode { + node_id: 99999, + kind: "rect".into(), + name: "Ghost".into(), + x: 0, + y: 0, + width: 10, + height: 10, + fill_hex: None, + }; + assert!(!doc.apply_mcp_command(&cmd)); + // Doc unchanged. + assert_eq!(doc.pages[0].children.len(), pre_root_len); +} + +#[test] +fn apply_mcp_command_replace_node_atomic_on_invalid_fill_hex() { + // The applier pre-validates fill_hex BEFORE allocating the + // fresh id or mutating pages (same atomicity pattern as + // update_node + move_node). + use crate::document::Document; + let mut doc = Document::sample(); + let pre_max = doc.max_node_id(); + let cmd = McpCommand::ReplaceNode { + node_id: 11, + kind: "rect".into(), + name: "WouldFail".into(), + x: 0, + y: 0, + width: 10, + height: 10, + fill_hex: Some("not-hex".into()), + }; + assert!(!doc.apply_mcp_command(&cmd)); + // Title still in place under Frame. + let frame = doc.pages[0].children.iter().find(|n| n.id.raw() == 10).unwrap(); + assert!(frame.children.iter().any(|n| n.id.raw() == 11)); + // No id was allocated. + assert_eq!(doc.max_node_id(), pre_max); +} diff --git a/crates/openpencil-shell-core/src/mcp/write_tools.rs b/crates/openpencil-shell-core/src/mcp/write_tools.rs index 1ffa9c155..4d12af52f 100644 --- a/crates/openpencil-shell-core/src/mcp/write_tools.rs +++ b/crates/openpencil-shell-core/src/mcp/write_tools.rs @@ -507,6 +507,119 @@ pub fn copy_node_snapshot() -> CopyNode { CopyNode } +/// First-party `replace_node` tool — swap an existing node with a +/// freshly-built one at the same parent slot. Required args: +/// node_id, kind, name, x, y, width, height. Optional: fill_hex. +/// Bounded scope: only leaf-style fields are accepted today (the +/// children of the replacement default to empty). TS-equivalent +/// behavior for primitives; subtree-replacement requires a JSON +/// Node parser that doesn't live on this side yet. +pub struct ReplaceNode; + +impl McpTool for ReplaceNode { + fn name(&self) -> &str { + "replace_node" + } + fn call(&self, args: &BTreeMap) -> ToolOutcome { + let raw = match args.get("node_id") { + Some(s) => s, + None => { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "node_id is required".into(), + ); + } + }; + let node_id: u64 = match raw.parse() { + Ok(n) if n > 0 => n, + _ => { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("node_id must be a positive u64, got {raw:?}"), + ); + } + }; + let kind = match args.get("kind") { + Some(s) => s.clone(), + None => { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "kind is required".into(), + ); + } + }; + if !ALLOWED_KINDS.iter().any(|k| *k == kind) { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!( + "kind {kind:?} not supported; allowed: {}", + ALLOWED_KINDS.join(", ") + ), + ); + } + let name = match args.get("name") { + Some(s) => s.clone(), + None => { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "name is required".into(), + ); + } + }; + let x = match parse_i32_arg(args, "x") { + Ok(v) => v, + Err(e) => return e, + }; + let y = match parse_i32_arg(args, "y") { + Ok(v) => v, + Err(e) => return e, + }; + let width = match parse_i32_arg(args, "width") { + Ok(v) => v, + Err(e) => return e, + }; + let height = match parse_i32_arg(args, "height") { + Ok(v) => v, + Err(e) => return e, + }; + if width < 0 || height < 0 { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + "width / height must be non-negative".into(), + ); + } + let fill_hex = match args.get("fill_hex") { + None => None, + Some(s) if !validate_hex(s) => { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("fill_hex must be #rgb/#rrggbb/#rrggbbaa, got {s:?}"), + ); + } + Some(s) => Some(s.clone()), + }; + let mut out = BTreeMap::new(); + out.insert("wrote".into(), "true".into()); + ToolOutcome::OkWithCommand( + out, + McpCommand::ReplaceNode { + node_id, + kind, + name, + x, + y, + width, + height, + fill_hex, + }, + ) + } +} + +pub fn replace_node_snapshot() -> ReplaceNode { + ReplaceNode +} + pub fn set_active_axis_value_snapshot( doc: &crate::document::Document, ) -> SetActiveAxisValue {