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.
This commit is contained in:
1 parent
28d87884b9
commit
e0a3312016
2 files changed
+101
-27
No files matched your search
@@ -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(),
|
||||
]
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user