Skip to content

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

Open
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
agent/fix-gem-line-semicolon-statement
Open

Mikola Lysenko (mikolalysenko) wants to merge 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

This branch has not been deployed

No deployments
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