From db93a2ac742fd1e25b9f3ee6fb892b1c4edbcd7d Mon Sep 17 00:00:00 2001 From: ganondev Date: Sun, 9 Aug 2026 18:16:27 -0400 Subject: [PATCH] fix(mcp): return get_screenshot as MCP ImageContent block (#205) The get_screenshot MCP tool serialized its PNG through OkJson, which wraps the payload in a text content block. Vision-capable MCP clients (Copilot, Claude Code) therefore received a large base64 string as text instead of an image, making the screenshot unusable for visual reasoning (openpencil issue #204). - Add ToolOutcome::OkImageContent carrying base64 + mime_type plus an optional metadata JSON string - Thread an optional ImageContent through ToolResponse::Ok and emit it as an MCP {"type":"image","data":...,"mimeType":...} content block in tool_response_to_json before any text block - get_screenshot now returns OkImageContent (image/png) while retaining image_base64 in its text metadata for the in-app chat-agent path - Update the get_screenshot schema description to state the use case (PNG image for visual verification) without the base64 implementation detail --- .../src/chat_agent_loop_tests.rs | 2 +- .../op-host-services/src/mcp_serve/schemas.rs | 2 +- .../src/mcp_serve/screenshot_tool.rs | 47 ++++++++++----- crates/op-mcp/src/json_serializer.rs | 25 ++++++-- crates/op-mcp/src/lib.rs | 48 +++++++++++++++ crates/op-mcp/src/mcp_tests.rs | 59 +++++++++++++++++++ 6 files changed, 163 insertions(+), 20 deletions(-) diff --git a/crates/op-host-services/src/chat_agent_loop_tests.rs b/crates/op-host-services/src/chat_agent_loop_tests.rs index 162eefca0..3cd6d491b 100644 --- a/crates/op-host-services/src/chat_agent_loop_tests.rs +++ b/crates/op-host-services/src/chat_agent_loop_tests.rs @@ -297,7 +297,7 @@ fn delete_node_tool_def() -> ChatToolDef { fn get_screenshot_tool_def() -> ChatToolDef { ChatToolDef { name: "get_screenshot".into(), - description: "Render a node to a base64 PNG".into(), + description: "Render a node to a PNG image for visual verification".into(), level: "read".into(), input_schema_json: r#"{"type":"object","properties":{"nodeId":{"type":"string"}}}"#.into(), } diff --git a/crates/op-host-services/src/mcp_serve/schemas.rs b/crates/op-host-services/src/mcp_serve/schemas.rs index e24e13545..61331f25e 100644 --- a/crates/op-host-services/src/mcp_serve/schemas.rs +++ b/crates/op-host-services/src/mcp_serve/schemas.rs @@ -27,7 +27,7 @@ pub const TOOL_SCHEMAS: &[&str] = &[ r#"{"name":"get_guidelines","description":"Return OpenPencil product-design guidelines. Default category is guide, which reads topic. category=style resolves an OpenPencil style using flat scalar string args.","inputSchema":{"type":"object","properties":{"category":{"type":"string","enum":["guide","style"],"default":"guide","description":"guide returns the existing topic guideline path; style resolves the style catalog"},"topic":{"type":"string","enum":["web-app","mobile","code-to-design"],"description":"Guide topic for category=guide"},"name":{"type":"string","description":"Style catalog name for category=style"},"colorPalette":{"type":"string","description":"Style palette name for category=style"},"roundness":{"type":"string","description":"Roundness profile for category=style"},"elevation":{"type":"string","description":"Elevation profile for category=style"},"headings":{"type":"string","description":"Heading font family for category=style"},"body":{"type":"string","description":"Body font family for category=style"},"captions":{"type":"string","description":"Caption font family for category=style"},"data":{"type":"string","description":"Data font family for category=style"},"decorativeImagery":{"type":"string","description":"Optional decorative imagery guidance for category=style"}}}}"#, r#"{"name":"spawn_agents","description":"Split a large design task into parallel subtasks. Each config item gives a prompt, the container node(s) to fill, and the styleguide + guideline NAMES to pass to the subagent (subagents cannot search styleguides). Returns the spawned agent ids. Execution runs the subagents in parallel.","inputSchema":{"type":"object","properties":{"config":{"type":"array","items":{"type":"object","properties":{"prompt":{"type":"string"},"containerNodes":{"type":"array","items":{"type":"string"}},"styleguideName":{"type":"string"},"guidelineNames":{"type":"array","items":{"type":"string"}}},"required":["prompt","styleguideName"]}}},"required":["config"]}}"#, r#"{"name":"ToolSearch","description":"Discover tools by keyword or exact selection. Use 'select:Name1,Name2' to load specific tools, or a keyword query to search names+descriptions.","inputSchema":{"type":"object","properties":{"query":{"type":"string"},"max_results":{"type":"integer","minimum":1,"default":5}},"required":["query"]}}"#, - r#"{"name":"get_screenshot","description":"Render a node to a base64 PNG for visual verification. nodeId may be \"root\" for the active page's top node.","inputSchema":{"type":"object","properties":{"nodeId":{"type":"string","description":"Node id, or \"root\""}},"required":["nodeId"]}}"#, + r#"{"name":"get_screenshot","description":"Render a node to a PNG image for visual verification. nodeId may be \"root\" for the active page's top node.","inputSchema":{"type":"object","properties":{"nodeId":{"type":"string","description":"Node id, or \"root\""}},"required":["nodeId"]}}"#, r#"{"name":"export_item","description":"Export a page, arbitrary node, or current selection to base64-encoded image/PDF bytes.","inputSchema":{"type":"object","properties":{"itemId":{"type":"string"},"format":{"type":"string","enum":["png","jpeg","webp","pdf"]},"scale":{"type":"number"}},"required":["format"]}}"#, r#"{"name":"export_nodes","description":"Export one or more nodes to base64-encoded image/PDF bytes. format is one of png|jpeg|webp|pdf.","inputSchema":{"type":"object","properties":{"nodeIds":{"type":"array","items":{"type":"string"}},"format":{"type":"string","enum":["png","jpeg","webp","pdf"]},"scale":{"type":"number"}},"required":["nodeIds","format"]}}"#, r#"{"name":"get_active_theme","description":"Return the active theme axis pinning per axis.","inputSchema":{"type":"object","properties":{},"additionalProperties":false}}"#, diff --git a/crates/op-host-services/src/mcp_serve/screenshot_tool.rs b/crates/op-host-services/src/mcp_serve/screenshot_tool.rs index a205a3dd9..2e820346b 100644 --- a/crates/op-host-services/src/mcp_serve/screenshot_tool.rs +++ b/crates/op-host-services/src/mcp_serve/screenshot_tool.rs @@ -74,11 +74,18 @@ impl McpTool for GetScreenshot { // (see `export/screenshot.rs` and `export/tests.rs`). let image_base64 = base64::engine::general_purpose::STANDARD.encode(&bytes); - let out = serde_json::json!({ - "image_base64": image_base64, + let metadata = serde_json::json!({ + "nodeId": node_id, "format": "png", + // include image_base64 in metadata for the in-app chat agent path — + // the MCP path uses the `image` field on ToolResponse::Ok separately + "image_base64": image_base64, }); - ToolOutcome::OkJson(out.to_string()) + ToolOutcome::OkImageContent { + image_base64, + mime_type: "image/png".into(), + metadata_json: Some(metadata.to_string()), + } } } @@ -148,15 +155,23 @@ mod tests { )]); let tool = tool_from_scene(scene); match call(&tool, "root") { - ToolOutcome::OkJson(json) => { - let v: serde_json::Value = serde_json::from_str(&json).expect("valid JSON"); - let b64 = v["image_base64"].as_str().expect("image_base64 field"); - assert!(!b64.is_empty(), "image_base64 must not be empty"); - let bytes = decode_base64(b64); + ToolOutcome::OkImageContent { + image_base64, + mime_type, + metadata_json, + } => { + assert_eq!(mime_type, "image/png"); + assert!(!image_base64.is_empty(), "image_base64 must not be empty"); + let bytes = decode_base64(&image_base64); assert_eq!(&bytes[..8], PNG_MAGIC, "must be a PNG payload"); - assert_eq!(v["format"], "png"); + // Metadata carries nodeId + format + image_base64 (for chat agent path). + let meta: serde_json::Value = + serde_json::from_str(&metadata_json.expect("metadata")).expect("valid JSON"); + assert_eq!(meta["format"], "png"); + assert_eq!(meta["nodeId"], "n1"); + assert!(!meta["image_base64"].as_str().unwrap_or("").is_empty()); } - other => panic!("expected OkJson, got {other:?}"), + other => panic!("expected OkImageContent, got {other:?}"), } } @@ -181,12 +196,16 @@ mod tests { ]); let tool = tool_from_scene(scene); match call(&tool, "r2") { - ToolOutcome::OkJson(json) => { - let v: serde_json::Value = serde_json::from_str(&json).expect("valid JSON"); - let bytes = decode_base64(v["image_base64"].as_str().expect("field")); + ToolOutcome::OkImageContent { + image_base64, + mime_type, + .. + } => { + assert_eq!(mime_type, "image/png"); + let bytes = decode_base64(&image_base64); assert_eq!(&bytes[..8], PNG_MAGIC, "must be a PNG payload"); } - other => panic!("expected OkJson, got {other:?}"), + other => panic!("expected OkImageContent, got {other:?}"), } } diff --git a/crates/op-mcp/src/json_serializer.rs b/crates/op-mcp/src/json_serializer.rs index a5f4d6308..74ce7515a 100644 --- a/crates/op-mcp/src/json_serializer.rs +++ b/crates/op-mcp/src/json_serializer.rs @@ -41,20 +41,37 @@ pub fn response_to_json(r: &ToolResponse) -> String { /// result — NOT a JSON-RPC `error` (those are reserved for transport/parse /// failures, still emitted via [`response_to_json`]). External MCP clients /// (Claude Code / Codex) require this envelope. +/// +/// When `image` is `Some`, an MCP `ImageContent` block is emitted BEFORE +/// any text block so vision-capable MCP clients receive the image as +/// multimodal content instead of a base64 string buried in JSON text. pub fn tool_response_to_json(r: &ToolResponse) -> String { let (id_repr, body) = match r { ToolResponse::Ok { - id, result, json, .. + id, + result, + json, + image, + .. } => { - // Nested-JSON read result rides verbatim in the text block; - // otherwise the flat string-map is encoded as the text. + let mut content_blocks: Vec = Vec::new(); + // Image block first (when present) so vision models see it. + if let Some(img) = image { + content_blocks.push(format!( + r#"{{"type":"image","data":{},"mimeType":{}}}"#, + json_escape(&img.data), + json_escape(&img.mime_type), + )); + } + // Text block: nested JSON or flat string-map. let text = match json { Some(raw) => json_escape(raw), None => json_escape(&btree_to_json(result)), }; + content_blocks.push(format!(r#"{{"type":"text","text":{text}}}"#)); ( id_to_json(id), - format!(r#""result":{{"content":[{{"type":"text","text":{text}}}]}}"#), + format!(r#""result":{{"content":[{}]}}"#, content_blocks.join(",")), ) } ToolResponse::Err { id, message, .. } => ( diff --git a/crates/op-mcp/src/lib.rs b/crates/op-mcp/src/lib.rs index ef39a085b..692ebc4e6 100644 --- a/crates/op-mcp/src/lib.rs +++ b/crates/op-mcp/src/lib.rs @@ -314,6 +314,17 @@ pub struct ToolCall { pub arguments: BTreeMap, } +/// MCP `ImageContent` block payload — carried by `ToolResponse::Ok` +/// so the serializer can emit a proper `{"type":"image",…}` content +/// block instead of wrapping base64 bytes in a text block. +#[derive(Debug, Clone, PartialEq)] +pub struct ImageContent { + /// Base64-encoded image bytes (no data-URI prefix). + pub data: String, + /// MIME type, e.g. `"image/png"`. + pub mime_type: String, +} + /// Tool response — either a structured result object or an error. /// Errors are typed enough for the LLM client to recover (e.g. /// `MissingArgument` vs `InvalidArgument` vs `ToolFailed`). @@ -334,6 +345,13 @@ pub enum ToolResponse { /// (the flat map) — so read tools match TS's arbitrary-JSON /// shapes byte-for-byte. `None` ⇒ use the flat-map encoding. json: Option, + /// When `Some`, the serializer emits an MCP `ImageContent` block + /// (`{"type":"image","data":"","mimeType":"..."}`) in the + /// `content[]` array BEFORE any text block (from `json` or + /// `result`). Tools like `get_screenshot` use this so MCP clients + /// (Copilot, Claude Code, …) receive the image as multimodal + /// content instead of a base64 string buried in JSON text. + image: Option, }, Err { id: RequestId, @@ -382,6 +400,18 @@ pub enum ToolOutcome { /// results (`fixes[]`, `layoutSnapshot`, `results[]`, …) while still /// mutating the document. The host applies `command`; the client sees `json`. OkJsonWithCommand(String, EditorCommand), + /// A tool returning a base64-encoded image as an MCP `ImageContent` block. + /// `image_base64` is the raw base64 (no data-URI prefix); + /// `mime_type` is e.g. `"image/png"`. An optional `metadata_json` + /// string is emitted as a separate `text` content block (like + /// `debug_screenshot`'s pretty-printed metadata block). MCP clients + /// that support vision (Copilot, Claude Code, …) receive the image as + /// multimodal content instead of a base64 string in JSON text. + OkImageContent { + image_base64: String, + mime_type: String, + metadata_json: Option, + }, Err(ToolErrorCode, String), } @@ -423,24 +453,42 @@ impl ToolRegistry { result, command: None, json: None, + image: None, }, ToolOutcome::OkWithCommand(result, command) => ToolResponse::Ok { id: call.id, result, command: Some(command), json: None, + image: None, }, ToolOutcome::OkJson(json) => ToolResponse::Ok { id: call.id, result: BTreeMap::new(), command: None, json: Some(json), + image: None, }, ToolOutcome::OkJsonWithCommand(json, command) => ToolResponse::Ok { id: call.id, result: BTreeMap::new(), command: Some(command), json: Some(json), + image: None, + }, + ToolOutcome::OkImageContent { + image_base64, + mime_type, + metadata_json, + } => ToolResponse::Ok { + id: call.id, + result: BTreeMap::new(), + command: None, + json: metadata_json, + image: Some(ImageContent { + data: image_base64, + mime_type, + }), }, ToolOutcome::Err(code, message) => ToolResponse::Err { id: call.id, diff --git a/crates/op-mcp/src/mcp_tests.rs b/crates/op-mcp/src/mcp_tests.rs index d538330f4..f0bf7f9f1 100644 --- a/crates/op-mcp/src/mcp_tests.rs +++ b/crates/op-mcp/src/mcp_tests.rs @@ -97,6 +97,7 @@ fn response_to_json_ok_payload() { result, command: None, json: None, + image: None, }; let j = response_to_json(&r); assert!(j.contains(r#""jsonrpc":"2.0""#)); @@ -129,6 +130,7 @@ fn tool_response_to_json_wraps_ok_in_mcp_content_envelope() { result, command: None, json: None, + image: None, }; let j = tool_response_to_json(&r); assert!(j.contains(r#""id":7"#), "{j}"); @@ -148,6 +150,7 @@ fn ok_json_rides_verbatim_in_both_serializers() { result: BTreeMap::new(), command: None, json: Some(nested.to_string()), + image: None, }; // tools/call envelope: text block holds the nested JSON verbatim. assert_eq!(tool_text(&tool_response_to_json(&r)), nested); @@ -159,6 +162,62 @@ fn ok_json_rides_verbatim_in_both_serializers() { ); } +#[test] +fn tool_response_to_json_emits_image_content_block_when_image_is_some() { + // When `image` is `Some`, the serializer emits an ImageContent block + // BEFORE the text block so vision-capable MCP clients receive the + // image as multimodal content. + let r = ToolResponse::Ok { + id: RequestId::Num(3), + result: BTreeMap::new(), + command: None, + json: Some(r#"{"nodeId":"n1","format":"png"}"#.into()), + image: Some(ImageContent { + data: "iVBORw0KGgo=".into(), + mime_type: "image/png".into(), + }), + }; + let j = tool_response_to_json(&r); + // Parse the JSON-RPC envelope to verify structure. + let v: serde_json::Value = serde_json::from_str(&j).expect("valid JSON-RPC"); + let content = v["result"]["content"].as_array().expect("content array"); + assert_eq!(content.len(), 2, "expected image + text blocks"); + // First block must be the image. + assert_eq!(content[0]["type"], "image"); + assert_eq!(content[0]["data"], "iVBORw0KGgo="); + assert_eq!(content[0]["mimeType"], "image/png"); + // Second block must be text with the metadata. + assert_eq!(content[1]["type"], "text"); + let text_val = content[1]["text"].as_str().expect("text string"); + let meta: serde_json::Value = + serde_json::from_str(text_val).expect("metadata parses as JSON"); + assert_eq!(meta["nodeId"], "n1"); + assert_eq!(meta["format"], "png"); + // No isError for successful results. + assert!(v["result"].get("isError").is_none(), "{j}"); +} + +#[test] +fn tool_response_to_json_image_without_text_metadata_still_emits_blocks() { + // When image is set but json/result are empty, the text block is still + // present (with empty object) — MCP clients expect at least one text + // block for tool result reporting. + let r = ToolResponse::Ok { + id: RequestId::Num(4), + result: BTreeMap::new(), + command: None, + json: None, + image: Some(ImageContent { + data: "AAAA".into(), + mime_type: "image/png".into(), + }), + }; + let j = tool_response_to_json(&r); + assert!(j.contains(r#""type":"image""#), "{j}"); + assert!(j.contains(r#""type":"text""#), "{j}"); + assert!(j.contains(r#""data":"AAAA""#), "{j}"); +} + #[test] fn tool_response_to_json_marks_tool_error_with_iserror() { // A tool-level failure is an `isError` result, NOT a JSON-RPC error.