fix(doctor): handle Claude Code Homebrew casks

crates/doctor/src/lib.rs

@@ -18,7 +18,7 @@ pub use environment::DoctorEnv;
 pub use types::{AgentVersionInfo, CheckStatus, DoctorCheck, DoctorReport, FixType};
 
 use std::collections::{HashMap, HashSet};
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
 
 use agents::{check_single_ai_agent, derive_update_command, lookup_fix_command, AI_AGENT_CHECKS};
@@ -30,10 +30,12 @@ use freshness::{
     fetch_version_info, is_self_updating, load_cache, save_cache, select_installed_probe,
     FetchVersionInfoOptions,
 };
-use package_ids::{lookup_package_id, LatestSource, Role};
+use package_ids::{lookup_package_id_for_binary, LatestSource, Role};
 use resolve::resolve_binary_with_diagnostics;
 use types::{InstallSource, ResolvedBinary};
 
+const SNAPSHOT_PATH_ENV: &str = "__BUILDERBOT_DOCTOR_SNAPSHOT_PATH";
+
 /// Fallback check returned when a spawn_blocking task panics.
 fn empty_check(id: &str, label: &str) -> DoctorCheck {
     DoctorCheck {
@@ -379,11 +381,12 @@ fn resolve_package(
     check_id: &str,
     source: Option<&InstallSource>,
     role: Role,
+    binary_path: Option<&Path>,
 ) -> (Option<String>, Option<LatestSource>) {
     source
         .cloned()
-        .and_then(|src| lookup_package_id(check_id, src, role))
-        .map(|(pkg, latest)| (Some(pkg.to_string()), Some(latest)))
+        .and_then(|src| lookup_package_id_for_binary(check_id, src, role, binary_path))
+        .map(|(pkg, latest)| (Some(pkg), Some(latest)))
         .unwrap_or((None, None))
 }
 
@@ -489,24 +492,34 @@ async fn populate_freshness(
         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(), Role::Main);
+                let path = PathBuf::from(path);
+                let (package_id, latest_source) = resolve_package(
+                    &check.id,
+                    readout.install_source.as_ref(),
+                    Role::Main,
+                    Some(&path),
+                );
                 targets.push(FreshnessTarget {
                     id: check.id.clone(),
                     slot: ReadoutSlot::Main,
-                    path: PathBuf::from(path),
+                    path,
                     latest_source,
                     package_id,
                     install_source: readout.install_source.clone(),
                 });
             }
             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(), Role::Bridge);
+                let path = PathBuf::from(path);
+                let (package_id, latest_source) = resolve_package(
+                    &check.id,
+                    readout.install_source.as_ref(),
+                    Role::Bridge,
+                    Some(&path),
+                );
                 targets.push(FreshnessTarget {
                     id: check.id.clone(),
                     slot: ReadoutSlot::Bridge,
-                    path: PathBuf::from(path),
+                    path,
                     latest_source,
                     package_id,
                     install_source: readout.install_source.clone(),
@@ -517,12 +530,17 @@ async fn populate_freshness(
             // 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(), Role::Any);
+            let path = PathBuf::from(path_str);
+            let (package_id, latest_source) = resolve_package(
+                &check.id,
+                check.install_source.as_ref(),
+                Role::Any,
+                Some(&path),
+            );
             targets.push(FreshnessTarget {
                 id: check.id.clone(),
                 slot: ReadoutSlot::Flat,
-                path: PathBuf::from(path_str),
+                path,
                 latest_source,
                 package_id,
                 install_source: check.install_source.clone(),
@@ -795,17 +813,19 @@ enum StreamLine {
 /// caller env snapshot, `path_prefix` keeps the legacy behavior of prepending
 /// resolved binary dirs plus conservative fallbacks. With a snapshot, doctor
 /// preserves the snapshot environment and appends missing prefix dirs to its
-/// `PATH` so auth probes can still find resolved binaries without replacing
-/// the caller's environment.
+/// `PATH` so auth probes can still find resolved binaries. Because login shell
+/// startup can rewrite `PATH` after process spawn, snapshot callers also carry
+/// the merged path in an internal env var and restore it inside the `-c`
+/// payload immediately before running the requested command.
 fn build_shell_command(
     command: &str,
     path_prefix: &[PathBuf],
     env: Option<&DoctorEnv>,
 ) -> std::process::Command {
-    let (shell, args) = if std::path::Path::new("/bin/zsh").exists() {
-        ("/bin/zsh", vec!["-l", "-c", command])
+    let shell = if std::path::Path::new("/bin/zsh").exists() {
+        "/bin/zsh"
     } else {
-        ("/bin/bash", vec!["-l", "-c", command])
+        "/bin/bash"
     };
     let home = env
         .and_then(|e| e.get("HOME").map(str::to_string))
@@ -816,8 +836,9 @@ fn build_shell_command(
         .or_else(|| std::env::var("USER").ok())
         .unwrap_or_default();
 
+    let mut shell_command = command.to_string();
     let mut cmd = std::process::Command::new(shell);
-    cmd.args(&args).env_clear();
+    cmd.env_clear();
 
     if let Some(env) = env {
         for (key, value) in &env.vars {
@@ -831,7 +852,9 @@ fn build_shell_command(
         }
         cmd.env("TERM", "xterm-256color").current_dir(&home);
         if let Some(path) = merged_snapshot_path(env.get("PATH"), path_prefix) {
-            cmd.env("PATH", path);
+            cmd.env("PATH", &path);
+            cmd.env(SNAPSHOT_PATH_ENV, path);
+            shell_command = command_with_snapshot_path_restore(command);
         }
     } else {
         cmd.env("HOME", &home)
@@ -844,9 +867,17 @@ fn build_shell_command(
         }
     }
 
+    cmd.arg("-l").arg("-c").arg(shell_command);
     cmd
 }
 
+fn command_with_snapshot_path_restore(command: &str) -> String {
+    let name = SNAPSHOT_PATH_ENV;
+    format!(
+        "if [ \"${{{name}+x}}\" = x ]; then PATH=\"${{{name}}}\"; export PATH; unset {name}; fi\n{command}",
+    )
+}
+
 fn legacy_prefixed_path(path_prefix: &[PathBuf]) -> String {
     let mut seen: std::collections::HashSet<String> = std::collections::HashSet::new();
     let mut path = String::new();
@@ -1071,6 +1102,19 @@ mod tests {
         }
     }
 
+    fn shell_quote(value: &str) -> String {
+        format!("'{}'", value.replace('\'', "'\\''"))
+    }
+
+    fn write_login_path_rewrite_profiles(home: &Path, path: &Path) {
+        let profile = format!(
+            "export PATH={}\n",
+            shell_quote(&format!("{}:/usr/bin:/bin", path.to_string_lossy())),
+        );
+        std::fs::write(home.join(".zprofile"), &profile).unwrap();
+        std::fs::write(home.join(".bash_profile"), profile).unwrap();
+    }
+
     #[test]
     fn timeout_diagnostic_ids_are_collision_safe() {
         let checks = timeout_diagnostic_checks(vec![
@@ -1247,6 +1291,41 @@ mod tests {
         }
     }
 
+    #[tokio::test]
+    async fn exec_command_with_env_snapshot_restores_path_after_login_shell_rewrite() {
+        let tmp = unique_tmp_dir("auth-env-path-login-rewrite");
+        let snapshot_bin = tmp.join("nvm/bin");
+        let login_rewrite_bin = tmp.join("homebrew/bin");
+        let script_name = "doctor-auth-path-probe";
+        write_executable(&snapshot_bin.join(script_name), "#!/bin/sh\nexit 0\n");
+        write_executable(&login_rewrite_bin.join(script_name), "#!/bin/sh\nexit 42\n");
+        write_login_path_rewrite_profiles(&tmp, &login_rewrite_bin);
+
+        let env = DoctorEnv::new(vec![
+            (
+                "PATH".to_string(),
+                format!("{}:/usr/bin:/bin", snapshot_bin.to_string_lossy()),
+            ),
+            ("HOME".to_string(), tmp.to_string_lossy().to_string()),
+            ("USER".to_string(), "doctor-test".to_string()),
+            ("ZDOTDIR".to_string(), tmp.to_string_lossy().to_string()),
+        ]);
+
+        let outcome = tokio::task::spawn_blocking(move || {
+            execute_command_with_path_prefix_with_env(script_name, &[], Some(&env))
+        })
+        .await
+        .unwrap();
+
+        let _ = std::fs::remove_dir_all(&tmp);
+        match outcome {
+            ExecOutcome::Ok => {}
+            other => {
+                panic!("expected snapshot PATH binary to beat login profile rewrite; got {other:?}")
+            }
+        }
+    }
+
     /// `apply_freshness` derives a source-aware update command for a Main slot
     /// when the npm-installed agent has an actionable update.
     #[tokio::test]
@@ -1274,6 +1353,55 @@ mod tests {
         assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain));
     }
 
+    /// Claude Code's Homebrew cask should update as the main CLI via brew, not
+    /// via the npm package used by npm-managed installs.
+    #[tokio::test]
+    async fn apply_freshness_brew_main_emits_claude_code_update_command() {
+        let mut readout = AgentVersionInfo {
+            install_source: Some(InstallSource::Brew),
+            ..AgentVersionInfo::default()
+        };
+        let info = freshness::VersionInfo {
+            installed: Some("2.1.152".into()),
+            latest: Some("2.1.153".into()),
+            update_available: Some(true),
+            command_timeouts: Vec::new(),
+        };
+        apply_freshness(&mut readout, &info, ReadoutSlot::Main, Some("claude-code"));
+        assert_eq!(
+            readout.update_command.as_deref(),
+            Some("brew upgrade claude-code"),
+        );
+        assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain));
+    }
+
+    /// The Homebrew latest-channel cask should keep its token all the way
+    /// through to the generated update command.
+    #[tokio::test]
+    async fn apply_freshness_brew_main_emits_claude_code_latest_update_command() {
+        let mut readout = AgentVersionInfo {
+            install_source: Some(InstallSource::Brew),
+            ..AgentVersionInfo::default()
+        };
+        let info = freshness::VersionInfo {
+            installed: Some("2.1.152".into()),
+            latest: Some("2.1.153".into()),
+            update_available: Some(true),
+            command_timeouts: Vec::new(),
+        };
+        apply_freshness(
+            &mut readout,
+            &info,
+            ReadoutSlot::Main,
+            Some("claude-code@latest"),
+        );
+        assert_eq!(
+            readout.update_command.as_deref(),
+            Some("brew upgrade claude-code@latest"),
+        );
+        assert_eq!(readout.update_fix_type, Some(FixType::UpdateMain));
+    }
+
     /// Bridge slot with a brew install upgrades via `brew upgrade <pkg>` and
     /// is tagged `UpdateBridge`.
     #[tokio::test]
@@ -1443,6 +1571,68 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn execute_fix_streaming_update_restores_snapshot_path_after_login_shell_rewrite() {
+        let tmp = unique_tmp_dir("fix-update-env-path-login-rewrite");
+        let snapshot_bin = tmp.join("nvm/bin");
+        let login_rewrite_bin = tmp.join("homebrew/bin");
+        write_executable(
+            &snapshot_bin.join("npm"),
+            "#!/bin/sh\n\
+             echo \"snapshot-npm $*\"\n",
+        );
+        write_executable(
+            &login_rewrite_bin.join("npm"),
+            "#!/bin/sh\n\
+             echo \"homebrew-npm $*\"\n\
+             exit 42\n",
+        );
+        write_login_path_rewrite_profiles(&tmp, &login_rewrite_bin);
+
+        let env = DoctorEnv::new(vec![
+            (
+                "PATH".to_string(),
+                format!("{}:/usr/bin:/bin", snapshot_bin.to_string_lossy()),
+            ),
+            ("HOME".to_string(), tmp.to_string_lossy().to_string()),
+            ("USER".to_string(), "doctor-test".to_string()),
+            ("ZDOTDIR".to_string(), tmp.to_string_lossy().to_string()),
+        ]);
+        let lines: Arc<Mutex<Vec<String>>> = Arc::new(Mutex::new(Vec::new()));
+        let lines_clone = lines.clone();
+        let command = "npm install -g @anthropic-ai/claude-code@latest";
+
+        let result = execute_fix_streaming_with_env_options(
+            "ai-agent-claude".to_string(),
+            FixType::UpdateMain,
+            ExecuteFixOptions {
+                command_override: Some(command.to_string()),
+                npm_registry: None,
+                env: Some(env),
+            },
+            move |line| {
+                lines_clone.lock().unwrap().push(line.to_string());
+            },
+        )
+        .await;
+
+        let captured = lines.lock().unwrap().clone();
+        let _ = std::fs::remove_dir_all(&tmp);
+        assert!(result.is_ok(), "snapshot npm update failed: {result:?}");
+        assert!(
+            captured
+                .iter()
+                .any(|line| line == "snapshot-npm install -g @anthropic-ai/claude-code@latest"),
+            "expected update to run through snapshot npm; captured: {captured:?}",
+        );
+        assert!(
+            captured
+                .iter()
+                .all(|line| !line.starts_with("homebrew-npm")),
+            "login profile PATH rewrite should not select homebrew npm; captured: {captured:?}",
+        );
+    }
+
     /// Without a `command_override`, `UpdateMain` has no static recipe and
     /// must surface as the standard "Unknown check / fix type" error.
     #[tokio::test]