Skip to content

fix(arborist): skip extraneous orphans in strict-allow-scripts gate #9683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Sanjays2402 wants to merge 3 commits into
base: latest
Choose a base branch
from
+47 −0
Open

fix(arborist): skip extraneous orphans in strict-allow-scripts gate #9683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a8821c
fix(arborist): skip extraneous orphans in strict-allow-scripts gate
Sanjays2402 Jun 28, 2026
735428d
fix(arborist): tidy review comments to single-line per feedback
Sanjays2402 Jul 2, 2026
5e1c6da
test: cover extraneous strict allow-scripts nodes
Sanjays2402 Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions workspaces/arborist/lib/unreviewed-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ const collectUnreviewedScripts = async ({
// must not be flagged (npm/cli#9562).
continue
}
if (node.extraneous) {

@JamieMagee JamieMagee Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the reported install case, but this collector also walks the actual tree for npm install-scripts ls and strict npm rebuild.

I tested an extraneous package already present in node_modules. With this change, install-scripts ls reports nothing and strict rebuild succeeds instead of throwing ESTRICTALLOWSCRIPTS. The lower-level gate still blocks the script, but strict mode silently passes.

Could we limit this skip to reify preflights where pruning is enabled and the node will not be installed?

@JamieMagee JamieMagee Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also skips extraneous packages when unreviewedScripts() receives the installed actualTree. npm rebuild checks that tree before rebuilding, and extraneous packages are still rebuild candidates. For example, remove a package from package.json but leave it in node_modules, then run npm rebuild --strict-allow-scripts. The command exits successfully without rebuilding the package, and npm install-scripts ls does not report its script. Could we limit this skip to ideal-tree nodes that reify will prune?

// Extraneous = the node has no incoming dep edge from a real
// dependency, so it is slated for pruning before reify runs any
// install scripts. buildIdealTree drops top-level orphans, but can
// retain an orphan registry node nested inside a workspace's
// node_modules. In every case the script never executes, so the
// strict-allow-scripts gate must not surface it as unreviewed
// (npm/cli#9680).

@manzoorwanijk manzoorwanijk Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is too verbose, and sentences are broken by newlines, which doesn't work well for users who use screen readers.

continue
}

const verdict = isScriptAllowed(node, resolvedPolicy)
if (verdict === true || verdict === false) {
Expand Down
31 changes: 31 additions & 0 deletions workspaces/arborist/test/unreviewed-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const node = ({
isLink = false,
inBundle = false,
inert = false,
extraneous = false,
resolved,
} = {}) => ({
name,
Expand All @@ -41,6 +42,7 @@ const node = ({
isLink,
inBundle,
inert,
extraneous,
isRegistryDependency: true,
package: { name, version, scripts },
})
Expand Down Expand Up @@ -92,6 +94,35 @@ t.test('collectUnreviewedScripts', async t => {
t.strictSame(result, [])
})

t.test('skips extraneous (orphan) registry nodes', async t => {

@JamieMagee JamieMagee Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we cover this at the command level? We need one test for the lockfile-only orphan from #9680, plus a regression test confirming that an extraneous package on disk is still listed and still fails strict npm rebuild. These unit tests only verify the unconditional filter, so they miss the actual-tree behavior.

@Sanjays2402 Sanjays2402 Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the distinction. The unconditional collector skip is too broad: it fixes the lockfile-only ideal-tree orphan, but it also hides an extraneous package from the actual-tree install-scripts ls and strict npm rebuild paths.

I pushed 5e1c6da with a RED/GREEN preflight regression for the orphan, but that test does not cover the actual-tree behavior, so it does not close your full request. The fix needs to carry the prune/reify context into the strict install/ci preflight and only skip extraneous nodes there; ls and rebuild should continue to report and block them. I will rework the collector around that boundary rather than claim this version is complete.

// An extraneous node has no incoming edges and is slated to be pruned
// before reify runs any install scripts, so it must not gate the install
// under strict-allow-scripts even when the policy neither allows nor
// denies it. Buildup walks the ideal tree and can retain a nested orphan
// when nothing in the workspace depends on it (npm/cli#9680).

@manzoorwanijk manzoorwanijk Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment sentences are supposed to be on a single line and this one also is too verbose

const result = await collectUnreviewedScripts({
tree: tree([
node({ name: 'orphan', scripts: { install: 'x' }, extraneous: true }),
]),
policy: null,
})
t.strictSame(result, [])
})

t.test('skips extraneous nodes even when policy denies by name', async t => {
// Same orphan, but the user has a name-only deny entry. An extraneous
// registry node typically has no resolved URL the matcher can verify
// against, so the deny would not match and the node would fall through
// to "unreviewed". It still must not gate the install (npm/cli#9680).

@manzoorwanijk manzoorwanijk Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Let us not make it too verbose and ensure that each sentense is on a single line - not broken by newlines.

const orphan = node({ name: 'esbuild', scripts: { install: 'x' }, extraneous: true })
orphan.isRegistryDependency = false
const result = await collectUnreviewedScripts({
tree: tree([orphan]),
policy: { esbuild: false },
})
t.strictSame(result, [])
})

t.test('skips nodes with no install-relevant scripts', async t => {
const result = await collectUnreviewedScripts({
tree: tree([node({ scripts: { test: 'jest' } })]),
Expand Down
Loading