From 8373030b449087f13114c1eaf53c741d2251e45e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20S=C3=BAkup?= Date: Tue, 18 Aug 2026 17:16:50 +0200 Subject: [PATCH] fix(server): normalize tool schemas for gemini/vertex clients schemars 1.2.2 renders every Option tool parameter as a JSON-Schema type union ("type": [T, "null"]) plus a stray "default": null, and every Rust integer parameter carries a uint64/uint32 format Gemini doesn't recognize. rmcp's SchemaSettings::draft2020_12() runs zero transforms, so this is exactly what list_tools/get_tool served. A Gemini/Vertex client rewrites the type union into anyOf while keeping description/format/default as siblings of it, a shape Vertex itself then refuses ("when using any_of, it must be the only field set") - so the whole tool list was rejected, not just the one property Vertex happened to name. BugWarden::new now runs a portable_schema pass once, at router construction, over every route's input_schema: collapse the type union to a plain type, drop the resulting default:null, and drop any format outside a portable keep-list (date-time, int32, int64, float, double, enum). serde still accepts an explicit null for every Option field - only the advertised schema got stricter, not what the server accepts, so no invariant (I1-I16) is touched. Verified against a real stdio tools/list probe: 0 type unions, 0 default:null, formats reduced to exactly {date-time} across all 20 served tools. --- crates/bugwarden/src/server.rs | 135 +++++++++++++++++++++++++++++++++ docs/DESIGN.md | 22 ++++++ 2 files changed, 157 insertions(+) diff --git a/crates/bugwarden/src/server.rs b/crates/bugwarden/src/server.rs index bf86916..d512579 100644 --- a/crates/bugwarden/src/server.rs +++ b/crates/bugwarden/src/server.rs @@ -1236,6 +1236,56 @@ pub struct BugFieldsParams { pub on_bug_entry_only: bool, } +/// `format` values a Gemini/Vertex client accepts in a tool's `inputSchema`. +/// +/// A keep-list, not a deny-list: schemars also emits `uint64` / `uint32` for +/// Rust integer widths, which Vertex does not recognize. Dropping them loses +/// no constraint the deserializer doesn't already enforce — `type: integer` +/// plus `minimum: 0` carries the non-negativity, and the actual width is +/// enforced by serde at deserialization, never by the advertised schema. +const PORTABLE_FORMATS: &[&str] = &["date-time", "int32", "int64", "float", "double", "enum"]; + +/// Rewrites a schemars-generated `inputSchema` node in place so a +/// Gemini/Vertex client accepts it. +/// +/// schemars renders `Option` as `"type": [T, "null"]` plus a stray +/// `"default": null`. Vertex's client rewrites that union into +/// `anyOf: [{...}, {"type": "null"}]` while keeping `description` / +/// `format` / `default` as *siblings* of `anyOf` — a shape Vertex itself +/// then refuses ("when using any_of, it must be the only field set"). +/// Collapsing the union to a plain type before it ever reaches the client +/// starves that rewrite of its input. Recurses into `properties` and +/// `items` because the same union can appear nested (for example inside an +/// array parameter's `items`). +fn portable_schema(node: &mut Value) { + let Value::Object(obj) = node else { + return; + }; + if let Some(Value::Array(types)) = obj.get_mut("type") { + types.retain(|t| t.as_str() != Some("null")); + if types.len() == 1 { + let only = types.remove(0); + obj.insert("type".to_string(), only); + } + } + if obj.get("default") == Some(&Value::Null) { + obj.remove("default"); + } + if let Some(format) = obj.get("format").and_then(Value::as_str) { + if !PORTABLE_FORMATS.contains(&format) { + obj.remove("format"); + } + } + if let Some(properties) = obj.get_mut("properties").and_then(Value::as_object_mut) { + for prop in properties.values_mut() { + portable_schema(prop); + } + } + if let Some(items) = obj.get_mut("items") { + portable_schema(items); + } +} + /// The MCP server: guard policy, Bugzilla client, and the pruned tool /// router (I13). Construct with [`BugWarden::new`]; serve over any rmcp /// transport. @@ -1307,6 +1357,15 @@ impl BugWarden { ); } let mut tool_router = Self::tool_router(); + // One normalization pass covers both `list_tools` and `get_tool`, + // since both read from the same routed `Tool::input_schema`. + for route in tool_router.map.values_mut() { + let mut schema = Value::Object((*route.attr.input_schema).clone()); + portable_schema(&mut schema); + if let Value::Object(obj) = schema { + route.attr.input_schema = Arc::new(obj); + } + } // Validate disabled_tools against the FULL router, before any route // removal: a write-tool name stays a valid entry even when read-only // mode removes that route first. @@ -4218,6 +4277,82 @@ mod tests { ); } + /// Recursive walk mirroring [`portable_schema`]'s own recursion, so the + /// assertion covers the same nodes the normalization pass touches. + fn assert_schema_is_client_portable(node: &Value, tool: &str) { + let Value::Object(obj) = node else { + return; + }; + assert!( + !matches!(obj.get("type"), Some(Value::Array(_))), + "{tool}: schema still has a type union: {obj:?}" + ); + assert_ne!( + obj.get("default"), + Some(&Value::Null), + "{tool}: schema still advertises \"default\": null: {obj:?}" + ); + if let Some(format) = obj.get("format").and_then(Value::as_str) { + assert!( + PORTABLE_FORMATS.contains(&format), + "{tool}: schema advertises non-portable format {format:?}: {obj:?}" + ); + } + for key in ["anyOf", "oneOf", "allOf", "$ref", "$defs"] { + assert!( + !obj.contains_key(key), + "{tool}: schema uses unsupported {key}: {obj:?}" + ); + } + if let Some(properties) = obj.get("properties").and_then(Value::as_object) { + for prop in properties.values() { + assert_schema_is_client_portable(prop, tool); + } + } + if let Some(items) = obj.get("items") { + assert_schema_is_client_portable(items, tool); + } + } + + #[test] + fn served_tool_schemas_are_client_portable() { + let (cfg, guard, bz) = parts("[global]\nallow_discovery = true\n"); + let server = BugWarden::new(cfg, guard, bz).expect("server builds"); + for tool in server.tool_router.list_all() { + assert_schema_is_client_portable( + &Value::Object((*tool.input_schema).clone()), + &tool.name, + ); + } + } + + #[test] + fn bug_comments_new_since_is_a_plain_date_time() { + let (cfg, guard, bz) = parts("[global]\nallow_discovery = true\n"); + let server = BugWarden::new(cfg, guard, bz).expect("server builds"); + let tool = server + .get_tool("bug_comments") + .expect("bug_comments is routed"); + let new_since = tool.input_schema["properties"]["new_since"].clone(); + assert_eq!( + new_since, + json!({ + "description": "Only return comments newer than this date.", + "format": "date-time", + "type": "string", + }), + "new_since must be a plain nullable-free date-time string" + ); + } + + #[test] + fn optional_params_still_accept_an_explicit_null() { + // The advertised schema got stricter; what serde accepts did not. + let value = json!({"id": 1, "new_since": null}); + serde_json::from_value::(value) + .expect("an explicit null for an Option field must still deserialize"); + } + #[test] fn decoded_len_counts_decoded_bytes_exactly() { assert_eq!(decoded_len(""), 0); diff --git a/docs/DESIGN.md b/docs/DESIGN.md index cbd2259..0f50c45 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1427,6 +1427,28 @@ wired, `server.rs` and `main.rs` are the reference. is unserved. When it is adopted, `cache_scope` is `Private` — the listing is pruned per deployment (I13), so a shared cache must never serve one deployment's list to another — and `CacheScope::default()` is `Public`. +- **rmcp trap — `input_schema` is schemars' rendering, unfiltered by rmcp.** + `SchemaSettings::draft2020_12()` (`handler/server/common.rs`) runs zero + transforms, so whatever schemars 1.x emits for a `#[tool]` param struct is + exactly what `list_tools` / `get_tool` serve. schemars renders `Option` + as `"type": [T, "null"]` plus a stray `"default": null`, and a Rust integer + width as `format: "uint64"` / `"uint32"` — neither is portable: a + Gemini/Vertex client rewrites the type union into `anyOf: [{...}, + {"type":"null"}]` while leaving `description` / `format` / `default` as + *siblings* of it, a shape Vertex itself then refuses ("when using any_of, + it must be the only field set"). `BugWarden::new` runs `portable_schema` + (server.rs) over every route's `input_schema` once, at construction — + before the `disabled_tools` check — collapsing the union to a plain type, + dropping the resulting `"default": null`, and dropping any `format` not in + `PORTABLE_FORMATS` (`date-time`, `int32`, `int64`, `float`, `double`, + `enum`; `uint64`/`uint32` are schemars' own annotations for a constraint + `type: integer` + `minimum: 0` already carries, enforced by the + deserializer regardless of what the schema advertises). serde still accepts + an explicit `null` for every `Option` field — only the advertisement got + stricter — so no tool's accepted input changed and no invariant (I1–I16) is + involved. `ToolRouter::map` and `ToolRoute::attr` are public fields of + `#[non_exhaustive]` structs; an rmcp upgrade that reshapes either breaks + the build loudly rather than silently skipping the pass. - **Every `StreamableHttpServerConfig` field is accounted for below** — set by name or inherited for a stated reason. `BugWarden::http_server_config()` (server.rs) names two; main.rs adds a third at the call site. The struct is