Skip to content

Add fallback to find claude path #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tiann wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add fallback to find claude path #83

tiann wants to merge 1 commit into from

Conversation

@tiann
Copy link

@tiann tiann commented Dec 6, 2025

No description provided.

@Enzime
Copy link

Enzime commented Dec 6, 2025

I just tested this and it works.

Do you think we should make PATH the highest priority? It would be useful as it allows the user to choose the version of Claude Code they want to use, and it probably deprecates all the other methods, due to the fact that if you install Claude Code using any of the existing methods they'll all add claude to the PATH

@tiann
Copy link
Author

tiann commented Dec 7, 2025

At the moment there are actually only two cases:

  1. js modules
  2. binaries

For the binary mode, directly searching the path should be able to replace all other logic; for js modules, searching the path should also work in theory. However, keeping npm root -g as the highest priority should give better historical compatibility.

This was referenced Dec 15, 2025
Open
Open
@ahundt
Copy link

ahundt commented Dec 17, 2025 •
edited
Loading

Hey @tiann! Thanks for this PR - the PATH fallback is a great addition.

I've merged your improvements into my bun-support branch (PR #91):

  • The stdio: ['pipe', 'pipe', 'pipe'] pattern for cleaner execution
  • The existence check before symlink resolution (safer pattern)
  • The fallback to original path if resolution fails

I extended it with:

  • Source detection (so we know if it came from Bun, npm, Homebrew, etc.)
  • PATH-first priority per @Enzime's suggestion
  • HAPPY_CLAUDE_PATH env var for explicit override

I'd be happy (ahem... pun intended :-) ) to coordinate so we can get both our contributions merged! Happy to adjust anything on my side too.

@ahundt
Copy link

ahundt commented Dec 17, 2025

+1 on PATH-first priority @Enzime! I've implemented this in PR #91 - user preferences should be respected. My detection order is:

  1. HAPPY_CLAUDE_PATH env var (explicit override)
  2. PATH lookup (user's shell config)
  3. npm → Bun → Homebrew → native installer

Env var first for explicit control, then PATH respects user's shell config.

ahundt added a commit to ahundt/happy-cli that referenced this pull request Dec 17, 2025
@ahundt
feat(cli): merge PATH fallback from tiann's PR slopus#83
a8ad6cc 
Incorporates tiann's improvements from slopus#83:
- stdio suppression for cleaner which/where execution
- Existence check before symlink resolution (safer)
- Fallback to original path if resolution fails

Combined with our branch's additions:
- Source detection (npm, Bun, Homebrew, etc.)
- PATH-first priority per @Enzime's suggestion

Detection priority:
  PATH > npm > Bun > Homebrew > native

Credit: @tiann (slopus#83)
ahundt added a commit to ahundt/happy-cli that referenced this pull request Dec 17, 2025
@ahundt
feat: merge bun support and CLI path detection improvements
d27cbf7 
Merges feature/bun-support-claude-cli-detection branch with:
- Bun runtime detection and CLI discovery
- HAPPY_CLAUDE_PATH env var override
- PATH fallback from tiann's PR slopus#83 (stdio suppression, existence check)
- Source detection (npm, Bun, Homebrew, native installer)

Detection priority:
  HAPPY_CLAUDE_PATH > PATH > npm > Bun > Homebrew > native

Also fixes test issues:
- apiSession.test.ts: vi.hoisted pattern for mock hoisting
- claudeLocal.test.ts: check spawn args instead of array mutation
- runtime.test.ts: ESM imports instead of require()

Credit: @tiann (slopus#83)
@ahundt
Copy link

ahundt commented Dec 30, 2025

I've now got a version with a large range of fixes merged at #107 and I've had a zoom conversation with @bra1nDump about it. Fixes to the issues mentioned here are included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tiann @Enzime @ahundt