From 555f0635ad14c1bcdb1833fd661777ad672a3443 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Wed, 12 Aug 2026 19:46:18 +0200 Subject: [PATCH] fix: address review round findings on head 3f47dc1 - backend/ and frontend/ guidance was Claude-only. Codex and Pi read AGENTS.md, not CLAUDE.md, so moving "Verifying Backend/Frontend Changes" and the $bindable ban out of the root AGENTS.md made them invisible to two of the three CLIs this repo supports. Renamed both to AGENTS.md with a one-line @AGENTS.md CLAUDE.md beside them, matching what the repo already does at the root and in ai_evals/, and retargeted the four references. - sqlx-cache.sh aborted with exit 2 and no output when .sqlx was empty: list_entries ran `ls -1 ./*.json`, and an unmatched glob under `set -euo pipefail` killed the script. An empty cache is precisely what a failed prepare leaves behind, so it broke in the one case it exists for. Replaced with a glob loop; reproduced the failure and verified the fix. - The oneshot prompt ("never leave the PR sitting in draft") contradicted the "Flip, or ask first" rule added in the same PR, which tells unattended runs to leave wide-blast-radius changes as clean drafts. The prompt now defers to the skill for the flip decision and keeps only "never stop at an unreviewed draft". - Bundled-resource references in the vendored skills were markdown links to `.agents/skills/...`, which resolve relative to the file, not the repo root. Replaced with inline paths stating they are repo-root relative. - The PR-ready calibration file was write-only: the skill said to record answers there but never to read it. It is now consulted before deciding. Co-Authored-By: Claude Opus 5 (1M context) --- .agents/skills/codebase-design/SKILL.md | 4 +- .agents/skills/domain-modeling/SKILL.md | 2 +- .../improve-codebase-architecture/SKILL.md | 2 +- .agents/skills/pr/SKILL.md | 7 +- .agents/skills/svelte-frontend/SKILL.md | 2 +- .agents/skills/update-sqlx/sqlx-cache.sh | 12 +- .webmux.yaml | 12 +- AGENTS.md | 4 +- backend/AGENTS.md | 209 +++++++++++++++++ backend/CLAUDE.md | 210 +----------------- frontend/AGENTS.md | 85 +++++++ frontend/CLAUDE.md | 86 +------ 12 files changed, 323 insertions(+), 312 deletions(-) create mode 100644 backend/AGENTS.md create mode 100644 frontend/AGENTS.md diff --git a/.agents/skills/codebase-design/SKILL.md b/.agents/skills/codebase-design/SKILL.md index c0647f5108..b7cedf4732 100644 --- a/.agents/skills/codebase-design/SKILL.md +++ b/.agents/skills/codebase-design/SKILL.md @@ -110,5 +110,5 @@ Good interfaces make testing natural: ## Going deeper -- **Deepening a cluster given its dependencies** — see [DEEPENING.md](.agents/skills/codebase-design/DEEPENING.md): dependency categories, seam discipline, and replace-don't-layer testing. -- **Exploring alternative interfaces** — see [DESIGN-IT-TWICE.md](.agents/skills/codebase-design/DESIGN-IT-TWICE.md): spin up parallel sub-agents to design the interface several radically different ways, then compare on depth, locality, and seam placement. +- **Deepening a cluster given its dependencies** — see `.agents/skills/codebase-design/DEEPENING.md` (path from the repo root): dependency categories, seam discipline, and replace-don't-layer testing. +- **Exploring alternative interfaces** — see `.agents/skills/codebase-design/DESIGN-IT-TWICE.md` (path from the repo root): spin up parallel sub-agents to design the interface several radically different ways, then compare on depth, locality, and seam placement. diff --git a/.agents/skills/domain-modeling/SKILL.md b/.agents/skills/domain-modeling/SKILL.md index 05971d6b5c..b0372a62b8 100644 --- a/.agents/skills/domain-modeling/SKILL.md +++ b/.agents/skills/domain-modeling/SKILL.md @@ -51,7 +51,7 @@ When the user states how something works, check whether the code agrees. If you ### Update CONTEXT.md inline -When a term is resolved, update `CONTEXT.md` right there. Don't batch these up — capture them as they happen. Use the format in [CONTEXT-FORMAT.md](.agents/skills/domain-modeling/CONTEXT-FORMAT.md). +When a term is resolved, update `CONTEXT.md` right there. Don't batch these up — capture them as they happen. Use the format in `.agents/skills/domain-modeling/CONTEXT-FORMAT.md` (path from the repo root). `CONTEXT.md` should be totally devoid of implementation details. Do not treat `CONTEXT.md` as a spec, a scratch pad, or a repository for implementation decisions. It is a glossary and nothing else. diff --git a/.agents/skills/improve-codebase-architecture/SKILL.md b/.agents/skills/improve-codebase-architecture/SKILL.md index 7f5f5b585a..488c850990 100644 --- a/.agents/skills/improve-codebase-architecture/SKILL.md +++ b/.agents/skills/improve-codebase-architecture/SKILL.md @@ -53,7 +53,7 @@ End the report with a **Top recommendation** section: which candidate you'd tack **Use CONTEXT.md vocabulary for the domain, and the `/codebase-design` vocabulary for the architecture.** If `CONTEXT.md` defines "Order," talk about "the Order intake module" — not "the FooBarHandler," and not "the Order service." -See [HTML-REPORT.md](.agents/skills/improve-codebase-architecture/HTML-REPORT.md) for the full HTML scaffold, diagram patterns, and styling guidance. +See `.agents/skills/improve-codebase-architecture/HTML-REPORT.md` (path from the repo root) for the full HTML scaffold, diagram patterns, and styling guidance. Do NOT propose interfaces yet. After the file is written, ask the user: "Which of these would you like to explore?" diff --git a/.agents/skills/pr/SKILL.md b/.agents/skills/pr/SKILL.md index 802a612250..6a72949c96 100644 --- a/.agents/skills/pr/SKILL.md +++ b/.agents/skills/pr/SKILL.md @@ -62,7 +62,7 @@ If `git diff main...HEAD --name-only` matches `^frontend/`, the PR body **must** screenshots of the affected UI. Skip only when there is no visible UI effect (types, tests, build config) — and say so in the body. -1. Verify the change in the browser (frontend/CLAUDE.md → "Verifying Frontend Changes"). +1. Verify the change in the browser (frontend/AGENTS.md → "Verifying Frontend Changes"). 2. Screenshot each affected page with `mcp__playwright__browser_take_screenshot` (save to a file). 3. Host each image and get its Markdown embed by pushing to the public `windmill-labs/agent-screenshots-internal` repo. **Pipe base64 through stdin** — @@ -216,8 +216,9 @@ description saying why — `left in draft: adds a migration, wants a human look Don't flip a wide-blast-radius change just because the round came back clean, and don't ask a question nobody will read. -When the call is genuinely ambiguous, ask, then record the answer under "PR ready calibration" in -`AGENTS.local.md` so the next one is less ambiguous. +`AGENTS.local.md` (gitignored, so it may not exist) carries a "PR ready calibration" section +recording how past ambiguous calls went. Read it before deciding; when a call is still genuinely +ambiguous, ask, then append the answer there so the next one is less ambiguous. ### When rounds stop converging diff --git a/.agents/skills/svelte-frontend/SKILL.md b/.agents/skills/svelte-frontend/SKILL.md index 74f386ef37..46bf970091 100644 --- a/.agents/skills/svelte-frontend/SKILL.md +++ b/.agents/skills/svelte-frontend/SKILL.md @@ -127,4 +127,4 @@ Use the Svelte MCP tools when working on Svelte code: ## Verifying in the Browser -After changing Svelte code, use the **Playwright MCP** (`mcp__playwright__*`) to drive the running frontend and confirm the change works. See frontend/CLAUDE.md → "Verifying Frontend Changes" for the full flow. Use `playwright` (headless) on devboxes; `playwright-headed` when a display is available. +After changing Svelte code, use the **Playwright MCP** (`mcp__playwright__*`) to drive the running frontend and confirm the change works. See frontend/AGENTS.md → "Verifying Frontend Changes" for the full flow. Use `playwright` (headless) on devboxes; `playwright-headed` when a display is available. diff --git a/.agents/skills/update-sqlx/sqlx-cache.sh b/.agents/skills/update-sqlx/sqlx-cache.sh index f75b43dfde..f8d91e1da7 100755 --- a/.agents/skills/update-sqlx/sqlx-cache.sh +++ b/.agents/skills/update-sqlx/sqlx-cache.sh @@ -21,8 +21,16 @@ state="${TMPDIR:-/tmp}/wm-sqlx-cache/$(basename "$repo_root")" backup="$state/backup" added="$state/added" -# `find -printf` is GNU-only; this stays portable to a macOS checkout. -list_entries() { (cd "$1" 2>/dev/null && ls -1 ./*.json 2>/dev/null | sed 's|^\./||') | sort; } +# `find -printf` is GNU-only; a glob loop stays portable to a macOS checkout and, unlike +# `ls *.json`, does not fail the script under `set -e` when the cache is empty — which is +# exactly the state a failed `prepare` leaves behind. +list_entries() { + local f + for f in "$1"/*.json; do + [ -e "$f" ] || continue + basename "$f" + done | sort +} show_query() { if command -v jq >/dev/null 2>&1; then diff --git a/.webmux.yaml b/.webmux.yaml index a1d80576ea..813ab66937 100644 --- a/.webmux.yaml +++ b/.webmux.yaml @@ -146,12 +146,12 @@ oneshot: — note the choice in the PR description if it matters. # PR readiness - Always open the PR as a draft, then take it to ready before you stop. - Follow the `pr` skill's "Review rounds": run the review round, fix every - finding, repeat until all reviewer verdicts are a go, then post the - `✅ Review round clean @ ` marker and `gh pr ready`. Never flip to - ready without a clean round behind it, and never leave the PR sitting in - draft — an unreviewed draft is an unfinished oneshot. + Always open the PR as a draft, then drive the `pr` skill's "Review rounds" + until every reviewer verdict is a go. Never flip to ready without a clean + round behind it, and never stop at an *unreviewed* draft — that is an + unfinished oneshot. Whether a clean round then flips the PR is the skill's + "Flip, or ask first" call, not this prompt's: self-contained changes flip, + wide-blast-radius ones stay a clean draft with the reason in the PR body. # Ending your turn Never end your turn with a question, a suggestion to "take a look", or a diff --git a/AGENTS.md b/AGENTS.md index 67416006f0..10fda34c49 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,7 +15,7 @@ Open-source platform for internal tools, workflows, API integrations, background change touches, get that path actually running, and stand up whatever that takes — this is expected, not a last resort. A few examples, not a closed list: drive the UI with the Playwright MCP, run a real job of the kind you touched, restart the backend with the cargo features the - path needs (`backend/CLAUDE.md`), put a stub in front of an upstream, start MinIO for an S3 + path needs (`backend/AGENTS.md`), put a stub in front of an upstream, start MinIO for an S3 path, plant state with SQL, exercise it through the `wmill` CLI. If the path you need has no obvious way in, invent one rather than skipping it; `docs/` carries recipes for several areas. If it needs a credential or a third-party account, ask for one rather than skipping the test or @@ -54,7 +54,7 @@ Open-source platform for internal tools, workflows, API integrations, background (`$WEBMUX_WORKTREE_PATH` is set) the backend and frontend are already up in sibling tmux panes — use those, don't spawn your own. `tmux list-panes -t "$(tmux display-message -p -t "$TMUX_PANE" '#{window_id}')" -F '#{pane_index} #{pane_current_command}'` shows what is running; read its log -with `tmux capture-pane`, and see `backend/CLAUDE.md` to restart it with different cargo features. +with `tmux capture-pane`, and see `backend/AGENTS.md` to restart it with different cargo features. A second server started in your own shell fights the first one for the port. The commands below are for a plain checkout with nothing running. diff --git a/backend/AGENTS.md b/backend/AGENTS.md new file mode 100644 index 0000000000..98ad6076d5 --- /dev/null +++ b/backend/AGENTS.md @@ -0,0 +1,209 @@ +# Backend (Rust) + +- **Coding patterns**: MUST use the `rust-backend` skill when writing Rust code +- **Validation**: `docs/validation.md` — which `cargo check` flags to use +- **Enterprise**: `docs/enterprise.md` — EE file conventions and PR workflow +- **DB schema**: `backend/summarized_schema.txt` +- **API routes entry point**: `windmill-api/src/lib.rs` +- **OpenAPI spec**: `windmill-api/openapi.yaml` +- **DuckDB local jobs**: build the dynamic FFI library before running DuckDB scripts locally: + ```bash + cd backend/windmill-duckdb-ffi-internal && ./build_dev.sh + ``` + Re-run after clean builds or when `target/debug/libwindmill_duckdb_ffi_internal.*` is missing. + The bundled DuckDB compile (~2min) is cached in a per-user dir shared across + worktrees (keyed by the crate's `Cargo.lock`), so a fresh worktree reuses it and + the build is near-instant — you don't pay the full compile per worktree. Editing + the FFI crate's own source falls back to an isolated per-worktree `./target`. + The engine is a **patched fork** of duckdb-rs, not the crates.io crate — read + `docs/duckdb-isolation.md` before bumping it or touching the isolation transform. +- **Running data pipelines (DuckLake) from source**: see the section below — a plain build + advertises the `duckdb` tag but cannot execute DuckDB scripts and has no working S3 proxy. + +## Cargo features & running the dev backend + +The dev backend runs under `cargo watch` and is launched by default with **only +`--features quickjs`** (see the tmux backend pane). That baseline compiles fast but +**deliberately omits most functionality** — notably S3/object storage, the S3 proxy, all +EE code, MCP, and every non-JS language runtime. A running server never gains a feature you +didn't compile in: feature-gated routes 404 or return a `"requires "` stub. So if +you touch code behind a feature gate, or need to *exercise* such a feature at runtime, you +MUST **restart the backend with the appropriate features** for what you're working on. + +### Restarting the dev backend with the right features + +Restart in the **same pane**, so the relaunch inherits that pane's `DATABASE_URL`, `BACKEND_PORT` +and the rest of `runtime.env`. Scope every kill to this worktree — **never** +`pkill -f target/debug/windmill`, which kills every sibling worktree's backend. + +1. Find the backend pane by what it is running, not by index. The index depends on the webmux + profile: pane 1 is the backend under `full`, but the *frontend* under `frontendOnly`. + + ```bash + WIN=$(tmux display-message -p -t "$TMUX_PANE" '#{window_id}') + tmux list-panes -t "$WIN" -F '#{pane_index} #{pane_current_command} #{pane_pid}' + ``` + +2. Read the feature set it is **actually** running. `CARGO_FEATURES` in `runtime.env` is only what + the pane started with, and goes stale the first time anyone restarts by hand: + + ```bash + ps --ppid -o args= + # /home/hugo/.cargo/bin/cargo-watch watch -x run --features quickjs + ``` + +3. Stop it and relaunch with the extended set. `PORT` in the pane shell can be stale, so pass it + explicitly: + + ```bash + tmux send-keys -t "$WIN." C-c + tmux send-keys -t "$WIN." 'PORT=$BACKEND_PORT cargo watch -x "run --features quickjs,private,parquet"' Enter + ``` + + Carry over every feature the old command had unless you mean to drop one — rebuilding the list + from memory is how a backend silently loses `quickjs`. + +4. Persist the new set so a recreated pane starts with it: set `CARGO_FEATURES` in + `$(git rev-parse --git-dir)/webmux/runtime.env`. That file is read at pane startup only, so it + changes nothing about the process you just relaunched — step 3 is what takes effect now. + +5. Re-capture the pane until `health check completed` appears before hitting the API. A cold + rebuild takes ~60s, and the previous run's success line is still in the scrollback, so a + capture taken too early reads as ready when it isn't. + +cargo-watch only re-runs on a file change, so after an idle/failed run `touch README.md` (from +`backend/`, where the watch runs) is a cheap retrigger (touching a `.rs` forces a full rebuild). + +### An orphaned backend is holding the port + +If the pane's `cargo watch` looks alive but the API never answers, or the build ends in an +address-already-in-use error, a backend from an earlier run is probably still bound to the port. +It gets reparented to `systemd --user` when its shell dies, so it survives everything that looks +like a cleanup. + +Confirm all three before killing anything — a dozen sibling worktrees run their own backend, and +`pkill -f windmill` (or `-f target/debug/windmill`) kills every one of them: + +```bash +ss -ltnp | grep ":$BACKEND_PORT" # 1. which pid holds the port +readlink /proc//cwd # 2. must be THIS worktree's backend/ +ps -o ppid= -p # 3. parent is systemd/pid 1, not your pane's cargo-watch +``` + +Only when the port owner is this worktree's backend **and** it is orphaned, kill that single pid +(`kill `, then `kill -9` if it does not exit). Ask first when there is a human in the loop; +unattended, the three checks are what make it safe. Then `touch README.md` to retrigger the +watch. + +### What each feature gate does (the ones you'll actually toggle) + +`backend/Cargo.toml` `[features]` is the source of truth; this is the practical dev map. Combine +only what you need — build time scales with the set. + +| Feature | Enables | Need it for | +|---|---|---| +| `quickjs` | Embedded JS engine for inline JS eval (the default dev baseline). | Keep in every dev set. | +| `private` | Compiles the `*_ee.rs` files (symlinked from `windmill-ee-private`). Gates **all** EE code, including the real S3 helpers, the S3 proxy, and advanced S3 permission checks. | Any EE code path, S3/object storage. | +| `enterprise` | EE business logic (autoscaling, SAML hooks, advanced S3 rule **enforcement**, WAP, forks, …). Pulls in `license`. | Running EE features. Advanced S3 permission rules only take effect with this. | +| `license` | License-key/plan plumbing (`LICENSE_KEY`). Pulled in by `enterprise`. Having the feature compiled does **not** require a license *key* at runtime — CE defaults to a free plan and most EE paths still run keyless. | License-gated behavior. | +| `parquet` | S3/object-storage support: the `job_helpers/*` and `apps_u/*` S3 endpoints, parquet/CSV preview, workspace large-file storage. Without it those routes return `"requires parquet"`. | Anything touching S3/object storage or datasets. | +| `duckdb` | DuckDB script executor (also needs the FFI dylib — see above). | DuckDB scripts, DuckLake. | +| `python` `rust` `php` `java` `ruby` `csharp` `nu` `deno_core` `mysql` `mssql` `bigquery` `snowflake` `oracledb` `rlang` | Each enables that language/DB runtime for job execution. | Running jobs in that language. | +| `mcp` | MCP gateway routes (baseline `quickjs` does NOT include it → MCP routes 404). | MCP work. | +| `websocket` `http_trigger` `kafka` `nats` `mqtt_trigger` `sqs_trigger` `gcp_trigger` `azure_trigger` `postgres_trigger` `native_trigger` | Each native trigger kind; none on by default (creating one 404s without its feature). | Working on / exercising that trigger. | +| `no_auth` | Treats every request as an admin superadmin (`CLOUD_HOSTED`-guarded). | Local auth-free experiments only. | + +Convenience bundles (`ce`, `ee`, `oss`, …) exist in `[features]` but are heavy — prefer the +minimal explicit set for dev. + +**Common combinations** (run from `backend/`): + +| Goal | `--features` | +|---|---| +| Plain dev baseline (JS eval only) | `quickjs` | +| S3 / object storage / datasets (CE) | `quickjs,private,parquet` | +| S3 + EE (advanced S3 rules, on-behalf app reads, WAP, forks) | `quickjs,enterprise,private,parquet` | +| DuckLake / DuckDB (CE) | `quickjs,duckdb,parquet,private` (+ build the FFI) | +| + Python jobs | append `,python` | + +## Workspace object storage in dev — use the local filesystem + +For a dev workspace you don't need MinIO/S3: use the built-in **`FilesystemStorage`** large-file +storage (a root path on local disk). It is intentionally hidden from the settings-UI storage +dropdown (dev-only), so set it via the API. Requires the backend built with `parquet` (+ `private` +for the real S3 helpers, + `enterprise` if you want advanced permission rules enforced): + +```bash +curl -X POST "$BASE/api/w//workspaces/edit_large_file_storage_config" \ + -H "Authorization: Bearer " -H "Content-Type: application/json" \ + -d '{"large_file_storage":{"type":"FilesystemStorage","root_path":"/abs/writable/dir", + "public_resource":false,"advanced_permissions":null,"secondary_storage":{}}}' +``` + +Optional `advanced_permissions` (EE) is a list of `{"pattern":"","allow":"read[,write,delete,list]"}` +rules: admins bypass them, non-admins are confined to matching grants. Uploads/reads then flow +through the normal `job_helpers/*` (viewer-scoped) and `apps_u/*` (app-author on-behalf) S3 +endpoints. Caveat: direct DuckDB access rejects filesystem stores (`"Filesystem is not supported +in DuckDB"`) — DuckLake/datatable go through the S3 proxy instead, which works. + +## Running data pipelines (DuckLake) from source + +DuckLake pipelines need **both** the right cargo features **and** the prebuilt DuckDB FFI. A +plain `cargo run` (or `cargo run --features quickjs`) does **not** suffice, and the failure modes +are silent-ish, so agents lose time. Verify feature names against `backend/Cargo.toml` `[features]`. + +**Feature sets** (run from `backend/`): + +| Goal | Command | +|---|---| +| CE DuckLake (DuckDB scripts + S3 proxy) | `cargo run --features quickjs,duckdb,parquet,private` | +| + Python scripts | add `,python` | +| EE features (WAP, partitioning, forks, …) | add `,enterprise,license` | + +`enterprise` already pulls in `license`, but list both when you want the license-gated paths. +`quickjs` is for JS eval, not DuckLake per se — keep it if your baseline build had it. + +**Before running any DuckDB script**, build the FFI (see the bullet above): +`cd backend/windmill-duckdb-ffi-internal && ./build_dev.sh`. + +**Two gotchas that a wrong feature set produces:** + +1. **`duckdb` tag advertised, feature missing.** The `duckdb` worker tag is in the *unconditional* + default tag list (`windmill-common/src/worker.rs`, `DEFAULT_TAGS`), so a worker advertises it even + without the `duckdb` feature. Jobs then dispatch but fail at execution with + `"Duck DB requires the duckdb feature to be enabled"` (`windmill-worker/src/worker.rs`). Fix: + compile with `--features duckdb`. +2. **DuckLake writes 404 (no S3 proxy).** The workspace S3 proxy (`/w/{ws}/s3_proxy/*`) that + DuckLake uses for reads/writes only mounts the real service under + `#[cfg(all(feature = "private", feature = "parquet"))]` (`windmill-api/src/s3_proxy_oss.rs`); + otherwise it's an empty router and every proxied request 404s. Fix: compile with **both** + `private` and `parquet`. + +## Cloud vs self-hosted gating + +The `cloud` cargo feature is compiled into **all** EE builds, so `#[cfg(feature = "cloud")]` is **not** a "cloud-only" runtime gate — it only means the code is present. The real gate for behavior specific to the managed cloud (app.windmill.dev) is the runtime flag `*CLOUD_HOSTED` (`windmill_common::worker::CLOUD_HOSTED`, from the `CLOUD_HOSTED` env var; note it's loaded from `.env` via `dotenv`, so it won't show in `/proc//environ` — check the running behavior, not the exec env). + +Cloud-only logic must be behind `if *CLOUD_HOSTED { ... }`: feature-gate the helper so it compiles, then **runtime-gate the call**. `#[cfg(feature = "cloud")]` on its own is only sufficient for: +- pure helper/struct definitions (they only run when a gated caller invokes them), +- code already inside an `if *CLOUD_HOSTED { ... }` block, +- handlers that early-return on `!*CLOUD_HOSTED`, +- idempotent no-ops that are harmless off-cloud (e.g. cache invalidation). + +## Verifying Backend Changes + +`cargo check` and the unit tests do not exercise a worker code path. **If you changed how +a job runs — an executor, `handle_child`, anything spawning or reading from a +subprocess — run an actual job of that kind** and confirm it completed, then say so. +Whole classes of defect compile and unit-test clean: + +- **Stack overflow from a large buffer in an async block.** An array declared across an + `.await` is baked into the future's state; once that future is boxed a few layers deep + by the job poller, two 16 KB arrays abort the worker *process* (`thread + 'tokio-runtime-worker' has overflowed its stack`). Heap-allocate read buffers + (`vec![0u8; N]`, not `[0u8; N]`). +- Deadlocks from draining only one of a child's pipes, missed cancellation or timeout + propagation, and anything depending on the real engine's output format. + +A crash like this takes down every job on that worker, not just yours, so check the +backend log after the run rather than only the job's own status. If you cannot run one, +say which path went unexercised instead of implying it was verified. diff --git a/backend/CLAUDE.md b/backend/CLAUDE.md index 98ad6076d5..43c994c2d3 100644 --- a/backend/CLAUDE.md +++ b/backend/CLAUDE.md @@ -1,209 +1 @@ -# Backend (Rust) - -- **Coding patterns**: MUST use the `rust-backend` skill when writing Rust code -- **Validation**: `docs/validation.md` — which `cargo check` flags to use -- **Enterprise**: `docs/enterprise.md` — EE file conventions and PR workflow -- **DB schema**: `backend/summarized_schema.txt` -- **API routes entry point**: `windmill-api/src/lib.rs` -- **OpenAPI spec**: `windmill-api/openapi.yaml` -- **DuckDB local jobs**: build the dynamic FFI library before running DuckDB scripts locally: - ```bash - cd backend/windmill-duckdb-ffi-internal && ./build_dev.sh - ``` - Re-run after clean builds or when `target/debug/libwindmill_duckdb_ffi_internal.*` is missing. - The bundled DuckDB compile (~2min) is cached in a per-user dir shared across - worktrees (keyed by the crate's `Cargo.lock`), so a fresh worktree reuses it and - the build is near-instant — you don't pay the full compile per worktree. Editing - the FFI crate's own source falls back to an isolated per-worktree `./target`. - The engine is a **patched fork** of duckdb-rs, not the crates.io crate — read - `docs/duckdb-isolation.md` before bumping it or touching the isolation transform. -- **Running data pipelines (DuckLake) from source**: see the section below — a plain build - advertises the `duckdb` tag but cannot execute DuckDB scripts and has no working S3 proxy. - -## Cargo features & running the dev backend - -The dev backend runs under `cargo watch` and is launched by default with **only -`--features quickjs`** (see the tmux backend pane). That baseline compiles fast but -**deliberately omits most functionality** — notably S3/object storage, the S3 proxy, all -EE code, MCP, and every non-JS language runtime. A running server never gains a feature you -didn't compile in: feature-gated routes 404 or return a `"requires "` stub. So if -you touch code behind a feature gate, or need to *exercise* such a feature at runtime, you -MUST **restart the backend with the appropriate features** for what you're working on. - -### Restarting the dev backend with the right features - -Restart in the **same pane**, so the relaunch inherits that pane's `DATABASE_URL`, `BACKEND_PORT` -and the rest of `runtime.env`. Scope every kill to this worktree — **never** -`pkill -f target/debug/windmill`, which kills every sibling worktree's backend. - -1. Find the backend pane by what it is running, not by index. The index depends on the webmux - profile: pane 1 is the backend under `full`, but the *frontend* under `frontendOnly`. - - ```bash - WIN=$(tmux display-message -p -t "$TMUX_PANE" '#{window_id}') - tmux list-panes -t "$WIN" -F '#{pane_index} #{pane_current_command} #{pane_pid}' - ``` - -2. Read the feature set it is **actually** running. `CARGO_FEATURES` in `runtime.env` is only what - the pane started with, and goes stale the first time anyone restarts by hand: - - ```bash - ps --ppid -o args= - # /home/hugo/.cargo/bin/cargo-watch watch -x run --features quickjs - ``` - -3. Stop it and relaunch with the extended set. `PORT` in the pane shell can be stale, so pass it - explicitly: - - ```bash - tmux send-keys -t "$WIN." C-c - tmux send-keys -t "$WIN." 'PORT=$BACKEND_PORT cargo watch -x "run --features quickjs,private,parquet"' Enter - ``` - - Carry over every feature the old command had unless you mean to drop one — rebuilding the list - from memory is how a backend silently loses `quickjs`. - -4. Persist the new set so a recreated pane starts with it: set `CARGO_FEATURES` in - `$(git rev-parse --git-dir)/webmux/runtime.env`. That file is read at pane startup only, so it - changes nothing about the process you just relaunched — step 3 is what takes effect now. - -5. Re-capture the pane until `health check completed` appears before hitting the API. A cold - rebuild takes ~60s, and the previous run's success line is still in the scrollback, so a - capture taken too early reads as ready when it isn't. - -cargo-watch only re-runs on a file change, so after an idle/failed run `touch README.md` (from -`backend/`, where the watch runs) is a cheap retrigger (touching a `.rs` forces a full rebuild). - -### An orphaned backend is holding the port - -If the pane's `cargo watch` looks alive but the API never answers, or the build ends in an -address-already-in-use error, a backend from an earlier run is probably still bound to the port. -It gets reparented to `systemd --user` when its shell dies, so it survives everything that looks -like a cleanup. - -Confirm all three before killing anything — a dozen sibling worktrees run their own backend, and -`pkill -f windmill` (or `-f target/debug/windmill`) kills every one of them: - -```bash -ss -ltnp | grep ":$BACKEND_PORT" # 1. which pid holds the port -readlink /proc//cwd # 2. must be THIS worktree's backend/ -ps -o ppid= -p # 3. parent is systemd/pid 1, not your pane's cargo-watch -``` - -Only when the port owner is this worktree's backend **and** it is orphaned, kill that single pid -(`kill `, then `kill -9` if it does not exit). Ask first when there is a human in the loop; -unattended, the three checks are what make it safe. Then `touch README.md` to retrigger the -watch. - -### What each feature gate does (the ones you'll actually toggle) - -`backend/Cargo.toml` `[features]` is the source of truth; this is the practical dev map. Combine -only what you need — build time scales with the set. - -| Feature | Enables | Need it for | -|---|---|---| -| `quickjs` | Embedded JS engine for inline JS eval (the default dev baseline). | Keep in every dev set. | -| `private` | Compiles the `*_ee.rs` files (symlinked from `windmill-ee-private`). Gates **all** EE code, including the real S3 helpers, the S3 proxy, and advanced S3 permission checks. | Any EE code path, S3/object storage. | -| `enterprise` | EE business logic (autoscaling, SAML hooks, advanced S3 rule **enforcement**, WAP, forks, …). Pulls in `license`. | Running EE features. Advanced S3 permission rules only take effect with this. | -| `license` | License-key/plan plumbing (`LICENSE_KEY`). Pulled in by `enterprise`. Having the feature compiled does **not** require a license *key* at runtime — CE defaults to a free plan and most EE paths still run keyless. | License-gated behavior. | -| `parquet` | S3/object-storage support: the `job_helpers/*` and `apps_u/*` S3 endpoints, parquet/CSV preview, workspace large-file storage. Without it those routes return `"requires parquet"`. | Anything touching S3/object storage or datasets. | -| `duckdb` | DuckDB script executor (also needs the FFI dylib — see above). | DuckDB scripts, DuckLake. | -| `python` `rust` `php` `java` `ruby` `csharp` `nu` `deno_core` `mysql` `mssql` `bigquery` `snowflake` `oracledb` `rlang` | Each enables that language/DB runtime for job execution. | Running jobs in that language. | -| `mcp` | MCP gateway routes (baseline `quickjs` does NOT include it → MCP routes 404). | MCP work. | -| `websocket` `http_trigger` `kafka` `nats` `mqtt_trigger` `sqs_trigger` `gcp_trigger` `azure_trigger` `postgres_trigger` `native_trigger` | Each native trigger kind; none on by default (creating one 404s without its feature). | Working on / exercising that trigger. | -| `no_auth` | Treats every request as an admin superadmin (`CLOUD_HOSTED`-guarded). | Local auth-free experiments only. | - -Convenience bundles (`ce`, `ee`, `oss`, …) exist in `[features]` but are heavy — prefer the -minimal explicit set for dev. - -**Common combinations** (run from `backend/`): - -| Goal | `--features` | -|---|---| -| Plain dev baseline (JS eval only) | `quickjs` | -| S3 / object storage / datasets (CE) | `quickjs,private,parquet` | -| S3 + EE (advanced S3 rules, on-behalf app reads, WAP, forks) | `quickjs,enterprise,private,parquet` | -| DuckLake / DuckDB (CE) | `quickjs,duckdb,parquet,private` (+ build the FFI) | -| + Python jobs | append `,python` | - -## Workspace object storage in dev — use the local filesystem - -For a dev workspace you don't need MinIO/S3: use the built-in **`FilesystemStorage`** large-file -storage (a root path on local disk). It is intentionally hidden from the settings-UI storage -dropdown (dev-only), so set it via the API. Requires the backend built with `parquet` (+ `private` -for the real S3 helpers, + `enterprise` if you want advanced permission rules enforced): - -```bash -curl -X POST "$BASE/api/w//workspaces/edit_large_file_storage_config" \ - -H "Authorization: Bearer " -H "Content-Type: application/json" \ - -d '{"large_file_storage":{"type":"FilesystemStorage","root_path":"/abs/writable/dir", - "public_resource":false,"advanced_permissions":null,"secondary_storage":{}}}' -``` - -Optional `advanced_permissions` (EE) is a list of `{"pattern":"","allow":"read[,write,delete,list]"}` -rules: admins bypass them, non-admins are confined to matching grants. Uploads/reads then flow -through the normal `job_helpers/*` (viewer-scoped) and `apps_u/*` (app-author on-behalf) S3 -endpoints. Caveat: direct DuckDB access rejects filesystem stores (`"Filesystem is not supported -in DuckDB"`) — DuckLake/datatable go through the S3 proxy instead, which works. - -## Running data pipelines (DuckLake) from source - -DuckLake pipelines need **both** the right cargo features **and** the prebuilt DuckDB FFI. A -plain `cargo run` (or `cargo run --features quickjs`) does **not** suffice, and the failure modes -are silent-ish, so agents lose time. Verify feature names against `backend/Cargo.toml` `[features]`. - -**Feature sets** (run from `backend/`): - -| Goal | Command | -|---|---| -| CE DuckLake (DuckDB scripts + S3 proxy) | `cargo run --features quickjs,duckdb,parquet,private` | -| + Python scripts | add `,python` | -| EE features (WAP, partitioning, forks, …) | add `,enterprise,license` | - -`enterprise` already pulls in `license`, but list both when you want the license-gated paths. -`quickjs` is for JS eval, not DuckLake per se — keep it if your baseline build had it. - -**Before running any DuckDB script**, build the FFI (see the bullet above): -`cd backend/windmill-duckdb-ffi-internal && ./build_dev.sh`. - -**Two gotchas that a wrong feature set produces:** - -1. **`duckdb` tag advertised, feature missing.** The `duckdb` worker tag is in the *unconditional* - default tag list (`windmill-common/src/worker.rs`, `DEFAULT_TAGS`), so a worker advertises it even - without the `duckdb` feature. Jobs then dispatch but fail at execution with - `"Duck DB requires the duckdb feature to be enabled"` (`windmill-worker/src/worker.rs`). Fix: - compile with `--features duckdb`. -2. **DuckLake writes 404 (no S3 proxy).** The workspace S3 proxy (`/w/{ws}/s3_proxy/*`) that - DuckLake uses for reads/writes only mounts the real service under - `#[cfg(all(feature = "private", feature = "parquet"))]` (`windmill-api/src/s3_proxy_oss.rs`); - otherwise it's an empty router and every proxied request 404s. Fix: compile with **both** - `private` and `parquet`. - -## Cloud vs self-hosted gating - -The `cloud` cargo feature is compiled into **all** EE builds, so `#[cfg(feature = "cloud")]` is **not** a "cloud-only" runtime gate — it only means the code is present. The real gate for behavior specific to the managed cloud (app.windmill.dev) is the runtime flag `*CLOUD_HOSTED` (`windmill_common::worker::CLOUD_HOSTED`, from the `CLOUD_HOSTED` env var; note it's loaded from `.env` via `dotenv`, so it won't show in `/proc//environ` — check the running behavior, not the exec env). - -Cloud-only logic must be behind `if *CLOUD_HOSTED { ... }`: feature-gate the helper so it compiles, then **runtime-gate the call**. `#[cfg(feature = "cloud")]` on its own is only sufficient for: -- pure helper/struct definitions (they only run when a gated caller invokes them), -- code already inside an `if *CLOUD_HOSTED { ... }` block, -- handlers that early-return on `!*CLOUD_HOSTED`, -- idempotent no-ops that are harmless off-cloud (e.g. cache invalidation). - -## Verifying Backend Changes - -`cargo check` and the unit tests do not exercise a worker code path. **If you changed how -a job runs — an executor, `handle_child`, anything spawning or reading from a -subprocess — run an actual job of that kind** and confirm it completed, then say so. -Whole classes of defect compile and unit-test clean: - -- **Stack overflow from a large buffer in an async block.** An array declared across an - `.await` is baked into the future's state; once that future is boxed a few layers deep - by the job poller, two 16 KB arrays abort the worker *process* (`thread - 'tokio-runtime-worker' has overflowed its stack`). Heap-allocate read buffers - (`vec![0u8; N]`, not `[0u8; N]`). -- Deadlocks from draining only one of a child's pipes, missed cancellation or timeout - propagation, and anything depending on the real engine's output format. - -A crash like this takes down every job on that worker, not just yours, so check the -backend log after the run rather than only the job's own status. If you cannot run one, -say which path went unexercised instead of implying it was verified. +@AGENTS.md diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md new file mode 100644 index 0000000000..445d2106cc --- /dev/null +++ b/frontend/AGENTS.md @@ -0,0 +1,85 @@ +# Frontend (Svelte 5) + +- **Coding patterns**: MUST use the `svelte-frontend` skill when writing Svelte code +- **Validation**: `docs/validation.md` — `npm run check:fast` (2s) for iteration, `npm run check` (50s) for final PR +- **UI components**: use Windmill's design-system components — never raw HTML elements. Start from the barrel `src/lib/components/common/index.ts` and grep `src/lib/components/`; the component you need almost certainly exists +- **Brand/design**: `frontend/brand-guidelines.md` — read the relevant section before building UI, not after; the `svelte-frontend` skill maps which section covers what +- **Backend API**: routes in `../backend/windmill-api/openapi.yaml`, generated types in `src/lib/gen/` +- **Regenerate client**: `npm run generate-backend-client` after backend API changes + +## Key Frontend Patterns + +### Prefer Composable State Over Two-Way Binding + +```typescript +// Use resource() from runed for async data +import { resource } from 'runed' +let items = resource(() => args, (args) => SomeService.list(args)) +// items.loading, items.current + +// Use composables for shared reactive state +function useLoader(argsGetter: () => Args) { + let items = $state([]) + let loading = $state(false) + $effect(() => { /* react to argsGetter() */ }) + return { get loading() { return loading }, get items() { return items } } +} +``` + +Two-way binding is fine for simple form inputs. Avoid it for component-to-component state. + +## Verifying Frontend Changes + +After modifying frontend code, drive the running dev server with the **Playwright MCP** to verify the change in a real browser — don't claim a UI change works without exercising it. + +Two MCP servers are registered in `.mcp.json`: +- `playwright` — headless Chromium, default for devboxes (no display required) +- `playwright-headed` — windowed Chromium, when a display is available + +**One-time setup:** run `npx playwright install chromium` to download the browser binary (Playwright won't fetch it automatically on first use). + +Typical flow: +1. Ensure backend (`cargo run`) and frontend (`REMOTE=http://localhost:8000 npm run dev`) are running +2. `mcp__playwright__browser_navigate` to the relevant page (login at `admin@windmill.dev` / `changeme`) +3. `mcp__playwright__browser_snapshot` to inspect the accessibility tree (preferred over screenshots for reading the DOM) +4. `mcp__playwright__browser_click` / `browser_fill_form` / `browser_type` to interact +5. `mcp__playwright__browser_take_screenshot` for visual confirmation +6. `mcp__playwright__browser_console_messages` / `browser_network_requests` to surface errors + +Write screenshots to an absolute path under `/tmp` (the MCP servers already do; standalone +Playwright scripts must be told): moving a PNG out of the checkout afterwards needs a `mv` the +permission hooks always prompt on. Same reason to run `rm`/`mv`/`cp` as one plain command per Bash +call: those hooks defer on `&&`, `;`, redirects, quotes and `$VAR`. + +**Attach the screenshots to the PR.** For any change under `frontend/`, embed screenshots of the affected UI in the PR body — the `pr` skill requires this and carries the upload recipe. + +If you cannot exercise a UI change (no dev server, etc.), say so explicitly rather than claiming success. + +### Traps while driving the UI + +- `critical_alerts` 404s are expected on CE builds (EE-only endpoint) — ignore them. +- VSCode worker 404s are dev-mode artifacts — ignore them. +- `` hides its checkbox (`sr-only`). Click the `