fix: skip unhealthy or disabled services form nacos and always marshal AllowTools field#3220
fix: skip unhealthy or disabled services form nacos and always marshal AllowTools field#3220Erica177 merged 5 commits intoalibaba:mainfrom
AllowTools field#3220Conversation
…l `AllowTools` field.
Aias00
requested review from
2456868764,
Erica177 and
johnlanni
as code owners
修复Nacos服务健康检查与工具字段序列化问题变更概述
变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
![lingma-agents[bot]](https://avatars.githubusercontent.com/in/1198481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 2 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 2 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
🔹 registry/mcp_model.go (1 💬)
🔹 registry/nacos/mcpserver/watcher.go (1 💬)
- 跳过不健康或禁用的服务实例以避免将它们注册到服务网格中。 (L820-L822)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. Nacos服务实例过滤逻辑可能影响服务发现的完整性
在generateServiceEntry函数中新增了对不健康或禁用服务实例的跳过逻辑。虽然这可以防止无效服务被注册到服务网格，但需确保该策略不会导致某些依赖这些状态信息进行决策的上层系统出现行为异常。建议明确文档说明此过滤机制，并评估是否需要提供配置开关以支持不同场景下的需求。
📌 关键代码
if !service.Healthy || !service.Enable {
continue
}可能导致部分监控、告警或自动化运维工具无法获取完整的服务实例列表，从而影响故障排查和自动恢复能力。
🔍2. `AllowTools`字段强制序列化可能引发兼容性问题
修改后的AllowTools字段去除了omitempty标签，这意味着即使为空也会出现在JSON输出中。这种变更可能会破坏现有客户端的反序列化逻辑，特别是那些未预期接收到空数组字段的旧版本客户端。应进行全面回归测试并考虑引入版本控制来管理API演化。
📌 关键代码
AllowTools []string `json:"allowTools"`与历史客户端的向后兼容性风险；潜在的数据传输体积增加。
🔍3. 工具配置处理中的空切片初始化缺乏上下文一致性检查
在processToolConfig方法中初始化了一个新的allowTools空切片，但在后续代码中没有看到对该变量的实际使用。如果这是为将来功能预留，则应当添加注释说明意图；如果是遗漏使用的错误，则应及时修正或移除冗余代码，以免造成资源浪费和维护困惑。
📌 关键代码
allowTools := []string{}代码可读性和维护性的降低；未来扩展时可能出现意料之外的行为。
审查详情
📒 文件清单 （2 个文件）
📝 变更: 2 个文件
📝 变更文件:
registry/mcp_model.goregistry/nacos/mcpserver/watcher.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3220 +/- ##
==========================================
+ Coverage 46.83% 46.90% +0.06%
==========================================
Files 87 87
Lines 12922 12922
==========================================
+ Hits 6052 6061 +9
+ Misses 6479 6471 -8
+ Partials 391 390 -1 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements two distinct fixes for the Nacos MCP server integration: filtering out unhealthy/disabled services from service discovery and ensuring the AllowTools field is always present in JSON marshaling output.
- Adds health and enabled status checking for Nacos service instances before including them in ServiceEntry endpoints
- Changes
AllowToolsfield to always marshal (removesomitemptytag) and initializes it as an empty slice instead of nil - Aligns
AllowToolsmarshaling behavior with other critical MCP API fields likeRequestTemplate,ResponseTemplate, andSecurity
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| registry/nacos/mcpserver/watcher.go | Adds health check filtering (lines 820-822) and initializes allowTools as empty slice (line 437) to ensure non-nil value |
| registry/mcp_model.go | Removes omitempty from AllowTools JSON tag to ensure field is always present in marshaled output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
please sign the license/cla |
i don |
|
Could you push another dummy commit and trigger the check again? |
okay, i will try |
… checks for Nacos services
…to fix/Issue_3192
hi，cla check is passed. |
…l `AllowTools` field (#3220) Co-authored-by: EricaLiu <30773688+Erica177@users.noreply.github.com>
…l `AllowTools` field (#3220) Co-authored-by: EricaLiu <30773688+Erica177@users.noreply.github.com>
4 participants
Ⅰ. Describe what this PR did
skip unhealthy or disabled services form nacos and always marshal
AllowToolsfieldⅡ. Does this pull request fix one issue?
fixes #3192
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
Ⅵ. AI Coding Tool Usage Checklist (if applicable)
Please check all applicable items:
For new standalone features (e.g., new wasm plugin or golang-filter plugin):
design/directory in the plugin folderdesign/directoryFor regular updates/changes (not new plugins):
AI Coding Prompts (for regular updates)
AI Coding Summary