Make `ShellBuilder::new` not branch on a remote shell (#39493)

Lukas Wirth , Max Brunsfeld , and Cole Miller created 1 month ago

Release Notes:

- Fixed claude code agent login on remotes

Co-authored-by: Max Brunsfeld <[e-mail]>
Co-authored-by: Cole Miller <[e-mail]>

Change summary

crates/acp_thread/src/acp_thread.rs           | 22 +++++++++-----------
crates/agent_servers/src/acp.rs               | 21 +++++++++++--------
crates/assistant_tools/src/terminal_tool.rs   | 21 +++++++++----------
crates/debugger_ui/src/session/running.rs     | 12 +++++++---
crates/project/src/debugger/locators/cargo.rs |  2 
crates/project/src/terminals.rs               |  8 ++++--
crates/remote/src/transport/ssh.rs            | 11 +++------
crates/task/src/shell_builder.rs              | 17 ++++++---------
crates/terminal/src/terminal.rs               |  6 ++--
crates/terminal_view/src/terminal_panel.rs    | 12 +++++++++-
10 files changed, 70 insertions(+), 62 deletions(-)

Detailed changes

crates/acp_thread/src/acp_thread.rs

@@ -2109,18 +2109,16 @@ impl AcpThread {
             let terminal_id = terminal_id.clone();
             async move |_this, cx| {
                 let env = env.await;
-                let (task_command, task_args) = ShellBuilder::new(
-                    project
-                        .update(cx, |project, cx| {
-                            project
-                                .remote_client()
-                                .and_then(|r| r.read(cx).default_system_shell())
-                        })?
-                        .as_deref(),
-                    &Shell::Program(get_default_system_shell()),
-                )
-                .redirect_stdin_to_dev_null()
-                .build(Some(command.clone()), &args);
+                let shell = project
+                    .update(cx, |project, cx| {
+                        project
+                            .remote_client()
+                            .and_then(|r| r.read(cx).default_system_shell())
+                    })?
+                    .unwrap_or_else(|| get_default_system_shell());
+                let (task_command, task_args) = ShellBuilder::new(&Shell::Program(shell))
+                    .redirect_stdin_to_dev_null()
+                    .build(Some(command.clone()), &args);
                 let terminal = project
                     .update(cx, |project, cx| {
                         project.create_terminal_task(

crates/agent_servers/src/acp.rs

@@ -9,6 +9,7 @@ use futures::io::BufReader;
 use project::Project;
 use project::agent_server_store::AgentServerCommand;
 use serde::Deserialize;
+use task::Shell;
 use util::ResultExt as _;
 
 use std::path::PathBuf;
@@ -820,15 +821,17 @@ impl acp::Client for ClientDelegate {
         }
 
         // Use remote shell or default system shell, as appropriate
-        let remote_shell = project.update(&mut self.cx.clone(), |project, cx| {
-            project
-                .remote_client()
-                .and_then(|r| r.read(cx).default_system_shell())
-        })?;
-        let (task_command, task_args) =
-            task::ShellBuilder::new(remote_shell.as_deref(), &task::Shell::System)
-                .redirect_stdin_to_dev_null()
-                .build(Some(args.command.clone()), &args.args);
+        let shell = project
+            .update(&mut self.cx.clone(), |project, cx| {
+                project
+                    .remote_client()
+                    .and_then(|r| r.read(cx).default_system_shell())
+                    .map(Shell::Program)
+            })?
+            .unwrap_or(task::Shell::System);
+        let (task_command, task_args) = task::ShellBuilder::new(&shell)
+            .redirect_stdin_to_dev_null()
+            .build(Some(args.command.clone()), &args.args);
 
         let terminal_entity = project
             .update(&mut self.cx.clone(), |project, cx| {

crates/assistant_tools/src/terminal_tool.rs

@@ -127,11 +127,13 @@ impl Tool for TerminalTool {
             }),
             None => Task::ready(None).shared(),
         };
-        let remote_shell = project.update(cx, |project, cx| {
-            project
-                .remote_client()
-                .and_then(|r| r.read(cx).default_system_shell())
-        });
+        let shell = project
+            .update(cx, |project, cx| {
+                project
+                    .remote_client()
+                    .and_then(|r| r.read(cx).default_system_shell())
+            })
+            .unwrap_or_else(|| get_default_system_shell());
 
         let env = cx.spawn(async move |_| {
             let mut env = env.await.unwrap_or_default();
@@ -144,12 +146,9 @@ impl Tool for TerminalTool {
         let build_cmd = {
             let input_command = input.command.clone();
             move || {
-                ShellBuilder::new(
-                    remote_shell.as_deref(),
-                    &Shell::Program(get_default_system_shell()),
-                )
-                .redirect_stdin_to_dev_null()
-                .build(Some(input_command.clone()), &[])
+                ShellBuilder::new(&Shell::Program(shell))
+                    .redirect_stdin_to_dev_null()
+                    .build(Some(input_command), &[])
             }
         };
 

crates/debugger_ui/src/session/running.rs

@@ -41,8 +41,8 @@ use serde_json::Value;
 use settings::Settings;
 use stack_frame_list::StackFrameList;
 use task::{
-    BuildTaskDefinition, DebugScenario, ShellBuilder, SpawnInTerminal, TaskContext, ZedDebugConfig,
-    substitute_variables_in_str,
+    BuildTaskDefinition, DebugScenario, Shell, ShellBuilder, SpawnInTerminal, TaskContext,
+    ZedDebugConfig, substitute_variables_in_str,
 };
 use terminal_view::TerminalView;
 use ui::{
@@ -988,7 +988,7 @@ impl RunningState {
                         (task, None)
                     }
                 };
-                let Some(task) = task_template.resolve_task("debug-build-task", &task_context) else {
+                let Some(mut task) = task_template.resolve_task("debug-build-task", &task_context) else {
                     anyhow::bail!("Could not resolve task variables within a debug scenario");
                 };
 
@@ -1025,7 +1025,11 @@ impl RunningState {
                     None
                 };
 
-                let builder = ShellBuilder::new(remote_shell.as_deref(), &task.resolved.shell);
+                if let Some(remote_shell) = remote_shell && task.resolved.shell == Shell::System {
+                    task.resolved.shell = Shell::Program(remote_shell);
+                }
+
+                let builder = ShellBuilder::new(&task.resolved.shell);
                 let command_label = builder.command_label(task.resolved.command.as_deref().unwrap_or(""));
                 let (command, args) =
                     builder.build(task.resolved.command.clone(), &task.resolved.args);

crates/project/src/debugger/locators/cargo.rs

@@ -117,7 +117,7 @@ impl DapLocator for CargoLocator {
             .cwd
             .clone()
             .context("Couldn't get cwd from debug config which is needed for locators")?;
-        let builder = ShellBuilder::new(None, &build_config.shell).non_interactive();
+        let builder = ShellBuilder::new(&build_config.shell).non_interactive();
         let (program, args) = builder.build(
             Some("cargo".into()),
             &build_config

crates/project/src/terminals.rs

@@ -452,10 +452,12 @@ impl Project {
         let path = self.first_project_directory(cx);
         let remote_client = self.remote_client.as_ref();
         let settings = self.terminal_settings(&path, cx).clone();
-        let remote_shell = remote_client
+        let shell = remote_client
             .as_ref()
-            .and_then(|remote_client| remote_client.read(cx).shell());
-        let builder = ShellBuilder::new(remote_shell.as_deref(), &settings.shell).non_interactive();
+            .and_then(|remote_client| remote_client.read(cx).shell())
+            .map(Shell::Program)
+            .unwrap_or_else(|| settings.shell.clone());
+        let builder = ShellBuilder::new(&shell).non_interactive();
         let (command, args) = builder.build(Some(command), &Vec::new());
 
         let mut env = self

crates/remote/src/transport/ssh.rs

@@ -1055,17 +1055,14 @@ fn build_command(
         }
     }
 
-    write!(exec, "{ssh_shell} ").unwrap();
     if let Some(input_program) = input_program {
-        let mut script = shlex::try_quote(&input_program)?.into_owned();
+        write!(exec, "{}", shlex::try_quote(&input_program).unwrap()).unwrap();
         for arg in input_args {
             let arg = shlex::try_quote(&arg)?;
-            script.push_str(" ");
-            script.push_str(&arg);
+            write!(exec, " {}", &arg).unwrap();
         }
-        write!(exec, "-c {}", shlex::try_quote(&script).unwrap()).unwrap();
     } else {
-        write!(exec, "-l").unwrap();
+        write!(exec, "{ssh_shell} -l").unwrap();
     };
 
     let mut args = Vec::new();
@@ -1115,7 +1112,7 @@ mod tests {
                 "-p",
                 "2222",
                 "-t",
-                "exec env -C \"$HOME/work\" INPUT_VA=val /bin/fish -c 'remote_program arg1 arg2'"
+                "exec env -C \"$HOME/work\" INPUT_VA=val remote_program arg1 arg2"
             ]
         );
         assert_eq!(command.env, env);

crates/task/src/shell_builder.rs

@@ -18,14 +18,11 @@ pub struct ShellBuilder {
 
 impl ShellBuilder {
     /// Create a new ShellBuilder as configured.
-    pub fn new(remote_system_shell: Option<&str>, shell: &Shell) -> Self {
-        let (program, args) = match remote_system_shell {
-            Some(program) => (program.to_string(), Vec::new()),
-            None => match shell {
-                Shell::System => (get_system_shell(), Vec::new()),
-                Shell::Program(shell) => (shell.clone(), Vec::new()),
-                Shell::WithArguments { program, args, .. } => (program.clone(), args.clone()),
-            },
+    pub fn new(shell: &Shell) -> Self {
+        let (program, args) = match shell {
+            Shell::System => (get_system_shell(), Vec::new()),
+            Shell::Program(shell) => (shell.clone(), Vec::new()),
+            Shell::WithArguments { program, args, .. } => (program.clone(), args.clone()),
         };
 
         let kind = ShellKind::new(&program);
@@ -123,7 +120,7 @@ mod test {
     #[test]
     fn test_nu_shell_variable_substitution() {
         let shell = Shell::Program("nu".to_owned());
-        let shell_builder = ShellBuilder::new(None, &shell);
+        let shell_builder = ShellBuilder::new(&shell);
 
         let (program, args) = shell_builder.build(
             Some("echo".into()),
@@ -151,7 +148,7 @@ mod test {
     #[test]
     fn redirect_stdin_to_dev_null_precedence() {
         let shell = Shell::Program("nu".to_owned());
-        let shell_builder = ShellBuilder::new(None, &shell);
+        let shell_builder = ShellBuilder::new(&shell);
 
         let (program, args) = shell_builder
             .redirect_stdin_to_dev_null()

crates/terminal/src/terminal.rs

@@ -2276,8 +2276,8 @@ mod tests {
         cx.executor().allow_parking();
 
         let (completion_tx, completion_rx) = smol::channel::unbounded();
-        let (program, args) = ShellBuilder::new(None, &Shell::System)
-            .build(Some("echo".to_owned()), &["hello".to_owned()]);
+        let (program, args) =
+            ShellBuilder::new(&Shell::System).build(Some("echo".to_owned()), &["hello".to_owned()]);
         let terminal = cx.new(|cx| {
             TerminalBuilder::new(
                 None,
@@ -2395,7 +2395,7 @@ mod tests {
         cx.executor().allow_parking();
 
         let (completion_tx, completion_rx) = smol::channel::unbounded();
-        let (program, args) = ShellBuilder::new(None, &Shell::System)
+        let (program, args) = ShellBuilder::new(&Shell::System)
             .build(Some("asdasdasdasd".to_owned()), &["@@@@@".to_owned()]);
         let terminal = cx.new(|cx| {
             TerminalBuilder::new(

crates/terminal_view/src/terminal_panel.rs

@@ -19,7 +19,7 @@ use itertools::Itertools;
 use project::{Fs, Project, ProjectEntryId};
 use search::{BufferSearchBar, buffer_search::DivRegistrar};
 use settings::{Settings, TerminalDockPosition};
-use task::{RevealStrategy, RevealTarget, ShellBuilder, SpawnInTerminal, TaskId};
+use task::{RevealStrategy, RevealTarget, Shell, ShellBuilder, SpawnInTerminal, TaskId};
 use terminal::{Terminal, terminal_settings::TerminalSettings};
 use ui::{
     ButtonCommon, Clickable, ContextMenu, FluentBuilder, PopoverMenu, Toggleable, Tooltip,
@@ -543,7 +543,15 @@ impl TerminalPanel {
             .as_ref()
             .and_then(|remote_client| remote_client.read(cx).shell());
 
-        let builder = ShellBuilder::new(remote_shell.as_deref(), &task.shell);
+        let shell = if let Some(remote_shell) = remote_shell
+            && task.shell == Shell::System
+        {
+            Shell::Program(remote_shell)
+        } else {
+            task.shell.clone()
+        };
+
+        let builder = ShellBuilder::new(&shell);
         let command_label = builder.command_label(task.command.as_deref().unwrap_or(""));
         let (command, args) = builder.build(task.command.clone(), &task.args);