diff --git a/pkg/workflow/agentic_engine.go b/pkg/workflow/agentic_engine.go index f47a2800c89..5e983cff594 100644 --- a/pkg/workflow/agentic_engine.go +++ b/pkg/workflow/agentic_engine.go @@ -152,7 +152,7 @@ type WorkflowExecutor interface { // Engines that support MCP servers should implement this type MCPConfigProvider interface { // RenderMCPConfig renders the MCP configuration for this engine to the given YAML builder - RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) + RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) error } // LogParser handles parsing and analyzing engine logs @@ -311,8 +311,9 @@ func (e *BaseEngine) GetLogParserScriptId() string { // RenderMCPConfig provides a default no-op implementation for MCP configuration // Engines can override this to provide custom MCP server configuration -func (e *BaseEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) { +func (e *BaseEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) error { // Default implementation does nothing - engines that support MCP should override this + return nil } // convertStepToYAML converts a step map to YAML string - uses proper YAML serialization diff --git a/pkg/workflow/agentic_engine_interfaces_test.go b/pkg/workflow/agentic_engine_interfaces_test.go index c4bb27d8ceb..9378b9b2478 100644 --- a/pkg/workflow/agentic_engine_interfaces_test.go +++ b/pkg/workflow/agentic_engine_interfaces_test.go @@ -105,8 +105,8 @@ func TestInterfaceSegregation(t *testing.T) { ParsedTools: &ToolsConfig{}, Tools: map[string]any{}, } - // Should not panic - engine.RenderMCPConfig(&yaml, map[string]any{}, []string{}, workflowData) + // Should not panic or error + require.NoError(t, engine.RenderMCPConfig(&yaml, map[string]any{}, []string{}, workflowData)) } }) diff --git a/pkg/workflow/agentic_workflow_test.go b/pkg/workflow/agentic_workflow_test.go index a17710d73ff..b946fe32b5e 100644 --- a/pkg/workflow/agentic_workflow_test.go +++ b/pkg/workflow/agentic_workflow_test.go @@ -110,7 +110,8 @@ func TestAgenticWorkflowsMCPConfigGeneration(t *testing.T) { var yaml strings.Builder mcpTools := []string{"agentic-workflows"} - e.engine.RenderMCPConfig(&yaml, workflowData.Tools, mcpTools, workflowData) + err := e.engine.RenderMCPConfig(&yaml, workflowData.Tools, mcpTools, workflowData) + require.NoError(t, err) result := yaml.String() // Verify the MCP config contains agentic-workflows @@ -143,7 +144,7 @@ func TestAgenticWorkflowsInstallStepIncludesGHToken(t *testing.T) { var yaml strings.Builder engine := NewCopilotEngine() - c.generateMCPSetup(&yaml, workflowData.Tools, engine, workflowData) + require.NoError(t, c.generateMCPSetup(&yaml, workflowData.Tools, engine, workflowData)) result := yaml.String() // Verify the install step is present @@ -178,7 +179,7 @@ func TestAgenticWorkflowsInstallStepSkippedWithImport(t *testing.T) { var yaml strings.Builder engine := NewCopilotEngine() - c.generateMCPSetup(&yaml, workflowData.Tools, engine, workflowData) + require.NoError(t, c.generateMCPSetup(&yaml, workflowData.Tools, engine, workflowData)) result := yaml.String() // Verify the install step is NOT present when import exists @@ -203,7 +204,7 @@ func TestAgenticWorkflowsInstallStepPresentWithoutImport(t *testing.T) { var yaml strings.Builder engine := NewCopilotEngine() - c.generateMCPSetup(&yaml, workflowData.Tools, engine, workflowData) + require.NoError(t, c.generateMCPSetup(&yaml, workflowData.Tools, engine, workflowData)) result := yaml.String() // Verify the install step IS present when no import exists diff --git a/pkg/workflow/claude_mcp.go b/pkg/workflow/claude_mcp.go index dad5045940b..33f9a696122 100644 --- a/pkg/workflow/claude_mcp.go +++ b/pkg/workflow/claude_mcp.go @@ -9,7 +9,7 @@ import ( var claudeMCPLog = logger.New("workflow:claude_mcp") // RenderMCPConfig renders the MCP configuration for Claude engine -func (e *ClaudeEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) { +func (e *ClaudeEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) error { claudeMCPLog.Printf("Rendering MCP config for Claude: tool_count=%d, mcp_tool_count=%d", len(tools), len(mcpTools)) // Create unified renderer with Claude-specific options @@ -29,7 +29,7 @@ func (e *ClaudeEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]a gatewayConfig := buildMCPGatewayConfig(workflowData) // Use shared JSON MCP config renderer with unified renderer methods - _ = RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, JSONMCPConfigOptions{ + return RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, JSONMCPConfigOptions{ ConfigPath: "/tmp/gh-aw/mcp-config/mcp-servers.json", GatewayConfig: gatewayConfig, Renderers: MCPToolRenderers{ diff --git a/pkg/workflow/codex_engine_test.go b/pkg/workflow/codex_engine_test.go index b31fb31959d..d185d4f7d46 100644 --- a/pkg/workflow/codex_engine_test.go +++ b/pkg/workflow/codex_engine_test.go @@ -343,7 +343,9 @@ func TestCodexEngineRenderMCPConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var yaml strings.Builder workflowData := &WorkflowData{Name: "test-workflow"} - engine.RenderMCPConfig(&yaml, tt.tools, tt.mcpTools, workflowData) + if err := engine.RenderMCPConfig(&yaml, tt.tools, tt.mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } result := yaml.String() lines := strings.Split(strings.TrimSpace(result), "\n") @@ -416,7 +418,9 @@ func TestCodexEngineUserAgentIdentifierConversion(t *testing.T) { tools := map[string]any{"github": map[string]any{}} mcpTools := []string{"github"} - engine.RenderMCPConfig(&yaml, tools, mcpTools, workflowData) + if err := engine.RenderMCPConfig(&yaml, tools, mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } result := yaml.String() expectedUserAgentLine := "user_agent = \"" + tt.expectedUA + "\"" @@ -487,7 +491,9 @@ func TestCodexEngineRenderMCPConfigUserAgentFromConfig(t *testing.T) { tools := map[string]any{"github": map[string]any{}} mcpTools := []string{"github"} - engine.RenderMCPConfig(&yaml, tools, mcpTools, workflowData) + if err := engine.RenderMCPConfig(&yaml, tools, mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } result := yaml.String() expectedUserAgentLine := "user_agent = \"" + tt.expectedUA + "\"" @@ -604,7 +610,9 @@ func TestCodexEngineRenderMCPConfigUserAgentWithHyphen(t *testing.T) { tools := map[string]any{"github": map[string]any{}} mcpTools := []string{"github"} - engine.RenderMCPConfig(&yaml, tools, mcpTools, workflowData) + if err := engine.RenderMCPConfig(&yaml, tools, mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } result := yaml.String() expectedUserAgentLine := "user_agent = \"" + tt.expectedUA + "\"" @@ -734,7 +742,9 @@ func TestCodexEngineHttpMCPServerRendered(t *testing.T) { t.Run(tt.name, func(t *testing.T) { var yaml strings.Builder workflowData := &WorkflowData{Name: "test-workflow"} - engine.RenderMCPConfig(&yaml, tt.tools, tt.mcpTools, workflowData) + if err := engine.RenderMCPConfig(&yaml, tt.tools, tt.mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } result := yaml.String() diff --git a/pkg/workflow/codex_mcp.go b/pkg/workflow/codex_mcp.go index 829ce3937ea..0a8f2c9efa3 100644 --- a/pkg/workflow/codex_mcp.go +++ b/pkg/workflow/codex_mcp.go @@ -10,7 +10,7 @@ import ( var codexMCPLog = logger.New("workflow:codex_mcp") // RenderMCPConfig generates MCP server configuration for Codex -func (e *CodexEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) { +func (e *CodexEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) error { if codexMCPLog.Enabled() { codexMCPLog.Printf("Rendering MCP config for Codex: mcp_tools=%v, tool_count=%d", mcpTools, len(tools)) } @@ -120,7 +120,7 @@ func (e *CodexEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]an }) } - _ = RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, JSONMCPConfigOptions{ + return RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, JSONMCPConfigOptions{ ConfigPath: "/tmp/gh-aw/mcp-config/mcp-servers.json", GatewayConfig: gatewayConfig, Renderers: MCPToolRenderers{ diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index 558d3f0e108..cf72ecb36c0 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -224,7 +224,9 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat c.generateGitHubMCPAppTokenMintingStep(yaml, data) // Add MCP setup - c.generateMCPSetup(yaml, data.Tools, engine, data) + if err := c.generateMCPSetup(yaml, data.Tools, engine, data); err != nil { + return fmt.Errorf("failed to generate MCP setup: %w", err) + } // Stop-time safety checks are now handled by a dedicated job (stop_time_check) // No longer generated in the main job steps diff --git a/pkg/workflow/copilot_mcp.go b/pkg/workflow/copilot_mcp.go index cfb5fb1d9dd..edfa8730e83 100644 --- a/pkg/workflow/copilot_mcp.go +++ b/pkg/workflow/copilot_mcp.go @@ -9,7 +9,7 @@ import ( var copilotMCPLog = logger.New("workflow:copilot_mcp") // RenderMCPConfig generates MCP server configuration for Copilot CLI -func (e *CopilotEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) { +func (e *CopilotEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) error { copilotMCPLog.Printf("Rendering MCP config for Copilot engine: mcpTools=%d", len(mcpTools)) // Create the directory first @@ -77,7 +77,7 @@ func (e *CopilotEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string] }, } - _ = RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, options) + return RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, options) } // renderCopilotMCPConfigWithContext generates custom MCP server configuration for Copilot CLI diff --git a/pkg/workflow/copilot_mcp_http_integration_test.go b/pkg/workflow/copilot_mcp_http_integration_test.go index 8008f40b7c5..c38f30e4df9 100644 --- a/pkg/workflow/copilot_mcp_http_integration_test.go +++ b/pkg/workflow/copilot_mcp_http_integration_test.go @@ -38,7 +38,9 @@ func TestCopilotEngine_HTTPMCPWithHeaderSecrets_Integration(t *testing.T) { // Test MCP config rendering var mcpConfig strings.Builder mcpTools := []string{"datadog"} - engine.RenderMCPConfig(&mcpConfig, workflowData.Tools, mcpTools, workflowData) + if err := engine.RenderMCPConfig(&mcpConfig, workflowData.Tools, mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } mcpOutput := mcpConfig.String() @@ -203,7 +205,9 @@ func TestCopilotEngine_HTTPMCPWithoutSecrets_Integration(t *testing.T) { // Test MCP config rendering var mcpConfig strings.Builder mcpTools := []string{"custom"} - engine.RenderMCPConfig(&mcpConfig, workflowData.Tools, mcpTools, workflowData) + if err := engine.RenderMCPConfig(&mcpConfig, workflowData.Tools, mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } mcpOutput := mcpConfig.String() diff --git a/pkg/workflow/gemini_mcp.go b/pkg/workflow/gemini_mcp.go index a4f7d666b00..a3d09f9ff34 100644 --- a/pkg/workflow/gemini_mcp.go +++ b/pkg/workflow/gemini_mcp.go @@ -9,7 +9,7 @@ import ( var geminiMCPLog = logger.New("workflow:gemini_mcp") // RenderMCPConfig renders MCP server configuration for Gemini CLI -func (e *GeminiEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) { +func (e *GeminiEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]any, mcpTools []string, workflowData *WorkflowData) error { geminiMCPLog.Printf("Rendering MCP config for Gemini: tool_count=%d, mcp_tool_count=%d", len(tools), len(mcpTools)) // Create unified renderer with Gemini-specific options @@ -24,7 +24,7 @@ func (e *GeminiEngine) RenderMCPConfig(yaml *strings.Builder, tools map[string]a } // Use shared JSON MCP config renderer - _ = RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, JSONMCPConfigOptions{ + return RenderJSONMCPConfig(yaml, tools, mcpTools, workflowData, JSONMCPConfigOptions{ ConfigPath: "/tmp/gh-aw/mcp-config/mcp-servers.json", GatewayConfig: buildMCPGatewayConfig(workflowData), Renderers: MCPToolRenderers{ diff --git a/pkg/workflow/mcp_api_key_masking_test.go b/pkg/workflow/mcp_api_key_masking_test.go index 8d84f9ddee9..48b264e3568 100644 --- a/pkg/workflow/mcp_api_key_masking_test.go +++ b/pkg/workflow/mcp_api_key_masking_test.go @@ -26,7 +26,7 @@ func TestSafeOutputsAPIKeyImmediateMasking(t *testing.T) { mockEngine := NewClaudeEngine() var yaml strings.Builder - compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData) + require.NoError(t, compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData)) output := yaml.String() // Find the Safe Outputs config generation section @@ -93,7 +93,7 @@ func TestSafeInputsAPIKeyImmediateMasking(t *testing.T) { mockEngine := NewClaudeEngine() var yaml strings.Builder - compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData) + require.NoError(t, compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData)) output := yaml.String() // Find the Safe Inputs config generation section @@ -149,7 +149,7 @@ func TestMCPGatewayAPIKeyImmediateMasking(t *testing.T) { mockEngine := NewClaudeEngine() var yaml strings.Builder - compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData) + require.NoError(t, compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData)) output := yaml.String() // Find the MCP gateway API key generation @@ -206,7 +206,7 @@ func TestAPIKeyMaskingNoEmptyDeclaration(t *testing.T) { mockEngine := NewClaudeEngine() var yaml strings.Builder - compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData) + require.NoError(t, compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData)) output := yaml.String() // Verify no empty API key declarations before assignment diff --git a/pkg/workflow/mcp_setup_generator.go b/pkg/workflow/mcp_setup_generator.go index 28999450a1c..c01bf4e43e5 100644 --- a/pkg/workflow/mcp_setup_generator.go +++ b/pkg/workflow/mcp_setup_generator.go @@ -73,14 +73,14 @@ import ( var mcpSetupGeneratorLog = logger.New("workflow:mcp_setup_generator") // generateMCPSetup generates the MCP server configuration setup -func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any, engine CodingAgentEngine, workflowData *WorkflowData) { +func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any, engine CodingAgentEngine, workflowData *WorkflowData) error { mcpSetupGeneratorLog.Print("Generating MCP server configuration setup") // Collect tools that need MCP server configuration var mcpTools []string // Check if workflowData is valid before accessing its fields if workflowData == nil { - return + return nil } workflowTools := workflowData.Tools @@ -138,7 +138,7 @@ func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any, // If no MCP tools, no configuration needed if len(mcpTools) == 0 { mcpSetupGeneratorLog.Print("No MCP tools configured, skipping MCP setup") - return + return nil } // Install gh-aw extension if agentic-workflows tool is enabled @@ -732,5 +732,5 @@ func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any, // Render MCP config - this will pipe directly to the gateway script // The MCP gateway is always enabled, even when agent sandbox is disabled - engine.RenderMCPConfig(yaml, tools, mcpTools, workflowData) + return engine.RenderMCPConfig(yaml, tools, mcpTools, workflowData) } diff --git a/pkg/workflow/mcp_setup_generator_test.go b/pkg/workflow/mcp_setup_generator_test.go index fd15d2d6976..dfe4324e553 100644 --- a/pkg/workflow/mcp_setup_generator_test.go +++ b/pkg/workflow/mcp_setup_generator_test.go @@ -63,7 +63,7 @@ func TestSafeInputsStepCodeGenerationStability(t *testing.T) { for i := 0; i < iterations; i++ { var yaml strings.Builder - compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData) + require.NoError(t, compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData)) outputs[i] = yaml.String() } @@ -219,7 +219,7 @@ func TestMCPGatewayVersionFromFrontmatter(t *testing.T) { var yaml strings.Builder mockEngine := NewClaudeEngine() - compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData) + require.NoError(t, compiler.generateMCPSetup(&yaml, workflowData.Tools, mockEngine, workflowData)) setupOutput := yaml.String() // The setup output should contain the container image with the correct version diff --git a/pkg/workflow/tools_timeout_test.go b/pkg/workflow/tools_timeout_test.go index 4ccd549368b..e52fd8b40b2 100644 --- a/pkg/workflow/tools_timeout_test.go +++ b/pkg/workflow/tools_timeout_test.go @@ -137,7 +137,9 @@ func TestCodexEngineWithToolsTimeout(t *testing.T) { // Render MCP config var configBuilder strings.Builder mcpTools := []string{"github"} - engine.RenderMCPConfig(&configBuilder, workflowData.Tools, mcpTools, workflowData) + if err := engine.RenderMCPConfig(&configBuilder, workflowData.Tools, mcpTools, workflowData); err != nil { + t.Fatalf("RenderMCPConfig returned unexpected error: %v", err) + } configContent := configBuilder.String() if !strings.Contains(configContent, tt.expectedTimeout) {