diff --git a/crates/openpencil-shell-core/src/document/mcp_apply.rs b/crates/openpencil-shell-core/src/document/mcp_apply.rs index 68f7f781c..80be979b5 100644 --- a/crates/openpencil-shell-core/src/document/mcp_apply.rs +++ b/crates/openpencil-shell-core/src/document/mcp_apply.rs @@ -50,6 +50,66 @@ fn remove_in_subtree(children: &mut Vec, target: NodeId) -> bool { false } +/// Immutable lookup for a node anywhere in the document. Used by +/// the move cycle-check (which needs to walk a subtree without +/// holding a mutable borrow on it). +fn find_node_in_doc(doc: &Document, target: NodeId) -> Option<&Node> { + for page in doc.pages.iter() { + if let Some(node) = find_in_subtree_ref(&page.children, target) { + return Some(node); + } + } + None +} + +fn find_in_subtree_ref(children: &[Node], target: NodeId) -> Option<&Node> { + for node in children.iter() { + if node.id == target { + return Some(node); + } + } + for node in children.iter() { + if let Some(found) = find_in_subtree_ref(&node.children, target) { + return Some(found); + } + } + None +} + +/// True when `node` or any descendant has id == `target`. Used by +/// move's cycle guard: reparenting source under a descendant of +/// itself would orphan + cycle the subtree. +fn subtree_contains(node: &Node, target: NodeId) -> bool { + if node.id == target { + return true; + } + node.children.iter().any(|c| subtree_contains(c, target)) +} + +/// Find + detach the node with `target` id from its parent's +/// children vec, returning the owned Node. None when not found. +/// Walks every page recursively. +fn detach_node(doc: &mut Document, target: NodeId) -> Option { + for page in doc.pages.iter_mut() { + if let Some(node) = detach_from_subtree(&mut page.children, target) { + return Some(node); + } + } + None +} + +fn detach_from_subtree(children: &mut Vec, target: NodeId) -> Option { + if let Some(idx) = children.iter().position(|n| n.id == target) { + return Some(children.remove(idx)); + } + for node in children.iter_mut() { + if let Some(detached) = detach_from_subtree(&mut node.children, target) { + return Some(detached); + } + } + None +} + /// 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`). @@ -193,6 +253,67 @@ impl Document { } removed } + crate::mcp::McpCommand::MoveNode { + node_id, + target_parent_id, + } => { + let Some(source) = NodeId::new_opt(*node_id) else { + return false; + }; + // Same-id reparent is a no-op (also: target == + // source would cycle). + if source.raw() == *target_parent_id { + return false; + } + let target_parent = NodeId::new_opt(*target_parent_id); + // Cycle guard: if target_parent is a descendant of + // source, the move would orphan + cycle the + // subtree. Walk source's subtree before detaching. + if let Some(target_id) = target_parent { + if let Some(src_node) = find_node_in_doc(self, source) { + if subtree_contains(src_node, target_id) { + return false; + } + } else { + return false; // source missing + } + } + // Detach the source from its current parent. + let Some(detached) = detach_node(self, source) else { + return false; + }; + // Reattach. None target_parent → active page root. + let attached = match target_parent { + None => { + let active_idx = self.active_page_index; + match self.pages.get_mut(active_idx) { + Some(page) => { + page.children.push(detached); + true + } + None => false, + } + } + Some(pid) => match find_node_mut_in_doc(self, pid) { + Some(parent) => { + parent.children.push(detached); + true + } + None => false, + }, + }; + if !attached { + // Reattachment failed — silently dropped the + // node. Return false so the caller knows the + // op didn't land; but the document state has + // already changed (detached). Realistically + // this only fires under corrupted target + // states; the cycle check + new_opt + sourceness + // checks above cover the validation paths. + return false; + } + true + } _ => self.var_table.apply_mcp_command(cmd), } } diff --git a/crates/openpencil-shell-core/src/document/variables.rs b/crates/openpencil-shell-core/src/document/variables.rs index 52d093c2a..d0345ada3 100644 --- a/crates/openpencil-shell-core/src/document/variables.rs +++ b/crates/openpencil-shell-core/src/document/variables.rs @@ -179,7 +179,8 @@ impl VariableTable { } crate::mcp::McpCommand::InsertNode { .. } | crate::mcp::McpCommand::UpdateNode { .. } - | crate::mcp::McpCommand::DeleteNode { .. } => { + | crate::mcp::McpCommand::DeleteNode { .. } + | crate::mcp::McpCommand::MoveNode { .. } => { // 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 3a89e9fe2..5a4ef9132 100644 --- a/crates/openpencil-shell-core/src/mcp.rs +++ b/crates/openpencil-shell-core/src/mcp.rs @@ -24,9 +24,9 @@ pub use tools::{ GetSelection, ListPages, ListVariables, NodeRecord, VariableRecord, }; pub use write_tools::{ - delete_node_snapshot, insert_node_snapshot, set_active_axis_value_snapshot, - set_variable_color_snapshot, update_node_snapshot, DeleteNode, InsertNode, - SetActiveAxisValue, SetVariableColor, UpdateNode, + delete_node_snapshot, insert_node_snapshot, move_node_snapshot, + set_active_axis_value_snapshot, set_variable_color_snapshot, update_node_snapshot, + DeleteNode, InsertNode, MoveNode, SetActiveAxisValue, SetVariableColor, UpdateNode, }; /// JSON-RPC-style request id. Strings + integers both supported by @@ -155,6 +155,15 @@ pub enum McpCommand { /// resolve OR points at a Page root (use page mutators for /// page deletion). DeleteNode { node_id: u64 }, + /// Reparent a node. `target_parent_id == 0` reparents to the + /// page root of the currently-active page. Non-zero ids must + /// resolve to an existing node; the applier rejects moves + /// where the target would create a cycle (target is a + /// descendant of the moved node). + MoveNode { + node_id: u64, + target_parent_id: u64, + }, } /// Trait every MCP tool implements. The MCP server walks its diff --git a/crates/openpencil-shell-core/src/mcp/write_tools.rs b/crates/openpencil-shell-core/src/mcp/write_tools.rs index e68bc9bf9..126551a7d 100644 --- a/crates/openpencil-shell-core/src/mcp/write_tools.rs +++ b/crates/openpencil-shell-core/src/mcp/write_tools.rs @@ -383,6 +383,70 @@ pub fn delete_node_snapshot() -> DeleteNode { DeleteNode } +/// First-party `move_node` tool — reparent a node. Required args: +/// node_id, target_parent_id. `target_parent_id == "0"` reparents +/// to the active page root. Non-zero ids must resolve to an +/// existing node; the applier rejects cycles (target descendant +/// of source) at apply time. +pub struct MoveNode; + +impl McpTool for MoveNode { + fn name(&self) -> &str { + "move_node" + } + fn call(&self, args: &BTreeMap) -> ToolOutcome { + let Some(raw_id) = args.get("node_id") else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "node_id is required".into(), + ); + }; + let node_id: u64 = match raw_id.parse() { + Ok(n) if n > 0 => n, + _ => { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("node_id must be a positive u64, got {raw_id:?}"), + ); + } + }; + let Some(raw_target) = args.get("target_parent_id") else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "target_parent_id is required (0 = page root)".into(), + ); + }; + let target_parent_id: u64 = match raw_target.parse() { + Ok(n) => n, + Err(_) => { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("target_parent_id must be u64, got {raw_target:?}"), + ); + } + }; + if node_id == target_parent_id { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + "node_id and target_parent_id must differ".into(), + ); + } + let mut out = BTreeMap::new(); + out.insert("wrote".into(), "true".into()); + ToolOutcome::OkWithCommand( + out, + McpCommand::MoveNode { + node_id, + target_parent_id, + }, + ) + } +} + +pub fn move_node_snapshot() -> MoveNode { + MoveNode +} + pub fn set_active_axis_value_snapshot( doc: &crate::document::Document, ) -> SetActiveAxisValue { diff --git a/crates/openpencil-shell-core/src/mcp/write_tools_tests.rs b/crates/openpencil-shell-core/src/mcp/write_tools_tests.rs index 5b6356d30..6dee11b21 100644 --- a/crates/openpencil-shell-core/src/mcp/write_tools_tests.rs +++ b/crates/openpencil-shell-core/src/mcp/write_tools_tests.rs @@ -595,3 +595,119 @@ fn apply_mcp_command_update_node_is_atomic_on_invalid_hex() { assert_eq!(title.bounds.origin.y, before.origin.y); assert_eq!(title.name, "Title"); } + +#[test] +fn move_node_validates_args() { + let tool = move_node_snapshot(); + // Missing both. + match tool.call(&BTreeMap::new()) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::MissingArgument), + _ => panic!(), + } + // node_id only. + let mut args = BTreeMap::new(); + args.insert("node_id".into(), "10".into()); + match tool.call(&args) { + ToolOutcome::Err(code, msg) => { + assert_eq!(code, ToolErrorCode::MissingArgument); + assert!(msg.contains("target_parent_id")); + } + _ => panic!(), + } + // node_id == 0 invalid. + args.insert("node_id".into(), "0".into()); + args.insert("target_parent_id".into(), "5".into()); + match tool.call(&args) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::InvalidArgument), + _ => panic!(), + } + // node_id == target_parent_id. + args.insert("node_id".into(), "10".into()); + args.insert("target_parent_id".into(), "10".into()); + match tool.call(&args) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::InvalidArgument), + _ => panic!(), + } +} + +#[test] +fn move_node_returns_command_with_zero_target_for_page_root() { + let tool = move_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("node_id".into(), "11".into()); + args.insert("target_parent_id".into(), "0".into()); + match tool.call(&args) { + ToolOutcome::OkWithCommand(_, McpCommand::MoveNode { node_id, target_parent_id }) => { + assert_eq!(node_id, 11); + assert_eq!(target_parent_id, 0); + } + other => panic!("expected MoveNode, got {other:?}"), + } +} + +#[test] +fn apply_mcp_command_move_node_reparents_to_page_root() { + use crate::document::Document; + // Sample doc layout: page → frame(10) → [title(11), button(12) → [rect(13), text(14)]] + let mut doc = Document::sample(); + let cmd = McpCommand::MoveNode { + node_id: 11, + target_parent_id: 0, // page root + }; + assert!(doc.apply_mcp_command(&cmd)); + // Title is now a sibling of Frame at the page root. + let root = &doc.pages[0].children; + assert!(root.iter().any(|n| n.id.raw() == 11), "title at page root"); + // Frame no longer carries Title. + let frame = root.iter().find(|n| n.id.raw() == 10).unwrap(); + assert!(frame.children.iter().all(|n| n.id.raw() != 11)); +} + +#[test] +fn apply_mcp_command_move_node_reparents_to_another_node() { + use crate::document::Document; + let mut doc = Document::sample(); + // Move Title (id 11) under the Button group (id 12). + let cmd = McpCommand::MoveNode { + node_id: 11, + target_parent_id: 12, + }; + assert!(doc.apply_mcp_command(&cmd)); + let frame = doc.pages[0].children.iter().find(|n| n.id.raw() == 10).unwrap(); + let button = frame.children.iter().find(|n| n.id.raw() == 12).unwrap(); + assert!(button.children.iter().any(|n| n.id.raw() == 11), "title under button"); + assert!(frame.children.iter().all(|n| n.id.raw() != 11), "title detached from frame"); +} + +#[test] +fn apply_mcp_command_move_node_rejects_cycle() { + use crate::document::Document; + let mut doc = Document::sample(); + // Frame (id 10) contains Button (id 12). Move Frame UNDER Button + // — that would cycle. Apply must reject. + let cmd = McpCommand::MoveNode { + node_id: 10, + target_parent_id: 12, + }; + assert!(!doc.apply_mcp_command(&cmd), "cycle move must reject"); + // Frame is still at the page root with Button as child. + let root = &doc.pages[0].children; + let frame = root.iter().find(|n| n.id.raw() == 10).unwrap(); + assert!(frame.children.iter().any(|n| n.id.raw() == 12)); +} + +#[test] +fn apply_mcp_command_move_node_rejects_unknown_id() { + use crate::document::Document; + let mut doc = Document::sample(); + // Unknown source. + assert!(!doc.apply_mcp_command(&McpCommand::MoveNode { + node_id: 99999, + target_parent_id: 10, + })); + // Unknown target. + assert!(!doc.apply_mcp_command(&McpCommand::MoveNode { + node_id: 11, + target_parent_id: 99999, + })); +}