Disallow invalid permissions: read shorthand, require read-all

pkg/cli/codemod_permissions.go

@@ -7,6 +7,84 @@ import (
 	"github.com/githubnext/gh-aw/pkg/logger"
 )
 
+var permissionsReadCodemodLog = logger.New("cli:codemod_permissions_read")
+
+// getPermissionsReadCodemod creates a codemod for converting invalid "read" and "write" shorthands
+func getPermissionsReadCodemod() Codemod {
+	return Codemod{
+		ID:           "permissions-read-to-read-all",
+		Name:         "Convert invalid permissions shorthand",
+		Description:  "Converts 'permissions: read' to 'permissions: read-all' and 'permissions: write' to 'permissions: write-all' as per GitHub Actions spec",
+		IntroducedIn: "0.5.0",
+		Apply: func(content string, frontmatter map[string]any) (string, bool, error) {
+			// Check if permissions exist
+			permissionsValue, hasPermissions := frontmatter["permissions"]
+			if !hasPermissions {
+				return content, false, nil
+			}
+
+			// Check if permissions uses invalid shorthand (read or write)
+			hasInvalidShorthand := false
+			if strValue, ok := permissionsValue.(string); ok {
+				if strValue == "read" || strValue == "write" {
+					hasInvalidShorthand = true
+				}
+			}
+
+			if !hasInvalidShorthand {
+				return content, false, nil
+			}
+
+			// Parse frontmatter to get raw lines
+			frontmatterLines, markdown, err := parseFrontmatterLines(content)
+			if err != nil {
+				return content, false, err
+			}
+
+			// Find and replace invalid shorthand permissions
+			var modified bool
+			result := make([]string, len(frontmatterLines))
+
+			for i, line := range frontmatterLines {
+				trimmedLine := strings.TrimSpace(line)
+
+				// Check for permissions line with shorthand
+				if strings.HasPrefix(trimmedLine, "permissions:") {
+					// Handle shorthand on same line: "permissions: read" or "permissions: write"
+					if strings.Contains(trimmedLine, ": read") && !strings.Contains(trimmedLine, "read-all") && !strings.Contains(trimmedLine, ": read\n") {
+						// Make sure it's "permissions: read" and not "contents: read"
+						if strings.TrimSpace(strings.Split(line, ":")[0]) == "permissions" {
+							result[i] = strings.Replace(line, ": read", ": read-all", 1)
+							modified = true
+							permissionsReadCodemodLog.Printf("Replaced 'permissions: read' with 'permissions: read-all' on line %d", i+1)
+							continue
+						}
+					} else if strings.Contains(trimmedLine, ": write") && !strings.Contains(trimmedLine, "write-all") {
+						// Make sure it's "permissions: write" and not "contents: write"
+						if strings.TrimSpace(strings.Split(line, ":")[0]) == "permissions" {
+							result[i] = strings.Replace(line, ": write", ": write-all", 1)
+							modified = true
+							permissionsReadCodemodLog.Printf("Replaced 'permissions: write' with 'permissions: write-all' on line %d", i+1)
+							continue
+						}
+					}
+				}
+
+				result[i] = line
+			}
+
+			if !modified {
+				return content, false, nil
+			}
+
+			// Reconstruct the content
+			newContent := reconstructContent(result, markdown)
+			permissionsReadCodemodLog.Print("Applied permissions read/write to read-all/write-all migration")
+			return newContent, true, nil
+		},
+	}
+}
+
 var writePermissionsCodemodLog = logger.New("cli:codemod_permissions")
 
 // getWritePermissionsCodemod creates a codemod for converting write permissions to read

pkg/cli/codemod_permissions_test.go

@@ -7,6 +7,180 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestGetPermissionsReadCodemod(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	assert.Equal(t, "permissions-read-to-read-all", codemod.ID)
+	assert.Equal(t, "Convert invalid permissions shorthand", codemod.Name)
+	assert.NotEmpty(t, codemod.Description)
+	assert.Equal(t, "0.5.0", codemod.IntroducedIn)
+	require.NotNil(t, codemod.Apply)
+}
+
+func TestPermissionsReadCodemod_Read(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	content := `---
+on: workflow_dispatch
+permissions: read
+---
+
+# Test`
+
+	frontmatter := map[string]any{
+		"on":          "workflow_dispatch",
+		"permissions": "read",
+	}
+
+	result, applied, err := codemod.Apply(content, frontmatter)
+
+	require.NoError(t, err)
+	assert.True(t, applied)
+	assert.Contains(t, result, "permissions: read-all")
+	assert.NotContains(t, result, "permissions: read\n")
+}
+
+func TestPermissionsReadCodemod_Write(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	content := `---
+on: workflow_dispatch
+permissions: write
+---
+
+# Test`
+
+	frontmatter := map[string]any{
+		"on":          "workflow_dispatch",
+		"permissions": "write",
+	}
+
+	result, applied, err := codemod.Apply(content, frontmatter)
+
+	require.NoError(t, err)
+	assert.True(t, applied)
+	assert.Contains(t, result, "permissions: write-all")
+	assert.NotContains(t, result, "permissions: write\n")
+}
+
+func TestPermissionsReadCodemod_NoChange_ReadAll(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	content := `---
+on: workflow_dispatch
+permissions: read-all
+---
+
+# Test`
+
+	frontmatter := map[string]any{
+		"on":          "workflow_dispatch",
+		"permissions": "read-all",
+	}
+
+	result, applied, err := codemod.Apply(content, frontmatter)
+
+	require.NoError(t, err)
+	assert.False(t, applied)
+	assert.Equal(t, content, result)
+}
+
+func TestPermissionsReadCodemod_NoChange_WriteAll(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	content := `---
+on: workflow_dispatch
+permissions: write-all
+---
+
+# Test`
+
+	frontmatter := map[string]any{
+		"on":          "workflow_dispatch",
+		"permissions": "write-all",
+	}
+
+	result, applied, err := codemod.Apply(content, frontmatter)
+
+	require.NoError(t, err)
+	assert.False(t, applied)
+	assert.Equal(t, content, result)
+}
+
+func TestPermissionsReadCodemod_NoChange_MapFormat(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	content := `---
+on: workflow_dispatch
+permissions:
+  contents: read
+  issues: read
+---
+
+# Test`
+
+	frontmatter := map[string]any{
+		"on": "workflow_dispatch",
+		"permissions": map[string]any{
+			"contents": "read",
+			"issues":   "read",
+		},
+	}
+
+	result, applied, err := codemod.Apply(content, frontmatter)
+
+	require.NoError(t, err)
+	assert.False(t, applied)
+	assert.Equal(t, content, result)
+}
+
+func TestPermissionsReadCodemod_NoPermissions(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	content := `---
+on: workflow_dispatch
+timeout-minutes: 30
+---
+
+# Test`
+
+	frontmatter := map[string]any{
+		"on":              "workflow_dispatch",
+		"timeout-minutes": 30,
+	}
+
+	result, applied, err := codemod.Apply(content, frontmatter)
+
+	require.NoError(t, err)
+	assert.False(t, applied)
+	assert.Equal(t, content, result)
+}
+
+func TestPermissionsReadCodemod_PreservesMarkdown(t *testing.T) {
+	codemod := getPermissionsReadCodemod()
+
+	content := `---
+on: workflow_dispatch
+permissions: read
+---
+
+# Test Workflow
+
+This workflow needs permissions.`
+
+	frontmatter := map[string]any{
+		"on":          "workflow_dispatch",
+		"permissions": "read",
+	}
+
+	result, applied, err := codemod.Apply(content, frontmatter)
+
+	require.NoError(t, err)
+	assert.True(t, applied)
+	assert.Contains(t, result, "# Test Workflow")
+	assert.Contains(t, result, "This workflow needs permissions.")
+}
+
 func TestGetWritePermissionsCodemod(t *testing.T) {
 	codemod := getWritePermissionsCodemod()
 

pkg/cli/fix_codemods.go

@@ -24,6 +24,7 @@ func GetAllCodemods() []Codemod {
 		getSafeInputsModeCodemod(),
 		getUploadAssetsCodemod(),
 		getWritePermissionsCodemod(),
+		getPermissionsReadCodemod(), // Fix permissions: read -> permissions: read-all
 		getAgentTaskToAgentSessionCodemod(),
 		getSandboxAgentFalseRemovalCodemod(),
 		getScheduleAtToAroundCodemod(),

pkg/cli/fix_codemods_test.go

@@ -41,7 +41,7 @@ func TestGetAllCodemods_ReturnsAllCodemods(t *testing.T) {
 	codemods := GetAllCodemods()
 
 	// Verify we have the expected number of codemods
-	expectedCount := 11
+	expectedCount := 12
 	assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount)
 
 	// Verify all codemods have required fields
@@ -71,6 +71,7 @@ func TestGetAllCodemods_ContainsExpectedCodemods(t *testing.T) {
 		"safe-inputs-mode-removal",
 		"upload-assets-to-upload-asset-migration",
 		"write-permissions-to-read-migration",
+		"permissions-read-to-read-all",
 		"agent-task-to-agent-session-migration",
 		"sandbox-agent-false-removal",
 		"schedule-at-to-around-migration",
@@ -106,6 +107,7 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) {
 		"safe-inputs-mode-removal",
 		"upload-assets-to-upload-asset-migration",
 		"write-permissions-to-read-migration",
+		"permissions-read-to-read-all",
 		"agent-task-to-agent-session-migration",
 		"sandbox-agent-false-removal",
 		"schedule-at-to-around-migration",

pkg/cli/update_check_integration_test.go

@@ -51,7 +51,7 @@ func TestUpdateCheckIntegration(t *testing.T) {
 		workflowContent := `---
 name: Test Workflow
 on: workflow_dispatch
-permissions: read
+permissions: read-all
 engine: copilot
 ---
 
@@ -94,7 +94,7 @@ Test workflow content.
 		workflowContent := `---
 name: Test Workflow
 on: workflow_dispatch
-permissions: read
+permissions: read-all
 engine: copilot
 ---
 
@@ -139,7 +139,7 @@ Test workflow content.
 		workflowContent := `---
 name: Test Workflow
 on: workflow_dispatch
-permissions: read
+permissions: read-all
 engine: copilot
 ---
 
@@ -203,7 +203,7 @@ Test workflow content.
 		workflowContent := `---
 name: Test Workflow
 on: workflow_dispatch
-permissions: read
+permissions: read-all
 engine: copilot
 ---
 

pkg/parser/frontmatter_fuzz_test.go

@@ -29,7 +29,7 @@ title: Simple Workflow
 on:
   push:
     branches: [main]
-permissions: read
+permissions: read-all
 ---
 
 # Simple markdown`)