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 <noreply@anthropic.com>
This commit is contained in:
parent
63f9ca9d74
commit
0299162db2
|
|
@ -169,11 +169,42 @@ pub struct BatchParams {
|
|||
pub stop_on_error: Option<bool>,
|
||||
}
|
||||
|
||||
/// 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<PointerKind>,
|
||||
/// Document x in pixels.
|
||||
x: f64,
|
||||
/// Document y in pixels.
|
||||
y: f64,
|
||||
/// Pen pressure 0..1 (default 1).
|
||||
pressure: Option<f64>,
|
||||
/// A simulated pen's tilt, as a stylus reports it.
|
||||
#[serde(rename = "tiltX")]
|
||||
tilt_x: Option<f64>,
|
||||
#[serde(rename = "tiltY")]
|
||||
tilt_y: Option<f64>,
|
||||
/// A simulated pen's barrel rotation.
|
||||
rotation: Option<f64>,
|
||||
}
|
||||
|
||||
#[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<PointerEvent>")]
|
||||
pub events: Vec<Value>,
|
||||
/// 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<String, Value>) {
|
||||
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<String, Value>, 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.
|
||||
|
|
|
|||
|
|
@ -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<String>) {
|
||||
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<String>) {
|
||||
match schema {
|
||||
|
|
|
|||
Loading…
Reference in New Issue