diff --git a/packages/agent/docs/agent-harness.md b/packages/agent/docs/agent-harness.md index 5baccdff..37b14cb7 100644 --- a/packages/agent/docs/agent-harness.md +++ b/packages/agent/docs/agent-harness.md @@ -18,6 +18,12 @@ The intended rule is: A final lifecycle hardening pass should prove these guarantees with a broad listener/hook reentrancy test suite. +## Error handling + +The target error-handling model is `Result` for fallible harness operations instead of thrown exceptions. Implementations should catch backend/provider/filesystem exceptions at the boundary and normalize them into typed error results; callers should inspect returned results instead of relying on thrown exceptions. + +This is currently implemented for `ExecutionEnv`, `NodeExecutionEnv`, shell-output capture, and skill/prompt-template resource loading. Older harness/session/compaction APIs still throw in several paths; finishing that migration is tracked in the cleanup todo. + ## State model The harness separates state into four categories. @@ -90,7 +96,7 @@ Structural operations require `phase === "idle"` and synchronously set the phase - `compact` - `navigateTree` -Starting another structural operation while the harness is not idle throws. +Starting another structural operation while the harness is not idle currently throws. This should become a typed result failure as part of the error-handling cleanup. The following operations are allowed during a turn where appropriate: @@ -141,7 +147,7 @@ The low-level loop converts harness `ThinkingLevel` to provider `reasoning` at t No state refresh is needed on `agent_end` except flushing leftover pending session writes and clearing the operation phase. The exact `settled` event timing is still under review. -If the system-prompt callback throws while starting `prompt`, `skill`, or `promptFromTemplate`, the operation throws and the harness returns to idle. If it throws from the save-point snapshot created by `prepareNextTurn`, the low-level agent run records an assistant error message. +If the system-prompt callback throws while starting `prompt`, `skill`, or `promptFromTemplate`, the operation currently throws and the harness returns to idle. This should become a typed result failure as part of the error-handling cleanup. If it throws from the save-point snapshot created by `prepareNextTurn`, the low-level agent run records an assistant error message. ## Hooks and events @@ -307,7 +313,7 @@ Implemented so far: - `setTools(tools, activeToolNames?)` - `setActiveTools(toolNames)` -- invalid active tool names throw +- invalid active tool names currently throw; convert to result errors - generic common app tool shape via `AgentHarness` - `QueueMode` exported from `Agent` - `AgentHarnessOptions.steeringMode` / `followUpMode` @@ -355,7 +361,33 @@ Still needed: - Document or change timing for model/thinking/stream-option events that may fire before queued session entries persist while busy. - Audit `abort()` barrier semantics. -### 7. Later coding-agent migration plan +### 7. Complete `Result`/non-throwing harness cleanup + +Started. + +Implemented so far: + +- Added generic `Result` plus helpers. +- Updated `ExecutionEnv` and `NodeExecutionEnv` to return typed results for filesystem/process operations. +- Added `ExecutionEnv.appendFile()` for streaming append use cases. +- Updated skill and prompt-template loaders to consume `ExecutionEnv` results. +- Updated shell output capture to return a result and use `ExecutionEnv` instead of Node APIs directly, including full-output spill via `appendFile()`. +- Removed `NodeExecutionEnv` from the browser-safe `execution-env.ts` re-export; Node-specific callers import from `harness/env/nodejs.js`. +- Expanded `NodeExecutionEnv` tests for file operations, exec errors, aborts, callbacks, timeouts, and shell-output full-output spill. + +Still needed: + +- Remove remaining throws from `src/harness` APIs and helpers, except explicit adapter/test helpers such as `getOrThrow()`. +- Convert session storage/repo/session APIs to typed result returns. +- Convert structural `AgentHarness` operations to typed result returns for busy, missing-resource, auth, compaction, and branch-summary failures. +- Convert compaction helpers to typed result returns. +- Keep Node-specific APIs isolated under `src/harness/env/nodejs.ts` and Node-backed storage/session implementations, or move those implementations behind explicit Node-only entry points. +- Replace Node globals in generic harness utilities, especially `Buffer` usage in truncation utilities, with runtime-neutral implementations. +- Audit package exports so browser/generic-JS imports do not pull Node-only modules such as `NodeExecutionEnv` or JSONL storage. +- Keep expanding `ExecutionEnv` and shell-output contract tests as the API evolves, especially for non-Node implementations. +- Add tests proving harness APIs return `ok: false` instead of throwing for expected failure paths. + +### 8. Later coding-agent migration plan Not started. @@ -367,7 +399,7 @@ Still needed: - Preserve UI/session behavior outside core. - Move coding-agent stream/auth/retry/header behavior onto the harness stream configuration and provider hooks. -### 8. Final lifecycle hardening suite +### 9. Final lifecycle hardening suite Before treating `AgentHarness` as migration-ready, add a broad test suite that exercises listeners and hooks closing over the harness and calling public APIs during every relevant event. diff --git a/packages/agent/src/harness/env/nodejs.ts b/packages/agent/src/harness/env/nodejs.ts index d4b6c0a9..6e79cf2c 100644 --- a/packages/agent/src/harness/env/nodejs.ts +++ b/packages/agent/src/harness/env/nodejs.ts @@ -1,33 +1,59 @@ import { spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; import { constants } from "node:fs"; -import { access, lstat, mkdir, mkdtemp, readdir, readFile, realpath, rm, writeFile } from "node:fs/promises"; +import { + access, + appendFile, + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rm, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; import { isAbsolute, join, resolve } from "node:path"; -import { type ExecutionEnv, FileError, type FileInfo, type FileKind } from "../types.js"; +import { + type ExecutionEnv, + ExecutionError, + err, + FileError, + type FileInfo, + type FileKind, + ok, + type Result, +} from "../types.js"; function resolvePath(cwd: string, path: string): string { return isAbsolute(path) ? path : resolve(cwd, path); } -function fileKindFromStats(stats: { isFile(): boolean; isDirectory(): boolean; isSymbolicLink(): boolean }): FileKind { +function fileKindFromStats(stats: { + isFile(): boolean; + isDirectory(): boolean; + isSymbolicLink(): boolean; +}): FileKind | undefined { if (stats.isFile()) return "file"; if (stats.isDirectory()) return "directory"; if (stats.isSymbolicLink()) return "symlink"; - throw new FileError("invalid", "Unsupported file type"); + return undefined; } function fileInfoFromStats( path: string, stats: { isFile(): boolean; isDirectory(): boolean; isSymbolicLink(): boolean; size: number; mtimeMs: number }, -): FileInfo { - return { +): Result { + const kind = fileKindFromStats(stats); + if (!kind) return err(new FileError("invalid", "Unsupported file type", path)); + return ok({ name: path.replace(/\/+$/, "").split("/").pop() ?? path, path, - kind: fileKindFromStats(stats), + kind, size: stats.size, mtimeMs: stats.mtimeMs, - }; + }); } function isNodeError(error: unknown): error is NodeJS.ErrnoException { @@ -39,20 +65,26 @@ function toFileError(error: unknown, path?: string): FileError { if (isNodeError(error)) { const message = error.message; switch (error.code) { + case "ABORT_ERR": + return new FileError("aborted", message, path, error); case "ENOENT": - return new FileError("not_found", message, path, { cause: error }); + return new FileError("not_found", message, path, error); case "EACCES": case "EPERM": - return new FileError("permission_denied", message, path, { cause: error }); + return new FileError("permission_denied", message, path, error); case "ENOTDIR": - return new FileError("not_directory", message, path, { cause: error }); + return new FileError("not_directory", message, path, error); case "EISDIR": - return new FileError("is_directory", message, path, { cause: error }); + return new FileError("is_directory", message, path, error); case "EINVAL": - return new FileError("invalid", message, path, { cause: error }); + return new FileError("invalid", message, path, error); } } - return new FileError("unknown", error instanceof Error ? error.message : String(error), path, { cause: error }); + return new FileError("unknown", error instanceof Error ? error.message : String(error), path, error); +} + +function abortResult(signal: AbortSignal | undefined, path?: string): Result | undefined { + return signal?.aborted ? err(new FileError("aborted", "aborted", path)) : undefined; } async function pathExists(path: string): Promise { @@ -71,7 +103,13 @@ async function runCommand( ): Promise<{ stdout: string; status: number | null }> { return await new Promise((resolve) => { let stdout = ""; - const child = spawn(command, args, { stdio: ["ignore", "pipe", "ignore"] }); + let child: ReturnType; + try { + child = spawn(command, args, { stdio: ["ignore", "pipe", "ignore"] }); + } catch { + resolve({ stdout: "", status: null }); + return; + } const timeout = setTimeout(() => { if (child.pid) killProcessTree(child.pid); }, timeoutMs); @@ -100,12 +138,14 @@ async function findBashOnPath(): Promise { return firstMatch && (await pathExists(firstMatch)) ? firstMatch : null; } -async function getShellConfig(customShellPath?: string): Promise<{ shell: string; args: string[] }> { +async function getShellConfig( + customShellPath?: string, +): Promise> { if (customShellPath) { if (await pathExists(customShellPath)) { - return { shell: customShellPath, args: ["-c"] }; + return ok({ shell: customShellPath, args: ["-c"] }); } - throw new Error(`Custom shell path not found: ${customShellPath}`); + return err(new ExecutionError("shell_unavailable", `Custom shell path not found: ${customShellPath}`)); } if (process.platform === "win32") { const candidates: string[] = []; @@ -115,24 +155,24 @@ async function getShellConfig(customShellPath?: string): Promise<{ shell: string if (programFilesX86) candidates.push(`${programFilesX86}\\Git\\bin\\bash.exe`); for (const candidate of candidates) { if (await pathExists(candidate)) { - return { shell: candidate, args: ["-c"] }; + return ok({ shell: candidate, args: ["-c"] }); } } const bashOnPath = await findBashOnPath(); if (bashOnPath) { - return { shell: bashOnPath, args: ["-c"] }; + return ok({ shell: bashOnPath, args: ["-c"] }); } - throw new Error("No bash shell found"); + return err(new ExecutionError("shell_unavailable", "No bash shell found")); } if (await pathExists("/bin/bash")) { - return { shell: "/bin/bash", args: ["-c"] }; + return ok({ shell: "/bin/bash", args: ["-c"] }); } const bashOnPath = await findBashOnPath(); if (bashOnPath) { - return { shell: bashOnPath, args: ["-c"] }; + return ok({ shell: bashOnPath, args: ["-c"] }); } - return { shell: "sh", args: ["-c"] }; + return ok({ shell: "sh", args: ["-c"] }); } function getShellEnv(baseEnv?: NodeJS.ProcessEnv, extraEnv?: Record): NodeJS.ProcessEnv { @@ -184,46 +224,69 @@ export class NodeExecutionEnv implements ExecutionEnv { cwd?: string; env?: Record; timeout?: number; - signal?: AbortSignal; + abortSignal?: AbortSignal; onStdout?: (chunk: string) => void; onStderr?: (chunk: string) => void; }, - ): Promise<{ stdout: string; stderr: string; exitCode: number }> { - const cwd = options?.cwd ? resolvePath(this.cwd, options.cwd) : this.cwd; - const { shell, args } = await getShellConfig(this.shellPath); + ): Promise> { + if (options?.abortSignal?.aborted) return err(new ExecutionError("aborted", "aborted")); - return await new Promise((resolvePromise, reject) => { + const cwd = options?.cwd ? resolvePath(this.cwd, options.cwd) : this.cwd; + const shellConfig = await getShellConfig(this.shellPath); + if (!shellConfig.ok) return shellConfig; + + return await new Promise((resolvePromise) => { let stdout = ""; let stderr = ""; let settled = false; let timedOut = false; - const child = spawn(shell, [...args, command], { - cwd, - detached: process.platform !== "win32", - env: getShellEnv(this.shellEnv, options?.env), - stdio: ["ignore", "pipe", "pipe"], - }); + let callbackError: ExecutionError | undefined; + let child: ReturnType | undefined; + let timeoutId: ReturnType | undefined; - const timeoutId = + const onAbort = () => { + if (child?.pid) { + killProcessTree(child.pid); + } + }; + + const settle = (result: Result<{ stdout: string; stderr: string; exitCode: number }, ExecutionError>) => { + if (timeoutId) clearTimeout(timeoutId); + if (options?.abortSignal) options.abortSignal.removeEventListener("abort", onAbort); + if (settled) return; + settled = true; + resolvePromise(result); + }; + + try { + child = spawn(shellConfig.value.shell, [...shellConfig.value.args, command], { + cwd, + detached: process.platform !== "win32", + env: getShellEnv(this.shellEnv, options?.env), + stdio: ["ignore", "pipe", "pipe"], + }); + } catch (error) { + settle( + err(new ExecutionError("spawn_error", error instanceof Error ? error.message : String(error), error)), + ); + return; + } + + timeoutId = typeof options?.timeout === "number" ? setTimeout(() => { timedOut = true; - if (child.pid) { + if (child?.pid) { killProcessTree(child.pid); } }, options.timeout * 1000) : undefined; - const onAbort = () => { - if (child.pid) { - killProcessTree(child.pid); - } - }; - if (options?.signal) { - if (options.signal.aborted) { + if (options?.abortSignal) { + if (options.abortSignal.aborted) { onAbort(); } else { - options.signal.addEventListener("abort", onAbort, { once: true }); + options.abortSignal.addEventListener("abort", onAbort, { once: true }); } } @@ -231,137 +294,231 @@ export class NodeExecutionEnv implements ExecutionEnv { child.stderr?.setEncoding("utf8"); child.stdout?.on("data", (chunk: string) => { stdout += chunk; - options?.onStdout?.(chunk); + try { + options?.onStdout?.(chunk); + } catch (error) { + callbackError = new ExecutionError( + "unknown", + error instanceof Error ? error.message : String(error), + error, + ); + onAbort(); + } }); child.stderr?.on("data", (chunk: string) => { stderr += chunk; - options?.onStderr?.(chunk); + try { + options?.onStderr?.(chunk); + } catch (error) { + callbackError = new ExecutionError( + "unknown", + error instanceof Error ? error.message : String(error), + error, + ); + onAbort(); + } }); child.on("error", (error) => { - if (timeoutId) clearTimeout(timeoutId); - if (options?.signal) options.signal.removeEventListener("abort", onAbort); - if (settled) return; - settled = true; - reject(error); + settle(err(new ExecutionError("spawn_error", error.message, error))); }); child.on("close", (code) => { - if (timeoutId) clearTimeout(timeoutId); - if (options?.signal) options.signal.removeEventListener("abort", onAbort); - if (settled) return; - settled = true; - if (options?.signal?.aborted) { - reject(new Error("aborted")); + if (callbackError) { + settle(err(callbackError)); + return; + } + if (options?.abortSignal?.aborted) { + settle(err(new ExecutionError("aborted", "aborted"))); return; } if (timedOut) { - reject(new Error(`timeout:${options?.timeout}`)); + settle(err(new ExecutionError("timeout", `timeout:${options?.timeout}`))); return; } - resolvePromise({ stdout, stderr, exitCode: code ?? 0 }); + settle(ok({ stdout, stderr, exitCode: code ?? 0 })); }); }); } - async readTextFile(path: string): Promise { + async readTextFile(path: string, abortSignal?: AbortSignal): Promise> { const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(abortSignal, resolved); + if (aborted) return aborted; try { - return await readFile(resolved, "utf8"); + return ok(await readFile(resolved, { encoding: "utf8", signal: abortSignal })); } catch (error) { - throw toFileError(error, resolved); + return err(toFileError(error, resolved)); } } - async readBinaryFile(path: string): Promise { + async readBinaryFile(path: string, abortSignal?: AbortSignal): Promise> { const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(abortSignal, resolved); + if (aborted) return aborted; try { - return await readFile(resolved); + return ok(await readFile(resolved, { signal: abortSignal })); } catch (error) { - throw toFileError(error, resolved); + return err(toFileError(error, resolved)); } } - async writeFile(path: string, content: string | Uint8Array): Promise { + async writeFile( + path: string, + content: string | Uint8Array, + abortSignal?: AbortSignal, + ): Promise> { const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(abortSignal, resolved); + if (aborted) return aborted; try { await mkdir(resolve(resolved, ".."), { recursive: true }); - await writeFile(resolved, content); + const afterMkdirAbort = abortResult(abortSignal, resolved); + if (afterMkdirAbort) return afterMkdirAbort; + await writeFile(resolved, content, { signal: abortSignal }); + return ok(undefined); } catch (error) { - throw toFileError(error, resolved); + return err(toFileError(error, resolved)); } } - async fileInfo(path: string): Promise { + async appendFile( + path: string, + content: string | Uint8Array, + abortSignal?: AbortSignal, + ): Promise> { const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(abortSignal, resolved); + if (aborted) return aborted; + try { + await mkdir(resolve(resolved, ".."), { recursive: true }); + const afterMkdirAbort = abortResult(abortSignal, resolved); + if (afterMkdirAbort) return afterMkdirAbort; + await appendFile(resolved, content); + const afterAppendAbort = abortResult(abortSignal, resolved); + if (afterAppendAbort) return afterAppendAbort; + return ok(undefined); + } catch (error) { + return err(toFileError(error, resolved)); + } + } + + async fileInfo(path: string, abortSignal?: AbortSignal): Promise> { + const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(abortSignal, resolved); + if (aborted) return aborted; try { return fileInfoFromStats(resolved, await lstat(resolved)); } catch (error) { - throw toFileError(error, resolved); + return err(toFileError(error, resolved)); } } - async listDir(path: string): Promise { + async listDir(path: string, abortSignal?: AbortSignal): Promise> { const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(abortSignal, resolved); + if (aborted) return aborted; try { const entries = await readdir(resolved, { withFileTypes: true }); const infos: FileInfo[] = []; for (const entry of entries) { + const loopAbort = abortResult(abortSignal, resolved); + if (loopAbort) return loopAbort; const entryPath = resolve(resolved, entry.name); try { - infos.push(fileInfoFromStats(entryPath, await lstat(entryPath))); + const info = fileInfoFromStats(entryPath, await lstat(entryPath)); + if (info.ok) infos.push(info.value); } catch (error) { - if (error instanceof FileError && error.code === "invalid") continue; - throw error; + return err(toFileError(error, entryPath)); } } - return infos; + return ok(infos); } catch (error) { - throw toFileError(error, resolved); + return err(toFileError(error, resolved)); } } - async realPath(path: string): Promise { + async realPath(path: string, abortSignal?: AbortSignal): Promise> { const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(abortSignal, resolved); + if (aborted) return aborted; try { - return await realpath(resolved); + return ok(await realpath(resolved)); } catch (error) { - throw toFileError(error, resolved); + return err(toFileError(error, resolved)); } } - async exists(path: string): Promise { - try { - await this.fileInfo(path); - return true; - } catch (error) { - if (error instanceof FileError && error.code === "not_found") return false; - throw error; - } + async exists(path: string, abortSignal?: AbortSignal): Promise> { + const result = await this.fileInfo(path, abortSignal); + if (result.ok) return ok(true); + if (result.error.code === "not_found") return ok(false); + return err(result.error); } - async createDir(path: string, options?: { recursive?: boolean }): Promise { - await mkdir(resolvePath(this.cwd, path), { recursive: options?.recursive }); - } - - async remove(path: string, options?: { recursive?: boolean; force?: boolean }): Promise { + async createDir( + path: string, + options?: { recursive?: boolean; abortSignal?: AbortSignal }, + ): Promise> { const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(options?.abortSignal, resolved); + if (aborted) return aborted; + try { + await mkdir(resolved, { recursive: options?.recursive ?? true }); + const afterMkdirAbort = abortResult(options?.abortSignal, resolved); + if (afterMkdirAbort) return afterMkdirAbort; + return ok(undefined); + } catch (error) { + return err(toFileError(error, resolved)); + } + } + + async remove( + path: string, + options?: { recursive?: boolean; force?: boolean; abortSignal?: AbortSignal }, + ): Promise> { + const resolved = resolvePath(this.cwd, path); + const aborted = abortResult(options?.abortSignal, resolved); + if (aborted) return aborted; try { await rm(resolved, { recursive: options?.recursive ?? false, force: options?.force ?? false }); + const afterRemoveAbort = abortResult(options?.abortSignal, resolved); + if (afterRemoveAbort) return afterRemoveAbort; + return ok(undefined); } catch (error) { - throw toFileError(error, resolved); + return err(toFileError(error, resolved)); } } - async createTempDir(prefix: string = "tmp-"): Promise { - return await mkdtemp(join(tmpdir(), prefix)); + async createTempDir(prefix: string = "tmp-", abortSignal?: AbortSignal): Promise> { + const aborted = abortResult(abortSignal); + if (aborted) return aborted; + try { + const path = await mkdtemp(join(tmpdir(), prefix)); + const afterMkdtempAbort = abortResult(abortSignal, path); + if (afterMkdtempAbort) return afterMkdtempAbort; + return ok(path); + } catch (error) { + return err(toFileError(error)); + } } - async createTempFile(options?: { prefix?: string; suffix?: string }): Promise { - const dir = await this.createTempDir("tmp-"); - const filePath = join(dir, `${options?.prefix ?? ""}${randomUUID()}${options?.suffix ?? ""}`); - await writeFile(filePath, ""); - return filePath; + async createTempFile(options?: { + prefix?: string; + suffix?: string; + abortSignal?: AbortSignal; + }): Promise> { + const dir = await this.createTempDir("tmp-", options?.abortSignal); + if (!dir.ok) return dir; + const filePath = join(dir.value, `${options?.prefix ?? ""}${randomUUID()}${options?.suffix ?? ""}`); + const aborted = abortResult(options?.abortSignal, filePath); + if (aborted) return aborted; + try { + await writeFile(filePath, "", { signal: options?.abortSignal }); + return ok(filePath); + } catch (error) { + return err(toFileError(error, filePath)); + } } async cleanup(): Promise { diff --git a/packages/agent/src/harness/execution-env.ts b/packages/agent/src/harness/execution-env.ts index 786586e5..915fa4da 100644 --- a/packages/agent/src/harness/execution-env.ts +++ b/packages/agent/src/harness/execution-env.ts @@ -1,3 +1,10 @@ -export { NodeExecutionEnv } from "./env/nodejs.js"; -export type { ExecutionEnv, ExecutionEnvExecOptions, FileErrorCode, FileInfo, FileKind } from "./types.js"; -export { FileError } from "./types.js"; +export type { + ExecutionEnv, + ExecutionEnvExecOptions, + ExecutionErrorCode, + FileErrorCode, + FileInfo, + FileKind, + Result, +} from "./types.js"; +export { ExecutionError, err, FileError, getOrThrow, getOrUndefined, ok } from "./types.js"; diff --git a/packages/agent/src/harness/prompt-templates.ts b/packages/agent/src/harness/prompt-templates.ts index e77682b8..e627b66c 100644 --- a/packages/agent/src/harness/prompt-templates.ts +++ b/packages/agent/src/harness/prompt-templates.ts @@ -1,5 +1,5 @@ import { parse } from "yaml"; -import type { ExecutionEnv, FileInfo, PromptTemplate } from "./types.js"; +import { type ExecutionEnv, type FileInfo, getOrUndefined, type PromptTemplate, type Result } from "./types.js"; /** Warning produced while loading prompt templates. */ export interface PromptTemplateDiagnostic { @@ -30,7 +30,7 @@ export async function loadPromptTemplates( const promptTemplates: PromptTemplate[] = []; const diagnostics: PromptTemplateDiagnostic[] = []; for (const path of Array.isArray(paths) ? paths : [paths]) { - const info = await safeFileInfo(env, path); + const info = getOrUndefined(await env.fileInfo(path)); if (!info) continue; const kind = await resolveKind(env, info); if (kind === "directory") { @@ -83,17 +83,16 @@ async function loadTemplatesFromDir( ): Promise<{ promptTemplates: PromptTemplate[]; diagnostics: PromptTemplateDiagnostic[] }> { const promptTemplates: PromptTemplate[] = []; const diagnostics: PromptTemplateDiagnostic[] = []; - let entries: FileInfo[]; - try { - entries = await env.listDir(dir); - } catch (error) { + const entriesResult = await env.listDir(dir); + if (!entriesResult.ok) { diagnostics.push({ type: "warning", - message: errorMessage(error, "failed to list prompt template directory"), + message: entriesResult.error.message, path: dir, }); return { promptTemplates, diagnostics }; } + const entries = entriesResult.value; for (const entry of entries.sort((a, b) => a.name.localeCompare(b.name))) { const kind = await resolveKind(env, entry); @@ -110,60 +109,66 @@ async function loadTemplateFromFile( filePath: string, ): Promise<{ promptTemplate: PromptTemplate | null; diagnostics: PromptTemplateDiagnostic[] }> { const diagnostics: PromptTemplateDiagnostic[] = []; - try { - const rawContent = await env.readTextFile(filePath); - const { frontmatter, body } = parseFrontmatter(rawContent); - const firstLine = body.split("\n").find((line) => line.trim()); - let description = typeof frontmatter.description === "string" ? frontmatter.description : ""; - if (!description && firstLine) { - description = firstLine.slice(0, 60); - if (firstLine.length > 60) description += "..."; - } - return { - promptTemplate: { - name: basenameEnvPath(filePath).replace(/\.md$/i, ""), - description, - content: body, - }, - diagnostics, - }; - } catch (error) { + const rawContent = await env.readTextFile(filePath); + if (!rawContent.ok) { diagnostics.push({ type: "warning", - message: errorMessage(error, "failed to load prompt template"), + message: rawContent.error.message, path: filePath, }); return { promptTemplate: null, diagnostics }; } -} -async function safeFileInfo(env: ExecutionEnv, path: string): Promise { - try { - return await env.fileInfo(path); - } catch { - return undefined; + const parsed = parseFrontmatter(rawContent.value); + if (!parsed.ok) { + diagnostics.push({ + type: "warning", + message: parsed.error.message, + path: filePath, + }); + return { promptTemplate: null, diagnostics }; } + + const { frontmatter, body } = parsed.value; + const firstLine = body.split("\n").find((line) => line.trim()); + let description = typeof frontmatter.description === "string" ? frontmatter.description : ""; + if (!description && firstLine) { + description = firstLine.slice(0, 60); + if (firstLine.length > 60) description += "..."; + } + return { + promptTemplate: { + name: basenameEnvPath(filePath).replace(/\.md$/i, ""), + description, + content: body, + }, + diagnostics, + }; } async function resolveKind(env: ExecutionEnv, info: FileInfo): Promise<"file" | "directory" | undefined> { if (info.kind === "file" || info.kind === "directory") return info.kind; - try { - const realPath = await env.realPath(info.path); - const target = await env.fileInfo(realPath); - return target.kind === "file" || target.kind === "directory" ? target.kind : undefined; - } catch { - return undefined; - } + const realPath = await env.realPath(info.path); + if (!realPath.ok) return undefined; + const target = getOrUndefined(await env.fileInfo(realPath.value)); + if (!target) return undefined; + return target.kind === "file" || target.kind === "directory" ? target.kind : undefined; } -function parseFrontmatter>(content: string): { frontmatter: T; body: string } { - const normalized = content.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); - if (!normalized.startsWith("---")) return { frontmatter: {} as T, body: normalized }; - const endIndex = normalized.indexOf("\n---", 3); - if (endIndex === -1) return { frontmatter: {} as T, body: normalized }; - const yamlString = normalized.slice(4, endIndex); - const body = normalized.slice(endIndex + 4).trim(); - return { frontmatter: (parse(yamlString) ?? {}) as T, body }; +function parseFrontmatter>( + content: string, +): Result<{ frontmatter: T; body: string }, Error> { + try { + const normalized = content.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); + if (!normalized.startsWith("---")) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; + const endIndex = normalized.indexOf("\n---", 3); + if (endIndex === -1) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; + const yamlString = normalized.slice(4, endIndex); + const body = normalized.slice(endIndex + 4).trim(); + return { ok: true, value: { frontmatter: (parse(yamlString) ?? {}) as T, body } }; + } catch (error) { + return { ok: false, error: error instanceof Error ? error : new Error(String(error)) }; + } } function basenameEnvPath(path: string): string { @@ -172,10 +177,6 @@ function basenameEnvPath(path: string): string { return slashIndex === -1 ? normalized : normalized.slice(slashIndex + 1); } -function errorMessage(error: unknown, fallback: string): string { - return error instanceof Error ? error.message : fallback; -} - /** Parse an argument string using simple shell-style single and double quotes. */ export function parseCommandArgs(argsString: string): string[] { const args: string[] = []; diff --git a/packages/agent/src/harness/skills.ts b/packages/agent/src/harness/skills.ts index db03d93c..bb8931f6 100644 --- a/packages/agent/src/harness/skills.ts +++ b/packages/agent/src/harness/skills.ts @@ -1,6 +1,6 @@ import ignore from "ignore"; import { parse } from "yaml"; -import type { ExecutionEnv, Skill } from "./types.js"; +import { type ExecutionEnv, type FileInfo, getOrUndefined, type Result, type Skill } from "./types.js"; const MAX_NAME_LENGTH = 64; const MAX_DESCRIPTION_LENGTH = 1024; @@ -44,7 +44,7 @@ export async function loadSkills( const skills: Skill[] = []; const diagnostics: SkillDiagnostic[] = []; for (const dir of Array.isArray(dirs) ? dirs : [dirs]) { - const rootInfo = await safeFileInfo(env, dir); + const rootInfo = getOrUndefined(await env.fileInfo(dir)); if (!rootInfo || (await resolveKind(env, rootInfo)) !== "directory") continue; const result = await loadSkillsFromDirInternal(env, rootInfo.path, true, ignore(), rootInfo.path); skills.push(...result.skills); @@ -89,18 +89,13 @@ async function loadSkillsFromDirInternal( const skills: Skill[] = []; const diagnostics: SkillDiagnostic[] = []; - if (!(await env.exists(dir))) return { skills, diagnostics }; - const dirInfo = await safeFileInfo(env, dir); + const dirInfo = getOrUndefined(await env.fileInfo(dir)); if (!dirInfo || (await resolveKind(env, dirInfo)) !== "directory") return { skills, diagnostics }; await addIgnoreRules(env, ignoreMatcher, dir, rootDir); - let entries: Awaited>; - try { - entries = await env.listDir(dir); - } catch { - return { skills, diagnostics }; - } + const entries = getOrUndefined(await env.listDir(dir)); + if (!entries) return { skills, diagnostics }; for (const entry of entries) { if (entry.name !== "SKILL.md") continue; @@ -148,16 +143,15 @@ async function addIgnoreRules(env: ExecutionEnv, ig: IgnoreMatcher, dir: string, for (const filename of IGNORE_FILE_NAMES) { const ignorePath = joinEnvPath(dir, filename); - const info = await safeFileInfo(env, ignorePath); + const info = getOrUndefined(await env.fileInfo(ignorePath)); if (info?.kind !== "file") continue; - try { - const content = await env.readTextFile(ignorePath); - const patterns = content - .split(/\r?\n/) - .map((line) => prefixIgnorePattern(line, prefix)) - .filter((line): line is string => Boolean(line)); - if (patterns.length > 0) ig.add(patterns); - } catch {} + const content = await env.readTextFile(ignorePath); + if (!content.ok) continue; + const patterns = content.value + .split(/\r?\n/) + .map((line) => prefixIgnorePattern(line, prefix)) + .filter((line): line is string => Boolean(line)); + if (patterns.length > 0) ig.add(patterns); } } @@ -184,40 +178,47 @@ async function loadSkillFromFile( filePath: string, ): Promise<{ skill: Skill | null; diagnostics: SkillDiagnostic[] }> { const diagnostics: SkillDiagnostic[] = []; - try { - const rawContent = await env.readTextFile(filePath); - const { frontmatter, body } = parseFrontmatter(rawContent); - const skillDir = dirnameEnvPath(filePath); - const parentDirName = basenameEnvPath(skillDir); - - for (const error of validateDescription(frontmatter.description)) { - diagnostics.push({ type: "warning", message: error, path: filePath }); - } - - const name = frontmatter.name || parentDirName; - for (const error of validateName(name, parentDirName)) { - diagnostics.push({ type: "warning", message: error, path: filePath }); - } - - if (!frontmatter.description || frontmatter.description.trim() === "") { - return { skill: null, diagnostics }; - } - - return { - skill: { - name, - description: frontmatter.description, - content: body, - filePath, - disableModelInvocation: frontmatter["disable-model-invocation"] === true, - }, - diagnostics, - }; - } catch (error) { - const message = error instanceof Error ? error.message : "failed to parse skill file"; - diagnostics.push({ type: "warning", message, path: filePath }); + const rawContent = await env.readTextFile(filePath); + if (!rawContent.ok) { + diagnostics.push({ type: "warning", message: rawContent.error.message, path: filePath }); return { skill: null, diagnostics }; } + + const parsed = parseFrontmatter(rawContent.value); + if (!parsed.ok) { + diagnostics.push({ type: "warning", message: parsed.error.message, path: filePath }); + return { skill: null, diagnostics }; + } + + const { frontmatter, body } = parsed.value; + const skillDir = dirnameEnvPath(filePath); + const parentDirName = basenameEnvPath(skillDir); + const description = typeof frontmatter.description === "string" ? frontmatter.description : undefined; + + for (const error of validateDescription(description)) { + diagnostics.push({ type: "warning", message: error, path: filePath }); + } + + const frontmatterName = typeof frontmatter.name === "string" ? frontmatter.name : undefined; + const name = frontmatterName || parentDirName; + for (const error of validateName(name, parentDirName)) { + diagnostics.push({ type: "warning", message: error, path: filePath }); + } + + if (!description || description.trim() === "") { + return { skill: null, diagnostics }; + } + + return { + skill: { + name, + description, + content: body, + filePath, + disableModelInvocation: frontmatter["disable-model-invocation"] === true, + }, + diagnostics, + }; } function validateName(name: string, parentDirName: string): string[] { @@ -242,39 +243,29 @@ function validateDescription(description: string | undefined): string[] { return errors; } -function parseFrontmatter>(content: string): { frontmatter: T; body: string } { - const normalized = content.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); - if (!normalized.startsWith("---")) return { frontmatter: {} as T, body: normalized }; - const endIndex = normalized.indexOf("\n---", 3); - if (endIndex === -1) return { frontmatter: {} as T, body: normalized }; - const yamlString = normalized.slice(4, endIndex); - const body = normalized.slice(endIndex + 4).trim(); - return { frontmatter: (parse(yamlString) ?? {}) as T, body }; -} - -async function safeFileInfo( - env: ExecutionEnv, - path: string, -): Promise> | undefined> { +function parseFrontmatter>( + content: string, +): Result<{ frontmatter: T; body: string }, Error> { try { - return await env.fileInfo(path); - } catch { - return undefined; + const normalized = content.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); + if (!normalized.startsWith("---")) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; + const endIndex = normalized.indexOf("\n---", 3); + if (endIndex === -1) return { ok: true, value: { frontmatter: {} as T, body: normalized } }; + const yamlString = normalized.slice(4, endIndex); + const body = normalized.slice(endIndex + 4).trim(); + return { ok: true, value: { frontmatter: (parse(yamlString) ?? {}) as T, body } }; + } catch (error) { + return { ok: false, error: error instanceof Error ? error : new Error(String(error)) }; } } -async function resolveKind( - env: ExecutionEnv, - info: Awaited>, -): Promise<"file" | "directory" | undefined> { +async function resolveKind(env: ExecutionEnv, info: FileInfo): Promise<"file" | "directory" | undefined> { if (info.kind === "file" || info.kind === "directory") return info.kind; - try { - const realPath = await env.realPath(info.path); - const target = await env.fileInfo(realPath); - return target.kind === "file" || target.kind === "directory" ? target.kind : undefined; - } catch { - return undefined; - } + const realPath = await env.realPath(info.path); + if (!realPath.ok) return undefined; + const target = getOrUndefined(await env.fileInfo(realPath.value)); + if (!target) return undefined; + return target.kind === "file" || target.kind === "directory" ? target.kind : undefined; } function joinEnvPath(base: string, child: string): string { diff --git a/packages/agent/src/harness/types.ts b/packages/agent/src/harness/types.ts index f50337f8..d80f2a21 100644 --- a/packages/agent/src/harness/types.ts +++ b/packages/agent/src/harness/types.ts @@ -3,6 +3,30 @@ import type { QueueMode } from "../agent.js"; import type { AgentEvent, AgentMessage, AgentTool, ThinkingLevel } from "../index.js"; import type { Session } from "./session/session.js"; +/** Result of a fallible operation. Expected failures are returned as `ok: false` instead of thrown. */ +export type Result = { ok: true; value: TValue } | { ok: false; error: TError }; + +/** Create a successful {@link Result}. */ +export function ok(value: TValue): Result { + return { ok: true, value }; +} + +/** Create a failed {@link Result}. */ +export function err(error: TError): Result { + return { ok: false, error }; +} + +/** Return the success value or throw the failure error. Intended for tests and explicit adapter boundaries. */ +export function getOrThrow(result: Result): TValue { + if (!result.ok) throw result.error; + return result.value; +} + +/** Return the success value or `undefined`. Only object values are allowed to avoid truthiness bugs with primitives. */ +export function getOrUndefined(result: Result): TValue | undefined { + return result.ok ? result.value : undefined; +} + /** * Skill loaded from a `SKILL.md` file or provided by an application. * @@ -73,8 +97,9 @@ export interface AgentHarnessStreamOptionsPatch /** Kind of filesystem object as addressed by an {@link ExecutionEnv}. Symlinks are not followed automatically. */ export type FileKind = "file" | "directory" | "symlink"; -/** Stable, backend-independent file error codes thrown by {@link ExecutionEnv} file operations. */ +/** Stable, backend-independent file error codes returned by {@link ExecutionEnv} file operations. */ export type FileErrorCode = + | "aborted" | "not_found" | "permission_denied" | "not_directory" @@ -83,7 +108,7 @@ export type FileErrorCode = | "not_supported" | "unknown"; -/** Error thrown by {@link ExecutionEnv} file operations. */ +/** Error returned by {@link ExecutionEnv} file operations. */ export class FileError extends Error { constructor( /** Backend-independent error code. */ @@ -91,13 +116,29 @@ export class FileError extends Error { message: string, /** Absolute addressed path associated with the failure, when available. */ public path?: string, - options?: ErrorOptions, + cause?: unknown, ) { - super(message, options); + super(message, cause === undefined ? undefined : { cause }); this.name = "FileError"; } } +/** Stable, backend-independent execution error codes returned by {@link ExecutionEnv.exec}. */ +export type ExecutionErrorCode = "aborted" | "timeout" | "shell_unavailable" | "spawn_error" | "unknown"; + +/** Error returned by {@link ExecutionEnv.exec}. */ +export class ExecutionError extends Error { + constructor( + /** Backend-independent error code. */ + public code: ExecutionErrorCode, + message: string, + cause?: unknown, + ) { + super(message, cause === undefined ? undefined : { cause }); + this.name = "ExecutionError"; + } +} + /** Metadata for one filesystem object in an {@link ExecutionEnv}. */ export interface FileInfo { /** Basename of {@link path}. */ @@ -114,14 +155,14 @@ export interface FileInfo { /** Options for {@link ExecutionEnv.exec}. */ export interface ExecutionEnvExecOptions { - /** Working directory for the command. Relative paths are resolved against {@link ExecutionEnv.cwd}. */ + /** Working directory for the command. Relative paths are resolved against {@link ExecutionEnv.cwd}. Defaults to {@link ExecutionEnv.cwd}. */ cwd?: string; - /** Additional environment variables for the command. Values override the environment defaults. */ + /** Additional environment variables for the command. Values override the environment defaults. Defaults to no overrides. */ env?: Record; - /** Timeout in seconds. Implementations should reject when the command exceeds this duration. */ + /** Timeout in seconds. Implementations should return a timeout error when the command exceeds this duration. Defaults to no timeout. */ timeout?: number; - /** Abort signal used to terminate the command. */ - signal?: AbortSignal; + /** Abort signal used to terminate the command. Defaults to no abort signal. */ + abortSignal?: AbortSignal; /** Called with stdout chunks as they are produced. */ onStdout?: (chunk: string) => void; /** Called with stderr chunks as they are produced. */ @@ -134,7 +175,8 @@ export interface ExecutionEnvExecOptions { * Paths passed to methods may be absolute or relative to {@link cwd}. Paths returned by this interface are absolute * addressed paths in the environment, but are not canonicalized through symlinks unless returned by {@link resolvePath}. * - * File operations throw {@link FileError} for expected filesystem failures such as missing paths or permission errors. + * Operation methods must never throw or reject. All filesystem/process failures, including unexpected backend failures, + * must be encoded in the returned {@link Result}. Implementations must preserve this invariant. */ export interface ExecutionEnv { /** Current working directory for relative paths and command execution. */ @@ -144,32 +186,44 @@ export interface ExecutionEnv { exec( command: string, options?: ExecutionEnvExecOptions, - ): Promise<{ stdout: string; stderr: string; exitCode: number }>; + ): Promise>; - /** Read a UTF-8 text file. Throws {@link FileError}. */ - readTextFile(path: string): Promise; - /** Read a binary file. Throws {@link FileError}. */ - readBinaryFile(path: string): Promise; - /** Create or overwrite a file, creating parent directories when supported. Throws {@link FileError}. */ - writeFile(path: string, content: string | Uint8Array): Promise; - /** Return metadata for the addressed path without following symlinks. Throws {@link FileError}. */ - fileInfo(path: string): Promise; - /** List direct children of a directory without following symlinks. Throws {@link FileError}. */ - listDir(path: string): Promise; - /** Return the canonical path for a path, following symlinks. Throws {@link FileError}. */ - realPath(path: string): Promise; - /** Return false for missing paths. Other errors, such as permission failures, may throw {@link FileError}. */ - exists(path: string): Promise; - /** Create a directory. */ - createDir(path: string, options?: { recursive?: boolean }): Promise; - /** Remove a file or directory. */ - remove(path: string, options?: { recursive?: boolean; force?: boolean }): Promise; - /** Create a temporary directory and return its absolute path. */ - createTempDir(prefix?: string): Promise; - /** Create a temporary file and return its absolute path. */ - createTempFile(options?: { prefix?: string; suffix?: string }): Promise; + /** Read a UTF-8 text file. */ + readTextFile(path: string, abortSignal?: AbortSignal): Promise>; + /** Read a binary file. */ + readBinaryFile(path: string, abortSignal?: AbortSignal): Promise>; + /** Create or overwrite a file, creating parent directories when supported. */ + writeFile(path: string, content: string | Uint8Array, abortSignal?: AbortSignal): Promise>; + /** Create or append to a file, creating parent directories when supported. */ + appendFile(path: string, content: string | Uint8Array, abortSignal?: AbortSignal): Promise>; + /** Return metadata for the addressed path without following symlinks. */ + fileInfo(path: string, abortSignal?: AbortSignal): Promise>; + /** List direct children of a directory without following symlinks. */ + listDir(path: string, abortSignal?: AbortSignal): Promise>; + /** Return the canonical path for a path, following symlinks. */ + realPath(path: string, abortSignal?: AbortSignal): Promise>; + /** Return false for missing paths. Other errors, such as permission failures, return a {@link FileError}. */ + exists(path: string, abortSignal?: AbortSignal): Promise>; + /** Create a directory. Defaults: `recursive: true`, no abort signal. */ + createDir( + path: string, + options?: { recursive?: boolean; abortSignal?: AbortSignal }, + ): Promise>; + /** Remove a file or directory. Defaults: `recursive: false`, `force: false`, no abort signal. */ + remove( + path: string, + options?: { recursive?: boolean; force?: boolean; abortSignal?: AbortSignal }, + ): Promise>; + /** Create a temporary directory and return its absolute path. Defaults: `prefix: "tmp-"`, no abort signal. */ + createTempDir(prefix?: string, abortSignal?: AbortSignal): Promise>; + /** Create a temporary file and return its absolute path. Defaults: `prefix: ""`, `suffix: ""`, no abort signal. */ + createTempFile(options?: { + prefix?: string; + suffix?: string; + abortSignal?: AbortSignal; + }): Promise>; - /** Release resources owned by the environment. */ + /** Release resources owned by the environment. Must be best-effort and must not throw or reject. */ cleanup(): Promise; } diff --git a/packages/agent/src/harness/utils/shell-output.ts b/packages/agent/src/harness/utils/shell-output.ts index d31d7a06..fcb8d538 100644 --- a/packages/agent/src/harness/utils/shell-output.ts +++ b/packages/agent/src/harness/utils/shell-output.ts @@ -1,8 +1,4 @@ -import { randomBytes } from "node:crypto"; -import { createWriteStream, type WriteStream } from "node:fs"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; -import type { ExecutionEnv, ExecutionEnvExecOptions } from "../types.js"; +import { type ExecutionEnv, type ExecutionEnvExecOptions, ExecutionError, err, ok, type Result } from "../types.js"; import { DEFAULT_MAX_BYTES, truncateTail } from "./truncate.js"; export interface ShellCaptureOptions extends Omit { @@ -17,6 +13,12 @@ export interface ShellCaptureResult { fullOutputPath?: string; } +function toExecutionError(error: unknown): ExecutionError { + return error instanceof ExecutionError + ? error + : new ExecutionError("unknown", error instanceof Error ? error.message : String(error), error); +} + export function sanitizeBinaryOutput(str: string): string { return Array.from(str) .filter((char) => { @@ -34,41 +36,62 @@ export async function executeShellWithCapture( env: ExecutionEnv, command: string, options?: ShellCaptureOptions, -): Promise { +): Promise> { const outputChunks: string[] = []; let outputBytes = 0; const maxOutputBytes = DEFAULT_MAX_BYTES * 2; + const encoder = new TextEncoder(); - let tempFilePath: string | undefined; - let tempFileStream: WriteStream | undefined; let totalBytes = 0; + let fullOutputPath: string | undefined; + let writeChain: Promise> = Promise.resolve(ok(undefined)); + let captureError: ExecutionError | undefined; - const ensureTempFile = () => { - if (tempFilePath) return; - const id = randomBytes(8).toString("hex"); - tempFilePath = join(tmpdir(), `bash-${id}.log`); - tempFileStream = createWriteStream(tempFilePath); - for (const chunk of outputChunks) { - tempFileStream.write(chunk); - } + const appendFullOutput = (text: string): void => { + if (!fullOutputPath || captureError) return; + const path = fullOutputPath; + writeChain = writeChain.then(async (previous) => { + if (!previous.ok) return previous; + const appendResult = await env.appendFile(path, text, options?.abortSignal); + return appendResult.ok ? ok(undefined) : err(toExecutionError(appendResult.error)); + }); + }; + + const ensureFullOutputFile = (initialContent: string): void => { + if (fullOutputPath || captureError) return; + writeChain = writeChain.then(async (previous) => { + if (!previous.ok) return previous; + const tempFile = await env.createTempFile({ + prefix: "bash-", + suffix: ".log", + abortSignal: options?.abortSignal, + }); + if (!tempFile.ok) return err(toExecutionError(tempFile.error)); + fullOutputPath = tempFile.value; + const appendResult = await env.appendFile(tempFile.value, initialContent, options?.abortSignal); + return appendResult.ok ? ok(undefined) : err(toExecutionError(appendResult.error)); + }); }; const onChunk = (chunk: string) => { - totalBytes += Buffer.byteLength(chunk, "utf-8"); - const text = sanitizeBinaryOutput(chunk).replace(/\r/g, ""); - if (totalBytes > DEFAULT_MAX_BYTES) { - ensureTempFile(); + try { + totalBytes += encoder.encode(chunk).byteLength; + const text = sanitizeBinaryOutput(chunk).replace(/\r/g, ""); + if (totalBytes > DEFAULT_MAX_BYTES && !fullOutputPath) { + ensureFullOutputFile(outputChunks.join("") + text); + } else { + appendFullOutput(text); + } + outputChunks.push(text); + outputBytes += text.length; + while (outputBytes > maxOutputBytes && outputChunks.length > 1) { + const removed = outputChunks.shift()!; + outputBytes -= removed.length; + } + options?.onChunk?.(text); + } catch (error) { + captureError = toExecutionError(error); } - if (tempFileStream) { - tempFileStream.write(text); - } - outputChunks.push(text); - outputBytes += text.length; - while (outputBytes > maxOutputBytes && outputChunks.length > 1) { - const removed = outputChunks.shift()!; - outputBytes -= removed.length; - } - options?.onChunk?.(text); }; try { @@ -77,37 +100,36 @@ export async function executeShellWithCapture( onStdout: onChunk, onStderr: onChunk, }); - const fullOutput = outputChunks.join(""); - const truncationResult = truncateTail(fullOutput); - if (truncationResult.truncated) { - ensureTempFile(); + const tailOutput = outputChunks.join(""); + const truncationResult = truncateTail(tailOutput); + if (truncationResult.truncated && !fullOutputPath) { + ensureFullOutputFile(tailOutput); } - tempFileStream?.end(); - const cancelled = options?.signal?.aborted ?? false; - return { - output: truncationResult.truncated ? truncationResult.content : fullOutput, - exitCode: cancelled ? undefined : result.exitCode, + const writeResult = await writeChain; + if (!writeResult.ok) return err(writeResult.error); + if (captureError) return err(captureError); + + if (!result.ok) { + if (result.error.code === "aborted" || options?.abortSignal?.aborted) { + return ok({ + output: truncationResult.truncated ? truncationResult.content : tailOutput, + exitCode: undefined, + cancelled: true, + truncated: truncationResult.truncated, + fullOutputPath, + }); + } + return err(result.error); + } + const cancelled = options?.abortSignal?.aborted ?? false; + return ok({ + output: truncationResult.truncated ? truncationResult.content : tailOutput, + exitCode: cancelled ? undefined : result.value.exitCode, cancelled, truncated: truncationResult.truncated, - fullOutputPath: tempFilePath, - }; - } catch (err) { - if (options?.signal?.aborted) { - const fullOutput = outputChunks.join(""); - const truncationResult = truncateTail(fullOutput); - if (truncationResult.truncated) { - ensureTempFile(); - } - tempFileStream?.end(); - return { - output: truncationResult.truncated ? truncationResult.content : fullOutput, - exitCode: undefined, - cancelled: true, - truncated: truncationResult.truncated, - fullOutputPath: tempFilePath, - }; - } - tempFileStream?.end(); - throw err; + fullOutputPath, + }); + } catch (error) { + return err(toExecutionError(error)); } } diff --git a/packages/agent/test/harness/agent-harness-stream.test.ts b/packages/agent/test/harness/agent-harness-stream.test.ts index e014b377..e5ca193f 100644 --- a/packages/agent/test/harness/agent-harness-stream.test.ts +++ b/packages/agent/test/harness/agent-harness-stream.test.ts @@ -1,7 +1,7 @@ import { fauxAssistantMessage, fauxToolCall, registerFauxProvider, type StreamOptions } from "@earendil-works/pi-ai"; import { afterEach, describe, expect, it } from "vitest"; import { AgentHarness } from "../../src/harness/agent-harness.js"; -import { NodeExecutionEnv } from "../../src/harness/execution-env.js"; +import { NodeExecutionEnv } from "../../src/harness/env/nodejs.js"; import { Session } from "../../src/harness/session/session.js"; import { InMemorySessionStorage } from "../../src/harness/session/storage/memory.js"; import { calculateTool } from "../utils/calculate.js"; diff --git a/packages/agent/test/harness/agent-harness.test.ts b/packages/agent/test/harness/agent-harness.test.ts index 453b1297..f8111f70 100644 --- a/packages/agent/test/harness/agent-harness.test.ts +++ b/packages/agent/test/harness/agent-harness.test.ts @@ -1,7 +1,7 @@ import { getModel } from "@earendil-works/pi-ai"; import { describe, expect, it } from "vitest"; import { AgentHarness } from "../../src/harness/agent-harness.js"; -import { NodeExecutionEnv } from "../../src/harness/execution-env.js"; +import { NodeExecutionEnv } from "../../src/harness/env/nodejs.js"; import { Session } from "../../src/harness/session/session.js"; import { InMemorySessionStorage } from "../../src/harness/session/storage/memory.js"; import type { PromptTemplate, Skill } from "../../src/harness/types.js"; diff --git a/packages/agent/test/harness/nodejs-env.test.ts b/packages/agent/test/harness/nodejs-env.test.ts index 2059c075..97c5da23 100644 --- a/packages/agent/test/harness/nodejs-env.test.ts +++ b/packages/agent/test/harness/nodejs-env.test.ts @@ -1,7 +1,9 @@ import { access, chmod, realpath, symlink } from "node:fs/promises"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { FileError, NodeExecutionEnv } from "../../src/harness/execution-env.js"; +import { NodeExecutionEnv } from "../../src/harness/env/nodejs.js"; +import { FileError, getOrThrow } from "../../src/harness/execution-env.js"; +import { executeShellWithCapture } from "../../src/harness/utils/shell-output.js"; import { createTempDir } from "./session-test-utils.js"; const chmodRestorePaths: string[] = []; @@ -19,61 +21,66 @@ describe("NodeExecutionEnv", () => { it("reads, writes, lists, and removes files and directories", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); - await env.createDir("nested", { recursive: true }); - await env.writeFile("nested/file.txt", "hello"); - expect(await env.readTextFile("nested/file.txt")).toBe("hello"); - expect(Buffer.from(await env.readBinaryFile("nested/file.txt")).toString("utf8")).toBe("hello"); + getOrThrow(await env.createDir("nested/child")); + getOrThrow(await env.writeFile("nested/child/file.txt", "hel")); + getOrThrow(await env.appendFile("nested/child/file.txt", "lo")); + expect(getOrThrow(await env.readTextFile("nested/child/file.txt"))).toBe("hello"); + expect(Buffer.from(getOrThrow(await env.readBinaryFile("nested/child/file.txt"))).toString("utf8")).toBe("hello"); - const entries = await env.listDir("nested"); + const entries = getOrThrow(await env.listDir("nested/child")); expect(entries).toHaveLength(1); expect(entries[0]).toMatchObject({ name: "file.txt", - path: join(root, "nested/file.txt"), + path: join(root, "nested/child/file.txt"), kind: "file", size: 5, }); expect(typeof entries[0]!.mtimeMs).toBe("number"); - expect(await env.exists("nested/file.txt")).toBe(true); - await env.remove("nested/file.txt"); - expect(await env.exists("nested/file.txt")).toBe(false); + expect(getOrThrow(await env.exists("nested/child/file.txt"))).toBe(true); + getOrThrow(await env.remove("nested/child/file.txt")); + expect(getOrThrow(await env.exists("nested/child/file.txt"))).toBe(false); }); it("returns fileInfo for files, directories, and symlinks without following symlinks", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); - await env.createDir("dir", { recursive: true }); - await env.writeFile("dir/file.txt", "hello"); + getOrThrow(await env.createDir("dir", { recursive: true })); + getOrThrow(await env.writeFile("dir/file.txt", "hello")); await symlink(join(root, "dir/file.txt"), join(root, "file-link")); await symlink(join(root, "dir"), join(root, "dir-link")); - expect(await env.fileInfo("dir")).toMatchObject({ name: "dir", path: join(root, "dir"), kind: "directory" }); - expect(await env.fileInfo("dir/file.txt")).toMatchObject({ + expect(getOrThrow(await env.fileInfo("dir"))).toMatchObject({ + name: "dir", + path: join(root, "dir"), + kind: "directory", + }); + expect(getOrThrow(await env.fileInfo("dir/file.txt"))).toMatchObject({ name: "file.txt", path: join(root, "dir/file.txt"), kind: "file", size: 5, }); - expect(await env.fileInfo("file-link")).toMatchObject({ + expect(getOrThrow(await env.fileInfo("file-link"))).toMatchObject({ name: "file-link", path: join(root, "file-link"), kind: "symlink", }); - expect(await env.fileInfo("dir-link")).toMatchObject({ + expect(getOrThrow(await env.fileInfo("dir-link"))).toMatchObject({ name: "dir-link", path: join(root, "dir-link"), kind: "symlink", }); - expect(await env.realPath("file-link")).toBe(await realpath(join(root, "dir/file.txt"))); + expect(getOrThrow(await env.realPath("file-link"))).toBe(await realpath(join(root, "dir/file.txt"))); }); it("lists symlinks as symlinks", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); - await env.writeFile("target.txt", "hello"); + getOrThrow(await env.writeFile("target.txt", "hello")); await symlink(join(root, "target.txt"), join(root, "link.txt")); - const entries = await env.listDir("."); + const entries = getOrThrow(await env.listDir(".")); expect( entries.map((entry) => ({ name: entry.name, kind: entry.kind })).sort((a, b) => a.name.localeCompare(b.name)), ).toEqual([ @@ -82,41 +89,112 @@ describe("NodeExecutionEnv", () => { ]); }); - it("throws FileError for missing paths and keeps exists false for missing paths", async () => { + it("returns FileError for missing paths and keeps exists false for missing paths", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); - await expect(env.fileInfo("missing.txt")).rejects.toMatchObject({ - name: "FileError", - code: "not_found", - path: join(root, "missing.txt"), - }); - expect(await env.exists("missing.txt")).toBe(false); + const info = await env.fileInfo("missing.txt"); + expect(info.ok).toBe(false); + if (!info.ok) { + expect(info.error).toBeInstanceOf(FileError); + expect(info.error).toMatchObject({ + name: "FileError", + code: "not_found", + path: join(root, "missing.txt"), + }); + } + expect(getOrThrow(await env.exists("missing.txt"))).toBe(false); }); - it("throws FileError for listing non-directories", async () => { + it("returns FileError for listing non-directories", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); - await env.writeFile("file.txt", "hello"); - await expect(env.listDir("file.txt")).rejects.toBeInstanceOf(FileError); - await expect(env.listDir("file.txt")).rejects.toMatchObject({ code: "not_directory" }); + getOrThrow(await env.writeFile("file.txt", "hello")); + const result = await env.listDir("file.txt"); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toBeInstanceOf(FileError); + expect(result.error).toMatchObject({ code: "not_directory" }); + } + }); + + it("appends to new files and creates parent directories", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + getOrThrow(await env.appendFile("new/nested/file.txt", "a")); + getOrThrow(await env.appendFile("new/nested/file.txt", "b")); + expect(getOrThrow(await env.readTextFile("new/nested/file.txt"))).toBe("ab"); }); it("creates temporary directories and files", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); - const tempDir = await env.createTempDir("node-env-test-"); + const tempDir = getOrThrow(await env.createTempDir("node-env-test-")); await expect(access(tempDir)).resolves.toBeUndefined(); - const tempFile = await env.createTempFile({ prefix: "prefix-", suffix: ".txt" }); + const tempFile = getOrThrow(await env.createTempFile({ prefix: "prefix-", suffix: ".txt" })); await expect(access(tempFile)).resolves.toBeUndefined(); expect(tempFile.endsWith(".txt")).toBe(true); }); + it("honors createDir recursive false and remove recursive/force options", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + const createResult = await env.createDir("missing/child", { recursive: false }); + expect(createResult.ok).toBe(false); + if (!createResult.ok) expect(createResult.error).toMatchObject({ code: "not_found" }); + + getOrThrow(await env.writeFile("dir/child/file.txt", "hello")); + const removeDirectory = await env.remove("dir", { recursive: false }); + expect(removeDirectory.ok).toBe(false); + getOrThrow(await env.remove("dir", { recursive: true })); + expect(getOrThrow(await env.exists("dir"))).toBe(false); + + const removeMissing = await env.remove("missing", { force: false }); + expect(removeMissing.ok).toBe(false); + getOrThrow(await env.remove("missing", { force: true })); + }); + + it("returns aborted results for pre-aborted file operations", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + getOrThrow(await env.writeFile("file.txt", "hello")); + const controller = new AbortController(); + controller.abort(); + const signal = controller.signal; + + const results = await Promise.all([ + env.readTextFile("file.txt", signal), + env.readBinaryFile("file.txt", signal), + env.writeFile("other.txt", "hello", signal), + env.appendFile("other.txt", "hello", signal), + env.fileInfo("file.txt", signal), + env.listDir(".", signal), + env.realPath("file.txt", signal), + env.exists("file.txt", signal), + env.createDir("dir", { abortSignal: signal }), + env.remove("file.txt", { abortSignal: signal }), + env.createTempDir("node-env-test-", signal), + env.createTempFile({ abortSignal: signal }), + ]); + for (const result of results) { + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toMatchObject({ code: "aborted" }); + } + }); + + it("cleanup is best-effort", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + await expect(env.cleanup()).resolves.toBeUndefined(); + }); + it("executes commands in cwd with env overrides", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); - const result = await env.exec('printf \'%s:%s\' "$PWD" "$NODE_ENV_TEST"', { - env: { NODE_ENV_TEST: "ok" }, - }); + const result = getOrThrow( + await env.exec('printf \'%s:%s\' "$PWD" "$NODE_ENV_TEST"', { + env: { NODE_ENV_TEST: "ok" }, + }), + ); expect(result).toEqual({ stdout: `${await realpath(root)}:ok`, stderr: "", exitCode: 0 }); }); @@ -125,25 +203,83 @@ describe("NodeExecutionEnv", () => { const env = new NodeExecutionEnv({ cwd: root }); let stdout = ""; let stderr = ""; - const result = await env.exec("printf out; printf err >&2", { - onStdout: (chunk) => { - stdout += chunk; - }, - onStderr: (chunk) => { - stderr += chunk; - }, - }); + const result = getOrThrow( + await env.exec("printf out; printf err >&2", { + onStdout: (chunk) => { + stdout += chunk; + }, + onStderr: (chunk) => { + stderr += chunk; + }, + }), + ); expect(result).toEqual({ stdout: "out", stderr: "err", exitCode: 0 }); expect(stdout).toBe("out"); expect(stderr).toBe("err"); }); - it("rejects aborted commands", async () => { + it("returns non-zero command exit codes as successful execution results", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + const result = getOrThrow(await env.exec("exit 7")); + expect(result).toEqual({ stdout: "", stderr: "", exitCode: 7 }); + }); + + it("returns timeout errors for commands exceeding the timeout", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + const result = await env.exec("sleep 5", { timeout: 0.01 }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toMatchObject({ code: "timeout" }); + }); + + it("returns callback errors from exec stream handlers", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + const result = await env.exec("printf out", { + onStdout: () => { + throw new Error("callback failed"); + }, + }); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toMatchObject({ code: "unknown", message: "callback failed" }); + }); + + it("returns shell unavailable and spawn errors", async () => { + const root = createTempDir(); + const missingShellEnv = new NodeExecutionEnv({ cwd: root, shellPath: join(root, "missing-shell") }); + const missingShell = await missingShellEnv.exec("printf ok"); + expect(missingShell.ok).toBe(false); + if (!missingShell.ok) expect(missingShell.error).toMatchObject({ code: "shell_unavailable" }); + + const shellPath = join(root, "not-executable-shell"); + const env = new NodeExecutionEnv({ cwd: root }); + getOrThrow(await env.writeFile(shellPath, "not executable")); + const spawnErrorEnv = new NodeExecutionEnv({ cwd: root, shellPath }); + const spawnError = await spawnErrorEnv.exec("printf ok"); + expect(spawnError.ok).toBe(false); + if (!spawnError.ok) expect(spawnError.error).toMatchObject({ code: "spawn_error" }); + }); + + it("returns an aborted result for aborted commands", async () => { const root = createTempDir(); const env = new NodeExecutionEnv({ cwd: root }); const controller = new AbortController(); - const promise = env.exec("sleep 5", { signal: controller.signal }); + const promise = env.exec("sleep 5", { abortSignal: controller.signal }); controller.abort(); - await expect(promise).rejects.toThrow("aborted"); + const result = await promise; + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toMatchObject({ code: "aborted" }); + }); + + it("captures large shell output to a full output file through the execution env", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + const result = getOrThrow(await executeShellWithCapture(env, "yes line | head -n 15000")); + expect(result.truncated).toBe(true); + expect(result.fullOutputPath).toBeDefined(); + const fullOutput = getOrThrow(await env.readTextFile(result.fullOutputPath!)); + expect(fullOutput.split("\n").length).toBeGreaterThan(10000); + expect(result.output.length).toBeLessThan(fullOutput.length); }); }); diff --git a/packages/agent/test/harness/prompt-templates.test.ts b/packages/agent/test/harness/prompt-templates.test.ts index 62ae9aee..adb11006 100644 --- a/packages/agent/test/harness/prompt-templates.test.ts +++ b/packages/agent/test/harness/prompt-templates.test.ts @@ -1,7 +1,7 @@ import { symlink } from "node:fs/promises"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; -import { NodeExecutionEnv } from "../../src/harness/execution-env.js"; +import { NodeExecutionEnv } from "../../src/harness/env/nodejs.js"; import { formatPromptTemplateInvocation, loadPromptTemplates, diff --git a/packages/agent/test/harness/skills.test.ts b/packages/agent/test/harness/skills.test.ts index f20c70bd..c4d53cbb 100644 --- a/packages/agent/test/harness/skills.test.ts +++ b/packages/agent/test/harness/skills.test.ts @@ -1,7 +1,7 @@ import { symlink } from "node:fs/promises"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; -import { NodeExecutionEnv } from "../../src/harness/execution-env.js"; +import { NodeExecutionEnv } from "../../src/harness/env/nodejs.js"; import { loadSkills, loadSourcedSkills } from "../../src/harness/skills.js"; import { createTempDir } from "./session-test-utils.js"; diff --git a/packages/agent/test/scratch/simple.ts b/packages/agent/test/scratch/simple.ts index c417740f..296b2160 100644 --- a/packages/agent/test/scratch/simple.ts +++ b/packages/agent/test/scratch/simple.ts @@ -1,13 +1,13 @@ import { homedir } from "node:os"; import { join } from "node:path"; import { getModel } from "@earendil-works/pi-ai"; +import { NodeExecutionEnv } from "../../src/harness/env/nodejs.js"; import { InMemorySessionStorage } from "../../src/harness/session/storage/memory.js"; import { AgentHarness, formatSkillsForSystemPrompt, loadSourcedPromptTemplates, loadSourcedSkills, - NodeExecutionEnv, type PromptTemplate, Session, type Skill,