Stop Gemfile rewrites declaring a gem twice

Hosted and vendored modes could leave a Gemfile that Bundler refuses
on every install ("You cannot specify the same gem twice"):

- Hosted mode rewrote only the first of several declarations of a gem
  (for example one in each of two `group` blocks), leaving an exact
  pin next to the original requirement (#548). It now refuses with
  redirect_gem_declared_more_than_once, as vendored mode already did.
- Both modes treated a gem they could not see declared in the root
  Gemfile as transitive and appended a second declaration, even when
  the lock lists it under DEPENDENCIES because the Gemfile declares it
  through eval_gemfile or a loop (#482). Both now refuse instead.

Fixes #482
Fixes #548

Assisted-by: Claude Code:claude-opus-5-5

Author: claude

crates/socket-patch-core/src/formats/gem/mod.rs

@@ -114,6 +114,10 @@ pub(crate) struct GemfileLock<'t> {
     pub(crate) checksums: Option<HashMap<(&'t str, &'t str), Option<String>>>,
     /// `DEPENDENCIES` entries bundler marks source-pinned (`name …!`).
     pub(crate) pinned: BTreeSet<&'t str>,
+    /// Every `DEPENDENCIES` entry: the gems bundler treats as DIRECT
+    /// dependencies, however the Gemfile declares them (`eval_gemfile`, a
+    /// loop, a `gemspec` development dependency, a plain `gem` line).
+    pub(crate) direct: BTreeSet<&'t str>,
     /// Why bundler would refuse this lock, first problem first.
     pub(crate) problems: Vec<String>,
 }
@@ -222,6 +226,7 @@ pub(crate) fn parse(text: &str) -> GemfileLock<'_> {
     let mut in_checksums = false;
     let mut in_dependencies = false;
     let mut pinned: BTreeSet<&str> = BTreeSet::new();
+    let mut direct: BTreeSet<&str> = BTreeSet::new();
     let mut seen_header = false;
     let mut bundler_shaped = false;
 
@@ -281,6 +286,10 @@ pub(crate) fn parse(text: &str) -> GemfileLock<'_> {
                 _ => {}
             }
         } else if in_dependencies && indent == 2 {
+            let name = trimmed.split([' ', '(', '!']).next().unwrap_or_default();
+            if !name.is_empty() {
+                direct.insert(name);
+            }
             if let Some(entry) = trimmed.strip_suffix('!') {
                 let name = entry.split([' ', '(']).next().unwrap_or_default();
                 if !name.is_empty() {
@@ -306,10 +315,21 @@ pub(crate) fn parse(text: &str) -> GemfileLock<'_> {
         sections,
         checksums,
         pinned,
+        direct,
         problems,
     }
 }
 
+/// Whether the Bundler lock `lock` lists `name` under `DEPENDENCIES`, i.e.
+/// bundler resolved it as a DIRECT dependency of the Gemfile. The Gemfile
+/// rewriters consult it before treating a gem they cannot see declared as
+/// transitive: appending a declaration for a gem the Gemfile already
+/// declares out of sight (`eval_gemfile`, a loop) leaves it declared twice,
+/// and bundler refuses every install (#482).
+pub(crate) fn lock_lists_direct_dependency(lock: &str, name: &str) -> bool {
+    parse(lock).direct.contains(name)
+}
+
 /// The plain gem-token charset (letters, digits, `.`, `_`, `-`). The vendor
 /// backend applies it before embedding coordinates into Ruby source and lock
 /// line grammar (see the SECURITY note in `crate::vendor::gem`'s

crates/socket-patch-core/src/patch/redirect/mod.rs

@@ -51,6 +51,7 @@ use crate::formats::pnpm::plan_hosted;
 use crate::formats::cargo::CargoLock;
 use crate::formats::composer::hosted::rewrite_composer_lock;
 use crate::formats::gem::hosted::{checksum_entry_span, converge_gem_lock_source};
+use crate::formats::gem::lock_lists_direct_dependency;
 pub(crate) use crate::formats::yarn::is_berry_lock;
 use crate::formats::cargo::hosted::CargoLockPlan;
 #[cfg(test)]
@@ -4945,6 +4946,31 @@ fn rewrite_gem(
         // written or already present) — the lock pin below is gated on it.
         let mut source_placed = false;
         if let Some(gf) = gemfile.as_mut() {
+            // Looser "declared at all?" probe: counts every `gem` call that
+            // names the gem, in any form (indented in a group, parenthesized,
+            // our own source block). It gates the append branch (appending
+            // next to a declaration the recognizer below cannot parse would
+            // leave the gem declared twice) and catches a gem declared more
+            // than once: rewriting only one of them leaves `= x.y.z` next to
+            // the other requirement, and bundler refuses the Gemfile (#548).
+            let declared_re = Regex::new(
+                &(String::from(r#"(?m)^[ \t]*gem\b[^\n]*["']"#)
+                    + &regex::escape(&dep.name)
+                    + r#"["']"#),
+            )
+            .expect("declaration probe regex from the escaped gem name is valid");
+            if declared_re.find_iter(gf).nth(1).is_some() {
+                result.warnings.push(RewriteWarning {
+                    code: "redirect_gem_declared_more_than_once".into(),
+                    detail: format!(
+                        "`gem \"{}\"` is declared more than once in {gemfile_name}; \
+                         rewriting one declaration would leave conflicting requirements \
+                         bundler refuses — merge them into one declaration and re-run",
+                        dep.name
+                    ),
+                });
+                continue;
+            }
             // Grant-agnostic idempotency guard: the grant-token (and patch
             // uuid) segments of the index URL rotate per request, so an
             // exact-URL check misses the block a previous run wrote and this
@@ -4994,16 +5020,6 @@ fn rewrite_gem(
                         + r#"["']([^\n]*)$"#),
                 )
                 .expect("gem-line regex from the escaped gem name is valid");
-                // Looser "declared at all?" probe: gates the append branch —
-                // appending next to a declaration the recognizer above cannot
-                // parse would leave the gem declared twice (bundler
-                // hard-fails on the duplicate).
-                let declared_re = Regex::new(
-                    &(String::from(r#"(?m)^[ \t]*gem\b[^\n]*["']"#)
-                        + &regex::escape(&dep.name)
-                        + r#"["']"#),
-                )
-                .expect("declaration probe regex from the escaped gem name is valid");
                 if let Some(m) = gem_line_re.captures(gf) {
                     let range = m.get(0).expect("group 0 is the whole match").range();
                     let original = m
@@ -5116,6 +5132,26 @@ fn rewrite_gem(
                         ),
                     });
                     continue;
+                } else if files
+                    .get(lock_name)
+                    .is_some_and(|lk| lock_lists_direct_dependency(lk, &dep.name))
+                {
+                    // Not declared where the rewriter can see it, yet bundler
+                    // resolved it as a DIRECT dependency: the Gemfile declares
+                    // it out of sight (`eval_gemfile`, a loop, a gemspec).
+                    // Appending a block would declare it twice (#482).
+                    result.warnings.push(RewriteWarning {
+                        code: "redirect_gem_declaration_not_visible".into(),
+                        detail: format!(
+                            "{lock_name} lists {} as a direct dependency, but {gemfile_name} \
+                             declares it somewhere the rewriter cannot edit (an \
+                             `eval_gemfile`d file, a loop, a gemspec); appending a second \
+                             declaration would make bundler refuse the Gemfile — redirect \
+                             skipped",
+                            dep.name
+                        ),
+                    });
+                    continue;
                 } else {
                     // Genuinely undeclared (a transitive dep): append a block.
                     let block = format!(
@@ -11042,6 +11078,97 @@ mod tests {
         );
     }
 
+    /// #548: bundler accepts a gem declared more than once with the same
+    /// requirement (two `group` blocks, or top level plus a group).
+    /// Rewriting only the first declaration leaves `= 1.0.0` next to
+    /// `>= 0`, which bundler refuses on every install. Fail closed before
+    /// any write, like vendored mode's `gemfile_declaration_not_editable`.
+    #[test]
+    fn gemfile_gem_declared_twice_fails_closed() {
+        let lock = "GEM\n  remote: https://rubygems.org/\n  specs:\n    vuln-gem (1.0.0)\n\n\
+                    PLATFORMS\n  ruby\n\nDEPENDENCIES\n  vuln-gem\n\n\
+                    CHECKSUMS\n  vuln-gem (1.0.0) sha256="
+            .to_string()
+            + &"2".repeat(64)
+            + "\n\nBUNDLED WITH\n   4.0.17\n";
+        for gemfile in [
+            "source \"https://rubygems.org\"\n\ngroup :development do\n  gem \"vuln-gem\"\nend\n\n\
+             group :test do\n  gem \"vuln-gem\"\nend\n",
+            "source \"https://rubygems.org\"\n\ngem \"vuln-gem\"\n\n\
+             group :test do\n  gem \"vuln-gem\"\nend\n",
+            "source \"https://rubygems.org\"\n\ngem \"vuln-gem\"\ngem(\"vuln-gem\")\n",
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert("Gemfile".to_string(), gemfile.to_string());
+            files.insert("Gemfile.lock".to_string(), lock.clone());
+            let r = rewrite_registry_redirect(&files, &[gem_override("vuln-gem", "1.0.0")]);
+            assert!(
+                r.files.is_empty() && r.edits.is_empty(),
+                "a gem declared twice must not be half-rewritten: {gemfile}\nfiles={:?} edits={:?}",
+                r.files,
+                r.edits
+            );
+            assert_eq!(
+                warning_codes(&r),
+                vec!["redirect_gem_declared_more_than_once"],
+                "{gemfile}: {:?}",
+                r.warnings
+            );
+        }
+    }
+
+    /// #482: a DIRECT dependency the root Gemfile declares out of the
+    /// rewriter's sight (`eval_gemfile`, a loop) is listed under the lock's
+    /// DEPENDENCIES. Appending a source block for it declares it twice and
+    /// bundler refuses every install, so fail closed instead.
+    #[test]
+    fn gemfile_direct_dependency_declared_out_of_sight_is_not_appended() {
+        let lock = "GEM\n  remote: https://rubygems.org/\n  specs:\n    rack (3.1.8)\n\n\
+                    PLATFORMS\n  ruby\n\nDEPENDENCIES\n  rack (~> 3.1)\n\n\
+                    BUNDLED WITH\n   4.0.17\n";
+        for gemfile in [
+            "source \"https://rubygems.org\"\neval_gemfile \"Gemfile.common\"\n",
+            "source \"https://rubygems.org\"\n%w[rack].each { |g| gem g, \"~> 3.1\" }\n",
+        ] {
+            let mut files = BTreeMap::new();
+            files.insert("Gemfile".to_string(), gemfile.to_string());
+            files.insert("Gemfile.lock".to_string(), lock.to_string());
+            let r = rewrite_registry_redirect(&files, &[gem_override("rack", "3.1.8")]);
+            assert!(
+                r.files.is_empty() && r.edits.is_empty(),
+                "a direct dep declared out of sight must not be appended: {gemfile}\n\
+                 files={:?} edits={:?}",
+                r.files,
+                r.edits
+            );
+            assert_eq!(
+                warning_codes(&r),
+                vec!["redirect_gem_declaration_not_visible"],
+                "{gemfile}: {:?}",
+                r.warnings
+            );
+        }
+        // Control: a genuinely transitive gem (absent from DEPENDENCIES) is
+        // still appended.
+        let mut files = BTreeMap::new();
+        files.insert(
+            "Gemfile".to_string(),
+            "source \"https://rubygems.org\"\ngem \"rails\"\n".to_string(),
+        );
+        files.insert(
+            "Gemfile.lock".to_string(),
+            lock.replace("DEPENDENCIES\n  rack (~> 3.1)", "DEPENDENCIES\n  rails"),
+        );
+        let r = rewrite_registry_redirect(&files, &[gem_override("rack", "3.1.8")]);
+        let out = r.files.get("Gemfile").expect("transitive gem appended");
+        assert!(
+            out.ends_with(
+                "source \"https://patch.test/gem/tok/uuid/\" do\n  gem \"rack\", \"3.1.8\"\nend\n"
+            ),
+            "{out}"
+        );
+    }
+
     /// The CHECKSUMS pin is gated on the Gemfile source redirect being in
     /// place: with no Gemfile in the candidate map, pinning the patched sha
     /// while the gem still resolves upstream guarantees a checksum failure.

crates/socket-patch-core/src/vendor/gem.rs

@@ -415,7 +415,9 @@ fn gem_edits(
 ) -> Result<(GemfilePlan, LockEdit), Box<VendorOutcome>> {
     let (name, version) = (prelude.name.as_str(), prelude.version.as_str());
     // ── Gemfile edit plan (refusals before any write) ────────────────────
-    let plan = match plan_gemfile_edit(&prelude.gemfile_text, name, version, &prelude.copy_rel) {
+    let plan = match plan_gemfile_edit(&prelude.gemfile_text, name, version, &prelude.copy_rel)
+        .and_then(|plan| refuse_append_of_direct_dependency(plan, &prelude.lock_text, name))
+    {
         Ok(p) => p,
         Err(detail) => {
             return Err(Box::new(refused(
@@ -1428,6 +1430,29 @@ fn plan_gemfile_edit(
     })
 }
 
+/// Refuse an [`GemfilePlan::Append`] for a gem the lock lists under
+/// `DEPENDENCIES`: bundler resolved it as a DIRECT dependency, so the Gemfile
+/// declares it somewhere the line grammar cannot see (`eval_gemfile`, a
+/// loop, a gemspec). The managed block would declare it a second time and
+/// bundler refuses every install (#482). Every other plan passes through.
+fn refuse_append_of_direct_dependency(
+    plan: GemfilePlan,
+    lock_text: &str,
+    name: &str,
+) -> Result<GemfilePlan, String> {
+    if matches!(plan, GemfilePlan::Append { .. })
+        && crate::formats::gem::lock_lists_direct_dependency(lock_text, name)
+    {
+        return Err(format!(
+            "Gemfile.lock lists \"{name}\" as a direct dependency, but the Gemfile declares \
+             it somewhere the line grammar cannot edit (an `eval_gemfile`d file, a loop, a \
+             gemspec); refusing to append a second declaration (bundler hard-fails on \
+             duplicates)"
+        ));
+    }
+    Ok(plan)
+}
+
 /// Looser "declared at all?" probe — the redirect rewriter's `declared_re`
 /// twin. True when a non-comment line is a `gem` call (the keyword followed
 /// by anything but an identifier character) whose arguments quote the exact
@@ -5488,6 +5513,36 @@ mod tests {
         );
     }
 
+    /// #482: a DIRECT dependency declared where the line grammar cannot see
+    /// it (`eval_gemfile`, a loop) is listed under the lock's DEPENDENCIES.
+    /// Appending the managed block would declare it twice and bundler
+    /// refuses every install, so the plan must refuse before any write.
+    #[tokio::test]
+    async fn direct_dependency_declared_out_of_sight_refuses_instead_of_appending() {
+        for gemfile in [
+            "source \"https://rubygems.org\"\n\ngem \"puma\"\neval_gemfile \"Gemfile.common\"\n",
+            "source \"https://rubygems.org\"\n\ngem \"puma\"\n%w[rack].each { |g| gem g, \"~> 3.1\" }\n",
+        ] {
+            let (_tmp, root, installed, blobs, record) = fixture(gemfile, LOCK_DIRECT).await;
+            let (code, detail) =
+                unwrap_refused(run_vendor(&root, &blobs, &installed, &record, false).await);
+            assert_eq!(code, "gemfile_declaration_not_editable", "{gemfile}");
+            assert!(detail.contains("direct dependency"), "{detail}");
+            assert!(!root.join(".socket").exists());
+            assert_eq!(
+                tokio::fs::read_to_string(root.join(GEMFILE)).await.unwrap(),
+                gemfile,
+                "refusal must write nothing: {detail}"
+            );
+            assert_eq!(
+                tokio::fs::read_to_string(root.join(GEMFILE_LOCK))
+                    .await
+                    .unwrap(),
+                LOCK_DIRECT
+            );
+        }
+    }
+
     /// Re-vendor (new uuid) over a lock whose CHECKSUMS entry was ALREADY
     /// bare pre-vendor: the first run recorded no checksum wiring, so the
     /// re-vendor's `original: None` checksum record has nothing to