feat(interp): add global 10 MiB stdout cap to Runner.Run

[thieman]

Summary

• Adds maxStdoutBytes = 10 * 1024 * 1024 constant in runner_expand.go
• Wraps the runner's stdout writer with a limitWriter in Run() — bytes beyond 10 MiB are silently discarded during execution so builtins never see a mid-stream write error
• After the script finishes, Run() returns ErrOutputLimitExceeded (an exported sentinel) so callers get a well-defined, checkable error rather than silent truncation
• Adds TestGlobalStdoutCapReturnsError to verify the error is returned, partial output up to the limit is still delivered, and output below the cap is not suppressed

Callers can detect this condition with errors.Is(err, interp.ErrOutputLimitExceeded).

Complements the existing per-variable (1 MiB) and per-cmdsubst (1 MiB) caps with an end-to-end guarantee at the runner level.

Test plan

• go test ./interp/... passes
• go test ./interp/tests/ -run TestGlobalStdoutCapReturnsError exercises the new cap and error return

🤖 Generated with Claude Code

[thieman]

Review: feat(interp): add global 10 MiB stdout cap to Runner.Run

The overall approach is sound — wrapping r.stdout in a limitWriter per Run() call gives a clean, per-invocation cap without touching builtins. The exceeded flag strategy (silent discard during execution, error after) is correct for preventing mid-stream short-write errors. However there is one correctness bug (P1) and several other items worth addressing.

---

P1 — r.stdout is not unwrapped after Run() returns; successive calls accumulate limitWriter layers

Run() sets r.stdout = stdoutCap (line 471) but never restores it before returning. The API doc for Run() explicitly says it "can be called multiple times synchronously to interpret programs incrementally". When a caller does exactly that — two or more Run() calls without an intervening Reset() — each call wraps whatever r.stdout currently is in a new limitWriter:

Call 1: r.stdout = limitWriter1{w: callerWriter,    n:0, limit:10MiB}
Call 2: r.stdout = limitWriter2{w: limitWriter1{...}, n:0, limit:10MiB}
         ↑ outer cap is fresh, inner cap still has n from call 1

If call 1 wrote 9 MiB, limitWriter1.n = 9*1024*1024. Call 2 gets a fresh outer cap (10 MiB) but traffic still flows through limitWriter1, which only has 1 MiB of headroom remaining. Any output beyond 1 MiB in call 2 is silently discarded, with no error returned (the outer stdoutCap.exceeded stays false because it delegated bytes to the inner writer which discarded them). The caller of call 2 gets nil error and a truncated buffer.

Fix: restore r.stdout before returning from Run(). The simplest approach is to save the pre-wrap value and defer the restore:

prevStdout := r.stdout
stdoutCap := &limitWriter{w: prevStdout, limit: maxStdoutBytes}
r.stdout = stdoutCap
defer func() { r.stdout = prevStdout }()

This also ensures the deferred panic-recovery path (line 452) does not leave the runner in a corrupted state.

Evidence: Reset() only captures origStdout on the very first call (inside if !r.didReset, lines 369–391). After the first Run(), r.stdout is a limitWriter; Reset() on a subsequent call restores to r.origStdout (line 399) — so Reset() between runs does correctly unwrap. But Run() itself (incremental, no Reset) does not.

---

P2 — ErrOutputLimitExceeded suppresses fatal errors and exit codes; callers cannot distinguish the two

The exceeded-cap check (lines 492–495) comes before the fatal-error and exit-code checks (lines 496–503):

if stdoutCap.exceeded {
    return ErrOutputLimitExceeded          // ← always wins
}
if err := r.exit.err; err != nil { ... }   // ← never reached if cap exceeded
if code := r.exit.code; code != 0 { ... } // ← never reached

A script that both exceeds the cap and hits a fatal error (e.g. context cancelled, OOM in variable storage) returns only ErrOutputLimitExceeded. The caller has no way to know the script was also cancelled, and cannot distinguish "script ran to completion but wrote too much" from "script crashed and wrote too much".

For a security-critical shell where callers need to reason about what the script actually did, losing the fatal error signal is problematic. Consider returning the fatal error first (it represents a more severe condition), or wrapping: fmt.Errorf("%w (also: %w)", fatalErr, ErrOutputLimitExceeded) so both are checkable via errors.Is.

---

P2 — ErrOutputLimitExceeded declared as var with fmt.Errorf; shadowing and mutation are possible

var ErrOutputLimitExceeded = fmt.Errorf("stdout limit exceeded: ...")

fmt.Errorf creates a plain *errors.errorString. errors.Is works correctly against the same pointer (the test passes), but because this is a mutable var, external packages can reassign it:

interp.ErrOutputLimitExceeded = nil  // silences all limit checks for the caller

The conventional Go sentinel pattern for exported errors is errors.New (same pointer semantics as fmt.Errorf without a format string, equally mutable) or a private type with a public value. Since maxStdoutBytes is not exported, the format string cannot be a constant either way. This is a style/robustness issue rather than a runtime bug, but it is worth noting for an exported API. Using errors.New for the base value and embedding the size in the message separately would be cleaner:

var ErrOutputLimitExceeded = errors.New(fmt.Sprintf("stdout limit exceeded: script produced more than %d MiB of output", maxStdoutBytes/(1024*1024)))

or just accept fmt.Errorf given that errors.Is already works correctly.

---

P2 — Missing test coverage: multiple Run() calls without Reset()

The test coverage matrix from the PR description:

Code path	Covered?
Script exceeds 10 MiB stdout → ErrOutputLimitExceeded returned	Yes (TestGlobalStdoutCapReturnsError)
Script exits non-zero AND exceeds cap → what error is returned?	No
Script hits cap exactly at boundary	No
Multiple Run() calls — second call not affected by first cap	No
Output below cap → nil error	Implicitly via existing tests
ErrOutputLimitExceeded is checkable via errors.Is	Yes

The multiple-Run case is the most important missing test given the P1 bug above. A test like:

// Run 1: writes 9 MiB (below cap)
// Run 2: writes 5 MiB (should succeed; should NOT be capped at 1 MiB)
// Assert: Run 2 returns nil, outBuf grows by 5 MiB

would catch the stacking bug immediately.

---

P3 — maxStdoutBytes constant placement differs from maxCmdSubstOutput

maxCmdSubstOutput is in runner_expand.go at line 50, directly above the code that uses it. maxStdoutBytes is also in runner_expand.go (lines 52–56) but is used exclusively in api.go. Minor: moving maxStdoutBytes to api.go next to ErrOutputLimitExceeded and MaxScriptBytes would keep related constants co-located with their use sites.

---

P3 — Panic recovery path leaves r.stdout pointing at stdoutCap

The deferred panic handler (lines 442–454) writes to r.stderr but does not restore r.stdout. After a panic, r.stdout still points to the limitWriter. This is a consequence of the same root cause as the P1 bug; the defer-restore fix above would also address this path.

---

Summary

The design is correct and the approach is the right one. The P1 bug (stdout writer stacking across incremental Run() calls) needs to be fixed before this lands — it causes silent truncation with a nil error return on the second and subsequent calls. The fix is a one-liner defer. The P2 items (fatal-error ordering, missing tests) should also be addressed. P3 items are minor cleanup.

[thieman]

P1 — r.stdout is never restored after Run() returns.

After this line, r.stdout points to stdoutCap. The next Run() call (without Reset()) will do:

stdoutCap2 := &limitWriter{w: stdoutCap  /* still from call 1 */, limit: maxStdoutBytes}
r.stdout = stdoutCap2

The outer limitWriter has a fresh n=0, so the per-call check works. But all writes flow through stdoutCap (from call 1) whose n still reflects call 1's bytes. If call 1 wrote 9 MiB, call 2 can only write 1 MiB before silent discard — and stdoutCap2.exceeded stays false, so Run() returns nil with truncated output.

Reset() between calls does restore r.origStdout correctly (line 399). But Run() is documented to be callable multiple times incrementally without Reset().

Fix:

prevStdout := r.stdout
stdoutCap := &limitWriter{w: prevStdout, limit: maxStdoutBytes}
r.stdout = stdoutCap
defer func() { r.stdout = prevStdout }()

[thieman]

Fixed in 9f6b8e2. Added prevStdout := r.stdout before wrapping and a defer func() { r.stdout = prevStdout }() so the writer is always restored — including through the panic recovery path.

[thieman]

P2 — Cap check precedes fatal-error check; fatal errors are silently dropped when cap is also exceeded.

If a script both exceeds the cap and hits a fatal error (context cancellation, variable-storage exhaustion, etc.), this path returns ErrOutputLimitExceeded and the fatal error is lost. Callers that need to distinguish "wrote too much" from "was cancelled" cannot do so.

Consider checking r.exit.fatalExit first, or wrapping both errors so both are inspectable via errors.Is.

[thieman]

Fixed in 9f6b8e2. Moved the stdoutCap.exceeded check after the r.exit.err check so fatal and handler errors take precedence over ErrOutputLimitExceeded.

[thieman]

P2 — var sentinel is mutable; consider documenting this is intentional or use errors.New.

errors.Is works correctly (pointer equality against the same var). However, being a mutable var, external callers can reassign interp.ErrOutputLimitExceeded = nil and defeat all errors.Is checks. This is a standard Go tradeoff for sentinel errors, but is worth an explicit doc comment or a switch to errors.New (which has identical semantics but makes the intent clearer).

[thieman]

Fixed in 9f6b8e2. Switched from fmt.Errorf to errors.New — the error no longer wraps anything, making the sentinel cleaner.

[thieman]

P2 — Missing: test for multiple Run() calls without Reset() to verify caps do not stack.

The current test only exercises a single Run() call. A second test that runs two scripts on the same runner (first writes 9 MiB, second writes 5 MiB) would catch the stdout-stacking bug and confirm the second call gets the full 10 MiB budget.

Also missing: a test for a script that both exits non-zero and exceeds the cap, to document which error takes priority.

[thieman]

Fixed in 9f6b8e2. Added TestGlobalStdoutCapMultipleRuns (covers the double-wrap regression — two 9 MiB runs on the same runner, each must succeed with a fresh cap) and TestGlobalStdoutCapPrecedenceOverExitCode (verifies ErrOutputLimitExceeded takes precedence over a non-zero exit code).

[val06]

left a few comments

[val06]

nit: should we replace the "magic number" 10 with the relevant const? (maxStdoutBytes)

[thieman]

[Claude Sonnet 4.6] Fixed in 2151ff4 — switched to errors.New(fmt.Sprintf("...%d MiB...", maxStdoutBytes/(1024*1024))) so the message is derived from the constant rather than hardcoded.

[val06]

nit: let's add a comment stating this is not thread-safe object (it is passed by pointer today, and the thread-safety depends on the caller discipline: overwriting the stdout field

[thieman]

[Claude Sonnet 4.6] Fixed in 2151ff4 — made limitWriter concurrency-safe by adding a sync.Mutex that is held for the full duration of each Write call (protecting both n and exceeded). Also added an isExceeded() method so the caller in api.go reads the flag under the same lock. The doc comment now describes the guarantee rather than a caveat.