fix(activate): always prepend shims to front of PATH in --shims mode#8757
fix(activate): always prepend shims to front of PATH in --shims mode#8757
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 resolves an issue where the 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
|
There was a problem hiding this comment.
Code Review
This pull request modifies the mise activate --shims command to always prepend the shims directory to the PATH environment variable, even if it's already present, which may result in duplicate entries for shells without native path deduplication. A new end-to-end test was added to verify this behavior. The review suggests improving the new test to explicitly assert that the shims directory is prepended to the PATH, rather than just checking for its presence.
| # Shims already in PATH: should still be prepended | ||
| assert_contains "env PATH='$CUSTOM_PATH' $MISE_BIN activate bash --shims" "$SHIMS_DIR" | ||
|
|
||
| # Shims not in PATH: should also be prepended | ||
| assert_contains "env PATH='/some/other/bin:/usr/bin' $MISE_BIN activate bash --shims" "$SHIMS_DIR" |
There was a problem hiding this comment.
The current assertions only check that the shims directory path is present in the output of the activate command. They don't verify that the path is actually prepended to the PATH environment variable, which is the core of the fix this PR introduces.
To make the test more robust and prevent regressions, I suggest evaluating the activation script and then asserting on the state of the PATH variable. This will ensure the shims directory is correctly placed at the beginning of the PATH.
For example, you could use a regex match to check if PATH starts with the shims directory.
# Shims already in PATH: should still be prepended
output=$(env PATH="$CUSTOM_PATH" "$MISE_BIN" activate bash --shims)
eval "$output"
assert_match "$PATH" "^$SHIMS_DIR:"
# Shims not in PATH: should also be prepended
output=$(env PATH="/some/other/bin:/usr/bin" "$MISE_BIN" activate bash --shims)
eval "$output"
assert_match "$PATH" "^$SHIMS_DIR:"
Greptile SummaryThis PR fixes Confidence Score: 4/5Safe to merge after updating the stale inline comment; core logic and tests are correct. The fix is minimal and correct — removing a single guard is exactly what's needed to ensure ordering. The e2e test has been strengthened to be position-aware, addressing the main concern from the previous review round. The only remaining issue is a stale inline comment in src/cli/activate.rs — inline comment at lines 93-95 needs updating Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["mise activate --shims"] --> B["activate_shims()"]
B --> C["shims_prepend_path(shell, exe_dir)"]
B --> D["shims_prepend_path(shell, &dirs::SHIMS)"]
C --> E{{"shell.supports_move_path()?\n(fish)"}}
D --> E
E -- Yes --> F["ActivatePrelude::MovePrependEnv\n(reorder, no duplicate)"]
E -- No --> G["ActivatePrelude::PrependEnv\n(unconditional prepend,\naccepts duplicate)"]
F --> H["format_activate_prelude()"]
G --> H
H --> I["emit shell script to stdout"]
style G fill:#d4edda,stroke:#28a745
style F fill:#cce5ff,stroke:#004085
Reviews (2): Last reviewed commit: "fix(activate): always prepend shims to f..." | Re-trigger Greptile |
| CUSTOM_PATH="/some/other/bin:$SHIMS_DIR:/usr/bin" | ||
|
|
||
| # Shims already in PATH: should still be prepended | ||
| assert_contains "env PATH='$CUSTOM_PATH' $MISE_BIN activate bash --shims" "$SHIMS_DIR" |
There was a problem hiding this comment.
Test verifies presence, not front-of-PATH position
assert_contains checks that $SHIMS_DIR appears somewhere in the activate output, but the stated fix goal is to ensure shims land at the front of PATH. The existing assertion would also pass if $SHIMS_DIR appeared anywhere else in the generated script (e.g. in a comment or a trailing append).
A stronger assertion would check that the PrependEnv line actually places $SHIMS_DIR first, for example by verifying the output starts with an export PATH="<shims>:…" assignment. Something like:
# Shims already in PATH: should be emitted as a prepend (shims first in the new line)
output="$(env PATH='$CUSTOM_PATH' $MISE_BIN activate bash --shims)"
assert_contains "$output" "export PATH=\"$SHIMS_DIR:"This is non-blocking since the current assertion still guards against the regression (missing shims in output), but a position-aware check would give higher confidence the fix is complete.
`mise activate --shims` now always prepends the shims directory even when it is already present in PATH, ensuring shims are first on every activation (e.g. when re-sourcing in VS Code). Previously, the prepend was skipped if shims was already in PATH, leaving it stranded at a lower priority position.
Problem
When using
mise activate bash --shims, if the shims directory was alreadypresent in
PATH(e.g. from a previous activation or a VS Code terminalre-sourcing the shell config), mise would skip the prepend entirely — leaving
shims stranded at whatever position they were already at, rather than at the
front.
Fix
shims_prepend_pathnow always emits a prepend, even when shims is alreadyin
PATH. For shells with native path deduplication (fish),MovePrependEnvis used to cleanly reorder without duplicating. For all other shells,
PrependEnvis emitted unconditionally, accepting a harmless duplicate entry in exchange for
guaranteed ordering.
Tests
e2e/shell/test_shims_activate_prepend— verifies shims are prepended whetheror not they are already present in
PATHe2e/shell/test_fish_shims_path_reorder— pre-existing fish coverage remainsunchanged