Skip to content

Conversation

Similarityoung
Copy link
Contributor

What this PR does:

This pull request introduces a new provider-agnostic MCP registry controller framework and a Nacos-specific implementation, along with several utility and conversion functions for handling registry configuration. It also updates the go.mod dependencies, adding new packages for Nacos v2 and Alibaba Cloud, while cleaning up unused or outdated dependencies. The changes are grouped into three main themes: registry controller framework, Nacos registry integration, and dependency updates.

Registry controller framework:

  • Added a provider-agnostic registry controller interface (Controller), a builder function type (BuildFunc), and a factory for provider registration and controller instantiation in pkg/adapter/mcpserver/registry/controller.go and pkg/adapter/mcpserver/registry/factory.go. [1] [2]

Nacos registry integration:

  • Implemented Nacos registry controller, including provider registration, controller lifecycle management, service discovery, and change notification in pkg/adapter/mcpserver/registry/nacos/adapter.go and pkg/adapter/mcpserver/registry/nacos/controller.go. [1] [2]
  • Added conversion logic for Nacos tool specifications to Pixiu tool configuration, including template parsing and argument extraction, in pkg/adapter/mcpserver/registry/nacos/converter.go.
  • Defined Nacos configuration data structures for tools, templates, and metadata in pkg/adapter/mcpserver/registry/nacos/types.go.
  • Added a utility function to parse host and port from URLs or address strings in pkg/adapter/mcpserver/common/url.go.

Copy link
Contributor

@Alanxtl Alanxtl left a comment

Choose a reason for hiding this comment

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

need to write docs about how to use this adapter, u can refer to #751

Comment on lines +709 to +710
dataId := ph[0]
group := ph[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

does access to ph have out of range problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

placeholders = append(placeholders, [2]string{dataId, group})

Because the parameter passed to this function is an array of two strings, it should not exceed the range

Copy link
Contributor

Choose a reason for hiding this comment

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

are there any mechanism from upper content to guarantee ph must have two elements? If not here should check whether ph have two elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// parseTemplatePlaceholders parses template placeholders and returns replaced content and required (dataId,group) list
func parseTemplatePlaceholders(content string) (string, [][2]string) {
	all := templateRegex.FindAllStringSubmatch(content, -1)
	if len(all) == 0 {
		return content, nil
	}
	placeholders := make([][2]string, 0, len(all))
	newContent := content
	seen := make(map[string]bool)
	for _, m := range all {
		if len(m) < 2 {
			continue
		}
		p := m[1] // dataId/group
		parts := strings.Split(p, "/")
		if len(parts) != 2 {
			continue
		}
		dataId := strings.TrimSpace(parts[0])
		group := strings.TrimSpace(parts[1])
		key := group + "::" + dataId
		if seen[key] {
			// replace duplicates as well
			newContent = strings.ReplaceAll(newContent, "${nacos."+p+"}", ".config.credentials."+group+"_"+dataId)
			continue
		}
		seen[key] = true
		placeholders = append(placeholders, [2]string{dataId, group})
		newContent = strings.ReplaceAll(newContent, "${nacos."+p+"}", ".config.credentials."+group+"_"+dataId)
	}
	return newContent, placeholders
}

In this function, len(parts) != 2 has been excluded wrong content.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool

@Similarityoung
Copy link
Contributor Author

need to write docs about how to use this adapter, u can refer to #751

👌, I will do it later.

@Similarityoung
Copy link
Contributor Author

sample is here apache/dubbo-go-pixiu-samples#97

@mark4z
Copy link
Member

mark4z commented Sep 16, 2025

This pull request introduces a new provider-agnostic registry controller framework for MCP servers, with an initial implementation for Nacos. It refactors the registry integration to support pluggable providers, adds utility functions for parsing registry addresses, and updates dependencies in go.mod to support new features and compatibility. The main changes are grouped below:

MCP Registry Controller Framework

  • Added a generic Controller interface and provider registration mechanism in pkg/adapter/mcpserver/registry/controller.go and pkg/adapter/mcpserver/registry/factory.go, allowing pluggable registry backends for MCP servers. [1] [2]

Nacos Registry Provider Implementation

  • Introduced a Nacos-specific controller in pkg/adapter/mcpserver/registry/nacos/controller.go and a provider registration/builder in pkg/adapter/mcpserver/registry/nacos/adapter.go, enabling dynamic discovery and watching of MCP servers via Nacos. [1] [2]

Utility Functions

  • Added ParseHostPortFromURL in pkg/adapter/mcpserver/common/url.go to robustly extract host and port from registry addresses, supporting multiple formats.

Dependency Updates

  • Updated go.mod to add new dependencies for Alibaba Cloud SDKs, Nacos SDK v2, and other libraries; also upgraded existing dependencies for compatibility and feature support. [1] [2] [3] [4] [5] [6] [7]

@mark4z mark4z requested a review from Copilot September 16, 2025 15:37
Copy link
Contributor

@Copilot 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 introduces a new provider-agnostic MCP (Model Context Protocol) registry controller framework with Nacos-specific implementation for dynamic configuration discovery and management. It also restructures the MCP server configuration data model from a local package to a shared model package to enable reuse across components.

  • Registry controller framework with provider-agnostic interfaces and factory pattern
  • Nacos registry integration for MCP server discovery and dynamic configuration updates
  • Configuration data model moved from pkg/filter/mcp/mcpserver/config.go to pkg/model/mcpserver.go

Reviewed Changes

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

Show a summary per file
File Description
pkg/model/mcpserver.go Moved and consolidated MCP server configuration data structures from filter package
pkg/filter/mcp/mcpserver/*.go Updated to use shared model package and added dynamic configuration capabilities
pkg/adapter/mcpserver/*.go New adapter for MCP registry integration with provider-agnostic controller pattern
pkg/adapter/mcpserver/registry/*.go Registry controller framework with Nacos implementation
pkg/adapter/mcpserver/common/url.go URL parsing utility for extracting host and port from various URL formats
go.mod Updated dependencies for Nacos v2 and Alibaba Cloud SDK
Comments suppressed due to low confidence (2)

pkg/filter/mcp/mcpserver/handlers.go:1

  • The code on line 456 executes unconditionally after the switch statement, causing all arguments to be added to bodyParams regardless of their intended location. This overwrites the proper handling done in the switch statement.
/*

pkg/filter/mcp/mcpserver/handlers.go:443

  • Potential nil pointer dereference if argConfig is nil when the argument name is not found in toolConfig.Args. The code should check if argConfig is nil before accessing argConfig.In.
		switch argConfig.In {

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2025

Codecov Report

❌ Patch coverage is 0% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.21%. Comparing base (26606a8) to head (08482a4).
⚠️ Report is 74 commits behind head on develop.

Files with missing lines Patch % Lines
pkg/filter/mcp/mcpserver/registry.go 0.00% 24 Missing ⚠️
pkg/filter/mcp/mcpserver/filter.go 0.00% 4 Missing ⚠️
pkg/filter/mcp/mcpserver/handlers.go 0.00% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (26606a8) and HEAD (08482a4). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (26606a8) HEAD (08482a4)
1 0
Additional details and impacted files
@@             Coverage Diff              @@
##           develop     #757       +/-   ##
============================================
- Coverage    54.67%   16.21%   -38.46%     
============================================
  Files          671      224      -447     
  Lines        78658    16723    -61935     
============================================
- Hits         43006     2712    -40294     
+ Misses       31981    13750    -18231     
+ Partials      3671      261     -3410     
Flag Coverage Δ
unittests 16.21% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Alanxtl
Copy link
Contributor

Alanxtl commented Oct 14, 2025

pls fix the CI

Copy link

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.

4 participants