Enforce sandbox-disable justification strings and surface new AWF import/safe-output constraints#38228
Conversation
…safe-output docs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot remove imports.if mentioned in documentation. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request updates gh-aw validation, codemods, tests, and documentation to align with newer AWF/compiler behavior—most notably requiring an explicit string justification to disable the agent sandbox, and surfacing safe-output field constraints to prevent downstream validation failures.
Changes:
- Enforces
features.dangerously-disable-sandbox-agentas a non-expression justification string (min length 20) whensandbox.agent: falseis used, and updates validation/tests accordingly. - Extends the network firewall codemod to inject the new sandbox-disable feature value when migrating
network.firewall: false. - Updates docs to reflect the new sandbox-disable requirement and safe-output
create_issue.bodylength limits, and adds a compiler test covering conditional import gating.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_run_validation_test.go | Updates test fixtures to use the new sandbox-disable justification string. |
| pkg/workflow/sandbox_validation.go | Implements validation requiring a justification string (min length, no expressions) to disable sandbox. |
| pkg/workflow/sandbox_test.go | Adds unit tests for valid/missing/short/expression sandbox-disable justifications. |
| pkg/workflow/sandbox_agent_false_test.go | Expands integration tests for rejecting short/expression justifications. |
| pkg/workflow/pull_request_target_validation_test.go | Updates fixtures to use justification strings for sandbox disable. |
| pkg/workflow/imports_test.go | Adds test coverage for imports[].if conditional gating behavior. |
| pkg/workflow/importable_tools_test.go | Updates fixtures to use justification strings for sandbox disable. |
| pkg/workflow/compiler_validators_test.go | Updates validator tests to reflect new justification-string requirement. |
| pkg/constants/feature_constants.go | Updates feature flag usage example to the new string form. |
| pkg/cli/codemod_network_firewall.go | Injects sandbox-disable justification when codemodding network.firewall disable cases. |
| pkg/cli/codemod_network_firewall_test.go | Adds assertions/tests verifying codemod adds/preserves sandbox-disable justification strings. |
| docs/src/content/docs/reference/safe-outputs.md | Documents create_issue.body min/max length constraints. |
| .github/aw/upgrade-agentic-workflows.md | Updates upgrade guidance to include the required sandbox-disable justification. |
| .github/aw/syntax-agentic.md | Documents the required justification-string shape and intent for sandbox disabling. |
| .github/aw/safe-outputs-content.md | Adds explicit create_issue.body length requirements to safe-outputs guidance. |
| .github/aw/experiments.md | Refines guidance text around evaluation order across import boundaries. |
| .github/aw/create-agentic-workflow.md | Adds authoring guidance to avoid too-short create-issue bodies. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 17/17 changed files
- Comments generated: 2
| ) | ||
| } | ||
| sandboxValidationLog.Printf("sandbox.agent: false permitted by %s feature flag", constants.DangerouslyDisableSandboxAgentFeatureFlag) | ||
| sandboxValidationLog.Printf("sandbox.agent: false permitted by %s justification: %q", constants.DangerouslyDisableSandboxAgentFeatureFlag, justification) |
| for i := featuresIdx + 1; i < featuresEnd; i++ { | ||
| if strings.HasPrefix(strings.TrimSpace(lines[i]), featureKey) { | ||
| return lines | ||
| } |
|
✅ 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. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (377 new lines in 📄 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. A security-sensitive escape hatch like disabling the agent sandbox is exactly the kind of decision future contributors will want the rationale for. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 96/100 — Excellent
📊 Metrics & Test Classification (8 new scenarios analyzed)
Test Classification Details
Language SupportTests analyzed:
Inflation Ratios
|
![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: 96/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 8 new test scenarios enforce behavioral contracts, with 87.5% covering error/edge cases. No mock-library violations or missing build tags detected. One minor non-blocking note: 3 assertions in TestNetworkFirewallCodemod_PreservesExistingSandboxDisableJustification are missing descriptive message arguments.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on test-coverage gaps in the new security-critical validation path.
📋 Key Themes & Highlights
Key Themes
- Missing breaking-change test case:
dangerously-disable-sandbox-agent: true(the old boolean) is not covered by a unit test. This is the most likely migration failure mode for users upgrading from the boolean form, and the resulting error message is generic and unhelpful. - Codemod injection path undertested:
ensureSandboxDisableFeatureFlaghas no test for whenfeatures:already exists with other flags. The key injection into an existing block is the most structurally risky code path in the codemod. - Security-gate regex lacks adversarial coverage: The expression-blocking regex guards a meaningful security constraint. Without near-miss test cases, a future regex edit could silently allow bypasses.
- Docs terminology inconsistency: "dangerous-disable justification feature" does not match
dangerously-disable-sandbox-agent.
Positive Highlights
- ✅ Boolean → string breaking change is well-motivated and the error message + suggestion text are clear and actionable
- ✅ Idempotent codemod (
TestNetworkFirewallCodemod_PreservesExistingSandboxDisableJustification) is a good defensive test - ✅
getFeatureValueCaseInsensitiveis a nice robustness touch - ✅ Three new integration-level reject tests (missing, too short, expression) in
sandbox_agent_false_test.goprovide solid end-to-end coverage - ✅ Conditional import test is a good first exercise of the import gating path
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 592.3 AIC · ⌖ 14 AIC
| }, | ||
| expectError: true, | ||
| errorMsg: "expressions", | ||
| }, |
There was a problem hiding this comment.
[/tdd] Missing test case: dangerously-disable-sandbox-agent: true (the old boolean form) being rejected. This is the most common migration failure mode — existing workflows with true will hit validation and see a generic "feature must be a string, got bool" error with no migration guidance.
💡 Suggested test case to add after this block
{
name: "sandbox.agent false with legacy boolean flag is rejected",
data: &WorkflowData{
Features: map[string]any{
"dangerously-disable-sandbox-agent": true,
},
SandboxConfig: &SandboxConfig{
Agent: &AgentSandboxConfig{Disabled: true},
},
},
expectError: true,
errorMsg: "string",
},This locks the breaking-change path: true → "feature must be a string" error, ensuring a future refactor of getSandboxDisableJustification cannot silently drop the type check.
| require.NoError(t, err) | ||
| assert.True(t, applied) | ||
| assert.Contains(t, result, `dangerously-disable-sandbox-agent: "already documented justification string with enough detail"`) | ||
| assert.NotContains(t, result, migratedSandboxDisableJustification, "Should not overwrite existing justification") |
There was a problem hiding this comment.
[/tdd] There is no test for when features: already exists with other keys (not dangerously-disable-sandbox-agent). The if featuresIdx >= 0 branch in ensureSandboxDisableFeatureFlag injects the key into the existing block — this path is currently untested.
💡 Suggested test scenario
Add a test where the content includes:
features:
some-other-flag: truebut not dangerously-disable-sandbox-agent. After the codemod runs, assert:
dangerously-disable-sandbox-agentappears inside thefeatures:blocksome-other-flag: trueis preserved- No duplicate
features:top-level key is added
Without this, a line-index off-by-one in ensureSandboxDisableFeatureFlag could corrupt the features: block without any test catching it.
|
|
||
| var networkFirewallCodemodLog = logger.New("cli:codemod_network_firewall") | ||
|
|
||
| const migratedSandboxDisableJustification = "migrated from deprecated network.firewall disable setting" |
There was a problem hiding this comment.
[/tdd] The migratedSandboxDisableJustification constant is auto-injected into migrated workflows, which are then validated by getSandboxDisableJustification() (≥20 chars, no expressions). There is no test asserting the constant itself satisfies those rules. A future edit that shortens the string below 20 characters would silently produce codemod output that fails validation.
💡 Suggested cross-validation test
func TestMigratedJustificationPassesValidation(t *testing.T) {
data := &workflow.WorkflowData{
Features: map[string]any{
"dangerously-disable-sandbox-agent": migratedSandboxDisableJustification,
},
SandboxConfig: &workflow.SandboxConfig{
Agent: &workflow.AgentSandboxConfig{Disabled: true},
},
}
// This would require exporting getSandboxDisableJustification or
// testing via the full compiler path:
require.NoError(t, compiler.CompileWorkflow(workflowWithMigratedJustification))
}Alternatively, a simple length assertion in a package test is enough to lock the contract.
| ``` | ||
|
|
||
| - To disable the agent firewall while keeping MCP gateway enabled (not allowed in strict mode): | ||
| - To disable the agent firewall while keeping MCP gateway enabled, you must provide the dangerous-disable justification feature: |
There was a problem hiding this comment.
[/grill-with-docs] Wording inconsistency: "the dangerous-disable justification feature" does not match the actual feature key dangerously-disable-sandbox-agent. A reader skimming this doc will not know what key to write.
Suggest replacing with: "you must provide the dangerously-disable-sandbox-agent justification:"
| name: "sandbox.agent false with expression justification", | ||
| data: &WorkflowData{ | ||
| Features: map[string]any{ | ||
| "dangerously-disable-sandbox-agent": "${{ inputs.reason }}", |
There was a problem hiding this comment.
[/tdd] The expression-blocking regex is a security gate, but only one expression form is tested (${{ inputs.reason }}). There is no adversarial coverage for near-misses that could indicate bypass or false-positive.
💡 Suggested additional cases in sandbox_test.go or a dedicated regex test
| Input | Expected |
|---|---|
"no expression, long enough to pass the min length check" |
✅ pass |
"long enough ${{ a }} and ${{ b }} double expression" |
❌ rejected (still matches) |
"single brace ${inputs.reason} long enough string here" |
✅ pass (not GH Actions syntax) |
"$(inputs.reason) long enough alternative syntax here" |
✅ pass (not GH Actions syntax) |
Without this, a future regex change (e.g. swapping [\s\S]* to .+) could silently create a bypass without any test failing.
| `{{#if experiments.strategy == "eager"}}`, | ||
| "{{/if}}", | ||
| "needs.activation.outputs.strategy == 'eager'", | ||
| } |
There was a problem hiding this comment.
[/tdd] These assertions verify that the template guard syntax is present in the compiled output, but not that the imported step content is wrapped inside the {{#if}} block. The step echo "from import" could appear outside the guard and all three assertions would still pass.
💡 Stronger structural assertion
ifIdx := strings.Index(compiled, `{{#if experiments.strategy == "eager"}}`)
endifIdx := strings.Index(compiled, "{{/if}}")
stepIdx := strings.Index(compiled, `echo "from import"`)
require.Greater(t, ifIdx, -1, "{{#if}} guard should be present")
require.Greater(t, endifIdx, -1, "{{/if}} guard should be present")
require.Greater(t, stepIdx, ifIdx, "imported step should appear after {{#if}} guard")
require.Less(t, stepIdx, endifIdx, "imported step should appear before {{/if}} guard")This turns a structural smoke test into a real gating assertion.
| } | ||
|
|
||
| trimmed := strings.TrimSpace(justification) | ||
| if len(trimmed) < minSandboxDisableJustificationLength { |
There was a problem hiding this comment.
[/tdd] len(trimmed) counts UTF-8 bytes, but the documentation says "minimum 20 characters". For ASCII-only justifications (the expected case) these are equivalent, but a user writing a justification with multi-byte Unicode characters (e.g. "Ü" = 2 bytes) or non-Latin scripts could see surprising behavior.
Consider utf8.RuneCountInString(trimmed) to match what the docs say, or update docs to say "20 bytes".
| return trimmed, nil | ||
| } | ||
|
|
||
| func getFeatureValueCaseInsensitive(features map[string]any, flagName string) (any, bool) { |
There was a problem hiding this comment.
[/grill-with-docs] getFeatureValueCaseInsensitive silently accepts case variants of the feature key (e.g. DANGEROUSLY-DISABLE-SANDBOX-AGENT: "..." is valid). This behaviour is undocumented in syntax-agentic.md and has no test coverage for the case-insensitive match path (the loop is only exercised when the exact-match on line 179 fails).
Either add a test that passes DANGEROUSLY-DISABLE-SANDBOX-AGENT as a key and verifies it is accepted, or document the case-insensitivity explicitly so it is a deliberate, tested contract rather than an implementation side-effect.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 12.7 AIC · ⌖ 13.3 AIC
This updates
gh-awsurfacing for newly introduced AWF/compiler behavior and closes migration gaps that could lead to compile-time failures. It enforces stronger sandbox-disable intent, keeps import conditional behavior covered by tests, and aligns safe-output docs with current validators.Sandbox disable feature now requires explicit justification
sandbox.agent: falsenow requiresfeatures.dangerously-disable-sandbox-agentto be a plain string justification (min 20 chars, expressions disallowed).network.firewall -> sandbox.agent) now injects a safe default justification when migrating disable cases.Import-level conditional gating behavior is covered
if:gating affects imported prompt/steps as expected.imports[].ifmentions from documentation per review feedback.Upgrade and syntax docs corrected for current behavior
sandbox.agent: false.syntax-agentic.mdto describe the flag’s required shape and security intent.Safe-output
create_issue.bodylimits surfacedcreate_issue.bodyconstraints (min 20, max 65000) in safe-output references and workflow-authoring guidance so generated workflows avoid placeholder-short bodies.