fix(ruby): lock resolved install options#9992
Conversation
Greptile SummaryThis PR fixes the Ruby backend's lock identity to include all inputs that can change the compiled or downloaded artifact: compile mode, installer choice (
Confidence Score: 4/5Safe to merge; the lock identity expansion is correct and well-tested, with one minor duplication that could drift when the precompiled-default TODO is resolved. The logic change is straightforward and backed by six targeted tests. The only concern is that src/plugins/core/ruby.rs — the Important Files Changed
Reviews (7): Last reviewed commit: "fix(ruby): lock resolved install options" | 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
📝 WalkthroughWalkthrough
ChangesRuby Lockfile Options Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
| if is_current_platform { | ||
| opts.insert("compile".to_string(), (!try_precompiled).to_string()); | ||
|
|
||
| // Ruby uses ruby-install vs ruby-build. The installer and its options | ||
| // can affect the source-built output, including fallback after a | ||
| // missing precompiled binary. | ||
| opts.insert("ruby_install".to_string(), ruby.ruby_install.to_string()); | ||
| if ruby.ruby_install { | ||
| if let Some(ruby_install_opts) = ruby.ruby_install_opts.clone() { | ||
| opts.insert("ruby_install_opts".to_string(), ruby_install_opts); | ||
| } | ||
| opts.insert( | ||
| "ruby_install_repo".to_string(), | ||
| ruby.ruby_install_repo.clone(), | ||
| ); | ||
| } else { | ||
| if let Some(ruby_build_opts) = ruby.ruby_build_opts.clone() { | ||
| opts.insert("ruby_build_opts".to_string(), ruby_build_opts); | ||
| } | ||
| opts.insert("ruby_build_repo".to_string(), ruby.ruby_build_repo.clone()); | ||
| } | ||
|
|
||
| // Ruby uses ruby-install vs ruby-build (ruby compiles from source either way) | ||
| // Only include if using non-default ruby-install tool | ||
| let ruby_install = if is_current_platform { | ||
| settings.ruby.ruby_install | ||
| } else { | ||
| false | ||
| }; | ||
| if ruby_install { | ||
| opts.insert("ruby_install".to_string(), "true".to_string()); | ||
| if let Some(apply_patches) = ruby.apply_patches.clone() { | ||
| opts.insert("apply_patches".to_string(), apply_patches); | ||
| } | ||
| } |
There was a problem hiding this comment.
Include resolved source-build defaults for non-current targets too.
For non-current targets this now drops compile, installer choice, repo, build opts, and patches entirely. Those values still affect the fallback/source artifact, and lockfile lookup later requires an exact options-map match, so a cross-platform lock entry can be generated with too little identity and then miss once that platform resolves locally.
🤖 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 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.
| fn test_ruby_lockfile_options_include_experimental_precompiled_default() { | ||
| let opts = resolve_ruby_lockfile_options(|settings| { | ||
| settings.ruby.precompiled_url = Some("acme/ruby".to_string()); | ||
| settings.ruby.precompiled_arch = Some("arm64".to_string()); | ||
| settings.ruby.precompiled_os = Some("linux".to_string()); | ||
| }); | ||
|
|
||
| assert_eq!( | ||
| opts, | ||
| BTreeMap::from([ | ||
| ("compile".to_string(), "false".to_string()), | ||
| ("precompiled_arch".to_string(), "arm64".to_string()), | ||
| ("precompiled_os".to_string(), "linux".to_string()), | ||
| ("precompiled_url".to_string(), "acme/ruby".to_string()), | ||
| ( | ||
| "ruby_build_repo".to_string(), | ||
| DEFAULT_RUBY_BUILD_REPO.to_string(), | ||
| ), | ||
| ("ruby_install".to_string(), "false".to_string()), | ||
| ]) | ||
| ); |
There was a problem hiding this comment.
These assertions encode precompiled-by-default behavior too early.
Both tests expect compile=false/precompiled_url with ruby.compile unset, but should_try_precompiled() and install_version_() still require either ruby.compile = Some(false) or settings.experimental = true. As written, the “experimental default” case never enables experimental, and the “resolved defaults” case asserts the future default instead of the current one.
Also applies to: 1329-1346
🤖 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 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.
1 participant
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