feat(#452): Transient Sandbox core — container-only execution + evidence (PR 1/3)#461
Conversation
…nce (PR 1/3) "The Scientist": execute untrusted builder commands and capture the REAL exit code/output as harness-authored evidence, replacing self-reported tests_run. Posture (Richardson's call): CONTAINER-ONLY. No unconfined child-process tier ever. runInSandbox runs the command only inside docker/podman; with no runtime it returns an `execution: unverified` record (status 'observed', exit null) so the gate degrades to contract validation — never unconfined, never a false green. Every invocation is locked down: --network none (default; opt-in bridge), no host env, --cap-drop ALL + no-new-privileges, non-root, --read-only root + 64m tmpfs, memory/pids/cpu caps, code mounted READ-ONLY, hard wall-clock timeout that SIGKILLs the tree. Evidence (kind:'execution' with exit_code, duration_ms, tier, bounded stdout/stderr tails, truncated) is authored from the child's exit code — never read from anything the executed code writes. Injectable runtime probe + spawn so CI verifies the argv lockdown, exit-code mapping, timeout kill, and the no-runtime degrade with no daemon. Next: PR 2 wires this into sdlc_validate (execution evidence replaces the self-report at the gate; failing stderr → the #451 critique). PR 3 adds a doctor tier report. 1565/0, typecheck + lint + security clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a container-only execution harness with runtime detection, hardened Docker/Podman arguments, bounded output capture, timeout termination, structured status mapping, and tests covering unavailable runtimes, isolation flags, exit outcomes, and timeouts. ChangesTransient sandbox execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/sandbox-execution-452.test.js (1)
20-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider an opt-in real-runtime smoke test alongside the mocked unit tests.
All tests here inject
spawn/probe/runtime, which is great for deterministic CI, but none exercise an actual docker/podman daemon. ThefakeChild.kill(line 24) always "succeeds" synchronously, which would never have surfaced the real-world gap where killing the CLI client doesn't necessarily stop the container (see sandbox.js comment). Consider adding one integration test — skipped cleanly viadetectContainerRuntime()when no runtime is present (mirroring the existing browser-test skip pattern) — that runs a real hung command and asserts the container itself is actually gone afterward (docker ps/podman ps).As per coding guidelines, "tests must exercise the real enforcement path rather than only mocks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sandbox-execution-452.test.js` around lines 20 - 33, Add an opt-in integration smoke test alongside the mocked tests that uses detectContainerRuntime() to skip cleanly when Docker or Podman is unavailable, then runs a genuinely hung command through the real sandbox enforcement path. After timeout or termination, verify via the runtime’s ps command that the container itself no longer exists, covering the behavior not represented by fakeChild.kill.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/harness/sandbox.js`:
- Around line 116-154: Update the timeout enforcement around the child process
and finish handler so the container started by the docker/podman client is given
a unique name or identifier and is explicitly stopped or killed on timeout,
rather than relying only on child.kill('SIGKILL'). Preserve the existing timeout
result while ensuring cleanup targets the actual container, and record any
failure from the container-kill operation instead of silently swallowing it.
In `@tests/sandbox-execution-452.test.js`:
- Line 26: Declare setImmediate as a recognized global for tests/**/*.js in
eslint.config.js, or enable the appropriate Node test environment, so the
setImmediate call in the sandbox execution test passes no-undef validation.
---
Nitpick comments:
In `@tests/sandbox-execution-452.test.js`:
- Around line 20-33: Add an opt-in integration smoke test alongside the mocked
tests that uses detectContainerRuntime() to skip cleanly when Docker or Podman
is unavailable, then runs a genuinely hung command through the real sandbox
enforcement path. After timeout or termination, verify via the runtime’s ps
command that the container itself no longer exists, covering the behavior not
represented by fakeChild.kill.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b097ce3-9e4f-4f4b-a94e-9a6cc6d97c8f
📒 Files selected for processing (2)
src/core/harness/sandbox.jstests/sandbox-execution-452.test.js
| let timedOut = false; | ||
| const timer = setTimeout(() => { | ||
| timedOut = true; | ||
| try { child.kill('SIGKILL'); } catch { /* already gone */ } | ||
| }, bounded); | ||
| timer.unref?.(); | ||
|
|
||
| const finish = (exitCode) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timer); | ||
| const stdout = tail(Buffer.concat(out.map((c) => (Buffer.isBuffer(c) ? c : Buffer.from(c))))); | ||
| const stderr = tail(Buffer.concat(errBuf.map((c) => (Buffer.isBuffer(c) ? c : Buffer.from(c))))); | ||
| const duration = (deps.now ? deps.now() : Date.now()) - start; | ||
| const passed = !timedOut && exitCode === 0; | ||
| resolvePromise({ | ||
| task_id: taskId, kind: 'execution', | ||
| status: passed ? 'PASS' : 'FAIL', | ||
| tier: runtime, | ||
| evidence: timedOut | ||
| ? `execution timed out after ${bounded}ms (killed)` | ||
| : `exit ${exitCode} in ${duration}ms`, | ||
| command, exit_code: timedOut ? null : exitCode, duration_ms: duration, | ||
| network, stdout_tail: stdout.text, stderr_tail: stderr.text, | ||
| truncated: stdout.truncated || stderr.truncated, | ||
| }); | ||
| }; | ||
|
|
||
| child.on('error', (err) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timer); | ||
| resolvePromise({ | ||
| task_id: taskId, kind: 'execution', status: 'observed', tier: 'unverified', | ||
| evidence: `container execution error: ${err?.message ?? err}`, | ||
| command, exit_code: null, duration_ms: 0, network, truncated: false, | ||
| }); | ||
| }); | ||
| child.on('close', (code) => finish(code)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Timeout only kills the local CLI client, not the container — "kills the whole process tree" is not actually enforced.
child.kill('SIGKILL') (line 119) terminates the spawned docker run/podman run client process, but that process is essentially a thin API client to the daemon (dockerd/podman). Killing the client does not reliably stop the running container — this is a well-documented Docker/Podman gotcha (orphaned containers surviving client termination). So:
- The header's promise ("A hard wall-clock timeout kills the whole process tree", lines 13-18/16) is not backed by runtime enforcement, violating the guideline that every documented guardrail must be enforced in code, not just comments.
- A runaway or malicious command — especially with
network: true— can keep executing (and potentially exfiltrating) after the harness has already reportedFAIL/timed-out, silently defeating the sandbox's core security guarantee. - Line 119's
catch { /* already gone */ }also silently swallows any kill failure with no record, compounding the gap (guideline: don't silently swallow errors on enforcement paths).
🔒️ Suggested fix — name the container and kill it by name/id, not just the client
export function buildSandboxArgv(runtime, { runDir, command, network = false, image = 'alpine:3.20', limits = {} }) {
const mem = limits.memory ?? '512m';
const pids = String(limits.pids ?? 256);
const cpus = String(limits.cpus ?? 1);
+ const name = limits.name ?? `rstack-sandbox-${Date.now()}-${Math.random().toString(36).slice(2)}`;
return [
runtime, 'run', '--rm',
+ '--name', name,
'--network', network ? 'bridge' : 'none', const timer = setTimeout(() => {
timedOut = true;
- try { child.kill('SIGKILL'); } catch { /* already gone */ }
+ try { child.kill('SIGKILL'); } catch (e) { /* record e via structured log */ }
+ try { spawnSync(runtime, ['kill', containerName], { stdio: 'ignore', timeout: 5_000 }); } catch (e) { /* record e */ }
}, bounded);As per coding guidelines, "Every documented guardrail, approval gate, and contract promise must be enforced at runtime in src/core/harness/, never by prompt text alone" and "Do not silently swallow errors in governance, audit, persistence, or enforcement paths."
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 116-119: Avoid using the initial state variable in setState
Context: setTimeout(() => {
timedOut = true;
try { child.kill('SIGKILL'); } catch { /* already gone */ }
}, bounded)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/harness/sandbox.js` around lines 116 - 154, Update the timeout
enforcement around the child process and finish handler so the container started
by the docker/podman client is given a unique name or identifier and is
explicitly stopped or killed on timeout, rather than relying only on
child.kill('SIGKILL'). Preserve the existing timeout result while ensuring
cleanup targets the actual container, and record any failure from the
container-kill operation instead of silently swallowing it.
Source: Coding guidelines
| child.stderr = new EventEmitter(); | ||
| child.kill = () => { child.killed = true; child.emit('close', null); }; | ||
| if (!hang) { | ||
| setImmediate(() => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ESLint config declares node/commonjs env or globals for the tests directory
fd -e json -e js -e cjs -e mjs eslintrc .
rg -n "env" -A5 $(fd -i eslintrc.json -o -i eslintrc.js 2>/dev/null | head -1) 2>/dev/null
rg -n "\"env\"|env:" package.json .eslintrc* eslint.config.* 2>/dev/nullRepository: richard-devbot/SDLC-rstack
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate ESLint/package files =="
fd -a -H -t f '(^|/)(eslint\.config\.[cm]?js|\.eslintrc(\..+)?|package\.json)$' .
echo
echo "== package.json (relevant lint bits) =="
if [ -f package.json ]; then
rg -n '"eslint|lint|test"|"type"|\"env\"|globals' package.json || true
fi
echo
echo "== eslint config contents (first 250 lines each) =="
for f in $(fd -a -H -t f '(^|/)(eslint\.config\.[cm]?js|\.eslintrc(\..+)?|package\.json)$' .); do
echo "--- $f ---"
sed -n '1,250p' "$f"
done
echo
echo "== test file around flagged line =="
sed -n '1,80p' tests/sandbox-execution-452.test.jsRepository: richard-devbot/SDLC-rstack
Length of output: 8422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "FILES:"
git ls-files 'package.json' '.eslintrc*' 'eslint.config.*' 'tests/sandbox-execution-452.test.js'
echo
for f in package.json .eslintrc* eslint.config.* tests/sandbox-execution-452.test.js; do
[ -f "$f" ] || continue
echo "===== $f ====="
sed -n '1,120p' "$f"
doneRepository: richard-devbot/SDLC-rstack
Length of output: 8359
Declare setImmediate for test files. eslint.config.js covers tests/**/*.js, but it omits setImmediate, so tests/sandbox-execution-452.test.js:26 can still trip no-undef. Add it to the test globals or use a Node test env.
🧰 Tools
🪛 ESLint
[error] 26-26: 'setImmediate' is not defined.
(no-undef)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/sandbox-execution-452.test.js` at line 26, Declare setImmediate as a
recognized global for tests/**/*.js in eslint.config.js, or enable the
appropriate Node test environment, so the setImmediate call in the sandbox
execution test passes no-undef validation.
Source: Linters/SAST tools
…out test) CI (cleaner event loop than local) surfaced 'Promise resolution is still pending but the event loop has already resolved' on the hung-command test: the timeout timer was unref'd, so with no other loop activity it never fired and the promise hung. A kill-timeout for a runaway container MUST fire — removed the unref (it is always cleared on close, so it never over-holds the loop). Verified stable x3 in isolation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Part of #452 (PR 1 of 3 — does not close the issue). Spec + threat model on the issue; posture signed off as container-only.
What this delivers (the core "Scientist")
runInSandbox(runDir, { taskId, command, network, timeoutMs })→ a harness-authoredexecutionevidence record from the real container exit code, so validation can stop trusting self-reportedtests_run.Container-only, locked down (per sign-off):
docker/podman. No unconfined child-process tier. No runtime →execution: unverified(statusobserved,exit_code: null) → gate degrades to contract validation, never unconfined, never false-green.--network none(default; opt-in bridge), no host env,--cap-drop ALL+no-new-privileges, non-root user,--read-onlyroot + 64m tmpfs, memory/pids/cpu caps, code mounted read-only, hard wall-clock timeout →SIGKILLthe tree.Verification
Injectable runtime probe + spawn, so CI proves it without a daemon: argv lockdown (no-net/caps/non-root/RO-mount/limits), docker→podman→null detection, exit-0 PASS / non-zero FAIL with captured stdout/stderr, timeout→kill→FAIL, and the no-runtime→unverified degrade. Full suite 1565/0 · typecheck + lint + security clean.
Next
sdlc_validate— execution evidence replaces the self-report at the gate; a failing run's stderr becomes the Structured Critique Loop-back: builders learn from their own failures #451 critique fed to the retry.doctorreports the active sandbox tier (container vs unverified).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests