acp_thread: Fix terminal tool incorrectly redirecting stdin to `/dev/null`

Lukas Wirth created

Change summary

crates/acp_thread/src/acp_thread.rs         |  8 ++--
crates/assistant_tools/src/terminal_tool.rs | 45 +++++++++++-----------
crates/task/src/shell_builder.rs            | 20 +++++++++-
3 files changed, 45 insertions(+), 28 deletions(-)

Detailed changes

crates/acp_thread/src/acp_thread.rs 🔗

@@ -1985,7 +1985,7 @@ impl AcpThread {
             let terminal_id = terminal_id.clone();
             async move |_this, cx| {
                 let env = env.await;
-                let (command, args) = ShellBuilder::new(
+                let (task_command, task_args) = ShellBuilder::new(
                     project
                         .update(cx, |project, cx| {
                             project
@@ -1996,13 +1996,13 @@ impl AcpThread {
                     &Shell::Program(get_default_system_shell()),
                 )
                 .redirect_stdin_to_dev_null()
-                .build(Some(command), &args);
+                .build(Some(command.clone()), &args);
                 let terminal = project
                     .update(cx, |project, cx| {
                         project.create_terminal_task(
                             task::SpawnInTerminal {
-                                command: Some(command.clone()),
-                                args: args.clone(),
+                                command: Some(task_command),
+                                args: task_args,
                                 cwd: cwd.clone(),
                                 env,
                                 ..Default::default()

crates/assistant_tools/src/terminal_tool.rs 🔗

@@ -139,18 +139,25 @@ impl Tool for TerminalTool {
             env
         });
 
+        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()), &[])
+            }
+        };
+
         let Some(window) = window else {
             // Headless setup, a test or eval. Our terminal subsystem requires a workspace,
             // so bypass it and provide a convincing imitation using a pty.
             let task = cx.background_spawn(async move {
                 let env = env.await;
                 let pty_system = native_pty_system();
-                let (command, args) = ShellBuilder::new(
-                    remote_shell.as_deref(),
-                    &Shell::Program(get_default_system_shell()),
-                )
-                .redirect_stdin_to_dev_null()
-                .build(Some(input.command.clone()), &[]);
+                let (command, args) = build_cmd();
                 let mut cmd = CommandBuilder::new(command);
                 cmd.args(args);
                 for (k, v) in env {
@@ -187,16 +194,10 @@ impl Tool for TerminalTool {
             };
         };
 
-        let command = input.command.clone();
         let terminal = cx.spawn({
             let project = project.downgrade();
             async move |cx| {
-                let (command, args) = ShellBuilder::new(
-                    remote_shell.as_deref(),
-                    &Shell::Program(get_default_system_shell()),
-                )
-                .redirect_stdin_to_dev_null()
-                .build(Some(input.command), &[]);
+                let (command, args) = build_cmd();
                 let env = env.await;
                 project
                     .update(cx, |project, cx| {
@@ -215,18 +216,18 @@ impl Tool for TerminalTool {
             }
         });
 
-        let command_markdown =
-            cx.new(|cx| Markdown::new(format!("```bash\n{}\n```", command).into(), None, None, cx));
-
-        let card = cx.new(|cx| {
-            TerminalToolCard::new(
-                command_markdown.clone(),
-                working_dir.clone(),
-                cx.entity_id(),
+        let command_markdown = cx.new(|cx| {
+            Markdown::new(
+                format!("```bash\n{}\n```", input.command).into(),
+                None,
+                None,
                 cx,
             )
         });
 
+        let card =
+            cx.new(|cx| TerminalToolCard::new(command_markdown, working_dir, cx.entity_id(), cx));
+
         let output = cx.spawn({
             let card = card.clone();
             async move |cx| {
@@ -267,7 +268,7 @@ impl Tool for TerminalTool {
                 let previous_len = content.len();
                 let (processed_content, finished_with_empty_output) = process_content(
                     &content,
-                    &command,
+                    &input.command,
                     exit_status.map(portable_pty::ExitStatus::from),
                 );
 

crates/task/src/shell_builder.rs 🔗

@@ -210,6 +210,7 @@ pub struct ShellBuilder {
     program: String,
     args: Vec<String>,
     interactive: bool,
+    /// Whether to redirect stdin to /dev/null for the spawned command as a subshell.
     redirect_stdin: bool,
     kind: ShellKind,
 }
@@ -279,10 +280,12 @@ impl ShellBuilder {
             if self.redirect_stdin {
                 match self.kind {
                     ShellKind::Posix | ShellKind::Nushell | ShellKind::Fish | ShellKind::Csh => {
-                        combined_command.push_str(" </dev/null");
+                        combined_command.insert(0, '(');
+                        combined_command.push_str(") </dev/null");
                     }
                     ShellKind::PowerShell => {
-                        combined_command.insert_str(0, "$null | ");
+                        combined_command.insert_str(0, "$null | {");
+                        combined_command.push_str("}");
                     }
                     ShellKind::Cmd => {
                         combined_command.push_str("< NUL");
@@ -329,4 +332,17 @@ 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 (program, args) = shell_builder
+            .redirect_stdin_to_dev_null()
+            .build(Some("echo".into()), &["nothing".to_string()]);
+
+        assert_eq!(program, "nu");
+        assert_eq!(args, vec!["-i", "-c", "(echo nothing) </dev/null"]);
+    }
 }