Skip to content

Ralph two-mechanism sandbox + security-review hardening#3

Merged
shuveb merged 2 commits into
mainfrom
sandbox
May 17, 2026
Merged

Ralph two-mechanism sandbox + security-review hardening#3
shuveb merged 2 commits into
mainfrom
sandbox

Conversation

@shuveb
Copy link
Copy Markdown
Contributor

@shuveb shuveb commented May 17, 2026

Adds the Ralph orchestrator's two-mechanism, hard-fail sandbox (macOS sandbox-exec / Linux bwrap; no unsandboxed mode) and applies the fixes from a multi-agent security review of the change.

Sandbox feature:

  • sandbox/ package: models, detect, exceptions, wrapper, adapters (sandbox_exec + bwrap), packaged SBPL profile (claude.sb).
  • Detection hard-fails before any agent subprocess; AgentRunner always pairs the OS sandbox with --dangerously-skip-permissions.

Security-review fixes:

  • C1: post-construction SandboxError now propagates out of the orchestrator (re-raise before the broad except) and hard-aborts via CLI/TUI wiring instead of being downgraded to a per-feature failure.
  • C2: SBPL /dev grant narrowed from a blanket (subpath "/dev") to a curated device-node allow-list; comment corrected to a bounded trade-off. Verified to compile under real sandbox-exec.
  • H1: line-anchored denial-signature matching ([Sandbox] prefix stripped); truncated stderr-drain / unattributed non-zero exit now logged instead of silent.
  • I2: fail-loud SandboxPermissions/SandboxConfig validation (no writable filesystem-root / bare $HOME; project_dir agreement).
  • I3: subprocess cwd pinned to the sandbox's realpath'd project_dir.
  • I4: TUI fail-closed paths now covered by tests.
  • I5: stale docker refs and drift-prone _DEFAULT_BASE_URL removed; CLAUDE.md synced.
  • Hardening: import-time dispatch-totality assertion; bwrap --die-with-parent/--chdir; control-char sanitization of surfaced sandbox stderr; observability/log-severity parity.

Tests: 1000 unit tests pass (incl. real sandbox-exec SBPL compile on macOS). No new ruff/mypy violations introduced.

Adds the Ralph orchestrator's two-mechanism, hard-fail sandbox
(macOS sandbox-exec / Linux bwrap; no unsandboxed mode) and applies
the fixes from a multi-agent security review of the change.

Sandbox feature:
- sandbox/ package: models, detect, exceptions, wrapper, adapters
  (sandbox_exec + bwrap), packaged SBPL profile (claude.sb).
- Detection hard-fails before any agent subprocess; AgentRunner always
  pairs the OS sandbox with --dangerously-skip-permissions.

Security-review fixes:
- C1: post-construction SandboxError now propagates out of the
  orchestrator (re-raise before the broad except) and hard-aborts via
  CLI/TUI wiring instead of being downgraded to a per-feature failure.
- C2: SBPL /dev grant narrowed from a blanket (subpath "/dev") to a
  curated device-node allow-list; comment corrected to a bounded
  trade-off. Verified to compile under real sandbox-exec.
- H1: line-anchored denial-signature matching ([Sandbox] prefix
  stripped); truncated stderr-drain / unattributed non-zero exit now
  logged instead of silent.
- I2: fail-loud SandboxPermissions/SandboxConfig validation
  (no writable filesystem-root / bare $HOME; project_dir agreement).
- I3: subprocess cwd pinned to the sandbox's realpath'd project_dir.
- I4: TUI fail-closed paths now covered by tests.
- I5: stale docker refs and drift-prone _DEFAULT_BASE_URL removed;
  CLAUDE.md synced.
- Hardening: import-time dispatch-totality assertion; bwrap
  --die-with-parent/--chdir; control-char sanitization of surfaced
  sandbox stderr; observability/log-severity parity.

Tests: 1000 unit tests pass (incl. real sandbox-exec SBPL compile on
macOS). No new ruff/mypy violations introduced.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 17, 2026

Greptile Summary

This PR introduces a mandatory two-mechanism sandbox — sandbox-exec on macOS and bwrap on Linux — that is required before any agent subprocess starts. Detection hard-fails and there is no unsandboxed fallback; post-detection sandbox failures re-raise out of the orchestrator loop and are handled as session-aborting errors in the TUI.

  • Sandbox infrastructure (sandbox/ package): SandboxMechanism, SandboxConfig, SandboxPermissions, SandboxWrapper, adapters, and exception hierarchy; import-time dispatch-totality assertion; _assert_confining validation at construction; [Sandbox]-prefixed stderr attribution and adapter-specific exit classifiers.
  • Agent/orchestrator wiring: AgentRunner.run() now wraps every Claude invocation with the detected mechanism and pins subprocess cwd to the sandbox's realpath'd project dir; sandbox errors propagate hard out of _implement_feature rather than being downgraded to per-feature failures.
  • SBPL profile hardening: blanket (subpath \"/dev\") replaced with a curated device-node allow-list; SBPL template token uniqueness enforced at render_profile time.
  • TUI: _exit_confirm_open guard prevents stacked confirmation modals; _safe_call_from_thread used for all post-construction cross-thread calls; RalphTUIDisplay._call_from_thread suppresses RuntimeError on teardown; SandboxError caught and displayed as a visible abort notification.

Confidence Score: 5/5

The sandbox implementation is correct and the hard-fail semantics are consistently enforced across detection, setup, runtime, and TUI abort paths. Safe to merge.

Detection, adapter dispatch, SBPL profile rendering, bwrap argument assembly, exception hierarchy, and TUI abort wiring all behave as described. The _assert_confining correctly expands tilde-relative paths before the root/$HOME check, the SBPL /dev grant is correctly narrowed to a curated literal allow-list, the import-time totality assertion prevents silent adapter gaps, and every cross-thread call in the post-bwrap-probe window uses the RuntimeError-tolerant helper. The one observation (double-write of captured sandbox stderr in _emit_sandbox_error) is a cosmetic redundancy, not a correctness or security defect.

No files require special attention. The _emit_sandbox_error double-output issue in agent.py is cosmetic and does not affect sandbox correctness.

Important Files Changed

Filename Overview
src/mfbt/commands/ralph/sandbox/models.py New data models: SandboxMechanism enum, SandboxPermissions with _assert_confining (correctly handles tilde-relative paths via os.path.expanduser), SandboxConfig with project_dir/permissions agreement check, and resolve_paths with symlink canonicalization for macOS sandbox correctness
src/mfbt/commands/ralph/sandbox/detect.py New detector: strict platform gating (Darwin → sandbox-exec binary presence; Linux → bwrap functional liveness probe with 5s timeout); no unsandboxed fallback; raises SandboxDetectionError with actionable install-hint messages
src/mfbt/commands/ralph/sandbox/adapters/init.py Adapter dispatch with import-time totality assertion over SandboxMechanism; a new enum member without both build_prefix and classify_exit entries fails at import rather than at first use
src/mfbt/commands/ralph/sandbox/adapters/sandbox_exec.py New adapter: renders SBPL profile from template, writes to temp file, builds sandbox-exec -f prefix; exit classifier strips [Sandbox] prefix before line-anchored pattern matching for deny(\d+) and sandbox-exec:
src/mfbt/commands/ralph/sandbox/adapters/bwrap.py New adapter: constructs bwrap prefix with --die-with-parent, --chdir, ro-bind system paths (skipping absent ones), and rw-bind for resolved writable paths; exit classifier anchors bwrap: match to line-start after stripping the [Sandbox] wrapper prefix
src/mfbt/commands/ralph/sandbox/wrapper.py New SandboxWrapper: binds mechanism to config; wrap_command prepends mechanism prefix and tracks temp files for cleanup; note_stderr_line accumulates [Sandbox]-prefixed stderr; classify_exit delegates to adapter classifier
src/mfbt/commands/ralph/agent.py Sandbox-wired AgentRunner: subprocess cwd pinned to sandbox realpath'd project dir; EPERM/EACCES at Popen mapped to SandboxDeniedError; typed per-subclass error surfacing via _emit_sandbox_error; stderr truncation logged with appropriate severity; control-char sanitization before replaying captured stderr to terminal
src/mfbt/commands/ralph/orchestrator.py Detection moved to init (hard-fail before any agent run); SandboxError re-raised from _implement_feature instead of downgraded to per-feature failure; SandboxDetectionError logged at error with traceback parity
src/mfbt/commands/ralph/profiles/claude.sb New SBPL profile: (allow default) base, global (deny file-write* (subpath "/")), then curated device-node allow-list and per-path writable re-grants via %%WRITABLE_PATHS%% substitution; no network restrictions
src/mfbt/tui/screens/feature_list.py TUI wiring: SandboxDetectionError caught at construction with visible notification and return-to-browsing; post-construction SandboxError caught and hard-aborted; _safe_call_from_thread used for all cross-thread calls after bwrap probe window
src/mfbt/tui/app.py Confirmation modal added for quit/back while Ralph is running; _exit_confirm_open flag prevents double modal; _ralph_active_panel() correctly gates only RUNNING/PAUSED states
src/mfbt/commands/ralph/tui_display.py _call_from_thread now suppresses RuntimeError on teardown; new sandbox_status method renders mechanism name in agent-output log with debug-level fallback if the widget is absent

Sequence Diagram

sequenceDiagram
    participant TUI as TUI Worker
    participant Orch as RalphOrchestrator
    participant Det as detect_sandbox()
    participant Wrap as SandboxWrapper
    participant Agent as AgentRunner
    participant Proc as subprocess (bwrap/sandbox-exec → claude)

    TUI->>Orch: __init__()
    Orch->>Det: detect_sandbox()
    Det-->>Orch: SandboxMechanism (or raises SandboxDetectionError)
    Orch->>Wrap: SandboxWrapper(mechanism, config)
    Orch->>Agent: "AgentRunner(..., sandbox_wrapper=wrapper)"
    TUI->>Orch: run()
    Orch->>Agent: run(prompt, on_output, on_stderr)
    Agent->>Wrap: prepare()
    Agent->>Wrap: wrap_command(cmd)
    Wrap-->>Agent: [bwrap/sandbox-exec, ...flags, claude, -p, ...]
    Agent->>Proc: "Popen(wrapped_cmd, cwd=realpath(project_dir))"
    Note over Proc: Sandbox enforces write-confinement
    Proc-->>Agent: stdout (stream-json JSONL)
    Proc-->>Agent: stderr ([Sandbox] lines)
    Agent->>Wrap: note_stderr_line(line) x N
    Agent->>Proc: wait()
    Agent->>Wrap: classify_exit(returncode)
    alt Sandbox denial detected
        Wrap-->>Agent: SandboxDeniedError
        Agent->>Agent: _emit_sandbox_error(exc)
        Agent-->>Orch: raises SandboxError
        Orch-->>TUI: re-raises (hard-fail)
        TUI->>TUI: notify(Sandbox failure) + return_to_browsing
    else Clean exit
        Wrap-->>Agent: None
        Agent-->>Orch: AgentResult
        Orch->>Orch: verify_feature_completed()
    end
Loading

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/mfbt/commands/ralph/agent.py:273-285
For a `SandboxDeniedError` raised from `classify_exit`, the exception's `detail` field is set to `sandbox_stderr.strip()` — i.e. the full accumulated `[Sandbox]`-prefixed stderr — and that content is already embedded in `str(exc)`, which becomes part of `line`. Then `captured = sanitize(get_sandbox_stderr())` pulls the identical content again and writes it a second time. The operator sees the sandbox stderr twice: once inside the `"[ralph] sandbox denied access: [bwrap] …"` prefix and once as the raw lines appended below it. This also makes `line` a multi-line string that is passed as a single argument to `notify()`, which may not render as expected in the TUI.

```suggestion
        if isinstance(exc, SandboxSetupError):
            line = f"[ralph] sandbox setup failed: {exc.message}"
        elif isinstance(exc, SandboxDeniedError):
            line = f"[ralph] sandbox denied access: {exc.message}"
        elif isinstance(exc, SandboxCompatibilityError):
            line = (
                f"[ralph] sandbox compatibility check failed: {exc.message}\n"
                "[ralph] the platform sandbox cannot run the agent here — "
                "only sandbox-exec (macOS) and bwrap (Linux) are supported; "
                "ensure the required tool is installed and functional"
            )
        else:
            line = f"[ralph] sandbox error: {exc.message}"
```

Reviews (2): Last reviewed commit: "Fix 4 sandbox/TUI review issues + clear ..." | Re-trigger Greptile

Comment thread src/mfbt/tui/screens/feature_list.py Outdated
Comment thread src/mfbt/tui/screens/feature_list.py Outdated
- feature_list.py: route the post-construction sandbox-header update
  through _safe_call_from_thread so a quit during the bwrap liveness
  probe no longer raises NoActiveAppError into the outer handler.
- feature_list.py: drop the dead _sandbox_mechanism_value write
  (no read site); RalphHeader.sandbox is the sole persistence.
- models.py: _assert_confining now expanduser-expands paths before the
  root/$HOME structural checks so a tilde-relative Path("~") is rejected
  at construction instead of expanding to $HOME in the adapters
  (env-var read only, no-FS-at-construction contract preserved).
- sandbox_exec.py: document the accepted deny(N) shared-fd
  false-positive trade-off explicitly (no behavior change).
- tests: tilde-confinement regression coverage; update header test for
  the removed cache attr; reflow an over-length dict literal (E501).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shuveb shuveb merged commit 827234e into main May 17, 2026
2 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant