From a1e6f83d28144cc300970eab49ae267afe79cd49 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Tue, 21 Oct 2025 17:01:46 +0200 Subject: [PATCH 1/5] fix(arborist): remove overly strict override conflict detection The override conflict detection added in b9225e524 is overly conservative and produces false positives in two scenarios: 1. Peer dependencies: These resolve to nodes in different parts of the tree with legitimately different override contexts. 2. Reference overrides ($syntax): Different override sets using references (e.g., $@vaadin/react-components vs $@vaadin/react-components-pro) are structurally different but functionally equivalent when they resolve to the same versions. This fix removes the override conflict check entirely from edge validation. The check was redundant because: - Version satisfaction is already validated by satisfiedBy() - Any actual version conflicts in dependencies are caught during normal dependency resolution in the build/reify phase - The check caused false positives that prevented valid dependency configurations from working Fixes: #8688 --- workspaces/arborist/lib/edge.js | 13 ++-- workspaces/arborist/test/edge.js | 118 +++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index 242d2669ae4ca..d9d5775ae98fd 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -275,12 +275,15 @@ class Edge { this.#error = 'PEER LOCAL' } 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. - this.#error = 'INVALID' } else { + // Note: We intentionally do NOT check for override conflicts here. + // Different override sets pointing to the same node are allowed as long as: + // 1. The node satisfies the version requirement (checked by satisfiedBy above) + // 2. Any actual version conflicts in the node's dependencies will be caught + // during normal dependency resolution in the build/reify phase + // The previous check (from b9225e524) was overly conservative and produced + // false positives, especially with reference overrides ($syntax) that resolve + // to the same effective versions despite being structurally different. this.#error = 'OK' } } diff --git a/workspaces/arborist/test/edge.js b/workspaces/arborist/test/edge.js index cf5e2e3326539..b4d30d9a3e6ea 100644 --- a/workspaces/arborist/test/edge.js +++ b/workspaces/arborist/test/edge.js @@ -1244,3 +1244,121 @@ t.test('override fallback to local when root missing dependency with from.overri t.equal(edge.spec, '^1.2.3', 'should fall back to local package version from devDependencies') t.end() }) + +t.test('multiple edges with different override contexts to same node should be valid', t => { + // Regression test for https://github.com/npm/cli/issues/8688 + // Multiple packages with different override contexts can legitimately depend on the same + // shared package. This happens with reference overrides ($syntax) where different override + // sets (e.g., $@vaadin/react-components vs $@vaadin/react-components-pro) resolve to the + // same effective versions despite being structurally different. + + // Create two different override sets (simulating Vaadin's structure) + const overridesComponents = new OverrideSet({ + overrides: { + '@vaadin/react-components': '24.9.2', + }, + }) + + const overridesComponentsPro = new OverrideSet({ + overrides: { + '@vaadin/react-components-pro': '24.9.2', + }, + }) + + const root = { + name: 'root', + edgesOut: new Map(), + edgesIn: new Set(), + explain: () => 'root', + package: { name: 'root', version: '1.0.0' }, + get version () { return this.package.version }, + isTop: true, + parent: null, + overrides: overridesComponents, + resolve (n) { + return n === 'lit' ? lit : null + }, + addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, + addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + } + + // Shared dependency with non-peer dependencies + const lit = { + name: 'lit', + edgesOut: new Map([ + ['@lit/reactive-element', { name: '@lit/reactive-element', peer: false }], + ['lit-html', { name: 'lit-html', peer: false }], + ]), + edgesIn: new Set(), + explain: () => 'lit', + package: { name: 'lit', version: '3.3.1' }, + get version () { return this.package.version }, + parent: root, + root, + overrides: overridesComponents, + resolve (n) { return this.parent.resolve(n) }, + addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, + addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + } + + const componentA = { + name: 'component-a', + edgesOut: new Map(), + edgesIn: new Set(), + explain: () => 'component-a', + package: { name: 'component-a', version: '1.0.0' }, + get version () { return this.package.version }, + parent: root, + root, + overrides: overridesComponents, + resolve (n) { return this.parent.resolve(n) }, + addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, + addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + } + + const componentB = { + name: 'component-b', + edgesOut: new Map(), + edgesIn: new Set(), + explain: () => 'component-b', + package: { name: 'component-b', version: '1.0.0' }, + get version () { return this.package.version }, + parent: root, + root, + overrides: overridesComponentsPro, // Different override context + resolve (n) { return this.parent.resolve(n) }, + addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, + addEdgeIn (edge) { this.edgesIn.add(edge) }, + deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + } + + // Create two edges with different override contexts pointing to the same node + const edge1 = new Edge({ + from: componentA, + type: 'prod', + spec: '^3.0.0', + name: 'lit', + overrides: overridesComponents.getEdgeRule({ name: 'lit', spec: '^3.0.0' }), + }) + + const edge2 = new Edge({ + from: componentB, + type: 'prod', + spec: '^3.0.0', + name: 'lit', + overrides: overridesComponentsPro.getEdgeRule({ name: 'lit', spec: '^3.0.0' }), + }) + + // Before the fix (b9225e524), edge2 would be marked as INVALID because it has a different + // override context than edge1, and the override conflict check would fail. + // After the fix, both edges should be valid because they both satisfy the version requirement. + // Any actual conflicts in lit's dependencies will be caught during normal dependency resolution. + t.ok(edge1.valid, 'first edge with override context should be valid') + t.ok(edge2.valid, 'second edge with different override context should also be valid') + t.notOk(edge1.error, 'first edge should not have an error') + t.notOk(edge2.error, 'second edge should not have an error') + t.end() +}) From 3ef99ed24b02376dd08d6640683b3ede8e28b188 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Tue, 21 Oct 2025 19:36:42 +0200 Subject: [PATCH 2/5] lintfix --- workspaces/arborist/lib/edge.js | 1 - workspaces/arborist/test/edge.js | 76 ++++++++++++++++++++++++-------- 2 files changed, 57 insertions(+), 20 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index d9d5775ae98fd..f7e941bc56f4c 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -4,7 +4,6 @@ const util = require('node:util') const npa = require('npm-package-arg') const depValid = require('./dep-valid.js') -const OverrideSet = require('./override-set.js') class ArboristEdge { constructor (edge) { diff --git a/workspaces/arborist/test/edge.js b/workspaces/arborist/test/edge.js index b4d30d9a3e6ea..79827d5b9a367 100644 --- a/workspaces/arborist/test/edge.js +++ b/workspaces/arborist/test/edge.js @@ -1271,16 +1271,24 @@ t.test('multiple edges with different override contexts to same node should be v edgesIn: new Set(), explain: () => 'root', package: { name: 'root', version: '1.0.0' }, - get version () { return this.package.version }, + get version () { + return this.package.version + }, isTop: true, parent: null, overrides: overridesComponents, resolve (n) { return n === 'lit' ? lit : null }, - addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, - addEdgeIn (edge) { this.edgesIn.add(edge) }, - deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + addEdgeOut (edge) { + this.edgesOut.set(edge.name, edge) + }, + addEdgeIn (edge) { + this.edgesIn.add(edge) + }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } // Shared dependency with non-peer dependencies @@ -1293,14 +1301,24 @@ t.test('multiple edges with different override contexts to same node should be v edgesIn: new Set(), explain: () => 'lit', package: { name: 'lit', version: '3.3.1' }, - get version () { return this.package.version }, + get version () { + return this.package.version + }, parent: root, root, overrides: overridesComponents, - resolve (n) { return this.parent.resolve(n) }, - addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, - addEdgeIn (edge) { this.edgesIn.add(edge) }, - deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + resolve (n) { + return this.parent.resolve(n) + }, + addEdgeOut (edge) { + this.edgesOut.set(edge.name, edge) + }, + addEdgeIn (edge) { + this.edgesIn.add(edge) + }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const componentA = { @@ -1309,14 +1327,24 @@ t.test('multiple edges with different override contexts to same node should be v edgesIn: new Set(), explain: () => 'component-a', package: { name: 'component-a', version: '1.0.0' }, - get version () { return this.package.version }, + get version () { + return this.package.version + }, parent: root, root, overrides: overridesComponents, - resolve (n) { return this.parent.resolve(n) }, - addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, - addEdgeIn (edge) { this.edgesIn.add(edge) }, - deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + resolve (n) { + return this.parent.resolve(n) + }, + addEdgeOut (edge) { + this.edgesOut.set(edge.name, edge) + }, + addEdgeIn (edge) { + this.edgesIn.add(edge) + }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } const componentB = { @@ -1325,14 +1353,24 @@ t.test('multiple edges with different override contexts to same node should be v edgesIn: new Set(), explain: () => 'component-b', package: { name: 'component-b', version: '1.0.0' }, - get version () { return this.package.version }, + get version () { + return this.package.version + }, parent: root, root, overrides: overridesComponentsPro, // Different override context - resolve (n) { return this.parent.resolve(n) }, - addEdgeOut (edge) { this.edgesOut.set(edge.name, edge) }, - addEdgeIn (edge) { this.edgesIn.add(edge) }, - deleteEdgeIn (edge) { this.edgesIn.delete(edge) }, + resolve (n) { + return this.parent.resolve(n) + }, + addEdgeOut (edge) { + this.edgesOut.set(edge.name, edge) + }, + addEdgeIn (edge) { + this.edgesIn.add(edge) + }, + deleteEdgeIn (edge) { + this.edgesIn.delete(edge) + }, } // Create two edges with different override contexts pointing to the same node From 960c242b2facdbcf35ca537af0cfb3b65d0e0770 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Tue, 21 Oct 2025 19:36:51 +0200 Subject: [PATCH 3/5] Test removed: Override conflict detection was removed from edge validation because it caused false positives with reference overrides ($syntax) that resolve to functionally equivalent versions. Real conflicts are caught during the build/reify phase. See issue #8688 and the fix in edge.js --- workspaces/arborist/test/node.js | 46 -------------------------------- 1 file changed, 46 deletions(-) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 85212e53a4a39..abb0380843380 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -3277,52 +3277,6 @@ 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', - }, - overrides: { - mockDep: '2.x', - }, - }, - children: [{ - name: 'mockDep', - version: '2.0.0', - pkg: { - dependencies: { - subDep: '1.0.0', - }, - }, - children: [{ - name: 'subDep', - version: '1.0.0', - pkg: {}, - }], - }], - }) - - // 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' }, - }) - 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.equal(tree.edgesOut.get('mockDep').error, 'INVALID', 'Edge should be marked INVALID due to conflicting overrides') - - t.end() -}) - t.test('shouldOmit method', t => { t.test('dev dependency with dev omit', t => { const node = new Node({ From 0b813efee6f46ad3b6c85cdd22d824cc3285d5d9 Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Wed, 22 Oct 2025 10:37:50 +0200 Subject: [PATCH 4/5] test: replace obsolete override conflict test with regression test Replace the test 'should find inconsistency between the edge's override set and the target's override set' which was testing the override conflict detection code that was intentionally removed. The new test 'edges with different override contexts to same node should be valid' is a regression test for issue #8688. It verifies that edges remain valid when the edge and target node have different override contexts, as long as the version requirements are satisfied. The override conflict check (from b9225e524) was causing false positives, especially with reference overrides ($syntax) that resolve to functionally equivalent versions despite being structurally different. The fix removes this check because: 1. satisfiedBy() already validates version requirements 2. Real conflicts are caught during build/reify phase 3. The check compared override sets structurally, not functionally --- workspaces/arborist/test/edge.js | 156 ------------------------------- workspaces/arborist/test/node.js | 74 +++++++++++++++ 2 files changed, 74 insertions(+), 156 deletions(-) diff --git a/workspaces/arborist/test/edge.js b/workspaces/arborist/test/edge.js index 79827d5b9a367..cf5e2e3326539 100644 --- a/workspaces/arborist/test/edge.js +++ b/workspaces/arborist/test/edge.js @@ -1244,159 +1244,3 @@ t.test('override fallback to local when root missing dependency with from.overri t.equal(edge.spec, '^1.2.3', 'should fall back to local package version from devDependencies') t.end() }) - -t.test('multiple edges with different override contexts to same node should be valid', t => { - // Regression test for https://github.com/npm/cli/issues/8688 - // Multiple packages with different override contexts can legitimately depend on the same - // shared package. This happens with reference overrides ($syntax) where different override - // sets (e.g., $@vaadin/react-components vs $@vaadin/react-components-pro) resolve to the - // same effective versions despite being structurally different. - - // Create two different override sets (simulating Vaadin's structure) - const overridesComponents = new OverrideSet({ - overrides: { - '@vaadin/react-components': '24.9.2', - }, - }) - - const overridesComponentsPro = new OverrideSet({ - overrides: { - '@vaadin/react-components-pro': '24.9.2', - }, - }) - - const root = { - name: 'root', - edgesOut: new Map(), - edgesIn: new Set(), - explain: () => 'root', - package: { name: 'root', version: '1.0.0' }, - get version () { - return this.package.version - }, - isTop: true, - parent: null, - overrides: overridesComponents, - resolve (n) { - return n === 'lit' ? lit : null - }, - addEdgeOut (edge) { - this.edgesOut.set(edge.name, edge) - }, - addEdgeIn (edge) { - this.edgesIn.add(edge) - }, - deleteEdgeIn (edge) { - this.edgesIn.delete(edge) - }, - } - - // Shared dependency with non-peer dependencies - const lit = { - name: 'lit', - edgesOut: new Map([ - ['@lit/reactive-element', { name: '@lit/reactive-element', peer: false }], - ['lit-html', { name: 'lit-html', peer: false }], - ]), - edgesIn: new Set(), - explain: () => 'lit', - package: { name: 'lit', version: '3.3.1' }, - get version () { - return this.package.version - }, - parent: root, - root, - overrides: overridesComponents, - resolve (n) { - return this.parent.resolve(n) - }, - addEdgeOut (edge) { - this.edgesOut.set(edge.name, edge) - }, - addEdgeIn (edge) { - this.edgesIn.add(edge) - }, - deleteEdgeIn (edge) { - this.edgesIn.delete(edge) - }, - } - - const componentA = { - name: 'component-a', - edgesOut: new Map(), - edgesIn: new Set(), - explain: () => 'component-a', - package: { name: 'component-a', version: '1.0.0' }, - get version () { - return this.package.version - }, - parent: root, - root, - overrides: overridesComponents, - resolve (n) { - return this.parent.resolve(n) - }, - addEdgeOut (edge) { - this.edgesOut.set(edge.name, edge) - }, - addEdgeIn (edge) { - this.edgesIn.add(edge) - }, - deleteEdgeIn (edge) { - this.edgesIn.delete(edge) - }, - } - - const componentB = { - name: 'component-b', - edgesOut: new Map(), - edgesIn: new Set(), - explain: () => 'component-b', - package: { name: 'component-b', version: '1.0.0' }, - get version () { - return this.package.version - }, - parent: root, - root, - overrides: overridesComponentsPro, // Different override context - resolve (n) { - return this.parent.resolve(n) - }, - addEdgeOut (edge) { - this.edgesOut.set(edge.name, edge) - }, - addEdgeIn (edge) { - this.edgesIn.add(edge) - }, - deleteEdgeIn (edge) { - this.edgesIn.delete(edge) - }, - } - - // Create two edges with different override contexts pointing to the same node - const edge1 = new Edge({ - from: componentA, - type: 'prod', - spec: '^3.0.0', - name: 'lit', - overrides: overridesComponents.getEdgeRule({ name: 'lit', spec: '^3.0.0' }), - }) - - const edge2 = new Edge({ - from: componentB, - type: 'prod', - spec: '^3.0.0', - name: 'lit', - overrides: overridesComponentsPro.getEdgeRule({ name: 'lit', spec: '^3.0.0' }), - }) - - // Before the fix (b9225e524), edge2 would be marked as INVALID because it has a different - // override context than edge1, and the override conflict check would fail. - // After the fix, both edges should be valid because they both satisfy the version requirement. - // Any actual conflicts in lit's dependencies will be caught during normal dependency resolution. - t.ok(edge1.valid, 'first edge with override context should be valid') - t.ok(edge2.valid, 'second edge with different override context should also be valid') - t.notOk(edge1.error, 'first edge should not have an error') - t.notOk(edge2.error, 'second edge should not have an error') - t.end() -}) diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index abb0380843380..a15b22f614ab5 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -3277,6 +3277,80 @@ t.test('should propagate the new override set to the target node', t => { t.end() }) +t.test('edges with different override contexts to same node should be valid', t => { + // Regression test for issue #8688 + // Replaces the removed test 'should find inconsistency between the edge's override set + // and the target's override set' which was testing override conflict detection that + // was intentionally removed because it caused false positives. + + // Create two different override sets (simulating Vaadin's structure) + const overridesComponents = new OverrideSet({ + overrides: { + '@vaadin/react-components': '24.9.2', + }, + }) + + const overridesComponentsPro = new OverrideSet({ + overrides: { + '@vaadin/react-components-pro': '24.9.2', + }, + }) + + 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: { + 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, simulating that the node satisfies the version + edge.satisfiedBy = () => true + + // Before the fix (b9225e524), if we then changed the target node's override to a different + // set, the edge would be marked INVALID due to override conflict detection. + // After the fix, the edge should remain valid because: + // 1. satisfiedBy returns true (version is satisfied) + // 2. We no longer check for override conflicts at the edge level + 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 + t.equal(edge.error, null, 'Edge should be valid despite different override contexts') + t.ok(edge.valid, 'Edge.valid should be true') + + t.end() +}) + t.test('shouldOmit method', t => { t.test('dev dependency with dev omit', t => { const node = new Node({ From 5c7b5ee144994221f5f8c3d03941f359abbde53c Mon Sep 17 00:00:00 2001 From: Artur Signell Date: Tue, 28 Oct 2025 22:06:45 +0200 Subject: [PATCH 5/5] fix(arborist): improve override conflict detection with semantic comparison Instead of removing override conflict detection entirely, enhance it to use semantic comparison of version requirements rather than pure structural equality checking. --- workspaces/arborist/lib/edge.js | 20 +- workspaces/arborist/lib/override-set.js | 78 +++++++- workspaces/arborist/test/node.js | 238 ++++++++++++++++++----- workspaces/arborist/test/override-set.js | 206 ++++++++++++++++---- 4 files changed, 442 insertions(+), 100 deletions(-) diff --git a/workspaces/arborist/lib/edge.js b/workspaces/arborist/lib/edge.js index f7e941bc56f4c..32e523cbc83ca 100644 --- a/workspaces/arborist/lib/edge.js +++ b/workspaces/arborist/lib/edge.js @@ -4,6 +4,7 @@ const util = require('node:util') const npa = require('npm-package-arg') const depValid = require('./dep-valid.js') +const OverrideSet = require('./override-set.js') class ArboristEdge { constructor (edge) { @@ -274,15 +275,18 @@ class Edge { this.#error = 'PEER LOCAL' } else if (!this.satisfiedBy(this.#to)) { this.#error = 'INVALID' + } else if (this.overrides && this.#to.edgesOut.size && OverrideSet.doOverrideSetsConflict(this.overrides, this.#to.overrides)) { + // 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 { - // Note: We intentionally do NOT check for override conflicts here. - // Different override sets pointing to the same node are allowed as long as: - // 1. The node satisfies the version requirement (checked by satisfiedBy above) - // 2. Any actual version conflicts in the node's dependencies will be caught - // during normal dependency resolution in the build/reify phase - // The previous check (from b9225e524) was overly conservative and produced - // false positives, especially with reference overrides ($syntax) that resolve - // to the same effective versions despite being structurally different. this.#error = 'OK' } } diff --git a/workspaces/arborist/lib/override-set.js b/workspaces/arborist/lib/override-set.js index 3f05609bfacc1..b4a11ba589df7 100644 --- a/workspaces/arborist/lib/override-set.js +++ b/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 } } diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index a15b22f614ab5..91fbfa22af721 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -3277,76 +3277,212 @@ t.test('should propagate the new override set to the target node', t => { t.end() }) -t.test('edges with different override contexts to same node should be valid', t => { - // Regression test for issue #8688 - // Replaces the removed test 'should find inconsistency between the edge's override set - // and the target's override set' which was testing override conflict detection that - // was intentionally removed because it caused false positives. - - // Create two different override sets (simulating Vaadin's structure) - const overridesComponents = new OverrideSet({ - overrides: { - '@vaadin/react-components': '24.9.2', - }, - }) +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: { - '@vaadin/react-components-pro': '24.9.2', - }, + const overridesComponentsPro = new OverrideSet({ + overrides: { + '@vaadin/react-components-pro': '24.9.2', + }, + }) + + 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: { + 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() }) - const tree = new Node({ - loadOverrides: true, - path: '/root', - pkg: { - name: 'root', - version: '1.0.0', - dependencies: { - 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: { - mockDep: '2.x', + lodash: '4.x', }, - }, - 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: { + 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') + t.test('reference overrides should not cause false positives', t => { + // This validates that reference overrides ($syntax) don't trigger false positives - // Manually set an override to the edge - edge.overrides = overridesComponents + const overridesRef1 = new OverrideSet({ + overrides: { + lodash: '$some-reference', + }, + }) - // Override satisfiedBy so it returns true, simulating that the node satisfies the version - edge.satisfiedBy = () => true + 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 - // Before the fix (b9225e524), if we then changed the target node's override to a different - // set, the edge would be marked INVALID due to override conflict detection. - // After the fix, the edge should remain valid because: - // 1. satisfiedBy returns true (version is satisfied) - // 2. We no longer check for override conflicts at the edge level - const mockDep = tree.children.get('mockDep') - mockDep.overrides = overridesComponentsPro + const mockDep = tree.children.get('mockDep') + mockDep.overrides = overridesRef2 - // Force edge to recalculate - edge.reload(true) + // Force edge to recalculate + edge.reload(true) - // The edge should be valid despite different override contexts - t.equal(edge.error, null, 'Edge should be valid despite different override contexts') - t.ok(edge.valid, 'Edge.valid should be 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() }) diff --git a/workspaces/arborist/test/override-set.js b/workspaces/arborist/test/override-set.js index 6acd8c6eecf62..8b8be32c9bb81 100644 --- a/workspaces/arborist/test/override-set.js +++ b/workspaces/arborist/test/override-set.js @@ -378,56 +378,184 @@ t.test('constructor', async (t) => { t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides5), 'override sets are the same object') t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides6), 'one override set is the specific version of the other') t.ok(!OverrideSet.doOverrideSetsConflict(overrides6, overrides5), 'one override set is the specific version of the other') - t.ok(OverrideSet.doOverrideSetsConflict(overrides5, overrides7), 'no override set is the specific version of the other') - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides5), 'no override set is the specific version of the other') + // With semantic conflict detection, overrides5 and overrides7 don't conflict because + // they have the same value for 'bat' (2.0.0). Structurally they're incomparable, + // but semantically they're compatible. + t.ok(!OverrideSet.doOverrideSetsConflict(overrides5, overrides7), 'structurally incomparable but semantically compatible') + t.ok(!OverrideSet.doOverrideSetsConflict(overrides7, overrides5), 'structurally incomparable but semantically compatible') t.ok(!overrides7.isEqual(overrides8), 'two override sets that differ in the version are not equal') t.ok(!overrides8.isEqual(overrides9), 'two override sets that differ in the range are not equal') t.ok(!overrides7.isEqual(overrides9), 'two override sets that differ in both version and range are not equal') - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), 'override sets are incomparable due to version') - t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides9), 'override sets are incomparable due to version and range') - t.ok(OverrideSet.doOverrideSetsConflict(overrides8, overrides9), 'override sets are incomparable due to range') + // overrides7 (bat: 2.0.0) and overrides8 (bat: 1.2.0) have conflicting versions for the same package + t.ok(OverrideSet.doOverrideSetsConflict(overrides7, overrides8), 'override sets have conflicting versions for bat') + // overrides7 (bat: 2.0.0) and overrides9 (bat@3.0.0: 1.2.0) don't directly conflict because + // overrides9 only applies to bat@3.x, not bat@2.x + t.ok(!OverrideSet.doOverrideSetsConflict(overrides7, overrides9), 'override sets apply to different version ranges') + // overrides8 (bat: 1.2.0) and overrides9 (bat@3.0.0: 1.2.0) have same target version + t.ok(!OverrideSet.doOverrideSetsConflict(overrides8, overrides9), 'override sets have compatible target versions') }) -}) -t.test('coverage for final line in isEqual (parent != null)', async t => { - // Both parents have the SAME config -> parent.isEqual(...) will return TRUE - const parentA = new OverrideSet({ overrides: { foo: '1.0.0' } }) - const parentB = new OverrideSet({ overrides: { foo: '1.0.0' } }) + t.test('semantic conflict detection (haveConflictingRules)', async (t) => { + t.test('no conflict when packages are different', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + foo: '1.0.0', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + bar: '1.0.0', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'different packages should not conflict') + }) - // Child override sets with the same parent config => should be equal - const childA = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentA, - }) - const childB = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentB, - }) + t.test('no conflict when version ranges intersect', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '^4.0.0', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.17.0', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'intersecting ranges should not conflict') + }) - // This specifically covers the code path where parent != null - // AND parent.isEqual(...) returns true - t.ok(childA.isEqual(childB), 'two children with equivalent parents are equal') + t.test('conflict when version ranges do not intersect', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '1.x', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.x', + }, + }) + t.ok(OverrideSet.haveConflictingRules(overrides1, overrides2), 'non-intersecting ranges should conflict') + }) - // Different parent configs -> parent.isEqual(...) will return FALSE - const parentC = new OverrideSet({ overrides: { foo: '1.0.0' } }) - const parentD = new OverrideSet({ overrides: { foo: '1.0.1' } }) + t.test('no conflict when using reference overrides', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '$ref1', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '$ref2', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'reference overrides should not conflict') + }) - const childC = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentC, + t.test('no conflict when one is a reference override', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '$ref1', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.17.21', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'reference override with version should not conflict') + }) + + t.test('no conflict when rules are equal', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + lodash: '4.17.21', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + lodash: '4.17.21', + }, + }) + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'equal rules should not conflict') + }) + + t.test('handles non-semver spec types gracefully', async (t) => { + const overrides1 = new OverrideSet({ + overrides: { + foo: 'github:user/repo', + }, + }) + const overrides2 = new OverrideSet({ + overrides: { + foo: 'file:./local', + }, + }) + // Non-semver types should not be considered conflicting + t.ok(!OverrideSet.haveConflictingRules(overrides1, overrides2), 'non-semver specs should not conflict') + }) + + t.test('regression test for issue #8688 - Vaadin case', async (t) => { + const overridesComponents = new OverrideSet({ + overrides: { + '@vaadin/react-components': '24.9.2', + }, + }) + const overridesComponentsPro = new OverrideSet({ + overrides: { + '@vaadin/react-components-pro': '24.9.2', + }, + }) + t.ok(!OverrideSet.doOverrideSetsConflict(overridesComponents, overridesComponentsPro), 'Vaadin components should not conflict') + }) }) - const childD = new OverrideSet({ - overrides: { bar: '2.0.0' }, - key: 'bar', - parent: parentD, +}) + +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' } }) + t.ok(!overrides.isEqual(null), 'override set is not equal to null') + t.ok(!overrides.isEqual(undefined), 'override set is not equal to undefined') }) - // This specifically covers the code path where parent != null - // AND parent.isEqual(...) returns false - t.notOk(childC.isEqual(childD), 'two children with different parents are not equal') + t.test('isEqual when parent != null', async t => { + // Both parents have the SAME config -> parent.isEqual(...) will return TRUE + const parentA = new OverrideSet({ overrides: { foo: '1.0.0' } }) + const parentB = new OverrideSet({ overrides: { foo: '1.0.0' } }) - t.end() + // Child override sets with the same parent config => should be equal + const childA = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentA, + }) + const childB = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentB, + }) + + // This specifically covers the code path where parent != null + // AND parent.isEqual(...) returns true + t.ok(childA.isEqual(childB), 'two children with equivalent parents are equal') + + // Different parent configs -> parent.isEqual(...) will return FALSE + const parentC = new OverrideSet({ overrides: { foo: '1.0.0' } }) + const parentD = new OverrideSet({ overrides: { foo: '1.0.1' } }) + + const childC = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentC, + }) + const childD = new OverrideSet({ + overrides: { bar: '2.0.0' }, + key: 'bar', + parent: parentD, + }) + + // This specifically covers the code path where parent != null + // AND parent.isEqual(...) returns false + t.notOk(childC.isEqual(childD), 'two children with different parents are not equal') + }) })