Cherry-pick fix for JS debug terminal scenario (#34101)

Cole Miller and Remco Smits created

Manual cherry-pick of #33924 

Release Notes:

- Debugger: Fix RunInTerminal not working for JavaScript debugger.

---------

Co-authored-by: Remco Smits <djsmits12@gmail.com>

Change summary

crates/assistant_tools/src/terminal_tool.rs             |   2 
crates/dap_adapters/src/javascript.rs                   |  13 
crates/debugger_ui/src/session/running.rs               |  87 +++--
crates/extension_host/src/wasm_host/wit.rs              |   2 
crates/extension_host/src/wasm_host/wit/since_v0_6_0.rs |  12 
crates/project/src/debugger/locators/cargo.rs           |   2 
crates/project/src/project_tests.rs                     |   2 
crates/project/src/terminals.rs                         |  19 
crates/proto/proto/debugger.proto                       |   2 
crates/task/src/lib.rs                                  |  72 ++-
crates/task/src/shell_builder.rs                        | 172 +++++++++++
crates/task/src/task_template.rs                        |   8 
crates/terminal_view/src/terminal_panel.rs              |   2 
crates/vim/src/command.rs                               |   2 
14 files changed, 306 insertions(+), 91 deletions(-)

Detailed changes

crates/assistant_tools/src/terminal_tool.rs 🔗

@@ -218,7 +218,7 @@ impl Tool for TerminalTool {
                     .update(cx, |project, cx| {
                         project.create_terminal(
                             TerminalKind::Task(task::SpawnInTerminal {
-                                command: program,
+                                command: Some(program),
                                 args,
                                 cwd,
                                 env,

crates/dap_adapters/src/javascript.rs 🔗

@@ -1,9 +1,10 @@
 use adapters::latest_github_release;
 use anyhow::Context as _;
+use collections::HashMap;
 use dap::{StartDebuggingRequestArguments, adapters::DebugTaskDefinition};
 use gpui::AsyncApp;
 use serde_json::Value;
-use std::{collections::HashMap, path::PathBuf, sync::OnceLock};
+use std::{path::PathBuf, sync::OnceLock};
 use task::DebugRequest;
 use util::{ResultExt, maybe};
 
@@ -70,6 +71,8 @@ impl JsDebugAdapter {
         let tcp_connection = task_definition.tcp_connection.clone().unwrap_or_default();
         let (host, port, timeout) = crate::configure_tcp_connection(tcp_connection).await?;
 
+        let mut envs = HashMap::default();
+
         let mut configuration = task_definition.config.clone();
         if let Some(configuration) = configuration.as_object_mut() {
             maybe!({
@@ -110,6 +113,12 @@ impl JsDebugAdapter {
                 }
             }
 
+            if let Some(env) = configuration.get("env").cloned() {
+                if let Ok(env) = serde_json::from_value(env) {
+                    envs = env;
+                }
+            }
+
             configuration
                 .entry("cwd")
                 .or_insert(delegate.worktree_root_path().to_string_lossy().into());
@@ -158,7 +167,7 @@ impl JsDebugAdapter {
             ),
             arguments,
             cwd: Some(delegate.worktree_root_path().to_path_buf()),
-            envs: HashMap::default(),
+            envs,
             connection: Some(adapters::TcpArguments {
                 host,
                 port,

crates/debugger_ui/src/session/running.rs 🔗

@@ -973,7 +973,7 @@ impl RunningState {
 
                 let task_with_shell = SpawnInTerminal {
                     command_label,
-                    command,
+                    command: Some(command),
                     args,
                     ..task.resolved.clone()
                 };
@@ -1085,19 +1085,6 @@ impl RunningState {
             .map(PathBuf::from)
             .or_else(|| session.binary().unwrap().cwd.clone());
 
-        let mut args = request.args.clone();
-
-        // Handle special case for NodeJS debug adapter
-        // If only the Node binary path is provided, we set the command to None
-        // This prevents the NodeJS REPL from appearing, which is not the desired behavior
-        // The expected usage is for users to provide their own Node command, e.g., `node test.js`
-        // This allows the NodeJS debug client to attach correctly
-        let command = if args.len() > 1 {
-            Some(args.remove(0))
-        } else {
-            None
-        };
-
         let mut envs: HashMap<String, String> =
             self.session.read(cx).task_context().project_env.clone();
         if let Some(Value::Object(env)) = &request.env {
@@ -1111,32 +1098,58 @@ impl RunningState {
             }
         }
 
-        let shell = project.read(cx).terminal_settings(&cwd, cx).shell.clone();
-        let kind = if let Some(command) = command {
-            let title = request.title.clone().unwrap_or(command.clone());
-            TerminalKind::Task(task::SpawnInTerminal {
-                id: task::TaskId("debug".to_string()),
-                full_label: title.clone(),
-                label: title.clone(),
-                command: command.clone(),
-                args,
-                command_label: title.clone(),
-                cwd,
-                env: envs,
-                use_new_terminal: true,
-                allow_concurrent_runs: true,
-                reveal: task::RevealStrategy::NoFocus,
-                reveal_target: task::RevealTarget::Dock,
-                hide: task::HideStrategy::Never,
-                shell,
-                show_summary: false,
-                show_command: false,
-                show_rerun: false,
-            })
+        let mut args = request.args.clone();
+        let command = if envs.contains_key("VSCODE_INSPECTOR_OPTIONS") {
+            // Handle special case for NodeJS debug adapter
+            // If the Node binary path is provided (possibly with arguments like --experimental-network-inspection),
+            // we set the command to None
+            // This prevents the NodeJS REPL from appearing, which is not the desired behavior
+            // The expected usage is for users to provide their own Node command, e.g., `node test.js`
+            // This allows the NodeJS debug client to attach correctly
+            if args
+                .iter()
+                .filter(|arg| !arg.starts_with("--"))
+                .collect::<Vec<_>>()
+                .len()
+                > 1
+            {
+                Some(args.remove(0))
+            } else {
+                None
+            }
+        } else if args.len() > 0 {
+            Some(args.remove(0))
         } else {
-            TerminalKind::Shell(cwd.map(|c| c.to_path_buf()))
+            None
         };
 
+        let shell = project.read(cx).terminal_settings(&cwd, cx).shell.clone();
+        let title = request
+            .title
+            .clone()
+            .filter(|title| !title.is_empty())
+            .or_else(|| command.clone())
+            .unwrap_or_else(|| "Debug terminal".to_string());
+        let kind = TerminalKind::Task(task::SpawnInTerminal {
+            id: task::TaskId("debug".to_string()),
+            full_label: title.clone(),
+            label: title.clone(),
+            command: command.clone(),
+            args,
+            command_label: title.clone(),
+            cwd,
+            env: envs,
+            use_new_terminal: true,
+            allow_concurrent_runs: true,
+            reveal: task::RevealStrategy::NoFocus,
+            reveal_target: task::RevealTarget::Dock,
+            hide: task::HideStrategy::Never,
+            shell,
+            show_summary: false,
+            show_command: false,
+            show_rerun: false,
+        });
+
         let workspace = self.workspace.clone();
         let weak_project = project.downgrade();
 

crates/extension_host/src/wasm_host/wit.rs 🔗

@@ -999,7 +999,7 @@ impl Extension {
     ) -> Result<Result<DebugRequest, String>> {
         match self {
             Extension::V0_6_0(ext) => {
-                let build_config_template = resolved_build_task.into();
+                let build_config_template = resolved_build_task.try_into()?;
                 let dap_request = ext
                     .call_run_dap_locator(store, &locator_name, &build_config_template)
                     .await?

crates/extension_host/src/wasm_host/wit/since_v0_6_0.rs 🔗

@@ -299,15 +299,17 @@ impl From<extension::DebugScenario> for DebugScenario {
     }
 }
 
-impl From<SpawnInTerminal> for ResolvedTask {
-    fn from(value: SpawnInTerminal) -> Self {
-        Self {
+impl TryFrom<SpawnInTerminal> for ResolvedTask {
+    type Error = anyhow::Error;
+
+    fn try_from(value: SpawnInTerminal) -> Result<Self, Self::Error> {
+        Ok(Self {
             label: value.label,
-            command: value.command,
+            command: value.command.context("missing command")?,
             args: value.args,
             env: value.env.into_iter().collect(),
             cwd: value.cwd.map(|s| s.to_string_lossy().into_owned()),
-        }
+        })
     }
 }
 

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

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

crates/project/src/project_tests.rs 🔗

@@ -568,7 +568,7 @@ async fn test_fallback_to_single_worktree_tasks(cx: &mut gpui::TestAppContext) {
             .into_iter()
             .map(|(source_kind, task)| {
                 let resolved = task.resolved;
-                (source_kind, resolved.command)
+                (source_kind, resolved.command.unwrap())
             })
             .collect::<Vec<_>>(),
         vec![(

crates/project/src/terminals.rs 🔗

@@ -149,7 +149,7 @@ impl Project {
         let settings = self.terminal_settings(&path, cx).clone();
 
         let builder = ShellBuilder::new(ssh_details.is_none(), &settings.shell).non_interactive();
-        let (command, args) = builder.build(command, &Vec::new());
+        let (command, args) = builder.build(Some(command), &Vec::new());
 
         let mut env = self
             .environment
@@ -297,7 +297,10 @@ impl Project {
                             .or_insert_with(|| "xterm-256color".to_string());
                         let (program, args) = wrap_for_ssh(
                             &ssh_command,
-                            Some((&spawn_task.command, &spawn_task.args)),
+                            spawn_task
+                                .command
+                                .as_ref()
+                                .map(|command| (command, &spawn_task.args)),
                             path.as_deref(),
                             env,
                             python_venv_directory.as_deref(),
@@ -317,14 +320,16 @@ impl Project {
                             add_environment_path(&mut env, &venv_path.join("bin")).log_err();
                         }
 
-                        (
-                            task_state,
+                        let shell = if let Some(program) = spawn_task.command {
                             Shell::WithArguments {
-                                program: spawn_task.command,
+                                program,
                                 args: spawn_task.args,
                                 title_override: None,
-                            },
-                        )
+                            }
+                        } else {
+                            Shell::System
+                        };
+                        (task_state, shell)
                     }
                 }
             }

crates/proto/proto/debugger.proto 🔗

@@ -535,7 +535,7 @@ message DebugScenario {
 
 message SpawnInTerminal {
     string label = 1;
-    string command = 2;
+    optional string command = 2;
     repeated string args = 3;
     map<string, string> env = 4;
     optional string cwd = 5;

crates/task/src/lib.rs 🔗

@@ -44,7 +44,7 @@ pub struct SpawnInTerminal {
     /// Human readable name of the terminal tab.
     pub label: String,
     /// Executable command to spawn.
-    pub command: String,
+    pub command: Option<String>,
     /// Arguments to the command, potentially unsubstituted,
     /// to let the shell that spawns the command to do the substitution, if needed.
     pub args: Vec<String>,
@@ -387,20 +387,26 @@ impl ShellBuilder {
     }
 
     /// Returns the program and arguments to run this task in a shell.
-    pub fn build(mut self, task_command: String, task_args: &Vec<String>) -> (String, Vec<String>) {
-        let combined_command = task_args
-            .into_iter()
-            .fold(task_command, |mut command, arg| {
-                command.push(' ');
-                command.push_str(&arg);
-                command
-            });
-        self.args.extend(
-            self.interactive
-                .then(|| "-i".to_owned())
+    pub fn build(
+        mut self,
+        task_command: Option<String>,
+        task_args: &Vec<String>,
+    ) -> (String, Vec<String>) {
+        if let Some(task_command) = task_command {
+            let combined_command = task_args
                 .into_iter()
-                .chain(["-c".to_owned(), combined_command]),
-        );
+                .fold(task_command, |mut command, arg| {
+                    command.push(' ');
+                    command.push_str(&arg);
+                    command
+                });
+            self.args.extend(
+                self.interactive
+                    .then(|| "-i".to_owned())
+                    .into_iter()
+                    .chain(["-c".to_owned(), combined_command]),
+            );
+        }
 
         (self.program, self.args)
     }
@@ -428,21 +434,29 @@ impl ShellBuilder {
     }
 
     /// Returns the program and arguments to run this task in a shell.
-    pub fn build(mut self, task_command: String, task_args: &Vec<String>) -> (String, Vec<String>) {
-        let combined_command = task_args
-            .into_iter()
-            .fold(task_command, |mut command, arg| {
-                command.push(' ');
-                command.push_str(&self.to_windows_shell_variable(arg.to_string()));
-                command
-            });
-
-        match self.windows_shell_type() {
-            WindowsShellType::Powershell => self.args.extend(["-C".to_owned(), combined_command]),
-            WindowsShellType::Cmd => self.args.extend(["/C".to_owned(), combined_command]),
-            WindowsShellType::Other => {
-                self.args
-                    .extend(["-i".to_owned(), "-c".to_owned(), combined_command])
+    pub fn build(
+        mut self,
+        task_command: Option<String>,
+        task_args: &Vec<String>,
+    ) -> (String, Vec<String>) {
+        if let Some(task_command) = task_command {
+            let combined_command = task_args
+                .into_iter()
+                .fold(task_command, |mut command, arg| {
+                    command.push(' ');
+                    command.push_str(&self.to_windows_shell_variable(arg.to_string()));
+                    command
+                });
+
+            match self.windows_shell_type() {
+                WindowsShellType::Powershell => {
+                    self.args.extend(["-C".to_owned(), combined_command])
+                }
+                WindowsShellType::Cmd => self.args.extend(["/C".to_owned(), combined_command]),
+                WindowsShellType::Other => {
+                    self.args
+                        .extend(["-i".to_owned(), "-c".to_owned(), combined_command])
+                }
             }
         }
 

crates/task/src/shell_builder.rs 🔗

@@ -0,0 +1,172 @@
+use crate::Shell;
+
+#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
+enum ShellKind {
+    #[default]
+    Posix,
+    Powershell,
+    Cmd,
+}
+
+impl ShellKind {
+    fn new(program: &str) -> Self {
+        if program == "powershell"
+            || program.ends_with("powershell.exe")
+            || program == "pwsh"
+            || program.ends_with("pwsh.exe")
+        {
+            ShellKind::Powershell
+        } else if program == "cmd" || program.ends_with("cmd.exe") {
+            ShellKind::Cmd
+        } else {
+            // Someother shell detected, the user might install and use a
+            // unix-like shell.
+            ShellKind::Posix
+        }
+    }
+
+    fn to_shell_variable(&self, input: &str) -> String {
+        match self {
+            Self::Powershell => Self::to_powershell_variable(input),
+            Self::Cmd => Self::to_cmd_variable(input),
+            Self::Posix => input.to_owned(),
+        }
+    }
+
+    fn to_cmd_variable(input: &str) -> String {
+        if let Some(var_str) = input.strip_prefix("${") {
+            if var_str.find(':').is_none() {
+                // If the input starts with "${", remove the trailing "}"
+                format!("%{}%", &var_str[..var_str.len() - 1])
+            } else {
+                // `${SOME_VAR:-SOME_DEFAULT}`, we currently do not handle this situation,
+                // which will result in the task failing to run in such cases.
+                input.into()
+            }
+        } else if let Some(var_str) = input.strip_prefix('$') {
+            // If the input starts with "$", directly append to "$env:"
+            format!("%{}%", var_str)
+        } else {
+            // If no prefix is found, return the input as is
+            input.into()
+        }
+    }
+    fn to_powershell_variable(input: &str) -> String {
+        if let Some(var_str) = input.strip_prefix("${") {
+            if var_str.find(':').is_none() {
+                // If the input starts with "${", remove the trailing "}"
+                format!("$env:{}", &var_str[..var_str.len() - 1])
+            } else {
+                // `${SOME_VAR:-SOME_DEFAULT}`, we currently do not handle this situation,
+                // which will result in the task failing to run in such cases.
+                input.into()
+            }
+        } else if let Some(var_str) = input.strip_prefix('$') {
+            // If the input starts with "$", directly append to "$env:"
+            format!("$env:{}", var_str)
+        } else {
+            // If no prefix is found, return the input as is
+            input.into()
+        }
+    }
+
+    fn args_for_shell(&self, interactive: bool, combined_command: String) -> Vec<String> {
+        match self {
+            ShellKind::Powershell => vec!["-C".to_owned(), combined_command],
+            ShellKind::Cmd => vec!["/C".to_owned(), combined_command],
+            ShellKind::Posix => interactive
+                .then(|| "-i".to_owned())
+                .into_iter()
+                .chain(["-c".to_owned(), combined_command])
+                .collect(),
+        }
+    }
+}
+
+fn system_shell() -> String {
+    if cfg!(target_os = "windows") {
+        // `alacritty_terminal` uses this as default on Windows. See:
+        // https://github.com/alacritty/alacritty/blob/0d4ab7bca43213d96ddfe40048fc0f922543c6f8/alacritty_terminal/src/tty/windows/mod.rs#L130
+        // We could use `util::get_windows_system_shell()` here, but we are running tasks here, so leave it to `powershell.exe`
+        // should be okay.
+        "powershell.exe".to_string()
+    } else {
+        std::env::var("SHELL").unwrap_or("/bin/sh".to_string())
+    }
+}
+
+/// ShellBuilder is used to turn a user-requested task into a
+/// program that can be executed by the shell.
+pub struct ShellBuilder {
+    /// The shell to run
+    program: String,
+    args: Vec<String>,
+    interactive: bool,
+    kind: ShellKind,
+}
+
+pub static DEFAULT_REMOTE_SHELL: &str = "\"${SHELL:-sh}\"";
+
+impl ShellBuilder {
+    /// Create a new ShellBuilder as configured.
+    pub fn new(is_local: bool, shell: &Shell) -> Self {
+        let (program, args) = match shell {
+            Shell::System => {
+                if is_local {
+                    (system_shell(), Vec::new())
+                } else {
+                    (DEFAULT_REMOTE_SHELL.to_string(), Vec::new())
+                }
+            }
+            Shell::Program(shell) => (shell.clone(), Vec::new()),
+            Shell::WithArguments { program, args, .. } => (program.clone(), args.clone()),
+        };
+        let kind = ShellKind::new(&program);
+        Self {
+            program,
+            args,
+            interactive: true,
+            kind,
+        }
+    }
+    pub fn non_interactive(mut self) -> Self {
+        self.interactive = false;
+        self
+    }
+    /// Returns the label to show in the terminal tab
+    pub fn command_label(&self, command_label: &str) -> String {
+        match self.kind {
+            ShellKind::Powershell => {
+                format!("{} -C '{}'", self.program, command_label)
+            }
+            ShellKind::Cmd => {
+                format!("{} /C '{}'", self.program, command_label)
+            }
+            ShellKind::Posix => {
+                let interactivity = self.interactive.then_some("-i ").unwrap_or_default();
+                format!("{} {interactivity}-c '{}'", self.program, command_label)
+            }
+        }
+    }
+    /// Returns the program and arguments to run this task in a shell.
+    pub fn build(
+        mut self,
+        task_command: Option<String>,
+        task_args: &Vec<String>,
+    ) -> (String, Vec<String>) {
+        if let Some(task_command) = task_command {
+            let combined_command = task_args
+                .into_iter()
+                .fold(task_command, |mut command, arg| {
+                    command.push(' ');
+                    command.push_str(&self.kind.to_shell_variable(arg));
+                    command
+                });
+
+            self.args
+                .extend(self.kind.args_for_shell(self.interactive, combined_command));
+        }
+
+        (self.program, self.args)
+    }
+}

crates/task/src/task_template.rs 🔗

@@ -253,7 +253,7 @@ impl TaskTemplate {
                         command_label
                     },
                 ),
-                command,
+                command: Some(command),
                 args: self.args.clone(),
                 env,
                 use_new_terminal: self.use_new_terminal,
@@ -633,7 +633,7 @@ mod tests {
                 "Human-readable label should have long substitutions trimmed"
             );
             assert_eq!(
-                spawn_in_terminal.command,
+                spawn_in_terminal.command.clone().unwrap(),
                 format!("echo test_file {long_value}"),
                 "Command should be substituted with variables and those should not be shortened"
             );
@@ -650,7 +650,7 @@ mod tests {
                 spawn_in_terminal.command_label,
                 format!(
                     "{} arg1 test_selected_text arg2 5678 arg3 {long_value}",
-                    spawn_in_terminal.command
+                    spawn_in_terminal.command.clone().unwrap()
                 ),
                 "Command label args should be substituted with variables and those should not be shortened"
             );
@@ -709,7 +709,7 @@ mod tests {
         assert_substituted_variables(&resolved_task, Vec::new());
         let resolved = resolved_task.resolved;
         assert_eq!(resolved.label, task.label);
-        assert_eq!(resolved.command, task.command);
+        assert_eq!(resolved.command, Some(task.command));
         assert_eq!(resolved.args, task.args);
     }
 

crates/vim/src/command.rs 🔗

@@ -1669,7 +1669,7 @@ impl ShellExec {
                     id: TaskId("vim".to_string()),
                     full_label: command.clone(),
                     label: command.clone(),
-                    command: command.clone(),
+                    command: Some(command.clone()),
                     args: Vec::new(),
                     command_label: command.clone(),
                     cwd,