fix(lsp,dap): resolve program names via PATHEXT before spawning - #70
Merged
Conversation
`tool_probe` walks PATHEXT on Windows on purpose -- every server from `vscode-langservers-extracted` installs as a `.cmd` shim, not an `.exe`. The spawn sites did not: they handed the bare name to `proc::command`, and Rust's Windows program resolution appends only `.exe`. So `vscode-css-language-server` was simultaneously installed (probe) and not found (spawn), and the missing-tools pill lied in both directions -- refresh cleared the entry, the next .css file put it straight back. The PATH walk now returns the resolved path rather than a bool (`tools::resolve_on_host`), and both the LSP and DAP clients spawn that path. One walk answers "is it installed?" and "what do I spawn?", 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 unchanged. PATHEXT spellings are now tried before the extensionless one, and that ordering is the fix rather than a detail: 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 would resolve to a path CreateProcessW cannot execute, turning a lookup miss into a spawn failure. Trying PATHEXT first selects the `.cmd`, which std routes through cmd.exe with the quoting added for CVE-2024-24576 -- so no hand-rolled `cmd /c`. DAP 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. The Windows branch is `#[cfg(windows)]` and so is not compiled by CI on Linux; `windows_tries_pathext_before_the_extensionless_name` asserts the ordering invariant on every platform by checking the suffix list's shape. The `.cmd` spawn itself still needs verification on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to the PATHEXT resolution fix, closing four gaps found in review.
Tree kill. Rust's std spawns a batch program as `cmd.exe /d /c "<shim> ..."`,
so once a server resolves to a `.cmd` the handle held is the wrapper and the
server itself is a grandchild. `Child::kill` is a bare `TerminateProcess`: it
took out the wrapper and left the server alive holding both pipe ends, so the
reader thread never saw EOF and every restart leaked another orphan. LSP and
DAP sessions now hold a `KILL_ON_JOB_CLOSE` Job Object, the guard ConPTY
children already had — moved from `pty/job.rs` to `modules/job.rs` and shared
rather than duplicated. Both also `wait()` after killing, to reap.
Empty PATHEXT. A set-but-empty `PATHEXT` reads back as `Some("")`, not absent,
so the `.COM;.EXE;.BAT;.CMD` default never fired and the suffix list collapsed
to the extensionless spelling — silently disabling the `.cmd` lookup this all
exists for. Parsing is now a pure function, so its ordering and fallback are
tested on every platform instead of only on a Windows runner.
Names that are paths. The separator branch skipped the suffix walk, so a
program given as a path — which the debugger panel's free-text field invites —
matched npm's extensionless bash script sitting next to the shim, resolving to
something `CreateProcessW` cannot run.
Unix executability. `mode & 0o111 != 0` accepts an exec bit belonging to
somebody else. Harmless while the answer was a bool; wrong once the answer is
the path being spawned, since `execvp` skips an EACCES match and keeps walking
PATH. Now `access(X_OK)`.
Also removes a stray `test/test.c` scratch file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Installing
vscode-langservers-extractedgave Nexis a working CSS/HTML/JSON language server that it still refused to start, and the missing-tools notice told the story backwards: pressing refresh cleared the entry, then opening a.cssfile put it straight back.The two sides were asking different questions.
tool_probe(src-tauri/src/modules/tools.rs) walksPATHEXTon Windows on purpose — 1.25.0 added that precisely because these servers land as.cmdshims.LspSession::starthanded the bare name toproc::command, and Rust's Windows program resolution appends only.exe; it never consultsPATHEXT.So
vscode-css-language-serverresolved for the probe and did not exist for the spawn.The fix
The PATH walk now returns the resolved path instead of a bool:
Both the LSP and DAP clients resolve first and spawn that path. One walk answers "is it installed?" and "what do I spawn?", 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 unchanged.
The part that matters more than the original diagnosis
PATHEXT spellings are now tried before the extensionless one. npm's shim writer emits three files per bin —
foo.cmd,foo.ps1, and an extensionlessfoothat is a bash script for MSYS/Git Bash. The old suffix list put the extensionless spelling first, which was harmless for a boolean probe but would have made the resolver return a pathCreateProcessWcannot execute — converting a lookup miss into a spawn failure.Trying PATHEXT first selects the
.cmd, which Rust's std routes throughcmd.exewith the hardened quoting added for CVE-2024-24576. No hand-rolledcmd /c.Why DAP is in scope
Not pre-emptive: the adapter command is free text in the debugger panel, so anyone naming an npm-installed adapter (
js-debug-adapter) hits the identical gap.ml.rsandpython.rsare deliberately untouched — they spawn interpreters and absolute paths, not npm shims.Verification
Passing locally on Linux:
cargo test— 195 lib + 11 pitfall tripwire testscargo clippy --all-targets -- -D warnings— cleancargo fmt --check— cleansrc/lib/pitfall-guards.test.ts— 16 passedKnown gap: the Windows branch is
#[cfg(windows)], so CI on Linux never compiles it. Its body was type-checked and run standalone (yields[".COM", ".EXE", ".BAT", ".CMD", ""], the ordering the fix depends on), andwindows_tries_pathext_before_the_extensionless_nameasserts the ordering invariant on every platform by checking the suffix list's shape. The actual spawn of a.cmdshim is unverified until this runs on Windows — rebuild, open a.cssfile, confirm the missing-tools pill stays clear instead of reappearing.Also included
[Unreleased] / Fixed.docs/vault/decisions/program-resolution-before-spawn.mdrecording the PATHEXT/Commandasymmetry, the alternatives rejected, and why resolution lives in the callers rather than inproc::command.docs/vault/Home.mdthat claimeddecisions/was empty; two notes already existed.🤖 Generated with Claude Code