fix(arborist): resolve sibling override sets via common ancestor (#9110)

Author: manzoorwanijk

lib/utils/explain-dep.js

@@ -1,9 +1,9 @@
 const { relative } = require('node:path')
 
-const explainNode = (node, depth, chalk) =>
+const explainNode = (node, depth, chalk, seen = new Set()) =>
   printNode(node, chalk) +
-  explainDependents(node, depth, chalk) +
-  explainLinksIn(node, depth, chalk)
+  explainDependents(node, depth, chalk, seen) +
+  explainLinksIn(node, depth, chalk, seen)
 
 const colorType = (type, chalk) => {
   const style = type === 'extraneous' ? chalk.red
@@ -34,24 +34,24 @@ const printNode = (node, chalk) => {
     (node.location ? chalk.dim(`\n${node.location}`) : '')
 }
 
-const explainLinksIn = ({ linksIn }, depth, chalk) => {
+const explainLinksIn = ({ linksIn }, depth, chalk, seen) => {
   if (!linksIn || !linksIn.length || depth <= 0) {
     return ''
   }
 
-  const messages = linksIn.map(link => explainNode(link, depth - 1, chalk))
+  const messages = linksIn.map(link => explainNode(link, depth - 1, chalk, seen))
   const str = '\n' + messages.join('\n')
   return str.split('\n').join('\n  ')
 }
 
-const explainDependents = ({ dependents }, depth, chalk) => {
+const explainDependents = ({ dependents }, depth, chalk, seen) => {
   if (!dependents || !dependents.length || depth <= 0) {
     return ''
   }
 
   const max = Math.ceil(depth / 2)
   const messages = dependents.slice(0, max)
-    .map(edge => explainEdge(edge, depth, chalk))
+    .map(edge => explainEdge(edge, depth, chalk, seen))
 
   // show just the names of the first 5 deps that overflowed the list
   if (dependents.length > max) {
@@ -75,29 +75,42 @@ const explainDependents = ({ dependents }, depth, chalk) => {
   return str.split('\n').join('\n  ')
 }
 
-const explainEdge = ({ name, type, bundled, from, spec, rawSpec, overridden }, depth, chalk) => {
+const explainEdge = (
+  { name, type, bundled, from, spec, rawSpec, overridden },
+  depth, chalk, seen = new Set()
+) => {
   let dep = type === 'workspace'
     ? chalk.bold(relative(from.location, spec.slice('file:'.length)))
     : `${name}@"${spec}"`
   if (overridden) {
     dep = `${colorType('overridden', chalk)} ${dep} (was "${rawSpec}")`
   }
 
-  const fromMsg = ` from ${explainFrom(from, depth, chalk)}`
+  const fromMsg = ` from ${explainFrom(from, depth, chalk, seen)}`
 
   return (type === 'prod' ? '' : `${colorType(type, chalk)} `) +
     (bundled ? `${colorType('bundled', chalk)} ` : '') +
     `${dep}${fromMsg}`
 }
 
-const explainFrom = (from, depth, chalk) => {
+const explainFrom = (from, depth, chalk, seen) => {
   if (!from.name && !from.version) {
     return 'the root project'
   }
 
-  return printNode(from, chalk) +
-    explainDependents(from, depth - 1, chalk) +
-    explainLinksIn(from, depth - 1, chalk)
+  // Prevent infinite recursion from cycles in the dependency graph (e.g. linked strategy store nodes). Use stack-based tracking so diamond dependencies (same node reached via different paths) are still explained, but recursive cycles are broken.
+  const nodeId = `${from.name}@${from.version}:${from.location}`
+  if (seen.has(nodeId)) {
+    return printNode(from, chalk)
+  }
+  seen.add(nodeId)
+
+  const result = printNode(from, chalk) +
+    explainDependents(from, depth - 1, chalk, seen) +
+    explainLinksIn(from, depth - 1, chalk, seen)
+
+  seen.delete(nodeId)
+  return result
 }
 
 module.exports = { explainNode, printNode, explainEdge }

tap-snapshots/test/lib/utils/explain-dep.js.test.cjs

@@ -5,6 +5,28 @@
  * Make sure to inspect the output below.  Do not ignore changes!
  */
 'use strict'
+exports[`test/lib/utils/explain-dep.js TAP basic > circular dependency does not recurse infinitely 1`] = `
+[email protected]
+node_modules/cycle-a
+  cycle-a@"1.x" from [email protected]
+  node_modules/cycle-b
+    cycle-b@"2.x" from [email protected]
+    node_modules/cycle-a
+      cycle-a@"1.x" from [email protected]
+      node_modules/cycle-b
+`
+
+exports[`test/lib/utils/explain-dep.js TAP basic > circular dependency from other side 1`] = `
+[email protected]
+node_modules/cycle-b
+  cycle-b@"2.x" from [email protected]
+  node_modules/cycle-a
+    cycle-a@"1.x" from [email protected]
+    node_modules/cycle-b
+      cycle-b@"2.x" from [email protected]
+      node_modules/cycle-a
+`
+
 exports[`test/lib/utils/explain-dep.js TAP basic > ellipses test one 1`] = `
 [email protected]
   manydep@"1.0.0" from [email protected]
@@ -21,6 +43,11 @@ [email protected]
   6 more (optdep, extra-neos, deep-dev, peer, the root project, a package with a pretty long name)
 `
 
+exports[`test/lib/utils/explain-dep.js TAP basic > explainEdge without seen parameter 1`] = `
+some-dep@"1.x" from [email protected]
+node_modules/parent-pkg
+`
+
 exports[`test/lib/utils/explain-dep.js TAP basic bundled > explain color deep 1`] = `
 [email protected] bundled
 node_modules/bundle-of-joy

test/lib/utils/explain-dep.js

@@ -1,6 +1,6 @@
 const { resolve } = require('node:path')
 const t = require('tap')
-const { explainNode, printNode } = require('../../../lib/utils/explain-dep.js')
+const { explainNode, printNode, explainEdge } = require('../../../lib/utils/explain-dep.js')
 const { cleanCwd } = require('../../fixtures/clean-snapshot')
 
 t.cleanSnapshot = (str) => cleanCwd(str)
@@ -268,6 +268,47 @@ t.test('basic', async t => {
     })
   }
 
+  // Regression test for https://github.com/npm/cli/issues/9109
+  // Circular dependency graphs (common in linked strategy store nodes) should not cause infinite recursion in explainNode.
+  const cycleA = {
+    name: 'cycle-a',
+    version: '1.0.0',
+    location: 'node_modules/cycle-a',
+    dependents: [],
+  }
+  const cycleB = {
+    name: 'cycle-b',
+    version: '2.0.0',
+    location: 'node_modules/cycle-b',
+    dependents: [{
+      type: 'prod',
+      name: 'cycle-b',
+      spec: '2.x',
+      from: cycleA,
+    }],
+  }
+  cycleA.dependents = [{
+    type: 'prod',
+    name: 'cycle-a',
+    spec: '1.x',
+    from: cycleB,
+  }]
+  t.matchSnapshot(explainNode(cycleA, Infinity, noColor), 'circular dependency does not recurse infinitely')
+  t.matchSnapshot(explainNode(cycleB, Infinity, noColor), 'circular dependency from other side')
+
+  // explainEdge called without seen parameter (covers default seen = new Set() branch on explainEdge and explainFrom)
+  t.matchSnapshot(explainEdge({
+    type: 'prod',
+    name: 'some-dep',
+    spec: '1.x',
+    from: {
+      name: 'parent-pkg',
+      version: '2.0.0',
+      location: 'node_modules/parent-pkg',
+      dependents: [],
+    },
+  }, 2, noColor), 'explainEdge without seen parameter')
+
   // make sure that we show the last one if it's the only one that would
   // hit the ...
   cases.manyDeps.dependents.pop()

workspaces/arborist/lib/override-set.js

@@ -195,8 +195,29 @@ class OverrideSet {
       }
     }
 
-    // The override sets are incomparable. Neither one contains the other.
-    log.silly('Conflicting override sets', first, second)
+    // The override sets are incomparable (e.g. siblings like the "react" and "react-dom" children of the root override set). Check if they have semantically conflicting rules before treating this as an error.
+    if (this.haveConflictingRules(first, second)) {
+      log.silly('Conflicting override sets', first, second)
+      return undefined
+    }
+
+    // The override sets are structurally incomparable but have compatible rules. Fall back to their nearest common ancestor so the node still has a valid override set.
+    return this.findCommonAncestor(first, second)
+  }
+
+  static findCommonAncestor (first, second) {
+    const firstAncestors = []
+    for (const ancestor of first.ancestry()) {
+      firstAncestors.push(ancestor)
+    }
+    for (const secondAnc of second.ancestry()) {
+      for (const firstAnc of firstAncestors) {
+        if (firstAnc.isEqual(secondAnc)) {
+          return firstAnc
+        }
+      }
+    }
+    return null
   }
 
   static doOverrideSetsConflict (first, second) {

workspaces/arborist/test/override-set.js

@@ -511,6 +511,78 @@ t.test('constructor', async (t) => {
   })
 })
 
+t.test('findSpecificOverrideSet returns common ancestor for compatible siblings', async (t) => {
+  // Regression test for https://github.com/npm/cli/issues/9109
+  // When two top-level overrides (e.g. react + react-dom) share a transitive dep (e.g. loose-envify), the dep's node gets override sets from both siblings. findSpecificOverrideSet must return their common ancestor instead of undefined.
+  const root = new OverrideSet({
+    overrides: {
+      react: '18.3.1',
+      'react-dom': '18.3.1',
+    },
+  })
+
+  const reactChild = root.children.get('react')
+  const reactDomChild = root.children.get('react-dom')
+
+  t.ok(reactChild, 'react child exists')
+  t.ok(reactDomChild, 'react-dom child exists')
+
+  // These are siblings - neither is an ancestor of the other
+  const result = OverrideSet.findSpecificOverrideSet(reactChild, reactDomChild)
+  t.ok(result, 'findSpecificOverrideSet should not return undefined for compatible siblings')
+  t.ok(result.isEqual(root), 'should return the common ancestor (root)')
+
+  const reverse = OverrideSet.findSpecificOverrideSet(reactDomChild, reactChild)
+  t.ok(reverse, 'reverse order should also return a result')
+  t.ok(reverse.isEqual(root), 'reverse order should also return the root')
+})
+
+t.test('findCommonAncestor', async (t) => {
+  const root = new OverrideSet({
+    overrides: {
+      foo: '1.0.0',
+      bar: '2.0.0',
+    },
+  })
+
+  const fooChild = root.children.get('foo')
+  const barChild = root.children.get('bar')
+
+  const ancestor = OverrideSet.findCommonAncestor(fooChild, barChild)
+  t.ok(ancestor, 'should find a common ancestor')
+  t.ok(ancestor.isEqual(root), 'common ancestor should be the root')
+
+  // Two unrelated roots have no common ancestor
+  const other = new OverrideSet({ overrides: { baz: '3.0.0' } })
+  const noAncestor = OverrideSet.findCommonAncestor(fooChild, other)
+  t.equal(noAncestor, null, 'unrelated sets have no common ancestor')
+})
+
+t.test('findSpecificOverrideSet returns undefined for truly conflicting siblings', async (t) => {
+  // Siblings with conflicting rules should still return undefined
+  const root = new OverrideSet({
+    overrides: {
+      foo: {
+        '.': '1.0.0',
+        shared: '1.0.0',
+      },
+      bar: {
+        '.': '2.0.0',
+        shared: '99.0.0',
+      },
+    },
+  })
+
+  const fooChild = root.children.get('foo')
+  const barChild = root.children.get('bar')
+
+  t.ok(fooChild, 'foo child exists')
+  t.ok(barChild, 'bar child exists')
+
+  const result = OverrideSet.findSpecificOverrideSet(fooChild, barChild)
+  t.equal(result, undefined, 'truly conflicting siblings should return undefined')
+})
+
 t.test('coverage for isEqual edge cases', async t => {
   t.test('isEqual with null/undefined other', async t => {
     const overrides = new OverrideSet({ overrides: { foo: '1.0.0' } })