Skip to content

Conversation

@spongehah
Copy link
Contributor

No description provided.


// 连接到 MCP 服务器
session, err := c.mcpClient.Connect(ctx, transport, nil)
if err != nil {
Copy link

Choose a reason for hiding this comment

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

🔴 严重资源泄漏风险

当连接多个 MCP 服务器时,如果中途某个服务器连接失败,函数会直接返回错误,但不会清理之前已建立的会话。这会导致:

  • Docker 容器持续运行
  • goroutine 和 I/O 资源泄漏
  • 长期运行会累积僵尸进程

建议修复:

func (c *MCPDiagnosisClient) ConnectMCPServers(ctx context.Context, configs []MCPConfig) error {
    var connectedServers []string
    
    for _, config := range configs {
        // ... 连接逻辑 ...
        session, err := c.mcpClient.Connect(ctx, transport, nil)
        if err != nil {
            // 清理已建立的连接
            for _, name := range connectedServers {
                if s := c.sessions[name]; s != nil {
                    s.Close()
                    delete(c.sessions, name)
                }
            }
            return fmt.Errorf("连接 %s MCP 失败: %w", config.Name, err)
        }
        
        c.sessions[config.Name] = session
        connectedServers = append(connectedServers, config.Name)
    }
    return nil
}


if tool.InputSchema != nil {
// 将 MCP InputSchema 转换为 map
schemaBytes, _ := json.Marshal(tool.InputSchema)
Copy link

Choose a reason for hiding this comment

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

🔴 忽略 JSON 错误会导致工具定义不完整

这里忽略了序列化错误。如果 tool.InputSchema 无法序列化,schemaMap 会是空值,导致:

  • 工具参数定义丢失
  • AI 调用工具时可能传递错误参数
  • 触发多次重试,浪费 API 配额

建议处理错误:

schemaBytes, err := json.Marshal(tool.InputSchema)
if err != nil {
    log.Printf("警告: 序列化工具 %s 的 InputSchema 失败: %v", tool.Name, err)
    continue // 跳过该工具
}
var schemaMap map[string]any
if err := json.Unmarshal(schemaBytes, &schemaMap); err != nil {
    log.Printf("警告: 反序列化工具 %s 的 schema 失败: %v", tool.Name, err)
    continue
}

性能优化: 实际上可以直接用类型断言避免序列化开销:

if schemaMap, ok := tool.InputSchema.(map[string]any); ok {
    if props, ok := schemaMap["properties"].(map[string]any); ok {
        inputSchemaParam.Properties = props
    }
}

for k, v := range config.Env {
env = append(env, fmt.Sprintf("%s=%s", k, v))
}
cmd.Env = env
Copy link

Choose a reason for hiding this comment

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

⚠️ 环境变量未继承父进程

当前实现只设置了自定义环境变量,会丢失 PATHHOME 等系统环境变量,可能导致子命令执行失败。

建议改进:

if len(config.Env) > 0 {
    // 继承父进程环境变量
    cmd.Env = os.Environ()
    // 追加自定义环境变量
    for k, v := range config.Env {
        cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v))
    }
}

}

// 执行诊断
ctx := context.Background()
Copy link

Choose a reason for hiding this comment

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

🔴 缺少超时控制会导致资源无法释放

使用 context.Background() 使得调用方无法控制超时和取消。如果 AI 诊断过程卡住(如网络问题、MCP 服务器无响应),会导致:

  • Goroutine 永久阻塞
  • Docker 容器持续运行
  • 服务器资源无法回收

建议修复:

// 函数签名改为接受 ctx 参数
func SimpleDiagnosis(
    ctx context.Context,  // 新增
    prompt string,
    // ... 其他参数
) (string, error) {
    // 如果调用方未提供 ctx,可以添加默认超时
    if ctx == nil {
        var cancel context.CancelFunc
        ctx, cancel = context.WithTimeout(context.Background(), 5*time.Minute)
        defer cancel()
    }
    
    return client.Diagnose(ctx, config)
}


log.Printf("[%s MCP] 调用工具: %s", serverName, toolName)
argJSON, _ := json.MarshalIndent(arguments, "", " ")
log.Printf("[%s MCP] 参数: %s", serverName, string(argJSON))
Copy link

Choose a reason for hiding this comment

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

🔒 安全: 敏感信息可能通过日志泄露

工具调用的参数和结果直接输出到日志,可能包含:

  • Prometheus 查询结果(业务敏感数据)
  • GitHub API 响应(代码、Issue 内容等)
  • 其他 MCP 工具返回的敏感信息

建议使用结构化日志并脱敏:

import "github.com/zeromicro/go-zero/core/logx"

// 对敏感数据脱敏
logx.Infof("[%s MCP] 调用工具: %s", serverName, toolName)
// 只记录参数类型,不记录实际值
logx.Debugf("[%s MCP] 参数字段: %v", serverName, getFieldNames(arguments))

@xgopilot
Copy link

xgopilot bot commented Oct 31, 2025

代码审查总结

这次使用 go-sdk 重构 MCP client 的工作整体质量不错,架构设计合理,实现了 Go 原生调用并保留了 Docker 回退机制。不过发现了几个必须修复的关键问题

🔴 必须立即修复

  1. 资源泄漏: MCP 服务器连接失败时未清理已建立的会话
  2. 缺少超时: SimpleDiagnosis 使用 context.Background() 无法控制超时
  3. 错误处理: 多处 JSON 序列化错误被忽略,可能导致工具定义不完整
  4. 安全风险: 敏感凭证通过命令行参数传递,日志可能泄露敏感数据

💡 建议改进

  • 定义常量替换 magic number (4096, 20, 200 等)
  • MCPDiagnosisClient 添加并发安全保护
  • 补充 package 级文档和关键函数注释

已为主要问题添加了内联评论,请优先修复标记为🔴的关键问题。

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.

1 participant