Refactor safe output handlers into sequential processing with handler registry

pkg/workflow/compiler_safe_outputs_core.go

@@ -51,9 +51,8 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa
 	threatDetectionEnabled := data.SafeOutputs.ThreatDetection != nil
 
 	// Track which outputs are created for dependency tracking
+	// These are tracked in the handler processing context now
 	var createIssueEnabled bool
-	var createDiscussionEnabled bool
-	var createPullRequestEnabled bool
 
 	// Add GitHub App token minting step if app is configured
 	if data.SafeOutputs.App != nil {
@@ -102,53 +101,33 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa
 
 	// === Build individual safe output steps ===
 
-	// 1. Create Issue step
-	if data.SafeOutputs.CreateIssues != nil {
-		createIssueEnabled = true
-		stepConfig := c.buildCreateIssueStepConfig(data, mainJobName, threatDetectionEnabled)
-		stepYAML := c.buildConsolidatedSafeOutputStep(data, stepConfig)
-		steps = append(steps, stepYAML...)
-		safeOutputStepNames = append(safeOutputStepNames, stepConfig.StepID)
-
-		// Add outputs
-		outputs["create_issue_issue_number"] = "${{ steps.create_issue.outputs.issue_number }}"
-		outputs["create_issue_issue_url"] = "${{ steps.create_issue.outputs.issue_url }}"
-		outputs["create_issue_temporary_id_map"] = "${{ steps.create_issue.outputs.temporary_id_map }}"
-
-		// Merge permissions
-		permissions.Merge(NewPermissionsContentsReadIssuesWrite())
-	}
-
-	// 2. Create Discussion step
-	if data.SafeOutputs.CreateDiscussions != nil {
-		createDiscussionEnabled = true
-		stepConfig := c.buildCreateDiscussionStepConfig(data, mainJobName, threatDetectionEnabled)
-		stepYAML := c.buildConsolidatedSafeOutputStep(data, stepConfig)
-		steps = append(steps, stepYAML...)
-		safeOutputStepNames = append(safeOutputStepNames, stepConfig.StepID)
-
-		outputs["create_discussion_discussion_number"] = "${{ steps.create_discussion.outputs.discussion_number }}"
-		outputs["create_discussion_discussion_url"] = "${{ steps.create_discussion.outputs.discussion_url }}"
-
-		permissions.Merge(NewPermissionsContentsReadDiscussionsWrite())
-	}
-
-	// 2a. Update Discussion step
-	if data.SafeOutputs.UpdateDiscussions != nil {
-		stepConfig := c.buildUpdateDiscussionStepConfig(data, mainJobName, threatDetectionEnabled)
-		stepYAML := c.buildConsolidatedSafeOutputStep(data, stepConfig)
-		steps = append(steps, stepYAML...)
-		safeOutputStepNames = append(safeOutputStepNames, stepConfig.StepID)
-
-		outputs["update_discussion_discussion_number"] = "${{ steps.update_discussion.outputs.discussion_number }}"
-		outputs["update_discussion_discussion_url"] = "${{ steps.update_discussion.outputs.discussion_url }}"
-
-		permissions.Merge(NewPermissionsContentsReadDiscussionsWrite())
+	// Process migrated handlers through the new handler registry system
+	// This handles: create_issue, create_discussion, update_issue, update_discussion, add_comment
+	registry := getHandlerRegistry()
+	handlerSteps, handlerOutputs, handlerPermissions, handlerStepNames := c.processHandlersSequentially(
+		data, mainJobName, threatDetectionEnabled, registry)
+
+	// Append handler steps and merge outputs/permissions
+	steps = append(steps, handlerSteps...)
+	for k, v := range handlerOutputs {
+		outputs[k] = v
+	}
+	permissions.Merge(handlerPermissions)
+	safeOutputStepNames = append(safeOutputStepNames, handlerStepNames...)
+
+	// Track which handlers were processed for backward compatibility
+	for _, stepName := range handlerStepNames {
+		switch stepName {
+		case "create_issue":
+			createIssueEnabled = true
+			// Note: createDiscussionEnabled is no longer needed as dependencies
+			// are now managed through the SafeOutputContext
+		}
 	}
 
 	// 3. Create Pull Request step
 	if data.SafeOutputs.CreatePullRequests != nil {
-		createPullRequestEnabled = true
+		// Note: createPullRequestEnabled tracking is now handled by SafeOutputContext
 		stepConfig := c.buildCreatePullRequestStepConfig(data, mainJobName, threatDetectionEnabled)
 		// Skip pre-steps if we've already added the shared checkout steps
 		if !prCheckoutStepsAdded {
@@ -165,39 +144,8 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa
 		permissions.Merge(NewPermissionsContentsWriteIssuesWritePRWrite())
 	}
 
-	// 4. Add Comment step (needs to come after create_issue, create_discussion, create_pull_request)
-	if data.SafeOutputs.AddComments != nil {
-		stepConfig := c.buildAddCommentStepConfig(data, mainJobName, threatDetectionEnabled,
-			createIssueEnabled, createDiscussionEnabled, createPullRequestEnabled)
-		stepYAML := c.buildConsolidatedSafeOutputStep(data, stepConfig)
-		steps = append(steps, stepYAML...)
-		safeOutputStepNames = append(safeOutputStepNames, stepConfig.StepID)
-
-		outputs["add_comment_comment_id"] = "${{ steps.add_comment.outputs.comment_id }}"
-		outputs["add_comment_comment_url"] = "${{ steps.add_comment.outputs.comment_url }}"
-
-		permissions.Merge(NewPermissionsContentsReadIssuesWritePRWriteDiscussionsWrite())
-	}
-
-	// 5. Close Discussion step
-	if data.SafeOutputs.CloseDiscussions != nil {
-		stepConfig := c.buildCloseDiscussionStepConfig(data, mainJobName, threatDetectionEnabled)
-		stepYAML := c.buildConsolidatedSafeOutputStep(data, stepConfig)
-		steps = append(steps, stepYAML...)
-		safeOutputStepNames = append(safeOutputStepNames, stepConfig.StepID)
-
-		permissions.Merge(NewPermissionsContentsReadDiscussionsWrite())
-	}
-
-	// 6. Close Issue step
-	if data.SafeOutputs.CloseIssues != nil {
-		stepConfig := c.buildCloseIssueStepConfig(data, mainJobName, threatDetectionEnabled)
-		stepYAML := c.buildConsolidatedSafeOutputStep(data, stepConfig)
-		steps = append(steps, stepYAML...)
-		safeOutputStepNames = append(safeOutputStepNames, stepConfig.StepID)
-
-		permissions.Merge(NewPermissionsContentsReadIssuesWrite())
-	}
+	// NOTE: Add Comment, Update Issue, Update Discussion, Close Issue, and Close Discussion 
+	// are now handled by the handler registry above
 
 	// 7. Close Pull Request step
 	if data.SafeOutputs.ClosePullRequests != nil {
@@ -290,16 +238,6 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa
 		permissions.Merge(NewPermissionsContentsReadIssuesWritePRWrite())
 	}
 
-	// 15. Update Issue step
-	if data.SafeOutputs.UpdateIssues != nil {
-		stepConfig := c.buildUpdateIssueStepConfig(data, mainJobName, threatDetectionEnabled)
-		stepYAML := c.buildConsolidatedSafeOutputStep(data, stepConfig)
-		steps = append(steps, stepYAML...)
-		safeOutputStepNames = append(safeOutputStepNames, stepConfig.StepID)
-
-		permissions.Merge(NewPermissionsContentsReadIssuesWrite())
-	}
-
 	// 16. Update Pull Request step
 	if data.SafeOutputs.UpdatePullRequests != nil {
 		stepConfig := c.buildUpdatePullRequestStepConfig(data, mainJobName, threatDetectionEnabled)

pkg/workflow/safe_output_handler.go

@@ -0,0 +1,129 @@
+package workflow
+
+import (
+	"github.com/githubnext/gh-aw/pkg/logger"
+)
+
+var safeOutputHandlerLog = logger.New("workflow:safe_output_handler")
+
+// SafeOutputHandler defines the interface for safe output message handlers.
+// Each handler is responsible for building the steps needed to process
+// a specific type of safe output message (e.g., create_issue, add_comment).
+type SafeOutputHandler interface {
+	// GetType returns the type identifier for this handler (e.g., "create_issue")
+	GetType() string
+
+	// IsEnabled checks if this handler should be processed based on the workflow configuration
+	IsEnabled(data *WorkflowData) bool
+
+	// BuildStepConfig builds the step configuration for this handler
+	// Returns nil if the handler is not enabled or cannot be built
+	BuildStepConfig(c *Compiler, data *WorkflowData, ctx *SafeOutputContext) *SafeOutputStepConfig
+
+	// GetOutputs returns the outputs that this handler produces
+	// These are used to build the job outputs map
+	GetOutputs() map[string]string
+
+	// RequiresTempIDMap returns true if this handler needs access to the temporary ID map
+	RequiresTempIDMap() bool
+}
+
+// SafeOutputContext holds contextual information about previously processed handlers
+// This allows handlers to reference outputs from earlier handlers in the sequence
+type SafeOutputContext struct {
+	// ThreatDetectionEnabled indicates if threat detection is enabled
+	ThreatDetectionEnabled bool
+
+	// MainJobName is the name of the main agent job
+	MainJobName string
+
+	// ProcessedHandlers tracks which handlers have been processed so far
+	// Map key is the handler type (e.g., "create_issue")
+	ProcessedHandlers map[string]bool
+
+	// HandlerOutputs tracks the outputs from each processed handler
+	// This allows later handlers to reference earlier outputs
+	HandlerOutputs map[string]map[string]string
+
+	// TempIDMapAvailable indicates if a temporary ID map is available from a previous handler
+	TempIDMapAvailable bool
+
+	// TempIDMapSource is the step ID that provides the temporary ID map
+	TempIDMapSource string
+}
+
+// NewSafeOutputContext creates a new context for processing safe output handlers
+func NewSafeOutputContext(mainJobName string, threatDetectionEnabled bool) *SafeOutputContext {
+	return &SafeOutputContext{
+		ThreatDetectionEnabled: threatDetectionEnabled,
+		MainJobName:            mainJobName,
+		ProcessedHandlers:      make(map[string]bool),
+		HandlerOutputs:         make(map[string]map[string]string),
+		TempIDMapAvailable:     false,
+		TempIDMapSource:        "",
+	}
+}
+
+// MarkProcessed marks a handler as processed and records its outputs
+func (ctx *SafeOutputContext) MarkProcessed(handlerType string, outputs map[string]string) {
+	ctx.ProcessedHandlers[handlerType] = true
+	if len(outputs) > 0 {
+		ctx.HandlerOutputs[handlerType] = outputs
+	}
+}
+
+// IsProcessed checks if a handler has been processed
+func (ctx *SafeOutputContext) IsProcessed(handlerType string) bool {
+	return ctx.ProcessedHandlers[handlerType]
+}
+
+// GetHandlerOutput retrieves a specific output from a processed handler
+func (ctx *SafeOutputContext) GetHandlerOutput(handlerType, outputKey string) string {
+	if outputs, exists := ctx.HandlerOutputs[handlerType]; exists {
+		return outputs[outputKey]
+	}
+	return ""
+}
+
+// SetTempIDMapSource marks that a temporary ID map is available from a specific step
+func (ctx *SafeOutputContext) SetTempIDMapSource(stepID string) {
+	ctx.TempIDMapAvailable = true
+	ctx.TempIDMapSource = stepID
+}
+
+// SafeOutputHandlerRegistry manages the collection of safe output handlers
+type SafeOutputHandlerRegistry struct {
+	handlers []SafeOutputHandler
+	logger   *logger.Logger
+}
+
+// NewSafeOutputHandlerRegistry creates a new handler registry
+func NewSafeOutputHandlerRegistry() *SafeOutputHandlerRegistry {
+	return &SafeOutputHandlerRegistry{
+		handlers: make([]SafeOutputHandler, 0),
+		logger:   safeOutputHandlerLog,
+	}
+}
+
+// Register adds a handler to the registry
+func (r *SafeOutputHandlerRegistry) Register(handler SafeOutputHandler) {
+	r.handlers = append(r.handlers, handler)
+	r.logger.Printf("Registered safe output handler: %s", handler.GetType())
+}
+
+// GetHandlers returns all registered handlers in order
+func (r *SafeOutputHandlerRegistry) GetHandlers() []SafeOutputHandler {
+	return r.handlers
+}
+
+// GetEnabledHandlers returns only the handlers that are enabled for the given workflow
+func (r *SafeOutputHandlerRegistry) GetEnabledHandlers(data *WorkflowData) []SafeOutputHandler {
+	enabled := make([]SafeOutputHandler, 0)
+	for _, handler := range r.handlers {
+		if handler.IsEnabled(data) {
+			enabled = append(enabled, handler)
+			r.logger.Printf("Handler enabled: %s", handler.GetType())
+		}
+	}
+	return enabled
+}