From 760000deabebaf9d670bf4a3792e9d5dbb58903d Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 20 Aug 2026 21:12:23 +0000 Subject: [PATCH] fix: hash a script version after its auto_parent lineage is resolved Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018DP2b87ZiA4EcXSVzTg2ex --- .../generate_mcp_endpoints_tools/README.md | 11 +++--- .../generate_mcp_tools.py | 14 +++++++ .../tests/scripts.rs | 38 +++++++++++++++++++ backend/windmill-api-scripts/src/scripts.rs | 37 ++++++++++-------- backend/windmill-api/openapi.yaml | 2 +- .../src/mcp/auto_generated_endpoints.rs | 2 +- frontend/src/lib/mcpEndpointTools.ts | 2 +- 7 files changed, 82 insertions(+), 24 deletions(-) diff --git a/backend/generate_mcp_endpoints_tools/README.md b/backend/generate_mcp_endpoints_tools/README.md index 57f3d78aa9..0291295911 100644 --- a/backend/generate_mcp_endpoints_tools/README.md +++ b/backend/generate_mcp_endpoints_tools/README.md @@ -36,11 +36,12 @@ To mark an endpoint as an MCP tool, add `x-mcp-tool: true` to the operation in t ### Body fields the agent never sees -`x-mcp-tool-body-constants` sets body fields on every call to the tool. They stay out of -the tool's input schema, so use it for a field whose only correct value over MCP is fixed — -exposing such a field earns nothing and invites a model to fill it with a placeholder the -API then rejects. A constant must name a real body property and must not also appear in -`x-mcp-tool-include-fields`; the generator fails otherwise. +`x-mcp-tool-body-constants` sets body fields on every call that carries a body. They stay +out of the tool's input schema, so use it for a field whose only correct value over MCP is +fixed — exposing such a field earns nothing and invites a model to fill it with a +placeholder the API then rejects. A constant must name a real body property, must not also +appear in `x-mcp-tool-include-fields`, and cannot cover every exposed property (a body made +of constants alone is never sent); the generator fails otherwise. ```yaml /w/{workspace}/scripts/create: diff --git a/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py b/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py index 57a7ad54ea..b1918ae913 100644 --- a/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py +++ b/backend/generate_mcp_endpoints_tools/generate_mcp_tools.py @@ -136,6 +136,20 @@ def extract_separate_schemas(parameters: List[Dict[str, Any]], request_body: Opt r for r in body_schema['required'] if r in include_fields ] + # A constant is never an argument: keep it out of the schema the agent sees, + # whatever the include list says. + if body_schema and body_constants and 'properties' in body_schema: + for field in body_constants: + body_schema['properties'].pop(field, None) + if 'required' in body_schema: + body_schema['required'] = [ + r for r in body_schema['required'] if r not in body_constants + ] + # Constants ride along with the caller's fields; a body made of nothing else + # reads as a pass-through body downstream and is sent without them. + if not body_schema['properties']: + raise ValueError("x-mcp-tool-body-constants cannot cover every exposed body property") + # Apply opaque_fields: simplify listed properties to {"type": "object"} if body_schema and opaque_fields and 'properties' in body_schema: for field in opaque_fields: diff --git a/backend/windmill-api-integration-tests/tests/scripts.rs b/backend/windmill-api-integration-tests/tests/scripts.rs index e5f6cb8aa8..18dd6a3fa6 100644 --- a/backend/windmill-api-integration-tests/tests/scripts.rs +++ b/backend/windmill-api-integration-tests/tests/scripts.rs @@ -458,6 +458,44 @@ async fn test_auto_parent_resolves_parent_hash(db: Pool) -> anyhow::Re Ok(()) } +/// A version's hash covers the parent it was deployed onto, so it must be computed +/// after `auto_parent` resolves that parent. Computed before, every version of a path +/// hashes as if it were the first, and restoring content the path already held +/// collides with the older version carrying it. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_auto_parent_can_restore_earlier_content(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/scripts"); + let path = "u/test-user/auto_parent_revert"; + let first = "export async function main() { return 1; }"; + + for content in [first, "export async function main() { return 2; }", first] { + let mut script = new_script(path, "v", content); + script["auto_parent"] = json!(true); + let resp = authed(client().post(format!("{base}/create"))) + .json(&script) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 201, "create: {}", resp.text().await?); + } + + let body = authed_get(port, "get/p", path) + .await + .json::() + .await?; + assert_eq!(body["content"], first, "the head must carry the restored content"); + assert_eq!( + body["parent_hashes"].as_array().unwrap().len(), + 2, + "the restored version chains onto the two before it" + ); + + Ok(()) +} + /// Regression test for GHSA-2ppx-66jv-wpw5: a path-scoped token must only see /// the scripts within its scope when listing, even though the route-level scope /// check only validates `domain:action`. Before the fix, `list_search` (and diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 3b66f7317b..2d96a9acbe 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/backend/windmill-api-scripts/src/scripts.rs @@ -1054,7 +1054,6 @@ async fn create_script_internal<'c>( // Caller-intent: CLI / git-sync deploys ask us to preserve any existing // user draft at this path instead of wiping it as part of the deploy. let skip_draft_deletion = ns.skip_draft_deletion.unwrap_or(false); - let hash = ScriptHash(hash_script(&ns)); let authed = maybe_refresh_folders(&ns.path, &w_id, authed, &db).await; let mut tx: Transaction<'_, Postgres> = user_db.begin(&authed).await?; @@ -1101,21 +1100,6 @@ async fn create_script_internal<'c>( let legacy_on_behalf_of_email = windmill_common::legacy_on_behalf_of_email(resolved_on_behalf_of.as_deref(), &w_id, &db) .await?; - if sqlx::query_scalar!( - "SELECT 1 FROM script WHERE hash = $1 AND workspace_id = $2", - hash.0, - &w_id - ) - .fetch_optional(&mut *tx) - .await? - .is_some() - { - return Err(Error::BadRequest( - "A script with same hash (hence same path, description, summary, content) already \ - exists!" - .to_owned(), - )); - }; // When auto_parent is set, serialize concurrent creates for the same (workspace, path) // so the clashing_script query always sees the latest committed head. if ns.auto_parent.unwrap_or(false) { @@ -1151,6 +1135,27 @@ async fn create_script_internal<'c>( } } + // Hashed here, once the request is fully resolved: the parent an `auto_parent` + // caller delegated, and any identity a folder default supplied above, both belong + // to the version's identity. Hashing before them makes reverting a path to content + // it already held collide with that older version. + let hash = ScriptHash(hash_script(&ns)); + if sqlx::query_scalar!( + "SELECT 1 FROM script WHERE hash = $1 AND workspace_id = $2", + hash.0, + &w_id + ) + .fetch_optional(&mut *tx) + .await? + .is_some() + { + return Err(Error::BadRequest( + "A script with same hash (hence same path, description, summary, content) already \ + exists!" + .to_owned(), + )); + }; + let parent_hashes_and_perms: Option = match (&ns.parent_hash, clashing_script) { (None, None) => Ok(None), (None, Some(s)) => Err(Error::BadRequest(format!( diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 263fe9d2e4..e703550457 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -9449,7 +9449,7 @@ paths: summary: create script description: | Creates a new script when the path does not already exist. - Creates a new version of an existing script when called with the same path. + Creates a new version of an existing script when called with the same path; the version it builds on comes from `parent_hash`, or from `auto_parent` resolving the current head. operationId: createScript x-mcp-tool: true x-mcp-instructions: "To create a NEW script, specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. To UPDATE an existing script, do NOT delete and recreate it: call this tool again with the same path. That creates a new version while preserving the script's history; the parent version is resolved for you, so you never need a script hash." diff --git a/backend/windmill-api/src/mcp/auto_generated_endpoints.rs b/backend/windmill-api/src/mcp/auto_generated_endpoints.rs index 551f26b5ae..0f6a8d8390 100644 --- a/backend/windmill-api/src/mcp/auto_generated_endpoints.rs +++ b/backend/windmill-api/src/mcp/auto_generated_endpoints.rs @@ -694,7 +694,7 @@ pub fn all_tools() -> Vec { EndpointTool { name: Cow::Borrowed("createScript"), description: Cow::Borrowed("create script: Creates a new script when the path does not already exist. -Creates a new version of an existing script when called with the same path"), +Creates a new version of an existing script when called with the same path; the version it builds on comes from `parent_hash`, or from `auto_parent` resolving the current head"), instructions: Cow::Borrowed("To create a NEW script, specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. To UPDATE an existing script, do NOT delete and recreate it: call this tool again with the same path. That creates a new version while preserving the script's history; the parent version is resolved for you, so you never need a script hash."), path: Cow::Borrowed("/w/{workspace}/scripts/create"), method: Cow::Borrowed("POST"), diff --git a/frontend/src/lib/mcpEndpointTools.ts b/frontend/src/lib/mcpEndpointTools.ts index f6165c0ec4..14e1156578 100644 --- a/frontend/src/lib/mcpEndpointTools.ts +++ b/frontend/src/lib/mcpEndpointTools.ts @@ -703,7 +703,7 @@ export const mcpEndpointTools: EndpointTool[] = [ }, { name: "createScript", - description: "create script: Creates a new script when the path does not already exist.\nCreates a new version of an existing script when called with the same path", + description: "create script: Creates a new script when the path does not already exist.\nCreates a new version of an existing script when called with the same path; the version it builds on comes from `parent_hash`, or from `auto_parent` resolving the current head", instructions: "To create a NEW script, specify the path (e.g., 'f/my_folder/my_script'), the content (source code), and the language. For TypeScript, use 'bun' unless deno-specific APIs are needed. To UPDATE an existing script, do NOT delete and recreate it: call this tool again with the same path. That creates a new version while preserving the script's history; the parent version is resolved for you, so you never need a script hash.", path: "/w/{workspace}/scripts/create", method: "POST",