Skip to content

fix: improve macOS Claude CLI discovery#119

Merged
delibae merged 1 commit intodelibae:mainfrom
WhiskyChoy:codex/macos-cli-discovery
Apr 2, 2026
Merged

fix: improve macOS Claude CLI discovery#119
delibae merged 1 commit intodelibae:mainfrom
WhiskyChoy:codex/macos-cli-discovery

Conversation

@WhiskyChoy
Copy link
Copy Markdown
Contributor

This improves Unix/macOS Claude CLI discovery for installs that are not visible to a GUI app's inherited PATH.

Summary:

  • check PNPM_HOME and known pnpm locations before falling back to PATH-only discovery
  • use a login shell to query command -v claude
  • probe pnpm bin -g, npm config get prefix, and yarn global bin when those tools exist
  • extend the child-process PATH bootstrap with pnpm-related directories
  • add deterministic tests for the new path resolution helpers

This may help with macOS detection failures like #118, where ClaudePrism can remain in the Claude Code Required state even when Claude is already installed.

Notes:

  • this was validated on Windows with cargo check and cargo test (149 passing tests)
  • I could not run a native macOS build on this machine, so the remaining risk is limited to real macOS shell/runtime behavior rather than the path logic itself

Copilot AI review requested due to automatic review settings April 1, 2026 16:26
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves Unix/macOS Claude CLI discovery in the Tauri desktop app, focusing on cases where GUI-launched apps don’t inherit the user’s interactive shell PATH (e.g., pnpm/npm/yarn installs that are otherwise visible in Terminal).

Changes:

  • Adds PNPM_HOME + known pnpm locations to early path probing before PATH-only discovery.
  • Switches Unix “login shell” resolution to command -v claude and probes package-manager global locations (pnpm bin -g, npm prefix, yarn global bin).
  • Extends child-process PATH bootstrapping to include pnpm-related directories and adds deterministic unit tests for the new helpers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +382 to +385
if let Ok(output) = std::process::Command::new(&shell)
.args(["-l", "-c", command])
.output()
{
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run_login_shell_command passes -l to every fallback shell, including /bin/sh. On many Linux systems /bin/sh is dash, which does not support -l (login shell) and will fail, making the fallback ineffective when SHELL is unset. Consider special-casing shells that don't support -l (e.g., use -c for sh/dash), or detect supported flags per shell before invoking it.

Suggested change
if let Ok(output) = std::process::Command::new(&shell)
.args(["-l", "-c", command])
.output()
{
let mut cmd = std::process::Command::new(&shell);
let shell_name = PathBuf::from(&shell)
.file_name()
.and_then(|s| s.to_str())
.unwrap_or("");
if shell_name == "sh" {
cmd.args(["-c", command]);
} else {
cmd.args(["-l", "-c", command]);
}
if let Ok(output) = cmd.output() {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a good catch. For this PR I intentionally kept the scope focused on the macOS discovery path that motivated #118. On macOS we expect SHELL/zsh to be available in normal user sessions, so the /bin/sh fallback should not be on the hot path for the reported case. I agree that broader Unix-shell compatibility could be hardened further, but I would prefer to keep that as a separate follow-up unless maintainers want this PR to expand beyond the macOS-focused fix.

Comment on lines +199 to 211
if let Some(path_str) = run_login_shell_command("command -v claude") {
if PathBuf::from(&path_str).exists() {
return Ok(path_str);
}
}

if let Some(home) = dirs::home_dir() {
for path in unix_shell_manager_candidate_paths(&home) {
if path.exists() {
return Ok(path.to_string_lossy().to_string());
}
}
}
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find_claude_binary now calls run_login_shell_command multiple times (direct claude lookup plus up to three package-manager probes), and each probe may try several shells. This can result in many sequential process spawns during status checks / command execution. Consider caching the resolved shell/tool paths (e.g., OnceLock) or consolidating the probes into a single shell invocation to reduce startup latency.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I agree this increases the number of shell probes compared with the previous implementation. In this PR I prioritized correctness for GUI-launched macOS sessions where Claude is already installed but not discoverable via inherited PATH. I have not seen evidence yet that the added probes create a user-visible regression, and I would prefer to keep caching / probe consolidation as a follow-up optimization rather than broaden the scope of this fix right away.

Comment on lines +341 to +354
let mut paths = Vec::new();
if let Some(pnpm_home) = pnpm_home.filter(|value| !value.is_empty()) {
paths.push(PathBuf::from(pnpm_home).join("claude"));
}
paths.extend([
home.join("Library").join("pnpm").join("claude"),
home.join(".local").join("share").join("pnpm").join("claude"),
home.join(".pnpm").join("claude"),
home.join(".claude").join("local").join("claude"),
home.join(".npm-global").join("bin").join("claude"),
home.join(".yarn").join("bin").join("claude"),
home.join(".bun").join("bin").join("claude"),
home.join("bin").join("claude"),
]);
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pnpm path lists are duplicated across unix_claude_candidate_paths and unix_known_pnpm_claude_paths (with partial overlap). This increases the risk of the two getting out of sync over time. Consider building unix_claude_candidate_paths by reusing unix_known_pnpm_claude_paths(home) (and adding PNPM_HOME/non-pnpm entries) so pnpm layouts live in one place.

Suggested change
let mut paths = Vec::new();
if let Some(pnpm_home) = pnpm_home.filter(|value| !value.is_empty()) {
paths.push(PathBuf::from(pnpm_home).join("claude"));
}
paths.extend([
home.join("Library").join("pnpm").join("claude"),
home.join(".local").join("share").join("pnpm").join("claude"),
home.join(".pnpm").join("claude"),
home.join(".claude").join("local").join("claude"),
home.join(".npm-global").join("bin").join("claude"),
home.join(".yarn").join("bin").join("claude"),
home.join(".bun").join("bin").join("claude"),
home.join("bin").join("claude"),
]);
// Start with all known pnpm-based Claude binary locations so pnpm layouts are
// defined in a single place (see `unix_known_pnpm_claude_paths`).
let mut paths = unix_known_pnpm_claude_paths(home);
// If PNPM_HOME is set explicitly, prefer that location.
if let Some(pnpm_home) = pnpm_home.filter(|value| !value.is_empty()) {
paths.insert(0, PathBuf::from(pnpm_home).join("claude"));
}
// Add non-pnpm candidate locations.
paths.extend([
home.join(".claude").join("local").join("claude"),
home.join(".npm-global").join("bin").join("claude"),
home.join(".yarn").join("bin").join("claude"),
home.join(".bun").join("bin").join("claude"),
home.join("bin").join("claude"),
]);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that is a fair maintainability suggestion. The current split was mainly to keep the unix_known_pnpm_claude_paths helper separate from the broader candidate list that also includes PNPM_HOME and non-pnpm paths. I agree there is some duplication, but I treated it as non-blocking for this fix and wanted to avoid mixing the behavioral change with a larger internal refactor. If maintainers prefer, I am happy to clean that up in a small follow-up.

@WhiskyChoy
Copy link
Copy Markdown
Contributor Author

I replied to the Copilot suggestions above. My current preference is to keep this PR scoped to the macOS Claude CLI discovery fix and treat the broader Unix-shell compatibility, probe-caching, and path-list cleanup ideas as follow-up work, but I am happy to fold any of them into this PR if you would prefer that direction.

@delibae delibae merged commit e57162f into delibae:main Apr 2, 2026
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants