fix(lock): disambiguate idiomatic lock target#10319
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds precedence tie-breaking to idiomatic version-file lockfile selection in ChangesIdiomatic version-file lockfile precedence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/lockfile/test_lockfile_idiomatic_version_file (1)
51-64: 💤 Low valueConsider adding a comment to clarify the test scenario.
The test correctly validates that when both
mise.tomland.mise/config.tomlexist in the same directory,.mise/config.tomlwins the precedence tie-breaker for idiomatic version file lockfile selection. However, the intentional coexistence of both configs (created on lines 45-49 and 52-56) might not be immediately obvious to future maintainers.Consider adding a comment before line 51 to make the test intent explicit:
# Test precedence when both mise.toml and .mise/config.toml exist at same root. # .mise/config.toml should win and idiomatic .dummy-version should map to .mise/mise.lock.This would make the test scenario clearer and help prevent confusion about why the previous
mise.tomlisn't cleaned up before creating.mise/config.toml.🤖 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 `@e2e/lockfile/test_lockfile_idiomatic_version_file` around lines 51 - 64, Add an inline comment in the test_lockfile_idiomatic_version_file before the block that creates .mise/config.toml to document the intent: explain that mise.toml and .mise/config.toml are intentionally created together to verify precedence (that .mise/config.toml wins and .dummy-version maps to .mise/mise.lock). Reference the created artifacts (.mise/config.toml, mise.toml, .dummy-version, and the expected .mise/mise.lock) in the comment so future maintainers understand why mise.toml is not removed prior to creating .mise/config.toml.src/lockfile.rs (1)
901-901: ⚡ Quick winDocument the precedence ordering assumption.
The comment on line 901 states that "the highest-precedence config wins" when multiple configs share a root, and the implementation uses iteration index (
idx) as the tie-breaker. However, the code doesn't document a critical assumption: thatconfig.config_filesis ordered such that higher indices correspond to higher precedence configs.Without this documentation, future maintainers might not understand:
- Why the enumeration index is used as a tie-breaker
- That the ordering of
config.config_filesis part of the contract- Whether changes to config loading order could break this logic
Consider adding a comment explaining this assumption, for example:
/// If multiple configs share that root, the highest-precedence config wins. /// Config precedence is determined by: /// 1. Root depth (deeper roots override shallower ones) /// 2. Base vs. local/env-specific (base configs preferred for idiomatic files) /// 3. Iteration order in config.config_files (higher index = higher precedence; /// e.g., .mise/config.toml typically has higher precedence than mise.toml)Also applies to: 911-927
🤖 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/lockfile.rs` at line 901, Update the comment around the logic that picks the "highest-precedence config" (the code that iterates config.config_files and uses idx as a tie-breaker) to explicitly document the precedence contract: that config.config_files is ordered with higher indices meaning higher precedence, that tie-breaking uses the enumeration index (idx), and the effective precedence rules (e.g., deeper root overrides shallower, base vs local/env rules, then iteration order where higher index wins — e.g., .mise/config.toml > mise.toml). Mention this assumption wherever the idx tie-break is used (the block around the "highest-precedence config wins" comment and the related code at lines ~911-927) so future maintainers know changing config loading order can break the logic.
🤖 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.
Nitpick comments:
In `@e2e/lockfile/test_lockfile_idiomatic_version_file`:
- Around line 51-64: Add an inline comment in the
test_lockfile_idiomatic_version_file before the block that creates
.mise/config.toml to document the intent: explain that mise.toml and
.mise/config.toml are intentionally created together to verify precedence (that
.mise/config.toml wins and .dummy-version maps to .mise/mise.lock). Reference
the created artifacts (.mise/config.toml, mise.toml, .dummy-version, and the
expected .mise/mise.lock) in the comment so future maintainers understand why
mise.toml is not removed prior to creating .mise/config.toml.
In `@src/lockfile.rs`:
- Line 901: Update the comment around the logic that picks the
"highest-precedence config" (the code that iterates config.config_files and uses
idx as a tie-breaker) to explicitly document the precedence contract: that
config.config_files is ordered with higher indices meaning higher precedence,
that tie-breaking uses the enumeration index (idx), and the effective precedence
rules (e.g., deeper root overrides shallower, base vs local/env rules, then
iteration order where higher index wins — e.g., .mise/config.toml > mise.toml).
Mention this assumption wherever the idx tie-break is used (the block around the
"highest-precedence config wins" comment and the related code at lines ~911-927)
so future maintainers know changing config loading order can break the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c7ac3b9-667d-4fce-9f40-8bb62fd069e5
📒 Files selected for processing (2)
e2e/lockfile/test_lockfile_idiomatic_version_filesrc/lockfile.rs
Greptile SummaryFixes ambiguous lockfile targeting when an idiomatic version file (e.g.
Confidence Score: 5/5Safe to merge — the change is minimal, targeted, and validated end-to-end. The logic change is a single additional tiebreaker key in one max_by_key call; it only activates when two base configs share the same root depth, and the e2e test directly exercises that exact scenario. Cleanup, teardown, and the surrounding test flow are all correct. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "docs(lock): clarify idiomatic lock targe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.2 x -- echo |
23.2 ± 2.5 | 20.2 | 35.1 | 1.00 |
mise x -- echo |
23.6 ± 1.7 | 21.3 | 47.5 | 1.02 ± 0.13 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.2 env |
21.7 ± 1.1 | 19.5 | 27.6 | 1.00 |
mise env |
22.3 ± 1.2 | 20.1 | 28.7 | 1.03 ± 0.08 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.2 hook-env |
22.2 ± 1.1 | 19.9 | 27.5 | 1.00 |
mise hook-env |
23.1 ± 1.1 | 20.5 | 28.6 | 1.04 ± 0.07 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.2 ls |
17.4 ± 1.2 | 14.7 | 23.7 | 1.00 |
mise ls |
17.8 ± 1.4 | 15.3 | 23.2 | 1.03 ± 0.11 |
xtasks/test/perf
| Command | mise-2026.6.2 | mise | Variance |
|---|---|---|---|
| install (cached) | 135ms | 134ms | +0% |
| ls (cached) | 58ms | 61ms | -4% |
| bin-paths (cached) | 71ms | 70ms | +1% |
| task-ls (cached) | 134ms | 132ms | +1% |
1 participant
Summary
Validation
Follow-up to #10309.
Note
Medium Risk
Changes which lockfile path is used for idiomatic version sources in multi-config repos; behavior is more deterministic but may differ from the previous ambiguous choice for edge cases.
Overview
Fixes ambiguous lockfile targeting when an idiomatic version file (e.g.
.dummy-version) is covered by more than one basemise.tomlat the same project root.lockfile_path_for_tool_sourcestill picks the deepest matching config root and prefers base configs, but when several configs tie on those criteria it now breaks ties usingconfig_filesorder (later entry wins). That makes colocatedmise.tomland.mise/config.tomlconsistently map the idiomatic file to.mise/mise.lockinstead of the rootmise.lock.E2e coverage runs
mise lock --dry-runwith both configs present and asserts the lock output targets.mise/mise.lock, not the root lockfile.Reviewed by Cursor Bugbot for commit be58caf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Tests
Bug Fixes