fix(ruby): lock resolved install options#9992
Conversation
Greptile SummaryThis PR extends the Ruby lock identity to include resolved install options — compile mode, installer choice, build repositories, build flags, patch sources, and precompiled URL/platform selectors — so that changing any of these settings (including mise's own defaults) correctly invalidates the lockfile rather than silently reusing a stale entry.
Confidence Score: 5/5Safe to merge; the change is self-contained to Ruby lockfile option resolution with no effect on install paths or other backends. All code paths are covered by the new test suite, the Windows early-return is clean, and the logic correctly mirrors should_try_precompiled semantics. No data loss or behavioral regression risk — worst case is a one-time lock invalidation for existing Ruby lockfiles, which is the stated intent of the PR. No files require special attention. Important Files Changed
Reviews (12): Last reviewed commit: "Merge branch 'main' into fix/ruby-build-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ToolOptionSource::InstallManifest to track and apply tool options from installation manifests, updating BackendArg and configuration resolution logic accordingly. The Ruby plugin's lockfile options were refactored into ruby_lockfile_options to include a broader set of settings such as compilation flags, installer repositories, and precompiled binary metadata. Feedback suggests simplifying a match block in the Ruby plugin using a more idiomatic if let pattern to improve readability.
This comment was marked as outdated.
This comment was marked as outdated.
risu729
force-pushed
the
fix/ruby-build-lock-options
branch
from
556e50e to
7fb77b1
Compare
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughprecompiled_platform_for_target now honors Settings overrides; resolve_lockfile_options returns an empty map for windows or deterministically builds options from Settings (compile, installer, repo/opts, apply_patches, precompiled_url/arch/os); tests add deterministic Settings scaffolding and exact-map assertions. ChangesRuby Lockfile Options Resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
risu729
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/plugins/core/ruby.rs (1)
1175-1179: ⚡ Quick winAvoid poisoning the shared test mutex.
A single panic while holding
TEST_SETTINGS_LOCKwill poison the mutex and make later tests fail during setup instead of surfacing the original assertion failure.Suggested change
- let lock = TEST_SETTINGS_LOCK.lock().unwrap(); + let lock = TEST_SETTINGS_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/core/ruby.rs` around lines 1175 - 1179, The TEST_SETTINGS_LOCK acquisition can poison the mutex if a panic occurs while held; change the lock call to recover from poisoning by using TEST_SETTINGS_LOCK.lock().unwrap_or_else(|poison| poison.into_inner()) so the guard still obtains the inner mutex guard, then proceed to call SettingsPartial::empty, configure_settings, Settings::reset and create the SettingsResetGuard as before; this ensures a poisoned mutex won't make subsequent test setup fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/core/ruby.rs`:
- Around line 1305-1325: The tests in
test_ruby_lockfile_options_include_experimental_precompiled_default incorrectly
assume precompiled-by-default; update the test cases that call
resolve_ruby_lockfile_options so they mirror the runtime gating used by
should_try_precompiled() and install_version_: for the “experimental default”
variant explicitly set settings.experimental = true (so precompiled behavior is
enabled), and for the “resolved defaults” variant either set
settings.ruby.compile = Some(false) to opt into precompiled behavior or else
change the expected BTreeMap to the current (non-precompiled) defaults; locate
these changes around the test function
test_ruby_lockfile_options_include_experimental_precompiled_default and the
sibling test referenced at lines 1329-1346 and adjust expectations to match the
presence or absence of settings.experimental and settings.ruby.compile.
- Around line 1031-1056: When building the options map for non-current targets
(i.e. where is_current_platform is false) ensure you still insert the resolved
source-build defaults so lockfile identity is complete: always set "compile" to
(!try_precompiled).to_string(), set "ruby_install" to
ruby.ruby_install.to_string(), and populate the installer-specific keys by
reading ruby.ruby_install_opts/ruby.ruby_install_repo when ruby.ruby_install is
true or ruby.ruby_build_opts/ruby.ruby_build_repo when false; also insert
"apply_patches" from ruby.apply_patches when present. Update the code around
opts insertion (the block that currently runs only under if is_current_platform)
to perform these same inserts for non-current targets so the options-map
includes compile, installer choice, repo, build opts, and patches for both
current and non-current platforms.
---
Nitpick comments:
In `@src/plugins/core/ruby.rs`:
- Around line 1175-1179: The TEST_SETTINGS_LOCK acquisition can poison the mutex
if a panic occurs while held; change the lock call to recover from poisoning by
using TEST_SETTINGS_LOCK.lock().unwrap_or_else(|poison| poison.into_inner()) so
the guard still obtains the inner mutex guard, then proceed to call
SettingsPartial::empty, configure_settings, Settings::reset and create the
SettingsResetGuard as before; this ensures a poisoned mutex won't make
subsequent test setup fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2d20b8e-0413-45d2-88f4-54d5eff4391c
📒 Files selected for processing (1)
src/plugins/core/ruby.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/core/ruby.rs (1)
1019-1063:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip Ruby build/precompiled options for Windows targets.
Line 1074 already routes Windows through RubyInstaller2, so
compile, installer choice, repos, patches, and precompiled selectors here do not affect the Windows artifact. Persisting them in the options map will make Windows lock entries miss exact-match reuse when unrelated Ruby build settings change.Proposed fix
fn resolve_lockfile_options( &self, _request: &ToolRequest, - _target: &PlatformTarget, + target: &PlatformTarget, ) -> BTreeMap<String, String> { + if target.os_name() == "windows" { + return BTreeMap::new(); + } + let mut opts = BTreeMap::new(); let settings = Settings::get(); let ruby = &settings.ruby;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/core/ruby.rs` around lines 1019 - 1063, In resolve_lockfile_options, skip adding Ruby build/precompiled related keys when the target is Windows: detect Windows from the _target/PlatformTarget and if Windows, return opts without inserting "compile", "ruby_install", "ruby_install_opts", "ruby_install_repo", "ruby_build_opts", "ruby_build_repo", "apply_patches", "precompiled_url", "precompiled_arch", or "precompiled_os"; leave other non-build keys as-is. Use the existing _target/PlatformTarget check (e.g., is_windows or target.os == "windows") before using ruby, try_precompiled, and inserting into opts so Windows lock entries aren’t affected by unrelated build settings.
🧹 Nitpick comments (1)
src/plugins/core/ruby.rs (1)
1025-1028: ⚡ Quick winReuse
should_try_precompiled()here.This reimplements the same gating used by
install_version_()andresolve_lock_info(). With the planned 2026.8.0 default flip already called out inshould_try_precompiled(), this copy can drift and make lock options disagree with runtime resolution.Proposed refactor
let mut opts = BTreeMap::new(); let settings = Settings::get(); let ruby = &settings.ruby; - let try_precompiled = - ruby.compile == Some(false) || (settings.experimental && ruby.compile.is_none()); + let try_precompiled = self.should_try_precompiled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/core/ruby.rs` around lines 1025 - 1028, The code recomputes the precompiled gating logic (using Settings::get(), ruby, and try_precompiled) instead of reusing the existing should_try_precompiled() helper, which risks drift with install_version_() and resolve_lock_info(); replace the inline logic with a call to should_try_precompiled() (or otherwise delegate to that helper) where try_precompiled is used so the same central decision is used across resolve_lock_info(), install_version_(), and this code path, ensuring consistent behavior with the planned default flip.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/plugins/core/ruby.rs`:
- Around line 1019-1063: In resolve_lockfile_options, skip adding Ruby
build/precompiled related keys when the target is Windows: detect Windows from
the _target/PlatformTarget and if Windows, return opts without inserting
"compile", "ruby_install", "ruby_install_opts", "ruby_install_repo",
"ruby_build_opts", "ruby_build_repo", "apply_patches", "precompiled_url",
"precompiled_arch", or "precompiled_os"; leave other non-build keys as-is. Use
the existing _target/PlatformTarget check (e.g., is_windows or target.os ==
"windows") before using ruby, try_precompiled, and inserting into opts so
Windows lock entries aren’t affected by unrelated build settings.
---
Nitpick comments:
In `@src/plugins/core/ruby.rs`:
- Around line 1025-1028: The code recomputes the precompiled gating logic (using
Settings::get(), ruby, and try_precompiled) instead of reusing the existing
should_try_precompiled() helper, which risks drift with install_version_() and
resolve_lock_info(); replace the inline logic with a call to
should_try_precompiled() (or otherwise delegate to that helper) where
try_precompiled is used so the same central decision is used across
resolve_lock_info(), install_version_(), and this code path, ensuring consistent
behavior with the planned default flip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 661bcca8-fe49-4392-9a49-8281d463d61a
📒 Files selected for processing (1)
src/plugins/core/ruby.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/core/ruby.rs (1)
1059-1066:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor precompiled arch/os overrides when resolving lock info.
This now records
precompiled_arch/precompiled_osin the lock identity, butresolve_lock_info()still resolves the artifact URL fromprecompiled_platform_for_target(target), which ignores those overrides and only uses the target’s native arch/OS. With overrides set, the lock entry can sayarm64/linuxinoptionswhilePlatformInfo.urlstill points at the target’s default platform, so the lockfile becomes internally inconsistent.Suggested fix
fn precompiled_platform_for_target(&self, target: &PlatformTarget) -> Option<String> { + let settings = Settings::get(); + if let (Some(arch), Some(os)) = ( + settings.ruby.precompiled_arch.as_deref(), + settings.ruby.precompiled_os.as_deref(), + ) { + return Some(format!("{}_{}", arch, os)); + } + match target.os_name() { "macos" => { match target.arch_name() { "arm64" | "aarch64" => Some("macos".to_string()), _ => None,A regression test alongside the existing precompiled-options tests would catch this mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/core/ruby.rs` around lines 1059 - 1066, The lock identity records precompiled_arch/precompiled_os in opts but resolve_lock_info() still uses precompiled_platform_for_target(target) which ignores those overrides, causing inconsistent PlatformInfo.url; fix by making resolve_lock_info() consult opts for "precompiled_arch" and "precompiled_os" (or add a new helper like precompiled_platform_for_target_with_overrides(target, opts)) and use those override values when computing the precompiled platform and artifact URL (ensure you reference resolve_lock_info, precompiled_platform_for_target, and the opts keys "precompiled_arch"/"precompiled_os"); add a regression test similar to the existing precompiled-options tests that sets these overrides and asserts the lockfile's PlatformInfo.url matches the opts-specified platform.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/plugins/core/ruby.rs`:
- Around line 1059-1066: The lock identity records
precompiled_arch/precompiled_os in opts but resolve_lock_info() still uses
precompiled_platform_for_target(target) which ignores those overrides, causing
inconsistent PlatformInfo.url; fix by making resolve_lock_info() consult opts
for "precompiled_arch" and "precompiled_os" (or add a new helper like
precompiled_platform_for_target_with_overrides(target, opts)) and use those
override values when computing the precompiled platform and artifact URL (ensure
you reference resolve_lock_info, precompiled_platform_for_target, and the opts
keys "precompiled_arch"/"precompiled_os"); add a regression test similar to the
existing precompiled-options tests that sets these overrides and asserts the
lockfile's PlatformInfo.url matches the opts-specified platform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8f02e84-372a-4039-987b-16af0a010b4c
📒 Files selected for processing (1)
src/plugins/core/ruby.rs
|
@coderabbitai review This comment was generated by an AI coding assistant. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR diff
gh pr view 9992 --repo jdx/mise --json files,additions,deletions,changedFiles
# Get the diff
gh pr diff 9992 --repo jdx/miseLength of output: 15642 🏁 Script executed: #!/bin/bash
# Check how apply_patches is used in the codebase - does it apply to ruby-install or only ruby-build?
rg -n "apply_patches" src/plugins/core/ruby.rs -C3Length of output: 3562 🏁 Script executed: #!/bin/bash
# Check where apply_patches / --patch is used - is it in the ruby-build path only or also ruby-install?
rg -n "ruby_install\|apply_patches\|--patch\|fetch_patch" src/plugins/core/ruby.rs -C2 | head -120Length of output: 457 🏁 Script executed: #!/bin/bash
rg -n "apply_patches|ruby_install|patch" src/plugins/core/ruby.rs | head -80Length of output: 4860
✅ What's done well
Test scaffolding
🔍 Minor observationsLockfile compatibility (intentional breaking change) Missing test: #[test]
fn test_ruby_lockfile_options_include_ruby_install_inputs_with_patches() {
let opts = resolve_ruby_lockfile_options(|settings| {
settings.ruby.compile = Some(true);
settings.ruby.ruby_install = Some(true);
settings.ruby.apply_patches = Some("https://example.com/ruby.patch".to_string());
});
assert_eq!(
opts,
BTreeMap::from([
("apply_patches".to_string(), "https://example.com/ruby.patch".to_string()),
("compile".to_string(), "true".to_string()),
("ruby_install".to_string(), "true".to_string()),
("ruby_install_repo".to_string(), DEFAULT_RUBY_INSTALL_REPO.to_string()),
])
);
}CI failures Overall this is a clean, well-reasoned change that correctly captures Ruby's artifact identity inputs. The observations above are minor. The extra test is optional but would add useful coverage. This comment was generated by an AI coding assistant. 🧠 Learnings used |
|
@coderabbitai review This comment was generated by an AI coding assistant. |
|
This comment was generated by an AI coding assistant. 🧠 Learnings used✅ Action performedReview finished.
|
This comment was marked as outdated.
This comment was marked as outdated.
2 participants
Summary
Rationale
Lock option values are part of the artifact identity. Recording resolved defaults keeps existing lockfiles stable if mise defaults change later, including Ruby's planned move toward precompiled binaries by default.
Classification
Mixed actual lock/install identity and stale-lock invalidation fix.
Actual identity: source-build settings such as installer choice, build repositories, build options, and patch inputs can change the compiled Ruby tree for the same version. Those inputs are not represented by a locked artifact URL.
Stale-lock invalidation: precompiled URL/platform selector settings mostly choose the binary artifact URL/checksum. Once the lock entry is selected, the locked URL identifies the binary, but the selector still belongs in the exact-options key so changing precompiled source or platform settings does not reuse an older lock entry.
Verification
cargo fmt --checkgit diff --checkcargo test ruby_lockfile_optionstestworkflow on0d98b51f1passed, including unit tests, lint, e2e shards, Windows, andtest-cihyperfine,registry, andreleaseworkflows on0d98b51f1passed or skipped only expected matrix jobsSummary by CodeRabbit
Improvements
Tests