diff --git a/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py b/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py index 3ed9cc259d..b2fbc18109 100644 --- a/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py +++ b/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py @@ -147,7 +147,14 @@ def extract_separate_schemas(parameters: List[Dict[str, Any]], request_body: Opt else: # Log warning when a required field is missing from schema properties print(f"Warning: Required field '{field}' not found in body schema properties", file=sys.stderr) - + + # Marks a body the MCP layer must refuse to leave empty (see non_empty_body_fields + # in windmill-mcp). A pass-through body (no declared properties, e.g. + # runScriptByPath) is excluded: `{}` is a valid body there, for a runnable that + # takes no arguments. + if body_schema and request_body.get('required') and body_schema.get('properties'): + body_schema['minProperties'] = 1 + # Sanitize empty schemas for JSON Schema draft 2020-12 compliance path_params_schema = sanitize_empty_schemas(path_params_schema) query_params_schema = sanitize_empty_schemas(query_params_schema) diff --git a/backend/windmill-api/src/mcp/auto_generated_endpoints.rs b/backend/windmill-api/src/mcp/auto_generated_endpoints.rs index 6971c4a715..86b9fdfb11 100644 --- a/backend/windmill-api/src/mcp/auto_generated_endpoints.rs +++ b/backend/windmill-api/src/mcp/auto_generated_endpoints.rs @@ -162,7 +162,8 @@ pub fn all_tools() -> Vec { "value", "is_secret", "description" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, @@ -244,7 +245,8 @@ pub fn all_tools() -> Vec { "type": "string", "description": "The path to the variable (body parameter). Defaults to `path` when omitted; set it only to change the path." } - } + }, + "minProperties": 1 })), query_field_renames: None, body_field_renames: Some(serde_json::json!({ @@ -392,7 +394,8 @@ pub fn all_tools() -> Vec { "path", "value", "resource_type" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, @@ -464,7 +467,8 @@ pub fn all_tools() -> Vec { "type": "string", "description": "The path to the resource (body parameter). Defaults to `path` when omitted; set it only to change the path." } - } + }, + "minProperties": 1 })), query_field_renames: None, body_field_renames: Some(serde_json::json!({ @@ -719,7 +723,8 @@ Creates a new version of an existing script when called with the same path and t "summary", "content", "language" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, @@ -980,7 +985,8 @@ Creates a new version of an existing script when called with the same path and t "value", "path" ], - "description": "Top-level flow definition containing metadata, configuration, and the flow structure" + "description": "Top-level flow definition containing metadata, configuration, and the flow structure", + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, @@ -1035,7 +1041,8 @@ Creates a new version of an existing script when called with the same path and t "summary", "value" ], - "description": "Top-level flow definition containing metadata, configuration, and the flow structure" + "description": "Top-level flow definition containing metadata, configuration, and the flow structure", + "minProperties": 1 })), query_field_renames: None, body_field_renames: Some(serde_json::json!({ @@ -1226,7 +1233,8 @@ Creates a new version of an existing script when called with the same path and t "value", "summary", "policy" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, @@ -1341,7 +1349,8 @@ Creates a new version of an existing script when called with the same path and t }, "required": [ "value" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: Some(serde_json::json!({ @@ -1463,7 +1472,8 @@ Creates a new version of an existing script when called with the same path and t "args", "content", "language" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, @@ -2034,7 +2044,8 @@ You should get the schema of the script or flow before creating the schedule to "script_path", "is_flow", "args" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, @@ -2239,7 +2250,8 @@ You should get the schema of the script or flow before updating the schedule to "schedule", "timezone", "args" - ] + ], + "minProperties": 1 })), query_field_renames: None, body_field_renames: None, diff --git a/backend/windmill-api/src/mcp/core.rs b/backend/windmill-api/src/mcp/core.rs index 971f8a890f..2b4350bcfe 100644 --- a/backend/windmill-api/src/mcp/core.rs +++ b/backend/windmill-api/src/mcp/core.rs @@ -291,14 +291,7 @@ impl McpBackend for WindmillBackend { ); // Prepare request body - let body_json = build_request_body( - &endpoint_tool.method, - args_map, - &endpoint_tool.body_schema, - &endpoint_tool.body_field_renames, - &endpoint_tool.path_params_schema, - &endpoint_tool.query_params_schema, - ); + let body_json = build_request_body(endpoint_tool, args_map)?; // Create and execute request let response = create_http_request( diff --git a/backend/windmill-api/src/mcp/utils.rs b/backend/windmill-api/src/mcp/utils.rs index 6dd2823959..9802d7acab 100644 --- a/backend/windmill-api/src/mcp/utils.rs +++ b/backend/windmill-api/src/mcp/utils.rs @@ -15,7 +15,9 @@ use windmill_common::scripts::{get_full_hub_script_by_path, Schema}; use windmill_common::utils::{query_elems_from_hub, StripPath}; use windmill_common::worker::to_raw_value; use windmill_common::{DB, HUB_BASE_URL}; -use windmill_mcp::server::{BackendResult, ErrorData, PathFilter}; +use windmill_mcp::server::{ + non_empty_body_fields, BackendResult, EndpointTool, ErrorData, PathFilter, +}; use windmill_mcp::{HubResponse, HubScriptInfo, ItemSchema, ResourceInfo, ResourceType}; use crate::db::ApiAuthed; @@ -448,15 +450,44 @@ pub fn build_query_string( } } -/// Build request body from arguments +/// Build request body from arguments, refusing a call that would reach an +/// endpoint requiring a body without one. pub fn build_request_body( - method: &str, + tool: &EndpointTool, + args_map: &serde_json::Map, +) -> BackendResult> { + let body = assemble_request_body(tool, args_map); + + // The fields that would have satisfied it are not expressible in the tool's input + // schema, so name them here rather than dispatching a call that cannot succeed. + if body.is_none() { + if let Some(fields) = non_empty_body_fields(tool) { + return Err(ErrorData::invalid_params( + format!( + "{} needs a request body: provide at least one of {}", + tool.name, + fields.join(", ") + ), + None, + )); + } + } + + Ok(body) +} + +fn assemble_request_body( + tool: &EndpointTool, args_map: &serde_json::Map, - body_schema: &Option, - body_field_renames: &Option, - path_params_schema: &Option, - query_params_schema: &Option, ) -> Option { + let EndpointTool { + method, + body_schema, + body_field_renames, + path_params_schema, + query_params_schema, + .. + } = tool; if method == "GET" { return None; } @@ -805,27 +836,64 @@ mod tests { ); } + fn endpoint_tool( + method: &'static str, + path_params_schema: Option, + body_schema: Option, + body_field_renames: Option, + ) -> EndpointTool { + EndpointTool { + name: std::borrow::Cow::Borrowed("testTool"), + description: std::borrow::Cow::Borrowed("desc"), + instructions: std::borrow::Cow::Borrowed(""), + path: std::borrow::Cow::Borrowed("/w/{workspace}/test/{path}"), + method: std::borrow::Cow::Borrowed(method), + path_params_schema, + query_params_schema: None, + body_schema, + query_field_renames: None, + body_field_renames, + } + } + + fn args_of(value: Value) -> serde_json::Map { + value.as_object().unwrap().clone() + } + + fn path_param_schema() -> Option { + Some(json!({ + "type": "object", + "properties": { "path": { "type": "string" } }, + "required": ["path"] + })) + } + + fn generated_tool(name: &str) -> EndpointTool { + crate::mcp::auto_generated_endpoints::all_tools() + .into_iter() + .find(|t| t.name.as_ref() == name) + .unwrap_or_else(|| panic!("{name} must be a generated endpoint tool")) + } + #[test] fn build_request_body_passthrough_forwards_script_args_minus_path() { // runScriptByPath-shaped body: additionalProperties, no declared props. // `path` is a path param and must be excluded; the rest are the script's // arguments and must be forwarded verbatim. - let body_schema = Some(json!({ "type": "object", "additionalProperties": true })); - let path_schema = Some(json!({ - "type": "object", - "properties": { "path": { "type": "string" } }, - "required": ["path"] - })); - let args: serde_json::Map = json!({ + let tool = endpoint_tool( + "POST", + path_param_schema(), + Some(json!({ "type": "object", "additionalProperties": true })), + None, + ); + let args = args_of(json!({ "path": "u/admin/my_script", "name": "alice", "count": 3 - }) - .as_object() - .unwrap() - .clone(); + })); - let body = build_request_body("POST", &args, &body_schema, &None, &path_schema, &None) + let body = build_request_body(&tool, &args) + .unwrap() .expect("passthrough body should be built"); let obj = body.as_object().unwrap(); assert_eq!(obj.get("name"), Some(&json!("alice"))); @@ -839,16 +907,18 @@ mod tests { #[test] fn build_request_body_declared_props_only_forwards_declared() { // Endpoints with explicit properties keep the strict declared-only behavior. - let body_schema = Some(json!({ - "type": "object", - "properties": { "value": { "type": "string" } }, - "required": ["value"] - })); - let args: serde_json::Map = json!({ "value": "x", "sneaky": "y" }) - .as_object() - .unwrap() - .clone(); - let body = build_request_body("POST", &args, &body_schema, &None, &None, &None).unwrap(); + let tool = endpoint_tool( + "POST", + None, + Some(json!({ + "type": "object", + "properties": { "value": { "type": "string" } }, + "required": ["value"] + })), + None, + ); + let args = args_of(json!({ "value": "x", "sneaky": "y" })); + let body = build_request_body(&tool, &args).unwrap().unwrap(); let obj = body.as_object().unwrap(); assert_eq!(obj.get("value"), Some(&json!("x"))); assert!( @@ -859,13 +929,10 @@ mod tests { // updateFlow-shaped: `path` is both a path parameter and a body field. The path // parameter keeps the plain name; only the body side is mangled. - fn update_flow_schemas() -> (Option, Option, Option) { - ( - Some(json!({ - "type": "object", - "properties": { "path": { "type": "string" } }, - "required": ["path"] - })), + fn update_flow_tool() -> EndpointTool { + endpoint_tool( + "POST", + path_param_schema(), Some(json!({ "type": "object", "properties": { @@ -873,7 +940,8 @@ mod tests { "value": { "type": "object" }, "path__body": { "type": "string" } }, - "required": ["summary", "value"] + "required": ["summary", "value"], + "minProperties": 1 })), Some(json!({ "path__body": "path" })), ) @@ -884,26 +952,16 @@ mod tests { // The mangled body field carries the *new* path when renaming; it must reach the // API under its original name `path`. (An omitted `path__body` is intentionally // absent from the body; the server defaults it from the URL path parameter.) - let (path_schema, body_schema, body_renames) = update_flow_schemas(); - let args: serde_json::Map = json!({ + let args = args_of(json!({ "path": "f/team/my_flow", "path__body": "f/team/renamed_flow", "summary": "s", "value": {} - }) - .as_object() - .unwrap() - .clone(); + })); - let body = build_request_body( - "POST", - &args, - &body_schema, - &body_renames, - &path_schema, - &None, - ) - .expect("body should be built"); + let body = build_request_body(&update_flow_tool(), &args) + .unwrap() + .expect("body should be built"); assert_eq!( body.as_object().unwrap().get("path"), Some(&json!("f/team/renamed_flow")), @@ -913,9 +971,51 @@ mod tests { #[test] fn build_request_body_get_has_no_body() { - let body_schema = Some(json!({ "type": "object", "additionalProperties": true })); - let args: serde_json::Map = json!({ "a": 1 }).as_object().unwrap().clone(); - assert!(build_request_body("GET", &args, &body_schema, &None, &None, &None).is_none()); + let tool = endpoint_tool( + "GET", + None, + Some(json!({ "type": "object", "additionalProperties": true })), + None, + ); + assert!(build_request_body(&tool, &args_of(json!({ "a": 1 }))) + .unwrap() + .is_none()); + } + + #[test] + fn build_request_body_refuses_bodyless_call_to_required_body_endpoint() { + // updateVariable's only required argument is the path parameter, so a + // path-only call satisfies the tool's input schema while leaving the body + // empty. Dispatching it would reach the API with no JSON body and come back + // as a 415, so it is refused here with the fields it could have set. + let tool = generated_tool("updateVariable"); + let err = build_request_body(&tool, &args_of(json!({ "path": "u/admin/a_var" }))) + .expect_err("a path-only updateVariable call must be refused"); + assert!( + err.message.contains("value"), + "the error must name the body fields, got: {}", + err.message + ); + + let body = build_request_body( + &tool, + &args_of(json!({ "path": "u/admin/a_var", "value": "v" })), + ) + .unwrap() + .expect("a call carrying an update must still build a body"); + assert_eq!(body.as_object().unwrap().get("value"), Some(&json!("v"))); + } + + #[test] + fn build_request_body_allows_bodyless_run_of_a_no_arg_runnable() { + // The counterpart: a runnable that takes no arguments has nothing to put in + // the body, and the run endpoints accept an empty one. + let tool = generated_tool("runScriptByPath"); + assert!( + build_request_body(&tool, &args_of(json!({ "path": "u/admin/no_args"}))) + .unwrap() + .is_none() + ); } #[test] diff --git a/backend/windmill-mcp/src/server/endpoints.rs b/backend/windmill-mcp/src/server/endpoints.rs index cea2896caa..5b08d211a5 100644 --- a/backend/windmill-mcp/src/server/endpoints.rs +++ b/backend/windmill-mcp/src/server/endpoints.rs @@ -31,6 +31,22 @@ pub fn is_endpoint_read_only(tool: &EndpointTool) -> bool { tool.method.as_ref() == "GET" } +/// The body fields of an endpoint that cannot be called with an empty body, or +/// `None` when a bodyless call is legitimate. +/// +/// `minProperties` on the body schema marks an operation whose OpenAPI declares +/// `requestBody: required: true` (see `generate_mcp_tools.py`). The API answers a +/// request carrying no JSON body with 415 before the handler runs, so a call +/// filling none of these fields can only fail. +pub fn non_empty_body_fields(tool: &EndpointTool) -> Option> { + let schema = tool.body_schema.as_ref()?; + if schema.get("minProperties").and_then(|m| m.as_u64())? == 0 { + return None; + } + let props = schema.get("properties")?.as_object()?; + Some(props.keys().map(|k| k.as_str()).collect()) +} + /// Convert a single endpoint tool to MCP tool pub fn endpoint_tool_to_mcp_tool(tool: &EndpointTool) -> Tool { let mut combined_properties = serde_json::Map::new(); @@ -54,7 +70,23 @@ pub fn endpoint_tool_to_mcp_tool(tool: &EndpointTool) -> Tool { }); make_schema_compatible(&mut combined_schema); - let description = format!("{}. {}", tool.description, tool.instructions); + let mut description = format!("{}. {}", tool.description, tool.instructions); + + // A body that must not be empty, none of whose fields is individually required, + // is a requirement no `required` array can express. Spell it out, or the schema + // reads as if the path alone were a complete call. + if let Some(fields) = non_empty_body_fields(tool) { + if !fields + .iter() + .any(|f| combined_required.iter().any(|r| r == f)) + { + description = format!( + "{} At least one of these arguments must be provided: {}.", + description.trim_end(), + fields.join(", ") + ); + } + } // Create annotations based on HTTP method and endpoint characteristics let annotations = create_endpoint_annotations(tool); @@ -289,6 +321,53 @@ mod tests { ); } + /// updateVariable-shaped: the body must not be empty, but no single field of it + /// is required, so `required` cannot carry the constraint and the prose must. + fn update_tool(body_required: &[&str]) -> EndpointTool { + let mut t = tool("updateVariable", "/w/{workspace}/variables/update/{path}"); + t.method = Cow::Borrowed("POST"); + t.path_params_schema = Some(serde_json::json!({ + "type": "object", + "properties": { "path": { "type": "string" } }, + "required": ["path"] + })); + t.body_schema = Some(serde_json::json!({ + "type": "object", + "properties": { + "value": { "type": "string" }, + "is_secret": { "type": "boolean" } + }, + "required": body_required, + "minProperties": 1 + })); + t + } + + #[test] + fn required_body_with_no_required_field_is_stated_in_the_description() { + let desc = endpoint_tool_to_mcp_tool(&update_tool(&[])) + .description + .unwrap_or_default() + .to_string(); + assert!( + desc.contains("value, is_secret"), + "the description must name the body fields, got: {desc}" + ); + } + + #[test] + fn required_body_with_a_required_field_keeps_its_description() { + // `required` already forbids the empty body here, so the sentence would be noise. + let desc = endpoint_tool_to_mcp_tool(&update_tool(&["value"])) + .description + .unwrap_or_default() + .to_string(); + assert!( + !desc.contains("At least one"), + "no extra sentence is needed when a body field is required, got: {desc}" + ); + } + #[test] fn list_workspaces_tool_has_no_params() { let t = list_workspaces_tool(); diff --git a/backend/windmill-mcp/src/server/mod.rs b/backend/windmill-mcp/src/server/mod.rs index 19c7e43df6..fcc0a3618d 100644 --- a/backend/windmill-mcp/src/server/mod.rs +++ b/backend/windmill-mcp/src/server/mod.rs @@ -15,7 +15,7 @@ pub use crate::common::types::{McpToken, MultiWorkspaceMcp, WorkspaceInfo}; pub use backend::{BackendResult, McpAuth, McpBackend, PathFilter}; pub use endpoints::{ endpoint_tool_to_mcp_tool, endpoint_tool_to_mcp_tool_multi, is_endpoint_read_only, - list_workspaces_tool, EndpointTool, + list_workspaces_tool, non_empty_body_fields, EndpointTool, }; pub use runner::Runner; pub use tools::create_tool_from_item; diff --git a/backend/windmill-mcp/src/server/runner.rs b/backend/windmill-mcp/src/server/runner.rs index e8248920ed..d891b737a7 100644 --- a/backend/windmill-mcp/src/server/runner.rs +++ b/backend/windmill-mcp/src/server/runner.rs @@ -607,12 +607,13 @@ impl Runner { // including the run-by-path path check (shared with multi mode). authorize_endpoint_call(scope_config, endpoint_tool, &args, read_only)?; - // This is an endpoint tool, call via backend + // This is an endpoint tool, call via backend. The backend's own error + // code is kept: a client that retries an internal error would loop on + // arguments the endpoint rejected. let result = self .backend .call_endpoint(auth, workspace_id, endpoint_tool, args) - .await - .map_err(|e| ErrorData::internal_error(e.message, None))?; + .await?; return Ok(CallToolResult::success(vec![ContentBlock::text( truncate_tool_result( @@ -865,8 +866,7 @@ impl Runner { let result = self .backend .call_endpoint(&resolved_auth, &workspace_id, endpoint_tool, args) - .await - .map_err(|e| ErrorData::internal_error(e.message, None))?; + .await?; Ok(CallToolResult::success(vec![ContentBlock::text( truncate_tool_result( diff --git a/frontend/src/lib/mcpEndpointTools.ts b/frontend/src/lib/mcpEndpointTools.ts index 17490da569..2326e46f22 100644 --- a/frontend/src/lib/mcpEndpointTools.ts +++ b/frontend/src/lib/mcpEndpointTools.ts @@ -171,7 +171,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "value", "is_secret", "description" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined @@ -253,7 +254,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "type": "string", "description": "The path to the variable (body parameter). Defaults to `path` when omitted; set it only to change the path." } - } + }, + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: { @@ -401,7 +403,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "path", "value", "resource_type" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined @@ -473,7 +476,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "type": "string", "description": "The path to the resource (body parameter). Defaults to `path` when omitted; set it only to change the path." } - } + }, + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: { @@ -727,7 +731,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "summary", "content", "language" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined @@ -988,7 +993,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "value", "path" ], - "description": "Top-level flow definition containing metadata, configuration, and the flow structure" + "description": "Top-level flow definition containing metadata, configuration, and the flow structure", + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined @@ -1043,7 +1049,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "summary", "value" ], - "description": "Top-level flow definition containing metadata, configuration, and the flow structure" + "description": "Top-level flow definition containing metadata, configuration, and the flow structure", + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: { @@ -1234,7 +1241,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "value", "summary", "policy" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined @@ -1349,7 +1357,8 @@ export const mcpEndpointTools: EndpointTool[] = [ }, "required": [ "value" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: { @@ -1471,7 +1480,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "args", "content", "language" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined @@ -2039,7 +2049,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "script_path", "is_flow", "args" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined @@ -2241,7 +2252,8 @@ export const mcpEndpointTools: EndpointTool[] = [ "schedule", "timezone", "args" - ] + ], + "minProperties": 1 }, queryFieldRenames: undefined, bodyFieldRenames: undefined