fix(activate): don't reorder PATH for the mise dir in --shims mode#10394
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSwitches shims activation to use guarded ChangesExecutable Directory Prepend Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Greptile SummaryThis PR fixes a PATH reordering bug in
Confidence Score: 5/5Safe to merge — the change is a one-line switch to an already-present guarded helper with a targeted regression test covering the fixed scenario. The diff is minimal: one call-site change in No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(activate): don't reorder PATH for th..." | Re-trigger Greptile |
| if let Some(p) = self.prepend_path(exe_dir) { | ||
| prelude.push(p); | ||
| } |
There was a problem hiding this comment.
Fish: exe_dir switches from
MovePrependEnv to PrependEnv
Before this PR, activate_shims() called shims_prepend_path(shell, exe_dir), which emitted MovePrependEnv for fish (using fish's native path dedup so re-sourcing is idempotent). The new prepend_path(exe_dir) always emits PrependEnv regardless of shell. For fish users where exe_dir is not already in PATH (e.g. a custom ~/.local/bin install), re-sourcing the config after a system-level rc resets PATH could duplicate exe_dir, because PrependEnv is not natively idempotent under fish. The guard !is_dir_in_path makes this safe for the common re-source case, but it breaks in the "PATH reset between sources" scenario that MovePrependEnv handles natively. Consider emitting MovePrependEnv(exe_dir) when shell.supports_move_path() is true, falling back to the guarded PrependEnv otherwise.
There was a problem hiding this comment.
Thanks, but I don''t think this one applies here. On fish, PrependEnv renders as fish_add_path --global --path (src/shell/fish.rs), and fish_add_path is idempotent — it won''t add a path that''s already present, so re-sourcing (even after a PATH reset) doesn''t duplicate exe_dir. The only behavioural difference vs MovePrependEnv (fish_add_path --move) is whether an already-present entry gets moved to the front.
Moving it is exactly what this PR is fixing: for a deb install exe_dir is /usr/bin, which is already in PATH, so MovePrependEnv would move /usr/bin ahead of /usr/local/bin — reintroducing #10264 on fish. The guarded prepend_path leaves an already-present exe_dir untouched on every shell (and fish_add_path --path adds it idempotently when it is genuinely missing), matching what the non-shims activate() path already does for exe_dir. So I''m keeping prepend_path here.
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 `@e2e/shell/test_shims_activate_prepend`:
- Around line 23-29: The test currently masks failures by appending "|| true" to
the activation command that sets out, so replace that pattern: run the
activation command capturing its stdout/stderr into out (keep env
PATH="$EXE_DIR:..." "$MISE_BIN" activate bash --shims 2>&1), then immediately
check the command's exit status and fail the test if it's non-zero (print out
the captured out for diagnostics and exit non‑zero) before continuing with
assert_contains_text and the path_lines checks; this removes the "|| true"
suppression while preserving captured output for assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d9745a8-d5bf-4ba3-989b-458787537dae
📒 Files selected for processing (2)
e2e/shell/test_shims_activate_prependsrc/cli/activate.rs
46296bd to
efbe835
Compare
`mise activate <shell> --shims` prepended both the shims dir and the directory containing the mise executable unconditionally (PR jdx#8757 dropped the "already in PATH" guard so the shims dir reliably moves to the front on re-source). For a deb/rpm install the mise binary lives at /usr/bin, so activation emitted `export PATH="/usr/bin:$PATH"`, duplicating /usr/bin and moving it ahead of /usr/local/bin -- changing command resolution (jdx#10264). Keep always-prepending the shims dir (preserving jdx#8757's re-source behavior), but emit the mise executable's own dir through the guarded prepend_path, which skips dirs already in PATH. The exe dir only needs to be present so `mise` is callable, never at the front. For a deb install, activation now emits only the shims line. Addresses discussion jdx#10264. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
efbe835 to
5498a86
Compare
Problem
On a deb/rpm install (mise at
/usr/bin/mise),mise activate bash --shimsemits:The first line duplicates
/usr/binand moves it ahead of/usr/local/bin, changing command resolution — locally/admin-installed binaries in/usr/local/binno longer take precedence. Reported in #10264.Root cause
activate_shims()prepends both the shims dir and the directory containing the mise executable. PR #8757 made--shimsactivation always prepend (dropping the "already in PATH" guard) so the shims dir reliably moves to the front when shell config is re-sourced (e.g. VS Code terminals). Butactivate_shims()routes both the shims dir and the mise exe dir through that same unconditional helper, so the exe dir lost the guard too. For a deb install the exe dir is/usr/bin, which is already in PATH — so it gets re-prepended and reordered.Fix
Keep always-prepending the shims dir (preserving #8757''s re-source behavior), but emit the mise executable''s own dir through the guarded
prepend_path, which skips directories already in PATH. The exe dir only needs to be present somiseis callable — never at the front.For a deb install, activation now emits only:
and never reorders existing PATH entries.
Testing
e2e/shell/test_shims_activate_prepend: when the mise executable''s dir is already in PATH, exactly oneexport PATH=line (the shims dir) is emitted — the exe dir is not re-prepended. The existing assertions (shims dir is always prepended, with or without it already in PATH) still hold.cargo fmt --all -- --checkpasses.Addresses #10264
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests