diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index cf2ea09cf2..78d242f378 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,24 @@ 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. + // +13: fetch_login_shell_path_inner Windows guard (POSIX PATH → None). + // resolve_git_bash made pub(crate) for Windows test access. + // +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. - ["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. + // +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. + // +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 @@ -395,7 +409,14 @@ 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. + // +53: pass 2 — three cfg(windows) install shell tests (resolve succeeds with + // Git, error hint content, install_shell_command succeeds). + // +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_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..bf4ba73152 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 `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()?; - 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 +575,62 @@ 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 `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))] + { + 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)] + { + 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 = 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 +692,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 +831,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 +1401,114 @@ 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 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. + #[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..5ba0500aa7 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,40 @@ 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: 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))] + { + vec![PathBuf::from("/bin/zsh"), PathBuf::from("/bin/bash")] + } + #[cfg(windows)] + { + super::git_bash::resolve_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() { @@ -661,9 +768,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. @@ -1129,7 +1249,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..b8d0bda4fa 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1026,3 +1026,244 @@ 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" + ); +} + +/// 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() { + 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" + ); +} + +/// 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. +#[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:?}" + ); +} + +// ── 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 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_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 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 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, registry disabled. + let result = git_bash::resolve_git_bash_no_registry("", None, None, None, None, None, None); + assert_eq!( + result, None, + "empty environment with registry disabled must not resolve a Git Bash" + ); +} + +/// 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 0a39d3fbb4..790f18cfa2 100644 --- a/desktop/src-tauri/src/managed_agents/git_bash.rs +++ b/desktop/src-tauri/src/managed_agents/git_bash.rs @@ -23,11 +23,21 @@ 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."; -pub(crate) fn discover_git_bash() -> 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). Honors `BUZZ_SHELL` (any executable) — +/// correct for the Doctor readiness gate where any shell suffices. +#[allow(dead_code)] // used only on Windows; called by discover_git_bash() +pub(crate) fn resolve_git_bash_path() -> Option { #[cfg(windows)] { let env = GitBashEnv::from_process(); - let path = resolve_git_bash( + return resolve_git_bash( &env.path, env.shell_override, env.git_bash_override, @@ -36,6 +46,43 @@ pub(crate) fn discover_git_bash() -> Option { env.program_files_x86, env.local_app_data, ); + } + + #[cfg(not(windows))] + 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)] + { + let path = resolve_git_bash_path(); return Some(GitBashPrerequisite { available: path.is_some(), path: path.map(|path| path.display().to_string()), @@ -115,7 +162,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, @@ -124,7 +171,32 @@ 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())) @@ -134,8 +206,39 @@ 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 @@ -414,4 +517,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" + ); + } } 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: "",