diff --git a/packages/server/src/executable-resolution/executable-resolution.ts b/packages/server/src/executable-resolution/executable-resolution.ts index a6edcc6e7b..f6cf7ffba4 100644 --- a/packages/server/src/executable-resolution/executable-resolution.ts +++ b/packages/server/src/executable-resolution/executable-resolution.ts @@ -6,7 +6,7 @@ import { windowsExecutableResolution } from "./windows.js"; export { quoteWindowsArgument, quoteWindowsCommand } from "../utils/windows-command.js"; -type Which = (command: string, options: { all: true }) => Promise; +type Which = (command: string, options: { all: true; path?: string }) => Promise; const require = createRequire(import.meta.url); const which = require("which") as Which; @@ -35,10 +35,10 @@ async function enumerateCandidatesViaSystemWhich(name: string): Promise { +async function enumerateCandidatesViaLibrary(name: string, path?: string): Promise { let candidates: string[]; try { - candidates = await which(name, { all: true }); + candidates = await which(name, { all: true, ...(path === undefined ? {} : { path }) }); } catch (error) { // `which` throws ENOENT when the command is absent from PATH. if ((error as NodeJS.ErrnoException).code === "ENOENT") { @@ -108,6 +108,23 @@ export function executableExists( return exists(executablePath) ? executablePath : null; } +// PATH lookup that never runs the candidate, unlike findExecutable which probes +// each one with `--version`. A terminal spawn path cannot afford that probe: it +// would execute the profile command once before the terminal even opens. +// `path` overrides the PATH to search, so a caller can resolve against the +// environment its child process will actually get rather than the daemon's own. +export async function findExecutableWithoutProbing( + name: string, + options: { path?: string } = {}, +): Promise { + const trimmed = name.trim(); + if (!trimmed) { + return null; + } + const candidates = await enumerateCandidatesViaLibrary(trimmed, options.path); + return candidates[0] ?? null; +} + export async function findExecutable( name: string, probeTimeoutMs = PROBE_TIMEOUT_MS, diff --git a/packages/server/src/terminal/terminal.test.ts b/packages/server/src/terminal/terminal.test.ts index 9881f4bdfd..fbd20f732b 100644 --- a/packages/server/src/terminal/terminal.test.ts +++ b/packages/server/src/terminal/terminal.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, afterEach, vi } from "vitest"; import { isPlatform } from "../test-utils/platform.js"; import { buildTerminalEnvironment, + assertTerminalCommandCanStart, createTerminal, ensureNodePtySpawnHelperExecutableForCurrentPlatform, resolveDefaultTerminalShell, @@ -24,7 +25,7 @@ import { } from "node:fs"; import { spawnSync } from "node:child_process"; import * as pty from "node-pty"; -import { join } from "node:path"; +import { delimiter, join } from "node:path"; import { tmpdir } from "node:os"; import { setImmediate as waitForImmediate, setTimeout as delay } from "node:timers/promises"; import { stripVTControlCharacters } from "node:util"; @@ -593,6 +594,148 @@ describe("createTerminal", () => { expect(exitInfo.lastOutputLines.length).toBeGreaterThan(0); expect(exitInfo.lastOutputLines[exitInfo.lastOutputLines.length - 1]).toBe("line-2999"); }); + + it.runIf(isPlatform("linux", "darwin"))( + "rejects a profile command that is not installed instead of opening a terminal", + async () => { + await expect( + createTerminal({ + workspaceId: "ws-test", + cwd: realpathSync(tmpdir()), + name: "Codex", + command: "paseo-qa-absent-codex", + args: [], + }), + ).rejects.toThrow("paseo-qa-absent-codex: command not found"); + }, + ); + + it.runIf(isPlatform("linux", "darwin"))( + "opens a command found only on the PATH carried by the env option", + async () => { + // execve resolves the command against the child's PATH, which carries the + // per-cwd registered environment. A version-manager shim lives there and + // not necessarily on the daemon's own PATH. + const binDir = mkdtempSync(join(tmpdir(), "terminal-envpath-bin-")); + temporaryDirs.push(binDir); + const toolPath = join(binDir, "paseo-qa-envpath-tool"); + writeFileSync(toolPath, "#!/usr/bin/env bash\nsleep 30\n"); + chmodSync(toolPath, 0o755); + + const session = trackSession( + await createTerminal({ + workspaceId: "ws-test", + cwd: realpathSync(tmpdir()), + name: "Shim", + command: "paseo-qa-envpath-tool", + args: [], + env: { PATH: `${binDir}${delimiter}${process.env.PATH ?? ""}` }, + }), + ); + + expect(session.id).toBeDefined(); + }, + ); + + it.runIf(isPlatform("linux", "darwin"))( + "reports command not found when the command is on neither PATH", + async () => { + await expect( + createTerminal({ + workspaceId: "ws-test", + cwd: realpathSync(tmpdir()), + name: "Codex", + command: "paseo-qa-absent-codex", + args: [], + env: { PATH: `/paseo-qa-empty${delimiter}${process.env.PATH ?? ""}` }, + }), + ).rejects.toThrow("paseo-qa-absent-codex: command not found"); + }, + ); + + it.runIf(isPlatform("linux", "darwin"))( + "opens a relative command path resolved against the terminal cwd", + async () => { + // execve resolves a path-ish command against the child's cwd, never the + // daemon's own working directory. + const projectDir = mkdtempSync(join(tmpdir(), "terminal-relative-cmd-")); + temporaryDirs.push(projectDir); + mkdirSync(join(projectDir, "scripts")); + const scriptPath = join(projectDir, "scripts", "dev.sh"); + writeFileSync(scriptPath, "#!/usr/bin/env bash\nsleep 30\n"); + chmodSync(scriptPath, 0o755); + + const session = trackSession( + await createTerminal({ + workspaceId: "ws-test", + cwd: realpathSync(projectDir), + name: "Dev script", + command: "./scripts/dev.sh", + args: [], + }), + ); + + expect(session.id).toBeDefined(); + }, + ); + + it.runIf(isPlatform("linux", "darwin"))("opens an absolute command path", async () => { + const projectDir = mkdtempSync(join(tmpdir(), "terminal-absolute-cmd-")); + temporaryDirs.push(projectDir); + const scriptPath = join(projectDir, "run.sh"); + writeFileSync(scriptPath, "#!/usr/bin/env bash\nsleep 30\n"); + chmodSync(scriptPath, 0o755); + + const session = trackSession( + await createTerminal({ + workspaceId: "ws-test", + cwd: realpathSync(tmpdir()), + name: "Absolute", + command: scriptPath, + args: [], + }), + ); + + expect(session.id).toBeDefined(); + }); + + it("leaves the command check to the terminal itself on Windows", async () => { + // Windows resolves inside resolveTerminalSpawnCommand, which has its own + // shim handling and its own fallback for an unresolved command. + await expect( + assertTerminalCommandCanStart({ + command: "paseo-qa-absent-codex", + cwd: realpathSync(tmpdir()), + env: { PATH: "/paseo-qa-empty" }, + platform: "win32", + }), + ).resolves.toBeUndefined(); + }); + + it.runIf(isPlatform("linux", "darwin"))( + "still opens a terminal for an installed command that exits non-zero", + async () => { + // A profile running a failing test suite must keep opening a tab with its + // output. Only a command that cannot start at all is reported as an error, + // and "exited non-zero" does not distinguish the two: a missing binary and + // `false` both surface as exit code 1 from node-pty. + const session = trackSession( + await createTerminal({ + workspaceId: "ws-test", + cwd: realpathSync(tmpdir()), + name: "Failing suite", + command: "false", + args: [], + }), + ); + + const exitInfo = await new Promise<{ exitCode: number | null }>((resolve) => { + session.onExit((info) => resolve({ exitCode: info.exitCode })); + }); + + expect(exitInfo).toEqual({ exitCode: 1 }); + }, + ); }); describe.skipIf(isPlatform("win32"))("send input", () => { diff --git a/packages/server/src/terminal/terminal.ts b/packages/server/src/terminal/terminal.ts index b9d1b74017..4892339472 100644 --- a/packages/server/src/terminal/terminal.ts +++ b/packages/server/src/terminal/terminal.ts @@ -1,14 +1,25 @@ import * as pty from "node-pty"; import xterm, { type Terminal as TerminalType } from "@xterm/headless"; import { randomUUID } from "crypto"; -import { chmodSync, existsSync, mkdirSync, readFileSync, statSync } from "node:fs"; +import { + accessSync, + chmodSync, + constants, + existsSync, + mkdirSync, + readFileSync, + statSync, +} from "node:fs"; import { tmpdir, userInfo } from "node:os"; import { basename, delimiter, dirname, extname, join, resolve as resolvePath } from "node:path"; import { createRequire } from "node:module"; import { fileURLToPath } from "node:url"; import { createExternalProcessEnv } from "../server/paseo-env.js"; import { writePrivateFileAtomicSync } from "../server/private-files.js"; -import { findExecutable } from "../executable-resolution/executable-resolution.js"; +import { + findExecutable, + findExecutableWithoutProbing, +} from "../executable-resolution/executable-resolution.js"; import type { TerminalCell, TerminalState } from "@getpaseo/protocol/messages"; import { TerminalInputModeTracker } from "@getpaseo/protocol/terminal-input-mode"; import { TerminalActivityTracker } from "./activity/terminal-activity-tracker.js"; @@ -33,6 +44,20 @@ export interface TerminalExitInfo { lastOutputLines: string[]; } +// The field is assigned explicitly rather than declared as a constructor +// parameter property: the terminal worker loads this module under +// --experimental-strip-types, which erases types but cannot emit the +// assignment a parameter property implies. +export class TerminalCommandNotFoundError extends Error { + readonly command: string; + + constructor(command: string) { + super(`${command}: command not found`); + this.name = "TerminalCommandNotFoundError"; + this.command = command; + } +} + export interface TerminalCommandFinishedInfo { exitCode: number | null; } @@ -882,6 +907,59 @@ function extractLastOutputLinesFromText(text: string, limit: number): string[] { return lines.slice(-limit); } +function isExecutableFile(path: string): boolean { + try { + if (!statSync(path).isFile()) { + return false; + } + accessSync(path, constants.X_OK); + return true; + } catch { + return false; + } +} + +export interface AssertTerminalCommandCanStartInput { + command: string | undefined; + // The terminal's own cwd and environment, not the daemon's: execve in the + // child resolves against those, so anything else produces false negatives. + cwd: string; + env: Record; + platform?: NodeJS.Platform; +} + +// node-pty spawns directly through execve on POSIX, with no shell, so a command +// that does not exist fails inside the spawn helper and surfaces as exit code 1 +// with no output -- indistinguishable from `false` or any program that simply +// failed. Resolving it up front is the only signal that separates "cannot start" +// from "ran and exited non-zero", and it lets the create report the failure +// instead of handing back a terminal that dies immediately. Windows keeps +// resolving inside resolveTerminalSpawnCommand, which has its own shim handling +// and its own fallback for an unresolved command. +export async function assertTerminalCommandCanStart( + input: AssertTerminalCommandCanStartInput, +): Promise { + const { command, cwd, env } = input; + const platform = input.platform ?? process.platform; + if (command === undefined || platform === "win32") { + return; + } + + // A path-ish command is resolved by execve against the child's cwd, never + // through PATH and never against the daemon's working directory. + if (command.includes("/")) { + if (isExecutableFile(resolvePath(cwd, command))) { + return; + } + throw new TerminalCommandNotFoundError(command); + } + + const resolved = await findExecutableWithoutProbing(command, { path: env[getPathEnvKey(env)] }); + if (!resolved) { + throw new TerminalCommandNotFoundError(command); + } +} + export async function createTerminal(options: CreateTerminalOptions): Promise { const { cwd, @@ -941,19 +1019,23 @@ export async function createTerminal(options: CreateTerminalOptions): Promise { + const worker = new FakeTerminalWorker(); + manager = createWorkerTerminalManager({ + requestTimeoutMs: 50, + forkWorker: () => worker, + }); + + const snapshots: Array<{ cwd: string; terminalIds: string[] }> = []; + manager.subscribeTerminalsChanged((event) => { + snapshots.push({ cwd: event.cwd, terminalIds: event.terminals.map((terminal) => terminal.id) }); + }); + + const created = manager.createTerminal({ + id: "terminal-a", + cwd: "/workspace", + workspaceId: "ws-test", + }); + const request = worker.sentMessages.find((message) => message.type === "createTerminal"); + if (!request) { + throw new Error("createTerminal request not sent"); + } + + // The worker delivers all three messages in one tick, so the awaiting + // createTerminal continuation resumes only after the exit was processed. + const terminal = { + id: "terminal-a", + name: "Shell", + cwd: "/workspace", + workspaceId: "ws-test", + activity: null, + }; + const state = createTerminalState(); + const exitInfo = { exitCode: 0, signal: null, lastOutputLines: [] }; + worker.emitWorkerMessage({ type: "terminalCreated", terminal, state }); + worker.emitWorkerMessage({ + type: "response", + requestId: request.requestId, + ok: true, + result: { terminal, state }, + }); + worker.emitWorkerMessage({ type: "terminalExit", terminalId: "terminal-a", info: exitInfo }); + + const session = await created; + + expect(manager.getTerminal("terminal-a")).toBeUndefined(); + expect(await manager.getTerminals("/workspace")).toEqual([]); + expect(session.getExitInfo()).toEqual(exitInfo); + expect(snapshots).toEqual([ + { cwd: "/workspace", terminalIds: ["terminal-a"] }, + { cwd: "/workspace", terminalIds: [] }, + ]); +}); + +it("replays the exit to a listener attached after a create that lost the race", async () => { + const worker = new FakeTerminalWorker(); + manager = createWorkerTerminalManager({ + requestTimeoutMs: 50, + forkWorker: () => worker, + }); + + const created = manager.createTerminal({ + id: "terminal-a", + cwd: "/workspace", + workspaceId: "ws-test", + }); + const request = worker.sentMessages.find((message) => message.type === "createTerminal"); + if (!request) { + throw new Error("createTerminal request not sent"); + } + + const terminal = { + id: "terminal-a", + name: "Shell", + cwd: "/workspace", + workspaceId: "ws-test", + activity: null, + }; + const state = createTerminalState(); + const exitInfo = { exitCode: 0, signal: null, lastOutputLines: [] }; + worker.emitWorkerMessage({ type: "terminalCreated", terminal, state }); + worker.emitWorkerMessage({ + type: "response", + requestId: request.requestId, + ok: true, + result: { terminal, state }, + }); + worker.emitWorkerMessage({ type: "terminalExit", terminalId: "terminal-a", info: exitInfo }); + + const session = await created; + + // The session controller subscribes only after createTerminal resolves; that + // subscription is what turns into a terminal_exit for the client. + const observedExits: Array<{ exitCode: number | null }> = []; + session.onExit((info) => { + observedExits.push({ exitCode: info.exitCode }); + }); + await new Promise((resolve) => setImmediate(resolve)); + + expect(observedExits).toEqual([{ exitCode: 0 }]); +}); + +it("rejects createTerminal and registers nothing when the worker cannot start the command", async () => { + const worker = new FakeTerminalWorker(); + manager = createWorkerTerminalManager({ + requestTimeoutMs: 50, + forkWorker: () => worker, + }); + + const snapshots: Array<{ cwd: string; terminalIds: string[] }> = []; + manager.subscribeTerminalsChanged((event) => { + snapshots.push({ cwd: event.cwd, terminalIds: event.terminals.map((terminal) => terminal.id) }); + }); + + const created = manager.createTerminal({ + id: "terminal-a", + cwd: "/workspace", + workspaceId: "ws-test", + command: "paseo-qa-absent-codex", + }); + const request = worker.sentMessages.find((message) => message.type === "createTerminal"); + if (!request) { + throw new Error("createTerminal request not sent"); + } + worker.emitWorkerMessage({ + type: "response", + requestId: request.requestId, + ok: false, + error: "paseo-qa-absent-codex: command not found", + }); + + await expect(created).rejects.toThrow("paseo-qa-absent-codex: command not found"); + expect(manager.getTerminal("terminal-a")).toBeUndefined(); + expect(await manager.getTerminals("/workspace")).toEqual([]); + expect(snapshots).toEqual([]); +}); diff --git a/packages/server/src/terminal/worker-terminal-manager.ts b/packages/server/src/terminal/worker-terminal-manager.ts index 0f0243d7a8..240a42143b 100644 --- a/packages/server/src/terminal/worker-terminal-manager.ts +++ b/packages/server/src/terminal/worker-terminal-manager.ts @@ -156,6 +156,14 @@ export function createWorkerTerminalManager( const recordsById = new Map(); const terminalIdsByCwd = new Map>(); const terminalActivityTokenById = new Map(); + // A terminal can die before its createTerminal response is handled: the worker + // sends terminalCreated, the response, and terminalExit in the same tick, so + // the awaiting create continuation resumes after the exit was already + // processed. These hold the ids being created and the finished records their + // exit removed, so that continuation returns the exited session instead of + // registering a live one for a terminal the worker no longer owns. + const creatingTerminalIds = new Set(); + const exitedDuringCreateById = new Map(); const terminalsChangedListeners = new Set(); const terminalActivityListeners = new Set(); const terminalWorkspaceContributionChangedListeners = @@ -448,6 +456,9 @@ export function createWorkerTerminalManager( record.exitListeners.clear(); const previousBucket = deriveTerminalActivityStatusBucket(record.activity); const removedRecord = removeRecord(message.terminalId); + if (removedRecord && creatingTerminalIds.has(message.terminalId)) { + exitedDuringCreateById.set(message.terminalId, removedRecord); + } if (previousBucket !== null && removedRecord) { emitTerminalWorkspaceContributionChanged({ terminalId: removedRecord.info.id, @@ -686,6 +697,7 @@ export function createWorkerTerminalManager( const activityToken = createActivityToken(); const terminalActivityUrl = managerOptions.getTerminalActivityUrl?.() ?? null; terminalActivityTokenById.set(terminalId, activityToken); + creatingTerminalIds.add(terminalId); let result: { terminal: RequiredWorkerTerminalInfo; state: TerminalState; @@ -705,7 +717,15 @@ export function createWorkerTerminalManager( }; } catch (error) { terminalActivityTokenById.delete(terminalId); + exitedDuringCreateById.delete(terminalId); throw error; + } finally { + creatingTerminalIds.delete(terminalId); + } + const exitedDuringCreate = exitedDuringCreateById.get(terminalId); + if (exitedDuringCreate) { + exitedDuringCreateById.delete(terminalId); + return exitedDuringCreate.session; } const session = registerRecord({ info: result.terminal, state: result.state }); return session;