Repository navigation
ci: install pinned npm 11 before every dependency install - #2729
Merged
Merged
Conversation
Node 22's bundled npm 10.x ignores each install's `allowScripts`, so the esbuild/fsevents denials from #2712 never applied in CI. Every job that installs dependencies (build, coverage, package, and the monthly dependency-refresh sweep) now installs an exact-pinned npm first, with --ignore-scripts, as `publish` already did for OIDC. main.yml states the version once as CI_NPM_VERSION, which `publish` now reads too. Also corrects comments and docs that said CI installs with npm 10. Closes #2715 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟢 Approval recommended
The targeted installs consistently use the exact npm pin before dependencies, and publishing safeguards remain intact.
No open issues 🎉
What changed in this PR
Pins CI to npm 11.20.0 so dependency installs enforce the existing allowScripts denials.
Changes:
- Shares one npm pin across build, coverage, package, and publish jobs.
- Adds the matching pin to the dependency-refresh workflow.
- Updates documentation and comments to distinguish policy checks from script blocking.
| File | Description |
|---|---|
scripts/verify-install-scripts.mjs |
Updates comments about CI’s npm version. |
scripts/install-clients.test.mjs |
Corrects the test’s npm-version commentary. |
docs/quality-gate.md |
Clarifies policy verification versus enforcement. |
.github/workflows/main.yml |
Installs the shared npm pin before dependency installs. |
.github/workflows/dependency-refresh.yml |
Adds the matching pinned npm installation. |
Review effort: Balanced
Member
Author
|
Copilot review loop closed: round 1 was clean. No inline comments, no headline finding, no suppressed comments ("No open issues"), so I'm stopping here without a confirming round. |
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
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.
Closes #2715
Problem
#2712 denies the
esbuildandfseventsinstall scripts throughallowScriptsin each v2 install. npm reads that field only from 11.19 onward. Node 22's bundled npm is 10.x, sonpm installin thebuild,coverageandpackagejobs still ran both scripts.verify:install-scriptsenforced the review decision in CI, but nothing actually blocked the scripts there.Change
main.ymlnow states the CI npm once, at workflow level:CI_NPM_VERSION: '11.20.0', the versionpublishalready pinned for OIDC (Release job runs install-time scripts and unpinned tooling while holding the npm OIDC publish token #2483).build,coverageandpackageeach runnpm install -g --ignore-scripts "npm@$CI_NPM_VERSION"right aftersetup-node, before any install.publishreads the same variable, so there is one place to bump it.dependency-refresh.ymlalso runs a scriptednpm install, so it gets the same step. It's a separate workflow and can't read the variable, so it pins the literal, and comments on both sides say to keep them equal.verify-install-scripts.mjsandinstall-clients.test.mjs, and theverify:install-scriptsrow indocs/quality-gate.md, said CI installs with npm 10. They now describe the current setup, and the docs row separates the decision (the guard, works on any npm) from the blocking (npm ≥ 11.19).Not changed:
sdk-watchanddependabot-alertsalready install withnpm ci --ignore-scripts, and thedcojobs install nothing.Answers to the issue's three checks
packagebuilds the tarball the credentialedpublishjob ships, so it follows the same reasoningpublishuses for its own pin.--ignore-scriptskeeps npm's own lifecycle scripts out.^20.17.0 || >=22.9.0, and CI runs22.x, so it fits. The comment keeps the existing instruction to stay on 11.x until npm 12's floor is checked.npx npm@11.20.0 install-scripts lsinclients/web, which reportsNo packages with unreviewed install scripts., so the pinned version reads the denials. Theinstall-clients"premise" test, which skipped on npm 10, will now run in CI. It already passes locally on 11.19.Verification
npm run local:gatepassed (EXIT=0) on this branch. The real proof is this PR's own CI run: thebuildandcoveragejobs install with the pinned npm.🤖 Generated with Claude Code