Fix vendored revert keeping artifact for removed lock entry (#665)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -717,7 +717,11 @@ worse, lets a warm cache silently serve unpatched bytes):
   whole-file wiring cannot tell a converged fragment from a drifted one, keep the artifact exactly
   while the live `composer.lock` / `pom.xml` / `nuget.config` still names its
   `.socket/vendor/<eco>/<uuid>` dir — a file that no longer references it is warned about and the
-  artifact removed), removes the artifacts, prunes the
+  artifact removed; in the npm family (npm, yarn classic and berry, pnpm, bun) a recorded lock entry
+  that no longer exists at all — the user removed the dependency — is not drift: it warns
+  `vendor_lock_entry_removed` and the artifact and entry are kept unless every wired file that exists
+  was read and none mentions the uuid in any spelling (an unreadable lock keeps them), so `rollback` / `remove` / `scan --prune` clean up
+  after `npm uninstall` / `yarn remove` / `pnpm remove` / `bun remove`), removes the artifacts, prunes the
   ledger, sweeps orphan uuid dirs, and (v5.0) prunes the now-empty `.socket/vendor/<eco>/` and
   `.socket/vendor/` levels — `.socket/` itself is removed by the lock guard when nothing else is
   left. It works without a manifest: with no manifest and no ledger it is a clean exit-0 no-op.

crates/socket-patch-cli/tests/in_process_vendor.rs

@@ -492,6 +492,57 @@ async fn revert_round_trip() {
     assert_eq!(env["summary"]["removed"], 0);
 }
 
+/// #665: `npm uninstall left-pad` after vendoring deletes the lock entry
+/// the wiring recorded, so nothing resolves through the vendored artifact
+/// any more. `rollback` used to report that as drift, keep the artifact
+/// and the ledger entry, and exit 1 on every run; `vendor --revert` then
+/// "succeeded" without cleaning anything up. Now the first rollback drops
+/// the unreferenced artifact and ledger entry, and every later run is a
+/// clean exit 0.
+#[tokio::test]
+async fn rollback_after_dependency_removed_cleans_up_and_converges() {
+    let fx = npm_fixture();
+    assert_eq!(vendor_run(vendor_args(fx.root())).await, 0);
+    assert!(fx.tgz_path().exists(), "sanity: vendored");
+
+    // What `npm uninstall left-pad` leaves behind.
+    let mut lock = fx.lock_value();
+    let packages = lock["packages"].as_object_mut().unwrap();
+    packages.remove("node_modules/left-pad");
+    packages[""]["dependencies"] = json!({});
+    let mut uninstalled = serde_json::to_vec_pretty(&lock).unwrap();
+    uninstalled.push(b'\n');
+    std::fs::write(fx.lock_path(), &uninstalled).unwrap();
+    std::fs::remove_dir_all(fx.root().join("node_modules/left-pad")).unwrap();
+
+    let cwd = fx.root().to_str().unwrap();
+    let (code, stdout, stderr) = run_cli(
+        fx.root(),
+        &["rollback", "--json", "--yes", "--offline", "--cwd", cwd],
+        &[],
+    );
+    assert_eq!(code, 0, "rollback must succeed:\n{stdout}\n{stderr}");
+    assert!(
+        !stdout.contains("vendor_artifact_kept"),
+        "nothing is kept:\n{stdout}"
+    );
+    assert!(
+        !fx.vendor_dir().exists(),
+        "the unreferenced artifact and ledger are cleaned up:\n{stdout}"
+    );
+    assert_eq!(fx.lock_bytes(), uninstalled, "the user's lock is untouched");
+
+    let (code, stdout, stderr) = run_cli(
+        fx.root(),
+        &["rollback", "--json", "--yes", "--offline", "--cwd", cwd],
+        &[],
+    );
+    assert_eq!(code, 0, "a second rollback is a no-op:\n{stdout}\n{stderr}");
+    let (code, env) = vendor_cli(fx.root(), &["--revert"]);
+    assert_eq!(code, 0, "{env:#}");
+    assert!(events(&env).is_empty(), "nothing left to revert: {env:#}");
+}
+
 // ─────────────────────────────────────────────────────────────────────
 // 5. revert works without a manifest
 // ─────────────────────────────────────────────────────────────────────

crates/socket-patch-cli/tests/scan_vendor_e2e.rs

@@ -1024,20 +1024,15 @@ async fn scan_vendor_resolves_percent_encoded_scoped_purl() {
 // ───────────────────── prune reconciles vendored state ─────────────────────
 
 /// After a dependency is removed and re-locked, `scan --prune` (without
-/// `--vendor`) honors the drift-keep contract, then completes the reclaim
-/// once the drift is undone:
+/// `--vendor`) reclaims its vendored entry in one run (#665):
 ///
-/// 1. The wired lock entry VANISHED (an uninstall is one drift flavor —
-///    the live lock no longer matches anything the wiring recorded), so
-///    the backend revert keeps the artifacts (`RevertOutcome::
-///    kept_artifact`) and the GC must keep the ledger entry too — pruning
-///    it would let the orphan sweep destroy the kept artifacts (with the
-///    recorded pre-vendor originals, the state a later `git checkout` of
-///    the vendored lock still points at).
-/// 2. Undoing the drift (restoring the pre-vendor registry lock — the
-///    keep warning's documented remediation) converges every recorded
-///    fragment, and the same prune then reverts fully: ledger entry
-///    dropped, artifact dir removed, lock untouched.
+/// 1. The wired lock entry VANISHED (`npm uninstall`). That is not drift:
+///    nothing in the lock resolves through the artifact any more, so the
+///    backend revert removes it and the GC drops the ledger entry, leaving
+///    the user's re-locked lock byte-identical. (Before #665 the vanished
+///    entry was drift-kept forever and the `scan --prune` remedy the
+///    vendored rescan prints never converged.)
+/// 2. A second prune is a no-op.
 ///
 /// Every vendored entry is ledger-owned (`detached`), and the lockfile-
 /// usage leg of the GC judges entries by the LIVE lock, so being detached
@@ -1048,7 +1043,6 @@ async fn scan_prune_reverts_unused_vendored_entry() {
     mount_patch_api(&mock, UUID).await;
     let tmp = tempfile::tempdir().unwrap();
     write_fixture(tmp.path());
-    let original_lock = std::fs::read(tmp.path().join("package-lock.json")).unwrap();
 
     // A second installed package so the later prune run's crawl is
     // non-empty (left-pad itself gets removed below).
@@ -1108,44 +1102,18 @@ async fn scan_prune_reverts_unused_vendored_entry() {
         serde_json::from_str::<serde_json::Value>(stdout.trim()).expect("valid JSON")
     };
 
-    // 1. Drifted (vanished) lock entry: everything is KEPT — nothing may
-    //    be reported reverted, and the artifacts must survive the sweep.
-    let v = run_prune();
-    assert_eq!(
-        v["gc"]["revertedVendoredEntries"],
-        serde_json::json!([]),
-        "a drift-kept entry must not be reported reverted: {v}"
-    );
-    let state: serde_json::Value = serde_json::from_str(
-        &std::fs::read_to_string(tmp.path().join(".socket/vendor/state.json")).unwrap(),
-    )
-    .unwrap();
-    assert!(
-        state["entries"][PURL].is_object(),
-        "ledger entry must be kept: {state}"
-    );
-    assert!(
-        tmp.path()
-            .join(format!(".socket/vendor/npm/{UUID}"))
-            .exists(),
-        "kept artifacts must survive the orphan sweep"
-    );
-    // The (already left-pad-free) lock stays exactly as the user re-locked
-    // it — the keep never edits a lock it refused to own.
-    assert_eq!(
-        std::fs::read(tmp.path().join("package-lock.json")).unwrap(),
-        lock_bytes
-    );
-
-    // 2. Undo the drift: restore the pre-vendor registry lock, so every
-    //    recorded fragment is converged. The same prune now reclaims fully.
-    std::fs::write(tmp.path().join("package-lock.json"), &original_lock).unwrap();
+    // 1. Vanished lock entry: reverted in one run.
     let v = run_prune();
     assert_eq!(
         v["gc"]["revertedVendoredEntries"],
         serde_json::json!([PURL]),
         "gc must report the reverted entry: {v}"
     );
+    assert_eq!(
+        v["gc"]["keptVendoredEntries"],
+        serde_json::json!([]),
+        "nothing resolves through the artifact, so nothing is kept: {v}"
+    );
 
     // Ledger empty (an emptied state file is removed outright), artifact
     // gone.
@@ -1166,11 +1134,22 @@ async fn scan_prune_reverts_unused_vendored_entry() {
             .exists(),
         "artifact dir removed"
     );
-    // The converged revert restores nothing (the lock already equals every
-    // recorded original), so the restored lock survives byte-for-byte.
+    // The user's re-locked lock is left exactly as they wrote it.
     assert_eq!(
         std::fs::read(tmp.path().join("package-lock.json")).unwrap(),
-        original_lock
+        lock_bytes
+    );
+
+    // 2. Nothing left to reclaim.
+    let v = run_prune();
+    assert_eq!(
+        v["gc"]["revertedVendoredEntries"],
+        serde_json::json!([]),
+        "{v}"
+    );
+    assert_eq!(
+        std::fs::read(tmp.path().join("package-lock.json")).unwrap(),
+        lock_bytes
     );
 }
 
@@ -1288,38 +1267,43 @@ async fn scan_vendor_prune_reconciles_unwired_entry_on_an_empty_crawl() {
     assert_eq!(v["scannedPackages"], 0, "envelope={v}");
     assert_eq!(unwired(&v), 1, "envelope={v}");
     assert!(v.get("gc").is_none(), "no --prune, no GC: {v}");
+    // The detail names the purl and the prune that reverts it.
+    let detail = v["warnings"]
+        .as_array()
+        .into_iter()
+        .flatten()
+        .find(|w| w["code"] == "vendor_ledger_entry_unwired")
+        .and_then(|w| w["detail"].as_str())
+        .unwrap_or_else(|| panic!("envelope={v}"))
+        .to_string();
+    assert!(
+        detail.contains(&format!("({PURL})")) && detail.contains("scan --prune"),
+        "{detail}"
+    );
 
     let (code, stdout, stderr) = run_scan_vendor(tmp.path(), &mock.uri(), &["--prune"]);
     assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}");
     let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("valid JSON");
     assert_eq!(unwired(&v), 0, "the pruning run reconciles instead: {v}");
-    // npm re-locked the entry away, so the wet revert drift-keeps it (see
-    // `scan_prune_reverts_unused_vendored_entry`): the point here is that
-    // the vendored GC ran at all on an empty crawl.
+    // The vendored GC ran on an empty crawl, and since npm re-locked the
+    // entry away (nothing resolves through the artifact) it reverts it
+    // (#665; see `scan_prune_reverts_unused_vendored_entry`).
     assert_eq!(
-        v["gc"]["keptVendoredEntries"],
+        v["gc"]["revertedVendoredEntries"],
         serde_json::json!([PURL]),
         "envelope={v}"
     );
+    assert_eq!(
+        v["gc"]["keptVendoredEntries"],
+        serde_json::json!([]),
+        "envelope={v}"
+    );
 
-    // The drift-kept entry is still unwired, so the next plain rescan warns
-    // again; its detail names the purl and the prune's `GC: kept` report
-    // instead of a lock edit that could unwire other vendored entries.
+    // Reconciled: the next plain rescan has nothing left to warn about.
     let (code, stdout, stderr) = run_scan_vendor(tmp.path(), &mock.uri(), &[]);
     assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}");
     let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("valid JSON");
-    let detail = v["warnings"]
-        .as_array()
-        .into_iter()
-        .flatten()
-        .find(|w| w["code"] == "vendor_ledger_entry_unwired")
-        .and_then(|w| w["detail"].as_str())
-        .unwrap_or_else(|| panic!("envelope={v}"))
-        .to_string();
-    assert!(
-        detail.contains(&format!("({PURL})")) && detail.contains("`GC: kept`"),
-        "{detail}"
-    );
+    assert_eq!(unwired(&v), 0, "envelope={v}");
 }
 
 /// Interactive (non-JSON) `scan --vendor` pre-verifies patch baselines:

crates/socket-patch-core/src/vendor/bun_lock.rs

@@ -912,6 +912,17 @@ pub(crate) async fn revert_bun_opts(
     // ran; the artifact dir stays behind (and the caller keeps the ledger
     // entry), so only the deletion is skipped.
     if !keep_artifact {
+        if super::npm_flavor::keep_artifact_while_lock_references_it(
+            &mut outcome,
+            project_root,
+            &[BUN_LOCK],
+            &entry.uuid,
+            &uuid_dir_rel,
+        )
+        .await
+        {
+            return outcome;
+        }
         // The last npm-family entry leaves `.socket/vendor/npm/` (and
         // `.socket/vendor/`) empty: the shared helper prunes them so a
         // reverted project carries no vendor residue (non-recursive:
@@ -1001,9 +1012,13 @@ fn revert_one_record(
         }
         return;
     }
-    warnings.push(drifted(format!(
-        "lock entry `{key}` no longer exists; nothing to restore"
-    )));
+    // REMOVED, not drifted (#665): `bun remove` dropped the entry. The
+    // caller keeps the artifact only while the lock still resolves
+    // through it.
+    warnings.push(VendorWarning::new(
+        super::LOCK_ENTRY_REMOVED_CODE,
+        format!("lock entry `{key}` no longer exists; nothing to restore"),
+    ));
 }
 
 // ───────────────────────── vendor-specific classification ─────────────────
@@ -3309,8 +3324,13 @@ mod tests {
         assert!(fx.root().join(fx.rel_tgz()).exists(), "artifact kept");
     }
 
+    /// #665: `bun remove left-pad` deleted the vendored entry line, so
+    /// nothing in bun.lock resolves through the artifact any more. A
+    /// vanished entry is not drift: the revert succeeds and removes the
+    /// unreferenced artifact instead of keeping it (and the ledger entry)
+    /// forever.
     #[tokio::test]
-    async fn vanished_entry_key_drift_keeps() {
+    async fn vanished_entry_drops_the_unreferenced_artifact() {
         let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await;
         let (_, entry, _) = expect_done(fx.vendor(false).await);
         let entry = entry.unwrap();
@@ -3329,17 +3349,50 @@ mod tests {
 
         let outcome = revert_bun(&entry, fx.root(), false).await;
         assert!(outcome.success, "{:?}", outcome.error);
+        assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings);
         assert!(
             outcome
                 .warnings
                 .iter()
-                .any(|w| w.code == "vendor_lock_entry_drifted"
+                .any(|w| w.code == "vendor_lock_entry_removed"
                     && w.detail.contains("no longer exists; nothing to restore")),
             "{:?}",
             outcome.warnings
         );
-        assert!(outcome.kept_artifact, "drift-skip keeps the artifact");
+        assert!(!outcome.kept_artifact, "{:?}", outcome.warnings);
         assert_eq!(fx.read_lock().await, without_entry, "nothing rewritten");
+        assert!(
+            !fx.root().join(fx.rel_tgz()).exists(),
+            "unreferenced artifact removed"
+        );
+    }
+
+    /// #665 guard: the recorded entry vanished but another entry line still
+    /// resolves through the artifact, so it is kept like a drift-skip.
+    #[tokio::test]
+    async fn vanished_entry_keeps_the_artifact_while_the_lock_references_it() {
+        let fx = fixture_with(BN3_BEFORE_LOCK, "node_modules/left-pad").await;
+        let (_, entry, _) = expect_done(fx.vendor(false).await);
+        let entry = entry.unwrap();
+        let new_line = entry.wiring[0]
+            .new
+            .as_ref()
+            .and_then(Value::as_str)
+            .unwrap();
+        let live = fx.read_lock().await;
+        // Re-key the entry (`"left-pad"` → `"other/left-pad"`): the
+        // recorded key is gone, the tuple still points into our uuid dir.
+        let rekeyed_line = new_line.replacen("\"left-pad\"", "\"other/left-pad\"", 1);
+        assert_ne!(rekeyed_line, new_line, "the re-key must hit");
+        let rekeyed = live.replace(new_line, &rekeyed_line);
+        tokio::fs::write(fx.root().join(BUN_LOCK), &rekeyed)
+            .await
+            .unwrap();
+
+        let outcome = revert_bun(&entry, fx.root(), false).await;
+        assert!(outcome.success, "{:?}", outcome.error);
+        assert!(outcome.kept_artifact, "{:?}", outcome.warnings);
+        assert_eq!(fx.read_lock().await, rekeyed, "nothing rewritten");
         assert!(fx.root().join(fx.rel_tgz()).exists(), "artifact kept");
     }