refactor(workflow): deduplicate logic, extract cross-engine helpers, fix interface bypass (#18671)

Author: Copilot

pkg/workflow/agentic_engine.go

@@ -155,6 +155,11 @@ type WorkflowExecutor interface {
 
 	// GetExecutionSteps returns the GitHub Actions steps for executing this engine
 	GetExecutionSteps(workflowData *WorkflowData, logFile string) []GitHubActionStep
+
+	// GetFirewallLogsCollectionStep returns steps that collect firewall-related log files
+	// before secret redaction runs. Engines that copy session or firewall state files should
+	// override this; the default implementation returns an empty slice.
+	GetFirewallLogsCollectionStep(workflowData *WorkflowData) []GitHubActionStep
 }
 
 // MCPConfigProvider handles MCP (Model Context Protocol) configuration
@@ -317,6 +322,12 @@ func (e *BaseEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubA
 	return GitHubActionStep{}
 }
 
+// GetFirewallLogsCollectionStep returns an empty slice by default.
+// Engines that need to copy session or firewall state files before secret redaction should override this.
+func (e *BaseEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData) []GitHubActionStep {
+	return []GitHubActionStep{}
+}
+
 // ParseLogMetrics provides a default no-op implementation for log parsing
 // Engines can override this to provide detailed log parsing and metrics extraction
 func (e *BaseEngine) ParseLogMetrics(logContent string, verbose bool) LogMetrics {

pkg/workflow/claude_engine.go

@@ -468,21 +468,5 @@ func (e *ClaudeEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData)
 
 // GetSquidLogsSteps returns the steps for uploading and parsing Squid logs (after secret redaction)
 func (e *ClaudeEngine) GetSquidLogsSteps(workflowData *WorkflowData) []GitHubActionStep {
-	var steps []GitHubActionStep
-
-	// Only add upload and parsing steps if firewall is enabled
-	if isFirewallEnabled(workflowData) {
-		claudeLog.Printf("Adding Squid logs upload and parsing steps for workflow: %s", workflowData.Name)
-
-		squidLogsUpload := generateSquidLogsUploadStep(workflowData.Name)
-		steps = append(steps, squidLogsUpload)
-
-		// Add firewall log parsing step to create step summary
-		firewallLogParsing := generateFirewallLogParsingStep(workflowData.Name)
-		steps = append(steps, firewallLogParsing)
-	} else {
-		claudeLog.Print("Firewall disabled, skipping Squid logs upload")
-	}
-
-	return steps
+	return defaultGetSquidLogsSteps(workflowData, claudeLog)
 }

pkg/workflow/claude_logs.go

@@ -308,7 +308,8 @@ func (e *ClaudeEngine) parseClaudeJSONLog(logContent string, verbose bool) LogMe
 				if messageMap, ok := message.(map[string]any); ok {
 					if content, exists := messageMap["content"]; exists {
 						if contentArray, ok := content.([]any); ok {
-							e.parseToolCalls(contentArray, toolCallMap)
+							// Sequence return value intentionally discarded; only toolCallMap is needed here.
+							e.parseToolCallsWithSequence(contentArray, toolCallMap)
 						}
 					}
 				}
@@ -429,86 +430,6 @@ func (e *ClaudeEngine) parseToolCallsWithSequence(contentArray []any, toolCallMa
 	return sequence
 }
 
-// parseToolCalls extracts tool call information from Claude log content array without sequence tracking
-func (e *ClaudeEngine) parseToolCalls(contentArray []any, toolCallMap map[string]*ToolCallInfo) {
-	for _, contentItem := range contentArray {
-		if contentMap, ok := contentItem.(map[string]any); ok {
-			if contentType, exists := contentMap["type"]; exists {
-				if typeStr, ok := contentType.(string); ok {
-					switch typeStr {
-					case "tool_use":
-						// Extract tool name
-						if toolName, exists := contentMap["name"]; exists {
-							if nameStr, ok := toolName.(string); ok {
-								// Prettify tool name
-								prettifiedName := PrettifyToolName(nameStr)
-
-								// Special handling for bash - each invocation is unique
-								if nameStr == "Bash" {
-									if input, exists := contentMap["input"]; exists {
-										if inputMap, ok := input.(map[string]any); ok {
-											if command, exists := inputMap["command"]; exists {
-												if commandStr, ok := command.(string); ok {
-													// Create unique bash entry with command info, avoiding colons
-													uniqueBashName := "bash_" + ShortenCommand(commandStr)
-													prettifiedName = uniqueBashName
-												}
-											}
-										}
-									}
-								}
-
-								// Calculate input size from the input field
-								inputSize := 0
-								if input, exists := contentMap["input"]; exists {
-									inputSize = e.estimateInputSize(input)
-								}
-
-								// Initialize or update tool call info
-								if toolInfo, exists := toolCallMap[prettifiedName]; exists {
-									toolInfo.CallCount++
-									if inputSize > toolInfo.MaxInputSize {
-										toolInfo.MaxInputSize = inputSize
-									}
-								} else {
-									toolCallMap[prettifiedName] = &ToolCallInfo{
-										Name:          prettifiedName,
-										CallCount:     1,
-										MaxInputSize:  inputSize,
-										MaxOutputSize: 0, // Will be updated when we find tool results
-										MaxDuration:   0, // Will be updated when we find execution timing
-									}
-								}
-							}
-						}
-					case "tool_result":
-						// Extract output size for tool results
-						if content, exists := contentMap["content"]; exists {
-							if contentStr, ok := content.(string); ok {
-								// Estimate token count (rough approximation: 1 token = ~4 characters)
-								outputSize := len(contentStr) / 4
-
-								// Find corresponding tool call to update max output size
-								if toolUseID, exists := contentMap["tool_use_id"]; exists {
-									if _, ok := toolUseID.(string); ok {
-										// This is simplified - in a full implementation we'd track tool_use_id to tool name mapping
-										// For now, we'll update the max output size for all tools (conservative estimate)
-										for _, toolInfo := range toolCallMap {
-											if outputSize > toolInfo.MaxOutputSize {
-												toolInfo.MaxOutputSize = outputSize
-											}
-										}
-									}
-								}
-							}
-						}
-					}
-				}
-			}
-		}
-	}
-}
-
 // estimateInputSize estimates the input size in tokens from a tool input object
 func (e *ClaudeEngine) estimateInputSize(input any) int {
 	// Convert input to JSON string to get approximate size

pkg/workflow/claude_mcp.go

@@ -45,7 +45,7 @@ func (e *ClaudeEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]a
 				renderer := createRenderer(isLast)
 				renderer.RenderSerenaMCP(yaml, serenaTool)
 			},
-			RenderCacheMemory: e.renderCacheMemoryMCPConfig,
+			RenderCacheMemory: noOpCacheMemoryRenderer,
 			RenderAgenticWorkflows: func(yaml *strings.Builder, isLast bool) {
 				renderer := createRenderer(isLast)
 				renderer.RenderAgenticWorkflowsMCP(yaml)
@@ -73,12 +73,3 @@ func (e *ClaudeEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]a
 func (e *ClaudeEngine) renderClaudeMCPConfigWithContext(yaml *strings.Builder, toolName string, toolConfig map[string]any, isLast bool, workflowData *WorkflowData) error {
 	return renderCustomMCPConfigWrapperWithContext(yaml, toolName, toolConfig, isLast, workflowData)
 }
-
-// renderCacheMemoryMCPConfig handles cache-memory configuration without MCP server mounting
-// Cache-memory is now a simple file share, not an MCP server
-func (e *ClaudeEngine) renderCacheMemoryMCPConfig(yaml *strings.Builder, isLast bool, workflowData *WorkflowData) {
-	// Cache-memory no longer uses MCP server mounting
-	// The cache folder is available as a simple file share at /tmp/gh-aw/cache-memory/
-	// The folder is created by the cache step and is accessible to all tools
-	// No MCP configuration is needed for simple file access
-}

pkg/workflow/codex_engine.go

@@ -352,23 +352,7 @@ func (e *CodexEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData)
 
 // GetSquidLogsSteps returns the steps for uploading and parsing Squid logs (after secret redaction)
 func (e *CodexEngine) GetSquidLogsSteps(workflowData *WorkflowData) []GitHubActionStep {
-	var steps []GitHubActionStep
-
-	// Only add upload and parsing steps if firewall is enabled
-	if isFirewallEnabled(workflowData) {
-		codexEngineLog.Printf("Adding Squid logs upload and parsing steps for workflow: %s", workflowData.Name)
-
-		squidLogsUpload := generateSquidLogsUploadStep(workflowData.Name)
-		steps = append(steps, squidLogsUpload)
-
-		// Add firewall log parsing step to create step summary
-		firewallLogParsing := generateFirewallLogParsingStep(workflowData.Name)
-		steps = append(steps, firewallLogParsing)
-	} else {
-		codexEngineLog.Print("Firewall disabled, skipping Squid logs upload")
-	}
-
-	return steps
+	return defaultGetSquidLogsSteps(workflowData, codexEngineLog)
 }
 
 // expandNeutralToolsToCodexTools converts neutral tools to Codex-specific tools format

pkg/workflow/codex_mcp.go

@@ -169,9 +169,7 @@ func (e *CodexEngine) renderCodexMCPConfigWithContext(yaml *strings.Builder, too
 
 	// Determine if localhost URLs should be rewritten to host.docker.internal
 	// This is needed when firewall is enabled (agent is not disabled)
-	rewriteLocalhost := workflowData != nil && (workflowData.SandboxConfig == nil ||
-		workflowData.SandboxConfig.Agent == nil ||
-		!workflowData.SandboxConfig.Agent.Disabled)
+	rewriteLocalhost := shouldRewriteLocalhostToDocker(workflowData)
 
 	// Use the shared MCP config renderer with TOML format
 	renderer := MCPConfigRenderer{
@@ -192,9 +190,7 @@ func (e *CodexEngine) renderCodexMCPConfigWithContext(yaml *strings.Builder, too
 // This is used to generate the JSON config file that the MCP gateway reads
 func (e *CodexEngine) renderCodexJSONMCPConfigWithContext(yaml *strings.Builder, toolName string, toolConfig map[string]any, isLast bool, workflowData *WorkflowData) error {
 	// Determine if localhost URLs should be rewritten to host.docker.internal
-	rewriteLocalhost := workflowData != nil && (workflowData.SandboxConfig == nil ||
-		workflowData.SandboxConfig.Agent == nil ||
-		!workflowData.SandboxConfig.Agent.Disabled)
+	rewriteLocalhost := shouldRewriteLocalhostToDocker(workflowData)
 
 	// Use the shared renderer with JSON format for gateway
 	renderer := MCPConfigRenderer{

pkg/workflow/compiler_yaml_main_job.go

@@ -276,36 +276,9 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat
 	}
 
 	// Collect firewall logs BEFORE secret redaction so secrets in logs can be redacted
-	if copilotEngine, ok := engine.(*CopilotEngine); ok {
-		collectionSteps := copilotEngine.GetFirewallLogsCollectionStep(data)
-		for _, step := range collectionSteps {
-			for _, line := range step {
-				yaml.WriteString(line + "\n")
-			}
-		}
-	}
-	if codexEngine, ok := engine.(*CodexEngine); ok {
-		collectionSteps := codexEngine.GetFirewallLogsCollectionStep(data)
-		for _, step := range collectionSteps {
-			for _, line := range step {
-				yaml.WriteString(line + "\n")
-			}
-		}
-	}
-	if claudeEngine, ok := engine.(*ClaudeEngine); ok {
-		collectionSteps := claudeEngine.GetFirewallLogsCollectionStep(data)
-		for _, step := range collectionSteps {
-			for _, line := range step {
-				yaml.WriteString(line + "\n")
-			}
-		}
-	}
-	if codexEngine, ok := engine.(*CodexEngine); ok {
-		collectionSteps := codexEngine.GetFirewallLogsCollectionStep(data)
-		for _, step := range collectionSteps {
-			for _, line := range step {
-				yaml.WriteString(line + "\n")
-			}
+	for _, step := range engine.GetFirewallLogsCollectionStep(data) {
+		for _, line := range step {
+			yaml.WriteString(line + "\n")
 		}
 	}
 

pkg/workflow/copilot_installer.go

@@ -1,9 +1,6 @@
 package workflow
 
 import (
-	"fmt"
-	"strings"
-
 	"github.com/github/gh-aw/pkg/constants"
 	"github.com/github/gh-aw/pkg/logger"
 )
@@ -30,50 +27,3 @@ func GenerateCopilotInstallerSteps(version, stepName string) []GitHubActionStep
 
 	return []GitHubActionStep{GitHubActionStep(stepLines)}
 }
-
-// generateSquidLogsUploadStep creates a GitHub Actions step to upload Squid logs as artifact.
-func generateSquidLogsUploadStep(workflowName string) GitHubActionStep {
-	sanitizedName := strings.ToLower(SanitizeWorkflowName(workflowName))
-	artifactName := "firewall-logs-" + sanitizedName
-	// Firewall logs are now at a known location in the sandbox folder structure
-	firewallLogsDir := "/tmp/gh-aw/sandbox/firewall/logs/"
-
-	stepLines := []string{
-		"      - name: Upload Firewall Logs",
-		"        if: always()",
-		"        continue-on-error: true",
-		"        uses: " + GetActionPin("actions/upload-artifact"),
-		"        with:",
-		"          name: " + artifactName,
-		"          path: " + firewallLogsDir,
-		"          if-no-files-found: ignore",
-	}
-
-	return GitHubActionStep(stepLines)
-}
-
-// generateFirewallLogParsingStep creates a GitHub Actions step to parse firewall logs and create step summary.
-func generateFirewallLogParsingStep(workflowName string) GitHubActionStep {
-	// Firewall logs are at a known location in the sandbox folder structure
-	firewallLogsDir := "/tmp/gh-aw/sandbox/firewall/logs"
-
-	stepLines := []string{
-		"      - name: Print firewall logs",
-		"        if: always()",
-		"        continue-on-error: true",
-		"        env:",
-		"          AWF_LOGS_DIR: " + firewallLogsDir,
-		"        run: |",
-		"          # Fix permissions on firewall logs so they can be uploaded as artifacts",
-		"          # AWF runs with sudo, creating files owned by root",
-		fmt.Sprintf("          sudo chmod -R a+r %s 2>/dev/null || true", firewallLogsDir),
-		"          # Only run awf logs summary if awf command exists (it may not be installed if workflow failed before install step)",
-		"          if command -v awf &> /dev/null; then",
-		"            awf logs summary | tee -a \"$GITHUB_STEP_SUMMARY\"",
-		"          else",
-		"            echo 'AWF binary not installed, skipping firewall log summary'",
-		"          fi",
-	}
-
-	return GitHubActionStep(stepLines)
-}

pkg/workflow/copilot_logs.go

@@ -448,23 +448,7 @@ func (e *CopilotEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData
 
 // GetSquidLogsSteps returns the steps for uploading and parsing Squid logs (after secret redaction)
 func (e *CopilotEngine) GetSquidLogsSteps(workflowData *WorkflowData) []GitHubActionStep {
-	var steps []GitHubActionStep
-
-	// Only add upload and parsing steps if firewall is enabled
-	if isFirewallEnabled(workflowData) {
-		copilotLogsLog.Printf("Adding Squid logs upload and parsing steps for workflow: %s", workflowData.Name)
-
-		squidLogsUpload := generateSquidLogsUploadStep(workflowData.Name)
-		steps = append(steps, squidLogsUpload)
-
-		// Add firewall log parsing step to create step summary
-		firewallLogParsing := generateFirewallLogParsingStep(workflowData.Name)
-		steps = append(steps, firewallLogParsing)
-	} else {
-		copilotLogsLog.Print("Firewall disabled, skipping Squid logs upload")
-	}
-
-	return steps
+	return defaultGetSquidLogsSteps(workflowData, copilotLogsLog)
 }
 
 // GetCleanupStep returns the post-execution cleanup step (currently empty)

pkg/workflow/copilot_mcp.go

@@ -87,9 +87,7 @@ func (e *CopilotEngine) renderCopilotMCPConfigWithContext(yaml *strings.Builder,
 
 	// Determine if localhost URLs should be rewritten to host.docker.internal
 	// This is needed when firewall is enabled (agent is not disabled)
-	rewriteLocalhost := workflowData != nil && (workflowData.SandboxConfig == nil ||
-		workflowData.SandboxConfig.Agent == nil ||
-		!workflowData.SandboxConfig.Agent.Disabled)
+	rewriteLocalhost := shouldRewriteLocalhostToDocker(workflowData)
 
 	// Use the shared renderer with copilot-specific requirements
 	renderer := MCPConfigRenderer{