Add sandbox adapter framework for CLI confinement - #45
Merged
Conversation
…; export commit gates
WalkthroughThis PR adds a pluggable sandbox abstraction with structured configuration, registry support, none/SRT/container adapters, provider integration, and CLI selection. It also adds workflow judge hooks, command hardening, first-failure verification, and exported commit gate helpers. ChangesVerification and workflow execution
Sandbox abstraction and selection
Sequence Diagram(s)sequenceDiagram
participant Provider as ClaudeCLI/CodexCLI/GeminiCLI
participant CLI as startCLIStream
participant Adapter as SandboxAdapter
participant Runtime as DockerOrSRTRuntime
Provider->>CLI: pass request with SandboxConfig
CLI->>Adapter: NewSandbox(config)
Adapter->>Adapter: Prepare(session)
CLI->>Adapter: Wrap(command)
Adapter->>Runtime: build wrapped command
Runtime-->>CLI: wrapped command and environment
CLI-->>Provider: streamed execution result
sequenceDiagram
participant Runner as prompt_run_live
participant Workflow as PromptHooksForWorkflow
participant Judge as LLMJudgeVerifier
participant Provider as ai.Provider
Runner->>Workflow: load and validate workflow prompts
Workflow-->>Runner: return judge hooks
Runner->>Judge: verify round
Judge->>Provider: execute judge request
Provider-->>Judge: return response
Judge-->>Runner: return verdict
Runner-->>Runner: stop on first invalid verdict
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Gavel summary
Totals: 0 passed · 0 failed · 0 skipped · - |
…he exec seam; container trust boundary for repo-supplied config
…ma oneOf, judge override guard, --mode passthrough
adityathebe
marked this pull request as ready for review
August 4, 2026 13:26
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
pkg/api/sandbox_registry.go (1)
29-40: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider synchronizing
sandboxFactories.
RegisterSandboxwrites this map andNewSandboxreads it. Registration atinit()time is safe, but tests re-register adapters while other tests construct sandboxes (see pkg/ai/provider/sandbox_seam_test.go:47), which is an unsynchronized concurrent map access undergo test -racewith parallel packages sharing the process. Async.RWMutexaround both accesses removes the hazard.🤖 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 `@pkg/api/sandbox_registry.go` around lines 29 - 40, Synchronize concurrent access to sandboxFactories by adding a sync.RWMutex, using the write lock in RegisterSandbox and the read lock around the map lookup in NewSandbox. Keep the existing registration validation and sandbox construction behavior unchanged.pkg/api/sandbox_ginkgo_test.go (1)
56-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the global sandbox registry after each spec.
These specs replace the process-global factories for
noneandsrtand never restore them. Ginkgo shares one process across the suite and can randomize spec order, so a later spec that expects the real adapter can observe a stub instead. The provider seam test already restores the previous factory (pkg/ai/provider/sandbox_seam_test.go:45-49). Note also that the spec at line 87 assumesgit-agenthas no registered adapter in this binary; that assumption breaks silently if a git-agent adapter is later linked in.♻️ Proposed cleanup helper
+// registerSandboxForSpec registers a stub factory and restores the previous +// registration when the spec ends. +func registerSandboxForSpec(kind api.SandboxKind, factory api.SandboxFactory) { + previous, existed := api.SandboxFactoryFor(kind) // add this accessor alongside RegisterSandbox + api.RegisterSandbox(kind, factory) + DeferCleanup(func() { + if existed { + api.RegisterSandbox(kind, previous) + return + } + api.UnregisterSandbox(kind) + }) +}It("constructs the registered adapter for a kind", func() { - api.RegisterSandbox(api.SandboxNone, func(cfg api.SandboxConfig) (api.Sandbox, error) { + registerSandboxForSpec(api.SandboxNone, func(cfg api.SandboxConfig) (api.Sandbox, error) { return sandboxStub{kind: api.SandboxNone}, nil })🤖 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 `@pkg/api/sandbox_ginkgo_test.go` around lines 56 - 113, Restore the process-global sandbox factories after each spec in the NewSandbox Describe block, preserving the previous registrations when tests replace the none or srt adapters and restoring them during cleanup. Also make the “no registered adapter” assertion in the known-kind test explicitly isolate or reset the git-agent registration so it remains valid regardless of adapters linked elsewhere.pkg/sandbox/adapter/container_test.go (1)
14-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused error return from the helpers.
newContaineralready fails the test witht.Fatal, so its error result is alwaysnil.prepareContainerthen captures that value and discards it with_ = err. Remove the return value so the helper contract matches its behaviour.♻️ Proposed refactor
func prepareContainer(t *testing.T, cwd string, options map[string]any) api.Sandbox { t.Helper() - sandbox, err := newContainer(t, options) - if _, err2 := sandbox.Prepare(context.Background(), specWithCwd(cwd)); err2 != nil { - t.Fatal(err2) - } - _ = err + sandbox := newContainer(t, options) + if _, err := sandbox.Prepare(context.Background(), specWithCwd(cwd)); err != nil { + t.Fatal(err) + } return sandbox } -func newContainer(t *testing.T, options map[string]any) (api.Sandbox, error) { +func newContainer(t *testing.T, options map[string]any) api.Sandbox { t.Helper() sandbox, err := api.NewSandbox(api.SandboxConfig{Kind: api.SandboxContainer, Options: options}) if err != nil { t.Fatal(err) } t.Cleanup(func() { _ = sandbox.Close() }) - return sandbox, nil + return sandbox }Update the three call sites at lines 114, 124 and 137 accordingly.
🤖 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 `@pkg/sandbox/adapter/container_test.go` around lines 14 - 32, Remove the unused error return from newContainer and update its callers to receive only the sandbox value; adjust prepareContainer to stop capturing and discarding err, while preserving the existing t.Fatal handling for sandbox creation and preparation failures. Update the three newContainer call sites accordingly.pkg/sandbox/adapter/srt.go (1)
75-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport runtime teardown failures, and consider bounding runtime accumulation.
Closediscards everyruntime.Closeerror, so a confinement that fails to tear down leaves no trace. Join the errors and return them.Each
Wrapcall also creates a new confinement and retains it untilClose. Resources therefore accumulate for the lifetime of the sandbox. If the policy depends only on the CLI name, cache the runtime per command instead of appending one per call.♻️ Proposed refactor for the error reporting
func (s *srtSandbox) Close() error { s.mu.Lock() runtimes := s.runtimes s.runtimes = nil s.mu.Unlock() + var errs []error for _, runtime := range runtimes { closeCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - _ = runtime.Close(closeCtx) + if err := runtime.Close(closeCtx); err != nil { + errs = append(errs, err) + } cancel() } - return nil + return errors.Join(errs...) }Add
"errors"to the imports.🤖 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 `@pkg/sandbox/adapter/srt.go` around lines 75 - 86, Update srtSandbox.Close to collect each runtime.Close failure and return the aggregated errors instead of discarding them, using the errors package. Also revise Wrap and the runtime storage so confinement instances are cached and reused by CLI name when the policy depends only on that name, rather than appending a new runtime for every call.pkg/sandbox/adapter/cli_env.go (1)
9-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPass through the CLI authentication environment variables each wrapper supports.
The repository pinpoints
ANTHROPIC_AUTH_TOKENplus the Anthropic mock also exportsANTHROPIC_BASE_URL; OpenAI mock exportsOPENAI_BASE_URL; Google Gen AI has a Vertex path and related env vars. Add the alternate variables needed for eachfilepath.Base(command)path, then update the sharedsrtandcontainertests so containers and sandbox-runtime both stay aligned.🤖 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 `@pkg/sandbox/adapter/cli_env.go` around lines 9 - 19, Update the cliCredentialEnv function to include all authentication variables that each CLI wrapper supports. For the "claude" case, add ANTHROPIC_AUTH_TOKEN and ANTHROPIC_BASE_URL to the returned environment variables. For the "codex" case, add OPENAI_BASE_URL alongside OPENAI_API_KEY. For the "gemini" case, add the Vertex-related environment variables alongside the existing GEMINI_API_KEY and GOOGLE_API_KEY. Then update the shared srt and container tests to verify that both the sandbox-runtime and containers pass through the complete set of environment variables for each CLI command path.
🤖 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 `@pkg/ai/agent/verify/verify.go`:
- Around line 111-143: Update the command execution flow around
context.WithTimeout and cmd.Run so verifier timeout detection uses a separate
context dedicated to c.Timeout, while preserving the original parent context for
cancellation errors. Return the timeout Verdict only when the verifier timeout
context exceeds its deadline; if the parent deadline fires first, return the
parent ctx.Err() instead. Add a regression test covering a parent deadline
shorter than Timeout.
In `@pkg/ai/agent/verify/workflow.go`:
- Around line 50-53: Reject whitespace-only entries in PromptHooksForWorkflow
instead of silently skipping them, returning an indexed validation error
consistent with pkg/api/workflow.go. Update pkg/ai/agent/verify/workflow.go
lines 50-53 accordingly; in pkg/ai/agent/verify/prompt_hooks_test.go lines
46-55, remove the blank entry from the successful case and add coverage
asserting that a blank prompt entry fails.
In `@pkg/api/runtime_registry.go`:
- Around line 52-63: The legacy sandbox validation guard that checks cfg.Sandbox
needs to respect SandboxSelection precedence. Locate the legacy guard that
validates based on cfg.Sandbox and wrap it in a condition so it only runs when
cfg.SandboxSelection is nil. This ensures that when a caller explicitly sets
SandboxSelection, the legacy guard does not override or reject that explicit
choice. The new SandboxSelection validation block shown in the diff should
remain unchanged and execute regardless.
In `@pkg/api/sandbox_ref.go`:
- Around line 172-178: Update SandboxRef.Validate to reject a set scalar
reference with an empty Backend, including sandbox: "" and scalar null decodings
that have no overrides. Preserve the existing backend requirement for
agent/policy overrides and the MaxAttempts validation, while ensuring any
present ref must select a non-empty backend.
- Around line 60-89: Update SandboxRef.UnmarshalJSON to return immediately for
explicit JSON null without modifying the receiver, and decode the object form
with a strict JSON decoder that rejects unknown fields such as “backed”.
Preserve the existing scalar-string handling and alias assignment for valid
object input.
In `@pkg/api/sandbox_registry.go`:
- Around line 62-69: Update NewSandbox immediately after the factory(cfg) call
to reject a nil Sandbox even when err is nil, returning an appropriate
construction error before verifySandboxCapabilities or any Close call. Preserve
the existing factory-error handling and capability verification for non-nil
sandbox instances, including SandboxNone.
In `@pkg/cli/ai_prompt_file.go`:
- Around line 196-204: The overlayCLI configuration must override inherited
sandbox settings when the resolved sandbox selection is none. Update the sandbox
handling around sandboxSelectionConfig so the none case explicitly clears both
cfg.Sandbox and cfg.SandboxSelection, while preserving named sandbox and
inherited SRT behavior. Add overlay tests covering inherited SRT and named
sandbox defaults.
In `@pkg/cli/prompt_run.go`:
- Around line 152-157: Update the workflow branch in executeSyncRunSingleDirect
to preserve opts.NoStream by threading it into executeSyncWorkflowRun or
selecting an equivalent non-streaming runner path. Ensure workflow prompts with
--no-stream avoid runPromptStream and do not require an ai.StreamingProvider,
while leaving the existing streaming behavior unchanged when the option is
unset.
In `@pkg/sandbox/adapter/container.go`:
- Around line 105-112: Update pathWithin to resolve relative path values against
root before calling filepath.Abs, while preserving absolute paths unchanged; use
the resulting path for symlink evaluation and containment checks so it matches
the project directory used by spec.Cwd and the Docker source path behavior.
- Around line 62-77: Restrict preset handling in the container configuration
flow around rejectUntrustedContainerConfig and c.options["presets"] so
repository-supplied presets cannot influence host environment variables or
mounts. Only apply presets from trusted backend options, or validate every
preset-derived environment and volume through the same trust checks before Wrap
expands them into -e or -v arguments; preserve trusted preset behavior.
In `@pkg/sandbox/adapter/srt.go`:
- Around line 68-72: Update the environment selection in the srt adapter’s Wrap
flow so variables declared through the env argument are preserved when cmd.Env
is non-empty. Either include those declared variables in
srtConfigFor(...).PassthroughEnv when it is the complete allowlist, or append
env to wrappedEnv before returning; retain existing command environment values.
- Around line 130-141: Extend the DenyRead list in the sandbox policy with the
remaining home-directory credential-store paths, then add the identical entries
to the denyRead fixture in the related SRT tests so policy and fixture coverage
remain synchronized.
In `@pkg/sandbox/config.go`:
- Around line 10-21: Preserve the existing ~/.captain.yaml key names for the
aliased configuration types in pkg/sandbox/config.go. Update the configuration
decoding path around MitmProxyConfig, NetworkConfig, FilesystemConfig,
RipgrepConfig, and SeccompConfig to use local wrapper structs or equivalent
YAML-tag overrides for legacy keys such as allowed_domains, socks_proxy_port,
and allow_git_config, rather than exposing sandboxruntime’s camelCase tags; keep
the upstream aliases only where they do not change the persisted schema.
---
Nitpick comments:
In `@pkg/api/sandbox_ginkgo_test.go`:
- Around line 56-113: Restore the process-global sandbox factories after each
spec in the NewSandbox Describe block, preserving the previous registrations
when tests replace the none or srt adapters and restoring them during cleanup.
Also make the “no registered adapter” assertion in the known-kind test
explicitly isolate or reset the git-agent registration so it remains valid
regardless of adapters linked elsewhere.
In `@pkg/api/sandbox_registry.go`:
- Around line 29-40: Synchronize concurrent access to sandboxFactories by adding
a sync.RWMutex, using the write lock in RegisterSandbox and the read lock around
the map lookup in NewSandbox. Keep the existing registration validation and
sandbox construction behavior unchanged.
In `@pkg/sandbox/adapter/cli_env.go`:
- Around line 9-19: Update the cliCredentialEnv function to include all
authentication variables that each CLI wrapper supports. For the "claude" case,
add ANTHROPIC_AUTH_TOKEN and ANTHROPIC_BASE_URL to the returned environment
variables. For the "codex" case, add OPENAI_BASE_URL alongside OPENAI_API_KEY.
For the "gemini" case, add the Vertex-related environment variables alongside
the existing GEMINI_API_KEY and GOOGLE_API_KEY. Then update the shared srt and
container tests to verify that both the sandbox-runtime and containers pass
through the complete set of environment variables for each CLI command path.
In `@pkg/sandbox/adapter/container_test.go`:
- Around line 14-32: Remove the unused error return from newContainer and update
its callers to receive only the sandbox value; adjust prepareContainer to stop
capturing and discarding err, while preserving the existing t.Fatal handling for
sandbox creation and preparation failures. Update the three newContainer call
sites accordingly.
In `@pkg/sandbox/adapter/srt.go`:
- Around line 75-86: Update srtSandbox.Close to collect each runtime.Close
failure and return the aggregated errors instead of discarding them, using the
errors package. Also revise Wrap and the runtime storage so confinement
instances are cached and reused by CLI name when the policy depends only on that
name, rather than appending a new runtime for every call.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dce7cba6-476b-4023-b88f-338707b5d407
📒 Files selected for processing (50)
pkg/ai/agent/commit/commit.gopkg/ai/agent/commit/gates.gopkg/ai/agent/runner.gopkg/ai/agent/verify/cmd_hardening_test.gopkg/ai/agent/verify/prompt_hooks_test.gopkg/ai/agent/verify/verify.gopkg/ai/agent/verify/workflow.gopkg/ai/agent/verify_order_test.gopkg/ai/provider/claude_cli.gopkg/ai/provider/cli.gopkg/ai/provider/cli_test.gopkg/ai/provider/codex_cli.gopkg/ai/provider/gemini_cli.gopkg/ai/provider/init.gopkg/ai/provider/sandbox_seam_test.gopkg/api/runtime_config.gopkg/api/runtime_registry.gopkg/api/sandbox.gopkg/api/sandbox_ginkgo_test.gopkg/api/sandbox_ref.gopkg/api/sandbox_ref_ginkgo_test.gopkg/api/sandbox_registry.gopkg/api/spec.gopkg/api/spec_merge.gopkg/api/spec_merge_differential_test.gopkg/api/workflow.gopkg/api/workflow_test.gopkg/captainconfig/config.gopkg/captainconfig/sandbox_test.gopkg/cli/ai.gopkg/cli/ai_prompt_file.gopkg/cli/ai_prompt_file_test.gopkg/cli/ai_sandbox.gopkg/cli/ai_sandbox_test.gopkg/cli/ai_test.gopkg/cli/prompt_run.gopkg/cli/prompt_run_live.gopkg/cli/prompt_run_stream.gopkg/cli/prompt_run_workflow_test.gopkg/cli/prompt_source.gopkg/cli/prompt_source_test.gopkg/cli/prompt_workflow.gopkg/sandbox/adapter/cli_env.gopkg/sandbox/adapter/container.gopkg/sandbox/adapter/container_test.gopkg/sandbox/adapter/none.gopkg/sandbox/adapter/none_test.gopkg/sandbox/adapter/srt.gopkg/sandbox/adapter/srt_test.gopkg/sandbox/config.go
… container preset trust, deny-read expansion)
Repository container configuration could still escape containment through a missing bind source below an outward symlink, while trusted preset values were exposed in Docker argv. Descriptor capabilities could also go unverified, and the prompt schema omitted configured sandbox selectors. Canonicalize existing bind-source parents, pass preset environment values by name, fail closed on unchecked capabilities, and inject configured sandbox names into both SandboxRef schema forms. Stop advertising SRT egress until that capability has a verifiable seam.
Gavel resultsGavel exited with code . |
Synchronous workflow runs always selected the streaming runner, dropping --no-stream and rejecting buffered-only providers.\n\nRoute no-stream generation through Provider.Execute while adapting the completed response into runner events, preserving workflow hooks and leaving normal streaming unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/test.yml:
- Around line 55-60: Add CI coverage for pkg/cli/webapp by adding a workflow job
or run that checks out or installs the sibling clicky-ui dependency, then
executes the vitest suite defined by pkg/cli/webapp/package.json. Keep the
existing Gavel exclusions for tests/e2e and pkg/cli/webapp, and ensure the new
step runs in an environment where the sibling dependency is available.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a840cf62-c768-435d-8c78-04cd52890a7a
📒 Files selected for processing (21)
.github/workflows/test.ymlpkg/ai/agent/verify/cmd_hardening_test.gopkg/ai/agent/verify/prompt_hooks_test.gopkg/ai/agent/verify/verify.gopkg/ai/agent/verify/workflow.gopkg/api/registry/sandboxes.gopkg/api/runtime_registry.gopkg/api/sandbox_ginkgo_test.gopkg/api/sandbox_ref.gopkg/api/sandbox_ref_ginkgo_test.gopkg/api/sandbox_registry.gopkg/cli/ai_prompt_file.gopkg/cli/ai_prompt_file_test.gopkg/cli/prompt_schema.gopkg/cli/prompt_schema_build.gopkg/cli/prompt_schema_test.gopkg/sandbox/adapter/cli_env.gopkg/sandbox/adapter/container.gopkg/sandbox/adapter/container_test.gopkg/sandbox/adapter/srt.gopkg/sandbox/adapter/srt_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/sandbox/adapter/srt_test.go
- pkg/ai/agent/verify/prompt_hooks_test.go
- pkg/sandbox/adapter/container_test.go
- pkg/ai/agent/verify/cmd_hardening_test.go
- pkg/sandbox/adapter/srt.go
- pkg/api/runtime_registry.go
- pkg/ai/agent/verify/workflow.go
- pkg/ai/agent/verify/verify.go
- pkg/sandbox/adapter/cli_env.go
- pkg/api/sandbox_registry.go
- pkg/cli/ai_prompt_file.go
- pkg/api/sandbox_ref_ginkgo_test.go
- pkg/sandbox/adapter/container.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a pluggable sandbox adapter framework that allows agent CLI processes (claude-cli, codex-cli, gemini-cli) to execute under different confinement mechanisms.
resolves: #39
Summary by CodeRabbit
New Features
Bug Fixes