Skip to content

Fix .bundle/config values keeping a trailing # comment (#951) - #953

Open
Mikola Lysenko (mikolalysenko) wants to merge 4 commits into
mainfrom
agent/fix-bundle-config-trailing-comment
Open

Mikola Lysenko (mikolalysenko) wants to merge 4 commits into
mainfrom
agent/fix-bundle-config-trailing-comment

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #951

Root cause

Every .bundle/config reader goes through unquote_bundle_config_value in crawlers/ruby_crawler.rs. That covers parse_bundle_config_path (the app and global path / path.system), bundle_config_setting* (cache_path, the tier-presence check) and formats::gem::manifest::config_gemfile. The helper trims and unquotes the value, but it never applies Bundler's YAMLSerializer#strip_comment, which cuts the value at its first # unless the value starts with #. So BUNDLE_PATH: .gems # note resolved to a directory literally named .gems # note. Agent apply then patched the gem env copy and vex attested not_affected, while Bundler loaded the unpatched .gems copy. A commented BUNDLE_CACHE_PATH likewise hid stale archives from the hosted stale-install guard.

Fix

  • unquote_bundle_config_value now matches Bundler's config loader (Gem::YAMLSerializer, which Bundler 2.4+ uses when present, and Bundler's own copy from 2.5.6). It trims and unwraps one matching quote pair that closes the line, then strips the comment. This was checked against the real loader: "a#b" → a, x#y → x, #z stays whole, "vendor/bundle" # c keeps its quotes.
  • Era handling. strip_comment first shipped in RubyGems/Bundler 3.5.6/2.5.6 (I checked the published gems: 3.5.5 doesn't have it, 3.5.6 does). Bundler < 2.4, or 2.4–2.5.5 on RubyGems < 3.5.6, keeps the comment and installs into .gems # note, which the old code happened to handle correctly. The installed Bundler's era isn't known to the crawler, so for the two directory settings (path from the app and global config, and cache_path), bundle_config_dir_reading uses the current reading unless only the legacy reading's directory exists. Bundler creates the directory it uses, so this follows whichever copy is actually installed. A value without a comment reads the same in both eras. An unset current reading, such as a commented path.system: true, always stands; a leftover directory at the recorded path doesn't bring that path back (Bugbot finding).
  • path.system and gemfile use the current reading only, because a boolean or a file name has no directory to tell the eras apart. On a legacy Bundler a commented gemfile still fails closed (redirect_gem_bundle_gemfile_unsupported).

Tests (red on main, green here)

Issue scenario Test
#951 value parsing (pinned against Bundler's own loader output) ruby_crawler::tests::bundle_config_values_drop_a_trailing_comment_like_bundler
#951 commented path, path.system, cache_path, gemfile ruby_crawler::tests::commented_bundle_config_settings_follow_bundler
#951 repro: .gems # comment store discovery ruby_crawler::tests::commented_app_config_bundle_path_discovers_the_bundler_store
#951 repro end to end, real bundle install + agent apply patches the copy bundle exec loads in_process_alternate_installers::bundler_commented_config_path_apply_patches_loaded_gem
#951 follow-up: hosted stale-install guard with commented BUNDLE_CACHE_PATH e2e_redirect_gem_stale_install::gem_hosted_stale_archive_at_configured_cache_path_warns_and_is_not_attested (new app-config-commented row)
Bugbot: commented path.system: true + leftover recorded dir stays on system gems ruby_crawler::tests::commented_path_system_true_ignores_a_leftover_recorded_path
No regression for legacy-era Bundler ruby_crawler::tests::commented_bundle_path_keeps_the_legacy_bundler_store (passes before and after)

Every test above except the legacy guard fails with the main crawler (verified locally by swapping ruby_crawler.rs back to main) and passes with the fix. The Bugbot test also fails against the first version of the era fallback. The legacy guard passes on both, by design. The e2e ran against Bundler 4.0.18 / RubyGems 3.5.22 (Ruby 3.3.6), and bundle exec confirmed the patched file is the one Bundler loads.

Local checks

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test --workspace --all-features --no-fail-fast: 226 suites pass. 12 tests in 4 targets fail locally only because this sandbox runs as root, which ignores the read-only chmod those tests depend on (covgap_commands_vendor ×3, in_process_redirect ×3, repair ×2, and core lib ×4: copy_tree::relax_loop…, vlt_heal::an_unremovable…, pypi_poetry::wire_write_failure…, pypi_requirements::wire_failure_rolls_back…). They are all permission-mode tests, none touch gem code, and CI runs them as a normal user.
  • Real-Bundler suites: e2e_redirect_gem_build -- --ignored (16 passed) and e2e_vendor_gem_build -- --ignored (8 passed); in_process_alternate_installers bundler legs pass.
  • Also ported Route Gradle digests through utils::digest #878 (3f8e1c1, cherry-picked with -x) to fix the production_digests_go_through_the_helpers failure that is already red on main. It becomes a no-op once Route Gradle digests through utils::digest #878 lands.

CI

All 412 check runs on 52542db are green (406 success, 6 skipped), and the merge state is clean. Two jobs needed one re-run each after hitting their timeout-minutes with no failing test: coverage passed in 14 minutes on re-run, and gradle 8.14.3 / jdk 21 / hosted / macos-latest passed on re-run. Both had passed on 3f8e1c1, which runs the same tests. Bugbot reviewed 52542db and found no new issues, and its one finding on 3f8e1c1 is fixed and resolved.

Notes: the npm, PyPI and gem wrappers only dispatch to the binary, so none of them needs a change. cargo fmt --all -- --check reports ~466 pre-existing diffs on main itself (CI has no fmt gate). The three files this PR touches are rustfmt-clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CcuTzWw24ikXbTnMaqW4JJ


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Bundler cuts a .bundle/config value at its first `#` (RubyGems and
Bundler 2.5.6+), but socket-patch kept the comment as part of the
value. A hand-commented `BUNDLE_PATH: .gems # note` therefore made
agent apply patch the system gem copy, and vex attest not_affected,
while Bundler loaded the unpatched .gems copy. A commented
BUNDLE_CACHE_PATH also hid stale archives from the hosted
stale-install guard, and commented path.system / gemfile values were
misread too.

The shared config value reader now strips the comment the way
Bundler's config loader does. Older Bundler keeps the comment in the
value, so for directory settings (path, cache_path) the legacy
reading is used when only its directory exists on disk.

Fixes #951

Assisted-by: Claude Code:claude-opus-5-5
main has failed socket-patch-core's lib tests since Gradle support
(#646) and the digest helpers (#865) both landed. The guard test
production_digests_go_through_the_helpers flags three files #646 added
that still hash inline: crawlers/gradle_cache.rs, patch/jvm_jar.rs and
patch/sidecars/maven.rs. That breaks test, test-release and coverage on
every open PR.

Each inline sha1/sha256 call now goes through sha1_hex_of or
sha256_hex_of, which compute the same lowercase hex. Behaviour is
unchanged.

Assisted-by: Claude Code:claude-opus-5-5
(cherry picked from commit 659ac2c)
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] On the start commit 7333034, which is identical to main, coverage and test (macos-latest) failed on utils::digest::tests::production_digests_go_through_the_helpers. That failure comes from main, not this PR: Gradle files from #646 still hash inline, and the guard test from #865 flags them. Its fix is open as #878, so I've ported that commit into this branch as 3f8e1c1 (a cherry-pick with -x). It becomes a no-op once #878 lands.


Generated by Claude Code

@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 6, 2026 18:18
@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.

Comment thread crates/socket-patch-core/src/crawlers/ruby_crawler.rs
A `BUNDLE_PATH__SYSTEM: true # note` line makes Bundler 2.5.6+ ignore
the recorded BUNDLE_PATH and load system gems. The era fallback added
for #951 could still pick the recorded path when a leftover directory
existed there, so apply would patch a copy Bundler never loads.

The legacy reading now only competes with another directory reading;
an unset current reading always stands.

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.

✅ 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 52542db. Configure here.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] On 52542db, coverage was cancelled at its 35-minute timeout-minutes, and the job left no logs. The same job passed in 15 minutes on 3f8e1c1, which has the same tests apart from a one-function change to bundle_config_dir_reading plus a pure unit test. Coverage also passed in 13–17 minutes on every other recent PR. I've re-run it once. If it times out again I'll treat it as real and dig in.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] The coverage re-run passed in 14 minutes. On 52542db, gradle 8.14.3 / jdk 21 / hosted / macos-latest was then cancelled at the workflow's 60-minute timeout-minutes. The same job passed in 27 minutes on 3f8e1c1 and in 22–28 minutes on the ten most recent PRs. This PR doesn't touch Gradle code: the only change since 3f8e1c1 is in the gem config reader. I've re-run it once. If it times out again I'll 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 6, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Ready for review at 52542db.

  • CI: every check suite on 52542db is green (the coverage and gradle 8.14.3 / jdk 21 / hosted / macos-latest timeouts were re-run and passed; this PR doesn't touch Gradle).
  • Bugbot: reviewed 52542db, no findings. No open review threads.
  • Reviewer note: the fix applies Bundler's strip_comment rule in unquote_bundle_config_value, so .bundle/config values like "vendor/bundle" # local resolve to vendor/bundle. The earlier approval was on 3f8e1c1, so this needs a re-approval.

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