diff --git a/src/core/agents/claude.ts b/src/core/agents/claude.ts index 1d3efe86..86d336c4 100644 --- a/src/core/agents/claude.ts +++ b/src/core/agents/claude.ts @@ -11,17 +11,14 @@ import { PermanentAgentError, } from "./types.js"; import { shutdownChildProcess } from "./managed-process.js"; -import { parseJSONLStream, setupAbortHandler } from "./stream-utils.js"; +import { + appendExitOutputTail, + describeChildProcessExit, + parseJSONLStream, + setupAbortHandler, +} from "./stream-utils.js"; const DEFAULT_FINAL_RESULT_EXIT_GRACE_MS = 15_000; -/** Upper bound on the stdout tail kept for non-zero-exit error reporting. */ -const MAX_EXIT_OUTPUT_CHARS = 4_000; -/** - * Tighter bound on unstructured stdout quoted back in the failure detail: that - * text lands in notes.md and is replayed in every later iteration prompt. - */ -const MAX_RAW_TAIL_CHARS = 400; -const RAW_TAIL_ELISION = "[...truncated, full output in the iteration log] "; interface ClaudeAssistantEvent { type: "assistant"; @@ -204,103 +201,6 @@ function isPermanentClaudeError(output: string): boolean { return /credit balance\s+is\s+too\s+low/i.test(output); } -/** Keep only the last `MAX_EXIT_OUTPUT_CHARS` characters so long streams stay bounded. */ -function appendBoundedTail(existing: string, chunk: string): string { - const combined = existing + chunk; - return combined.length > MAX_EXIT_OUTPUT_CHARS - ? combined.slice(combined.length - MAX_EXIT_OUTPUT_CHARS) - : combined; -} - -function errorTextFromEvent(event: unknown): string | null { - if (!event || typeof event !== "object") return null; - const record = event as Record; - - const error = record.error; - if (typeof error === "string" && error.trim()) return error.trim(); - if (error && typeof error === "object") { - const message = (error as Record).message; - if (typeof message === "string" && message.trim()) return message.trim(); - } - - if (record.is_error === true || record.type === "error") { - for (const key of ["result", "message", "subtype"]) { - const value = record[key]; - if (typeof value === "string" && value.trim()) return value.trim(); - } - } - - return null; -} - -/** Quote only the end of unstructured output, marking what was dropped. */ -function elideRawTail(raw: string): string { - return raw.length > MAX_RAW_TAIL_CHARS - ? `${RAW_TAIL_ELISION}${raw.slice(raw.length - MAX_RAW_TAIL_CHARS)}` - : raw; -} - -interface StdoutFailure { - /** Error text the CLI itself authored in structured stdout events. */ - structured: string; - /** Text worth reporting: the structured text, or a short raw tail. */ - reported: string; -} - -/** - * Pull the CLI's own error text out of its stdout, which is JSONL when the run - * got far enough to stream events and plain text otherwise. Falls back to a - * bounded raw tail so the reported detail is never empty when stdout had - * content. - */ -function extractStdoutError(stdoutTail: string): StdoutFailure { - const messages: string[] = []; - for (const line of stdoutTail.split("\n")) { - if (!line.trim()) continue; - try { - const message = errorTextFromEvent(JSON.parse(line)); - if (message) messages.push(message); - } catch { - // Not JSON: covered by the raw-tail fallback below. - } - } - const structured = messages.join("\n"); - return { - structured, - reported: structured || elideRawTail(stdoutTail.trim()), - }; -} - -interface ExitFailure { - detail: string; - permanent: boolean; -} - -/** - * Describe a non-zero exit. `detail` reports everything both streams offered, - * while `permanent` is decided only from text the CLI itself authored - stderr - * and structured stdout error fields - so agent output that merely quotes a - * permanent-failure phrase cannot abort an otherwise retryable run. - */ -function describeExitFailure( - code: number | null, - stdoutTail: string, - stderr: string, -): ExitFailure { - const trimmedStderr = stderr.trim(); - const stdoutError = extractStdoutError(stdoutTail); - const segments = [trimmedStderr, stdoutError.reported].filter(Boolean); - return { - detail: - segments.length > 0 - ? `claude exited with code ${code}: ${segments.join("\n")}` - : `claude exited with code ${code} and produced no output`, - permanent: isPermanentClaudeError( - [trimmedStderr, stdoutError.structured].filter(Boolean).join("\n"), - ), - }; -} - export class ClaudeAgent implements Agent { name = "claude"; @@ -375,7 +275,7 @@ export class ClaudeAgent implements Agent { }); child.stdout!.on("data", (data: Buffer) => { - stdoutTail = appendBoundedTail(stdoutTail, data.toString()); + stdoutTail = appendExitOutputTail(stdoutTail, data.toString()); }); child.on("error", (err) => { @@ -501,9 +401,14 @@ export class ClaudeAgent implements Agent { } logStream?.end(); if (code !== 0 && !closedAfterFinalCleanup) { - const failure = describeExitFailure(code, stdoutTail, stderr); + const failure = describeChildProcessExit( + "claude", + code, + stdoutTail, + stderr, + ); reject( - failure.permanent + isPermanentClaudeError(failure.errorOutput) ? new PermanentAgentError( "claude credit balance too low - see gnhf.log", failure.detail, diff --git a/src/core/agents/codex.test.ts b/src/core/agents/codex.test.ts index 86369a8d..db0c34c7 100644 --- a/src/core/agents/codex.test.ts +++ b/src/core/agents/codex.test.ts @@ -203,4 +203,21 @@ describe("CodexAgent", () => { ); expect(proc.kill).not.toHaveBeenCalled(); }); + + it("surfaces a structured error emitted on stdout after a non-zero exit", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CodexAgent("/tmp/schema.json"); + + const promise = agent.run("test prompt", "/work/dir"); + proc.stdout.emit( + "data", + Buffer.from('{"type":"error","error":{"message":"login required"}}\n'), + ); + proc.emit("close", 1); + + await expect(promise).rejects.toThrow( + "codex exited with code 1: login required", + ); + }); }); diff --git a/src/core/agents/copilot.test.ts b/src/core/agents/copilot.test.ts index e5e326c0..64b73e94 100644 --- a/src/core/agents/copilot.test.ts +++ b/src/core/agents/copilot.test.ts @@ -332,4 +332,18 @@ describe("CopilotAgent", () => { await expect(promise).rejects.toThrow("Failed to parse copilot output"); }); + + it("surfaces a structured error emitted on stdout after a non-zero exit", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CopilotAgent(); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, { type: "error", error: { message: "login required" } }); + proc.emit("close", 1); + + await expect(promise).rejects.toThrow( + "copilot exited with code 1: login required", + ); + }); }); diff --git a/src/core/agents/pi.test.ts b/src/core/agents/pi.test.ts index 3ce1a367..f627caac 100644 --- a/src/core/agents/pi.test.ts +++ b/src/core/agents/pi.test.ts @@ -509,4 +509,18 @@ describe("PiAgent", () => { await expect(promise).rejects.toThrow("pi exited with code 2: bad things"); }); + + it("surfaces a structured error emitted on stdout after a non-zero exit", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new PiAgent(); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, { type: "error", error: { message: "login required" } }); + proc.emit("close", 1); + + await expect(promise).rejects.toThrow( + "pi exited with code 1: login required", + ); + }); }); diff --git a/src/core/agents/stream-utils.test.ts b/src/core/agents/stream-utils.test.ts index 81a00491..9b3d9639 100644 --- a/src/core/agents/stream-utils.test.ts +++ b/src/core/agents/stream-utils.test.ts @@ -8,9 +8,11 @@ import { function createMockChild() { const child = new EventEmitter() as EventEmitter & { + stdout: EventEmitter; stderr: EventEmitter; kill: ReturnType; }; + child.stdout = new EventEmitter(); child.stderr = new EventEmitter(); child.kill = vi.fn(); return child; @@ -93,6 +95,35 @@ describe("setupChildProcessHandlers", () => { ); }); + it("rejects with a structured stdout error when stderr is empty", () => { + const child = createMockChild(); + const reject = vi.fn(); + + setupChildProcessHandlers(child as never, "codex", null, reject, vi.fn()); + + child.stdout.emit( + "data", + Buffer.from('{"type":"error","error":{"message":"login required"}}\n'), + ); + child.emit("close", 1); + + expect(reject).toHaveBeenCalledWith( + new Error("codex exited with code 1: login required"), + ); + }); + + it("says so when a non-zero exit produced no output", () => { + const child = createMockChild(); + const reject = vi.fn(); + + setupChildProcessHandlers(child as never, "pi", null, reject, vi.fn()); + child.emit("close", 1); + + expect(reject).toHaveBeenCalledWith( + new Error("pi exited with code 1 and produced no output"), + ); + }); + it("wraps spawn errors and resolves successful exits through the success callback", () => { const child = createMockChild(); const reject = vi.fn(); diff --git a/src/core/agents/stream-utils.ts b/src/core/agents/stream-utils.ts index 22899469..acdf2e03 100644 --- a/src/core/agents/stream-utils.ts +++ b/src/core/agents/stream-utils.ts @@ -2,8 +2,105 @@ import type { ChildProcess } from "node:child_process"; import type { Readable } from "node:stream"; import type { WriteStream } from "node:fs"; +/** Upper bound on the stdout tail kept for non-zero-exit error reporting. */ +const MAX_EXIT_OUTPUT_CHARS = 4_000; /** - * Wire stderr collection, spawn-error handling, and the common close-handler + * Tighter bound on unstructured stdout quoted back in the failure detail: that + * text lands in notes.md and is replayed in every later iteration prompt. + */ +const MAX_RAW_TAIL_CHARS = 400; +const RAW_TAIL_ELISION = "[...truncated, full output in the iteration log] "; + +/** Keep only the end of a stream so long-running processes stay bounded. */ +export function appendExitOutputTail(existing: string, chunk: string): string { + const combined = existing + chunk; + return combined.length > MAX_EXIT_OUTPUT_CHARS + ? combined.slice(combined.length - MAX_EXIT_OUTPUT_CHARS) + : combined; +} + +function errorTextFromEvent(event: unknown): string | null { + if (!event || typeof event !== "object") return null; + const record = event as Record; + + const error = record.error; + if (typeof error === "string" && error.trim()) return error.trim(); + if (error && typeof error === "object") { + const message = (error as Record).message; + if (typeof message === "string" && message.trim()) return message.trim(); + } + + if (record.is_error === true || record.type === "error") { + for (const key of ["result", "message", "subtype"]) { + const value = record[key]; + if (typeof value === "string" && value.trim()) return value.trim(); + } + } + + return null; +} + +function elideRawTail(raw: string): string { + return raw.length > MAX_RAW_TAIL_CHARS + ? `${RAW_TAIL_ELISION}${raw.slice(raw.length - MAX_RAW_TAIL_CHARS)}` + : raw; +} + +interface StdoutFailure { + structured: string; + reported: string; +} + +function extractStdoutError(stdoutTail: string): StdoutFailure { + const messages: string[] = []; + for (const line of stdoutTail.split("\n")) { + if (!line.trim()) continue; + try { + const message = errorTextFromEvent(JSON.parse(line)); + if (message) messages.push(message); + } catch { + // Not JSON: covered by the raw-tail fallback below. + } + } + const structured = messages.join("\n"); + return { + structured, + reported: structured || elideRawTail(stdoutTail.trim()), + }; +} + +export interface ChildProcessExitFailure { + detail: string; + /** CLI-authored error text suitable for permanent-error classification. */ + errorOutput: string; +} + +/** + * Describe a non-zero exit. The detail reports both streams, while + * `errorOutput` excludes unstructured stdout that may merely quote an error. + */ +export function describeChildProcessExit( + agentName: string, + code: number | null, + stdoutTail: string, + stderr: string, +): ChildProcessExitFailure { + const trimmedStderr = stderr.trim(); + const stdoutError = extractStdoutError(stdoutTail); + const segments = [trimmedStderr, stdoutError.reported].filter(Boolean); + return { + detail: + segments.length > 0 + ? `${agentName} exited with code ${code}: ${segments.join("\n")}` + : `${agentName} exited with code ${code} and produced no output`, + errorOutput: [trimmedStderr, stdoutError.structured] + .filter(Boolean) + .join("\n"), + }; +} + +/** + * Wire output collection, spawn-error handling, and the common close-handler * prefix (logStream.end + non-zero exit code rejection) for a child process. * Calls `onSuccess` only when the process exits with code 0. */ @@ -15,6 +112,11 @@ export function setupChildProcessHandlers( onSuccess: () => void, ): void { let stderr = ""; + let stdoutTail = ""; + + child.stdout!.on("data", (data: Buffer) => { + stdoutTail = appendExitOutputTail(stdoutTail, data.toString()); + }); child.stderr!.on("data", (data: Buffer) => { stderr += data.toString(); @@ -27,7 +129,13 @@ export function setupChildProcessHandlers( child.on("close", (code) => { logStream?.end(); if (code !== 0) { - reject(new Error(`${agentName} exited with code ${code}: ${stderr}`)); + const failure = describeChildProcessExit( + agentName, + code, + stdoutTail, + stderr, + ); + reject(new Error(failure.detail)); return; } onSuccess();