feat: add M6 sandbox backend#77
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- The
internal/sandboxpackage is well-factored.types.gois pure data (typed strings for every classification, plusPolicy,Request,Decision,Risk,Violation,Grant).normalize.gois a small input-validation layer.risk.godoes classification.paths.godoes workspace enforcement.grants.godoes persistence.engine.gois the policy evaluator.adapters.gois platform selection. Each file is single-purpose and easy to test in isolation, and the package depends only onredactionfrom earlier slices. - The engine is conservative by default and degrades predictably.
DefaultPolicyisMode: enforce,Network: deny,EnforceWorkspace: true,DenyDestructiveShell: true, with a policy-only backend on platforms that do not havebwraporsandbox-exec.SelectBackendreportsAvailable: falseand aMessagefor the fallback case, andBuildPlanalways documents the active restrictions, including "platform sandbox unavailable; policy engine still evaluates tool requests". Users running on Windows or a stripped-down Linux image get a clear signal that the OS-level sandbox is not engaged. - Workspace enforcement is symlink-aware and consistent with the read/write tools.
validateWorkspacePathsresolves the workspace root viafilepath.EvalSymlinks, joins andAbs-normalizes the target, and then walks the relative path segment-by-segment, rejecting anyLstatsymlink whose resolved target escapes the root. The sameoutside_workspaceandsymlink_traversalviolations are raised whether the path is absolute, relative, or traverses a pre-existing symlink. This matches the gate logic in the existingwrite_file/read_filetools and gives the agent runtime defense-in-depth. - Risk classification is a separate concern from policy.
ClassifyreadsSideEffect, scanscommandfornetworkCommandPattern,destructiveCommandPattern, and piped installers (| sh,| bash), and flagspath_escapefor..and absolute paths. Categories are sorted for stable JSON. The engine then usesHasRiskCategoryto decide which policy clauses apply. This split means a future slice can add a new risk category without touching the engine. - Persistent grants are first-class and well-protected.
GrantStorevalidates tool names against^[A-Za-z0-9._-]+$(rejecting empty and../escape), normalizes decision and autonomy, redactsReasonon both write and load, persists with atomic temp+rename under a 0o600 file in a 0o700 directory, and protects writers with async.Mutex. Schema versioning is in place (schemaVersion: 1with explicit rejection of other versions), so a v2 migration is one branch away.Lookupmatches grants only whenrequestedAutonomy <= grant.MaxAutonomy(low/medium/high), so amediumgrant does not satisfy ahighrequest. - Sandbox is enforced in the right places.
tools.Registry.RunWithOptionsevaluates the sandbox before the tool runs and turnsActionDenyinto a structured error result ("Sandbox violation [code] for tool at path: reason").ActionPromptwithoutPermissionGrantedreturns a structured "Sandbox approval required" error, so the agent sees a clear next step. The agent runtime forwardsAutonomyandSandboxto the registry, andTestRunAppliesSandboxEvenInUnsafeModelocks in that the sandbox still applies whenPermissionModeUnsafeis set, which is exactly the M6 promise. - CLI surface is consistent with the rest of the codebase.
zero sandbox policyprints either text (Zero sandbox policy / root / mode / network / backend / backend_available / grants) or JSON with{policy, backend, plan, grantsPath}.zero sandbox grants {list,allow,deny,revoke,clear}mirrors the MCP grants surface from #40 and uses the same--jsonand--confirmpatterns.--confirmis required forclear, andTestRunSandboxHelpDoesNotOpenStoreproves that help paths do not touch the store.nextFlagValueis reused for--autoand--reason, so the parser is consistent withzero execand does not regress the flag-shaped-value gap from #54. - Redaction is applied at the persistence boundary.
Grant.Reasonis redacted increateGrant(on the way in) and again innormalizeStoredGrant(on the way out), so a secret reason saved by a user or hand-edited in the JSON file is rewritten as[REDACTED]on next load. The grants JSON file therefore never stores raw secrets even if a user pastes one in. - Backend selection is injectable.
SelectBackendtakesBackendOptions{GOOS, LookupExecutable}with sensible defaults (runtime.GOOS,exec.LookPath), andappDeps.selectSandboxBackendlets tests pin the backend topolicy-only.TestRunSandboxPolicyInspectTextAndJSONexercises both text and JSON without depending on the host'sbwrap/sandbox-execinstall, which keeps CI portable. - Context cancellation is handled explicitly.
Engine.Evaluatechecksctx.Err()at entry and returnsViolationContextCanceledwith a redacted reason, andTestEngineReportsContextCancellationexercises it. The grant store'sLookuppath is not in the cancellation critical section (it is a fast read), which is appropriate.
Observations (non-blocking)
-
Lookupis not mutex-protected.Grant/Revoke/Clearhold the mutex while reading, mutating, and rewriting the file, butLookupreads the file outside the lock. For a single-process CLI this is fine, but a concurrentGrant+Lookupcan race:Lookupmay see the pre-grant state and returnMatched: false, orGrantmay read pre-revoke state and rewrite an entry the caller intended to remove. Ifinternal/agentever callsLookupfrom a background goroutine, prefersync.RWMutexwith a cachedgrantFilesnapshot, or move the read inside the mutex. -
TOCTOU between sandbox check and tool execution.
Engine.Evaluatechecks the path, but the actual write happens ininternal/tools/write_file.goafter the registry returns. A pre-created symlink placed between the two checks (or created by a parallel process) could still escape the workspace. The existing write tool's per-segmentLstatrejection is the real defense; the sandbox check is a fast pre-filter. Worth a comment on the engine noting that the write tool is the source of truth for the file boundary. -
Risk regex is best-effort.
networkCommandPatterncoverscurl,wget,scp,ssh,rsync,nc/netcat,python -m http.server,npm/pnpm/yarn/bun add|install|publish|login,pip install,go get,git clone,gh release download. A payload likepython3 -c "import urllib.request, os; os.system('curl ...')"does not match, and neither doesbash -c 'curl ...'. The shell tool isSideEffectShellso the engine still classifies it asRiskHigh; users in unsafe mode opt into this. A future slice should consider a stronger detection story (e.g., a shell-aware AST or a vetted allowlist of commands). -
Piped-installer detection uses
strings.Contains.| shand| bashare caught, but> sh,exec sh, backticks, oreval "curl"are not. Again, classified asRiskHigh; the user explicitly approves by being in unsafe mode. The detection is a hint, not a guarantee. -
RiskReasonalways emits the level, but the level is duplicated withDecision.Risk.Level. A consumer that prints a JSON decision already has both fields. KeepingRiskReasonis fine for text output, but a future refactor could derive it at format time and drop the field. -
Engine does not use the platform backend at runtime.
SelectBackendandBuildPlanare informational; the engine does not actually wrapbashinvocations inbwrap/sandbox-exec. The M6 PR description explicitly says this is the backend contract and that platform integration comes later. The currentBackendPlanis a good debug surface forzero sandbox policy, but the engine should be revisited when a future slice actually wires the platform sandbox. -
AllowPolicyOnlyRunneris inPolicybut unused by the engine. The default istrueand there is no engine branch that reads it. Either it is a future knob (e.g., fail closed when the platform sandbox is unavailable and the policy says "no") or it should be removed. Keeping it is fine if a follow-up PR will reference it, but the current engine does not enforce it. -
EngineOptions.Backendhas no default for the zero value.NewEngineaccepts aBackendvalue, andbuildExecSandboxEnginealways passes a result fromSelectBackend, but a test that constructsNewEngine(EngineOptions{WorkspaceRoot: ...})directly will seeBackend.Name == "". The engine does not look atBackendfor evaluation, so this is harmless today, but worth adefaultBackend()helper for future engine branches that do. -
runExecbuilds a newGrantStoreon every invocation viadeps.newSandboxStore. That is fine (the file is small, the reads are infrequent), but if a future slice needs to cache a per-process snapshot, the store has noSnapshot()helper. A small addition would be aLookupthat takes a snapshot to avoid file I/O on every tool call. -
runExecdoes not pass the per-exec autonomy to the engine. The agent runtime readsOptions.Autonomyand forwards it on everyRunWithOptionscall, so the engine does see it. The split betweenbuildExecSandboxEngine(no autonomy) andagent.Run(autonomy per request) is correct, but a reader ofcli/exec.gomay wonder why autonomy is not threaded at engine construction. A small comment would help. -
runWithDepsdoes not add asandboxtest to the no-TUI list atapp_test.go. It does (TestRunCommandsDoNotLaunchTUIincludes{"sandbox"}), andassertHelpOutputincludes"sandbox". The follow-up is just noting the test coverage is consistent with the other commands. -
TestRunSandboxGrantsAllowListDenyRevokeAndClearassertslen(listPayload.Grants) != 2and a specific sort order. TheStore.Listhelper sorts tool names alphabetically, sobashcomes beforewrite_file. The test relies on that ordering. A test that addsaptandcatafterbashwould confirm the sort is stable across more than two entries. Minor. -
parseSandboxPositionalOptionsdefaults autonomy toAutonomyLowif--autois not provided. The CLI then sends aLow-autonomy request to the store, which means a tool that was granted atMediumwill not be matched for aLowrequest... wait,autonomyAllowedusesrequested <= max, so aLowrequest matches aMediumgrant. Good. But if a tool was granted atLowonly, aMediumrequest would not match — that is the intended semantic. Worth documenting on the grant help text. -
runSandboxGrantsusesparseSandboxCommandOptionsfor thelistsubcommand but the parser disallows any positional argument. If a user runszero sandbox grants list extra, they get a usage error. That is the right behavior; the test does not cover it, but the production code rejects the input. No action needed. -
BuildPlanreturns a flat[]stringof restrictions. The text format prints each on a separate line, but a JSON consumer sees a list of free-form strings. A typedRestrictionenum (filesystem,network,destructive,platform_unavailable) would be more discoverable for tools that consume the JSON. Cosmetic; can wait for a real consumer. -
Engine.Evaluatealways overridesrequest.WorkspaceRootwithengine.workspaceRootviafirstNonEmpty. That is the right precedence (engine-level wins), but a per-request override is silently ignored. If a future slice wants to evaluate a tool against a sub-workspace (e.g., a worktree), the precedence needs an explicitRequest.WorkspaceRootfield that the engine respects. -
runExecdoes not currently pass--autoto the sandbox at construction time. A user runningzero exec --auto highwould setoptions.autonomy = "high"(per earlier exec wiring), which is forwarded toagent.Options.Autonomyand then to the registry. The path is correct; the gap is that the sandbox engine itself is not parameterized by autonomy, so a future "autonomy ceiling" knob would need to be added toEngineOptions. -
The
WriteFiletool is exercised by the sandbox test, butReadFile,Bash,UpdatePlan, etc. are not. The engine tests cover all of these via directRequestconstruction, which is the right level for engine coverage. A future PR that registers more tools should add aTestRegistryAppliesSandboxFor<EveryTool>matrix test if the per-tool safety metadata diverges from the engine's risk classification. -
Engine.EvaluatereturnsDecision{Action: ActionAllow, Risk: ..., Reason: "sandbox policy disabled"}whenpolicy.Mode == ModeDisabled. That is correct, but theRiskfield is still computed. Consumers that printDecision.Riskfor a disabled sandbox will see a populated risk object even though the decision is unconditional allow. Either dropRiskin the disabled branch or document the invariant. -
grantSchemaVersion = 1is a private constant. If a future migration script wants to introspect the version, it has to read the file. A small exportedCurrentGrantSchemaVersionwould help tooling. Minor.
No blockers. The M6 backend contract is in place: structured policy engine, risk classifier, persistent allow/deny grants, Linux/macOS/policy-only backend selection, enforcement in the tool registry and agent runtime, and a consistent zero sandbox {policy,grants ...} CLI surface. Tests cover workspace symlinks, out-of-workspace paths, network/destructive shell risk, persistent allow/deny matching, autonomy rank filtering, schema-version rejection, malformed-file rejection, atomic write durability (via reopen), context cancellation, and the unsafe-mode invariant. The two follow-ups I would prioritize next: (1) make Lookup concurrency-safe (RWMutex + cached snapshot) before the agent runtime calls it from background goroutines, and (2) document or remove AllowPolicyOnlyRunner (currently unused) so the policy struct matches what the engine actually enforces.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds a sandbox subsystem: typed policy model, risk classifier, workspace/path validation, persistent grant store, backend selection, CLI management commands, and integration that enforces sandbox decisions before tool execution across agent and exec flows. ChangesSandbox Core System and Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
internal/cli/sandbox_test.go (1)
13-100: ⚡ Quick winConsider adding error path coverage in follow-up work.
The test validates the happy path lifecycle comprehensively. For completeness, consider adding tests for error scenarios: invalid autonomy values (e.g.,
--auto invalid), store failures, revoking non-existent grants, and clearing an empty store. These would increase confidence that error messages are user-friendly.🤖 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 `@internal/cli/sandbox_test.go` around lines 13 - 100, Add negative-path tests to internal/cli/sandbox_test.go alongside TestRunSandboxGrantsAllowListDenyRevokeAndClear: cover (1) invalid autonomy by calling runWithDeps with e.g. {"sandbox","grants","allow","write_file","--auto=invalid","--json"} and assert a usage/validation exit code and stderr contains an autonomy error, (2) store failures by providing appDeps.newSandboxStore that returns an error and asserting commands return failure and surface the store error, (3) revoking a non-existent grant by calling runWithDeps revoke on a name not added and asserting the JSON revoked==0, and (4) clearing an already-empty store by ensuring clear with --confirm returns cleared==0; reuse runWithDeps, sandbox.GrantStore/test helpers (e.g. newSandboxTestStore) to inject the failing store behavior.internal/sandbox/adapters.go (1)
52-55: ⚡ Quick winAvoid mutating input parameters.
Line 54 reassigns the
policyparameter directly, which violates Go's convention of treating input parameters as read-only. While this doesn't cause a bug (sincePolicyis passed by value), it reduces clarity.♻️ Proposed fix using a local variable
func (backend Backend) BuildPlan(workspaceRoot string, policy Policy) BackendPlan { + effectivePolicy := policy if policy.Mode == "" { - policy = DefaultPolicy() + effectivePolicy = DefaultPolicy() } restrictions := []string{} - if policy.EnforceWorkspace { + if effectivePolicy.EnforceWorkspace { restrictions = append(restrictions, "filesystem writes must stay inside workspace") } - if policy.Network == NetworkDeny { + if effectivePolicy.Network == NetworkDeny { restrictions = append(restrictions, "network access denied unless a future adapter grants it explicitly") } - if policy.DenyDestructiveShell { + if effectivePolicy.DenyDestructiveShell { restrictions = append(restrictions, "destructive shell patterns denied before execution") } if backend.Name == BackendPolicyOnly { restrictions = append(restrictions, "platform sandbox unavailable; policy engine still evaluates tool requests") } return BackendPlan{ Backend: backend, WorkspaceRoot: workspaceRoot, - Policy: policy, + Policy: effectivePolicy, Restrictions: restrictions, } }🤖 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 `@internal/sandbox/adapters.go` around lines 52 - 55, The BuildPlan function currently reassigns the input parameter policy; change this to use a new local variable (e.g., p) so you don't mutate the parameter: copy policy into a local (p := policy), check/assign default with p if needed (if p.Mode == "" { p = DefaultPolicy() }), and then use p throughout the rest of BuildPlan instead of policy; this keeps the input parameter immutable and follows Go conventions.
🤖 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 `@internal/sandbox/grants.go`:
- Around line 151-166: The List method reads shared state without holding the
store.mu lock, causing a race; modify GrantStore.List to acquire store.mu.Lock()
before calling readState() and release it with defer store.mu.Unlock() (mirror
the locking pattern used in Lookup), ensuring the entire read and subsequent
iteration over state.Grants occurs under the mutex to prevent concurrent
read/write races.
- Around line 132-149: Acquire the store mutex at the start of Lookup before
calling readState to prevent concurrent mutation races: in Lookup (the method
using readState and returning GrantLookup) call store.mu.Lock() and defer
store.mu.Unlock() prior to invoking store.readState(), mirroring how
Grant/Revoke/Clear protect state; keep ValidateToolName/NormalizeAutonomy usage
as appropriate but ensure the readState access is guarded by the lock.
In `@internal/sandbox/risk.go`:
- Around line 47-49: The piped installer detection is case-sensitive (uses
strings.Contains(command, "| sh") / "| bash") and can be bypassed with
capitalized variants; update the check that triggers add("piped_installer",
RiskCritical) to perform a case-insensitive match (e.g., convert command to
lower-case before checking or use a (?i) regex like the other
network/destructive patterns) so "| SH", "| Sh", "| BASH", etc. are caught
consistently.
---
Nitpick comments:
In `@internal/cli/sandbox_test.go`:
- Around line 13-100: Add negative-path tests to internal/cli/sandbox_test.go
alongside TestRunSandboxGrantsAllowListDenyRevokeAndClear: cover (1) invalid
autonomy by calling runWithDeps with e.g.
{"sandbox","grants","allow","write_file","--auto=invalid","--json"} and assert a
usage/validation exit code and stderr contains an autonomy error, (2) store
failures by providing appDeps.newSandboxStore that returns an error and
asserting commands return failure and surface the store error, (3) revoking a
non-existent grant by calling runWithDeps revoke on a name not added and
asserting the JSON revoked==0, and (4) clearing an already-empty store by
ensuring clear with --confirm returns cleared==0; reuse runWithDeps,
sandbox.GrantStore/test helpers (e.g. newSandboxTestStore) to inject the failing
store behavior.
In `@internal/sandbox/adapters.go`:
- Around line 52-55: The BuildPlan function currently reassigns the input
parameter policy; change this to use a new local variable (e.g., p) so you don't
mutate the parameter: copy policy into a local (p := policy), check/assign
default with p if needed (if p.Mode == "" { p = DefaultPolicy() }), and then use
p throughout the rest of BuildPlan instead of policy; this keeps the input
parameter immutable and follows Go conventions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e83454a-1c60-4981-a842-ddbebb0b9616
📒 Files selected for processing (20)
internal/agent/loop.gointernal/agent/loop_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/app_test.gointernal/cli/exec.gointernal/cli/sandbox.gointernal/cli/sandbox_test.gointernal/sandbox/adapters.gointernal/sandbox/adapters_test.gointernal/sandbox/engine.gointernal/sandbox/engine_test.gointernal/sandbox/grants.gointernal/sandbox/grants_test.gointernal/sandbox/normalize.gointernal/sandbox/paths.gointernal/sandbox/risk.gointernal/sandbox/types.gointernal/tools/registry.gointernal/tools/registry_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Blockers
- Persistent sandbox grants do not currently authorize prompt-gated core tools through the actual registry/agent execution path.
internal/tools/registry.goreturns at the prompt-permission check before it callsoptions.Sandbox.Evaluate, so a savedzero sandbox grants allow write_file --auto mediumis persisted and listed, butRegistry.RunWithOptions(... Sandbox: engine, PermissionGranted: false ...)still fails withPermission required for write_file. That makes the newzero sandbox grants allowworkflow ineffective forwrite_file,edit_file,apply_patch, andbash, and leaves the M6 backend contract different from the CLI/API promise. I confirmed this with a local probe test that creates a grant store, grantswrite_file, routes throughRegistry.RunWithOptions, and still gets the prompt-denial error before sandbox grant lookup.
Non-Blocking
- None.
Looks Good
- The sandbox package has a clean split between policy evaluation, risk classification, workspace/symlink checks, backend selection, and grant persistence.
- CLI coverage for
zero sandbox policy/zero sandbox grantsis solid, and the persisted grant file uses schema validation, sorted output, redacted reasons, and 0600 writes. - Validation passed locally: targeted Go packages, full
go test -count=1 -p 1 ./...,bun run typecheck, 212/212 Bun tests, build, smoke build, smoke Go, sandbox CLI JSON/list/clear probes, andgit diff --check. PR base is current withorigin/main; no extra stale-base merge was needed.
Verdict: Changes Requested — clean structure and tests, but the persistent grant path needs to authorize prompt-gated tools before this can land.
BlockersNone found. The prior sandbox grant blocker is fixed at Non-BlockingNone. Looks Good
Verdict: Approve — clean fix for the authorization-path blocker; ready from my side. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approved after rereview: prior sandbox grant authorization blocker is fixed and validation is green.
3 participants
Summary
zero sandbox policyandzero sandbox grantsbackend commands for policy inspection plus persistent grant allow/deny/list/revoke/clear workflows.Scope
Validation
git diff --check origin/main...HEADgo test -count=1 -p 1 ./...bun run typecheckbun test ./tests --timeout 15000bun run buildbun run smoke:buildbun run smoke:goZERO_SANDBOX_GRANTS_PATH=$(mktemp -d)/grants.json ./zero sandbox policy --jsonZERO_SANDBOX_GRANTS_PATH=$(mktemp -d)/grants.json ./zero sandbox grants allow write_file --auto medium --reason final-local-check --jsonZERO_SANDBOX_GRANTS_PATH=$(mktemp -d)/grants.json ./zero sandbox grants list --jsonZERO_SANDBOX_GRANTS_PATH=$(mktemp -d)/grants.json ./zero sandbox grants clear --confirm --jsonSize
Summary by CodeRabbit
New Features
zero sandboxCLI withpolicy(view policy/backend) andgrants(allow/deny/list/revoke/clear) commands and JSON/text output options.Tests