Skip to content

ci(release): drop redundant second Setup Node.JS in release-npm#3702

Merged
shumkov merged 2 commits into
v3.1-devfrom
ci/fix-release-node-pin
May 20, 2026
Merged

ci(release): drop redundant second Setup Node.JS in release-npm#3702
shumkov merged 2 commits into
v3.1-devfrom
ci/fix-release-node-pin

Conversation

@shumkov

@shumkov shumkov commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Remove the second Setup Node.JS step in release-npm. It used to bump Node 20 → 24 mid-job for trusted-publishers; the composite action now defaults to Node 24+ so the second call is pure dead-weight (and ~30s of re-running yarn install --inline-builds).
  • Add a short comment above the surviving Setup Node.JS step recording the Node 24+ requirement so future maintainers do not lower the version.

Background

release-npm set up Node twice in the same runner: once for the build with the composite action default, then again right before npm publish with node-version: 24 to satisfy trusted-publishers OIDC (which requires npm 11.5.1+, shipped only with Node 24+).

That second call dates back to #2923, when the composite action still defaulted to Node 20. Since then the composite default has been bumped to 24.14.1, so both calls install the same Node binary — the second one only repeats yarn install --inline-builds.

The repeat is also what broke the v3.1.0-dev.2 release: the override node-version: 24 widened the pin past the composite default and let actions/setup-node resolve to 24.15.0 from the runner cache. 24.15.0 ships a regression in the ESM CJS translator that raises EBADF: bad file descriptor, fstat during native postinstall scripts (bufferutil, utf-8-validate, unrs-resolver, @pshenmic/zeromq).

bufferutil@npm:4.0.6 couldn't be built successfully (exit code 1)
utf-8-validate@npm:5.0.9 couldn't be built successfully (exit code 1)
unrs-resolver@npm:1.11.1 couldn't be built successfully (exit code 1)
@pshenmic/zeromq@npm:6.0.0-beta.22 couldn't be built successfully (exit code 127)
...
Error: EBADF: bad file descriptor, fstat
Node.js v24.15.0

The composite action already documents the regression:

# Pinned: Node 24.15.0 shipped a regression in the ESM loader's
# CJS translation path that raises `EBADF: bad file descriptor,
# fstat` in getSourceSync -> readFileSync during postinstall
# scripts of native packages (bufferutil, utf-8-validate,
# unrs-resolver, @pshenmic/zeromq). 24.14.1 is unaffected.
default: "24.14.1"

Dropping the second Setup Node.JS step removes the bug class entirely (no second yarn install ⇒ no native-module rebuild under a possibly different Node ABI), is simpler than narrowing the override to 24.14.1, and saves ~30s per release run.

History of this PR

This branch initially landed two commits:

  1. Narrow the override from node-version: 2424.14.1 and add a verify-release-node-pin.yml workflow to exercise the two-call pattern. The verify job ran green on this PR, confirming both the failure mechanism and the fix.
  2. After identifying that the second call is redundant, drop the step entirely and remove the verify workflow (the pattern it was guarding no longer exists).

Test plan

  • CI green on this PR
  • After merge, re-tag v3.1.0-dev.2 against a commit including this fix (or wait for the next release bump) and confirm release-npm completes through Publish NPM packages

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Improved release workflow configuration to simplify build process setup and reduce redundant steps.

Review Change Stack

The release-npm job calls `./.github/actions/nodejs` twice: first with
the composite default (24.14.1), then again right before `npm publish`
with `node-version: 24` to satisfy the trusted-publishers requirement.
actions/setup-node resolves "24" to the latest 24.x in the runner cache.
On 2026-05-20 that was 24.15.0, which ships a regression in the ESM CJS
translator: `EBADF: bad file descriptor, fstat` during native postinstall
scripts (bufferutil, utf-8-validate, unrs-resolver, @pshenmic/zeromq).
Result: `yarn install` failed three retries on the v3.1.0-dev.2 release.

The composite action already pins to 24.14.1 to avoid this regression;
the call site just needs to honour the pin instead of widening it to "24".

Also add `verify-release-node-pin.yml`, a workflow_dispatch + path-scoped
PR job that mirrors the two-step Setup Node.JS pattern and runs
`yarn install --inline-builds` twice. Safe to delete after the next
release goes green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shumkov
shumkov requested a review from QuantumExplorer as a code owner May 20, 2026 17:04
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fe371a1d-7889-4c44-bf08-8077205d1d7d

📥 Commits

Reviewing files that changed from the base of the PR and between d3d1796 and 2ad0286.

📒 Files selected for processing (1)
  • .github/workflows/release.yml

📝 Walkthrough

Walkthrough

The release workflow is simplified to rely on the composite Setup Node.JS action's default Node 24+ configuration. Clarifying comments are added, and explicit version pins and a duplicate setup step are removed, reducing redundancy in the publish process.

Changes

Release workflow Node version simplification

Layer / File(s) Summary
Simplify Node version configuration in release workflow
.github/workflows/release.yml
Composite Setup Node.JS action step adds comments describing Node 24+ provision, removes inline node-version pin, and eliminates duplicate Setup Node.JS step before Publish NPM packages. Workflow now relies on composite action defaults instead of explicit overrides.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 The workflow now trusts the composite way,
Pins removed, comments stay to guide the day,
Node 24 flows through defaults so clean,
Simpler pipelines, more graceful to be seen! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: removing a redundant second Node.JS setup step in the release workflow to rely on the composite action's pinned default.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/fix-release-node-pin

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added this to the v3.1.0 milestone May 20, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/verify-release-node-pin.yml:
- Around line 51-52: The "Confirm yarn workspaces resolve" step runs "yarn
workspaces list --json | head -5" but can mask a failing yarn exit code due to
the pipe; ensure the pipeline fails when yarn fails by enabling pipefail for
that run — for example, run the command under bash with pipefail (or call set -o
pipefail before the pipeline) so that a non-zero exit from "yarn workspaces list
--json" is propagated and the job fails.
- Around line 30-32: Replace the floating actions/checkout@v4 usage with a
pinned full commit SHA and explicitly disable credential persistence; update the
checkout step (the uses: actions/checkout entry) to reference the repository
commit SHA instead of "v4" and add the persist-credentials: false input so the
step does not store GITHUB_TOKEN for subsequent steps.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 07b2b0b5-1a72-4e03-997d-78bedbff92f3

📥 Commits

Reviewing files that changed from the base of the PR and between 637a8e5 and d3d1796.

📒 Files selected for processing (2)
  • .github/workflows/release.yml
  • .github/workflows/verify-release-node-pin.yml

Comment thread .github/workflows/verify-release-node-pin.yml Outdated
Comment thread .github/workflows/verify-release-node-pin.yml Outdated

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Narrowly-scoped CI fix pinning the second Setup Node.JS call in the release-npm job to 24.14.1 to match the composite action's default and dodge the Node 24.15.0 EBADF regression. The new verification workflow is a reasonable smoke test but, as its own comment acknowledges, is intended to be deleted post-release. Verified against the worktree at d3d1796: the first setup-node call (line 70) inherits the composite default of 24.14.1, the second (line 130-132) pins it explicitly. Three maintainability suggestions; no blockers.

🟡 2 suggestion(s) | 💬 1 nitpick(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `.github/workflows/release.yml`:
- [SUGGESTION] lines 129-132: Two setup-node calls track the pinned version independently
  The first `Setup Node.JS` invocation in `release-npm` (line 70) passes no `node-version` and inherits the composite action's default (`24.14.1`, per `.github/actions/nodejs/action.yaml:13`). The second call now hardcodes `24.14.1` literally. They match today, but the two sites are coupled only by convention. When upstream ships a non-regressed Node (e.g. 24.16.0) and someone bumps the composite default, the first call will move and the second will silently stay behind — recreating the exact two-version-in-one-job mismatch this PR is fixing. Either pin both calls explicitly (so a future bump is a single intentional edit at one site) or drop the `node-version` input here entirely and let both calls inherit the composite default. The latter keeps the pin in exactly one place — the composite action — where the EBADF comment already lives.

In `.github/workflows/verify-release-node-pin.yml`:
- [SUGGESTION] lines 31-52: Verification workflow exercises only the fixed path, not the regression
  Both `Setup Node.JS` steps resolve to `24.14.1` — the first via composite default, the second via the explicit pin matching the fix. This confirms the fixed configuration installs cleanly but does not reproduce the failure mode being guarded against (the second setup-node resolving `24` to a newer minor with the EBADF/fstat regression). If a future change re-introduces a floating `node-version: 24` override here or in `release.yml`, this workflow still passes. A stronger guard would add a step (or matrix entry) that re-runs the second setup-node with `node-version: 24` and asserts the resolved `node --version` matches `v24.14.1`, failing loudly if the runner cache returns something newer. The file's own comment already flags it as disposable post-release, so this is purely about making the one-shot verification reproduce the actual failure mode while it exists.

In `.github/workflows/release.yml`:
- [NITPICK] lines 128-132: Inline comment explains the major version but not the patch pin
  The comment `# We need NodeJS 24 for https://docs.npmjs.com/trusted-publishers` justifies the 24.x requirement but not why a specific patch (`24.14.1`) is hardcoded here while the rest of the workflow relies on the composite default. A reader looking only at `release.yml` sees a very specific pin with no in-file reasoning, inviting a future 'cleanup' back to `24` or a forward bump to `24.15.x`. A one-line addition pointing to the composite action's EBADF note (or duplicating it) would prevent that regression.

Reviewed commit: d3d1796

Inline comments omitted because GitHub rejected the generated inline review payload; findings are included above.

The release-npm job set up Node twice in the same runner: once for the
build with the composite action default, then again right before
`npm publish` with `node-version: 24` to satisfy trusted-publishers
OIDC (npm 11.5.1+ ships only with Node 24+).

That second call dates back to #2923, when the composite action still
defaulted to Node 20. Since then the composite default has been bumped
to 24.14.1, so both calls install the same Node binary and the second
call only re-runs `yarn install --inline-builds`.

The re-run is also what broke the v3.1.0-dev.2 release: the previous
override `node-version: 24` widened the pin and let actions/setup-node
resolve to 24.15.0 from the runner cache. 24.15.0 ships a regression in
the ESM CJS translator that raises EBADF during native postinstall
scripts (bufferutil, utf-8-validate, unrs-resolver, @pshenmic/zeromq).

Dropping the second Setup Node.JS step removes the bug class entirely
and saves ~30s per release run. A short comment above the surviving
Setup Node.JS step records the Node 24+ requirement so future
maintainers do not lower the version. The companion verify workflow
introduced in the first commit on this branch is no longer needed and
has been removed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shumkov shumkov changed the title ci(release): pin trusted-publishers Node setup to 24.14.1 ci(release): drop redundant second Setup Node.JS in release-npm May 20, 2026
@shumkov
shumkov merged commit c30369e into v3.1-dev May 20, 2026
19 checks passed
@shumkov
shumkov deleted the ci/fix-release-node-pin branch May 20, 2026 17:47

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

CI simplification removes the redundant second Setup Node.JS step in release-npm and deletes the now-unneeded verify-release-node-pin.yml workflow. The remaining Setup step inherits the composite action's pinned Node default (24.14.1 in .github/actions/nodejs/action.yaml:13), which ships npm 11.5.1+ for trusted-publishers OIDC. The patch-pin rationale (ESM loader regression in 24.15.0) is documented inline in the composite action. All three prior findings are resolved by this delta and no new findings were identified.

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.

2 participants