fix(Windows): Command resolution for executables#427
Conversation
Greptile SummaryThis PR fixes Windows command resolution by using Confidence Score: 5/5Safe to merge — no P0/P1 issues; previous compilation concerns have been addressed. All three changed sites correctly define the No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User runs: fnox exec -- npm install"] --> B{"Platform?"}
B -- "Windows" --> C["which::which('npm')"]
C -- "Found" --> D["cmd_path = PathBuf (e.g. npm.cmd)"]
C -- "Not found" --> E["cmd_path = 'npm' (original fallback)"]
B -- "Non-Windows" --> F["cmd_path = 'npm' (unchanged)"]
D --> G["Command::new(cmd_path)"]
E --> G
F --> G
G --> H["Spawn process with secrets injected"]
Reviews (5): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request implements platform-specific command resolution on Windows by using the which crate to locate executables in edit.rs, exec.rs, and mcp_server.rs. The review feedback recommends using #[cfg(windows)] attributes instead of runtime if cfg!(windows) checks to avoid potential compilation errors on non-Windows platforms where the which dependency might be absent. Additionally, the feedback highlights that which::which is a blocking operation, which is an important consideration for the asynchronous implementation in mcp_server.rs.
| let editor_path = if cfg!(windows) { | ||
| which::which(&editor).unwrap_or_else(|_| editor.clone().into()) | ||
| } else { | ||
| editor.clone().into() | ||
| }; |
There was a problem hiding this comment.
Using #[cfg(windows)] is generally preferred over if cfg!(windows) when calling functions from platform-specific dependencies. If the which crate is defined as a target-specific dependency in Cargo.toml (e.g., only for Windows), the current code will fail to compile on other platforms because the compiler still checks the inactive branch. Additionally, the logic can be simplified by avoiding redundant clones for the non-Windows case.
#[cfg(windows)]
let editor_path = which::which(&editor).unwrap_or_else(|_| editor.clone().into());
#[cfg(not(windows))]
let editor_path = &editor;| let mut cmd = if cfg!(windows) { | ||
| let resolved = which::which(cmd_name).unwrap_or_else(|_| cmd_name.into()); | ||
| Command::new(resolved) | ||
| } else { | ||
| Command::new(cmd_name) | ||
| }; |
There was a problem hiding this comment.
Consider using #[cfg(windows)] to ensure that the which crate call is only compiled on Windows. This avoids potential build failures on other platforms if which is a target-specific dependency. The suggested refactoring also simplifies the command initialization by separating the path resolution from the Command construction.
#[cfg(windows)]
let cmd_path = which::which(cmd_name).unwrap_or_else(|_| cmd_name.into());
#[cfg(not(windows))]
let cmd_path = cmd_name;
let mut cmd = Command::new(cmd_path);| let mut cmd = if cfg!(windows) { | ||
| let resolved = which::which(cmd_name).unwrap_or_else(|_| cmd_name.into()); | ||
| tokio::process::Command::new(resolved) | ||
| } else { | ||
| tokio::process::Command::new(cmd_name) | ||
| }; |
There was a problem hiding this comment.
In an async context, which::which is a blocking I/O operation that can stall the Tokio executor thread. While the impact may be low for this specific tool, it's a best practice to use tokio::task::spawn_blocking for filesystem lookups in high-performance async applications. At a minimum, switching to #[cfg(windows)] is recommended to avoid compilation issues on non-Windows platforms if which is a conditional dependency.
#[cfg(windows)]
let cmd_path = which::which(cmd_name).unwrap_or_else(|_| cmd_name.into());
#[cfg(not(windows))]
let cmd_path = cmd_name;
let mut cmd = tokio::process::Command::new(cmd_path);### 🚀 Features - Powershell integration by [@nbfritch](https://github.com/nbfritch) in [#421](#421) ### 🐛 Bug Fixes - **(Windows)** Nushell integration by [@john-trieu-nguyen](https://github.com/john-trieu-nguyen) in [#425](#425) - **(Windows)** Command resolution for executables by [@john-trieu-nguyen](https://github.com/john-trieu-nguyen) in [#427](#427) ### 📚 Documentation - add releases nav and aube lock by [@jdx](https://github.com/jdx) in [#422](#422) - include linux native packages in aube lock by [@jdx](https://github.com/jdx) in [#423](#423) ### 🔍 Other Changes - Use published `clap-sort` crate instead of inlined module by [@jdx](https://github.com/jdx) in [#409](#409) - add communique 1.0.1 by [@jdx](https://github.com/jdx) in [#424](#424) ### 📦️ Dependency Updates - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#381](#381) - update taiki-e/upload-rust-binary-action digest to 10c1cf6 by [@renovate[bot]](https://github.com/renovate[bot]) in [#383](#383) - update rust crate tokio to v1.51.1 by [@renovate[bot]](https://github.com/renovate[bot]) in [#384](#384) - update rust crate indexmap to v2.14.0 by [@renovate[bot]](https://github.com/renovate[bot]) in [#385](#385) - update rust crate rmcp to v1.4.0 by [@renovate[bot]](https://github.com/renovate[bot]) in [#389](#389) - update rust crate strum to 0.28 by [@renovate[bot]](https://github.com/renovate[bot]) in [#395](#395) - update rust crate toml_edit to 0.25 by [@renovate[bot]](https://github.com/renovate[bot]) in [#396](#396) - update rust crate miniz_oxide to 0.9 by [@renovate[bot]](https://github.com/renovate[bot]) in [#390](#390) - update rust crate ratatui to 0.30 by [@renovate[bot]](https://github.com/renovate[bot]) in [#392](#392) - update actions/checkout action to v6 by [@renovate[bot]](https://github.com/renovate[bot]) in [#397](#397) - update actions/deploy-pages action to v5 by [@renovate[bot]](https://github.com/renovate[bot]) in [#399](#399) - update actions/configure-pages action to v6 by [@renovate[bot]](https://github.com/renovate[bot]) in [#398](#398) - update actions/setup-node action to v6 by [@renovate[bot]](https://github.com/renovate[bot]) in [#400](#400) - update actions/upload-pages-artifact action to v4 by [@renovate[bot]](https://github.com/renovate[bot]) in [#401](#401) - update dependency node to v24 by [@renovate[bot]](https://github.com/renovate[bot]) in [#403](#403) - update apple-actions/import-codesign-certs action to v6 by [@renovate[bot]](https://github.com/renovate[bot]) in [#402](#402) - update nick-fields/retry action to v4 by [@renovate[bot]](https://github.com/renovate[bot]) in [#406](#406) - update github artifact actions (major) by [@renovate[bot]](https://github.com/renovate[bot]) in [#404](#404) - update jdx/mise-action action to v4 by [@renovate[bot]](https://github.com/renovate[bot]) in [#405](#405) - update rust crate which to v8 by [@renovate[bot]](https://github.com/renovate[bot]) in [#408](#408) - update rust crate usage-lib to v3 by [@renovate[bot]](https://github.com/renovate[bot]) in [#407](#407) - bump rustcrypto stack (aes-gcm, sha2, hkdf) together by [@jdx](https://github.com/jdx) in [#410](#410) - update rust crate reqwest to 0.13 by [@renovate[bot]](https://github.com/renovate[bot]) in [#393](#393) - update rust crate libloading to 0.9 by [@renovate[bot]](https://github.com/renovate[bot]) in [#388](#388) - update rust crate keepass to 0.10 by [@renovate[bot]](https://github.com/renovate[bot]) in [#387](#387) - update rust crate rand to 0.10 by [@renovate[bot]](https://github.com/renovate[bot]) in [#391](#391) - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#411](#411) - update rust crate google-cloud-secretmanager-v1 to v1.8.0 by [@renovate[bot]](https://github.com/renovate[bot]) in [#415](#415) - update actions/upload-pages-artifact action to v5 by [@renovate[bot]](https://github.com/renovate[bot]) in [#418](#418) - update rust crate rmcp to v1.5.0 by [@renovate[bot]](https://github.com/renovate[bot]) in [#416](#416) - update rust crate clap to v4.6.1 by [@renovate[bot]](https://github.com/renovate[bot]) in [#413](#413) - update rust crate tokio to v1.52.1 by [@renovate[bot]](https://github.com/renovate[bot]) in [#417](#417) - update rust crate keepass to v0.10.6 by [@renovate[bot]](https://github.com/renovate[bot]) in [#414](#414) - update taiki-e/upload-rust-binary-action digest to f0d45ae by [@renovate[bot]](https://github.com/renovate[bot]) in [#419](#419) - update rust crate aws-sdk-sts to v1.102.0 by [@renovate[bot]](https://github.com/renovate[bot]) in [#420](#420) ### New Contributors - @john-trieu-nguyen made their first contribution in [#427](#427) - @nbfritch made their first contribution in [#421](#421)
Some commands on Windows would fail when the actual executable has an extension (e.g npm.cmd).
This fails:
fnox exec -- npm installThis works:
fnox exec -- npm.cmd installUpdate commands to use Which for better path resolution.
Now npm install works as expected.