fix(arborist): drop orphaned patch entry on uninstall instead of EPATCHUNUSED

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

@@ -187,6 +187,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
       await resolvePatchedDependencies(this.idealTree, {
         path: this.path,
         allowUnusedPatches: this.options.allowUnusedPatches,
+        rm: options.rm || [],
       })
       this.#warnWorkspacePackageExtensions()
     } finally {

workspaces/arborist/lib/arborist/reify.js

@@ -1824,6 +1824,8 @@ module.exports = cls => class Reifier extends cls {
           // field so defaulting this to an empty array would add that field to
           // every package.json file.
           bundleDependencies,
+          // resolvePatchedDependencies drops entries orphaned by uninstall; persist that removal
+          patchedDependencies,
         } = tree.package
 
         pkgJson.update({
@@ -1832,6 +1834,7 @@ module.exports = cls => class Reifier extends cls {
           optionalDependencies,
           peerDependencies,
           bundleDependencies,
+          patchedDependencies,
         })
         await pkgJson.save()
       }

workspaces/arborist/lib/patched-dependencies.js

@@ -3,6 +3,7 @@
 // Enforces the failure modes (workspace-member entry, missing file, unused patch, non-registry target, ambiguous selectors) as hard errors.
 const semver = require('semver')
 const npa = require('npm-package-arg')
+const { log } = require('proc-log')
 const { resolve, relative, isAbsolute } = require('node:path')
 const { readFile } = require('node:fs/promises')
 const { patchIntegrity } = require('./patch.js')
@@ -61,7 +62,7 @@ const matchSelector = (selectors, node) => {
   return matches.find(s => s.spec === null) || null
 }
 
-const resolvePatchedDependencies = async (tree, { path, allowUnusedPatches }) => {
+const resolvePatchedDependencies = async (tree, { path, allowUnusedPatches, rm }) => {
   const patchedDependencies = tree.package?.patchedDependencies || {}
   const selectors = Object.entries(patchedDependencies)
     .map(([key, patchPath]) => ({ ...parseSelector(key), key, patchPath }))
@@ -135,15 +136,30 @@ const resolvePatchedDependencies = async (tree, { path, allowUnusedPatches }) =>
     usedKeys.add(selector.key)
   }
 
-  if (selectors.length && !allowUnusedPatches) {
+  if (selectors.length) {
     const unused = selectors.filter(s => !usedKeys.has(s.key))
-    if (unused.length) {
+
+    // an explicit `npm uninstall <name>` orphans that package's patch entry, so drop it instead of failing
+    const removed = new Set(rm)
+    const dropped = unused.filter(s => removed.has(s.name))
+    if (dropped.length) {
+      const patched = { ...tree.package.patchedDependencies }
+      for (const s of dropped) {
+        delete patched[s.key]
+        log.notice('patch', `Removing patch entry "${s.key}" for uninstalled ${s.name}; left ${s.patchPath} in place.`)
+      }
+      // undefined drops the key entirely when reify writes package.json
+      tree.package.patchedDependencies = Object.keys(patched).length ? patched : undefined
+    }
+
+    const stillUnused = unused.filter(s => !removed.has(s.name))
+    if (stillUnused.length && !allowUnusedPatches) {
       throw err(
         `The following patches were registered but matched no installed ` +
-          `package:\n${unused.map(s => `  ${s.key} -> ${s.patchPath}`).join('\n')}\n` +
+          `package:\n${stillUnused.map(s => `  ${s.key} -> ${s.patchPath}`).join('\n')}\n` +
           `Use --allow-unused-patches to install anyway.`,
         'EPATCHUNUSED',
-        { unused: unused.map(s => s.key) }
+        { unused: stillUnused.map(s => s.key) }
       )
     }
   }

workspaces/arborist/test/arborist/reify-patch.js

@@ -102,6 +102,29 @@ t.test('registry dep with patch is applied and recorded in lockfile', async t =>
   t.match(pkgEntry.patched.integrity, /^sha512-/, 'patched.integrity is an SSRI')
 })
 
+t.test('uninstalling a patched package drops its entry from package.json', async t => {
+  const registry = createRegistry(t)
+  // one install, then an uninstall that resolves from the lockfile so no extra fetches
+  await mockPackage(t, registry)
+
+  const patch = filePatch('index.js', ORIGINAL, PATCHED)
+  const path = makeProject(t, {
+    patch,
+    patchedDependencies: { [`${PKG_NAME}@${PKG_VERSION}`]: `patches/${PKG_NAME}@${PKG_VERSION}.patch` },
+  })
+
+  await newArb({ path }).reify()
+  t.equal(fs.readFileSync(installedFile(path), 'utf8'), PATCHED, 'package installed and patched')
+
+  // npm uninstall <pkg>: removes the dep but leaves the now-orphaned patch entry
+  await newArb({ path }).reify({ rm: [PKG_NAME] })
+
+  const rootPkg = JSON.parse(fs.readFileSync(resolve(path, 'package.json'), 'utf8'))
+  t.notOk(rootPkg.dependencies?.[PKG_NAME], 'dependency removed from package.json')
+  t.notOk(rootPkg.patchedDependencies, 'orphaned patch entry dropped from package.json')
+  t.notOk(fs.existsSync(resolve(path, 'node_modules', PKG_NAME)), 'package removed from node_modules')
+})
+
 t.test('patch is re-applied on a patch-change reify even with ignoreScripts', async t => {
   const registry = createRegistry(t)
   // two reifys: the second re-extracts the node due to the patch change.

workspaces/arborist/test/patched-dependencies-resolve.js

@@ -239,6 +239,114 @@ t.test('EPATCHUNUSED when a registered patch matches no node', async t => {
   await t.rejects(buildIdeal(path), { code: 'EPATCHUNUSED', unused: ['ghost@1.0.0'] })
 })
 
+t.test('uninstalling a patched package drops its orphaned patch entry', async t => {
+  // npm uninstall removes the node but leaves the patchedDependencies entry, which would otherwise be EPATCHUNUSED
+  const path = t.testdir({
+    'fix.patch': PATCH,
+    'package.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      dependencies: { dep: '^1.0.0' },
+      patchedDependencies: { 'dep@1.0.0': 'fix.patch' },
+    }),
+    'package-lock.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      lockfileVersion: 3,
+      requires: true,
+      packages: {
+        '': { name: 'root', version: '1.0.0', dependencies: { dep: '^1.0.0' } },
+        'node_modules/dep': lockEntry('dep', '1.0.0'),
+      },
+    }),
+    node_modules: {
+      dep: { 'package.json': JSON.stringify({ name: 'dep', version: '1.0.0' }) },
+    },
+  })
+
+  const tree = await buildIdeal(path, { rm: ['dep'] })
+  t.notOk(tree.inventory.query('name', 'dep').size, 'dep node is removed')
+  t.notOk(tree.package.patchedDependencies, 'the orphaned patch entry is dropped')
+})
+
+t.test('uninstall keeps patch entries for packages that remain', async t => {
+  // removing one package must not drop another package's still-used patch
+  const path = t.testdir({
+    'a.patch': PATCH,
+    'b.patch': PATCH,
+    'package.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      dependencies: { a: '^1.0.0', b: '^1.0.0' },
+      patchedDependencies: { 'a@1.0.0': 'a.patch', 'b@1.0.0': 'b.patch' },
+    }),
+    'package-lock.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      lockfileVersion: 3,
+      requires: true,
+      packages: {
+        '': { name: 'root', version: '1.0.0', dependencies: { a: '^1.0.0', b: '^1.0.0' } },
+        'node_modules/a': lockEntry('a', '1.0.0'),
+        'node_modules/b': lockEntry('b', '1.0.0'),
+      },
+    }),
+    node_modules: {
+      a: { 'package.json': JSON.stringify({ name: 'a', version: '1.0.0' }) },
+      b: { 'package.json': JSON.stringify({ name: 'b', version: '1.0.0' }) },
+    },
+  })
+
+  const tree = await buildIdeal(path, { rm: ['a'] })
+  t.notOk(tree.inventory.query('name', 'a').size, 'a node is removed')
+  t.same(tree.package.patchedDependencies, { 'b@1.0.0': 'b.patch' }, 'b patch entry survives')
+  const b = tree.inventory.query('name', 'b').values().next().value
+  t.ok(b.patched, 'b is still patched')
+})
+
+t.test('uninstall does not drop a patch when the package survives as a transitive dep', async t => {
+  // dep is a direct dep being removed but stays in the tree under parent, so its patch is still used
+  const path = t.testdir({
+    'fix.patch': PATCH,
+    'package.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      dependencies: { dep: '^1.0.0', parent: '^1.0.0' },
+      patchedDependencies: { 'dep@1.0.0': 'fix.patch' },
+    }),
+    'package-lock.json': JSON.stringify({
+      name: 'root',
+      version: '1.0.0',
+      lockfileVersion: 3,
+      requires: true,
+      packages: {
+        '': { name: 'root', version: '1.0.0', dependencies: { dep: '^1.0.0', parent: '^1.0.0' } },
+        'node_modules/dep': lockEntry('dep', '1.0.0'),
+        'node_modules/parent': {
+          ...lockEntry('parent', '1.0.0'),
+          dependencies: { dep: '^1.0.0' },
+        },
+      },
+    }),
+    node_modules: {
+      dep: { 'package.json': JSON.stringify({ name: 'dep', version: '1.0.0' }) },
+      parent: {
+        'package.json': JSON.stringify({
+          name: 'parent',
+          version: '1.0.0',
+          dependencies: { dep: '^1.0.0' },
+        }),
+      },
+    },
+  })
+
+  const tree = await buildIdeal(path, { rm: ['dep'] })
+  const dep = tree.inventory.query('name', 'dep').values().next().value
+  t.ok(dep, 'dep node survives as a transitive dependency')
+  t.ok(dep.patched, 'dep is still patched')
+  t.same(tree.package.patchedDependencies, { 'dep@1.0.0': 'fix.patch' }, 'patch entry is kept')
+})
+
 t.test('allowUnusedPatches:true suppresses EPATCHUNUSED', async t => {
   const path = t.testdir({
     'fix.patch': PATCH,