From e548d703566460b74c2a16ac59b1790fdcf02afb Mon Sep 17 00:00:00 2001 From: murashit Date: Sun, 28 Jun 2026 06:41:36 +0900 Subject: [PATCH] Fix Windows npm shim app-server launch (#16) --- knip.json | 2 +- src/app-server/connection/transport.ts | 56 ++++++++++++- .../transport-windows-launch.test.ts | 83 +++++++++++++++++++ tests/app-server/transport.test.ts | 36 +++++++- 4 files changed, 172 insertions(+), 5 deletions(-) create mode 100644 tests/app-server/transport-windows-launch.test.ts diff --git a/knip.json b/knip.json index 4a5a53ef..804bc894 100644 --- a/knip.json +++ b/knip.json @@ -2,5 +2,5 @@ "entry": ["src/main.ts", "scripts/**/*.mjs", "*.config.{ts,mjs}", "tests/**/*.test.{ts,tsx}"], "project": ["src/**/*.{ts,tsx}", "tests/**/*.{ts,tsx}", "scripts/**/*.mjs", "*.config.{ts,mjs}"], "ignore": ["src/generated/**"], - "ignoreBinaries": ["codex"] + "ignoreBinaries": ["codex", "taskkill"] } diff --git a/src/app-server/connection/transport.ts b/src/app-server/connection/transport.ts index 233a2eb5..f3a90bb3 100644 --- a/src/app-server/connection/transport.ts +++ b/src/app-server/connection/transport.ts @@ -3,6 +3,12 @@ import * as readline from "node:readline"; import type { RpcOutboundMessage } from "./rpc-messages"; +interface AppServerSpawnSpec { + command: string; + args: string[]; + killProcessTreeOnStop: boolean; +} + export interface AppServerTransport { start(): void; send(message: RpcOutboundMessage): void; @@ -17,10 +23,36 @@ export interface AppServerTransportHandlers { onError: (error: Error) => void; } +export function createAppServerSpawnSpec( + codexPath: string, + options: { platform?: NodeJS.Platform; comSpec?: string } = {}, +): AppServerSpawnSpec { + const platform = options.platform ?? process.platform; + if (platform !== "win32" || !isWindowsCommandScript(codexPath)) { + return { command: codexPath, args: ["app-server"], killProcessTreeOnStop: false }; + } + + const comSpec = options.comSpec?.trim() || process.env["ComSpec"]?.trim() || process.env["COMSPEC"]?.trim() || "cmd.exe"; + return { + command: comSpec, + args: ["/d", "/c", `${quoteWindowsCmdArgument(codexPath)} app-server`], + killProcessTreeOnStop: true, + }; +} + +function isWindowsCommandScript(path: string): boolean { + return /\.(?:bat|cmd)$/i.test(path); +} + +function quoteWindowsCmdArgument(value: string): string { + return `"${value.replace(/"/g, '""')}"`; +} + export class StdioAppServerTransport implements AppServerTransport { private process: ChildProcessWithoutNullStreams | null = null; private reader: readline.Interface | null = null; private stderrBuffer = ""; + private killProcessTreeOnStop = false; constructor( private readonly codexPath: string, @@ -33,7 +65,9 @@ export class StdioAppServerTransport implements AppServerTransport { throw new Error("Codex app-server is already running."); } - this.process = spawn(this.codexPath, ["app-server"], { + const launch = createAppServerSpawnSpec(this.codexPath); + this.killProcessTreeOnStop = launch.killProcessTreeOnStop; + this.process = spawn(launch.command, launch.args, { cwd: this.cwd, stdio: ["pipe", "pipe", "pipe"], }); @@ -43,6 +77,7 @@ export class StdioAppServerTransport implements AppServerTransport { this.reader?.close(); this.reader = null; this.process = null; + this.killProcessTreeOnStop = false; this.handlers.onError(error instanceof Error ? error : new Error(String(error))); }); @@ -51,6 +86,7 @@ export class StdioAppServerTransport implements AppServerTransport { this.reader?.close(); this.reader = null; this.process = null; + this.killProcessTreeOnStop = false; this.handlers.onExit(code, signal); }); @@ -80,10 +116,16 @@ export class StdioAppServerTransport implements AppServerTransport { this.flushStderr(); this.reader?.close(); this.reader = null; - if (this.process && !this.process.killed) { - this.process.kill(); + const child = this.process; + if (child && !child.killed) { + if (this.killProcessTreeOnStop && typeof child.pid === "number") { + killWindowsProcessTree(child.pid); + } else { + child.kill(); + } } this.process = null; + this.killProcessTreeOnStop = false; } isRunning(): boolean { @@ -106,3 +148,11 @@ export class StdioAppServerTransport implements AppServerTransport { if (trimmed.length > 0) this.handlers.onLog(trimmed); } } + +function killWindowsProcessTree(pid: number): void { + const killer = spawn("taskkill", ["/pid", String(pid), "/t", "/f"], { + stdio: "ignore", + windowsHide: true, + }); + killer.on("error", () => undefined); +} diff --git a/tests/app-server/transport-windows-launch.test.ts b/tests/app-server/transport-windows-launch.test.ts new file mode 100644 index 00000000..ef8fd0f6 --- /dev/null +++ b/tests/app-server/transport-windows-launch.test.ts @@ -0,0 +1,83 @@ +import { EventEmitter } from "node:events"; +import { PassThrough } from "node:stream"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const spawnMock = vi.fn(); + +vi.mock("node:child_process", () => ({ + spawn: spawnMock, +})); + +describe("StdioAppServerTransport Windows launch", () => { + const originalPlatform = process.platform; + + afterEach(() => { + Object.defineProperty(process, "platform", { value: originalPlatform }); + vi.resetModules(); + spawnMock.mockReset(); + vi.unstubAllEnvs(); + }); + + it("starts Windows command shims through cmd.exe", async () => { + Object.defineProperty(process, "platform", { value: "win32" }); + vi.stubEnv("ComSpec", String.raw`C:\Windows\System32\cmd.exe`); + const { StdioAppServerTransport } = await import("../../src/app-server/connection/transport"); + const codexCmd = String.raw`C:\Users\me\AppData\Roaming\npm\codex.cmd`; + const cwd = String.raw`C:\vault`; + + spawnMock.mockReturnValue(fakeChildProcess()); + + new StdioAppServerTransport(codexCmd, cwd, { + onLine: vi.fn(), + onLog: vi.fn(), + onExit: vi.fn(), + onError: vi.fn(), + }).start(); + + expect(spawnMock).toHaveBeenCalledWith(String.raw`C:\Windows\System32\cmd.exe`, ["/d", "/c", `"${codexCmd}" app-server`], { + cwd, + stdio: ["pipe", "pipe", "pipe"], + }); + }); + + it("kills the cmd.exe process tree when stopping Windows command shim launches", async () => { + Object.defineProperty(process, "platform", { value: "win32" }); + vi.stubEnv("ComSpec", String.raw`C:\Windows\System32\cmd.exe`); + const { StdioAppServerTransport } = await import("../../src/app-server/connection/transport"); + const codexCmd = String.raw`C:\Users\me\AppData\Roaming\npm\codex.cmd`; + const cmdProcess = fakeChildProcess({ pid: 4242 }); + + spawnMock.mockReturnValue(cmdProcess); + + const transport = new StdioAppServerTransport(codexCmd, String.raw`C:\vault`, { + onLine: vi.fn(), + onLog: vi.fn(), + onExit: vi.fn(), + onError: vi.fn(), + }); + transport.start(); + transport.stop(); + + expect(cmdProcess.kill).not.toHaveBeenCalled(); + expect(spawnMock).toHaveBeenLastCalledWith("taskkill", ["/pid", "4242", "/t", "/f"], { + stdio: "ignore", + windowsHide: true, + }); + }); +}); + +function fakeChildProcess(options: { pid?: number } = {}) { + const stdin = Object.assign(new EventEmitter(), { + destroyed: false, + writable: true, + write: vi.fn(), + }); + return Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + stdin, + killed: false, + kill: vi.fn(), + pid: options.pid, + }); +} diff --git a/tests/app-server/transport.test.ts b/tests/app-server/transport.test.ts index 66ec97f9..0498dadb 100644 --- a/tests/app-server/transport.test.ts +++ b/tests/app-server/transport.test.ts @@ -1,12 +1,46 @@ import { describe, expect, it, vi } from "vitest"; -import { type AppServerTransportHandlers, StdioAppServerTransport } from "../../src/app-server/connection/transport"; +import { + type AppServerTransportHandlers, + createAppServerSpawnSpec, + StdioAppServerTransport, +} from "../../src/app-server/connection/transport"; interface TestableTransport { handleStderr(text: string): void; flushStderr(): void; } +describe("createAppServerSpawnSpec", () => { + it("starts regular commands directly", () => { + expect(createAppServerSpawnSpec("codex", { platform: "darwin" })).toEqual({ + command: "codex", + args: ["app-server"], + killProcessTreeOnStop: false, + }); + }); + + it("starts Windows executables directly", () => { + const codexExe = String.raw`C:\Program Files\Codex\codex.exe`; + + expect(createAppServerSpawnSpec(codexExe, { platform: "win32" })).toEqual({ + command: codexExe, + args: ["app-server"], + killProcessTreeOnStop: false, + }); + }); + + it("quotes Windows command shim paths that contain spaces", () => { + const codexBat = String.raw`C:\Program Files\nodejs\codex.bat`; + + expect(createAppServerSpawnSpec(codexBat, { platform: "win32", comSpec: " " })).toEqual({ + command: "cmd.exe", + args: ["/d", "/c", `"${codexBat}" app-server`], + killProcessTreeOnStop: true, + }); + }); +}); + describe("StdioAppServerTransport", () => { it("emits complete stderr lines and flushes the final partial line", () => { const { onLog, transport } = transportFixture();