From 63aa67af9240c5309ca65489fc9237b53e16e92a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 17:14:58 +0000 Subject: [PATCH 1/7] Initial plan From ab7ddc160e3c635b61da8c497a30f0dfee024e14 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 18:26:16 +0000 Subject: [PATCH 2/7] feat(fix): add codemods for strict-mode secrets in steps and engine.env Agent-Logs-Url: https://github.com/github/gh-aw/sessions/41aed0dc-59cf-45eb-b96f-b353d787e84c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/codemod_engine_env_secrets.go | 206 +++++++++++++ pkg/cli/codemod_engine_env_secrets_test.go | 93 ++++++ pkg/cli/codemod_steps_run_secrets_env.go | 284 ++++++++++++++++++ pkg/cli/codemod_steps_run_secrets_env_test.go | 120 ++++++++ pkg/cli/fix_codemods.go | 2 + pkg/cli/fix_codemods_test.go | 6 +- 6 files changed, 710 insertions(+), 1 deletion(-) create mode 100644 pkg/cli/codemod_engine_env_secrets.go create mode 100644 pkg/cli/codemod_engine_env_secrets_test.go create mode 100644 pkg/cli/codemod_steps_run_secrets_env.go create mode 100644 pkg/cli/codemod_steps_run_secrets_env_test.go diff --git a/pkg/cli/codemod_engine_env_secrets.go b/pkg/cli/codemod_engine_env_secrets.go new file mode 100644 index 00000000000..49fe7edab35 --- /dev/null +++ b/pkg/cli/codemod_engine_env_secrets.go @@ -0,0 +1,206 @@ +package cli + +import ( + "strings" + + "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/workflow" +) + +var engineEnvSecretsCodemodLog = logger.New("cli:codemod_engine_env_secrets") + +// getEngineEnvSecretsCodemod creates a codemod that removes unsafe secret-bearing entries +// from engine.env while preserving allowed engine-required secret overrides. +func getEngineEnvSecretsCodemod() Codemod { + return Codemod{ + ID: "engine-env-secrets-to-engine-config", + Name: "Remove unsafe secrets from engine.env", + Description: "Removes secret-bearing engine.env entries that are not required engine secret overrides, preventing strict-mode leaks.", + IntroducedIn: "0.26.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + engineValue, hasEngine := frontmatter["engine"] + if !hasEngine { + return content, false, nil + } + + engineMap, ok := engineValue.(map[string]any) + if !ok { + return content, false, nil + } + + envAny, hasEnv := engineMap["env"] + if !hasEnv { + return content, false, nil + } + + envMap, ok := envAny.(map[string]any) + if !ok { + return content, false, nil + } + + engineID := extractEngineIDForCodemod(frontmatter, engineMap) + allowed := allowedEngineEnvSecretKeys(engineID) + unsafeKeys := findUnsafeEngineEnvSecretKeys(envMap, allowed) + if len(unsafeKeys) == 0 { + return content, false, nil + } + + newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { + updated, modified := removeUnsafeEngineEnvKeys(lines, unsafeKeys) + if !modified { + return lines, false + } + cleaned := removeEmptyEngineEnvBlock(updated) + return cleaned, true + }) + if applied { + engineEnvSecretsCodemodLog.Printf("Removed unsafe engine.env secret keys: %v", mapKeys(unsafeKeys)) + } + return newContent, applied, err + }, + } +} + +func extractEngineIDForCodemod(frontmatter map[string]any, engineMap map[string]any) string { + if id, ok := engineMap["id"].(string); ok && id != "" { + return id + } + if id, ok := frontmatter["engine"].(string); ok && id != "" { + return id + } + return "" +} + +func allowedEngineEnvSecretKeys(engineID string) map[string]bool { + allowed := make(map[string]bool) + for _, req := range getSecretRequirementsForEngine(engineID, false, false) { + allowed[req.Name] = true + } + return allowed +} + +func findUnsafeEngineEnvSecretKeys(envMap map[string]any, allowed map[string]bool) map[string]bool { + unsafe := make(map[string]bool) + for key, value := range envMap { + if allowed[key] { + continue + } + strVal, ok := value.(string) + if !ok { + continue + } + if len(workflow.ExtractSecretsFromMap(map[string]string{key: strVal})) > 0 { + unsafe[key] = true + } + } + return unsafe +} + +func removeUnsafeEngineEnvKeys(lines []string, unsafeKeys map[string]bool) ([]string, bool) { + result := make([]string, 0, len(lines)) + modified := false + + inEngine := false + engineIndent := "" + inEnv := false + envIndent := "" + removingKey := false + removingKeyIndent := "" + + for _, line := range lines { + trimmed := strings.TrimSpace(line) + indent := getIndentation(line) + + if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { + inEngine = true + engineIndent = indent + inEnv = false + removingKey = false + result = append(result, line) + continue + } + + if inEngine && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") && len(indent) <= len(engineIndent) { + inEngine = false + inEnv = false + removingKey = false + } + + if inEngine && !inEnv && strings.HasPrefix(trimmed, "env:") && strings.TrimSpace(strings.TrimPrefix(trimmed, "env:")) == "" { + inEnv = true + envIndent = indent + removingKey = false + result = append(result, line) + continue + } + + if inEnv && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") && len(indent) <= len(envIndent) { + inEnv = false + removingKey = false + } + + if inEnv && removingKey { + if len(trimmed) == 0 { + continue + } + if strings.HasPrefix(trimmed, "#") && len(indent) > len(removingKeyIndent) { + continue + } + if len(indent) > len(removingKeyIndent) { + continue + } + removingKey = false + } + + if inEnv && !removingKey && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") && len(indent) > len(envIndent) { + key := parseYAMLMapKey(trimmed) + if key != "" && unsafeKeys[key] { + modified = true + removingKey = true + removingKeyIndent = indent + continue + } + } + + result = append(result, line) + } + + return result, modified +} + +func removeEmptyEngineEnvBlock(lines []string) []string { + result := make([]string, 0, len(lines)) + for i := range lines { + line := lines[i] + trimmed := strings.TrimSpace(line) + if trimmed == "env:" { + envIndent := getIndentation(line) + hasValues := false + j := i + 1 + for ; j < len(lines); j++ { + t := strings.TrimSpace(lines[j]) + if len(t) == 0 { + continue + } + if len(getIndentation(lines[j])) <= len(envIndent) { + break + } + hasValues = true + break + } + if !hasValues { + continue + } + } + result = append(result, line) + } + return result +} + +func mapKeys(m map[string]bool) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} diff --git a/pkg/cli/codemod_engine_env_secrets_test.go b/pkg/cli/codemod_engine_env_secrets_test.go new file mode 100644 index 00000000000..d3a98731800 --- /dev/null +++ b/pkg/cli/codemod_engine_env_secrets_test.go @@ -0,0 +1,93 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEngineEnvSecretsCodemod(t *testing.T) { + codemod := getEngineEnvSecretsCodemod() + + t.Run("removes unsafe secret-bearing engine env keys and keeps allowed override", func(t *testing.T) { + content := `--- +on: workflow_dispatch +engine: + id: codex + env: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + OPENAI_BASE_URL: ${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1 +--- +` + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "engine": map[string]any{ + "id": "codex", + "env": map[string]any{ + "OPENAI_API_KEY": "${{ secrets.OPENAI_API_KEY }}", + "OPENAI_BASE_URL": "${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, "OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}", "allowed engine secret override should remain") + assert.NotContains(t, result, "OPENAI_BASE_URL: ${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1", "unsafe engine env secret should be removed") + }) + + t.Run("removes empty env block after deleting unsafe entries", func(t *testing.T) { + content := `--- +on: workflow_dispatch +engine: + id: codex + env: + OPENAI_BASE_URL: ${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1 +--- +` + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "engine": map[string]any{ + "id": "codex", + "env": map[string]any{ + "OPENAI_BASE_URL": "${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.NotContains(t, result, "\n env:\n", "empty env block should be removed") + assert.Contains(t, result, "id: codex", "engine block should remain") + }) + + t.Run("no-op when only allowed engine secret override is used", func(t *testing.T) { + content := `--- +on: workflow_dispatch +engine: + id: copilot + env: + COPILOT_GITHUB_TOKEN: ${{ secrets.MY_ORG_COPILOT_TOKEN }} +--- +` + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "engine": map[string]any{ + "id": "copilot", + "env": map[string]any{ + "COPILOT_GITHUB_TOKEN": "${{ secrets.MY_ORG_COPILOT_TOKEN }}", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should not error in no-op case") + assert.False(t, applied, "codemod should not apply") + assert.Equal(t, content, result, "content should be unchanged") + }) +} diff --git a/pkg/cli/codemod_steps_run_secrets_env.go b/pkg/cli/codemod_steps_run_secrets_env.go new file mode 100644 index 00000000000..bff9d9e7c17 --- /dev/null +++ b/pkg/cli/codemod_steps_run_secrets_env.go @@ -0,0 +1,284 @@ +package cli + +import ( + "fmt" + "regexp" + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var stepsRunSecretsEnvCodemodLog = logger.New("cli:codemod_steps_run_secrets_env") + +var stepsSecretExprRe = regexp.MustCompile(`\$\{\{\s*secrets\.([A-Za-z_][A-Za-z0-9_]*)\s*\}\}`) + +// getStepsRunSecretsToEnvCodemod creates a codemod that moves secrets interpolated directly +// in run fields to step-level env bindings in steps-like sections. +func getStepsRunSecretsToEnvCodemod() Codemod { + return Codemod{ + ID: "steps-run-secrets-to-env", + Name: "Move step run secrets to env bindings", + Description: "Rewrites secrets interpolated directly in run commands to $VARS and adds step-level env bindings for strict-mode compatibility.", + IntroducedIn: "0.26.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + sections := []string{"pre-steps", "steps", "post-steps", "pre-agent-steps"} + hasTargetSection := false + for _, section := range sections { + if _, ok := frontmatter[section]; ok { + hasTargetSection = true + break + } + } + if !hasTargetSection { + return content, false, nil + } + + newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { + modified := false + current := lines + for _, section := range sections { + var sectionChanged bool + current, sectionChanged = transformSectionStepsRunSecrets(current, section) + modified = modified || sectionChanged + } + return current, modified + }) + if applied { + stepsRunSecretsEnvCodemodLog.Print("Moved inline step run secrets to step-level env bindings") + } + return newContent, applied, err + }, + } +} + +func transformSectionStepsRunSecrets(lines []string, sectionName string) ([]string, bool) { + sectionStart := -1 + sectionIndent := "" + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if isTopLevelKey(line) && strings.HasPrefix(trimmed, sectionName+":") { + sectionStart = i + sectionIndent = getIndentation(line) + break + } + } + if sectionStart == -1 { + return lines, false + } + + sectionEnd := len(lines) - 1 + for i := sectionStart + 1; i < len(lines); i++ { + trimmed := strings.TrimSpace(lines[i]) + if len(trimmed) == 0 || strings.HasPrefix(trimmed, "#") { + continue + } + if len(getIndentation(lines[i])) <= len(sectionIndent) { + sectionEnd = i - 1 + break + } + } + + sectionLines := lines[sectionStart : sectionEnd+1] + updatedSection, changed := transformStepsWithinSection(sectionLines, sectionIndent) + if !changed { + return lines, false + } + + result := make([]string, 0, len(lines)-(len(sectionLines)-len(updatedSection))) + result = append(result, lines[:sectionStart]...) + result = append(result, updatedSection...) + result = append(result, lines[sectionEnd+1:]...) + return result, true +} + +func transformStepsWithinSection(sectionLines []string, sectionIndent string) ([]string, bool) { + result := make([]string, 0, len(sectionLines)) + modified := false + + for i := 0; i < len(sectionLines); { + line := sectionLines[i] + trimmed := strings.TrimSpace(line) + indent := getIndentation(line) + + if strings.HasPrefix(trimmed, "- ") && len(indent) > len(sectionIndent) { + stepStart := i + stepIndent := indent + stepEnd := len(sectionLines) - 1 + for j := i + 1; j < len(sectionLines); j++ { + t := strings.TrimSpace(sectionLines[j]) + if len(t) == 0 { + continue + } + jIndent := getIndentation(sectionLines[j]) + if strings.HasPrefix(t, "- ") && len(jIndent) == len(stepIndent) { + stepEnd = j - 1 + break + } + } + + chunk := append([]string(nil), sectionLines[stepStart:stepEnd+1]...) + updatedChunk, changed := rewriteStepRunSecretsToEnv(chunk, stepIndent) + modified = modified || changed + result = append(result, updatedChunk...) + i = stepEnd + 1 + continue + } + + result = append(result, line) + i++ + } + + return result, modified +} + +func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string, bool) { + modified := false + seen := make(map[string]bool) + orderedSecrets := make([]string, 0) + firstRunLine := -1 + envStart := -1 + envEnd := -1 + envIndent := "" + existingEnvKeys := make(map[string]bool) + + for i := 0; i < len(stepLines); i++ { + line := stepLines[i] + trimmed := strings.TrimSpace(line) + indent := getIndentation(line) + + if strings.HasPrefix(trimmed, "env:") && len(indent) > len(stepIndent) && strings.TrimSpace(strings.TrimPrefix(trimmed, "env:")) == "" { + envStart = i + envIndent = indent + envEnd = i + for j := i + 1; j < len(stepLines); j++ { + t := strings.TrimSpace(stepLines[j]) + if len(t) == 0 { + envEnd = j + continue + } + if len(getIndentation(stepLines[j])) <= len(envIndent) { + break + } + envEnd = j + key := parseYAMLMapKey(t) + if key != "" { + existingEnvKeys[key] = true + } + } + } + + if !strings.HasPrefix(trimmed, "run:") || len(indent) <= len(stepIndent) { + continue + } + if firstRunLine == -1 { + firstRunLine = i + } + + runValue := strings.TrimSpace(strings.TrimPrefix(trimmed, "run:")) + if runValue == "|" || runValue == "|-" || runValue == ">" || runValue == ">-" { + runIndent := indent + for j := i + 1; j < len(stepLines); j++ { + t := strings.TrimSpace(stepLines[j]) + if len(t) == 0 { + continue + } + if len(getIndentation(stepLines[j])) <= len(runIndent) { + break + } + updatedLine, names := replaceStepSecretRefs(stepLines[j]) + if len(names) > 0 { + stepLines[j] = updatedLine + modified = true + } + for _, name := range names { + if !seen[name] { + seen[name] = true + orderedSecrets = append(orderedSecrets, name) + } + } + } + continue + } + + newLine, names := replaceStepSecretRefs(line) + if len(names) > 0 { + stepLines[i] = newLine + modified = true + } + for _, name := range names { + if !seen[name] { + seen[name] = true + orderedSecrets = append(orderedSecrets, name) + } + } + } + + if len(orderedSecrets) == 0 { + return stepLines, modified + } + + missingSecrets := make([]string, 0, len(orderedSecrets)) + for _, name := range orderedSecrets { + if !existingEnvKeys[name] { + missingSecrets = append(missingSecrets, name) + } + } + if len(missingSecrets) == 0 { + return stepLines, true + } + + if envStart != -1 { + insertAt := envEnd + 1 + envValueIndent := envIndent + " " + insertLines := make([]string, 0, len(missingSecrets)) + for _, name := range missingSecrets { + insertLines = append(insertLines, fmt.Sprintf("%s%s: ${{ secrets.%s }}", envValueIndent, name, name)) + } + stepLines = append(stepLines[:insertAt], append(insertLines, stepLines[insertAt:]...)...) + return stepLines, true + } + + if firstRunLine == -1 { + return stepLines, modified + } + + insertIndent := stepIndent + " " + insertLines := []string{insertIndent + "env:"} + for _, name := range missingSecrets { + insertLines = append(insertLines, fmt.Sprintf("%s %s: ${{ secrets.%s }}", insertIndent, name, name)) + } + stepLines = append(stepLines[:firstRunLine], append(insertLines, stepLines[firstRunLine:]...)...) + return stepLines, true +} + +func replaceStepSecretRefs(line string) (string, []string) { + matches := stepsSecretExprRe.FindAllStringSubmatch(line, -1) + if len(matches) == 0 { + return line, nil + } + seen := make(map[string]bool) + ordered := make([]string, 0, len(matches)) + for _, match := range matches { + if len(match) < 2 { + continue + } + name := match[1] + if !seen[name] { + seen[name] = true + ordered = append(ordered, name) + } + } + updated := stepsSecretExprRe.ReplaceAllString(line, `$$$1`) + return updated, ordered +} + +func parseYAMLMapKey(trimmedLine string) string { + if strings.HasPrefix(trimmedLine, "- ") { + return "" + } + parts := strings.SplitN(trimmedLine, ":", 2) + if len(parts) < 2 { + return "" + } + return strings.TrimSpace(parts[0]) +} diff --git a/pkg/cli/codemod_steps_run_secrets_env_test.go b/pkg/cli/codemod_steps_run_secrets_env_test.go new file mode 100644 index 00000000000..af0d14bba84 --- /dev/null +++ b/pkg/cli/codemod_steps_run_secrets_env_test.go @@ -0,0 +1,120 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestStepsRunSecretsToEnvCodemod(t *testing.T) { + codemod := getStepsRunSecretsToEnvCodemod() + + t.Run("moves inline run secret to env binding", func(t *testing.T) { + content := `--- +on: push +steps: + - name: Clone runtime + run: git clone https://x:${{ secrets.RUNTIME_TRIAGE_TOKEN }}@github.com/org/repo.git +--- +` + frontmatter := map[string]any{ + "on": "push", + "steps": []any{ + map[string]any{ + "name": "Clone runtime", + "run": "git clone https://x:${{ secrets.RUNTIME_TRIAGE_TOKEN }}@github.com/org/repo.git", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, "run: git clone https://x:$RUNTIME_TRIAGE_TOKEN@github.com/org/repo.git", "run should use env var") + assert.Contains(t, result, "env:", "step env block should be added") + assert.Contains(t, result, "RUNTIME_TRIAGE_TOKEN: ${{ secrets.RUNTIME_TRIAGE_TOKEN }}", "secret should be bound in env") + }) + + t.Run("appends missing binding to existing env block", func(t *testing.T) { + content := `--- +on: push +steps: + - name: Run checks + env: + FOO: bar + run: echo ${{ secrets.TEST_TOKEN }} +--- +` + frontmatter := map[string]any{ + "on": "push", + "steps": []any{ + map[string]any{ + "name": "Run checks", + "env": map[string]any{ + "FOO": "bar", + }, + "run": "echo ${{ secrets.TEST_TOKEN }}", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, "FOO: bar", "existing env entries should be preserved") + assert.Contains(t, result, "TEST_TOKEN: ${{ secrets.TEST_TOKEN }}", "missing env binding should be added") + assert.Contains(t, result, "run: echo $TEST_TOKEN", "run should use env var") + }) + + t.Run("supports pre-steps section", func(t *testing.T) { + content := `--- +on: pull_request +pre-steps: + - name: Pre check + run: npm config set //registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }} +--- +` + frontmatter := map[string]any{ + "on": "pull_request", + "pre-steps": []any{ + map[string]any{ + "name": "Pre check", + "run": "npm config set //registry.npmjs.org/:_authToken=${{ secrets.NPM_TOKEN }}", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, "_authToken=$NPM_TOKEN", "secret should be replaced with shell env reference") + assert.Contains(t, result, "NPM_TOKEN: ${{ secrets.NPM_TOKEN }}", "env binding should be added") + }) + + t.Run("no-op when no inline run secrets are present", func(t *testing.T) { + content := `--- +on: push +steps: + - name: Safe + run: echo "hello" +--- +` + frontmatter := map[string]any{ + "on": "push", + "steps": []any{ + map[string]any{ + "name": "Safe", + "run": `echo "hello"`, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should not error in no-op case") + assert.False(t, applied, "codemod should not apply") + assert.Equal(t, content, result, "content should be unchanged") + }) +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index e0f627d7eaf..61bc8034f38 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -43,6 +43,8 @@ func GetAllCodemods() []Codemod { getRolesToOnRolesCodemod(), // Move top-level roles to on.roles getBotsToOnBotsCodemod(), // Move top-level bots to on.bots getEngineStepsToTopLevelCodemod(), // Move engine.steps to top-level steps + getStepsRunSecretsToEnvCodemod(), // Move inline secrets in step run fields to step env bindings + getEngineEnvSecretsCodemod(), // Remove unsafe secret-bearing engine.env entries getAssignToAgentDefaultAgentCodemod(), // Rename deprecated default-agent to name in assign-to-agent getPlaywrightDomainsToNetworkAllowedCodemod(), // Migrate tools.playwright.allowed_domains to network.allowed getExpiresIntegerToDayStringCodemod(), // Convert expires integer (days) to string with 'd' suffix diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 04ec1009338..71923c4289d 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -43,7 +43,7 @@ func TestGetAllCodemods_ReturnsAllCodemods(t *testing.T) { codemods := GetAllCodemods() // Verify we have the expected number of codemods - expectedCount := 30 + expectedCount := 32 assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount) // Verify all codemods have required fields @@ -81,6 +81,8 @@ func TestGetAllCodemods_ContainsExpectedCodemods(t *testing.T) { "grep-tool-removal", "mcp-network-to-top-level-migration", "safe-inputs-to-mcp-scripts", + "steps-run-secrets-to-env", + "engine-env-secrets-to-engine-config", } for _, expectedID := range expectedIDs { @@ -126,6 +128,8 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) { "roles-to-on-roles", "bots-to-on-bots", "engine-steps-to-top-level", + "steps-run-secrets-to-env", + "engine-env-secrets-to-engine-config", "assign-to-agent-default-agent-to-name", "playwright-allowed-domains-migration", "expires-integer-to-string", From e6ef77a21b2b4098f2a2d7613228f011a18534e1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 18:27:25 +0000 Subject: [PATCH 3/7] chore: address validation feedback in engine env codemod Agent-Logs-Url: https://github.com/github/gh-aw/sessions/41aed0dc-59cf-45eb-b96f-b353d787e84c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/codemod_engine_env_secrets.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/cli/codemod_engine_env_secrets.go b/pkg/cli/codemod_engine_env_secrets.go index 49fe7edab35..50b68128901 100644 --- a/pkg/cli/codemod_engine_env_secrets.go +++ b/pkg/cli/codemod_engine_env_secrets.go @@ -73,7 +73,11 @@ func extractEngineIDForCodemod(frontmatter map[string]any, engineMap map[string] func allowedEngineEnvSecretKeys(engineID string) map[string]bool { allowed := make(map[string]bool) - for _, req := range getSecretRequirementsForEngine(engineID, false, false) { + for _, req := range getSecretRequirementsForEngine( + engineID, + false, // includeSystemSecrets + false, // includeOptional + ) { allowed[req.Name] = true } return allowed From ac096d71b5bf439c38a4797dd979c5d4818bee29 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 18:29:09 +0000 Subject: [PATCH 4/7] test: expand strict secrets codemod coverage and clarify replacement logic Agent-Logs-Url: https://github.com/github/gh-aw/sessions/41aed0dc-59cf-45eb-b96f-b353d787e84c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/codemod_engine_env_secrets.go | 3 ++ pkg/cli/codemod_steps_run_secrets_env.go | 2 + pkg/cli/codemod_steps_run_secrets_env_test.go | 37 +++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/pkg/cli/codemod_engine_env_secrets.go b/pkg/cli/codemod_engine_env_secrets.go index 50b68128901..80f671a046c 100644 --- a/pkg/cli/codemod_engine_env_secrets.go +++ b/pkg/cli/codemod_engine_env_secrets.go @@ -73,6 +73,9 @@ func extractEngineIDForCodemod(frontmatter map[string]any, engineMap map[string] func allowedEngineEnvSecretKeys(engineID string) map[string]bool { allowed := make(map[string]bool) + // Keep only required, engine-specific secret names here. + // We intentionally exclude system and optional secrets so this codemod only + // preserves strict-mode-safe engine credential overrides. for _, req := range getSecretRequirementsForEngine( engineID, false, // includeSystemSecrets diff --git a/pkg/cli/codemod_steps_run_secrets_env.go b/pkg/cli/codemod_steps_run_secrets_env.go index bff9d9e7c17..91ac8f7aeee 100644 --- a/pkg/cli/codemod_steps_run_secrets_env.go +++ b/pkg/cli/codemod_steps_run_secrets_env.go @@ -268,6 +268,8 @@ func replaceStepSecretRefs(line string) (string, []string) { ordered = append(ordered, name) } } + // "$$$1" means: "$$" -> literal "$", "$1" -> capture group 1 (secret name), + // resulting in "$SECRET_NAME" shell env references. updated := stepsSecretExprRe.ReplaceAllString(line, `$$$1`) return updated, ordered } diff --git a/pkg/cli/codemod_steps_run_secrets_env_test.go b/pkg/cli/codemod_steps_run_secrets_env_test.go index af0d14bba84..554d4ca99b1 100644 --- a/pkg/cli/codemod_steps_run_secrets_env_test.go +++ b/pkg/cli/codemod_steps_run_secrets_env_test.go @@ -34,6 +34,7 @@ steps: require.NoError(t, err, "codemod should apply cleanly") assert.True(t, applied, "codemod should apply") assert.Contains(t, result, "run: git clone https://x:$RUNTIME_TRIAGE_TOKEN@github.com/org/repo.git", "run should use env var") + assert.NotContains(t, result, "${{ secrets.RUNTIME_TRIAGE_TOKEN }}@github.com", "run should no longer include secret interpolation") assert.Contains(t, result, "env:", "step env block should be added") assert.Contains(t, result, "RUNTIME_TRIAGE_TOKEN: ${{ secrets.RUNTIME_TRIAGE_TOKEN }}", "secret should be bound in env") }) @@ -94,6 +95,42 @@ pre-steps: assert.Contains(t, result, "NPM_TOKEN: ${{ secrets.NPM_TOKEN }}", "env binding should be added") }) + t.Run("supports post-steps and pre-agent-steps sections", func(t *testing.T) { + content := `--- +on: pull_request +post-steps: + - name: Notify + run: 'curl -H "Authorization: Bearer ${{ secrets.POST_TOKEN }}" https://example.com' +pre-agent-steps: + - name: Setup + run: echo "${{ secrets.PRE_AGENT_TOKEN }}" +--- +` + frontmatter := map[string]any{ + "on": "pull_request", + "post-steps": []any{ + map[string]any{ + "name": "Notify", + "run": `curl -H "Authorization: Bearer ${{ secrets.POST_TOKEN }}" https://example.com`, + }, + }, + "pre-agent-steps": []any{ + map[string]any{ + "name": "Setup", + "run": `echo "${{ secrets.PRE_AGENT_TOKEN }}"`, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, `Authorization: Bearer $POST_TOKEN`, "post-steps run command should use env variable") + assert.Contains(t, result, "POST_TOKEN: ${{ secrets.POST_TOKEN }}", "post-steps should receive env binding") + assert.Contains(t, result, `echo "$PRE_AGENT_TOKEN"`, "pre-agent-steps run command should use env variable") + assert.Contains(t, result, "PRE_AGENT_TOKEN: ${{ secrets.PRE_AGENT_TOKEN }}", "pre-agent-steps should receive env binding") + }) + t.Run("no-op when no inline run secrets are present", func(t *testing.T) { content := `--- on: push From 91f7c48877089f7a9183c9b678343bf9bd1a33e9 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:07:33 +0000 Subject: [PATCH 5/7] docs(adr): add draft ADR-26919 for strict-mode secret leak remediation codemods Co-Authored-By: Claude Sonnet 4.6 --- ...for-strict-mode-secret-leak-remediation.md | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 docs/adr/26919-automated-codemods-for-strict-mode-secret-leak-remediation.md diff --git a/docs/adr/26919-automated-codemods-for-strict-mode-secret-leak-remediation.md b/docs/adr/26919-automated-codemods-for-strict-mode-secret-leak-remediation.md new file mode 100644 index 00000000000..e9eef635017 --- /dev/null +++ b/docs/adr/26919-automated-codemods-for-strict-mode-secret-leak-remediation.md @@ -0,0 +1,93 @@ +# ADR-26919: Automated Codemods for Strict-Mode Secret Leak Remediation + +**Date**: 2026-04-17 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +With the introduction of strict mode for `gh-aw` workflows (see ADR-0002), two common authoring patterns became non-conformant: (1) secrets interpolated directly into step `run:` commands as `${{ secrets.NAME }}`, and (2) secret-bearing entries in `engine.env` that are not required engine credential overrides. Both patterns cause recurring `aw-compat` CI failures because the validator rejects them under strict mode. Authors need a way to automatically migrate existing workflow files to the compliant form without manually editing every affected file. The `gh aw fix --write` command already provides a codemod registry for automated workflow migrations; extending it is the natural path to zero-friction remediation. + +### Decision + +We will add two new codemods to the `gh aw fix` codemod registry. The first codemod (`steps-run-secrets-to-env`) rewrites inline `${{ secrets.NAME }}` expressions in step `run:` fields to shell environment variable references (`$NAME`) and injects the corresponding `NAME: ${{ secrets.NAME }}` binding into the step's `env:` block, creating one if absent. The second codemod (`engine-env-secrets-to-engine-config`) removes secret-bearing `engine.env` entries that are not on the allowlist of required engine credential overrides, and prunes any `engine.env` block left empty by those removals. Both codemods use text-based (line-by-line) YAML transformation to preserve original formatting. + +### Alternatives Considered + +#### Alternative 1: Emit Only Errors, Require Manual Migration + +The validator already reports which lines violate strict mode. Requiring authors to fix violations manually, without providing an automated path, was the status quo. It was rejected because the same patterns appear across many workflow files, making manual remediation costly and error-prone, and because the codemod framework exists specifically to absorb this class of migration burden. + +#### Alternative 2: Full YAML Parse-and-Serialize for Transformation + +Transforming by parsing the YAML into a structured representation and re-serializing would be more robust against edge cases (e.g., unusual indentation, multi-line scalars). It was rejected because full round-trip YAML serialization destroys formatting, comments, and non-standard quoting that authors rely on. Every other codemod in the registry uses the same line-based approach, so consistency and formatting preservation outweigh the robustness gain. + +#### Alternative 3: Mask or Remove Secrets Instead of Injecting env: Bindings + +For step `run:` secrets, an alternative is to replace the `${{ secrets.NAME }}` expression with a fixed mask string (e.g., `***`) or remove the expression entirely. This was rejected because it produces non-functional workflows: the `run:` command needs the credential at runtime. The `env:` injection strategy produces a fully functional, compliant workflow by moving the secret to a location the runner safely masks. + +#### Alternative 4: Migrate engine.env Secrets to engine.config Instead of Removing + +Rather than removing unsafe `engine.env` keys, they could be moved to a different engine configuration location. This was rejected because there is no structural equivalent of `engine.env` that is both strict-mode-safe and semantically correct for arbitrary user-supplied secrets. Allowed engine credential overrides are a defined, finite set; everything else in `engine.env` that carries a secret is an authoring mistake rather than a value that belongs somewhere else. + +### Consequences + +#### Positive +- Authors can run `gh aw fix --write` to automatically remediate both classes of strict-mode secret leak with no manual edits. +- The transformation produces fully functional workflows: step `run:` commands continue to work via shell environment variables; engine configuration is consistent with the allowlist. +- Formatting, comments, and indentation of the original YAML are preserved, minimizing diff noise. +- Both codemods are version-stamped (`IntroducedIn: 0.26.0`) so the registry can report when they were added. + +#### Negative +- Line-based transformation can mishandle pathological YAML (e.g., multi-line block scalars with atypical indentation). Edge cases not covered by tests may produce incorrect output. +- The `engine.env` codemod silently removes keys without migrating their values elsewhere. If a removed key was intentional, the author must restore the credential through a different mechanism. +- The allowlist for required engine credential overrides is computed at codemod execution time via `getSecretRequirementsForEngine()`; if that function's output changes, previously safe keys may become removable (or vice versa). + +#### Neutral +- Both codemods are appended to the existing ordered registry in `GetAllCodemods()`. The registry order matters for codemods that may interact; these two are appended after existing codemods and have no known ordering dependencies. +- Tests for the new codemods follow the established pattern of table-driven before/after string comparisons, consistent with the rest of the codemod test suite. +- The `removeEmptyEngineEnvBlock` cleanup pass runs unconditionally after key removal, regardless of whether any keys were actually removed; callers must check the `modified` flag from the prior step before invoking it. + +--- + +## 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). + +### steps-run-secrets-to-env Codemod + +1. Implementations **MUST** scan the `pre-steps`, `steps`, `post-steps`, and `pre-agent-steps` sections of a workflow frontmatter for step entries that contain `${{ secrets.NAME }}` expressions in their `run:` field. +2. For each matched secret expression in a `run:` field, implementations **MUST** replace the expression with the shell variable reference `$NAME` (where `NAME` is the secret identifier captured from the expression). +3. Implementations **MUST** add a `NAME: ${{ secrets.NAME }}` binding to the step's `env:` block for each secret replaced, merging into an existing `env:` block if one is present, or inserting a new `env:` block immediately before the `run:` field if none exists. +4. Implementations **MUST NOT** add a duplicate `env:` binding for a secret name that is already present as a key in the step's existing `env:` block. +5. Implementations **MUST** handle multi-line block scalar `run:` values (indicated by `|`, `|-`, `>`, or `>-` scalar indicators) by replacing secret expressions within each continuation line. +6. Implementations **MUST NOT** modify workflow files that contain no `pre-steps`, `steps`, `post-steps`, or `pre-agent-steps` keys in their frontmatter. +7. Implementations **SHOULD** deduplicate env bindings when the same secret name appears multiple times within a single `run:` field. + +### engine-env-secrets-to-engine-config Codemod + +1. Implementations **MUST** inspect the `engine.env` map of a workflow frontmatter and identify entries whose values contain one or more `${{ secrets.* }}` expressions. +2. Implementations **MUST** compute the allowlist of required engine credential override key names by calling `getSecretRequirementsForEngine()` with `includeSystemSecrets=false` and `includeOptional=false` for the engine identified in the frontmatter. +3. Implementations **MUST** remove any `engine.env` key that (a) contains a secret expression and (b) is not present in the allowlist computed in requirement 2. +4. Implementations **MUST NOT** remove `engine.env` keys that contain secret expressions if those keys are in the computed allowlist. +5. Implementations **MUST NOT** remove `engine.env` keys whose values do not contain any secret expression, regardless of allowlist membership. +6. Implementations **MUST** remove the `env:` block from `engine:` if, after removing unsafe keys, the block contains no remaining entries. +7. Implementations **MUST NOT** modify workflow files that have no `engine` key or no `engine.env` block in their frontmatter. + +### Codemod Registry Integration + +1. Both codemods **MUST** be registered in `GetAllCodemods()` with stable, unique `ID` values (`steps-run-secrets-to-env` and `engine-env-secrets-to-engine-config` respectively). +2. Each codemod **MUST** declare an `IntroducedIn` version string reflecting the `gh-aw` CLI version in which the codemod was first available. +3. Codemods **MUST** return `(content, false, nil)` when no transformation applies to a given file, leaving the content unchanged. + +### 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. In particular: silently dropping secrets from `run:` without injecting the corresponding `env:` binding, and removing allowlisted engine credential override keys, are both non-conformant behaviors. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24581919524) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* From edd2eb3ae790c7ab12a60d00cab9308d8a724802 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 21:53:21 +0000 Subject: [PATCH 6/7] fix(codemod): support inline step keys and runtime engine id extraction Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b02b73dc-c444-45fa-a95d-8079903d100f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/codemod_engine_env_secrets.go | 10 +++- pkg/cli/codemod_engine_env_secrets_test.go | 31 +++++++++++ pkg/cli/codemod_steps_run_secrets_env.go | 37 ++++++++++--- pkg/cli/codemod_steps_run_secrets_env_test.go | 52 +++++++++++++++++++ 4 files changed, 122 insertions(+), 8 deletions(-) diff --git a/pkg/cli/codemod_engine_env_secrets.go b/pkg/cli/codemod_engine_env_secrets.go index 80f671a046c..8ea38dae4ae 100644 --- a/pkg/cli/codemod_engine_env_secrets.go +++ b/pkg/cli/codemod_engine_env_secrets.go @@ -65,6 +65,13 @@ func extractEngineIDForCodemod(frontmatter map[string]any, engineMap map[string] if id, ok := engineMap["id"].(string); ok && id != "" { return id } + if runtimeAny, hasRuntime := engineMap["runtime"]; hasRuntime { + if runtimeMap, ok := runtimeAny.(map[string]any); ok { + if id, ok := runtimeMap["id"].(string); ok && id != "" { + return id + } + } + } if id, ok := frontmatter["engine"].(string); ok && id != "" { return id } @@ -74,7 +81,8 @@ func extractEngineIDForCodemod(frontmatter map[string]any, engineMap map[string] func allowedEngineEnvSecretKeys(engineID string) map[string]bool { allowed := make(map[string]bool) // Keep only required, engine-specific secret names here. - // We intentionally exclude system and optional secrets so this codemod only + // We intentionally exclude system secrets (for example GH_AW_GITHUB_TOKEN) + // and optional secrets so this codemod only // preserves strict-mode-safe engine credential overrides. for _, req := range getSecretRequirementsForEngine( engineID, diff --git a/pkg/cli/codemod_engine_env_secrets_test.go b/pkg/cli/codemod_engine_env_secrets_test.go index d3a98731800..a1c03c258a4 100644 --- a/pkg/cli/codemod_engine_env_secrets_test.go +++ b/pkg/cli/codemod_engine_env_secrets_test.go @@ -90,4 +90,35 @@ engine: assert.False(t, applied, "codemod should not apply") assert.Equal(t, content, result, "content should be unchanged") }) + + t.Run("supports inline engine runtime.id for allowlist", func(t *testing.T) { + content := `--- +on: workflow_dispatch +engine: + runtime: + id: codex + env: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + OPENAI_BASE_URL: ${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1 +--- +` + frontmatter := map[string]any{ + "on": "workflow_dispatch", + "engine": map[string]any{ + "runtime": map[string]any{ + "id": "codex", + }, + "env": map[string]any{ + "OPENAI_API_KEY": "${{ secrets.OPENAI_API_KEY }}", + "OPENAI_BASE_URL": "${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, "OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}", "required engine secret should be preserved when using runtime.id") + assert.NotContains(t, result, "OPENAI_BASE_URL: ${{ secrets.AZURE_OPENAI_ENDPOINT }}openai/v1", "unsafe secret-bearing key should still be removed") + }) } diff --git a/pkg/cli/codemod_steps_run_secrets_env.go b/pkg/cli/codemod_steps_run_secrets_env.go index 91ac8f7aeee..8c8ba438aa4 100644 --- a/pkg/cli/codemod_steps_run_secrets_env.go +++ b/pkg/cli/codemod_steps_run_secrets_env.go @@ -139,6 +139,7 @@ func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string envStart := -1 envEnd := -1 envIndent := "" + var envKeyIndentLen int existingEnvKeys := make(map[string]bool) for i := 0; i < len(stepLines); i++ { @@ -146,9 +147,11 @@ func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string trimmed := strings.TrimSpace(line) indent := getIndentation(line) - if strings.HasPrefix(trimmed, "env:") && len(indent) > len(stepIndent) && strings.TrimSpace(strings.TrimPrefix(trimmed, "env:")) == "" { + envMatch, envValue, currentEnvKeyIndentLen := parseStepKeyLine(trimmed, indent, stepIndent, "env") + if envMatch && envValue == "" { envStart = i envIndent = indent + envKeyIndentLen = currentEnvKeyIndentLen envEnd = i for j := i + 1; j < len(stepLines); j++ { t := strings.TrimSpace(stepLines[j]) @@ -156,7 +159,7 @@ func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string envEnd = j continue } - if len(getIndentation(stepLines[j])) <= len(envIndent) { + if effectiveStepLineIndentLen(t, getIndentation(stepLines[j]), stepIndent) <= envKeyIndentLen { break } envEnd = j @@ -167,22 +170,21 @@ func rewriteStepRunSecretsToEnv(stepLines []string, stepIndent string) ([]string } } - if !strings.HasPrefix(trimmed, "run:") || len(indent) <= len(stepIndent) { + runMatch, runValue, runKeyIndentLen := parseStepKeyLine(trimmed, indent, stepIndent, "run") + if !runMatch { continue } if firstRunLine == -1 { firstRunLine = i } - runValue := strings.TrimSpace(strings.TrimPrefix(trimmed, "run:")) if runValue == "|" || runValue == "|-" || runValue == ">" || runValue == ">-" { - runIndent := indent for j := i + 1; j < len(stepLines); j++ { t := strings.TrimSpace(stepLines[j]) if len(t) == 0 { continue } - if len(getIndentation(stepLines[j])) <= len(runIndent) { + if effectiveStepLineIndentLen(t, getIndentation(stepLines[j]), stepIndent) <= runKeyIndentLen { break } updatedLine, names := replaceStepSecretRefs(stepLines[j]) @@ -268,12 +270,33 @@ func replaceStepSecretRefs(line string) (string, []string) { ordered = append(ordered, name) } } - // "$$$1" means: "$$" -> literal "$", "$1" -> capture group 1 (secret name), + // In Go regexp replacement syntax, "$$$1" means: + // "$$" -> literal "$" in output, then "$1" -> capture group 1 (secret name), // resulting in "$SECRET_NAME" shell env references. updated := stepsSecretExprRe.ReplaceAllString(line, `$$$1`) return updated, ordered } +func parseStepKeyLine(trimmed, indent, stepIndent, key string) (bool, string, int) { + if strings.HasPrefix(trimmed, key+":") && len(indent) > len(stepIndent) { + value := strings.TrimSpace(strings.TrimPrefix(trimmed, key+":")) + return true, value, len(indent) + } + listKeyPrefix := "- " + key + ":" + if strings.HasPrefix(trimmed, listKeyPrefix) && len(indent) == len(stepIndent) { + value := strings.TrimSpace(strings.TrimPrefix(trimmed, listKeyPrefix)) + return true, value, len(stepIndent) + 2 + } + return false, "", 0 +} + +func effectiveStepLineIndentLen(trimmed, indent, stepIndent string) int { + if strings.HasPrefix(trimmed, "- ") && len(indent) == len(stepIndent) { + return len(stepIndent) + 2 + } + return len(indent) +} + func parseYAMLMapKey(trimmedLine string) string { if strings.HasPrefix(trimmedLine, "- ") { return "" diff --git a/pkg/cli/codemod_steps_run_secrets_env_test.go b/pkg/cli/codemod_steps_run_secrets_env_test.go index 554d4ca99b1..f7a938c86dc 100644 --- a/pkg/cli/codemod_steps_run_secrets_env_test.go +++ b/pkg/cli/codemod_steps_run_secrets_env_test.go @@ -131,6 +131,58 @@ pre-agent-steps: assert.Contains(t, result, "PRE_AGENT_TOKEN: ${{ secrets.PRE_AGENT_TOKEN }}", "pre-agent-steps should receive env binding") }) + t.Run("supports list-item-inline run key", func(t *testing.T) { + content := `--- +on: push +steps: + - run: echo ${{ secrets.INLINE_TOKEN }} +--- +` + frontmatter := map[string]any{ + "on": "push", + "steps": []any{ + map[string]any{ + "run": "echo ${{ secrets.INLINE_TOKEN }}", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, "run: echo $INLINE_TOKEN", "inline run should be rewritten") + assert.Contains(t, result, "INLINE_TOKEN: ${{ secrets.INLINE_TOKEN }}", "inline run should get env binding") + }) + + t.Run("supports list-item-inline env key with run sibling", func(t *testing.T) { + content := `--- +on: push +steps: + - env: + PRESENT_TOKEN: ${{ secrets.PRESENT_TOKEN }} + run: echo ${{ secrets.NEW_TOKEN }} +--- +` + frontmatter := map[string]any{ + "on": "push", + "steps": []any{ + map[string]any{ + "env": map[string]any{ + "PRESENT_TOKEN": "${{ secrets.PRESENT_TOKEN }}", + }, + "run": "echo ${{ secrets.NEW_TOKEN }}", + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "codemod should apply cleanly") + assert.True(t, applied, "codemod should apply") + assert.Contains(t, result, "PRESENT_TOKEN: ${{ secrets.PRESENT_TOKEN }}", "existing env key should remain") + assert.Contains(t, result, "NEW_TOKEN: ${{ secrets.NEW_TOKEN }}", "new env key should be added") + assert.Contains(t, result, "run: echo $NEW_TOKEN", "run should be rewritten to env var") + }) + t.Run("no-op when no inline run secrets are present", func(t *testing.T) { content := `--- on: push From b152c28a406a9c5bd0eff3bee81aaf5fa5f0082a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 21:54:14 +0000 Subject: [PATCH 7/7] docs: document inline step parsing helpers in codemod Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b02b73dc-c444-45fa-a95d-8079903d100f Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/codemod_steps_run_secrets_env.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pkg/cli/codemod_steps_run_secrets_env.go b/pkg/cli/codemod_steps_run_secrets_env.go index 8c8ba438aa4..5a20afa2d36 100644 --- a/pkg/cli/codemod_steps_run_secrets_env.go +++ b/pkg/cli/codemod_steps_run_secrets_env.go @@ -277,6 +277,19 @@ func replaceStepSecretRefs(line string) (string, []string) { return updated, ordered } +// parseStepKeyLine detects a YAML step key in both standard form ("key: value") +// and list-item-inline form ("- key: value"). +// +// Parameters: +// - trimmed: current line with surrounding whitespace trimmed +// - indent: raw indentation of the current line +// - stepIndent: indentation of the step list item line +// - key: YAML key name to match (for example "run" or "env") +// +// Returns: +// - matched: whether the line contains the requested key in either supported form +// - value: trimmed value after the key (empty for block-style keys) +// - keyIndentLen: effective indentation length for block-boundary checks func parseStepKeyLine(trimmed, indent, stepIndent, key string) (bool, string, int) { if strings.HasPrefix(trimmed, key+":") && len(indent) > len(stepIndent) { value := strings.TrimSpace(strings.TrimPrefix(trimmed, key+":")) @@ -290,6 +303,12 @@ func parseStepKeyLine(trimmed, indent, stepIndent, key string) (bool, string, in return false, "", 0 } +// effectiveStepLineIndentLen returns the logical indentation length for a line +// within a step block. +// +// For list-item-inline lines like "- run: ...", the "- " marker contributes two +// characters to the effective YAML nesting level, so this function adds 2 to the +// physical step indentation when computing boundary comparisons. func effectiveStepLineIndentLen(trimmed, indent, stepIndent string) int { if strings.HasPrefix(trimmed, "- ") && len(indent) == len(stepIndent) { return len(stepIndent) + 2