Conversation
diegoimbert
requested review from
alpetric,
hugocasa and
rubenfiszel
as code owners
Deploying windmill with
|
| Latest commit: |
7c5b285
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3d39993c.windmill.pages.dev |
| Branch Preview URL: | https://di-run-inline-scripts-by-pat.windmill.pages.dev |
|
Claude finished @diegoimbert's task in 2m 37s —— View job Review of PR #8019: runScript inline for path and hash
SummaryThis PR adds two new API endpoints to run deployed scripts inline (bypassing the job queue), analogous to the existing
The implementation follows the existing Key Issues1. 🔴 Security: Missing Every other script execution endpoint in 2. 🟡 Code Duplication (~170 lines in worker.rs) The two new closures in 3. 🟡 Tag mismatch: Both new closures set 4. 🟢 Minor: Duplicate request body structs
5. 🟢 Empty Both closures receive empty strings for What Looks Good
Testing InstructionsTo verify these changes, use the Windmill API directly (these are backend-only endpoints with no UI). After deploying a build with the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| pub struct RunInlineScriptByPathFnParams { | ||
| pub workspace_id: String, | ||
| pub path: String, | ||
| pub args: Option<HashMap<String, Box<RawValue>>>, | ||
| pub created_by: String, | ||
| pub permissioned_as: String, | ||
| pub permissioned_as_email: String, | ||
| pub base_internal_url: String, | ||
| pub worker_name: String, | ||
| pub conn: crate::worker::Connection, | ||
| pub client: AuthedClient, | ||
| pub job_dir: String, | ||
| pub worker_dir: String, | ||
| pub killpill_rx: tokio::sync::broadcast::Receiver<()>, | ||
| } | ||
|
|
||
| pub struct RunInlineScriptByHashFnParams { | ||
| pub workspace_id: String, | ||
| pub hash: i64, | ||
| pub args: Option<HashMap<String, Box<RawValue>>>, | ||
| pub created_by: String, | ||
| pub permissioned_as: String, | ||
| pub permissioned_as_email: String, | ||
| pub base_internal_url: String, | ||
| pub worker_name: String, | ||
| pub conn: crate::worker::Connection, | ||
| pub client: AuthedClient, | ||
| pub job_dir: String, | ||
| pub worker_dir: String, | ||
| pub killpill_rx: tokio::sync::broadcast::Receiver<()>, | ||
| } | ||
|
|
There was a problem hiding this comment.
Code duplication: RunInlineScriptByPathFnParams and RunInlineScriptByHashFnParams are nearly identical.
These two structs share 11 out of 12 fields — only the identifier field differs (path: String vs hash: i64). Consider unifying them into a single struct with an enum for the identifier:
pub enum InlineScriptTarget {
Path(String),
Hash(i64),
}
pub struct RunInlineScriptFnParams {
pub workspace_id: String,
pub target: InlineScriptTarget,
pub args: Option<HashMap<String, Box<RawValue>>>,
// ... all the shared fields
}This would also let you collapse the two nearly-identical closures in worker.rs into a single implementation. Up to you whether it's worth the refactor now, but the current approach results in ~170 lines of duplicated logic in worker.rs.
There was a problem hiding this comment.
/ai can you do a commit for that
There was a problem hiding this comment.
Claude finished @diegoimbert's task in 6m 47s —— View job
Refactoring inline script code duplication
- Read current code in
windmill-common/src/jobs.rs - Read current code in
windmill-worker/src/worker.rs - Read current code in
windmill-api/src/jobs.rs - Unify
RunInlineScriptByPathFnParamsandRunInlineScriptByHashFnParamsinto a single struct with an enum - Collapse duplicate closures in
worker.rsinto a single implementation - Unify duplicate
InlineByPath/InlineByHashrequest body structs - Add missing
check_scopescall for hash endpoint - Fix duplicate lines from prior commit in
run_inline_script_by_path - Run
cargo checkto validate - Commit and push
Summary
Pushed commit a32924f with the following changes across 3 files (-84 lines net):
-
windmill-common/src/jobs.rs— ReplacedRunInlineScriptByPathFnParamsandRunInlineScriptByHashFnParamswith a singleRunInlineScriptFnParamsstruct using anInlineScriptTargetenum (Path(String)/Hash(i64)). Collapsed two function pointers inWorkerInternalServerInlineUtilsinto onerun_inline_script. -
windmill-worker/src/worker.rs— Merged the two nearly-identical closures (run_inline_script_by_path/run_inline_script_by_hash) into a singlerun_inline_scriptclosure that pattern-matches onInlineScriptTarget. Also changed the tag from"inline_preview"to"inline"since these execute deployed scripts. -
windmill-api/src/jobs.rs— Merged duplicateInlineByPath/InlineByHashstructs intoInlineScriptArgs. Extracted a sharedrun_inline_script_innerhelper. Added missingcheck_scopesto the hash endpoint. Fixed duplicate lines introduced by the prior commit.
| run_inline_script_by_path: Arc::new(|params: RunInlineScriptByPathFnParams| { | ||
| Box::pin(async move { | ||
| let db = params | ||
| .conn | ||
| .as_sql() | ||
| .ok_or_else(|| { | ||
| error::Error::InternalErr( | ||
| "run_inline_script_by_path requires a SQL connection".to_string(), | ||
| ) | ||
| })? | ||
| .clone(); | ||
| let script_hash_info = | ||
| get_latest_deployed_hash_for_path(None, db, ¶ms.workspace_id, ¶ms.path) | ||
| .await?; | ||
| let script_hash = ScriptHash(script_hash_info.hash); | ||
| let content_info = | ||
| get_script_content_by_hash(&script_hash, ¶ms.workspace_id, ¶ms.conn) | ||
| .await?; | ||
| let job = MiniPulledJob { | ||
| workspace_id: params.workspace_id, | ||
| id: Uuid::new_v4(), | ||
| args: params.args.map(Json), | ||
| parent_job: None, | ||
| created_by: params.created_by, | ||
| scheduled_for: chrono::Utc::now(), | ||
| started_at: None, | ||
| runnable_path: Some(params.path), | ||
| kind: JobKind::Script, | ||
| runnable_id: Some(script_hash), | ||
| canceled_reason: None, | ||
| canceled_by: None, | ||
| permissioned_as: params.permissioned_as, | ||
| permissioned_as_email: params.permissioned_as_email, | ||
| flow_status: None, | ||
| tag: "inline_preview".to_string(), | ||
| script_lang: content_info.language, | ||
| same_worker: true, | ||
| pre_run_error: None, | ||
| flow_innermost_root_job: None, | ||
| root_job: None, | ||
| timeout: None, | ||
| flow_step_id: None, | ||
| cache_ttl: None, | ||
| cache_ignore_s3_path: None, | ||
| priority: None, | ||
| preprocessed: None, | ||
| script_entrypoint_override: None, | ||
| trigger: None, | ||
| trigger_kind: None, | ||
| visible_to_owner: false, | ||
| permissioned_as_end_user_email: None, | ||
| runnable_settings_handle: None, | ||
| concurrent_limit: None, | ||
| concurrency_time_window_s: None, | ||
| }; | ||
| let mut mem_peak: i32 = -1; | ||
| let mut canceled_by: Option<CanceledBy> = None; | ||
| let mut column_order: Option<Vec<String>> = None; | ||
| let mut new_args: Option<HashMap<String, Box<RawValue>>> = None; | ||
| let mut occupancy_metrics = OccupancyMetrics::new(Instant::now()); | ||
| let mut has_stream: bool = false; | ||
| let mut killpill_rx = params.killpill_rx; | ||
|
|
||
| run_language_executor( | ||
| &job, | ||
| ¶ms.conn, | ||
| ¶ms.client, | ||
| None, | ||
| ¶ms.job_dir, | ||
| ¶ms.worker_dir, | ||
| &mut mem_peak, | ||
| &mut canceled_by, | ||
| ¶ms.base_internal_url, | ||
| ¶ms.worker_name, | ||
| &mut column_order, | ||
| &mut new_args, | ||
| &mut occupancy_metrics, | ||
| &mut killpill_rx, | ||
| None, | ||
| &mut has_stream, | ||
| content_info.language, | ||
| &content_info.content, | ||
| &content_info.envs, | ||
| &content_info.codebase, | ||
| &content_info.lockfile, | ||
| true, | ||
| ) |
There was a problem hiding this comment.
Empty job_dir and worker_dir passed from API handler.
Both run_inline_script_by_path and run_inline_script_by_hash receive job_dir: "" and worker_dir: "" from the API handler. While this matches run_inline_preview_script, it's worth confirming that run_language_executor can handle scripts fetched by path/hash with empty directories — preview scripts execute arbitrary code inline, but path/hash scripts may depend on lockfiles or codebases (content_info.lockfile, content_info.codebase) that could require a real working directory for dependency installation.
If this is intentional and works as expected for all supported languages, a brief comment explaining why empty strings are acceptable here would help future readers.
| #[cfg(feature = "inline_preview")] | ||
| #[derive(Debug, Deserialize)] | ||
| struct InlineByPath { | ||
| args: Option<HashMap<String, Box<JsonRawValue>>>, | ||
| } | ||
|
|
||
| #[cfg(feature = "inline_preview")] | ||
| #[derive(Debug, Deserialize)] | ||
| struct InlineByHash { | ||
| args: Option<HashMap<String, Box<JsonRawValue>>>, | ||
| } |
There was a problem hiding this comment.
Minor: InlineByPath and InlineByHash are identical structs.
Both contain only args: Option<HashMap<String, Box<JsonRawValue>>>. You could use a single type alias or struct:
#[cfg(feature = "inline_preview")]
#[derive(Debug, Deserialize)]
struct InlineScriptArgs {
args: Option<HashMap<String, Box<JsonRawValue>>>,
}This is a minor nit — using a single type makes it clearer that the request format is the same for both endpoints.
| args: params.args.map(Json), | ||
| parent_job: None, | ||
| created_by: params.created_by, | ||
| scheduled_for: chrono::Utc::now(), |
There was a problem hiding this comment.
Potential issue: tag: "inline_preview" for non-preview jobs.
Both new closures set tag: "inline_preview".to_string(), which was appropriate for the existing preview endpoint since those are genuinely preview runs. For path/hash runs, these are executing deployed, production scripts — the tag should likely reflect that (e.g., "inline" or something distinct from "inline_preview"). This could cause confusion in logging, metrics, or any tag-based routing logic.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
- Replace RunInlineScriptByPathFnParams and RunInlineScriptByHashFnParams with a single RunInlineScriptFnParams using InlineScriptTarget enum - Collapse two nearly-identical closures in worker.rs into one - Merge duplicate InlineByPath/InlineByHash into InlineScriptArgs - Extract shared run_inline_script_inner helper in API handler - Add missing check_scopes to run_inline_script_by_hash endpoint - Fix duplicate lines from prior commit in run_inline_script_by_path - Change tag from "inline_preview" to "inline" for deployed scripts Co-authored-by: Diego Imbert <diegoimbert@users.noreply.github.com>
1 participant
No description provided.