From cc211b3bd14b69861b5a73675be8d76ec6f52a41 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 10:54:00 -0400 Subject: [PATCH 1/5] fix(desktop): resolve Doctor install shell and command detection on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit install_shell_command() hardcoded /bin/zsh / /bin/bash, which don't exist on Windows — every Doctor install failed with os error 3. Additionally, Claude/Codex install.sh scripts hard-exit on MINGW/MSYS/CYGWIN, and resolve_command only tried .exe extensions (missing npm .cmd shims). Phase A: install_shell_command now resolves Git Bash via the same resolver chain Doctor's prereq uses (resolve_git_bash_path), adds CREATE_NO_WINDOW on Windows, and fixes the timeout-kill path to use taskkill instead of the unix-only libc::kill. Phase B: Claude and Codex get Windows-specific cli_install_commands delegating to their official install.ps1 via powershell.exe. Goose unchanged (its script is already Windows-aware). Phase C: resolve_command_uncached now tries .cmd/.bat candidates on Windows so npm-generated shims are discoverable. common_binary_paths includes %APPDATA%\npm and %LOCALAPPDATA%\Programs\OpenAI\Codex\bin. run_in_login_shell falls back to Git Bash on Windows instead of the hardcoded /bin/zsh, /bin/bash. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 19 ++- .../src-tauri/src/commands/agent_config.rs | 1 + .../src-tauri/src/commands/agent_discovery.rs | 127 +++++++++++++++-- .../config_bridge/reader_tests.rs | 2 + .../src-tauri/src/managed_agents/discovery.rs | 128 ++++++++++++++++-- .../src/managed_agents/discovery/tests.rs | 77 +++++++++++ .../src-tauri/src/managed_agents/git_bash.rs | 29 ++++ desktop/src-tauri/src/managed_agents/mod.rs | 2 +- .../src-tauri/src/managed_agents/readiness.rs | 2 + 9 files changed, 361 insertions(+), 26 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index cf2ea09cf2..01fe16098a 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -163,7 +163,8 @@ const overrides = new Map([ // Git Bash readiness is intentionally colocated with buzz-agent's other // setup-mode requirements. The Windows-only requirement and serialization // test add eight lines; split remains queued with the existing file debt. - ["src-tauri/src/managed_agents/readiness.rs", 1762], + // Windows Doctor install fix: cli_install_commands_windows field added to test stubs. + ["src-tauri/src/managed_agents/readiness.rs", 1764], // 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 @@ -261,11 +262,17 @@ const overrides = new Map([ // + updated adapter_availability_cached() signature (Option return, cold=None) // prevents false restart badge on newly restarted agents. Correctness fix; // load-bearing — required by Thufir's IMPORTANT findings. (+15 lines) - ["src-tauri/src/managed_agents/discovery.rs", 1245], + // Windows Doctor install fix: cli_install_commands_windows field, impl block + // for cli_install_commands_for_os(), command_basenames() + .cmd/.bat resolution, + // Windows well-known dirs in common_binary_paths(), login_shell_candidates(), + // path_candidates_from_env_raw(). Load-bearing Windows platform support. + ["src-tauri/src/managed_agents/discovery.rs", 1352], // rebase over codex-acp-package-swap: its version-probe tests union with the // doctor-install-reliability nvm/login-shell/semver tests — each side alone // stayed under the 1000 default; the union exceeds it. - ["src-tauri/src/managed_agents/discovery/tests.rs", 1029], + // Windows Doctor install fix: command_basenames, cli_install_commands_for_os, + // and login_shell_candidates tests. Load-bearing platform-awareness coverage. + ["src-tauri/src/managed_agents/discovery/tests.rs", 1106], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode @@ -395,7 +402,11 @@ const overrides = new Map([ // Git Bash Doctor discovery exposes a narrow async Tauri command at the // existing discovery boundary. The ten-line addition preserves the platform // neutral frontend contract; split remains queued. - ["src-tauri/src/commands/agent_discovery.rs", 1357], + // Windows Doctor install fix: resolve_install_shell() + install_shell_command() + // returns Result (Windows Git Bash resolution, CREATE_NO_WINDOW, taskkill timeout + // kill), cli_install_commands_for_os() callsite, unit tests for shell selection + // and per-OS install command accessor. Load-bearing Windows platform support. + ["src-tauri/src/commands/agent_discovery.rs", 1462], // draft-persistence predicate: submit-time `loadDraft` check + inline comment // + deps-array entry in submitMessage closes the never-persisted-boundary // defect (Thufir Pass-3 finding). Load-bearing correctness fix; queued to diff --git a/desktop/src-tauri/src/commands/agent_config.rs b/desktop/src-tauri/src/commands/agent_config.rs index f30fb26d04..30b74f6fca 100644 --- a/desktop/src-tauri/src/commands/agent_config.rs +++ b/desktop/src-tauri/src/commands/agent_config.rs @@ -582,6 +582,7 @@ mod tests { mcp_hooks: false, underlying_cli: None, cli_install_commands: &[], + cli_install_commands_windows: &[], adapter_install_commands: &[], install_instructions_url: "", cli_install_hint: "", diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 3ddd226ad0..bf7c14019e 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -152,7 +152,7 @@ fn install_acp_runtime_blocking(runtime_id: &str) -> Result std::process::Command { - let shell = if std::path::Path::new("/bin/zsh").exists() { - "/bin/zsh" - } else { - "/bin/bash" - }; +/// +/// On Windows, resolves Git Bash via the same chain Doctor's prereq uses +/// (`resolve_git_bash_path`). Returns `Err` when no shell can be found. +fn install_shell_command(command: &str) -> Result { + let shell: std::path::PathBuf = resolve_install_shell()?; - let mut cmd = std::process::Command::new(shell); + let mut cmd = std::process::Command::new(&shell); cmd.args(["-l", "-c", command]); // Strip hermit env vars so npm/node use the user's normal registry and @@ -575,11 +574,52 @@ fn install_shell_command(command: &str) -> std::process::Command { } } - cmd + // Suppress the console window on Windows. + #[cfg(windows)] + { + use std::os::windows::process::CommandExt; + const CREATE_NO_WINDOW: u32 = 0x0800_0000; + cmd.creation_flags(CREATE_NO_WINDOW); + } + + Ok(cmd) +} + +/// Resolve the shell binary for install commands. +/// +/// Unix: `/bin/zsh` if present, else `/bin/bash`. +/// Windows: Git Bash via the Doctor resolver chain (`resolve_git_bash_path`). +fn resolve_install_shell() -> Result { + #[cfg(not(windows))] + { + if std::path::Path::new("/bin/zsh").exists() { + return Ok(std::path::PathBuf::from("/bin/zsh")); + } + Ok(std::path::PathBuf::from("/bin/bash")) + } + + #[cfg(windows)] + { + crate::managed_agents::git_bash::resolve_git_bash_path() + .ok_or_else(|| crate::managed_agents::git_bash::GIT_BASH_INSTALL_HINT.to_string()) + } } fn run_install_command(step: &str, command: &str) -> InstallStepResult { - let mut cmd = install_shell_command(command); + let mut cmd = match install_shell_command(command) { + Ok(cmd) => cmd, + Err(hint) => { + return InstallStepResult { + step: step.to_string(), + command: command.to_string(), + success: false, + stdout: String::new(), + stderr: "no suitable shell found for install commands".to_string(), + exit_code: None, + hint: Some(hint), + }; + } + }; let mut child = match cmd .stdin(std::process::Stdio::null()) @@ -641,6 +681,10 @@ fn run_install_command(step: &str, command: &str) -> InstallStepResult { unsafe { libc::kill(child_pid as i32, libc::SIGTERM); } + #[cfg(windows)] + { + let _ = crate::managed_agents::taskkill_tree(child_pid); + } drop(rx); let _ = wait_thread.join(); let _ = stdout_thread.join(); @@ -776,7 +820,10 @@ enum NpmPrefix { /// `npm prefix -g` to discover where npm would install global packages. #[cfg(unix)] fn resolve_npm_prefix() -> NpmPrefix { - let mut cmd = install_shell_command("npm prefix -g"); + let mut cmd = match install_shell_command("npm prefix -g") { + Ok(cmd) => cmd, + Err(_) => return NpmPrefix::Unavailable, + }; let mut child = match cmd .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::piped()) @@ -1343,6 +1390,64 @@ mod tests { "non-codex agent (None stamp) must never trigger drift badge" ); } + + // ── Phase A: install shell selection ───────────────────────────────────── + + /// On Unix, resolve_install_shell always succeeds (returns zsh or bash). + #[cfg(unix)] + #[test] + fn test_resolve_install_shell_succeeds_on_unix() { + let result = super::resolve_install_shell(); + assert!(result.is_ok(), "Unix must always resolve a shell"); + let shell = result.unwrap(); + assert!( + shell == std::path::Path::new("/bin/zsh") || shell == std::path::Path::new("/bin/bash"), + "expected /bin/zsh or /bin/bash, got {shell:?}" + ); + } + + /// install_shell_command returns a valid Command on Unix. + #[cfg(unix)] + #[test] + fn test_install_shell_command_returns_ok_on_unix() { + let result = super::install_shell_command("echo test"); + assert!(result.is_ok(), "install_shell_command must succeed on Unix"); + } + + // ── Phase B: per-OS install commands ────────────────────────────────────── + + /// On non-Windows, cli_install_commands_for_os returns the default commands. + #[cfg(not(windows))] + #[test] + fn test_cli_install_commands_for_os_returns_default_on_unix() { + let claude = crate::managed_agents::known_acp_runtime_exact("claude").unwrap(); + assert_eq!( + claude.cli_install_commands_for_os(), + claude.cli_install_commands, + "on Unix, cli_install_commands_for_os must return the default install.sh commands" + ); + } + + /// Goose install commands are the same on all platforms (script is Windows-aware). + #[test] + fn test_goose_install_commands_same_on_all_platforms() { + let goose = crate::managed_agents::known_acp_runtime_exact("goose").unwrap(); + assert_eq!( + goose.cli_install_commands_for_os(), + goose.cli_install_commands, + "goose install commands must be identical across platforms" + ); + } + + /// buzz-agent has no install commands on any platform. + #[test] + fn test_buzz_agent_has_no_install_commands() { + let buzz = crate::managed_agents::known_acp_runtime_exact("buzz-agent").unwrap(); + assert!( + buzz.cli_install_commands_for_os().is_empty(), + "buzz-agent ships with the app — must never have install commands" + ); + } } /// Returns the Windows-only Git Bash prerequisite used by buzz-agent's shell MCP. diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs index b7829ff111..bba5469f09 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs @@ -38,6 +38,7 @@ fn test_runtime() -> &'static KnownAcpRuntime { mcp_hooks: false, underlying_cli: None, cli_install_commands: &[], + cli_install_commands_windows: &[], adapter_install_commands: &[], install_instructions_url: "", cli_install_hint: "", @@ -611,6 +612,7 @@ fn buzz_agent_runtime() -> &'static KnownAcpRuntime { mcp_hooks: false, underlying_cli: None, cli_install_commands: &[], + cli_install_commands_windows: &[], adapter_install_commands: &[], install_instructions_url: "", cli_install_hint: "", diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 2c4821a4c5..382786d569 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -22,6 +22,10 @@ pub(crate) struct KnownAcpRuntime { pub underlying_cli: Option<&'static str>, /// Shell commands to install the runtime CLI itself (run sequentially). pub cli_install_commands: &'static [&'static str], + /// Windows-specific CLI install commands (e.g. PowerShell installers). + /// When non-empty on Windows, these are used instead of `cli_install_commands`. + #[allow(dead_code)] // read only on Windows via cli_install_commands_for_os() + pub cli_install_commands_windows: &'static [&'static str], /// Shell commands to install the ACP adapter (run sequentially, after CLI). pub adapter_install_commands: &'static [&'static str], /// Link to docs/repo for manual instructions. @@ -66,6 +70,23 @@ pub(crate) struct KnownAcpRuntime { pub auth_probe_args: Option<&'static [&'static str]>, } +impl KnownAcpRuntime { + /// Return the CLI install commands for the current platform. + /// + /// On Windows, returns `cli_install_commands_windows` when non-empty, + /// falling back to the default `cli_install_commands`. On other platforms + /// always returns `cli_install_commands`. + pub fn cli_install_commands_for_os(&self) -> &[&str] { + #[cfg(windows)] + { + if !self.cli_install_commands_windows.is_empty() { + return self.cli_install_commands_windows; + } + } + self.cli_install_commands + } +} + const GOOSE_AVATAR_URL: &str = "https://goose-docs.ai/img/logo_dark.png"; const CLAUDE_CODE_AVATAR_URL: &str = "https://anthropic.gallerycdn.vsassets.io/extensions/anthropic/claude-code/2.1.77/1773707456892/Microsoft.VisualStudio.Services.Icons.Default"; const CODEX_AVATAR_URL: &str = "https://openai.gallerycdn.vsassets.io/extensions/openai/chatgpt/26.5313.41514/1773706730621/Microsoft.VisualStudio.Services.Icons.Default"; @@ -90,6 +111,22 @@ fn common_binary_paths() -> &'static [PathBuf] { home.join(".asdf/shims"), ]); } + // Windows well-known dirs for npm global shims and standalone installer targets. + #[cfg(windows)] + { + if let Some(appdata) = std::env::var_os("APPDATA") { + paths.push(PathBuf::from(appdata).join("npm")); + } + if let Some(local) = std::env::var_os("LOCALAPPDATA") { + paths.push( + PathBuf::from(local) + .join("Programs") + .join("OpenAI") + .join("Codex") + .join("bin"), + ); + } + } paths }) } @@ -105,6 +142,7 @@ const KNOWN_ACP_RUNTIMES: &[KnownAcpRuntime] = &[ mcp_hooks: false, underlying_cli: Some("goose"), cli_install_commands: &["curl -fsSL https://github.com/block-open-source/goose/releases/download/stable/download_cli.sh | CONFIGURE=false bash"], + cli_install_commands_windows: &[], // goose install script is already Windows-aware adapter_install_commands: &[], install_instructions_url: "https://block.github.io/goose/", cli_install_hint: "Install Goose via the official install script.", @@ -135,6 +173,7 @@ const KNOWN_ACP_RUNTIMES: &[KnownAcpRuntime] = &[ mcp_hooks: false, underlying_cli: Some("claude"), cli_install_commands: &["curl -fsSL https://claude.ai/install.sh | bash"], + cli_install_commands_windows: &["powershell.exe -NoProfile -ExecutionPolicy Bypass -Command \"irm https://claude.ai/install.ps1 | iex\""], adapter_install_commands: &["npm install -g @agentclientprotocol/claude-agent-acp"], install_instructions_url: "https://github.com/agentclientprotocol/claude-agent-acp", cli_install_hint: "Install the Claude Code CLI via the official install script.", @@ -165,6 +204,7 @@ const KNOWN_ACP_RUNTIMES: &[KnownAcpRuntime] = &[ mcp_hooks: false, underlying_cli: Some("codex"), cli_install_commands: &["curl -fsSL https://chatgpt.com/codex/install.sh | sh"], + cli_install_commands_windows: &["powershell.exe -NoProfile -ExecutionPolicy Bypass -Command \"irm https://chatgpt.com/codex/install.ps1 | iex\""], adapter_install_commands: &["npm install -g @agentclientprotocol/codex-acp"], install_instructions_url: "https://github.com/agentclientprotocol/codex-acp", cli_install_hint: "Install the Codex CLI via the official install script.", @@ -196,6 +236,7 @@ const KNOWN_ACP_RUNTIMES: &[KnownAcpRuntime] = &[ mcp_hooks: true, underlying_cli: None, cli_install_commands: &[], + cli_install_commands_windows: &[], adapter_install_commands: &[], install_instructions_url: "https://github.com/block/buzz", cli_install_hint: "Ships with the Buzz desktop app.", @@ -567,6 +608,26 @@ pub(crate) fn availability_drift( } } +/// Return all candidate basenames for `command` on the current platform. +/// +/// Always includes `executable_basename(command)` (appends `.exe` on Windows). +/// On Windows also includes `.cmd` and `.bat` variants so npm-generated shims +/// (e.g. `codex-acp.cmd` in `%APPDATA%\npm`) are discoverable. +fn command_basenames(command: &str) -> Vec { + let candidates = vec![executable_basename(command)]; + #[cfg(windows)] + { + let mut candidates = candidates; + if !command.contains('.') { + candidates.push(format!("{command}.cmd")); + candidates.push(format!("{command}.bat")); + } + return candidates; + } + #[allow(unreachable_code)] + candidates +} + fn resolve_command_uncached(command: &str) -> Option { if let Some(path) = resolve_workspace_command(command) { return Some(path); @@ -583,13 +644,28 @@ fn resolve_command_uncached(command: &str) -> Option { } } + // On Windows, also scan PATH for .cmd/.bat shims (npm globals). + #[cfg(windows)] + { + for basename in command_basenames(command).iter().skip(1) { + for candidate in path_candidates_from_env_raw(basename) { + if candidate.is_file() { + return Some(candidate); + } + } + } + } + if let Some(path) = find_via_login_shell(command) { return Some(path); } + let basenames = command_basenames(command); for dir in common_binary_paths() { - let candidate = dir.join(executable_basename(command)); - if is_executable_file(&candidate) { - return Some(candidate); + for basename in &basenames { + let candidate = dir.join(basename); + if is_executable_file(&candidate) { + return Some(candidate); + } } } @@ -599,9 +675,11 @@ fn resolve_command_uncached(command: &str) -> Option { // invisible. if let Some(home) = dirs::home_dir() { if let Some(nvm_bin) = find_nvm_default_bin(&home) { - let candidate = nvm_bin.join(executable_basename(command)); - if is_executable_file(&candidate) { - return Some(candidate); + for basename in &basenames { + let candidate = nvm_bin.join(basename); + if is_executable_file(&candidate) { + return Some(candidate); + } } } } @@ -619,11 +697,41 @@ fn path_candidates_from_env(command: &str) -> Vec { .unwrap_or_default() } -/// Run a command in a login shell (tries zsh then bash). +/// Like `path_candidates_from_env` but joins `basename` as-is (no `.exe` suffix). +/// Used for `.cmd`/`.bat` shim resolution on Windows. +#[cfg(windows)] +fn path_candidates_from_env_raw(basename: &str) -> Vec { + std::env::var_os("PATH") + .map(|paths| { + std::env::split_paths(&paths) + .map(|dir| dir.join(basename)) + .collect::>() + }) + .unwrap_or_default() +} + +/// Collect login shell candidates for the current platform. +/// +/// On Unix: `/bin/zsh`, `/bin/bash` (the historical defaults). +/// On Windows: the resolved Git Bash (via the same chain Doctor uses). +fn login_shell_candidates() -> Vec { + #[cfg(not(windows))] + { + vec![PathBuf::from("/bin/zsh"), PathBuf::from("/bin/bash")] + } + #[cfg(windows)] + { + super::git_bash::resolve_git_bash_path() + .into_iter() + .collect() + } +} + +/// Run a command in a login shell (tries zsh then bash on Unix, Git Bash on Windows). /// Returns trimmed stdout if the command succeeds with non-empty output. fn run_in_login_shell(args: &[&str]) -> Option { - for shell in ["/bin/zsh", "/bin/bash"] { - let Ok(output) = Command::new(shell).args(args).output() else { + for shell in login_shell_candidates() { + let Ok(output) = Command::new(&shell).args(args).output() else { continue; }; if !output.status.success() { @@ -1129,7 +1237,7 @@ pub fn discover_acp_runtimes() -> Vec { .map(|cmd| normalize_agent_args(cmd, Vec::new())) .unwrap_or_default(); - let can_auto_install = !runtime.cli_install_commands.is_empty() + let can_auto_install = !runtime.cli_install_commands_for_os().is_empty() || !runtime.adapter_install_commands.is_empty(); let cli_hint = runtime.cli_install_hint; diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index d1c1cc18d8..30959af52d 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1026,3 +1026,80 @@ fn find_nvm_default_bin_rejects_absolute_hop_tag() { let result = find_nvm_default_bin(home.path()); assert_eq!(result, None, "absolute-path hop tag must be rejected"); } + +// ── Phase C: command_basenames ────────────────────────────────────────────── + +/// On non-Windows, command_basenames returns only the executable_basename. +#[cfg(not(windows))] +#[test] +fn test_command_basenames_single_candidate_on_unix() { + let candidates = super::command_basenames("codex-acp"); + assert_eq!( + candidates, + vec!["codex-acp"], + "Unix must produce a single candidate" + ); +} + +/// command_basenames with a dotted name never adds .cmd/.bat. +#[test] +fn test_command_basenames_dotted_name_no_extra_candidates() { + let candidates = super::command_basenames("codex-acp.exe"); + // Should contain the executable_basename only, no .cmd/.bat. + assert_eq!( + candidates.len(), + 1, + "dotted name must produce exactly one candidate" + ); +} + +// ── Phase B: cli_install_commands_for_os ──────────────────────────────────── + +/// Claude and Codex have non-empty default cli_install_commands (install.sh). +#[test] +fn test_claude_and_codex_have_cli_install_commands() { + let claude = super::known_acp_runtime_exact("claude").unwrap(); + let codex = super::known_acp_runtime_exact("codex").unwrap(); + assert!( + !claude.cli_install_commands.is_empty(), + "claude must have cli install commands" + ); + assert!( + !codex.cli_install_commands.is_empty(), + "codex must have cli install commands" + ); +} + +/// cli_install_commands_for_os returns a non-empty slice for claude and codex. +#[test] +fn test_cli_install_commands_for_os_non_empty_for_claude_codex() { + let claude = super::known_acp_runtime_exact("claude").unwrap(); + let codex = super::known_acp_runtime_exact("codex").unwrap(); + assert!( + !claude.cli_install_commands_for_os().is_empty(), + "claude must have install commands on every platform" + ); + assert!( + !codex.cli_install_commands_for_os().is_empty(), + "codex must have install commands on every platform" + ); +} + +// ── Phase C: login_shell_candidates ───────────────────────────────────────── + +/// On Unix, login_shell_candidates returns at least one candidate. +#[cfg(unix)] +#[test] +fn test_login_shell_candidates_non_empty_on_unix() { + let candidates = super::login_shell_candidates(); + assert!( + !candidates.is_empty(), + "Unix must have at least one login shell candidate" + ); + // The first candidate should be /bin/zsh or /bin/bash. + let first = &candidates[0]; + assert!( + first == std::path::Path::new("/bin/zsh") || first == std::path::Path::new("/bin/bash"), + "expected /bin/zsh or /bin/bash, got {first:?}" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/git_bash.rs b/desktop/src-tauri/src/managed_agents/git_bash.rs index 0a39d3fbb4..ac1cdf1932 100644 --- a/desktop/src-tauri/src/managed_agents/git_bash.rs +++ b/desktop/src-tauri/src/managed_agents/git_bash.rs @@ -23,6 +23,35 @@ const INSTALL_URL: &str = "https://git-scm.com/download/win"; const INSTALL_HINT: &str = "Install Git for Windows and select \"Git from the command line and also from 3rd-party software\" for its PATH option."; +/// Install hint for error messages when `install_shell_command` can't find a shell on Windows. +#[cfg(windows)] +pub(crate) const GIT_BASH_INSTALL_HINT: &str = INSTALL_HINT; + +/// Resolve the Git Bash executable path using the same resolver chain as Doctor. +/// +/// Returns `Some(path)` on Windows when a usable bash is found, `None` otherwise +/// (including all non-Windows platforms). Used by `install_shell_command` to keep +/// a single contract: Doctor prereq green ⇒ install shell spawns. +#[allow(dead_code)] // used only on Windows, from install_shell_command + login_shell_candidates +pub(crate) fn resolve_git_bash_path() -> Option { + #[cfg(windows)] + { + let env = GitBashEnv::from_process(); + return resolve_git_bash( + &env.path, + env.shell_override, + env.git_bash_override, + env.system_root, + env.program_files, + env.program_files_x86, + env.local_app_data, + ); + } + + #[cfg(not(windows))] + None +} + pub(crate) fn discover_git_bash() -> Option { #[cfg(windows)] { diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index c8037f463c..14dc76267d 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -9,7 +9,7 @@ mod backend; pub(crate) mod config_bridge; mod discovery; mod env_vars; -mod git_bash; +pub(crate) mod git_bash; pub(crate) mod global_config; mod nest; mod persona_avatars; diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index 8349cff014..bf419b0cd1 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -960,6 +960,7 @@ mod tests { mcp_hooks: false, underlying_cli, cli_install_commands: &[], + cli_install_commands_windows: &[], adapter_install_commands: &[], install_instructions_url: "", cli_install_hint: "", @@ -1152,6 +1153,7 @@ mod tests { mcp_hooks: false, underlying_cli, cli_install_commands: &[], + cli_install_commands_windows: &[], adapter_install_commands: &[], install_instructions_url: "", cli_install_hint: "", From dfcb654520d433327126a6f72770766cdf970290 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 11:31:43 -0400 Subject: [PATCH 2/5] fix(desktop): guard POSIX PATH from native Windows children, add behavioral tests fetch_login_shell_path_inner() returns None on Windows so Git Bash's colon-delimited PATH never reaches agent_models, build_augmented_path, or cli_probe consumers that split on semicolons. resolve_git_bash() made pub(crate) for cross-module test access. Five cfg(windows) behavioral tests added: - command_basenames produces .exe/.cmd/.bat candidates - cli_install_commands_for_os selects PowerShell installers - login_shell_path returns None (regression lock) - .cmd shim resolves from well-known dirs - empty env yields no Git Bash + Doctor hint is non-empty Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 13 +- .../src-tauri/src/commands/agent_discovery.rs | 50 +++++++ .../src-tauri/src/managed_agents/discovery.rs | 19 ++- .../src/managed_agents/discovery/tests.rs | 132 ++++++++++++++++++ .../src-tauri/src/managed_agents/git_bash.rs | 2 +- 5 files changed, 209 insertions(+), 7 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 01fe16098a..7525ab2980 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -266,13 +266,18 @@ const overrides = new Map([ // for cli_install_commands_for_os(), command_basenames() + .cmd/.bat resolution, // Windows well-known dirs in common_binary_paths(), login_shell_candidates(), // path_candidates_from_env_raw(). Load-bearing Windows platform support. - ["src-tauri/src/managed_agents/discovery.rs", 1352], + // +13: fetch_login_shell_path_inner Windows guard (POSIX PATH → None). + // resolve_git_bash made pub(crate) for Windows test access. + ["src-tauri/src/managed_agents/discovery.rs", 1365], // rebase over codex-acp-package-swap: its version-probe tests union with the // doctor-install-reliability nvm/login-shell/semver tests — each side alone // stayed under the 1000 default; the union exceeds it. // Windows Doctor install fix: command_basenames, cli_install_commands_for_os, // and login_shell_candidates tests. Load-bearing platform-awareness coverage. - ["src-tauri/src/managed_agents/discovery/tests.rs", 1106], + // +132: pass 2 — five cfg(windows) behavioral tests: command_basenames .cmd/.bat + // candidates, cli_install_commands_for_os PowerShell selection, login_shell_path + // None regression, .cmd shim resolution, no-git-bash error hint. + ["src-tauri/src/managed_agents/discovery/tests.rs", 1238], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode @@ -406,7 +411,9 @@ const overrides = new Map([ // returns Result (Windows Git Bash resolution, CREATE_NO_WINDOW, taskkill timeout // kill), cli_install_commands_for_os() callsite, unit tests for shell selection // and per-OS install command accessor. Load-bearing Windows platform support. - ["src-tauri/src/commands/agent_discovery.rs", 1462], + // +53: pass 2 — three cfg(windows) install shell tests (resolve succeeds with + // Git, error hint content, install_shell_command succeeds). + ["src-tauri/src/commands/agent_discovery.rs", 1515], // draft-persistence predicate: submit-time `loadDraft` check + inline comment // + deps-array entry in submitMessage closes the never-persisted-boundary // defect (Thufir Pass-3 finding). Load-bearing correctness fix; queued to diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index bf7c14019e..38d0ed0ce1 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -1414,6 +1414,56 @@ mod tests { assert!(result.is_ok(), "install_shell_command must succeed on Unix"); } + // ── Phase A: Windows install shell selection ─────────────────────────────── + + /// On Windows (CI runner has Git pre-installed), resolve_install_shell succeeds. + #[cfg(windows)] + #[test] + fn test_resolve_install_shell_succeeds_on_windows_with_git() { + let result = super::resolve_install_shell(); + assert!( + result.is_ok(), + "Windows CI runner has Git — resolve_install_shell must succeed; got: {:?}", + result.err() + ); + let shell = result.unwrap(); + // The resolved path must end with bash.exe (Git Bash). + let fname = shell.file_name().and_then(|n| n.to_str()).unwrap_or(""); + assert!( + fname.eq_ignore_ascii_case("bash.exe"), + "Windows install shell must be bash.exe, got: {shell:?}" + ); + } + + /// On Windows, when no Git Bash is found, the error carries the Doctor hint. + #[cfg(windows)] + #[test] + fn test_resolve_install_shell_error_contains_doctor_hint() { + // We can't force resolve_install_shell to fail on CI (Git is installed), + // but we can verify the error string it would use matches the hint. + let hint = crate::managed_agents::git_bash::GIT_BASH_INSTALL_HINT; + assert!( + hint.contains("Git for Windows"), + "GIT_BASH_INSTALL_HINT must mention Git for Windows; got: {hint}" + ); + assert!( + hint.contains("PATH"), + "GIT_BASH_INSTALL_HINT must mention PATH option; got: {hint}" + ); + } + + /// install_shell_command returns a valid Command on Windows. + #[cfg(windows)] + #[test] + fn test_install_shell_command_returns_ok_on_windows() { + let result = super::install_shell_command("echo test"); + assert!( + result.is_ok(), + "install_shell_command must succeed on Windows with Git; got: {:?}", + result.err() + ); + } + // ── 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/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 382786d569..90d2cecc95 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -769,9 +769,22 @@ fn path_cache() -> &'static std::sync::Mutex { } fn fetch_login_shell_path_inner() -> Option { - let stdout = run_in_login_shell(&["-l", "-c", "echo $PATH"])?; - let last_line = stdout.lines().rfind(|l| !l.trim().is_empty())?; - Some(last_line.trim().to_string()) + // On Windows, Git Bash's `echo $PATH` returns POSIX colon-delimited paths + // (`/mingw64/bin:/c/Users/...`) which poison native Windows children that + // split on `;`. login_shell_path() feeds agent_models, runtime, and + // cli_probe — all native processes. Return None so they inherit the real + // Windows PATH instead. + #[cfg(windows)] + { + return None; + } + + #[cfg(not(windows))] + { + let stdout = run_in_login_shell(&["-l", "-c", "echo $PATH"])?; + let last_line = stdout.lines().rfind(|l| !l.trim().is_empty())?; + Some(last_line.trim().to_string()) + } } /// Return the user's full PATH from a login shell. diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 30959af52d..72e5dd54d2 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1041,6 +1041,22 @@ fn test_command_basenames_single_candidate_on_unix() { ); } +/// On Windows, command_basenames adds .exe, .cmd, and .bat candidates. +#[cfg(windows)] +#[test] +fn test_command_basenames_includes_cmd_bat_on_windows() { + let candidates = super::command_basenames("codex-acp"); + assert_eq!( + candidates, + vec![ + "codex-acp.exe".to_string(), + "codex-acp.cmd".to_string(), + "codex-acp.bat".to_string(), + ], + "Windows must produce .exe, .cmd, and .bat candidates" + ); +} + /// command_basenames with a dotted name never adds .cmd/.bat. #[test] fn test_command_basenames_dotted_name_no_extra_candidates() { @@ -1085,6 +1101,45 @@ fn test_cli_install_commands_for_os_non_empty_for_claude_codex() { ); } +/// On Windows, Claude and Codex select the PowerShell install commands. +#[cfg(windows)] +#[test] +fn test_cli_install_commands_for_os_selects_powershell_on_windows() { + let claude = super::known_acp_runtime_exact("claude").unwrap(); + let codex = super::known_acp_runtime_exact("codex").unwrap(); + + // Windows must select the PowerShell commands, not the curl|bash ones. + let claude_cmds = claude.cli_install_commands_for_os(); + let codex_cmds = codex.cli_install_commands_for_os(); + + assert_ne!( + claude_cmds, claude.cli_install_commands, + "Windows must NOT use the default curl|bash commands for claude" + ); + assert_ne!( + codex_cmds, codex.cli_install_commands, + "Windows must NOT use the default curl|bash commands for codex" + ); + + // Verify they are the PowerShell installers. + assert!( + claude_cmds.iter().any(|c| c.contains("powershell")), + "claude Windows install must use powershell; got: {claude_cmds:?}" + ); + assert!( + codex_cmds.iter().any(|c| c.contains("powershell")), + "codex Windows install must use powershell; got: {codex_cmds:?}" + ); + + // Goose and buzz-agent must NOT use Windows-specific commands. + let goose = super::known_acp_runtime_exact("goose").unwrap(); + assert_eq!( + goose.cli_install_commands_for_os(), + goose.cli_install_commands, + "goose must use the same commands on all platforms" + ); +} + // ── Phase C: login_shell_candidates ───────────────────────────────────────── /// On Unix, login_shell_candidates returns at least one candidate. @@ -1103,3 +1158,80 @@ fn test_login_shell_candidates_non_empty_on_unix() { "expected /bin/zsh or /bin/bash, got {first:?}" ); } + +// ── Regression: POSIX PATH must never reach native Windows consumers ─────── + +/// `login_shell_path()` must return `None` on Windows so native-process +/// consumers (`agent_models`, `build_augmented_path`, `cli_probe`) inherit +/// the real Windows PATH instead of a POSIX colon-delimited string from +/// Git Bash's `echo $PATH`. +#[cfg(windows)] +#[test] +fn test_login_shell_path_returns_none_on_windows() { + let _guard = crate::managed_agents::lock_path_mutex(); + + // Force a fresh fetch — don't rely on whatever prior tests cached. + refresh_login_shell_path(); + let path = super::login_shell_path(); + + assert_eq!( + path, None, + "login_shell_path() must return None on Windows to prevent POSIX PATH leaking into native children" + ); +} + +// ── Phase C: .cmd shim resolution on Windows ─────────────────────────────── + +/// `resolve_command_uncached` finds `.cmd` shims in `common_binary_paths()`. +/// This test creates a fake `.cmd` shim in a temp dir added to +/// `common_binary_paths()` via the `APPDATA` env var's `npm` subdirectory. +#[cfg(windows)] +#[test] +fn test_cmd_shim_resolves_from_well_known_dir() { + // Create a fake npm shim: %APPDATA%\npm\codex-acp.cmd + let temp = tempfile::tempdir().expect("tempdir"); + let npm_dir = temp.path().join("npm"); + std::fs::create_dir_all(&npm_dir).expect("mkdir npm"); + let shim = npm_dir.join("codex-acp.cmd"); + std::fs::write(&shim, "@echo off\r\nnode codex-acp.js %*\r\n").expect("write shim"); + + // common_binary_paths is a OnceLock — we can't re-init it. Instead, + // test the component that matters: command_basenames generates .cmd + // candidates, and the candidate path exists and is a file. + let basenames = super::command_basenames("codex-acp"); + assert!( + basenames.contains(&"codex-acp.cmd".to_string()), + "command_basenames must include .cmd candidate on Windows" + ); + let candidate = npm_dir.join(&basenames[1]); // .cmd is second + assert!( + candidate.is_file(), + "the .cmd shim must be resolvable as a file: {candidate:?}" + ); +} + +// ── Phase A: no-shell-resolved error on Windows ──────────────────────────── + +/// When no Git Bash is found, `resolve_git_bash()` returns `None`. +/// The install shell path uses this to return the Doctor hint. +/// +/// Uses the parameterized `resolve_git_bash()` with empty inputs to avoid +/// the CI runner's pre-installed Git influencing the result. +#[cfg(windows)] +#[test] +fn test_no_git_bash_resolved_returns_none() { + use crate::managed_agents::git_bash; + + // Empty PATH, no overrides, no well-known dirs, no registry. + let result = git_bash::resolve_git_bash("", None, None, None, None, None, None); + assert_eq!( + result, None, + "empty environment must not resolve a Git Bash" + ); + + // Verify the error message that install_shell_command would return. + assert!( + !git_bash::GIT_BASH_INSTALL_HINT.is_empty(), + "GIT_BASH_INSTALL_HINT must be non-empty for the Doctor error message" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/git_bash.rs b/desktop/src-tauri/src/managed_agents/git_bash.rs index ac1cdf1932..0c2a625f1a 100644 --- a/desktop/src-tauri/src/managed_agents/git_bash.rs +++ b/desktop/src-tauri/src/managed_agents/git_bash.rs @@ -144,7 +144,7 @@ impl GitBashEnv { } #[cfg(windows)] -fn resolve_git_bash( +pub(crate) fn resolve_git_bash( path_env: &str, shell_override: Option, git_bash_override: Option, From 161b1729bf6a78ddca15c20eeb88fbf91faf7ceb Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 12:03:35 -0400 Subject: [PATCH 3/5] fix(desktop): skip non-bash BUZZ_SHELL for install and login-shell resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUZZ_SHELL intentionally accepts any executable (cmd, pwsh) for the MCP child's readiness gate, but install_shell_command and login_shell_candidates invoke the result with bash-only -l -c syntax. A BUZZ_SHELL=cmd/pwsh config broke every Doctor install and command discovery path. Add resolve_bash_path() that passes None for shell_override, skipping BUZZ_SHELL and falling through to GIT_BASH → PATH scan → derive-from-git → well-known → registry. The existing resolve_git_bash_path() keeps its any-shell semantics for Doctor/readiness. Two regression tests prove both contracts hold simultaneously (readiness accepts pwsh/cmd; install skips them and finds bash). --- desktop/scripts/check-file-sizes.mjs | 3 +- .../src-tauri/src/commands/agent_discovery.rs | 11 ++- .../src-tauri/src/managed_agents/discovery.rs | 7 +- .../src-tauri/src/managed_agents/git_bash.rs | 92 ++++++++++++++++++- 4 files changed, 101 insertions(+), 12 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 7525ab2980..84a6ed6e98 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -268,7 +268,8 @@ const overrides = new Map([ // path_candidates_from_env_raw(). Load-bearing Windows platform support. // +13: fetch_login_shell_path_inner Windows guard (POSIX PATH → None). // resolve_git_bash made pub(crate) for Windows test access. - ["src-tauri/src/managed_agents/discovery.rs", 1365], + // +1: login_shell_candidates doc comment expanded for resolve_bash_path. + ["src-tauri/src/managed_agents/discovery.rs", 1366], // rebase over codex-acp-package-swap: its version-probe tests union with the // doctor-install-reliability nvm/login-shell/semver tests — each side alone // stayed under the 1000 default; the union exceeds it. diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 38d0ed0ce1..b5c7c8ff9b 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -542,8 +542,9 @@ fn persist_last_error_on_install( /// and `resolve_npm_prefix` — keeping them in sync so the hermit-strip list /// can't drift between the two paths. /// -/// On Windows, resolves Git Bash via the same chain Doctor's prereq uses -/// (`resolve_git_bash_path`). Returns `Err` when no shell can be found. +/// On Windows, resolves Git Bash via `resolve_bash_path` (skips `BUZZ_SHELL` +/// since install commands require bash syntax). Returns `Err` when no shell +/// can be found. fn install_shell_command(command: &str) -> Result { let shell: std::path::PathBuf = resolve_install_shell()?; @@ -588,7 +589,9 @@ fn install_shell_command(command: &str) -> Result /// Resolve the shell binary for install commands. /// /// Unix: `/bin/zsh` if present, else `/bin/bash`. -/// Windows: Git Bash via the Doctor resolver chain (`resolve_git_bash_path`). +/// Windows: Git Bash via `resolve_bash_path` — skips `BUZZ_SHELL` because install +/// commands use bash-only `-l -c` syntax. A `BUZZ_SHELL=pwsh` user gets a green +/// Doctor prereq (their agents work) but installs use the Git Bash fallback chain. fn resolve_install_shell() -> Result { #[cfg(not(windows))] { @@ -600,7 +603,7 @@ fn resolve_install_shell() -> Result { #[cfg(windows)] { - crate::managed_agents::git_bash::resolve_git_bash_path() + crate::managed_agents::git_bash::resolve_bash_path() .ok_or_else(|| crate::managed_agents::git_bash::GIT_BASH_INSTALL_HINT.to_string()) } } diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 90d2cecc95..5ba0500aa7 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -713,7 +713,8 @@ fn path_candidates_from_env_raw(basename: &str) -> Vec { /// Collect login shell candidates for the current platform. /// /// On Unix: `/bin/zsh`, `/bin/bash` (the historical defaults). -/// On Windows: the resolved Git Bash (via the same chain Doctor uses). +/// On Windows: Git Bash via `resolve_bash_path` — skips `BUZZ_SHELL` because +/// login-shell callers use bash-only `-l -c` syntax. fn login_shell_candidates() -> Vec { #[cfg(not(windows))] { @@ -721,9 +722,7 @@ fn login_shell_candidates() -> Vec { } #[cfg(windows)] { - super::git_bash::resolve_git_bash_path() - .into_iter() - .collect() + super::git_bash::resolve_bash_path().into_iter().collect() } } diff --git a/desktop/src-tauri/src/managed_agents/git_bash.rs b/desktop/src-tauri/src/managed_agents/git_bash.rs index 0c2a625f1a..aaebd02bbd 100644 --- a/desktop/src-tauri/src/managed_agents/git_bash.rs +++ b/desktop/src-tauri/src/managed_agents/git_bash.rs @@ -30,9 +30,9 @@ pub(crate) const GIT_BASH_INSTALL_HINT: &str = INSTALL_HINT; /// Resolve the Git Bash executable path using the same resolver chain as Doctor. /// /// Returns `Some(path)` on Windows when a usable bash is found, `None` otherwise -/// (including all non-Windows platforms). Used by `install_shell_command` to keep -/// a single contract: Doctor prereq green ⇒ install shell spawns. -#[allow(dead_code)] // used only on Windows, from install_shell_command + login_shell_candidates +/// (including all non-Windows platforms). Honors `BUZZ_SHELL` (any executable) — +/// correct for the Doctor readiness gate where any shell suffices. +#[allow(dead_code)] // used only on Windows, from discover_git_bash + git_bash_available pub(crate) fn resolve_git_bash_path() -> Option { #[cfg(windows)] { @@ -52,6 +52,33 @@ pub(crate) fn resolve_git_bash_path() -> Option { None } +/// Resolve a bash-compatible shell for install commands and login-shell discovery. +/// +/// Unlike `resolve_git_bash_path`, this skips `BUZZ_SHELL` entirely — that override +/// intentionally accepts any executable (`cmd`, `pwsh`) for the MCP child, but install +/// commands and `login_shell_candidates` use bash-only `-l -c` syntax. Skipping the +/// override means the chain falls through to: `GIT_BASH` → PATH scan → derive-from-git +/// → well-known locations → registry. +#[allow(dead_code)] // used only on Windows, from install_shell_command + login_shell_candidates +pub(crate) fn resolve_bash_path() -> Option { + #[cfg(windows)] + { + let env = GitBashEnv::from_process(); + return resolve_git_bash( + &env.path, + None, // skip BUZZ_SHELL — install/login-shell callers require bash + env.git_bash_override, + env.system_root, + env.program_files, + env.program_files_x86, + env.local_app_data, + ); + } + + #[cfg(not(windows))] + None +} + pub(crate) fn discover_git_bash() -> Option { #[cfg(windows)] { @@ -443,4 +470,63 @@ mod tests { Some(shell) ); } + + // ── Regression: install/login-shell must skip non-bash BUZZ_SHELL ───────── + + /// When BUZZ_SHELL=pwsh.exe, `resolve_git_bash` with `shell_override=None` + /// (the `resolve_bash_path` code path) skips it and falls through to the + /// bash.exe on PATH. The readiness gate (`shell_override=Some`) still + /// returns pwsh — both contracts hold simultaneously. + #[test] + fn test_install_path_skips_buzz_shell_pwsh() { + let temp = tempdir().expect("tempdir"); + let pwsh = temp.path().join("pwsh.exe"); + let bash = temp.path().join("bash.exe"); + std::fs::write(&pwsh, []).expect("pwsh"); + std::fs::write(&bash, []).expect("bash"); + + let path = std::env::join_paths([temp.path()]).expect("PATH"); + let path_str = path.to_str().expect("utf8"); + + // Readiness gate: BUZZ_SHELL=pwsh accepted (Doctor green). + assert_eq!( + resolve_git_bash(path_str, Some(pwsh.clone()), None, None, None, None, None), + Some(pwsh), + "readiness gate must accept BUZZ_SHELL=pwsh" + ); + + // Install path: shell_override=None skips pwsh, finds bash on PATH. + assert_eq!( + resolve_git_bash(path_str, None, None, None, None, None, None), + Some(bash), + "install path must skip BUZZ_SHELL and find bash on PATH" + ); + } + + /// Same as above but with BUZZ_SHELL=cmd.exe. + #[test] + fn test_install_path_skips_buzz_shell_cmd() { + let temp = tempdir().expect("tempdir"); + let cmd = temp.path().join("cmd.exe"); + let bash = temp.path().join("bash.exe"); + std::fs::write(&cmd, []).expect("cmd"); + std::fs::write(&bash, []).expect("bash"); + + let path = std::env::join_paths([temp.path()]).expect("PATH"); + let path_str = path.to_str().expect("utf8"); + + // Readiness gate: BUZZ_SHELL=cmd accepted. + assert_eq!( + resolve_git_bash(path_str, Some(cmd.clone()), None, None, None, None, None), + Some(cmd), + "readiness gate must accept BUZZ_SHELL=cmd" + ); + + // Install path: shell_override=None skips cmd, finds bash on PATH. + assert_eq!( + resolve_git_bash(path_str, None, None, None, None, None, None), + Some(bash), + "install path must skip BUZZ_SHELL and find bash on PATH" + ); + } } From edf4ff834c1b83e869fd3b9e65e62f7e191150c3 Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 12:24:07 -0400 Subject: [PATCH 4/5] refactor(desktop): wire discover_git_bash through resolve_git_bash_path discover_git_bash() inlined the same GitBashEnv::from_process() + resolve_git_bash() call that resolve_git_bash_path() wraps. Replace with a call to resolve_git_bash_path() so the "two named resolvers, two contracts" story is true in code: resolve_git_bash_path() has a real caller (discover_git_bash), resolve_bash_path() has its two (resolve_install_shell, login_shell_candidates). Fix the #[allow(dead_code)] comment to name the actual caller. --- desktop/src-tauri/src/managed_agents/git_bash.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/git_bash.rs b/desktop/src-tauri/src/managed_agents/git_bash.rs index aaebd02bbd..d7b568b254 100644 --- a/desktop/src-tauri/src/managed_agents/git_bash.rs +++ b/desktop/src-tauri/src/managed_agents/git_bash.rs @@ -32,7 +32,7 @@ pub(crate) const GIT_BASH_INSTALL_HINT: &str = INSTALL_HINT; /// Returns `Some(path)` on Windows when a usable bash is found, `None` otherwise /// (including all non-Windows platforms). Honors `BUZZ_SHELL` (any executable) — /// correct for the Doctor readiness gate where any shell suffices. -#[allow(dead_code)] // used only on Windows, from discover_git_bash + git_bash_available +#[allow(dead_code)] // used only on Windows; called by discover_git_bash() pub(crate) fn resolve_git_bash_path() -> Option { #[cfg(windows)] { @@ -82,16 +82,7 @@ pub(crate) fn resolve_bash_path() -> Option { pub(crate) fn discover_git_bash() -> Option { #[cfg(windows)] { - let env = GitBashEnv::from_process(); - let path = resolve_git_bash( - &env.path, - env.shell_override, - env.git_bash_override, - env.system_root, - env.program_files, - env.program_files_x86, - env.local_app_data, - ); + let path = resolve_git_bash_path(); return Some(GitBashPrerequisite { available: path.is_some(), path: path.map(|path| path.display().to_string()), From 2759a9a16194b1eafb0ba5a944a1fa1672fd500b Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Tue, 14 Jul 2026 12:43:20 -0400 Subject: [PATCH 5/5] fix(desktop): deterministic Windows tests for registry and install-shell error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_git_bash_no_registry skips the ambient HKLM/HKCU registry lookup so test_no_git_bash_resolved_returns_none passes deterministically on CI runners with Git for Windows installed. test_cmd_shim_resolves_from_path now calls the real resolve_command_uncached via PATH mutation under lock_path_mutex. install_shell_from seam enables pure testing of the Option→Result error mapping against GIT_BASH_INSTALL_HINT. --- desktop/scripts/check-file-sizes.mjs | 6 +- .../src-tauri/src/commands/agent_discovery.rs | 12 ++- .../src/managed_agents/discovery/tests.rs | 100 ++++++++++++------ .../src-tauri/src/managed_agents/git_bash.rs | 62 ++++++++++- 4 files changed, 139 insertions(+), 41 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 84a6ed6e98..78d242f378 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -278,7 +278,8 @@ const overrides = new Map([ // +132: pass 2 — five cfg(windows) behavioral tests: command_basenames .cmd/.bat // candidates, cli_install_commands_for_os PowerShell selection, login_shell_path // None regression, .cmd shim resolution, no-git-bash error hint. - ["src-tauri/src/managed_agents/discovery/tests.rs", 1238], + // +32: deterministic .cmd resolver + no-registry + install_shell_from tests. + ["src-tauri/src/managed_agents/discovery/tests.rs", 1270], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode @@ -414,7 +415,8 @@ const overrides = new Map([ // and per-OS install command accessor. Load-bearing Windows platform support. // +53: pass 2 — three cfg(windows) install shell tests (resolve succeeds with // Git, error hint content, install_shell_command succeeds). - ["src-tauri/src/commands/agent_discovery.rs", 1515], + // +8: install_shell_from pure seam extracted for deterministic testing. + ["src-tauri/src/commands/agent_discovery.rs", 1523], // draft-persistence predicate: submit-time `loadDraft` check + inline comment // + deps-array entry in submitMessage closes the never-persisted-boundary // defect (Thufir Pass-3 finding). Load-bearing correctness fix; queued to diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index b5c7c8ff9b..bf4ba73152 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -603,11 +603,19 @@ fn resolve_install_shell() -> Result { #[cfg(windows)] { - crate::managed_agents::git_bash::resolve_bash_path() - .ok_or_else(|| crate::managed_agents::git_bash::GIT_BASH_INSTALL_HINT.to_string()) + install_shell_from(crate::managed_agents::git_bash::resolve_bash_path()) } } +/// Pure mapping from a resolved bash path to the install-shell result. +/// `None` → `Err(GIT_BASH_INSTALL_HINT)`, `Some(path)` → `Ok(path)`. +#[cfg(windows)] +pub(crate) fn install_shell_from( + resolved: Option, +) -> Result { + resolved.ok_or_else(|| crate::managed_agents::git_bash::GIT_BASH_INSTALL_HINT.to_string()) +} + fn run_install_command(step: &str, command: &str) -> InstallStepResult { let mut cmd = match install_shell_command(command) { Ok(cmd) => cmd, diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 72e5dd54d2..b8d0bda4fa 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1182,56 +1182,88 @@ fn test_login_shell_path_returns_none_on_windows() { // ── Phase C: .cmd shim resolution on Windows ─────────────────────────────── -/// `resolve_command_uncached` finds `.cmd` shims in `common_binary_paths()`. -/// This test creates a fake `.cmd` shim in a temp dir added to -/// `common_binary_paths()` via the `APPDATA` env var's `npm` subdirectory. +/// `resolve_command_uncached` finds `.cmd` shims via the Windows PATH scan +/// (discovery.rs lines 648-657). Creates a temp dir with ONLY a `.cmd` shim +/// (no `.exe`), mutates PATH under the serialized lock, and calls the actual +/// resolver — proving the full resolution chain finds `.cmd` extensions. #[cfg(windows)] #[test] -fn test_cmd_shim_resolves_from_well_known_dir() { - // Create a fake npm shim: %APPDATA%\npm\codex-acp.cmd +fn test_cmd_shim_resolves_from_path() { + let _guard = crate::managed_agents::lock_path_mutex(); + + // Create a temp dir with only a .cmd shim — no .exe. let temp = tempfile::tempdir().expect("tempdir"); - let npm_dir = temp.path().join("npm"); - std::fs::create_dir_all(&npm_dir).expect("mkdir npm"); - let shim = npm_dir.join("codex-acp.cmd"); - std::fs::write(&shim, "@echo off\r\nnode codex-acp.js %*\r\n").expect("write shim"); - - // common_binary_paths is a OnceLock — we can't re-init it. Instead, - // test the component that matters: command_basenames generates .cmd - // candidates, and the candidate path exists and is a file. - let basenames = super::command_basenames("codex-acp"); - assert!( - basenames.contains(&"codex-acp.cmd".to_string()), - "command_basenames must include .cmd candidate on Windows" - ); - let candidate = npm_dir.join(&basenames[1]); // .cmd is second - assert!( - candidate.is_file(), - "the .cmd shim must be resolvable as a file: {candidate:?}" + let shim = temp.path().join("test-shim-cmd-resolve.cmd"); + std::fs::write(&shim, "@echo off\r\n").expect("write shim"); + + // Prepend the temp dir to PATH so the resolver sees it. + // path_candidates_from_env_raw reads PATH live (std::env::var_os each call). + let old_path = std::env::var_os("PATH").unwrap_or_default(); + let mut new_path = std::env::split_paths(&old_path).collect::>(); + new_path.insert(0, temp.path().to_path_buf()); + let joined = std::env::join_paths(&new_path).expect("join PATH"); + std::env::set_var("PATH", &joined); + + let result = super::resolve_command_uncached("test-shim-cmd-resolve"); + + // Restore PATH before any assertion can panic. + std::env::set_var("PATH", &old_path); + + assert_eq!( + result.as_deref(), + Some(shim.as_path()), + "resolve_command_uncached must find a .cmd shim on PATH when no .exe exists" ); } // ── Phase A: no-shell-resolved error on Windows ──────────────────────────── -/// When no Git Bash is found, `resolve_git_bash()` returns `None`. -/// The install shell path uses this to return the Doctor hint. -/// -/// Uses the parameterized `resolve_git_bash()` with empty inputs to avoid -/// the CI runner's pre-installed Git influencing the result. +/// When all resolution sources are empty AND the registry is disabled, +/// `resolve_git_bash_no_registry` returns `None` deterministically — +/// regardless of the CI runner's installed software. #[cfg(windows)] #[test] fn test_no_git_bash_resolved_returns_none() { use crate::managed_agents::git_bash; - // Empty PATH, no overrides, no well-known dirs, no registry. - let result = git_bash::resolve_git_bash("", None, None, None, None, None, None); + // Empty PATH, no overrides, no well-known dirs, registry disabled. + let result = git_bash::resolve_git_bash_no_registry("", None, None, None, None, None, None); assert_eq!( result, None, - "empty environment must not resolve a Git Bash" + "empty environment with registry disabled must not resolve a Git Bash" ); +} - // Verify the error message that install_shell_command would return. - assert!( - !git_bash::GIT_BASH_INSTALL_HINT.is_empty(), - "GIT_BASH_INSTALL_HINT must be non-empty for the Doctor error message" +/// When `resolve_bash_path` returns `None` (no Git Bash anywhere), +/// `install_shell_from` maps it to `Err(GIT_BASH_INSTALL_HINT)` — the exact +/// Doctor hint shown to the user. Tests the pure error-mapping seam, not the +/// resolution chain. +#[cfg(windows)] +#[test] +fn test_install_shell_from_none_returns_hint() { + use crate::commands::install_shell_from; + use crate::managed_agents::git_bash; + + let result = install_shell_from(None); + assert_eq!( + result, + Err(git_bash::GIT_BASH_INSTALL_HINT.to_string()), + "install_shell_from(None) must return the Git Bash install hint" + ); +} + +/// When `resolve_bash_path` returns `Some(path)`, `install_shell_from` +/// passes it through as `Ok`. +#[cfg(windows)] +#[test] +fn test_install_shell_from_some_returns_path() { + use crate::commands::install_shell_from; + + let path = std::path::PathBuf::from(r"C:\Git\bin\bash.exe"); + let result = install_shell_from(Some(path.clone())); + assert_eq!( + result, + Ok(path), + "install_shell_from(Some) must return the path as Ok" ); } diff --git a/desktop/src-tauri/src/managed_agents/git_bash.rs b/desktop/src-tauri/src/managed_agents/git_bash.rs index d7b568b254..790f18cfa2 100644 --- a/desktop/src-tauri/src/managed_agents/git_bash.rs +++ b/desktop/src-tauri/src/managed_agents/git_bash.rs @@ -171,7 +171,32 @@ pub(crate) fn resolve_git_bash( program_files_x86: Option, local_app_data: Option, ) -> Option { - shell_override + resolve_git_bash_inner( + path_env, + shell_override, + git_bash_override, + system_root, + program_files, + program_files_x86, + local_app_data, + true, + ) +} + +/// Inner resolver with an explicit `check_registry` toggle so tests can +/// disable the ambient `HKLM/HKCU\SOFTWARE\GitForWindows` lookup. +#[cfg(windows)] +fn resolve_git_bash_inner( + path_env: &str, + shell_override: Option, + git_bash_override: Option, + system_root: Option, + program_files: Option, + program_files_x86: Option, + local_app_data: Option, + check_registry: bool, +) -> Option { + let result = shell_override .and_then(|path| resolve_shell_override(&path, path_env)) .or_else(|| git_bash_override.filter(|path| path.is_file())) .or_else(|| scan_path_for_bash(path_env, system_root.as_deref())) @@ -181,8 +206,39 @@ pub(crate) fn resolve_git_bash( }) .or_else(|| { git_bash_from_standard_paths([program_files, program_files_x86, local_app_data]) - }) - .or_else(git_bash_from_registry) + }); + if result.is_some() { + return result; + } + if check_registry { + return git_bash_from_registry(); + } + None +} + +/// Like `resolve_git_bash` but skips the ambient Windows registry lookup, so +/// tests can assert "no resolution" deterministically regardless of the CI +/// runner's installed software. +#[cfg(all(windows, test))] +pub(crate) fn resolve_git_bash_no_registry( + path_env: &str, + shell_override: Option, + git_bash_override: Option, + system_root: Option, + program_files: Option, + program_files_x86: Option, + local_app_data: Option, +) -> Option { + resolve_git_bash_inner( + path_env, + shell_override, + git_bash_override, + system_root, + program_files, + program_files_x86, + local_app_data, + false, + ) } /// Resolve `BUZZ_SHELL` with the same rooted/bare-name semantics as the MCP