diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f004f107f..95ead56e3 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed npm-family package commands on Windows to avoid shell argument splitting when install prefixes contain spaces ([#4623](https://github.com/earendil-works/pi/issues/4623)). + ## [0.75.0] - 2026-05-17 ### Breaking Changes diff --git a/packages/coding-agent/src/config.ts b/packages/coding-agent/src/config.ts index bf268a77a..901bd2968 100644 --- a/packages/coding-agent/src/config.ts +++ b/packages/coding-agent/src/config.ts @@ -3,7 +3,7 @@ import { accessSync, constants, existsSync, readFileSync, realpathSync } from "f import { homedir } from "os"; import { basename, dirname, join, resolve, sep, win32 } from "path"; import { fileURLToPath } from "url"; -import { shouldUseWindowsShell } from "./utils/child-process.js"; +import { resolveSpawnCommand } from "./utils/child-process.js"; // ============================================================================= // Package Detection @@ -151,10 +151,18 @@ function readCommandOutput( args: string[], options: { requireSuccess?: boolean } = {}, ): string | undefined { - const result = spawnSync(command, args, { + let resolved: ReturnType; + try { + resolved = resolveSpawnCommand(command, args); + } catch (error) { + if (options.requireSuccess) { + throw error; + } + return undefined; + } + const result = spawnSync(resolved.command, resolved.args, { encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"], - shell: shouldUseWindowsShell(command), }); if (result.status === 0) return result.stdout.trim() || undefined; if (options.requireSuccess) { diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 92f9bedee..53ff6fea0 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -28,7 +28,7 @@ import { globSync } from "glob"; import ignore from "ignore"; import { minimatch } from "minimatch"; import { CONFIG_DIR_NAME } from "../config.js"; -import { shouldUseWindowsShell } from "../utils/child-process.js"; +import { resolveSpawnCommand } from "../utils/child-process.js"; import { type GitSource, parseGitUrl } from "../utils/git.js"; import { canonicalizePath, isLocalPath } from "../utils/paths.js"; import { isStdoutTakenOver } from "./output-guard.js"; @@ -2408,11 +2408,12 @@ export class DefaultPackageManager implements PackageManager { } private spawnCommand(command: string, args: string[], options?: { cwd?: string }): ChildProcess { - return spawn(command, args, { + const env = getEnv(); + const resolved = resolveSpawnCommand(command, args, { env }); + return spawn(resolved.command, resolved.args, { cwd: options?.cwd, stdio: isStdoutTakenOver() ? ["ignore", 2, 2] : "inherit", - shell: shouldUseWindowsShell(command), - env: getEnv(), + env, }); } @@ -2422,11 +2423,12 @@ export class DefaultPackageManager implements PackageManager { options?: { cwd?: string; env?: Record }, ): ChildProcessByStdio { const baseEnv = getEnv(); - return spawn(command, args, { + const env = options?.env ? { ...baseEnv, ...options.env } : baseEnv; + const resolved = resolveSpawnCommand(command, args, { env }); + return spawn(resolved.command, resolved.args, { cwd: options?.cwd, stdio: ["ignore", "pipe", "pipe"], - shell: shouldUseWindowsShell(command), - env: options?.env ? { ...baseEnv, ...options.env } : baseEnv, + env, }); } @@ -2489,11 +2491,12 @@ export class DefaultPackageManager implements PackageManager { } private runCommandSync(command: string, args: string[]): string { - const result = spawnSync(command, args, { + const env = getEnv(); + const resolved = resolveSpawnCommand(command, args, { env }); + const result = spawnSync(resolved.command, resolved.args, { stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8", - shell: shouldUseWindowsShell(command), - env: getEnv(), + env, }); if (result.error || result.status !== 0) { throw new Error( diff --git a/packages/coding-agent/src/package-manager-cli.ts b/packages/coding-agent/src/package-manager-cli.ts index a41fb7a6c..f421af322 100644 --- a/packages/coding-agent/src/package-manager-cli.ts +++ b/packages/coding-agent/src/package-manager-cli.ts @@ -12,7 +12,7 @@ import { } from "./config.js"; import { DefaultPackageManager } from "./core/package-manager.js"; import { SettingsManager } from "./core/settings-manager.js"; -import { shouldUseWindowsShell } from "./utils/child-process.js"; +import { resolveSpawnCommand } from "./utils/child-process.js"; import { getLatestPiRelease, isNewerPackageVersion } from "./utils/version-check.js"; export type PackageCommand = "install" | "remove" | "update" | "list"; @@ -316,10 +316,9 @@ async function runSelfUpdate(command: SelfUpdateCommand): Promise { console.log(chalk.dim(`Updating ${APP_NAME} with ${command.display}...`)); for (const step of command.steps ?? [command]) { await new Promise((resolve, reject) => { - // Windows package managers are commonly .cmd shims. Use the shell so Node can execute them. - const child = spawn(step.command, step.args, { + const resolved = resolveSpawnCommand(step.command, step.args); + const child = spawn(resolved.command, resolved.args, { stdio: "inherit", - shell: shouldUseWindowsShell(step.command), }); child.on("error", (error) => { reject(error); diff --git a/packages/coding-agent/src/utils/child-process.ts b/packages/coding-agent/src/utils/child-process.ts index 9f75a285d..73fdf5af0 100644 --- a/packages/coding-agent/src/utils/child-process.ts +++ b/packages/coding-agent/src/utils/child-process.ts @@ -1,14 +1,82 @@ import type { ChildProcess } from "node:child_process"; -import { basename } from "node:path"; +import { existsSync, readFileSync } from "node:fs"; +import { dirname, extname, join, resolve, sep } from "node:path"; const EXIT_STDIO_GRACE_MS = 100; +const WINDOWS_COMMAND_EXTENSIONS = ["", ".exe", ".cmd", ".bat"]; +const WINDOWS_COMMAND_SHIM_RE = /\.(?:cmd|bat)$/i; +const NODE_SHIM_SCRIPT_RE = /(?:%~dp0|%dp0%|%basedir%)[^"'\r\n<>|&]*?\.(?:cjs|mjs|js)/i; -const WINDOWS_SHELL_COMMANDS = new Set(["npm", "npx", "pnpm", "yarn", "yarnpkg", "corepack"]); +export interface ResolvedSpawnCommand { + command: string; + args: string[]; +} -export function shouldUseWindowsShell(command: string): boolean { - if (process.platform !== "win32") return false; - const commandName = basename(command).toLowerCase(); - return commandName.endsWith(".cmd") || commandName.endsWith(".bat") || WINDOWS_SHELL_COMMANDS.has(commandName); +function findWindowsCommand(command: string, env: NodeJS.ProcessEnv): string | undefined { + const candidates = extname(command) + ? [command] + : WINDOWS_COMMAND_EXTENSIONS.map((extension) => `${command}${extension}`); + const hasPath = command.includes("/") || command.includes("\\") || /^[a-zA-Z]:/.test(command); + if (hasPath) { + const match = candidates.find((candidate) => existsSync(candidate)); + return match ? resolve(match) : undefined; + } + + const pathValue = env.PATH ?? env.Path ?? env.path; + if (!pathValue) return undefined; + for (const dir of pathValue.split(";")) { + for (const candidate of candidates) { + const path = join(dir, candidate); + if (existsSync(path)) return resolve(path); + } + } + return undefined; +} + +function expandShimPath(path: string, shimPath: string): string { + const shimDir = dirname(shimPath); + return resolve( + path + .replace(/%~dp0[\\/]?/gi, `${shimDir}${sep}`) + .replace(/%dp0%[\\/]?/gi, `${shimDir}${sep}`) + .replace(/%basedir%[\\/]?/gi, `${shimDir}${sep}`) + .replace(/\\/g, sep), + ); +} + +function findNodeShimScript(shimPath: string): string | undefined { + const match = readFileSync(shimPath, "utf-8").match(NODE_SHIM_SCRIPT_RE); + if (!match) return undefined; + const scriptPath = expandShimPath(match[0], shimPath); + return existsSync(scriptPath) ? scriptPath : undefined; +} + +export function resolveSpawnCommand( + command: string, + args: string[], + options: { env?: NodeJS.ProcessEnv } = {}, +): ResolvedSpawnCommand { + if (process.platform !== "win32") { + return { command, args }; + } + + const env = options.env ?? process.env; + const resolvedCommand = findWindowsCommand(command, env); + if (!resolvedCommand) { + return { command, args }; + } + + if (!WINDOWS_COMMAND_SHIM_RE.test(resolvedCommand)) { + return { command: resolvedCommand, args }; + } + + const script = findNodeShimScript(resolvedCommand); + if (!script) { + throw new Error(`Refusing to run Windows command shim without a shell: ${resolvedCommand}`); + } + const localNode = join(dirname(resolvedCommand), "node.exe"); + const nodeCommand = existsSync(localNode) ? localNode : (findWindowsCommand("node.exe", env) ?? "node"); + return { command: nodeCommand, args: [script, ...args] }; } /** diff --git a/packages/coding-agent/test/package-manager.test.ts b/packages/coding-agent/test/package-manager.test.ts index edd2ce959..3cdf8cb0e 100644 --- a/packages/coding-agent/test/package-manager.test.ts +++ b/packages/coding-agent/test/package-manager.test.ts @@ -6,7 +6,7 @@ import { PassThrough } from "node:stream"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { DefaultPackageManager, type ProgressEvent, type ResolvedResource } from "../src/core/package-manager.js"; import { SettingsManager } from "../src/core/settings-manager.js"; -import { shouldUseWindowsShell } from "../src/utils/child-process.js"; +import { resolveSpawnCommand } from "../src/utils/child-process.js"; function normalizeForMatch(value: string): string { return value.replace(/\\/g, "/"); @@ -618,13 +618,58 @@ Content`, }); describe("windows command spawning", () => { - it("should avoid the shell for git so Windows paths with spaces stay single arguments", () => { + it("should keep unresolved executables as argv commands", () => { vi.spyOn(process, "platform", "get").mockReturnValue("win32"); - expect(shouldUseWindowsShell("git")).toBe(false); - expect(shouldUseWindowsShell("npm")).toBe(true); - expect(shouldUseWindowsShell("pnpm")).toBe(true); - expect(shouldUseWindowsShell("C:/Program Files/nodejs/npm.cmd")).toBe(true); + const resolved = resolveSpawnCommand("git", ["clone", "repo", "C:\\Users\\A B\\repo"], { + env: { PATH: tempDir }, + }); + + expect(resolved).toEqual({ command: "git", args: ["clone", "repo", "C:\\Users\\A B\\repo"] }); + }); + + it("should prefer Windows executables over command scripts", () => { + vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + const binDir = join(tempDir, "bin"); + mkdirSync(binDir, { recursive: true }); + writeFileSync(join(binDir, "npm.exe"), ""); + writeFileSync(join(binDir, "npm.cmd"), "@echo off\r\n"); + + const args = ["install", "pkg", "--prefix", "C:\\Users\\A B\\.pi\\npm"]; + const resolved = resolveSpawnCommand("npm", args, { env: { PATH: binDir } }); + + expect(resolved).toEqual({ command: join(binDir, "npm.exe"), args }); + }); + + it("should resolve npm command shims to their Node entrypoint without a shell", () => { + vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + const binDir = join(tempDir, "node-bin"); + const npmCli = join(binDir, "node_modules", "npm", "bin", "npm-cli.js"); + mkdirSync(join(binDir, "node_modules", "npm", "bin"), { recursive: true }); + writeFileSync(join(binDir, "node.exe"), ""); + writeFileSync(npmCli, ""); + writeFileSync( + join(binDir, "npm.cmd"), + '@ECHO off\r\nSET "dp0=%~dp0"\r\nSET "_prog=%dp0%\\node.exe"\r\nendLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & "%_prog%" "%dp0%\\node_modules\\npm\\bin\\npm-cli.js" %*\r\n', + ); + + const args = ["install", "pkg", "--prefix", "C:\\Users\\A B\\.pi\\npm"]; + const resolved = resolveSpawnCommand("npm", args, { env: { PATH: binDir } }); + + expect(resolved).toEqual({ command: join(binDir, "node.exe"), args: [npmCli, ...args] }); + }); + + it("should reject unrecognized Windows command scripts instead of using cmd.exe", () => { + vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + const binDir = join(tempDir, "bin"); + mkdirSync(binDir, { recursive: true }); + writeFileSync(join(binDir, "npm.cmd"), "@echo off\r\necho %*\r\n"); + + expect(() => + resolveSpawnCommand("npm", ["install", "pkg"], { + env: { PATH: binDir }, + }), + ).toThrow("Refusing to run Windows command shim without a shell"); }); });