feat(doctor): add Pi agent detection, install fixes, and update flows

crates/doctor/src/agents.rs

@@ -7,7 +7,7 @@ use crate::command::{
     run_command_with_timeout, CommandError, CommandTimeout, DEFAULT_PROBE_TIMEOUT,
 };
 use crate::environment::{apply_doctor_env, DoctorEnv};
-use crate::resolve::format_command_output;
+use crate::resolve::{format_command_output, shell_quote};
 use crate::timeout_check::{command_timeout_check, TimeoutCheck};
 use crate::types::{
     AgentVersionInfo, AuthStatus, CheckStatus, DoctorCheck, FixType, InstallSource, ResolvedBinary,
@@ -106,15 +106,21 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
         // codex's clap short flag reaches the vendored binary.
         bundled_version_args: Some(&["cli", "-V"]),
     },
+    // Pi (pi.dev, github.com/earendil-works/pi) is npm-under-the-hood in every
+    // install method: `npm install -g --ignore-scripts` (its docs and its
+    // pi.dev/install.sh curl installer both use --ignore-scripts), pnpm, or
+    // bun. The ACP bridge is the separately-maintained `pi-acp` npm package
+    // (the same one Zed's ACP registry spawns). No auth commands: Pi owns its
+    // provider/model configuration (`pi` manages API keys itself).
     AgentCheckInfo {
         id: "ai-agent-pi",
         label: "Pi",
         commands: &["pi-acp"],
         main_command: Some("pi"),
-        install_url: None,
-        install_command: None,
-        bridge_install_url: None,
-        bridge_install_command: None,
+        install_url: Some("https://pi.dev"),
+        install_command: Some("npm install -g --ignore-scripts @earendil-works/pi-coding-agent"),
+        bridge_install_url: Some("https://www.npmjs.com/package/pi-acp"),
+        bridge_install_command: Some("npm install -g pi-acp"),
         auth_command: None,
         auth_status_command: None,
         install_source_override: None,
@@ -192,17 +198,38 @@ pub(crate) fn bundled_version_probe_args(
 /// sources with no canonical update recipe (`Mise`/`Asdf`/`Unknown`/`System`),
 /// or when the package id is unknown.
 ///
+/// `npm_prefix` is the prefix that owns the binary being updated (see
+/// [`crate::resolve::npm_prefix_for_binary`]). npm installs go to whatever
+/// prefix npm is *configured* with, which is not always the one the resolved
+/// binary lives in — a package installed with an explicit `--prefix`, or under a
+/// node version the user has since switched away from, would otherwise be
+/// "updated" by installing a second copy somewhere else while the stale binary
+/// keeps resolving. Passing the derived prefix back through pins the update to
+/// the install it is about; where the two agree — the common case — the flag is
+/// a no-op. `None` falls back to the bare command.
+///
 /// The caller is responsible for gating on `update_available == Some(true)` —
 /// this function only knows how to update, not whether to. `apply_npm_registry`
 /// runs over the final command string downstream, so npm commands automatically
 /// pick up a registry override when one is configured.
 pub fn derive_update_command(
     install_source: Option<&InstallSource>,
     package_id: Option<&str>,
+    npm_prefix: Option<&Path>,
 ) -> Option<String> {
     let pkg = package_id?;
     match install_source? {
-        InstallSource::Npm => Some(format!("npm install -g {pkg}@latest")),
+        InstallSource::Npm => Some(match npm_prefix {
+            // Quoted: this is executed via `sh -c`, and `/Users/Mary Smith` is a
+            // real home directory.
+            Some(prefix) => format!(
+                "npm install -g --prefix {} {pkg}@latest",
+                shell_quote(&prefix.to_string_lossy()),
+            ),
+            None => format!("npm install -g {pkg}@latest"),
+        }),
+        InstallSource::Pnpm => Some(format!("pnpm add -g {pkg}@latest")),
+        InstallSource::Bun => Some(format!("bun add -g {pkg}@latest")),
         InstallSource::Brew => Some(format!("brew upgrade {pkg}")),
         InstallSource::Cargo => Some(format!("cargo install --force {pkg}")),
         InstallSource::CurlPipe
@@ -737,10 +764,6 @@ mod tests {
         }
     }
 
-    fn shell_quote(value: &str) -> String {
-        format!("'{}'", value.replace('\'', "'\\''"))
-    }
-
     fn write_login_path_rewrite_profiles(home: &Path, path: &Path) {
         let profile = format!(
             "export PATH={}\n",
@@ -1058,6 +1081,101 @@ mod tests {
         }
     }
 
+    /// Pi's registry entry: npm-shaped install commands for both binaries (so
+    /// the registry override applies) and no auth commands (Pi owns its own
+    /// provider/model configuration).
+    #[test]
+    fn pi_declares_install_and_bridge_commands() {
+        let pi = agent("ai-agent-pi");
+        assert_eq!(pi.main_command, Some("pi"));
+        assert_eq!(pi.commands, &["pi-acp"]);
+        assert_eq!(pi.install_url, Some("https://pi.dev"));
+        assert_eq!(
+            pi.install_command,
+            Some("npm install -g --ignore-scripts @earendil-works/pi-coding-agent"),
+        );
+        assert_eq!(
+            pi.bridge_install_url,
+            Some("https://www.npmjs.com/package/pi-acp"),
+        );
+        assert_eq!(pi.bridge_install_command, Some("npm install -g pi-acp"));
+        assert_eq!(pi.auth_command, None);
+        assert_eq!(pi.auth_status_command, None);
+        assert_eq!(pi.install_source_override, None);
+    }
+
+    /// Main CLI present, bridge missing → `FixType::Bridge` with the
+    /// registry-routed bridge install command. Berd treats a missing bridge as
+    /// not-installed (ACP sessions spawn `pi-acp`), so without
+    /// `bridge_install_command` this state would look ready but be unusable.
+    #[test]
+    fn pi_main_only_offers_registry_routed_bridge_fix() {
+        let bridge_missing = resolved(None, None);
+        let main = resolved(Some("/opt/homebrew/bin/pi"), Some(InstallSource::Npm));
+        let check = check_single_ai_agent(
+            agent("ai-agent-pi"),
+            true,
+            std::slice::from_ref(&bridge_missing),
+            Some(&main),
+            Some("https://artifactory/npm"),
+            None,
+        );
+
+        assert_eq!(check.status, CheckStatus::Warn);
+        assert_eq!(check.fix_type, Some(FixType::Bridge));
+        assert_eq!(
+            check.fix_command.as_deref(),
+            Some("npm install -g pi-acp --registry=https://artifactory/npm"),
+        );
+        assert_eq!(check.path.as_deref(), Some("/opt/homebrew/bin/pi"));
+        assert!(check.bridge_path.is_none());
+    }
+
+    /// Neither binary installed → `FixType::Command` with the registry-routed
+    /// main-CLI install command and the pi.dev fix URL.
+    #[test]
+    fn pi_not_installed_offers_registry_routed_install_command() {
+        let missing = resolved(None, None);
+        let check = check_single_ai_agent(
+            agent("ai-agent-pi"),
+            true,
+            std::slice::from_ref(&missing),
+            Some(&missing),
+            Some("https://artifactory/npm"),
+            None,
+        );
+
+        assert_eq!(check.status, CheckStatus::Warn);
+        assert_eq!(check.fix_type, Some(FixType::Command));
+        assert_eq!(
+            check.fix_command.as_deref(),
+            Some(
+                "npm install -g --ignore-scripts @earendil-works/pi-coding-agent \
+                 --registry=https://artifactory/npm"
+            ),
+        );
+        assert_eq!(check.fix_url.as_deref(), Some("https://pi.dev"));
+        assert!(check.path.is_none());
+        assert!(check.bridge_path.is_none());
+    }
+
+    #[test]
+    fn derive_update_command_pnpm_and_bun_emit_add_g_latest() {
+        assert_eq!(
+            derive_update_command(
+                Some(&InstallSource::Pnpm),
+                Some("@earendil-works/pi-coding-agent"),
+                None,
+            )
+            .as_deref(),
+            Some("pnpm add -g @earendil-works/pi-coding-agent@latest"),
+        );
+        assert_eq!(
+            derive_update_command(Some(&InstallSource::Bun), Some("pi-acp"), None).as_deref(),
+            Some("bun add -g pi-acp@latest"),
+        );
+    }
+
     #[test]
     fn apply_npm_registry_appends_to_npm_install() {
         assert_eq!(
@@ -1161,30 +1279,90 @@ mod tests {
         );
     }
 
+    /// Without a derived prefix the npm recipe stays bare — today's behaviour,
+    /// and the fail-safe when the package tree can't be located.
     #[test]
-    fn derive_update_command_npm_emits_at_latest() {
+    fn derive_update_command_npm_falls_back_to_bare_without_prefix() {
         assert_eq!(
             derive_update_command(
                 Some(&InstallSource::Npm),
                 Some("@agentclientprotocol/claude-agent-acp"),
+                None,
             )
             .as_deref(),
             Some("npm install -g @agentclientprotocol/claude-agent-acp@latest"),
         );
     }
 
+    /// With a prefix, the recipe pins the install to it — so an agent under
+    /// `~/.local` is upgraded there instead of a second copy appearing in
+    /// whichever prefix npm is configured with.
+    #[test]
+    fn derive_update_command_npm_emits_prefix() {
+        assert_eq!(
+            derive_update_command(
+                Some(&InstallSource::Npm),
+                Some("@earendil-works/pi-coding-agent"),
+                Some(Path::new("/Users/test/.local")),
+            )
+            .as_deref(),
+            Some(
+                "npm install -g --prefix '/Users/test/.local' \
+                 @earendil-works/pi-coding-agent@latest"
+            ),
+        );
+    }
+
+    /// The command is handed to `sh -c`, so a prefix with a space in it (a real
+    /// home directory shape) must survive quoting intact.
+    #[test]
+    fn derive_update_command_npm_quotes_prefix_containing_spaces() {
+        assert_eq!(
+            derive_update_command(
+                Some(&InstallSource::Npm),
+                Some("pi-acp"),
+                Some(Path::new("/Users/Mary Smith/.local")),
+            )
+            .as_deref(),
+            Some("npm install -g --prefix '/Users/Mary Smith/.local' pi-acp@latest"),
+        );
+    }
+
+    /// A prefixed npm recipe is still recognised as npm-backed, so a configured
+    /// registry override lands on it.
+    #[test]
+    fn derive_update_command_npm_with_prefix_still_takes_registry_override() {
+        let command = derive_update_command(
+            Some(&InstallSource::Npm),
+            Some("@earendil-works/pi-coding-agent"),
+            Some(Path::new("/Users/test/.local")),
+        )
+        .expect("npm recipe");
+        assert_eq!(
+            apply_npm_registry(&command, Some("https://artifactory/npm")),
+            "npm install -g --prefix '/Users/test/.local' \
+             @earendil-works/pi-coding-agent@latest --registry=https://artifactory/npm",
+        );
+    }
+
+    /// Non-npm sources ignore the prefix entirely.
     #[test]
     fn derive_update_command_brew_emits_upgrade() {
         assert_eq!(
-            derive_update_command(Some(&InstallSource::Brew), Some("codex")).as_deref(),
+            derive_update_command(
+                Some(&InstallSource::Brew),
+                Some("codex"),
+                Some(Path::new("/Users/test/.local")),
+            )
+            .as_deref(),
             Some("brew upgrade codex"),
         );
     }
 
     #[test]
     fn derive_update_command_cargo_emits_install_force() {
         assert_eq!(
-            derive_update_command(Some(&InstallSource::Cargo), Some("some-crate")).as_deref(),
+            derive_update_command(Some(&InstallSource::Cargo), Some("some-crate"), None).as_deref(),
             Some("cargo install --force some-crate"),
         );
     }
@@ -1200,7 +1378,7 @@ mod tests {
             InstallSource::System,
         ] {
             assert_eq!(
-                derive_update_command(Some(&src), Some("pkg")),
+                derive_update_command(Some(&src), Some("pkg"), None),
                 None,
                 "expected None for {src:?}",
             );
@@ -1209,7 +1387,10 @@ mod tests {
 
     #[test]
     fn derive_update_command_returns_none_without_package_id() {
-        assert_eq!(derive_update_command(Some(&InstallSource::Npm), None), None,);
+        assert_eq!(
+            derive_update_command(Some(&InstallSource::Npm), None, None),
+            None,
+        );
     }
 
     #[test]