Skip to content

Refactor: Extract duplicate GitHub MCP remote config rendering into shared helper#2249

Merged
pelikhan merged 3 commits intomainfrom
copilot/remove-duplicate-github-mcp-code
Oct 23, 2025
Merged

Refactor: Extract duplicate GitHub MCP remote config rendering into shared helper#2249
pelikhan merged 3 commits intomainfrom
copilot/remove-duplicate-github-mcp-code

Conversation

Copy link
Contributor

Copilot AI commented Oct 23, 2025

Problem

The GitHub MCP remote configuration rendering logic was duplicated between claude_engine.go and copilot_engine.go, creating a maintenance burden and increasing the risk of divergent behavior. Both engines built the same header map and YAML block with only minor differences (token format, tools field, and env section), requiring any future updates to be applied in two places.

Duplicate locations:

  • pkg/workflow/claude_engine.go:652-681 (30 lines)
  • pkg/workflow/copilot_engine.go:402-444 (43 lines)

Solution

Extracted the common GitHub MCP remote configuration rendering logic into a new shared helper function RenderGitHubMCPRemoteConfig() in engine_shared_helpers.go. This follows the existing pattern established by RenderGitHubMCPDockerConfig() for local mode rendering.

New Shared Helper

The RenderGitHubMCPRemoteConfig() function accepts a GitHubMCPRemoteOptions struct that allows engines to specify their variations:

type GitHubMCPRemoteOptions struct {
    ReadOnly           bool     // Enable read-only mode
    Toolsets           string   // GitHub toolsets to enable
    AuthorizationValue string   // Authorization header value
    IncludeToolsField  bool     // Whether to include tools field
    AllowedTools       []string // Allowed tools list
    IncludeEnvSection  bool     // Whether to include env section
}

Claude usage (no tools, no env):

RenderGitHubMCPRemoteConfig(yaml, GitHubMCPRemoteOptions{
    ReadOnly:           readOnly,
    Toolsets:           toolsets,
    AuthorizationValue: fmt.Sprintf("Bearer %s", effectiveToken),
    IncludeToolsField:  false,
    IncludeEnvSection:  false,
})

Copilot usage (with tools and env):

RenderGitHubMCPRemoteConfig(yaml, GitHubMCPRemoteOptions{
    ReadOnly:           readOnly,
    Toolsets:           toolsets,
    AuthorizationValue: "Bearer \\${GITHUB_PERSONAL_ACCESS_TOKEN}",
    IncludeToolsField:  true,
    AllowedTools:       allowedTools,
    IncludeEnvSection:  true,
})

Changes

  • pkg/workflow/engine_shared_helpers.go (+79 lines): Added shared helper function and options struct
  • pkg/workflow/claude_engine.go (-22 lines net): Refactored to use shared helper
  • pkg/workflow/copilot_engine.go (-44 lines net): Refactored to use shared helper
  • pkg/workflow/github_remote_config_test.go (+240 lines): Comprehensive test coverage

Total duplicate code eliminated: 73 lines

Testing

Added comprehensive unit tests covering:

  • Claude-style configuration (without tools or env sections)
  • Copilot-style configuration (with tools and env sections)
  • Read-only mode with X-MCP-Readonly header
  • Wildcard tools (["*"])
  • Specific allowed tools list
  • Header alphabetical ordering
  • Tools array comma formatting

All existing tests pass:

  • ✅ 240+ unit tests
  • ✅ GitHub remote mode integration tests (Claude, Copilot, Codex)
  • ✅ All 67 workflow files compile successfully
  • ✅ Generated YAML output unchanged

Benefits

  1. Single source of truth - Remote config logic now exists in one location
  2. Easier maintenance - Future updates only need to be applied once
  3. Reduced risk - Eliminates possibility of divergent behavior between engines
  4. Better testability - Shared helper can be tested independently
  5. Consistent behavior - Both engines use identical rendering logic
  6. Follows patterns - Matches existing RenderGitHubMCPDockerConfig approach

This refactoring maintains 100% backward compatibility with zero breaking changes while significantly improving code quality and maintainability.


Fixes #[issue number] (duplicate code detected in commit 82e9c3e)

Original prompt

This section details on the original issue you should resolve

<issue_title>[duplicate-code] 🔍 Duplicate Code Detected: GitHub MCP Remote Config Rendering</issue_title>
<issue_description># 🔍 Duplicate Code Detected: GitHub MCP Remote Config Rendering

Analysis of commit 82e9c3e

Assignee: @copilot

Summary

The GitHub MCP remote configuration block is duplicated between the Claude and Copilot engine renderers. Both functions build the same header map and YAML block with only minor token differences, making future fixes error-prone.

Duplication Details

Pattern: Repeated GitHub MCP remote header rendering logic

  • Severity: Medium
  • Occurrences: 2
  • Locations:
    • pkg/workflow/claude_engine.go:652
    • pkg/workflow/copilot_engine.go:402
  • Code Sample:
    // Remote mode - use hosted GitHub MCP server
    yaml.WriteString("                \"type\": \"http\",\n")
    yaml.WriteString("                \"url\": \"(redacted)",\n")
    yaml.WriteString("                \"headers\": {\n")
    
    headers := make(map[string]string)
    headers["Authorization"] = fmt.Sprintf("Bearer %s", effectiveToken)
    
    if readOnly {
        headers["X-MCP-Readonly"] = "true"
    }
    
    if toolsets != "" {
        headers["X-MCP-Toolsets"] = toolsets
    }
    
    writeHeadersToYAML(yaml, headers, "                  ")
    yaml.WriteString("                }\n")

Impact Analysis

  • Maintainability: Two nearly identical blocks must be kept in sync; fixes to headers or indentation must be applied twice.
  • Bug Risk: Minor differences (e.g., Authorization value) already exist, increasing the chance of inconsistent behavior across engines.
  • Code Bloat: Adds ~15 repeated lines that could live in a single helper.

Refactoring Recommendations

  1. Extract Common Helper

    • Lift the shared remote-rendering logic into a function (e.g. renderGitHubMCPRemoteConfig) that accepts token formatting callbacks.
    • Estimated effort: 1-2 hours.
    • Benefits: Single source of truth, easier updates.
  2. Parameterize Authorization Value

    • Pass the differing Authorization string as a parameter so both engines can reuse the helper.
    • Benefits: Eliminates divergence and clarifies intentional differences.

Implementation Checklist

  • Review duplication findings
  • Prioritize refactoring tasks
  • Create refactoring plan
  • Implement changes
  • Update tests
  • Verify no functionality broken

Analysis Metadata

  • Analyzed Files: 2
  • Detection Method: Serena semantic code analysis
  • Commit: 82e9c3e
  • Analysis Date: 2025-10-23

AI generated by Duplicate Code Detector</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #2246


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove duplicate GitHub MCP remote config rendering code Refactor: Extract duplicate GitHub MCP remote config rendering into shared helper Oct 23, 2025
Copilot AI requested a review from pelikhan October 23, 2025 22:50
@pelikhan pelikhan marked this pull request as ready for review October 23, 2025 22:52
@github-actions
Copy link
Contributor

Agentic Changeset Generator triggered by this pull request.

@pelikhan pelikhan merged commit d8ae1f0 into main Oct 23, 2025
4 checks passed
@pelikhan pelikhan deleted the copilot/remove-duplicate-github-mcp-code branch October 23, 2025 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[duplicate-code] 🔍 Duplicate Code Detected: GitHub MCP Remote Config Rendering

2 participants