From b6f2583ae564d3d43318211e660f0325cb4ede8e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Feb 2026 12:39:19 +0000 Subject: [PATCH 1/3] Initial plan From b53ff2333cef9ed5e05c37b912f6dbf9fd5b55a1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Feb 2026 12:57:21 +0000 Subject: [PATCH 2/3] Refactor: Issues 2,4,5,6,7,9,10 - extract helpers, fix interface, remove duplicates Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/agentic_engine.go | 11 ++++ pkg/workflow/claude_engine.go | 18 +----- pkg/workflow/claude_logs.go | 82 +------------------------ pkg/workflow/claude_mcp.go | 11 +--- pkg/workflow/codex_engine.go | 18 +----- pkg/workflow/codex_mcp.go | 8 +-- pkg/workflow/compiler_yaml_main_job.go | 33 +--------- pkg/workflow/copilot_installer.go | 50 --------------- pkg/workflow/copilot_logs.go | 18 +----- pkg/workflow/copilot_mcp.go | 4 +- pkg/workflow/engine_firewall_support.go | 71 +++++++++++++++++++++ pkg/workflow/gemini_mcp.go | 9 +-- pkg/workflow/mcp_config_custom.go | 4 +- pkg/workflow/mcp_config_utils.go | 15 +++++ pkg/workflow/safe_output_config.go | 42 ------------- pkg/workflow/safe_outputs_config.go | 41 +++++++++++++ 16 files changed, 151 insertions(+), 284 deletions(-) delete mode 100644 pkg/workflow/safe_output_config.go diff --git a/pkg/workflow/agentic_engine.go b/pkg/workflow/agentic_engine.go index 388bd976f3b..af89f2f1f18 100644 --- a/pkg/workflow/agentic_engine.go +++ b/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 { diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index 72abe76b628..e0fd2c3fdca 100644 --- a/pkg/workflow/claude_engine.go +++ b/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) } diff --git a/pkg/workflow/claude_logs.go b/pkg/workflow/claude_logs.go index 6d705a24b3b..8de548fb31f 100644 --- a/pkg/workflow/claude_logs.go +++ b/pkg/workflow/claude_logs.go @@ -308,7 +308,7 @@ 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) + e.parseToolCallsWithSequence(contentArray, toolCallMap) } } } @@ -429,86 +429,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 diff --git a/pkg/workflow/claude_mcp.go b/pkg/workflow/claude_mcp.go index 33f9a696122..eadc161b1e8 100644 --- a/pkg/workflow/claude_mcp.go +++ b/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 -} diff --git a/pkg/workflow/codex_engine.go b/pkg/workflow/codex_engine.go index e4c24da881d..e626ab5fb40 100644 --- a/pkg/workflow/codex_engine.go +++ b/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 diff --git a/pkg/workflow/codex_mcp.go b/pkg/workflow/codex_mcp.go index 0a023a116dc..b2e140d9183 100644 --- a/pkg/workflow/codex_mcp.go +++ b/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{ diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index ff45765315a..c52b72b3676 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/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") } } diff --git a/pkg/workflow/copilot_installer.go b/pkg/workflow/copilot_installer.go index e228a1a2dce..669fed90b2d 100644 --- a/pkg/workflow/copilot_installer.go +++ b/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) -} diff --git a/pkg/workflow/copilot_logs.go b/pkg/workflow/copilot_logs.go index 4c574d395de..242148b301a 100644 --- a/pkg/workflow/copilot_logs.go +++ b/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) diff --git a/pkg/workflow/copilot_mcp.go b/pkg/workflow/copilot_mcp.go index edfa8730e83..b954cd0abce 100644 --- a/pkg/workflow/copilot_mcp.go +++ b/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{ diff --git a/pkg/workflow/engine_firewall_support.go b/pkg/workflow/engine_firewall_support.go index 71df4cc9625..7700292a66f 100644 --- a/pkg/workflow/engine_firewall_support.go +++ b/pkg/workflow/engine_firewall_support.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "strings" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" @@ -119,3 +120,73 @@ func (c *Compiler) checkFirewallDisable(engine CodingAgentEngine, networkPermiss return nil } + +// 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) +} + +// defaultGetSquidLogsSteps returns the steps for uploading and parsing Squid logs after +// secret redaction. It is shared across engines (Claude, Codex, Copilot) whose +// GetSquidLogsSteps implementations are otherwise identical save for the logger used. +func defaultGetSquidLogsSteps(workflowData *WorkflowData, log *logger.Logger) []GitHubActionStep { +var steps []GitHubActionStep + +// Only add upload and parsing steps if firewall is enabled +if isFirewallEnabled(workflowData) { +log.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 { +log.Print("Firewall disabled, skipping Squid logs upload") +} + +return steps +} diff --git a/pkg/workflow/gemini_mcp.go b/pkg/workflow/gemini_mcp.go index a3d09f9ff34..95b289c6403 100644 --- a/pkg/workflow/gemini_mcp.go +++ b/pkg/workflow/gemini_mcp.go @@ -40,7 +40,7 @@ func (e *GeminiEngine) 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) @@ -62,10 +62,3 @@ func (e *GeminiEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]a }, }) } - -// renderCacheMemoryMCPConfig handles cache-memory configuration for Gemini -func (e *GeminiEngine) renderCacheMemoryMCPConfig(yaml *strings.Builder, isLast bool, workflowData *WorkflowData) { - // Cache-memory is a simple file share, not an MCP server - // No MCP configuration needed - geminiMCPLog.Print("Cache-memory tool detected, no MCP config needed") -} diff --git a/pkg/workflow/mcp_config_custom.go b/pkg/workflow/mcp_config_custom.go index 06ea7b6c307..e7217b1e57b 100644 --- a/pkg/workflow/mcp_config_custom.go +++ b/pkg/workflow/mcp_config_custom.go @@ -50,9 +50,7 @@ func renderCustomMCPConfigWrapperWithContext(yaml *strings.Builder, toolName str // 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 JSON format renderer := MCPConfigRenderer{ diff --git a/pkg/workflow/mcp_config_utils.go b/pkg/workflow/mcp_config_utils.go index 346274d5a93..89933f9d287 100644 --- a/pkg/workflow/mcp_config_utils.go +++ b/pkg/workflow/mcp_config_utils.go @@ -76,3 +76,18 @@ func rewriteLocalhostToDockerHost(url string) string { return url } + +// shouldRewriteLocalhostToDocker returns true when MCP server localhost URLs should be +// rewritten to host.docker.internal so that containerised AI agents can reach servers +// running on the host. Rewriting is enabled whenever the agent sandbox is active +// (i.e. sandbox.agent is not explicitly disabled). +func shouldRewriteLocalhostToDocker(workflowData *WorkflowData) bool { +return workflowData != nil && (workflowData.SandboxConfig == nil || +workflowData.SandboxConfig.Agent == nil || +!workflowData.SandboxConfig.Agent.Disabled) +} + +// noOpCacheMemoryRenderer is a no-op MCPToolRenderers.RenderCacheMemory function for engines +// that do not need an MCP server entry for cache-memory. Cache-memory is a simple file share +// accessible at /tmp/gh-aw/cache-memory/ and requires no MCP configuration. +func noOpCacheMemoryRenderer(_ *strings.Builder, _ bool, _ *WorkflowData) {} diff --git a/pkg/workflow/safe_output_config.go b/pkg/workflow/safe_output_config.go deleted file mode 100644 index 09060c92266..00000000000 --- a/pkg/workflow/safe_output_config.go +++ /dev/null @@ -1,42 +0,0 @@ -package workflow - -import "strings" - -// parseBaseSafeOutputConfig parses common fields (max, github-token) from a config map. -// If defaultMax is provided (> 0), it will be set as the default value for config.Max -// before parsing the max field from configMap. Supports both integer values and GitHub -// Actions expression strings (e.g. "${{ inputs.max }}"). -func (c *Compiler) parseBaseSafeOutputConfig(configMap map[string]any, config *BaseSafeOutputConfig, defaultMax int) { - // Set default max if provided - if defaultMax > 0 { - safeOutputsConfigLog.Printf("Setting default max: %d", defaultMax) - config.Max = defaultIntStr(defaultMax) - } - - // Parse max (this will override the default if present in configMap) - if max, exists := configMap["max"]; exists { - switch v := max.(type) { - case string: - // Accept GitHub Actions expression strings - if strings.HasPrefix(v, "${{") && strings.HasSuffix(v, "}}") { - safeOutputsConfigLog.Printf("Parsed max as GitHub Actions expression: %s", v) - config.Max = &v - } - default: - // Convert integer/float64/etc to string via parseIntValue - if maxInt, ok := parseIntValue(max); ok { - safeOutputsConfigLog.Printf("Parsed max as integer: %d", maxInt) - s := defaultIntStr(maxInt) - config.Max = s - } - } - } - - // Parse github-token - if githubToken, exists := configMap["github-token"]; exists { - if githubTokenStr, ok := githubToken.(string); ok { - safeOutputsConfigLog.Print("Parsed custom github-token from config") - config.GitHubToken = githubTokenStr - } - } -} diff --git a/pkg/workflow/safe_outputs_config.go b/pkg/workflow/safe_outputs_config.go index 559aafe32dd..4895fda3fd9 100644 --- a/pkg/workflow/safe_outputs_config.go +++ b/pkg/workflow/safe_outputs_config.go @@ -1,6 +1,8 @@ package workflow import ( + "strings" + "github.com/github/gh-aw/pkg/logger" ) @@ -506,3 +508,42 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut return config } + +// parseBaseSafeOutputConfig parses common fields (max, github-token) from a config map. +// If defaultMax is provided (> 0), it will be set as the default value for config.Max +// before parsing the max field from configMap. Supports both integer values and GitHub +// Actions expression strings (e.g. "${{ inputs.max }}"). +func (c *Compiler) parseBaseSafeOutputConfig(configMap map[string]any, config *BaseSafeOutputConfig, defaultMax int) { +// Set default max if provided +if defaultMax > 0 { +safeOutputsConfigLog.Printf("Setting default max: %d", defaultMax) +config.Max = defaultIntStr(defaultMax) +} + +// Parse max (this will override the default if present in configMap) +if max, exists := configMap["max"]; exists { +switch v := max.(type) { +case string: +// Accept GitHub Actions expression strings +if strings.HasPrefix(v, "${{") && strings.HasSuffix(v, "}}") { +safeOutputsConfigLog.Printf("Parsed max as GitHub Actions expression: %s", v) +config.Max = &v +} +default: +// Convert integer/float64/etc to string via parseIntValue +if maxInt, ok := parseIntValue(max); ok { +safeOutputsConfigLog.Printf("Parsed max as integer: %d", maxInt) +s := defaultIntStr(maxInt) +config.Max = s +} +} +} + +// Parse github-token +if githubToken, exists := configMap["github-token"]; exists { +if githubTokenStr, ok := githubToken.(string); ok { +safeOutputsConfigLog.Print("Parsed custom github-token from config") +config.GitHubToken = githubTokenStr +} +} +} From b1d0b2f3bc6aeab65879f9a219b1be76cc4373e4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 27 Feb 2026 13:07:36 +0000 Subject: [PATCH 3/3] Refactor: Issue 1 - unify missing_data/missing_tool into shared IssueReportingConfig; address review comments Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/claude_logs.go | 1 + pkg/workflow/engine_firewall_support.go | 100 ++++++------- pkg/workflow/mcp_config_utils.go | 6 +- pkg/workflow/missing_data.go | 150 ++------------------ pkg/workflow/missing_issue_reporting.go | 177 ++++++++++++++++++++++++ pkg/workflow/missing_tool.go | 150 ++------------------ pkg/workflow/safe_outputs_config.go | 60 ++++---- 7 files changed, 279 insertions(+), 365 deletions(-) create mode 100644 pkg/workflow/missing_issue_reporting.go diff --git a/pkg/workflow/claude_logs.go b/pkg/workflow/claude_logs.go index 8de548fb31f..7a6449c8029 100644 --- a/pkg/workflow/claude_logs.go +++ b/pkg/workflow/claude_logs.go @@ -308,6 +308,7 @@ 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 { + // Sequence return value intentionally discarded; only toolCallMap is needed here. e.parseToolCallsWithSequence(contentArray, toolCallMap) } } diff --git a/pkg/workflow/engine_firewall_support.go b/pkg/workflow/engine_firewall_support.go index 7700292a66f..245c8e1b2db 100644 --- a/pkg/workflow/engine_firewall_support.go +++ b/pkg/workflow/engine_firewall_support.go @@ -123,70 +123,70 @@ func (c *Compiler) checkFirewallDisable(engine CodingAgentEngine, networkPermiss // 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", -} + 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) + 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", -} + // 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) + return GitHubActionStep(stepLines) } // defaultGetSquidLogsSteps returns the steps for uploading and parsing Squid logs after // secret redaction. It is shared across engines (Claude, Codex, Copilot) whose // GetSquidLogsSteps implementations are otherwise identical save for the logger used. func defaultGetSquidLogsSteps(workflowData *WorkflowData, log *logger.Logger) []GitHubActionStep { -var steps []GitHubActionStep + var steps []GitHubActionStep -// Only add upload and parsing steps if firewall is enabled -if isFirewallEnabled(workflowData) { -log.Printf("Adding Squid logs upload and parsing steps for workflow: %s", workflowData.Name) + // Only add upload and parsing steps if firewall is enabled + if isFirewallEnabled(workflowData) { + log.Printf("Adding Squid logs upload and parsing steps for workflow: %s", workflowData.Name) -squidLogsUpload := generateSquidLogsUploadStep(workflowData.Name) -steps = append(steps, squidLogsUpload) + 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 { -log.Print("Firewall disabled, skipping Squid logs upload") -} + // Add firewall log parsing step to create step summary + firewallLogParsing := generateFirewallLogParsingStep(workflowData.Name) + steps = append(steps, firewallLogParsing) + } else { + log.Print("Firewall disabled, skipping Squid logs upload") + } -return steps + return steps } diff --git a/pkg/workflow/mcp_config_utils.go b/pkg/workflow/mcp_config_utils.go index 89933f9d287..19b049df7c2 100644 --- a/pkg/workflow/mcp_config_utils.go +++ b/pkg/workflow/mcp_config_utils.go @@ -82,9 +82,9 @@ func rewriteLocalhostToDockerHost(url string) string { // running on the host. Rewriting is enabled whenever the agent sandbox is active // (i.e. sandbox.agent is not explicitly disabled). func shouldRewriteLocalhostToDocker(workflowData *WorkflowData) bool { -return workflowData != nil && (workflowData.SandboxConfig == nil || -workflowData.SandboxConfig.Agent == nil || -!workflowData.SandboxConfig.Agent.Disabled) + return workflowData != nil && (workflowData.SandboxConfig == nil || + workflowData.SandboxConfig.Agent == nil || + !workflowData.SandboxConfig.Agent.Disabled) } // noOpCacheMemoryRenderer is a no-op MCPToolRenderers.RenderCacheMemory function for engines diff --git a/pkg/workflow/missing_data.go b/pkg/workflow/missing_data.go index 30127749f82..b34f05d386e 100644 --- a/pkg/workflow/missing_data.go +++ b/pkg/workflow/missing_data.go @@ -1,163 +1,31 @@ package workflow import ( - "encoding/json" "errors" - "fmt" "github.com/github/gh-aw/pkg/logger" ) var missingDataLog = logger.New("workflow:missing_data") -// MissingDataConfig holds configuration for reporting missing data required to achieve goals -type MissingDataConfig struct { - BaseSafeOutputConfig `yaml:",inline"` - CreateIssue bool `yaml:"create-issue,omitempty"` // Whether to create/update issues for missing data (default: true) - TitlePrefix string `yaml:"title-prefix,omitempty"` // Prefix for issue titles (default: "[missing data]") - Labels []string `yaml:"labels,omitempty"` // Labels to add to created issues -} - // buildCreateOutputMissingDataJob creates the missing_data job func (c *Compiler) buildCreateOutputMissingDataJob(data *WorkflowData, mainJobName string) (*Job, error) { - missingDataLog.Printf("Building missing_data job for workflow: %s", data.Name) - if data.SafeOutputs == nil || data.SafeOutputs.MissingData == nil { return nil, errors.New("safe-outputs.missing-data configuration is required") } - // Build custom environment variables specific to missing-data - var customEnvVars []string - if data.SafeOutputs.MissingData.Max != nil { - missingDataLog.Printf("Setting max missing data limit: %s", *data.SafeOutputs.MissingData.Max) - customEnvVars = append(customEnvVars, buildTemplatableIntEnvVar("GH_AW_MISSING_DATA_MAX", data.SafeOutputs.MissingData.Max)...) - } - - // Add create-issue configuration - if data.SafeOutputs.MissingData.CreateIssue { - customEnvVars = append(customEnvVars, " GH_AW_MISSING_DATA_CREATE_ISSUE: \"true\"\n") - missingDataLog.Print("create-issue enabled for missing-data") - } - - // Add title-prefix configuration - if data.SafeOutputs.MissingData.TitlePrefix != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_MISSING_DATA_TITLE_PREFIX: %q\n", data.SafeOutputs.MissingData.TitlePrefix)) - missingDataLog.Printf("title-prefix: %s", data.SafeOutputs.MissingData.TitlePrefix) - } - - // Add labels configuration - if len(data.SafeOutputs.MissingData.Labels) > 0 { - labelsJSON, err := json.Marshal(data.SafeOutputs.MissingData.Labels) - if err == nil { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_MISSING_DATA_LABELS: %q\n", string(labelsJSON))) - missingDataLog.Printf("labels: %v", data.SafeOutputs.MissingData.Labels) - } - } - - // Add workflow metadata for consistency - customEnvVars = append(customEnvVars, buildWorkflowMetadataEnvVarsWithTrackerID(data.Name, data.Source, data.TrackerID)...) - - // Create outputs for the job - outputs := map[string]string{ - "data_reported": "${{ steps.missing_data.outputs.data_reported }}", - "total_count": "${{ steps.missing_data.outputs.total_count }}", - } - - // Build the job condition using BuildSafeOutputType - jobCondition := BuildSafeOutputType("missing_data") - - // Set permissions based on whether issue creation is enabled - permissions := NewPermissionsContentsRead() - if data.SafeOutputs.MissingData.CreateIssue { - // Add issues:write permission for creating/updating issues - permissions.Set(PermissionIssues, PermissionWrite) - missingDataLog.Print("Added issues:write permission for create-issue functionality") - } - - // Use the shared builder function to create the job - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: "missing_data", - StepName: "Record Missing Data", - StepID: "missing_data", - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - Script: "const { main } = require('/opt/gh-aw/actions/missing_data.cjs'); await main();", - Permissions: permissions, - Outputs: outputs, - Condition: jobCondition, - Token: data.SafeOutputs.MissingData.GitHubToken, + return c.buildIssueReportingJob(data, mainJobName, issueReportingJobParams{ + kind: "missing_data", + envPrefix: envVarPrefix("missing_data"), + defaultTitle: "[missing data]", + outputKey: "data_reported", + stepName: "Record Missing Data", + config: data.SafeOutputs.MissingData, + log: missingDataLog, }) } // parseMissingDataConfig handles missing-data configuration func (c *Compiler) parseMissingDataConfig(outputMap map[string]any) *MissingDataConfig { - if configData, exists := outputMap["missing-data"]; exists { - // Handle the case where configData is false (explicitly disabled) - if configBool, ok := configData.(bool); ok && !configBool { - missingDataLog.Print("Missing-data configuration explicitly disabled") - return nil - } - - // Create config with no defaults - they will be applied in JavaScript - missingDataConfig := &MissingDataConfig{} - - // Handle the case where configData is nil (missing-data: with no value) - if configData == nil { - missingDataLog.Print("Missing-data configuration enabled with defaults") - // Set create-issue to true as default when missing-data is enabled - missingDataConfig.CreateIssue = true - missingDataConfig.TitlePrefix = "[missing data]" - missingDataConfig.Labels = []string{} - return missingDataConfig - } - - if configMap, ok := configData.(map[string]any); ok { - missingDataLog.Print("Parsing missing-data configuration from map") - // Parse common base fields with default max of 0 (no limit) - c.parseBaseSafeOutputConfig(configMap, &missingDataConfig.BaseSafeOutputConfig, 0) - - // Parse create-issue field, default to true if not specified - if createIssue, exists := configMap["create-issue"]; exists { - if createIssueBool, ok := createIssue.(bool); ok { - missingDataConfig.CreateIssue = createIssueBool - missingDataLog.Printf("create-issue: %v", createIssueBool) - } - } else { - // Default to true when config map exists but create-issue not specified - missingDataConfig.CreateIssue = true - } - - // Parse title-prefix field, default to "[missing data]" if not specified - if titlePrefix, exists := configMap["title-prefix"]; exists { - if titlePrefixStr, ok := titlePrefix.(string); ok { - missingDataConfig.TitlePrefix = titlePrefixStr - missingDataLog.Printf("title-prefix: %s", titlePrefixStr) - } - } else { - // Default title prefix - missingDataConfig.TitlePrefix = "[missing data]" - } - - // Parse labels field, default to empty array if not specified - if labels, exists := configMap["labels"]; exists { - if labelsArray, ok := labels.([]any); ok { - var labelStrings []string - for _, label := range labelsArray { - if labelStr, ok := label.(string); ok { - labelStrings = append(labelStrings, labelStr) - } - } - missingDataConfig.Labels = labelStrings - missingDataLog.Printf("labels: %v", labelStrings) - } - } else { - // Default to empty labels - missingDataConfig.Labels = []string{} - } - } - - return missingDataConfig - } - - return nil + return c.parseIssueReportingConfig(outputMap, "missing-data", "[missing data]", missingDataLog) } diff --git a/pkg/workflow/missing_issue_reporting.go b/pkg/workflow/missing_issue_reporting.go new file mode 100644 index 00000000000..8ac6522743f --- /dev/null +++ b/pkg/workflow/missing_issue_reporting.go @@ -0,0 +1,177 @@ +package workflow + +import ( + "encoding/json" + "fmt" + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +// IssueReportingConfig holds configuration shared by safe-output types that create GitHub issues +// (missing-data and missing-tool). Both types have identical fields; the yaml tags on the +// parent struct fields give them their distinct YAML keys. +type IssueReportingConfig struct { + BaseSafeOutputConfig `yaml:",inline"` + CreateIssue bool `yaml:"create-issue,omitempty"` // Whether to create/update issues (default: true) + TitlePrefix string `yaml:"title-prefix,omitempty"` // Prefix for issue titles + Labels []string `yaml:"labels,omitempty"` // Labels to add to created issues +} + +// Type aliases so existing code (compiler_types.go, tests, etc.) continues to compile unchanged. +// Both resolve to IssueReportingConfig; the distinct names preserve semantic clarity at usage sites. +type MissingDataConfig = IssueReportingConfig +type MissingToolConfig = IssueReportingConfig + +// issueReportingJobParams carries the varying values that distinguish the missing-data and +// missing-tool jobs. All logic that differs between the two is expressed through these fields. +type issueReportingJobParams struct { + // kind is the snake_case identifier, e.g. "missing_data" or "missing_tool". + // It is used for job/step IDs, the safe-output type condition, and to derive the script path. + kind string + // envPrefix is the upper-case env-var prefix, e.g. "GH_AW_MISSING_DATA". + envPrefix string + // defaultTitle is the default issue title prefix, e.g. "[missing data]". + defaultTitle string + // outputKey is the primary output key in the job outputs map, e.g. "data_reported" or "tools_reported". + outputKey string + // stepName is the human-readable step name, e.g. "Record Missing Data". + stepName string + // config holds the resolved configuration values. + config *IssueReportingConfig + // log is the caller's package-scoped logger. + log *logger.Logger +} + +// buildIssueReportingJob constructs the GitHub Actions job for a missing-data or missing-tool +// safe-output type. The two callers differ only in the params they supply. +func (c *Compiler) buildIssueReportingJob(data *WorkflowData, mainJobName string, p issueReportingJobParams) (*Job, error) { + p.log.Printf("Building %s job for workflow: %s", p.kind, data.Name) + + var customEnvVars []string + + if p.config.Max != nil { + p.log.Printf("Setting max %s limit: %s", p.kind, *p.config.Max) + customEnvVars = append(customEnvVars, buildTemplatableIntEnvVar(p.envPrefix+"_MAX", p.config.Max)...) + } + + if p.config.CreateIssue { + customEnvVars = append(customEnvVars, fmt.Sprintf(" %s_CREATE_ISSUE: \"true\"\n", p.envPrefix)) + p.log.Printf("create-issue enabled for %s", p.kind) + } + + if p.config.TitlePrefix != "" { + customEnvVars = append(customEnvVars, fmt.Sprintf(" %s_TITLE_PREFIX: %q\n", p.envPrefix, p.config.TitlePrefix)) + p.log.Printf("title-prefix: %s", p.config.TitlePrefix) + } + + if len(p.config.Labels) > 0 { + labelsJSON, err := json.Marshal(p.config.Labels) + if err == nil { + customEnvVars = append(customEnvVars, fmt.Sprintf(" %s_LABELS: %q\n", p.envPrefix, string(labelsJSON))) + p.log.Printf("labels: %v", p.config.Labels) + } + } + + customEnvVars = append(customEnvVars, buildWorkflowMetadataEnvVarsWithTrackerID(data.Name, data.Source, data.TrackerID)...) + + outputs := map[string]string{ + p.outputKey: fmt.Sprintf("${{ steps.%s.outputs.%s }}", p.kind, p.outputKey), + "total_count": fmt.Sprintf("${{ steps.%s.outputs.total_count }}", p.kind), + } + + jobCondition := BuildSafeOutputType(p.kind) + + permissions := NewPermissionsContentsRead() + if p.config.CreateIssue { + permissions.Set(PermissionIssues, PermissionWrite) + p.log.Printf("Added issues:write permission for create-issue functionality") + } + + script := fmt.Sprintf("const { main } = require('/opt/gh-aw/actions/%s.cjs'); await main();", p.kind) + + return c.buildSafeOutputJob(data, SafeOutputJobConfig{ + JobName: p.kind, + StepName: p.stepName, + StepID: p.kind, + MainJobName: mainJobName, + CustomEnvVars: customEnvVars, + Script: script, + Permissions: permissions, + Outputs: outputs, + Condition: jobCondition, + Token: p.config.GitHubToken, + }) +} + +// parseIssueReportingConfig is the shared parsing implementation for missing-data and +// missing-tool configuration blocks. The caller supplies the YAML key and default title. +func (c *Compiler) parseIssueReportingConfig(outputMap map[string]any, yamlKey, defaultTitle string, log *logger.Logger) *IssueReportingConfig { + configData, exists := outputMap[yamlKey] + if !exists { + return nil + } + + // Explicitly disabled: missing-data: false + if configBool, ok := configData.(bool); ok && !configBool { + log.Printf("%s configuration explicitly disabled", yamlKey) + return nil + } + + cfg := &IssueReportingConfig{} + + // Enabled with no value: missing-data: (nil) + if configData == nil { + log.Printf("%s configuration enabled with defaults", yamlKey) + cfg.CreateIssue = true + cfg.TitlePrefix = defaultTitle + cfg.Labels = []string{} + return cfg + } + + if configMap, ok := configData.(map[string]any); ok { + log.Printf("Parsing %s configuration from map", yamlKey) + c.parseBaseSafeOutputConfig(configMap, &cfg.BaseSafeOutputConfig, 0) + + if createIssue, exists := configMap["create-issue"]; exists { + if createIssueBool, ok := createIssue.(bool); ok { + cfg.CreateIssue = createIssueBool + log.Printf("create-issue: %v", createIssueBool) + } + } else { + cfg.CreateIssue = true + } + + if titlePrefix, exists := configMap["title-prefix"]; exists { + if titlePrefixStr, ok := titlePrefix.(string); ok { + cfg.TitlePrefix = titlePrefixStr + log.Printf("title-prefix: %s", titlePrefixStr) + } + } else { + cfg.TitlePrefix = defaultTitle + } + + if labels, exists := configMap["labels"]; exists { + if labelsArray, ok := labels.([]any); ok { + var labelStrings []string + for _, label := range labelsArray { + if labelStr, ok := label.(string); ok { + labelStrings = append(labelStrings, labelStr) + } + } + cfg.Labels = labelStrings + log.Printf("labels: %v", labelStrings) + } + } else { + cfg.Labels = []string{} + } + } + + return cfg +} + +// envVarPrefix converts a snake_case kind (e.g. "missing_data") to its env-var prefix +// (e.g. "GH_AW_MISSING_DATA"). +func envVarPrefix(kind string) string { + return "GH_AW_" + strings.ToUpper(kind) +} diff --git a/pkg/workflow/missing_tool.go b/pkg/workflow/missing_tool.go index 864d26568ac..9a4b63c91f9 100644 --- a/pkg/workflow/missing_tool.go +++ b/pkg/workflow/missing_tool.go @@ -1,163 +1,31 @@ package workflow import ( - "encoding/json" "errors" - "fmt" "github.com/github/gh-aw/pkg/logger" ) var missingToolLog = logger.New("workflow:missing_tool") -// MissingToolConfig holds configuration for reporting missing tools or functionality -type MissingToolConfig struct { - BaseSafeOutputConfig `yaml:",inline"` - CreateIssue bool `yaml:"create-issue,omitempty"` // Whether to create/update issues for missing tools (default: true) - TitlePrefix string `yaml:"title-prefix,omitempty"` // Prefix for issue titles (default: "[missing tool]") - Labels []string `yaml:"labels,omitempty"` // Labels to add to created issues -} - // buildCreateOutputMissingToolJob creates the missing_tool job func (c *Compiler) buildCreateOutputMissingToolJob(data *WorkflowData, mainJobName string) (*Job, error) { - missingToolLog.Printf("Building missing_tool job for workflow: %s", data.Name) - if data.SafeOutputs == nil || data.SafeOutputs.MissingTool == nil { return nil, errors.New("safe-outputs.missing-tool configuration is required") } - // Build custom environment variables specific to missing-tool - var customEnvVars []string - if data.SafeOutputs.MissingTool.Max != nil { - missingToolLog.Printf("Setting max missing tools limit: %s", *data.SafeOutputs.MissingTool.Max) - customEnvVars = append(customEnvVars, buildTemplatableIntEnvVar("GH_AW_MISSING_TOOL_MAX", data.SafeOutputs.MissingTool.Max)...) - } - - // Add create-issue configuration - if data.SafeOutputs.MissingTool.CreateIssue { - customEnvVars = append(customEnvVars, " GH_AW_MISSING_TOOL_CREATE_ISSUE: \"true\"\n") - missingToolLog.Print("create-issue enabled for missing-tool") - } - - // Add title-prefix configuration - if data.SafeOutputs.MissingTool.TitlePrefix != "" { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_MISSING_TOOL_TITLE_PREFIX: %q\n", data.SafeOutputs.MissingTool.TitlePrefix)) - missingToolLog.Printf("title-prefix: %s", data.SafeOutputs.MissingTool.TitlePrefix) - } - - // Add labels configuration - if len(data.SafeOutputs.MissingTool.Labels) > 0 { - labelsJSON, err := json.Marshal(data.SafeOutputs.MissingTool.Labels) - if err == nil { - customEnvVars = append(customEnvVars, fmt.Sprintf(" GH_AW_MISSING_TOOL_LABELS: %q\n", string(labelsJSON))) - missingToolLog.Printf("labels: %v", data.SafeOutputs.MissingTool.Labels) - } - } - - // Add workflow metadata for consistency - customEnvVars = append(customEnvVars, buildWorkflowMetadataEnvVarsWithTrackerID(data.Name, data.Source, data.TrackerID)...) - - // Create outputs for the job - outputs := map[string]string{ - "tools_reported": "${{ steps.missing_tool.outputs.tools_reported }}", - "total_count": "${{ steps.missing_tool.outputs.total_count }}", - } - - // Build the job condition using BuildSafeOutputType - jobCondition := BuildSafeOutputType("missing_tool") - - // Set permissions based on whether issue creation is enabled - permissions := NewPermissionsContentsRead() - if data.SafeOutputs.MissingTool.CreateIssue { - // Add issues:write permission for creating/updating issues - permissions.Set(PermissionIssues, PermissionWrite) - missingToolLog.Print("Added issues:write permission for create-issue functionality") - } - - // Use the shared builder function to create the job - return c.buildSafeOutputJob(data, SafeOutputJobConfig{ - JobName: "missing_tool", - StepName: "Record Missing Tool", - StepID: "missing_tool", - MainJobName: mainJobName, - CustomEnvVars: customEnvVars, - Script: "const { main } = require('/opt/gh-aw/actions/missing_tool.cjs'); await main();", - Permissions: permissions, - Outputs: outputs, - Condition: jobCondition, - Token: data.SafeOutputs.MissingTool.GitHubToken, + return c.buildIssueReportingJob(data, mainJobName, issueReportingJobParams{ + kind: "missing_tool", + envPrefix: envVarPrefix("missing_tool"), + defaultTitle: "[missing tool]", + outputKey: "tools_reported", + stepName: "Record Missing Tool", + config: data.SafeOutputs.MissingTool, + log: missingToolLog, }) } // parseMissingToolConfig handles missing-tool configuration func (c *Compiler) parseMissingToolConfig(outputMap map[string]any) *MissingToolConfig { - if configData, exists := outputMap["missing-tool"]; exists { - // Handle the case where configData is false (explicitly disabled) - if configBool, ok := configData.(bool); ok && !configBool { - missingToolLog.Print("Missing-tool configuration explicitly disabled") - return nil - } - - // Create config with no defaults - they will be applied in JavaScript - missingToolConfig := &MissingToolConfig{} - - // Handle the case where configData is nil (missing-tool: with no value) - if configData == nil { - missingToolLog.Print("Missing-tool configuration enabled with defaults") - // Set create-issue to true as default when missing-tool is enabled - missingToolConfig.CreateIssue = true - missingToolConfig.TitlePrefix = "[missing tool]" - missingToolConfig.Labels = []string{} - return missingToolConfig - } - - if configMap, ok := configData.(map[string]any); ok { - missingToolLog.Print("Parsing missing-tool configuration from map") - // Parse common base fields with default max of 0 (no limit) - c.parseBaseSafeOutputConfig(configMap, &missingToolConfig.BaseSafeOutputConfig, 0) - - // Parse create-issue field, default to true if not specified - if createIssue, exists := configMap["create-issue"]; exists { - if createIssueBool, ok := createIssue.(bool); ok { - missingToolConfig.CreateIssue = createIssueBool - missingToolLog.Printf("create-issue: %v", createIssueBool) - } - } else { - // Default to true when config map exists but create-issue not specified - missingToolConfig.CreateIssue = true - } - - // Parse title-prefix field, default to "[missing tool]" if not specified - if titlePrefix, exists := configMap["title-prefix"]; exists { - if titlePrefixStr, ok := titlePrefix.(string); ok { - missingToolConfig.TitlePrefix = titlePrefixStr - missingToolLog.Printf("title-prefix: %s", titlePrefixStr) - } - } else { - // Default title prefix - missingToolConfig.TitlePrefix = "[missing tool]" - } - - // Parse labels field, default to empty array if not specified - if labels, exists := configMap["labels"]; exists { - if labelsArray, ok := labels.([]any); ok { - var labelStrings []string - for _, label := range labelsArray { - if labelStr, ok := label.(string); ok { - labelStrings = append(labelStrings, labelStr) - } - } - missingToolConfig.Labels = labelStrings - missingToolLog.Printf("labels: %v", labelStrings) - } - } else { - // Default to empty labels - missingToolConfig.Labels = []string{} - } - } - - return missingToolConfig - } - - return nil + return c.parseIssueReportingConfig(outputMap, "missing-tool", "[missing tool]", missingToolLog) } diff --git a/pkg/workflow/safe_outputs_config.go b/pkg/workflow/safe_outputs_config.go index 4895fda3fd9..9a1a8f666f5 100644 --- a/pkg/workflow/safe_outputs_config.go +++ b/pkg/workflow/safe_outputs_config.go @@ -514,36 +514,36 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut // before parsing the max field from configMap. Supports both integer values and GitHub // Actions expression strings (e.g. "${{ inputs.max }}"). func (c *Compiler) parseBaseSafeOutputConfig(configMap map[string]any, config *BaseSafeOutputConfig, defaultMax int) { -// Set default max if provided -if defaultMax > 0 { -safeOutputsConfigLog.Printf("Setting default max: %d", defaultMax) -config.Max = defaultIntStr(defaultMax) -} + // Set default max if provided + if defaultMax > 0 { + safeOutputsConfigLog.Printf("Setting default max: %d", defaultMax) + config.Max = defaultIntStr(defaultMax) + } -// Parse max (this will override the default if present in configMap) -if max, exists := configMap["max"]; exists { -switch v := max.(type) { -case string: -// Accept GitHub Actions expression strings -if strings.HasPrefix(v, "${{") && strings.HasSuffix(v, "}}") { -safeOutputsConfigLog.Printf("Parsed max as GitHub Actions expression: %s", v) -config.Max = &v -} -default: -// Convert integer/float64/etc to string via parseIntValue -if maxInt, ok := parseIntValue(max); ok { -safeOutputsConfigLog.Printf("Parsed max as integer: %d", maxInt) -s := defaultIntStr(maxInt) -config.Max = s -} -} -} + // Parse max (this will override the default if present in configMap) + if max, exists := configMap["max"]; exists { + switch v := max.(type) { + case string: + // Accept GitHub Actions expression strings + if strings.HasPrefix(v, "${{") && strings.HasSuffix(v, "}}") { + safeOutputsConfigLog.Printf("Parsed max as GitHub Actions expression: %s", v) + config.Max = &v + } + default: + // Convert integer/float64/etc to string via parseIntValue + if maxInt, ok := parseIntValue(max); ok { + safeOutputsConfigLog.Printf("Parsed max as integer: %d", maxInt) + s := defaultIntStr(maxInt) + config.Max = s + } + } + } -// Parse github-token -if githubToken, exists := configMap["github-token"]; exists { -if githubTokenStr, ok := githubToken.(string); ok { -safeOutputsConfigLog.Print("Parsed custom github-token from config") -config.GitHubToken = githubTokenStr -} -} + // Parse github-token + if githubToken, exists := configMap["github-token"]; exists { + if githubTokenStr, ok := githubToken.(string); ok { + safeOutputsConfigLog.Print("Parsed custom github-token from config") + config.GitHubToken = githubTokenStr + } + } }