feat(sandbox): add runtime-install field to control sbx/gVisor install step generation - #51413
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…r sbx/gvisor steps Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds sandbox.agent.runtime-install to skip runtime setup when gVisor or docker-sbx is preinstalled, while retaining credential refresh.
Changes:
- Adds runtime-install gating and import merge semantics.
- Extracts inline runtime setup commands into shell scripts.
- Updates runtime setup tests and script assertions.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/sandbox.go |
Defines and merges runtime-install configuration. |
pkg/workflow/nodejs.go |
Gates npm-engine runtime setup steps. |
pkg/workflow/gvisor_test.go |
Tests gVisor script delegation. |
pkg/workflow/firewall.go |
Adds the runtime-install helper. |
pkg/workflow/docker_sbx_test.go |
Tests docker-sbx scripts and gating. |
pkg/workflow/docker_sbx_install.go |
Delegates docker-sbx setup to scripts. |
pkg/workflow/copilot_engine_installation.go |
Delegates gVisor installation to a script. |
pkg/workflow/compiler_orchestrator_engine.go |
Applies imported runtime-install values. |
pkg/workflow/codex_engine.go |
Gates Codex runtime setup steps. |
pkg/parser/import_processor.go |
Exposes the merged import value. |
pkg/parser/import_field_extractor.go |
Extracts runtime-install from imports. |
eslint-factory/src/rules/require-invalid-date-check-before-compare.ts |
Applies formatting-only changes. |
actions/setup/sh/sudo_gvisor_install.sh |
Installs and verifies gVisor. |
actions/setup/sh/sudo_docker_sbx_install.sh |
Installs docker-sbx. |
actions/setup/sh/docker_sbx_secrets_check.sh |
Validates Docker Hub credentials. |
actions/setup/sh/docker_sbx_preflight.sh |
Runs the docker-sbx smoke test. |
actions/setup/sh/docker_sbx_kvm_check.sh |
Checks KVM availability. |
actions/setup/sh/docker_sbx_daemon.sh |
Configures and starts docker-sbx. |
actions/setup/sh/docker_sbx_credential_refresh.sh |
Refreshes sbx credentials. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Balanced
| ModelFallback *TemplatableBool `yaml:"model-fallback,omitempty"` // AWF API proxy model fallback enable/disable flag (optional) | ||
| TokenSteering *bool `yaml:"token-steering,omitempty"` // AWF API proxy token steering enable/disable flag (optional) | ||
| Targets map[string]*AgentAPIProxyTargetConfig `yaml:"targets,omitempty"` // Per-provider API proxy target overrides keyed by provider name (e.g. "openai", "anthropic") | ||
| RuntimeInstall *bool `yaml:"runtime-install,omitempty"` // Controls generation of runtime installation steps (gVisor/docker-sbx). Default: true. Noop when runtime is not set. |
There was a problem hiding this comment.
Fixed in 0c37a5b. runtime-install is now accepted by pkg/parser/schemas/main_workflow_schema.json, extracted in extractAgentSandboxConfig, and covered by direct frontmatter regression tests in pkg/workflow/docker_sbx_test.go and pkg/workflow/frontmatter_extraction_security_test.go.
| if isRuntimeInstallEnabled(workflowData) { | ||
| steps = append(steps, generateDockerSbxKVMCheckStep()) | ||
| steps = append(steps, generateDockerSbxSecretsCheckStep()) | ||
| steps = append(steps, generateDockerSbxInstallStep()) | ||
| steps = append(steps, generateDockerSbxAuthAndDaemonStep()) | ||
| steps = append(steps, generateDockerSbxPreFlightStep()) |
There was a problem hiding this comment.
Fixed in 0c37a5b. validateSandboxConfig now requires sandbox.agent.sudo: true for docker-sbx only when runtime installation remains enabled, and TestDockerSbxValidation_RuntimeInstallFalseAllowsPreinstalledRuntime plus the frontmatter compile regression cover the preinstalled-runtime path.
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100 ✅ Excellent OverviewThis PR introduces comprehensive tests for the new Changes Summary
Test BreakdownNew Test Functions (6 tests, 100% design quality)
Strengths:
Test Inflation Analysis
Analysis: Apparent 2–2.3:1 ratio is not inflation. Production code was refactored and simplified (106 deleted from docker_sbx_install.go). New tests validate the actual implementation layer (shell scripts) that now contain the logic—this is healthy boundary testing. Quality Score DetailsCalculation: Thresholds:
Key Quality SignalsBehavioral Coverage
Design Rigor
Refactoring QualityThe modified tests (
Hard Checks: All Passing ✅
Recommendation✅ APPROVE — This is a high-quality test PR that comprehensively validates the new
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. All tests are behavioral contracts with comprehensive edge-case coverage. Zero implementation tests, no violations. Production code was simplified while test coverage expanded—healthy refactoring. The shift to script-driven contracts improves maintainability.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two test gaps and one potential logic bug in the false-wins merge.
📋 Key Themes & Highlights
Issues Found
-
TestDockerSbxRuntimeInstallFalseOmitsInstallStepsmissing positive assertion — the test verifies install steps are absent but does not assert that credential-refresh is still emitted. The PR stated guarantee ("credential-refresh always emitted") is untested. -
TestGenerateGVisorInstallSteplost supply-chain security assertions — moving logic to a script is correct, but the test no longer checks SHA-512 verification,uname -m, orsudo install. ATestGVisorInstallScriptContentanalogous toTestDockerSbxShellScriptContentwould restore that coverage. -
mergeImportedSandboxAgentRuntimeInstallmay violate false-wins contract — an import withruntime-install: truecan overwrite a main workflow that explicitly setruntime-install: false. See inline comment onsandbox.go:344.
Positive Highlights
- Clean extraction of inlined shell into named scripts — greatly improves maintainability and testability
isRuntimeInstallEnabledis a well-scoped helper with comprehensive unit tests- Credential-refresh ungating is the right design: it does not require sudo and should always run
- Import accumulator pattern is consistent with existing merge semantics in the codebase
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 53.6 AIC · ⌖ 7.22 AIC · ⊞ 7.1K
Comment /matt to run again
| assert.NotContains(t, content, "sudo_docker_sbx_install.sh", "install step must be omitted when runtime-install: false") | ||
| assert.NotContains(t, content, "docker_sbx_daemon.sh", "daemon step must be omitted when runtime-install: false") | ||
| assert.NotContains(t, content, "docker-sbx pre-flight smoke test", "pre-flight must be omitted when runtime-install: false") | ||
| } |
There was a problem hiding this comment.
[/tdd] The test verifies that install steps are absent when runtime-install: false, but it does not assert that the credential-refresh step is still emitted — the core invariant this PR claims.
Without a positive assertion, a regression that accidentally drops the credential-refresh step would go undetected.
💡 Suggested addition
// Credential refresh must still be present.
assert.Contains(t, content, "docker_sbx_credential_refresh.sh",
"credential refresh step must still be emitted when runtime-install: false")Add this inside TestDockerSbxRuntimeInstallFalseOmitsInstallSteps after the existing NotContains block.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c37a5b. I added an end-to-end compile regression in TestDockerSbxFrontmatterExtractionRuntimeInstallFalse that asserts Refresh sbx credentials is still emitted when runtime-install: false, and kept the install-step omission test focused on the gated install builder.
| // Must detect architecture dynamically, not hardcode amd64/arm64. | ||
| assert.Contains(t, s, "uname -m", "must detect architecture via uname -m") | ||
| assert.NotContains(t, s, "amd64", "must NOT remap architecture to amd64") | ||
| assert.NotContains(t, s, "arm64", "must NOT remap architecture to arm64") |
There was a problem hiding this comment.
[/tdd] The refactored TestGenerateGVisorInstallStep loses precise supply-chain assertions: SHA-512 verification, uname -m architecture detection, sudo install, and the official gVisor URL. The new test only checks that the script filename is referenced — it no longer guards against the script being modified to skip integrity checks.
Consider adding a TestGVisorInstallScriptContent (similar to TestDockerSbxShellScriptContent) that reads sudo_gvisor_install.sh and asserts sha512sum -c, uname -m, storage.googleapis.com/gvisor, and sudo install are present.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0c37a5b. pkg/workflow/gvisor_test.go now includes TestGVisorInstallScriptContent, which asserts sha512sum -c, uname -m, the official storage.googleapis.com/gvisor URL, and sudo install are present in sudo_gvisor_install.sh.
|
|
||
| // isSandboxEnabled checks if the sandbox is enabled (either explicitly or auto-enabled) | ||
| // Returns true when: | ||
| // - sandbox.agent is explicitly set to awf |
There was a problem hiding this comment.
[/codebase-design] mergeImportedSandboxAgentRuntimeInstall applies false-wins semantics, but when the imported value is true and the main workflow has already explicitly set RuntimeInstall: &false, this line overwrites it:
if !*importedRuntimeInstall || sandboxConfig.Agent.RuntimeInstall == nil {
sandboxConfig.Agent.RuntimeInstall = importedRuntimeInstall
}If the main workflow sets runtime-install: false and an import sets runtime-install: true, the import wins — contradicting the false-wins contract described in the PR description and the comment above the function.
The condition should be: apply only when the imported value is false (never let an import make it true if already false).
💡 Suggested fix
// Only set from import when: import is false (always wins), or field is not yet set.
if !*importedRuntimeInstall {
sandboxConfig.Agent.RuntimeInstall = importedRuntimeInstall
} else if sandboxConfig.Agent.RuntimeInstall == nil {
sandboxConfig.Agent.RuntimeInstall = importedRuntimeInstall
}@copilot please address this.
There was a problem hiding this comment.
Checked in 0c37a5b. The merge logic already preserved false-wins semantics; I added TestMergeImportedSandboxAgentRuntimeInstall to lock that in, including the case where a main workflow’s explicit false must not be overwritten by an imported true.
… script extraction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ADR Required - Draft CommittedThis PR adds 545 lines of new code in business-logic directories and no Architecture Decision Record (ADR) was found. A draft ADR has been committed to this branch at: docs/adr/51413-sandbox-agent-runtime-install-field-with-script-extraction.md This PR cannot merge until the ADR is reviewed and its status updated from Draft to Accepted. What to do next:
Why ADRs matter: ADRs create a permanent, searchable record of why a decision was made. Without one, future contributors have no way to understand the constraints that shaped the code, making changes riskier and context loss inevitable. Required ADR sections (Michael Nygard template):
Merge blocked until docs/adr/51413 status is Accepted (or Proposed).
|
There was a problem hiding this comment.
Review: feat(sandbox): add runtime-install field
One blocking issue found — schema validation will reject the new field.
The runtime-install field is correctly added to the Go struct (AgentSandboxConfig.RuntimeInstall *bool) and wired through the full stack (import accumulator → merge function → codex/nodejs engine). However, pkg/parser/schemas/main_workflow_schema.json is not updated, and the sandbox.agent object definition there has "additionalProperties": false. Any workflow that sets sandbox.agent.runtime-install: false will receive a schema validation error from the compiler before the feature ever executes.
Fix required: Add "runtime-install": { "type": "boolean", ... } to the sandbox.agent properties block in the schema file (around the existing runtime property definition, ~line 3523).
Other observations (non-blocking)
- False-wins merge semantics: The "false wins" accumulation in
mergeSandboxAgentRuntimeInstallis clearly documented and intentional — this is correct for a security-oriented field. - Shell script extraction: Moving inline bash from Go string slices into
actions/setup/sh/*.shscripts is a sound refactor — scripts are now independently testable and readable. generateDockerSbxCredentialRefreshStepis always emitted even whenruntime-install: false— the comment "always emitted" is consistent with the implementation innodejs.go(it sits outside theisRuntimeInstallEnabledguard). This is the intended behaviour for pre-installed runtimes.- Import-level override precedence:
mergeImportedSandboxAgentRuntimeInstallcorrectly lets the main workflow's explicittrueresist being overridden by an import'strue, while still being overridden by an import'sfalse. Correct. - Tests are thorough and cover the new helper, the omission of steps, and the shell script contents.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 79.1 AIC · ⌖ 7.48 AIC · ⊞ 5.5K
|
@copilot Quick triage nudge for this PR. Please address the latest blocking review feedback, update the ADR status if it is ready, refresh the branch if GitHub can do so cleanly, then run the Open review context (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31267317361
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Quick triage nudge for this PR. Please address the latest blocking review feedback, update the ADR status if it is ready, refresh the branch if GitHub can do so cleanly, then run the Open review context (newest first):
Branch refresh was requested. Run: https://github.com/github/gh-aw/actions/runs/31268056080
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 0c37a5b. I fixed the schema/frontmatter wiring, made docker-sbx sudo validation conditional on runtime installation being enabled, restored the missing credential-refresh and gVisor script coverage, added a false-wins merge regression test, and marked the ADR Accepted. The branch was not behind base, so no update-branch action was needed. Local |
|
🎉 This pull request is included in a new release. Release: |
Adds
sandbox.agent.runtime-install: bool(defaulttrue) so workflows can opt out of generating sbx/gVisor installation steps — useful when the runtime is pre-installed on the runner. The credential-refresh step is always emitted regardless, since it does not require sudo. Field is importable from shared agent workflows (false-wins merge semantics). Noop when no runtime is specified.Also refactors all previously-inlined shell code into standalone scripts under
actions/setup/sh/; scripts requiring sudo are prefixedsudo_.New shell scripts
docker_sbx_kvm_check.shdocker_sbx_secrets_check.shsudo_docker_sbx_install.shdocker_sbx_daemon.shdocker_sbx_preflight.shdocker_sbx_credential_refresh.shsudo_gvisor_install.shUsage
Key changes
sandbox.go—RuntimeInstall *boolonAgentSandboxConfig;mergeImportedSandboxAgentRuntimeInstall()(false-wins across imports)firewall.go—isRuntimeInstallEnabled(): returnstruewhen runtime is unset (noop),falseonly when runtime is set and field is explicitlyfalsedocker_sbx_install.go/copilot_engine_installation.go— step generators now call external scripts instead of inlining shellcodex_engine.go/nodejs.go— install steps gated onisRuntimeInstallEnabled(); credential-refresh remains ungatedimport_field_extractor.go/import_processor.go— accumulator field +MergedSandboxAgentRuntimeInstallinImportsResultRun: https://github.com/github/gh-aw/actions/runs/31268056080> Generated by 👨🍳 PR Sous Chef · gpt54 · 4.6 AIC · ⌖ 5.27 AIC · ⊞ 8.5K · ◷