diff --git a/agents/claude.ts b/agents/claude.ts index fb96af0..b0d6d07 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -22,7 +22,13 @@ import { formatJsonValue, log } from "../utils/cli.ts"; import { installFromNpmTarball } from "../utils/install.ts"; import { detectProviderError } from "../utils/providerErrors.ts"; import { addSkill, installBundledSkills } from "../utils/skills.ts"; -import { SPAWN_ACTIVITY_TIMEOUT_CODE, SpawnTimeoutError, spawn } from "../utils/subprocess.ts"; +import { + DEFAULT_MAX_RETAINED_BYTES, + SPAWN_ACTIVITY_TIMEOUT_CODE, + SpawnTimeoutError, + spawn, + TailBuffer, +} from "../utils/subprocess.ts"; import { ThinkingTimer } from "../utils/timer.ts"; import type { TodoTracker } from "../utils/todoTracking.ts"; import { getDevDependencyVersion } from "../utils/version.ts"; @@ -536,7 +542,8 @@ export async function runClaude(params: RunParams): Promise { let lastProviderError: string | null = null; - let output = ""; + // capped accumulator — see opencode.ts for rationale (issue #680). + const output = new TailBuffer(DEFAULT_MAX_RETAINED_BYTES); let stdoutBuffer = ""; try { @@ -554,9 +561,14 @@ export async function runClaude(params: RunParams): Promise { // there's no shim-orphan issue like opencode-ai/bin/opencode, but // detached + killGroup is the right default for any agent runtime. killGroup: true, + // claude already drains every chunk via onStdout (NDJSON parsing) and + // onStderr (recentStderr ring buffer). retaining a second copy in the + // spawn wrapper would grow unbounded for long sessions and previously + // crashed the wrapper with RangeError. see issue #680. + retain: "none", onStdout: async (chunk) => { const text = chunk.toString(); - output += text; + output.append(text); markActivity(); stdoutBuffer += text; @@ -666,20 +678,26 @@ export async function runClaude(params: RunParams): Promise { // hides the actionable provider message and pollutes the run log. cap // the stdout fallback to the last 2KB so it stays readable when neither // a structured error nor stderr is available. - const truncatedStdout = result.stdout ? tailLines(result.stdout, 2048) : ""; + // + // result.stdout / result.stderr are empty because we pass retain:"none" + // to spawn (see issue #680); the agent layer keeps its own bounded + // mirrors via `output` (TailBuffer) and `recentStderr` (ring buffer). + const stdoutSnapshot = output.toString(); + const stderrSnapshot = recentStderr.join("\n"); + const truncatedStdout = stdoutSnapshot ? tailLines(stdoutSnapshot, 2048) : ""; const errorMessage = lastResultError || - result.stderr || + stderrSnapshot || truncatedStdout || `unknown error - no output from Claude CLI${errorContext}`; log.error( `${params.label} exited with code ${result.exitCode}${errorContext}: ${errorMessage}` ); - log.debug(`stdout: ${result.stdout?.substring(0, 500)}`); - log.debug(`stderr: ${result.stderr?.substring(0, 500)}`); + log.debug(`stdout: ${stdoutSnapshot.substring(0, 500)}`); + log.debug(`stderr: ${stderrSnapshot.substring(0, 500)}`); return { success: false, - output: finalOutput || output, + output: finalOutput || stdoutSnapshot, error: errorMessage, usage, sessionId, @@ -689,7 +707,7 @@ export async function runClaude(params: RunParams): Promise { if (eventCount === 0 && lastProviderError) { return { success: false, - output: finalOutput || output, + output: finalOutput || output.toString(), error: `provider error: ${lastProviderError}`, usage, sessionId, @@ -699,14 +717,14 @@ export async function runClaude(params: RunParams): Promise { if (resultErrorSubtype) { return { success: false, - output: finalOutput || output, + output: finalOutput || output.toString(), error: lastResultError || `result subtype: ${resultErrorSubtype}`, usage, sessionId, }; } - return { success: true, output: finalOutput || output, usage, sessionId }; + return { success: true, output: finalOutput || output.toString(), usage, sessionId }; } catch (error) { params.todoTracker?.cancel(); const duration = performance.now() - startTime; @@ -732,7 +750,7 @@ export async function runClaude(params: RunParams): Promise { return { success: false, - output: finalOutput || output, + output: finalOutput || output.toString(), error: `${errorMessage} [${diagnosis}]`, usage: buildUsage(), sessionId, diff --git a/agents/opencode.ts b/agents/opencode.ts index fadb42f..abce0bf 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -22,7 +22,13 @@ import { formatJsonValue, log } from "../utils/cli.ts"; import { installFromNpmTarball } from "../utils/install.ts"; import { detectProviderError } from "../utils/providerErrors.ts"; import { addSkill, installBundledSkills } from "../utils/skills.ts"; -import { SPAWN_ACTIVITY_TIMEOUT_CODE, SpawnTimeoutError, spawn } from "../utils/subprocess.ts"; +import { + DEFAULT_MAX_RETAINED_BYTES, + SPAWN_ACTIVITY_TIMEOUT_CODE, + SpawnTimeoutError, + spawn, + TailBuffer, +} from "../utils/subprocess.ts"; import { ThinkingTimer } from "../utils/timer.ts"; import type { TodoTracker } from "../utils/todoTracking.ts"; import { getDevDependencyVersion } from "../utils/version.ts"; @@ -888,7 +894,12 @@ async function runOpenCode(params: RunParams): Promise { let lastProviderError: string | null = null; let agentErrorEvent: OpenCodeErrorEvent | null = null; - let output = ""; + // capped accumulator for the agent's narration. used as a post-run fallback + // when `finalOutput` (the orchestrator's final assistant message) is empty. + // unbounded `output += text` previously grew to ~1 GiB on multi-lens Reviews + // and contributed to the wrapper-level RangeError. retain:"none" on spawn + // skips the duplicate buffer there; this TailBuffer caps the agent layer. + const output = new TailBuffer(DEFAULT_MAX_RETAINED_BYTES); let stdoutBuffer = ""; try { @@ -907,6 +918,11 @@ async function runOpenCode(params: RunParams): Promise { // never fires — producing zombie runs. detached + killGroup nukes the // whole tree. killGroup: true, + // we already drain every chunk via onStdout/onStderr (NDJSON parsing + // + recentStderr ring buffer). retaining a second copy in the spawn + // wrapper would grow unbounded for multi-lens Reviews and previously + // crashed the wrapper with RangeError at ~1 GiB. see issue #680. + retain: "none", // NB: we used to pass `isPausedExternally: isSubagentInFlight` to suspend // the activity timer during subagent dispatches. unnecessary now that // our injected plugin (action/agents/opencodePlugin.ts) re-emits @@ -916,7 +932,7 @@ async function runOpenCode(params: RunParams): Promise { // (~3.3 plugin events/sec during a typical subagent run). onStdout: async (chunk) => { const text = chunk.toString(); - output += text; + output.append(text); markActivity(); stdoutBuffer += text; @@ -1041,22 +1057,31 @@ async function runOpenCode(params: RunParams): Promise { if (result.exitCode !== 0) { const errorContext = lastProviderError ? ` (${lastProviderError})` : ""; + // result.stdout / result.stderr are empty because we pass retain:"none" + // to spawn (see issue #680); use the agent's bounded mirrors instead. + const stdoutSnapshot = output.toString(); + const stderrSnapshot = recentStderr.join("\n"); const errorMessage = - result.stderr || - result.stdout || + stderrSnapshot || + stdoutSnapshot || `unknown error - no output from OpenCode CLI${errorContext}`; log.error( `${params.label} exited with code ${result.exitCode}${errorContext}: ${errorMessage}` ); - log.debug(`stdout: ${result.stdout?.substring(0, 500)}`); - log.debug(`stderr: ${result.stderr?.substring(0, 500)}`); - return { success: false, output: finalOutput || output, error: errorMessage, usage }; + log.debug(`stdout: ${stdoutSnapshot.substring(0, 500)}`); + log.debug(`stderr: ${stderrSnapshot.substring(0, 500)}`); + return { + success: false, + output: finalOutput || stdoutSnapshot, + error: errorMessage, + usage, + }; } if (eventCount === 0 && lastProviderError) { return { success: false, - output: finalOutput || output, + output: finalOutput || output.toString(), error: `provider error: ${lastProviderError}`, usage, }; @@ -1069,13 +1094,13 @@ async function runOpenCode(params: RunParams): Promise { errorEvent.error?.data?.message || errorEvent.error?.name || JSON.stringify(errorEvent); return { success: false, - output: finalOutput || output, + output: finalOutput || output.toString(), error: `${errorName}: ${errorMessage}`, usage, }; } - return { success: true, output: finalOutput || output, usage }; + return { success: true, output: finalOutput || output.toString(), usage }; } catch (error) { params.todoTracker?.cancel(); const duration = performance.now() - startTime; @@ -1101,7 +1126,7 @@ async function runOpenCode(params: RunParams): Promise { return { success: false, - output: finalOutput || output, + output: finalOutput || output.toString(), error: `${errorMessage} [${diagnosis}]`, usage: buildUsage(), }; diff --git a/utils/subprocess.test.ts b/utils/subprocess.test.ts index f490b23..e409903 100644 --- a/utils/subprocess.test.ts +++ b/utils/subprocess.test.ts @@ -1,6 +1,6 @@ import { performance } from "node:perf_hooks"; import { describe, expect, it } from "vitest"; -import { spawn } from "./subprocess.ts"; +import { spawn, TailBuffer } from "./subprocess.ts"; describe("spawn error path", () => { it("surfaces ENOENT-style spawn failures in stderr so callers can diagnose", async () => { @@ -79,6 +79,76 @@ describe("spawn error path", () => { expect(elapsed).toBeLessThan(10_000); }, 20_000); + it('retain:"tail" caps stderr at maxRetainedBytes and prepends a truncation sentinel', async () => { + // regression for issue #680: unbounded `stderrBuffer += chunk` previously + // crashed the wrapper with `RangeError: Invalid string length` once V8's + // ~1 GiB kMaxLength was breached on long-lived agent runs. the fix caps + // retention with a TailBuffer; this test exercises the cap end-to-end by + // emitting ~2 MiB of stderr against a 256 KiB ceiling and asserts the + // wrapper does not crash, the result is bounded, and the sentinel is + // present so downstream consumers can detect the truncation. + const result = await spawn({ + cmd: "bash", + // print ~2 MiB to stderr in 64 KiB chunks. `yes` + head gives us a + // reliable byte budget that's well above the 256 KiB cap below. + args: ["-c", "yes ABCDEFGH | head -c 2097152 1>&2"], + env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, + activityTimeout: 0, + maxRetainedBytes: 256 * 1024, + }); + + expect(result.exitCode).toBe(0); + expect(result.stderr).toMatch(/truncated by retain:tail cap/); + expect(result.stderr.length).toBeLessThan(256 * 1024 + 200); + }, 15_000); + + it('retain:"none" returns empty stdout/stderr regardless of child output', async () => { + // long-lived agent callers (opencode, claude) drain via onStdout/onStderr + // and never read result.stdout/result.stderr — they pass retain:"none" + // to skip the per-chunk concatenation entirely. assert that contract: + // empty strings out, but onStdout still fires. + const chunks: string[] = []; + const result = await spawn({ + cmd: "bash", + args: ["-c", "echo hello; echo world 1>&2"], + env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, + activityTimeout: 0, + retain: "none", + onStdout: (chunk) => chunks.push(chunk), + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toBe(""); + expect(result.stderr).toBe(""); + expect(chunks.join("")).toContain("hello"); + }); + + it('retain defaults to "tail" so short-lived callers keep failure-surfacing snapshots', async () => { + // lock the default explicitly. gitAuth, package installs, and lifecycle + // hooks all rely on `result.stderr` being non-empty on failure — flipping + // the default to "none" would silently break their error messages while + // all other tests in this file kept passing. + const result = await spawn({ + cmd: "bash", + args: ["-c", "echo -n diagnostic-output 1>&2; exit 7"], + env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" }, + activityTimeout: 0, + }); + + expect(result.exitCode).toBe(7); + expect(result.stderr).toBe("diagnostic-output"); + }); + + it("TailBuffer drops oldest bytes once the cap is exceeded", () => { + const buf = new TailBuffer(10); + buf.append("0123456789"); + expect(buf.toString()).toBe("0123456789"); + buf.append("abcde"); + // 0-9 plus abcde = 15 chars; cap is 10, so we keep the last 10 = "56789abcde" + expect(buf.toString()).toMatch(/truncated by retain:tail cap/); + expect(buf.toString()).toContain("56789abcde"); + }); + it("reports signal-killed subprocesses as failures, not success", async () => { // regression: before the fix, `child.on("close", (exitCode) => ...)` // discarded the signal parameter and `exitCode || 0` coerced the diff --git a/utils/subprocess.ts b/utils/subprocess.ts index c84314f..d1e6294 100644 --- a/utils/subprocess.ts +++ b/utils/subprocess.ts @@ -88,6 +88,29 @@ function installSignalHandler(): void { }); } +/** + * Controls what the wrapper retains in memory across the child's lifetime + * for the post-hoc `SpawnResult.stdout` / `SpawnResult.stderr` snapshots. + * + * Streaming callbacks (`onStdout` / `onStderr`) fire regardless — `retain` + * only governs the buffered snapshot returned in `SpawnResult`. + * + * - `"tail"` (default): keep the last `maxRetainedBytes` UTF-16 code units + * of each stream. Once the cap is exceeded, oldest bytes are sliced off + * and the result is prefixed with a `... [N MiB truncated] ...` sentinel. + * Right default for short-lived commands whose failure mode is in their + * final output (git errors, install failures, hook scripts). + * - `"none"`: skip the buffer entirely. `SpawnResult.stdout` / `.stderr` + * are empty strings. Use this for long-lived streaming agents that already + * drain via `onStdout` / `onStderr` and never read the buffered snapshot. + * + * Default cap is 8 MiB — well below V8's ~1 GiB `kMaxLength` so `+= chunk` + * can never throw `RangeError: Invalid string length`. + */ +export type RetainMode = "tail" | "none"; + +export const DEFAULT_MAX_RETAINED_BYTES = 8 * 1024 * 1024; + export interface SpawnOptions { cmd: string; args: string[]; @@ -115,6 +138,46 @@ export interface SpawnOptions { // open, keeps emitting NDJSON, and `child.on("close")` never fires — // producing zombie runs that hang until the GitHub Actions job timeout). killGroup?: boolean; + retain?: RetainMode; + maxRetainedBytes?: number; +} + +/** + * Bounded string accumulator that keeps the tail of appended chunks. + * Once the cap is exceeded, oldest bytes are sliced off and `toString()` + * prefixes the survivors with a sentinel describing the elided byte count. + * + * Exported because long-lived agent runtimes (opencode, claude) also + * accumulate per-run narration strings independently of the spawn wrapper + * and need the same protection against V8's `kMaxLength`. + */ +export class TailBuffer { + // explicit field declarations rather than constructor parameter properties: + // node's strip-only TS loader (used by action/test/run.ts in CI) rejects + // `constructor(private readonly cap: number)` with ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX. + private readonly cap: number; + private buffer = ""; + private truncatedBytes = 0; + + constructor(cap: number) { + this.cap = cap; + } + + append(chunk: string): void { + if (this.cap <= 0) return; + this.buffer += chunk; + if (this.buffer.length > this.cap) { + const drop = this.buffer.length - this.cap; + this.truncatedBytes += drop; + this.buffer = this.buffer.slice(drop); + } + } + + toString(): string { + if (this.truncatedBytes === 0) return this.buffer; + const mib = (this.truncatedBytes / 1024 / 1024).toFixed(1); + return `... [${mib} MiB truncated by retain:tail cap] ...\n${this.buffer}`; + } } export interface SpawnResult { @@ -133,8 +196,15 @@ export async function spawn(options: SpawnOptions): Promise { installSignalHandler(); const startTime = performance.now(); - let stdoutBuffer = ""; - let stderrBuffer = ""; + // capped accumulators — unbounded `+= chunk` previously crashed the wrapper + // with `RangeError: Invalid string length` once V8's ~1 GiB kMaxLength was + // breached on long-lived agent subprocesses (e.g. multi-lens opencode + // Reviews on large monorepos). retain:"none" skips the buffer entirely + // for callers that already drain via onStdout/onStderr. + const retain: RetainMode = options.retain ?? "tail"; + const cap = options.maxRetainedBytes ?? DEFAULT_MAX_RETAINED_BYTES; + const stdoutBuffer = retain === "none" ? null : new TailBuffer(cap); + const stderrBuffer = retain === "none" ? null : new TailBuffer(cap); const killGroup = options.killGroup ?? false; @@ -234,20 +304,46 @@ export async function spawn(options: SpawnOptions): Promise { lastActivityTime = performance.now(); } + // wrap handlers in try/catch as defense in depth for synchronous throws + // inside the listener body. the historical `+= chunk` RangeError was such + // a throw — synchronous and fatal under node's default uncaught-exception + // policy. with the TailBuffer cap in place the wrapper-side `append` can + // no longer throw, but the catch keeps protecting against any future + // synchronous regression in this path. + // + // note: this does NOT catch rejections from async user callbacks — + // `options.onStdout?.(chunk)` returns a Promise in the agent callers + // (claude.ts, opencode.ts) and a throw inside an async callback surfaces + // as an unhandled Promise rejection, not a synchronous exception. agent + // callers handle their own NDJSON-parse failures internally; the + // synchronous protection here is what matters for the RangeError class + // of bugs (issue #680). if (child.stdout) { child.stdout.on("data", (data: Buffer) => { - updateActivity(); - const chunk = data.toString(); - stdoutBuffer += chunk; - options.onStdout?.(chunk); + try { + updateActivity(); + const chunk = data.toString(); + stdoutBuffer?.append(chunk); + options.onStdout?.(chunk); + } catch (err) { + log.debug( + `spawn stdout handler threw: ${err instanceof Error ? err.message : String(err)}` + ); + } }); } if (child.stderr) { child.stderr.on("data", (data: Buffer) => { - const chunk = data.toString(); - stderrBuffer += chunk; - options.onStderr?.(chunk); + try { + const chunk = data.toString(); + stderrBuffer?.append(chunk); + options.onStderr?.(chunk); + } catch (err) { + log.debug( + `spawn stderr handler threw: ${err instanceof Error ? err.message : String(err)}` + ); + } }); } @@ -289,7 +385,7 @@ export async function spawn(options: SpawnOptions): Promise { // appeared to succeed when they'd actually been killed — caller // checked `result.exitCode !== 0` and moved on. let resolvedExitCode = exitCode ?? 0; - let resolvedStderr = stderrBuffer; + let resolvedStderr = stderrBuffer?.toString() ?? ""; if (exitCode === null && signal) { const killMsg = `[spawn] ${options.cmd}: killed by signal ${signal}`; resolvedStderr = resolvedStderr ? `${resolvedStderr}\n${killMsg}` : killMsg; @@ -297,7 +393,7 @@ export async function spawn(options: SpawnOptions): Promise { } resolve({ - stdout: stdoutBuffer, + stdout: stdoutBuffer?.toString() ?? "", stderr: resolvedStderr, exitCode: resolvedExitCode, durationMs, @@ -319,11 +415,12 @@ export async function spawn(options: SpawnOptions): Promise { // per the guidance, and hit the same wall every run. const errMsg = `[spawn] ${options.cmd}: ${error.message}`; console.error(errMsg); - stderrBuffer = stderrBuffer ? `${stderrBuffer}\n${errMsg}` : errMsg; + const existingStderr = stderrBuffer?.toString() ?? ""; + const finalStderr = existingStderr ? `${existingStderr}\n${errMsg}` : errMsg; resolve({ - stdout: stdoutBuffer, - stderr: stderrBuffer, + stdout: stdoutBuffer?.toString() ?? "", + stderr: finalStderr, exitCode: 1, durationMs, });