diff --git a/test/lib/commands/ci.js b/test/lib/commands/ci.js index 8dc0f1d3cc149..6db01d7b6ec7a 100644 --- a/test/lib/commands/ci.js +++ b/test/lib/commands/ci.js @@ -308,3 +308,142 @@ t.test('should use --workspace flag', async t => { assert.packageMissing('node_modules/abbrev@1.1.0') assert.packageInstalled('node_modules/lodash@1.1.1') }) + +// Issue #8726 - npm ci fails because npm install produces an out-of-sync lockfile +// https://github.com/npm/cli/issues/8726 +// +// Root cause: an optional peerDependency at an exact version causes buildIdealTree() to resolve a different version than what's in the lockfile. +// +// Pattern (mirrors real-world addons-linter / htmlhint / node-fetch scenario): +// - scanner@1.0.0 has optional peerDep: fetcher@1.0.0 (exact version) +// - hint@1.0.0 has regular dep: fetcher@^1.0.0 (semver range) +// - npm install resolves fetcher to 1.1.0 (latest matching ^1.0.0) +// - npm ci's buildIdealTree sees fetcher@1.1.0 doesn't satisfy the exact peerDep "1.0.0", treats it as a problem edge, resolves fetcher to 1.0.0 +// - validateLockfile: lockfile has 1.1.0, ideal tree has 1.0.0 → MISMATCH + +t.test('issue-8726: npm ci with optional peerDep causing lockfile version mismatch', async t => { + // Pre-built lockfile locks fetcher@1.1.0 (what npm install would pick). + // scanner has optional peerDep fetcher@1.0.0 (exact version). + // buildIdealTree should respect the lockfile version, but the bug causes it to resolve fetcher to 1.0.0, failing validateLockfile. + const { npm, registry } = await loadMockNpm(t, { + config: { audit: false, 'ignore-scripts': true }, + strictRegistryNock: false, + prefixDir: { + 'linter-tarball': { + 'package.json': JSON.stringify({ + name: 'linter', + version: '1.0.0', + dependencies: { scanner: '1.0.0' }, + }), + }, + 'scanner-tarball': { + 'package.json': JSON.stringify({ + name: 'scanner', + version: '1.0.0', + peerDependencies: { fetcher: '1.0.0' }, + peerDependenciesMeta: { fetcher: { optional: true } }, + }), + }, + 'hint-tarball': { + 'package.json': JSON.stringify({ + name: 'hint', + version: '1.0.0', + dependencies: { fetcher: '^1.0.0' }, + }), + }, + 'fetcher-1.0.0-tarball': { + 'package.json': JSON.stringify({ name: 'fetcher', version: '1.0.0' }), + }, + 'fetcher-1.1.0-tarball': { + 'package.json': JSON.stringify({ name: 'fetcher', version: '1.1.0' }), + }, + 'package.json': JSON.stringify({ + name: 'test-package', + version: '1.0.0', + devDependencies: { + linter: '1.0.0', + hint: '1.0.0', + }, + }), + 'package-lock.json': JSON.stringify({ + name: 'test-package', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'test-package', + version: '1.0.0', + devDependencies: { linter: '1.0.0', hint: '1.0.0' }, + }, + 'node_modules/linter': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/linter/-/linter-1.0.0.tgz', + dev: true, + dependencies: { scanner: '1.0.0' }, + }, + 'node_modules/scanner': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/scanner/-/scanner-1.0.0.tgz', + dev: true, + peerDependencies: { fetcher: '1.0.0' }, + peerDependenciesMeta: { fetcher: { optional: true } }, + }, + 'node_modules/hint': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/hint/-/hint-1.0.0.tgz', + dev: true, + dependencies: { fetcher: '^1.0.0' }, + }, + 'node_modules/fetcher': { + version: '1.1.0', + resolved: 'https://registry.npmjs.org/fetcher/-/fetcher-1.1.0.tgz', + dev: true, + }, + }, + }), + }, + }) + + // With the fix, buildIdealTree no longer treats the invalid peerOptional edge as a problem, so npm ci proceeds to reification and needs tarballs. + const linterManifest = registry.manifest({ name: 'linter' }) + await registry.tarball({ + manifest: linterManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'linter-tarball'), + }) + + const scannerManifest = registry.manifest({ name: 'scanner' }) + await registry.tarball({ + manifest: scannerManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'scanner-tarball'), + }) + + const hintManifest = registry.manifest({ name: 'hint' }) + await registry.tarball({ + manifest: hintManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'hint-tarball'), + }) + + const fetcherManifest = registry.manifest({ + name: 'fetcher', + versions: ['1.0.0', '1.1.0'], + }) + await registry.tarball({ + manifest: fetcherManifest.versions['1.1.0'], + tarball: path.join(npm.prefix, 'fetcher-1.1.0-tarball'), + }) + + // npm ci should succeed - the lockfile is valid and the fix ensures the peerOptional edge doesn't cause a version mismatch. + await npm.exec('ci', []) + + const installedFetcher = JSON.parse( + fs.readFileSync( + path.join(npm.prefix, 'node_modules', 'fetcher', 'package.json'), 'utf8' + ) + ) + t.equal( + installedFetcher.version, + '1.1.0', + 'installed the locked fetcher version, not the peer dep version' + ) +}) diff --git a/test/lib/commands/install.js b/test/lib/commands/install.js index 7fd5eb7f16100..584690b68b5c6 100644 --- a/test/lib/commands/install.js +++ b/test/lib/commands/install.js @@ -1,3 +1,4 @@ +const fs = require('node:fs') const tspawk = require('../../fixtures/tspawk') const { cleanCwd, @@ -770,3 +771,272 @@ t.test('devEngines', async t => { t.ok(!output.includes('EBADDEVENGINES')) }) }) + +// Issue #8726 - npm install should re-resolve to satisfy peerOptional constraints +// https://github.com/npm/cli/issues/8726 +// +// When a lockfile has fetcher@1.1.0 but a peerOptional wants fetcher@1.0.0 (exact), npm install (save: true) should re-resolve fetcher to 1.0.0 to satisfy both the regular dep range (^1.0.0) and the exact peerOptional constraint. +t.test('issue-8726: npm install re-resolves to satisfy peerOptional constraint', async t => { + const { npm, registry } = await loadMockNpm(t, { + config: { audit: false, 'ignore-scripts': true }, + prefixDir: { + 'linter-tarball': { + 'package.json': JSON.stringify({ + name: 'linter', + version: '1.0.0', + dependencies: { scanner: '1.0.0' }, + }), + }, + 'scanner-tarball': { + 'package.json': JSON.stringify({ + name: 'scanner', + version: '1.0.0', + peerDependencies: { fetcher: '1.0.0' }, + peerDependenciesMeta: { fetcher: { optional: true } }, + }), + }, + 'hint-tarball': { + 'package.json': JSON.stringify({ + name: 'hint', + version: '1.0.0', + dependencies: { fetcher: '^1.0.0' }, + }), + }, + 'fetcher-1.0.0-tarball': { + 'package.json': JSON.stringify({ name: 'fetcher', version: '1.0.0' }), + }, + 'fetcher-1.1.0-tarball': { + 'package.json': JSON.stringify({ name: 'fetcher', version: '1.1.0' }), + }, + 'package.json': JSON.stringify({ + name: 'test-package', + version: '1.0.0', + devDependencies: { + linter: '1.0.0', + hint: '1.0.0', + }, + }), + 'package-lock.json': JSON.stringify({ + name: 'test-package', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'test-package', + version: '1.0.0', + devDependencies: { linter: '1.0.0', hint: '1.0.0' }, + }, + 'node_modules/linter': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/linter/-/linter-1.0.0.tgz', + dev: true, + dependencies: { scanner: '1.0.0' }, + }, + 'node_modules/scanner': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/scanner/-/scanner-1.0.0.tgz', + dev: true, + peerDependencies: { fetcher: '1.0.0' }, + peerDependenciesMeta: { fetcher: { optional: true } }, + }, + 'node_modules/hint': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/hint/-/hint-1.0.0.tgz', + dev: true, + dependencies: { fetcher: '^1.0.0' }, + }, + 'node_modules/fetcher': { + version: '1.1.0', + resolved: 'https://registry.npmjs.org/fetcher/-/fetcher-1.1.0.tgz', + dev: true, + }, + }, + }), + }, + }) + + // Only set up mocks that npm install actually needs: tarballs for all installed packages (linter, scanner, hint, fetcher@1.0.0) and the fetcher packument (needed for re-resolution via #problemEdges). + // Packuments for linter/scanner/hint are NOT needed (already in lockfile). + // Fetcher@1.1.0 tarball is NOT needed (gets replaced by 1.0.0). + const linterManifest = registry.manifest({ name: 'linter' }) + await registry.tarball({ + manifest: linterManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'linter-tarball'), + }) + + const scannerManifest = registry.manifest({ name: 'scanner' }) + await registry.tarball({ + manifest: scannerManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'scanner-tarball'), + }) + + const hintManifest = registry.manifest({ name: 'hint' }) + await registry.tarball({ + manifest: hintManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'hint-tarball'), + }) + + const fetcherManifest = registry.manifest({ + name: 'fetcher', + versions: ['1.0.0', '1.1.0'], + }) + await registry.package({ manifest: fetcherManifest }) + await registry.tarball({ + manifest: fetcherManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'fetcher-1.0.0-tarball'), + }) + + await npm.exec('install', []) + + // Read the updated lockfile and verify fetcher was re-resolved to 1.0.0 + const lockfile = JSON.parse( + fs.readFileSync(path.join(npm.prefix, 'package-lock.json'), 'utf8') + ) + t.equal( + lockfile.packages['node_modules/fetcher'].version, + '1.0.0', + 'lockfile updated fetcher to satisfy peerOptional constraint' + ) + + // Also verify the installed package + const installedFetcher = JSON.parse( + fs.readFileSync( + path.join(npm.prefix, 'node_modules', 'fetcher', 'package.json'), 'utf8' + ) + ) + t.equal( + installedFetcher.version, + '1.0.0', + 'installed fetcher version satisfies peerOptional constraint' + ) +}) + +// Issue #8726 - fresh npm install (no lockfile) should pick a version that satisfies both the regular dep range AND the exact peerOptional constraint, even when the peerOptional holder is processed BEFORE the dep is placed. +// https://github.com/npm/cli/issues/8726 +// +// This test uses package names that reproduce the real-world alphabetical ordering from the original issue (addons-linter < htmlhint), which causes addons-scanner to be processed from the queue BEFORE htmlhint places node-fetcher. +// At that point the peerOptional edge has no destination (MISSING, valid for peerOptional). +// Later, htmlhint places node-fetcher@1.1.0 and the edge becomes INVALID. +// The fix re-queues addons-scanner so #problemEdges can trigger re-resolution of node-fetcher to 1.0.0. +// +// Dependency graph: +// root -> addons-linter@1.0.0 -> addons-scanner@1.0.0 -> PEER_OPTIONAL node-fetcher@1.0.0 +// root -> htmlhint@1.0.0 -> node-fetcher@^1.0.0 +// +// Processing order (alphabetical): addons-linter, then addons-scanner (dep of addons-linter), THEN htmlhint (which places node-fetcher@1.1.0) +t.test('issue-8726: fresh install re-queues scanner when dep placed later', async t => { + const { npm, registry } = await loadMockNpm(t, { + config: { audit: false, 'ignore-scripts': true }, + prefixDir: { + 'addons-linter-tarball': { + 'package.json': JSON.stringify({ + name: 'addons-linter', + version: '1.0.0', + dependencies: { 'addons-scanner': '1.0.0' }, + }), + }, + 'addons-scanner-tarball': { + 'package.json': JSON.stringify({ + name: 'addons-scanner', + version: '1.0.0', + peerDependencies: { 'node-fetcher': '1.0.0' }, + peerDependenciesMeta: { 'node-fetcher': { optional: true } }, + }), + }, + 'htmlhint-tarball': { + 'package.json': JSON.stringify({ + name: 'htmlhint', + version: '1.0.0', + dependencies: { 'node-fetcher': '^1.0.0' }, + }), + }, + 'node-fetcher-1.0.0-tarball': { + 'package.json': JSON.stringify({ name: 'node-fetcher', version: '1.0.0' }), + }, + 'node-fetcher-1.1.0-tarball': { + 'package.json': JSON.stringify({ name: 'node-fetcher', version: '1.1.0' }), + }, + 'package.json': JSON.stringify({ + name: 'test-package', + version: '1.0.0', + devDependencies: { + 'addons-linter': '1.0.0', + htmlhint: '1.0.0', + }, + }), + // NO package-lock.json — this is a fresh install + }, + }) + + // Fresh install needs packuments for all packages + const linterManifest = registry.manifest({ + name: 'addons-linter', + packuments: [{ version: '1.0.0', dependencies: { 'addons-scanner': '1.0.0' } }], + }) + await registry.package({ manifest: linterManifest }) + await registry.tarball({ + manifest: linterManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'addons-linter-tarball'), + }) + + const scannerManifest = registry.manifest({ + name: 'addons-scanner', + packuments: [{ + version: '1.0.0', + peerDependencies: { 'node-fetcher': '1.0.0' }, + peerDependenciesMeta: { 'node-fetcher': { optional: true } }, + }], + }) + await registry.package({ manifest: scannerManifest }) + await registry.tarball({ + manifest: scannerManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'addons-scanner-tarball'), + }) + + const hintManifest = registry.manifest({ + name: 'htmlhint', + packuments: [{ version: '1.0.0', dependencies: { 'node-fetcher': '^1.0.0' } }], + }) + await registry.package({ manifest: hintManifest }) + await registry.tarball({ + manifest: hintManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'htmlhint-tarball'), + }) + + const fetcherManifest = registry.manifest({ + name: 'node-fetcher', + packuments: [{ version: '1.0.0' }, { version: '1.1.0' }], + }) + // Packument is fetched twice: once when htmlhint resolves node-fetcher@^1.0.0 (picking 1.1.0), and again when addons-scanner is re-queued and re-resolves node-fetcher (picking 1.0.0 to satisfy the exact peerOptional spec). + await registry.package({ manifest: fetcherManifest, times: 2 }) + await registry.tarball({ + manifest: fetcherManifest.versions['1.0.0'], + tarball: path.join(npm.prefix, 'node-fetcher-1.0.0-tarball'), + }) + // node-fetcher@1.1.0 tarball is NOT needed: it's replaced by 1.0.0 during tree building (before reification), so it's never downloaded. + + await npm.exec('install', []) + + // Verify the lockfile has node-fetcher@1.0.0 + const lockfile = JSON.parse( + fs.readFileSync(path.join(npm.prefix, 'package-lock.json'), 'utf8') + ) + t.equal( + lockfile.packages['node_modules/node-fetcher'].version, + '1.0.0', + 'fresh install picks node-fetcher@1.0.0 satisfying peerOptional constraint' + ) + + // Also verify the installed package + const installedFetcher = JSON.parse( + fs.readFileSync( + path.join(npm.prefix, 'node_modules', 'node-fetcher', 'package.json'), 'utf8' + ) + ) + t.equal( + installedFetcher.version, + '1.0.0', + 'installed node-fetcher version satisfies peerOptional constraint' + ) +}) diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index b4319fab6bd37..a256b79049eba 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -339,7 +339,8 @@ module.exports = cls => class IdealTreeBuilder extends cls { filter: node => node, visit: node => { for (const edge of node.edgesOut.values()) { - if ((!edge.to && edge.type !== 'peerOptional') || !edge.valid) { + const skipPeerOptional = edge.type === 'peerOptional' && this.options.save === false + if (!skipPeerOptional && (!edge.to || !edge.valid)) { this.#depsQueue.push(node) break // no need to continue the loop after the first hit } @@ -966,9 +967,17 @@ This is a one-time fix-up, please be patient... continue } const { from, valid, peerConflicted } = edgeIn - if (!peerConflicted && !valid && !this.#depsSeen.has(from)) { - this.addTracker('idealTree', from.name, from.location) - this.#depsQueue.push(edgeIn.from) + 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. + // See npm/cli#8726. + this.#depsSeen.delete(from) + this.#depsQueue.push(from) + } else if (!this.#depsSeen.has(from)) { + this.addTracker('idealTree', from.name, from.location) + this.#depsQueue.push(from) + } } } } else { @@ -1165,9 +1174,13 @@ This is a one-time fix-up, please be patient... continue } - // If the edge has an error, there's a problem. + // 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. + // See npm/cli#8726. if (!edge.valid) { - problems.push(edge) + if (edge.type !== 'peerOptional' || this.options.save !== false) { + problems.push(edge) + } continue } diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index e56daa7ed76ad..aa632426c03e1 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -4460,3 +4460,114 @@ t.test('installLinks behavior with project-internal file dependencies', async t t.equal(edgeToA.to, packageA, 'the edge from b should point to package a') }) }) + +t.test('re-queue already-seen nodes when placed dep invalidates peerOptional (save=true, #8726)', async t => { + // Scenario: alpha has peerOptional on shared@1.0.0, beta has dep on shared@^1.0.0. + // With save=true, alpha is processed first (alphabetical order in DepsQueue), + // its peerOptional is not a problem (missing is OK for peerOptional). + // Beta is processed next, placing shared@1.1.0 (latest ^1.0.0). + // This invalidates alpha's peerOptional edge, triggering re-queue of alpha. + 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 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-install', + version: '1.0.0', + dependencies: { + alpha: '1.0.0', + beta: '1.0.0', + }, + }), + }) + + const arb = newArb(path, { save: true }) + 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.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. + 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 path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'test-8726-ci', + version: '1.0.0', + dependencies: { + alpha: '1.0.0', + beta: '1.0.0', + }, + }), + 'package-lock.json': JSON.stringify({ + name: 'test-8726-ci', + version: '1.0.0', + lockfileVersion: 3, + requires: true, + packages: { + '': { + name: 'test-8726-ci', + version: '1.0.0', + dependencies: { alpha: '1.0.0', beta: '1.0.0' }, + }, + '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 } }, + }, + 'node_modules/beta': { + version: '1.0.0', + resolved: 'https://registry.npmjs.org/beta/-/beta-1.0.0.tgz', + dependencies: { shared: '^1.0.0' }, + }, + 'node_modules/shared': { + 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'] } }) + + 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') +})