From 8881376ef00885c761bf1a21686dcce7474947f4 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 23 Jul 2026 13:07:00 -0400 Subject: [PATCH 1/4] fix(desktop): fix Windows PATH clobber and .cmd shim EINVAL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 4 + .../src-tauri/src/commands/agent_discovery.rs | 54 +++++++- .../src-tauri/src/managed_agents/runtime.rs | 12 ++ .../src/managed_agents/runtime/path.rs | 131 +++++++++++++++++- .../src/managed_agents/runtime/tests.rs | 92 ++++++++++++ 5 files changed, 291 insertions(+), 2 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index c57e0e982e..a901d5010d 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -178,6 +178,10 @@ const overrides = new Map([ // team-instructions-first-class: ManagedAgentRecord fixture gains the new // team_id field (+1 line). ["src-tauri/src/managed_agents/readiness.rs", 1765], + // Windows PATH-correctness fix: 3 #[cfg(windows)] test functions covering + // .cmd shim rejection, .bat shim rejection, and .exe acceptance for + // configure_runtime_cli (fix #2397). Test-only growth; queued to split. + ["src-tauri/src/managed_agents/runtime/tests.rs", 1041], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index acc8f30ad3..9ea6523437 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -575,9 +575,22 @@ fn install_shell_command(command: &str) -> Result if let Some(managed_bin) = crate::managed_agents::buzz_managed_npm_bin_dir() { path_parts.push(managed_bin); } - if let Some(ref path) = crate::managed_agents::login_shell_path() { + let login_path = crate::managed_agents::login_shell_path(); + if let Some(ref path) = login_path { path_parts.extend(std::env::split_paths(path)); } + // On Windows, `login_shell_path()` always returns `None` because Git Bash + // returns POSIX-shaped colon-delimited paths that poison native children. + // `cmd.env("PATH", …)` replaces rather than extends, so without this the + // install shell loses node/npm and the install fails with + // `'npm' is not recognized`. Append the inherited process PATH last so + // Buzz-managed dirs (when present) keep precedence. + #[cfg(windows)] + if login_path.is_none() { + if let Some(proc_path) = std::env::var_os("PATH") { + path_parts.extend(std::env::split_paths(&proc_path)); + } + } if !path_parts.is_empty() { if let Ok(path) = std::env::join_paths(path_parts) { cmd.env("PATH", path); @@ -1296,6 +1309,45 @@ mod tests { ); } + /// On Windows, `install_shell_command` must set PATH to a value that + /// includes the inherited process PATH, so node/npm are visible inside + /// the install shell even when no managed Node runtime is present. + #[cfg(windows)] + #[test] + fn test_install_shell_command_includes_process_path_on_windows() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + // Plant a sentinel in the process PATH that the test can detect. + let sentinel = r"C:\TestSentinel\bin"; + std::env::set_var("PATH", sentinel); + + let result = super::install_shell_command("echo test"); + + match previous { + Some(p) => std::env::set_var("PATH", p), + None => std::env::remove_var("PATH"), + } + + let cmd = result.expect("install_shell_command must succeed on Windows with Git"); + let path_value = cmd + .get_envs() + .find(|(key, _)| *key == "PATH") + .and_then(|(_, val)| val) + .map(|v| v.to_string_lossy().into_owned()); + + // PATH may or may not be set (depends on whether managed dirs or + // sentinel contributed entries). If set, it must contain the sentinel. + if let Some(path) = path_value { + assert!( + path.contains(sentinel), + "install_shell_command PATH must include the inherited process PATH; got: {path}" + ); + } + // If PATH is not set, the child inherits it — also acceptable, but our + // fix guarantees at least the sentinel is reachable. The absence of + // PATH env override is safe because the child inherits the process PATH. + } + // ── Phase B: per-OS install commands ────────────────────────────────────── /// On non-Windows, cli_install_commands_for_os returns the default commands. diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index cef669ab25..364cb64432 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1602,6 +1602,18 @@ pub(crate) fn configure_runtime_cli( return; } if let Some(cli_path) = runtime.underlying_cli.and_then(resolve_command) { + // On Windows, `.cmd` and `.bat` files are batch shims — they cannot be + // passed directly to `CreateProcess` and cause EINVAL when the Claude + // adapter tries to spawn them (issue #2397). Skip setting + // `CLAUDE_CODE_EXECUTABLE` for shim paths so the adapter falls back to + // its own PATH lookup and finds the real binary instead. + #[cfg(windows)] + if let Some(ext) = cli_path.extension() { + let ext_lower = ext.to_string_lossy().to_lowercase(); + if ext_lower == "cmd" || ext_lower == "bat" { + return; + } + } command.env("CLAUDE_CODE_EXECUTABLE", cli_path); } } diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index c1baadda7f..069738892d 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -11,6 +11,10 @@ use std::path::PathBuf; /// 4. `nvm_bin` — nvm's default Node.js bin dir (if the user uses nvm) /// 5. exe parent dir — DMG sidecars under `Contents/MacOS/` /// 6. user's login-shell `PATH` — runtimes like node/python from other managers +/// 7. Windows only: the current process `PATH` (appended when no login-shell +/// PATH exists, because callers use `Command::env("PATH", …)` which +/// *replaces* the child's PATH — without this, the child loses node/npm/git +/// and every npm `.cmd` shim fails with `'node' is not recognized`) /// /// `shell_path` is the raw colon-delimited string from a login shell, so it is /// split into individual entries before joining. Pushing it as a single segment @@ -32,7 +36,8 @@ pub(in crate::managed_agents) fn build_augmented_path( // Only add managed runtime dirs when a home or executable context exists. // This keeps tests/utility callers that intentionally pass no local context // from manufacturing a PATH out of ambient platform dirs alone. - if home_added || exe_parent.is_some() { + let exe_added = exe_parent.is_some(); + if home_added || exe_added { if let Some(managed_npm_bin) = crate::managed_agents::buzz_managed_npm_bin_dir() { parts.push(managed_npm_bin); } @@ -46,9 +51,29 @@ pub(in crate::managed_agents) fn build_augmented_path( if let Some(parent) = exe_parent { parts.push(parent); } + // Track whether a login-shell PATH was provided; used by the Windows + // fallback below to avoid appending the process PATH when a shell PATH + // was already supplied (cfg-gated so the variable is not unused on Unix). + #[cfg(windows)] + let had_shell_path = shell_path.is_some(); if let Some(shell_path) = shell_path { parts.extend(std::env::split_paths(&shell_path)); } + + // On Windows, `login_shell_path()` always returns `None` because Git Bash + // reports POSIX colon-delimited paths that poison native children. Nothing + // above contributes the user's real Windows PATH, and `Command::env("PATH", + // …)` replaces rather than extends, so every child loses node/npm/git. + // Append the inherited process PATH here — after the Buzz-managed dirs so + // those still win — but only when there is local context (home or exe_parent + // was supplied) to prevent manufacturing a PATH from ambient state alone. + #[cfg(windows)] + if !had_shell_path && (home_added || exe_added) { + if let Some(proc_path) = std::env::var_os("PATH") { + parts.extend(std::env::split_paths(&proc_path)); + } + } + if parts.is_empty() { return None; } @@ -134,4 +159,108 @@ mod tests { assert!(result.starts_with("/home/user/.local/bin:"), "{result}"); assert!(result.ends_with(":/usr/local/bin"), "{result}"); } + + /// On Unix, supplying a `shell_path` must NOT trigger the Windows process-PATH + /// fallback — the output must be byte-identical to what it was before this + /// fix. (The `#[cfg(windows)]` block is dead on this platform, but the + /// `had_shell_path` variable introduced alongside it must not affect non-Windows + /// output.) + #[cfg(unix)] + #[test] + fn unix_shell_path_output_unchanged_by_windows_fallback_logic() { + let result = build_augmented_path( + Some(PathBuf::from("/home/user")), + None, + Some("/usr/local/bin:/usr/bin:/bin".to_string()), + None, + ); + let result = result.expect("path"); + // Must end exactly with the login-shell PATH — no ambient process PATH + // appended even though shell_path is set. + assert!( + result.ends_with(":/usr/local/bin:/usr/bin:/bin"), + "Unix output must not append process PATH: {result}" + ); + } + + /// On Windows: when no login-shell PATH is available, `build_augmented_path` + /// must append the inherited process PATH so node/npm remain visible. + /// + /// This test manipulates `std::env::var_os("PATH")` directly — it must hold + /// the `lock_path_mutex` to avoid racing with other tests. + #[cfg(windows)] + #[test] + fn windows_appends_process_path_when_no_shell_path() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + std::env::set_var("PATH", r"C:\Program Files\nodejs"); + + let result = build_augmented_path(Some(PathBuf::from(r"C:\Users\agent")), None, None, None); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + + let result = result.expect("path must not be None with a home dir"); + assert!( + result.starts_with(r"C:\Users\agent\.local\bin;"), + "home/.local/bin must be first: {result}" + ); + assert!( + result.ends_with(r";C:\Program Files\nodejs"), + "process PATH must be last: {result}" + ); + } + + /// On Windows: when a login-shell PATH IS supplied (hypothetically), the + /// process PATH must NOT also be appended — that would double the PATH. + #[cfg(windows)] + #[test] + fn windows_does_not_append_process_path_when_shell_path_present() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + std::env::set_var("PATH", r"C:\ShouldNotAppear"); + + let result = build_augmented_path( + Some(PathBuf::from(r"C:\Users\agent")), + None, + Some(r"C:\Program Files\nodejs".to_string()), + None, + ); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + + let result = result.expect("path"); + assert!( + !result.contains("ShouldNotAppear"), + "process PATH must not be appended when shell_path is present: {result}" + ); + } + + /// On Windows: when no local context is provided (home=None, exe_parent=None), + /// the function must return None even if the process PATH is set — callers + /// that pass no context must not get a PATH manufactured from ambient state. + #[cfg(windows)] + #[test] + fn windows_no_process_path_without_local_context() { + let _guard = crate::managed_agents::lock_path_mutex(); + let previous = std::env::var_os("PATH"); + std::env::set_var("PATH", r"C:\Windows\System32"); + + let result = build_augmented_path(None, None, None, None); + + match previous { + Some(value) => std::env::set_var("PATH", value), + None => std::env::remove_var("PATH"), + } + + assert_eq!( + result, None, + "must return None when no local context and no shell_path" + ); + } } diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index d86b69938f..fb6fc8172c 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -618,6 +618,98 @@ fn codex_spawn_does_not_set_a_claude_executable() { .any(|(key, _)| key == "CLAUDE_CODE_EXECUTABLE")); } +/// On Windows, `.cmd` and `.bat` batch shims must NOT be assigned to +/// `CLAUDE_CODE_EXECUTABLE` — `CreateProcess` cannot exec them directly and +/// returns EINVAL (issue #2397). The adapter must fall back to its own PATH +/// lookup instead. +#[cfg(windows)] +#[test] +fn windows_cmd_shim_does_not_set_claude_code_executable() { + let _guard = crate::managed_agents::lock_path_mutex(); + let temp = tempfile::tempdir().expect("temp dir"); + + // Write a `.cmd` shim (no execute bit needed on Windows). + let shim = temp.path().join("claude.cmd"); + std::fs::write(&shim, "@echo off\r\necho shim\r\n").expect("write shim"); + + let original_path = std::env::var_os("PATH"); + std::env::set_var("PATH", temp.path()); + + let mut command = std::process::Command::new("buzz-acp"); + super::configure_runtime_cli(&mut command, super::known_acp_runtime("claude-agent-acp")); + + match original_path { + Some(p) => std::env::set_var("PATH", p), + None => std::env::remove_var("PATH"), + } + + assert!( + !command + .get_envs() + .any(|(key, _)| key == "CLAUDE_CODE_EXECUTABLE"), + "a .cmd shim must not be assigned to CLAUDE_CODE_EXECUTABLE (EINVAL on CreateProcess)" + ); +} + +/// On Windows, a `.bat` shim is equally unsafe — same EINVAL path as `.cmd`. +#[cfg(windows)] +#[test] +fn windows_bat_shim_does_not_set_claude_code_executable() { + let _guard = crate::managed_agents::lock_path_mutex(); + let temp = tempfile::tempdir().expect("temp dir"); + + let shim = temp.path().join("claude.bat"); + std::fs::write(&shim, "@echo off\r\necho shim\r\n").expect("write shim"); + + let original_path = std::env::var_os("PATH"); + std::env::set_var("PATH", temp.path()); + + let mut command = std::process::Command::new("buzz-acp"); + super::configure_runtime_cli(&mut command, super::known_acp_runtime("claude-agent-acp")); + + match original_path { + Some(p) => std::env::set_var("PATH", p), + None => std::env::remove_var("PATH"), + } + + assert!( + !command + .get_envs() + .any(|(key, _)| key == "CLAUDE_CODE_EXECUTABLE"), + "a .bat shim must not be assigned to CLAUDE_CODE_EXECUTABLE" + ); +} + +/// On Windows, a real `.exe` binary IS safe to assign — it does not trigger +/// the shim-rejection path. +#[cfg(windows)] +#[test] +fn windows_exe_sets_claude_code_executable() { + let _guard = crate::managed_agents::lock_path_mutex(); + let temp = tempfile::tempdir().expect("temp dir"); + + let exe = temp.path().join("claude.exe"); + std::fs::write(&exe, "").expect("write fake exe"); + + let original_path = std::env::var_os("PATH"); + std::env::set_var("PATH", temp.path()); + + let mut command = std::process::Command::new("buzz-acp"); + super::configure_runtime_cli(&mut command, super::known_acp_runtime("claude-agent-acp")); + + match original_path { + Some(p) => std::env::set_var("PATH", p), + None => std::env::remove_var("PATH"), + } + + assert!( + command + .get_envs() + .any(|(key, value)| key == "CLAUDE_CODE_EXECUTABLE" && value == Some(exe.as_os_str())), + "a real .exe must still be assigned to CLAUDE_CODE_EXECUTABLE" + ); +} + // ── PGID-based orphan sweep tests ─────────────────────────────────────── /// Validates the kernel invariant that the orphan sweep PGID fix relies on: From 535c50e49fe654186fa5027ea19955f0f54fe315 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 23 Jul 2026 13:37:37 -0400 Subject: [PATCH 2/4] fix(desktop): extract pure PATH composer and batch-shim predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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; 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 Signed-off-by: Will Pfleger --- .../src-tauri/src/commands/agent_discovery.rs | 73 +++---- .../src-tauri/src/managed_agents/runtime.rs | 23 ++- .../src/managed_agents/runtime/path.rs | 184 ++++++++++++++++-- .../src/managed_agents/runtime/tests.rs | 105 ++++------ 4 files changed, 253 insertions(+), 132 deletions(-) diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 9ea6523437..d44d8a93e6 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -568,29 +568,32 @@ fn install_shell_command(command: &str) -> Result cmd.env("npm_config_cache", prefix.join("cache")); } - let mut path_parts = Vec::new(); - if let Some(managed_node_bin) = crate::managed_agents::buzz_managed_node_bin_dir() { - path_parts.push(managed_node_bin); - } - if let Some(managed_bin) = crate::managed_agents::buzz_managed_npm_bin_dir() { - path_parts.push(managed_bin); - } + // Compose the PATH for the install shell using the same kernel as the + // runtime/probe path so the two can never drift. managed entries first + // (Node/npm bins keep precedence); login-shell entries next; inherited + // process PATH appended last on Windows when no login-shell PATH exists + // (login_shell_path() always returns None on Windows — Git Bash paths are + // POSIX-shaped and poison native children; cmd.env("PATH", …) replaces + // rather than extends, so without inherited the install shell loses npm). let login_path = crate::managed_agents::login_shell_path(); - if let Some(ref path) = login_path { - path_parts.extend(std::env::split_paths(path)); - } - // On Windows, `login_shell_path()` always returns `None` because Git Bash - // returns POSIX-shaped colon-delimited paths that poison native children. - // `cmd.env("PATH", …)` replaces rather than extends, so without this the - // install shell loses node/npm and the install fails with - // `'npm' is not recognized`. Append the inherited process PATH last so - // Buzz-managed dirs (when present) keep precedence. - #[cfg(windows)] - if login_path.is_none() { - if let Some(proc_path) = std::env::var_os("PATH") { - path_parts.extend(std::env::split_paths(&proc_path)); - } - } + let had_login = login_path.is_some(); + let managed: Vec = [ + crate::managed_agents::buzz_managed_node_bin_dir(), + crate::managed_agents::buzz_managed_npm_bin_dir(), + ] + .into_iter() + .flatten() + .collect(); + let login: Vec = login_path + .as_deref() + .map(|p| std::env::split_paths(p).collect()) + .unwrap_or_default(); + let inherited: Vec = std::env::var_os("PATH") + .map(|p| std::env::split_paths(&p).collect()) + .unwrap_or_default(); + let use_inherited = !had_login && cfg!(windows); + let path_parts = + crate::managed_agents::compose_path_entries(managed, login, inherited, use_inherited); if !path_parts.is_empty() { if let Ok(path) = std::env::join_paths(path_parts) { cmd.env("PATH", path); @@ -1333,19 +1336,19 @@ mod tests { .get_envs() .find(|(key, _)| *key == "PATH") .and_then(|(_, val)| val) - .map(|v| v.to_string_lossy().into_owned()); - - // PATH may or may not be set (depends on whether managed dirs or - // sentinel contributed entries). If set, it must contain the sentinel. - if let Some(path) = path_value { - assert!( - path.contains(sentinel), - "install_shell_command PATH must include the inherited process PATH; got: {path}" - ); - } - // If PATH is not set, the child inherits it — also acceptable, but our - // fix guarantees at least the sentinel is reachable. The absence of - // PATH env override is safe because the child inherits the process PATH. + .map(|v| v.to_string_lossy().into_owned()) + .expect("install_shell_command must always set a PATH env var on Windows"); + + // The sentinel (inherited process PATH) must appear in the composed PATH. + assert!( + path_value.contains(sentinel), + "install_shell_command PATH must include the inherited process PATH; got: {path_value}" + ); + // The sentinel must appear LAST — managed Buzz dirs must have precedence. + assert!( + path_value.ends_with(sentinel), + "inherited process PATH must be appended LAST so managed dirs keep precedence; got: {path_value}" + ); } // ── Phase B: per-OS install commands ────────────────────────────────────── diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 364cb64432..38b505cc08 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -16,6 +16,7 @@ use crate::{ mod path; pub(in crate::managed_agents) use path::build_augmented_path; +pub(crate) use path::compose_path_entries; mod stop; pub(crate) use stop::managed_agent_runtime_keys; @@ -1607,17 +1608,27 @@ pub(crate) fn configure_runtime_cli( // adapter tries to spawn them (issue #2397). Skip setting // `CLAUDE_CODE_EXECUTABLE` for shim paths so the adapter falls back to // its own PATH lookup and finds the real binary instead. - #[cfg(windows)] - if let Some(ext) = cli_path.extension() { - let ext_lower = ext.to_string_lossy().to_lowercase(); - if ext_lower == "cmd" || ext_lower == "bat" { - return; - } + if is_batch_shim(&cli_path) { + return; } command.env("CLAUDE_CODE_EXECUTABLE", cli_path); } } +/// Return `true` when `path` is a Windows batch shim (`.cmd` or `.bat`, +/// case-insensitive) that cannot be passed directly to `CreateProcess`. +/// +/// Extracted as a pure function so it can be unit-tested on any host without +/// touching the global PATH or `resolve_command` cache (issue #2397). +pub(crate) fn is_batch_shim(path: &std::path::Path) -> bool { + path.extension() + .map(|ext| { + let lower = ext.to_string_lossy().to_lowercase(); + lower == "cmd" || lower == "bat" + }) + .unwrap_or(false) +} + /// Spawn an agent process without holding any locks on records or runtimes. /// Returns the child process and log path on success. The caller is responsible /// for updating `ManagedAgentRecord` fields and inserting into the runtimes map. diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index 069738892d..2cd49e6371 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -2,6 +2,32 @@ use std::path::PathBuf; +/// Pure PATH composition kernel shared by the install shell and the runtime/probe paths. +/// +/// Merges already-split PATH entries in precedence order: +/// 1. `managed` — Buzz-controlled dirs (highest precedence, e.g. managed Node/npm bins) +/// 2. `login` — login-shell PATH entries (split before calling) +/// 3. `inherited` — current-process PATH entries (split before calling), appended +/// only when `use_inherited` is `true` +/// +/// Callers are responsible for splitting raw PATH strings and for prepending any +/// additional prefix entries (e.g. `home/.local/bin`, `nvm`, `exe_parent`) before +/// passing them in `managed`. `split_paths`/`join_paths` are kept at the wrapper +/// boundaries so this function remains fully pure and testable on any host. +pub(crate) fn compose_path_entries( + managed: Vec, + login: Vec, + inherited: Vec, + use_inherited: bool, +) -> Vec { + let mut parts = managed; + parts.extend(login); + if use_inherited { + parts.extend(inherited); + } + parts +} + /// Assemble the augmented `PATH` for a launched managed-agent child process. /// /// Concatenates, in priority order: @@ -28,37 +54,39 @@ pub(in crate::managed_agents) fn build_augmented_path( shell_path: Option, nvm_bin: Option, ) -> Option { - let mut parts: Vec = Vec::new(); let home_added = home.is_some(); + let exe_added = exe_parent.is_some(); + let has_local_context = home_added || exe_added; + + // Build the managed/prefix entries (everything before login-shell PATH). + let mut managed: Vec = Vec::new(); if let Some(home) = home { - parts.push(home.join(".local").join("bin")); + managed.push(home.join(".local").join("bin")); } // Only add managed runtime dirs when a home or executable context exists. // This keeps tests/utility callers that intentionally pass no local context // from manufacturing a PATH out of ambient platform dirs alone. - let exe_added = exe_parent.is_some(); - if home_added || exe_added { + if has_local_context { if let Some(managed_npm_bin) = crate::managed_agents::buzz_managed_npm_bin_dir() { - parts.push(managed_npm_bin); + managed.push(managed_npm_bin); } if let Some(managed_node_bin) = crate::managed_agents::buzz_managed_node_bin_dir() { - parts.push(managed_node_bin); + managed.push(managed_node_bin); } } if let Some(nvm_bin) = nvm_bin { - parts.push(nvm_bin); + managed.push(nvm_bin); } if let Some(parent) = exe_parent { - parts.push(parent); + managed.push(parent); } - // Track whether a login-shell PATH was provided; used by the Windows - // fallback below to avoid appending the process PATH when a shell PATH - // was already supplied (cfg-gated so the variable is not unused on Unix). - #[cfg(windows)] + + // Split the login-shell PATH into individual entries. let had_shell_path = shell_path.is_some(); - if let Some(shell_path) = shell_path { - parts.extend(std::env::split_paths(&shell_path)); - } + let login: Vec = shell_path + .as_deref() + .map(|s| std::env::split_paths(s).collect()) + .unwrap_or_default(); // On Windows, `login_shell_path()` always returns `None` because Git Bash // reports POSIX colon-delimited paths that poison native children. Nothing @@ -67,13 +95,16 @@ pub(in crate::managed_agents) fn build_augmented_path( // Append the inherited process PATH here — after the Buzz-managed dirs so // those still win — but only when there is local context (home or exe_parent // was supplied) to prevent manufacturing a PATH from ambient state alone. - #[cfg(windows)] - if !had_shell_path && (home_added || exe_added) { - if let Some(proc_path) = std::env::var_os("PATH") { - parts.extend(std::env::split_paths(&proc_path)); - } - } + let inherited: Vec = std::env::var_os("PATH") + .map(|p| std::env::split_paths(&p).collect()) + .unwrap_or_default(); + // `use_inherited`: Windows-only policy — append process PATH when no + // login-shell PATH was available and there is local context. On non-Windows + // platforms this is always false, making the inherited entries a dead weight + // that compose_path_entries simply drops; the compiler eliminates the branch. + let use_inherited = !had_shell_path && has_local_context && cfg!(windows); + let parts = compose_path_entries(managed, login, inherited, use_inherited); if parts.is_empty() { return None; } @@ -264,3 +295,114 @@ mod tests { ); } } + +// ── Pure compose_path_entries tests — cover the Windows policy matrix on any host ── +// +// These test the composition kernel directly with explicit inputs, so they run +// on macOS/Linux CI and validate the Windows `use_inherited` behavior without +// touching process state or needing a Windows target. +#[cfg(test)] +mod compose_tests { + use super::compose_path_entries; + use std::path::PathBuf; + + fn p(s: &str) -> PathBuf { + PathBuf::from(s) + } + + #[test] + fn managed_entries_appear_first() { + let managed = vec![p("/buzz/node/bin"), p("/buzz/npm/bin")]; + let login = vec![p("/usr/local/bin"), p("/usr/bin")]; + let result = compose_path_entries(managed, login, vec![], false); + assert_eq!(result[0], p("/buzz/node/bin"), "managed[0] must be first"); + assert_eq!(result[1], p("/buzz/npm/bin"), "managed[1] must be second"); + assert_eq!( + result[2], + p("/usr/local/bin"), + "login[0] must follow managed" + ); + } + + #[test] + fn login_path_suppresses_inherited_when_use_inherited_false() { + let login = vec![p("/usr/local/bin")]; + let inherited = vec![p("/should/not/appear")]; + let result = compose_path_entries(vec![], login, inherited, false); + assert!( + !result.contains(&p("/should/not/appear")), + "inherited must not appear when use_inherited=false" + ); + } + + #[test] + fn inherited_appended_last_when_use_inherited_true() { + let managed = vec![p("/buzz/npm/bin")]; + let login = vec![]; + let inherited = vec![p("C:/windows/node"), p("C:/windows/npm")]; + let result = compose_path_entries(managed, login, inherited.clone(), true); + assert_eq!(result[0], p("/buzz/npm/bin"), "managed must be first"); + assert_eq!( + &result[1..], + &inherited[..], + "inherited entries must be appended last" + ); + } + + #[test] + fn empty_managed_and_login_with_inherited_appended() { + let inherited = vec![p("C:/windows/system32"), p("C:/windows")]; + let result = compose_path_entries(vec![], vec![], inherited.clone(), true); + assert_eq!( + result, inherited, + "only inherited entries when others are empty" + ); + } + + #[test] + fn empty_all_inputs_returns_empty() { + let result = compose_path_entries(vec![], vec![], vec![], false); + assert!( + result.is_empty(), + "all-empty inputs must produce empty output" + ); + } + + #[test] + fn install_runtime_parity_same_kernel() { + // Both install and runtime paths share compose_path_entries. Verify that + // two callers with identical inputs produce identical output — the drift + // that upstream PRs #2247 vs #2533 introduced cannot happen here. + let managed = vec![p("/buzz/node/bin"), p("/buzz/npm/bin")]; + let inherited = vec![p("C:/win/node")]; + let install_result = compose_path_entries(managed.clone(), vec![], inherited.clone(), true); + let runtime_result = compose_path_entries(managed.clone(), vec![], inherited.clone(), true); + assert_eq!( + install_result, runtime_result, + "install and runtime callers with identical inputs must produce identical PATH" + ); + } + + /// Non-Windows behavior: `use_inherited=false` (what the runtime passes on Unix) + /// must produce byte-identical output to what existed before this fix. + /// The inherited entries are collected but never appended — they are dead weight + /// that compose_path_entries drops. + #[cfg(unix)] + #[test] + fn unix_use_inherited_false_output_unchanged() { + let managed = vec![p("/buzz/npm/bin")]; + let login = vec![p("/usr/local/bin"), p("/usr/bin"), p("/bin")]; + let inherited = vec![p("/proc/ambient/PATH")]; // would be real proc PATH on Unix + let result = compose_path_entries(managed, login, inherited, false); + assert_eq!( + result, + vec![ + p("/buzz/npm/bin"), + p("/usr/local/bin"), + p("/usr/bin"), + p("/bin") + ], + "Unix output must not include inherited entries when use_inherited=false" + ); + } +} diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index fb6fc8172c..cda14fb32f 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -622,91 +622,56 @@ fn codex_spawn_does_not_set_a_claude_executable() { /// `CLAUDE_CODE_EXECUTABLE` — `CreateProcess` cannot exec them directly and /// returns EINVAL (issue #2397). The adapter must fall back to its own PATH /// lookup instead. -#[cfg(windows)] +/// +/// These tests exercise `is_batch_shim` directly — a pure path predicate with +/// no global PATH or resolve_command cache involvement — so they run on every +/// host and cannot be poisoned by the `claude_spawn_uses_the_probed_cli_executable` +/// test that runs before them. #[test] -fn windows_cmd_shim_does_not_set_claude_code_executable() { - let _guard = crate::managed_agents::lock_path_mutex(); - let temp = tempfile::tempdir().expect("temp dir"); - - // Write a `.cmd` shim (no execute bit needed on Windows). - let shim = temp.path().join("claude.cmd"); - std::fs::write(&shim, "@echo off\r\necho shim\r\n").expect("write shim"); - - let original_path = std::env::var_os("PATH"); - std::env::set_var("PATH", temp.path()); - - let mut command = std::process::Command::new("buzz-acp"); - super::configure_runtime_cli(&mut command, super::known_acp_runtime("claude-agent-acp")); - - match original_path { - Some(p) => std::env::set_var("PATH", p), - None => std::env::remove_var("PATH"), - } - +fn batch_shim_cmd_extension_is_rejected() { assert!( - !command - .get_envs() - .any(|(key, _)| key == "CLAUDE_CODE_EXECUTABLE"), - "a .cmd shim must not be assigned to CLAUDE_CODE_EXECUTABLE (EINVAL on CreateProcess)" + super::is_batch_shim(std::path::Path::new("claude.cmd")), + "claude.cmd must be identified as a batch shim" ); } -/// On Windows, a `.bat` shim is equally unsafe — same EINVAL path as `.cmd`. -#[cfg(windows)] #[test] -fn windows_bat_shim_does_not_set_claude_code_executable() { - let _guard = crate::managed_agents::lock_path_mutex(); - let temp = tempfile::tempdir().expect("temp dir"); - - let shim = temp.path().join("claude.bat"); - std::fs::write(&shim, "@echo off\r\necho shim\r\n").expect("write shim"); - - let original_path = std::env::var_os("PATH"); - std::env::set_var("PATH", temp.path()); - - let mut command = std::process::Command::new("buzz-acp"); - super::configure_runtime_cli(&mut command, super::known_acp_runtime("claude-agent-acp")); - - match original_path { - Some(p) => std::env::set_var("PATH", p), - None => std::env::remove_var("PATH"), - } - +fn batch_shim_cmd_extension_uppercase_is_rejected() { assert!( - !command - .get_envs() - .any(|(key, _)| key == "CLAUDE_CODE_EXECUTABLE"), - "a .bat shim must not be assigned to CLAUDE_CODE_EXECUTABLE" + super::is_batch_shim(std::path::Path::new("claude.CMD")), + "claude.CMD must be identified as a batch shim (case-insensitive)" ); } -/// On Windows, a real `.exe` binary IS safe to assign — it does not trigger -/// the shim-rejection path. -#[cfg(windows)] #[test] -fn windows_exe_sets_claude_code_executable() { - let _guard = crate::managed_agents::lock_path_mutex(); - let temp = tempfile::tempdir().expect("temp dir"); - - let exe = temp.path().join("claude.exe"); - std::fs::write(&exe, "").expect("write fake exe"); - - let original_path = std::env::var_os("PATH"); - std::env::set_var("PATH", temp.path()); +fn batch_shim_bat_extension_is_rejected() { + assert!( + super::is_batch_shim(std::path::Path::new("claude.bat")), + "claude.bat must be identified as a batch shim" + ); +} - let mut command = std::process::Command::new("buzz-acp"); - super::configure_runtime_cli(&mut command, super::known_acp_runtime("claude-agent-acp")); +#[test] +fn batch_shim_bat_extension_uppercase_is_rejected() { + assert!( + super::is_batch_shim(std::path::Path::new("claude.BAT")), + "claude.BAT must be identified as a batch shim (case-insensitive)" + ); +} - match original_path { - Some(p) => std::env::set_var("PATH", p), - None => std::env::remove_var("PATH"), - } +#[test] +fn batch_shim_exe_extension_is_not_rejected() { + assert!( + !super::is_batch_shim(std::path::Path::new("claude.exe")), + "claude.exe must not be identified as a batch shim" + ); +} +#[test] +fn batch_shim_no_extension_is_not_rejected() { assert!( - command - .get_envs() - .any(|(key, value)| key == "CLAUDE_CODE_EXECUTABLE" && value == Some(exe.as_os_str())), - "a real .exe must still be assigned to CLAUDE_CODE_EXECUTABLE" + !super::is_batch_shim(std::path::Path::new("claude")), + "claude (no extension) must not be identified as a batch shim" ); } From dacba771452151612366662211654ba912359755 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 23 Jul 2026 14:02:15 -0400 Subject: [PATCH 3/4] fix(desktop): move is_batch_shim to path.rs and wire should_use_inherited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Will Pfleger --- .../src-tauri/src/commands/agent_discovery.rs | 2 +- .../src-tauri/src/managed_agents/runtime.rs | 16 +- .../src/managed_agents/runtime/path.rs | 243 ++++++++++++++---- 3 files changed, 191 insertions(+), 70 deletions(-) diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index d44d8a93e6..40c4333a11 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -591,7 +591,7 @@ fn install_shell_command(command: &str) -> Result let inherited: Vec = std::env::var_os("PATH") .map(|p| std::env::split_paths(&p).collect()) .unwrap_or_default(); - let use_inherited = !had_login && cfg!(windows); + let use_inherited = crate::managed_agents::should_use_inherited(had_login, true, cfg!(windows)); let path_parts = crate::managed_agents::compose_path_entries(managed, login, inherited, use_inherited); if !path_parts.is_empty() { diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 38b505cc08..734b1476d3 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -17,6 +17,8 @@ use crate::{ mod path; pub(in crate::managed_agents) use path::build_augmented_path; pub(crate) use path::compose_path_entries; +pub(crate) use path::is_batch_shim; +pub(crate) use path::should_use_inherited; mod stop; pub(crate) use stop::managed_agent_runtime_keys; @@ -1615,20 +1617,6 @@ pub(crate) fn configure_runtime_cli( } } -/// Return `true` when `path` is a Windows batch shim (`.cmd` or `.bat`, -/// case-insensitive) that cannot be passed directly to `CreateProcess`. -/// -/// Extracted as a pure function so it can be unit-tested on any host without -/// touching the global PATH or `resolve_command` cache (issue #2397). -pub(crate) fn is_batch_shim(path: &std::path::Path) -> bool { - path.extension() - .map(|ext| { - let lower = ext.to_string_lossy().to_lowercase(); - lower == "cmd" || lower == "bat" - }) - .unwrap_or(false) -} - /// Spawn an agent process without holding any locks on records or runtimes. /// Returns the child process and log path on success. The caller is responsible /// for updating `ManagedAgentRecord` fields and inserting into the runtimes map. diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index 2cd49e6371..cffb081671 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -2,6 +2,49 @@ use std::path::PathBuf; +/// Return `true` when `path` is a Windows batch shim (`.cmd` or `.bat`, +/// case-insensitive) that cannot be passed directly to `CreateProcess`. +/// +/// Extracted as a pure function so it can be unit-tested on any host without +/// touching the global PATH or `resolve_command` cache (issue #2397). +pub(crate) fn is_batch_shim(path: &std::path::Path) -> bool { + path.extension() + .map(|ext| { + let lower = ext.to_string_lossy().to_lowercase(); + lower == "cmd" || lower == "bat" + }) + .unwrap_or(false) +} + +/// Decide whether the inherited process PATH should be appended to the +/// composed PATH. +/// +/// On Windows, `login_shell_path()` always returns `None` because Git Bash +/// returns POSIX colon-delimited paths that poison native children. +/// `Command::env("PATH", …)` replaces rather than extends, so without the +/// inherited PATH every child loses node/npm/git. +/// +/// This pure function takes an explicit `is_windows` flag so it can be +/// unit-tested cross-host (macOS CI can pass `true` to exercise the Windows +/// policy without needing the `cfg!(windows)` target). +/// +/// Rules: +/// - Only append when `is_windows` — on Unix the login-shell PATH always covers +/// the needed runtimes. +/// - Suppress when `had_shell_path` is `true` — if a login-shell PATH was +/// supplied it already carries the user's native entries; appending the +/// process PATH would double them. +/// - Suppress when `has_local_context` is `false` — callers that pass no home +/// or exe-parent context must not receive a PATH manufactured from ambient +/// process state alone. +pub(crate) fn should_use_inherited( + had_shell_path: bool, + has_local_context: bool, + is_windows: bool, +) -> bool { + is_windows && !had_shell_path && has_local_context +} + /// Pure PATH composition kernel shared by the install shell and the runtime/probe paths. /// /// Merges already-split PATH entries in precedence order: @@ -88,21 +131,10 @@ pub(in crate::managed_agents) fn build_augmented_path( .map(|s| std::env::split_paths(s).collect()) .unwrap_or_default(); - // On Windows, `login_shell_path()` always returns `None` because Git Bash - // reports POSIX colon-delimited paths that poison native children. Nothing - // above contributes the user's real Windows PATH, and `Command::env("PATH", - // …)` replaces rather than extends, so every child loses node/npm/git. - // Append the inherited process PATH here — after the Buzz-managed dirs so - // those still win — but only when there is local context (home or exe_parent - // was supplied) to prevent manufacturing a PATH from ambient state alone. let inherited: Vec = std::env::var_os("PATH") .map(|p| std::env::split_paths(&p).collect()) .unwrap_or_default(); - // `use_inherited`: Windows-only policy — append process PATH when no - // login-shell PATH was available and there is local context. On non-Windows - // platforms this is always false, making the inherited entries a dead weight - // that compose_path_entries simply drops; the compiler eliminates the branch. - let use_inherited = !had_shell_path && has_local_context && cfg!(windows); + let use_inherited = should_use_inherited(had_shell_path, has_local_context, cfg!(windows)); let parts = compose_path_entries(managed, login, inherited, use_inherited); if parts.is_empty() { @@ -193,9 +225,7 @@ mod tests { /// On Unix, supplying a `shell_path` must NOT trigger the Windows process-PATH /// fallback — the output must be byte-identical to what it was before this - /// fix. (The `#[cfg(windows)]` block is dead on this platform, but the - /// `had_shell_path` variable introduced alongside it must not affect non-Windows - /// output.) + /// fix. #[cfg(unix)] #[test] fn unix_shell_path_output_unchanged_by_windows_fallback_logic() { @@ -206,8 +236,6 @@ mod tests { None, ); let result = result.expect("path"); - // Must end exactly with the login-shell PATH — no ambient process PATH - // appended even though shell_path is set. assert!( result.ends_with(":/usr/local/bin:/usr/bin:/bin"), "Unix output must not append process PATH: {result}" @@ -216,9 +244,6 @@ mod tests { /// On Windows: when no login-shell PATH is available, `build_augmented_path` /// must append the inherited process PATH so node/npm remain visible. - /// - /// This test manipulates `std::env::var_os("PATH")` directly — it must hold - /// the `lock_path_mutex` to avoid racing with other tests. #[cfg(windows)] #[test] fn windows_appends_process_path_when_no_shell_path() { @@ -244,8 +269,8 @@ mod tests { ); } - /// On Windows: when a login-shell PATH IS supplied (hypothetically), the - /// process PATH must NOT also be appended — that would double the PATH. + /// On Windows: when a login-shell PATH IS supplied, the process PATH must + /// NOT also be appended. #[cfg(windows)] #[test] fn windows_does_not_append_process_path_when_shell_path_present() { @@ -272,9 +297,8 @@ mod tests { ); } - /// On Windows: when no local context is provided (home=None, exe_parent=None), - /// the function must return None even if the process PATH is set — callers - /// that pass no context must not get a PATH manufactured from ambient state. + /// On Windows: when no local context is provided, the function must return + /// None even if the process PATH is set. #[cfg(windows)] #[test] fn windows_no_process_path_without_local_context() { @@ -296,20 +320,64 @@ mod tests { } } -// ── Pure compose_path_entries tests — cover the Windows policy matrix on any host ── +// ── Pure policy and composition tests — run on every host ──────────────────── // -// These test the composition kernel directly with explicit inputs, so they run -// on macOS/Linux CI and validate the Windows `use_inherited` behavior without -// touching process state or needing a Windows target. +// These test `should_use_inherited` and `compose_path_entries` with explicit +// inputs, so they run on macOS/Linux CI and validate the Windows policy +// behavior without touching process state or requiring a Windows target. #[cfg(test)] mod compose_tests { - use super::compose_path_entries; - use std::path::PathBuf; + use super::{compose_path_entries, is_batch_shim, should_use_inherited}; + use std::path::{Path, PathBuf}; fn p(s: &str) -> PathBuf { PathBuf::from(s) } + // ── should_use_inherited policy matrix ──────────────────────────────────── + + /// Windows + no shell path + has local context → must use inherited. + #[test] + fn policy_windows_no_shell_with_context_uses_inherited() { + assert!( + should_use_inherited(false, true, true), + "Windows, no shell path, has context → must append inherited" + ); + } + + /// Windows + shell path present → must NOT use inherited (login path covers it). + #[test] + fn policy_windows_shell_path_present_suppresses_inherited() { + assert!( + !should_use_inherited(true, true, true), + "Windows, shell path present → must not append inherited" + ); + } + + /// Windows + no local context → must NOT use inherited (no ambient state). + #[test] + fn policy_windows_no_local_context_suppresses_inherited() { + assert!( + !should_use_inherited(false, false, true), + "Windows, no local context → must not append inherited" + ); + } + + /// Non-Windows → never use inherited, regardless of other flags. + #[test] + fn policy_non_windows_never_uses_inherited() { + assert!( + !should_use_inherited(false, true, false), + "non-Windows must never append inherited PATH" + ); + assert!( + !should_use_inherited(false, false, false), + "non-Windows + no context must never append inherited PATH" + ); + } + + // ── compose_path_entries ordering ───────────────────────────────────────── + #[test] fn managed_entries_appear_first() { let managed = vec![p("/buzz/node/bin"), p("/buzz/npm/bin")]; @@ -338,9 +406,8 @@ mod compose_tests { #[test] fn inherited_appended_last_when_use_inherited_true() { let managed = vec![p("/buzz/npm/bin")]; - let login = vec![]; let inherited = vec![p("C:/windows/node"), p("C:/windows/npm")]; - let result = compose_path_entries(managed, login, inherited.clone(), true); + let result = compose_path_entries(managed, vec![], inherited.clone(), true); assert_eq!(result[0], p("/buzz/npm/bin"), "managed must be first"); assert_eq!( &result[1..], @@ -349,19 +416,35 @@ mod compose_tests { ); } + /// Windows policy ON + empty inherited PATH — should produce just managed + /// entries, not None and not a phantom segment. #[test] - fn empty_managed_and_login_with_inherited_appended() { - let inherited = vec![p("C:/windows/system32"), p("C:/windows")]; - let result = compose_path_entries(vec![], vec![], inherited.clone(), true); + fn windows_policy_on_empty_inherited_produces_managed_only() { + let managed = vec![p("/buzz/npm/bin")]; + let result = compose_path_entries(managed.clone(), vec![], vec![], true); assert_eq!( - result, inherited, - "only inherited entries when others are empty" + result, managed, + "empty inherited must not add phantom entries" ); } + /// Windows policy ON + unset/absent inherited (empty vec from var_os None) — + /// same result as above; no crash, no phantom. #[test] - fn empty_all_inputs_returns_empty() { - let result = compose_path_entries(vec![], vec![], vec![], false); + fn windows_policy_on_unset_inherited_path_produces_managed_only() { + // Simulates std::env::var_os("PATH") returning None → empty vec. + let managed = vec![p("/buzz/npm/bin")]; + let inherited: Vec = vec![]; // empty, as if PATH is unset + let result = compose_path_entries(managed.clone(), vec![], inherited, true); + assert_eq!(result, managed); + } + + /// No local context + Windows policy ON — compose_path_entries itself still + /// works (no crash), and the caller is responsible for not calling it. + /// Specifically: all-empty inputs with use_inherited=true still returns empty. + #[test] + fn all_empty_with_use_inherited_true_returns_empty() { + let result = compose_path_entries(vec![], vec![], vec![], true); assert!( result.is_empty(), "all-empty inputs must produce empty output" @@ -369,24 +452,16 @@ mod compose_tests { } #[test] - fn install_runtime_parity_same_kernel() { - // Both install and runtime paths share compose_path_entries. Verify that - // two callers with identical inputs produce identical output — the drift - // that upstream PRs #2247 vs #2533 introduced cannot happen here. - let managed = vec![p("/buzz/node/bin"), p("/buzz/npm/bin")]; - let inherited = vec![p("C:/win/node")]; - let install_result = compose_path_entries(managed.clone(), vec![], inherited.clone(), true); - let runtime_result = compose_path_entries(managed.clone(), vec![], inherited.clone(), true); - assert_eq!( - install_result, runtime_result, - "install and runtime callers with identical inputs must produce identical PATH" + fn empty_all_inputs_use_inherited_false_returns_empty() { + let result = compose_path_entries(vec![], vec![], vec![], false); + assert!( + result.is_empty(), + "all-empty inputs must produce empty output" ); } - /// Non-Windows behavior: `use_inherited=false` (what the runtime passes on Unix) - /// must produce byte-identical output to what existed before this fix. - /// The inherited entries are collected but never appended — they are dead weight - /// that compose_path_entries drops. + /// Non-Windows behavior: `use_inherited=false` must produce byte-identical + /// output to before this fix. Inherited entries are collected but dropped. #[cfg(unix)] #[test] fn unix_use_inherited_false_output_unchanged() { @@ -405,4 +480,62 @@ mod compose_tests { "Unix output must not include inherited entries when use_inherited=false" ); } + + // ── Structural wrapper-alignment test ────────────────────────────────────── + // + // Verifies that both `build_augmented_path` and `install_shell_command` + // compute the same `should_use_inherited` decision for equivalent inputs. + // Tests the policy function directly to confirm the wrappers can't drift. + + /// install and runtime wrappers derive `use_inherited` from the same pure + /// policy function. Verify all four relevant input combinations agree. + #[test] + fn wrapper_policy_parity_install_and_runtime_agree() { + // (had_shell, has_context, is_windows) → expected + let cases = [ + (false, true, true, true), // Windows, no shell, context → USE + (true, true, true, false), // Windows, shell present → NO + (false, false, true, false), // Windows, no context → NO + (false, true, false, false), // non-Windows → NO + ]; + for (had_shell, has_ctx, is_win, expected) in cases { + let result = should_use_inherited(had_shell, has_ctx, is_win); + assert_eq!( + result, expected, + "policy mismatch: had_shell={had_shell} has_ctx={has_ctx} is_win={is_win}" + ); + } + } + + // ── is_batch_shim extension tests ───────────────────────────────────────── + + #[test] + fn batch_shim_cmd_lower() { + assert!(is_batch_shim(Path::new("claude.cmd"))); + } + + #[test] + fn batch_shim_cmd_upper() { + assert!(is_batch_shim(Path::new("claude.CMD"))); + } + + #[test] + fn batch_shim_bat_lower() { + assert!(is_batch_shim(Path::new("claude.bat"))); + } + + #[test] + fn batch_shim_bat_upper() { + assert!(is_batch_shim(Path::new("claude.BAT"))); + } + + #[test] + fn batch_shim_exe_not_shim() { + assert!(!is_batch_shim(Path::new("claude.exe"))); + } + + #[test] + fn batch_shim_no_extension_not_shim() { + assert!(!is_batch_shim(Path::new("claude"))); + } } From b610a64d883173b5633f348c43179394a84aee4c Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Thu, 23 Jul 2026 14:23:12 -0400 Subject: [PATCH 4/4] fix(desktop): restore non-Windows CLAUDE_CODE_EXECUTABLE behavior via should_skip_claude_executable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Will Pfleger --- .../src-tauri/src/managed_agents/runtime.rs | 5 +- .../src/managed_agents/runtime/path.rs | 71 ++++++++++++++++++- .../src/managed_agents/runtime/tests.rs | 12 ++-- 3 files changed, 77 insertions(+), 11 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 734b1476d3..6687cbbcf2 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -17,7 +17,7 @@ use crate::{ mod path; pub(in crate::managed_agents) use path::build_augmented_path; pub(crate) use path::compose_path_entries; -pub(crate) use path::is_batch_shim; +pub(crate) use path::should_skip_claude_executable; pub(crate) use path::should_use_inherited; mod stop; @@ -1610,7 +1610,8 @@ pub(crate) fn configure_runtime_cli( // adapter tries to spawn them (issue #2397). Skip setting // `CLAUDE_CODE_EXECUTABLE` for shim paths so the adapter falls back to // its own PATH lookup and finds the real binary instead. - if is_batch_shim(&cli_path) { + // Non-Windows: `.cmd`/`.bat` are valid executables and must be assigned. + if should_skip_claude_executable(&cli_path, cfg!(windows)) { return; } command.env("CLAUDE_CODE_EXECUTABLE", cli_path); diff --git a/desktop/src-tauri/src/managed_agents/runtime/path.rs b/desktop/src-tauri/src/managed_agents/runtime/path.rs index cffb081671..cf6950f577 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/path.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/path.rs @@ -16,6 +16,17 @@ pub(crate) fn is_batch_shim(path: &std::path::Path) -> bool { .unwrap_or(false) } +/// Return `true` when the resolved CLI path should be skipped for +/// `CLAUDE_CODE_EXECUTABLE` assignment. +/// +/// On Windows, `.cmd`/`.bat` batch shims cannot be passed directly to +/// `CreateProcess` (EINVAL, issue #2397). On non-Windows those extensions are +/// valid executables and must not be suppressed — the `is_windows` flag keeps +/// this decision testable cross-host on macOS CI. +pub(crate) fn should_skip_claude_executable(path: &std::path::Path, is_windows: bool) -> bool { + is_windows && is_batch_shim(path) +} + /// Decide whether the inherited process PATH should be appended to the /// composed PATH. /// @@ -487,10 +498,11 @@ mod compose_tests { // compute the same `should_use_inherited` decision for equivalent inputs. // Tests the policy function directly to confirm the wrappers can't drift. - /// install and runtime wrappers derive `use_inherited` from the same pure - /// policy function. Verify all four relevant input combinations agree. + /// Exhaustive truth-table for `should_use_inherited` — all four input + /// combinations that affect real callers. Confirms the policy is correct + /// before either wrapper binds to it. #[test] - fn wrapper_policy_parity_install_and_runtime_agree() { + fn should_use_inherited_policy_truth_table() { // (had_shell, has_context, is_windows) → expected let cases = [ (false, true, true, true), // Windows, no shell, context → USE @@ -538,4 +550,57 @@ mod compose_tests { fn batch_shim_no_extension_not_shim() { assert!(!is_batch_shim(Path::new("claude"))); } + + // ── should_skip_claude_executable policy tests ──────────────────────────── + // + // Cross-host policy: shim + Windows → skip; shim + non-Windows → assign; + // non-shim either OS → assign. Mirrors the `should_use_inherited` pattern. + + #[test] + fn skip_claude_executable_shim_windows_returns_true() { + assert!( + super::should_skip_claude_executable(Path::new("claude.cmd"), true), + "shim + windows=true must skip" + ); + assert!( + super::should_skip_claude_executable(Path::new("claude.BAT"), true), + "shim + windows=true must skip" + ); + } + + #[test] + fn skip_claude_executable_shim_non_windows_returns_false() { + assert!( + !super::should_skip_claude_executable(Path::new("claude.cmd"), false), + "shim + windows=false must NOT skip (valid executable on non-Windows)" + ); + assert!( + !super::should_skip_claude_executable(Path::new("claude.bat"), false), + "shim + windows=false must NOT skip" + ); + } + + #[test] + fn skip_claude_executable_exe_both_platforms_returns_false() { + assert!( + !super::should_skip_claude_executable(Path::new("claude.exe"), true), + "non-shim + windows=true must NOT skip" + ); + assert!( + !super::should_skip_claude_executable(Path::new("claude.exe"), false), + "non-shim + windows=false must NOT skip" + ); + } + + #[test] + fn skip_claude_executable_no_ext_both_platforms_returns_false() { + assert!( + !super::should_skip_claude_executable(Path::new("claude"), true), + "no-ext + windows=true must NOT skip" + ); + assert!( + !super::should_skip_claude_executable(Path::new("claude"), false), + "no-ext + windows=false must NOT skip" + ); + } } diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index cda14fb32f..fd758047c3 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -630,7 +630,7 @@ fn codex_spawn_does_not_set_a_claude_executable() { #[test] fn batch_shim_cmd_extension_is_rejected() { assert!( - super::is_batch_shim(std::path::Path::new("claude.cmd")), + super::path::is_batch_shim(std::path::Path::new("claude.cmd")), "claude.cmd must be identified as a batch shim" ); } @@ -638,7 +638,7 @@ fn batch_shim_cmd_extension_is_rejected() { #[test] fn batch_shim_cmd_extension_uppercase_is_rejected() { assert!( - super::is_batch_shim(std::path::Path::new("claude.CMD")), + super::path::is_batch_shim(std::path::Path::new("claude.CMD")), "claude.CMD must be identified as a batch shim (case-insensitive)" ); } @@ -646,7 +646,7 @@ fn batch_shim_cmd_extension_uppercase_is_rejected() { #[test] fn batch_shim_bat_extension_is_rejected() { assert!( - super::is_batch_shim(std::path::Path::new("claude.bat")), + super::path::is_batch_shim(std::path::Path::new("claude.bat")), "claude.bat must be identified as a batch shim" ); } @@ -654,7 +654,7 @@ fn batch_shim_bat_extension_is_rejected() { #[test] fn batch_shim_bat_extension_uppercase_is_rejected() { assert!( - super::is_batch_shim(std::path::Path::new("claude.BAT")), + super::path::is_batch_shim(std::path::Path::new("claude.BAT")), "claude.BAT must be identified as a batch shim (case-insensitive)" ); } @@ -662,7 +662,7 @@ fn batch_shim_bat_extension_uppercase_is_rejected() { #[test] fn batch_shim_exe_extension_is_not_rejected() { assert!( - !super::is_batch_shim(std::path::Path::new("claude.exe")), + !super::path::is_batch_shim(std::path::Path::new("claude.exe")), "claude.exe must not be identified as a batch shim" ); } @@ -670,7 +670,7 @@ fn batch_shim_exe_extension_is_not_rejected() { #[test] fn batch_shim_no_extension_is_not_rejected() { assert!( - !super::is_batch_shim(std::path::Path::new("claude")), + !super::path::is_batch_shim(std::path::Path::new("claude")), "claude (no extension) must not be identified as a batch shim" ); }