Skip to content

Fix gem restore of Bundler 4.1 override targets (#1989) - #1991

Merged
Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-gem-override-transitive-restore
Nov 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-gem-override-transitive-restore

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1989

Summary

Hosted gem rollback / remove no longer turn a gem reached only through a Bundler 4.1 override "other", with: "gem", in: "parent" into a new exact top-level pin. The Gemfile and Gemfile.lock now come back byte for byte, as they do for an ordinary transitive gem. Before this fix, the stray gem "vuln-gem", "1.0.0" pin blocked the project from updating past the vulnerable version.

Root cause

restore_one (crates/socket-patch-core/src/patch/redirect/upstream/gem.rs) only treats the appended Socket block as a transitive gem's when is_subdependency finds another locked spec depending on the gem. It checked only 6-space spec sub-lines. A Bundler 4.1 dependency override keeps the parent's original edge in the specs (parent (1.0.0) → other-dep) and records the replacement only in the lock's new OVERRIDES section. No sub-line names the gem, so the restore took the Decl::Direct arm.

Change

  • is_subdependency also counts the with: target of any OVERRIDES entry, via a small override_targets reader.
  • The reader parses the form Bundler writes (Override#to_lock joins inspected values with , ). That covers version: requirements containing , , and with: nil yields no target.
  • Module docs for the provably-transitive rule now mention the OVERRIDES source.
  • Vendored revert is unaffected: it records the pre-vendor DEPENDENCIES entry rather than inferring it. No wrapper (npm/, pypi/, gem/) changes are needed.

Test evidence

Issue Regression test Without fix With fix
#1989 override_target_redirect_round_trips_through_restore (hosted rewrite → restore_one on a Bundler 4.1 OVERRIDES pair) fail: manifest gained gem "rails", "7.0.0" pass, byte-exact
#1989 an_overrides_with_target_is_a_subdependency (argument order, version: with a comma, with: nil, name as target/parent/prefix, section scoping) fail pass
  • The fixture's OVERRIDES shape was checked against a real lock written by bundle _4.1.0.beta3_ lock for the issue's Gemfile. It has the same section position and entry format.
  • cargo fmt --all -- --check: clean.
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-core --all-features --lib: 6997 passed, 4 failed. The 4 also fail on unmodified main in this sandbox, because it runs as root (copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry, pypi_poetry::wire_write_failure_…, pypi_requirements::wire_failure_…; same as Build: share one test RNG across randomized tests (#1868) #1882 and Build: share one test env guard in core (#1889) #1891).
  • cargo test -p socket-patch-cli --all-features --lib --bins: 940 passed.
  • cargo test -p socket-patch-cli --all-features --test e2e_redirect_gem_build --test e2e_vendor_gem_build -- --ignored with real Ruby 3.3.6 and Bundler: 37 + 11 passed.
  • The full cargo test --workspace ran out of this sandbox's disk allowance while linking the ~100 CLI integration binaries, so CI covers the rest.

Review findings not fixed (/code-review high)

  • "override_targets only recognizes one serialization": Bundler 4.1 writes exactly one form. Override#to_lock is [target.inspect, "with: #{op.inspect}", "in: #{from.inspect}", ...].join(", "), and its own LOCK_LINE parser accepts only that form.
  • "a stale in: parent still counts": the existing sub-line rule takes the same risk. Both are only a secondary guard behind provably_appended (an option-less block after a blank line that the in-place rewrite cannot produce).
  • "no global override without in:": Bundler rejects with: without in: (add_dependency_override raises).
  • "re-scans section ranges": this is one extra linear pass per restored gem, so it's negligible.

Follow-ups

  • No e2e leg runs Bundler 4.1 yet: CI pins Bundler ≤ 4.0.21, and 4.1 is still a beta. Once a 4.1 leg is added, the issue's zz_parent_unwind scenario belongs in e2e_redirect_gem_build.

Note

Medium Risk
Changes transitive-vs-direct classification during gem restore; wrong parsing could drop real declarations or leave stray pins, but behavior is gated by existing provably_appended checks and only extends dependency detection.

Overview
Fixes hosted gem rollback/remove (#1989) when a patched gem is wired only through Bundler 4.1’s override "…", with: "<gem>", in: "<parent>". Those locks keep the parent’s original spec edge and record the swap in OVERRIDES, so the gem never appears on a 6-space sub-line. Restore used to treat it as direct and inject gem "<name>", "<version>", blocking updates past the pinned version.

is_subdependency in gem.rs now also treats the with: gem name from lock OVERRIDES entries (via new override_targets parser) as “another spec depends on this,” alongside existing sub-line checks. That lets restore_one classify the hosted append as transitive again—drop the Socket block and DEPENDENCIES entry instead of adding an exact pin—so Gemfile / Gemfile.lock round-trip byte-for-byte like ordinary transitive redirects.

Docs and unit/async tests cover argument order, version: clauses with commas, with: nil, and full rewrite → restore for an override-only rails fixture.

Reviewed by Cursor Bugbot for commit 927fa4c. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
With Bundler 4.1, `override "other", with: "gem", in: "parent"`
records the replacement only in the lock's OVERRIDES section and keeps
parent's original edge in the specs. Hosted rollback and remove then
treated the gem as directly declared and wrote a new exact
`gem "gem", "x.y.z"` pin into the Gemfile, which blocked the project
from updating past the vulnerable version.

The restore now counts an OVERRIDES `with:` target as a dependency of
another spec, so it drops the appended Socket block and its
DEPENDENCIES entry and gives back the original pair byte for byte.

Fixes #1989

Assisted-by: Claude Code:claude-opus-5-5
Cover the two other OVERRIDES shapes Bundler 4.1 writes: a replacement
with a version requirement (which holds a comma) and `with: nil`.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) force-pushed the agent/fix-gem-override-transitive-restore branch from 630dd19 to 26062a3 Compare November 2, 2026 15:38
Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review November 2, 2026 15:48
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 927fa4c. Configure here.

@Wenxin-Jiang Wenxin Jiang (Wenxin-Jiang) left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[agent] Reviewed 927fa4ccdf118bf8a01e1164112f8de68d318980. Checked Bundler OVERRIDES target recognition and restore ownership; the exact-head override_target round-trip test passed locally (1 test). No major correctness issues found. Validation: The focused core override_target round-trip test passed locally (1 test). Source and regression-test review plus available exact-head CI checks; broader local suites were not run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 2, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review.

  • Head: 927fa4ccdf118bf8a01e1164112f8de68d318980, mergeable, no conflicts with main.
  • CI: all checks on the head passed or were skipped (merge state clean).
  • Bugbot: reviewed this head, no findings. No open review threads.
  • Already approved on this head; needs a maintainer to merge.

Generated by Claude Code

@mikolalysenko
Mikola Lysenko (mikolalysenko) added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit 318193c Nov 2, 2026
82 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-gem-override-transitive-restore branch November 2, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

3 participants