Fix gem supersede on Bundler < 2.2 locks (#1688)

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");

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<Cow<'_, str>> = 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::<u32>());
+    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<String>) {
+        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(&current, &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"));
+    }
 }