- Notifications
You must be signed in to change notification settings - Fork 46
Fix Claude adapter to support shell function definitions#20
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.
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
Fix Claude adapter to support shell function definitions #20
Uh oh!
There was an error while loading. Please reload this page.
Conversation
him0 commented Nov 18, 2025 • edited by coderabbitai bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by coderabbitai bot
Uh oh!
There was an error while loading. Please reload this page.
When `claude` is defined as a shell function (e.g., wrapping ~/.claude/local/claude), the previous implementation failed because shell functions are not inherited by subshells. Changes: - Add `find_claude_executable()` helper to detect executable files only - Check common installation path (~/.claude/local/claude) first - Use `type -P` to find executables, excluding shell functions - Both `ai_can_start()` and `ai_start()` now use the executable path directly This allows the adapter to work correctly even when users have `claude` defined as a shell function that wraps the actual executable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
coderabbitaibot commented Nov 18, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
📝 WalkthroughWalkthroughRefactors the Claude adapter by adding Changes
Sequence DiagramsequenceDiagram participant Caller participant Finder as find_claude_executable() participant ai_can_start participant ai_start participant System Caller->>Finder: Locate Claude executable Finder->>System: Check local path alt found Finder-->>Caller: return path else not found Finder->>System: type -P check alt found Finder-->>Caller: return path else fallback Finder->>System: other type checks Finder-->>Caller: return path or empty end end Caller->>ai_can_start: Is Claude available? ai_can_start->>Finder: resolve executable ai_can_start-->>Caller: availability (true/false) Caller->>ai_start: start Claude in target dir ai_start->>Finder: resolve executable alt executable resolved ai_start->>System: exec resolved command in target dir System-->>Caller: started else not resolved ai_start-->>Caller: error: claude command not found end Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
adapters/ai/claude.sh (2)
15-24: Remove the redundant shell availability check on line 17.
typeis a bash builtin and will always be available in this bash script, makingif command -v type >/dev/null 2>&1;ineffective and confusing. The check always succeeds, adding unnecessary overhead.Apply this diff to simplify:
# Try to find executable using type -P (Bash/Zsh) for cmd in claude claude-code; do - if command -v type >/dev/null 2>&1; then- local exe_path- exe_path="$(type -P "$cmd" 2>/dev/null)" && [ -n "$exe_path" ] &&{- echo "$exe_path"- return 0- }- fi+ local exe_path+ exe_path="$(type -P "$cmd" 2>/dev/null)" && [ -n "$exe_path" ] &&{+ echo "$exe_path"+ return 0+ } done
26-34: Fallback logic may accept unintended command types (aliases, builtins).The pattern
grep -qv "function"correctly excludes shell functions but has loose filtering. The condition matches anytypeoutput that doesn't contain the word "function"—this includes aliases (aliased to), builtins, and executables. Ifclaudeis aliased to something else in a user's shell, this fallback would incorrectly accept it. Consider a more explicit check, such as verifying that the resolved command is actually an executable file.For robustness, prefer using
type -P(which only returns executables) or explicitly verify the command is a file path:# Fallback: use command -v but exclude functions for cmd in claude claude-code; do - if type "$cmd" 2>/dev/null | grep -qv "function"; then- if command -v "$cmd" >/dev/null 2>&1; then- echo "$cmd"- return 0- fi- fi+ if ! type "$cmd" 2>/dev/null | grep -q "^.*is a function"; then+ if [ -x "$(command -v "$cmd")" ]; then+ echo "$(command -v "$cmd")"+ return 0+ fi+ fi doneThis ensures we only accept executables (files with execute permission) rather than aliases or builtins.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
adapters/ai/claude.sh(3 hunks)
🔇 Additional comments (1)
adapters/ai/claude.sh (1)
40-65: Updated functions correctly resolve and use the Claude executable.The refactoring successfully centralizes executable detection in
find_claude_executable(). The primary fix (checking~/.claude/local/claudedirectly) correctly addresses the PR objective: whenclaudeis a shell function, the subshell spawned on line 64 will inherit and execute the resolved executable path, not the function definition.
- Line 41:
ai_can_start()delegation is idiomatic and correct.- Lines 50–57: Defensive check for empty
claude_cmdis good practice.- Line 64: Subshell execution with
"$claude_cmd" "$@"works correctly regardless of whether the command is a full path or a searchable name.
| fi | ||
| # Try to find executable using type -P (Bash/Zsh) | ||
| forcmdin claude claude-code;do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code checks if type exists before using it, but type is a Bash builtin that's always available in bash scripts.
adapters/ai/claude.sh Outdated
| # Fallback: use command -v but exclude functions | ||
| forcmdin claude claude-code;do | ||
| iftype"$cmd"2>/dev/null | grep -qv "function";then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback tier uses grep -qv "function" which matches anything that doesn't contain the word "function", including aliases
- Remove unnecessary check for 'type' builtin (always available in Bash) - Replace grep-based function filtering with 'type -t' for accurate detection - Now properly excludes both shell functions and aliases - Only detects actual executable files This addresses PR review feedback about redundant checks and potential false positives when aliases are present. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
adapters/ai/claude.sh (1)
4-33: Well-structured multi-tier resolution strategy to bypass shell functions.The function correctly distinguishes between executables and shell functions by:
- Testing an explicit common install path first
- Using
type -Pto locate only actual executables (functions/aliases excluded)- Adding a defensive fallback that verifies
type -t = "file"before invokingcommand -vThis addresses the PR objective of supporting
claudedefined as a shell function, sincetype -Pand the explicit path check bypass function definitions entirely, while Strategy 3 confirms any fallback match is an actual file, not a function.Consider: Strategy 3 (lines 25–30) may be redundant—if
type -Pdoesn't find an executable in PATH, thetype -t = "file"check is unlikely to succeed either. If this tier never executes in practice, you could simplify by removing it (or keeping it defensively). Verify whether your test suite or real deployments actually hit this fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
adapters/ai/claude.sh(3 hunks)
🔇 Additional comments (2)
adapters/ai/claude.sh (2)
36-38: Clean delegation to helper.Straightforward use of the new helper to check availability.
42-61: Robust error handling and clean execution pattern.The function correctly:
- Captures the resolved executable path upfront (lines 46–47)
- Validates non-empty result with an informative error message (lines 49–53)
- Executes in a subshell (line 60) to isolate the working directory change and prevent function-pollution
b26f67c into coderabbitai:mainUh oh!
There was an error while loading. Please reload this page.
him0 commented Nov 22, 2025
Thank you for your review. |
Problem
When users define
claudeas a shell function (e.g., a wrapper around~/.claude/local/claude), the Claude adapter fails with "Claude Code not found" error. This happens because shell functions are not inherited by subshells.Users often wrap the Claude CLI in a shell function to set environment variables (like HTTP headers for authentication) or customize the execution environment:
Solution
This PR adds a
find_claude_executable()helper function that:~/.claude/local/claude(the typical installation location)type -Pto find executable files only - Excludes shell functions and aliases (Bash/Zsh compatible)command -vwith function detection for compatibilityBoth
ai_can_start()andai_start()now use this helper to find the actual executable path, ensuring the adapter works correctly even whenclaudeis defined as a shell function.Testing
Tested with
claudedefined as a shell function. The adapter now correctly:~/.claude/local/claude🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Bug Fixes