Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions workspaces/arborist/lib/arborist/load-actual.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ module.exports = cls => class ActualLoader extends cls {

this.#transplant(root)

this.#repropagateOverrides()

if (global) {
// need to depend on the children, or else all of them
// will end up being flagged as extraneous, since the
Expand Down Expand Up @@ -389,6 +391,18 @@ module.exports = cls => class ActualLoader extends cls {
}
}

// Re-forward overrides through links after the tree is complete, since a store Link may forward before its subtree resolves and miss a transitive match (npm/cli#9619).
#repropagateOverrides () {
if (!this.#actualTree.overrides) {
return
}
for (const node of this.#actualTree.inventory.values()) {
if (node.isLink && node.overrides) {
node.recalculateOutEdgesOverrides()
}
}
}

// .npm-extension transformManifest, like packageExtensions, never rewrites a package's package.json, so re-derive its edges and provenance on a filesystem-scanned actual tree.
// This executes the root extension code; ignore-extension (and ignore-scripts via flatten) disables it.
async #applyNpmExtension () {
Expand Down
38 changes: 27 additions & 11 deletions workspaces/arborist/lib/link.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,21 +109,12 @@ class Link extends Node {
// so this is a no-op
[_loadDeps] () {}

// When a Link receives overrides (via edgesIn), forward them to the target node which holds the actual edgesOut — but only when the OverrideSet has at least one rule that names a dep the target actually depends on.
// Without this scope, the link forwards a generic ancestor OverrideSet that has no real effect on the target's edges, but still flips the target to "has overrides", which changes downstream `canReplaceWith` / placement decisions and causes `npm ci` to re-resolve lockfile-pinned edges from the registry.
// See npm/cli#9357.
// Forward overrides to the target only when a rule names a dep it needs, directly or transitively (npm/cli#9357, #9619).
recalculateOutEdgesOverrides () {
if (!this.target || !this.overrides) {
return
}
let hasMatchingRule = false
for (const rule of this.overrides.ruleset.values()) {
if (this.target.edgesOut.has(rule.name)) {
hasMatchingRule = true
break
}
}
if (!hasMatchingRule) {
if (!overrideMatchesSubtree(this.overrides, this.target)) {
return
}
this.target.updateOverridesEdgeInAdded(this.overrides)
Expand All @@ -143,4 +134,29 @@ class Link extends Node {
}
}

// True when an override rule applies to an edge reachable from the target at any depth.
// getEdgeRule matches on name and spec, returning the set itself when nothing applies, so a non-applicable version-qualified rule doesn't flip an intermediate node to "has overrides" (npm/cli#9357).
// Not a method: runs from the Node super-constructor, before subclass private methods exist.
const overrideMatchesSubtree = (overrides, target) => {
const seen = new Set()
const stack = [target]
while (stack.length) {
const node = stack.pop()
const resolved = node.isLink ? node.target : node
if (seen.has(resolved)) {
continue
}
seen.add(resolved)
for (const edge of resolved.edgesOut.values()) {
if (overrides.getEdgeRule(edge).name === edge.name) {
return true
}
if (edge.to) {
stack.push(edge.to)
}
}
}
return false
}

module.exports = Link
66 changes: 66 additions & 0 deletions workspaces/arborist/test/arborist/load-actual.js
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,72 @@ t.test('applies root packageExtensions to a linked actual tree', async t => {
t.strictSame(brokenLink.packageExtensionsApplied, applied, 'provenance mirrored onto the link')
})

t.test('forwards a transitive override through a linked store link — npm/cli#9619', async t => {
// The override must propagate through the intermediate store Link whose own direct deps don't name the overridden package, or `npm ls` reports the edge `invalid` instead of `overridden`.
// Shaped like glob -> minimatch -> brace-expansion: the override target sits two links deep, with a shared node reached by two paths.
const path = t.testdir({
'package.json': JSON.stringify({
name: 'root',
version: '1.0.0',
dependencies: { a: '1.0.0' },
overrides: { leaf: '2.0.0' },
}),
node_modules: {
a: t.fixture('symlink', '.store/a@1.0.0/node_modules/a'),
'.store': {
'a@1.0.0': {
node_modules: {
a: { 'package.json': JSON.stringify({ name: 'a', version: '1.0.0', dependencies: { b: '1.0.0', c: '1.0.0' } }) },
b: t.fixture('symlink', '../../b@1.0.0/node_modules/b'),
c: t.fixture('symlink', '../../c@1.0.0/node_modules/c'),
},
},
'b@1.0.0': {
node_modules: {
// leaf is declared ^1.0.0 but overridden to 2.0.0, which is outside that range
b: { 'package.json': JSON.stringify({ name: 'b', version: '1.0.0', dependencies: { leaf: '^1.0.0', shared: '1.0.0' } }) },
leaf: t.fixture('symlink', '../../leaf@2.0.0/node_modules/leaf'),
shared: t.fixture('symlink', '../../shared@1.0.0/node_modules/shared'),
},
},
'c@1.0.0': {
node_modules: {
// c's subtree has no overridden package but reaches shared via both c and c->d, so its walk revisits a seen node
c: { 'package.json': JSON.stringify({ name: 'c', version: '1.0.0', dependencies: { d: '1.0.0', shared: '1.0.0' } }) },
d: t.fixture('symlink', '../../d@1.0.0/node_modules/d'),
shared: t.fixture('symlink', '../../shared@1.0.0/node_modules/shared'),
},
},
'd@1.0.0': {
node_modules: {
d: { 'package.json': JSON.stringify({ name: 'd', version: '1.0.0', dependencies: { shared: '1.0.0' } }) },
shared: t.fixture('symlink', '../../shared@1.0.0/node_modules/shared'),
},
},
'leaf@2.0.0': {
node_modules: {
leaf: { 'package.json': JSON.stringify({ name: 'leaf', version: '2.0.0' }) },
},
},
'shared@1.0.0': {
node_modules: {
shared: { 'package.json': JSON.stringify({ name: 'shared', version: '1.0.0' }) },
},
},
},
},
})

const tree = await loadActual(path)
const b = tree.children.get('a').target.edgesOut.get('b').to.target
const leafEdge = b.edgesOut.get('leaf')
t.ok(leafEdge && !leafEdge.error, 'transitive overridden edge resolves without error')
t.ok(leafEdge.overrides, 'edge carries the override rule')
t.equal(leafEdge.spec, '2.0.0', 'edge spec is the overridden version')
t.equal(leafEdge.rawSpec, '^1.0.0', 'edge rawSpec is the original declared range')
t.equal(leafEdge.to.target.version, '2.0.0', 'edge resolves to the overridden package')
})

t.test('store nodes do not load devDependencies as required edges', async t => {
// A package in the linked store is structurally a tree top, so without the isInStore guard its devDependencies would load as required edges and surface as missing (e.g. npm sbom ESBOMPROBLEMS).
const path = t.testdir({
Expand Down
38 changes: 38 additions & 0 deletions workspaces/arborist/test/link.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,44 @@ t.test('recalculateOutEdgesOverrides does not forward when no rule matches a tar
t.end()
})

t.test('recalculateOutEdgesOverrides does not forward a version-qualified rule that does not apply', t => {
// The rule names the target's dep but its keySpec (^3) does not intersect the declared range (^1), so the override does not apply and must not flip the target to "has overrides".
const root = new Node({
path: '/path/to/root',
pkg: {
name: 'root',
dependencies: { foo: '1.0.0' },
overrides: { 'leaf@^3': '3.0.0' },
},
loadOverrides: true,
})

const target = new Node({
path: '/path/to/store/foo',
pkg: {
name: 'foo',
version: '1.0.0',
dependencies: { leaf: '^1.0.0' },
},
root,
})

// eslint-disable-next-line no-new
new Link({
pkg: { name: 'foo', version: '1.0.0' },
path: '/path/to/root/node_modules/foo',
realpath: '/path/to/store/foo',
target,
parent: root,
})

t.notOk(target.overrides, 'target.overrides stays undefined for a non-applicable rule')
const leafEdge = target.edgesOut.get('leaf')
t.notOk(leafEdge.overrides, 'edge keeps edge.overrides undefined')
t.equal(leafEdge.spec, '^1.0.0', 'edge spec is unchanged')
t.end()
})

t.test('link to root path gets root as target', t => {
const root = new Node({
path: '/project/root',
Expand Down
Loading