From 64150668b882f50bf2dc73557e154847f1f9d11f Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Mon, 18 May 2026 11:30:41 +0200 Subject: [PATCH] fix(coding-agent): use cross-spawn for Windows shims closes #4665 --- package-lock.json | 72 +++++++++++- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/package.json | 2 + packages/coding-agent/src/config.ts | 14 +-- .../coding-agent/src/core/package-manager.ts | 13 +-- .../coding-agent/src/package-manager-cli.ts | 6 +- .../coding-agent/src/utils/child-process.ts | 104 +++++------------- .../coding-agent/test/package-manager.test.ts | 65 ++--------- 8 files changed, 124 insertions(+), 153 deletions(-) diff --git a/package-lock.json b/package-lock.json index 47505733..b0c47a95 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3162,6 +3162,16 @@ "assertion-error": "^2.0.1" } }, + "node_modules/@types/cross-spawn": { + "version": "6.0.6", + "resolved": "https://registry.npmjs.org/@types/cross-spawn/-/cross-spawn-6.0.6.tgz", + "integrity": "sha512-fXRhhUkG4H3TQk5dBhQ7m/JDdSNHKwR2BBia62lhwEIq9xGiQKLxd6LymNhn47SjXhsUEPmxi+PKw2OkW4LLjA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/deep-eql": { "version": "4.0.2", "resolved": "https://registry.npmjs.org/@types/deep-eql/-/deep-eql-4.0.2.tgz", @@ -4796,7 +4806,6 @@ "version": "2.0.0", "resolved": "https://registry.npmjs.org/isexe/-/isexe-2.0.0.tgz", "integrity": "sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw==", - "dev": true, "license": "ISC" }, "node_modules/istanbul-lib-coverage": { @@ -7860,6 +7869,7 @@ "@earendil-works/pi-tui": "^0.75.1", "@silvia-odwyer/photon-node": "^0.3.4", "chalk": "^5.5.0", + "cross-spawn": "^7.0.6", "diff": "^8.0.2", "glob": "^13.0.1", "highlight.js": "^10.7.3", @@ -7876,6 +7886,7 @@ "pi": "dist/cli.js" }, "devDependencies": { + "@types/cross-spawn": "^6.0.6", "@types/diff": "^7.0.2", "@types/hosted-git-info": "^3.0.5", "@types/ms": "^2.1.0", @@ -7939,6 +7950,50 @@ "undici-types": "~7.16.0" } }, + "packages/coding-agent/node_modules/cross-spawn": { + "version": "7.0.6", + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.6.tgz", + "integrity": "sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==", + "license": "MIT", + "dependencies": { + "path-key": "^3.1.0", + "shebang-command": "^2.0.0", + "which": "^2.0.1" + }, + "engines": { + "node": ">= 8" + } + }, + "packages/coding-agent/node_modules/path-key": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/path-key/-/path-key-3.1.1.tgz", + "integrity": "sha512-ojmeN0qd+y0jszEtoY48r0Peq5dwMEkIlCOu6Q5f41lfkswXuKtYrhgoTpLnyIcHm24Uhqx+5Tqm2InSwLhE6Q==", + "license": "MIT", + "engines": { + "node": ">=8" + } + }, + "packages/coding-agent/node_modules/shebang-command": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/shebang-command/-/shebang-command-2.0.0.tgz", + "integrity": "sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==", + "license": "MIT", + "dependencies": { + "shebang-regex": "^3.0.0" + }, + "engines": { + "node": ">=8" + } + }, + "packages/coding-agent/node_modules/shebang-regex": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/shebang-regex/-/shebang-regex-3.0.0.tgz", + "integrity": "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==", + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "packages/coding-agent/node_modules/undici-types": { "version": "7.16.0", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.16.0.tgz", @@ -7946,6 +8001,21 @@ "dev": true, "license": "MIT" }, + "packages/coding-agent/node_modules/which": { + "version": "2.0.2", + "resolved": "https://registry.npmjs.org/which/-/which-2.0.2.tgz", + "integrity": "sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA==", + "license": "ISC", + "dependencies": { + "isexe": "^2.0.0" + }, + "bin": { + "node-which": "bin/node-which" + }, + "engines": { + "node": ">= 8" + } + }, "packages/tui": { "name": "@earendil-works/pi-tui", "version": "0.75.1", diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a9e4a552..543af174 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -9,6 +9,7 @@ - Fixed Windows npm self-updates to move loaded native dependency packages out of the active install before reinstalling pi ([#4157](https://github.com/earendil-works/pi/issues/4157)). - Fixed `pi update --self` detection for pnpm v11 global installs whose package path resolves through the pnpm store ([#4647](https://github.com/earendil-works/pi/issues/4647)). - Fixed Windows pnpm self-updates to resolve pnpm command shims and run through pnpm instead of requiring manual updates ([#4157](https://github.com/earendil-works/pi/issues/4157)). +- Fixed Windows npm-family command execution to use cross-spawn instead of parsing `.cmd` shim internals ([#4665](https://github.com/earendil-works/pi/issues/4665)). ## [0.75.1] - 2026-05-18 diff --git a/packages/coding-agent/package.json b/packages/coding-agent/package.json index 31d1c95d..5171b159 100644 --- a/packages/coding-agent/package.json +++ b/packages/coding-agent/package.json @@ -43,6 +43,7 @@ "@earendil-works/pi-tui": "^0.75.1", "@silvia-odwyer/photon-node": "^0.3.4", "chalk": "^5.5.0", + "cross-spawn": "^7.0.6", "diff": "^8.0.2", "glob": "^13.0.1", "highlight.js": "^10.7.3", @@ -65,6 +66,7 @@ "@mariozechner/clipboard": "^0.3.6" }, "devDependencies": { + "@types/cross-spawn": "^6.0.6", "@types/diff": "^7.0.2", "@types/hosted-git-info": "^3.0.5", "@types/ms": "^2.1.0", diff --git a/packages/coding-agent/src/config.ts b/packages/coding-agent/src/config.ts index ad306b4c..e2ac87e1 100644 --- a/packages/coding-agent/src/config.ts +++ b/packages/coding-agent/src/config.ts @@ -1,9 +1,8 @@ -import { spawnSync } from "child_process"; import { accessSync, constants, existsSync, readFileSync, realpathSync } from "fs"; import { homedir } from "os"; import { basename, dirname, join, resolve, sep, win32 } from "path"; import { fileURLToPath } from "url"; -import { resolveSpawnCommand } from "./utils/child-process.js"; +import { spawnProcessSync } from "./utils/child-process.js"; // ============================================================================= // Package Detection @@ -151,16 +150,7 @@ function readCommandOutput( args: string[], options: { requireSuccess?: boolean } = {}, ): string | undefined { - let resolved: ReturnType; - try { - resolved = resolveSpawnCommand(command, args); - } catch (error) { - if (options.requireSuccess) { - throw error; - } - return undefined; - } - const result = spawnSync(resolved.command, resolved.args, { + const result = spawnProcessSync(command, args, { encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"], }); diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 53ff6fea..04135cbf 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -1,4 +1,4 @@ -import { type ChildProcess, type ChildProcessByStdio, spawn, spawnSync } from "node:child_process"; +import type { ChildProcess, ChildProcessByStdio } from "node:child_process"; import { createHash } from "node:crypto"; import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; @@ -28,7 +28,7 @@ import { globSync } from "glob"; import ignore from "ignore"; import { minimatch } from "minimatch"; import { CONFIG_DIR_NAME } from "../config.js"; -import { resolveSpawnCommand } from "../utils/child-process.js"; +import { spawnProcess, spawnProcessSync } 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"; @@ -2409,8 +2409,7 @@ export class DefaultPackageManager implements PackageManager { private spawnCommand(command: string, args: string[], options?: { cwd?: string }): ChildProcess { const env = getEnv(); - const resolved = resolveSpawnCommand(command, args, { env }); - return spawn(resolved.command, resolved.args, { + return spawnProcess(command, args, { cwd: options?.cwd, stdio: isStdoutTakenOver() ? ["ignore", 2, 2] : "inherit", env, @@ -2424,8 +2423,7 @@ export class DefaultPackageManager implements PackageManager { ): ChildProcessByStdio { const baseEnv = getEnv(); const env = options?.env ? { ...baseEnv, ...options.env } : baseEnv; - const resolved = resolveSpawnCommand(command, args, { env }); - return spawn(resolved.command, resolved.args, { + return spawnProcess(command, args, { cwd: options?.cwd, stdio: ["ignore", "pipe", "pipe"], env, @@ -2492,8 +2490,7 @@ export class DefaultPackageManager implements PackageManager { private runCommandSync(command: string, args: string[]): string { const env = getEnv(); - const resolved = resolveSpawnCommand(command, args, { env }); - const result = spawnSync(resolved.command, resolved.args, { + const result = spawnProcessSync(command, args, { stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8", env, diff --git a/packages/coding-agent/src/package-manager-cli.ts b/packages/coding-agent/src/package-manager-cli.ts index d1a51e3b..a55a3750 100644 --- a/packages/coding-agent/src/package-manager-cli.ts +++ b/packages/coding-agent/src/package-manager-cli.ts @@ -1,5 +1,4 @@ import chalk from "chalk"; -import { spawn } from "child_process"; import { selectConfig } from "./cli/config-selector.js"; import { APP_NAME, @@ -14,7 +13,7 @@ import { } from "./config.js"; import { DefaultPackageManager } from "./core/package-manager.js"; import { SettingsManager } from "./core/settings-manager.js"; -import { resolveSpawnCommand } from "./utils/child-process.js"; +import { spawnProcess } from "./utils/child-process.js"; import { getLatestPiRelease, isNewerPackageVersion } from "./utils/version-check.js"; import { cleanupWindowsSelfUpdateQuarantine, @@ -322,8 +321,7 @@ 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) => { - const resolved = resolveSpawnCommand(step.command, step.args); - const child = spawn(resolved.command, resolved.args, { + const child = spawnProcess(step.command, step.args, { stdio: "inherit", }); child.on("error", (error) => { diff --git a/packages/coding-agent/src/utils/child-process.ts b/packages/coding-agent/src/utils/child-process.ts index 57ebd3ae..3d97d3da 100644 --- a/packages/coding-agent/src/utils/child-process.ts +++ b/packages/coding-agent/src/utils/child-process.ts @@ -1,84 +1,38 @@ -import type { ChildProcess } from "node:child_process"; -import { existsSync, readFileSync } from "node:fs"; -import { dirname, extname, join, resolve, sep } from "node:path"; +import { + type ChildProcess, + type ChildProcessByStdio, + spawn as nodeSpawn, + spawnSync as nodeSpawnSync, + type SpawnOptions, + type SpawnOptionsWithStdioTuple, + type SpawnSyncOptionsWithStringEncoding, + type SpawnSyncReturns, + type StdioNull, + type StdioPipe, +} from "node:child_process"; +import type { Readable } from "node:stream"; +import crossSpawn from "cross-spawn"; 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)/gi; -export interface ResolvedSpawnCommand { - command: string; - args: string[]; -} - -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 matches = [...readFileSync(shimPath, "utf-8").matchAll(NODE_SHIM_SCRIPT_RE)]; - for (const match of matches.reverse()) { - const scriptPath = expandShimPath(match[0], shimPath); - if (existsSync(scriptPath)) return scriptPath; - } - return undefined; -} - -export function resolveSpawnCommand( +export function spawnProcess( command: string, args: string[], - options: { env?: NodeJS.ProcessEnv } = {}, -): ResolvedSpawnCommand { - if (process.platform !== "win32") { - return { command, args }; - } + options: SpawnOptionsWithStdioTuple, +): ChildProcessByStdio; +export function spawnProcess(command: string, args: string[], options: SpawnOptions): ChildProcess; +export function spawnProcess(command: string, args: string[], options: SpawnOptions): ChildProcess { + return process.platform === "win32" ? crossSpawn(command, args, options) : nodeSpawn(command, args, options); +} - 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] }; +export function spawnProcessSync( + command: string, + args: string[], + options: SpawnSyncOptionsWithStringEncoding, +): SpawnSyncReturns { + return process.platform === "win32" + ? crossSpawn.sync(command, args, options) + : nodeSpawnSync(command, args, options); } /** diff --git a/packages/coding-agent/test/package-manager.test.ts b/packages/coding-agent/test/package-manager.test.ts index 3cdf8cb0..cc6afbc4 100644 --- a/packages/coding-agent/test/package-manager.test.ts +++ b/packages/coding-agent/test/package-manager.test.ts @@ -6,7 +6,6 @@ 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 { resolveSpawnCommand } from "../src/utils/child-process.js"; function normalizeForMatch(value: string): string { return value.replace(/\\/g, "/"); @@ -617,59 +616,19 @@ Content`, }); }); - describe("windows command spawning", () => { - it("should keep unresolved executables as argv commands", () => { - vi.spyOn(process, "platform", "get").mockReturnValue("win32"); + describe("command spawning", () => { + it("should preserve argv entries containing spaces", () => { + const managerWithInternals = packageManager as unknown as { + runCommandSync(command: string, args: string[]): string; + }; + const valueWithSpace = "C:\\Users\\A B\\.pi\\npm"; + const output = managerWithInternals.runCommandSync(process.execPath, [ + "-e", + "console.log(process.argv[1])", + valueWithSpace, + ]); - 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"); + expect(output).toBe(valueWithSpace); }); });