Skip to content

Comments

Refactor MCP server configuration into centralized provider and remove deprecated RenderMCPConfig function#801

Closed
Copilot wants to merge 6 commits intomainfrom
copilot/fix-4be7eb21-3209-4329-a17e-92723787293c
Closed

Refactor MCP server configuration into centralized provider and remove deprecated RenderMCPConfig function#801
Copilot wants to merge 6 commits intomainfrom
copilot/fix-4be7eb21-3209-4329-a17e-92723787293c

Conversation

Copy link
Contributor

Copilot AI commented Sep 12, 2025

Refactored MCP server configuration to use a centralized provider pattern and removed deprecated functions based on feedback. This PR consolidates MCP configuration logic, eliminates code duplication, and removes backward compatibility for unused functions.

Summary

Successfully addressed all feedback comments with comprehensive refactoring:

  1. Restored user-agent generation: GitHub MCP configurations now properly compute user-agent from workflow names
  2. Fixed API design: Added NewMCPServerConfigurationsFromFrontmatter() for cleaner single-call usage
  3. Removed deprecated functions: Completely removed RenderMCPConfig function and all related helper functions since they were only used by tests
  4. Centralized configuration logic: Created ComputeMCPServerConfigurationsFromFrontmatter() that handles workflow name extraction internally, eliminating code duplication in mcp_inspect.go
  5. Updated all tests: Modified test functions to use the new RenderMCPConfigFromConfigurations() approach with proper configuration providers
  6. Clean codebase: Removed all unused helper functions and ensured linting passes

Changes Made

  • Centralized MCP configuration: New ComputeMCPServerConfigurationsFromFrontmatter() function handles provider creation and workflow name extraction internally
  • Removed deprecated API: Eliminated RenderMCPConfig function from engine interface and all implementations
  • Code cleanup: Removed unused helper functions like generatePlaywrightDockerArgs, getPlaywrightDockerImageVersion, etc.
  • Test updates: All MCP-related tests now use the new configuration provider approach
  • API simplification: Reduced function parameters and cleaner call patterns throughout the codebase

The centralized MCP configuration provider now properly generates user-agent fields for Codex engine while maintaining a much cleaner and more maintainable codebase architecture.


💡 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>
Copilot AI changed the title [WIP] @copilot - refactor the code that computes the MCP server configuration into a separate file. It should populate a list of configuration objects which contain the information about the server - update the agentic engine interface to use these computed... Refactor MCP server configuration into centralized provider Sep 12, 2025
Copilot AI requested a review from pelikhan September 12, 2025 20:35
Copilot AI and others added 2 commits September 12, 2025 20:51
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Sep 12, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.contoso.com
    • Triggering command: /tmp/go-build2429348446/b278/cli.test -test.testlogfile=/tmp/go-build2429348446/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)
    • Triggering command: /tmp/go-build1673032990/b278/cli.test -test.testlogfile=/tmp/go-build1673032990/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Refactor MCP server configuration into centralized provider Refactor MCP server configuration into centralized provider and fix task job generation Sep 12, 2025
Copilot AI requested a review from pelikhan September 12, 2025 20:55
@@ -48,6 +48,9 @@ type CodingAgentEngine 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot is this function still used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the RenderMCPConfig function is still used by tests and as part of the engine interface. However, the new compiler uses RenderMCPConfigFromConfigurations which leverages the centralized configuration provider. The old function remains for backward compatibility and testing. Commit: 0931e39

Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot no backward compatibility. If only tests use this function remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the RenderMCPConfig function completely as requested. The function was only used by tests, so I updated all tests to use the new RenderMCPConfigFromConfigurations approach with the centralized configuration provider. All tests pass. Commit: ff5125a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the RenderMCPConfig function completely as requested. The function was only used by tests, so I updated all tests to use the new RenderMCPConfigFromConfigurations approach with the centralized configuration provider. All tests pass. Commit: ff5125a

…ve API

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

Copilot AI commented Sep 12, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.contoso.com
    • Triggering command: /tmp/go-build1615415865/b278/cli.test -test.testlogfile=/tmp/go-build1615415865/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

configProvider := workflow.NewMCPServerConfigProvider()

// Create minimal WorkflowData for user-agent computation
minimalWorkflowData := &workflow.WorkflowData{
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot move this logic inside of ComputeMCPServerConfigurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the logic into ComputeMCPServerConfigurations by creating ComputeMCPServerConfigurationsFromFrontmatter() that handles workflow name extraction internally. This eliminates the repetitive MCPServerConfigProvider creation and minimalWorkflowData construction. Commit: ff5125a

configProvider := workflow.NewMCPServerConfigProvider()

// Create minimal WorkflowData for user-agent computation
minimalWorkflowData := &workflow.WorkflowData{
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot move this logic inside of ComputeMCPServerConfigurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the logic into ComputeMCPServerConfigurations by creating ComputeMCPServerConfigurationsFromFrontmatter() that handles workflow name extraction internally. This eliminates the repetitive MCPServerConfigProvider creation and minimalWorkflowData construction. Commit: ff5125a

configProvider := workflow.NewMCPServerConfigProvider()

// Create minimal WorkflowData for user-agent computation
minimalWorkflowData := &workflow.WorkflowData{
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot move this logic inside of ComputeMCPServerConfigurations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the logic into ComputeMCPServerConfigurations by creating ComputeMCPServerConfigurationsFromFrontmatter() that handles workflow name extraction internally. This eliminates the repetitive MCPServerConfigProvider creation and minimalWorkflowData construction. Commit: ff5125a

@@ -48,6 +48,9 @@ type CodingAgentEngine 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

@copilot no backward compatibility. If only tests use this function remove.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@pelikhan pelikhan closed this Sep 12, 2025
Copy link
Contributor Author

Copilot AI commented Sep 12, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • api.contoso.com
    • Triggering command: /tmp/go-build3969010265/b278/cli.test -test.testlogfile=/tmp/go-build3969010265/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title Refactor MCP server configuration into centralized provider and fix task job generation Refactor MCP server configuration into centralized provider and remove deprecated RenderMCPConfig function Sep 12, 2025
Copilot AI requested a review from pelikhan September 12, 2025 21:41
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.

2 participants