From 7af2d27dced8f1f2aff3a678f2e7bc748693dfef Mon Sep 17 00:00:00 2001 From: Vegard Stikbakke Date: Wed, 26 Aug 2026 10:33:10 +0200 Subject: [PATCH] fix(coding-agent): prevent Windows taskkill spawn crashes closes #6596 --- packages/agent/CHANGELOG.md | 4 ++ packages/agent/src/harness/env/nodejs.ts | 16 ++++-- .../agent/test/harness/nodejs-env.test.ts | 42 +++++++++++++++ packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/utils/shell.ts | 22 +++++--- .../regressions/6596-taskkill-enoent.test.ts | 52 +++++++++++++++++++ 6 files changed, 124 insertions(+), 13 deletions(-) create mode 100644 packages/coding-agent/test/suite/regressions/6596-taskkill-enoent.test.ts diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index c057c3228..de33d4554 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed Windows `NodeExecutionEnv` aborts crashing when `taskkill.exe` is unavailable on `PATH` ([#6596](https://github.com/earendil-works/pi/issues/6596)). + ## [0.84.3] - 2026-08-24 ### Fixed diff --git a/packages/agent/src/harness/env/nodejs.ts b/packages/agent/src/harness/env/nodejs.ts index 1225efb92..09bff9bec 100644 --- a/packages/agent/src/harness/env/nodejs.ts +++ b/packages/agent/src/harness/env/nodejs.ts @@ -253,11 +253,17 @@ function getShellEnv( function killProcessTree(pid: number): void { if (process.platform === "win32") { try { - spawn("taskkill", ["/F", "/T", "/PID", String(pid)], { - stdio: "ignore", - detached: true, - windowsHide: true, - }); + const child = spawn( + join(process.env.SystemRoot ?? "C:\\Windows", "System32", "taskkill.exe"), + ["/F", "/T", "/PID", String(pid)], + { + stdio: "ignore", + detached: true, + windowsHide: true, + }, + ); + // A failed spawn emits "error" asynchronously; consume it to avoid crashing Node. + child.once("error", () => {}); } catch { // Ignore errors. } diff --git a/packages/agent/test/harness/nodejs-env.test.ts b/packages/agent/test/harness/nodejs-env.test.ts index becaa8fc3..4595ea30f 100644 --- a/packages/agent/test/harness/nodejs-env.test.ts +++ b/packages/agent/test/harness/nodejs-env.test.ts @@ -496,6 +496,48 @@ describe("NodeExecutionEnv", () => { if (!result.ok) expect(result.error).toMatchObject({ code: "aborted" }); }); + it.skipIf(process.platform === "win32")("ignores asynchronous taskkill spawn errors during abort", async () => { + const root = createTempDir(); + const pidFile = join(root, "shell.pid"); + const controller = new AbortController(); + const env = new NodeExecutionEnv({ cwd: root, shellPath: "/bin/bash" }); + const platformDescriptor = Object.getOwnPropertyDescriptor(process, "platform"); + const previousSystemRoot = process.env.SystemRoot; + process.env.SystemRoot = "/definitely/missing/windows"; + Object.defineProperty(process, "platform", { configurable: true, value: "win32" }); + + let pid: number | undefined; + try { + const execution = env.exec(`echo $$ > ${toBashSingleQuotedArg(pidFile)}; exec sleep 60`, { + abortSignal: controller.signal, + }); + for (let attempt = 0; attempt < 100 && !existsSync(pidFile); attempt++) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + expect(existsSync(pidFile)).toBe(true); + + controller.abort(); + await new Promise((resolve) => setTimeout(resolve, 0)); + pid = Number.parseInt(readFileSync(pidFile, "utf8"), 10); + process.kill(pid, "SIGKILL"); + + const result = await execution; + expect(result).toMatchObject({ ok: false, error: { code: "aborted" } }); + } finally { + if (pid === undefined && existsSync(pidFile)) { + pid = Number.parseInt(readFileSync(pidFile, "utf8"), 10); + } + if (pid !== undefined && Number.isFinite(pid)) { + try { + process.kill(pid, "SIGKILL"); + } catch {} + } + if (previousSystemRoot === undefined) delete process.env.SystemRoot; + else process.env.SystemRoot = previousSystemRoot; + if (platformDescriptor) Object.defineProperty(process, "platform", platformDescriptor); + } + }); + it("captures large shell output to a full output file through the execution env", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 381fb7e8a..5cc12fd66 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixed +- Fixed Windows shell aborts crashing Pi when `taskkill.exe` is unavailable on `PATH` ([#6596](https://github.com/earendil-works/pi/issues/6596)). - Fixed extension messages sent with `triggerTurn: false` while the agent is running being inserted between a tool call and its result, which made providers that validate message order reject the replayed history. They are now appended once the turn's tool results are in ([#8537](https://github.com/earendil-works/pi/issues/8537)). ## [0.84.3] - 2026-08-24 diff --git a/packages/coding-agent/src/utils/shell.ts b/packages/coding-agent/src/utils/shell.ts index fae75267e..c352a3482 100644 --- a/packages/coding-agent/src/utils/shell.ts +++ b/packages/coding-agent/src/utils/shell.ts @@ -1,5 +1,5 @@ import { existsSync } from "node:fs"; -import { delimiter } from "node:path"; +import { delimiter, join } from "node:path"; import { spawn, spawnSync } from "child_process"; import { getBinDir } from "../config.ts"; @@ -215,15 +215,21 @@ export function killTrackedDetachedChildren(): void { */ export function killProcessTree(pid: number): void { if (process.platform === "win32") { - // Use taskkill on Windows to kill process tree + // Use the trusted System32 executable so cleanup does not depend on PATH. try { - spawn("taskkill", ["/F", "/T", "/PID", String(pid)], { - stdio: "ignore", - detached: true, - windowsHide: true, - }); + const child = spawn( + join(process.env.SystemRoot ?? "C:\\Windows", "System32", "taskkill.exe"), + ["/F", "/T", "/PID", String(pid)], + { + stdio: "ignore", + detached: true, + windowsHide: true, + }, + ); + // A failed spawn emits "error" asynchronously; consume it to avoid crashing Node. + child.once("error", () => {}); } catch { - // Ignore errors if taskkill fails + // Ignore errors if taskkill fails. } } else { // Use SIGKILL on Unix/Linux/Mac diff --git a/packages/coding-agent/test/suite/regressions/6596-taskkill-enoent.test.ts b/packages/coding-agent/test/suite/regressions/6596-taskkill-enoent.test.ts new file mode 100644 index 000000000..c5e47f98e --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/6596-taskkill-enoent.test.ts @@ -0,0 +1,52 @@ +import type { ChildProcess } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() })); + +vi.mock("child_process", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, spawn: spawnMock }; +}); + +import { killProcessTree } from "../../../src/utils/shell.ts"; + +function withWindowsPlatform(test: () => void): void { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, "platform"); + try { + Object.defineProperty(process, "platform", { configurable: true, value: "win32" }); + test(); + } finally { + if (platformDescriptor) Object.defineProperty(process, "platform", platformDescriptor); + } +} + +afterEach(() => { + spawnMock.mockReset(); +}); + +describe("issue #6596 taskkill spawn failures", () => { + it("uses System32 taskkill and consumes its asynchronous spawn error", () => { + const child = new EventEmitter() as ChildProcess; + const previousSystemRoot = process.env.SystemRoot; + process.env.SystemRoot = "C:\\CustomWindows"; + spawnMock.mockReturnValue(child); + + try { + withWindowsPlatform(() => { + killProcessTree(1234); + }); + } finally { + if (previousSystemRoot === undefined) delete process.env.SystemRoot; + else process.env.SystemRoot = previousSystemRoot; + } + + expect(spawnMock).toHaveBeenCalledWith( + join("C:\\CustomWindows", "System32", "taskkill.exe"), + ["/F", "/T", "/PID", "1234"], + { detached: true, stdio: "ignore", windowsHide: true }, + ); + expect(() => child.emit("error", new Error("spawn taskkill ENOENT"))).not.toThrow(); + }); +});