diff --git a/crates/op-host-services/src/design_agent_tools.rs b/crates/op-host-services/src/design_agent_tools.rs index 2a8e9ea24..056d65512 100644 --- a/crates/op-host-services/src/design_agent_tools.rs +++ b/crates/op-host-services/src/design_agent_tools.rs @@ -1,23 +1,34 @@ //! In-process design tool surface for the AI design agent loop. //! -//! Mirrors `chat_canvas_tools.rs` for the 14-tool design toolset (vs the -//! 7-tool CRUD set). Schema definitions are derived from -//! `mcp_serve::schemas::TOOL_SCHEMAS` — the same source the MCP server -//! advertises — so the in-process and MCP surfaces stay byte-equal as JSON. +//! Mirrors `chat_canvas_tools.rs` for the 15-tool design toolset (vs the +//! 7-tool CRUD set). For the 14 MCP-shared tools, schema definitions are +//! derived from `mcp_serve::schemas::TOOL_SCHEMAS` — the same source the MCP +//! server advertises — so the in-process and MCP surfaces stay byte-equal as +//! JSON. //! -//! This module provides the defs + registry + executor that Task 2.3 will -//! wire into the design agent tool-loop. It does NOT touch `op-orchestrator` -//! and does NOT wire routing (that is Task 2.3). +//! The 15th tool, `emit_elements`, is LOOP-ONLY: it gives the loop the +//! element-builder surface (the same builders the orchestrator's MANIFEST path +//! uses, via `crate::emit_elements::EmitElements` → +//! `op_orchestrator::manifest::parse_manifest`). Its schema is self-contained +//! in [`crate::emit_elements::EMIT_ELEMENTS_SCHEMA`] rather than `TOOL_SCHEMAS`, +//! so it never enters the MCP server's advertised catalog. use op_ai::chat_provider::{ChatToolDef, ChatToolResult}; use op_editor_core::EditorState; use op_mcp::ToolRegistry; use crate::chat_canvas_tools::{execute_chat_tool, execute_with_registry}; +use crate::emit_elements::{EmitElements, EMIT_ELEMENTS_SCHEMA, EMIT_ELEMENTS_TOOL}; use crate::mcp_serve::schemas; -/// The 14-tool design toolset with auth levels. -/// Reads = "read"; batch_design / set_variables / spawn_agents / export_nodes = "create". +/// The 15-tool design toolset with auth levels. +/// Reads = "read"; batch_design / emit_elements / set_variables / spawn_agents +/// / export_nodes = "create". +/// +/// `emit_elements` is the loop's element-builder surface (the same builders the +/// orchestrator's MANIFEST path uses). It is LOOP-ONLY — its schema comes from +/// [`EMIT_ELEMENTS_SCHEMA`], NOT from `mcp_serve::schemas::TOOL_SCHEMAS`, so the +/// MCP server's advertised catalog is unchanged. pub const DESIGN_TOOLS: &[(&str, &str)] = &[ ("get_editor_state", "read"), ("get_guidelines", "read"), @@ -29,6 +40,7 @@ pub const DESIGN_TOOLS: &[(&str, &str)] = &[ ("snapshot_layout", "read"), ("find_empty_space", "read"), ("batch_design", "create"), + (EMIT_ELEMENTS_TOOL, "create"), ("get_screenshot", "read"), ("export_nodes", "create"), ("spawn_agents", "create"), @@ -50,8 +62,17 @@ pub fn design_tool_defs() -> Vec { DESIGN_TOOLS .iter() .map(|(name, _)| { - let (description, input_schema_json) = extract_from_schemas(name) - .unwrap_or_else(|| panic!("design tool {name} not found in TOOL_SCHEMAS")); + // `emit_elements` is loop-only: its schema lives beside the tool + // (EMIT_ELEMENTS_SCHEMA), NOT in TOOL_SCHEMAS, so it never enters + // the MCP server's advertised catalog. Every other design tool is + // sourced from TOOL_SCHEMAS for byte-equal MCP parity. + let (description, input_schema_json) = if *name == EMIT_ELEMENTS_TOOL { + extract_from_schema_entry(EMIT_ELEMENTS_SCHEMA) + .expect("EMIT_ELEMENTS_SCHEMA must be a valid tool descriptor") + } else { + extract_from_schemas(name) + .unwrap_or_else(|| panic!("design tool {name} not found in TOOL_SCHEMAS")) + }; ChatToolDef { name: name.to_string(), description, @@ -129,6 +150,7 @@ fn design_tool_registry(state: &EditorState, requested: &str) -> ToolRegistry { "get_screenshot" => r.register(Box::new(get_screenshot_snapshot(state))), "export_nodes" => r.register(Box::new(export_nodes_snapshot(state))), "spawn_agents" => r.register(Box::new(op_mcp::spawn_agents_snapshot())), + EMIT_ELEMENTS_TOOL => r.register(Box::new(EmitElements)), "ToolSearch" => r.register(Box::new(op_mcp::tool_search_snapshot( schemas::TOOL_SCHEMAS, ))), @@ -147,25 +169,32 @@ fn extract_from_schemas(name: &str) -> Option<(String, String)> { for entry in schemas::TOOL_SCHEMAS { let v: serde_json::Value = serde_json::from_str(entry).ok()?; if v.get("name").and_then(|n| n.as_str()) == Some(name) { - let description = v.get("description")?.as_str()?.to_string(); - let input_schema = v.get("inputSchema")?.clone(); - let input_schema_json = input_schema.to_string(); - return Some((description, input_schema_json)); + return extract_from_schema_entry(entry); } } None } +/// Parse one tool descriptor JSON string into `(description, inputSchema)`. +/// Used for `TOOL_SCHEMAS` entries and the loop-only `EMIT_ELEMENTS_SCHEMA`. +fn extract_from_schema_entry(entry: &str) -> Option<(String, String)> { + let v: serde_json::Value = serde_json::from_str(entry).ok()?; + let description = v.get("description")?.as_str()?.to_string(); + let input_schema = v.get("inputSchema")?.clone(); + Some((description, input_schema.to_string())) +} + #[cfg(test)] mod tests { use super::*; #[test] - fn design_tool_defs_cover_all_14_tools_with_schema_parity() { + fn design_tool_defs_cover_all_15_tools_with_schema_parity() { let defs = design_tool_defs(); - // All 14 tools are present. - assert_eq!(defs.len(), 14, "expected 14 design tool defs"); + // All 15 tools are present (14 MCP-sourced + the loop-only + // `emit_elements`). + assert_eq!(defs.len(), 15, "expected 15 design tool defs"); for (name, _) in DESIGN_TOOLS { assert!( defs.iter().any(|d| d.name == *name), @@ -173,10 +202,12 @@ mod tests { ); } - // PARITY: for each tool, the input_schema_json in the def must - // equal the inputSchema value from TOOL_SCHEMAS (as parsed JSON). - // This ensures in-process defs stay byte-equal to the MCP server. - for def in &defs { + // PARITY: for each MCP-sourced tool, the input_schema_json in the def + // must equal the inputSchema value from TOOL_SCHEMAS (as parsed JSON), + // so in-process defs stay byte-equal to the MCP server. `emit_elements` + // is loop-only and intentionally NOT in TOOL_SCHEMAS — it is parity- + // checked separately below against EMIT_ELEMENTS_SCHEMA. + for def in defs.iter().filter(|d| d.name != EMIT_ELEMENTS_TOOL) { // Find the matching TOOL_SCHEMAS entry. let schema_entry = schemas::TOOL_SCHEMAS .iter() @@ -205,14 +236,38 @@ mod tests { ); } - // Every DESIGN_TOOLS entry must exist in TOOL_SCHEMAS (no orphans). - for (name, _) in DESIGN_TOOLS { + // Every DESIGN_TOOLS entry except the loop-only `emit_elements` must + // exist in TOOL_SCHEMAS (no orphans). `emit_elements` is deliberately + // absent so it never enters the MCP server's advertised catalog. + for (name, _) in DESIGN_TOOLS + .iter() + .filter(|(n, _)| *n != EMIT_ELEMENTS_TOOL) + { let found = schemas::TOOL_SCHEMAS.iter().any(|entry| { let v: serde_json::Value = serde_json::from_str(entry).unwrap(); v.get("name").and_then(|n| n.as_str()) == Some(*name) }); assert!(found, "design tool {name} is not in TOOL_SCHEMAS — orphan!"); } + + // `emit_elements` is loop-only: it must NOT be advertised by the MCP + // server, and its def must equal EMIT_ELEMENTS_SCHEMA. + assert!( + !schemas::TOOL_SCHEMAS.iter().any(|entry| { + let v: serde_json::Value = serde_json::from_str(entry).unwrap(); + v.get("name").and_then(|n| n.as_str()) == Some(EMIT_ELEMENTS_TOOL) + }), + "emit_elements must stay OUT of the MCP server's TOOL_SCHEMAS catalog" + ); + let emit_def = defs + .iter() + .find(|d| d.name == EMIT_ELEMENTS_TOOL) + .expect("emit_elements def present"); + let canonical: serde_json::Value = serde_json::from_str(EMIT_ELEMENTS_SCHEMA).unwrap(); + let def_schema: serde_json::Value = + serde_json::from_str(&emit_def.input_schema_json).unwrap(); + assert_eq!(def_schema, canonical["inputSchema"]); + assert_eq!(emit_def.level, "create"); } #[test] diff --git a/crates/op-host-services/src/emit_elements.rs b/crates/op-host-services/src/emit_elements.rs new file mode 100644 index 000000000..0606985ef --- /dev/null +++ b/crates/op-host-services/src/emit_elements.rs @@ -0,0 +1,316 @@ +//! `emit_elements` — the design loop's element-builder tool. +//! +//! ## Why this exists +//! +//! Data-proven (ab-v8/ab-v9): the orchestrator beats the bare agentic loop +//! on weak models (≈50% vs 0% on M3) NOT because of its planner/scaffold, +//! but because its MANIFEST mode lets the model emit high-level +//! `{"el":""}` element lines that Rust element-builders expand into +//! role-tagged subtrees (`stat-card`, `profile-header`, …). The loop only had +//! raw `batch_design` (primitive frames/text, no element kinds, no roles) → 0%. +//! +//! `emit_elements` closes that gap by giving the loop the SAME element-builder +//! surface the orchestrator's MANIFEST path uses — without exposing the 188 +//! `add_*_v0/v1` element tools (too heavy for a loop prompt's tool list). +//! +//! ## Reuse, not reimplementation +//! +//! The tool runs `op_orchestrator::manifest::parse_manifest` — the EXACT +//! assembler the orchestrator MANIFEST mode calls. `parse_manifest` invokes +//! `op_mcp::element_manifest::build_element` per `el` line (semantic builders + +//! the repairing argument layer + `el:"ref"` component instances), then +//! assembles the el-line forest by `in:` references into role-tagged +//! `PenNode`s. So `emit_elements`' output is byte-equivalent in shape to the +//! orchestrator's manifest path: same builders, same role tags, same +//! post-processing surface. The result rides `EditorCommand::InsertSubtree`, +//! the same command `batch_design` emits. +//! +//! ## Gating +//! +//! This tool is wired ONLY into the design agent loop's tool set +//! (`design_agent_tools::DESIGN_TOOLS`). It is NOT registered with the MCP +//! server (`TOOL_SCHEMAS` is untouched), so the external MCP surface and its +//! advertised catalog are unchanged. + +use std::collections::BTreeMap; + +use op_editor_core::{EditorCommand, NodeId}; +use op_mcp::{McpTool, ToolErrorCode, ToolOutcome}; +use serde_json::Value; + +/// Loop-only tool name. +pub const EMIT_ELEMENTS_TOOL: &str = "emit_elements"; + +/// The `emit_elements` tool schema, kept self-contained here (NOT sourced from +/// `mcp_serve::schemas::TOOL_SCHEMAS`) precisely so the tool stays loop-only and +/// the MCP server's advertised catalog count is unaffected. +pub const EMIT_ELEMENTS_SCHEMA: &str = r#"{"name":"emit_elements","description":"PREFERRED design tool. Emit a high-level element manifest — a JSON array of element lines like {\"el\":\"stat_card\",\"label\":\"MRR\",\"value\":\"$48k\"} — and the host expands each into a polished, role-tagged subtree (stat-card, profile-header, nav-item, …) and inserts it. Use {\"el\":\"section\",\"role\":\"hero\",\"direction\":\"vertical\",\"gap\":16} as a 1-based container, then nest later lines into it with \"in\": . NEVER write ids; nesting is by \"in\" only. Prefer this over hand-building primitives with batch_design.","inputSchema":{"type":"object","properties":{"elements":{"type":"string","description":"JSON array of element-line objects. Each object has an \"el\" kind plus that kind's params; \"section\" lines are containers other lines nest under via \"in\". e.g. [{\"el\":\"section\",\"role\":\"stats\",\"direction\":\"horizontal\",\"gap\":16},{\"el\":\"stat_card\",\"in\":1,\"label\":\"MRR\",\"value\":\"$48k\",\"trend\":\"up\"}]"},"parent_id":{"type":"string","description":"Optional existing container node id to insert under; omit/empty/0/root = active page root."}},"required":["elements"]}}"#; + +/// The `emit_elements` `McpTool`. Stateless: it depends only on its args (the +/// manifest assembler is pure), so it carries no snapshot — `parent_id` is a +/// caller-supplied existing node id, resolved by the host's `InsertSubtree` +/// apply against the live document. +pub struct EmitElements; + +impl McpTool for EmitElements { + fn name(&self) -> &str { + EMIT_ELEMENTS_TOOL + } + + fn call(&self, args: &BTreeMap) -> ToolOutcome { + let Some(raw) = args + .get("elements") + .map(|s| s.trim()) + .filter(|s| !s.is_empty()) + else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "elements is required: a JSON array of element-line objects".into(), + ); + }; + + // Parse the array of el-line objects, then re-serialize each object on + // its own line — `parse_manifest` scans balanced `{…}` spans, so a + // newline-joined JSONL feed is what it expects. + let value: Value = match serde_json::from_str(raw) { + Ok(v) => v, + Err(e) => { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!("elements must be a JSON array of objects: {e}"), + ) + } + }; + let Value::Array(items) = value else { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + "elements must be a JSON array (e.g. [{\"el\":\"stat_card\",...}])".into(), + ); + }; + if items.is_empty() { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + "elements must contain at least one element line".into(), + ); + } + let mut jsonl = String::new(); + for item in &items { + if !item.is_object() { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + "every elements entry must be an object with an \"el\" kind".into(), + ); + } + // Compact form keeps each object on one line so `balanced_spans` + // sees exactly one span per element line, matching the line-number + // semantics `in:` references rely on. + jsonl.push_str(&item.to_string()); + jsonl.push('\n'); + } + + // Run the SAME assembler the orchestrator MANIFEST path uses: + // el lines → `op_mcp::element_manifest::build_element` per line → + // role-tagged `PenNode` forest assembled by `in:` references. + let Some(outcome) = op_orchestrator::manifest::parse_manifest(&jsonl) else { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + "no element lines found: every entry needs an \"el\" kind (e.g. \"stat_card\", \"section\")".into(), + ); + }; + if outcome.nodes.is_empty() { + return ToolOutcome::Err( + ToolErrorCode::InvalidArgument, + format!( + "element manifest produced no nodes ({} dropped). warnings: {}", + outcome.dropped_lines, + outcome.warnings.join("; ") + ), + ); + } + + let parent_id = parse_parent_id(args.get("parent_id")); + let mut result = BTreeMap::new(); + result.insert("wrote".into(), "true".into()); + result.insert("count".into(), count_forest(&outcome.nodes).to_string()); + result.insert("elementLines".into(), outcome.element_lines.to_string()); + result.insert("rawNodeLines".into(), outcome.raw_node_lines.to_string()); + result.insert("droppedLines".into(), outcome.dropped_lines.to_string()); + if !outcome.warnings.is_empty() { + // Surface the repair/degrade trail so weak-model misses stay + // observable in the loop's tool-result stream. + result.insert("warnings".into(), outcome.warnings.join("; ")); + } + + ToolOutcome::OkWithCommand( + result, + EditorCommand::InsertSubtree { + nodes: outcome.nodes, + parent_id, + page_id: None, + }, + ) + } +} + +/// Resolve the optional `parent_id` arg: empty / `0` / `root` / `document` / +/// `null` all mean the active page root (`NodeId::NONE`). +fn parse_parent_id(raw: Option<&String>) -> NodeId { + match raw.map(|s| s.trim()) { + None | Some("") | Some("0") | Some("root") | Some("document") | Some("null") => { + NodeId::NONE + } + Some(id) => NodeId::new(id), + } +} + +/// Count every node in a forest (subtree-inclusive). +fn count_forest(nodes: &[jian_ops_schema::node::PenNode]) -> usize { + fn count_subtree(node: &jian_ops_schema::node::PenNode) -> usize { + use op_editor_core::PenNodeExt; + 1 + node + .children() + .map(|c| c.iter().map(count_subtree).sum::()) + .unwrap_or(0) + } + nodes.iter().map(count_subtree).sum() +} + +#[cfg(test)] +mod tests { + use super::*; + use jian_ops_schema::node::PenNode; + use op_editor_core::PenNodeExt; + + fn args(pairs: &[(&str, &str)]) -> BTreeMap { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() + } + + /// Recursively collect every node `role` (`base.role`) in a forest. The + /// element builders stamp semantic roles the orchestrator's MANIFEST path + /// relies on; `emit_elements` must produce the same. + fn collect_roles(node: &PenNode, out: &mut Vec) { + if let Some(role) = node.base().role.as_deref() { + if !role.is_empty() { + out.push(role.to_string()); + } + } + if let Some(children) = node.children() { + for child in children { + collect_roles(child, out); + } + } + } + + fn all_roles(nodes: &[PenNode]) -> Vec { + let mut out = Vec::new(); + for node in nodes { + collect_roles(node, &mut out); + } + out + } + + #[test] + fn emit_elements_builds_role_tagged_nodes() { + // A couple of el lines (the task's example) must expand into the SAME + // role-tagged structure the orchestrator MANIFEST path produces — + // proving the loop reuses the element builders, not raw primitives. + let tool = EmitElements; + let elements = r#"[{"el":"stat_card","label":"MRR","value":"$48.2k","trend":"up"},{"el":"stat_card","label":"Users","value":"12.4k"}]"#; + let outcome = tool.call(&args(&[("elements", elements)])); + let (result, command) = match outcome { + ToolOutcome::OkWithCommand(result, command) => (result, command), + other => panic!("expected OkWithCommand, got {other:?}"), + }; + assert_eq!(result.get("elementLines").map(String::as_str), Some("2")); + assert_eq!(result.get("droppedLines").map(String::as_str), Some("0")); + + let nodes = match command { + EditorCommand::InsertSubtree { nodes, .. } => nodes, + other => panic!("expected InsertSubtree, got {other:?}"), + }; + assert_eq!(nodes.len(), 2, "two top-level stat cards"); + let roles = all_roles(&nodes); + assert!( + roles.iter().any(|r| r == "stat-card"), + "stat-card role must be present (proves element-builder expansion), got {roles:?}" + ); + } + + #[test] + fn emit_elements_nests_under_a_section_via_in() { + // `{"el":"section",...}` is line 1; later lines nest via `in:1`. + let tool = EmitElements; + let elements = r#"[{"el":"section","role":"stats","direction":"horizontal","gap":16},{"el":"stat_card","in":1,"label":"MRR","value":"$48k"},{"el":"stat_card","in":1,"label":"DAU","value":"3.1k"}]"#; + let outcome = tool.call(&args(&[("elements", elements)])); + let command = match outcome { + ToolOutcome::OkWithCommand(_, command) => command, + other => panic!("expected OkWithCommand, got {other:?}"), + }; + let nodes = match command { + EditorCommand::InsertSubtree { nodes, .. } => nodes, + other => panic!("expected InsertSubtree, got {other:?}"), + }; + assert_eq!(nodes.len(), 1, "one section root"); + let section_kids = nodes[0].children().map(|c| c.len()).unwrap_or(0); + assert_eq!(section_kids, 2, "both stat cards nested in the section"); + } + + #[test] + fn emit_elements_resolves_parent_id_to_active_root() { + let tool = EmitElements; + let elements = r#"[{"el":"badge","label":"New"}]"#; + // Empty / sentinel parent_id values resolve to the active page root. + for sentinel in ["", "0", "root", "document"] { + let outcome = tool.call(&args(&[("elements", elements), ("parent_id", sentinel)])); + match outcome { + ToolOutcome::OkWithCommand(_, EditorCommand::InsertSubtree { parent_id, .. }) => { + assert_eq!(parent_id, NodeId::NONE, "{sentinel:?} → active page root"); + } + other => panic!("expected InsertSubtree for {sentinel:?}, got {other:?}"), + } + } + // A real id is preserved as the insert target. + let outcome = tool.call(&args(&[("elements", elements), ("parent_id", "42")])); + match outcome { + ToolOutcome::OkWithCommand(_, EditorCommand::InsertSubtree { parent_id, .. }) => { + assert_eq!(parent_id, NodeId::new("42")); + } + other => panic!("expected InsertSubtree, got {other:?}"), + } + } + + #[test] + fn emit_elements_rejects_missing_or_malformed_elements() { + let tool = EmitElements; + assert!(matches!( + tool.call(&BTreeMap::new()), + ToolOutcome::Err(ToolErrorCode::MissingArgument, _) + )); + assert!(matches!( + tool.call(&args(&[("elements", "not json")])), + ToolOutcome::Err(ToolErrorCode::InvalidArgument, _) + )); + assert!(matches!( + tool.call(&args(&[("elements", "{\"el\":\"badge\"}")])), + ToolOutcome::Err(ToolErrorCode::InvalidArgument, _) // object, not array + )); + assert!(matches!( + tool.call(&args(&[("elements", "[]")])), + ToolOutcome::Err(ToolErrorCode::InvalidArgument, _) + )); + } + + #[test] + fn emit_elements_schema_is_valid_json_with_required_elements() { + let schema: Value = + serde_json::from_str(EMIT_ELEMENTS_SCHEMA).expect("schema must be valid JSON"); + assert_eq!(schema["name"], "emit_elements"); + let required = schema["inputSchema"]["required"] + .as_array() + .expect("required array"); + assert!(required.iter().any(|v| v == "elements")); + } +} diff --git a/crates/op-host-services/src/lib.rs b/crates/op-host-services/src/lib.rs index ef2935e8c..f454e2981 100644 --- a/crates/op-host-services/src/lib.rs +++ b/crates/op-host-services/src/lib.rs @@ -38,6 +38,7 @@ pub mod design_agent_tools; pub mod design_md_llm; pub mod design_session; pub mod doc_io; +pub mod emit_elements; pub mod export; pub mod export_pdf; pub mod mcp_live;