diff --git a/.changeset/fix-sandbox-gvisor-privileged-and-min-integrity-bash-codemods.md b/.changeset/fix-sandbox-gvisor-privileged-and-min-integrity-bash-codemods.md new file mode 100644 index 00000000000..54cbdfd0951 --- /dev/null +++ b/.changeset/fix-sandbox-gvisor-privileged-and-min-integrity-bash-codemods.md @@ -0,0 +1,8 @@ +--- +"gh-aw": patch +--- + +Fix two `gh aw fix --write` codemod gaps that left `--strict` compile failures unrepaired: + +- `sandbox-runtime-profiles` no longer hard-errors when `sandbox.agent.runtime: gvisor` is combined with privileged security options (`sudo`/`legacy-security`). It now keeps `runtime: gvisor` and drops the no-longer-supported `sudo`/`legacy-security` fields, instead of leaving the file untouched. +- Added a new codemod that inserts `tools.bash: false` when `tools.github.min-integrity` is set to `none` and `tools.bash` is not already specified, satisfying the strict-mode requirement that shell access be explicit. It runs before the `cli-proxy-false-when-bash-disabled` codemod so a single fix pass also emits the required `tools.cli-proxy: false`, and it supports single-line inline `tools: {github: {min-integrity: none}}` mappings. diff --git a/docs/adr/53964-prefer-restrictive-safe-default-in-ambiguous-codemod-cases.md b/docs/adr/53964-prefer-restrictive-safe-default-in-ambiguous-codemod-cases.md new file mode 100644 index 00000000000..6713ccda5a9 --- /dev/null +++ b/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.* diff --git a/pkg/cli/codemod_cli_proxy_bash.go b/pkg/cli/codemod_cli_proxy_bash.go index 326df61d10f..1521074cbcd 100644 --- a/pkg/cli/codemod_cli_proxy_bash.go +++ b/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 { diff --git a/pkg/cli/codemod_cli_proxy_bash_test.go b/pkg/cli/codemod_cli_proxy_bash_test.go index eb92972bc24..630e6ab3a4d 100644 --- a/pkg/cli/codemod_cli_proxy_bash_test.go +++ b/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{ diff --git a/pkg/cli/codemod_min_integrity_none_bash.go b/pkg/cli/codemod_min_integrity_none_bash.go new file mode 100644 index 00000000000..730d6a66bb7 --- /dev/null +++ b/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 +} diff --git a/pkg/cli/codemod_min_integrity_none_bash_test.go b/pkg/cli/codemod_min_integrity_none_bash_test.go new file mode 100644 index 00000000000..90fb743030e --- /dev/null +++ b/pkg/cli/codemod_min_integrity_none_bash_test.go @@ -0,0 +1,325 @@ +//go:build !integration + +package cli + +import ( + "os" + "path/filepath" + "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) + }) +} + +func TestMinIntegrityNoneRequiresBashCodemod_InlineToolsMapping(t *testing.T) { + t.Parallel() + codemod := getMinIntegrityNoneRequiresBashCodemod() + + t.Run("inserts bash: false into an inline tools mapping", 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, "tools: {bash: false, github: {min-integrity: none}}") + }) + + t.Run("preserves spacing of an inline tools mapping", 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, "tools: {bash: false, github: { min-integrity: none } }") + }) + + t.Run("skips multi-line inline tools mappings", 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.False(t, applied) + assert.Equal(t, content, result) + }) +} + +// TestMinIntegrityNoneRequiresBash_SingleFixPassAlsoDisablesCLIProxy verifies that the +// registry order lets one fix pass emit both 'bash: false' and the 'cli-proxy: false' +// that strict mode requires once bash is disabled. +func TestMinIntegrityNoneRequiresBash_SingleFixPassAlsoDisablesCLIProxy(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + content string + }{ + { + name: "block mapping", + content: `--- +on: workflow_dispatch +tools: + github: + min-integrity: none +--- + +# Test +`, + }, + { + name: "inline mapping", + content: `--- +on: workflow_dispatch +tools: {github: {min-integrity: none}} +--- + +# Test +`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + tmpDir := t.TempDir() + workflowFile := filepath.Join(tmpDir, "test-workflow.md") + require.NoError(t, os.WriteFile(workflowFile, []byte(tc.content), 0644)) + + fixed, _, err := processWorkflowFileWithInfo(workflowFile, GetAllCodemods(), true, false) + require.NoError(t, err) + require.True(t, fixed) + + result, err := os.ReadFile(workflowFile) + require.NoError(t, err) + assert.Contains(t, string(result), "bash: false") + assert.Contains(t, string(result), "cli-proxy: false") + }) + } +} + +func TestInsertBashFalseIntoTopLevelTools_SkipsCommentsAndBlankLines(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + lines []string + expected []string + }{ + { + name: "inserts after leading comments and blank lines, matching field indentation", + lines: []string{ + "on: workflow_dispatch", + "tools:", + " # keep the github toolset minimal", + "", + " github:", + " min-integrity: none", + }, + expected: []string{ + "on: workflow_dispatch", + "tools:", + " # keep the github toolset minimal", + "", + " bash: false", + " github:", + " min-integrity: none", + }, + }, + { + name: "uses the first field indentation even when it is not two spaces", + lines: []string{ + "tools:", + " # comment", + " github:", + " min-integrity: none", + }, + expected: []string{ + "tools:", + " # comment", + " bash: false", + " github:", + " min-integrity: none", + }, + }, + { + name: "inserts right after the tools key when the block has only comments", + lines: []string{ + "tools:", + " # nothing configured yet", + "engine: copilot", + }, + expected: []string{ + "tools:", + " bash: false", + " # nothing configured yet", + "engine: copilot", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result, applied := insertBashFalseIntoTopLevelTools(tt.lines) + assert.True(t, applied) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/pkg/cli/codemod_sandbox_runtime_profile.go b/pkg/cli/codemod_sandbox_runtime_profile.go index 137602f1f10..84ddb51e9f8 100644 --- a/pkg/cli/codemod_sandbox_runtime_profile.go +++ b/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+trailingCommentSuffix(strings.TrimPrefix(trimmed, "runtime:"))) + modified = true + continue + } if getIndentation(line) != indent || (!strings.HasPrefix(trimmed, "sudo:") && !strings.HasPrefix(trimmed, "legacy-security:")) { result = append(result, line) @@ -146,6 +169,17 @@ func migrateSandboxAgentSecurityLines(lines []string, targetRuntime string, hasR return result, modified } +// trailingCommentSuffix returns the trailing YAML comment of a value part, prefixed by a +// space so it can be appended to a rewritten key line, or an empty string when the value +// has no trailing comment. +func trailingCommentSuffix(valuePart string) string { + idx := findTrailingCommentIndex(valuePart) + if idx < 0 { + return "" + } + return " " + strings.TrimRight(valuePart[idx:], " \t") +} + // findSandboxAgentBlock locates the child lines of the sandbox.agent mapping. // It returns the half-open line range of the block's children and the indentation // shared by those children. diff --git a/pkg/cli/codemod_sandbox_runtime_profile_test.go b/pkg/cli/codemod_sandbox_runtime_profile_test.go index d92b76e6108..e4c4d3e92c0 100644 --- a/pkg/cli/codemod_sandbox_runtime_profile_test.go +++ b/pkg/cli/codemod_sandbox_runtime_profile_test.go @@ -132,6 +132,26 @@ sandbox: expectApplied: true, expectExcludes: []string{"sudo:"}, }, + { + name: "rewritten runtime line keeps its trailing comment", + content: `--- +on: workflow_dispatch +sandbox: + agent: + runtime: docker # keep this note + sudo: true +--- + +# Test`, + frontmatter: map[string]any{ + "sandbox": map[string]any{ + "agent": map[string]any{"runtime": "docker", "sudo": true}, + }, + }, + expectApplied: true, + expectContains: []string{" runtime: docker-sudo-iptables # keep this note"}, + expectExcludes: []string{"sudo:"}, + }, { name: "workflow without sandbox.agent is untouched", content: `--- @@ -144,7 +164,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 +179,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 +199,30 @@ sandbox: "agent": map[string]any{"runtime": "gvisor", "sudo": true}, }, }, - expectErrSubstr: "gvisor", + expectApplied: true, + expectContains: []string{" runtime: gvisor"}, + expectExcludes: []string{"sudo:", "docker-sudo-iptables"}, + }, + { + name: "gvisor combined with both sudo and legacy-security keeps gvisor and drops both", + content: `--- +on: workflow_dispatch +sandbox: + agent: + runtime: gvisor + sudo: true + legacy-security: enable +--- + +# Test`, + frontmatter: map[string]any{ + "sandbox": map[string]any{ + "agent": map[string]any{"runtime": "gvisor", "sudo": true, "legacy-security": "enable"}, + }, + }, + expectApplied: true, + expectContains: []string{" runtime: gvisor"}, + expectExcludes: []string{"sudo:", "legacy-security:", "docker-sudo-iptables"}, }, } diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index b16c50a60b1..828f26f4448 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -110,6 +110,7 @@ func GetAllCodemods() []Codemod { getCliProxyFeatureToGitHubModeCodemod(), // Migrate features.cli-proxy: true to tools.github.mode: gh-proxy getDIFCProxyToIntegrityProxyCodemod(), // Migrate deprecated features.difc-proxy to tools.github.integrity-proxy getMountAsCLIsToCLIProxyCodemod(), // Rename tools.mount-as-clis to tools.cli-proxy and remove features.mcp-cli + getMinIntegrityNoneRequiresBashCodemod(), // Add tools.bash: false when tools.github.min-integrity is 'none' getCLIProxyBashDisabledCodemod(), // Set tools.cli-proxy: false when tools.bash is disabled getSandboxMCPContainerRemovalCodemod(), // Remove deprecated sandbox.mcp.container (now managed internally) getSandboxMCPVersionRemovalCodemod(), // Remove deprecated sandbox.mcp.version (now managed internally) diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 7d8a67d4287..56cc5e79857 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -255,6 +255,7 @@ func expectedCodemodOrder() []string { "features-cli-proxy-to-tools-github-mode", "features-difc-proxy-to-tools-github", "mount-as-clis-to-cli-proxy", + "min-integrity-none-requires-bash", "cli-proxy-false-when-bash-disabled", "sandbox-mcp-container-removal", "sandbox-mcp-version-removal",