Skip to content

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

Description

[agent] Filed by the scheduled architecture audit routine (ecosystems and formats). Register: register comment.

Kind: bug. Source: new finding, register E68 (same class as E24, the nine revert mechanisms).

Problem

Vendored gem decides whether the Gemfile wiring is "ours" in two different ways:

  • Forward (vendor, hot path): the Gemfile only has to contain the copy path: gemfile_text.contains(&copy_rel) (gem.rs#L361-L362).
  • Revert (revert_gemfile_record): the recorded line must match a Gemfile line exactly: lines.iter().position(|l| *l == written) (gem.rs#L2226-L2242).

The records are also reverted one by one, lock first (gem.rs#L1194-L1240). A drifted Gemfile record doesn't stop the lock records from being written. Poetry's legacy formats have the atomic variant revert_lock_fragment_splice_atomic (common.rs#L714-L723) for exactly this, and #822 fixed the same half-revert in uv.

Proof: I ran a throwaway test in vendor::gem::tests three times on 9c43dfc, using the existing fixture(GEMFILE_DIRECT, LOCK_DIRECT) and run_vendor:

  1. Vendor rack 3.2.6. The Gemfile line becomes gem "rack", "3.2.6", path: ".socket/vendor/gem/<uuid>/rack-3.2.6".
  2. Append # CVE fix, do not bump to that line.
  3. Re-run vendor: success=true, no files patched, no new entry (in sync).
  4. revert_gem(&entry): success=true, kept_artifact=true, with warnings vendor_lock_entry_drifted ("Gemfile no longer carries what vendor wrote for rack") and vendor_artifact_kept.
    • The Gemfile still says path: ".socket/vendor/gem/<uuid>/rack-3.2.6".
    • Gemfile.lock was restored to the registry: the PATH section is gone, rack (3.2.6) is back under GEM, and DEPENDENCIES says rack (~> 3.1).
  5. A second revert_gem: drift-keep again, the same two warnings. It can never finish.

Symptoms

Impact

  • A user who annotates their Gemfile line, or whose formatter rewrites it, ends up with a Gemfile that names a path: source while the lock says rubygems.org. bundle install --frozen / --deployment refuse that pair, because the Gemfile and lock disagree.
  • The revert reports success, and the warning's remedy ("undo the drift … or re-vendor") doesn't help: re-vendor thinks it is in sync until the lock is half-reverted, and after that the second revert still drift-keeps.
  • The size is small: one file plus tests.

Proposed change

  1. Make the Gemfile revert recognize our line by the same predicate the forward path uses: a gem "<name>" declaration whose path: names this uuid's copy. Then restore original over that line, keeping any trailing comment the user added. Delete the exact-line position(|l| *l == written) match.
  2. Make the gem revert all-or-nothing across its coupled records (gemfile_line, gemfile_lock_spec, gemfile_lock_checksum): plan every record first, and write nothing if any record drifted, as revert_lock_fragment_splice_atomic does.

Size and scope

Acceptance criteria

  • A regression test: vendor, then append a comment to the wired Gemfile line, then revert. Both files return to registry form, the comment is either kept on the restored line or documented as dropped, and the artifact is removed.
  • A regression test: a genuinely drifted Gemfile line (the path: was removed or points elsewhere) leaves both the Gemfile and Gemfile.lock byte-identical (no half-revert), and the artifact is kept.
  • The existing test_revert_round_trip_*, test_revert_converged_files_are_silent_and_still_remove and the legacy-ledger gem fixtures stay green.

Dependencies

None. This is independent of the E24 tracking issue, which it informs.


Backlog review — 2026-10-08

Priority: P1 → P2. A trailing comment creates a partial Gem rollback and install failure. Concrete defect, but conditional and observable.

Activity

  1. added
    bugSomething isn't working
    arch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)
    on Oct 7, 2026
  2. mikolalysenko commented on Oct 7, 2026

    @mikolalysenko
    CollaboratorAuthor

    [agent] Triaged as priority:p1 (Bundler). Confirmed on main (9c43dfc): the forward in-sync check is a substring test (gemfile_text.contains(&copy_rel), crates/socket-patch-core/src/vendor/gem.rs:362), but revert_gemfile_record only accepts an exact line match (lines.iter().position(|l| *l == written), gem.rs:2232). So a trailing comment on the wired line reads as "in sync" when vendoring and as "drifted" when reverting.

    Related to #977 (requirements.txt has the same forward/revert recognizer split), but the fixes live in separate code (vendor/gem.rs vs. the requirements line reverter), so I'm not clustering them. #989 tracks the shared engine that would fix both.


    Generated by Claude Code

  3. mikolalysenko commented on Oct 9, 2026

    @mikolalysenko
    CollaboratorAuthor

    [agent] Re-checked against main @ 03b9418 (architecture audit, ecosystems and formats). The code moved (#768, #1043), and the defect is unchanged.

    • Forward still treats the Gemfile as wired when it merely contains the copy path: gem.rs#L416-L419 (gemfile_text.contains(&copy_rel)).
    • Revert still needs the exact recorded line: revert_gemfile_record, gem.rs#L2273-L2300 (lines.iter().position(|l| *l == written)), returning Drifted otherwise.
    • revert_gem still reverts record by record, so a drifted Gemfile record does not stop the lock records.

    No open PR touches these lines. vendor/gem.rs is still listed as blocked by an open PR in the refactor routine's queue.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent:triagedarch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)bugSomething isn't workingpm:bundlerBundler (RubyGems)priority:p2

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions