fix: correct engine.env secrets warning — excluded from sandbox, not leaked

docs/adr/43302-section-aware-secrets-validation-messages.md

@@ -0,0 +1,73 @@
+# ADR-43302: Section-Aware Secret Validation Messages in Workflow Compiler
+
+**Date**: 2026-07-04
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The workflow compiler's `validateEnvSecretsSection` function validates secrets across multiple
+env sections (`env`, `engine.env`) and emits warnings or errors when `${{ secrets.* }}`
+references are detected. Historically it used a single generic message — "will be leaked to the
+agent container" — regardless of which section contained the secret reference.
+
+However, `engine.env` secrets are handled differently from top-level `env` secrets: the
+`ComputeAWFExcludeEnvVarNames` function auto-detects any `engine.env` value that contains a
+`${{ secrets.* }}` reference and adds the corresponding variable name to AWF's `--exclude-env`
+list. This means `engine.env` secrets never reach the agent sandbox process, making the
+"will be leaked" warning factually incorrect for that section and a source of user confusion.
+
+### Decision
+
+We will differentiate the validation messages emitted by `validateEnvSecretsSection` based on
+which section is being validated, rather than using a single generic message for all sections.
+For `engine.env`, messages will accurately reflect that secrets are auto-excluded from the
+agent sandbox via AWF `--exclude-env`; for all other sections (e.g., top-level `env`), the
+existing "will be leaked" language is retained because those secrets genuinely reach the
+agent container. Strict mode continues to treat `engine.env` secrets as an error (encouraging
+engine-specific secret configuration), but with accurate, non-misleading error text.
+
+### Alternatives Considered
+
+#### Alternative 1: Remove the `engine.env` warning entirely
+
+Since `engine.env` secrets are auto-excluded and never reach the agent process, one option is
+to suppress the warning entirely for that section — no message, no error. This eliminates the
+false positive and simplifies the code path. It was rejected because users should still be
+nudged toward engine-specific secret configuration (the more secure, explicit pattern), and
+removing all feedback would silently permit a configuration style that the platform discourages.
+
+#### Alternative 2: Extract a separate validation function for `engine.env`
+
+Rather than branching inside `validateEnvSecretsSection`, a dedicated
+`validateEngineEnvSecretsSection` function could own all `engine.env` secret logic. This would
+keep each function single-purpose and avoid section-name string comparisons. It was rejected
+for this fix because it would duplicate the secret-detection regex and collection logic that
+lives in `validateEnvSecretsSection`, increasing maintenance surface for a change whose primary
+goal is correcting a misleading string — not restructuring validation logic.
+
+### Consequences
+
+#### Positive
+- Users see accurate feedback: `engine.env` secret warnings now correctly describe sandbox
+  exclusion rather than falsely claiming leakage to the agent container.
+- Strict mode error messages for `engine.env` remain actionable and no longer contradict the
+  platform's actual security behavior, reducing support burden and user confusion.
+
+#### Negative
+- `validateEnvSecretsSection` now contains section-specific branching (`if sectionName == "engine.env"`),
+  slightly increasing cyclomatic complexity and making the function less generic.
+- Any future env section added to the validation path must be evaluated for whether it also
+  requires a custom message, creating an implicit maintenance contract.
+
+#### Neutral
+- Test assertions for `engine.env` strict-mode cases were updated to match the new message
+  text; this is a mechanical change with no behavioral impact on test coverage.
+- The `awf_helpers_test.go` additions verify the existing `ComputeAWFExcludeEnvVarNames`
+  behavior that this fix depends on for correctness, improving confidence in the invariant.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/awf_helpers_test.go

@@ -1118,6 +1118,104 @@ func TestAWFSupportsExcludeEnv(t *testing.T) {
 	}
 }
 
+// TestComputeAWFExcludeEnvVarNames verifies that engine.env vars whose values contain
+// ${{ secrets.* }} are automatically included in the --exclude-env list, and that
+// non-secret engine.env vars and plain-value core secrets are handled correctly.
+func TestComputeAWFExcludeEnvVarNames(t *testing.T) {
+	tests := []struct {
+		name               string
+		workflowData       *WorkflowData
+		coreSecretVarNames []string
+		want               []string
+		notWant            []string
+	}{
+		{
+			name: "engine.env secret var is auto-excluded",
+			workflowData: &WorkflowData{
+				EngineConfig: &EngineConfig{
+					Env: map[string]string{
+						"GOOGLE_API_KEY": "${{ secrets.SOME_KEY }}",
+					},
+				},
+			},
+			coreSecretVarNames: []string{},
+			want:               []string{"GOOGLE_API_KEY"},
+		},
+		{
+			name: "engine.env non-secret var is not excluded",
+			workflowData: &WorkflowData{
+				EngineConfig: &EngineConfig{
+					Env: map[string]string{
+						"DEBUG":     "true",
+						"LOG_LEVEL": "info",
+					},
+				},
+			},
+			coreSecretVarNames: []string{},
+			want:               []string{},
+			notWant:            []string{"DEBUG", "LOG_LEVEL"},
+		},
+		{
+			name: "engine.env mixes secret and non-secret vars: only secrets excluded",
+			workflowData: &WorkflowData{
+				EngineConfig: &EngineConfig{
+					Env: map[string]string{
+						"GOOGLE_API_KEY": "${{ secrets.SOME_KEY }}",
+						"LOG_LEVEL":      "debug",
+					},
+				},
+			},
+			coreSecretVarNames: []string{},
+			want:               []string{"GOOGLE_API_KEY"},
+			notWant:            []string{"LOG_LEVEL"},
+		},
+		{
+			name: "engine.env secret combined with core secret vars",
+			workflowData: &WorkflowData{
+				EngineConfig: &EngineConfig{
+					Env: map[string]string{
+						"CUSTOM_API_KEY": "${{ secrets.CUSTOM_KEY }}",
+					},
+				},
+			},
+			coreSecretVarNames: []string{"GEMINI_API_KEY"},
+			want:               []string{"GEMINI_API_KEY", "CUSTOM_API_KEY"},
+		},
+		{
+			name: "engine.env secret embedded in a larger string is excluded",
+			workflowData: &WorkflowData{
+				EngineConfig: &EngineConfig{
+					Env: map[string]string{
+						"AUTH_HEADER": "Bearer ${{ secrets.TOKEN }}",
+					},
+				},
+			},
+			coreSecretVarNames: []string{},
+			want:               []string{"AUTH_HEADER"},
+		},
+		{
+			name: "nil engine config produces no exclusions beyond core secrets",
+			workflowData: &WorkflowData{
+				EngineConfig: nil,
+			},
+			coreSecretVarNames: []string{"COPILOT_GITHUB_TOKEN"},
+			want:               []string{"COPILOT_GITHUB_TOKEN"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ComputeAWFExcludeEnvVarNames(tt.workflowData, tt.coreSecretVarNames)
+			for _, name := range tt.want {
+				assert.Contains(t, got, name, "expected %q in exclude list", name)
+			}
+			for _, name := range tt.notWant {
+				assert.NotContains(t, got, name, "expected %q to be absent from exclude list", name)
+			}
+		})
+	}
+}
+
 // TestBuildAWFArgsCliProxy tests that BuildAWFArgs correctly injects --difc-proxy-host
 // and --difc-proxy-ca-cert based on the cli-proxy feature flag.
 func TestBuildAWFArgsCliProxy(t *testing.T) {

pkg/workflow/env_secrets_validation_test.go

@@ -3,9 +3,11 @@
 package workflow
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
+	"github.com/github/gh-aw/pkg/constants"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -537,7 +539,7 @@ func TestValidateEngineEnvSecrets(t *testing.T) {
 			},
 			strictMode:  true,
 			expectError: true,
-			errorMsg:    "strict mode: secrets detected in 'engine.env' section will be leaked to the agent container. Found: ${{ secrets.API_KEY }}",
+			errorMsg:    fmt.Sprintf("are excluded from the agent sandbox via awf --exclude-env (requires AWF %s+) and are not accessible to the agent when that version is in use. Found: ${{ secrets.API_KEY }}", constants.AWFExcludeEnvMinVersion),
 		},
 		{
 			name: "engine.env with multiple secrets in strict mode fails",
@@ -553,7 +555,7 @@ func TestValidateEngineEnvSecrets(t *testing.T) {
 			},
 			strictMode:  true,
 			expectError: true,
-			errorMsg:    "strict mode: secrets detected in 'engine.env' section will be leaked to the agent container",
+			errorMsg:    fmt.Sprintf("are excluded from the agent sandbox via awf --exclude-env (requires AWF %s+)", constants.AWFExcludeEnvMinVersion),
 		},
 		{
 			name: "engine.env with secret embedded in string in strict mode fails",
@@ -568,7 +570,7 @@ func TestValidateEngineEnvSecrets(t *testing.T) {
 			},
 			strictMode:  true,
 			expectError: true,
-			errorMsg:    "strict mode: secrets detected in 'engine.env' section will be leaked to the agent container. Found: ${{ secrets.TOKEN }}",
+			errorMsg:    fmt.Sprintf("are excluded from the agent sandbox via awf --exclude-env (requires AWF %s+) and are not accessible to the agent when that version is in use. Found: ${{ secrets.TOKEN }}", constants.AWFExcludeEnvMinVersion),
 		},
 		{
 			name: "engine.env with secret in non-strict mode emits warning (no error)",

pkg/workflow/strict_mode_env_validation.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"github.com/github/gh-aw/pkg/console"
+	"github.com/github/gh-aw/pkg/constants"
 	"github.com/github/gh-aw/pkg/setutil"
 )
 
@@ -132,11 +133,25 @@ func (c *Compiler) validateEnvSecretsSection(config map[string]any, sectionName
 
 	// In strict mode, this is an error
 	if c.strictMode {
+		if sectionName == "engine.env" {
+			// engine.env secrets are excluded from the agent sandbox via awf --exclude-env
+			// (requires AWF v0.25.3+), so they are not leaked, but strict mode still requires
+			// engine-specific configuration.
+			return fmt.Errorf("strict mode: secrets detected in 'engine.env' section are excluded from the agent sandbox via awf --exclude-env (requires AWF %s+) and are not accessible to the agent when that version is in use. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", constants.AWFExcludeEnvMinVersion, strings.Join(secretRefs, ", "))
+		}
 		return fmt.Errorf("strict mode: secrets detected in '%s' section will be leaked to the agent container. Found: %s. Use engine-specific secret configuration instead. See: https://github.github.com/gh-aw/reference/engines/", sectionName, strings.Join(secretRefs, ", "))
 	}
 
 	// In non-strict mode, emit a warning
-	warningMsg := fmt.Sprintf("Warning: secrets detected in '%s' section will be leaked to the agent container. Found: %s. Consider using engine-specific secret configuration instead.", sectionName, strings.Join(secretRefs, ", "))
+	var warningMsg string
+	if sectionName == "engine.env" {
+		// engine.env secrets are excluded from the agent sandbox via awf --exclude-env
+		// (requires AWF v0.25.3+). On older AWF versions this protection is not applied and
+		// the values will reach the agent container.
+		warningMsg = fmt.Sprintf("Warning: secrets detected in 'engine.env' section will be excluded from the agent sandbox via awf --exclude-env (requires AWF %s+); on older AWF versions the agent process will see these values. Found: %s. Consider using engine-specific secret configuration instead.", constants.AWFExcludeEnvMinVersion, strings.Join(secretRefs, ", "))
+	} else {
+		warningMsg = fmt.Sprintf("Warning: secrets detected in '%s' section will be leaked to the agent container. Found: %s. Consider using engine-specific secret configuration instead.", sectionName, strings.Join(secretRefs, ", "))
+	}
 	fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg))
 	c.IncrementWarningCount()