Skip to content

fix(install): tolerate permission errors for read-only system install dirs#9408

Closed
andrewthauer wants to merge 1 commit intojdx:mainfrom
andrewthauer:andrewthauer/fix-system-install-permissions
Closed

fix(install): tolerate permission errors for read-only system install dirs#9408
andrewthauer wants to merge 1 commit intojdx:mainfrom
andrewthauer:andrewthauer/fix-system-install-permissions

Conversation

@andrewthauer
Copy link
Copy Markdown
Contributor

Summary

Fixes a regression that prevents non-root users from running mise install when 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:

Error:
   0: failed to rebuild runtime symlinks
   1: failed rm -rf: /usr/local/share/mise/installs/<tool>/latest
   2: Permission denied (os error 13)

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() sets BackendArg::installs_path to the system path. runtime_symlinks::rebuild then iterates installs_dirs starting with ba.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

  1. runtime_symlinks::rebuild — tolerance is now decided by env::install_path_category(): any non-Local directory gets permission-error tolerance regardless of position. Demoted the message to debug! since it's the expected case when system/shared dirs are owned by another user; users debugging a custom setup can enable MISE_LOG_LEVEL=debug.

  2. 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 verifies mise install of 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-fix
  • mise run test:unit (one pre-existing flaky test in backend::latest_version_tests unrelated to this change — passes in isolation; likely shared Settings state)
  • mise run test:e2e test_install_system_permissions test_reshim_skips_system_installs test_shared_install_dirs — all pass
  • Manually verified in a Docker devcontainer with system-installed tools — install completes silently

🤖 This PR was created with the assistance of Claude Code (claude-opus-4-7).

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 26, 2026

Greptile Summary

Fixes a regression where mise install aborted with a permission error when any tool was system-only (its installs_path pointed to a read-only system dir) by replacing the fragile index-0 heuristic with an explicit install_path_category() check inside a new run_with_permission_tolerance helper, and demotes the log level from warn to debug for expected I/O failures on non-local install dirs. The two new e2e tests cover both the write-failure path and the shim-creation path cleanly.

Confidence Score: 5/5

Safe 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

Filename Overview
src/runtime_symlinks.rs Refactored permission-error tolerance to use install_path_category() instead of index-based logic; extracted helper run_with_permission_tolerance used for both rebuild and migrate_real_dirs.
src/shims.rs Changed error-path log level from warn to debug for non-Local install dirs; shim creation itself is unchanged — system installs still produce shims when readable.
e2e/cli/test_install_system_permissions New e2e regression test: verifies mise install succeeds for a user-level tool when a system-only tool lives in a read-only directory.
e2e/cli/test_reshim_includes_system_installs New e2e regression test: verifies readable system-installed tools still produce user shims after mise reshim; test name contradicts PR description which claimed shims would be skipped.

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
Loading

Reviews (2): Last reviewed commit: "fix(install): tolerate permission errors..." | Re-trigger Greptile

Comment thread src/shims.rs Outdated
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/shims.rs Outdated
Comment on lines +497 to +502
if matches!(
env::install_path_category(&tv.install_path()),
env::InstallPathCategory::System | env::InstallPathCategory::Shared
) {
continue;
}
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.

high

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +37 to +38
# The system-only tool should NOT have a user shim created for it
assert_fail "test -f $MISE_DATA_DIR/shims/dummy"
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.

medium

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".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.
@andrewthauer andrewthauer force-pushed the andrewthauer/fix-system-install-permissions branch from efb0866 to d74f0d0 Compare April 26, 2026 18:51
@jdx
Copy link
Copy Markdown
Owner

jdx commented Apr 26, 2026

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 rebuild_symlinks_in_dir: the "stale runtime dir" branch always fired for healthy runtime symlinks (because from.file_name() = "latest" differs from to.file_name() = "22.5.0" for any latest -> ./<v> symlink, and concrete_installs doesn't include "latest"). This made remove_all run on healthy symlinks on every install — wasted I/O for everyone, and a permission failure when the install dir is read-only.

#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:

  1. The runtime_symlinks change masks the false-positive instead of fixing it. Category-based tolerance still does the bogus I/O — just under a debug! instead of warn!. Means even tools the user owns get pointlessly removed-and-recreated.
  2. The shims::get_desired_shims change skips system/shared installs unconditionally, which (as @greptile-apps and @gemini-code-assist both pointed out) breaks the "root installs once, all users get shims" pattern that the system installs feature was added to support.

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 test_install_system_readonly in #9410 (with an added assertion that no warning appears in output, since the goal here is silent success when nothing needs to change).

This comment was generated by an AI coding assistant.

@jdx jdx closed this Apr 26, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 26, 2026

Makes sense — #9410's fix is cleaner and more principled. The continue on already-correct symlinks means the read-only dir is never touched at all when state is healthy, which is exactly right. No tolerance needed if there's no write attempt in the first place.

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 mise install can't write something it genuinely needs to change, it should fail loudly. That's better than broad permission swallowing.

The test_install_system_readonly test with the "no warning in output" assertion is a good regression gate — it catches both the permission error and the unnecessary I/O. Looks good from here.

jdx added a commit that referenced this pull request Apr 26, 2026
## 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>
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.

2 participants