fix(env): use fix-path package for dynamic shell PATH resolution #399
Conversation
📝 WalkthroughWalkthroughAdds the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Electron Main
participant FixPath as fix-path (dynamic import)
participant Shell as User Shell
participant Child as Child Process
Note over App: app.whenReady() → await initializeShellPath()
App->>FixPath: import & call fixPath()
FixPath->>Shell: spawn login shell (-ilc) to read PATH
Shell-->>FixPath: returns PATH string
FixPath-->>App: sets process.env.PATH
Note right of App: continue startup
App->>Child: spawn child processes (inherit PATH)
alt fix-path fails or unsupported (Windows)
FixPath-->>App: error / no-op
App->>App: restore prior PATH, log warning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/frontend/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (2)
apps/frontend/package.jsonapps/frontend/src/main/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/index.ts
🔇 Additional comments (2)
apps/frontend/src/main/index.ts (1)
1-5: Hardcoded PATH entries were not removed from env-utils.ts.The PR description states "Replaces hardcoded PATH entries in env-utils.ts," but
env-utils.tsstill contains theCOMMON_BIN_PATHSconstant (line 63) with full platform-specific paths (/opt/homebrew/bin,/usr/local/bin, etc.), andgetAugmentedEnv()still usesCOMMON_BIN_PATHS[platform]at line 100. ThefixPath()call inmain/index.tssupplements the system PATH but does not replace the reliance on these hardcoded entries.Additionally, hardcoded paths persist throughout the codebase in
cli-tool-manager.ts,python-detector.ts,changelog/generator.ts, andchangelog/version-suggester.ts, making the PR description inaccurate about achieving its stated objective.apps/frontend/package.json (1)
80-80: Thefix-pathpackage at version 4.0.0 exists on npm and is not deprecated. No known security vulnerabilities were found. The package is maintained by Sindre Sorhus and is a legitimate dependency.
…oded paths - Add fix-path package for dynamic shell PATH resolution on macOS/Linux - Implement initializeShellPath() with proper try-catch error handling and logging - Call fixPath() at app startup before spawning child processes - Remove redundant hardcoded PATH additions from generator.ts and version-suggester.ts - Update changelog-service.ts comment to clarify fallback behavior - Remove unused 'path' imports from generator.ts and version-suggester.ts Resolves CodeRabbit review comments on PR AndyMik90#399: - Adds error handling to prevent crashes during shell spawning - Adds logging for visibility on PATH resolution success/failure - Cleans up redundant hardcoded PATH entries
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/main/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/index.ts
🔇 Additional comments (1)
apps/frontend/src/main/index.ts (1)
143-145: LGTM: Correct placement of PATH initialization.Awaiting
initializeShellPath()before spawning child processes ensures that all downstream operations (AgentManager, TerminalManager, etc.) inherit the correct PATH. The async/await pattern is clean and the comment clearly explains the intent.Note: This blocks app startup until the shell PATH is resolved. On systems with complex shell configurations (many plugins, slow dotfiles), this could add noticeable delay (potentially 1-2 seconds). This tradeoff is acceptable given the critical need for correct PATH, but consider the performance timing suggestion in the previous comment to help identify slow shells in the wild.
| // Fix PATH for macOS/Linux GUI apps | ||
| // GUI apps launched from Finder/Dock don't inherit shell PATH (.zshrc, .bashrc, etc.) | ||
| // This ensures tools like `gh`, `git`, `node` are available to child processes | ||
| // Note: We use dynamic import with error handling to prevent app crashes | ||
| async function initializeShellPath(): Promise<void> { | ||
| // Only needed on macOS/Linux - Windows GUI apps inherit PATH correctly | ||
| if (process.platform === 'win32') { | ||
| return; | ||
| } | ||
|
|
||
| const originalPath = process.env.PATH; | ||
|
|
||
| try { | ||
| const { default: fixPath } = await import('fix-path'); | ||
| fixPath(); | ||
|
|
||
| if (process.env.PATH !== originalPath) { | ||
| console.warn('[main] Shell PATH initialized from user shell configuration'); | ||
| } | ||
| } catch (error) { | ||
| // Log warning but don't crash - app can still work with limited PATH | ||
| console.warn( | ||
| '[main] Failed to initialize shell PATH:', | ||
| error instanceof Error ? error.message : error | ||
| ); | ||
| console.warn('[main] Some CLI tools may not be found. Falling back to system PATH.'); | ||
| // Restore PATH if it was partially modified | ||
| if (process.env.PATH !== originalPath) { | ||
| process.env.PATH = originalPath; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Error handling looks solid; consider enhancing logging and timing.
The implementation successfully addresses the critical concerns from the previous review by wrapping fixPath() in try-catch and restoring the original PATH on failure. The Windows skip and dynamic import are also appropriate.
However, the previous review suggested additional logging and performance visibility that would help with production debugging:
-
Success logging level: Line 18 uses
console.warn()for a non-warning message. Useconsole.log()orconsole.info()instead. -
PATH value in logs: The previous review recommended logging the new PATH (or a truncated version) on success. This helps verify the fix worked and which directories were added.
-
Performance timing: The previous review suggested measuring call duration and warning if it exceeds a threshold (e.g., 2 seconds), since spawning shells with
-ilccan be slow on systems with complex dotfiles.
🔎 Proposed enhancements
async function initializeShellPath(): Promise<void> {
// Only needed on macOS/Linux - Windows GUI apps inherit PATH correctly
if (process.platform === 'win32') {
return;
}
const originalPath = process.env.PATH;
+ const startTime = Date.now();
try {
const { default: fixPath } = await import('fix-path');
fixPath();
+ const duration = Date.now() - startTime;
if (process.env.PATH !== originalPath) {
- console.warn('[main] Shell PATH initialized from user shell configuration');
+ console.log('[main] Shell PATH initialized from user shell configuration');
+ console.log(`[main] PATH updated in ${duration}ms:`, process.env.PATH?.substring(0, 200) + '...');
+ }
+
+ if (duration > 2000) {
+ console.warn(`[main] Shell PATH initialization took ${duration}ms - consider simplifying shell dotfiles`);
}
} catch (error) {
// Log warning but don't crash - app can still work with limited PATH
console.warn(
'[main] Failed to initialize shell PATH:',
error instanceof Error ? error.message : error
);
console.warn('[main] Some CLI tools may not be found. Falling back to system PATH.');
// Restore PATH if it was partially modified
if (process.env.PATH !== originalPath) {
process.env.PATH = originalPath;
}
}
}📝 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.
| // Fix PATH for macOS/Linux GUI apps | |
| // GUI apps launched from Finder/Dock don't inherit shell PATH (.zshrc, .bashrc, etc.) | |
| // This ensures tools like `gh`, `git`, `node` are available to child processes | |
| // Note: We use dynamic import with error handling to prevent app crashes | |
| async function initializeShellPath(): Promise<void> { | |
| // Only needed on macOS/Linux - Windows GUI apps inherit PATH correctly | |
| if (process.platform === 'win32') { | |
| return; | |
| } | |
| const originalPath = process.env.PATH; | |
| try { | |
| const { default: fixPath } = await import('fix-path'); | |
| fixPath(); | |
| if (process.env.PATH !== originalPath) { | |
| console.warn('[main] Shell PATH initialized from user shell configuration'); | |
| } | |
| } catch (error) { | |
| // Log warning but don't crash - app can still work with limited PATH | |
| console.warn( | |
| '[main] Failed to initialize shell PATH:', | |
| error instanceof Error ? error.message : error | |
| ); | |
| console.warn('[main] Some CLI tools may not be found. Falling back to system PATH.'); | |
| // Restore PATH if it was partially modified | |
| if (process.env.PATH !== originalPath) { | |
| process.env.PATH = originalPath; | |
| } | |
| } | |
| } | |
| // Fix PATH for macOS/Linux GUI apps | |
| // GUI apps launched from Finder/Dock don't inherit shell PATH (.zshrc, .bashrc, etc.) | |
| // This ensures tools like `gh`, `git`, `node` are available to child processes | |
| // Note: We use dynamic import with error handling to prevent app crashes | |
| async function initializeShellPath(): Promise<void> { | |
| // Only needed on macOS/Linux - Windows GUI apps inherit PATH correctly | |
| if (process.platform === 'win32') { | |
| return; | |
| } | |
| const originalPath = process.env.PATH; | |
| const startTime = Date.now(); | |
| try { | |
| const { default: fixPath } = await import('fix-path'); | |
| fixPath(); | |
| const duration = Date.now() - startTime; | |
| if (process.env.PATH !== originalPath) { | |
| console.log('[main] Shell PATH initialized from user shell configuration'); | |
| console.log(`[main] PATH updated in ${duration}ms:`, process.env.PATH?.substring(0, 200) + '...'); | |
| } | |
| if (duration > 2000) { | |
| console.warn(`[main] Shell PATH initialization took ${duration}ms - consider simplifying shell dotfiles`); | |
| } | |
| } catch (error) { | |
| // Log warning but don't crash - app can still work with limited PATH | |
| console.warn( | |
| '[main] Failed to initialize shell PATH:', | |
| error instanceof Error ? error.message : error | |
| ); | |
| console.warn('[main] Some CLI tools may not be found. Falling back to system PATH.'); | |
| // Restore PATH if it was partially modified | |
| if (process.env.PATH !== originalPath) { | |
| process.env.PATH = originalPath; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/frontend/src/main/index.ts around lines 1-32, update the success logging
and add timing/safer PATH output: replace the console.warn used for successful
initialization with console.info (or console.log), record start/end time around
the dynamic import and fixPath call (e.g., Date.now() or performance.now()) and
compute duration, if duration > 2000 ms emit a console.warn including the
duration, and on success log a truncated PATH (limit to ~200 characters or show
leading/trailing segments) with console.info; keep the existing error catch and
PATH-restore behavior unchanged.
MikeeBuilds
added
bug
|
Thank you for your contribution! Before we can accept your PR, you need to sign our Contributor License Agreement (CLA). To sign the CLA, please comment on this PR with exactly: You can read the full CLA here: CLA.md Why do we need a CLA? Auto Claude is licensed under AGPL-3.0. The CLA ensures the project has proper licensing flexibility should we introduce additional licensing options in the future. You retain full copyright ownership of your contributions. I have read the CLA Document and I hereby sign the CLA 0 out of 3 committers have signed the CLA. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/frontend/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (2)
apps/frontend/package.jsonapps/frontend/src/main/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/index.ts
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
apps/frontend/src/main/index.ts (1)
147-150: LGTM! PATH initialization properly sequenced at startup.The async callback and await ensure that
initializeShellPath()completes before any child processes are spawned. The placement before AgentManager, TerminalManager, and window creation is correct and follows the PR objectives.apps/frontend/package.json (1)
81-81: Thefix-pathpackage version is secure and compatible with your ESM-based Electron project. No security vulnerabilities are reported for any version. The version 4.0.0 is stable and matches your project's ESM configuration. Note that version 5.0.0 is available if you prefer to use the latest release.
| if (process.env.PATH !== originalPath) { | ||
| console.warn('[main] Shell PATH initialized from user shell configuration'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add logging for successful execution even when PATH is unchanged.
The success log on line 18 only fires if process.env.PATH differs from originalPath. If fixPath() executes successfully but the PATH is already correct (or was a no-op), there's no confirmation that the function ran at all. This makes debugging harder when troubleshooting PATH-related issues.
Add an unconditional log to confirm execution completed, then log the conditional change message:
🔎 Proposed enhancement
try {
const { default: fixPath } = await import('fix-path');
fixPath();
+ console.log('[main] Shell PATH initialization completed');
if (process.env.PATH !== originalPath) {
console.warn('[main] Shell PATH initialized from user shell configuration');
}
} catch (error) {📝 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.
| if (process.env.PATH !== originalPath) { | |
| console.warn('[main] Shell PATH initialized from user shell configuration'); | |
| } | |
| async function initializeShellPath(): Promise<void> { | |
| // Only needed on macOS/Linux - Windows GUI apps inherit PATH correctly | |
| if (process.platform === 'win32') { | |
| return; | |
| } | |
| const originalPath = process.env.PATH; | |
| try { | |
| const { default: fixPath } = await import('fix-path'); | |
| fixPath(); | |
| console.log('[main] Shell PATH initialization completed'); | |
| if (process.env.PATH !== originalPath) { | |
| console.warn('[main] Shell PATH initialized from user shell configuration'); | |
| } | |
| } catch (error) { | |
| // Log warning but don't crash - app can still work with limited PATH | |
| console.warn( | |
| '[main] Failed to initialize shell PATH:', | |
| error instanceof Error ? error.message : error | |
| ); | |
| console.warn('[main] Some CLI tools may not be found. Falling back to system PATH.'); | |
| // Restore PATH if it was partially modified | |
| if (process.env.PATH !== originalPath) { | |
| process.env.PATH = originalPath; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/frontend/src/main/index.ts around lines 17 to 19, the code only logs
when process.env.PATH differs from originalPath, so there is no confirmation
that fixPath() ran when PATH was unchanged; add an unconditional info/debug log
immediately after calling fixPath() indicating fixPath executed (e.g., "Shell
PATH initialization attempted"), then keep the existing conditional warn message
that logs only when the PATH was changed; ensure the unconditional log runs
regardless of the conditional and uses the same logger/practice used in the
file.
|
|
Replace hardcoded PATH entries with the fix-path package which dynamically reads the user's actual shell PATH from their dotfiles (.zshrc, .bashrc, etc). This fixes issues where CLI tools like `gh`, `node`, or custom-installed binaries are not found when the app is launched from Finder/Dock on macOS/Linux. Benefits: - Works with ANY user shell configuration (asdf, mise, nvm, custom paths) - No maintenance burden for new tool locations - Handles edge cases automatically (custom Homebrew prefix, version managers) - No-op on Windows (not needed - Windows GUI apps inherit PATH correctly) Closes AndyMik90#398
Address CodeRabbit review feedback: - Wrap fixPath() in try-catch to prevent app crashes on shell spawn failures - Add logging for visibility on PATH resolution success/failure - Skip on Windows where PATH inheritance works correctly - Restore original PATH if partially modified during error
tyler-b4innova
force-pushed
the
fix/use-fix-path-for-shell-env
branch
from
03edd08 to
18f5478
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/frontend/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (2)
apps/frontend/package.jsonapps/frontend/src/main/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{ts,tsx}: Always use translation keys withuseTranslation()for all user-facing text in React/TypeScript frontend components - use formatnamespace:section.key(e.g.,navigation:items.githubPRs)
Never use hardcoded strings in JSX/TSX files for user-facing text - always reference translation keys fromapps/frontend/src/shared/i18n/locales/
Files:
apps/frontend/src/main/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/index.ts
🔇 Additional comments (2)
apps/frontend/src/main/index.ts (2)
1-32: Core implementation is solid; past logging suggestions remain optional improvements.The implementation correctly addresses the PR objectives:
- ✅ Dynamic import of fix-path with error handling
- ✅ Platform check (no-op on Windows)
- ✅ PATH restoration on failure
- ✅ Non-fatal error handling (app continues with fallback PATH)
The past review comments suggesting logging improvements (performance timing, PATH value logging, console.log vs console.warn) remain valid optional enhancements but are not blocking issues. The critical error handling concern has been addressed.
147-149: Correct placement for PATH initialization.Calling
await initializeShellPath()at the start ofapp.whenReady()ensures the shell PATH is resolved before any child processes (agent manager, terminal manager, Python environment) are spawned. This aligns with the PR objectives.
| "clsx": "^2.1.1", | ||
| "electron-log": "^5.4.3", | ||
| "electron-updater": "^6.6.2", | ||
| "fix-path": "^4.0.0", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check fix-path package information
echo "=== Latest version info ==="
curl -s https://registry.npmjs.org/fix-path | jq -r '.dist-tags'
echo -e "\n=== Version 4.0.0 availability ==="
curl -s https://registry.npmjs.org/fix-path/4.0.0 | jq '{version: .version, published: .time, deprecated: .deprecated}'
echo -e "\n=== All versions (last 15) ==="
curl -s https://registry.npmjs.org/fix-path | jq -r '.versions | keys[]' | tail -15
echo -e "\n=== Package metadata ==="
curl -s https://registry.npmjs.org/fix-path/latest | jq '{name: .name, version: .version, deprecated: .deprecated, repository: .repository}'Repository: AndyMik90/Auto-Claude
Length of output: 580
Consider using fix-path@^5.0.0 instead of ^4.0.0.
Version 4.0.0 exists and is valid, but version 5.0.0 is the latest available release from this actively maintained package (sindresorhus/fix-path). Unless there's a specific compatibility requirement for version 4.x, updating to the latest major version is recommended.
🤖 Prompt for AI Agents
In apps/frontend/package.json around line 82 the dependency "fix-path" is pinned
to ^4.0.0; update it to ^5.0.0 to use the latest major release. Edit
package.json to change the version string for fix-path from ^4.0.0 to ^5.0.0,
then run your package manager (npm/yarn/pnpm) to install and test for any
compatibility issues, and update lockfile accordingly.
5 participants
Summary
Replace hardcoded PATH entries in
env-utils.tswith thefix-pathpackage which dynamically reads the user's actual shell PATH from their dotfiles.Changes:
fix-path@^4.0.0dependencyfixPath()at app startup inmain/index.tsbefore any child processes are spawnedProblem
The current approach uses hardcoded paths:
This breaks for users with:
asdf,mise,rtx,nvm)Solution
fix-pathspawns the user's shell with-ilcflags to source their dotfiles (.zshrc,.bashrc, etc.) and captures the actual$PATH. This works with ANY user shell configuration.Platform behavior
Test plan
ghCLI is detected when installed via HomebrewCloses #398
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.