diff --git a/lib/utils/reify-finish.js b/lib/utils/reify-finish.js index 1041c53fdb935..2e846c36bb068 100644 --- a/lib/utils/reify-finish.js +++ b/lib/utils/reify-finish.js @@ -5,6 +5,34 @@ const ini = require('ini') const { writeFile } = require('node:fs/promises') const { resolve } = require('node:path') +// Collect the set of node locations touched (added or changed) by this reify +// run, using arb.diff. Pre-existing untouched packages don't run their install +// scripts this run, so we shouldn't flag them as "blocked" (npm/cli#9797). +const collectTouchedLocations = (diff) => { + const touched = new Set() + if (!diff) { + return null + } + const stack = [diff] + while (stack.length) { + const d = stack.pop() + if (d.action === 'ADD' || d.action === 'CHANGE') { + const location = d.ideal?.location + if (location != null) { + touched.add(location) + } + } + if (d.children?.length) { + for (const child of d.children) { + if (child) { + stack.push(child) + } + } + } + } + return touched +} + const reifyFinish = async (npm, arb) => { // if we are using a builtin config, and just installed npm as a top-level global package, we have to preserve that config. if (arb.options.global) { @@ -18,7 +46,13 @@ const reifyFinish = async (npm, arb) => { } } warnWorkspaceAllowScripts(arb.actualTree) - const unreviewedScripts = await checkAllowScripts({ arb, npm }) + const allUnreviewed = await checkAllowScripts({ arb, npm }) + // Only warn about install scripts on packages this reify actually touched; + // untouched pre-existing packages don't run scripts here (npm/cli#9797). + const touched = collectTouchedLocations(arb.diff) + const unreviewedScripts = touched + ? allUnreviewed.filter(({ node }) => touched.has(node.location)) + : allUnreviewed reifyOutput(npm, arb, { unreviewedScripts }) } diff --git a/test/lib/utils/reify-finish.js b/test/lib/utils/reify-finish.js index a1dd165034a46..70cf84ede7741 100644 --- a/test/lib/utils/reify-finish.js +++ b/test/lib/utils/reify-finish.js @@ -11,7 +11,14 @@ const readRc = async (dir) => { return cleanNewlines(res).trim() } -const mockReififyFinish = async (t, { actualTree = {}, otherDirs = {}, ...config }) => { +const mockReififyFinish = async (t, { + actualTree = {}, + otherDirs = {}, + diff, + unreviewedNodes, + captureReifyOutput, + ...config +} = {}) => { const mock = await mockNpm(t, { npm: ({ other }) => ({ npmRoot: other, @@ -23,12 +30,18 @@ const mockReififyFinish = async (t, { actualTree = {}, otherDirs = {}, ...config config, }) - const reifyFinish = tmock(t, '{LIB}/utils/reify-finish.js', { - '{LIB}/utils/reify-output.js': () => {}, - }) + const mocks = { + '{LIB}/utils/reify-output.js': captureReifyOutput || (() => {}), + } + if (unreviewedNodes !== undefined) { + mocks['{LIB}/utils/check-allow-scripts.js'] = async () => + unreviewedNodes.map((node) => ({ node, scripts: { install: 'x' } })) + } + const reifyFinish = tmock(t, '{LIB}/utils/reify-finish.js', mocks) await reifyFinish(mock.npm, { options: { global: mock.npm.global }, + diff, actualTree: typeof actualTree === 'function' ? actualTree(mock) : actualTree, }) @@ -93,3 +106,62 @@ t.test('should write if everything above passes', async t => { const newFile = await readRc(join(mock.other, 'new-npm')) t.equal(mock.builtinRc.raw, newFile) }) + +t.test('unreviewedScripts filtered to nodes touched by this reify (npm/cli#9797)', async t => { + const captured = [] + const touched = { location: 'node_modules/touched', name: 'touched' } + const untouched = { location: 'node_modules/untouched', name: 'untouched' } + await mockReififyFinish(t, { + global: false, + unreviewedNodes: [touched, untouched], + diff: { + children: [ + { action: 'ADD', ideal: touched, children: [] }, + { action: 'REMOVE', actual: { location: 'node_modules/removed' }, children: [] }, + ], + }, + captureReifyOutput: (_npm, _arb, extras) => captured.push(extras), + }) + t.equal(captured.length, 1) + t.equal(captured[0].unreviewedScripts.length, 1, + 'untouched package is filtered out; only touched package is warned about') + t.equal(captured[0].unreviewedScripts[0].node.name, 'touched') +}) + +t.test('unreviewedScripts pass through when there is no diff (defensive)', async t => { + const captured = [] + const a = { location: 'node_modules/a', name: 'a' } + await mockReififyFinish(t, { + global: false, + unreviewedNodes: [a], + captureReifyOutput: (_npm, _arb, extras) => captured.push(extras), + }) + t.equal(captured[0].unreviewedScripts.length, 1) +}) + +t.test('diff walker handles CHANGE, nested children, and nullish diff entries', async t => { + const captured = [] + const changed = { location: 'node_modules/changed', name: 'changed' } + const nested = { location: 'node_modules/nested', name: 'nested' } + const untouched = { location: 'node_modules/untouched', name: 'untouched' } + await mockReififyFinish(t, { + global: false, + unreviewedNodes: [changed, nested, untouched], + diff: { + children: [ + null, + { action: 'CHANGE', ideal: changed, children: [] }, + { + action: 'ADD', + ideal: { location: null }, + children: [ + { action: 'ADD', ideal: nested }, + ], + }, + ], + }, + captureReifyOutput: (_npm, _arb, extras) => captured.push(extras), + }) + const names = captured[0].unreviewedScripts.map(u => u.node.name).sort() + t.strictSame(names, ['changed', 'nested']) +})