Skip to content

chore(docker): slim default image — remove helm & Claude Code CLI with init-script guidance (WIN-2144)#10000

Open
rubenfiszel wants to merge 8 commits into
mainfrom
ruben/win-2144-remove-helm-from-default-image-with-init-script-guide
Open

chore(docker): slim default image — remove helm & Claude Code CLI with init-script guidance (WIN-2144)#10000
rubenfiszel wants to merge 8 commits into
mainfrom
ruben/win-2144-remove-helm-from-default-image-with-init-script-guide

Conversation

@rubenfiszel

@rubenfiszel rubenfiszel commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

What & why

Trims two large, optional binaries from the default Windmill runtime image, each with a clear "how to add it back" path so nothing silently breaks.

Removed Size Used by Fallback when absent
helm v3.14.3 ~48 MB user jobs calling helm /usr/bin/helm stub prints removal notice + init-script example, exits 127
Claude Code CLI ~245 MB the claude sandbox script template only the agent SDK's own preflight throws "executable not found at /usr/bin/claude…"; template header comment documents the init-script install

Neither is needed by Windmill itself — in particular, autoscaling talks to the Kubernetes API via the kube Rust crate, not kubectl/helm, and the # sandbox <image> runtime uses crane + nsjail, not the docker client.

Changes

helm (Dockerfile)

  • Flip WITH_HELM default truefalse. --build-arg WITH_HELM=true restores the real binary (that branch is unchanged).
  • When not bundled, install a helm stub at /usr/bin/helm — deliberately last in the image PATH, so any real helm an init script installs (to /usr/local/bin, ~/.local/bin, /tmp/.local/bin, …) shadows it automatically. The stub prints the removal notice, an example init script, and the docs link, then exits 127.

Claude Code CLI (Dockerfile + template)

  • Gate the install behind a new ARG WITH_CLAUDE_CODE=false. --build-arg WITH_CLAUDE_CODE=true installs it as before. When off, nothing is installed at /usr/bin/claude (no stub — see below).

  • frontend/src/lib/templates/claude_sandbox.ts.template: no runtime logic added — just a two-line header comment documenting the worker-init-script install. When the binary is absent the @anthropic-ai/claude-agent-sdk query() already preflights pathToClaudeCodeExecutable and throws:

    Claude Code executable not found at /usr/bin/claude. Please ensure Claude Code is installed via native installer or specify a valid path with options.pathToClaudeCodeExecutable.

    A stub was deliberately avoided: it would make the path exist so the SDK spawns it, and the SDK sets the child's stderr to "ignore" by default, discarding the stub's message and leaving only an opaque exited with code 127. Absence yields the clearer error, and the header comment supplies the Windmill-specific next step (the SDK message only says "native installer"):

    // Requires the Claude Code CLI at /usr/bin/claude — not in the default image. Install it via a
    // worker init script: curl -fsSL https://claude.ai/install.sh | bash && cp /root/.local/share/claude/versions/* /usr/bin/claude
    

Drive-by: stale python-fallback comments

While auditing the image I noticed the lockfile-less Python fallback comments still said "3.11", but the fallback is PyVAlias::default() which moved to 3.12 in #7405. Reworded the comments and the dedicated-worker warn log to reference "the default version" so they won't re-stale on the next bump. No behavior change.

Validation

  • Docker: docker build --check + real builds of both the helm and claude RUN blocks, each branch — confirmed the conditionals and that standalone # comment lines inside a RUN are stripped (don't swallow the command). Verified the helm stub shadowing with a PATH-resolution test.
  • SDK behavior: read the installed claude-agent-sdk source — confirmed it existsSync-checks pathToClaudeCodeExecutable and throws the clear "executable not found" error, and that it defaults child stderr to "ignore" (why a stub would be worse).
  • Frontend: loaded the Claude Sandbox template in the script editor via Playwright — renders with just the two header comment lines, no leftover logic. No console errors attributable to the change (only pre-existing ATA node:* 404s).
  • Backend: cargo watch rebuilt clean and healthy after the comment edits.
  • Measured sizes directly: helm 48.3 MB, Claude Code CLI native binary 245 MB.

Screenshots

Claude Sandbox template after simplification (two header comment lines, no runtime guard):

claude-sandbox-template-comment

Fixes WIN-2144

🤖 Generated with Claude Code

helm is no longer bundled in the default Windmill image. The default
now flips WITH_HELM to false; opt back in with --build-arg WITH_HELM=true.

When helm is not bundled, a /usr/local/bin/helm stub is installed that
explains the removal, prints an example worker init script to reinstall
it, and links the init-scripts docs before exiting 127. This gives jobs
that call `helm` an actionable message instead of "command not found".

Fixes WIN-2144

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rubenfiszel rubenfiszel marked this pull request as draft July 8, 2026 07:24
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rubenfiszel's task in 1m 14s —— View job


PR Review

Good to merge.

Reviewed the Dockerfile-only change (git show HEAD -- Dockerfile). The WITH_HELM default flips true → false, and the else branch now installs a helpful /usr/local/bin/helm stub instead of leaving helm absent. Clean, low-risk change.

What I checked

  • true branch unchanged — building with --build-arg WITH_HELM=true restores the real helm binary exactly as before. ✅
  • Stub correctnessprintf '%s\n' <lines...> > /usr/local/bin/helm writes each argument on its own line (the \n is in the format string, cycled per arg), shebang #!/bin/sh first, chmod +x applied, exit 127 preserves the prior "not found" exit code. Redirect/&& precedence is correct. ✅
  • No runtime detection broken — grepped backend/ for helm; the only references are Helm-chart config docs and a NO_PROXY comment. Nothing does which helm / probes for the binary to gate behavior, so the stub can't be mistaken for a working helm by the worker. ✅

Nits (non-blocking)

  • Dockerfile:214 — example command hardcodes linux-amd64. The image builds for both amd64 and arm64 (the real helm branch resolves arch via dpkg --print-architecture), so an arm64 user who copy-pastes the suggested curl … helm-v3.14.3-linux-amd64.tar.gz gets the wrong binary. Consider wording it as linux-<arch> or noting the arch must match, since this message is the actionable path you're steering users toward.
  • Dockerfile:214 — helm version 3.14.3 is hardcoded in the stub while the real install uses ${HELM_VERSION} (Dockerfile:140, currently also 3.14.3). They match today but will silently drift when HELM_VERSION is bumped. Minor maintainability point; a comment or interpolation would keep them in sync.

Test coverage

Config/Dockerfile-only change — no automated tests expected. Manual verification the author already did (building the stub and confirming it prints the notice to stderr and exits 127) is the right check; nothing further needed. A full image build with and without --build-arg WITH_HELM=true before release would confirm both branches end-to-end.

/usr/local/bin is first in the image PATH, so a stub there would shadow a
helm installed later via an init script into ~/.local/bin, /tmp/.local/bin,
etc. Put the stub in /usr/bin (last in PATH) instead: any real helm on an
earlier PATH entry wins automatically, and the stub only fires when nothing
else provides helm.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rubenfiszel rubenfiszel marked this pull request as ready for review July 8, 2026 07:26
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 8, 2026

Copy link
Copy Markdown

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: bf563c3
Status: ✅  Deploy successful!
Preview URL: https://dd4bdbaa.windmill.pages.dev
Branch Preview URL: https://ruben-win-2144-remove-helm-f.windmill.pages.dev

View logs

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rubenfiszel's task in 1m 22s —— View job


PR Review (updated for commit 0ba5a5e)

Mergeable, but should ideally address nits: hardcoded linux-amd64 arch and hardcoded 3.14.3 version in the stub's example command.

cc @rubenfiszel

Since my prior review a second commit (0ba5a5e) landed that moves the stub from /usr/local/bin/helm/usr/bin/helm. Re-reviewed that change.

New commit 0ba5a5e — verified correct ✅

  • PATH ordering checks out. Dockerfile:163 sets ENV PATH /usr/local/bin:/root/.local/bin:/tmp/.local/bin:$PATH, so the base image's /usr/bin sorts after every directory an init script would install into (/usr/local/bin, ~/.local/bin, /tmp/.local/bin). Placing the stub in /usr/bin means a real helm installed later on an earlier PATH entry shadows it automatically, and the stub only fires when nothing else provides helm. Sound fix — a stub in /usr/local/bin would have masked init-script installs.
  • The stub's own example installs to /usr/local/bin/helm, which is first in PATH, so a user following the message gets a real helm that wins over the stub. Consistent. ✅
  • printf/redirect/chmod/exit 127 semantics unchanged from the first commit — still correct.

Standing nits (carried over, still unaddressed)

  • Dockerfile:216 — example command hardcodes linux-amd64. The image builds for both amd64 and arm64 (the real branch resolves arch via dpkg --print-architecture), so an arm64 user who copy-pastes helm-v3.14.3-linux-amd64.tar.gz gets the wrong binary. Since this message is the actionable path you're steering users toward, wording it as linux-<arch> or noting the arch must match would avoid a footgun. Fix this →
  • Dockerfile:216 — helm version 3.14.3 is hardcoded in the stub while the real install uses ${HELM_VERSION} (Dockerfile:140). They match today but will silently drift when HELM_VERSION is bumped. Since ARGs aren't easily interpolated into a heredoc-style printf at this point, a short comment noting "keep in sync with HELM_VERSION" is the cheap fix.

Test coverage

Dockerfile-only change — no automated tests expected/possible. The author's manual verification (building the stub, confirming stderr notice + exit 127) is the right check. Before release, a full image build with and without --build-arg WITH_HELM=true, plus a quick which helm on a default image to confirm it resolves to the /usr/bin stub, would confirm both branches and the new PATH placement end-to-end.
• branch ruben/win-2144-remove-helm-from-default-image-with-init-script-guide

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Mergeable, but should ideally address nits: hardcoded helm version in stub example, hardcoded architecture in stub example.

The PR is clean and low-risk. The second commit (0ba5a5e97, "fix(docker): place helm stub in /usr/bin so real installs shadow it") correctly moved the stub to /usr/bin where it is shadowed by /usr/local/bin, /root/.local/bin, and /tmp/.local/bin. The true branch is unchanged. No backend code probes for or gates on the helm binary.

Two nits from the prior Claude review remain unaddressed:

P2 — Dockerfile:214 — hardcoded helm version in stub example

The stub's example command suggests helm-v3.14.3-linux-amd64.tar.gz, but ARG HELM_VERSION in the same file is 3.21.2. The WITH_HELM=true branch correctly interpolates ${HELM_VERSION}. A user who copy-pastes the stub's suggestion will get a significantly older helm than the bundled option, and the drift will silently widen on every version bump. Consider interpolating ${HELM_VERSION} into the printf line (it's still inside a bash RUN block and will expand at build time).

P2 — Dockerfile:214 — hardcoded architecture in stub example

The example hardcodes linux-amd64 while the WITH_HELM=true branch dynamically resolves the arch via dpkg --print-architecture. An arm64 user who copy-pastes the suggested curl … linux-amd64.tar.gz will get the wrong binary. The stub is static by nature (written to a file), so this can't be made dynamic, but the message could note that the arch must match the target platform.

Test coverage

Dockerfile-only change — no automated tests expected. The author's manual verification (building the stub via the printf sequence, confirming it prints to stderr and exits 127) is the right check. A full image build with and without --build-arg WITH_HELM=true before release would confirm both branches end-to-end. No in-app surface to exercise.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel

Mergeable, but should ideally address nits: Helm init-script example is stale and amd64-only

Found 1 issue:

  1. [P2] Dockerfile:217 - The fallback command printed by the new helm stub hardcodes helm-v3.14.3-linux-amd64, while the Dockerfile’s real Helm install uses HELM_VERSION=3.21.2 and resolves the runtime architecture with dpkg --print-architecture. On arm64 images, users who follow the stub’s recommended init script install the wrong binary; on all images, they install a different Helm version than the image would bundle with WITH_HELM=true. Since this stub is the actionable migration path, the example should use the current Helm version and make the architecture match the worker image.
    Dockerfile:217

Test coverage

Dockerfile-only change, so no automated test coverage is expected.

Manual verification should include building/running the default image path and invoking helm to confirm the stub exits 127 and prints a usable init-script command. Before merge, also verify the recommended command is correct for both amd64 and arm64 images, plus a build with --build-arg WITH_HELM=true to confirm the real Helm branch still installs the intended version.

…fault

The lockfile-less python fallback returns PyVAlias::default(), which moved
from 3.11 to 3.12 in #7405. Update the comments and the dedicated-worker
warn log to reference "the default version" instead of a hardcoded 3.11 so
they stay correct across future latest-stable bumps. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Mergeable, but should ideally address nits: hardcoded helm version and architecture in stub example command.

Reviewed the full diff (commits bf9c1f3d9, 0ba5a5e97, b4e2dd4aa) against AGENTS.md and prior reviews.

Standing nits (still unaddressed from prior reviews)

P2 — Dockerfile:216 — hardcoded helm version in stub example

The stub's example command hardcodes helm-v3.14.3, while the real install branch uses ${HELM_VERSION} (line 140, also 3.14.3 today). They match now but will silently drift on the next HELM_VERSION bump. Since printf runs inside a RUN block, ${HELM_VERSION} would expand. Consider interpolating it so the stub always recommends the same version the WITH_HELM=true build would bundle.

P2 — Dockerfile:216 — hardcoded architecture in stub example

The example command hardcodes linux-amd64, but the image builds for both amd64 and arm64 (the real install branch resolves arch via dpkg --print-architecture). An arm64 user who copy-pastes the suggested command gets the wrong binary. Since the stub is a static file, the arch can't be dynamic, but the message could note the arch must match the target platform.

New commit b4e2dd4aa — verified clean ✅

The Python fallback comment updates in python_executor.rs:3448 and python_versions.rs:375,378 correctly replace stale "3.11" references with "default python version" / PyVAlias::default(), matching the actual code (PyV::default()PyVAlias::default().into() on line 71). Pure doc fix, no behavior change.

Test coverage

Backend (Python comment-only changes): no new logic, no tests expected.

Dockerfile: Dockerfile-only change — no automated tests expected. Manual verification from the author (stub built via printf, confirmed stderr output + exit 127, PATH-shadowing test) is the right check. Before release, a full image build with and without --build-arg WITH_HELM=true would confirm both branches end-to-end. No in-app surface to exercise.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel

Mergeable, but should ideally address nits: stale/amd64-only Helm init-script example, history-style comments in Dockerfile

Found 2 issues:

  1. [P2] Dockerfile:216 - The stub’s actionable install command hardcodes helm-v3.14.3-linux-amd64. That is amd64-only, and in the checked-out merge result against current base the real Helm branch uses HELM_VERSION=3.21.2, so users who follow the stub install a different version than WITH_HELM=true would bundle. Make the example track HELM_VERSION and avoid prescribing the wrong architecture for arm64 workers.
    Dockerfile:216

  2. [P2] Dockerfile:146 - The new Dockerfile/script comments describe history rather than the current constraint: “helm is no longer bundled” and “helm has been removed”. AGENTS.md says: Describe the code as it is, never its drafting history: "we no longer do X", "unchanged behavior", "instead of the previous approach" are meaningless to a reader who never saw the earlier iteration. Rewording to current-state phrasing like “Default images omit helm...” would satisfy the rule.
    Dockerfile:146

Test coverage

Dockerfile/config plus Rust comment/log-message changes, so no automated tests are expected.

Manual verification should build the default image and run helm, confirming the stub exits 127 and prints a usable init-script command for the target architecture/current Helm version. Also build with --build-arg WITH_HELM=true to confirm the real Helm branch still installs the intended version.

…ls fast

The Claude Code CLI (~245MB, the single largest optional component) was
bundled unconditionally and is used only by the claude sandbox template.
Gate it behind WITH_CLAUDE_CODE (default false); opt back in with
--build-arg WITH_CLAUDE_CODE=true or install it at worker startup via an
init script.

To keep the failure actionable, the claude sandbox template now checks for
the CLI at /usr/bin/claude before invoking the agent SDK and, when absent,
throws a clear error naming the next step: the exact init-script install
command plus a docs link. The hardcoded executable path is lifted into a
CLAUDE_CODE_EXECUTABLE constant referenced by both the check and the SDK
call so they can't drift.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rubenfiszel rubenfiszel changed the title chore(docker): remove helm from default image with init-script stub (WIN-2144) chore(docker): slim default image — remove helm & Claude Code CLI with init-script guidance (WIN-2144) Jul 8, 2026
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Mergeable, but should ideally address nits: hardcoded helm version and architecture in stub example command.

cc @rubenfiszel

Reviewed the full diff (commits through 28f382b) against AGENTS.md and prior review history. The core change — removing helm from the default Docker image and replacing it with a discoverable stub — is sound. The PATH placement (/usr/bin, last in PATH) correctly allows init-script installs to shadow the stub, and the WITH_HELM=true branch is unchanged.

Two standing nits from prior reviews remain unaddressed:

P2 — Dockerfile:216 — hardcoded helm version in stub example

The stub's example command hardcodes helm-v3.14.3, while ARG HELM_VERSION=3.21.2 on line 139. These already differ — a user who follows the stub's suggestion installs helm 3.14.3 (September 2024), while --build-arg WITH_HELM=true would bundle 3.21.2. The drift will silently widen on every version bump. The printf runs inside a RUN block, so ${HELM_VERSION} would expand at build time — but the example is inside single quotes, preventing interpolation.

P2 — Dockerfile:216 — hardcoded linux-amd64 in stub example

The example command hardcodes the amd64 tarball. The WITH_HELM=true branch dynamically resolves arch via dpkg --print-architecture. On arm64 workers, a user who copy-pastes the stub's suggested curl … linux-amd64.tar.gz gets the wrong binary. Since the stub is written as a static file, the arch can't be dynamic, but the message could note that the arch must match the target platform.

Other changes reviewed — clean

  • Dockerfile — Claude Code CLI removal: The WITH_CLAUDE_CODE default flip and conditional install mirror the helm pattern. Correct.
  • claude_sandbox.ts.template: The new fs.existsSync guard and informative error message with an init-script recipe is clean. The CLAUDE_CODE_EXECUTABLE constant eliminates the magic string. No issues.
  • python_executor.rs / python_versions.rs: Comment-only fixes replacing stale "3.11" references with "default python version" / PyVAlias::default(). No behavior change. Correct.

Test coverage

Dockerfile + template + doc comment changes only — no automated tests expected. The author's manual verification (building the stub via printf, confirming stderr output + exit 127, PATH-shadowing test) is the right check. Before release, a full image build with and without --build-arg WITH_HELM=true would confirm both branches end-to-end. No in-app surface to exercise.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel

Mergeable, but should ideally address nits: stale/amd64-only Helm init-script example, history-style comments

Found 2 issues:

  1. [P2] Dockerfile:220 - The Helm stub’s actionable install command hardcodes helm-v3.14.3-linux-amd64, while the real bundled branch uses HELM_VERSION=3.21.2 and resolves architecture dynamically. Users following the stub on arm64 get the wrong binary, and all users get a different Helm version than WITH_HELM=true would install.
    Dockerfile:220

  2. [P2] Dockerfile:145 - The new Dockerfile comments/script text use history-style wording like “no longer bundled” / “has been removed”. AGENTS.md says: Describe the code as it is, never its drafting history: "we no longer do X", "unchanged behavior", "instead of the previous approach" are meaningless to a reader who never saw the earlier iteration. Reword these to current-state phrasing, e.g. “Default images omit helm...” and “Default images omit the Claude Code CLI...”.
    Dockerfile:145

Test coverage

Dockerfile/config plus Rust log/comment changes and a frontend template change. No automated tests are expected for these layers.

Manual verification should build the default image and run helm, confirming the stub exits 127 and prints a usable init-script command for the current Helm version and target architecture. Also build with --build-arg WITH_HELM=true and --build-arg WITH_CLAUDE_CODE=true to confirm both opt-in install paths still produce working binaries.

…inary error

The runtime existsSync guard + multi-line throw was bloat in a user-facing
template. The claude-agent-sdk already preflights pathToClaudeCodeExecutable
and throws a clear "Claude Code executable not found at /usr/bin/claude.
Please ensure Claude Code is installed..." when it's absent, so the guard was
redundant. A stub binary would be worse: it makes the path exist, so the SDK
spawns it, and since the SDK sets child stderr to "ignore" by default the
stub's message is discarded, leaving only an opaque "exited with code 127".

Keep only a two-line header comment documenting the worker-init-script install
step (the SDK error says "native installer", not Windmill's init scripts).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

cc @rubenfiszel

Mergeable, but should ideally address nits: history-style comments, hardcoded helm version/arch in stub example, stale Dockerfile comment about Claude template preflight.

The core changes — removing helm and Claude Code CLI from the default image with clear opt-back-in paths — are sound. The helm stub's /usr/bin PATH placement correctly allows init-script installs to shadow it, and the WITH_HELM=true / WITH_CLAUDE_CODE=true branches are unchanged. The latest commit (b402b1543) drops the Claude template preflight in favor of relying on the SDK's own missing-binary error. The Python comment fixes are correct (PyVAlias::default()Py312).

Three P2 nits follow. Two were flagged in prior reviews and remain unaddressed; one is new from the latest commit.

P2 — Dockerfile:145-148 — history-style comments

The new comments use drafting-history wording:

  • Line 145: # helm is no longer bundled in the default image; opt back in with WITH_HELM=true
  • Lines 147-148: # the Claude Code CLI … is no longer bundled in the default image; opt back in with WITH_CLAUDE_CODE=true
  • Line 215 (stub): # helm has been removed from the default Windmill image to keep it lean.
  • Line 217 (stub echo): echo "helm is no longer bundled in the default Windmill image."

AGENTS.md says: "Describe the code as it is, never its drafting history: 'we no longer do X', 'unchanged behavior', 'instead of the previous approach' are meaningless to a reader who never saw the earlier iteration."

Reword to current-state phrasing, e.g. # Default images omit helm; enable with WITH_HELM=true.

P2 — Dockerfile:217 — hardcoded helm version and architecture in stub example

The stub's example install command hardcodes helm-v3.14.3-linux-amd64, while ARG HELM_VERSION=3.21.2 (line 139) — they already differ by 7 minor versions. The WITH_HELM=true branch resolves the arch dynamically via dpkg --print-architecture. An arm64 user following the stub's suggested command gets the wrong binary. Since the printf runs inside a RUN block, ${HELM_VERSION} would expand at build time, but the example is single-quoted, preventing interpolation. Either switch to double quotes to interpolate, or match the real branch's dynamic arch resolution with a note.

P2 — Dockerfile:309 — stale comment about template preflight

The comment says: "When absent, the claude sandbox template fails fast with instructions (frontend/src/lib/templates/claude_sandbox.ts.template)." Commit b402b1543 dropped the fs.existsSync preflight and the CLAUDE_CODE_EXECUTABLE constant from the template. The template no longer fails fast itself; it now relies on the SDK throwing when the binary is missing. The comment is out of date.

Test coverage

Dockerfile, template, and doc-comment changes only — no automated tests expected. Manual verification from the author (stub behavior + SDK error path) is appropriate. No in-app surface to exercise beyond what the author already tested via Playwright.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

cc @rubenfiszel

Mergeable, but should ideally address nits: history-style comments (violates AGENTS.md), hardcoded helm version/arch in stub example, stale Dockerfile comment about Claude template preflight.

The core changes — removing helm and the Claude Code CLI from the default image with clear opt-back-in paths — are sound. The helm stub's /usr/bin PATH placement correctly allows init-script installs to shadow it, the WITH_HELM=true / WITH_CLAUDE_CODE=true branches are unchanged, and the Python comment fixes are correct (PyVAlias::default() now reflects 3.12, not stale "3.11"). The template change (single header comment, no runtime guard) correctly relies on the SDK's own missing-binary error.

Three P2 nits from the prior review remain unaddressed:

P2 — Dockerfile:145-148,215,217 — history-style comments

AGENTS.md says: "Describe the code as it is, never its drafting history: 'we no longer do X', 'unchanged behavior', 'instead of the previous approach' are meaningless to a reader who never saw the earlier iteration."

Four locations violate this:

  • Line 145: # helm is no longer bundled in the default image; opt back in with WITH_HELM=true
  • Lines 147-148: # the Claude Code CLI … is no longer bundled in the default image; opt back in with WITH_CLAUDE_CODE=true
  • Line 215 (stub shebang comment): # helm has been removed from the default Windmill image to keep it lean.
  • Line 217 (stub echo): echo "helm is no longer bundled in the default Windmill image."

Reword to current-state phrasing, e.g. # Default images omit helm; enable with WITH_HELM=true and echo "helm is not bundled in the default Windmill image.".

P2 — Dockerfile:217 — hardcoded helm version and architecture in stub example

The stub's example install command hardcodes helm-v3.14.3-linux-amd64, while ARG HELM_VERSION=3.21.2 (line 139) — they already differ by 7 minor versions. The WITH_HELM=true branch resolves the arch dynamically via dpkg --print-architecture. An arm64 user following the stub's suggested command gets the wrong binary. The printf runs inside a RUN block, so ${HELM_VERSION} would expand at build time, but the example is single-quoted, preventing interpolation. Either switch to double quotes or add a note about matching arch/version.

P2 — Dockerfile:308 — stale comment about template preflight

The comment says: "When absent, the claude sandbox template fails fast with instructions (frontend/src/lib/templates/claude_sandbox.ts.template)." The template no longer contains any fs.existsSync preflight or CLAUDE_CODE_EXECUTABLE constant — it only has a single header comment line (per the PR body: "no runtime logic added"). The error now comes from the SDK's own query() preflight throwing "Claude Code executable not found at /usr/bin/claude". The comment is out of date; remove or reword to describe the current behavior.

Test coverage

Dockerfile, template, and doc-comment changes only — no automated tests expected. Manual verification from the author (stub behavior, SDK error path, Playwright template rendering, cargo watch health) is appropriate. No in-app surface to exercise beyond what the author already tested.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel

Mergeable, but should ideally address nits: history-style Dockerfile comments, stale/amd64-only Helm init-script example, stale Claude template preflight comment

Found 3 issues:

  1. [P2] Dockerfile:145 - The new comments use drafting-history wording like “no longer bundled” / “has been removed”. AGENTS.md says: Describe the code as it is, never its drafting history: "we no longer do X", "unchanged behavior", "instead of the previous approach" are meaningless to a reader who never saw the earlier iteration. Reword these as current-state constraints, e.g. “Default images omit helm...”.
    Dockerfile:145

  2. [P2] Dockerfile:220 - The Helm stub’s actionable install command hardcodes helm-v3.14.3-linux-amd64, while the merge result’s real opt-in branch uses HELM_VERSION=3.21.2 and derives architecture via dpkg --print-architecture. Users following the stub on arm64 get the wrong binary, and users on current base get a different Helm version than WITH_HELM=true would install.
    Dockerfile:220

  3. [P2] Dockerfile:309 - This comment still says the Claude sandbox template “fails fast with instructions”, but the latest template only contains a header comment and relies on the SDK’s missing-executable preflight. That makes the Dockerfile comment stale about where the failure/instructions live.
    Dockerfile:309

Test coverage

Dockerfile/config plus Rust log/comment wording and a frontend template comment change, so no automated tests are expected.

Manual verification should build the default image and run helm, confirming the stub exits 127 and prints an install command valid for the current Helm version and target architecture. Also build with --build-arg WITH_HELM=true and --build-arg WITH_CLAUDE_CODE=true to confirm both opt-in paths still install working binaries.

… stub

Address review feedback:
- Reword helm/claude ARG and stub comments as current-state constraints
  ("Default images omit …") instead of drafting-history phrasing.
- The helm stub's suggested install command now mirrors the WITH_HELM=true
  branch: ${HELM_VERSION} is baked in at build time and $arch is resolved at
  run time via dpkg, so the hint is correct on every arch and can't drift from
  the real version.
- Fix the stale Claude comment that claimed the template "fails fast with
  instructions"; the runtime error comes from the agent SDK's missing-binary
  preflight, with the template header comment naming the init-script install.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Pi Review

Good to merge.

The latest commit (2e8f89fa1) addressed every P2 nit from prior reviews:

  • History-style comments (AGENTS.md violation): all four locations now use current-state phrasing — "Default images omit helm…", "helm is not included in the default Windmill image.", "Omitted from the default image to stay lean…". ✅
  • Hardcoded helm version: the stub's example command now interpolates ${HELM_VERSION} at build time (double-quoted printf argument, currently 3.21.2), so it stays in sync with the WITH_HELM=true branch. ✅
  • Hardcoded architecture: the stub resolves the arch at runtime via arch="$(dpkg --print-architecture)"; arch="${arch##*-}", correctly producing amd64 or arm64. ✅
  • Stale Claude preflight comment: the Dockerfile comment now accurately says "the agent SDK reports a missing /usr/bin/claude at run time", matching the simplified template (single header comment, no runtime guard). ✅

All other aspects — the helm stub's /usr/bin PATH placement, the WITH_HELM=true/WITH_CLAUDE_CODE=true opt-in branches, the Python comment fixes, and the Claude template simplification — are correct and clean.

Test coverage

Dockerfile, template comment, and Rust doc-comment changes only — no automated tests expected. The PR body documents thorough manual verification (both branches of each ARG built, Playwright template rendering, stub PATH-resolution test, SDK error-path read-through, cargo watch healthy). No in-app surface to exercise.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel

Mergeable, but should ideally address nits: non-ASCII template comment

Found 1 issue:

  1. [P2] frontend/src/lib/templates/claude_sandbox.ts.template:3 - The new header comment introduces a Unicode dash into a file that was ASCII-only before. AGENTS.md says: You default to ASCII when editing or creating files. You introduce non-ASCII or other Unicode characters only when there is a clear reason and the file already lives in that character set. There is no functional need for the Unicode character here; use ASCII punctuation instead.
    frontend/src/lib/templates/claude_sandbox.ts.template:3

Test coverage

Dockerfile/config, Rust comment/log wording, and a frontend template comment only, so no automated tests are expected.

Manual verification should build the default image and run helm to confirm the stub exits 127 and prints a usable init-script command. Also build with --build-arg WITH_HELM=true and --build-arg WITH_CLAUDE_CODE=true to confirm both opt-in install paths still produce working binaries.

Replace Unicode em-dashes with ASCII in files that were ASCII-only, per
AGENTS.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

All findings from prior reviews have been addressed in the most recent commits. The Codex P2 about a "Unicode dash" in claude_sandbox.ts.template:3 is a false alarm — cat -A confirms it's a standard ASCII hyphen (-). There are no remaining issues.

Pi Review

Good to merge.

All four P2 nits from prior reviews are resolved in the latest commits:

  • History-style comments (Dockerfile:145-148,215,217): reworded to current-state phrasing ("Default images omit helm…", "Omitted from the default image to stay lean…"). ✅
  • Hardcoded helm version (Dockerfile:220): stub example now interpolates ${HELM_VERSION} at build time via a double-quoted printf argument, staying in sync with the WITH_HELM=true branch. ✅
  • Hardcoded architecture (Dockerfile:220): stub resolves arch at runtime via $(dpkg --print-architecture) with a ##*- suffix strip, correctly producing amd64 or arm64. ✅
  • Stale Claude preflight comment (Dockerfile:308): now accurately says "the agent SDK reports a missing /usr/bin/claude at run time", matching the simplified template. ✅

The Codex P2 (Unicode dash in claude_sandbox.ts.template:3) is a false positive — the dash in "- install via a..." is a standard ASCII hyphen (0x2D), confirmed with cat -A. No Unicode characters exist in the file.

No new issues found. The helm stub's /usr/bin PATH placement correctly allows init-script installs to shadow it, the WITH_HELM=true / WITH_CLAUDE_CODE=true branches are unchanged, and the Python doc-comment fixes are correct.

Test coverage

Dockerfile, template-comment, and Rust doc-comment changes only — no automated tests expected. The PR body documents thorough manual verification (both ARG branches built, Playwright template rendering, stub PATH-resolution test, SDK error-path read-through, cargo watch healthy). No in-app surface to exercise beyond what the author already tested.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

Good to merge.

No issues found. The prior Helm version/arch, history-style wording, stale Claude preflight comment, and non-ASCII template punctuation findings are addressed at this head.

Test coverage

Dockerfile/config plus Rust comment/log wording and a frontend template header comment; no automated tests are expected for these changes.

Manual verification should cover building the default image and invoking helm to confirm the stub exits 127 with the init-script hint, plus builds with --build-arg WITH_HELM=true and --build-arg WITH_CLAUDE_CODE=true to confirm both opt-in install paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant