fix(arborist): run workspace lifecycle scripts once under linked strategy

workspaces/arborist/lib/arborist/isolated-reifier.js

@@ -139,6 +139,8 @@ module.exports = cls => class IsolatedReifier extends cls {
     result.localLocation = node.location
     result.localPath = node.path
     result.isWorkspace = true
+    // isWorkspace above marks every local proxy, file: deps included; this is only the project's workspaces.
+    result.isProjectWorkspace = node.isWorkspace
     result.resolved = node.resolved
     await this.#assignCommonProperties(node, result)
     return result
@@ -316,6 +318,7 @@ module.exports = cls => class IsolatedReifier extends cls {
         package: c.package,
         path: c.localPath,
         resolved: c.resolved,
+        isWorkspace: c.isProjectWorkspace,
       })
       root.fsChildren.add(workspace)
       root.inventory.set(workspace.location, workspace)
@@ -441,7 +444,8 @@ module.exports = cls => class IsolatedReifier extends cls {
       version: dep.package.version,
     }
     const link = new IsolatedLink({
-      isStoreLink: true,
+      // Links to local targets (workspaces, file: deps) must take the links build pass so their prepare scripts run.
+      isStoreLink: external,
       location: join(nmFolder, dep.name),
       name: toKey,
       optional,

workspaces/arborist/lib/arborist/rebuild.js

@@ -25,6 +25,7 @@ const _trashList = Symbol.for('trashList')
 
 module.exports = cls => class Builder extends cls {
   #doHandleOptionalFailure
+  #sharedLinks = new WeakMap()
   #oldMeta = null
   #queues = {
     preinstall: [],
@@ -194,6 +195,18 @@ module.exports = cls => class Builder extends cls {
     // there's no particular reason for doing it in this order rather
     // than another, but sorting *somehow* makes it consistent.
     const queue = [...set].sort(sortNodes)
+    // Links in this pass sharing a target run its scripts once, through a required link if any so a failure is not swallowed as optional.
+    const linksByTarget = new Map()
+    for (const node of queue.filter(n => n.isLink)) {
+      const links = linksByTarget.get(node.target) ?? []
+      linksByTarget.set(node.target, [...links, node])
+    }
+    const scriptLinks = new Set()
+    for (const links of linksByTarget.values()) {
+      const picked = links.find(l => !l.optional) ?? links[0]
+      scriptLinks.add(picked)
+      this.#sharedLinks.set(picked, links)
+    }
 
     for (const node of queue) {
       const { package: { bin, scripts = {} } } = node.target
@@ -211,13 +224,14 @@ module.exports = cls => class Builder extends cls {
       // For non-links node.target === node, so registry deps are unaffected.
       const scriptsAllowed =
         this.options.dangerouslyAllowAllScripts ||
-        node.isWorkspace ||
+        node.target.isWorkspace ||
         isScriptAllowed(node.target, this.options.allowScripts) === true
+      const runScripts = scriptsAllowed && (!node.isLink || scriptLinks.has(node))
       for (const [key, has] of Object.entries(tests)) {
         if (!has) {
           continue
         }
-        if (key !== 'bin' && !scriptsAllowed) {
+        if (key !== 'bin' && !runScripts) {
           continue
         }
         this.#queues[key].push(node)
@@ -366,8 +380,10 @@ module.exports = cls => class Builder extends cls {
         log.info('run', pkg._id, event, { code, signal })
       })
 
+      // A failure applies to every link that skipped the script in favor of this one.
+      const links = this.#sharedLinks.get(node) ?? [node]
       await (this.#doHandleOptionalFailure
-        ? this[_handleOptionalFailure](node, p)
+        ? Promise.all(links.map(link => this[_handleOptionalFailure](link, p)))
         : p)
 
       timeEndLocation()

workspaces/arborist/lib/isolated-classes.js

@@ -22,6 +22,7 @@ class IsolatedNode {
   inBundle = false
   isRegistryDependency = false
   isRootDependency = false
+  isWorkspace = false
   linksIn = new Set()
   meta = { loadedFromDisk: false }
   optional = false
@@ -58,6 +59,9 @@ class IsolatedNode {
     if (options.isRootDependency) {
       this.isRootDependency = true
     }
+    if (options.isWorkspace) {
+      this.isWorkspace = true
+    }
     if (options.optional) {
       this.optional = true
     }

workspaces/arborist/test/arborist/rebuild.js

@@ -242,6 +242,106 @@ t.test('workspaces bypass the allowScripts gate (owner-managed)', async t => {
   )
 })
 
+t.test('links sharing a target run its scripts once through a required link', async t => {
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      optionalDependencies: { 'a-opt': 'file:./shared' },
+      dependencies: { 'b-req': 'file:./shared', 'c-req': 'file:./shared' },
+    }),
+    node_modules: {
+      'a-opt': t.fixture('symlink', '../shared'),
+      'b-req': t.fixture('symlink', '../shared'),
+      'c-req': t.fixture('symlink', '../shared'),
+    },
+    shared: {
+      'package.json': JSON.stringify({
+        name: 'shared',
+        version: '1.0.0',
+        scripts: { postinstall: 'exit 1' },
+      }),
+    },
+  })
+  const runs = []
+  const Arborist = t.mock('../../lib/arborist/index.js', {
+    '@npmcli/run-script': async ({ event }) => {
+      runs.push(event)
+      throw Object.assign(new Error('script failed'), { code: 1 })
+    },
+  })
+  const arb = new Arborist({ path, dangerouslyAllowAllScripts: true })
+  const tree = await arb.loadActual()
+  const nodes = ['a-opt', 'b-req', 'c-req'].map(name => tree.children.get(name))
+  t.equal(nodes[0].optional, true, 'first sorted link is optional')
+  await t.rejects(arb.rebuild({ nodes, handleOptionalFailure: true }), { message: 'script failed' })
+  t.same(runs, ['postinstall'], 'script ran once, and its failure was not swallowed')
+})
+
+t.test('failed script of a target shared by optional links removes every link', async t => {
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      optionalDependencies: { 'a-opt': 'file:./shared', 'b-opt': 'file:./shared' },
+    }),
+    node_modules: {
+      'a-opt': t.fixture('symlink', '../shared'),
+      'b-opt': t.fixture('symlink', '../shared'),
+    },
+    shared: {
+      'package.json': JSON.stringify({
+        name: 'shared',
+        version: '1.0.0',
+        scripts: { postinstall: 'exit 1' },
+      }),
+    },
+  })
+  const runs = []
+  const Arborist = t.mock('../../lib/arborist/index.js', {
+    '@npmcli/run-script': async ({ event }) => {
+      runs.push(event)
+      throw Object.assign(new Error('script failed'), { code: 1 })
+    },
+  })
+  const arb = new Arborist({ path, dangerouslyAllowAllScripts: true })
+  const tree = await arb.loadActual()
+  const nodes = ['a-opt', 'b-opt'].map(name => tree.children.get(name))
+  await arb.rebuild({ nodes, handleOptionalFailure: true })
+  t.same(runs, ['postinstall'], 'script ran once')
+  t.ok(arb[_trashList].has(resolve(path, 'node_modules/a-opt')), 'link that ran the script is removed')
+  t.ok(arb[_trashList].has(resolve(path, 'node_modules/b-opt')), 'link that skipped the script is removed')
+})
+
+t.test('rebuilding shared links twice with one arborist does not throw', async t => {
+  const path = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      dependencies: { a: 'file:./shared', b: 'file:./shared' },
+    }),
+    node_modules: {
+      a: t.fixture('symlink', '../shared'),
+      b: t.fixture('symlink', '../shared'),
+    },
+    shared: {
+      'package.json': JSON.stringify({
+        name: 'shared',
+        version: '1.0.0',
+        scripts: { postinstall: 'exit 0' },
+      }),
+    },
+  })
+  const Arborist = t.mock('../../lib/arborist/index.js', {
+    '@npmcli/run-script': async () => ({ code: 0 }),
+  })
+  const arb = new Arborist({ path, dangerouslyAllowAllScripts: true })
+  const tree = await arb.loadActual()
+  const nodes = ['a', 'b'].map(name => tree.children.get(name))
+  await arb.rebuild({ nodes, handleOptionalFailure: true })
+  await t.resolves(arb.rebuild({ nodes, handleOptionalFailure: true }))
+})
+
 t.test('do nothing if ignoreScripts=true and binLinks=false', async t => {
   const path = fixture(t, 'testing-rebuild-bundle-reified')
   const file = resolve(path, 'node_modules/@isaacs/testing-rebuild-bundle-a/node_modules/@isaacs/testing-rebuild-bundle-b/cwd')

workspaces/arborist/test/isolated-mode.js

@@ -1473,6 +1473,36 @@ tap.test('postinstall scripts run once for store packages', async t => {
   t.equal(count, 1, 'postinstall ran exactly once')
 })
 
+tap.test('workspace lifecycle scripts run once without allowScripts approval', async t => {
+  // Workspaces are exempt from allowScripts; a workspace linked into several dependents still runs each script once.
+  const log = event => `node -e "fs.appendFileSync('runs.log', '${event}\\n')"`
+  const scripts = Object.fromEntries(['preinstall', 'install', 'postinstall', 'prepare'].map(e => [e, log(e)]))
+  const graph = {
+    registry: [
+      { name: 'which', version: '1.0.0', scripts: { postinstall: log('postinstall') } },
+    ],
+    root: {
+      name: 'foo', version: '1.2.3', dependencies: { bar: '*', which: '1.0.0' },
+    },
+    workspaces: [
+      { name: 'bar', version: '1.0.0', scripts },
+      { name: 'baz', version: '1.0.0', dependencies: { bar: '*' }, scripts: { postinstall: log('postinstall') } },
+      { name: 'qux', version: '1.0.0', dependencies: { bar: '*' } },
+    ],
+  }
+
+  const { dir, registry } = await getRepo(graph)
+
+  const cache = fs.mkdtempSync(`${getTempDir()}/test-`)
+  const arborist = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
+  await arborist.reify({ installStrategy: 'linked' })
+
+  const runs = ws => fs.readFileSync(path.join(dir, 'packages', ws, 'runs.log'), 'utf8')
+  t.equal(runs('bar'), 'preinstall\nprepare\ninstall\npostinstall\n', 'declared workspace with dependents runs each script once')
+  t.equal(runs('baz'), 'postinstall\n', 'undeclared workspace runs its script')
+  t.notOk(pathExists(`${setupRequire(dir)('which')}/runs.log`), 'unapproved registry dependency script is blocked')
+})
+
 tap.test('workspace-filtered install with linked strategy', async t => {
   // Two workspaces sharing the same dependency must not crash when installing with --workspace + --install-strategy=linked.
   const graph = {