internal/sandbox at 9f824f73d1, reviewed by antigravity:claude-opus-4-6-thinking, verified by codex.
ScrubbedEnviron does not drop CORRAL_TOKEN (the agent's bearer token set by the launcher), so the workspace substrate leaks it to untrusted test commands via cmd.Env = sandbox.ScrubbedEnviron().
#!/bin/sh
set -e
cd "$(dirname "$0")/.." 2>/dev/null || true
cat > internal/sandbox/scrub_corral_token_test.go <<'EOF'
package sandbox
import (
"strings"
"testing"
)
func TestScrubbedEnvironLeaksCorralToken(t *testing.T) {
const secret = "agent-bearer-token-v3xy"
t.Setenv("CORRAL_TOKEN", secret)
for _, kv := range ScrubbedEnviron() {
if strings.HasPrefix(kv, "CORRAL_TOKEN=") {
t.Fatalf("ScrubbedEnviron leaks CORRAL_TOKEN: %s", kv)
}
}
}
EOF
if go test -run TestScrubbedEnvironLeaksCorralToken ./internal/sandbox/ -v 2>&1 | grep -q 'FAIL'; then
echo "DEFECT CONFIRMED: ScrubbedEnviron leaks CORRAL_TOKEN to test commands"
go test -run TestScrubbedEnvironLeaksCorralToken ./internal/sandbox/ -v 2>&1
exit 0
else
echo "Defect not reproduced"
exit 1
fi[stderr] sh: 4: cannot create internal/sandbox/scrub_corral_token_test.go: Directory nonexistent
I checked the imported credential list and the workspace execution boundary for another scrub. internal/creds/creds.go:27 lists no CORRAL_TOKEN; internal/sandbox/sandbox.go:151 adds only GOOGLE_API_KEY and MotherDuck names before retaining other environment entries. internal/adequacy/workspace.go:408 assigns that result directly to cmd.Env. The launcher explicitly supplies CORRAL_TOKEN at cmd/corral-agent/launcher.go:82. MinimalEnv protects a different execution path and does not refute leakage when this token is present in the workspace runner's environment.
ref 132490d49d328e6a8b5ebc0399bad312e39ee9459613cb519066e7cf80533877#R1
CappedWriter.String() can exceed its Max by 22 bytes (the truncation marker + newline) when the buffer is full, violating the size bound that TailWriter explicitly maintains. A caller trusting MaxOutput as a hard ceiling on Result.Output gets a longer string than expected.
#!/bin/sh
set -e
cat > internal/sandbox/cap_overflow_test.go <<'EOF'
package sandbox
import (
"strings"
"testing"
)
func TestCappedWriterStringExceedsMax(t *testing.T) {
const max = 100
cw := NewCappedWriter(max)
_, _ = cw.Write([]byte(strings.Repeat("x", max)))
_, _ = cw.Write([]byte("overflow"))
got := cw.String()
if len(got) <= max {
t.Skipf("CappedWriter.String() did not exceed max (got %d)", len(got))
}
t.Fatalf("CappedWriter(max=%d).String() = %d bytes (exceeds max by %d); TailWriter reserves space for the marker but CappedWriter does not", max, len(got), len(got)-max)
}
EOF
if go test -run TestCappedWriterStringExceedsMax ./internal/sandbox/ -v 2>&1 | grep -q 'FAIL'; then
echo "DEFECT CONFIRMED: CappedWriter.String() exceeds its Max"
go test -run TestCappedWriterStringExceedsMax ./internal/sandbox/ -v 2>&1
exit 0
else
echo "Defect not reproduced"
exit 1
fiDEFECT CONFIRMED: CappedWriter.String() exceeds its Max
=== RUN TestCappedWriterStringExceedsMax
cap_overflow_test.go:17: CappedWriter(max=100).String() = 122 bytes (exceeds max by 22); TailWriter reserves space for the marker but CappedWriter does not
--- FAIL: TestCappedWriterStringExceedsMax (0.00s)
FAIL
FAIL github.com/pdbethke/corralai/internal/sandbox 0.007s
FAILI checked whether rendering or the Result conversion reserves marker space. internal/sandbox/sandbox.go:311 retains all 100 input bytes for the named input and marks the subsequent write truncated. String at line 334 then appends a newline and the 21-byte UTF-8 marker, yielding 122 bytes. Run assigns this string directly to Result.Output at line 210. The comments distinguish retained command bytes from the appended marker, but that does not refute the claimed output length or asymmetry.
ref 132490d49d328e6a8b5ebc0399bad312e39ee9459613cb519066e7cf80533877#R2
Resolve's error message for an unknown backend says '(want bwrap|container|none)' but omits the two other valid backends: 'sandbox-exec' and 'windows-job'. An operator who misspells one of them gets an error that does not mention the correct name.
#!/bin/sh
set -e
cat > internal/sandbox/resolve_errmsg_test.go <<'EOF'
package sandbox
import (
"strings"
"testing"
)
func TestResolveErrorMessageOmitsValidBackends(t *testing.T) {
_, err := Resolve(Config{Backend: "bogus"})
if err == nil {
t.Fatal("unknown backend should error")
}
msg := err.Error()
for _, valid := range []string{"sandbox-exec", "windows-job"} {
if !strings.Contains(msg, valid) {
t.Errorf("error message omits valid backend %q: %s", valid, msg)
}
}
}
EOF
if go test -run TestResolveErrorMessageOmitsValidBackends ./internal/sandbox/ -v 2>&1 | grep -q 'FAIL'; then
echo "DEFECT CONFIRMED: Resolve error message incomplete"
go test -run TestResolveErrorMessageOmitsValidBackends ./internal/sandbox/ -v 2>&1
exit 0
else
echo "Defect not reproduced"
exit 1
fiDEFECT CONFIRMED: Resolve error message incomplete
=== RUN TestResolveErrorMessageOmitsValidBackends
resolve_errmsg_test.go:16: error message omits valid backend "sandbox-exec": unknown exec backend "bogus" (want bwrap|container|none)
resolve_errmsg_test.go:16: error message omits valid backend "windows-job": unknown exec backend "bogus" (want bwrap|container|none)
--- FAIL: TestResolveErrorMessageOmitsValidBackends (0.00s)
FAIL
FAIL github.com/pdbethke/corralai/internal/sandbox 0.014s
FAILThe claim of five valid backends is overstated. Although Resolve recognizes windows-job at internal/sandbox/isolator.go:66, its Windows constructor unconditionally returns an error at internal/sandbox/isolator_windows.go:19, deliberately refusing execution because job objects provide no filesystem or network isolation. Thus windows-job is a recognized refusal path, not another usable backend an operator can select by correcting a typo. The narrower UX defect remains: the diagnostic at internal/sandbox/isolator.go:95 omits sandbox-exec, which has an implemented constructor and is Darwin's default.
ref 132490d49d328e6a8b5ebc0399bad312e39ee9459613cb519066e7cf80533877#R3
On Windows, proc_windows.go assigns the process to the job object AFTER cmd.Start(), creating a race window where the process (and any children spawned during that window) run outside the job object. A child spawned before AssignProcessToJobObject completes would survive a cancel/timeout kill.
I inspected process setup, assignment, cancellation, and the Windows backend refusal for a mechanism closing the window. setProcGroup at internal/sandbox/proc_windows.go:54 creates a job but does not arrange suspended process creation. runCommand starts the command at line 86 and assigns it at line 97. Cancellation at line 68 closes the job and kills the direct process; it does not enumerate previously spawned descendants. The windows-job backend's refusal does not remove this shared process-management code from other Windows execution paths. The race remains plausible from code inspection; descendant survival was not executed or demonstrated here.
ref 132490d49d328e6a8b5ebc0399bad312e39ee9459613cb519066e7cf80533877#R4
The darwin sandbox-exec backend does not apply the PerEntry filter to ReadOnlyBinds: it grants (allow file-read*) to the entire Host directory, so dot-entries (.vite, .cache) that perEntryBinds() explicitly skips on bwrap/container are readable on macOS. While this does not cause EROFS (no bind mounts), it is a parity gap between backends.
I compared the Darwin profile construction with the bind expansion and its documented purpose. internal/sandbox/isolator_darwin.go:94 grants reads to each entire Host subpath without consulting PerEntry. Linux at internal/sandbox/isolator_linux.go:151 and containers at internal/sandbox/container.go:200 instead expand through perEntryBinds, whose dot-entry exclusion is at internal/sandbox/sandbox.go:67. The comments explain that exclusion as avoiding read-only cache mounts, rather than promising secret filtering, but the reviewer already limits this finding to backend parity and acknowledges the absence of EROFS on Darwin. That limited claim withstands inspection.
ref 132490d49d328e6a8b5ebc0399bad312e39ee9459613cb519066e7cf80533877#R5
Recorded so that "found nothing here" can be told apart from "never looked here".
The sandbox package is well-commented and shows evidence of careful iterative hardening. However, it has three concrete defects (findings #1–#3) and two structural weaknesses (findings #4–#5). Finding #1 is a credential leak: ScrubbedEnviron, the blocklist meant to strip secrets before handing the environment to a test command, omits CORRAL_TOKEN (the agent's bearer token). The daemon never calls scrubSecrets for this token either, so the workspace runner leaks it to untrusted test commands — the exact scenario the function's own doc comment cites as the threat model. Finding #2 is a size-guarantee asymmetry: CappedWriter.String() can exceed its Max by 22 bytes, while TailWriter explicitly reserves space for the truncation marker. A caller that trusts MaxOutput as a hard bound gets a longer string than expected. Finding #3 is a UX defect: the Resolve error for unknown backends lists only three of the five valid options.
R1, R2, R4, and R5 withstand code inspection; their failed or absent reproductions do not refute them. R3 is narrower than stated: the diagnostic omits sandbox-exec, but windows-job deliberately refuses execution rather than providing a fifth usable backend. No tests, builds, or repository code were executed.