diff --git a/CHANGELOG.md b/CHANGELOG.md index be24f69e..9277dc84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to Nexis. Format loosely follows [Keep a Changelog](https:// ## [Unreleased] +### Fixed +- **npm-installed language servers were found by the probe and then failed to spawn on Windows.** Installing `vscode-langservers-extracted` gave Nexis a working CSS/HTML/JSON server that it still refused to start, and the missing-tools notice told the story backwards: pressing refresh cleared the entry, then opening a `.css` file put it straight back. The two sides were asking different questions. `tool_probe` walks `PATHEXT` deliberately — 1.25.0 added that precisely because these servers land as `.cmd` shims — but the spawn in `lsp/session.rs` handed the bare name to `std::process::Command`, and Rust's Windows program resolution appends **only** `.exe`; it never consults `PATHEXT`. So `vscode-css-language-server` resolved for the probe and did not exist for the spawn. + - `tool_probe`'s PATH walk now returns the resolved path instead of a bool (`tools::resolve_on_host`), and both the LSP client and the DAP client spawn that path rather than the bare name. One walk answers both questions, so the two cannot drift apart again. When resolution finds nothing the bare name is still passed through, leaving the failure path — and its error message — exactly as it was. + - **PATHEXT spellings are now tried before the extensionless one**, and that ordering is the fix, not a detail. npm's shim writer emits three files per bin: `foo.cmd`, `foo.ps1`, and an extensionless `foo` that is a *bash* script for MSYS/Git Bash. Preferring the bare spelling — which the old suffix list did — would have resolved to a path `CreateProcessW` cannot execute, turning a lookup miss into a spawn failure. Trying PATHEXT first selects the `.cmd`, which `Command` routes through `cmd.exe` with its own hardened quoting. + - The DAP client is covered for the same reason rather than pre-emptively: the adapter command is free text in the debugger panel, so anyone naming an npm-installed adapter (`js-debug-adapter`) hits the identical gap. + - **A `.cmd` server is not the process Nexis holds a handle to**, so stopping it needed a tree kill. Rust's std spawns a batch program as `cmd.exe /d /c " ..."` — the handle is the wrapper and the server is a grandchild, so `Child::kill` (a bare `TerminateProcess`) left the server alive holding both pipe ends: the reader thread never saw EOF and every restart leaked another orphan. LSP and DAP sessions now hold a Windows Job Object with `KILL_ON_JOB_CLOSE`, the same guard ConPTY children have always had — the primitive moved from `pty/job.rs` to `modules/job.rs` and is now shared rather than duplicated. Both sessions also `wait()` after killing, so the child is reaped instead of lingering as a zombie until the app exits. + - **Resolution now answers the question the spawn asks, not an approximation of it.** Three ways it could return a path that would not run: a set-but-empty `PATHEXT` reads back as `Some("")` rather than absent, so the `.COM;.EXE;.BAT;.CMD` default never fired and the suffix list collapsed to the extensionless spelling — silently disabling `.cmd` lookup altogether; a program named as a *path* (`.\node_modules\.bin\js-debug-adapter`, which the debugger panel invites) skipped the suffix walk entirely and matched npm's extensionless bash script sitting next to the shim; and on Unix `mode & 0o111 != 0` accepted an exec bit belonging to somebody else, so a root-owned `0700` copy early on `PATH` would shadow the `0755` one later, where `execvp` skips the `EACCES` match and keeps walking. The suffix parsing is now a pure function tested on every platform rather than only on a Windows runner, and Unix executability is `access(X_OK)`. + ## [1.25.0] — 2026-08-19 ### Added diff --git a/docs/vault/Home.md b/docs/vault/Home.md index 68bfc087..7c6cbc3c 100644 --- a/docs/vault/Home.md +++ b/docs/vault/Home.md @@ -49,6 +49,6 @@ This vault is the **navigational knowledge base** for the Nexis codebase. It ans ## Other sections -- `decisions/` — lightweight ADRs: why something is the way it is, alternatives rejected (empty so far — use `templates/decision.md`) +- `decisions/` — lightweight ADRs: why something is the way it is, alternatives rejected. [[expansion-packs]], [[nexis-ml-artifact-pinning]], [[program-resolution-before-spawn]] (new ones from `templates/decision.md`) - `runbooks/` — how to do rare-but-recurring tasks (release, debugging a class of bug, forcing cache refreshes) - `templates/` — copy these when creating a new note diff --git a/docs/vault/decisions/program-resolution-before-spawn.md b/docs/vault/decisions/program-resolution-before-spawn.md new file mode 100644 index 00000000..fed23157 --- /dev/null +++ b/docs/vault/decisions/program-resolution-before-spawn.md @@ -0,0 +1,42 @@ +--- +type: decision +description: LSP and DAP resolve a program name to a concrete path via the same PATH walk tool_probe uses, then spawn that path — because Rust's Command ignores PATHEXT on Windows +--- + +# External programs are resolved to a path before spawning, not handed to `Command` as a bare name + +**Date:** 2026-08 +**Status:** active + +## Context + +Nexis asks two different questions about the same external tool, and they were answered by two different pieces of code: + +- **"Is it installed?"** — `tool_probe` (`src-tauri/src/modules/tools.rs`), behind the missing-tools pill's refresh button. +- **"Run it."** — `LspSession::start` / `DapSession::start`, which called `proc::command()`. + +`tool_probe` walks `PATHEXT` on Windows on purpose: every server from `vscode-langservers-extracted` installs as a `.cmd` shim, not an `.exe`. `std::process::Command` does **not** walk `PATHEXT` — its Windows resolution appends only `.exe`. So `vscode-css-language-server` was simultaneously "installed" (probe) and "not found" (spawn). + +The user-visible shape of this was a notice that lied in both directions: refresh cleared the entry, and the next `.css` file put it back, forever. + +## Decision + +The PATH walk returns the **resolved path**, not a bool. `tools::resolve_on_host(name) -> Option` is the single implementation; `resolves_on_host` is now `resolve_on_host(..).is_some()`. Both spawn sites resolve first and pass the resulting path to `proc::command`, falling back to the bare name when resolution finds nothing so the failure path and its error message are unchanged. + +On Windows the suffix list tries **PATHEXT entries before the extensionless spelling**. npm writes three files per bin — `foo.cmd`, `foo.ps1`, and an extensionless `foo` that is a bash script for MSYS/Git Bash. Preferring the bare spelling resolves to a path `CreateProcessW` cannot execute, which is strictly worse than not resolving at all. + +## Alternatives rejected + +- **Append `.cmd` on Windows at the call site** — hard-codes one shim flavour, ignores the user's actual `PATHEXT`, and puts platform knowledge in every spawn site instead of one. +- **Spawn through `cmd /c`** — Rust's std already detects `.bat`/`.cmd` by extension and routes them through `cmd.exe` with the hardened quoting added for CVE-2024-24576. Doing it by hand re-opens the quoting hole that fix closed. +- **Teach `proc::command` to resolve** — tempting, but `proc::command` is also used for `wsl.exe`, `git`, `nvidia-smi` and absolute interpreter paths, where a PATH walk is either wasted or actively wrong. Resolution belongs to the callers that take a *user- or config-supplied* program name. + +## Consequences + +- Probe and spawn can no longer disagree: they are the same walk. A future divergence would require someone to reintroduce a second lookup. +- Any **new** spawn site whose program name comes from config or user input should call `resolve_on_host` first. Sites that spawn a fixed system binary (`wsl.exe`, `git`) or an already-absolute path do not need it. +- This is host-side resolution only. WSL tools resolve inside the distro via `command -v` through `wsl_exec_capture` — see pitfall #20 in CLAUDE.md for why the two sides must not answer for each other. +- Resolving means the returned path is *spawned*, so it has to be runnable by **this** user, not merely marked executable for someone: Unix executability is `access(X_OK)`, matching what `execvp` asks before it skips a match and keeps walking `PATH`. +- Spawning a resolved `.cmd` makes the direct child `cmd.exe`, not the server. Both session types therefore hold a `KILL_ON_JOB_CLOSE` Job Object (`modules/job.rs`, shared with the ConPTY children it was written for) — `Child::kill` alone terminates the wrapper and orphans the server. +- The `PATHEXT` parsing is a pure function (`pathext_suffixes`) rather than an env read inside the `#[cfg(windows)]` branch. +- The Windows branch is `#[cfg(windows)]`, so CI on Linux does not exercise it. The `windows_tries_pathext_before_the_extensionless_name` test asserts the ordering invariant on every platform by checking the suffix list's shape. diff --git a/src-tauri/src/modules/dap/session.rs b/src-tauri/src/modules/dap/session.rs index 0c1c457a..64d17525 100644 --- a/src-tauri/src/modules/dap/session.rs +++ b/src-tauri/src/modules/dap/session.rs @@ -25,6 +25,16 @@ use crate::modules::proc; type PendingMap = Arc>>>>; pub struct DapSession { + /// Windows only: kills the whole process tree when the session drops. + /// + /// Load-bearing since programs are resolved before spawning. A server that + /// resolves to a `.cmd` shim is run as `cmd.exe /d /c " ..."`, so + /// `_child` is the wrapper and the server itself is a grandchild — + /// `kill()` would terminate the wrapper and leave the server alive holding + /// both pipe ends, so the reader thread never sees EOF and every restart + /// leaks another orphan. Declared before `_child` so the Job closes first. + #[cfg(windows)] + _job: Option, _child: Child, stdin: Arc>>, pending: PendingMap, @@ -42,7 +52,13 @@ impl DapSession { session_id: u32, app: AppHandle, ) -> Result { - let mut cmd = proc::command(adapter_cmd); + // Same PATHEXT resolution the LSP client does, and for the same + // reason: the adapter command is free text in the debugger panel, so + // a user who types an npm-installed adapter (`js-debug-adapter`) hits + // the `.cmd`-shim gap that `Command` alone cannot bridge on Windows. + let program = crate::modules::tools::resolve_on_host(adapter_cmd) + .unwrap_or_else(|| std::path::PathBuf::from(adapter_cmd)); + let mut cmd = proc::command(&program); cmd.args(adapter_args) .stdin(Stdio::piped()) .stdout(Stdio::piped()) @@ -52,6 +68,18 @@ impl DapSession { .spawn() .map_err(|e| format!("dap: failed to start '{adapter_cmd}': {e}"))?; + // Tree-kill guard, set up before anything can fail out of this + // function. See the `_job` field for why `kill()` alone is not enough + // once a `.cmd` shim is in play. + #[cfg(windows)] + let job = match crate::modules::job::ProcessJob::create_for(child.id()) { + Ok(j) => Some(j), + Err(e) => { + log::warn!("dap job-object setup failed for pid={}: {e}", child.id()); + None + } + }; + let stdin = child.stdin.take().ok_or("dap: no stdin")?; let stdout = child.stdout.take().ok_or("dap: no stdout")?; @@ -66,6 +94,8 @@ impl DapSession { } Ok(Self { + #[cfg(windows)] + _job: job, _child: child, stdin, pending, @@ -232,6 +262,8 @@ impl Drop for DapSession { fn drop(&mut self) { let _ = self.disconnect(); let _ = self._child.kill(); + // Reap, so the killed adapter does not linger as a zombie. + let _ = self._child.wait(); } } diff --git a/src-tauri/src/modules/pty/job.rs b/src-tauri/src/modules/job.rs similarity index 79% rename from src-tauri/src/modules/pty/job.rs rename to src-tauri/src/modules/job.rs index aaec7bb1..8af54a91 100644 --- a/src-tauri/src/modules/pty/job.rs +++ b/src-tauri/src/modules/job.rs @@ -4,9 +4,17 @@ // ║ 2026 ║ // ╚══════════════════════════════════════╝ -//! Windows Job Object with KILL_ON_JOB_CLOSE for ConPTY children. +//! Windows Job Object with KILL_ON_JOB_CLOSE for spawned children. //! Dropping the handle kills the whole tree — only reliable orphan guard //! on Windows. +//! +//! Two callers need it, for the same reason from different directions. A +//! ConPTY child is a shell, so anything it started is a grandchild. And a +//! program resolved to a `.cmd` shim is not run directly at all: Rust's std +//! detects the extension and spawns `cmd.exe /d /c " ..."`, so the +//! handle the caller holds is the wrapper and the real process — an +//! npm-installed language server or debug adapter — is a grandchild again. +//! `Child::kill` is `TerminateProcess`, which does not walk the tree. // Panic-lint gate: no `.unwrap()`/`.expect()` in production code here. // Tests may still panic (allow-*-in-tests in clippy.toml). CI's @@ -25,14 +33,14 @@ use windows_sys::Win32::System::JobObjects::{ }; use windows_sys::Win32::System::Threading::{OpenProcess, PROCESS_SET_QUOTA, PROCESS_TERMINATE}; -pub struct PtyJob { +pub struct ProcessJob { handle: HANDLE, } -unsafe impl Send for PtyJob {} -unsafe impl Sync for PtyJob {} +unsafe impl Send for ProcessJob {} +unsafe impl Sync for ProcessJob {} -impl PtyJob { +impl ProcessJob { pub fn create_for(pid: u32) -> io::Result { unsafe { let job = CreateJobObjectW(std::ptr::null(), std::ptr::null()); @@ -74,7 +82,7 @@ impl PtyJob { } } -impl Drop for PtyJob { +impl Drop for ProcessJob { fn drop(&mut self) { if !self.handle.is_null() && self.handle != INVALID_HANDLE_VALUE { unsafe { CloseHandle(self.handle) }; diff --git a/src-tauri/src/modules/lsp/session.rs b/src-tauri/src/modules/lsp/session.rs index 9c36ee97..d4c2702a 100644 --- a/src-tauri/src/modules/lsp/session.rs +++ b/src-tauri/src/modules/lsp/session.rs @@ -25,6 +25,16 @@ use crate::modules::proc; type PendingMap = Arc>>>>; pub struct LspSession { + /// Windows only: kills the whole process tree when the session drops. + /// + /// Load-bearing since programs are resolved before spawning. A server that + /// resolves to a `.cmd` shim is run as `cmd.exe /d /c " ..."`, so + /// `_child` is the wrapper and the server itself is a grandchild — + /// `kill()` would terminate the wrapper and leave the server alive holding + /// both pipe ends, so the reader thread never sees EOF and every restart + /// leaks another orphan. Declared before `_child` so the Job closes first. + #[cfg(windows)] + _job: Option, /// Kept alive so stdin/stdout pipes stay open. _child: Child, stdin: Arc>>, @@ -44,7 +54,17 @@ impl LspSession { initialization_options: Option, app: AppHandle, ) -> Result { - let mut cmd = proc::command(server_cmd); + // Resolve before spawning. `Command` on Windows only ever appends + // `.exe` to a bare name, but every npm-installed server here lands as + // a `.cmd` shim — so `vscode-css-language-server` fails to spawn even + // though `tool_probe` (which does walk PATHEXT) reports it present. + // That split made the missing-tools pill lie in both directions: it + // cleared on refresh, then came straight back on the next open. + // Falling back to the bare name keeps the failure path unchanged when + // resolution finds nothing, so the error still comes from the spawn. + let program = crate::modules::tools::resolve_on_host(server_cmd) + .unwrap_or_else(|| std::path::PathBuf::from(server_cmd)); + let mut cmd = proc::command(&program); cmd.args(server_args) .stdin(Stdio::piped()) .stdout(Stdio::piped()) @@ -54,6 +74,18 @@ impl LspSession { .spawn() .map_err(|e| format!("lsp: failed to start '{server_cmd}': {e}"))?; + // Tree-kill guard, set up before anything can fail out of this + // function. See the `_job` field for why `kill()` alone is not enough + // once a `.cmd` shim is in play. + #[cfg(windows)] + let job = match crate::modules::job::ProcessJob::create_for(child.id()) { + Ok(j) => Some(j), + Err(e) => { + log::warn!("lsp job-object setup failed for pid={}: {e}", child.id()); + None + } + }; + let stdin = child.stdin.take().ok_or("lsp: no stdin pipe")?; let stdout = child.stdout.take().ok_or("lsp: no stdout pipe")?; @@ -70,6 +102,8 @@ impl LspSession { } let session = Self { + #[cfg(windows)] + _job: job, _child: child, stdin, pending, @@ -222,9 +256,12 @@ impl LspSession { impl Drop for LspSession { fn drop(&mut self) { - // Best-effort graceful shutdown then kill. + // Best-effort graceful shutdown then kill. `wait` is not optional: + // without it the killed child stays a zombie until the app exits, and + // on Windows it is what releases the wrapper before the Job closes. let _ = self.notify("exit".to_string(), None); let _ = self._child.kill(); + let _ = self._child.wait(); } } diff --git a/src-tauri/src/modules/mod.rs b/src-tauri/src/modules/mod.rs index 84eeb310..e6ac43dc 100644 --- a/src-tauri/src/modules/mod.rs +++ b/src-tauri/src/modules/mod.rs @@ -13,6 +13,8 @@ pub mod fs; pub mod fswatch; pub mod git; pub mod http_share; +#[cfg(windows)] +pub mod job; pub mod lsp; pub mod ml; pub mod net; diff --git a/src-tauri/src/modules/pty/mod.rs b/src-tauri/src/modules/pty/mod.rs index 93e11078..64f66384 100644 --- a/src-tauri/src/modules/pty/mod.rs +++ b/src-tauri/src/modules/pty/mod.rs @@ -5,8 +5,6 @@ // ╚══════════════════════════════════════╝ pub(crate) mod da_filter; -#[cfg(windows)] -mod job; mod session; pub(crate) mod shell_init; mod watchdog; diff --git a/src-tauri/src/modules/pty/session.rs b/src-tauri/src/modules/pty/session.rs index a35f1760..58cbb191 100644 --- a/src-tauri/src/modules/pty/session.rs +++ b/src-tauri/src/modules/pty/session.rs @@ -47,7 +47,7 @@ pub struct Session { // 4. `master` — last; ClosePseudoConsole on Windows. By now the child // is dead and conhost has nothing left to drain. #[cfg(windows)] - _job: Option, + _job: Option, pub killer: Mutex>, /// FIFO input queue drained by the dedicated writer thread. `pty_write` /// enqueues here (never blocks); the thread does the actual pipe write, @@ -193,7 +193,7 @@ pub fn spawn( #[cfg(windows)] let job = match child.process_id() { - Some(pid) => match super::job::PtyJob::create_for(pid) { + Some(pid) => match crate::modules::job::ProcessJob::create_for(pid) { Ok(j) => Some(j), Err(e) => { log::warn!("pty job-object setup failed for pid={pid}: {e}"); diff --git a/src-tauri/src/modules/tools.rs b/src-tauri/src/modules/tools.rs index f84c84c2..fb8a355c 100644 --- a/src-tauri/src/modules/tools.rs +++ b/src-tauri/src/modules/tools.rs @@ -23,7 +23,7 @@ //! name resolves to an executable is the question the spawn itself will ask. use crate::modules::workspace::WorkspaceEnv; -use std::path::Path; +use std::path::{Path, PathBuf}; /// Which of `binaries` resolve to something runnable right now. /// @@ -66,40 +66,72 @@ fn resolves(workspace: &WorkspaceEnv, binary: &str) -> bool { } } -/// A `which`, in-process: no subprocess, so it cannot hang or flash a console. fn resolves_on_host(binary: &str) -> bool { + resolve_on_host(binary).is_some() +} + +/// A `which`, in-process: no subprocess, so it cannot hang or flash a console. +/// +/// Returns the concrete path rather than a bool because callers need both +/// answers from the same walk. `tool_probe` only wants "is it there", but a +/// spawn site needs the resolved path itself: `std::process::Command` on +/// Windows appends **only** `.exe` to a bare name — it does not consult +/// PATHEXT — so handing it `vscode-css-language-server` fails even though the +/// probe found `vscode-css-language-server.cmd` one directory over. Resolving +/// here and spawning the result is what keeps the two sides agreeing. +pub fn resolve_on_host(binary: &str) -> Option { + if binary.is_empty() { + return None; + } let path = Path::new(binary); // A name carrying a separator is a path, not a PATH lookup — that is what // the OS does when it spawns it, so match that here. + // + // The suffix walk still applies. A config entry or debugger-panel command + // that names a *path* into an npm bin directory + // (`.\node_modules\.bin\js-debug-adapter`) has the identical `.cmd`-shim + // problem, and on Windows the extensionless sibling sitting right next to + // the shim is a bash script — a file that exists and cannot be spawned, so + // checking the bare spelling alone would "resolve" to a guaranteed + // failure. On a non-Windows host the suffix list is `[""]`, which makes + // this exactly the single exact-path check it looks like. if path.components().count() > 1 { - return is_executable_file(path); + return executable_suffixes().into_iter().find_map(|suffix| { + let candidate = if suffix.is_empty() { + path.to_path_buf() + } else { + PathBuf::from(format!("{binary}{suffix}")) + }; + is_executable_file(&candidate).then_some(candidate) + }); } - let Some(path_var) = std::env::var_os("PATH") else { - return false; - }; + let path_var = std::env::var_os("PATH")?; + let suffixes = executable_suffixes(); + // Suffixes inner, directories outer: the first PATH entry holding *any* + // spawnable spelling wins, which is the order Windows itself searches. std::env::split_paths(&path_var) .filter(|dir| !dir.as_os_str().is_empty()) - .any(|dir| { - executable_suffixes() - .iter() - .any(|suffix| is_executable_file(&dir.join(format!("{binary}{suffix}")))) + .find_map(|dir| { + suffixes.iter().find_map(|suffix| { + let candidate = dir.join(format!("{binary}{suffix}")); + is_executable_file(&candidate).then_some(candidate) + }) }) } /// Suffixes to try for a bare name. On Windows the npm-installed servers here /// land as `.cmd` shims rather than `.exe`, so PATHEXT is not optional. +/// +/// The extensionless spelling goes **last** on Windows, and that ordering is +/// load-bearing. npm's shim writer emits three files per bin — `foo`, `foo.cmd` +/// and `foo.ps1` — where the extensionless `foo` is a *bash* script for +/// MSYS/Git Bash. `CreateProcessW` cannot run it, so preferring it would +/// resolve to a path that is guaranteed not to spawn. Trying PATHEXT first +/// picks the `.cmd`, which `Command` routes through `cmd.exe` for us. fn executable_suffixes() -> Vec { #[cfg(windows)] { - let mut out = vec![String::new()]; - let raw = std::env::var("PATHEXT").unwrap_or_else(|_| ".COM;.EXE;.BAT;.CMD".to_string()); - out.extend( - raw.split(';') - .map(str::trim) - .filter(|e| e.starts_with('.')) - .map(str::to_string), - ); - out + pathext_suffixes(std::env::var("PATHEXT").ok().as_deref()) } #[cfg(not(windows))] { @@ -107,13 +139,62 @@ fn executable_suffixes() -> Vec { } } +/// The Windows suffix list, as a pure function of the raw `PATHEXT` value. +/// +/// Deliberately **not** `#[cfg(windows)]`: the branch it feeds is, so its +/// tests would otherwise never run on the Linux CI that gates every push. Same +/// reasoning as `parse_wsl_probe` (pitfall #21) — which is also why the +/// dead-code allow is scoped to non-Windows rather than blanket: on Windows +/// this is live production code and must stay lint-visible. +#[cfg_attr(not(windows), allow(dead_code))] +fn pathext_suffixes(raw: Option<&str>) -> Vec { + const DEFAULT_PATHEXT: &str = ".COM;.EXE;.BAT;.CMD"; + fn parse(raw: &str) -> Vec { + raw.split(';') + .map(str::trim) + .filter(|e| e.starts_with('.')) + .map(str::to_string) + .collect() + } + // A set-but-*empty* `PATHEXT` reads back as `Some("")`, not `None`, so a + // plain `unwrap_or` on the absent case would never fire and the list would + // collapse to the extensionless spelling — silently disabling the whole + // `.cmd` resolution this exists for. The second guard covers a `PATHEXT` + // whose entries all fail the leading-dot filter (`";;"`), which lands in + // the same place by a different road. + let mut out = raw + .filter(|v| !v.trim().is_empty()) + .map(parse) + .unwrap_or_default(); + if out.is_empty() { + out = parse(DEFAULT_PATHEXT); + } + out.push(String::new()); + out +} + #[cfg(unix)] fn is_executable_file(path: &Path) -> bool { - use std::os::unix::fs::PermissionsExt; + use std::os::unix::ffi::OsStrExt; // `metadata` follows symlinks, which is what an exec would do too. - std::fs::metadata(path) - .map(|m| m.is_file() && m.permissions().mode() & 0o111 != 0) + if !std::fs::metadata(path) + .map(|m| m.is_file()) .unwrap_or(false) + { + return false; + } + // Permission *bits* are the wrong question now that the answer is spawned + // rather than counted. `execvp` skips a match it cannot actually run + // (EACCES) and keeps walking PATH, so a root-owned 0700 `pylsp` early on + // PATH must not shadow the 0755 one later — which is exactly what + // `mode & 0o111 != 0` would do, since it accepts an exec bit belonging to + // somebody else. `access(X_OK)` asks the kernel the question the spawn + // will ask. + let Ok(c_path) = std::ffi::CString::new(path.as_os_str().as_bytes()) else { + return false; + }; + // Safety: `c_path` outlives the call and `access` only reads it. + unsafe { libc::access(c_path.as_ptr(), libc::X_OK) == 0 } } #[cfg(not(unix))] @@ -165,6 +246,134 @@ mod tests { assert!(!resolves_on_host("no/such/dir/sh")); } + #[test] + fn resolution_returns_a_spawnable_path_not_the_bare_name() { + // The whole point of `resolve_on_host` over a bool: what comes back + // must be something `Command` can spawn as-is. On Windows that means + // a PATHEXT spelling, never the bare name a `Command::new` would fail + // to find. + let probe = if cfg!(windows) { "cmd" } else { "sh" }; + let resolved = resolve_on_host(probe).expect("probe binary on PATH"); + assert!(resolved.is_absolute(), "{resolved:?} is not absolute"); + assert!( + is_executable_file(&resolved), + "{resolved:?} is not runnable" + ); + #[cfg(windows)] + assert!( + resolved.extension().is_some(), + "{resolved:?} has no extension; CreateProcessW cannot run it" + ); + } + + #[test] + fn windows_tries_pathext_before_the_extensionless_name() { + // npm writes `foo` (a bash script), `foo.cmd` and `foo.ps1` into the + // same directory. Resolving to the extensionless bash script would be + // a path that never spawns, so PATHEXT must come first. + assert_eq!( + executable_suffixes().last().map(String::as_str), + Some(""), + "the extensionless spelling must be the last resort" + ); + // Asserted through the pure parser so the Windows ordering is covered + // on every platform, not only on a Windows runner. + let suffixes = pathext_suffixes(Some(".COM;.EXE;.BAT;.CMD")); + assert!( + suffixes.len() > 1 && !suffixes[0].is_empty(), + "PATHEXT entries must precede the bare name: {suffixes:?}" + ); + assert_eq!(suffixes.last().map(String::as_str), Some("")); + } + + #[test] + fn an_empty_or_unusable_pathext_still_yields_the_shim_extensions() { + // `PATHEXT=""` reads back as `Some("")`, and `";;"` parses to nothing. + // Either one collapsing the list to `[""]` would turn every `.cmd` + // shim back into "not found" — the exact bug this module fixes, but + // arrived at through the environment instead of through `Command`. + for raw in [None, Some(""), Some(" "), Some(";;"), Some("bogus")] { + let suffixes = pathext_suffixes(raw); + assert!( + suffixes.iter().any(|s| s.eq_ignore_ascii_case(".cmd")), + "{raw:?} produced {suffixes:?}, which cannot find a .cmd shim" + ); + assert_eq!(suffixes.last().map(String::as_str), Some("")); + } + } + + #[test] + fn a_real_pathext_is_honoured_and_ordered() { + let suffixes = pathext_suffixes(Some(".COM; .EXE ;.CMD")); + assert_eq!(suffixes, vec![".COM", ".EXE", ".CMD", ""]); + } + + #[cfg(unix)] + #[test] + fn a_file_this_user_cannot_execute_is_not_a_match() { + // `mode & 0o111 != 0` accepted an exec bit belonging to somebody + // else, so a 0o001 file looked runnable. That was harmless while the + // answer was a bool, and wrong once the answer is the path we spawn: + // `execvp` skips an EACCES match and keeps walking PATH. + use std::os::unix::fs::PermissionsExt; + // Root bypasses the check (any exec bit is enough), so this cannot + // assert anything in a root container. + // Safety: `geteuid` reads process state and cannot fail. + if unsafe { libc::geteuid() } == 0 { + return; + } + let path = std::env::temp_dir().join(format!( + "nexis-tools-x-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::write(&path, b"#!/bin/sh\ntrue\n").expect("write probe file"); + let set = |mode| { + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)) + .expect("chmod probe file") + }; + + set(0o001); // executable by "other" only — not by us + assert!( + !is_executable_file(&path), + "0o001 must not count as runnable" + ); + set(0o644); + assert!(!is_executable_file(&path)); + set(0o755); + assert!(is_executable_file(&path)); + + let _ = std::fs::remove_file(&path); + } + + #[test] + fn an_explicit_path_to_a_non_executable_file_is_not_a_match() { + // The separator branch now walks suffixes too, so it must still reject + // a path that exists but cannot be run rather than returning it. + let path = std::env::temp_dir().join(format!( + "nexis-tools-plain-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::write(&path, b"not a program").expect("write probe file"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)) + .expect("chmod probe file"); + assert!(resolve_on_host(&path.to_string_lossy()).is_none()); + } + let _ = std::fs::remove_file(&path); + } + + #[test] + fn an_empty_name_resolves_to_nothing() { + // `Path::new("").components().count()` is 0, so this must be rejected + // up front rather than falling through to a PATH walk that joins the + // suffix onto every directory and matches the directory itself. + assert!(resolve_on_host("").is_none()); + } + #[test] fn a_directory_is_not_executable() { let dir = std::env::temp_dir();