fix(install): create runtime symlinks in system/shared install directories#8722
fix(install): create runtime symlinks in system/shared install directories#8722
Conversation
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 where system-wide installations of tools were not correctly generating runtime symlinks, leading to potential usability issues for users relying on shared or system-level tool management. The changes ensure that Highlights
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. Footnotes
|
Greptile SummaryThis PR fixes Key changes:
The all three concerns from prior reviews — permission propagation, Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rebuild called] --> B[for each backend]
B --> C[Collect installs dirs\nuser dir + shared_install_dirs]
C --> D[Process user dir\nrebuild_symlinks_in_dir?]
D -->|error| E[propagate error]
D -->|ok| F[Process each shared/system dir\nrebuild_symlinks_in_dir?]
F -->|permission error| G[warn + skip]
F -->|other error| H[propagate error]
F -->|ok| I[continue]
subgraph rebuild_symlinks_in_dir
J[list_symlinks_for_dir\nversions from dir scan] --> K[for each symlink\ncreate/update]
K --> L[remove_missing_symlinks_in_dir\nclean dangling symlinks]
end
D & F --> rebuild_symlinks_in_dir
Reviews (3): Last reviewed commit: "fix: restore semver sort for installed v..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
The pull request modifies the runtime symlink creation logic to support shared install directories. The changes involve collecting install directories from user and shared locations, iterating through them to create symlinks, and removing missing symlinks. Review feedback suggests verifying that file::dir_subdirs provides equivalent information to backend.list_installed_versions(), renaming the ba variable to backend_abstraction for better clarity, using a HashSet for installs_dirs to improve lookup complexity, handling the None case explicitly to avoid removing potentially valid symlinks, and handling the None case explicitly and ensuring that invalid versions are sorted appropriately.
I am having trouble creating individual review comments. Click here to see my feedback.
src/runtime_symlinks.rs (75-80)
The original installed_versions function used backend.list_installed_versions(). The new installed_versions_in_dir function replaces this with file::dir_subdirs(installs_dir). It's crucial to ensure that file::dir_subdirs provides the same information as backend.list_installed_versions() in all cases. If backend.list_installed_versions() performs additional filtering or processing, the new function might not be equivalent, potentially leading to incorrect symlink creation or removal.
Suggestion: Verify that file::dir_subdirs provides equivalent information to backend.list_installed_versions() or reimplement the filtering logic.
src/runtime_symlinks.rs (108)
The remove_missing_symlinks function now calls remove_missing_symlinks_in_dir with backend.ba().installs_path. This change ensures that missing symlinks are removed from the user install directory. However, it's important to ensure that this function correctly handles symlinks in all install directories (user + shared/system). If the logic for removing missing symlinks is different for shared/system directories, this change could introduce a bug.
Suggestion: Verify that remove_missing_symlinks_in_dir correctly handles symlinks in all install directories.
src/runtime_symlinks.rs (17)
This change introduces a new variable ba without a clear explanation of its purpose. While it's derived from backend.ba(), adding a comment clarifying what ba represents (e.g., "backend abstraction") would improve readability.
Consider renaming ba to backend_abstraction for better clarity.
let backend_abstraction = backend.ba();
src/runtime_symlinks.rs (23-25)
This check prevents adding duplicate install directories, which is good. However, the contains method on Vec has O(n) complexity. If the number of shared install directories grows, this could become a performance bottleneck. Consider using a HashSet for installs_dirs to improve the lookup complexity to O(1).
Suggestion: Use HashSet for installs_dirs.
use std::collections::HashSet;
// Collect all install directories for this backend: user dir + shared/system dirs
let mut installs_dirs: HashSet<_> = vec![ba.installs_path.clone()].into_iter().collect();
let tool_dir_name = ba.tool_dir_name();
for shared_dir in env::shared_install_dirs() {
let dir = shared_dir.join(&tool_dir_name);
if dir.is_dir() && !installs_dirs.contains(&dir) {
installs_dirs.insert(dir);
}
}src/runtime_symlinks.rs (33-34)
The condition file::resolve_symlink(&from)?.unwrap_or_default() != to checks if an existing symlink points to the correct target. However, unwrap_or_default() will return a default PathBuf if resolve_symlink returns None (i.e., the symlink doesn't exist or is broken). This could lead to unnecessary symlink removals if a broken symlink already exists.
Suggestion: Handle the None case explicitly to avoid removing potentially valid symlinks.
if is_runtime_symlink(&from) {
match file::resolve_symlink(&from)? {
Some(existing_target) if existing_target != to => {
trace!("Removing existing symlink: {}", from.display());
file::remove_file(&from)?;
}
_ => continue,
}
} else {src/runtime_symlinks.rs (103)
The sorted_by_cached_key function sorts the versions. This sorting is important for determining the latest symlink. However, the sorting key (Versioning::new(v), v.to_string()) might not be the most efficient. If Versioning::new(v) fails (returns None), it defaults to Versioning::new(0.0.0), which could lead to incorrect sorting if some versions are invalid.
Suggestion: Handle the None case explicitly and ensure that invalid versions are sorted appropriately (e.g., by placing them at the end of the list or logging an error).
.sorted_by_cached_key(|v| {
Versioning::new(v).map_or_else(
|| {
eprintln!("Invalid version found: {}", v);
Versioning::new("0.0.0").unwrap()
},
|version| version
)
})
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.3.12 x -- echo |
24.1 ± 0.4 | 23.4 | 26.6 | 1.00 ± 0.02 |
mise x -- echo |
24.1 ± 0.4 | 23.4 | 26.8 | 1.00 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.3.12 env |
23.8 ± 0.8 | 22.8 | 29.5 | 1.00 ± 0.04 |
mise env |
23.6 ± 0.3 | 23.0 | 25.5 | 1.00 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.3.12 hook-env |
24.2 ± 0.3 | 23.6 | 25.8 | 1.00 |
mise hook-env |
24.5 ± 0.4 | 23.8 | 26.5 | 1.01 ± 0.02 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.3.12 ls |
23.8 ± 0.4 | 22.9 | 29.5 | 1.00 |
mise ls |
23.9 ± 0.3 | 23.0 | 25.3 | 1.00 ± 0.02 |
xtasks/test/perf
| Command | mise-2026.3.12 | mise | Variance |
|---|---|---|---|
| install (cached) | 152ms | 159ms | -4% |
| ls (cached) | 84ms | 84ms | +0% |
| bin-paths (cached) | 87ms | 86ms | +1% |
| task-ls (cached) | 848ms | 833ms | +1% |
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.
…ories When tools are installed with `--system`, runtime symlinks (like `latest`) were only created in the user installs directory, not the system directory. This caused errors when the user dir didn't have the tool, or broken symlinks when versions existed in different directories. The fix makes `rebuild()` scan all install directories (user + shared/system) and create symlinks in each one based on the versions actually present there. Fixes #8596 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…build Gracefully handle permission errors when updating symlinks in shared/system install directories (e.g., root-owned dirs). Warns and continues instead of aborting the entire rebuild. Also removes redundant sort in installed_versions_in_dir. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Restore the semantic version sort in installed_versions_in_dir — it determines which version wins for `latest` via insertion order, so lexicographic ordering from BTreeSet would produce wrong symlinks for multi-digit version components (e.g. "8.0.0" after "18.0.0"). Also widen the permission error check to handle ReadOnlyFilesystem (EROFS), which occurs on read-only mounted shared dirs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
618d71b to
34280bf
Compare
### 🐛 Bug Fixes - **(env)** improve hook-env watch_files tracking and early-exits by @rpendleton in [#8716](#8716) - **(install)** create runtime symlinks in system/shared install directories by @jdx in [#8722](#8722) - apply --silent flag to global settings to suppress output by @nkakouros in [#8720](#8720) ### 📦️ Dependency Updates - ignore RUSTSEC-2026-0066 astral-tokio-tar advisory by @jdx in [#8723](#8723) ### 📦 Registry - add acli by @ggoggam in [#8721](#8721) ### New Contributors - @rpendleton made their first contribution in [#8716](#8716) - @ggoggam made their first contribution in [#8721](#8721) ## 📦 Aqua Registry Updates #### Updated Packages (1) - [`astral-sh/ty`](https://github.com/astral-sh/ty)
## Summary Fixes the `mise install` regression where a non-root user gets `Permission denied` while rebuilding runtime symlinks against a read-only system installs dir (typical Docker pattern: root populates `/usr/local/share/mise/installs/` at build time, non-root user runs `mise install` at runtime). Reported in [#8596](#8596) and addressed by [#9408](#9408) — this PR is an alternative. ## Root cause `rebuild_symlinks_in_dir` always called `remove_all` + `make_symlink_or_file` on every existing runtime symlink, because the "stale runtime dir" branch fired for symlinks too: ```rust if from.exists() { if is_runtime_symlink(&from) && file::resolve_symlink(&from)?.unwrap_or_default() != to { // Branch 1 — only fires when the symlink target is wrong ... } else if from.file_name() != to.file_name() && !concrete_installs.contains(&from_name) { // Branch 2 — fires for ANY symlink because file_name("latest") != file_name("22.5.0") file::remove_all(&from)?; } ... } ``` For a perfectly healthy `latest -> ./22.5.0`: - Branch 1 fails (`target == to`). - Branch 2 enters because `from.file_name()` (`"latest"`) differs from `to.file_name()` (`"22.5.0"`) and `concrete_installs` doesn't include `"latest"`. - `remove_all` then runs on a healthy symlink — wasted I/O on every install in the common case, hard failure when the dir is read-only. ## Fix - Branch 1 now handles all runtime-symlink cases: rewrite if target differs, otherwise `continue`. - Branch 2 only fires for non-symlink stale dirs (its actual purpose — cleanup for the 2026.4 regression that created real `latest/` directories). - Drop the position-based permission tolerance added in #8722. With healthy symlinks taking the no-op path, read-only system dirs aren't touched at all. If a write IS genuinely required and can't happen, fail loudly so the user knows to fix permissions rather than silently ending up with a stale `latest` after `mise install --system`. ## Why not [#9408](#9408) That PR replaces position-based tolerance with category-based tolerance (any non-`Local` dir gets permission errors swallowed) and silently skips system/shared installs from user shims. Both: - Mask the real bug (the false-positive `remove_all` above) instead of fixing it. Even with category-based tolerance the install does I/O it shouldn't, just hidden under a `debug!`. - Trade one form of silent behavior for another. Reviewers (greptile, gemini) flagged the shims change as a regression for the "root installs once, all users get shims" pattern. - Don't match the maintainer's stated policy: if `mise install` doesn't touch system tools, it should work fine; if it does and the dir isn't writable, it should fail loudly. This PR achieves both via a delta-based no-op rather than tolerance. ## Tests - `e2e/cli/test_install_system_readonly` — sets up `tiny@1.0.0` in user dir, `tiny@2.1.0` in a fake system dir, `chmod -R a-w` the system dir, then runs `mise install --force tiny@1.0.0` and `mise install dummy@1.0.0`. Asserts both succeed AND that the `skipping symlink update` / `Permission denied` / `failed rm` warnings do NOT appear in output. Verified to fail before this fix (with `WARN skipping symlink update for ...: failed rm -rf ... Permission denied`) and pass after. ## Test plan - [x] `cargo test --bin mise` — 774 pass - [x] `mise run lint-fix` — clean - [x] `mise run test:e2e test_install_system_readonly` — pass - [x] `mise run test:e2e test_install_system test_shared_install_dirs test_install_before_ignores_stale_latest_dirs test_upgrade_latest_stale test_reshim_with_shims_on_path test_uninstall test_install_before test_install_short_ignores_full_backend_config test_install_dry_run test_install_concurrent_via_exec` — all pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes runtime symlink rebuild behavior across both user and shared/system install dirs, which could affect installs/reshims if edge cases exist. It also removes permission-error swallowing for shared dirs, so genuine mismatches in read-only system installs will now fail loudly. > > **Overview** > Prevents `mise install` from rewriting existing runtime symlinks when they already point at the desired target, avoiding unnecessary deletes/recreates that previously caused `Permission denied` failures on read-only system install directories. > > Refactors shared/system install-dir enumeration into `install_dirs_for` and removes the prior "skip on permission error" behavior; shared/system dirs are now treated like user dirs, but the rebuild path should be a no-op unless a change is actually needed. Adds an E2E regression test (`test_install_system_readonly`) that simulates a read-only system installs dir and asserts installs succeed without system-dir warnings. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit acf5798. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Summary
mise install --systemfailing to create runtime symlinks (latest, partial version aliases) in the system/shared installs directoryruntime_symlinks::rebuild()now scans all install directories (user + shared/system) and creates symlinks in each one based on the versions actually present there, instead of only usingbackend.ba().installs_pathFixes #8596
Test plan
test_install_systempasses with fix, fails without itmise run lintpassescargo checkpasses--system— the fix handles the case wherearg_to_backenddiscards the systeminstalls_pathfor core plugins🤖 Generated with Claude Code
Note
Medium Risk
Touches runtime symlink generation/removal across multiple install roots and changes error-handling to tolerate permission issues in system/shared dirs, which could affect which versions
latest/partial aliases resolve to.Overview
Fixes runtime symlink creation for
--system/shared installs by rebuildinglatestand partial-version symlinks per install directory (user plusenv::shared_install_dirs()), based only on the versions physically present in each directory.Refactors
runtime_symlinks::rebuild()into per-directory helpers, adds permission/read-only tolerance when updating shared/system locations, and includes a new e2e regression test ensuring user and system install roots get correct, non-brokenlatestsymlinks.Written by Cursor Bugbot for commit 34280bf. This will update automatically on new commits. Configure here.