diff --git a/src/commands/hooks.ts b/src/commands/hooks.ts index 1d7b55c9a..b257ef990 100644 --- a/src/commands/hooks.ts +++ b/src/commands/hooks.ts @@ -601,7 +601,7 @@ export function buildCodexWrapperSpawn( agentArgs: string[], platform: NodeJS.Platform = process.platform, env: NodeJS.ProcessEnv = process.env, -): { binary: string; args: string[] } { +): { binary: string; args: string[]; windowsVerbatimArguments?: boolean } { const childArgs = buildCodexWrapperChildArgs(agentArgs); if (!shouldUseWindowsCmdLaunch(realBinary, platform, env)) { return { @@ -612,6 +612,7 @@ export function buildCodexWrapperSpawn( return { binary: resolveWindowsComSpec(env), args: buildWindowsCmdArgsArray(realBinary, childArgs), + windowsVerbatimArguments: true, }; } @@ -663,6 +664,7 @@ async function runCodexWrapperSubcommand(wrapperArgs: CodexWrapperArgs): Promise const child = spawn(childLaunch.binary, childLaunch.args, { stdio: "inherit", env: childEnv, + windowsVerbatimArguments: childLaunch.windowsVerbatimArguments, }); const forwardSignal = (signal: NodeJS.Signals) => { diff --git a/src/core/windows-cmd-launch.ts b/src/core/windows-cmd-launch.ts index c4073f8b5..1875f04f5 100644 --- a/src/core/windows-cmd-launch.ts +++ b/src/core/windows-cmd-launch.ts @@ -6,6 +6,16 @@ const WINDOWS_CMD_EXTENSIONS = new Set([".cmd", ".bat"]); const WINDOWS_DIRECT_EXTENSIONS = new Set([".exe", ".com"]); const DEFAULT_WINDOWS_PATHEXT = [".COM", ".EXE", ".BAT", ".CMD"]; +export interface ResolvedWindowsBinary { + path: string; + extension: string; +} + +export interface WindowsLaunchDecision { + useWindowsShellLaunch: boolean; + resolvedBinary: ResolvedWindowsBinary | null; +} + // `process.env` behaves case-insensitively on Windows, but once we copy env into a // plain object for child-process merging we need to preserve that behavior ourselves. function getWindowsEnvValue(env: NodeJS.ProcessEnv, key: string): string | undefined { @@ -54,24 +64,35 @@ function getWindowsPathExtensions(env: NodeJS.ProcessEnv): string[] { return configured; } -function resolveWindowsBinaryExtension(binary: string, env: NodeJS.ProcessEnv): string | null { +export function resolveWindowsBinary( + binary: string, + env: NodeJS.ProcessEnv = process.env, +): ResolvedWindowsBinary | null { const trimmed = binary.trim(); if (!trimmed) { return null; } const extension = extname(trimmed); - if (extension) { - return extension.toLowerCase(); - } - const pathExtensions = getWindowsPathExtensions(env); const hasDirectorySeparators = trimmed.includes("\\") || trimmed.includes("/"); if (hasDirectorySeparators) { + if (extension && canAccessPath(trimmed)) { + return { + path: trimmed, + extension: extension.toLowerCase(), + }; + } + if (extension) { + return null; + } for (const pathExtension of pathExtensions) { const candidate = `${trimmed}${pathExtension}`; if (canAccessPath(candidate)) { - return pathExtension.toLowerCase(); + return { + path: candidate, + extension: pathExtension.toLowerCase(), + }; } } return null; @@ -86,10 +107,14 @@ function resolveWindowsBinaryExtension(binary: string, env: NodeJS.ProcessEnv): } for (const pathEntry of pathEntries) { - for (const pathExtension of pathExtensions) { - const candidate = join(pathEntry, `${trimmed}${pathExtension}`); + const candidates = extension ? [trimmed] : pathExtensions.map((pathExtension) => `${trimmed}${pathExtension}`); + for (const candidateName of candidates) { + const candidate = join(pathEntry, candidateName); if (canAccessPath(candidate)) { - return pathExtension.toLowerCase(); + return { + path: candidate, + extension: extname(candidate).toLowerCase(), + }; } } } @@ -132,40 +157,76 @@ export function buildWindowsCmdArgsArray(binary: string, args: string[]): string return ["/d", "/s", "/c", `"${shellCommand}"`]; } -export function shouldUseWindowsCmdLaunch( +export function resolveWindowsLaunchDecision( binary: string, platform: NodeJS.Platform = process.platform, env: NodeJS.ProcessEnv = process.env, -): boolean { +): WindowsLaunchDecision { if (platform !== "win32") { - return false; + return { + useWindowsShellLaunch: false, + resolvedBinary: null, + }; } + const normalized = binary.trim().toLowerCase(); if (!normalized) { - return false; + return { + useWindowsShellLaunch: false, + resolvedBinary: null, + }; } if (normalized === "cmd" || normalized === "cmd.exe") { - return false; + return { + useWindowsShellLaunch: false, + resolvedBinary: null, + }; } if (normalized === resolveWindowsComSpec(env).toLowerCase()) { - return false; + return { + useWindowsShellLaunch: false, + resolvedBinary: null, + }; } const explicitExtension = extname(normalized).toLowerCase(); if (WINDOWS_CMD_EXTENSIONS.has(explicitExtension)) { - return true; + return { + useWindowsShellLaunch: true, + resolvedBinary: null, + }; } if (WINDOWS_DIRECT_EXTENSIONS.has(explicitExtension)) { - return false; - } - - const resolvedExtension = resolveWindowsBinaryExtension(binary, env); - if (resolvedExtension && WINDOWS_DIRECT_EXTENSIONS.has(resolvedExtension)) { - return false; - } - if (resolvedExtension && WINDOWS_CMD_EXTENSIONS.has(resolvedExtension)) { - return true; - } + return { + useWindowsShellLaunch: false, + resolvedBinary: resolveWindowsBinary(binary, env), + }; + } + + const resolvedBinary = resolveWindowsBinary(binary, env); + if (resolvedBinary && WINDOWS_DIRECT_EXTENSIONS.has(resolvedBinary.extension)) { + return { + useWindowsShellLaunch: false, + resolvedBinary, + }; + } + if (resolvedBinary && WINDOWS_CMD_EXTENSIONS.has(resolvedBinary.extension)) { + return { + useWindowsShellLaunch: true, + resolvedBinary: null, + }; + } + + return { + useWindowsShellLaunch: true, + resolvedBinary: null, + }; +} - return true; +export function shouldUseWindowsCmdLaunch( + binary: string, + platform: NodeJS.Platform = process.platform, + env: NodeJS.ProcessEnv = process.env, +): boolean { + return resolveWindowsLaunchDecision(binary, platform, env).useWindowsShellLaunch; } diff --git a/src/terminal/pty-session.ts b/src/terminal/pty-session.ts index 44171584f..a8488e029 100644 --- a/src/terminal/pty-session.ts +++ b/src/terminal/pty-session.ts @@ -3,7 +3,7 @@ import * as pty from "node-pty"; import { buildWindowsCmdArgsCommandLine, resolveWindowsComSpec, - shouldUseWindowsCmdLaunch, + resolveWindowsLaunchDecision, } from "../core/windows-cmd-launch"; export interface PtyExitEvent { @@ -87,8 +87,12 @@ export class PtySession { const normalizedArgs = typeof args === "string" ? [args] : args; const terminalName = env?.TERM?.trim() || process.env.TERM?.trim() || "xterm-256color"; const launchEnv: NodeJS.ProcessEnv = env ? { ...process.env, ...env } : process.env; - const useWindowsShellLaunch = shouldUseWindowsCmdLaunch(binary, process.platform, launchEnv); - const spawnBinary = useWindowsShellLaunch ? resolveWindowsComSpec(launchEnv) : binary; + const windowsLaunchDecision = resolveWindowsLaunchDecision(binary, process.platform, launchEnv); + const useWindowsShellLaunch = windowsLaunchDecision.useWindowsShellLaunch; + const resolvedWindowsBinary = windowsLaunchDecision.resolvedBinary; + const spawnBinary = useWindowsShellLaunch + ? resolveWindowsComSpec(launchEnv) + : (resolvedWindowsBinary?.path ?? binary); const spawnArgs = useWindowsShellLaunch ? buildWindowsCmdArgsCommandLine(binary, normalizedArgs) : normalizedArgs; const ptyOptions: pty.IPtyForkOptions = { name: terminalName, diff --git a/test/runtime/core/windows-cmd-launch.test.ts b/test/runtime/core/windows-cmd-launch.test.ts index babf94fd3..915ab4e05 100644 --- a/test/runtime/core/windows-cmd-launch.test.ts +++ b/test/runtime/core/windows-cmd-launch.test.ts @@ -3,7 +3,11 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { shouldUseWindowsCmdLaunch } from "../../../src/core/windows-cmd-launch"; +import { + resolveWindowsBinary, + resolveWindowsLaunchDecision, + shouldUseWindowsCmdLaunch, +} from "../../../src/core/windows-cmd-launch"; function createWindowsBinary(directory: string, fileName: string): string { const filePath = join(directory, fileName); @@ -29,6 +33,26 @@ describe("shouldUseWindowsCmdLaunch", () => { expect(shouldUseWindowsCmdLaunch("codex.exe", "win32")).toBe(false); }); + it("resolves explicit .exe binaries from PATH", () => { + const tempDirectory = mkdtempSync(join(tmpdir(), "kanban-win-launch-")); + tempDirectories.push(tempDirectory); + const binaryPath = createWindowsBinary(tempDirectory, "codex.exe"); + + expect( + resolveWindowsLaunchDecision("codex.exe", "win32", { + PATH: tempDirectory, + PATHEXT: ".com;.exe;.bat;.cmd", + ComSpec: "C:\\Windows\\System32\\cmd.exe", + }), + ).toEqual({ + useWindowsShellLaunch: false, + resolvedBinary: { + path: binaryPath, + extension: ".exe", + }, + }); + }); + it("returns true for explicit .cmd shims", () => { expect(shouldUseWindowsCmdLaunch("codex.cmd", "win32")).toBe(true); }); @@ -36,7 +60,7 @@ describe("shouldUseWindowsCmdLaunch", () => { it("returns false when PATH resolves a bare binary to .exe", () => { const tempDirectory = mkdtempSync(join(tmpdir(), "kanban-win-launch-")); tempDirectories.push(tempDirectory); - createWindowsBinary(tempDirectory, "codex.exe"); + const binaryPath = createWindowsBinary(tempDirectory, "codex.exe"); expect( shouldUseWindowsCmdLaunch("codex", "win32", { @@ -45,6 +69,39 @@ describe("shouldUseWindowsCmdLaunch", () => { ComSpec: "C:\\Windows\\System32\\cmd.exe", }), ).toBe(false); + expect( + resolveWindowsBinary("codex", { + PATH: tempDirectory, + PATHEXT: ".com;.exe;.bat;.cmd", + }), + ).toEqual({ + path: binaryPath, + extension: ".exe", + }); + expect( + resolveWindowsLaunchDecision("codex", "win32", { + PATH: tempDirectory, + PATHEXT: ".com;.exe;.bat;.cmd", + ComSpec: "C:\\Windows\\System32\\cmd.exe", + }), + ).toEqual({ + useWindowsShellLaunch: false, + resolvedBinary: { + path: binaryPath, + extension: ".exe", + }, + }); + }); + + it("does not append PATHEXT to path-qualified binaries with explicit extensions", () => { + const tempDirectory = mkdtempSync(join(tmpdir(), "kanban-win-launch-")); + tempDirectories.push(tempDirectory); + + expect( + resolveWindowsBinary(join(tempDirectory, "missing.exe"), { + PATHEXT: ".com;.exe;.bat;.cmd", + }), + ).toBeNull(); }); it("treats Windows env keys case-insensitively when PATH resolves a bare binary to .exe", () => { diff --git a/test/runtime/hooks-codex-wrapper.test.ts b/test/runtime/hooks-codex-wrapper.test.ts index 9d6eebd0a..d9bd13fd8 100644 --- a/test/runtime/hooks-codex-wrapper.test.ts +++ b/test/runtime/hooks-codex-wrapper.test.ts @@ -1,7 +1,20 @@ -import { describe, expect, it } from "vitest"; +import { spawnSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; import { buildCodexWrapperChildArgs, buildCodexWrapperSpawn } from "../../src/commands/hooks"; +const tempDirectories: string[] = []; + +afterEach(() => { + for (const directory of tempDirectories) { + rmSync(directory, { recursive: true, force: true }); + } + tempDirectories.length = 0; +}); + describe("buildCodexWrapperChildArgs", () => { it("injects notify config", () => { const args = buildCodexWrapperChildArgs(["exec", "fix the bug"]); @@ -33,6 +46,7 @@ describe("buildCodexWrapperChildArgs", () => { expect(launch.args[2]).toBe("/c"); expect(launch.args[3]).toContain("codex"); expect(launch.args[3]).toContain("exec"); + expect(launch.windowsVerbatimArguments).toBe(true); }); it("does not wrap cmd itself on Windows and still applies notify fallback args", () => { @@ -44,5 +58,30 @@ describe("buildCodexWrapperChildArgs", () => { expect(launch.args[0]).toBe("-c"); expect(launch.args[1]).toContain("notify="); expect(launch.args.slice(2)).toEqual(["/c", "echo hi"]); + expect(launch.windowsVerbatimArguments).toBeUndefined(); + }); + + it.skipIf(process.platform !== "win32")("launches npm cmd shims without quote mangling", () => { + const shimDirectory = mkdtempSync(join(tmpdir(), "kanban-codex-wrapper-")); + tempDirectories.push(shimDirectory); + writeFileSync(join(shimDirectory, "codex.cmd"), "@echo off\r\necho fake-codex-ok\r\nexit /b 0\r\n"); + + const env: NodeJS.ProcessEnv = { + ...process.env, + ComSpec: process.env.ComSpec ?? process.env.COMSPEC ?? "C:\\Windows\\System32\\cmd.exe", + PATH: `${shimDirectory};${process.env.PATH ?? ""}`, + PATHEXT: ".COM;.EXE;.BAT;.CMD", + }; + const launch = buildCodexWrapperSpawn("codex", ["--version"], "win32", env); + const result = spawnSync(launch.binary, launch.args, { + encoding: "utf8", + env, + windowsVerbatimArguments: launch.windowsVerbatimArguments, + }); + + expect(result.error).toBeUndefined(); + expect(result.status).toBe(0); + expect(result.stdout).toContain("fake-codex-ok"); + expect(result.stderr).toBe(""); }); }); diff --git a/test/runtime/terminal/pty-session.test.ts b/test/runtime/terminal/pty-session.test.ts index 8aef0533f..d52083022 100644 --- a/test/runtime/terminal/pty-session.test.ts +++ b/test/runtime/terminal/pty-session.test.ts @@ -155,7 +155,7 @@ describe("PtySession", () => { } expect(ptyMocks.spawn).toHaveBeenCalledTimes(1); - expect(ptyMocks.spawn.mock.calls[0]?.[0]).toBe("codex"); + expect(ptyMocks.spawn.mock.calls[0]?.[0]).toBe(join(windowsBinDir, "codex.exe")); expect(ptyMocks.spawn.mock.calls[0]?.[1]).toEqual(["--foo", "bar"]); }); @@ -216,7 +216,7 @@ describe("PtySession", () => { }); expect(ptyMocks.spawn).toHaveBeenCalledTimes(1); - expect(ptyMocks.spawn.mock.calls[0]?.[0]).toBe("cmd.exe"); + expect(ptyMocks.spawn.mock.calls[0]?.[0].toLowerCase()).toMatch(/(?:^|[\\/])cmd\.exe$/u); }); it("ignores resize calls after the pty has exited", () => {