fix: enhanced PATH for npx/npm in Electron apps (Issue #523) #540
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughA new function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @adryserage, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request provides a crucial fix for Electron applications that fail to locate Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of finding npx/npm in Electron apps by enhancing the PATH environment variable. The new get_enhanced_path function in Python is well-implemented and covers a comprehensive set of common installation locations for Node.js across different platforms. The PR also includes related improvements for locating the claude CLI, which is a welcome addition. My review includes a few suggestions to improve code clarity and maintainability, such as removing empty code blocks, ensuring important debug logs are not lost, and standardizing module imports. Overall, these are solid changes that improve the application's robustness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
auto-claude-ui/src/main/python-env-manager.ts (1)
544-556: Consider optimizing Python version directory discovery.While the hardcoded Python version list works, it could miss newer/older versions or non-standard naming. Consider dynamically discovering Python version directories:
🔎 Optional: Dynamic Python version discovery
} else { - // Unix: venv/lib/pythonX.Y/site-packages/claude_agent_sdk/_bundled/claude - // Try common Python versions - for (const pyVer of ['python3.12', 'python3.11', 'python3.10', 'python3.9', 'python3']) { + // Unix: venv/lib/pythonX.Y/site-packages/claude_agent_sdk/_bundled/claude + // Dynamically discover Python version directories + const libPath = path.join(venvPath, 'lib') + if (existsSync(libPath)) { + try { + const entries = require('node:fs').readdirSync(libPath, { withFileTypes: true }) + const pythonDirs = entries + .filter(e => e.isDirectory() && e.name.startsWith('python')) + .map(e => e.name) + + for (const pyVer of pythonDirs) { + candidates.push( + path.join(venvPath, 'lib', pyVer, 'site-packages', 'claude_agent_sdk', '_bundled', cliName) + ) + } + } catch { + // Fallback to hardcoded list + for (const pyVer of ['python3.12', 'python3.11', 'python3.10', 'python3.9', 'python3']) { + candidates.push( + path.join(venvPath, 'lib', pyVer, 'site-packages', 'claude_agent_sdk', '_bundled', cliName) + ) + } + } + } - candidates.push( - path.join( - venvPath, - 'lib', - pyVer, - 'site-packages', - 'claude_agent_sdk', - '_bundled', - cliName - ) - ) - } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
auto-claude-ui/src/main/agent/agent-process.tsauto-claude-ui/src/main/python-env-manager.tsauto-claude/core/auth.pyauto-claude/core/client.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/.env* : Enable Electron MCP for E2E testing by setting `ELECTRON_MCP_ENABLED=true` in `.env` and starting the Electron app with `npm run dev`
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/core/client.py : Implement agent-specific tool permissions in the Claude SDK client based on agent role (planner, coder, qa_reviewer, qa_fixer)
Applied to files:
auto-claude/core/client.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (`claude-agent-sdk` package) for all AI interactions, never use the Anthropic API directly
Applied to files:
auto-claude/core/client.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/**/*.py : Use the `create_client()` function from `apps/backend/core/client.py` to instantiate Claude SDK clients, not direct `ClaudeSDKClient` initialization
Applied to files:
auto-claude/core/client.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/core/**/*.py : Implement dynamic command allowlisting based on detected project stack using `core/security.py` for bash command validation
Applied to files:
auto-claude/core/client.py
🧬 Code graph analysis (2)
auto-claude-ui/src/main/python-env-manager.ts (2)
auto-claude-ui/src/__mocks__/electron.ts (1)
app(8-22)auto-claude-ui/scripts/postinstall.js (1)
isWindows(19-19)
auto-claude-ui/src/main/agent/agent-process.ts (5)
auto-claude-ui/src/main/python-detector.ts (2)
findPythonCommand(9-37)parsePythonCommand(56-61)auto-claude-ui/src/main/project-store.ts (1)
projectStore(618-618)auto-claude-ui/src/main/rate-limit-detector.ts (2)
detectRateLimit(97-154)createSDKRateLimitInfo(357-382)auto-claude-ui/src/main/claude-profile-manager.ts (1)
getClaudeProfileManager(531-536)auto-claude-ui/src/main/python-env-manager.ts (1)
pythonEnvManager(626-626)
🔇 Additional comments (11)
auto-claude/core/auth.py (2)
163-243: LGTM! Enhanced PATH implementation is thorough and well-structured.The function correctly addresses Issue #523 by prepending common Node.js/npm/npx locations to PATH. The implementation handles Windows, macOS, and Linux appropriately with comprehensive coverage of version managers (nvm, fnm, volta, asdf) and system locations.
Key strengths:
- Validates directories exist before adding
- Avoids duplicates by checking current PATH entries
- Uses
os.pathsepfor cross-platform compatibility- Properly handles Windows environment variables and registry paths
- Covers multiple version manager patterns
265-266: The enhanced PATH is correctly constructed and passed to the ClaudeSDKClient via theenvparameter. However, verification that theclaude_agent_sdklibrary actually uses this environment when spawning MCP server subprocesses (npx, npm) requires access to the SDK's implementation, which is external to this repository. Confirm with Anthropic SDK documentation or testing that theenvparameter is respected during subprocess spawning.auto-claude/core/client.py (3)
174-175: LGTM! CLI path handling addresses packaged Electron app issues.The explicit CLI path retrieval and passing to
ClaudeSDKClientcorrectly addresses Issue #529 for packaged Electron apps where the bundled CLI may not be in PATH. The implementation follows best practices by:
- Using environment variable for configuration
- Making it optional (only set when needed)
- Passing directly to SDK options
Also applies to: 351-351
243-246: Security allowlist correctly updated for new tools.The permissions for WebSearch, WebFetch, and Task are appropriately added with wildcard matchers, maintaining consistency with other tool permissions like Bash(*).
129-131: WebSearch, WebFetch, and Task tools are supported and properly configured.The web search results confirm these tools are supported in the Claude Agent SDK. The implementation correctly integrates them into the BUILTIN_TOOLS list and passes them to ClaudeSDKClient via role-based filtering through
get_agent_allowed_tools(), which respects agent type (planner, coder, qa_reviewer, qa_fixer). No SDK compatibility issues.auto-claude-ui/src/main/agent/agent-process.ts (2)
1-4: LGTM! Node.js built-in imports modernized with node: prefix.The import updates follow Node.js best practices by using the
node:prefix for built-in modules, which provides better clarity and prevents conflicts with npm packages.
409-421: LGTM! Claude CLI path correctly integrated into environment.The
getCombinedEnvmethod now includes the Claude CLI path frompythonEnvManager, ensuring that spawned processes can locate the bundled CLI. The conditional spreading pattern (...(claudeCliPath ? { CLAUDE_CLI_PATH: claudeCliPath } : {})) cleanly handles the case where the CLI path is not found.auto-claude-ui/src/main/python-env-manager.ts (4)
422-453: LGTM! Synchronous dependency checker is secure and well-implemented.The
checkDepsForPathSyncmethod usesspawnSyncwith array arguments (not shell execution), which prevents command injection. The implementation:
- Validates Python path existence upfront
- Uses direct package imports for checking
- Returns structured results with missing dependencies
- Handles errors gracefully
459-514: LGTM! Dependency installer is secure with proper progress reporting.The
installDepsForPathmethod correctly usesspawnwith array arguments for security. Key strengths:
- No shell execution (prevents injection)
- Progress callbacks for UI feedback
- Proper error handling with descriptive messages
- Clean async/Promise pattern
526-615: LGTM! Claude CLI discovery is comprehensive and well-documented.The
getClaudeCliPathmethod implements thorough CLI discovery across multiple locations:
- Venv site-packages (checking multiple Python versions on Unix)
- Packaged app resources
- Common user/system locations
- System PATH via
which/whereSecurity considerations are properly handled:
- Uses
existsSyncfor validationexecFileSyncwith array args (no shell injection)- Appropriate timeout protection
The multi-version Python path checking (lines 544-556) is particularly good for handling different Python installations.
620-622: LGTM! Public accessor provides clean API for venv path.The
getVenvPathmethod appropriately exposes the privategetVenvBasePathmethod, maintaining encapsulation while allowing external components to access the venv location.
adryserage
force-pushed
the
hotfix/electron-path-npx-523
branch
from
0b4951a to
3c3355e
Compare
adryserage
force-pushed
the
hotfix/electron-path-npx-523
branch
3 times, most recently
from
4566a90 to
b7ebbbf
Compare
MikeeBuilds
added
bug
Review Feedback AcknowledgedIssue: Empty These empty DEBUG check blocks should be removed for code clarity. Will clean up in a follow-up. |
|
AlexMadera
left a comment
AlexMadera
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
4 issue(s) must be addressed (0 required, 4 recommended), 3 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 4 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (7 selected of 7 total)
🟡 [MEDIUM] Missing exception handling for iterdir() calls
📁 auto-claude/core/auth.py:203
The iterdir() calls on lines 203, 224, and 231 can raise PermissionError or OSError if directories exist but are not readable. Even though .exists() is checked first, the directory enumeration can still fail if permissions change or the directory becomes inaccessible during iteration.
Suggested fix:
Wrap iterdir() calls in try-except blocks to gracefully handle PermissionError: `try: for version_dir in nvm_path.iterdir(): ... except PermissionError: pass`
🟡 [MEDIUM] Path.home() can raise RuntimeError in containerized environments
📁 auto-claude/core/auth.py:177
Path.home() can raise RuntimeError if the home directory cannot be determined, which can occur in containerized environments, CI systems, or unusual system configurations. The function would crash instead of gracefully falling back to the current PATH.
Suggested fix:
Wrap Path.home() in try-except: `try: home = Path.home() except RuntimeError: return current_path`
🟡 [MEDIUM] Windows fnm_multishells path does not contain node binaries
📁 auto-claude/core/auth.py:194
The code adds home / 'AppData' / 'Local' / 'fnm_multishells' to PATH, but fnm uses dynamically-named subdirectories for each shell session within this directory. The parent directory itself does not contain node binaries, making this path ineffective for finding npx/npm.
Suggested fix:
Either remove this path or iterate over fnm_multishells subdirectories similar to how nvm versions are handled. fnm on Windows stores current version in shell-specific numbered subdirectories.
🟡 [MEDIUM] User-writable ~/node_modules/.bin added to PATH with high priority
📁 auto-claude/core/auth.py:218
The path ~/node_modules/.bin is added as a candidate and will be prepended to PATH (high priority). This directory is easily writable by any process running as the user, and is not a standard Node.js installation location. Any malware or compromised npm package could place executables here that would then be executed by Auto-Claude.
Suggested fix:
Remove `home / 'node_modules' / '.bin'` from the candidates list. This is not a standard global npm installation location.
🔵 [LOW] PATH manipulation function in auth.py violates Single Responsibility
📁 auto-claude/core/auth.py:163
The get_enhanced_path() function handles environment PATH manipulation but is placed in auth.py, a module explicitly documented for 'authentication token resolution'. This violates the Single Responsibility Principle and makes the codebase harder to navigate. The SDK env vars function name also doesn't indicate it modifies PATH.
Suggested fix:
Create a follow-up issue to refactor get_enhanced_path() to a dedicated module like environment.py or path_utils.py. Consider renaming get_sdk_env_vars() to indicate PATH modification.
🔵 [LOW] Additional paths prepended give user directories higher priority than system paths
📁 auto-claude/core/auth.py:241
The additional paths are prepended to existing PATH (higher priority). This means user-writable directories like ~/.volta/bin or ~/.local/bin take precedence over system paths. While convenient, this could allow path hijacking if any of these directories are compromised.
Suggested fix:
Consider reordering so system paths (/usr/local/bin, /usr/bin) come before user-writable paths, or append rather than prepend user directories.
🔵 [LOW] No unit tests exist for get_enhanced_path() function
📁 auto-claude/core/auth.py:163
The new get_enhanced_path() function has tight coupling with os.environ, Path.home(), and filesystem operations, making it difficult to test. No tests were added in this PR despite the function having platform-specific logic and multiple edge cases.
Suggested fix:
Add unit tests using dependency injection or monkeypatching to test: empty PATH, inaccessible home directory, permission errors during iterdir, various platform scenarios.
This review was generated by Auto Claude.
MikeeBuilds
added
❌ MERGE CONFLICTS
os/macos
Adds get_enhanced_path() function to include common Node.js/npm/npx locations for GUI-launched Electron apps that don't inherit shell PATH modifications. Fixes AndyMik90#523
adryserage
force-pushed
the
hotfix/electron-path-npx-523
branch
from
b7ebbbf to
2bc087f
Compare
|
Rebased onto |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/backend/core/auth.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/auth.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/auth.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/auth.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (4)
apps/backend/core/auth.py (4)
13-13: LGTM: Import is appropriate for the new functionality.The
pathlib.Pathimport is needed for the filesystem operations inget_enhanced_path()and is correctly utilized throughout.
219-231: Good approach to solving the PATH discovery issue.The function correctly addresses the core problem: Electron apps launched from GUI lack shell-initialized environment variables. The docstring clearly explains the purpose and references Issue #523.
304-326: LGTM: Enhanced PATH integration is correct.The modification to
get_sdk_env_vars()correctly adds the enhanced PATH to the SDK subprocess environment. This ensures npx/npm commands are discoverable when spawning MCP servers, addressing the core issue from #523.The unconditional PATH override is appropriate here since:
- PATH is not in the
SDK_ENV_VARSwhitelist, so it wasn't being passed beforeget_enhanced_path()safely returns the original PATH if no enhancements are found- The prepended paths have higher priority, which is the desired behavior
219-326: No action required — PR #533 does not exist.The duplicate PR concern cannot be substantiated. PR #533 does not appear in the repository's git history or GitHub PR system. The current implementation properly addresses Issue #523 with clear documentation and appropriate code structure.
| def get_enhanced_path() -> str: | ||
| """ | ||
| Get an enhanced PATH that includes common Node.js/npm/npx locations. | ||
| Electron apps launched from GUI don't inherit shell PATH modifications | ||
| from .bashrc/.zshrc or Node version managers (nvm, fnm, volta, asdf). | ||
| This function adds common locations to ensure npx/npm are found. | ||
| Fixes Issue #523: Cannot add custom mcp servers using npx | ||
| Returns: | ||
| Enhanced PATH string with additional Node.js locations | ||
| """ | ||
| current_path = os.environ.get("PATH", "") | ||
| home = Path.home() | ||
| additional_paths: list[str] = [] | ||
|
|
||
| if platform.system() == "Windows": | ||
| # Windows common locations | ||
| appdata = os.environ.get("APPDATA", "") | ||
| programfiles = os.environ.get("PROGRAMFILES", "C:\\Program Files") | ||
| programfiles_x86 = os.environ.get( | ||
| "PROGRAMFILES(X86)", "C:\\Program Files (x86)" | ||
| ) | ||
| nvm_home = os.environ.get("NVM_HOME", "") | ||
|
|
||
| candidates = [ | ||
| Path(appdata) / "npm" if appdata else None, | ||
| Path(programfiles) / "nodejs", | ||
| Path(programfiles_x86) / "nodejs", | ||
| home / "AppData" / "Roaming" / "npm", | ||
| home / "AppData" / "Local" / "fnm_multishells", | ||
| home / ".volta" / "bin", | ||
| ] | ||
|
|
||
| # Add nvm-windows paths | ||
| if nvm_home: | ||
| nvm_path = Path(nvm_home) | ||
| if nvm_path.exists(): | ||
| # Find installed node versions | ||
| for version_dir in nvm_path.iterdir(): | ||
| if version_dir.is_dir() and version_dir.name.startswith("v"): | ||
| candidates.append(version_dir) | ||
|
|
||
| else: | ||
| # macOS / Linux common locations | ||
| candidates = [ | ||
| Path("/usr/local/bin"), | ||
| Path("/usr/bin"), | ||
| Path("/opt/homebrew/bin"), # Apple Silicon Homebrew | ||
| home / ".local" / "bin", | ||
| home / ".volta" / "bin", | ||
| home / ".fnm" / "current" / "bin", | ||
| home / ".asdf" / "shims", | ||
| home / ".npm-global" / "bin", | ||
| home / "node_modules" / ".bin", | ||
| ] | ||
|
|
||
| # Add nvm paths (find installed versions) | ||
| nvm_dir = home / ".nvm" / "versions" / "node" | ||
| if nvm_dir.exists(): | ||
| for version_dir in nvm_dir.iterdir(): | ||
| if version_dir.is_dir(): | ||
| candidates.append(version_dir / "bin") | ||
|
|
||
| # Add fnm paths | ||
| fnm_dir = home / ".fnm" / "node-versions" | ||
| if fnm_dir.exists(): | ||
| for version_dir in fnm_dir.iterdir(): | ||
| if version_dir.is_dir(): | ||
| candidates.append(version_dir / "installation" / "bin") | ||
|
|
||
| # Filter to existing directories not already in PATH | ||
| path_entries = set(current_path.split(os.pathsep)) | ||
| for candidate in candidates: | ||
| if candidate and candidate.exists() and str(candidate) not in path_entries: | ||
| additional_paths.append(str(candidate)) | ||
|
|
||
| # Prepend additional paths (higher priority than existing) | ||
| if additional_paths: | ||
| return os.pathsep.join(additional_paths) + os.pathsep + current_path | ||
|
|
||
| return current_path |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for calls to get_sdk_env_vars and get_enhanced_path
rg -n -C 10 --type=py 'get_sdk_env_vars\(\)|get_enhanced_path\(\)'Repository: AndyMik90/Auto-Claude
Length of output: 8719
🏁 Script executed:
#!/bin/bash
# Let's examine get_sdk_env_vars() more closely and how it's used in calling contexts
# First, get the full implementation of get_sdk_env_vars()
sed -n '304,326p' apps/backend/core/auth.py
echo "---"
# Check the calling contexts - let's see how get_sdk_env_vars() is used
# Start with client.py
sed -n '480,510p' apps/backend/core/client.py
echo "---"
# Check simple_client.py
sed -n '60,90p' apps/backend/core/simple_client.py
echo "---"
# Check the linear updater
sed -n '115,145p' apps/backend/integrations/linear/updater.pyRepository: AndyMik90/Auto-Claude
Length of output: 4208
🏁 Script executed:
#!/bin/bash
# Let's check if there's any try-except handling at higher levels
# Check where create_client() is called
rg -n -B5 -A10 'create_client\(' --type=py | head -100
echo "---"
# Check error handling in the scripts that use simple_client or client functions
rg -n 'try:' apps/backend/core/client.py | head -20
echo "---"
# Check if there's any global error handler
rg -n 'except' apps/backend/core/client.py | head -20Repository: AndyMik90/Auto-Claude
Length of output: 9143
🏁 Script executed:
#!/bin/bash
# Let's look at higher-level error handling in the calling contexts
# Check what happens when create_client is called
rg -n -B20 'client = create_client\(' apps/backend/runners/github/services/triage_engine.py | head -50
echo "---"
# Check the full error handling in one of the calling functions
rg -n -A20 'async with client:' apps/backend/runners/github/services/triage_engine.py
echo "---"
# Now let's verify if Path operations in get_enhanced_path could actually fail
# Check what Python version is being used
head -20 apps/backend/pyproject.toml 2>/dev/null || head -20 setup.py 2>/dev/null || find . -name "*.toml" -o -name "setup.py" | head -5Repository: AndyMik90/Auto-Claude
Length of output: 1811
🏁 Script executed:
#!/bin/bash
# Find and read the Python version requirement
find apps/backend -name "pyproject.toml" -exec cat {} \;
echo "---"
# Let's also verify what specific Path operations could fail
# Check if Path.home() could raise exceptions
python3 <<'EOF'
from pathlib import Path
import sys
# Document what exceptions Path operations can raise
print("Path operations that could raise exceptions:")
print("1. Path.home() - could raise RuntimeError if HOME env var is not set")
print("2. Path.exists() - generally safe, returns False if path doesn't exist")
print("3. Path.iterdir() - raises PermissionError, NotADirectoryError if problems occur")
print("\nPython version:", sys.version)
EOFRepository: AndyMik90/Auto-Claude
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Let's find pyproject.toml with a simpler approach
fd -name "pyproject.toml" -path "*/apps/backend/*" | head -1 | xargs cat | head -30
echo "---"
# Also check what specific exceptions could be raised
# Let's look at the get_enhanced_path function more carefully for potential issues
sed -n '219,301p' apps/backend/core/auth.py | grep -n "\.home()\|\.exists()\|\.iterdir()"Repository: AndyMik90/Auto-Claude
Length of output: 694
🏁 Script executed:
#!/bin/bash
# Let's verify if create_client() calls are wrapped in try-except where get_sdk_env_vars is called
# Check client.py to see if the call to get_sdk_env_vars is protected
sed -n '470,520p' apps/backend/core/client.py
echo "---"
# Check simple_client.py
sed -n '55,85p' apps/backend/core/simple_client.py
echo "---"
# Check where these functions are called - are they wrapped in try-except?
# Look at what happens if an exception is raised during client creation
rg -n -B5 'create_client\(' apps/backend/runners/github/services/triage_engine.pyRepository: AndyMik90/Auto-Claude
Length of output: 3501
Add defensive error handling for Path operations in get_enhanced_path().
The function performs filesystem operations (Path.home(), .exists(), .iterdir()) that can raise exceptions in edge cases—RuntimeError if HOME is unset, and PermissionError/NotADirectoryError if permissions change during iteration. Since get_sdk_env_vars() calls this without error handling and exceptions would propagate during create_client() (which is called before try-except blocks at higher levels), failures would crash client initialization. Wrap directory iteration in try-except blocks, and consider handling Path.home() gracefully with a fallback.
🤖 Prompt for AI Agents
In apps/backend/core/auth.py around lines 219 to 301, add defensive error
handling around Path.home() and all filesystem operations (.exists(),
.iterdir()) so exceptions do not propagate and crash initialization: obtain home
with a safe fallback (use os.environ.get("HOME","") or an empty Path) guarded by
try/except, wrap any .exists()/.iterdir() loops in try/except that catches
RuntimeError, PermissionError, NotADirectoryError (and OSError as a catch‑all),
skip problematic candidate entries on error, and continue building the candidate
list; optionally log warnings when a path is skipped so callers can debug but
ensure the function always returns a valid PATH string (current_path) even if
all enhancements fail.
| """ | ||
| current_path = os.environ.get("PATH", "") | ||
| home = Path.home() | ||
| additional_paths: list[str] = [] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add type hint for candidates list.
The candidates list (created at lines 245, 265) should be type-hinted for clarity. Since it contains Path objects (and currently potentially None on Windows), declare it explicitly:
current_path = os.environ.get("PATH", "")
home = Path.home()
additional_paths: list[str] = []
+ candidates: list[Path] = []If you adopt the refactor from the previous comment to eliminate None values, the type would be clean list[Path]. Otherwise, use list[Path | None].
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| additional_paths: list[str] = [] | |
| additional_paths: list[str] = [] | |
| candidates: list[Path] = [] |
🤖 Prompt for AI Agents
In apps/backend/core/auth.py around lines 234 and specifically where
`candidates` is created at lines 245 and 265, the `candidates` list is missing
an explicit type hint; add a type annotation to the variable declaration — use
`list[Path]` if you have already removed None values via the previous refactor,
otherwise use `list[Path | None]` to reflect possible None entries — place the
annotation where `candidates` is defined so callers and linters see the correct
type.
| if platform.system() == "Windows": | ||
| # Windows common locations | ||
| appdata = os.environ.get("APPDATA", "") | ||
| programfiles = os.environ.get("PROGRAMFILES", "C:\\Program Files") | ||
| programfiles_x86 = os.environ.get( | ||
| "PROGRAMFILES(X86)", "C:\\Program Files (x86)" | ||
| ) | ||
| nvm_home = os.environ.get("NVM_HOME", "") | ||
|
|
||
| candidates = [ | ||
| Path(appdata) / "npm" if appdata else None, | ||
| Path(programfiles) / "nodejs", | ||
| Path(programfiles_x86) / "nodejs", | ||
| home / "AppData" / "Roaming" / "npm", | ||
| home / "AppData" / "Local" / "fnm_multishells", | ||
| home / ".volta" / "bin", | ||
| ] | ||
|
|
||
| # Add nvm-windows paths | ||
| if nvm_home: | ||
| nvm_path = Path(nvm_home) | ||
| if nvm_path.exists(): | ||
| # Find installed node versions | ||
| for version_dir in nvm_path.iterdir(): | ||
| if version_dir.is_dir() and version_dir.name.startswith("v"): | ||
| candidates.append(version_dir) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Refactor to avoid None in candidates list.
The conditional Path(appdata) / "npm" if appdata else None at line 246 introduces None values that must be filtered later. Consider building candidates more cleanly:
🔎 Proposed refactor to avoid None values
- candidates = [
- Path(appdata) / "npm" if appdata else None,
- Path(programfiles) / "nodejs",
- Path(programfiles_x86) / "nodejs",
- home / "AppData" / "Roaming" / "npm",
- home / "AppData" / "Local" / "fnm_multishells",
- home / ".volta" / "bin",
- ]
+ candidates = [
+ Path(programfiles) / "nodejs",
+ Path(programfiles_x86) / "nodejs",
+ home / "AppData" / "Roaming" / "npm",
+ home / "AppData" / "Local" / "fnm_multishells",
+ home / ".volta" / "bin",
+ ]
+ if appdata:
+ candidates.insert(0, Path(appdata) / "npm")This eliminates the need for the if candidate check at line 294.
🤖 Prompt for AI Agents
In apps/backend/core/auth.py around lines 236 to 262, the candidates list
currently includes a None element because of the expression Path(appdata) /
"npm" if appdata else None; change the construction so only actual Path objects
are added (e.g., build the list with conditional appends or a comprehension that
filters out empty strings before wrapping with Path), thereby avoiding None
values and eliminating the need for later `if candidate` checks; keep the
nvm_home block logic the same but ensure any appended paths are valid Path
instances.
| candidates = [ | ||
| Path("/usr/local/bin"), | ||
| Path("/usr/bin"), | ||
| Path("/opt/homebrew/bin"), # Apple Silicon Homebrew | ||
| home / ".local" / "bin", | ||
| home / ".volta" / "bin", | ||
| home / ".fnm" / "current" / "bin", | ||
| home / ".asdf" / "shims", | ||
| home / ".npm-global" / "bin", | ||
| home / "node_modules" / ".bin", | ||
| ] | ||
|
|
||
| # Add nvm paths (find installed versions) | ||
| nvm_dir = home / ".nvm" / "versions" / "node" | ||
| if nvm_dir.exists(): | ||
| for version_dir in nvm_dir.iterdir(): | ||
| if version_dir.is_dir(): | ||
| candidates.append(version_dir / "bin") | ||
|
|
||
| # Add fnm paths | ||
| fnm_dir = home / ".fnm" / "node-versions" | ||
| if fnm_dir.exists(): | ||
| for version_dir in fnm_dir.iterdir(): | ||
| if version_dir.is_dir(): | ||
| candidates.append(version_dir / "installation" / "bin") |
There was a problem hiding this comment.
Inconsistent fnm path references.
Line 271 adds a static candidate home / ".fnm" / "current" / "bin", but lines 285-289 dynamically discover versions under home / ".fnm" / "node-versions". The "current" symlink may not exist in all fnm setups, and the dynamic discovery uses a different path structure ("node-versions" / version / "installation" / "bin").
Verify that both paths are necessary, or consolidate to the dynamic discovery approach. The fnm documentation shows the default alias is typically at ~/.fnm/aliases/default/bin, not ~/.fnm/current/bin.
🔎 Suggested consolidation
candidates = [
Path("/usr/local/bin"),
Path("/usr/bin"),
Path("/opt/homebrew/bin"), # Apple Silicon Homebrew
home / ".local" / "bin",
home / ".volta" / "bin",
- home / ".fnm" / "current" / "bin",
+ home / ".fnm" / "aliases" / "default" / "bin",
home / ".asdf" / "shims",
home / ".npm-global" / "bin",
home / "node_modules" / ".bin",
]🤖 Prompt for AI Agents
In apps/backend/core/auth.py around lines 265 to 289, the fnm path handling is
inconsistent: the code adds a static ~/.fnm/current/bin but also dynamically
discovers ~/.fnm/node-versions/<version>/installation/bin, while fnm typically
stores aliases at ~/.fnm/aliases/default/bin. Remove the hardcoded home / ".fnm"
/ "current" / "bin", and instead consolidate to: (1) keep the dynamic discovery
of home / ".fnm" / "node-versions" / <version> / "installation" / "bin" as-is,
and (2) add an explicit check+append for the alias path home / ".fnm" /
"aliases" / "default" / "bin"; ensure all added candidates are
path-existence-checked before appending to candidates.
| # Filter to existing directories not already in PATH | ||
| path_entries = set(current_path.split(os.pathsep)) | ||
| for candidate in candidates: | ||
| if candidate and candidate.exists() and str(candidate) not in path_entries: | ||
| additional_paths.append(str(candidate)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider case-insensitive path comparison on Windows.
Line 292 uses str(candidate) not in path_entries to avoid duplicates, but Windows paths are case-insensitive. C:\Program Files and c:\program files would both be added.
🔎 Proposed enhancement for Windows
# Filter to existing directories not already in PATH
path_entries = set(current_path.split(os.pathsep))
+ if platform.system() == "Windows":
+ # Normalize to lowercase for case-insensitive comparison on Windows
+ path_entries = {p.lower() for p in path_entries}
+
for candidate in candidates:
- if candidate and candidate.exists() and str(candidate) not in path_entries:
+ candidate_str = str(candidate)
+ if platform.system() == "Windows":
+ candidate_str_check = candidate_str.lower()
+ else:
+ candidate_str_check = candidate_str
+ if candidate and candidate.exists() and candidate_str_check not in path_entries:
- additional_paths.append(str(candidate))
+ additional_paths.append(candidate_str)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/backend/core/auth.py around lines 291-295, the duplicate-path check uses
a case-sensitive string comparison which will allow duplicate paths on Windows
(e.g., "C:\Program Files" vs "c:\program files"); normalize paths for comparison
on Windows by using os.path.normcase (and optionally os.path.normpath) when
building the path_entries set and when checking candidates so you compare
normalized forms, but still append the original candidate string to
additional_paths.
4 participants
Summary
Fixes the "Command 'npx' not found in PATH" error when adding custom MCP servers in the Electron app. Electron apps launched from GUI don't inherit shell PATH modifications, causing npx/npm commands to fail.
Type of Change
Related Issues
Fixes #523
Changes Made
get_enhanced_path()function inauto-claude/core/auth.pyWhy This Change
Problem:
When Auto-Claude is launched from the desktop (double-click, Spotlight, etc.), the Electron process doesn't inherit shell environment modifications from
.bashrc,.zshrc, or Node version managers like:This causes
npxandnpmcommands to fail with "command not found" even though they work perfectly in terminal.Root Cause:
GUI-launched apps on macOS/Linux start with a minimal environment that doesn't include user shell customizations. On Windows, the issue is similar with different installation paths.
How It Works
The
get_enhanced_path()function:Windows
%APPDATA%\npmC:\Program Files\nodejs%NVM_HOME%paths (if set)~\.volta\bin~\.fnm\aliases\default\*macOS/Linux
~/.nvm/versions/node/*/bin(nvm)~/.fnm/aliases/default/bin(fnm)~/.volta/bin(volta)~/.asdf/shims(asdf)/usr/local/bin(homebrew on Intel Mac)/opt/homebrew/bin(homebrew on Apple Silicon)Checklist
Testing
Reproduction Steps (Before Fix):
Verification (After Fix):
Manual Verification:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.