refactor: consolidate duplicate provider extraction and remove redundant engine overrides

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

@@ -134,7 +134,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
@@ -154,9 +154,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))
@@ -324,5 +332,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

@@ -434,7 +434,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:

pkg/workflow/compiler_yaml_main_job.go

@@ -461,7 +461,9 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat
 
 	// Add output collection step only if safe-outputs feature is used (GH_AW_SAFE_OUTPUTS functionality)
 	if data.SafeOutputs != nil {
-		c.generateOutputCollectionStep(yaml, data)
+		if err := c.generateOutputCollectionStep(yaml, data); err != nil {
+			return err
+		}
 	}
 
 	// Merge engine-declared output files into the unified artifact instead of creating a

pkg/workflow/crush_engine.go

@@ -140,12 +140,18 @@ func (e *CrushEngine) GetExecutionSteps(workflowData *WorkflowData, logFile stri
 		if modelConfigured {
 			model = workflowData.EngineConfig.Model
 		}
-		allowedDomains := GetCrushAllowedDomainsWithToolsAndRuntimes(
+		// The model was validated by validateUniversalLLMConsumerModel before reaching here,
+		// so a malformed model (e.g. leading slash) must never occur. Panic is the correct
+		// response to an internal invariant violation.
+		allowedDomains, err := GetCrushAllowedDomainsWithToolsAndRuntimes(
 			model,
 			workflowData.NetworkPermissions,
 			workflowData.Tools,
 			workflowData.Runtimes,
 		)
+		if err != nil {
+			panic(fmt.Sprintf("BUG: invalid model %q reached domain computation (should have been caught by validation): %v", model, err))
+		}
 
 		npmPathSetup := GetNpmBinPathSetup()
 		crushCommandWithPath := fmt.Sprintf("%s && %s", npmPathSetup, crushCommand)

pkg/workflow/crush_engine_test.go

@@ -446,20 +446,40 @@ func TestCrushEngineFirewallIntegration(t *testing.T) {
 
 func TestExtractProviderFromModel(t *testing.T) {
 	t.Run("standard provider/model format", func(t *testing.T) {
-		assert.Equal(t, "anthropic", extractCrushProviderFromModel("anthropic/claude-sonnet-4-20250514"))
-		assert.Equal(t, "openai", extractCrushProviderFromModel("openai/gpt-4.1"))
-		assert.Equal(t, "google", extractCrushProviderFromModel("google/gemini-2.5-pro"))
+		provider, err := extractProviderFromModel("anthropic/claude-sonnet-4-20250514")
+		require.NoError(t, err)
+		assert.Equal(t, "anthropic", provider)
+
+		provider, err = extractProviderFromModel("openai/gpt-4.1")
+		require.NoError(t, err)
+		assert.Equal(t, "openai", provider)
+
+		provider, err = extractProviderFromModel("google/gemini-2.5-pro")
+		require.NoError(t, err)
+		assert.Equal(t, "google", provider)
 	})
 
-	t.Run("empty model defaults to copilot", func(t *testing.T) {
-		assert.Equal(t, "copilot", extractCrushProviderFromModel(""))
+	t.Run("empty model returns empty provider", func(t *testing.T) {
+		provider, err := extractProviderFromModel("")
+		require.NoError(t, err)
+		assert.Empty(t, provider)
 	})
 
-	t.Run("no slash defaults to copilot", func(t *testing.T) {
-		assert.Equal(t, "copilot", extractCrushProviderFromModel("claude-sonnet-4-20250514"))
+	t.Run("no slash returns empty provider", func(t *testing.T) {
+		provider, err := extractProviderFromModel("claude-sonnet-4-20250514")
+		require.NoError(t, err)
+		assert.Empty(t, provider)
 	})
 
 	t.Run("case insensitive provider", func(t *testing.T) {
-		assert.Equal(t, "openai", extractCrushProviderFromModel("OpenAI/gpt-4.1"))
+		provider, err := extractProviderFromModel("OpenAI/gpt-4.1")
+		require.NoError(t, err)
+		assert.Equal(t, "openai", provider)
+	})
+
+	t.Run("leading slash returns error", func(t *testing.T) {
+		_, err := extractProviderFromModel("/gpt-4.1")
+		require.Error(t, err, "Leading slash (empty provider prefix) must return an error")
+		assert.Contains(t, err.Error(), "provider prefix is empty")
 	})
 }