From 1bb3975e51f77fb0fecfd4217d4a369286fb773c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 12:04:18 +0000 Subject: [PATCH 1/2] Initial plan From c7dbbd6c5a1385f7b2ec1287f821ac87e0746327 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 12:25:49 +0000 Subject: [PATCH 2/2] refactor: extract generic parseConfigScaffold helper to eliminate safe-output parser duplication Addresses: https://github.com/github/gh-aw/issues (duplicate safe-output parser scaffold) - Add parseConfigScaffold[T any] generic helper to config_helpers.go - Refactor 8 handler parse*Config methods to use the scaffold - Add 7 focused unit tests in config_scaffold_helpers_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw/sessions/620a8f3e-6f77-4846-ac02-e47a24f52822 --- pkg/workflow/add_labels.go | 19 +-- pkg/workflow/assign_milestone.go | 22 +-- pkg/workflow/assign_to_agent.go | 13 +- pkg/workflow/assign_to_user.go | 15 +- pkg/workflow/config_helpers.go | 48 +++++++ pkg/workflow/config_scaffold_helpers_test.go | 136 ++++++++++++++++++ .../mark_pull_request_as_ready_for_review.go | 25 ++-- pkg/workflow/remove_labels.go | 19 +-- pkg/workflow/set_issue_type.go | 19 +-- pkg/workflow/unassign_from_user.go | 18 +-- 10 files changed, 233 insertions(+), 101 deletions(-) create mode 100644 pkg/workflow/config_scaffold_helpers_test.go diff --git a/pkg/workflow/add_labels.go b/pkg/workflow/add_labels.go index cb2adcb6547..74b95d4fce1 100644 --- a/pkg/workflow/add_labels.go +++ b/pkg/workflow/add_labels.go @@ -16,23 +16,14 @@ type AddLabelsConfig struct { // parseAddLabelsConfig handles add-labels configuration func (c *Compiler) parseAddLabelsConfig(outputMap map[string]any) *AddLabelsConfig { - // Check if the key exists - if _, exists := outputMap["add-labels"]; !exists { - return nil - } - - addLabelsLog.Print("Parsing add-labels configuration") - - // Unmarshal into typed config struct - var config AddLabelsConfig - if err := unmarshalConfig(outputMap, "add-labels", &config, addLabelsLog); err != nil { + config := parseConfigScaffold(outputMap, "add-labels", addLabelsLog, func(err error) *AddLabelsConfig { addLabelsLog.Printf("Failed to unmarshal config: %v", err) // Handle null case: create empty config (allows any labels) addLabelsLog.Print("Using empty configuration (allows any labels)") return &AddLabelsConfig{} + }) + if config != nil { + addLabelsLog.Printf("Parsed configuration: allowed_count=%d, blocked_count=%d, target=%s", len(config.Allowed), len(config.Blocked), config.Target) } - - addLabelsLog.Printf("Parsed configuration: allowed_count=%d, blocked_count=%d, target=%s", len(config.Allowed), len(config.Blocked), config.Target) - - return &config + return config } diff --git a/pkg/workflow/assign_milestone.go b/pkg/workflow/assign_milestone.go index 70a82f900a2..427e9d09073 100644 --- a/pkg/workflow/assign_milestone.go +++ b/pkg/workflow/assign_milestone.go @@ -15,25 +15,15 @@ type AssignMilestoneConfig struct { // parseAssignMilestoneConfig handles assign-milestone configuration func (c *Compiler) parseAssignMilestoneConfig(outputMap map[string]any) *AssignMilestoneConfig { - // Check if the key exists - if _, exists := outputMap["assign-milestone"]; !exists { - assignMilestoneLog.Print("No assign-milestone configuration found") - return nil - } - - assignMilestoneLog.Print("Parsing assign-milestone configuration") - - // Unmarshal into typed config struct - var config AssignMilestoneConfig - if err := unmarshalConfig(outputMap, "assign-milestone", &config, assignMilestoneLog); err != nil { + config := parseConfigScaffold(outputMap, "assign-milestone", assignMilestoneLog, func(err error) *AssignMilestoneConfig { assignMilestoneLog.Printf("Failed to unmarshal config: %v", err) // Handle null case: create empty config (allows any milestones) assignMilestoneLog.Print("Null milestone config, allowing any milestones") return &AssignMilestoneConfig{} + }) + if config != nil { + assignMilestoneLog.Printf("Parsed milestone config: target=%s, allowed_count=%d", + config.Target, len(config.Allowed)) } - - assignMilestoneLog.Printf("Parsed milestone config: target=%s, allowed_count=%d", - config.Target, len(config.Allowed)) - - return &config + return config } diff --git a/pkg/workflow/assign_to_agent.go b/pkg/workflow/assign_to_agent.go index d6ebe822b2d..419ea8c119e 100644 --- a/pkg/workflow/assign_to_agent.go +++ b/pkg/workflow/assign_to_agent.go @@ -23,13 +23,11 @@ type AssignToAgentConfig struct { // parseAssignToAgentConfig handles assign-to-agent configuration func (c *Compiler) parseAssignToAgentConfig(outputMap map[string]any) *AssignToAgentConfig { - // Check if the key exists + // Check key existence first so we can preprocess templatable fields before YAML unmarshaling if _, exists := outputMap["assign-to-agent"]; !exists { return nil } - assignToAgentLog.Print("Parsing assign-to-agent configuration") - // Get config data for pre-processing before YAML unmarshaling configData, _ := outputMap["assign-to-agent"].(map[string]any) @@ -39,12 +37,13 @@ func (c *Compiler) parseAssignToAgentConfig(outputMap map[string]any) *AssignToA return nil } - // Unmarshal into typed config struct - var config AssignToAgentConfig - if err := unmarshalConfig(outputMap, "assign-to-agent", &config, assignToAgentLog); err != nil { + config := parseConfigScaffold(outputMap, "assign-to-agent", assignToAgentLog, func(err error) *AssignToAgentConfig { assignToAgentLog.Printf("Failed to unmarshal config: %v", err) // Handle null case: create empty config return &AssignToAgentConfig{} + }) + if config == nil { + return nil } // Set default max if not specified @@ -55,5 +54,5 @@ func (c *Compiler) parseAssignToAgentConfig(outputMap map[string]any) *AssignToA assignToAgentLog.Printf("Parsed assign-to-agent config: default_agent=%s, default_model=%s, default_custom_agent=%s, allowed_count=%d, target=%s, max=%s, pull_request_repo=%s, base_branch=%s", config.DefaultAgent, config.DefaultModel, config.DefaultCustomAgent, len(config.Allowed), config.Target, *config.Max, config.PullRequestRepoSlug, config.BaseBranch) - return &config + return config } diff --git a/pkg/workflow/assign_to_user.go b/pkg/workflow/assign_to_user.go index 7933060e018..743946d5d32 100644 --- a/pkg/workflow/assign_to_user.go +++ b/pkg/workflow/assign_to_user.go @@ -17,13 +17,11 @@ type AssignToUserConfig struct { // parseAssignToUserConfig handles assign-to-user configuration func (c *Compiler) parseAssignToUserConfig(outputMap map[string]any) *AssignToUserConfig { - // Check if the key exists + // Check key existence first so we can preprocess templatable fields before YAML unmarshaling if _, exists := outputMap["assign-to-user"]; !exists { return nil } - assignToUserLog.Print("Parsing assign-to-user configuration") - // Get config data for pre-processing before YAML unmarshaling configData, _ := outputMap["assign-to-user"].(map[string]any) @@ -39,15 +37,16 @@ func (c *Compiler) parseAssignToUserConfig(outputMap map[string]any) *AssignToUs return nil } - // Unmarshal into typed config struct - var config AssignToUserConfig - if err := unmarshalConfig(outputMap, "assign-to-user", &config, assignToUserLog); err != nil { + config := parseConfigScaffold(outputMap, "assign-to-user", assignToUserLog, func(err error) *AssignToUserConfig { assignToUserLog.Printf("Failed to unmarshal config: %v", err) // For backward compatibility, use defaults assignToUserLog.Print("Using default configuration") - config = AssignToUserConfig{ + return &AssignToUserConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(1)}, } + }) + if config == nil { + return nil } // Set default max if not specified @@ -57,5 +56,5 @@ func (c *Compiler) parseAssignToUserConfig(outputMap map[string]any) *AssignToUs assignToUserLog.Printf("Parsed configuration: allowed_count=%d, target=%s", len(config.Allowed), config.Target) - return &config + return config } diff --git a/pkg/workflow/config_helpers.go b/pkg/workflow/config_helpers.go index d00d1f88b42..42e4f9827ac 100644 --- a/pkg/workflow/config_helpers.go +++ b/pkg/workflow/config_helpers.go @@ -31,6 +31,9 @@ // Configuration Integer Parsing: // - parseExpiresFromConfig() - Extract expiration time // - parseRelativeTimeSpec() - Parse relative time specifications +// +// Parser Scaffold: +// - parseConfigScaffold() - Generic safe-output config parser scaffold package workflow @@ -241,3 +244,48 @@ func unmarshalConfig(m map[string]any, key string, target any, log *logger.Logge return nil } + +// parseConfigScaffold is the generic parser scaffold for safe-output handler config parsers. +// It implements the common four-step pattern shared across safe-output handlers: +// 1. Key existence check – returns nil immediately if the key is absent in outputMap +// 2. Entry log: "Parsing configuration" +// 3. Typed unmarshal via unmarshalConfig into a zero-value T +// 4. On unmarshal error: delegate to the onError callback, which handles +// additional logging and returns the appropriate fallback (or nil to disable) +// +// The caller is responsible for any preprocessing (e.g. preprocessIntFieldAsString) +// that must happen before YAML unmarshaling, and for any postprocessing (e.g. setting +// default max values or computing derived fields) after a successful parse. +// +// Example – empty-config fallback: +// +// config := parseConfigScaffold(outputMap, "add-labels", addLabelsLog, +// func(err error) *AddLabelsConfig { +// addLabelsLog.Printf("Failed to unmarshal config: %v", err) +// addLabelsLog.Print("Using empty configuration (allows any labels)") +// return &AddLabelsConfig{} +// }) +// +// Example – disable-on-error: +// +// config := parseConfigScaffold(outputMap, "set-issue-type", setIssueTypeLog, +// func(err error) *SetIssueTypeConfig { +// setIssueTypeLog.Printf("Failed to unmarshal config, disabling handler: %v", err) +// return nil +// }) +func parseConfigScaffold[T any]( + outputMap map[string]any, + key string, + log *logger.Logger, + onError func(err error) *T, +) *T { + if _, exists := outputMap[key]; !exists { + return nil + } + log.Printf("Parsing %s configuration", key) + var config T + if err := unmarshalConfig(outputMap, key, &config, log); err != nil { + return onError(err) + } + return &config +} diff --git a/pkg/workflow/config_scaffold_helpers_test.go b/pkg/workflow/config_scaffold_helpers_test.go new file mode 100644 index 00000000000..ea9ff1401d8 --- /dev/null +++ b/pkg/workflow/config_scaffold_helpers_test.go @@ -0,0 +1,136 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/github/gh-aw/pkg/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testScaffoldConfig is a minimal config type used only in scaffold helper tests. +type testScaffoldConfig struct { + Name string `yaml:"name,omitempty"` + Allowed []string `yaml:"allowed,omitempty"` +} + +var testScaffoldLog = logger.New("workflow:test_scaffold") + +func TestParseConfigScaffold_KeyAbsent(t *testing.T) { + outputMap := map[string]any{ + "other-key": map[string]any{"name": "value"}, + } + + result := parseConfigScaffold(outputMap, "my-key", testScaffoldLog, + func(err error) *testScaffoldConfig { + t.Error("onError should not be called when key is absent") + return nil + }) + + assert.Nil(t, result, "should return nil when key is absent") +} + +func TestParseConfigScaffold_KeyPresentNullValue(t *testing.T) { + // Null YAML value (nil) should unmarshal into a zero-value struct successfully + outputMap := map[string]any{ + "my-key": nil, + } + + onErrorCalled := false + result := parseConfigScaffold(outputMap, "my-key", testScaffoldLog, + func(err error) *testScaffoldConfig { + onErrorCalled = true + return nil + }) + + assert.False(t, onErrorCalled, "onError should not be called for nil (null) config value") + require.NotNil(t, result, "should return non-nil zero-value config for null config") + assert.Empty(t, result.Name, "zero-value Name should be empty string") +} + +func TestParseConfigScaffold_ValidConfig(t *testing.T) { + outputMap := map[string]any{ + "my-key": map[string]any{ + "name": "test-name", + "allowed": []any{"foo", "bar"}, + }, + } + + onErrorCalled := false + result := parseConfigScaffold(outputMap, "my-key", testScaffoldLog, + func(err error) *testScaffoldConfig { + onErrorCalled = true + return nil + }) + + assert.False(t, onErrorCalled, "onError should not be called for valid config") + require.NotNil(t, result, "should return parsed config") + assert.Equal(t, "test-name", result.Name, "should parse name field") + assert.Equal(t, []string{"foo", "bar"}, result.Allowed, "should parse allowed array") +} + +func TestParseConfigScaffold_EmptyConfigFallback(t *testing.T) { + // A non-map value causes unmarshal to fail → onError must return empty fallback + outputMap := map[string]any{ + "my-key": "not-a-map", + } + + fallback := &testScaffoldConfig{Name: "fallback"} + result := parseConfigScaffold(outputMap, "my-key", testScaffoldLog, + func(err error) *testScaffoldConfig { + assert.Error(t, err, "onError should receive the unmarshal error") + return fallback + }) + + assert.Equal(t, fallback, result, "should return the fallback from onError") +} + +func TestParseConfigScaffold_DisableOnError(t *testing.T) { + // A non-map value causes unmarshal to fail → onError returns nil (disable handler) + outputMap := map[string]any{ + "my-key": "not-a-map", + } + + result := parseConfigScaffold(outputMap, "my-key", testScaffoldLog, + func(err error) *testScaffoldConfig { + return nil + }) + + assert.Nil(t, result, "should return nil when onError disables the handler") +} + +func TestParseConfigScaffold_OnErrorReceivesError(t *testing.T) { + outputMap := map[string]any{ + "my-key": "not-a-map", + } + + var capturedErr error + parseConfigScaffold(outputMap, "my-key", testScaffoldLog, + func(err error) *testScaffoldConfig { + capturedErr = err + return nil + }) + + require.Error(t, capturedErr, "onError should receive a non-nil error") +} + +func TestParseConfigScaffold_EmptyMap(t *testing.T) { + // An empty map should unmarshal into a zero-value struct successfully + outputMap := map[string]any{ + "my-key": map[string]any{}, + } + + onErrorCalled := false + result := parseConfigScaffold(outputMap, "my-key", testScaffoldLog, + func(err error) *testScaffoldConfig { + onErrorCalled = true + return nil + }) + + assert.False(t, onErrorCalled, "onError should not be called for empty map config") + require.NotNil(t, result, "should return non-nil zero-value config for empty map") + assert.Empty(t, result.Name, "zero-value Name should be empty string") + assert.Nil(t, result.Allowed, "zero-value Allowed should be nil") +} diff --git a/pkg/workflow/mark_pull_request_as_ready_for_review.go b/pkg/workflow/mark_pull_request_as_ready_for_review.go index 85e2b833c1e..f1f32464e0d 100644 --- a/pkg/workflow/mark_pull_request_as_ready_for_review.go +++ b/pkg/workflow/mark_pull_request_as_ready_for_review.go @@ -15,38 +15,31 @@ type MarkPullRequestAsReadyForReviewConfig struct { // parseMarkPullRequestAsReadyForReviewConfig handles mark-pull-request-as-ready-for-review configuration func (c *Compiler) parseMarkPullRequestAsReadyForReviewConfig(outputMap map[string]any) *MarkPullRequestAsReadyForReviewConfig { - // Check if the key exists - if _, exists := outputMap["mark-pull-request-as-ready-for-review"]; !exists { - markPullRequestAsReadyForReviewLog.Print("No configuration found for mark-pull-request-as-ready-for-review") + config := parseConfigScaffold(outputMap, "mark-pull-request-as-ready-for-review", markPullRequestAsReadyForReviewLog, + func(err error) *MarkPullRequestAsReadyForReviewConfig { + return nil + }) + if config == nil { return nil } - markPullRequestAsReadyForReviewLog.Print("Parsing mark-pull-request-as-ready-for-review configuration") - - // Get the configuration map + // Postprocess: parse common target configuration (target, target-repo) and + // filter configuration (required-labels, required-title-prefix) from the raw map, + // as these fields require additional extraction beyond YAML unmarshaling. var configMap map[string]any if configVal, exists := outputMap["mark-pull-request-as-ready-for-review"]; exists { if cfgMap, ok := configVal.(map[string]any); ok { configMap = cfgMap } else { - // Handle null case - use empty config configMap = make(map[string]any) } } - // Unmarshal into typed config struct - var config MarkPullRequestAsReadyForReviewConfig - if err := unmarshalConfig(outputMap, "mark-pull-request-as-ready-for-review", &config, markPullRequestAsReadyForReviewLog); err != nil { - return nil - } - - // Parse common target configuration (target, target-repo) targetConfig, _ := ParseTargetConfig(configMap) config.SafeOutputTargetConfig = targetConfig - // Parse filter configuration (required-labels, required-title-prefix) filterConfig := ParseFilterConfig(configMap) config.SafeOutputFilterConfig = filterConfig - return &config + return config } diff --git a/pkg/workflow/remove_labels.go b/pkg/workflow/remove_labels.go index c4bbdb42f5e..1369d893ae7 100644 --- a/pkg/workflow/remove_labels.go +++ b/pkg/workflow/remove_labels.go @@ -16,23 +16,14 @@ type RemoveLabelsConfig struct { // parseRemoveLabelsConfig handles remove-labels configuration func (c *Compiler) parseRemoveLabelsConfig(outputMap map[string]any) *RemoveLabelsConfig { - // Check if the key exists - if _, exists := outputMap["remove-labels"]; !exists { - return nil - } - - removeLabelsLog.Print("Parsing remove-labels configuration") - - // Unmarshal into typed config struct - var config RemoveLabelsConfig - if err := unmarshalConfig(outputMap, "remove-labels", &config, removeLabelsLog); err != nil { + config := parseConfigScaffold(outputMap, "remove-labels", removeLabelsLog, func(err error) *RemoveLabelsConfig { removeLabelsLog.Printf("Failed to unmarshal config: %v", err) // Handle null case: create empty config (allows any labels) removeLabelsLog.Print("Using empty configuration (allows any labels)") return &RemoveLabelsConfig{} + }) + if config != nil { + removeLabelsLog.Printf("Parsed configuration: allowed_count=%d, blocked_count=%d, target=%s", len(config.Allowed), len(config.Blocked), config.Target) } - - removeLabelsLog.Printf("Parsed configuration: allowed_count=%d, blocked_count=%d, target=%s", len(config.Allowed), len(config.Blocked), config.Target) - - return &config + return config } diff --git a/pkg/workflow/set_issue_type.go b/pkg/workflow/set_issue_type.go index 116518da2fb..1058a593cae 100644 --- a/pkg/workflow/set_issue_type.go +++ b/pkg/workflow/set_issue_type.go @@ -15,21 +15,12 @@ type SetIssueTypeConfig struct { // parseSetIssueTypeConfig handles set-issue-type configuration func (c *Compiler) parseSetIssueTypeConfig(outputMap map[string]any) *SetIssueTypeConfig { - // Check if the key exists - if _, exists := outputMap["set-issue-type"]; !exists { - return nil - } - - setIssueTypeLog.Print("Parsing set-issue-type configuration") - - // Unmarshal into typed config struct - var config SetIssueTypeConfig - if err := unmarshalConfig(outputMap, "set-issue-type", &config, setIssueTypeLog); err != nil { + config := parseConfigScaffold(outputMap, "set-issue-type", setIssueTypeLog, func(err error) *SetIssueTypeConfig { setIssueTypeLog.Printf("Failed to unmarshal set-issue-type config, disabling handler: %v", err) return nil + }) + if config != nil { + setIssueTypeLog.Printf("Parsed configuration: allowed_count=%d, target=%s", len(config.Allowed), config.Target) } - - setIssueTypeLog.Printf("Parsed configuration: allowed_count=%d, target=%s", len(config.Allowed), config.Target) - - return &config + return config } diff --git a/pkg/workflow/unassign_from_user.go b/pkg/workflow/unassign_from_user.go index 25d53d3403c..8a42735f112 100644 --- a/pkg/workflow/unassign_from_user.go +++ b/pkg/workflow/unassign_from_user.go @@ -16,22 +16,16 @@ type UnassignFromUserConfig struct { // parseUnassignFromUserConfig handles unassign-from-user configuration func (c *Compiler) parseUnassignFromUserConfig(outputMap map[string]any) *UnassignFromUserConfig { - // Check if the key exists - if _, exists := outputMap["unassign-from-user"]; !exists { - return nil - } - - unassignFromUserLog.Print("Parsing unassign-from-user configuration") - - // Unmarshal into typed config struct - var config UnassignFromUserConfig - if err := unmarshalConfig(outputMap, "unassign-from-user", &config, unassignFromUserLog); err != nil { + config := parseConfigScaffold(outputMap, "unassign-from-user", unassignFromUserLog, func(err error) *UnassignFromUserConfig { unassignFromUserLog.Printf("Failed to unmarshal config: %v", err) // For backward compatibility, use defaults unassignFromUserLog.Print("Using default configuration") - config = UnassignFromUserConfig{ + return &UnassignFromUserConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(1)}, } + }) + if config == nil { + return nil } // Set default max if not specified @@ -41,5 +35,5 @@ func (c *Compiler) parseUnassignFromUserConfig(outputMap map[string]any) *Unassi unassignFromUserLog.Printf("Parsed configuration: allowed_count=%d, target=%s", len(config.Allowed), config.Target) - return &config + return config }