Skip to content

Windows port foundations: MCP IPC, CLI resolution, portable tooling#160

Closed
arul28 wants to merge 2 commits into
mainfrom
cursor/windows-port-foundations-ede6
Closed

Windows port foundations: MCP IPC, CLI resolution, portable tooling#160
arul28 wants to merge 2 commits into
mainfrom
cursor/windows-port-foundations-ede6

Conversation

@arul28
Copy link
Copy Markdown
Owner

@arul28 arul28 commented Apr 17, 2026

Summary

Windows port foundations plus follow-ups, updated to address all 12 inline review comments from Greptile and CodeRabbit on this PR.

MCP / IPC

  • Windows: named pipe path via resolveAdeMcpIpcPath; Unix: .ade/mcp.sock.
  • Headless MCP: connect-first with 2s timeout, targeted listener cleanup, placeholder error handler on connected socket (apps/mcp-server/src/index.ts).
  • adeMcpProxy --probe: socketExists uses a short connect probe for named pipes (not always true).

CLI / usage / sandbox

  • cliExecutableResolver: Windows PATH augmentation and known dirs; test fixture PATH uses realistic escaping.
  • usageTrackingService Codex RPC: total stdout/stderr caps; on Windows cmd /d/s/c when spawning .cmd/.bat or bare codex.
  • orchestratorConstants: format regex fixed; reg.exe / format.exe / diskpart.exe / bcdedit.exe / takeown.exe variants; removed wholesale ProgramFiles trust (only System32 / SysWOW64).
  • resolveWorkerShellInvocation: /d /s /c on Windows.

OpenCode on Windows

  • Process list: WMIC first, then Get-CimInstance Win32_Process | ConvertTo-Csv (shared CSV parser).
  • Orphan recovery: Windows launches cmd.exe /d/s/c with set "ADE_OPENCODE_*" before opencode … serve so markers appear in CommandLine; isManagedOpenCodeServeCommand handles cmd-wrapped lines; parseManagedOwnerPid tolerates set quoting.
  • __isManagedOpenCodeServeCommandForTests for unit coverage.

Tests

  • adeMcpIpc.test.ts, cliExecutableResolver.test.ts, universalTools.test.ts, openCodeServerManager.test.ts updated accordingly.

If new bot comments appear after this push, paste them or re-run review on the latest commit.

Open in Web Open in Cursor 

Summary by CodeRabbit

Release Notes

  • New Features

    • Added Windows packaging support with native installer configuration
    • Added Windows named-pipe IPC communication for improved reliability
  • Bug Fixes

    • Fixed file URL path formatting and resolution
    • Improved process cleanup and termination across all platforms
    • Enhanced Windows executable and PATH resolution
  • Improvements

    • Better cross-platform shell command execution
    • Platform-aware keyboard shortcut display
    • Improved Windows path handling and normalization

Greptile Summary

This PR adds Windows port foundations across MCP IPC (named pipes), CLI resolution, process-tree killing, and OpenCode orphan recovery. Most prior review concerns have been resolved: stdout accumulation is now correctly capped, isSystemExecutableSandboxPath is narrowed to System32/SysWOW64 only, the format regex trailing \b was dropped, WMIC unavailability now falls back to PowerShell, and orphan recovery uses taskkill /T /F.

  • P1 (universalTools.ts:627): resolveWorkerShellInvocation passes the raw AI command string directly to cmd.exe /d /s /c. When the command starts with \" (e.g. a quoted executable path), cmd.exe's /s flag strips the outer \"…\" pair, corrupting the command token. The same outer-wrap fix needed for resolveWindowsCmdInvocation applies here.

Confidence Score: 4/5

Safe to merge after addressing the cmd.exe /s /c quote-stripping bug in resolveWorkerShellInvocation.

The PR resolves the majority of prior P1/P2 findings. One new P1 remains in resolveWorkerShellInvocation (universalTools.ts) — the same cmd.exe /s /c outer-quote issue as flagged for resolveWindowsCmdInvocation, but in a new function not covered by the prior thread.

apps/desktop/src/main/services/ai/tools/universalTools.ts (resolveWorkerShellInvocation), apps/desktop/src/main/services/shared/processExecution.ts (resolveWindowsCmdInvocation — prior thread fix still outstanding)

Important Files Changed

Filename Overview
apps/desktop/src/main/services/ai/tools/universalTools.ts New resolveWorkerShellInvocation for Windows bash-tool execution has the same cmd.exe /s /c quote-stripping bug; isSystemExecutableSandboxPath correctly narrowed to System32/SysWOW64 only (addressing prior P2).
apps/desktop/src/main/services/shared/processExecution.ts New Windows process-execution helpers: quoteWindowsCmdArg, resolveWindowsCmdInvocation, terminateProcessTree. The /s /c quoting issue flagged in prior review (adding outer "..." around cmdLine) remains unaddressed in this file.
apps/desktop/src/main/services/opencode/openCodeServerManager.ts Windows orphan recovery now uses killProcessTree (taskkill /T /F), WMIC/PowerShell process listing with CSV fallback, and cmd.exe set marker injection into CommandLine. The cmdLine for the launch spec starts with set, not ", so /s quote-stripping is not triggered here.
apps/desktop/src/main/services/usage/usageTrackingService.ts Output accumulation is now correctly capped via maxStdout - stdout.length differential slicing, addressing the prior P1 comment. Windows cmd /d/s/c wrapper for .cmd/.bat codex launchers added correctly.
apps/desktop/src/main/services/orchestrator/orchestratorConstants.ts Trailing \b after drive-letter format pattern removed (fixing the never-matching regex); Windows executables added to safeCommands/blockedPatterns with .exe/.cmd optional suffixes.
apps/desktop/src/shared/adeMcpIpc.ts Windows named-pipe path via SHA-256 hash of lowercased resolved root; Unix .ade/ade.sock. Tests confirm stable casing-normalized pipe names.
apps/desktop/src/main/services/ai/cliExecutableResolver.ts Windows PATH augmentation with getWindowsKnownBinDirs, case-insensitive PATH key lookup (getPathEnvKey), and PATHEXT-aware extension resolution all look correct.
apps/desktop/src/renderer/lib/shell.ts Platform-aware quoting and parsing added; navigator.platform used for browser-side Windows detection is deprecated and may silently fail to detect Windows on modern engines.
apps/desktop/src/renderer/lib/pathUtils.ts New path utility functions for cross-platform normalization, Windows drive/UNC detection, and rename remapping. Logic is correct and well-tested.

Comments Outside Diff (2)

  1. apps/desktop/src/main/services/opencode/openCodeServerManager.ts, line 827-841 (link)

    P2 Silent failure when WMIC is unavailable

    listWindowsProcessesFromWmic returns [] on any error including wmic not being found (deprecated/removed on Windows 11 22H2+ without the optional feature). The caller then sees an empty process list, meaning orphan OpenCode processes are never detected or killed. A PowerShell Get-CimInstance fallback — or at least a logged warning — would prevent silent orphan leakage on affected systems.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/desktop/src/main/services/opencode/openCodeServerManager.ts
    Line: 827-841
    
    Comment:
    **Silent failure when WMIC is unavailable**
    
    `listWindowsProcessesFromWmic` returns `[]` on any error including `wmic` not being found (deprecated/removed on Windows 11 22H2+ without the optional feature). The caller then sees an empty process list, meaning orphan OpenCode processes are never detected or killed. A PowerShell `Get-CimInstance` fallback — or at least a logged warning — would prevent silent orphan leakage on affected systems.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Claude Code

  2. apps/desktop/src/main/services/opencode/openCodeServerManager.ts, line 577-581 (link)

    P1 Orphan recovery kills only cmd.exe wrapper, not the opencode process tree

    On Windows, openCodeProcessController.killProcess calls process.kill(pid, signal), which terminates only the wrapper cmd.exe (PID=X). Its children — another cmd.exe running opencode.cmd, and the actual node.exe/opencode.exe server beneath it — are left running as invisible orphans, defeating the purpose of orphan recovery.

    By contrast, stopChildProcess (used for directly managed children) correctly calls taskkill /pid X /T /F to kill the whole tree. The orphan recovery path needs the same treatment.

    Evidence: killProcess at line 299–305 uses process.kill(pid, signal) with no tree-kill; stopChildProcess at lines 389–393 uses taskkill /T /F. isManagedOpenCodeServeCommand only matches the cmd.exe wrapper (via the managed-env markers), so the actual opencode subprocess is never found in subsequent recovery runs, leaving it running indefinitely and causing EADDRINUSE collisions on the same port.

    Suggested fix: add a Windows-specific taskkill call in the orphan kill path (before or instead of process.kill), mirroring stopChildProcess:

    openCodeProcessController.killProcess(proc.pid, "SIGTERM");
    // Windows: also kill the entire process subtree so opencode.exe / node.exe beneath cmd.exe is also terminated
    if (process.platform === "win32" && proc.pid) {
      spawnSync("taskkill", ["/pid", String(proc.pid), "/T", "/F"], { windowsHide: true });
    }
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/desktop/src/main/services/opencode/openCodeServerManager.ts
    Line: 577-581
    
    Comment:
    **Orphan recovery kills only cmd.exe wrapper, not the opencode process tree**
    
    On Windows, `openCodeProcessController.killProcess` calls `process.kill(pid, signal)`, which terminates only the wrapper `cmd.exe` (PID=X). Its children — another `cmd.exe` running `opencode.cmd`, and the actual `node.exe`/`opencode.exe` server beneath it — are left running as invisible orphans, defeating the purpose of orphan recovery.
    
    By contrast, `stopChildProcess` (used for directly managed children) correctly calls `taskkill /pid X /T /F` to kill the whole tree. The orphan recovery path needs the same treatment.
    
    Evidence: `killProcess` at line 299–305 uses `process.kill(pid, signal)` with no tree-kill; `stopChildProcess` at lines 389–393 uses `taskkill /T /F`. `isManagedOpenCodeServeCommand` only matches the cmd.exe wrapper (via the managed-env markers), so the actual opencode subprocess is never found in subsequent recovery runs, leaving it running indefinitely and causing EADDRINUSE collisions on the same port.
    
    Suggested fix: add a Windows-specific `taskkill` call in the orphan kill path (before or instead of `process.kill`), mirroring `stopChildProcess`:
    
    ```typescript
    openCodeProcessController.killProcess(proc.pid, "SIGTERM");
    // Windows: also kill the entire process subtree so opencode.exe / node.exe beneath cmd.exe is also terminated
    if (process.platform === "win32" && proc.pid) {
      spawnSync("taskkill", ["/pid", String(proc.pid), "/T", "/F"], { windowsHide: true });
    }
    ```
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Claude Code

Fix All in Claude Code

Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/ai/tools/universalTools.ts
Line: 627-633

Comment:
**`/s /c` quote-stripping in `resolveWorkerShellInvocation`**

When `command` starts with `"` — e.g. `"C:\Program Files\script.bat" --arg` — cmd.exe's `/s` flag strips the first and last `"` from the entire argument, producing `C:\Program Files\script.bat" --arg`. The executable token becomes `C:\Program Files\script.bat"` (with a dangling `"`), causing the spawn to fail.

The same class of fix used for `resolveWindowsCmdInvocation` (wrapping the inner cmdLine in an additional outer pair of `"..."` that `/s` consumes) is needed here.

```suggestion
export function resolveWorkerShellInvocation(command: string): { file: string; args: string[] } {
  if (process.platform === "win32") {
    const comSpec = process.env.ComSpec?.trim() || "cmd.exe";
    return { file: comSpec, args: ["/d", "/s", "/c", `"${command}"`] };
  }
  return { file: "bash", args: ["-c", command] };
}
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/shell.ts
Line: 5-8

Comment:
**`navigator.platform` is deprecated**

`navigator.platform` is deprecated and may return an empty string in some browser environments, causing the Windows detection to silently fall through to `process.platform`. In Electron renderer, `process.platform` is always available and correct, making the `navigator.platform` check redundant and a potential source of false negatives on modern runtimes.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (7): Last reviewed commit: "Add /shipLane command and portable ship-..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Loading
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.

1 participant