Conversation
Preview sandbox Image: |
Preview mcp_server Image: |
Docs Preview:🚀 FastGPT Document Preview Ready! |
c121914yu
left a comment
There was a problem hiding this comment.
📊 代码审查总结
这个 PR 在架构上有很好的改进,统一了 tool ID 的命名规范,但存在一些需要注意的问题。
主要问题
- 🔴 缺少错误处理,可能导致运行时错误
- 🔴 权限验证逻辑变更需要仔细验证
- 🟡 测试覆盖不完整
- 🟡 命名规范不一致
优点
- ✅ 统一了 tool ID 命名规范
- ✅ 删除了冗余代码
- ✅ 保持了向后兼容性
详细的审查意见请查看下方的行级评论。
There was a problem hiding this comment.
Pull request overview
This PR updates the “tool id” contract across workflow/tool execution paths, aligning runtime node naming and tool-id parsing for MCP/HTTP tools, and adjusting related API/controllers, tests, i18n, and docs.
Changes:
- Refactors tool-id parsing (
splitCombineToolId) and updates downstream usage in workflow dispatch + system-tool lookup paths. - Updates MCP/HTTP runtime node builders to prefix runtime node names with
toolsetName, and removes legacytoolIdfrom MCP toolset config. - Renames/adjusts permission helper (
authWorkflowToolByTmbId), updates i18n key for “tool deleted”, and adds 4.14.9 upgrade doc entry.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/cases/global/core/app/tool/utils.test.ts | Updates assertions around tool-id parsing (but still contains outdated expectations). |
| test/cases/global/core/app/tool/mcpTool/utils.test.ts | Updates tests for MCP toolset/runtime node contract changes. |
| test/cases/global/core/app/tool/httpTool/utils.test.ts | Updates tests for HTTP runtime node naming contract changes. |
| projects/app/src/pages/api/core/app/tool/getVersionList.ts | Adjusts system-tool version-list lookup to use correct id. |
| projects/app/src/pages/api/core/app/mcpTools/update.ts | Removes legacy toolId field when building MCP toolset node. |
| projects/app/src/pages/api/core/app/mcpTools/create.ts | Removes legacy toolId field when building MCP toolset node. |
| projects/app/package.json | Bumps app version. |
| packages/web/i18n/zh-Hant/common.json | Renames i18n key for “tool deleted”. |
| packages/web/i18n/zh-CN/common.json | Renames i18n key for “tool deleted”. |
| packages/web/i18n/en/common.json | Renames i18n key for “tool deleted”. |
| packages/service/support/permission/app/auth.ts | Renames helper to authWorkflowToolByTmbId and simplifies auth path. |
| packages/service/core/workflow/dispatch/utils.ts | Passes toolsetName into MCP/HTTP runtime node creation. |
| packages/service/core/workflow/dispatch/plugin/run.ts | Refactors workflow-tool execution path; adds version lookup + name i18n parsing. |
| packages/service/core/workflow/dispatch/child/runTool.ts | Introduces parseToolId helper for MCP/HTTP tool ids. |
| packages/service/core/workflow/dispatch/ai/agent/utils.ts | Renames runtime tool builder function used by agent. |
| packages/service/core/workflow/dispatch/ai/agent/sub/tool/utils.ts | Updates MCP/HTTP runtime node creation to pass toolsetName. |
| packages/service/core/workflow/dispatch/ai/agent/sub/tool/index.ts | Reuses parseToolId for MCP/HTTP tool dispatch. |
| packages/service/core/plugin/tool/systemToolSchema.ts | Adds clarifying comment for pluginId. |
| packages/service/core/app/utils.ts | Minor comment cleanup. |
| packages/service/core/app/tool/runtime/utils.ts | Refines which tool sources incur direct plugin cost. |
| packages/service/core/app/tool/controller.ts | Renames params for system-tool lookup and updates preview-node formatting logic. |
| packages/global/core/workflow/type/node.ts | Updates tool config schema (removes MCP toolset toolId, adds comments). |
| packages/global/core/app/tool/utils.ts | Refactors tool-id parsing contract + return shape. |
| packages/global/core/app/tool/mcpTool/utils.ts | Removes MCP toolset toolId and requires toolsetName for tool runtime node naming. |
| packages/global/core/app/tool/httpTool/utils.ts | Requires toolsetName for HTTP tool runtime node naming. |
| packages/global/common/error/code/plugin.ts | Updates “tool deleted” i18n key. |
| document/data/doc-last-modified.json | Updates doc timestamps. |
| document/content/docs/toc.mdx | Adds 4.14.9 entry to upgrading TOC. |
| document/content/docs/self-host/upgrading/4-14/4149.mdx | Adds 4.14.9 (in-progress) upgrade notes. |
Comments suppressed due to low confidence (1)
packages/service/core/workflow/dispatch/plugin/run.ts:72
- In the system-tool adaptation branch,
toolConfig.systemTool.toolIdis set toformatPluginIdfromsplitCombineToolId(pluginId). IfsplitCombineToolIdreturns the raw tool suffix (e.g.websearch) instead of the full id (systemTool-websearch),getSystemToolById/MongoSystemTool.findOne({ pluginId })will fail to resolve the tool. Safer options: pass the originalpluginIdthrough here, or ensuresplitCombineToolIdpreserves the fullsystemTool-*id for system tools.
// Adapt <= 4.10 system tool
const { source, pluginId: formatPluginId } = splitCombineToolId(pluginId);
if (source === AppToolSourceEnum.systemTool) {
return await dispatchRunTool({
...props,
node: {
...props.node,
toolConfig: {
systemTool: {
toolId: formatPluginId
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
|
||
| expect(result.source).toBe(AppToolSourceEnum.http); | ||
| expect(result.pluginId).toBe('507f1f77bcf86cd799439011'); | ||
| expect(result.parentId).toBe('507f1f77bcf86cd799439011'); | ||
| expect(result.authAppId).toBe('507f1f77bcf86cd799439011'); | ||
| }); |
There was a problem hiding this comment.
This test file still asserts the old splitCombineToolId contract in multiple places (e.g. commercial/systemTool/community pluginId expectations and the thrown error message text). With the updated implementation (now throwing 'toolId not found' and changing what pluginId contains for some prefixed ids), these assertions will fail unless the tests are updated to the new expected outputs (or the implementation is adjusted to preserve the old outputs).
Preview fastgpt Image: |
✅ 测试更新已为 测试文件
测试覆盖
测试结果性能测试结果
测试覆盖了新旧格式的兼容性、边界情况、真实场景和性能要求。 |
No description provided.