From e84c6dfa932ace02f297c80a8e0bf44be40acb1d Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 14:03:12 +1000 Subject: [PATCH 1/2] feat(doctor): add Pi agent detection, install fixes, and update flows Fill in the ai-agent-pi stub with full support for every realistic Pi install method: - agents.rs: install/bridge commands for the ai-agent-pi check (npm install -g --ignore-scripts @earendil-works/pi-coding-agent / npm install -g pi-acp), so a missing bridge reports fixType bridge and berd can route both through the configured registry. New per-agent self_update_command ("pi update --self"): a curl-installed Pi does not auto-update, but ships an updater that infers its own prefix and package manager, so a CurlPipe main readout keeps the update nag and surfaces that command instead of a broken npm recipe. - types.rs/resolve.rs: new Pnpm and Bun install sources. Both link global bins into node_modules trees, so without them the classifier called these installs Npm and nagged with a command that would not update them. Classification reads $PNPM_HOME/$BUN_INSTALL from the probe env with platform defaults. Pi's curl installer footprint (~/.local/lib/node_modules tree or ~/.local/share/pi-node) classifies as CurlPipe ahead of the npm-layout check. - freshness.rs: Pnpm/Bun readouts resolve installed versions from package.json like Npm. pnpm global bins are generated shim scripts (not symlinks), so the canonicalize-and-walk probe now falls back to parsing the shim's embedded entrypoint path, then to scanning the global/ trees (pnpm <= 10 flat and pnpm 11 hashed layouts). - package_ids.rs: Pi main/bridge package ids for Npm, Pnpm, Bun, and CurlPipe (main only) sources. - lib.rs: apply_freshness threads the agent's self-update command into main readouts; derive_update_command emits pnpm add -g / bun add -g recipes for the new sources. Every scenario was live-verified on a real machine through run_checks_with_options and execute_fix_streaming_with_env_options: not-installed, main-only + bridge fix, bridge-only, curl ~/.local layout + pi update --self, npm and pnpm outdated-to-updated flows (0.82.1 -> 0.83.0, pi-acp 0.0.32 -> 0.0.33), with and without a registry override. Bun coverage is unit-test only (not installed here). Co-Authored-By: Claude Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 155 ++++++++++++++- crates/doctor/src/freshness.rs | 285 ++++++++++++++++++++++++++-- crates/doctor/src/lib.rs | 145 ++++++++++++-- crates/doctor/src/package_ids.rs | 107 ++++++++++- crates/doctor/src/resolve.rs | 312 +++++++++++++++++++++++++++++-- crates/doctor/src/types.rs | 10 + 6 files changed, 963 insertions(+), 51 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index 1ed3b11a1..15bbabf21 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -50,6 +50,15 @@ pub struct AgentCheckInfo { /// by the embedding app's lock and the version worth surfacing is the /// vendored harness CLI's (e.g. Claude Code 2.1.x). pub bundled_version_args: Option<&'static [&'static str]>, + /// Shell command that runs the agent's own updater for its main CLI (e.g. + /// `pi update --self`). Used by the freshness pass as the update command + /// for a [`InstallSource::CurlPipe`] main readout, where registry-derived + /// recipes would target the wrong prefix (Pi's installer lays down an + /// npm-shaped tree under `~/.local` that a plain `npm install -g` can't + /// reach) but the tool's own updater re-installs in place. Unlike + /// Cursor/Amp-style background auto-updaters, a declared self-update + /// command marks the install as user-actioned: the update nag stays. + pub self_update_command: Option<&'static str>, } /// All AI agents we check for individually. @@ -67,6 +76,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ auth_status_command: None, install_source_override: None, bundled_version_args: None, + self_update_command: None, }, // The claude-agent-acp bridge vendors the complete Claude Code CLI and // forwards `--cli ` to it, sharing the user's credential store @@ -85,6 +95,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ auth_status_command: Some("claude-agent-acp --cli auth status"), install_source_override: None, bundled_version_args: Some(&["--cli", "--version"]), + self_update_command: None, }, // The codex-acp bridge vendors the full `codex` binary and forwards // `cli ` to it, sharing the user's ~/.codex/auth.json — same @@ -105,20 +116,33 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ // `--version` anywhere in argv and prints its own version, so only // codex's clap short flag reaches the vendored binary. bundled_version_args: Some(&["cli", "-V"]), + self_update_command: None, }, + // 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, bundled_version_args: None, + // Pi's curl installer (pi.dev/install.sh) falls back to an npm-shaped + // tree under `~/.local` when the global npm prefix isn't writable; + // `pi update --self` re-installs in place for every layout (it infers + // its own prefix/package manager), where `npm install -g …@latest` + // would target the wrong prefix. + self_update_command: Some("pi update --self"), }, AgentCheckInfo { id: "ai-agent-amp", @@ -134,6 +158,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ // Main `amp` curl installer; bridge `amp-acp` is npm (detected positively). install_source_override: Some(InstallSource::CurlPipe), bundled_version_args: None, + self_update_command: None, }, AgentCheckInfo { id: "ai-agent-copilot", @@ -148,6 +173,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ auth_status_command: None, install_source_override: None, bundled_version_args: None, + self_update_command: None, }, AgentCheckInfo { id: "ai-agent-cursor", @@ -164,6 +190,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ // resolved binary is the curl install — the override's primary use case. install_source_override: Some(InstallSource::CurlPipe), bundled_version_args: None, + self_update_command: None, }, ]; @@ -203,6 +230,8 @@ pub fn derive_update_command( let pkg = package_id?; match install_source? { InstallSource::Npm => Some(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 @@ -214,6 +243,17 @@ pub fn derive_update_command( } } +/// The agent's declared self-update command for its main CLI (e.g. Pi's +/// `pi update --self`), when one exists. Consulted by the freshness pass for +/// [`InstallSource::CurlPipe`] main readouts — see +/// [`AgentCheckInfo::self_update_command`]. +pub(crate) fn agent_self_update_command(check_id: &str) -> Option<&'static str> { + AI_AGENT_CHECKS + .iter() + .find(|info| info.id == check_id) + .and_then(|info| info.self_update_command) +} + /// Append `--registry=` to `command` when a registry override is supplied /// and the command is npm-backed. Non-npm commands (curl-pipe installers, auth /// commands, the git-clonefile fix, …) and the `None` registry case return the @@ -1058,6 +1098,113 @@ mod tests { } } + /// Pi's registry entry: npm-shaped install commands for both binaries (so + /// the registry override applies), no auth commands (Pi owns its own + /// provider/model configuration), and its own updater for curl installs. + #[test] + fn pi_declares_install_bridge_and_self_update_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); + assert_eq!(pi.self_update_command, Some("pi update --self")); + } + + /// 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 self_update_command_lookup_only_for_declaring_agents() { + assert_eq!( + agent_self_update_command("ai-agent-pi"), + Some("pi update --self"), + ); + // Cursor/Amp curl installs auto-update in the background — they keep + // the self-updating suppression, not a user-actioned updater. + assert_eq!(agent_self_update_command("ai-agent-cursor"), None); + assert_eq!(agent_self_update_command("ai-agent-amp"), 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"), + ) + .as_deref(), + Some("pnpm add -g @earendil-works/pi-coding-agent@latest"), + ); + assert_eq!( + derive_update_command(Some(&InstallSource::Bun), Some("pi-acp")).as_deref(), + Some("bun add -g pi-acp@latest"), + ); + } + #[test] fn apply_npm_registry_appends_to_npm_install() { assert_eq!( diff --git a/crates/doctor/src/freshness.rs b/crates/doctor/src/freshness.rs index 7cea45147..a9cc442ff 100644 --- a/crates/doctor/src/freshness.rs +++ b/crates/doctor/src/freshness.rs @@ -244,13 +244,18 @@ pub(crate) enum InstalledProbe<'a> { } /// Pick the installed-version probe for a readout from its install source. -/// `Npm` installs read `package.json`; everything else uses the CLI -/// `--version` probe. +/// `Npm`/`Pnpm`/`Bun` installs read `package.json` — all three link the bin +/// entry into a `node_modules/` tree the walk can canonicalize into, and +/// npm-distributed bridges don't reliably honor `--version`. Everything else +/// uses the CLI `--version` probe. pub(crate) fn select_installed_probe<'a>( install_source: Option<&InstallSource>, package_id: Option<&'a str>, ) -> InstalledProbe<'a> { - if matches!(install_source, Some(InstallSource::Npm)) { + if matches!( + install_source, + Some(InstallSource::Npm | InstallSource::Pnpm | InstallSource::Bun) + ) { InstalledProbe::NpmPackageJson { package_id } } else { InstalledProbe::Cli(&["--version"]) @@ -282,7 +287,10 @@ impl InstalledProbe<'_> { /// `bin/`), then walk up a bounded number of levels looking for the first /// `package.json`. When the package id is known, the file's `name` must match /// it — otherwise we keep walking, so a dependency's `package.json` nested -/// below the real one is never mistaken for the target. +/// below the real one is never mistaken for the target. When the walk misses +/// entirely, fall back to pnpm's global layout: pnpm bin entries are generated +/// shim scripts rather than symlinks, so canonicalizing never lands inside a +/// `node_modules/` tree even though the package sits right next door. fn installed_version_from_package_json( binary_path: &Path, expected_pkg: Option<&str>, @@ -290,24 +298,100 @@ fn installed_version_from_package_json( let resolved = std::fs::canonicalize(binary_path).ok()?; let mut dir = resolved.parent(); for _ in 0..6 { - let d = dir?; + let Some(d) = dir else { break }; let pj = d.join("package.json"); if pj.is_file() { - if let Ok(bytes) = std::fs::read(&pj) { - if let Ok(v) = serde_json::from_slice::(&bytes) { - let name_ok = expected_pkg - .map(|p| v.get("name").and_then(|n| n.as_str()) == Some(p)) - .unwrap_or(true); - if name_ok { - return v - .get("version") - .and_then(|x| x.as_str()) - .map(str::to_string); + if let Some(version) = read_package_json_version(&pj, expected_pkg) { + return Some(version); + } + } + dir = d.parent(); + } + let expected = expected_pkg?; + pnpm_shim_package_json(&resolved, expected) + .or_else(|| pnpm_global_package_json(resolved.parent()?, expected)) +} + +/// Parse a `package.json` and return its `version`, or `None` when the file is +/// unreadable, malformed, or names a different package than expected. +fn read_package_json_version(pj: &Path, expected_pkg: Option<&str>) -> Option { + let bytes = std::fs::read(pj).ok()?; + let v = serde_json::from_slice::(&bytes).ok()?; + let name_ok = expected_pkg + .map(|p| v.get("name").and_then(|n| n.as_str()) == Some(p)) + .unwrap_or(true); + if !name_ok { + return None; + } + v.get("version") + .and_then(|x| x.as_str()) + .map(str::to_string) +} + +/// Extract the target package's version out of a pnpm shim script. pnpm shims +/// embed the resolved entrypoint path (e.g. +/// `"$basedir/global/5/.pnpm/@_.../node_modules//dist/cli.js"`), +/// which pins exactly the install the shim runs — preferred over scanning the +/// layout, which can hold several `global//` trees after a +/// pnpm major upgrade. +fn pnpm_shim_package_json(shim: &Path, expected_pkg: &str) -> Option { + let text = std::fs::read_to_string(shim).ok()?; + let needle = format!("node_modules/{expected_pkg}/"); + for quoted in text.split('"') { + let Some(idx) = quoted.find(&needle) else { + continue; + }; + // Everything up to (and including) `node_modules/` is the + // package dir, either relative to the shim's own dir or absolute. + let pkg_dir_str = "ed[..idx + needle.len() - 1]; + let pkg_dir = if let Some(rest) = pkg_dir_str.strip_prefix("$basedir/") { + shim.parent()?.join(rest) + } else if Path::new(pkg_dir_str).is_absolute() { + PathBuf::from(pkg_dir_str) + } else { + continue; + }; + if let Some(version) = + read_package_json_version(&pkg_dir.join("package.json"), Some(expected_pkg)) + { + return Some(version); + } + } + None +} + +/// Resolve an installed version out of pnpm's global layout. The global bin +/// dir (`$PNPM_HOME`) holds shim scripts while the packages live under +/// `global//node_modules/` (pnpm ≤10) or +/// `global///node_modules/` (pnpm 11), so scan those +/// trees for the expected package. Also checks the bin dir's parent to cover a +/// `global-bin-dir` pointed one level down (e.g. `$PNPM_HOME/bin`). +fn pnpm_global_package_json(bin_dir: &Path, expected_pkg: &str) -> Option { + let mut bases = vec![bin_dir]; + if let Some(parent) = bin_dir.parent() { + bases.push(parent); + } + for base in bases { + let Ok(layouts) = std::fs::read_dir(base.join("global")) else { + continue; + }; + for layout in layouts.flatten() { + let mut candidates = vec![layout.path()]; + if let Ok(hashed) = std::fs::read_dir(layout.path()) { + candidates.extend(hashed.flatten().map(|e| e.path())); + } + for candidate in candidates { + let pj = candidate + .join("node_modules") + .join(expected_pkg) + .join("package.json"); + if pj.is_file() { + if let Some(version) = read_package_json_version(&pj, Some(expected_pkg)) { + return Some(version); } } } } - dir = d.parent(); } None } @@ -770,6 +854,8 @@ mod tests { assert!(is_self_updating(Some(&InstallSource::CurlPipe))); assert!(is_self_updating(Some(&InstallSource::Bundled))); assert!(!is_self_updating(Some(&InstallSource::Npm))); + assert!(!is_self_updating(Some(&InstallSource::Pnpm))); + assert!(!is_self_updating(Some(&InstallSource::Bun))); assert!(!is_self_updating(Some(&InstallSource::Brew))); assert!(!is_self_updating(None)); } @@ -934,6 +1020,159 @@ mod tests { ); } + #[test] + fn package_json_pnpm_shim_reads_global_layout() { + let root = scratch_dir("pj-pnpm-shim"); + // pnpm's global bin entry is a generated shim script, not a symlink, + // so the walk-up never reaches a node_modules tree; the package lives + // under global//node_modules/ next to the shim. + let pkg = root.join("global/5/node_modules/@earendil-works/pi-coding-agent"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "@earendil-works/pi-coding-agent", "version": "0.82.1"}"#, + ) + .unwrap(); + let shim = root.join("pi"); + std::fs::write(&shim, "#!/bin/sh\nexec node dist/cli.js\n").unwrap(); + + assert_eq!( + installed_version_from_package_json(&shim, Some("@earendil-works/pi-coding-agent")) + .as_deref(), + Some("0.82.1"), + ); + // Without a known package id there is nothing to scan for. + assert_eq!(installed_version_from_package_json(&shim, None), None); + } + + #[test] + fn package_json_pnpm_shim_in_bin_subdir_reads_parent_global_layout() { + let root = scratch_dir("pj-pnpm-bin-subdir"); + // A global-bin-dir pointed at $PNPM_HOME/bin keeps packages one level + // up from the shims. + let pkg = root.join("global/5/node_modules/pi-acp"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "pi-acp", "version": "0.0.32"}"#, + ) + .unwrap(); + std::fs::create_dir_all(root.join("bin")).unwrap(); + let shim = root.join("bin/pi-acp"); + std::fs::write(&shim, "#!/bin/sh\nexec node dist/index.js\n").unwrap(); + + assert_eq!( + installed_version_from_package_json(&shim, Some("pi-acp")).as_deref(), + Some("0.0.32"), + ); + } + + #[test] + fn package_json_pnpm_v11_bin_shim_reads_hashed_layout() { + let root = scratch_dir("pj-pnpm-v11"); + // pnpm 11 puts shims in $PNPM_HOME/bin and packages under a hashed + // project dir: global/v11//node_modules/. The shim embeds + // the entrypoint relative to its own dir via `$basedir/..`. + let pkg = root.join("global/v11/68ba-19fcaecfd18/node_modules/pi-acp"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "pi-acp", "version": "0.0.33"}"#, + ) + .unwrap(); + std::fs::create_dir_all(root.join("bin")).unwrap(); + let shim = root.join("bin/pi-acp"); + std::fs::write( + &shim, + concat!( + "#!/bin/sh\n", + "basedir=$(dirname \"$0\")\n", + "exec node \"$basedir/../global/v11/68ba-19fcaecfd18/", + "node_modules/pi-acp/dist/index.js\" \"$@\"\n", + ), + ) + .unwrap(); + + assert_eq!( + installed_version_from_package_json(&shim, Some("pi-acp")).as_deref(), + Some("0.0.33"), + ); + + // The hashed layout is also reachable by the scan fallback alone + // (shim formats can change; the scan must not depend on parsing). + assert_eq!( + pnpm_global_package_json(&root.join("bin"), "pi-acp").as_deref(), + Some("0.0.33"), + ); + } + + #[test] + fn package_json_pnpm_shim_path_wins_over_second_layout_tree() { + let root = scratch_dir("pj-pnpm-two-layouts"); + // After a pnpm major upgrade two layout trees can coexist (e.g. + // `global/5` and `global/v11`). The shim script embeds the exact + // entrypoint it runs, so that version must win over a layout scan. + for (layout, version) in [("5", "0.82.1"), ("v11", "0.83.0")] { + let pkg = root.join(format!( + "global/{layout}/node_modules/@earendil-works/pi-coding-agent" + )); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + format!(r#"{{"name": "@earendil-works/pi-coding-agent", "version": "{version}"}}"#), + ) + .unwrap(); + } + let shim = root.join("pi"); + std::fs::write( + &shim, + concat!( + "#!/bin/sh\n", + "basedir=$(dirname \"$0\")\n", + "exec node \"$basedir/global/5/.pnpm/@earendil-works+pi-coding-agent@0.82.1_x/", + "node_modules/@earendil-works/pi-coding-agent/dist/cli.js\" \"$@\"\n", + ), + ) + .unwrap(); + // The .pnpm store path in the shim: mirror the package.json there too, + // matching pnpm's real layout (global//node_modules symlinks into it). + let store_pkg = root.join( + "global/5/.pnpm/@earendil-works+pi-coding-agent@0.82.1_x/node_modules/@earendil-works/pi-coding-agent", + ); + std::fs::create_dir_all(&store_pkg).unwrap(); + std::fs::write( + store_pkg.join("package.json"), + br#"{"name": "@earendil-works/pi-coding-agent", "version": "0.82.1"}"#, + ) + .unwrap(); + + assert_eq!( + installed_version_from_package_json(&shim, Some("@earendil-works/pi-coding-agent")) + .as_deref(), + Some("0.82.1"), + "shim-embedded path must beat the newer layout tree", + ); + } + + #[test] + fn package_json_pnpm_layout_mismatched_name_returns_none() { + let root = scratch_dir("pj-pnpm-mismatch"); + let pkg = root.join("global/5/node_modules/pi-acp"); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "someone-else", "version": "9.9.9"}"#, + ) + .unwrap(); + let shim = root.join("pi-acp"); + std::fs::write(&shim, "#!/bin/sh\n").unwrap(); + + assert_eq!( + installed_version_from_package_json(&shim, Some("pi-acp")), + None + ); + } + #[test] fn select_probe_npm_reads_package_json() { match select_installed_probe(Some(&InstallSource::Npm), Some("amp-acp")) { @@ -944,6 +1183,20 @@ mod tests { } } + /// pnpm/bun global installs are npm-registry packages linked through + /// `node_modules/` trees — the package.json probe applies to them too. + #[test] + fn select_probe_pnpm_and_bun_read_package_json() { + for src in [InstallSource::Pnpm, InstallSource::Bun] { + match select_installed_probe(Some(&src), Some("@earendil-works/pi-coding-agent")) { + InstalledProbe::NpmPackageJson { package_id } => { + assert_eq!(package_id, Some("@earendil-works/pi-coding-agent")); + } + _ => panic!("{src:?} install source should select NpmPackageJson probe"), + } + } + } + #[test] fn select_probe_non_npm_uses_cli_version() { for src in [ diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index d51109a3f..97b25ec4e 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -426,18 +426,29 @@ fn resolve_package( /// source-aware `update_command` + `update_fix_type` from the readout's install /// source and the supplied package id. The flat (non-agent) slot never gets an /// update command — non-agent updates are out of scope. +/// +/// `self_update_command` is the agent's own updater for this readout's binary +/// (e.g. Pi's `pi update --self`), if it declares one. For a `CurlPipe` install +/// it replaces both halves of the usual curl-pipe treatment: the install is +/// *not* marked self-updating (the updater is user-actioned, unlike Cursor's +/// background auto-update), and the declared command — which re-installs in +/// place regardless of prefix — is emitted instead of a registry recipe. fn apply_freshness( readout: &mut AgentVersionInfo, info: &freshness::VersionInfo, slot: ReadoutSlot, package_id: Option<&str>, + self_update_command: Option<&str>, ) { readout.installed_version = info.installed.clone(); readout.latest_version = info.latest.clone(); + let own_updater = self_update_command + .filter(|_| matches!(readout.install_source, Some(InstallSource::CurlPipe))); // Self-updating tools (curl/native installers) manage their own freshness: // report installed/latest for display, but never raise an "update available" - // nag — the update isn't the user's to action. - let self_updating = is_self_updating(readout.install_source.as_ref()); + // nag — the update isn't the user's to action. An agent-declared updater + // (`own_updater`) opts back in: that update *is* the user's to action. + let self_updating = own_updater.is_none() && is_self_updating(readout.install_source.as_ref()); readout.self_updating = Some(self_updating); readout.update_available = if self_updating { None @@ -452,7 +463,10 @@ fn apply_freshness( ReadoutSlot::Flat => None, }; if let (true, Some(fix_type)) = (actionable, slot_fix_type) { - if let Some(cmd) = derive_update_command(readout.install_source.as_ref(), package_id) { + let command = own_updater + .map(str::to_string) + .or_else(|| derive_update_command(readout.install_source.as_ref(), package_id)); + if let Some(cmd) = command { readout.update_command = Some(cmd); readout.update_fix_type = Some(fix_type); } @@ -618,13 +632,22 @@ async fn populate_freshness( let mut freshness_timeouts = Vec::new(); if let Some((info, pkg)) = by_target.remove(&(check.id.clone(), ReadoutSlot::Main)) { if let Some(readout) = check.main.as_mut() { - apply_freshness(readout, &info, ReadoutSlot::Main, pkg.as_deref()); + apply_freshness( + readout, + &info, + ReadoutSlot::Main, + pkg.as_deref(), + // The declared self-update command targets the agent's + // main CLI (`pi update --self` updates `pi`); bridges + // keep their registry recipes. + agents::agent_self_update_command(&check.id), + ); } freshness_timeouts.extend(info.command_timeouts); } if let Some((info, pkg)) = by_target.remove(&(check.id.clone(), ReadoutSlot::Bridge)) { if let Some(readout) = check.bridge.as_mut() { - apply_freshness(readout, &info, ReadoutSlot::Bridge, pkg.as_deref()); + apply_freshness(readout, &info, ReadoutSlot::Bridge, pkg.as_deref(), None); } freshness_timeouts.extend(info.command_timeouts); } @@ -1380,6 +1403,7 @@ mod tests { &info, ReadoutSlot::Main, Some("@agentclientprotocol/claude-agent-acp"), + None, ); assert_eq!( readout.update_command.as_deref(), @@ -1401,7 +1425,13 @@ mod tests { update_available: Some(true), command_timeouts: Vec::new(), }; - apply_freshness(&mut readout, &info, ReadoutSlot::Main, Some("ampcode")); + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("ampcode"), + None, + ); assert_eq!( readout.update_command.as_deref(), Some("brew upgrade ampcode"), @@ -1425,7 +1455,7 @@ mod tests { update_available: Some(true), command_timeouts: Vec::new(), }; - apply_freshness(&mut readout, &info, ReadoutSlot::Main, None); + apply_freshness(&mut readout, &info, ReadoutSlot::Main, None, None); assert_eq!(readout.installed_version.as_deref(), Some("2.1.205")); assert!(readout.update_available.is_none()); assert!(readout.update_command.is_none()); @@ -1486,14 +1516,14 @@ mod tests { update_available: Some(true), command_timeouts: Vec::new(), }; - apply_freshness(&mut readout, &info, ReadoutSlot::Bridge, Some("amp")); + apply_freshness(&mut readout, &info, ReadoutSlot::Bridge, Some("amp"), None); assert_eq!(readout.update_command.as_deref(), Some("brew upgrade amp"),); assert_eq!(readout.update_fix_type, Some(FixType::UpdateBridge)); } - /// Self-updating (CurlPipe) readouts never get an update command, even when - /// upstream reports a newer version — `is_self_updating` suppresses both - /// `update_available` and the derived update command. + /// A CurlPipe readout with no agent-declared updater never gets an update + /// command, even when upstream reports a newer version — `is_self_updating` + /// suppresses both `update_available` and the derived update command. #[tokio::test] async fn apply_freshness_curl_pipe_never_emits_update_command() { let mut readout = AgentVersionInfo { @@ -1511,6 +1541,7 @@ mod tests { &info, ReadoutSlot::Main, Some("getcursor/cursor"), + None, ); assert!(readout.update_command.is_none()); assert!(readout.update_fix_type.is_none()); @@ -1521,6 +1552,90 @@ mod tests { ); } + /// A CurlPipe readout whose agent declares its own updater (Pi's + /// `pi update --self`) keeps the update nag and emits that command instead + /// of a registry recipe: the updater is user-actioned, so the install is + /// not marked self-updating. + #[tokio::test] + async fn apply_freshness_curl_pipe_own_updater_emits_self_update_command() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::CurlPipe), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("0.82.1".into()), + latest: Some("0.83.0".into()), + update_available: Some(true), + command_timeouts: Vec::new(), + }; + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("@earendil-works/pi-coding-agent"), + Some("pi update --self"), + ); + assert_eq!(readout.update_command.as_deref(), Some("pi update --self")); + assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain)); + assert_eq!(readout.self_updating, Some(false)); + assert_eq!(readout.update_available, Some(true)); + } + + /// The declared updater is still gated on an actionable update: already + /// up to date -> no command, no fix type. + #[tokio::test] + async fn apply_freshness_own_updater_requires_actionable_update() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::CurlPipe), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("0.83.0".into()), + latest: Some("0.83.0".into()), + update_available: Some(false), + command_timeouts: Vec::new(), + }; + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("@earendil-works/pi-coding-agent"), + Some("pi update --self"), + ); + assert!(readout.update_command.is_none()); + assert!(readout.update_fix_type.is_none()); + assert_eq!(readout.update_available, Some(false)); + } + + /// The declared updater only overrides CurlPipe installs — a registry + /// install (npm) keeps its source-consistent registry recipe. + #[tokio::test] + async fn apply_freshness_own_updater_ignored_for_registry_installs() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::Npm), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("0.82.1".into()), + latest: Some("0.83.0".into()), + update_available: Some(true), + command_timeouts: Vec::new(), + }; + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("@earendil-works/pi-coding-agent"), + Some("pi update --self"), + ); + assert_eq!( + readout.update_command.as_deref(), + Some("npm install -g @earendil-works/pi-coding-agent@latest"), + ); + assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain)); + assert_eq!(readout.self_updating, Some(false)); + } + /// No update available -> no update command, even on a registry source. #[tokio::test] async fn apply_freshness_no_update_available_emits_no_command() { @@ -1534,7 +1649,13 @@ mod tests { update_available: Some(false), command_timeouts: Vec::new(), }; - apply_freshness(&mut readout, &info, ReadoutSlot::Main, Some("amp-acp")); + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("amp-acp"), + None, + ); assert!(readout.update_command.is_none()); assert!(readout.update_fix_type.is_none()); } diff --git a/crates/doctor/src/package_ids.rs b/crates/doctor/src/package_ids.rs index 2ca961563..566c49f5f 100644 --- a/crates/doctor/src/package_ids.rs +++ b/crates/doctor/src/package_ids.rs @@ -105,14 +105,59 @@ pub(crate) const PACKAGE_IDS: &[(&str, &[PackageEntry])] = &[ Role::Any, )], ), + // ai-agent-pi: every install method is npm-under-the-hood, so `latest` + // always comes from the npm registry. The main CLI ships as + // `@earendil-works/pi-coding-agent` via npm/pnpm/bun, plus the pi.dev curl + // installer (classified `CurlPipe` when it lands in its `~/.local` + // fallback prefix — updated via `pi update --self`, see the agent's + // `self_update_command`). The ACP bridge is the separately-maintained + // `pi-acp` npm package. ( "ai-agent-pi", - &[( - InstallSource::Npm, - "pi-acp", - LatestSource::Npm, - Role::Bridge, - )], + &[ + ( + InstallSource::Npm, + "@earendil-works/pi-coding-agent", + LatestSource::Npm, + Role::Main, + ), + ( + InstallSource::Pnpm, + "@earendil-works/pi-coding-agent", + LatestSource::Npm, + Role::Main, + ), + ( + InstallSource::Bun, + "@earendil-works/pi-coding-agent", + LatestSource::Npm, + Role::Main, + ), + ( + InstallSource::CurlPipe, + "@earendil-works/pi-coding-agent", + LatestSource::Npm, + Role::Main, + ), + ( + InstallSource::Npm, + "pi-acp", + LatestSource::Npm, + Role::Bridge, + ), + ( + InstallSource::Pnpm, + "pi-acp", + LatestSource::Npm, + Role::Bridge, + ), + ( + InstallSource::Bun, + "pi-acp", + LatestSource::Npm, + Role::Bridge, + ), + ], ), ( "ai-agent-amp", @@ -314,6 +359,56 @@ mod tests { assert_eq!(latest, LatestSource::GitHubReleases); } + /// Pi's main CLI ships from the npm registry through every install method + /// (npm, pnpm, bun, and the curl installer's npm-under-the-hood layout) — + /// each source must resolve the same package with an npm latest lookup. + #[test] + fn pi_main_resolves_for_all_npm_backed_sources() { + for source in [ + InstallSource::Npm, + InstallSource::Pnpm, + InstallSource::Bun, + InstallSource::CurlPipe, + ] { + assert_eq!( + lookup_package_id("ai-agent-pi", source.clone(), Role::Main), + Some(("@earendil-works/pi-coding-agent", LatestSource::Npm)), + "{source:?}", + ); + } + } + + /// The pi-acp bridge is registry-installed only (npm/pnpm/bun) — there is + /// no curl install of the bridge, so CurlPipe must miss. + #[test] + fn pi_bridge_resolves_for_registry_sources_only() { + for source in [InstallSource::Npm, InstallSource::Pnpm, InstallSource::Bun] { + assert_eq!( + lookup_package_id("ai-agent-pi", source.clone(), Role::Bridge), + Some(("pi-acp", LatestSource::Npm)), + "{source:?}", + ); + } + assert_eq!( + lookup_package_id("ai-agent-pi", InstallSource::CurlPipe, Role::Bridge), + None, + ); + } + + /// Role tagging keeps the two Pi binaries from answering each other's + /// lookups even though both ship via npm. + #[test] + fn pi_roles_do_not_cross_answer() { + assert_eq!( + lookup_package_id("ai-agent-pi", InstallSource::Npm, Role::Main), + Some(("@earendil-works/pi-coding-agent", LatestSource::Npm)), + ); + assert_eq!( + lookup_package_id("ai-agent-pi", InstallSource::Npm, Role::Bridge), + Some(("pi-acp", LatestSource::Npm)), + ); + } + #[test] fn copilot_brew_resolves_to_copilot_cli_cask() { assert_eq!( diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index dcb9d082e..610b2d39d 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -291,7 +291,14 @@ fn detect_install_source_with_env(path: &Path, env: Option<&DoctorEnv>) -> Insta let home = env .and_then(|env| env.get("HOME").map(PathBuf::from)) .or_else(std::env::home_dir); - let base = detect_install_source_with_home(path, home.as_deref()); + let pnpm_home = env_or_process_var(env, "PNPM_HOME").map(PathBuf::from); + let bun_install = env_or_process_var(env, "BUN_INSTALL").map(PathBuf::from); + let base = detect_install_source_inner( + path, + home.as_deref(), + pnpm_home.as_deref(), + bun_install.as_deref(), + ); if base == InstallSource::Unknown { if let Some(home) = home.as_deref() { if fingerprint_curl_pipe(path, home) { @@ -302,6 +309,17 @@ fn detect_install_source_with_env(path: &Path, env: Option<&DoctorEnv>) -> Insta base } +/// Read a variable from the caller snapshot when one is supplied, otherwise +/// from the process environment. Empty values are treated as absent. +fn env_or_process_var(env: Option<&DoctorEnv>, name: &str) -> Option { + let value = if let Some(env) = env { + env.get(name).map(str::to_string) + } else { + std::env::var(name).ok() + }; + value.filter(|s| !s.is_empty()) +} + fn shell_lookup_commands(cmd: &str) -> [(&'static str, String); 2] { let quoted = shell_quote(cmd); [ @@ -449,33 +467,67 @@ const CURL_INSTALLER_FOOTPRINTS: &[CurlInstallerFootprint] = &[ binary: "amp", markers: &[".local/share/amp", ".cache/amp"], }, + // Pi installer — pi.dev/install.sh. When the global npm prefix isn't + // writable it runs `npm install -g --prefix ~/.local`, leaving the package + // tree under `~/.local/lib/node_modules` (always) and a standalone Node + // runtime under `~/.local/share/pi-node` (only when no usable node was + // found). The package-tree marker must be checked *before* the generic + // npm-layout classification — the bin entry canonicalizes into + // `node_modules/`, but a plain `npm install -g` can't update this prefix; + // `pi update --self` can. + CurlInstallerFootprint { + binary: "pi", + markers: &[ + ".local/lib/node_modules/@earendil-works/pi-coding-agent", + ".local/share/pi-node", + ], + }, ]; +/// Signal 1 of the curl-pipe fingerprint: the binary sits in a user-local bin +/// dir (`~/.local/bin`, `~/bin`) and a known installer footprint marker for +/// that binary name exists under `$HOME`. Low-false-positive enough to run +/// *before* the npm-layout classification (some installers, e.g. Pi's, lay +/// down npm-shaped trees that `npm install -g` nevertheless can't update). +fn matches_curl_installer_footprint(path: &Path, home: &Path) -> bool { + if !in_user_local_bin(path, home) { + return false; + } + if let Some(name) = path.file_name().and_then(|n| n.to_str()) { + for fp in CURL_INSTALLER_FOOTPRINTS { + if fp.binary == name && fp.markers.iter().any(|m| home.join(m).exists()) { + return true; + } + } + } + false +} + +fn in_user_local_bin(path: &Path, home: &Path) -> bool { + path.starts_with(home.join(".local/bin")) || path.starts_with(home.join("bin")) +} + /// Cheap filesystem fingerprint for curl/native installs that path-prefix /// heuristics can't classify. Only considers binaries inside a user-local bin /// dir (`~/.local/bin`, `~/bin`) and uses two low-false-positive signals: /// /// 1. A known installer footprint marker (see [`CURL_INSTALLER_FOOTPRINTS`]) -/// exists under `$HOME` and the binary name matches that installer. +/// exists under `$HOME` and the binary name matches that installer +/// ([`matches_curl_installer_footprint`], which also runs earlier in the +/// detection chain, ahead of the npm-layout check). /// 2. The bin entry is a symlink into a *versioned* install dir under `$HOME` /// (the layout Cursor's native installer uses: /// `~/.local/bin/` → `…/versions//`). /// /// No subprocess or network access — only `read_link`/`exists`/`canonicalize`. fn fingerprint_curl_pipe(path: &Path, home: &Path) -> bool { - let in_user_local_bin = - path.starts_with(home.join(".local/bin")) || path.starts_with(home.join("bin")); - if !in_user_local_bin { + if !in_user_local_bin(path, home) { return false; } // Signal 1 — a known installer footprint marker exists under $HOME. - if let Some(name) = path.file_name().and_then(|n| n.to_str()) { - for fp in CURL_INSTALLER_FOOTPRINTS { - if fp.binary == name && fp.markers.iter().any(|m| home.join(m).exists()) { - return true; - } - } + if matches_curl_installer_footprint(path, home) { + return true; } // Signal 2 — the bin entry is a symlink into a versioned install dir under @@ -548,9 +600,46 @@ fn looks_like_npm_global(path: &Path) -> bool { target.components().any(|c| c.as_os_str() == "node_modules") } -/// Testable inner: same logic as [`detect_install_source`] but takes the home -/// directory as a parameter so unit tests can inject a fixed value. +/// Whether the binary lives in pnpm's global install dir — `$PNPM_HOME` when +/// the caller environment declares it, else the platform defaults +/// (`~/Library/pnpm` on macOS, `~/.local/share/pnpm` on Linux). pnpm links +/// global bins directly inside this dir, so a prefix check on the resolved +/// path is sufficient; no symlink chasing needed. +fn looks_like_pnpm_global(path: &Path, home: Option<&Path>, pnpm_home: Option<&Path>) -> bool { + if pnpm_home.is_some_and(|dir| path.starts_with(dir)) { + return true; + } + let Some(home) = home else { + return false; + }; + path.starts_with(home.join("Library/pnpm")) || path.starts_with(home.join(".local/share/pnpm")) +} + +/// Whether the binary lives in bun's install dir — `$BUN_INSTALL` when the +/// caller environment declares it, else the default `~/.bun`. `bun add -g` +/// links bins at `/bin/` pointing into +/// `/install/global/node_modules/…`. +fn looks_like_bun_global(path: &Path, home: Option<&Path>, bun_install: Option<&Path>) -> bool { + if bun_install.is_some_and(|dir| path.starts_with(dir)) { + return true; + } + home.is_some_and(|home| path.starts_with(home.join(".bun"))) +} + +/// Test-only shorthand for [`detect_install_source_inner`] with no +/// environment-declared pnpm/bun dirs — unit tests inject a fixed home and +/// rely on the platform-default locations. +#[cfg(test)] fn detect_install_source_with_home(path: &Path, home: Option<&Path>) -> InstallSource { + detect_install_source_inner(path, home, None, None) +} + +fn detect_install_source_inner( + path: &Path, + home: Option<&Path>, + pnpm_home: Option<&Path>, + bun_install: Option<&Path>, +) -> InstallSource { // Homebrew cask ownership beats npm internals. In a mixed Claude install, // the active `/opt/homebrew/bin/claude` can be a cask symlink while an // unrelated npm global package also exists; update planning must follow @@ -559,6 +648,27 @@ fn detect_install_source_with_home(path: &Path, home: Option<&Path>) -> InstallS return InstallSource::Brew; } + // Known curl-installer footprints in user-local bin dirs beat the npm + // layout check below: Pi's installer lays down an npm-shaped tree under + // `~/.local` (bin symlink into `~/.local/lib/node_modules/…`) that a plain + // `npm install -g` can't update — the install is owned by the installer's + // own update path, not the user's global npm prefix. + if let Some(home) = home { + if matches_curl_installer_footprint(path, home) { + return InstallSource::CurlPipe; + } + } + + // pnpm/bun global installs also canonicalize into `node_modules/` trees, + // but `npm install -g` doesn't own them either — classify by their global + // install dirs before the npm check. + if looks_like_pnpm_global(path, home, pnpm_home) { + return InstallSource::Pnpm; + } + if looks_like_bun_global(path, home, bun_install) { + return InstallSource::Bun; + } + // npm global install (any node distribution). Checked first: the bin entry // is a symlink into `node_modules/`, which may live under a brew prefix // (`npm config get prefix = /opt/homebrew`), so this must win over the @@ -1224,6 +1334,182 @@ mod tests { } } + /// Pi's curl installer fallback layout: `~/.local/bin/pi` symlinked into + /// `~/.local/lib/node_modules/@earendil-works/pi-coding-agent/…`. The tree + /// is npm-shaped (the npm-layout check alone would say `Npm`), but a plain + /// `npm install -g` can't update this prefix — the footprint check must win + /// and classify it `CurlPipe`. + #[test] + fn pi_curl_installer_user_local_layout_classifies_as_curl_pipe() { + #[cfg(unix)] + { + let home = std::env::temp_dir().join(format!("doctor-pi-curl-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + let pkg = home.join(".local/lib/node_modules/@earendil-works/pi-coding-agent/dist"); + fs::create_dir_all(&pkg).unwrap(); + let real = pkg.join("cli.js"); + File::create(&real).unwrap(); + fs::create_dir_all(home.join(".local/bin")).unwrap(); + let link = home.join(".local/bin/pi"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + assert!( + looks_like_npm_global(&link), + "layout is npm-shaped — the footprint must be what reclassifies it", + ); + assert!(matches_curl_installer_footprint(&link, &home)); + assert_eq!( + detect_install_source_with_home(&link, Some(home.as_path())), + InstallSource::CurlPipe, + ); + let _ = fs::remove_dir_all(&home); + } + } + + /// The standalone-node marker (`~/.local/share/pi-node`, laid down when the + /// installer had no usable Node.js) also fingerprints the curl install. + #[test] + fn pi_standalone_node_marker_classifies_as_curl_pipe() { + let home = std::env::temp_dir().join(format!("doctor-pi-node-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + fs::create_dir_all(home.join(".local/share/pi-node")).unwrap(); + fs::create_dir_all(home.join(".local/bin")).unwrap(); + let bin = home.join(".local/bin/pi"); + File::create(&bin).unwrap(); + + assert_eq!( + detect_install_source_with_home(&bin, Some(home.as_path())), + InstallSource::CurlPipe, + ); + let _ = fs::remove_dir_all(&home); + } + + /// A `pi` on the global npm prefix (installed with plain `npm install -g`, + /// or by the curl installer when the prefix was writable) stays `Npm` even + /// when a stale `~/.local` footprint marker exists — the footprint only + /// claims binaries inside user-local bin dirs. + #[test] + fn pi_on_npm_prefix_stays_npm_despite_footprint_marker() { + #[cfg(unix)] + { + let root = std::env::temp_dir().join(format!("doctor-pi-npm-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let home = root.join("home"); + fs::create_dir_all(home.join(".local/share/pi-node")).unwrap(); + let pkg = root.join("prefix/lib/node_modules/@earendil-works/pi-coding-agent/dist"); + fs::create_dir_all(&pkg).unwrap(); + let real = pkg.join("cli.js"); + File::create(&real).unwrap(); + fs::create_dir_all(root.join("prefix/bin")).unwrap(); + let link = root.join("prefix/bin/pi"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + assert_eq!( + detect_install_source_with_home(&link, Some(home.as_path())), + InstallSource::Npm, + ); + let _ = fs::remove_dir_all(&root); + } + } + + /// pnpm global installs live under `$PNPM_HOME` (defaults `~/Library/pnpm` + /// on macOS, `~/.local/share/pnpm` on Linux) and must classify `Pnpm`, not + /// `Npm` — `npm install -g` neither owns nor updates them. + #[test] + fn pnpm_default_dirs_classify_as_pnpm() { + let home = PathBuf::from("/home/test"); + assert_eq!( + detect_install_source_with_home(&home.join("Library/pnpm/pi"), Some(home.as_path())), + InstallSource::Pnpm, + ); + assert_eq!( + detect_install_source_with_home( + &home.join(".local/share/pnpm/pi"), + Some(home.as_path()), + ), + InstallSource::Pnpm, + ); + } + + /// A custom `$PNPM_HOME` from the caller snapshot wins even outside the + /// default locations. + #[test] + fn custom_pnpm_home_from_env_classifies_as_pnpm() { + let env = crate::DoctorEnv::new(vec![ + ("HOME".to_string(), "/home/test".to_string()), + ("PNPM_HOME".to_string(), "/data/pnpm".to_string()), + ]); + assert_eq!( + detect_install_source_with_env(Path::new("/data/pnpm/pi"), Some(&env)), + InstallSource::Pnpm, + ); + } + + /// The pnpm check must beat the npm-layout check even when the bin entry is + /// a real symlink into pnpm's global `node_modules` store. + #[test] + fn pnpm_symlink_into_global_store_classifies_as_pnpm_not_npm() { + #[cfg(unix)] + { + let home = std::env::temp_dir().join(format!("doctor-pnpm-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + let pkg = home + .join("Library/pnpm/global/5/node_modules/@earendil-works/pi-coding-agent/dist"); + fs::create_dir_all(&pkg).unwrap(); + let real = pkg.join("cli.js"); + File::create(&real).unwrap(); + fs::create_dir_all(home.join("Library/pnpm")).unwrap(); + let link = home.join("Library/pnpm/pi"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + assert!(looks_like_npm_global(&link), "pnpm layout is npm-shaped"); + assert_eq!( + detect_install_source_with_home(&link, Some(home.as_path())), + InstallSource::Pnpm, + ); + let _ = fs::remove_dir_all(&home); + } + } + + /// bun global installs live under `~/.bun` (or `$BUN_INSTALL`) with bin + /// symlinks into `~/.bun/install/global/node_modules` — `Bun`, not `Npm`. + #[test] + fn bun_global_symlink_classifies_as_bun_not_npm() { + #[cfg(unix)] + { + let home = std::env::temp_dir().join(format!("doctor-bun-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + let pkg = + home.join(".bun/install/global/node_modules/@earendil-works/pi-coding-agent/dist"); + fs::create_dir_all(&pkg).unwrap(); + let real = pkg.join("cli.js"); + File::create(&real).unwrap(); + fs::create_dir_all(home.join(".bun/bin")).unwrap(); + let link = home.join(".bun/bin/pi"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + assert!(looks_like_npm_global(&link), "bun layout is npm-shaped"); + assert_eq!( + detect_install_source_with_home(&link, Some(home.as_path())), + InstallSource::Bun, + ); + let _ = fs::remove_dir_all(&home); + } + } + + /// A custom `$BUN_INSTALL` from the caller snapshot classifies as `Bun`. + #[test] + fn custom_bun_install_from_env_classifies_as_bun() { + let env = crate::DoctorEnv::new(vec![ + ("HOME".to_string(), "/home/test".to_string()), + ("BUN_INSTALL".to_string(), "/data/bun".to_string()), + ]); + assert_eq!( + detect_install_source_with_env(Path::new("/data/bun/bin/pi"), Some(&env)), + InstallSource::Bun, + ); + } + #[test] fn fingerprint_curl_pipe_no_match_without_footprint() { let home = std::env::temp_dir().join(format!("doctor-fp-none-{}", std::process::id())); diff --git a/crates/doctor/src/types.rs b/crates/doctor/src/types.rs index 0aa18fbee..7b5ab7a45 100644 --- a/crates/doctor/src/types.rs +++ b/crates/doctor/src/types.rs @@ -54,6 +54,16 @@ pub enum AuthStatus { pub enum InstallSource { Brew, Npm, + /// pnpm global install (`pnpm add -g`), resolved from the pnpm global dir + /// (`$PNPM_HOME`; defaults `~/Library/pnpm` on macOS, `~/.local/share/pnpm` + /// on Linux). Distinct from [`InstallSource::Npm`] even though the binary + /// canonicalizes into a `node_modules/` tree: `npm install -g` neither owns + /// nor updates these binaries. + Pnpm, + /// Bun global install (`bun add -g`), resolved from `$BUN_INSTALL/bin` + /// (default `~/.bun/bin`). Distinct from [`InstallSource::Npm`] for the + /// same reason as [`InstallSource::Pnpm`]. + Bun, Cargo, Mise, Asdf, From e601bf43bf61feb82328948984d988b35231a3e1 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 17:11:47 +1000 Subject: [PATCH 2/2] fix(doctor): derive install source and npm prefix from the binary itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CURL_INSTALLER_FOOTPRINTS` decided a binary was curl-installed by checking whether some path existed under `$HOME`. That answers a question about the machine, not about the binary PATH resolved to: with amp installed both ways (curl at `~/.amp/bin`, npm-global on a user-configured prefix) the marker is present either way, so a user-managed npm install could be labelled `CurlPipe` and have its update nag silenced. Delete the heuristic and fix the *action* it was standing in for. - resolve.rs: drop `CurlInstallerFootprint`, `CURL_INSTALLER_FOOTPRINTS`, `matches_curl_installer_footprint`, its early promotion in `detect_install_source_inner`, and signal 1 of `fingerprint_curl_pipe`. The fingerprint itself stays: signal 2 (a `~/.local/bin` entry symlinked into a versioned install dir) is the sole positive classifier for cursor-agent's and the native claude's layouts. - resolve.rs: repair signal 2's home gate, which compared a canonicalized symlink target against a non-canonicalized `$HOME` — any home reached through a symlinked ancestor never matched, and the footprint was masking it. - resolve.rs: add `npm_prefix_for_binary`, deriving `` from the package tree the bin entry canonicalizes into (`/lib/node_modules//…`), anchored on the first `lib`/`node_modules` pair so nested dependency trees still yield the outer prefix. - agents.rs/lib.rs: `derive_update_command` takes that prefix and emits `npm install -g --prefix

@latest` (shell-quoted — the command runs under `sh -c`). npm installs into whichever prefix npm is *configured* with, which is not always the one the resolved binary lives in; without this, an install made with an explicit `--prefix`, or one under a node version the user has since switched away from, gets "updated" by a second copy landing elsewhere while the stale binary keeps resolving. Where the two agree the flag is a no-op, and an underivable prefix falls back to the bare command. Main and bridge readouts derive independently. - Pi's `~/.local` installer fallback now classifies `Npm` — honestly, since the installer runs `npm install -g --ignore-scripts --prefix ~/.local` — and updates through the npm recipe. That makes the `CurlPipe`-gated `self_update_command` path unreachable, so drop the field, its `pi update --self` entry, and `agent_self_update_command`. `pi update --self` is not a drop-in replacement in the general case: it errors out on a pnpm-managed install ("not managed by a global npm install"), so gating on it per source would reintroduce the same species of per-agent table. - package_ids.rs: add Amp's npm main package `@ampcode/cli` (canonical; `@sourcegraph/amp` is the renamed alias). Correct but inert behind a mirror that filters young versions — Amp publishes continuously, so Block's Artifactory serves both packages with no `latest` dist-tag; the comment says so. Live-verified through `run_checks_with_options` and `execute_fix_streaming_with_env_options`: pi at `~/.local` classifies `Npm`, gets `--prefix '/Users/…/.local'`, and upgrades 0.82.1 -> 0.83.0 in place with no second install in any other prefix; pi at the configured prefix emits a prefix equal to `npm prefix -g` (a no-op) and upgrades in place, main and bridge (pi-acp 0.0.32 -> 0.0.33); a pi under a prefix that differs from the configured one upgrades in place while the configured prefix stays untouched. cursor-agent and the native claude still classify `CurlPipe`; curl-installed amp still reports `Unknown` raw and `CurlPipe` via its per-agent override. All with the Artifactory registry override appended. Note for berd: `InstallSource` is a cross-repo wire contract and the `pnpm`/ `bun` variants added by e84c6df are still missing from berd's TS union and its exhaustive `SOURCE_LABEL_KEYS`, so those installs render a broken "installed via …" line. That remains outstanding and must land with the doctor rev bump. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 154 +++++++----- crates/doctor/src/lib.rs | 170 +++++-------- crates/doctor/src/package_ids.rs | 36 ++- crates/doctor/src/resolve.rs | 393 +++++++++++++++++-------------- 4 files changed, 403 insertions(+), 350 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index 15bbabf21..8a9ebc0bd 100644 --- a/crates/doctor/src/agents.rs +++ b/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, @@ -50,15 +50,6 @@ pub struct AgentCheckInfo { /// by the embedding app's lock and the version worth surfacing is the /// vendored harness CLI's (e.g. Claude Code 2.1.x). pub bundled_version_args: Option<&'static [&'static str]>, - /// Shell command that runs the agent's own updater for its main CLI (e.g. - /// `pi update --self`). Used by the freshness pass as the update command - /// for a [`InstallSource::CurlPipe`] main readout, where registry-derived - /// recipes would target the wrong prefix (Pi's installer lays down an - /// npm-shaped tree under `~/.local` that a plain `npm install -g` can't - /// reach) but the tool's own updater re-installs in place. Unlike - /// Cursor/Amp-style background auto-updaters, a declared self-update - /// command marks the install as user-actioned: the update nag stays. - pub self_update_command: Option<&'static str>, } /// All AI agents we check for individually. @@ -76,7 +67,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ auth_status_command: None, install_source_override: None, bundled_version_args: None, - self_update_command: None, }, // The claude-agent-acp bridge vendors the complete Claude Code CLI and // forwards `--cli ` to it, sharing the user's credential store @@ -95,7 +85,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ auth_status_command: Some("claude-agent-acp --cli auth status"), install_source_override: None, bundled_version_args: Some(&["--cli", "--version"]), - self_update_command: None, }, // The codex-acp bridge vendors the full `codex` binary and forwards // `cli ` to it, sharing the user's ~/.codex/auth.json — same @@ -116,7 +105,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ // `--version` anywhere in argv and prints its own version, so only // codex's clap short flag reaches the vendored binary. bundled_version_args: Some(&["cli", "-V"]), - self_update_command: None, }, // 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 @@ -137,12 +125,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ auth_status_command: None, install_source_override: None, bundled_version_args: None, - // Pi's curl installer (pi.dev/install.sh) falls back to an npm-shaped - // tree under `~/.local` when the global npm prefix isn't writable; - // `pi update --self` re-installs in place for every layout (it infers - // its own prefix/package manager), where `npm install -g …@latest` - // would target the wrong prefix. - self_update_command: Some("pi update --self"), }, AgentCheckInfo { id: "ai-agent-amp", @@ -158,7 +140,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ // Main `amp` curl installer; bridge `amp-acp` is npm (detected positively). install_source_override: Some(InstallSource::CurlPipe), bundled_version_args: None, - self_update_command: None, }, AgentCheckInfo { id: "ai-agent-copilot", @@ -173,7 +154,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ auth_status_command: None, install_source_override: None, bundled_version_args: None, - self_update_command: None, }, AgentCheckInfo { id: "ai-agent-cursor", @@ -190,7 +170,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ // resolved binary is the curl install — the override's primary use case. install_source_override: Some(InstallSource::CurlPipe), bundled_version_args: None, - self_update_command: None, }, ]; @@ -219,6 +198,16 @@ 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 @@ -226,10 +215,19 @@ pub(crate) fn bundled_version_probe_args( pub fn derive_update_command( install_source: Option<&InstallSource>, package_id: Option<&str>, + npm_prefix: Option<&Path>, ) -> Option { 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}")), @@ -243,17 +241,6 @@ pub fn derive_update_command( } } -/// The agent's declared self-update command for its main CLI (e.g. Pi's -/// `pi update --self`), when one exists. Consulted by the freshness pass for -/// [`InstallSource::CurlPipe`] main readouts — see -/// [`AgentCheckInfo::self_update_command`]. -pub(crate) fn agent_self_update_command(check_id: &str) -> Option<&'static str> { - AI_AGENT_CHECKS - .iter() - .find(|info| info.id == check_id) - .and_then(|info| info.self_update_command) -} - /// Append `--registry=` to `command` when a registry override is supplied /// and the command is npm-backed. Non-npm commands (curl-pipe installers, auth /// commands, the git-clonefile fix, …) and the `None` registry case return the @@ -777,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", @@ -1099,10 +1082,10 @@ mod tests { } /// Pi's registry entry: npm-shaped install commands for both binaries (so - /// the registry override applies), no auth commands (Pi owns its own - /// provider/model configuration), and its own updater for curl installs. + /// the registry override applies) and no auth commands (Pi owns its own + /// provider/model configuration). #[test] - fn pi_declares_install_bridge_and_self_update_commands() { + 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"]); @@ -1119,7 +1102,6 @@ mod tests { assert_eq!(pi.auth_command, None); assert_eq!(pi.auth_status_command, None); assert_eq!(pi.install_source_override, None); - assert_eq!(pi.self_update_command, Some("pi update --self")); } /// Main CLI present, bridge missing → `FixType::Bridge` with the @@ -1177,30 +1159,19 @@ mod tests { assert!(check.bridge_path.is_none()); } - #[test] - fn self_update_command_lookup_only_for_declaring_agents() { - assert_eq!( - agent_self_update_command("ai-agent-pi"), - Some("pi update --self"), - ); - // Cursor/Amp curl installs auto-update in the background — they keep - // the self-updating suppression, not a user-actioned updater. - assert_eq!(agent_self_update_command("ai-agent-cursor"), None); - assert_eq!(agent_self_update_command("ai-agent-amp"), 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")).as_deref(), + derive_update_command(Some(&InstallSource::Bun), Some("pi-acp"), None).as_deref(), Some("bun add -g pi-acp@latest"), ); } @@ -1308,22 +1279,82 @@ 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"), ); } @@ -1331,7 +1362,7 @@ mod tests { #[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"), ); } @@ -1347,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:?}", ); @@ -1356,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] diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index 97b25ec4e..c2a20232b 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -427,28 +427,23 @@ fn resolve_package( /// source and the supplied package id. The flat (non-agent) slot never gets an /// update command — non-agent updates are out of scope. /// -/// `self_update_command` is the agent's own updater for this readout's binary -/// (e.g. Pi's `pi update --self`), if it declares one. For a `CurlPipe` install -/// it replaces both halves of the usual curl-pipe treatment: the install is -/// *not* marked self-updating (the updater is user-actioned, unlike Cursor's -/// background auto-update), and the declared command — which re-installs in -/// place regardless of prefix — is emitted instead of a registry recipe. +/// `npm_prefix` is the npm prefix that owns this readout's binary (see +/// [`resolve::npm_prefix_for_binary`]), so an emitted npm recipe updates the +/// install this readout is about rather than whichever prefix npm happens to be +/// configured with. fn apply_freshness( readout: &mut AgentVersionInfo, info: &freshness::VersionInfo, slot: ReadoutSlot, package_id: Option<&str>, - self_update_command: Option<&str>, + npm_prefix: Option<&Path>, ) { readout.installed_version = info.installed.clone(); readout.latest_version = info.latest.clone(); - let own_updater = self_update_command - .filter(|_| matches!(readout.install_source, Some(InstallSource::CurlPipe))); // Self-updating tools (curl/native installers) manage their own freshness: // report installed/latest for display, but never raise an "update available" - // nag — the update isn't the user's to action. An agent-declared updater - // (`own_updater`) opts back in: that update *is* the user's to action. - let self_updating = own_updater.is_none() && is_self_updating(readout.install_source.as_ref()); + // nag — the update isn't the user's to action. + let self_updating = is_self_updating(readout.install_source.as_ref()); readout.self_updating = Some(self_updating); readout.update_available = if self_updating { None @@ -463,9 +458,8 @@ fn apply_freshness( ReadoutSlot::Flat => None, }; if let (true, Some(fix_type)) = (actionable, slot_fix_type) { - let command = own_updater - .map(str::to_string) - .or_else(|| derive_update_command(readout.install_source.as_ref(), package_id)); + let command = + derive_update_command(readout.install_source.as_ref(), package_id, npm_prefix); if let Some(cmd) = command { readout.update_command = Some(cmd); readout.update_fix_type = Some(fix_type); @@ -630,6 +624,17 @@ async fn populate_freshness( let is_agent = check.main.is_some() || check.bridge.is_some(); if is_agent { let mut freshness_timeouts = Vec::new(); + // Each readout's update recipe is pinned to the prefix owning *its* + // binary: main and bridge can be installed under different prefixes + // (e.g. `pi` under `~/.local`, `pi-acp` under the global one). + let main_prefix = check + .path + .as_deref() + .and_then(|path| resolve::npm_prefix_for_binary(Path::new(path))); + let bridge_prefix = check + .bridge_path + .as_deref() + .and_then(|path| resolve::npm_prefix_for_binary(Path::new(path))); if let Some((info, pkg)) = by_target.remove(&(check.id.clone(), ReadoutSlot::Main)) { if let Some(readout) = check.main.as_mut() { apply_freshness( @@ -637,17 +642,20 @@ async fn populate_freshness( &info, ReadoutSlot::Main, pkg.as_deref(), - // The declared self-update command targets the agent's - // main CLI (`pi update --self` updates `pi`); bridges - // keep their registry recipes. - agents::agent_self_update_command(&check.id), + main_prefix.as_deref(), ); } freshness_timeouts.extend(info.command_timeouts); } if let Some((info, pkg)) = by_target.remove(&(check.id.clone(), ReadoutSlot::Bridge)) { if let Some(readout) = check.bridge.as_mut() { - apply_freshness(readout, &info, ReadoutSlot::Bridge, pkg.as_deref(), None); + apply_freshness( + readout, + &info, + ReadoutSlot::Bridge, + pkg.as_deref(), + bridge_prefix.as_deref(), + ); } freshness_timeouts.extend(info.command_timeouts); } @@ -1412,6 +1420,38 @@ mod tests { assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain)); } + /// The npm prefix owning the readout's binary reaches the emitted recipe, so + /// an agent installed under a non-default prefix is upgraded where it lives + /// instead of a second copy landing in npm's configured prefix. + #[tokio::test] + async fn apply_freshness_npm_main_pins_update_to_owning_prefix() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::Npm), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("0.82.1".into()), + latest: Some("0.83.0".into()), + update_available: Some(true), + command_timeouts: Vec::new(), + }; + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("@earendil-works/pi-coding-agent"), + Some(Path::new("/Users/test/.local")), + ); + assert_eq!( + readout.update_command.as_deref(), + Some( + "npm install -g --prefix '/Users/test/.local' \ + @earendil-works/pi-coding-agent@latest" + ), + ); + assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain)); + } + /// A brew-installed main CLI updates via `brew upgrade `. #[tokio::test] async fn apply_freshness_brew_main_emits_update_main_command() { @@ -1521,9 +1561,9 @@ mod tests { assert_eq!(readout.update_fix_type, Some(FixType::UpdateBridge)); } - /// A CurlPipe readout with no agent-declared updater never gets an update - /// command, even when upstream reports a newer version — `is_self_updating` - /// suppresses both `update_available` and the derived update command. + /// A CurlPipe readout never gets an update command, even when upstream + /// reports a newer version — `is_self_updating` suppresses both + /// `update_available` and the derived update command. #[tokio::test] async fn apply_freshness_curl_pipe_never_emits_update_command() { let mut readout = AgentVersionInfo { @@ -1552,90 +1592,6 @@ mod tests { ); } - /// A CurlPipe readout whose agent declares its own updater (Pi's - /// `pi update --self`) keeps the update nag and emits that command instead - /// of a registry recipe: the updater is user-actioned, so the install is - /// not marked self-updating. - #[tokio::test] - async fn apply_freshness_curl_pipe_own_updater_emits_self_update_command() { - let mut readout = AgentVersionInfo { - install_source: Some(InstallSource::CurlPipe), - ..AgentVersionInfo::default() - }; - let info = freshness::VersionInfo { - installed: Some("0.82.1".into()), - latest: Some("0.83.0".into()), - update_available: Some(true), - command_timeouts: Vec::new(), - }; - apply_freshness( - &mut readout, - &info, - ReadoutSlot::Main, - Some("@earendil-works/pi-coding-agent"), - Some("pi update --self"), - ); - assert_eq!(readout.update_command.as_deref(), Some("pi update --self")); - assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain)); - assert_eq!(readout.self_updating, Some(false)); - assert_eq!(readout.update_available, Some(true)); - } - - /// The declared updater is still gated on an actionable update: already - /// up to date -> no command, no fix type. - #[tokio::test] - async fn apply_freshness_own_updater_requires_actionable_update() { - let mut readout = AgentVersionInfo { - install_source: Some(InstallSource::CurlPipe), - ..AgentVersionInfo::default() - }; - let info = freshness::VersionInfo { - installed: Some("0.83.0".into()), - latest: Some("0.83.0".into()), - update_available: Some(false), - command_timeouts: Vec::new(), - }; - apply_freshness( - &mut readout, - &info, - ReadoutSlot::Main, - Some("@earendil-works/pi-coding-agent"), - Some("pi update --self"), - ); - assert!(readout.update_command.is_none()); - assert!(readout.update_fix_type.is_none()); - assert_eq!(readout.update_available, Some(false)); - } - - /// The declared updater only overrides CurlPipe installs — a registry - /// install (npm) keeps its source-consistent registry recipe. - #[tokio::test] - async fn apply_freshness_own_updater_ignored_for_registry_installs() { - let mut readout = AgentVersionInfo { - install_source: Some(InstallSource::Npm), - ..AgentVersionInfo::default() - }; - let info = freshness::VersionInfo { - installed: Some("0.82.1".into()), - latest: Some("0.83.0".into()), - update_available: Some(true), - command_timeouts: Vec::new(), - }; - apply_freshness( - &mut readout, - &info, - ReadoutSlot::Main, - Some("@earendil-works/pi-coding-agent"), - Some("pi update --self"), - ); - assert_eq!( - readout.update_command.as_deref(), - Some("npm install -g @earendil-works/pi-coding-agent@latest"), - ); - assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain)); - assert_eq!(readout.self_updating, Some(false)); - } - /// No update available -> no update command, even on a registry source. #[tokio::test] async fn apply_freshness_no_update_available_emits_no_command() { diff --git a/crates/doctor/src/package_ids.rs b/crates/doctor/src/package_ids.rs index 566c49f5f..1fd9e33db 100644 --- a/crates/doctor/src/package_ids.rs +++ b/crates/doctor/src/package_ids.rs @@ -107,11 +107,13 @@ pub(crate) const PACKAGE_IDS: &[(&str, &[PackageEntry])] = &[ ), // ai-agent-pi: every install method is npm-under-the-hood, so `latest` // always comes from the npm registry. The main CLI ships as - // `@earendil-works/pi-coding-agent` via npm/pnpm/bun, plus the pi.dev curl - // installer (classified `CurlPipe` when it lands in its `~/.local` - // fallback prefix — updated via `pi update --self`, see the agent's - // `self_update_command`). The ACP bridge is the separately-maintained - // `pi-acp` npm package. + // `@earendil-works/pi-coding-agent` via npm/pnpm/bun; the pi.dev curl + // installer is npm too (`npm install -g --ignore-scripts --prefix`), so its + // `~/.local` fallback layout classifies `Npm` and updates through the npm + // recipe like any other. The `CurlPipe` entry is only a display fallback — + // should a Pi ever fingerprint as a native install, the readout still shows + // a latest version (report-only, no nag). The ACP bridge is the + // separately-maintained `pi-acp` npm package. ( "ai-agent-pi", &[ @@ -162,14 +164,34 @@ pub(crate) const PACKAGE_IDS: &[(&str, &[PackageEntry])] = &[ ( "ai-agent-amp", &[ - // Bridge: npm. Main: brew. Main curl-pipe install is `CurlPipe`, - // not present in the table (self-updating, report-only). + // Bridge: npm. Main: npm or brew. Main curl-pipe install is + // `CurlPipe`, not present in the table (self-updating, report-only). ( InstallSource::Npm, "amp-acp", LatestSource::Npm, Role::Bridge, ), + // Amp's own npm package. `@ampcode/cli` is canonical (bin `amp`); + // `@sourcegraph/amp` is the renamed alias that re-exports it and was + // slated for removal on 2026-06-15. + // + // CAVEAT: correct but inert behind a mirror that filters by version + // age. Amp publishes continuously (a new version every few minutes), + // so a mirror that withholds young versions serves the package with + // no `latest` dist-tag at all — Block's Artifactory currently + // reports `{"placeholder": "0.0.0-placeholder"}` for `@ampcode/cli` + // and `{"next": …}` for `@sourcegraph/amp`, though upstream npm has + // `latest` for both. There, `npm view … version` comes back empty → + // `latest_version: None` → no nag, and the update command would fail + // `ETARGET`. Pi is unaffected because its releases age past the + // filter. + ( + InstallSource::Npm, + "@ampcode/cli", + LatestSource::Npm, + Role::Main, + ), // Sourcegraph Amp ships from the `ampcode/tap` tap as `ampcode`. // WARNING: homebrew-core's `amp` formula is an unrelated GPL-3.0 // terminal text editor — do NOT use that package id here, or diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 610b2d39d..aabb49a5f 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -285,8 +285,8 @@ fn resolved_binary( /// (mirroring the dirs in [`npm_search_dirs`]), and the System dirs. When those /// fall through to [`InstallSource::Unknown`] for a binary in a user-local bin /// dir, a cheap filesystem fingerprint (see [`fingerprint_curl_pipe`]) is -/// attempted to recognise curl/native installers (Cursor, Amp), using the -/// caller snapshot's `HOME` when one is supplied. +/// attempted to recognise the versioned-symlink layout native installers use +/// (Cursor, Claude), using the caller snapshot's `HOME` when one is supplied. fn detect_install_source_with_env(path: &Path, env: Option<&DoctorEnv>) -> InstallSource { let home = env .and_then(|env| env.get("HOME").map(PathBuf::from)) @@ -328,7 +328,8 @@ fn shell_lookup_commands(cmd: &str) -> [(&'static str, String); 2] { ] } -fn shell_quote(value: &str) -> String { +/// Single-quote `value` for safe interpolation into a `sh -c` command line. +pub(crate) fn shell_quote(value: &str) -> String { format!("'{}'", value.replace('\'', "'\\''")) } @@ -440,114 +441,66 @@ fn npm_global_bin_dir( Some(bin) } -/// A known curl/native installer footprint for a binary that lands in a -/// user-local bin dir. Pairs the binary name with marker paths (relative to -/// `$HOME`) that the installer also creates; if the binary lives in a user-local -/// bin dir and any marker exists, the install is fingerprinted as a curl-pipe -/// install. -struct CurlInstallerFootprint { - /// Binary file name as it appears in `~/.local/bin` or `~/bin`. - binary: &'static str, - /// Marker paths relative to `$HOME`; if any exists the fingerprint matches. - markers: &'static [&'static str], -} - -/// Known footprints of curl/native installers whose binaries land in a -/// user-local bin dir. Conservative on purpose — only well-known data dirs are -/// listed so a bare `~/.local/bin/` with no installer footprint stays -/// [`InstallSource::Unknown`]. -const CURL_INSTALLER_FOOTPRINTS: &[CurlInstallerFootprint] = &[ - // Cursor CLI installer — cursor.com/install. - CurlInstallerFootprint { - binary: "cursor-agent", - markers: &[".local/share/cursor-agent/versions", ".cursor/bin"], - }, - // Amp installer — ampcode.com/install.sh. - CurlInstallerFootprint { - binary: "amp", - markers: &[".local/share/amp", ".cache/amp"], - }, - // Pi installer — pi.dev/install.sh. When the global npm prefix isn't - // writable it runs `npm install -g --prefix ~/.local`, leaving the package - // tree under `~/.local/lib/node_modules` (always) and a standalone Node - // runtime under `~/.local/share/pi-node` (only when no usable node was - // found). The package-tree marker must be checked *before* the generic - // npm-layout classification — the bin entry canonicalizes into - // `node_modules/`, but a plain `npm install -g` can't update this prefix; - // `pi update --self` can. - CurlInstallerFootprint { - binary: "pi", - markers: &[ - ".local/lib/node_modules/@earendil-works/pi-coding-agent", - ".local/share/pi-node", - ], - }, -]; - -/// Signal 1 of the curl-pipe fingerprint: the binary sits in a user-local bin -/// dir (`~/.local/bin`, `~/bin`) and a known installer footprint marker for -/// that binary name exists under `$HOME`. Low-false-positive enough to run -/// *before* the npm-layout classification (some installers, e.g. Pi's, lay -/// down npm-shaped trees that `npm install -g` nevertheless can't update). -fn matches_curl_installer_footprint(path: &Path, home: &Path) -> bool { - if !in_user_local_bin(path, home) { - return false; - } - if let Some(name) = path.file_name().and_then(|n| n.to_str()) { - for fp in CURL_INSTALLER_FOOTPRINTS { - if fp.binary == name && fp.markers.iter().any(|m| home.join(m).exists()) { - return true; - } - } - } - false -} - fn in_user_local_bin(path: &Path, home: &Path) -> bool { path.starts_with(home.join(".local/bin")) || path.starts_with(home.join("bin")) } -/// Cheap filesystem fingerprint for curl/native installs that path-prefix -/// heuristics can't classify. Only considers binaries inside a user-local bin -/// dir (`~/.local/bin`, `~/bin`) and uses two low-false-positive signals: +/// Cheap filesystem fingerprint for native installs that path-prefix heuristics +/// can't classify. Only considers binaries inside a user-local bin dir +/// (`~/.local/bin`, `~/bin`), and on a single low-false-positive signal: the bin +/// entry is a symlink into a *versioned* install dir under `$HOME` — the layout +/// Cursor's and Claude's native installers use +/// (`~/.local/bin/` → `…/versions//`). /// -/// 1. A known installer footprint marker (see [`CURL_INSTALLER_FOOTPRINTS`]) -/// exists under `$HOME` and the binary name matches that installer -/// ([`matches_curl_installer_footprint`], which also runs earlier in the -/// detection chain, ahead of the npm-layout check). -/// 2. The bin entry is a symlink into a *versioned* install dir under `$HOME` -/// (the layout Cursor's native installer uses: -/// `~/.local/bin/` → `…/versions//`). +/// Every input is local to the resolved binary: what this path *is*, never +/// whether some marker happens to exist under `$HOME`. An ambient marker +/// describes the machine rather than the binary PATH resolved to — with a tool +/// installed both by an installer and by npm, the marker is present either way. /// -/// No subprocess or network access — only `read_link`/`exists`/`canonicalize`. +/// No subprocess or network access — only `read_link`/`canonicalize`. fn fingerprint_curl_pipe(path: &Path, home: &Path) -> bool { if !in_user_local_bin(path, home) { return false; } - // Signal 1 — a known installer footprint marker exists under $HOME. - if matches_curl_installer_footprint(path, home) { - return true; - } - - // Signal 2 — the bin entry is a symlink into a versioned install dir under - // $HOME. - if let Ok(target) = std::fs::read_link(path) { - let resolved = if target.is_absolute() { - target - } else if let Some(parent) = path.parent() { - parent.join(target) - } else { - target - }; - let resolved = resolved.canonicalize().unwrap_or(resolved); - if resolved.starts_with(home) && resolved.components().any(|c| c.as_os_str() == "versions") - { - return true; - } - } + let Ok(target) = std::fs::read_link(path) else { + return false; + }; + let resolved = if target.is_absolute() { + target + } else if let Some(parent) = path.parent() { + parent.join(target) + } else { + target + }; + let resolved = resolved.canonicalize().unwrap_or(resolved); + // `resolved` is canonical, so `$HOME` must be too before comparing: a home + // reached through a symlinked ancestor (macOS `/tmp` → `/private/tmp`) never + // matches as written. Falls back to `home` as-is if canonicalize fails. + let home = home.canonicalize().unwrap_or_else(|_| home.to_path_buf()); + resolved.starts_with(&home) && resolved.components().any(|c| c.as_os_str() == "versions") +} - false +/// The npm prefix that owns `path`, derived from where the package tree actually +/// lives: `/lib/node_modules//…` → ``. +/// +/// The global bin entry is a symlink into the package tree, so the path is +/// canonicalized first (falling back to the path as-is, like +/// [`looks_like_npm_global`]). Anchors on the *first* adjacent +/// `lib`/`node_modules` component pair so a nested dependency tree +/// (`/lib/node_modules/a/node_modules/b`) still yields ``. +/// +/// Returns `None` when the path has no such pair — callers then fall back to a +/// prefix-less `npm install -g`, which targets npm's configured global prefix. +pub fn npm_prefix_for_binary(path: &Path) -> Option { + let canonical = path.canonicalize(); + let target = canonical.as_deref().unwrap_or(path); + let components: Vec<_> = target.components().collect(); + let lib = components + .windows(2) + .position(|pair| pair[0].as_os_str() == "lib" && pair[1].as_os_str() == "node_modules")?; + let prefix: PathBuf = components[..lib].iter().collect(); + (!prefix.as_os_str().is_empty()).then_some(prefix) } /// Whether the active binary path is owned by a Homebrew cask. @@ -648,17 +601,6 @@ fn detect_install_source_inner( return InstallSource::Brew; } - // Known curl-installer footprints in user-local bin dirs beat the npm - // layout check below: Pi's installer lays down an npm-shaped tree under - // `~/.local` (bin symlink into `~/.local/lib/node_modules/…`) that a plain - // `npm install -g` can't update — the install is owned by the installer's - // own update path, not the user's global npm prefix. - if let Some(home) = home { - if matches_curl_installer_footprint(path, home) { - return InstallSource::CurlPipe; - } - } - // pnpm/bun global installs also canonicalize into `node_modules/` trees, // but `npm install -g` doesn't own them either — classify by their global // install dirs before the npm check. @@ -768,6 +710,14 @@ mod tests { format!("'{}'", value.replace('\'', "'\\''")) } + /// A caller snapshot declaring only `HOME`. Classifying through + /// [`detect_install_source_with_env`] runs the full chain — unlike the + /// [`detect_install_source_with_home`] shorthand, it also reaches the + /// [`fingerprint_curl_pipe`] fallback. + fn home_env(home: &Path) -> crate::DoctorEnv { + crate::DoctorEnv::new(vec![("HOME".to_string(), home.display().to_string())]) + } + fn write_login_path_rewrite_profiles(home: &Path, path: &Path) { let profile = format!( "export PATH={}\n", @@ -1299,20 +1249,6 @@ mod tests { let _ = fs::remove_dir_all(&root); } - #[test] - fn fingerprint_curl_pipe_matches_known_installer_marker() { - let home = std::env::temp_dir().join(format!("doctor-fp-marker-{}", std::process::id())); - let _ = fs::remove_dir_all(&home); - // Amp installer: ~/.local/bin/amp + ~/.local/share/amp. - fs::create_dir_all(home.join(".local/bin")).unwrap(); - fs::create_dir_all(home.join(".local/share/amp")).unwrap(); - let bin = home.join(".local/bin/amp"); - File::create(&bin).unwrap(); - - assert!(fingerprint_curl_pipe(&bin, &home)); - let _ = fs::remove_dir_all(&home); - } - #[test] fn fingerprint_curl_pipe_matches_versioned_symlink() { #[cfg(unix)] @@ -1334,16 +1270,18 @@ mod tests { } } - /// Pi's curl installer fallback layout: `~/.local/bin/pi` symlinked into - /// `~/.local/lib/node_modules/@earendil-works/pi-coding-agent/…`. The tree - /// is npm-shaped (the npm-layout check alone would say `Npm`), but a plain - /// `npm install -g` can't update this prefix — the footprint check must win - /// and classify it `CurlPipe`. + /// Pi's installer fallback layout: `npm install -g --prefix ~/.local` leaves + /// `~/.local/bin/pi` symlinked into + /// `~/.local/lib/node_modules/@earendil-works/pi-coding-agent/…`. The tree is + /// npm-shaped because it *is* an npm install, and it classifies `Npm` — + /// updatable with an npm recipe aimed at the prefix that owns it + /// ([`npm_prefix_for_binary`]). No `$HOME` marker gets a say: living under + /// `~/.local` is not evidence about how a binary was installed. #[test] - fn pi_curl_installer_user_local_layout_classifies_as_curl_pipe() { + fn pi_user_local_npm_layout_classifies_as_npm() { #[cfg(unix)] { - let home = std::env::temp_dir().join(format!("doctor-pi-curl-{}", std::process::id())); + let home = std::env::temp_dir().join(format!("doctor-pi-local-{}", std::process::id())); let _ = fs::remove_dir_all(&home); let pkg = home.join(".local/lib/node_modules/@earendil-works/pi-coding-agent/dist"); fs::create_dir_all(&pkg).unwrap(); @@ -1352,66 +1290,157 @@ mod tests { fs::create_dir_all(home.join(".local/bin")).unwrap(); let link = home.join(".local/bin/pi"); std::os::unix::fs::symlink(&real, &link).unwrap(); + // The standalone-node runtime the installer drops when it finds no + // usable node. Present or absent, it changes nothing. + fs::create_dir_all(home.join(".local/share/pi-node")).unwrap(); - assert!( - looks_like_npm_global(&link), - "layout is npm-shaped — the footprint must be what reclassifies it", + assert_eq!( + detect_install_source_with_env(&link, Some(&home_env(&home))), + InstallSource::Npm, ); - assert!(matches_curl_installer_footprint(&link, &home)); assert_eq!( - detect_install_source_with_home(&link, Some(home.as_path())), - InstallSource::CurlPipe, + npm_prefix_for_binary(&link), + Some(home.join(".local").canonicalize().unwrap()), ); let _ = fs::remove_dir_all(&home); } } - /// The standalone-node marker (`~/.local/share/pi-node`, laid down when the - /// installer had no usable Node.js) also fingerprints the curl install. + /// An npm-global install under a user-configured `~/.local` prefix stays + /// `Npm` even for a tool whose installer also writes runtime dirs there + /// (`~/.cache/amp`, `~/.local/share/amp` — both created by `amp` on first + /// run, whatever installed it). Classification looks at the resolved binary, + /// so an ambient data dir can no longer flip a user-managed npm install to + /// `CurlPipe` and silence its update nag. #[test] - fn pi_standalone_node_marker_classifies_as_curl_pipe() { - let home = std::env::temp_dir().join(format!("doctor-pi-node-{}", std::process::id())); - let _ = fs::remove_dir_all(&home); - fs::create_dir_all(home.join(".local/share/pi-node")).unwrap(); - fs::create_dir_all(home.join(".local/bin")).unwrap(); - let bin = home.join(".local/bin/pi"); - File::create(&bin).unwrap(); + fn amp_npm_layout_under_user_local_prefix_stays_npm_with_runtime_dirs_present() { + #[cfg(unix)] + { + let home = + std::env::temp_dir().join(format!("doctor-amp-local-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + let pkg = home.join(".local/lib/node_modules/@ampcode/cli/bin"); + fs::create_dir_all(&pkg).unwrap(); + let real = pkg.join("amp.exe"); + File::create(&real).unwrap(); + fs::create_dir_all(home.join(".local/bin")).unwrap(); + let link = home.join(".local/bin/amp"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + fs::create_dir_all(home.join(".cache/amp")).unwrap(); + fs::create_dir_all(home.join(".local/share/amp")).unwrap(); - assert_eq!( - detect_install_source_with_home(&bin, Some(home.as_path())), - InstallSource::CurlPipe, - ); - let _ = fs::remove_dir_all(&home); + assert_eq!( + detect_install_source_with_env(&link, Some(&home_env(&home))), + InstallSource::Npm, + ); + let _ = fs::remove_dir_all(&home); + } } - /// A `pi` on the global npm prefix (installed with plain `npm install -g`, - /// or by the curl installer when the prefix was writable) stays `Npm` even - /// when a stale `~/.local` footprint marker exists — the footprint only - /// claims binaries inside user-local bin dirs. + /// Regression guard for the one fingerprint signal that survives: Cursor's + /// native installer layout (`~/.local/bin/cursor-agent` → a versioned dir) + /// classifies `CurlPipe` all the way through `detect_install_source_*`, with + /// no installer-marker table involved. The same shape covers the native + /// `claude` install (`~/.local/share/claude/versions/`). #[test] - fn pi_on_npm_prefix_stays_npm_despite_footprint_marker() { + fn cursor_agent_versioned_symlink_classifies_as_curl_pipe() { #[cfg(unix)] { - let root = std::env::temp_dir().join(format!("doctor-pi-npm-{}", std::process::id())); - let _ = fs::remove_dir_all(&root); - let home = root.join("home"); - fs::create_dir_all(home.join(".local/share/pi-node")).unwrap(); - let pkg = root.join("prefix/lib/node_modules/@earendil-works/pi-coding-agent/dist"); - fs::create_dir_all(&pkg).unwrap(); - let real = pkg.join("cli.js"); + let home = + std::env::temp_dir().join(format!("doctor-cursor-vers-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + let versioned = home.join(".local/share/cursor-agent/versions/2025.09.18-7ae6800"); + fs::create_dir_all(&versioned).unwrap(); + let real = versioned.join("cursor-agent"); File::create(&real).unwrap(); - fs::create_dir_all(root.join("prefix/bin")).unwrap(); - let link = root.join("prefix/bin/pi"); + fs::create_dir_all(home.join(".local/bin")).unwrap(); + let link = home.join(".local/bin/cursor-agent"); std::os::unix::fs::symlink(&real, &link).unwrap(); assert_eq!( - detect_install_source_with_home(&link, Some(home.as_path())), - InstallSource::Npm, + detect_install_source_with_env(&link, Some(&home_env(&home))), + InstallSource::CurlPipe, + ); + let _ = fs::remove_dir_all(&home); + } + } + + /// The npm global bin entry is a *relative* symlink into the package tree, so + /// the prefix has to come out of the canonicalized target. + #[test] + fn npm_prefix_for_binary_follows_relative_bin_symlink() { + #[cfg(unix)] + { + let root = + std::env::temp_dir().join(format!("doctor-npm-prefix-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let prefix = root.join("prefix"); + let pkg = prefix.join("lib/node_modules/@earendil-works/pi-coding-agent/dist"); + fs::create_dir_all(&pkg).unwrap(); + File::create(pkg.join("cli.js")).unwrap(); + fs::create_dir_all(prefix.join("bin")).unwrap(); + std::os::unix::fs::symlink( + "../lib/node_modules/@earendil-works/pi-coding-agent/dist/cli.js", + prefix.join("bin/pi"), + ) + .unwrap(); + + assert_eq!( + npm_prefix_for_binary(&prefix.join("bin/pi")), + Some(prefix.canonicalize().unwrap()), ); let _ = fs::remove_dir_all(&root); } } + /// Prefix derivation over the layouts seen in the wild. These paths need not + /// exist — with canonicalize unavailable the components are read as given. + #[test] + fn npm_prefix_for_binary_derives_prefix_from_package_tree() { + for (path, expected) in [ + ( + "/opt/homebrew/lib/node_modules/@earendil-works/pi-coding-agent/dist/cli.js", + "/opt/homebrew", + ), + ( + "/Users/test/.local/lib/node_modules/@earendil-works/pi-coding-agent/dist/cli.js", + "/Users/test/.local", + ), + ( + "/Users/test/.nvm/versions/node/v22.14.0/lib/node_modules/pi-acp/dist/index.js", + "/Users/test/.nvm/versions/node/v22.14.0", + ), + // A nested dependency tree resolves to the *outer* prefix, not the + // inner `node_modules`. + ( + "/opt/homebrew/lib/node_modules/outer/node_modules/inner/cli.js", + "/opt/homebrew", + ), + ] { + assert_eq!( + npm_prefix_for_binary(Path::new(path)), + Some(PathBuf::from(expected)), + "{path}", + ); + } + } + + /// No `lib/node_modules` pair — no prefix, and the caller falls back to a + /// bare `npm install -g`. Covers non-npm binaries and pnpm's global shims + /// (which are generated scripts, not symlinks into a `lib/` tree). + #[test] + fn npm_prefix_for_binary_none_without_package_tree() { + for path in [ + "/usr/local/bin/goose", + "/Users/test/Library/pnpm/pi", + // `node_modules` without the `lib` parent: a project-local install, + // which is nobody's global prefix. + "/Users/test/project/node_modules/.bin/tsc", + ] { + assert_eq!(npm_prefix_for_binary(Path::new(path)), None, "{path}"); + } + } + /// pnpm global installs live under `$PNPM_HOME` (defaults `~/Library/pnpm` /// on macOS, `~/.local/share/pnpm` on Linux) and must classify `Pnpm`, not /// `Npm` — `npm install -g` neither owns nor updates them. @@ -1511,10 +1540,10 @@ mod tests { } #[test] - fn fingerprint_curl_pipe_no_match_without_footprint() { + fn fingerprint_curl_pipe_no_match_for_plain_user_local_binary() { let home = std::env::temp_dir().join(format!("doctor-fp-none-{}", std::process::id())); let _ = fs::remove_dir_all(&home); - // A bare ~/.local/bin binary with no installer footprint stays Unknown. + // A real file in ~/.local/bin — not a versioned symlink — stays Unknown. fs::create_dir_all(home.join(".local/bin")).unwrap(); let bin = home.join(".local/bin/mytool"); File::create(&bin).unwrap(); @@ -1523,16 +1552,28 @@ mod tests { let _ = fs::remove_dir_all(&home); } + /// The fingerprint only claims user-local bin dirs: the same versioned-symlink + /// layout parked somewhere else on PATH is left to the path-prefix checks. #[test] fn fingerprint_curl_pipe_ignores_binaries_outside_user_local_bin() { - let home = std::env::temp_dir().join(format!("doctor-fp-outside-{}", std::process::id())); - let _ = fs::remove_dir_all(&home); - // Marker exists, but the binary is elsewhere — must not fingerprint. - fs::create_dir_all(home.join(".local/share/amp")).unwrap(); - let bin = PathBuf::from("/tmp/elsewhere/amp"); + #[cfg(unix)] + { + let root = + std::env::temp_dir().join(format!("doctor-fp-outside-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let home = root.join("home"); + let versioned = home.join(".local/share/cursor-agent/versions/1.0.0"); + fs::create_dir_all(&versioned).unwrap(); + let real = versioned.join("cursor-agent"); + File::create(&real).unwrap(); + let elsewhere = root.join("elsewhere"); + fs::create_dir_all(&elsewhere).unwrap(); + let link = elsewhere.join("cursor-agent"); + std::os::unix::fs::symlink(&real, &link).unwrap(); - assert!(!fingerprint_curl_pipe(&bin, &home)); - let _ = fs::remove_dir_all(&home); + assert!(!fingerprint_curl_pipe(&link, &home)); + let _ = fs::remove_dir_all(&root); + } } #[test]