From 7c50b7a08092b52a5ef579edcd1d5146a4813f4f Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 16 Apr 2026 16:36:13 +0800 Subject: [PATCH] fix(server): harden Codex/Windows startup and provider resolution (#454) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(server): harden Codex/Windows startup and provider resolution Addresses #452, #443, #353, #418, #403, #221, #307, and locks in #284. - Replace custom where.exe/which output parsing with npm `which@^5` plus a spawn probe. findExecutable now enumerates all PATH+PATHEXT candidates and returns the first invokable one. A WindowsApps-ACL'd codex.exe no longer wins over a working codex.cmd (#452). - Make default provider isAvailable() check the binary instead of always returning true. Codex/Claude/OpenCode default isAvailable() now defer to isCommandAvailable() so missing CLIs surface as unavailable instead of throwing later from spawn (#221, #443). - Gate AgentManager.resumeAgentFromPersistence on isAvailable() so a persisted agent record with a missing binary cannot reach provider spawn during rehydration. The daemon stays up and the agent reports unavailable (#443, #353, #418, #403). - Drop --path-format=absolute from rev-parse callers and validate stdout through a shared parser that rejects multi-line output and unknown-flag echoes. --show-toplevel is absolute by default in modern Git; --git-common-dir is resolved against the command cwd. Fixes workspace registration on pre-2.31 Git that echoed the unknown flag and produced a two-line "path" (#307). Tests: - Real-FS executable.test.ts using temp PATH fixtures, covering the .cmd fallback after .exe pre-spawn failure (Windows-only) plus the null-on-no-invokable-candidate case. - provider-availability.test.ts builds real provider clients against a temp-dir-only PATH for Codex/Claude/OpenCode. - bootstrap-provider-availability.test.ts builds the daemon and triggers ensureAgentLoaded so it actually exercises resumeAgentFromPersistence. - claude-agent.spawn.test.ts asserts shell: false reaches spawnProcess from the Claude SDK spawn override, locking in 39b56af4 for #284. - checkout-git-rev-parse.test.ts covers nested-checkout resolution and the old-Git multi-line stdout case via a tightly-scoped runGitCommand fake. CI: - server-tests-windows now also runs the new and modified test files so the Windows behaviors are exercised on windows-latest. * fix: update lockfile signatures and Nix hash * fix(server): handle synchronous spawn UNKNOWN on Windows On Windows, child_process.spawn() throws synchronously when invoked on a corrupt or invalid .exe (e.g., a WindowsApps stub the current user cannot execute, or a zero-byte file). The executable probe did not guard the spawn call, so the synchronous throw rejected the probe promise instead of resolving false, preventing findExecutable() from trying the next candidate. This is the root cause of the daemon-crash pattern in #452: codex.exe from WindowsApps would hard-fail before the .cmd shim ever got a chance. Wrap spawn() in try/catch and settle false on sync throw. The existing error-event and exit-event handlers already cover async failure modes; sync throw just needed one more guard. Also adjust three Windows-only test comparisons that were asserting platform-dependent string equality: - executable.test: compare .cmd paths case-insensitively (which@5 preserves PATHEXT casing, which is uppercase in production). - workspace-registry-model.test: expect normalizeWorkspaceId(path), not the hardcoded POSIX form. - checkout-git-rev-parse.test: normalize separators/case when comparing git's Windows forward-slash output against realpathSync. * test(server): canonicalize repo root via git on Windows to fix short-name mismatch realpathSync on Windows preserves 8.3 short names (e.g. RUNNER~1) while git's rev-parse --show-toplevel always returns the long-name form (runneradmin). Use git as the canonicalizer on both sides of the assertion so the comparison holds regardless of how Windows exposes the temp directory path. * fix(server): Claude is always available in default mode; SDK bundles cli.js The Phase 2 change wrongly tied Claude's default-mode isAvailable() to isCommandAvailable("claude"). Claude's default runtime does not use an external `claude` binary — @anthropic-ai/claude-agent-sdk ships its own cli.js and spawnClaudeCodeProcess runs it via process.execPath. The previous `return true` was correct; restore it. Update provider-availability.test.ts to assert the truthful behavior: Claude reports available even when no `claude` binary is on PATH. Codex and OpenCode genuinely require their binaries on PATH, so their availability checks remain unchanged. --------- Co-authored-by: github-actions[bot] --- .github/workflows/ci.yml | 15 +- nix/package.nix | 2 +- package-lock.json | 25 ++ packages/server/package.json | 1 + .../server/src/server/agent/agent-manager.ts | 6 + .../providers/claude-agent.spawn.test.ts | 103 ++++++ .../server/agent/providers/claude-agent.ts | 2 + .../agent/providers/codex-app-server-agent.ts | 2 +- .../server/agent/providers/opencode-agent.ts | 2 +- .../providers/provider-availability.test.ts | 147 ++++++++ .../bootstrap-provider-availability.test.ts | 116 ++++++ .../src/server/workspace-git-service.ts | 15 +- .../server/workspace-registry-model.test.ts | 32 +- .../src/server/workspace-registry-model.ts | 4 +- .../src/utils/checkout-git-rev-parse.test.ts | 109 ++++++ packages/server/src/utils/checkout-git.ts | 28 +- packages/server/src/utils/executable.test.ts | 336 +++++++----------- packages/server/src/utils/executable.ts | 155 ++++---- .../server/src/utils/git-rev-parse-path.ts | 25 ++ packages/server/src/utils/worktree.ts | 41 +-- 20 files changed, 842 insertions(+), 324 deletions(-) create mode 100644 packages/server/src/server/agent/providers/claude-agent.spawn.test.ts create mode 100644 packages/server/src/server/agent/providers/provider-availability.test.ts create mode 100644 packages/server/src/server/bootstrap-provider-availability.test.ts create mode 100644 packages/server/src/utils/checkout-git-rev-parse.test.ts create mode 100644 packages/server/src/utils/git-rev-parse-path.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4b144c5e1..b8f438ed8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -99,7 +99,20 @@ jobs: - name: Run Windows-critical server tests working-directory: packages/server - run: npx vitest run src/utils/executable.test.ts src/utils/spawn.test.ts src/utils/run-git-command.test.ts src/server/agent/provider-registry.test.ts src/server/agent/provider-launch-config.test.ts src/server/agent/provider-snapshot-manager.test.ts src/server/persisted-config.test.ts + run: > + npx vitest run + src/utils/executable.test.ts + src/utils/spawn.test.ts + src/utils/run-git-command.test.ts + src/utils/checkout-git-rev-parse.test.ts + src/server/agent/provider-registry.test.ts + src/server/agent/provider-launch-config.test.ts + src/server/agent/provider-snapshot-manager.test.ts + src/server/agent/providers/claude-agent.spawn.test.ts + src/server/agent/providers/provider-availability.test.ts + src/server/workspace-registry-model.test.ts + src/server/persisted-config.test.ts + src/server/bootstrap-provider-availability.test.ts app-tests: runs-on: ubuntu-latest diff --git a/nix/package.nix b/nix/package.nix index f3d346853..82309bdad 100644 --- a/nix/package.nix +++ b/nix/package.nix @@ -42,7 +42,7 @@ buildNpmPackage rec { # To update: run `nix build` with lib.fakeHash, copy the `got:` hash. # CI auto-updates this when package-lock.json changes (see .github/workflows/). - npmDepsHash = "sha256-UrTPJVju3jiSlkUMfh25rWobIo9hWNVFoVewFGxRQC0="; + npmDepsHash = "sha256-Wn7g+VXuUe0LI733MUj+WRX/fJQHOO/HFEkqyHrWTf0="; # Prevent onnxruntime-node's install script from running during automatic # npm rebuild (it tries to download from api.nuget.org, which fails in the sandbox). diff --git a/package-lock.json b/package-lock.json index 483514e98..6a4a735f1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -35417,6 +35417,7 @@ "strip-ansi": "^7.1.2", "tiny-invariant": "^1.3.3", "uuid": "^9.0.1", + "which": "^5.0.0", "ws": "^8.14.2", "zod": "^3.23.8", "zod-to-json-schema": "^3.25.1" @@ -35640,6 +35641,15 @@ "node": ">= 0.8" } }, + "packages/server/node_modules/isexe": { + "version": "3.1.5", + "resolved": "https://registry.npmjs.org/isexe/-/isexe-3.1.5.tgz", + "integrity": "sha512-6B3tLtFqtQS4ekarvLVMZ+X+VlvQekbe4taUkf/rhVO3d/h0M2rfARm/pXLcPEsjjMsFgrFgSrhQIxcSVrBz8w==", + "license": "BlueOak-1.0.0", + "engines": { + "node": ">=18" + } + }, "packages/server/node_modules/media-typer": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/media-typer/-/media-typer-1.1.0.tgz", @@ -35794,6 +35804,21 @@ "node": ">= 0.6" } }, + "packages/server/node_modules/which": { + "version": "5.0.0", + "resolved": "https://registry.npmjs.org/which/-/which-5.0.0.tgz", + "integrity": "sha512-JEdGzHwwkrbWoGOlIHqQ5gtprKGOenpDHpxE9zVR1bWbOtYRyPPHMe9FaP6x61CmNaTThSkb0DAJte5jD+DmzQ==", + "license": "ISC", + "dependencies": { + "isexe": "^3.1.1" + }, + "bin": { + "node-which": "bin/which.js" + }, + "engines": { + "node": "^18.17.0 || >=20.5.0" + } + }, "packages/server/node_modules/yocto-queue": { "version": "1.2.2", "resolved": "https://registry.npmjs.org/yocto-queue/-/yocto-queue-1.2.2.tgz", diff --git a/packages/server/package.json b/packages/server/package.json index 84006f5ab..f2b7a6329 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -93,6 +93,7 @@ "strip-ansi": "^7.1.2", "tiny-invariant": "^1.3.3", "uuid": "^9.0.1", + "which": "^5.0.0", "ws": "^8.14.2", "zod": "^3.23.8", "zod-to-json-schema": "^3.25.1" diff --git a/packages/server/src/server/agent/agent-manager.ts b/packages/server/src/server/agent/agent-manager.ts index ba201b3ce..cd1e28f9b 100644 --- a/packages/server/src/server/agent/agent-manager.ts +++ b/packages/server/src/server/agent/agent-manager.ts @@ -800,6 +800,12 @@ export class AgentManager { : overrides; const launchContext = this.buildLaunchContext(resolvedAgentId); const client = this.requireClient(handle.provider); + const available = await client.isAvailable(); + if (!available) { + throw new Error( + `Provider '${handle.provider}' is not available. Please ensure the CLI is installed.`, + ); + } const session = await client.resumeSession(handle, resumeOverrides, launchContext); return this.registerSession(session, normalizedConfig, resolvedAgentId, options); } diff --git a/packages/server/src/server/agent/providers/claude-agent.spawn.test.ts b/packages/server/src/server/agent/providers/claude-agent.spawn.test.ts new file mode 100644 index 000000000..f227c8548 --- /dev/null +++ b/packages/server/src/server/agent/providers/claude-agent.spawn.test.ts @@ -0,0 +1,103 @@ +import { EventEmitter } from "node:events"; +import type { ChildProcess } from "node:child_process"; +import { + query, + type Options, + type Query, + type SpawnOptions as ClaudeSpawnOptions, +} from "@anthropic-ai/claude-agent-sdk"; +import { afterEach, describe, expect, test, vi } from "vitest"; + +import { createTestLogger } from "../../../test-utils/test-logger.js"; +import * as spawnUtils from "../../../utils/spawn.js"; +import { ClaudeAgentClient } from "./claude-agent.js"; + +function createQueryMock(events: unknown[]): Query { + let index = 0; + return { + next: vi.fn(async () => + index < events.length + ? { done: false, value: events[index++] } + : { done: true, value: undefined }, + ), + return: vi.fn(async () => ({ done: true, value: undefined })), + interrupt: vi.fn(async () => undefined), + close: vi.fn(() => undefined), + setPermissionMode: vi.fn(async () => undefined), + setModel: vi.fn(async () => undefined), + supportedModels: vi.fn(async () => [{ value: "opus", displayName: "Opus" }]), + supportedCommands: vi.fn(async () => []), + rewindFiles: vi.fn(async () => ({ canRewind: true })), + [Symbol.asyncIterator]() { + return this; + }, + } as Query; +} + +function createChildProcessStub(): ChildProcess { + return { + stderr: new EventEmitter(), + } as ChildProcess; +} + +describe("Claude spawn override", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("bypasses the shell when spawning Claude Code", async () => { + let capturedOptions: Options | undefined; + const queryFactory = vi.fn(({ options }: Parameters[0]) => { + capturedOptions = options; + return createQueryMock([ + { + type: "system", + subtype: "init", + session_id: "claude-spawn-shell-regression-session", + permissionMode: "default", + model: "opus", + }, + { + type: "assistant", + message: { content: "done" }, + }, + { + type: "result", + subtype: "success", + usage: { + input_tokens: 1, + cache_read_input_tokens: 0, + output_tokens: 1, + }, + total_cost_usd: 0, + }, + ]); + }); + const spawnSpy = vi.spyOn(spawnUtils, "spawnProcess").mockReturnValue(createChildProcessStub()); + const client = new ClaudeAgentClient({ + logger: createTestLogger(), + queryFactory, + }); + const session = await client.createSession({ + provider: "claude", + cwd: process.cwd(), + }); + + try { + await session.run("spawn shell regression"); + capturedOptions?.spawnClaudeCodeProcess?.({ + command: "node", + args: ["claude.js", "--mcp-config", '{"mcpServers":{"paseo":{"type":"http"}}}'], + cwd: process.cwd(), + env: {}, + signal: new AbortController().signal, + } satisfies ClaudeSpawnOptions); + } finally { + await session.close(); + } + + expect(spawnSpy).toHaveBeenCalledTimes(1); + const spawnOptions = spawnSpy.mock.calls[0]?.[2]; + expect(spawnOptions?.shell).toBe(false); + }); +}); diff --git a/packages/server/src/server/agent/providers/claude-agent.ts b/packages/server/src/server/agent/providers/claude-agent.ts index 90b137654..8030e534f 100644 --- a/packages/server/src/server/agent/providers/claude-agent.ts +++ b/packages/server/src/server/agent/providers/claude-agent.ts @@ -1134,6 +1134,8 @@ export class ClaudeAgentClient implements AgentClient { if (command?.mode === "replace") { return await isCommandAvailable(command.argv[0]); } + // Default mode uses @anthropic-ai/claude-agent-sdk's bundled cli.js run + // via process.execPath. No external `claude` binary is required. return true; } 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 d52d4df45..330cf4a37 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 @@ -4205,7 +4205,7 @@ export class CodexAppServerAgentClient implements AgentClient { if (command?.mode === "replace") { return await isCommandAvailable(command.argv[0]); } - return true; + return await isCommandAvailable("codex"); } async getDiagnostic(): Promise<{ diagnostic: string }> { diff --git a/packages/server/src/server/agent/providers/opencode-agent.ts b/packages/server/src/server/agent/providers/opencode-agent.ts index 83cc5b7f0..356a81f78 100644 --- a/packages/server/src/server/agent/providers/opencode-agent.ts +++ b/packages/server/src/server/agent/providers/opencode-agent.ts @@ -1036,7 +1036,7 @@ export class OpenCodeAgentClient implements AgentClient { if (command?.mode === "replace") { return await isCommandAvailable(command.argv[0]); } - return true; + return await isCommandAvailable("opencode"); } async getDiagnostic(): Promise<{ diagnostic: string }> { diff --git a/packages/server/src/server/agent/providers/provider-availability.test.ts b/packages/server/src/server/agent/providers/provider-availability.test.ts new file mode 100644 index 000000000..999f6a669 --- /dev/null +++ b/packages/server/src/server/agent/providers/provider-availability.test.ts @@ -0,0 +1,147 @@ +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; + +import { createTestLogger } from "../../../test-utils/test-logger.js"; +import type { AgentProvider } from "../agent-sdk-types.js"; +import { AgentManager } from "../agent-manager.js"; +import { AgentStorage } from "../agent-storage.js"; + +import { ClaudeAgentClient } from "./claude-agent.js"; +import { CodexAppServerAgentClient } from "./codex-app-server-agent.js"; +import { OpenCodeAgentClient } from "./opencode-agent.js"; + +const originalEnv = { + PATH: process.env.PATH, + PATHEXT: process.env.PATHEXT, +}; +const tempDirs: string[] = []; + +function makeTempDir(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tempDirs.push(dir); + return dir; +} + +function isolatePathTo(dir: string): void { + process.env.PATH = dir; + if (process.platform === "win32") { + process.env.PATHEXT = ".CMD"; + } +} + +function writeProviderShim(dir: string, command: string): string { + const filePath = process.platform === "win32" ? join(dir, `${command}.cmd`) : join(dir, command); + const content = + process.platform === "win32" + ? `@echo off\r\necho ${command} 1.0\r\n` + : `#!/bin/sh\necho ${command} 1.0\n`; + writeFileSync(filePath, content); + if (process.platform !== "win32") { + chmodSync(filePath, 0o755); + } + return filePath; +} + +afterEach(() => { + process.env.PATH = originalEnv.PATH; + process.env.PATHEXT = originalEnv.PATHEXT; + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("default provider availability", () => { + test("Codex reports unavailable when the default command cannot be resolved", async () => { + const binDir = makeTempDir("provider-availability-codex-"); + isolatePathTo(binDir); + const client = new CodexAppServerAgentClient(createTestLogger()); + + await expect(client.isAvailable()).resolves.toBe(false); + }); + + test("Claude reports available without a PATH binary because the SDK bundles its own cli.js", async () => { + const binDir = makeTempDir("provider-availability-claude-"); + isolatePathTo(binDir); + const client = new ClaudeAgentClient({ logger: createTestLogger() }); + + await expect(client.isAvailable()).resolves.toBe(true); + }); + + test("OpenCode reports unavailable when the default command cannot be resolved", async () => { + const binDir = makeTempDir("provider-availability-opencode-"); + isolatePathTo(binDir); + const client = new OpenCodeAgentClient(createTestLogger()); + + await expect(client.isAvailable()).resolves.toBe(false); + }); + + test("Codex reports available when the default command resolves from PATH", async () => { + const binDir = makeTempDir("provider-availability-codex-"); + isolatePathTo(binDir); + writeProviderShim(binDir, "codex"); + const client = new CodexAppServerAgentClient(createTestLogger()); + + await expect(client.isAvailable()).resolves.toBe(true); + }); + + test("OpenCode reports available when the default command resolves from PATH", async () => { + const binDir = makeTempDir("provider-availability-opencode-"); + isolatePathTo(binDir); + writeProviderShim(binDir, "opencode"); + const client = new OpenCodeAgentClient(createTestLogger()); + + await expect(client.isAvailable()).resolves.toBe(true); + }); + + test("AgentManager reports Codex unavailable without throwing", async () => { + const binDir = makeTempDir("provider-availability-manager-bin-"); + isolatePathTo(binDir); + const workdir = makeTempDir("provider-availability-manager-work-"); + const storage = new AgentStorage(join(workdir, "agents"), createTestLogger()); + const manager = new AgentManager({ + clients: { + codex: new CodexAppServerAgentClient(createTestLogger()), + }, + registry: storage, + logger: createTestLogger(), + }); + + await expect(manager.listProviderAvailability()).resolves.toEqual([ + { + provider: "codex", + available: false, + error: null, + }, + ]); + }); + + test("resumeAgentFromPersistence stops before provider spawn when Codex is unavailable", async () => { + const binDir = makeTempDir("provider-availability-resume-bin-"); + isolatePathTo(binDir); + const workdir = makeTempDir("provider-availability-resume-work-"); + const storage = new AgentStorage(join(workdir, "agents"), createTestLogger()); + const manager = new AgentManager({ + clients: { + codex: new CodexAppServerAgentClient(createTestLogger()), + }, + registry: storage, + logger: createTestLogger(), + }); + + await expect( + manager.resumeAgentFromPersistence( + { + provider: "codex" as AgentProvider, + sessionId: "missing-codex-session", + metadata: { + provider: "codex", + cwd: workdir, + }, + }, + { cwd: workdir }, + ), + ).rejects.toThrow("Provider 'codex' is not available"); + }); +}); diff --git a/packages/server/src/server/bootstrap-provider-availability.test.ts b/packages/server/src/server/bootstrap-provider-availability.test.ts new file mode 100644 index 000000000..a3ff7f361 --- /dev/null +++ b/packages/server/src/server/bootstrap-provider-availability.test.ts @@ -0,0 +1,116 @@ +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import pino from "pino"; +import { afterEach, describe, expect, test } from "vitest"; +import { ensureAgentLoaded } from "./agent/agent-loading.js"; +import type { PaseoDaemonConfig } from "./bootstrap.js"; + +const originalEnv = { + PATH: process.env.PATH, + PATHEXT: process.env.PATHEXT, +}; + +describe("bootstrap provider availability", () => { + const tempRoots: string[] = []; + + afterEach(async () => { + process.env.PATH = originalEnv.PATH; + process.env.PATHEXT = originalEnv.PATHEXT; + for (const root of tempRoots.splice(0)) { + await rm(root, { recursive: true, force: true }); + } + }); + + test("loads a persisted Codex record without spawning a missing Codex binary", async () => { + const { createPaseoDaemon } = await import("./bootstrap.js"); + const root = await mkdtemp(path.join(os.tmpdir(), "paseo-bootstrap-provider-")); + tempRoots.push(root); + const binDir = await mkdtemp(path.join(os.tmpdir(), "paseo-bootstrap-provider-bin-")); + tempRoots.push(binDir); + process.env.PATH = binDir; + if (process.platform === "win32") { + process.env.PATHEXT = ".CMD"; + } + const paseoHome = path.join(root, ".paseo"); + const staticDir = path.join(root, "static"); + const agentStoragePath = path.join(paseoHome, "agents"); + const now = new Date("2026-04-16T00:00:00.000Z").toISOString(); + const agentId = "11111111-1111-4111-8111-111111111111"; + await mkdir(agentStoragePath, { recursive: true }); + await mkdir(staticDir, { recursive: true }); + await writeFile( + path.join(agentStoragePath, `${agentId}.json`), + JSON.stringify({ + id: agentId, + provider: "codex", + cwd: root, + createdAt: now, + updatedAt: now, + lastStatus: "idle", + lastModeId: "auto", + config: { + modeId: "auto", + model: "gpt-5.4", + }, + persistence: { + provider: "codex", + sessionId: "codex-session-1", + metadata: { + provider: "codex", + cwd: root, + }, + }, + }), + ); + + const config: PaseoDaemonConfig = { + listen: "127.0.0.1:0", + paseoHome, + corsAllowedOrigins: [], + hostnames: true, + mcpEnabled: false, + staticDir, + mcpDebug: false, + agentClients: {}, + agentStoragePath, + relayEnabled: false, + appBaseUrl: "https://app.paseo.sh", + openai: undefined, + speech: undefined, + }; + const processFailures: Error[] = []; + const onUnhandledRejection = (reason: unknown) => { + processFailures.push(reason instanceof Error ? reason : new Error(String(reason))); + }; + const onUncaughtException = (error: Error) => { + processFailures.push(error); + }; + process.on("unhandledRejection", onUnhandledRejection); + process.on("uncaughtException", onUncaughtException); + + const daemon = await createPaseoDaemon(config, pino({ level: "silent" })); + try { + await expect(daemon.agentStorage.list()).resolves.toHaveLength(1); + await expect(daemon.agentManager.listProviderAvailability()).resolves.toContainEqual({ + provider: "codex", + available: false, + error: null, + }); + await expect( + ensureAgentLoaded(agentId, { + agentManager: daemon.agentManager, + agentStorage: daemon.agentStorage, + logger: pino({ level: "silent" }), + }), + ).rejects.toThrow("Provider 'codex' is not available"); + await new Promise((resolve) => setImmediate(resolve)); + expect(processFailures).toEqual([]); + } finally { + process.off("unhandledRejection", onUnhandledRejection); + process.off("uncaughtException", onUncaughtException); + await daemon.stop().catch(() => undefined); + await daemon.agentManager.flush().catch(() => undefined); + } + }); +}); diff --git a/packages/server/src/server/workspace-git-service.ts b/packages/server/src/server/workspace-git-service.ts index 2adf7d315..1f2e1324a 100644 --- a/packages/server/src/server/workspace-git-service.ts +++ b/packages/server/src/server/workspace-git-service.ts @@ -11,6 +11,7 @@ import { resolveGhPath, resolveAbsoluteGitDir, } from "../utils/checkout-git.js"; +import { parseGitRevParsePath } from "../utils/git-rev-parse-path.js"; import { runGitCommand } from "../utils/run-git-command.js"; import { READ_ONLY_GIT_ENV } from "./checkout-git-utils.js"; import { normalizeWorkspaceId } from "./workspace-registry-model.js"; @@ -368,15 +369,11 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { private async resolveCheckoutWatchRoot(cwd: string): Promise { try { - const { stdout } = await this.deps.runGitCommand( - ["rev-parse", "--path-format=absolute", "--show-toplevel"], - { - cwd, - env: READ_ONLY_GIT_ENV, - }, - ); - const root = stdout.trim(); - return root.length > 0 ? root : null; + const { stdout } = await this.deps.runGitCommand(["rev-parse", "--show-toplevel"], { + cwd, + env: READ_ONLY_GIT_ENV, + }); + return parseGitRevParsePath(stdout); } catch { return null; } diff --git a/packages/server/src/server/workspace-registry-model.test.ts b/packages/server/src/server/workspace-registry-model.test.ts index 2005ae2b5..a05889b16 100644 --- a/packages/server/src/server/workspace-registry-model.test.ts +++ b/packages/server/src/server/workspace-registry-model.test.ts @@ -1,6 +1,10 @@ import { describe, expect, test, vi } from "vitest"; -import { deriveWorkspaceId, detectStaleWorkspaces } from "./workspace-registry-model.js"; +import { + deriveWorkspaceId, + detectStaleWorkspaces, + normalizeWorkspaceId, +} from "./workspace-registry-model.js"; import { createPersistedWorkspaceRecord } from "./workspace-registry.js"; function createWorkspaceRecord(workspaceId: string) { @@ -68,10 +72,28 @@ describe("deriveWorkspaceId", () => { ).toBe("/tmp/repo"); }); - test("falls back to normalized cwd for non-git directories", () => { + test("falls back to normalized cwd when git worktree root contains multiple lines", () => { + const cwd = String.raw`E:\project\node-ai`; + expect( - deriveWorkspaceId("/tmp/repo/../repo/scratch", { - cwd: "/tmp/repo/../repo/scratch", + deriveWorkspaceId(cwd, { + cwd, + isGit: true, + currentBranch: "main", + remoteUrl: null, + worktreeRoot: `--path-format=absolute\n${cwd}`, + isPaseoOwnedWorktree: false, + mainRepoRoot: null, + }), + ).toBe(normalizeWorkspaceId(cwd)); + }); + + test("falls back to normalized cwd for non-git directories", () => { + const cwd = "/tmp/repo/../repo/scratch"; + + expect( + deriveWorkspaceId(cwd, { + cwd, isGit: false, currentBranch: null, remoteUrl: null, @@ -79,6 +101,6 @@ describe("deriveWorkspaceId", () => { isPaseoOwnedWorktree: false, mainRepoRoot: null, }), - ).toBe("/tmp/repo/scratch"); + ).toBe(normalizeWorkspaceId("/tmp/repo/scratch")); }); }); diff --git a/packages/server/src/server/workspace-registry-model.ts b/packages/server/src/server/workspace-registry-model.ts index 8bce33279..976f9edc6 100644 --- a/packages/server/src/server/workspace-registry-model.ts +++ b/packages/server/src/server/workspace-registry-model.ts @@ -1,6 +1,7 @@ import { resolve } from "node:path"; import type { ProjectCheckoutLitePayload, ProjectPlacementPayload } from "../shared/messages.js"; +import { parseGitRevParsePath } from "../utils/git-rev-parse-path.js"; import type { WorkspaceGitService } from "./workspace-git-service.js"; import type { PersistedWorkspaceRecord } from "./workspace-registry.js"; @@ -20,7 +21,8 @@ export function normalizeWorkspaceId(cwd: string): string { } export function deriveWorkspaceId(cwd: string, checkout: ProjectCheckoutLitePayload): string { - return checkout.worktreeRoot ?? normalizeWorkspaceId(cwd); + const worktreeRoot = checkout.worktreeRoot ? parseGitRevParsePath(checkout.worktreeRoot) : null; + return worktreeRoot ?? normalizeWorkspaceId(cwd); } function deriveRemoteProjectKey(remoteUrl: string | null): string | null { diff --git a/packages/server/src/utils/checkout-git-rev-parse.test.ts b/packages/server/src/utils/checkout-git-rev-parse.test.ts new file mode 100644 index 000000000..9096ed89a --- /dev/null +++ b/packages/server/src/utils/checkout-git-rev-parse.test.ts @@ -0,0 +1,109 @@ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const VALID_WINDOWS_ROOT = String.raw`E:\project\node-ai`; +const OLD_GIT_PATH_FORMAT_ECHO = `--path-format=absolute\n${VALID_WINDOWS_ROOT}\n`; +const tempDirs: string[] = []; + +function makeTempDir(): string { + const dir = mkdtempSync(join(tmpdir(), "paseo-checkout-git-")); + tempDirs.push(dir); + return dir; +} + +function gitResult(stdout: string) { + return { stdout, stderr: "", exitCode: 0 }; +} + +function normalizePathForPlatform(value: string): string { + if (process.platform !== "win32") { + return value; + } + return value.replace(/\\/g, "/").toLowerCase(); +} + +function gitCanonicalize(dir: string): string { + return execFileSync("git", ["-C", dir, "rev-parse", "--show-toplevel"], { + encoding: "utf8", + }).trim(); +} + +async function loadCheckoutGitWithRevParseTopLevelOutput(stdout: string) { + vi.resetModules(); + + const runGitCommand = vi.fn(async (args: string[]) => { + if (args.join(" ") === "rev-parse --show-toplevel") { + return gitResult(stdout); + } + + if (args.join(" ") === "rev-parse --abbrev-ref HEAD") { + return gitResult("main\n"); + } + + if (args.join(" ") === "status --porcelain") { + return gitResult(""); + } + + if (args.join(" ") === "branch --format=%(refname:short)") { + return gitResult("main\n"); + } + + throw new Error(`Unexpected git command: git ${args.join(" ")}`); + }); + + vi.doMock("./run-git-command.js", () => ({ runGitCommand })); + + const checkoutGit = await import("./checkout-git.js"); + return { getCheckoutStatus: checkoutGit.getCheckoutStatus, runGitCommand }; +} + +describe("checkout git rev-parse path handling", () => { + afterEach(() => { + vi.doUnmock("./run-git-command.js"); + vi.resetModules(); + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("resolves the worktree root from a nested real git checkout", async () => { + const repoRoot = makeTempDir(); + const nested = join(repoRoot, "packages", "server", "src"); + mkdirSync(nested, { recursive: true }); + execFileSync("git", ["init"], { cwd: repoRoot, stdio: "ignore" }); + + const { getCheckoutStatus } = await import("./checkout-git.js"); + const status = await getCheckoutStatus(nested); + + expect(status.isGit).toBe(true); + if (!status.isGit) { + throw new Error("Expected nested checkout to be detected as a git repository"); + } + expect(normalizePathForPlatform(status.repoRoot)).toBe( + normalizePathForPlatform(gitCanonicalize(repoRoot)), + ); + }); + + it("rejects multi-line rev-parse stdout and never calls the removed path-format command", async () => { + // Pre-2.31 Git is difficult to install in CI; inject its multi-line stdout + // shape at the exact production command boundary that consumes rev-parse. + const { getCheckoutStatus, runGitCommand } = + await loadCheckoutGitWithRevParseTopLevelOutput(OLD_GIT_PATH_FORMAT_ECHO); + + const status = await getCheckoutStatus(VALID_WINDOWS_ROOT); + + expect(status).toEqual({ isGit: false }); + expect(runGitCommand).toHaveBeenCalledWith(["rev-parse", "--show-toplevel"], expect.anything()); + expect(runGitCommand).not.toHaveBeenCalledWith( + ["rev-parse", "--path-format=absolute", "--show-toplevel"], + expect.anything(), + ); + expect(runGitCommand).not.toHaveBeenCalledWith( + ["rev-parse", "--git-common-dir"], + expect.anything(), + ); + }); +}); diff --git a/packages/server/src/utils/checkout-git.ts b/packages/server/src/utils/checkout-git.ts index 7b2567947..6a8d834ef 100644 --- a/packages/server/src/utils/checkout-git.ts +++ b/packages/server/src/utils/checkout-git.ts @@ -5,6 +5,7 @@ import { TTLCache } from "@isaacs/ttlcache"; import type { ParsedDiffFile } from "../server/utils/diff-highlighter.js"; import { parseAndHighlightDiff } from "../server/utils/diff-highlighter.js"; import { findExecutable } from "./executable.js"; +import { parseGitRevParsePath, resolveGitRevParsePath } from "./git-rev-parse-path.js"; import { runGitCommand } from "./run-git-command.js"; import { execCommand } from "./spawn.js"; import { isPaseoOwnedWorktreeCwd } from "./worktree.js"; @@ -539,26 +540,25 @@ export async function getCurrentBranch(cwd: string): Promise { async function getWorktreeRoot(cwd: string): Promise { try { - const { stdout } = await runGitCommand( - ["rev-parse", "--path-format=absolute", "--show-toplevel"], - { - cwd, - env: READ_ONLY_GIT_ENV, - }, - ); - const root = stdout.trim(); - return root.length > 0 ? root : null; + const { stdout } = await runGitCommand(["rev-parse", "--show-toplevel"], { + cwd, + env: READ_ONLY_GIT_ENV, + }); + return parseGitRevParsePath(stdout); } catch { return null; } } export async function getMainRepoRoot(cwd: string): Promise { - const { stdout: commonDirOut } = await runGitCommand( - ["rev-parse", "--path-format=absolute", "--git-common-dir"], - { cwd, env: READ_ONLY_GIT_ENV }, - ); - const commonDir = commonDirOut.trim(); + const { stdout: commonDirOut } = await runGitCommand(["rev-parse", "--git-common-dir"], { + cwd, + env: READ_ONLY_GIT_ENV, + }); + const commonDir = resolveGitRevParsePath(cwd, commonDirOut); + if (!commonDir) { + throw new Error("Not in a git repository"); + } const normalized = realpathSync(commonDir); if (basename(normalized) === ".git") { diff --git a/packages/server/src/utils/executable.test.ts b/packages/server/src/utils/executable.test.ts index 92481a8e3..b24dc2c1a 100644 --- a/packages/server/src/utils/executable.test.ts +++ b/packages/server/src/utils/executable.test.ts @@ -1,213 +1,157 @@ -import { promisify } from "node:util"; -import { afterEach, describe, expect, test, vi } from "vitest"; +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; -type ExecFileCallback = (error: Error | null, stdout: string, stderr: string) => void; +import { + executableExists, + findExecutable, + quoteWindowsArgument, + quoteWindowsCommand, +} from "./executable.js"; -async function loadExecutableModule(params?: { - execFileImpl?: ( - command: string, - args: string[], - options: unknown, - callback: ExecFileCallback, - ) => void; -}) { - vi.resetModules(); +const originalEnv = { + PATH: process.env.PATH, + PATHEXT: process.env.PATHEXT, +}; +const tempDirs: string[] = []; - const execFileMock = vi.fn( - params?.execFileImpl ?? - ((_command: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { - callback(new Error("execFile not mocked"), "", ""); - }), - ); - Object.assign(execFileMock, { - [promisify.custom]: (command: string, args: string[], options: unknown) => - new Promise<{ stdout: string; stderr: string }>((resolve, reject) => { - execFileMock( - command, - args, - options, - (error: Error | null, stdout: string, stderr: string) => { - if (error) { - reject(error); - return; - } - resolve({ stdout, stderr }); - }, - ); - }), - }); - - vi.doMock("node:child_process", () => ({ - execFile: execFileMock, - })); - const module = await import("./executable.js"); - return { - ...module, - execFileMock, - }; +function makeTempDir(): string { + const dir = mkdtempSync(path.join(os.tmpdir(), "paseo-executable-test-")); + tempDirs.push(dir); + return dir; } -describe("findExecutable", () => { - const originalPlatform = process.platform; - const missingBinaryName = "nonexistent-binary-xyz-12345"; +function prependPath(...dirs: string[]): void { + process.env.PATH = [...dirs, originalEnv.PATH].filter(Boolean).join(path.delimiter); +} - function setPlatform(value: string) { - Object.defineProperty(process, "platform", { value, writable: true }); +function writeExecutable(filePath: string, content: string): string { + writeFileSync(filePath, content); + if (process.platform !== "win32") { + chmodSync(filePath, 0o755); } + return filePath; +} - afterEach(() => { - setPlatform(originalPlatform); - }); +function writeInvokableFixture(dir: string, name: string): string { + if (process.platform === "win32") { + return writeExecutable(path.join(dir, `${name}.cmd`), "@echo off\r\necho 0.1\r\n"); + } + return writeExecutable(path.join(dir, name), "#!/bin/sh\necho 0.1\n"); +} - test("on Windows, resolves executables using where.exe with inherited PATH", async () => { - setPlatform("win32"); - const { execFileMock, findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback(null, "C:\\Users\\boudr\\.local\\bin\\claude.exe\r\n", ""); - }, - }); +function writeBrokenAbsoluteFixture(dir: string): string { + const filePath = + process.platform === "win32" ? path.join(dir, "broken.exe") : path.join(dir, "broken"); + writeFileSync(filePath, "not executable"); + if (process.platform !== "win32") { + chmodSync(filePath, 0o644); + } + return filePath; +} - await expect(findExecutable("claude")).resolves.toBe( - "C:\\Users\\boudr\\.local\\bin\\claude.exe", - ); - expect(execFileMock).toHaveBeenCalledOnce(); - const call = execFileMock.mock.calls[0]; - expect(call?.[0]).toBe("where.exe"); - expect(call?.[1]).toEqual(["claude"]); - expect(call?.[2]).toMatchObject({ - encoding: "utf8", - windowsHide: true, +function expectWindowsPathsEqual(actual: string | null, expected: string): void { + expect(actual?.toLowerCase()).toBe(expected.toLowerCase()); +} + +afterEach(() => { + process.env.PATH = originalEnv.PATH; + process.env.PATHEXT = originalEnv.PATHEXT; + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("findExecutable", () => { + describe.skipIf(process.platform === "win32")("POSIX", () => { + test("finds an extensionless executable and skips an earlier non-executable candidate", async () => { + const executableDir = makeTempDir(); + const nonExecutableDir = makeTempDir(); + const executable = writeExecutable(path.join(executableDir, "foo"), "#!/bin/sh\necho 0.1\n"); + const nonExecutable = path.join(nonExecutableDir, "foo"); + writeFileSync(nonExecutable, "#!/bin/sh\necho broken\n"); + chmodSync(nonExecutable, 0o644); + prependPath(nonExecutableDir, executableDir); + + await expect(findExecutable("foo")).resolves.toBe(executable); }); }); - test("on Windows, prefers an executable match from where.exe output", async () => { - setPlatform("win32"); - const { findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback(null, "C:\\nvm4w\\nodejs\\codex\r\nC:\\nvm4w\\nodejs\\codex.cmd\r\n", ""); - }, + describe.runIf(process.platform === "win32")("Windows", () => { + test("returns a working .cmd when an invalid .exe candidate appears first", async () => { + const dir = makeTempDir(); + process.env.PATHEXT = [".EXE", ".CMD"].join(path.delimiter); + const brokenExe = path.join(dir, "foo.exe"); + const cmd = writeExecutable(path.join(dir, "foo.cmd"), "@echo off\r\necho 0.1\r\n"); + writeFileSync(brokenExe, ""); + prependPath(dir); + + expectWindowsPathsEqual(await findExecutable("foo"), cmd); }); - await expect(findExecutable("codex")).resolves.toBe("C:\\nvm4w\\nodejs\\codex.cmd"); + test("returns null when the only candidate is a broken .exe", async () => { + const dir = makeTempDir(); + process.env.PATHEXT = ".EXE"; + writeFileSync(path.join(dir, "foo.exe"), ""); + prependPath(dir); + + await expect(findExecutable("foo")).resolves.toBeNull(); + }); + + test("returns a .cmd when it is the only candidate", async () => { + const dir = makeTempDir(); + process.env.PATHEXT = ".CMD"; + const cmd = writeExecutable(path.join(dir, "foo.cmd"), "@echo off\r\necho 0.1\r\n"); + prependPath(dir); + + expectWindowsPathsEqual(await findExecutable("foo"), cmd); + }); }); - test("on Windows, prefers .exe over .cmd, .ps1, and extensionless candidates", async () => { - setPlatform("win32"); - const { findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback( - null, - [ - "C:\\nvm4w\\nodejs\\codex", - "C:\\nvm4w\\nodejs\\codex.ps1", - "C:\\nvm4w\\nodejs\\codex.cmd", - "C:\\nvm4w\\nodejs\\codex.exe", - ].join("\r\n"), - "", - ); - }, - }); + test("returns an invokable absolute path", async () => { + const dir = makeTempDir(); + const fixture = writeInvokableFixture(dir, "absolute-ok"); - await expect(findExecutable("codex")).resolves.toBe("C:\\nvm4w\\nodejs\\codex.exe"); + await expect(findExecutable(fixture)).resolves.toBe(fixture); }); - test("on Windows, returns null when where.exe output is empty", async () => { - setPlatform("win32"); - const { findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback(null, "\r\n", ""); - }, - }); + test("returns null for an absolute path that cannot spawn", async () => { + const dir = makeTempDir(); + const fixture = writeBrokenAbsoluteFixture(dir); - await expect(findExecutable(missingBinaryName)).resolves.toBeNull(); + await expect(findExecutable(fixture)).resolves.toBeNull(); }); - test("on Windows, falls back to the first extensionless candidate when needed", async () => { - setPlatform("win32"); - const { findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback(null, "C:\\nvm4w\\nodejs\\codex\r\n", ""); - }, - }); + test("returns null when the command is not on PATH", async () => { + const dir = makeTempDir(); + prependPath(dir); - await expect(findExecutable("codex")).resolves.toBe("C:\\nvm4w\\nodejs\\codex"); - }); - - test("on Unix, uses the last line from which output", async () => { - const { execFileMock, findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback(null, "/usr/local/bin/codex\n", ""); - }, - }); - - await expect(findExecutable("codex")).resolves.toBe("/usr/local/bin/codex"); - expect(execFileMock).toHaveBeenCalledWith( - process.platform === "win32" ? "where.exe" : "which", - ["codex"], - process.platform === "win32" ? { encoding: "utf8", windowsHide: true } : { encoding: "utf8" }, - expect.any(Function), - ); - }); - - test.skipIf(process.platform === "win32")( - "warns and returns null when the final which line is not an absolute path", - async () => { - const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); - const { findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback(null, "codex\n", ""); - }, - }); - - await expect(findExecutable("codex")).resolves.toBeNull(); - expect(warnSpy).toHaveBeenCalledOnce(); - - warnSpy.mockRestore(); - }, - ); - - test("returns null when which lookup fails", async () => { - const { findExecutable } = await loadExecutableModule({ - execFileImpl: (_command, _args, _options, callback) => { - callback(new Error("which failed"), "", ""); - }, - }); - - await expect(findExecutable(missingBinaryName)).resolves.toBeNull(); + await expect(findExecutable("paseo-definitely-missing-command")).resolves.toBeNull(); }); }); describe("executableExists", () => { - const originalPlatform = process.platform; - - function setPlatform(value: string) { - Object.defineProperty(process, "platform", { value, writable: true }); - } - - afterEach(() => { - setPlatform(originalPlatform); - }); - - test("returns the path when it already exists", async () => { - const { executableExists } = await loadExecutableModule(); - const exists = vi.fn((candidate: string) => candidate === "/usr/local/bin/codex"); + test("returns the path when it already exists", () => { + const exists = (candidate: string) => candidate === "/usr/local/bin/codex"; expect(executableExists("/usr/local/bin/codex", exists)).toBe("/usr/local/bin/codex"); }); - test("on Windows, falls back to .exe, .cmd, then .ps1 for extensionless paths", async () => { - setPlatform("win32"); - const { executableExists } = await loadExecutableModule(); - const exists = vi.fn((candidate: string) => candidate === "C:\\tools\\codex.cmd"); + test("on Windows, falls back to .exe, .cmd, then .ps1 for extensionless paths", () => { + const originalPlatform = process.platform; + Object.defineProperty(process, "platform", { value: "win32", writable: true }); + try { + const exists = (candidate: string) => candidate === "C:\\tools\\codex.cmd"; - expect(executableExists("C:\\tools\\codex", exists)).toBe("C:\\tools\\codex.cmd"); + expect(executableExists("C:\\tools\\codex", exists)).toBe("C:\\tools\\codex.cmd"); + } finally { + Object.defineProperty(process, "platform", { value: originalPlatform, writable: true }); + } }); - test("returns null when no matching path exists", async () => { - const { executableExists } = await loadExecutableModule(); - const exists = vi.fn(() => false); - - expect(executableExists("/missing/codex", exists)).toBeNull(); + test("returns null when no matching path exists", () => { + expect(executableExists("/missing/codex", () => false)).toBeNull(); }); }); @@ -222,71 +166,61 @@ describe("quoteWindowsCommand", () => { setPlatform(originalPlatform); }); - test("quotes a Windows path with spaces", async () => { + test("quotes a Windows path with spaces", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("C:\\Program Files\\Anthropic\\claude.exe")).toBe( '"C:\\Program Files\\Anthropic\\claude.exe"', ); }); - test("does not double-quote an already-quoted path", async () => { + test("does not double-quote an already-quoted path", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); 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", async () => { + test("returns the command unchanged when there are no spaces", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("C:\\nvm4w\\nodejs\\codex")).toBe("C:\\nvm4w\\nodejs\\codex"); }); - test("escapes ampersands", async () => { + test("escapes ampersands", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("feature&bugfix")).toBe("feature^&bugfix"); }); - test("escapes pipes", async () => { + test("escapes pipes", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("feature|bugfix")).toBe("feature^|bugfix"); }); - test("doubles percent signs", async () => { + test("doubles percent signs", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("100%")).toBe("100%%"); }); - test("escapes carets", async () => { + test("escapes carets", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("feature^bugfix")).toBe("feature^^bugfix"); }); - test("escapes multiple metacharacters", async () => { + test("escapes multiple metacharacters", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("build&(test|deploy)!")).toBe( "build^&^(test^|deploy^)^!^", ); }); - test("quotes commands with spaces after escaping metacharacters", async () => { + test("quotes commands with spaces after escaping metacharacters", () => { setPlatform("win32"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("C:\\Program Files\\My Tool&Stuff\\run 100%.cmd")).toBe( '"C:\\Program Files\\My Tool^&Stuff\\run 100%%.cmd"', ); }); - test("returns the command unchanged on non-Windows platforms", async () => { + test("returns the command unchanged on non-Windows platforms", () => { setPlatform("darwin"); - const { quoteWindowsCommand } = await loadExecutableModule(); expect(quoteWindowsCommand("/usr/local/bin/claude code")).toBe("/usr/local/bin/claude code"); }); }); @@ -302,31 +236,27 @@ describe("quoteWindowsArgument", () => { setPlatform(originalPlatform); }); - test("quotes a Windows argument with spaces", async () => { + test("quotes a Windows argument with spaces", () => { setPlatform("win32"); - const { quoteWindowsArgument } = await loadExecutableModule(); expect(quoteWindowsArgument("C:\\Program Files\\Anthropic\\cli.js")).toBe( '"C:\\Program Files\\Anthropic\\cli.js"', ); }); - test("does not double-quote an already-quoted argument", async () => { + test("does not double-quote an already-quoted argument", () => { setPlatform("win32"); - const { quoteWindowsArgument } = await loadExecutableModule(); 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", async () => { + test("returns the argument unchanged when there are no spaces", () => { setPlatform("win32"); - const { quoteWindowsArgument } = await loadExecutableModule(); expect(quoteWindowsArgument("--version")).toBe("--version"); }); - test("returns the argument unchanged on non-Windows platforms", async () => { + test("returns the argument unchanged on non-Windows platforms", () => { setPlatform("darwin"); - const { quoteWindowsArgument } = await loadExecutableModule(); 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 index 8ed93e382..051a4994b 100644 --- a/packages/server/src/utils/executable.ts +++ b/packages/server/src/utils/executable.ts @@ -1,54 +1,100 @@ -import { execFile } from "node:child_process"; +import { spawn, type ChildProcess } from "node:child_process"; +import { createRequire } from "node:module"; import { existsSync } from "node:fs"; -import path, { extname } from "node:path"; -import { promisify } from "node:util"; +import { extname } from "node:path"; -const execFileAsync = promisify(execFile); +type Which = (command: string, options: { all: true }) => Promise; -function pickBestWindowsCandidate(lines: string[]): string | null { - const candidates = lines.filter((line) => line.length > 0); - if (candidates.length === 0) return null; +const require = createRequire(import.meta.url); +const which = require("which") as Which; +const PROBE_TIMEOUT_MS = 2000; - const extPriority = [".exe", ".cmd", ".ps1"]; - for (const ext of extPriority) { - const match = candidates.find((candidate) => candidate.toLowerCase().endsWith(ext)); - if (match) return match; - } - - return candidates[0] ?? null; +function hasPathSeparator(value: string): boolean { + return value.includes("/") || value.includes("\\"); } -function resolveExecutableFromWhichOutput( - name: string, - output: string, - source: "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; +async function enumerateCandidates(name: string): Promise { + let candidates: string[]; + try { + candidates = await which(name, { all: true }); + } catch (error) { + // `which` throws ENOENT when the command is absent from PATH. + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return []; + } + throw error; } - if (!path.isAbsolute(candidate)) { - console.warn( - `[findExecutable] Ignoring non-absolute ${source} output for '${name}': ${JSON.stringify(candidate)}`, - ); - return null; - } + const seen = new Set(); + return candidates.filter((candidate) => { + if (seen.has(candidate)) { + return false; + } + seen.add(candidate); + return true; + }); +} - return candidate; +function isWindowsCommandScript(executablePath: string): boolean { + const extension = extname(executablePath).toLowerCase(); + return process.platform === "win32" && (extension === ".cmd" || extension === ".bat"); +} + +async function probeExecutable(executablePath: string): Promise { + return await new Promise((resolve) => { + let settled = false; + let started = false; + let timer: ReturnType | undefined; + + const settle = (result: boolean) => { + if (settled) { + return; + } + settled = true; + if (timer) { + clearTimeout(timer); + } + resolve(result); + }; + + let child: ChildProcess; + try { + child = spawn(executablePath, ["--version"], { + stdio: "ignore", + windowsHide: true, + // Windows batch shims (.cmd/.bat) require cmd.exe; native binaries do not. + shell: isWindowsCommandScript(executablePath), + }); + } catch { + settle(false); + return; + } + + timer = setTimeout(() => { + if (started) { + child.kill(); + settle(true); + return; + } + settle(false); + }, PROBE_TIMEOUT_MS); + timer.unref?.(); + + child.once("spawn", () => { + started = true; + }); + child.once("error", () => { + // ENOENT/EACCES/EPERM/UNKNOWN here means the OS could not start the candidate. + settle(started); + }); + child.once("exit", () => { + settle(started); + }); + }); } /** - * On Unix we use `which`. On Windows we use `where.exe`. - * - * 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. + * Check a literal executable path. PATH search is handled by findExecutable(). */ export function executableExists( executablePath: string, @@ -70,35 +116,18 @@ export async function findExecutable(name: string): Promise { return null; } - if (trimmed.includes("/") || trimmed.includes("\\")) { - return executableExists(trimmed); + if (hasPathSeparator(trimmed)) { + return (await probeExecutable(trimmed)) ? trimmed : null; } - if (process.platform === "win32") { - try { - const { stdout } = await execFileAsync("where.exe", [trimmed], { - encoding: "utf8", - windowsHide: true, - }); - return ( - pickBestWindowsCandidate( - stdout - .trim() - .split(/\r?\n/) - .map((line) => line.trim()), - ) ?? null - ); - } catch { - return null; + const candidates = await enumerateCandidates(trimmed); + for (const candidate of candidates) { + if (await probeExecutable(candidate)) { + return candidate; } } - try { - const { stdout } = await execFileAsync("which", [trimmed], { encoding: "utf8" }); - return resolveExecutableFromWhichOutput(trimmed, stdout.trim(), "which"); - } catch { - return null; - } + return null; } export async function isCommandAvailable(command: string): Promise { diff --git a/packages/server/src/utils/git-rev-parse-path.ts b/packages/server/src/utils/git-rev-parse-path.ts new file mode 100644 index 000000000..b79981505 --- /dev/null +++ b/packages/server/src/utils/git-rev-parse-path.ts @@ -0,0 +1,25 @@ +import { resolve } from "node:path"; + +export function parseGitRevParsePath(stdout: string): string | null { + const trimmed = stdout.trim(); + if (!trimmed) { + return null; + } + + const lines = trimmed.split(/\r?\n/); + if (lines.length !== 1) { + return null; + } + + const path = lines[0]?.trim() ?? ""; + if (!path || path.startsWith("--")) { + return null; + } + + return path; +} + +export function resolveGitRevParsePath(cwd: string, stdout: string): string | null { + const parsed = parseGitRevParsePath(stdout); + return parsed ? resolve(cwd, parsed) : null; +} diff --git a/packages/server/src/utils/worktree.ts b/packages/server/src/utils/worktree.ts index e114dce9d..f721c4683 100644 --- a/packages/server/src/utils/worktree.ts +++ b/packages/server/src/utils/worktree.ts @@ -15,6 +15,7 @@ import { import { runGitCommand } from "./run-git-command.js"; import { platformBash, spawnProcess } from "./spawn.js"; import { resolvePaseoHome } from "../server/paseo-home.js"; +import { parseGitRevParsePath, resolveGitRevParsePath } from "./git-rev-parse-path.js"; interface PaseoConfig { worktree?: { @@ -380,14 +381,11 @@ async function inferRepoRootPathFromWorktreePath(worktreePath: string): Promise< } catch { // Fallback: best-effort resolve toplevel (will be the worktree root in typical cases) try { - const { stdout } = await runGitCommand( - ["rev-parse", "--path-format=absolute", "--show-toplevel"], - { - cwd: worktreePath, - env: READ_ONLY_GIT_ENV, - }, - ); - const topLevel = stdout.trim(); + const { stdout } = await runGitCommand(["rev-parse", "--show-toplevel"], { + cwd: worktreePath, + env: READ_ONLY_GIT_ENV, + }); + const topLevel = parseGitRevParsePath(stdout); if (topLevel) { return normalizePathForOwnership(topLevel); } @@ -567,14 +565,11 @@ export async function runWorktreeTeardownCommands(options: { * This is where refs, objects, etc. are stored. */ export async function getGitCommonDir(cwd: string): Promise { - const { stdout } = await runGitCommand( - ["rev-parse", "--path-format=absolute", "--git-common-dir"], - { - cwd, - env: READ_ONLY_GIT_ENV, - }, - ); - const commonDir = stdout.trim(); + const { stdout } = await runGitCommand(["rev-parse", "--git-common-dir"], { + cwd, + env: READ_ONLY_GIT_ENV, + }); + const commonDir = resolveGitRevParsePath(cwd, stdout); if (!commonDir) { throw new Error("Not in a git repository"); } @@ -834,15 +829,11 @@ export async function resolvePaseoWorktreeRootForCwd( let worktreeRoot: string | null = null; try { - const { stdout } = await runGitCommand( - ["rev-parse", "--path-format=absolute", "--show-toplevel"], - { - cwd, - env: READ_ONLY_GIT_ENV, - }, - ); - const trimmed = stdout.trim(); - worktreeRoot = trimmed.length > 0 ? trimmed : null; + const { stdout } = await runGitCommand(["rev-parse", "--show-toplevel"], { + cwd, + env: READ_ONLY_GIT_ENV, + }); + worktreeRoot = parseGitRevParsePath(stdout); } catch { worktreeRoot = null; }