Windows port foundations: MCP IPC, CLI resolution, portable tooling#186
Windows port foundations: MCP IPC, CLI resolution, portable tooling#186
Conversation
Consolidates the windows-port-foundations work (MCP named pipes, CLI PATH, portable shell usage, sandbox defaults, cmd worker shell, WMIC process list, CLI resolution/sandbox hardening, process spawning) and the subsequent ADE CLI launch + path handling polish into a single squashed commit on top of the latest main. Previously this lane consisted of: - c2a299f feat(windows): MCP named pipes, CLI PATH, portable shell usage - bd5eb3b feat(windows): sandbox defaults, cmd worker shell, WMIC process list - b8508fa fix(pr-160): address Greptile and CodeRabbit review comments - 1d528ba Harden Windows CLI resolution and sandbox checks - 5fa50fc Fix Windows CLI process spawning - 0ecf039 Merge origin/main (absorbed MCP proxy removal) - 8ada560 Enable Windows ADE CLI launches and path handling Squashed because main removed the MCP proxy (PR #166) after the earlier Windows commits were authored, so replaying the original commits atop the new main hits many modify/delete conflicts against files that no longer exist. The final state here matches what the merge commit already reconciled plus the subsequent ADE CLI launch work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extract resolveTailscaleCliPath with path.win32 joins and env override. Wire deviceRegistryService and syncHostService. Add unit tests. Document in the Windows port lane and ARCHITECTURE. Made-with: Cursor
Restores review polish (processExecution, FilesPage, LaneGitActionsPane, AgentChatComposer, pathUtils) and exports PowerShell command tokenizer with doubled-single-quote escape support. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (89)
✨ 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 |
|
@copilot review but do not make fixes |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
|
|
||
| export function getPathEnvKey(env: NodeJS.ProcessEnv): string { | ||
| if (process.platform !== "win32") return "PATH"; | ||
| return Object.keys(env).find((key) => key.toLowerCase() === "path") ?? "Path"; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
On Windows this helper now returns the first case-insensitive path key, which regresses the common state where the process inherited Path from Windows and later code appends to process.env.PATH directly. The repo still has those direct writes in @apps/desktop/src/main/main.ts, @apps/desktop/src/main/services/ai/authDetector.ts, and @apps/desktop/src/main/services/opencode/openCodeBinaryManager.ts, so after those mutations getPathEnvValue() will keep reading the older Path entry and ignore the newly augmented PATH. That means augmentProcessPathWithShellAndKnownCliDirs() / resolveExecutableFromKnownLocations() can miss directories the app just added, defeating the Windows fallback this PR is trying to introduce. Prefer the canonical updated key when duplicates exist (or collapse duplicates before reading).
// apps/desktop/src/main/services/ai/cliExecutableResolver.ts
export function getPathEnvKey(env: NodeJS.ProcessEnv): string {
if (process.platform !== "win32") return "PATH";
return Object.keys(env).find((key) => key.toLowerCase() === "path") ?? "Path";
}| const getHeadlessLaneRuntimeEnv = async (laneId: string): Promise<Record<string, string>> => { | ||
| const lanes = await laneService.list({ includeArchived: false, includeStatus: false }); | ||
| const lane = lanes.find((entry) => entry.id === laneId); | ||
| const laneIndex = Math.max(0, lanes.findIndex((entry) => entry.id === laneId)); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
@apps/ade-cli/src/bootstrap.ts now derives lane runtime env from the lane’s current list position and a raw slug:
const laneIndex = Math.max(0, lanes.findIndex((entry) => entry.id === laneId));
const portStart = 3000 + laneIndex * 100;
const portEnd = portStart + 99;
const slug = (lane?.name ?? lane?.branchRef ?? laneId)I verified that laneService.list() is ordered by created_at desc (@apps/desktop/src/main/services/lanes/laneService.ts:573-579,854-887), while the desktop runtime uses portAllocationService.getLease() plus laneProxyService.generateHostname() (@apps/desktop/src/main/main.ts:1791-1805) specifically to keep per-lane ports stable and hostnames collision-safe. With this helper, creating a newer lane shifts every existing lane’s PORT/PORT_RANGE_* on its next headless restart, and two lanes whose names slugify the same way can receive the same HOSTNAME/PROXY_HOSTNAME. That breaks the existing desktop contract for lane-local URLs/cookie isolation in headless runs. Reuse the same lease/hostname generation logic here, or persist an equivalent per-lane allocation, instead of deriving these values from list order and raw slugification.
| canonicalRoot = projectRoot; | ||
| } | ||
| const root = path.win32.resolve(canonicalRoot).replace(/\//g, "\\"); | ||
| const id = createHash("sha256").update(root.toLowerCase()).digest("hex").slice(0, 24); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
resolveAdeMcpIpcPath now hashes root.toLowerCase() before building the pipe name:
// apps/desktop/src/shared/adeMcpIpc.ts
const root = path.win32.resolve(canonicalRoot).replace(/\//g, "\\");
const id = createHash("sha256").update(root.toLowerCase()).digest("hex").slice(0, 24);
return `\\\\.\\pipe\\ade-${id}`;Windows can host per-directory case-sensitive trees (common with WSL/DrvFs interop), so C:\Work\Repo and C:\Work\repo can be different directories even though this helper forces them to the same hash. In that setup, both projects resolve to the same named pipe, so one desktop instance can block the other or the CLI can attach to the wrong project. Use the canonical realpathSync.native() result as-is for the hash, or limit normalization to components that are truly case-insensitive (for example the drive prefix) instead of lowercasing the entire path.
…tion The late-polish commit wrapped the joined cmd line in an extra pair of quotes, producing ""exe" "arg"" with windowsVerbatimArguments. That double-wrap broke three CI shards by shifting expected strings in usageTrackingService, openCodeServerManager, and ptyService tests, and corrupted cmd.exe parsing when openCodeServerManager concatenates args[3] with other set commands. Reverts resolveWindowsCmdInvocation to emit a single unquoted joined line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
| if (process.platform === "win32") { | ||
| spawnSync("taskkill.exe", ["/pid", String(child.pid), "/t", "/f"], { stdio: "ignore", windowsHide: true }); | ||
| } else { | ||
| child.kill("SIGTERM"); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
nodeWorkerLaunch() is now the execution path for CLI-wrapped orchestrator workers from both the adapter and @apps/desktop/src/main/services/orchestrator/orchestratorService.ts, but the embedded launcher only does a direct child.kill("SIGTERM") on Unix. That means when ADE cancels/stops a Claude or Codex worker while the CLI has spawned tool subprocesses, the new Node wrapper exits and the top-level CLI gets signaled, but its grandchildren can remain running in the lane after the session is marked ended. This is introduced by the new launcher indirection: the old startupCommand path used exec ..., so the PTY owned the actual CLI process directly instead of a wrapper that only forwards SIGTERM to one PID. The wrapper needs Unix tree semantics here (for example by spawning the worker in its own process group and signaling -child.pid, or by walking the child tree) before this path is safe.
// apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts
if (process.platform === "win32") {
spawnSync("taskkill.exe", ["/pid", String(child.pid), "/t", "/f"], { stdio: "ignore", windowsHide: true });
} else {
child.kill("SIGTERM");
}| const role = isLocalBrain ? "brain" : "viewer"; | ||
| const canHostPhonePairing = role === "brain" && hostStartupEnabled; | ||
| const crdtSyncAvailable = isCrdtSyncAvailable(); | ||
| const canHostPhonePairing = role === "brain" && hostStartupEnabled && crdtSyncAvailable; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
getStatus() now nulls out all pairing data when crdtSyncAvailable is false, but it still reports the desktop as role === "brain":
// apps/desktop/src/main/services/sync/syncService.ts
const crdtSyncAvailable = isCrdtSyncAvailable();
const canHostPhonePairing = role === "brain" && hostStartupEnabled && crdtSyncAvailable;
...
bootstrapToken: canHostPhonePairing ? readToken() : null,
pairingConnectInfo: canHostPhonePairing ? buildPairingConnectInfo({ localDevice }) : null,The existing renderer uses status.role === "brain" to render PairPhoneCard, and that component treats qrPayloadText: null as "Generating QR..." while still exposing PIN actions. On Windows builds where db.sync.isAvailable() is false, the settings screen will therefore present a broken host-pairing UI that only fails on interaction instead of surfacing a clear disabled state. Either add an explicit availability flag to SyncRoleSnapshot and gate the renderer on it, or return a status shape that lets callers distinguish host desktop from phone pairing unavailable.
| } | ||
| }); | ||
| if (!next || next === targetPath) return; | ||
| if (!next || areWorkspacePathsEqual(next, targetPath, workspaceComparisonRoot)) return; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
On Windows, areWorkspacePathsEqual() folds case, so this new early return treats src/Main.ts → src/main.ts as unchanged and skips window.ade.files.rename(...) entirely. That is a user-visible regression introduced by this diff: the dialog validation still only rejects exact string equality, the older renderer (FloatingFilesWorkspace) still allows case-only renames, and the backend path (files.rename → secureRenameWithinRoot → fs.renameSync) does not forbid them. The result is that case-correction renames silently no-op in the main Files page.
// apps/desktop/src/renderer/components/files/FilesPage.tsx
validate: (value) => {
if (!value) return "Path is required.";
if (value === targetPath) return "Path is unchanged.";
}
});
if (!next || areWorkspacePathsEqual(next, targetPath, workspaceComparisonRoot)) return;| if (!next || areWorkspacePathsEqual(next, targetPath, workspaceComparisonRoot)) return; | |
| if (!next || next === targetPath) return; |
|
|
||
| const WRITE_COMMAND_RE = /(?:>|>>|\btee\b|\bcp\s|\bmv\s|\brm\s|\bwrite\b|\bedit\b)/; | ||
| const MUTATING_BASH_RE = /\b(?:rm|mv|cp|mkdir|touch|chmod|chown|patch|install|uninstall|add|remove|upgrade|apply|commit|rebase|merge|reset|checkout|switch|restore|sed\s+-i|perl\s+-i)\b|>>?|tee\b/i; | ||
| const MUTATING_CMD_RE = /\b(?:copy|xcopy|robocopy|move|del|erase|rd|rmdir|md|mkdir|ren|rename)\b|>>?|tee\b/i; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new Windows mutation regex matches md as a standalone word anywhere in the command string, which also matches common file extensions like README.md. Because createBashTool() gates required-turn execution on bashCommandLikelyMutates(), harmless reads such as powershell.exe -Command "Get-Content .\\README.md" are now rejected with EXECUTION DENIED even though the command is read-only. Narrow this detection to actual command positions / parsed command names instead of a raw substring regex.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
const MUTATING_CMD_RE = /\b(?:copy|xcopy|robocopy|move|del|erase|rd|rmdir|md|mkdir|ren|rename)\b|>>?|tee\b/i;
if (requiresTurnMemoryGuard(turnMemoryPolicyState) && bashCommandLikelyMutates(command)) {
return {| ["rename-item", "rename"], | ||
| ["ren", "rename"], | ||
| ["rni", "rename"], | ||
| ["set-content", "path-write"], |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The new PowerShell mutation map recognizes set-content but omits the built-in Windows PowerShell alias sc. On powershell.exe, a command like powershell.exe -Command "sc .env secret" still writes to .env, but inspectPowerShellScript() will not mark it as a mutation, so checkWorkerSandbox() skips the protected-file write check and allows the command as long as the path stays inside the repo. Add the alias (and ideally audit the rest of the built-in mutation aliases) so the new Windows sandbox cannot be bypassed with standard shorthand.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
["rename-item", "rename"],
["ren", "rename"],
["rni", "rename"],
["set-content", "path-write"],
["set-item", "provider-item"],
Greptile P1: - openCodeServerManager killProcessTree: on Unix, send SIGTERM to the process group and pkill children, then fall back to single-pid SIGTERM. - ptyService: strengthen startup-command guard to only type into a real interactive shell (&& selectedShell). Greptile P2: - adeCliService: drop duplicated PATH env helpers; import from cliExecutableResolver; drop mixed-case PATHEXT fallback. - processExecution: document quoteWindowsCmdArg is only safe inside resolveWindowsCmdInvocation's /s /c wrapper. - shell: restore [] return for empty/whitespace parseCommandLine input (behavioral regression). capy-ai Medium: - cliExecutableResolver: getPathEnvKey prefers canonical PATH over case-variants; direct writers (main, authDetector, openCodeBinaryManager) go through setPathEnvValue so Windows duplicate Path/PATH keys collapse. - bootstrap (ade-cli) getHeadlessLaneRuntimeEnv: derive lane slot from sha256(laneId) mod 70 so archival/reordering doesn't shift ports; add 6-char hash suffix to hostname to prevent same-slug collisions. - adeMcpIpc resolveAdeMcpIpcPath: hash realpathSync.native output directly instead of lowercasing first, so genuinely distinct case-sensitive directories get distinct pipes while case-variants of the same dir collapse via realpath canonicalization. Verification: 7 touched test files, 119 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ting Future agents have been pushing CI-only fixes the moment CI fails, while review bots (Greptile in particular) are still running. That wastes the next CI cycle because review comments land on a SHA the agent already abandoned, forcing a second push. The 12-minute wait was supposed to cover this but the playbook only talked about CI. Changes: - Execution contract: add an explicit "one push per iteration" rule and spell out the 12-minute floor as the wait that lets both signals land. - Phase 1: document that reviewBotsRunning is its own signal; name the expected bots for this repo (Greptile, CodeRabbit, Copilot) and how to detect each one's terminal state; add pendingReviewBots to the summary shape. - Phase 2 decide table: replace the "ciRunning" row with a broader rule that also blocks on reviewBotsRunning; clarify that the "go to fix" row requires BOTH signals terminal and that CI + review fixes go in the same push. - Phase 5 cadence: add "do not re-wake at 270s after the settled poll" guidance so agents don't iterate every 4.5 min and starve review bots. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
| if (fromWmic.length > 0 && fromWmic.every((process) => process.command.trim().length > 0)) { | ||
| return fromWmic; | ||
| } |
There was a problem hiding this comment.
fromWmic.every((process) => process.command.trim().length > 0) will always be false on any Windows system because kernel processes like "System" (PID 4), SMSS, and csrss.exe have a NULL/empty CommandLine in every WMIC query. This means listWindowsProcesses unconditionally falls through to PowerShell on every call, even when WMIC succeeds. PowerShell startup incurs ~1–2 s of latency every time process enumeration is needed (orphan recovery, server detection), making server lifecycle operations noticeably slower.
A better guard checks whether at least one non-trivial process has a populated command line:
function listWindowsProcesses(): OpenCodeProcessSnapshot[] {
const fromWmic = listWindowsProcessesFromWmic();
// Kernel processes (System, SMSS, etc.) always have empty CommandLine —
// use .some() so WMIC results aren't discarded just because they exist.
const wmicUsable = fromWmic.length > 0 && fromWmic.some((p) => p.command.trim().length > 0);
if (wmicUsable) return fromWmic;
return listWindowsProcessesFromPowerShell();
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/opencode/openCodeServerManager.ts
Line: 257-259
Comment:
**WMIC primary path is always bypassed — WMIC is effectively dead code**
`fromWmic.every((process) => process.command.trim().length > 0)` will always be `false` on any Windows system because kernel processes like "System" (PID 4), SMSS, and csrss.exe have a `NULL`/empty `CommandLine` in every WMIC query. This means `listWindowsProcesses` unconditionally falls through to PowerShell on every call, even when WMIC succeeds. PowerShell startup incurs ~1–2 s of latency every time process enumeration is needed (orphan recovery, server detection), making server lifecycle operations noticeably slower.
A better guard checks whether at least one non-trivial process has a populated command line:
```typescript
function listWindowsProcesses(): OpenCodeProcessSnapshot[] {
const fromWmic = listWindowsProcessesFromWmic();
// Kernel processes (System, SMSS, etc.) always have empty CommandLine —
// use .some() so WMIC results aren't discarded just because they exist.
const wmicUsable = fromWmic.length > 0 && fromWmic.some((p) => p.command.trim().length > 0);
if (wmicUsable) return fromWmic;
return listWindowsProcessesFromPowerShell();
}
```
How can I resolve this? If you propose a fix, please make it concise.
| export function resolveWindowsCmdInvocation( | ||
| command: string, | ||
| args: string[], | ||
| env: NodeJS.ProcessEnv = process.env, | ||
| ): SpawnInvocation { | ||
| const comSpec = env.ComSpec?.trim() || "cmd.exe"; | ||
| const cmdLine = [command, ...args].map(quoteWindowsCmdArg).join(" "); |
There was a problem hiding this comment.
resolveWindowsCmdInvocation missing outer-quote wrapper for cmd.exe /s
cmdLine is built from individually-quoted tokens ("exe.cmd" "arg1" "arg2") and placed directly as the 4th args element. With windowsVerbatimArguments: true, Node.js produces a Win32 command line like:
cmd.exe /d /s /c "exe.cmd" "arg1" "arg2"
The /s flag causes cmd.exe to strip only the first and last " from the entire string after /c, leaving exe.cmd" "arg1" "arg — a broken command. Node's own shell: true avoids this by wrapping the entire cmdLine in an extra outer pair of quotes so /s strips only the wrapper:
return {
command: comSpec,
args: ["/d", "/s", "/c", `"${cmdLine}"`],
windowsVerbatimArguments: true,
};Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/shared/processExecution.ts
Line: 62-68
Comment:
**`resolveWindowsCmdInvocation` missing outer-quote wrapper for cmd.exe `/s`**
`cmdLine` is built from individually-quoted tokens (`"exe.cmd" "arg1" "arg2"`) and placed directly as the 4th args element. With `windowsVerbatimArguments: true`, Node.js produces a Win32 command line like:
```
cmd.exe /d /s /c "exe.cmd" "arg1" "arg2"
```
The `/s` flag causes cmd.exe to strip **only the first and last** `"` from the entire string after `/c`, leaving `exe.cmd" "arg1" "arg` — a broken command. Node's own `shell: true` avoids this by wrapping the entire cmdLine in an extra outer pair of quotes so `/s` strips only the wrapper:
```typescript
return {
command: comSpec,
args: ["/d", "/s", "/c", `"${cmdLine}"`],
windowsVerbatimArguments: true,
};
```
How can I resolve this? If you propose a fix, please make it concise.
| if (finalPrompt) { | ||
| commandParts.push(shellEscapeArg(finalPrompt)); | ||
| commandArgs.push(finalPrompt); | ||
| commandPreviewParts.push(previewShellEscapeArg(finalPrompt)); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When providerExecutable cannot be resolved, this path falls back to typing startupCommand into an interactive shell. On Windows that shell is powershell.exe first (see @apps/desktop/src/main/services/pty/ptyService.ts), but the new preview command is built with previewShellEscapeArg(), which uses windowsShellEscapeArg()'s cmd-style double-quote rules instead of PowerShell quoting. In PowerShell, double-quoted strings expand $... and use backtick rather than backslash for quote escaping, so prompts like Use $env:USERPROFILE or text containing " arrive mutated instead of literal whenever the fallback path is used.
// apps/ade-cli/src/adeRpcServer.ts
if (finalPrompt) {
commandArgs.push(finalPrompt);
commandPreviewParts.push(previewShellEscapeArg(finalPrompt));
}That makes the new unresolved-provider fallback incorrect for non-trivial Windows prompts. Either build a PowerShell-safe startup command for this branch, or force unresolved Windows fallbacks through cmd.exe so the escaping matches the shell that actually executes it.
| const cmdLine = [command, ...args].map(quoteWindowsCmdArg).join(" "); | ||
| return { | ||
| command: comSpec, | ||
| args: ["/d", "/s", "/c", cmdLine], |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
resolveWindowsCmdInvocation() still passes the tokenized command line directly as the /c payload instead of wrapping the whole payload in an outer quote pair:
// apps/desktop/src/main/services/shared/processExecution.ts
const cmdLine = [command, ...args].map(quoteWindowsCmdArg).join(" ");
return {
command: comSpec,
args: ["/d", "/s", "/c", cmdLine],With windowsVerbatimArguments: true, cmd.exe /s strips the first and last " from that entire string, so .cmd and extensionless launches like codex.cmd exec ... get mangled before cmd.exe resolves the executable. This helper is now on the hot path for PTY direct commands, provider task runs, worker CLI RPC, automation planners, lane env setup, and other Windows launches added in this PR, so those callers will fail on Windows. Wrap the full cmdLine in one extra outer quote pair and update the new tests to assert the fixed shape.
| args: ["/d", "/s", "/c", cmdLine], | |
| args: ["/d", "/s", "/c", `"${cmdLine}"`], |
| const lanes = await laneService.list({ includeArchived: false, includeStatus: false }); | ||
| const lane = lanes.find((entry) => entry.id === laneId); | ||
| const laneHash = createHash("sha256").update(laneId).digest(); | ||
| const slotIndex = laneHash.readUInt32BE(0) % HEADLESS_MAX_SLOTS; |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
This replaces the previous order-based mapping with a modulo hash, but unlike desktop's portAllocationService.acquire() it never checks whether the chosen slot is already in use by another active lane. With only 70 slots, collisions are guaranteed once there are 71 active lanes and can occur earlier for any two laneIds whose hash lands on the same bucket, so separate headless lanes can receive identical PORT / PORT_RANGE_* values and then fight over the same dev-server ports.
// apps/ade-cli/src/bootstrap.ts
const laneHash = createHash("sha256").update(laneId).digest();
const slotIndex = laneHash.readUInt32BE(0) % HEADLESS_MAX_SLOTS;
const portStart = HEADLESS_BASE_PORT + slotIndex * HEADLESS_PORTS_PER_LANE;
const portEnd = portStart + HEADLESS_PORTS_PER_LANE - 1;That breaks the runtime invariant the desktop code relies on: active lanes must have unique port ranges. Fix by reusing the same lease-based allocator in headless mode, or at minimum detect occupied slots and probe for a free range instead of returning hash % 70 directly.
|
|
||
| function listWindowsProcesses(): OpenCodeProcessSnapshot[] { | ||
| const fromWmic = listWindowsProcessesFromWmic(); | ||
| if (fromWmic.length > 0 && fromWmic.every((process) => process.command.trim().length > 0)) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
parseWindowsWmicProcessCsv() preserves rows with an empty CommandLine, but this guard requires every WMIC row to have a non-empty command before it will return the WMIC result. System/kernel rows like System or Registry routinely have blank command lines, so successful WMIC output still falls through to PowerShell/CIM on every call. That makes the WMIC path dead code, adds a second expensive process listing before every orphan-recovery scan, and breaks recovery on hosts where PowerShell is blocked even though WMIC succeeded. Use a success signal like some(...) instead of every(...).
// apps/desktop/src/main/services/opencode/openCodeServerManager.ts
const fromWmic = listWindowsProcessesFromWmic();
if (fromWmic.length > 0 && fromWmic.every((process) => process.command.trim().length > 0)) {
return fromWmic;
}
return listWindowsProcessesFromPowerShell();| if (fromWmic.length > 0 && fromWmic.every((process) => process.command.trim().length > 0)) { | |
| if (fromWmic.length > 0 && fromWmic.some((process) => process.command.trim().length > 0)) { |
| fail(`Packaged smoke expected Codex executable resolver to be available, got ${String(payload?.codexExecutable)}`); | ||
| } | ||
|
|
||
| const defaultHelp = await runCommand(adeCliBinPath, ["--help"], { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new Windows release gate only smoke-tests ade.cmd --help (and the same help path for the ADE_CLI_NODE, plain-node fallback, and installed shim cases), but --help is a fast path that never opens either a desktop-socket or headless ADE CLI connection. In @apps/ade-cli/src/cli.ts, buildCliPlan() returns { kind: "help" } for --help, and runCli() exits immediately for help plans before createConnection() is called. That means the release validator can pass even if the packaged Windows wrapper/runtime is broken for the two execution modes the repo instructions explicitly require ADE CLI changes to verify.
// apps/desktop/scripts/validate-win-artifacts.mjs
const defaultHelp = await runCommand(adeCliBinPath, ["--help"], {
cwd: resourcesPath,
env: { ...process.env },
});
assertAdeCliHelp(defaultHelp.stdout, "Bundled ADE CLI wrapper");This matters because packaging regressions in project-root resolution, named-pipe/socket setup, or the headless desktop-backed RPC path would ship undetected on Windows. Augment the validator with at least one real headless command and one desktop-socket-backed command instead of treating help output as sufficient CLI validation.
| for (const flag of flags) { | ||
| const values = parsed.named.get(flag); | ||
| if (values?.length) { | ||
| return addResolvedPath(values[0]!, access); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
// apps/desktop/src/main/services/ai/tools/universalTools.ts
for (const flag of flags) {
const values = parsed.named.get(flag);
if (values?.length) {
return addResolvedPath(values[0]!, access);
}
}addFirstNamedOrPositionalPath() stops at values[0], but the PowerShell cmdlets you added support -Path <string[]> / -LiteralPath <string[]>. A command like powershell.exe -Command "Set-Content -Path .\\ok.txt, ..\\escape.txt -Value hi" is therefore marked as mutating while only ok.txt is recorded in pathRefs; the second target is never checked against allowedPaths or protectedFiles, so the new Windows sandbox can still write outside the repo or into protected files. Iterate and validate every supplied path value for list-valued parameters instead of returning after the first one.
| 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] }; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
// apps/desktop/src/main/services/ai/tools/universalTools.ts
if (process.platform === "win32") {
const comSpec = process.env.ComSpec?.trim() || "cmd.exe";
return { file: comSpec, args: ["/d", "/s", "/c", command] };
}This new launcher passes the raw shell string as the /c payload while also enabling /s. Per cmd.exe's /s rules, if the command begins with a quoted executable path, the first and last quotes are stripped before execution. So a perfectly normal Windows shell command like "C:\\Program Files\\nodejs\\npm.cmd" test loses the quotes that protect the spaced path and fails to start. The shared Windows spawn helper in @apps/desktop/src/main/services/shared/processExecution.ts already handles this by wrapping the whole payload in one extra outer quote pair; this path should do the same or reuse that helper.
Vercel was triggering a preview deploy for every push on every branch, including PRs that don't touch apps/web (like this Windows port). That burned the Vercel free tier's 100-deploys-per-day cap on byte-identical previews. ignoreCommand exits 0 (skip) when HEAD^..HEAD has no changes under the Vercel project root (apps/web), which is exactly the common case for desktop/CLI PRs. Active change sets still get previews as before. Takes effect on the next push. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/arul28s-projects?upgradeToPro=build-rate-limit |
| } | ||
|
|
||
| const WORKER_CLI_LAUNCHER_SCRIPT = ` | ||
| const fs = require("fs"); |
There was a problem hiding this comment.
process.argv[1] is always '' (empty) when invoked via node -e
When Node.js runs node -e <script> <launchFilePath>, the argv[1] is set to '' (empty string) — the actual first positional argument lands at argv[2]. The launcher reads:
const specPath = process.argv[1]; // '' — always empty in -e mode
const spec = JSON.parse(fs.readFileSync(specPath, "utf8")); // throws ENOENTThis will throw on every worker CLI invocation. The fix is to use process.argv[2]. The same off-by-one applies to the unsupported-model fallback in this file: args: ["-e", "console.error(process.argv[1]); process.exit(64);", failureMessage] — should be process.argv[2].
| } | |
| const WORKER_CLI_LAUNCHER_SCRIPT = ` | |
| const fs = require("fs"); | |
| const specPath = process.argv[2]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts
Line: 99-102
Comment:
**`process.argv[1]` is always `''` (empty) when invoked via `node -e`**
When Node.js runs `node -e <script> <launchFilePath>`, the `argv[1]` is set to `''` (empty string) — the actual first positional argument lands at `argv[2]`. The launcher reads:
```js
const specPath = process.argv[1]; // '' — always empty in -e mode
const spec = JSON.parse(fs.readFileSync(specPath, "utf8")); // throws ENOENT
```
This will throw on every worker CLI invocation. The fix is to use `process.argv[2]`. The same off-by-one applies to the unsupported-model fallback in this file: `args: ["-e", "console.error(process.argv[1]); process.exit(64);", failureMessage]` — should be `process.argv[2]`.
```suggestion
const specPath = process.argv[2];
```
How can I resolve this? If you propose a fix, please make it concise.
| return `'${sanitized.replace(/'/g, `'"'"'`)}'`; | ||
| } | ||
|
|
||
| function windowsShellEscapeArg(value: string): string { |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
windowsShellEscapeArg() produces a cmd.exe-style double-quoted token, but the resulting startupCommand is later executed by the tracked-PTY resume/fallback path in powershell.exe first on Windows (resolveShellCandidates() prefers PowerShell, and useWorkSessions feeds startupCommand back into pty.create() on resume). In PowerShell, $name, ` escapes, and $() are still interpreted inside double quotes, so prompts like Investigate $env:USERPROFILE and $(Get-Date) are mutated or partially executed before Claude/Codex ever receives them whenever resolveExecutableOnPath() falls back to shell startup. Use PowerShell-safe quoting for the Windows startupCommand path, or force cmd.exe for the shell that consumes this string.
// apps/ade-cli/src/adeRpcServer.ts
function windowsShellEscapeArg(value: string): string {
const sanitized = stripInjectionChars(value);
if (!sanitized.length) return "\"\"";
if (/^[a-zA-Z0-9_.:/\\-]+$/.test(sanitized)) return sanitized;
let quoted = "\"";| **Wait for both CI and review bots before iterating.** The poll must treat these as two independent signals and only report "ready to fix" when **both** are terminal: | ||
|
|
||
| - **CI terminal** = every required check has a final conclusion (`success`, `failure`, `cancelled`, `skipped`, `neutral`). If any required check is still `QUEUED` or `IN_PROGRESS`, CI is not terminal. | ||
| - **Review bots terminal** = every expected review bot has either posted its review (`gh api repos/.../pulls/{N}/reviews` contains a submission newer than `lastPushSha`'s commit time for each bot) **or** its status check entry has settled. Expected bots for this repo include **Greptile** (appears as the `Greptile Review` status check — wait for it to leave `pending`), **CodeRabbit** (posts a status check and/or review), and **Copilot** (posts an issue comment after being pinged; allow ~3–5 min from the ping). Greptile in particular is slow enough that the 12-minute wait is driven primarily by it. |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
Phase 1 makes Greptile and CodeRabbit unconditional members of the expected-bot set.
// docs/playbooks/ship-lane.md
- **Review bots terminal** = every expected review bot has either posted its review ...
- **Review bots terminal** = every expected review bot has either posted its review ... Expected bots for this repo include **Greptile** ... **CodeRabbit** ... **Copilot** ...But Phase 4 only posts @greptile review and @coderabbit review when the PR touches more than 250 files. On ordinary PRs, following this playbook literally leaves pendingReviewBots waiting on bots that were never invoked, so the lane can stay stuck in reviewBotsRunning and never advance to the combined fix iteration. Define the expected-bot set from the bots actually pinged for that PR (or persist that set in state during Phase 0/4) instead of listing Greptile and CodeRabbit as always expected.
| } | ||
| return { | ||
| command: env.ComSpec && env.ComSpec.trim() ? env.ComSpec.trim() : "cmd.exe", | ||
| args: ["/d", "/s", "/c", [cmd, ...args].map(quoteWindowsCmdArg).join(" ")], |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
@apps/desktop/scripts/dev.cjs repeats the broken Windows cmd.exe /s /c invocation shape:
// apps/desktop/scripts/dev.cjs
return {
command: env.ComSpec && env.ComSpec.trim() ? env.ComSpec.trim() : "cmd.exe",
args: ["/d", "/s", "/c", [cmd, ...args].map(quoteWindowsCmdArg).join(" ")],
windowsVerbatimArguments: true,
};On Windows every spawnProcess(..., "npx", ...) call in this script takes this branch because npx has no extension, so cmd.exe receives a post-/c string like "npx" "vite" .... With /s, cmd.exe strips only the first and last quote from the entire string, corrupting the executable token instead of preserving the individually quoted arguments. That breaks the core Windows dev flow here (npx vite, npx tsup, npx electron). Wrap the whole joined command line in an extra outer pair of quotes before passing it as the /c payload.
| args: ["/d", "/s", "/c", [cmd, ...args].map(quoteWindowsCmdArg).join(" ")], | |
| args: ["/d", "/s", "/c", `"${[cmd, ...args].map(quoteWindowsCmdArg).join(" ")}"`], |
| const windowsShell = process.env.ComSpec?.trim() || "cmd.exe"; | ||
| attempts.push({ | ||
| command: windowsShell, | ||
| args: ["/d", "/s", "/c", `start "" ${quoteWindowsCmdArg(cliCommand)} ${quoteWindowsCmdArg(targetPath)}`], |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new Windows editor-launch branch in @apps/desktop/src/main/services/ipc/registerIpc.ts builds a raw start ... command string and passes it directly as the /c payload:
const windowsShell = process.env.ComSpec?.trim() || "cmd.exe";
attempts.push({
command: windowsShell,
args: ["/d", "/s", "/c", `start "" ${quoteWindowsCmdArg(cliCommand)} ${quoteWindowsCmdArg(targetPath)}`],quoteWindowsCmdArg() is defined in @apps/desktop/src/main/services/shared/processExecution.ts as safe only when the entire post-/c command is wrapped in an outer quote pair. Reusing it here without that wrapper means cmd.exe reparses the start command directly, so valid Windows project/file paths containing shell metacharacters such as &, ^, or | can break openPathInEditor for VS Code/Cursor/Zed. Wrap the full start ... payload before passing it to /c so cmd strips only the outer quotes and preserves the individual quoted tokens.
| args: ["/d", "/s", "/c", `start "" ${quoteWindowsCmdArg(cliCommand)} ${quoteWindowsCmdArg(targetPath)}`], | |
| args: ["/d", "/s", "/c", `"start "" ${quoteWindowsCmdArg(cliCommand)} ${quoteWindowsCmdArg(targetPath)}"`], |
1 participant
Greptile Summary
This PR lays the Windows porting foundations: Windows named-pipe MCP IPC (
adeMcpIpc.ts), portable PATH helpers (getPathEnvKey/Value/setPathEnvValue), cmd.exe-safe process launching (resolveWindowsCmdInvocation,quoteWindowsCmdArg), Windows process enumeration (WMIC/PowerShell),taskkill /T /Ftree-kill, and.cmdshims for the ADE CLI.WORKER_CLI_LAUNCHER_SCRIPTreadsprocess.argv[1]to locate the launch-spec JSON, but when Node.js runsnode -e <script> <path>,argv[1]is always''(empty); the path is atargv[2]. Every call tonodeWorkerLaunchwill throw ENOENT before the child CLI is spawned. The same index is used in the unsupported-model fallback error handler.resolveWindowsCmdInvocationomitting an outer-quote wrapper for cmd.exe/swas not addressed and remains in the codebase.Confidence Score: 3/5
Not safe to merge: the Node.js worker launcher reads the wrong argv index and will crash on every invocation.
One confirmed P1 regression: process.argv[1] in WORKER_CLI_LAUNCHER_SCRIPT is always '' when invoked via node -e, so every orchestrated claude/codex worker launch throws ENOENT. The rest of the Windows-port foundations (IPC, PATH helpers, WMIC, CLI shims) look sound.
apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts (WORKER_CLI_LAUNCHER_SCRIPT argv index), apps/desktop/src/main/services/shared/processExecution.ts (resolveWindowsCmdInvocation /s quoting)
Important Files Changed
Sequence Diagram
sequenceDiagram participant Orchestrator as OrchestratorService participant Adapter as ProviderOrchestratorAdapter participant PTY as PTYService participant Launcher as node -e WORKER_CLI_LAUNCHER_SCRIPT participant CLI as claude/codex CLI Orchestrator->>Adapter: buildStartupCommand(prompt, model) Adapter->>Adapter: writeWorkerPromptFile(attemptId, prompt) Adapter->>Adapter: writeWorkerLaunchFile(attemptId, command, args, env) Adapter-->>Orchestrator: AdapterLaunchCommand {command: node, args: [-e, script, launchFile]} Orchestrator->>PTY: createTrackedSession(command, args, env, startupCommand) PTY->>Launcher: spawn(node, [-e, SCRIPT, launchFile]) Note over Launcher: process.argv[1] == '' ⚠️ should be process.argv[2] Launcher->>Launcher: readFileSync(argv[1]) → ENOENT Launcher--xCLI: never spawned Note over Orchestrator,CLI: MCP IPC: Windows uses named pipe, Unix uses .ade/ade.sockPrompt To Fix All With AI
Reviews (4): Last reviewed commit: "ci(web): skip Vercel build when apps/web..." | Re-trigger Greptile