Import and merge on.bots from shared workflows

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.*

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, ":")
 }