Fix gem revert of a commented Gemfile line (#988)

[Mikola Lysenko (mikolalysenko)]

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

Fixes #988

Summary

Vendored gem vendor --revert (and remove / rollback) no longer
half-reverts when the user has commented the wired Gemfile line. Before,
the lock went back to rubygems.org while the Gemfile kept its
path: ".socket/vendor/gem/<uuid>/…" line, a pair bundle install --frozen refuses, and every later revert drift-kept again.

Root cause

Vendor's in-sync check treats the Gemfile as wired when it contains the
copy path, but revert_gemfile_record only accepted the recorded line
byte for byte. Records were also reverted and written one at a time
(lock first), so a drifted Gemfile record did not stop the lock from
being restored.

Fix (crates/socket-patch-core/src/vendor/gem.rs)

• Find the wired line by its code, not its comment. A line whose
  code equals what vendor wrote matches whatever its trailing comment
  says. The original line is put back, carrying the user's comment when
  they added or changed one. CRLF endings follow the original line.
  Lines whose code changed (for example , require: false added) are
  not rewritten, so a user edit is never silently dropped.
• All-or-nothing across Gemfile and Gemfile.lock. Records are planned
  against in-memory text (RevertFiles). When any known record drifts,
  nothing is written and the artifact is kept. If the Gemfile write fails
  after the lock was written, the lock's original bytes are put back.
• Convergence follows vendor's predicate. When the wired line is gone,
  a Gemfile that still contains the copy path, or that points the gem at
  another path:/source, counts as drift. A Gemfile that declares the
  gem from the registry again (a hand restore, even with a different
  constraint) has converged, so the lock follows it back. An edited
  managed block that still names the copy is now drift instead of "already
  removed". A lock with no CHECKSUMS section at all now counts as
  converged for the checksum record. Without that, the all-or-nothing rule
  would wedge that case forever.
• The drift warning now says that neither file was changed.
• formats::gem::gemfile::code_of is now pub(crate) (a quote-aware
  comment splitter).

Existing tests whose expectations changed

They pinned the old per-record partial revert, which is the behavior
#988 reports:

• test_checksums_revert_drift_warning and
  revert_missing_dep_pin_leaves_both_files_alone (renamed): a drifted
  lock record now leaves both files exactly as found.
• revert_checksums_section_gone_is_converged (renamed): no CHECKSUMS
  section counts as converged, so the artifact is removed.
• Two unit tests now build a RevertFiles instead of passing a path.

Test evidence

New tests failed on main before the fix (red run:
cargo test -p socket-patch-core --lib vendor::gem::tests::revert_ →
4 failed: revert_commented_gemfile_line_restores_both_files,
revert_edited_carried_comment_keeps_the_users_comment,
revert_commented_gemfile_line_keeps_crlf,
revert_drifted_gemfile_line_writes_neither_file). All pass after it.

Per-issue checklist (#988 acceptance criteria):

• Vendor, add a comment to the wired line, revert → both files back in
  registry form, comment kept, artifact removed:
  revert_commented_gemfile_line_restores_both_files (plus
  …edited_carried_comment… and …keeps_crlf).
• Truly drifted Gemfile line → Gemfile and lock unchanged, artifact
  kept, and the same on a second revert:
  revert_drifted_gemfile_line_writes_neither_file,
  revert_still_wired_but_edited_gemfile_writes_neither_file.
• test_revert_round_trip_*,
  test_revert_converged_files_are_silent_and_still_remove and the
  legacy-ledger fixtures stay green.

Commands run locally:

• cargo fmt --all -- --check: ok
• cargo clippy --workspace --all-features -- -D warnings: ok
• cargo test -p socket-patch-core --all-features: 6324 passed. The 4
  failures are chmod-based write-failure tests in copy_tree, vlt_heal,
  pypi_poetry and pypi_requirements, which can't trigger as root in this
  sandbox. None is in gem code, and CI runs them as non-root.
• cargo test -p socket-patch-cli --all-features --test e2e_vendor_gem_build --test e2e_redirect_gem_build --test in_process_vendor -- --include-ignored
  (ruby 3.3.6, bundler 4.0.18): 41 + 49 + 132 passed.
• The full CLI workspace build ran out of sandbox disk, so CI covers the
  remaining suites.

/code-review high: findings not fixed

• An unknown wiring kind still lets the known records restore. This keeps
  the forward compatibility that
  revert_unrecognized_wiring_kind_warns_and_continues pins, and the
  artifact is still kept.
• A wired line moved into a group block or split across lines is drift
  (nothing written) rather than rewritten. Revert won't rewrite indented
  or multi-line declarations, the same rule the forward path refuses on.

Follow-ups (not in this PR)

• The forward vendor rewrite writes the wired line with a bare LF in a
  CRLF Gemfile (seen while writing the CRLF test). This is part of the
  line-ending tracking in Tracking: classify line endings in one place instead of five drifting rules #814.
• requirements.txt has the same forward/revert recognizer split (Vendored requirements.txt can't be reverted once the user touches the # socket-patch vendor: comment: vendor --revert drift-keeps forever (exit 0), and the suggested "re-vendor" remedy is a no-op or adds a duplicate line #977).

🤖 Generated with Claude Code

https://claude.ai/code/session_01QFoDP1VMMeczUdpPaToVKx

---

Note

Medium Risk
Changes gem vendor revert semantics for Gemfile/Gemfile.lock pairing; behavior is narrower (no partial writes) but affects frozen bundle install if revert logic is wrong.

Overview
Fixes #988: vendored gem revert no longer leaves Gemfile and Gemfile.lock in a mismatched state (e.g. registry lock with a path: line still in the Gemfile).

Gemfile matching now treats a wired line as the same when only the trailing # comment differs from what vendor recorded, using quote-aware code_of (now pub(crate)). Revert restores the original declaration while keeping the user’s comment (and CRLF where applicable). Lines whose code changed (extra options, reformats) are not rewritten and count as drift.

All-or-nothing revert: wiring records are applied against in-memory RevertFiles; disk writes happen only if no record drifted. Any drift leaves both files untouched, updates warnings to say neither file changed, and keeps the artifact. Failed writes roll back an already-written lock. Convergence rules are tightened so a hand-restored registry gem can still revert the lock, a missing CHECKSUMS section is converged (not drift), and edited managed blocks that still point at the copy are drift.

Tests add #988 scenarios and update expectations that previously allowed partial lock-only reverts.

^{Reviewed by Cursor Bugbot for commit f2a11a3. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ 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 f2a11a3. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review (burn-down agent)

• Head: f2a11a3
• CI: 36/36 non-skipped checks green (50 skipped by path filters); mergeable, no conflicts.
• Bugbot: reviewed f2a11a3, no findings (the neutral Cursor Bugbot check is a cancelled duplicate request); no open review threads.
• Reviewer focus: vendor/gem.rs now reverts a vendored gem's Gemfile line and its coupled lock records together, and matches commented Gemfile lines instead of requiring exact text (Vendored gem revert restores Gemfile.lock but leaves the Gemfile path: line when that line carries a trailing comment, and no re-run can finish it #988).

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief (final reviewer agent)

What it does. vendor --revert for a gem used to match the wired Gemfile line byte for byte. If a user added a trailing # comment, the lock was reverted but the Gemfile kept its path: line (#988). Revert now finds the line by its code, ignoring the comment, and restores the original while keeping the user's comment and any CR. It also plans every lock and Gemfile record in memory first and writes nothing if any record drifted. If the Gemfile write fails after the lock was written, the lock's original bytes are put back.

Risk: medium. The fix is narrow and well tested, but it changes what revert, remove and rollback write for every gem entry. It also reclassifies three cases, all disclosed in the PR body:

• a missing CHECKSUMS section now counts as converged;
• a Gemfile the user put back on the registry by hand now counts as converged;
• an edited managed block now counts as drift.

Look here

• gem.rs:1157: the all-or-nothing gate. Nothing is written if any record drifts or errors mid-loop.
• gem.rs:2123 / 2174: RevertFiles and write_changed, which writes the lock first and rolls it back if the Gemfile write fails.
• gem.rs:2268: find_wired_gemfile_line. It tries an exact match first, then a code-only match, and accepts only a single hit.
• gem.rs:2288: gemfile_left_without_our_line, the new convergence rule.
• gem.rs:2312: restore_gemfile_line, which handles comment carry-over and CRLF.

Verified

• Read the full diff (vendor/gem.rs +556/-91; the only other edit makes gemfile::code_of pub(crate)).
• cargo test -p socket-patch-core --lib gem: 351 passed.
• Mutation check: with find_wired_gemfile_line made exact-match only, 3 of the new tests fail, so they exercise the fix.
• Clippy -D warnings on the core lib is clean. Its --all-targets errors are all in files this PR doesn't touch.
• CHANGELOG.md is untouched, there is no debug leftover, and there are no path-safety concerns (fixed file names joined to the root, uuid validation unchanged).
• CI: 35/35 non-skipped checks green (50 path-skipped), including ci-ok and clippy. Bugbot reviewed f2a11a3 with no findings; the neutral check is a cancelled duplicate request. There are no open threads.

Changes I made: none.

Open questions (non-blocking, could be follow-ups)

• Hand-restore case: if the user hand-edits the Gemfile to gem "rack", "~> 3.2" and then reverts, revert now counts it as converged with no warning. The lock goes back to ~> 3.1, so bundle install --frozen still fails. Before this PR the same bytes came with a drift warning. Worth an info warning?
• Ignored rollback failure: write_changed discards the result of its lock rollback (let _ =, around L2188). If the Gemfile write and the rollback both fail, the error doesn't say the lock is left reverted. That path has no test either.
• Commented-out wired line: if the user comments out the wired line (# gem "rack", path: ".socket/vendor/..."), revert reports drift on every run until the comment is deleted. That matches vendor's own wired check, but the warning doesn't say what to remove.

Auto-merge is armed, so approving sends this straight to the merge queue.

---

Generated by Claude Code