Conversation
Summary of ChangesHello @mackwic, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
4da7037 to
dd8546f
Compare
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue with cross-platform lockfile generation for the aqua backend, where host-specific overrides were incorrectly applied to other target platforms. The fix is elegant, fetching the base package configuration before applying target-specific overrides, which directly addresses the root cause. The new end-to-end test is excellent, as it clearly reproduces the bug and validates the fix, preventing future regressions. The code changes are clean, the logic is sound, and the pull request is well-documented. I have no further suggestions; this looks good to merge.
There was a problem hiding this comment.
Pull request overview
This PR fixes a cross-platform lockfile resolution bug in the aqua backend where host-specific package overrides could leak into lockfiles generated for different target platforms.
Changes:
- Modified
resolve_lock_infoto use base package with target platform overrides instead of host platform overrides - Added regression test verifying cross-platform lockfile resolution
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/backend/aqua.rs | Fixed lockfile resolution to use target platform instead of host platform when applying aqua package overrides |
| e2e/lockfile/test_lockfile_aqua_cross_platform_override | Added regression test that verifies lockfile URLs use target platform and don't leak host-only overrides |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code change looks correct. The bug is clear: package_with_version() calls with_version(versions, os(), arch()) using the host platform, and then the caller was calling with_version() again with the target platform. But since overrides mutate the package in place (replacing fields like url), the host override was already baked in and the second with_version() for the target couldn't undo it.
The fix to use package() (no overrides) then a single with_version(&versions, target_os, target_arch) is the right approach.
Two minor notes on the e2e test, neither blocking:
-
The test uses
return 0for the unsupported-OS early exit, but e2e tests are sourced/executed as scripts, not functions —exit 0would be more conventional here (other tests liketest_lockfile_pythonuseexit 0in similar guards). This could cause a syntax error or unexpected behavior if the test runner doesn't source the file. -
The test file was committed with
+xpermission. CLAUDE.md says "we don't chmod mise e2e tests to be executable" — though the autofix commit may have handled this, worth confirming it's consistent with other tests ine2e/lockfile/.
The Rust change itself is clean and minimal — nice fix.
AI Generated
### 🚀 Features - **(task)** stream keep-order output in real-time per task by @jdx in [#8164](#8164) ### 🐛 Bug Fixes - **(aqua)** resolve lockfile artifacts for target platform (fix discussion #7479) by @mackwic in [#8183](#8183) - **(exec)** strip shims from PATH to prevent recursive shim execution by @jdx in [#8189](#8189) - **(hook-env)** preserve PATH reordering done after activation by @jdx in [#8190](#8190) - **(lockfile)** resolve version aliases before lockfile lookup by @jdx in [#8194](#8194) - **(registry)** set helm-diff archive bin name to diff by @jean-humann in [#8173](#8173) - **(task)** improve source freshness checks with dynamic task dirs by @rooperuu in [#8169](#8169) - **(task)** resolve global tasks when running from monorepo root by @jdx in [#8192](#8192) - **(task)** prevent wildcard glob `test:*` from matching parent task `test` by @jdx in [#8165](#8165) - **(task)** resolve task_config.includes relative to config root by @jdx in [#8193](#8193) - **(upgrade)** skip untrusted tracked configs during upgrade by @jdx in [#8195](#8195) ### 🚜 Refactor - use enum for npm.pacakge_manager by @risu729 in [#8180](#8180) ### 📚 Documentation - **(plugins)** replace node/asdf-nodejs examples with vfox plugins by @jdx in [#8191](#8191) ### ⚡ Performance - call npm view only once by @risu729 in [#8181](#8181) ### New Contributors - @jean-humann made their first contribution in [#8173](#8173) - @mackwic made their first contribution in [#8183](#8183) - @rooperuu made their first contribution in [#8169](#8169) ## 📦 Aqua Registry Updates #### New Packages (2) - [`BetterDiscord/cli`](https://github.com/BetterDiscord/cli) - [`glossia.ai/cli`](https://github.com/glossia.ai/cli)
…sion jdx#7479) (jdx#8183) ## Summary This PR fixes cross-platform lockfile resolution for the `aqua` backend. When running `mise lock --platform <target>`, `mise.lock` could contain a host-specific artifact URL if the aqua package had host-only overrides. This is the behavior reported in: jdx#7479 ## Root Cause In `resolve_lock_info`, we were calling `AQUA_REGISTRY.package_with_version(...)` first, which applies overrides using the **host** OS/arch. We then applied `with_version(..., target_os, target_arch)` again. Because host overrides were already merged into the package, host-specific values could leak into lock entries for a different target platform. ## Fix In `src/backend/aqua.rs`, `resolve_lock_info` now: 1. loads the base package with `AQUA_REGISTRY.package(...)` 2. applies version/overrides once, directly for the requested target: `pkg.with_version(&versions, target_os, target_arch)` This ensures lockfile URL/checksum resolution is based on the **target** platform only. ## Tests Added a regression e2e test in `e2e/lockfile/test_lockfile_aqua_cross_platform_override`. The test creates a local aqua registry package with: - a generic base URL - a host-only override URL Then it runs `mise lock --platform windows-x64` and asserts: - lockfile uses the expected base target URL - lockfile does **not** contain the host override URL Validation: - test fails on pre-fix code (reproduces discussion jdx#7479) - test passes with this patch --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
### 🚀 Features - **(task)** stream keep-order output in real-time per task by @jdx in [jdx#8164](jdx#8164) ### 🐛 Bug Fixes - **(aqua)** resolve lockfile artifacts for target platform (fix discussion jdx#7479) by @mackwic in [jdx#8183](jdx#8183) - **(exec)** strip shims from PATH to prevent recursive shim execution by @jdx in [jdx#8189](jdx#8189) - **(hook-env)** preserve PATH reordering done after activation by @jdx in [jdx#8190](jdx#8190) - **(lockfile)** resolve version aliases before lockfile lookup by @jdx in [jdx#8194](jdx#8194) - **(registry)** set helm-diff archive bin name to diff by @jean-humann in [jdx#8173](jdx#8173) - **(task)** improve source freshness checks with dynamic task dirs by @rooperuu in [jdx#8169](jdx#8169) - **(task)** resolve global tasks when running from monorepo root by @jdx in [jdx#8192](jdx#8192) - **(task)** prevent wildcard glob `test:*` from matching parent task `test` by @jdx in [jdx#8165](jdx#8165) - **(task)** resolve task_config.includes relative to config root by @jdx in [jdx#8193](jdx#8193) - **(upgrade)** skip untrusted tracked configs during upgrade by @jdx in [jdx#8195](jdx#8195) ### 🚜 Refactor - use enum for npm.pacakge_manager by @risu729 in [jdx#8180](jdx#8180) ### 📚 Documentation - **(plugins)** replace node/asdf-nodejs examples with vfox plugins by @jdx in [jdx#8191](jdx#8191) ### ⚡ Performance - call npm view only once by @risu729 in [jdx#8181](jdx#8181) ### New Contributors - @jean-humann made their first contribution in [jdx#8173](jdx#8173) - @mackwic made their first contribution in [jdx#8183](jdx#8183) - @rooperuu made their first contribution in [jdx#8169](jdx#8169) ## 📦 Aqua Registry Updates #### New Packages (2) - [`BetterDiscord/cli`](https://github.com/BetterDiscord/cli) - [`glossia.ai/cli`](https://github.com/glossia.ai/cli)
### 🚀 Features - **(task)** stream keep-order output in real-time per task by @jdx in [jdx#8164](jdx#8164) ### 🐛 Bug Fixes - **(aqua)** resolve lockfile artifacts for target platform (fix discussion jdx#7479) by @mackwic in [jdx#8183](jdx#8183) - **(exec)** strip shims from PATH to prevent recursive shim execution by @jdx in [jdx#8189](jdx#8189) - **(hook-env)** preserve PATH reordering done after activation by @jdx in [jdx#8190](jdx#8190) - **(lockfile)** resolve version aliases before lockfile lookup by @jdx in [jdx#8194](jdx#8194) - **(registry)** set helm-diff archive bin name to diff by @jean-humann in [jdx#8173](jdx#8173) - **(task)** improve source freshness checks with dynamic task dirs by @rooperuu in [jdx#8169](jdx#8169) - **(task)** resolve global tasks when running from monorepo root by @jdx in [jdx#8192](jdx#8192) - **(task)** prevent wildcard glob `test:*` from matching parent task `test` by @jdx in [jdx#8165](jdx#8165) - **(task)** resolve task_config.includes relative to config root by @jdx in [jdx#8193](jdx#8193) - **(upgrade)** skip untrusted tracked configs during upgrade by @jdx in [jdx#8195](jdx#8195) ### 🚜 Refactor - use enum for npm.pacakge_manager by @risu729 in [jdx#8180](jdx#8180) ### 📚 Documentation - **(plugins)** replace node/asdf-nodejs examples with vfox plugins by @jdx in [jdx#8191](jdx#8191) ### ⚡ Performance - call npm view only once by @risu729 in [jdx#8181](jdx#8181) ### New Contributors - @jean-humann made their first contribution in [jdx#8173](jdx#8173) - @mackwic made their first contribution in [jdx#8183](jdx#8183) - @rooperuu made their first contribution in [jdx#8169](jdx#8169) ## 📦 Aqua Registry Updates #### New Packages (2) - [`BetterDiscord/cli`](https://github.com/BetterDiscord/cli) - [`glossia.ai/cli`](https://github.com/glossia.ai/cli)
Summary
This PR fixes cross-platform lockfile resolution for the
aquabackend.When running
mise lock --platform <target>,mise.lockcould contain a host-specific artifact URL if the aqua package had host-only overrides.This is the behavior reported in: #7479
Root Cause
In
resolve_lock_info, we were callingAQUA_REGISTRY.package_with_version(...)first, which appliesoverrides using the host OS/arch.
We then applied
with_version(..., target_os, target_arch)again.Because host overrides were already merged into the package, host-specific values could leak into lock entries for a different target platform.
Fix
In
src/backend/aqua.rs,resolve_lock_infonow:AQUA_REGISTRY.package(...)pkg.with_version(&versions, target_os, target_arch)This ensures lockfile URL/checksum resolution is based on the target platform only.
Tests
Added a regression e2e test in
e2e/lockfile/test_lockfile_aqua_cross_platform_override.The test creates a local aqua registry package with:
Then it runs
mise lock --platform windows-x64and asserts:Validation: