fix(patch): keep the update marker on a no-op commit so a retry finalizes (#9567)

A conflicted `npm patch update` leaves an edit dir and a
`.npm-patch-update.json` marker that the finalizing `npm patch commit`
reads to finish the update — a metadata-only finalize that drops the
renamed-from selector and tolerates the new version not being installed
yet.

`commit()` deleted that marker before checking whether the edit dir
produced a diff. So a first commit that did no net work — the conflict
resolved to the new version verbatim, giving "nothing to commit" —
consumed the marker. A corrected retry then found none, ran a full
reify, and failed `EPATCHUNUSED` (the rebased-to version isn't
installed), leaving both the old and new selector in the manifest. The
same loss happened on any non-finalizing path (e.g. the `EPATCHUNSAFE`
throw).

## Fix

The marker was deleted eagerly only to keep it out of the generated
patch. Instead, keep it and teach `diffDirs` to skip it:

- `diffDirs(originalDir, editedDir, ignore = new Set())` skips a set of
root-relative filenames, like it already skips the root `package.json`.
- `commit()` no longer deletes the marker — it reads/parses it (still
throwing `EPATCHBADMARKER` on bad JSON before any diff), and passes `new
Set([UPDATE_MARKER])` to `diffDirs`.

The marker now survives any non-finalizing path, so a corrected retry
still finalizes. A successful commit removes the whole edit dir (unless
`--keep-edit-dir`), so nothing lingers.

## References

Fixes #9566
Follow-up to #9439 (native dependency patching).

Author: manzoorwanijk

lib/commands/patch.js

@@ -235,12 +235,11 @@ class Patch extends BaseCommand {
     }
 
     // a conflicted `patch update` leaves a marker so this commit drops the renamed-from selector
+    // it is kept on disk (and excluded from the diff) so a re-run after a no-op resolution still finalizes the update
     const markerPath = join(editDir, UPDATE_MARKER)
     const markerRaw = await readFile(markerPath, 'utf8').catch(() => null)
     let marker = null
     if (markerRaw !== null) {
-      // always remove the marker so it never lands in the generated diff, even if it is malformed
-      await rm(markerPath, { force: true })
       try {
         marker = JSON.parse(markerRaw)
       } catch {
@@ -253,7 +252,7 @@ class Patch extends BaseCommand {
     let diff, packageJsonChanged
     try {
       await pacote.extract(selectorKey(name, version), base, this.npm.flatOptions)
-      ;({ diff, packageJsonChanged } = await diffDirs(base, editDir))
+      ;({ diff, packageJsonChanged } = await diffDirs(base, editDir, new Set([UPDATE_MARKER])))
     } finally {
       await rm(base, { recursive: true, force: true })
     }

lib/utils/patch-diff.js

@@ -38,7 +38,8 @@ const readMaybe = async file => {
 // Diff originalDir against editedDir, returning { diff, packageJsonChanged }.
 // Added files use `--- /dev/null`, deleted files use `+++ /dev/null`.
 // The root package.json is excluded: Arborist resolves the pre-patch manifest, so a patched manifest would apply to disk without being honored.
-const diffDirs = async (originalDir, editedDir) => {
+// ignore holds extra root-relative filenames the caller keeps out of the diff, e.g. the patch-update marker.
+const diffDirs = async (originalDir, editedDir, ignore = new Set()) => {
   const [origFiles, editFiles] = await Promise.all([
     listFiles(originalDir),
     listFiles(editedDir),
@@ -63,6 +64,11 @@ const diffDirs = async (originalDir, editedDir) => {
       continue
     }
 
+    // caller-owned control files (e.g. the patch-update marker) never belong in the diff
+    if (ignore.has(file)) {
+      continue
+    }
+
     let patch = createTwoFilesPatch(
       `a/${file}`, `b/${file}`, a || '', b || '', '', ''
     ).replace('===================================================================\n', '')

test/lib/commands/patch.js

@@ -648,6 +648,42 @@ t.test('update conflict leaves an edit dir; commit finalizes the rename', async
   t.notOk(fs.existsSync(path.join(npm.prefix, 'patches', `${name}@1.0.0.patch`)), 'old patch file removed')
 })
 
+t.test('a no-op resolving commit keeps the marker so a corrected retry still finalizes', async t => {
+  const name = 'upd-noop-retry'
+  const { npm, joinedOutput, outputs, registry } = await loadMockNpm(t, {
+    config: { 'ignore-scripts': true, audit: false },
+    strictRegistryNock: false,
+    prefixDir: rootWith({ [name]: '^1.0.0' }),
+  })
+  await setupVersions(npm, registry, name, { '1.0.0': 'a\nb\nc\n', '2.0.0': 'a\nBB\nc\n' })
+  await npm.exec('install', [])
+  outputs.length = 0
+  await npm.exec('patch', ['add', name])
+  const addDir = joinedOutput().match(/directory: (.+)/)[1].trim()
+  fs.writeFileSync(path.join(addDir, 'index.js'), 'a\nMINE\nc\n')
+  await npm.exec('patch', ['commit', addDir])
+
+  npm.config.set('to', '2.0.0')
+  outputs.length = 0
+  await npm.exec('patch', ['update', name])
+  const editDir = joinedOutput().match(/Resolve the conflicts in: (.+)/)[1].trim()
+  const markerPath = path.join(editDir, '.npm-patch-update.json')
+  t.ok(fs.existsSync(markerPath), 'marker written on conflict')
+
+  // resolve to the new version verbatim (no net change) and commit: a no-op
+  let src = fs.readFileSync(path.join(editDir, 'index.js'), 'utf8')
+  src = src.replace(/<<<<<<<[^\n]*\n([\s\S]*?)=======\n[\s\S]*?>>>>>>>[^\n]*\n/, '$1')
+  fs.writeFileSync(path.join(editDir, 'index.js'), src)
+  await npm.exec('patch', ['commit', editDir])
+  t.ok(fs.existsSync(markerPath), 'marker survives a no-op commit so the update context is not lost')
+
+  // now resolve properly and commit again: must finalize the rename, not throw EPATCHUNUSED on the uninstalled 2.0.0
+  fs.writeFileSync(path.join(editDir, 'index.js'), src.replace('BB', 'MINE'))
+  await npm.exec('patch', ['commit', editDir])
+  t.same(readJson(path.join(npm.prefix, 'package.json')).patchedDependencies,
+    { [`${name}@2.0.0`]: `patches/${name}@2.0.0.patch` }, 'finalized: old selector dropped, new one added')
+})
+
 t.test('update conflict on a name-only selector forks and commits without EPATCHUNUSED', async t => {
   const name = 'upd-rconflict'
   const { npm, joinedOutput, outputs, registry } = await loadMockNpm(t, {