Fix gvisor+privileged sandbox codemod gap and add missing tools.bash codemod for min-integrity: none

pkg/cli/codemod_min_integrity_none_bash.go

@@ -0,0 +1,88 @@
+package cli
+
+import (
+	"strings"
+
+	"github.com/github/gh-aw/pkg/logger"
+)
+
+var minIntegrityNoneBashCodemodLog = logger.New("cli:codemod_min_integrity_none_bash")
+
+// getMinIntegrityNoneRequiresBashCodemod creates a codemod that adds an explicit
+// 'tools.bash: false' when 'tools.github.min-integrity' is set to 'none' and
+// 'tools.bash' is not already specified.
+//
+// Strict mode requires bash access to be explicit whenever min-integrity is none, since
+// any external user can trigger the workflow. No bash tool was configured before, so
+// inserting 'bash: false' preserves the existing behavior while satisfying the new
+// strict-mode requirement.
+func getMinIntegrityNoneRequiresBashCodemod() Codemod {
+	return Codemod{
+		ID:           "min-integrity-none-requires-bash",
+		Name:         "Add explicit 'tools.bash: false' when 'tools.github.min-integrity' is 'none'",
+		Description:  "Inserts 'tools.bash: false' when 'tools.github.min-integrity' is set to 'none' and 'tools.bash' is not already specified, preserving current behavior while satisfying strict mode",
+		IntroducedIn: "1.5.0",
+		Apply: func(content string, frontmatter map[string]any) (string, bool, error) {
+			toolsMap, ok := frontmatter["tools"].(map[string]any)
+			if !ok {
+				return content, false, nil
+			}
+
+			if _, hasBash := toolsMap["bash"]; hasBash {
+				return content, false, nil
+			}
+
+			githubMap, ok := toolsMap["github"].(map[string]any)
+			if !ok {
+				return content, false, nil
+			}
+
+			minIntegrity, ok := githubMap["min-integrity"].(string)
+			if !ok || minIntegrity != "none" {
+				return content, false, nil
+			}
+
+			newContent, applied, err := applyFrontmatterLineTransform(content, insertBashFalseIntoTopLevelTools)
+			if applied {
+				minIntegrityNoneBashCodemodLog.Print("Inserted 'tools.bash: false' because tools.github.min-integrity is 'none'")
+			}
+			return newContent, applied, err
+		},
+	}
+}
+
+// insertBashFalseIntoTopLevelTools inserts 'bash: false' as the first child of the
+// top-level 'tools:' block. It assumes the caller has already verified that 'tools'
+// exists as a block mapping and that 'tools.bash' is not already present.
+func insertBashFalseIntoTopLevelTools(lines []string) ([]string, bool) {
+	toolsLine := -1
+	for i, line := range lines {
+		if isTopLevelBlockKey(line, "tools") {
+			toolsLine = i
+			break
+		}
+	}
+	if toolsLine == -1 {
+		return lines, false
+	}
+
+	fieldIndent := "  "
+	insertAt := toolsLine + 1
+
+	for i := toolsLine + 1; i < len(lines); i++ {
+		line := lines[i]
+		trimmed := strings.TrimSpace(line)
+		if trimmed == "" || strings.HasPrefix(trimmed, "#") {
+			continue
+		}
+		if hasExitedBlock(line, "") {
+			break
+		}
+		fieldIndent = getIndentation(line)
+		insertAt = i
+		break
+	}
+
+	result := insertLine(lines, insertAt, fieldIndent+"bash: false")
+	return result, true
+}

pkg/cli/codemod_min_integrity_none_bash_test.go

@@ -0,0 +1,133 @@
+//go:build !integration
+
+package cli
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMinIntegrityNoneRequiresBashCodemod(t *testing.T) {
+	t.Parallel()
+	codemod := getMinIntegrityNoneRequiresBashCodemod()
+
+	assert.Equal(t, "min-integrity-none-requires-bash", codemod.ID)
+	assert.NotEmpty(t, codemod.Name)
+	assert.NotEmpty(t, codemod.Description)
+	assert.NotEmpty(t, codemod.IntroducedIn)
+	require.NotNil(t, codemod.Apply)
+
+	t.Run("inserts bash: false when min-integrity is none and bash is absent", func(t *testing.T) {
+		t.Parallel()
+		content := `---
+on: workflow_dispatch
+tools:
+  github:
+    min-integrity: none
+---
+
+# Test
+`
+		frontmatter := map[string]any{
+			"tools": map[string]any{
+				"github": map[string]any{"min-integrity": "none"},
+			},
+		}
+
+		result, applied, err := codemod.Apply(content, frontmatter)
+		require.NoError(t, err)
+		assert.True(t, applied)
+		assert.Contains(t, result, "  bash: false")
+		assert.Contains(t, result, "  bash: false\n  github:")
+	})
+
+	t.Run("does nothing when bash is already specified", func(t *testing.T) {
+		t.Parallel()
+		content := `---
+on: workflow_dispatch
+tools:
+  bash: ["cat", "ls"]
+  github:
+    min-integrity: none
+---
+
+# Test
+`
+		frontmatter := map[string]any{
+			"tools": map[string]any{
+				"bash":   []any{"cat", "ls"},
+				"github": map[string]any{"min-integrity": "none"},
+			},
+		}
+
+		result, applied, err := codemod.Apply(content, frontmatter)
+		require.NoError(t, err)
+		assert.False(t, applied)
+		assert.Equal(t, content, result)
+	})
+
+	t.Run("does nothing when min-integrity is not none", func(t *testing.T) {
+		t.Parallel()
+		content := `---
+on: workflow_dispatch
+tools:
+  github:
+    min-integrity: approved
+---
+
+# Test
+`
+		frontmatter := map[string]any{
+			"tools": map[string]any{
+				"github": map[string]any{"min-integrity": "approved"},
+			},
+		}
+
+		result, applied, err := codemod.Apply(content, frontmatter)
+		require.NoError(t, err)
+		assert.False(t, applied)
+		assert.Equal(t, content, result)
+	})
+
+	t.Run("does nothing when tools.github is absent", func(t *testing.T) {
+		t.Parallel()
+		content := `---
+on: workflow_dispatch
+engine: copilot
+---
+
+# Test
+`
+		frontmatter := map[string]any{"engine": "copilot"}
+
+		result, applied, err := codemod.Apply(content, frontmatter)
+		require.NoError(t, err)
+		assert.False(t, applied)
+		assert.Equal(t, content, result)
+	})
+
+	t.Run("does nothing when min-integrity is absent", func(t *testing.T) {
+		t.Parallel()
+		content := `---
+on: workflow_dispatch
+tools:
+  github:
+    allowed-repos: all
+---
+
+# Test
+`
+		frontmatter := map[string]any{
+			"tools": map[string]any{
+				"github": map[string]any{"allowed-repos": "all"},
+			},
+		}
+
+		result, applied, err := codemod.Apply(content, frontmatter)
+		require.NoError(t, err)
+		assert.False(t, applied)
+		assert.Equal(t, content, result)
+	})
+}

pkg/cli/codemod_sandbox_runtime_profile.go

@@ -14,6 +14,7 @@ const (
 	sandboxRuntimeDockerSudoIptables = "docker-sudo-iptables"
 	sandboxRuntimeDockerSbx          = "docker-sbx"
 	sandboxRuntimeCloudHypervisor    = "cloud-hypervisor"
+	sandboxRuntimeGvisor             = "gvisor"
 )
 
 // getSandboxRuntimeProfileCodemod creates a codemod that migrates the removed
@@ -24,10 +25,13 @@ const (
 //	legacy-security: enable           -> runtime: docker-sudo-iptables
 //	runtime: docker-sbx + sudo: true  -> runtime: docker-sbx
 //	sudo: true (no other runtime)     -> runtime: docker-sudo-iptables
+//	runtime: gvisor + sudo/legacy     -> runtime: gvisor (sudo/legacy-security are dropped)
 //
-// Mixed profiles that cannot be migrated unambiguously (for example gVisor combined
-// with legacy security) return an actionable error so the author can choose between
-// strict isolation and the privileged iptables profile.
+// gVisor combined with privileged security options keeps the strict 'runtime: gvisor'
+// isolation and simply drops the no-longer-supported 'sudo'/'legacy-security' fields,
+// since gVisor's network isolation already takes precedence over the privileged intent.
+// Other mixed profiles that cannot be migrated unambiguously return an actionable error
+// so the author can choose between strict isolation and the privileged iptables profile.
 func getSandboxRuntimeProfileCodemod() Codemod {
 	return Codemod{
 		ID:           "sandbox-runtime-profiles",
@@ -59,7 +63,7 @@ func getSandboxRuntimeProfileCodemod() Codemod {
 			}
 
 			newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) {
-				result, modified := migrateSandboxAgentSecurityLines(lines, targetRuntime, runtime != "")
+				result, modified := migrateSandboxAgentSecurityLines(lines, runtime, targetRuntime)
 				if modified {
 					// Dropping the only key under sandbox.agent leaves a dangling
 					// "agent:" (and possibly "sandbox:") key that YAML parses as null.
@@ -97,6 +101,16 @@ func resolveMigratedSandboxRuntime(runtime string, sudoEnabled, legacyEnabled bo
 			return "", mixedSandboxProfileError(runtime)
 		}
 		return "", nil
+	case sandboxRuntimeGvisor:
+		// gVisor combined with privileged security options is no longer a supported
+		// combination. gVisor's strict network isolation takes precedence, so keep
+		// 'runtime: gvisor' and drop the 'sudo'/'legacy-security' fields instead of
+		// aborting the fix pass so `gh aw fix --write` can still repair the file.
+		sandboxRuntimeProfileCodemodLog.Printf(
+			"sandbox.agent.runtime: gvisor combined with privileged security options is not supported; keeping %q and dropping sudo/legacy-security",
+			sandboxRuntimeGvisor,
+		)
+		return sandboxRuntimeGvisor, nil
 	default:
 		return "", mixedSandboxProfileError(runtime)
 	}
@@ -114,14 +128,18 @@ func mixedSandboxProfileError(runtime string) error {
 // migrateSandboxAgentSecurityLines removes the sudo and legacy-security keys from the
 // sandbox.agent block. When targetRuntime is non-empty and the block has no runtime
 // key yet, the first removed key is replaced by the runtime key so the profile is
-// preserved in place.
-func migrateSandboxAgentSecurityLines(lines []string, targetRuntime string, hasRuntime bool) ([]string, bool) {
+// preserved in place. When the block already has a runtime key but its value differs
+// from targetRuntime (for example gVisor migrating to the privileged iptables profile),
+// the existing runtime line's value is rewritten in place.
+func migrateSandboxAgentSecurityLines(lines []string, oldRuntime, targetRuntime string) ([]string, bool) {
 	start, end, indent, found := findSandboxAgentBlock(lines)
 	if !found {
 		return lines, false
 	}
 
+	hasRuntime := oldRuntime != ""
 	needsRuntime := targetRuntime != "" && !hasRuntime
+	needsRuntimeUpdate := targetRuntime != "" && hasRuntime && targetRuntime != oldRuntime
 	result := make([]string, 0, len(lines))
 	modified := false
 
@@ -131,6 +149,11 @@ func migrateSandboxAgentSecurityLines(lines []string, targetRuntime string, hasR
 			continue
 		}
 		trimmed := strings.TrimSpace(line)
+		if needsRuntimeUpdate && getIndentation(line) == indent && strings.HasPrefix(trimmed, "runtime:") {
+			result = append(result, indent+"runtime: "+targetRuntime)
+			modified = true
+			continue
+		}
 		if getIndentation(line) != indent ||
 			(!strings.HasPrefix(trimmed, "sudo:") && !strings.HasPrefix(trimmed, "legacy-security:")) {
 			result = append(result, line)

pkg/cli/codemod_sandbox_runtime_profile_test.go

@@ -144,7 +144,7 @@ engine: copilot
 			expectApplied: false,
 		},
 		{
-			name: "gvisor combined with legacy-security is an actionable error",
+			name: "gvisor combined with legacy-security keeps gvisor and drops legacy-security",
 			content: `---
 on: workflow_dispatch
 sandbox:
@@ -159,10 +159,12 @@ sandbox:
 					"agent": map[string]any{"runtime": "gvisor", "legacy-security": "enable"},
 				},
 			},
-			expectErrSubstr: "docker-sudo-iptables",
+			expectApplied:  true,
+			expectContains: []string{"    runtime: gvisor"},
+			expectExcludes: []string{"legacy-security:", "docker-sudo-iptables"},
 		},
 		{
-			name: "gvisor combined with sudo: true is an actionable error",
+			name: "gvisor combined with sudo: true keeps gvisor and drops sudo",
 			content: `---
 on: workflow_dispatch
 sandbox:
@@ -177,7 +179,9 @@ sandbox:
 					"agent": map[string]any{"runtime": "gvisor", "sudo": true},
 				},
 			},
-			expectErrSubstr: "gvisor",
+			expectApplied:  true,
+			expectContains: []string{"    runtime: gvisor"},
+			expectExcludes: []string{"sudo:", "docker-sudo-iptables"},
 		},
 	}
 

pkg/cli/fix_codemods.go

@@ -120,6 +120,7 @@ func GetAllCodemods() []Codemod {
 		getMentionsAllowTeamMembersCodemod(),                       // Rename allow-team-members to allowed-collaborators in safe-outputs.mentions
 		getEngineCopilotSDKDriverToDriverCodemod(),                 // Rename deprecated engine.copilot-sdk-driver to engine.driver
 		getEngineModelToTopLevelCodemod(),                          // Move engine.model to top-level model
+		getMinIntegrityNoneRequiresBashCodemod(),                   // Add tools.bash: false when tools.github.min-integrity is 'none'
 	}
 	fixCodemodsLog.Printf("Loaded codemod registry: %d codemods available", len(codemods))
 	return codemods

pkg/cli/fix_codemods_test.go

@@ -257,5 +257,6 @@ func expectedCodemodOrder() []string {
 		"mentions-allow-team-members-to-allowed-collaborators",
 		"engine-copilot-sdk-driver-to-driver",
 		"engine-model-to-top-level",
+		"min-integrity-none-requires-bash",
 	}
 }