Fix/windows provider cli resolution#1668
Conversation
node-pty is rebuilt for Electron in postinstall via electron-rebuild. Skipping the package install-time node-gyp step avoids Windows toolchains where VS2022 lacks a detected SDK while electron-rebuild still succeeds. Made-with: Cursor
|
@Abdel-E is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis pull request fixes a Windows-specific bug where extensionless npm shims were incorrectly selected over proper executable wrappers during provider command path resolution. Changes modify path resolution logic to prefer executable-like entries with valid extensions and reorder search candidates accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/services/ConnectionsService.ts (1)
393-398: Align Windows executable filtering withPATHEXTto avoid resolver drift.This hardcoded extension list can diverge from the PATHEXT-driven logic already used in
src/main/services/ptyManager.ts, causing inconsistent command selection paths over time.♻️ Suggested refactor
if (process.platform === 'win32') { - const executable = lines.find((line) => /\.(com|exe|bat|cmd|ps1)$/i.test(line)); + const pathExts = new Set( + (process.env.PATHEXT || '.COM;.EXE;.BAT;.CMD') + .split(';') + .map((ext) => ext.trim().toLowerCase()) + .filter(Boolean) + ); + const executable = lines.find((line) => { + const lower = line.toLowerCase(); + return Array.from(pathExts).some((ext) => lower.endsWith(ext)); + }); if (executable) { return executable; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/services/ConnectionsService.ts` around lines 393 - 398, The Windows-only executable filter in ConnectionsService.ts (the process.platform === 'win32' branch that sets the executable variable) uses a hardcoded extension list and should be aligned with the PATHEXT-driven logic used in ptyManager.ts to avoid drift; update this branch to derive allowed extensions from process.env.PATHEXT (split on ';', normalize and build a case-insensitive match) or, better, reuse the same helper/utility used by ptyManager.ts (import the function that parses PATHEXT if available) and then test lines against that dynamic extension set instead of /\.(com|exe|bat|cmd|ps1)$/i.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/services/ConnectionsService.ts`:
- Around line 393-398: The Windows-only executable filter in
ConnectionsService.ts (the process.platform === 'win32' branch that sets the
executable variable) uses a hardcoded extension list and should be aligned with
the PATHEXT-driven logic used in ptyManager.ts to avoid drift; update this
branch to derive allowed extensions from process.env.PATHEXT (split on ';',
normalize and build a case-insensitive match) or, better, reuse the same
helper/utility used by ptyManager.ts (import the function that parses PATHEXT if
available) and then test lines against that dynamic extension set instead of
/\.(com|exe|bat|cmd|ps1)$/i.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5570c75-fac3-4c7e-9f91-230e8b4d066f
📒 Files selected for processing (4)
package.jsonsrc/main/services/ConnectionsService.tssrc/main/services/ptyManager.tssrc/test/main/ConnectionsService.test.ts
1 participant
Summary
On Windows, provider discovery can return multiple lines from
where-style resolution (e.g. an extensionless npm shim pluscodex.cmd). This PR prefers a line that looks like a real executable onwin32, and updatesresolveCommandPathsoPATHEXT-qualified candidates are tried before the bare name—so spawned CLIs resolve to an actual executable.A second commit adjusts pnpm’s native build allowlist so
node-pty’s install-timenode-gypstep is skipped. Nativenode-ptyis still built inpostinstallviaelectron-rebuildfor Electron, which is what the app loads; this avoids redundant/broken Node-targeted builds on some Windows toolchain setups.Fixes
Fixes #1667
Snapshot
N/A — behavior fix (CLI path resolution / install reliability on Windows); no UI change.
Type of change
Mandatory Tasks
Checklist
Summary by CodeRabbit
Bug Fixes
Tests