diff --git a/packages/coding-agent/src/utils/shell.ts b/packages/coding-agent/src/utils/shell.ts index 14436cff2..ab30cdacd 100644 --- a/packages/coding-agent/src/utils/shell.ts +++ b/packages/coding-agent/src/utils/shell.ts @@ -1,4 +1,4 @@ -import { existsSync } from "node:fs"; +import { accessSync, constants } from "node:fs"; import { delimiter, join } from "node:path"; import { spawn, spawnSync } from "child_process"; import { getBinDir } from "../config.ts"; @@ -21,9 +21,22 @@ function getBashShellConfig(shell: string): ShellConfig { return isLegacyWslBashPath(shell) ? { shell, args: ["-s"], commandTransport: "stdin" } : { shell, args: ["-c"] }; } +// Matches harness-v2's pathExists (packages/durable/src/env/node.ts). +// On Unix this is identical to existsSync() (both are access(F_OK)), just +// spelled with try/catch. On Windows we explicitly avoid existsSync(): its +// extra stat check rejects runnable Store aliases (nodejs/node#36790). +function shellPathExists(path: string): boolean { + try { + accessSync(path, constants.F_OK); + return true; + } catch { + return false; + } +} + function findExecutableOnPath(executable: string): string | null { if (process.platform === "win32") { - // Windows: Use 'where' and verify file exists (where can return non-existent paths) + // Windows: use 'where' and check the returned path. try { const result = spawnSync("where", [executable], { encoding: "utf-8", @@ -32,7 +45,7 @@ function findExecutableOnPath(executable: string): string | null { }); if (result.status === 0 && result.stdout) { const firstMatch = result.stdout.trim().split(/\r?\n/)[0]; - if (firstMatch && existsSync(firstMatch)) { + if (firstMatch && shellPathExists(firstMatch)) { return firstMatch; } } @@ -67,7 +80,7 @@ function findExecutableOnPath(executable: string): string | null { export function getShellConfig(customShellPath?: string): ShellConfig { // 1. Check user-specified shell path if (customShellPath) { - if (existsSync(customShellPath)) { + if (shellPathExists(customShellPath)) { return getBashShellConfig(customShellPath); } throw new Error(`Custom shell path not found: ${customShellPath}`); @@ -86,7 +99,7 @@ export function getShellConfig(customShellPath?: string): ShellConfig { } for (const path of paths) { - if (existsSync(path)) { + if (shellPathExists(path)) { return getBashShellConfig(path); } } @@ -107,7 +120,7 @@ export function getShellConfig(customShellPath?: string): ShellConfig { } // Unix: try /bin/bash, then bash on PATH, then fallback to sh - if (existsSync("/bin/bash")) { + if (shellPathExists("/bin/bash")) { return getBashShellConfig("/bin/bash"); } diff --git a/packages/coding-agent/test/shell-path-exists.test.ts b/packages/coding-agent/test/shell-path-exists.test.ts new file mode 100644 index 000000000..9451de933 --- /dev/null +++ b/packages/coding-agent/test/shell-path-exists.test.ts @@ -0,0 +1,106 @@ +import type * as ChildProcess from "node:child_process"; +import type * as Fs from "node:fs"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { getPowerShellConfig, getShellConfig } from "../src/utils/shell.ts"; + +const { accessSyncMock, spawnSyncMock } = vi.hoisted(() => ({ + accessSyncMock: vi.fn(), + spawnSyncMock: vi.fn(), +})); + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + accessSync: accessSyncMock, + }; +}); + +vi.mock("child_process", async (importOriginal) => ({ + ...(await importOriginal()), + spawnSync: spawnSyncMock, +})); + +const platformDescriptor = Object.getOwnPropertyDescriptor(process, "platform")!; +const aliasesDir = "D:\\Users\\test\\AppData\\Local\\Microsoft\\WindowsApps"; + +function stubPlatform(platform: string) { + Object.defineProperty(process, "platform", { configurable: true, value: platform }); +} + +beforeEach(() => { + vi.stubEnv("ProgramFiles", undefined); + vi.stubEnv("ProgramFiles(x86)", undefined); + accessSyncMock.mockReset(); + spawnSyncMock.mockReset(); +}); + +afterEach(() => { + Object.defineProperty(process, "platform", platformDescriptor); + vi.unstubAllEnvs(); +}); + +// nodejs/node#36790: runnable Store aliases pass access(F_OK) but fail the extra +// stat check existsSync performs on Windows only. +describe("Windows Store shell aliases", () => { + beforeEach(() => { + stubPlatform("win32"); + }); + + it.each(["bash.exe", "pwsh.exe"])("accepts %s found on PATH", (executable) => { + const alias = `${aliasesDir}\\${executable}`; + spawnSyncMock.mockReturnValue({ status: 0, stdout: `${alias}\r\n` }); + + const config = executable === "pwsh.exe" ? getPowerShellConfig() : getShellConfig(); + + expect(config.shell).toBe(alias); + }); + + it("accepts an explicitly configured shell alias", () => { + const alias = `${aliasesDir}\\bash.exe`; + + expect(getShellConfig(alias).shell).toBe(alias); + }); + + it("still rejects PATH results that fail the existence check", () => { + const windowsPowerShell = "D:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe"; + spawnSyncMock + .mockReturnValueOnce({ status: 0, stdout: `${aliasesDir}\\pwsh.exe` }) + .mockReturnValueOnce({ status: 0, stdout: windowsPowerShell }); + accessSyncMock.mockImplementationOnce(() => { + throw Object.assign(new Error("not found"), { code: "ENOENT" }); + }); + + expect(getPowerShellConfig().shell).toBe(windowsPowerShell); + }); +}); + +// The access(F_OK) check is intentionally used on every platform, matching +// harness-v2 (packages/durable/src/env/node.ts pathExists). On Unix it is the +// same syscall existsSync already performs. +describe("Unix shell existence checks", () => { + beforeEach(() => { + stubPlatform("linux"); + }); + + it("accepts /bin/bash when access(F_OK) succeeds", () => { + expect(getShellConfig().shell).toBe("/bin/bash"); + }); + + it("falls back to bash on PATH when /bin/bash fails access(F_OK)", () => { + accessSyncMock.mockImplementationOnce(() => { + throw Object.assign(new Error("not found"), { code: "ENOENT" }); + }); + spawnSyncMock.mockReturnValue({ status: 0, stdout: "/usr/local/bin/bash\n" }); + + expect(getShellConfig().shell).toBe("/usr/local/bin/bash"); + }); + + it("rejects an explicitly configured shell path that fails access(F_OK)", () => { + accessSyncMock.mockImplementationOnce(() => { + throw Object.assign(new Error("not found"), { code: "ENOENT" }); + }); + + expect(() => getShellConfig("/missing/bash")).toThrow("Custom shell path not found: /missing/bash"); + }); +});