fix(aqua): canonicalize and reject duplicate vars#10187
Conversation
|
Ready to act? Review this PR in Change Stack to turn feedback into patch suggestions you can inspect and refine. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR makes lockfile option resolution fallible across all backends and plugins, returning ChangesLockfile Options Resolution Fallibility & Aqua Canonicalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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. Comment |
Greptile SummaryThis PR canonicalizes Aqua backend variable options across three spellings — plain key (
Confidence Score: 5/5Safe to merge. The canonicalization logic is correct across all three spellings, conflict detection fires exactly when it should, and the config-layer override path correctly avoids false positives. The new No files require special attention. Important Files Changed
Reviews (15): Last reviewed commit: "fix(lint): allow dead_code on unused Pro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
2 times, most recently
from
98b8e86 to
71b89ca
Compare
risu729
changed the title
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
from
71b89ca to
192214e
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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-1039:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude the precompiled/source selector in Ruby lockfile options.
This still keys only on
ruby_install, butresolve_lock_infobelow switches artifact source based onshould_try_precompiled()andresolve_precompiled_url(...). Aruby.compile=falseor custom precompiled source can therefore collide with the source-tarball entry and reuse the wrong lock metadata.Suggested fix
fn resolve_lockfile_options( &self, _request: &ToolRequest, target: &PlatformTarget, ) -> Result<BTreeMap<String, String>> { let mut opts = BTreeMap::new(); let settings = Settings::get(); let is_current_platform = target.is_current(); + let use_precompiled = self.should_try_precompiled(); + if use_precompiled { + opts.insert("precompiled".to_string(), "true".to_string()); + opts.insert( + "precompiled_url".to_string(), + settings.ruby.precompiled_url.clone(), + ); + if let Some(arch) = settings.ruby.precompiled_arch.clone() { + opts.insert("precompiled_arch".to_string(), arch); + } + if let Some(os) = settings.ruby.precompiled_os.clone() { + opts.insert("precompiled_os".to_string(), os); + } + } + let ruby_install = if is_current_platform { settings.ruby.ruby_install } else { false };🤖 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 - 1039, The current resolve_lockfile_options function only records ruby_install and omits the artifact source selector, causing resolve_lock_info to pick wrong lock metadata; update resolve_lockfile_options to include the precompiled/source flags used by resolve_lock_info — specifically add the ruby.compile (or equivalent Settings::ruby.compile) value and any custom precompiled source identifier (the value used by resolve_precompiled_url and should_try_precompiled) into the returned opts BTreeMap so resolve_lock_info can deterministically choose precompiled vs source artifacts.
🤖 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/backend/aqua.rs`:
- Around line 74-95: canonical_var_options and lockfile_options currently drop
invalid vars silently; change them to fail fast: in canonical_var_options(),
when encountering key == "vars" and the value is not a toml::Value::Table,
return an Err describing "vars must be a table" (refer to canonical_var_options
and insert_nested_var_options for where to validate), and in lockfile_options()
stop using filter_map so non-string toml values are not ignored—call
toml_value_to_string for each value and if it fails return an Err (include the
offending key in the error) instead of skipping it so invalid config surfaces
during lockfile canonicalization (update the error messages to reference
toml_value_to_string, lockfile_options, and canonical_var_options).
---
Outside diff comments:
In `@src/plugins/core/ruby.rs`:
- Around line 1019-1039: The current resolve_lockfile_options function only
records ruby_install and omits the artifact source selector, causing
resolve_lock_info to pick wrong lock metadata; update resolve_lockfile_options
to include the precompiled/source flags used by resolve_lock_info — specifically
add the ruby.compile (or equivalent Settings::ruby.compile) value and any custom
precompiled source identifier (the value used by resolve_precompiled_url and
should_try_precompiled) into the returned opts BTreeMap so resolve_lock_info can
deterministically choose precompiled vs source artifacts.
🪄 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: 96139857-6fc5-4e1f-bb6a-ebe6fd5ba934
📒 Files selected for processing (18)
src/backend/aqua.rssrc/backend/cargo.rssrc/backend/conda.rssrc/backend/github.rssrc/backend/go.rssrc/backend/mod.rssrc/backend/npm.rssrc/backend/pipx.rssrc/backend/spm.rssrc/backend/ubi.rssrc/lockfile.rssrc/plugins/core/erlang.rssrc/plugins/core/java.rssrc/plugins/core/node.rssrc/plugins/core/python.rssrc/plugins/core/ruby.rssrc/plugins/core/rust.rssrc/toolset/tool_request.rs
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
from
192214e to
967bccb
Compare
|
@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 generated by an AI coding assistant. 🧠 Learnings used✅ Action performedReview finished.
|
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
from
967bccb to
7766806
Compare
|
@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.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/backend/aqua.rs (1)
74-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on invalid Aqua var values.
lockfile_options()still drops non-string vars viafilter_map, andcanonical_var_options()still treatsvars = <non-table>as a no-op. That means invalid Aqua config can still produce a partial/empty lockfile or bin-path cache key instead of surfacing through the newResultpath.Also applies to: 91-95
🤖 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/backend/aqua.rs` around lines 74 - 81, The code currently swallows invalid var values by filtering out non-string TOML values; update lockfile_options to propagate errors instead of using filter_map: call toml_value_to_string for each (key,value), map failures into an Err and collect into a Result<BTreeMap<...>, Error> (so use a fallible collect or explicit loop that returns Err on the first invalid value) and return that Err; likewise, change canonical_var_options to treat a non-table `vars` entry as an error (return Err when vars is present but not a table) rather than treating it as a no-op; use the existing helper toml_value_to_string, and reference lockfile_options, canonical_var_options and toml_value_to_string when making these changes.
🤖 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.
Duplicate comments:
In `@src/backend/aqua.rs`:
- Around line 74-81: The code currently swallows invalid var values by filtering
out non-string TOML values; update lockfile_options to propagate errors instead
of using filter_map: call toml_value_to_string for each (key,value), map
failures into an Err and collect into a Result<BTreeMap<...>, Error> (so use a
fallible collect or explicit loop that returns Err on the first invalid value)
and return that Err; likewise, change canonical_var_options to treat a non-table
`vars` entry as an error (return Err when vars is present but not a table)
rather than treating it as a no-op; use the existing helper
toml_value_to_string, and reference lockfile_options, canonical_var_options and
toml_value_to_string when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eabc5b97-7897-4281-974b-3a9a4ee15cbc
📒 Files selected for processing (22)
e2e/backend/test_aqua_varssrc/backend/aqua.rssrc/backend/cargo.rssrc/backend/conda.rssrc/backend/github.rssrc/backend/go.rssrc/backend/mod.rssrc/backend/npm.rssrc/backend/pipx.rssrc/backend/spm.rssrc/backend/ubi.rssrc/cli/lock.rssrc/config/config_file/mise_toml.rssrc/lockfile.rssrc/plugins/core/erlang.rssrc/plugins/core/java.rssrc/plugins/core/node.rssrc/plugins/core/python.rssrc/plugins/core/ruby.rssrc/plugins/core/rust.rssrc/toolset/tool_request.rssrc/toolset/tool_version_options.rs
💤 Files with no reviewable changes (1)
- src/toolset/tool_version_options.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- src/plugins/core/erlang.rs
- src/plugins/core/rust.rs
- src/backend/pipx.rs
- src/backend/cargo.rs
- src/backend/conda.rs
- src/backend/spm.rs
- src/plugins/core/java.rs
- src/plugins/core/ruby.rs
- src/backend/github.rs
- src/backend/npm.rs
- src/lockfile.rs
- src/backend/mod.rs
- src/plugins/core/python.rs
- src/backend/go.rs
- src/backend/ubi.rs
- e2e/backend/test_aqua_vars
- src/toolset/tool_request.rs
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
from
7766806 to
c5d9b1a
Compare
|
@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.
|
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
3 times, most recently
from
2dcb80d to
f87decf
Compare
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
from
f87decf to
824a202
Compare
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 4 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 5 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 6 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
2 times, most recently
from
ce37c9a to
b2f9098
Compare
risu729
force-pushed
the
fix/aqua-vars-canonical-options
branch
from
b2f9098 to
297fc35
Compare
2 participants
Summary
vars, and literalvars.<name>backend-option keys into one lock/cache identityconflicting aqua varerror instead of picking one spellingDetails
TOML already treats unquoted dotted keys and tables as the same structure:
vars.channel = "stable"and[vars] channel = "stable"both parse as a nestedvarstable. This PR does not distinguish those TOML forms.The extra spelling mise has to normalize is the literal backend-option key
"vars.channel", which can come from backend arg/manual option parsing or persisted raw option maps rather than TOML dotted-key parsing. Aqua now canonicalizes that literal key to the samevars.channellock/cache identity as nested TOMLvars.channeland the documented plainchannelvar option.If two entries in the final flattened options map collapse to the same Aqua var, Aqua stops with
conflicting aqua var. If a higher-precedence config layer overrides a lower-precedence layer before Aqua sees the options, only the winning value reaches the backend, so normal config precedence still applies silently.Note
mise lockstill has existing best-effort behavior that can ignore invalid config/tool request conversion or skip failed per-platform lock resolution while writing successful entries. This PR does not change that broader policy; making invalid in-scope config fatal formise lockshould be handled separately.Test plan
mise run test:e2e e2e/backend/test_aqua_vars