Repository navigation
fix(dotnet): validate SDK installs with list-sdks - #10691
Conversation
📝 WalkthroughWalkthroughDotnet command handling now includes a reusable async stdout reader, dotnet SDK checks use exact ChangesDotnet SDK verification via
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
Greptile SummaryFixes flaky post-install validation for .NET SDKs by replacing
Confidence Score: 5/5Safe to merge — all changed paths are scoped to .NET post-install validation and the new The core fix (switching from No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "fix(dotnet): validate SDK installs with ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
6bd6dae to
f73ef0e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/plugins/core/dotnet.rs (2)
316-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a test for
sdk_list_displaywith non-empty input.Only the empty-input placeholder case is tested; the join-with-newline branch for non-empty lines is not directly covered.
✅ Suggested additional test
+ #[test] + fn sdk_list_display_joins_lines() { + let lines = vec!["8.0.128 [/path]".to_string(), "10.0.301 [/path2]".to_string()]; + assert_eq!(sdk_list_display(&lines), "8.0.128 [/path]\n10.0.301 [/path2]"); + }🤖 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/dotnet.rs` around lines 316 - 322, Add a test that exercises sdk_list_display with a non-empty slice so the join-with-newline branch is covered. Locate the helper sdk_list_display in dotnet.rs and add a unit test that passes multiple SDK strings and asserts the returned value is the same items joined by newline, complementing the existing empty-input test.
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment references the old
--versioncheck.The comment still explains the skip rationale in terms of
dotnet --version, but the function below no longer calls--version— it now uses--list-sdks. Worth updating for clarity since a future reader may be confused about which command the skip applies to.✏️ Suggested comment update
- // Skip version check for runtime-only installs — `dotnet --version` exits non-zero without an SDK + // Skip SDK check for runtime-only installs — no SDK is expected to be present🤖 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/dotnet.rs` at line 72, Update the stale inline comment in the runtime-only install check near the dotnet version probing logic so it matches the current `--list-sdks` behavior instead of `dotnet --version`. Keep the comment in the same area of the `dotnet.rs` code that handles the skip path, and revise the wording to explain that runtime-only installs are skipped because the SDK list check can fail when no SDK is present.
🤖 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 `@src/plugins/core/dotnet.rs`:
- Around line 316-322: Add a test that exercises sdk_list_display with a
non-empty slice so the join-with-newline branch is covered. Locate the helper
sdk_list_display in dotnet.rs and add a unit test that passes multiple SDK
strings and asserts the returned value is the same items joined by newline,
complementing the existing empty-input test.
- Line 72: Update the stale inline comment in the runtime-only install check
near the dotnet version probing logic so it matches the current `--list-sdks`
behavior instead of `dotnet --version`. Keep the comment in the same area of the
`dotnet.rs` code that handles the skip path, and revise the wording to explain
that runtime-only installs are skipped because the SDK list check can fail when
no SDK is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c070eba-a18c-43f4-a735-7197797cd858
📒 Files selected for processing (2)
e2e/core/test_dotnetsrc/plugins/core/dotnet.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/core/test_dotnet
d005dca to
ec6f721
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cmd.rs`:
- Around line 927-961: The read() path in cmd::Cmd currently spawns a child
process but never registers its PID in RUNNING_PIDS, so cleanup paths like
kill_all() cannot see or terminate it. Update Cmd::read (and its
spawn_async_with_etxtbsy_retry / wait_with_output flow if needed) to add the
spawned child PID to RUNNING_PIDS right after spawn and ensure it is removed
when the process exits or on error/timeout, matching the tracking used by other
execution paths.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d099bbf-a3f4-4a0c-b38d-7624e9c49a5c
📒 Files selected for processing (3)
e2e/core/test_dotnetsrc/cmd.rssrc/plugins/core/dotnet.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/core/test_dotnet
ec6f721 to
d3cdcf2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d3cdcf2. Configure here.
d3cdcf2 to
db64320
Compare
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.14 x -- echo |
32.8 ± 3.1 | 25.4 | 45.2 | 1.00 |
mise x -- echo |
33.4 ± 3.3 | 25.0 | 46.8 | 1.02 ± 0.14 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.14 env |
29.2 ± 2.6 | 22.9 | 36.7 | 1.00 |
mise env |
31.9 ± 2.9 | 24.9 | 45.6 | 1.09 ± 0.14 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.14 hook-env |
30.9 ± 2.8 | 22.9 | 39.9 | 1.00 |
mise hook-env |
33.1 ± 3.4 | 25.7 | 44.1 | 1.07 ± 0.15 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.6.14 ls |
24.8 ± 2.1 | 19.4 | 31.7 | 1.00 |
mise ls |
26.8 ± 2.4 | 21.2 | 35.0 | 1.08 ± 0.13 |
xtasks/test/perf
| Command | mise-2026.6.14 | mise | Variance |
|---|---|---|---|
| install (cached) | 184ms | 182ms | +1% |
| ls (cached) | 92ms | 98ms | -6% |
| bin-paths (cached) | 99ms | 99ms | +0% |
| task-ls (cached) | 183ms | 188ms | -2% |
Summary
dotnet --list-sdksinstead ofdotnet --versionglobal.jsonselects an unavailable SDK while installing an older SDKContext
dotnet --versionis affected byglobal.jsonand SDK roll-forward rules, so it can fail even afterdotnet-installsuccessfully installs the requested SDK.dotnet --list-sdkschecks the installed SDK inventory directly, which matches the sharedDOTNET_ROOTmodel introduced for side-by-side SDK installs.Addresses discussion #10688.
Tests
cargo fmt --checkcargo test plugins::core::dotnet::testsCMAKE="$(mise x cmake@latest -- command -v cmake)" mise run test:e2e test_dotnetNote: the e2e run also matched
backend/test_dotnet, which skipped because systemdotnetis not installed;core/test_dotnetpassed. The run emitted the existing slow-test warning forcore/test_dotnettaking over 20s.This PR was generated by an AI coding assistant.
Note
Medium Risk
Touches shared command execution (
CmdLineRunner::read) and .NET install verification; behavior change is targeted but affects all SDK post-install checks.Overview
Fixes false failures when a repo’s
global.jsonpins an SDK that isn’t installed: post-install verification no longer usesdotnet --version(selection/roll-forward) and instead checksdotnet --list-sdksfor an exact match on the requested SDK version, with clearer errors that include the captured list output.CmdLineRunnergains asyncread()so callers can capture stdout while still piping (including whenMISE_RAW/ raw mode would otherwise inherit stdio); failures replay captured stderr viareplay_captured_stderr, andRunningPidGuardtracks PIDs for cleanup duringread().E2E
core/test_dotnetnow installs SDK 8.0.408 under aglobal.jsonthat demands 10.0.301 with roll-forward disabled, asserting--list-sdksstill shows the installed SDK.Reviewed by Cursor Bugbot for commit db64320. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
--list-sdksoutput when the requested SDK isn’t found.Tests
global.json.