fix(arborist): improve override conflict detection with semantic comparison (#8689)

Resolves the issue where different override contexts (like Vaadin's
$@vaadin/react-components vs $@vaadin/react-components-pro) were
incorrectly treated as conflicts by structural comparison.

Fixes #8688

Author: Artur-

workspaces/arborist/lib/edge.js

@@ -276,9 +276,15 @@ class Edge {
       } else if (!this.satisfiedBy(this.#to)) {
         this.#error = 'INVALID'
       } else if (this.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) {
-        // Any inconsistency between the edge's override set and the target's override set is potentially problematic.
-        // But we only say the edge is in error if the override sets are plainly conflicting.
-        // Note that if the target doesn't have any dependencies of their own, then this inconsistency is irrelevant.
+        // Check for conflicts between the edge's override set and the target node's override set.
+        // This catches cases where different parts of the tree have genuinely incompatible
+        // version requirements for the same package.
+        // The improved conflict detection uses semantic comparison (checking for incompatible
+        // version ranges) rather than pure structural equality, avoiding false positives from:
+        // - Reference overrides ($syntax) that resolve to compatible versions
+        // - Peer dependencies with different but compatible override contexts
+        // Note: We only check if the target has dependencies (edgesOut.size > 0), since
+        // override conflicts are only relevant if the target has its own dependencies.
         this.#error = 'INVALID'
       } else {
         this.#error = 'OK'

workspaces/arborist/lib/override-set.js

@@ -201,8 +201,82 @@ class OverrideSet {
 
   static doOverrideSetsConflict (first, second) {
     // If override sets contain one another then we can try to use the more specific one.
-    // If neither one is more specific, then we consider them to be in conflict.
-    return (this.findSpecificOverrideSet(first, second) === undefined)
+    // If neither one is more specific, check for semantic conflicts.
+    const specificSet = this.findSpecificOverrideSet(first, second)
+    if (specificSet !== undefined) {
+      // One contains the other, so no conflict
+      return false
+    }
+
+    // The override sets are structurally incomparable, but this doesn't necessarily
+    // mean they conflict. We need to check if they have conflicting version requirements
+    // for any package that appears in both rulesets.
+    return this.haveConflictingRules(first, second)
+  }
+
+  static haveConflictingRules (first, second) {
+    // Get all rules from both override sets
+    const firstRules = first.ruleset
+    const secondRules = second.ruleset
+
+    // Check each package that appears in both rulesets
+    for (const [key, firstRule] of firstRules) {
+      const secondRule = secondRules.get(key)
+      if (!secondRule) {
+        // Package only appears in one ruleset, no conflict
+        continue
+      }
+
+      // Same rule object means no conflict
+      if (firstRule === secondRule || firstRule.isEqual(secondRule)) {
+        continue
+      }
+
+      // Both rulesets have rules for this package with different values.
+      // Check if the version requirements are actually incompatible.
+      const firstValue = firstRule.value
+      const secondValue = secondRule.value
+
+      // If either value is a reference (starts with $), we can't determine
+      // compatibility here - the reference might resolve to compatible versions.
+      // We defer to runtime resolution rather than failing early.
+      if (firstValue.startsWith('$') || secondValue.startsWith('$')) {
+        continue
+      }
+
+      // Check if the version ranges are compatible using semver
+      // If both specify version ranges, they conflict only if they have no overlap
+      try {
+        const firstSpec = npa(`${firstRule.name}@${firstValue}`)
+        const secondSpec = npa(`${secondRule.name}@${secondValue}`)
+
+        // For range/version types, check if they intersect
+        if ((firstSpec.type === 'range' || firstSpec.type === 'version') &&
+            (secondSpec.type === 'range' || secondSpec.type === 'version')) {
+          // Check if the ranges intersect
+          const firstRange = firstSpec.fetchSpec
+          const secondRange = secondSpec.fetchSpec
+
+          // If the ranges don't intersect, we have a real conflict
+          if (!semver.intersects(firstRange, secondRange)) {
+            log.silly('Found conflicting override rules', {
+              package: firstRule.name,
+              first: firstValue,
+              second: secondValue,
+            })
+            return true
+          }
+        }
+        // For other types (git, file, directory, tag), we can't easily determine
+        // compatibility, so we conservatively assume no conflict
+      } catch {
+        // If we can't parse the specs, conservatively assume no conflict
+        // Real conflicts will be caught during dependency resolution
+      }
+    }
+
+    // No conflicting rules found
+    return false
   }
 }
 

workspaces/arborist/test/node.js

@@ -3277,48 +3277,212 @@ t.test('should propagate the new override set to the target node', t => {
   t.end()
 })
 
-t.test('should find inconsistency between the edge\'s override set and the target\'s override set', t => {
-  const tree = new Node({
-    loadOverrides: true,
-    path: '/root',
-    pkg: {
-      name: 'root',
-      version: '1.0.0',
-      dependencies: {
-        mockDep: '1.x',
+t.test('override conflict detection with semantic comparison', t => {
+  t.test('non-conflicting different override sets should be valid', t => {
+    // Regression test for issue #8688
+    // This validates that the improved semantic conflict detection allows
+    // structurally different override sets that don't actually conflict.
+
+    // Create two different override sets (simulating Vaadin's structure)
+    // These override different packages, so they don't conflict
+    const overridesComponents = new OverrideSet({
+      overrides: {
+        '@vaadin/react-components': '24.9.2',
       },
+    })
+
+    const overridesComponentsPro = new OverrideSet({
       overrides: {
-        mockDep: '2.x',
+        '@vaadin/react-components-pro': '24.9.2',
       },
-    },
-    children: [{
-      name: 'mockDep',
-      version: '2.0.0',
+    })
+
+    const tree = new Node({
+      loadOverrides: true,
+      path: '/root',
       pkg: {
+        name: 'root',
+        version: '1.0.0',
         dependencies: {
-          subDep: '1.0.0',
+          mockDep: '1.x',
+        },
+        overrides: {
+          mockDep: '2.x',
         },
       },
       children: [{
-        name: 'subDep',
-        version: '1.0.0',
-        pkg: {},
+        name: 'mockDep',
+        version: '2.0.0',
+        pkg: {
+          dependencies: {
+            subDep: '1.0.0',
+          },
+        },
+        children: [{
+          name: 'subDep',
+          version: '1.0.0',
+          pkg: {},
+        }],
       }],
-    }],
+    })
+
+    const edge = tree.edgesOut.get('mockDep')
+
+    // Manually set an override to the edge
+    edge.overrides = overridesComponents
+
+    // Override satisfiedBy so it returns true, ensuring the conflict branch is reached
+    edge.satisfiedBy = () => true
+
+    // Set different but non-conflicting override on the target node
+    const mockDep = tree.children.get('mockDep')
+    mockDep.overrides = overridesComponentsPro
+
+    // Force edge to recalculate
+    edge.reload(true)
+
+    // The edge should be valid despite different override contexts
+    // because they don't have conflicting version requirements
+    t.equal(edge.error, null, 'Edge should be valid with non-conflicting override contexts')
+    t.ok(edge.valid, 'Edge.valid should be true')
+
+    t.end()
   })
 
-  // Force edge.override to a conflicting object so that it will differ from
-  // the computed override coming from the parent's override set.
-  const conflictingOverride = new OverrideSet({
-    overrides: { mockDep: '1.x' },
+  t.test('conflicting override sets should be detected as invalid', t => {
+    // This validates that actual conflicts ARE still caught by the semantic detection
+
+    // Create two override sets with conflicting version requirements for the same package
+    const overridesV1 = new OverrideSet({
+      overrides: {
+        lodash: '1.x',
+      },
+    })
+
+    const overridesV4 = new OverrideSet({
+      overrides: {
+        lodash: '4.x',
+      },
+    })
+
+    const tree = new Node({
+      loadOverrides: true,
+      path: '/root',
+      pkg: {
+        name: 'root',
+        version: '1.0.0',
+        dependencies: {
+          mockDep: '1.x',
+        },
+        overrides: {
+          mockDep: '2.x',
+        },
+      },
+      children: [{
+        name: 'mockDep',
+        version: '2.0.0',
+        pkg: {
+          dependencies: {
+            lodash: '1.0.0',
+          },
+        },
+        children: [{
+          name: 'lodash',
+          version: '1.0.0',
+          pkg: {},
+        }],
+      }],
+    })
+
+    const edge = tree.edgesOut.get('mockDep')
+    const mockDep = tree.children.get('mockDep')
+
+    // Manually set conflicting overrides
+    edge.overrides = overridesV1
+    mockDep.overrides = overridesV4
+
+    // Override satisfiedBy so it returns true, ensuring the conflict branch is reached
+    edge.satisfiedBy = () => true
+
+    // Clear the cached error by calling reload(true)
+    edge.reload(true)
+
+    // Re-set the overrides after reload (since reload may have changed them)
+    edge.overrides = overridesV1
+    mockDep.overrides = overridesV4
+
+    // The edge should be INVALID due to conflicting override requirements
+    t.equal(edge.error, 'INVALID', 'Edge should be invalid with conflicting override versions')
+    t.notOk(edge.valid, 'Edge.valid should be false')
+
+    t.end()
   })
-  const edge = tree.edgesOut.get('mockDep')
-  edge.overrides = conflictingOverride
 
-  // Override satisfiedBy so it returns true, ensuring the conflict branch is reached
-  edge.satisfiedBy = () => true
+  t.test('reference overrides should not cause false positives', t => {
+    // This validates that reference overrides ($syntax) don't trigger false positives
+
+    const overridesRef1 = new OverrideSet({
+      overrides: {
+        lodash: '$some-reference',
+      },
+    })
 
-  t.equal(tree.edgesOut.get('mockDep').error, 'INVALID', 'Edge should be marked INVALID due to conflicting overrides')
+    const overridesRef2 = new OverrideSet({
+      overrides: {
+        lodash: '$another-reference',
+      },
+    })
+
+    const tree = new Node({
+      loadOverrides: true,
+      path: '/root',
+      pkg: {
+        name: 'root',
+        version: '1.0.0',
+        dependencies: {
+          mockDep: '1.x',
+        },
+        overrides: {
+          mockDep: '2.x',
+        },
+      },
+      children: [{
+        name: 'mockDep',
+        version: '2.0.0',
+        pkg: {
+          dependencies: {
+            lodash: '4.0.0',
+          },
+        },
+        children: [{
+          name: 'lodash',
+          version: '4.0.0',
+          pkg: {},
+        }],
+      }],
+    })
+
+    const edge = tree.edgesOut.get('mockDep')
+
+    // Set reference overrides
+    edge.overrides = overridesRef1
+
+    // Override satisfiedBy so it returns true, ensuring the conflict branch is reached
+    edge.satisfiedBy = () => true
+
+    const mockDep = tree.children.get('mockDep')
+    mockDep.overrides = overridesRef2
+
+    // Force edge to recalculate
+    edge.reload(true)
+
+    // Reference overrides should not cause conflicts because we can't determine
+    // their compatibility at this stage - they might resolve to the same version
+    t.equal(edge.error, null, 'Edge should be valid with reference overrides')
+    t.ok(edge.valid, 'Edge.valid should be true with reference overrides')
+
+    t.end()
+  })
 
   t.end()
 })