Conversation
…y type in strict mode
When `sandbox.agent` is specified as an object without `id` or `type` (e.g., only
`version`), `getAgentType()` returned `""` and `isSandboxEnabled()` returned false,
silently disabling the AWF firewall.
Two changes:
1. `applySandboxDefaults` (sandbox.go): after setting a nil agent to AWF, also default
the type to AWF when the agent exists but has no id/type. This covers patterns like
`sandbox: { agent: { version: "v0.25.29" } }` where the user intended AWF.
2. `validateStrictSandboxCustomization` (strict_mode_sandbox_validation.go): in strict
mode, reject `sandbox.agent` objects that have no explicit `id`. Ambiguous
configurations are not accepted when strict: true.
Also fixes `.github/workflows/hourly-ci-cleaner.md` (which used `sandbox.agent.mounts`
without `id: awf`) and adds test coverage for both new behaviours.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5fc3fd85-6e8d-44a9-a20b-95bf0cd1689f
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…t clarity Agent-Logs-Url: https://github.com/github/gh-aw/sessions/5fc3fd85-6e8d-44a9-a20b-95bf0cd1689f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a sandbox enablement bug where sandbox.agent specified as an object without id/type could be treated as “sandbox disabled”, unintentionally running the agent on the host runner and breaking MCP connectivity.
Changes:
- Default
sandbox.agentto AWF when the agent object is present but noid/typeis provided. - Add strict-mode validation intended to reject ambiguous
sandbox.agentconfigurations. - Update workflow config and add/extend unit tests covering the new behaviors.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/sandbox.go |
Extends defaulting logic to treat “version-only agent object” as AWF-enabled. |
pkg/workflow/sandbox_test.go |
Adds tests for version-only / empty agent object defaulting and disabled preservation. |
pkg/workflow/strict_mode_sandbox_validation.go |
Adds strict-mode rejection for ambiguous sandbox.agent configurations. |
pkg/workflow/strict_mode_sandbox_validation_test.go |
Adds strict-mode tests for missing-id / empty agent object and disabled exception. |
.github/workflows/hourly-ci-cleaner.md |
Adds explicit id: awf under sandbox.agent. |
.github/workflows/hourly-ci-cleaner.lock.yml |
Regenerates lock metadata to reflect updated workflow content. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
| // the type to awf so the sandbox is always enabled. This prevents a bare | ||
| // sandbox.agent object from silently disabling the firewall by leaving the type empty. | ||
| // Note: this block is only reached when Agent != nil and Disabled == false (the | ||
| // Disabled case returned early above). | ||
| if !isSupportedSandboxType(getAgentType(sandboxConfig.Agent)) { |
| // In strict mode, sandbox.agent must carry an explicit type/id so the sandbox | ||
| // configuration is unambiguous. A bare object (e.g. { version: "v0.25.29" } | ||
| // with no id) would silently default to AWF in non-strict builds but that | ||
| // implicit defaulting is not acceptable in strict mode. | ||
| if !agent.Disabled { | ||
| if !isSupportedSandboxType(getAgentType(agent)) { | ||
| return fmt.Errorf( | ||
| "strict mode: 'sandbox.agent' must specify an explicit 'id' (e.g., id: awf). " + | ||
| "A sandbox agent without an 'id' is ambiguous and not allowed in strict mode. " + | ||
| "Add 'id: awf' to your sandbox.agent configuration. " + | ||
| "See: https://github.github.com/gh-aw/reference/sandbox/", | ||
| ) | ||
| } |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent
Test Classification DetailsView all 6 test cases
Test Inflation Note
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25240361044
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ct mode explicit id Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (115 new lines in The Design Decision Gate has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: Decision Summary (from the draft)The draft captures two tightly coupled decisions introduced by this PR:
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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does — especially for security-boundary decisions like this one. Future contributors need to understand why the sandbox defaults the way it does. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25240361053
|
Summary
Fixes the bug where
sandbox.agentspecified as an object withoutidortype(e.g.{ version: "v0.25.29" }) causedisSandboxEnabled()to returnfalse, silently disabling the AWF firewall and causing the agent to run on the host runner — breaking MCP connectivity.Root Cause
getAgentType()returns""when neitherIDnorTypeis set onAgentSandboxConfig.isSupportedSandboxType("")returnsfalse, soisSandboxEnabled()returnedfalseeven though the user clearly intended to use AWF (e.g. by pinning a specific AWF version).applySandboxDefaultsonly defaulted to AWF for anilagent, not for a non-nil agent with an empty type.Changes
pkg/workflow/sandbox.goapplySandboxDefaults: after handling theAgent == nilcase, also defaultAgent.Typetoawfwhen the agent object is present but has noid/type. This covers patterns like:pkg/workflow/strict_mode_sandbox_validation.govalidateStrictSandboxCustomization: in strict mode, reject any non-disabledsandbox.agentthat has no explicitid. Ambiguous configurations are not acceptable in strict mode — users must writeid: awfexplicitly..github/workflows/hourly-ci-cleaner.mdAdded the missing
id: awfto thesandbox.agentblock (was usingmountswithout an explicit id).Tests
TestApplySandboxDefaults: added cases for version-only, empty-ID, and disabled agent configsTestValidateStrictSandboxCustomization: added cases for agent without id and empty agent in strict mode; confirmed disabled agent is exempt