Skip to content

fix(arborist): honor gypfile:false on lockfile-driven installs #9859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lazerg wants to merge 2 commits into
base: latest
Choose a base branch
from
Open

fix(arborist): honor gypfile:false on lockfile-driven installs #9859

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfd31e1
fix(arborist): honor gypfile:false on lockfile-driven installs
lazerg Aug 8, 2026
e240df9
fix(arborist): only trust on-disk gypfile when the package matches
lazerg Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions workspaces/arborist/lib/arborist/rebuild.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const { promiseRetry } = require('@gar/promise-retry')
const { log, time } = require('proc-log')
const { resolve, delimiter } = require('node:path')
const { isScriptAllowed } = require('../script-allowed.js')
const { hasGypfileOptOut } = require('../gypfile.js')

const boolEnv = b => b ? '1' : ''
const sortNodes = (a, b) => (a.depth - b.depth) || localeCompare(a.path, b.path)
Expand Down Expand Up @@ -277,10 +278,10 @@ module.exports = cls => class Builder extends cls {
// Rebuild node-gyp dependencies lacking an install or preinstall script
// note that 'scripts' might be missing entirely, and the package may
// set gypfile:false to avoid this automatic detection.
const isGyp = gypfile !== false &&
!install &&
const isGyp = !install &&
!preinstall &&
await isNodeGypPackage(node.path)
await isNodeGypPackage(node.path) &&
!await hasGypfileOptOut(node.path, gypfile)

if (bin || preinstall || install || postinstall || prepare || isGyp) {
if (bin) {
Expand Down
19 changes: 19 additions & 0 deletions workspaces/arborist/lib/gypfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
const PackageJson = require('@npmcli/package-json')

// `gypfile: false` opts a package out of the synthetic `node-gyp rebuild`
// install script npm adds when it finds a `binding.gyp`.
//
// The flag is only on the tree node when the node came from a packument or
// from disk. Lockfile-derived nodes (`npm ci`, a repeat `npm install`) carry
// no `gypfile` field at all, so the opt-out is invisible there and has to be
// read back off the installed package.json.
const hasGypfileOptOut = async (path, gypfile) => {
if (gypfile !== undefined) {
return gypfile === false
}

const { content } = await PackageJson.load(path).catch(() => ({ content: {} }))
return content.gypfile === false
}

module.exports = { hasGypfileOptOut }
5 changes: 3 additions & 2 deletions workspaces/arborist/lib/install-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const { isNodeGypPackage } = require('@npmcli/node-gyp')
const PackageJson = require('@npmcli/package-json')
const { hasGypfileOptOut } = require('./gypfile.js')

// Returns the install-relevant lifecycle scripts that would run for a
// given arborist Node, or `{}` if there are none.
Expand Down Expand Up @@ -65,8 +66,8 @@ const getInstallScripts = async (node) => {
const hasExplicitGypGate = !!(collected.preinstall || collected.install)
if (
!hasExplicitGypGate &&
pkg.gypfile !== false &&
await isNodeGypPackage(node.path).catch(() => false)

@JamieMagee JamieMagee Aug 10, 2026

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.

getInstallScripts() also runs during the strict allow-scripts preflight, before reify replaces the existing node_modules entry. At that point, node.path can refer to a different version.

For example, suppose the installed version has binding.gyp and gypfile: false, while the locked version has binding.gyp without the opt-out. This reads false from the installed version, so strict preflight passes. Rebuild later sees the new package and synthesizes node-gyp rebuild.

Can we check that the package on disk matches the node's name and version before trusting this fallback?

@lazerg lazerg Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, that is real. The preflight walks the ideal tree, so node.path still points at whatever version is installed right now.

hasGypfileOptOut now takes the node's package object instead of only the gypfile value, and it trusts the disk read only when the package.json at that path carries the same name and version as the node. If either one differs, or the node has no name or version to compare against, the opt-out counts as absent and node-gyp rebuild is enumerated. That is what npm did before this fallback existed, so the preflight can no longer come out quieter than the later rebuild.

Added three cases in workspaces/arborist/test/install-scripts.js: a version mismatch on disk, a name mismatch, and a node with nothing to match on.

await isNodeGypPackage(node.path).catch(() => false) &&
!await hasGypfileOptOut(node.path, pkg.gypfile)
) {
collected.install = 'node-gyp rebuild'
}
Expand Down
46 changes: 46 additions & 0 deletions workspaces/arborist/test/arborist/rebuild.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,52 @@ t.test('do not rebuild node-gyp dependencies with gypfile:false', async t => {
await arb.rebuild()
})

// ref: https://github.com/npm/cli/issues/9837
t.test('do not rebuild node-gyp dependencies with gypfile:false from a lockfile', async t => {
const Arborist = t.mock('../../lib/arborist/index.js', {
'@npmcli/run-script': async () => {
throw new Error('should not run any scripts')
},
})
const path = t.testdir({
node_modules: {
dep: {
'package.json': JSON.stringify({
name: 'dep',
version: '1.0.0',
gypfile: false,
}),
'binding.gyp': '',
},
},
'package-lock.json': JSON.stringify({
name: 'project',
lockfileVersion: 3,
requires: true,
packages: {
'': {
name: 'project',
dependencies: {
dep: '1',
},
},
'node_modules/dep': {
version: '1.0.0',
},
},
}),
'package.json': JSON.stringify({
name: 'project',
dependencies: {
dep: '1',
},
}),
})
const arb = new Arborist({ path, dangerouslyAllowAllScripts: true })
const tree = await arb.loadVirtual()
await arb.rebuild({ nodes: [...tree.inventory.values()] })
})

// ref: https://github.com/npm/cli/issues/2905
t.test('do not run lifecycle scripts of linked deps twice', async t => {
const testdir = t.testdir({
Expand Down
23 changes: 23 additions & 0 deletions workspaces/arborist/test/install-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,29 @@ t.test('synthetic node-gyp suppressed when gypfile: false', async t => {
)
})

t.test('synthetic node-gyp suppressed by gypfile: false on disk', async t => {
const getInstallScripts = mockGetInstallScripts(t, () => true)
const path = t.testdir({
'package.json': JSON.stringify({
name: 'dep',
version: '1.0.0',
gypfile: false,
}),
})
t.strictSame(await getInstallScripts(node({ path })), {})
})

t.test('synthetic node-gyp still detected when disk has no gypfile', async t => {
const getInstallScripts = mockGetInstallScripts(t, () => true)
const path = t.testdir({
'package.json': JSON.stringify({ name: 'dep', version: '1.0.0' }),
})
t.strictSame(
await getInstallScripts(node({ path })),
{ install: 'node-gyp rebuild' }
)
})

t.test('synthetic node-gyp suppressed when explicit install is present', async t => {
const getInstallScripts = mockGetInstallScripts(t, () => true)
t.strictSame(
Expand Down