diff --git a/backend/windmill-api/src/mcp/utils.rs b/backend/windmill-api/src/mcp/utils.rs index ce5cb478cf..d15a580bb0 100644 --- a/backend/windmill-api/src/mcp/utils.rs +++ b/backend/windmill-api/src/mcp/utils.rs @@ -114,7 +114,14 @@ pub async fn get_resources_types( ) -> Result, ErrorData> { let mut sqlb = SqlBuilder::select_from("resource_type as o"); sqlb.fields(&["o.name", "o.description"]); - sqlb.and_where("o.workspace_id = ?".bind(&workspace_id)); + // Every built-in resource type (slack, postgres, openai, ...) lives in the `admins` + // workspace, so a workspace-only filter finds almost none of them and the tool + // builder falls back to describing the parameter as a plain object, dropping the + // `$res:` instruction the model needs. Mirrors `list_resource_types`. + sqlb.and_where("(o.workspace_id = ? OR o.workspace_id = 'admins')".bind(&workspace_id)); + // A workspace may redefine a built-in type name; its own row sorts first so the + // name lookup picks it over the `admins` one. + sqlb.order_asc("(o.workspace_id = 'admins')"); let sql = sqlb.sql().map_err(|e| { tracing::error!("failed to build sql: {}", e); ErrorData::internal_error(format!("failed to build sql: {}", e), None) diff --git a/backend/windmill-mcp/src/common/schema.rs b/backend/windmill-mcp/src/common/schema.rs index d49fa468ae..2684287ad3 100644 --- a/backend/windmill-mcp/src/common/schema.rs +++ b/backend/windmill-mcp/src/common/schema.rs @@ -99,20 +99,26 @@ fn apply_resource_enrichment( .iter() .find(|rt| rt.name == resource_type_key); let resources_count = resource_cache.len(); + let availability = if resources_count == 0 { + "This resource does not have any available instances, you should create one from your windmill workspace." + } else if resources_count > 1 { + "This resource has multiple available instances, you should precisely select the one you want to use." + } else { + "There is 1 resource available." + }; + // The `$res:` sentence is the only thing telling the model how to fill this + // argument, so it is stated whether or not the workspace declares the type. let description = match resource_type { Some(rt) => format!( "This is a resource named `{}` with the following description: `{}`.\nPass it as the bare string `$res:` — the whole value of this argument, never an object wrapper like {{\"$res\": \"\"}} and never a plain path.\n{}", rt.name, rt.description.as_deref().unwrap_or("No description"), - if resources_count == 0 { - "This resource does not have any available instances, you should create one from your windmill workspace." - } else if resources_count > 1 { - "This resource has multiple available instances, you should precisely select the one you want to use." - } else { - "There is 1 resource available." - } + availability + ), + None => format!( + "This is a resource of type `{}`.\nPass it as the bare string `$res:` — the whole value of this argument, never an object wrapper like {{\"$res\": \"\"}} and never a plain path.\n{}", + resource_type_key, availability ), - None => "An object parameter.".to_string(), }; prop_map.insert("type".to_string(), Value::String("string".to_string())); prop_map.insert("description".to_string(), Value::String(description)); @@ -120,6 +126,21 @@ fn apply_resource_enrichment( // regardless of whether `make_schema_compatible` runs after us. (Its strip // only fires while `type == "resource"`, which is no longer true here.) prop_map.remove("resourceType"); + // The node now takes a `$res:` string. Leaving the resource's own object shape + // behind would make `make_schema_compatible` retype it back to "object" (its + // rule: a node with `properties` is an object), and the model would send `{}`. + prop_map.remove("properties"); + prop_map.remove("required"); + prop_map.remove("items"); + // Windmill stores `""` as the empty default for a resource field; on a `$res:` + // string that reads as a usable value and the model passes it through. + if !prop_map + .get("default") + .and_then(Value::as_str) + .is_some_and(|s| s.starts_with("$res:")) + { + prop_map.remove("default"); + } if prop_map .get("format") .and_then(Value::as_str) @@ -830,6 +851,71 @@ mod tests { assert!(desc.contains("$res:f/platform/aws_dev")); } + #[test] + fn enriched_resource_stays_a_string_through_make_schema_compatible() { + // A resource param is stored with the resource's own object shape. Enrichment + // retypes it to the `$res:` string, and the leftover `properties` used to make + // `make_schema_compatible` retype it back to "object" with nothing in it -- + // the model then sent `{}` instead of a resource path. + let (cache, types) = aws_resources(); + let mut node = json!({ + "type": "object", + "format": "resource-c_aws_account", + "default": "", + "properties": {}, + "required": [] + }); + + enrich_resource_schemas(&mut node, &cache, &types); + make_schema_compatible(&mut node); + + assert_eq!(node["type"], json!("string")); + assert!(node.get("properties").is_none()); + assert!(node.get("default").is_none()); + assert!(node["description"] + .as_str() + .unwrap() + .contains("$res:f/platform/aws_dev")); + } + + #[test] + fn enriched_resource_keeps_a_res_default() { + let (cache, types) = aws_resources(); + let mut node = json!({ + "type": "object", + "format": "resource-c_aws_account", + "default": "$res:f/platform/aws_dev" + }); + + enrich_resource_schemas(&mut node, &cache, &types); + + assert_eq!(node["default"], json!("$res:f/platform/aws_dev")); + } + + #[test] + fn undeclared_resource_type_still_states_the_res_form() { + // The workspace has an instance but no `resource_type` row for it. The + // description is the only place the `$res:` form is stated, so it must + // survive the missing declaration. + let mut cache = HashMap::new(); + cache.insert( + "slack".to_string(), + vec![ResourceInfo { + path: "f/examples/slack".to_string(), + description: None, + resource_type: "slack".to_string(), + }], + ); + let mut node = json!({ "type": "object", "format": "resource-slack" }); + + enrich_resource_schemas(&mut node, &cache, &[]); + + assert_eq!(node["type"], json!("string")); + let desc = node["description"].as_str().unwrap(); + assert!(desc.contains("$res:f/examples/slack"), "{desc}"); + assert!(desc.contains("`slack`"), "{desc}"); + } + #[test] fn enrich_is_noop_when_resource_type_not_in_cache() { let mut node = json!({ diff --git a/backend/windmill-mcp/src/common/transform.rs b/backend/windmill-mcp/src/common/transform.rs index a477b0bd0d..2402fc2a92 100644 --- a/backend/windmill-mcp/src/common/transform.rs +++ b/backend/windmill-mcp/src/common/transform.rs @@ -69,7 +69,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) @@ -216,10 +220,6 @@ pub fn transform_property_keys(schema_obj: &mut SchemaType) { .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); @@ -231,8 +231,10 @@ pub fn transform_property_keys(schema_obj: &mut SchemaType) { } } - // 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. + // A repeat reaches here either from two keys collapsing onto the same name + // (`a.b` and `ab`) or straight out of the stored schema. `required` is + // `uniqueItems`, and a strict validator rejects the whole tool over a repeat -- + // the tool then vanishes from the client's list rather than failing loudly. let mut seen = HashSet::new(); schema_obj.required.retain(|name| seen.insert(name.clone())); } @@ -430,7 +432,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); @@ -488,4 +493,21 @@ mod tests { assert_eq!(schema.required, vec!["ab".to_string()]); } + + #[test] + fn transform_property_keys_dedupes_required_without_any_rename() { + // A duplicate straight out of the stored schema, with no key needing a + // rename. `required` is `uniqueItems`, so a repeat makes a strict client + // drop the whole tool from its list. + let mut schema: SchemaType = serde_json::from_value(serde_json::json!({ + "type": "object", + "properties": { "user": { "type": "string" } }, + "required": ["user", "user"], + })) + .unwrap(); + + transform_property_keys(&mut schema); + + assert_eq!(schema.required, vec!["user".to_string()]); + } }