From 8ddb87bd22991cd36898651e288c6c1f8a0485b7 Mon Sep 17 00:00:00 2001 From: Manzoor Wani Date: Thu, 25 Jun 2026 00:10:47 +0530 Subject: [PATCH] fix(arborist): invalid filterNode crash under the linked strategy (#9637) In continuation of our exploration of using `install-strategy=linked` in the [Gutenberg monorepo](https://github.com/WordPress/gutenberg/pull/75814), which powers the WordPress Block Editor. Under `install-strategy=linked`, several common installs failed with `npm error invalid filterNode: outside idealTree/actualTree`, with no workaround besides dropping the linked strategy. Hoisted handled all of them. This fixes two distinct paths that both produced that error. ## Why A linked reify diffs the ideal tree against a synthesized actual wrapper (`#linkedActualForDiff`) rather than `this.actualTree`. `Diff.calculate` rejects any filter node whose root is neither the ideal nor the actual it was given, so a filter node taken from the real `this.actualTree` is "outside" the diff and throws. Two places fed it such nodes: - `--workspaces=false` and `-w --include-workspace-root` go through the `includeRootDeps` branch of `_diffTrees()`, which collected root-dep edge targets from both `this.idealTree` and `this.actualTree`. The actual-side targets are rooted at the real actual tree, not the wrapper, so they tripped the guard. The sibling `includeWorkspaces` branch already accounted for this; the root-dep branch did not. - A global install with a per-call `installStrategy: 'linked'` re-engaged the linked path even though the constructor normalizes global installs to `shallow` (the linked layout is unsupported for globals). Re-installing an already-present global package then hit the global explicit-request branch, which pushes actual-side nodes, and tripped the same guard. Suppressing the crash there was worse: the isolated reifier does not materialize the global layout and removed the package instead. ## How `_diffTrees()` now iterates only the ideal tree for root-dep filter nodes when the linked wrapper is in use, matching the existing workspace-node handling. The ideal-side nodes are sufficient to scope the diff, and the post-reify orphan sweep continues to prune deps removed from the manifest. `reify()` now honors the constructor's global-to-shallow normalization when deriving the `linked` flag, so a global install never engages the linked path regardless of a per-call `installStrategy`. Global installs fall back to shallow, which materializes and upgrades packages correctly. No change to the global explicit-request branch is needed once global is never linked. ## References Fixes #9614 Part of #9608 (cherry picked from commit 2b976b521e6cf3804a79bd99d17757358d8bfa6b) --- workspaces/arborist/lib/arborist/reify.js | 9 +++- workspaces/arborist/test/arborist/reify.js | 58 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 6f1e2f402e55a..0c7b4fd0acfbf 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -90,7 +90,9 @@ module.exports = cls => class Reifier extends cls { // public method async reify (options = {}) { - const linked = (options.installStrategy || this.options.installStrategy) === 'linked' + // Global installs are normalized to the shallow strategy in the constructor; honor that here so a per-call installStrategy:'linked' can't re-engage the unsupported linked path. + const linked = !this.options.global && + (options.installStrategy || this.options.installStrategy) === 'linked' if (this.options.packageLockOnly && this.options.global) { const er = new Error('cannot generate lockfile for global packages') @@ -474,7 +476,10 @@ module.exports = cls => class Reifier extends cls { } if (includeRootDeps) { // add all non-workspace nodes to filterNodes - for (const tree of [this.idealTree, this.actualTree]) { + // Skip the actual tree under the linked diff wrapper: its edge targets have root===actualTree, not the wrapper, which trips Diff.calculate's filterNode guard. + // The ideal-side targets alone scope the diff. + const trees = this.#linkedActualForDiff ? [this.idealTree] : [this.idealTree, this.actualTree] + for (const tree of trees) { for (const { type, to } of tree.edgesOut.values()) { if (type !== 'workspace' && to) { filterNodes.push(to) diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index 08c2c2bde15be..42bd8f5941aa5 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -4514,6 +4514,64 @@ t.test('install strategy linked', async (t) => { }) }) +t.test('linked strategy --workspaces=false and --include-workspace-root do not crash', async t => { + // Regression for #9614. Under linked, the root-dep filter nodes came from the real actual tree, not the synthesized diff wrapper, tripping Diff.calculate's "invalid filterNode" guard. + const manifest = deps => JSON.stringify({ + name: 'root', + version: '1.0.0', + workspaces: ['packages/*'], + dependencies: deps, + }) + const path = t.testdir({ + 'package.json': manifest({ abbrev: '1.1.1', wrappy: '1.0.2' }), + packages: { + a: { + 'package.json': JSON.stringify({ name: 'a', version: '1.0.0' }), + }, + }, + }) + + createRegistry(t, true) + await reify(path, { installStrategy: 'linked' }) + + // --workspaces=false: only root deps are in scope. + await t.resolves( + reify(path, { installStrategy: 'linked', workspacesEnabled: false }), + '--workspaces=false does not crash' + ) + + // -w a --include-workspace-root: workspace a plus root deps in scope. + await t.resolves( + reify(path, { installStrategy: 'linked', workspaces: ['a'], includeWorkspaceRoot: true }), + '-w a --include-workspace-root does not crash' + ) + + t.ok(fs.lstatSync(resolve(path, 'node_modules/abbrev')).isSymbolicLink(), 'root dep still linked') + + // Dropping the actual-side filter nodes must not stop a filtered install from pruning a removed root dep. + fs.writeFileSync(resolve(path, 'package.json'), manifest({ abbrev: '1.1.1' })) + await reify(path, { installStrategy: 'linked', workspacesEnabled: false }) + t.notOk(fs.existsSync(resolve(path, 'node_modules/wrappy')), 'removed root dep pruned under filtered install') + t.ok(fs.lstatSync(resolve(path, 'node_modules/abbrev')).isSymbolicLink(), 'remaining root dep still linked') +}) + +t.test('global install ignores a per-call linked strategy', async t => { + // Regression for #9614. Global installs are normalized to shallow; a per-call installStrategy:'linked' must not re-engage the linked path, which would trip Diff.calculate's filterNode guard on re-install and delete the global package. + const path = t.testdir({ lib: {} }) + const lib = resolve(path, 'lib') + const nm = resolve(lib, 'node_modules') + + createRegistry(t, true) + await reify(lib, { add: ['abbrev@1.1.1'], global: true }) + + // Re-install the already-present package under linked: must not crash and must not remove it. + await t.resolves( + reify(lib, { add: ['abbrev@1.1.1'], global: true, installStrategy: 'linked' }), + 'global re-install under linked does not crash' + ) + t.strictSame(fs.readdirSync(nm), ['abbrev'], 'global package retained, no .store created') +}) + t.test('linked strategy exposes store node_modules via NODE_PATH for lifecycle scripts', async t => { // Regression for #9549. In the linked strategy a store package's deps are symlinked siblings in its store node_modules. // A separate bin invoked by the script (e.g. napi-postinstall) resolves modules from its own store realpath and cannot see them, so npm exposes them via NODE_PATH.