feat(sandbox,module): SandboxRunner abstraction + exec_env factory (infra-admin P3a PR6/12)#846
Merged
Merged
Conversation
…ildSandboxConfig Introduces the SandboxRunner interface (Exec + Close) satisfied by *DockerSandbox with a compile-time assertion. Adds NewLocalDockerRunner as the default backend constructor. Extracts the security-profile→SandboxConfig mapping into sandbox.BuildSandboxConfig so remote runners (PR7/PR8) can reuse the same profile clamping logic. step.sandbox_exec now delegates its buildSandboxConfig() to BuildSandboxConfig, keeping behaviour identical. Refs ADR-0018 (Task 11). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tep.sandbox_exec Adds an optional exec_env config field to step.sandbox_exec. The new resolveSandboxRunner factory dispatches "" / "local-docker" to NewLocalDockerRunner (current behaviour, byte-identical result contract) and returns a clear error for "remote" / "ephemeral" / unknown values (deferred to PR7/PR8/PR9). step.Execute now calls the factory instead of sandbox.NewDockerSandbox directly. Adds exec_env to the StepSchema config fields. Refs ADR-0018 (Task 12). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on (review) - module_schema.go step.sandbox_exec gains exec_env ConfigField (the editor golden serializes ModuleSchemaRegistry, not step_schema_builtins — exec_env was invisible to the editor; golden regenerated). - exec_env options reduced to [local-docker] in both schema sources (remote/ ephemeral error until PR7/8/9 wire them — don't advertise unusable options). - step factory validates exec_env at construction (fail-early like security_profile); unknown value → config error. Test added. - execenv_factory: comment that the reserved app param is for PR7/8/9 wiring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces an execution-backend abstraction for step.sandbox_exec so the step can switch between different sandbox backends (future remote/ephemeral runners) while preserving the existing {exit_code, stdout, stderr} output contract.
Changes:
- Added
sandbox.SandboxRunnerinterface plus aNewLocalDockerRunnerconstructor that wraps the existing local Docker sandbox. - Extracted
security_profile→SandboxConfigmapping intosandbox.BuildSandboxConfig(...)and updatedstep.sandbox_execto use it. - Added an
exec_envconfig option (schema + step parsing) and amodule.resolveSandboxRunner(...)factory that currently wires onlylocal-docker.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/testdata/editor-schemas.golden.json | Updates editor schema golden to include exec_env for step.sandbox_exec. |
| schema/step_schema_builtins.go | Adds exec_env field to the builtin step schema with local-docker option/default. |
| schema/module_schema.go | Adds exec_env field to the module schema for step.sandbox_exec. |
| sandbox/runner.go | Introduces SandboxRunner interface + local-docker runner constructor. |
| sandbox/runner_test.go | Adds tests around the new runner constructor/interface assertion. |
| sandbox/profile.go | Adds BuildSandboxConfig shared mapping from profile → config. |
| sandbox/profile_test.go | Adds unit tests covering strict/standard/permissive/unknown profile mapping behavior. |
| module/pipeline_step_sandbox_exec.go | Threads exec_env through step.sandbox_exec and resolves runner via factory; delegates profile mapping to sandbox.BuildSandboxConfig. |
| module/execenv_factory.go | Adds resolveSandboxRunner factory (local-docker now; remote/ephemeral deferred). |
| module/execenv_factory_test.go | Adds tests for exec_env routing + step factory validation. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…nt (Copilot round-2) - runner/factory tests skip (not fail) when the Docker client env is unavailable — runner creation depends on DOCKER_HOST/TLS, not an exec_env regression (matches sandbox/docker_test.go tolerance). - module_schema.go exec_env gains DefaultValue: local-docker (golden regen) so the editor schema advertises the default. - resolveSandboxRunner comment clarified: it errors at runner-resolution (runtime); the step factory is the construction-time guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
PR6/12 — SandboxRunner abstraction + exec_env factory (infra-admin Phase 3a)
Locked plan Tasks 11–12; ADR 0018. Abstracts sandbox execution so
step.sandbox_execcan run on alternative backends (remote-runner PR7/8, Argo PR9) without changing its{exit_code, stdout, stderr}contract. Ships only the abstraction + the default local-docker backend; remote/ephemeral resolve in later PRs.Changes
sandbox.SandboxRunnerinterface (Exec+Close— the methodsstep.sandbox_execactually uses);*DockerSandboxsatisfies it;NewLocalDockerRunner(cfg).sandbox.BuildSandboxConfig(profile, image)— the strict/standard/permissive→SandboxConfigmapping extracted from the step (now the SINGLE shared mapping the remote agent will reuse for its profile clamp in PR8). Step still applies its own overrides after.module.resolveSandboxRunner(app, exec_env, cfg)factory (inmoduleto avoid a sandbox→module cycle):""/local-docker→local;remote/ephemeral/unknown→clear error (deferred to PR7/8/9).step.sandbox_execgains an optionalexec_envconfig, validated at factory time (onlylocal-dockerwired now). Behavior byte-identical when absent.Review notes (resolved)
ModuleSchemaRegistry(module_schema.go), which has its ownstep.sandbox_execconfig block —exec_envwas added there too (was only instep_schema_builtins.go; golden regenerated).exec_envoptions reduced to[local-docker]in both schema sources (don't advertise remote/ephemeral until wired).exec_envvalidation (fail-early, mirrorssecurity_profile).Verified:
go build ./...exit 0; fullgo test ./...exit 0 (149 ok);golangci-lint v2.12.0 --new-from-rev0 issues. Behavior-preserving extraction confirmed (BuildSandboxConfig byte-matches the original switch; overrides retained).🤖 Generated with Claude Code