fix(lifecycle): skip verify for install hooks (#8957)

close #8954

---------

Co-authored-by: Zoltan Kochan <z@kochan.io>

.changeset/old-bees-study.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/config": patch
+"@pnpm/plugin-commands-script-runners": patch
+"pnpm": patch
+---
+
+Fix infinite loop caused by lifecycle scripts using `pnpm` to execute other scripts during `pnpm install` with `verify-deps-before-run=install` [#8954](https://github.com/pnpm/pnpm/issues/8954).

config/config/src/index.ts

@@ -361,6 +361,9 @@ export async function getConfig (opts: {
     pnpmConfig.extraBinPaths = []
   }
 
+  pnpmConfig.extraEnv = {
+    npm_config_verify_deps_before_run: 'false',
+  }
   if (pnpmConfig.preferSymlinkedExecutables && !isWindows()) {
     const cwd = pnpmConfig.lockfileDir ?? pnpmConfig.dir
 
@@ -370,9 +373,7 @@ export async function getConfig (opts: {
         ? path.join(pnpmConfig.modulesDir, '.pnpm')
         : 'node_modules/.pnpm'
 
-    pnpmConfig.extraEnv = {
-      NODE_PATH: pathAbsolute(path.join(virtualStoreDir, 'node_modules'), cwd),
-    }
+    pnpmConfig.extraEnv['NODE_PATH'] = pathAbsolute(path.join(virtualStoreDir, 'node_modules'), cwd)
   }
 
   if (pnpmConfig.shamefullyFlatten) {

exec/plugin-commands-script-runners/src/exec.ts

@@ -27,7 +27,6 @@ import { PnpmError } from '@pnpm/error'
 import which from 'which'
 import writeJsonFile from 'write-json-file'
 import { getNearestProgram, getNearestScript } from './buildCommandNotFoundHint'
-import { DISABLE_DEPS_CHECK_ENV, SKIP_ENV_KEY } from './shouldRunCheck'
 import { runDepsStatusCheck } from './runDepsStatusCheck'
 
 export const shorthands: Record<string, string | string[]> = {
@@ -177,7 +176,7 @@ export async function handler (
   }
   const limitRun = pLimit(opts.workspaceConcurrency ?? 4)
 
-  if (opts.verifyDepsBeforeRun && !process.env[SKIP_ENV_KEY]) {
+  if (opts.verifyDepsBeforeRun) {
     await runDepsStatusCheck(opts)
   }
 
@@ -253,7 +252,6 @@ export async function handler (
               ...extraEnv,
               PNPM_PACKAGE_NAME: opts.selectedProjectsGraph[prefix]?.package.manifest.name,
               ...(opts.nodeOptions ? { NODE_OPTIONS: opts.nodeOptions } : {}),
-              ...opts.verifyDepsBeforeRun ? DISABLE_DEPS_CHECK_ENV : undefined,
             },
             prependPaths,
             userAgent: opts.userAgent,

exec/plugin-commands-script-runners/src/run.ts

@@ -24,7 +24,6 @@ import { runRecursive, type RecursiveRunOpts, getSpecifiedScripts as getSpecifie
 import { existsInDir } from './existsInDir'
 import { handler as exec } from './exec'
 import { buildCommandNotFoundHint } from './buildCommandNotFoundHint'
-import { DISABLE_DEPS_CHECK_ENV, shouldRunCheck } from './shouldRunCheck'
 import { runDepsStatusCheck } from './runDepsStatusCheck'
 
 export const IF_PRESENT_OPTION: Record<string, unknown> = {
@@ -197,20 +196,12 @@ export async function handler (
   }
   const [scriptName, ...passedThruArgs] = params
 
-  // verifyDepsBeforeRun is outside of shouldRunCheck because TypeScript's tagged union
-  // only works when the tag is directly placed in the condition.
-  if (opts.verifyDepsBeforeRun && shouldRunCheck(process.env, scriptName)) {
+  if (opts.verifyDepsBeforeRun) {
     await runDepsStatusCheck(opts)
   }
 
   if (opts.recursive) {
     if (scriptName || Object.keys(opts.selectedProjectsGraph).length > 1) {
-      if (opts.verifyDepsBeforeRun) {
-        opts.extraEnv = {
-          ...opts.extraEnv,
-          ...DISABLE_DEPS_CHECK_ENV,
-        }
-      }
       return runRecursive(params, opts) as Promise<undefined>
     }
     dir = Object.keys(opts.selectedProjectsGraph)[0]
@@ -268,7 +259,6 @@ so you may run "pnpm -w run ${scriptName}"`,
   const extraEnv = {
     ...opts.extraEnv,
     ...(opts.nodeOptions ? { NODE_OPTIONS: opts.nodeOptions } : {}),
-    ...opts.verifyDepsBeforeRun ? DISABLE_DEPS_CHECK_ENV : undefined,
   }
 
   const lifecycleOpts: RunLifecycleHookOptions = {

exec/plugin-commands-script-runners/src/shouldRunCheck.ts

@@ -1,22 +0,0 @@
-// The scripts that `pnpm run` executes are likely to also execute other `pnpm run`.
-// We don't want this potentially expensive check to repeat.
-// The solution is to use an env key to disable the check.
-export const SKIP_ENV_KEY = 'pnpm_run_skip_deps_check'
-export const DISABLE_DEPS_CHECK_ENV = {
-  [SKIP_ENV_KEY]: 'true',
-} as const satisfies Env
-
-export interface Env extends NodeJS.ProcessEnv {
-  [SKIP_ENV_KEY]?: string
-}
-
-const SCRIPTS_TO_SKIP = [
-  'preinstall',
-  'install',
-  'postinstall',
-  'preuninstall',
-  'uninstall',
-  'postuninstall',
-]
-
-export const shouldRunCheck = (env: Env, scriptName: string): boolean => !env[SKIP_ENV_KEY] && !SCRIPTS_TO_SKIP.includes(scriptName)

exec/plugin-commands-script-runners/test/shouldRunCheck.test.ts

@@ -1,20 +0,0 @@
-import { DISABLE_DEPS_CHECK_ENV, shouldRunCheck } from '../src/shouldRunCheck'
-
-test('should return true if skip env is not defined and script name is not special', () => {
-  expect(shouldRunCheck({}, 'start')).toBe(true)
-})
-
-test('should return false if skip env is defined', () => {
-  expect(shouldRunCheck({ ...DISABLE_DEPS_CHECK_ENV }, 'start')).toBe(false)
-})
-
-test.each([
-  'preinstall',
-  'install',
-  'postinstall',
-  'preuninstall',
-  'uninstall',
-  'postuninstall',
-])('should return false if script name is %p', scriptName => {
-  expect(shouldRunCheck({}, scriptName)).toBe(false)
-})

exec/plugin-commands-script-runners/test/verifyDepsBeforeRun.ts

@@ -19,8 +19,6 @@ jest.mock('enquirer', () => ({
   prompt: jest.fn(),
 }))
 
-delete process.env['pnpm_run_skip_deps_check']
-
 const rootProjectManifest = {
   name: 'root',
   private: true,

pnpm/test/install/lifecycleScripts.ts

@@ -6,7 +6,7 @@ import { sync as rimraf } from '@zkochan/rimraf'
 import PATH from 'path-name'
 import loadJsonFile from 'load-json-file'
 import writeYamlFile from 'write-yaml-file'
-import { execPnpm, execPnpmSync } from '../utils'
+import { execPnpm, execPnpmSync, pnpmBinLocation } from '../utils'
 import { getIntegrity } from '@pnpm/registry-mock'
 
 const pkgRoot = path.join(__dirname, '..', '..')
@@ -263,3 +263,24 @@ test('ignores pnpm.executionEnv specified by dependencies', async () => {
     versions: process.versions,
   })
 })
+
+test('preinstall script does not trigger verify-deps-before-run (#8954)', async () => {
+  const pnpm = `${process.execPath} ${pnpmBinLocation}` // this would fail if either paths happen to contain spaces
+
+  prepare({
+    name: 'preinstall-script-does-not-trigger-verify-deps-before-run',
+    version: '1.0.0',
+    private: true,
+    scripts: {
+      sayHello: 'echo hello world',
+      preinstall: `${pnpm} run sayHello`,
+    },
+    dependencies: {
+      cowsay: '1.5.0', // to make the default state outdated, any dependency will do
+    },
+  })
+
+  const output = execPnpmSync(['--config.verify-deps-before-run=error', 'install'], { expectSuccess: true })
+  expect(output.status).toBe(0)
+  expect(output.stdout.toString()).toContain('hello world')
+})

pnpm/test/verifyDepsBeforeRun/exec.ts

@@ -91,8 +91,8 @@ test('single package workspace', async () => {
     expect(stdout.toString()).toContain('hello from exec')
   }
 
-  // should set env.pnpm_run_skip_deps_check for the script
-  await execPnpm([...CONFIG, 'exec', 'node', '--eval', 'assert.strictEqual(process.env.pnpm_run_skip_deps_check, "true")'])
+  // should set env.npm_config_verify_deps_before_run to false for the script (to skip check for nested script)
+  await execPnpm([...CONFIG, 'exec', 'node', '--eval', 'assert.strictEqual(process.env.npm_config_verify_deps_before_run, "false")'])
 })
 
 test('multi-project workspace', async () => {
@@ -339,6 +339,6 @@ test('multi-project workspace', async () => {
     expect(stdout.toString()).toContain(`hello from exec: ${process.cwd()}`)
   }
 
-  // should set env.pnpm_run_skip_deps_check for all the scripts
-  await execPnpm([...CONFIG, 'exec', 'node', '--eval', 'assert.strictEqual(process.env.pnpm_run_skip_deps_check, "true")'])
+  // should set env.npm_config_verify_deps_before_run to false for all the scripts (to skip check for nested script)
+  await execPnpm([...CONFIG, 'exec', 'node', '--eval', 'assert.strictEqual(process.env.npm_config_verify_deps_before_run, "false")'])
 })

pnpm/test/verifyDepsBeforeRun/multiProjectWorkspace.ts

@@ -9,7 +9,7 @@ import { execPnpm, execPnpmSync, pnpmBinLocation } from '../utils'
 const CONFIG = ['--config.verify-deps-before-run=error'] as const
 
 test('single dependency', async () => {
-  const checkEnv = 'node --eval "assert.strictEqual(process.env.pnpm_run_skip_deps_check, \'true\')"'
+  const checkEnv = 'node --eval "assert.strictEqual(process.env.npm_config_verify_deps_before_run, \'false\')"'
 
   const manifests: Record<string, ProjectManifest> = {
     root: {
@@ -278,7 +278,7 @@ test('single dependency', async () => {
     expect(stdout.toString()).toContain('hello from root')
   }
 
-  // should set env.pnpm_run_skip_deps_check for all the scripts
+  // should set env.npm_config_verify_deps_before_run to false for all the scripts (to skip check in nested scripts)
   await execPnpm([...CONFIG, '--recursive', 'run', 'checkEnv'])
 })
 
@@ -738,11 +738,12 @@ test('nested `pnpm run` should not check for mutated manifest', async () => {
   // add to every manifest file a script named `start` which would inherit `config` and invoke `nestedScript`
   for (const name in projects) {
     manifests[name].scripts!.start =
-      `node mutate-manifest.js && node ${pnpmBinLocation} ${CONFIG.join(' ')} run nestedScript`
+      `node mutate-manifest.js && node ${pnpmBinLocation} run nestedScript`
     projects[name].writePackageJson(manifests[name])
   }
 
   writeYamlFile('pnpm-workspace.yaml', { packages: ['**', '!store/**'] })
+  writeYamlFile('.npmrc', 'verify-deps-before-run=error')
 
   // attempting to execute a script without installing dependencies should fail
   {

pnpm/test/verifyDepsBeforeRun/singleProjectWorkspace.ts

@@ -16,7 +16,7 @@ test('single dependency', async () => {
     },
     scripts: {
       start: 'echo hello from script',
-      checkEnv: 'node --eval "assert.strictEqual(process.env.pnpm_run_skip_deps_check, \'true\')"',
+      checkEnv: 'node --eval "assert.strictEqual(process.env.npm_config_verify_deps_before_run, \'false\')"',
     },
   }
 
@@ -98,7 +98,7 @@ test('single dependency', async () => {
     expect(stdout.toString()).toContain('hello from script')
   }
 
-  // should set env.pnpm_run_skip_deps_check for the script
+  // should set env.npm_config_verify_deps_before_run to false for the script (to skip check in nested script)
   await execPnpm([...CONFIG, 'run', 'checkEnv'])
 })
 
@@ -198,16 +198,13 @@ test('nested `pnpm run` should not check for mutated manifest', async () => {
     fs.writeFileSync(require.resolve('./package.json'), jsonText)
     console.log('manifest mutated')
   `)
+  fs.writeFileSync('.npmrc', 'verify-deps-before-run=error', 'utf8')
 
   const cacheDir = path.resolve('cache')
-  const config = [
-    CONFIG,
-    `--config.cache-dir=${cacheDir}`,
-  ]
 
   // add a script named `start` which would inherit `config` and invoke `nestedScript`
   manifest.scripts!.start =
-    `node mutate-manifest.js && node ${pnpmBinLocation} ${config.join(' ')} run nestedScript`
+    `node mutate-manifest.js && node ${pnpmBinLocation} --config.cache-dir=${cacheDir} run nestedScript`
   project.writePackageJson(manifest)
 
   // attempting to execute a script without installing dependencies should fail