Skip to content

Commit b335c40

Browse files
matt2eclaude
andcommitted
fix(doctor): derive install source and npm prefix from the binary itself
`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 `<prefix>` from the package tree the bin entry canonicalizes into (`<prefix>/lib/node_modules/<pkg>/…`), 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 <P> <pkg>@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) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
1 parent 146440b commit b335c40

4 files changed

Lines changed: 403 additions & 350 deletions

File tree

crates/doctor/src/agents.rs

Lines changed: 94 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use crate::command::{
66
run_command_with_timeout, CommandError, CommandTimeout, DEFAULT_PROBE_TIMEOUT,
77
};
88
use crate::environment::{apply_doctor_env, DoctorEnv};
9-
use crate::resolve::format_command_output;
9+
use crate::resolve::{format_command_output, shell_quote};
1010
use crate::timeout_check::{command_timeout_check, TimeoutCheck};
1111
use crate::types::{
1212
AgentVersionInfo, AuthStatus, CheckStatus, DoctorCheck, FixType, InstallSource, ResolvedBinary,
@@ -49,15 +49,6 @@ pub struct AgentCheckInfo {
4949
/// by the embedding app's lock and the version worth surfacing is the
5050
/// vendored harness CLI's (e.g. Claude Code 2.1.x).
5151
pub bundled_version_args: Option<&'static [&'static str]>,
52-
/// Shell command that runs the agent's own updater for its main CLI (e.g.
53-
/// `pi update --self`). Used by the freshness pass as the update command
54-
/// for a [`InstallSource::CurlPipe`] main readout, where registry-derived
55-
/// recipes would target the wrong prefix (Pi's installer lays down an
56-
/// npm-shaped tree under `~/.local` that a plain `npm install -g` can't
57-
/// reach) but the tool's own updater re-installs in place. Unlike
58-
/// Cursor/Amp-style background auto-updaters, a declared self-update
59-
/// command marks the install as user-actioned: the update nag stays.
60-
pub self_update_command: Option<&'static str>,
6152
}
6253

6354
/// All AI agents we check for individually.
@@ -75,7 +66,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
7566
auth_status_command: None,
7667
install_source_override: None,
7768
bundled_version_args: None,
78-
self_update_command: None,
7969
},
8070
// The claude-agent-acp bridge vendors the complete Claude Code CLI and
8171
// forwards `--cli <args>` to it, sharing the user's credential store
@@ -94,7 +84,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
9484
auth_status_command: Some("claude-agent-acp --cli auth status"),
9585
install_source_override: None,
9686
bundled_version_args: Some(&["--cli", "--version"]),
97-
self_update_command: None,
9887
},
9988
// The codex-acp bridge vendors the full `codex` binary and forwards
10089
// `cli <args>` to it, sharing the user's ~/.codex/auth.json — same
@@ -115,7 +104,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
115104
// `--version` anywhere in argv and prints its own version, so only
116105
// codex's clap short flag reaches the vendored binary.
117106
bundled_version_args: Some(&["cli", "-V"]),
118-
self_update_command: None,
119107
},
120108
// Pi (pi.dev, github.com/earendil-works/pi) is npm-under-the-hood in every
121109
// install method: `npm install -g --ignore-scripts` (its docs and its
@@ -136,12 +124,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
136124
auth_status_command: None,
137125
install_source_override: None,
138126
bundled_version_args: None,
139-
// Pi's curl installer (pi.dev/install.sh) falls back to an npm-shaped
140-
// tree under `~/.local` when the global npm prefix isn't writable;
141-
// `pi update --self` re-installs in place for every layout (it infers
142-
// its own prefix/package manager), where `npm install -g …@latest`
143-
// would target the wrong prefix.
144-
self_update_command: Some("pi update --self"),
145127
},
146128
AgentCheckInfo {
147129
id: "ai-agent-amp",
@@ -157,7 +139,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
157139
// Main `amp` curl installer; bridge `amp-acp` is npm (detected positively).
158140
install_source_override: Some(InstallSource::CurlPipe),
159141
bundled_version_args: None,
160-
self_update_command: None,
161142
},
162143
AgentCheckInfo {
163144
id: "ai-agent-copilot",
@@ -172,7 +153,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
172153
auth_status_command: None,
173154
install_source_override: None,
174155
bundled_version_args: None,
175-
self_update_command: None,
176156
},
177157
AgentCheckInfo {
178158
id: "ai-agent-cursor",
@@ -189,7 +169,6 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[
189169
// resolved binary is the curl install — the override's primary use case.
190170
install_source_override: Some(InstallSource::CurlPipe),
191171
bundled_version_args: None,
192-
self_update_command: None,
193172
},
194173
];
195174

@@ -218,17 +197,36 @@ pub(crate) fn bundled_version_probe_args(
218197
/// sources with no canonical update recipe (`Mise`/`Asdf`/`Unknown`/`System`),
219198
/// or when the package id is unknown.
220199
///
200+
/// `npm_prefix` is the prefix that owns the binary being updated (see
201+
/// [`crate::resolve::npm_prefix_for_binary`]). npm installs go to whatever
202+
/// prefix npm is *configured* with, which is not always the one the resolved
203+
/// binary lives in — a package installed with an explicit `--prefix`, or under a
204+
/// node version the user has since switched away from, would otherwise be
205+
/// "updated" by installing a second copy somewhere else while the stale binary
206+
/// keeps resolving. Passing the derived prefix back through pins the update to
207+
/// the install it is about; where the two agree — the common case — the flag is
208+
/// a no-op. `None` falls back to the bare command.
209+
///
221210
/// The caller is responsible for gating on `update_available == Some(true)` —
222211
/// this function only knows how to update, not whether to. `apply_npm_registry`
223212
/// runs over the final command string downstream, so npm commands automatically
224213
/// pick up a registry override when one is configured.
225214
pub fn derive_update_command(
226215
install_source: Option<&InstallSource>,
227216
package_id: Option<&str>,
217+
npm_prefix: Option<&Path>,
228218
) -> Option<String> {
229219
let pkg = package_id?;
230220
match install_source? {
231-
InstallSource::Npm => Some(format!("npm install -g {pkg}@latest")),
221+
InstallSource::Npm => Some(match npm_prefix {
222+
// Quoted: this is executed via `sh -c`, and `/Users/Mary Smith` is a
223+
// real home directory.
224+
Some(prefix) => format!(
225+
"npm install -g --prefix {} {pkg}@latest",
226+
shell_quote(&prefix.to_string_lossy()),
227+
),
228+
None => format!("npm install -g {pkg}@latest"),
229+
}),
232230
InstallSource::Pnpm => Some(format!("pnpm add -g {pkg}@latest")),
233231
InstallSource::Bun => Some(format!("bun add -g {pkg}@latest")),
234232
InstallSource::Brew => Some(format!("brew upgrade {pkg}")),
@@ -242,17 +240,6 @@ pub fn derive_update_command(
242240
}
243241
}
244242

245-
/// The agent's declared self-update command for its main CLI (e.g. Pi's
246-
/// `pi update --self`), when one exists. Consulted by the freshness pass for
247-
/// [`InstallSource::CurlPipe`] main readouts — see
248-
/// [`AgentCheckInfo::self_update_command`].
249-
pub(crate) fn agent_self_update_command(check_id: &str) -> Option<&'static str> {
250-
AI_AGENT_CHECKS
251-
.iter()
252-
.find(|info| info.id == check_id)
253-
.and_then(|info| info.self_update_command)
254-
}
255-
256243
/// Append `--registry=<url>` to `command` when a registry override is supplied
257244
/// and the command is npm-backed. Non-npm commands (curl-pipe installers, auth
258245
/// commands, …) and the `None` registry case return the command unchanged.
@@ -770,10 +757,6 @@ mod tests {
770757
}
771758
}
772759

773-
fn shell_quote(value: &str) -> String {
774-
format!("'{}'", value.replace('\'', "'\\''"))
775-
}
776-
777760
fn write_login_path_rewrite_profiles(home: &Path, path: &Path) {
778761
let profile = format!(
779762
"export PATH={}\n",
@@ -1092,10 +1075,10 @@ mod tests {
10921075
}
10931076

10941077
/// Pi's registry entry: npm-shaped install commands for both binaries (so
1095-
/// the registry override applies), no auth commands (Pi owns its own
1096-
/// provider/model configuration), and its own updater for curl installs.
1078+
/// the registry override applies) and no auth commands (Pi owns its own
1079+
/// provider/model configuration).
10971080
#[test]
1098-
fn pi_declares_install_bridge_and_self_update_commands() {
1081+
fn pi_declares_install_and_bridge_commands() {
10991082
let pi = agent("ai-agent-pi");
11001083
assert_eq!(pi.main_command, Some("pi"));
11011084
assert_eq!(pi.commands, &["pi-acp"]);
@@ -1112,7 +1095,6 @@ mod tests {
11121095
assert_eq!(pi.auth_command, None);
11131096
assert_eq!(pi.auth_status_command, None);
11141097
assert_eq!(pi.install_source_override, None);
1115-
assert_eq!(pi.self_update_command, Some("pi update --self"));
11161098
}
11171099

11181100
/// Main CLI present, bridge missing → `FixType::Bridge` with the
@@ -1170,30 +1152,19 @@ mod tests {
11701152
assert!(check.bridge_path.is_none());
11711153
}
11721154

1173-
#[test]
1174-
fn self_update_command_lookup_only_for_declaring_agents() {
1175-
assert_eq!(
1176-
agent_self_update_command("ai-agent-pi"),
1177-
Some("pi update --self"),
1178-
);
1179-
// Cursor/Amp curl installs auto-update in the background — they keep
1180-
// the self-updating suppression, not a user-actioned updater.
1181-
assert_eq!(agent_self_update_command("ai-agent-cursor"), None);
1182-
assert_eq!(agent_self_update_command("ai-agent-amp"), None);
1183-
}
1184-
11851155
#[test]
11861156
fn derive_update_command_pnpm_and_bun_emit_add_g_latest() {
11871157
assert_eq!(
11881158
derive_update_command(
11891159
Some(&InstallSource::Pnpm),
11901160
Some("@earendil-works/pi-coding-agent"),
1161+
None,
11911162
)
11921163
.as_deref(),
11931164
Some("pnpm add -g @earendil-works/pi-coding-agent@latest"),
11941165
);
11951166
assert_eq!(
1196-
derive_update_command(Some(&InstallSource::Bun), Some("pi-acp")).as_deref(),
1167+
derive_update_command(Some(&InstallSource::Bun), Some("pi-acp"), None).as_deref(),
11971168
Some("bun add -g pi-acp@latest"),
11981169
);
11991170
}
@@ -1301,30 +1272,90 @@ mod tests {
13011272
);
13021273
}
13031274

1275+
/// Without a derived prefix the npm recipe stays bare — today's behaviour,
1276+
/// and the fail-safe when the package tree can't be located.
13041277
#[test]
1305-
fn derive_update_command_npm_emits_at_latest() {
1278+
fn derive_update_command_npm_falls_back_to_bare_without_prefix() {
13061279
assert_eq!(
13071280
derive_update_command(
13081281
Some(&InstallSource::Npm),
13091282
Some("@agentclientprotocol/claude-agent-acp"),
1283+
None,
13101284
)
13111285
.as_deref(),
13121286
Some("npm install -g @agentclientprotocol/claude-agent-acp@latest"),
13131287
);
13141288
}
13151289

1290+
/// With a prefix, the recipe pins the install to it — so an agent under
1291+
/// `~/.local` is upgraded there instead of a second copy appearing in
1292+
/// whichever prefix npm is configured with.
1293+
#[test]
1294+
fn derive_update_command_npm_emits_prefix() {
1295+
assert_eq!(
1296+
derive_update_command(
1297+
Some(&InstallSource::Npm),
1298+
Some("@earendil-works/pi-coding-agent"),
1299+
Some(Path::new("/Users/test/.local")),
1300+
)
1301+
.as_deref(),
1302+
Some(
1303+
"npm install -g --prefix '/Users/test/.local' \
1304+
@earendil-works/pi-coding-agent@latest"
1305+
),
1306+
);
1307+
}
1308+
1309+
/// The command is handed to `sh -c`, so a prefix with a space in it (a real
1310+
/// home directory shape) must survive quoting intact.
1311+
#[test]
1312+
fn derive_update_command_npm_quotes_prefix_containing_spaces() {
1313+
assert_eq!(
1314+
derive_update_command(
1315+
Some(&InstallSource::Npm),
1316+
Some("pi-acp"),
1317+
Some(Path::new("/Users/Mary Smith/.local")),
1318+
)
1319+
.as_deref(),
1320+
Some("npm install -g --prefix '/Users/Mary Smith/.local' pi-acp@latest"),
1321+
);
1322+
}
1323+
1324+
/// A prefixed npm recipe is still recognised as npm-backed, so a configured
1325+
/// registry override lands on it.
1326+
#[test]
1327+
fn derive_update_command_npm_with_prefix_still_takes_registry_override() {
1328+
let command = derive_update_command(
1329+
Some(&InstallSource::Npm),
1330+
Some("@earendil-works/pi-coding-agent"),
1331+
Some(Path::new("/Users/test/.local")),
1332+
)
1333+
.expect("npm recipe");
1334+
assert_eq!(
1335+
apply_npm_registry(&command, Some("https://artifactory/npm")),
1336+
"npm install -g --prefix '/Users/test/.local' \
1337+
@earendil-works/pi-coding-agent@latest --registry=https://artifactory/npm",
1338+
);
1339+
}
1340+
1341+
/// Non-npm sources ignore the prefix entirely.
13161342
#[test]
13171343
fn derive_update_command_brew_emits_upgrade() {
13181344
assert_eq!(
1319-
derive_update_command(Some(&InstallSource::Brew), Some("codex")).as_deref(),
1345+
derive_update_command(
1346+
Some(&InstallSource::Brew),
1347+
Some("codex"),
1348+
Some(Path::new("/Users/test/.local")),
1349+
)
1350+
.as_deref(),
13201351
Some("brew upgrade codex"),
13211352
);
13221353
}
13231354

13241355
#[test]
13251356
fn derive_update_command_cargo_emits_install_force() {
13261357
assert_eq!(
1327-
derive_update_command(Some(&InstallSource::Cargo), Some("some-crate")).as_deref(),
1358+
derive_update_command(Some(&InstallSource::Cargo), Some("some-crate"), None).as_deref(),
13281359
Some("cargo install --force some-crate"),
13291360
);
13301361
}
@@ -1340,7 +1371,7 @@ mod tests {
13401371
InstallSource::System,
13411372
] {
13421373
assert_eq!(
1343-
derive_update_command(Some(&src), Some("pkg")),
1374+
derive_update_command(Some(&src), Some("pkg"), None),
13441375
None,
13451376
"expected None for {src:?}",
13461377
);
@@ -1349,7 +1380,10 @@ mod tests {
13491380

13501381
#[test]
13511382
fn derive_update_command_returns_none_without_package_id() {
1352-
assert_eq!(derive_update_command(Some(&InstallSource::Npm), None), None,);
1383+
assert_eq!(
1384+
derive_update_command(Some(&InstallSource::Npm), None, None),
1385+
None,
1386+
);
13531387
}
13541388

13551389
#[test]

0 commit comments

Comments
 (0)