-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(activate): preserve ordering of paths appended after mise activate #7919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Test that paths appended to PATH after `mise activate` are not reordered | ||
| # to the front. Paths added after activation that appear at the end of PATH | ||
| # should stay at the end after hook-env runs. | ||
| # | ||
| # Regression test for https://github.com/jdx/mise/discussions/7694 | ||
| # where `mise activate` would move post-activation appended paths to the | ||
| # beginning of PATH, changing the intended priority order. | ||
|
|
||
| # Create test directories | ||
| mkdir -p "$HOME/system/bin" | ||
| mkdir -p "$HOME/appended/bin" | ||
|
|
||
| # Create executables in both dirs with the same name | ||
| cat >"$HOME/system/bin/test-priority" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| echo "system version" | ||
| EOF | ||
|
|
||
| cat >"$HOME/appended/bin/test-priority" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| echo "appended version" | ||
| EOF | ||
|
|
||
| chmod +x "$HOME/system/bin/test-priority" | ||
| chmod +x "$HOME/appended/bin/test-priority" | ||
|
|
||
| # Set up initial PATH with system/bin | ||
| export PATH="$HOME/system/bin:$PATH" | ||
|
|
||
| # Activate mise (this captures __MISE_ORIG_PATH) | ||
| eval "$(mise activate bash)" | ||
|
|
||
| # Simulate a user appending a path AFTER mise activation (e.g., path+=$FOO/bin in .zshrc) | ||
| export PATH="$PATH:$HOME/appended/bin" | ||
|
|
||
| # Create an empty mise.toml so hook-env has something to work with | ||
| cat >mise.toml <<'EOF' | ||
| EOF | ||
|
|
||
| # Run hook-env to reconstruct PATH | ||
| eval "$(mise hook-env)" | ||
|
|
||
| echo "DEBUG: Final PATH=$PATH" | ||
| echo "DEBUG: __MISE_ORIG_PATH=${__MISE_ORIG_PATH:-not set}" | ||
|
|
||
| # The system version should still win because system/bin was earlier in PATH | ||
| # and appended/bin was added at the end | ||
| assert_contains "test-priority" "system version" | ||
|
|
||
| # Verify appended/bin is AFTER system/bin in PATH (not moved to front) | ||
| SYSTEM_POS=$(echo "$PATH" | tr ':' '\n' | grep -n "system/bin" | head -1 | cut -d: -f1) | ||
| APPENDED_POS=$(echo "$PATH" | tr ':' '\n' | grep -n "appended/bin" | head -1 | cut -d: -f1) | ||
|
|
||
| echo "DEBUG: system/bin at position $SYSTEM_POS, appended/bin at position $APPENDED_POS" | ||
|
|
||
| if [[ $APPENDED_POS -gt $SYSTEM_POS ]]; then | ||
| echo "SUCCESS: appended/bin stays after system/bin in PATH" | ||
| else | ||
| echo "FAIL: appended/bin ($APPENDED_POS) was moved before system/bin ($SYSTEM_POS)" | ||
| exit 1 | ||
| fi | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -230,7 +230,7 @@ impl HookEnv { | |||||||||||||||||||||||
| let full = join_paths(&*env::PATH)?.to_string_lossy().to_string(); | ||||||||||||||||||||||||
| let current_paths: Vec<PathBuf> = split_paths(&full).collect(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let (pre, post) = match &*env::__MISE_ORIG_PATH { | ||||||||||||||||||||||||
| let (pre, post, post_user) = match &*env::__MISE_ORIG_PATH { | ||||||||||||||||||||||||
| Some(orig_path) if !Settings::get().activate_aggressive => { | ||||||||||||||||||||||||
| let orig_paths: Vec<PathBuf> = split_paths(orig_path).collect(); | ||||||||||||||||||||||||
| let orig_set: HashSet<_> = orig_paths.iter().collect(); | ||||||||||||||||||||||||
|
|
@@ -239,12 +239,17 @@ impl HookEnv { | |||||||||||||||||||||||
| // to_remove contains ALL paths that mise added (tool installs, config paths, etc.) | ||||||||||||||||||||||||
| let mise_paths_set: HashSet<_> = to_remove.iter().collect(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Find paths in current that are not in original and not mise-managed | ||||||||||||||||||||||||
| // These are genuine user additions after mise activation. | ||||||||||||||||||||||||
| // Find paths in current that are not in original and not mise-managed. | ||||||||||||||||||||||||
| // Split them into "pre" (before the original PATH entries) and "post_user" | ||||||||||||||||||||||||
| // (after the original PATH entries) to preserve their intended position. | ||||||||||||||||||||||||
| // This prevents paths appended after `mise activate` in shell rc from | ||||||||||||||||||||||||
| // being moved to the front of PATH. | ||||||||||||||||||||||||
| let mut pre = Vec::new(); | ||||||||||||||||||||||||
| let mut post_user = Vec::new(); | ||||||||||||||||||||||||
| let mut seen_orig = false; | ||||||||||||||||||||||||
| for path in ¤t_paths { | ||||||||||||||||||||||||
| // Skip if in original PATH | ||||||||||||||||||||||||
| if orig_set.contains(path) { | ||||||||||||||||||||||||
| seen_orig = true; | ||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -253,14 +258,18 @@ impl HookEnv { | |||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // This is a genuine user addition | ||||||||||||||||||||||||
| pre.push(path.clone()); | ||||||||||||||||||||||||
| // Place in pre or post_user based on position relative to original PATH | ||||||||||||||||||||||||
| if seen_orig { | ||||||||||||||||||||||||
| post_user.push(path.clone()); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| pre.push(path.clone()); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Use the original PATH directly as "post" to ensure it's preserved exactly | ||||||||||||||||||||||||
| (pre, orig_paths) | ||||||||||||||||||||||||
| (pre, orig_paths, post_user) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| _ => (vec![], current_paths), | ||||||||||||||||||||||||
| _ => (vec![], current_paths, vec![]), | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Filter out tool paths that are already in the original PATH (post) or | ||||||||||||||||||||||||
|
|
@@ -280,9 +289,12 @@ impl HookEnv { | |||||||||||||||||||||||
| // and other path variants that refer to the same filesystem location. | ||||||||||||||||||||||||
| let post_canonical: HashSet<PathBuf> = | ||||||||||||||||||||||||
| post.iter().filter_map(|p| p.canonicalize().ok()).collect(); | ||||||||||||||||||||||||
| let pre_set: HashSet<_> = pre.iter().collect(); | ||||||||||||||||||||||||
| let pre_canonical: HashSet<PathBuf> = | ||||||||||||||||||||||||
| pre.iter().filter_map(|p| p.canonicalize().ok()).collect(); | ||||||||||||||||||||||||
| let user_additions_set: HashSet<_> = pre.iter().chain(post_user.iter()).collect(); | ||||||||||||||||||||||||
| let user_additions_canonical: HashSet<PathBuf> = pre | ||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||
| .chain(post_user.iter()) | ||||||||||||||||||||||||
| .filter_map(|p| p.canonicalize().ok()) | ||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||
|
Comment on lines
+292
to
+297
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid creating the
Suggested change
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let tool_paths_filtered: Vec<PathBuf> = tool_paths | ||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||
|
|
@@ -301,12 +313,12 @@ impl HookEnv { | |||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Also filter against pre (user additions) to avoid duplicates | ||||||||||||||||||||||||
| if pre_set.contains(p) { | ||||||||||||||||||||||||
| // Also filter against user additions (pre + post_user) to avoid duplicates | ||||||||||||||||||||||||
| if user_additions_set.contains(p) { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if let Ok(canonical) = p.canonicalize() | ||||||||||||||||||||||||
| && pre_canonical.contains(&canonical) | ||||||||||||||||||||||||
| && user_additions_canonical.contains(&canonical) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
@@ -316,23 +328,19 @@ impl HookEnv { | |||||||||||||||||||||||
| .cloned() | ||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Filter user_paths against pre (user manual additions) to avoid duplicates | ||||||||||||||||||||||||
| // Filter user_paths against user additions (pre + post_user) to avoid duplicates | ||||||||||||||||||||||||
| // when users manually add paths after mise activation. | ||||||||||||||||||||||||
| // IMPORTANT: Do NOT filter against post (__MISE_ORIG_PATH) - this would break | ||||||||||||||||||||||||
| // the intended behavior where user-configured paths should take precedence | ||||||||||||||||||||||||
| // even if they already exist in the original PATH. | ||||||||||||||||||||||||
| let pre_set: HashSet<_> = pre.iter().collect(); | ||||||||||||||||||||||||
| let pre_canonical: HashSet<PathBuf> = | ||||||||||||||||||||||||
| pre.iter().filter_map(|p| p.canonicalize().ok()).collect(); | ||||||||||||||||||||||||
| let user_paths_filtered: Vec<PathBuf> = user_paths | ||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||
| .filter(|p| { | ||||||||||||||||||||||||
| // Filter against pre only (user manual additions after mise activation) | ||||||||||||||||||||||||
| if pre_set.contains(p) { | ||||||||||||||||||||||||
| if user_additions_set.contains(p) { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if let Ok(canonical) = p.canonicalize() | ||||||||||||||||||||||||
| && pre_canonical.contains(&canonical) | ||||||||||||||||||||||||
| && user_additions_canonical.contains(&canonical) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
@@ -342,12 +350,13 @@ impl HookEnv { | |||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Combine paths in the correct order: | ||||||||||||||||||||||||
| // pre (user shell additions) -> user_paths (from config, filtered against pre) -> tool_paths (filtered) -> post (original PATH) | ||||||||||||||||||||||||
| // pre (user shell prepends) -> user_paths (from config) -> tool_paths -> post (original PATH) -> post_user (user shell appends) | ||||||||||||||||||||||||
| let new_path = join_paths( | ||||||||||||||||||||||||
| pre.iter() | ||||||||||||||||||||||||
| .chain(user_paths_filtered.iter()) | ||||||||||||||||||||||||
| .chain(tool_paths_filtered.iter()) | ||||||||||||||||||||||||
| .chain(post.iter()), | ||||||||||||||||||||||||
| .chain(post.iter()) | ||||||||||||||||||||||||
| .chain(post_user.iter()), | ||||||||||||||||||||||||
| )? | ||||||||||||||||||||||||
| .to_string_lossy() | ||||||||||||||||||||||||
| .into_owned(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
grepcommand could be made more robust by matching the full path with-F(fixed string) instead of a substring. This prevents potential false positives if another directory inPATHcontainssystem/binorappended/bin.