diff --git a/pkg/workflow/config_parsing_helpers_test.go b/pkg/workflow/config_parsing_helpers_test.go index 24d58df941a..d87d1b8426c 100644 --- a/pkg/workflow/config_parsing_helpers_test.go +++ b/pkg/workflow/config_parsing_helpers_test.go @@ -255,6 +255,77 @@ func TestParsePullRequestsConfigWithHelpers(t *testing.T) { } } +func TestParseIssuesConfigWithSingleStringAssignee(t *testing.T) { + compiler := &Compiler{} + outputMap := map[string]any{ + "create-issue": map[string]any{ + "assignees": "single-assignee", + }, + } + + result := compiler.parseCreateIssuesConfig(outputMap) + if result == nil { + t.Fatal("expected non-nil result") + } + + if len(result.Assignees) != 1 { + t.Fatalf("expected 1 assignee, got %d", len(result.Assignees)) + } + if result.Assignees[0] != "single-assignee" { + t.Errorf("expected assignee 'single-assignee', got %q", result.Assignees[0]) + } +} + +func TestParsePullRequestsConfigWithSingleStringReviewerAndTeamReviewer(t *testing.T) { + compiler := &Compiler{} + outputMap := map[string]any{ + "create-pull-request": map[string]any{ + "reviewers": "single-reviewer", + "team-reviewers": "single-team", + }, + } + + result := compiler.parseCreatePullRequestsConfig(outputMap) + if result == nil { + t.Fatal("expected non-nil result") + } + + if len(result.Reviewers) != 1 { + t.Fatalf("expected 1 reviewer, got %d", len(result.Reviewers)) + } + if result.Reviewers[0] != "single-reviewer" { + t.Errorf("expected reviewer 'single-reviewer', got %q", result.Reviewers[0]) + } + + if len(result.TeamReviewers) != 1 { + t.Fatalf("expected 1 team reviewer, got %d", len(result.TeamReviewers)) + } + if result.TeamReviewers[0] != "single-team" { + t.Errorf("expected team reviewer 'single-team', got %q", result.TeamReviewers[0]) + } +} + +func TestParsePullRequestsConfigWithSingleStringAssignee(t *testing.T) { + compiler := &Compiler{} + outputMap := map[string]any{ + "create-pull-request": map[string]any{ + "assignees": "single-assignee", + }, + } + + result := compiler.parseCreatePullRequestsConfig(outputMap) + if result == nil { + t.Fatal("expected non-nil result") + } + + if len(result.Assignees) != 1 { + t.Fatalf("expected 1 assignee, got %d", len(result.Assignees)) + } + if result.Assignees[0] != "single-assignee" { + t.Errorf("expected assignee 'single-assignee', got %q", result.Assignees[0]) + } +} + func TestParsePullRequestsConfigExpires(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go index 96d1ab7f249..51f6d0680bb 100644 --- a/pkg/workflow/create_discussion.go +++ b/pkg/workflow/create_discussion.go @@ -30,61 +30,48 @@ type CreateDiscussionsConfig struct { // parseCreateDiscussionsConfig handles create-discussion configuration func (c *Compiler) parseCreateDiscussionsConfig(outputMap map[string]any) *CreateDiscussionsConfig { - // Check if the key exists - if _, exists := outputMap["create-discussion"]; !exists { - return nil - } - - // Get the config data to check for special cases before unmarshaling - configData, _ := outputMap["create-discussion"].(map[string]any) - - // Pre-process the expires field (convert to hours before unmarshaling) - expiresDisabled := preprocessExpiresField(configData, discussionLog) - - // Pre-process templatable bool fields - for _, field := range []string{"close-older-discussions", "footer"} { - if err := preprocessBoolFieldAsString(configData, field, discussionLog); err != nil { - discussionLog.Printf("Invalid %s value: %v", field, err) - return nil - } - } + config := parseCreateEntityConfig( + outputMap, + "create-discussion", + CreateParseOptions{ + BoolFields: []string{"close-older-discussions", "footer"}, + IntFields: []string{"max"}, + HandleExpires: true, + }, + discussionLog, + func(err error) *CreateDiscussionsConfig { + discussionLog.Printf("Failed to unmarshal config: %v", err) + // For backward compatibility, handle nil/empty config + return &CreateDiscussionsConfig{} + }, + nil, + func(_ map[string]any, config *CreateDiscussionsConfig, expiresDisabled bool) { + // Set default max if not specified + if config.Max == nil { + config.Max = defaultIntStr(1) + } - // Pre-process templatable int fields - if err := preprocessIntFieldAsString(configData, "max", discussionLog); err != nil { - discussionLog.Printf("Invalid max value: %v", err) - return nil - } + // Set default expires to 7 days (168 hours) if not specified and not explicitly disabled + if config.Expires == 0 && !expiresDisabled { + config.Expires = 168 // 7 days = 168 hours + discussionLog.Print("Using default expiration: 7 days (168 hours)") + } else if expiresDisabled { + config.Expires = 0 + discussionLog.Print("Expiration explicitly disabled") + } - config := parseConfigScaffold(outputMap, "create-discussion", discussionLog, func(err error) *CreateDiscussionsConfig { - discussionLog.Printf("Failed to unmarshal config: %v", err) - // For backward compatibility, handle nil/empty config - return &CreateDiscussionsConfig{} - }) + // Set default fallback-to-issue to true if not specified + if config.FallbackToIssue == nil { + trueVal := true + config.FallbackToIssue = &trueVal + discussionLog.Print("Using default fallback-to-issue: true") + } + }, + ) if config == nil { return nil } - // Set default max if not specified - if config.Max == nil { - config.Max = defaultIntStr(1) - } - - // Set default expires to 7 days (168 hours) if not specified and not explicitly disabled - if config.Expires == 0 && !expiresDisabled { - config.Expires = 168 // 7 days = 168 hours - discussionLog.Print("Using default expiration: 7 days (168 hours)") - } else if expiresDisabled { - config.Expires = 0 - discussionLog.Print("Expiration explicitly disabled") - } - - // Set default fallback-to-issue to true if not specified - if config.FallbackToIssue == nil { - trueVal := true - config.FallbackToIssue = &trueVal - discussionLog.Print("Using default fallback-to-issue: true") - } - // Normalize and validate category naming convention config.Category = normalizeDiscussionCategory(config.Category, discussionLog, c.markdownPath) diff --git a/pkg/workflow/create_entity_helpers.go b/pkg/workflow/create_entity_helpers.go new file mode 100644 index 00000000000..b686e5f4181 --- /dev/null +++ b/pkg/workflow/create_entity_helpers.go @@ -0,0 +1,83 @@ +package workflow + +import "github.com/github/gh-aw/pkg/logger" + +// CreateParseOptions defines common preprocessing options for create-entity parsers. +// +// BoolFields and IntFields list config field names that should be normalized through +// templatable preprocessing before YAML unmarshaling. +// HandleExpires enables shared expires normalization via preprocessExpiresField. +type CreateParseOptions struct { + BoolFields []string + IntFields []string + HandleExpires bool +} + +// parseCreateEntityConfig parses create-* config scaffolding shared by issue/discussion/PR handlers. +// +// Parameters: +// - outputMap: full safe-output map from frontmatter parsing. +// - configKey: create-* key to parse (for example "create-issue"). +// - opts: shared preprocessing configuration for bool/int/expires fields. +// - log: logger used for preprocessing and parse diagnostics. +// - onError: required error handler invoked on unmarshal failures. +// +// Callback lifecycle: +// - preUnmarshal is optional (may be nil). When provided, it is invoked first with the raw +// config map. The map may be nil when configKey exists but is not a map; if preUnmarshal +// returns false, parsing is aborted. +// - onError is invoked when YAML unmarshaling fails and returns the fallback config behavior. +// - postUnmarshal is optional (may be nil). When provided, it is invoked after successful +// unmarshaling and receives expiresDisabled (true when expires was explicitly set to false). +func parseCreateEntityConfig[T any]( + outputMap map[string]any, + configKey string, + opts CreateParseOptions, + log *logger.Logger, + onError func(error) *T, + preUnmarshal func(map[string]any) bool, + postUnmarshal func(map[string]any, *T, bool), +) *T { + if _, exists := outputMap[configKey]; !exists { + return nil + } + + configDataAny := outputMap[configKey] + configData, isMap := configDataAny.(map[string]any) + if !isMap { + configData = nil + } + if preUnmarshal != nil && !preUnmarshal(configData) { + return nil + } + + expiresDisabled := false + if opts.HandleExpires { + expiresDisabled = preprocessExpiresField(configData, log) + } + + for _, field := range opts.BoolFields { + if err := preprocessBoolFieldAsString(configData, field, log); err != nil { + log.Printf("Invalid %s value: %v", field, err) + return nil + } + } + + for _, field := range opts.IntFields { + if err := preprocessIntFieldAsString(configData, field, log); err != nil { + log.Printf("Invalid %s value: %v", field, err) + return nil + } + } + + config := parseConfigScaffold(outputMap, configKey, log, onError) + if config == nil { + return nil + } + + if postUnmarshal != nil { + postUnmarshal(configData, config, expiresDisabled) + } + + return config +} diff --git a/pkg/workflow/create_issue.go b/pkg/workflow/create_issue.go index 500acf94350..4f09c97b05d 100644 --- a/pkg/workflow/create_issue.go +++ b/pkg/workflow/create_issue.go @@ -28,64 +28,38 @@ type CreateIssuesConfig struct { // parseCreateIssuesConfig handles create-issue configuration func (c *Compiler) parseCreateIssuesConfig(outputMap map[string]any) *CreateIssuesConfig { - // Check if the key exists - if _, exists := outputMap["create-issue"]; !exists { - return nil - } - - // Get the config data to check for special cases before unmarshaling - configData, _ := outputMap["create-issue"].(map[string]any) - - // Pre-process the expires field (convert to hours before unmarshaling) - expiresDisabled := preprocessExpiresField(configData, createIssueLog) - - // Pre-process templatable bool fields: convert literal booleans to strings so that - // GitHub Actions expression strings (e.g. "${{ inputs.close-older-issues }}") are also accepted. - for _, field := range []string{"close-older-issues", "group", "footer", "group-by-day"} { - if err := preprocessBoolFieldAsString(configData, field, createIssueLog); err != nil { - createIssueLog.Printf("Invalid %s value: %v", field, err) - return nil - } - } - - // Pre-process templatable int fields - if err := preprocessIntFieldAsString(configData, "max", createIssueLog); err != nil { - createIssueLog.Printf("Invalid max value: %v", err) - return nil - } - - config := parseConfigScaffold(outputMap, "create-issue", createIssueLog, func(err error) *CreateIssuesConfig { - createIssueLog.Printf("Failed to unmarshal config: %v", err) - // For backward compatibility, handle nil/empty config - return &CreateIssuesConfig{} - }) - if config == nil { - return nil - } - - // Handle single string assignee (YAML unmarshaling won't convert string to []string) - if len(config.Assignees) == 0 && configData != nil { - if assignees, exists := configData["assignees"]; exists { - if assigneeStr, ok := assignees.(string); ok { - config.Assignees = []string{assigneeStr} - createIssueLog.Printf("Converted single assignee string to array: %v", config.Assignees) + return parseCreateEntityConfig( + outputMap, + "create-issue", + CreateParseOptions{ + BoolFields: []string{"close-older-issues", "group", "footer", "group-by-day"}, + IntFields: []string{"max"}, + HandleExpires: true, + }, + createIssueLog, + func(err error) *CreateIssuesConfig { + createIssueLog.Printf("Failed to unmarshal config: %v", err) + // For backward compatibility, handle nil/empty config + return &CreateIssuesConfig{} + }, + func(configData map[string]any) bool { + coerceStringOrArrayFields(configData, []string{"assignees"}, createIssueLog) + return true + }, + func(_ map[string]any, config *CreateIssuesConfig, expiresDisabled bool) { + // Set default max if not specified + if config.Max == nil { + config.Max = defaultIntStr(1) } - } - } - - // Set default max if not specified - if config.Max == nil { - config.Max = defaultIntStr(1) - } - // Log expires if configured or explicitly disabled - if expiresDisabled { - createIssueLog.Print("Issue expiration explicitly disabled") - } else if config.Expires > 0 { - createIssueLog.Printf("Issue expiration configured: %d hours", config.Expires) - } - - return config + // Log expires if configured or explicitly disabled + if expiresDisabled { + createIssueLog.Print("Issue expiration explicitly disabled") + } else if config.Expires > 0 { + createIssueLog.Printf("Issue expiration configured: %d hours", config.Expires) + } + }, + ) } // hasCopilotAssignee checks if "copilot" is in the assignees list diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index c4be15299bc..43cec718516 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -6,6 +6,9 @@ import ( var createPRLog = logger.New("workflow:create_pull_request") +var createPRStringOrArrayFields = []string{"reviewers", "team-reviewers", "assignees"} +var createPRExpressionArrayFields = []string{"labels", "allowed-repos", "allowed-base-branches"} + // getFallbackAsIssue returns the effective fallback-as-issue setting (defaults to true). func getFallbackAsIssue(config *CreatePullRequestsConfig) bool { if config == nil || config.FallbackAsIssue == nil { @@ -57,114 +60,73 @@ func (c *Compiler) parseCreatePullRequestsConfig(outputMap map[string]any) *Crea return nil } - // Get the config data to check for special cases before unmarshaling - configData, _ := outputMap["create-pull-request"].(map[string]any) - - // Pre-process the reviewers field to convert single string to array BEFORE unmarshaling - // This prevents YAML unmarshal errors when reviewers is a string instead of []string - if configData != nil { - if reviewers, exists := configData["reviewers"]; exists { - if reviewerStr, ok := reviewers.(string); ok { - // Convert single string to array - configData["reviewers"] = []string{reviewerStr} - createPRLog.Printf("Converted single reviewer string to array before unmarshaling") - } - } - if teamReviewers, exists := configData["team-reviewers"]; exists { - if teamReviewerStr, ok := teamReviewers.(string); ok { - // Convert single string to array - configData["team-reviewers"] = []string{teamReviewerStr} - createPRLog.Printf("Converted single team-reviewer string to array before unmarshaling") - } - } - // Pre-process the assignees field to convert single string to array BEFORE unmarshaling - if assignees, exists := configData["assignees"]; exists { - if assigneeStr, ok := assignees.(string); ok { - // Convert single string to array - configData["assignees"] = []string{assigneeStr} - createPRLog.Printf("Converted single assignee string to array before unmarshaling") - } - } - } - - // Pre-process the expires field (convert to hours before unmarshaling) - // Use preprocessExpiresField to handle all types: integers (days), strings (time specs), and boolean false - // This is consistent with how parseCreateIssuesConfig and parseCreateDiscussionsConfig handle expires - expiresDisabled := preprocessExpiresField(configData, createPRLog) - if expiresDisabled { - createPRLog.Print("Pull request expiration disabled") - } - - // Pre-process templatable bool fields: convert literal booleans to strings so that - // GitHub Actions expression strings (e.g. "${{ inputs.draft-prs }}") are also accepted. - for _, field := range []string{"draft", "allow-empty", "auto-merge", "footer", "auto-close-issue"} { - if err := preprocessBoolFieldAsString(configData, field, createPRLog); err != nil { - createPRLog.Printf("Invalid %s value: %v", field, err) - return nil - } - } - - // Pre-process protected-files: supports string enum OR object form {policy, exclude}. - // Object form is preprocessed to extract the policy (stored back as string) and - // the exclude list (stored in a local variable and assigned to the config after unmarshaling). var protectedFilesExclude []string - if configData != nil { - protectedFilesExclude = preprocessProtectedFilesField(configData, createPRLog) - } - - // Validate protected-files string enum after object-form preprocessing. - manifestFilesEnums := []string{"blocked", "allowed", "fallback-to-issue"} - if configData != nil { - validateStringEnumField(configData, "protected-files", manifestFilesEnums, createPRLog) - } + config := parseCreateEntityConfig( + outputMap, + "create-pull-request", + CreateParseOptions{ + BoolFields: []string{"draft", "allow-empty", "auto-merge", "footer", "auto-close-issue"}, + IntFields: []string{"max"}, + HandleExpires: true, + }, + createPRLog, + func(err error) *CreatePullRequestsConfig { + createPRLog.Printf("Failed to unmarshal config: %v", err) + // For backward compatibility, handle nil/empty config + return &CreatePullRequestsConfig{} + }, + func(configData map[string]any) bool { + coerceStringOrArrayFields(configData, createPRStringOrArrayFields, createPRLog) + + // Pre-process protected-files: supports string enum OR object form {policy, exclude}. + // Object form is preprocessed to extract the policy (stored back as string) and + // the exclude list (stored in a local variable and assigned to the config after unmarshaling). + protectedFilesExclude = preprocessProtectedFilesField(configData, createPRLog) + + // Validate protected-files string enum after object-form preprocessing. + validateStringEnumField(configData, "protected-files", []string{"blocked", "allowed", "fallback-to-issue"}, createPRLog) + + // Pre-process patch-format: valid values are "bundle" (default) and "am". + validateStringEnumField(configData, "patch-format", []string{"am", "bundle"}, createPRLog) + + // Pre-process list fields that also accept a GitHub Actions expression string. + // An expression is wrapped in a single-element []string so the []string struct field + // can receive it after YAML unmarshaling; the handler config builder later re-emits it + // as a JSON string for runtime evaluation. + for _, field := range createPRExpressionArrayFields { + if err := preprocessStringArrayFieldAsTemplatable(configData, field, createPRLog); err != nil { + createPRLog.Printf("Invalid %s value: %v", field, err) + return false + } + } - // Pre-process patch-format: valid values are "bundle" (default) and "am". - patchFormatEnums := []string{"am", "bundle"} - if configData != nil { - validateStringEnumField(configData, "patch-format", patchFormatEnums, createPRLog) - } + return true + }, + func(_ map[string]any, config *CreatePullRequestsConfig, expiresDisabled bool) { + if expiresDisabled { + createPRLog.Print("Pull request expiration disabled") + } - // Pre-process templatable int fields - if err := preprocessIntFieldAsString(configData, "max", createPRLog); err != nil { - createPRLog.Printf("Invalid max value: %v", err) - return nil - } + // Log expires if configured + if config.Expires > 0 { + createPRLog.Printf("Pull request expiration configured: %d hours", config.Expires) + } - // Pre-process list fields that also accept a GitHub Actions expression string. - // An expression is wrapped in a single-element []string so the []string struct field - // can receive it after YAML unmarshaling; the handler config builder later re-emits it - // as a JSON string for runtime evaluation. - for _, field := range []string{"labels", "allowed-repos", "allowed-base-branches"} { - if err := preprocessStringArrayFieldAsTemplatable(configData, field, createPRLog); err != nil { - createPRLog.Printf("Invalid %s value: %v", field, err) - return nil - } - } + // Apply the exclude list extracted from the object-form protected-files field. + config.ProtectedFilesExclude = protectedFilesExclude - config := parseConfigScaffold(outputMap, "create-pull-request", createPRLog, func(err error) *CreatePullRequestsConfig { - createPRLog.Printf("Failed to unmarshal config: %v", err) - // For backward compatibility, handle nil/empty config - return &CreatePullRequestsConfig{} - }) + // Set default max if not explicitly configured (default is 1) + if config.Max == nil { + config.Max = defaultIntStr(1) + createPRLog.Print("Using default max count: 1") + } else { + createPRLog.Printf("Pull request max count configured: %s", *config.Max) + } + }, + ) if config == nil { return nil } - // Log expires if configured - if config.Expires > 0 { - createPRLog.Printf("Pull request expiration configured: %d hours", config.Expires) - } - - // Apply the exclude list extracted from the object-form protected-files field. - config.ProtectedFilesExclude = protectedFilesExclude - - // Set default max if not explicitly configured (default is 1) - if config.Max == nil { - config.Max = defaultIntStr(1) - createPRLog.Print("Using default max count: 1") - } else { - createPRLog.Printf("Pull request max count configured: %s", *config.Max) - } - return config } diff --git a/pkg/workflow/parse_helpers.go b/pkg/workflow/parse_helpers.go index b65feb7a4e4..7b4a1a466f8 100644 --- a/pkg/workflow/parse_helpers.go +++ b/pkg/workflow/parse_helpers.go @@ -8,6 +8,8 @@ // - parseStringSliceAny() - Canonical coercion of []string/[]any to []string; skips non-string items. // For GitHub Actions fields where a bare string is valid shorthand for a single-element list // (e.g. `needs: job-name`, `state: failure`), handle the string case explicitly at the call site. +// - coerceStringOrArrayField() - Converts a single string scalar field into a one-element []string +// for fields that accept either a single value or an array in workflow YAML. // - preprocessProtectedFilesField() - Normalises the "protected-files" field from its object or // string form before downstream enum validation. @@ -15,6 +17,33 @@ package workflow import "github.com/github/gh-aw/pkg/logger" +// coerceStringOrArrayField converts configData[key] from a string to []string{value} +// so YAML unmarshaling into []string fields succeeds for single-value shorthand. +// +// When key is missing, nil, or already a non-string type, this function is a no-op. +// The log parameter is optional; pass nil to suppress debug output. +func coerceStringOrArrayField(configData map[string]any, key string, log *logger.Logger) { + if configData == nil { + return + } + + if value, exists := configData[key]; exists { + if stringValue, ok := value.(string); ok { + configData[key] = []string{stringValue} + if log != nil { + log.Printf("Converted single %s string to array before unmarshaling", key) + } + } + } +} + +// coerceStringOrArrayFields applies coerceStringOrArrayField to multiple keys. +func coerceStringOrArrayFields(configData map[string]any, keys []string, log *logger.Logger) { + for _, key := range keys { + coerceStringOrArrayField(configData, key, log) + } +} + // preprocessProtectedFilesField preprocesses the "protected-files" field in configData, // handling both the legacy string-enum form and the new object form. //