fix: bound the MCP optional-params hint and correct its wording

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::sanitize_schema_property_keys;
 use windmill_mcp::common::types::{
     FlowInfo, HubScriptInfo, ResourceInfo, ResourceType, SchemaType, ScriptInfo, WorkspaceInfo,
 };
@@ -194,31 +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);
-            // `required` names properties; leaving the untransformed name behind marks a
-            // property that no longer exists as required, and drops the real one from the
-            // required set.
-            for name in schema_obj.required.iter_mut() {
-                if *name == old_key {
-                    *name = new_key.clone();
-                }
-            }
-            schema_obj.properties.insert(new_key, value);
-        }
+        sanitize_schema_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/transform.rs

@@ -3,6 +3,8 @@
 //! Contains functions for transforming paths, keys, and other identifiers
 //! to make them compatible with MCP tool naming requirements.
 
+use serde_json::Value;
+
 use super::types::SchemaType;
 use windmill_common::utils::calculate_hash;
 
@@ -68,7 +70,11 @@ pub fn transform_hub_path(version_id: u64, summary: &str) -> String {
 /// Returns `(type_str, is_hub, is_hashed)`.
 /// Hashed names use an uppercase first character as the signal.
 pub fn parse_tool_prefix(name: &str) -> Result<(&str, bool, bool), String> {
-    let is_hashed = name.chars().next().map(|c| c.is_ascii_uppercase()).unwrap_or(false);
+    let is_hashed = name
+        .chars()
+        .next()
+        .map(|c| c.is_ascii_uppercase())
+        .unwrap_or(false);
     let lower = name.to_ascii_lowercase();
     let (type_str, is_hub) = if lower.starts_with("hs-") {
         ("script", true)
@@ -201,6 +207,31 @@ pub fn apply_key_transformation(key: &str) -> String {
         .collect::<String>()
 }
 
+/// Rename every schema property whose key is not a valid identifier, in place.
+///
+/// `required` names properties, so it has to follow the rename: left alone, it marks a
+/// property that no longer exists as required and drops the real one from the required set.
+/// `reverse_transform_key` maps arguments back at call time and consults only `properties`,
+/// so it is unaffected either way.
+pub fn sanitize_schema_property_keys(schema_obj: &mut SchemaType) {
+    let replacements: Vec<(String, String, Value)> = schema_obj
+        .properties
+        .iter()
+        .filter(|(key, _)| key.chars().any(|c| !c.is_alphanumeric() && c != '_'))
+        .map(|(key, value)| (key.clone(), apply_key_transformation(key), value.clone()))
+        .collect();
+
+    for (old_key, new_key, value) in replacements {
+        schema_obj.properties.remove(&old_key);
+        for name in schema_obj.required.iter_mut() {
+            if *name == old_key {
+                *name = new_key.clone();
+            }
+        }
+        schema_obj.properties.insert(new_key, value);
+    }
+}
+
 /// Reverse the transformation of a key
 ///
 /// This function takes a transformed key and a schema object and reverses
@@ -394,7 +425,10 @@ mod tests {
     #[test]
     fn test_extract_path_prefix_handles_hs_prefix() {
         // Hs- is 3 chars, not 2 — ensure the prefix is stripped correctly
-        let hashed = transform_hub_path(12345, "a]very long hub script summary that exceeds the limit");
+        let hashed = transform_hub_path(
+            12345,
+            "a]very long hub script summary that exceeds the limit",
+        );
         let (_, is_hub, is_hashed) = parse_tool_prefix(&hashed).unwrap();
         assert!(is_hub);
         assert!(is_hashed);
@@ -423,4 +457,27 @@ mod tests {
         assert_eq!(apply_key_transformation("key!@#"), "key");
         assert_eq!(apply_key_transformation("key_123"), "key_123");
     }
+
+    #[test]
+    fn sanitizing_a_property_key_renames_it_in_required_too() {
+        let mut schema = SchemaType {
+            r#type: "object".to_string(),
+            properties: [
+                ("my key".to_string(), serde_json::json!({"type": "string"})),
+                ("plain".to_string(), serde_json::json!({"type": "string"})),
+            ]
+            .into_iter()
+            .collect(),
+            required: vec!["my key".to_string(), "plain".to_string()],
+        };
+
+        sanitize_schema_property_keys(&mut schema);
+
+        assert!(schema.properties.contains_key("my_key"));
+        assert!(!schema.properties.contains_key("my key"));
+        // Stale entry would mark a nonexistent property required and drop the real one.
+        assert!(schema.required.contains(&"my_key".to_string()));
+        assert!(!schema.required.contains(&"my key".to_string()));
+        assert!(schema.required.contains(&"plain".to_string()));
+    }
 }

backend/windmill-mcp/src/server/tools.rs

@@ -124,15 +124,29 @@ impl ToolableItem for HubScriptInfo {
     }
 }
 
+/// OpenAI rejects a chat-completions request outright when any `tools[].function.description`
+/// exceeds this, and the agent step copies an MCP tool's description into that field verbatim.
+/// One over-long tool would fail the request for every tool in the step, so the hint below is
+/// only ever appended when it fits in what the base description leaves.
+const MAX_TOOL_DESCRIPTION_CHARS: usize = 1024;
+
 /// Spell out, in the tool description, that optional parameters may be left out.
 ///
-/// A runnable applies its own defaults for every argument the caller omits, so an
-/// optional parameter never needs a placeholder. Models given a non-strict tool schema
-/// otherwise tend to fill in every property with a type-zero value (`""`, `[]`, `0`,
-/// `false`); those reach the runnable as real arguments and override its defaults.
-/// `required` alone does not deter this, and a client-side system prompt is weighed far
-/// less than the tool's own description.
-fn optional_params_hint(schema: &SchemaType) -> Option<String> {
+/// Models given a non-strict tool schema tend to fill every property with a type-zero
+/// value (`""`, `[]`, `0`, `false`), which reaches the runnable as a real argument rather
+/// than as the absent value it stands in for. `required` alone does not deter this, and a
+/// client-side system prompt is weighed far less than the tool's own description.
+///
+/// The wording must not promise that an omitted parameter falls back to a default: only a
+/// script gets that, from its own language-level default. Nothing applies schema defaults
+/// server-side, so an omitted flow input is simply absent.
+///
+/// Returns the longest form that fits `budget` characters: naming the parameters is worth
+/// more than the generic sentence alone, but the names are the unbounded part and are
+/// already in the schema, so they are the first thing dropped.
+fn optional_params_hint(schema: &SchemaType, budget: usize) -> Option<String> {
+    const INSTRUCTION: &str = "Omit any parameter the request does not call for; leaving one out is always valid and is not the same as passing an empty value. Never pass an empty string, empty array, empty object, `false`, or `0` as a placeholder for a value you were not given.";
+
     let mut optional = schema
         .properties
         .keys()
@@ -148,10 +162,21 @@ fn optional_params_hint(schema: &SchemaType) -> Option<String> {
     // in the cached prefix of a provider request, which only matches when byte-identical.
     optional.sort_unstable();
 
-    Some(format!(
-        " Optional parameters: {}. Omit any parameter the request does not call for, and it falls back to its default. Never pass an empty string, empty array, empty object, `false`, or `0` as a placeholder for a value you were not given.",
-        optional.join(", ")
-    ))
+    let enumerated = format!(
+        " Optional parameters: {}. {}",
+        optional.join(", "),
+        INSTRUCTION
+    );
+    if enumerated.chars().count() <= budget {
+        return Some(enumerated);
+    }
+
+    let generic = format!(" {}", INSTRUCTION);
+    if generic.chars().count() <= budget {
+        return Some(generic);
+    }
+
+    None
 }
 
 /// Create an MCP Tool from a ToolableItem
@@ -172,9 +197,8 @@ pub fn create_tool_from_item<T: ToolableItem, B: McpBackend>(
     let schema_obj =
         backend.transform_schema_for_resources(&schema, resources_cache, resources_types);
 
-    // Derived from the transformed schema, so the names match the ones the client sees.
-    let description = format!(
-        "This is a {} named `{}` with the following description: `{}`.{}{}",
+    let base_description = format!(
+        "This is a {} named `{}` with the following description: `{}`.{}",
         item_type,
         item.get_summary(),
         item.get_description(),
@@ -186,10 +210,17 @@ pub fn create_tool_from_item<T: ToolableItem, B: McpBackend>(
             )
         } else {
             "".to_string()
-        },
-        optional_params_hint(&schema_obj).unwrap_or_default()
+        }
     );
 
+    // The hint is derived from the transformed schema, so the names it lists are the ones
+    // the client receives, and it only gets appended if it fits alongside the base.
+    let budget = MAX_TOOL_DESCRIPTION_CHARS.saturating_sub(base_description.chars().count());
+    let description = match optional_params_hint(&schema_obj, budget) {
+        Some(hint) => format!("{}{}", base_description, hint),
+        None => base_description,
+    };
+
     let input_schema_map = match serde_json::to_value(schema_obj) {
         Ok(mut value) => {
             make_schema_compatible(&mut value);
@@ -255,8 +286,11 @@ mod tests {
 
     #[test]
     fn hint_lists_every_optional_param_sorted() {
-        let hint = optional_params_hint(&schema(&["query", "sort", "filters", "page"], &["query"]))
-            .expect("a schema with optional params must produce a hint");
+        let hint = optional_params_hint(
+            &schema(&["query", "sort", "filters", "page"], &["query"]),
+            MAX_TOOL_DESCRIPTION_CHARS,
+        )
+        .expect("a schema with optional params must produce a hint");
 
         assert!(
             hint.starts_with(" Optional parameters: `filters`, `page`, `sort`."),
@@ -265,9 +299,37 @@ mod tests {
         assert!(!hint.contains("`query`"), "required param listed: {hint}");
     }
 
+    /// OpenAI 400s the whole request over a 1024-char tool description, taking every other
+    /// tool in the step down with it, so the hint sheds the parameter names and then itself
+    /// rather than overrun the budget.
+    #[test]
+    fn hint_degrades_then_disappears_as_the_budget_shrinks() {
+        let many = (0..200).map(|i| format!("param_{i}")).collect::<Vec<_>>();
+        let names = many.iter().map(String::as_str).collect::<Vec<_>>();
+        let wide = schema(&names, &[]);
+
+        let full = optional_params_hint(&wide, usize::MAX).expect("unbounded budget lists names");
+        assert!(full.contains("`param_0`"), "expected names: {full}");
+
+        let generic = optional_params_hint(&wide, 400).expect("a tight budget keeps the sentence");
+        assert!(
+            !generic.contains("`param_0`"),
+            "names not dropped: {generic}"
+        );
+        assert!(generic.chars().count() <= 400, "over budget: {generic}");
+
+        assert_eq!(optional_params_hint(&wide, 10), None);
+    }
+
     #[test]
     fn no_hint_when_every_param_is_required() {
-        assert_eq!(optional_params_hint(&schema(&["query"], &["query"])), None);
-        assert_eq!(optional_params_hint(&schema(&[], &[])), None);
+        assert_eq!(
+            optional_params_hint(&schema(&["query"], &["query"]), MAX_TOOL_DESCRIPTION_CHARS),
+            None
+        );
+        assert_eq!(
+            optional_params_hint(&schema(&[], &[]), MAX_TOOL_DESCRIPTION_CHARS),
+            None
+        );
     }
 }