refactor: make executable resolution async and centralize spawn
- Rename findExecutable → findExecutableSync, add async findExecutable that uses promisified execFile instead of execFileSync - Simplify Windows PATH resolution: rely on inherited env from Explorer instead of shelling out to PowerShell for registry PATH entries - Add spawnProcess() helper that centralizes Windows shell/quoting concerns - Update all agent providers and consumers to use async executable resolution - Add windowsHide: true across all Windows child process calls
This commit is contained in:
@@ -1,4 +1,5 @@
|
||||
import { exec, execFile, spawn } from "child_process";
|
||||
import { exec, execFile } from "child_process";
|
||||
import { spawnProcess } from "./spawn.js";
|
||||
import { promisify } from "util";
|
||||
import { resolve, dirname, basename } from "path";
|
||||
import { realpathSync } from "fs";
|
||||
@@ -87,7 +88,7 @@ async function spawnLimitedText(params: {
|
||||
const accept = new Set(params.acceptExitCodes ?? [0]);
|
||||
|
||||
return new Promise((resolvePromise, rejectPromise) => {
|
||||
const child = spawn(params.cmd, params.args, {
|
||||
const child = spawnProcess(params.cmd, params.args, {
|
||||
cwd: params.cwd,
|
||||
env: params.env,
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
@@ -106,7 +107,7 @@ async function spawnLimitedText(params: {
|
||||
}
|
||||
};
|
||||
|
||||
child.stdout.on("data", (chunk: Buffer) => {
|
||||
child.stdout!.on("data", (chunk: Buffer) => {
|
||||
if (truncated) return;
|
||||
stdoutBytes += chunk.length;
|
||||
if (stdoutBytes > params.maxBytes) {
|
||||
@@ -119,7 +120,7 @@ async function spawnLimitedText(params: {
|
||||
|
||||
// We don't buffer stderr (it can be large too). Keep it minimal for debugging.
|
||||
let stderrPreview = "";
|
||||
child.stderr.on("data", (chunk: Buffer) => {
|
||||
child.stderr!.on("data", (chunk: Buffer) => {
|
||||
if (stderrPreview.length > 2048) return;
|
||||
stderrPreview += chunk.toString("utf8");
|
||||
});
|
||||
@@ -1783,9 +1784,9 @@ export interface PullRequestStatusResult {
|
||||
githubFeaturesEnabled: boolean;
|
||||
}
|
||||
|
||||
function resolveGhPath(): string {
|
||||
async function resolveGhPath(): Promise<string> {
|
||||
if (cachedGhPath === undefined) {
|
||||
cachedGhPath = findExecutable("gh");
|
||||
cachedGhPath = await findExecutable("gh");
|
||||
}
|
||||
if (cachedGhPath === null) {
|
||||
throw new Error("GitHub CLI (gh) is not installed or not in PATH");
|
||||
@@ -1859,7 +1860,7 @@ export async function createPullRequest(
|
||||
options: CreatePullRequestOptions,
|
||||
): Promise<{ url: string; number: number }> {
|
||||
await requireGitRepo(cwd);
|
||||
const ghPath = resolveGhPath();
|
||||
const ghPath = await resolveGhPath();
|
||||
const repo = await resolveGitHubRepo(cwd);
|
||||
if (!repo) {
|
||||
throw new Error("Unable to determine GitHub repo from git remote");
|
||||
@@ -1932,7 +1933,7 @@ async function getPullRequestStatusUncached(cwd: string): Promise<PullRequestSta
|
||||
}
|
||||
let ghPath: string;
|
||||
try {
|
||||
ghPath = resolveGhPath();
|
||||
ghPath = await resolveGhPath();
|
||||
} catch {
|
||||
return {
|
||||
status: null,
|
||||
|
||||
@@ -1,12 +1,12 @@
|
||||
import { afterEach, beforeEach, describe, expect, test, vi } from "vitest";
|
||||
|
||||
import {
|
||||
findExecutable,
|
||||
findExecutableSync,
|
||||
quoteWindowsArgument,
|
||||
quoteWindowsCommand,
|
||||
} from "./executable.js";
|
||||
|
||||
type FindExecutableDependencies = NonNullable<Parameters<typeof findExecutable>[1]>;
|
||||
type FindExecutableDependencies = NonNullable<Parameters<typeof findExecutableSync>[1]>;
|
||||
|
||||
function createFindExecutableDependencies(): FindExecutableDependencies {
|
||||
return {
|
||||
@@ -22,57 +22,31 @@ beforeEach(() => {
|
||||
findExecutableDependencies = createFindExecutableDependencies();
|
||||
});
|
||||
|
||||
describe("findExecutable", () => {
|
||||
test("on Windows, resolves executables using current machine and user PATH entries", () => {
|
||||
describe("findExecutableSync", () => {
|
||||
test("on Windows, resolves executables using where.exe with inherited PATH", () => {
|
||||
findExecutableDependencies.platform = vi.fn(() => "win32");
|
||||
process.env.Path = "C:\\Windows\\System32";
|
||||
findExecutableDependencies.execFileSync.mockImplementation(
|
||||
((command: string, args?: string[]) => {
|
||||
if (command === "powershell") {
|
||||
return "C:\\Windows\\System32\r\nC:\\Users\\boudr\\.local\\bin\r\n";
|
||||
}
|
||||
if (command === "where.exe") {
|
||||
return "C:\\Users\\boudr\\.local\\bin\\claude.exe\r\n";
|
||||
}
|
||||
throw new Error(`unexpected command ${command}`);
|
||||
}) as any,
|
||||
findExecutableDependencies.execFileSync.mockReturnValue(
|
||||
"C:\\Users\\boudr\\.local\\bin\\claude.exe\r\n",
|
||||
);
|
||||
|
||||
expect(findExecutable("claude", findExecutableDependencies)).toBe(
|
||||
expect(findExecutableSync("claude", findExecutableDependencies)).toBe(
|
||||
"C:\\Users\\boudr\\.local\\bin\\claude.exe",
|
||||
);
|
||||
const powershellCall = findExecutableDependencies.execFileSync.mock.calls[0];
|
||||
expect(powershellCall?.[0]).toBe("powershell");
|
||||
expect(powershellCall?.[1]).toContain("-NoProfile");
|
||||
expect(powershellCall?.[1]).toContain("-NonInteractive");
|
||||
expect(powershellCall?.[1]).toContain(
|
||||
'$machine = [Environment]::GetEnvironmentVariable("Path", "Machine"); $user = [Environment]::GetEnvironmentVariable("Path", "User"); if ($machine) { Write-Output $machine }; if ($user) { Write-Output $user }',
|
||||
);
|
||||
const whereCall = findExecutableDependencies.execFileSync.mock.calls[1];
|
||||
expect(whereCall?.[0]).toBe("where.exe");
|
||||
expect(whereCall?.[1]).toEqual(["claude"]);
|
||||
expect(whereCall?.[2]?.encoding).toBe("utf8");
|
||||
const env = whereCall?.[2]?.env as Record<string, string | undefined>;
|
||||
expect(env.PATH).toContain("C:\\Users\\boudr\\.local\\bin");
|
||||
expect(env.Path).toContain("C:\\Users\\boudr\\.local\\bin");
|
||||
expect(findExecutableDependencies.execFileSync).toHaveBeenCalledOnce();
|
||||
const call = findExecutableDependencies.execFileSync.mock.calls[0];
|
||||
expect(call?.[0]).toBe("where.exe");
|
||||
expect(call?.[1]).toEqual(["claude"]);
|
||||
expect(call?.[2]?.encoding).toBe("utf8");
|
||||
expect(call?.[2]?.windowsHide).toBe(true);
|
||||
});
|
||||
|
||||
test("on Windows, preserves the first where.exe match", () => {
|
||||
findExecutableDependencies.platform = vi.fn(() => "win32");
|
||||
process.env.Path = "C:\\Windows\\System32";
|
||||
findExecutableDependencies.execFileSync.mockImplementation(
|
||||
((command: string) => {
|
||||
if (command === "powershell") {
|
||||
return "C:\\Windows\\System32\r\nC:\\nvm4w\\nodejs\r\n";
|
||||
}
|
||||
if (command === "where.exe") {
|
||||
return "C:\\nvm4w\\nodejs\\codex\r\nC:\\nvm4w\\nodejs\\codex.cmd\r\n";
|
||||
}
|
||||
throw new Error(`unexpected command ${command}`);
|
||||
}) as any,
|
||||
findExecutableDependencies.execFileSync.mockReturnValue(
|
||||
"C:\\nvm4w\\nodejs\\codex\r\nC:\\nvm4w\\nodejs\\codex.cmd\r\n",
|
||||
);
|
||||
|
||||
expect(findExecutable("codex", findExecutableDependencies)).toBe("C:\\nvm4w\\nodejs\\codex");
|
||||
expect(findExecutableSync("codex", findExecutableDependencies)).toBe("C:\\nvm4w\\nodejs\\codex");
|
||||
});
|
||||
|
||||
test("on Unix, uses the last line from which output", () => {
|
||||
@@ -80,7 +54,7 @@ describe("findExecutable", () => {
|
||||
"/usr/local/bin/codex\n",
|
||||
);
|
||||
|
||||
expect(findExecutable("codex", findExecutableDependencies)).toBe("/usr/local/bin/codex");
|
||||
expect(findExecutableSync("codex", findExecutableDependencies)).toBe("/usr/local/bin/codex");
|
||||
expect(findExecutableDependencies.execFileSync).toHaveBeenCalledWith(
|
||||
"which",
|
||||
["codex"],
|
||||
@@ -92,7 +66,7 @@ describe("findExecutable", () => {
|
||||
findExecutableDependencies.execFileSync.mockReturnValue("codex\n");
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
|
||||
expect(findExecutable("codex", findExecutableDependencies)).toBeNull();
|
||||
expect(findExecutableSync("codex", findExecutableDependencies)).toBeNull();
|
||||
expect(warnSpy).toHaveBeenCalledOnce();
|
||||
|
||||
warnSpy.mockRestore();
|
||||
@@ -101,7 +75,7 @@ describe("findExecutable", () => {
|
||||
test("returns direct paths when they exist", () => {
|
||||
findExecutableDependencies.existsSync.mockReturnValue(true);
|
||||
|
||||
expect(findExecutable("/usr/local/bin/codex", findExecutableDependencies)).toBe(
|
||||
expect(findExecutableSync("/usr/local/bin/codex", findExecutableDependencies)).toBe(
|
||||
"/usr/local/bin/codex",
|
||||
);
|
||||
expect(findExecutableDependencies.existsSync).toHaveBeenCalledWith("/usr/local/bin/codex");
|
||||
|
||||
@@ -1,40 +1,17 @@
|
||||
import { execFileSync } from "node:child_process";
|
||||
import { execFile, execFileSync } from "node:child_process";
|
||||
import { existsSync } from "node:fs";
|
||||
import { platform } from "node:os";
|
||||
import path from "node:path";
|
||||
import { promisify } from "node:util";
|
||||
|
||||
const execFileAsync = promisify(execFile);
|
||||
|
||||
export interface FindExecutableDependencies {
|
||||
execFileSync: typeof execFileSync;
|
||||
existsSync: typeof existsSync;
|
||||
platform: typeof platform;
|
||||
}
|
||||
|
||||
function resolveWindowsPathEntries(deps: FindExecutableDependencies): string[] {
|
||||
try {
|
||||
const output = deps.execFileSync(
|
||||
"powershell",
|
||||
[
|
||||
"-NoProfile",
|
||||
"-NonInteractive",
|
||||
"-Command",
|
||||
[
|
||||
'$machine = [Environment]::GetEnvironmentVariable("Path", "Machine")',
|
||||
'$user = [Environment]::GetEnvironmentVariable("Path", "User")',
|
||||
"if ($machine) { Write-Output $machine }",
|
||||
"if ($user) { Write-Output $user }",
|
||||
].join("; "),
|
||||
],
|
||||
{ encoding: "utf8" },
|
||||
);
|
||||
return output
|
||||
.split(/\r?\n/)
|
||||
.flatMap((line) => line.split(";"))
|
||||
.map((entry) => entry.trim())
|
||||
.filter((entry) => entry.length > 0);
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
}
|
||||
|
||||
function resolveExecutableFromWhichOutput(
|
||||
name: string,
|
||||
output: string,
|
||||
@@ -61,15 +38,13 @@ function resolveExecutableFromWhichOutput(
|
||||
}
|
||||
|
||||
/**
|
||||
* On Unix we use plain `which` — the daemon's process.env.PATH is enriched
|
||||
* with the login shell environment at Electron desktop startup.
|
||||
* On Unix we use `which`. On Windows we use `where.exe`.
|
||||
*
|
||||
* On Windows we augment the daemon PATH with machine/user registry PATH values
|
||||
* and return the first `where.exe` match. Launch-time execution decides whether
|
||||
* the resolved path needs `cmd.exe` semantics (for example npm shims under
|
||||
* nvm4w such as `C:\nvm4w\nodejs\codex`).
|
||||
* Both rely on the inherited process.env.PATH — on macOS/Linux, Electron
|
||||
* enriches it at startup via inheritLoginShellEnv(); on Windows, Electron
|
||||
* inherits the full user environment from Explorer.
|
||||
*/
|
||||
export function findExecutable(
|
||||
export function findExecutableSync(
|
||||
name: string,
|
||||
dependencies?: FindExecutableDependencies,
|
||||
): string | null {
|
||||
@@ -91,21 +66,10 @@ export function findExecutable(
|
||||
|
||||
if (deps.platform() === "win32") {
|
||||
try {
|
||||
const inheritedPath = process.env["Path"] ?? process.env["PATH"] ?? "";
|
||||
const resolvedPath = [
|
||||
...inheritedPath.split(";"),
|
||||
...resolveWindowsPathEntries(deps),
|
||||
]
|
||||
.map((entry) => entry.trim())
|
||||
.filter((entry) => entry.length > 0)
|
||||
.filter((entry, index, entries) => entries.indexOf(entry) === index)
|
||||
.join(";");
|
||||
const env = {
|
||||
...process.env,
|
||||
PATH: resolvedPath,
|
||||
Path: resolvedPath,
|
||||
};
|
||||
const out = deps.execFileSync("where.exe", [trimmed], { encoding: "utf8", env }).trim();
|
||||
const out = deps.execFileSync("where.exe", [trimmed], {
|
||||
encoding: "utf8",
|
||||
windowsHide: true,
|
||||
}).trim();
|
||||
return (
|
||||
out
|
||||
.split(/\r?\n/)
|
||||
@@ -128,8 +92,48 @@ export function findExecutable(
|
||||
}
|
||||
}
|
||||
|
||||
export function isCommandAvailable(command: string): boolean {
|
||||
return findExecutable(command) !== null;
|
||||
export function isCommandAvailableSync(command: string): boolean {
|
||||
return findExecutableSync(command) !== null;
|
||||
}
|
||||
|
||||
export async function findExecutable(name: string): Promise<string | null> {
|
||||
const trimmed = name.trim();
|
||||
if (!trimmed) {
|
||||
return null;
|
||||
}
|
||||
|
||||
if (trimmed.includes("/") || trimmed.includes("\\")) {
|
||||
return existsSync(trimmed) ? trimmed : null;
|
||||
}
|
||||
|
||||
if (platform() === "win32") {
|
||||
try {
|
||||
const { stdout } = await execFileAsync("where.exe", [trimmed], {
|
||||
encoding: "utf8",
|
||||
windowsHide: true,
|
||||
});
|
||||
return (
|
||||
stdout
|
||||
.trim()
|
||||
.split(/\r?\n/)
|
||||
.map((line) => line.trim())
|
||||
.find((line) => line.length > 0) ?? null
|
||||
);
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
const { stdout } = await execFileAsync("which", [trimmed], { encoding: "utf8" });
|
||||
return resolveExecutableFromWhichOutput(trimmed, stdout.trim(), "which");
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
export async function isCommandAvailable(command: string): Promise<boolean> {
|
||||
return (await findExecutable(command)) !== null;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
import {
|
||||
spawn,
|
||||
type ChildProcess,
|
||||
type SpawnOptions,
|
||||
} from "node:child_process";
|
||||
|
||||
/**
|
||||
* Platform-aware spawn that centralizes Windows shell and quoting concerns.
|
||||
*
|
||||
* On Windows:
|
||||
* - Enables `shell: true` (routes through cmd.exe) unless the caller explicitly sets `shell`
|
||||
* - Quotes the command and arguments so paths with spaces survive cmd.exe parsing
|
||||
* - Always sets `windowsHide: true` to prevent console window flashes
|
||||
*
|
||||
* On other platforms the call is passed through to `spawn` unchanged (with `windowsHide: true`).
|
||||
*/
|
||||
export function spawnProcess(
|
||||
command: string,
|
||||
args: string[],
|
||||
options?: SpawnOptions,
|
||||
): ChildProcess {
|
||||
const isWindows = process.platform === "win32";
|
||||
|
||||
const resolvedCommand = isWindows ? quoteForCmd(command) : command;
|
||||
const resolvedArgs = isWindows ? args.map(quoteForCmd) : args;
|
||||
|
||||
return spawn(resolvedCommand, resolvedArgs, {
|
||||
...options,
|
||||
shell: options?.shell ?? isWindows,
|
||||
windowsHide: true,
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Quote a string for cmd.exe if it contains spaces and isn't already quoted.
|
||||
* No-op for strings without spaces or strings that are already double-quoted.
|
||||
*/
|
||||
function quoteForCmd(value: string): string {
|
||||
if (!value.includes(" ")) return value;
|
||||
if (value.startsWith('"') && value.endsWith('"')) return value;
|
||||
return `"${value}"`;
|
||||
}
|
||||
Reference in New Issue
Block a user