Repository navigation
feat(sandbox): native platform sandbox backends (ADR-0060) - #532
Conversation
Converge sandbox_mode to none|native|podman|docker and drop nsjail from validation (configs setting it are now rejected; NsjailSandboxRunner kept temporarily, unreachable, slated for removal). native resolves per-platform via _resolve_native() that build_sandbox() delegates to: - Darwin -> MacosSeatbeltSandboxRunner (sandbox-exec, deny-default profile) - Linux -> LinuxNativeSandboxRunner (bubblewrap, ro root + rw workspace) - Windows -> fail closed Add explicit PodmanSandboxRunner mirroring Docker. All backends fail closed when their binary is missing rather than silently downgrading. Default sandbox_mode stays none; flipping to native is deferred to a later PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 32 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR implements ADR-0060, converging sandbox modes by replacing ChangesSandbox Mode Convergence with Native Platform Backends
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/tests/tools/test_shell.py (1)
626-683:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLock down the Podman parity contract too.
These tests currently prove
--network noneand the workspace bind, but the PR contract also says Podman mirrors Docker’sno-new-privilegeshardening and explicit/validated env propagation. Without assertions for those, a Podman-specific privilege or env leak regression would still pass.Suggested follow-up coverage
assert command[:3] == ["podman", "run", "--rm"] assert "--network" in command assert command[command.index("--network") + 1] == "none" + assert "--security-opt" in command + assert command[command.index("--security-opt") + 1] == "no-new-privileges" root = str(workspace.resolve()) assert any( f"type=bind,src={root},dst={root}" in part for part in command ) assert command[-2:] == ["echo", "hi"]I’d also mirror the existing Docker env tests here so Podman is covered for:
- explicit env only
- rejection of unsafe env names
🤖 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 `@coding-agent/tests/tools/test_shell.py` around lines 626 - 683, The Podman test is missing parity checks for Docker hardening and env handling: update test_podman_runner_builds_podman_run_with_network_none_and_workspace_bind to also assert that the generated command includes "--no-new-privileges" and that environment propagation matches the Docker contract (i.e., only explicitly provided safe env entries from SandboxRequest.env are translated into "-e" / "--env" parts and unsafe names are not); additionally add two Podman-specific tests mirroring the Docker env coverage that call PodmanSandboxRunner.run (using SandboxRequest) to verify explicit env-only propagation and rejection of unsafe env names. Reference PodmanSandboxRunner.run, SandboxRequest, and the existing test functions when adding these assertions/tests.
🤖 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 `@coding-agent/src/coding_agent/tools/sandbox.py`:
- Around line 118-133: The native runner's run method ignores
SandboxConfig.limits so cpu_limit_seconds and memory_limit_mb are dropped;
update run (and the analogous native runner at lines 168-183) to apply
configured resource limits by reusing _resource_limit_preexec() as the
subprocess preexec_fn when limits are present, passing the request limits into
that helper, and include the preexec_fn in the subprocess.run call; if a
specific limit cannot be enforced by the native backend, explicitly raise
SandboxUnavailableError or ValueError indicating the unsupported limit instead
of silently dropping it.
- Around line 125-133: The subprocess.run invocation in bash_run (and the
similar call later) currently forwards request.env (which may be None) into
subprocess.run causing inheritance of the host environment; instead construct
and pass an explicit minimal env allowlist (e.g., only PATH, HOME or specific
safe keys) or an empty dict when request.env is None, merging request.env only
after validating/whitelisting keys; update the calls in bash_run and the second
run site to pass that sanitized env dict rather than forwarding None so the
sandbox cannot see host secrets and adheres to the ADR’s explicit/controlled env
contract.
In `@coding-agent/tests/tools/test_shell.py`:
- Around line 565-595: The test
test_linux_native_command_includes_network_off_workspace_bind_and_cwd must also
assert the read-only root bind from ADR-0060; after capturing the generated
command (captured["command"] / variable command) add an assertion that the
command contains the '--ro-bind' token and that the following two entries are
'/' and '/' (i.e., verify the sequence ["--ro-bind", "/", "/"]) so the bwrap
invocation enforces a read-only root mount; keep this check alongside the
existing workspace bind and --chdir assertions.
---
Outside diff comments:
In `@coding-agent/tests/tools/test_shell.py`:
- Around line 626-683: The Podman test is missing parity checks for Docker
hardening and env handling: update
test_podman_runner_builds_podman_run_with_network_none_and_workspace_bind to
also assert that the generated command includes "--no-new-privileges" and that
environment propagation matches the Docker contract (i.e., only explicitly
provided safe env entries from SandboxRequest.env are translated into "-e" /
"--env" parts and unsafe names are not); additionally add two Podman-specific
tests mirroring the Docker env coverage that call PodmanSandboxRunner.run (using
SandboxRequest) to verify explicit env-only propagation and rejection of unsafe
env names. Reference PodmanSandboxRunner.run, SandboxRequest, and the existing
test functions when adding these assertions/tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cd52334-90c7-47e2-87fe-006fb43982c4
📒 Files selected for processing (4)
coding-agent/docs/adr/0060-native-platform-sandbox-backends.mdcoding-agent/src/coding_agent/tools/sandbox.pycoding-agent/src/coding_agent/tools/shell.pycoding-agent/tests/tools/test_shell.py
- Critical: native runners no longer inherit the host environment. _native_sandbox_env() exposes only a minimal allowlist when no env is supplied, and uses explicit env verbatim (validated) otherwise — honoring ADR-0060's explicit/controlled-env contract. - Apply SandboxConfig.limits in native runners via _resource_limit_preexec(); reject memory limits on macOS Seatbelt (consistent with none mode) instead of silently dropping them. - Tests: assert bwrap --ro-bind / /, assert native env is not host-inherited, cover _native_sandbox_env allowlist/explicit/unsafe-name paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assert podman uses --security-opt no-new-privileges, and add explicit-env-only and unsafe-env-name tests mirroring the Docker coverage, so a Podman-specific privilege or env-leak regression is caught. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review Addressed all four findings from the previous review:
|
|
✅ Action performedFull review finished. |
Summary
Implements ADR-0060: converge
sandbox_modeand add platform-native sandbox backends, copying the shape of Codex's macOS/Linux sandboxes without expanding the config surface.sandbox_modeconverges tonone | native | podman | dockerNsjailSandboxRunnerleft temporarily, unreachable, slated for removal.nativeresolves per-platform via_resolve_native()thatbuild_sandbox()delegates to:MacosSeatbeltSandboxRunner(sandbox-exec, deny-default profile: reads allowed, writes confined to workspace + scratch, network denied)LinuxNativeSandboxRunner(bubblewrap:--ro-bind / /+ rw workspace bind,--unshare-net, cwd pinned)PodmanSandboxRunneradded as an explicit container backend (mirrors Docker;--network none,no-new-privileges, workspace bind). Never auto-selected bynative.none).none— flipping tonativeis deferred to a later PR.Test plan
uv run pytest tests/tools/test_shell.py→ 42/42 pass (16 new tests)uv run ruff check src/coding_agent/tools/sandbox.py src/coding_agent/tools/shell.pyReviewer notes / follow-ups (deferred per ADR)
native.sandbox_mode = "nsjail"now rejected — needs a changelog note. No in-repo config uses it (agent.tomldefaults tonone).native; remove orphanedNsjailSandboxRunner.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests