Skip to content

fix(lock): fail when active tools cannot resolve - #10586

Merged
jdx merged 5 commits into
mainfrom
codex/fail-lock-unresolved-tools
Jun 23, 2026
Merged

jdx merged 5 commits into
mainfrom
codex/fail-lock-unresolved-tools

Conversation

@jdx

@jdx jdx commented Jun 23, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • make mise lock return an error when an active lock-scope tool request cannot resolve
  • keep overridden fallback behavior for latest and idiomatic version files
  • add an e2e regression covering explicit and unfiltered lock runs leaving stale lockfile content untouched on resolution failure

Tests

  • cargo fmt --check
  • cargo test --bin mise cli::lock::tests
  • mise run test:e2e e2e/cli/test_lock_resolution_failure

Note

Medium Risk
Changes lockfile update preconditions and error propagation in get_tools_to_lock; behavior shifts from silent skip to hard failure for active unresolved tools, which is intended but affects CI and local lock workflows.

Overview
mise lock now errors when a tool in the current lock scope has an active config request that cannot be resolved, instead of silently skipping it and still updating the lockfile.

In get_tools_to_lock, matching requests use a shared request_matches helper, and active_unresolved marks config requests that are still active in the toolset for the tools being locked (all tools on an unfiltered run, or only the named tools on a filtered run). Resolution/option failures for those requests propagate via Result; other overridden/latest/idiomatic fallbacks still log and continue as before.

The lock command awaits that Result, so a failed resolve aborts the run and leaves existing lock content unchanged. A new e2e test (RTX_FAILURE=1) covers unfiltered and explicit dummy failures plus mise lock jq succeeding without touching a broken dummy entry.

Reviewed by Cursor Bugbot for commit 7f812f8. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Bug Fixes
    • Improved lockfile resolution behavior so the lock command correctly errors when overridden tool requests cannot be resolved, avoiding unintended updates to the lockfile.
    • Enhanced resolution logic for active/unresolved and version override scenarios, providing clearer failure behavior instead of silently continuing.
  • Tests
    • Added end-to-end coverage for lock resolution failure cases, including scenarios with and without successful overridden tool resolution.

@coderabbitai

coderabbitai Bot commented Jun 23, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

get_tools_to_lock in src/cli/lock.rs is changed to return Result<Vec<LockTool>>. A request_matches helper is added for version/options comparison. active_unresolved detection routes overridden-request resolution failures as hard errors (with lockfile path context) when the active toolset already matches the override, rather than silently skipping. Lock::run now propagates these errors via .await?. A new e2e test verifies that mise lock emits "failed to resolve" and leaves mise.lock unchanged on failure.

Changes

Lock resolution failure propagation

Layer / File(s) Summary
request_matches helper, Result return type, and initialization
src/cli/lock.rs
Adds request_matches predicate to compare ToolRequest by version and options; changes get_tools_to_lock return type to Result<Vec<LockTool>>; replaces inline equality check with the shared helper.
active_unresolved detection and dual-path failure handling
src/cli/lock.rs
Introduces active_unresolved detection by checking whether the resolved toolset already contains a matching request. Routes option-resolution and override-resolution failures as hard errors (with lockfile-path context) when active_unresolved is true; otherwise logs debug and continues.
Result wrapping at return points and error propagation in Lock::run
src/cli/lock.rs
Wraps both early-return paths (unfiltered and explicit-tools branch) with Ok(...); in Lock::run, awaits get_tools_to_lock with ? operator so resolution errors propagate instead of being implicitly ignored.
E2e test: resolution failure leaves mise.lock unchanged
e2e/cli/test_lock_resolution_failure
Test setup enables MISE_LOCKFILE=1. First scenario seeds mise.lock with an older dummy version, runs mise lock under RTX_FAILURE=1, asserts "failed to resolve" output, and verifies mise.lock retains the original version. Second scenario adds a jq tool, runs mise lock jq under RTX_FAILURE=1, asserts it processes only jq, avoids reporting dummy failure, and updates mise.lock to jq without switching to the newer dummy version.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • jdx/mise#10309: Directly modifies get_tools_to_lock and override-resolution logic in src/cli/lock.rs, overlapping with the same code paths changed here for idiomatic-version-file tool targeting and override resolution criteria.

Poem

🐇 Hop, hop, the lock must hold tight,
When resolution fails in the night.
No rewriting versions of old,
The lockfile stays as previously told.
mise lock now shouts "failed to resolve!"
And keeps your versions safe to evolve. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(lock): fail when active tools cannot resolve' clearly and concisely summarizes the main change—modifying the lock command to fail with an error when active tools cannot be resolved, rather than silently skipping them.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jun 23, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes mise lock return an error when a tool that is active in the current toolset cannot be resolved during lockfile generation, preventing stale lockfile entries from being silently preserved. Overridden tools, latest-version requests that are not the winning active request, and idiomatic-version-file sources retain the previous log-and-continue fallback.

  • Converts get_tools_to_lock to Result<Vec<LockTool>> and propagates the error via ? at the call site, ensuring no partial lockfile write occurs on failure.
  • Introduces active_unresolved — gated on the priority-winning requests in tvl.requests (consistent with Toolset::merge semantics) and the tool filter — to gate the new error paths.
  • Adds request_matches helper and a dedicated e2e test covering explicit-filter, unfiltered, and cross-tool-filter failure scenarios.

Confidence Score: 5/5

Safe to merge — changes are narrowly scoped to the resolution-failure path and the active_unresolved guard correctly limits new errors to tools in both the active toolset and the user's filter.

The active_unresolved logic relies on tvl.requests reflecting only priority-winning requests (confirmed via Toolset::merge), so overridden configs cannot accidentally trigger the new error path. The tool-filter guard is evaluated before active_unresolved, correctly short-circuiting for out-of-filter tools. The e2e test exercises all three distinguishable outcomes.

No files require special attention.

Important Files Changed

Filename Overview
src/cli/lock.rs Adds active_unresolved detection so resolution failures on actively-requested tools propagate as errors; converts get_tools_to_lock to return Result. Logic is sound given tvl.requests only holds the winning priority-resolved requests (due to Toolset::merge semantics).
e2e/cli/test_lock_resolution_failure New e2e regression test covering explicit-filter, unfiltered, and cross-tool-filter failure scenarios; correctly validates that the lockfile is not mutated on failure and that a tool excluded from the filter does not surface errors.

Reviews (2): Last reviewed commit: "fix(lock): respect tool filters for unre..." | Re-trigger Greptile

Comment thread src/cli/lock.rs Outdated
Comment thread src/cli/lock.rs Outdated
@github-actions

github-actions Bot commented Jun 23, 2026 •

Copy link
Copy Markdown
Contributor

Hyperfine Performance

mise x -- echo

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.6.12 x -- echo 22.5 ± 1.9 19.0 30.4 1.00
mise x -- echo 24.5 ± 2.1 20.7 45.1 1.09 ± 0.13

mise env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.6.12 env 23.3 ± 1.8 19.4 30.4 1.00
mise env 23.7 ± 1.7 20.0 32.6 1.02 ± 0.11

mise hook-env

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.6.12 hook-env 23.0 ± 1.5 20.5 33.7 1.00
mise hook-env 24.0 ± 1.4 20.0 30.7 1.04 ± 0.09

mise ls

Command Mean [ms] Min [ms] Max [ms] Relative
mise-2026.6.12 ls 19.5 ± 1.3 16.7 26.7 1.00
mise ls 20.1 ± 1.1 16.7 24.5 1.03 ± 0.09

xtasks/test/perf

Command mise-2026.6.12 mise Variance
install (cached) 154ms 155ms +0%
ls (cached) 69ms 71ms -2%
bin-paths (cached) 75ms 77ms -2%
task-ls (cached) 144ms 143ms +0%

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
e2e/cli/test_lock_resolution_failure (1)

48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider more robust quoting for output variable.

The pattern "echo '$output'" works but is fragile—if $output contains single quotes, the command could break. Using double quotes would be safer and more conventional for bash string interpolation.

♻️ Suggested alternative quoting patterns
 output=$(RTX_FAILURE=1 mise lock jq --platform linux-x64 2>&1)
-assert_contains "echo '$output'" "Processing 1 tool(s)"
-assert_not_contains "echo '$output'" "failed to resolve dummy@3.0.0"
+assert_contains "echo \"$output\"" "Processing 1 tool(s)"
+assert_not_contains "echo \"$output\"" "failed to resolve dummy@3.0.0"
🤖 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/cli/test_lock_resolution_failure` around lines 48 - 50, The pattern
`"echo '$output'"` used in both the assert_contains and assert_not_contains
function calls is fragile due to mixed quoting. If the output variable contains
single quotes, the command could break. Update both assert_contains and
assert_not_contains calls to use consistent double-quote wrapping for the output
variable interpolation instead of the current mixed single and double quote
pattern, ensuring robust bash string handling for all edge cases.
🤖 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/cli/test_lock_resolution_failure`:
- Around line 48-50: The pattern `"echo '$output'"` used in both the
assert_contains and assert_not_contains function calls is fragile due to mixed
quoting. If the output variable contains single quotes, the command could break.
Update both assert_contains and assert_not_contains calls to use consistent
double-quote wrapping for the output variable interpolation instead of the
current mixed single and double quote pattern, ensuring robust bash string
handling for all edge cases.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: b8c5cdbc-68ab-4417-893c-8cbb72a465d5

📥 Commits

Reviewing files that changed from the base of the PR and between 8911463 and 7f812f8.

📒 Files selected for processing (2)
  • e2e/cli/test_lock_resolution_failure
  • src/cli/lock.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cli/lock.rs

@jdx
jdx enabled auto-merge (squash) June 23, 2026 18:41
@jdx
jdx merged commit ad92f34 into main Jun 23, 2026
34 checks passed
@jdx
jdx deleted the codex/fail-lock-unresolved-tools branch June 23, 2026 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant