fix: report blocked install scripts when reify or a root script fails

lib/commands/ci.js

@@ -1,6 +1,7 @@
 const reifyFinish = require('../utils/reify-finish.js')
 const resolveAllowScripts = require('../utils/resolve-allow-scripts.js')
 const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js')
+const { warnUnreviewedScripts } = require('../utils/unreviewed-scripts-message.js')
 const runScript = require('@npmcli/run-script')
 const fs = require('node:fs/promises')
 const path = require('node:path')
@@ -146,20 +147,26 @@ class CI extends ArboristWorkspaceCmd {
       await runRootScript('preinstall')
     }
 
-    await arb.reify(opts)
+    try {
+      await arb.reify(opts)
 
-    if (!ignoreScripts) {
-      const postReifyScripts = [
-        'install',
-        'postinstall',
-        'prepublish', // XXX should we remove this finally??
-        'preprepare',
-        'prepare',
-        'postprepare',
-      ]
-      for (const event of postReifyScripts) {
-        await runRootScript(event)
+      if (!ignoreScripts) {
+        const postReifyScripts = [
+          'install',
+          'postinstall',
+          'prepublish', // XXX should we remove this finally??
+          'preprepare',
+          'prepare',
+          'postprepare',
+        ]
+        for (const event of postReifyScripts) {
+          await runRootScript(event)
+        }
       }
+    } catch (err) {
+      // Both a failed reify and a failed root script exit before reifyFinish would have reported the blocked scripts, and a rolled back reify leaves nothing in actualTree, so report against the tree npm meant to build.
+      await warnUnreviewedScripts(this.npm, arb, arb.idealTree)
+      throw err
     }
     await reifyFinish(this.npm, arb)
   }

lib/commands/install.js

@@ -7,6 +7,7 @@ const checks = require('npm-install-checks')
 const reifyFinish = require('../utils/reify-finish.js')
 const resolveAllowScripts = require('../utils/resolve-allow-scripts.js')
 const strictAllowScriptsPreflight = require('../utils/strict-allow-scripts-preflight.js')
+const { warnUnreviewedScripts } = require('../utils/unreviewed-scripts-message.js')
 const { patchRelaxOpts } = require('../utils/cli-only-flag.js')
 const ArboristWorkspaceCmd = require('../arborist-cmd.js')
 
@@ -173,20 +174,26 @@ class Install extends ArboristWorkspaceCmd {
 
     const arb = new Arborist(opts)
     await strictAllowScriptsPreflight({ arb, npm: this.npm, idealTreeOpts: opts })
-    await arb.reify(opts)
-
-    if (runRootLifecycle) {
-      const postReifyScripts = [
-        'install',
-        'postinstall',
-        'prepublish', // XXX(npm9) should we remove this finally??
-        'preprepare',
-        'prepare',
-        'postprepare',
-      ]
-      for (const event of postReifyScripts) {
-        await runRootScript(event)
+    try {
+      await arb.reify(opts)
+
+      if (runRootLifecycle) {
+        const postReifyScripts = [
+          'install',
+          'postinstall',
+          'prepublish', // XXX(npm9) should we remove this finally??
+          'preprepare',
+          'prepare',
+          'postprepare',
+        ]
+        for (const event of postReifyScripts) {
+          await runRootScript(event)
+        }
       }
+    } catch (err) {
+      // Both a failed reify and a failed root script exit before reifyFinish would have reported the blocked scripts, and a rolled back reify leaves nothing in actualTree, so report against the tree npm meant to build.
+      await warnUnreviewedScripts(this.npm, arb, arb.idealTree)
+      throw err
     }
     await reifyFinish(this.npm, arb)
   }

lib/utils/reify-output.js

@@ -16,7 +16,7 @@ const npmAuditReport = require('npm-audit-report')
 const { readTree: getFundingInfo } = require('libnpmfund')
 const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
 const auditError = require('./audit-error.js')
-const { configSetAllowScripts } = require('./allow-scripts-remediation.js')
+const { unreviewedScriptsMessage } = require('./unreviewed-scripts-message.js')
 
 const reifyOutput = (npm, arb, extras = {}) => {
   const { diff, actualTree } = arb
@@ -233,61 +233,4 @@ const packagesFundingMessage = (npm, { funding }) => {
   output.standard('  run `npm fund` for details')
 }
 
-const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
-  if (!unreviewedScripts.length) {
-    return
-  }
-
-  // Goes through log.warn so it respects --loglevel / --silent and lands
-  // on stderr like every other "FYI, here's something to know" message.
-  // stdout is reserved for things the user explicitly asked to see
-  // (npm ls, npm view).
-  const count = unreviewedScripts.length
-  const pkg = count === 1 ? 'package had' : 'packages had'
-  const header =
-    `${count} ${pkg} install scripts blocked because they are not covered by allowScripts:`
-
-  const names = []
-  const lines = unreviewedScripts.map(({ node, scripts }) => {
-    const { name, version } = trustedDisplay(node)
-    /* istanbul ignore next: every test node has a name */
-    const display = name || '<unknown>'
-    names.push(display)
-    const ver = version ? `@${version}` : ''
-    const events = Object.entries(scripts)
-      .map(([event, cmd]) => `${event}: ${cmd}`)
-      .join('; ')
-    return `  ${display}${ver} (${events})`
-  })
-
-  log.warn(
-    'install-scripts',
-    [
-      header,
-      ...lines,
-      '',
-      ...remediationLines(npm, names),
-    ].join('\n')
-  )
-}
-
-// `npm install-scripts` writes to a project package.json, which doesn't
-// exist for global installs (it throws EGLOBAL). For those, point users at
-// the mechanism that does work globally: the `--allow-scripts` flag for a
-// one-off, or `npm config set allow-scripts` to persist it.
-const remediationLines = (npm, names) => {
-  if (npm.global) {
-    const list = names.join(',')
-    return [
-      `Run \`npm install -g --allow-scripts=${list}\` to allow these scripts ` +
-      `once, or \`${configSetAllowScripts(names)}\` to allow them for ` +
-      'all global installs.',
-    ]
-  }
-  return [
-    'Run `npm install-scripts ls` to review, ' +
-    'or `npm install-scripts approve <pkg>` to allow.',
-  ]
-}
-
 module.exports = reifyOutput

lib/utils/unreviewed-scripts-message.js

@@ -0,0 +1,71 @@
+const { log } = require('proc-log')
+const { trustedDisplay } = require('@npmcli/arborist/lib/script-allowed.js')
+const checkAllowScripts = require('./check-allow-scripts.js')
+const { configSetAllowScripts } = require('./allow-scripts-remediation.js')
+
+// `unreviewedScriptsMessage` renders the install-time warning listing the deps
+// whose scripts were blocked by the allowScripts policy.
+// `warnUnreviewedScripts` is the check-and-warn variant, for callers that fail
+// before `reifyFinish` gets the chance to report them.
+
+const unreviewedScriptsMessage = (npm, unreviewedScripts) => {
+  if (!unreviewedScripts.length) {
+    return
+  }
+
+  // Goes through log.warn so it respects --loglevel / --silent and lands
+  // on stderr like every other "FYI, here's something to know" message.
+  // stdout is reserved for things the user explicitly asked to see
+  // (npm ls, npm view).
+  const count = unreviewedScripts.length
+  const pkg = count === 1 ? 'package had' : 'packages had'
+  const header =
+    `${count} ${pkg} install scripts blocked because they are not covered by allowScripts:`
+
+  const names = []
+  const lines = unreviewedScripts.map(({ node, scripts }) => {
+    const { name, version } = trustedDisplay(node)
+    /* istanbul ignore next: every test node has a name */
+    const display = name || '<unknown>'
+    names.push(display)
+    const ver = version ? `@${version}` : ''
+    const events = Object.entries(scripts)
+      .map(([event, cmd]) => `${event}: ${cmd}`)
+      .join('; ')
+    return `  ${display}${ver} (${events})`
+  })
+
+  log.warn(
+    'install-scripts',
+    [
+      header,
+      ...lines,
+      '',
+      ...remediationLines(npm, names),
+    ].join('\n')
+  )
+}
+
+// `npm install-scripts` writes to a project package.json, which doesn't
+// exist for global installs (it throws EGLOBAL). For those, point users at
+// the mechanism that does work globally: the `--allow-scripts` flag for a
+// one-off, or `npm config set allow-scripts` to persist it.
+const remediationLines = (npm, names) => {
+  if (npm.global) {
+    const list = names.join(',')
+    return [
+      `Run \`npm install -g --allow-scripts=${list}\` to allow these scripts ` +
+      `once, or \`${configSetAllowScripts(names)}\` to allow them for ` +
+      'all global installs.',
+    ]
+  }
+  return [
+    'Run `npm install-scripts ls` to review, ' +
+    'or `npm install-scripts approve <pkg>` to allow.',
+  ]
+}
+
+const warnUnreviewedScripts = async (npm, arb, tree) =>
+  unreviewedScriptsMessage(npm, await checkAllowScripts({ arb, npm, tree }))
+
+module.exports = { unreviewedScriptsMessage, warnUnreviewedScripts }

test/lib/commands/ci.js

@@ -357,6 +357,61 @@ t.test('a failing preinstall prevents reify for npm ci', async t => {
   )
 })
 
+// Regression test: symmetric to the install-side guarantee — the blocked-scripts warning is printed by reifyFinish, which a failing root post-reify script would otherwise skip past.
+t.test('a failing root prepare still reports blocked install scripts for npm ci', async t => {
+  const { npm, registry, logs } = await loadMockNpm(t, {
+    config: { audit: false },
+    prefixDir: {
+      abbrev: {
+        ...abbrev,
+        'package.json': JSON.stringify({
+          name: 'abbrev',
+          version: '1.0.0',
+          scripts: { postinstall: 'echo postinstall' },
+        }),
+      },
+      'package.json': JSON.stringify({
+        ...packageJson,
+        scripts: {
+          prepare: 'exit 1',
+        },
+      }),
+      'package-lock.json': JSON.stringify({
+        ...packageLock,
+        packages: {
+          ...packageLock.packages,
+          'node_modules/abbrev': {
+            ...packageLock.packages['node_modules/abbrev'],
+            hasInstallScript: true,
+          },
+        },
+      }),
+    },
+    mocks: {
+      '@npmcli/run-script': async (opts) => {
+        if (opts.path === npm.prefix && opts.event === 'prepare') {
+          throw Object.assign(new Error('prepare failed'), { code: 'ELIFECYCLE' })
+        }
+      },
+    },
+  })
+  const manifest = registry.manifest({
+    name: 'abbrev',
+    packuments: [{ version: '1.0.0', scripts: { postinstall: 'echo postinstall' } }],
+  })
+  await registry.tarball({
+    manifest: manifest.versions['1.0.0'],
+    tarball: path.join(npm.prefix, 'abbrev'),
+  })
+
+  await t.rejects(npm.exec('ci', []), /prepare failed/, 'ci rejects when prepare fails')
+  t.match(
+    logs.warn.byTitle('install-scripts').join('\n'),
+    /1 package had install scripts blocked/,
+    'blocked install scripts are reported before the failure'
+  )
+})
+
 t.test('should throw if package-lock.json is missing', async t => {
   const { npm } = await loadMockNpm(t, {
     prefixDir: {

test/lib/commands/install.js

@@ -213,6 +213,79 @@ t.test('exec commands', async t => {
     )
   })
 
+  // Regression test: the blocked-scripts warning is printed by reifyFinish, which a failing root post-reify script would otherwise skip past, hiding the very thing that most likely broke the build.
+  await t.test('a failing root prepare still reports blocked install scripts', async t => {
+    const { npm, registry, logs } = await loadMockNpm(t, {
+      config: { audit: false },
+      prefixDir: {
+        'package.json': JSON.stringify({
+          ...packageJson,
+          scripts: {
+            prepare: 'exit 1',
+          },
+        }),
+        abbrev: {
+          ...abbrev,
+          'package.json': JSON.stringify({
+            name: 'abbrev',
+            version: '1.0.0',
+            scripts: { postinstall: 'echo postinstall' },
+          }),
+        },
+      },
+      mocks: {
+        '@npmcli/run-script': async (opts) => {
+          if (opts.path === npm.prefix && opts.event === 'prepare') {
+            throw Object.assign(new Error('prepare failed'), { code: 'ELIFECYCLE' })
+          }
+        },
+      },
+    })
+    const manifest = registry.manifest({
+      name: 'abbrev',
+      packuments: [{ version: '1.0.0', scripts: { postinstall: 'echo postinstall' } }],
+    })
+    await registry.package({ manifest })
+    await registry.tarball({
+      manifest: manifest.versions['1.0.0'],
+      tarball: path.join(npm.prefix, 'abbrev'),
+    })
+
+    await t.rejects(npm.exec('install'), /prepare failed/, 'install rejects when prepare fails')
+    t.match(
+      logs.warn.byTitle('install-scripts').join('\n'),
+      /1 package had install scripts blocked/,
+      'blocked install scripts are reported before the failure'
+    )
+  })
+
+  // Regression test: a reify that throws rolls its tree back, so the blocked scripts have to be read off the ideal tree to still be reported.
+  await t.test('a failing reify still reports blocked install scripts', async t => {
+    const { npm, registry, logs } = await loadMockNpm(t, {
+      config: { audit: false },
+      prefixDir: {
+        'package.json': JSON.stringify(packageJson),
+        abbrev,
+      },
+      mocks: {
+        '@npmcli/run-script': async () => {},
+      },
+    })
+    const manifest = registry.manifest({
+      name: 'abbrev',
+      packuments: [{ version: '1.0.0', scripts: { postinstall: 'echo postinstall' } }],
+    })
+    await registry.package({ manifest })
+    registry.nock.get('/abbrev/-/abbrev-1.0.0.tgz').reply(404)
+
+    await t.rejects(npm.exec('install'), 'install rejects when a dependency cannot be fetched')
+    t.match(
+      logs.warn.byTitle('install-scripts').join('\n'),
+      /1 package had install scripts blocked/,
+      'blocked install scripts are reported before the failure'
+    )
+  })
+
   await t.test('should ignore scripts with --ignore-scripts', async t => {
     const { npm, registry } = await loadMockNpm(t, {
       config: {