From 3dfbd61c7a9d8239e5b74f452422f36627952974 Mon Sep 17 00:00:00 2001 From: HeavyGee <133152184+heavygee@users.noreply.github.com> Date: Thu, 18 Jun 2026 03:12:02 +0100 Subject: [PATCH] fix(runner): surface dangling-symlink errors instead of misleading EEXIST (#892) * fix(runner): surface dangling-symlink errors instead of misleading EEXIST When a session's workspace path is a symbolic link to a directory that no longer exists, the runner previously failed with a kernel-level error: Unable to create directory at '/path'. System error: EEXIST: file already exists, mkdir '/path'. The cause: `fs.access` follows symlinks and throws ENOENT when the target is missing, then the fallback `fs.mkdir(dir, { recursive: true })` cannot tolerate the existing non-directory entry (the dangling symlink itself) and surfaces EEXIST verbatim. Users had no way to tell that the symlink target was the actual problem. Replace the inline `fs.access` + `fs.mkdir` pair with a small `validateWorkspaceDirectory` helper that uses `fs.lstat` so symlinks are inspected without being followed, then explicitly handles: - missing path -> approval flow / mkdir as before - existing directory -> ok - regular file at the workspace path -> "non-directory file" error - symlink to existing directory -> ok - symlink to a non-directory -> "not a directory" error - dangling symlink -> diagnostic naming both the symlink path and the missing target, with recovery options (recreate the target, remove the symlink, archive the session) The mkdir error switch is preserved (EACCES, ENOTDIR, ENOSPC, EROFS) and extended with an EEXIST race-recheck that lstat's the path again so the kernel error code never leaks to the user. Adds focused unit tests for both the fs-touching paths (real tmpdir + symlinks) and the pure errno-to-message mapper. Closes #890 Co-authored-by: Cursor * fix(runner): drop copy-pasteable rm command from dangling-symlink hint The dangling-symlink recovery message embedded the user-controlled workspace path inside a literal `rm '...'` shell command shape. A path containing a single quote would break the quoting and turn the diagnostic into a shell-injection / accidental-delete vector when the user copy-pasted the suggested command. Describe the recovery action in prose instead ("remove the dangling symlink at ''") so the path is no longer presented as a copy-pasteable command. Adds a regression test that exercises a path containing a single quote and asserts the message never contains the literal `rm ...` shape. Codex review on PR #892. Co-authored-by: Cursor * fix(runner): preserve ENOTDIR diagnostic on lstat parent-path failure When the workspace path sits under a regular-file parent, fs.lstat throws ENOTDIR before mkdir runs. Route that through describeMkdirError so the user still sees the historic "file already exists at this path" message instead of the generic inspect-workspace-path text. Codex follow-up review on PR #892 (post-rebase). Co-authored-by: Cursor --------- Co-authored-by: Cursor --- cli/src/runner/run.ts | 62 ++--- .../runner/validateWorkspaceDirectory.test.ts | 209 +++++++++++++++ cli/src/runner/validateWorkspaceDirectory.ts | 237 ++++++++++++++++++ 3 files changed, 468 insertions(+), 40 deletions(-) create mode 100644 cli/src/runner/validateWorkspaceDirectory.test.ts create mode 100644 cli/src/runner/validateWorkspaceDirectory.ts 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), + }; +}