Skip to content

Conversation

@JKamsker
Copy link
Contributor

@JKamsker JKamsker commented Jan 15, 2026

Summary

This PR fixes provider CLI detection/spawning on Windows (e.g. codex), and makes the dev/build scripts cross-platform so Windows can run the project without relying on Unix shell utilities.

What changed

  • Windows CLI spawning: src/main/services/ConnectionsService.ts now prefers executable extensions from where (e.g. .cmd) and spawns the resolved executable path.
  • Windows .cmd/.bat execution: on win32, when the resolved executable is .cmd/.bat/.ps1, spawning uses shell: true so spawn() doesn’t fail (e.g. EINVAL).
  • Cross-platform npm scripts: replaces rm -rf, mkdir -p, and cp usage with small Node-based scripts.
  • Postinstall native rebuild: postinstall now runs a script that rebuilds required native deps and optionally rebuilds PTY support (skipped when PTY is disabled).

Why

  • On Windows, where codex can return an extensionless shim first (e.g. %APPDATA%\npm\codex). Node’s spawn() cannot execute that shim directly, causing spawn ... ENOENT even though the tool is installed.
  • Spawning .cmd directly without a shell can also produce EINVAL.
  • The repo’s dev/build scripts used Unix commands that don’t work in default Windows shells.

Testing

  • Verified where codex resolves both shim and .cmd, and the code now selects .cmd.
  • Ran npm run type-check.
  • Ran npm run dev on Windows and confirmed providers like codex show as connected.

Notes

  • PTY/native rebuild behavior remains opt-in: PTY rebuild can be skipped via EMDASH_DISABLE_PTY=1 for environments lacking the required Visual Studio Spectre-mitigated libs.

Note

Improves Windows CLI detection/execution and makes tooling cross‑platform, plus better UI surfacing of terminal start failures.

  • Windows: prefer real executable paths from where and spawn with shell for .cmd/.bat/.ps1 in ConnectionsService and PrGenerationService (also windowsHide)
  • ptyManager: cleaner environment initialization; add Windows PATH/PATHEXT/SystemRoot vars and better shell resolution/errors
  • UI: InstallBanner gains start_failed mode with error details (incl. PTY-disabled hint); ChatInterface captures and displays CLI start errors
  • Scripts: replace Unix shell commands with Node scripts (copy-app-config.cjs, clean.cjs); new postinstall.cjs selectively runs electron-rebuild for native deps (e.g., sqlite3, node-pty, keytar); update package.json scripts accordingly

Written by Cursor Bugbot for commit 83f319a. This will update automatically on new commits. Configure here.

@vercel
Copy link

vercel bot commented Jan 15, 2026

@JKamsker is attempting to deploy a commit to the General Action Team on Vercel.

A member of the Team first needs to authorize it.

@JKamsker
Copy link
Contributor Author

@codex review

@arnestrickmann
Copy link
Contributor

@JKamsker - Your PR looks great! I have one suggestion that would make the PTY spawning even more
robust:

Currently on Unix, when spawning provider terminals, we rely on the shell's PATH to find the CLI:
/bin/zsh -lic "codex; exec /bin/zsh -il"

To make this more reliable (especially for users with non-standard shell configs), we could resolve
the CLI path explicitly first. I've tested a small addition that uses the same path resolution logic
from ConnectionsService.ts:

Add this helper function near the top of ptyManager.ts (around line 15):
function resolveCliPath(command: string): string | null {
const { execFileSync } = require('child_process');
const { extname } = require('node:path');
const resolver = process.platform === 'win32' ? 'where' : 'which';

try {
  const result = execFileSync(resolver, [command], { encoding: 'utf8' });
  const lines = result
    .split(/\r?\n/)
    .map((l) => l.trim())
    .filter(Boolean);

  if (process.platform !== 'win32') {
    return lines[0] ?? null;
  }

  // Windows: prefer executable extensions
  const extensionPreference: Record<string, number> = {
    '.exe': 0,
    '.cmd': 1,
    '.bat': 2,
    '.com': 3,
    '.ps1': 50,
    '': 100,
  };

  const best = [...lines].sort((a, b) => {
    const aExt = extname(a).toLowerCase();
    const bExt = extname(b).toLowerCase();
    const aRank = extensionPreference[aExt] ?? extensionPreference[''];
    const bRank = extensionPreference[bExt] ?? extensionPreference[''];
    return aRank - bRank;
  })[0];

  return best ?? null;
} catch {
  return null;
}

}

Then modify the provider command building (around line 182-189):
// Change from:
const cliCommand = provider.cli || baseLower;
const commandString =
cliArgs.length > 0
? ${cliCommand} ${cliArgs .map((arg) => /[\s'"\\$\n\r\t]/.test(arg) ? '${arg.replace(/'/g, "'\\''")}' : arg
)
.join(' ')}`
: cliCommand;

// To:
const cliCommand = provider.cli || baseLower;
const resolvedCliPath = resolveCliPath(cliCommand);
const finalCommand = resolvedCliPath || cliCommand;
const commandString =
cliArgs.length > 0
? ${finalCommand} ${cliArgs .map((arg) => /[\s'"\\$\n\r\t]/.test(arg) ? '${arg.replace(/'/g, "'\\''")}' : arg
)
.join(' ')}`
: finalCommand;

This would make the provider spawning use absolute paths (e.g.,
/Users/x/.nvm/versions/node/v20/bin/codex) instead of just codex, making it independent of shell
PATH configuration while still falling back to the current behavior if resolution fails.

What do you think? Happy to test this addition if you'd like to include it! The rest of your PR
works great on macOS.

@arnestrickmann
Copy link
Contributor

lmk what you think @JKamsker :)

@JKamsker
Copy link
Contributor Author

Hey @arnestrickmann, thanks for the quick reply!
I have updated the PR (and it still works for me) - is that what you had in mind?

JKamsker and others added 6 commits January 18, 2026 13:42
- Prefer executable extensions from where on win32 (e.g. .cmd)

- Spawn the resolved executable path (and use shell for cmd/bat/ps1)

- Replace unix-only npm scripts (rm/cp/mkdir) with node scripts

- Make postinstall rebuild native deps with optional PTY rebuild
- Show real PTY start errors instead of 'isn’t installed'

- Allow suppressing install command for start failures

- Resolve and spawn provider CLIs robustly on Windows
Include PATH/PATHEXT and key Windows env vars in the PTY environment so codex.cmd can locate
ode.
@JKamsker JKamsker force-pushed the fix/windows-provider-cli-spawn branch from 0af9715 to 2596e48 Compare January 18, 2026 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants