Repository navigation
fix(shim): prevent recursion when directory env is filtered - #10982
Conversation
📝 WalkthroughWalkthroughThe change adds ChangesShim recursion prevention
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Shim
participant mise
participant exec_program
participant which
participant RealTool
Shim->>mise: delegate with __MISE_SHIM_PATH
mise->>exec_program: resolve requested tool
exec_program->>which: search PATH without mise shim directories
which-->>exec_program: return executable candidates
exec_program->>RealTool: execute non-active shim candidate
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR refines the shim recursion guard from directory-level exclusion to per-candidate exclusion, so legitimate executables that share a directory with an active shim are no longer hidden. It records the active shim's absolute path in
Confidence Score: 5/5Safe to merge. The change is narrowly scoped to recursion detection, all three shim modes are covered, and the deny-env edge case is handled by forcing Lazy initialization before the env-clearing pass. The core logic — per-candidate filtering via No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "test(shim): exercise canonical active sh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/file.rs (1)
895-916: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid allocating a new
PathBuffor the active shim directory.Because
is_mise_shims_diris called in a loop for every entry in thePATHduring executable resolution, we can avoid cloning and allocating a newPathBufon each iteration by using.as_deref()and keepingactive_shimsas anOption<&Path>.♻️ Proposed refactor
- let active_shims = env::MISE_SHIM_PATH - .as_ref() - .cloned() - .and_then(|shim| shim.parent().map(Path::to_path_buf)); + let active_shims = env::MISE_SHIM_PATH + .as_deref() + .and_then(Path::parent); if paths_eq(&resolved, &dirs::SHIMS) || paths_eq(&resolved, &sys_shims) || active_shims - .as_ref() .is_some_and(|active| paths_eq(&resolved, active)) { return true; } let canon_input = canonicalize_or_self(&resolved); let canon_user = canonicalize_or_self(&dirs::SHIMS); let canon_sys = canonicalize_or_self(&sys_shims); - let canon_active = active_shims.as_deref().map(canonicalize_or_self); + let canon_active = active_shims.map(canonicalize_or_self); paths_eq(&canon_input, &canon_user) || paths_eq(&canon_input, &canon_sys) || canon_active🤖 Prompt for 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. In `@src/file.rs` around lines 895 - 916, Update the active shim directory handling in is_mise_shims_dir to keep active_shims as an Option<&Path> by using as_deref() on env::MISE_SHIM_PATH, avoiding cloned PathBuf allocation on each PATH iteration. Preserve the existing direct and canonicalized comparisons for the active directory.
🤖 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.
Nitpick comments:
In `@src/file.rs`:
- Around line 895-916: Update the active shim directory handling in
is_mise_shims_dir to keep active_shims as an Option<&Path> by using as_deref()
on env::MISE_SHIM_PATH, avoiding cloned PathBuf allocation on each PATH
iteration. Preserve the existing direct and canonicalized comparisons for the
active directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ff31bc43-90b3-4160-944a-a7c77d6409b0
📒 Files selected for processing (6)
crates/mise-shim/src/main.rse2e-win/shim_recursion.Tests.ps1src/cli/exec.rssrc/env.rssrc/file.rssrc/shims.rs
7af693d to
85aa8ca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/cli/test_exec_shim_recursion (1)
31-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptionally create the mock shim file on disk to fully mirror the comment's scenario.
The comment rightly mentions that "File shims can coexist with a real executable...". While
miseaccurately uses the candidate path identity (and thus passing the string to__MISE_SHIM_PATHis enough to test the path-matching logic), actually creating an emptysiblingtool.cmdon disk makes the directory state 100% true to reality. It ensures the file goes through the happy path of canonicalization rather than relying on the non-existent file string fallback.💡 Proposed tweak
echo SAME_DIRECTORY_REAL_TOOL TOOL chmod +x "$mixed_dir/siblingtool" +touch "$mixed_dir/siblingtool.cmd" export PATH="$mixed_dir:$PATH"🤖 Prompt for 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. In `@e2e/cli/test_exec_shim_recursion` around lines 31 - 54, Optionally create an empty mock shim file at "$mixed_dir/siblingtool.cmd" before the PATH setup in the test, so the candidate path used by __MISE_SHIM_PATH exists and exercises canonicalization while preserving the existing siblingtool assertions.
🤖 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.
Nitpick comments:
In `@e2e/cli/test_exec_shim_recursion`:
- Around line 31-54: Optionally create an empty mock shim file at
"$mixed_dir/siblingtool.cmd" before the PATH setup in the test, so the candidate
path used by __MISE_SHIM_PATH exists and exercises canonicalization while
preserving the existing siblingtool assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ed7dad5-47d7-445f-88af-4ed277b3f643
📒 Files selected for processing (5)
e2e-win/shim_recursion.Tests.ps1e2e/cli/test_exec_shim_recursionsrc/cli/exec.rssrc/file.rssrc/shims.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/exec.rs
- src/shims.rs
Summary
--deny-envlookup, then remove the internal marker before starting the real toolMotivation
Before this change, mise primarily avoided shim recursion by reconstructing its user and system shim directories from the current process environment. That works when the mise process sees the same directory configuration that was used to create or expose the shim.
It is not sufficient whenever a shim remains reachable through
PATHbut the process that mise starts with has different directory context. Examples include:MISE_SHIMS_DIR,MISE_DATA_DIR,XDG_DATA_HOME, orMISE_SYSTEM_DATA_DIRbeing filtered or changedPATHThis does not require normal shell activation.
mise activatenormally adds the selected tools' real bin directories toPATH, but shims are also used independently throughmise activate --shims, direct OS-levelPATHconfiguration, IDE/GUI integration, and CI. An existing shim entry can also be retained whennot_found_auto_installis enabled.In the failure case, the active shim delegates to
mise x -- <tool>. If mise cannot recognize that shim from its reconstructed directories, executable lookup can select the same shim again and recurse.This is therefore a general shim identity issue rather than behavior specific to any one parent application.
Implementation
Each native or generated shim sets an internal
__MISE_SHIM_PATHmarker to its own absolute path before delegating to mise. Mise then:--deny-envcan remove process variablesThe active shim's parent directory is not excluded wholesale. A file shim such as
tool.cmdcan legitimately coexist with a realtool.exein the same directory, so filtering is based on file identity rather than directory membership.The marker is internal dispatch state; the change does not preserve arbitrary
MISE_*variables through--deny-env.Validation
The particular parent-application environment was not reproduced locally. Validation models the general failure condition directly by placing an active shim outside mise's reconstructed shim directories and filtering its directory configuration.
mise run lint-fixmise run formatcargo test --all-features --no-runmise run test:e2e cli/test_exec_shim_recursion cli/test_system_shims_no_recursion cli/test_shims_fallbackSummary by CodeRabbit