Skip to content

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

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

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

Conversation

@arul28
Copy link
Copy Markdown
Owner

@arul28 arul28 commented Apr 23, 2026

Summary

Foundational Windows port for the ADE desktop app and ADE CLI. No feature work; this lane enables the app to build, ship, and run on Windows with parity for the core agent surfaces (chat, files, MCP, orchestrator, PTY-like workflows).

Scope

  • MCP IPC portability — named-pipe path on Windows, Unix socket on macOS/Linux (adeMcpIpc.ts, preload, registerIpc).
  • ADE CLI resolution on Windows — PATH search, .cmd/.exe probing, wrapper scripts (ade-cli-windows-wrapper.cmd, ade-cli-install-path.cmd), install-path validator.
  • Renderer path helpers — Windows drive/UNC/absolute-path predicates in pathUtils.ts, shell detection in shell.ts.
  • Packagingafter-pack-runtime-fixes.cjs runtime fixes, validate-win-artifacts.mjs artifact validator, cr-sqlite win32-x64 vendor binary, release-core.yml workflow tweaks, .ico icon.
  • Tools & orchestrator — Windows-aware safe/unsafe command list in universalTools.ts, providerTaskRunner + providerOrchestratorAdapter + orchestratorService worker-launch plumbing, taskkill /T /F process-tree termination in processExecution.ts.
  • RendererAgentChatMessageList, FilesPage, dirtyWorkspaceBuffers portability fixes.

Tests added

  • providerTaskRunner.test.ts, windowsPackaging.test.ts, new tests in processExecution.test.ts (killWindowsProcessTree, terminateProcessTree), pathUtils.test.ts Windows predicates, shell.test.ts, dirtyWorkspaceBuffers.test.ts.
  • Total: ~46 new/extended tests across colocated files. All 8 vitest shards + ade-cli tests green locally.

Tangentially included (unblocked ade prs create for this lane)

  • Drop trailing slash on node_modules gitignore (worktree symlinks were showing up as untracked).
  • Fallback to gh auth token in ADE CLI when ADE_GITHUB_TOKEN / GITHUB_TOKEN env vars are missing.
  • Minor /finalize playbook addition (Phase 3j — stale worker-process cleanup).

Test plan

  • macOS: desktop app boots, MCP IPC reconnects across lane switches, PTY works.
  • Windows: desktop app installs from release artifact, ADE CLI resolves on PATH, MCP named pipe connects, PTY/shell workflows work.
  • CI: 8 desktop shards + ade-cli + typecheck + lint + build all green.

Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added Windows desktop app support with NSIS installer and bundled ADE CLI wrapper.
    • Added "default" system file browser option for opening files in external editors.
    • Added computer-use screenshot capability checks with platform-specific commands.
  • Bug Fixes

    • Improved path handling for Windows backslashes and case-insensitive comparisons.
    • Fixed shell command execution on Windows via proper cmd.exe wrapping.
    • Fixed process tree termination and timeout handling across platforms.
  • Improvements

    • Enhanced platform-aware UI labels (Cmd/Ctrl based on OS).
    • Improved CLI executable resolution and PATH handling.
    • Strengthened sandbox enforcement with Windows command patterns.

Greptile Summary

This is a large Windows port laying the groundwork for building and running ADE on Windows: named-pipe MCP IPC, Windows-aware CLI resolution and PATH augmentation, stdin-based prompt delivery to avoid shell quoting issues, a node-script launcher for orchestrator workers replacing PTY shell startup, PowerShell/cmd.exe sandbox enforcement, and a full release validation pipeline.

  • P1 — worker launcher crashes on every invocation: WORKER_CLI_LAUNCHER_SCRIPT reads process.argv[1] for the launch-file path, but Node.js (and Electron with ELECTRON_RUN_AS_NODE) sets process.argv[1] to "" when using the -e flag; the first positional argument is at process.argv[2]. fs.readFileSync("", "utf8") throws ENOENT and every orchestrator worker exits with code 1.
  • P1 — PowerShell sandbox tokenizer mishandles '' escape: tokenizePowerShellCommand treats the first ' in a '' pair as the closing delimiter for a single-quoted string, silently splitting a path like 'C:\work\user''s dir\file' into two tokens. An agent-submitted path traversal sequence (e.g., 'C:\workspace\allowed''/../outside.txt') can pass the workspace-root check while PowerShell resolves the full, out-of-sandbox path.

Confidence Score: 3/5

Two P1 defects — a worker launcher that crashes on every invocation and a sandbox tokenizer path-traversal — need resolution before merging.

The process.argv[1] bug would make all orchestrator workers fail silently with exit code 1. The PowerShell tokenizer '' handling is a security boundary bypass in the sandbox enforcement introduced in this PR. Both are present in new code and would affect real users.

providerOrchestratorAdapter.ts (argv index bug) and universalTools.ts (PowerShell tokenizer '' escape) require fixes before merge.

Security Review

  • PowerShell sandbox path-traversal via tokenizer bug (universalTools.ts): The tokenizePowerShellCommand function does not implement PowerShell's ''-escape rule for single-quoted strings. A crafted path argument containing '' can cause the sandbox path extractor to check only the prefix of the path (which may be inside the workspace), while PowerShell operates on the full path (which may be outside). This allows an agent-supplied PowerShell command to bypass workspace-boundary enforcement.
  • No credential leakage, injection, or auth/authz issues identified in the remaining changes.

Important Files Changed

Filename Overview
apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts Major refactor: replaces shell-string worker launch with direct node -e SCRIPT launchFile invocation via nodeWorkerLaunch. Contains a likely bug where WORKER_CLI_LAUNCHER_SCRIPT reads process.argv[1] for the launch file path, but Node's -e mode places the first positional argument at process.argv[2].
apps/desktop/src/main/services/ai/tools/universalTools.ts Extensive new Windows sandbox enforcement: PowerShell command tokenizer and inspector, MUTATING_CMD_RE for cmd.exe. The PowerShell tokenizer does not handle '' as an escaped single-quote within single-quoted strings, which can cause path-argument extraction to fail for paths containing apostrophes, potentially weakening sandbox path checks.
apps/desktop/src/main/services/shared/processExecution.ts New file providing Windows-portable process management: quoteWindowsCmdArg, resolveCliSpawnInvocation, killWindowsProcessTree, and terminateProcessTree. Well-structured with good error handling.
apps/desktop/src/main/services/ai/cliExecutableResolver.ts Extended with getPathEnvKey/getPathEnvValue/setPathEnvValue for case-insensitive Windows PATH lookup, plus getWindowsKnownBinDirs covering Scoop, Winget, Volta, pnpm, npm, and common install paths.
apps/desktop/src/main/services/ipc/registerIpc.ts New resolveRendererSuppliedPath helper centralizes ade-artifact:// and file:// URI-to-path resolution across IPC handlers; adds "default" editor target using shell.openPath; adds Windows start "" cmd.exe editor launch.
apps/desktop/src/main/services/ai/providerTaskRunner.ts Refactored to deliver prompts via stdin rather than a positional shell argument, using resolveCliSpawnInvocation for cross-platform spawn and terminateProcessTree on timeout.
apps/desktop/src/renderer/lib/shell.ts Extended with Windows-aware argument quoting (quoteWindowsArg) and parseWindowsCommandLine implementing CommandLineToArgvW rules. Uses deprecated navigator.platform.
apps/desktop/src/renderer/lib/pathUtils.ts New renderer-side path utilities for Windows drive/UNC path detection, normalization, and case-insensitive comparison. Comprehensive coverage of edge cases with corresponding test file.
apps/desktop/src/shared/adeMcpIpc.ts New file: resolves MCP IPC socket path as a Windows named pipe (\\.\pipe\ade-<sha256[:24]>) or POSIX Unix socket. Correct canonical path normalization before hashing.
apps/desktop/scripts/validate-win-artifacts.mjs New CI validation script checking package.json configuration, NSIS installer artifact integrity, and latest.yml sha512 metadata. Thorough preflight and release validation stages.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[buildStartupCommand] --> B[writeWorkerPromptFile\n.ade/worker-prompts/worker-ID.txt]
    A --> C[writeWorkerLaunchFile\n.ade/worker-prompts/worker-ID.launch.json]
    C --> D[resolveCliSpawnInvocation\nwraps .cmd/.bat in cmd.exe on Windows]
    A --> E[nodeWorkerLaunch]
    E --> F["spawn(process.execPath,\n  ['-e', WORKER_CLI_LAUNCHER_SCRIPT, launchFilePath],\n  {ELECTRON_RUN_AS_NODE: '1'})"]
    F -->|"reads process.argv[1]\n⚠ should be argv[2]"| G[fs.readFileSync launch.json]
    G --> H[spawn worker CLI\ne.g. claude/codex with args+env]
    H --> I[stdin piped from promptFilePath]
    H --> J[child.on exit → finish code]
    K[terminateProcessTree] --> L{win32?}
    L -->|yes| M[taskkill /T /F /PID]
    L -->|no| N[child.kill SIGTERM]
    M -->|fails| N
    style F fill:#ffd,stroke:#f90
    style G fill:#fdd,stroke:#f00
Loading

Comments Outside Diff (1)

  1. apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts, line 348-413 (link)

    P1 process.argv[1] is empty when Node runs with -e

    The WORKER_CLI_LAUNCHER_SCRIPT is launched as:

    node -e SCRIPT launchFilePath
    

    When Node.js (or Electron in ELECTRON_RUN_AS_NODE mode) runs with -e, process.argv[1] is set to the empty string "" — the first positional argument lands at process.argv[2]. The script does:

    const specPath = process.argv[1]; // ← always "" when launched with -e

    fs.readFileSync("", "utf8") will throw ENOENT, the finish(1) handler fires, and every orchestrator worker launch fails with exit code 1 rather than the real exit code. This makes the worker silently fail without a meaningful error message.

    Change the script to read process.argv[2]:

    const specPath = process.argv[2];

    The same fix applies to the failure-path inline script on the last return branch of buildStartupCommand:

    args: ["-e", "console.error(process.argv[2]); process.exit(64);", failureMessage],
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts
    Line: 348-413
    
    Comment:
    **`process.argv[1]` is empty when Node runs with `-e`**
    
    The `WORKER_CLI_LAUNCHER_SCRIPT` is launched as:
    ```
    node -e SCRIPT launchFilePath
    ```
    When Node.js (or Electron in `ELECTRON_RUN_AS_NODE` mode) runs with `-e`, `process.argv[1]` is set to the empty string `""` — the first positional argument lands at `process.argv[2]`. The script does:
    
    ```js
    const specPath = process.argv[1]; // ← always "" when launched with -e
    ```
    
    `fs.readFileSync("", "utf8")` will throw `ENOENT`, the `finish(1)` handler fires, and every orchestrator worker launch fails with exit code 1 rather than the real exit code. This makes the worker silently fail without a meaningful error message.
    
    Change the script to read `process.argv[2]`:
    ```js
    const specPath = process.argv[2];
    ```
    
    The same fix applies to the failure-path inline script on the last `return` branch of `buildStartupCommand`:
    ```js
    args: ["-e", "console.error(process.argv[2]); process.exit(64);", failureMessage],
    ```
    
    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: 247-268

Comment:
**PowerShell single-quoted string `''` escape not handled in tokenizer**

In PowerShell, `''` inside a single-quoted string is the escape sequence for a literal single-quote character (analogous to SQL). The tokenizer closes the string when it sees the first `'`, so `Remove-Item 'C:\project\user''s dir\file.txt'` is parsed as two tokens — `C:\project\user` and `s dir\file.txt` — rather than the single path `C:\project\user's dir\file.txt`.

The `addResolvedPath` logic then checks `C:\project\user` (which is inside the allowed workspace root) and ignores the trailing fragment. An agent could craft a command like `Remove-Item 'C:\workspace\allowed''/../outside.txt'` where the first token is an allowed workspace path, causing the sandbox path check to pass while PowerShell actually resolves and operates on a path outside the sandbox.

When the `quote` is `'` and the next character is also `'`, the tokenizer should emit a literal `'` and stay inside the string rather than closing it:
```typescript
if (quote) {
  if (ch === quote) {
    if (quote === "'" && command[i + 1] === "'") {
      // PowerShell escaped single-quote inside single-quoted string
      current += "'";
      i += 1;
    } else {
      quote = null;
    }
  } else {
    current += ch;
  }
  continue;
}
```

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: 951-955

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

`navigator.platform` is deprecated in all modern browsers and may return an empty string in some environments. The MDN-recommended replacement is `navigator.userAgentData?.platform` for browser contexts. In practice the Electron renderer always has `process.platform`, so the `navigator.platform` branch is only reached in unit tests or pure-browser contexts. Consider checking `navigator.userAgentData?.platform` first:

```typescript
function currentShellPlatform(): ShellPlatform {
  if (typeof navigator !== "undefined") {
    const p = (navigator as { userAgentData?: { platform?: string } }).userAgentData?.platform?.toLowerCase()
              ?? navigator.platform;
    if (/win/i.test(p)) return "win32";
  }
  if (typeof process !== "undefined" && typeof process.platform === "string") return process.platform;
  return "browser";
}
```

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/main/services/orchestrator/providerOrchestratorAdapter.ts
Line: 348-413

Comment:
**`process.argv[1]` is empty when Node runs with `-e`**

The `WORKER_CLI_LAUNCHER_SCRIPT` is launched as:
```
node -e SCRIPT launchFilePath
```
When Node.js (or Electron in `ELECTRON_RUN_AS_NODE` mode) runs with `-e`, `process.argv[1]` is set to the empty string `""` — the first positional argument lands at `process.argv[2]`. The script does:

```js
const specPath = process.argv[1]; // ← always "" when launched with -e
```

`fs.readFileSync("", "utf8")` will throw `ENOENT`, the `finish(1)` handler fires, and every orchestrator worker launch fails with exit code 1 rather than the real exit code. This makes the worker silently fail without a meaningful error message.

Change the script to read `process.argv[2]`:
```js
const specPath = process.argv[2];
```

The same fix applies to the failure-path inline script on the last `return` branch of `buildStartupCommand`:
```js
args: ["-e", "console.error(process.argv[2]); process.exit(64);", failureMessage],
```

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

Reviews (9): Last reviewed commit: "Merge main into Windows port lane" | 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