Deduplicate safe-output parser wrapper boilerplate with parseConfigScaffoldWithPostProcess

docs/adr/52158-deduplicate-safe-output-parser-wrappers-with-post-process-helper.md

@@ -0,0 +1,47 @@
+# ADR-52158: Deduplicate Safe-Output Parser Wrappers with a Post-Process Helper
+
+**Date**: 2026-08-11
+**Status**: Proposed — pending maintainer acceptance on merge
+**Deciders**: pelikhan (via copilot-swe-agent, PR #52158)
+
+---
+
+### Context
+
+The `pkg/workflow` package provides safe-output handlers for GitHub Actions workflow steps. Each handler calls the shared `parseConfigScaffold` helper to unmarshal its YAML config, but then repeats the same boilerplate: check whether the returned pointer is nil, apply default field values, and emit a post-parse debug log line. This pattern appeared identically across 13+ non-test files (`add_comment.go`, `add_labels.go`, `add_reviewer.go`, `assign_milestone.go`, `assign_to_agent.go`, `assign_to_user.go`, `close_entity_helpers.go`, `mark_pull_request_as_ready_for_review.go`, `remove_labels.go`, `replace_label.go`, `set_issue_field.go`, `set_issue_type.go`, `unassign_from_user.go`). Because each copy was hand-maintained, defaulting rules and log formatting could drift silently when one handler received a fix that peers did not.
+
+### Decision
+
+We will introduce `parseConfigScaffoldWithPostProcess[T any]` in `pkg/workflow/config_helpers.go`. This generic wrapper calls `parseConfigScaffold`, and if the result is non-nil and a `postProcess` callback is provided, invokes that callback before returning. All 13+ existing callers are refactored to use the new helper, moving their nil-check, default-value assignment, and post-parse logging into the `postProcess` closure. Handlers with genuinely unique pre-parse logic (e.g., `create_entity_helpers.go`) are left unchanged. No behavioral changes are introduced; existing defaults, error messages, and log formats are preserved exactly.
+
+The callback contract is: `postProcess` runs for **every** non-nil result, including a non-nil fallback returned by `onError`, and is skipped when the result is nil (key absent, or `onError` returned nil to disable the handler). This keeps defaulting and logging consistent between successfully parsed configs and error fallbacks. The contract is pinned down by direct tests in `config_scaffold_helpers_test.go` covering valid config, non-nil error fallback, nil error fallback, absent key, and a nil callback.
+
+### Alternatives Considered
+
+#### Alternative 1: Extend `parseConfigScaffold` directly with an optional postProcess parameter
+
+Add a `postProcess func(config *T)` parameter to the existing `parseConfigScaffold` signature so all callers can supply post-processing inline without a new function.
+
+This was not chosen because changing `parseConfigScaffold`'s signature would require updating every existing call site (including those that pass `nil` or do not need post-processing), and would complicate the function's existing contract around nil handling and error fallback. A thin wrapper preserves backward compatibility and keeps `parseConfigScaffold` focused.
+
+#### Alternative 2: Define a `PostProcessable` interface on each config type
+
+Give each config struct a `PostProcess()` method implementing a shared interface, and have `parseConfigScaffold` detect and call it via a type assertion or a separate generic constraint.
+
+This was not chosen because it would scatter post-processing logic across many struct definitions rather than co-locating it with the handler's parse function. It also increases coupling between the generic scaffold and the concrete config types, making the scaffold harder to reason about in isolation.
+
+### Consequences
+
+#### Positive
+- The nil-check-and-callback wrapper is written once and reused across all handlers, eliminating 13+ nearly identical code blocks.
+- Default-value policy and post-parse logging conventions can be changed in a single `postProcess` closure per handler rather than tracked across many files.
+- New handlers automatically follow the pattern by passing a `postProcess` closure instead of copying boilerplate.
+- Handler files are shorter and easier to read.
+
+#### Negative
+- `config_helpers.go` grows by one additional generic function, adding to the surface area readers must understand when onboarding.
+- Handlers with unusual post-processing (e.g., extracting nested map keys as in `mark_pull_request_as_ready_for_review.go`) must still write non-trivial closures, so the helper provides less benefit for those cases.
+
+#### Neutral
+- `create_entity_helpers.go` is explicitly excluded from the refactor because it already owns a distinct `parseCreateEntityConfig` scaffold with pre- and post-hooks; the two scaffolds coexist without conflict.
+- The refactor is purely internal to `pkg/workflow`; no public API or configuration schema changes.

pkg/workflow/add_comment.go

@@ -69,19 +69,18 @@ func (c *Compiler) parseCommentsConfig(outputMap map[string]any) *AddCommentsCon
 		return nil
 	}
 
-	config := parseConfigScaffold(outputMap, "add-comment", addCommentLog, func(err error) *AddCommentsConfig {
-		addCommentLog.Printf("Failed to unmarshal config: %v", err)
-		// For backward compatibility, handle nil/empty config
-		return &AddCommentsConfig{}
-	})
-	if config == nil {
-		return nil
-	}
-
-	// Set default max if not specified
-	if config.Max == nil {
-		config.Max = defaultIntStr(1)
-	}
+	config := parseConfigScaffoldWithPostProcess(outputMap, "add-comment", addCommentLog,
+		func(err error) *AddCommentsConfig {
+			addCommentLog.Printf("Failed to unmarshal config: %v", err)
+			// For backward compatibility, handle nil/empty config
+			return &AddCommentsConfig{}
+		},
+		func(config *AddCommentsConfig) {
+			// Set default max if not specified
+			if config.Max == nil {
+				config.Max = defaultIntStr(1)
+			}
+		})
 
 	return config
 }

pkg/workflow/add_labels.go

@@ -20,16 +20,16 @@ type AddLabelsConfig struct {
 
 // parseAddLabelsConfig handles add-labels configuration
 func (c *Compiler) parseAddLabelsConfig(outputMap map[string]any) *AddLabelsConfig {
-	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)
-	}
-	return config
+	return parseConfigScaffoldWithPostProcess(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{}
+		},
+		func(config *AddLabelsConfig) {
+			addLabelsLog.Printf("Parsed configuration: allowed_count=%d, blocked_count=%d, target=%s", len(config.Allowed), len(config.Blocked), config.Target)
+		})
 }
 
 // buildAddLabelsPermissions computes the permissions for add_labels based on config.

pkg/workflow/add_reviewer.go

@@ -51,29 +51,28 @@ func (c *Compiler) parseAddReviewerConfig(outputMap map[string]any) *AddReviewer
 		return nil
 	}
 
-	config := parseConfigScaffold(outputMap, "add-reviewer", addReviewerLog, func(err error) *AddReviewerConfig {
-		addReviewerLog.Printf("Failed to unmarshal config: %v", err)
-		// For backward compatibility, handle nil/empty config
-		return &AddReviewerConfig{}
-	})
-	if config == nil {
-		return nil
-	}
-
-	// Set default max if not specified
-	if config.Max == nil {
-		config.Max = defaultIntStr(3)
-	}
+	config := parseConfigScaffoldWithPostProcess(outputMap, "add-reviewer", addReviewerLog,
+		func(err error) *AddReviewerConfig {
+			addReviewerLog.Printf("Failed to unmarshal config: %v", err)
+			// For backward compatibility, handle nil/empty config
+			return &AddReviewerConfig{}
+		},
+		func(config *AddReviewerConfig) {
+			// Set default max if not specified
+			if config.Max == nil {
+				config.Max = defaultIntStr(3)
+			}
 
-	// Fallback from deprecated field names to preferred names
-	if len(config.AllowedReviewers) == 0 {
-		config.AllowedReviewers = config.Reviewers
-	}
-	if len(config.AllowedTeamReviewers) == 0 {
-		config.AllowedTeamReviewers = config.TeamReviewers
-	}
+			// Fallback from deprecated field names to preferred names
+			if len(config.AllowedReviewers) == 0 {
+				config.AllowedReviewers = config.Reviewers
+			}
+			if len(config.AllowedTeamReviewers) == 0 {
+				config.AllowedTeamReviewers = config.TeamReviewers
+			}
 
-	addReviewerLog.Printf("Parsed add-reviewer config: allowed_reviewers=%d, target=%s", len(config.AllowedReviewers), config.Target)
+			addReviewerLog.Printf("Parsed add-reviewer config: allowed_reviewers=%d, target=%s", len(config.AllowedReviewers), config.Target)
+		})
 
 	return config
 }

pkg/workflow/assign_milestone.go

@@ -17,15 +17,15 @@ type AssignMilestoneConfig struct {
 
 // parseAssignMilestoneConfig handles assign-milestone configuration
 func (c *Compiler) parseAssignMilestoneConfig(outputMap map[string]any) *AssignMilestoneConfig {
-	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))
-	}
-	return config
+	return parseConfigScaffoldWithPostProcess(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{}
+		},
+		func(config *AssignMilestoneConfig) {
+			assignMilestoneLog.Printf("Parsed milestone config: target=%s, allowed_count=%d",
+				config.Target, len(config.Allowed))
+		})
 }

pkg/workflow/assign_to_agent.go

@@ -37,22 +37,21 @@ func (c *Compiler) parseAssignToAgentConfig(outputMap map[string]any) *AssignToA
 		return 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
-	if config.Max == nil {
-		config.Max = defaultIntStr(1)
-	}
-
-	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)
+	config := parseConfigScaffoldWithPostProcess(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{}
+		},
+		func(config *AssignToAgentConfig) {
+			// Set default max if not specified
+			if config.Max == nil {
+				config.Max = defaultIntStr(1)
+			}
+
+			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
 }

pkg/workflow/assign_to_user.go

@@ -37,24 +37,23 @@ func (c *Compiler) parseAssignToUserConfig(outputMap map[string]any) *AssignToUs
 		return 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")
-		return &AssignToUserConfig{
-			BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(1)},
-		}
-	})
-	if config == nil {
-		return nil
-	}
-
-	// Set default max if not specified
-	if config.Max == nil {
-		config.Max = defaultIntStr(1)
-	}
-
-	assignToUserLog.Printf("Parsed configuration: allowed_count=%d, target=%s", len(config.Allowed), config.Target)
+	config := parseConfigScaffoldWithPostProcess(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")
+			return &AssignToUserConfig{
+				BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(1)},
+			}
+		},
+		func(config *AssignToUserConfig) {
+			// Set default max if not specified
+			if config.Max == nil {
+				config.Max = defaultIntStr(1)
+			}
+
+			assignToUserLog.Printf("Parsed configuration: allowed_count=%d, target=%s", len(config.Allowed), config.Target)
+		})
 
 	return config
 }

pkg/workflow/close_entity_helpers.go

@@ -129,27 +129,26 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close
 		}
 	}
 
-	config := parseConfigScaffold(outputMap, params.ConfigKey, logger, func(err error) *CloseEntityConfig {
-		logger.Printf("Failed to unmarshal config: %v", err)
-		// For backward compatibility, handle nil/empty config
-		return &CloseEntityConfig{}
-	})
-	if config == nil {
-		return nil
-	}
-
-	// Set default max if not specified
-	if config.Max == nil {
-		config.Max = defaultIntStr(1)
-		logger.Printf("Set default max to 1 for %s", params.ConfigKey)
-	}
+	config := parseConfigScaffoldWithPostProcess(outputMap, params.ConfigKey, logger,
+		func(err error) *CloseEntityConfig {
+			logger.Printf("Failed to unmarshal config: %v", err)
+			// For backward compatibility, handle nil/empty config
+			return &CloseEntityConfig{}
+		},
+		func(config *CloseEntityConfig) {
+			// Set default max if not specified
+			if config.Max == nil {
+				config.Max = defaultIntStr(1)
+				logger.Printf("Set default max to 1 for %s", params.ConfigKey)
+			}
 
-	// Backward compatibility: map deprecated title-prefix to required-title-prefix.
-	if config.RequiredTitlePrefix == "" && config.TitlePrefix != "" {
-		config.RequiredTitlePrefix = config.TitlePrefix
-	}
+			// Backward compatibility: map deprecated title-prefix to required-title-prefix.
+			if config.RequiredTitlePrefix == "" && config.TitlePrefix != "" {
+				config.RequiredTitlePrefix = config.TitlePrefix
+			}
 
-	logger.Printf("Parsed %s configuration: max=%s, target=%s", params.ConfigKey, *config.Max, config.Target)
+			logger.Printf("Parsed %s configuration: max=%s, target=%s", params.ConfigKey, *config.Max, config.Target)
+		})
 
 	return config
 }

pkg/workflow/config_helpers.go

@@ -31,6 +31,8 @@
 //
 // Parser Scaffold:
 //   - parseConfigScaffold() - Generic safe-output config parser scaffold
+//   - parseConfigScaffoldWithPostProcess() - parseConfigScaffold plus an optional
+//     post-parse callback for defaulting/logging, avoiding repeated nil-check wrappers
 
 package workflow
 
@@ -278,3 +280,40 @@ func parseConfigScaffold[T any](
 	}
 	return &config
 }
+
+// parseConfigScaffoldWithPostProcess wraps parseConfigScaffold and additionally invokes the
+// optional postProcess callback for every non-nil result. This includes non-nil fallback
+// configs returned by onError, not just successfully unmarshalled ones, so that defaults and
+// logging apply consistently to fallbacks. postProcess is skipped when the result is nil
+// (key absent, or onError returned nil to disable the handler).
+//
+// This removes the repeated "if config == nil { return nil } ... return config" wrapper that
+// most safe-output handlers need around parseConfigScaffold for applying default values and/or
+// emitting a post-parse debug log line.
+//
+// postProcess may be nil if no post-processing is required.
+//
+// Example:
+//
+//	return parseConfigScaffoldWithPostProcess(outputMap, "assign-milestone", assignMilestoneLog,
+//	    func(err error) *AssignMilestoneConfig {
+//	        assignMilestoneLog.Printf("Failed to unmarshal config: %v", err)
+//	        return &AssignMilestoneConfig{}
+//	    },
+//	    func(config *AssignMilestoneConfig) {
+//	        assignMilestoneLog.Printf("Parsed milestone config: target=%s, allowed_count=%d",
+//	            config.Target, len(config.Allowed))
+//	    })
+func parseConfigScaffoldWithPostProcess[T any](
+	outputMap map[string]any,
+	key string,
+	debugLog *logger.Logger,
+	onError func(err error) *T,
+	postProcess func(config *T),
+) *T {
+	config := parseConfigScaffold(outputMap, key, debugLog, onError)
+	if config != nil && postProcess != nil {
+		postProcess(config)
+	}
+	return config
+}