fix: teach MCP tool schemas the $res: form and drop duplicate required entries

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw

backend/windmill-api/src/mcp/utils.rs

@@ -114,7 +114,14 @@ pub async fn get_resources_types(
 ) -> Result<Vec<ResourceType>, 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:<path>` 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)

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:<path>` — the whole value of this argument, never an object wrapper like {{\"$res\": \"<path>\"}} 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:<path>` — the whole value of this argument, never an object wrapper like {{\"$res\": \"<path>\"}} 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!({

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