diff --git a/docs/adr/57968-import-bot-allowlists-from-on-bots.md b/docs/adr/57968-import-bot-allowlists-from-on-bots.md new file mode 100644 index 00000000000..e24685a3efa --- /dev/null +++ b/docs/adr/57968-import-bot-allowlists-from-on-bots.md @@ -0,0 +1,50 @@ +# ADR-57968: Import Bot Allowlists from `on.bots` + +**Date**: 2026-09-02 +**Status**: Draft +**Deciders**: pelikhan, adr-writer agent + +--- + +### Context + +This pull request fixes how shared workflows contribute bot allowlists during import processing. The PR description states that imports incorrectly read `bots` from the top level even though the supported frontmatter structure now stores bot allowlists under `on.bots`. The diff updates the import field extractor to read `bots` from the `on` section, updates schema coverage to reject unsupported top-level `bots`, and revises workflow and codemod tests around imported-only, merged, and overlapping allowlists. Because this changes how imported workflow activation metadata is interpreted and migrated, the behavior should be documented explicitly. + +### Decision + +We will treat `on.bots` as the canonical location for bot allowlists in shared and main workflows, and imported workflows will contribute their bot allowlists by reading from that nested field. We will merge imported and importing workflow allowlists by preserving first-seen order and removing duplicates. We will also update the legacy codemod so that when both top-level `bots` and `on.bots` exist, it consolidates them into a single `on.bots` entry instead of leaving conflicting representations. + +### Alternatives Considered + +#### Alternative 1: Continue Reading Imported Bots from Top-Level `bots` + +Keep the existing importer behavior and continue extracting bot allowlists from a top-level `bots` field. + +This was considered because it would avoid changing the importer and keep older expectations intact. It was not chosen because the PR evidence shows top-level `bots` is no longer a supported frontmatter field, so continuing to read it from imports would preserve incorrect behavior and make shared workflows inconsistent with the validated schema. + +#### Alternative 2: Require Main Workflows to Duplicate Imported Bot Allowlists Manually + +Do not merge bot allowlists from imported workflows and instead require each importing workflow to restate every allowed bot in its own frontmatter. + +This was considered because it reduces implicit behavior in the importer. It was not chosen because the regression tests in this PR explicitly cover imported-only and combined allowlists, showing that shared workflows are expected to contribute bot activation metadata and that manual duplication would be repetitive and error-prone. + +### Consequences + +#### Positive +- Imported shared workflows now contribute bot allowlists from the supported `on.bots` field, matching the documented schema. +- Combined bot allowlists are deterministic because merge order is preserved and duplicates are removed. +- Legacy workflow migration becomes safer because the codemod consolidates dual representations into one canonical `on.bots` field. + +#### Negative +- Import behavior now depends on nested-field extraction, which adds some implementation complexity in parser and codemod logic. +- Workflows or tests that still assume top-level `bots` is accepted must be updated to the canonical nested form. +- The codemod emits JSON-style inline bot arrays when merging, which may alter formatting compared with the original YAML layout. + +#### Neutral +- Regression coverage now focuses on imported-only, merged, and overlapping allowlist cases. +- Schema validation and import extraction become more tightly coupled around the canonical `on.bots` structure. +- The change affects activation metadata handling rather than the compiled workflow steps themselves. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/codemod_bots.go b/pkg/cli/codemod_bots.go index 5f8b16830cc..59267d928bb 100644 --- a/pkg/cli/codemod_bots.go +++ b/pkg/cli/codemod_bots.go @@ -1,6 +1,7 @@ package cli import ( + "encoding/json" "strings" "github.com/github/gh-aw/pkg/logger" @@ -10,7 +11,7 @@ var botsCodemodLog = logger.New("cli:codemod_bots") // getBotsToOnBotsCodemod creates a codemod for moving top-level 'bots' to 'on.bots' func getBotsToOnBotsCodemod() Codemod { - return newMoveTopLevelKeyToOnBlockCodemod(moveToOnBlockConfig{ + codemod := newMoveTopLevelKeyToOnBlockCodemod(moveToOnBlockConfig{ 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", @@ -21,4 +22,262 @@ func getBotsToOnBotsCodemod() Codemod { }, Log: botsCodemodLog, }) + baseApply := codemod.Apply + codemod.Apply = func(content string, frontmatter map[string]any) (string, bool, error) { + topBots, hasTopBots := frontmatter["bots"] + onMap, hasOnMap := frontmatter["on"].(map[string]any) + onBots, hasOnBots := onMap["bots"] + if !hasTopBots || !hasOnMap || !hasOnBots { + return baseApply(content, frontmatter) + } + + mergedBots, ok := mergeLegacyBots(onBots, topBots) + if !ok { + return content, false, nil + } + return applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { + return mergeLegacyBotsLines(lines, mergedBots) + }) + } + return codemod +} + +func mergeLegacyBots(onBots, topBots any) ([]string, bool) { + merged := make([]string, 0) + seen := make(map[string]struct{}) + for _, value := range []any{onBots, topBots} { + bots, ok := value.([]any) + if !ok { + return nil, false + } + for _, botValue := range bots { + bot, ok := botValue.(string) + if !ok { + return nil, false + } + if _, exists := seen[bot]; !exists { + seen[bot] = struct{}{} + merged = append(merged, bot) + } + } + } + return merged, true +} + +func mergeLegacyBotsLines(lines []string, bots []string) ([]string, bool) { + topBotsStart, topBotsEnd := findBotsBlock(lines, 0, len(lines), 0, false) + onStart := -1 + for i, line := range lines { + if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "on:") { + onStart = i + break + } + } + if topBotsStart == -1 || onStart == -1 { + return lines, false + } + + onEnd := len(lines) + for i := onStart + 1; i < len(lines); i++ { + if isTopLevelKey(lines[i]) { + onEnd = i + break + } + } + onBotsStart, onBotsEnd := findBotsBlock(lines, onStart+1, onEnd, len(getIndentation(lines[onStart])), true) + if onBotsStart == -1 { + return lines, false + } + onBotsIndent := getIndentation(lines[onBotsStart]) + topBlock := lines[topBotsStart : topBotsEnd+1] + onBlock := lines[onBotsStart : onBotsEnd+1] + comments := append(collectComments(lines, topBotsStart, topBotsEnd), collectComments(lines, onBotsStart, onBotsEnd)...) + itemComments := mergeBotItemComments(topBlock, onBlock) + isInline := isInlineBotsValue(lines[onBotsStart]) && len(itemComments) == 0 && len(comments) == 0 + declaration := buildBotsDeclaration(onBotsIndent, bots, itemComments, lines[onBotsStart], isInline) + if len(comments) > 0 { + declaration = append(append([]string{}, comments...), declaration...) + } + + result := make([]string, 0, len(lines)) + for i, line := range lines { + if (i >= topBotsStart && i <= topBotsEnd) || (i >= onBotsStart && i <= onBotsEnd) { + continue + } + result = append(result, line) + if i == onStart { + result = append(result, declaration...) + } + } + return result, true +} + +func buildBotsDeclaration(indent string, bots []string, itemComments map[string]string, originalLine string, inline bool) []string { + if inline { + encodedBots, err := json.Marshal(bots) + if err != nil { + return nil + } + line := indent + "bots: " + string(encodedBots) + if comment := trailingComment(originalLine); comment != "" { + line += " " + comment + } + return []string{line} + } + out := []string{indent + "bots:"} + for _, bot := range bots { + line := indent + " - " + bot + if comment, ok := itemComments[bot]; ok && comment != "" { + line += " " + comment + } + out = append(out, line) + } + return out +} + +func trailingComment(line string) string { + trimmed := strings.TrimSpace(line) + inSingleQuoted := false + inDoubleQuoted := false + for i := range trimmed { + ch := trimmed[i] + switch ch { + case '\'': + if !inDoubleQuoted { + inSingleQuoted = !inSingleQuoted + } + case '"': + if !inSingleQuoted { + inDoubleQuoted = !inDoubleQuoted + } + case '#': + if !inSingleQuoted && !inDoubleQuoted { + return strings.TrimSpace(trimmed[i:]) + } + } + } + return "" +} + +func collectComments(lines []string, start, end int) []string { + comments := make([]string, 0) + for i := start; i <= end; i++ { + if i >= len(lines) { + break + } + trimmed := strings.TrimSpace(lines[i]) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + comments = append(comments, lines[i]) + } + } + return comments +} + +func mergeBotItemComments(blocks ...[]string) map[string]string { + commentsByBot := make(map[string]string) + for _, block := range blocks { + for i := range block { + trimmed := strings.TrimSpace(block[i]) + if strings.HasPrefix(trimmed, "- ") { + entry := strings.TrimSpace(strings.TrimPrefix(trimmed, "-")) + if entry == "" { + continue + } + key, comment := splitBotEntry(entry) + if key != "" { + if _, ok := commentsByBot[key]; !ok && comment != "" { + commentsByBot[key] = comment + } + } + } + } + } + return commentsByBot +} + +func splitBotEntry(entry string) (string, string) { + trimmed := strings.TrimSpace(entry) + inSingleQuoted := false + inDoubleQuoted := false + for i := range trimmed { + ch := trimmed[i] + switch ch { + case '\'': + if !inDoubleQuoted { + inSingleQuoted = !inSingleQuoted + } + case '"': + if !inSingleQuoted { + inDoubleQuoted = !inDoubleQuoted + } + case '#': + if !inSingleQuoted && !inDoubleQuoted { + return strings.TrimSpace(trimmed[:i]), strings.TrimSpace(trimmed[i:]) + } + } + } + return trimmed, "" +} + +func isInlineBotsValue(line string) bool { + trimmed := strings.TrimSpace(line) + return strings.HasPrefix(trimmed, "bots:") && !strings.Contains(trimmed, "\n") && !strings.Contains(trimmed, "- ") +} + +func findBotsBlock(lines []string, start, end, indent int, nested bool) (int, int) { + var directChildIndent = -1 + for i := start; i < end; i++ { + line := lines[i] + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + lineIndent := len(getIndentation(line)) + if nested { + if lineIndent <= indent { + continue + } + if directChildIndent == -1 { + directChildIndent = lineIndent + } + if lineIndent > directChildIndent { + continue + } + if lineIndent != directChildIndent { + continue + } + } else if lineIndent != indent { + continue + } + if !strings.HasPrefix(trimmed, "bots:") { + continue + } + blockEnd := i + for j := i + 1; j < end; j++ { + next := strings.TrimSpace(lines[j]) + if next == "" || strings.HasPrefix(next, "#") { + blockEnd = j + continue + } + nextIndent := len(getIndentation(lines[j])) + if nextIndent <= lineIndent && isYAMLKeyLike(lines[j]) { + break + } + if nextIndent > lineIndent { + blockEnd = j + continue + } + break + } + return i, blockEnd + } + return -1, -1 +} + +func isYAMLKeyLike(line string) bool { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") || strings.HasPrefix(trimmed, "- ") { + return false + } + return strings.Contains(trimmed, ":") } diff --git a/pkg/cli/codemod_bots_test.go b/pkg/cli/codemod_bots_test.go index 8878724799c..be0b6e0a351 100644 --- a/pkg/cli/codemod_bots_test.go +++ b/pkg/cli/codemod_bots_test.go @@ -162,15 +162,15 @@ engine: copilot assert.Equal(t, content, result) } -func TestBotsToOnBotsCodemod_NoChange_OnBotsExists(t *testing.T) { +func TestBotsToOnBotsCodemod_MergesWhenOnBotsExists(t *testing.T) { t.Parallel() codemod := getBotsToOnBotsCodemod() content := `--- on: - issues: - types: [opened] - bots: [dependabot, renovate] + issues: + types: [opened] + bots: [dependabot, renovate] bots: [dependabot, renovate, github-actions] --- @@ -189,6 +189,78 @@ bots: [dependabot, renovate, github-actions] result, applied, err := codemod.Apply(content, frontmatter) require.NoError(t, err) - assert.False(t, applied) - assert.Equal(t, content, result) + assert.True(t, applied) + assert.Contains(t, result, ` bots: ["dependabot","renovate","github-actions"]`) + assert.NotContains(t, result, "\nbots:") +} + +func TestBotsToOnBotsCodemod_IgnoresNestedBotsKey(t *testing.T) { + t.Parallel() + codemod := getBotsToOnBotsCodemod() + + content := `--- +on: + workflow_call: + inputs: + bots: + type: string + bots: [dependabot] +bots: [github-actions] +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": map[string]any{ + "workflow_call": map[string]any{ + "inputs": map[string]any{ + "bots": map[string]any{"type": "string"}, + }, + }, + "bots": []any{"dependabot"}, + }, + "bots": []any{"github-actions"}, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "workflow_call:") + assert.Contains(t, result, "type: string") + assert.Contains(t, result, "bots: [\"dependabot\",\"github-actions\"]") +} + +func TestBotsToOnBotsCodemod_PreservesCommentsAndBlankLines(t *testing.T) { + t.Parallel() + codemod := getBotsToOnBotsCodemod() + + content := `--- +on: + bots: + - dependabot # keep existing bot comment + + # keep block comment + - renovate +bots: + - github-actions # legacy comment +--- + +# Test workflow` + + frontmatter := map[string]any{ + "on": map[string]any{ + "bots": []any{"dependabot", "renovate"}, + }, + "bots": []any{"github-actions", "dependabot", "renovate"}, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "# keep block comment") + assert.Contains(t, result, "# legacy comment") + assert.Contains(t, result, "dependabot # keep existing bot comment") + assert.Contains(t, result, "bots:") } diff --git a/pkg/parser/import_field_extractor.go b/pkg/parser/import_field_extractor.go index 62107269a5f..6c6d78edb42 100644 --- a/pkg/parser/import_field_extractor.go +++ b/pkg/parser/import_field_extractor.go @@ -568,16 +568,20 @@ func (acc *importAccumulator) extractActivationFields(fm map[string]any, item im func (acc *importAccumulator) mergeBots(fm map[string]any) { mergeJSONStringListField(fm, "bots", "[]", acc.botsSet, &acc.bots, func(m map[string]any, field string) (string, error) { - return extractFieldJSONFromMap(m, field, "[]") + return extractOnSectionFieldFromMap(m, field) }) } func (acc *importAccumulator) mergeSkipRoles(fm map[string]any) { - mergeJSONStringListField(fm, "skip-roles", "[]", acc.skipRolesSet, &acc.skipRoles, extractOnSectionFieldFromMap) + mergeJSONStringListField(fm, "skip-roles", "[]", acc.skipRolesSet, &acc.skipRoles, func(m map[string]any, field string) (string, error) { + return extractOnSectionFieldFromMap(m, field) + }) } func (acc *importAccumulator) mergeSkipBots(fm map[string]any) { - mergeJSONStringListField(fm, "skip-bots", "[]", acc.skipBotsSet, &acc.skipBots, extractOnSectionFieldFromMap) + mergeJSONStringListField(fm, "skip-bots", "[]", acc.skipBotsSet, &acc.skipBots, func(m map[string]any, field string) (string, error) { + return extractOnSectionFieldFromMap(m, field) + }) } func (acc *importAccumulator) mergeAmbientFolders(fm map[string]any) { diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index 5b7352967f3..631c72194b7 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -27,7 +27,7 @@ func TestValidateMainWorkflowFrontmatter_IssueFieldActivityTypes(t *testing.T) { func TestValidateMainWorkflowFrontmatter_RejectsUnsupportedTopLevelFields(t *testing.T) { t.Parallel() - for _, field := range []string{"version", "include"} { + for _, field := range []string{"version", "include", "bots"} { t.Run(field, func(t *testing.T) { t.Parallel() diff --git a/pkg/workflow/bots_test.go b/pkg/workflow/bots_test.go index b2488d2cb7a..129185ebd1c 100644 --- a/pkg/workflow/bots_test.go +++ b/pkg/workflow/bots_test.go @@ -325,19 +325,19 @@ Test workflow content.` `Expected compiled workflow to expand "copilot" alias to all Copilot bot identifiers`) } -// TestBotsImportMerge tests that bots from imported workflows are merged with top-level bots +// TestBotsImportMerge tests that on.bots from imported workflows are merged with main workflow bots // in the compiled output (regression test for the fix in compiler_orchestrator_workflow.go). func TestBotsImportMerge(t *testing.T) { compiler := NewCompiler() - t.Run("imported_bots_merged_with_top_level_bots", func(t *testing.T) { + t.Run("imported_bots_merged_with_main_workflow_bots", func(t *testing.T) { tmpDir := testutil.TempDir(t, "bots-import-merge-test") - // Shared workflow defines a bot at the top level (the format used by the importer) + // Shared workflow defines an on.bots allowlist without defining a trigger sharedContent := `--- -on: issues -bots: - - "renovate[bot]" +on: + bots: + - "renovate[bot]" --- ` sharedPath := filepath.Join(tmpDir, "shared-bots.md") @@ -376,9 +376,9 @@ imports: tmpDir := testutil.TempDir(t, "bots-import-only-test") sharedContent := `--- -on: issues -bots: - - "github-actions[bot]" +on: + bots: + - "github-actions[bot]" --- ` sharedPath := filepath.Join(tmpDir, "shared-bots-only.md") @@ -412,15 +412,50 @@ imports: "Expected compiled workflow to contain bots from import when main workflow has none") }) - t.Run("duplicate_bots_across_top_level_and_import_deduped", func(t *testing.T) { - tmpDir := testutil.TempDir(t, "bots-import-dedup-test") + t.Run("legacy_top_level_bots_in_import_are_ignored", func(t *testing.T) { + tmpDir := testutil.TempDir(t, "bots-import-legacy-ignored-test") sharedContent := `--- -on: issues bots: - - "dependabot[bot]" - "renovate[bot]" --- +` + sharedPath := filepath.Join(tmpDir, "shared-legacy-bots.md") + err := os.WriteFile(sharedPath, []byte(sharedContent), 0644) + require.NoError(t, err, "Failed to write legacy bot import file") + + mainContent := `--- +on: + issues: + types: [opened] +imports: + - shared-legacy-bots.md +--- + +# Main workflow importing a legacy bot allowlist. +` + mainPath := filepath.Join(tmpDir, "main-legacy-bots.md") + err = os.WriteFile(mainPath, []byte(mainContent), 0644) + require.NoError(t, err, "Failed to write main workflow file") + + err = compiler.CompileWorkflow(mainPath) + require.NoError(t, err, "Legacy top-level bot allowlists should not be merged into imports") + + lockContent, err := os.ReadFile(stringutil.MarkdownToLockFile(mainPath)) + require.NoError(t, err, "Failed to read lock file") + assert.NotContains(t, string(lockContent), `GH_AW_ALLOWED_BOTS: "renovate[bot]"`, + "Expected legacy top-level bot allowlists in imports to be ignored") + }) + + t.Run("duplicate_bots_across_top_level_and_import_deduped", func(t *testing.T) { + tmpDir := testutil.TempDir(t, "bots-import-dedup-test") + + sharedContent := `--- +on: + bots: + - "dependabot[bot]" + - "renovate[bot]" +--- ` sharedPath := filepath.Join(tmpDir, "shared-bots-dup.md") err := os.WriteFile(sharedPath, []byte(sharedContent), 0644) diff --git a/pkg/workflow/role_checks_test.go b/pkg/workflow/role_checks_test.go index f044f1fc286..2da6c6c2e1f 100644 --- a/pkg/workflow/role_checks_test.go +++ b/pkg/workflow/role_checks_test.go @@ -564,10 +564,13 @@ func TestCommentAuthorAssociationImportedExpressionBot(t *testing.T) { tmpDir := testutil.TempDir(t, "comment-auth-import-test") compiler := NewCompiler() - // Shared agentic workflow: no on: field, but defines a bot with a GHA expression. + // Shared agentic workflow: defines a bot with a GHA expression using the supported + // on.bots path. This should disable the static author_association guard because the + // bot identity is only known at runtime. sharedContent := `--- -bots: - - "${{ vars.TRUSTED_BOT }}" +on: + bots: + - "${{ vars.TRUSTED_BOT }}" --- ` sharedPath := filepath.Join(tmpDir, "shared-bots.md")