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

workspaces/arborist/lib/arborist/rebuild.js

@@ -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)
@@ -251,7 +252,7 @@ module.exports = cls => class Builder extends cls {
     }
 
     const { package: pkg, hasInstallScript } = node.target
-    const { gypfile, bin, scripts = {} } = pkg
+    const { bin, scripts = {} } = pkg
 
     const { preinstall, install, postinstall, prepare } = scripts
     const anyScript = preinstall || install || postinstall || prepare
@@ -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, pkg, node.target.name)
 
     if (bin || preinstall || install || postinstall || prepare || isGyp) {
       if (bin) {

workspaces/arborist/lib/gypfile.js

@@ -0,0 +1,39 @@
+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.
+//
+// The disk read is only trustworthy when the package.json at `path` is the
+// package the node describes. The strict allow-scripts preflight runs before
+// reify replaces `node_modules`, so `path` can still hold a different version
+// of the package, or a different package altogether. Trusting a stale
+// `gypfile: false` there would let the preflight pass while the later rebuild
+// (which sees the real package) synthesises `node-gyp rebuild` anyway. So the
+// name and the version on disk must match the node before the flag counts.
+// Without both, treat the opt-out as absent, which is what npm did before the
+// fallback existed.
+const hasGypfileOptOut = async (path, pkg, nodeName) => {
+  if (pkg.gypfile !== undefined) {
+    return pkg.gypfile === false
+  }
+
+  const name = pkg.name || nodeName
+  const { version } = pkg
+  if (!name || !version) {
+    return false
+  }
+
+  const { content } = await PackageJson.load(path).catch(() => ({ content: {} }))
+  if (content.name !== name || content.version !== version) {
+    return false
+  }
+
+  return content.gypfile === false
+}
+
+module.exports = { hasGypfileOptOut }

workspaces/arborist/lib/install-scripts.js

@@ -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.
@@ -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)
+    await isNodeGypPackage(node.path).catch(() => false) &&
+    !await hasGypfileOptOut(node.path, pkg, node.name)
   ) {
     collected.install = 'node-gyp rebuild'
   }

workspaces/arborist/test/arborist/rebuild.js

@@ -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({

workspaces/arborist/test/install-scripts.js

@@ -17,10 +17,18 @@ const node = ({
   gypfile,
   resolved = 'https://registry.npmjs.org/pkg/-/pkg-1.0.0.tgz',
   path = '/fake',
+  name = 'dep',
+  version = '1.0.0',
 } = {}) => ({
   resolved,
   path,
-  package: { scripts, ...(gypfile !== undefined ? { gypfile } : {}) },
+  name,
+  package: {
+    name,
+    version,
+    scripts,
+    ...(gypfile !== undefined ? { gypfile } : {}),
+  },
 })
 
 t.test('collects preinstall, install, postinstall', async t => {
@@ -95,6 +103,83 @@ 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('gypfile: false on disk ignored when the version does not match', async t => {
+  const getInstallScripts = mockGetInstallScripts(t, () => true)
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'dep',
+      version: '1.0.0',
+      gypfile: false,
+    }),
+  })
+  // The strict allow-scripts preflight runs before reify swaps node_modules,
+  // so the opt-out on disk belongs to the version that is already installed,
+  // not to the one the node describes.
+  t.strictSame(
+    await getInstallScripts(node({ path, version: '2.0.0' })),
+    { install: 'node-gyp rebuild' }
+  )
+})
+
+t.test('gypfile: false on disk ignored when the name does not match', async t => {
+  const getInstallScripts = mockGetInstallScripts(t, () => true)
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'other',
+      version: '1.0.0',
+      gypfile: false,
+    }),
+  })
+  t.strictSame(
+    await getInstallScripts(node({ path })),
+    { install: 'node-gyp rebuild' }
+  )
+})
+
+t.test('gypfile: false on disk ignored when the node has no identity', async t => {
+  const getInstallScripts = mockGetInstallScripts(t, () => true)
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'dep',
+      version: '1.0.0',
+      gypfile: false,
+    }),
+  })
+  // No version to compare against, so the disk read cannot be trusted.
+  t.strictSame(
+    await getInstallScripts(node({ path, version: '' })),
+    { install: 'node-gyp rebuild' }
+  )
+  // No name either.
+  t.strictSame(
+    await getInstallScripts(node({ path, name: '' })),
+    { install: 'node-gyp rebuild' }
+  )
+})
+
 t.test('synthetic node-gyp suppressed when explicit install is present', async t => {
   const getInstallScripts = mockGetInstallScripts(t, () => true)
   t.strictSame(