mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-11-05 23:37:25 +00:00
fix(coding-agent): accept Windows Store shell aliases
Node's existsSync() can reject runnable Store aliases because its extra
Windows-only stat check fails with EACCES (nodejs/node#36790). Use
access(F_OK) for shell validation on all platforms instead, matching the
check harness-v2's async refactor settled on (617d8b317, now pathExists
in packages/durable/src/env/node.ts). On Unix this is a no-op: existsSync
is already access(F_OK) there, so shell search order and behavior stay
unchanged; on Windows runnable aliases now pass validation.
This commit is contained in:
parent
5c1736f82d
commit
28bea006be
2 changed files with 125 additions and 6 deletions
|
|
@ -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");
|
||||
}
|
||||
|
||||
|
|
|
|||
106
packages/coding-agent/test/shell-path-exists.test.ts
Normal file
106
packages/coding-agent/test/shell-path-exists.test.ts
Normal file
|
|
@ -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<typeof Fs>();
|
||||
return {
|
||||
...actual,
|
||||
accessSync: accessSyncMock,
|
||||
};
|
||||
});
|
||||
|
||||
vi.mock("child_process", async (importOriginal) => ({
|
||||
...(await importOriginal<typeof ChildProcess>()),
|
||||
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");
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue