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

Sibling override sets (e.g. react + react-dom children of root) were treated as conflicting because findSpecificOverrideSet only checked ancestor relationships. Now checks haveConflictingRules and falls back to the nearest common ancestor when rules are compatible.

Also caps ERESOLVE report depth to 16 and adds cycle detection in explain-dep to prevent RangeError: Invalid string length in large trees.

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 }

lib/utils/explain-eresolve.js

@@ -54,7 +54,7 @@ const report = (expl, chalk, noColorChalk) => {
 
   return {
     explanation: `${explain(expl, chalk, 4)}\n\n${fix}`,
-    file: `# npm resolution error report\n\n${explain(expl, noColorChalk, Infinity)}\n\n${fix}`,
+    file: `# npm resolution error report\n\n${explain(expl, noColorChalk, 16)}\n\n${fix}`,
   }
 }
 

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`] = `
+cycle-a@1.0.0
+node_modules/cycle-a
+  cycle-a@"1.x" from cycle-b@2.0.0
+  node_modules/cycle-b
+    cycle-b@"2.x" from cycle-a@1.0.0
+    node_modules/cycle-a
+      cycle-a@"1.x" from cycle-b@2.0.0
+      node_modules/cycle-b
+`
+
+exports[`test/lib/utils/explain-dep.js TAP basic > circular dependency from other side 1`] = `
+cycle-b@2.0.0
+node_modules/cycle-b
+  cycle-b@"2.x" from cycle-a@1.0.0
+  node_modules/cycle-a
+    cycle-a@"1.x" from cycle-b@2.0.0
+    node_modules/cycle-b
+      cycle-b@"2.x" from cycle-a@1.0.0
+      node_modules/cycle-a
+`
+
 exports[`test/lib/utils/explain-dep.js TAP basic > ellipses test one 1`] = `
 manydep@1.0.0
   manydep@"1.0.0" from prod-dep@1.2.3
@@ -21,6 +43,11 @@ manydep@1.0.0
   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 parent-pkg@2.0.0
+node_modules/parent-pkg
+`
+
 exports[`test/lib/utils/explain-dep.js TAP basic bundled > explain color deep 1`] = `
 bundle-of-joy@1.0.0 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' } })