diff --git a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs index 3d1e01b83..64d7229ba 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs @@ -2278,6 +2278,9 @@ async fn gem_hosted_superseding_patch_keeps_gem_sections_in_bundler_order() { /// `rollback` / `remove` in that window must undo the new block along with /// the lock, or the pair stays inconsistent (frozen installs fail) and the /// next unfrozen install re-patches. +/// On bundler < 2.2 (one merged GEM section) the re-scan moves the lock's +/// Socket remote along with the block (#1688), so there is no such window; +/// the test proves the superseding patch lands there and unwinds. #[tokio::test(flavor = "multi_thread")] #[ignore = "host capstone: shells out to a real ruby/gem/bundler; \ the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"] @@ -2339,15 +2342,38 @@ async fn unwind_after_unconverged_supersede(command: &str) { assert_eq!(code, 0, "{command}: gen 3 scan: {out}\n{err}"); let gemfile_path = fx.proj.join(fx.gemfile_name); let lock_path = fx.proj.join(fx.lock_name); - // The unconverged window this test is about: Gemfile on gen 3, lock on - // gen 2. let gemfile = std::fs::read_to_string(&gemfile_path).unwrap(); let lock = std::fs::read_to_string(&lock_path).unwrap(); assert!(gemfile.contains(GEN3), "{command}: Gemfile:\n{gemfile}"); - assert!( - lock.contains(GEN2) && !lock.contains(GEN3), - "{command}: lock:\n{lock}" - ); + if fx.bundler.at_least(2, 2) { + // The unconverged window this test is about: Gemfile on gen 3, lock + // on gen 2. + assert!( + lock.contains(GEN2) && !lock.contains(GEN3), + "{command}: lock:\n{lock}" + ); + } else { + // #1688: bundler < 2.2 locks every remote in one merged GEM section, + // whose Socket remote moves to gen 3 with the Gemfile block (the + // pair stays converged, so a frozen install accepts it). The + // unwind below then undoes the converged gen 3 pair. + assert!( + lock.contains(GEN3) && !lock.contains(GEN2), + "{command}: lock:\n{lock}" + ); + let frozen = bundle_env(&fx.proj, &["install"], &[("BUNDLE_FROZEN", "true")]); + assert!( + frozen.status.success(), + "{command}: frozen install of the gen 3 pair:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&frozen.stdout), + String::from_utf8_lossy(&frozen.stderr) + ); + assert_eq!( + std::fs::read_to_string(&lock_path).unwrap(), + lock, + "{command}: the frozen install leaves the gen 3 lock as is" + ); + } let upstream = format!("{api}/upstream"); let cwd = fx.proj.to_str().expect("utf8 tmp path"); diff --git a/crates/socket-patch-core/src/formats/gem/hosted.rs b/crates/socket-patch-core/src/formats/gem/hosted.rs index 6a12e2517..224d69c1e 100644 --- a/crates/socket-patch-core/src/formats/gem/hosted.rs +++ b/crates/socket-patch-core/src/formats/gem/hosted.rs @@ -195,17 +195,9 @@ pub(crate) fn converge_gem_lock_source( // Already ours. Rotated grant or superseding patch: refresh the // remote. if remote_url != index_url { - let ending = - lines[remote_idx][gem_lock_line_content(&lines[remote_idx]).len()..].to_string(); - lines[remote_idx] = Cow::Owned(format!(" remote: {index_url}{ending}")); - result.edits.push(FileEdit { - path: lock_name.into(), - kind: "redirect_gemfile_lock_source_url".into(), - action: "rewritten".into(), - key: Some(dep.name.clone()), - original: Some(Value::String(remote_url)), - new: Some(Value::String(index_url.to_string())), - }); + refresh_remote_line( + &mut lines, remote_idx, remote_url, index_url, dep, lock_name, result, + ); changed = true; } // The refreshed URL (a superseding patch uuid, a rotated grant) can @@ -296,6 +288,118 @@ pub(crate) fn converge_gem_lock_source( true } +/// Keep a merged multi-remote `GEM` section (bundler ≤ 2.1, and locks that +/// started there) on the patch the Gemfile now names (#1688). Such a +/// section lists every rubygems remote, so the Socket remote a previous +/// run's prescribed `bundle install` added is attributed to a gem only +/// through the Gemfile's `source … do` block (`vex::discover::gem`, "Merged +/// sections"). When a superseding patch or rotated grant moves that block +/// from `prior_url` to `index_url`, the section's remote must move with it, +/// or discovery sees the block and the lock name different patches and the +/// scan refuses the new pin as unattributable on every run. +/// +/// Refreshes only the shape a converged earlier redirect leaves: the dep's +/// spec listed once, in a merged section, which lists `prior_url` (the dep's +/// own block URL, so another gem's Socket remote is never taken) exactly +/// once, and a source-pinned (`!`) DEPENDENCIES entry. The remote keeps its +/// position (bundler orders a merged section's remotes by the Gemfile's +/// declarations, which the in-place Gemfile refresh keeps). Returns true +/// when the section's remote reads `index_url` (already, or via the +/// recorded `redirect_gemfile_lock_source_url` edit); false, touching +/// nothing, for any other shape. +pub(crate) fn refresh_merged_gem_lock_remote( + lk: &mut String, + dep: &DepOverride, + prior_url: &str, + index_url: &str, + lock_name: &str, + lock_changed: &mut bool, + result: &mut RewriteResult, +) -> bool { + let model = super::parse(lk); + if !model.problems.is_empty() || !model.pinned.contains(dep.name.as_str()) { + return false; + } + let spec_raw = format!("{} ({})", dep.name, dep.version); + let holders: Vec<&Section<'_>> = model + .gem_sections() + .filter(|s| s.specs.iter().any(|spec| spec.raw == spec_raw)) + .collect(); + let [sec] = holders.as_slice() else { + return false; + }; + if sec.remotes.len() < 2 || sec.specs.iter().filter(|s| s.raw == spec_raw).count() != 1 { + return false; + } + let ours: Vec<(usize, &str)> = sec + .remotes + .iter() + .zip(&sec.remote_line_nos) + .filter(|(remote, _)| super::same_remote(remote, prior_url)) + .map(|(remote, line_no)| (line_no - 1, *remote)) + .collect(); + let [(remote_idx, remote_url)] = ours.as_slice() else { + return false; + }; + if *remote_url != index_url { + let (remote_idx, remote_url) = (*remote_idx, remote_url.to_string()); + let mut lines: Vec> = lk.split_inclusive('\n').map(Cow::Borrowed).collect(); + refresh_remote_line( + &mut lines, remote_idx, remote_url, index_url, dep, lock_name, result, + ); + let text = lines.concat(); + drop(lines); + drop(model); + *lk = text; + *lock_changed = true; + } + true +} + +/// Point the `remote:` line `idx` (reading `remote_url`) at `index_url`, +/// keeping its line ending, and record the `redirect_gemfile_lock_source_url` +/// edit a revert reads. +fn refresh_remote_line( + lines: &mut [Cow<'_, str>], + idx: usize, + remote_url: String, + index_url: &str, + dep: &DepOverride, + lock_name: &str, + result: &mut RewriteResult, +) { + let ending = lines[idx][gem_lock_line_content(&lines[idx]).len()..].to_string(); + lines[idx] = Cow::Owned(format!(" remote: {index_url}{ending}")); + result.edits.push(FileEdit { + path: lock_name.into(), + kind: "redirect_gemfile_lock_source_url".into(), + action: "rewritten".into(), + key: Some(dep.name.clone()), + original: Some(Value::String(remote_url)), + new: Some(Value::String(index_url.to_string())), + }); +} + +/// Whether the lock's `BUNDLED WITH` names a bundler older than 2.2 — the +/// era that writes one merged `GEM` section, where a refreshed merged +/// section is proven frozen-installable. False when it names none. +pub(crate) fn bundled_before_2_2(lock: &str) -> bool { + let mut lines = lock.lines().map(|l| l.trim_end_matches('\r')); + let Some(version) = lines + .by_ref() + .find(|l| *l == "BUNDLED WITH") + .and_then(|_| lines.next()) + .map(str::trim) + else { + return false; + }; + let mut parts = version.split('.').map(|p| p.parse::()); + match (parts.next(), parts.next()) { + (Some(Ok(major)), Some(Ok(minor))) => (major, minor) < (2, 2), + _ => false, + } +} + /// The byte span (line content, ending excluded) of the `CHECKSUMS` entry /// for exactly `name (version)` — the platform-less spec the hosted planner /// pins — read with the shared entry grammar @@ -581,4 +685,134 @@ DEPENDENCIES "GEM\n remote: https://patch.test/gem/tok/uuid/\n specs:\n racc (1.7.3)\n\nGEM\n remote: https://rubygems.org/\n specs:\n\nDEPENDENCIES\n racc (= 1.7.3)!" ); } + + /// `(refreshed, lock after, edit kinds)` for + /// [`refresh_merged_gem_lock_remote`] from the dep's block URL `prior`. + fn refresh(lock: &str, dep: &DepOverride, prior: &str) -> (bool, String, Vec) { + let mut lk = lock.to_string(); + let mut changed = false; + let mut result = RewriteResult::default(); + let ok = refresh_merged_gem_lock_remote( + &mut lk, + dep, + prior, + INDEX_URL, + "Gemfile.lock", + &mut changed, + &mut result, + ); + assert_eq!(changed, lk != lock, "lock_changed must report a change"); + let edits = result + .edits + .iter() + .map(|e| format!("{}:{}", e.kind, e.action)) + .collect(); + (ok, lk, edits) + } + + /// A bundler ≤ 2.1 merged section an earlier redirect converged: + /// `remotes` listed in order, `rails` source-pinned when `bang`, and + /// `extra` sections before PLATFORMS. + fn merged(remotes: &[&str], bang: &str, extra: &str) -> String { + let remotes: String = remotes.iter().map(|r| format!(" remote: {r}\n")).collect(); + format!( + "GEM\n{remotes} specs:\n racc (1.7.3)\n rails (7.0.0)\n racc\n\n{extra}\ + PLATFORMS\n ruby\n\nDEPENDENCIES\n rails (= 7.0.0){bang}\n\nBUNDLED WITH\n 2.1.4\n" + ) + } + + const UPSTREAM: &str = "https://rubygems.org/"; + const OLD: &str = "https://patch.test/gem/tok/older-uuid/"; + + /// #1688: the merged section's remote for the dep's own block moves to + /// the new index URL in place (an earlier patch uuid or grant token), + /// CRLF kept; a lock already on the URL is refreshed with no edit. + #[test] + fn merged_section_socket_remote_follows_the_new_index_url() { + for old in [OLD, "https://patch.test/gem/old-tok/uuid/"] { + for eol in ["\n", "\r\n"] { + let lock = merged(&[UPSTREAM, old], "!", "").replace('\n', eol); + let (ok, out, edits) = refresh(&lock, &dep("rails", "7.0.0"), old); + assert!(ok, "{old}"); + assert_eq!(out, lock.replace(old, INDEX_URL), "{old} {eol:?}"); + assert_eq!(edits, vec!["redirect_gemfile_lock_source_url:rewritten"]); + } + } + let current = merged(&[UPSTREAM, INDEX_URL], "!", ""); + assert_eq!( + refresh(¤t, &dep("rails", "7.0.0"), INDEX_URL), + (true, current.clone(), vec![]) + ); + } + + /// Every other shape is left alone (false, no edit): a remote that is + /// not the dep's own block URL (another gem's Socket remote, or none), + /// a gem not source-pinned, a single-remote section, a spec listed in + /// two sections, or a version the lock does not list. + #[test] + fn merged_section_refresh_refuses_other_shapes() { + let other_gem = "https://patch.test/gem/tok/other-gem-uuid/"; + let second = "GEM\n remote: https://gems.mirror.example/\n specs:\n rails (7.0.0)\n\n"; + let rails = dep("rails", "7.0.0"); + let cases = [ + ( + "another gem's Socket remote", + merged(&[UPSTREAM, other_gem], "!", ""), + &rails, + ), + ( + "no Socket remote", + merged(&[UPSTREAM, "https://gems.mirror.example/"], "!", ""), + &rails, + ), + ( + "not source-pinned", + merged(&[UPSTREAM, OLD], "", ""), + &rails, + ), + ("single remote", merged(&[OLD], "!", ""), &rails), + ( + "spec in two sections", + merged(&[UPSTREAM, OLD], "!", second), + &rails, + ), + ( + "unlocked version", + merged(&[UPSTREAM, OLD], "!", ""), + &dep("rails", "7.1.0"), + ), + ]; + for (label, lock, dep) in &cases { + assert!( + super::super::parse(lock).problems.is_empty(), + "{label}: the fixture must be a valid lock" + ); + let (ok, out, edits) = refresh(lock, dep, OLD); + assert!(!ok, "{label}"); + assert_eq!(&out, lock, "{label}"); + assert!(edits.is_empty(), "{label}: {edits:?}"); + } + } + + /// Only a lock bundler < 2.2 wrote counts as the merged era. + #[test] + fn bundled_before_2_2_reads_the_bundled_with_version() { + for (version, want) in [ + ("1.17.3", true), + ("2.1.4", true), + ("2.2.0", false), + ("2.5.23", false), + ("4.0.18", false), + ("garbage", false), + ] { + let lock = format!("DEPENDENCIES\n rails\n\nBUNDLED WITH\n {version}\n"); + assert_eq!(bundled_before_2_2(&lock), want, "{version}"); + assert_eq!( + bundled_before_2_2(&lock.replace('\n', "\r\n")), + want, + "{version} CRLF" + ); + } + assert!(!bundled_before_2_2("DEPENDENCIES\n rails\n")); + } } diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 0fb484b36..0c70f2490 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -5440,6 +5440,116 @@ snapshots: } } + /// #1688: bundler ≤ 2.1 writes ONE merged `GEM` section listing every + /// remote, so after an earlier redirect + `bundle install` the lock's + /// Socket remote names the earlier patch. A superseding patch (or a + /// rotated grant) moves the Gemfile's `source … do` block to the new + /// URL; the lock's remote must move with it, or discovery reads the + /// pair as naming two patches and the run skips the new one as + /// `redirect_unattributable` on every scan. + #[tokio::test] + async fn merged_section_supersede_moves_the_lock_remote_with_the_gemfile() { + const OLD_UUID: &str = "10000000-1a2b-4a1b-8c2d-3e4f5a6b7c8d"; + const OLD_TOKEN: &str = "22222222-2222-4222-8222-222222222222"; + let new_url = + format!("https://patch.test/patch-registry/gem/{FIXTURE_TOKEN}/{FIXTURE_UUID}/"); + for (label, old_url) in [ + ( + "superseding patch", + format!("https://patch.test/patch-registry/gem/{FIXTURE_TOKEN}/{OLD_UUID}/"), + ), + ( + "rotated grant", + format!("https://patch.test/patch-registry/gem/{OLD_TOKEN}/{FIXTURE_UUID}/"), + ), + ] { + for eol in ["\n", "\r\n"] { + let gemfile = format!( + "source \"https://rubygems.org\"\n\nsource \"{old_url}\" do\n gem \"rails\", \"7.0.0\"\nend\n" + ) + .replace('\n', eol); + let lock = format!( + "GEM\n remote: https://rubygems.org/\n remote: {old_url}\n specs:\n \ + rails (7.0.0)\n\nPLATFORMS\n ruby\n\nDEPENDENCIES\n rails (= 7.0.0)!\n\n\ + BUNDLED WITH\n 2.1.4\n" + ) + .replace('\n', eol); + let mut p = MemoryProject::new(); + p.insert_text("Gemfile", gemfile.as_str()); + p.insert_text("Gemfile.lock", lock.as_str()); + let outer = OuterAllowRemote::default; + let options = RewriteOptions { + dry_run: false, + targets_pipenv_lock: false, + pipenv_major: None, + pipenv_unknown_detail: String::new(), + trust_lockfile_config: true, + npm_allow_remote_config: true, + npm_outer: &outer, + yarn_classic_outer: &OuterYarnMirror::default, + cargo_outer: &no_cargo_outer, + pnpm_env: &no_env, + blocking: false, + takeover_uuids: Default::default(), + patch_server_origins: vec!["https://patch.test".to_string()], + prior_discovery: None, + }; + let mut candidate = gem_candidate(); + if let Some(ov) = candidate.dep.registry_override.as_mut() { + ov.index_url = new_url.clone(); + } + let candidates = vec![candidate]; + let view = ProjectView::Memory(&p); + let read = read_candidate_files(&view, &BTreeSet::new(), &candidates).await; + let done = rewrite( + &view, + read, + &candidates, + BTreeMap::new(), + &BTreeSet::new(), + options, + ) + .await; + let ctx = format!("{label} ({eol:?})"); + assert!( + done.unattributed.is_empty(), + "{ctx}: {:?}", + done.unattributed + ); + assert_eq!( + done.confirmed, + vec![("pkg:gem/rails@7.0.0".to_string(), FIXTURE_UUID.to_string())], + "{ctx}: {:?}", + done.rewrite.warnings + ); + assert_eq!( + done.rewrite.files.get("Gemfile").map(String::as_str), + Some(gemfile.replace(&old_url, &new_url).as_str()), + "{ctx}" + ); + assert_eq!( + done.rewrite.files.get("Gemfile.lock").map(String::as_str), + Some(lock.replace(&old_url, &new_url).as_str()), + "{ctx}: the merged section's Socket remote moves in place" + ); + assert!( + done.rewrite.edits.iter().any(|e| { + e.path == "Gemfile.lock" + && e.kind == "redirect_gemfile_lock_source_url" + && e.original == Some(serde_json::Value::String(old_url.clone())) + }), + "{ctx}: {:?}", + done.rewrite.edits + ); + assert!( + !warning_codes(&done).contains(&"redirect_gem_frozen_install"), + "{ctx}: the refreshed pair stays converged: {:?}", + done.rewrite.warnings + ); + } + } + } + /// A twin whose other spelling this run can't read (a symlink, an /// unreadable or a non-UTF-8 file) is still a twin: bundler's /// `File.file?` sees it, so neither pair is wired (Bugbot on #768). diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 83e5c6561..99765c02d 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -63,7 +63,10 @@ use crate::formats::cargo::source_replacement::{crates_io_source, ConfigLayer, C use crate::formats::cargo::CargoLock; use crate::formats::composer::hosted::rewrite_composer_lock; use crate::formats::gem::gemfile; -use crate::formats::gem::hosted::{checksum_entry_span, converge_gem_lock_source}; +use crate::formats::gem::hosted::{ + bundled_before_2_2, checksum_entry_span, converge_gem_lock_source, + refresh_merged_gem_lock_remote, +}; use crate::formats::gem::{lock_lists_direct_dependency, locked_specs as gem_locked_specs}; #[cfg(test)] use crate::formats::pnpm::hosted::pnpm_unrewritten_instances; @@ -7392,6 +7395,10 @@ fn rewrite_gem( // Whether THIS dep's Gemfile source redirect is in place (just // written or already present) — the lock pin below is gated on it. let mut source_placed = false; + // The URL of the Socket `source … do` block an earlier run left for + // THIS dep (before any in-place refresh below): the remote a merged + // lock section attributes to it (#1688). + let mut prior_block_url: Option = None; if let Some(gf) = gemfile.as_mut() { // Looser "declared at all?" probe: matches every `gem` call that // names the gem anywhere in its arguments, in any form. It gates @@ -7451,6 +7458,7 @@ fn rewrite_gem( let url = m .get(1) .expect("block_re always captures group 1 (index URL)"); + prior_block_url = Some(url.as_str().to_string()); if url.as_str() == ov.index_url { source_placed = true; } else { @@ -7722,16 +7730,38 @@ fn rewrite_gem( // "mismatched checksums"; frozen: exit 16). A pre-CHECKSUMS lock // has no sha to converge around, so it keeps the // mixed-but-installable state + the frozen-install caveat. - if !checksums_era - || !converge_gem_lock_source( + // + // A lock an earlier redirect converged into a merged + // multi-remote section (bundler ≤ 2.1, or a lock that started + // there) instead has its Socket remote follow this dep's Gemfile + // block to the new URL, or discovery reads the pair as naming + // two patches and every scan skips the new one (#1688). Only a + // lock bundler < 2.2 wrote is proven frozen-installable after + // that refresh (e2e_redirect_gem_build); a later bundler keeping + // a merged lock keeps the caveat. + let converged = checksums_era + && converge_gem_lock_source( lk, dep, &ov.index_url, lock_name, &mut lock_changed, result, - ) - { + ); + let merged_refreshed = !converged + && prior_block_url.as_deref().is_some_and(|prior| { + refresh_merged_gem_lock_remote( + lk, + dep, + prior, + &ov.index_url, + lock_name, + &mut lock_changed, + result, + ) + }); + let frozen_installable = converged || (merged_refreshed && bundled_before_2_2(lk)); + if !frozen_installable { mixed_state = true; } } @@ -7741,7 +7771,8 @@ fn rewrite_gem( // section still records the upstream source, so `bundle install` with // `frozen`/`--deployment` set rejects the Gemfile's new source block. // Mirror of the CLI's pnpm trust-lockfile warning. A converged pair (the - // CHECKSUMS-era path) is frozen-installable as written — no caveat. + // CHECKSUMS-era path, or a bundler < 2.2 merged section refreshed in + // place) is frozen-installable as written — no caveat. if (gemfile_changed || lock_changed) && mixed_state { result.warnings.push(RewriteWarning { code: "redirect_gem_frozen_install".into(),