Default sandbox.agent.sudo to false (network isolation) - #42354
Conversation
…ict mode, warning in non-strict Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This pull request changes the default sandbox behavior so sandbox.agent.sudo now defaults to false, making network-isolation/rootless the default execution mode for the firewall (AWF). It adds an explicit-tracking flag to distinguish “explicit sudo: true” from “unset”, and updates strict-mode validation and golden/test expectations accordingly.
Changes:
- Default
AgentSandboxConfigtoNetworkIsolation=truewhensudois omitted (and in default-initialization paths). - Add
SudoExplicitlyEnabled(non-serialized) to detect explicitsudo: trueand emit strict-mode errors / non-strict warnings. - Update unit tests and Wasm golden outputs to reflect rootless AWF invocation and bridge-network MCP gateway wiring.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/sandbox.go | Adds SudoExplicitlyEnabled and applies network-isolation defaults in applySandboxDefaults. |
| pkg/workflow/frontmatter_extraction_security.go | Defaults NetworkIsolation=true when sudo is omitted; records explicit sudo: true. |
| pkg/workflow/strict_mode_sandbox_validation.go | Emits strict error / non-strict warning when sudo: true was explicitly set. |
| pkg/workflow/strict_mode_sandbox_validation_test.go | Adds coverage for strict error + non-strict warning behavior for sudo: true. |
| pkg/workflow/frontmatter_extraction_security_test.go | Extends extraction tests for omitted sudo default + explicit sudo tracking. |
| pkg/workflow/sandbox_network_isolation_rootless_test.go | Updates the “sudo omitted” scenario assertions to expect rootless/network-isolated defaults. |
| pkg/workflow/sandbox_custom_agent_test.go | Updates expectations to assert rootless awf --config rather than sudo -E awf. |
| pkg/workflow/compiler_permissions_test.go | Updates expectations for default AWF invocation under the new rootless default. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Updates compiled workflow golden output for rootless install/invocation and isolated MCP gateway topology. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Same as above for smoke-copilot fixture. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Same as above for Playwright CLI mode fixture. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Same as above for basic-copilot fixture. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Updates all-engines Pi golden to reflect isolated/rootless defaults and topology attach. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Updates all-engines Gemini golden for rootless defaults and MCP gateway bridge wiring. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Updates all-engines Copilot golden for rootless defaults and MCP gateway bridge wiring. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Updates all-engines Codex golden for rootless defaults and MCP gateway bridge wiring. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Updates all-engines Claude golden for rootless defaults and MCP gateway bridge wiring. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Low
…alse Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core sandbox security logic (231 new lines in business logic directories) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel — Score: 83/100 ✅ ExcellentThis PR ("Default sandbox.agent.sudo to false") adds well-structured behavioral tests covering the 📊 Metrics & Scoring
Score breakdown
🔍 Test Classification Table
Verdict: ✅ PASS — 0% implementation tests (threshold: 30%). No coding-guideline violations. All new tests enforce behavioral contracts and the security invariant is well-covered end-to-end.
|
Skills-Based Review Summary 🧠Applied 🔴 Blocking issues (3)
🟡 Improvements (7)
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /tdd, and /zoom-out — requesting changes on test coverage gaps and a documentation/API-contract issue.
📋 Key Themes & Highlights
Key Themes
- Silent API footgun (
sandbox.go:213):SudoExplicitlyEnableddrives both compile-time validation and runtime defaults inapplySandboxDefaults, but the field doc only mentions the former. Any caller that setsNetworkIsolation: falsewithout also settingSudoExplicitlyEnabled: truewill be silently overridden. The existingsandbox_test.gofixtures already hit this path with zero assertions. - Brittle test assertions (
sandbox_network_isolation_rootless_test.go): Two assertions hardcode 10-space YAML indentation ("\n awf --config "). These are silent false-negatives if the emitter indentation ever shifts. - Test isolation (
strict_mode_sandbox_validation_test.go): SharedsudoTrueSandboxpointer across subtests; fragile if validation ever mutates the struct before the early-return. - Missing integration test: No test verifies that
sudo: truein frontmatter generatessudo -E awfin the compiled output — all four updated cases only assert the negative. - Misleading log message (
strict_mode_sandbox_validation.go:62): "Skipping sandbox customization validation" fires after the sudo deprecation warning already emitted.
Positive Highlights
- ✅ Clean three-layer default strategy covers nil-config, nil-agent, and pre-existing-agent paths
- ✅
SudoExplicitlyEnabledneatly separates tracking from semantics — right field for the job - ✅ Comprehensive golden-file and test updates throughout; no existing assertions left unaddressed
- ✅ Deprecation UX is excellent: strict-mode error + non-strict warning with a documentation URL
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 137.1 AIC · ⌖ 8.58 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/sandbox.go:213
[/improve-codebase-architecture] Post-construction block silently overrides NetworkIsolation=false for any caller that builds AgentSandboxConfig{NetworkIsolation: false} without also setting SudoExplicitlyEnabled: true. Since SudoExplicitlyEnabled is yaml:"-" and only set by extractAgentSandboxConfig, any programmatic caller of applySandboxDefaults passing a pre-built config will have its setting stomped silently.
<details>
<summary>💡 Suggested fix / documentation</summary>
…
pkg/workflow/sandbox_network_isolation_rootless_test.go:64
[/tdd] Brittle whitespace assertion: "\n awf --config " hardcodes 10 spaces of YAML indentation. If the YAML emitter or run-block indentation ever changes, this check silently fails with a misleading error message ("Expected rootless 'awf --config'...") even though the AWF invocation is present.
<details>
<summary>💡 Suggested fix</summary>
Drop the newline and leading spaces — strings.Contains is already specific enough to distinguish this from the awf logs summary line:
…
pkg/workflow/sandbox_network_isolation_rootless_test.go:126
[/tdd] Same brittle whitespace issue as line 64 — "\n awf --config " hardcodes YAML indentation. Silently breaks if the lock-file indentation changes.
<details>
<summary>💡 Suggested fix</summary>
if !strings.Contains(lockStr, "awf --config") {
t.Error("Expected rootless 'awf --config' invocation in lock file")
}</details>
@copilot please address this.
pkg/workflow/strict_mode_sandbox_validation_test.go:252
[/tdd] sudoTrueSandbox is a shared pointer used across three subtests. While safe today (strict-mode path returns before mutating agent.Type), the strict-mode path in validateStrictSandboxCustomization does contain a struct mutation (agent.Type = SandboxTypeAWF) lower in the function. If future validation logic runs before the sudo early-return, the shared struct would cause cross-test contamination.
<details>
<summary>💡 Suggested fix</summary>
Construct a fresh fixture per subte…
pkg/workflow/strict_mode_sandbox_validation_test.go:281
[/tdd] The non-strict warning test only verifies that GetWarningCount() incremented — it doesn't capture or assert the actual stderr warning message. A refactor that swapped out the message (or broke formatting) would pass this test silently. Other warning tests in this codebase (e.g., action_pins_test.go) capture stderr to check content.
<details>
<summary>💡 Suggested addition</summary>
Capture stderr and assert the message contains the key details:
// Capture stderr
oldStder…
</details>
<details><summary>pkg/workflow/strict_mode_sandbox_validation.go:62</summary>
**[/zoom-out]** Misleading log message: `"Strict mode disabled, skipping sandbox customization validation"` fires *after* the sudo check has already run and potentially emitted a deprecation warning. A reader of this log line would incorrectly conclude that nothing happened for this invocation.
<details>
<summary>💡 Suggested fix</summary>
Update the message to reflect partial execution:
```go
strictModeValidationLog.Printf("Strict mode disabled, skipping remaining strict-only sandbox checks…
</details>
<details><summary>pkg/workflow/sandbox.go:174</summary>
**[/improve-codebase-architecture]** The fresh-construction paths here (lines 174, 194) set `NetworkIsolation: true` inline, but the post-construction block at line 213 also unconditionally sets `NetworkIsolation = true` for any existing agent with `!SudoExplicitlyEnabled`. Both the fresh and existing paths end up at the same result, but via different mechanisms with no comment explaining the relationship. This makes it hard to reason about invariants.
<details>
<summary>💡 Suggestion</summary…
</details>
<details><summary>pkg/workflow/frontmatter_extraction_security.go:216</summary>
**[/zoom-out]** The `SudoExplicitlyEnabled` flag is only set for `sudo: true`, not for `sudo: false`. This is correct — the flag tracks the *dangerous* explicit opt-in. However, a future reader might wonder why `sudo: false` doesn't also set a flag. A one-line comment would pre-empt this confusion.
<details>
<summary>💡 Suggested clarification</summary>
```go
if sudoBool {
// Only track explicit sudo: true (host-access opt-in), not sudo: false
// (which is the safe default and require…
</details>
<details><summary>pkg/workflow/compiler_permissions_test.go:149</summary>
**[/tdd]** The four test cases here all assert that `sudo -E awf` is absent (the new default). There's no integration test in this file that compiles a workflow with `sudo: true` explicitly set and verifies that `sudo -E awf` IS generated. Without such a test, a regression that drops `sudo -E awf` from the explicit-sudo path would go undetected.
<details>
<summary>💡 Suggested addition</summary>
Add a test case (or a subtest) that sets `sudo: true` in the workflow frontmatter and verifies:
`…
</details>
<details><summary>pkg/workflow/sandbox.go:54</summary>
**[/improve-codebase-architecture]** The `SudoExplicitlyEnabled` field doc says it's used to emit errors/warnings at compile time, but it also drives runtime behaviour in `applySandboxDefaults` (line 213). The doc comment should mention both uses so that readers understand the full scope of impact:
<details>
<summary>💡 Suggested doc update</summary>
```go
// True when sudo: true was explicitly set in frontmatter.
// Used in two places:
// 1. validateStrictSandboxCustomization — emits an er…
</details>
There was a problem hiding this comment.
Non-blocking observations on two code paths
The core logic is sound: the three-pronged approach (frontmatter default, two early-creation defaults in applySandboxDefaults, and the late safety net at line 213) correctly enforces network isolation for all standard-format workflows. Test coverage is thorough. Two medium concerns are worth addressing before or shortly after merge.
### Issues found (2 medium)
1. Legacy sandbox.type format silently bypasses the new default (sandbox.go line 183–186)
When sandboxConfig.Type != "" the function exits before line 213, so any workflow still using the old top-level sandbox: {type: awf} format remains in sudo mode with no warning. isAWFNetworkIsolationEnabled returns false for those configs because Agent stays nil. If operators believe this change covers all workflows, they are wrong for legacy-format configs. See inline comment.
2. Strict-mode sudo: true error short-circuits all other strict-mode checks (strict_mode_sandbox_validation.go line 54)
Before this PR, strict mode reported command/args/env/MCP violations in a single compilation pass. Now, a workflow that combines sudo: true with other forbidden fields will only reveal the sudo: true error; subsequent violations surface only after each fix-compile cycle. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer · 146 AIC · ⌖ 6.99 AIC · ⊞ 5.2K
Comment /review to run again
| // Apply the default sudo: false (network isolation) when sudo was not explicitly | ||
| // set to true in frontmatter. This ensures network isolation is the default. | ||
| if !sandboxConfig.Agent.SudoExplicitlyEnabled { | ||
| sandboxConfig.Agent.NetworkIsolation = true |
There was a problem hiding this comment.
The legacy sandbox.type code path above silently keeps sudo mode, inconsistent with the new default.
When sandboxConfig.Type != "" (line 183), the function returns early before reaching this block, so any workflow still using the old top-level sandbox.type: awf format never gets NetworkIsolation = true applied. The security policy change does not cover that format — there is no warning, no migration, and isAWFNetworkIsolationEnabled returns false for those configs because Agent remains nil.
💡 Suggested fix
Either apply the new default inside the legacy early-return path, or emit a deprecation warning so operators know their config is outside the new policy:
if sandboxConfig.Type != "" {
sandboxLog.Printf("Sandbox config uses legacy Type field: %s, preserving it", sandboxConfig.Type)
// Apply network isolation default to Agent if present.
if sandboxConfig.Agent != nil && !sandboxConfig.Agent.SudoExplicitlyEnabled {
sandboxConfig.Agent.NetworkIsolation = true
}
ensureDefaultAgentWritePath(sandboxConfig)
return sandboxConfig
}If the legacy path is intentionally excluded, document that explicitly in the function comment so reviewers and operators can audit their workflow inventory.
| "Remove 'sudo: true' to use the secure default. " + | ||
| "See: https://github.github.com/gh-aw/reference/sandbox/" | ||
| if c.strictMode { | ||
| return fmt.Errorf("strict mode: %s", sudoTrueMsg) |
There was a problem hiding this comment.
Returning here in strict mode short-circuits all remaining strict-mode checks, making error discovery order-dependent.
A workflow with sudo: true plus command: "custom-wrapper" plus a forbidden MCP field in strict mode will only surface the sudo: true error on first compile. The user must fix-compile-fix to find each subsequent violation, whereas before this change strict mode reported command/args/env/MCP violations in a single pass.
💡 Suggested fix
Collect the sudo-true error rather than returning immediately, so all strict-mode violations are visible together:
var errs []string
if agent.SudoExplicitlyEnabled {
if c.strictMode {
errs = append(errs, sudoTrueMsg)
} else {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(sudoTrueMsg))
c.IncrementWarningCount()
}
}
// ... remaining checks append to errs ...
if len(errs) > 0 {
return fmt.Errorf("strict mode: %s", strings.Join(errs, "; "))
}Alternatively, keep the early return but document the ordering contract explicitly, since the behaviour is now observable to users.
Review: Default
|
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 158 AIC · ⌖ 7.12 AIC · ⊞ 4.9K
| // (omitted) → sudo enabled = normal mode → NetworkIsolation=false (zero value) | ||
| // sudo: true → sudo enabled = normal mode → NetworkIsolation=false (deprecated; error in strict mode, warning otherwise) | ||
| // (omitted) → default = network isolation mode → NetworkIsolation=true (same as sudo: false) | ||
| agentConfig.NetworkIsolation = true // Default: sudo: false (network isolation enabled) |
There was a problem hiding this comment.
The object-format branch now correctly defaults NetworkIsolation = true here, but the string-format branch (line ~164, unchanged) still returns &AgentSandboxConfig{Type: agentType} with NetworkIsolation = false.
The default is fixed up later by applySandboxDefaults via the !SudoExplicitlyEnabled guard, so current behavior is correct. However, any future code reading agentConfig.NetworkIsolation from the pre-defaults sandboxConfig (e.g., inside runPostEngineValidations) would see false for string-format agents, which contradicts the stated default.
Consider making the default eager and consistent:
return &AgentSandboxConfig{
Type: agentType,
NetworkIsolation: true, // Default: sudo: false (network isolation)
}@copilot please address this.
| // Apply the default sudo: false (network isolation) when sudo was not explicitly | ||
| // set to true in frontmatter. This ensures network isolation is the default. | ||
| if !sandboxConfig.Agent.SudoExplicitlyEnabled { | ||
| sandboxConfig.Agent.NetworkIsolation = true |
There was a problem hiding this comment.
The !SudoExplicitlyEnabled → NetworkIsolation = true guard correctly applies the new default for configs that reach this point.
However, the early-return path at line 183–186 (when sandboxConfig.Type != "") bypasses this block:
if sandboxConfig.Type != "" {
ensureDefaultAgentWritePath(sandboxConfig)
return sandboxConfig // ← bypasses the NetworkIsolation default below
}For frontmatter-sourced configs this is safe because extractAgentSandboxConfig already sets NetworkIsolation = true as the default (line 212 of frontmatter_extraction_security.go). But programmatically-created configs with sandboxConfig.Type != "" and sandboxConfig.Agent != nil will silently skip the new default.
A defensive fix or a comment explaining the invariant would prevent future regressions:
if sandboxConfig.Type != "" {
// NetworkIsolation is already set by extractAgentSandboxConfig; skip re-defaulting.
ensureDefaultAgentWritePath(sandboxConfig)\n return sandboxConfig\n}\n```\n\n@copilot please address this.| if strings.Contains(lockStr, "--rootless") { | ||
| t.Error("Expected no '--rootless' flag in install step when sudo is not set") | ||
| // AWF must still be invoked (without sudo). Check for the main AWF invocation pattern. | ||
| if !strings.Contains(lockStr, "\n awf --config ") { |
There was a problem hiding this comment.
The "\n awf --config " assertion relies on the exact indentation of the generated YAML run block. If the YAML renderer ever changes indentation (e.g., moving from 10 to 8 spaces), this test would fail with a confusing message rather than a useful one.
The first test case (line 64) uses the same pattern with a comment explaining the rationale. Consider extracting this into a helper or using a slightly looser match:
// Checks that AWF is invoked directly (not via sudo) as the main execution step.
if !strings.Contains(lockStr, "awf --config ") || strings.Contains(lockStr, "sudo -E awf") {
t.Error("Expected rootless 'awf --config' invocation without sudo")
}\n```
@copilot please address this.| const sudoTrueMsg = "sandbox.agent.sudo: true re-enables host-access (sudo) mode. " + | ||
| "The default is now sudo: false (network isolation). " + | ||
| "Remove 'sudo: true' to use the secure default. " + | ||
| "See: https://github.github.com/gh-aw/reference/sandbox/" |
There was a problem hiding this comment.
Please verify that https://github.github.com/gh-aw/reference/sandbox/ is a live URL before this PR merges. If the docs page doesn't exist yet, consider using a placeholder or the repo URL (https://github.com/github/gh-aw) until it is published — a broken link in a user-facing warning message is confusing.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the The following review threads are unresolved and need attention:
Please address the unresolved review feedback, ensure all checks pass, and run
|
Added sandbox.agent.sudo glossary entry documenting the field's behavior and new default (false = network isolation mode), introduced by PR #42354 which changed the default from sudo mode to rootless/network-isolation mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Changes the global default for
sandbox.agent.sudofromtrue(sudo/host-access mode) tofalse(network isolation/rootless mode). No.mdworkflow files changed — Go default behavior only.Behavior
sudoomittedsudo -E awfawfsudo: falseawfawf(no change)sudo: truesudo -E awfsudo -E awf+ error (strict) / warning (non-strict)Implementation
AgentSandboxConfig— addsSudoExplicitlyEnabled bool(not serialized) to distinguishsudo: truein frontmatter from the defaultfrontmatter_extraction_security.go—extractAgentSandboxConfigdefaultsNetworkIsolation=truewhensudois absent; setsSudoExplicitlyEnabled=trueonly on explicitsudo: truesandbox.go—applySandboxDefaultsappliesNetworkIsolation: trueto all freshly-createdAgentSandboxConfiginstances (covers code paths that bypass frontmatter extraction)strict_mode_sandbox_validation.go—validateStrictSandboxCustomizationgates onSudoExplicitlyEnabled: hard error in strict mode, stderr warning + counter increment in non-strict modeTest changes
TestNetworkIsolationRootless"sudo omitted" case to assert rootless behaviorTestValidateStrictSandboxCustomizationSudoTruefor both strict error and non-strict warning pathsTestExtractAgentSandboxConfigSudowith omitted-sudo default andSudoExplicitlyEnabledassertionscompiler_permissions_test.go,sandbox_custom_agent_test.go, and Wasm golden files that assertedsudo -E awfas the default