Skip to content

Commit bae3f74

Browse files
Saibamencursoragent
Saibamen
and
cursoragent
committed
fix(arborist): re-queue nodes when newly placed dep invalidates peerOptional
When a dependency is placed (OK) and creates an invalid peerOptional edge with an already-processed node, re-queue that node for re-resolution. This handles the case where the peerOptional holder is processed before the dep exists in the tree (due to alphabetical processing order), and the dep is later placed by another path with a version that doesn't satisfy the peer spec. Also adds npm install tests for issue #8726 covering both the existing-lockfile and fresh-install (no lockfile) scenarios. Refs: #8726 Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 383aa4b commit bae3f74

File tree

2 files changed

+307
-6
lines changed

2 files changed

+307
-6
lines changed

‎test/lib/commands/install.js‎

Lines changed: 285 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
const fs = require('node:fs')
12
const tspawk = require('../../fixtures/tspawk')
23
const {
34
cleanCwd,
@@ -770,3 +771,287 @@ t.test('devEngines', async t => {
770771
t.ok(!output.includes('EBADDEVENGINES'))
771772
})
772773
})
774+
775+
// Issue #8726 - npm install should re-resolve to satisfy peerOptional constraints
776+
// https://github.com/npm/cli/issues/8726
777+
//
778+
// When a lockfile has fetcher@1.1.0 but a peerOptional wants fetcher@1.0.0 (exact),
779+
// npm install (save: true) should re-resolve fetcher to 1.0.0 to satisfy both
780+
// the regular dep range (^1.0.0) and the exact peerOptional constraint.
781+
t.test('issue-8726: npm install re-resolves to satisfy peerOptional constraint', async t => {
782+
const { npm, registry } = await loadMockNpm(t, {
783+
config: { audit: false, 'ignore-scripts': true },
784+
prefixDir: {
785+
'linter-tarball': {
786+
'package.json': JSON.stringify({
787+
name: 'linter',
788+
version: '1.0.0',
789+
dependencies: { scanner: '1.0.0' },
790+
}),
791+
},
792+
'scanner-tarball': {
793+
'package.json': JSON.stringify({
794+
name: 'scanner',
795+
version: '1.0.0',
796+
peerDependencies: { fetcher: '1.0.0' },
797+
peerDependenciesMeta: { fetcher: { optional: true } },
798+
}),
799+
},
800+
'hint-tarball': {
801+
'package.json': JSON.stringify({
802+
name: 'hint',
803+
version: '1.0.0',
804+
dependencies: { fetcher: '^1.0.0' },
805+
}),
806+
},
807+
'fetcher-1.0.0-tarball': {
808+
'package.json': JSON.stringify({ name: 'fetcher', version: '1.0.0' }),
809+
},
810+
'fetcher-1.1.0-tarball': {
811+
'package.json': JSON.stringify({ name: 'fetcher', version: '1.1.0' }),
812+
},
813+
'package.json': JSON.stringify({
814+
name: 'test-package',
815+
version: '1.0.0',
816+
devDependencies: {
817+
linter: '1.0.0',
818+
hint: '1.0.0',
819+
},
820+
}),
821+
'package-lock.json': JSON.stringify({
822+
name: 'test-package',
823+
version: '1.0.0',
824+
lockfileVersion: 3,
825+
requires: true,
826+
packages: {
827+
'': {
828+
name: 'test-package',
829+
version: '1.0.0',
830+
devDependencies: { linter: '1.0.0', hint: '1.0.0' },
831+
},
832+
'node_modules/linter': {
833+
version: '1.0.0',
834+
resolved: 'https://registry.npmjs.org/linter/-/linter-1.0.0.tgz',
835+
dev: true,
836+
dependencies: { scanner: '1.0.0' },
837+
},
838+
'node_modules/scanner': {
839+
version: '1.0.0',
840+
resolved: 'https://registry.npmjs.org/scanner/-/scanner-1.0.0.tgz',
841+
dev: true,
842+
peerDependencies: { fetcher: '1.0.0' },
843+
peerDependenciesMeta: { fetcher: { optional: true } },
844+
},
845+
'node_modules/hint': {
846+
version: '1.0.0',
847+
resolved: 'https://registry.npmjs.org/hint/-/hint-1.0.0.tgz',
848+
dev: true,
849+
dependencies: { fetcher: '^1.0.0' },
850+
},
851+
'node_modules/fetcher': {
852+
version: '1.1.0',
853+
resolved: 'https://registry.npmjs.org/fetcher/-/fetcher-1.1.0.tgz',
854+
dev: true,
855+
},
856+
},
857+
}),
858+
},
859+
})
860+
861+
// Only set up mocks that npm install actually needs:
862+
// - Tarballs for all installed packages (linter, scanner, hint, fetcher@1.0.0)
863+
// - Fetcher packument (needed for re-resolution via #problemEdges)
864+
// Packuments for linter/scanner/hint are NOT needed (already in lockfile).
865+
// Fetcher@1.1.0 tarball is NOT needed (gets replaced by 1.0.0).
866+
const linterManifest = registry.manifest({ name: 'linter' })
867+
await registry.tarball({
868+
manifest: linterManifest.versions['1.0.0'],
869+
tarball: path.join(npm.prefix, 'linter-tarball'),
870+
})
871+
872+
const scannerManifest = registry.manifest({ name: 'scanner' })
873+
await registry.tarball({
874+
manifest: scannerManifest.versions['1.0.0'],
875+
tarball: path.join(npm.prefix, 'scanner-tarball'),
876+
})
877+
878+
const hintManifest = registry.manifest({ name: 'hint' })
879+
await registry.tarball({
880+
manifest: hintManifest.versions['1.0.0'],
881+
tarball: path.join(npm.prefix, 'hint-tarball'),
882+
})
883+
884+
const fetcherManifest = registry.manifest({
885+
name: 'fetcher',
886+
versions: ['1.0.0', '1.1.0'],
887+
})
888+
await registry.package({ manifest: fetcherManifest })
889+
await registry.tarball({
890+
manifest: fetcherManifest.versions['1.0.0'],
891+
tarball: path.join(npm.prefix, 'fetcher-1.0.0-tarball'),
892+
})
893+
894+
await npm.exec('install', [])
895+
896+
// Read the updated lockfile and verify fetcher was re-resolved to 1.0.0
897+
const lockfile = JSON.parse(
898+
fs.readFileSync(path.join(npm.prefix, 'package-lock.json'), 'utf8')
899+
)
900+
t.equal(
901+
lockfile.packages['node_modules/fetcher'].version,
902+
'1.0.0',
903+
'lockfile updated fetcher to satisfy peerOptional constraint'
904+
)
905+
906+
// Also verify the installed package
907+
const installedFetcher = JSON.parse(
908+
fs.readFileSync(
909+
path.join(npm.prefix, 'node_modules', 'fetcher', 'package.json'), 'utf8'
910+
)
911+
)
912+
t.equal(
913+
installedFetcher.version,
914+
'1.0.0',
915+
'installed fetcher version satisfies peerOptional constraint'
916+
)
917+
})
918+
919+
// Issue #8726 - fresh npm install (no lockfile) should pick a version that
920+
// satisfies both the regular dep range AND the exact peerOptional constraint,
921+
// even when the peerOptional holder is processed BEFORE the dep is placed.
922+
// https://github.com/npm/cli/issues/8726
923+
//
924+
// This test uses package names that reproduce the real-world alphabetical
925+
// ordering from the original issue (addons-linter < htmlhint), which causes
926+
// addons-scanner to be processed from the queue BEFORE htmlhint places
927+
// node-fetcher. At that point the peerOptional edge has no destination
928+
// (MISSING, valid for peerOptional). Later, htmlhint places node-fetcher@1.1.0
929+
// and the edge becomes INVALID. The fix re-queues addons-scanner so
930+
// #problemEdges can trigger re-resolution of node-fetcher to 1.0.0.
931+
//
932+
// Dependency graph:
933+
// root -> addons-linter@1.0.0 -> addons-scanner@1.0.0
934+
// -> PEER_OPTIONAL node-fetcher@1.0.0
935+
// root -> htmlhint@1.0.0 -> node-fetcher@^1.0.0
936+
//
937+
// Processing order (alphabetical):
938+
// addons-linter, then addons-scanner (dep of addons-linter),
939+
// THEN htmlhint (which places node-fetcher@1.1.0)
940+
t.test('issue-8726: fresh install re-queues scanner when dep placed later', async t => {
941+
const { npm, registry } = await loadMockNpm(t, {
942+
config: { audit: false, 'ignore-scripts': true },
943+
prefixDir: {
944+
'addons-linter-tarball': {
945+
'package.json': JSON.stringify({
946+
name: 'addons-linter',
947+
version: '1.0.0',
948+
dependencies: { 'addons-scanner': '1.0.0' },
949+
}),
950+
},
951+
'addons-scanner-tarball': {
952+
'package.json': JSON.stringify({
953+
name: 'addons-scanner',
954+
version: '1.0.0',
955+
peerDependencies: { 'node-fetcher': '1.0.0' },
956+
peerDependenciesMeta: { 'node-fetcher': { optional: true } },
957+
}),
958+
},
959+
'htmlhint-tarball': {
960+
'package.json': JSON.stringify({
961+
name: 'htmlhint',
962+
version: '1.0.0',
963+
dependencies: { 'node-fetcher': '^1.0.0' },
964+
}),
965+
},
966+
'node-fetcher-1.0.0-tarball': {
967+
'package.json': JSON.stringify({ name: 'node-fetcher', version: '1.0.0' }),
968+
},
969+
'node-fetcher-1.1.0-tarball': {
970+
'package.json': JSON.stringify({ name: 'node-fetcher', version: '1.1.0' }),
971+
},
972+
'package.json': JSON.stringify({
973+
name: 'test-package',
974+
version: '1.0.0',
975+
devDependencies: {
976+
'addons-linter': '1.0.0',
977+
htmlhint: '1.0.0',
978+
},
979+
}),
980+
// NO package-lock.json — this is a fresh install
981+
},
982+
})
983+
984+
// Fresh install needs packuments for all packages
985+
const linterManifest = registry.manifest({
986+
name: 'addons-linter',
987+
packuments: [{ version: '1.0.0', dependencies: { 'addons-scanner': '1.0.0' } }],
988+
})
989+
await registry.package({ manifest: linterManifest })
990+
await registry.tarball({
991+
manifest: linterManifest.versions['1.0.0'],
992+
tarball: path.join(npm.prefix, 'addons-linter-tarball'),
993+
})
994+
995+
const scannerManifest = registry.manifest({
996+
name: 'addons-scanner',
997+
packuments: [{
998+
version: '1.0.0',
999+
peerDependencies: { 'node-fetcher': '1.0.0' },
1000+
peerDependenciesMeta: { 'node-fetcher': { optional: true } },
1001+
}],
1002+
})
1003+
await registry.package({ manifest: scannerManifest })
1004+
await registry.tarball({
1005+
manifest: scannerManifest.versions['1.0.0'],
1006+
tarball: path.join(npm.prefix, 'addons-scanner-tarball'),
1007+
})
1008+
1009+
const hintManifest = registry.manifest({
1010+
name: 'htmlhint',
1011+
packuments: [{ version: '1.0.0', dependencies: { 'node-fetcher': '^1.0.0' } }],
1012+
})
1013+
await registry.package({ manifest: hintManifest })
1014+
await registry.tarball({
1015+
manifest: hintManifest.versions['1.0.0'],
1016+
tarball: path.join(npm.prefix, 'htmlhint-tarball'),
1017+
})
1018+
1019+
const fetcherManifest = registry.manifest({
1020+
name: 'node-fetcher',
1021+
packuments: [{ version: '1.0.0' }, { version: '1.1.0' }],
1022+
})
1023+
// Packument is fetched twice: once when htmlhint resolves node-fetcher@^1.0.0
1024+
// (picking 1.1.0), and again when addons-scanner is re-queued and re-resolves
1025+
// node-fetcher (picking 1.0.0 to satisfy the exact peerOptional spec).
1026+
await registry.package({ manifest: fetcherManifest, times: 2 })
1027+
await registry.tarball({
1028+
manifest: fetcherManifest.versions['1.0.0'],
1029+
tarball: path.join(npm.prefix, 'node-fetcher-1.0.0-tarball'),
1030+
})
1031+
// node-fetcher@1.1.0 tarball is NOT needed: it's replaced by 1.0.0 during
1032+
// tree building (before reification), so it's never downloaded.
1033+
1034+
await npm.exec('install', [])
1035+
1036+
// Verify the lockfile has node-fetcher@1.0.0
1037+
const lockfile = JSON.parse(
1038+
fs.readFileSync(path.join(npm.prefix, 'package-lock.json'), 'utf8')
1039+
)
1040+
t.equal(
1041+
lockfile.packages['node_modules/node-fetcher'].version,
1042+
'1.0.0',
1043+
'fresh install picks node-fetcher@1.0.0 satisfying peerOptional constraint'
1044+
)
1045+
1046+
// Also verify the installed package
1047+
const installedFetcher = JSON.parse(
1048+
fs.readFileSync(
1049+
path.join(npm.prefix, 'node_modules', 'node-fetcher', 'package.json'), 'utf8'
1050+
)
1051+
)
1052+
t.equal(
1053+
installedFetcher.version,
1054+
'1.0.0',
1055+
'installed node-fetcher version satisfies peerOptional constraint'
1056+
)
1057+
})

‎workspaces/arborist/lib/arborist/build-ideal-tree.js‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,8 @@ module.exports = cls => class IdealTreeBuilder extends cls {
339339
filter: node => node,
340340
visit: node => {
341341
for (const edge of node.edgesOut.values()) {
342-
if (edge.type !== 'peerOptional' && (!edge.to || !edge.valid)) {
342+
const skipPeerOptional = edge.type === 'peerOptional' && this.options.save === false
343+
if (!skipPeerOptional && (!edge.to || !edge.valid)) {
343344
this.#depsQueue.push(node)
344345
break // no need to continue the loop after the first hit
345346
}
@@ -966,9 +967,21 @@ This is a one-time fix-up, please be patient...
966967
continue
967968
}
968969
const { from, valid, peerConflicted } = edgeIn
969-
if (!peerConflicted && !valid && !this.#depsSeen.has(from)) {
970-
this.addTracker('idealTree', from.name, from.location)
971-
this.#depsQueue.push(edgeIn.from)
970+
if (!peerConflicted && !valid) {
971+
if (this.#depsSeen.has(from) && this.options.save) {
972+
// Re-queue already-processed nodes when a newly placed
973+
// dep creates an invalid edge during npm install
974+
// (save=true). This handles the case where a peerOptional
975+
// dep was valid (missing) when the node was first
976+
// processed, but becomes invalid when the dep is later
977+
// placed by another path with a version that doesn't
978+
// satisfy the peer spec. See npm/cli#8726.
979+
this.#depsSeen.delete(from)
980+
this.#depsQueue.push(from)
981+
} else if (!this.#depsSeen.has(from)) {
982+
this.addTracker('idealTree', from.name, from.location)
983+
this.#depsQueue.push(from)
984+
}
972985
}
973986
}
974987
} else {
@@ -1166,9 +1179,12 @@ This is a one-time fix-up, please be patient...
11661179
}
11671180

11681181
// If the edge has an error, there's a problem, unless
1169-
// it's peerOptional and not explicitly requested.
1182+
// it's peerOptional and we're not saving (e.g. npm ci),
1183+
// in which case we trust the lockfile and skip re-resolution.
1184+
// When saving (npm install), peerOptional invalid edges ARE
1185+
// treated as problems so the lockfile gets fixed.
11701186
if (!edge.valid) {
1171-
if (edge.type !== 'peerOptional' ||
1187+
if ((edge.type !== 'peerOptional' || this.options.save !== false) ||
11721188
this.#explicitRequests.has(edge)) {
11731189
problems.push(edge)
11741190
}

0 commit comments

Comments
 (0)