diff --git a/.changeset/patch-extract-github-mcp-remote-config.md b/.changeset/patch-extract-github-mcp-remote-config.md new file mode 100644 index 00000000000..4a584b3daf0 --- /dev/null +++ b/.changeset/patch-extract-github-mcp-remote-config.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Refactor: Extract duplicate GitHub MCP remote config rendering into shared helper diff --git a/pkg/workflow/claude_engine.go b/pkg/workflow/claude_engine.go index cccaa8a8312..19ffaaa8265 100644 --- a/pkg/workflow/claude_engine.go +++ b/pkg/workflow/claude_engine.go @@ -653,32 +653,18 @@ func (e *ClaudeEngine) renderGitHubClaudeMCPConfig(yaml *strings.Builder, github // Check if remote mode is enabled (type: remote) if githubType == "remote" { - // Remote mode - use hosted GitHub MCP server - yaml.WriteString(" \"type\": \"http\",\n") - yaml.WriteString(" \"url\": \"https://api.githubcopilot.com/mcp/\",\n") - yaml.WriteString(" \"headers\": {\n") - // Use effective token with precedence: custom > top-level > default effectiveToken := getEffectiveGitHubToken(customGitHubToken, workflowData.GitHubToken) - // Collect headers in a map - headers := make(map[string]string) - headers["Authorization"] = fmt.Sprintf("Bearer %s", effectiveToken) - - // Add X-MCP-Readonly header if read-only mode is enabled - if readOnly { - headers["X-MCP-Readonly"] = "true" - } - - // Add X-MCP-Toolsets header if toolsets are configured - if toolsets != "" { - headers["X-MCP-Toolsets"] = toolsets - } - - // Write headers using helper - writeHeadersToYAML(yaml, headers, " ") - - yaml.WriteString(" }\n") + // Render remote configuration using shared helper + RenderGitHubMCPRemoteConfig(yaml, GitHubMCPRemoteOptions{ + ReadOnly: readOnly, + Toolsets: toolsets, + AuthorizationValue: fmt.Sprintf("Bearer %s", effectiveToken), + IncludeToolsField: false, // Claude doesn't use tools field + AllowedTools: nil, + IncludeEnvSection: false, // Claude doesn't use env section + }) } else { // Local mode - use Docker-based GitHub MCP server (default) githubDockerImageVersion := getGitHubDockerImageVersion(githubTool) diff --git a/pkg/workflow/copilot_engine.go b/pkg/workflow/copilot_engine.go index c7f31506a8c..e7ab8d256d3 100644 --- a/pkg/workflow/copilot_engine.go +++ b/pkg/workflow/copilot_engine.go @@ -399,49 +399,15 @@ func (e *CopilotEngine) renderGitHubCopilotMCPConfig(yaml *strings.Builder, gith // Check if remote mode is enabled (type: remote) if githubType == "remote" { - // Remote mode - use hosted GitHub MCP server - yaml.WriteString(" \"type\": \"http\",\n") - yaml.WriteString(" \"url\": \"https://api.githubcopilot.com/mcp/\",\n") - yaml.WriteString(" \"headers\": {\n") - - // Collect headers in a map - headers := make(map[string]string) - headers["Authorization"] = "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}" - - // Add X-MCP-Readonly header if read-only mode is enabled - if readOnly { - headers["X-MCP-Readonly"] = "true" - } - - // Add X-MCP-Toolsets header if toolsets are configured - if toolsets != "" { - headers["X-MCP-Toolsets"] = toolsets - } - - // Write headers using helper - writeHeadersToYAML(yaml, headers, " ") - - yaml.WriteString(" },\n") - - // Populate tools field with allowed tools or "*" if none specified - if len(allowedTools) > 0 { - yaml.WriteString(" \"tools\": [\n") - for i, tool := range allowedTools { - comma := "," - if i == len(allowedTools)-1 { - comma = "" - } - fmt.Fprintf(yaml, " \"%s\"%s\n", tool, comma) - } - yaml.WriteString(" ],\n") - } else { - yaml.WriteString(" \"tools\": [\"*\"],\n") - } - - // Add env section for passthrough - yaml.WriteString(" \"env\": {\n") - yaml.WriteString(" \"GITHUB_PERSONAL_ACCESS_TOKEN\": \"\\${GITHUB_PERSONAL_ACCESS_TOKEN}\"\n") - yaml.WriteString(" }\n") + // Render remote configuration using shared helper + RenderGitHubMCPRemoteConfig(yaml, GitHubMCPRemoteOptions{ + ReadOnly: readOnly, + Toolsets: toolsets, + AuthorizationValue: "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}", + IncludeToolsField: true, // Copilot uses tools field + AllowedTools: allowedTools, + IncludeEnvSection: true, // Copilot uses env section for passthrough + }) } else { // Local mode - use Docker-based GitHub MCP server (default) githubDockerImageVersion := getGitHubDockerImageVersion(githubTool) diff --git a/pkg/workflow/engine_shared_helpers.go b/pkg/workflow/engine_shared_helpers.go index 7a3d78c22a3..ec8627712f1 100644 --- a/pkg/workflow/engine_shared_helpers.go +++ b/pkg/workflow/engine_shared_helpers.go @@ -222,6 +222,85 @@ func RenderGitHubMCPDockerConfig(yaml *strings.Builder, options GitHubMCPDockerO yaml.WriteString(" }\n") } +// GitHubMCPRemoteOptions defines configuration for GitHub MCP remote mode rendering +type GitHubMCPRemoteOptions struct { + // ReadOnly enables read-only mode for GitHub API operations + ReadOnly bool + // Toolsets specifies the GitHub toolsets to enable + Toolsets string + // AuthorizationValue is the value for the Authorization header + // For Claude: "Bearer {effectiveToken}" + // For Copilot: "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}" + AuthorizationValue string + // IncludeToolsField indicates whether to include the "tools" field (Copilot needs it, Claude doesn't) + IncludeToolsField bool + // AllowedTools specifies the list of allowed tools (Copilot uses this, Claude doesn't) + AllowedTools []string + // IncludeEnvSection indicates whether to include the env section (Copilot needs it, Claude doesn't) + IncludeEnvSection bool +} + +// RenderGitHubMCPRemoteConfig renders the GitHub MCP server configuration for remote (hosted) mode. +// This shared function extracts the duplicate pattern from Claude and Copilot engines. +// +// Parameters: +// - yaml: The string builder for YAML output +// - options: GitHub MCP remote rendering options +func RenderGitHubMCPRemoteConfig(yaml *strings.Builder, options GitHubMCPRemoteOptions) { + // Remote mode - use hosted GitHub MCP server + yaml.WriteString(" \"type\": \"http\",\n") + yaml.WriteString(" \"url\": \"https://api.githubcopilot.com/mcp/\",\n") + yaml.WriteString(" \"headers\": {\n") + + // Collect headers in a map + headers := make(map[string]string) + headers["Authorization"] = options.AuthorizationValue + + // Add X-MCP-Readonly header if read-only mode is enabled + if options.ReadOnly { + headers["X-MCP-Readonly"] = "true" + } + + // Add X-MCP-Toolsets header if toolsets are configured + if options.Toolsets != "" { + headers["X-MCP-Toolsets"] = options.Toolsets + } + + // Write headers using helper + writeHeadersToYAML(yaml, headers, " ") + + // Close headers section + if options.IncludeToolsField || options.IncludeEnvSection { + yaml.WriteString(" },\n") + } else { + yaml.WriteString(" }\n") + } + + // Add tools field if needed (Copilot uses this, Claude doesn't) + if options.IncludeToolsField { + if len(options.AllowedTools) > 0 { + yaml.WriteString(" \"tools\": [\n") + for i, tool := range options.AllowedTools { + comma := "," + if i == len(options.AllowedTools)-1 { + comma = "" + } + fmt.Fprintf(yaml, " \"%s\"%s\n", tool, comma) + } + yaml.WriteString(" ],\n") + } else { + yaml.WriteString(" \"tools\": [\"*\"],\n") + } + } + + // Add env section if needed (Copilot uses this, Claude doesn't) + if options.IncludeEnvSection { + yaml.WriteString(" \"env\": {\n") + yaml.WriteString(" \"GITHUB_PERSONAL_ACCESS_TOKEN\": \"\\${GITHUB_PERSONAL_ACCESS_TOKEN}\"\n") + yaml.WriteString(" }\n") + } +} + // RenderJSONMCPConfig renders MCP configuration in JSON format with the common mcpServers structure. // This shared function extracts the duplicate pattern from Claude, Copilot, and Custom engines. // diff --git a/pkg/workflow/github_remote_config_test.go b/pkg/workflow/github_remote_config_test.go new file mode 100644 index 00000000000..b89f2553b82 --- /dev/null +++ b/pkg/workflow/github_remote_config_test.go @@ -0,0 +1,240 @@ +package workflow + +import ( + "strings" + "testing" +) + +func TestRenderGitHubMCPRemoteConfig(t *testing.T) { + tests := []struct { + name string + options GitHubMCPRemoteOptions + expectedOutput []string // Expected strings to be present in output + notExpected []string // Strings that should NOT be present + }{ + { + name: "Claude-style config without tools or env", + options: GitHubMCPRemoteOptions{ + ReadOnly: false, + Toolsets: "default", + AuthorizationValue: "Bearer ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}", + IncludeToolsField: false, + AllowedTools: nil, + IncludeEnvSection: false, + }, + expectedOutput: []string{ + `"type": "http"`, + `"url": "https://api.githubcopilot.com/mcp/"`, + `"headers": {`, + `"Authorization": "Bearer ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}"`, + `"X-MCP-Toolsets": "default"`, + }, + notExpected: []string{ + `"tools"`, + `"env"`, + `"X-MCP-Readonly"`, + }, + }, + { + name: "Claude-style config with read-only", + options: GitHubMCPRemoteOptions{ + ReadOnly: true, + Toolsets: "repos,issues", + AuthorizationValue: "Bearer ${{ secrets.CUSTOM_PAT }}", + IncludeToolsField: false, + AllowedTools: nil, + IncludeEnvSection: false, + }, + expectedOutput: []string{ + `"type": "http"`, + `"url": "https://api.githubcopilot.com/mcp/"`, + `"headers": {`, + `"Authorization": "Bearer ${{ secrets.CUSTOM_PAT }}"`, + `"X-MCP-Readonly": "true"`, + `"X-MCP-Toolsets": "repos,issues"`, + }, + notExpected: []string{ + `"tools"`, + `"env"`, + }, + }, + { + name: "Copilot-style config with tools and env", + options: GitHubMCPRemoteOptions{ + ReadOnly: false, + Toolsets: "default", + AuthorizationValue: "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}", + IncludeToolsField: true, + AllowedTools: []string{"list_issues", "create_issue"}, + IncludeEnvSection: true, + }, + expectedOutput: []string{ + `"type": "http"`, + `"url": "https://api.githubcopilot.com/mcp/"`, + `"headers": {`, + `"Authorization": "Bearer \${GITHUB_PERSONAL_ACCESS_TOKEN}"`, + `"X-MCP-Toolsets": "default"`, + `"tools": [`, + `"list_issues"`, + `"create_issue"`, + `"env": {`, + `"GITHUB_PERSONAL_ACCESS_TOKEN": "\${GITHUB_PERSONAL_ACCESS_TOKEN}"`, + }, + notExpected: []string{ + `"X-MCP-Readonly"`, + }, + }, + { + name: "Copilot-style config with wildcard tools", + options: GitHubMCPRemoteOptions{ + ReadOnly: false, + Toolsets: "all", + AuthorizationValue: "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}", + IncludeToolsField: true, + AllowedTools: nil, // Empty array should result in wildcard + IncludeEnvSection: true, + }, + expectedOutput: []string{ + `"type": "http"`, + `"url": "https://api.githubcopilot.com/mcp/"`, + `"headers": {`, + `"Authorization": "Bearer \${GITHUB_PERSONAL_ACCESS_TOKEN}"`, + `"X-MCP-Toolsets": "all"`, + `"tools": ["*"]`, + `"env": {`, + `"GITHUB_PERSONAL_ACCESS_TOKEN": "\${GITHUB_PERSONAL_ACCESS_TOKEN}"`, + }, + notExpected: []string{ + `"X-MCP-Readonly"`, + }, + }, + { + name: "Copilot-style config with read-only and specific tools", + options: GitHubMCPRemoteOptions{ + ReadOnly: true, + Toolsets: "repos", + AuthorizationValue: "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}", + IncludeToolsField: true, + AllowedTools: []string{"list_repositories", "get_repository"}, + IncludeEnvSection: true, + }, + expectedOutput: []string{ + `"type": "http"`, + `"url": "https://api.githubcopilot.com/mcp/"`, + `"headers": {`, + `"Authorization": "Bearer \${GITHUB_PERSONAL_ACCESS_TOKEN}"`, + `"X-MCP-Readonly": "true"`, + `"X-MCP-Toolsets": "repos"`, + `"tools": [`, + `"list_repositories"`, + `"get_repository"`, + `"env": {`, + `"GITHUB_PERSONAL_ACCESS_TOKEN": "\${GITHUB_PERSONAL_ACCESS_TOKEN}"`, + }, + notExpected: []string{}, + }, + { + name: "No toolsets configured", + options: GitHubMCPRemoteOptions{ + ReadOnly: false, + Toolsets: "", + AuthorizationValue: "Bearer ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}", + IncludeToolsField: false, + AllowedTools: nil, + IncludeEnvSection: false, + }, + expectedOutput: []string{ + `"type": "http"`, + `"url": "https://api.githubcopilot.com/mcp/"`, + `"headers": {`, + `"Authorization": "Bearer ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}"`, + }, + notExpected: []string{ + `"X-MCP-Toolsets"`, + `"X-MCP-Readonly"`, + `"tools"`, + `"env"`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var yaml strings.Builder + RenderGitHubMCPRemoteConfig(&yaml, tt.options) + output := yaml.String() + + // Check for expected strings + for _, expected := range tt.expectedOutput { + if !strings.Contains(output, expected) { + t.Errorf("Expected output to contain %q, but it didn't.\nGot:\n%s", expected, output) + } + } + + // Check that unexpected strings are not present + for _, notExpected := range tt.notExpected { + if strings.Contains(output, notExpected) { + t.Errorf("Expected output NOT to contain %q, but it did.\nGot:\n%s", notExpected, output) + } + } + }) + } +} + +func TestRenderGitHubMCPRemoteConfigHeaderOrder(t *testing.T) { + // Test that headers are sorted alphabetically for deterministic output + var yaml strings.Builder + RenderGitHubMCPRemoteConfig(&yaml, GitHubMCPRemoteOptions{ + ReadOnly: true, + Toolsets: "repos,issues", + AuthorizationValue: "Bearer token", + IncludeToolsField: false, + AllowedTools: nil, + IncludeEnvSection: false, + }) + output := yaml.String() + + // Authorization should come before X-MCP-Readonly, which should come before X-MCP-Toolsets + authIndex := strings.Index(output, `"Authorization"`) + readonlyIndex := strings.Index(output, `"X-MCP-Readonly"`) + toolsetsIndex := strings.Index(output, `"X-MCP-Toolsets"`) + + if authIndex == -1 || readonlyIndex == -1 || toolsetsIndex == -1 { + t.Fatal("Expected all three headers to be present") + } + + if authIndex >= readonlyIndex { + t.Errorf("Expected Authorization to come before X-MCP-Readonly, but got:\n%s", output) + } + + if readonlyIndex >= toolsetsIndex { + t.Errorf("Expected X-MCP-Readonly to come before X-MCP-Toolsets, but got:\n%s", output) + } +} + +func TestRenderGitHubMCPRemoteConfigToolsCommas(t *testing.T) { + // Test that tools array is properly formatted with commas + var yaml strings.Builder + RenderGitHubMCPRemoteConfig(&yaml, GitHubMCPRemoteOptions{ + ReadOnly: false, + Toolsets: "default", + AuthorizationValue: "Bearer token", + IncludeToolsField: true, + AllowedTools: []string{"tool1", "tool2", "tool3"}, + IncludeEnvSection: true, + }) + output := yaml.String() + + // First and second tools should have commas + if !strings.Contains(output, `"tool1",`) { + t.Errorf("Expected first tool to have comma, got:\n%s", output) + } + if !strings.Contains(output, `"tool2",`) { + t.Errorf("Expected second tool to have comma, got:\n%s", output) + } + + // Last tool should NOT have a comma + if !strings.Contains(output, `"tool3"`) || strings.Contains(output, `"tool3",`) { + t.Errorf("Expected last tool to NOT have comma, got:\n%s", output) + } +}