Skip to content

Commit dfa7f42

Browse files
Fix vendored npm/Bun revert treating an upgrade as drift (#1155) (#1187)
* Revert a vendored npm/Bun package after upgrade Moving a vendored package to another version (`npm install pkg@x`, `bun update`, a Dependabot bump) left the vendored copy and its ledger entry stuck. `scan --prune`, `vendor --revert`, `remove` and `rollback` called the moved lock entry "drift" and kept everything, so `vendor --check` stayed red and every remedy it named looped. A lock entry that now locks a different version than the one vendored means the vendored version left the lock graph, the same as after `npm uninstall`. The npm and Bun text-lock reverts now report it as `vendor_lock_entry_removed`, leave the user's lock untouched, and delete the artifact once no lock resolves through it. A re-resolution at the same version is still drift and still keeps the artifact. Fixes #1155 Assisted-by: Claude Code:claude-opus-5-5 * Test scan --prune after a vendored npm upgrade Covers the `scan --prune` leg of #1155 end to end: after `npm install left-pad@1.3.1` over a vendored 1.3.0, one prune run reverts the entry, removes the artifact and leaves the user's lock byte-identical. Assisted-by: Claude Code:claude-opus-5-5 * Only treat same-registry version moves as upgrades A lock entry that kept the vendored key but changed its version was reverted as an upgrade no matter where it resolved. An edited lock could point that entry at any tarball, and `scan --prune`, `remove` or `rollback` would then delete the vendored copy and turn `vendor --check` green. An upgrade now has to be the same package from the registry the pre-vendor entry used: for npm the new `resolved` must share the original's `<registry>/<name>/-/` tarball directory, and for Bun the spec's package name and the tuple's registry field must match. Anything else stays drift, keeps the artifact and leaves `vendor --check` red. Assisted-by: Claude Code:claude-opus-5-5 * Accept http-to-https registry upgrades Older npm locks record `http://` registry tarball URLs, and the next `npm install` rewrites them to `https://`. An upgrade made that way was still called drift, so the vendored copy stayed stuck as in #1155. A move from `http` to `https` on the same registry now counts as an upgrade; a move from `https` down to `http` stays drift. Assisted-by: Claude Code:claude-opus-5-5 * Require the exact tarball name for upgrades An upgraded npm entry was accepted when its `resolved` sat under the recorded registry directory and ended in `.tgz`. A URL written with backslashes or `..` passed that check, but npm normalizes it before fetching, so it could point at another package's tarball while the vendored copy was deleted. The tarball file must now be exactly `<name>-<version>.tgz`, with the name taken from the pre-vendor tarball and a plain version; anything else stays drift. Assisted-by: Claude Code:claude-opus-5-5 * Read legacy npm alias versions on upgrade Lockfile v1 alias rows store their version as `npm:left-pad@1.3.0`, so the new exact tarball-name check never matched them and a real alias upgrade was still kept as drift. The tarball version is now read from after the last `@` of an `npm:` spec before the same strict check runs. Assisted-by: Claude Code:claude-opus-5-5 * Keep non-registry installs out of upgrades An entry npm installs from a git, URL or `file:` spec could still be read as a registry upgrade when its lock `resolved` was written in the registry tarball shape. npm ci installs such an edge from the spec, so the revert deleted the vendored copy while something else got installed. The revert now reads the same non-registry edge set the vendor scan uses (including a registry override's rescue, #490) and keeps any such entry as drift. Assisted-by: Claude Code:claude-opus-5-5 * Distrust upgrades a project config can redirect Two more ways an edited project could make the revert delete a vendored copy while something else gets installed: - npm: a package.json override can swap a registry edge for a git, URL or file: spec, which npm ci installs instead of the lock's resolved tarball. While any override names the vendored package, a version change now stays drift. - Bun: an empty registry field means the default registry, which a committed bunfig.toml or .npmrc can rebind. When either file names a registry, an upgrade from the default registry now stays drift. Both fall back to the pre-#1155 behavior, which keeps the vendored copy, whenever the project's own config could change the install source. Assisted-by: Claude Code:claude-opus-5-5 * Trust upgrades only without redirecting config Review found more ways committed project config can change where an "upgraded" package installs from, while the revert deletes the vendored copy: a project .npmrc registry or replace-registry-host rewrites npmjs dist URLs at fetch time, a Bun [install.scopes] table rebinds a scope, and an npm override keyed on an alias edge swaps its source. Instead of matching each spelling, the upgrade shortcut now applies only when the project has no such config at all: no `overrides` in package.json, and no .npmrc or bunfig.toml that mentions a registry or a scope (or can't be read). Otherwise the revert drift-keeps exactly as before #1155. Assisted-by: Claude Code:claude-opus-5-5 * Fail closed on any project or env registry config The upgrade shortcut skipped a project .npmrc that only set a proxy, `strict-ssl=false` or a CA file, since it looked for the words "registry" and "scope". Those settings can also change what npm fetches. Any setting at all in the project .npmrc or bunfig.toml, or a `NPM_CONFIG_REGISTRY` / `npm_config_registry` / `BUN_CONFIG_REGISTRY` in the environment, now keeps a version move as drift. A file of only comments or blank lines still counts as no config. Assisted-by: Claude Code:claude-opus-5-5 * Fix the build after merging main's bun.lockb rework #1147 landed a bun.lockb revert that calls bun_lock::revert_one_record for a migrated text record, and main moved npm_lock's npm_origin import. Both broke against this branch in the merge queue (clippy: E0061, E0425). Pass `false` for the new default-registry argument from the bun.lockb path, which keeps a moved default-registry tuple there as drift: bun.lockb stays outside the #1155 upgrade path. Import legacy_packages_key again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mo7HM9gyRkUAMxWqi62Dxz * Repin live minimist@1.2.2 suites to republished patch 642d7f02 Port of #1301 (fixes #1293). Production withdrew the free minimist@1.2.2 patch 80630680 and republished the fix as 642d7f02, which turned hosted-e2e, e2e_safety_pnpm and every Bun native leg red here as on main. The vlt harness also now reads the republished patch's unprefixed file keys. Test-only; a no-op once main carries #1301. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mo7HM9gyRkUAMxWqi62Dxz --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2677eda commit dfa7f42

7 files changed

Lines changed: 993 additions & 4 deletions

File tree

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

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -543,6 +543,88 @@ async fn rollback_after_dependency_removed_cleans_up_and_converges() {
543543
assert!(events(&env).is_empty(), "nothing left to revert: {env:#}");
544544
}
545545

546+
/// What `npm install left-pad@1.3.1` leaves behind after vendoring 1.3.0:
547+
/// the same lock key, now resolving the new version from the registry.
548+
fn upgrade_vendored_left_pad(fx: &NpmFixture) -> Vec<u8> {
549+
let mut lock = fx.lock_value();
550+
lock["packages"]["node_modules/left-pad"] = json!({
551+
"version": "1.3.1",
552+
"resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz",
553+
"integrity": "sha512-upgraded=="
554+
});
555+
let mut upgraded = serde_json::to_vec_pretty(&lock).unwrap();
556+
upgraded.push(b'\n');
557+
std::fs::write(fx.lock_path(), &upgraded).unwrap();
558+
upgraded
559+
}
560+
561+
/// #1155: moving a vendored package off its patched version (`npm install
562+
/// left-pad@1.3.1`, a Dependabot bump) takes the vendored version out of
563+
/// the lock graph just like `npm uninstall`. `rollback` used to call that
564+
/// drift, keep the artifact and ledger entry and exit 1 on every run, so
565+
/// `vendor --check` stayed red. Now the first rollback cleans up, leaves
566+
/// the user's upgraded lock alone, and later runs are clean no-ops.
567+
#[tokio::test]
568+
async fn rollback_after_version_upgrade_cleans_up_and_converges() {
569+
let fx = npm_fixture();
570+
assert_eq!(vendor_run(vendor_args(fx.root())).await, 0);
571+
let upgraded = upgrade_vendored_left_pad(&fx);
572+
573+
let cwd = fx.root().to_str().unwrap();
574+
let (code, stdout, stderr) = run_cli(
575+
fx.root(),
576+
&["rollback", "--json", "--yes", "--offline", "--cwd", cwd],
577+
&[],
578+
);
579+
assert_eq!(code, 0, "rollback must succeed:\n{stdout}\n{stderr}");
580+
assert!(
581+
!stdout.contains("vendor_artifact_kept") && !stdout.contains("drifted"),
582+
"nothing is kept as drift:\n{stdout}"
583+
);
584+
assert!(
585+
!fx.vendor_dir().exists(),
586+
"the unreferenced artifact and ledger are cleaned up:\n{stdout}"
587+
);
588+
assert_eq!(fx.lock_bytes(), upgraded, "the user's lock is untouched");
589+
590+
let (code, env) = vendor_cli(fx.root(), &["--revert"]);
591+
assert_eq!(code, 0, "{env:#}");
592+
assert!(events(&env).is_empty(), "nothing left to revert: {env:#}");
593+
let (code, env) = vendor_cli(fx.root(), &["--check"]);
594+
assert_eq!(code, 0, "vendor --check is green again: {env:#}");
595+
}
596+
597+
/// #1155, `remove` leg: it used to end on "drift-kept …; re-run `scan
598+
/// --mode vendored` to normalize, then remove again", a remedy that
599+
/// changes nothing. Now it reverts the entry and exits 0.
600+
#[tokio::test]
601+
async fn remove_after_version_upgrade_reverts_vendoring() {
602+
let fx = npm_fixture();
603+
assert_eq!(vendor_run(vendor_args(fx.root())).await, 0);
604+
let upgraded = upgrade_vendored_left_pad(&fx);
605+
606+
let (code, stdout, stderr) = run_cli(
607+
fx.root(),
608+
&[
609+
"remove",
610+
PURL,
611+
"--json",
612+
"--offline",
613+
"--yes",
614+
"--cwd",
615+
fx.root().to_str().unwrap(),
616+
],
617+
&[],
618+
);
619+
assert_eq!(code, 0, "remove must succeed:\n{stdout}\n{stderr}");
620+
let env: Value = serde_json::from_str(&stdout).unwrap();
621+
let reverted = find_event(&env, "removed", Some("vendor_reverted"));
622+
assert_eq!(reverted["purl"], PURL);
623+
assert!(!stdout.contains("drift-kept"), "{env:#}");
624+
assert!(!fx.vendor_dir().exists(), "vendor tree fully removed");
625+
assert_eq!(fx.lock_bytes(), upgraded, "the user's lock is untouched");
626+
}
627+
546628
// ─────────────────────────────────────────────────────────────────────
547629
// 5. revert works without a manifest
548630
// ─────────────────────────────────────────────────────────────────────

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

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1161,6 +1161,97 @@ async fn scan_prune_reverts_unused_vendored_entry() {
11611161
);
11621162
}
11631163

1164+
/// #1155: `npm install left-pad@1.3.1` after vendoring 1.3.0 keeps the
1165+
/// `node_modules/left-pad` key but locks the new version from the
1166+
/// registry. The vendored version left the lock graph just as after
1167+
/// `npm uninstall`, so `scan --prune` must revert the entry in one run.
1168+
/// It used to call the moved entry drift and keep it forever, so the
1169+
/// prune remedy `vendor --check` names never converged.
1170+
#[tokio::test]
1171+
async fn scan_prune_reverts_vendored_entry_after_version_upgrade() {
1172+
let mock = MockServer::start().await;
1173+
mount_patch_api(&mock, UUID).await;
1174+
let tmp = tempfile::tempdir().unwrap();
1175+
write_fixture(tmp.path());
1176+
1177+
let (code, stdout, stderr) = run_scan_vendor(tmp.path(), &mock.uri(), &[]);
1178+
assert_eq!(code, 0, "stdout={stdout}; stderr={stderr}");
1179+
assert!(tmp
1180+
.path()
1181+
.join(format!(".socket/vendor/npm/{UUID}"))
1182+
.exists());
1183+
1184+
// What `npm install left-pad@1.3.1` leaves behind.
1185+
let lock = serde_json::json!({
1186+
"name": "scan-vendor-test",
1187+
"version": "0.0.0",
1188+
"lockfileVersion": 3,
1189+
"requires": true,
1190+
"packages": {
1191+
"": {
1192+
"name": "scan-vendor-test",
1193+
"version": "0.0.0",
1194+
"dependencies": { "left-pad": "^1.3.1" }
1195+
},
1196+
"node_modules/left-pad": {
1197+
"version": "1.3.1",
1198+
"resolved": "https://registry.npmjs.org/left-pad/-/left-pad-1.3.1.tgz",
1199+
"integrity": "sha512-upgraded==",
1200+
"license": "WTFPL"
1201+
}
1202+
}
1203+
});
1204+
let mut lock_bytes = serde_json::to_vec_pretty(&lock).unwrap();
1205+
lock_bytes.push(b'\n');
1206+
std::fs::write(tmp.path().join("package-lock.json"), &lock_bytes).unwrap();
1207+
std::fs::write(
1208+
tmp.path().join("node_modules/left-pad/package.json"),
1209+
br#"{"name":"left-pad","version":"1.3.1"}"#,
1210+
)
1211+
.unwrap();
1212+
1213+
let out = Command::new(binary())
1214+
.args([
1215+
"scan",
1216+
"--json",
1217+
"--prune",
1218+
"--yes",
1219+
"--api-url",
1220+
&mock.uri(),
1221+
"--api-token",
1222+
"fake-token",
1223+
"--org",
1224+
ORG_SLUG,
1225+
])
1226+
.current_dir(tmp.path())
1227+
.output()
1228+
.expect("run");
1229+
let stdout = String::from_utf8_lossy(&out.stdout).into_owned();
1230+
assert_eq!(out.status.code(), Some(0), "stdout={stdout}");
1231+
let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("valid JSON");
1232+
assert_eq!(
1233+
v["gc"]["revertedVendoredEntries"],
1234+
serde_json::json!([PURL]),
1235+
"gc must revert the upgraded-away entry: {v}"
1236+
);
1237+
assert_eq!(
1238+
v["gc"]["keptVendoredEntries"],
1239+
serde_json::json!([]),
1240+
"nothing resolves through the artifact, so nothing is kept: {v}"
1241+
);
1242+
assert!(
1243+
!tmp.path()
1244+
.join(format!(".socket/vendor/npm/{UUID}"))
1245+
.exists(),
1246+
"artifact dir removed"
1247+
);
1248+
assert_eq!(
1249+
std::fs::read(tmp.path().join("package-lock.json")).unwrap(),
1250+
lock_bytes,
1251+
"the user's upgraded lock is left exactly as they wrote it"
1252+
);
1253+
}
1254+
11641255
/// #541, npm package-lock flavor: after `npm uninstall left-pad` re-locks
11651256
/// the project without the vendored dependency, a vendored rescan skips
11661257
/// the stale ledger entry with a `vendor_ledger_entry_unwired` warning

‎crates/socket-patch-core/src/vendor/bun_binary.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -644,10 +644,14 @@ pub(crate) async fn revert(entry: &VendorEntry, root: &Path, opts: RevertOpts) -
644644
if let RevertLock::Migrated(lines) = &mut lock {
645645
if rec.file == TEXT_LOCK && rec.kind == super::bun_lock::KIND_LOCK_PACKAGE {
646646
let mut dirty = false;
647+
// `false`: bun.lockb migrations stay outside the #1155
648+
// upgrade path, so a moved default-registry tuple keeps
649+
// its drift verdict here.
647650
super::bun_lock::revert_one_record(
648651
lines,
649652
rec,
650653
&entry.uuid,
654+
false,
651655
&mut dirty,
652656
&mut outcome.warnings,
653657
);

0 commit comments

Comments
 (0)