From 0fa978cd96bc022f996209f93c6ca7784b69056a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 2 May 2026 01:21:54 +0000 Subject: [PATCH 1/3] fix: default sandbox agent type to AWF when not explicit; reject empty 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> --- .github/workflows/hourly-ci-cleaner.lock.yml | 30 +++++----- .github/workflows/hourly-ci-cleaner.md | 1 + pkg/workflow/sandbox.go | 11 ++++ pkg/workflow/sandbox_test.go | 55 +++++++++++++++++-- .../strict_mode_sandbox_validation.go | 16 ++++++ .../strict_mode_sandbox_validation_test.go | 33 +++++++++++ 6 files changed, 127 insertions(+), 19 deletions(-) diff --git a/.github/workflows/hourly-ci-cleaner.lock.yml b/.github/workflows/hourly-ci-cleaner.lock.yml index 75b71f9c5fd..0c37573f6a2 100644 --- a/.github/workflows/hourly-ci-cleaner.lock.yml +++ b/.github/workflows/hourly-ci-cleaner.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"de62dfd5c5ecd54286230921c1400c0cd8753073f35b1fc93f6524bf1e330793","strict":true,"agent_id":"claude"} +# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"426f66db494453f3e49396a8e2b342b5a5f7579d10f7590f401886df0237ed9d","strict":true,"agent_id":"claude"} # gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","GH_AW_CI_TRIGGER_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"373c709c69115d41ff229c7e5df9f8788daa9553","version":"v9"},{"repo":"actions/setup-go","sha":"4a3601121dd01d1626a1e23e37211e3254c1c06c","version":"v6.4.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.25.29","digest":"sha256:e68f37e36962dcb3f3d1de680a49bc2302cefd001b941a7dc377155ec7ce42f4","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.25.29@sha256:e68f37e36962dcb3f3d1de680a49bc2302cefd001b941a7dc377155ec7ce42f4"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.29","digest":"sha256:d1219e4110684402aabbeb5a43858f26790c9d0be210581cf3f7a521bd2c87b6","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.29@sha256:d1219e4110684402aabbeb5a43858f26790c9d0be210581cf3f7a521bd2c87b6"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.25.29","digest":"sha256:29917488eb90a01ff9544ffeeb5cc26434a8ea16d69ae8972f5f6be0e567e276","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.25.29@sha256:29917488eb90a01ff9544ffeeb5cc26434a8ea16d69ae8972f5f6be0e567e276"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.25.29","digest":"sha256:8a71ad9e40454051672312917e51567abfb8251d7c294d086c48f63d84e4cb53","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.25.29@sha256:8a71ad9e40454051672312917e51567abfb8251d7c294d086c48f63d84e4cb53"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.3"},{"image":"ghcr.io/github/github-mcp-server:v1.0.3","digest":"sha256:2ac27ef03461ef2b877031b838a7d1fd7f12b12d4ace7796d8cad91446d55959","pinned_image":"ghcr.io/github/github-mcp-server:v1.0.3@sha256:2ac27ef03461ef2b877031b838a7d1fd7f12b12d4ace7796d8cad91446d55959"},{"image":"node:lts-alpine","digest":"sha256:d1b3b4da11eefd5941e7f0b9cf17783fc99d9c6fc34884a665f40a06dbdfc94f","pinned_image":"node:lts-alpine@sha256:d1b3b4da11eefd5941e7f0b9cf17783fc99d9c6fc34884a665f40a06dbdfc94f"}]} # ___ _ _ # / _ \ | | (_) @@ -182,23 +182,23 @@ jobs: run: | bash "${RUNNER_TEMP}/gh-aw/actions/create_prompt_first.sh" { - cat << 'GH_AW_PROMPT_a731ae99ed1ce5c7_EOF' + cat << 'GH_AW_PROMPT_1d4032b69939e80e_EOF' - GH_AW_PROMPT_a731ae99ed1ce5c7_EOF + GH_AW_PROMPT_1d4032b69939e80e_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/xpia.md" cat "${RUNNER_TEMP}/gh-aw/prompts/temp_folder_prompt.md" cat "${RUNNER_TEMP}/gh-aw/prompts/markdown.md" cat "${RUNNER_TEMP}/gh-aw/prompts/safe_outputs_prompt.md" - cat << 'GH_AW_PROMPT_a731ae99ed1ce5c7_EOF' + cat << 'GH_AW_PROMPT_1d4032b69939e80e_EOF' Tools: create_pull_request, missing_tool, missing_data, noop - GH_AW_PROMPT_a731ae99ed1ce5c7_EOF + GH_AW_PROMPT_1d4032b69939e80e_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/safe_outputs_create_pull_request.md" - cat << 'GH_AW_PROMPT_a731ae99ed1ce5c7_EOF' + cat << 'GH_AW_PROMPT_1d4032b69939e80e_EOF' - GH_AW_PROMPT_a731ae99ed1ce5c7_EOF + GH_AW_PROMPT_1d4032b69939e80e_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/mcp_cli_tools_prompt.md" - cat << 'GH_AW_PROMPT_a731ae99ed1ce5c7_EOF' + cat << 'GH_AW_PROMPT_1d4032b69939e80e_EOF' The following GitHub context information is available for this workflow: {{#if __GH_AW_GITHUB_ACTOR__ }} @@ -227,13 +227,13 @@ jobs: {{/if}} - GH_AW_PROMPT_a731ae99ed1ce5c7_EOF + GH_AW_PROMPT_1d4032b69939e80e_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/cli_proxy_with_safeoutputs_prompt.md" - cat << 'GH_AW_PROMPT_a731ae99ed1ce5c7_EOF' + cat << 'GH_AW_PROMPT_1d4032b69939e80e_EOF' {{#runtime-import .github/agents/ci-cleaner.agent.md}} {{#runtime-import .github/workflows/hourly-ci-cleaner.md}} - GH_AW_PROMPT_a731ae99ed1ce5c7_EOF + GH_AW_PROMPT_1d4032b69939e80e_EOF } > "$GH_AW_PROMPT" - name: Interpolate variables and render templates uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 @@ -470,9 +470,9 @@ jobs: mkdir -p "${RUNNER_TEMP}/gh-aw/safeoutputs" mkdir -p /tmp/gh-aw/safeoutputs mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs - cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_c91e96cdcd4885fa_EOF' + cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_a09071c5ba8b847d_EOF' {"create_pull_request":{"expires":48,"max":1,"max_patch_files":100,"max_patch_size":1024,"protect_top_level_dot_folders":true,"protected_files":["package.json","bun.lockb","bunfig.toml","deno.json","deno.jsonc","deno.lock","global.json","NuGet.Config","Directory.Packages.props","mix.exs","mix.lock","go.mod","go.sum","stack.yaml","stack.yaml.lock","pom.xml","build.gradle","build.gradle.kts","settings.gradle","settings.gradle.kts","gradle.properties","package-lock.json","yarn.lock","pnpm-lock.yaml","npm-shrinkwrap.json","requirements.txt","Pipfile","Pipfile.lock","pyproject.toml","setup.py","setup.cfg","Gemfile","Gemfile.lock","uv.lock","CODEOWNERS","DESIGN.md","CLAUDE.md","AGENTS.md"],"protected_files_policy":"fallback-to-issue","title_prefix":"[ca] "},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"report_incomplete":{}} - GH_AW_SAFE_OUTPUTS_CONFIG_c91e96cdcd4885fa_EOF + GH_AW_SAFE_OUTPUTS_CONFIG_a09071c5ba8b847d_EOF - name: Write Safe Outputs Tools env: GH_AW_TOOLS_META_JSON: | @@ -677,7 +677,7 @@ jobs: export MCP_GATEWAY_DOCKER_COMMAND='docker run -i --rm --network host --add-host host.docker.internal:127.0.0.1 --user '"${MCP_GATEWAY_UID}"':'"${MCP_GATEWAY_GID}"' --group-add '"${DOCKER_SOCK_GID}"' -v /var/run/docker.sock:/var/run/docker.sock -e MCP_GATEWAY_PORT -e MCP_GATEWAY_DOMAIN -e MCP_GATEWAY_API_KEY -e MCP_GATEWAY_PAYLOAD_DIR -e MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD -e DEBUG -e MCP_GATEWAY_LOG_DIR -e GH_AW_MCP_LOG_DIR -e GH_AW_SAFE_OUTPUTS -e GH_AW_SAFE_OUTPUTS_CONFIG_PATH -e GH_AW_SAFE_OUTPUTS_TOOLS_PATH -e GH_AW_ASSETS_BRANCH -e GH_AW_ASSETS_MAX_SIZE_KB -e GH_AW_ASSETS_ALLOWED_EXTS -e DEFAULT_BRANCH -e GITHUB_MCP_SERVER_TOKEN -e GITHUB_MCP_GUARD_MIN_INTEGRITY -e GITHUB_MCP_GUARD_REPOS -e GITHUB_REPOSITORY -e GITHUB_SERVER_URL -e GITHUB_SHA -e GITHUB_WORKSPACE -e GITHUB_TOKEN -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RUN_ATTEMPT -e GITHUB_JOB -e GITHUB_ACTION -e GITHUB_EVENT_NAME -e GITHUB_EVENT_PATH -e GITHUB_ACTOR -e GITHUB_ACTOR_ID -e GITHUB_TRIGGERING_ACTOR -e GITHUB_WORKFLOW -e GITHUB_WORKFLOW_REF -e GITHUB_WORKFLOW_SHA -e GITHUB_REF -e GITHUB_REF_NAME -e GITHUB_REF_TYPE -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GH_AW_SAFE_OUTPUTS_PORT -e GH_AW_SAFE_OUTPUTS_API_KEY -v /tmp/gh-aw/mcp-payloads:/tmp/gh-aw/mcp-payloads:rw -v /opt:/opt:ro -v /tmp:/tmp:rw -v '"${GITHUB_WORKSPACE}"':'"${GITHUB_WORKSPACE}"':rw ghcr.io/github/gh-aw-mcpg:v0.3.3' GH_AW_NODE=$(which node 2>/dev/null || command -v node 2>/dev/null || echo node) - cat << GH_AW_MCP_CONFIG_7078a8594a51c2fb_EOF | "$GH_AW_NODE" "${RUNNER_TEMP}/gh-aw/actions/start_mcp_gateway.cjs" + cat << GH_AW_MCP_CONFIG_2d8fd99c39dba0eb_EOF | "$GH_AW_NODE" "${RUNNER_TEMP}/gh-aw/actions/start_mcp_gateway.cjs" { "mcpServers": { "safeoutputs": { @@ -702,7 +702,7 @@ jobs: "payloadDir": "${MCP_GATEWAY_PAYLOAD_DIR}" } } - GH_AW_MCP_CONFIG_7078a8594a51c2fb_EOF + GH_AW_MCP_CONFIG_2d8fd99c39dba0eb_EOF - name: Mount MCP servers as CLIs id: mount-mcp-clis continue-on-error: true diff --git a/.github/workflows/hourly-ci-cleaner.md b/.github/workflows/hourly-ci-cleaner.md index 7ac83acd00a..44a1a177e33 100644 --- a/.github/workflows/hourly-ci-cleaner.md +++ b/.github/workflows/hourly-ci-cleaner.md @@ -31,6 +31,7 @@ tools: edit: sandbox: agent: + id: awf mounts: - "/usr/bin/make:/usr/bin/make:ro" - "/usr/bin/go:/usr/bin/go:ro" diff --git a/pkg/workflow/sandbox.go b/pkg/workflow/sandbox.go index b4fc2cf90e0..dacf048bb04 100644 --- a/pkg/workflow/sandbox.go +++ b/pkg/workflow/sandbox.go @@ -135,6 +135,7 @@ func migrateSRTToAWF(sandboxConfig *SandboxConfig) *SandboxConfig { // applySandboxDefaults applies default values to sandbox configuration // If no sandbox config exists, creates one with awf as default agent // If sandbox config exists but has no agent, sets agent to awf (unless agent is explicitly disabled) +// If sandbox.agent is an object with no id/type (e.g., version-only), defaults the type to awf func applySandboxDefaults(sandboxConfig *SandboxConfig, engineConfig *EngineConfig) *SandboxConfig { // First, migrate any SRT references to AWF (codemod) sandboxConfig = migrateSRTToAWF(sandboxConfig) @@ -168,6 +169,16 @@ func applySandboxDefaults(sandboxConfig *SandboxConfig, engineConfig *EngineConf sandboxConfig.Agent = &AgentSandboxConfig{ Type: SandboxTypeAWF, } + return sandboxConfig + } + + // If sandbox.agent is configured but has no type/ID set (e.g., version-only object + // like { version: "v0.25.29" }), default 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. + if !isSupportedSandboxType(getAgentType(sandboxConfig.Agent)) { + sandboxLog.Print("Sandbox agent has no type/ID configured, defaulting to awf") + sandboxConfig.Agent.Type = SandboxTypeAWF } return sandboxConfig diff --git a/pkg/workflow/sandbox_test.go b/pkg/workflow/sandbox_test.go index 41de09faa50..2d4fc289236 100644 --- a/pkg/workflow/sandbox_test.go +++ b/pkg/workflow/sandbox_test.go @@ -89,16 +89,63 @@ func TestApplySandboxDefaults(t *testing.T) { }, }, }, + { + // version-only object (no id/type) must default to AWF so the sandbox is + // always enabled, matching the previous analysis of the smoke-gemini bug. + name: "version-only agent defaults to AWF", + config: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Version: "v0.25.29", + }, + }, + engine: &EngineConfig{ID: "gemini"}, + expected: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Type: SandboxTypeAWF, + Version: "v0.25.29", + }, + }, + }, + { + // An agent object with only an empty string ID must also default to AWF. + name: "empty ID agent defaults to AWF", + config: &SandboxConfig{ + Agent: &AgentSandboxConfig{}, + }, + engine: &EngineConfig{ID: "copilot"}, + expected: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Type: SandboxTypeAWF, + }, + }, + }, + { + // Explicitly disabled agent must never be overridden. + name: "disabled agent is preserved", + config: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Disabled: true, + }, + }, + engine: nil, + expected: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Disabled: true, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result := applySandboxDefaults(tt.config, tt.engine) - if tt.expected != nil { - require.NotNil(t, result) - require.NotNil(t, result.Agent) - assert.Equal(t, tt.expected.Agent.Type, result.Agent.Type) + require.NotNil(t, result) + require.NotNil(t, result.Agent) + assert.Equal(t, tt.expected.Agent.Type, result.Agent.Type, "agent type") + if tt.expected.Agent.Version != "" { + assert.Equal(t, tt.expected.Agent.Version, result.Agent.Version, "agent version") } + assert.Equal(t, tt.expected.Agent.Disabled, result.Agent.Disabled, "agent disabled flag") }) } } diff --git a/pkg/workflow/strict_mode_sandbox_validation.go b/pkg/workflow/strict_mode_sandbox_validation.go index 46881adb6b0..f295855f62d 100644 --- a/pkg/workflow/strict_mode_sandbox_validation.go +++ b/pkg/workflow/strict_mode_sandbox_validation.go @@ -25,6 +25,9 @@ func internalSandboxFieldError(fieldPath string) error { // - sandbox.agent.command, sandbox.agent.args, sandbox.agent.env (AWF customization) // - sandbox.mcp.container, sandbox.mcp.version, sandbox.mcp.entrypoint, // sandbox.mcp.args, sandbox.mcp.entrypointArgs (MCP gateway customization) +// +// Additionally, a sandbox.agent object without an explicit 'id' field is rejected in +// strict mode: users must be unambiguous about which sandbox they are enabling. func (c *Compiler) validateStrictSandboxCustomization(sandboxConfig *SandboxConfig) error { if !c.strictMode { strictModeValidationLog.Printf("Strict mode disabled, skipping sandbox customization validation") @@ -37,6 +40,19 @@ func (c *Compiler) validateStrictSandboxCustomization(sandboxConfig *SandboxConf // Check agent sandbox internal fields if agent := sandboxConfig.Agent; agent != nil { + // 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 && !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/", + ) + } + if agent.Command != "" { return internalSandboxFieldError("sandbox.agent.command") } diff --git a/pkg/workflow/strict_mode_sandbox_validation_test.go b/pkg/workflow/strict_mode_sandbox_validation_test.go index 73c07916593..09de46894b8 100644 --- a/pkg/workflow/strict_mode_sandbox_validation_test.go +++ b/pkg/workflow/strict_mode_sandbox_validation_test.go @@ -133,6 +133,39 @@ func TestValidateStrictSandboxCustomization(t *testing.T) { }, expectError: false, }, + { + // A bare sandbox.agent object with no id/type is ambiguous and must be + // rejected in strict mode. Users must write id: awf explicitly. + name: "sandbox.agent without id is rejected in strict mode", + sandbox: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Version: "v0.25.29", + }, + }, + expectError: true, + errorMsg: "strict mode: 'sandbox.agent' must specify an explicit 'id'", + }, + { + // An empty AgentSandboxConfig (no id, no type, no version) is equally + // ambiguous and must be rejected in strict mode. + name: "empty sandbox.agent is rejected in strict mode", + sandbox: &SandboxConfig{ + Agent: &AgentSandboxConfig{}, + }, + expectError: true, + errorMsg: "strict mode: 'sandbox.agent' must specify an explicit 'id'", + }, + { + // sandbox.agent: false (Disabled) is handled by validateStrictFirewall, not here. + // validateStrictSandboxCustomization must not produce an additional error for it. + name: "disabled sandbox.agent is not rejected here (handled by validateStrictFirewall)", + sandbox: &SandboxConfig{ + Agent: &AgentSandboxConfig{ + Disabled: true, + }, + }, + expectError: false, + }, } for _, tt := range tests { From 0e2b161bf25c829657797424aa65b695e51bbb52 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 2 May 2026 01:26:12 +0000 Subject: [PATCH 2/3] refactor: address code review - use positive nested logic, add comment 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> --- pkg/workflow/sandbox.go | 10 ++++++---- pkg/workflow/strict_mode_sandbox_validation.go | 16 +++++++++------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/workflow/sandbox.go b/pkg/workflow/sandbox.go index dacf048bb04..1d040932742 100644 --- a/pkg/workflow/sandbox.go +++ b/pkg/workflow/sandbox.go @@ -172,10 +172,12 @@ func applySandboxDefaults(sandboxConfig *SandboxConfig, engineConfig *EngineConf return sandboxConfig } - // If sandbox.agent is configured but has no type/ID set (e.g., version-only object - // like { version: "v0.25.29" }), default 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. + // If sandbox.agent is configured but has no type/ID set (e.g., a version-only object + // like { version: "v0.25.29" } that reached here without a prior `return`), default + // 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)) { sandboxLog.Print("Sandbox agent has no type/ID configured, defaulting to awf") sandboxConfig.Agent.Type = SandboxTypeAWF diff --git a/pkg/workflow/strict_mode_sandbox_validation.go b/pkg/workflow/strict_mode_sandbox_validation.go index f295855f62d..f52002f3299 100644 --- a/pkg/workflow/strict_mode_sandbox_validation.go +++ b/pkg/workflow/strict_mode_sandbox_validation.go @@ -44,13 +44,15 @@ func (c *Compiler) validateStrictSandboxCustomization(sandboxConfig *SandboxConf // 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 && !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/", - ) + 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/", + ) + } } if agent.Command != "" { From d17923a3701e5d68b8c4fe77a199fa3b21ae8a26 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 2 May 2026 01:34:19 +0000 Subject: [PATCH 3/3] docs(adr): add draft ADR-29663 for sandbox agent AWF default and strict mode explicit id Co-Authored-By: Claude Sonnet 4.6 --- ...default-awf-and-strict-mode-explicit-id.md | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/adr/29663-sandbox-agent-default-awf-and-strict-mode-explicit-id.md diff --git a/docs/adr/29663-sandbox-agent-default-awf-and-strict-mode-explicit-id.md b/docs/adr/29663-sandbox-agent-default-awf-and-strict-mode-explicit-id.md new file mode 100644 index 00000000000..870e9bda474 --- /dev/null +++ b/docs/adr/29663-sandbox-agent-default-awf-and-strict-mode-explicit-id.md @@ -0,0 +1,74 @@ +# ADR-29663: Default Sandbox Agent Type to AWF for Ambiguous Configurations and Require Explicit ID in Strict Mode + +**Date**: 2026-05-02 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The `sandbox.agent` configuration in gh-aw workflow files allows users to customize the AWF (Agent Workflow Firewall) sandbox that isolates agent execution. A user can specify just a version pin — for example `{ version: "v0.25.29" }` — without providing an explicit `id` or `type` field, intending to use the AWF sandbox at a specific version. However, `getAgentType()` returns an empty string when neither `ID` nor `Type` is set on `AgentSandboxConfig`, and `isSupportedSandboxType("")` returns `false`, which caused `isSandboxEnabled()` to return `false`. This silently disabled the AWF firewall and ran the agent on the host runner, breaking MCP connectivity (the "smoke-gemini" incident). `applySandboxDefaults` only defaulted the agent type to AWF for a `nil` agent config, not for a non-nil agent config with an empty type. + +### Decision + +We will default `sandbox.agent.type` to `awf` whenever `sandbox.agent` is a non-nil, non-disabled object that carries no explicit `id` or `type`. This is a fail-safe default: a user who writes any `sandbox.agent` block clearly intends to use a sandbox, so we treat an absent `id`/`type` as an implicit AWF selection rather than as disabled. Additionally, we will reject `sandbox.agent` configurations without an explicit `id` in strict mode, where ambiguous configurations are not acceptable and must fail loudly rather than silently defaulting. + +### Alternatives Considered + +#### Alternative 1: Fail-Closed — Reject Ambiguous Configurations in All Modes + +Require `id: awf` explicitly in every `sandbox.agent` block, in both normal and strict mode. Return a validation error for any agent object with no `id`/`type`. + +This was not chosen for non-strict mode because it would break existing workflow files that pin a version without an explicit id — a common and reasonable pattern. The intent is unambiguous enough (user provided an agent block) to justify defaulting. Failing closed in non-strict mode would be a breaking change with no security benefit, since the only ambiguity is *which* sandbox type, not whether a sandbox should run. + +#### Alternative 2: Document the Behavior and Do Nothing + +Accept the existing behavior where a version-only agent config disables the sandbox, and document this as a known footgun. Users would need to always write `id: awf` explicitly to opt into the sandbox. + +This was not chosen because the prior behavior (silently disabling the firewall) is a security regression. Any `sandbox.agent` object signals clear intent to run in a sandbox; treating it as disabled is surprising and unsafe. The cost of changing the behavior is minimal while the security benefit (always-on firewall when configured) is high. + +### Consequences + +#### Positive +- Workflows that pin an AWF version without an explicit `id` now correctly run inside the sandbox, closing the silent firewall-bypass footgun. +- Strict mode now provides a clear, actionable error when `sandbox.agent` lacks an explicit `id`, preventing ambiguous configurations from reaching production. +- The default behavior is "more sandboxed" rather than "less sandboxed", which is the safer default for a security boundary. + +#### Negative +- Any workflow that intentionally relied on the empty-type behavior to bypass the sandbox (i.e., accidentally "using" the bug as a feature) will now have the sandbox re-enabled. Such workflows must add `disabled: true` explicitly if they truly intend to skip the sandbox. +- Strict mode is now more restrictive for `sandbox.agent` blocks: workflows that previously passed strict validation with a version-only agent must be updated to add `id: awf`. + +#### Neutral +- The `hourly-ci-cleaner.md` workflow required a one-line fix (`id: awf`) to conform to strict mode after this change was introduced. +- `applySandboxDefaults` now has an additional code path; the behavior is guarded by `isSupportedSandboxType(getAgentType(...))`, reusing existing helper functions without adding new abstractions. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Sandbox Agent Default Behavior + +1. When `sandbox.agent` is a non-nil object and `Disabled` is `false`, `applySandboxDefaults` **MUST** set `Agent.Type` to `awf` if `isSupportedSandboxType(getAgentType(agent))` returns `false`. +2. `applySandboxDefaults` **MUST NOT** override `Agent.Type` when the agent is explicitly disabled (`Disabled == true`). +3. `applySandboxDefaults` **MUST NOT** override an already-valid, explicitly-set `Agent.Type` (i.e., when `isSupportedSandboxType(getAgentType(agent))` returns `true`). +4. The AWF default **SHOULD** be applied before returning from `applySandboxDefaults` so that downstream consumers always receive a fully-resolved sandbox configuration. + +### Strict Mode Validation + +1. In strict mode, `validateStrictSandboxCustomization` **MUST** return an error if `sandbox.agent` is non-nil, `Disabled` is `false`, and `isSupportedSandboxType(getAgentType(agent))` returns `false`. +2. The error message **MUST** instruct the user to add `id: awf` explicitly to their `sandbox.agent` configuration. +3. A `sandbox.agent` block with `Disabled: true` **MUST NOT** trigger this strict-mode validation error; the disabled-agent case is handled by `validateStrictFirewall`. +4. Workflow files that use the AWF sandbox in strict mode **MUST** include an explicit `id: awf` field in their `sandbox.agent` block. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25240361053) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*