fix(desktop): fix Windows PATH clobber and .cmd shim EINVAL#2563
Open
wpfleger96 wants to merge 4 commits into
Open
fix(desktop): fix Windows PATH clobber and .cmd shim EINVAL#2563wpfleger96 wants to merge 4 commits into
wpfleger96 wants to merge 4 commits into
Conversation
On Windows, three interacting defects caused Claude Code and Codex
installs to fail with 'npm: command not found', installed adapters to
appear missing, and agent turns to fail with EINVAL.
1. install_shell_command() built PATH from managed-Node dirs +
login_shell_path(), but both are always None on Windows (no managed
Node for Windows; login_shell_path() returns None because Git Bash
returns POSIX paths that poison native children). With an empty
path_parts the npm install ran with PATH unset: install failed.
Fix: on Windows, append the inherited process PATH when no
login-shell PATH is available, after Buzz-managed dirs.
2. build_augmented_path() had the same omission: callers pass its
result to Command::env("PATH", …) which *replaces* the child PATH.
Without the inherited process PATH, agents and probes lose node/npm/
git entirely. Fix: same approach, gated on local context (home or
exe_parent supplied) to prevent manufacturing a PATH from ambient
state when no context is provided. Shared by both the install path
and the runtime/probe/launch path — the single shared helper is the
point of the fix so the two callers cannot drift again.
3. configure_runtime_cli() resolved the claude CLI and set
CLAUDE_CODE_EXECUTABLE unconditionally. On Windows resolve_command
returns claude.cmd (an npm shim); passing that path to CreateProcess
causes EINVAL — batch scripts cannot be exec'd directly. Fix: on
Windows, skip .cmd/.bat extensions so the adapter falls back to its
own PATH lookup.
Non-Windows behaviour is provably unchanged: the Windows-only blocks
are #[cfg(windows)]-gated; the exe_added refactor (replacing the inline
exe_parent.is_some() after exe_parent was consumed) is a pure rename
with identical semantics. All existing Unix tests pass unmodified.
Fixes #2327, Fixes #2342, Fixes #2397
Supersedes #2247, #2420, #2506, #2533 (PATH), #2495 (.cmd shim)
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Addresses Thufir pass-1 blockers on PR #2563: 1. is_batch_shim — pure path predicate (no cfg(windows) guard, no PATH/cache involvement). Replaces three Windows-only integration tests that were poisoned by the resolve_command cache from the preceding claude_spawn_uses_the_probed_cli_executable test. Covered by 6 pure predicate tests that run on every host: .cmd, .CMD, .bat, .BAT (.exe and no-extension are accepted). 2. compose_path_entries — pure PATH composition kernel (pub(crate)) in runtime/path.rs, called by both build_augmented_path and install_shell_command. Eliminates the drift hazard between the two paths. Inputs are already-split Vec<PathBuf>; split_paths/join_paths stay at wrapper boundaries. use_inherited computed as !had_shell_path && has_local_context && cfg!(windows) so non-Windows output is byte-identical to before. Covered by 7 pure compose_tests (managed-first ordering, login-suppresses-inherited, inherited-appended-last, empty inputs, install/runtime parity, unix-parity) plus un-vacuoused Windows integration test that asserts PATH env var is set and sentinel appears last. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ited Remove the duplicate is_batch_shim body from runtime.rs (was 2223 lines, over the 2216 cap) and re-export it from path.rs alongside compose_path_entries and should_use_inherited. agent_discovery.rs now calls should_use_inherited(had_login, true, cfg!(windows)) instead of the open-coded !had_login && cfg!(windows) — the shared pure predicate is the same expression at compile time, but it eliminates the last place the Windows policy could drift from the runtime wrapper. The cross-host policy matrix (should_use_inherited tests in compose_tests) already covers all four cases; the wrapper_policy_parity test exercises all of them explicitly. No new tests needed — the pure-function coverage is already exhaustive and runs on every host. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
… should_skip_claude_executable is_batch_shim is a host-independent predicate; calling it unconditionally made configure_runtime_cli silently skip .cmd/.bat on macOS/Linux, which are valid executables there (not CreateProcess batch shims). Wrap the production guard in should_skip_claude_executable(path, is_windows) — the same pure-policy pattern used by should_use_inherited — so non-Windows behavior is byte-identical to main and the OS gate is testable cross-host. Four new cross-host tests cover all four OS × shim-type combinations: shim+windows → skip, shim+non-windows → assign, exe/no-ext both → assign. Also rename wrapper_policy_parity_install_and_runtime_agree to should_use_inherited_policy_truth_table: the old name overclaimed structural wrapper parity; the test is an exhaustive policy truth table, which is what it says now. runtime/tests.rs batch-shim tests updated to use super::path::is_batch_shim directly (the pub(crate) re-export of is_batch_shim from runtime.rs was removed since is_batch_shim is no longer called from production code — only should_skip_claude_executable wraps it). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.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.
What
Three interacting defects caused Claude Code and Codex installs to fail on Windows with 'npm: command not found', installed adapters to appear missing, and agent turns to EINVAL-fail.
1. Install PATH clobber (
install_shell_command)install_shell_command()built PATH from managed-Node dirs +login_shell_path(), but both are alwaysNoneon Windows: no managed Node runtime for Windows exists yet, andlogin_shell_path()returnsNonebecause Git Bash returns POSIX colon-delimited paths that poison native children. With an emptypath_partsthe PATH was unset entirely, so the Git Bash install shell couldn't findnpm→ install failed.2. Probe/runtime PATH clobber (
build_augmented_path)Same omission in
build_augmented_path(). Callers pass its result toCommand::env("PATH", …)which replaces rather than extends the child's PATH. Without the inherited process PATH every readiness probe, version probe, and launched agent lostnode/npm/git— installed adapters appeared missing and agent turns failed with'node' is not recognized.3.
.cmdshim EINVAL (configure_runtime_cli)configure_runtime_cli()resolved the Claude CLI and setCLAUDE_CODE_EXECUTABLEunconditionally. On Windowsresolve_commandreturnsclaude.cmd(an npm shim); passing that path toCreateProcesscauses EINVAL — batch scripts cannot be exec'd directly by Node.Fix
is_batch_shim(path: &Path) -> bool— pure host-independent predicate inruntime/path.rs. Returnstruefor.cmd/.batextensions (case-insensitive). Used internally byshould_skip_claude_executableand tested directly viasuper::path::is_batch_shiminruntime/tests.rs.should_skip_claude_executable(path: &Path, is_windows: bool) -> bool— pure policy predicate inruntime/path.rs, re-exported fromruntime.rs. On Windows, skips.cmd/.batpaths forCLAUDE_CODE_EXECUTABLEso the adapter falls back to its own PATH lookup. On non-Windows,.cmd/.batare valid executables and must be assigned — the explicitis_windowsflag restores that behavior and makes the OS gate testable cross-host on macOS CI. Production call:should_skip_claude_executable(&cli_path, cfg!(windows)).should_use_inherited(had_shell_path, has_local_context, is_windows) -> bool— pure policy predicate inruntime/path.rs, re-exported fromruntime.rs. Bothbuild_augmented_pathandinstall_shell_commandcall it — the inherited-PATH policy lives in exactly one place and is testable cross-host.compose_path_entries(managed, login, inherited, use_inherited) -> Vec<PathBuf>— pure PATH composition kernel (pub(crate)) inruntime/path.rs, called by bothbuild_augmented_pathandinstall_shell_command. Eliminates the drift hazard between the two paths. Inputs are already-splitVec<PathBuf>;split_paths/join_pathsstay at wrapper boundaries.Non-Windows impact
None.
should_use_inheritedreturnsfalsewhenis_windows=false;should_skip_claude_executablereturnsfalsewhenis_windows=false. Both usecfg!(windows)at their production call sites, which is a compile-time constant — the Windows branches are dead-code-eliminated on non-Windows. Unix output is byte-identical to before.Tests
Pure, host-agnostic (run on all CI targets including macOS):
runtime/tests.rs: 6is_batch_shimpredicate tests viasuper::path::is_batch_shimruntime/path.rs compose_tests:should_use_inheritedtruth table (4 cases),compose_path_entriesordering, empty/unset inherited with Windows policy ON,should_skip_claude_executablepolicy matrix (4 tests covering shim+Windows→skip, shim+non-Windows→assign, exe/no-ext both ways)Platform-gated:
runtime/path.rs#[cfg(unix)]:unix_shell_path_output_unchanged_by_windows_fallback_logicruntime/path.rs#[cfg(windows)]: process-PATH append, shell-PATH-present guard, no-context guardagent_discovery.rs#[cfg(windows)]:test_install_shell_command_includes_process_path_on_windowsFixes #2327, Fixes #2342, Fixes #2397
Supersedes #2247, #2420, #2506, #2533 (PATH family), #2495 (.cmd shim)