diff --git a/crates/socket-patch-cli/tests/e2e_vendor_yarn_classic_dev_flow.rs b/crates/socket-patch-cli/tests/e2e_vendor_yarn_classic_dev_flow.rs index 5dd8c95d1..0f51f0748 100644 --- a/crates/socket-patch-cli/tests/e2e_vendor_yarn_classic_dev_flow.rs +++ b/crates/socket-patch-cli/tests/e2e_vendor_yarn_classic_dev_flow.rs @@ -817,3 +817,167 @@ fn yarn_classic_drift_skipped_revert_keeps_artifacts_and_recovers() { ); eprintln!("FULL RECOVERY OK"); } + +/// #692: a workspace member adds a second range of the vendored package +/// after vendoring, so yarn dedupes it into the wired block and re-keys it +/// (`left-pad@^1.1.0, left-pad@^1.3.0:`) while keeping our `resolved`. +/// `rollback` must still restore the registry resolution, under the merged +/// key, and drop the vendored artifact; it used to report the block as gone +/// and exit 1 on every run with the lock still wired. +#[test] +fn yarn_classic_rollback_after_member_add_merges_the_vendored_key() { + const LEG: &str = "yarn_classic_rollback_after_member_add_merges_the_vendored_key"; + if !yarn_ready(LEG) { + return; + } + + let tmp = tempfile::tempdir().unwrap(); + let proj = tmp.path().join("proj"); + for member in ["a", "b"] { + std::fs::create_dir_all(proj.join(member)).unwrap(); + } + std::fs::write( + proj.join("package.json"), + r#"{"name":"yarn-classic-692","private":true,"workspaces":["a","b"]}"#, + ) + .unwrap(); + std::fs::write( + proj.join("a/package.json"), + format!(r#"{{"name":"a","version":"1.0.0","dependencies":{{"{DEP}":"^1.1.0"}}}}"#), + ) + .unwrap(); + std::fs::write( + proj.join("b/package.json"), + r#"{"name":"b","version":"1.0.0"}"#, + ) + .unwrap(); + + let cache = tmp.path().join("yarn-cache"); + let yarn_env = [("YARN_CACHE_FOLDER", cache.to_str().unwrap())]; + let install = corepack( + &proj, + &yarn_classic(), + &["install", "--no-progress"], + &yarn_env, + ); + if !install.status.success() { + yarn_classic_vex::skip( + LEG, + &format!( + "fixture `yarn install` failed (registry unreachable?):\n{}", + String::from_utf8_lossy(&install.stderr) + ), + ); + return; + } + + let installed_index = proj.join("node_modules").join(DEP).join("index.js"); + let orig = std::fs::read(&installed_index).expect("installed index.js"); + let patched: Vec = [MARKER.as_bytes(), orig.as_slice()].concat(); + let purl = format!("pkg:npm/{DEP}@{DEP_VERSION}"); + stage_patch(&proj, &purl, "package/index.js", &orig, &patched); + let lock_path = proj.join("yarn.lock"); + let lock_pristine = std::fs::read_to_string(&lock_path).unwrap(); + assert!( + lock_pristine.contains(&format!("\n{DEP}@^1.1.0:\n")), + "fixture lock keys the block by the one range:\n{lock_pristine}" + ); + + let cwd = proj.to_str().unwrap(); + let (code, stdout, stderr) = + run_socket(&proj, &["vendor", "--json", "--offline", "--cwd", cwd]); + assert_eq!( + code, 0, + "vendor failed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let tgz_rel = format!(".socket/vendor/npm/{UUID}/{DEP}-{DEP_VERSION}.tgz"); + assert!(proj.join(&tgz_rel).is_file(), "vendored tarball missing"); + + // Install the vendored tarball, which also puts it in the yarn cache: a + // member's `yarn add` resolves the lock's `file:./` path from the member + // directory and only finds the tarball there (#691). + let frozen = corepack( + &proj, + &yarn_classic(), + &["install", "--frozen-lockfile", "--no-progress"], + &yarn_env, + ); + assert!( + frozen.status.success(), + "frozen install of the vendored lock failed.\nstderr:\n{}", + String::from_utf8_lossy(&frozen.stderr) + ); + assert_eq!(std::fs::read(&installed_index).unwrap(), patched); + + // Member `b` asks for a second range that resolves to the same version: + // yarn merges it into the vendored block. + let add = corepack( + &proj.join("b"), + &yarn_classic(), + &[ + "add", + &format!("{DEP}@^1.3.0"), + "--no-progress", + "--prefer-offline", + ], + &yarn_env, + ); + assert!( + add.status.success(), + "member `yarn add` failed.\nstderr:\n{}", + String::from_utf8_lossy(&add.stderr) + ); + let merged_key = format!("{DEP}@^1.1.0, {DEP}@^1.3.0:"); + let lock_merged = std::fs::read_to_string(&lock_path).unwrap(); + assert!( + lock_merged.contains(&format!("\n{merged_key}\n")), + "yarn must merge the second range into the vendored block:\n{lock_merged}" + ); + assert!( + lock_block(&lock_merged, DEP).contains(&format!("file:./{tgz_rel}#")), + "the merged block must still resolve through the vendored tarball:\n{lock_merged}" + ); + + let (code, stdout, stderr) = run_socket( + &proj, + &["rollback", "--json", "--yes", "--offline", "--cwd", cwd], + ); + assert_eq!( + code, 0, + "rollback of the merged block must succeed.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + let lock_after = std::fs::read_to_string(&lock_path).unwrap(); + assert_eq!( + lock_block(&lock_after, DEP), + lock_block(&lock_pristine, DEP).replacen(&format!("{DEP}@^1.1.0:"), &merged_key, 1), + "the registry resolution is restored under the merged key" + ); + assert!( + !lock_after.contains(".socket/vendor/"), + "nothing may stay wired to the vendored tarball:\n{lock_after}" + ); + assert!( + !proj.join(&tgz_rel).exists(), + "the vendored tarball is removed once nothing resolves through it" + ); + + // The restored lock installs the registry bytes, frozen. + std::fs::remove_dir_all(proj.join("node_modules")).unwrap(); + let frozen = corepack( + &proj, + &yarn_classic(), + &[ + "install", + "--frozen-lockfile", + "--no-progress", + "--prefer-offline", + ], + &yarn_env, + ); + assert!( + frozen.status.success(), + "frozen install of the restored lock failed.\nstderr:\n{}", + String::from_utf8_lossy(&frozen.stderr) + ); + assert_eq!(std::fs::read(&installed_index).unwrap(), orig); +} 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 20a6f0333..2d8633693 100644 --- a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs @@ -907,6 +907,7 @@ pub async fn revert_yarn_berry_opts( KIND_LOCK_ENTRY, "lock entry", |lines| berry_field(lines, "resolution"), + None, &mut outcome.warnings, ); } 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 5190f9b7c..029ae36c7 100644 --- a/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs @@ -690,6 +690,7 @@ pub async fn revert_yarn_classic_opts( KIND_LOCK_BLOCK, "lock block", |lines| classic_field(lines, "resolved"), + Some(split_key_patterns), &mut outcome.warnings, ); } @@ -777,6 +778,15 @@ pub async fn revert_yarn_classic_opts( /// wiring `kind` they own, the noun their warnings use (`lock block` vs /// `lock entry`), and the field carrying the vendor path — `vendor_field` /// reads it (classic `resolved` / berry `resolution`). +/// +/// `key_patterns` (classic only) splits a key line into its patterns. With +/// it, a recorded key that is gone is looked for in a live block sharing +/// one of its patterns (#692): yarn classic re-keys a block when it dedupes +/// another range into it (`left-pad@^1.1.0, left-pad@^1.3.0:`) or drops one, +/// keeping its `resolved`, so the block is still the one we wired. The +/// restore then keeps the live key line. Berry passes `None`: its revert +/// re-keys entries itself and reads the old key as already converged. +#[allow(clippy::too_many_arguments)] pub(super) fn revert_recorded_block( text: &mut String, rec: &WiringRecord, @@ -784,6 +794,7 @@ pub(super) fn revert_recorded_block( expected_kind: &str, noun: &str, vendor_field: fn(&[String]) -> Option<&str>, + key_patterns: Option Vec>, warnings: &mut Vec, ) -> bool { let Some(key) = rec.key.as_deref() else { @@ -805,9 +816,15 @@ pub(super) fn revert_recorded_block( // The recorded pre-vendor block (key line first), used both for the // restore and for the ALREADY-CONVERGED checks below. let orig_lines = rec.original.as_ref().and_then(json_to_lines); + let ours = |lines: &[String]| { + vendor_field(lines) + .and_then(parse_vendor_path) + .is_some_and(|p| p.eco == "npm" && p.uuid == entry_uuid) + }; let edit = { let blocks = scan_blocks(text); - let Some(block) = blocks.iter().find(|b| b.key == key) else { + let rekeyed = || rekeyed_block(&blocks, key, key_patterns?, ours); + let Some(block) = blocks.iter().find(|b| b.key == key).or_else(rekeyed) else { // ALREADY CONVERGED: an earlier partial revert restored this // record, and the restore rekeyed the block (berry's `file:` // locator key reverts to the pre-vendor descriptor), so the @@ -828,6 +845,16 @@ pub(super) fn revert_recorded_block( )); return false; }; + // A block yarn re-keyed (#692) is restored under its live key line, + // so the ranges merged into it since vendoring stay locked. + let orig_lines = orig_lines.map(|mut orig| { + if block.key != key { + if let (Some(first), Some(live)) = (orig.first_mut(), block.lines.first()) { + first.clone_from(live); + } + } + orig + }); // ALREADY CONVERGED: the live block equals the recorded pre-vendor // original — an earlier partial revert (or the user, by hand) // already restored it in place. Not drift. @@ -837,13 +864,13 @@ pub(super) fn revert_recorded_block( // Ownership gate: the live block's vendor field must still point // into OUR uuid dir — anything else means a third party re-resolved // it. - let ours = vendor_field(&block.lines) - .and_then(parse_vendor_path) - .is_some_and(|p| p.eco == "npm" && p.uuid == entry_uuid); - if !ours { + if !ours(&block.lines) { warnings.push(VendorWarning::new( "vendor_lock_entry_drifted", - format!("{noun} `{key}` was re-resolved since vendoring; left alone"), + format!( + "{noun} `{}` was re-resolved since vendoring; left alone", + block.key + ), )); return false; } @@ -854,8 +881,9 @@ pub(super) fn revert_recorded_block( warnings.push(VendorWarning::new( "vendor_lock_entry_drifted", format!( - "{noun} `{key}` has no recorded pre-vendor original; left as-is \ - (re-run `yarn install` to re-resolve it from the registry)" + "{noun} `{}` has no recorded pre-vendor original; left as-is \ + (re-run `yarn install` to re-resolve it from the registry)", + block.key ), )); return false; @@ -869,6 +897,27 @@ pub(super) fn revert_recorded_block( true } +/// The live block a re-keyed `key` now lives in (#692): the one block whose +/// key shares a pattern with `key` and that still resolves through our +/// artifact. A re-keyed block resolved elsewhere is not followed: like a +/// removed one, it no longer needs the artifact, and once nothing in the +/// lock does the revert can finish. +fn rekeyed_block<'b>( + blocks: &'b [LockBlock], + key: &str, + key_patterns: fn(&str) -> Vec, + ours: impl Fn(&[String]) -> bool, +) -> Option<&'b LockBlock> { + let recorded = key_patterns(key); + let mut owned = blocks + .iter() + .filter(|b| ours(&b.lines) && key_patterns(&b.key).iter().any(|p| recorded.contains(p))); + match (owned.next(), owned.next()) { + (Some(one), None) => Some(one), + _ => None, + } +} + // ─────────────────────────── block classification ─────────────────────────── enum BlockClass { @@ -2467,6 +2516,144 @@ left-pad@^1.3.0: ); } + /// `text` with the block keyed `key` re-keyed to `merged`, as yarn + /// classic writes it after deduping another range into that block. + fn merge_key(text: &str, key: &str, merged: &str) -> String { + let out = text.replacen(&format!("\n{key}:\n"), &format!("\n{merged}:\n"), 1); + assert_ne!(out, text, "the key `{key}` must be in the lock"); + out + } + + /// #692: after vendoring, yarn dedupes a second range resolving to the + /// same version into the wired block and rewrites its key line to + /// `left-pad@^1.1.0, left-pad@^1.3.0:`, keeping our `resolved`. The + /// block is still ours: revert must restore the pre-vendor resolution + /// under the merged key and remove the artifact, instead of reporting + /// the block as gone and keeping the lock wired forever. + #[tokio::test] + async fn issue_692_revert_restores_a_block_yarn_merged_after_vendoring() { + let fx = fixture_with_lock(Y2_BEFORE).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + let merged_key = "left-pad@^1.1.0, left-pad@^1.3.0"; + let merged = merge_key(&fx.lock_text().await, "left-pad@^1.3.0", merged_key); + tokio::fs::write(fx.lock_path(), &merged).await.unwrap(); + + let outcome = revert_yarn_classic(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert!(!outcome.kept_artifact); + assert_eq!( + fx.lock_text().await, + merge_key(Y2_BEFORE, "left-pad@^1.3.0", merged_key), + "pre-vendor resolution restored under the merged key" + ); + assert!( + !fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "nothing resolves through the artifact any more, so it is removed" + ); + + // A repeat revert (rollback re-run) is a quiet no-op. + let again = revert_yarn_classic(&entry, fx.root(), false).await; + assert!(again.success, "{:?}", again.error); + assert!(!again.drift_skipped(), "{:?}", again.warnings); + } + + /// #692, the CRLF lock and the shrunk key: yarn drops one range of a + /// merged block after vendoring (`left-pad@^1.3.0, left-pad@~1.3.0` → + /// `left-pad@^1.3.0`). The block still resolves through our artifact, + /// so the revert restores it under its live key, in the lock's own line + /// endings. + #[tokio::test] + async fn issue_692_revert_restores_a_block_whose_key_lost_a_range() { + let before = Y5_BEFORE.replace('\n', "\r\n"); + let fx = fixture_with_lock(&before).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + let shrunk = fx.lock_text().await.replacen( + "\r\nleft-pad@^1.3.0, left-pad@~1.3.0:\r\n", + "\r\nleft-pad@^1.3.0:\r\n", + 1, + ); + tokio::fs::write(fx.lock_path(), &shrunk).await.unwrap(); + + let outcome = revert_yarn_classic(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert_eq!( + fx.lock_text().await, + before.replacen( + "\r\nleft-pad@^1.3.0, left-pad@~1.3.0:\r\n", + "\r\nleft-pad@^1.3.0:\r\n", + 1 + ), + ); + assert!(!fx + .root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists()); + } + + /// #692: re-vendoring a merged block is in sync (nothing to rewrite), + /// and the ledger it keeps still reverts afterwards. + #[tokio::test] + async fn issue_692_revendor_after_merge_then_revert_converges() { + let fx = fixture_with_lock(Y2_BEFORE).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + let merged_key = "left-pad@^1.1.0, left-pad@^1.3.0"; + let merged = merge_key(&fx.lock_text().await, "left-pad@^1.3.0", merged_key); + tokio::fs::write(fx.lock_path(), &merged).await.unwrap(); + + let (_, again, _) = expect_done(fx.vendor(false).await); + assert!(again.is_none(), "a merged, still-wired block is in sync"); + assert_eq!(fx.lock_text().await, merged, "re-vendor writes nothing"); + + let outcome = revert_yarn_classic(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(outcome.warnings.is_empty(), "{:?}", outcome.warnings); + assert_eq!( + fx.lock_text().await, + merge_key(Y2_BEFORE, "left-pad@^1.3.0", merged_key) + ); + } + + /// #692 ownership gate: a merged block that a third party re-resolved + /// away from our artifact is not ours and stays untouched. Like a + /// removed block, it no longer needs the artifact, so the revert + /// finishes instead of keeping it (and the ledger entry) forever. + #[tokio::test] + async fn issue_692_merged_block_resolved_elsewhere_is_left_alone() { + let fx = fixture_with_lock(Y2_BEFORE).await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + let (sha1, _) = fx.packed_hashes().await; + let ours = + format!(" resolved \"file:./.socket/vendor/npm/{UUID}/left-pad-1.3.0.tgz#{sha1}\""); + let theirs = " resolved \"https://example.com/their-fork.tgz#0000000000000000000000000000000000000000\""; + let wired = fx.lock_text().await; + assert!(wired.contains(&ours)); + let merged = merge_key( + &wired.replacen(&ours, theirs, 1), + "left-pad@^1.3.0", + "left-pad@^1.1.0, left-pad@^1.3.0", + ); + tokio::fs::write(fx.lock_path(), &merged).await.unwrap(); + + let outcome = revert_yarn_classic(&entry, fx.root(), false).await; + assert!(outcome.success, "{:?}", outcome.error); + assert!(!outcome.drift_skipped(), "{:?}", outcome.warnings); + assert_eq!(fx.lock_text().await, merged, "their block is untouched"); + assert!( + !fx.root() + .join(format!(".socket/vendor/npm/{UUID}")) + .exists(), + "nothing resolves through the artifact, so it is removed" + ); + } + /// #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. @@ -3061,6 +3248,7 @@ left-pad@^1.3.0: KIND_LOCK_BLOCK, "lock block", |lines| classic_field(lines, "resolved"), + Some(split_key_patterns), &mut warnings, ); (changed, text, warnings)