fix(interp): add 1 MiB total variable cap and fix stdin goroutine context cancellation

[thieman]

Problem

M-5: No cap on total number of shell variables

interp/vars.go caps individual variable values at MaxVarBytes (1 MiB each), but placed no limit on the number of distinct variables. A script like:

for i in $(seq 1 100000); do eval "var_$i=x"; done

...could create 100,000 variables. Even at 1 byte each, that's 100,000 map entries with non-trivial overhead. At larger values, total memory use grows proportionally.

M-6: Stdin-copy goroutine ignores context cancellation

interp/api.go's stdinFile function spawned a goroutine calling io.Copy(pw, r) without a context. If the caller cancels the context but the reader provides an unbounded stream, this goroutine runs indefinitely after cancellation.

Fix

M-5: Added MaxTotalVarsBytes = 1 MiB — the sum of all variable value sizes cannot exceed 1 MiB total. Deltas are tracked on set/unset so the check is O(1). The existing per-value MaxVarBytes cap remains in place. System-init variables (PWD, IFS, OPTIND set during Reset()) are excluded from the cap by resetting totalBytes to 0 after they are written, so a single 1 MiB user variable still works correctly alongside the interpreter's own bookkeeping variables. Exceeding the total cap sets exiting = true to abort the script (same as exit 1 semantics).

M-6: Replaced io.Copy with a context-aware 32 KiB read/write loop that checks ctx.Err() at the top of each iteration. stdinFile now accepts a context.Context and both callers (StdIO option using context.Background(), and runner_redir.go using the per-execution ctx) pass their respective contexts.

Test plan

• Setting many small variables (~1100 × 1024 bytes) eventually hits the 1 MiB total cap with a clear error
• Updating an existing variable correctly adjusts the total byte counter (update/shrink cycle allows a second large variable)
• System variables (PWD, IFS, OPTIND) do not count against the cap
• Single 1 MiB variable still works (existing TestCmdSubstOutputCapped passes)
• Existing TestOversizedInlineVarAbortsCommand continues to pass
• All interp/... and tests/... scenario tests pass

🤖 Generated with Claude Code

[thieman]

Code Review — PR #153: Shell Variable Limits

Overview

This PR adds two safety features:

1. Total variable storage cap (MaxTotalVarsBytes = 1 MiB): Prevents scripts from accumulating many small variables to exhaust memory.
2. Context-aware stdin copy goroutine: stdinFile now takes a context.Context so the copy goroutine can stop when the context is cancelled.

Overall Assessment: Needs fixes (two P2 issues, one P3)

#	Priority	File	Finding
1		interp/api.go:296	StdIO passes context.Background(), defeating context cancellation for stdin goroutine
2		interp/vars.go:45-49	Non-background subshells start with totalBytes=0, allowing cap bypass via inherited parent variables
3		interp/var_size_test.go:35	Missing scenario tests for total variable storage cap behavior

Positive Observations

• The errTotalVarStorageExceeded distinct error type enabling targeted exiting=true treatment in setVar is excellent design.
• Reset() correctly zeroes totalBytes after writing PWD/IFS/OPTIND so system vars don't count against the user script cap.
• Background subshells correctly initialize totalBytes from the full parent env snapshot.
• The context-propagation fix in runner_redir.go is correct and important.
• Fixed-size 32KB buffer in the goroutine avoids unbounded allocation from io.Copy's internal buffer growth.

[thieman]

@codex review this PR

[thieman]

Iteration 1 self-review result:

• P0: 0
• P1: 0
• P2: 3 findings
• P3: 0 findings

Brief summary: The PR correctly adds a total variable storage cap (1 MiB) and context-aware stdin goroutines. Three P2 issues found: (1) StdIO passes context.Background() defeating the context-cancellation fix for stdin goroutines, (2) non-background subshells start totalBytes=0 allowing the cap to be bypassed via inherited parent variables, (3) missing scenario tests for the cap behavior.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a891ceb10f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[val06]

Bug: Each() enumerates overridden variables twice, inflating the seed

parent.Each() calls the parent's Each() recursively and then iterates its own values. If a variable was overridden in the parent overlay, it appears in both passes — once from r.Env and once from the overlay — so totalBytes is seeded at 2× the real storage. Any assignment in the subshell then hits the cap even though actual memory use is within MaxTotalVarsBytes.

Failing test demonstrating this: https://github.com/DataDog/rshell/tree/valeri.pliskin/subshell-double-count-test

[thieman]

@val06 Fixed in f8a8473. The fix reorders Each() to emit own values first, then skips parent keys already in the overlay — so overridden variables are emitted exactly once. Brought your test in as a regression test (credited in the commit). Thanks for the catch!

[thieman]

Review Summary

Reviewed PR #153: fix(interp): add 1 MiB total variable cap and fix stdin goroutine context cancellation

Overall assessment: needs fixes — the core approach is sound, but there is a correctness bug in the totalBytes accounting for non-background subshells that overrides inherited parent variables.

What was reviewed

• interp/vars.go — overlayEnviron totalBytes tracking, Set(), Each(), newOverlayEnviron()
• interp/api.go — stdinFile() context-aware goroutine, Reset() totalBytes zeroing
• interp/runner_redir.go — context pass-through to stdinFile()
• interp/var_size_test.go — new Go tests
• tests/scenarios/shell/var_expand/basic/*.yaml — new scenario tests
• allowedsymbols/symbols_interp.go — context.Background addition

Positive observations

• The Each() deduplication fix (emitting overlay values first, skipping parent overrides) correctly prevents the 2× seed inflation for background subshells — a well-reasoned fix.
• Using errors.As to distinguish errTotalVarStorageExceeded from other errors and setting exiting = true is the right approach for clean script abort semantics.
• The Reset() zeroing of totalBytes after PWD/IFS/OPTIND assignments is a clean way to exclude interpreter bookkeeping from the user-visible cap.
• The context-aware read loop in stdinFile() for runner_redir.go correctly passes the per-execution context, bounding the goroutine's lifetime to the script execution context.
• defer pw.Close() is correctly used to prevent pipe write-end leaks.

Findings summary

#	Priority	File	Finding
1		interp/vars.go:128	totalBytes double-counts when overriding a parent-inherited variable in a non-background subshell
2		interp/var_size_test.go	Missing test coverage for non-background subshell overriding an inherited variable
3		tests/scenarios/shell/var_expand/basic/total_var_storage_cap_exceeded.yaml:12	Uses stderr_contains instead of expect.stderr (project convention)

[thieman]

@codex review this PR

[thieman]

Iteration 1 self-review result: 0 P0, 1 P1, 1 P2, 1 P3 findings.

Summary: One logic bug (P1) in overlayEnviron.Set() where totalBytes double-counts when a non-background subshell overrides a parent-inherited variable — fix is to make the old-bytes credit unconditional rather than overlay-conditional. One missing test (P2) for that path. One test style issue (P3) using stderr_contains instead of expect.stderr.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8a8473838

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thieman]

Review Summary

This PR addresses two legitimate security issues:

M-5 (Total variable storage cap): Adds MaxTotalVarsBytes = 1 MiB enforced via an O(1) delta tracker in overlayEnviron. The fix is well-structured and the fix for the Each() deduplication bug (emit overlay-first, skip parent dupes) is correct. Three regression Go tests and two scenario tests are added.

M-6 (Stdin goroutine context awareness): Replaces io.Copy with a context-checking read loop. The runner_redir.go caller correctly passes the per-execution context.

Overall assessment: needs minor fixes — one P2 correctness issue found (interaction between setVarRestore and the new total cap when large Env() variables are in use), one P2 about the goroutine still blocking on r.Read past cancellation, plus two P3 hardening suggestions.

#	Priority	File	Finding
1		interp/api.go:263	r.Read(buf) blocks past context cancellation
2		interp/vars.go	setVarRestore can fail with storage-cap error for large Env() variables
3		interp/vars.go:122	totalBytes can go negative (underflow) on unset in fresh overlay
4		tests/scenarios/shell/var_expand/	Missing scenario: subshell nesting cannot bypass cap via counter reset

---

Finding 2 (not inline — setVarRestore is unchanged in this PR)

setVarRestore can spuriously fail with storage-cap error for large Env() variables

setVarRestore in vars.go bypasses MaxVarBytes (by design, to restore inherited vars larger than 1 MiB), but it calls r.writeEnv.Set(name, vr) which now enforces MaxTotalVarsBytes.

A caller that initialises the runner with interp.Env("BIG=" + strings.Repeat("x", 900*1024)) and then runs a script containing BIG=small echo hello will hit this: after echo returns, setVarRestore tries to write BIG=<900KiB> back. If totalBytes is already ≥ 150 KiB from other script variables, the restore fails with variable storage limit exceeded — even though the script never actually allocated more than MaxTotalVarsBytes.

Remediation: setVarRestore should bypass the total cap check the same way it bypasses the per-value check, or the overlay should expose a setRaw(name, vr) method. Alternatively, add a test in TestOversizedInlineVarAbortsCommand or similar that exercises setVarRestore with a value inherited from Env(), and document the limitation.

---

Positive observations

• The O(1) delta-tracking design is elegant and avoids any iteration over the full variable set on every assignment.
• The Reset() zero-out of totalBytes after system-init vars (PWD, IFS, OPTIND) is a clean solution to the bootstrapping problem.
• The Each() fix (emit overlay-first, parent skips overrides) correctly solves the double-count without introducing a full copy of parent variables.
• The context.Background() documentation in StdIO godoc is thorough and accurate.

[thieman]

@codex review this PR

[thieman]

Iteration 2 self-review result: 0 P0, 0 P1, 2 P2, 2 P3 findings.

Summary: Two P2 issues: (1) r.Read(buf) in api.go:263 can block past context cancellation; (2) setVarRestore now routes through the capped overlayEnviron.Set, which could spuriously reject large inherited vars being restored. Two P3 issues: (1) totalBytes underflow if seeding/unset paths fall out of sync; (2) missing scenario test for subshell nesting bypass of cap.

[thieman]

Review Summary

This PR adds two safety features to the restricted shell interpreter:

1. M-5 (Total variable storage cap): MaxTotalVarsBytes = 1 MiB limits the combined size of all script-assigned variable values. The implementation tracks deltas in overlayEnviron.totalBytes, carefully handles subshell inheritance, and fixes a pre-existing double-count bug in overlayEnviron.Each().
2. M-6 (Stdin goroutine context awareness): Replaces io.Copy with a context-aware read loop in stdinFile, so scripts that use input redirects (e.g. cmd < file) respect context cancellation.

The overall approach is sound and the edge cases around subshell inheritance, Env() exclusion from the cap, and restore paths are well-handled. The fix to Each() (preventing double-emission of overridden variables) is a necessary correctness fix that ripples correctly through the new accounting.

No P0/P1 findings. Two P2 issues (one correctness gap, one missing doc update) and two P3 notes.

Assessment: safe to merge after addressing P2 findings.

---

Findings Summary

#	Priority	File	Finding
1		interp/vars.go:174	Missing underflow guard on totalBytes when delta < 0 in Set()
2		SHELL_FEATURES.md	Variable size limits (MaxVarBytes and MaxTotalVarsBytes) not documented
3		interp/vars.go	Missing scenario test for unset interaction with totalBytes cap
4		tests/scenarios/.../total_var_storage_cap_exceeded.yaml:12	stdout: "" should use `

---

Positive Observations

• The overlayEnviron.Each() dedup fix is correct and necessary — without it newOverlayEnviron would double-count bytes when a variable is overridden, causing false cap hits.
• Seeding totalBytes from the parent *overlayEnviron counter (rather than re-summing via Each()) correctly excludes Env()-provided variables from the cap, matching the top-level Reset() zero-out behaviour.
• The setUncapped / setVarRestore separation is a good design: it prevents a restore from failing when the script legitimately fills storage during a command's execution.
• The expandErr case for errTotalVarStorageExceeded is a good defence-in-depth catch for any future parameter-expansion paths that might set variables.
• Context-awareness in the stdin goroutine is correctly limited to the runner_redir.go path (execution-scoped context); the StdIO option correctly uses context.Background() since no run-scoped context exists at option-application time, and this limitation is well-documented.
• Test coverage is thorough: the Go tests exercise all the double-count regression scenarios, and the scenario YAML files cover the observable behaviour.

---

Finding 2: SHELL_FEATURES.md — Variable size limits not documented

SHELL_FEATURES.md does not mention either the per-variable 1 MiB limit (MaxVarBytes, which predates this PR) or the new total storage cap (MaxTotalVarsBytes = 1 MiB). Operators and AI agents using rshell will see an unexpected script abort when either limit is hit. Suggested addition after the command substitution output cap note (line 45):

- ✅ Variable limits: single variable value capped at 1 MiB (`MaxVarBytes`); total combined script-assigned variable storage capped at 1 MiB (`MaxTotalVarsBytes`); exceeding either limit aborts the script with exit code 1. Variables provided via `Env()` do not count against the total cap.

[thieman]

@codex review this PR

[thieman]

Iteration 6 self-review result: 0 P0, 0 P1, 2 P2, 2 P3 findings.

Summary: Two P2 issues: (1) missing underflow guard on totalBytes in the main assignment path of Set(); (2) neither variable cap is documented in SHELL_FEATURES.md. Two P3 issues: (1) missing scenario test for unset freeing storage; (2) new scenario files use bare empty string instead of |+ block scalar.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 984f035685

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thieman]

Review Summary

Reviewed M-5 (total variable storage cap) and M-6 (stdin goroutine context cancellation) fixes in interp/vars.go, interp/api.go, interp/runner_redir.go, interp/runner_expand.go, and associated tests.

Overall assessment: safe to merge — no security vulnerabilities or correctness regressions found. The implementation is carefully designed and well-tested. Two P2 design notes and one P3 coverage gap are filed below.

---

Findings Summary

#	Priority	File	Finding
1		interp/vars.go:22,26	MaxVarBytes == MaxTotalVarsBytes makes a single 1 MiB variable consume the entire total cap
2		interp/vars.go:273-274	setVarErr (used by expandEnv.Set) bypasses the MaxVarBytes per-value check (pre-existing gap)
3		interp/vars.go:188-210	No integration test for inline variable restore (FOO=val cmd) at the storage limit boundary

---

Findings

Finding 1 — MaxVarBytes == MaxTotalVarsBytes leaves no headroom for a second variable after a 1 MiB assignment

Severity: P2 — Design / Correctness
Location: interp/vars.go:22,26

Description:
Both constants are 1 << 20 (1 MiB). A script that assigns exactly MaxVarBytes to a single variable fills the total cap to the brim. Any subsequent assignment — even X=y (1 byte) — is rejected with variable storage limit exceeded.

# Builds a 1 MiB value byte-by-byte via repeated doubling.
A=x
A=$A$A;A=$A$A;A=$A$A;A=$A$A;A=$A$A;A=$A$A;A=$A$A;A=$A$A;A=$A$A;A=$A$A  # 1024 bytes
# ... continued doublings ...
B=$A  # B is 1 MiB — exactly at the total cap
C=x   # <-- this fails: variable storage limit exceeded

This is exactly what total_var_storage_cap_exceeded.yaml asserts — so the behaviour is tested and intentional. However, it means real scripts holding a large value in one variable cannot store anything else. Whether this is the right trade-off depends on the expected use case; callers may need to be aware that the total cap effectively equals the per-variable cap.

Suggested follow-up: Document this constraint in a code comment near the constant definitions, or consider raising MaxTotalVarsBytes to a small multiple of MaxVarBytes (e.g. 4 MiB), depending on the memory budget for the shell.

---

Finding 2 — setVarErr bypasses the MaxVarBytes per-value check (pre-existing)

Severity: P2 — Pre-existing gap surfaced by the new total-cap logic
Location: interp/vars.go:273-274, interp/runner_expand.go:220-224

Description:
setVarErr delegates directly to r.writeEnv.Set without checking len(vr.Str) > MaxVarBytes. It is called from expandEnv.Set, which is invoked by the expand package for parameter-expansion assignments. The total-cap check in overlayEnviron.Set still fires, so a >1 MiB value would be caught by the total cap. But the per-value guard and its specific error message ("value too large") are never surfaced through this code path.

This is not introduced by this PR — it predates it. But the new total-cap machinery makes the gap slightly more visible because the two limits are now peers.

Suggested follow-up: Add a len(vr.Str) > MaxVarBytes guard inside setVarErr (or inside expandEnv.Set) with the same error message as setVar, so both assignment paths produce a consistent error.

---

Finding 3 — No integration test for inline variable restore at the storage boundary

Severity: P3 — Coverage gap
Location: interp/vars.go:188-210 (setUncapped)

Description:
The setUncapped path is exercised by the TestSetUncappedNoTombstone unit test (tombstone check) and the restore logic in runner_exec.go. However, no test verifies the full inline-variable restore flow when the storage cap is close to full during the command's execution.

A scenario like:

A=$(python3 -c 'print("x" * 512000)')  # fill ~500 KiB
FOO=bar echo SHOULD_WORK              # inline var, command must run and FOO restored
echo $A  # A must still be accessible after the restore

…would verify that setUncapped correctly restores the previous value when the script holds significant storage during the inline assignment. Without this, a future refactor that accidentally re-enables the cap in the restore path would only be caught by a runtime failure.

Suggested follow-up: Add a Go test in interp/var_size_test.go that assigns a large variable, then runs an inline FOO=val builtin command, and asserts the original large variable is still accessible and that exit code is 0.

---

Coverage Table

Code path	Scenario test	Go test	Status
Total cap enforced (many small vars)	total_var_storage_cap_exceeded.yaml	TestTotalVarStorageCapEnforced	Covered
Nested subshell cannot reset counter	total_var_storage_cap_subshell_nesting.yaml	—	Covered
Shrink frees space	total_var_storage_cap_update_tracking.yaml	TestTotalVarStorageCapUpdateTracking	Covered
Background subshell cap inherited	—	TestBackgroundSubshellCapEnforced	Covered
Double-count in Each() prevented	—	TestSubshellTotalVarStorageDoubleCount	Covered
Non-background subshell override tracking	—	TestNonBackgroundSubshellVarOverrideTracking	Covered
Env() vars not counted in subshell	—	TestNonBackgroundSubshellDoesNotCountEnvVars	Covered
expandErr treats storage err as fatal	—	TestExpandErrStorageExhaustedAborts	Covered
Unset frees storage	—	TestOverlayEnvironUnsetFreesStorage	Covered
setUncapped no-tombstone on restore	—	TestSetUncappedNoTombstone	Covered
inline var restore at cap boundary	—	—	Missing (P3)
stdinFile respects context cancellation	—	—	Missing (acceptable — hard to test reliably)

---

Positive Observations

• Correct O(1) accounting: using a tracked totalBytes counter rather than re-summing Each() on every write is the right approach — avoids O(n) cost per assignment and prevents the double-count bug that plagued the Each()-based approach.
• Careful subshell seeding: the distinction between background (snapshot-copy) and non-background (parent-pointer) subshells and the corresponding totalBytes seed logic is well-reasoned and regression-tested.
• setUncapped for restore: the dedicated bypass for inline variable restore is a clean solution that keeps the hot path safe without breaking the restore invariant.
• Context-aware goroutine: the stdinFile goroutine fix correctly checks ctx.Err() at the top of each iteration and documents its limitations for blocking readers.
• Defensive underflow guards: the if o.totalBytes < 0 { o.totalBytes = 0 } guards throughout protect against invariant violations without panicking.

[thieman]

@codex review this PR

[thieman]

Iteration 7 self-review result: 0 P0, 0 P1, 2 P2, 1 P3 findings. Overall: Safe to merge.

Summary: Two P2 design notes: (1) MaxVarBytes == MaxTotalVarsBytes means a single 1 MiB variable exhausts the entire cap; (2) setVarErr bypasses the per-value MaxVarBytes check (pre-existing gap). One P3 coverage gap for inline variable restore at storage boundary.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16b427e999

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thieman]

Review Summary

This PR adds two safety improvements to the restricted shell interpreter:

1. M-5 — Total variable storage cap (MaxTotalVarsBytes = 1 MiB): Prevents unbounded memory use from many small variables. The accounting is O(1) per assignment via a totalBytes delta counter on overlayEnviron. The implementation correctly handles subshell nesting (non-background and background), Env()-supplied variables that should be excluded from the cap, inline command variables (FOO=val cmd) via an uncapped restore path, and parameter-expansion assignments via expandErr.

2. M-6 — Context-aware stdin copy goroutine: Replaces io.Copy with a ctx.Err()-checked read loop, bounding goroutine lifetime after context cancellation.

The code is well-structured, thoroughly commented, and has excellent test coverage — both Go unit tests and YAML scenario tests for the main cap behaviors. All existing tests pass.

Overall assessment: safe to merge — no P0/P1 findings.

---

Findings Summary

#	Priority	File	Finding
1		tests/scenarios/shell/var_expand/basic/total_var_storage_cap_exceeded.yaml:12	stdout: |+ with no content is technically valid but the project style guide does not have precedent for this — stdout: "" is the more explicit and common form
2		interp/vars.go:199	Cap check uses delta > 0 guard but skips the cap check entirely when delta <= 0; this is correct for shrink/no-change but worth a brief inline comment explaining why shrinks never need cap enforcement
3		interp/var_size_test.go	Missing scenario test for inline command variable restore at storage boundary (FOO=val cmd when near cap) — covered by TestInlineVarRestoreAtStorageBoundary Go test, but a YAML scenario would also validate bash divergence awareness

---

Positive Observations

• The totalBytes accounting correctly uses the parent's counter (not parent.Each() summing) to avoid counting Env()-supplied variables, preventing false cap violations for callers with large initial environments.
• The Reset() zeroing of totalBytes after seeding PWD/IFS/OPTIND is a clean design that excludes interpreter bookkeeping variables from the user-visible cap.
• The setUncapped method + setVarRestore bypass prevents restore failures when a script has filled storage during a command's inline-variable scope — a subtle but important correctness detail.
• The Each() deduplication fix (emit parent vars only if not already in overlay) correctly prevents the double-count bug that caused false cap violations in subshells.
• The expandErr storage cap arm ensures that any future parameter expansion assignment path (e.g. ${var:=value} if ever re-enabled) will also abort the script, not just set exit code 1.
• TestEnvVarReassignDoesNotExpandQuota specifically guards against the negative-delta quota expansion bug — this is exactly the kind of security regression test that belongs here.
• The context-aware stdin goroutine uses defer pw.Close() which is correct; the original io.Copy approach was missing this pattern.

[thieman]

@codex review this PR

[thieman]

Iteration 8 self-review result: 0 P0, 0 P1, 0 P2, 3 P3 findings. Overall: Safe to merge.

Summary: All findings are cosmetic P3 style items only. Core implementation is correct: cap accounting is O(1), handles all subshell types correctly, Env() vars excluded from cap, setVarRestore bypass path clean. All 9 new Go tests and 3 new YAML scenarios pass.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16b427e999

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thieman]

Review Summary

This PR implements two mitigations:

1. M-5 — a 1 MiB total variable-storage cap (MaxTotalVarsBytes) tracked via overlayEnviron.totalBytes, enforced in all assignment paths (setVar, setVarErr/expandErr)
2. M-6 — context-aware stdinFile goroutine that checks ctx.Err() each iteration instead of using the unbounded io.Copy

Overall assessment: safe to merge. No P0/P1 findings. One P2 finding (dead-code fallback path in setVarRestore that silently records exit code 1 without aborting) and two P3 observations.

---

Findings Summary

#	Priority	File	Finding
1		interp/vars.go:351-354	setVarRestore fallback silently records exit.code = 1 for storage errors without setting exiting=true
2		interp/vars.go:86-89	Background-subshell non-*overlayEnviron fallback is effectively dead code without a comment noting it
3		interp/vars.go:139-173	Unset path in Set() uses unconditional len(prev.Str) to subtract, but earlier diff version used a tracked unsetOldBytes guard — verify current file matches the merged text

---

P2 Findings

1. setVarRestore fallback doesn't abort on storage-cap error

Severity: P2 — Correctness / Defense in Depth

Location: interp/vars.go — the else branch of setVarRestore (after the *overlayEnviron type assertion)

The primary path (ov, ok := r.writeEnv.(*overlayEnviron)) correctly routes to setUncapped, bypassing the cap. The fallback path calls r.writeEnv.Set(name, vr), which can return errTotalVarStorageExceeded. When it does, the fallback only sets r.exit.code = 1 — it does not set r.exit.exiting = true. This means a restore at the cap boundary could fail silently while the script continues with inconsistent variable state.

In practice this path is unreachable today because Reset() always creates *overlayEnviron for r.writeEnv. However, the inconsistency is a latent correctness risk if that invariant ever changes.

Remediation: Add the same errors.As(err, &storageErr) abort logic used in setVar, or add a comment explicitly stating the fallback is dead code and cannot receive errTotalVarStorageExceeded in the current implementation.

---

P3 Findings

2. Background-subshell fallback is unreachable — worth a comment

Severity: P3 — Code Quality

Location: interp/vars.go:86-89 (inside newOverlayEnviron, background=true, non-*overlayEnviron parent)

For background subshells, parent is always r.writeEnv, which is always *overlayEnviron after Reset(). The else fallback that sums oenv.values is effectively unreachable. A reader may wonder whether this path is tested or whether it correctly excludes Env() variables (it would not, but that's moot since it's unreachable).

Remediation: Add a comment: // Unreachable in practice: parent is always *overlayEnviron after Reset() so future readers understand the invariant.

3. Each() ordering change — verify maps.Insert background-subshell behavior

Severity: P3 — Correctness (low risk, worth confirming)

Location: interp/vars.go:247-265 (Each() implementation)

The old Each() emitted parent first, then overlay (allowing duplicates). The new code emits overlay first, then parent (skipping duplicates). For background subshells, maps.Insert(oenv.values, parent.Each) calls the parent's Each() to populate the snapshot. Since the parent here is an *overlayEnviron, the new Each() correctly emits each variable exactly once (overlay value wins). This is correct and actually fixes the double-count bug.

However, maps.Insert uses the last-write-wins semantic for duplicate keys. With the new ordering (overlay first, parent skips dupes), duplicates are suppressed before reaching maps.Insert, so only one value per key is ever written to oenv.values. This is the correct value (overlay wins). Confirmed safe.

---

Coverage Summary

Code path	Scenario test	Go test	Status
Total cap exceeded (many small vars)	total_var_storage_cap_exceeded.yaml	TestTotalVarStorageCapEnforced	Covered
Update/shrink tracking	total_var_storage_cap_update_tracking.yaml	TestTotalVarStorageCapUpdateTracking	Covered
Subshell nesting bypass prevention	total_var_storage_cap_subshell_nesting.yaml	—	Covered
Inline var restore at boundary	total_var_storage_cap_inline_var_restore.yaml	TestInlineVarRestoreAtStorageBoundary	Covered
Non-background subshell double-count fix	—	TestNonBackgroundSubshellVarOverrideTracking	Covered
Env() vars excluded from cap in subshells	—	TestNonBackgroundSubshellDoesNotCountEnvVars	Covered
Background subshell cap enforced	—	TestBackgroundSubshellCapEnforced	Covered
Env() var shrink quota-expansion guard	—	TestEnvVarReassignDoesNotExpandQuota	Covered
expandErr aborts on storage error	—	TestExpandErrStorageExhaustedAborts	Covered
stdinFile context cancellation	—	—	Gap: no direct test for context-cancel path

The stdinFile context-cancel path (M-6 fix) has no dedicated test. The existing stdin behavior is covered indirectly by scenario tests that use stdin redirects, but there's no test that cancels the context mid-read to verify the goroutine exits. This is a P3 gap — acceptable given the fix is straightforward and the goroutine's defer pw.Close() ensures cleanup regardless.

---

Positive Observations

• The accounting logic correctly handles all three subshell types with unified totalBytes seeding from the parent's tracked counter, avoiding the double-count bug with Each() deduplication.
• The Reset() zero-out pattern to exclude interpreter-managed variables (PWD, IFS, OPTIND) from the cap is clean and well-commented.
• setUncapped correctly bypasses the cap for variable restores while still updating totalBytes for accurate baseline accounting.
• The expandErr case for errTotalVarStorageExceeded closes a real cap-bypass that would exist if parameter-expansion assignments hit the storage limit.
• Test coverage is thorough: 6 focused Go tests and 4 YAML scenario tests covering the new behavior and all identified regression cases.
• skip_assert_against_bash: true is correctly applied to all new scenarios (bash has no variable storage limit).
• The context.Background() usage in StdIO is justified and well-documented in the new godoc comment.

[thieman]

@codex review this PR

[thieman]

Iteration 9 self-review result: 0 P0, 0 P1, 1 P2, 2 P3 findings. Overall: Safe to merge.

Summary: One P2: setVarRestore fallback branch silently sets exit.code=1 without exiting=true on storage-cap error (dead code in practice since writeEnv is always *overlayEnviron after Reset(), but latent inconsistency). Two P3 style items.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16b427e999

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thieman]

Review Summary

This PR addresses two distinct issues:

1. M-5 (total variable cap): Adds MaxTotalVarsBytes = 1 MiB enforced in overlayEnviron.Set, with O(1) delta tracking, correct inheritance across subshell nesting levels, and exclusion of Env()-supplied variables from the cap. The accounting logic is careful and well-documented.

2. M-6 (stdin goroutine context): Replaces io.Copy with a context-aware read loop in stdinFile, checking ctx.Err() at the top of each iteration to bound goroutine lifetime after cancellation.

Overall assessment: safe to merge with one minor YAML convention finding.

Findings Summary

#	Priority	File	Finding
1		tests/scenarios/shell/var_expand/basic/total_var_storage_cap_exceeded.yaml:12	stdout: "" should use |+ block scalar per project convention

Coverage Summary

Code path	Scenario test	Go test	Status
Total cap exceeded (direct assignment)	total_var_storage_cap_exceeded.yaml	TestTotalVarStorageCapEnforced	Covered
Subshell cannot reset counter	total_var_storage_cap_subshell_nesting.yaml	—	Covered
Shrink frees space	total_var_storage_cap_update_tracking.yaml	TestTotalVarStorageCapUpdateTracking	Covered
Non-background subshell double-count regression	—	TestSubshellTotalVarStorageDoubleCount	Covered
Non-background subshell override tracking	—	TestNonBackgroundSubshellVarOverrideTracking	Covered
Env() vars not counted in subshell	—	TestNonBackgroundSubshellDoesNotCountEnvVars	Covered
Background subshell inherits cap	—	TestBackgroundSubshellCapEnforced	Covered
setVarRestore / setUncapped at boundary	—	TestInlineVarRestoreAtStorageBoundary	Covered
Env() shrink quota bypass prevention	—	TestEnvVarReassignDoesNotExpandQuota	Covered
expandErr handles storage error (fatal)	—	TestExpandErrStorageExhaustedAborts	Covered
Context-aware stdin goroutine	—	TestStdinFileContextCancelExitsGoroutine	Covered

Coverage: Adequate — all changed code paths have corresponding tests.

Positive Observations

• The totalBytes delta accounting is O(1) and correct for all cases: new variables, updates (grow/shrink), unsets, and inline-command restores.
• Correct handling of the Env()-exclusion invariant: Reset() zeros totalBytes after initial variable seeding, and both Set() and setUncapped() apply the same parentIsOverlay guard to avoid crediting untracked bytes.
• The Each() fix (emitting parent variables only when not already in the overlay) correctly prevents the 2× double-count bug when building subshell environments.
• The setUncapped function correctly uses delete(o.values, name) instead of storing a tombstone, preventing unbounded map growth.
• The expandErr storage-error arm closes a previously-open cap-bypass via parameter expansion.
• The context-aware read loop correctly handles n > 0 bytes before checking the err return (read-before-error pattern), which is the correct way to handle io.Reader contracts.
• All new scenario tests correctly use skip_assert_against_bash: true since this is intentional divergence from bash.

[thieman]

@codex review this PR

[thieman]

Iteration 10 self-review result: 0 P0, 0 P1, 0 P2, 1 P3 finding. Overall: Safe to merge.

Summary: One P3 style item: total_var_storage_cap_exceeded.yaml uses stdout: "" instead of |+ block scalar. No security or correctness issues found. All accounting paths verified correct.

[thieman]

Review Summary

PR: fix(interp): add 1 MiB total variable cap and fix stdin goroutine context cancellation

Overall assessment: needs fixes — one P1 correctness bug found (cap bypass in background subshells via Env()-provided variable manipulation), plus a P2 error message cosmetic issue. The core architecture is sound and the test coverage is comprehensive.

Findings Summary

#	Priority	File	Finding
1		interp/vars.go:200	Background subshell cap bypass via Env() variable shrink/clear
2		interp/vars.go:320	setVarErr double-embeds variable name in MaxVarBytes error message

Coverage Table

Code path	Scenario test	Go test	Status
Cap exceeded (simple)	total_var_storage_cap_exceeded.yaml	TestTotalVarStorageCapEnforced	Covered
Cap with update/shrink	total_var_storage_cap_update_tracking.yaml	TestTotalVarStorageCapUpdateTracking	Covered
Subshell nesting bypass	total_var_storage_cap_subshell_nesting.yaml	TestSubshellTotalVarStorageDoubleCount, TestNonBackgroundSubshellVarOverrideTracking	Covered
Inline-var restore at boundary	total_var_storage_cap_inline_var_restore.yaml	TestInlineVarRestoreAtStorageBoundary	Covered
Background subshell + Env() var clear	—	—	Missing (P1 bug)
Env() var reassign quota guard	—	TestEnvVarReassignDoesNotExpandQuota	Covered
expandErr fatal for storage cap	—	TestExpandErrStorageExhaustedAborts	Covered
Context-cancel on stdin goroutine	—	TestStdinFileContextCancelExitsGoroutine	Covered
setUncapped tombstone prevention	—	TestSetUncappedNoTombstone	Covered

Positive Observations

• Thorough documentation of the design decisions (especially the Env()-exclusion rationale)
• Comprehensive regression tests covering the subshell double-count and non-background seeding bugs
• The setUncapped tombstone fix prevents unbounded map growth
• The expandErr storage arm correctly handles the parameter-expansion path to prevent cap bypass via ${var:=value}
• The Each() ordering change correctly prevents double-counting while preserving semantics
• stdinFile context-cancel fix is clean and the context.Background() limitation is clearly documented

[thieman]

@codex review this PR

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16b427e999

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thieman]

Iteration 11 self-review result: 0 P0, 1 P1, 1 P2, 0 P3 findings.

Summary: P1 cap bypass — background subshells copy Env() vars into values but totalBytes doesn't include them; shrinking/clearing an Env() var drives totalBytes negative via underflow clamp, allowing subsequent assignments past the cap. P2 — setVarErr double-embeds variable name in error message.

[thieman]

Review-Fix Loop Summary

• PR: fix(interp): add 1 MiB total variable cap and fix stdin goroutine context cancellation #153 (fix(interp): add 1 MiB total variable cap and fix stdin goroutine context cancellation #153)
• Iterations completed: 11
• Final status: CLEAN

Iteration log

#	Unresolved threads	Fixes applied	CI status
1	4	3 fixed (P1 double-count bug, P2 missing test, P3 yaml convention)	Passing
2	5	3 fixed (Read blocking comment, totalBytes underflow guard, nesting scenario)	Passing
3	0	2 fixed (expandErr fatal exit, background subshell cap + setVarRestore bypass)	Passing
4	2	4 fixed (Env() seeding fix, expandErr test, yaml format, godoc update)	Passing
5	2	2 fixed (totalBytes comment, background subshell cap test)	Passing
6	2	5 fixed (underflow guard, tombstone fix, unset test, yaml format, codex threads)	Passing
7	0	6 fixed (Env() delta accounting, MaxTotalVarsBytes design doc, setVarErr check, tests)	Passing
8	0	3 fixed (stdout clarity, cap-check comment, inline-var YAML test)	Passing
9	1	3 fixed (setVarRestore exiting=true, unreachable comment, stdinFile test)	Passing
10	0	4 fixed (yaml	+ style, codex threads resolved)
11	0	2 fixed (background subshell Env() bypass P1, setVarErr double-name P2)	Passing

Final state

• Unresolved threads: 0 (1 from val06 — excluded per workflow rules)
• CI: All 34 checks passing

Remaining issues (if any)

• 1 thread from val06 (not from thieman or chatgpt-codex-connector[bot], excluded per review-fix-loop rules)