refactor: consolidate duplicate provider extraction and remove redundant engine overrides (#28745)

Author: Copilot

pkg/workflow/agentic_engine.go

@@ -378,7 +378,10 @@ func (e *BaseEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubA
 }
 
 // GetFirewallLogsCollectionStep returns an empty slice by default.
-// Engines that need to copy session or firewall state files before secret redaction should override this.
+// Firewall logs are written to a known location (/tmp/gh-aw/sandbox/firewall/logs/)
+// and do not require a separate collection step. The method is still called from
+// compiler_yaml_main_job.go to maintain a consistent interface; engines that need
+// to copy session or firewall state files before secret redaction should override it.
 func (e *BaseEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData) []GitHubActionStep {
 	return []GitHubActionStep{}
 }

pkg/workflow/allowed_domains_sanitization_test.go

@@ -10,8 +10,8 @@ import (
 	"testing"
 
 	"github.com/github/gh-aw/pkg/stringutil"
-
 	"github.com/github/gh-aw/pkg/testutil"
+	"github.com/stretchr/testify/require"
 )
 
 // extractQuotedCSV returns the comma-separated domain list embedded inside
@@ -492,7 +492,8 @@ func TestComputeAllowedDomainsForSanitization(t *testing.T) {
 			}
 
 			// Call the function
-			domainsStr := compiler.computeAllowedDomainsForSanitization(data)
+			domainsStr, err := compiler.computeAllowedDomainsForSanitization(data)
+			require.NoError(t, err, "computeAllowedDomainsForSanitization should not return an error for valid test data")
 
 			// Verify expected domains are present (substring match is fine here since domain names
 			// in a CSV string that are exact entries won't appear as substrings of other entries

pkg/workflow/claude_engine.go

@@ -463,13 +463,6 @@ func (e *ClaudeEngine) GetLogParserScriptId() string {
 	return "parse_claude_log"
 }
 
-// GetFirewallLogsCollectionStep returns the step for collecting firewall logs (before secret redaction)
-// No longer needed since we know where the logs are in the sandbox folder structure
-func (e *ClaudeEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData) []GitHubActionStep {
-	// Collection step removed - firewall logs are now at a known location
-	return []GitHubActionStep{}
-}
-
 // GetSquidLogsSteps returns the steps for uploading and parsing Squid logs (after secret redaction)
 func (e *ClaudeEngine) GetSquidLogsSteps(workflowData *WorkflowData) []GitHubActionStep {
 	return defaultGetSquidLogsSteps(workflowData, claudeLog)

pkg/workflow/codex_engine.go

@@ -380,15 +380,6 @@ mkdir -p "$CODEX_HOME/logs"
 	return steps
 }
 
-// GetFirewallLogsCollectionStep returns the step for collecting firewall logs (before secret redaction).
-// This method is part of the firewall integration interface. It returns an empty slice because
-// firewall logs are written to a known location (/tmp/gh-aw/sandbox/firewall/logs/) and don't need
-// a separate collection step. The method is still called from compiler_yaml_main_job.go to maintain
-// consistent behavior with other engines that may need log collection steps.
-func (e *CodexEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData) []GitHubActionStep {
-	return []GitHubActionStep{}
-}
-
 // GetSquidLogsSteps returns the steps for uploading and parsing Squid logs (after secret redaction)
 func (e *CodexEngine) GetSquidLogsSteps(workflowData *WorkflowData) []GitHubActionStep {
 	return defaultGetSquidLogsSteps(workflowData, codexEngineLog)

pkg/workflow/compile_config_test.go

@@ -198,7 +198,10 @@ func TestSafeOutputsConfigGeneration(t *testing.T) {
 
 			// Use the compiler's generateOutputCollectionStep to verify config is not in env vars
 			var yamlBuilder strings.Builder
-			compiler.generateOutputCollectionStep(&yamlBuilder, workflowData)
+			err := compiler.generateOutputCollectionStep(&yamlBuilder, workflowData)
+			if err != nil {
+				t.Fatalf("generateOutputCollectionStep returned unexpected error: %v", err)
+			}
 			generatedYAML := yamlBuilder.String()
 
 			// Config should NOT be in environment variables anymore - it's in a file

pkg/workflow/compiler_activation_job_builder.go

@@ -238,9 +238,17 @@ func (c *Compiler) addActivationRepositoryAndOutputSteps(ctx *activationJobBuild
 		ctx.steps = append(ctx.steps, fmt.Sprintf("        uses: %s\n", getCachedActionPin("actions/github-script", data)))
 		var domainsStr string
 		if data.SafeOutputs != nil && len(data.SafeOutputs.AllowedDomains) > 0 {
-			domainsStr = c.computeExpandedAllowedDomainsForSanitization(data)
+			expanded, err := c.computeExpandedAllowedDomainsForSanitization(data)
+			if err != nil {
+				return err
+			}
+			domainsStr = expanded
 		} else {
-			domainsStr = c.computeAllowedDomainsForSanitization(data)
+			computed, err := c.computeAllowedDomainsForSanitization(data)
+			if err != nil {
+				return err
+			}
+			domainsStr = computed
 		}
 		var envLines []string
 		if len(data.Bots) > 0 {

pkg/workflow/compiler_safe_outputs_job.go

@@ -201,7 +201,10 @@ func (c *Compiler) buildConsolidatedSafeOutputsJob(data *WorkflowData, mainJobNa
 	// Critical for workflows that create projects and then add issues/PRs to those projects
 	if hasHandlerManagerTypes {
 		consolidatedSafeOutputsJobLog.Print("Using handler manager for safe outputs")
-		handlerManagerSteps := c.buildHandlerManagerStep(data)
+		handlerManagerSteps, err := c.buildHandlerManagerStep(data)
+		if err != nil {
+			return nil, nil, err
+		}
 		steps = append(steps, handlerManagerSteps...)
 		safeOutputStepNames = append(safeOutputStepNames, "process_safe_outputs")
 

pkg/workflow/compiler_safe_outputs_steps.go

@@ -142,7 +142,7 @@ func (c *Compiler) buildSharedPRCheckoutSteps(data *WorkflowData) []string {
 // buildHandlerManagerStep builds a single step that uses the safe output handler manager
 // to dispatch messages to appropriate handlers. This replaces multiple individual steps
 // with a single dispatcher step that processes all safe output types.
-func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) []string {
+func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) ([]string, error) {
 	consolidatedSafeOutputsStepsLog.Print("Building handler manager step")
 
 	var steps []string
@@ -162,9 +162,17 @@ func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) []string {
 	var domainsStr string
 	if data.SafeOutputs != nil && len(data.SafeOutputs.AllowedDomains) > 0 {
 		// allowed-domains: additional domains unioned with engine/network base set; supports ecosystem identifiers
-		domainsStr = c.computeExpandedAllowedDomainsForSanitization(data)
+		expanded, err := c.computeExpandedAllowedDomainsForSanitization(data)
+		if err != nil {
+			return nil, err
+		}
+		domainsStr = expanded
 	} else {
-		domainsStr = c.computeAllowedDomainsForSanitization(data)
+		computed, err := c.computeAllowedDomainsForSanitization(data)
+		if err != nil {
+			return nil, err
+		}
+		domainsStr = computed
 	}
 	if domainsStr != "" {
 		steps = append(steps, fmt.Sprintf("          GH_AW_ALLOWED_DOMAINS: %q\n", domainsStr))
@@ -332,5 +340,5 @@ func (c *Compiler) buildHandlerManagerStep(data *WorkflowData) []string {
 	steps = append(steps, "            const { main } = require('"+SetupActionDestination+"/safe_output_handler_manager.cjs');\n")
 	steps = append(steps, "            await main();\n")
 
-	return steps
+	return steps, nil
 }

pkg/workflow/compiler_safe_outputs_steps_test.go

@@ -479,7 +479,8 @@ func TestBuildHandlerManagerStep(t *testing.T) {
 				ParsedFrontmatter: tt.parsedFrontmatter,
 			}
 
-			steps := compiler.buildHandlerManagerStep(workflowData)
+			steps, err := compiler.buildHandlerManagerStep(workflowData)
+			require.NoError(t, err)
 
 			require.NotEmpty(t, steps)
 

pkg/workflow/compiler_yaml.go

@@ -830,7 +830,7 @@ func (c *Compiler) generateCreateAwInfo(yaml *strings.Builder, data *WorkflowDat
 	yaml.WriteString("            await main(core, context);\n")
 }
 
-func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *WorkflowData) {
+func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *WorkflowData) error {
 	// Copy the raw safe-output NDJSON to a /tmp/gh-aw/ path so it can be included in the
 	// unified agent artifact together with all other /tmp/gh-aw/ outputs.
 	yaml.WriteString("      - name: Copy Safe Outputs\n")
@@ -857,10 +857,18 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor
 	var domainsStr string
 	if data.SafeOutputs != nil && len(data.SafeOutputs.AllowedDomains) > 0 {
 		// allowed-domains: additional domains unioned with engine/network base set; supports ecosystem identifiers
-		domainsStr = c.computeExpandedAllowedDomainsForSanitization(data)
+		expanded, err := c.computeExpandedAllowedDomainsForSanitization(data)
+		if err != nil {
+			return err
+		}
+		domainsStr = expanded
 	} else {
 		// Fall back to computing from network configuration (same as firewall)
-		domainsStr = c.computeAllowedDomainsForSanitization(data)
+		computed, err := c.computeAllowedDomainsForSanitization(data)
+		if err != nil {
+			return err
+		}
+		domainsStr = computed
 	}
 	if domainsStr != "" {
 		fmt.Fprintf(yaml, "          GH_AW_ALLOWED_DOMAINS: %q\n", domainsStr)
@@ -892,6 +900,7 @@ func (c *Compiler) generateOutputCollectionStep(yaml *strings.Builder, data *Wor
 	yaml.WriteString("            const { main } = require('${{ runner.temp }}/gh-aw/actions/collect_ndjson_output.cjs');\n")
 	yaml.WriteString("            await main();\n")
 
+	return nil
 }
 
 // processMarkdownBody applies the standard post-processing pipeline to a markdown body: