From 07e9a2d25aa296c2513c9d59cb3ff675efb58c5e Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Tue, 14 Apr 2026 13:10:31 +0200 Subject: [PATCH] acp: Use npm --prefix for registry npx agents (#53560) 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. --- crates/node_runtime/src/node_runtime.rs | 107 ++++++++++++++++++----- crates/project/src/agent_server_store.rs | 21 ++++- 2 files changed, 101 insertions(+), 27 deletions(-) diff --git a/crates/node_runtime/src/node_runtime.rs b/crates/node_runtime/src/node_runtime.rs index 8d838cbe7505a696fbfbeebfa53e96dbd3b19e29..9d4bfe9cffb538e576b521f6cdb9f7ca7480a8c8 100644 --- a/crates/node_runtime/src/node_runtime.rs +++ b/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 { + pub async fn npm_command( + &self, + prefix_dir: Option<&Path>, + subcommand: &str, + args: &[&str], + ) -> Result { 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 { 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 { @@ -722,11 +725,13 @@ impl NodeRuntimeTrait for SystemNodeRuntime { subcommand: &str, args: &[&str], ) -> anyhow::Result { - 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 { 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 { 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 { 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 { #[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(), + ] + ); + } } diff --git a/crates/project/src/agent_server_store.rs b/crates/project/src/agent_server_store.rs index 5a9721d827cf3d189c7954f0698b662e5aaf4852..103a44197aefd0b73ab58c03792f35f94bf2b1c1 100644 --- a/crates/project/src/agent_server_store.rs +++ b/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 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(®istry_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, node_runtime: NodeRuntime, project_environment: Entity, + registry_id: Arc, version: SharedString, package: SharedString, args: Vec, @@ -1527,8 +1531,10 @@ impl ExternalAgentServer for LocalRegistryNpxAgent { extra_env: HashMap, cx: &mut AsyncApp, ) -> Task> { + 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(®istry_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::>(), )