From 03b96bf2ffb2d109d503202919b54e5c85b5b9d3 Mon Sep 17 00:00:00 2001 From: Sanjays2402 <51058514+Sanjays2402@users.noreply.github.com> Date: Fri, 24 Jul 2026 01:04:10 -0700 Subject: [PATCH] fix(cli): scope install-scripts warning to packages touched by this reify npm reify's post-install "install scripts blocked" warning walked the whole actual tree, so `npm update -g ` (or any partial install) warned about pre-existing, untouched global packages whose install scripts were never going to run this session. Filter the unreviewed list against arb.diff so only packages actually added or changed in this run are reported. Fixes #9797 --- lib/utils/reify-finish.js | 36 ++++++++++++++- test/lib/utils/reify-finish.js | 80 ++++++++++++++++++++++++++++++++-- 2 files changed, 111 insertions(+), 5 deletions(-) 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']) +})