acp: Use npm --prefix for registry npx agents (#53560)

Ben Brandt created 12 hours ago

Because we weren't going through the normal npm subcommand route, we
weren't getting a prefix flag applied. Which meant some users were
seeing errors of incorrect package managers when used with a JS project.

We still want the agent to run in the project dir if we can, we just
don't want it resolving packages from that project. (This actually means
I can run it in our claude-agent-acp repo again :D )

I refactored the node_runtime functions a bit to make these subcommand
functions a bit more inline wiht each other and actually fixed an issue
where --prefix might have been added after `--` previously which
wouldn't be correct anyway.

Self-Review Checklist:

- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable

Release Notes:

- acp: Fix npm-based agents resolving based on current project.

Change summary

crates/node_runtime/src/node_runtime.rs  | 107 ++++++++++++++++++++-----
crates/project/src/agent_server_store.rs |  21 ++++
2 files changed, 101 insertions(+), 27 deletions(-)

Detailed changes

crates/node_runtime/src/node_runtime.rs

@@ -238,11 +238,16 @@ impl NodeRuntime {
             .await
     }
 
-    pub async fn npm_command(&self, subcommand: &str, args: &[&str]) -> Result<NpmCommand> {
+    pub async fn npm_command(
+        &self,
+        prefix_dir: Option<&Path>,
+        subcommand: &str,
+        args: &[&str],
+    ) -> Result<NpmCommand> {
         let http = self.0.lock().await.http.clone();
         self.instance()
             .await
-            .npm_command(http.proxy(), subcommand, args)
+            .npm_command(prefix_dir, http.proxy(), subcommand, args)
             .await
     }
 
@@ -372,6 +377,7 @@ trait NodeRuntimeTrait: Send + Sync {
 
     async fn npm_command(
         &self,
+        prefix_dir: Option<&Path>,
         proxy: Option<&Url>,
         subcommand: &str,
         args: &[&str],
@@ -554,11 +560,13 @@ impl NodeRuntimeTrait for ManagedNodeRuntime {
         args: &[&str],
     ) -> Result<Output> {
         let attempt = || async {
-            let npm_command = self.npm_command(proxy, subcommand, args).await?;
+            let npm_command = self.npm_command(directory, proxy, subcommand, args).await?;
             let mut command = util::command::new_command(npm_command.path);
             command.args(npm_command.args);
             command.envs(npm_command.env);
-            configure_npm_command(&mut command, directory);
+            if let Some(directory) = directory {
+                command.current_dir(directory);
+            }
             command.output().await.map_err(|e| anyhow!("{e}"))
         };
 
@@ -586,6 +594,7 @@ impl NodeRuntimeTrait for ManagedNodeRuntime {
 
     async fn npm_command(
         &self,
+        prefix_dir: Option<&Path>,
         proxy: Option<&Url>,
         subcommand: &str,
         args: &[&str],
@@ -604,6 +613,7 @@ impl NodeRuntimeTrait for ManagedNodeRuntime {
 
         let command_args = build_npm_command_args(
             Some(&npm_file),
+            prefix_dir,
             &self.installation_path.join("cache"),
             Some(&self.installation_path.join("blank_user_npmrc")),
             Some(&self.installation_path.join("blank_global_npmrc")),
@@ -633,7 +643,6 @@ impl NodeRuntimeTrait for ManagedNodeRuntime {
 pub struct SystemNodeRuntime {
     node: PathBuf,
     npm: PathBuf,
-    global_node_modules: PathBuf,
     scratch_dir: PathBuf,
 }
 
@@ -667,17 +676,11 @@ impl SystemNodeRuntime {
         fs::create_dir(&scratch_dir).await.ok();
         fs::create_dir(scratch_dir.join("cache")).await.ok();
 
-        let mut this = Self {
+        Ok(Self {
             node,
             npm,
-            global_node_modules: PathBuf::default(),
             scratch_dir,
-        };
-        let output = this.run_npm_subcommand(None, None, "root", &["-g"]).await?;
-        this.global_node_modules =
-            PathBuf::from(String::from_utf8_lossy(&output.stdout).to_string());
-
-        Ok(this)
+        })
     }
 
     async fn detect() -> std::result::Result<Self, DetectError> {
@@ -722,11 +725,13 @@ impl NodeRuntimeTrait for SystemNodeRuntime {
         subcommand: &str,
         args: &[&str],
     ) -> anyhow::Result<Output> {
-        let npm_command = self.npm_command(proxy, subcommand, args).await?;
+        let npm_command = self.npm_command(directory, proxy, subcommand, args).await?;
         let mut command = util::command::new_command(npm_command.path);
         command.args(npm_command.args);
         command.envs(npm_command.env);
-        configure_npm_command(&mut command, directory);
+        if let Some(directory) = directory {
+            command.current_dir(directory);
+        }
         let output = command.output().await?;
         anyhow::ensure!(
             output.status.success(),
@@ -739,12 +744,14 @@ impl NodeRuntimeTrait for SystemNodeRuntime {
 
     async fn npm_command(
         &self,
+        prefix_dir: Option<&Path>,
         proxy: Option<&Url>,
         subcommand: &str,
         args: &[&str],
     ) -> Result<NpmCommand> {
         let command_args = build_npm_command_args(
             None,
+            prefix_dir,
             &self.scratch_dir.join("cache"),
             None,
             None,
@@ -825,6 +832,7 @@ impl NodeRuntimeTrait for UnavailableNodeRuntime {
 
     async fn npm_command(
         &self,
+        _: Option<&Path>,
         _proxy: Option<&Url>,
         _subcommand: &str,
         _args: &[&str],
@@ -841,13 +849,6 @@ impl NodeRuntimeTrait for UnavailableNodeRuntime {
     }
 }
 
-fn configure_npm_command(command: &mut util::command::Command, directory: Option<&Path>) {
-    if let Some(directory) = directory {
-        command.current_dir(directory);
-        command.args(["--prefix".into(), directory.to_path_buf()]);
-    }
-}
-
 fn proxy_argument(proxy: Option<&Url>) -> Option<String> {
     let mut proxy = proxy.cloned()?;
     // Map proxy settings from `http://localhost:10809` to `http://127.0.0.1:10809`
@@ -865,6 +866,7 @@ fn proxy_argument(proxy: Option<&Url>) -> Option<String> {
 
 fn build_npm_command_args(
     entrypoint: Option<&Path>,
+    prefix_dir: Option<&Path>,
     cache_dir: &Path,
     user_config: Option<&Path>,
     global_config: Option<&Path>,
@@ -876,6 +878,10 @@ fn build_npm_command_args(
     if let Some(entrypoint) = entrypoint {
         command_args.push(entrypoint.to_string_lossy().into_owned());
     }
+    if let Some(prefix_dir) = prefix_dir {
+        command_args.push("--prefix".into());
+        command_args.push(prefix_dir.to_string_lossy().into_owned());
+    }
     command_args.push(subcommand.to_string());
     command_args.push(format!("--cache={}", cache_dir.display()));
     if let Some(user_config) = user_config {
@@ -928,9 +934,11 @@ fn npm_command_env(node_binary: Option<&Path>) -> HashMap<String, String> {
 
 #[cfg(test)]
 mod tests {
+    use std::path::Path;
+
     use http_client::Url;
 
-    use super::proxy_argument;
+    use super::{build_npm_command_args, proxy_argument};
 
     // Map localhost to 127.0.0.1
     // NodeRuntime without environment information can not parse `localhost` correctly.
@@ -960,4 +968,57 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn test_build_npm_command_args_inserts_prefix_before_subcommand() {
+        let args = build_npm_command_args(
+            None,
+            Some(Path::new("/tmp/zed-prefix")),
+            Path::new("/tmp/cache"),
+            None,
+            None,
+            None,
+            "exec",
+            &["--yes", "--", "agent-package"],
+        );
+
+        assert_eq!(
+            args,
+            vec![
+                "--prefix".to_string(),
+                "/tmp/zed-prefix".to_string(),
+                "exec".to_string(),
+                "--cache=/tmp/cache".to_string(),
+                "--yes".to_string(),
+                "--".to_string(),
+                "agent-package".to_string(),
+            ]
+        );
+    }
+
+    #[test]
+    fn test_build_npm_command_args_keeps_entrypoint_before_prefix() {
+        let args = build_npm_command_args(
+            Some(Path::new("/tmp/npm-cli.js")),
+            Some(Path::new("/tmp/zed-prefix")),
+            Path::new("/tmp/cache"),
+            None,
+            None,
+            None,
+            "exec",
+            &["--yes"],
+        );
+
+        assert_eq!(
+            args,
+            vec![
+                "/tmp/npm-cli.js".to_string(),
+                "--prefix".to_string(),
+                "/tmp/zed-prefix".to_string(),
+                "exec".to_string(),
+                "--cache=/tmp/cache".to_string(),
+                "--yes".to_string(),
+            ]
+        );
+    }
 }

crates/project/src/agent_server_store.rs

@@ -568,8 +568,10 @@ impl AgentServerStore {
                                 agent_name.clone(),
                                 ExternalAgentEntry::new(
                                     Box::new(LocalRegistryNpxAgent {
+                                        fs: fs.clone(),
                                         node_runtime: node_runtime.clone(),
                                         project_environment: project_environment.clone(),
+                                        registry_id: Arc::from(name.as_str()),
                                         version: agent.metadata.version.clone(),
                                         package: agent.package.clone(),
                                         args: agent.args.clone(),
@@ -1084,8 +1086,8 @@ fn github_release_archive_from_url(archive_url: &str) -> Option<GithubReleaseArc
     })
 }
 
-fn sanitized_version_component(version: &str) -> String {
-    let sanitized = version
+fn sanitize_path_component(input: &str) -> String {
+    let sanitized = input
         .chars()
         .map(|character| match character {
             'a'..='z' | 'A'..='Z' | '0'..='9' | '.' | '_' | '-' => character,
@@ -1106,7 +1108,7 @@ fn versioned_archive_cache_dir(
     archive_url: &str,
 ) -> PathBuf {
     let version = version.unwrap_or_default();
-    let sanitized_version = sanitized_version_component(version);
+    let sanitized_version = sanitize_path_component(version);
 
     let mut version_hasher = Sha256::new();
     version_hasher.update(version.as_bytes());
@@ -1368,7 +1370,7 @@ impl ExternalAgentServer for LocalRegistryArchiveAgent {
 
             let dir = paths::external_agents_dir()
                 .join("registry")
-                .join(registry_id.as_ref());
+                .join(sanitize_path_component(&registry_id));
             fs.create_dir(&dir).await?;
 
             let os = if cfg!(target_os = "macos") {
@@ -1498,8 +1500,10 @@ impl ExternalAgentServer for LocalRegistryArchiveAgent {
 }
 
 struct LocalRegistryNpxAgent {
+    fs: Arc<dyn Fs>,
     node_runtime: NodeRuntime,
     project_environment: Entity<ProjectEnvironment>,
+    registry_id: Arc<str>,
     version: SharedString,
     package: SharedString,
     args: Vec<String>,
@@ -1527,8 +1531,10 @@ impl ExternalAgentServer for LocalRegistryNpxAgent {
         extra_env: HashMap<String, String>,
         cx: &mut AsyncApp,
     ) -> Task<Result<AgentServerCommand>> {
+        let fs = self.fs.clone();
         let node_runtime = self.node_runtime.clone();
         let project_environment = self.project_environment.downgrade();
+        let registry_id = self.registry_id.clone();
         let package = self.package.clone();
         let args = self.args.clone();
         let distribution_env = self.distribution_env.clone();
@@ -1542,11 +1548,18 @@ impl ExternalAgentServer for LocalRegistryNpxAgent {
                 .await
                 .unwrap_or_default();
 
+            let prefix_dir = paths::external_agents_dir()
+                .join("registry")
+                .join("npx")
+                .join(sanitize_path_component(&registry_id));
+            fs.create_dir(&prefix_dir).await?;
+
             let mut exec_args = vec!["--yes".to_string(), "--".to_string(), package.to_string()];
             exec_args.extend(args);
 
             let npm_command = node_runtime
                 .npm_command(
+                    Some(&prefix_dir),
                     "exec",
                     &exec_args.iter().map(|a| a.as_str()).collect::<Vec<_>>(),
                 )