From 0299162db280ad920d78a21934cf1dc103f35521 Mon Sep 17 00:00:00 2001 From: kayrockscreenprinting Date: Fri, 9 Oct 2026 10:29:50 -0700 Subject: [PATCH] MCP: tool schemas without $ref, $defs or boolean schemas (#1845) Every tool's input schema is rewritten once at startup: $refs inlined (use-site descriptions win), $defs dropped, boolean schemas turned into {}, with a depth cap for recursive definitions. ui_pointer events get a concrete schema. Strict providers no longer reject the whole tool list. Fixes #1782. Co-Authored-By: Claude Opus 5.5 --- crates/automation/src/server.rs | 122 +++++++++++++++++++++++++++++++- crates/automation/tests/mcp.rs | 57 +++++++++++++++ 2 files changed, 178 insertions(+), 1 deletion(-) diff --git a/crates/automation/src/server.rs b/crates/automation/src/server.rs index fb8e31cb..3debf5ed 100644 --- a/crates/automation/src/server.rs +++ b/crates/automation/src/server.rs @@ -169,11 +169,42 @@ pub struct BatchParams { pub stop_on_error: Option, } +/// One `ui_pointer` event, for the tool schema only: events are forwarded to `ui.pointer` as JSON. +#[derive(JsonSchema)] +#[allow(dead_code)] +struct PointerEvent { + /// `down`, `move` (default) or `up`. + kind: Option, + /// Document x in pixels. + x: f64, + /// Document y in pixels. + y: f64, + /// Pen pressure 0..1 (default 1). + pressure: Option, + /// A simulated pen's tilt, as a stylus reports it. + #[serde(rename = "tiltX")] + tilt_x: Option, + #[serde(rename = "tiltY")] + tilt_y: Option, + /// A simulated pen's barrel rotation. + rotation: Option, +} + +#[derive(JsonSchema)] +#[serde(rename_all = "lowercase")] +#[allow(dead_code)] +enum PointerKind { + Down, + Move, + Up, +} + /// Strict: an argument the tool doesn't forward is an error, not silently dropped. #[derive(Debug, Deserialize, JsonSchema)] #[serde(deny_unknown_fields)] pub struct PointerParams { /// Events in document coordinates: `[{"kind":"down|move|up","x":..,"y":..,"pressure":..}]`. + #[schemars(with = "Vec")] pub events: Vec, /// Modifier keys, e.g. `{"shift":true}` (also `alt`, `command`, `ctrl`, `space`). #[serde(default)] @@ -220,6 +251,65 @@ pub struct ControlParams { // Helpers // --------------------------------------------------------------------------- +/// Rewrites a tool's input schema into the subset strict tool-schema validators accept (#1782): +/// `$ref`s into `$defs` are inlined and `$defs` dropped, and a `true` schema (any value, as +/// schemars writes for `Value`) becomes `{}`. Tool schemas travel to every provider a client uses, +/// and some reject the whole request over either construct. +fn strict_schema(schema: &mut serde_json::Map) { + let defs = schema.remove("$defs").and_then(|d| d.as_object().cloned()).unwrap_or_default(); + let mut root = Value::Object(std::mem::take(schema)); + strict_node(&mut root, &defs, 0, true); + if let Value::Object(m) = root { + *schema = m; + } +} + +/// `v` (a schema when `is_schema`, else any value that may contain schemas) made strict. Inlining +/// stops at a fixed depth so a recursive definition can't loop. +fn strict_node(v: &mut Value, defs: &serde_json::Map, depth: usize, is_schema: bool) { + const MAX_DEPTH: usize = 32; + if is_schema && *v == Value::Bool(true) { + *v = json!({}); + return; + } + let Value::Object(m) = v else { + if let Value::Array(a) = v { + for x in a { + strict_node(x, defs, depth, is_schema); + } + } + return; + }; + if let Some(def) = m.get("$ref").and_then(Value::as_str).and_then(|r| r.strip_prefix("#/$defs/")).and_then(|name| defs.get(name)) + && depth < MAX_DEPTH + { + let mut inlined = def.clone(); + if let Value::Object(im) = &mut inlined { + // Keep the use site's own annotations (its description) over the definition's. + for (k, x) in m.iter().filter(|(k, _)| k.as_str() != "$ref") { + im.insert(k.clone(), x.clone()); + } + } + *v = inlined; + strict_node(v, defs, depth + 1, true); + return; + } + for (k, x) in m.iter_mut() { + match k.as_str() { + // Maps of name → schema. + "properties" | "patternProperties" => { + if let Value::Object(props) = x { + for p in props.values_mut() { + strict_node(p, defs, depth, true); + } + } + } + "items" | "additionalProperties" | "not" | "anyOf" | "oneOf" | "allOf" | "prefixItems" => strict_node(x, defs, depth, true), + _ => {} + } + } +} + fn ok_json(v: &Value) -> CallToolResult { let text = match json_bytes(v).and_then(|bytes| String::from_utf8(bytes).map_err(|error| AutomationError::Other(error.to_string()))) { Ok(text) => text, @@ -286,7 +376,13 @@ impl PhotocraftMcp { } pub fn with_backend(backend: Backend) -> Self { - PhotocraftMcp { backend: Arc::new(backend), tool_router: Self::tool_router() } + let mut tool_router = Self::tool_router(); + for route in tool_router.map.values_mut() { + let mut schema = (*route.attr.input_schema).clone(); + strict_schema(&mut schema); + route.attr.input_schema = Arc::new(schema); + } + PhotocraftMcp { backend: Arc::new(backend), tool_router } } /// Serve MCP over stdin/stdout until the client disconnects. @@ -751,6 +847,30 @@ pub fn render_document_png(doc: &photocraft_doc::Document, max_side: u32) -> Res mod tests { use super::*; + #[test] + fn strict_schema_inlines_refs_and_stops_on_recursive_definitions() { + // A definition that refers to itself: inlining stops at a depth instead of looping. + let mut s = json!({ + "type": "object", + "properties": { + "node": {"$ref": "#/$defs/Node", "description": "use-site text"}, + "any": true, + "list": {"type": "array", "items": true} + }, + "$defs": {"Node": {"type": "object", "description": "def text", "properties": {"child": {"$ref": "#/$defs/Node"}}}} + }); + let m = s.as_object_mut().unwrap(); + strict_schema(m); + assert!(m.get("$defs").is_none()); + let props = &m["properties"]; + assert_eq!(props["node"]["type"], "object"); + assert_eq!(props["node"]["description"], "use-site text", "the use site's description wins"); + assert_eq!(props["node"]["properties"]["child"]["type"], "object", "nested refs inline too"); + assert_eq!((&props["any"], &props["list"]["items"]), (&json!({}), &json!({}))); + let text = serde_json::to_string(&s).unwrap(); + assert!(text.len() < 1 << 20, "bounded: {} bytes", text.len()); + } + #[test] fn mcp_text_checks_the_encoded_tool_envelope() { // JSON escaping in the outer text-content envelope also consumes the budget. diff --git a/crates/automation/tests/mcp.rs b/crates/automation/tests/mcp.rs index 46ab35d7..42a41126 100644 --- a/crates/automation/tests/mcp.rs +++ b/crates/automation/tests/mcp.rs @@ -169,6 +169,63 @@ async fn command_list_filters() { client.cancel().await.unwrap(); } +/// Constructs strict tool-schema validators reject: a `$ref` / `$defs`, or a boolean in a schema +/// position (`items: true`), anywhere in `schema`. +fn strict_schema_problems(path: &str, schema: &Value, out: &mut Vec) { + let Value::Object(m) = schema else { + if schema.is_boolean() { + out.push(format!("{path}: boolean schema {schema}")); + } + return; + }; + for key in ["$ref", "$defs", "definitions"] { + if m.contains_key(key) { + out.push(format!("{path}: {key}")); + } + } + if let Some(Value::Object(props)) = m.get("properties") { + for (k, v) in props { + strict_schema_problems(&format!("{path}.properties.{k}"), v, out); + } + } + for key in ["items", "not"] { + if let Some(v) = m.get(key) { + strict_schema_problems(&format!("{path}.{key}"), v, out); + } + } + for key in ["anyOf", "oneOf", "allOf", "prefixItems"] { + for (i, v) in m.get(key).and_then(Value::as_array).into_iter().flatten().enumerate() { + strict_schema_problems(&format!("{path}.{key}[{i}]"), v, out); + } + } + if let Some(v) = m.get("additionalProperties").filter(|v| v.is_object()) { + strict_schema_problems(&format!("{path}.additionalProperties"), v, out); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn tool_schemas_pass_strict_validators() { + // #1782: `ui_pointer`'s `items: true` and `command_batch`'s `$ref`/`$defs` made strict + // providers (e.g. Moonshot via Volcano Ark) reject the whole request. + let client = connect(PhotocraftMcp::headless()).await; + let tools = client.list_all_tools().await.unwrap(); + let mut problems = Vec::new(); + for t in &tools { + strict_schema_problems(&t.name, &Value::Object((*t.input_schema).clone()), &mut problems); + } + assert!(problems.is_empty(), "{}", problems.join("\n")); + let schema = |name: &str| Value::Object((*tools.iter().find(|t| t.name == name).unwrap().input_schema).clone()); + // The inlined shapes still describe the arguments. + let event = &schema("ui_pointer")["properties"]["events"]["items"]; + assert_eq!(event["type"], "object", "{event}"); + for k in ["kind", "x", "y", "pressure"] { + assert!(event["properties"][k].is_object(), "ui_pointer event field {k}: {event}"); + } + let step = &schema("command_batch")["properties"]["steps"]["items"]; + assert!(step["properties"]["id"]["description"].as_str().is_some_and(|d| d.contains("layer.new.layer")), "{step}"); + client.cancel().await.unwrap(); +} + /// The backticked examples in every `id` property description of a schema. fn id_examples(schema: &Value, out: &mut Vec) { match schema {