Skip to content

fix(arborist): validate peerOptional conflicts in no-save mutations #9641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
owlstronaut merged 1 commit into from
Jun 24, 2026
+231 −21
Merged

fix(arborist): validate peerOptional conflicts in no-save mutations #9641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 24 additions & 8 deletions workspaces/arborist/lib/arborist/build-ideal-tree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
#loadFailures = new Set()
#manifests = new Map()
#mutateTree = false
#requestedTreeMutation = false
// a map of each module in a peer set to the thing that depended on
// that set of peers in the first place. Use a WeakMap so that we
// don't hold onto references for nodes that are garbage collected.
Expand Down Expand Up @@ -261,12 +262,13 @@ module.exports = cls => class IdealTreeBuilder extends cls {

// set if we add anything, but also set here if we know we'll make
// changes and thus have to maybe prune later.
this.#mutateTree = !!(
this.#requestedTreeMutation = !!(
options.add ||
options.rm ||
update.all ||
update.names.length
)
this.#mutateTree = this.#requestedTreeMutation
}

// load the initial tree, either the virtualTree from a shrinkwrap,
Expand Down Expand Up @@ -344,7 +346,9 @@ module.exports = cls => class IdealTreeBuilder extends cls {
filter: node => node,
visit: node => {
for (const edge of node.edgesOut.values()) {
const skipPeerOptional = edge.type === 'peerOptional' && this.options.save === false
const skipPeerOptional = edge.type === 'peerOptional' &&
this.options.save === false &&
!this.#requestedTreeMutation
if (!skipPeerOptional && (!edge.to || !edge.valid)) {
this.#depsQueue.push(node)
break // no need to continue the loop after the first hit
Expand Down Expand Up @@ -1047,9 +1051,16 @@ This is a one-time fix-up, please be patient...
}
const { from, valid, peerConflicted } = edgeIn
if (!peerConflicted && !valid) {
if (this.#depsSeen.has(from) && this.options.save) {
// Re-queue already-processed nodes when a newly placed dep creates an invalid edge during npm install (save=true).
// This handles the case where a peerOptional dep was valid (missing) when the node was first processed, but becomes invalid when the dep is later placed by another path with a version that doesn't satisfy the peer spec.
if (this.#depsSeen.has(from) &&
(this.options.save ||
(this.options.save === false && this.#requestedTreeMutation))) {
// Re-queue already-processed nodes when a newly placed dep
// creates an invalid edge during npm install or another
// lockfile-mutating operation. This handles the case where a
// peerOptional dep was valid (missing) when the node was
// first processed, but becomes invalid when the dep is later
// placed by another path with a version that doesn't satisfy
// the peer spec.
// See npm/cli#8726.
this.#depsSeen.delete(from)
this.#depsQueue.push(from)
Expand Down Expand Up @@ -1260,11 +1271,16 @@ This is a one-time fix-up, please be patient...
continue
}

// If the edge has an error, there's a problem, unless it's peerOptional and we're not saving (e.g. npm ci), in which case we trust the lockfile and skip re-resolution.
// When saving (npm install), peerOptional invalid edges ARE treated as problems so the lockfile gets fixed.
// If the edge has an error, there's a problem, unless it's peerOptional
// and we're not saving or otherwise mutating (e.g. npm ci), in which
// case we trust the lockfile and skip re-resolution. When saving (npm
// install) or updating, peerOptional invalid edges ARE treated as
// problems so the lockfile gets fixed.
// See npm/cli#8726.
if (!edge.valid) {
if (edge.type !== 'peerOptional' || this.options.save !== false) {
if (edge.type !== 'peerOptional' ||
this.options.save !== false ||
this.#requestedTreeMutation) {
problems.push(edge)
}
continue
Expand Down
220 changes: 207 additions & 13 deletions workspaces/arborist/test/arborist/build-ideal-tree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4631,14 +4631,53 @@ t.test('re-queue already-seen nodes when placed dep invalidates peerOptional (sa
t.ok(tree.children.get('shared'), 'shared is in the tree')
})

t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)', async t => {
// With save=false (npm ci behavior), invalid peerOptional edges should NOT be treated as problems.
// We use update.names to force alpha into #problemEdges while shared@1.1.0 (invalid for alpha's peerOptional spec of 1.0.0) is already in the tree from the lockfile.
t.test('re-queue already-seen nodes when save=false update invalidates peerOptional', async t => {
const registry = createRegistry(t, false)

const utilPacks = registry.packuments(['1.0.0', '1.0.1'], 'util')
const utilManifest = registry.manifest({ name: 'util', packuments: utilPacks })
await registry.package({ manifest: utilManifest })
const alphaPack = registry.packument({
name: 'alpha',
version: '1.0.0',
peerDependencies: { shared: '1.0.0' },
peerDependenciesMeta: { shared: { optional: true } },
})
const alphaManifest = registry.manifest({ name: 'alpha', packuments: [alphaPack] })
await registry.package({ manifest: alphaManifest })

const betaPack = registry.packument({
name: 'beta',
version: '1.0.0',
dependencies: { shared: '^1.0.0' },
})
const betaManifest = registry.manifest({ name: 'beta', packuments: [betaPack] })
await registry.package({ manifest: betaManifest })

const sharedPacks = registry.packuments(['1.0.0', '1.1.0'], 'shared')
const sharedManifest = registry.manifest({ name: 'shared', packuments: sharedPacks })
await registry.package({ manifest: sharedManifest, times: 2 })

const path = t.testdir({
'package.json': JSON.stringify({
name: 'test-8726-save-false-update',
version: '1.0.0',
dependencies: {
alpha: '1.0.0',
beta: '1.0.0',
},
}),
})

const arb = newArb(path, { save: false })
const tree = await arb.buildIdealTree({ update: true })

t.ok(tree.children.get('alpha'), 'alpha is in the tree')
t.ok(tree.children.get('beta'), 'beta is in the tree')
t.ok(tree.children.get('shared'), 'shared is in the tree')
})

t.test('skip invalid peerOptional edges when save=false and not mutating (#8726)', async t => {
// With save=false and no tree mutation (npm ci behavior), invalid peerOptional
// edges should NOT be treated as problems. The locked tree is trusted as-is.
createRegistry(t, false)

const path = t.testdir({
'package.json': JSON.stringify({
Expand All @@ -4663,7 +4702,6 @@ t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)'
'node_modules/alpha': {
version: '1.0.0',
resolved: 'https://registry.npmjs.org/alpha/-/alpha-1.0.0.tgz',
dependencies: { util: '^1.0.0' },
peerDependencies: { shared: '1.0.0' },
peerDependenciesMeta: { shared: { optional: true } },
},
Expand All @@ -4676,22 +4714,178 @@ t.test('skip invalid peerOptional edges in problemEdges when save=false (#8726)'
version: '1.1.0',
resolved: 'https://registry.npmjs.org/shared/-/shared-1.1.0.tgz',
},
'node_modules/util': {
version: '1.0.0',
resolved: 'https://registry.npmjs.org/util/-/util-1.0.0.tgz',
},
},
}),
})

const arb = newArb(path, { save: false })
const tree = await arb.buildIdealTree({ update: { names: ['util'] } })
const tree = await arb.buildIdealTree()

t.ok(tree.children.get('alpha'), 'alpha is in the tree')
t.ok(tree.children.get('beta'), 'beta is in the tree')
t.equal(tree.children.get('shared').version, '1.1.0',
'shared stays at 1.1.0 - peerOptional mismatch is not treated as a problem')
t.ok(tree.children.get('util'), 'util is in the tree')
})

t.test('save=false non-mutating build ignores invalid peerOptional problem edges', async t => {
const registry = createRegistry(t, false)

const gammaPack = registry.packument({
name: 'gamma',
version: '1.0.0',
})
const gammaManifest = registry.manifest({ name: 'gamma', packuments: [gammaPack] })
await registry.package({ manifest: gammaManifest })

const path = t.testdir({
'package.json': JSON.stringify({
name: 'test-8726-save-false-non-mutating-problem-edges',
version: '1.0.0',
dependencies: {
alpha: '1.0.0',
},
}),
'package-lock.json': JSON.stringify({
name: 'test-8726-save-false-non-mutating-problem-edges',
version: '1.0.0',
lockfileVersion: 3,
requires: true,
packages: {
'': {
name: 'test-8726-save-false-non-mutating-problem-edges',
version: '1.0.0',
dependencies: { alpha: '1.0.0' },
},
'node_modules/alpha': {
version: '1.0.0',
resolved: 'https://registry.npmjs.org/alpha/-/alpha-1.0.0.tgz',
dependencies: { gamma: '1.0.0' },
peerDependencies: { shared: '1.0.0' },
peerDependenciesMeta: { shared: { optional: true } },
},
'node_modules/shared': {
version: '1.1.0',
resolved: 'https://registry.npmjs.org/shared/-/shared-1.1.0.tgz',
},
},
}),
})

const arb = newArb(path, { save: false })
const tree = await arb.buildIdealTree()

t.ok(tree.children.get('gamma'), 'missing non-peer dependency is resolved')
})

t.test('update does not leave invalid peerOptional edges when save=false', async t => {
// npm update defaults to save=false because it should not rewrite package.json
// ranges. It still mutates and writes the lockfile, so it cannot use the
// npm-ci behavior of trusting invalid peerOptional edges in the locked tree.
const registry = createRegistry(t, false)

const alphaPacks = [
registry.packument({ name: 'alpha', version: '1.0.0' }),
registry.packument({
name: 'alpha',
version: '1.1.0',
peerDependencies: { shared: '^1.0.0' },
peerDependenciesMeta: { shared: { optional: true } },
}),
]
const alphaManifest = registry.manifest({ name: 'alpha', packuments: alphaPacks })
await registry.package({ manifest: alphaManifest })

const sharedPacks = registry.packuments(['1.0.0', '2.0.0', '2.0.1'], 'shared')
const sharedManifest = registry.manifest({ name: 'shared', packuments: sharedPacks })
await registry.package({ manifest: sharedManifest, times: 2 })

const path = t.testdir({
'package.json': JSON.stringify({
name: 'test-peer-optional-update',
version: '1.0.0',
dependencies: {
alpha: '^1.0.0',
shared: '^2.0.0',
},
}),
'package-lock.json': JSON.stringify({
name: 'test-peer-optional-update',
version: '1.0.0',
lockfileVersion: 3,
requires: true,
packages: {
'': {
name: 'test-peer-optional-update',
version: '1.0.0',
dependencies: { alpha: '^1.0.0', shared: '^2.0.0' },
},
'node_modules/alpha': {
version: '1.0.0',
resolved: 'https://registry.npmjs.org/alpha/-/alpha-1.0.0.tgz',
},
'node_modules/shared': {
version: '2.0.0',
resolved: 'https://registry.npmjs.org/shared/-/shared-2.0.0.tgz',
},
},
}),
})

const arb = newArb(path, { save: false })
await t.rejects(arb.buildIdealTree({ update: true }), {
code: 'ERESOLVE',
}, 'lockfile-mutating update should reject instead of keeping an invalid peerOptional edge')
})

t.test('add does not leave invalid peerOptional edges when save=false', async t => {
// npm install <pkg> --no-save also mutates and writes the lockfile without
// changing package.json. It should not use npm-ci behavior either.
const registry = createRegistry(t, false)

const alphaPack = registry.packument({
name: 'alpha',
version: '1.0.0',
peerDependencies: { shared: '^1.0.0' },
peerDependenciesMeta: { shared: { optional: true } },
})
const alphaManifest = registry.manifest({ name: 'alpha', packuments: [alphaPack] })
await registry.package({ manifest: alphaManifest })

const sharedPacks = registry.packuments(['1.0.0', '2.0.0'], 'shared')
const sharedManifest = registry.manifest({ name: 'shared', packuments: sharedPacks })
await registry.package({ manifest: sharedManifest, times: 2 })

const path = t.testdir({
'package.json': JSON.stringify({
name: 'test-peer-optional-add',
version: '1.0.0',
dependencies: {
shared: '^2.0.0',
},
}),
'package-lock.json': JSON.stringify({
name: 'test-peer-optional-add',
version: '1.0.0',
lockfileVersion: 3,
requires: true,
packages: {
'': {
name: 'test-peer-optional-add',
version: '1.0.0',
dependencies: { shared: '^2.0.0' },
},
'node_modules/shared': {
version: '2.0.0',
resolved: 'https://registry.npmjs.org/shared/-/shared-2.0.0.tgz',
},
},
}),
})

const arb = newArb(path, { save: false })
await t.rejects(arb.buildIdealTree({ add: ['alpha@1.0.0'] }), {
code: 'ERESOLVE',
}, 'lockfile-mutating add should reject instead of keeping an invalid peerOptional edge')
})

t.test('peerOptional prefers existing tree node over registry fetch (#9249)', async t => {
Expand Down
Loading