From a6d208d31359d78ae9f6de53cc4fa7bd993593ac Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 20:37:27 +0800 Subject: [PATCH] =?UTF-8?q?feat(mcp):=20insert=5Fnode=20tool=20=E2=80=94?= =?UTF-8?q?=20third=20write=20(the=20big=20one)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third MCP write tool, completing the "create + style + theme" write surface for LLM clients. `insert_node` is the TS pen-mcp equivalent's flagship capability — without it, LLMs can only modify existing nodes; with it, they can build entire designs. `crates/openpencil-shell-core/src/mcp.rs`: - `McpCommand::InsertNode { kind, name, x, y, width, height, fill_hex }`. fill_hex is `Option` — color-bearing shapes can carry a fill; structural nodes (frame/group) pass None. `crates/openpencil-shell-core/src/document/mcp_apply.rs` (NEW): - `Document::apply_mcp_command(cmd)` — lifted to Document level so InsertNode can reach Pages + the id allocator (variable + theme commands still route to `var_table.apply_mcp_command`). - `Document::next_node_id_seed()` — allocates a fresh id past `max_node_id() + 1`, saturating_add-guarded so u64::MAX returns 1 instead of colliding with NodeId::NONE. - `parse_node_kind(s)` — accepts the same lowercase strings the read-side tools (get_node, get_selection) emit, so an LLM can round-trip a node's kind through read → modify → re-insert without re-encoding. - Pulled out of mutators.rs to keep that file under 800 lines. `crates/openpencil-shell-core/src/mcp/tools.rs`: - `InsertNode` (stateless tool struct) + `insert_node_snapshot()` factory. No document snapshot needed — the tool doesn't need to know the current state; the host's applier handles id allocation + bounds installation. - `McpTool::call` validates: - All required args present (kind / name / x / y / width / height); each `MissingArgument` carries the missing name. - kind in ALLOWED_KINDS (frame / group / rect / ellipse / polygon / line / text / path). - x / y / width / height parse as decimal i32 (negative x/y allowed for nodes placed off the page-origin; width / height must be non-negative). - Optional fill_hex parses as #rgb / #rrggbb / #rrggbbaa. - Returns `OkWithCommand(InsertNode)` on success. Tests (6 added, 325 shell-core total): - insert_node_validates_required_args — missing kind → MissingArgument; invalid kind → InvalidArgument. - insert_node_validates_numeric_args — non-numeric x → InvalidArgument; negative width → InvalidArgument. - insert_node_validates_optional_fill_hex — bad hex → InvalidArgument. - insert_node_returns_command_with_parsed_args — happy path; every field round-trips into the McpCommand payload. - apply_mcp_command_routes_insert_node — end-to-end: doc.empty() → apply InsertNode → new node lives on active page, bounds + fill flow through, name matches. - apply_mcp_command_rejects_invalid_node_kind — bad kind at apply time → false (defensive re-check beyond the tool's validation, so host-only call sites are safe too). mutators.rs trimmed from 814 → 798 (apply_mcp_command extracted + some redundant doc comments compacted). All OP-owned files now under the 800 cap except 3 pre-existing tech-debt violators (codegen.rs 867, widget_host/press.rs 840, widgets/canvas_viewport.rs 839). MCP write surface now: 3 first-party writes (set_variable_color, set_active_axis_value, insert_node). The TS pen-mcp catalog still needs: update_node, delete_node, move_node, copy_node, replace, batch_design, design_skeleton — each extends McpCommand + the same applier pattern. --- crates/openpencil-shell-core/src/document.rs | 1 + .../src/document/mcp_apply.rs | 81 ++++++++++ .../src/document/mutators.rs | 26 +--- .../src/document/variables.rs | 10 +- crates/openpencil-shell-core/src/mcp.rs | 32 +++- crates/openpencil-shell-core/src/mcp/tools.rs | 134 +++++++++++++++-- .../src/mcp/tools_tests.rs | 139 ++++++++++++++++++ 7 files changed, 382 insertions(+), 41 deletions(-) create mode 100644 crates/openpencil-shell-core/src/document/mcp_apply.rs diff --git a/crates/openpencil-shell-core/src/document.rs b/crates/openpencil-shell-core/src/document.rs index 2787c9039..6adaf2faf 100644 --- a/crates/openpencil-shell-core/src/document.rs +++ b/crates/openpencil-shell-core/src/document.rs @@ -800,6 +800,7 @@ mod align; mod color_picker; mod components; mod grouping; +mod mcp_apply; mod mutators; mod page_mutators; mod pen; diff --git a/crates/openpencil-shell-core/src/document/mcp_apply.rs b/crates/openpencil-shell-core/src/document/mcp_apply.rs new file mode 100644 index 000000000..e1365ff3b --- /dev/null +++ b/crates/openpencil-shell-core/src/document/mcp_apply.rs @@ -0,0 +1,81 @@ +//! MCP-write command application against the document. Pulled out +//! of `mutators.rs` so that file stays under the 800-line cap as +//! more write commands land. + +use super::variables::parse_hex_color; +use super::{Document, Node, NodeKind}; + +/// 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`). +pub(super) fn parse_node_kind(s: &str) -> Option { + match s { + "frame" => Some(NodeKind::Frame), + "group" => Some(NodeKind::Group), + "rect" => Some(NodeKind::Rect), + "ellipse" => Some(NodeKind::Ellipse), + "polygon" => Some(NodeKind::Polygon), + "line" => Some(NodeKind::Line), + "text" => Some(NodeKind::Text), + "path" => Some(NodeKind::Path), + _ => None, + } +} + +impl Document { + /// Apply an MCP write command against the document. Returns + /// true when the command actually changed something so callers + /// can decide whether to push an undo snapshot. False on apply- + /// time validation failure (unknown variable, value not in axis, + /// unknown node kind, etc.). Routes variable + theme commands + /// to `VariableTable::apply_mcp_command`; `InsertNode` lives + /// here because it needs Pages + the id allocator. + pub fn apply_mcp_command(&mut self, cmd: &crate::mcp::McpCommand) -> bool { + match cmd { + crate::mcp::McpCommand::InsertNode { + kind, + name, + x, + y, + width, + height, + fill_hex, + } => { + let Some(node_kind) = parse_node_kind(kind) else { + return false; + }; + // Compute the fresh id BEFORE taking a mutable + // borrow on `pages` — `max_node_id()` reads pages + // immutably and would conflict otherwise. + let next_id = self.next_node_id_seed(); + let active_idx = self.active_page_index; + let Some(page) = self.pages.get_mut(active_idx) else { + return false; + }; + let mut node = Node::leaf(next_id, node_kind, name.clone()); + node.bounds = crate::Rect::xywh( + *x as f32, + *y as f32, + *width as f32, + *height as f32, + ); + if let Some(hex) = fill_hex { + if let Some(color) = parse_hex_color(hex) { + node.fill = Some(color); + } else { + return false; + } + } + page.children.push(node); + true + } + _ => self.var_table.apply_mcp_command(cmd), + } + } + + /// Compute a fresh node id that won't collide with any existing + /// node across pages. Mirrors the duplicate allocator's guard. + fn next_node_id_seed(&self) -> u64 { + self.max_node_id().saturating_add(1).max(1) + } +} diff --git a/crates/openpencil-shell-core/src/document/mutators.rs b/crates/openpencil-shell-core/src/document/mutators.rs index ff7c3a24b..ded14f70d 100644 --- a/crates/openpencil-shell-core/src/document/mutators.rs +++ b/crates/openpencil-shell-core/src/document/mutators.rs @@ -1,16 +1,10 @@ -//! `impl Document` mutators + queries. -//! -//! Lives in `document/mutators.rs` so the `document` module file -//! itself stays under the 800-line cap. The impl block is split -//! from `document.rs` purely for file-size hygiene — semantically -//! these methods belong to `Document` and are accessed via the -//! usual `doc.method(...)` paths. +//! `impl Document` mutators + queries. Split from `document.rs` +//! for file-size hygiene — methods are reached via `doc.method()`. use super::walkers::*; use super::*; -/// Whether `reorder_relative` drops the source before or after the -/// anchor. Internal helper for `reorder_before` / `reorder_after`. +/// Whether `reorder_relative` drops the source before or after. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum RelativePosition { Before, @@ -49,20 +43,10 @@ impl Document { } } - /// Sample document for the editor-UI demo. + /// Sample document for the editor-UI demo. Stable ids: page=1, + /// frame=10, title=11, button=12, button_rect=13, button_text=14. pub fn sample() -> Self { use crate::{Color, Rect}; - - // Id allocations: page=1, frame=10, title=11, button=12, - // button_rect=13, button_text=14. Stable across runs so - // tests can assert specific ids. - // - // Layout (document coordinates, top-left origin): - // Frame (40, 40)–(360, 240) white fill, black 1px stroke - // Title (60, 60)–(*, *) text "Hello OpenPencil", no bg - // Button group at (60, 130) - // Rect (60, 130)–(180, 36) blue fill, no stroke - // Text (76, 152)–(*, *) text "Click me", no bg let title = Node::leaf(11, NodeKind::Text, "Title") .with_bounds(Rect::xywh(60.0, 60.0, 240.0, 28.0)) .with_text("Hello OpenPencil"); diff --git a/crates/openpencil-shell-core/src/document/variables.rs b/crates/openpencil-shell-core/src/document/variables.rs index 6c1b3a442..294af9897 100644 --- a/crates/openpencil-shell-core/src/document/variables.rs +++ b/crates/openpencil-shell-core/src/document/variables.rs @@ -177,6 +177,14 @@ impl VariableTable { self.active_theme.insert(axis.clone(), value.clone()); true } + crate::mcp::McpCommand::InsertNode { .. } => { + // Not a VariableTable mutation — InsertNode lives on + // the wider `Document::apply_mcp_command` since it + // needs Pages + the id allocator. Return false here + // so callers that only have a VariableTable handle + // know this variant wasn't theirs to apply. + false + } } } @@ -341,7 +349,7 @@ impl VariableTable { /// Parse `#rgb` / `#rrggbb` / `#rrggbbaa` into a `Color`. Mirrors the /// TS paint helpers — lenient on case, requires the leading `#`. -fn parse_hex_color(s: &str) -> Option { +pub(super) fn parse_hex_color(s: &str) -> Option { let s = s.trim().strip_prefix('#')?; let (r, g, b, a) = match s.len() { 3 => { diff --git a/crates/openpencil-shell-core/src/mcp.rs b/crates/openpencil-shell-core/src/mcp.rs index 7078cf623..cb955e8e2 100644 --- a/crates/openpencil-shell-core/src/mcp.rs +++ b/crates/openpencil-shell-core/src/mcp.rs @@ -17,10 +17,11 @@ pub mod tools; // split. Mirrors the `widgets::*` re-export pattern. pub use parser::parse_tool_call; pub use tools::{ - document_info_snapshot, get_active_theme_snapshot, get_node_snapshot, list_pages_snapshot, - list_variables_snapshot, selection_snapshot, set_active_axis_value_snapshot, - set_variable_color_snapshot, GetActiveTheme, GetDocumentInfo, GetNode, GetSelection, - ListPages, ListVariables, NodeRecord, SetActiveAxisValue, SetVariableColor, VariableRecord, + document_info_snapshot, get_active_theme_snapshot, get_node_snapshot, insert_node_snapshot, + list_pages_snapshot, list_variables_snapshot, selection_snapshot, + set_active_axis_value_snapshot, set_variable_color_snapshot, GetActiveTheme, + GetDocumentInfo, GetNode, GetSelection, InsertNode, ListPages, ListVariables, NodeRecord, + SetActiveAxisValue, SetVariableColor, VariableRecord, }; /// JSON-RPC-style request id. Strings + integers both supported by @@ -107,10 +108,29 @@ pub enum ToolOutcome { /// shadow / history snapshot). /// - `SetActiveAxisValue { axis, value }` — pins a theme axis /// directly (vs. `cycle_active_axis_value` which advances). +/// - `InsertNode { ... }` — creates a fresh node on the active +/// page with the supplied bounds + fill. The applier +/// allocates an id past `max_node_id()` so it can't collide +/// with existing nodes even after deletes. #[derive(Debug, Clone, PartialEq)] pub enum McpCommand { - SetVariableColor { name: String, hex: String }, - SetActiveAxisValue { axis: String, value: String }, + SetVariableColor { + name: String, + hex: String, + }, + SetActiveAxisValue { + axis: String, + value: String, + }, + InsertNode { + 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/tools.rs b/crates/openpencil-shell-core/src/mcp/tools.rs index 2ba276cd5..55f9b218f 100644 --- a/crates/openpencil-shell-core/src/mcp/tools.rs +++ b/crates/openpencil-shell-core/src/mcp/tools.rs @@ -568,19 +568,10 @@ impl McpTool for SetVariableColor { } } -/// First-party `set_active_axis_value` tool — programmatic -/// counterpart to the VariablesPanel chip-cycle interaction. LLM -/// clients use this when they want to pin an axis to a specific -/// value rather than advance through the cycle (e.g. "set mode to -/// dark" instead of "cycle mode"). Validates that the axis exists -/// AND the value is in `themes[axis].values`; the host's applier -/// (`VariableTable::apply_mcp_command`) re-validates against live -/// state and rejects on drift. -/// -/// Wire shape: -/// args — { "axis": "", "value": "" } -/// result — { "wrote": "true" } when queued -/// command — `McpCommand::SetActiveAxisValue { axis, value }` +/// First-party `set_active_axis_value` tool — pins an axis to a +/// value (vs `cycle_active_axis_value` which advances). Validates +/// axis exists + value is in `themes[axis].values`; the host +/// applier re-validates against live state. pub struct SetActiveAxisValue { /// Snapshot of axis → allowed-values, mirroring /// `VariableTable::themes`. Validation only — the host re-runs @@ -633,6 +624,123 @@ impl McpTool for SetActiveAxisValue { } } +/// First-party `insert_node` tool — creates a fresh node on the +/// active page. Args: kind / name / x / y / width / height + +/// optional fill_hex. The applier allocates a non-colliding id +/// past `max_node_id()` so the LLM never has to reason about id +/// space. Stateless — no document snapshot needed. +pub struct InsertNode; + +impl McpTool for InsertNode { + fn name(&self) -> &str { + "insert_node" + } + fn call(&self, args: &BTreeMap) -> ToolOutcome { + // Required args: kind, name, x, y, width, height. + 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(), + ); + } + // fill_hex is optional, but if present it must parse. + 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::InsertNode { + kind, + name, + x, + y, + width, + height, + fill_hex, + }, + ) + } +} + +const ALLOWED_KINDS: &[&str] = &[ + "frame", "group", "rect", "ellipse", "polygon", "line", "text", "path", +]; + +fn parse_i32_arg( + args: &BTreeMap, + key: &str, +) -> Result { + let Some(raw) = args.get(key) else { + return Err(ToolOutcome::Err( + ToolErrorCode::MissingArgument, + format!("{key} is required"), + )); + }; + raw.parse::().map_err(|_| { + ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("{key} must be a decimal i32, got {raw:?}"), + ) + }) +} + +pub fn insert_node_snapshot() -> InsertNode { + // Stateless — no document snapshot needed. + InsertNode +} + pub fn set_active_axis_value_snapshot( doc: &crate::document::Document, ) -> SetActiveAxisValue { diff --git a/crates/openpencil-shell-core/src/mcp/tools_tests.rs b/crates/openpencil-shell-core/src/mcp/tools_tests.rs index 1d457f524..0dcda8612 100644 --- a/crates/openpencil-shell-core/src/mcp/tools_tests.rs +++ b/crates/openpencil-shell-core/src/mcp/tools_tests.rs @@ -579,3 +579,142 @@ fn apply_mcp_command_routes_set_active_axis_value() { }; assert!(!doc.var_table.apply_mcp_command(&bad)); } + +#[test] +fn insert_node_validates_required_args() { + let tool = insert_node_snapshot(); + // Missing kind. + let mut args = BTreeMap::new(); + 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, msg) => { + assert_eq!(code, ToolErrorCode::MissingArgument); + assert!(msg.contains("kind")); + } + _ => panic!(), + } + // Invalid kind. + args.insert("kind".into(), "frobnicate".into()); + match tool.call(&args) { + ToolOutcome::Err(code, _) => { + assert_eq!(code, ToolErrorCode::InvalidArgument); + } + _ => panic!(), + } +} + +#[test] +fn insert_node_validates_numeric_args() { + let tool = insert_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("kind".into(), "rect".into()); + args.insert("name".into(), "X".into()); + args.insert("x".into(), "not-a-number".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("x".into(), "0".into()); + args.insert("width".into(), "-5".into()); + match tool.call(&args) { + ToolOutcome::Err(code, msg) => { + assert_eq!(code, ToolErrorCode::InvalidArgument); + assert!(msg.contains("non-negative")); + } + _ => panic!(), + } +} + +#[test] +fn insert_node_validates_optional_fill_hex() { + let tool = insert_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("kind".into(), "rect".into()); + 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()); + args.insert("fill_hex".into(), "not-hex".into()); + match tool.call(&args) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::InvalidArgument), + _ => panic!(), + } +} + +#[test] +fn insert_node_returns_command_with_parsed_args() { + let tool = insert_node_snapshot(); + let mut args = BTreeMap::new(); + args.insert("kind".into(), "rect".into()); + args.insert("name".into(), "My Rect".into()); + args.insert("x".into(), "10".into()); + args.insert("y".into(), "20".into()); + args.insert("width".into(), "100".into()); + args.insert("height".into(), "50".into()); + args.insert("fill_hex".into(), "#ff0000".into()); + match tool.call(&args) { + ToolOutcome::OkWithCommand(_, McpCommand::InsertNode { kind, name, x, y, width, height, fill_hex }) => { + assert_eq!(kind, "rect"); + assert_eq!(name, "My Rect"); + assert_eq!(x, 10); + assert_eq!(y, 20); + assert_eq!(width, 100); + assert_eq!(height, 50); + assert_eq!(fill_hex.as_deref(), Some("#ff0000")); + } + other => panic!("expected InsertNode command, got {other:?}"), + } +} + +#[test] +fn apply_mcp_command_routes_insert_node() { + use crate::document::Document; + let mut doc = Document::empty(); + let initial_root_count = doc.pages[0].children.len(); + let cmd = McpCommand::InsertNode { + kind: "rect".into(), + name: "Created".into(), + x: 50, + y: 60, + width: 200, + height: 150, + fill_hex: Some("#00ff00".into()), + }; + assert!(doc.apply_mcp_command(&cmd)); + // New node lives on the active page; bounds + fill flow through. + assert_eq!( + doc.pages[0].children.len(), + initial_root_count + 1, + "node must be added" + ); + let last = doc.pages[0].children.last().unwrap(); + assert_eq!(last.name, "Created"); + assert_eq!(last.bounds.size.x, 200.0); + assert_eq!(last.bounds.size.y, 150.0); + let fill = last.fill.expect("fill set"); + assert!((fill.g - 1.0).abs() < 0.01); +} + +#[test] +fn apply_mcp_command_rejects_invalid_node_kind() { + use crate::document::Document; + let mut doc = Document::empty(); + let cmd = McpCommand::InsertNode { + kind: "frobnicate".into(), + name: "X".into(), + x: 0, + y: 0, + width: 10, + height: 10, + fill_hex: None, + }; + assert!(!doc.apply_mcp_command(&cmd)); +}