Skip to content

feat: script module mode with CLI sync, preview, and WAC UI improvements#8380

Merged
rubenfiszel merged 30 commits intomainfrom
module-mode
Mar 17, 2026
Merged

feat: script module mode with CLI sync, preview, and WAC UI improvements#8380
rubenfiszel merged 30 commits intomainfrom
module-mode

Conversation

@rubenfiszel
Copy link
Contributor

@rubenfiszel rubenfiszel commented Mar 15, 2026

Summary

This PR introduces script module mode — a foundational feature that allows Windmill scripts to have companion module files, enabling the Workflow-as-Code (WAC) pattern where scripts can reference task modules via taskScript("./helper.ts").

Script Module System

  • Scripts can have associated module files in a __mod/ folder, supporting two layouts:
    • Flat layout: my_script.ts + my_script__mod/helper.ts
    • Folder layout: my_script__mod/script.ts + my_script__mod/helper.ts
  • Modules are stored as a modules JSONB column on the script table
  • Backend executors (Bun, Python) write module files to the job directory for execution
  • Modules are included in script hashing, cloning, workspace export/import

CLI: Sync Pull/Push

  • readModulesFromDisk() / writeModulesToDisk() handle reading/writing module files from __mod/ folders
  • Fix pull→push idempotency: lock: lock ?? undefined preserves empty strings (API returns lock: "" for modules without deps)
  • Module files are pushed/pulled alongside their parent script
  • handleScriptMetadata() detects folder layout via isModuleEntryPoint()

CLI: Per-Module Hash Tracking

  • Each module gets its own hash entry in wmill-lock.yaml (following the flow inline script pattern)
  • SCRIPT_TOP_HASH meta-hash combines main script hash + sorted module hashes
  • computeModuleHashes() walks the __mod/ folder and hashes each module's content + workspace deps
  • updateModuleLocks() accepts optional changedModules parameter for selective lock regeneration
  • Stale detection output shows which specific modules changed: f/test/my_script (bun) [changed modules: helper.ts]

CLI: Preview with Modules

  • wmill script preview reads modules from __mod/ and passes them in the runScriptPreviewAndWaitResult API call
  • Supports both flat and folder layouts

CLI: Unified generate-metadata

  • Module helper files are filtered out (isScriptModulePath && !isModuleEntryPoint) so they're not treated as standalone scripts
  • Module changes are detected per-module, not all-or-nothing

Backend: auto_kind Refactor

  • Renamed no_main_func: boolauto_kind: VARCHAR(20) on the script table
  • Values: NULL (normal script), "lib" (library/no main), "wac" (workflow-as-code)
  • Parser detects WAC scripts by analyzing workflow() + task() patterns in TypeScript and Python
  • Migration converts existing no_main_func = true rows to auto_kind = 'lib'

Backend: WAC v2 Execution

  • Bun executor generates a WAC v2 wrapper that imports WorkflowCtx, StepSuspend, setWorkflowCtx from windmill-client
  • Wrapper handles checkpoint/replay semantics: complete, dispatch, sleep, approval modes
  • Python executor supports WAC via wmill.workflow() / wmill.task() decorators

Frontend: WAC UI

  • WAC TypeScript/Python templates with task(), taskScript(), step(), sleep(), waitForApproval()
  • TypeScript templates shown before Python in all template selectors
  • Module tab system in ScriptEditor: add, rename (pencil icon popover on hover), delete modules
  • Fixed-width icon container (w-[32px]) prevents layout shift on hover
  • Simplified +Script button (removed dropdown), consolidated WAC actions under +Flow dropdown
  • "Import Workflow-as-Code" drawer for importing WAC scripts from YAML/JSON
  • WAC export drawer for exporting scripts as YAML

Key Files Changed

Area Files Changes
Backend types windmill-types/src/scripts.rs auto_kind field, ScriptModule struct, hash impl
Backend API windmill-api-scripts/src/scripts.rs auto_kind in create/update
Backend workers bun_executor.rs, python_executor.rs, worker.rs Module file writing, WAC v2 wrapper
Backend parsers windmill-parser-ts, windmill-parser-py WAC detection
Backend migration 20260313000000_script_auto_kind.up.sql Column rename
CLI script cli/src/commands/script/script.ts readModulesFromDisk, preview, push/pull
CLI metadata cli/src/utils/metadata.ts Hash tracking, selective lock regen
CLI generate-metadata cli/src/commands/generate-metadata/ Module filtering
Frontend editor ScriptEditor.svelte Module tabs, rename popover
Frontend builder ScriptBuilder.svelte WAC templates, ordering
Frontend actions CreateActionsFlow.svelte, CreateActionsScript.svelte WAC import/dropdown

Test plan

  • bun test test/script_modules.test.ts — module sync unit tests
  • bun test test/resource_folders_unit.test.ts — path utility tests
  • bun test test/unified_generate_metadata.test.ts — metadata generation including module modification detection
  • bun test test/sync_pull_push.test.ts — sync round-trip tests
  • bun test test/preview.test.ts — preview with modules (flat + folder layout)
  • cargo check --features zip with -D warnings — clean compilation
  • svelte-check — 0 errors in changed files
  • Manual: wmill sync pullwmill sync push produces 0 changes for scripts with modules
  • Manual: Change one module → wmill generate-metadata only regenerates that module's lock
  • Manual: wmill script preview works with WAC scripts that have modules
  • Manual: Module tab rename popover in ScriptEditor

🤖 Generated with Claude Code

rubenfiszel and others added 12 commits March 8, 2026 17:12
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add prominent comments explaining that all computation must happen
inside task/step/taskScript or it will be replayed on resume/retry.
Clarify that waitForApproval does not hold a worker and that
approve/reject URLs are available in the timeline step details.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts:
#	frontend/package-lock.json
#	frontend/package.json
…nd preview support

- Fix pull→push idempotency: use `??` instead of `||` for module lock
  field so empty strings are preserved (matches API's `lock: ""`)
- Add per-module hash tracking in wmill-lock.yaml following the flow
  inline script pattern (SCRIPT_TOP_HASH + per-module subpath hashes)
- Selective module lock regeneration: only regenerate locks for modules
  whose content actually changed, not all modules
- Use unfiltered rawWorkspaceDependencies for module hashes to match
  what updateModuleLocks passes to fetchScriptLock
- Show changed module names in stale script output for clarity
- Add module support to `script preview` command: read modules from
  __mod/ folder and pass them in the preview API request
- Add preview tests for taskScript pattern (flat and folder layout)
- Update test assertion for module stale detection output

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ename, import consolidation

- Reorder WAC template buttons: TypeScript before Python in
  ScriptBuilder, CreateActionsScript, and CreateActionsFlow
- Remove dropdown items from +Script button (simplify to direct link)
- Move "Import Workflow-as-Code" to +Flow dropdown with dedicated drawer
- Add module tab rename: pencil icon on hover opens popover with
  validation, fixed-width icon container prevents layout shift

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflicts:
- cli/src/commands/script/script.ts: keep both isRawAppPath and module filters
- frontend/package.json: keep local parser wasm paths
- frontend/package-lock.json: regenerated from main + local deps
- Restore backend/.sqlx cache from main
- Add isScriptModulePath filter to unified generate-metadata command

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Backend parser updates for WAC detection
- CLI sync/types updates for raw app path and module support
- Frontend UI polish (Dev.svelte, ScriptRow, script hash page)
- Test fixture updates

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 15, 2026

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: bcd8b4d
Status:⚡️  Build in progress...

View logs

rubenfiszel and others added 10 commits March 15, 2026 19:23
…adata

Verifies that modifying a single module file re-triggers stale
detection and only the changed module is listed, not all modules.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix hardcoded dev path in bun_executor.rs WAC v2 wrapper — use
  "windmill-client" import instead of absolute filesystem path
- Fix missed no_main_func → auto_kind rename in parser TS test
- Add modules column to clone_script SQL (windmill-common and
  windmill-api-workspaces) so cloned scripts retain their modules
- Add modules: None to RawCode structs in worker tests
- Restore complete sqlx cache (merge main's cache + our new queries)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change `.clone()` on double reference to `*k` dereference in
scripts.rs hash implementation. Update sqlx cache with new query
hashes from modified clone_script SQL.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The local file:// paths for windmill-parser-wasm-py and
windmill-parser-wasm-ts don't exist in the Cloudflare Pages build
environment. Revert to published npm versions (1.655.0).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use newly published windmill-parser-wasm-ts and windmill-parser-wasm-py
v1.657.2 which include auto_kind/WAC detection changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ackages

Regenerating package-lock.json from scratch pulled different dependency
versions causing svelte-check type errors. Instead, start from main's
lockfile and only update the two changed packages.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Script<SR> struct has a modules field (FromRow), but
fetch_script_for_update didn't SELECT modules, causing a runtime
error "no column found for name: modules" when the worker processed
dependency jobs. This was the root cause of the relock_skip test
timeout.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix modules not passed through job queue: inject _MODULES into
  PushArgs.extra when pushing Code jobs so worker can extract them
- Fix Python module imports: use relative imports (from .helper)
  and add sys.path.insert for module directory in wrapper
- Fix Python tests: use relative imports and empty lock to prevent
  pip from resolving module names as packages
- Add local file check in Bun loader for module resolution
- Ignore Bun module test (bundle mode loader integration tracked
  separately)
- Add missing modules column to fetch_script_for_update query

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Relative imports (from .helper) are not parsed as pip packages,
so the empty lock workaround is not needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Python modules:
- Use relative imports (from .helper import greet) since scripts run
  as packages
- Add sys.path.insert for module directory in wrapper to ensure local
  modules take precedence over pip packages with same name

Bun modules:
- Use bundled output (./out/main.js) as wrapper import when modules
  are present — the bundled output has module content inlined by
  Bun.build, avoiding runtime loader resolution issues
- Add local file check in loader.bun.js onResolve to short-circuit
  API URL resolution for module files on disk

Job queue:
- Inject _MODULES into PushArgs.extra when pushing Code jobs so
  the worker can extract them at execution time

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rubenfiszel and others added 4 commits March 16, 2026 08:34
…code

Critical fixes:
- Replace all CLI `no_main_func` references with `auto_kind` (string)
  to match the backend migration and API changes
- Remove duplicated `compute_python_module_dir` in worker.rs, use
  the canonical version from python_executor.rs

High priority:
- Auto-create `__init__.py` in intermediate directories for nested
  Python modules so imports like `from .utils.math import add` work
  without users manually creating __init__.py files
- Remove redundant `sys_path_insert` — relative imports use Python's
  package system, not sys.path

Medium:
- Fix lock file base name extraction: use regex to strip only the
  final extension (`.replace(/\.[^.]+$/, '')`) instead of `indexOf(".")`
  which breaks for files like `helper.test.ts`

Simplification:
- Remove dead `{#if false}` Popover block in ScriptEditor.svelte
- Guard loader.bun.js local file check to only run for relative paths
  (matching the Windows loader pattern)
- Add clarifying comment on Bun dual mechanism (build + run phases)
- Add maintenance comment on manual Hash impl for NewScript

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix sync.ts baseName extraction using indexOf(".") → regex
  (same fix as script.ts/metadata.ts, missed this instance)
- Add stale module file cleanup in writeModulesToDisk: removes files
  from __mod/ that are no longer in the modules map before writing,
  fixing the pull→push cycle that couldn't delete modules
- Log warning when _MODULES serialization fails in job push instead
  of silently dropping modules
- Use strict equality (===) for auto_kind comparison
- Exclude auto_kind from workspace export — it is auto-detected by
  the parser at deploy time from script content

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
auto_kind is auto-detected by the parser at deploy time, so the CLI
should not send it, compare it, or write it to script.yaml.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@rubenfiszel rubenfiszel marked this pull request as ready for review March 16, 2026 16:01
@claude
Copy link
Contributor

claude bot commented Mar 16, 2026

Claude finished @rubenfiszel's task in 8m 4s —— View job


Review of PR #8380: Script Module Mode

  • Gather context and understand the PR scope
  • Review backend changes (Rust: types, API, workers, parsers, migration)
  • Review CLI changes (script sync, metadata, preview)
  • Review frontend changes (ScriptEditor, ScriptBuilder, WAC UI)
  • Check for bugs, security issues, and performance concerns
  • Provide testing instructions

This is a well-architected foundational feature. The backend implementation is solid — deterministic hashing, proper SQL parameterization, good error handling, and correct data flow through the stack. The CLI module sync with per-module hash tracking is a smart design.

Below are the issues I found, organized by severity.


Bugs

1. switchToMain() missing lastSyncedCode update — ScriptEditor.svelte:214-222

When switching from a module tab back to the main script, lastSyncedCode isn't updated. This means the $effect.pre sync guard (line 195-200) will see code !== lastSyncedCode on the next external code change and may incorrectly trigger, potentially causing stale code to overwrite the editor.

function switchToMain() {
    if (activeModuleTab !== null && modules) {
        modules[activeModuleTab] = { ...modules[activeModuleTab], content: editorCode }
    }
    activeModuleTab = null
    editorCode = code
+   lastSyncedCode = code  // Missing — keep sync guard in sync
    editor?.setCode(editorCode)
}

Fix this →

2. formatAction saves module code to main script's localStorage draft — ScriptEditor.svelte:1896-1904

When editing a module (activeModuleTab !== null), pressing format saves editorCode (module content) to localStorage.setItem(path ?? 'last_save', editorCode) — overwriting the main script's draft with module code.

formatAction={async () => {
    if (activeModuleTab === null) {
        await inferSchema(editorCode)
    }
    try {
-       localStorage.setItem(path ?? 'last_save', editorCode)
+       localStorage.setItem(path ?? 'last_save', activeModuleTab === null ? editorCode : code)
    } catch (e) {

3. Non-null assertion on inferModuleLang in renameModuleScriptEditor.svelte:381

const newLang = inferModuleLang(newPath)
modules[newPath] = { ...mod, language: newLang! } // ← force unwrap

If the user renames to an unsupported extension, inferModuleLang returns undefined and newLang! silently lies to TypeScript. The validation function validateRenameModulePath should catch invalid extensions, but if it doesn't, this will set language: undefined at runtime. Consider adding the extension check to the validation, or falling back to the original language:

modules[newPath] = { ...mod, language: newLang ?? mod.language }

4. cleanDir() in writeModulesToDisk doesn't remove empty directories — cli/src/commands/script/script.ts

After deleting stale module files, the recursive cleanDir() function doesn't rmdirSync empty directories afterward. This leaves empty directory stubs in __mod/ folders after module deletion.

5. Redundant condition in runTest()ScriptEditor.svelte:565

if (activeModuleTab !== null && modules && activeModuleTab) {
//   ^^^ already checked non-null      ^^^ redundant truthy check

Minor, but the double check on activeModuleTab is confusing. The second activeModuleTab truthy check is redundant since non-null was already asserted.


Security — Defense in Depth

6. Module relpath not sanitized against path traversal — worker.rs:3984-3988

let full_path = match base_dir {
    Some(dir) => format!("{}/{}/{}", job_dir, dir, relpath),
    None => format!("{}/{}", job_dir, relpath),
};

Module paths (relpath) come from the modules HashMap keys which originate from user-provided filenames. While job directories are ephemeral and sandboxed (mitigating impact), a malicious relpath like ../../etc/something could write outside job_dir. Consider adding a path normalization check:

let canonical = std::path::Path::new(&full_path).canonicalize()?;
if !canonical.starts_with(job_dir) {
    return Err(Error::internal_err(format!("Invalid module path: {}", relpath)));
}

Or simply reject paths containing .. components. This is low-risk given job isolation but good defense-in-depth.


Design Observations

7. flushModuleState() has UI side effect — ScriptEditor.svelte:390-396

This exported function both flushes state AND resets the active tab. Parent components calling flushModuleState() before save may not expect the UI to jump back to the main tab. Consider splitting into flushModuleContent() (just saves) and resetToMain() (saves + switches tab).

8. auto_kind migration is irreversible — 20260313000000_script_auto_kind.up.sql

The migration drops no_main_func with no down migration. This follows the project's convention (most migrations lack down files), but since it renames a column, any rollback would require manual intervention. Worth noting for deployment planning.

9. WAC detection in frontend is heuristic-based — ScriptEditor.svelte:230-241

let isWacV2 = $derived.by(() => {
    const isTsWac = mainCode.includes('windmill-client') && mainCode.includes('workflow') && mainCode.includes('task')

This can false-positive on comments containing these words. The backend parser does proper AST-level analysis; the frontend check is just for UI gating (showing module tabs), so this is acceptable for now.

10. Module language inference ignores main script language — ScriptEditor.svelte:271-276

inferModuleLang('.py') returns 'python3' even in a TypeScript WAC script. Since WAC modules must match the parent script's language, consider filtering supported extensions based on the main lang prop. Currently the supportsModules guard (line 242) only allows bun/python3, but the addModule popover doesn't restrict the extension accordingly.


Testing Instructions

To verify the changes through the Windmill UI:

Module Tab System: Navigate to the script editor (create a new TypeScript or Python script). Write a WAC-style script that imports from windmill-client and uses workflow() + task(). A module tab bar should appear below the editor tabs. Click the "+" to add a module file (e.g., helper.ts), verify it opens in the editor, type some content, switch back to the main script tab, and confirm content is preserved in both. Hover over a module tab to see the pencil (rename) and X (delete) icons — try renaming and deleting modules.

WAC Templates: On the home page, click "+Flow" — a modal should appear offering "Flow Editor" vs "Workflow as Code". Choose WAC TypeScript or Python to verify the template includes workflow(), task(), and a helper module is auto-created.

Script View with Modules: Navigate to a saved script that has modules (via /scripts/get/[hash]). The detail page should show module tabs with content and lock files.

WAC Export: On a WAC script's detail page, look for the "Export WAC" action to open a drawer that exports the script as YAML or JSON.

WAC Import: From the "+Flow" dropdown, select "Import Workflow-as-Code" to open a drawer where you can paste YAML/JSON and import it as a new WAC script.


Duplicate .sqlx cache was committed at the wrong nested path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@rubenfiszel
Copy link
Contributor Author

/ai-fast do full re-review of the PR

@claude
Copy link
Contributor

claude bot commented Mar 16, 2026

Claude finished @rubenfiszel's task in 8m 15s —— View job


Full Re-Review of PR #8380: Script Module Mode

  • Review backend changes (Rust: types, API, workers, parsers, migration)
  • Review CLI changes (script sync, metadata, preview)
  • Review frontend changes (ScriptEditor, ScriptBuilder, WAC UI)
  • Check for bugs, security issues, and performance concerns
  • Post final review

Backend (Rust)

Security

1. Path traversal in write_module_files (Medium)
backend/windmill-worker/src/worker.rs — The relpath key from the modules HashMap is used directly in file path construction without sanitization:

let full_path = match base_dir {
    Some(dir) => format!("{}/{}/{}", job_dir, dir, relpath),
    None => format!("{}/{}", job_dir, relpath),
};

A malicious module key like ../../etc/passwd could write files outside the job directory. Add a check that the resolved path stays within job_dir, or reject keys containing ... Fix this →

2. No validation on auto_kind values (Low)
backend/windmill-api-scripts/src/scripts.rsauto_kind is a free-form Option<String> with no validation against accepted values ("wac", "lib", or None). Any string up to 20 chars is accepted.

3. No size limits on modules (Low)
backend/windmill-types/src/scripts.rs — The modules field has no limits on number of modules, individual content size, or total combined size.

Performance

4. load_checkpoint called for ALL non-flow jobs (Medium)
backend/windmill-worker/src/worker.rsload_checkpoint is queried for every non-flow job to decide whether to suppress verbose headers for WAC v2 replays. The vast majority of jobs are not WAC v2, so this adds a wasted DB query per job. Gate it behind an auto_kind check.

5. Double load_checkpoint for WAC v2 Bun jobs (Low)
Called once in handle_queued_job (worker.rs) and again in handle_bun_job (bun_executor.rs). Pass the result through instead of querying twice.

Bug Risk

6. Migration not idempotent (Low)
backend/migrations/20260313000000_script_auto_kind.up.sqlALTER TABLE script ADD COLUMN auto_kind VARCHAR(20) lacks IF NOT EXISTS. If migration fails after adding the column but before completing, re-running will error. (The other new migration correctly uses IF NOT EXISTS.)

7. WAC scripts hidden from default listings — behavioral change (Medium)
backend/windmill-api-scripts/src/scripts.rs — Filter changed from o.no_main_func IS NOT TRUE to o.auto_kind IS NULL. Previously WAC scripts (which had no_main_func = false) were visible; now scripts with auto_kind = 'wac' are hidden. Verify this is intentional.


CLI (TypeScript)

Security

8. Path traversal in writeModulesToDisk (Medium-High)
cli/src/commands/script/script.tsrelPath from the remote API is used in path.join(moduleFolderPath, relPath) without validation. A malicious server could return keys like ../../etc/shadow to write files outside the module folder. Add: assert(fullPath.startsWith(moduleFolderPath)) after path.join. Same issue in zip extraction in sync.ts. Fix this →

Bugs

9. clearGlobalLock prefix match can delete unrelated entries (Medium)
cli/src/utils/metadata.ts ~L1164 — Uses k.startsWith(key) which would match u/admin/script AND u/admin/script_v2. Needs a delimiter-aware check, e.g. k === key || k.startsWith(key + "+"). Fix this →

10. Flat-layout scripts with JSON metadata not found by pushParentScriptForModule (Medium)
cli/src/commands/sync/sync.ts — The fallback probes with .script.yaml but never tries .script.json. Scripts using JSON metadata in flat layout with modules won't be pushed when a module file changes. Same issue in addToChangedIfNotExists.

Minor

11. clearGlobalLock is a no-op for v1 lockfiles (Low) — No else branch when conf.version is not "v2", leaving stale entries.

12. writeModulesToDisk cleanup doesn't remove empty directories (Low) — After deleting stale files from subdirectories, empty directories are left behind.

13. Repeated lockfile read/write in loop (Low) — updateMetadataGlobalLock reads and writes the YAML lockfile N+2 times for N modules. Could batch.


Frontend (Svelte)

CLAUDE.md Compliance

14. Banned pattern: $bindable(undefined) on optional prop
frontend/src/lib/components/ScriptEditor.sveltemodules = $bindable(undefined) on an optional prop violates the CLAUDE.md ban on $bindable(default_value). Should be modules = $bindable(). Fix this →

Bugs

15. lastSyncedCode is not reactive (Medium)
frontend/src/lib/components/ScriptEditor.svelte — Declared as plain let, but read inside $effect.pre. Should be let lastSyncedCode = $state(code) to properly participate in Svelte 5 reactivity.

16. formatAction corrupts localStorage when module tab is active (Medium)
frontend/src/lib/components/ScriptEditor.svelte — When a module tab is active, formatAction saves module content under the script's path key in localStorage, not the main script content. This corrupts the last_save entry.

17. No confirmation before deleting modules (Medium UX)
frontend/src/lib/components/ScriptEditor.svelteremoveModule permanently deletes a module with no confirmation dialog. A single misclick on the X button loses all content with no undo.

Medium Issues

18. Missing error handling in WAC import (Medium)
frontend/src/lib/components/flows/CreateActionsFlow.svelteYAML.parse() / JSON.parse() in importWacRaw() are not wrapped in try-catch. Invalid user input will cause an unhandled exception.

19. pollInterval in WorkflowTimeline runs unconditionally (Medium)
frontend/src/lib/components/WorkflowTimeline.svelte — 2-second polling interval starts even when flowDone is true and no jobs can be running.

20. isWacV2 detection is fragile (Medium)
frontend/src/lib/components/ScriptEditor.svelte — String includes() checks for 'windmill-client', 'workflow', 'task' could match comments or variable names. Consider more targeted regex patterns.

21. Module tab rename reorders tabs (Low UX)
frontend/src/lib/components/ScriptEditor.svelterenameModule uses delete + re-insert on the object, which moves the renamed tab to the end of the tab bar.

22. Duplicated asWorkflowStatus / getStepResults functions (Low)
Copy-pasted into 4 files (LogPanel, FlowStatusViewerInner, run page, JobRunsPreview). Should be extracted to a shared utility.


Summary

Severity Count Key Items
High 2 Path traversal in backend write_module_files (#1) and CLI writeModulesToDisk (#8)
Medium 9 clearGlobalLock prefix bug (#9), load_checkpoint perf (#4), flat-layout JSON metadata (#10), lastSyncedCode reactivity (#15), formatAction localStorage corruption (#16), WAC listing visibility (#7), no module delete confirmation (#17), WAC import error handling (#18), unconditional polling (#19)
Low 11 Various code quality, UX, and minor correctness issues

The two path traversal issues (#1 and #8) are the most important to address before merge — they allow writing files outside intended directories if module keys contain .. segments.

Frontend (ScriptEditor.svelte):
- Fix switchToMain() missing lastSyncedCode update — prevents stale
  code sync on external changes while editing a module tab
- Fix formatAction saving module code to main script's localStorage
  draft — now saves main code when on a module tab
- Fix non-null assertion on inferModuleLang in renameModule — fall
  back to original language instead of force unwrap
- Remove redundant activeModuleTab truthy check in runTest

CLI (script.ts):
- Clean up empty directories after removing stale module files in
  writeModulesToDisk

Backend:
- Add path traversal guard in write_module_files — reject module
  paths containing ".."
- Fix dead_code warning on auto_kind field in workspace export struct

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@rubenfiszel
Copy link
Contributor Author

/ai-fast do a full review

@claude
Copy link
Contributor

claude bot commented Mar 16, 2026

Claude finished @rubenfiszel's task in 13m 26s —— View job


Full Review of PR #8380: Script Module Mode

  • Review backend changes (types, API, workers, parsers, migration)
  • Review CLI changes (script sync, metadata, preview)
  • Review frontend changes (ScriptEditor, ScriptBuilder, WAC UI)
  • Compile findings and post review

Overall this is a well-structured PR. The backend module system, deterministic hashing, WAC v2 execution model, and CLI per-module hash tracking are all solid. The path traversal fix in write_module_files (backend) was addressed in bd27a7d. Below are the remaining issues I found.


Medium Issues

1. load_checkpoint DB query runs for ALL non-flow jobsworker.rs:3317-3324

Every non-flow job (REST triggers, cron scripts, manual runs) now makes a DB query to v2_job_status just to decide whether to suppress the log header. The vast majority of jobs are not WAC v2. Gate this behind an auto_kind check:

let is_wac_replay = if job.auto_kind.as_deref() == Some("wac") {
    if let Connection::Sql(db) = conn {
        crate::wac_executor::load_checkpoint(db, &job.id)
            .await
            .map(|c| !c.completed_steps.is_empty())
            .unwrap_or(false)
    } else { false }
} else { false };

Fix this →

2. clearGlobalLock prefix match can delete unrelated script entriescli/src/utils/metadata.ts:1168

if (k.startsWith(key)) {
    delete conf.locks[k];
}

v2LockPath("u/admin/script") returns "u/admin/script". k.startsWith(key) matches both u/admin/script+SCRIPT_TOP_HASH and u/admin/script_v2+SCRIPT_TOP_HASH. Should use delimiter-aware matching:

if (k === key || k.startsWith(key + "+")) {

Fix this →

3. Missing path traversal validation in CLI writeModulesToDiskcli/src/commands/script/script.ts:642-648

The backend added .. rejection in bd27a7d, but the CLI doesn't validate relPath from the remote API before writing to disk:

for (const [relPath, mod] of Object.entries(modules)) {
    const fullPath = path.join(moduleFolderPath, relPath);
    // No validation that fullPath stays within moduleFolderPath

Add a check:

if (relPath.includes('..')) {
    log.warn(`Skipping module with path traversal: ${relPath}`);
    continue;
}

Fix this →

4. Missing try-catch in importWacRaw()CreateActionsFlow.svelte:40-46

async function importWacRaw() {
    const parsed =
        wacImportType === 'yaml' ? YAML.parse(pendingWacRaw ?? '') : JSON.parse(pendingWacRaw ?? '')

Invalid user input will throw an unhandled exception. Wrap in try-catch with a toast error message like other import handlers in the codebase.

5. WAC scripts hidden from default script listings — behavioral changescripts.rs:338

Changed from o.no_main_func IS NOT TRUE to o.auto_kind IS NULL. Previously, WAC scripts (which had no_main_func = false) were visible in default listings. Now scripts with auto_kind = 'wac' are filtered out. Verify this is intentional — users may be surprised that their WAC scripts disappear from the script browser after upgrading.


Low Issues

6. CLAUDE.md compliance: $bindable(undefined) on optional propScriptEditor.svelte:180

modules = $bindable(undefined),

Per CLAUDE.md, $bindable(default_value) on optional props is banned. Should be $bindable().

7. No confirmation dialog before deleting modulesScriptEditor.svelte:346-353

removeModule permanently deletes a module's content with no confirmation. A misclick on the small X icon loses all work with no undo. Consider adding a confirmation dialog, especially since there's no version history for unsaved modules.

8. Migration not idempotent20260313000000_script_auto_kind.up.sql:1

ALTER TABLE script ADD COLUMN auto_kind VARCHAR(20);

Lacks IF NOT EXISTS. If the migration partially fails (column added but UPDATE or DROP COLUMN fails), re-running will error. The other new migration (20260307000000) correctly uses IF NOT EXISTS.

9. Double load_checkpoint for WAC v2 Bun jobsbun_executor.rs:1124,1653

load_checkpoint is called at line 1124 (to determine wac_replay_info) and again at line 1653 (to write checkpoint.json). These are in different code paths (the second is inside the Bun execution block), but for WAC v2 jobs that go through the full path, the result from the first call could be passed through to avoid the second DB query.

10. No auto_kind value validationwindmill-api-scripts/src/scripts.rs

auto_kind is Option<String> with no validation against accepted values ("wac", "lib", or None). Any string up to 20 chars is accepted and stored. Consider adding an enum check.


Positive Observations

  • Deterministic hashing: NewScript Hash impl sorts HashMap by key before hashing — correct for script versioning
  • Path traversal guard (backend): Properly rejects .. in module paths in write_module_files
  • Python __init__.py generation: Correctly creates intermediate __init__.py files for relative imports
  • Per-module hash tracking: Smart design — only regenerates locks for changed modules
  • Frontend fixes from prior review: lastSyncedCode update in switchToMain(), localStorage guard in formatAction, newLang ?? mod.language fallback — all properly addressed
  • Empty directory cleanup: writeModulesToDisk now removes empty dirs after cleaning stale files
  • Test coverage: Good CLI test coverage for module sync, preview, hash tracking, and metadata generation

| Branch: module-mode

- Rename "Include without main function" toggle to "Include library
  scripts" in script list (ItemsList.svelte)
- Update NoMainFuncBadge: "No main" → "Library" with clearer tooltip
- Filter module file extensions by main script language — Python
  scripts only allow .py modules, TypeScript only .ts, etc.
- Split flushModuleState into flushModuleContent (no UI side-effect)
  and flushModuleState (flush + reset tab), reducing duplication
- Dynamic placeholder and hint text in add module popover based on
  main script language

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@rubenfiszel rubenfiszel merged commit 31d6660 into main Mar 17, 2026
8 of 9 checks passed
@rubenfiszel rubenfiszel deleted the module-mode branch March 17, 2026 01:20
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant