fix(arborist): v10 - backport multiple fixes for linked install (#9098)

manzoorwanijk · web-flow · commit d6fe671dfb61 · 2026-03-10T10:45:36.000-07:00
This is a cherry-pick of - c7702d0 - #9094 - b56986a - #9095 - 3b70a9d - #9097 - 5b7c0cc - #9096
diff --git a/lib/commands/ls.js b/lib/commands/ls.js
@@ -59,6 +59,7 @@ class LS extends ArboristWorkspaceCmd {
     const unicode = this.npm.config.get('unicode')
     const packageLockOnly = this.npm.config.get('package-lock-only')
     const workspacesEnabled = this.npm.flatOptions.workspacesEnabled
+    const installStrategy = this.npm.flatOptions.installStrategy
 
     const path = global ? resolve(this.npm.globalDir, '..') : this.npm.prefix
 
@@ -138,6 +139,9 @@ class LS extends ArboristWorkspaceCmd {
               link,
               omit,
             }) : () => true)
+            .filter(installStrategy === 'linked'
+              ? filterLinkedStrategyEdges({ node, currentDepth })
+              : () => true)
             .map(mapEdgesToNodes({ seenPaths }))
             .concat(appendExtraneousChildren({ node, seenPaths }))
             .sort(sortAlphabetically)
@@ -405,6 +409,34 @@ const getJsonOutputItem = (node, { global, long }) => {
   return augmentItemWithIncludeMetadata(node, item)
 }
 
+// In linked strategy, two types of edges produce false UNMET DEPENDENCYs:
+// 1. Workspace edges for undeclared workspaces: the lockfile records edges from root to ALL workspaces, but only declared workspaces are hoisted to root/node_modules in linked mode.  Undeclared ones are intentionally absent.
+// 2. Dev edges on non-root packages: store package link targets have no parent in the node tree, so they are treated as "top" nodes and their devDependencies are loaded as edges.  Those devDeps are never installed.
+const filterLinkedStrategyEdges = ({ node, currentDepth }) => {
+  const declaredDeps = new Set(Object.keys(Object.assign({},
+    node.target.package.dependencies,
+    node.target.package.devDependencies,
+    node.target.package.optionalDependencies,
+    node.target.package.peerDependencies
+  )))
+
+  return (edge) => {
+    // Skip workspace edges for undeclared workspaces at root level
+    if (currentDepth === 0 && edge.type === 'workspace' && edge.missing) {
+      if (!declaredDeps.has(edge.name)) {
+        return false
+      }
+    }
+
+    // Skip dev edges for non-root packages (store packages)
+    if (currentDepth > 0 && edge.dev) {
+      return false
+    }
+
+    return true
+  }
+}
+
 const filterByEdgesTypes = ({ link, omit }) => (edge) => {
   for (const omitType of omit) {
     if (edge[omitType]) {
diff --git a/test/lib/commands/ls.js b/test/lib/commands/ls.js
@@ -5325,3 +5325,123 @@ t.test('ls --package-lock-only', async t => {
     })
   })
 })
+
+t.test('completion', async t => {
+  const { ls } = await mockNpm(t, {
+    command: 'ls',
+    prefixDir: {
+      node_modules: {
+        foo: {
+          'package.json': JSON.stringify({ name: 'foo', version: '1.0.0' }),
+        },
+      },
+      'package.json': JSON.stringify({ name: 'project', version: '1.0.0' }),
+    },
+  })
+  const res = await ls.completion({ conf: { argv: { remain: ['npm', 'ls'] } } })
+  t.type(res, Array)
+})
+
+t.test('ls --install-strategy=linked', async t => {
+  t.test('should not report undeclared workspaces as UNMET DEPENDENCY', async t => {
+    const { result, ls } = await mockLs(t, {
+      config: {
+        'install-strategy': 'linked',
+      },
+      prefixDir: {
+        'package.json': JSON.stringify({
+          name: 'test-linked-ws',
+          version: '1.0.0',
+          workspaces: ['packages/*'],
+          dependencies: { 'workspace-a': '*' },
+        }),
+        packages: {
+          'workspace-a': {
+            'package.json': JSON.stringify({
+              name: 'workspace-a',
+              version: '1.0.0',
+            }),
+          },
+          'workspace-b': {
+            'package.json': JSON.stringify({
+              name: 'workspace-b',
+              version: '1.0.0',
+            }),
+          },
+        },
+        node_modules: {
+          'workspace-a': t.fixture('symlink', '../packages/workspace-a'),
+          // workspace-b intentionally NOT linked (undeclared in dependencies)
+        },
+      },
+    })
+    await ls.exec([])
+    const output = cleanCwd(result())
+    t.notMatch(output, /UNMET DEPENDENCY/, 'should not report undeclared workspace as UNMET DEPENDENCY')
+    t.match(output, /workspace-a/, 'should list declared workspace')
+  })
+
+  t.test('should not report devDeps of store packages as UNMET DEPENDENCY', async t => {
+    const { result, ls } = await mockLs(t, {
+      config: {
+        'install-strategy': 'linked',
+      },
+      prefixDir: {
+        'package.json': JSON.stringify({
+          name: 'test-linked-store',
+          version: '1.0.0',
+          dependencies: { nopt: '^1.0.0' },
+        }),
+        node_modules: {
+          nopt: t.fixture('symlink', '.store/nopt@1.0.0/node_modules/nopt'),
+          '.store': {
+            'nopt@1.0.0': {
+              node_modules: {
+                nopt: {
+                  'package.json': JSON.stringify({
+                    name: 'nopt',
+                    version: '1.0.0',
+                    devDependencies: { tap: '^16.0.0' },
+                  }),
+                },
+              },
+            },
+          },
+        },
+      },
+    })
+    await ls.exec([])
+    const output = cleanCwd(result())
+    t.notMatch(output, /UNMET DEPENDENCY/, 'should not report devDeps of store packages')
+    t.match(output, /nopt/, 'should list the dependency')
+  })
+
+  t.test('should still report declared workspace as UNMET DEPENDENCY when missing', async t => {
+    const { ls } = await mockLs(t, {
+      config: {
+        'install-strategy': 'linked',
+      },
+      prefixDir: {
+        'package.json': JSON.stringify({
+          name: 'test-linked-ws-missing',
+          version: '1.0.0',
+          workspaces: ['packages/*'],
+          dependencies: { 'workspace-a': '*' },
+        }),
+        packages: {
+          'workspace-a': {
+            'package.json': JSON.stringify({
+              name: 'workspace-a',
+              version: '1.0.0',
+            }),
+          },
+        },
+        node_modules: {
+          // workspace-a is declared but its symlink is missing
+        },
+      },
+    })
+    await t.rejects(ls.exec([]), { code: 'ELSPROBLEMS' },
+      'should report declared workspace as UNMET DEPENDENCY')
+  })
+})
diff --git a/workspaces/arborist/lib/arborist/isolated-reifier.js b/workspaces/arborist/lib/arborist/isolated-reifier.js
@@ -80,12 +80,12 @@ module.exports = cls => class IsolatedReifier extends cls {
     // For isolated/linked mode, only include workspaces that root explicitly declares as dependencies.
     // When omitting dep types, exclude those from the declared set so their workspaces aren't hoisted.
     const rootPkg = idealTree.package
-    this.#rootDeclaredDeps = new Set([
-      ...Object.keys(rootPkg.dependencies || {}),
-      ...(!omit.has('dev') ? Object.keys(rootPkg.devDependencies || {}) : []),
-      ...(!omit.has('optional') ? Object.keys(rootPkg.optionalDependencies || {}) : []),
-      ...(!omit.has('peer') ? Object.keys(rootPkg.peerDependencies || {}) : []),
-    ])
+    this.#rootDeclaredDeps = new Set(Object.keys(Object.assign({},
+      rootPkg.dependencies,
+      (!omit.has('dev') && rootPkg.devDependencies),
+      (!omit.has('optional') && rootPkg.optionalDependencies),
+      (!omit.has('peer') && rootPkg.peerDependencies)
+    )))
 
     // XXX this sometimes acts like a node too
     this.idealGraph = {
diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js
@@ -842,24 +842,23 @@ module.exports = cls => class Reifier extends cls {
   #buildLinkedActualForDiff (idealTree, actualTree) {
     const combined = new Map()
 
-    for (const child of actualTree.children.values()) {
-      combined.set(child.path, child)
-    }
-
+    // Create synthetic actual entries for ALL ideal children that exist on disk.
+    // The isolated ideal tree is flat (all entries as root children), but loadActual() produces a nested tree where workspace deps are under fsChildren and store entries are deep link targets.
+    // Synthetic entries ensure the diff compares matching resolved/integrity values (e.g. workspace links have resolved=undefined in the ideal tree but resolved="file:../packages/..." in the actual tree).
     for (const child of idealTree.children.values()) {
-      if (!combined.has(child.path) && (child.isInStore || child.isStoreLink) &&
-          existsSync(child.path)) {
-        let entry
-        if (child.isLink) {
-          entry = new IsolatedLink(child)
-        } else {
-          entry = new IsolatedNode(child)
-        }
-        if (child.isLink && combined.has(child.realpath)) {
-          entry.target = combined.get(child.realpath)
-        }
-        combined.set(child.path, entry)
+      if (combined.has(child.path) || !existsSync(child.path)) {
+        continue
+      }
+      let entry
+      if (child.isLink) {
+        entry = new IsolatedLink(child)
+      } else {
+        entry = new IsolatedNode(child)
+      }
+      if (child.isLink && combined.has(child.realpath)) {
+        entry.target = combined.get(child.realpath)
       }
+      combined.set(child.path, entry)
     }
 
     const origGet = actualTree.children.get.bind(actualTree.children)
@@ -878,7 +877,9 @@ module.exports = cls => class Reifier extends cls {
     wrapper.binPaths = actualTree.binPaths
     wrapper.children = combined
     wrapper.edgesOut = actualTree.edgesOut
-    wrapper.fsChildren = actualTree.fsChildren
+    // Use empty fsChildren so that allChildren() only picks up entries from the combined map.
+    // The actual fsChildren have real children with different resolved values (e.g. file:../../../node_modules/.store/... vs file:.store/...) that would overwrite our synthetic entries in allChildren().
+    wrapper.fsChildren = new Set()
     wrapper.integrity = actualTree.integrity
     wrapper.inventory = actualTree.inventory
 
diff --git a/workspaces/arborist/lib/query-selector-all.js b/workspaces/arborist/lib/query-selector-all.js
@@ -785,9 +785,14 @@ const hasParent = (node, compareNodes) => {
       compareNode = compareNode.target
     }
 
-    // follows logical parent for link anscestors
+    // Follows logical parent for link ancestors (e.g. workspaces whose target lives outside node_modules).
+    // Only match if the node has a link whose parent is the compareNode. Without this check, nodes deep in the store (linked strategy) would incorrectly match as children of root via their fsParent chain.
     if (node.isTop && (node.resolveParent === compareNode)) {
-      return true
+      for (const link of node.linksIn) {
+        if (link.parent === compareNode) {
+          return true
+        }
+      }
     }
     // follows edges-in to check if they match a possible parent
     for (const edge of node.edgesIn) {
diff --git a/workspaces/arborist/test/isolated-mode.js b/workspaces/arborist/test/isolated-mode.js
@@ -1863,6 +1863,43 @@ tap.test('workspace links are not affected by store resolved fix', async t => {
   t.ok(arb2.diff.unchanged.length > 0, 'second install should have unchanged nodes')
 })
 
+tap.test('idempotent install with cross-workspace deps (diamond pattern)', async t => {
+  // Regression: when workspace-x depends on workspace-y (and both share a registry dep), the second install would report "changed N packages" because (1) workspace link resolved values didn't match between ideal and actual trees, and (2) actual fsChildren overwrote synthetic store entries in the diff proxy.
+  const graph = {
+    registry: [
+      { name: 'abbrev', version: '2.0.0' },
+    ],
+    root: {
+      name: 'myproject',
+      version: '1.0.0',
+      dependencies: { 'workspace-x': '*', 'workspace-y': '*' },
+    },
+    workspaces: [
+      { name: 'workspace-x', version: '1.0.0', dependencies: { abbrev: '2.0.0', 'workspace-y': '*' } },
+      { name: 'workspace-y', version: '1.0.0', dependencies: { abbrev: '2.0.0' } },
+    ],
+  }
+  const { dir, registry } = await getRepo(graph)
+  const cache = fs.mkdtempSync(`${getTempDir()}/test-`)
+
+  // First install
+  const arb1 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
+  await arb1.reify({ installStrategy: 'linked' })
+
+  // Second install — should detect everything is up-to-date
+  const arb2 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
+  await arb2.reify({ installStrategy: 'linked' })
+
+  const leaves = arb2.diff?.leaves || []
+  const actions = leaves.filter(l => l.action)
+  t.equal(actions.length, 0, 'second install should have no diff actions')
+  t.ok(arb2.diff.unchanged.length > 0, 'second install should have unchanged nodes')
+
+  // Verify packages are still correctly installed (abbrev is a workspace dep, not root)
+  t.ok(setupRequire(path.join(dir, 'packages', 'workspace-y'))('abbrev'),
+    'abbrev is requireable from workspace after second install')
+})
+
 tap.test('orphaned store entries are cleaned up on dependency update', async t => {
   const graph = {
     registry: [
diff --git a/workspaces/arborist/test/query-selector-all.js b/workspaces/arborist/test/query-selector-all.js
@@ -1019,3 +1019,90 @@ t.test('query-selector-all', async t => {
     ['#b *', ['a@1.0.0', 'bar@2.0.0', 'baz@1.0.0', 'lorem@1.0.0', 'moo@3.0.0']],
   ])
 })
+
+// Simulates the linked install strategy layout where packages live in node_modules/.store/ and are symlinked from node_modules/.
+t.test('linked strategy: :root > * excludes transitive deps and store nodes', async t => {
+  /*
+   fixture tree (linked strategy layout):
+
+    linked-test@1.0.0
+    ├── nopt@7.2.1 (symlink -> .store/nopt-hash/node_modules/nopt)
+    ├── ini@4.1.3 (symlink -> .store/ini-hash/node_modules/ini)
+    └── .store/
+        ├── nopt-hash/
+        │   └── node_modules/
+        │       ├── nopt@7.2.1 (actual)
+        │       └── abbrev (symlink -> ../../abbrev-hash/node_modules/abbrev)
+        ├── ini-hash/
+        │   └── node_modules/
+        │       └── ini@4.1.3 (actual)
+        └── abbrev-hash/
+            └── node_modules/
+                └── abbrev@2.0.0 (actual)
+  */
+
+  const path = t.testdir({
+    node_modules: {
+      nopt: t.fixture('symlink', '.store/nopt-hash/node_modules/nopt'),
+      ini: t.fixture('symlink', '.store/ini-hash/node_modules/ini'),
+      '.store': {
+        'nopt-hash': {
+          node_modules: {
+            nopt: {
+              'package.json': JSON.stringify({
+                name: 'nopt',
+                version: '7.2.1',
+                dependencies: { abbrev: '^2.0.0' },
+              }),
+            },
+            abbrev: t.fixture('symlink', '../../abbrev-hash/node_modules/abbrev'),
+          },
+        },
+        'ini-hash': {
+          node_modules: {
+            ini: {
+              'package.json': JSON.stringify({
+                name: 'ini',
+                version: '4.1.3',
+              }),
+            },
+          },
+        },
+        'abbrev-hash': {
+          node_modules: {
+            abbrev: {
+              'package.json': JSON.stringify({
+                name: 'abbrev',
+                version: '2.0.0',
+              }),
+            },
+          },
+        },
+      },
+    },
+    'package.json': JSON.stringify({
+      name: 'linked-test',
+      version: '1.0.0',
+      dependencies: {
+        nopt: '^7.0.0',
+        ini: '^4.0.0',
+      },
+    }),
+  })
+
+  const arb = new Arborist({ path })
+  const tree = await arb.loadActual()
+
+  const rootChildren = await querySelectorAll(tree, ':root > *')
+  t.same(rootChildren, [
+    'ini@4.1.3',
+    'nopt@7.2.1',
+  ], ':root > * should only return direct dependencies, not transitive deps or store nodes')
+
+  const rootDescendants = await querySelectorAll(tree, ':root *')
+  t.same(rootDescendants, [
+    'abbrev@2.0.0',
+    'ini@4.1.3',
+    'nopt@7.2.1',
+  ], ':root * should return all descendants including transitive deps')
+})
