fix(action): cap subprocess stdout/stderr retention to prevent RangeError crashes (#680) (#715)

* fix(action): cap subprocess stdout/stderr retention to prevent RangeError crashes (#680)

unbounded `stdoutBuffer += chunk` / `stderrBuffer += chunk` in
`action/utils/subprocess.ts` previously crashed the wrapper with
`RangeError: Invalid string length` once V8's ~1 GiB kMaxLength was
breached on long-lived agent runs. multi-lens opencode Reviews on large
monorepos (e.g. tambo-ai/buildy) hit this consistently — 23 runs in the
last 24h, 100% of Review-mode hard failures on that repo.

- add `retain: "tail" | "none"` to SpawnOptions, defaulting to "tail"
  with an 8 MiB cap. tail-mode prepends a `... [N MiB truncated] ...`
  sentinel so downstream consumers can detect truncation.
- export `TailBuffer` helper for callers that need the same bounded
  accumulator semantics at their own layer.
- wrap stream `data` listeners in try/catch as defense in depth — any
  synchronous throw inside a stream handler is otherwise fatal.
- opencode + claude pass `retain: "none"` (they drain via onStdout /
  onStderr) and switch their own `output` accumulators to TailBuffer.
  their error paths read the agent-layer bounded mirrors instead of
  the now-empty `result.stdout` / `result.stderr`.
- add `failure:string-length-overflow` heuristic to scripts/analyze-logs.ts
  so post-fix recurrences are visible at a glance instead of bucketing
  into `failure:unknown`.
- regression tests cover >1 MiB stderr without crash, retain:"none"
  contract, and TailBuffer truncation semantics.

* fix: avoid TS parameter property syntax in TailBuffer for strip-only node loader

* address review: clarify try/catch scope + lock retain default to "tail"

- the original comment claimed the try/catch caught "any synchronous throw"
  in the data listener, but `options.onStdout?.(chunk)` returns a Promise
  in the agent callers (claude.ts:569, opencode.ts:933) — a throw inside
  an async user callback surfaces as an unhandled Promise rejection, not
  a synchronous exception. reword to describe the actual protection:
  defense-in-depth for synchronous throws in the listener body, which is
  exactly the shape of the original RangeError on `+= chunk`.
- add a test that locks `retain` default to "tail" by spawning without
  the option and asserting `result.stderr` is non-empty. a future refactor
  that flipped the default to "none" would silently break gitAuth,
  package installs, and lifecycle hooks that read result.stderr for
  failure messages, and the rest of the suite wouldn't catch it.

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

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<SpawnResult> {
   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<SpawnResult> {
       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<SpawnResult> {
       // 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<SpawnResult> {
       }
 
       resolve({
-        stdout: stdoutBuffer,
+        stdout: stdoutBuffer?.toString() ?? "",
         stderr: resolvedStderr,
         exitCode: resolvedExitCode,
         durationMs,
@@ -319,11 +415,12 @@ export async function spawn(options: SpawnOptions): Promise<SpawnResult> {
       // 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,
       });