diff --git a/src-tauri/src/acp/preflight.rs b/src-tauri/src/acp/preflight.rs index e0a57969e5..c1d75e0f8e 100644 --- a/src-tauri/src/acp/preflight.rs +++ b/src-tauri/src/acp/preflight.rs @@ -123,12 +123,13 @@ pub async fn run_preflight(agent_type: AgentType) -> PreflightResult { /// diagnostics report, which bounds every command with `DIAG_PROBE_TIMEOUT`. /// /// The two lookups run concurrently because either can fall through to -/// `resolve_npx_command`'s `npm prefix -g` probe (only when `which` misses), and -/// a FAILED prefix resolution is never cached — `cached_npm_global_prefix_with` -/// short-circuits on `None` before it reaches `OnceCell::set`. On a machine -/// where that probe stalls, running these in sequence would pay -/// `NPM_PREFIX_TIMEOUT` twice per adapter agent; overlapping them costs the same -/// two spawns but bounds the added Settings latency to one timeout. +/// `resolve_npx_command`'s `npm prefix -g` probe (only when `which` and +/// `~/.local/bin` both miss), and a FAILED prefix resolution is never cached — +/// `cached_npm_global_prefix_with` short-circuits on `None` before it reaches +/// `OnceCell::set`. On a machine where that probe stalls, running these in +/// sequence would pay `NPM_PREFIX_TIMEOUT` twice per adapter agent; overlapping +/// them costs the same two spawns but bounds the added Settings latency to one +/// timeout. async fn probe_adapter(meta: &AcpAgentMeta) -> Option { let relation = registry::acp_adapter_relation(meta.agent_type)?; let adapter_cmd = match &meta.distribution { diff --git a/src-tauri/src/acp/registry.rs b/src-tauri/src/acp/registry.rs index 091279f4da..1518e6eb24 100644 --- a/src-tauri/src/acp/registry.rs +++ b/src-tauri/src/acp/registry.rs @@ -2977,10 +2977,13 @@ pub fn get_agent_meta(agent_type: AgentType) -> AcpAgentMeta { // interrupted `hermes update` in the standalone `hermes-acp` entry, // a no-op under the `hermes acp` codeg launches. // - // Launch preference: `resolve_npx_command("hermes")` checks PATH - // first, so an official-installer `hermes` (which self-updates) - // naturally outranks the npm-managed copy; the npm global install - // is the managed/one-click channel codeg's Install button drives. + // Launch preference: `resolve_npx_command("hermes")` checks PATH, + // then `~/.local/bin` (the official installer's target), then the + // npm global prefix. Whatever is on the app's PATH wins first; past + // that, an official-installer `hermes` (which self-updates) + // outranks the npm-managed copy even when the app's PATH lacks + // `~/.local/bin`. The npm global install is the managed/one-click + // channel codeg's Install button drives. distribution: AgentDistribution::Npx { version: "0.21.5", package: "hermes-agent@0.21.5", @@ -4234,8 +4237,9 @@ mod tests { // `acp` subcommand — the package's OTHER bins (`hermes-agent`, // `hermes-npm`) map to different console scripts (`run_agent:main` and // the bridge maintenance CLI), not the ACP adapter. `resolve_npx_command` - // checks PATH before the npm prefix, so an official-installer `hermes` - // keeps outranking the npm-managed copy without any policy bit. + // checks PATH and `~/.local/bin` before the npm prefix, so an + // official-installer `hermes` keeps outranking the npm-managed copy + // without any policy bit. #[test] fn hermes_launches_the_hermes_bin_with_acp_subcommand() { let meta = get_agent_meta(AgentType::Hermes); diff --git a/src-tauri/src/commands/acp.rs b/src-tauri/src/commands/acp.rs index bb6fc62bfa..889d207907 100644 --- a/src-tauri/src/commands/acp.rs +++ b/src-tauri/src/commands/acp.rs @@ -195,9 +195,10 @@ pub(crate) fn apply_custom_version_to_url( url.replace(registry_version, custom_version) } -/// Check whether an NPX agent command is spawnable. -/// Uses PATH first, then falls back to the current npm global prefix to handle -/// GUI environments that don't inherit the user's shell PATH. +/// Check whether an NPX agent command is spawnable, by the same lookup as +/// [`resolve_npx_command`]: PATH, then `~/.local/bin`, then the current npm +/// global prefix. The fallbacks handle GUI environments that don't inherit the +/// user's shell PATH. pub(crate) async fn is_cmd_available(cmd: &str) -> bool { resolve_npx_command(cmd).await.is_some() } @@ -212,7 +213,9 @@ pub(crate) fn resolve_command_on_path(cmd: &str) -> Option { /// cached — the managed cache always wins, the system CLI only fills the /// gap, mirroring how npx agents already prefer a PATH install at launch. /// Checks PATH first, then `~/.local/bin` — a common install-script target -/// a macOS GUI app's PATH typically lacks. +/// a macOS GUI app's PATH typically lacks. [`resolve_npx_command`] runs these +/// same two steps before the npm global prefix, so an npx agent's official +/// installer copy (Hermes) is found the same way. pub(crate) fn resolve_system_agent_binary(cmd: &str) -> Option { if let Some(path) = resolve_command_on_path(cmd) { return Some(path); @@ -256,16 +259,13 @@ pub(crate) fn resolve_system_agent_binary_for(agent_type: AgentType, cmd: &str) /// it doesn't speak ACP" instead of a bare "not installed". /// /// Widest resolution of the three probes, because a false negative here loses -/// the most convincing evidence: PATH + the npm global prefix (the GUI-PATH-gap -/// fallback `resolve_npx_command` already implements), then `~/.local/bin` via -/// [`resolve_system_agent_binary`], then the vendor installers' own dirs. +/// the most convincing evidence: everything [`resolve_npx_command`] checks +/// (PATH, `~/.local/bin`, the npm global prefix), then the vendor installers' +/// own dirs. pub(crate) async fn resolve_vendor_cli(cmd: &str, extra_dirs: &[&str]) -> Option { if let Some(path) = resolve_npx_command(cmd).await { return Some(path); } - if let Some(path) = resolve_system_agent_binary(cmd) { - return Some(path); - } let exe = if cfg!(windows) { format!("{cmd}.exe") } else { @@ -405,8 +405,19 @@ async fn prewarm_uvx_agent( Ok(()) } +/// Resolve an npx agent's command the way connect, launch, status and +/// diagnostics all see it: PATH, then `~/.local/bin` (both via +/// [`resolve_system_agent_binary`]), then the current npm global prefix. +/// +/// Both fallbacks cover an app PATH that lacks the user's shell PATH. +/// `~/.local/bin` goes first on purpose: when neither copy is on the app's +/// PATH, a vendor's official installer copy (which targets `~/.local/bin`) +/// beats an npm copy, as it typically does in a terminal where the installer +/// put `~/.local/bin` first on PATH. See the Hermes "Launch preference" note +/// in `acp/registry.rs`. [`NpxCommandResolver::resolve_for_list`] must keep +/// the same order. pub(crate) async fn resolve_npx_command(cmd: &str) -> Option { - if let Some(path) = resolve_command_on_path(cmd) { + if let Some(path) = resolve_system_agent_binary(cmd) { return Some(path); } resolve_npx_command_from_current_npm_prefix(cmd).await @@ -424,7 +435,7 @@ impl NpxCommandResolver { return cached.clone(); } - let resolved = if let Some(path) = resolve_command_on_path(cmd) { + let resolved = if let Some(path) = resolve_system_agent_binary(cmd) { Some(path) } else { let prefix = if let Some(prefix) = &self.request_npm_prefix { @@ -1236,6 +1247,16 @@ fn compute_verdict(inp: &DiagInputs) -> DiagnosticsVerdict { } if agent.resolve_npx.is_none() { + if agent.db_version.is_none() + && agent.detected_version.is_none() + && inp.terminal.cmd_resolved.is_some() + { + return diag_verdict( + DiagLevel::Fail, + "terminal_only_path", + "The command resolves in your terminal but not in the app process — a GUI PATH gap.", + ); + } // An ACP adapter agent that was never installed is the single most // reported "bug": the user has the vendor CLI and reads "not installed" // as codeg failing to see it. Answer the question they're actually @@ -1694,6 +1715,51 @@ mod diagnostics_tests { assert_eq!(compute_verdict(&inp).code, "terminal_only_path"); } + #[test] + fn verdict_terminal_only_path_without_npm_evidence() { + let mut inp = base_inputs(); + inp.agent = Some(AgentDiag { + name: "Hermes Agent".to_string(), + cmd: "hermes".to_string(), + distribution: "npx", + ..Default::default() + }); + inp.terminal.cmd_resolved = Some("/Users/u/.local/bin/hermes".to_string()); + let v = compute_verdict(&inp); + assert_eq!(v.code, "terminal_only_path"); + assert_eq!(v.level, DiagLevel::Fail); + } + + // That no-record check sits after the Node checks, so Node problems are + // still reported first, as for every npx agent... + #[test] + fn verdict_node_missing_outranks_terminal_only_path() { + let mut inp = DiagInputs { + agent: Some(AgentDiag { + name: "Hermes Agent".to_string(), + cmd: "hermes".to_string(), + distribution: "npx", + ..Default::default() + }), + ..Default::default() + }; + inp.terminal.cmd_resolved = Some("/Users/u/.local/bin/hermes".to_string()); + assert_eq!(compute_verdict(&inp).code, "node_missing"); + } + + // ...and before the adapter explainer: when the adapter's OWN command + // resolves in the login shell, the adapter is installed, so "the adapter + // is not installed" would be wrong. + #[test] + fn verdict_terminal_only_path_outranks_adapter_explainer() { + let mut inp = base_inputs(); + inp.agent = Some(adapter_agent_never_installed(Some( + "/opt/homebrew/bin/codex", + ))); + inp.terminal.cmd_resolved = Some("/Users/u/bin/codex-acp".to_string()); + assert_eq!(compute_verdict(&inp).code, "terminal_only_path"); + } + #[test] fn verdict_node_too_old() { let mut inp = base_inputs(); @@ -2053,7 +2119,8 @@ async fn system_probed_version_with( let mut version = None; if let Some(probe) = declared_probe { // The declared probe is a full command line (`agent-cli --version`); - // its program resolves like an agent command (PATH, then npm prefix). + // its program resolves like an agent command (PATH, then + // `~/.local/bin`, then npm prefix). let mut parts = probe.split_whitespace(); if let Some(program) = parts.next() { let args: Vec = parts.map(str::to_string).collect(); @@ -7983,14 +8050,14 @@ fn shell_join(argv: &[String]) -> String { /// The argv for Hermes's `--setup` and `model` flows: a `hermes` CLI on PATH /// by bare name (official installer or an npm global bin on PATH — the short -/// form reads best in guidance), else the npm-prefix-resolved absolute path -/// (an npm install whose bin dir is NOT on the terminal's PATH must be -/// addressed absolutely or the displayed command fails), else a documented -/// `npx -y --package hermes …` form that bootstraps on demand. -/// `--package` is required in that form: the package's default bin is -/// `hermes-agent` (a different console script), not `hermes`. Returned as -/// argv vectors so callers can shell-quote per platform for display or -/// execute them. +/// form reads best in guidance), else the absolute path `resolve_npx_command` +/// finds in `~/.local/bin` or the npm prefix (a copy whose dir is NOT on the +/// terminal's PATH must be addressed absolutely or the displayed command +/// fails), else a documented `npx -y --package hermes …` form that +/// bootstraps on demand. `--package` is required in that form: the package's +/// default bin is `hermes-agent` (a different console script), not `hermes`. +/// Returned as argv vectors so callers can shell-quote per platform for +/// display or execute them. async fn hermes_setup_argvs() -> (Vec, Vec) { let meta = registry::get_agent_meta(AgentType::Hermes); let package = match meta.distribution { @@ -13273,10 +13340,10 @@ pub(crate) async fn acp_prepare_npx_agent_core( // postinstall leaves a shim that exits "runtime is not ready". // Verify the binary a launch would resolve actually answers // `--version` BEFORE recording success — the same resolution - // order connect uses, so an official-installer CLI on PATH - // legitimately satisfies the check. Without this, a broken - // install is recorded as installed and only fails at connect - // time with an opaque error. + // order connect uses, so an official-installer CLI (on PATH or in + // `~/.local/bin`) legitimately satisfies the check. Without this, + // a broken install is recorded as installed and only fails at + // connect time with an opaque error. if npm_package_requires_scripts(&install_spec) { emit_agent_install_event( emitter, @@ -18099,6 +18166,53 @@ wire_api = "chat" let _ = std::fs::remove_dir_all(prefix); } + #[cfg(not(windows))] + #[tokio::test] + async fn npx_command_resolution_checks_local_bin_before_npm_prefix() { + use std::os::unix::fs::PermissionsExt; + + let home = tempfile::tempdir().expect("tempdir"); + let bin_dir = home.path().join(".local").join("bin"); + let prefix = home.path().join("npm-prefix"); + let prefix_bin_dir = npm_prefix_bin_dir(&prefix); + let cmd = "codeg-test-local-bin-npx-agent"; + let command_path = bin_dir.join(cmd); + // Only in the npm prefix: the list path must still fall through to it. + let prefix_only_cmd = "codeg-test-npm-prefix-only-npx-agent"; + let prefix_only_path = prefix_bin_dir.join(prefix_only_cmd); + for path in [ + bin_dir.join(cmd), + prefix_bin_dir.join(cmd), + prefix_only_path.clone(), + ] { + std::fs::create_dir_all(path.parent().expect("bin directory")) + .expect("create bin directory"); + std::fs::write(&path, "").expect("write command"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .expect("mark command executable"); + } + + let (direct, listed, listed_prefix_only) = + temp_env::async_with_vars([("HOME", Some(home.path()))], async { + let direct = resolve_npx_command(cmd).await; + let mut resolver = NpxCommandResolver { + request_npm_prefix: Some(Some(prefix.clone())), + ..Default::default() + }; + let listed = resolver.resolve_for_list(cmd).await; + let listed_prefix_only = resolver.resolve_for_list(prefix_only_cmd).await; + (direct, listed, listed_prefix_only) + }) + .await; + + assert_eq!(direct.as_deref(), Some(command_path.as_path())); + assert_eq!(listed.as_deref(), Some(command_path.as_path())); + assert_eq!( + listed_prefix_only.as_deref(), + Some(prefix_only_path.as_path()) + ); + } + fn write_skill_md(name: &str, body: &str) -> (PathBuf, PathBuf) { let dir = unique_test_dir(name); let path = dir.join("SKILL.md");