fix(command): resolve relative PATH entries against cwd - #2350
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
b6e28ea to
990e718
Compare
|
@RSS1102 Can you add a snapshot test to cover this bug fix change? |
7abfc83 to
4e44ec1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| cwd: &AbsolutePath, | ||
| ) -> Result<OsString, std::env::JoinPathsError> { | ||
| std::env::join_paths(std::env::split_paths(path_env).map(|path| { | ||
| if path.is_absolute() || path.starts_with("~") { path } else { cwd.as_path().join(path) } |
There was a problem hiding this comment.
Need to consider compatibility issues on Windows, for example C:foo and \foo on Windows will be judged as relative, so the path after cwd join will be incorrect?
Alternatively, we could only check for explicit relative paths, supporting only paths that clearly start with . like ./foo/bar, while not supporting paths like foo/bar.
2 participants
Summary
PATHentries against the command cwd before binary lookup~expansion semanticsProblem
which::which_insearches relative PATH entries against the process cwd rather than the cwd supplied for the command. The npm-distributed CLI can therefore fail to resolvenodewhen Vite+ git hooks prepend./node_modules/.bin, even though the binary exists.Validation
cargo test -p vp_command(17 passed)cargo test -p vp_global_cli relative_path_entry(1 passed)cargo clippy -p vp_command --all-targets --all-features -- -D warningscargo fmt --all -- --checkgit diff --checkvite-plus@0.2.7and a relative PATH entry, then verified the same command reachesvp checkwith the patched NAPI bindingFixes #2326