Repository navigation
feat: add alpine based images support - #237
Conversation
Build and publish musl-linked LSP binaries so the extension runs inside Alpine-based containers, where the glibc binary fails to spawn against musl's loader (os error 2). - release.yml: add x86_64/aarch64-unknown-linux-musl matrix entries and their cross toolchains (musl-tools, aarch64-linux-musl-cross), wiring CC so the cc-crate tree-sitter grammar build cross-compiles to musl - lib.rs: detect musl at runtime (musl loaders / alpine-release marker) and select the -musl asset; non-musl gnu paths unchanged - unit tests for the platform->binary mapping and musl detection Fixes #236
There was a problem hiding this comment.
🔄 Changes Requested
Solid, well-structured PR — the pure-helper split (platform_binary_name / detect_musl with an injected existence check) is genuinely clean and the unit tests are meaningful. Two blockers stand between this and approval, both cheap to fix.
Issues Found
🔴 1. The new tests never run in CI (.github/workflows/ci.yml:70-92)
The five unit tests you added live in the root zed-laravel crate (src/lib.rs), but no CI job runs cargo test against that crate. The extension job runs only cargo fmt, cargo clippy --target wasm32-wasip2, and cargo check --target wasm32-wasip2 — none of which execute #[cfg(test)] blocks. The cargo test --all-features step you're relying on (the PR's Test Plan checks it off) lives in the separate laravel-lsp job with working-directory: laravel-lsp — a different crate, no workspace relationship. Net effect: the regression protection the PR claims doesn't exist; a future break to platform_binary_name/detect_musl lands green.
Fix: add a cargo test step to the extension job. I verified cargo test --lib builds and runs all 5 on the host target (0.21s, all pass) — no wasm test runner needed, despite the crate being cdylib. So this is one extra step:
- name: cargo test
run: cargo test🔴 2. Unverified third-party toolchain compiles a published release binary (.github/workflows/release.yml:226-229)
The arm64-musl step downloads aarch64-linux-musl-cross.tgz from musl.cc over HTTPS with no checksum or signature check, installs it to /opt, and wires it in as the linker + CC_aarch64_unknown_linux_musl. That toolchain then compiles and links the laravel-lsp-linux-arm64-musl asset end users download and execute. A compromise or MITM of musl.cc at build time silently backdoors the published binary — and musl.cc is also a flaky single-maintainer mirror, so this doubles as a build-reliability risk. The x64 path correctly uses distro-signed apt-get install musl-tools; only arm64 takes the unverified route.
I'll note the repo already has prior art for unverified downloads (laravel-lsp/build.rs pulls the tree-sitter-blade grammar without a checksum), so this isn't a brand-new risk class — but it's a new download added here that feeds the shipped binary, and the fix is trivial:
Fix (minimum): pin and verify a sha256 of the tarball before extracting, e.g.
curl -fL --retry 3 --retry-delay 5 https://musl.cc/aarch64-linux-musl-cross.tgz -o /tmp/aarch64-musl-cross.tgz
echo "<known-sha256> /tmp/aarch64-musl-cross.tgz" | sha256sum -c -(Pinning to a versioned/immutable URL or a mirror you control is even better, but a checksum is the bar.)
What's Good
- The
platform_binary_name(os, arch, is_musl)/detect_musl(path_exists)split with dependency injection is the right call — pure, testable, no fs in the unit tests. 👌 - AC #3/#4 wiring is correct: musl guard-arms are ordered before the unguarded gnu arms, so glibc hosts fall through unchanged (and
linux_gnu_names_unchangedpins that). - AC #5 archive-name chain verified end-to-end:
download_binarybuilds{binary_name}.tar.gz→ exactly what release.yml'sCreate archivestep uploads. No special-casing needed — nicely transitive. - Tests are honest: each would fail on a real mutation (suffix typo, dropped marker path, is_musl leaking into a non-Linux arm). Confirmed they pass on host.
- Transparent PR write-up — calling out the manual ACs and the WASI absolute-path risk up front is exactly right.
📋 Non-blocking follow-ups
- The
(zed::Os::Linux, _)wildcard arm (src/lib.rs:189) returns the non-musl genericlaravel-lspname even whenis_musl == true— harmless today (the SDKArchitectureenum only has X8664/Aarch64), but if a third Linux arch ever appears it'd silently serve the glibc binary on musl. Optional one-liner while you're in the file.
(Watson: you're already in the code fixing the blockers above — fold this in too if it's quick. No separate issue.)
Notes (not blocking)
- I checked the sqlx → OpenSSL/musl-link concern a lens raised: it's a false positive.
sqlxisdefault-features = falsewith no TLS feature, andtiberiususesrustls— nothing pullsopenssl/native-tls, so no musl link wall there. - ACs #6 and #7 remain a manual gate (extension starts in an Alpine devcontainer; go-to-def works end-to-end). Neither CI nor review can close these — they need a real release + Alpine container, so they're @mikebronner's to verify when this PR returns clean. The WASI absolute-path probe is exactly what they'll exercise.
Please address the two blockers and re-request review.
The unit tests in src/lib.rs live in the root zed-laravel crate, which the extension CI job only fmt/clippy/checks — the laravel-lsp job's cargo test is a separate crate. Add a host-target cargo test step so the platform-binary-name and musl-detection tests actually gate the build.
musl.cc is an unsigned single-maintainer mirror, and its toolchain compiles the published laravel-lsp-linux-arm64-musl binary end users run. Pin and verify a known-good sha256 before extracting so a MITM'd, compromised, or silently-rebuilt tarball fails the build loudly instead of backdooring the shipped binary.
The fallback (Os::Linux, _) arm returned the glibc-generic laravel-lsp name even when musl was detected. Harmless today (the SDK Architecture enum only has X8664/Aarch64), but if a third Linux arch ever appears it would silently serve a glibc binary to a musl host. Guard the fallback on is_musl.
|
Both blockers fixed and the non-blocking follow-up implemented in this PR — CI is green. 🔴 Blocker 1 — tests never run in CI: added a 🔴 Blocker 2 — unverified musl.cc toolchain: pinned the 📋 Follow-up — Three atomic commits ( |
There was a problem hiding this comment.
✅ Approved
Round 2 — both round-1 blockers are verifiably resolved and the diff is clean. Fanned out four lenses + adversarial verification; no hard defect survived, and nothing actionable remains in the PR's own code.
Review Summary
- Blocker 1 (tests never ran in CI) — fixed.
.github/workflows/ci.ymlnow has acargo teststep in theextensionjob (ci.yml:96-99). The 5 root-crate unit tests actually gate the build now; CI confirms the job ran green (47s). Verified thecdylibcrate compiles + tests on the host target —wit-bindgenstubs the WIT imports under#[cfg(not(target_arch = "wasm32"))], and the tests only touch the pure helpers, so no wasm runner is needed. - Blocker 2 (unverified musl.cc toolchain) — fixed.
release.yml:234-236now pins theaarch64-linux-musl-cross.tgztarball withsha256sum -c -beforetar -xzf— correct verify-then-extract ordering. A MITM'd/rebuilt tarball fails the build loudly instead of backdooring the shipped arm64-musl binary. - Round-1 follow-up implemented: the defensive
(zed::Os::Linux, _) if is_muslarm is in place (src/lib.rs:189-191).
Acceptance criteria
- AC1–AC5: met. Release matrix adds both musl targets with the exact asset names (
release.yml:163-168); x64 uses signedmusl-tools, arm64 uses the now-pinned toolchain;Create archiveemits{binary_name}.tar.gz(release.yml:266). Runtime detection probes all three markers (src/lib.rs:208-214), musl guard-arms correctly precede the unguarded gnu arms so glibc hosts are unchanged (linux_gnu_names_unchangedpins this), anddownload_binarybuilds{binary_name}.tar.gztransitively — names agree end-to-end. - AC6 & AC7: manual gate — @mikebronner's to verify on merge. These require a live Alpine devcontainer (extension starts clean; go-to-def works end-to-end). Neither CI nor static review can exercise a running LSP inside Alpine — the source-side enablement (detection + musl download + musl release builds) is correct and in place, but the live confirmation is yours.
Tests verified
Five unit tests, all honest (each fails on a real mutation: dropped -musl suffix, swapped arm, is_musl leaking into a non-Linux arm, a removed marker path). The wasm-untestable paths (download_binary archive composition; the future-arch defensive arm — no third Architecture variant is constructable) are reasoned-through, not faked.
📋 Non-blocking follow-ups
- GitHub Actions expression-injection hardening (
release.yml:36,:38,:121) — event data (github.event.inputs.tag,github.event.release.tag_name) is interpolated directly intorun:shell blocks. Should route throughenv:intermediate vars (the patterndependabot-auto-merge.ymlalready uses). General scope, pre-existing — none of these lines are touched by this PR, and exploitation requires a privileged actor (write access to trigger), so it's defense-in-depth, not a ship blocker. Filing one umbrella issue for the class.
Ready for @mikebronner to merge — and to close out AC6/AC7 against a dunglas/frankenphp:php8.5-alpine container.
Summary
Implements #236 — adds Alpine/musl support so the Laravel LSP runs inside musl-based containers (e.g.
dunglas/frankenphp:php8.5-alpinedevcontainers), where the glibc binary fails withNo such file or directory (os error 2).Changes
release.yml— add build-matrix entries forx86_64-unknown-linux-muslandaarch64-unknown-linux-musl, producinglaravel-lsp-linux-x64-musl/laravel-lsp-linux-arm64-musl. Each musl target installs its cross toolchain (musl-toolsfor x64;aarch64-linux-musl-crossfrom musl.cc for arm64) and exportsCC_<target>+ a cargolinkerso thecc-crate tree-sitter grammar build cross-compiles to musl too. The existing archive/upload steps then publish the*.tar.gzassets alongside the gnu ones.src/lib.rs—get_platform_binary_name()now probes for musl at runtime (/lib/ld-musl-x86_64.so.1,/lib/ld-musl-aarch64.so.1,/etc/alpine-release) and returns the-muslasset name on musl Linux. Non-musl Linux is unchanged. The binary-name mapping and musl probe are split into pure helpers (platform_binary_name,detect_musl) for unit testing.download_binary()needs no change: it derives the archive name and download URL from the binary name, so the-muslsuffix flows through to extraction +make_file_executableautomatically.Acceptance Criteria
laravel-lsp-linux-x64-musl,laravel-lsp-linux-arm64-musl) via the*-unknown-linux-musltargetsrelease.ymladds both musl matrix entries, installs the musl cross toolchain, and uploads the*.tar.gzarchives alongside the gnu assetsget_platform_binary_name()detects musl Linux at runtime and returns the-muslnamedownload_binary()constructs the correct URL/archive name for musl variants and extracts + makes them executable (transitive via binary name)os error 2in the LSP logTest Plan
cargo test— 5 new unit tests for the platform→binary mapping (gnu unchanged, musl suffixed, non-Linux ignores the flag) and musl detectioncargo fmt --check,cargo clippy --target wasm32-wasip2 -- -D warnings,cargo check --target wasm32-wasip2all clean-muslassets build + upload; then load the extension inside an Alpine devcontainer and confirm the LSP starts and go-to-definition worksNotes for review
release.ymlbuild is exercised only on a real release, not by PR CI — the workflow changes can't be validated by this PR's checks.std::fsreads of absolute host paths from the WASM extension. The existing code only reads relative paths under the extension work dir; if Zed's WASI sandbox doesn't preopen/, the absolute-path probe may need an alternate vector. This is the detection approach the issue's AC prescribes, and is covered by the two manual AC items above.Fixes #236