Repository navigation
Fix Bundler path tier read two ways (#1931) - #1943
Conversation
Assisted-by: Claude Code:claude-opus-5-5
A project whose Bundler config sets disable_shared_gems to a false
value ("false", "no", "0" or empty, in .bundle/config, the
environment or the global config) installs and loads its gems from
the system gem homes, just like path.system true. Store discovery
only looked at path.system, so it still crawled a leftover
vendor/bundle, dropped the gem env homes, and apply patched a copy
Bundler never loads.
The tier rule ("the first tier that sets path, path.system or
disable_shared_gems decides") now has one resolver,
BundlerPathTiers::deciding, read by store discovery, the global-path
shadowing check and bundler_sets_explicit_path. The second struct
(BundlerPathTier), config_path_tier, app_config_sets_path and the
env pre-filter on the global config are gone.
Fixes #1931
Assisted-by: Claude Code:claude-opus-5-5
A Bundler config that records a path and, in the same file, sets disable_shared_gems to a false value sends Bundler to the system gem homes, so the recorded path (local or global) is not where it loads gems. Discovery still crawled it and apply could patch that unused copy; it now drops the path the same way path.system true does. bundler_sets_explicit_path also picks its deciding tier through BundlerPathTiers::deciding_of, the same resolver discovery uses, and the discovery comments now name both system-gem flags. Refs #1931 Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ 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 fe20764. Configure here.
|
[agent]
All other checks are green, and Bugbot's review of Generated by Claude Code |
|
[agent] Update: Generated by Claude Code |
|
[burn-down agent] Ready for review at
Generated by Claude Code |
Final review briefWhat it does: Fixes #1931. Store discovery had its own copy of Bundler's Risk: low. The change is one file of crawler logic. I checked it line by line against Bundler 4.0.18's
Look here:
Verified:
Changes I made: None. Open questions: None that block merging. Small side effects the description doesn't mention, all consistent with Bundler:
The env- Auto-merge is armed: approving sends this straight to the merge queue. Generated by Claude Code |
Wenxin Jiang (Wenxin-Jiang)
left a comment
There was a problem hiding this comment.
[agent] Reviewed fe207642712edbc2c7b0c810a718f22146afaba3. Checked Bundler local/env/global path tier precedence and system/shared-gem flags. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.
LLM Description written by Claude Code:claude-opus-5-5
Fixes #1931
What changes for users
In a Ruby project whose Bundler config sets
disable_shared_gemsto a false value ("false","no","0"or empty) in.bundle/config, the environment or the global config, Bundler installs and loads gems from the system gem homes, exactly as it does forpath.system: true. socket-patch now treats it the same way:vendor/bundleis no longer crawled and no longer switches off thegem envhomes, so apply patches the copy Bundler actually loads.Before this change, apply could report success after patching a copy Bundler never loads.
Root cause
Bundler's
Settings#pathtier rule (the first of local, env and global that setspath,path.systemordisable_shared_gemsdecides, and a truthypath.systemor a falsydisable_shared_gemsmeans system gems) was written twice incrawlers/ruby_crawler.rs.bundler_sets_explicit_pathfollowed Bundler. Store discovery's copy (bundler_path_tier+config_path_tier) readsystemfrompath.systemonly and never saw the envBUNDLE_DISABLE_SHARED_GEMS.The fix
BundlerPathSettings::decides_path/uses_system_gemsandBundlerPathTiers::decidingform the one tier resolver. Store discovery, the global-path shadowing check andbundler_sets_explicit_pathall read through it.Removed:
BundlerPathTier,config_path_tier,bundler_path_tier,app_config_sets_path,global_path_config_unless_env_path_settingsandambient_path_system_env. The env tier's flags reach discovery asBundlerEnvPathFlagsinstead of pre-filtering the global config file.parse_bundle_config_pathdrops a recorded path when the same file sets a falsydisable_shared_gems, as it already did forpath.system: true.Side effects, both consistent with Bundler: an env
BUNDLE_PATH__SYSTEMorBUNDLE_DISABLE_SHARED_GEMSno longer hides the whole global config file from discovery (only itspathis shadowed), so a globalBUNDLE_GEMFILEis still honored; andverification_only_gem_pathsnow passes the ambient env flags into discovery.Tests (red → green)
disable_shared_gemsfalsy + leftovervendor/bundlefalsy_disable_shared_gems_drops_leftover_vendor_bundlelocal false: [".../vendor/bundle/ruby/3.3.0/gems"])BUNDLE_DISABLE_SHARED_GEMS=falseenv_falsy_disable_shared_gems_drops_leftover_vendor_bundledisable_shared_gemsarm)falsy_disable_shared_gems_drops_same_tier_pathlocal: [".../vendor/x/ruby/3.3.0/gems"])truthy_or_shadowed_disable_shared_gems_keeps_vendor_bundleenv_path_flags_shadow_the_global_config_pathThe mutation check (dropping the
disable_shared_gemsarm fromuses_system_gems) also fails the existingbundler_sets_explicit_path_follows_settings_tiers, which shows discovery and the stale-install/vex guard now share one rule.Local runs:
cargo fmt --all -- --check: okcargo clippy --workspace --all-features -- -D warnings: okcargo test --workspace --all-features --no-fail-fast: 236 targets pass. Four targets fail only in this sandbox and none touch Ruby code:socket-patch-core --lib(4 tests),in_process_redirect(3),repair(2),e2e_bun_lockb(2). The first three inject write failures with read-only dirs, which the container's root user ignores.e2e_bun_lockbneeds the live patch API (see Fix bun.lockb nightly e2e reaching the live blob API #1905) and depends on the local bun 1.4.4 build. Draft CI onba9a49bwas green.crawler_ruby_e2e: 30 pass.e2e_redirect_gem_stale_install: 42 pass.e2e_redirect_gem_build -- --ignored(Bundler 4.0.18): 37 pass.e2e_vendor_gem_build -- --ignored: 11 pass.Code review findings not fixed here
/code-review highran on the diff. The real findings are fixed infe20764(same-tier path, single resolver call, stale comments). Three were left as they are:BUNDLE_PATHroot when a local tier decides without a path. Bundler ignores the env path there, but discovery probes both roots on purpose:bundle_roots_probe_in_bundler_precedence_orderandrefused_config_root_is_verification_onlyassert it. Narrowing that is a design change, so it's left for a maintainer to decide.formats/gem/settings.rswith one snapshot) is where it belongs.BundlerEnvPathFlagsbesideBundlerPathSettings. It keeps the envBUNDLE_PATHas a rawOsStrpath (non-UTF-8 safe) with a single source. Move Bundler settings parsing out of ruby_crawler into formats/gem/settings.rs, and read .bundle/config keys with one parser (two drifted today) #1935'sBundlerEnv::ambient()snapshot can fold it in.Follow-ups
formats/gem/settings.rs.Generated by Claude Code