Windows port foundations: MCP IPC, CLI resolution, portable tooling#182
Windows port foundations: MCP IPC, CLI resolution, portable tooling#182
Conversation
|
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 12 minutes and 37 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 selected for processing (20)
📝 WalkthroughWalkthroughThis PR introduces comprehensive Windows platform support for the desktop application and CLI. It adds Windows-specific build/release pipelines, Windows-aware process execution utilities, path normalization and comparison for Windows paths, shell command parsing for Windows, Windows MCP IPC named-pipe resolution, and Windows-specific sandbox enforcement. Additionally, the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
@copilot review but do not make fixes |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
arul28
force-pushed
the
cursor/windows-port-foundations-ede6
branch
from
cb78734 to
79926e6
Compare
|
@copilot review but do not make fixes |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| title, | ||
| tracked: true, | ||
| toolType: `${provider}-orchestrated`, | ||
| command: provider, |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
This change switches spawn_agent from shell-typing a startup string to PTY direct spawn, but it still passes the bare provider name instead of a resolved executable path:
// apps/ade-cli/src/adeRpcServer.ts
toolType: `${provider}-orchestrated`,
command: provider,
args: commandArgs,
env: workerEnv,
startupCommandOn the direct path, ptyService.create() calls resolveCliSpawnInvocation() against the ADE process env only, so it no longer benefits from the interactive shell that previously discovered user-installed CLIs via shell init. ADE’s existing direct-launch path in providerOrchestratorAdapter explicitly resolves claude/codex first (resolveClaudeCodeExecutable() / resolveCodexExecutable()), which is the resolution step this code now needs as well. In packaged desktop setups where the app process PATH does not already contain the provider binary, spawn_agent will now fail to start the worker session instead of launching the CLI. Resolve the provider executable before calling ptyService.create, or keep shell execution for unresolved providers.
arul28
force-pushed
the
cursor/windows-port-foundations-ede6
branch
from
79926e6 to
bcd3638
Compare
|
@copilot review but do not make fixes |
|
@copilot review but do not make fixes |
| - `npm run dist:win` — x64 installer via `electron-builder --win --x64`, wrapped with `validate:win:artifacts` (preflight) and `validate:win:release` (post-build) checks in `apps/desktop/scripts/validate-win-artifacts.mjs`. | ||
| - Windows-only wrappers for the bundled `ade` CLI ship in `apps/desktop/scripts/`: `ade-cli-windows-wrapper.cmd` (launcher) and `ade-cli-install-path.cmd` (idempotent PATH install helper). The platform-agnostic `.sh` wrapper covers macOS/Linux. | ||
| - The Windows installer bundles the prebuilt `cr-sqlite` native binary from `apps/desktop/vendor/crsqlite/win32-x64/` plus a Windows node-pty ConPTY worker. | ||
| - GitHub Actions `release-core.yml` drives signed/notarized Windows artifacts. |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This new bullet says the Windows release path is signed/notarized, but the checked-in workflow only wires signing/notarization credentials for the macOS job and the Windows job just runs npm run dist:win; Windows also has no notarization step. That means the architecture doc is asserting a release guarantee the current pipeline does not enforce, which can mislead release operators and agents validating artifact provenance. Update the wording to describe the actual behavior today (build + validate + upload for publish) unless Windows signing is added to release-core.yml.
# docs/ARCHITECTURE.md
- `npm run dist:win` — x64 installer via `electron-builder --win --x64`, wrapped with `validate:win:artifacts` (preflight) and `validate:win:release` (post-build) checks in `apps/desktop/scripts/validate-win-artifacts.mjs`.
- Windows-only wrappers for the bundled `ade` CLI ship in `apps/desktop/scripts/`: `ade-cli-windows-wrapper.cmd` (launcher) and `ade-cli-install-path.cmd` (idempotent PATH install helper). The platform-agnostic `.sh` wrapper covers macOS/Linux.
- The Windows installer bundles the prebuilt `cr-sqlite` native binary from `apps/desktop/vendor/crsqlite/win32-x64/` plus a Windows node-pty ConPTY worker.
- GitHub Actions `release-core.yml` drives signed/notarized Windows artifacts.| - GitHub Actions `release-core.yml` drives signed/notarized Windows artifacts. | |
| - GitHub Actions `release-core.yml` builds and validates Windows release artifacts, then uploads them for the publish step. |
| const env = buildIsolatedOpenCodeEnv(args.config, xdgPaths); | ||
| if (process.platform === "win32") { | ||
| const comSpec = process.env.ComSpec?.trim() || "cmd.exe"; | ||
| const exeForCmd = `"${executable.replace(/"/g, "\"\"")}"`; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This Windows launch path hand-rolls quoting for the executable token, but it only doubles " and does not escape %. cmd.exe expands %...% even inside quoted arguments, which is why the shared helper in processExecution.ts converts % to %%. As written, a resolved OpenCode path containing % will be rewritten before execution and opencode serve will fail to start on Windows. Reuse quoteWindowsCmdArg() / resolveWindowsCmdInvocation() here so the OpenCode launcher follows the same escaping contract as the rest of the Windows process code.
// apps/desktop/src/main/services/opencode/openCodeServerManager.ts
const comSpec = process.env.ComSpec?.trim() || "cmd.exe";
const exeForCmd = `"${executable.replace(/"/g, "\"\"")}"`;
const cmdLine =
`set "${ADE_OPENCODE_MANAGED_ENV}=1"&&set "OPENCODE_DISABLE_PROJECT_CONFIG=1"&&set "${ADE_OPENCODE_OWNER_PID_ENV}=${process.pid}"&&${exeForCmd} serve --hostname=127.0.0.1 --port=${args.port}`;| if ( | ||
| /\bcmd(?:\.exe)?\b/i.test(command) | ||
| && command.includes(`${ADE_OPENCODE_MANAGED_ENV}=1`) | ||
| && /\bopencode(?:\.cmd|\.exe)?\b/i.test(command) |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The Windows resolver/launcher now accepts opencode.bat as a valid executable candidate, but the managed-process detector still only matches opencode, opencode.exe, and opencode.cmd. If Windows resolves a .bat wrapper (via the new bundled fallback list or PATHEXT search), ADE can launch it, but recoverManagedOpenCodeOrphans() will never classify that command line as ADE-managed, so stale opencode serve trees survive crashes/restarts and can interfere with later launches. Keep the executable matcher in sync with the resolver by including .bat in both regexes or centralizing the pattern.
// apps/desktop/src/main/services/opencode/openCodeServerManager.ts
/\bopencode(?:\.cmd|\.exe)?\b/i.test(command)
...
if (!/\bopencode(?:\.cmd|\.exe)?\b\s+serve\b/i.test(command)) return false;| // Default to claude since it's the most common | ||
| return `exec claude -p ${shellInlineDecodedArg(prompt)}`; | ||
| return { | ||
| startupCommand: `exec claude -p ${shellInlineDecodedArg(prompt)}`, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new direct-launch override path hard-codes claude instead of using the same executable resolver as the normal worker-launch path, so provider steps that set metadata.startupCommand will fail on Windows/package installs where Claude is found via CLAUDE_CODE_EXECUTABLE_PATH, auth detection, or common-dir probing rather than plain PATH lookup. The rest of this file already treats executable resolution as required for Windows portability, so this override branch should do the same.
// apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts
return {
startupCommand: `exec claude -p ${shellInlineDecodedArg(prompt)}`,
command: "claude",
args: ["-p", prompt],
};| startupCommand: `exec claude -p ${shellInlineDecodedArg(prompt)}`, | |
| command: resolveClaudeCodeExecutable().path, |
| cwd: cwdLane.worktreePath, | ||
| env: process.env, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| timeout: 8 * 60_000, |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
resolveCliSpawnInvocation() now routes extensionless and .cmd resolver commands through cmd.exe /d /s /c ... on Windows, but this call site still relies on Node's built-in timeout, which only sends the kill signal to the spawned child process. In the new Windows path that child is cmd.exe, so a timed-out npm-style resolver can keep running after ADE has marked the run failed and started snapshotting git diff, leaving the lane mutating in the background and the recorded patch/log stale or incomplete. Use an explicit timer here and call terminateProcessTree(child, ...) instead of spawn(..., { timeout }) so the entire resolver tree is terminated consistently, matching the timeout handling already used in providerTaskRunner and shared/utils.
// apps/desktop/src/main/services/conflicts/conflictService.ts
const child = spawn(invocation.command, invocation.args, {
cwd: cwdLane.worktreePath,
env: process.env,
stdio: ["ignore", "pipe", "pipe"],
timeout: 8 * 60_000,
windowsVerbatimArguments: invocation.windowsVerbatimArguments,
});| const invocation = resolveCliSpawnInvocation(directCommand, directArgs, launchEnv); | ||
| created = ptyLib.spawn(invocation.command, invocation.args, opts); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
resolveCliSpawnInvocation() is doing the right thing for Windows .cmd/extensionless commands, but this PTY path drops the only part that matters to node-pty: its Windows API accepts either string[] argv or a single pre-escaped command-line string, and it does not expose windowsVerbatimArguments. The new code still passes an argv array into ptyLib.spawn(), so cmd.exe /c shims get re-escaped by node-pty and can misparse spaced/quoted arguments (for example workspace paths or resolved codex.cmd/claude.cmd launches). That means the new Windows direct-command PTY flow can fail even though the helper already produced the correct command line. Pass a single joined command-line string when invocation.windowsVerbatimArguments is true.
// apps/desktop/src/main/services/pty/ptyService.ts
const invocation = resolveCliSpawnInvocation(directCommand, directArgs, launchEnv);
created = ptyLib.spawn(invocation.command, invocation.args, opts);| const invocation = resolveCliSpawnInvocation(directCommand, directArgs, launchEnv); | |
| created = ptyLib.spawn(invocation.command, invocation.args, opts); | |
| const invocation = resolveCliSpawnInvocation(directCommand, directArgs, launchEnv); | |
| created = ptyLib.spawn(invocation.command, invocation.windowsVerbatimArguments ? invocation.args.join(" ") : invocation.args, opts); |
| } | ||
| } | ||
|
|
||
| if (!script) continue; |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
inspectPowerShellInvocations() only extracts scripts from -Command and -EncodedCommand, then silently skips every other PowerShell entrypoint with if (!script) continue;. PowerShell itself accepts both -File <script.ps1> and a bare script path as the default parameter, so commands like pwsh -File .\worker.ps1 or pwsh .\worker.ps1 will reach checkWorkerSandbox() with no inspected inner operations. That lets an agent run a workspace-local .ps1 whose contents write outside allowedPaths or touch protected files, which defeats the new Windows sandboxing path. Fail closed for -File/default-script invocation unless you also parse and validate the target script contents.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
if (POWERSHELL_COMMAND_FLAGS.has(flag)) { /* ... */ }
if (POWERSHELL_ENCODED_COMMAND_FLAGS.has(flag)) { /* ... */ }
if (!script) continue;| 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]
The new parser fails open for non-trivial -Path forms. parsePowerShellArgs() records an empty string when a path flag is followed by punctuation such as (, and addFirstNamedOrPositionalPath() only checks values[0]. As a result, Set-Content -Path (Join-Path .. outside.txt) ... records no path at all, and array-valued forms like Remove-Item .\ok.txt, ..\outside.txt validate only the first target. Because checkWorkerSandbox() still marks these commands as mutating, but never sees the real destination(s), writes outside the sandbox or to protected files slip through. Treat parenthesized/subexpression/array path arguments as uninspectable and reject them unless every resolved target is enumerated and checked.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
if (next.length > 0 && !isPowerShellParameterToken(next) && !POWERSHELL_PUNCTUATION_TOKENS.has(next)) {
values.push(next);
} else {
values.push("");
}
...
if (values?.length) {
return addResolvedPath(values[0]!, access);
}
|
@copilot review but do not make fixes |
|
|
||
| for (const segment of outerSegments) { | ||
| const commandName = normalizePathToken(segment[0] ?? ""); | ||
| if (!commandName.length || !isPowerShellHostCommand(commandName)) continue; |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
inspectPowerShellInvocations only analyzes segments whose first token is itself a PowerShell host, so a wrapped command like cmd /c powershell.exe -Command "Set-Content -Path .env -Value secret" never reaches the new PowerShell parser even though Windows workers are now executed through cmd.exe. The generic token/path scan also treats the quoted script as opaque, so protected targets such as .env or paths outside allowedPaths are never recorded and checkWorkerSandbox() allows the command. That weakens the API-worker sandbox boundary on Windows for exactly the PowerShell mutation cases this diff adds support for. Fix by unwrapping cmd.exe /c (and equivalent shell wrappers) before PowerShell inspection or by recursively inspecting nested shell hosts.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
const outerSegments = splitCommandSegments(tokenizeCommand(command, pathApi === path.win32));
let mutates = false;
for (const segment of outerSegments) {
const commandName = normalizePathToken(segment[0] ?? "");
if (!commandName.length || !isPowerShellHostCommand(commandName)) continue;| 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]
The new status gate only hides pairing data; it does not enforce the same invariant in the pairing mutators. ts // apps/desktop/src/main/services/sync/syncService.ts const crdtSyncAvailable = isCrdtSyncAvailable(); const canHostPhonePairing = role === "brain" && hostStartupEnabled && crdtSyncAvailable; ... pairingPin: canHostPhonePairing ? pinStore.getPin() : null, pairingPinConfigured: canHostPhonePairing ? pinStore.hasPin() : false, On platforms where db.sync.isAvailable() is false, setPin() still succeeds because it only checks hostStartupEnabled, while getStatus() immediately reports pairingPinConfigured: false and pairingConnectInfo: null. In the renderer this produces a false-success flow (PIN updated. followed by a still-disabled pairing card), which is not graceful degradation. Apply the same isCrdtSyncAvailable() guard to setPin/clearPin or expose an explicit unavailable state that disables the pairing controls.
| export function resolveAdeMcpIpcPath(projectRoot: string): string { | ||
| const root = path.resolve(projectRoot); | ||
| if (process.platform === "win32") { | ||
| const id = createHash("sha256").update(root.toLowerCase()).digest("hex").slice(0, 24); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
resolveAdeMcpIpcPath() hashes path.resolve(projectRoot) directly, so on Windows the same repo opened through different aliases (junction/symlink, mapped drive vs UNC, etc.) produces different pipe names even though both paths point at the same project. I verified that both the desktop server and the CLI consume this helper via resolveAdeLayout(), so a path-alias mismatch makes the CLI miss the live desktop socket and fall back to headless mode.
// apps/desktop/src/shared/adeMcpIpc.ts
const root = path.resolve(projectRoot);
if (process.platform === "win32") {
const id = createHash("sha256").update(root.toLowerCase()).digest("hex").slice(0, 24);
return `\\\\.\\pipe\\ade-${id}`;
}Canonicalize the Windows root with fs.realpathSync.native() (or equivalent fallback) before hashing so every alias of the same project resolves to one pipe id.
| await new Promise((resolve) => setTimeout(resolve, 25)); | ||
| } | ||
| if (activeHealthSweepRuns.has(run.id)) continue; | ||
| if (activeHealthSweepRuns.has(run.id)) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When an explicit sweep (chat_status, manual, etc.) arrives while another sweep for the same run is still active for more than 3 seconds, this branch now logs the overlap and then falls through into the sweep body without acquiring activeHealthSweepRuns. That reintroduces concurrent per-run sweeps, so two loops can race through tick(), onTrackedSessionEnded(), watchdog completeAttempt(), and autopilot startup for the same run, producing duplicate recovery side effects and nondeterministic state transitions. ```ts
// apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts
if (activeHealthSweepRuns.has(run.id)) {
logger.debug("ai_orchestrator.health_sweep_explicit_overlap", {
runId: run.id,
reason
});
}
if (!activeHealthSweepRuns.has(run.id)) {
|
|
||
| const vite = spawnProcess("renderer", "npx", ["vite", "--port", String(devPort), "--strictPort", "--force"]); | ||
| const main = spawnProcess("main", "npx", ["tsup", "--watch"]); | ||
| const vite = spawnProcess("renderer", npxCommand, ["vite", "--port", String(devPort), "--strictPort", "--force"]); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The new Windows branch routes all three launcher processes through npx.cmd, but spawnProcess() still calls child_process.spawn(cmd, args, ...) without shell: true or a cmd.exe /c wrapper. On Node 22.x (the repo's required runtime), .cmd files cannot be spawned this way on Windows, so the first Vite/Tsup launch fails with EINVAL and npm run dev never comes up. This also affects the later Electron launch/restart path because it uses the same helper. Fix by spawning through cmd.exe/shell: true on Windows, or by avoiding the .cmd wrapper entirely.
// apps/desktop/scripts/dev.cjs
const vite = spawnProcess("renderer", npxCommand, ["vite", "--port", String(devPort), "--strictPort", "--force"]);
const main = spawnProcess("main", npxCommand, ["tsup", "--watch"]);| workerEnv.ADE_DEFAULT_ROLE = "agent"; | ||
| envPrefixParts.push("ADE_DEFAULT_ROLE=agent"); | ||
|
|
||
| const startupCommand = envPrefixParts.length > 0 |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When resolveExecutableOnPath(provider) returns null, this code falls back to startupCommand only, but the preview string still embeds POSIX-style inline env assignments. ts // apps/ade-cli/src/adeRpcServer.ts const startupCommand = envPrefixParts.length > 0 ? `${envPrefixParts.join(" ")} ${commandPreviewParts.join(" ")}` : commandPreviewParts.join(" "); const providerExecutable = resolveExecutableOnPath(provider); In @apps/desktop/src/main/services/pty/ptyService.ts, sessions without command are launched by opening powershell.exe/cmd.exe on Windows and literally typing startupCommand into that shell. That makes the new fallback path fail on Windows for unresolved-but-shell-launchable providers, because ADE_RUN_ID=... claude ... is not valid PowerShell/cmd syntax. Fix by avoiding POSIX env-prefix fallback on Windows: either always pass command/args for Windows PTY launches, or build a shell-specific fallback command that sets env vars with Windows syntax before invoking the provider.
| let script: string | null = null; | ||
| for (let i = 0; i < args.length; i += 1) { | ||
| const flag = normalizePathToken(args[i] ?? "").toLowerCase(); | ||
| if (POWERSHELL_COMMAND_FLAGS.has(flag)) { |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
inspectPowerShellInvocations() only extracts script text from -Command and -EncodedCommand, then silently skips every other valid host form. PowerShell's own CLI supports -File (and even bare pwsh .\script.ps1 as the default parameter), so a worker can place a .ps1 file inside the repo and run pwsh -File .\mutate.ps1 to mutate protected files or paths outside allowedPaths without any of the new PowerShell path checks running. That breaks the sandbox’s security contract. Treat -File/bare-script execution as non-inspectable and block it, or read and inspect the referenced script before allowing execution.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
if (POWERSHELL_COMMAND_FLAGS.has(flag)) {
...
}
if (POWERSHELL_ENCODED_COMMAND_FLAGS.has(flag)) {
...
}
if (!script) continue;|
|
||
| if (isPowerShellParameterToken(raw)) { | ||
| const key = raw.toLowerCase(); | ||
| const next = normalizePathToken(args[i + 1] ?? ""); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
parsePowerShellArgs() assumes every -foo parameter consumes the next token unless that token also starts with -. That is not true for PowerShell switch parameters such as -NoNewline, -Force, or -Recurse. In a command like powershell -Command "Set-Content -NoNewline ..\\outside.txt secret", the parser incorrectly binds ..\\outside.txt as the value of -NoNewline, so the later positional-path lookup never sees the real write target and the sandbox can miss an out-of-root or protected-file write. Fix this by modeling switch parameters for the supported mutating cmdlets, or failing closed when an unknown flag appears before a positional mutation target.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
if (isPowerShellParameterToken(raw)) {
const key = raw.toLowerCase();
const next = normalizePathToken(args[i + 1] ?? "");
if (next.length > 0 && !isPowerShellParameterToken(next) && !POWERSHELL_PUNCTUATION_TOKENS.has(next)) {
values.push(next);
arul28
force-pushed
the
cursor/windows-port-foundations-ede6
branch
from
5eed441 to
ddecf19
Compare
|
@copilot review but do not make fixes |
| "Live missions, chats, terminals, or run processes must stop first.", | ||
| crdtSyncAvailable | ||
| ? "Live missions, chats, terminals, or run processes must stop first." | ||
| : "Install a Windows cr-sqlite runtime before pairing or syncing devices.", |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
These new status strings say the desktop cannot pair or sync when CRDT support is unavailable, but the guard is only applied to host startup and PIN management. ts // apps/desktop/src/main/services/sync/syncService.ts blockingStateText: crdtSyncAvailable ? "Live missions, chats, terminals, or run processes must stop first." : "Install a Windows cr-sqlite runtime before pairing or syncing devices.", connectToBrain() and transferBrainToLocal() still run without checking isCrdtSyncAvailable(), SyncDevicesSection still enables host transfer from status.transferReadiness.ready, and once connected the peer layer keeps calling db.sync.exportChangesSince() / applyChanges() even though kvDb turns both into empty/no-op results when hasCrsqlite is false. The result is a Windows viewer that can appear connected or take over hosting while replication silently does nothing. Apply the CRDT-availability guard symmetrically to viewer connect/transfer paths (or make transferReadiness.ready false when CRDT is unavailable) so the action fails fast instead of entering a non-working sync state.
|
|
||
| function installTargetPath(): string { | ||
| if (process.platform === "win32") { | ||
| const localAppData = process.env.LOCALAPPDATA?.trim() || path.join(os.homedir(), "AppData", "Local"); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
// apps/desktop/src/main/services/cli/adeCliService.ts
if (process.platform === "win32") {
const localAppData = process.env.LOCALAPPDATA?.trim() || path.join(os.homedir(), "AppData", "Local");
return path.join(localAppData, "ADE", "bin", "ade.cmd");
}When LOCALAPPDATA is unset, this service reports and validates ...\AppData\Local\ADE\bin\ade.cmd, but @apps/desktop/scripts/ade-cli-install-path.cmd falls back to %USERPROFILE%\.ade\bin\ade.cmd in the same situation. That means installForUser() can succeed while getStatus() still points the settings UI and post-install PATH hint at a different location, so the Windows Terminal install flow remains visibly broken for users running without LOCALAPPDATA. Make both code paths share the same fallback target (or pass the explicit target path into the installer).
| const resolveOn = options?.resolveOn ?? "spawn"; | ||
| try { | ||
| const child = spawn(command, args, { detached: true, stdio: "ignore" }); | ||
| const child = spawn(command, args, { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
On Windows this new editor-launch path shells through cmd.exe /d /s /c start ..., but the detached spawn() call does not set windowsHide, so Node uses its default (false) and the helper shell can visibly flash a console window every time the user opens a file in VS Code/Cursor/Zed from ADE. This is a deterministic user-facing regression introduced by the Windows portability path, and nearby Windows helper spawns in this repo already opt into hidden windows for the same reason. Set windowsHide: true (or gate it on process.platform === "win32") in launchDetached() so the shell wrapper stays background-only.
// apps/desktop/src/main/services/ipc/registerIpc.ts
const child = spawn(command, args, {
detached: true,
stdio: "ignore",
windowsVerbatimArguments: options?.windowsVerbatimArguments,
});| broadcastData: () => {}, | ||
| broadcastExit: () => {}, | ||
| onSessionEnded: () => {}, | ||
| getAdeCliAgentEnv: createHeadlessAdeCliAgentEnv, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This change adds ADE CLI PATH augmentation to headless PTY launches, but it still never passes getLaneRuntimeEnv into either createPtyService or the newly-added createProcessService:
// apps/ade-cli/src/bootstrap.ts
const ptyService = createPtyService({
...
getAdeCliAgentEnv: createHeadlessAdeCliAgentEnv,
loadPty: () => nodePty
});In the desktop app, both services receive getLaneRuntimeEnv, and processService.startByDefinition() explicitly reads that hook before launching a PTY-backed command. In headless mode the hook stays undefined, so lane-scoped vars like PORT, PORT_RANGE_*, and HOSTNAME are never injected when ade --headless starts a session or managed process in a lane. That breaks lane-runtime parity and causes processes to bind to default ports/hostnames instead of the lane allocation. Add a headless getLaneRuntimeEnv provider and pass it to both service constructors.
| env: childEnv, | ||
| shell: false, | ||
| stdio: [spec.stdinFilePath ? "pipe" : "inherit", "inherit", "inherit"], | ||
| windowsHide: false, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new Node-based worker launcher still spawns the inner Claude/Codex process with windowsHide: false, so packaged Windows builds will show a transient console window for every orchestrated worker launch instead of keeping the helper process hidden. That regresses the Windows UX this refactor is meant to enable, and it is isolated to the newly added launcher path. Set windowsHide to true on the inner spawn so the launcher stays behaviorally consistent with the rest of ADE's Windows subprocess handling.
// apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts
shell: false,
stdio: [spec.stdinFilePath ? "pipe" : "inherit", "inherit", "inherit"],
windowsHide: false,
windowsVerbatimArguments: !!spec.windowsVerbatimArguments
});| windowsHide: false, | |
| windowsHide: true, |
| } catch { | ||
| // Best-effort only. | ||
| } | ||
| terminateProcessTree(child, "SIGTERM"); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
On Windows, terminateProcessTree(child, "SIGTERM") already delegates to taskkill /T /F, so the first timeout step is not graceful anymore — it is the final force-kill. The follow-up timer still treats the child as killable because taskkill does not set child.killed, only child.kill() does. That means these runners can issue a second PID-based kill before 'close' has updated exitCode, which is unsafe once the original process has exited because PIDs can be reassigned. This pattern is present here and in the external-resolver timeout path at @apps/desktop/src/main/services/conflicts/conflictService.ts. Fix by treating a successful Windows tree kill as terminal (skip the second timer / track your own killRequested flag instead of relying on child.killed).
// apps/desktop/src/main/services/orchestrator/orchestratorContext.ts
terminateProcessTree(child, "SIGTERM");
setTimeout(() => {
if (child.exitCode == null && !child.killed) {
terminateProcessTree(child, "SIGKILL");
}
}, 1_000).unref();| NO_COLOR: "1", | ||
| TERM: "dumb" | ||
| }; | ||
| const invocation = resolveCliSpawnInvocation(codexExecutable, cliArgs, env); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
runCodexExec() always appends the planner prompt as a single argv entry, and that prompt is built with embedded \n lines by buildPlannerPrompt(). After this change, Windows shim installs are routed through cmd.exe /d /s /c, but cmd.exe does not preserve raw CR/LF inside a quoted command string, so a normal npm-installed codex.cmd launch will split the prompt at newlines and misparse/fail instead of delivering the full prompt to Codex. That makes the automation planner path fail on a standard Windows Codex install. Feed multiline prompt text through stdin/a temp file (or apply cmd-aware newline escaping) instead of passing it as a raw argv element through the cmd.exe wrapper.
// apps/desktop/src/main/services/automations/automationPlannerService.ts
cliArgs.push(args.prompt);
...
const invocation = resolveCliSpawnInvocation(codexExecutable, cliArgs, env);
const child = spawn(invocation.command, invocation.args, {| child.on("close", (code) => { | ||
| finish(() => resolve({ stdout, stderr, exitCode: code })); | ||
| }); | ||
| child.stdin?.on("error", (error) => { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new JSON-RPC path rejects immediately on any child.stdin error, which means a post-write EPIPE now wins over the close handler and throws away the collected stdout/exitCode. If Codex closes stdin immediately after consuming the three JSONL requests, Node can emit EPIPE even though the child has already produced the rate-limit response; in that case this code reports codex: CLI RPC error: EPIPE and never parses the returned windows. The new test at usageTrackingService.test.ts:542 codifies that failure mode, so this is an introduced regression rather than a hypothetical race. Treat post-write EPIPE as best-effort cleanup and let close/exitCode decide success.
// apps/desktop/src/main/services/usage/usageTrackingService.ts
child.stdin?.on("error", (error) => {
logger.warn("usage.poll.codex_cli_rpc_stdin_failed", {
error: getErrorMessage(error),
});
finish(() => reject(error));
});| continue; | ||
| } | ||
| if (looksLikePathToken(value)) { | ||
| if (looksLikePathToken(value, windowsMode)) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The first token pass now runs in Windows mode, but it still feeds every token through looksLikePathToken() before excluding /c, /d, /s, and similar slash-style switches. Because looksLikePathToken() treats any token containing / as a path, a benign wrapper like cmd.exe /c type .\\README.md adds /c as a fake path and gets rejected as Path outside sandbox, which breaks the newly-added nested cmd.exe support for read-only commands. ```ts
// apps/desktop/src/main/services/ai/tools/universalTools.ts
if (value.startsWith("-")) continue;
if (value === "|" || value === "||" || value === "&&" || value === ";" || value === "&") continue;
...
if (looksLikePathToken(value, windowsMode)) {
addPath(value, "unknown");
}
Skip Windows `/[a-z?]` switches in this raw-token loop the same way `markOperands()` already does, so wrapper flags are not mistaken for filesystem paths.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/main/services/ai/tools/workflowTools.ts (1)
161-184:⚠️ Potential issue | 🟡 MinorArgs and backend name are darwin-only by design; consider parameterizing them if adding non-darwin support.
The current code is safe:
localComputerUse.tsexplicitly blocks non-darwin platforms (line 44) by setting all capabilities toavailable: false, so line 174 is only reachable on macOS wherecommandwill always be"screencapture". However, if non-darwin screenshot support is added in the future (e.g., Windows, Linux), the hardcoded args["-x", tmpPath](macOS-specific flag) andbackend.name: "screencapture"would need updating.To future-proof this:
- Extend
LocalComputerUseCapabilityto includeargs?: string[]andbackend?: stringfields, or- Add a platform check at line 174 to branch args/backend per platform if adding non-darwin support
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/workflowTools.ts` around lines 161 - 184, The code assumes macOS-only screencapture args and backend name; update getLocalComputerUseCapabilities / LocalComputerUseCapability to expose args?: string[] and backendName?: string (or similar) so capabilities.screenshot includes the platform-specific exec args and backend identifier, then use capabilities.screenshot.args (fallback to ["-x", tmpPath] for existing macOS) and capabilities.screenshot.backendName (fallback to "screencapture") when calling execFileAsync and computerUseArtifactBrokerService.ingest; alternatively, if you prefer not to change the capability shape, add an explicit platform branch around the execFileAsync call and the ingest backend.name assignment that selects args and backend name per platform (using getLocalComputerUseCapabilities(), capabilities.platform, and capabilities.screenshot.command to locate the correct logic).apps/desktop/src/main/services/automations/automationService.ts (1)
1327-1355:⚠️ Potential issue | 🟠 MajorUse
terminateProcessTreeto kill the full command tree on timeout.On line 1327 the code routes Windows commands through
cmd.exe /d /s /c, but line 1350 still callschild.kill("SIGKILL"). That only terminates the shell wrapper process; the actual command survives timeout and continues running in the background. Switch toterminateProcessTreewhich usestaskkill.exe /T /Fon Windows to kill the entire process tree.🛠️ Proposed fix
import { randomUUID } from "node:crypto"; import { spawn } from "node:child_process"; +import { terminateProcessTree } from "../shared/processExecution"; @@ const timer = setTimeout(() => { - try { - child.kill("SIGKILL"); - } catch { - // ignore - } + terminateProcessTree(child, "SIGKILL"); settled = true; reject(new Error(`Command timed out after ${timeoutMs}ms`)); }, timeoutMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/automations/automationService.ts` around lines 1327 - 1355, The timeout handler currently calls child.kill("SIGKILL") which only kills the shell wrapper and leaves the real command running; replace that call with the project's terminateProcessTree utility to kill the whole process tree (e.g., call terminateProcessTree(child.pid) and await it or call it in the try block), ensure you import/require terminateProcessTree at the top of the file, and wrap the call in try/catch as the existing code does so you still set settled = true and reject with the timeout Error if termination fails; locate this change around the Promise timeout code that references child, spawn, and the timeoutMs variable.apps/desktop/src/main/services/ai/cliExecutableResolver.test.ts (1)
53-108:⚠️ Potential issue | 🟠 MajorPin the POSIX-path cases to POSIX semantics.
These three tests still inherit the host platform, so on a native Windows run they'll take the win32 branch and the
/usr/.../ Homebrew expectations can fail spuriously. Since this PR is adding Windows coverage, please force a POSIX platform and:delimiter in these cases.Suggested fix
it("discovers codex from an npm prefix configured in ~/.npmrc", () => { + setPlatform("darwin"); + setPathDelimiter(":"); tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-cli-path-")); const homeDir = path.join(tempRoot, "home"); const prefixDir = path.join(homeDir, ".npm-global"); @@ it("augments PATH with npm-global bins discovered from ~/.npmrc", () => { + setPlatform("darwin"); + setPathDelimiter(":"); tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-cli-path-")); const homeDir = path.join(tempRoot, "home"); @@ it("keeps both Intel and Apple Silicon Homebrew bins on PATH", () => { + setPlatform("darwin"); + setPathDelimiter(":"); const nextPath = augmentPathWithKnownCliDirs("/usr/local/bin:/usr/bin:/bin", { HOME: "/tmp/ade-home", PATH: "/usr/local/bin:/usr/bin:/bin",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/cliExecutableResolver.test.ts` around lines 53 - 108, The tests assume POSIX path semantics and fail on Windows; force POSIX behavior by using path.posix when constructing expected paths and by stubbing currentPathDelimiter() to return ':' for these test cases. Update the three tests that call resolveExecutableFromKnownLocations and augmentPathWithKnownCliDirs to build paths with path.posix.join/path.posix.normalize and, where the test checks split(currentPathDelimiter()), mock or spyOn currentPathDelimiter to return ':' so PATH splitting and expected entries are evaluated with POSIX semantics.apps/ade-cli/src/cli.ts (1)
1829-1831:⚠️ Potential issue | 🟡 MinorMake the PATH remediation hint platform-aware.
On Windows this still tells users to run
install-path.sh, which is the wrong recovery path for the exact scenario this PR is fixing. Please return Windows-specific install-path guidance here instead of the Unix shell script hint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` around lines 1829 - 1831, The current nextAction message uses a Unix-specific hint; update the logic that sets nextAction (the ternary using onPath in apps/ade-cli/src/cli.ts) to choose a platform-aware string: when on Windows (process.platform === 'win32') return a Windows-specific remediation (e.g., instruct running npm link in apps/ade-cli or running the packaged install-path.bat / PowerShell instructions to add the CLI to PATH), otherwise keep the existing install-path.sh hint for Unix. Ensure you modify the nextAction assignment that references onPath so it selects the appropriate message based on platform.
♻️ Duplicate comments (9)
apps/desktop/src/main/services/sync/syncService.ts (1)
555-569:⚠️ Potential issue | 🟠 MajorViewer-side connection paths are still not gated on CRDT availability.
connectToBrain()(and the saved-draft reconnect inrefreshRoleState) callsyncPeerService.connect(draft)without consultingisCrdtSyncAvailable(). When CR-SQLite is unavailable,db.sync.applyChangesis a no-op andgetDbVersion()stays0, so a viewer on an unsupported platform can show as "connected" while no remote changes will ever land locally and no local edits will ever be exported. Reject viewer connections (both manual and saved-draft auto-reconnect) when!isCrdtSyncAvailable()so the new "sync unavailable" state is enforced consistently with the host-side gate at Line 402.🔒 Suggested guard locations
async connectToBrain( draft: SyncDesktopConnectionDraft, ): Promise<SyncRoleSnapshot> { + if (!isCrdtSyncAvailable()) { + throw new Error( + "Desktop sync is unavailable because the CRDT database extension is unavailable on this platform.", + ); + } await stopHostIfRunning();} else { await stopHostIfRunning(); const draft = savedDraft ?? resolveViewerDraftFromRegistry(); - if (draft && !syncPeerService.isConnected()) { + if (draft && !syncPeerService.isConnected() && isCrdtSyncAvailable()) { syncPeerService.setSavedDraft(draft);Also applies to: 771-791
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncService.ts` around lines 555 - 569, Both manual viewer connects (connectToBrain) and the saved-draft auto-reconnect path in refreshRoleState currently call syncPeerService.connect(draft) without checking CRDT availability; add a guard using isCrdtSyncAvailable() in both places and refuse the viewer connect when it returns false. Concretely, before calling syncPeerService.setSavedDraft(...) / syncPeerService.connect(draft) in refreshRoleState and before invoking syncPeerService.connect in connectToBrain, check if (!isCrdtSyncAvailable()) then log/warn the appropriate "sync unavailable" message and skip setting draft/connecting so the viewer state is not marked connected.apps/desktop/src/main/services/ai/providerTaskRunner.ts (1)
109-114:⚠️ Potential issue | 🟠 MajorStdin
errorhandler still rejects eagerly — EPIPE on early child exit will mask the real exit code.This path settles the promise on any
stdinerror, including theEPIPE/ERR_STREAM_DESTROYEDthat Node emits when Claude/Codex exits before finishing stdin consumption. When that happens,closeis skipped andrunProviderTaskthrows a pipe error instead ofClaude exited with code N\n\n<stderr>. Prefer lettingcloseown resolution for early-exit cases and only reject on genuine setup failures.🐛 Proposed fix
- child.stdin?.on("error", (error) => { - if (settled) return; - settled = true; - clearTimeout(timeoutHandle); - reject(error); - }); + child.stdin?.on("error", (error) => { + // Swallow EPIPE / ERR_STREAM_DESTROYED — the child likely exited before + // consuming stdin, and the `close` handler will surface the real exit + // code + stderr. Only fail hard on other, unexpected stdin errors. + const code = (error as NodeJS.ErrnoException)?.code; + if (code === "EPIPE" || code === "ERR_STREAM_DESTROYED") return; + if (settled) return; + settled = true; + clearTimeout(timeoutHandle); + reject(error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/providerTaskRunner.ts` around lines 109 - 114, The stdin error handler in runProviderTask (child.stdin?.on("error", ...)) currently rejects on any error and sets settled/clears timeout, which causes EPIPE/ERR_STREAM_DESTROYED from an early child exit to mask the real exit handled in the child's "close" path; change the handler to ignore transient stream-destroy errors by returning early when error.code is "EPIPE" or "ERR_STREAM_DESTROYED" (do not clear timeout or set settled), and only clearTimeout/set settled and reject for genuine setup failures (other error codes) so the existing "close" logic owns resolution on early-exit cases.apps/ade-cli/src/adeRpcServer.ts (1)
5909-5967:⚠️ Potential issue | 🟠 MajorWindows unresolved-provider fallback still uses POSIX quoting.
If
resolveExecutableOnPath(provider)returnsnull, Lines 5963-5967 still rely onstartupCommand, but the pieces feeding that string were quoted withshellEscapeArg()on Lines 5912-5942. Prompts with apostrophes or other shell-sensitive characters will still become POSIX fragments that PowerShell/cmd won't execute correctly, so the Windows fallback path remains broken for unresolved-but-shell-launchable providers. Either build a Windows-shell command here or avoid shell auto-start on Windows when resolution fails.#!/bin/bash set -euo pipefail printf '\n# spawn_agent command/startupCommand construction\n' sed -n '5909,5968p' apps/ade-cli/src/adeRpcServer.ts printf '\n# PTY Windows startupCommand handling\n' rg -n -C3 'startupCommand|powershell\.exe|cmd\.exe|shellEscapeArg\(' \ apps/ade-cli/src/adeRpcServer.ts \ apps/desktop/src/main/services/pty/ptyService.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/adeRpcServer.ts` around lines 5909 - 5967, The startupCommand is built using shellEscapeArg (used when pushing into commandPreviewParts) but is later used for a Windows fallback when resolveExecutableOnPath(provider) returns null, producing POSIX-quoted fragments that won’t run in cmd/powershell; update the logic around resolveExecutableOnPath, startupCommand and the process.platform === "win32" branch so that when providerExecutable === null on Windows you do not attempt to use the POSIX-quoted startupCommand — either construct a proper Windows-safe command string (using Windows quoting/escaping) or, simpler and safer, refuse/skip auto-start on Windows and surface an error/require the caller to resolve the executable; modify code paths referencing startupCommand and providerExecutable (and places using shellEscapeArg/addWorkerEnv) to enforce this behavior.apps/desktop/src/main/services/ai/tools/universalTools.ts (3)
726-749:⚠️ Potential issue | 🔴 Critical
cmd.exe /cstill hides non-PowerShell mutations.The
/cpayload is only re-routed throughinspectPowerShellInvocations(). For commands likecmd.exe /c "copy foo ..\outside.txt"orcmd.exe /c "del .env", that recursive call records no generic path refs, so the sandbox never sees the real write/delete target. Recurse into the generic command/path collector for/cpayloads too, not just the PowerShell inspector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/universalTools.ts` around lines 726 - 749, The cmd.exe /c branch only forwards the payload to inspectPowerShellInvocations, which misses generic file operations; update the handler in the isCmdHostCommand block so that when commandArgIndex >= 0 you also run the generic command/path collector (the same collector used for non-cmd branches — e.g., the function that produces path refs for plain shell commands) on nestedCommand, merge its result via mergeInspection (or equivalent), and combine its mutates/pathRefs with nestedInspection (and respect blockedReason) before continuing; ensure you still call inspectPowerShellInvocations for PowerShell-specific checks but also call and merge the generic inspection so copy/del style mutations are detected.
764-801:⚠️ Potential issue | 🔴 CriticalHandle bare
pwsh script.ps1entrypoints.This branch only inspects
-Command,-EncodedCommand, and-File. A valid invocation likepwsh .\worker.ps1leavesscriptnull, so the sandbox never reads or validates the script contents. Treat the first non-flag.ps1argument as a script file, or fail closed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/universalTools.ts` around lines 764 - 801, The loop over args currently only handles -Command, -EncodedCommand and -File flags so a bare invocation like "pwsh script.ps1" leaves script null and skips inspection; fix by adding a branch inside that loop (near the checks using POWERSHELL_COMMAND_FLAGS, POWERSHELL_ENCODED_COMMAND_FLAGS, POWERSHELL_FILE_FLAGS and normalizePathToken) that treats the first non-flag argument ending with ".ps1" (allowing relative prefixes like "./" or ".\\") as a script file: call inspectScriptFile(scriptPath), mergeInspection(scriptFileInspection), update mutates from scriptFileInspection.mutates, handle scriptFileInspection.blockedReason by returning { ...scriptFileInspection, pathRefs: [...refs.values()] } and set script = null/break to prevent continuing; alternatively if you prefer fail-closed, return a blockedReason when an apparent .ps1 argument is present but inspectScriptFile fails to produce a valid inspection.
535-555:⚠️ Potential issue | 🔴 CriticalValidate every PowerShell target, not just the first one.
addFirstNamedOrPositionalPath()stops aftervalues[0]or a single positional operand. Commands likeRemove-Item .\ok.txt ..\outside.txtorCopy-Item a,b ..\destcan still hide a later out-of-root/protected target because only the first path is checked. Enumerate all mutating path operands, or fail closed when more than one target is supplied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/universalTools.ts` around lines 535 - 555, addFirstNamedOrPositionalPath currently only checks the first named value (values[0]) or a single positional, letting later targets escape validation; change it to validate every path target by iterating all entries in parsed.named.get(flag) and all parsed.positional entries starting at positionalIndex (for each named/positional entry also split on commas and trim to handle comma-separated lists), call addResolvedPath for each target and if any call returns a blockedReason return it immediately; if no targets are found and access !== "read" and failOnMissing is true keep returning the same blockedReason; reference addFirstNamedOrPositionalPath, parsePowerShellArgs, parsed.named.get, parsed.positional and addResolvedPath when making the change.apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts (1)
101-144:⚠️ Potential issue | 🟠 MajorPropagate wrapper shutdown to the spawned worker.
This launcher only watches the child’s
error/exit. If the Node wrapper itself is terminated, the real Claude/Codex process can keep running in the background, which breaks cancel/close flows and can leave a worker alive after the PTY/session is gone. Please trap wrapper shutdown and terminate the child process tree before exiting.Suggested fix
-const { spawn } = require("child_process"); +const { spawn, spawnSync } = require("child_process"); ... const child = spawn(spec.command, Array.isArray(spec.args) ? spec.args : [], { cwd: spec.cwd || process.cwd(), env: childEnv, shell: false, stdio: [spec.stdinFilePath ? "pipe" : "inherit", "inherit", "inherit"], windowsHide: false, windowsVerbatimArguments: !!spec.windowsVerbatimArguments }); +function terminateChild() { + if (!child || child.exitCode !== null || child.signalCode !== null) return; + try { + if (process.platform === "win32" && child.pid) { + spawnSync("taskkill.exe", ["/T", "/F", "/PID", String(child.pid)], { windowsHide: true }); + return; + } + child.kill("SIGTERM"); + } catch {} +} +for (const signal of ["SIGINT", "SIGTERM", "SIGHUP"]) { + process.on(signal, () => { + terminateChild(); + finish(1); + }); +} +process.on("exit", terminateChild); child.on("error", (err) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts` around lines 101 - 144, The launcher string (WORKER_CLI_LAUNCHER_SCRIPT) currently only listens to child "error"/"exit" and can leave the spawned child running if the wrapper exits; add shutdown handlers in the script to propagate wrapper termination to the spawned process: register listeners on process events (at least 'exit', 'SIGINT', 'SIGTERM', 'SIGQUIT' and fallback for 'uncaughtException'/'unhandledRejection') and in each handler call a cleanup that attempts to kill the child process or child process tree (use child.kill() on POSIX and a Windows fallback such as spawning "taskkill /PID <pid> /T /F" when process.platform === "win32"), ensure the cleanup sets a flag to avoid reentrancy and then calls finish(...) to exit the wrapper; refer to the child variable, finish function, and WORKER_CLI_LAUNCHER_SCRIPT to locate where to inject these handlers.apps/desktop/src/main/services/opencode/openCodeServerManager.ts (1)
255-260:⚠️ Potential issue | 🟠 MajorFallback to PowerShell when WMIC omits
CommandLine.This still treats any non-empty WMIC result set as authoritative.
parseWindowsWmicProcessCsv()keeps rows even whencommandis blank, so orphan recovery never sees the ADE markers for those processes and stale managedopencode servetrees can survive restarts. Please fall back whenever WMIC returns rows with missing command text, not only when it returns zero rows.Suggested fix
function listWindowsProcesses(): OpenCodeProcessSnapshot[] { const fromWmic = listWindowsProcessesFromWmic(); - if (fromWmic.length > 0) { - return fromWmic; + if (fromWmic.length > 0 && fromWmic.every((row) => row.command.trim().length > 0)) { + return fromWmic; } - return listWindowsProcessesFromPowerShell(); + const fromPowerShell = listWindowsProcessesFromPowerShell(); + return fromPowerShell.length > 0 ? fromPowerShell : fromWmic; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/opencode/openCodeServerManager.ts` around lines 255 - 260, The current listWindowsProcesses() returns the WMIC results if listWindowsProcessesFromWmic() yields any rows even when some rows have an empty command/CommandLine, which prevents orphan recovery; change listWindowsProcesses() to inspect the array returned by listWindowsProcessesFromWmic() (or by parseWindowsWmicProcessCsv()) and if any snapshot is missing the command text (e.g., snapshot.command or snapshot.CommandLine is falsy/empty) then ignore WMIC and return listWindowsProcessesFromPowerShell() instead; ensure you reference listWindowsProcessesFromWmic, parseWindowsWmicProcessCsv, and listWindowsProcessesFromPowerShell when implementing the check so WMIC is only treated as authoritative when all returned snapshots include non-empty command text.apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx (1)
459-459:⚠️ Potential issue | 🟠 MajorUNC
file://host/share/...URIs still lose their authority.
trimmed.replace(/^file:\/\//i, "")strips the scheme and the authority, sofile://server/share/pathbecomesserver/share/path. Downstream it no longer starts with/and isn't Windows-absolute, solooksLikeWorkspacePathaccepts it as a relative path andresolveFilesNavigationTargetfalls through to the relative branch (Line 598), mis-navigating within the current lane instead of into the UNC workspace — the exact Windows/UNC portability this PR is intended to land.Only the two-slash form (empty authority) should be collapsed; a non-empty authority needs to be preserved as a UNC root, e.g. via
URLor by conditionally reinserting//:🛠️ Suggested fix
- const withoutScheme = trimmed.replace(/^file:\/\//i, ""); + // Preserve UNC authority: file://server/share/path -> //server/share/path. + // file:///C:/... and file:///posix/... collapse to the local form as before. + const fileUriMatch = trimmed.match(/^file:\/\/([^/?#]*)(.*)$/i); + const withoutScheme = fileUriMatch + ? (fileUriMatch[1] ? `//${fileUriMatch[1]}${fileUriMatch[2]}` : fileUriMatch[2]) + : trimmed;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx` at line 459, The current replacement that builds withoutScheme from trimmed removes the entire authority for UNC paths (e.g. file://server/share), so preserve UNC authorities: change the logic that computes withoutScheme (used by looksLikeWorkspacePath and resolveFilesNavigationTarget) to only collapse an empty authority (file:/// -> remove leading file:///) but keep non-empty authorities (file://server/... -> retain //server/...) — implement this by parsing trimmed with the URL API or by detecting a host and reinserting the leading '//' when a host is present so UNC roots remain Windows-absolute and downstream resolveFilesNavigationTarget handles them correctly.
🧹 Nitpick comments (14)
apps/desktop/src/main/services/ai/tools/globSearch.test.ts (1)
68-78: LGTM — good Windows-separator coverage.The fixture/assertion pair is tight:
helper.tsmatches andhelper.mdis excluded, confirming both the\→/normalization and that the extension filter still applies after normalization.Optional: consider adding one more assertion for a mixed-separator pattern (e.g.
src\**/*.tsor**\*.ts) to lock in behavior for patterns that might be hand-edited or concatenated on Windows. Not required.🧪 Optional: add a mixed-separator case
it("accepts Windows-style separators in glob patterns", async () => { const cwd = makeTmpDir("glob-windows-pattern-"); writeFixtureFile(cwd, "src/lib/helper.ts", "export {}"); writeFixtureFile(cwd, "src/lib/helper.md", "# Hello"); const tool = createGlobSearchTool(cwd); const result = await tool.execute({ pattern: "src\\**\\*.ts" }); expect(result.count).toBe(1); expect(result.displayFiles).toEqual(["src/lib/helper.ts"]); + + const mixed = await tool.execute({ pattern: "src\\**/*.ts" }); + expect(mixed.count).toBe(1); + expect(mixed.displayFiles).toEqual(["src/lib/helper.ts"]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/tools/globSearch.test.ts` around lines 68 - 78, Add an additional assertion to the test to cover mixed Windows/Unix separators: call createGlobSearchTool(cwd) and invoke tool.execute with a mixed-separator pattern such as "src\\**/*.ts" (or "**\\*.ts"), then assert result.count === 1 and result.displayFiles equals ["src/lib/helper.ts"] to ensure normalization still works when patterns contain both "\" and "/"; update the existing "accepts Windows-style separators in glob patterns" test or add a new it block that uses createGlobSearchTool and tool.execute to verify the mixed-separator behavior.apps/desktop/scripts/after-pack-runtime-fixes.cjs (1)
47-62: Minor: Linux branch asserts files but neverchmods theadewrapper.No Linux target is defined in
build, so this is a latent concern, but if a Linux dist is ever enabled the packagedade-cli/bin/adewill ship non-executable (unlike the darwin branch). Consider either restricting theelsebranch to throw-unsupported until Linux is a real target, or applying the samechmodSync(..., 0o755)there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/after-pack-runtime-fixes.cjs` around lines 47 - 62, The Linux/else branch currently calls requireFile for the bundled ADE files but doesn’t mark the POSIX wrapper executable, so ade-cli/bin/ade will be non-executable on Linux; update the else branch in after-pack-runtime-fixes.cjs (the platform === "darwin" code uses fs.chmodSync(bundledCliBinPath, 0o755) and chmodSync for the installer) to likewise call fs.chmodSync on the POSIX paths (path.join(resourcesRoot, "ade-cli", "bin", "ade") and path.join(resourcesRoot, "ade-cli", "install-path.sh")) or, if Linux is not supported, replace the else branch with an explicit unsupported-platform throw to avoid shipping non-executable binaries; adjust code around requireFile, fs.chmodSync, and the platform conditional accordingly.apps/desktop/scripts/ade-cli-windows-wrapper.cmd (1)
10-17: Nit: dead branch —NODE_PATH_VALUEis always defined at this point.Line 10 unconditionally sets
NODE_PATH_VALUE, so the innerif defined NODE_PATH_VALUEon line 12 is always true and theelseon lines 14‑16 is unreachable. Collapse to a single append:♻️ Simplification
set "NODE_PATH_VALUE=%RESOURCES_DIR%\app.asar.unpacked\node_modules;%RESOURCES_DIR%\app.asar\node_modules" -if defined NODE_PATH ( - if defined NODE_PATH_VALUE ( - set "NODE_PATH_VALUE=%NODE_PATH_VALUE%;%NODE_PATH%" - ) else ( - set "NODE_PATH_VALUE=%NODE_PATH%" - ) -) +if defined NODE_PATH set "NODE_PATH_VALUE=%NODE_PATH_VALUE%;%NODE_PATH%"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/ade-cli-windows-wrapper.cmd` around lines 10 - 17, The branch using NODE_PATH_VALUE is dead because NODE_PATH_VALUE is always set earlier; simplify the if block by removing the inner conditional and, inside if defined NODE_PATH, just append NODE_PATH onto the existing NODE_PATH_VALUE (i.e., update NODE_PATH_VALUE by concatenating its current value and %NODE_PATH%). Refer to the symbols NODE_PATH_VALUE and NODE_PATH to locate and change the logic in the script.apps/desktop/src/main/services/cli/windowsPackaging.test.ts (1)
11-20: Test coverage gap: wrapper tests are content-only.These assertions grep for substrings (e.g.
call :run_with_runtime_env …) but don't exercise the wrapper end-to-end, so they won't catch behavioral regressions like exit-code propagation. If feasible on thewindows-latestrunner, consider adding a test that invokes the.cmdwithADE_CLI_NODEpointing at a stub script thatexit /b 42and asserts the wrapper returns 42 — this is exactly the kind of issue flagged inade-cli-windows-wrapper.cmd.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/cli/windowsPackaging.test.ts` around lines 11 - 20, The current tests only assert string contents of the ADE Windows wrapper; add an end-to-end test that actually executes the ade-cli-windows-wrapper.cmd to verify exit-code propagation: create a temporary stub Windows script (e.g., a .cmd that does exit /b 42), set the ADE_CLI_NODE environment variable to point at that stub, spawn the wrapper (use the same wrapperPath/ade-cli-windows-wrapper.cmd used in windowsPackaging.test.ts), capture its exit code, and assert it equals 42; run this test conditionally on windows-latest CI (or skip on non-Windows) so it only runs where cmd execution is supported.apps/desktop/scripts/validate-win-artifacts.mjs (2)
363-373: Rename/restore ofappExecan leave the packaged app broken if the script is interrupted.If this process is killed (CI timeout, Ctrl+C, crash) between the
rename(appExe → .bak)on line 364 and thefinallyrename back on line 372,win-unpacked/<ProductName>.exewill be left missing until a human cleans it up, which also means any later re-run of this validator (or subsequent pipeline step that expects the packaged exe) will fail with a confusing "missing packaged Windows app executable" error fromassertPathExistson line 288.Consider either (a) installing a SIGINT/SIGTERM handler that restores the
.bakbefore propagating, (b) detecting a leftover*.exe.bakat the start ofvalidatePackagedRuntimeand restoring it, or (c) exercising the plain-node fallback in a temp copy of the unpacked app rather than mutating the release output in place.♻️ Optional: self-healing leftover handling before the fallback test
const disabledAppExe = `${appExe}.bak`; + // Recover from a previous interrupted run before we rename again. + if (fs.existsSync(disabledAppExe) && !fs.existsSync(appExe)) { + await fsp.rename(disabledAppExe, appExe); + } await fsp.rename(appExe, disabledAppExe); - try { + const restore = async () => { + if (fs.existsSync(disabledAppExe)) { + await fsp.rename(disabledAppExe, appExe); + } + }; + const onSignal = () => { restore().finally(() => process.exit(1)); }; + process.once("SIGINT", onSignal); + process.once("SIGTERM", onSignal); + try { const plainNodeHelp = await runCommand(adeCliBinPath, ["--help"], { cwd: resourcesPath, env: { ...process.env }, }); assertAdeCliHelp(plainNodeHelp.stdout, "Bundled ADE CLI wrapper with plain node fallback"); } finally { - await fsp.rename(disabledAppExe, appExe); + process.off("SIGINT", onSignal); + process.off("SIGTERM", onSignal); + await restore(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/validate-win-artifacts.mjs` around lines 363 - 373, The rename/restore around appExe (variables appExe and disabledAppExe in validatePackagedRuntime) can leave the packaged exe missing if the process is interrupted; to fix, add a recovery step at the start of validatePackagedRuntime that checks for an existing `${appExe}.bak` (or pattern `*.exe.bak`) and renames it back to the original appExe before proceeding, and additionally wrap the temporary disable/restore block with a SIGINT/SIGTERM handler that restores disabledAppExe to appExe before rethrowing so interruptions don't leave the unpacked app mutated; reference the rename calls around appExe/disabledAppExe and the runCommand/assertAdeCliHelp block to ensure the restore logic runs both at startup and on signals.
336-346: Fix fail-closed state check forclaudeStartup.state—success state is"ready", not"ok", and"auth-failed"must also fail validation.Lines 339–343 only branch on
"binary-missing"(warn) and"runtime-failed"(fail). This leaves"ready"(success) and"auth-failed"(authentication failure) unhandled, both silently passing validation. SincepackagedRuntimeSmoke.tsalways returns one of four states ("ready","auth-failed","binary-missing","runtime-failed"), the validator should explicitly assert the expected success state and fail on anything else, including authentication failures.♻️ Fail-closed state check
if (payload.claudeStartup.state === "binary-missing") { console.warn("[validate-win-artifacts] Claude CLI is not installed on this machine; skipping live Claude startup check."); } else if (payload.claudeStartup.state === "runtime-failed") { fail(`Packaged smoke could not start Claude from the packaged app: ${String(payload.claudeStartup.message || "unknown error")}`); + } else if (payload.claudeStartup.state !== "ready") { + fail(`Packaged smoke reported an unexpected Claude startup state: ${String(payload.claudeStartup.state)}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/validate-win-artifacts.mjs` around lines 336 - 346, The validator currently only handles "binary-missing" and "runtime-failed" for payload.claudeStartup.state; update the logic to explicitly treat "ready" as the only success state and call fail(...) for "auth-failed" and any other unexpected state—keep the existing console.warn for "binary-missing", and for failing cases include payload.claudeStartup.message in the failure text; refer to payload.claudeStartup.state and the fail(...) helper when making these checks so packagedRuntimeSmoke's four states ("ready", "auth-failed", "binary-missing", "runtime-failed") are all handled explicitly.apps/desktop/src/main/services/shared/processExecution.ts (1)
86-86: Consider resolvingtaskkill.exevia%SystemRoot%\System32rather than PATH.In hardened/packaged Electron environments,
PATHcan be stripped down andtaskkill.exemay not resolve. Using the absolute path avoids PATH-shadowing or DLL-search issues and is a common hardening pattern for calling Windows system utilities from Electron main.♻️ Proposed refactor
- const out = spawnSync("taskkill.exe", ["/T", "/F", "/PID", String(pid)], { windowsHide: true }); + const systemRoot = process.env.SystemRoot || process.env.windir || "C:\\Windows"; + const taskkill = path.win32.join(systemRoot, "System32", "taskkill.exe"); + const out = spawnSync(taskkill, ["/T", "/F", "/PID", String(pid)], { windowsHide: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/shared/processExecution.ts` at line 86, The spawnSync call that invokes "taskkill.exe" (see the call to spawnSync in processExecution.ts) can fail in hardened Electron environments where PATH is stripped; change the command to resolve the absolute Windows system executable (e.g. build the path from process.env.SystemRoot || "C:\\Windows" and append "\\System32\\taskkill.exe") and pass that absolute path as the first argument to spawnSync (keeping the same args and options) so taskkill is located reliably without depending on PATH.apps/desktop/src/main/services/pty/ptyService.test.ts (1)
450-482: Windows cmd.exe shim test: tight coupling to the invocation format.The assertion matches
/d /s /c "npm.cmd" "run" "dev"as a single verbatim string. That tightly couples the test to the current quoting/flag choice (e.g., ifresolveCliSpawnInvocationever omits the trailing quotes for bare tokens, or swaps/d /s /cordering, this test breaks even though behavior is still correct).Consider asserting structural invariants instead — e.g., first arg is
expect.stringContaining("cmd.exe"), second argexpect.stringContaining("npm.cmd")andexpect.stringContaining("run")— to keep the test resilient to minor formatting changes in the shim resolver. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.test.ts` around lines 450 - 482, The test "wraps direct Windows command shims through cmd.exe" is too tightly coupled to the exact cmd.exe invocation string; change the assertion on ptyLib.spawn to assert structural invariants instead of a verbatim second argument — e.g., verify the first arg contains the ComSpec path (expect.stringContaining("cmd.exe")), and assert the second arg uses expect.stringContaining for the command and each arg such as expect.stringContaining("npm.cmd") and expect.stringContaining("run") (while still keeping expect.any(Object) for the options), locating this change around the ptyService.create call and the ptyLib.spawn expectation in the test file.apps/desktop/src/main/services/tests/testService.ts (2)
299-310: Minor: env object is duplicated — consider factoring for DRY.The merged env is computed twice (once for
resolveCliSpawnInvocation, once forspawnoptions). Hoist into a local to avoid drift if one copy is later adjusted.♻️ Proposed tweak
- const invocation = resolveCliSpawnInvocation(suite.command[0]!, suite.command.slice(1), { - ...process.env, - ...suite.env, - ...(overlay.env ?? {}) - }); - const child: ChildProcessByStdio<null, Readable, Readable> = spawn(invocation.command, invocation.args, { - cwd, - env: { ...process.env, ...suite.env, ...(overlay.env ?? {}) }, - shell: false, - stdio: ["ignore", "pipe", "pipe"], - windowsVerbatimArguments: invocation.windowsVerbatimArguments, - }); + const spawnEnv = { ...process.env, ...suite.env, ...(overlay.env ?? {}) }; + const invocation = resolveCliSpawnInvocation(suite.command[0]!, suite.command.slice(1), spawnEnv); + const child: ChildProcessByStdio<null, Readable, Readable> = spawn(invocation.command, invocation.args, { + cwd, + env: spawnEnv, + shell: false, + stdio: ["ignore", "pipe", "pipe"], + windowsVerbatimArguments: invocation.windowsVerbatimArguments, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/tests/testService.ts` around lines 299 - 310, The merged environment object is built twice (once when calling resolveCliSpawnInvocation and again in the spawn options); to make this DRY, compute a single envMerged (e.g., const mergedEnv = { ...process.env, ...suite.env, ...(overlay.env ?? {}) }) and pass mergedEnv into resolveCliSpawnInvocation and into spawn's env option, keeping other options (cwd, shell, stdio, windowsVerbatimArguments) unchanged and still using the existing invocation, child, and suite identifiers.
480-496:disposeAllstill useschild.kill("SIGKILL")— inconsistent with new tree-termination.Timeout/stop paths now route SIGTERM/SIGKILL through
terminateProcessTree, but app-shutdowndisposeAllstill callsentry.child.kill("SIGKILL")directly. On Windows (and for Unix test runners that spawn grandchildren likenpm → node) this can leak descendants at shutdown, which is exactly the parity issue this PR is trying to close elsewhere.♻️ Proposed fix
for (const entry of activeRuns.values()) { if (entry.timeoutTimer) clearTimeout(entry.timeoutTimer); if (entry.killTimer) clearTimeout(entry.killTimer); try { - entry.child.kill("SIGKILL"); + terminateProcessTree(entry.child, "SIGKILL"); } catch { // ignore }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/tests/testService.ts` around lines 480 - 496, disposeAll currently calls entry.child.kill("SIGKILL") directly which bypasses the new tree-aware shutdown and can leak descendants; update disposeAll to call the tree-termination helper (terminateProcessTree) with the child (or child's pid) and the SIGKILL signal instead of entry.child.kill, and ensure terminateProcessTree is imported/available in this module so that activeRuns cleanup uses the same termination logic as the timeout/stop paths.apps/desktop/src/main/services/lanes/laneEnvironmentService.ts (1)
246-249: Timeout path has no SIGKILL fallback — a process that ignores SIGTERM will hangexecCommandforever.
testServiceescalates toSIGKILLafter a 3s grace window; here the timer only callsterminateProcessTree(child)(default SIGTERM on Unix). If the child masks/ignores SIGTERM,closenever fires and the returned promise never settles, leaving callers likestartDocker/installDependencies/cleanupLaneEnvironmentstuck. Consider mirroring the two-phase termination used elsewhere in the PR.♻️ Proposed fix
- const timer = setTimeout(() => { - if (settled) return; - terminateProcessTree(child); - }, timeoutMs); + let killTimer: NodeJS.Timeout | null = null; + const timer = setTimeout(() => { + if (settled) return; + terminateProcessTree(child, "SIGTERM"); + killTimer = setTimeout(() => { + if (settled) return; + terminateProcessTree(child, "SIGKILL"); + }, 3000); + }, timeoutMs);Make sure to also
clearTimeout(killTimer)inside theerror/closehandlers alongsideclearTimeout(timer).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/laneEnvironmentService.ts` around lines 246 - 249, The timeout handler that currently calls terminateProcessTree(child) with default SIGTERM should be changed to a two-phase termination: when the timer (timer) fires, send a graceful SIGTERM via terminateProcessTree(child, 'SIGTERM') and start a secondary killTimer that after a short graceWindow (e.g., 3s) forces termination with terminateProcessTree(child, 'SIGKILL'); ensure both timers (timer and killTimer) are cleared inside the process handlers (the error and close callbacks that set/observe settled) so the execCommand promise always settles; update references in execCommand and callers (startDocker, installDependencies, cleanupLaneEnvironment) as needed and keep the settled flag logic intact.apps/ade-cli/src/bootstrap.ts (1)
125-134: Cache the augmented agent env instead of recomputing per PTY spawn.
getAdeCliAgentEnvis invoked on every PTY launch inptyService.ts:1104, so each spawn re-runsaugmentProcessPathWithShellAndKnownCliDirswithincludeInteractiveShell: true— which spawns shell subprocesses (viaexecFileSyncwith-lcand-icflags, bounded by the 1s timeout) to resolve PATH. This pattern differs from the desktop implementation, which cachesadeCliService.agentEnvat startup.♻️ Proposed refactor: compute once, reuse
-function createHeadlessAdeCliAgentEnv(baseEnv: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { - const next: NodeJS.ProcessEnv = { ...baseEnv }; - const nextPath = augmentProcessPathWithShellAndKnownCliDirs({ - env: next, - includeInteractiveShell: true, - timeoutMs: 1_000, - }); - if (nextPath) setPathEnvValue(next, nextPath); - return next; -} +// Resolve the augmented PATH once at module load and reuse it for every +// PTY spawn so we don't re-spawn shells on each session. +const headlessAugmentedPath = augmentProcessPathWithShellAndKnownCliDirs({ + env: process.env, + includeInteractiveShell: true, + timeoutMs: 1_000, +}); +if (headlessAugmentedPath) setPathEnvValue(process.env, headlessAugmentedPath); + +function createHeadlessAdeCliAgentEnv(baseEnv: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { + const next: NodeJS.ProcessEnv = { ...baseEnv }; + if (headlessAugmentedPath) setPathEnvValue(next, headlessAugmentedPath); + return next; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/bootstrap.ts` around lines 125 - 134, The code currently calls createHeadlessAdeCliAgentEnv (which runs augmentProcessPathWithShellAndKnownCliDirs with includeInteractiveShell:true) on every PTY spawn via getAdeCliAgentEnv, causing repeated shell subprocesses; instead compute the augmented env once at startup and cache it on the service (e.g., set adeCliService.agentEnv = createHeadlessAdeCliAgentEnv()) and change getAdeCliAgentEnv to return that cached adeCliService.agentEnv (falling back to process.env if missing); keep createHeadlessAdeCliAgentEnv as the initializer but remove per-spawn calls to augmentProcessPathWithShellAndKnownCliDirs so PTY spawn no longer recomputes the PATH.apps/desktop/src/main/services/ipc/registerIpc.ts (1)
1936-1958: ReuseresolveRendererSuppliedPath()forrootPathhere.
appRevealPath,appOpenPath, andcomputerUseReadArtifactPreviewnow normalizefile://andade-artifact://inputs, butappOpenPathInEditorstill does a rawpath.resolve(rootRaw). If a renderer caller reuses those URI forms with the new"default"target, this handler will reject them before the allowlist check.Suggested refactor
const rootRaw = typeof arg?.rootPath === "string" ? arg.rootPath.trim() : ""; const relRaw = typeof arg?.relativePath === "string" ? arg.relativePath.trim() : ""; const target = arg?.target; if (!rootRaw) throw new Error("Missing root path."); if (target !== "default" && target !== "finder" && target !== "vscode" && target !== "cursor" && target !== "zed") { throw new Error("Unsupported editor target."); } - const rootPath = path.resolve(rootRaw); + const ctx = getCtx(); + const rootPath = resolveRendererSuppliedPath(rootRaw, ctx.project.rootPath); // Validate the renderer-supplied root is a known workspace root // (same pattern as appRevealPath). - const allowedRoots = getAllowedDirs(getCtx); + const allowedRoots = getAllowedDirs(() => ctx);As per coding guidelines, Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 1936 - 1958, The handler in appOpenPathInEditor currently does path.resolve(rootRaw) and rejects renderer-supplied file:// or ade-artifact:// URIs; replace that raw resolution with the same normalization used elsewhere by calling resolveRendererSuppliedPath(rootRaw) (the same helper used by appRevealPath/appOpenPath/computerUseReadArtifactPreview), assign its result to rootPath, and preserve the existing allowlist check that uses resolvePathWithinRoot and getAllowedDirs(getCtx); ensure you propagate or throw the same error behavior when resolveRendererSuppliedPath fails so the security checks remain intact.apps/desktop/src/renderer/lib/shell.test.ts (1)
50-56: Optional: improve readability of the expected Windows-quoted line.The assertion on line 54 mixes JS string escapes with Windows double-quote/backslash escapes, which makes it hard to audit visually. Using
String.rawplus explicit concatenation or a helper (e.g., a smallexpectedbuilt from['"', 'C:\\repo path\\\\', '""quoted""', '"'].join("")) would make the intent clearer without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/shell.test.ts` around lines 50 - 56, The expected Windows-quoted string in the test is hard to read due to nested JS and Windows escaping; update the assertion by constructing the expected value with a clearer expression (e.g., use String.raw or build an `expected` variable from joined parts) instead of the inline escaped literal, keeping the same content and using the existing symbols (`argv`, `commandArrayToLine`, `parseCommandLine`) so the test still asserts equality of `line` to `expected` and that `parseCommandLine(line, { platform: "win32" })` round-trips to `argv`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f73f454-e9ba-4319-a320-d5fe06836108
⛔ Files ignored due to path filters (5)
.claude/scheduled_tasks.lockis excluded by!**/*.lockapps/desktop/build/icon.icois excluded by!**/*.icoapps/desktop/vendor/crsqlite/win32-x64/crsqlite.dllis excluded by!**/*.dlldocs/ARCHITECTURE.mdis excluded by!docs/**docs/playbooks/ship-lane.mdis excluded by!docs/**
📒 Files selected for processing (84)
.claude/commands/shipLane.md.github/workflows/release-core.ymlapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/package.jsonapps/desktop/scripts/ade-cli-install-path.cmdapps/desktop/scripts/ade-cli-windows-wrapper.cmdapps/desktop/scripts/after-pack-runtime-fixes.cjsapps/desktop/scripts/dev.cjsapps/desktop/scripts/validate-win-artifacts.mjsapps/desktop/src/main/main.tsapps/desktop/src/main/packagedRuntimeSmoke.tsapps/desktop/src/main/services/ai/cliExecutableResolver.test.tsapps/desktop/src/main/services/ai/cliExecutableResolver.tsapps/desktop/src/main/services/ai/providerCredentialSources.tsapps/desktop/src/main/services/ai/providerTaskRunner.test.tsapps/desktop/src/main/services/ai/providerTaskRunner.tsapps/desktop/src/main/services/ai/tools/globSearch.test.tsapps/desktop/src/main/services/ai/tools/globSearch.tsapps/desktop/src/main/services/ai/tools/universalTools.test.tsapps/desktop/src/main/services/ai/tools/universalTools.tsapps/desktop/src/main/services/ai/tools/workflowTools.tsapps/desktop/src/main/services/automations/automationPlannerService.tsapps/desktop/src/main/services/automations/automationService.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/cursorAcpPool.tsapps/desktop/src/main/services/cli/adeCliService.test.tsapps/desktop/src/main/services/cli/adeCliService.tsapps/desktop/src/main/services/cli/windowsPackaging.test.tsapps/desktop/src/main/services/computerUse/localComputerUse.tsapps/desktop/src/main/services/conflicts/conflictService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneEnvironmentService.tsapps/desktop/src/main/services/opencode/openCodeBinaryManager.tsapps/desktop/src/main/services/opencode/openCodeRuntime.tsapps/desktop/src/main/services/opencode/openCodeServerManager.test.tsapps/desktop/src/main/services/opencode/openCodeServerManager.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.tsapps/desktop/src/main/services/orchestrator/baseOrchestratorAdapter.tsapps/desktop/src/main/services/orchestrator/orchestratorConstants.tsapps/desktop/src/main/services/orchestrator/orchestratorContext.tsapps/desktop/src/main/services/orchestrator/orchestratorService.tsapps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.test.tsapps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/shared/processExecution.test.tsapps/desktop/src/main/services/shared/processExecution.tsapps/desktop/src/main/services/shared/utils.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/sync/syncService.test.tsapps/desktop/src/main/services/sync/syncService.tsapps/desktop/src/main/services/tests/testService.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/app/CommandPalette.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/ChatComputerUsePanel.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsxapps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsxapps/desktop/src/renderer/components/settings/AdeCliSection.tsxapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.test.tsapps/desktop/src/renderer/lib/dirtyWorkspaceBuffers.tsapps/desktop/src/renderer/lib/pathUtils.test.tsapps/desktop/src/renderer/lib/pathUtils.tsapps/desktop/src/renderer/lib/platform.tsapps/desktop/src/renderer/lib/shell.test.tsapps/desktop/src/renderer/lib/shell.tsapps/desktop/src/renderer/onboarding/tours/filesTour.tsapps/desktop/src/shared/adeLayout.tsapps/desktop/src/shared/adeMcpIpc.test.tsapps/desktop/src/shared/adeMcpIpc.ts
| @@ -0,0 +1,37 @@ | |||
| @echo off | |||
There was a problem hiding this comment.
Convert file to CRLF line endings.
Batch files with LF-only line endings are known to trigger the Windows batch parser's 512‑byte boundary bug and break GOTO/CALL label resolution — worth fixing before this ships as an installer script. Either commit as CRLF or add a .gitattributes entry pinning *.cmd to CRLF.
As per static-analysis hint (Blinter E018): "Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures".
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 37 LF sequences found
(E018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/scripts/ade-cli-install-path.cmd` at line 1, The file
apps/desktop/scripts/ade-cli-install-path.cmd uses LF-only line endings which
can trigger Windows batch parser bugs; convert this .cmd file to CRLF line
endings (or add a .gitattributes rule forcing *.cmd text eol=crlf) so the batch
parser handles GOTO/CALL labels correctly; update the repository by committing
the converted ade-cli-install-path.cmd (or adding the .gitattributes entry) and
verify the file now has CRLF endings.
| @@ -0,0 +1,45 @@ | |||
| @echo off | |||
There was a problem hiding this comment.
Convert file to CRLF line endings.
Same concern as ade-cli-install-path.cmd — the Windows batch parser has known 512‑byte-boundary issues with LF-only line endings that can corrupt GOTO/CALL label resolution. Commit as CRLF or pin via .gitattributes.
As per static-analysis hint (Blinter E018): "Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures".
🧰 Tools
🪛 Blinter (1.0.112)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 45 LF sequences found
(E018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/scripts/ade-cli-windows-wrapper.cmd` at line 1, The batch file
starting with the line "@echo off" uses LF-only endings which can break Windows
batch label parsing; convert this file (ade-cli-windows-wrapper.cmd) to CRLF
line endings or add a .gitattributes rule forcing CRLF for *.cmd files so the
Windows batch parser handles GOTO/CALL labels correctly; ensure the commit
contains the converted CRLF file (or the .gitattributes change) and re-run the
Windows batch linter to verify the issue is resolved.
| if defined ADE_CLI_NODE ( | ||
| call :run_with_runtime_env "%ADE_CLI_NODE%" "%CLI_JS%" %* | ||
| exit /b %ERRORLEVEL% | ||
| ) | ||
|
|
||
| if exist "%APP_EXE%" ( | ||
| set "ELECTRON_RUN_AS_NODE=1" | ||
| call :run_with_runtime_env "%APP_EXE%" "%CLI_JS%" %* | ||
| exit /b %ERRORLEVEL% | ||
| ) | ||
|
|
||
| where node >nul 2>nul | ||
| if not errorlevel 1 ( | ||
| node -e "process.exit(Number(process.versions.node.split('.')[0]) >= 22 ? 0 : 1)" >nul 2>nul | ||
| if not errorlevel 1 ( | ||
| call :run_with_runtime_env node "%CLI_JS%" %* | ||
| exit /b %ERRORLEVEL% | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
cmd.exe batch ERRORLEVEL inside parentheses parse time expansion delayed expansion
💡 Result:
In Windows cmd.exe batch files, using %ERRORLEVEL% inside parentheses (parenthesized blocks like IF or FOR) fails to show the current value after a command changes it. This is due to parse-time expansion: the entire block is parsed and variables expanded before execution begins. So %ERRORLEVEL% captures the value at parse time (usually 0), not runtime. To fix this, use delayed expansion: add setlocal EnableDelayedExpansion at the script start (or top of scope), then use !ERRORLEVEL! instead of %ERRORLEVEL% inside the block. This expands variables at execution time. Example (fails): if 1==1 ( exit /b 1 echo %ERRORLEVEL% REM Shows 0 ) Example (works): setlocal EnableDelayedExpansion if 1==1 ( exit /b 1 echo !ERRORLEVEL! REM Shows 1 ) endlocal IF ERRORLEVEL n checks the runtime value correctly without delayed expansion, as it compares the current ERRORLEVEL directly (true if >= n). Alternatives without delayed expansion: - Use && / || for conditional execution. - call :subroutine to check %ERRORLEVEL% outside the block. - if errorlevel 1 (handle error) else if not errorlevel 1 (handle non-zero). This behavior is unchanged as of 2026; see Microsoft SETLOCAL docs and SS64 for details.
Citations:
- 1: https://superuser.com/questions/1650633/wrong-errorlevel-inside-parenthesis-in-windows-batch-script
- 2: https://stackoverflow.com/questions/10935693/foolproof-way-to-check-for-nonzero-error-return-code-in-windows-batch-file
- 3: https://stackoverflow.com/questions/4367930/errorlevel-inside-if
- 4: https://devblogs.microsoft.com/oldnewthing/20060823-00/?p=29993
- 5: https://ss64.com/nt/errorlevel.html
- 6: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/setlocal
- 7: https://ss64.com/nt/setlocal.html
- 8: https://ss64.com/nt/delayedexpansion.html
- 9: http://ss64.com/nt/errorlevel.html
- 10: https://ss64.com/nt/syntax-brackets.html
🏁 Script executed:
# Check if the file exists and examine lines 19-37
if [ -f "apps/desktop/scripts/ade-cli-windows-wrapper.cmd" ]; then
echo "=== File exists, examining lines 1-50 ==="
head -50 "apps/desktop/scripts/ade-cli-windows-wrapper.cmd" | cat -n
else
echo "File not found at expected path"
# Search for the file
find . -name "ade-cli-windows-wrapper.cmd" 2>/dev/null
fiRepository: arul28/ADE
Length of output: 1586
🏁 Script executed:
# Search for install-path shim or similar references
rg -i "install.*shim|shim.*install" --type batch,cmd,powershell 2>/dev/null | head -20
# Also search for other .cmd files related to the CLI
find . -name "*.cmd" -o -name "*.bat" | grep -i "cli\|install" | head -20Repository: arul28/ADE
Length of output: 152
🏁 Script executed:
cat -n "apps/desktop/scripts/ade-cli-install-path.cmd"Repository: arul28/ADE
Length of output: 1138
Critical: exit /b %ERRORLEVEL% inside ( ... ) captures the parse-time value, not the post-call exit code.
In cmd.exe, all %VAR% references inside a parenthesized compound statement are expanded once when the block is first parsed, before the block executes. That means all three of these branches:
- Lines 19‑22 (
ADE_CLI_NODE) - Lines 24‑28 (
APP_EXE/ Electron-as-Node) - Lines 31‑37 (system
node)
will exit with whatever ERRORLEVEL was before the call :run_with_runtime_env ran — effectively 0/stale — regardless of the actual CLI exit code. This silently breaks &&/|| piping, CI gates, and any caller checking %ERRORLEVEL%.
The install-path shim (which generates flat, non-parenthesized statements) does not have this issue.
Fix: Replace exit /b %ERRORLEVEL% with bare exit /b (uses the live ERRORLEVEL at exit time):
Proposed fix (use bare `exit /b`)
if defined ADE_CLI_NODE (
call :run_with_runtime_env "%ADE_CLI_NODE%" "%CLI_JS%" %*
- exit /b %ERRORLEVEL%
+ exit /b
)
if exist "%APP_EXE%" (
set "ELECTRON_RUN_AS_NODE=1"
call :run_with_runtime_env "%APP_EXE%" "%CLI_JS%" %*
- exit /b %ERRORLEVEL%
+ exit /b
)
where node >nul 2>nul
if not errorlevel 1 (
node -e "process.exit(Number(process.versions.node.split('.')[0]) >= 22 ? 0 : 1)" >nul 2>nul
if not errorlevel 1 (
call :run_with_runtime_env node "%CLI_JS%" %*
- exit /b %ERRORLEVEL%
+ exit /b
)
)Consider adding an end-to-end test in windowsPackaging.test.ts that verifies a failing CLI propagates a non-zero exit code through the wrapper.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if defined ADE_CLI_NODE ( | |
| call :run_with_runtime_env "%ADE_CLI_NODE%" "%CLI_JS%" %* | |
| exit /b %ERRORLEVEL% | |
| ) | |
| if exist "%APP_EXE%" ( | |
| set "ELECTRON_RUN_AS_NODE=1" | |
| call :run_with_runtime_env "%APP_EXE%" "%CLI_JS%" %* | |
| exit /b %ERRORLEVEL% | |
| ) | |
| where node >nul 2>nul | |
| if not errorlevel 1 ( | |
| node -e "process.exit(Number(process.versions.node.split('.')[0]) >= 22 ? 0 : 1)" >nul 2>nul | |
| if not errorlevel 1 ( | |
| call :run_with_runtime_env node "%CLI_JS%" %* | |
| exit /b %ERRORLEVEL% | |
| ) | |
| ) | |
| if defined ADE_CLI_NODE ( | |
| call :run_with_runtime_env "%ADE_CLI_NODE%" "%CLI_JS%" %* | |
| exit /b | |
| ) | |
| if exist "%APP_EXE%" ( | |
| set "ELECTRON_RUN_AS_NODE=1" | |
| call :run_with_runtime_env "%APP_EXE%" "%CLI_JS%" %* | |
| exit /b | |
| ) | |
| where node >nul 2>nul | |
| if not errorlevel 1 ( | |
| node -e "process.exit(Number(process.versions.node.split('.')[0]) >= 22 ? 0 : 1)" >nul 2>nul | |
| if not errorlevel 1 ( | |
| call :run_with_runtime_env node "%CLI_JS%" %* | |
| exit /b | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/scripts/ade-cli-windows-wrapper.cmd` around lines 19 - 37, The
three branches that call :run_with_runtime_env (the ADE_CLI_NODE branch, the
APP_EXE/ELECTRON_RUN_AS_NODE branch, and the system node branch) currently use
"exit /b %ERRORLEVEL%" which is expanded at parse time and therefore returns a
stale value; change each "exit /b %ERRORLEVEL%" to a bare "exit /b" so cmd.exe
uses the live ERRORLEVEL set by the called CLI, ensuring :run_with_runtime_env's
exit code (and any failures from node/Electron) propagate correctly; update the
statements that reference ADE_CLI_NODE, APP_EXE, and the node check around
CLI_JS accordingly and consider adding a windowsPackaging.test.ts case that runs
the wrapper with a failing CLI to assert non-zero exit propagation.
| if (args.stdinText != null && child.stdin) { | ||
| child.stdin.end(args.stdinText); | ||
| } |
There was a problem hiding this comment.
Defend the stdin write against early-exit races.
Paired with the stdin error-handler fix above, wrap the end() call so a synchronous throw on an already-destroyed stdin doesn't escape the Promise executor. Today, if the child dies between spawn and this line, child.stdin.end(...) can throw ERR_STREAM_DESTROYED synchronously and bypass the outer handlers.
🛡️ Proposed fix
- if (args.stdinText != null && child.stdin) {
- child.stdin.end(args.stdinText);
- }
+ if (args.stdinText != null && child.stdin) {
+ try {
+ child.stdin.end(args.stdinText);
+ } catch {
+ // Ignore — stdin may already be destroyed if the child exited immediately.
+ // The `close` handler will surface the real exit code.
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (args.stdinText != null && child.stdin) { | |
| child.stdin.end(args.stdinText); | |
| } | |
| if (args.stdinText != null && child.stdin) { | |
| try { | |
| child.stdin.end(args.stdinText); | |
| } catch { | |
| // Ignore — stdin may already be destroyed if the child exited immediately. | |
| // The `close` handler will surface the real exit code. | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ai/providerTaskRunner.ts` around lines 130 -
132, Protect the synchronous call to child.stdin.end from throwing if the stream
was destroyed: inside providerTaskRunner where you check if (args.stdinText !=
null && child.stdin) wrap the child.stdin.end(args.stdinText) invocation in a
try/catch so any immediate ERR_STREAM_DESTROYED (or other sync throws) are
caught and converted into the same Promise rejection/handlers used elsewhere
(matching the stdin 'error' handler change); reference variables: child,
args.stdinText, and the stdin.end call to locate and update the code.
| @@ -388,15 +389,18 @@ async function runCodexExec(args: { | |||
| } | |||
| const commandPreview = [quoteIfNeeded(codexExecutable), ...cliArgs.map(quoteIfNeeded)].join(" "); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check where plannerCommandPreview / commandPreview surfaces to users
rg -nP --type=ts -C2 '\b(plannerCommandPreview|commandPreview)\b'Repository: arul28/ADE
Length of output: 12061
🏁 Script executed:
#!/bin/bash
# Read the automationPlannerService.ts file around lines 390-430 and 474-530
# to see how the child process is invoked and if Windows wrapping occurs
sed -n '390,430p' apps/desktop/src/main/services/automations/automationPlannerService.ts
echo "---"
sed -n '474,530p' apps/desktop/src/main/services/automations/automationPlannerService.tsRepository: arul28/ADE
Length of output: 3402
🏁 Script executed:
#!/bin/bash
# Find where resolveCliSpawnInvocation is defined
rg -n "resolveCliSpawnInvocation" --type=ts -B2 -A10Repository: arul28/ADE
Length of output: 35211
🏁 Script executed:
#!/bin/bash
# Read the processExecution.ts file to see the full implementation
sed -n '64,80p' apps/desktop/src/main/services/shared/processExecution.tsRepository: arul28/ADE
Length of output: 480
🏁 Script executed:
#!/bin/bash
# Find the shouldUseWindowsCmdWrapper and resolveWindowsCmdInvocation implementations
sed -n '1,100p' apps/desktop/src/main/services/shared/processExecution.ts | grep -A 20 "shouldUseWindowsCmdWrapper\|resolveWindowsCmdInvocation"Repository: arul28/ADE
Length of output: 1547
🏁 Script executed:
#!/bin/bash
# Verify the invocation object is available and could be used for preview
# Look at lines 398-430 and 481-530 to see if invocation is available for the preview
sed -n '398,430p' apps/desktop/src/main/services/automations/automationPlannerService.tsRepository: arul28/ADE
Length of output: 1146
commandPreview diverges from the actual invocation on Windows.
commandPreview is built from the original codexExecutable/claudeExecutable + cliArgs (lines 390 and 474), but on Windows the real child process is cmd.exe /d /s /c "..." (wrapped by resolveCliSpawnInvocation). If this preview surfaces in logs, UI, or the plannerCommandPreview returned to callers, Windows users may see a command that doesn't match what actually ran and wouldn't be directly copy-pasteable to reproduce failures.
If preserving the logical command is intentional (likely — it reads better), consider a brief comment noting the divergence; otherwise consider deriving the preview from invocation.command/invocation.args which are available immediately after the resolveCliSpawnInvocation call.
Also applies to: 474-474
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/automations/automationPlannerService.ts` at
line 390, commandPreview currently concatenates codexExecutable/claudeExecutable
with cliArgs which diverges from the actual Windows spawn (resolved by
resolveCliSpawnInvocation), so update the code to either (a) derive the
displayed preview from the resolved invocation (use invocation.command and
invocation.args immediately after resolveCliSpawnInvocation) so the preview
matches the real child process on all platforms, or (b) if you want to preserve
the logical command string, add a brief comment where commandPreview is
constructed (and in plannerCommandPreview) stating that the preview is a
human-readable logical command and may differ from the actual Windows invocation
(cmd.exe /d /s /c "..."); reference symbols: commandPreview, codexExecutable,
claudeExecutable, cliArgs, resolveCliSpawnInvocation, invocation.command,
invocation.args, plannerCommandPreview.
|
@copilot review but do not make fixes |
|
|
||
| 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.
[🟠 High] [🔵 Bug]
The new Windows sandbox path never treats < file as a path-bearing operation. checkWorkerSandbox() only derives mutating/read targets from >>? and the new MUTATING_CMD_RE, so a command such as type nul < C:\Windows\win.ini | upload-tool reaches the final allowed: true path without any allowedPaths or protected-file check on win.ini. That is a real sandbox escape on the new cmd.exe execution path, because redirected input can read arbitrary files outside the repo. Extend both mutation detection and path-reference extraction to include < so redirected input files are validated the same way as > targets.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
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;| try { | ||
| const realCwd = canonicalizePathForContainment(cwd, pathApi); | ||
| const realScriptPath = canonicalizePathForContainment(scriptPath, pathApi); | ||
| if (!isWithinDirForPathApi(pathApi, realCwd, realScriptPath)) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new inspectScriptFile() path check hard-codes the script to be under cwd, so powershell.exe -File rejects scripts from directories that the sandbox explicitly allowlisted in WorkerSandboxConfig.allowedPaths. The rest of the sandbox honors those extra roots (for example normal reads/writes do), so this new PowerShell branch is not symmetric with the existing contract and will block valid Windows worker workflows that keep helper scripts in an approved external tools directory. Reuse the same allowed-root containment logic here instead of checking only against cwd.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
const realCwd = canonicalizePathForContainment(cwd, pathApi);
const realScriptPath = canonicalizePathForContainment(scriptPath, pathApi);
if (!isWithinDirForPathApi(pathApi, realCwd, realScriptPath)) {
return {| await startHostIfNeeded(); | ||
| } else { | ||
| await stopHostIfRunning(); | ||
| if (!isCrdtSyncAvailable()) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new unavailable-CRDT branch disconnects the viewer but preserves the saved draft/cluster state, which leaves unsupported desktops looking like transferable viewers even though they cannot host sync: ts // apps/desktop/src/main/services/sync/syncService.ts await stopHostIfRunning(); if (!isCrdtSyncAvailable()) { if (syncPeerService.isConnected()) { syncPeerService.disconnect({ preserveDraft: true }); } continue; } I verified that getStatus() still derives role: "viewer" from the saved draft / remote cluster, SyncDevicesSection enables the handoff button from status.transferReadiness.ready, and transferBrainToLocal() has no matching availability guard. On a Windows machine without cr-sqlite opening a previously paired project, clicking “Make this desktop the host” will rewrite local cluster ownership without starting a host or propagating the handoff to the real brain, leaving sync state split. Make the transfer path fail fast when !isCrdtSyncAvailable() or report transfer readiness as blocked in this state.
| } | ||
|
|
||
| export function shouldAttemptDesktopSocketConnection(socketPath: string): boolean { | ||
| return isAdeMcpNamedPipePath(socketPath) || fs.existsSync(socketPath); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This helper now treats any Windows named-pipe path as immediately eligible for a desktop connection attempt: ts // apps/ade-cli/src/cli.ts export function shouldAttemptDesktopSocketConnection(socketPath: string): boolean { return isAdeMcpNamedPipePath(socketPath) || fs.existsSync(socketPath); } ... if (!options.headless && shouldAttemptDesktopSocketConnection(layout.socketPath)) { const socketClient = await SocketJsonRpcClient.connect(layout.socketPath, options.timeoutMs); } I verified that SocketJsonRpcClient.connect() waits on that probe for up to min(timeoutMs, 5000), so every ade invocation inside a Windows ADE project now goes through a failed desktop-socket connect path before headless fallback when the desktop app is not running. That is a user-visible startup regression for headless-only workflows. Restore a fast-fail readiness check for named pipes (or another side channel that proves a live desktop instance) before attempting the full socket connection.
| linearIngressService: headlessLinearServices.linearIngressService, | ||
| linearRoutingService: headlessLinearServices.linearRoutingService, | ||
| processService: headlessLinearServices.processService, | ||
| processService, |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
This change exposes a real processService from the transient headless CLI runtime:
// apps/ade-cli/src/bootstrap.ts
linearSyncService: headlessLinearServices.linearSyncService,
linearIngressService: headlessLinearServices.linearIngressService,
linearRoutingService: headlessLinearServices.linearRoutingService,
processService,
computerUseArtifactBrokerService,But the CLI still creates that runtime per invocation and always disposes it in InProcessJsonRpcClient.close() / executePlan(... finally { await connection.close(); }), which calls runtime.dispose() immediately after the action finishes. process.startAll/stopAll are exposed through run_ade_action in @apps/ade-cli/src/adeRpcServer.ts, so a headless command like ade run start-all --lane ... now appears to succeed and then tears down the started PTYs before the user can use them. That makes the new headless ProcessService integration semantically broken rather than merely incomplete. Fix by either keeping process actions unavailable in headless mode until a long-lived daemon owns the sessions, or moving process execution/state to a runtime that survives past a single CLI command.
| 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]
getHeadlessLaneRuntimeEnv() derives a lane's runtime ports from its current index in laneService.list(), but laneService.list() is backed by created_at desc ordering, so creating any newer lane renumbers every older lane on the next process start/restart. That makes headless ade run start sessions change PORT/PORT_RANGE_*/HOSTNAME for the same lane over time, unlike desktop where main.ts uses the persisted portAllocationService lease for stable values. A process that was previously reachable on one port can restart on a different port after unrelated lane creation. Reuse a stable per-lane lease/mapping in headless mode instead of deriving from transient list position.
// apps/ade-cli/src/bootstrap.ts
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));
const portStart = 3000 + laneIndex * 100;
const portEnd = portStart + 99;| return { | ||
| startupCommand: `exec claude -p ${shellInlineDecodedArg(prompt)}`, | ||
| command: resolvedClaude.path, | ||
| args: ["-p", prompt], |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This refactor changes override execution from a shell-decoded prompt to a raw argv prompt:
// apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts
return {
startupCommand: `exec claude -p ${shellInlineDecodedArg(prompt)}`,
command: resolvedClaude.path,
args: ["-p", prompt],
};On Windows the resolved Claude binary is typically a .cmd, so ptyService routes it through resolveCliSpawnInvocation(), and quoteWindowsCmdArg() explicitly rewrites \r/\n to spaces before building the cmd.exe command line (the new test in @apps/desktop/src/main/services/shared/processExecution.test.ts even asserts "line one\r\nline two" -> "line one line two"). step.metadata.startupCommand is treated here as free-form prompt text, not a shell command, so multiline overrides now arrive at Claude as one flattened line only on Windows. That changes fenced code blocks, numbered instructions, and copied command sequences. Route override prompts through the same prompt-file/stdin launcher used for regular workers, or otherwise avoid the Windows cmd.exe argv path for multiline prompt text.
| child: ReturnType<typeof spawn>, | ||
| signal: NodeJS.Signals, | ||
| ): boolean { | ||
| if (child.exitCode !== null || child.signalCode !== null) return false; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
terminateExternalResolverProcessTree now relies on a detached Unix process group so the timeout path can escalate from SIGTERM to SIGKILL, but the new early return stops the second-stage kill as soon as the group leader records an exit/signal. In the common timeout case the leader can die on SIGTERM while grandchildren in the same process group keep running, so the 2-second fallback at lines 3534-3535 never sends SIGKILL to -child.pid and the resolver leaks background processes after ADE marks the run failed. Restrict that short-circuit to Windows, or otherwise keep attempting process.kill(-child.pid, signal) on Unix whenever a valid detached-group pid exists.
// apps/desktop/src/main/services/conflicts/conflictService.ts
function terminateExternalResolverProcessTree(
child: ReturnType<typeof spawn>,
signal: NodeJS.Signals,
): boolean {
if (child.exitCode !== null || child.signalCode !== null) return false;| if (child.exitCode !== null || child.signalCode !== null) return false; | |
| if (process.platform === "win32" && (child.exitCode !== null || child.signalCode !== null)) return false; |
| } | ||
|
|
||
| const mutationKind = POWERSHELL_MUTATION_KIND_BY_COMMAND.get(innerCommandName); | ||
| if (!mutationKind) continue; |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
Inside the new deep-inspection path, decoded -EncodedCommand payloads and inspectable .ps1 files are parsed only against POWERSHELL_MUTATION_KIND_BY_COMMAND, and any other inner command is ignored entirely: ts // apps/desktop/src/main/services/ai/tools/universalTools.ts const mutationKind = POWERSHELL_MUTATION_KIND_BY_COMMAND.get(innerCommandName); if (!mutationKind) continue; scriptMutates = true; Because checkWorkerSandbox() applies the blocked-command regexes only to the outer command string before trusting powerShellInspection, dangerous native commands hidden inside the decoded/scripted PowerShell body (for example reg.exe add ..., bcdedit.exe ..., or cmd.exe /c copy ...) can pass the sandbox even though this PR’s goal is to enforce Windows policy directly. Treat unknown inner commands in inspectable PowerShell bodies as deny-by-default, or run the blocked/mutating checks recursively on each inner segment before allowing execution.
|
@copilot review but do not make fixes |
| ["set-content", "path-write"], | ||
| ["set-item", "provider-item"], | ||
| ["set-itemproperty", "provider-item"], | ||
| ["new-itemproperty", "provider-item"], | ||
| ["remove-itemproperty", "provider-item"], | ||
| ["rename-itemproperty", "provider-item"], | ||
| ["move-itemproperty", "provider-item"], | ||
| ["clear-itemproperty", "provider-item"], | ||
| ]); | ||
|
|
||
| function requiresTurnMemoryGuard(state?: TurnMemoryPolicyState): boolean { | ||
| return !!state && state.classification === "required" && !state.orientationSatisfied && !state.explicitSearchPerformed; | ||
| } | ||
|
|
||
| function bashCommandLikelyMutates(command: string): boolean { | ||
| return MUTATING_BASH_RE.test(command) || WRITE_COMMAND_RE.test(command); | ||
| function tokenizePowerShellCommand(command: string): string[] { | ||
| const tokens: string[] = []; | ||
| let current = ""; | ||
| let quote: "'" | '"' | null = null; | ||
| let escaped = false; | ||
|
|
||
| const pushCurrent = () => { | ||
| if (current.length > 0) { |
There was a problem hiding this comment.
'' 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:
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;
}Prompt To Fix 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.
| value.startsWith("\\\\") || | ||
| value.startsWith(".\\") || | ||
| value.startsWith("..\\") || | ||
| value.includes("\\") |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This new Windows heuristic treats any token containing \ as a filesystem path, even when the token is just a regex/pattern literal. ts // apps/desktop/src/main/services/ai/tools/universalTools.ts (windowsMode && ( /^[a-zA-Z]:(?:[\\/]|$)/.test(value) || value.startsWith("\\\\") || value.startsWith(".\\") || value.startsWith("..\\") || value.includes("\\") )) checkWorkerSandbox() still feeds every token accepted here through collectPathReferences() and path.win32.resolve(...), so a read-only command like rg "\\bfoo\\b" src on Windows resolves the regex token to C:\bfoo\b and rejects the command as Path outside sandbox. That breaks common search workflows the sandbox is supposed to allow. Tighten this predicate so only actual Windows path forms (drive paths, UNC paths, or explicit relative prefixes like .\ / ..\) are treated as paths.
| }; | ||
| } | ||
|
|
||
| function createHeadlessAdeCliAgentEnv(baseEnv: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
createHeadlessAdeCliAgentEnv() only returns an augmented clone for PTY launches; it never updates the headless runtime's own process.env. ```ts
// apps/ade-cli/src/bootstrap.ts
function createHeadlessAdeCliAgentEnv(baseEnv: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv {
const next: NodeJS.ProcessEnv = { ...baseEnv };
const nextPath = augmentProcessPathWithShellAndKnownCliDirs({
env: next,
includeInteractiveShell: true,
That means helpers running inside the CLI process still execute on the unmodified PATH. In this same headless runtime, `createHeadlessGitHubService()` falls back to `spawnSync("gh", ["auth", "token"])` when `ADE_GITHUB_TOKEN` / `GITHUB_TOKEN` are absent, so Windows installs discoverable only through the new known-dir augmentation remain invisible there and `ade prs create` still fails to pick up GitHub auth. Apply the augmented PATH to `process.env` during `createAdeRuntime()` as desktop does in `fixElectronShellPath()`, or route internal spawns through the same augmented env helper.
| ? resolveRouteRebaseSelection({ rebaseNeeds: [], routeItemId: route.laneId }) | ||
| : null, | ||
| }; | ||
| const params = new URLSearchParams(window.location.search); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
PrsContext now initializes the PR page from window.location.search only, even though the production app runs under HashRouter and stores PR route state in the hash. ts // apps/desktop/src/renderer/components/prs/state/PrsContext.tsx const params = new URLSearchParams(window.location.search); const tab = params.get("tab"); ... const prId = params.get("prId"); This means routes opened as #/prs?tab=rebase&laneId=... (still emitted by CreatePrModal/IntegrationTab) mount with activeTab === "normal" and no selection. On that first render, @apps/desktop/src/renderer/components/prs/PRsPage.tsx still runs its URL-sync effect and serializes the stale state back into the route, so the requested rebase/PR deep link is dropped before the page can settle. Restore the old route-based initialization here — e.g. parse the same hash/search contract that PRsPage uses instead of reading the outer search string directly.
| let stderr = ""; | ||
| let settled = false; | ||
| const maxBuffer = 10 * 1024 * 1024; | ||
| const timer = setTimeout(() => { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
execCommand() no longer settles its promise on timeout; it only asks the shared helper to kill the child and waits for a later close event. In @apps/desktop/src/main/services/shared/processExecution.ts, terminateProcessTree() is explicitly best-effort and returns false when taskkill/child.kill cannot terminate the process, so this timeout branch has a real no-settlement path. That leaves callers like Docker startup, dependency install, and Docker cleanup stuck awaiting forever, with the lane progress step never leaving running.
// apps/desktop/src/main/services/lanes/laneEnvironmentService.ts
const timer = setTimeout(() => {
if (settled) return;
terminateProcessTree(child);
}, timeoutMs);Settle the promise from the timeout handler as well (or at minimum when terminateProcessTree() returns false) so a failed kill attempt becomes a normal command failure instead of an infinite wait.
| `set ${quoteWindowsCmdArg(`${ADE_OPENCODE_MANAGED_ENV}=1`)}` | ||
| + `&&set ${quoteWindowsCmdArg("OPENCODE_DISABLE_PROJECT_CONFIG=1")}` | ||
| + `&&set ${quoteWindowsCmdArg(`${ADE_OPENCODE_OWNER_PID_ENV}=${process.pid}`)}` | ||
| + `&&${invocation.args[3] ?? ""}`; |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
resolveWindowsCmdInvocation() already returns args[3] as a complete cmd.exe /c payload wrapped in outer quotes (for example the shared helper tests assert ""npm" "run" "test""). Appending that string verbatim after && produces a mid-command segment that starts with ""C:\... instead of a normal quoted executable, so cmd.exe sees a malformed command when the OpenCode binary resolves to a Windows shim (.cmd/.bat), which is the common npm-installed case. Strip the outer wrapper before concatenating so the injected marker chain executes the actual OpenCode command.
// apps/desktop/src/main/services/opencode/openCodeServerManager.ts
const cmdLine =
`set ${quoteWindowsCmdArg(`${ADE_OPENCODE_MANAGED_ENV}=1`)}`
+ `&&set ${quoteWindowsCmdArg("OPENCODE_DISABLE_PROJECT_CONFIG=1")}`
+ `&&set ${quoteWindowsCmdArg(`${ADE_OPENCODE_OWNER_PID_ENV}=${process.pid}`)}`
+ `&&${invocation.args[3] ?? ""}`;| + `&&${invocation.args[3] ?? ""}`; | |
| + `&&${(invocation.args[3] ?? "").replace(/^"(.*)"$/, "$1")}`; |
| } | ||
|
|
||
| if (!inQuotes && /\s/.test(ch)) { | ||
| if (current.length) { |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
parseWindowsCommandLine() only emits an argument when current.length is nonzero, so a valid quoted empty argument ("") is discarded instead of preserved. That breaks the renderer’s edit/save round-trip on Windows: @apps/desktop/src/renderer/components/run/RunPage.tsx formats stored argv back into text with commandArrayToLine(definition.command) and later reparses that text via normalizeCommandForConfig() → parseCommandLine(), so a saved definition containing an empty string arg is silently rewritten without it after editing. Track whether an argument has started (including opening quotes) and push current when that flag is set, even when the resulting string is empty.
// apps/desktop/src/renderer/lib/shell.ts
if (!inQuotes && /\s/.test(ch)) {
if (current.length) {
out.push(current);
current = "";
}
}| } | ||
| 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.
[🟡 Medium] [🔵 Bug]
The new Windows branch in resolveSpawnInvocation() no longer matches the shared, tested resolveWindowsCmdInvocation() contract used elsewhere in the app: it passes the joined command as the /c payload without wrapping the full command string. The repo’s own Windows quoting tests expect the outer "..." wrapper for cmd.exe /d /s /c, and omitting it here means npm run dev uses a different parsing path than the one already validated in @apps/desktop/src/main/services/shared/processExecution.test.ts. That makes the Windows dev launcher fragile to normal quoted arguments and can misparse the spawned npx command line. Re-wrap the full joined command string so this script uses the same quoting shape as the shared helper.
// 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,
};| args: ["/d", "/s", "/c", [cmd, ...args].map(quoteWindowsCmdArg).join(" ")], | |
| args: ["/d", "/s", "/c", `"${[cmd, ...args].map(quoteWindowsCmdArg).join(" ")}"`], |
| if (finalPrompt) { | ||
| commandParts.push(shellEscapeArg(finalPrompt)); | ||
| commandArgs.push(finalPrompt); | ||
| commandPreviewParts.push(previewShellEscapeArg(finalPrompt)); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When resolveExecutableOnPath(provider) fails on Windows, this code falls back to a startupCommand string, and that string is later executed by the PTY shell rather than used as display-only text. The problem is that the fallback command is built with previewShellEscapeArg(...), which uses Windows cmd.exe quoting rules, while ptyService still prefers powershell.exe for shell startup on Windows. That means prompt text containing PowerShell interpolation characters like $..., $(...), or PowerShell-specific quoting cases can be rewritten before Claude/Codex ever sees it. This breaks exactly the unresolved-executable path the new tests exercise.
// apps/ade-cli/src/adeRpcServer.ts
if (finalPrompt) {
commandArgs.push(finalPrompt);
commandPreviewParts.push(previewShellEscapeArg(finalPrompt));
}
const startupEnvPrefixParts = process.platform === "win32" ? [] : envPrefixParts;Use PowerShell-safe quoting for fallback startupCommand strings, or force the fallback shell to cmd.exe when emitting windowsShellEscapeArg output.
| 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.
[🟡 Medium] [🔵 Bug]
shouldUseWindowsCmdWrapper() routes every extensionless Windows command through cmd.exe, but resolveWindowsCmdInvocation() then quotes the command token itself. That is correct for .cmd/.bat shims, but not for built-ins like echo or dir: per cmd /?, preserved quotes only work when the quoted token names an executable file. So a call such as createTerminal({ command: "echo", args: ["hi"] }) becomes cmd.exe /d /s /c ""echo" "hi"", which cmd.exe misparses and fails to run. This is a user-facing regression for generic Windows terminal requests introduced by the new shared wrapper.
// apps/desktop/src/main/services/shared/processExecution.ts
const cmdLine = [command, ...args].map(quoteWindowsCmdArg).join(" ");
return {
command: comSpec,
args: ["/d", "/s", "/c", `"${cmdLine}"`],Only emit the fully quoted ""path\\tool.cmd" ..."" form for actual batch/shim files, and leave plain built-ins / extensionless shell commands unquoted or handle them via a true shell-command path.
| continue; | ||
| } | ||
| if (quote) { | ||
| if (ch === quote) { |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
checkWorkerSandbox() now trusts tokenizePowerShellCommand() to extract mutation targets from powershell.exe -Command ..., but this branch closes a single-quoted string on the first ' it sees and never handles PowerShell's documented '' escape for a literal apostrophe. That means a path like 'C:\repo\allow''ed\file.txt' is parsed as C:\repo\allowed\file.txt, so a worker can point at a sibling directory outside the sandbox whose name differs only by an apostrophe while the inspector resolves it as an allowed in-sandbox path. This weakens the new Windows sandbox boundary and needs to preserve doubled single quotes as a literal ' while staying inside the same token; add a regression test for a mutation command using '' inside a single-quoted path.
// apps/desktop/src/main/services/ai/tools/universalTools.ts
if (quote) {
if (ch === quote) {
quote = null;
} else {
current += ch;| if (ch === quote) { | |
| if (ch === quote) { | |
| if (quote === "'" && command[i + 1] === "'") { | |
| current += "'"; | |
| i += 1; | |
| } else { | |
| quote = null; | |
| } | |
| } else { | |
| current += ch; | |
| } |
1 participant
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
adeMcpIpc.ts, preload, registerIpc)..cmd/.exeprobing, wrapper scripts (ade-cli-windows-wrapper.cmd,ade-cli-install-path.cmd), install-path validator.pathUtils.ts, shell detection inshell.ts.after-pack-runtime-fixes.cjsruntime fixes,validate-win-artifacts.mjsartifact validator,cr-sqlitewin32-x64 vendor binary,release-core.ymlworkflow tweaks,.icoicon.universalTools.ts,providerTaskRunner+providerOrchestratorAdapter+orchestratorServiceworker-launch plumbing,taskkill /T /Fprocess-tree termination inprocessExecution.ts.AgentChatMessageList,FilesPage,dirtyWorkspaceBuffersportability fixes.Tests added
providerTaskRunner.test.ts,windowsPackaging.test.ts, new tests inprocessExecution.test.ts(killWindowsProcessTree,terminateProcessTree),pathUtils.test.tsWindows predicates,shell.test.ts,dirtyWorkspaceBuffers.test.ts.Tangentially included (unblocked
ade prs createfor this lane)node_modulesgitignore (worktree symlinks were showing up as untracked).gh auth tokenin ADE CLI whenADE_GITHUB_TOKEN/GITHUB_TOKENenv vars are missing./finalizeplaybook addition (Phase 3j — stale worker-process cleanup).Test plan
Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
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.
WORKER_CLI_LAUNCHER_SCRIPTreadsprocess.argv[1]for the launch-file path, but Node.js (and Electron withELECTRON_RUN_AS_NODE) setsprocess.argv[1]to""when using the-eflag; the first positional argument is atprocess.argv[2].fs.readFileSync("", "utf8")throws ENOENT and every orchestrator worker exits with code 1.''escape:tokenizePowerShellCommandtreats 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) anduniversalTools.ts(PowerShell tokenizer''escape) require fixes before merge.Security Review
universalTools.ts): ThetokenizePowerShellCommandfunction 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.Important Files Changed
node -e SCRIPT launchFileinvocation vianodeWorkerLaunch. Contains a likely bug whereWORKER_CLI_LAUNCHER_SCRIPTreadsprocess.argv[1]for the launch file path, but Node's-emode places the first positional argument atprocess.argv[2].MUTATING_CMD_REfor 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.quoteWindowsCmdArg,resolveCliSpawnInvocation,killWindowsProcessTree, andterminateProcessTree. Well-structured with good error handling.getPathEnvKey/getPathEnvValue/setPathEnvValuefor case-insensitive Windows PATH lookup, plusgetWindowsKnownBinDirscovering Scoop, Winget, Volta, pnpm, npm, and common install paths.resolveRendererSuppliedPathhelper centralizesade-artifact://andfile://URI-to-path resolution across IPC handlers; adds "default" editor target usingshell.openPath; adds Windowsstart ""cmd.exe editor launch.resolveCliSpawnInvocationfor cross-platform spawn andterminateProcessTreeon timeout.quoteWindowsArg) andparseWindowsCommandLineimplementing CommandLineToArgvW rules. Uses deprecatednavigator.platform.\\.\pipe\ade-<sha256[:24]>) or POSIX Unix socket. Correct canonical path normalization before hashing.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:#f00Comments Outside Diff (1)
apps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.ts, line 348-413 (link)process.argv[1]is empty when Node runs with-eThe
WORKER_CLI_LAUNCHER_SCRIPTis launched as:When Node.js (or Electron in
ELECTRON_RUN_AS_NODEmode) runs with-e,process.argv[1]is set to the empty string""— the first positional argument lands atprocess.argv[2]. The script does:fs.readFileSync("", "utf8")will throwENOENT, thefinish(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]:The same fix applies to the failure-path inline script on the last
returnbranch ofbuildStartupCommand:Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (9): Last reviewed commit: "Merge main into Windows port lane" | Re-trigger Greptile