Skip to content

Commit b1f9818

Browse files
Fix Gemfile rewrites duplicating gem declarations (#482, #548) (#552)
* Start fix for #482, #548 Assisted-by: Claude Code:claude-opus-5-5 * 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 * Test hosted gem refusals with real Bundler Covers a gem declared in two group blocks (#548) and a direct dependency declared through eval_gemfile (#482): the hosted scan must leave the Gemfile pair untouched, attest nothing, and Bundler must still install the project. Assisted-by: Claude Code:claude-opus-5-5 * Test vendored eval_gemfile refusal with Bundler A direct dependency declared through eval_gemfile must be refused before any write (#482), and Bundler must still install the project frozen and unfrozen. Assisted-by: Claude Code:claude-opus-5-5 * Format the duplicate-declaration e2e assert Wraps one long assert in the new Bundler capstone so rustfmt leaves the file unchanged. No behavior change. Assisted-by: Claude Code:claude-opus-5-5 * Count only real gem declarations as duplicates Hosted redirect refused a Gemfile with one editable declaration when another gem call quoted the same name, e.g. `gem "other", require: "rack"` or a trailing comment. Only `gem` calls whose first argument is the gem now count toward the more-than-once refusal. The looser probe still gates appending a source block. Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 42e7734 commit b1f9818

5 files changed

Lines changed: 489 additions & 24 deletions

File tree

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

Lines changed: 155 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -413,6 +413,17 @@ enum Driver {
413413
/// `Gemfile.next`, so the run must redirect nothing and attest nothing.
414414
/// The fixture asserts that contract itself and yields `None`.
415415
ScanVexDualBoot,
416+
/// [`Driver::ScanVex`] on a Gemfile that declares the gem in two `group`
417+
/// blocks (#548): bundler accepts the duplicate, but rewriting only one
418+
/// declaration would leave conflicting requirements. The run must
419+
/// refuse, write nothing and attest nothing; the fixture asserts that
420+
/// and yields `None`.
421+
ScanVexDuplicateDeclaration,
422+
/// [`Driver::ScanVex`] on a Gemfile that declares the gem through
423+
/// `eval_gemfile` (#482): the lock lists it as a direct dependency, so
424+
/// appending a source block would declare it twice. Same contract as
425+
/// [`Driver::ScanVexDuplicateDeclaration`].
426+
ScanVexEvalGemfile,
416427
/// [`Driver::ScanVexDualBoot`] with `BUNDLE_GEMFILE=Gemfile` exported to
417428
/// socket-patch too (#507): bundler's local app config outranks the
418429
/// environment, so bundler still loads `Gemfile.next` and the run must
@@ -426,6 +437,8 @@ impl Driver {
426437
Driver::ScanVex => "scan --mode hosted",
427438
Driver::GetUuid => "get <uuid> --mode hosted",
428439
Driver::ScanVexDualBoot => "scan --mode hosted (BUNDLE_GEMFILE=Gemfile.next)",
440+
Driver::ScanVexDuplicateDeclaration => "scan --mode hosted (gem in two groups)",
441+
Driver::ScanVexEvalGemfile => "scan --mode hosted (gem via eval_gemfile)",
429442
Driver::ScanVexDualBootEnvGemfile => {
430443
"scan --mode hosted (config Gemfile.next, env BUNDLE_GEMFILE=Gemfile)"
431444
}
@@ -691,11 +704,22 @@ async fn redirect_scanned_project(
691704
// 3. The fixture project, installed from the MOCK upstream (hermetic).
692705
let proj = tmp.path().join("proj");
693706
std::fs::create_dir_all(&proj).unwrap();
694-
std::fs::write(
695-
proj.join(gemfile_name),
696-
format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()),
697-
)
698-
.unwrap();
707+
let gemfile_body = match driver {
708+
Driver::ScanVexDuplicateDeclaration => format!(
709+
"source \"{}/upstream\"\n\ngroup :development do\n gem \"{DEP}\"\nend\n\n\
710+
group :test do\n gem \"{DEP}\"\nend\n",
711+
server.uri()
712+
),
713+
Driver::ScanVexEvalGemfile => {
714+
std::fs::write(proj.join("Gemfile.common"), format!("gem \"{DEP}\"\n")).unwrap();
715+
format!(
716+
"source \"{}/upstream\"\n\neval_gemfile \"Gemfile.common\"\n",
717+
server.uri()
718+
)
719+
}
720+
_ => format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()),
721+
};
722+
std::fs::write(proj.join(gemfile_name), gemfile_body).unwrap();
699723
let config_args = bundler.config_local_args("path", "vendor/bundle");
700724
let config_args: Vec<&str> = config_args.iter().map(String::as_str).collect();
701725
let config = bundle(&proj, &config_args);
@@ -795,7 +819,11 @@ async fn redirect_scanned_project(
795819
);
796820
}
797821
let argv: Vec<&str> = match driver {
798-
Driver::ScanVex | Driver::ScanVexDualBoot | Driver::ScanVexDualBootEnvGemfile => vec![
822+
Driver::ScanVex
823+
| Driver::ScanVexDualBoot
824+
| Driver::ScanVexDualBootEnvGemfile
825+
| Driver::ScanVexDuplicateDeclaration
826+
| Driver::ScanVexEvalGemfile => vec![
799827
"scan",
800828
"--mode",
801829
"hosted",
@@ -849,6 +877,21 @@ async fn redirect_scanned_project(
849877
assert_dual_boot_redirects_nothing(&env, &proj, &pristine_gemfile, &pristine_lock);
850878
return None;
851879
}
880+
if let Some(warning) = match driver {
881+
Driver::ScanVexDuplicateDeclaration => Some("redirect_gem_declared_more_than_once"),
882+
Driver::ScanVexEvalGemfile => Some("redirect_gem_declaration_not_visible"),
883+
_ => None,
884+
} {
885+
assert_unwirable_declaration_redirects_nothing(
886+
&proj,
887+
&bundler,
888+
warning,
889+
(code, &stdout, &stderr),
890+
&pristine_gemfile,
891+
&pristine_lock,
892+
);
893+
return None;
894+
}
852895
assert_eq!(
853896
code,
854897
0,
@@ -922,9 +965,10 @@ async fn redirect_scanned_project(
922965
"in-run hosted VEX is attested from this run's fetched record, not hash-verified: {env}"
923966
);
924967
}
925-
Driver::ScanVexDualBoot | Driver::ScanVexDualBootEnvGemfile => {
926-
unreachable!("asserted and returned above")
927-
}
968+
Driver::ScanVexDualBoot
969+
| Driver::ScanVexDualBootEnvGemfile
970+
| Driver::ScanVexDuplicateDeclaration
971+
| Driver::ScanVexEvalGemfile => unreachable!("asserted and returned above"),
928972
Driver::GetUuid => {
929973
// get's hosted envelope (CLI_CONTRACT.md "get --mode and
930974
// installed narrowing"): `found` counts the resolved patch;
@@ -978,6 +1022,49 @@ async fn redirect_scanned_project(
9781022
})
9791023
}
9801024

1025+
/// #482 / #548: a Gemfile whose declarations of the gem the rewriter cannot
1026+
/// edit as one (two `group` blocks, an `eval_gemfile`d file). The scan names
1027+
/// the refusal, leaves the pair byte-identical and attests nothing, and the
1028+
/// real bundler still installs the project (before the fix the Gemfile was
1029+
/// left declaring the gem twice and every install exited 4).
1030+
fn assert_unwirable_declaration_redirects_nothing(
1031+
proj: &Path,
1032+
bundler: &bundler_e2e::Bundler,
1033+
warning: &str,
1034+
(code, stdout, stderr): (i32, &str, &str),
1035+
pristine_gemfile: &[u8],
1036+
pristine_lock: &[u8],
1037+
) {
1038+
let env: serde_json::Value = serde_json::from_str(stdout)
1039+
.unwrap_or_else(|e| panic!("not JSON: {e}\nstdout:\n{stdout}\nstderr:\n{stderr}"));
1040+
assert!(
1041+
stdout.contains(warning),
1042+
"the refusal must be named ({warning}): {env}"
1043+
);
1044+
assert_ne!(code, 0, "nothing was patched or attested: {env}");
1045+
assert!(
1046+
env["vex"]["statements"].as_u64().unwrap_or(0) == 0,
1047+
"nothing may be attested: {env}"
1048+
);
1049+
assert_eq!(
1050+
std::fs::read(proj.join("Gemfile")).unwrap(),
1051+
pristine_gemfile,
1052+
"the Gemfile must be byte-identical"
1053+
);
1054+
assert_eq!(
1055+
std::fs::read(proj.join("Gemfile.lock")).unwrap(),
1056+
pristine_lock,
1057+
"the lock must be byte-identical"
1058+
);
1059+
let install = bundle(proj, &["install"]);
1060+
assert!(
1061+
install.status.success(),
1062+
"bundler {} must still install the untouched project:\n{}",
1063+
bundler.version,
1064+
String::from_utf8_lossy(&install.stderr)
1065+
);
1066+
}
1067+
9811068
/// #390's contract on a `BUNDLE_GEMFILE: Gemfile.next` project: the hosted
9821069
/// scan names the setting, rewrites neither the `Gemfile` pair (which
9831070
/// bundler ignores) nor `Gemfile.next`, and its in-run VEX attests nothing.
@@ -1530,14 +1617,30 @@ fn vendor_takeover_keeps_the_hosted_gems_rb_pin(fx: &RedirectFixture) {
15301617
let (code, stdout, stderr) = run_socket(
15311618
&fx.proj,
15321619
&[
1533-
"get", UUID, "--mode", "vendored", "--json", "--yes", "--cwd", proj, "--api-url",
1534-
&api, "--org", ORG, "--api-token", "fake",
1620+
"get",
1621+
UUID,
1622+
"--mode",
1623+
"vendored",
1624+
"--json",
1625+
"--yes",
1626+
"--cwd",
1627+
proj,
1628+
"--api-url",
1629+
&api,
1630+
"--org",
1631+
ORG,
1632+
"--api-token",
1633+
"fake",
15351634
// The mock serves the patch registry: its origin is the one a
15361635
// hosted pin is trusted on.
1537-
"--patch-server-url", &api,
1636+
"--patch-server-url",
1637+
&api,
15381638
],
15391639
);
1540-
assert_ne!(code, 0, "vendor must refuse.\nstdout:\n{stdout}\nstderr:\n{stderr}");
1640+
assert_ne!(
1641+
code, 0,
1642+
"vendor must refuse.\nstdout:\n{stdout}\nstderr:\n{stderr}"
1643+
);
15411644
assert!(
15421645
stdout.contains("gemfile_not_loaded"),
15431646
"the manifest refusal names its cause:\n{stdout}"
@@ -1575,6 +1678,45 @@ async fn gem_hosted_bundle_gemfile_dual_boot_redirects_nothing() {
15751678
assert!(fx.is_none(), "the dual-boot driver asserts in place");
15761679
}
15771680

1681+
/// #548: a gem declared in two `group` blocks must not be half-rewritten
1682+
/// (bundler refuses `= 1.0.0` next to `>= 0` on every install).
1683+
#[tokio::test(flavor = "multi_thread")]
1684+
#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17; CHECKSUMS arm >= 2.6); \
1685+
the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"]
1686+
async fn gem_hosted_gem_declared_in_two_groups_is_refused_and_still_installs() {
1687+
let fx = redirect_scanned_project(
1688+
"two-groups",
1689+
Spelling::Gemfile,
1690+
false,
1691+
true,
1692+
None,
1693+
Driver::ScanVexDuplicateDeclaration,
1694+
)
1695+
.await;
1696+
assert!(
1697+
fx.is_none(),
1698+
"the duplicate-declaration driver asserts in place"
1699+
);
1700+
}
1701+
1702+
/// #482: a direct dependency declared through `eval_gemfile` must not get a
1703+
/// second, appended declaration.
1704+
#[tokio::test(flavor = "multi_thread")]
1705+
#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17; CHECKSUMS arm >= 2.6); \
1706+
the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"]
1707+
async fn gem_hosted_eval_gemfile_direct_dep_is_refused_and_still_installs() {
1708+
let fx = redirect_scanned_project(
1709+
"eval-gemfile",
1710+
Spelling::Gemfile,
1711+
false,
1712+
true,
1713+
None,
1714+
Driver::ScanVexEvalGemfile,
1715+
)
1716+
.await;
1717+
assert!(fx.is_none(), "the eval_gemfile driver asserts in place");
1718+
}
1719+
15781720
/// #507: the same dual boot with `BUNDLE_GEMFILE=Gemfile` exported. Bundler
15791721
/// ranks the committed `.bundle/config` above the environment (it still
15801722
/// loads `Gemfile.next`), so socket-patch must not follow the env value and

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

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1562,3 +1562,78 @@ fn gem_vendor_refuses_a_bundle_gemfile_dual_boot() {
15621562
],
15631563
);
15641564
}
1565+
1566+
/// #482: a direct dependency declared through `eval_gemfile` is invisible to
1567+
/// the Gemfile line grammar. Vendor used to treat it as transitive, append a
1568+
/// managed `path:` declaration, exit 0, and leave every `bundle install`
1569+
/// failing on the duplicate; it must refuse before any write.
1570+
#[test]
1571+
#[ignore = "host capstone: shells out to a real bundler >= 1.17; the unpinned `test` job \
1572+
skips it, the e2e job runs it with a pinned toolchain via --ignored"]
1573+
fn gem_vendor_refuses_an_eval_gemfile_direct_dep() {
1574+
let Some((_tmp, proj, _bundler, purl)) = staged_rack_project("eval_gemfile") else {
1575+
return;
1576+
};
1577+
std::fs::write(proj.join("Gemfile.common"), "gem \"rack\", \"~> 3.1\"\n").unwrap();
1578+
std::fs::write(
1579+
proj.join("Gemfile"),
1580+
"source \"https://rubygems.org\"\n\neval_gemfile \"Gemfile.common\"\n",
1581+
)
1582+
.unwrap();
1583+
let relock = bundle(&proj, &["install"], false);
1584+
assert!(
1585+
relock.status.success(),
1586+
"the eval_gemfile layout installs (test premise):\n{}",
1587+
String::from_utf8_lossy(&relock.stderr)
1588+
);
1589+
let files = ["Gemfile", "Gemfile.common", "Gemfile.lock"];
1590+
let before: Vec<Vec<u8>> = files
1591+
.iter()
1592+
.map(|f| std::fs::read(proj.join(f)).unwrap())
1593+
.collect();
1594+
let (code, stdout, stderr) = run_socket(
1595+
&proj,
1596+
&[
1597+
"vendor",
1598+
"--json",
1599+
"--offline",
1600+
"--cwd",
1601+
proj.to_str().unwrap(),
1602+
],
1603+
);
1604+
assert_ne!(
1605+
code, 0,
1606+
"vendor must not succeed.\nstdout:\n{stdout}\nstderr:\n{stderr}"
1607+
);
1608+
let env = parse_envelope(&stdout);
1609+
assert_eq!(env["summary"]["applied"], 0, "nothing vendored: {env}");
1610+
let event = env["events"]
1611+
.as_array()
1612+
.unwrap()
1613+
.iter()
1614+
.find(|e| e["purl"] == purl)
1615+
.unwrap_or_else(|| panic!("an event for {purl}: {env}"));
1616+
assert_eq!(
1617+
event["errorCode"], "gemfile_declaration_not_editable",
1618+
"event: {event}"
1619+
);
1620+
for (file, before) in files.iter().zip(before) {
1621+
assert_eq!(
1622+
std::fs::read(proj.join(file)).unwrap(),
1623+
before,
1624+
"{file} must be byte-untouched"
1625+
);
1626+
}
1627+
assert!(
1628+
!proj.join(".socket/vendor/gem").exists(),
1629+
"no vendored copy is written"
1630+
);
1631+
for frozen in [false, true] {
1632+
let install = bundle(&proj, &["install"], frozen);
1633+
assert!(
1634+
install.status.success(),
1635+
"bundle install (frozen: {frozen}) must still succeed:\n{}",
1636+
String::from_utf8_lossy(&install.stderr)
1637+
);
1638+
}
1639+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,10 @@ pub(crate) struct GemfileLock<'t> {
114114
pub(crate) checksums: Option<HashMap<(&'t str, &'t str), Option<String>>>,
115115
/// `DEPENDENCIES` entries bundler marks source-pinned (`name …!`).
116116
pub(crate) pinned: BTreeSet<&'t str>,
117+
/// Every `DEPENDENCIES` entry: the gems bundler treats as DIRECT
118+
/// dependencies, however the Gemfile declares them (`eval_gemfile`, a
119+
/// loop, a `gemspec` development dependency, a plain `gem` line).
120+
pub(crate) direct: BTreeSet<&'t str>,
117121
/// Why bundler would refuse this lock, first problem first.
118122
pub(crate) problems: Vec<String>,
119123
}
@@ -222,6 +226,7 @@ pub(crate) fn parse(text: &str) -> GemfileLock<'_> {
222226
let mut in_checksums = false;
223227
let mut in_dependencies = false;
224228
let mut pinned: BTreeSet<&str> = BTreeSet::new();
229+
let mut direct: BTreeSet<&str> = BTreeSet::new();
225230
let mut seen_header = false;
226231
let mut bundler_shaped = false;
227232

@@ -281,6 +286,10 @@ pub(crate) fn parse(text: &str) -> GemfileLock<'_> {
281286
_ => {}
282287
}
283288
} else if in_dependencies && indent == 2 {
289+
let name = trimmed.split([' ', '(', '!']).next().unwrap_or_default();
290+
if !name.is_empty() {
291+
direct.insert(name);
292+
}
284293
if let Some(entry) = trimmed.strip_suffix('!') {
285294
let name = entry.split([' ', '(']).next().unwrap_or_default();
286295
if !name.is_empty() {
@@ -306,10 +315,21 @@ pub(crate) fn parse(text: &str) -> GemfileLock<'_> {
306315
sections,
307316
checksums,
308317
pinned,
318+
direct,
309319
problems,
310320
}
311321
}
312322

323+
/// Whether the Bundler lock `lock` lists `name` under `DEPENDENCIES`, i.e.
324+
/// bundler resolved it as a DIRECT dependency of the Gemfile. The Gemfile
325+
/// rewriters consult it before treating a gem they cannot see declared as
326+
/// transitive: appending a declaration for a gem the Gemfile already
327+
/// declares out of sight (`eval_gemfile`, a loop) leaves it declared twice,
328+
/// and bundler refuses every install (#482).
329+
pub(crate) fn lock_lists_direct_dependency(lock: &str, name: &str) -> bool {
330+
parse(lock).direct.contains(name)
331+
}
332+
313333
/// The plain gem-token charset (letters, digits, `.`, `_`, `-`). The vendor
314334
/// backend applies it before embedding coordinates into Ruby source and lock
315335
/// line grammar (see the SECURITY note in `crate::vendor::gem`'s

0 commit comments

Comments
 (0)