fix(install): tolerate permission errors for read-only system install dirs#9408
fix(install): tolerate permission errors for read-only system install dirs#9408andrewthauer wants to merge 1 commit intojdx:mainfrom
Conversation
Greptile SummaryFixes a regression where Confidence Score: 5/5Safe to merge; only P2 finding is a stale PR description, not a code defect. The core fix is a targeted, well-tested change with no observable regressions. The single comment is about PR description accuracy rather than a code bug. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rebuild / migrate_real_dirs] --> B{for each installs_dir}
B --> C[run_with_permission_tolerance]
C --> D[rebuild_symlinks_in_dir / migrate_real_dirs_in_dir]
D --> E{Result?}
E -- Ok --> F[Continue]
E -- Err --> G[install_path_category installs_dir]
G --> H{is Local?}
H -- Yes --> I[Propagate error]
H -- No shared/system --> J{is_permission_error?}
J -- Yes --> K[debug! log, return Ok]
J -- No --> I
Reviews (2): Last reviewed commit: "fix(install): tolerate permission errors..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces permission tolerance for runtime symlink updates in system or shared directories to prevent installation failures in read-only environments. It also modifies shim generation to skip system-installed tools. Feedback highlights that skipping these shims is a regression, as users expect shims for all installed tools regardless of installation path. It is recommended to maintain shim generation for system tools and handle potential permission errors by lowering log levels.
| if matches!( | ||
| env::install_path_category(&tv.install_path()), | ||
| env::InstallPathCategory::System | env::InstallPathCategory::Shared | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Skipping shims for tools installed in System or Shared directories is a regression in functionality.
While shims are stored in the user's local data directory, they are generic entry points that should ideally cover all installed tools available to the user, regardless of their installation path. If a tool is only installed in a system directory (e.g., pre-installed in a Docker image), the user still expects mise to provide a shim so the tool is available in their PATH via ~/.local/share/mise/shims.
If the concern is permission errors during binary listing, the existing unwrap_or_else block at line 505 already handles this by warning and returning an empty list. If you wish to suppress that warning for system tools, it would be better to demote the log level to debug! rather than skipping the shim entirely.
There was a problem hiding this comment.
Agreed — reverted in d74f0d0. get_desired_shims keeps creating shims for system/shared installs as before; only the existing unwrap_or_else warning was demoted to debug for non-Local dirs (per your suggestion) so unreadable system dirs don't spam warnings on every reshim. Verified in a Docker devcontainer that shims for system-installed tools (node, claude, gh, etc.) are present in the user's shims dir.
This comment was generated by an AI coding assistant.
| # The system-only tool should NOT have a user shim created for it | ||
| assert_fail "test -f $MISE_DATA_DIR/shims/dummy" |
There was a problem hiding this comment.
This test assertion codifies the behavior of skipping shims for system-installed tools. As noted in the feedback for src/shims.rs, this is likely a regression for users who expect system-installed tools to be available via mise shims.
If the logic in src/shims.rs is reverted to allow shimming system tools, this test should be updated to assert "test -f $MISE_DATA_DIR/shims/dummy".
There was a problem hiding this comment.
Done — test_reshim_skips_system_installs is removed in d74f0d0 since it codified the wrong behavior. Replaced with e2e/cli/test_reshim_includes_system_installs which asserts the OPPOSITE: a system-only tool DOES get a user shim after mise reshim, locking in the behavior you and Greptile flagged.
This comment was generated by an AI coding assistant.
… dirs When tools are installed only in the system directory (e.g. root-installed in a Docker image at build time), `mise install` run by a non-root user fails with a permission error while rebuilding runtime symlinks. The system dir is intentionally read-only for non-root users in this setup. `runtime_symlinks::rebuild` previously decided whether to tolerate permission errors based on position in `installs_dirs` (treating index 0 as the user's local dir). For system-only tools, `ba.installs_path` IS the system path, so index 0 was the system dir — and the error propagated and aborted the install. Tolerance is now decided by `env::install_path_category()`: any non-Local dir gets tolerance, regardless of position. Demoted to debug-level since it's expected when system/shared dirs are owned by another user. Also demotes the `Error listing bin paths` warning in `shims::get_desired_shims` to debug-level for non-Local install dirs, so unreadable system dirs don't spam warnings on every reshim. Shims are still created for system-installed tools whose bin dirs are readable — the typical Docker scenario.
efb0866 to
d74f0d0
Compare
|
Closing in favor of #9410, which fixes the same scenario differently — please take a look. After investigating, the actual root cause turned out to be a bug in #9410 fixes that conditional directly, which means healthy symlinks take the no-op path and the read-only system dir is never touched. The position-based permission tolerance you correctly identified as fragile is no longer needed at all once the underlying false-positive is fixed. The two reasons not to land #9408 as-is:
Thanks for the digging on this — the issue write-up made tracking down the actual condition much faster, and the tests in your PR are great. I pulled the same scenario into This comment was generated by an AI coding assistant. |
|
Makes sense — #9410's fix is cleaner and more principled. The The two concerns I raised during review — the shims regression and silently swallowing errors for dirs the user owns — both go away with this approach. #9410 also benefits from a tighter invariant: if The |
## 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
Fixes a regression that prevents non-root users from running
mise installwhen system-level tools are installed in/usr/local/share/mise/installs(e.g., in Docker images where root installs tools at build time and a non-root user consumes them at runtime).The bug surfaces as:
The trigger is the shared/system install directories feature added in v2026.3.7 (
a63eb16e5, #8581).Root cause
For tools installed only in the system directory,
init_tools()setsBackendArg::installs_pathto the system path.runtime_symlinks::rebuildthen iteratesinstalls_dirsstarting withba.installs_path, applying error tolerance only to entries at index>= 1. For these system-only tools the system path is at index 0, so any permission error during symlink creation propagates and aborts the install.Fix
runtime_symlinks::rebuild— tolerance is now decided byenv::install_path_category(): any non-Localdirectory gets permission-error tolerance regardless of position. Demoted the message todebug!since it's the expected case when system/shared dirs are owned by another user; users debugging a custom setup can enableMISE_LOG_LEVEL=debug.shims::get_desired_shims— skips system/shared versions. User shims are user-local and shouldn't reference tools the current user doesn't own; this also avoids unnecessary I/O on system bin directories during reshim.Tests
e2e/cli/test_install_system_permissions— bootstraps a tool, moves it into a fake system dir to make it system-only, locks down the dir read-only, then verifiesmise installof a different tool succeeds. Verified to fail without the runtime_symlinks fix with the exact same error pattern reported in the wild.e2e/cli/test_reshim_skips_system_installs— verifies system-only tools don't get user shims, while user-installed tools do.Test plan
mise run lint-fixmise run test:unit(one pre-existing flaky test inbackend::latest_version_testsunrelated to this change — passes in isolation; likely sharedSettingsstate)mise run test:e2e test_install_system_permissions test_reshim_skips_system_installs test_shared_install_dirs— all pass🤖 This PR was created with the assistance of Claude Code (claude-opus-4-7).