diff --git a/.github/workflows/ai-moderator.lock.yml b/.github/workflows/ai-moderator.lock.yml index 64ae5d2456..367ff6fdb0 100644 --- a/.github/workflows/ai-moderator.lock.yml +++ b/.github/workflows/ai-moderator.lock.yml @@ -38,7 +38,7 @@ name: "AI Moderator" # forks: "*" # Fork filtering applied via job conditions types: - opened - roles: all + # roles: all # Roles processed as role check in pre-activation job # skip-bots: # Skip-bots processed as bot check in pre-activation job # - github-actions # Skip-bots processed as bot check in pre-activation job # - copilot # Skip-bots processed as bot check in pre-activation job diff --git a/.github/workflows/poem-bot.lock.yml b/.github/workflows/poem-bot.lock.yml index fb68c83801..6b10f4629a 100644 --- a/.github/workflows/poem-bot.lock.yml +++ b/.github/workflows/poem-bot.lock.yml @@ -37,9 +37,9 @@ name: "Poem Bot - A Creative Agentic Workflow" - opened - edited - reopened - roles: - - admin - - maintainer + # roles: # Roles processed as role check in pre-activation job + # - admin # Roles processed as role check in pre-activation job + # - maintainer # Roles processed as role check in pre-activation job workflow_dispatch: inputs: poem_theme: diff --git a/.github/workflows/q.lock.yml b/.github/workflows/q.lock.yml index 92594ae275..c011eaba0a 100644 --- a/.github/workflows/q.lock.yml +++ b/.github/workflows/q.lock.yml @@ -57,10 +57,10 @@ name: "Q" types: - created - edited - roles: - - admin - - maintainer - - write + # roles: # Roles processed as role check in pre-activation job + # - admin # Roles processed as role check in pre-activation job + # - maintainer # Roles processed as role check in pre-activation job + # - write # Roles processed as role check in pre-activation job permissions: {} diff --git a/.github/workflows/release.lock.yml b/.github/workflows/release.lock.yml index 1f6ce71797..1174b89401 100644 --- a/.github/workflows/release.lock.yml +++ b/.github/workflows/release.lock.yml @@ -31,9 +31,9 @@ name: "Release" "on": - roles: - - admin - - maintainer + # roles: # Roles processed as role check in pre-activation job + # - admin # Roles processed as role check in pre-activation job + # - maintainer # Roles processed as role check in pre-activation job workflow_dispatch: inputs: release_type: diff --git a/.github/workflows/scout.lock.yml b/.github/workflows/scout.lock.yml index 482ef70fe2..fac3a01fd9 100644 --- a/.github/workflows/scout.lock.yml +++ b/.github/workflows/scout.lock.yml @@ -64,10 +64,10 @@ name: "Scout" types: - created - edited - roles: - - admin - - maintainer - - write + # roles: # Roles processed as role check in pre-activation job + # - admin # Roles processed as role check in pre-activation job + # - maintainer # Roles processed as role check in pre-activation job + # - write # Roles processed as role check in pre-activation job workflow_dispatch: inputs: topic: diff --git a/pkg/cli/codemod_bots.go b/pkg/cli/codemod_bots.go new file mode 100644 index 0000000000..9a7ce5dd26 --- /dev/null +++ b/pkg/cli/codemod_bots.go @@ -0,0 +1,209 @@ +package cli + +import ( + "fmt" + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var botsCodemodLog = logger.New("cli:codemod_bots") + +// getBotsToOnBotsCodemod creates a codemod for moving top-level 'bots' to 'on.bots' +func getBotsToOnBotsCodemod() Codemod { + return Codemod{ + ID: "bots-to-on-bots", + Name: "Move bots to on.bots", + Description: "Moves the top-level 'bots' field to 'on.bots' as per the new frontmatter structure", + IntroducedIn: "0.10.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + // Check if top-level bots exists + _, hasTopLevelBots := frontmatter["bots"] + if !hasTopLevelBots { + return content, false, nil + } + + // Check if on.bots already exists (shouldn't happen, but be safe) + if onValue, hasOn := frontmatter["on"]; hasOn { + if onMap, ok := onValue.(map[string]any); ok { + if _, hasOnBots := onMap["bots"]; hasOnBots { + botsCodemodLog.Print("Both top-level 'bots' and 'on.bots' exist - skipping migration") + return content, false, nil + } + } + } + + // Parse frontmatter to get raw lines + frontmatterLines, markdown, err := parseFrontmatterLines(content) + if err != nil { + return content, false, err + } + + // Find bots line and on: block + var botsLineIdx = -1 + var botsLineValue string + var onBlockIdx = -1 + var onIndent string + + // First pass: find the bots line and on: block + for i, line := range frontmatterLines { + trimmedLine := strings.TrimSpace(line) + + // Find top-level bots + if isTopLevelKey(line) && strings.HasPrefix(trimmedLine, "bots:") { + botsLineIdx = i + // Extract the value (could be on same line or on next lines) + parts := strings.SplitN(line, ":", 2) + if len(parts) == 2 { + botsLineValue = strings.TrimSpace(parts[1]) + } + botsCodemodLog.Printf("Found top-level bots at line %d", i+1) + } + + // Find on: block + if isTopLevelKey(line) && strings.HasPrefix(trimmedLine, "on:") { + onBlockIdx = i + onIndent = getIndentation(line) + botsCodemodLog.Printf("Found 'on:' block at line %d", i+1) + } + } + + // If no bots found, nothing to do + if botsLineIdx == -1 { + return content, false, nil + } + + // Determine how bots is formatted + var botsLines []string + var botsEndIdx int + + if strings.HasPrefix(botsLineValue, "[") { + // bots: [dependabot, renovate] - single line format + botsLines = []string{frontmatterLines[botsLineIdx]} + botsEndIdx = botsLineIdx + } else { + // Multi-line array format OR bots: with empty value + // Find all lines that are part of the bots block + botsStartIndent := getIndentation(frontmatterLines[botsLineIdx]) + botsLines = append(botsLines, frontmatterLines[botsLineIdx]) + botsEndIdx = botsLineIdx + + for j := botsLineIdx + 1; j < len(frontmatterLines); j++ { + line := frontmatterLines[j] + trimmed := strings.TrimSpace(line) + + // Empty lines or comments might be part of the block + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + botsLines = append(botsLines, line) + botsEndIdx = j + continue + } + + // Check if still in the bots block (indented more than bots:) + if isNestedUnder(line, botsStartIndent) { + botsLines = append(botsLines, line) + botsEndIdx = j + } else { + // Exited the block + break + } + } + } + + botsCodemodLog.Printf("Bots spans lines %d to %d (%d lines)", botsLineIdx+1, botsEndIdx+1, len(botsLines)) + + // If no on: block found, we need to create one + result := make([]string, 0, len(frontmatterLines)) + modified := false + + if onBlockIdx == -1 { + // No on: block exists - create one with bots inside it + botsCodemodLog.Print("No 'on:' block found - creating new one with bots") + + for i, line := range frontmatterLines { + if i >= botsLineIdx && i <= botsEndIdx { + // Skip the original bots lines - we'll add them to the new on: block + if i == botsLineIdx { + // Add new on: block with bots inside + result = append(result, "on:") + // Add bots lines with proper indentation + for _, botsLine := range botsLines { + trimmed := strings.TrimSpace(botsLine) + if trimmed == "" { + result = append(result, botsLine) + } else if strings.HasPrefix(trimmed, "bots:") { + // bots: line gets 2 spaces (nested under on:) + result = append(result, " "+botsLine) + } else { + // Array items get 4 spaces (nested under on: and bots:) + result = append(result, " "+trimmed) + } + } + modified = true + } + // Skip all other bots lines + continue + } + result = append(result, line) + } + } else { + // on: block exists - add bots to it + botsCodemodLog.Print("Found 'on:' block - adding bots to it") + + // Determine indentation for items inside on: block + onItemIndent := onIndent + " " + + // Track if we've inserted bots + insertedBots := false + + for i, line := range frontmatterLines { + // Skip the original bots lines + if i >= botsLineIdx && i <= botsEndIdx { + modified = true + continue + } + + // Add the line + result = append(result, line) + + // After the on: line, insert bots + if i == onBlockIdx && !insertedBots { + // Add bots lines with proper indentation inside on: block + for _, botsLine := range botsLines { + trimmed := strings.TrimSpace(botsLine) + if trimmed == "" { + result = append(result, botsLine) + } else { + // Adjust indentation to be nested under on: + // Remove "bots:" prefix and re-add with proper indentation + if strings.HasPrefix(trimmed, "bots:") { + // bots: value or bots: + parts := strings.SplitN(trimmed, ":", 2) + if len(parts) == 2 { + result = append(result, fmt.Sprintf("%sbots:%s", onItemIndent, parts[1])) + } else { + result = append(result, fmt.Sprintf("%sbots:", onItemIndent)) + } + } else { + // Array item line (e.g., "- dependabot") + // These should be indented 2 more spaces than bots: to be nested under it + result = append(result, onItemIndent+" "+trimmed) + } + } + } + insertedBots = true + } + } + } + + if !modified { + return content, false, nil + } + + // Reconstruct the content + newContent := reconstructContent(result, markdown) + botsCodemodLog.Print("Successfully migrated top-level 'bots' to 'on.bots'") + return newContent, true, nil + }, + } +} diff --git a/pkg/cli/codemod_bots_test.go b/pkg/cli/codemod_bots_test.go new file mode 100644 index 0000000000..cc6a575f5a --- /dev/null +++ b/pkg/cli/codemod_bots_test.go @@ -0,0 +1,193 @@ +//go:build !integration + +package cli + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetBotsToOnBotsCodemod(t *testing.T) { + codemod := getBotsToOnBotsCodemod() + + assert.Equal(t, "bots-to-on-bots", codemod.ID) + assert.Equal(t, "Move bots to on.bots", codemod.Name) + assert.NotEmpty(t, codemod.Description) + assert.Equal(t, "0.10.0", codemod.IntroducedIn) + require.NotNil(t, codemod.Apply) +} + +func TestBotsToOnBotsCodemod_SingleLineArray(t *testing.T) { + codemod := getBotsToOnBotsCodemod() + + content := `--- +on: + issues: + types: [opened] +bots: [dependabot, renovate] +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": map[string]any{ + "issues": map[string]any{ + "types": []any{"opened"}, + }, + }, + "bots": []any{"dependabot", "renovate"}, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "on:") + assert.Contains(t, result, "bots: [dependabot, renovate]") + assert.NotContains(t, result, "\nbots: [dependabot, renovate]") + // Ensure bots is nested under on: + lines := strings.Split(result, "\n") + foundOn := false + foundBots := false + for _, line := range lines { + if line == "on:" { + foundOn = true + } + if foundOn && strings.Contains(line, "bots:") { + foundBots = true + // Check that bots line has indentation (nested under on:) + assert.Greater(t, len(line), len(strings.TrimSpace(line)), "bots should be indented under on:") + break + } + } + assert.True(t, foundBots, "Should find bots nested under on:") +} + +func TestBotsToOnBotsCodemod_MultiLineArray(t *testing.T) { + codemod := getBotsToOnBotsCodemod() + + content := `--- +on: + issues: + types: [opened] +bots: + - dependabot + - renovate + - github-actions +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": map[string]any{ + "issues": map[string]any{ + "types": []any{"opened"}, + }, + }, + "bots": []any{"dependabot", "renovate", "github-actions"}, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "on:") + assert.Contains(t, result, "bots:") + assert.Contains(t, result, "- dependabot") + assert.Contains(t, result, "- renovate") + assert.Contains(t, result, "- github-actions") +} + +func TestBotsToOnBotsCodemod_NoOnBlock(t *testing.T) { + codemod := getBotsToOnBotsCodemod() + + content := `--- +bots: [dependabot, renovate] +engine: copilot +--- + +# Test workflow` + + frontmatter := map[string]any{ + "bots": []any{"dependabot", "renovate"}, + "engine": "copilot", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "on:") + assert.Contains(t, result, "bots: [dependabot, renovate]") + // Ensure on: block is created + lines := strings.Split(result, "\n") + foundOn := false + for _, line := range lines { + if line == "on:" { + foundOn = true + break + } + } + assert.True(t, foundOn, "Should create new on: block") +} + +func TestBotsToOnBotsCodemod_NoChange_NoBots(t *testing.T) { + codemod := getBotsToOnBotsCodemod() + + content := `--- +on: + issues: + types: [opened] +engine: copilot +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": map[string]any{ + "issues": map[string]any{ + "types": []any{"opened"}, + }, + }, + "engine": "copilot", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestBotsToOnBotsCodemod_NoChange_OnBotsExists(t *testing.T) { + codemod := getBotsToOnBotsCodemod() + + content := `--- +on: + issues: + types: [opened] + bots: [dependabot, renovate] +bots: [dependabot, renovate, github-actions] +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": map[string]any{ + "issues": map[string]any{ + "types": []any{"opened"}, + }, + "bots": []any{"dependabot", "renovate"}, + }, + "bots": []any{"dependabot", "renovate", "github-actions"}, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 0a0d8147f4..47c48f95f8 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -37,5 +37,6 @@ func GetAllCodemods() []Codemod { getBashAnonymousRemovalCodemod(), // Replace bash: with bash: false getActivationOutputsCodemod(), // Transform needs.activation.outputs.* to steps.sanitized.outputs.* getRolesToOnRolesCodemod(), // Move top-level roles to on.roles + getBotsToOnBotsCodemod(), // Move top-level bots to on.bots } } diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index af3ee6c542..c2f6168f01 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -1401,6 +1401,15 @@ } ] }, + "bots": { + "type": "array", + "description": "Allow list of bot identifiers that can trigger the workflow even if they don't meet the required role permissions. When the actor is in this list, the bot must be active (installed) on the repository to trigger the workflow.", + "items": { + "type": "string", + "minLength": 1, + "description": "Bot identifier/name (e.g., 'dependabot[bot]', 'renovate[bot]', 'github-actions[bot]')" + } + }, "manual-approval": { "type": "string", "description": "Environment name that requires manual approval before the workflow can run. Must match a valid environment configured in the repository settings." diff --git a/pkg/workflow/bots_test.go b/pkg/workflow/bots_test.go index acc6558e35..acf2978177 100644 --- a/pkg/workflow/bots_test.go +++ b/pkg/workflow/bots_test.go @@ -29,7 +29,7 @@ func TestBotsFieldExtraction(t *testing.T) { on: issues: types: [opened] -bots: ["dependabot[bot]", "renovate[bot]"] + bots: ["dependabot[bot]", "renovate[bot]"] --- # Test Workflow @@ -43,7 +43,7 @@ Test workflow content.`, on: pull_request: types: [opened] -bots: ["github-actions[bot]"] + bots: ["github-actions[bot]"] --- # Test Workflow @@ -109,8 +109,8 @@ func TestBotsEnvironmentVariableGeneration(t *testing.T) { on: issues: types: [opened] -roles: [triage] -bots: ["dependabot[bot]", "renovate[bot]"] + roles: [triage] + bots: ["dependabot[bot]", "renovate[bot]"] --- # Test Workflow with Bots @@ -158,7 +158,7 @@ func TestBotsWithDefaultRoles(t *testing.T) { on: pull_request: types: [opened] -bots: ["dependabot[bot]"] + bots: ["dependabot[bot]"] --- # Test Workflow @@ -207,7 +207,7 @@ on: issues: types: [opened] roles: all -bots: ["dependabot[bot]"] + bots: ["dependabot[bot]"] --- # Test Workflow diff --git a/pkg/workflow/compiler_orchestrator_test.go b/pkg/workflow/compiler_orchestrator_test.go index 9a82fc3fd4..893e84adcd 100644 --- a/pkg/workflow/compiler_orchestrator_test.go +++ b/pkg/workflow/compiler_orchestrator_test.go @@ -608,12 +608,13 @@ func TestExtractAdditionalConfigurations(t *testing.T) { tmpDir := testutil.TempDir(t, "additional-configs") testContent := `--- -on: push +on: + push: null + roles: + - admin + bots: + - copilot engine: copilot -roles: - - admin -bots: - - copilot --- # Test Workflow diff --git a/pkg/workflow/compiler_orchestrator_workflow_test.go b/pkg/workflow/compiler_orchestrator_workflow_test.go index 866a8a7028..ea3fbfcba2 100644 --- a/pkg/workflow/compiler_orchestrator_workflow_test.go +++ b/pkg/workflow/compiler_orchestrator_workflow_test.go @@ -1098,46 +1098,13 @@ on: pull_request: types: [opened, synchronize] draft: false + roles: + - admin + - maintainer + bots: + - copilot + - dependabot engine: copilot -name: Complete Workflow -description: Test all sections -source: complete-test -timeout-minutes: 60 -strict: false -features: - dangerous-permissions-write: true -permissions: - contents: read - issues: write - pull-requests: write -network: - allowed: - - github.com - - api.example.com -concurrency: - group: ci-${{ github.ref }} - cancel-in-progress: true -run-name: Test Run ${{ github.run_id }} -env: - NODE_ENV: production - DEBUG: "true" -if: github.event_name == 'push' -runs-on: ubuntu-latest -environment: production -container: node:18 -cache: - - key: ${{ runner.os }}-node - path: node_modules -tools: - bash: ["echo", "ls", "cat"] - github: - allowed: [list_issues, create_issue] -roles: - - admin - - maintainer -bots: - - copilot - - dependabot steps: - name: Custom step run: echo "test" diff --git a/pkg/workflow/frontmatter_extraction_yaml.go b/pkg/workflow/frontmatter_extraction_yaml.go index 50dd76c69a..30a9ed6989 100644 --- a/pkg/workflow/frontmatter_extraction_yaml.go +++ b/pkg/workflow/frontmatter_extraction_yaml.go @@ -157,6 +157,8 @@ func (c *Compiler) commentOutProcessedFieldsInOnSection(yamlStr string, frontmat inSkipIfNoMatch := false inSkipRolesArray := false inSkipBotsArray := false + inRolesArray := false + inBotsArray := false currentSection := "" // Track which section we're in ("issues", "pull_request", "discussion", or "issue_comment") for _, line := range lines { @@ -238,6 +240,18 @@ func (c *Compiler) commentOutProcessedFieldsInOnSection(yamlStr string, frontmat inSkipBotsArray = true } + // Check if we're entering roles field + if !inPullRequest && !inIssues && !inDiscussion && !inIssueComment && strings.HasPrefix(trimmedLine, "roles:") { + // Check if this is an array (next line will be "- ") or inline value + inRolesArray = true + } + + // Check if we're entering bots array + if !inPullRequest && !inIssues && !inDiscussion && !inIssueComment && strings.HasPrefix(trimmedLine, "bots:") { + // Check if this is an array (next line will be "- ") or inline value + inBotsArray = true + } + // Check if we're entering skip-if-match object if !inPullRequest && !inIssues && !inDiscussion && !inIssueComment && !inSkipIfMatch { // Check both uncommented and commented forms @@ -315,6 +329,28 @@ func (c *Compiler) commentOutProcessedFieldsInOnSection(yamlStr string, frontmat } } + // Check if we're leaving the roles array by encountering another top-level field + if inRolesArray && strings.TrimSpace(line) != "" { + // Get the indentation of the current line + lineIndent := len(line) - len(strings.TrimLeft(line, " \t")) + + // If this is a non-dash line at the same level as roles (2 spaces), we're out of the array + if lineIndent == 2 && !strings.HasPrefix(trimmedLine, "-") && !strings.HasPrefix(trimmedLine, "roles:") && !strings.HasPrefix(trimmedLine, "#") { + inRolesArray = false + } + } + + // Check if we're leaving the bots array by encountering another top-level field + if inBotsArray && strings.TrimSpace(line) != "" { + // Get the indentation of the current line + lineIndent := len(line) - len(strings.TrimLeft(line, " \t")) + + // If this is a non-dash line at the same level as bots (2 spaces), we're out of the array + if lineIndent == 2 && !strings.HasPrefix(trimmedLine, "-") && !strings.HasPrefix(trimmedLine, "bots:") && !strings.HasPrefix(trimmedLine, "#") { + inBotsArray = false + } + } + // Determine if we should comment out this line shouldComment := false var commentReason string @@ -355,6 +391,20 @@ func (c *Compiler) commentOutProcessedFieldsInOnSection(yamlStr string, frontmat // Comment out array items in skip-bots shouldComment = true commentReason = " # Skip-bots processed as bot check in pre-activation job" + } else if strings.HasPrefix(trimmedLine, "roles:") { + shouldComment = true + commentReason = " # Roles processed as role check in pre-activation job" + } else if inRolesArray && strings.HasPrefix(trimmedLine, "-") { + // Comment out array items in roles + shouldComment = true + commentReason = " # Roles processed as role check in pre-activation job" + } else if strings.HasPrefix(trimmedLine, "bots:") { + shouldComment = true + commentReason = " # Bots processed as bot check in pre-activation job" + } else if inBotsArray && strings.HasPrefix(trimmedLine, "-") { + // Comment out array items in bots + shouldComment = true + commentReason = " # Bots processed as bot check in pre-activation job" } else if strings.HasPrefix(trimmedLine, "reaction:") { shouldComment = true commentReason = " # Reaction processed as activation job step" diff --git a/pkg/workflow/frontmatter_types.go b/pkg/workflow/frontmatter_types.go index d5e75baaf1..41ed80ed57 100644 --- a/pkg/workflow/frontmatter_types.go +++ b/pkg/workflow/frontmatter_types.go @@ -151,9 +151,7 @@ type FrontmatterConfig struct { Metadata map[string]string `json:"metadata,omitempty"` // Custom metadata key-value pairs SecretMasking *SecretMaskingConfig `json:"secret-masking,omitempty"` - // Command/bot configuration - Roles []string `json:"roles,omitempty"` - Bots []string `json:"bots,omitempty"` + // Rate limiting configuration RateLimit *RateLimitConfig `json:"rate-limit,omitempty"` } @@ -667,12 +665,6 @@ func (fc *FrontmatterConfig) ToMap() map[string]any { if fc.SecretMasking != nil { result["secret-masking"] = fc.SecretMasking } - if fc.Roles != nil { - result["roles"] = fc.Roles - } - if fc.Bots != nil { - result["bots"] = fc.Bots - } return result } diff --git a/pkg/workflow/permission_restriction_test.go b/pkg/workflow/permission_restriction_test.go index c8550a025b..7527bcb2e3 100644 --- a/pkg/workflow/permission_restriction_test.go +++ b/pkg/workflow/permission_restriction_test.go @@ -67,7 +67,7 @@ Test workflow content.`, on: push: branches: [main] -roles: all + roles: all tools: github: allowed: [list_issues] @@ -85,7 +85,7 @@ Test workflow content.`, on: pull_request: types: [opened] -roles: [admin, maintainer, write] + roles: [admin, maintainer, write] tools: github: allowed: [list_issues] @@ -101,8 +101,8 @@ Test workflow content.`, name: "workflow with workflow_dispatch only should NOT include permission check (safe event)", frontmatter: `--- on: - workflow_dispatch: -roles: [admin, maintainer, write] + workflow_dispatch: null + roles: [admin, maintainer, write] tools: github: allowed: [list_issues] @@ -118,8 +118,8 @@ Test workflow content.`, name: "workflow with workflow_dispatch without write role should include permission check", frontmatter: `--- on: - workflow_dispatch: -roles: [admin, maintainer] + workflow_dispatch: null + roles: [admin, maintainer] tools: github: allowed: [list_issues] @@ -190,8 +190,8 @@ Test workflow content.`, on: command: name: scout - workflow_dispatch: -roles: [admin, maintainer, write] + workflow_dispatch: null + roles: [admin, maintainer, write] tools: github: allowed: [list_issues] diff --git a/pkg/workflow/pre_activation_custom_fields_test.go b/pkg/workflow/pre_activation_custom_fields_test.go index 437eb8c32f..8a0857dec6 100644 --- a/pkg/workflow/pre_activation_custom_fields_test.go +++ b/pkg/workflow/pre_activation_custom_fields_test.go @@ -22,10 +22,10 @@ func TestPreActivationCustomSteps(t *testing.T) { t.Run("custom_steps_imported", func(t *testing.T) { workflowContent := `--- on: - workflow_dispatch: + workflow_dispatch: null stop-after: "+48h" + roles: [admin, maintainer] engine: claude -roles: [admin, maintainer] jobs: pre-activation: steps: @@ -74,10 +74,10 @@ Test workflow with custom pre-activation steps t.Run("custom_outputs_imported", func(t *testing.T) { workflowContent := `--- on: - workflow_dispatch: + workflow_dispatch: null stop-after: "+48h" + roles: [admin, maintainer] engine: claude -roles: [admin, maintainer] jobs: pre-activation: outputs: @@ -121,8 +121,8 @@ Test workflow with custom pre-activation outputs on: workflow_dispatch: stop-after: "+48h" + roles: [admin, maintainer] engine: claude -roles: [admin, maintainer] jobs: pre-activation: steps: @@ -168,8 +168,8 @@ Test workflow with both custom steps and outputs on: workflow_dispatch: stop-after: "+48h" + roles: [admin, maintainer] engine: claude -roles: [admin, maintainer] jobs: pre-activation: runs-on: ubuntu-latest @@ -201,8 +201,8 @@ Test workflow with unsupported field in pre-activation on: workflow_dispatch: stop-after: "+48h" + roles: [admin, maintainer] engine: claude -roles: [admin, maintainer] jobs: pre-activation: steps: @@ -255,8 +255,8 @@ Test that pre-activation is not added as a custom job on: workflow_dispatch: stop-after: "+48h" + roles: [admin, maintainer] engine: claude -roles: [admin, maintainer] jobs: pre-activation: steps: diff --git a/pkg/workflow/processing_benchmark_test.go b/pkg/workflow/processing_benchmark_test.go index e4f3a4d7c4..bd446bc9a8 100644 --- a/pkg/workflow/processing_benchmark_test.go +++ b/pkg/workflow/processing_benchmark_test.go @@ -288,8 +288,9 @@ func BenchmarkProcessRoles(b *testing.B) { defer os.RemoveAll(tmpDir) testContent := `--- -on: issues -roles: [admin, maintainer, write, read] +on: + issues: null + roles: [admin, maintainer, write, read] permissions: contents: read issues: write diff --git a/pkg/workflow/role_checks.go b/pkg/workflow/role_checks.go index a326247072..c04ba16e3a 100644 --- a/pkg/workflow/role_checks.go +++ b/pkg/workflow/role_checks.go @@ -139,33 +139,48 @@ func parseRolesValue(rolesValue any, fieldName string) []string { // extractBots extracts the 'bots' field from frontmatter to determine allowed bot identifiers func (c *Compiler) extractBots(frontmatter map[string]any) []string { - if botsValue, exists := frontmatter["bots"]; exists { - switch v := botsValue.(type) { - case []any: - // Array of bot identifiers - var bots []string - for _, item := range v { - if str, ok := item.(string); ok { - bots = append(bots, str) + // Check on.bots + if onValue, exists := frontmatter["on"]; exists { + if onMap, ok := onValue.(map[string]any); ok { + if botsValue, hasBots := onMap["bots"]; hasBots { + bots := parseBotsValue(botsValue, "on.bots") + if bots != nil { + return bots } } - roleLog.Printf("Extracted %d bot identifiers from array: %v", len(bots), bots) - return bots - case []string: - // Already a string slice - roleLog.Printf("Extracted %d bot identifiers: %v", len(v), v) - return v - case string: - // Single bot identifier as string - roleLog.Printf("Extracted single bot identifier: %s", v) - return []string{v} } } + // No bots specified, return empty array roleLog.Print("No bots specified") return []string{} } +// parseBotsValue parses a bots value from frontmatter (supports string, []any, []string) +func parseBotsValue(botsValue any, fieldName string) []string { + switch v := botsValue.(type) { + case []any: + // Array of bot identifiers + var bots []string + for _, item := range v { + if str, ok := item.(string); ok { + bots = append(bots, str) + } + } + roleLog.Printf("Extracted %d bot identifiers from '%s' array: %v", len(bots), fieldName, bots) + return bots + case []string: + // Already a string slice + roleLog.Printf("Extracted %d bot identifiers from '%s': %v", len(v), fieldName, v) + return v + case string: + // Single bot identifier as string + roleLog.Printf("Extracted single bot identifier from '%s': %s", fieldName, v) + return []string{v} + } + return nil +} + // extractRateLimitConfig extracts the 'rate-limit' field from frontmatter func (c *Compiler) extractRateLimitConfig(frontmatter map[string]any) *RateLimitConfig { if rateLimitValue, exists := frontmatter["rate-limit"]; exists && rateLimitValue != nil { diff --git a/pkg/workflow/role_checks_test.go b/pkg/workflow/role_checks_test.go index b70dc6ecfb..f78264989b 100644 --- a/pkg/workflow/role_checks_test.go +++ b/pkg/workflow/role_checks_test.go @@ -115,7 +115,7 @@ on: pull_request: types: [opened] roles: [write] -bots: ["dependabot[bot]"] + bots: ["dependabot[bot]"] --- # Test Workflow diff --git a/pkg/workflow/skip_bots_test.go b/pkg/workflow/skip_bots_test.go index 607bc2d7dd..744d79bcb9 100644 --- a/pkg/workflow/skip_bots_test.go +++ b/pkg/workflow/skip_bots_test.go @@ -133,7 +133,7 @@ on: issues: types: [opened] skip-bots: [user1, user2] -roles: [maintainer] + roles: [maintainer] engine: copilot --- diff --git a/pkg/workflow/skip_if_match_test.go b/pkg/workflow/skip_if_match_test.go index 5d5ffc1bdf..51a679cfa5 100644 --- a/pkg/workflow/skip_if_match_test.go +++ b/pkg/workflow/skip_if_match_test.go @@ -87,10 +87,10 @@ This workflow has a skip-if-match configuration. t.Run("pre_activation_job_with_multiple_checks", func(t *testing.T) { workflowContent := `--- on: - workflow_dispatch: + workflow_dispatch: null stop-after: "+48h" skip-if-match: "is:pr is:open" -roles: [admin, maintainer] + roles: [admin, maintainer] engine: claude --- diff --git a/pkg/workflow/skip_if_no_match_test.go b/pkg/workflow/skip_if_no_match_test.go index d5a50bf092..a51657d1fc 100644 --- a/pkg/workflow/skip_if_no_match_test.go +++ b/pkg/workflow/skip_if_no_match_test.go @@ -92,10 +92,10 @@ This workflow has a skip-if-no-match configuration. t.Run("pre_activation_job_with_multiple_checks", func(t *testing.T) { workflowContent := `--- on: - workflow_dispatch: + workflow_dispatch: null stop-after: "+48h" skip-if-no-match: "is:pr is:open label:urgent" -roles: [admin, maintainer] + roles: [admin, maintainer] engine: claude --- diff --git a/pkg/workflow/stop_time_check_job_test.go b/pkg/workflow/stop_time_check_job_test.go index 312b3a560f..db61f8c6cd 100644 --- a/pkg/workflow/stop_time_check_job_test.go +++ b/pkg/workflow/stop_time_check_job_test.go @@ -22,9 +22,9 @@ func TestPreActivationJob(t *testing.T) { t.Run("pre_activation_job_created_with_stop_after", func(t *testing.T) { workflowContent := `--- on: - workflow_dispatch: + workflow_dispatch: null stop-after: "+48h" -roles: [admin, maintainer] + roles: [admin, maintainer] engine: claude --- @@ -105,9 +105,9 @@ This workflow has a stop-after configuration. t.Run("no_pre_activation_job_without_stop_after_or_roles", func(t *testing.T) { workflowContent := `--- on: - workflow_dispatch: + workflow_dispatch: null + roles: all engine: claude -roles: all --- # Normal Workflow diff --git a/pkg/workflow/task_job_generation_fix_test.go b/pkg/workflow/task_job_generation_fix_test.go index e54493d877..3a4936d82c 100644 --- a/pkg/workflow/task_job_generation_fix_test.go +++ b/pkg/workflow/task_job_generation_fix_test.go @@ -26,8 +26,8 @@ func TestTaskJobGenerationFix(t *testing.T) { // 2. Even with safe events and roles: all, we still want the timestamp check workflowContent := `--- on: - workflow_dispatch: -roles: all + workflow_dispatch: null + roles: all --- # Simple Workflow