diff --git a/workspaces/arborist/lib/arborist/load-actual.js b/workspaces/arborist/lib/arborist/load-actual.js index d6615caa42fb2..3fdd007563939 100644 --- a/workspaces/arborist/lib/arborist/load-actual.js +++ b/workspaces/arborist/lib/arborist/load-actual.js @@ -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 @@ -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 () { diff --git a/workspaces/arborist/lib/link.js b/workspaces/arborist/lib/link.js index 3824dc81ffb3c..89f993bb16900 100644 --- a/workspaces/arborist/lib/link.js +++ b/workspaces/arborist/lib/link.js @@ -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) @@ -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 diff --git a/workspaces/arborist/test/arborist/load-actual.js b/workspaces/arborist/test/arborist/load-actual.js index 9d724e934fdca..4b40e6276b884 100644 --- a/workspaces/arborist/test/arborist/load-actual.js +++ b/workspaces/arborist/test/arborist/load-actual.js @@ -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({ diff --git a/workspaces/arborist/test/link.js b/workspaces/arborist/test/link.js index 472f33c2f4535..8532de7d6f39b 100644 --- a/workspaces/arborist/test/link.js +++ b/workspaces/arborist/test/link.js @@ -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',