fix: capture shell PATH for CLI spawn on macOS (#4579) #4586
Conversation
Replace shell command-based CLI detection with filesystem-based executable resolution using PATHEXT environment variable.
When the editor is launched from Finder/Spotlight, the extension host doesn't inherit the user's shell environment. This causes the CLI spawn to fail as tools like git may not be in PATH. - Added getLoginShellPath() to capture PATH from login shell - Return CliDiscoveryResult with both cliPath and shellPath - Use captured shell PATH when spawning CLI process - Added telemetry for session timeout diagnostics
# Conflicts: # src/core/kilocode/agent-manager/CliProcessHandler.ts
- Use error.code instead of error.message for EACCES detection - Rename fileExistsAsFile to pathExistsAsFile - Remove redundant isSymbolicLink check (stat follows symlinks) - Add clarifying comment about symlink behavior
Merged the Windows fix from kilocode-win-fix branch which: - Adds findExecutable() for proper Windows PATHEXT handling - Adds case-insensitive environment variable lookup - Adds cli_spawn_error telemetry for Windows debugging Combined with the existing macOS fix for shell PATH capture: - Captures shell PATH from login shell on macOS - Uses captured PATH when spawning CLI process - Adds session_timeout telemetry for debugging Both fixes now work together to resolve: - Windows: CLI spawn ENOENT errors due to PATHEXT handling - macOS: CLI spawn failures when editor launched from Finder
Merges the Windows CLI spawn fix with provisional session handling, while preserving the macOS shell PATH fix: Windows fix (from kilocode-win-fix): - findExecutable() for proper Windows PATHEXT handling - Case-insensitive environment variable lookup - Provisional session handling for early events - cli_spawn_error telemetry macOS fix (preserved): - Captures shell PATH from login shell - Uses captured PATH when spawning CLI process - session_timeout telemetry with stderr preview
🦋 Changeset detectedLatest commit: 43e5190 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
- Skip platform-switching tests when already on target platform - Add dedicated native Windows tests that run only on Windows CI - Add proper lstat mock to fs mocks (code uses both stat and lstat) - Use proper error codes in mock rejections
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 3 Suggestions Found
| Severity | Issue | Location |
|---|---|---|
| SUGGESTION | EACCES error check uses message instead of code property | CliPathResolver.ts:37-38 |
| SUGGESTION | Windows PATHEXT doesn't check base command without extension | CliPathResolver.ts:88-91 |
| SUGGESTION | spawnSync error handling could be more robust | CliPathResolver.ts:218-220 |
Recommendation: Good to merge - these are minor suggestions for robustness
Review Details (7 files)
Files reviewed:
AgentManagerProvider.ts(53 additions, 41 deletions) - Simplified by removing CliSessionLauncher, now directly uses findKilocodeCliCliPathResolver.ts(157 additions, 55 deletions) - Core fix: captures shell PATH and adds Windows PATHEXT supportCliProcessHandler.ts(39 additions, 2 deletions) - Uses shellPath when spawning, enhanced telemetrytelemetry.ts(10 additions) - New diagnostic fields for debuggingAgentManagerProvider.spec.ts(45 additions, 23 deletions) - Test updatesCliPathResolver.spec.ts(272 additions, 24 deletions) - Comprehensive new teststelemetry.test.ts(20 additions) - Test for new telemetry fields
Checked: Security, bugs, error handling, edge cases
Overall Assessment:
This is a well-structured fix for the macOS Finder/Spotlight launch issue (#4579). The approach of capturing the login shell PATH early and passing it to the CLI spawn is sound. The Windows PATHEXT handling is a nice addition that addresses #4584.
Key improvements:
- ✅ Uses
spawnSyncwithstdio: ['ignore', 'pipe', 'pipe']to prevent stdin blocking - ✅ Captures shell PATH before CLI discovery to ensure consistency
- ✅ Proper Windows PATHEXT handling with case-insensitive env var lookup
- ✅ Enhanced telemetry for diagnosing future issues
- ✅ Good test coverage for new functionality
The suggestions above are minor robustness improvements and don't block merging.
Platform simulation (mocking process.platform) is fragile and doesn't truly test platform-specific behavior. Instead: - Windows tests run only on Windows CI (skipped elsewhere) - Non-Windows tests run only on non-Windows (skipped on Windows) - Let actual CI environments test their native platform behavior
There was a problem hiding this comment.
✅ No New Issues Found
7 files reviewed | Confidence: 90% | Recommendation: Merge
Changes since last review (3 commits) are test-related fixes for platform simulation. The core source code is unchanged.
Review Details
Files reviewed:
AgentManagerProvider.ts- Simplified CLI spawning, removed CliSessionLauncherCliPathResolver.ts- Core fix: shell PATH capture + Windows PATHEXT supportCliProcessHandler.ts- Uses shellPath when spawning, enhanced telemetrytelemetry.ts- New diagnostic fields- Test files (3) - Platform-specific test improvements
Previous review suggestions still applicable:
The 3 SUGGESTION-level comments from the previous review remain valid minor improvements:
- EACCES error check could use
e.codeinstead ofe.message - Windows PATHEXT could also check base command without extension
- spawnSync could check
result.errorproperty
These are robustness improvements and don't block merging.
Checked: Security, bugs, error handling, edge cases
There was a problem hiding this comment.
✅ Previous Issue Addressed
Latest commit (665f4bd) properly handles spawnSync errors by checking result.error before accessing stdout.
Remaining suggestions (2):
The following SUGGESTION-level comments from previous review still apply but don't block merging:
- EACCES error check could use
e.codeinstead ofe.message.startsWith() - Windows PATHEXT could also check base command without extension
7 files reviewed | Confidence: 90% | Recommendation: Merge
Review Details
Files reviewed:
CliPathResolver.ts- Core fix: shell PATH capture + Windows PATHEXT supportCliProcessHandler.ts- Uses shellPath when spawning, enhanced telemetryAgentManagerProvider.ts- Simplified CLI spawningtelemetry.ts- New diagnostic fields- Test files (3) - Platform-specific test improvements
Checked: Security, bugs, error handling, edge cases
Resolved merge conflicts in agent-manager files, keeping: - CliDiscoveryResult interface with shellPath for macOS PATH capture - getLoginShellPath() function with spawnSync error handling - hadShellPath telemetry field for tracking fix effectiveness - Windows PATHEXT handling from main branch - Updated test assertions for CliDiscoveryResult return type
Update CliSessionLauncher to handle the new CliDiscoveryResult interface returned by findKilocodeCli, passing shellPath to spawnProcess for macOS shell environment inheritance.
There was a problem hiding this comment.
✅ No New Issues Found
9 files reviewed | Confidence: 92% | Recommendation: Merge
This is a well-structured fix for the macOS Finder/Spotlight launch issue (#4579). The latest commits have addressed the previous review suggestions.
Review Details
Files reviewed:
CliPathResolver.ts- Core fix: shell PATH capture with markers + Windows PATHEXT supportCliProcessHandler.ts- Uses shellPath when spawning, enhanced telemetryAgentManagerProvider.ts- Simplified CLI spawning, removed CliSessionLauncher dependencyCliSessionLauncher.ts- Updated to use CliDiscoveryResult typetelemetry.ts- New diagnostic fields (hasStderr, stderrPreview, hadShellPath)- Test files (3) - Comprehensive coverage for new functionality
.changeset/fix-macos-cli-spawn-path.md- Proper changeset
Key improvements verified:
- ✅
spawnSyncerror handling now checksresult.errorbefore accessing stdout - ✅ EACCES check uses
e.code === "EACCES"(proper Node.js error handling) - ✅ Uses markers (
__KILO_PATH_START__/__KILO_PATH_END__) to reliably extract PATH even with shell banners - ✅
stdio: ['ignore', 'pipe', 'pipe']prevents stdin blocking - ✅ Enhanced telemetry for diagnosing future issues
Checked: Security, bugs, error handling, edge cases, race conditions
Previous suggestions status:
- EACCES error check: ✅ Fixed
- spawnSync error handling: ✅ Fixed
- Windows PATHEXT base command check: Minor edge case, doesn't block merge
There was a problem hiding this comment.
✅ No New Issues Found
9 files reviewed | Confidence: 92% | Recommendation: Merge
Latest commit is a merge from main with no new feature code changes. Previous review suggestions have been addressed.
Review Details
Files reviewed:
CliPathResolver.ts- Core fix: shell PATH capture with markers + Windows PATHEXT supportCliProcessHandler.ts- Uses shellPath when spawning, enhanced telemetryAgentManagerProvider.ts- Simplified CLI spawning, removed CliSessionLauncher dependencyCliSessionLauncher.ts- Updated to use CliDiscoveryResult typetelemetry.ts- New diagnostic fields (hasStderr, stderrPreview, hadShellPath)- Test files (3) - Comprehensive coverage for new functionality
.changeset/fix-macos-cli-spawn-path.md- Proper changeset
Previous issues status:
- ✅ EACCES error check now uses
e.code === "EACCES"(proper Node.js error handling) - ✅ spawnSync error handling checks
result.errorbefore accessing stdout - ⚪ Windows PATHEXT base command check - Minor edge case, doesn't block merge
Checked: Security, bugs, error handling, edge cases
Summary
Problem
When VSCode is launched from Finder/Spotlight on macOS, the extension host process doesn't inherit the user's shell environment. This means the PATH is minimal (
/usr/bin:/bin) and tools likegitthat the CLI depends on aren't accessible, causing the CLI to hang or fail silently.Solution
getLoginShellPath()spawns an interactive login shell to capture the user's actual PATH (including/opt/homebrew/bin, nvm paths, etc.)process.env.PATHwith the captured shell PATHhadShellPath,stderrPreviewfields to help diagnose any remaining issuesRelated