From e8b23894dd0b1b4caeb1c06c77c40148c2e34cc6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 12:31:19 +0000 Subject: [PATCH 1/4] Start fix for #665 Assisted-by: Claude Code:claude-opus-5-5 From 8f841c012747fca8a0df6b41d50ed889a5d9709d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 12:51:01 +0000 Subject: [PATCH 2/4] Drop vendored artifact after dependency removal After `npm uninstall`, `yarn remove`, `pnpm remove` or `bun remove` of a vendored package, the revert treated the vanished lock entry as drift. It kept the artifact and ledger entry forever, so `rollback` and `remove` exited 1 on every run, `scan --prune` kept the entry, and the printed remedies looped. A vanished entry now warns `vendor_lock_entry_removed` instead of `vendor_lock_entry_drifted`. The artifact and entry are dropped once no wired file still mentions the uuid dir, and are kept, as before, while one does. Re-resolved entries are still drift-kept. Fixes #665 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 6 +- .../tests/in_process_vendor.rs | 51 +++++++++ .../socket-patch-core/src/vendor/bun_lock.rs | 65 ++++++++++- crates/socket-patch-core/src/vendor/mod.rs | 30 ++++- .../src/vendor/npm_flavor.rs | 25 +++++ .../socket-patch-core/src/vendor/npm_lock.rs | 103 +++++++++++++++++- .../socket-patch-core/src/vendor/pnpm_lock.rs | 70 ++++++++---- .../src/vendor/pnpm_lock_legacy.rs | 81 +++++++++++--- .../src/vendor/yarn_berry_lock.rs | 12 ++ .../src/vendor/yarn_classic_lock.rs | 103 +++++++++++++++++- 10 files changed, 489 insertions(+), 57 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 69fb09df9..8d933ffd9 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/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//` 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 only while a wired file still + mentions the `.socket/vendor/npm//` dir, 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//` 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. diff --git a/crates/socket-patch-cli/tests/in_process_vendor.rs b/crates/socket-patch-cli/tests/in_process_vendor.rs index 1b6b410fc..c0eb71045 100644 --- a/crates/socket-patch-cli/tests/in_process_vendor.rs +++ b/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 // ───────────────────────────────────────────────────────────────────── diff --git a/crates/socket-patch-core/src/vendor/bun_lock.rs b/crates/socket-patch-core/src/vendor/bun_lock.rs index 314429442..24a3dae22 100644 --- a/crates/socket-patch-core/src/vendor/bun_lock.rs +++ b/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"); } diff --git a/crates/socket-patch-core/src/vendor/mod.rs b/crates/socket-patch-core/src/vendor/mod.rs index a2592e400..776e0d460 100644 --- a/crates/socket-patch-core/src/vendor/mod.rs +++ b/crates/socket-patch-core/src/vendor/mod.rs @@ -657,6 +657,10 @@ impl RevertOpts { } } +/// Warning code for a recorded lock entry that no longer exists at revert +/// time (the dependency was removed). See [`RevertOutcome::lock_entry_removed`]. +pub const LOCK_ENTRY_REMOVED_CODE: &str = "vendor_lock_entry_removed"; + /// The result of one backend `revert_*` call. #[derive(Debug)] pub struct RevertOutcome { @@ -695,9 +699,14 @@ impl RevertOutcome { } /// True when any wiring record was left alone during the restore — - /// every left-alone branch (ownership-gate drift, vanished block, - /// missing pre-vendor original, unknown kind/key, allowlist skip) - /// warns with the stable code `vendor_lock_entry_drifted`. + /// every left-alone branch (ownership-gate drift, missing pre-vendor + /// original, unknown kind/key, allowlist skip) warns with the stable + /// code `vendor_lock_entry_drifted`. + /// + /// A recorded lock entry that has VANISHED (the user removed the + /// dependency) is not drift: it warns `vendor_lock_entry_removed` + /// instead (see [`Self::lock_entry_removed`]), and the backend keeps + /// the artifact only while the lock still resolves through it. /// /// LIVENESS CONTRACT: backends must NOT emit that code for a record /// whose live state already equals its reverted state (the fragment @@ -714,6 +723,19 @@ impl RevertOutcome { .any(|w| w.code == "vendor_lock_entry_drifted") } + /// True when any recorded lock entry no longer existed at revert time + /// (`vendor_lock_entry_removed`): the user removed the dependency + /// (`yarn remove`, `npm uninstall`, ...), so there was nothing to + /// restore. Unlike drift, nothing of the user's is being protected, so + /// this alone must not keep the artifact forever (#665). The backend + /// keeps it only while a lockfile still mentions the uuid dir, see + /// `npm_flavor::keep_artifact_while_lock_references_it`. + pub fn lock_entry_removed(&self) -> bool { + self.warnings + .iter() + .any(|w| w.code == LOCK_ENTRY_REMOVED_CODE) + } + /// Mark the artifact dir as deliberately kept after a drift-skip and /// surface it honestly. Backends call this INSTEAD of removing the /// uuid dir when [`Self::drift_skipped`] is true: deleting it would be @@ -726,7 +748,7 @@ impl RevertOutcome { "vendor_artifact_kept", format!( "kept {uuid_dir_rel}: some recorded lock entries were left alone (see the \ - vendor_lock_entry_drifted warnings) and the vendored artifacts may still be \ + vendor_lock_entry_drifted / vendor_lock_entry_removed warnings) and the vendored artifacts may still be \ needed for a later restore; undo the drift (restore the vendored lock entries \ or re-vendor) and re-run `vendor --revert` to finish cleaning up" ), diff --git a/crates/socket-patch-core/src/vendor/npm_flavor.rs b/crates/socket-patch-core/src/vendor/npm_flavor.rs index 667b6ff51..36b3fcdd5 100644 --- a/crates/socket-patch-core/src/vendor/npm_flavor.rs +++ b/crates/socket-patch-core/src/vendor/npm_flavor.rs @@ -654,6 +654,31 @@ pub(super) async fn lock_text_mentions_uuid( any_readable.then_some(false) } +/// The keep gate for recorded lock entries that VANISHED during a revert +/// ([`RevertOutcome::lock_entry_removed`], #665). The user removed the +/// dependency, so there was nothing to restore; the artifact can go once +/// no lockfile in `names` resolves through it any more. While one still +/// mentions the uuid dir (the entry moved to a key the wiring never +/// recorded), or none can be read, the artifact may be the only copy an +/// install needs: keep it exactly like a drift-skip. Returns true when the +/// artifact was kept and the caller must stop before deleting it. +pub(super) async fn keep_artifact_while_lock_references_it( + outcome: &mut RevertOutcome, + project_root: &Path, + names: &[&str], + uuid: &str, + uuid_dir_rel: &str, +) -> bool { + if !outcome.lock_entry_removed() { + return false; + } + if lock_text_mentions_uuid(project_root, names, uuid).await == Some(false) { + return false; + } + outcome.keep_artifact(uuid_dir_rel); + true +} + /// Does this build have a backend for an npm entry's recorded flavor? /// `None` is a pre-flavor (package-lock) ledger. An unknown flavor was /// wired by a newer socket-patch, so health checks and rebuilds must not diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index b1911ba90..0dbdd3ebb 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -780,6 +780,18 @@ pub async fn revert_npm_opts( return outcome; } + if super::npm_flavor::keep_artifact_while_lock_references_it( + &mut outcome, + project_root, + &[SHRINKWRAP, PACKAGE_LOCK], + &entry.uuid, + &uuid_dir_rel, + ) + .await + { + return outcome; + } + // FAIL-CLOSED (same brick class as the unwired guard above): the // restore only rewrites the lock files the wiring names, but the lock // npm actually installs from can still resolve through the artifact — @@ -1116,8 +1128,11 @@ fn revert_one_record( } }; let Some(live) = live else { + // REMOVED, not drifted (#665): `npm uninstall` dropped the entry. + // The caller keeps the artifact only while a lock still resolves + // through it. warnings.push(VendorWarning::new( - "vendor_lock_entry_drifted", + super::LOCK_ENTRY_REMOVED_CODE, format!("lock entry `{key}` no longer exists; nothing to restore"), )); return; @@ -3185,6 +3200,92 @@ mod tests { ); } + /// #665: the user dropped the patched dependency (`npm uninstall + /// left-pad`), so npm deleted every lock entry the wiring recorded and + /// nothing in the lock resolves through the artifact any more. That is + /// not a third-party re-resolution to protect: the revert must succeed, + /// remove the artifact (so the CLI drops the ledger entry) and say why, + /// instead of drift-keeping it forever. + #[tokio::test] + async fn revert_after_dependency_removed_drops_the_unreferenced_artifact() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let mut live = fx.read_lock().await; + let packages = live["packages"].as_object_mut().unwrap(); + packages.remove("node_modules/left-pad"); + packages.remove("node_modules/foo/node_modules/left-pad"); + packages[""]["dependencies"] = json!({}); + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + let uninstalled = tokio::fs::read(fx.lock_path()).await.unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!( + outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_removed" + && w.detail.contains("node_modules/left-pad")), + "the vanished entry is still surfaced: {:?}", + outcome.warnings + ); + assert!( + !fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "nothing resolves through the artifact, so it is removed" + ); + assert_eq!( + tokio::fs::read(fx.lock_path()).await.unwrap(), + uninstalled, + "the user's post-uninstall lock is left byte-identical" + ); + } + + /// #665 guard: a recorded entry vanished but the lock still resolves + /// through the artifact under a key the wiring never recorded (npm + /// re-hoisted it). The artifact may be the only copy that install + /// needs, so it is kept, exactly like a drift-skip. + #[tokio::test] + async fn revert_keeps_artifact_when_a_vanished_entry_moved_to_an_unrecorded_key() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let mut live = fx.read_lock().await; + let packages = live["packages"].as_object_mut().unwrap(); + let wired = packages.remove("node_modules/left-pad").unwrap(); + packages.remove("node_modules/foo/node_modules/left-pad"); + packages.insert("node_modules/bar/node_modules/left-pad".into(), wired); + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.kept_artifact, "{:?}", outcome.warnings); + assert!( + outcome + .warnings + .iter() + .any(|w| w.code == "vendor_artifact_kept"), + "{:?}", + outcome.warnings + ); + assert!( + fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "the lock still resolves through the artifact" + ); + } + /// The WIRED revert (non-empty wiring) fails closed on an unparseable /// lock — editing JSON we cannot parse risks destroying it. (The /// unreadable-lock test below covers only the EMPTY-wiring guard.) diff --git a/crates/socket-patch-core/src/vendor/pnpm_lock.rs b/crates/socket-patch-core/src/vendor/pnpm_lock.rs index 1b08ba52d..d35a8e7be 100644 --- a/crates/socket-patch-core/src/vendor/pnpm_lock.rs +++ b/crates/socket-patch-core/src/vendor/pnpm_lock.rs @@ -999,6 +999,17 @@ pub(super) async fn revert_pnpm_dialect( // 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, + &[PNPM_LOCK, PACKAGE_JSON, PNPM_WORKSPACE], + &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: @@ -3108,7 +3119,7 @@ fn revert_importer_dep( } break; } - warnings.push(drifted(format!( + warnings.push(removed(format!( "importer dep `{key}` no longer exists; nothing to restore" ))); } @@ -3187,7 +3198,7 @@ fn revert_block( j = block.end; } } - warnings.push(drifted(format!( + warnings.push(removed(format!( "{section} entry `{new_key}` no longer exists; nothing to restore" ))); } @@ -3251,7 +3262,7 @@ fn revert_snapshot_ref( } break; } - warnings.push(drifted(format!( + warnings.push(removed(format!( "snapshot ref `{key}` no longer exists; nothing to restore" ))); } @@ -3260,6 +3271,13 @@ pub(super) fn drifted(detail: impl Into) -> VendorWarning { VendorWarning::new("vendor_lock_entry_drifted", detail.into()) } +/// A recorded lock entry that no longer exists (`pnpm remove` dropped the +/// dependency). Not drift (#665): the revert keeps the artifact only while +/// a wired file still resolves through it. +pub(super) fn removed(detail: impl Into) -> VendorWarning { + VendorWarning::new(super::LOCK_ENTRY_REMOVED_CODE, detail.into()) +} + // ────────────────────────── surfaces commit + unwind ────────────────────── /// Write the override surfaces FIRST (package.json, then pnpm-workspace.yaml), @@ -6579,23 +6597,24 @@ snapshots: .await .unwrap(); + // #665: a vanished block is not drift. Once the other recorded + // fragments are restored nothing resolves through the artifact, so + // it is removed instead of kept forever. let outcome = revert_pnpm(&entry, fx.root(), false).await; assert!(outcome.success, "{:?}", outcome.error); assert_warning( &outcome.warnings, - "vendor_lock_entry_drifted", - "no longer exists; nothing to restore", - ); - assert_warning( - &outcome.warnings, - "vendor_lock_entry_drifted", + "vendor_lock_entry_removed", "packages entry `left-pad@file:", ); - assert!(outcome.kept_artifact); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!(!uuid_dir(&fx).exists(), "unreferenced artifact removed"); } - /// Snapshot dep refs: the re-resolved (foreign value) arm and the - /// line-vanished arm both warn and keep. + /// Snapshot dep refs: the re-resolved (foreign value) arm warns and + /// keeps; the line-vanished arm warns `vendor_lock_entry_removed` and, + /// with nothing left resolving through the artifact, removes it (#665). #[tokio::test] async fn snapshot_ref_drift_and_vanished_arms_warn() { // Re-resolved behind our back. @@ -6636,13 +6655,16 @@ snapshots: assert!(outcome.success, "{:?}", outcome.error); assert_warning( &outcome.warnings, - "vendor_lock_entry_drifted", + "vendor_lock_entry_removed", "snapshot ref `consumer@file:consumer|left-pad` no longer exists; nothing to restore", ); - assert!(outcome.kept_artifact); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!(!uuid_dir(&fx).exists(), "unreferenced artifact removed"); } - /// The whole importer dep entry deleted since vendoring: warn + keep. + /// The whole importer dep entry deleted since vendoring: warned as + /// removed, and the artifact goes once nothing resolves through it + /// (#665). #[tokio::test] async fn vanished_importer_dep_entry_warns_nothing_to_restore() { let fx = fixture_with(P1_BEFORE_PKG, P1_BEFORE_LOCK).await; @@ -6665,10 +6687,11 @@ snapshots: assert!(outcome.success, "{:?}", outcome.error); assert_warning( &outcome.warnings, - "vendor_lock_entry_drifted", + "vendor_lock_entry_removed", "importer dep `.|left-pad` no longer exists; nothing to restore", ); - assert!(outcome.kept_artifact); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!(!uuid_dir(&fx).exists(), "unreferenced artifact removed"); } /// The user hand-restored the importer dep to its pre-vendor pair before @@ -7973,7 +7996,7 @@ snapshots: &mut warnings, ); assert!(!dirty); - assert_warning(&warnings, "vendor_lock_entry_drifted", "no longer exists"); + assert_warning(&warnings, "vendor_lock_entry_removed", "no longer exists"); } /// A record whose `original` lost its `specifier`/`version` fields @@ -8015,8 +8038,9 @@ snapshots: } /// A rekeyed block that vanished, where the recorded original ALSO - /// matches no live block, is drift ("no longer exists") — the converged - /// silent return applies only when the original block is live verbatim. + /// matches no live block, is warned as removed ("no longer exists") — + /// the converged silent return applies only when the original block is + /// live verbatim. #[test] fn packages_block_revert_with_no_live_or_original_match_warns_vanished() { let mut lines = @@ -8047,10 +8071,10 @@ snapshots: &mut warnings, ); assert!(!dirty); - assert_warning(&warnings, "vendor_lock_entry_drifted", "no longer exists"); + assert_warning(&warnings, "vendor_lock_entry_removed", "no longer exists"); // Same vanish with NO recorded original (a stripped/reconstructed - // record): the converged probe is skipped — still the same drift + // record): the converged probe is skipped — still the same removed // warning, never a silent pass. let rec = WiringRecord { original: None, @@ -8068,7 +8092,7 @@ snapshots: &mut warnings, ); assert!(!dirty); - assert_warning(&warnings, "vendor_lock_entry_drifted", "no longer exists"); + assert_warning(&warnings, "vendor_lock_entry_removed", "no longer exists"); } /// The unwind helper restores exactly what was written: with no diff --git a/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs b/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs index ca815da75..21e68f6d1 100644 --- a/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs +++ b/crates/socket-patch-core/src/vendor/pnpm_lock_legacy.rs @@ -64,8 +64,8 @@ use crate::utils::fs::read_regular_to_string; use super::common::refused; use super::path::parse_vendor_path; use super::pnpm_lock::{ - dep_field_lines, drifted, lines_value, revert_overrides_line, value_lines, vendor_value_is_for, - EditCtx, PnpmDialect, KIND_LOCK_OVERRIDES, KIND_LOCK_PACKAGE, + dep_field_lines, drifted, lines_value, removed, revert_overrides_line, value_lines, + vendor_value_is_for, EditCtx, PnpmDialect, KIND_LOCK_OVERRIDES, KIND_LOCK_PACKAGE, }; use super::source::PackageSource; use super::state::{VendorEntry, WiringAction, WiringRecord}; @@ -986,7 +986,7 @@ fn revert_value_line( *dirty = true; return; } - warnings.push(drifted(format!( + warnings.push(removed(format!( "{section} entry `{dep}` no longer exists; nothing to restore" ))); } @@ -1069,7 +1069,7 @@ fn revert_root_dep_pair( *dirty = true; return; } - warnings.push(drifted(format!( + warnings.push(removed(format!( "root dep `{key}` no longer exists; nothing to restore" ))); } @@ -1156,7 +1156,7 @@ fn revert_package_block( j = block.end; } } - warnings.push(drifted(format!( + warnings.push(removed(format!( "packages entry `{new_key}` no longer exists; nothing to restore" ))); } @@ -1229,7 +1229,7 @@ fn revert_pkg_dep_ref( } break; } - warnings.push(drifted(format!( + warnings.push(removed(format!( "dep ref `{key}` no longer exists; nothing to restore" ))); } @@ -2782,6 +2782,47 @@ packages: ); } + /// Revert and assert the #665 removed-entry contract: success, no + /// drift, one `vendor_lock_entry_removed` warning containing `want`, + /// and the artifact kept exactly while the lock still resolves through + /// it (`still_referenced`). A removed artifact is put back afterwards + /// so the caller's next matrix case starts from the vendored state. + async fn assert_removed(fx: &Fixture, entry: &VendorEntry, want: &str, still_referenced: bool) { + let uuid_dir = fx.root().join(fx.rel_tgz()).parent().unwrap().to_path_buf(); + let mut saved = Vec::new(); + for file in std::fs::read_dir(&uuid_dir).unwrap() { + let path = file.unwrap().path(); + saved.push((path.clone(), std::fs::read(&path).unwrap())); + } + + let outcome = revert_pnpm_legacy(entry, fx.root(), false).await; + assert!(outcome.success, "{want}: {:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{want}: {:?}", outcome.warnings); + assert!( + outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_removed" && w.detail.contains(want)), + "expected a removed warning containing `{want}`: {:?}", + outcome.warnings + ); + assert_eq!( + outcome.kept_artifact, still_referenced, + "{want}: {:?}", + outcome.warnings + ); + assert_eq!( + fx.root().join(fx.rel_tgz()).exists(), + still_referenced, + "{want}: the artifact goes once nothing resolves through it" + ); + + std::fs::create_dir_all(&uuid_dir).unwrap(); + for (path, bytes) in saved { + std::fs::write(path, bytes).unwrap(); + } + } + /// A dry run previews and records NOTHING through the legacy entry /// point, and a missing patch-target file fails the vendor before any /// pack — both land in the `staged == None` early return with the @@ -3344,8 +3385,9 @@ packages: } /// v5.4 drift matrix: third-party re-resolutions are left alone with ONE - /// precise warning each, vanished fragments/sections warn, and every - /// case keeps the artifact. + /// precise warning each and keep the artifact; vanished sections warn + /// and keep; a vanished fragment warns as removed and the artifact goes + /// once nothing resolves through it (#665). #[tokio::test] async fn reresolved_and_vanished_fragments_drift_keep_v54() { let (fx, entry) = vendored(T7_BEFORE_LOCK).await; @@ -3381,11 +3423,11 @@ packages: tokio::fs::write(fx.root().join(PNPM_LOCK), &tampered) .await .unwrap(); - assert_drift_keep( + assert_removed( &fx, &entry, "specifiers entry `left-pad` no longer exists", - 1, + false, ) .await; rewire(&fx, &wired_pkg, &wired).await; @@ -3412,11 +3454,11 @@ packages: tokio::fs::write(fx.root().join(PNPM_LOCK), &tampered) .await .unwrap(); - assert_drift_keep( + assert_removed( &fx, &entry, "dep ref `file:consumer|left-pad` no longer exists", - 1, + false, ) .await; rewire(&fx, &wired_pkg, &wired).await; @@ -3430,7 +3472,7 @@ packages: .await .unwrap(); let want_block_gone = format!("packages entry `file:{rel}` no longer exists"); - assert_drift_keep(&fx, &entry, &want_block_gone, 1).await; + assert_removed(&fx, &entry, &want_block_gone, false).await; rewire(&fx, &wired_pkg, &wired).await; // (f) the whole specifiers section is gone. @@ -3482,11 +3524,13 @@ packages: tokio::fs::write(fx.root().join(PNPM_LOCK), &tampered) .await .unwrap(); - assert_drift_keep( + // The surviving `specifier:` line still names the artifact, so it + // is kept. + assert_removed( &fx, &entry, "root dep `dependencies|left-pad` no longer exists", - 1, + true, ) .await; rewire(&fx, &wired_pkg, &wired).await; @@ -4350,7 +4394,8 @@ packages: /// Our rekeyed block gone (user re-locked) AND the record's original /// stripped (corrupted ledger): with neither the live block nor a /// converged original to anchor on, the record warns `no longer exists` - /// and the artifact is kept. + /// (removed), and with nothing left resolving through the artifact it is + /// removed (#665). #[tokio::test] async fn missing_block_with_no_recorded_original_warns_no_longer_exists() { let (fx, mut entry) = vendored(T7_BEFORE_LOCK).await; @@ -4369,11 +4414,11 @@ packages: .find(|r| r.kind == KIND_LOCK_PACKAGE) .unwrap(); rec.original = None; - assert_drift_keep( + assert_removed( &fx, &entry, &format!("packages entry `file:{rel}` no longer exists"), - 1, + false, ) .await; } diff --git a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs index 5721d2a51..c5669f77f 100644 --- a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs @@ -857,6 +857,18 @@ pub async fn revert_yarn_berry_opts( return outcome; } + if super::npm_flavor::keep_artifact_while_lock_references_it( + &mut outcome, + project_root, + &[YARN_LOCK, PACKAGE_JSON], + &entry.uuid, + &uuid_dir_rel, + ) + .await + { + return outcome; + } + // FAIL-CLOSED (same brick class as the unwired guard above, twin of // npm_lock's post-restore probe): the restore only rewrites the // fragments the wiring recorded, but yarn can still resolve through the diff --git a/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs b/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs index 70602238b..fce6985e5 100644 --- a/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs @@ -514,6 +514,18 @@ pub async fn revert_yarn_classic_opts( return outcome; } + if super::npm_flavor::keep_artifact_while_lock_references_it( + &mut outcome, + project_root, + &[YARN_LOCK], + &entry.uuid, + &uuid_dir_rel, + ) + .await + { + return outcome; + } + // FAIL-CLOSED (same brick class as the unwired guard above, twin of // npm_lock's and yarn_berry's post-restore probes): the restore only // rewrites the blocks the wiring recorded, but yarn can still resolve @@ -602,8 +614,11 @@ pub(super) fn revert_recorded_block( return false; } } + // REMOVED, not drifted (#665): the user dropped the dependency. + // The caller keeps the artifact only while the lock still + // resolves through it. warnings.push(VendorWarning::new( - "vendor_lock_entry_drifted", + super::LOCK_ENTRY_REMOVED_CODE, format!("{noun} `{key}` no longer exists; nothing to restore"), )); return false; @@ -1959,6 +1974,75 @@ left-pad@^1.3.0: ); } + /// #665: `yarn remove left-pad` deleted the vendored block, so nothing + /// in yarn.lock resolves through the artifact any more. A vanished + /// block is not a re-resolution to protect: the revert must succeed and + /// remove the artifact (letting rollback / `remove` / `scan --prune` + /// drop the ledger entry), instead of drift-keeping it forever. + #[tokio::test] + async fn revert_after_yarn_remove_drops_the_unreferenced_artifact() { + let fx = fixture_with_lock(Y2_BEFORE).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + // What `yarn remove left-pad` leaves behind: the other dependency's + // block only. + let removed = "# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.\n\ + # yarn lockfile v1\n\n\n\ + is-number@7.0.0:\n version \"7.0.0\"\n \ + resolved \"https://registry.yarnpkg.com/is-number/-/is-number-7.0.0.tgz\"\n"; + tokio::fs::write(fx.lock_path(), removed).await.unwrap(); + + for _ in 0..2 { + let outcome = revert_yarn_classic(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!( + !fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "nothing resolves through the artifact, so it is removed" + ); + assert_eq!( + fx.lock_text().await, + removed, + "the user's lock is untouched" + ); + } + let outcome = revert_yarn_classic(&entry, fx.root(), false).await; + assert!( + outcome + .warnings + .iter() + .any(|w| w.code == "vendor_lock_entry_removed"), + "the vanished block is surfaced: {:?}", + outcome.warnings + ); + } + + /// #665 guard: the recorded block vanished, but another (hand-copied or + /// re-keyed) block still resolves through the artifact. Deleting it + /// would break that install, so the artifact is kept, as for drift. + #[tokio::test] + async fn revert_keeps_artifact_when_a_vanished_block_was_rekeyed() { + let fx = fixture_with_lock(Y2_BEFORE).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let text = fx + .lock_text() + .await + .replace("left-pad@^1.3.0:", "left-pad@1.3.0:"); + tokio::fs::write(fx.lock_path(), &text).await.unwrap(); + + let outcome = revert_yarn_classic(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.kept_artifact, "{:?}", outcome.warnings); + assert!(fx.tgz_path().exists(), "the re-keyed block still needs it"); + assert_eq!(fx.lock_text().await, text); + } + #[tokio::test] async fn revert_allowlist_fails_closed_on_foreign_files() { let fx = fixture_with_lock(Y2_BEFORE).await; @@ -2523,7 +2607,8 @@ left-pad@^1.3.0: /// Direct matrix over [`revert_recorded_block`]'s degradation arms /// (shared with the berry backend): every poisoned-or-newer-ledger - /// record must degrade to a `vendor_lock_entry_drifted` warning and + /// record must degrade to a `vendor_lock_entry_drifted` warning (a + /// vanished block to `vendor_lock_entry_removed`) and /// leave the text byte-untouched, the converged-under-a-different-key /// arm must stay silent, and (control) a well-formed record restores. #[test] @@ -2587,7 +2672,9 @@ left-pad@^1.3.0: assert_drift_warning(&warnings, "unknown wiring kind `future_kind`"); // (c) Recorded key gone AND the original block not live anywhere: - // deleted-since-vendoring drift. + // the dependency was removed since vendoring. Not drift (#665): it + // warns `vendor_lock_entry_removed`, and the caller keeps the + // artifact only while the lock still resolves through it. let absent = vec!["absent@^9:".to_string(), " version \"9.0.0\"".to_string()]; let rec = wrec( Some("absent@^9"), @@ -2597,7 +2684,15 @@ left-pad@^1.3.0: let (changed, text, warnings) = run(Y2_AFTER, &rec); assert!(!changed); assert_eq!(text, Y2_AFTER); - assert_drift_warning(&warnings, "no longer exists; nothing to restore"); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert_eq!(warnings[0].code, super::super::LOCK_ENTRY_REMOVED_CODE); + assert!( + warnings[0] + .detail + .contains("no longer exists; nothing to restore"), + "{}", + warnings[0].detail + ); // (d) ALREADY CONVERGED under a different key: the recorded key is // gone but the original block is live verbatim — a silent no-op so From 6bad4f905b47f471360c50fa48df53ab4b244467 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 13:01:39 +0000 Subject: [PATCH 3/4] Expect prune to reclaim removed vendored deps The two scan --prune e2e tests encoded the old behavior that an uninstalled vendored dependency is drift-kept forever. With #665 the prune now reverts it in one run, so the tests assert that instead and check the unwired warning before the prune. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/scan_vendor_e2e.rs | 120 ++++++++---------- 1 file changed, 52 insertions(+), 68 deletions(-) diff --git a/crates/socket-patch-cli/tests/scan_vendor_e2e.rs b/crates/socket-patch-cli/tests/scan_vendor_e2e.rs index 97a194e5a..2f0208055 100644 --- a/crates/socket-patch-cli/tests/scan_vendor_e2e.rs +++ b/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::(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: From 43a6967b1facd18b203bac99f086bf59c5e92028 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 14:36:08 +0000 Subject: [PATCH 4/4] Require proof before deleting a removed entry's artifact The #665 gate deleted a vendored artifact once one readable lockfile lacked the literal `.socket/vendor/npm//` path. That missed a lock whose resolution uses JSON-escaped slashes, and it ignored an alternate lockfile (npm-shrinkwrap.json) that exists but cannot be read, so the next fresh install failed with ENOENT. Deletion now needs every existing wired file to be read and to not mention the uuid in any spelling: only NotFound counts as absent, the match is on the uuid case-insensitively, JSON is parsed so \u escapes are decoded, and text with an escape the scan cannot see fails closed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BY1gbCU7vLkvqCmxRY9FF4 --- crates/socket-patch-cli/CLI_CONTRACT.md | 4 +- .../src/vendor/npm_flavor.rs | 71 ++++++++++++++++-- .../socket-patch-core/src/vendor/npm_lock.rs | 72 +++++++++++++++++++ 3 files changed, 138 insertions(+), 9 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index 8d933ffd9..ad0d44ab5 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -719,8 +719,8 @@ worse, lets a warm cache silently serve unpatched bytes): `.socket/vendor//` dir — a file that no longer references it is warned about and 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 only while a wired file still - mentions the `.socket/vendor/npm//` dir, so `rollback` / `remove` / `scan --prune` clean up + `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//` and `.socket/vendor/` levels — `.socket/` itself is removed by the lock guard when nothing else is diff --git a/crates/socket-patch-core/src/vendor/npm_flavor.rs b/crates/socket-patch-core/src/vendor/npm_flavor.rs index 36b3fcdd5..39c9378b7 100644 --- a/crates/socket-patch-core/src/vendor/npm_flavor.rs +++ b/crates/socket-patch-core/src/vendor/npm_flavor.rs @@ -656,12 +656,13 @@ pub(super) async fn lock_text_mentions_uuid( /// The keep gate for recorded lock entries that VANISHED during a revert /// ([`RevertOutcome::lock_entry_removed`], #665). The user removed the -/// dependency, so there was nothing to restore; the artifact can go once -/// no lockfile in `names` resolves through it any more. While one still -/// mentions the uuid dir (the entry moved to a key the wiring never -/// recorded), or none can be read, the artifact may be the only copy an -/// install needs: keep it exactly like a drift-skip. Returns true when the -/// artifact was kept and the caller must stop before deleting it. +/// dependency, so there was nothing to restore; the artifact can go only +/// once [`uuid_proven_unreferenced`] shows no file in `names` resolves +/// through it any more. Anything short of that proof (the entry moved to a +/// key the wiring never recorded, a lock that cannot be read) keeps the +/// artifact exactly like a drift-skip, since it may be the only copy an +/// install needs. Returns true when the artifact was kept and the caller +/// must stop before deleting it. pub(super) async fn keep_artifact_while_lock_references_it( outcome: &mut RevertOutcome, project_root: &Path, @@ -672,13 +673,69 @@ pub(super) async fn keep_artifact_while_lock_references_it( if !outcome.lock_entry_removed() { return false; } - if lock_text_mentions_uuid(project_root, names, uuid).await == Some(false) { + if uuid_proven_unreferenced(project_root, names, uuid).await { return false; } outcome.keep_artifact(uuid_dir_rel); true } +/// True only when at least one file in `names` exists, every existing one +/// was read, and none of them references `uuid`. Stricter than +/// [`lock_text_mentions_uuid`] because `true` here deletes the artifact: +/// +/// - Only a missing file (`NotFound`) counts as absent. A lock that exists +/// but cannot be read (permissions, invalid UTF-8, not a regular file) +/// may be the one an install resolves through, so it fails closed. +/// - The match is on the uuid alone, ASCII case-insensitively, so any +/// spelling of the artifact path counts (`\/`-escaped JSON slashes, a +/// backslash separator, a different case on a case-insensitive disk). +/// - JSON files are also parsed and every key and string value checked, +/// which decodes `\u` escapes. A file that might hide the uuid behind an +/// escape the raw scan cannot see (a `\u`/`\x`/`\U` sequence in YAML, +/// yarn.lock or JSON that does not parse) fails closed. +async fn uuid_proven_unreferenced(project_root: &Path, names: &[&str], uuid: &str) -> bool { + let mut any_present = false; + for name in names { + match read_regular_to_string(&project_root.join(name)).await { + Ok(text) => { + if text_may_reference_uuid(name, &text, uuid) { + return false; + } + any_present = true; + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(_) => return false, + } + } + any_present +} + +fn text_may_reference_uuid(name: &str, text: &str, uuid: &str) -> bool { + let needle = uuid.to_ascii_lowercase(); + if text.to_ascii_lowercase().contains(&needle) { + return true; + } + if name.ends_with(".json") { + if let Ok(value) = serde_json::from_str::(text) { + return json_mentions(&value, &needle); + } + } + ["\\u", "\\x", "\\U"].iter().any(|esc| text.contains(esc)) +} + +fn json_mentions(value: &serde_json::Value, needle: &str) -> bool { + let hit = |s: &str| s.to_ascii_lowercase().contains(needle); + match value { + serde_json::Value::String(s) => hit(s), + serde_json::Value::Array(items) => items.iter().any(|v| json_mentions(v, needle)), + serde_json::Value::Object(map) => { + map.iter().any(|(k, v)| hit(k) || json_mentions(v, needle)) + } + _ => false, + } +} + /// Does this build have a backend for an npm entry's recorded flavor? /// `None` is a pre-flavor (package-lock) ledger. An unknown flavor was /// wired by a newer socket-patch, so health checks and rebuilds must not diff --git a/crates/socket-patch-core/src/vendor/npm_lock.rs b/crates/socket-patch-core/src/vendor/npm_lock.rs index 0dbdd3ebb..4beee71ff 100644 --- a/crates/socket-patch-core/src/vendor/npm_lock.rs +++ b/crates/socket-patch-core/src/vendor/npm_lock.rs @@ -3286,6 +3286,78 @@ mod tests { ); } + /// #665 guard, escaped spelling: the moved entry's resolution is + /// written with JSON-escaped slashes (`.socket\/vendor\/npm\/...`), + /// which parses to the same path npm installs from. A literal-path scan + /// misses it, so the gate matches the uuid itself. + #[tokio::test] + async fn revert_keeps_artifact_when_a_moved_entry_uses_escaped_slashes() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let mut live = fx.read_lock().await; + let packages = live["packages"].as_object_mut().unwrap(); + let wired = packages.remove("node_modules/left-pad").unwrap(); + packages.remove("node_modules/foo/node_modules/left-pad"); + packages.insert("node_modules/bar/node_modules/left-pad".into(), wired); + let text = String::from_utf8(serialize_json(&live, " ").unwrap()) + .unwrap() + .replace(".socket/vendor/npm/", ".socket\\/vendor\\/npm\\/"); + assert!(!text.contains(".socket/vendor/npm/"), "{text}"); + tokio::fs::write(fx.lock_path(), text).await.unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.kept_artifact, "{:?}", outcome.warnings); + assert!( + fx.root().join(fx.expected_rel_tgz()).exists(), + "the escaped lock still resolves through the artifact" + ); + } + + /// #665 guard, unreadable alternate lock: package-lock.json no longer + /// has the entry, but an npm-shrinkwrap.json that exists and cannot be + /// read may be the lock an install resolves through. One readable, + /// clean lock is not proof of absence, so the artifact is kept. + #[tokio::test] + async fn revert_keeps_artifact_when_an_alternate_lock_is_unreadable() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let mut live = fx.read_lock().await; + let packages = live["packages"].as_object_mut().unwrap(); + let wired = packages.remove("node_modules/left-pad").unwrap(); + packages.remove("node_modules/foo/node_modules/left-pad"); + tokio::fs::write(fx.lock_path(), serialize_json(&live, " ").unwrap()) + .await + .unwrap(); + // Invalid UTF-8 makes the shrinkwrap unreadable even as root, where + // a permission bit would not. + let mut shrinkwrap = b"\xff".to_vec(); + shrinkwrap.extend(serde_json::to_vec(&wired).unwrap()); + tokio::fs::write(fx.root().join(SHRINKWRAP), shrinkwrap) + .await + .unwrap(); + + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.kept_artifact, "{:?}", outcome.warnings); + assert!( + fx.root().join(fx.expected_rel_tgz()).exists(), + "an unreadable lock is not proof the artifact is unused" + ); + + // Once the shrinkwrap is gone, the same revert reclaims it. + tokio::fs::remove_file(fx.root().join(SHRINKWRAP)) + .await + .unwrap(); + let outcome = revert_npm(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.kept_artifact, "{:?}", outcome.warnings); + assert!(!fx.root().join(fx.expected_rel_tgz()).exists()); + } + /// The WIRED revert (non-empty wiring) fails closed on an unparseable /// lock — editing JSON we cannot parse risks destroying it. (The /// unreadable-lock test below covers only the EMPTY-wiring guard.)