From 54f0d1e637d981be14c55ac46c8160b90d7c804e Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Wed, 10 Jun 2026 07:30:54 -0700 Subject: [PATCH] fix: keep nested file: deps and re-resolve changed git refs (#9523) The lockfile pruning added in 11.15.0 dropped every extraneous node outside node_modules to remove stale workspace entries. This also removed legitimate nested entries for a file: dependency that itself has a file: dependency (e.g. lib/b/lib/a), leaving the lockfile incomplete so `npm ci` failed with "Missing: a@1.0.0 from lock file". Only prune extraneous nodes that are direct fsChildren of the root or detached link targets (removed workspaces, removed root-level file: deps, orphaned link targets). Extraneous fsChildren nested under another package are kept so `npm ci` can resolve the parent's dependency. Fixes: https://github.com/npm/cli/issues/9433 Fixes: https://github.com/npm/cli/issues/9486 Supersedes: https://github.com/npm/cli/pull/9524 (cherry picked from commit fc6268ab7fa4ce1dcaefd6cd470a1f73eb192565) --- workspaces/arborist/lib/dep-valid.js | 29 +++++++++++ workspaces/arborist/lib/shrinkwrap.js | 10 +++- workspaces/arborist/test/arborist/reify.js | 50 ++++++++++++++++++ workspaces/arborist/test/dep-valid.js | 60 ++++++++++++++++++++++ 4 files changed, 147 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/dep-valid.js b/workspaces/arborist/lib/dep-valid.js index bee7ce2768c4d..db93c2980852a 100644 --- a/workspaces/arborist/lib/dep-valid.js +++ b/workspaces/arborist/lib/dep-valid.js @@ -9,6 +9,27 @@ const npa = require('npm-package-arg') const { relative } = require('node:path') const fromPath = require('./from-path.js') +// A named ref (tag or branch) resolves to a commit hash, so look up the +// committish recorded for this edge in the lockfile to detect spec changes. +const lockedGitCommittish = (child, requestor) => { + const lock = requestor.root?.meta?.data?.packages?.[requestor.location] + const spec = lock && ( + lock.dependencies?.[child.name] || + lock.optionalDependencies?.[child.name] || + lock.devDependencies?.[child.name] || + lock.peerDependencies?.[child.name] + ) + if (!spec) { + return null + } + try { + const parsed = npa.resolve(child.name, spec, requestor.realpath) + return parsed.type === 'git' ? parsed.gitCommittish || '' : null + } catch { + return null + } +} + const depValid = (child, requested, requestor) => { // NB: we don't do much to verify 'tag' type requests. // Just verify that we got a remote resolution. Presumably, it @@ -94,6 +115,14 @@ const depValid = (child, requested, requestor) => { } } if (!requested.gitRange) { + // a named ref can't be verified against the resolved commit offline, + // so re-resolve if it differs from the committish in the lockfile + if (!reqCommit) { + const locked = lockedGitCommittish(child, requestor) + if (locked !== null && locked !== (requested.gitCommittish || '')) { + return false + } + } return true } return semver.satisfies(child.package.version, requested.gitRange, { diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index 6af902cdf8a48..cf27bcf4ac726 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/workspaces/arborist/lib/shrinkwrap.js @@ -929,8 +929,14 @@ class Shrinkwrap { continue } const loc = relpath(this.path, node.path) - // Drop lockfile entries for extraneous nodes outside node_modules. These are stale workspace entries: the workspace was removed from package.json or its directory was deleted, so it should not be tracked in package-lock.json. - if (node.extraneous && !/(^|\/)node_modules\//.test(loc) && loc !== 'node_modules') { + // Drop lockfile entries for extraneous nodes outside node_modules that + // are direct fsChildren of the root (or detached link targets). These + // are stale top-level entries: a workspace or file: dep removed from + // the root manifest, or whose directory was deleted. Extraneous + // fsChildren nested under another package (e.g. a file: dep of another + // file: dep) are kept so `npm ci` can resolve the parent's dependency. + if (node.extraneous && !/(^|\/)node_modules\//.test(loc) && loc !== 'node_modules' && + (!node.fsParent || node.fsParent.isRoot)) { continue } const meta = Shrinkwrap.metaFromNode( diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index 2b83f918547f1..d80d4d2998758 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -2134,6 +2134,56 @@ t.test('removed workspace is pruned from package-lock.json', async t => { } }) +// Regression for https://github.com/npm/cli/issues/9433: a file: dependency +// that itself has a file: dependency leaves a nested extraneous fsChild. That +// entry must stay in package-lock.json, otherwise `npm ci` reports the nested +// dep as missing and refuses to install. +t.test('nested file: dep keeps extraneous fsChild in package-lock.json', async t => { + const path = t.testdir({ + 'package.json': JSON.stringify({ + name: 'c', + version: '1.0.0', + private: true, + dependencies: { a: 'file:lib/a', b: 'file:lib/b' }, + }), + lib: { + a: { 'package.json': JSON.stringify({ name: 'a', version: '1.0.0', private: true }) }, + b: { + 'package.json': JSON.stringify({ + name: 'b', + version: '1.0.0', + private: true, + dependencies: { a: 'file:lib/a' }, + }), + lib: { + a: { 'package.json': JSON.stringify({ name: 'a', version: '1.0.0', private: true }) }, + }, + }, + }, + }) + createRegistry(t, false) + await reify(path) + + const lock = JSON.parse(fs.readFileSync(`${path}/package-lock.json`, 'utf8')) + t.match(lock.packages['lib/b/lib/a'], { version: '1.0.0', extraneous: true }, + 'nested file: dep is recorded as an extraneous entry in the lockfile') + + // Mirror the `npm ci` sync check: every node in the ideal tree built from + // package.json must be present in the virtual tree loaded from the lockfile. + const virtual = newArb({ path }) + await virtual.loadVirtual() + const virtualInventory = new Map(virtual.virtualTree.inventory) + const ideal = newArb({ path }) + await ideal.buildIdealTree() + const missing = [] + for (const [loc, node] of ideal.idealTree.inventory.entries()) { + if (!virtualInventory.has(loc)) { + missing.push(`${node.name}@${node.version}`) + } + } + t.same(missing, [], 'lockfile is in sync with package.json, so npm ci would succeed') +}) + t.test('project with bundled deps and a link dep on itself', async t => { const pkg = { name: '@isaacs/testing-bundle-self-link', diff --git a/workspaces/arborist/test/dep-valid.js b/workspaces/arborist/test/dep-valid.js index 104a29cb96516..436e2afac1222 100644 --- a/workspaces/arborist/test/dep-valid.js +++ b/workspaces/arborist/test/dep-valid.js @@ -253,3 +253,63 @@ t.test('sha-1 and sha-256', t => { t.end() }) + +t.test('git tag/branch change detected via lockfile committish', t => { + // a named ref points at a commit hash, so the recorded committish tells us + // whether the spec changed + const mkRequestor = (recorded) => ({ + errors: [], + edgesOut: new Map(), + realpath: resolve('/some/path'), + location: '', + root: { + meta: { data: { packages: { '': recorded } } }, + }, + }) + + const child = { + name: 'repo', + resolved: 'git+ssh://git@github.com/npm/repo.git#0d7bd85a85fa2571fa532d2fc842ed099b236ad2', + package: { version: '1.0.0' }, + get version () { + return this.package.version + }, + } + + t.ok(depValid(child, 'npm/repo#v1.0.0', null, + mkRequestor({ dependencies: { repo: 'npm/repo#v1.0.0' } })), + 'unchanged tag is valid') + + t.notOk(depValid(child, 'npm/repo#v2.0.0', null, + mkRequestor({ dependencies: { repo: 'npm/repo#v1.0.0' } })), + 'changed tag must be re-resolved') + + t.notOk(depValid(child, 'npm/repo#other', null, + mkRequestor({ devDependencies: { repo: 'npm/repo#main' } })), + 'changed branch in devDependencies must be re-resolved') + + t.notOk(depValid(child, 'npm/repo#v2.0.0', null, + mkRequestor({ optionalDependencies: { repo: 'npm/repo#v1.0.0' } })), + 'changed tag in optionalDependencies must be re-resolved') + + t.notOk(depValid(child, 'npm/repo#other', null, + mkRequestor({ peerDependencies: { repo: 'npm/repo#main' } })), + 'changed branch in peerDependencies must be re-resolved') + + t.notOk(depValid(child, 'npm/repo#v2.0.0', null, + mkRequestor({ dependencies: { repo: 'npm/repo' } })), + 'lockfile git spec without a committish differs from a named ref') + + t.ok(depValid(child, 'npm/repo#v2.0.0', null, + mkRequestor({ dependencies: { repo: '^1.0.0' } })), + 'non-git lockfile spec is ignored, falling back to the repo-only check') + + t.ok(depValid(child, 'npm/repo#v2.0.0', null, emptyRequestor), + 'without lockfile data, fall back to the repo-only check') + + t.ok(depValid(child, 'npm/repo#v2.0.0', null, + mkRequestor({ dependencies: { repo: 'invalid spec with spaces' } })), + 'unparseable lockfile spec is ignored, falling back to the repo-only check') + + t.end() +})