fix(developer): run shell tool under bash/sh regardless of login shell (#8659)

Signed-off-by: jh-block <jhugo@block.xyz>

crates/goose/src/agents/platform_extensions/developer/mod.rs

@@ -15,7 +15,7 @@ use rmcp::model::{
 };
 use schemars::{schema_for, JsonSchema};
 use serde_json::Value;
-use shell::{ShellOutput, ShellParams, ShellTool};
+use shell::{shell_display_name, ShellOutput, ShellParams, ShellTool};
 use std::sync::Arc;
 use tokio_util::sync::CancellationToken;
 use tree::{TreeParams, TreeTool};
@@ -120,7 +120,14 @@ impl DeveloperClient {
             )),
             Tool::new(
                 "shell".to_string(),
-                "Execute a shell command in the user's default shell in the current dir. Returns an object with stdout and stderr as separate fields. The output of each stream is limited to up to 2000 lines, and longer outputs will be saved to a temporary file.".to_string(),
+                format!(
+                    "Execute a shell command in the current dir. Commands run under `{shell}` \
+                     (set GOOSE_SHELL to override) - write command strings in that shell's \
+                     syntax. Returns an object with stdout and stderr as separate fields. The \
+                     output of each stream is limited to up to 2000 lines, and longer outputs \
+                     will be saved to a temporary file.",
+                    shell = shell_display_name(),
+                ),
                 Self::schema::<ShellParams>(),
             )
             .with_output_schema::<ShellOutput>()

crates/goose/src/agents/platform_extensions/developer/shell.rs

@@ -47,50 +47,67 @@ fn flatpak_spawn_process() -> std::process::Command {
     command
 }
 
-/// Resolve the preferred Unix shell, respecting GOOSE_SHELL.
+/// Resolve the preferred Unix shell for command execution, respecting GOOSE_SHELL.
 ///
-/// Returns `(shell_path, is_user_configured)` — the boolean is true when
-/// `GOOSE_SHELL` was explicitly set, which matters in Flatpak mode: an
-/// explicit path is passed through as-is (the user likely intends a host
-/// path), whereas auto-detected defaults are reduced to their basename so
-/// the host's PATH resolves the correct binary.
+/// Auto-detected shells are returned as basenames (e.g. `"bash"`) so that
+/// `Command::new` resolves them on `PATH` at spawn time — this also keeps
+/// Flatpak happy, where absolute paths from inside the sandbox don't match
+/// the host filesystem. `GOOSE_SHELL` is passed through as-is.
 ///
-/// In Flatpak mode without an explicit GOOSE_SHELL, we skip sandbox
-/// filesystem checks (e.g. `/bin/bash` existing inside the sandbox tells
-/// us nothing about the host) and default to `"bash"` directly.
-#[cfg(not(windows))]
-fn unix_shell() -> (String, bool) {
-    match std::env::var("GOOSE_SHELL") {
-        Ok(shell) => (shell, true),
-        Err(_) => {
-            let shell = if is_flatpak() {
-                // Don't inspect sandbox paths — they don't reflect the host.
-                // Default to "bash" (ubiquitous on Flatpak-capable Linux hosts).
-                "bash".to_string()
-            } else if PathBuf::from("/bin/bash").is_file() {
-                "/bin/bash".to_string()
-            } else {
-                std::env::var("SHELL").unwrap_or_else(|_| "sh".to_string())
-            };
-            (shell, false)
-        }
-    }
+#[cfg(windows)]
+fn windows_shell() -> String {
+    std::env::var("GOOSE_SHELL").unwrap_or_else(|_| "cmd".to_string())
+}
+
+/// Short, human-readable name of a shell path (the file stem), used both to
+/// pick the right argument style on Windows and to tell the LLM which
+/// dialect to write in the tool description.
+#[cfg(windows)]
+fn shell_basename(shell: &str) -> String {
+    Path::new(shell)
+        .file_stem()
+        .and_then(|s| s.to_str())
+        .unwrap_or("cmd")
+        .to_lowercase()
 }
 
-/// Return the shell reference to pass to `flatpak-spawn --host`.
-///
-/// If the user explicitly configured GOOSE_SHELL, honour the full path
-/// (it likely refers to a host binary, e.g. a Nix-profile shell).
-/// Otherwise strip to basename so the host's default PATH resolves it.
 #[cfg(not(windows))]
-fn flatpak_shell_arg(shell: &str, is_user_configured: bool) -> &str {
-    if is_user_configured {
-        shell
+fn shell_basename(shell: &str) -> String {
+    std::path::Path::new(shell)
+        .file_name()
+        .and_then(|s| s.to_str())
+        .unwrap_or(shell)
+        .to_string()
+}
+
+/// Basename of the shell the `shell` tool will invoke, for use in the tool
+/// description so the LLM knows which dialect to write.
+#[cfg(windows)]
+pub fn shell_display_name() -> String {
+    shell_basename(&windows_shell())
+}
+
+#[cfg(not(windows))]
+pub fn shell_display_name() -> String {
+    shell_basename(&unix_shell())
+}
+
+/// The shell tool runs commands with `-c "..."`, and LLMs routinely emit
+/// POSIX-style patterns such as heredocs (`cat <<EOF > file`), `$VAR`
+/// expansion, and `2>&1` redirection. Non-POSIX shells (fish, csh, tcsh,
+/// nu, ...) reject or mis-interpret these, so we don't auto-select based
+/// on `$SHELL`: we check whether `bash` is on PATH and otherwise fall back
+/// to `sh`. Users who really want their login shell can opt in via
+/// `GOOSE_SHELL`.
+#[cfg(not(windows))]
+fn unix_shell() -> String {
+    if let Ok(shell) = std::env::var("GOOSE_SHELL") {
+        return shell;
+    }
+    if which::which("bash").is_ok() {
+        "bash".to_string()
     } else {
-        std::path::Path::new(shell)
-            .file_name()
-            .and_then(|s| s.to_str())
-            .unwrap_or("bash")
+        "sh".to_string()
     }
 }
 
@@ -149,17 +166,11 @@ pub struct ShellOutput {
 /// source the user's profile and recover the full PATH.
 #[cfg(not(windows))]
 fn resolve_login_shell_path() -> Option<String> {
-    let (shell, is_user_configured) = unix_shell();
+    let shell = unix_shell();
 
     let mut child = if is_flatpak() {
         flatpak_spawn_process()
-            .args([
-                flatpak_shell_arg(&shell, is_user_configured),
-                "-l",
-                "-i",
-                "-c",
-                "echo $PATH",
-            ])
+            .args([&shell, "-l", "-i", "-c", "echo $PATH"])
             .stdin(Stdio::null())
             .stdout(Stdio::piped())
             .stderr(Stdio::null())
@@ -540,12 +551,8 @@ fn build_shell_command(
 ) -> tokio::process::Command {
     #[cfg(windows)]
     let mut command = {
-        let shell = std::env::var("GOOSE_SHELL").unwrap_or_else(|_| "cmd".to_string());
-        let shell_stem = Path::new(&shell)
-            .file_stem()
-            .and_then(|s| s.to_str())
-            .unwrap_or("cmd")
-            .to_lowercase();
+        let shell = windows_shell();
+        let shell_stem = shell_basename(&shell);
         let mut command = tokio::process::Command::new(&shell);
         match shell_stem.as_str() {
             "pwsh" | "powershell" => {
@@ -570,7 +577,7 @@ fn build_shell_command(
 
     #[cfg(not(windows))]
     let mut command = {
-        let (shell, is_user_configured) = unix_shell();
+        let shell = unix_shell();
 
         if is_flatpak() {
             let mut command = flatpak_spawn_command();
@@ -580,12 +587,7 @@ fn build_shell_command(
             if let Some(path) = login_path {
                 command.arg(format!("--env=PATH={}", path));
             }
-            // If GOOSE_SHELL was explicitly set, honour the full path (likely a host
-            // binary). Otherwise use basename so the host's PATH resolves it.
-            command
-                .arg(flatpak_shell_arg(&shell, is_user_configured))
-                .arg("-c")
-                .arg(command_line);
+            command.arg(&shell).arg("-c").arg(command_line);
             command
         } else {
             let mut command = tokio::process::Command::new(shell);