Repository navigation
ci(musl): build the Alpine image from a pinned Dockerfile Dependabot can move - #1107
Conversation
🦋 Changeset detectedLatest commit: ee87669 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 28 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (13)
Comment |
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🔴 fix before merging
Make three changes before merging. The failure notice for the weekly check and the other test gaps can wait for a follow-up.
- Add
.github/docker/musl-buildto the sparse checkout inauth-preflight.ymlandffi-preflight.yml. With this PR, every preflight run fails at the musl smoke test. - Pin the compiler packages that
build-baseinstalls. - Restore the check for a
node:<n>-alpinetag with no digest.
The rest of the PR is correct. The base image digest has one copy, and Dependabot reads it. Both build workflows build from the Dockerfile. The tests fail if someone adds a second copy of the digest or an inline apk add.
I reproduced the sparse checkout with git sparse-checkout set --cone scripts. I ran apk info and apk add --simulate in the pinned image. With them, I checked the build-base dependencies and the proposed pins.
I did not comment on the trade-off in "Review notes". The trade-off is between exact apk pins and a release that fails when Alpine removes a version. The PR description already asks for that decision.
Other findings not posted as comments
- Optional:
scripts/__tests__/musl-build-image.test.mjs:63reads theFROMline with/^FROM\s+/and.trim(). The preflights read it withsed -n 's/^FROM //p'. That command removes exactly one space and does not trim the line. AFROMline with a trailing space or a CRLF line ending passes the test. Butdocker runthen stops withinvalid reference format. Fix: read the line the same wayseddoes. Keep lines that start withFROM, remove those 5 characters, and do not trim. - Optional: no test checks that the sentence at
skills/stash-supply-chain-security/SKILL.md:62names everypackage-ecosystemin.github/dependabot.yml. Before this PR, the sentence did not namegomod, and no test failed. Fix: add a test ine2e/tests/supply-chain.e2e.test.ts. The test expects each ecosystem, in backticks, in that sentence. - Optional: the test at
scripts/__tests__/musl-build-image.test.mjs:173accepts any schedule that is not empty. So a monthly schedule passes. Fix: assert the cron value. In this example,onis the parsedon:block of.github/workflows/musl-build-image.yml:expect(on?.schedule).toEqual([{ cron: '0 7 * * 1' }]). Use the name that the test already uses for that block. - Optional: no test checks the
Check the tools the build steps usestep in.github/workflows/musl-build-image.yml:41. Someone can remove the step, or remove a tool from its list, and the tests still pass. A release build then fails later because the tool is missing. Fix: add a test that finds the step and checks itsfor tool in ...list. - Optional: the
# Alpine 3.24.2comment at.github/docker/musl-build/Dockerfile:14becomes wrong after the first Dependabot digest PR that moves Alpine. Dependabot changes only theFROMline. Fix: remove the comment, or print/etc/alpine-releaseinmusl-build-image.yml.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 5 found, 3 posted |
| claude | claude-opus-5-5 | infracode | 3 found, 3 posted |
| codex | gpt-5.6-terra | test-gap | 3 found, 1 posted |
| codex | gpt-5.6-terra | infracode | 2 found, 2 posted |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. 3 comment(s) had a problem that stopped the reader acting; it rewrote 2. It also rewrote the review body.
Stack: not part of a stack.
Context loaded: the description, 1 linked issue(s) and 2 discussion entries.
| ALPINE_NODE_IMAGE: node:22-alpine@sha256:0a7108bf6c7bf5de370ffb1a3ed6be93d405b43ff159f681a8d18c0e2bc2e402 | ||
| run: | | ||
| set -euo pipefail | ||
| ALPINE_NODE_IMAGE=$(sed -n 's/^FROM //p' .github/docker/musl-build/Dockerfile) |
There was a problem hiding this comment.
Fix before merge: The smoke job's checkout does not include .github/docker/musl-build, so this sed cannot read the Dockerfile.
Impact: Every dispatched auth-preflight run fails in this step, before Docker starts. The preflight never installs or loads the musl tarball. So the preflight does not check that tarball before a release.
Evidence: The actions/checkout step at line 50 sets sparse-checkout: scripts. In cone mode, Git then checks out only scripts/ and the files at the repository root. The checkout does not include .github/. I reproduced the checkout with git sparse-checkout set --cone scripts. Then I ran this line, and it printed:
sed: can't read .github/docker/musl-build/Dockerfile: No such file or directory
set -euo pipefail then stops the step with exit code 2. The test smoke-test the musl artifacts on the image the Dockerfile starts from checks the sed text. It does not check the checkout, so the test passes.
Fix: Add the Dockerfile directory to the sparse checkout at line 50:
sparse-checkout: |
scripts
.github/docker/musl-buildWith this list, git sparse-checkout set --cone checks out the Dockerfile, and the sed prints the pinned image.
To stop the same mistake later, add a check to scripts/__tests__/musl-build-image.test.mjs. Find the loop in the test smoke-test the musl artifacts on the image the Dockerfile starts from. Put this code at the end of the loop body:
const job = Object.values(wf.jobs).find((j) =>
(j?.steps ?? []).some((s) => String(s?.run ?? '').includes(DOCKERFILE)),
)
const checkout = job.steps.find((s) =>
String(s?.uses ?? '').startsWith('actions/checkout'),
)
const sparse = String(checkout?.with?.['sparse-checkout'] ?? '')
.split('\n')
.map((path) => path.trim())
.filter(Boolean)
if (sparse.length > 0) expect(sparse).toContain(DOCKERFILE_DIR)The code finds the job that has a step whose run text contains DOCKERFILE. Then it finds the actions/checkout step in that job. If that step sets sparse-checkout, the code expects DOCKERFILE_DIR in the list of paths.
The code uses three names. Check that the test defines them with these values:
wfis the parsed workflow file for the current pass of the loop.DOCKERFILEis the Dockerfile path that the step'sruntext contains:.github/docker/musl-build/Dockerfile.DOCKERFILE_DIRis exactly.github/docker/musl-build, with no leading slash. With a leading slash,toContainfails.
If the test uses other names for these values, change the code to use those names.
.github/workflows/ffi-preflight.yml has the same problem at line 161.
Found by 1 model: codex
There was a problem hiding this comment.
Fixed in ee87669: .github/docker/musl-build is in the sparse checkout of both preflights, and the test now reads each smoke job's checkout and demands it.
| ALPINE_NODE_IMAGE: node:22-alpine@sha256:0a7108bf6c7bf5de370ffb1a3ed6be93d405b43ff159f681a8d18c0e2bc2e402 | ||
| run: | | ||
| set -euo pipefail | ||
| ALPINE_NODE_IMAGE=$(sed -n 's/^FROM //p' .github/docker/musl-build/Dockerfile) |
There was a problem hiding this comment.
Fix before merge: The smoke job's checkout does not include .github/docker/musl-build, so this sed cannot read the Dockerfile.
Impact: Every dispatched ffi-preflight run fails in this step, before Docker starts. The preflight never installs or loads the musl tarball. AGENTS.md says to dispatch this workflow before you merge a release that moves an FFI version. That check no longer works.
Evidence: The actions/checkout step at line 52 sets sparse-checkout: scripts. In cone mode, Git then checks out only scripts/ and the files at the repository root. The checkout does not include .github/. With that checkout, this line prints:
sed: can't read .github/docker/musl-build/Dockerfile: No such file or directory
set -euo pipefail then stops the step with exit code 2.
Fix: Add the Dockerfile directory to the sparse checkout at line 52:
sparse-checkout: |
scripts
.github/docker/musl-build.github/workflows/auth-preflight.yml needs the same change at line 50.
To stop the same mistake later, add a check to scripts/__tests__/musl-build-image.test.mjs. Find the loop in the test smoke-test the musl artifacts on the image the Dockerfile starts from. Put this code at the end of the loop body:
const job = Object.values(wf.jobs).find((j) =>
(j?.steps ?? []).some((s) => String(s?.run ?? '').includes(DOCKERFILE)),
)
const checkout = job.steps.find((s) =>
String(s?.uses ?? '').startsWith('actions/checkout'),
)
const sparse = String(checkout?.with?.['sparse-checkout'] ?? '')
.split('\n')
.map((path) => path.trim())
.filter(Boolean)
if (sparse.length > 0) expect(sparse).toContain(DOCKERFILE_DIR)The code finds the job that has a step whose run text contains DOCKERFILE. Then it finds the actions/checkout step in that job. If that step sets sparse-checkout, the code expects DOCKERFILE_DIR in the list of paths.
The code uses three names. Check that the test defines them with these values:
wfis the parsed workflow file for the current pass of the loop.DOCKERFILEis the Dockerfile path that the step'sruntext contains:.github/docker/musl-build/Dockerfile.DOCKERFILE_DIRis exactly.github/docker/musl-build, with no leading slash. With a leading slash,toContainfails.
If the test uses other names for these values, change the code to use those names.
Found by 1 model: codex
|
|
||
| # Alpine 3.24.2 | ||
| RUN apk add --no-cache \ | ||
| build-base=0.5-r4 \ |
There was a problem hiding this comment.
Change before merge: build-base=0.5-r4 pins a metapackage, so the C compiler and the linker are still not pinned.
Impact: Two builds of one commit can still use different compilers. That is the problem #1042 describes. build-base depends on gcc, g++, binutils, make and libc-dev with no version. Alpine keeps only the newest build of each package in the 3.24 index. When Alpine publishes a new gcc build, build-base=0.5-r4 still resolves and installs it. The weekly musl-build-image.yml check stays green. The release then links the binary with a different gcc and ld, under the same provenance. 39 of the 46 packages that this apk add installs have no pin.
Evidence: In the pinned image (Alpine 3.24.2):
$ apk info -R build-base
build-base-0.5-r4 depends on:
binutils
file
gcc
g++
make
libc-dev
fortify-headers
patch
$ apk add --simulate --no-cache build-base=0.5-r4 cmake=4.2.3-r0 ... rustup=1.29.0-r0
( 3/46) Installing binutils (2.45.1-r1)
(13/46) Installing gcc (15.2.0-r5)
(15/46) Installing musl-dev (1.2.6-r2)
(16/46) Installing g++ (15.2.0-r5)
(17/46) Installing make (4.4.1-r4)
Fix: Also pin the packages that compile and link the binary: gcc, g++, binutils, musl-dev and make. Add the same names to TOOLCHAIN in scripts/__tests__/musl-build-image.test.mjs, so the test fails if a pin is removed. This list resolves in the pinned image (apk add --simulate prints OK):
RUN apk add --no-cache \
binutils=2.45.1-r1 \
build-base=0.5-r4 \
cmake=4.2.3-r0 \
curl=8.22.0-r0 \
g++=15.2.0-r5 \
gcc=15.2.0-r5 \
git=2.54.0-r0 \
linux-headers=7.0.0-r1 \
make=4.4.1-r4 \
musl-dev=1.2.6-r2 \
perl=5.42.2-r1 \
rustup=1.29.0-r0Each new pin can stop resolving, like the existing ones. gcc and binutils change rarely inside one Alpine stable branch. If you do not want more pins, change the Dockerfile comment and the PR description so they do not say the compiler is pinned. Then keep #1042 open for the compiler.
Found by 1 model: claude
There was a problem hiding this comment.
Fixed in ee87669: binutils, g++, gcc, make and musl-dev pinned at the versions you listed (image rebuilt locally, 46 packages, OK), and all five added to TOOLCHAIN.
|
|
||
| it('name the base image digest nowhere, so the Dockerfile is its one home', () => { | ||
| const copies = workflows | ||
| .filter(({ text }) => /node:\d+-alpine@sha256:/.test(text)) |
There was a problem hiding this comment.
Change before merge: This test passes when a workflow names node:22-alpine with no digest.
Impact: The regex matches only node:<n>-alpine@sha256:. A step that runs docker run node:22-alpine ... passes the test. That tag can move to a new image at any time, which is the drift #1042 describes. The error message tells the engineer to remove the copy. Deleting only the @sha256:… part also makes the test pass.
Evidence: This PR removed expect(run).not.toMatch(/\bnode:\d+-alpine(?!@)/) from auth-build-artifacts.test.mjs and ffi-build-artifacts.test.mjs. No test in scripts/__tests__/ checks for a tag with no digest now. No workflow names node:<n>-alpine today, so the wider pattern below passes on this branch.
Fix: Match any reference to the node Alpine image, with or without a digest:
it('name the node Alpine image nowhere, so the Dockerfile is its one home', () => {
// A copy with no digest is worse than a stale copy: the tag can move.
const copies = workflows
.filter(({ text }) => /\bnode:\d+-alpine\b/.test(text))
.map(({ relPath }) => relPath)
expect(
copies,
`These workflows name the node Alpine image themselves, pinned or not. Read it from ${DOCKERFILE} as the preflights do.`,
).toEqual([])
})Found by 1 model: claude
There was a problem hiding this comment.
Fixed in ee87669: the test matches node:<n>-alpine with or without a digest.
| - ".github/docker/musl-build/**" | ||
| - ".github/workflows/musl-build-image.yml" | ||
| schedule: | ||
| - cron: "0 7 * * 1" |
There was a problem hiding this comment.
Fix in a follow-up: When the weekly run fails, GitHub notifies only one person.
Impact: GitHub sends the failure notice for a schedule run only to the user who created the workflow, or to the last user who changed its cron line. No issue opens, and no one else gets a message. If that person misses the notice, the team first sees the failure in a release build. This workflow exists to stop that outcome.
Evidence: GitHub documentation, "Notifications for workflow runs": "Notifications for scheduled workflows are sent to the user who initially created the workflow. If a different user updates the cron syntax in the workflow file, subsequent notifications will be sent to that user instead." No scheduled workflow in .github/workflows/ opens an issue or sends a message today. One shared fix can serve all of them.
Fix: Open an issue when the scheduled run fails. Give issues: write to this job only:
jobs:
build:
permissions:
contents: read
issues: write
steps:
# ... existing steps ...
- name: Open an issue when the weekly build fails
if: ${{ failure() && github.event_name == 'schedule' }}
env:
GH_TOKEN: ${{ github.token }}
run: |
gh issue create --repo "$GITHUB_REPOSITORY" \
--title "musl build image no longer builds" \
--body "Run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID. Re-pin with the command in .github/docker/musl-build/Dockerfile."Found by 1 model: claude
There was a problem hiding this comment.
Taken now rather than later, in ee87669: the job has issues: write and opens an issue on a failed scheduled run; the test asserts the step's if.
| // Join `\`-continued lines so one instruction is one string. | ||
| const instructions = text.replace(/\\\n/g, ' ').split('\n') | ||
| return instructions | ||
| .filter((line) => /^RUN\s+apk\s+add\b/.test(line)) |
There was a problem hiding this comment.
Fix in a follow-up: apkAdds() reads only lines that start with RUN apk add, so other apk add forms skip the pin check.
Impact: A line such as RUN apk update && apk add --no-cache curl installs curl with no version. The test pins every apk package to an exact version still passes. The Dockerfile comment shows the form apk update >/dev/null && apk search …, so an engineer can easily copy it into a RUN line. No test checks for apk upgrade, which also changes package versions with no pin.
Evidence: I ran apkAdds() on a Dockerfile with RUN apk add --no-cache git=1-r0 and RUN apk update && apk add --no-cache curl. It returned [["git=1-r0"]].
Fix: Count every apk add in the Dockerfile's instructions, and require the parser to see all of them:
it('reads every apk add in the Dockerfile, so none skips the pin check', () => {
const code = dockerfile
.split('\n')
.filter((line) => !/^\s*#/.test(line))
.join('\n')
const written = code.match(/\bapk\s+add\b/g) ?? []
expect(written.length).toBeGreaterThan(0)
expect(
apkAdds(dockerfile),
'An `apk add` is not at the start of a `RUN` line, so its packages are not checked for a `=version`.',
).toHaveLength(written.length)
expect(code).not.toMatch(/\bapk\s+upgrade\b/)
})This test passes on this branch.
Found by 1 model: claude
There was a problem hiding this comment.
Fixed in ee87669: your test as written, over the Dockerfile's instructions with comments dropped, plus the apk upgrade refusal.
| interval: weekly | ||
| day: monday | ||
| cooldown: | ||
| default-days: 7 |
There was a problem hiding this comment.
Optional: No test checks the cooldown or the schedule of the new docker Dependabot entry.
Impact: The skill now says Dependabot opens 7-day cooldown PRs for docker. If someone deletes cooldown: or changes the schedule here, no test fails. Dependabot can then propose a new base image digest on the day it is published.
Evidence: e2e/tests/supply-chain.e2e.test.ts checks the cooldown only for npm and github-actions. scripts/__tests__/musl-build-image.test.mjs checks only this entry's directory. Every entry in .github/dependabot.yml has default-days: 7 today, so a check on every entry passes on this branch.
Fix: In e2e/tests/supply-chain.e2e.test.ts, add this to the supply chain — automated dependency updates (Dependabot) block:
it('every entry has a ≥ 3 day cooldown', () => {
expect(db.updates.length).toBeGreaterThan(0)
for (const entry of db.updates) {
expect(
entry.cooldown?.['default-days'] ?? 0,
`${entry['package-ecosystem']} at ${entry.directory} has no cooldown of at least 3 days`,
).toBeGreaterThanOrEqual(3)
}
})Then change "cooldown ≥ 3 days on npm/github-actions" on the "Test asserts" line in skills/stash-supply-chain-security/SKILL.md to "cooldown ≥ 3 days on every entry". To also keep the weekly schedule, extend the Dependabot test in musl-build-image.test.mjs:
const entry = docker.find((update) => update.directory === `/${DOCKERFILE_DIR}`)
expect(entry).toMatchObject({ schedule: { interval: 'weekly' } })Found by 2 models: claude, codex
There was a problem hiding this comment.
Fixed in ee87669: every-entry cooldown test in supply-chain.e2e.test.ts, the skill's "Test asserts" line updated, and the docker entry's weekly schedule asserted in musl-build-image.test.mjs.
| open-pull-requests-limit: 2 | ||
| labels: | ||
| - dependencies | ||
| - github-actions |
There was a problem hiding this comment.
Optional: The docker entry uses the github-actions label, but it does not update GitHub Actions.
Impact: A Dependabot PR that moves the Alpine base image appears under the github-actions label. The npm, cargo and gomod entries use supply-chain. No workflow reads these labels, so only people who filter PRs by label see the wrong one.
Fix: Use the same label as the other entries that are not GitHub Actions:
labels:
- dependencies
- supply-chainFound by 1 model: claude
…can move The linux-x64-musl binaries of @cipherstash/auth and @cipherstash/protect-ffi were built in node:22-alpine named by digest inside a `run:` script, where Dependabot's github-actions ecosystem cannot see it, so nothing would ever move the digest; and the container ran an unpinned `apk add`, so two builds of one commit could use different compilers under the same provenance. The image is now .github/docker/musl-build/Dockerfile: the base image by digest, which a new Dependabot docker entry updates, and every apk package at an exact version. Both build workflows `docker build --pull` it and run in the result; both preflights read its FROM line for their smoke test, so the digest has one home. Alpine drops a package version from its index when it publishes the next one, so a pin can stop resolving between Dependabot runs; musl-build-image.yml builds the Dockerfile weekly and on every change so that is a failed check rather than a failed release. musl-build-image.test.mjs holds the digest pin, the version pins, the Dependabot entry, and that no workflow carries its own copy of the digest or its own `apk add`. Closes #1042. Found by cipherstash-bot in review of #1018.
…er, and widen the guards The preflights sparse-checkout `scripts` alone, so the smoke test's `sed` over the Dockerfile read nothing and every dispatched run died before Docker started; the directory is in both checkouts now, and the test demands it. `build-base` is a metapackage whose gcc, g++, binutils, musl-dev and make dependencies carry no version, so pinning it alone left the compiler unpinned — the five are pinned beside it and demanded by name. Guards from the review: no workflow may name `node:<n>-alpine` with or without a digest; every `apk add` in the Dockerfile must be one the pin check parses, and no `apk upgrade`; the FROM line is read as the preflights' `sed` reads it; the weekly cron, the tools check step and the Dependabot schedule are asserted; every Dependabot entry needs a cooldown, and the skill must name every monitored ecosystem. The weekly run opens an issue when it fails, since GitHub mails a scheduled failure to one person. The docker entry carries the `supply-chain` label the other non-Actions entries use, and the Alpine version is printed by the check rather than written in a comment Dependabot would leave stale. Review: cipherstash-bot on #1107.
8d0fac1 to
ee87669
Compare
|
The five body-only findings are in ee87669 too: |
3 participants
Summary
The Linux musl builds of
@cipherstash/authand@cipherstash/protect-ffi(the native modules that let Node.js on Alpine Linux talk to CipherStash) are compiled inside an Alpine Docker image. That image was named by digest inside a shell script, where Dependabot cannot see it, so its security fixes would never have arrived; and the compiler was installed with an unpinnedapk add, so two builds of one commit could differ while carrying the same provenance. The image now lives in a Dockerfile with both the base digest and every package pinned, Dependabot updates the digest, and a weekly check builds the Dockerfile so a pin that stops resolving fails a check rather than a release.Changes
.github/docker/musl-build/Dockerfile— new.FROM node:22-alpine@sha256:…plusapk addof the seven build packages at exact versions (Alpine 3.24.2, resolved from the pinned image today)..github/workflows/_build-auth-artifacts.yml,_build-ffi-artifacts.yml— the musl leg runsdocker build --pullon that directory and builds in the result; the inlineapk addis gone..github/workflows/auth-preflight.yml,ffi-preflight.yml— the musl smoke test reads the base image from the Dockerfile'sFROMline, so there is one copy of the digest. It runs on plain Node.js-on-musl, as a user's container would..github/dependabot.yml— adockerentry for/.github/docker/musl-build, weekly, 7-day cooldown, majors ignored like every other entry..github/workflows/musl-build-image.yml— new. Builds the Dockerfile on changes to it and every Monday, and checks the tools the build steps use are present.scripts/__tests__/musl-build-image.test.mjs— new. Pins: one digest-pinnedFROM; every apk packagename=version; the toolchain list; both build workflows build from the Dockerfile; no workflow carries the digest or anapk add; the Dependabot entry; the check workflow's triggers.scripts/__tests__/auth-build-artifacts.test.mjs,ffi-build-artifacts.test.mjs— assert the built image rather thanALPINE_NODE_IMAGE.e2e/tests/supply-chain.e2e.test.ts—docker→Dockerfilein the manifest map, so the "every entry's directory contains its manifest" check knows the ecosystem.skills/stash-supply-chain-security/SKILL.md— namesgomodanddockeramong the monitored ecosystems (the sentence already omittedgomod). Changeset:stashpatch.Verification
docker build --pull --platform linux/amd64 .github/docker/musl-buildsucceeds locally with the pins as written.pnpm run test:scripts— 70 files, 1259 tests pass.pnpm --filter ./e2e exec vitest run tests/supply-chain.e2e.test.ts -t "ependabot|ecosystem|cooldown|majors"— 6 pass.auth-preflight.ymlorffi-preflight.ymldispatched against this branch would exercise the build leg and the smoke test;musl-build-image.ymlruns on this PR because it changes the Dockerfile.Related
apk addinto the FFI build, which this PR also covers.Review notes
The trade-off worth a decision: pinning
apkversions makes the build reproducible, and makes it fail when Alpine rotates a package (it removes the superseded version from the index, often within days forcurlandgit). The weeklymusl-build-image.ymlrun is what turns that into a check rather than a surprise during a release; re-pinning is the one-lineapk search --exactthe Dockerfile comment gives. If a failing release is judged worse than an unreproducible one, drop the=versionpins and the test that demands them, and keep the rest.🤖 Generated with Claude Code
https://claude.ai/code/session_01AniWvz2FadFRNCYs3dDfEs