diff --git a/backend/windmill-api/src/mcp/core.rs b/backend/windmill-api/src/mcp/core.rs index ab362e2fa1..b1483926f7 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::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 diff --git a/backend/windmill-mcp/src/common/transform.rs b/backend/windmill-mcp/src/common/transform.rs index 0a12c85892..2aa57d8235 100644 --- a/backend/windmill-mcp/src/common/transform.rs +++ b/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::() } +/// 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())); + } } diff --git a/backend/windmill-mcp/src/server/tools.rs b/backend/windmill-mcp/src/server/tools.rs index b693add887..8205be4d33 100644 --- a/backend/windmill-mcp/src/server/tools.rs +++ b/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 { +/// 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 { + 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 { // 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( 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( ) } 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::>(); + let names = many.iter().map(String::as_str).collect::>(); + 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 + ); } }