Require dangerously-disable-sandbox-agent feature flag to allow sandbox.agent: false#38205
Conversation
…ent: false Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
dangerously-disable-sandbox-agent feature flag for sandbox.agent: falsedangerously-disable-sandbox-agent feature flag to allow sandbox.agent: false
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38205 does not have the 'implementation' label (has_implementation_label=false) and has 95 new lines of code in business logic directories, which is below the 100-line threshold (requires_adr_by_default_volume=false). |
|
❌ Test Quality Sentinel failed during test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR tightens security posture by preventing workflows from silently disabling the agent sandbox (sandbox.agent: false) unless they explicitly opt in via a new “dangerously” feature flag.
Changes:
- Introduces
dangerously-disable-sandbox-agentas a first-class feature flag constant. - Adds validation that rejects
sandbox.agent: falseunless the feature flag is enabled. - Updates several workflow/test fixtures to include the feature flag and adds a negative-path test.
Show a summary per file
| File | Description |
|---|---|
| pkg/constants/feature_constants.go | Adds the DangerouslyDisableSandboxAgentFeatureFlag constant and documents intended usage. |
| pkg/workflow/sandbox_validation.go | Enforces the new feature-flag gate when sandbox.agent: false is set. |
| pkg/workflow/sandbox_agent_false_test.go | Updates the “allowed” acceptance test to include the feature flag and adds a rejection test when the flag is missing. |
| pkg/workflow/compiler_validators_test.go | Updates a validator test fixture to include the new feature flag when agent sandbox is disabled. |
| pkg/workflow/importable_tools_test.go | Updates embedded workflow fixtures that disable the agent sandbox to include the feature flag. |
| pkg/workflow/pull_request_target_validation_test.go | Updates embedded workflow fixtures that disable the agent sandbox to include the feature flag. |
| pkg/workflow/workflow_run_validation_test.go | Updates embedded workflow fixtures that disable the agent sandbox to include the feature flag. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
| @@ -39,9 +41,9 @@ Test workflow to verify sandbox.agent: false is accepted and disables agent sand | |||
| compiler := NewCompiler() | |||
| compiler.SetSkipValidation(true) | |||
|
|
|||
| // Should succeed in non-strict mode | |||
| // Should succeed when the feature flag is set | |||
| if err := compiler.CompileWorkflow(workflowPath); err != nil { | |||
| t.Fatalf("Expected compilation to succeed with sandbox.agent: false in non-strict mode, but got error: %v", err) | |||
| t.Fatalf("Expected compilation to succeed with sandbox.agent: false and feature flag, but got error: %v", err) | |||
| if !isFeatureEnabled(constants.DangerouslyDisableSandboxAgentFeatureFlag, workflowData) { | ||
| return NewValidationError( | ||
| "sandbox.agent", | ||
| "false", | ||
| "disabling the agent sandbox requires the 'dangerously-disable-sandbox-agent' feature flag", | ||
| fmt.Sprintf("Add the feature flag to your workflow frontmatter:\n\nfeatures:\n dangerously-disable-sandbox-agent: true\nsandbox:\n agent: false\n\nSee: %s", constants.DocsSandboxURL), | ||
| ) | ||
| } | ||
| sandboxValidationLog.Print("sandbox.agent: false permitted by dangerously-disable-sandbox-agent feature flag") |
| // Check if sandbox.agent: false was specified | ||
| // In non-strict mode, this is allowed (with a warning shown at compile time) | ||
| // The strict mode check happens in validateStrictFirewall() | ||
| // This requires the "dangerously-disable-sandbox-agent" feature flag to be enabled. | ||
| // Without the feature flag, setting sandbox.agent: false is a validation error. | ||
| if sandboxConfig.Agent != nil && sandboxConfig.Agent.Disabled { |
![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 /tdd and /grill-with-docs — commenting with minor gaps to address, no blocking issues.
📋 Key Themes & Highlights
Positive Highlights
- ✅ The core gate is clean and correctly placed in
validateSandboxConfig, which runs unconditionally (not gated byskipValidation). - ✅ Both the acceptance path (flag present → compiles) and the rejection path (flag absent →
ValidationErrornaming the flag) are covered by tests. - ✅ The
DangerouslyDisableSandboxAgentFeatureFlagconstant doc follows the established pattern infeature_constants.go, with frontmatter example included. - ✅ Fixture updates across 6 test files are mechanical and correct — no missed callsites.
Observations
-
Env-var bypass path (
GH_AW_FEATURES) —isFeatureEnabledaccepts the flag via environment variable as well as frontmatter. For a gate whose stated purpose is to make the opt-in "visible in every workflow", it is worth deciding (and testing) whether the env-var path is intentional. See inline comment onsandbox_validation.go:82. -
Explicit
falseedge case —features: dangerously-disable-sandbox-agent: false+sandbox.agent: falseshould be rejected.parseFeatureValuehandles it correctly but there is no test. See inline comment onsandbox_agent_false_test.go:163. -
Pre-existing error message now incomplete — The
NewConfigurationErroratsandbox_validation.go:130(unchanged code) suggestssandbox: agent: falseas the remedy when MCP servers are missing, but does not mention that doing so now also requires the feature flag. Out of scope for this PR, but worth a follow-up.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 401.1 AIC · ⌖ 13.3 AIC
| // sandbox.agent: false is allowed in non-strict mode, so we don't error here | ||
| // The warning is emitted in compiler.go | ||
| sandboxValidationLog.Print("sandbox.agent: false detected, will be validated by strict mode check") | ||
| if !isFeatureEnabled(constants.DangerouslyDisableSandboxAgentFeatureFlag, workflowData) { |
There was a problem hiding this comment.
[/tdd] isFeatureEnabled also accepts this flag via the GH_AW_FEATURES environment variable, so GH_AW_FEATURES=dangerously-disable-sandbox-agent on the runner silently bypasses the frontmatter gate. This path is untested, and for a "dangerously"-named security flag the behaviour should be explicit.
💡 Suggested options
If the env-var bypass is intentional (useful for local dev/CI runner-level override), add a test to document it:
t.Run("GH_AW_FEATURES env var enables sandbox.agent: false", func(t *testing.T) {
t.Setenv("GH_AW_FEATURES", "dangerously-disable-sandbox-agent")
// compile a workflow with sandbox.agent: false but no frontmatter flag
// expect: compilation succeeds
})If the env-var bypass is not intended (the whole point is that the opt-in must be visible in the committed workflow file), isFeatureEnabled would need a frontmatter-only mode for security-critical flags.
Either way, a one-line comment on the isFeatureEnabled call here stating the intent would help reviewers.
| } | ||
|
|
||
| func TestSandboxAgentFalseRequiresFeatureFlag(t *testing.T) { | ||
| t.Run("sandbox.agent: false without feature flag is rejected", func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The new test only covers sandbox.agent: false with the flag entirely absent. An edge case worth locking in: setting dangerously-disable-sandbox-agent: false explicitly should also be rejected — parseFeatureValue returns false for a boolean false, so the code handles it correctly, but there is no test asserting that behaviour.
💡 Suggested additional sub-test
t.Run("sandbox.agent: false with feature flag explicitly false is rejected", func(t *testing.T) {
workflowsDir := t.TempDir()
markdown := `---
engine: copilot
features:
dangerously-disable-sandbox-agent: false
sandbox:
agent: false
strict: false
on: workflow_dispatch
---
Test workflow.
`
workflowPath := filepath.Join(workflowsDir, "test.md")
_ = os.WriteFile(workflowPath, []byte(markdown), 0644)
compiler := NewCompiler()
err := compiler.CompileWorkflow(workflowPath)
if err == nil {
t.Fatal("Expected compilation to fail when flag is explicitly false")
}
if !strings.Contains(err.Error(), "dangerously-disable-sandbox-agent") {
t.Fatalf("Expected error to reference flag, got: %v", err)
}
})
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES: Two integration-test files were not updated to include the new feature flag, so go test -tags=integration will fail after this merge.
### Broken integration tests (blocking)
pkg/workflow/sandbox_agent_disabled_test.go (build tag: integration) — 4 test cases broken
All four sandbox.agent: false cases in this file are missing dangerously-disable-sandbox-agent: true in their workflow frontmatter. With the new validateSandboxConfig logic, each will fail before reaching its expected outcome:
| Test case | Expected | Will actually get |
|---|---|---|
| "disables firewall but keeps MCP gateway" | require.NoError |
feature-flag validation error |
| "shows warning at compile time" | require.NoError |
feature-flag validation error |
TestSandboxAgentFalseWithTools |
require.NoError |
feature-flag validation error |
| "is refused in strict mode" | error containing "strict mode" |
error containing "dangerously-disable-sandbox-agent" (feature-flag check fires before strict-mode check) |
pkg/workflow/aw_info_steps_test.go (build tag: integration) — 1 test case broken
The "firewall disabled" test case (the markdown at the sandbox: agent: false line) calls compiler.CompileWorkflow and then t.Fatalf("Failed to compile workflow: %v", err) if it fails. With the new validation it will fail.
Fix: apply the same treatment used in this PR for all the other test files — add:
features:
dangerously-disable-sandbox-agent: trueto the frontmatter strings in those two files.
🔎 Code quality review by PR Code Quality Reviewer · 7.01 AIC · ⌖ 13.6 AIC
| t.Fatalf("Failed to write workflow file: %v", err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() |
There was a problem hiding this comment.
Fragile test: missing SetSkipValidation(true) breaks error-message assertion if the default changes.
Every other compiler := NewCompiler() in this file is immediately followed by compiler.SetSkipValidation(true) (lines 41-42, 93-94, 139-140). This test omits that call and silently inherits the current default (skipValidation: true, annotated in compiler_types.go as "for now").
💡 Why this is fragile
The test asserts the error message contains "dangerously-disable-sandbox-agent", which is only true when validateSandboxConfig is the first validator to fail. If skipValidation defaults change to false, schema validation runs first and can produce a different error (e.g. "missing required field: permissions"), causing the strings.Contains assertion to fail — masking the real reason.
Fix:
compiler := NewCompiler()
compiler.SetSkipValidation(true) // ensure schema validation is skipped so validateSandboxConfig fires first…ges; remove misleading SetSkipValidation(true) from feature-flag test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed both actionable review threads:
Commit: |
|
@copilot review all comments and address the unresolved integration-test feedback in sandbox_agent_disabled_test.go and aw_info_steps_test.go.
|
|
Please refresh the branch and rerun checks after those fixture updates.
|
sandbox.agent: falsepreviously compiled without restriction, making it easy to silently disable the agent firewall. This change gates it behind an explicit opt-in feature flag so the security trade-off is visible in every workflow that disables the sandbox.Behavior
Without the flag, compilation now fails:
To allow
sandbox.agent: false, the workflow must declare:Changes
pkg/constants/feature_constants.go— newDangerouslyDisableSandboxAgentFeatureFlag = "dangerously-disable-sandbox-agent"constantpkg/workflow/sandbox_validation.go—validateSandboxConfigreturns aValidationErrorwhensandbox.agent: falseis present without the feature flagpkg/workflow/sandbox_agent_false_test.go— acceptance test updated to include the feature flag; newTestSandboxAgentFalseRequiresFeatureFlagcovers the rejection pathcompiler_validators_test.go,importable_tools_test.go,pull_request_target_validation_test.go,workflow_run_validation_test.go) — workflows usingsandbox.agent: falseas test setup now carry the feature flag to preserve existing assertionspr-sous-chef: updated branch for run https://github.com/github/gh-aw/actions/runs/27236053686