feat(mcp): rename_page / delete_page / duplicate_page — full page CRUD
Rounds out the pages MCP surface. 32 tools total now. Full
LLM-driven page lifecycle:
- list_pages (already existed)
- set_active_page (commit 1af880c2)
- add_page (commit d45af32f)
- rename_page (new — also adds Document::rename_page mutator)
- delete_page (new)
- duplicate_page (new)
Wire shape:
rename_page: { "index": "<u32>", "name": "<non-empty>" }
delete_page: { "index": "<u32>" }
duplicate_page: { "index": "<u32>" }
result: { "wrote": "true" }
New `Document::rename_page(idx, name)` mutator in
page_mutators.rs (peer of start_rename_page which is the UI
inline-rename flow). Rejects out-of-range indices + empty /
whitespace-only names so list_pages never returns an unusable
label.
Apply branches delegate to existing `remove_page` / `duplicate_page`
mutators. Variables table rejects all three Pages-level commands
in its defense-in-depth guard.
Desktop --mcp registry + tools/list schema + exact-count test
updated for 32 tools.
This commit is contained in:
parent
85ea875765
commit
0232329d7b
|
|
@ -27,8 +27,9 @@ use openpencil_shell_core::mcp::{
|
|||
batch_design_snapshot, copy_node_snapshot, delete_node_snapshot,
|
||||
design_content_snapshot, design_refine_snapshot, design_skeleton_snapshot,
|
||||
add_page_snapshot, create_component_snapshot, delete_component_snapshot,
|
||||
document_info_snapshot, get_component_snapshot, instantiate_component_snapshot,
|
||||
list_components_snapshot, rename_component_snapshot, set_active_page_snapshot,
|
||||
delete_page_snapshot, document_info_snapshot, duplicate_page_snapshot,
|
||||
get_component_snapshot, instantiate_component_snapshot, list_components_snapshot,
|
||||
rename_component_snapshot, rename_page_snapshot, set_active_page_snapshot,
|
||||
get_active_theme_snapshot, get_node_snapshot, insert_node_snapshot,
|
||||
list_pages_snapshot, list_variables_snapshot, move_node_snapshot,
|
||||
replace_node_snapshot, run_stdio_with_applier, selection_snapshot,
|
||||
|
|
@ -164,6 +165,9 @@ fn rebuild_registry(doc: &Document) -> ToolRegistry {
|
|||
r.register(Box::new(rename_component_snapshot()));
|
||||
r.register(Box::new(set_active_page_snapshot()));
|
||||
r.register(Box::new(add_page_snapshot()));
|
||||
r.register(Box::new(rename_page_snapshot()));
|
||||
r.register(Box::new(delete_page_snapshot()));
|
||||
r.register(Box::new(duplicate_page_snapshot()));
|
||||
r
|
||||
}
|
||||
|
||||
|
|
@ -393,6 +397,9 @@ const TOOL_SCHEMAS: &[&str] = &[
|
|||
r#"{"name":"rename_component","description":"Rename a registered component. Name must be non-empty / non-whitespace.","inputSchema":{"type":"object","properties":{"component_id":{"type":"string","description":"positive u64 component id"},"name":{"type":"string"}},"required":["component_id","name"]}}"#,
|
||||
r#"{"name":"set_active_page","description":"Switch which page is the active target for subsequent inserts / batch_design / design_* commands. index is 0-based.","inputSchema":{"type":"object","properties":{"index":{"type":"string","description":"0-based page index"}},"required":["index"]}}"#,
|
||||
r#"{"name":"add_page","description":"Append a fresh empty page and switch the active page to it. Returns false on id-space exhaustion.","inputSchema":{"type":"object","properties":{},"additionalProperties":false}}"#,
|
||||
r#"{"name":"rename_page","description":"Set a page's display name. Name must be non-empty / non-whitespace.","inputSchema":{"type":"object","properties":{"index":{"type":"string","description":"0-based page index"},"name":{"type":"string"}},"required":["index","name"]}}"#,
|
||||
r#"{"name":"delete_page","description":"Remove a page by index. The applier keeps the active page valid (clamps if needed).","inputSchema":{"type":"object","properties":{"index":{"type":"string","description":"0-based page index"}},"required":["index"]}}"#,
|
||||
r#"{"name":"duplicate_page","description":"Clone the page at index and switch the active page to the clone.","inputSchema":{"type":"object","properties":{"index":{"type":"string","description":"0-based page index"}},"required":["index"]}}"#,
|
||||
r#"{"name":"set_active_axis_value","description":"Pin a theme axis to one of its allowed values.","inputSchema":{"type":"object","properties":{"axis":{"type":"string"},"value":{"type":"string"}},"required":["axis","value"]}}"#,
|
||||
r#"{"name":"insert_node","description":"Create a new leaf node on the active page.","inputSchema":{"type":"object","properties":{"kind":{"type":"string","enum":["frame","group","rect","ellipse","polygon","line","text","path"]},"name":{"type":"string"},"x":{"type":"string"},"y":{"type":"string"},"width":{"type":"string"},"height":{"type":"string"},"fill_hex":{"type":"string"}},"required":["kind","name","x","y","width","height"]}}"#,
|
||||
r#"{"name":"update_node","description":"Patch fields on an existing node. Pass any subset of x/y/width/height/name/fill_hex.","inputSchema":{"type":"object","properties":{"node_id":{"type":"string"},"x":{"type":"string"},"y":{"type":"string"},"width":{"type":"string"},"height":{"type":"string"},"name":{"type":"string"},"fill_hex":{"type":"string"}},"required":["node_id"]}}"#,
|
||||
|
|
@ -446,7 +453,7 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn tools_list_response_includes_all_twenty_nine_tools() {
|
||||
fn tools_list_response_includes_all_thirty_two_tools() {
|
||||
let r = tools_list_response("3");
|
||||
// Exact-count assertion: any tool added without
|
||||
// updating this test will trip the count first. Codex
|
||||
|
|
@ -455,7 +462,7 @@ mod tests {
|
|||
// without being added to the list below.
|
||||
assert_eq!(
|
||||
TOOL_SCHEMAS.len(),
|
||||
29,
|
||||
32,
|
||||
"tools/list catalog count must match the registered tools — add the new tool to this test"
|
||||
);
|
||||
for name in [
|
||||
|
|
@ -473,6 +480,9 @@ mod tests {
|
|||
"rename_component",
|
||||
"set_active_page",
|
||||
"add_page",
|
||||
"rename_page",
|
||||
"delete_page",
|
||||
"duplicate_page",
|
||||
"set_variable_color",
|
||||
"set_active_axis_value",
|
||||
"insert_node",
|
||||
|
|
|
|||
|
|
@ -510,6 +510,15 @@ impl Document {
|
|||
self.set_active_page(*index as usize)
|
||||
}
|
||||
crate::mcp::McpCommand::AddPage => self.add_page().is_some(),
|
||||
crate::mcp::McpCommand::RenamePage { index, name } => {
|
||||
self.rename_page(*index as usize, name.clone())
|
||||
}
|
||||
crate::mcp::McpCommand::DeletePage { index } => {
|
||||
self.remove_page(*index as usize)
|
||||
}
|
||||
crate::mcp::McpCommand::DuplicatePage { index } => {
|
||||
self.duplicate_page(*index as usize).is_some()
|
||||
}
|
||||
crate::mcp::McpCommand::BatchInsert { items } => {
|
||||
// Validate EVERY descriptor before any mutation.
|
||||
// A single bad entry rejects the entire batch so
|
||||
|
|
|
|||
|
|
@ -88,6 +88,23 @@ impl Document {
|
|||
true
|
||||
}
|
||||
|
||||
/// Set a page's name directly (MCP-friendly entry point —
|
||||
/// the UI rename flow uses `start_rename_page` +
|
||||
/// `rename_append`). Rejects out-of-range indices and empty
|
||||
/// / whitespace-only names so list_pages never returns a
|
||||
/// page label a user can't recognize.
|
||||
pub fn rename_page(&mut self, idx: usize, name: impl Into<String>) -> bool {
|
||||
let name: String = name.into();
|
||||
if name.trim().is_empty() {
|
||||
return false;
|
||||
}
|
||||
let Some(page) = self.pages.get_mut(idx) else {
|
||||
return false;
|
||||
};
|
||||
page.name = name;
|
||||
true
|
||||
}
|
||||
|
||||
/// Start inline rename on a page row.
|
||||
pub fn start_rename_page(&mut self, idx: usize) -> bool {
|
||||
let Some(page) = self.pages.get(idx) else {
|
||||
|
|
|
|||
|
|
@ -199,7 +199,10 @@ impl VariableTable {
|
|||
| crate::mcp::McpCommand::DeleteComponent { .. }
|
||||
| crate::mcp::McpCommand::RenameComponent { .. }
|
||||
| crate::mcp::McpCommand::SetActivePage { .. }
|
||||
| crate::mcp::McpCommand::AddPage => {
|
||||
| crate::mcp::McpCommand::AddPage
|
||||
| crate::mcp::McpCommand::RenamePage { .. }
|
||||
| crate::mcp::McpCommand::DeletePage { .. }
|
||||
| crate::mcp::McpCommand::DuplicatePage { .. } => {
|
||||
// Not VariableTable mutations — Pages-level commands
|
||||
// live on `Document::apply_mcp_command`. Return false
|
||||
// so callers with only a VariableTable handle know
|
||||
|
|
|
|||
|
|
@ -40,9 +40,10 @@ pub use write_tools::{
|
|||
};
|
||||
pub use component_tools::{
|
||||
add_page_snapshot, create_component_snapshot, delete_component_snapshot,
|
||||
instantiate_component_snapshot, rename_component_snapshot, set_active_page_snapshot,
|
||||
AddPage, CreateComponent, DeleteComponent, InstantiateComponent, RenameComponent,
|
||||
SetActivePage,
|
||||
delete_page_snapshot, duplicate_page_snapshot, instantiate_component_snapshot,
|
||||
rename_component_snapshot, rename_page_snapshot, set_active_page_snapshot, AddPage,
|
||||
CreateComponent, DeleteComponent, DeletePage, DuplicatePage, InstantiateComponent,
|
||||
RenameComponent, RenamePage, SetActivePage,
|
||||
};
|
||||
pub use batch_design::{
|
||||
batch_design_snapshot, design_content_snapshot, design_refine_snapshot,
|
||||
|
|
@ -299,6 +300,23 @@ pub enum McpCommand {
|
|||
/// id-space exhaustion (same guard as the rest of the write
|
||||
/// tools).
|
||||
AddPage,
|
||||
/// Set a page's display name. Rejects out-of-range indices
|
||||
/// and empty / whitespace-only names.
|
||||
RenamePage {
|
||||
index: u32,
|
||||
name: String,
|
||||
},
|
||||
/// Remove a page by index. The applier reuses the existing
|
||||
/// `Document::remove_page` which keeps the active page valid.
|
||||
DeletePage {
|
||||
index: u32,
|
||||
},
|
||||
/// Duplicate the page at `index` (clone + insert right after).
|
||||
/// Switches active page to the clone. Mirrors TS
|
||||
/// `duplicatePage(idx)`.
|
||||
DuplicatePage {
|
||||
index: u32,
|
||||
},
|
||||
}
|
||||
|
||||
/// Wire-friendly value payload for `McpCommand::SetVariableScalar`.
|
||||
|
|
|
|||
|
|
@ -239,3 +239,122 @@ pub fn add_page_snapshot() -> AddPage {
|
|||
AddPage
|
||||
}
|
||||
|
||||
/// First-party `rename_page` tool — set a page's display name.
|
||||
/// Rejects out-of-range indices + empty / whitespace-only names.
|
||||
pub struct RenamePage;
|
||||
|
||||
impl McpTool for RenamePage {
|
||||
fn name(&self) -> &str {
|
||||
"rename_page"
|
||||
}
|
||||
fn call(&self, args: &BTreeMap<String, String>) -> ToolOutcome {
|
||||
let Some(raw) = args.get("index") else {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::MissingArgument,
|
||||
"index is required (0-based page index)".into(),
|
||||
);
|
||||
};
|
||||
let index: u32 = match raw.parse() {
|
||||
Ok(n) => n,
|
||||
_ => {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::InvalidArgument,
|
||||
format!("index must be a u32, got {raw:?}"),
|
||||
);
|
||||
}
|
||||
};
|
||||
let Some(name) = args.get("name") else {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::MissingArgument,
|
||||
"name is required".into(),
|
||||
);
|
||||
};
|
||||
if name.trim().is_empty() {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::InvalidArgument,
|
||||
"name must not be empty / whitespace-only".into(),
|
||||
);
|
||||
}
|
||||
let mut out = BTreeMap::new();
|
||||
out.insert("wrote".into(), "true".into());
|
||||
ToolOutcome::OkWithCommand(
|
||||
out,
|
||||
McpCommand::RenamePage {
|
||||
index,
|
||||
name: name.clone(),
|
||||
},
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
pub fn rename_page_snapshot() -> RenamePage {
|
||||
RenamePage
|
||||
}
|
||||
|
||||
/// First-party `delete_page` tool — remove a page by index.
|
||||
pub struct DeletePage;
|
||||
|
||||
impl McpTool for DeletePage {
|
||||
fn name(&self) -> &str {
|
||||
"delete_page"
|
||||
}
|
||||
fn call(&self, args: &BTreeMap<String, String>) -> ToolOutcome {
|
||||
let Some(raw) = args.get("index") else {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::MissingArgument,
|
||||
"index is required (0-based page index)".into(),
|
||||
);
|
||||
};
|
||||
let index: u32 = match raw.parse() {
|
||||
Ok(n) => n,
|
||||
_ => {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::InvalidArgument,
|
||||
format!("index must be a u32, got {raw:?}"),
|
||||
);
|
||||
}
|
||||
};
|
||||
let mut out = BTreeMap::new();
|
||||
out.insert("wrote".into(), "true".into());
|
||||
ToolOutcome::OkWithCommand(out, McpCommand::DeletePage { index })
|
||||
}
|
||||
}
|
||||
|
||||
pub fn delete_page_snapshot() -> DeletePage {
|
||||
DeletePage
|
||||
}
|
||||
|
||||
/// First-party `duplicate_page` tool — clone the page at `index`
|
||||
/// and switch the active page to the clone.
|
||||
pub struct DuplicatePage;
|
||||
|
||||
impl McpTool for DuplicatePage {
|
||||
fn name(&self) -> &str {
|
||||
"duplicate_page"
|
||||
}
|
||||
fn call(&self, args: &BTreeMap<String, String>) -> ToolOutcome {
|
||||
let Some(raw) = args.get("index") else {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::MissingArgument,
|
||||
"index is required (0-based page index)".into(),
|
||||
);
|
||||
};
|
||||
let index: u32 = match raw.parse() {
|
||||
Ok(n) => n,
|
||||
_ => {
|
||||
return ToolOutcome::Err(
|
||||
ToolErrorCode::InvalidArgument,
|
||||
format!("index must be a u32, got {raw:?}"),
|
||||
);
|
||||
}
|
||||
};
|
||||
let mut out = BTreeMap::new();
|
||||
out.insert("wrote".into(), "true".into());
|
||||
ToolOutcome::OkWithCommand(out, McpCommand::DuplicatePage { index })
|
||||
}
|
||||
}
|
||||
|
||||
pub fn duplicate_page_snapshot() -> DuplicatePage {
|
||||
DuplicatePage
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue