fix: keep nested file: deps and re-resolve changed git refs

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, {

workspaces/arborist/lib/shrinkwrap.js

@@ -905,8 +905,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(

workspaces/arborist/test/arborist/reify.js

@@ -2039,6 +2039,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',

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()
+})