fix(run): enforce sandbox and worktree isolation - #440
Conversation
Closes #437 Co-Authored-By: Cursor <cursoragent@cursor.com> Co-Authored-By: Codex <codex@openai.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change adds content-sensitive pre-run snapshots for ground-truth attribution, branch/HEAD drift checks throughout execution, explicit Claude sandbox enforcement, and dirty-worktree restrictions for primary checkouts. ChangesRun isolation and sandbox enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/brigade/agents.py (1)
102-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnrecognized
sandboxvalues are mislabeled as "no sandbox".The final
raise ValueError(_CLAUDE_NO_SANDBOX_ERROR)branch fires both whensandbox is Noneand whensandboxis any unrecognized string (e.g. a typo like"Read-Only"or"full-access")._CLAUDE_NO_SANDBOX_ERRORsays the run was "requested without an explicit sandbox," which is misleading when a sandbox value was actually supplied but not recognized.🛠️ Proposed fix: distinguish "no sandbox" from "unrecognized sandbox"
if sandbox == "danger-full-access": return [ "claude", "-p", "--dangerously-skip-permissions", "--disallowedTools", _CLAUDE_DISALLOWED_ALWAYS, prompt, ] - # sandbox is None: refuse to guess between stalling and granting full access. - raise ValueError(_CLAUDE_NO_SANDBOX_ERROR) + if sandbox is not None: + raise ValueError(f"claude does not support sandbox {sandbox!r}. " + _CLAUDE_NO_SANDBOX_ERROR) + # sandbox is None: refuse to guess between stalling and granting full access. + raise ValueError(_CLAUDE_NO_SANDBOX_ERROR)🤖 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/brigade/agents.py` around lines 102 - 122, Update _claude_argv so sandbox is validated separately: retain _CLAUDE_NO_SANDBOX_ERROR only when sandbox is None, and raise a distinct error for any unrecognized non-null sandbox value. Ensure recognized read-only, workspace-write, and danger-full-access behavior remains unchanged.
🤖 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/brigade/agents.py`:
- Around line 909-932: Distinguish sandbox rejection errors from other
build_argv failures in the agent dispatch handling. Update build_argv and the
exception handling around it to use or detect a dedicated
UnsupportedSandboxError, assigning unsupported-sandbox only for that case;
preserve clean failure handling for other ValueErrors with an accurate
failure_kind rather than mislabeling reasoning, model, or session validation
errors.
---
Nitpick comments:
In `@src/brigade/agents.py`:
- Around line 102-122: Update _claude_argv so sandbox is validated separately:
retain _CLAUDE_NO_SANDBOX_ERROR only when sandbox is None, and raise a distinct
error for any unrecognized non-null sandbox value. Ensure recognized read-only,
workspace-write, and danger-full-access behavior remains unchanged.
🪄 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: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f25d304-1ce4-4aff-ad3c-7ca3f783b61c
📒 Files selected for processing (10)
src/brigade/aboyeur.pysrc/brigade/agents.pysrc/brigade/cli/run.pysrc/brigade/runguard.pytests/test_aboyeur.pytests/test_agents.pytests/test_agents_model_pin.pytests/test_read_only_enforcement.pytests/test_run_cli.pytests/test_runguard.py
| try: | ||
| argv = build_argv( | ||
| cli_ref, | ||
| prompt, | ||
| read_only=read_only, | ||
| sandbox=sandbox, | ||
| model=model, | ||
| reasoning=reasoning, | ||
| cwd=cwd, | ||
| resume_session_id=resume_session_id, | ||
| ) | ||
| except ValueError as exc: | ||
| # A builder rejected the launch before spawning (e.g. claude | ||
| # workspace-write, which this CLI version cannot enforce). Fail the | ||
| # seat cleanly instead of crashing the run or stalling on a prompt. | ||
| return AgentResult( | ||
| text="", | ||
| ok=False, | ||
| detail=str(exc)[:200], | ||
| failure_phase="dispatch", | ||
| failure_kind="unsupported-sandbox", | ||
| requested_model=model, | ||
| reasoning=reasoning, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Broad except ValueError mislabels unrelated build_argv failures as unsupported-sandbox.
build_argv raises ValueError for several reasons that have nothing to do with sandbox rejection: unsupported reasoning pins (e.g. claude+reasoning="high", proven reachable by test_build_argv_rejects_reasoning_for_unsupported_adapter), ollama model-name conflicts, and grok exact-session validation (e.g. an empty resume_session_id reaches build_argv's own check since the earlier guard at Line 848 doesn't verify non-empty). All of these now get failure_kind="unsupported-sandbox" even though the real cause is unrelated, which is misleading for any caller/telemetry that branches on failure_kind.
🛠️ Proposed fix: use a dedicated exception type for sandbox rejections
+class UnsupportedSandboxError(ValueError):
+ """Raised when a builder cannot safely honor the requested sandbox/read-only mode."""
+
+
def _claude_argv(prompt: str, read_only: bool, sandbox: str | None, cwd: Path | None) -> List[str]:
if read_only or sandbox == "read-only":
return ["claude", "-p", "--disallowedTools", _CLAUDE_DISALLOWED_READ_ONLY, prompt]
if sandbox == "workspace-write":
- raise ValueError(_CLAUDE_WORKSPACE_WRITE_ERROR)
+ raise UnsupportedSandboxError(_CLAUDE_WORKSPACE_WRITE_ERROR)
...
- raise ValueError(_CLAUDE_NO_SANDBOX_ERROR)
+ raise UnsupportedSandboxError(_CLAUDE_NO_SANDBOX_ERROR) try:
argv = build_argv(...)
- except ValueError as exc:
+ except UnsupportedSandboxError as exc:
return AgentResult(
text="",
ok=False,
detail=str(exc)[:200],
failure_phase="dispatch",
failure_kind="unsupported-sandbox",
requested_model=model,
reasoning=reasoning,
)Note: narrowing the except to UnsupportedSandboxError alone would let other ValueErrors (reasoning/model/ollama misconfiguration) propagate uncaught again, as they apparently did before this diff. If that regression is unacceptable, keep the broad except ValueError but branch the failure_kind on isinstance(exc, UnsupportedSandboxError) instead of hardcoding it.
🤖 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/brigade/agents.py` around lines 909 - 932, Distinguish sandbox rejection
errors from other build_argv failures in the agent dispatch handling. Update
build_argv and the exception handling around it to use or detect a dedicated
UnsupportedSandboxError, assigning unsupported-sandbox only for that case;
preserve clean failure handling for other ValueErrors with an accurate
failure_kind rather than mislabeling reasoning, model, or session validation
errors.
There was a problem hiding this comment.
Fixed in 8b7f933. Sandbox rejections now use UnsupportedSandboxError and retain failure_kind=unsupported-sandbox; other build_argv ValueErrors return invalid-dispatch-args. Regression tests cover unsupported reasoning and unknown CLI cases.
Greptile SummaryThis PR enforces sandbox and worktree isolation for runs. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(run): close isolation review gaps" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-Authored-By: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
1 participant
Summary
danger-full-accessrequest before adding permission bypass.--allow-dirtyruns in a primary checkout and direct callers to a linked worktree.Root cause
The Claude adapter ignored
read_onlyandsandbox. Dirty-run ground truth compared the final worktree directly with HEAD, which mixed pre-existing changes into the receipt. Run finalization also lacked branch and HEAD invariants.Verification
brigade work verify run --target . --command "./scripts/verify" --capture brigade-work20260722-171948-work-verify-20d6b2Closes #437
Summary by CodeRabbit
New Features
Bug Fixes
brigade run --allow-dirtyis now rejected for dirty primary checkouts, while remaining supported for linked worktrees.