Refactor: Eliminate 447 lines of duplicate safe output config handling

[Copilot]

Refactor Safe Output Configuration - Phase 1 Complete ✅

Summary

Successfully eliminated ~450 lines of duplicate code in compiler_safe_outputs_config.go by implementing a generic, reflection-based configuration builder. This addresses the first major refactoring opportunity identified in the issue.

Implementation Statistics

Metric	Before	After	Change
compiler_safe_outputs_config.go	476 lines	29 lines	-94% (447 lines removed)
Duplicate handler blocks	18 blocks × ~25 lines	1 generic function	18→1 implementations
Code added	-	570 lines	New builder + tests
Net change	476 lines	599 lines	+123 lines (well-tested, reusable)

Files Created

1. pkg/workflow/safe_output_config_builder.go (256 lines)

   • Generic buildHandlerConfig() using reflection
   • Handles embedded structs (BaseSafeOutputConfig, SafeOutputTargetConfig, etc.)
   • Handler registry with customizers for special cases
   • Field mapping for struct-to-config translation

2. pkg/workflow/safe_output_config_builder_test.go (314 lines)

   • Comprehensive test coverage
   • All config types tested (CreateIssues, AddComments, UpdateIssues, etc.)
   • Edge cases (nil pointers, empty configs, embedded structs)
   • Validation of customizers and defaults

Key Features

✅ Automatic field extraction via reflection
✅ Handles embedded structs recursively
✅ Boolean pointer support for allow_* fields
✅ Customizers for special cases (base_branch, defaults)
✅ Type-safe with compile-time checking
✅ Zero regressions - all tests pass
✅ Properly formatted and linted

Adding New Safe Output Types

Before (required ~25 lines of duplicate code):

if data.SafeOutputs.NewType != nil {
    cfg := data.SafeOutputs.NewType
    handlerConfig := make(map[string]any)
    if cfg.Max > 0 {
        handlerConfig["max"] = cfg.Max
    }
    if cfg.Field != "" {
        handlerConfig["field"] = cfg.Field
    }
    // ... 20 more lines ...
    config["new_type"] = handlerConfig
}

After (requires 1 line):

{fieldName: "NewType", handlerName: "new_type"},

Benefits

1. Maintainability: Adding new safe output types is trivial
2. Consistency: All handlers use identical extraction logic
3. Testing: Generic approach validated once, applies to all
4. Readability: 17-line function vs 460-line monster
5. Extensibility: Easy to add new field types or behaviors

Test Coverage

All existing tests pass:

• ✅ TestAddHandlerManagerConfigEnvVar (11 test cases)
• ✅ TestBuildHandlerManagerStep (2 test cases)
• ✅ TestBuildSafeOutputConfigs (7 new test cases)
• ✅ TestBuildHandlerConfig (6 new test cases)
• ✅ All safe output integration tests

Code Quality

• ✅ Properly formatted with gofmt
• ✅ Passes golangci-lint with zero issues
• ✅ Well-documented with clear comments
• ✅ Follows Go best practices

---

Impact: This refactoring eliminates a major source of technical debt and establishes a pattern for future consolidation.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[refactor] Semantic Function Clustering Analysis - Major Refactoring Opportunities Identified</issue_title>
<issue_description>## Executive Summary

Analysis of 220 non-test Go files in the pkg/ directory has identified significant opportunities for code consolidation and improved organization. The most critical finding is ~2,000 lines of duplicated code in safe output configuration handling, with repetitive patterns across 60+ nearly identical functions.

Full Report

Analysis Overview

• Total Go files analyzed: 220 (excluding tests)
• Total functions cataloged: 1,200+
• Function clusters identified: 8 major clusters
• Critical duplications found: 5 high-impact areas
• Files needing reorganization: 47
• Detection method: Pattern analysis + semantic code review

---

1. Critical Issues - High Priority

🔴 Issue #1: Massive Safe Output Config Duplication

Scope: 19 files with 60+ nearly identical functions
Lines of duplicate code: ~2,000
Impact: High - makes maintenance difficult and error-prone

Problem Details

The safe output configuration system has severe code duplication:

1. Single 476-line mega-function in pkg/workflow/compiler_safe_outputs_config.go:17-476

   • Function: addHandlerManagerConfigEnvVar()
   • Contains 15 nearly identical if-blocks, one per output type
   • Each block follows identical pattern with minor variations

2. 60+ duplicate parse*Config() functions across 30+ files

   • Files: create_issue.go, create_discussion.go, create_pull_request.go, etc.
   • Pattern: 80-90% identical code structure
   • Each has same boilerplate for expires handling, base config parsing, validation

3. 37+ duplicate build*Job() functions

   • Pattern: Similar job-building logic repeated
   • Note: BuildListSafeOutputJob() exists in safe_output_builder.go but is underutilized

Example: Repetitive Code in compiler_safe_outputs_config.go

// Lines 17-48 - CreateIssues handler
if data.SafeOutputs.CreateIssues != nil {
    cfg := data.SafeOutputs.CreateIssues
    handlerConfig := make(map[string]any)
    if cfg.Max > 0 { handlerConfig["max"] = cfg.Max }
    if len(cfg.AllowedLabels) > 0 { handlerConfig["allowed_labels"] = cfg.AllowedLabels }
    if cfg.RequiredTitlePrefix != "" { handlerConfig["required-title-prefix"] = cfg.RequiredTitlePrefix }
    // ... 8 more similar lines
    config["create_issue"] = handlerConfig
}

// Lines 50-70 - AddComments handler (nearly identical!)
if data.SafeOutputs.AddComments != nil {
    cfg := data.SafeOutputs.AddComments
    handlerConfig := make(map[string]any)
    if cfg.Max > 0 { handlerConfig["max"] = cfg.Max }
    // ... same pattern repeated
    config["add_comment"] = handlerConfig
}

// REPEATS 13 MORE TIMES with minor variations

Example: Duplicate Parse Pattern

Found in create_issue.go:23-50, create_discussion.go:23-50, create_pull_request.go:23-50, and 10+ other files:

func (c *Compiler) parseXConfig(outputMap map[string]any) *XConfig {
    if _, exists := outputMap["x"]; !exists { 
        return nil 
    }
    
    log.Print("Parsing x configuration")
    configData, _ := outputMap["x"].(map[string]any)
    
    // Expires handling - DUPLICATED in 10+ files
    if configData != nil {
        if expires, exists := configData["expires"]; exists {
            if _, ok := expires.(string); ok {
                expiresInt := parseExpiresFromConfig(configData)
                if expiresInt > 0 {
                    configData["expires"] = expiresInt
                }
            }
        }
    }
    
    var config XConfig
    if err := unmarshalConfig(outputMap, "x", &config, log); err != nil {
        return nil
    }
    
    c.parseBaseSafeOutputConfig(configData, &config.BaseSafeOutputConfig, defaultMax)
    return &config
}

This pattern is repeated 30+ times with only the type name changing!

Recommendation

Refactor using generics and registry pattern:

1. Create generic config parser in pkg/workflow/config_helpers.go:

   func ParseSafeOutputConfig[T any](
       outputMap map[string]any,
       key string,
       defaultMax int,
       preprocessor func(map[string]any),
   ) (*T, error)

2. Replace 476-line function with handler registry:

   type HandlerConfigBuilder interface {
       BuildConfig(cfg any) map[string]any
   }
   
   var handlerRegistry = map[string]HandlerConfigBuilder{
       "create_issue": &CreateIssueBuilder{},
       "add_comment": &AddCommentBuilder{},
       // ... register all handlers
   }

3. Consolidate into subdirectory structure:

   pkg/workflow/safe_outputs/
     config/
       parser.go          # Generic config parser
       builder.go         # Generic handler config builder
       valida...
   
   

• Fixes [refactor] Semantic Function Clustering Analysis - Major Refactoring Opportunities Identified #9254

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[pelikhan]

@copilot can we define a struct and marshal?

[Copilot]

@copilot can we define a struct and marshal?

Good suggestion! I explored using JSON marshaling + struct tags instead of manual reflection. However, the config structs currently only have yaml tags, not json tags, which means JSON marshaling uses Go field names (PascalCase) instead of the desired snake_case format.

Two options:

1. Add json tags to all safe output config structs (18+ structs across multiple files) - more invasive but cleaner
2. Keep the reflection approach with the field mapping registry - more code but localized to one file

Which would you prefer? The reflection approach is already working and well-tested. Adding JSON tags would be cleaner long-term but requires changes to many struct definitions.

[pelikhan]

@copilot format, lint

[Copilot]

@copilot format, lint

Done! Formatted and linted the code in commit 89aa836.

Changes:

• Applied gofmt to align field mappings and struct literals
• Verified with golangci-lint - zero issues
• All tests still pass