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) <noreply@anthropic.com>

* refactor: make the mcp property-key rename testable and shorten the hint

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: keep the mcp omission hint from calling flow inputs optional

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: skip the mcp omission hint on a parameterless tool

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

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

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::*;

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::<String>()
 }
 
+/// 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()]);
+    }
 }

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<String, Value>, item_type: &str) -> Option<String> {
+    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::<Vec<_>>()
+        .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<T: ToolableItem, B: McpBackend>(
     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<T: ToolableItem, B: McpBackend>(
         }
     };
 
+    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<T: ToolableItem, B: McpBackend>(
             .open_world(true), // Can interact with external services
     )
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use serde_json::json;
+
+    fn script_schema() -> Map<String, Value> {
+        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}");
+    }
+}