mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-16 00:03:02 +00:00
fix: hash a script version after its auto_parent lineage is resolved
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DP2b87ZiA4EcXSVzTg2ex
This commit is contained in:
co-authored by
Claude Opus 5
parent
27870e8d93
commit
760000deab
@@ -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:
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -458,6 +458,44 @@ async fn test_auto_parent_resolves_parent_hash(db: Pool<Postgres>) -> 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<Postgres>) -> 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::<serde_json::Value>()
|
||||
.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
|
||||
|
||||
@@ -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<ParentInfo> = match (&ns.parent_hash, clashing_script) {
|
||||
(None, None) => Ok(None),
|
||||
(None, Some(s)) => Err(Error::BadRequest(format!(
|
||||
|
||||
@@ -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."
|
||||
|
||||
@@ -694,7 +694,7 @@ pub fn all_tools() -> Vec<EndpointTool> {
|
||||
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"),
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user