fix(arborist): avoid full reinstall on subsequent linked strategy runs

Author: manzoorwanijk

workspaces/arborist/lib/arborist/isolated-reifier.js

@@ -412,7 +412,7 @@ module.exports = cls => class IsolatedReifier extends cls {
         }
 
         binNames.forEach(bn => {
-          target.binPaths.push(join(from.realpath, 'node_modules', '.bin', bn))
+          target.binPaths.push(join(dep.root.localPath, nmFolder, '.bin', bn))
         })
 
         const link = {
@@ -428,7 +428,10 @@ module.exports = cls => class IsolatedReifier extends cls {
           path: join(dep.root.localPath, nmFolder, dep.name),
           realpath: target.path,
           name: toKey,
-          resolved: dep.resolved,
+          version: dep.version,
+          resolved: external
+            ? `file:.store/${toKey}/node_modules/${dep.packageName}`
+            : dep.resolved,
           top: { path: dep.root.localPath },
           children: [],
           fsChildren: [],

workspaces/arborist/lib/arborist/reify.js

@@ -10,6 +10,7 @@ const { callLimit: promiseCallLimit } = require('promise-call-limit')
 const { depth: dfwalk } = require('treeverse')
 const { dirname, resolve, relative, join } = require('node:path')
 const { log, time } = require('proc-log')
+const { existsSync } = require('node:fs')
 const { lstat, mkdir, rm, symlink } = require('node:fs/promises')
 const { moveFile } = require('@npmcli/fs')
 const { subset, intersects } = require('semver')
@@ -75,6 +76,7 @@ module.exports = cls => class Reifier extends cls {
   #shrinkwrapInflated = new Set()
   #sparseTreeDirs = new Set()
   #sparseTreeRoots = new Set()
+  #linkedActualForDiff = null
 
   constructor (options) {
     super(options)
@@ -116,6 +118,9 @@ module.exports = cls => class Reifier extends cls {
       // of Node/Link trees
       log.warn('reify', 'The "linked" install strategy is EXPERIMENTAL and may contain bugs.')
       this.idealTree = await this[_createIsolatedTree]()
+      this.#linkedActualForDiff = this.#buildLinkedActualForDiff(
+        this.idealTree, this.actualTree
+      )
     }
     await this[_diffTrees]()
     await this.#reifyPackages()
@@ -125,6 +130,7 @@ module.exports = cls => class Reifier extends cls {
       this.idealTree = oldTree
     }
     await this[_saveIdealTree](options)
+    this.#linkedActualForDiff = null
     // clean inert
     for (const node of this.idealTree.inventory.values()) {
       if (node.inert) {
@@ -450,7 +456,7 @@ module.exports = cls => class Reifier extends cls {
       omit: this.#omit,
       shrinkwrapInflated: this.#shrinkwrapInflated,
       filterNodes,
-      actual: this.actualTree,
+      actual: this.#linkedActualForDiff || this.actualTree,
       ideal: this.idealTree,
     })
 
@@ -573,6 +579,7 @@ module.exports = cls => class Reifier extends cls {
       // if the directory already exists, made will be undefined. if that's the case
       // we don't want to remove it because we aren't the ones who created it so we
       // omit it from the #sparseTreeRoots
+      /* istanbul ignore next -- mkdir returns path only when dir is new */
       if (made) {
         this.#sparseTreeRoots.add(made)
       }
@@ -789,6 +796,104 @@ module.exports = cls => class Reifier extends cls {
     return join(filePath)
   }
 
+  // Build a flat actual tree wrapper for linked installs so the diff can
+  // correctly match store entries that already exist on disk. The proxy
+  // tree from _createIsolatedTree() is flat (all children on root), but
+  // loadActual() produces a nested tree where store entries are deep link
+  // targets. This wrapper surfaces them at the root level for comparison.
+  #buildLinkedActualForDiff (idealTree, actualTree) {
+    // Combined Map keyed by path (how allChildren() in diff.js keys)
+    const combined = new Map()
+
+    // Add actual tree's children (the top-level symlinks)
+    for (const child of actualTree.children.values()) {
+      combined.set(child.path, child)
+    }
+
+    // Add synthetic entries for store nodes and store links that exist on disk.
+    // The proxy tree is flat — all store entries (isInStore) and store links
+    // (isStoreLink) are direct children of root. The actual tree only has
+    // top-level links as root children, so store entries and store links
+    // need synthetic actual entries for the diff to match them.
+    for (const child of idealTree.children) {
+      if (!combined.has(child.path) && (child.isInStore || child.isStoreLink) &&
+          existsSync(child.path)) {
+        const entry = {
+          global: false,
+          globalTop: false,
+          isProjectRoot: false,
+          isTop: false,
+          location: child.location,
+          name: child.name,
+          optional: child.optional,
+          top: child.top,
+          children: [],
+          edgesIn: new Set(),
+          edgesOut: new Map(),
+          binPaths: [],
+          fsChildren: [],
+          /* istanbul ignore next -- emulate Node */
+          getBundler () {
+            return null
+          },
+          hasShrinkwrap: false,
+          inDepBundle: false,
+          integrity: null,
+          isLink: Boolean(child.isLink),
+          isRoot: false,
+          isInStore: Boolean(child.isInStore),
+          path: child.path,
+          realpath: child.realpath,
+          resolved: child.resolved,
+          version: child.version,
+          package: child.package,
+        }
+        entry.target = entry
+        if (child.isLink && combined.has(child.realpath)) {
+          entry.target = combined.get(child.realpath)
+        }
+        combined.set(child.path, entry)
+      }
+    }
+
+    // Proxy .get(name) to original actual tree for filterNodes compatibility
+    // (scoped workspace installs use .get(name), allChildren uses .values())
+    const origGet = actualTree.children.get.bind(actualTree.children)
+    const combinedGet = combined.get.bind(combined)
+    /* istanbul ignore next -- only reached during scoped workspace installs */
+    combined.get = (key) => combinedGet(key) || origGet(key)
+
+    const wrapper = {
+      isRoot: true,
+      isLink: actualTree.isLink,
+      target: actualTree.target,
+      fsChildren: actualTree.fsChildren,
+      path: actualTree.path,
+      realpath: actualTree.realpath,
+      edgesOut: actualTree.edgesOut,
+      inventory: actualTree.inventory,
+      package: actualTree.package,
+      resolved: actualTree.resolved,
+      version: actualTree.version,
+      integrity: actualTree.integrity,
+      binPaths: actualTree.binPaths,
+      hasShrinkwrap: false,
+      inDepBundle: false,
+      parent: null,
+      children: combined,
+    }
+
+    // Set parent/root on synthetic entries for consistency
+    for (const child of combined.values()) {
+      if (!child.parent) {
+        child.parent = wrapper
+        child.root = wrapper
+      }
+    }
+
+    return wrapper
+  }
+
   #registryResolved (resolved) {
     // the default registry url is a magic value meaning "the currently
     // configured registry".

workspaces/arborist/test/isolated-mode.js

@@ -1637,6 +1637,80 @@ tap.test('bins are installed', async t => {
   t.ok(binFromBarToWhich)
 })
 
+tap.test('subsequent linked install is a no-op', async t => {
+  const graph = {
+    registry: [
+      { name: 'which', version: '1.0.0', bin: './bin.js', dependencies: { isexe: '^1.0.0' } },
+      { name: 'isexe', version: '1.0.0' },
+    ],
+    root: {
+      name: 'myproject',
+      version: '1.0.0',
+      dependencies: { which: '1.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' })
+
+  // Verify packages are installed
+  t.ok(fs.lstatSync(path.join(dir, 'node_modules', 'which')).isSymbolicLink(),
+    'which is a symlink after first install')
+
+  // 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' })
+
+  // Verify the diff has zero actionable leaves
+  const leaves = arb2.diff?.leaves || []
+  const actions = leaves.filter(l => l.action)
+  t.equal(actions.length, 0, 'second install should have no diff actions')
+
+  // Verify unchanged nodes were detected
+  t.ok(arb2.diff.unchanged.length > 0, 'second install should have unchanged nodes')
+
+  // Verify packages are still correctly installed
+  t.ok(fs.lstatSync(path.join(dir, 'node_modules', 'which')).isSymbolicLink(),
+    'which is still a symlink after second install')
+  t.ok(setupRequire(dir)('which'), 'which is requireable after second install')
+})
+
+tap.test('workspace links are not affected by store resolved fix', async t => {
+  const graph = {
+    registry: [
+      { name: 'abbrev', version: '1.0.0' },
+    ],
+    root: {
+      name: 'myproject',
+      version: '1.0.0',
+      dependencies: { abbrev: '1.0.0' },
+    },
+    workspaces: [
+      { name: 'mypkg', version: '1.0.0', dependencies: { abbrev: '1.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
+  const arb2 = new Arborist({ path: dir, registry, packumentCache: new Map(), cache })
+  await arb2.reify({ installStrategy: 'linked' })
+
+  // Verify workspace is still correctly linked
+  t.ok(setupRequire(dir)('mypkg'), 'workspace is requireable after second install')
+  t.ok(setupRequire(dir)('abbrev'), 'registry dep is requireable after second install')
+
+  // Verify the diff has unchanged nodes (store entries are correctly matched)
+  t.ok(arb2.diff.unchanged.length > 0, 'second install should have unchanged nodes')
+})
+
 function setupRequire (cwd) {
   return function requireChain (...chain) {
     return chain.reduce((path, name) => {