From f23a5d78b2ea0fcd4607161c0c4cccceff6c4f0c Mon Sep 17 00:00:00 2001 From: hugocasa Date: Tue, 11 Aug 2026 20:20:47 +0200 Subject: [PATCH] fix: tell MCP clients which tool parameters may be omitted (#10642) * fix: tell MCP clients which tool parameters may be omitted Co-Authored-By: Claude Opus 5 (1M context) * refactor: make the mcp property-key rename testable and shorten the hint Co-Authored-By: Claude Opus 5 (1M context) * fix: keep the mcp omission hint from calling flow inputs optional Co-Authored-By: Claude Opus 5 (1M context) * fix: skip the mcp omission hint on a parameterless tool Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- backend/windmill-api/src/mcp/core.rs | 20 +-- backend/windmill-mcp/src/common/mod.rs | 6 +- backend/windmill-mcp/src/common/transform.rs | 65 ++++++++++ backend/windmill-mcp/src/server/tools.rs | 129 ++++++++++++++++++- 4 files changed, 197 insertions(+), 23 deletions(-) diff --git a/backend/windmill-api/src/mcp/core.rs b/backend/windmill-api/src/mcp/core.rs index af7e22fcba..971f8a890f 100644 --- a/backend/windmill-api/src/mcp/core.rs +++ b/backend/windmill-api/src/mcp/core.rs @@ -8,7 +8,7 @@ use serde_json::Value; use std::collections::HashMap; use windmill_common::{db::UserDB, utils::StripPath, DB}; use windmill_mcp::common::schema::enrich_resource_schemas; -use windmill_mcp::common::transform::apply_key_transformation; +use windmill_mcp::common::transform::transform_property_keys; use windmill_mcp::common::types::{ FlowInfo, HubScriptInfo, ResourceInfo, ResourceType, SchemaType, ScriptInfo, WorkspaceInfo, }; @@ -194,23 +194,7 @@ impl McpBackend for WindmillBackend { let mut schema_obj = schema.clone(); // Replace invalid char in property key with underscore - let replacements: Vec<(String, String, Value)> = schema_obj - .properties - .iter() - .filter_map(|(key, value)| { - if key.chars().any(|c| !c.is_alphanumeric() && c != '_') { - let new_key = apply_key_transformation(key); - Some((key.clone(), new_key, value.clone())) - } else { - None - } - }) - .collect(); - - for (old_key, new_key, value) in replacements { - schema_obj.properties.remove(&old_key); - schema_obj.properties.insert(new_key, value); - } + transform_property_keys(&mut schema_obj); // Enrich every resource reference in the schema — including those // inside `items`, nested `properties`, etc. — with a description diff --git a/backend/windmill-mcp/src/common/mod.rs b/backend/windmill-mcp/src/common/mod.rs index 60ad18a85c..2632a28962 100644 --- a/backend/windmill-mcp/src/common/mod.rs +++ b/backend/windmill-mcp/src/common/mod.rs @@ -11,8 +11,8 @@ pub mod types; pub use schema::convert_schema_to_schema_type; pub use scope::{is_resource_allowed, parse_mcp_scopes, McpScopeConfig}; pub use transform::{ - apply_key_transformation, extract_hub_version_id_from_hashed, - extract_path_prefix_from_hashed, parse_tool_prefix, reverse_transform, reverse_transform_key, - transform_hub_path, transform_path, + apply_key_transformation, extract_hub_version_id_from_hashed, extract_path_prefix_from_hashed, + parse_tool_prefix, reverse_transform, reverse_transform_key, transform_hub_path, + transform_path, transform_property_keys, }; pub use types::*; diff --git a/backend/windmill-mcp/src/common/transform.rs b/backend/windmill-mcp/src/common/transform.rs index 0a12c85892..a477b0bd0d 100644 --- a/backend/windmill-mcp/src/common/transform.rs +++ b/backend/windmill-mcp/src/common/transform.rs @@ -4,6 +4,7 @@ //! to make them compatible with MCP tool naming requirements. use super::types::SchemaType; +use std::collections::HashSet; use windmill_common::utils::calculate_hash; /// Max tool name length. The MCP spec allows 64 chars, but some clients @@ -201,6 +202,41 @@ pub fn apply_key_transformation(key: &str) -> String { .collect::() } +/// Rename every property key that MCP argument names cannot carry, in both +/// `properties` and `required`. +/// +/// `required` names properties, so it has to follow the rename: an entry left +/// pointing at the original key names a property that no longer exists, which reads +/// to a client as "this parameter is optional". +pub fn transform_property_keys(schema_obj: &mut SchemaType) { + let renames: Vec<(String, String)> = schema_obj + .properties + .keys() + .filter(|key| key.chars().any(|c| !c.is_alphanumeric() && c != '_')) + .map(|key| (key.clone(), apply_key_transformation(key))) + .collect(); + + if renames.is_empty() { + return; + } + + for (old_key, new_key) in renames { + if let Some(value) = schema_obj.properties.remove(&old_key) { + schema_obj.properties.insert(new_key.clone(), value); + } + for name in schema_obj.required.iter_mut() { + if *name == old_key { + *name = new_key.clone(); + } + } + } + + // Two keys can collapse onto the same name (`a.b` and `ab`). `required` is + // `uniqueItems`, and a strict validator rejects the whole tool over a repeat. + let mut seen = HashSet::new(); + schema_obj.required.retain(|name| seen.insert(name.clone())); +} + /// Reverse the transformation of a key /// /// This function takes a transformed key and a schema object and reverses @@ -423,4 +459,33 @@ mod tests { assert_eq!(apply_key_transformation("key!@#"), "key"); assert_eq!(apply_key_transformation("key_123"), "key_123"); } + + #[test] + fn transform_property_keys_renames_required_alongside_properties() { + let mut schema: SchemaType = serde_json::from_value(serde_json::json!({ + "type": "object", + "properties": { "my param": { "type": "string" }, "kept": { "type": "string" } }, + "required": ["my param"], + })) + .unwrap(); + + transform_property_keys(&mut schema); + + assert!(schema.properties.contains_key("my_param")); + assert_eq!(schema.required, vec!["my_param".to_string()]); + } + + #[test] + fn transform_property_keys_does_not_repeat_a_collided_required_name() { + let mut schema: SchemaType = serde_json::from_value(serde_json::json!({ + "type": "object", + "properties": { "a.b": { "type": "string" }, "ab": { "type": "string" } }, + "required": ["a.b", "ab"], + })) + .unwrap(); + + transform_property_keys(&mut schema); + + assert_eq!(schema.required, vec!["ab".to_string()]); + } } diff --git a/backend/windmill-mcp/src/server/tools.rs b/backend/windmill-mcp/src/server/tools.rs index efdf86ed22..4fcbe086cd 100644 --- a/backend/windmill-mcp/src/server/tools.rs +++ b/backend/windmill-mcp/src/server/tools.rs @@ -4,8 +4,9 @@ //! into MCP tools. use rmcp::model::{Tool, ToolAnnotations}; +use serde_json::{Map, Value}; use std::borrow::Cow; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use crate::common::schema::{convert_schema_to_schema_type, make_schema_compatible}; @@ -124,6 +125,59 @@ impl ToolableItem for HubScriptInfo { } } +/// Placeholder values a model reaches for when it will not leave a parameter out. +const PLACEHOLDERS: &str = "`\"\"`, `[]`, `{}`, `false`, or `0`"; + +/// State the omission rule in the tool's own description. +/// +/// Models routinely fill every property of a non-strict tool schema with a placeholder +/// rather than omitting it, turning "absent" into "explicitly empty" for the code that +/// runs, and they weigh a tool's own description far more heavily than the caller's +/// system prompt. Kept terse: this repeats on every tool in the list. +fn omission_hint(input_schema: &Map, item_type: &str) -> Option { + let properties = input_schema.get("properties")?.as_object()?; + if properties.is_empty() { + return None; + } + + // A script's `required` is derived from its signature, so "not required" means the + // parameter has a default and may genuinely be left out. A flow's is a per-input + // toggle that defaults to off, so naming its inputs optional would invite the model + // to drop inputs the flow needs -- flows get the rule without the list. + if item_type == "flow" { + return Some(format!( + " Never send {PLACEHOLDERS} as a placeholder for a parameter you were not given a value for." + )); + } + + let required: HashSet<&str> = input_schema + .get("required") + .and_then(|r| r.as_array()) + .map(|names| names.iter().filter_map(|n| n.as_str()).collect()) + .unwrap_or_default(); + + let mut optional: Vec<&str> = properties + .keys() + .map(String::as_str) + .filter(|name| !required.contains(name)) + .collect(); + if optional.is_empty() { + return None; + } + // `properties` comes from an unordered map, so sort for a stable description. + optional.sort_unstable(); + + let names = optional + .iter() + .map(|name| format!("`{}`", name)) + .collect::>() + .join(", "); + + Some(format!( + " Optional parameters: {names}. Omit any you were not given a value for rather than sending {PLACEHOLDERS} as a placeholder." + )) +} + /// Create an MCP Tool from a ToolableItem /// /// The resources_cache should be pre-populated with all resource types @@ -137,7 +191,7 @@ pub fn create_tool_from_item( let is_hub = item.is_hub(); let path = item.get_transformed_path(); let item_type = item.item_type(); - let description = format!( + let mut description = format!( "This is a {} named `{}` with the following description: `{}`.{}", item_type, item.get_summary(), @@ -181,6 +235,10 @@ pub fn create_tool_from_item( } }; + if let Some(hint) = omission_hint(&input_schema_map, item_type) { + description.push_str(&hint); + } + let title = { let summary = item.get_summary(); if summary == "No summary" { @@ -204,3 +262,70 @@ pub fn create_tool_from_item( .open_world(true), // Can interact with external services ) } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn script_schema() -> Map { + json!({ + "type": "object", + "properties": { + "query": { "type": "string" }, + "page": { "type": "number", "default": 1 }, + "filters": { "type": "object" }, + }, + "required": ["query"], + }) + .as_object() + .unwrap() + .clone() + } + + #[test] + fn hint_lists_optional_params_and_never_a_required_one() { + let hint = omission_hint(&script_schema(), "script") + .expect("a schema with optional params gets a hint"); + + assert!( + hint.contains("Optional parameters: `filters`, `page`."), + "{hint}" + ); + assert!(!hint.contains("`query`"), "{hint}"); + } + + #[test] + fn no_hint_when_every_param_is_required() { + let all_required = json!({ + "type": "object", + "properties": { "query": { "type": "string" } }, + "required": ["query"], + }) + .as_object() + .unwrap() + .clone(); + + assert!(omission_hint(&all_required, "script").is_none()); + } + + #[test] + fn no_hint_for_a_parameterless_tool() { + let empty = json!({ "type": "object", "properties": {}, "required": [] }) + .as_object() + .unwrap() + .clone(); + + assert!(omission_hint(&empty, "flow").is_none()); + } + + #[test] + fn flow_hint_names_no_parameter() { + // A flow's `required` does not track defaults, so its inputs must never be + // advertised as optional. + let hint = omission_hint(&script_schema(), "flow").expect("flows still get the rule"); + + assert!(!hint.contains("Optional parameters"), "{hint}"); + assert!(!hint.contains("`page`"), "{hint}"); + } +}