From 120df7d676b448e36abffbdd948e967d1bcc4ac3 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 12:42:17 -0400 Subject: [PATCH 1/4] WIP: Fix gem unwind dropping source-block `!` (#1056) Co-Authored-By: Claude Opus 5.5 (1M context) From 41bda596d0dc0f796249e9d209f3e39fec34fee2 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 13:06:16 -0400 Subject: [PATCH 2/4] Keep gem source-block `!` through hosted unwind A gem declared inside the user's own `source "..." do` block is locked by Bundler as `name (= v)!`. The hosted unwind (`rollback`, `remove`) puts the declaration back inside that block but always dropped the `!` from DEPENDENCIES, so the restored pair no longer matched what Bundler writes and every frozen install failed with exit 16. The restore now drops the `!` only when the restored declaration is outside every `source ... do` block (the case where hosted mode added the pin), and keeps it otherwise. Unit and real-bundler e2e tests pin the byte-identical round trip and the frozen install. Fixes #1056 Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- .../tests/e2e_redirect_gem_build.rs | 111 +++++++++ .../src/patch/redirect/upstream/gem.rs | 226 +++++++++++++++--- 3 files changed, 300 insertions(+), 39 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bb4592f4b..1cee808e7 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -964,7 +964,7 @@ v5.0 replaces v4's per-purl reverts and whole-ledger reverse replay (`revert_rem * **cargo** — `Cargo.lock` back on crates.io (source + the sparse index's checksum, `SOCKET_CRATES_INDEX`); every `Cargo.toml` declaration loses its `registry = "socket-patch-"` pin (the shorthand the rewriter produced collapses back); every `[registries.socket-patch-]` block no manifest or lock still references leaves the project cargo config — including a superseded patch generation's block an earlier re-pin left behind (#864). A declaration it cannot unpin refuses. * **golang** — the hosted `replace` and the socket module's go.sum lines go; the upstream module's two go.sum lines come back, hashed from the module proxy (`SOCKET_GOPROXY`, else `GOPROXY` / `GONOPROXY` / `GOPRIVATE` as go reads them) and cross-checked against the checksum database (`SOCKET_GOSUMDB_URL`, else `sum.golang.org` unless `GOSUMDB=off` / `GONOSUMDB` / `GOPRIVATE` say go would not ask it). A `replace` the user had before the hosted run is not recorded anywhere, so the restore lands on the plain upstream module. * **pypi** — `Pipfile.lock`, `requirements.txt` (+ in-root `-r` includes), Hatch PEP 508 direct references (`pyproject.toml` / `hatch.toml`), `poetry.lock`, `pdm.lock`, `uv.lock`, PEP 723 script locks and PEP 751 `pylock*.toml` (+ the paired `pyproject.toml` / script metadata): hashes re-derived from PyPI's JSON API (`SOCKET_PYPI_JSON_API`). A restored `requirements.txt` line gets `--hash` options only when the file is in pip's hash-checking mode. The mode is read off the file's other requirement lines (an `-e` / `--editable` line means unhashed). When every requirement is a hosted pin, it is read off the hosted line itself (`--hash` vs a `#sha256=` url fragment) (#410). Refused: a `pdm.lock` without `cross_platform`, or a uv / script / pylock lock, whose release has a wheel that is not pure Python 3 (which files the lock keeps is not re-derivable); a uv lock whose options filter files (`exclude-newer`, `no-binary`, `no-build`), or whose other registry packages name no registry, several, or one other than PyPI's simple index; a pylock whose other registry packages show neither an `index` nor (as `uv pip compile` writes them) only PyPI files with none, which restores the entry without an `index` too; uv 0.2 `[[distribution]]` locks. Restored artifact fields keep the spelling the lock's other entries show, including the `upload_time` that uv 0.6.15–0.6.17 write. A restored pylock entry's `upload-time`s are whole seconds, as uv writes them, unless the lock's other entries show fractions. Its artifacts come back in the TOML spelling the other entries use: uv's inline `wheels = [{ … }]`, or the standard tables `pip lock` writes (`[[packages.wheels]]` with a `[packages.wheels.hashes]` sub-table, `[packages.sdist]`). A `pip lock` file (`created-by = "pip"`) records only the artifact pip selected, so the entry is restored with only the release's wheel (its sdist when it has none), and a release with several wheels is refused. A transitive `override-dependencies` entry hosted mode added is removed (`upstream_uv_override_removed`). - * **gem** — `Gemfile.lock` / `gems.locked` + `Gemfile` / `gems.rb`: the spec moves back into the upstream `GEM` section (or the Socket remote leaves a merged section), the `source "" do … end` block is undone, the `CHECKSUMS` entry is re-pinned from the rubygems.org compact index (`SOCKET_RUBYGEMS_URL`) and the `DEPENDENCIES` pin loses its `!`. The declaration's original constraint is not recorded, so it comes back as the exact pin `gem "", ""`. A transitive gem (one the manifest never declared) gets an appended block with a blank line before it; the restore removes that block, its blank line and the `DEPENDENCIES` entry, so the pair comes back byte for byte. An appended block with no blank line before it (written by a release before this one) can't be told apart from an in-place rewrite, so it still comes back as the exact pin. Refused: an ambiguous upstream section, an upstream remote other than rubygems.org. The manifest pair can't make a committed bundler cache upstream: a `-.gem` left in Bundler's cache dir that isn't the upstream archive is named by `upstream_gem_stale_cache`, never deleted. + * **gem** — `Gemfile.lock` / `gems.locked` + `Gemfile` / `gems.rb`: the spec moves back into the upstream `GEM` section (or the Socket remote leaves a merged section), the `source "" do … end` block is undone, the `CHECKSUMS` entry is re-pinned from the rubygems.org compact index (`SOCKET_RUBYGEMS_URL`) and the `DEPENDENCIES` pin loses its `!`, unless the declaration comes back inside the user's own `source "…" do` block (Bundler writes the `!` for any dependency declared in one, rubygems.org included), where the `!` stays. The declaration's original constraint is not recorded, so it comes back as the exact pin `gem "", ""`. A transitive gem (one the manifest never declared) gets an appended block with a blank line before it; the restore removes that block, its blank line and the `DEPENDENCIES` entry, so the pair comes back byte for byte. An appended block with no blank line before it (written by a release before this one) can't be told apart from an in-place rewrite, so it still comes back as the exact pin. Refused: an ambiguous upstream section, an upstream remote other than rubygems.org. The manifest pair can't make a committed bundler cache upstream: a `-.gem` left in Bundler's cache dir that isn't the upstream archive is named by `upstream_gem_stale_cache`, never deleted. * **composer** — `composer.lock`: `dist` and the deleted `source` block from packagist's composer v2 metadata (`SOCKET_PACKAGIST_URL`). Refused unless the entry is packagist-sourced and packagist still serves the lock's `dist.reference` for the version. * **maven** — `pom.xml` (the `-socket.` version suffix, the added `` / `` entry) and the `.mvn/maven.config` / `.mvn/checksums/checksums.sha256` lines hosted mode writes: **no network**, so it restores under `--offline` too. `.mvn` files holding anything else keep the resolver lines (`maven_trusted_checksums_left`). * **nuget** — `nuget.config` loses the `socket-patch-` source and its exact-id mapping; every `packages.lock.json` entry of the id gets nuget.org's `contentHash` back (`SOCKET_NUGET_URL`). Refused when the restored config would not resolve the id from nuget.org alone. A config hosted mode created from scratch is kept (`nuget_default_config_left`). 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 b1ecbc022..da164ce60 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_gem_build.rs @@ -440,6 +440,10 @@ enum Driver { /// A modifier adjacent to a top-level constant is still a modifier, /// not a hash label (`if::ENV`, #340). ScanVexScopedConstantModifier, + /// [`Driver::ScanVex`] on a gem declared inside the user's own + /// `source "" do` block (#1056): bundler locks it with a `!` + /// source pin, which the unwind must keep. + ScanVexSourceBlock, /// A heredoc option continues beyond the declaration's physical line. ScanVexHeredocDeclaration, /// A double-quoted interpolation can itself contain a heredoc opener. @@ -521,6 +525,7 @@ impl Driver { Driver::ScanVexCustomLockfile => "scan --mode hosted (lockfile custom.lock)", Driver::ScanVexTwin => "scan --mode hosted (Gemfile + gems.rb twin)", Driver::ScanVexGroupBlock => "scan --mode hosted (gem in a group block)", + Driver::ScanVexSourceBlock => "scan --mode hosted (gem in a source block)", Driver::ScanVexSemicolonJoinedDeclaration => { "scan --mode hosted (two `;`-joined gem declarations)" } @@ -814,6 +819,10 @@ async fn redirect_scanned_project( "source \"{}/upstream\"\n\ngroup :development do\n gem \"{DEP}\"\nend\n", server.uri() ), + Driver::ScanVexSourceBlock => format!( + "source \"{up}/upstream\"\n\nsource \"{up}/upstream\" do\n gem \"{DEP}\", \"{DEP_VERSION}\"\nend\n", + up = server.uri() + ), Driver::ScanVexEvalGemfile => { std::fs::write(proj.join("Gemfile.common"), format!("gem \"{DEP}\"\n")).unwrap(); format!( @@ -1061,6 +1070,7 @@ async fn redirect_scanned_project( | Driver::ScanVexDuplicateDeclaration | Driver::ScanVexEvalGemfile | Driver::ScanVexGroupBlock + | Driver::ScanVexSourceBlock | Driver::ScanVexCustomGitSource | Driver::ScanVexMultiLineDeclaration | Driver::ScanVexConditionalDeclaration @@ -1263,6 +1273,7 @@ async fn redirect_scanned_project( match driver { Driver::ScanVex | Driver::ScanVexGroupBlock + | Driver::ScanVexSourceBlock | Driver::ScanVexTrailingSemicolonDeclaration => { assert_eq!(env["vex"]["statements"], 1, "vex block: {env}"); assert_eq!( @@ -2329,6 +2340,106 @@ async fn gem_hosted_group_block_pin_survives_a_refused_vendored_takeover() { } } +/// #1056: a gem the user declared inside their own `source "…" do` block +/// is locked as `name (= v)!`. After the hosted scan converges (an +/// unfrozen install), `rollback` / `remove` must hand back the exact +/// pre-scan pair, `!` included, so a FROZEN install of the restored pair +/// still succeeds (it exited 16 when the unwind dropped the `!`). +/// Converged without CHECKSUMS so the restore needs no rubygems.org sha. +#[tokio::test(flavor = "multi_thread")] +#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 2.2); \ + the unpinned `test` job skips it, an e2e job with a pinned toolchain runs it via --ignored"] +async fn gem_hosted_unwind_keeps_a_source_block_bang() { + for command in ["rollback", "remove"] { + let Some(fx) = redirect_scanned_project( + &format!("source-block {command}"), + Spelling::Gemfile, + false, + true, + None, + Driver::ScanVexSourceBlock, + ) + .await + else { + return; + }; + if !fx.bundler.at_least(2, 2) { + println!( + "SKIP e2e_redirect_gem_build (source-block unwind): bundler {} merges every \ + rubygems source into one GEM section", + fx.bundler.version + ); + return; + } + let pristine_lock = String::from_utf8(fx.pristine_lock.clone()).unwrap(); + assert!( + pristine_lock.contains(&format!(" {DEP} (= {DEP_VERSION})!\n")), + "bundler pins a source-block gem with `!` (test premise):\n{pristine_lock}" + ); + // Converge the mixed pair: the lock now resolves from the registry. + let install = bundle(&fx.proj, &["install"]); + assert!( + install.status.success(), + "converging install:\n{}", + String::from_utf8_lossy(&install.stderr) + ); + let lock = std::fs::read_to_string(fx.proj.join(fx.lock_name)).unwrap(); + assert!( + lock.contains(&format!("remote: {}", fx.index_url)), + "converged:\n{lock}" + ); + + let api = fx._server.uri(); + let cwd = fx.proj.to_str().expect("utf8 tmp path"); + let mut argv = vec![command]; + if command == "remove" { + argv.extend([PURL, "--yes"]); + } + argv.extend([ + "--json", + "--cwd", + cwd, + "--api-url", + &api, + "--org", + ORG, + "--api-token", + "fake", + "--patch-server-url", + &api, + ]); + let (code, stdout, stderr) = run_socket(&fx.proj, &argv); + assert_eq!(code, 0, "{command}:\nstdout:\n{stdout}\nstderr:\n{stderr}"); + assert_eq!( + std::fs::read_to_string(fx.proj.join(fx.lock_name)).unwrap(), + pristine_lock, + "{command}: the lock comes back byte for byte, `!` included" + ); + assert_eq!( + std::fs::read(fx.proj.join(fx.gemfile_name)).unwrap(), + fx.pristine_gemfile, + "{command}: the Gemfile comes back byte for byte" + ); + let fresh = stage_fresh_checkout(&fx, &format!("source-block-{command}")); + let install = bundle_env(&fresh, &["install"], &[("BUNDLE_FROZEN", "true")]); + assert!( + install.status.success(), + "{command}: frozen install of the restored pair:\n{}", + String::from_utf8_lossy(&install.stderr) + ); + assert_eq!( + std::fs::read(fresh_installed_lib( + &fresh, + &format!("{DEP}-{DEP_VERSION}"), + "vuln_gem.rb" + )) + .unwrap(), + orig_lib().into_bytes(), + "{command}: the upstream bytes are installed" + ); + } +} + /// One refused takeover of the group-block fixture: the /// `gemfile_declaration_not_editable` refusal (exit 1 wet; a `would_refuse` /// preview row dry), no revert (nor a preview of one), and the hosted diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs b/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs index 565f63450..fe124cd4d 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs @@ -26,7 +26,9 @@ //! from the rubygems.org compact index (`info/`, what bundler itself //! records; other upstream remotes are refused: their index may need //! credentials and is not the default registry), and the `DEPENDENCIES` -//! source pin (`name (= v)!`) loses its `!`. +//! source pin (`name (= v)!`) loses its `!` — unless the declaration comes +//! back inside the user's own `source … do` block, where bundler writes the +//! `!` itself ([`in_source_block`], #1056). //! //! What the rewrite discards is NOT derivable, so the restore picks the //! installable reading and documents it: @@ -277,13 +279,25 @@ struct Gem<'p> { version: String, } -/// Restore `gem` in the LF lock text (module docs). `transitive`: drop the -/// `DEPENDENCIES` entry instead of unpinning it. +/// What the restore does to the gem's `DEPENDENCIES` entry. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum DepEntry { + /// Drop the `!` source pin the rewriter added (module docs). + Unpin, + /// Keep the entry as is: the restored declaration sits in a user + /// `source … do` block, which bundler records with a `!` (#1056). + Keep, + /// Drop the entry: the rewriter added it for a transitive gem. + Drop, +} + +/// Restore `gem` in the LF lock text (module docs); `dep` says what becomes +/// of its `DEPENDENCIES` entry. fn lock_edit( lock: &str, gem: &Gem<'_>, globals: &[String], - transitive: bool, + dep: DepEntry, sha: Option<&str>, ctx: &Ctx<'_>, ) -> Result { @@ -346,10 +360,16 @@ fn lock_edit( }; if wiring.is_some() { if let Some((k, entry)) = dependency_line(&lines, &gem.name) { - if transitive { - drop.insert(k); - } else if let Some(unpinned) = entry.strip_suffix('!') { - replace.insert(k, format!(" {unpinned}")); + match dep { + DepEntry::Drop => { + drop.insert(k); + } + DepEntry::Unpin => { + if let Some(unpinned) = entry.strip_suffix('!') { + replace.insert(k, format!(" {unpinned}")); + } + } + DepEntry::Keep => {} } } } @@ -510,6 +530,32 @@ fn declaration_prefix(text: &str, block: &Block, eol: &str) -> String { format!("{eol}{indent}") } +/// Whether byte `at` of the manifest sits inside a `source "" do` +/// block (at any depth: a `group … do` nested in it keeps the source). +/// Bundler records a dependency declared there with a `!` source pin in the +/// lock's `DEPENDENCIES`, whichever URL the block names (#1056). A line-level +/// `do` / `end` count, like [`declaration_prefix`]'s: a block opened and +/// closed on one line nets out. +fn in_source_block(text: &str, at: usize) -> bool { + let mut open: Vec = Vec::new(); + for line in text[..at].lines() { + let code = line.split('#').next().unwrap_or_default().trim(); + let opens = code == "do" + || code.ends_with(" do") + || code.ends_with(")do") + || (code.ends_with('|') && code.contains(" do |")); + if opens { + let source = code + .strip_prefix("source") + .is_some_and(|rest| rest.starts_with([' ', '\t', '('])); + open.push(source); + } else if code == "end" || code.starts_with("end ") || code.starts_with("end.") { + open.pop(); + } + } + open.contains(&true) +} + /// How the gem comes back into the manifest. enum Decl { /// Gone: the block was the rewriter's append for a transitive gem. @@ -807,32 +853,36 @@ async fn restore_one( } else { Decl::Direct(format!(", \"{}\"", gem.version)) }; - let transitive = matches!(decl, Decl::Transitive); + // The restored declaration goes back where the block is: a user + // `source … do` block around it keeps the `!` bundler wrote for it. + let dep = match (&decl, &block, manifest) { + (Decl::Transitive, _, _) => DepEntry::Drop, + (Decl::Direct(_), Some(b), Some(text)) if in_source_block(text, b.start) => DepEntry::Keep, + (Decl::Direct(_), _, _) => DepEntry::Unpin, + }; let next_lock = match lock { None => None, - Some(text) => Some( - match lock_edit(text, gem, globals, transitive, None, ctx)? { - LockEdit::Done(next) => next, - LockEdit::NeedsSha { remote } => { - let remote = remote.ok_or("its upstream GEM remote is ambiguous")?; - if !same_remote(&remote, RUBYGEMS_REMOTE) { - return Err(format!( + Some(text) => Some(match lock_edit(text, gem, globals, dep, None, ctx)? { + LockEdit::Done(next) => next, + LockEdit::NeedsSha { remote } => { + let remote = remote.ok_or("its upstream GEM remote is ambiguous")?; + if !same_remote(&remote, RUBYGEMS_REMOTE) { + return Err(format!( "its upstream GEM remote {remote} is not rubygems.org, so its CHECKSUMS \ sha256 cannot be re-derived" )); - } - let sha = ctx - .client - .rubygems_sha256(&gem.name, &gem.version) - .await - .map_err(|why| format!("{} {}: {why}", gem.name, gem.version))?; - match lock_edit(text, gem, globals, transitive, Some(&sha), ctx)? { - LockEdit::Done(next) => next, - LockEdit::NeedsSha { .. } => unreachable!("a sha was supplied"), - } } - }, - ), + let sha = ctx + .client + .rubygems_sha256(&gem.name, &gem.version) + .await + .map_err(|why| format!("{} {}: {why}", gem.name, gem.version))?; + match lock_edit(text, gem, globals, dep, Some(&sha), ctx)? { + LockEdit::Done(next) => next, + LockEdit::NeedsSha { .. } => unreachable!("a sha was supplied"), + } + } + }), }; let next_manifest = match (manifest, &block) { (Some(text), Some(b)) => Some(restore_manifest(text, b, gem, &decl)), @@ -886,11 +936,11 @@ mod tests { ruby\n\nDEPENDENCIES\n puma\n rails (= 7.0.0)\n\nBUNDLED WITH\n 2.4.0\n"; // No CHECKSUMS (bundler 2.2–2.5): no registry lookup, offline works. assert_eq!( - done(lock_edit(&hosted, &gem(), &[], false, None, &ctx).unwrap()), + done(lock_edit(&hosted, &gem(), &[], DepEntry::Unpin, None, &ctx).unwrap()), want ); // Transitive: the DEPENDENCIES entry the rewriter added goes. - let out = done(lock_edit(&hosted, &gem(), &[], true, None, &ctx).unwrap()); + let out = done(lock_edit(&hosted, &gem(), &[], DepEntry::Drop, None, &ctx).unwrap()); assert!(out.contains("DEPENDENCIES\n puma\n\n"), "{out}"); } @@ -904,7 +954,7 @@ mod tests { sha256={}\n", "d".repeat(64) ); - match lock_edit(&hosted, &gem(), &[], false, None, &ctx).unwrap() { + match lock_edit(&hosted, &gem(), &[], DepEntry::Unpin, None, &ctx).unwrap() { LockEdit::NeedsSha { remote } => { assert_eq!(remote.as_deref(), Some("https://rubygems.org/")) } @@ -912,7 +962,7 @@ mod tests { } let sha = "2".repeat(64); assert_eq!( - done(lock_edit(&hosted, &gem(), &[], false, Some(&sha), &ctx).unwrap()), + done(lock_edit(&hosted, &gem(), &[], DepEntry::Unpin, Some(&sha), &ctx).unwrap()), format!( "GEM\n remote: https://rubygems.org/\n specs:\n rails (7.0.0)\n\nDEPENDENCIES\n \ rails (= 7.0.0)\n\nCHECKSUMS\n rails (7.0.0) sha256={sha}\n" @@ -921,7 +971,7 @@ mod tests { // The rewriter's ADDED entry next to bundler's bare one is dropped. let added = hosted.replace("CHECKSUMS\n", "CHECKSUMS\n rails (7.0.0)\n"); assert!( - done(lock_edit(&added, &gem(), &[], false, None, &ctx).unwrap()) + done(lock_edit(&added, &gem(), &[], DepEntry::Unpin, None, &ctx).unwrap()) .ends_with("CHECKSUMS\n rails (7.0.0)\n") ); } @@ -935,7 +985,7 @@ mod tests { rails (7.0.0)\n\nDEPENDENCIES\n puma\n rails (= 7.0.0)!\n\nBUNDLED WITH\n 2.1.4\n" ); assert_eq!( - done(lock_edit(&hosted, &gem(), &[], false, None, &ctx).unwrap()), + done(lock_edit(&hosted, &gem(), &[], DepEntry::Unpin, None, &ctx).unwrap()), "GEM\n remote: https://rubygems.org/\n specs:\n puma (6.0.0)\n rails (7.0.0)\n\n\ DEPENDENCIES\n puma\n rails (= 7.0.0)\n\nBUNDLED WITH\n 2.1.4\n" ); @@ -949,10 +999,20 @@ mod tests { "GEM\n remote: https://gems.example/\n specs:\n\nGEM\n remote: https://mirror.example/\n \ specs:\n\nGEM\n remote: {IDX}\n specs:\n rails (7.0.0)\n\nDEPENDENCIES\n rails (= 7.0.0)!\n" ); - assert!(lock_edit(&two_upstreams, &gem(), &[], false, None, &ctx).is_err()); + assert!(lock_edit(&two_upstreams, &gem(), &[], DepEntry::Unpin, None, &ctx).is_err()); // The manifest's global source singles one out. let globals = vec!["https://mirror.example".to_string()]; - let out = done(lock_edit(&two_upstreams, &gem(), &globals, false, None, &ctx).unwrap()); + let out = done( + lock_edit( + &two_upstreams, + &gem(), + &globals, + DepEntry::Unpin, + None, + &ctx, + ) + .unwrap(), + ); assert!( out.contains("remote: https://mirror.example/\n specs:\n rails (7.0.0)\n"), "{out}" @@ -962,11 +1022,11 @@ mod tests { "GEM\n remote: https://rubygems.org/\n specs:\n\nGEM\n remote: {IDX}\n specs:\n \ rack (3.0.0)\n rails (7.0.0)\n\nDEPENDENCIES\n rails (= 7.0.0)!\n" ); - assert!(lock_edit(&extra, &gem(), &[], false, None, &ctx).is_err()); + assert!(lock_edit(&extra, &gem(), &[], DepEntry::Unpin, None, &ctx).is_err()); let none = format!( "GEM\n remote: {IDX}\n specs:\n rails (7.0.0)\n\nDEPENDENCIES\n rails!\n" ); - assert!(lock_edit(&none, &gem(), &[], false, None, &ctx).is_err()); + assert!(lock_edit(&none, &gem(), &[], DepEntry::Unpin, None, &ctx).is_err()); } #[test] @@ -1233,6 +1293,96 @@ mod tests { assert_eq!(next_lock.as_deref(), Some(lock.as_str())); } + /// #1056: a gem the user declared inside a `source "…" do` block is + /// locked with a `!` source pin in `DEPENDENCIES` (bundler writes it + /// for any dependency with an explicit source, rubygems.org included). + /// The unwind puts the declaration back inside that block, so the `!` + /// must stay or every frozen install refuses the restored pair. + #[tokio::test] + async fn source_block_declaration_keeps_its_bang_through_restore() { + let client = super::super::UpstreamClient::new(true); + let upstream_sha = "2".repeat(64); + client + .seed_rubygems_sha256("rails", "7.0.0", &upstream_sha) + .await; + let ctx = ctx_with(&client); + for manifest in [ + "source \"https://rubygems.org\"\n\ngem \"puma\"\nsource \"https://rubygems.org\" do\n \ + gem \"rails\", \"7.0.0\"\nend\n", + // Nested in a group inside the source block, parenthesized opener. + "source \"https://rubygems.org\"\n\ngem \"puma\"\nsource(\"https://rubygems.org\") do\n \ + group :web do\n gem \"rails\", \"7.0.0\"\n end\nend\n", + ] { + let lock = format!( + "GEM\n remote: https://rubygems.org/\n specs:\n puma (6.0.0)\n \ + rails (7.0.0)\n\nPLATFORMS\n ruby\n\nDEPENDENCIES\n puma\n \ + rails (= 7.0.0)!\n\nCHECKSUMS\n puma (6.0.0) sha256={}\n rails (7.0.0) \ + sha256={upstream_sha}\n\nBUNDLED WITH\n 4.0.22\n", + "a".repeat(64) + ); + let files = BTreeMap::from([ + ("Gemfile".to_string(), manifest.to_string()), + ("Gemfile.lock".to_string(), lock.clone()), + ]); + let r = crate::patch::redirect::rewrite_registry_redirect( + &files, + &[crate::patch::redirect::DepOverride { + ecosystem: "gem".into(), + name: "rails".into(), + namespace: None, + version: "7.0.0".into(), + token: "tok".into(), + patch_uuid: UUID.into(), + artifact_url: "https://patch.test/rails-7.0.0.gem".into(), + registry_override: Some(crate::patch::redirect::RegistryOverride { + kind: "rubygems-compact-index".into(), + index_url: IDX.into(), + identifiers: crate::patch::redirect::RegistryOverrideIdentifiers { + name: "rails".into(), + version: "7.0.0".into(), + gem_checksum_sha256: Some("f".repeat(64)), + ..Default::default() + }, + }), + integrity: Default::default(), + }], + ); + let (hosted_manifest, hosted_lock) = (&r.files["Gemfile"], &r.files["Gemfile.lock"]); + assert!(hosted_lock.contains(IDX), "{hosted_lock}"); + let (next_lock, next_manifest) = restore_one( + &gem(), + Some(hosted_lock), + Some(hosted_manifest), + "Gemfile", + &global_sources(hosted_manifest), + &ctx, + ) + .await + .unwrap() + .expect("the redirect is found"); + assert_eq!(next_manifest.as_deref(), Some(manifest)); + assert_eq!(next_lock.as_deref(), Some(lock.as_str())); + } + } + + #[test] + fn source_block_detection() { + let at = |text: &str| in_source_block(text, text.find("gem \"rails\"").unwrap()); + assert!(!at("source \"https://rubygems.org\"\ngem \"rails\"\n")); + assert!(!at("group :test do\n gem \"rails\"\nend\n")); + assert!(at( + "source \"https://x\" do # comment\n gem \"rails\"\nend\n" + )); + // A closed source block before the declaration does not count. + assert!(!at( + "source \"https://x\" do\n gem \"a\"\nend\ngroup :t do |g|\n gem \"rails\"\nend\n" + )); + // A one-line block opens and closes on its line. + assert!(!at( + "source \"https://x\" do gem \"a\" end\ngem \"rails\"\n" + )); + } + /// A hosted Gemfile + lock pair for `rails 7.0.0` (a direct gem), from /// the real forward rewrite: `checksums` picks a converged /// (bundler 2.6+) or a pre-CHECKSUMS (2.5) lock. From 9b9441de8a249d7176ed05e658a244b0b26972dc Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 18:50:12 +0000 Subject: [PATCH 3/4] Count keyword blocks when finding a Gemfile source block in_source_block pushed only on `do` lines but popped on every `end`, so the `end` of an `if`/`case`/`def`/`begin` inside a user `source "..." do` block cleared the source from the stack. restore_one then chose Unpin and stripped the DEPENDENCIES `!` for a gem still inside that block, and the frozen install failed as in #1056. Push a non-source entry for line-leading keyword constructs (not modifiers, not ones closed on the same line) so their `end` pops that instead. Co-Authored-By: Claude --- .../src/patch/redirect/upstream/gem.rs | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs b/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs index fe124cd4d..2180db8ae 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs @@ -535,7 +535,9 @@ fn declaration_prefix(text: &str, block: &Block, eol: &str) -> String { /// Bundler records a dependency declared there with a `!` source pin in the /// lock's `DEPENDENCIES`, whichever URL the block names (#1056). A line-level /// `do` / `end` count, like [`declaration_prefix`]'s: a block opened and -/// closed on one line nets out. +/// closed on one line nets out. Keyword constructs (`if`, `case`, `def`, …) +/// that start a line are counted too, so their `end` does not close an +/// enclosing `source … do` block. fn in_source_block(text: &str, at: usize) -> bool { let mut open: Vec = Vec::new(); for line in text[..at].lines() { @@ -549,6 +551,8 @@ fn in_source_block(text: &str, at: usize) -> bool { .strip_prefix("source") .is_some_and(|rest| rest.starts_with([' ', '\t', '('])); open.push(source); + } else if opens_keyword_block(code) { + open.push(false); } else if code == "end" || code.starts_with("end ") || code.starts_with("end.") { open.pop(); } @@ -556,6 +560,20 @@ fn in_source_block(text: &str, at: usize) -> bool { open.contains(&true) } +/// Whether the (comment-stripped, trimmed) line opens a keyword construct +/// that Ruby closes with `end`: one that starts the line, not a modifier +/// (`gem "x" if cond`), and not closed on the same line. +fn opens_keyword_block(code: &str) -> bool { + let first = code + .split(|c: char| c.is_whitespace() || c == '(' || c == ';') + .next() + .unwrap_or_default(); + matches!( + first, + "if" | "unless" | "while" | "until" | "case" | "begin" | "def" | "class" | "module" + ) && !(code.ends_with(" end") || code.ends_with(";end")) +} + /// How the gem comes back into the manifest. enum Decl { /// Gone: the block was the rewriter's append for a transitive gem. @@ -1381,6 +1399,19 @@ mod tests { assert!(!at( "source \"https://x\" do gem \"a\" end\ngem \"rails\"\n" )); + // The `end` of an `if`/`case`/`def` inside the source block does + // not close it. + assert!(at( + "source \"https://x\" do\n if ENV[\"A\"]\n gem \"a\"\n end\n gem \"rails\"\nend\n" + )); + assert!(at( + "source \"https://x\" do\n case RUBY_ENGINE\n when \"jruby\" then gem \"a\"\n end\n def x; end\n gem \"rails\"\nend\n" + )); + // A modifier `if` opens nothing. + assert!(!at( + "source \"https://x\" do\n gem \"a\" if true\nend\ngem \"rails\"\n" + )); + assert!(!at("if true\n gem \"a\"\nend\ngem \"rails\"\n")); } /// A hosted Gemfile + lock pair for `rails 7.0.0` (a direct gem), from From 3d73a04409dddc135115815a3361432daafcbf43 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Sat, 10 Oct 2026 10:16:33 -0400 Subject: [PATCH 4/4] Keep a quoted # out of the Gemfile comment strip in_source_block and declaration_prefix cut each line at the first '#', so a user source line interpolating its URL ("https://#{host}/") lost its trailing do, never opened a source frame, and the restore unpinned the lock's ! for a gem put back inside that block. ruby_code now strips only a # outside a quoted string. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/patch/redirect/upstream/gem.rs | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs b/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs index 307c7432c..c44c6f24b 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/gem.rs @@ -514,7 +514,7 @@ fn declaration_prefix(text: &str, block: &Block, eol: &str) -> String { if trimmed.is_empty() { return String::new(); } - let code = trimmed.split('#').next().unwrap_or_default().trim_end(); + let code = ruby_code(trimmed).trim_end(); if code == "do" || code.ends_with(" do") || (code.ends_with('|') && code.contains(" do |")) { return format!("{} ", indent_of(prev)); } @@ -541,7 +541,7 @@ fn declaration_prefix(text: &str, block: &Block, eol: &str) -> String { fn in_source_block(text: &str, at: usize) -> bool { let mut open: Vec = Vec::new(); for line in text[..at].lines() { - let code = line.split('#').next().unwrap_or_default().trim(); + let code = ruby_code(line).trim(); let opens = code == "do" || code.ends_with(" do") || code.ends_with(")do") @@ -560,6 +560,26 @@ fn in_source_block(text: &str, at: usize) -> bool { open.contains(&true) } +/// `line` without its trailing `#` comment. A `#` inside a quoted string +/// (`"https://#{host}/"` interpolation, a URL fragment) is code, not a +/// comment. +fn ruby_code(line: &str) -> &str { + let mut quote: Option = None; + let mut escaped = false; + for (i, c) in line.char_indices() { + match quote { + Some(_) if escaped => escaped = false, + Some(_) if c == '\\' => escaped = true, + Some(q) if c == q => quote = None, + Some(_) => {} + None if c == '"' || c == '\'' => quote = Some(c), + None if c == '#' => return &line[..i], + None => {} + } + } + line +} + /// Whether the (comment-stripped, trimmed) line opens a keyword construct /// that Ruby closes with `end`: one that starts the line, not a modifier /// (`gem "x" if cond`), and not closed on the same line. @@ -1423,6 +1443,14 @@ mod tests { "source \"https://x\" do\n gem \"a\" if true\nend\ngem \"rails\"\n" )); assert!(!at("if true\n gem \"a\"\nend\ngem \"rails\"\n")); + // A `#` inside a string (`#{}` interpolation) is no comment: the + // trailing `do` still opens the source block. + assert!(at( + "source \"https://#{ENV[\"HOST\"]}/gems\" do\n gem \"rails\"\nend\n" + )); + assert!(at( + "source 'https://x/#frag' do # comment\n gem \"rails\"\nend\n" + )); } /// A hosted Gemfile + lock pair for `rails 7.0.0` (a direct gem), from