From b0681201baecc5b111f55aa47ce6c70f14271aca Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Apr 2026 11:51:18 +0000 Subject: [PATCH 1/2] Initial plan From 0117191521e0949fc21b9a4796e269503e514809 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Apr 2026 12:17:18 +0000 Subject: [PATCH 2/2] refactor: eliminate engine installation and secret logic duplication - Add BuildNpmEngineInstallStepsWithAWF helper to remove AWF-injection boilerplate from Claude, Gemini, and Copilot GetInstallationSteps - Add BuildDefaultSecretValidationStep helper to consolidate the custom-command guard + GenerateMultiSecretValidationStep call across all four engine GetSecretValidationStep implementations - Add collectCommonMCPSecrets helper to remove repeated MCP_GATEWAY_API_KEY and mcp-scripts secret collection from Claude, Codex, and Gemini GetRequiredSecretNames implementations Net: -118 lines; no behavioral changes; all tests pass Agent-Logs-Url: https://github.com/github/gh-aw/sessions/03077ca9-404d-4c4f-b816-eb0aaa4c505c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/claude_engine.go | 90 +++----------------- pkg/workflow/codex_engine.go | 27 +----- pkg/workflow/copilot_engine_installation.go | 71 ++-------------- pkg/workflow/engine_helpers.go | 90 ++++++++++++++++++++ pkg/workflow/gemini_engine.go | 92 +++------------------ 5 files changed, 126 insertions(+), 244 deletions(-) diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index 06036ad9270..5a3eeb9856f 100644 --- a/pkg/workflow/claude_engine.go +++ b/pkg/workflow/claude_engine.go @@ -46,38 +46,19 @@ func (e *ClaudeEngine) GetAPMTarget() string { } // GetRequiredSecretNames returns the list of secrets required by the Claude engine -// This includes ANTHROPIC_API_KEY and optionally MCP_GATEWAY_API_KEY +// This includes ANTHROPIC_API_KEY and optionally MCP_GATEWAY_API_KEY and mcp-scripts secrets func (e *ClaudeEngine) GetRequiredSecretNames(workflowData *WorkflowData) []string { - secrets := []string{"ANTHROPIC_API_KEY"} - - // Add MCP gateway API key if MCP servers are present (gateway is always started with MCP servers) - if HasMCPServers(workflowData) { - secrets = append(secrets, "MCP_GATEWAY_API_KEY") - } - - // Add mcp-scripts secret names - if IsMCPScriptsEnabled(workflowData.MCPScripts, workflowData) { - mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) - for varName := range mcpScriptsSecrets { - secrets = append(secrets, varName) - } - } - - return secrets + return append([]string{"ANTHROPIC_API_KEY"}, collectCommonMCPSecrets(workflowData)...) } // GetSecretValidationStep returns the secret validation step for the Claude engine. // Returns an empty step if custom command is specified. func (e *ClaudeEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep { - if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" { - claudeLog.Printf("Skipping secret validation step: custom command specified (%s)", workflowData.EngineConfig.Command) - return GitHubActionStep{} - } - return GenerateMultiSecretValidationStep( + return BuildDefaultSecretValidationStep( + workflowData, []string{"ANTHROPIC_API_KEY"}, "Claude Code", "https://github.github.com/gh-aw/reference/engines/#anthropic-claude-code", - getEngineEnvOverrides(workflowData), ) } @@ -90,63 +71,14 @@ func (e *ClaudeEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHub return []GitHubActionStep{} } - var steps []GitHubActionStep - - // Define engine configuration for shared validation - config := EngineInstallConfig{ - Secrets: []string{"ANTHROPIC_API_KEY"}, - DocsURL: "https://github.github.com/gh-aw/reference/engines/#anthropic-claude-code", - NpmPackage: "@anthropic-ai/claude-code", - Version: string(constants.DefaultClaudeCodeVersion), - Name: "Claude Code", - CliName: "claude", - InstallStepName: "Install Claude Code CLI", - } - - // Secret validation step is now generated in the activation job (GetSecretValidationStep). - - // Determine Claude version - claudeVersion := config.Version - if workflowData.EngineConfig != nil && workflowData.EngineConfig.Version != "" { - claudeVersion = workflowData.EngineConfig.Version - } - - // Add Node.js setup step first (before sandbox installation) - npmSteps := GenerateNpmInstallSteps( - config.NpmPackage, - claudeVersion, - config.InstallStepName, - config.CliName, - true, // Include Node.js setup + npmSteps := BuildStandardNpmEngineInstallSteps( + "@anthropic-ai/claude-code", + string(constants.DefaultClaudeCodeVersion), + "Install Claude Code CLI", + "claude", + workflowData, ) - - if len(npmSteps) > 0 { - steps = append(steps, npmSteps[0]) // Setup Node.js step - } - - // Add AWF installation if firewall is enabled - if isFirewallEnabled(workflowData) { - // Install AWF after Node.js setup but before Claude CLI installation - firewallConfig := getFirewallConfig(workflowData) - agentConfig := getAgentConfig(workflowData) - var awfVersion string - if firewallConfig != nil { - awfVersion = firewallConfig.Version - } - - // Install AWF binary (or skip if custom command is specified) - awfInstall := generateAWFInstallationStep(awfVersion, agentConfig) - if len(awfInstall) > 0 { - steps = append(steps, awfInstall) - } - } - - // Add Claude CLI installation step after sandbox installation - if len(npmSteps) > 1 { - steps = append(steps, npmSteps[1:]...) // Install Claude CLI and subsequent steps - } - - return steps + return BuildNpmEngineInstallStepsWithAWF(npmSteps, workflowData) } // GetDeclaredOutputFiles returns the output files that Claude may produce diff --git a/pkg/workflow/codex_engine.go b/pkg/workflow/codex_engine.go index 969003d821a..f3f22dacd5d 100644 --- a/pkg/workflow/codex_engine.go +++ b/pkg/workflow/codex_engine.go @@ -53,38 +53,19 @@ func (e *CodexEngine) GetModelEnvVarName() string { } // GetRequiredSecretNames returns the list of secrets required by the Codex engine -// This includes CODEX_API_KEY, OPENAI_API_KEY, and optionally MCP_GATEWAY_API_KEY +// This includes CODEX_API_KEY, OPENAI_API_KEY, and optionally MCP_GATEWAY_API_KEY and mcp-scripts secrets func (e *CodexEngine) GetRequiredSecretNames(workflowData *WorkflowData) []string { - secrets := []string{"CODEX_API_KEY", "OPENAI_API_KEY"} - - // Add MCP gateway API key if MCP servers are present (gateway is always started with MCP servers) - if HasMCPServers(workflowData) { - secrets = append(secrets, "MCP_GATEWAY_API_KEY") - } - - // Add mcp-scripts secret names - if IsMCPScriptsEnabled(workflowData.MCPScripts, workflowData) { - mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) - for varName := range mcpScriptsSecrets { - secrets = append(secrets, varName) - } - } - - return secrets + return append([]string{"CODEX_API_KEY", "OPENAI_API_KEY"}, collectCommonMCPSecrets(workflowData)...) } // GetSecretValidationStep returns the secret validation step for the Codex engine. // Returns an empty step if custom command is specified. func (e *CodexEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep { - if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" { - codexEngineLog.Printf("Skipping secret validation step: custom command specified (%s)", workflowData.EngineConfig.Command) - return GitHubActionStep{} - } - return GenerateMultiSecretValidationStep( + return BuildDefaultSecretValidationStep( + workflowData, []string{"CODEX_API_KEY", "OPENAI_API_KEY"}, "Codex", "https://github.github.com/gh-aw/reference/engines/#openai-codex", - getEngineEnvOverrides(workflowData), ) } diff --git a/pkg/workflow/copilot_engine_installation.go b/pkg/workflow/copilot_engine_installation.go index f53ea7e7c2a..15a3fea557e 100644 --- a/pkg/workflow/copilot_engine_installation.go +++ b/pkg/workflow/copilot_engine_installation.go @@ -26,19 +26,15 @@ var copilotInstallLog = logger.New("workflow:copilot_engine_installation") // GetSecretValidationStep returns the secret validation step for the Copilot engine. // Returns an empty step if copilot-requests feature is enabled or custom command is specified. func (e *CopilotEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep { - if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" { - copilotInstallLog.Printf("Skipping secret validation step: custom command specified (%s)", workflowData.EngineConfig.Command) - return GitHubActionStep{} - } if isFeatureEnabled(constants.CopilotRequestsFeatureFlag, workflowData) { copilotInstallLog.Print("Skipping secret validation step: copilot-requests feature enabled, using GitHub Actions token") return GitHubActionStep{} } - return GenerateMultiSecretValidationStep( + return BuildDefaultSecretValidationStep( + workflowData, []string{"COPILOT_GITHUB_TOKEN"}, "GitHub Copilot CLI", "https://github.github.com/gh-aw/reference/engines/#github-copilot-default", - getEngineEnvOverrides(workflowData), ) } @@ -47,7 +43,7 @@ func (e *CopilotEngine) GetSecretValidationStep(workflowData *WorkflowData) GitH // Secret validation is handled separately in the activation job via GetSecretValidationStep. // The installation order is: // 1. Node.js setup -// 2. Sandbox installation (SRT or AWF, if needed) +// 2. Sandbox installation (AWF, if needed) // 3. Copilot CLI installation // // If a custom command is specified in the engine configuration, this function returns @@ -61,67 +57,16 @@ func (e *CopilotEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHu return []GitHubActionStep{} } - var steps []GitHubActionStep - - // Define engine configuration for shared validation - config := EngineInstallConfig{ - Secrets: []string{"COPILOT_GITHUB_TOKEN"}, - DocsURL: "https://github.github.com/gh-aw/reference/engines/#github-copilot-default", - NpmPackage: "@github/copilot", - Version: string(constants.DefaultCopilotVersion), - Name: "GitHub Copilot CLI", - CliName: "copilot", - InstallStepName: "Install GitHub Copilot CLI", - } - - // Secret validation step is now generated in the activation job (GetSecretValidationStep). - // Determine Copilot version - copilotVersion := config.Version + copilotVersion := string(constants.DefaultCopilotVersion) if workflowData.EngineConfig != nil && workflowData.EngineConfig.Version != "" { copilotVersion = workflowData.EngineConfig.Version } - // Determine if Copilot should be installed globally - // Always install globally now (SRT removed) - installGlobally := true - - // Generate install steps based on installation scope - var npmSteps []GitHubActionStep - if installGlobally { - // Use the new installer script for global installation - copilotInstallLog.Print("Using new installer script for Copilot installation") - npmSteps = GenerateCopilotInstallerSteps(copilotVersion, config.InstallStepName) - } - - // Add Node.js setup step first (before sandbox installation) - if len(npmSteps) > 0 { - steps = append(steps, npmSteps[0]) // Setup Node.js step - } - - // Add sandbox installation steps (AWF only) - if isFirewallEnabled(workflowData) { - // Install AWF after Node.js setup but before Copilot CLI installation - firewallConfig := getFirewallConfig(workflowData) - agentConfig := getAgentConfig(workflowData) - var awfVersion string - if firewallConfig != nil { - awfVersion = firewallConfig.Version - } - - // Install AWF binary (or skip if custom command is specified) - awfInstall := generateAWFInstallationStep(awfVersion, agentConfig) - if len(awfInstall) > 0 { - steps = append(steps, awfInstall) - } - } - - // Add Copilot CLI installation step after sandbox installation - if len(npmSteps) > 1 { - steps = append(steps, npmSteps[1:]...) // Install Copilot CLI and subsequent steps - } - - return steps + // Use the installer script for global installation + copilotInstallLog.Print("Using new installer script for Copilot installation") + npmSteps := GenerateCopilotInstallerSteps(copilotVersion, "Install GitHub Copilot CLI") + return BuildNpmEngineInstallStepsWithAWF(npmSteps, workflowData) } // generateAWFInstallationStep creates a GitHub Actions step to install the AWF binary diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index 81f4715edc1..78562ac7706 100644 --- a/pkg/workflow/engine_helpers.go +++ b/pkg/workflow/engine_helpers.go @@ -181,6 +181,96 @@ func BuildStandardNpmEngineInstallSteps( ) } +// BuildNpmEngineInstallStepsWithAWF injects an AWF installation step between the Node.js +// setup step and the CLI install steps when the firewall is enabled. This eliminates the +// duplicated AWF-injection pattern shared by Claude, Gemini, and Copilot engines. +// +// The expected layout of npmSteps is: +// - npmSteps[0] – Node.js setup step +// - npmSteps[1:] – CLI installation step(s) +// +// Parameters: +// - npmSteps: Pre-computed npm installation steps (from BuildStandardNpmEngineInstallSteps +// or GenerateCopilotInstallerSteps) +// - workflowData: The workflow data (used to determine firewall configuration) +// +// Returns: +// - []GitHubActionStep: Steps in order: Node.js setup, AWF (if enabled), CLI install +func BuildNpmEngineInstallStepsWithAWF(npmSteps []GitHubActionStep, workflowData *WorkflowData) []GitHubActionStep { + var steps []GitHubActionStep + + if len(npmSteps) > 0 { + steps = append(steps, npmSteps[0]) // Node.js setup step + } + + // Inject AWF installation after Node.js setup but before the CLI install steps + if isFirewallEnabled(workflowData) { + firewallConfig := getFirewallConfig(workflowData) + agentConfig := getAgentConfig(workflowData) + var awfVersion string + if firewallConfig != nil { + awfVersion = firewallConfig.Version + } + awfInstall := generateAWFInstallationStep(awfVersion, agentConfig) + if len(awfInstall) > 0 { + steps = append(steps, awfInstall) + } + } + + if len(npmSteps) > 1 { + steps = append(steps, npmSteps[1:]...) // CLI installation and subsequent steps + } + + return steps +} + +// BuildDefaultSecretValidationStep returns a secret validation step for the given engine +// configuration, or an empty step when a custom command is specified. This consolidates +// the common guard+delegate pattern shared across all engine GetSecretValidationStep +// implementations. +// +// Parameters: +// - workflowData: The workflow data (checked for custom command) +// - secrets: The secret names to validate (e.g., []string{"ANTHROPIC_API_KEY"}) +// - name: The engine display name used in the step (e.g., "Claude Code") +// - docsURL: The documentation URL shown when validation fails +// +// Returns: +// - GitHubActionStep: The validation step, or an empty step if a custom command is set +func BuildDefaultSecretValidationStep(workflowData *WorkflowData, secrets []string, name, docsURL string) GitHubActionStep { + if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" { + engineHelpersLog.Printf("Skipping secret validation step: custom command specified (%s)", workflowData.EngineConfig.Command) + return GitHubActionStep{} + } + return GenerateMultiSecretValidationStep(secrets, name, docsURL, getEngineEnvOverrides(workflowData)) +} + +// collectCommonMCPSecrets returns the MCP-related secret names shared across all engines: +// - MCP_GATEWAY_API_KEY (when MCP servers are present) +// - mcp-scripts secrets (when mcp-scripts feature is enabled) +// +// Parameters: +// - workflowData: The workflow data used to check MCP server and mcp-scripts configuration +// +// Returns: +// - []string: Common MCP secret names (may be empty) +func collectCommonMCPSecrets(workflowData *WorkflowData) []string { + var secrets []string + + if HasMCPServers(workflowData) { + secrets = append(secrets, "MCP_GATEWAY_API_KEY") + } + + if IsMCPScriptsEnabled(workflowData.MCPScripts, workflowData) { + mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) + for varName := range mcpScriptsSecrets { + secrets = append(secrets, varName) + } + } + + return secrets +} + // RenderCustomMCPToolConfigHandler is a function type that engines must provide to render their specific MCP config // FormatStepWithCommandAndEnv formats a GitHub Actions step with command and environment variables. // This shared function extracts the common pattern used by Copilot and Codex engines. diff --git a/pkg/workflow/gemini_engine.go b/pkg/workflow/gemini_engine.go index 1787d6a2854..99c960a7a14 100644 --- a/pkg/workflow/gemini_engine.go +++ b/pkg/workflow/gemini_engine.go @@ -38,16 +38,14 @@ func (e *GeminiEngine) GetModelEnvVarName() string { } // GetRequiredSecretNames returns the list of secrets required by the Gemini engine -// This includes GEMINI_API_KEY and optionally MCP_GATEWAY_API_KEY +// This includes GEMINI_API_KEY and optionally MCP_GATEWAY_API_KEY, GITHUB_MCP_SERVER_TOKEN, +// HTTP MCP header secrets, and mcp-scripts secrets func (e *GeminiEngine) GetRequiredSecretNames(workflowData *WorkflowData) []string { geminiLog.Print("Collecting required secrets for Gemini engine") secrets := []string{"GEMINI_API_KEY"} - // Add MCP gateway API key if MCP servers are present (gateway is always started with MCP servers) - if HasMCPServers(workflowData) { - geminiLog.Print("Adding MCP_GATEWAY_API_KEY secret") - secrets = append(secrets, "MCP_GATEWAY_API_KEY") - } + // Add common MCP secrets (MCP_GATEWAY_API_KEY if MCP servers present, mcp-scripts secrets) + secrets = append(secrets, collectCommonMCPSecrets(workflowData)...) // Add GitHub token for GitHub MCP server if present if hasGitHubTool(workflowData.ParsedTools) { @@ -64,32 +62,17 @@ func (e *GeminiEngine) GetRequiredSecretNames(workflowData *WorkflowData) []stri geminiLog.Printf("Added %d HTTP MCP header secrets", len(headerSecrets)) } - // Add mcp-scripts secret names - if IsMCPScriptsEnabled(workflowData.MCPScripts, workflowData) { - mcpScriptsSecrets := collectMCPScriptsSecrets(workflowData.MCPScripts) - for varName := range mcpScriptsSecrets { - secrets = append(secrets, varName) - } - if len(mcpScriptsSecrets) > 0 { - geminiLog.Printf("Added %d mcp-scripts secrets", len(mcpScriptsSecrets)) - } - } - return secrets } // GetSecretValidationStep returns the secret validation step for the Gemini engine. // Returns an empty step if custom command is specified. func (e *GeminiEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep { - if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" { - geminiLog.Printf("Skipping secret validation step: custom command specified (%s)", workflowData.EngineConfig.Command) - return GitHubActionStep{} - } - return GenerateMultiSecretValidationStep( + return BuildDefaultSecretValidationStep( + workflowData, []string{"GEMINI_API_KEY"}, "Gemini CLI", "https://geminicli.com/docs/get-started/authentication/", - getEngineEnvOverrides(workflowData), ) } @@ -102,63 +85,14 @@ func (e *GeminiEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHub return []GitHubActionStep{} } - var steps []GitHubActionStep - - // Define engine configuration for shared validation - config := EngineInstallConfig{ - Secrets: []string{"GEMINI_API_KEY"}, - DocsURL: "https://geminicli.com/docs/get-started/authentication/", - NpmPackage: "@google/gemini-cli", - Version: string(constants.DefaultGeminiVersion), - Name: "Gemini CLI", - CliName: "gemini", - InstallStepName: "Install Gemini CLI", - } - - // Secret validation step is now generated in the activation job (GetSecretValidationStep). - - // Determine Gemini version - geminiVersion := config.Version - if workflowData.EngineConfig != nil && workflowData.EngineConfig.Version != "" { - geminiVersion = workflowData.EngineConfig.Version - } - - // Add Node.js setup step first (before sandbox installation) - npmSteps := GenerateNpmInstallSteps( - config.NpmPackage, - geminiVersion, - config.InstallStepName, - config.CliName, - true, // Include Node.js setup + npmSteps := BuildStandardNpmEngineInstallSteps( + "@google/gemini-cli", + string(constants.DefaultGeminiVersion), + "Install Gemini CLI", + "gemini", + workflowData, ) - - if len(npmSteps) > 0 { - steps = append(steps, npmSteps[0]) // Setup Node.js step - } - - // Add AWF installation if firewall is enabled - if isFirewallEnabled(workflowData) { - // Install AWF after Node.js setup but before Gemini CLI installation - firewallConfig := getFirewallConfig(workflowData) - agentConfig := getAgentConfig(workflowData) - var awfVersion string - if firewallConfig != nil { - awfVersion = firewallConfig.Version - } - - // Install AWF binary (or skip if custom command is specified) - awfInstall := generateAWFInstallationStep(awfVersion, agentConfig) - if len(awfInstall) > 0 { - steps = append(steps, awfInstall) - } - } - - // Add Gemini CLI installation step after sandbox installation - if len(npmSteps) > 1 { - steps = append(steps, npmSteps[1:]...) // Install Gemini CLI and subsequent steps - } - - return steps + return BuildNpmEngineInstallStepsWithAWF(npmSteps, workflowData) } // GetDeclaredOutputFiles returns the output files that Gemini may produce.