-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(mcp): cannot rename MCP Server #4766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
+47
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When editing the MCP server configuration, you can now change the server name. The frontend will save the original name in edit mode, and the backend will recognize the rename operation through the oldName field.
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - 我发现了 1 个问题,并且给出了一些整体性的反馈:
- 在
update_mcp_server中,name/old_name/is_rename的分支逻辑分散在多个位置;可以考虑提取一个小的辅助函数,或至少把“重命名 vs 非重命名”的路径(配置变更 + 客户端禁用/启用)集中处理,这样在未来再次更新这块逻辑时就不容易出现分支不一致或遗漏。 - 在前端中,
originalServerName只在从editMcpServer打开编辑弹窗时被设置;如果这个弹窗将来会在其他流程中复用或再次打开,那么更安全的做法可能是:始终基于当前加载的 server 初始化它(以及/或者从路由/状态中派生),以避免发送过期或为空的oldName。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- In `update_mcp_server`, the `name`/`old_name`/`is_rename` branching is spread across several places; consider extracting a small helper or at least centralizing the rename vs non-rename path (config mutation + client disable/enable) to avoid future divergence or missed branches when this logic is updated again.
- On the frontend, `originalServerName` is only set when opening the edit dialog from `editMcpServer`; if the dialog can be reused or reopened in other flows later, it may be safer to always initialize it based on the loaded server (and/or derive it from route/state) to avoid stale or empty `oldName` being sent.
## Individual Comments
### Comment 1
<location> `astrbot/dashboard/routes/tools.py:143` </location>
<code_context>
+ if old_name not in config["mcpServers"]:
+ return Response().error(f"服务器 {old_name} 不存在").__dict__
+
+ is_rename = name != old_name
# 获取活动状态
</code_context>
<issue_to_address>
**issue:** 请考虑处理“新的服务器名称已存在”的情况,以避免被静默覆盖。
当 `is_rename` 为 `True` 时,下面这段代码:
```python
if is_rename:
config["mcpServers"].pop(old_name)
config["mcpServers"][name] = server_config
```
会覆盖已有的 `config['mcpServers'][name]` 条目,从而导致之前的配置丢失。可以考虑在执行前检查 `if name in config['mcpServers'] and name != old_name`,如果条件满足则返回一个错误(例如:`"服务器 X 已存在"`),而不是直接覆盖,以保持重命名行为可预测并且避免名称冲突。
</issue_to_address>帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈来改进后续的 Review。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
update_mcp_server, thename/old_name/is_renamebranching is spread across several places; consider extracting a small helper or at least centralizing the rename vs non-rename path (config mutation + client disable/enable) to avoid future divergence or missed branches when this logic is updated again. - On the frontend,
originalServerNameis only set when opening the edit dialog fromeditMcpServer; if the dialog can be reused or reopened in other flows later, it may be safer to always initialize it based on the loaded server (and/or derive it from route/state) to avoid stale or emptyoldNamebeing sent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `update_mcp_server`, the `name`/`old_name`/`is_rename` branching is spread across several places; consider extracting a small helper or at least centralizing the rename vs non-rename path (config mutation + client disable/enable) to avoid future divergence or missed branches when this logic is updated again.
- On the frontend, `originalServerName` is only set when opening the edit dialog from `editMcpServer`; if the dialog can be reused or reopened in other flows later, it may be safer to always initialize it based on the loaded server (and/or derive it from route/state) to avoid stale or empty `oldName` being sent.
## Individual Comments
### Comment 1
<location> `astrbot/dashboard/routes/tools.py:143` </location>
<code_context>
+ if old_name not in config["mcpServers"]:
+ return Response().error(f"服务器 {old_name} 不存在").__dict__
+
+ is_rename = name != old_name
# 获取活动状态
</code_context>
<issue_to_address>
**issue:** Consider handling the case where the new server name already exists to avoid silent overwrites.
When `is_rename` is `True`, this block:
```python
if is_rename:
config["mcpServers"].pop(old_name)
config["mcpServers"][name] = server_config
```
will overwrite an existing `config['mcpServers'][name]` entry, causing loss of the previous configuration. Consider checking `if name in config['mcpServers'] and name != old_name` and returning an error (e.g., `"服务器 X 已存在"`) instead of overwriting to keep rename behavior predictable and collision-free.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…for name conflicts When renaming an MCP server, add a check to see if the target name already exists. If the name exists and it is a rename operation, return an error message to avoid overwriting the configuration.
Contributor
Author
|
为了快速修复问题,本次修改以最小变更为原则,便未进行架构调整 |
Soulter
approved these changes
Feb 1, 2026
Member
Soulter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area:core
The bug / feature is about astrbot's core, backend
area:webui
The bug / feature is about webui(dashboard) of astrbot.
lgtm
This PR has been approved by a maintainer
size:M
This PR changes 30-99 lines, ignoring generated files.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
修复更新MCP服务名称时更新失败的问题
fix #4756
Modifications / 改动点
tools.py: 支持接收oldName参数以重命名MCP服务
McpServersSection.vue: 添加oldName参数,在更新MCP服务名称时保存原名称
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
错误修复:
Original summary in English
Summary by Sourcery
Bug Fixes: