diff --git a/cli/src/runner/run.ts b/cli/src/runner/run.ts index ea827f87..694ad719 100644 --- a/cli/src/runner/run.ts +++ b/cli/src/runner/run.ts @@ -21,6 +21,7 @@ import { isRetryableConnectionError } from '@/utils/errorUtils'; import { cleanupRunnerState, getInstalledCliMtimeMs, isRunnerRunningCurrentlyInstalledHappyVersion, stopRunner, waitForRunnerHandoff } from './controlClient'; import { startRunnerControlServer } from './controlServer'; import { createWorktree, removeWorktree, type WorktreeInfo } from './worktree'; +import { validateWorkspaceDirectory } from './validateWorkspaceDirectory'; import { join } from 'path'; import { buildMachineMetadata } from '@/agent/sessionFactory'; import { resolveWorkspaceRoots } from '@/utils/workspaceRoot'; @@ -295,47 +296,28 @@ export async function startRunner(options: { workspaceRoots?: string[] } = {}): let happyProcess: ReturnType | null = null; if (sessionType === 'simple') { - try { - await fs.access(directory); + const validation = await validateWorkspaceDirectory(directory, { + approvedNewDirectoryCreation + }); + if (validation.type === 'requestApproval') { + logger.debug(`[RUNNER RUN] Directory creation not approved for: ${directory}`); + return { + type: 'requestToApproveDirectoryCreation', + directory + }; + } + if (validation.type === 'error') { + logger.debug(`[RUNNER RUN] Workspace directory validation failed: ${validation.errorMessage}`); + return { + type: 'error', + errorMessage: validation.errorMessage + }; + } + directoryCreated = validation.created; + if (validation.created) { + logger.debug(`[RUNNER RUN] Successfully created directory: ${directory}`); + } else { logger.debug(`[RUNNER RUN] Directory exists: ${directory}`); - } catch (error) { - logger.debug(`[RUNNER RUN] Directory doesn't exist, creating: ${directory}`); - - // Check if directory creation is approved - if (!approvedNewDirectoryCreation) { - logger.debug(`[RUNNER RUN] Directory creation not approved for: ${directory}`); - return { - type: 'requestToApproveDirectoryCreation', - directory - }; - } - - try { - await fs.mkdir(directory, { recursive: true }); - logger.debug(`[RUNNER RUN] Successfully created directory: ${directory}`); - directoryCreated = true; - } catch (mkdirError: any) { - let errorMessage = `Unable to create directory at '${directory}'. `; - - // Provide more helpful error messages based on the error code - if (mkdirError.code === 'EACCES') { - errorMessage += `Permission denied. You don't have write access to create a folder at this location. Try using a different path or check your permissions.`; - } else if (mkdirError.code === 'ENOTDIR') { - errorMessage += `A file already exists at this path or in the parent path. Cannot create a directory here. Please choose a different location.`; - } else if (mkdirError.code === 'ENOSPC') { - errorMessage += `No space left on device. Your disk is full. Please free up some space and try again.`; - } else if (mkdirError.code === 'EROFS') { - errorMessage += `The file system is read-only. Cannot create directories here. Please choose a writable location.`; - } else { - errorMessage += `System error: ${mkdirError.message || mkdirError}. Please verify the path is valid and you have the necessary permissions.`; - } - - logger.debug(`[RUNNER RUN] Directory creation failed: ${errorMessage}`); - return { - type: 'error', - errorMessage - }; - } } } else { try { diff --git a/cli/src/runner/validateWorkspaceDirectory.test.ts b/cli/src/runner/validateWorkspaceDirectory.test.ts new file mode 100644 index 00000000..9798c97a --- /dev/null +++ b/cli/src/runner/validateWorkspaceDirectory.test.ts @@ -0,0 +1,209 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { mkdtemp, mkdir, writeFile, symlink, rm } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +import { + describeMkdirError, + validateWorkspaceDirectory, +} from './validateWorkspaceDirectory'; + +let workRoot: string; + +beforeEach(async () => { + workRoot = await mkdtemp(join(tmpdir(), 'hapi-validate-workspace-')); +}); + +afterEach(async () => { + await rm(workRoot, { recursive: true, force: true }); +}); + +describe('validateWorkspaceDirectory', () => { + it('creates a missing directory when approved', async () => { + const target = join(workRoot, 'new-workspace'); + const result = await validateWorkspaceDirectory(target, { + approvedNewDirectoryCreation: true, + }); + expect(result).toEqual({ type: 'ok', created: true }); + }); + + it('requests approval when path is missing and creation is not approved', async () => { + const target = join(workRoot, 'unapproved'); + const result = await validateWorkspaceDirectory(target, { + approvedNewDirectoryCreation: false, + }); + expect(result).toEqual({ type: 'requestApproval' }); + }); + + it('returns ok without creating when path is already a directory', async () => { + const target = join(workRoot, 'existing-dir'); + await mkdir(target); + const result = await validateWorkspaceDirectory(target, { + approvedNewDirectoryCreation: true, + }); + expect(result).toEqual({ type: 'ok', created: false }); + }); + + it('returns an error when path is a regular file', async () => { + const target = join(workRoot, 'collision-file'); + await writeFile(target, 'hello'); + const result = await validateWorkspaceDirectory(target, { + approvedNewDirectoryCreation: true, + }); + expect(result.type).toBe('error'); + if (result.type === 'error') { + expect(result.errorMessage).toContain('non-directory file'); + expect(result.errorMessage).toContain(target); + } + }); + + it('preserves the ENOTDIR diagnostic when the parent path is a regular file', async () => { + const parentFile = join(workRoot, 'parent-file'); + await writeFile(parentFile, 'hello'); + const target = join(parentFile, 'child-dir'); + const result = await validateWorkspaceDirectory(target, { + approvedNewDirectoryCreation: true, + }); + expect(result.type).toBe('error'); + if (result.type === 'error') { + expect(result.errorMessage).toContain(target); + expect(result.errorMessage).toMatch(/file already exists/i); + expect(result.errorMessage).not.toMatch(/Unable to inspect workspace path/); + } + }); + + it('returns ok when path is a symlink to an existing directory', async () => { + const realTarget = join(workRoot, 'real-target'); + await mkdir(realTarget); + const link = join(workRoot, 'good-symlink'); + await symlink(realTarget, link); + + const result = await validateWorkspaceDirectory(link, { + approvedNewDirectoryCreation: true, + }); + expect(result).toEqual({ type: 'ok', created: false }); + }); + + it('returns a diagnostic error when path is a dangling symlink', async () => { + const missingTarget = join(workRoot, 'gone-target'); + const link = join(workRoot, 'dangling-symlink'); + await symlink(missingTarget, link); + // missingTarget is never created, so the link is dangling. + + const result = await validateWorkspaceDirectory(link, { + approvedNewDirectoryCreation: true, + }); + expect(result.type).toBe('error'); + if (result.type === 'error') { + expect(result.errorMessage).toContain(link); + expect(result.errorMessage).toContain(missingTarget); + expect(result.errorMessage).toMatch(/symbolic link/i); + expect(result.errorMessage).toMatch(/no longer exists/i); + expect(result.errorMessage).toMatch(/Recovery:/); + expect(result.errorMessage).not.toMatch(/EEXIST/); + // Regression: must not embed the user-controlled path inside a + // copy-pasteable shell command (`rm '...'`) - a path with a + // single quote would break the quoting and create an injection + // / accidental-delete vector. (Codex review on PR #892.) + expect(result.errorMessage).not.toMatch(/`rm /); + } + }); + + it('does not produce a copy-pasteable rm command when the path contains a single quote', async () => { + // Regression for the PR #892 Codex review Major: paths with + // single quotes used to break out of the literal `rm '...'` + // recovery hint and turn the diagnostic into a shell-injection + // / accidental-delete vector. + const trickyDir = join(workRoot, "weird'name"); + await mkdir(trickyDir); + const missingTarget = join(trickyDir, 'gone-target'); + const link = join(trickyDir, 'dangling-symlink'); + await symlink(missingTarget, link); + + const result = await validateWorkspaceDirectory(link, { + approvedNewDirectoryCreation: true, + }); + expect(result.type).toBe('error'); + if (result.type === 'error') { + expect(result.errorMessage).toContain(link); + expect(result.errorMessage).toContain(missingTarget); + expect(result.errorMessage).not.toMatch(/`rm /); + } + }); + + it('returns an error when path is a symlink to a non-directory', async () => { + const targetFile = join(workRoot, 'target-file'); + await writeFile(targetFile, 'hello'); + const link = join(workRoot, 'symlink-to-file'); + await symlink(targetFile, link); + + const result = await validateWorkspaceDirectory(link, { + approvedNewDirectoryCreation: true, + }); + expect(result.type).toBe('error'); + if (result.type === 'error') { + expect(result.errorMessage).toContain(link); + expect(result.errorMessage).toContain(targetFile); + expect(result.errorMessage).toMatch(/not a directory/i); + } + }); +}); + +describe('describeMkdirError', () => { + const directory = '/tmp/hapi-test-target'; + + it('produces a Permission denied message for EACCES', () => { + const msg = describeMkdirError(directory, { + code: 'EACCES', + message: 'permission denied', + }); + expect(msg).toContain(directory); + expect(msg).toContain('Permission denied'); + }); + + it('produces an ENOTDIR message for ENOTDIR', () => { + const msg = describeMkdirError(directory, { + code: 'ENOTDIR', + message: 'not a directory', + }); + expect(msg).toContain(directory); + expect(msg).toMatch(/file already exists/i); + }); + + it('produces a No space left on device message for ENOSPC', () => { + const msg = describeMkdirError(directory, { + code: 'ENOSPC', + message: 'no space left on device', + }); + expect(msg).toContain(directory); + expect(msg).toMatch(/No space left on device/i); + }); + + it('produces a read-only file system message for EROFS', () => { + const msg = describeMkdirError(directory, { + code: 'EROFS', + message: 'read-only file system', + }); + expect(msg).toContain(directory); + expect(msg).toMatch(/read-only/i); + }); + + it('produces a non-directory race message for EEXIST', () => { + const msg = describeMkdirError(directory, { + code: 'EEXIST', + message: 'file already exists', + }); + expect(msg).toContain(directory); + expect(msg).toMatch(/non-directory file/i); + expect(msg).not.toMatch(/EEXIST/); + }); + + it('falls back to System error for unknown codes', () => { + const msg = describeMkdirError(directory, { + code: 'EWEIRD', + message: 'something strange', + }); + expect(msg).toContain(directory); + expect(msg).toContain('System error: something strange'); + }); +}); diff --git a/cli/src/runner/validateWorkspaceDirectory.ts b/cli/src/runner/validateWorkspaceDirectory.ts new file mode 100644 index 00000000..d33fcf98 --- /dev/null +++ b/cli/src/runner/validateWorkspaceDirectory.ts @@ -0,0 +1,237 @@ +import fs from 'fs/promises'; + +/** + * Result of validating (and optionally creating) a workspace directory before + * a session is spawned at it. + * + * - `ok`: the directory exists (or was just created) and is usable as a cwd. + * `created` distinguishes the just-created case so the runner can surface a + * user-visible "we created this folder for you" message. + * - `requestApproval`: the path does not exist and the caller has not approved + * new-directory creation. Surfaces back to the web UI as the existing + * `requestToApproveDirectoryCreation` flow. + * - `error`: validation failed. `errorMessage` is the user-facing string and + * is preferred over leaking raw kernel errors (EEXIST etc.). + */ +export type ValidateWorkspaceDirectoryResult = + | { type: 'ok'; created: boolean } + | { type: 'requestApproval' } + | { type: 'error'; errorMessage: string }; + +export interface ValidateWorkspaceDirectoryOptions { + approvedNewDirectoryCreation: boolean; +} + +/** + * Resolve a workspace directory before spawning a session at it. + * + * Replaces the historic `fs.access` + `fs.mkdir({ recursive: true })` pair in + * `run.ts`, which produced a misleading EEXIST error on dangling symlinks + * (symlink points at a deleted target, `fs.access` follows the link and + * throws ENOENT, then `mkdir` cannot tolerate the existing non-directory + * entry and surfaces `EEXIST: file already exists, mkdir '...'` to the user). + * + * The replacement uses `fs.lstat` so symlinks are inspected without being + * followed, distinguishes dangling symlinks from genuinely missing paths and + * from regular files squatting at the workspace path, and only attempts + * `mkdir` when the path truly does not exist. + */ +export async function validateWorkspaceDirectory( + directory: string, + options: ValidateWorkspaceDirectoryOptions +): Promise { + const { approvedNewDirectoryCreation } = options; + + let lstat: Awaited> | null = null; + try { + lstat = await fs.lstat(directory); + } catch (err: any) { + if (err?.code === 'ENOENT') { + // path does not exist - fall through to mkdir / approval flow + } else if (err?.code === 'ENOTDIR') { + // Parent path contains a regular file; preserve the historic + // mkdir ENOTDIR diagnostic instead of the generic inspect text. + return { + type: 'error', + errorMessage: describeMkdirError(directory, err), + }; + } else { + return { + type: 'error', + errorMessage: + `Unable to inspect workspace path '${directory}'. ` + + `System error: ${err?.message || err}. ` + + `Please verify the path is valid and you have the necessary permissions.`, + }; + } + } + + if (lstat) { + if (lstat.isSymbolicLink()) { + return await handleSymlink(directory); + } + if (lstat.isDirectory()) { + return { type: 'ok', created: false }; + } + return { + type: 'error', + errorMessage: + `A non-directory file already exists at '${directory}'. ` + + `Cannot use it as a workspace. Please move or remove the file, or pick a different workspace path.`, + }; + } + + if (!approvedNewDirectoryCreation) { + return { type: 'requestApproval' }; + } + + try { + await fs.mkdir(directory, { recursive: true }); + return { type: 'ok', created: true }; + } catch (err: any) { + return await buildMkdirError(directory, err); + } +} + +async function handleSymlink(directory: string): Promise { + let linkTarget = ''; + try { + linkTarget = await fs.readlink(directory); + } catch { + // Best-effort: if we can't read the link, we still report a useful error below. + } + + let realPath: string; + try { + realPath = await fs.realpath(directory); + } catch (err: any) { + if (err?.code === 'ENOENT') { + const targetDescription = linkTarget + ? `'${linkTarget}'` + : 'a target that no longer exists'; + // Deliberately do NOT embed `directory` inside a copy-pasteable + // shell command (e.g. `rm '...'`): a path containing a single + // quote would break the quoting and turn this diagnostic into + // a shell-injection / accidental-delete vector. Describe the + // recovery action in prose instead. (Codex review on PR #892.) + return { + type: 'error', + errorMessage: + `Workspace path '${directory}' is a symbolic link to ${targetDescription}, ` + + `which no longer exists. This usually means the target was deleted ` + + `(e.g. via \`git worktree remove\`) without removing the symlink. ` + + `Recovery: recreate the directory at the target path, remove the dangling symlink at '${directory}', ` + + `or archive this session.`, + }; + } + return { + type: 'error', + errorMessage: + `Unable to resolve symbolic link at '${directory}'. ` + + `System error: ${err?.message || err}. ` + + `Please verify the symlink target is reachable and you have the necessary permissions.`, + }; + } + + let resolvedStat; + try { + resolvedStat = await fs.stat(realPath); + } catch (err: any) { + return { + type: 'error', + errorMessage: + `Unable to stat resolved path '${realPath}' (symlinked from '${directory}'). ` + + `System error: ${err?.message || err}.`, + }; + } + + if (resolvedStat.isDirectory()) { + return { type: 'ok', created: false }; + } + + return { + type: 'error', + errorMessage: + `Workspace path '${directory}' is a symbolic link to '${realPath}', which is not a directory. ` + + `Please update the symlink to point at a directory, or pick a different workspace path.`, + }; +} + +/** + * Pure mapping of `mkdir` errno codes to user-facing messages. Exported for + * unit tests; production callers go through `buildMkdirError` which adds + * `EEXIST` race-handling on top. + */ +export function describeMkdirError( + directory: string, + err: { code?: string; message?: string } | undefined | null +): string { + const prefix = `Unable to create directory at '${directory}'. `; + switch (err?.code) { + case 'EACCES': + return ( + prefix + + `Permission denied. You don't have write access to create a folder at this location. ` + + `Try using a different path or check your permissions.` + ); + case 'ENOTDIR': + return ( + prefix + + `A file already exists at this path or in the parent path. ` + + `Cannot create a directory here. Please choose a different location.` + ); + case 'ENOSPC': + return ( + prefix + + `No space left on device. Your disk is full. Please free up some space and try again.` + ); + case 'EROFS': + return ( + prefix + + `The file system is read-only. Cannot create directories here. Please choose a writable location.` + ); + case 'EEXIST': + return ( + prefix + + `A non-directory file appeared at this path between the existence check ` + + `and directory creation. Please move or remove it, or pick a different path.` + ); + default: + return ( + prefix + + `System error: ${err?.message || err}. ` + + `Please verify the path is valid and you have the necessary permissions.` + ); + } +} + +async function buildMkdirError( + directory: string, + err: any +): Promise { + if (err?.code === 'EEXIST') { + // Race with a parallel writer between the initial lstat and mkdir, OR + // a non-directory entry that mkdir({ recursive: true }) refused to + // tolerate. lstat the path again to produce a targeted message + // instead of leaking the kernel error verbatim. + try { + const raceStat = await fs.lstat(directory); + if (raceStat.isDirectory()) { + // mkdir({ recursive: true }) should not throw EEXIST on an + // existing directory; if it did, treat the directory as good + // enough rather than failing the user. + return { type: 'ok', created: false }; + } + if (raceStat.isSymbolicLink()) { + return handleSymlink(directory); + } + } catch { + // Fall through to the message-only path below if we can't even + // lstat the path again (very unusual race). + } + } + return { + type: 'error', + errorMessage: describeMkdirError(directory, err), + }; +}