Conversation
…ol>` runs When running `mise lock <tool>` after a tool's resolved version changes, the old lockfile entry was preserved and a new one appended, resulting in duplicate [[tools.<tool>]] sections with different versions. This happened because filtered lock runs skipped all pruning (to avoid removing non-targeted tools), and set_platform_info matches by (version, options), so a version change always creates a new entry. Add retain_tool_versions() to Lockfile and call it for filtered runs to remove stale version entries for targeted tools before processing.
Summary of ChangesHello, 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 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
|
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where mise lock <tool> would not prune stale version entries, leading to duplicate tool sections in the lockfile. The fix introduces a new pruning mechanism specifically for filtered lock runs, which correctly removes old versions of the targeted tool while leaving other tools untouched. The changes are well-implemented and include a comprehensive set of unit tests and a new end-to-end test to verify the fix.
My review includes one suggestion to improve the robustness of a unit test by making its assertion more specific.
| fn test_filtered_run_preserves_non_targeted_tools() { | ||
| // Simulate: lockfile has dummy@1.0.0 and jq@1.7.1, filter targets only dummy | ||
| let cmd = lock_cmd(&["dummy"]); | ||
| let mut lockfile = lockfile_with_dummy(); // has dummy@1.0.0 | ||
| lockfile.set_platform_info( | ||
| "jq", | ||
| "1.7.1", | ||
| Some("aqua:jqlang/jq"), | ||
| &BTreeMap::new(), | ||
| "linux-x64", | ||
| PlatformInfo { | ||
| checksum: Some("sha256:jq".to_string()), | ||
| ..Default::default() | ||
| }, | ||
| ); | ||
| // Resolve dummy to a new version; jq is not targeted | ||
| let tools = vec![configured_tool("dummy", "2.0.0")]; | ||
|
|
||
| cmd.prune_stale_versions_for_targeted_tools(&mut lockfile, &tools); | ||
|
|
||
| // dummy@1.0.0 should be removed, jq@1.7.1 should remain | ||
| assert_eq!( | ||
| lockfile.all_platform_keys(), | ||
| std::collections::BTreeSet::from(["linux-x64".to_string()]) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The assertion in this test is a bit weak. It confirms that a tool on linux-x64 still exists, but doesn't explicitly verify that dummy was pruned and jq was preserved.
To make the test more robust, you could add the jq tool on a different platform (e.g., macos-x64). This way, after pruning dummy (which is on linux-x64), you can assert that only macos-x64 remains in all_platform_keys(), which would strongly prove that dummy was removed while jq was untouched.
#[test]
fn test_filtered_run_preserves_non_targeted_tools() {
// Simulate: lockfile has dummy@1.0.0 and jq@1.7.1, filter targets only dummy
let cmd = lock_cmd(&["dummy"]);
let mut lockfile = lockfile_with_dummy(); // has dummy@1.0.0 on linux-x64
lockfile.set_platform_info(
"jq",
"1.7.1",
Some("aqua:jqlang/jq"),
&BTreeMap::new(),
"macos-x64", // Use a different platform for jq
PlatformInfo {
checksum: Some("sha256:jq".to_string()),
..Default::default()
},
);
// Resolve dummy to a new version; jq is not targeted
let tools = vec![configured_tool("dummy", "2.0.0")];
cmd.prune_stale_versions_for_targeted_tools(&mut lockfile, &tools);
// dummy@1.0.0 (linux-x64) should be removed, jq@1.7.1 (macos-x64) should remain
assert_eq!(
lockfile.all_platform_keys(),
std::collections::BTreeSet::from(["macos-x64".to_string()])
);
}There was a problem hiding this comment.
Good catch — the original assertion was indeed trivially satisfied since both tools shared linux-x64. Moved jq to macos-x64 so the assertion now proves dummy was actually pruned. Addressed in a56583bb1.
This comment was generated by Claude Code.
Greptile SummaryThis PR fixes a bug where running Confidence Score: 5/5Safe to merge — correct fix, well-tested, no blocking issues found. All changes are logically sound: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["mise lock [tool]"] --> B{tools.is_empty?}
B -- Yes --> C{dry_run?}
C -- Yes --> D[show stale tool prune message]
C -- No --> E[prune_stale_entries_if_needed\nwrite lockfile if changed]
B -- No --> F[determine_target_platforms]
F --> G{dry_run?}
G -- Yes --> H[show_dry_run\nread lockfile\nshow stale tool prune\nshow stale version prune]
G -- No --> I[read lockfile\nprune_stale_entries_if_needed\nshow stale tool msg]
I --> J["stale_versions = stale_versions_if_pruned\n(computed BEFORE process_tools)"]
J --> K[process_tools\nupdate lockfile entries]
K --> L["prune_stale_versions\n(removes old versions AFTER process_tools)"]
L --> M[show stale version msg\nwrite lockfile]
style J fill:#ffffcc
style L fill:#ccffcc
Greploops — Automatically fix all review issues by running Reviews (7): Last reviewed commit: "Merge branch 'main' into fix/lock-filter..." | Re-trigger Greptile |
Put jq on a different platform (macos-x64) than dummy (linux-x64) so that after pruning, the assertion on all_platform_keys() actually proves dummy was removed rather than passing trivially. jdx#8599 (comment)
Add dry-run reporting for stale version entries that would be pruned during filtered `mise lock <tool>` runs. Previously, `--dry-run` only reported stale tool pruning for unfiltered runs, silently omitting the version-level pruning that filtered runs perform. Also report version pruning in the non-dry-run path for consistency. jdx#8599 (comment)
|
Addressed the dry-run reporting gap for filtered runs (item 1 from Greptile review). Addressed in |
src/cli/lock.rs
Outdated
| /// entries from accumulating when a tool's resolved version changes. | ||
| fn prune_stale_versions_for_targeted_tools(&self, lockfile: &mut Lockfile, tools: &[LockTool]) { | ||
| if self.is_unfiltered_lock_run() { | ||
| return; // unfiltered runs handle pruning via prune_stale_entries_if_needed |
There was a problem hiding this comment.
I think this might not be true. I haven't looked in detail, but
- I have the same problem with plain
mise lock. - According to AI, (I haven't double-checked, sorry)
prune_stale_entries_if_neededcallsretain_tools_by_short_or_backend, which retains or removes entire tools by name — it answers "ispnpmstill in the config?" If you removedpnpmfrommise.tomlentirely, it removes allpnpmentries from the lockfile.But it never looks at versions within a tool. If
pnpmis still configured and the lockfile has both 10.31.0 and 10.32.0,retain_tools_by_short_or_backendsees "pnpm is configured" and keeps all entries for it, duplicates and all.
There was a problem hiding this comment.
no worry about the ai usage... it's happening here to too. gotta learn how to use the tool well.
thanks for the feedback, i'll take a look into this myself and see what turns up.
There was a problem hiding this comment.
Here is a reproduction, also AI-generated. (I'm not so much apologizing for using AI as for not taking the time to make its output more readable, but if you don't already have a reproduction and want one, it shouldn't be that bad to follow)
https://gist.github.com/ilyagr/aacb2ca0ee868ba10e29f43425a4b692
|
Thanks for the fix! The core approach looks correct and well-tested. Two things to address: 1. Non-dry-run path needs an unfiltered guard In the non-dry-run path (lines 134-136), The dry-run path correctly guards this with the if !self.is_unfiltered_lock_run() {
let stale_versions = self.stale_versions_if_pruned(&lockfile, &tools);
self.prune_stale_versions_for_targeted_tools(&mut lockfile, &tools);
self.show_stale_version_prune_message(lockfile_path, &stale_versions, false)?;
}2. Unfiltered As ilyagr pointed out, unfiltered This comment was generated by Claude Code. |
|
a million monkeys may or may not have written shakespeare so far... but gosh darn it, we'll at least manage to get this pr done with 6 AIs, or whatever we're up to. |
Previously, version-level pruning only happened for filtered runs (`mise lock <tool>`). Unfiltered runs (`mise lock`) would only prune entire tools removed from config, leaving stale version entries when a tool's resolved version changed. This extends version pruning to all runs by removing the early return in prune_stale_versions (renamed from prune_stale_versions_for_targeted_tools). Both dry-run and non-dry-run paths now report tool-level pruning (unfiltered only) and version-level pruning (all runs) consistently.
The guard is unnecessary because prune_stale_entries_if_needed() already has an internal early-return for filtered runs, and show_stale_prune_message() returns early when the stale_tools set is empty.
Deduplicate the map-building logic that was repeated in both prune_stale_versions() and stale_versions_if_pruned(). This follows the same pattern as the existing configured_tool_selectors() helper.
Have prune_stale_versions() return the stale versions that were pruned, matching the pattern of prune_stale_entries_if_needed(). This avoids calling stale_versions_if_pruned() separately before pruning.
Mirrors the pattern used for stale tool pruning where both prune_stale_entries_if_needed() and stale_entries_if_pruned() delegate to stale_entries_for_selectors(). Now prune_stale_versions() and stale_versions_if_pruned() both delegate to the shared stale_versions_for_current() helper.
The stale version pruning was removing old lockfile entries before process_tools() could run provenance checks against them. This caused provenance regression detection to fail because there was no prior version to compare against. Reorder operations to: 1. Compute stale versions (read-only) before process_tools() 2. Run process_tools() with provenance checks against unpruned lockfile 3. Prune stale versions after provenance checks complete
|
@jdx, i think this is ready for review whenever |
### 🚀 Features - **(ci)** auto-convert external PRs to draft mode by @jdx in [#8896](#8896) - **(deps)** add `depends` field for user-specified tool dependencies by @cprecioso in [#8776](#8776) - **(dotnet)** support runtime-only installs by @fragon10 in [#8524](#8524) - **(npm)** apply install_before to transitive dependencies by @risu729 in [#8851](#8851) - **(task)** allow passing arguments to task dependencies via {{usage.*}} templates by @jdx in [#8893](#8893) - add options field to BackendListVersionsCtx by @esteve in [#8875](#8875) ### 🐛 Bug Fixes - **(backend)** filter PEP 440 .dev versions in fuzzy version matching by @richardthe3rd in [#8849](#8849) - **(ci)** update COPR BuildRequires rust version to match MSRV 1.88 by @jdx in [#8911](#8911) - **(ci)** add Ruby build dependencies to e2e Docker image by @jdx in [#8910](#8910) - **(ci)** add missing build dependencies to e2e Docker image by @jdx in [#8912](#8912) - **(ci)** add missing build dependencies to e2e Docker image by @jdx in [#8914](#8914) - **(ci)** use Node 24 LTS for corepack e2e test by @jdx in [#8915](#8915) - **(ci)** add libxml2 and pkg-config to e2e Docker image by @jdx in [#8917](#8917) - **(ci)** add libxml2-dev to e2e image and disable Swift SPM tests by @jdx in [#8918](#8918) - **(docs)** use sans-serif font for badges by @jdx in [#8887](#8887) - **(env)** parse --env=VALUE and -E=VALUE flag forms correctly by @jdx in [#8889](#8889) - **(exec)** use i64::from() for seccomp syscall numbers to survive autofix by @jdx in [#8882](#8882) - **(github)** preserve tool options like filter_bins when version specified via CLI by @jdx in [#8888](#8888) - **(github)** use alias-specific options when tool_alias has its own config by @jdx in [#8892](#8892) - **(install)** add locked_verify_provenance setting and detect github attestations at lock time by @jdx in [#8901](#8901) - **(lock)** prune stale version entries during filtered `mise lock <tool>` runs by @altendky in [#8599](#8599) - **(python)** use lockfile URL for precompiled installs by @hehaoqian in [#8750](#8750) - **(release)** verify all build targets succeed before releasing by @jdx in [#8886](#8886) - **(ruby)** support build revisions for precompiled binaries in mise.lock by @jdx in [#8900](#8900) - **(swift)** fall back to Ubuntu 24.04 for unsupported Ubuntu versions by @jdx in [#8916](#8916) - **(zsh)** avoid duplicate trust warning after cd by @timothysparg in [#8898](#8898) - update flake.lock and add fix for rust-bindgen to default.nix by @esteve in [#8874](#8874) - when direnv diff is empty, do not try to parse it by @yaleman in [#8857](#8857) - skip trust check for plain .tool-versions in task list by @dportalesr in [#8876](#8876) ### 🚜 Refactor - **(go)** rename go_* settings to go.* namespace by @jdbruijn in [#8598](#8598) ### 📚 Documentation - **(tasks)** clarify task_config.includes behavior by @risu729 in [#8905](#8905) ### 🧪 Testing - **(ci)** run e2e tests inside Docker containers by @jdx in [#8899](#8899) ### 📦️ Dependency Updates - bump ubi from 0.8 to 0.9 by @jdx in [#8906](#8906) - bump zip from 3 to 8 by @jdx in [#8908](#8908) - update lockfile deps (hold back rattler) by @jdx in [#8909](#8909) - update bun.lock by @jdx in [#8913](#8913) ### 📦 Registry - add turso ([github:tursodatabase/turso-cli](https://github.com/tursodatabase/turso-cli)) by @kenn in [#8884](#8884) - remove carp test by @jdx in [#8894](#8894) ### Chore - **(ci)** add workflow to warn PRs modifying vendored aqua-registry by @jdx in [#8897](#8897) - **(ci)** use github.token for draft conversion in auto-draft workflow by @jdx in [#8903](#8903) - remove deprecated settings older than 12 months by @jdx in [#8904](#8904) ### New Contributors - @dportalesr made their first contribution in [#8876](#8876) - @timothysparg made their first contribution in [#8898](#8898) - @hehaoqian made their first contribution in [#8750](#8750) - @jdbruijn made their first contribution in [#8598](#8598) - @cprecioso made their first contribution in [#8776](#8776) - @yaleman made their first contribution in [#8857](#8857) - @kenn made their first contribution in [#8884](#8884) - @fragon10 made their first contribution in [#8524](#8524) ## 📦 Aqua Registry Updates #### New Packages (6) - [`ahkohd/oyo`](https://github.com/ahkohd/oyo) - [`bellicose100xp/jiq`](https://github.com/bellicose100xp/jiq) - [`kurama/dealve-tui`](https://github.com/kurama/dealve-tui) - [`micahkepe/jsongrep`](https://github.com/micahkepe/jsongrep) - [`textfuel/lazyjira`](https://github.com/textfuel/lazyjira) - [`ubugeeei/vize`](https://github.com/ubugeeei/vize) #### Updated Packages (1) - [`sigstore/cosign`](https://github.com/sigstore/cosign)
Summary
When running
mise lock <tool>after a tool's resolved version changes, the old lockfile entry was preserved and a new one appended, resulting in duplicate[[tools.<tool>]]sections with different versions.For example, if
mise.tomlhasnode = "24"andmise.lockpreviously resolved to24.13.0, then after24.14.0becomes available and is installed, runningmise lock nodeproduces:The expected behavior is that
mise lock nodeshould replace the stale24.13.0entry with the24.14.0entry, not append alongside it.Cause
Two code locations interact to produce this bug:
Filtered lock runs skip all pruning (
src/cli/lock.rs:168-176):prune_stale_entries_if_needed()returns early for filtered runs (mise lock <tool>) to avoid removing non-targeted tools. But this also prevents pruning old versions of the targeted tool itself.set_platform_infomatches by(version, options)(src/lockfile.rs:655-705): a version change always creates a new entry rather than updating the existing one.Prior art
update_lockfiles()code path used bymise install/mise upgrade. Themise lockCLI uses a separate code path that was not covered.mise lock, but only for unfiltered runs and only by tool name (tools removed from config entirely), not by stale versions within a targeted tool.Fix
Lockfile::retain_tool_versions(short, keep_versions)that removes entries for a tool whose version is not in the given set.Lock::prune_stale_versions_for_targeted_tools()that, for filtered runs only, collects the current resolved versions per tool and callsretain_tool_versionsbefore processing.Tests
test_lockfile_lock_filtered_prune_stale_versions) verifying the end-to-end scenario.