From e51600f068a135f3c70a347f69ba402e654aa607 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 20:26:55 +0800 Subject: [PATCH] =?UTF-8?q?feat(mcp):=20set=5Factive=5Faxis=5Fvalue=20tool?= =?UTF-8?q?=20=E2=80=94=20second=20write?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second MCP write tool, completing the variable + theme write surface. Mirrors `cycle_active_axis_value` (which the VariablesPanel chip click drives) but PINS the value rather than cycles — LLM clients use it when they want to land on a specific axis state ("switch to dark mode") instead of stepping through options ("flip to whatever's next"). `crates/openpencil-shell-core/src/mcp/tools.rs`: - `SetActiveAxisValue { axes: BTreeMap> }` snapshot. Mirrors `var_table.themes` keyed by axis name. - `set_active_axis_value_snapshot(doc)` factory. - `McpTool::call` validates: - Both `axis` and `value` args present (`MissingArgument` on omission). - Axis exists in the themes table (`ToolFailed`). - Value is in `themes[axis].values` (`InvalidArgument`, error message lists allowed values). - Returns `OkWithCommand(SetActiveAxisValue { axis, value })`. The McpCommand variant has existed since the write arch landed (0f09671a); `Document::apply_mcp_command` already routes it to `var_table.active_theme.insert`. - Re-validates at apply time so a stale snapshot can't slip an unauthorized value through (host applier rejects with `false`, which the stdio path demotes to Internal). Tests (5 added, 319 shell-core total): - `set_active_axis_value_validates_args_and_returns_command` — happy path; OkWithCommand variant + correct payload. - `set_active_axis_value_errors_on_missing_args` — both args required, message names the missing one. - `set_active_axis_value_errors_on_unknown_axis` — ToolFailed + error names the axis. - `set_active_axis_value_errors_on_value_not_in_axis` — InvalidArgument; error message lists every allowed value. - `apply_mcp_command_routes_set_active_axis_value` — end-to-end happy path + the stale-state rejection branch (invalid value at apply time returns `false`). MCP write tool surface now: - set_variable_color (Color hex through var_table) - set_active_axis_value (theme axis through active_theme map) - insert_node, update_node_position, etc. land later — each extends `McpCommand` + the existing applier pattern. --- crates/openpencil-shell-core/src/mcp.rs | 6 +- crates/openpencil-shell-core/src/mcp/tools.rs | 77 +++++++++++++ .../src/mcp/tools_tests.rs | 102 ++++++++++++++++++ 3 files changed, 182 insertions(+), 3 deletions(-) diff --git a/crates/openpencil-shell-core/src/mcp.rs b/crates/openpencil-shell-core/src/mcp.rs index 1ca53bf06..7078cf623 100644 --- a/crates/openpencil-shell-core/src/mcp.rs +++ b/crates/openpencil-shell-core/src/mcp.rs @@ -18,9 +18,9 @@ pub mod tools; 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_variable_color_snapshot, GetActiveTheme, - GetDocumentInfo, GetNode, GetSelection, ListPages, ListVariables, NodeRecord, - SetVariableColor, VariableRecord, + list_variables_snapshot, selection_snapshot, set_active_axis_value_snapshot, + set_variable_color_snapshot, GetActiveTheme, GetDocumentInfo, GetNode, GetSelection, + ListPages, ListVariables, NodeRecord, SetActiveAxisValue, SetVariableColor, VariableRecord, }; /// JSON-RPC-style request id. Strings + integers both supported by diff --git a/crates/openpencil-shell-core/src/mcp/tools.rs b/crates/openpencil-shell-core/src/mcp/tools.rs index 23f402b0c..2ba276cd5 100644 --- a/crates/openpencil-shell-core/src/mcp/tools.rs +++ b/crates/openpencil-shell-core/src/mcp/tools.rs @@ -568,6 +568,83 @@ 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 }` +pub struct SetActiveAxisValue { + /// Snapshot of axis → allowed-values, mirroring + /// `VariableTable::themes`. Validation only — the host re-runs + /// the same check at apply time so a stale snapshot can't slip + /// an unauthorized value through. + pub axes: BTreeMap>, +} + +impl McpTool for SetActiveAxisValue { + fn name(&self) -> &str { + "set_active_axis_value" + } + fn call(&self, args: &BTreeMap) -> ToolOutcome { + let Some(axis) = args.get("axis") else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "axis is required".into(), + ); + }; + let Some(value) = args.get("value") else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "value is required".into(), + ); + }; + let Some(allowed) = self.axes.get(axis) else { + return ToolOutcome::Err( + ToolErrorCode::ToolFailed, + format!("axis {axis:?} not defined in themes"), + ); + }; + if !allowed.iter().any(|v| v == value) { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!( + "value {value:?} not in axis {axis:?}; allowed: {}", + allowed.join(", ") + ), + ); + } + let mut out = BTreeMap::new(); + out.insert("wrote".into(), "true".into()); + ToolOutcome::OkWithCommand( + out, + McpCommand::SetActiveAxisValue { + axis: axis.clone(), + value: value.clone(), + }, + ) + } +} + +pub fn set_active_axis_value_snapshot( + doc: &crate::document::Document, +) -> SetActiveAxisValue { + let axes = doc + .var_table + .themes + .iter() + .map(|t| (t.name.clone(), t.values.clone())) + .collect(); + SetActiveAxisValue { axes } +} + pub fn set_variable_color_snapshot( doc: &crate::document::Document, ) -> SetVariableColor { diff --git a/crates/openpencil-shell-core/src/mcp/tools_tests.rs b/crates/openpencil-shell-core/src/mcp/tools_tests.rs index e23d62fa6..1d457f524 100644 --- a/crates/openpencil-shell-core/src/mcp/tools_tests.rs +++ b/crates/openpencil-shell-core/src/mcp/tools_tests.rs @@ -477,3 +477,105 @@ fn apply_mcp_command_routes_set_variable_color_to_var_table() { assert!((c.g - 0xcc as f32 / 255.0).abs() < 0.01); assert!((c.b - 0xaa as f32 / 255.0).abs() < 0.01); } + +fn doc_with_theme_axis(name: &str, values: &[&str]) -> crate::document::Document { + use crate::document::{Document, ThemeAxis}; + let mut doc = Document::empty(); + doc.var_table.themes.push(ThemeAxis { + name: name.into(), + values: values.iter().map(|s| s.to_string()).collect(), + }); + doc +} + +#[test] +fn set_active_axis_value_validates_args_and_returns_command() { + let doc = doc_with_theme_axis("mode", &["light", "dark"]); + let tool = set_active_axis_value_snapshot(&doc); + let mut args = BTreeMap::new(); + args.insert("axis".into(), "mode".into()); + args.insert("value".into(), "dark".into()); + match tool.call(&args) { + ToolOutcome::OkWithCommand(out, cmd) => { + assert_eq!(out.get("wrote"), Some(&"true".to_string())); + match cmd { + McpCommand::SetActiveAxisValue { axis, value } => { + assert_eq!(axis, "mode"); + assert_eq!(value, "dark"); + } + other => panic!("expected SetActiveAxisValue, got {other:?}"), + } + } + other => panic!("expected OkWithCommand, got {other:?}"), + } +} + +#[test] +fn set_active_axis_value_errors_on_missing_args() { + let doc = doc_with_theme_axis("mode", &["light", "dark"]); + let tool = set_active_axis_value_snapshot(&doc); + match tool.call(&BTreeMap::new()) { + ToolOutcome::Err(code, _) => assert_eq!(code, ToolErrorCode::MissingArgument), + _ => panic!(), + } + let mut args = BTreeMap::new(); + args.insert("axis".into(), "mode".into()); + match tool.call(&args) { + ToolOutcome::Err(code, msg) => { + assert_eq!(code, ToolErrorCode::MissingArgument); + assert!(msg.contains("value")); + } + _ => panic!(), + } +} + +#[test] +fn set_active_axis_value_errors_on_unknown_axis() { + let doc = doc_with_theme_axis("mode", &["light", "dark"]); + let tool = set_active_axis_value_snapshot(&doc); + let mut args = BTreeMap::new(); + args.insert("axis".into(), "density".into()); + args.insert("value".into(), "compact".into()); + match tool.call(&args) { + ToolOutcome::Err(code, msg) => { + assert_eq!(code, ToolErrorCode::ToolFailed); + assert!(msg.contains("density")); + } + _ => panic!(), + } +} + +#[test] +fn set_active_axis_value_errors_on_value_not_in_axis() { + let doc = doc_with_theme_axis("mode", &["light", "dark"]); + let tool = set_active_axis_value_snapshot(&doc); + let mut args = BTreeMap::new(); + args.insert("axis".into(), "mode".into()); + args.insert("value".into(), "sepia".into()); + match tool.call(&args) { + ToolOutcome::Err(code, msg) => { + assert_eq!(code, ToolErrorCode::InvalidArgument); + assert!(msg.contains("light")); + assert!(msg.contains("dark")); + } + _ => panic!(), + } +} + +#[test] +fn apply_mcp_command_routes_set_active_axis_value() { + let mut doc = doc_with_theme_axis("mode", &["light", "dark"]); + let cmd = McpCommand::SetActiveAxisValue { + axis: "mode".into(), + value: "dark".into(), + }; + assert!(doc.var_table.apply_mcp_command(&cmd)); + assert_eq!(doc.var_table.active_theme.get("mode"), Some(&"dark".to_string())); + + // Invalid value rejected at apply time too (host re-validates). + let bad = McpCommand::SetActiveAxisValue { + axis: "mode".into(), + value: "sepia".into(), + }; + assert!(!doc.var_table.apply_mcp_command(&bad)); +}