Skip to content

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

Merged
Mikola Lysenko (mikolalysenko) merged 5 commits into
mainfrom
agent/fix-gem-line-semicolon-statement
Oct 7, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 5 commits into
mainfrom
agent/fix-gem-line-semicolon-statement

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

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

Fixes #826

Summary

A Gemfile line holding two ;-joined declarations (gem "colorize", "0.8.1"; gem "rainbow", "3.1.1") lost its second gem when socket-patch redirected (hosted) or vendored the first one. Hosted scan exited 0, and the next frozen bundle install failed. Such lines are now refused with the existing redirect_gem_unrecognized_declaration warning (hosted) or a not editable refusal (vendored), and nothing is written.

The PR also fixes the opposite regression from #637. A complete declaration ending in a bare ; (gem "colorize", "0.8.1";, optionally followed by # comment) was refused as "continues on the next line". It is rewritten again, and the ; is dropped from the kept options.

Root cause

Both Gemfile rewriters replace the declaration's whole physical line, and both gate that on the shared gem_line_tail_blocks_edit (crates/socket-patch-core/src/patch/redirect/mod.rs; vendored calls it through rest_blocks_edit). The gate had no notion of a top-level ; statement terminator:

  • When something followed the ;, it looked like ordinary code, so the gate accepted the tail and the line rewrite deleted the second statement.
  • When nothing followed it, the last-character check read ; as a dangling continuation.

Fix

  • gem_line_tail_blocks_edit: a ; outside strings and brackets ends the statement. If anything other than more ;s, whitespace or a # comment follows, it refuses with "another statement follows the declaration on its line". Otherwise scanning stops there, so the end-of-code checks see the declaration without its terminator.
  • formats::gem::gemfile (the shared option reader both rewriters use since Fix gem source-option guard missing git sources (#652) #731): trailing_options and source_option first drop that terminator via the new without_statement_end, keeping a trailing comment and any ; inside strings or the comment. Without that, "1.0"; would read as a positional argument and be kept, and a bare ; # c tail would fail closed as an unreadable, source-selecting option.

There is one fix point for both modes. The npm/pypi/gem wrappers have no Gemfile logic and need no change.

Ported from #878 (commit 5b9953d): main is red on utils::digest::tests::production_digests_go_through_the_helpers, because #646 added inline digest calls that #865's guard rejects. This port routes them through utils::digest and becomes a no-op once #878 lands.

Test evidence

Issue case Test Without fix With fix
#826 ;-joined second declaration deleted (hosted) patch::redirect::tests::gemfile_semicolon_joined_declarations_fail_closed FAILED (Gemfile rewritten, rainbow gone) ok
same, shared gate patch::redirect::tests::gem_line_tail_semicolon_statements FAILED ok
same (vendored) vendor::gem::tests::plan_gemfile_edit_semicolon_statements FAILED ok
#826 bare trailing ; refused (hosted) patch::redirect::tests::gemfile_trailing_semicolon_declaration_rewrites FAILED ok
option reader drops the terminator formats::gem::gemfile::tests::trailing_options_drop_the_statement_terminator new ok
real bundler: ;-joined refused, project still installs e2e_redirect_gem_build::gem_hosted_semicolon_joined_declarations_are_refused_and_still_install FAILED ok
real bundler: trailing ; redirects, fresh checkout installs patched bytes e2e_redirect_gem_build::gem_hosted_trailing_semicolon_declaration_redirects_and_installs FAILED ok

Commands run locally on the merge with main 9c43dfc (Linux, Ruby 3.3.6, Bundler 4.0.17, toolchain 1.93.1):

  • cargo test -p socket-patch-core --all-features --lib: 5251 passed. The 4 failures (relax_loop_must_not_traverse_symlinked_root, an_unremovable_hidden_lock_keeps_every_store_entry, wire_write_failure_maps_error_and_leaves_lock_untouched, wire_failure_rolls_back_already_written_files) all rely on permission bits, which have no effect when the container runs as root.
  • SOCKET_PATCH_BUNDLER_E2E_REQUIRED=1 cargo test -p socket-patch-cli --all-features --test e2e_redirect_gem_build --test e2e_vendor_gem_build -- --ignored: 18 + 8 passed.
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo fmt --all -- --check: clean for every hunk in this PR. main itself has pre-existing rustfmt diffs elsewhere, which CI doesn't check, and this PR leaves them alone.

CI on 5b9953d: all 12 workflows are green. The macOS/ubuntu/Windows jobs that were cancelled without ever getting a runner, and the one Bun Windows workspace-nested vendored cell, passed on a single re-run. Bugbot is clean at 5b9953d. On 2026-10-06 one Bun native (macos-latest, 0.8.1) job had sat queued with no runner since the previous re-run. A second re-run of that workflow passed, and every check on 5b9953d is now green.

Follow-ups

None for #826. Parenthesized calls ending in ); (gem("x", "1");) are still refused. That fails closed with a warning, and nobody has reported it.

🤖 Generated with Claude Code

https://claude.ai/code/session_017ZTJ2BGpGJYPCaQvncWGPm

Assisted-by: Claude Code:claude-opus-5-5
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
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
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 5, 2026 18:04
@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.

Stale Bugbot comment from a previous run.

main moved the Gemfile option reader into formats::gem::gemfile, so the
`;` terminator handling moves with it: trailing_options and the new
source_option both drop a bare statement terminator. Otherwise a
complete `gem "x", "1";` line was refused as an unreadable,
source-selecting tail.

Assisted-by: Claude Code:claude-opus-5-5
@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.

Stale Bugbot comment from a previous run.

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
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] coverage failed on 52c6875, and the cause is not this PR. It is utils::digest::tests::production_digests_go_through_the_helpers, which is red on main too:

I ported the fix from #878 here in 5b9953d. It becomes a no-op once #878 lands. With it, cargo test -p socket-patch-core --lib passes locally, except for 4 permission-bit tests that only fail because this container runs as root.


Generated by Claude Code

@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 5b9953d. Configure here.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] CI status on 5b9953d. None of these look like this PR's failures:

  • Go go (macos-latest, 1.26.3) and Poetry native (macos-latest, 1.2.2 / 1.3.2): these cells were cancelled before any step ran because no macOS runner picked them up. Every cell that did run passed, and the same cells passed on 52c6875. I re-ran them once.
  • Bun native (windows-latest, 1.3.9): 52 of 53 cells passed. The one failure is workspace-nested vendored, a refusalCodesExact mismatch. This PR doesn't touch Bun code. That cell passed on 52c6875 and on the latest two main runs, and workspace-nested hosted passed in the same job. GitHub refused a re-run (403) while the Bun run is still in progress, so I'll re-run it once the run finishes. If it fails again, I'll pull the refusal code from the bun-results-windows-latest-1.3.9 artifact and treat it as real.

Generated by Claude Code

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

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at 5b9953d (5b9953d336d5e7ff2dd9ba5f0813ab5952f7bf67).

  • CI: 537/537 checks green on the head commit (6 skipped by matrix rule). One check, native (macos-latest, 0.8.1), still reads queued. It's a duplicate job record (111996281266) that GitHub left behind during today's runner outage. The same job in that attempt (111996281209) passed, and its workflow run (Bun patch compatibility, 37360214294) concluded success.
  • Bugbot: reviewed 5b9953d with no new issues, and no review threads are open.
  • Mergeable against main, with no conflicts.

Generated by Claude Code

@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit d201fce into main Oct 7, 2026
1229 of 1278 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-gem-line-semicolon-statement branch October 7, 2026 12:02
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