fix: allow SQL args in managed // materialize scripts (#9733)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

backend/windmill-api-scripts/src/scripts.rs

@@ -1284,28 +1284,10 @@ async fn create_script_internal<'c>(
             if let Err(e) = windmill_parser::sql_materialize::classify_wrap(&ns.content) {
                 return Err(Error::BadRequest(e.message()));
             }
-            // Managed materialize strips line comments when it wraps the SELECT,
-            // so a `-- $name (TYPE)` declaration is lost while its `$name`
-            // reference survives in the embedded SELECT — it would run unbound.
-            // Managed materialize takes no SQL args (the partition is supplied by
-            // the engine, not bound). Reject declared args with a clear error.
-            if let Ok(sig) = windmill_parser_sql::parse_duckdb_sig(&ns.content) {
-                if !sig.args.is_empty() {
-                    let names = sig
-                        .args
-                        .iter()
-                        .map(|a| format!("${}", a.name))
-                        .collect::<Vec<_>>()
-                        .join(", ");
-                    return Err(Error::BadRequest(format!(
-                        "managed `// materialize` cannot take SQL arguments ({names}): wrapping your \
-                         SELECT drops the `-- $arg` declarations, so they would run unbound. The \
-                         partition is supplied by the engine — reference its value with the \
-                         `{{partition}}` token, or use `// materialize manual` to write the DDL (and \
-                         bind args) yourself."
-                    )));
-                }
-            }
+            // SQL args are supported: managed materialize strips line comments
+            // (including `-- $name (type)` declarations) when it wraps the SELECT,
+            // but the executor parses the signature from the un-wrapped script, so
+            // `$name` references in the SELECT stay bound at run time.
         }
         // `key=` (merge) and `append` are mutually exclusive reconciliation
         // strategies; append (INSERT-only) wins. Surface the conflict rather

backend/windmill-worker/src/duckdb_executor.rs

@@ -255,6 +255,14 @@ pub async fn do_duckdb(
         } else {
             None
         };
+        // Parse the signature from the ORIGINAL script: managed materialize wraps
+        // the trailing SELECT and strips line comments, which drops the
+        // `-- $name (type)` arg declarations while their `$name` references
+        // survive in the embedded SELECT. Parsing args here (pre-wrap) keeps them
+        // declared so they are still bound — and s3object args translated to
+        // `s3://` URIs — at run time.
+        let sig = parse_duckdb_sig(query)?.args;
+
         let materialized_query;
         let query: &str = match &materialize {
             Some((Some(rewritten), _)) => {
@@ -263,8 +271,6 @@ pub async fn do_duckdb(
             }
             _ => query,
         };
-
-        let sig = parse_duckdb_sig(query)?.args;
         let mut job_args = build_args_values(job, client, conn).await?;
 
         let reserved_variables =
@@ -1106,6 +1112,40 @@ mod tests {
         );
     }
 
+    // Managed `// materialize` may take SQL args (e.g. an s3object uploaded on
+    // the run form). The wrap strips line comments — including the
+    // `-- $name (type)` declarations — so the executor parses the signature from
+    // the original script (done above, before the rewrite) while the `$name`
+    // references survive inside the wrapped SELECT. This pins both halves of that
+    // contract so a regression that drops either is caught.
+    #[test]
+    fn materialize_preserves_sql_args() {
+        let script = "-- materialize ducklake://main/rows\n\
+                      -- $file (s3object)\n\
+                      SELECT * FROM read_json_auto($file)";
+
+        // The signature is recoverable from the original (un-wrapped) script.
+        let sig = parse_duckdb_sig(script).expect("sig parses").args;
+        let file_arg = sig
+            .iter()
+            .find(|a| a.name == "file")
+            .expect("`$file` declared");
+        assert_eq!(file_arg.otyp.as_deref(), Some("s3object"));
+
+        // The wrapped query still references `$file`, so the parsed sig binds it.
+        let (rewritten, _) = build_materialized_query(script, None)
+            .expect("materialize builds")
+            .expect("materialize present");
+        let rewritten = rewritten.expect("managed mode rewrites the query");
+        assert!(
+            rewritten.contains("$file"),
+            "wrapped query must keep the `$file` reference, got:\n{rewritten}"
+        );
+        // The declaration comment is gone (wrap strips line comments) — which is
+        // exactly why the sig must come from the original, not the rewrite.
+        assert!(!rewritten.contains("-- $file"));
+    }
+
     // Tests for parse_attach_db_resource function
     #[test]
     fn test_parse_attach_db_resource_postgres_res_prefix() {