diff --git a/packages/pi-coding-agent/src/core/bash-executor.ts b/packages/pi-coding-agent/src/core/bash-executor.ts index dcdb32ef3..f043b9379 100644 --- a/packages/pi-coding-agent/src/core/bash-executor.ts +++ b/packages/pi-coding-agent/src/core/bash-executor.ts @@ -87,8 +87,12 @@ export function executeBash(command: string, options?: BashExecutorOptions & { l } else { ({ shell, args } = getShellConfig()); } + // On Windows, detached: true sets CREATE_NEW_PROCESS_GROUP which can + // cause EINVAL in VSCode/ConPTY terminal contexts. The bg-shell + // extension already guards this (process-manager.ts); align here. + // Process-tree cleanup uses taskkill /F /T on Windows regardless. const child: ChildProcess = spawn(shell, [...args, sanitizeCommand(command)], { - detached: true, + detached: process.platform !== "win32", env: getShellEnv(), stdio: ["ignore", "pipe", "pipe"], }); diff --git a/packages/pi-coding-agent/src/core/tools/bash-spawn-windows.test.ts b/packages/pi-coding-agent/src/core/tools/bash-spawn-windows.test.ts new file mode 100644 index 000000000..9247addf2 --- /dev/null +++ b/packages/pi-coding-agent/src/core/tools/bash-spawn-windows.test.ts @@ -0,0 +1,101 @@ +/** + * bash-spawn-windows.test.ts — Regression test for Windows spawn EINVAL. + * + * Verifies that bash tool spawn options disable `detached: true` on Windows + * to prevent EINVAL errors in ConPTY / VSCode terminal contexts. + * + * Background: + * On Windows, `spawn()` with `detached: true` sets the + * CREATE_NEW_PROCESS_GROUP flag in CreateProcess. In certain terminal + * contexts (VSCode integrated terminal, ConPTY, Windows Terminal) this + * flag conflicts with the parent process group and causes a synchronous + * EINVAL from libuv. The bg-shell extension already guards against this + * with `detached: process.platform !== "win32"` (process-manager.ts); + * this test ensures all other spawn sites are aligned. + * + * See: gsd-build/gsd-2#XXXX + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; + +// Verify the spawn option pattern used across the codebase. +// This is a static/structural test — it reads the source files and asserts +// they use the platform-guarded detached flag. +import { readFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); + +const SPAWN_FILES = [ + join(__dirname, "bash.ts"), + join(__dirname, "..", "bash-executor.ts"), + join(__dirname, "..", "..", "utils", "shell.ts"), +]; + +test("spawn calls use platform-guarded detached flag (no unconditional detached: true)", () => { + for (const file of SPAWN_FILES) { + const content = readFileSync(file, "utf-8"); + const lines = content.split("\n"); + + for (let i = 0; i < lines.length; i++) { + const line = lines[i]!; + // Skip comments + if (line.trim().startsWith("//") || line.trim().startsWith("*")) continue; + // Check for unconditional `detached: true` + if (/detached:\s*true\b/.test(line)) { + assert.fail( + `${file}:${i + 1} has unconditional 'detached: true' — ` + + `must use 'detached: process.platform !== "win32"' ` + + `to prevent EINVAL on Windows (ConPTY / VSCode terminal)`, + ); + } + } + } +}); + +test("killProcessTree does not use detached: true for taskkill on Windows", () => { + const shellFile = join(__dirname, "..", "..", "utils", "shell.ts"); + const content = readFileSync(shellFile, "utf-8"); + + // Find the taskkill spawn call and ensure it doesn't have detached: true + const taskkillRegion = content.match(/spawn\("taskkill"[\s\S]*?\}\)/); + if (taskkillRegion) { + assert.ok( + !/detached:\s*true/.test(taskkillRegion[0]), + "taskkill spawn should not use detached: true — " + + "it can cause EINVAL on Windows and is unnecessary for a utility process", + ); + } +}); + +// Smoke test: spawn with platform-guarded detached flag actually works +test("spawn with detached: process.platform !== 'win32' succeeds", async () => { + const { promise, resolve, reject } = Promise.withResolvers(); + + const child = spawn( + process.platform === "win32" ? "cmd" : "sh", + process.platform === "win32" ? ["/c", "echo ok"] : ["-c", "echo ok"], + { + detached: process.platform !== "win32", + stdio: ["ignore", "pipe", "pipe"], + }, + ); + + let output = ""; + child.stdout?.on("data", (d: Buffer) => { output += d.toString(); }); + child.on("error", reject); + child.on("close", (code) => { + try { + assert.equal(code, 0, "spawn should succeed"); + assert.ok(output.trim().includes("ok"), `Expected 'ok' in output, got: ${output}`); + resolve(); + } catch (e) { + reject(e); + } + }); + + await promise; +}); diff --git a/packages/pi-coding-agent/src/core/tools/bash.ts b/packages/pi-coding-agent/src/core/tools/bash.ts index 4e1d65257..eccda574b 100644 --- a/packages/pi-coding-agent/src/core/tools/bash.ts +++ b/packages/pi-coding-agent/src/core/tools/bash.ts @@ -158,9 +158,13 @@ const defaultBashOperations: BashOperations = { return; } + // On Windows, detached: true sets CREATE_NEW_PROCESS_GROUP which can + // cause EINVAL in VSCode/ConPTY terminal contexts. The bg-shell + // extension already guards this (process-manager.ts); align here. + // Process-tree cleanup uses taskkill /F /T on Windows regardless. const child = spawn(shell, [...args, command], { cwd, - detached: true, + detached: process.platform !== "win32", env: env ?? getShellEnv(), stdio: ["ignore", "pipe", "pipe"], }); diff --git a/packages/pi-coding-agent/src/utils/shell.ts b/packages/pi-coding-agent/src/utils/shell.ts index ba77a4441..86708125f 100644 --- a/packages/pi-coding-agent/src/utils/shell.ts +++ b/packages/pi-coding-agent/src/utils/shell.ts @@ -192,7 +192,6 @@ export function killProcessTree(pid: number): void { try { spawn("taskkill", ["/F", "/T", "/PID", String(pid)], { stdio: "ignore", - detached: true, }); } catch { // Ignore errors if taskkill fails diff --git a/src/resources/extensions/async-jobs/async-bash-tool.ts b/src/resources/extensions/async-jobs/async-bash-tool.ts index 4314b5c89..034fd207e 100644 --- a/src/resources/extensions/async-jobs/async-bash-tool.ts +++ b/src/resources/extensions/async-jobs/async-bash-tool.ts @@ -14,7 +14,7 @@ import { DEFAULT_MAX_LINES, } from "@gsd/pi-coding-agent"; import { Type } from "@sinclair/typebox"; -import { spawn } from "node:child_process"; +import { spawn, spawnSync } from "node:child_process"; import { createWriteStream } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -38,17 +38,24 @@ function getTempFilePath(): string { } /** - * Kill a process and its children. Uses process group kill on Unix. + * Kill a process and its children (cross-platform). + * Uses process group kill on Unix; taskkill /F /T on Windows. */ function killTree(pid: number): void { - try { - // Kill the process group (negative PID) - process.kill(-pid, "SIGTERM"); - } catch { + if (process.platform === "win32") { try { - process.kill(pid, "SIGTERM"); + spawnSync("taskkill", ["/F", "/T", "/PID", String(pid)], { + timeout: 5_000, + stdio: "ignore", + }); } catch { - // Already exited + try { process.kill(pid, "SIGTERM"); } catch { /* already exited */ } + } + } else { + try { + process.kill(-pid, "SIGTERM"); + } catch { + try { process.kill(pid, "SIGTERM"); } catch { /* already exited */ } } } } @@ -118,9 +125,13 @@ function executeBashInBackground( const rewrittenCommand = rewriteCommandWithRtk(command); const resolvedCommand = sanitizeCommand(rewrittenCommand); + // On Windows, detached: true sets CREATE_NEW_PROCESS_GROUP which can + // cause EINVAL in VSCode/ConPTY terminal contexts. The bg-shell + // extension already guards this (process-manager.ts); align here. + // Process-tree cleanup uses taskkill /F /T on Windows regardless. const child = spawn(shell, [...args, resolvedCommand], { cwd, - detached: true, + detached: process.platform !== "win32", env: { ...process.env }, stdio: ["ignore", "pipe", "pipe"], }); @@ -143,8 +154,8 @@ function executeBashInBackground( // If the process ignores SIGTERM, escalate to SIGKILL sigkillHandle = setTimeout(() => { if (child.pid) { - try { process.kill(-child.pid, "SIGKILL"); } catch { /* ignore */ } - try { process.kill(child.pid, "SIGKILL"); } catch { /* ignore */ } + // killTree already uses taskkill /F /T on Windows + killTree(child.pid); } // Hard deadline: if even SIGKILL doesn't trigger 'close',