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

docs/adr/53964-prefer-restrictive-safe-default-in-ambiguous-codemod-cases.md

@@ -0,0 +1,72 @@
+# ADR-53964: Prefer the Restrictive Safe Default When Codemods Encounter Ambiguous Security Configurations
+
+**Date**: 2026-08-19
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+### Context
+
+`gh aw fix --write` applies a sequence of registered codemods to repair workflow files that fail strict-mode compilation. Two gaps caused the fix pass to leave files unrepaired:
+
+1. The `sandbox-runtime-profiles` codemod hard-errored (and aborted the entire file's fix pass) when it encountered `sandbox.agent.runtime: gvisor` combined with `sudo: true` or `legacy-security: enable`. This combination is no longer supported, but gVisor and privileged options have conflicting intent — gVisor enforces strict network isolation while `sudo`/`legacy-security` request elevated host access.
+
+2. No codemod existed for the strict-mode requirement that `tools.bash` must be explicitly specified when `tools.github.min-integrity: none`. Files with this configuration reported "No fixes needed" from `gh aw fix --write` yet still failed `--strict` compilation, silently blocking cross-repo audits.
+
+Both gaps were reproduced across multiple independently-verified external repositories (e.g. `github/gh-aw-firewall`, `github/gh-aw-mcpg`, `chrizbo/agentics-beyond-code`) during the daily compilation audit.
+
+### Decision
+
+We will resolve ambiguous security configurations by **choosing the more restrictive safe default and auto-applying the fix** rather than aborting or requiring manual intervention:
+
+- For `runtime: gvisor` combined with `sudo`/`legacy-security`: keep `runtime: gvisor` (the stricter isolation) and drop the incompatible privileged fields. This lets `gh aw fix --write` complete the file instead of aborting.
+- For `min-integrity: none` without explicit `tools.bash`: insert `tools.bash: false`. This preserves the pre-existing behavior (bash was never configured) while satisfying the strict-mode requirement.
+
+The guiding principle is that when a configuration is ambiguous, the codemod should not block the fix pass — it should apply the change that is safest and most likely correct, and log what it did so the author can review.
+
+### Alternatives Considered
+
+#### Alternative 1: Migrate gVisor + privileged to `docker-sudo-iptables`
+
+Rewrite `runtime: gvisor` to `runtime: docker-sudo-iptables` when privileged options are present, on the grounds that the author's intent was privileged access and gVisor was incidental.
+
+Not chosen because gVisor is an explicit runtime choice that signals a deliberate preference for strict network isolation. Silently downgrading isolation to satisfy a `sudo` flag would be a security regression and harder to review. Dropping the privileged fields is the smaller, more auditable change.
+
+#### Alternative 2: Keep aborting with an actionable error (previous behavior for gVisor)
+
+Continue returning an error that names the two choices and requires the author to resolve manually.
+
+Not chosen because this leaves the file completely untouched by `gh aw fix --write` — every other codemod that would have applied to the same file is also skipped. The actionable error approach scales poorly when the same pattern appears across many external repos during automated audits.
+
+#### Alternative 3: No codemod for `min-integrity: none` + missing `tools.bash`; require manual fix
+
+Keep the existing behavior where `gh aw fix --write` reports "No fixes needed" and let authors add `tools.bash` themselves.
+
+Not chosen because `tools.bash: false` is a safe, behavior-preserving default (bash was not configured before) and the strict-mode requirement is mechanical. Requiring manual action for a deterministic, zero-ambiguity fix creates unnecessary friction at scale.
+
+#### Alternative 4: Insert `tools.bash: true` instead of `false` for the `min-integrity: none` codemod
+
+Explicitly allow bash when min-integrity is none, arguing that the workflow might need shell access.
+
+Not chosen because this changes behavior (enabling a tool that was previously absent) and could introduce unintended capabilities. `false` is the conservative, behavior-preserving choice.
+
+### Consequences
+
+#### Positive
+- `gh aw fix --write` can now fully auto-repair all files affected by these two patterns without any manual intervention.
+- gVisor's strict network isolation is preserved wherever it was already explicitly configured, avoiding unintended security downgrades.
+- `tools.bash: false` satisfies the strict-mode compile requirement without changing runtime behavior for workflows that never relied on bash access.
+- The fix pass no longer aborts an entire file when one codemod encounters an ambiguous case, allowing other codemods in the same file to run.
+
+#### Negative
+- Authors who had both `runtime: gvisor` and `sudo: true` with a genuine intent for privileged host access will have `sudo` silently dropped. The fix log records this, but the author must actively check it to notice.
+- Auto-insertion of `tools.bash: false` is invisible to the author unless they diff the fixed file. Workflows that intended to add bash access later will need to update the field explicitly.
+
+#### Neutral
+- The `migrateSandboxAgentSecurityLines` function signature changed (added `oldRuntime` parameter, changed `hasRuntime bool` to a derived local variable) to support in-place rewriting of existing `runtime:` values. This is an internal refactor with no external API surface.
+- Both codemods are registered in the standard codemod registry and covered by unit tests, following the existing extension pattern.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/codemod_cli_proxy_bash.go

@@ -96,6 +96,13 @@ func setShellBackedModesDisabledInTools(lines []string, setCLIProxyFalse, setGit
 	}
 	if toolsLine == -1 {
 		if hasTopLevelKey(lines, "tools") {
+			// A single-line inline mapping can still take an explicit 'cli-proxy: false';
+			// rewriting a nested inline 'github' mapping is not attempted.
+			if setCLIProxyFalse && !setGitHubLocal {
+				if result, inserted := insertEntryIntoInlineMapping(lines, "tools", "cli-proxy: false"); inserted {
+					return result, true
+				}
+			}
 			cliProxyBashCodemodLog.Print("Top-level tools key is not block syntax, skipping")
 			return lines, false
 		}
@@ -227,6 +234,53 @@ func isTopLevelBlockKey(line, key string) bool {
 	return getIndentation(line) == "" && isBlockKey(line, key)
 }
 
+// insertEntryIntoInlineMapping inserts entry as the first item of a top-level inline flow
+// mapping (for example "tools: {github: {min-integrity: none}}"). It only rewrites flow
+// mappings that open and close on a single line, and reports false when the key is absent,
+// is not an inline mapping, or spans multiple lines.
+func insertEntryIntoInlineMapping(lines []string, key, entry string) ([]string, bool) {
+	prefix := key + ":"
+	for i, line := range lines {
+		if getIndentation(line) != "" {
+			continue
+		}
+		rest, ok := strings.CutPrefix(strings.TrimSpace(line), prefix)
+		if !ok {
+			continue
+		}
+		if !strings.HasPrefix(strings.TrimSpace(rest), "{") {
+			return lines, false
+		}
+		braceIndex := strings.Index(line, "{")
+		if !hasBalancedBraces(line[braceIndex:]) {
+			return lines, false
+		}
+		inner := strings.TrimLeft(line[braceIndex+1:], " ")
+		separator := ", "
+		if strings.HasPrefix(inner, "}") {
+			separator = ""
+		}
+		result := append([]string{}, lines...)
+		result[i] = line[:braceIndex+1] + entry + separator + inner
+		return result, true
+	}
+	return lines, false
+}
+
+// hasBalancedBraces reports whether every '{' in value is closed within value.
+func hasBalancedBraces(value string) bool {
+	depth := 0
+	for _, r := range value {
+		switch r {
+		case '{':
+			depth++
+		case '}':
+			depth--
+		}
+	}
+	return depth == 0
+}
+
 func hasTopLevelKey(lines []string, key string) bool {
 	prefix := key + ":"
 	for _, line := range lines {

pkg/cli/codemod_cli_proxy_bash_test.go

@@ -135,12 +135,32 @@ tools: # security settings
 		assert.Contains(t, result, "  cli-proxy: false")
 	})
 
-	t.Run("does not treat flow tools value as block header", func(t *testing.T) {
+	t.Run("adds cli-proxy: false to an inline flow tools mapping", func(t *testing.T) {
 		t.Parallel()
 		content := `---
 tools: {bash: false}
 ---
 
+# Test
+`
+		frontmatter := map[string]any{
+			"tools": map[string]any{"bash": false},
+		}
+
+		result, applied, err := codemod.Apply(content, frontmatter)
+		require.NoError(t, err)
+		assert.True(t, applied)
+		assert.Contains(t, result, "tools: {cli-proxy: false, bash: false}")
+	})
+
+	t.Run("does not treat a multi-line flow tools value as block header", func(t *testing.T) {
+		t.Parallel()
+		content := `---
+tools: {
+  bash: false
+}
+---
+
 # Test
 `
 		frontmatter := map[string]any{

pkg/cli/codemod_min_integrity_none_bash.go

@@ -0,0 +1,89 @@
+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, supporting both block mappings and inline flow mappings.
+// It assumes the caller has already verified that 'tools' exists as a 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 insertEntryIntoInlineMapping(lines, "tools", "bash: 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
+}