fix(release): name the crate for neon dist, which pnpm exec cannot default - #882
Merged
Conversation
…default Every platform leg of `_build-ffi-artifacts.yml` failed with `error: $npm_package_name is not defined`, so the FFI release pipeline could not have produced a single tarball. Found by the first dispatch of `ffi-preflight.yml` (run 31555436823) — which is what it is for. `neon dist` locates the compiled cdylib in the cargo log by crate name, defaulting it to `basename($npm_package_name)`. That variable comes from the package-script runner. The step invokes the binary directly, because that is the only way to pass `-o platforms/<platform>/index.node` and so avoid the bare-`neon dist` default of `./index.node` — the `debug:` fallback in `load.cts`, right locally and wrong in a published tarball. Upstream never met this: its `postcargo-build` / `postzig-build` hooks run `neon dist` as lifecycle scripts, where the variable exists. Nor would the plan's spelling have, which was `npx` — npx sets `npm_package_name`, `pnpm exec` does not. Moving to `pnpm exec` was right on its own terms, npx will fetch a missing binary over the network, but it removed the default's only source and nothing said so. `scripts/__tests__/neon-dist-crate-name.test.mjs` pins both halves across every workflow, discovered rather than listed: the flag is present, and its value equals the crate in `crates/protect-ffi/Cargo.toml`. The second is the quiet one — a renamed crate with a stale `-n` does not error, it generates nothing. No changeset: repo tooling, no published surface. The pipeline remains inert until the trusted-publishing cutover.
|
…lure
The second defect the pre-flight found, and the one the first was hiding.
With the crate name fixed, both Darwin legs went green and the other four
failed one step later: `<tarball> has no index.node`, on a tarball whose
own pack listing shows index.node present.
`grep -q` exits at the first match. The writer upstream then takes
SIGPIPE, and under `pipefail` — which GitHub sets for every `shell: bash`
step before the block's own `set -euo pipefail` — the pipeline's status
becomes the killed writer's 141. The match succeeded and the pipeline
reports failure.
It presents as a platform problem, which is why it cost a full matrix run
to see. GNU tar writes an entry at a time and hits it; bsdtar buffers a
four-entry listing into one write and does not. Hence macOS green, Linux
and Windows red, on the packaging step, in a job that had just changed.
Three call sites, all in the FFI release path, and they do not fail in the
same direction:
- `_build-ffi-artifacts.yml` `tar tzf | grep -qx package/index.node`
fails CLOSED — this run's visible failure.
- `ffi-preflight.yml`'s glibc check on the two gnu platforms, also
closed.
- `ffi-preflight.yml`'s musl check fails OPEN. `if readelf | grep -q`
with a poisoned status is false, so it reports "no glibc NEEDED entry"
for precisely the binary it exists to reject — a gnu build shipped
inside the musl package, landing on an Alpine user at dlopen rather
than in CI. A dynamic section is much longer than a tarball listing,
so it would have been reliably wrong. Neither preflight check had ever
executed.
All three capture first and match against the variable.
`scripts/__tests__/workflow-grep-q-pipelines.test.mjs` rejects the pattern
across every workflow and composite action. `grep -q` against a file or a
here-string is untouched: no writer, no signal.
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Coverage review: found one targeted gap in the new workflow guard test. The workflow change itself has a positive guard for the current invocation, but the scanner is not covered for multiple neon dist invocations inside one run: block.
| describe('neon dist through pnpm exec', () => { | ||
| const calls = workflowFiles().flatMap((file) => | ||
| runSteps(readWorkflow(file)) | ||
| .filter((step) => NEON_DIST.test(step.run)) |
There was a problem hiding this comment.
Gap: The guard has no negative test for a single workflow step containing multiple pnpm exec neon dist invocations, so one correctly named call can mask a later unnamed call in the same run: block.
it('does not let an unnamed second neon dist call hide behind a named first one', () => {
const run = [
`pnpm exec neon dist -n ${crateName()} -o one < cargo.log`,
'pnpm exec neon dist -o two < cargo.log',
].join('\n')
const names = run
.split('\n')
.filter((line) => /pnpm\s+exec\s+neon\s+dist\b/.test(line))
.map((line) => line.match(/(?:^|\s)(?:-n|--name)\s+(\S+)/)?.[1])
expect(names).toEqual([crateName(), crateName()])
})Expected: this fails today with the second entry undefined, then passes after the scanner validates each neon dist invocation rather than only the first named call in a step.
Review finding, and the gap was wider than reported. The guard matched a single non-global regex over the whole `run:` body, so a step with two `neon dist` calls was judged by whichever `-n` appeared first — and the flag it found did not have to belong to a `neon dist` at all. `echo -n protect-ffi` on an earlier line satisfied it, as the new coverage shows. The scan now splits a body into individual commands (continuations joined first, so a newline is a real boundary) and holds every invocation to the crate name separately. Two tests drive the scanner over shapes no workflow in the tree has: a second unnamed call behind a correctly named one, and a `-n` belonging to a neighbouring command. Both fail against the previous body-wide match — `length 1, expected 2` and `['protect-ffi'], expected [undefined]` — so they pin the fix rather than restating it. Reported alongside a suggested test that scanned an inline string with logic defined in the test body. That version never calls the scanner, so its result does not move when the scanner changes: it would have failed before and after. These call the real thing.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The FFI release pipeline merged in #878 could not have produced a single tarball. The first dispatch of
ffi-preflight.yml— run 31555436823, againstmain— failed all six platform legs at the same step,Place the binding in its platform package:The binding itself built fine on every platform. This is the step after it.
Why
neon distlocates the compiled cdylib in the cargo log by crate name, and defaults that name tobasename($npm_package_name)—@neon-rs/cli/index.js,ensureDefined(process.env['npm_package_name'], '$npm_package_name'). That variable is set by the package-script runner.The step has to invoke the binary directly, because that is the only way to pass
-o platforms/<platform>/index.node. A bareneon distwrites./index.node, which is thedebug:fallback inload.cts— right for local development, wrong for a package about to be published.Two reasons nobody caught it earlier, and neither was carelessness:
Upstream never met it.
protectjs-ffireachedneon distonly through thepostcargo-build/postzig-buildlifecycle hooks, where the variable exists.The plan's spelling would not have either. It carried
npx neon dist -o …; the implementation usespnpm exec. Measured locally:Moving to
pnpm execis right on its own terms —npxwill fetch a missing binary over the network — but it removed the default's only source, and nothing in the diff said so.The fix
-nnames the crate (crates/protect-ffi/Cargo.toml), not the npm package.The guard
scripts/__tests__/neon-dist-crate-name.test.mjsasserts, across every workflow and by discovery rather than a list, that eachpnpm exec neon distpasses-nand that its value matches the crate inCargo.toml.Both halves earn their place. Dropping the flag fails loudly and identically on all six platforms. Renaming the crate without updating the flag does not —
neon distfinds no matching artifact and generates nothing, which fails a step later as an empty output rather than an error.Mutation-tested both ways (flag removed; name changed to
protectffi); each fails the guard.Verification
actionlinton all four release workflows — clean, shellcheck includedbiome check— cleanpnpm run test:scripts— 31 files, 424 tests, green-n, usage; with-n protect-ffi,error: No artifacts were generated for crate protect-ffi— the flag is parsed and the crate lookup runsThis cannot be fully verified without another preflight dispatch, which is the next step once this is on a branch CI can reach:
Notes
lint-no-ffi-changeset.mjswould reject one naming these packages anyway.docs/plans/2026-08-04-protect-ffi-monorepo-absorption.mdis updated in both places that describe this step — theneon distsection and the inline workflow source, which still showednpx.ffi-preflight.ymlruns green against a versioned release-PR ref", and it was never ticked because the workflow had never run. That box was load-bearing.