From 99114ddd1196755d05ade0bc1ac7c9cc6ee3d936 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sat, 4 Apr 2026 14:24:06 +0700 Subject: [PATCH] fix(server): resolve gh executable via login shell for desktop users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Electron apps on macOS inherit a minimal PATH (/usr/bin:/bin:/usr/sbin:/sbin) that doesn't include Homebrew or other user-installed tools. This caused GitHub features (PR status, Create PR) to show as unavailable in the desktop app even when gh CLI was installed and authenticated. Extract findExecutable, resolveShellEnv, and related utilities from provider-launch-config.ts into a shared utils/executable.ts module. Use findExecutable("gh") with lazy caching so the login-shell resolution happens once per daemon lifetime. Pass resolveShellEnv() as the process env for all gh subprocess calls. Update all consumers to import directly from utils/executable.ts — no re-exports through provider-launch-config.ts. --- .../agent/provider-launch-config.test.ts | 181 +--------------- .../server/agent/provider-launch-config.ts | 203 +----------------- ...ude-agent.event-stream.integration.test.ts | 2 +- .../src/server/agent/providers/acp-agent.ts | 2 +- .../claude-agent-commands.real.e2e.test.ts | 2 +- .../claude-agent.integration.test.ts | 2 +- .../claude-agent.max-effort.real.e2e.test.ts | 2 +- .../server/agent/providers/claude-agent.ts | 6 +- .../agent/providers/codex-app-server-agent.ts | 8 +- .../opencode-agent-commands.real.e2e.test.ts | 2 +- .../server/agent/providers/opencode-agent.ts | 8 +- .../src/server/daemon-e2e/agent-configs.ts | 2 +- packages/server/src/server/exports.ts | 4 +- .../server/src/utils/checkout-git.test.ts | 64 ++---- packages/server/src/utils/checkout-git.ts | 34 ++- packages/server/src/utils/executable.test.ts | 183 ++++++++++++++++ packages/server/src/utils/executable.ts | 197 +++++++++++++++++ 17 files changed, 453 insertions(+), 449 deletions(-) create mode 100644 packages/server/src/utils/executable.test.ts create mode 100644 packages/server/src/utils/executable.ts diff --git a/packages/server/src/server/agent/provider-launch-config.test.ts b/packages/server/src/server/agent/provider-launch-config.test.ts index a2b674558..8cb7aca71 100644 --- a/packages/server/src/server/agent/provider-launch-config.test.ts +++ b/packages/server/src/server/agent/provider-launch-config.test.ts @@ -1,32 +1,11 @@ -import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { describe, expect, test, vi } from "vitest"; import { - findExecutable, - quoteWindowsArgument, - quoteWindowsCommand, resolveProviderCommandPrefix, applyProviderEnv, type ProviderRuntimeSettings, } from "./provider-launch-config.js"; -type FindExecutableDependencies = NonNullable[1]>; - -function createFindExecutableDependencies(): FindExecutableDependencies { - return { - execFileSync: vi.fn(), - execSync: vi.fn(), - existsSync: vi.fn(), - platform: vi.fn(() => "darwin"), - shell: undefined, - }; -} - -let findExecutableDependencies: FindExecutableDependencies; - -beforeEach(() => { - findExecutableDependencies = createFindExecutableDependencies(); -}); - describe("resolveProviderCommandPrefix", () => { test("uses resolved default command in default mode", () => { const resolveDefault = vi.fn(() => "/usr/local/bin/claude"); @@ -134,161 +113,3 @@ describe("applyProviderEnv", () => { expect(env.CLAUDE_CODE_ENABLE_SDK_FILE_CHECKPOINTING).toBeUndefined(); }); }); - -describe("findExecutable", () => { - test("on Windows, resolves executables using current machine and user PATH entries", () => { - 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, - ); - - expect(findExecutable("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; - expect(env.PATH).toContain("C:\\Users\\boudr\\.local\\bin"); - expect(env.Path).toContain("C:\\Users\\boudr\\.local\\bin"); - }); - - 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, - ); - - expect(findExecutable("codex", findExecutableDependencies)).toBe("C:\\nvm4w\\nodejs\\codex"); - }); - - test("uses the last line from login-shell which output", () => { - findExecutableDependencies.shell = "/bin/zsh"; - findExecutableDependencies.execSync.mockReturnValue( - "echo from profile\n/usr/local/bin/codex\n", - ); - - expect(findExecutable("codex", findExecutableDependencies)).toBe("/usr/local/bin/codex"); - expect(findExecutableDependencies.execSync).toHaveBeenCalledOnce(); - expect(findExecutableDependencies.execFileSync).not.toHaveBeenCalled(); - }); - - test("warns and returns null when the final which line is not an absolute path", () => { - findExecutableDependencies.shell = "/bin/zsh"; - findExecutableDependencies.execSync.mockReturnValue("profile noise\ncodex\n"); - findExecutableDependencies.execFileSync.mockReturnValue("codex\n"); - const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); - - expect(findExecutable("codex", findExecutableDependencies)).toBeNull(); - expect(warnSpy).toHaveBeenCalledTimes(2); - - warnSpy.mockRestore(); - }); - - test("returns direct paths when they exist", () => { - findExecutableDependencies.existsSync.mockReturnValue(true); - - expect(findExecutable("/usr/local/bin/codex", findExecutableDependencies)).toBe( - "/usr/local/bin/codex", - ); - expect(findExecutableDependencies.existsSync).toHaveBeenCalledWith("/usr/local/bin/codex"); - }); -}); - -describe("quoteWindowsCommand", () => { - const originalPlatform = process.platform; - - function setPlatform(value: string) { - Object.defineProperty(process, "platform", { value, writable: true }); - } - - afterEach(() => { - setPlatform(originalPlatform); - }); - - test("quotes a Windows path with spaces", () => { - setPlatform("win32"); - expect(quoteWindowsCommand("C:\\Program Files\\Anthropic\\claude.exe")).toBe( - '"C:\\Program Files\\Anthropic\\claude.exe"', - ); - }); - - test("does not double-quote an already-quoted path", () => { - setPlatform("win32"); - expect(quoteWindowsCommand('"C:\\Program Files\\Anthropic\\claude.exe"')).toBe( - '"C:\\Program Files\\Anthropic\\claude.exe"', - ); - }); - - test("returns the command unchanged when there are no spaces", () => { - setPlatform("win32"); - expect(quoteWindowsCommand("C:\\nvm4w\\nodejs\\codex")).toBe("C:\\nvm4w\\nodejs\\codex"); - }); - - test("returns the command unchanged on non-Windows platforms", () => { - setPlatform("darwin"); - expect(quoteWindowsCommand("/usr/local/bin/claude code")).toBe("/usr/local/bin/claude code"); - }); -}); - -describe("quoteWindowsArgument", () => { - const originalPlatform = process.platform; - - function setPlatform(value: string) { - Object.defineProperty(process, "platform", { value, writable: true }); - } - - afterEach(() => { - setPlatform(originalPlatform); - }); - - test("quotes a Windows argument with spaces", () => { - setPlatform("win32"); - expect(quoteWindowsArgument("C:\\Program Files\\Anthropic\\cli.js")).toBe( - '"C:\\Program Files\\Anthropic\\cli.js"', - ); - }); - - test("does not double-quote an already-quoted argument", () => { - setPlatform("win32"); - expect(quoteWindowsArgument('"C:\\Program Files\\Anthropic\\cli.js"')).toBe( - '"C:\\Program Files\\Anthropic\\cli.js"', - ); - }); - - test("returns the argument unchanged when there are no spaces", () => { - setPlatform("win32"); - expect(quoteWindowsArgument("--version")).toBe("--version"); - }); - - test("returns the argument unchanged on non-Windows platforms", () => { - setPlatform("darwin"); - expect(quoteWindowsArgument("/usr/local/bin/claude code")).toBe("/usr/local/bin/claude code"); - }); -}); diff --git a/packages/server/src/server/agent/provider-launch-config.ts b/packages/server/src/server/agent/provider-launch-config.ts index b20ad8a86..facd7671a 100644 --- a/packages/server/src/server/agent/provider-launch-config.ts +++ b/packages/server/src/server/agent/provider-launch-config.ts @@ -1,10 +1,9 @@ -import { execFileSync, execSync } from "node:child_process"; -import { existsSync } from "node:fs"; -import { platform } from "node:os"; -import path from "node:path"; -import { shellEnvSync } from "shell-env"; import { z } from "zod"; +import { + isCommandAvailable, + resolveShellEnv, +} from "../../utils/executable.js"; import type { AgentProvider } from "./agent-sdk-types.js"; import { AgentProviderSchema } from "./provider-manifest.js"; @@ -57,66 +56,6 @@ export type ProviderCommandPrefix = { args: string[]; }; -interface FindExecutableDependencies { - execSync: typeof execSync; - execFileSync: typeof execFileSync; - existsSync: typeof existsSync; - platform: typeof platform; - shell: string | undefined; -} - -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, - source: "login-shell" | "which", -): string | null { - const lines = output - .split(/\r?\n/) - .map((line) => line.trim()) - .filter((line) => line.length > 0); - const candidate = lines.at(-1); - - if (!candidate) { - return null; - } - - if (!path.isAbsolute(candidate)) { - console.warn( - `[findExecutable] Ignoring non-absolute ${source} output for '${name}': ${JSON.stringify(candidate)}`, - ); - return null; - } - - return candidate; -} - export function resolveProviderCommandPrefix( commandConfig: ProviderCommand | undefined, resolveDefaultCommand: () => string, @@ -141,18 +80,6 @@ export function resolveProviderCommandPrefix( }; } -let cachedShellEnv: Record | null = null; - -export function resolveShellEnv(): Record { - if (cachedShellEnv) return cachedShellEnv; - try { - cachedShellEnv = shellEnvSync(); - } catch { - cachedShellEnv = { ...process.env } as Record; - } - return cachedShellEnv; -} - // Env vars that indicate a running Claude Code session. If the daemon itself is // launched from inside Claude Code (e.g. by a Paseo agent), these leak into // child processes and cause "cannot be launched inside another session" errors. @@ -180,128 +107,6 @@ export function applyProviderEnv( return merged; } -/** - * Resolve an executable name to its absolute path the way the user's shell would. - * - * On Unix we first try `$SHELL -lic "which "` so that rc-file PATH - * additions (asdf, nvm, homebrew, nix, etc.) are visible — exactly as if the - * user opened a terminal and typed the command. If that fails (e.g. the login - * shell itself errors) we fall back to a plain `which`. - * - * 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`). - */ -export function findExecutable( - name: string, - dependencies?: FindExecutableDependencies, -): string | null { - const trimmed = name.trim(); - if (!trimmed) { - return null; - } - - const deps: FindExecutableDependencies = { - execSync, - execFileSync, - existsSync, - platform, - shell: process.env["SHELL"], - ...dependencies, - }; - - if (trimmed.includes("/") || trimmed.includes("\\")) { - return deps.existsSync(trimmed) ? trimmed : null; - } - - 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(); - return ( - out - .split(/\r?\n/) - .map((line) => line.trim()) - .find((line) => line.length > 0) ?? null - ); - } catch { - return null; - } - } - - // Unix: try the user's login shell so rc-file PATH entries are visible. - const shell = deps.shell; - if (shell) { - try { - const out = deps - .execSync(`${shell} -lic "which ${trimmed}"`, { - encoding: "utf8", - timeout: 5000, - }) - .trim(); - const resolved = resolveExecutableFromWhichOutput(trimmed, out, "login-shell"); - if (resolved) { - return resolved; - } - } catch { - // Login shell failed (broken rc, etc.) — fall through to plain which. - } - } - - try { - return resolveExecutableFromWhichOutput( - trimmed, - deps.execFileSync("which", [trimmed], { encoding: "utf8" }).trim(), - "which", - ); - } catch { - return null; - } -} - -/** - * When spawning with `shell: true` on Windows, the command is passed to - * `cmd.exe /d /s /c "command args"`. The `/s` strips outer quotes, so a - * command path with spaces (e.g. `C:\Program Files\...`) is split at the - * space. Wrapping it in quotes produces the correct `"C:\Program Files\..." args`. - */ -export function quoteWindowsCommand(command: string): string { - if (process.platform !== "win32") return command; - if (!command.includes(" ")) return command; - if (command.startsWith('"') && command.endsWith('"')) return command; - return `"${command}"`; -} - -/** - * `spawn(..., { shell: true })` on Windows also passes argv through `cmd.exe`. - * Any argument containing spaces must be quoted or it will be split before the - * child process sees it. - */ -export function quoteWindowsArgument(argument: string): string { - if (process.platform !== "win32") return argument; - if (!argument.includes(" ")) return argument; - if (argument.startsWith('"') && argument.endsWith('"')) return argument; - return `"${argument}"`; -} - -export function isCommandAvailable(command: string): boolean { - return findExecutable(command) !== null; -} - export function isProviderCommandAvailable( commandConfig: ProviderCommand | undefined, resolveDefaultCommand: () => string, diff --git a/packages/server/src/server/agent/providers/__tests__/claude-agent.event-stream.integration.test.ts b/packages/server/src/server/agent/providers/__tests__/claude-agent.event-stream.integration.test.ts index 4d7f6cded..14c684297 100644 --- a/packages/server/src/server/agent/providers/__tests__/claude-agent.event-stream.integration.test.ts +++ b/packages/server/src/server/agent/providers/__tests__/claude-agent.event-stream.integration.test.ts @@ -17,7 +17,7 @@ import path from "node:path"; import pino from "pino"; import type { AgentSession, AgentStreamEvent } from "../../agent-sdk-types.js"; -import { isCommandAvailable } from "../../provider-launch-config.js"; +import { isCommandAvailable } from "../../../../utils/executable.js"; import { ClaudeAgentClient } from "../claude-agent.js"; // --------------------------------------------------------------------------- diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index fe40c125d..c288a9315 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -83,10 +83,10 @@ import type { } from "../agent-sdk-types.js"; import { applyProviderEnv, - findExecutable, resolveProviderCommandPrefix, type ProviderRuntimeSettings, } from "../provider-launch-config.js"; +import { findExecutable } from "../../../utils/executable.js"; const DEFAULT_ACP_CAPABILITIES: AgentCapabilityFlags = { supportsStreaming: true, diff --git a/packages/server/src/server/agent/providers/claude-agent-commands.real.e2e.test.ts b/packages/server/src/server/agent/providers/claude-agent-commands.real.e2e.test.ts index 846774483..baf548bfe 100644 --- a/packages/server/src/server/agent/providers/claude-agent-commands.real.e2e.test.ts +++ b/packages/server/src/server/agent/providers/claude-agent-commands.real.e2e.test.ts @@ -1,8 +1,8 @@ import { describe, expect, test } from "vitest"; import pino from "pino"; -import { isCommandAvailable } from "../provider-launch-config.js"; import type { AgentSlashCommand } from "../agent-sdk-types.js"; +import { isCommandAvailable } from "../../../utils/executable.js"; import { ClaudeAgentClient } from "./claude-agent.js"; // Real-Claude contract coverage: validates slash command shape from a live Claude CLI session. diff --git a/packages/server/src/server/agent/providers/claude-agent.integration.test.ts b/packages/server/src/server/agent/providers/claude-agent.integration.test.ts index e109d47e4..90c7d2ca4 100644 --- a/packages/server/src/server/agent/providers/claude-agent.integration.test.ts +++ b/packages/server/src/server/agent/providers/claude-agent.integration.test.ts @@ -6,7 +6,7 @@ import pino from "pino"; import { query, type SDKUserMessage } from "@anthropic-ai/claude-agent-sdk"; import type { AgentSession, AgentStreamEvent, ToolCallTimelineItem } from "../agent-sdk-types.js"; -import { isCommandAvailable } from "../provider-launch-config.js"; +import { isCommandAvailable } from "../../../utils/executable.js"; import { ClaudeAgentClient } from "./claude-agent.js"; import { streamSession } from "./test-utils/session-stream-adapter.js"; diff --git a/packages/server/src/server/agent/providers/claude-agent.max-effort.real.e2e.test.ts b/packages/server/src/server/agent/providers/claude-agent.max-effort.real.e2e.test.ts index 29bcd0eda..34f5fe5b0 100644 --- a/packages/server/src/server/agent/providers/claude-agent.max-effort.real.e2e.test.ts +++ b/packages/server/src/server/agent/providers/claude-agent.max-effort.real.e2e.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "vitest"; import pino from "pino"; import type { AgentStreamEvent, AgentSession } from "../agent-sdk-types.js"; -import { isCommandAvailable } from "../provider-launch-config.js"; +import { isCommandAvailable } from "../../../utils/executable.js"; import { ClaudeAgentClient } from "./claude-agent.js"; import { streamSession } from "./test-utils/session-stream-adapter.js"; diff --git a/packages/server/src/server/agent/providers/claude-agent.ts b/packages/server/src/server/agent/providers/claude-agent.ts index ae72ba30a..e1b4e8b19 100644 --- a/packages/server/src/server/agent/providers/claude-agent.ts +++ b/packages/server/src/server/agent/providers/claude-agent.ts @@ -69,11 +69,13 @@ import type { } from "../agent-sdk-types.js"; import { applyProviderEnv, + type ProviderRuntimeSettings, +} from "../provider-launch-config.js"; +import { findExecutable, quoteWindowsArgument, quoteWindowsCommand, - type ProviderRuntimeSettings, -} from "../provider-launch-config.js"; +} from "../../../utils/executable.js"; import { getOrchestratorModeInstructions } from "../orchestrator-instructions.js"; const fsPromises = promises; diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.ts index c0052a14f..bd9bbdff4 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.ts @@ -41,12 +41,14 @@ import { } from "./codex/tool-call-mapper.js"; import { applyProviderEnv, - findExecutable, - quoteWindowsArgument, - quoteWindowsCommand, resolveProviderCommandPrefix, type ProviderRuntimeSettings, } from "../provider-launch-config.js"; +import { + findExecutable, + quoteWindowsArgument, + quoteWindowsCommand, +} from "../../../utils/executable.js"; import { extractCodexTerminalSessionId, nonEmptyString } from "./tool-call-mapper-utils.js"; const DEFAULT_TIMEOUT_MS = 14 * 24 * 60 * 60 * 1000; diff --git a/packages/server/src/server/agent/providers/opencode-agent-commands.real.e2e.test.ts b/packages/server/src/server/agent/providers/opencode-agent-commands.real.e2e.test.ts index dcc2b97ea..ca9a49ae6 100644 --- a/packages/server/src/server/agent/providers/opencode-agent-commands.real.e2e.test.ts +++ b/packages/server/src/server/agent/providers/opencode-agent-commands.real.e2e.test.ts @@ -1,8 +1,8 @@ import { describe, expect, test } from "vitest"; import pino from "pino"; -import { isCommandAvailable } from "../provider-launch-config.js"; import type { AgentSlashCommand } from "../agent-sdk-types.js"; +import { isCommandAvailable } from "../../../utils/executable.js"; import { OpenCodeAgentClient } from "./opencode-agent.js"; describe("opencode agent commands contract (real)", () => { diff --git a/packages/server/src/server/agent/providers/opencode-agent.ts b/packages/server/src/server/agent/providers/opencode-agent.ts index a6ea4ab5a..d0f47b3e5 100644 --- a/packages/server/src/server/agent/providers/opencode-agent.ts +++ b/packages/server/src/server/agent/providers/opencode-agent.ts @@ -34,12 +34,14 @@ import type { } from "../agent-sdk-types.js"; import { applyProviderEnv, - findExecutable, - quoteWindowsArgument, - quoteWindowsCommand, resolveProviderCommandPrefix, type ProviderRuntimeSettings, } from "../provider-launch-config.js"; +import { + findExecutable, + quoteWindowsArgument, + quoteWindowsCommand, +} from "../../../utils/executable.js"; import { mapOpencodeToolCall } from "./opencode/tool-call-mapper.js"; const OPENCODE_CAPABILITIES: AgentCapabilityFlags = { diff --git a/packages/server/src/server/daemon-e2e/agent-configs.ts b/packages/server/src/server/daemon-e2e/agent-configs.ts index 803450f26..82bfd805a 100644 --- a/packages/server/src/server/daemon-e2e/agent-configs.ts +++ b/packages/server/src/server/daemon-e2e/agent-configs.ts @@ -7,7 +7,7 @@ import { join, dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { homedir } from "node:os"; import dotenv from "dotenv"; -import { isCommandAvailable } from "../agent/provider-launch-config.js"; +import { isCommandAvailable } from "../../utils/executable.js"; // Load .env.test eagerly so isProviderAvailable() has credentials at collection time. const serverRoot = resolve(dirname(fileURLToPath(import.meta.url)), "../../.."); diff --git a/packages/server/src/server/exports.ts b/packages/server/src/server/exports.ts index cc07163ba..c617066d3 100644 --- a/packages/server/src/server/exports.ts +++ b/packages/server/src/server/exports.ts @@ -31,11 +31,9 @@ export { // Provider binary resolution export { - findExecutable, applyProviderEnv, - quoteWindowsArgument, - quoteWindowsCommand, } from "./agent/provider-launch-config.js"; +export { findExecutable, quoteWindowsArgument, quoteWindowsCommand } from "../utils/executable.js"; // Provider manifest (source of truth for provider definitions) export { diff --git a/packages/server/src/utils/checkout-git.test.ts b/packages/server/src/utils/checkout-git.test.ts index 26904e40a..77e416669 100644 --- a/packages/server/src/utils/checkout-git.test.ts +++ b/packages/server/src/utils/checkout-git.test.ts @@ -12,7 +12,9 @@ import { import { join } from "path"; import { tmpdir } from "os"; import { + __resetGhPathCacheForTests, __resetPullRequestStatusCacheForTests, + __setGhPathForTests, __setPullRequestStatusCacheTtlForTests, commitAll, getCheckoutDiff, @@ -62,10 +64,12 @@ describe("checkout git utilities", () => { tempDir = setup.tempDir; repoDir = setup.repoDir; paseoHome = join(tempDir, "paseo-home"); + __resetGhPathCacheForTests(); __resetPullRequestStatusCacheForTests(); }); afterEach(() => { + __resetGhPathCacheForTests(); __resetPullRequestStatusCacheForTests(); rmSync(tempDir, { recursive: true, force: true }); }); @@ -582,18 +586,14 @@ const x = 1; const gitPath = execSync("command -v git", { stdio: "pipe" }).toString().trim(); symlinkSync(gitPath, join(fakeBinDir, "git")); - const originalPath = process.env.PATH; - process.env.PATH = fakeBinDir; try { + __setGhPathForTests(null); const status = await getPullRequestStatus(repoDir); expect(status.githubFeaturesEnabled).toBe(false); expect(status.status).toBeNull(); } finally { - if (originalPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = originalPath; - } + __resetGhPathCacheForTests(); + __resetPullRequestStatusCacheForTests(); } }); @@ -627,9 +627,8 @@ const x = 1; ); execSync(`chmod +x ${join(fakeBinDir, "gh")}`); - const originalPath = process.env.PATH; - process.env.PATH = `${fakeBinDir}:${originalPath ?? ""}`; try { + __setGhPathForTests(join(fakeBinDir, "gh")); const status = await getPullRequestStatus(repoDir); expect(status.githubFeaturesEnabled).toBe(true); expect(status.status).not.toBeNull(); @@ -639,11 +638,8 @@ const x = 1; expect(status.status?.isMerged).toBe(true); expect(status.status?.state).toBe("merged"); } finally { - if (originalPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = originalPath; - } + __resetGhPathCacheForTests(); + __resetPullRequestStatusCacheForTests(); } }); @@ -677,9 +673,8 @@ const x = 1; ); execSync(`chmod +x ${join(fakeBinDir, "gh")}`); - const originalPath = process.env.PATH; - process.env.PATH = `${fakeBinDir}:${originalPath ?? ""}`; try { + __setGhPathForTests(join(fakeBinDir, "gh")); const status = await getPullRequestStatus(repoDir); expect(status.githubFeaturesEnabled).toBe(true); expect(status.status).not.toBeNull(); @@ -689,11 +684,8 @@ const x = 1; expect(status.status?.isMerged).toBe(false); expect(status.status?.state).toBe("closed"); } finally { - if (originalPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = originalPath; - } + __resetGhPathCacheForTests(); + __resetPullRequestStatusCacheForTests(); } }); @@ -732,20 +724,16 @@ const x = 1; ); execSync(`chmod +x ${join(fakeBinDir, "gh")}`); - const originalPath = process.env.PATH; - process.env.PATH = `${fakeBinDir}:${originalPath ?? ""}`; try { + __setGhPathForTests(join(fakeBinDir, "gh")); const first = await getPullRequestStatus(repoDir); const second = await getPullRequestStatus(repoDir); expect(first).toEqual(second); expect(first.status?.url).toContain("/pull/123"); expect(readFileSync(callCountPath, "utf8").trim()).toBe("1"); } finally { - if (originalPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = originalPath; - } + __resetGhPathCacheForTests(); + __resetPullRequestStatusCacheForTests(); } }); @@ -785,10 +773,9 @@ const x = 1; ); execSync(`chmod +x ${join(fakeBinDir, "gh")}`); - const originalPath = process.env.PATH; - process.env.PATH = `${fakeBinDir}:${originalPath ?? ""}`; __setPullRequestStatusCacheTtlForTests(50); try { + __setGhPathForTests(join(fakeBinDir, "gh")); const first = await getPullRequestStatus(repoDir); await sleep(80); const second = await getPullRequestStatus(repoDir); @@ -796,11 +783,8 @@ const x = 1; expect(second.status?.url).toContain("/pull/2"); expect(readFileSync(callCountPath, "utf8").trim()).toBe("2"); } finally { - if (originalPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = originalPath; - } + __resetGhPathCacheForTests(); + __resetPullRequestStatusCacheForTests(); } }); @@ -840,9 +824,8 @@ const x = 1; ); execSync(`chmod +x ${join(fakeBinDir, "gh")}`); - const originalPath = process.env.PATH; - process.env.PATH = `${fakeBinDir}:${originalPath ?? ""}`; try { + __setGhPathForTests(join(fakeBinDir, "gh")); const [first, second] = await Promise.all([ getPullRequestStatus(repoDir), getPullRequestStatus(repoDir), @@ -850,11 +833,8 @@ const x = 1; expect(first).toEqual(second); expect(readFileSync(callCountPath, "utf8").trim()).toBe("1"); } finally { - if (originalPath === undefined) { - delete process.env.PATH; - } else { - process.env.PATH = originalPath; - } + __resetGhPathCacheForTests(); + __resetPullRequestStatusCacheForTests(); } }); diff --git a/packages/server/src/utils/checkout-git.ts b/packages/server/src/utils/checkout-git.ts index 1fbda4393..7324e7607 100644 --- a/packages/server/src/utils/checkout-git.ts +++ b/packages/server/src/utils/checkout-git.ts @@ -6,6 +6,7 @@ import { open as openFile, stat as statFile } from "fs/promises"; import { TTLCache } from "@isaacs/ttlcache"; import type { ParsedDiffFile } from "../server/utils/diff-highlighter.js"; import { parseAndHighlightDiff } from "../server/utils/diff-highlighter.js"; +import { findExecutable, resolveShellEnv } from "./executable.js"; import { isPaseoOwnedWorktreeCwd } from "./worktree.js"; import { requirePaseoWorktreeBaseRefName } from "./worktree-metadata.js"; @@ -23,6 +24,7 @@ const PULL_REQUEST_STATUS_CACHE_MAX = 1_000; let pullRequestStatusCacheTtlMs = DEFAULT_PULL_REQUEST_STATUS_CACHE_TTL_MS; let pullRequestStatusCache = createPullRequestStatusCache(pullRequestStatusCacheTtlMs); const pullRequestStatusInFlight = new Map>(); +let cachedGhPath: string | null | undefined = undefined; function createPullRequestStatusCache(ttlMs: number) { return new TTLCache({ @@ -52,6 +54,14 @@ export function __setPullRequestStatusCacheTtlForTests(ttlMs: number): void { pullRequestStatusInFlight.clear(); } +export function __resetGhPathCacheForTests(): void { + cachedGhPath = undefined; +} + +export function __setGhPathForTests(path: string | null): void { + cachedGhPath = path; +} + async function execGit( command: string, options: { cwd: string; env?: NodeJS.ProcessEnv }, @@ -1731,12 +1741,14 @@ export interface PullRequestStatusResult { githubFeaturesEnabled: boolean; } -async function ensureGhAvailable(cwd: string): Promise { - try { - await execAsync("gh --version", { cwd }); - } catch { - throw new Error("GitHub CLI (gh) is not available or not authenticated"); +function resolveGhPath(): string { + if (cachedGhPath === undefined) { + cachedGhPath = findExecutable("gh"); } + if (cachedGhPath === null) { + throw new Error("GitHub CLI (gh) is not installed or not in PATH"); + } + return cachedGhPath; } function getCommandErrorText(error: unknown): string { @@ -1805,7 +1817,7 @@ export async function createPullRequest( options: CreatePullRequestOptions, ): Promise<{ url: string; number: number }> { await requireGitRepo(cwd); - await ensureGhAvailable(cwd); + const ghPath = resolveGhPath(); const repo = await resolveGitHubRepo(cwd); if (!repo) { throw new Error("Unable to determine GitHub repo from git remote"); @@ -1827,13 +1839,14 @@ export async function createPullRequest( await execAsync(`git push -u origin ${head}`, { cwd }); + const ghEnv = { ...resolveShellEnv(), GIT_TERMINAL_PROMPT: "0" }; const args = ["api", "-X", "POST", `repos/${repo}/pulls`, "-f", `title=${options.title}`]; args.push("-f", `head=${head}`); args.push("-f", `base=${normalizedBase}`); if (options.body) { args.push("-f", `body=${options.body}`); } - const { stdout } = await execFileAsync("gh", args, { cwd }); + const { stdout } = await execFileAsync(ghPath, args, { cwd, env: ghEnv }); const parsed = JSON.parse(stdout.trim()); if (!parsed?.url || !parsed?.number) { throw new Error("GitHub CLI did not return PR url/number"); @@ -1875,8 +1888,9 @@ async function getPullRequestStatusUncached(cwd: string): Promise[1]>; + +function createFindExecutableDependencies(): FindExecutableDependencies { + return { + execFileSync: vi.fn(), + execSync: vi.fn(), + existsSync: vi.fn(), + platform: vi.fn(() => "darwin"), + shell: undefined, + }; +} + +let findExecutableDependencies: FindExecutableDependencies; + +beforeEach(() => { + findExecutableDependencies = createFindExecutableDependencies(); +}); + +describe("findExecutable", () => { + test("on Windows, resolves executables using current machine and user PATH entries", () => { + 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, + ); + + expect(findExecutable("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; + expect(env.PATH).toContain("C:\\Users\\boudr\\.local\\bin"); + expect(env.Path).toContain("C:\\Users\\boudr\\.local\\bin"); + }); + + 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, + ); + + expect(findExecutable("codex", findExecutableDependencies)).toBe("C:\\nvm4w\\nodejs\\codex"); + }); + + test("uses the last line from login-shell which output", () => { + findExecutableDependencies.shell = "/bin/zsh"; + findExecutableDependencies.execSync.mockReturnValue( + "echo from profile\n/usr/local/bin/codex\n", + ); + + expect(findExecutable("codex", findExecutableDependencies)).toBe("/usr/local/bin/codex"); + expect(findExecutableDependencies.execSync).toHaveBeenCalledOnce(); + expect(findExecutableDependencies.execFileSync).not.toHaveBeenCalled(); + }); + + test("warns and returns null when the final which line is not an absolute path", () => { + findExecutableDependencies.shell = "/bin/zsh"; + findExecutableDependencies.execSync.mockReturnValue("profile noise\ncodex\n"); + findExecutableDependencies.execFileSync.mockReturnValue("codex\n"); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + expect(findExecutable("codex", findExecutableDependencies)).toBeNull(); + expect(warnSpy).toHaveBeenCalledTimes(2); + + warnSpy.mockRestore(); + }); + + test("returns direct paths when they exist", () => { + findExecutableDependencies.existsSync.mockReturnValue(true); + + expect(findExecutable("/usr/local/bin/codex", findExecutableDependencies)).toBe( + "/usr/local/bin/codex", + ); + expect(findExecutableDependencies.existsSync).toHaveBeenCalledWith("/usr/local/bin/codex"); + }); +}); + +describe("quoteWindowsCommand", () => { + const originalPlatform = process.platform; + + function setPlatform(value: string) { + Object.defineProperty(process, "platform", { value, writable: true }); + } + + afterEach(() => { + setPlatform(originalPlatform); + }); + + test("quotes a Windows path with spaces", () => { + setPlatform("win32"); + expect(quoteWindowsCommand("C:\\Program Files\\Anthropic\\claude.exe")).toBe( + '"C:\\Program Files\\Anthropic\\claude.exe"', + ); + }); + + test("does not double-quote an already-quoted path", () => { + setPlatform("win32"); + expect(quoteWindowsCommand('"C:\\Program Files\\Anthropic\\claude.exe"')).toBe( + '"C:\\Program Files\\Anthropic\\claude.exe"', + ); + }); + + test("returns the command unchanged when there are no spaces", () => { + setPlatform("win32"); + expect(quoteWindowsCommand("C:\\nvm4w\\nodejs\\codex")).toBe("C:\\nvm4w\\nodejs\\codex"); + }); + + test("returns the command unchanged on non-Windows platforms", () => { + setPlatform("darwin"); + expect(quoteWindowsCommand("/usr/local/bin/claude code")).toBe("/usr/local/bin/claude code"); + }); +}); + +describe("quoteWindowsArgument", () => { + const originalPlatform = process.platform; + + function setPlatform(value: string) { + Object.defineProperty(process, "platform", { value, writable: true }); + } + + afterEach(() => { + setPlatform(originalPlatform); + }); + + test("quotes a Windows argument with spaces", () => { + setPlatform("win32"); + expect(quoteWindowsArgument("C:\\Program Files\\Anthropic\\cli.js")).toBe( + '"C:\\Program Files\\Anthropic\\cli.js"', + ); + }); + + test("does not double-quote an already-quoted argument", () => { + setPlatform("win32"); + expect(quoteWindowsArgument('"C:\\Program Files\\Anthropic\\cli.js"')).toBe( + '"C:\\Program Files\\Anthropic\\cli.js"', + ); + }); + + test("returns the argument unchanged when there are no spaces", () => { + setPlatform("win32"); + expect(quoteWindowsArgument("--version")).toBe("--version"); + }); + + test("returns the argument unchanged on non-Windows platforms", () => { + setPlatform("darwin"); + expect(quoteWindowsArgument("/usr/local/bin/claude code")).toBe("/usr/local/bin/claude code"); + }); +}); diff --git a/packages/server/src/utils/executable.ts b/packages/server/src/utils/executable.ts new file mode 100644 index 000000000..4c5fd61a8 --- /dev/null +++ b/packages/server/src/utils/executable.ts @@ -0,0 +1,197 @@ +import { execFileSync, execSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { platform } from "node:os"; +import path from "node:path"; +import { shellEnvSync } from "shell-env"; + +export interface FindExecutableDependencies { + execSync: typeof execSync; + execFileSync: typeof execFileSync; + existsSync: typeof existsSync; + platform: typeof platform; + shell: string | undefined; +} + +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, + source: "login-shell" | "which", +): string | null { + const lines = output + .split(/\r?\n/) + .map((line) => line.trim()) + .filter((line) => line.length > 0); + const candidate = lines.at(-1); + + if (!candidate) { + return null; + } + + if (!path.isAbsolute(candidate)) { + console.warn( + `[findExecutable] Ignoring non-absolute ${source} output for '${name}': ${JSON.stringify(candidate)}`, + ); + return null; + } + + return candidate; +} + +/** + * On Unix we first try `$SHELL -lic "which "` so that rc-file PATH + * additions (asdf, nvm, homebrew, nix, etc.) are visible — exactly as if the + * user opened a terminal and typed the command. If that fails (e.g. the login + * shell itself errors) we fall back to a plain `which`. + * + * 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`). + */ +export function findExecutable( + name: string, + dependencies?: FindExecutableDependencies, +): string | null { + const trimmed = name.trim(); + if (!trimmed) { + return null; + } + + const deps: FindExecutableDependencies = { + execSync, + execFileSync, + existsSync, + platform, + shell: process.env["SHELL"], + ...dependencies, + }; + + if (trimmed.includes("/") || trimmed.includes("\\")) { + return deps.existsSync(trimmed) ? trimmed : null; + } + + 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(); + return ( + out + .split(/\r?\n/) + .map((line) => line.trim()) + .find((line) => line.length > 0) ?? null + ); + } catch { + return null; + } + } + + // Unix: try the user's login shell so rc-file PATH entries are visible. + const shell = deps.shell; + if (shell) { + try { + const out = deps + .execSync(`${shell} -lic "which ${trimmed}"`, { + encoding: "utf8", + timeout: 5000, + }) + .trim(); + const resolved = resolveExecutableFromWhichOutput(trimmed, out, "login-shell"); + if (resolved) { + return resolved; + } + } catch { + // Login shell failed (broken rc, etc.) — fall through to plain which. + } + } + + try { + return resolveExecutableFromWhichOutput( + trimmed, + deps.execFileSync("which", [trimmed], { encoding: "utf8" }).trim(), + "which", + ); + } catch { + return null; + } +} + +export function isCommandAvailable(command: string): boolean { + return findExecutable(command) !== null; +} + +/** + * When spawning with `shell: true` on Windows, the command is passed to + * `cmd.exe /d /s /c "command args"`. The `/s` strips outer quotes, so a + * command path with spaces (e.g. `C:\Program Files\...`) is split at the + * space. Wrapping it in quotes produces the correct `"C:\Program Files\..." args`. + */ +export function quoteWindowsCommand(command: string): string { + if (process.platform !== "win32") return command; + if (!command.includes(" ")) return command; + if (command.startsWith('"') && command.endsWith('"')) return command; + return `"${command}"`; +} + +/** + * `spawn(..., { shell: true })` on Windows also passes argv through `cmd.exe`. + * Any argument containing spaces must be quoted or it will be split before the + * child process sees it. + */ +export function quoteWindowsArgument(argument: string): string { + if (process.platform !== "win32") return argument; + if (!argument.includes(" ")) return argument; + if (argument.startsWith('"') && argument.endsWith('"')) return argument; + return `"${argument}"`; +} + +let cachedShellEnv: Record | null = null; + +export function resolveShellEnv(): Record { + if (cachedShellEnv) return cachedShellEnv; + try { + cachedShellEnv = shellEnvSync(); + } catch { + cachedShellEnv = { ...process.env } as Record; + } + return cachedShellEnv; +}