Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 15, 2025

Refactor CopilotEngine to use unified MCPConfigRenderer

Objective: Replace duplicated MCP rendering functions in copilot_engine.go with unified renderer calls.

Plan Checklist

  • Understand the codebase structure and existing unified renderer pattern
  • Verify current tests pass and build succeeds
  • Update RenderMCPConfig method in copilot_engine.go to create renderer with Copilot options
  • Replace renderGitHubCopilotMCPConfig with unified renderer call
  • Replace renderPlaywrightCopilotMCPConfig with unified renderer call
  • Replace renderSafeOutputsCopilotMCPConfig with unified renderer call
  • Replace renderAgenticWorkflowsCopilotMCPConfig with unified renderer call
  • Remove the 4 duplicated rendering functions
  • Run tests to verify behavior is preserved
  • Build and verify compilation succeeds
  • Fix Copilot remote mode authentication header syntax
  • Merge main branch and run linter

Implementation Summary

Following the Claude engine pattern (from claude_mcp.go):

  1. ✅ Created a createRenderer closure in RenderMCPConfig with Copilot-specific options (IncludeCopilotFields: true)
  2. ✅ Replaced function references in MCPToolRenderers with lambda functions that call the unified renderer
  3. ✅ Removed the 4 duplicated function implementations
  4. ✅ Updated tests to use unified renderer directly
  5. ✅ Fixed authentication header syntax for Copilot remote mode (MCP passthrough vs shell variable)
  6. ✅ Merged latest changes from main branch

Changes Made

Files Modified:

  • pkg/workflow/copilot_engine.go - Refactored to use unified renderer, removed ~60 lines of duplicate code
  • pkg/workflow/mcp_renderer.go - Added logic to use correct auth syntax based on engine (Copilot vs others)
  • pkg/workflow/copilot_engine_test.go - Updated tests to use unified renderer
  • pkg/workflow/copilot_github_mcp_test.go - Updated tests to use unified renderer
  • pkg/workflow/github_toolset_test.go - Updated tests to use unified renderer

Key Differences:

  • Copilot uses MCP passthrough syntax: "Bearer \${GITHUB_PERSONAL_ACCESS_TOKEN}"
  • Other engines use shell variable: "Bearer $GITHUB_MCP_SERVER_TOKEN"
  • The unified renderer now automatically selects the correct syntax based on IncludeCopilotFields option

All tests passing ✅
Build successful ✅
Linter passing ✅
Merged with main ✅

Original prompt

This section details on the original issue you should resolve

<issue_title>[task] Refactor CopilotEngine to use unified MCPConfigRenderer</issue_title>
<issue_description>## Objective
Update pkg/workflow/copilot_engine.go to use the unified MCPConfigRenderer instead of duplicated rendering functions.

Context

CopilotEngine currently has 4 duplicated MCP rendering functions with Copilot-specific fields (e.g., includeCopilotFields). These should be replaced with calls to the unified renderer. Related to #3942.

Approach

  1. Update copilot_engine.go to use NewMCPConfigRenderer() with Copilot-specific options
  2. Replace these functions with unified renderer calls:
    • renderGitHubCopilotMCPConfig() → Use renderer.RenderGitHubMCP()
    • renderPlaywrightCopilotMCPConfig() → Use renderer.RenderPlaywrightMCP()
    • renderSafeOutputsCopilotMCPConfig() → Use renderer.RenderSafeOutputsMCP()
    • renderAgenticWorkflowsCopilotMCPConfig() → Use renderer.RenderAgenticWorkflowsMCP()
  3. Configure renderer options with IncludeCopilotFields: true
  4. Update tests to verify Copilot-specific behavior is preserved

Files to Modify

  • pkg/workflow/copilot_engine.go - Update to use unified renderer
  • pkg/workflow/copilot_engine_test.go - Update tests if needed

Acceptance Criteria

  • All 4 Copilot MCP rendering functions replaced with unified renderer calls
  • CopilotEngine configured with IncludeCopilotFields: true option
  • All existing tests pass without modification to expected behavior
  • Code compiles without errors
  • No duplicate rendering logic remains in copilot_engine.go
  • Copilot-specific fields are correctly included in MCP configuration output
    Related to [refactor] 🔧 Semantic Function Clustering Analysis: Refactoring Opportunities #3942

AI generated by Plan Command for #3942</issue_description>

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


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan
Copy link
Contributor

@copilot

- Update unified renderer to use MCP passthrough syntax for Copilot
- Copilot now uses "Bearer \${GITHUB_PERSONAL_ACCESS_TOKEN}" instead of shell variable
- Fixes TestGitHubRemoteModeConfiguration test failures
- All tests now passing

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot finished work on behalf of pelikhan November 15, 2025 16:54
@pelikhan pelikhan marked this pull request as ready for review November 15, 2025 17:18
Copilot AI review requested due to automatic review settings November 15, 2025 17:18
Copilot finished reviewing on behalf of pelikhan November 15, 2025 17:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the CopilotEngine to use the unified MCPConfigRenderer, eliminating duplicate MCP rendering code. The key change involves replacing four engine-specific rendering functions with calls to a unified renderer configured with Copilot-specific options, and implementing engine-aware authentication header syntax for remote GitHub MCP configurations.

Key Changes:

  • Replaced duplicated Copilot MCP rendering functions with unified renderer calls
  • Added engine-aware authentication header logic in unified renderer
  • Updated tests to use unified renderer directly instead of removed engine methods

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/workflow/copilot_engine.go Refactored to use unified renderer with lambda functions, removed ~60 lines of duplicate code
pkg/workflow/mcp_renderer.go Added conditional authentication header syntax based on IncludeCopilotFields option
pkg/workflow/copilot_engine_test.go Updated tests to create and use unified renderer directly
pkg/workflow/copilot_github_mcp_test.go Updated tests to create and use unified renderer directly
pkg/workflow/github_toolset_test.go Updated tests to create and use unified renderer directly

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pelikhan
Copy link
Contributor

@copilot merge main and lint

Copilot AI and others added 2 commits November 15, 2025 17:54
- Merged latest changes from main branch
- Ran formatter to fix code formatting
- All tests passing ✅
- Linter passing ✅
- Build successful ✅

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 15, 2025

@copilot merge main and lint

Done! Merged main branch and ran linter in commit c637eef.

  • ✅ Merged latest changes from main
  • ✅ Code formatted with make fmt
  • ✅ Linter passing
  • ✅ All tests passing
  • ✅ Build successful

Copilot AI requested a review from pelikhan November 15, 2025 17:58
Copilot finished work on behalf of pelikhan November 15, 2025 17:58
@pelikhan
Copy link
Contributor

@copilot recompile and make sure generate lock file did not change

Copy link
Contributor Author

Copilot AI commented Nov 15, 2025

@copilot recompile and make sure generate lock file did not change

✅ Recompiled all workflows - no lock files changed.

  • Compiled 78 out of 78 workflow files successfully
  • Working tree clean - no changes to generated .lock.yml files
  • Refactoring maintains identical output behavior

Copilot finished work on behalf of pelikhan November 15, 2025 18:07
@pelikhan pelikhan merged commit 2a992ac into main Nov 15, 2025
51 of 57 checks passed
@pelikhan pelikhan deleted the copilot/refactor-copilot-engine-rendering branch November 15, 2025 18:10
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.

[task] Refactor CopilotEngine to use unified MCPConfigRenderer

2 participants