fix: resolve Windows CLI spawn ENOENT error in Agent Manager - #4584
Conversation
Replace shell command-based CLI detection with filesystem-based executable resolution using PATHEXT environment variable.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 1 Issue Found
| Severity | Issue | Location |
|---|---|---|
| WARNING | EACCES error detection uses incorrect check | CliPathResolver.ts:25 |
Recommendation: Address the warning before merge
Review Details (5 files)
Files: CliPathResolver.ts (1 issue), CliProcessHandler.ts, AgentManagerProvider.spec.ts, CliPathResolver.spec.ts, telemetry.ts
Checked: Security, bugs, error handling, Windows compatibility
Summary: The PR correctly addresses the Windows CLI spawn ENOENT error by implementing filesystem-based executable resolution with PATHEXT support. The new findExecutable function properly handles Windows-specific path resolution. One minor issue found in the EACCES error detection logic.
# 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
There was a problem hiding this comment.
✅ Previous Issues Addressed
The EACCES error detection issue from my previous review has been resolved:
- Now correctly uses
e.code === "EACCES"instead ofe.message.startsWith("EACCES") - Function renamed to
pathExistsAsFileas suggested by reviewer
No new issues found in the latest changes.
Review Details (6 files)
Files: CliPathResolver.ts (fixes applied), CliProcessHandler.ts, AgentManagerProvider.spec.ts, CliPathResolver.spec.ts, telemetry.ts, slackbot.md
Checked: Security, bugs, error handling, Windows compatibility, PATHEXT resolution
Summary: The PR correctly implements filesystem-based executable resolution with proper PATHEXT support on Windows. The findExecutable function handles case-insensitive environment variable lookup and systematically tries Windows extensions. Telemetry additions provide good debugging context for spawn errors.
- 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
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
Changes since last review look good.
The latest commits (285c427d, 268a5c86, d0160a57) improve test reliability by:
- Removing fragile platform simulation tests
- Using native platform tests that run only on their target CI environment
- Using platform-appropriate paths in Windows tests
Review Details (5 files)
Files: CliPathResolver.ts, CliProcessHandler.ts, AgentManagerProvider.spec.ts, CliPathResolver.spec.ts, telemetry.ts
Checked: Security, bugs, error handling, Windows compatibility, PATHEXT resolution
Summary: The PR correctly implements filesystem-based executable resolution with proper PATHEXT support on Windows. Previous review issues (EACCES error detection, function naming) have been addressed. Test improvements ensure reliable cross-platform testing.
The Windows .cmd shell:true behavior is already tested in CliProcessHandler. The PATHEXT resolution is tested in CliPathResolver.spec.ts. Production code works on Windows (confirmed), so remove complex integration test.
There was a problem hiding this comment.
✅ No New Issues
5 files reviewed | Confidence: 95% | Recommendation: Merge
The latest commit (364feee) simplifies tests by removing a fragile Windows integration test. This is a test-only change that improves reliability without affecting production code.
Review Details
Files: CliPathResolver.ts, CliProcessHandler.ts, AgentManagerProvider.spec.ts, CliPathResolver.spec.ts, telemetry.ts
Checked: Security, bugs, error handling, Windows compatibility, PATHEXT resolution
Summary:
- The
findExecutablefunction correctly implements filesystem-based executable resolution with PATHEXT support on Windows - Case-insensitive environment variable lookup handles Windows env var casing inconsistency
- EACCES error detection properly uses
e.code === "EACCES"(fixed from previous review) - Telemetry additions provide good debugging context for spawn errors
- Tests are appropriately platform-conditional (Windows tests run only on Windows CI)
Previous Issues: All resolved (EACCES error detection, function naming)
Unix-style paths like /usr/bin/kilocode are not absolute on Windows (Windows requires drive letters like C:\). Skip these tests on Windows since the Windows-specific behavior is already tested by the PATHEXT tests.
On Windows, login shell is skipped and findExecutable uses fs.promises.stat instead of execSync. The tests were relying on execSync returning MOCK_CLI_PATH, which doesn't work on Windows. Fix: Mock getLocalCliPath() to return MOCK_CLI_PATH and make fileExistsAtPath return true for that path. This ensures findKilocodeCli finds the CLI via the local path check on all platforms.
There was a problem hiding this comment.
✅ No New Issues
5 files reviewed | Confidence: 95% | Recommendation: Merge
The latest commit (6130fad) improves test reliability by mocking getLocalCliPath() to return a consistent path across platforms. This ensures findKilocodeCli finds the CLI via the local path check on all platforms, avoiding platform-specific behavior in tests.
Review Details
Files: CliPathResolver.ts, CliProcessHandler.ts, AgentManagerProvider.spec.ts, CliPathResolver.spec.ts, telemetry.ts
Checked: Security, bugs, error handling, Windows compatibility, PATHEXT resolution
Summary:
findExecutablecorrectly implements filesystem-based executable resolution with PATHEXT support on Windows- Case-insensitive environment variable lookup handles Windows env var casing inconsistency
- EACCES error detection properly uses
e.code === "EACCES"(fixed from earlier review) - Telemetry additions provide good debugging context for spawn errors
- Tests are appropriately platform-conditional and use consistent mocking
Previous Issues: All resolved
2 participants
Issue
Agent Manager fails to spawn the CLI on Windows with error:
spawn C:\Users\user\AppData\Roaming\npm\kilocode ENOENT
This occurs even though the CLI is correctly installed and
where kilocodefinds it.Root Cause
On Windows, npm creates two files for global CLI tools:
kilocode- a shell script stub (for Git Bash compatibility)kilocode.cmd- the actual Windows batch fileThe previous implementation used
where kilocodewhich returns both paths, but takes thefirst result (the extensionless stub). Node.js
spawn()cannot execute this stub directlywithout
shell: true, causing ENOENT.Solution
Replace shell command-based CLI detection with filesystem-based executable resolution:
PATHEXTenvironment variable (
.COM,.EXE,.BAT,.CMD) until finding a file that exists(
PATHvsPathvspath)This ensures the
.cmdfile is found and used, which then correctly triggersshell: truein the spawn options.
Telemetry
Added spawn error context to help debug future issues:
errorMessage- the actual errorcliPath- the path that was attemptedcliPathExtension- file extension (helps identify stub vs .cmd issues)