Skip to content

Feature/tool node support mcp#84

Closed
xixingwang wants to merge 6 commits intoLianjiaTech:developfrom
xixingwang:feature/tool_node_support_mcp
Closed

Feature/tool node support mcp#84
xixingwang wants to merge 6 commits intoLianjiaTech:developfrom
xixingwang:feature/tool_node_support_mcp

Conversation

@xixingwang
Copy link
Copy Markdown

Pull Request

Description

添加了MCP(Model Context Protocol)工具类型支持,增强了工作流工具集成的能力。

Fixes # (issue)

Type of Change

Please delete options that are not relevant.

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🧹 Code refactoring (no functional changes)
  • ⚡ Performance improvement
  • 🔧 Configuration change

Component

Please check the components affected by this PR:

  • 🔧 API (Backend)
  • 🎨 Web (Frontend)
  • 🐳 Docker/Infrastructure
  • 📖 Documentation
  • 🧪 Tests
  • 📦 Dependencies

Testing

Please describe the tests that you ran to verify your changes:

  • Unit tests pass (npm test / mvn test)
  • Integration tests pass
  • Manual testing completed
  • Docker build successful

Test Configuration:

  • OS: [e.g. Ubuntu 20.04]
  • Node.js version: [e.g. 20.x]
  • Java version: [e.g. 8]

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Screenshots (if applicable)

Please add screenshots to help explain your changes.

Additional Notes

Add any additional notes, concerns, or questions here.

@Jiakaic
Copy link
Copy Markdown
Member

Jiakaic commented Nov 11, 2025

@claude 检查提交的代码,使用中文回复

@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 11, 2025

Claude finished @Jiakaic's task —— View job


MCP工具支持代码审查

我已经完成了对PR中MCP工具支持功能的代码审查。以下是详细的审查反馈:

任务清单:

  • 分析核心MCP工具实现 (McpTool.java)
  • 审查后端服务层变更
  • 检查控制器层变更
  • 审查前端TypeScript变更
  • 检查Docker配置变更
  • 评估整体架构和集成
  • 提供最终代码审查反馈

代码审查结果

✅ 优点

  1. 架构设计合理

    • MCP工具实现遵循了现有的ITool接口,保持了架构一致性
    • 前后端分离设计良好,TypeScript类型定义完整
  2. 资源管理

    • McpTool实现了AutoCloseable接口,正确处理资源释放
    • 使用AtomicReferencecompareAndSet实现线程安全的单例模式
  3. 错误处理

    • 自定义了McpToolException异常类型,错误处理清晰
    • ToolNode中有完整的异常捕获和错误状态返回

⚠️ 需要关注的问题

  1. 安全性问题 (McpTool.java:32)

    return provider.callToolAsText(toolName, params).getContent();
    • 直接调用外部MCP服务,需要确保对输入参数进行适当的验证和清理
    • 建议添加参数白名单验证机制
  2. 资源泄漏风险 (McpTool.java:43-47)

    if (!providerRef.compareAndSet(null, provider)) {
        try {
            provider.close();
        } catch (Exception ignored) {
            // 空的catch块
        }
    }
    • 忽略了关闭异常,可能导致资源泄漏问题未被发现
    • 建议至少记录日志
  3. API端点验证 (BellaToolService.java:70,81)

    String toolListUrl = Configs.BELLA_MCP_API_BASE + "/external/api/workflow/server/list";
    return get(Configs.BELLA_MCP_API_BASE + "/external/api/workflow/server/link", param, ...);
    • 硬编码的API路径,建议提取为常量
    • 缺少对BELLA_MCP_API_BASE配置的null检查
  4. 类型转换安全 (ToolNode.java:47-50)

    if(Objects.nonNull(data.getResult()) && "json".equals(data.getResult().getType())) {
        outputs.put("result", JsonUtils.fromJson(response, Map.class));
    }
    • JSON解析可能失败,建议添加异常处理

🔧 改进建议

  1. 配置验证

    • Configs.java中添加MCP配置的验证逻辑
    • 确保BELLA_MCP_API_BASE等配置项的有效性
  2. 日志记录

    • 在MCP工具调用过程中添加适当的日志记录
    • 记录工具调用的开始、结束和异常信息
  3. 超时处理

    • MCP工具调用可能耗时较长,建议添加超时机制
    • McpClientProvider配置中添加超时设置
  4. 前端用户体验

    • 考虑在前端添加MCP工具加载状态指示
    • 为MCP工具调用失败提供更友好的错误提示

📋 测试建议

  1. 单元测试

    • McpTool类添加单元测试,包括正常流程和异常情况
    • 测试资源清理逻辑的正确性
  2. 集成测试

    • 测试MCP工具在完整工作流中的运行
    • 验证前后端集成的正确性
  3. 性能测试

    • 测试MCP工具调用的性能表现
    • 验证并发调用的稳定性

总体评价

这个PR实现了MCP工具支持的核心功能,代码结构合理,遵循了项目的编码规范。主要的改进空间在于安全性加固、错误处理完善和用户体验优化。在解决上述安全性问题后,这个功能可以投入使用。


@Jiakaic
Copy link
Copy Markdown
Member

Jiakaic commented Nov 11, 2025

辛苦提供一个简单的case的前端展示,运行交互的截图吧

@Jiakaic
Copy link
Copy Markdown
Member

Jiakaic commented Nov 11, 2025

以及,MCP Server支持Streamable HTTP还是SSE,目前是如何表示,关于协议的设计是否可以先讲讲?

@Jiakaic
Copy link
Copy Markdown
Member

Jiakaic commented Nov 11, 2025

关于鉴权是如何设计的呢?

@xixingwang
Copy link
Copy Markdown
Author

辛苦提供一个简单的case的前端展示,运行交互的截图吧

1、工具节点筛选MCP工具
image
2、single run
image
3、workflow run
image

@xixingwang
Copy link
Copy Markdown
Author

以及,MCP Server支持Streamable HTTP还是SSE,目前是如何表示,关于协议的设计是否可以先讲讲?

目前只支持Streamable HTTP

@Jiakaic
Copy link
Copy Markdown
Member

Jiakaic commented Nov 11, 2025

以及,MCP Server支持Streamable HTTP还是SSE,目前是如何表示,关于协议的设计是否可以先讲讲?

目前只支持Streamable HTTP

协议设计或者execute的coding上需要考虑,不局限在目前你们提供的mcp集市的实现,从一个mcp集市应该具备的能力上设计

@xixingwang
Copy link
Copy Markdown
Author

关于鉴权是如何设计的呢?

目前Client侧不提供鉴权验证,Client端是“无状态的透传者”,Server或者网关是“认证逻辑的绝对中心”

工作流编排时,MCP工具列表会内置绑定Server相关的信息,包括鉴权信息,发布后统一维护在DSL中,后面Client直接Header透传

@Jiakaic
Copy link
Copy Markdown
Member

Jiakaic commented Nov 11, 2025

关于鉴权是如何设计的呢?

目前Client侧不提供鉴权验证,Client端是“无状态的透传者”,Server或者网关是“认证逻辑的绝对中心”

工作流编排时,MCP工具列表会内置绑定Server相关的信息,包括鉴权信息,发布后统一维护在DSL中,后面Client直接Header透传

关于鉴权是如何设计的呢?

目前Client侧不提供鉴权验证,Client端是“无状态的透传者”,Server或者网关是“认证逻辑的绝对中心”

工作流编排时,MCP工具列表会内置绑定Server相关的信息,包括鉴权信息,发布后统一维护在DSL中,后面Client直接Header透传

  • workflow作为mcp client,理论上需要实现MCP继承的OAuth2.1,比如refresh_token这个可选特性?
  • 关于鉴权,对于token直接存储在dsl中不太可取,目前是明文存储;对于MCP/工具,授权逻辑通常在工作台中实现,而之前本服务并未提供工具工作台;建议低成本可以先参考之前的工具设计,token从MCP集市中获取(也并不安全)。

@xixingwang
Copy link
Copy Markdown
Author

关于鉴权是如何设计的呢?

目前Client侧不提供鉴权验证,Client端是“无状态的透传者”,Server或者网关是“认证逻辑的绝对中心”
工作流编排时,MCP工具列表会内置绑定Server相关的信息,包括鉴权信息,发布后统一维护在DSL中,后面Client直接Header透传

关于鉴权是如何设计的呢?

目前Client侧不提供鉴权验证,Client端是“无状态的透传者”,Server或者网关是“认证逻辑的绝对中心”
工作流编排时,MCP工具列表会内置绑定Server相关的信息,包括鉴权信息,发布后统一维护在DSL中,后面Client直接Header透传

  • workflow作为mcp client,理论上需要实现MCP继承的OAuth2.1,比如refresh_token这个可选特性?
  • 关于鉴权,对于token直接存储在dsl中不太可取,目前是明文存储;对于MCP/工具,授权逻辑通常在工作台中实现,而之前本服务并未提供工具工作台;建议低成本可以先参考之前的工具设计,token从MCP集市中获取(也并不安全)。

是的,合理应该是提供工具注册、管理及授权的完整解决方案,静态令牌需由应用方自己维护且平台加密存储,或者提供Access Token 和 Refresh Token动态令牌能力

@xixingwang
Copy link
Copy Markdown
Author

关于鉴权是如何设计的呢?

目前Client侧不提供鉴权验证,Client端是“无状态的透传者”,Server或者网关是“认证逻辑的绝对中心”
工作流编排时,MCP工具列表会内置绑定Server相关的信息,包括鉴权信息,发布后统一维护在DSL中,后面Client直接Header透传

关于鉴权是如何设计的呢?

目前Client侧不提供鉴权验证,Client端是“无状态的透传者”,Server或者网关是“认证逻辑的绝对中心”
工作流编排时,MCP工具列表会内置绑定Server相关的信息,包括鉴权信息,发布后统一维护在DSL中,后面Client直接Header透传

  • workflow作为mcp client,理论上需要实现MCP继承的OAuth2.1,比如refresh_token这个可选特性?
  • 关于鉴权,对于token直接存储在dsl中不太可取,目前是明文存储;对于MCP/工具,授权逻辑通常在工作台中实现,而之前本服务并未提供工具工作台;建议低成本可以先参考之前的工具设计,token从MCP集市中获取(也并不安全)。

这个也看工作台长期迭代计划,后期会不会增加工具管理能力,如果有可以长期规划和设计一下

@xixingwang xixingwang closed this Nov 24, 2025
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