From 0bc79bf80b42a309a1eda0cc51c6259e021d9f6f Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 15:32:06 +1000 Subject: [PATCH 01/18] feat(doctor): add auth/version/install-source fields to DoctorCheck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the doctor reunification plan — purely additive schema changes so downstream consumers can begin depending on the new fields ahead of Phase 2's behavior changes. - Add AuthStatus and InstallSource enums. - Add Auth variant to FixType (no-op match arm in lookup_fix_command). - Extend DoctorCheck with auth_status, installed_version, latest_version, update_available, install_source — all Option-defaulting-None. - Extend ResolvedBinary with install_source (Option, default None). - Extend AgentCheckInfo with auth_command and auth_status_command, initialized to None for every existing AI_AGENT_CHECKS entry. The staged Tauri app compiles unchanged because every new field is Option<...> defaulting to None — no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 45 +++++++++++++++ crates/doctor/src/checks.rs | 105 +++++++++++++++++++++++++++++++++++ crates/doctor/src/lib.rs | 6 ++ crates/doctor/src/resolve.rs | 3 + crates/doctor/src/types.rs | 36 ++++++++++++ 5 files changed, 195 insertions(+) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index eccfa4cd8..527ce7cd7 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -24,6 +24,10 @@ pub struct AgentCheckInfo { pub bridge_install_url: Option<&'static str>, /// Shell command to install the ACP bridge (used as fix_command for partial installs). pub bridge_install_command: Option<&'static str>, + /// Shell command that triggers an interactive authentication flow for this agent. + pub auth_command: Option<&'static str>, + /// Shell command that reports whether the user is currently authenticated. + pub auth_status_command: Option<&'static str>, } /// All AI agents we check for individually. @@ -37,6 +41,8 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: None, bridge_install_url: None, bridge_install_command: None, + auth_command: None, + auth_status_command: None, }, AgentCheckInfo { id: "ai-agent-claude", @@ -47,6 +53,8 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: Some("curl -fsSL https://claude.ai/install.sh | bash"), bridge_install_url: Some("https://github.com/zed-industries/claude-agent-acp#installation"), bridge_install_command: Some("npm install -g @zed-industries/claude-agent-acp"), + auth_command: None, + auth_status_command: None, }, AgentCheckInfo { id: "ai-agent-codex", @@ -57,6 +65,8 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: Some("brew install --cask codex"), bridge_install_url: Some("https://github.com/zed-industries/codex-acp#installation"), bridge_install_command: Some("npm install -g @zed-industries/codex-acp"), + auth_command: None, + auth_status_command: None, }, AgentCheckInfo { id: "ai-agent-pi", @@ -67,6 +77,8 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: None, bridge_install_url: None, bridge_install_command: None, + auth_command: None, + auth_status_command: None, }, AgentCheckInfo { id: "ai-agent-amp", @@ -77,6 +89,8 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: Some("curl -fsSL https://ampcode.com/install.sh | bash"), bridge_install_url: Some("https://www.npmjs.com/package/amp-acp"), bridge_install_command: Some("npm install -g amp-acp"), + auth_command: None, + auth_status_command: None, }, ]; @@ -122,6 +136,11 @@ pub fn check_single_ai_agent( path: resolved_path, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Ok(output) => { @@ -142,6 +161,11 @@ pub fn check_single_ai_agent( path: resolved_path, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Err(e) => DoctorCheck { @@ -157,6 +181,11 @@ pub fn check_single_ai_agent( raw_output: Some(format!( "{header}\n$ goose acp --help\nerror: {e}\n{search}" )), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }, } } else { @@ -179,6 +208,11 @@ pub fn check_single_ai_agent( path: main_path, bridge_path, raw_output: Some(format!("{header}\n{search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } } else { @@ -204,6 +238,11 @@ pub fn check_single_ai_agent( path: Some(main_path.to_string_lossy().to_string()), bridge_path: None, raw_output: Some(format!("{header}\n{search}\n{main_search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }; } @@ -228,6 +267,11 @@ pub fn check_single_ai_agent( path: None, bridge_path: None, raw_output: Some(format!("{header}\n{search}{extra_search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } } @@ -247,6 +291,7 @@ pub fn lookup_fix_command(check_id: &str, fix_type: &FixType) -> Option return match fix_type { FixType::Command => info.install_command.map(|s| s.to_string()), FixType::Bridge => info.bridge_install_command.map(|s| s.to_string()), + FixType::Auth => None, }; } } diff --git a/crates/doctor/src/checks.rs b/crates/doctor/src/checks.rs index 7aff2e3d1..6d17e2cef 100644 --- a/crates/doctor/src/checks.rs +++ b/crates/doctor/src/checks.rs @@ -29,6 +29,11 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(format!("{header}\nnot found via resolve_binary\n{search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }; } }; @@ -53,6 +58,11 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { path: Some(path_str), bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Ok(output) => { @@ -72,6 +82,11 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { path: Some(path_str), bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Err(e) => DoctorCheck { @@ -85,6 +100,11 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { path: Some(path_str), bridge_path: None, raw_output: Some(format!("{header}\n$ git --version\nerror: {e}\n{search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }, } } @@ -110,6 +130,11 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(format!("{header}\nnot found via resolve_binary\n{search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }; } }; @@ -135,6 +160,11 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { path: Some(path_str), bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Ok(output) => { @@ -154,6 +184,11 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { path: Some(path_str), bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Err(e) => DoctorCheck { @@ -167,6 +202,11 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { path: Some(path_str), bridge_path: None, raw_output: Some(format!("{header}\n$ gh --version\nerror: {e}\n{search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }, } } @@ -191,6 +231,11 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(format!("{header}\ngh not found via resolve_binary")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }; } }; @@ -213,6 +258,11 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } else { let stderr = String::from_utf8_lossy(&output.stderr); @@ -233,6 +283,11 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } } @@ -247,6 +302,11 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(format!("{header}\n$ gh auth status\nerror: {e}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }, } } @@ -275,6 +335,11 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh raw_output: Some(format!( "{header}\ngit not found via resolve_binary\n{search}" )), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }; } }; @@ -302,6 +367,11 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh path, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Ok(output) => { @@ -321,6 +391,11 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh path: None, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Err(e) => DoctorCheck { @@ -334,6 +409,11 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh path: None, bridge_path: None, raw_output: Some(format!("{header}\n$ git lfs version\nerror: {e}\n{search}")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }, } } @@ -359,6 +439,11 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(format!("{header}\ngit not found via resolve_binary")), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }; } }; @@ -387,6 +472,11 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } else { DoctorCheck { @@ -401,6 +491,11 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } } @@ -421,6 +516,11 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { path: None, bridge_path: None, raw_output: Some(raw), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } Err(e) => DoctorCheck { @@ -436,6 +536,11 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { raw_output: Some(format!( "{header}\n$ git config --global core.clonefile\nerror: {e}" )), + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, }, } } diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index b3608e736..7f4425889 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -31,6 +31,11 @@ fn empty_check(id: &str, label: &str) -> DoctorCheck { path: None, bridge_path: None, raw_output: None, + auth_status: None, + installed_version: None, + latest_version: None, + update_available: None, + install_source: None, } } @@ -65,6 +70,7 @@ pub async fn run_checks() -> DoctorReport { let fallback = ResolvedBinary { path: None, search_output: "resolution task panicked".to_string(), + install_source: None, }; let r_git = resolved .get("git") diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 89482f4d7..83c1b43fe 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -33,6 +33,7 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { return ResolvedBinary { path: Some(path.clone()), search_output: lines.join("\n"), + install_source: None, }; } @@ -70,6 +71,7 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { return ResolvedBinary { path: Some(path), search_output: lines.join("\n"), + install_source: None, }; } lines.push(format!(" {} => not found", path.display())); @@ -79,6 +81,7 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { ResolvedBinary { path: None, search_output: lines.join("\n"), + install_source: None, } } diff --git a/crates/doctor/src/types.rs b/crates/doctor/src/types.rs index 24981b72c..93c8796fa 100644 --- a/crates/doctor/src/types.rs +++ b/crates/doctor/src/types.rs @@ -20,6 +20,31 @@ pub enum FixType { Command, /// A shell command to install the ACP bridge binary. Bridge, + /// A shell command that triggers an authentication flow. + Auth, +} + +/// Authentication state for a check that probes credentials. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub enum AuthStatus { + Authenticated, + NotAuthenticated, + NotApplicable, +} + +/// How a binary was installed on disk. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub enum InstallSource { + Brew, + Npm, + Cargo, + Mise, + Asdf, + CurlPipe, + System, + Unknown, } /// A single health-check result shown in the UI. @@ -48,6 +73,16 @@ pub struct DoctorCheck { /// Raw debug output: command stdout/stderr, search paths tried, etc. /// Used by the "Copy details" feature for support diagnostics. pub raw_output: Option, + /// Authentication status, when the check probes credentials. + pub auth_status: Option, + /// Installed version string, if detected. + pub installed_version: Option, + /// Latest available version string, if known. + pub latest_version: Option, + /// Whether a newer version is available than the installed one. + pub update_available: Option, + /// How the binary was installed (brew, npm, cargo, etc.), if detected. + pub install_source: Option, } /// The full report returned to the frontend. @@ -62,4 +97,5 @@ pub struct DoctorReport { pub struct ResolvedBinary { pub path: Option, pub search_output: String, + pub install_source: Option, } From 68bbbda87c05af0bdb0d38c3cf83a71fb69b09a8 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 15:38:56 +1000 Subject: [PATCH 02/18] feat(doctor): add copilot/cursor agents and wire auth commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of the doctor reunification plan — data-only widening of the AI agent table. Switch the Claude bridge to its canonical npm package name `@agentclientprotocol/claude-agent-acp` (the `@zed-industries/...` package is deprecated/renamed, not a competing fork). Add ai-agent-copilot and ai-agent-cursor entries, and populate auth_command / auth_status_command for Claude, Codex, Amp, Copilot, and Cursor. The runtime does not yet read these — Phase 3 wires them. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index 527ce7cd7..bab8d4040 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -52,9 +52,9 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_url: Some("https://code.claude.com/docs/en/overview"), install_command: Some("curl -fsSL https://claude.ai/install.sh | bash"), bridge_install_url: Some("https://github.com/zed-industries/claude-agent-acp#installation"), - bridge_install_command: Some("npm install -g @zed-industries/claude-agent-acp"), - auth_command: None, - auth_status_command: None, + bridge_install_command: Some("npm install -g @agentclientprotocol/claude-agent-acp"), + auth_command: Some("claude /login"), + auth_status_command: Some("claude auth status"), }, AgentCheckInfo { id: "ai-agent-codex", @@ -65,8 +65,8 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: Some("brew install --cask codex"), bridge_install_url: Some("https://github.com/zed-industries/codex-acp#installation"), bridge_install_command: Some("npm install -g @zed-industries/codex-acp"), - auth_command: None, - auth_status_command: None, + auth_command: Some("codex login"), + auth_status_command: Some("codex login status"), }, AgentCheckInfo { id: "ai-agent-pi", @@ -89,9 +89,33 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: Some("curl -fsSL https://ampcode.com/install.sh | bash"), bridge_install_url: Some("https://www.npmjs.com/package/amp-acp"), bridge_install_command: Some("npm install -g amp-acp"), - auth_command: None, + auth_command: Some("amp login"), + auth_status_command: Some("amp usage"), + }, + AgentCheckInfo { + id: "ai-agent-copilot", + label: "GitHub Copilot", + commands: &["copilot"], + main_command: None, + install_url: Some("https://github.com/github/copilot-cli"), + install_command: Some("npm install -g @github/copilot"), + bridge_install_url: None, + bridge_install_command: None, + auth_command: Some("copilot login"), auth_status_command: None, }, + AgentCheckInfo { + id: "ai-agent-cursor", + label: "Cursor Agent", + commands: &["cursor-agent"], + main_command: None, + install_url: Some("https://cursor.com/"), + install_command: Some("curl -fsSL https://cursor.com/install | bash"), + bridge_install_url: None, + bridge_install_command: None, + auth_command: Some("cursor-agent login"), + auth_status_command: Some("cursor-agent status"), + }, ]; /// Check whether a single AI agent is installed. From 0aec1268cc8061fdf1344e62760fee7001572f9e Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 15:53:28 +1000 Subject: [PATCH 03/18] fix(doctor): align Claude auth_command with goose-internal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 follow-up — switch Claude's auth_command from `claude /login` to `claude auth login` so the string is byte-equivalent to goose-internal's agent_setup.rs, keeping Phase 6 reunification a true no-op rather than a behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index bab8d4040..18f827dd7 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -53,7 +53,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ install_command: Some("curl -fsSL https://claude.ai/install.sh | bash"), bridge_install_url: Some("https://github.com/zed-industries/claude-agent-acp#installation"), bridge_install_command: Some("npm install -g @agentclientprotocol/claude-agent-acp"), - auth_command: Some("claude /login"), + auth_command: Some("claude auth login"), auth_status_command: Some("claude auth status"), }, AgentCheckInfo { From 4795a68d6da3a67d3bdf8fc70122fa8ccc2e32ff Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 15:57:14 +1000 Subject: [PATCH 04/18] feat(doctor): probe AI agent auth status and dispatch auth fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3a of the doctor reunification plan — runtime auth probing. For auth-aware agents, bridge presence is now necessary-but-not-sufficient for Pass: a failed auth probe downgrades the check to Warn with AuthStatus::NotAuthenticated, and surfaces the agent's auth_command via fix_type/fix_command so the UI can offer a one-click fix. Agents with an auth_command but no auth_status_command (copilot) report NotApplicable; agents with neither (goose, pi) keep auth_status = None. execute_fix(_, FixType::Auth) is now wired through lookup_fix_command, which returns the matching agent's auth_command for the new arm. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 73 +++++++++++++++++++++++++++++++++---- crates/doctor/src/lib.rs | 65 ++++++++++++++++++--------------- 2 files changed, 100 insertions(+), 38 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index 18f827dd7..24545caaa 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -3,8 +3,9 @@ use std::process::Command; use crate::checks::CLONEFILE_FIX_COMMAND; +use crate::execute_command_blocking; use crate::resolve::format_command_output; -use crate::types::{CheckStatus, DoctorCheck, FixType, ResolvedBinary}; +use crate::types::{AuthStatus, CheckStatus, DoctorCheck, FixType, ResolvedBinary}; /// Metadata for an individual AI agent check. pub struct AgentCheckInfo { @@ -221,18 +222,52 @@ pub fn check_single_ai_agent( } else { (resolved_path, None) }; + + let (auth_status, auth_output) = match info.auth_status_command { + Some(cmd) => match execute_command_blocking(cmd) { + Ok(()) => ( + Some(AuthStatus::Authenticated), + Some(format!("$ {cmd}\n(exit 0)")), + ), + Err(err) => ( + Some(AuthStatus::NotAuthenticated), + Some(format!("$ {cmd}\n{err}")), + ), + }, + None if info.auth_command.is_some() => (Some(AuthStatus::NotApplicable), None), + None => (None, None), + }; + + let mut raw = format!("{header}\n{search}"); + if let Some(extra) = auth_output { + raw.push('\n'); + raw.push_str(&extra); + } + + let (status, message, fix_type, fix_command) = + if matches!(auth_status, Some(AuthStatus::NotAuthenticated)) { + ( + CheckStatus::Warn, + "Installed, not authenticated".to_string(), + info.auth_command.map(|_| FixType::Auth), + info.auth_command.map(|s| s.to_string()), + ) + } else { + (CheckStatus::Pass, "Installed".to_string(), None, None) + }; + DoctorCheck { id: info.id.to_string(), label: info.label.to_string(), - status: CheckStatus::Pass, - message: "Installed".to_string(), + status, + message, fix_url: None, - fix_command: None, - fix_type: None, + fix_command, + fix_type, path: main_path, bridge_path, - raw_output: Some(format!("{header}\n{search}")), - auth_status: None, + raw_output: Some(raw), + auth_status, installed_version: None, latest_version: None, update_available: None, @@ -315,10 +350,32 @@ pub fn lookup_fix_command(check_id: &str, fix_type: &FixType) -> Option return match fix_type { FixType::Command => info.install_command.map(|s| s.to_string()), FixType::Bridge => info.bridge_install_command.map(|s| s.to_string()), - FixType::Auth => None, + FixType::Auth => info.auth_command.map(|s| s.to_string()), }; } } None } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn auth_fix_lookup_returns_agent_auth_command() { + assert_eq!( + lookup_fix_command("ai-agent-claude", &FixType::Auth).as_deref(), + Some("claude auth login"), + ); + assert_eq!( + lookup_fix_command("ai-agent-copilot", &FixType::Auth).as_deref(), + Some("copilot login"), + ); + } + + #[test] + fn auth_fix_lookup_returns_none_for_agents_without_auth_command() { + assert_eq!(lookup_fix_command("ai-agent-goose", &FixType::Auth), None); + } +} diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index 7f4425889..987c8da3d 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -164,35 +164,40 @@ pub async fn execute_fix(check_id: String, fix_type: FixType) -> Result<(), Stri /// Internal primitive used by [`execute_fix`]. Not exposed publicly — callers /// should use `execute_fix` which looks up the command from static definitions. pub(crate) async fn execute_command(command: String) -> Result<(), String> { - tokio::task::spawn_blocking(move || { - let (shell, args) = if std::path::Path::new("/bin/zsh").exists() { - ("/bin/zsh", vec!["-l", "-c", &command]) - } else { - ("/bin/bash", vec!["-l", "-c", &command]) - }; - let home = std::env::var("HOME").unwrap_or_else(|_| "/".to_string()); - let user = std::env::var("USER").unwrap_or_default(); - let output = std::process::Command::new(shell) - .args(&args) - .env_clear() - .env("HOME", &home) - .env("USER", &user) - .env("TERM", "xterm-256color") - .current_dir(&home) - .output() - .map_err(|e| format!("Failed to run command: {e}"))?; - - if output.status.success() { - Ok(()) + tokio::task::spawn_blocking(move || execute_command_blocking(&command)) + .await + .unwrap_or_else(|e| Err(format!("Task failed: {e}"))) +} + +/// Synchronous twin of [`execute_command`] for use inside `spawn_blocking` +/// closures (e.g. per-check auth probes that run in the existing check +/// parallelism). +pub(crate) fn execute_command_blocking(command: &str) -> Result<(), String> { + let (shell, args) = if std::path::Path::new("/bin/zsh").exists() { + ("/bin/zsh", vec!["-l", "-c", command]) + } else { + ("/bin/bash", vec!["-l", "-c", command]) + }; + let home = std::env::var("HOME").unwrap_or_else(|_| "/".to_string()); + let user = std::env::var("USER").unwrap_or_default(); + let output = std::process::Command::new(shell) + .args(&args) + .env_clear() + .env("HOME", &home) + .env("USER", &user) + .env("TERM", "xterm-256color") + .current_dir(&home) + .output() + .map_err(|e| format!("Failed to run command: {e}"))?; + + if output.status.success() { + Ok(()) + } else { + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + Err(if stderr.is_empty() { + format!("Command failed with exit code {}", output.status) } else { - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - Err(if stderr.is_empty() { - format!("Command failed with exit code {}", output.status) - } else { - stderr - }) - } - }) - .await - .unwrap_or_else(|e| Err(format!("Task failed: {e}"))) + stderr + }) + } } From 5ade34edfdd751d87e20256fb0b4671e4b06f693 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 16:03:43 +1000 Subject: [PATCH 05/18] feat(doctor): resolve binaries from npm global dirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3b of the doctor reunification plan — closes a binary-presence resolution gap so doctor sees what goose-internal's ACP inventory sees. Adds a third resolve strategy after login-shell `which` and the hardcoded /opt/homebrew, /usr/local, etc. list: ~/.npm-global/bin, ~/.npm/bin, all ~/.nvm/versions/node/*/bin entries, the macOS Homebrew nvm symlink at /opt/homebrew/opt/nvm/versions/node/*/bin, and finally `npm prefix -g`/bin as a one-subprocess last resort. Mirrors the dirs in goose-internal's services::path_env (and upstream goose's SearchPaths::with_npm) so npm-only bridge installs no longer surface as false "agent missing" warnings. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/resolve.rs | 143 +++++++++++++++++++++++++++++++++-- 1 file changed, 138 insertions(+), 5 deletions(-) diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 83c1b43fe..0fb743050 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -5,7 +5,8 @@ use std::process::Command; use super::types::ResolvedBinary; -/// Resolve a binary by trying login shell path lookup then common install paths. +/// Resolve a binary by trying login shell path lookup, common install paths, +/// then npm global install dirs. pub fn resolve_binary(cmd: &str) -> ResolvedBinary { let mut lines = vec![format!("resolve '{cmd}':")]; @@ -57,8 +58,8 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { } } - // Strategy 2: Common install paths (fallback) - lines.push(" strategy 2 — common install paths (fallback):".to_string()); + // Strategy 2: Common install paths (fallback). + lines.push(" strategy 2 — common install paths:".to_string()); for dir in &[ "/opt/homebrew/bin", "/usr/local/bin", @@ -77,6 +78,49 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { lines.push(format!(" {} => not found", path.display())); } + // Strategy 3: npm global install dirs. + // + // Mirrors the dirs added by goose-internal's `services::path_env` (and the + // upstream goose `config::search_path::SearchPaths::with_npm`) so that + // bridges installed via `npm install -g` resolve here too. Without this, + // npm-only installs are "found" by goose-internal's ACP inventory but + // reported missing by doctor. + lines.push(" strategy 3 — npm global install dirs:".to_string()); + let home = std::env::home_dir(); + if let Some(home) = home.as_deref() { + for dir in npm_search_dirs(home) { + let path = dir.join(cmd); + if path.exists() { + lines.push(format!(" {} => found (resolved)", path.display())); + return ResolvedBinary { + path: Some(path), + search_output: lines.join("\n"), + install_source: None, + }; + } + lines.push(format!(" {} => not found", path.display())); + } + } else { + lines.push(" (could not determine HOME)".to_string()); + } + + // Strategy 3 last resort: ask npm itself (the most authoritative answer, + // but costs one subprocess — only invoked when the static probes above + // didn't find the binary). `npm prefix -g` is the version-stable + // equivalent of the older `npm bin -g`; the bin dir is `/bin`. + if let Some(npm_bin_dir) = npm_global_bin_dir(&mut lines) { + let path = npm_bin_dir.join(cmd); + if path.exists() { + lines.push(format!(" {} => found (resolved)", path.display())); + return ResolvedBinary { + path: Some(path), + search_output: lines.join("\n"), + install_source: None, + }; + } + lines.push(format!(" {} => not found", path.display())); + } + lines.push(" not found in any location".to_string()); ResolvedBinary { path: None, @@ -136,6 +180,54 @@ fn summarize_output(output: &str) -> String { format!("{}...", summary.replace('\n', "\\n")) } +/// Candidate npm global install dirs to probe for a given $HOME. +/// +/// Includes the standard per-user dirs plus all detected nvm node versions +/// (both `~/.nvm` and Homebrew's `/opt/homebrew/opt/nvm` layout). +fn npm_search_dirs(home: &Path) -> Vec { + let mut dirs = vec![home.join(".npm-global/bin"), home.join(".npm/bin")]; + + // ~/.nvm/versions/node/*/bin + for version_dir in read_subdirs(&home.join(".nvm/versions/node")) { + dirs.push(version_dir.join("bin")); + } + + // macOS Homebrew nvm: /opt/homebrew/opt/nvm/versions/node/*/bin + #[cfg(target_os = "macos")] + for version_dir in read_subdirs(Path::new("/opt/homebrew/opt/nvm/versions/node")) { + dirs.push(version_dir.join("bin")); + } + + dirs +} + +fn read_subdirs(parent: &Path) -> Vec { + match std::fs::read_dir(parent) { + Ok(entries) => entries + .filter_map(|e| e.ok()) + .filter(|e| e.file_type().map(|t| t.is_dir()).unwrap_or(false)) + .map(|e| e.path()) + .collect(), + Err(_) => Vec::new(), + } +} + +fn npm_global_bin_dir(lines: &mut Vec) -> Option { + let output = Command::new("npm").args(["prefix", "-g"]).output().ok()?; + if !output.status.success() { + lines.push(" npm prefix -g => failed".to_string()); + return None; + } + let prefix = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if prefix.is_empty() { + lines.push(" npm prefix -g => empty output".to_string()); + return None; + } + let bin = PathBuf::from(prefix).join("bin"); + lines.push(format!(" npm prefix -g => {}", bin.display())); + Some(bin) +} + /// Format the raw output of a command invocation for debug diagnostics. pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> String { let stdout = String::from_utf8_lossy(&output.stdout); @@ -152,9 +244,8 @@ pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> S #[cfg(test)] mod tests { - use super::{candidate_paths_from_shell_output, is_executable_file, shell_quote}; + use super::*; use std::fs::{self, File}; - use std::path::PathBuf; #[test] fn candidate_accepts_single_absolute_path() { @@ -250,4 +341,46 @@ mod tests { assert!(!is_executable_file(&dir)); let _ = fs::remove_dir_all(&dir); } + + #[test] + fn npm_search_dirs_includes_expected_paths_for_fixed_home() { + let home = PathBuf::from("/home/test"); + let dirs = npm_search_dirs(&home); + + assert!( + dirs.iter().any(|p| p == &home.join(".npm-global/bin")), + "missing ~/.npm-global/bin in {dirs:?}" + ); + assert!( + dirs.iter().any(|p| p == &home.join(".npm/bin")), + "missing ~/.npm/bin in {dirs:?}" + ); + } + + #[test] + fn search_output_includes_npm_dirs_when_binary_not_found() { + let resolved = resolve_binary("definitely-not-a-real-binary-xyz-123abc"); + assert!( + resolved.path.is_none(), + "did not expect to find a real binary" + ); + assert!( + resolved + .search_output + .contains("strategy 3 — npm global install dirs"), + "expected strategy 3 marker in search_output:\n{}", + resolved.search_output + ); + if let Some(home) = std::env::home_dir() { + let expected = home.join(".npm-global/bin"); + assert!( + resolved + .search_output + .contains(&expected.display().to_string()), + "expected {} in search_output:\n{}", + expected.display(), + resolved.search_output + ); + } + } } From fc8160caeefe98b86407f922aea4985c8f2a55f9 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 16:10:59 +1000 Subject: [PATCH 06/18] feat(doctor): detect install source from binary path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3c of the doctor reunification plan — populate `InstallSource` on resolved binaries. Path-prefix heuristics only (Brew, Cargo, Mise, Asdf, Npm, System) — no subprocess probes for v1; ambiguous targets like ~/.local/bin curl-pipe installs report Unknown rather than risk a false positive. `detect_install_source` runs on every successful `resolve_binary` hit and the value flows through `ResolvedBinary` into each `DoctorCheck` site (git, gh, git-lfs, clonefile, AI agents — bridge preferred over main). Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 26 ++++-- crates/doctor/src/checks.rs | 22 ++--- crates/doctor/src/resolve.rs | 175 ++++++++++++++++++++++++++++++++++- 3 files changed, 199 insertions(+), 24 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index 24545caaa..cde9562c1 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -136,10 +136,15 @@ pub fn check_single_ai_agent( .collect(); let search = search_lines.join("\n"); - let resolved_path = resolved_cmds - .iter() - .find_map(|rb| rb.path.as_ref()) + let resolved_bridge: Option<&ResolvedBinary> = + resolved_cmds.iter().find(|rb| rb.path.is_some()); + let resolved_path = resolved_bridge + .and_then(|rb| rb.path.as_ref()) .map(|p| p.to_string_lossy().to_string()); + // Prefer the bridge's install_source — that's the binary the UI cares about + // for "how did your acp adapter get installed". For goose (no separate + // main_command) the bridge is the main binary itself, so this is correct. + let bridge_install_source = resolved_bridge.and_then(|rb| rb.install_source.clone()); if let Some(ref path_str) = resolved_path { if info.id == "ai-agent-goose" { @@ -165,7 +170,7 @@ pub fn check_single_ai_agent( installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: bridge_install_source.clone(), } } Ok(output) => { @@ -190,7 +195,7 @@ pub fn check_single_ai_agent( installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: bridge_install_source.clone(), } } Err(e) => DoctorCheck { @@ -210,7 +215,7 @@ pub fn check_single_ai_agent( installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: bridge_install_source.clone(), }, } } else { @@ -271,7 +276,7 @@ pub fn check_single_ai_agent( installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: bridge_install_source, } } } else { @@ -280,6 +285,11 @@ pub fn check_single_ai_agent( if let Some(main_path) = resolved_main.as_ref().and_then(|rm| rm.path.as_ref()) { let bridge_cmd = info.commands[0]; let main_search = &resolved_main.as_ref().unwrap().search_output; + // Bridge wasn't found, so fall back to the main binary's + // install_source — the only resolved path we have to describe. + let main_install_source = resolved_main + .as_ref() + .and_then(|rm| rm.install_source.clone()); return DoctorCheck { id: info.id.to_string(), label: info.label.to_string(), @@ -301,7 +311,7 @@ pub fn check_single_ai_agent( installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: main_install_source, }; } diff --git a/crates/doctor/src/checks.rs b/crates/doctor/src/checks.rs index 6d17e2cef..bc7ef7db4 100644 --- a/crates/doctor/src/checks.rs +++ b/crates/doctor/src/checks.rs @@ -62,7 +62,7 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: resolved.install_source.clone(), } } Ok(output) => { @@ -86,7 +86,7 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: resolved.install_source.clone(), } } Err(e) => DoctorCheck { @@ -104,7 +104,7 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: resolved.install_source.clone(), }, } } @@ -164,7 +164,7 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: resolved.install_source.clone(), } } Ok(output) => { @@ -188,7 +188,7 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: resolved.install_source.clone(), } } Err(e) => DoctorCheck { @@ -206,7 +206,7 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: resolved.install_source.clone(), }, } } @@ -371,7 +371,7 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: git_lfs.install_source.clone(), } } Ok(output) => { @@ -476,7 +476,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: git.install_source.clone(), } } else { DoctorCheck { @@ -495,7 +495,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: git.install_source.clone(), } } } @@ -520,7 +520,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: git.install_source.clone(), } } Err(e) => DoctorCheck { @@ -540,7 +540,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { installed_version: None, latest_version: None, update_available: None, - install_source: None, + install_source: git.install_source.clone(), }, } } diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 0fb743050..99ccb9fa5 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use std::process::Command; -use super::types::ResolvedBinary; +use super::types::{InstallSource, ResolvedBinary}; /// Resolve a binary by trying login shell path lookup, common install paths, /// then npm global install dirs. @@ -31,10 +31,11 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { " {shell} -l -c '{lookup_cmd}' => {} (resolved)", path.display() )); + let install_source = Some(detect_install_source(path)); return ResolvedBinary { path: Some(path.clone()), search_output: lines.join("\n"), - install_source: None, + install_source, }; } @@ -69,10 +70,11 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { let path = PathBuf::from(dir).join(cmd); if is_executable_file(&path) { lines.push(format!(" {} => found (resolved)", path.display())); + let install_source = Some(detect_install_source(&path)); return ResolvedBinary { path: Some(path), search_output: lines.join("\n"), - install_source: None, + install_source, }; } lines.push(format!(" {} => not found", path.display())); @@ -92,10 +94,11 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { let path = dir.join(cmd); if path.exists() { lines.push(format!(" {} => found (resolved)", path.display())); + let install_source = Some(detect_install_source(&path)); return ResolvedBinary { path: Some(path), search_output: lines.join("\n"), - install_source: None, + install_source, }; } lines.push(format!(" {} => not found", path.display())); @@ -112,10 +115,11 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { let path = npm_bin_dir.join(cmd); if path.exists() { lines.push(format!(" {} => found (resolved)", path.display())); + let install_source = Some(detect_install_source(&path)); return ResolvedBinary { path: Some(path), search_output: lines.join("\n"), - install_source: None, + install_source, }; } lines.push(format!(" {} => not found", path.display())); @@ -228,6 +232,74 @@ fn npm_global_bin_dir(lines: &mut Vec) -> Option { Some(bin) } +/// Infer how a binary was installed from its path alone — no subprocess or +/// network probes. Path-prefix heuristics cover Brew, Cargo, Mise, Asdf, Npm +/// (mirroring the dirs in [`npm_search_dirs`]), and the System dirs; anything +/// else (including `~/.local/bin` curl-pipe installs, which can't be +/// fingerprinted reliably from the path) is reported as [`InstallSource::Unknown`]. +pub(crate) fn detect_install_source(path: &Path) -> InstallSource { + let home = std::env::home_dir(); + detect_install_source_with_home(path, home.as_deref()) +} + +/// Testable inner: same logic as [`detect_install_source`] but takes the home +/// directory as a parameter so unit tests can inject a fixed value. +fn detect_install_source_with_home(path: &Path, home: Option<&Path>) -> InstallSource { + // Homebrew-managed nvm — checked before the generic `/opt/homebrew/` + // Brew prefix, since this path is a strict subset of it. + if path.starts_with("/opt/homebrew/opt/nvm/versions/node") { + return InstallSource::Npm; + } + + // Brew (path-prefix). `/usr/local/Cellar` covers Intel-mac brew; if a + // binary appears as `/usr/local/bin/` that's a symlink into Cellar, we + // only follow the chain if `canonicalize` succeeds cheaply. + if path.starts_with("/opt/homebrew/") + || path.starts_with("/usr/local/Cellar/") + || path.starts_with("/home/linuxbrew/.linuxbrew/") + { + return InstallSource::Brew; + } + if path.starts_with("/usr/local/bin/") { + if let Ok(canonical) = path.canonicalize() { + if canonical.starts_with("/usr/local/Cellar/") { + return InstallSource::Brew; + } + } + } + + if let Some(home) = home { + if path.starts_with(home.join(".cargo/bin")) { + return InstallSource::Cargo; + } + if path.starts_with(home.join(".local/share/mise")) || path.starts_with(home.join(".mise")) + { + return InstallSource::Mise; + } + if path.starts_with(home.join(".asdf")) { + return InstallSource::Asdf; + } + if path.starts_with(home.join(".npm-global/bin")) + || path.starts_with(home.join(".npm/bin")) + || path.starts_with(home.join(".nvm/versions/node")) + { + return InstallSource::Npm; + } + } + + // System dirs. Checked last so e.g. a Brew binary surfacing at + // `/usr/local/bin/x` was already classified above. + if path.starts_with("/usr/bin/") + || path.starts_with("/bin/") + || path.starts_with("/usr/sbin/") + || path.starts_with("/sbin/") + { + return InstallSource::System; + } + + InstallSource::Unknown +} + /// Format the raw output of a command invocation for debug diagnostics. pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> String { let stdout = String::from_utf8_lossy(&output.stdout); @@ -357,6 +429,99 @@ mod tests { ); } + #[test] + fn detect_install_source_classifies_brew() { + assert_eq!( + detect_install_source_with_home(Path::new("/opt/homebrew/bin/git"), None), + InstallSource::Brew, + ); + assert_eq!( + detect_install_source_with_home(Path::new("/home/linuxbrew/.linuxbrew/bin/git"), None), + InstallSource::Brew, + ); + } + + #[test] + fn detect_install_source_classifies_system() { + assert_eq!( + detect_install_source_with_home(Path::new("/usr/bin/git"), None), + InstallSource::System, + ); + assert_eq!( + detect_install_source_with_home(Path::new("/bin/sh"), None), + InstallSource::System, + ); + } + + #[test] + fn detect_install_source_classifies_cargo_under_home() { + let home = PathBuf::from("/home/test"); + assert_eq!( + detect_install_source_with_home(&home.join(".cargo/bin/cargo"), Some(home.as_path())), + InstallSource::Cargo, + ); + } + + #[test] + fn detect_install_source_classifies_npm_dirs() { + let home = PathBuf::from("/home/test"); + assert_eq!( + detect_install_source_with_home( + &home.join(".npm-global/bin/foo"), + Some(home.as_path()) + ), + InstallSource::Npm, + ); + assert_eq!( + detect_install_source_with_home( + &home.join(".nvm/versions/node/v20.10.0/bin/foo"), + Some(home.as_path()) + ), + InstallSource::Npm, + ); + assert_eq!( + detect_install_source_with_home( + Path::new("/opt/homebrew/opt/nvm/versions/node/v20.10.0/bin/foo"), + None, + ), + InstallSource::Npm, + ); + } + + #[test] + fn detect_install_source_classifies_mise_and_asdf() { + let home = PathBuf::from("/home/test"); + assert_eq!( + detect_install_source_with_home( + &home.join(".local/share/mise/installs/node/20/bin/foo"), + Some(home.as_path()), + ), + InstallSource::Mise, + ); + assert_eq!( + detect_install_source_with_home( + &home.join(".asdf/installs/nodejs/20/bin/foo"), + Some(home.as_path()), + ), + InstallSource::Asdf, + ); + } + + #[test] + fn detect_install_source_unknown_for_curl_pipe_and_other() { + let home = PathBuf::from("/home/test"); + // ~/.local/bin and ~/bin are common curl-pipe targets but unreliable to + // fingerprint from path alone — Unknown beats false positives. + assert_eq!( + detect_install_source_with_home(&home.join(".local/bin/foo"), Some(home.as_path())), + InstallSource::Unknown, + ); + assert_eq!( + detect_install_source_with_home(Path::new("/tmp/weird/foo"), Some(home.as_path())), + InstallSource::Unknown, + ); + } + #[test] fn search_output_includes_npm_dirs_when_binary_not_found() { let resolved = resolve_binary("definitely-not-a-real-binary-xyz-123abc"); From 3fd83a979bbacd00e65e1a864cdf1948f7660b00 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 16:20:50 +1000 Subject: [PATCH 07/18] feat(doctor): add opt-in version freshness with disk cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3d of the doctor reunification plan — installed/latest version detection gated behind RunChecksOptions::check_freshness. Default run_checks() is unchanged: no version probes, no network, no new subprocess fan-out — the staged Tauri app sees the same latency profile as before this commit. Latest-version sources covered: brew (brew info --json=v2), npm (npm view version), and crates.io (HTTP GET via reqwest). Results are cached under /doctor/freshness.json with a 1-hour TTL; read once at the start of run_checks_with_options, persisted atomically at the end. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- Cargo.lock | 92 +++++- apps/staged/src-tauri/Cargo.lock | 5 + crates/doctor/Cargo.toml | 4 + crates/doctor/src/freshness.rs | 486 +++++++++++++++++++++++++++++++ crates/doctor/src/lib.rs | 145 ++++++++- crates/doctor/src/package_ids.rs | 98 +++++++ 6 files changed, 826 insertions(+), 4 deletions(-) create mode 100644 crates/doctor/src/freshness.rs create mode 100644 crates/doctor/src/package_ids.rs diff --git a/Cargo.lock b/Cargo.lock index 5a5c3ac64..736633219 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -456,6 +456,16 @@ dependencies = [ "unicode-segmentation", ] +[[package]] +name = "core-foundation" +version = "0.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91e195e091a93c46f7102ec7818a2aa394e1e1771c3ab4825963fa03e45afb8f" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "core-foundation" version = "0.10.1" @@ -598,7 +608,11 @@ dependencies = [ name = "doctor" version = "0.1.0" dependencies = [ + "dirs", + "futures", + "reqwest", "serde", + "serde_json", "tokio", ] @@ -620,6 +634,15 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "34aa73646ffb006b8f5147f3dc182bd4bcb190227ce861fc4a4844bf8e3cb2c0" +[[package]] +name = "encoding_rs" +version = "0.8.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75030f3c4f45dafd7586dd6780965a8c7e8e285a5ecb86713e63a79c5b2766f3" +dependencies = [ + "cfg-if", +] + [[package]] name = "equivalent" version = "1.0.2" @@ -669,6 +692,12 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" +[[package]] +name = "fnv" +version = "1.0.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" + [[package]] name = "foldhash" version = "0.1.5" @@ -858,6 +887,25 @@ dependencies = [ "regex-syntax", ] +[[package]] +name = "h2" +version = "0.4.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "171fefbc92fe4a4de27e0698d6a5b392d6a0e333506bc49133760b3bcf948733" +dependencies = [ + "atomic-waker", + "bytes", + "fnv", + "futures-core", + "futures-sink", + "http", + "indexmap", + "slab", + "tokio", + "tokio-util", + "tracing", +] + [[package]] name = "hashbrown" version = "0.15.5" @@ -943,6 +991,7 @@ dependencies = [ "bytes", "futures-channel", "futures-core", + "h2", "http", "http-body", "httparse", @@ -987,9 +1036,11 @@ dependencies = [ "percent-encoding", "pin-project-lite", "socket2", + "system-configuration", "tokio", "tower-service", "tracing", + "windows-registry", ] [[package]] @@ -1771,7 +1822,11 @@ checksum = "ab3f43e3283ab1488b624b44b0e988d0acea0b3214e694730a055cb6b2efa801" dependencies = [ "base64", "bytes", + "encoding_rs", + "futures-channel", "futures-core", + "futures-util", + "h2", "http", "http-body", "http-body-util", @@ -1780,6 +1835,7 @@ dependencies = [ "hyper-util", "js-sys", "log", + "mime", "percent-encoding", "pin-project-lite", "quinn", @@ -1885,7 +1941,7 @@ version = "0.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d99feebc72bae7ab76ba994bb5e121b8d83d910ca40b36e0921f53becc41784" dependencies = [ - "core-foundation", + "core-foundation 0.10.1", "core-foundation-sys", "jni", "log", @@ -1986,7 +2042,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b7f4bc775c73d9a02cde8bf7b2ec4c9d12743edf609006c7facc23998404cd1d" dependencies = [ "bitflags", - "core-foundation", + "core-foundation 0.10.1", "core-foundation-sys", "libc", "security-framework-sys", @@ -2233,6 +2289,27 @@ dependencies = [ "syn", ] +[[package]] +name = "system-configuration" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a13f3d0daba03132c0aa9767f98351b3488edc2c100cda2d2ec2b04f3d8d3c8b" +dependencies = [ + "bitflags", + "core-foundation 0.9.4", + "system-configuration-sys", +] + +[[package]] +name = "system-configuration-sys" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e1d1b10ced5ca923a1fcb8d03e96b8d3268065d724548c0211415ff6ac6bac4" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "tempfile" version = "3.27.0" @@ -2732,6 +2809,17 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-registry" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "02752bf7fbdcce7f2a27a742f798510f3e5ad88dbe84871e5168e2120c3d5720" +dependencies = [ + "windows-link", + "windows-result", + "windows-strings", +] + [[package]] name = "windows-result" version = "0.4.1" diff --git a/apps/staged/src-tauri/Cargo.lock b/apps/staged/src-tauri/Cargo.lock index 2f914d9b4..c3e2a7ced 100644 --- a/apps/staged/src-tauri/Cargo.lock +++ b/apps/staged/src-tauri/Cargo.lock @@ -1274,7 +1274,11 @@ dependencies = [ name = "doctor" version = "0.1.0" dependencies = [ + "dirs", + "futures", + "reqwest", "serde", + "serde_json", "tokio", ] @@ -4162,6 +4166,7 @@ dependencies = [ "base64 0.22.1", "bytes", "encoding_rs", + "futures-channel", "futures-core", "futures-util", "h2", diff --git a/crates/doctor/Cargo.toml b/crates/doctor/Cargo.toml index 46ddff82f..a4b2600f4 100644 --- a/crates/doctor/Cargo.toml +++ b/crates/doctor/Cargo.toml @@ -6,4 +6,8 @@ description = "Health-check system for verifying external tool dependencies" [dependencies] serde = { version = "1.0", features = ["derive"] } +serde_json = "1" tokio = { version = "1.0", features = ["rt", "macros"] } +futures = "0.3" +reqwest = { version = "0.13", features = ["blocking", "json"] } +dirs = "6" diff --git a/crates/doctor/src/freshness.rs b/crates/doctor/src/freshness.rs new file mode 100644 index 000000000..003e2bb5e --- /dev/null +++ b/crates/doctor/src/freshness.rs @@ -0,0 +1,486 @@ +//! Version-freshness lookups: installed version (local subprocess) + +//! latest available version (brew/npm/crates.io) with a disk cache. +//! +//! Only runs when `RunChecksOptions::check_freshness` is set — the default +//! `run_checks()` path is unaffected. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::process::Command; +use std::sync::{Arc, Mutex}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +use crate::types::InstallSource; + +/// One hour: long enough that repeated `run_checks_with_options` calls in the +/// same session don't re-hit registries; short enough that real upgrades +/// surface within a reasonable window. +const CACHE_TTL_SECONDS: i64 = 60 * 60; + +/// Result of a single version-freshness probe. +#[derive(Debug, Clone, Default)] +pub(crate) struct VersionInfo { + pub installed: Option, + pub latest: Option, + pub update_available: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +struct CacheEntry { + value: String, + fetched_at: i64, +} + +/// In-memory mirror of the on-disk cache. Read once at the start of a freshness +/// run, mutated as fetches happen, persisted once at the end. +#[derive(Debug, Default)] +pub(crate) struct FreshnessCache { + entries: HashMap, +} + +impl FreshnessCache { + fn cache_key(source: InstallSource, package_id: &str) -> String { + format!("{:?}:{}", source, package_id) + } + + fn get_fresh(&self, source: InstallSource, package_id: &str, now: i64) -> Option { + let key = Self::cache_key(source, package_id); + let entry = self.entries.get(&key)?; + if now.saturating_sub(entry.fetched_at) <= CACHE_TTL_SECONDS { + Some(entry.value.clone()) + } else { + None + } + } + + fn insert(&mut self, source: InstallSource, package_id: &str, value: String, now: i64) { + let key = Self::cache_key(source, package_id); + self.entries.insert( + key, + CacheEntry { + value, + fetched_at: now, + }, + ); + } +} + +/// Resolve the on-disk cache file path. Prefers `dirs::cache_dir()`; the +/// `/doctor` parent is created lazily on save. +pub(crate) fn cache_file_path() -> Option { + let base = dirs::cache_dir()?; + Some(base.join("doctor").join("freshness.json")) +} + +/// Read the on-disk cache. Missing-or-malformed → empty (and we'll overwrite +/// on next save). Errors are logged, never propagated. +pub(crate) fn load_cache() -> FreshnessCache { + let Some(path) = cache_file_path() else { + return FreshnessCache::default(); + }; + match std::fs::read(&path) { + Ok(bytes) => match serde_json::from_slice::>(&bytes) { + Ok(entries) => FreshnessCache { entries }, + Err(e) => { + eprintln!( + "doctor: freshness cache malformed at {}: {e}", + path.display() + ); + FreshnessCache::default() + } + }, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => FreshnessCache::default(), + Err(e) => { + eprintln!("doctor: failed to read freshness cache: {e}"); + FreshnessCache::default() + } + } +} + +/// Atomically persist the cache: write to `.tmp`, then rename. +pub(crate) fn save_cache(cache: &FreshnessCache) { + let Some(path) = cache_file_path() else { + return; + }; + if let Some(parent) = path.parent() { + if let Err(e) = std::fs::create_dir_all(parent) { + eprintln!( + "doctor: failed to create freshness cache dir {}: {e}", + parent.display(), + ); + return; + } + } + let json = match serde_json::to_vec_pretty(&cache.entries) { + Ok(j) => j, + Err(e) => { + eprintln!("doctor: failed to serialize freshness cache: {e}"); + return; + } + }; + let tmp = path.with_extension("json.tmp"); + if let Err(e) = std::fs::write(&tmp, &json) { + eprintln!("doctor: failed to write freshness cache tmp file: {e}"); + return; + } + if let Err(e) = std::fs::rename(&tmp, &path) { + eprintln!("doctor: failed to rename freshness cache tmp -> final: {e}"); + } +} + +fn now_epoch_seconds() -> i64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_secs() as i64) + .unwrap_or(0) +} + +/// Locate the first semver-shaped token (`X.Y.Z`) on any line of `text`. +/// +/// We accept an optional leading `v` and ignore anything after the patch +/// component (pre-release / build metadata is fine to include in the returned +/// string — it's compared as a string only). +pub(crate) fn extract_version(text: &str) -> Option { + for line in text.lines() { + if let Some(v) = find_semver_in_line(line) { + return Some(v); + } + } + None +} + +fn find_semver_in_line(line: &str) -> Option { + let bytes = line.as_bytes(); + let mut i = 0; + while i < bytes.len() { + if bytes[i].is_ascii_digit() { + // Greedy: extend through digits.dots; require at least two dots. + let start = i; + let mut dots = 0; + while i < bytes.len() { + let b = bytes[i]; + if b.is_ascii_digit() { + i += 1; + } else if b == b'.' && i + 1 < bytes.len() && bytes[i + 1].is_ascii_digit() { + dots += 1; + i += 1; + } else { + break; + } + } + if dots >= 2 { + return Some(line[start..i].to_string()); + } + } else { + i += 1; + } + } + None +} + +/// Lightweight parse of `X.Y.Z[...]` into `(major, minor, patch)`. Returns +/// `None` if any of the first three numeric components is missing. +fn parse_semver_triple(v: &str) -> Option<(u64, u64, u64)> { + let trimmed = v.trim().trim_start_matches('v'); + let core = trimmed + .split(|c: char| c == '-' || c == '+' || c == ' ') + .next()?; + let mut parts = core.split('.'); + let major = parts.next()?.parse::().ok()?; + let minor = parts.next()?.parse::().ok()?; + let patch_str = parts.next()?; + let patch = patch_str + .chars() + .take_while(|c| c.is_ascii_digit()) + .collect::() + .parse::() + .ok()?; + Some((major, minor, patch)) +} + +/// Compute `update_available` if both sides parse as semver triples; otherwise +/// `None` (so the UI doesn't render a misleading "outdated" badge based on +/// string inequality of unparseable versions). +fn compute_update_available(installed: &Option, latest: &Option) -> Option { + let i = installed.as_deref()?; + let l = latest.as_deref()?; + let it = parse_semver_triple(i)?; + let lt = parse_semver_triple(l)?; + Some(lt > it) +} + +/// Run ` ` and parse the first semver-shaped token out of the +/// combined stdout/stderr. Errors and missing tokens both yield `None`. +fn installed_version(binary_path: &Path, version_args: &[&str]) -> Option { + let output = Command::new(binary_path).args(version_args).output().ok()?; + let combined = format!( + "{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + extract_version(&combined) +} + +/// Dispatch a latest-version probe per source. +fn latest_version(source: InstallSource, package_id: &str) -> Option { + match source { + InstallSource::Brew => latest_brew(package_id), + InstallSource::Npm => latest_npm(package_id), + InstallSource::Cargo => latest_crates_io(package_id), + InstallSource::CurlPipe + | InstallSource::System + | InstallSource::Unknown + | InstallSource::Mise + | InstallSource::Asdf => None, + } +} + +fn latest_brew(package_id: &str) -> Option { + let output = Command::new("brew") + .args(["info", "--json=v2", package_id]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + parse_brew_info_v2(&output.stdout, package_id) +} + +/// Pull `formulae[0].versions.stable` or `casks[0].version` out of a +/// `brew info --json=v2` payload. Public to the crate so unit tests can +/// drive it with a fixture without shelling out. +pub(crate) fn parse_brew_info_v2(bytes: &[u8], _package_id: &str) -> Option { + let root: Value = serde_json::from_slice(bytes).ok()?; + if let Some(formulae) = root.get("formulae").and_then(|v| v.as_array()) { + if let Some(first) = formulae.first() { + if let Some(v) = first + .get("versions") + .and_then(|v| v.get("stable")) + .and_then(|v| v.as_str()) + { + return Some(v.to_string()); + } + } + } + if let Some(casks) = root.get("casks").and_then(|v| v.as_array()) { + if let Some(first) = casks.first() { + if let Some(v) = first.get("version").and_then(|v| v.as_str()) { + return Some(v.to_string()); + } + } + } + None +} + +fn latest_npm(package_id: &str) -> Option { + let output = Command::new("npm") + .args(["view", package_id, "version"]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let raw = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if raw.is_empty() { + None + } else { + Some(raw) + } +} + +fn latest_crates_io(package_id: &str) -> Option { + let url = format!("https://crates.io/api/v1/crates/{package_id}"); + let client = reqwest::blocking::Client::builder() + .user_agent("block-builderbot-doctor/0.1") + .timeout(std::time::Duration::from_secs(10)) + .build() + .ok()?; + let resp = client.get(&url).send().ok()?; + if !resp.status().is_success() { + return None; + } + let json: Value = resp.json().ok()?; + json.get("crate") + .and_then(|c| c.get("max_stable_version")) + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) +} + +/// Top-level: returns installed (always, when binary present), latest (only +/// if `package_id` is `Some` and we're not offline), and `update_available` +/// (only if both parse as semver triples). +pub(crate) async fn fetch_version_info( + install_source: Option, + package_id: Option<&str>, + binary_path: &Path, + version_args: &[&str], + offline: bool, + cache: Arc>, +) -> VersionInfo { + let path = binary_path.to_path_buf(); + let args: Vec = version_args.iter().map(|s| s.to_string()).collect(); + let pkg = package_id.map(|s| s.to_string()); + + let result = tokio::task::spawn_blocking(move || { + let installed = if path.as_os_str().is_empty() { + None + } else { + installed_version( + &path, + &args.iter().map(|s| s.as_str()).collect::>(), + ) + }; + + let latest = if offline { + None + } else if let (Some(source), Some(pkg)) = (install_source, pkg) { + let now = now_epoch_seconds(); + // Cache lookup first. + let cached = { + let guard = cache.lock().ok(); + guard + .as_ref() + .and_then(|c| c.get_fresh(source.clone(), &pkg, now)) + }; + if let Some(v) = cached { + Some(v) + } else if let Some(v) = latest_version(source.clone(), &pkg) { + if let Ok(mut guard) = cache.lock() { + guard.insert(source, &pkg, v.clone(), now); + } + Some(v) + } else { + None + } + } else { + None + }; + + let update_available = compute_update_available(&installed, &latest); + VersionInfo { + installed, + latest, + update_available, + } + }) + .await; + + result.unwrap_or_default() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn extract_version_finds_git_style_string() { + assert_eq!( + extract_version("git version 2.39.5 (Apple Git-154)"), + Some("2.39.5".to_string()), + ); + } + + #[test] + fn extract_version_finds_gh_multiline() { + let s = "gh version 2.62.0 (2024-11-12)\nhttps://github.com/cli/cli/releases/tag/v2.62.0"; + assert_eq!(extract_version(s), Some("2.62.0".to_string())); + } + + #[test] + fn extract_version_returns_none_when_no_semver() { + assert_eq!(extract_version("no version here"), None); + assert_eq!(extract_version("1.2"), None); // need 3 components + } + + #[test] + fn extract_version_handles_leading_garbage() { + assert_eq!( + extract_version("Codex CLI 0.21.4-beta"), + Some("0.21.4".to_string()), + ); + } + + #[test] + fn parse_semver_triple_basic() { + assert_eq!(parse_semver_triple("1.2.3"), Some((1, 2, 3))); + assert_eq!(parse_semver_triple("v1.2.3"), Some((1, 2, 3))); + assert_eq!(parse_semver_triple("1.2.3-rc1"), Some((1, 2, 3))); + assert_eq!(parse_semver_triple("1.2"), None); + } + + #[test] + fn compute_update_available_handles_parse_failure() { + assert_eq!( + compute_update_available( + &Some("not-a-version".to_string()), + &Some("1.0.0".to_string()), + ), + None, + ); + assert_eq!( + compute_update_available(&Some("1.0.0".to_string()), &Some("1.0.0".to_string())), + Some(false), + ); + assert_eq!( + compute_update_available(&Some("1.0.0".to_string()), &Some("1.0.1".to_string())), + Some(true), + ); + } + + #[test] + fn brew_info_v2_parses_formula_versions_stable() { + let json = br#"{ + "formulae": [{ + "name": "git", + "versions": {"stable": "2.50.1", "head": null, "bottle": true} + }], + "casks": [] + }"#; + assert_eq!(parse_brew_info_v2(json, "git").as_deref(), Some("2.50.1"),); + } + + #[test] + fn brew_info_v2_parses_cask_version() { + let json = br#"{ + "formulae": [], + "casks": [{"token": "codex", "version": "0.21.4"}] + }"#; + assert_eq!(parse_brew_info_v2(json, "codex").as_deref(), Some("0.21.4"),); + } + + #[test] + fn brew_info_v2_returns_none_on_empty_payload() { + let json = br#"{"formulae": [], "casks": []}"#; + assert_eq!(parse_brew_info_v2(json, "ghost"), None); + } + + #[test] + fn cache_ttl_treats_old_entries_as_stale() { + let mut cache = FreshnessCache::default(); + let now = 1_000_000; + cache.insert(InstallSource::Brew, "git", "2.50.0".to_string(), now); + + // Within TTL. + assert_eq!( + cache.get_fresh(InstallSource::Brew, "git", now + 60), + Some("2.50.0".to_string()), + ); + + // 2 hours later — stale. + let two_hours_later = now + 2 * 60 * 60; + assert_eq!( + cache.get_fresh(InstallSource::Brew, "git", two_hours_later), + None, + ); + } + + #[test] + fn cache_key_uses_source_and_package_id() { + let k1 = FreshnessCache::cache_key(InstallSource::Brew, "git"); + let k2 = FreshnessCache::cache_key(InstallSource::Npm, "git"); + assert_ne!(k1, k2, "different sources should map to different keys"); + } +} diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index 987c8da3d..b8bf83347 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -6,17 +6,23 @@ pub mod agents; pub mod checks; +pub(crate) mod freshness; +pub(crate) mod package_ids; pub mod resolve; pub mod types; pub use types::{CheckStatus, DoctorCheck, DoctorReport, FixType}; use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::{Arc, Mutex}; use agents::{check_single_ai_agent, lookup_fix_command, AI_AGENT_CHECKS}; use checks::{check_clonefile, check_gh, check_gh_auth, check_git, check_git_lfs}; +use freshness::{fetch_version_info, load_cache, save_cache}; +use package_ids::lookup_package_id; use resolve::resolve_binary; -use types::ResolvedBinary; +use types::{InstallSource, ResolvedBinary}; /// Fallback check returned when a spawn_blocking task panics. fn empty_check(id: &str, label: &str) -> DoctorCheck { @@ -39,8 +45,38 @@ fn empty_check(id: &str, label: &str) -> DoctorCheck { } } -/// Run all health checks and return the report. +/// Options controlling optional, slower passes layered on top of the core +/// check set. Defaults preserve the original [`run_checks`] behavior — no +/// network, no extra subprocess fan-out. +#[derive(Debug, Clone, Default)] +pub struct RunChecksOptions { + /// If true, populate `installed_version`/`latest_version`/`update_available` + /// on each check by probing the binary and the relevant registry. + pub check_freshness: bool, + /// If true (in combination with `check_freshness`), skip the remote + /// registry lookups — only the local installed-version probe runs. + pub offline: bool, +} + +/// Run all health checks and return the report. Equivalent to +/// `run_checks_with_options(RunChecksOptions::default())`. pub async fn run_checks() -> DoctorReport { + run_checks_with_options(RunChecksOptions::default()).await +} + +/// Run all health checks with explicit options. Existing callers that want +/// the cheap, no-network path should keep using [`run_checks`]. +pub async fn run_checks_with_options(opts: RunChecksOptions) -> DoctorReport { + let report = collect_base_report().await; + + if opts.check_freshness { + populate_freshness(report, opts.offline).await + } else { + report + } +} + +async fn collect_base_report() -> DoctorReport { let mut binary_names: Vec<&'static str> = vec!["git", "gh", "git-lfs"]; for info in AI_AGENT_CHECKS { for cmd in info.commands { @@ -148,6 +184,78 @@ pub async fn run_checks() -> DoctorReport { DoctorReport { checks } } +/// Post-hoc pass: for every check that has a usable binary path and a known +/// package id, run the installed/latest version probes in parallel and update +/// the corresponding fields on the report. The on-disk cache is read once at +/// the start and written once at the end. +async fn populate_freshness(mut report: DoctorReport, offline: bool) -> DoctorReport { + let cache = Arc::new(Mutex::new(load_cache())); + + let mut targets: Vec = Vec::new(); + for check in &report.checks { + // Prefer the bridge path when present (matches the install_source the + // check carries — see check_single_ai_agent). + let path_str = check.bridge_path.as_deref().or(check.path.as_deref()); + let Some(path_str) = path_str else { continue }; + + let package_id = check + .install_source + .clone() + .and_then(|src| lookup_package_id(&check.id, src)) + .map(|s| s.to_string()); + + targets.push(FreshnessTarget { + id: check.id.clone(), + path: PathBuf::from(path_str), + install_source: check.install_source.clone(), + package_id, + }); + } + + let futures = targets.into_iter().map(|t| { + let cache = cache.clone(); + async move { + let info = fetch_version_info( + t.install_source, + t.package_id.as_deref(), + &t.path, + &["--version"], + offline, + cache, + ) + .await; + (t.id, info) + } + }); + + let results = futures::future::join_all(futures).await; + let mut by_id: HashMap = HashMap::new(); + for (id, info) in results { + by_id.insert(id, info); + } + + for check in &mut report.checks { + if let Some(info) = by_id.remove(&check.id) { + check.installed_version = info.installed; + check.latest_version = info.latest; + check.update_available = info.update_available; + } + } + + if let Ok(guard) = cache.lock() { + save_cache(&guard); + } + + report +} + +struct FreshnessTarget { + id: String, + path: PathBuf, + install_source: Option, + package_id: Option, +} + /// Run a fix command for a doctor check, identified by check ID and fix type. /// /// The actual shell command is looked up from the static check definitions — @@ -201,3 +309,36 @@ pub(crate) fn execute_command_blocking(command: &str) -> Result<(), String> { }) } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Default `run_checks()` must not populate any of the version-freshness + /// fields — guards against accidentally flipping the default to on + /// (which would slow down every staged Tauri app launch). + #[tokio::test] + async fn run_checks_default_leaves_freshness_fields_empty() { + let report = run_checks().await; + for check in &report.checks { + assert!( + check.installed_version.is_none(), + "check {} unexpectedly populated installed_version = {:?}", + check.id, + check.installed_version, + ); + assert!( + check.latest_version.is_none(), + "check {} unexpectedly populated latest_version = {:?}", + check.id, + check.latest_version, + ); + assert!( + check.update_available.is_none(), + "check {} unexpectedly populated update_available = {:?}", + check.id, + check.update_available, + ); + } + } +} diff --git a/crates/doctor/src/package_ids.rs b/crates/doctor/src/package_ids.rs new file mode 100644 index 000000000..3d09435d5 --- /dev/null +++ b/crates/doctor/src/package_ids.rs @@ -0,0 +1,98 @@ +//! Per-check package identifiers for version-freshness lookups. +//! +//! Maps a doctor check ID to one or more `(InstallSource, package_id)` pairs. +//! The freshness module dispatches to the registry that matches the binary's +//! detected install source. Checks not listed here (or with no matching +//! source) skip the latest-version probe. + +use crate::types::InstallSource; + +/// Static table of `check_id -> &[(install_source, package_id)]`. +/// +/// A single check can have multiple entries when the same agent ships through +/// different registries (e.g. brew for the main binary, npm for the ACP bridge). +/// `lookup_package_id` picks the first entry whose `install_source` matches the +/// binary's detected source. +pub(crate) const PACKAGE_IDS: &[(&str, &[(InstallSource, &str)])] = &[ + ("git", &[(InstallSource::Brew, "git")]), + ("gh", &[(InstallSource::Brew, "gh")]), + ("git-lfs", &[(InstallSource::Brew, "git-lfs")]), + // ai-agent-goose: brew tap exists but no canonical formula yet — skip. + // TODO: revisit when block/goose lands a stable brew formula. + ( + "ai-agent-claude", + &[(InstallSource::Npm, "@agentclientprotocol/claude-agent-acp")], + ), + ( + "ai-agent-codex", + &[ + (InstallSource::Npm, "@zed-industries/codex-acp"), + (InstallSource::Brew, "codex"), + ], + ), + ("ai-agent-pi", &[(InstallSource::Npm, "pi-acp")]), + ( + "ai-agent-amp", + &[ + (InstallSource::Npm, "amp-acp"), + (InstallSource::Brew, "amp"), + ], + ), + ( + "ai-agent-copilot", + &[(InstallSource::Npm, "@github/copilot")], + ), + // ai-agent-cursor: curl-pipe installer with no registry presence. + // TODO: GitHub releases would be a reasonable follow-up source. +]; + +/// Pick the package id whose source matches `source`. Returns `None` if the +/// check isn't in the table, or if no entry matches. +pub(crate) fn lookup_package_id(check_id: &str, source: InstallSource) -> Option<&'static str> { + for (id, entries) in PACKAGE_IDS { + if *id == check_id { + for (entry_source, pkg) in entries.iter() { + if entry_source == &source { + return Some(*pkg); + } + } + return None; + } + } + None +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn lookup_matches_source() { + assert_eq!(lookup_package_id("git", InstallSource::Brew), Some("git"),); + assert_eq!( + lookup_package_id("ai-agent-claude", InstallSource::Npm), + Some("@agentclientprotocol/claude-agent-acp"), + ); + } + + #[test] + fn lookup_returns_none_for_mismatched_source() { + // git is registered under Brew only — Npm should miss. + assert_eq!(lookup_package_id("git", InstallSource::Npm), None); + } + + #[test] + fn lookup_returns_none_for_unknown_check() { + assert_eq!( + lookup_package_id("ai-agent-cursor", InstallSource::CurlPipe), + None, + ); + assert_eq!(lookup_package_id("nonexistent", InstallSource::Brew), None); + } + + #[test] + fn codex_has_both_npm_and_brew_entries() { + assert!(lookup_package_id("ai-agent-codex", InstallSource::Npm).is_some()); + assert!(lookup_package_id("ai-agent-codex", InstallSource::Brew).is_some()); + } +} From 155dcad42f3f97730328a15d53ac90e0ea7b73a2 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 27 May 2026 16:25:53 +1000 Subject: [PATCH 08/18] feat(doctor): stream subprocess output from execute_fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 5 of the doctor reunification plan. execute_fix is now a |_| {} wrapper over execute_fix_streaming, which invokes a callback once per stdout/stderr line so downstream Tauri apps can emit per-line install/auth progress (goose-internal's existing agent-setup:output event pattern). Line ordering across stdout/stderr is best-effort — within each stream lines arrive in order, but interleaving depends on scheduling. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/lib.rs | 169 ++++++++++++++++++++++++++++++++++----- 1 file changed, 151 insertions(+), 18 deletions(-) diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index b8bf83347..176dc819b 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -261,26 +261,58 @@ struct FreshnessTarget { /// The actual shell command is looked up from the static check definitions — /// the caller never sends a raw command string. pub async fn execute_fix(check_id: String, fix_type: FixType) -> Result<(), String> { + execute_fix_streaming(check_id, fix_type, |_| {}).await +} + +/// Run a fix command and stream its output line-by-line to `on_line`. +/// +/// `on_line` is invoked once per output line, with the trailing newline +/// stripped. Both stdout and stderr lines are delivered through the same +/// callback; ordering across the two streams is best-effort (each stream is +/// in order internally, but interleaving between them depends on scheduling). +/// +/// The callback runs on the tokio runtime's blocking pool — do not block in it +/// for unbounded periods, but emitting Tauri events / writing to a channel is +/// fine. +pub async fn execute_fix_streaming( + check_id: String, + fix_type: FixType, + on_line: F, +) -> Result<(), String> +where + F: FnMut(&str) + Send + 'static, +{ let command = lookup_fix_command(&check_id, &fix_type) .ok_or_else(|| format!("Unknown check '{check_id}' or fix type '{fix_type:?}'"))?; - execute_command(command).await + run_command_streaming(command, on_line).await } -/// Run an arbitrary shell command in a login shell. -/// -/// Internal primitive used by [`execute_fix`]. Not exposed publicly — callers -/// should use `execute_fix` which looks up the command from static definitions. -pub(crate) async fn execute_command(command: String) -> Result<(), String> { - tokio::task::spawn_blocking(move || execute_command_blocking(&command)) +/// Async wrapper that runs `run_command_streaming_blocking` on the blocking pool. +pub(crate) async fn run_command_streaming(command: String, on_line: F) -> Result<(), String> +where + F: FnMut(&str) + Send + 'static, +{ + tokio::task::spawn_blocking(move || run_command_streaming_blocking(&command, on_line)) .await .unwrap_or_else(|e| Err(format!("Task failed: {e}"))) } -/// Synchronous twin of [`execute_command`] for use inside `spawn_blocking` -/// closures (e.g. per-check auth probes that run in the existing check -/// parallelism). -pub(crate) fn execute_command_blocking(command: &str) -> Result<(), String> { +enum StreamLine { + Stdout(String), + Stderr(String), +} + +/// Spawn `command` through a login shell, stream stdout/stderr lines to +/// `on_line`, and return based on the process exit status. Stderr lines are +/// also accumulated so a non-zero exit can surface a useful error message +/// (matching the non-streaming behavior of the previous `execute_command`). +fn run_command_streaming_blocking(command: &str, mut on_line: F) -> Result<(), String> +where + F: FnMut(&str), +{ + use std::io::{BufRead, BufReader}; + let (shell, args) = if std::path::Path::new("/bin/zsh").exists() { ("/bin/zsh", vec!["-l", "-c", command]) } else { @@ -288,32 +320,133 @@ pub(crate) fn execute_command_blocking(command: &str) -> Result<(), String> { }; let home = std::env::var("HOME").unwrap_or_else(|_| "/".to_string()); let user = std::env::var("USER").unwrap_or_default(); - let output = std::process::Command::new(shell) + + let mut child = std::process::Command::new(shell) .args(&args) .env_clear() .env("HOME", &home) .env("USER", &user) .env("TERM", "xterm-256color") .current_dir(&home) - .output() + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() .map_err(|e| format!("Failed to run command: {e}"))?; - if output.status.success() { + let stdout = child.stdout.take().expect("stdout was piped"); + let stderr = child.stderr.take().expect("stderr was piped"); + + let (tx, rx) = std::sync::mpsc::channel::(); + let tx_err = tx.clone(); + + let stdout_thread = std::thread::spawn(move || { + let reader = BufReader::new(stdout); + for line in reader.lines().map_while(Result::ok) { + if tx.send(StreamLine::Stdout(line)).is_err() { + break; + } + } + }); + let stderr_thread = std::thread::spawn(move || { + let reader = BufReader::new(stderr); + for line in reader.lines().map_while(Result::ok) { + if tx_err.send(StreamLine::Stderr(line)).is_err() { + break; + } + } + }); + + let mut stderr_accum = String::new(); + for msg in rx.iter() { + match msg { + StreamLine::Stdout(s) => { + on_line(&s); + } + StreamLine::Stderr(s) => { + on_line(&s); + if !stderr_accum.is_empty() { + stderr_accum.push('\n'); + } + stderr_accum.push_str(&s); + } + } + } + + let _ = stdout_thread.join(); + let _ = stderr_thread.join(); + + let status = child + .wait() + .map_err(|e| format!("Failed to wait for command: {e}"))?; + + if status.success() { Ok(()) } else { - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - Err(if stderr.is_empty() { - format!("Command failed with exit code {}", output.status) + let trimmed = stderr_accum.trim().to_string(); + Err(if trimmed.is_empty() { + format!("Command failed with exit code {status}") } else { - stderr + trimmed }) } } +/// Synchronous, non-streaming variant for callers running inside an existing +/// `spawn_blocking` closure (the auth probes in `check_single_ai_agent` use +/// this — they need a sync result, not an `on_line` stream). +pub(crate) fn execute_command_blocking(command: &str) -> Result<(), String> { + run_command_streaming_blocking(command, |_| {}) +} + #[cfg(test)] mod tests { use super::*; + use std::sync::{Arc, Mutex}; + + /// Streaming helper must invoke `on_line` for each output line of a + /// successful command. Lines from `.zshrc` etc. may also appear; we only + /// assert that our expected payload showed up. + #[tokio::test] + async fn run_command_streaming_emits_each_stdout_line() { + let lines: Arc>> = Arc::new(Mutex::new(Vec::new())); + let lines_clone = lines.clone(); + + let result = run_command_streaming( + "echo doctor-streaming-marker-hello && echo doctor-streaming-marker-world".to_string(), + move |line| { + lines_clone.lock().unwrap().push(line.to_string()); + }, + ) + .await; + + assert!(result.is_ok(), "streaming command failed: {result:?}"); + let captured = lines.lock().unwrap().clone(); + assert!( + captured + .iter() + .any(|l| l == "doctor-streaming-marker-hello"), + "did not see 'hello' marker; captured: {captured:?}", + ); + assert!( + captured + .iter() + .any(|l| l == "doctor-streaming-marker-world"), + "did not see 'world' marker; captured: {captured:?}", + ); + } + + /// `execute_fix(|_| {})` and `execute_fix_streaming(.., |_| {})` must + /// produce identical results for the same fix lookup — `execute_fix` is + /// supposed to be a thin delegate. + #[tokio::test] + async fn execute_fix_delegates_to_streaming() { + let direct = execute_fix("ai-agent-nonexistent".to_string(), FixType::Auth).await; + let streamed = + execute_fix_streaming("ai-agent-nonexistent".to_string(), FixType::Auth, |_| {}).await; + assert_eq!(direct, streamed); + } + /// Default `run_checks()` must not populate any of the version-freshness /// fields — guards against accidentally flipping the default to on /// (which would slow down every staged Tauri app launch). From beb4016ace007e65657ddd89a80a2e20e62dac42 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 3 Jun 2026 11:56:37 +1000 Subject: [PATCH 09/18] feat(doctor): add optional npm registry override to fix + freshness commands Restore BOT-686 / goose-internal PR #358 parity that the better-doctor migration dropped: direct npmjs.org access is blocked by Cloudflare WARP, so npm install/view must be routed through Block's internal Artifactory proxy via --registry=. This wires an optional, caller-supplied registry through the crate without baking in any Block-specific URL. - Add `npm_registry: Option` to RunChecksOptions (default None). - Add `apply_npm_registry(command, registry)` helper that appends ` --registry=` only to npm-backed commands (npm install / npm view); curl-pipe installers, auth commands, and the git-clonefile fix pass through unchanged. - Apply the registry to the displayed fix_command: threaded through run_checks_with_options -> collect_base_report -> check_single_ai_agent so the shown command matches what runs. - Apply the registry to the freshness probe: latest_npm now adds --registry , threaded via populate_freshness -> fetch_version_info. - Apply the registry to the execute path: new execute_fix_with_options / execute_fix_streaming_with_options resolve the command, run it through apply_npm_registry, then shell out. The existing execute_fix / execute_fix_streaming delegate with None. Purely additive: every new field/param is Option-defaulting-None, so RunChecksOptions::default() reproduces today's exact commands and the staged Tauri app compiles and behaves unchanged. The caller (goose-internal) supplies the Block Artifactory URL. Not in this commit (follow-ups): per-OS/platform install recipes (amp's CLI installer) and per-package name overrides. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 70 +++++++++++++++++++++++++++++++++- crates/doctor/src/freshness.rs | 24 ++++++++---- crates/doctor/src/lib.rs | 54 +++++++++++++++++++++++--- 3 files changed, 133 insertions(+), 15 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index cde9562c1..7672294dc 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -119,12 +119,37 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ }, ]; +/// 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 +/// command unchanged. +/// +/// The registry URL is always caller-supplied — the crate never bakes in a +/// Block-specific (or any other) registry. +pub(crate) fn apply_npm_registry(command: &str, registry: Option<&str>) -> String { + match registry { + Some(url) if is_npm_command(command) => format!("{command} --registry={url}"), + _ => command.to_string(), + } +} + +/// Heuristic for whether `command` shells out to npm. Matches a leading `npm ` +/// invocation as well as the `npm install` / `npm view` forms that may be +/// preceded by other tokens. +fn is_npm_command(command: &str) -> bool { + command.starts_with("npm ") || command.contains("npm install") || command.contains("npm view") +} + /// Check whether a single AI agent is installed. +/// +/// `npm_registry`, when `Some`, routes any npm-backed install/bridge fix +/// command through that registry (see [`apply_npm_registry`]). pub fn check_single_ai_agent( info: &AgentCheckInfo, any_agent_found: bool, resolved_cmds: &[ResolvedBinary], resolved_main: Option<&ResolvedBinary>, + npm_registry: Option<&str>, ) -> DoctorCheck { let header = format!( "# Check: {} — verify {} agent is installed", @@ -302,7 +327,9 @@ pub fn check_single_ai_agent( .bridge_install_url .or(info.install_url) .map(|s| s.to_string()), - fix_command: info.bridge_install_command.map(|s| s.to_string()), + fix_command: info + .bridge_install_command + .map(|s| apply_npm_registry(s, npm_registry)), fix_type: info.bridge_install_command.map(|_| FixType::Bridge), path: Some(main_path.to_string_lossy().to_string()), bridge_path: None, @@ -331,7 +358,9 @@ pub fn check_single_ai_agent( "Not installed — at least one AI agent is needed".to_string() }, fix_url: info.install_url.map(|s| s.to_string()), - fix_command: info.install_command.map(|s| s.to_string()), + fix_command: info + .install_command + .map(|s| apply_npm_registry(s, npm_registry)), fix_type: info.install_command.map(|_| FixType::Command), path: None, bridge_path: None, @@ -388,4 +417,41 @@ mod tests { fn auth_fix_lookup_returns_none_for_agents_without_auth_command() { assert_eq!(lookup_fix_command("ai-agent-goose", &FixType::Auth), None); } + + #[test] + fn apply_npm_registry_appends_to_npm_install() { + assert_eq!( + apply_npm_registry("npm install -g amp-acp", Some("https://artifactory/npm")), + "npm install -g amp-acp --registry=https://artifactory/npm", + ); + } + + #[test] + fn apply_npm_registry_appends_to_npm_view() { + assert_eq!( + apply_npm_registry("npm view amp-acp version", Some("https://artifactory/npm")), + "npm view amp-acp version --registry=https://artifactory/npm", + ); + } + + #[test] + fn apply_npm_registry_leaves_non_npm_commands_unchanged() { + let curl = "curl -fsSL https://cursor.com/install | bash"; + assert_eq!( + apply_npm_registry(curl, Some("https://artifactory/npm")), + curl, + ); + assert_eq!( + apply_npm_registry("claude auth login", Some("https://artifactory/npm")), + "claude auth login", + ); + } + + #[test] + fn apply_npm_registry_none_is_passthrough() { + assert_eq!( + apply_npm_registry("npm install -g amp-acp", None), + "npm install -g amp-acp", + ); + } } diff --git a/crates/doctor/src/freshness.rs b/crates/doctor/src/freshness.rs index 003e2bb5e..1f73e78c4 100644 --- a/crates/doctor/src/freshness.rs +++ b/crates/doctor/src/freshness.rs @@ -225,10 +225,14 @@ fn installed_version(binary_path: &Path, version_args: &[&str]) -> Option Option { +fn latest_version( + source: InstallSource, + package_id: &str, + npm_registry: Option<&str>, +) -> Option { match source { InstallSource::Brew => latest_brew(package_id), - InstallSource::Npm => latest_npm(package_id), + InstallSource::Npm => latest_npm(package_id, npm_registry), InstallSource::Cargo => latest_crates_io(package_id), InstallSource::CurlPipe | InstallSource::System @@ -275,11 +279,13 @@ pub(crate) fn parse_brew_info_v2(bytes: &[u8], _package_id: &str) -> Option Option { - let output = Command::new("npm") - .args(["view", package_id, "version"]) - .output() - .ok()?; +fn latest_npm(package_id: &str, npm_registry: Option<&str>) -> Option { + let mut cmd = Command::new("npm"); + cmd.args(["view", package_id, "version"]); + if let Some(registry) = npm_registry { + cmd.args(["--registry", registry]); + } + let output = cmd.output().ok()?; if !output.status.success() { return None; } @@ -318,11 +324,13 @@ pub(crate) async fn fetch_version_info( binary_path: &Path, version_args: &[&str], offline: bool, + npm_registry: Option<&str>, cache: Arc>, ) -> VersionInfo { let path = binary_path.to_path_buf(); let args: Vec = version_args.iter().map(|s| s.to_string()).collect(); let pkg = package_id.map(|s| s.to_string()); + let npm_registry = npm_registry.map(|s| s.to_string()); let result = tokio::task::spawn_blocking(move || { let installed = if path.as_os_str().is_empty() { @@ -347,7 +355,7 @@ pub(crate) async fn fetch_version_info( }; if let Some(v) = cached { Some(v) - } else if let Some(v) = latest_version(source.clone(), &pkg) { + } else if let Some(v) = latest_version(source.clone(), &pkg, npm_registry.as_deref()) { if let Ok(mut guard) = cache.lock() { guard.insert(source, &pkg, v.clone(), now); } diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index 176dc819b..394bf6199 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -56,6 +56,12 @@ pub struct RunChecksOptions { /// If true (in combination with `check_freshness`), skip the remote /// registry lookups — only the local installed-version probe runs. pub offline: bool, + /// Optional npm registry override. When `Some`, every npm-backed + /// install/probe command (`npm install -g …`, `npm view …`) is routed + /// through this registry via `--registry=`. The URL is always + /// caller-supplied; the crate bakes in no registry of its own. `None` + /// (the default) reproduces the original commands exactly. + pub npm_registry: Option, } /// Run all health checks and return the report. Equivalent to @@ -67,16 +73,17 @@ pub async fn run_checks() -> DoctorReport { /// Run all health checks with explicit options. Existing callers that want /// the cheap, no-network path should keep using [`run_checks`]. pub async fn run_checks_with_options(opts: RunChecksOptions) -> DoctorReport { - let report = collect_base_report().await; + let npm_registry = opts.npm_registry.as_deref(); + let report = collect_base_report(npm_registry).await; if opts.check_freshness { - populate_freshness(report, opts.offline).await + populate_freshness(report, opts.offline, npm_registry).await } else { report } } -async fn collect_base_report() -> DoctorReport { +async fn collect_base_report(npm_registry: Option<&str>) -> DoctorReport { let mut binary_names: Vec<&'static str> = vec!["git", "gh", "git-lfs"]; for info in AI_AGENT_CHECKS { for cmd in info.commands { @@ -140,10 +147,12 @@ async fn collect_base_report() -> DoctorReport { let c_git_lfs = tokio::task::spawn_blocking(move || check_git_lfs(&git_r2, &git_lfs_r)); let c_clonefile = tokio::task::spawn_blocking(move || check_clonefile(&git_r3)); + let npm_registry_owned = npm_registry.map(|s| s.to_string()); let agent_handles: Vec<_> = AI_AGENT_CHECKS .iter() .map(|info| { let found = any_agent_found; + let registry = npm_registry_owned.clone(); let cmds: Vec = info .commands .iter() @@ -156,7 +165,7 @@ async fn collect_base_report() -> DoctorReport { .collect(); let main = info.main_command.and_then(|cmd| resolved.get(cmd).cloned()); tokio::task::spawn_blocking(move || { - check_single_ai_agent(info, found, &cmds, main.as_ref()) + check_single_ai_agent(info, found, &cmds, main.as_ref(), registry.as_deref()) }) }) .collect(); @@ -188,8 +197,13 @@ async fn collect_base_report() -> DoctorReport { /// package id, run the installed/latest version probes in parallel and update /// the corresponding fields on the report. The on-disk cache is read once at /// the start and written once at the end. -async fn populate_freshness(mut report: DoctorReport, offline: bool) -> DoctorReport { +async fn populate_freshness( + mut report: DoctorReport, + offline: bool, + npm_registry: Option<&str>, +) -> DoctorReport { let cache = Arc::new(Mutex::new(load_cache())); + let npm_registry = npm_registry.map(|s| s.to_string()); let mut targets: Vec = Vec::new(); for check in &report.checks { @@ -214,6 +228,7 @@ async fn populate_freshness(mut report: DoctorReport, offline: bool) -> DoctorRe let futures = targets.into_iter().map(|t| { let cache = cache.clone(); + let npm_registry = npm_registry.clone(); async move { let info = fetch_version_info( t.install_source, @@ -221,6 +236,7 @@ async fn populate_freshness(mut report: DoctorReport, offline: bool) -> DoctorRe &t.path, &["--version"], offline, + npm_registry.as_deref(), cache, ) .await; @@ -264,6 +280,18 @@ pub async fn execute_fix(check_id: String, fix_type: FixType) -> Result<(), Stri execute_fix_streaming(check_id, fix_type, |_| {}).await } +/// Like [`execute_fix`], but routes npm-backed fix commands through an optional +/// caller-supplied registry (see [`RunChecksOptions::npm_registry`]). Callers +/// that displayed a registry-overridden `fix_command` should pass the same +/// `npm_registry` here so the executed command matches what was shown. +pub async fn execute_fix_with_options( + check_id: String, + fix_type: FixType, + npm_registry: Option<&str>, +) -> Result<(), String> { + execute_fix_streaming_with_options(check_id, fix_type, npm_registry, |_| {}).await +} + /// Run a fix command and stream its output line-by-line to `on_line`. /// /// `on_line` is invoked once per output line, with the trailing newline @@ -279,11 +307,27 @@ pub async fn execute_fix_streaming( fix_type: FixType, on_line: F, ) -> Result<(), String> +where + F: FnMut(&str) + Send + 'static, +{ + execute_fix_streaming_with_options(check_id, fix_type, None, on_line).await +} + +/// Like [`execute_fix_streaming`], but routes npm-backed fix commands through an +/// optional caller-supplied registry before shelling out (see +/// [`RunChecksOptions::npm_registry`]). Non-npm commands are run verbatim. +pub async fn execute_fix_streaming_with_options( + check_id: String, + fix_type: FixType, + npm_registry: Option<&str>, + on_line: F, +) -> Result<(), String> where F: FnMut(&str) + Send + 'static, { let command = lookup_fix_command(&check_id, &fix_type) .ok_or_else(|| format!("Unknown check '{check_id}' or fix type '{fix_type:?}'"))?; + let command = agents::apply_npm_registry(&command, npm_registry); run_command_streaming(command, on_line).await } From 962662ed7a073f0844104a85878516a91a7297f7 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 3 Jun 2026 12:51:40 +1000 Subject: [PATCH 10/18] fix(doctor): satisfy clippy manual_pattern_char_comparison in freshness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the split closure `|c: char| c == '-' || c == '+' || c == ' '` with the char-array pattern `['-', '+', ' ']`. Same delimiters, same behavior — clears the new clippy::manual_pattern_char_comparison lint (Rust 1.93.0) that was failing crates-lint in the pre-push hook. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/freshness.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/doctor/src/freshness.rs b/crates/doctor/src/freshness.rs index 1f73e78c4..49cc6ae83 100644 --- a/crates/doctor/src/freshness.rs +++ b/crates/doctor/src/freshness.rs @@ -185,9 +185,7 @@ fn find_semver_in_line(line: &str) -> Option { /// `None` if any of the first three numeric components is missing. fn parse_semver_triple(v: &str) -> Option<(u64, u64, u64)> { let trimmed = v.trim().trim_start_matches('v'); - let core = trimmed - .split(|c: char| c == '-' || c == '+' || c == ' ') - .next()?; + let core = trimmed.split(['-', '+', ' ']).next()?; let mut parts = core.split('.'); let major = parts.next()?.parse::().ok()?; let minor = parts.next()?.parse::().ok()?; From 5c0fc443d69ace98712a2402314ee2f189a3b7ac Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 3 Jun 2026 14:35:31 +1000 Subject: [PATCH 11/18] feat(doctor): fingerprint curl/native installs as CurlPipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase A of the per-agent install-method plan — fix install-source detection for curl/native installs (Claude native, Cursor, Amp) that land in ~/.local/bin (or ~/bin) and currently collapse to Unknown. Path-only heuristics can't classify these, so add two layers on top of the existing path-prefix detector, both keeping Unknown as the honest fallback: - Filesystem fingerprinting (resolve.rs): when path-prefix detection yields Unknown for a binary in a user-local bin dir, fingerprint_curl_pipe recognises a curl/native install via two low-false-positive signals — a known installer footprint marker under $HOME (CURL_INSTALLER_FOOTPRINTS for claude/cursor-agent/amp), or a symlink into a versioned install dir under $HOME (the Cursor/Claude native layout). Only read_link/exists/ canonicalize — no subprocess or network. detect_install_source now upgrades Unknown to CurlPipe on a match; the pure path-only inner (detect_install_source_with_home) is unchanged. - Per-tool override hook (agents.rs): AgentCheckInfo gains an optional install_source_override, applied only when the resolved binary's source is Unknown (apply_install_source_override). Claude, Amp, and Cursor declare CurlPipe; a positively-detected source (Brew/Npm) and a missing binary (None) are left untouched, so registry installs are never mislabelled. Purely additive: the new field is Option-defaulting-None and latest-version lookup still returns None for CurlPipe (Phase B territory), so default run_checks() behaviour and the staged Tauri app are unchanged. Adds unit tests for both the fingerprint signals and the override precedence. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 95 ++++++++++++++++++-- crates/doctor/src/resolve.rs | 164 +++++++++++++++++++++++++++++++++-- 2 files changed, 248 insertions(+), 11 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index 7672294dc..b2aa4ae00 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -5,7 +5,7 @@ use std::process::Command; use crate::checks::CLONEFILE_FIX_COMMAND; use crate::execute_command_blocking; use crate::resolve::format_command_output; -use crate::types::{AuthStatus, CheckStatus, DoctorCheck, FixType, ResolvedBinary}; +use crate::types::{AuthStatus, CheckStatus, DoctorCheck, FixType, InstallSource, ResolvedBinary}; /// Metadata for an individual AI agent check. pub struct AgentCheckInfo { @@ -29,6 +29,13 @@ pub struct AgentCheckInfo { pub auth_command: Option<&'static str>, /// Shell command that reports whether the user is currently authenticated. pub auth_status_command: Option<&'static str>, + /// Per-tool override declaring this agent's install source when generic path + /// and filesystem-fingerprint heuristics fall through to + /// [`InstallSource::Unknown`]. Lets a curl/native-only agent (Claude native, + /// Cursor, Amp) report its true source instead of `Unknown`. Applied only on + /// `Unknown`; a positively-detected source (Brew/Npm/…) and a missing binary + /// (`None`) are left untouched. + pub install_source_override: Option, } /// All AI agents we check for individually. @@ -44,6 +51,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ bridge_install_command: None, auth_command: None, auth_status_command: None, + install_source_override: None, }, AgentCheckInfo { id: "ai-agent-claude", @@ -56,6 +64,9 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ bridge_install_command: Some("npm install -g @agentclientprotocol/claude-agent-acp"), auth_command: Some("claude auth login"), auth_status_command: Some("claude auth status"), + // Main `claude` is a curl/native install; the bridge is npm and is + // detected positively, so this only takes effect on the main-only path. + install_source_override: Some(InstallSource::CurlPipe), }, AgentCheckInfo { id: "ai-agent-codex", @@ -68,6 +79,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ bridge_install_command: Some("npm install -g @zed-industries/codex-acp"), auth_command: Some("codex login"), auth_status_command: Some("codex login status"), + install_source_override: None, }, AgentCheckInfo { id: "ai-agent-pi", @@ -80,6 +92,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ bridge_install_command: None, auth_command: None, auth_status_command: None, + install_source_override: None, }, AgentCheckInfo { id: "ai-agent-amp", @@ -92,6 +105,8 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ bridge_install_command: Some("npm install -g amp-acp"), auth_command: Some("amp login"), auth_status_command: Some("amp usage"), + // Main `amp` curl installer; bridge `amp-acp` is npm (detected positively). + install_source_override: Some(InstallSource::CurlPipe), }, AgentCheckInfo { id: "ai-agent-copilot", @@ -104,6 +119,7 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ bridge_install_command: None, auth_command: Some("copilot login"), auth_status_command: None, + install_source_override: None, }, AgentCheckInfo { id: "ai-agent-cursor", @@ -116,6 +132,9 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ bridge_install_command: None, auth_command: Some("cursor-agent login"), auth_status_command: Some("cursor-agent status"), + // Cursor has only a curl/native installer and no ACP bridge, so the + // resolved binary is the curl install — the override's primary use case. + install_source_override: Some(InstallSource::CurlPipe), }, ]; @@ -140,6 +159,21 @@ fn is_npm_command(command: &str) -> bool { command.starts_with("npm ") || command.contains("npm install") || command.contains("npm view") } +/// Resolve the effective install source for an agent binary. Generic path and +/// filesystem fingerprinting (in [`crate::resolve`]) win when they identify a +/// source; only when they fall through to [`InstallSource::Unknown`] does the +/// agent's declared `override_hint` take effect. A missing binary (`None`) is +/// never fabricated into a source. +fn apply_install_source_override( + detected: Option, + override_hint: Option<&InstallSource>, +) -> Option { + match detected { + Some(InstallSource::Unknown) => override_hint.cloned().or(Some(InstallSource::Unknown)), + other => other, + } +} + /// Check whether a single AI agent is installed. /// /// `npm_registry`, when `Some`, routes any npm-backed install/bridge fix @@ -169,7 +203,10 @@ pub fn check_single_ai_agent( // Prefer the bridge's install_source — that's the binary the UI cares about // for "how did your acp adapter get installed". For goose (no separate // main_command) the bridge is the main binary itself, so this is correct. - let bridge_install_source = resolved_bridge.and_then(|rb| rb.install_source.clone()); + let bridge_install_source = apply_install_source_override( + resolved_bridge.and_then(|rb| rb.install_source.clone()), + info.install_source_override.as_ref(), + ); if let Some(ref path_str) = resolved_path { if info.id == "ai-agent-goose" { @@ -312,9 +349,12 @@ pub fn check_single_ai_agent( let main_search = &resolved_main.as_ref().unwrap().search_output; // Bridge wasn't found, so fall back to the main binary's // install_source — the only resolved path we have to describe. - let main_install_source = resolved_main - .as_ref() - .and_then(|rm| rm.install_source.clone()); + let main_install_source = apply_install_source_override( + resolved_main + .as_ref() + .and_then(|rm| rm.install_source.clone()), + info.install_source_override.as_ref(), + ); return DoctorCheck { id: info.id.to_string(), label: info.label.to_string(), @@ -418,6 +458,51 @@ mod tests { assert_eq!(lookup_fix_command("ai-agent-goose", &FixType::Auth), None); } + #[test] + fn override_applies_only_to_unknown() { + // Unknown + declared override => override wins. + assert_eq!( + apply_install_source_override( + Some(InstallSource::Unknown), + Some(&InstallSource::CurlPipe), + ), + Some(InstallSource::CurlPipe), + ); + // Unknown + no override => stays Unknown. + assert_eq!( + apply_install_source_override(Some(InstallSource::Unknown), None), + Some(InstallSource::Unknown), + ); + // A positively-detected source is never overridden. + assert_eq!( + apply_install_source_override(Some(InstallSource::Npm), Some(&InstallSource::CurlPipe),), + Some(InstallSource::Npm), + ); + // A missing binary is never fabricated into a source. + assert_eq!( + apply_install_source_override(None, Some(&InstallSource::CurlPipe)), + None, + ); + } + + #[test] + fn curl_native_agents_declare_curl_pipe_override() { + for id in ["ai-agent-claude", "ai-agent-amp", "ai-agent-cursor"] { + let info = AI_AGENT_CHECKS.iter().find(|i| i.id == id).unwrap(); + assert_eq!( + info.install_source_override, + Some(InstallSource::CurlPipe), + "{id} should declare a CurlPipe override", + ); + } + // Registry-installed agents declare no override. + let codex = AI_AGENT_CHECKS + .iter() + .find(|i| i.id == "ai-agent-codex") + .unwrap(); + assert_eq!(codex.install_source_override, None); + } + #[test] fn apply_npm_registry_appends_to_npm_install() { assert_eq!( diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 99ccb9fa5..631c1f627 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -232,14 +232,106 @@ fn npm_global_bin_dir(lines: &mut Vec) -> Option { Some(bin) } -/// Infer how a binary was installed from its path alone — no subprocess or -/// network probes. Path-prefix heuristics cover Brew, Cargo, Mise, Asdf, Npm -/// (mirroring the dirs in [`npm_search_dirs`]), and the System dirs; anything -/// else (including `~/.local/bin` curl-pipe installs, which can't be -/// fingerprinted reliably from the path) is reported as [`InstallSource::Unknown`]. +/// Infer how a binary was installed. First applies path-prefix heuristics (no +/// subprocess or network probes) covering Brew, Cargo, Mise, Asdf, Npm +/// (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 (Claude native, Cursor, Amp), +/// which land in `~/.local/bin`/`~/bin`. [`InstallSource::Unknown`] remains the +/// honest fallback when no fingerprint matches. pub(crate) fn detect_install_source(path: &Path) -> InstallSource { let home = std::env::home_dir(); - detect_install_source_with_home(path, home.as_deref()) + let base = detect_install_source_with_home(path, home.as_deref()); + if base == InstallSource::Unknown { + if let Some(home) = home.as_deref() { + if fingerprint_curl_pipe(path, home) { + return InstallSource::CurlPipe; + } + } + } + base +} + +/// 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] = &[ + // Claude native installer — claude.ai/install.sh. + CurlInstallerFootprint { + binary: "claude", + markers: &[".local/share/claude", ".claude/local", ".claude/bin"], + }, + // 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"], + }, +]; + +/// 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. +/// 2. 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//`). +/// +/// 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 { + 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; + } + } + } + + // 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; + } + } + + false } /// Testable inner: same logic as [`detect_install_source`] but takes the home @@ -522,6 +614,66 @@ mod tests { ); } + #[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); + // Claude native installer: ~/.local/bin/claude + ~/.local/share/claude. + fs::create_dir_all(home.join(".local/bin")).unwrap(); + fs::create_dir_all(home.join(".local/share/claude")).unwrap(); + let bin = home.join(".local/bin/claude"); + 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)] + { + let home = + std::env::temp_dir().join(format!("doctor-fp-symlink-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + // Cursor layout: ~/.local/bin/cursor-agent -> ~/.local/share/cursor-agent/versions/1.0.0/cursor-agent + 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(); + 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!(fingerprint_curl_pipe(&link, &home)); + let _ = fs::remove_dir_all(&home); + } + } + + #[test] + fn fingerprint_curl_pipe_no_match_without_footprint() { + 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. + fs::create_dir_all(home.join(".local/bin")).unwrap(); + let bin = home.join(".local/bin/mytool"); + File::create(&bin).unwrap(); + + assert!(!fingerprint_curl_pipe(&bin, &home)); + let _ = fs::remove_dir_all(&home); + } + + #[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/claude")).unwrap(); + let bin = PathBuf::from("/tmp/elsewhere/claude"); + + assert!(!fingerprint_curl_pipe(&bin, &home)); + let _ = fs::remove_dir_all(&home); + } + #[test] fn search_output_includes_npm_dirs_when_binary_not_found() { let resolved = resolve_binary("definitely-not-a-real-binary-xyz-123abc"); From 3605c4545c27a9e5b753c4314d33d744c0317733 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 3 Jun 2026 14:44:07 +1000 Subject: [PATCH 12/18] feat(doctor): add latest sources for non-registry installs (Phase B) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase B of the per-agent install-method plan — give curl/native installs a "latest version" path and stop nagging on tools that update themselves. latest_version previously returned None for any source other than Brew/Npm/Cargo, so CurlPipe/Unknown/Mise/Asdf installs (the very tools Phase A just learned to fingerprint) had no freshness signal at all. This adds: - GitHub Releases fetcher: latest_github_releases(owner/repo) mirrors the existing latest_crates_io reqwest pattern — GET releases/latest, parse tag_name (leading v stripped). Degrades gracefully to None on missing token, rate limits, network errors, or unparseable payloads; an optional GITHUB_TOKEN/GH_TOKEN bearer credential relaxes the anonymous rate limit but is never required. JSON parsing is split into parse_github_release_tag for unit testing without a network call. - Explicit per-package latest source: PACKAGE_IDS entries now carry a LatestSource (Brew/Npm/CratesIo/GitHubReleases) alongside the install source, so the fetcher no longer infers the mechanism from InstallSource. This lets Cursor's CurlPipe install resolve to a GitHub-releases lookup (best-effort repo slug, report-only) without overloading CurlPipe to always mean GitHub. Claude native keeps a clear TODO: its channel manifest isn't parsed yet, so it stays report-only. - SelfUpdating marker: is_self_updating(source) treats CurlPipe installs (Claude native, Cursor, Amp-curl) as tool-managed. Per the resolved decision, these are report-only — installed/latest are surfaced for display but update_available is forced to None (never a stale-version nag), and the new DoctorCheck.self_updating flag carries "managed by tool". A brew/npm install of the same agent is not CurlPipe, so it stays user-managed and actionable. Additive and gated behind check_freshness: default run_checks() still does no network/subprocess version probes (self_updating defaults None), so the staged Tauri app's latency profile is unchanged. Adds unit tests for the GitHub tag parser, the self-updating predicate, and the Cursor→GitHubReleases wiring. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 6 ++ crates/doctor/src/checks.rs | 21 +++++ crates/doctor/src/freshness.rs | 136 +++++++++++++++++++++++++------ crates/doctor/src/lib.rs | 29 +++++-- crates/doctor/src/package_ids.rs | 129 ++++++++++++++++++++++------- crates/doctor/src/types.rs | 10 +++ 6 files changed, 268 insertions(+), 63 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index b2aa4ae00..e91b23add 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -233,6 +233,7 @@ pub fn check_single_ai_agent( latest_version: None, update_available: None, install_source: bridge_install_source.clone(), + self_updating: None, } } Ok(output) => { @@ -258,6 +259,7 @@ pub fn check_single_ai_agent( latest_version: None, update_available: None, install_source: bridge_install_source.clone(), + self_updating: None, } } Err(e) => DoctorCheck { @@ -278,6 +280,7 @@ pub fn check_single_ai_agent( latest_version: None, update_available: None, install_source: bridge_install_source.clone(), + self_updating: None, }, } } else { @@ -339,6 +342,7 @@ pub fn check_single_ai_agent( latest_version: None, update_available: None, install_source: bridge_install_source, + self_updating: None, } } } else { @@ -379,6 +383,7 @@ pub fn check_single_ai_agent( latest_version: None, update_available: None, install_source: main_install_source, + self_updating: None, }; } @@ -410,6 +415,7 @@ pub fn check_single_ai_agent( latest_version: None, update_available: None, install_source: None, + self_updating: None, } } } diff --git a/crates/doctor/src/checks.rs b/crates/doctor/src/checks.rs index bc7ef7db4..8de3f817f 100644 --- a/crates/doctor/src/checks.rs +++ b/crates/doctor/src/checks.rs @@ -34,6 +34,7 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, }; } }; @@ -63,6 +64,7 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: resolved.install_source.clone(), + self_updating: None, } } Ok(output) => { @@ -87,6 +89,7 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: resolved.install_source.clone(), + self_updating: None, } } Err(e) => DoctorCheck { @@ -105,6 +108,7 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: resolved.install_source.clone(), + self_updating: None, }, } } @@ -135,6 +139,7 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, }; } }; @@ -165,6 +170,7 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: resolved.install_source.clone(), + self_updating: None, } } Ok(output) => { @@ -189,6 +195,7 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: resolved.install_source.clone(), + self_updating: None, } } Err(e) => DoctorCheck { @@ -207,6 +214,7 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: resolved.install_source.clone(), + self_updating: None, }, } } @@ -236,6 +244,7 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, }; } }; @@ -263,6 +272,7 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, } } else { let stderr = String::from_utf8_lossy(&output.stderr); @@ -288,6 +298,7 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, } } } @@ -307,6 +318,7 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, }, } } @@ -340,6 +352,7 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh latest_version: None, update_available: None, install_source: None, + self_updating: None, }; } }; @@ -372,6 +385,7 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh latest_version: None, update_available: None, install_source: git_lfs.install_source.clone(), + self_updating: None, } } Ok(output) => { @@ -396,6 +410,7 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh latest_version: None, update_available: None, install_source: None, + self_updating: None, } } Err(e) => DoctorCheck { @@ -414,6 +429,7 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh latest_version: None, update_available: None, install_source: None, + self_updating: None, }, } } @@ -444,6 +460,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, }; } }; @@ -477,6 +494,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: git.install_source.clone(), + self_updating: None, } } else { DoctorCheck { @@ -496,6 +514,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: git.install_source.clone(), + self_updating: None, } } } @@ -521,6 +540,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: git.install_source.clone(), + self_updating: None, } } Err(e) => DoctorCheck { @@ -541,6 +561,7 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { latest_version: None, update_available: None, install_source: git.install_source.clone(), + self_updating: None, }, } } diff --git a/crates/doctor/src/freshness.rs b/crates/doctor/src/freshness.rs index 49cc6ae83..f960a7f50 100644 --- a/crates/doctor/src/freshness.rs +++ b/crates/doctor/src/freshness.rs @@ -13,6 +13,7 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde::{Deserialize, Serialize}; use serde_json::Value; +use crate::package_ids::LatestSource; use crate::types::InstallSource; /// One hour: long enough that repeated `run_checks_with_options` calls in the @@ -42,11 +43,11 @@ pub(crate) struct FreshnessCache { } impl FreshnessCache { - fn cache_key(source: InstallSource, package_id: &str) -> String { + fn cache_key(source: LatestSource, package_id: &str) -> String { format!("{:?}:{}", source, package_id) } - fn get_fresh(&self, source: InstallSource, package_id: &str, now: i64) -> Option { + fn get_fresh(&self, source: LatestSource, package_id: &str, now: i64) -> Option { let key = Self::cache_key(source, package_id); let entry = self.entries.get(&key)?; if now.saturating_sub(entry.fetched_at) <= CACHE_TTL_SECONDS { @@ -56,7 +57,7 @@ impl FreshnessCache { } } - fn insert(&mut self, source: InstallSource, package_id: &str, value: String, now: i64) { + fn insert(&mut self, source: LatestSource, package_id: &str, value: String, now: i64) { let key = Self::cache_key(source, package_id); self.entries.insert( key, @@ -222,21 +223,27 @@ fn installed_version(binary_path: &Path, version_args: &[&str]) -> Option) -> bool { + matches!(source, Some(InstallSource::CurlPipe)) +} + /// Dispatch a latest-version probe per source. fn latest_version( - source: InstallSource, + source: LatestSource, package_id: &str, npm_registry: Option<&str>, ) -> Option { match source { - InstallSource::Brew => latest_brew(package_id), - InstallSource::Npm => latest_npm(package_id, npm_registry), - InstallSource::Cargo => latest_crates_io(package_id), - InstallSource::CurlPipe - | InstallSource::System - | InstallSource::Unknown - | InstallSource::Mise - | InstallSource::Asdf => None, + LatestSource::Brew => latest_brew(package_id), + LatestSource::Npm => latest_npm(package_id, npm_registry), + LatestSource::CratesIo => latest_crates_io(package_id), + LatestSource::GitHubReleases => latest_github_releases(package_id), } } @@ -313,11 +320,60 @@ fn latest_crates_io(package_id: &str) -> Option { .map(|s| s.to_string()) } +/// Fetch the latest release tag for a `owner/repo` slug via the GitHub REST API. +/// +/// Used for tools published only through GitHub releases (no brew/npm/crates +/// presence), e.g. Cursor's curl install. Degrades gracefully to `None` on any +/// error — no token, rate-limited, network failure, or unparseable payload — +/// and never returns a hard error. A `GITHUB_TOKEN`/`GH_TOKEN` in the +/// environment is used as a bearer credential to relax the unauthenticated +/// rate limit, but is entirely optional. +fn latest_github_releases(repo: &str) -> Option { + let url = format!("https://api.github.com/repos/{repo}/releases/latest"); + let client = reqwest::blocking::Client::builder() + // GitHub rejects requests without a User-Agent. + .user_agent("block-builderbot-doctor/0.1") + .timeout(std::time::Duration::from_secs(10)) + .build() + .ok()?; + let mut req = client + .get(&url) + .header("Accept", "application/vnd.github+json"); + if let Some(token) = github_token() { + req = req.bearer_auth(token); + } + let resp = req.send().ok()?; + if !resp.status().is_success() { + return None; + } + let bytes = resp.bytes().ok()?; + parse_github_release_tag(&bytes) +} + +/// Read GitHub's optional release-auth token from the environment. Empty values +/// are treated as absent. Kept separate so the fetcher stays testable. +fn github_token() -> Option { + std::env::var("GITHUB_TOKEN") + .ok() + .or_else(|| std::env::var("GH_TOKEN").ok()) + .filter(|s| !s.is_empty()) +} + +/// Pull `tag_name` out of a GitHub `releases/latest` payload, stripping a +/// leading `v` so it lines up with the semver shapes the rest of the module +/// produces. Crate-public so unit tests can drive it without a network call. +pub(crate) fn parse_github_release_tag(bytes: &[u8]) -> Option { + let root: Value = serde_json::from_slice(bytes).ok()?; + root.get("tag_name") + .and_then(|v| v.as_str()) + .map(|s| s.trim_start_matches('v').to_string()) +} + /// Top-level: returns installed (always, when binary present), latest (only /// if `package_id` is `Some` and we're not offline), and `update_available` /// (only if both parse as semver triples). pub(crate) async fn fetch_version_info( - install_source: Option, + latest_source: Option, package_id: Option<&str>, binary_path: &Path, version_args: &[&str], @@ -342,18 +398,16 @@ pub(crate) async fn fetch_version_info( let latest = if offline { None - } else if let (Some(source), Some(pkg)) = (install_source, pkg) { + } else if let (Some(source), Some(pkg)) = (latest_source, pkg) { let now = now_epoch_seconds(); // Cache lookup first. let cached = { let guard = cache.lock().ok(); - guard - .as_ref() - .and_then(|c| c.get_fresh(source.clone(), &pkg, now)) + guard.as_ref().and_then(|c| c.get_fresh(source, &pkg, now)) }; if let Some(v) = cached { Some(v) - } else if let Some(v) = latest_version(source.clone(), &pkg, npm_registry.as_deref()) { + } else if let Some(v) = latest_version(source, &pkg, npm_registry.as_deref()) { if let Ok(mut guard) = cache.lock() { guard.insert(source, &pkg, v.clone(), now); } @@ -467,26 +521,62 @@ mod tests { fn cache_ttl_treats_old_entries_as_stale() { let mut cache = FreshnessCache::default(); let now = 1_000_000; - cache.insert(InstallSource::Brew, "git", "2.50.0".to_string(), now); + cache.insert(LatestSource::Brew, "git", "2.50.0".to_string(), now); // Within TTL. assert_eq!( - cache.get_fresh(InstallSource::Brew, "git", now + 60), + cache.get_fresh(LatestSource::Brew, "git", now + 60), Some("2.50.0".to_string()), ); // 2 hours later — stale. let two_hours_later = now + 2 * 60 * 60; assert_eq!( - cache.get_fresh(InstallSource::Brew, "git", two_hours_later), + cache.get_fresh(LatestSource::Brew, "git", two_hours_later), None, ); } #[test] fn cache_key_uses_source_and_package_id() { - let k1 = FreshnessCache::cache_key(InstallSource::Brew, "git"); - let k2 = FreshnessCache::cache_key(InstallSource::Npm, "git"); + let k1 = FreshnessCache::cache_key(LatestSource::Brew, "git"); + let k2 = FreshnessCache::cache_key(LatestSource::Npm, "git"); assert_ne!(k1, k2, "different sources should map to different keys"); } + + #[test] + fn github_release_tag_strips_leading_v() { + let json = br#"{"tag_name": "v1.2.3", "name": "1.2.3"}"#; + assert_eq!( + parse_github_release_tag(json).as_deref(), + Some("1.2.3"), + "leading v should be stripped", + ); + } + + #[test] + fn github_release_tag_without_v_prefix() { + let json = br#"{"tag_name": "2025.06.01"}"#; + assert_eq!( + parse_github_release_tag(json).as_deref(), + Some("2025.06.01") + ); + } + + #[test] + fn github_release_tag_missing_field_is_none() { + assert_eq!( + parse_github_release_tag(br#"{"name": "no tag here"}"#), + None + ); + assert_eq!(parse_github_release_tag(b"not json"), None); + } + + #[test] + fn self_updating_only_for_curl_pipe() { + assert!(is_self_updating(Some(&InstallSource::CurlPipe))); + assert!(!is_self_updating(Some(&InstallSource::Npm))); + assert!(!is_self_updating(Some(&InstallSource::Brew))); + assert!(!is_self_updating(None)); + } } diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index 394bf6199..e01f374e9 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -19,10 +19,10 @@ use std::sync::{Arc, Mutex}; use agents::{check_single_ai_agent, lookup_fix_command, AI_AGENT_CHECKS}; use checks::{check_clonefile, check_gh, check_gh_auth, check_git, check_git_lfs}; -use freshness::{fetch_version_info, load_cache, save_cache}; -use package_ids::lookup_package_id; +use freshness::{fetch_version_info, is_self_updating, load_cache, save_cache}; +use package_ids::{lookup_package_id, LatestSource}; use resolve::resolve_binary; -use types::{InstallSource, ResolvedBinary}; +use types::ResolvedBinary; /// Fallback check returned when a spawn_blocking task panics. fn empty_check(id: &str, label: &str) -> DoctorCheck { @@ -42,6 +42,7 @@ fn empty_check(id: &str, label: &str) -> DoctorCheck { latest_version: None, update_available: None, install_source: None, + self_updating: None, } } @@ -212,16 +213,17 @@ async fn populate_freshness( let path_str = check.bridge_path.as_deref().or(check.path.as_deref()); let Some(path_str) = path_str else { continue }; - let package_id = check + let (package_id, latest_source) = check .install_source .clone() .and_then(|src| lookup_package_id(&check.id, src)) - .map(|s| s.to_string()); + .map(|(pkg, latest)| (Some(pkg.to_string()), Some(latest))) + .unwrap_or((None, None)); targets.push(FreshnessTarget { id: check.id.clone(), path: PathBuf::from(path_str), - install_source: check.install_source.clone(), + latest_source, package_id, }); } @@ -231,7 +233,7 @@ async fn populate_freshness( let npm_registry = npm_registry.clone(); async move { let info = fetch_version_info( - t.install_source, + t.latest_source, t.package_id.as_deref(), &t.path, &["--version"], @@ -254,7 +256,16 @@ async fn populate_freshness( if let Some(info) = by_id.remove(&check.id) { check.installed_version = info.installed; check.latest_version = info.latest; - check.update_available = info.update_available; + // 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(check.install_source.as_ref()); + check.self_updating = Some(self_updating); + check.update_available = if self_updating { + None + } else { + info.update_available + }; } } @@ -268,7 +279,7 @@ async fn populate_freshness( struct FreshnessTarget { id: String, path: PathBuf, - install_source: Option, + latest_source: Option, package_id: Option, } diff --git a/crates/doctor/src/package_ids.rs b/crates/doctor/src/package_ids.rs index 3d09435d5..8dd85ea4a 100644 --- a/crates/doctor/src/package_ids.rs +++ b/crates/doctor/src/package_ids.rs @@ -1,59 +1,114 @@ //! Per-check package identifiers for version-freshness lookups. //! -//! Maps a doctor check ID to one or more `(InstallSource, package_id)` pairs. -//! The freshness module dispatches to the registry that matches the binary's -//! detected install source. Checks not listed here (or with no matching -//! source) skip the latest-version probe. +//! Maps a doctor check ID to one or more `(InstallSource, package_id, +//! LatestSource)` entries. The freshness module picks the entry whose +//! `InstallSource` matches the binary's detected install source, then fetches +//! the latest version via the entry's `LatestSource`. Checks not listed here +//! (or with no matching source) skip the latest-version probe. use crate::types::InstallSource; -/// Static table of `check_id -> &[(install_source, package_id)]`. +/// How to fetch the "latest available" version for a package. +/// +/// For registry installs this mirrors the install source 1:1 (a brew binary +/// checks brew, an npm binary checks npm). Non-registry installs (curl/native) +/// need an explicit mechanism that doesn't follow from the install source — +/// e.g. Cursor's curl install has no registry presence and is tracked via +/// GitHub releases, so its `CurlPipe` entry fetches from `GitHubReleases`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum LatestSource { + Brew, + Npm, + /// Retained so a future cargo-installed tool can reach `latest_crates_io`; + /// no entry in `PACKAGE_IDS` selects it yet (no agent ships via crates.io). + #[allow(dead_code)] + CratesIo, + GitHubReleases, +} + +/// One freshness entry: the install source it applies to, the package id to +/// query, and how to fetch that package's latest version. +type PackageEntry = (InstallSource, &'static str, LatestSource); + +/// Static table of `check_id -> &[PackageEntry]`. /// /// A single check can have multiple entries when the same agent ships through /// different registries (e.g. brew for the main binary, npm for the ACP bridge). /// `lookup_package_id` picks the first entry whose `install_source` matches the /// binary's detected source. -pub(crate) const PACKAGE_IDS: &[(&str, &[(InstallSource, &str)])] = &[ - ("git", &[(InstallSource::Brew, "git")]), - ("gh", &[(InstallSource::Brew, "gh")]), - ("git-lfs", &[(InstallSource::Brew, "git-lfs")]), +pub(crate) const PACKAGE_IDS: &[(&str, &[PackageEntry])] = &[ + ("git", &[(InstallSource::Brew, "git", LatestSource::Brew)]), + ("gh", &[(InstallSource::Brew, "gh", LatestSource::Brew)]), + ( + "git-lfs", + &[(InstallSource::Brew, "git-lfs", LatestSource::Brew)], + ), // ai-agent-goose: brew tap exists but no canonical formula yet — skip. // TODO: revisit when block/goose lands a stable brew formula. ( "ai-agent-claude", - &[(InstallSource::Npm, "@agentclientprotocol/claude-agent-acp")], + &[( + InstallSource::Npm, + "@agentclientprotocol/claude-agent-acp", + LatestSource::Npm, + )], + // TODO: the main `claude` native (CurlPipe) install has no registry + // entry — its latest is published via the native installer's channel + // manifest, which we don't parse yet. Claude native is self-updating + // (see `freshness::is_self_updating`), so it stays report-only for now. ), ( "ai-agent-codex", &[ - (InstallSource::Npm, "@zed-industries/codex-acp"), - (InstallSource::Brew, "codex"), + ( + InstallSource::Npm, + "@zed-industries/codex-acp", + LatestSource::Npm, + ), + (InstallSource::Brew, "codex", LatestSource::Brew), ], ), - ("ai-agent-pi", &[(InstallSource::Npm, "pi-acp")]), + ( + "ai-agent-pi", + &[(InstallSource::Npm, "pi-acp", LatestSource::Npm)], + ), ( "ai-agent-amp", &[ - (InstallSource::Npm, "amp-acp"), - (InstallSource::Brew, "amp"), + (InstallSource::Npm, "amp-acp", LatestSource::Npm), + (InstallSource::Brew, "amp", LatestSource::Brew), ], ), ( "ai-agent-copilot", - &[(InstallSource::Npm, "@github/copilot")], + &[(InstallSource::Npm, "@github/copilot", LatestSource::Npm)], + ), + // ai-agent-cursor: curl-pipe installer with no registry presence; its + // releases are published on GitHub. The repo slug is a best-effort default + // and the GitHub fetcher degrades to `None` on a miss. Cursor is + // self-updating, so this latest is report-only (no update nag). + ( + "ai-agent-cursor", + &[( + InstallSource::CurlPipe, + "getcursor/cursor", + LatestSource::GitHubReleases, + )], ), - // ai-agent-cursor: curl-pipe installer with no registry presence. - // TODO: GitHub releases would be a reasonable follow-up source. ]; -/// Pick the package id whose source matches `source`. Returns `None` if the -/// check isn't in the table, or if no entry matches. -pub(crate) fn lookup_package_id(check_id: &str, source: InstallSource) -> Option<&'static str> { +/// Pick the package id and its latest-version source for the entry whose +/// install source matches `source`. Returns `None` if the check isn't in the +/// table, or if no entry matches. +pub(crate) fn lookup_package_id( + check_id: &str, + source: InstallSource, +) -> Option<(&'static str, LatestSource)> { for (id, entries) in PACKAGE_IDS { if *id == check_id { - for (entry_source, pkg) in entries.iter() { + for (entry_source, pkg, latest) in entries.iter() { if entry_source == &source { - return Some(*pkg); + return Some((*pkg, *latest)); } } return None; @@ -68,10 +123,13 @@ mod tests { #[test] fn lookup_matches_source() { - assert_eq!(lookup_package_id("git", InstallSource::Brew), Some("git"),); + assert_eq!( + lookup_package_id("git", InstallSource::Brew), + Some(("git", LatestSource::Brew)), + ); assert_eq!( lookup_package_id("ai-agent-claude", InstallSource::Npm), - Some("@agentclientprotocol/claude-agent-acp"), + Some(("@agentclientprotocol/claude-agent-acp", LatestSource::Npm)), ); } @@ -83,16 +141,25 @@ mod tests { #[test] fn lookup_returns_none_for_unknown_check() { - assert_eq!( - lookup_package_id("ai-agent-cursor", InstallSource::CurlPipe), - None, - ); assert_eq!(lookup_package_id("nonexistent", InstallSource::Brew), None); } #[test] fn codex_has_both_npm_and_brew_entries() { - assert!(lookup_package_id("ai-agent-codex", InstallSource::Npm).is_some()); - assert!(lookup_package_id("ai-agent-codex", InstallSource::Brew).is_some()); + assert_eq!( + lookup_package_id("ai-agent-codex", InstallSource::Npm), + Some(("@zed-industries/codex-acp", LatestSource::Npm)), + ); + assert_eq!( + lookup_package_id("ai-agent-codex", InstallSource::Brew), + Some(("codex", LatestSource::Brew)), + ); + } + + #[test] + fn cursor_curl_pipe_resolves_to_github_releases() { + let (_, latest) = + lookup_package_id("ai-agent-cursor", InstallSource::CurlPipe).expect("cursor entry"); + assert_eq!(latest, LatestSource::GitHubReleases); } } diff --git a/crates/doctor/src/types.rs b/crates/doctor/src/types.rs index 93c8796fa..9470bc102 100644 --- a/crates/doctor/src/types.rs +++ b/crates/doctor/src/types.rs @@ -80,9 +80,19 @@ pub struct DoctorCheck { /// Latest available version string, if known. pub latest_version: Option, /// Whether a newer version is available than the installed one. + /// + /// For self-updating tools (see `self_updating`) this is never `Some(true)`: + /// the tool manages its own freshness, so we don't raise an update nag even + /// when a newer version exists upstream. pub update_available: Option, /// How the binary was installed (brew, npm, cargo, etc.), if detected. pub install_source: Option, + /// Whether this tool keeps itself up to date (curl/native installers such as + /// Claude native, Cursor, and Amp-curl). When `Some(true)`, the freshness + /// pass reports the installed/latest versions for display but suppresses + /// `update_available` — the update is "managed by the tool", not actionable. + /// Only populated by the freshness pass; `None` on the default cheap path. + pub self_updating: Option, } /// The full report returned to the frontend. From a49ec1a8e2ca6cbf38db616da485c4e5dcd94695 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 3 Jun 2026 14:53:27 +1000 Subject: [PATCH 13/18] feat(doctor): split main CLI vs ACP bridge into two readouts (Phase C) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase C of the per-agent install-method plan — stop collapsing an agent's own CLI and its ACP bridge into a single install_source/version. Per the resolved decision, surface both independently. - Add `AgentVersionInfo { install_source, installed_version, latest_version, update_available, self_updating }` (Default, camelCase) and two Option-defaulting-None fields on `DoctorCheck`: `main` (the agent CLI / the single binary for agents with no separate bridge) and `bridge` (the ACP bridge, when distinct). Non-agent checks and absent binaries leave the corresponding readout `None`. - `check_single_ai_agent` already resolves both binaries; it previously kept only the bridge's source and discarded the main CLI's. It now builds both readouts: when the agent has a separate `main_command`, the curl/native install-source override applies to the main CLI while the bridge keeps its positively-detected source; when there is no separate bridge, the single resolved binary is reported under `main`. - Freshness fans out to both readouts: `populate_freshness` builds per-readout targets (Main/Bridge) keyed by each readout's own install source, so e.g. Codex probes brew for its main CLI and npm for its bridge in the same pass. `PACKAGE_IDS` already carried the multiple `(source, package_id, LatestSource)` pairs this needs — no table redesign. Non-agent checks keep a Flat target. - Backward compat: the flat version fields stay populated by mirroring the headline readout (bridge if present, else main) — byte-identical to the pre-split pass. Existing consumers (and the staged Tauri app) are unaffected. Additive and gated behind `check_freshness`: default `run_checks()` still does no network/subprocess version probes (readouts carry only install_source on the cheap path), so the staged app's latency profile is unchanged. Adds unit tests for the two-readout split, override-on-main-only, single-binary-as-main, the unresolved case, and the no-freshness invariant on the readouts. The TypeScript `DoctorCheck` mirror lives in goose-internal and is updated when that repo re-pins the crate (Phases D/E) — out of scope here. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 152 +++++++++++++++++++++++++++++++- crates/doctor/src/checks.rs | 42 +++++++++ crates/doctor/src/lib.rs | 170 ++++++++++++++++++++++++++++++------ crates/doctor/src/types.rs | 38 ++++++++ 4 files changed, 373 insertions(+), 29 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index e91b23add..fc53a9a59 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -5,7 +5,9 @@ use std::process::Command; use crate::checks::CLONEFILE_FIX_COMMAND; use crate::execute_command_blocking; use crate::resolve::format_command_output; -use crate::types::{AuthStatus, CheckStatus, DoctorCheck, FixType, InstallSource, ResolvedBinary}; +use crate::types::{ + AgentVersionInfo, AuthStatus, CheckStatus, DoctorCheck, FixType, InstallSource, ResolvedBinary, +}; /// Metadata for an individual AI agent check. pub struct AgentCheckInfo { @@ -174,6 +176,17 @@ fn apply_install_source_override( } } +/// Build a per-binary version readout from a resolved install source. Returns +/// `Some` only when the binary was resolved (i.e. a source was detected); the +/// version fields stay `None` until the freshness pass fills them in. A binary +/// that wasn't found (`None`) yields `None` — no empty readout is fabricated. +fn version_readout(source: Option) -> Option { + source.map(|src| AgentVersionInfo { + install_source: Some(src), + ..AgentVersionInfo::default() + }) +} + /// Check whether a single AI agent is installed. /// /// `npm_registry`, when `Some`, routes any npm-backed install/bridge fix @@ -234,6 +247,10 @@ pub fn check_single_ai_agent( update_available: None, install_source: bridge_install_source.clone(), self_updating: None, + // Goose has no separate ACP bridge — the single binary + // is the agent CLI, reported under `main`. + main: version_readout(bridge_install_source.clone()), + bridge: None, } } Ok(output) => { @@ -260,6 +277,10 @@ pub fn check_single_ai_agent( update_available: None, install_source: bridge_install_source.clone(), self_updating: None, + // Goose has no separate ACP bridge — the single binary + // is the agent CLI, reported under `main`. + main: version_readout(bridge_install_source.clone()), + bridge: None, } } Err(e) => DoctorCheck { @@ -281,6 +302,8 @@ pub fn check_single_ai_agent( update_available: None, install_source: bridge_install_source.clone(), self_updating: None, + main: version_readout(bridge_install_source.clone()), + bridge: None, }, } } else { @@ -293,6 +316,23 @@ pub fn check_single_ai_agent( (resolved_path, None) }; + // Surface the agent CLI and its ACP bridge as two independent + // readouts. When the agent has a separate `main_command`, the + // install-source override (a curl/native hint) applies to the main + // CLI; the bridge keeps its positively-detected source. When there + // is no separate bridge, the single resolved binary is the agent + // CLI itself, reported under `main` (bridge stays `None`). + let (main_readout, bridge_readout) = if info.main_command.is_some() { + let main_src = apply_install_source_override( + resolved_main.and_then(|rb| rb.install_source.clone()), + info.install_source_override.as_ref(), + ); + let bridge_src = resolved_bridge.and_then(|rb| rb.install_source.clone()); + (version_readout(main_src), version_readout(bridge_src)) + } else { + (version_readout(bridge_install_source.clone()), None) + }; + let (auth_status, auth_output) = match info.auth_status_command { Some(cmd) => match execute_command_blocking(cmd) { Ok(()) => ( @@ -343,6 +383,8 @@ pub fn check_single_ai_agent( update_available: None, install_source: bridge_install_source, self_updating: None, + main: main_readout, + bridge: bridge_readout, } } } else { @@ -382,8 +424,11 @@ pub fn check_single_ai_agent( installed_version: None, latest_version: None, update_available: None, - install_source: main_install_source, + install_source: main_install_source.clone(), self_updating: None, + // Bridge is absent; only the agent CLI resolved. + main: version_readout(main_install_source), + bridge: None, }; } @@ -416,6 +461,8 @@ pub fn check_single_ai_agent( update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, } } } @@ -446,6 +493,107 @@ pub fn lookup_fix_command(check_id: &str, fix_type: &FixType) -> Option #[cfg(test)] mod tests { use super::*; + use std::path::PathBuf; + + fn resolved(path: Option<&str>, source: Option) -> ResolvedBinary { + ResolvedBinary { + path: path.map(PathBuf::from), + search_output: String::new(), + install_source: source, + } + } + + fn agent(id: &str) -> &'static AgentCheckInfo { + AI_AGENT_CHECKS.iter().find(|i| i.id == id).unwrap() + } + + /// An agent with a separate ACP bridge surfaces both binaries as independent + /// readouts, each carrying its own install source. Uses Pi (no auth + /// commands) so the check runs without shelling out. + #[test] + fn agent_check_populates_main_and_bridge_readouts() { + let bridge = resolved(Some("/n/bin/pi-acp"), Some(InstallSource::Npm)); + let main = resolved(Some("/c/bin/pi"), Some(InstallSource::Cargo)); + let check = check_single_ai_agent( + agent("ai-agent-pi"), + true, + std::slice::from_ref(&bridge), + Some(&main), + None, + ); + + let m = check.main.expect("main readout present"); + assert_eq!(m.install_source, Some(InstallSource::Cargo)); + let b = check.bridge.expect("bridge readout present"); + assert_eq!(b.install_source, Some(InstallSource::Npm)); + // Flat field mirrors the bridge (headline) readout for back-compat. + assert_eq!(check.install_source, Some(InstallSource::Npm)); + // Version fields stay empty on the cheap (no-freshness) path. + assert!(m.installed_version.is_none()); + assert!(b.installed_version.is_none()); + assert!(m.self_updating.is_none()); + } + + /// When only the agent CLI resolves (no bridge), the main readout carries + /// the CLI's source — upgraded from `Unknown` via the per-agent override — + /// and the bridge readout is `None`. Claude's main-only path returns before + /// any auth probe, so this runs without shelling out. + #[test] + fn agent_check_main_only_applies_override_and_omits_bridge() { + let bridge_missing = resolved(None, None); + let main = resolved(Some("/h/.local/bin/claude"), Some(InstallSource::Unknown)); + let check = check_single_ai_agent( + agent("ai-agent-claude"), + true, + std::slice::from_ref(&bridge_missing), + Some(&main), + None, + ); + + let m = check.main.expect("main readout present"); + assert_eq!( + m.install_source, + Some(InstallSource::CurlPipe), + "Unknown main CLI upgraded to CurlPipe via override", + ); + assert!(check.bridge.is_none(), "no bridge resolved"); + assert_eq!(check.install_source, Some(InstallSource::CurlPipe)); + } + + /// An agent with no separate bridge reports its single binary under `main`, + /// leaving `bridge` as `None`. Copilot has an auth command but no + /// auth-status command, so it reports NotApplicable without shelling out. + #[test] + fn agent_without_bridge_reports_single_binary_as_main() { + let single = resolved(Some("/n/bin/copilot"), Some(InstallSource::Npm)); + let check = check_single_ai_agent( + agent("ai-agent-copilot"), + true, + std::slice::from_ref(&single), + None, + None, + ); + + let m = check.main.expect("main readout present"); + assert_eq!(m.install_source, Some(InstallSource::Npm)); + assert!(check.bridge.is_none()); + assert_eq!(check.install_source, Some(InstallSource::Npm)); + } + + /// A fully unresolved agent carries neither readout. + #[test] + fn unresolved_agent_has_no_readouts() { + let missing = resolved(None, None); + let check = check_single_ai_agent( + agent("ai-agent-pi"), + false, + std::slice::from_ref(&missing), + Some(&missing), + None, + ); + assert!(check.main.is_none()); + assert!(check.bridge.is_none()); + } #[test] fn auth_fix_lookup_returns_agent_auth_command() { diff --git a/crates/doctor/src/checks.rs b/crates/doctor/src/checks.rs index 8de3f817f..897f942ae 100644 --- a/crates/doctor/src/checks.rs +++ b/crates/doctor/src/checks.rs @@ -35,6 +35,8 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, }; } }; @@ -65,6 +67,8 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: resolved.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } Ok(output) => { @@ -90,6 +94,8 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: resolved.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } Err(e) => DoctorCheck { @@ -109,6 +115,8 @@ pub fn check_git(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: resolved.install_source.clone(), self_updating: None, + main: None, + bridge: None, }, } } @@ -140,6 +148,8 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, }; } }; @@ -171,6 +181,8 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: resolved.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } Ok(output) => { @@ -196,6 +208,8 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: resolved.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } Err(e) => DoctorCheck { @@ -215,6 +229,8 @@ pub fn check_gh(resolved: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: resolved.install_source.clone(), self_updating: None, + main: None, + bridge: None, }, } } @@ -245,6 +261,8 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, }; } }; @@ -273,6 +291,8 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, } } else { let stderr = String::from_utf8_lossy(&output.stderr); @@ -299,6 +319,8 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, } } } @@ -319,6 +341,8 @@ pub fn check_gh_auth(gh: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, }, } } @@ -353,6 +377,8 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, }; } }; @@ -386,6 +412,8 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh update_available: None, install_source: git_lfs.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } Ok(output) => { @@ -411,6 +439,8 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, } } Err(e) => DoctorCheck { @@ -430,6 +460,8 @@ pub fn check_git_lfs(git: &ResolvedBinary, git_lfs: &ResolvedBinary) -> DoctorCh update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, }, } } @@ -461,6 +493,8 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, }; } }; @@ -495,6 +529,8 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: git.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } else { DoctorCheck { @@ -515,6 +551,8 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: git.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } } @@ -541,6 +579,8 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: git.install_source.clone(), self_updating: None, + main: None, + bridge: None, } } Err(e) => DoctorCheck { @@ -562,6 +602,8 @@ pub fn check_clonefile(git: &ResolvedBinary) -> DoctorCheck { update_available: None, install_source: git.install_source.clone(), self_updating: None, + main: None, + bridge: None, }, } } diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index e01f374e9..eed8c44c0 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -22,7 +22,7 @@ use checks::{check_clonefile, check_gh, check_gh_auth, check_git, check_git_lfs} use freshness::{fetch_version_info, is_self_updating, load_cache, save_cache}; use package_ids::{lookup_package_id, LatestSource}; use resolve::resolve_binary; -use types::ResolvedBinary; +use types::{AgentVersionInfo, InstallSource, ResolvedBinary}; /// Fallback check returned when a spawn_blocking task panics. fn empty_check(id: &str, label: &str) -> DoctorCheck { @@ -43,6 +43,8 @@ fn empty_check(id: &str, label: &str) -> DoctorCheck { update_available: None, install_source: None, self_updating: None, + main: None, + bridge: None, } } @@ -194,10 +196,61 @@ async fn collect_base_report(npm_registry: Option<&str>) -> DoctorReport { DoctorReport { checks } } +/// Which binary behind a check a freshness probe targets. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +enum ReadoutSlot { + /// The agent's own CLI (or, for agents without a separate bridge, the + /// single resolved binary). Maps to `DoctorCheck.path` / `DoctorCheck.main`. + Main, + /// The agent's ACP bridge. Maps to `DoctorCheck.bridge_path` / + /// `DoctorCheck.bridge`. + Bridge, + /// A non-agent check (git, gh, …) with no main/bridge split. Maps directly + /// to the flat version fields on `DoctorCheck`. + Flat, +} + +/// Resolve the `(package_id, latest_source)` pair for a check + install source. +/// `None`/`None` when the source is missing or the check has no matching entry. +fn resolve_package( + check_id: &str, + source: Option<&InstallSource>, +) -> (Option, Option) { + source + .cloned() + .and_then(|src| lookup_package_id(check_id, src)) + .map(|(pkg, latest)| (Some(pkg.to_string()), Some(latest))) + .unwrap_or((None, None)) +} + +/// Fold a freshness [`freshness::VersionInfo`] into a per-binary readout, +/// applying the self-updating suppression rule for `update_available`. +fn apply_freshness(readout: &mut AgentVersionInfo, info: &freshness::VersionInfo) { + readout.installed_version = info.installed.clone(); + readout.latest_version = info.latest.clone(); + // 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()); + readout.self_updating = Some(self_updating); + readout.update_available = if self_updating { + None + } else { + info.update_available + }; +} + /// Post-hoc pass: for every check that has a usable binary path and a known /// package id, run the installed/latest version probes in parallel and update /// the corresponding fields on the report. The on-disk cache is read once at /// the start and written once at the end. +/// +/// Agent checks front up to two independent binaries — the agent CLI (`main`) +/// and its ACP bridge (`bridge`) — and each is probed and reported separately. +/// The flat version fields are kept in sync for backward compatibility: they +/// mirror the bridge readout when a bridge exists, otherwise the main readout +/// (the same headline the pre-split pass produced). Non-agent checks keep +/// writing straight to the flat fields. async fn populate_freshness( mut report: DoctorReport, offline: bool, @@ -208,24 +261,45 @@ async fn populate_freshness( let mut targets: Vec = Vec::new(); for check in &report.checks { - // Prefer the bridge path when present (matches the install_source the - // check carries — see check_single_ai_agent). - let path_str = check.bridge_path.as_deref().or(check.path.as_deref()); - let Some(path_str) = path_str else { continue }; - - let (package_id, latest_source) = check - .install_source - .clone() - .and_then(|src| lookup_package_id(&check.id, src)) - .map(|(pkg, latest)| (Some(pkg.to_string()), Some(latest))) - .unwrap_or((None, None)); - - targets.push(FreshnessTarget { - id: check.id.clone(), - path: PathBuf::from(path_str), - latest_source, - package_id, - }); + let is_agent = check.main.is_some() || check.bridge.is_some(); + if is_agent { + if let (Some(readout), Some(path)) = (&check.main, check.path.as_deref()) { + let (package_id, latest_source) = + resolve_package(&check.id, readout.install_source.as_ref()); + targets.push(FreshnessTarget { + id: check.id.clone(), + slot: ReadoutSlot::Main, + path: PathBuf::from(path), + latest_source, + package_id, + }); + } + if let (Some(readout), Some(path)) = (&check.bridge, check.bridge_path.as_deref()) { + let (package_id, latest_source) = + resolve_package(&check.id, readout.install_source.as_ref()); + targets.push(FreshnessTarget { + id: check.id.clone(), + slot: ReadoutSlot::Bridge, + path: PathBuf::from(path), + latest_source, + package_id, + }); + } + } else { + // Non-agent check: prefer the bridge path when present (matches the + // install_source the check carries — see check_single_ai_agent). + let path_str = check.bridge_path.as_deref().or(check.path.as_deref()); + let Some(path_str) = path_str else { continue }; + let (package_id, latest_source) = + resolve_package(&check.id, check.install_source.as_ref()); + targets.push(FreshnessTarget { + id: check.id.clone(), + slot: ReadoutSlot::Flat, + path: PathBuf::from(path_str), + latest_source, + package_id, + }); + } } let futures = targets.into_iter().map(|t| { @@ -242,23 +316,50 @@ async fn populate_freshness( cache, ) .await; - (t.id, info) + ((t.id, t.slot), info) } }); let results = futures::future::join_all(futures).await; - let mut by_id: HashMap = HashMap::new(); - for (id, info) in results { - by_id.insert(id, info); + let mut by_target: HashMap<(String, ReadoutSlot), freshness::VersionInfo> = HashMap::new(); + for (key, info) in results { + by_target.insert(key, info); } for check in &mut report.checks { - if let Some(info) = by_id.remove(&check.id) { + let is_agent = check.main.is_some() || check.bridge.is_some(); + if is_agent { + if let (Some(readout), Some(info)) = ( + check.main.as_mut(), + by_target.remove(&(check.id.clone(), ReadoutSlot::Main)), + ) { + apply_freshness(readout, &info); + } + if let (Some(readout), Some(info)) = ( + check.bridge.as_mut(), + by_target.remove(&(check.id.clone(), ReadoutSlot::Bridge)), + ) { + apply_freshness(readout, &info); + } + // Mirror the headline readout (bridge if present, else main) into the + // flat fields for backward-compatible consumers. + let headline = check.bridge.as_ref().or(check.main.as_ref()).map(|r| { + ( + r.installed_version.clone(), + r.latest_version.clone(), + r.update_available, + r.self_updating, + ) + }); + if let Some((installed, latest, update_available, self_updating)) = headline { + check.installed_version = installed; + check.latest_version = latest; + check.update_available = update_available; + check.self_updating = self_updating; + } + } else if let Some(info) = by_target.remove(&(check.id.clone(), ReadoutSlot::Flat)) { check.installed_version = info.installed; check.latest_version = info.latest; - // 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(check.install_source.as_ref()); check.self_updating = Some(self_updating); check.update_available = if self_updating { @@ -278,6 +379,7 @@ async fn populate_freshness( struct FreshnessTarget { id: String, + slot: ReadoutSlot, path: PathBuf, latest_source: Option, package_id: Option, @@ -527,6 +629,20 @@ mod tests { check.id, check.update_available, ); + // The split main/bridge readouts may carry an install source on the + // cheap path, but never version fields — those are freshness-only. + for (slot, readout) in [("main", &check.main), ("bridge", &check.bridge)] { + if let Some(r) = readout { + assert!( + r.installed_version.is_none() + && r.latest_version.is_none() + && r.update_available.is_none() + && r.self_updating.is_none(), + "check {} {slot} readout unexpectedly populated version fields: {r:?}", + check.id, + ); + } + } } } } diff --git a/crates/doctor/src/types.rs b/crates/doctor/src/types.rs index 9470bc102..ae1bd57fc 100644 --- a/crates/doctor/src/types.rs +++ b/crates/doctor/src/types.rs @@ -47,6 +47,31 @@ pub enum InstallSource { Unknown, } +/// Version + install-source readout for one binary behind an agent check. +/// +/// An AI-agent check may front two distinct binaries — the agent's own CLI +/// (`main`) and its ACP bridge (`bridge`) — each installed and versioned +/// independently. This struct carries one binary's readout so the two can be +/// surfaced side by side instead of collapsed into a single source/version. +/// +/// All fields default to `None`; the cheap (no-freshness) path populates only +/// `install_source`, and the freshness pass fills in the version fields. +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct AgentVersionInfo { + /// How this binary was installed (brew, npm, curl/native, …), if detected. + pub install_source: Option, + /// Installed version string, if detected by the freshness pass. + pub installed_version: Option, + /// Latest available version string, if known. + pub latest_version: Option, + /// Whether a newer version is available. Suppressed (`None`) for + /// self-updating installs — see [`DoctorCheck::update_available`]. + pub update_available: Option, + /// Whether this binary keeps itself up to date (curl/native installers). + pub self_updating: Option, +} + /// A single health-check result shown in the UI. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -93,6 +118,19 @@ pub struct DoctorCheck { /// `update_available` — the update is "managed by the tool", not actionable. /// Only populated by the freshness pass; `None` on the default cheap path. pub self_updating: Option, + /// Independent readout for the agent's own CLI (e.g. `claude`, `codex`). + /// + /// For AI-agent checks this carries the main CLI's install source and (when + /// freshness runs) its versions, separate from the ACP bridge. `None` for + /// non-agent checks and for agents with no resolvable main CLI. + /// + /// The flat fields above remain populated for backward compatibility: they + /// mirror the `bridge` readout when a bridge exists, otherwise `main`. + pub main: Option, + /// Independent readout for the agent's ACP bridge (e.g. `claude-agent-acp`, + /// `codex-acp`). `None` for non-agent checks and for agents that have no + /// separate bridge binary (the single binary is reported under `main`). + pub bridge: Option, } /// The full report returned to the frontend. From db59d9b186c2e90e0c4489dd1627a408fc8850e8 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 3 Jun 2026 15:46:02 +1000 Subject: [PATCH 14/18] feat(doctor): read npm-bridge installed versions from package.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix the freshness review finding that `populate_freshness` hardcoded `--version` as the installed-version probe for every target. npm- distributed ACP bridges are `node` scripts that don't honor it (`codex-acp --version` errors with a clap "unexpected argument"; `amp-acp --version` exits 0 but prints nothing), so installed_version stayed None while latest_version was still fetched — leaving the bridge readout with a latest and no installed counterpart, and update_available uncomputable. Make the installed-version mechanism data-driven off each readout's already-known install source: - freshness.rs: add `InstalledProbe` (Cli(args) vs NpmPackageJson) and a pure-filesystem `installed_version_from_package_json` that canonicalizes the binary (resolving npm's bin/ symlink), then walks up a bounded six levels to the owning `node_modules//package.json`. When the package id is known the file's `name` must match, so a nested dependency's package.json is never picked up; scoped packages resolve correctly. No subprocess, no network. `fetch_version_info` now takes an `InstalledProbe` instead of `&[&str]`, branching to the package.json reader for npm targets and the existing `--version` CLI probe otherwise. Add `select_installed_probe(install_source, package_id)` so the choice is unit-testable. - lib.rs: carry each readout's `install_source` into `FreshnessTarget` (from `readout.install_source` for Main/Bridge, `check.install_source` for Flat) and select the probe via `select_installed_probe`. Codex's bridge (npm) now reads package.json while its main CLI (brew) keeps `--version`, in the same pass — the Phase C per-readout split already gives each readout the right source. Purely under `check_freshness`; the default `run_checks()` path does no version probing, so its latency profile is unchanged, and no PACKAGE_IDS redesign is needed. Adds unit tests for the package.json reader (symlink resolution, mismatched-name walk-past, scoped packages, missing file) and the probe selector (npm vs brew/curl/cargo/none). Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/freshness.rs | 226 ++++++++++++++++++++++++++++++++- crates/doctor/src/lib.rs | 13 +- 2 files changed, 231 insertions(+), 8 deletions(-) diff --git a/crates/doctor/src/freshness.rs b/crates/doctor/src/freshness.rs index f960a7f50..0171cf80d 100644 --- a/crates/doctor/src/freshness.rs +++ b/crates/doctor/src/freshness.rs @@ -211,6 +211,89 @@ fn compute_update_available(installed: &Option, latest: &Option) Some(lt > it) } +/// How to read a target's *installed* version. The mechanism depends on how the +/// binary was installed: a CLI `--version` probe works for native/brew/cargo +/// binaries, but npm-distributed ACP bridges are `node` scripts that don't +/// implement `--version` (e.g. `codex-acp --version` errors, `amp-acp +/// --version` prints nothing), so those are read straight from the package's +/// `package.json` instead — no subprocess, no network. +pub(crate) enum InstalledProbe<'a> { + /// Run ` ` and extract a semver (the default behavior). + Cli(&'a [&'a str]), + /// Walk up from the canonicalized binary to the owning + /// `node_modules//package.json` and read its `version`. + NpmPackageJson { package_id: Option<&'a str> }, +} + +/// Pick the installed-version probe for a readout from its install source. +/// `Npm` installs read `package.json`; 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)) { + InstalledProbe::NpmPackageJson { package_id } + } else { + InstalledProbe::Cli(&["--version"]) + } +} + +/// Owned mirror of [`InstalledProbe`] so the probe can cross the +/// `spawn_blocking` boundary (which requires `'static`). +enum OwnedProbe { + Cli(Vec), + NpmPackageJson { package_id: Option }, +} + +impl InstalledProbe<'_> { + fn to_owned_probe(&self) -> OwnedProbe { + match self { + InstalledProbe::Cli(args) => { + OwnedProbe::Cli(args.iter().map(|s| s.to_string()).collect()) + } + InstalledProbe::NpmPackageJson { package_id } => OwnedProbe::NpmPackageJson { + package_id: package_id.map(|s| s.to_string()), + }, + } + } +} + +/// Read an npm package's installed version from its `package.json`. Pure +/// filesystem: canonicalize the binary (resolving the symlink npm leaves in +/// `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. +fn installed_version_from_package_json( + binary_path: &Path, + expected_pkg: Option<&str>, +) -> Option { + let resolved = std::fs::canonicalize(binary_path).ok()?; + let mut dir = resolved.parent(); + for _ in 0..6 { + let d = dir?; + 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); + } + } + } + } + dir = d.parent(); + } + None +} + /// Run ` ` and parse the first semver-shaped token out of the /// combined stdout/stderr. Errors and missing tokens both yield `None`. fn installed_version(binary_path: &Path, version_args: &[&str]) -> Option { @@ -376,13 +459,13 @@ pub(crate) async fn fetch_version_info( latest_source: Option, package_id: Option<&str>, binary_path: &Path, - version_args: &[&str], + probe: InstalledProbe<'_>, offline: bool, npm_registry: Option<&str>, cache: Arc>, ) -> VersionInfo { let path = binary_path.to_path_buf(); - let args: Vec = version_args.iter().map(|s| s.to_string()).collect(); + let probe = probe.to_owned_probe(); let pkg = package_id.map(|s| s.to_string()); let npm_registry = npm_registry.map(|s| s.to_string()); @@ -390,10 +473,15 @@ pub(crate) async fn fetch_version_info( let installed = if path.as_os_str().is_empty() { None } else { - installed_version( - &path, - &args.iter().map(|s| s.as_str()).collect::>(), - ) + match &probe { + OwnedProbe::Cli(args) => installed_version( + &path, + &args.iter().map(|s| s.as_str()).collect::>(), + ), + OwnedProbe::NpmPackageJson { package_id } => { + installed_version_from_package_json(&path, package_id.as_deref()) + } + } }; let latest = if offline { @@ -579,4 +667,130 @@ mod tests { assert!(!is_self_updating(Some(&InstallSource::Brew))); assert!(!is_self_updating(None)); } + + /// Fresh per-test scratch dir under the system temp dir, unique by name + + /// process id (matches the pattern resolve.rs's tests use — no extra dev + /// dependency). + fn scratch_dir(name: &str) -> PathBuf { + let dir = + std::env::temp_dir().join(format!("doctor-freshness-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[test] + fn package_json_version_read_through_bin_symlink() { + let root = scratch_dir("pj-symlink"); + let pkg = root.join("lib/node_modules/amp-acp"); + std::fs::create_dir_all(pkg.join("dist")).unwrap(); + let index = pkg.join("dist/index.js"); + std::fs::write(&index, "// node script\n").unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "amp-acp", "version": "0.4.2"}"#, + ) + .unwrap(); + + // npm leaves a symlink in bin/ pointing at the package's entrypoint. + std::fs::create_dir_all(root.join("bin")).unwrap(); + let bin = root.join("bin/amp-acp"); + #[cfg(unix)] + std::os::unix::fs::symlink(&index, &bin).unwrap(); + + assert_eq!( + installed_version_from_package_json(&bin, Some("amp-acp")).as_deref(), + Some("0.4.2"), + ); + // No expected name → first package.json found still wins. + assert_eq!( + installed_version_from_package_json(&bin, None).as_deref(), + Some("0.4.2"), + ); + } + + #[test] + fn package_json_skips_mismatched_name_and_keeps_walking() { + let root = scratch_dir("pj-mismatch"); + let pkg = root.join("node_modules/amp-acp"); + // A nested dependency's package.json sits closer to the binary; its + // name doesn't match, so the walk must continue up to amp-acp's. + let dep = pkg.join("node_modules/inner-dep"); + std::fs::create_dir_all(&dep).unwrap(); + std::fs::write( + dep.join("package.json"), + br#"{"name": "inner-dep", "version": "9.9.9"}"#, + ) + .unwrap(); + let index = dep.join("index.js"); + std::fs::write(&index, "// dep\n").unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "amp-acp", "version": "1.0.0"}"#, + ) + .unwrap(); + + assert_eq!( + installed_version_from_package_json(&index, Some("amp-acp")).as_deref(), + Some("1.0.0"), + "should skip inner-dep and find amp-acp", + ); + } + + #[test] + fn package_json_resolves_scoped_package() { + let root = scratch_dir("pj-scoped"); + let pkg = root.join("node_modules/@zed-industries/codex-acp"); + std::fs::create_dir_all(pkg.join("dist")).unwrap(); + let index = pkg.join("dist/index.js"); + std::fs::write(&index, "// node script\n").unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "@zed-industries/codex-acp", "version": "0.7.1"}"#, + ) + .unwrap(); + + assert_eq!( + installed_version_from_package_json(&index, Some("@zed-industries/codex-acp")) + .as_deref(), + Some("0.7.1"), + ); + } + + #[test] + fn package_json_missing_returns_none() { + let root = scratch_dir("pj-missing"); + let bin = root.join("bin/whatever"); + std::fs::create_dir_all(root.join("bin")).unwrap(); + std::fs::write(&bin, "x\n").unwrap(); + assert_eq!( + installed_version_from_package_json(&bin, Some("nope")), + None + ); + } + + #[test] + fn select_probe_npm_reads_package_json() { + match select_installed_probe(Some(&InstallSource::Npm), Some("amp-acp")) { + InstalledProbe::NpmPackageJson { package_id } => { + assert_eq!(package_id, Some("amp-acp")); + } + _ => panic!("npm install source should select NpmPackageJson probe"), + } + } + + #[test] + fn select_probe_non_npm_uses_cli_version() { + for src in [ + Some(&InstallSource::Brew), + Some(&InstallSource::CurlPipe), + Some(&InstallSource::Cargo), + None, + ] { + match select_installed_probe(src, Some("amp-acp")) { + InstalledProbe::Cli(args) => assert_eq!(args, &["--version"]), + _ => panic!("non-npm source {src:?} should select Cli probe"), + } + } + } } diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index eed8c44c0..707a995ef 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -19,7 +19,9 @@ use std::sync::{Arc, Mutex}; use agents::{check_single_ai_agent, lookup_fix_command, AI_AGENT_CHECKS}; use checks::{check_clonefile, check_gh, check_gh_auth, check_git, check_git_lfs}; -use freshness::{fetch_version_info, is_self_updating, load_cache, save_cache}; +use freshness::{ + fetch_version_info, is_self_updating, load_cache, save_cache, select_installed_probe, +}; use package_ids::{lookup_package_id, LatestSource}; use resolve::resolve_binary; use types::{AgentVersionInfo, InstallSource, ResolvedBinary}; @@ -272,6 +274,7 @@ async fn populate_freshness( path: PathBuf::from(path), latest_source, package_id, + install_source: readout.install_source.clone(), }); } if let (Some(readout), Some(path)) = (&check.bridge, check.bridge_path.as_deref()) { @@ -283,6 +286,7 @@ async fn populate_freshness( path: PathBuf::from(path), latest_source, package_id, + install_source: readout.install_source.clone(), }); } } else { @@ -298,6 +302,7 @@ async fn populate_freshness( path: PathBuf::from(path_str), latest_source, package_id, + install_source: check.install_source.clone(), }); } } @@ -306,11 +311,14 @@ async fn populate_freshness( let cache = cache.clone(); let npm_registry = npm_registry.clone(); async move { + // npm-distributed bridges don't honor `--version`; read their + // installed version straight from the owning `package.json`. + let probe = select_installed_probe(t.install_source.as_ref(), t.package_id.as_deref()); let info = fetch_version_info( t.latest_source, t.package_id.as_deref(), &t.path, - &["--version"], + probe, offline, npm_registry.as_deref(), cache, @@ -383,6 +391,7 @@ struct FreshnessTarget { path: PathBuf, latest_source: Option, package_id: Option, + install_source: Option, } /// Run a fix command for a doctor check, identified by check ID and fix type. From fb6cb2923574d222ace10a3eddba629d2d94eb8a Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 3 Jun 2026 23:12:27 +1000 Subject: [PATCH 15/18] fix(doctor): correct Claude main package id; distinguish auth-probe spawn failures; use resolved bin dir in auth PATH MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four tightly-coupled bugs that all surface in the Claude Code row when goose-internal (or any embedder) is launched with a restricted PATH — the macOS Finder/launchd case where the child shell only sees /usr/bin:/bin:/usr/sbin:/sbin. Bundled because the intermediate states would each leave the Claude row showing different misleading output. 1. Add main-vs-bridge role to PACKAGE_IDS lookup. The table was keyed only by (check_id, InstallSource); Claude's main CLI under nvm (tagged Npm) collided with the bridge entry and produced the wrong latest_version (and None installed_version, since the package.json name mismatched in freshness.rs). Each entry now carries a `Role` (Main/Bridge/Any) and `lookup_package_id` takes the readout's role as a third argument. `ReadoutSlot::Main`/`Bridge`/`Flat` in populate_freshness map straight to `Role::Main`/`Bridge`/`Any`. Registered `@anthropic-ai/claude-code` as Claude main on npm and kept `@agentclientprotocol/claude-agent-acp` as Claude bridge. Codex/amp have distinct install sources for main vs bridge so their flat lookups were already unambiguous; they're tagged for clarity but otherwise unchanged. 2. Prepend the main CLI's resolve trace to `raw_output` on the happy path. `check_single_ai_agent` previously only included `resolved_main.search_output` on the "bridge missing" and "neither found" branches, so a successful Claude row showed a main `path` with no trace explaining how it was found. Now happy-path raw_output renders both traces with `# main CLI (claude):` and `# ACP bridge (claude-agent-acp):` section headers, main first. 3. Distinguish "command not found / spawn failure" from "not authenticated". Added `AuthStatus::Unknown` and a new `execute_command_with_path_prefix` returning an `ExecOutcome` (`Ok` / `Spawn(io::Error)` / `Exit { code, stderr }`). The auth probe maps `Exit { code: Some(127), .. }` and `Spawn(_)` to `Unknown` (PATH-shadowed agents shouldn't read as signed out); any other non-zero exit stays `NotAuthenticated`. `fix_command` / `fix_type` are no longer surfaced for `Unknown` — `claude auth login` doesn't help if claude isn't on PATH. Raw output labels the not-found case explicitly. Downstream TS mirrors should add a `unknown` arm to their AuthStatus union and UI handling. 4. Build a PATH prefix from the resolved binary directories (`resolved_main.path` parent + each `resolved_cmds[*].path` parent, deduped) and thread it into the auth-probe exec. The shared login-shell setup is now in `build_shell_command`, which adds the prefix dirs ahead of a conservative fallback `/usr/local/bin:/opt/homebrew/bin:/usr/bin:/bin:/usr/sbin:/sbin` when supplied. Existing streaming callers pass an empty prefix and see the same env layout as before. Follow-up: the same PATH prefix isn't yet plumbed to `execute_fix` / `execute_fix_streaming` for `FixType::Auth`, because the fix-execution path doesn't currently re-resolve the binary or carry resolved-bin info. `claude auth login` has the same PATH problem and will benefit from this once the resolved bin dir is plumbed through `execute_fix`. Tests cover: the new role-tagged package_id lookups (claude main and bridge resolving to different packages despite sharing InstallSource), `installed_version_from_package_json` finding `@anthropic-ai/claude-code` for a typical npm layout, exit-127 mapping to `Unknown` semantics via `execute_command_with_path_prefix`, a scripted command findable only via the supplied PATH prefix, and the main/bridge resolve-trace ordering in `raw_output`. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 189 +++++++++++++++++++++++++++--- crates/doctor/src/freshness.rs | 28 +++++ crates/doctor/src/lib.rs | 183 +++++++++++++++++++++++++---- crates/doctor/src/package_ids.rs | 194 ++++++++++++++++++++++++------- crates/doctor/src/types.rs | 6 + 5 files changed, 519 insertions(+), 81 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index fc53a9a59..6977fe754 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -1,13 +1,14 @@ //! AI agent checks and fix command lookup. +use std::path::{Path, PathBuf}; use std::process::Command; use crate::checks::CLONEFILE_FIX_COMMAND; -use crate::execute_command_blocking; use crate::resolve::format_command_output; use crate::types::{ AgentVersionInfo, AuthStatus, CheckStatus, DoctorCheck, FixType, InstallSource, ResolvedBinary, }; +use crate::{execute_command_with_path_prefix, ExecOutcome}; /// Metadata for an individual AI agent check. pub struct AgentCheckInfo { @@ -161,6 +162,59 @@ fn is_npm_command(command: &str) -> bool { command.starts_with("npm ") || command.contains("npm install") || command.contains("npm view") } +/// Parent directories of every resolved binary behind a check, deduplicated. +/// Used to build a PATH prefix for the auth probe so the agent is findable +/// even when the parent process has a restricted PATH (Finder/launchd case). +/// Order: main first, then bridges, in the order they were resolved. +fn collect_resolved_bin_dirs( + main: Option<&ResolvedBinary>, + bridges: &[ResolvedBinary], +) -> Vec { + let mut out: Vec = Vec::new(); + let mut push = |p: &Path| { + if let Some(parent) = p.parent() { + let pb = parent.to_path_buf(); + if !out.contains(&pb) { + out.push(pb); + } + } + }; + if let Some(m) = main { + if let Some(p) = m.path.as_deref() { + push(p); + } + } + for b in bridges { + if let Some(p) = b.path.as_deref() { + push(p); + } + } + out +} + +/// Build the resolve-trace portion of an agent check's `raw_output`. When the +/// agent has a separate `main_command`, prepends the main CLI's resolve trace +/// so the happy path explains *both* binaries' resolution, not just the +/// bridge's. Single-binary agents (no `main_command`) leave only the bridge +/// trace, which is the binary that was searched for. +fn build_raw_with_main_search( + header: &str, + info: &AgentCheckInfo, + resolved_main: Option<&ResolvedBinary>, + bridge_search: &str, +) -> String { + match (info.main_command, resolved_main) { + (Some(main_cmd), Some(rm)) => { + let bridge_cmd = info.commands.first().copied().unwrap_or(""); + format!( + "{header}\n# main CLI ({main_cmd}):\n{}\n# ACP bridge ({bridge_cmd}):\n{bridge_search}", + rm.search_output, + ) + } + _ => format!("{header}\n{bridge_search}"), + } +} + /// Resolve the effective install source for an agent binary. Generic path and /// filesystem fingerprinting (in [`crate::resolve`]) win when they identify a /// source; only when they fall through to [`InstallSource::Unknown`] does the @@ -333,38 +387,77 @@ pub fn check_single_ai_agent( (version_readout(bridge_install_source.clone()), None) }; + // Build a PATH prefix from the resolved binary directories so the + // auth probe can find the agent even when the parent process was + // launched with a restricted PATH (Finder/launchd case). The probe + // command itself is left intact — only PATH is augmented. + let auth_path_prefix = collect_resolved_bin_dirs(resolved_main, resolved_cmds); + let (auth_status, auth_output) = match info.auth_status_command { - Some(cmd) => match execute_command_blocking(cmd) { - Ok(()) => ( + Some(cmd) => match execute_command_with_path_prefix(cmd, &auth_path_prefix) { + ExecOutcome::Ok => ( Some(AuthStatus::Authenticated), Some(format!("$ {cmd}\n(exit 0)")), ), - Err(err) => ( + // Shell itself couldn't be spawned — we have no signal at + // all about auth state. + ExecOutcome::Spawn(e) => ( + Some(AuthStatus::Unknown), + Some(format!("$ {cmd}\n(spawn failure: {e})")), + ), + // Exit 127 means the shell couldn't find the command — + // PATH-shadowed or uninstalled. Not the same as "signed + // out"; report Unknown so the UI doesn't offer a useless + // `... auth login` fix. + ExecOutcome::Exit { + code: Some(127), + stderr, + } => ( + Some(AuthStatus::Unknown), + Some(format!( + "$ {cmd}\n(command not found / exit 127)\n{}", + stderr.trim_end(), + )), + ), + // Genuine non-zero exit — the command ran and rejected us. + ExecOutcome::Exit { code, stderr } => ( Some(AuthStatus::NotAuthenticated), - Some(format!("$ {cmd}\n{err}")), + Some(format!( + "$ {cmd}\n(exit {})\n{}", + code.map(|c| c.to_string()) + .unwrap_or_else(|| "signal".to_string()), + stderr.trim_end(), + )), ), }, None if info.auth_command.is_some() => (Some(AuthStatus::NotApplicable), None), None => (None, None), }; - let mut raw = format!("{header}\n{search}"); + let mut raw = build_raw_with_main_search(&header, info, resolved_main, &search); if let Some(extra) = auth_output { raw.push('\n'); raw.push_str(&extra); } - let (status, message, fix_type, fix_command) = - if matches!(auth_status, Some(AuthStatus::NotAuthenticated)) { - ( - CheckStatus::Warn, - "Installed, not authenticated".to_string(), - info.auth_command.map(|_| FixType::Auth), - info.auth_command.map(|s| s.to_string()), - ) - } else { - (CheckStatus::Pass, "Installed".to_string(), None, None) - }; + let (status, message, fix_type, fix_command) = match auth_status { + Some(AuthStatus::NotAuthenticated) => ( + CheckStatus::Warn, + "Installed, not authenticated".to_string(), + info.auth_command.map(|_| FixType::Auth), + info.auth_command.map(|s| s.to_string()), + ), + // PATH-shadowed / command not found: we can't tell. Surface + // the state but don't offer an auth fix — `claude auth login` + // won't help if `claude` isn't on PATH. + Some(AuthStatus::Unknown) => ( + CheckStatus::Warn, + "Installed, auth status unknown".to_string(), + None, + None, + ), + _ => (CheckStatus::Pass, "Installed".to_string(), None, None), + }; DoctorCheck { id: info.id.to_string(), @@ -686,6 +779,68 @@ mod tests { ); } + /// On the happy path (both main and bridge resolved), `raw_output` includes + /// the main CLI's resolve trace alongside the bridge's, with clear labels + /// and a stable main-first ordering — diagnoses PATH-shadowed agents from + /// the Copy details payload alone. + #[test] + fn raw_output_includes_main_and_bridge_resolve_traces() { + let bridge = ResolvedBinary { + path: Some(PathBuf::from("/n/bin/pi-acp")), + search_output: "BRIDGE-SEARCH-MARKER".to_string(), + install_source: Some(InstallSource::Npm), + }; + let main = ResolvedBinary { + path: Some(PathBuf::from("/c/bin/pi")), + search_output: "MAIN-SEARCH-MARKER".to_string(), + install_source: Some(InstallSource::Cargo), + }; + let check = check_single_ai_agent( + agent("ai-agent-pi"), + true, + std::slice::from_ref(&bridge), + Some(&main), + None, + ); + let raw = check.raw_output.expect("raw_output set"); + let main_at = raw + .find("MAIN-SEARCH-MARKER") + .expect("main trace in raw_output"); + let bridge_at = raw + .find("BRIDGE-SEARCH-MARKER") + .expect("bridge trace in raw_output"); + assert!( + main_at < bridge_at, + "main should appear before bridge; raw_output:\n{raw}", + ); + assert!( + raw.contains("# main CLI") && raw.contains("# ACP bridge"), + "expected labeled sections; raw_output:\n{raw}", + ); + } + + #[test] + fn collect_resolved_bin_dirs_dedupes_and_preserves_order() { + let main = ResolvedBinary { + path: Some(PathBuf::from("/tmp/a/main")), + search_output: String::new(), + install_source: None, + }; + let b1 = ResolvedBinary { + path: Some(PathBuf::from("/tmp/b/bridge1")), + search_output: String::new(), + install_source: None, + }; + // Same dir as main — should dedupe. + let b2 = ResolvedBinary { + path: Some(PathBuf::from("/tmp/a/another")), + search_output: String::new(), + install_source: None, + }; + let dirs = collect_resolved_bin_dirs(Some(&main), &[b1, b2]); + assert_eq!(dirs, vec![PathBuf::from("/tmp/a"), PathBuf::from("/tmp/b")],); + } + #[test] fn apply_npm_registry_none_is_passthrough() { assert_eq!( diff --git a/crates/doctor/src/freshness.rs b/crates/doctor/src/freshness.rs index 0171cf80d..5c548f9d2 100644 --- a/crates/doctor/src/freshness.rs +++ b/crates/doctor/src/freshness.rs @@ -757,6 +757,34 @@ mod tests { ); } + /// Exercises the Claude main CLI's new npm package id end-to-end at the + /// freshness layer: when claude is npm-installed under nvm, its main + /// readout walks up to `@anthropic-ai/claude-code`'s `package.json`. + #[test] + fn package_json_resolves_claude_main_npm_layout() { + let root = scratch_dir("pj-claude-main"); + let pkg = root.join("node_modules/@anthropic-ai/claude-code"); + std::fs::create_dir_all(pkg.join("cli")).unwrap(); + let entry = pkg.join("cli/cli.js"); + std::fs::write(&entry, "// node script\n").unwrap(); + std::fs::write( + pkg.join("package.json"), + br#"{"name": "@anthropic-ai/claude-code", "version": "2.1.0"}"#, + ) + .unwrap(); + + // npm leaves a `claude` symlink in `bin/`. + std::fs::create_dir_all(root.join("bin")).unwrap(); + let bin = root.join("bin/claude"); + #[cfg(unix)] + std::os::unix::fs::symlink(&entry, &bin).unwrap(); + + assert_eq!( + installed_version_from_package_json(&bin, Some("@anthropic-ai/claude-code")).as_deref(), + Some("2.1.0"), + ); + } + #[test] fn package_json_missing_returns_none() { let root = scratch_dir("pj-missing"); diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index 707a995ef..a4aa0a018 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -22,7 +22,7 @@ use checks::{check_clonefile, check_gh, check_gh_auth, check_git, check_git_lfs} use freshness::{ fetch_version_info, is_self_updating, load_cache, save_cache, select_installed_probe, }; -use package_ids::{lookup_package_id, LatestSource}; +use package_ids::{lookup_package_id, LatestSource, Role}; use resolve::resolve_binary; use types::{AgentVersionInfo, InstallSource, ResolvedBinary}; @@ -217,10 +217,11 @@ enum ReadoutSlot { fn resolve_package( check_id: &str, source: Option<&InstallSource>, + role: Role, ) -> (Option, Option) { source .cloned() - .and_then(|src| lookup_package_id(check_id, src)) + .and_then(|src| lookup_package_id(check_id, src, role)) .map(|(pkg, latest)| (Some(pkg.to_string()), Some(latest))) .unwrap_or((None, None)) } @@ -267,7 +268,7 @@ async fn populate_freshness( if is_agent { if let (Some(readout), Some(path)) = (&check.main, check.path.as_deref()) { let (package_id, latest_source) = - resolve_package(&check.id, readout.install_source.as_ref()); + resolve_package(&check.id, readout.install_source.as_ref(), Role::Main); targets.push(FreshnessTarget { id: check.id.clone(), slot: ReadoutSlot::Main, @@ -279,7 +280,7 @@ async fn populate_freshness( } if let (Some(readout), Some(path)) = (&check.bridge, check.bridge_path.as_deref()) { let (package_id, latest_source) = - resolve_package(&check.id, readout.install_source.as_ref()); + resolve_package(&check.id, readout.install_source.as_ref(), Role::Bridge); targets.push(FreshnessTarget { id: check.id.clone(), slot: ReadoutSlot::Bridge, @@ -295,7 +296,7 @@ async fn populate_freshness( let path_str = check.bridge_path.as_deref().or(check.path.as_deref()); let Some(path_str) = path_str else { continue }; let (package_id, latest_source) = - resolve_package(&check.id, check.install_source.as_ref()); + resolve_package(&check.id, check.install_source.as_ref(), Role::Any); targets.push(FreshnessTarget { id: check.id.clone(), slot: ReadoutSlot::Flat, @@ -469,16 +470,14 @@ enum StreamLine { Stderr(String), } -/// Spawn `command` through a login shell, stream stdout/stderr lines to -/// `on_line`, and return based on the process exit status. Stderr lines are -/// also accumulated so a non-zero exit can surface a useful error message -/// (matching the non-streaming behavior of the previous `execute_command`). -fn run_command_streaming_blocking(command: &str, mut on_line: F) -> Result<(), String> -where - F: FnMut(&str), -{ - use std::io::{BufRead, BufReader}; - +/// Build the login-shell `Command` used by every doctor exec path. Optionally +/// prepends `path_prefix` directories to the child `PATH` so a binary that's +/// only findable in the resolved location (e.g. an nvm bin dir) is visible to +/// the spawned shell even when the parent process was launched with a +/// restricted `PATH` (the macOS Finder/launchd case). When `path_prefix` is +/// empty the env layout is byte-identical to the previous implementation, so +/// existing call sites are unaffected. +fn build_shell_command(command: &str, path_prefix: &[PathBuf]) -> std::process::Command { let (shell, args) = if std::path::Path::new("/bin/zsh").exists() { ("/bin/zsh", vec!["-l", "-c", command]) } else { @@ -487,13 +486,93 @@ where let home = std::env::var("HOME").unwrap_or_else(|_| "/".to_string()); let user = std::env::var("USER").unwrap_or_default(); - let mut child = std::process::Command::new(shell) - .args(&args) + let mut cmd = std::process::Command::new(shell); + cmd.args(&args) .env_clear() .env("HOME", &home) .env("USER", &user) .env("TERM", "xterm-256color") - .current_dir(&home) + .current_dir(&home); + if !path_prefix.is_empty() { + let mut seen: std::collections::HashSet = std::collections::HashSet::new(); + let mut path = String::new(); + for p in path_prefix { + let s = p.to_string_lossy().to_string(); + if !seen.insert(s.clone()) { + continue; + } + if !path.is_empty() { + path.push(':'); + } + path.push_str(&s); + } + // Conservative fallback ~ what login zsh on macOS sees before + // /etc/zprofile augments it. Keeps the rest of the resolved-binary + // dir's command graph reachable (e.g. node, npm) without depending on + // the parent process's PATH. + path.push_str(":/usr/local/bin:/opt/homebrew/bin:/usr/bin:/bin:/usr/sbin:/sbin"); + cmd.env("PATH", path); + } + cmd +} + +/// Detailed outcome of running a command. Lets the caller distinguish +/// `command not found` (exit 127 from the shell, or a spawn failure on the +/// shell itself) from a genuine non-zero exit — important for the auth probe, +/// where the former means "we can't tell" and the latter means "not signed in". +#[derive(Debug)] +pub(crate) enum ExecOutcome { + Ok, + /// The shell itself couldn't be spawned (or `wait()` failed). Rare; means + /// we have no signal at all from the inner command. + Spawn(std::io::Error), + /// The shell ran, the inner command exited non-zero. Code is `Some(127)` + /// when the shell reports "command not found". + Exit { + code: Option, + stderr: String, + }, +} + +/// Run `command` through a login shell, with the caller-supplied `path_prefix` +/// prepended to the child `PATH`. Returns the detailed exec outcome. No +/// streaming — used by the auth probe which wants a single sync result. +pub(crate) fn execute_command_with_path_prefix( + command: &str, + path_prefix: &[PathBuf], +) -> ExecOutcome { + let mut cmd = build_shell_command(command, path_prefix); + cmd.stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); + let child = match cmd.spawn() { + Ok(c) => c, + Err(e) => return ExecOutcome::Spawn(e), + }; + let output = match child.wait_with_output() { + Ok(o) => o, + Err(e) => return ExecOutcome::Spawn(e), + }; + if output.status.success() { + ExecOutcome::Ok + } else { + ExecOutcome::Exit { + code: output.status.code(), + stderr: String::from_utf8_lossy(&output.stderr).to_string(), + } + } +} + +/// Spawn `command` through a login shell, stream stdout/stderr lines to +/// `on_line`, and return based on the process exit status. Stderr lines are +/// also accumulated so a non-zero exit can surface a useful error message +/// (matching the non-streaming behavior of the previous `execute_command`). +fn run_command_streaming_blocking(command: &str, mut on_line: F) -> Result<(), String> +where + F: FnMut(&str), +{ + use std::io::{BufRead, BufReader}; + + let mut child = build_shell_command(command, &[]) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) .spawn() @@ -557,13 +636,6 @@ where } } -/// Synchronous, non-streaming variant for callers running inside an existing -/// `spawn_blocking` closure (the auth probes in `check_single_ai_agent` use -/// this — they need a sync result, not an `on_line` stream). -pub(crate) fn execute_command_blocking(command: &str) -> Result<(), String> { - run_command_streaming_blocking(command, |_| {}) -} - #[cfg(test)] mod tests { use super::*; @@ -613,6 +685,67 @@ mod tests { assert_eq!(direct, streamed); } + /// A command not found by the shell must surface as `Exit { code: 127, .. }` + /// — the auth probe maps that to `AuthStatus::Unknown` instead of + /// `NotAuthenticated`. Using an unambiguously-nonexistent command name so + /// the shell hits its "command not found" path regardless of rc files. + #[tokio::test] + async fn exec_command_not_found_reports_exit_127() { + let outcome = tokio::task::spawn_blocking(|| { + execute_command_with_path_prefix("doctor-nonexistent-xyz-12345", &[]) + }) + .await + .unwrap(); + match outcome { + ExecOutcome::Exit { code, .. } => assert_eq!( + code, + Some(127), + "expected exit 127 for command-not-found; got {code:?}", + ), + other => panic!("expected Exit(127); got {other:?}"), + } + } + + /// PATH prefix lets the spawned shell find a command that's only in the + /// supplied dir — without it, the same command would exit 127. This is the + /// PATH-shadowing fix in miniature. + #[tokio::test] + async fn exec_command_with_path_prefix_finds_command_in_prefix_dir() { + use std::os::unix::fs::PermissionsExt; + + let tmp = std::env::temp_dir().join(format!( + "doctor-pathprefix-{}-{}", + std::process::id(), + // Coarse uniqueness — enough for this test. + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(), + )); + std::fs::create_dir_all(&tmp).unwrap(); + // Distinct, unguessable name so no real PATH entry could shadow it. + let script_name = "doctor-pathprefix-probe-abcdef"; + let script = tmp.join(script_name); + std::fs::write(&script, "#!/bin/sh\nexit 0\n").unwrap(); + let mut perms = std::fs::metadata(&script).unwrap().permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(&script, perms).unwrap(); + + let prefix = vec![tmp.clone()]; + let outcome = tokio::task::spawn_blocking(move || { + execute_command_with_path_prefix(script_name, &prefix) + }) + .await + .unwrap(); + let _ = std::fs::remove_dir_all(&tmp); + match outcome { + ExecOutcome::Ok => {} + other => { + panic!("expected Ok with the script reachable via path prefix; got {other:?}",) + } + } + } + /// Default `run_checks()` must not populate any of the version-freshness /// fields — guards against accidentally flipping the default to on /// (which would slow down every staged Tauri app launch). diff --git a/crates/doctor/src/package_ids.rs b/crates/doctor/src/package_ids.rs index 8dd85ea4a..34ded0563 100644 --- a/crates/doctor/src/package_ids.rs +++ b/crates/doctor/src/package_ids.rs @@ -1,10 +1,12 @@ //! Per-check package identifiers for version-freshness lookups. //! //! Maps a doctor check ID to one or more `(InstallSource, package_id, -//! LatestSource)` entries. The freshness module picks the entry whose -//! `InstallSource` matches the binary's detected install source, then fetches -//! the latest version via the entry's `LatestSource`. Checks not listed here -//! (or with no matching source) skip the latest-version probe. +//! LatestSource, Role)` entries. The freshness module picks the entry whose +//! `InstallSource` matches the binary's detected install source AND whose +//! `Role` matches the readout being built (the agent's main CLI vs. its ACP +//! bridge), then fetches the latest version via the entry's `LatestSource`. +//! Checks not listed here (or with no matching source) skip the latest-version +//! probe. use crate::types::InstallSource; @@ -26,32 +28,84 @@ pub(crate) enum LatestSource { GitHubReleases, } +/// Which binary an entry describes. An AI-agent check fronts up to two distinct +/// binaries — the agent's own CLI (`Main`) and its ACP bridge (`Bridge`). When +/// both are installed from the same registry (e.g. both via npm, as with +/// Claude) the install source alone is ambiguous and the role is what +/// disambiguates them. Non-agent checks (and agents whose two binaries already +/// have distinct install sources) use `Any`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Role { + Main, + Bridge, + /// Entry applies to either readout (and to flat, non-agent checks). + Any, +} + +impl Role { + /// `query` matches `entry` when: + /// - the entry's role is `Any` (it applies anywhere), or + /// - the query is `Any` (the caller doesn't care which role), or + /// - both roles are equal. + fn matches(entry: Role, query: Role) -> bool { + matches!(entry, Role::Any) || matches!(query, Role::Any) || entry == query + } +} + /// One freshness entry: the install source it applies to, the package id to -/// query, and how to fetch that package's latest version. -type PackageEntry = (InstallSource, &'static str, LatestSource); +/// query, how to fetch that package's latest version, and which readout (main +/// CLI vs ACP bridge) it describes. +type PackageEntry = (InstallSource, &'static str, LatestSource, Role); /// Static table of `check_id -> &[PackageEntry]`. /// /// A single check can have multiple entries when the same agent ships through -/// different registries (e.g. brew for the main binary, npm for the ACP bridge). -/// `lookup_package_id` picks the first entry whose `install_source` matches the -/// binary's detected source. +/// different registries (e.g. brew for the main binary, npm for the ACP bridge) +/// or when both binaries share an install source but have distinct package ids +/// (Claude: `@anthropic-ai/claude-code` for the main CLI, `@agentclientprotocol +/// /claude-agent-acp` for the bridge — both npm). `lookup_package_id` picks the +/// first entry whose `(install_source, role)` matches the query. pub(crate) const PACKAGE_IDS: &[(&str, &[PackageEntry])] = &[ - ("git", &[(InstallSource::Brew, "git", LatestSource::Brew)]), - ("gh", &[(InstallSource::Brew, "gh", LatestSource::Brew)]), + ( + "git", + &[(InstallSource::Brew, "git", LatestSource::Brew, Role::Any)], + ), + ( + "gh", + &[(InstallSource::Brew, "gh", LatestSource::Brew, Role::Any)], + ), ( "git-lfs", - &[(InstallSource::Brew, "git-lfs", LatestSource::Brew)], + &[( + InstallSource::Brew, + "git-lfs", + LatestSource::Brew, + Role::Any, + )], ), // ai-agent-goose: brew tap exists but no canonical formula yet — skip. // TODO: revisit when block/goose lands a stable brew formula. ( "ai-agent-claude", - &[( - InstallSource::Npm, - "@agentclientprotocol/claude-agent-acp", - LatestSource::Npm, - )], + &[ + // Main CLI when installed via npm (e.g. under nvm). The native + // curl-pipe install is fingerprinted as `CurlPipe` (no registry + // entry here, self-updating) so this only applies when Claude + // landed via `npm i -g @anthropic-ai/claude-code`. + ( + InstallSource::Npm, + "@anthropic-ai/claude-code", + LatestSource::Npm, + Role::Main, + ), + // ACP bridge — separate npm package. + ( + InstallSource::Npm, + "@agentclientprotocol/claude-agent-acp", + LatestSource::Npm, + Role::Bridge, + ), + ], // TODO: the main `claude` native (CurlPipe) install has no registry // entry — its latest is published via the native installer's channel // manifest, which we don't parse yet. Claude native is self-updating @@ -60,28 +114,49 @@ pub(crate) const PACKAGE_IDS: &[(&str, &[PackageEntry])] = &[ ( "ai-agent-codex", &[ + // Bridge ships via npm; main CLI via brew — distinct install + // sources so role disambiguation isn't strictly needed, but tag it + // anyway for clarity. ( InstallSource::Npm, "@zed-industries/codex-acp", LatestSource::Npm, + Role::Bridge, ), - (InstallSource::Brew, "codex", LatestSource::Brew), + (InstallSource::Brew, "codex", LatestSource::Brew, Role::Main), ], ), ( "ai-agent-pi", - &[(InstallSource::Npm, "pi-acp", LatestSource::Npm)], + &[( + InstallSource::Npm, + "pi-acp", + LatestSource::Npm, + Role::Bridge, + )], ), ( "ai-agent-amp", &[ - (InstallSource::Npm, "amp-acp", LatestSource::Npm), - (InstallSource::Brew, "amp", LatestSource::Brew), + // Bridge: npm. Main: brew. Main curl-pipe install is `CurlPipe`, + // not present in the table (self-updating, report-only). + ( + InstallSource::Npm, + "amp-acp", + LatestSource::Npm, + Role::Bridge, + ), + (InstallSource::Brew, "amp", LatestSource::Brew, Role::Main), ], ), ( "ai-agent-copilot", - &[(InstallSource::Npm, "@github/copilot", LatestSource::Npm)], + &[( + InstallSource::Npm, + "@github/copilot", + LatestSource::Npm, + Role::Any, + )], ), // ai-agent-cursor: curl-pipe installer with no registry presence; its // releases are published on GitHub. The repo slug is a best-effort default @@ -93,21 +168,23 @@ pub(crate) const PACKAGE_IDS: &[(&str, &[PackageEntry])] = &[ InstallSource::CurlPipe, "getcursor/cursor", LatestSource::GitHubReleases, + Role::Any, )], ), ]; /// Pick the package id and its latest-version source for the entry whose -/// install source matches `source`. Returns `None` if the check isn't in the -/// table, or if no entry matches. +/// `(install_source, role)` matches. Returns `None` if the check isn't in the +/// table, or if no entry matches both the source and the role. pub(crate) fn lookup_package_id( check_id: &str, source: InstallSource, + role: Role, ) -> Option<(&'static str, LatestSource)> { for (id, entries) in PACKAGE_IDS { if *id == check_id { - for (entry_source, pkg, latest) in entries.iter() { - if entry_source == &source { + for (entry_source, pkg, latest, entry_role) in entries.iter() { + if entry_source == &source && Role::matches(*entry_role, role) { return Some((*pkg, *latest)); } } @@ -122,44 +199,83 @@ mod tests { use super::*; #[test] - fn lookup_matches_source() { + fn lookup_matches_source_with_any_role() { assert_eq!( - lookup_package_id("git", InstallSource::Brew), + lookup_package_id("git", InstallSource::Brew, Role::Any), Some(("git", LatestSource::Brew)), ); - assert_eq!( - lookup_package_id("ai-agent-claude", InstallSource::Npm), - Some(("@agentclientprotocol/claude-agent-acp", LatestSource::Npm)), - ); } #[test] fn lookup_returns_none_for_mismatched_source() { // git is registered under Brew only — Npm should miss. - assert_eq!(lookup_package_id("git", InstallSource::Npm), None); + assert_eq!( + lookup_package_id("git", InstallSource::Npm, Role::Any), + None + ); } #[test] fn lookup_returns_none_for_unknown_check() { - assert_eq!(lookup_package_id("nonexistent", InstallSource::Brew), None); + assert_eq!( + lookup_package_id("nonexistent", InstallSource::Brew, Role::Any), + None + ); } #[test] - fn codex_has_both_npm_and_brew_entries() { + fn codex_has_role_tagged_entries() { assert_eq!( - lookup_package_id("ai-agent-codex", InstallSource::Npm), + lookup_package_id("ai-agent-codex", InstallSource::Npm, Role::Bridge), Some(("@zed-industries/codex-acp", LatestSource::Npm)), ); assert_eq!( - lookup_package_id("ai-agent-codex", InstallSource::Brew), + lookup_package_id("ai-agent-codex", InstallSource::Brew, Role::Main), Some(("codex", LatestSource::Brew)), ); } #[test] fn cursor_curl_pipe_resolves_to_github_releases() { - let (_, latest) = - lookup_package_id("ai-agent-cursor", InstallSource::CurlPipe).expect("cursor entry"); + let (_, latest) = lookup_package_id("ai-agent-cursor", InstallSource::CurlPipe, Role::Any) + .expect("cursor entry"); assert_eq!(latest, LatestSource::GitHubReleases); } + + /// The whole point of the role split: claude's main CLI under npm must + /// resolve to `@anthropic-ai/claude-code`, not the bridge package. + #[test] + fn claude_main_npm_resolves_to_main_package() { + assert_eq!( + lookup_package_id("ai-agent-claude", InstallSource::Npm, Role::Main), + Some(("@anthropic-ai/claude-code", LatestSource::Npm)), + ); + } + + /// And the bridge readout still resolves to the bridge package even though + /// they share an install source. + #[test] + fn claude_bridge_npm_resolves_to_bridge_package() { + assert_eq!( + lookup_package_id("ai-agent-claude", InstallSource::Npm, Role::Bridge), + Some(("@agentclientprotocol/claude-agent-acp", LatestSource::Npm)), + ); + } + + /// A `Role::Any` query on a role-tagged check returns the first matching + /// entry — used by non-agent (flat) lookups and as a permissive fallback. + #[test] + fn claude_npm_with_any_role_returns_first_match() { + // The Main entry appears first in the table; Any should hit it. + let (pkg, _) = lookup_package_id("ai-agent-claude", InstallSource::Npm, Role::Any).unwrap(); + assert_eq!(pkg, "@anthropic-ai/claude-code"); + } + + /// `Role::Any` entries match any query role — confirms copilot's single + /// untagged entry is reachable from both Main and Bridge queries. + #[test] + fn copilot_any_entry_matches_main_and_bridge_queries() { + assert!(lookup_package_id("ai-agent-copilot", InstallSource::Npm, Role::Main).is_some()); + assert!(lookup_package_id("ai-agent-copilot", InstallSource::Npm, Role::Bridge).is_some()); + } } diff --git a/crates/doctor/src/types.rs b/crates/doctor/src/types.rs index ae1bd57fc..6d9d2f4b8 100644 --- a/crates/doctor/src/types.rs +++ b/crates/doctor/src/types.rs @@ -31,6 +31,12 @@ pub enum AuthStatus { Authenticated, NotAuthenticated, NotApplicable, + /// Probe couldn't determine auth state because the agent's + /// `auth_status_command` failed to spawn or exited with 127 (command not + /// found). Distinct from `NotAuthenticated`: a PATH-shadowed agent isn't + /// signed out, we just can't tell. Downstream consumers should treat this + /// as informational (no auth fix to offer). + Unknown, } /// How a binary was installed on disk. From 46c8ea771bd010fc9042fe9ed3bb999df4f6a6a5 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 4 Jun 2026 13:37:20 +1000 Subject: [PATCH 16/18] feat(doctor): derive source-aware update commands per readout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking *Update* on an AI agent today invokes the doctor crate with `FixType::Command`, which looks up the static `install_command` from `AI_AGENT_CHECKS`. For Claude that's the native curl-pipe installer — so if the user has Claude installed via npm under nvm, clicking Update silently side-loads `~/.local/bin/claude` with a fresh native install while the npm install (the one we meant to update) stays stale. Bridge has the same shape. Derive the update command from each readout's already-known `(install_source, package_id)` instead, and route execution through an explicit `command_override` so the per-readout update string isn't forced through the static install-time recipe table. - `FixType` gains `UpdateMain` / `UpdateBridge` variants for the per-readout update slots. Serde `rename_all` switches to camelCase so the new variants serialize as `updateMain` / `updateBridge`; the existing single-word variants are identical under both rules. - `AgentVersionInfo` gains `update_command` + `update_fix_type` (Option, default None). Both populated together by the freshness pass when an update is computable and actionable. - `derive_update_command(install_source, package_id)` in `agents.rs` maps Npm/Brew/Cargo to their canonical update recipes (`npm install -g @latest`, `brew upgrade `, `cargo install --force `); CurlPipe/Mise/Asdf/Unknown/System and missing package ids return None. `apply_npm_registry` runs over the final command downstream, so the npm form picks up the Block Artifactory registry append automatically. - `apply_freshness` (in `lib.rs`) now takes the readout's slot and package id alongside the freshness result. When the readout is Main/Bridge, the update is actionable (`update_available == Some(true)` and not self-updating), and `derive_update_command` returns Some, the readout's `update_command` + `update_fix_type` are filled in. Flat (non-agent) readouts never get an update command — out of scope. - `execute_fix_with_options` / `execute_fix_streaming_with_options` gain a `command_override: Option` parameter. When Some, the static `lookup_fix_command` is bypassed and the override is run verbatim (still threaded through `apply_npm_registry`). The Update variants have no static recipe — they can only dispatch through the override path — so `lookup_fix_command` returns None for them. The convenience wrappers (`execute_fix`, `execute_fix_streaming`) pass None and behave as before. Tests cover: `derive_update_command` per InstallSource variant, `lookup_fix_command` returning None for the Update variants, `apply_freshness` emitting npm/brew update commands on actionable Main/Bridge readouts, CurlPipe never producing an update command (belt-and-braces with the existing self-updating suppression), no update available leaving fields None, and the executor running an override for `UpdateMain` (where the static lookup has nothing) vs. erroring out without an override. The downstream goose-internal pin bump, TypeScript mirror update, and frontend rewire are a separate session. Signed-off-by: Matt Toohey --- crates/doctor/src/agents.rs | 90 ++++++++++++++++ crates/doctor/src/lib.rs | 209 +++++++++++++++++++++++++++++++----- crates/doctor/src/types.rs | 20 +++- 3 files changed, 294 insertions(+), 25 deletions(-) diff --git a/crates/doctor/src/agents.rs b/crates/doctor/src/agents.rs index 6977fe754..d7d85ce95 100644 --- a/crates/doctor/src/agents.rs +++ b/crates/doctor/src/agents.rs @@ -141,6 +141,32 @@ pub const AI_AGENT_CHECKS: &[AgentCheckInfo] = &[ }, ]; +/// Derive a source-aware update command from a readout's install source and +/// package id. Returns `None` for self-updating sources (`CurlPipe`), sources +/// with no canonical update recipe (`Mise`/`Asdf`/`Unknown`/`System`), or when +/// the package id is unknown. +/// +/// The caller is responsible for gating on `update_available == Some(true)` — +/// this function only knows how to update, not whether to. `apply_npm_registry` +/// runs over the final command string downstream, so npm commands automatically +/// pick up a registry override when one is configured. +pub fn derive_update_command( + install_source: Option<&InstallSource>, + package_id: Option<&str>, +) -> Option { + let pkg = package_id?; + match install_source? { + InstallSource::Npm => Some(format!("npm install -g {pkg}@latest")), + InstallSource::Brew => Some(format!("brew upgrade {pkg}")), + InstallSource::Cargo => Some(format!("cargo install --force {pkg}")), + InstallSource::CurlPipe + | InstallSource::Mise + | InstallSource::Asdf + | InstallSource::Unknown + | InstallSource::System => None, + } +} + /// 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 @@ -576,6 +602,10 @@ pub fn lookup_fix_command(check_id: &str, fix_type: &FixType) -> Option FixType::Command => info.install_command.map(|s| s.to_string()), FixType::Bridge => info.bridge_install_command.map(|s| s.to_string()), FixType::Auth => info.auth_command.map(|s| s.to_string()), + // Update commands are derived per-readout from + // `(install_source, package_id)` and supplied to the executor + // via `command_override`; there is no static fallback. + FixType::UpdateMain | FixType::UpdateBridge => None, }; } } @@ -848,4 +878,64 @@ mod tests { "npm install -g amp-acp", ); } + + #[test] + fn derive_update_command_npm_emits_at_latest() { + assert_eq!( + derive_update_command(Some(&InstallSource::Npm), Some("@anthropic-ai/claude-code"),) + .as_deref(), + Some("npm install -g @anthropic-ai/claude-code@latest"), + ); + } + + #[test] + fn derive_update_command_brew_emits_upgrade() { + assert_eq!( + derive_update_command(Some(&InstallSource::Brew), Some("codex")).as_deref(), + Some("brew upgrade codex"), + ); + } + + #[test] + fn derive_update_command_cargo_emits_install_force() { + assert_eq!( + derive_update_command(Some(&InstallSource::Cargo), Some("some-crate")).as_deref(), + Some("cargo install --force some-crate"), + ); + } + + #[test] + fn derive_update_command_self_updating_and_opaque_sources_return_none() { + for src in [ + InstallSource::CurlPipe, + InstallSource::Mise, + InstallSource::Asdf, + InstallSource::Unknown, + InstallSource::System, + ] { + assert_eq!( + derive_update_command(Some(&src), Some("pkg")), + None, + "expected None for {src:?}", + ); + } + } + + #[test] + fn derive_update_command_returns_none_without_package_id() { + assert_eq!(derive_update_command(Some(&InstallSource::Npm), None), None,); + } + + #[test] + fn update_fix_lookup_returns_none_for_update_variants() { + // Update commands are derived per-readout, not statically registered. + assert_eq!( + lookup_fix_command("ai-agent-claude", &FixType::UpdateMain), + None, + ); + assert_eq!( + lookup_fix_command("ai-agent-claude", &FixType::UpdateBridge), + None, + ); + } } diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index a4aa0a018..99cfeb964 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -11,20 +11,20 @@ pub(crate) mod package_ids; pub mod resolve; pub mod types; -pub use types::{CheckStatus, DoctorCheck, DoctorReport, FixType}; +pub use types::{AgentVersionInfo, CheckStatus, DoctorCheck, DoctorReport, FixType}; use std::collections::HashMap; use std::path::PathBuf; use std::sync::{Arc, Mutex}; -use agents::{check_single_ai_agent, lookup_fix_command, AI_AGENT_CHECKS}; +use agents::{check_single_ai_agent, derive_update_command, lookup_fix_command, AI_AGENT_CHECKS}; use checks::{check_clonefile, check_gh, check_gh_auth, check_git, check_git_lfs}; use freshness::{ fetch_version_info, is_self_updating, load_cache, save_cache, select_installed_probe, }; use package_ids::{lookup_package_id, LatestSource, Role}; use resolve::resolve_binary; -use types::{AgentVersionInfo, InstallSource, ResolvedBinary}; +use types::{InstallSource, ResolvedBinary}; /// Fallback check returned when a spawn_blocking task panics. fn empty_check(id: &str, label: &str) -> DoctorCheck { @@ -227,8 +227,17 @@ fn resolve_package( } /// Fold a freshness [`freshness::VersionInfo`] into a per-binary readout, -/// applying the self-updating suppression rule for `update_available`. -fn apply_freshness(readout: &mut AgentVersionInfo, info: &freshness::VersionInfo) { +/// applying the self-updating suppression rule for `update_available`. When an +/// update is actionable (and the slot is `Main`/`Bridge`), derive the +/// 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. +fn apply_freshness( + readout: &mut AgentVersionInfo, + info: &freshness::VersionInfo, + slot: ReadoutSlot, + package_id: Option<&str>, +) { readout.installed_version = info.installed.clone(); readout.latest_version = info.latest.clone(); // Self-updating tools (curl/native installers) manage their own freshness: @@ -241,6 +250,19 @@ fn apply_freshness(readout: &mut AgentVersionInfo, info: &freshness::VersionInfo } else { info.update_available }; + + let actionable = readout.update_available == Some(true) && !self_updating; + let slot_fix_type = match slot { + ReadoutSlot::Main => Some(FixType::UpdateMain), + ReadoutSlot::Bridge => Some(FixType::UpdateBridge), + 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) { + readout.update_command = Some(cmd); + readout.update_fix_type = Some(fix_type); + } + } } /// Post-hoc pass: for every check that has a usable binary path and a known @@ -325,30 +347,31 @@ async fn populate_freshness( cache, ) .await; - ((t.id, t.slot), info) + ((t.id, t.slot), (info, t.package_id)) } }); let results = futures::future::join_all(futures).await; - let mut by_target: HashMap<(String, ReadoutSlot), freshness::VersionInfo> = HashMap::new(); - for (key, info) in results { - by_target.insert(key, info); + let mut by_target: HashMap<(String, ReadoutSlot), (freshness::VersionInfo, Option)> = + HashMap::new(); + for (key, payload) in results { + by_target.insert(key, payload); } for check in &mut report.checks { let is_agent = check.main.is_some() || check.bridge.is_some(); if is_agent { - if let (Some(readout), Some(info)) = ( + if let (Some(readout), Some((info, pkg))) = ( check.main.as_mut(), by_target.remove(&(check.id.clone(), ReadoutSlot::Main)), ) { - apply_freshness(readout, &info); + apply_freshness(readout, &info, ReadoutSlot::Main, pkg.as_deref()); } - if let (Some(readout), Some(info)) = ( + if let (Some(readout), Some((info, pkg))) = ( check.bridge.as_mut(), by_target.remove(&(check.id.clone(), ReadoutSlot::Bridge)), ) { - apply_freshness(readout, &info); + apply_freshness(readout, &info, ReadoutSlot::Bridge, pkg.as_deref()); } // Mirror the headline readout (bridge if present, else main) into the // flat fields for backward-compatible consumers. @@ -366,7 +389,8 @@ async fn populate_freshness( check.update_available = update_available; check.self_updating = self_updating; } - } else if let Some(info) = by_target.remove(&(check.id.clone(), ReadoutSlot::Flat)) { + } else if let Some((info, _pkg)) = by_target.remove(&(check.id.clone(), ReadoutSlot::Flat)) + { check.installed_version = info.installed; check.latest_version = info.latest; let self_updating = is_self_updating(check.install_source.as_ref()); @@ -404,15 +428,24 @@ pub async fn execute_fix(check_id: String, fix_type: FixType) -> Result<(), Stri } /// Like [`execute_fix`], but routes npm-backed fix commands through an optional -/// caller-supplied registry (see [`RunChecksOptions::npm_registry`]). Callers -/// that displayed a registry-overridden `fix_command` should pass the same -/// `npm_registry` here so the executed command matches what was shown. +/// caller-supplied registry (see [`RunChecksOptions::npm_registry`]) and +/// optionally accepts a `command_override` — the exact shell command to run, +/// bypassing the static [`lookup_fix_command`] lookup. The override is the only +/// way to dispatch the per-readout `FixType::UpdateMain` / `UpdateBridge` +/// commands, which aren't in the static table. +/// +/// When `command_override` is `Some`, `fix_type` is informational only; the +/// override is always used. When `None`, the command is looked up exactly as +/// before. `apply_npm_registry` runs over the final command string regardless +/// of where it came from. pub async fn execute_fix_with_options( check_id: String, fix_type: FixType, + command_override: Option, npm_registry: Option<&str>, ) -> Result<(), String> { - execute_fix_streaming_with_options(check_id, fix_type, npm_registry, |_| {}).await + execute_fix_streaming_with_options(check_id, fix_type, command_override, npm_registry, |_| {}) + .await } /// Run a fix command and stream its output line-by-line to `on_line`. @@ -433,23 +466,28 @@ pub async fn execute_fix_streaming( where F: FnMut(&str) + Send + 'static, { - execute_fix_streaming_with_options(check_id, fix_type, None, on_line).await + execute_fix_streaming_with_options(check_id, fix_type, None, None, on_line).await } -/// Like [`execute_fix_streaming`], but routes npm-backed fix commands through an -/// optional caller-supplied registry before shelling out (see -/// [`RunChecksOptions::npm_registry`]). Non-npm commands are run verbatim. +/// Like [`execute_fix_streaming`], but accepts a `command_override` to run an +/// exact shell command (skipping [`lookup_fix_command`]) and routes npm-backed +/// commands through an optional caller-supplied registry. See +/// [`execute_fix_with_options`] for the semantics of `command_override`. pub async fn execute_fix_streaming_with_options( check_id: String, fix_type: FixType, + command_override: Option, npm_registry: Option<&str>, on_line: F, ) -> Result<(), String> where F: FnMut(&str) + Send + 'static, { - let command = lookup_fix_command(&check_id, &fix_type) - .ok_or_else(|| format!("Unknown check '{check_id}' or fix type '{fix_type:?}'"))?; + let command = match command_override { + Some(cmd) => cmd, + None => lookup_fix_command(&check_id, &fix_type) + .ok_or_else(|| format!("Unknown check '{check_id}' or fix type '{fix_type:?}'"))?, + }; let command = agents::apply_npm_registry(&command, npm_registry); run_command_streaming(command, on_line).await @@ -746,6 +784,129 @@ mod tests { } } + /// `apply_freshness` derives a source-aware update command for a Main slot + /// when the npm-installed agent has an actionable update. + #[tokio::test] + async fn apply_freshness_npm_main_emits_update_main_command() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::Npm), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("0.1.0".into()), + latest: Some("0.2.0".into()), + update_available: Some(true), + }; + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("@anthropic-ai/claude-code"), + ); + assert_eq!( + readout.update_command.as_deref(), + Some("npm install -g @anthropic-ai/claude-code@latest"), + ); + assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain)); + } + + /// Bridge slot with a brew install upgrades via `brew upgrade ` and + /// is tagged `UpdateBridge`. + #[tokio::test] + async fn apply_freshness_brew_bridge_emits_update_bridge_command() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::Brew), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("0.1.0".into()), + latest: Some("0.2.0".into()), + update_available: Some(true), + }; + apply_freshness(&mut readout, &info, ReadoutSlot::Bridge, Some("amp")); + 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. + #[tokio::test] + async fn apply_freshness_curl_pipe_never_emits_update_command() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::CurlPipe), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("1.0.0".into()), + latest: Some("2.0.0".into()), + update_available: Some(true), + }; + apply_freshness( + &mut readout, + &info, + ReadoutSlot::Main, + Some("getcursor/cursor"), + ); + assert!(readout.update_command.is_none()); + assert!(readout.update_fix_type.is_none()); + assert_eq!(readout.self_updating, Some(true)); + assert!( + readout.update_available.is_none(), + "self-updating readout suppresses update_available too", + ); + } + + /// No update available -> no update command, even on a registry source. + #[tokio::test] + async fn apply_freshness_no_update_available_emits_no_command() { + let mut readout = AgentVersionInfo { + install_source: Some(InstallSource::Npm), + ..AgentVersionInfo::default() + }; + let info = freshness::VersionInfo { + installed: Some("0.2.0".into()), + latest: Some("0.2.0".into()), + update_available: Some(false), + }; + apply_freshness(&mut readout, &info, ReadoutSlot::Main, Some("amp-acp")); + assert!(readout.update_command.is_none()); + assert!(readout.update_fix_type.is_none()); + } + + /// `command_override` makes the executor run the exact string supplied, + /// bypassing `lookup_fix_command`. We test by overriding for the + /// `UpdateMain` variant — which has no static lookup — so a successful + /// run can only be the override path. + #[tokio::test] + async fn execute_fix_with_options_runs_command_override() { + let result = execute_fix_with_options( + "ai-agent-claude".to_string(), + FixType::UpdateMain, + Some("true".to_string()), + None, + ) + .await; + assert!( + result.is_ok(), + "override should execute `true` and exit 0; got {result:?}", + ); + } + + /// Without a `command_override`, `UpdateMain` has no static recipe and + /// must surface as the standard "Unknown check / fix type" error. + #[tokio::test] + async fn execute_fix_with_options_update_without_override_errors() { + let result = execute_fix_with_options( + "ai-agent-claude".to_string(), + FixType::UpdateMain, + None, + None, + ) + .await; + assert!(result.is_err()); + } + /// Default `run_checks()` must not populate any of the version-freshness /// fields — guards against accidentally flipping the default to on /// (which would slow down every staged Tauri app launch). diff --git a/crates/doctor/src/types.rs b/crates/doctor/src/types.rs index 6d9d2f4b8..e5f947ab9 100644 --- a/crates/doctor/src/types.rs +++ b/crates/doctor/src/types.rs @@ -14,7 +14,7 @@ pub enum CheckStatus { /// The type of fix available for a check. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] -#[serde(rename_all = "lowercase")] +#[serde(rename_all = "camelCase")] pub enum FixType { /// A shell command to install or configure the dependency. Command, @@ -22,6 +22,15 @@ pub enum FixType { Bridge, /// A shell command that triggers an authentication flow. Auth, + /// A shell command to update the agent's main CLI. Source-aware: derived + /// from the readout's `(install_source, package_id)` so an npm-installed + /// agent is updated via `npm install -g …@latest`, not the install-time + /// curl-pipe recipe. The command is not in the static AI_AGENT_CHECKS + /// table; the executor receives it via `command_override`. + UpdateMain, + /// A shell command to update the agent's ACP bridge. Same shape as + /// `UpdateMain` — derived per-readout, passed via `command_override`. + UpdateBridge, } /// Authentication state for a check that probes credentials. @@ -76,6 +85,15 @@ pub struct AgentVersionInfo { pub update_available: Option, /// Whether this binary keeps itself up to date (curl/native installers). pub self_updating: Option, + /// Source-aware update command for this readout, derived from + /// `(install_source, package_id)`. `Some` only when an update is both + /// computable (the install source has a known update recipe and a + /// registered package id) and actionable (`update_available == Some(true)` + /// and the binary is not self-updating). Pairs with `update_fix_type`. + pub update_command: Option, + /// `FixType::UpdateMain` or `FixType::UpdateBridge` matching this readout's + /// slot. Always paired with `update_command`: both `Some` or both `None`. + pub update_fix_type: Option, } /// A single health-check result shown in the UI. From 721f1798acd5ae215fd9733b05242c8171b84d80 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 4 Jun 2026 16:24:38 +1000 Subject: [PATCH 17/18] fix(doctor): classify npm-under-brew-prefix installs as Npm, not Brew MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `npm config get prefix = /opt/homebrew`, `npm install -g` lands the package at `/opt/homebrew/lib/node_modules//...` with a bin symlink at `/opt/homebrew/bin/`. `detect_install_source` classified this as `InstallSource::Brew` because the `/opt/homebrew/` path-prefix check fired before any node_modules check and the detector never canonicalized the symlink. Three downstream failures followed: the UI mislabels the install as "via Homebrew"; `select_installed_probe(Brew, ..)` uses the `--version` CLI probe instead of the package.json reader, so npm bridges (`claude-agent-acp --version` errors) report no installed_version; and `lookup_package_id("ai-agent-claude", Brew, ..)` returns None (only Npm entries are registered), so `derive_update_command` returns None and the Update affordance disappears. Fix: before the path-prefix heuristics, canonicalize the binary path and check whether any resulting component is exactly `node_modules`. If so, return `InstallSource::Npm` regardless of the brew/cargo/etc. ancestor. Brew formulae and cargo installs never place binaries inside `node_modules/` — only `npm install -g` does, on any node distribution (brew-shipped, nvm, fnm, asdf, mise) — so this is a clean positive signal. `looks_like_npm_global` falls back to checking the path as-is when canonicalize fails (broken symlink, permissions). The Phase A curl-pipe fingerprint layer is unaffected: it only runs when the path-prefix layer returns Unknown. Tests cover: a brew-prefixed bin symlink resolving into `lib/node_modules//...` returns Npm; a genuine Cellar binary (no node_modules) is not misclassified; an nvm-layout symlink into `node_modules` returns Npm; and a plain non-node_modules binary falls through to existing detection. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/resolve.rs | 114 +++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index 631c1f627..d0e1d3681 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -334,9 +334,34 @@ fn fingerprint_curl_pipe(path: &Path, home: &Path) -> bool { false } +/// Whether the canonicalized binary path lives inside a `node_modules/` +/// directory — a clean positive signal for `npm install -g`, regardless of +/// which node distribution (brew-shipped, nvm, fnm, asdf, mise) hosts the npm +/// prefix. Brew formulae and cargo installs never place binaries inside +/// `node_modules/`, so this dominates the path-prefix heuristics below. +/// +/// The npm global bin entry is a symlink (e.g. `/opt/homebrew/bin/claude` → +/// `../lib/node_modules/@anthropic-ai/claude-code/...`), so the check resolves +/// the symlink via [`fs::canonicalize`] before inspecting components. If +/// canonicalize fails (broken symlink, permissions), fall back to the path +/// as-is — better to attempt the check than skip it. No subprocess or network. +fn looks_like_npm_global(path: &Path) -> bool { + let resolved = std::fs::canonicalize(path); + let target = resolved.as_deref().unwrap_or(path); + 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. fn detect_install_source_with_home(path: &Path, home: Option<&Path>) -> InstallSource { + // 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 + // `/opt/homebrew/` Brew path-prefix check below. + if looks_like_npm_global(path) { + return InstallSource::Npm; + } + // Homebrew-managed nvm — checked before the generic `/opt/homebrew/` // Brew prefix, since this path is a strict subset of it. if path.starts_with("/opt/homebrew/opt/nvm/versions/node") { @@ -614,6 +639,95 @@ mod tests { ); } + #[test] + fn npm_global_under_brew_prefix_classifies_as_npm() { + // `npm config get prefix = /opt/homebrew` lands the package under + // `/lib/node_modules/...` with a bin symlink at `/bin`. + // The brew path-prefix check must not win — node_modules in the + // canonicalized target is the authoritative npm signal. + let root = std::env::temp_dir().join(format!("doctor-npm-brew-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let pkg = root.join("lib/node_modules/@anthropic-ai/claude-code/bin"); + fs::create_dir_all(&pkg).unwrap(); + let real = pkg.join("claude.exe"); + File::create(&real).unwrap(); + fs::create_dir_all(root.join("bin")).unwrap(); + let link = root.join("bin/claude"); + std::os::unix::fs::symlink( + "../lib/node_modules/@anthropic-ai/claude-code/bin/claude.exe", + &link, + ) + .unwrap(); + + assert!(looks_like_npm_global(&link)); + assert_eq!( + detect_install_source_with_home(&link, None), + InstallSource::Npm, + ); + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn genuine_brew_cellar_not_misclassified_as_npm() { + // A real Cellar binary (no node_modules in its canonicalized path) must + // stay Brew — `looks_like_npm_global` returns false for it. + let root = std::env::temp_dir().join(format!("doctor-brew-cellar-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let cellar = root.join("Cellar/git/2.44.0/bin"); + fs::create_dir_all(&cellar).unwrap(); + let bin = cellar.join("git"); + File::create(&bin).unwrap(); + + assert!(!looks_like_npm_global(&bin)); + // The real `/opt/homebrew` prefix check is exercised by + // `detect_install_source_classifies_brew`; here we only assert the new + // npm layer leaves non-npm paths to fall through. + let _ = fs::remove_dir_all(&root); + } + + #[test] + fn npm_global_under_nvm_classifies_as_npm() { + // The nvm layout: ~/.local/bin/ → ~/.nvm/versions/node//lib/ + // node_modules//... Path-prefix already handled ~/.nvm directly; + // the node_modules layer must keep classifying the symlinked bin too. + let home = std::env::temp_dir().join(format!("doctor-npm-nvm-{}", std::process::id())); + let _ = fs::remove_dir_all(&home); + let pkg = home.join(".nvm/versions/node/v20.10.0/lib/node_modules/@scope/tool/bin"); + fs::create_dir_all(&pkg).unwrap(); + let real = pkg.join("tool.js"); + File::create(&real).unwrap(); + fs::create_dir_all(home.join(".local/bin")).unwrap(); + let link = home.join(".local/bin/tool"); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + assert!(looks_like_npm_global(&link)); + assert_eq!( + detect_install_source_with_home(&link, Some(home.as_path())), + InstallSource::Npm, + ); + let _ = fs::remove_dir_all(&home); + } + + #[test] + fn non_node_modules_path_falls_through_to_existing_detection() { + // A plain binary (no symlink, no node_modules) is not npm; the new layer + // returns false and existing path-prefix detection decides. + let root = std::env::temp_dir().join(format!("doctor-no-npm-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let dir = root.join("usr/local/bin"); + fs::create_dir_all(&dir).unwrap(); + let bin = dir.join("foo"); + File::create(&bin).unwrap(); + + assert!(!looks_like_npm_global(&bin)); + // System dirs still classify correctly through the unchanged fall-through. + assert_eq!( + detect_install_source_with_home(Path::new("/usr/bin/foo"), None), + InstallSource::System, + ); + 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())); From 01e90e17788d856222bc95d77e74f532b07720d8 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 4 Jun 2026 16:38:51 +1000 Subject: [PATCH 18/18] feat(doctor): echo resolved fix command as a $-preamble callback line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fix-execution streaming callback only ever forwarded subprocess stdout/stderr, so downstream callers had no record of *what* command actually ran. goose-internal's `run_fix` logs read: [agent-setup claude-acp UpdateMain] starting fix [agent-setup claude-acp UpdateMain] [agent-setup claude-acp UpdateMain] changed 2 packages in 1s [agent-setup claude-acp UpdateMain] fix succeeded — the "starting"/"succeeded" framing is goose-internal's own; the two middle lines are callback output, with no trace of the command. After this change: [agent-setup claude-acp UpdateMain] starting fix [agent-setup claude-acp UpdateMain] $ npm install -g @anthropic-ai/claude-code@latest --registry=... [agent-setup claude-acp UpdateMain] [agent-setup claude-acp UpdateMain] changed 2 packages in 1s [agent-setup claude-acp UpdateMain] fix succeeded `execute_fix_streaming_with_options` now invokes the user's per-line callback exactly once with `$ ` — after `lookup_fix_command` / `command_override` resolution and after `apply_npm_registry` has appended any `--registry=`, so the echoed string is byte-identical to what spawns — and before `Command::spawn`. Every existing caller (installs via `Command`, `Auth`, and the per-readout `UpdateMain`/`UpdateBridge` updates) gains the preamble for free. Non-breaking: the callback signature is unchanged; callers simply receive one extra line per fix invocation. The non-streaming `execute_fix_with_options` delegates through the streaming path with a no-op callback (it returns `()`, not buffered output), so no separate change is needed there. The auth probe already writes this exact `$ ` phrasing into `raw_output` (no trailing newline; the callback adds line framing), so this keeps the convention consistent across the crate. Adds a unit test driving the executor against a `command_override` of `echo hello` — the codepath `UpdateMain`/`UpdateBridge` take, since they have no static recipe — asserting the first callback line is exactly `$ echo hello` and `hello` follows, locking in preamble-before-output ordering. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- crates/doctor/src/lib.rs | 43 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs index 99cfeb964..30f9b519b 100644 --- a/crates/doctor/src/lib.rs +++ b/crates/doctor/src/lib.rs @@ -478,7 +478,7 @@ pub async fn execute_fix_streaming_with_options( fix_type: FixType, command_override: Option, npm_registry: Option<&str>, - on_line: F, + mut on_line: F, ) -> Result<(), String> where F: FnMut(&str) + Send + 'static, @@ -490,6 +490,13 @@ where }; let command = agents::apply_npm_registry(&command, npm_registry); + // Echo the resolved command as a preamble line so downstream callers (e.g. + // goose-internal's `run_fix`, which `info!`s every callback line and emits + // it via the `agent-setup:output` Tauri event) record *what* ran, not just + // its output. Matches the `$ ` phrasing the auth probe already + // writes into `raw_output`. + on_line(&format!("$ {command}")); + run_command_streaming(command, on_line).await } @@ -893,6 +900,40 @@ mod tests { ); } + /// The streaming executor must emit a `$ ` preamble line + /// through the callback *before* any subprocess output, so downstream + /// callers record what ran. Exercises the `command_override` branch + /// (the codepath `UpdateMain`/`UpdateBridge` clicks take, since they have + /// no static recipe). + #[tokio::test] + async fn execute_fix_streaming_emits_command_preamble_first() { + let lines: Arc>> = Arc::new(Mutex::new(Vec::new())); + let lines_clone = lines.clone(); + + let result = execute_fix_streaming_with_options( + "ai-agent-claude".to_string(), + FixType::UpdateMain, + Some("echo hello".to_string()), + None, + move |line| { + lines_clone.lock().unwrap().push(line.to_string()); + }, + ) + .await; + + assert!(result.is_ok(), "override should execute; got {result:?}"); + let captured = lines.lock().unwrap().clone(); + assert_eq!( + captured.first().map(String::as_str), + Some("$ echo hello"), + "first callback line must be the command preamble; captured: {captured:?}", + ); + assert!( + captured.iter().any(|l| l == "hello"), + "subprocess output line should follow the preamble; captured: {captured:?}", + ); + } + /// Without a `command_override`, `UpdateMain` has no static recipe and /// must surface as the standard "Unknown check / fix type" error. #[tokio::test]