fix(ci): build the musl @cipherstash/auth binary in Alpine, and check every binary's C library #1018
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,8 @@ describe('_build-auth-artifacts.yml', () => { | |
|
|
||
| it('builds each binding without writing over the committed loader', () => { | ||
| const build = runs(binaries).filter((run) => /\bnapi build\b/.test(run)) | ||
| // One host build, and one inside Alpine for the musl leg. | ||
| expect(build).toHaveLength(2) | ||
|
There was a problem hiding this comment. Change before merge: this test now expects two Impact: Fix: Check the order against every build step. it('fails the leg when the build changed a tracked file', () => {
const all = runs(binaries)
const builds = all
.map((run, index) => index)
.filter((index) => /\bnapi build\b/.test(all[index]))
const check = all.findIndex((run) =>
new RegExp(`git diff --exit-code\\b.*${AUTH_DIR}`).test(run),
)
expect(builds).toHaveLength(2)
for (const build of builds) expect(check).toBeGreaterThan(build)
})Found by 1 model: claude
Contributor
Author
There was a problem hiding this comment. Fixed in |
||
| for (const flag of [ | ||
| '--platform', | ||
| '--release', | ||
|
|
@@ -60,17 +61,107 @@ describe('_build-auth-artifacts.yml', () => { | |
| '--dts native.d.ts', | ||
| '--js false', | ||
| ]) { | ||
| for (const run of build) expect(run).toContain(flag) | ||
| } | ||
| }) | ||
|
|
||
| it('fails the leg when either build changed a tracked file', () => { | ||
| const all = runs(binaries) | ||
| const builds = all | ||
| .map((_, index) => index) | ||
| .filter((index) => /\bnapi build\b/.test(all[index])) | ||
| const check = all.findIndex((run) => | ||
| new RegExp(`git diff --exit-code\\b.*${AUTH_DIR}`).test(run), | ||
| ) | ||
| expect(builds).toHaveLength(2) | ||
| for (const build of builds) expect(check).toBeGreaterThan(build) | ||
| }) | ||
|
|
||
| it('builds the musl binding inside Alpine, from an image pinned by digest', () => { | ||
| // Built on the Ubuntu runner, the musl binary linked glibc and failed to | ||
| // load on musl. Alpine is a musl system, so its compiler links musl. | ||
| const steps = binaries?.steps ?? [] | ||
| const musl = steps.filter((step) => | ||
| String(step?.if ?? '').includes("== 'linux-x64-musl'"), | ||
| ) | ||
| const run = musl.map((step) => String(step?.run ?? '')).join('\n') | ||
| const env = Object.assign({}, ...musl.map((step) => step?.env ?? {})) | ||
| expect(env.ALPINE_NODE_IMAGE).toMatch(/-alpine@sha256:[0-9a-f]{64}$/) | ||
|
There was a problem hiding this comment. Change before merge: this test checks the value of Impact: An edit can write Fix: Assert that expect(env.ALPINE_NODE_IMAGE).toMatch(/-alpine@sha256:[0-9a-f]{64}$/)
expect(run).toMatch(/docker run[\s\S]*"\$ALPINE_NODE_IMAGE"/)
expect(run).not.toMatch(/docker run[\s\S]*\bnode:\d+-alpine(?!@)/)Found by 1 model: claude
Contributor
Author
There was a problem hiding this comment. Fixed in Keeping the digest current, and pinning the |
||
| // The pinned image is the one that runs, not a mutable tag. | ||
| expect(run).toMatch(/docker run[\s\S]*"\$ALPINE_NODE_IMAGE"/) | ||
| expect(run).not.toMatch(/\bnode:\d+-alpine(?!@)/) | ||
| expect(run).toContain('RUSTFLAGS="-C target-feature=-crt-static"') | ||
| // The container loads the binary it built, on musl. | ||
| expect(run).toMatch( | ||
| /napi build[\s\S]*node -e "require\(process\.argv\[1\]\)" "\$PWD\/stack-auth-node\.linux-x64-musl\.node"/, | ||
| ) | ||
| // The container hands its files back even when the build fails. | ||
| expect(run).toMatch(/^\s*trap "chown -R .*\/build" EXIT$/m) | ||
| // The host steps that only the host build uses skip the musl leg. | ||
| for (const name of [ | ||
| 'Add the Rust target', | ||
| 'Install node-gyp', | ||
| 'Install dependencies', | ||
| 'Build the native binding', | ||
| ]) { | ||
| const step = steps.find((s) => s?.name === name) | ||
| expect(String(step?.if ?? '')).toContain("!= 'linux-x64-musl'") | ||
| } | ||
| }) | ||
|
|
||
| it('checks the C library of every Linux binary before it is packed', () => { | ||
| // Without this, only a hand-run auth-preflight sees a glibc-linked musl | ||
| // binary, and a release would publish it. | ||
| const steps = binaries?.steps ?? [] | ||
| const check = steps.findIndex((step) => | ||
| String(step?.run ?? '').includes('readelf -d'), | ||
| ) | ||
| const pack = steps.findIndex((step) => | ||
| /\bnpm pack\b/.test(String(step?.run ?? '')), | ||
| ) | ||
| expect(check).toBeGreaterThan(-1) | ||
| expect(check).toBeLessThan(pack) | ||
| const run = String(steps[check].run) | ||
| expect(String(steps[check].if)).toContain("runner.os == 'Linux'") | ||
| expect(run).toMatch( | ||
| /linux-x64-gnu\|linux-arm64-gnu\)[\s\S]*libc\\\.so\\\.6/, | ||
| ) | ||
| expect(run).toContain('linux-x64-musl links glibc') | ||
| // A static binary has no libc entry, so musl must be named, not only | ||
| // glibc refused. | ||
| expect(run).toMatch(/linux-x64-musl\)[\s\S]*libc\\\.musl-/) | ||
| // Every rejection fails the job, so the binary is never packed. | ||
| const errors = run.match(/::error::/g) ?? [] | ||
| expect(errors.length).toBeGreaterThan(0) | ||
| expect((run.match(/exit 1/g) ?? []).length).toBe(errors.length) | ||
| // Every Linux platform in the matrix has a rule. | ||
| const linux = (binaries?.strategy?.matrix?.include ?? []) | ||
| .filter((leg) => String(leg.os).startsWith('ubuntu')) | ||
| .map((leg) => leg.platform) | ||
| expect(linux.length).toBeGreaterThan(0) | ||
| for (const platform of linux) { | ||
| expect(run).toMatch(new RegExp(`(^|[|\\s])${platform}[|)]`, 'm')) | ||
| } | ||
| }) | ||
|
|
||
| it('auth-preflight requires musl, and loads the musl artifact inside Alpine', () => { | ||
| const preflight = readWorkflow('.github/workflows/auth-preflight.yml') | ||
| const steps = preflight?.jobs?.smoke?.steps ?? [] | ||
| const verify = steps.map((step) => String(step?.run ?? '')).join('\n') | ||
| expect(verify).toMatch(/linux-x64-musl\)[\s\S]*libc\\\.musl-/) | ||
| // The host smoke test installs only the runner's own platform, so without | ||
| // this step no job loads the musl binary. | ||
| const alpine = steps.find((step) => | ||
| /\bdocker run\b/.test(String(step?.run ?? '')), | ||
| ) | ||
| expect(alpine).toBeDefined() | ||
| expect(String(alpine.run)).toContain('linux-x64-musl') | ||
| expect(String(alpine.run)).toMatch(/docker run[\s\S]*"\$ALPINE_NODE_IMAGE"/) | ||
| // The same image as the build, so the load test matches the build. | ||
| const build = (binaries?.steps ?? []).find( | ||
| (step) => step?.env?.ALPINE_NODE_IMAGE, | ||
| ) | ||
| expect(alpine.env?.ALPINE_NODE_IMAGE).toBe(build?.env?.ALPINE_NODE_IMAGE) | ||
| }) | ||
|
|
||
| it('packs the wrapper with pnpm, which rewrites its workspace peers', () => { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in a follow-up: this comment says the check is the same as the one in
auth-preflight.yml, and nothing in the repository keeps the two copies the same.Impact: The same C library rules now exist in three files: here,
auth-preflight.yml:84-97andffi-preflight.yml:109-118. If someone changes a rule or adds a platform in one file, the others keep the old rule. The build step would then accept a binary the preflight rejects, or the reverse. The preflight is the dry run people read before a release, so the two disagreeing is worse than either rule alone. The new test inscripts/__tests__/auth-build-artifacts.test.mjsreads only this file.Fix: Put the rules in one shell script, for example
.github/scripts/check-c-library.sh <platform> <binary-path>, and call it from both auth workflows. If you keep the copies, add a test that reads thecaseblock out of each file and asserts they match. The repository already does this for duplicated workflow logic inscripts/__tests__/eql-workflow-filters.test.mjs.Found by 1 model: claude
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now Linear issue CIP-4284. It moves the rules into one script that all three workflows call, and tests that script directly.