Conversation
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>
|
| 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
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
- 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>
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:
Security-review fixes:
Tests: 1000 unit tests pass (incl. real sandbox-exec SBPL compile on macOS). No new ruff/mypy violations introduced.