Fix gem rewrite deleting a ;-joined declaration (#826) (#875)

* Start fix for #826

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

* Keep gem declarations sharing a line with ;

A Gemfile line like `gem "a", "1"; gem "b", "2"` had its second
declaration deleted when socket-patch redirected or vendored gem "a",
because the rewrite replaces the whole line and the safety check did
not know that `;` starts a new statement. The next frozen
`bundle install` then failed. Such lines are now refused with a
warning and left untouched.

A declaration ending in a bare `;` (optionally followed by a comment)
was refused as "continues on the next line" since #637. It is complete,
so it is rewritten again, without the `;`.

Fixes #826

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

* Use the reported line shape in the ; e2e test

The `;`-joined fixture had no version argument, so the old check
already refused it as "unexpected tokens" and the test passed without
the fix. Use `gem "x", "v"; gem "y", "v"` from #826, which the old code
rewrote and lost the second gem.

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

* Port #878: route Gradle digests through helpers

main is red: #646 added inline sha1/sha256 calls that #865's
production_digests_go_through_the_helpers guard rejects. This ports
the fix from #878 so this PR's coverage job can go green. It becomes a
no-op once #878 lands.

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

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: mikolalysenko

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

@@ -445,6 +445,13 @@ enum Driver {
     ScanVexHeredocDeclaration,
     /// A double-quoted interpolation can itself contain a heredoc opener.
     ScanVexInterpolatedHeredocDeclaration,
+    /// A second declaration joined to the gem's line by `;` (#826): the
+    /// line rewrite would delete it. Same contract as
+    /// [`Driver::ScanVexDuplicateDeclaration`].
+    ScanVexSemicolonJoinedDeclaration,
+    /// [`Driver::ScanVex`] on a declaration ending in a bare `;` and a
+    /// comment (#826): a complete declaration, so it is redirected.
+    ScanVexTrailingSemicolonDeclaration,
     /// [`Driver::ScanVexDualBoot`] with `BUNDLE_GEMFILE=Gemfile` exported to
     /// socket-patch too (#507): bundler's local app config outranks the
     /// environment, so bundler still loads `Gemfile.next` and the run must
@@ -495,6 +502,12 @@ impl Driver {
             Driver::ScanVexDualBootEnvGemfile => {
                 "scan --mode hosted (config Gemfile.next, env BUNDLE_GEMFILE=Gemfile)"
             }
+            Driver::ScanVexSemicolonJoinedDeclaration => {
+                "scan --mode hosted (two `;`-joined gem declarations)"
+            }
+            Driver::ScanVexTrailingSemicolonDeclaration => {
+                "scan --mode hosted (gem line ending in `;`)"
+            }
             Driver::ScanVexCustomGitSource => "scan --mode hosted (gem from a custom git_source)",
         }
     }
@@ -833,6 +846,14 @@ async fn redirect_scanned_project(
             "source \"{}/upstream\"\n\ngem \"{DEP}\", require: \"#{{<<~REQUIRE_PATH}}\".chomp\n  vuln_gem\nREQUIRE_PATH\n",
             server.uri()
         ),
+        Driver::ScanVexSemicolonJoinedDeclaration => format!(
+            "source \"{}/upstream\"\n\ngem \"{DEP}\", \"{DEP_VERSION}\"; gem \"{TRANSITIVE}\", \"1.0.0\"\n",
+            server.uri()
+        ),
+        Driver::ScanVexTrailingSemicolonDeclaration => format!(
+            "source \"{}/upstream\"\n\ngem \"{DEP}\"; # the vulnerable one\n",
+            server.uri()
+        ),
         _ => format!("source \"{}/upstream\"\n\ngem \"{DEP}\"\n", server.uri()),
     };
     std::fs::write(proj.join(gemfile_name), gemfile_body).unwrap();
@@ -984,7 +1005,9 @@ async fn redirect_scanned_project(
         | Driver::ScanVexConditionalDeclaration
         | Driver::ScanVexScopedConstantModifier
         | Driver::ScanVexHeredocDeclaration
-        | Driver::ScanVexInterpolatedHeredocDeclaration => vec![
+        | Driver::ScanVexInterpolatedHeredocDeclaration
+        | Driver::ScanVexSemicolonJoinedDeclaration
+        | Driver::ScanVexTrailingSemicolonDeclaration => vec![
             "scan",
             "--mode",
             "hosted",
@@ -1060,7 +1083,8 @@ async fn redirect_scanned_project(
         | Driver::ScanVexConditionalDeclaration
         | Driver::ScanVexScopedConstantModifier
         | Driver::ScanVexHeredocDeclaration
-        | Driver::ScanVexInterpolatedHeredocDeclaration => {
+        | Driver::ScanVexInterpolatedHeredocDeclaration
+        | Driver::ScanVexSemicolonJoinedDeclaration => {
             Some("redirect_gem_unrecognized_declaration")
         }
         _ => None,
@@ -1147,7 +1171,7 @@ async fn redirect_scanned_project(
         );
     }
     match driver {
-        Driver::ScanVex => {
+        Driver::ScanVex | Driver::ScanVexTrailingSemicolonDeclaration => {
             assert_eq!(env["vex"]["statements"], 1, "vex block: {env}");
             assert_eq!(
                 env["vex"]["verified"], false,
@@ -1168,7 +1192,8 @@ async fn redirect_scanned_project(
         | Driver::ScanVexConditionalDeclaration
         | Driver::ScanVexScopedConstantModifier
         | Driver::ScanVexHeredocDeclaration
-        | Driver::ScanVexInterpolatedHeredocDeclaration => {
+        | Driver::ScanVexInterpolatedHeredocDeclaration
+        | Driver::ScanVexSemicolonJoinedDeclaration => {
             unreachable!("asserted and returned above")
         }
         Driver::GetUuid => {
@@ -1956,6 +1981,53 @@ async fn gem_hosted_multi_line_declaration_is_refused_and_still_installs() {
     assert!(fx.is_none(), "the multi-line driver asserts in place");
 }
 
+/// #826: `gem "x"; gem "y"` must not be rewritten. The line rewrite
+/// deleted `gem "y"`, so the next frozen install failed.
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17); \
+            run with a pinned toolchain via --ignored"]
+async fn gem_hosted_semicolon_joined_declarations_are_refused_and_still_install() {
+    let fx = redirect_scanned_project(
+        "semicolon-joined",
+        Spelling::Gemfile,
+        false,
+        true,
+        None,
+        Driver::ScanVexSemicolonJoinedDeclaration,
+    )
+    .await;
+    assert!(fx.is_none(), "the `;`-joined driver asserts in place");
+}
+
+/// #826: `gem "x"; # c` is a complete declaration. Since #637 it was
+/// refused as continuing on the next line; it must redirect, and a fresh
+/// checkout must install the patched bytes.
+#[tokio::test(flavor = "multi_thread")]
+#[ignore = "host capstone: shells out to a real ruby/gem/bundler (>= 1.17); \
+            run with a pinned toolchain via --ignored"]
+async fn gem_hosted_trailing_semicolon_declaration_redirects_and_installs() {
+    let Some(fx) = redirect_scanned_project(
+        "trailing-semicolon",
+        Spelling::Gemfile,
+        false,
+        true,
+        None,
+        Driver::ScanVexTrailingSemicolonDeclaration,
+    )
+    .await
+    else {
+        return;
+    };
+    let (fresh, install) = fresh_checkout_bundle_install(&fx);
+    assert!(
+        install.status.success(),
+        "fresh-checkout `bundle install` must succeed from the patch registry.\nstdout:\n{}\nstderr:\n{}",
+        String::from_utf8_lossy(&install.stdout),
+        String::from_utf8_lossy(&install.stderr),
+    );
+    assert_patched_install(&fx, &fresh);
+}
+
 /// #340: a `gem` declaration with an `if` modifier must not be rewritten
 /// (the rewrite dropped the condition and declared the gem unconditionally).
 #[tokio::test(flavor = "multi_thread")]

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

@@ -243,6 +243,7 @@ pub(crate) struct SourceOption {
 /// it may carry one. Positional arguments (`*V`, constants, method calls)
 /// are version constraints and never do.
 pub(crate) fn source_option(tail: &str) -> Option<SourceOption> {
+    let tail = &without_statement_end(tail);
     let Some(args) = args(tail) else {
         return Some(SourceOption {
             key: tail.trim().to_string(),
@@ -273,6 +274,7 @@ pub(crate) fn source_option(tail: &str) -> Option<SourceOption> {
 /// `path:`. Empty when the line carries none; bails to empty on an
 /// unparseable tail (unbalanced quote or bracket).
 pub(crate) fn trailing_options(tail: &str) -> String {
+    let tail = &without_statement_end(tail);
     let Some(args) = args(tail) else {
         return String::new();
     };
@@ -284,6 +286,43 @@ pub(crate) fn trailing_options(tail: &str) -> String {
         .unwrap_or_default()
 }
 
+/// `opts` minus a top-level `;` statement terminator (and any extra `;`s),
+/// keeping a trailing `#` comment. Both rewriters first refuse a tail where
+/// another statement follows the `;` (`gem_line_tail_blocks_edit`), so only
+/// `;`s, whitespace and a comment can follow it here (#826).
+fn without_statement_end(opts: &str) -> String {
+    let mut quote: Option<char> = None;
+    let mut depth: i64 = 0;
+    let mut chars = opts.char_indices();
+    while let Some((i, c)) = chars.next() {
+        if let Some(q) = quote {
+            if c == '\\' {
+                chars.next();
+            } else if c == q {
+                quote = None;
+            }
+            continue;
+        }
+        match c {
+            '#' => break,
+            '"' | '\'' => quote = Some(c),
+            '(' | '[' | '{' => depth += 1,
+            ')' | ']' | '}' => depth -= 1,
+            ';' if depth == 0 => {
+                let code = opts[..i].trim_end();
+                let rest = opts[i..].trim_start_matches(|c: char| c == ';' || c.is_whitespace());
+                return if rest.is_empty() {
+                    code.to_string()
+                } else {
+                    format!("{code} {rest}")
+                };
+            }
+            _ => {}
+        }
+    }
+    opts.to_string()
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -397,4 +436,30 @@ mod tests {
             "require: \"a,b\", group: [:x, :y]"
         );
     }
+
+    /// #826: a bare `;` ending the statement is not part of the options
+    /// (it would otherwise read as a positional `"7.0";` and be kept).
+    #[test]
+    fn trailing_options_drop_the_statement_terminator() {
+        // Nor is it an unreadable tail, which would fail closed as a
+        // source-selecting option.
+        for tail in [";", "; # c", ", \"0.8.1\";", ", require: false; # c"] {
+            assert_eq!(key(tail), None, "{tail:?}");
+        }
+        assert_eq!(key(", git: \"x\";"), Some("git:".into()));
+        for (tail, opts) in [
+            (", \"0.8.1\";", ""),
+            (", \"0.8.1\"; # c", ""),
+            (", require: false;", "require: false"),
+            (
+                ", require: false ;; # lazy; ok",
+                "require: false # lazy; ok",
+            ),
+            (", require: \"a;b\";", "require: \"a;b\""),
+            (", require: \"a;b\"", "require: \"a;b\""),
+            (", require: false # x;", "require: false # x;"),
+        ] {
+            assert_eq!(trailing_options(tail), opts, "{tail:?}");
+        }
+    }
 }

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

@@ -5651,7 +5651,9 @@ fn gem_source_option_detail(dep: &DepOverride, what: &str, socket_vendored: bool
 ///   after the rewrite, and bundler refuses the Gemfile;
 /// - a modifier (`if` / `unless` / `while` / `until` / `rescue` / `and` /
 ///   `or`) or a `do` block would be dropped, silently changing when the gem
-///   is declared.
+///   is declared;
+/// - another statement after a top-level `;` would be deleted with the line
+///   (#826). A bare trailing `;` ends the declaration and is fine.
 ///
 /// Only code outside ordinary string literals and before a `#` comment
 /// counts, so a keyword or `,` inside `require: "…"` or a comment is fine.
@@ -5684,6 +5686,18 @@ pub(crate) fn gem_line_tail_blocks_edit(tail: &str) -> Option<String> {
         }
         match c {
             '#' => break,
+            // A top-level `;` ends the declaration's statement. Anything
+            // after it but more `;`s or a comment is another statement on
+            // the line the rewrite replaces, so it would be deleted (#826).
+            ';' if depth == 0 => {
+                let rest = chars
+                    .as_str()
+                    .trim_start_matches(|c: char| c == ';' || c.is_whitespace());
+                if rest.is_empty() || rest.starts_with('#') {
+                    break;
+                }
+                return Some("another statement follows the declaration on its line".to_string());
+            }
             '"' | '\'' => quote = Some(c),
             '(' | '[' | '{' => depth += 1,
             ')' | ']' | '}' => depth -= 1,
@@ -13915,6 +13929,113 @@ mod tests {
         }
     }
 
+    /// #826: a top-level `;` ends the declaration's statement. Another
+    /// statement after it (`gem "a", "1"; gem "b", "2"`) shares the line the
+    /// rewrite replaces, so it would be deleted: refuse. A bare trailing
+    /// `;` (optionally before a comment) ends nothing else, so it is a
+    /// complete one-line declaration, not a continuation.
+    #[test]
+    fn gem_line_tail_semicolon_statements() {
+        for tail in [
+            ", \"0.8.1\"; gem \"rainbow\", \"3.1.1\"",
+            ", \"0.8.1\";gem \"rainbow\"",
+            ", require: false; gem \"rainbow\" # c",
+            ";gem \"rainbow\"",
+            ", \"0.8.1\"; ; puts 1",
+        ] {
+            let reason = gem_line_tail_blocks_edit(tail);
+            assert!(
+                reason
+                    .as_deref()
+                    .is_some_and(|r| r.contains("another statement")),
+                "{tail:?}: {reason:?}"
+            );
+        }
+        for tail in [
+            ", \"0.8.1\";",
+            ", \"0.8.1\"; ",
+            ", \"0.8.1\"; # c",
+            ", \"0.8.1\";; ",
+            ", require: false;",
+            ", require: \"a;b\"",
+            ", require: \"a\" # x; gem \"b\"",
+            ";",
+        ] {
+            assert_eq!(gem_line_tail_blocks_edit(tail), None, "{tail:?}");
+        }
+    }
+
+    /// #826: the hosted rewrite replaces the whole physical line, so a
+    /// second `;`-joined declaration on it must refuse instead of being
+    /// deleted (the next `bundle install` would drop that dependency).
+    #[test]
+    fn gemfile_semicolon_joined_declarations_fail_closed() {
+        let lock = "GEM\n  remote: https://rubygems.org/\n  specs:\n    rainbow (3.1.1)\n    \
+                    vuln-gem (1.0.0)\n\nPLATFORMS\n  ruby\n\nDEPENDENCIES\n  rainbow (= 3.1.1)\n  \
+                    vuln-gem (= 1.0.0)\n\nBUNDLED WITH\n   4.0.17\n";
+        for decl in [
+            "gem \"vuln-gem\", \"1.0.0\"; gem \"rainbow\", \"3.1.1\"",
+            "gem \"vuln-gem\", \"1.0.0\";gem \"rainbow\", \"3.1.1\" # pair",
+            "gem \"vuln-gem\", require: false; gem \"rainbow\", \"3.1.1\"",
+        ] {
+            let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n");
+            let files = BTreeMap::from([
+                ("Gemfile".to_string(), gemfile),
+                ("Gemfile.lock".to_string(), lock.to_string()),
+            ]);
+            let r = rewrite_registry_redirect(&files, &[gem_override("vuln-gem", "1.0.0")]);
+            assert!(
+                r.files.is_empty() && r.edits.is_empty(),
+                "{decl:?} must not be rewritten: files={:?}",
+                r.files
+            );
+            assert_eq!(
+                warning_codes(&r),
+                vec!["redirect_gem_unrecognized_declaration"],
+                "{decl:?}: {:?}",
+                r.warnings
+            );
+        }
+    }
+
+    /// #826 (the #637 regression): a declaration ending in a bare `;`,
+    /// with or without a trailing comment, is complete and still rewrites.
+    #[test]
+    fn gemfile_trailing_semicolon_declaration_rewrites() {
+        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\
+                    BUNDLED WITH\n   4.0.17\n";
+        for (decl, want) in [
+            (
+                "gem \"vuln-gem\", \"1.0.0\";",
+                "  gem \"vuln-gem\", \"1.0.0\"\nend",
+            ),
+            (
+                "gem \"vuln-gem\", \"1.0.0\"; # c",
+                "  gem \"vuln-gem\", \"1.0.0\"\nend",
+            ),
+            (
+                "gem \"vuln-gem\", require: false;",
+                "  gem \"vuln-gem\", \"1.0.0\", require: false\nend",
+            ),
+        ] {
+            let gemfile = format!("source \"https://rubygems.org\"\n\n{decl}\n");
+            let files = BTreeMap::from([
+                ("Gemfile".to_string(), gemfile),
+                ("Gemfile.lock".to_string(), lock.to_string()),
+            ]);
+            let r = rewrite_registry_redirect(&files, &[gem_override("vuln-gem", "1.0.0")]);
+            assert!(
+                !warning_codes(&r).contains(&"redirect_gem_unrecognized_declaration"),
+                "{decl:?}: {:?}",
+                r.warnings
+            );
+            let out = r.files.get("Gemfile").expect("declaration rewritten");
+            assert!(out.contains(want), "{decl:?}: {out}");
+            assert!(!out.contains(';'), "{decl:?}: {out}");
+        }
+    }
+
     /// Control for #340: single-line declarations whose tails merely look
     /// like the refused shapes (a keyword inside a string or a comment, a
     /// symbol or a key named like a keyword, a closed bracket) still