feat: 支持配置工具调用超时时间并适配 ModelScope 的 MCP Server 配置 #3039
Merged
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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
嗨，你好 - 我已经审阅了你的修改，它们看起来很棒！
AI 代理的提示
请处理此代码审查中的评论：
## 个人评论
### 评论 1
<location> `astrbot/core/pipeline/process_stage/method/llm_request.py:211` </location>
<code_context>
+ # 如果为空,则说明没有发送消息给用户,并且返回值为空,将返回一个特殊的 TextContent,其内容如"工具没有返回内容"
+ yield None
+ except asyncio.TimeoutError:
+ raise Exception(
+ f"tool {tool.name} execution timeout after {run_context.context.tool_call_timeout} seconds."
+ )
</code_context>
<issue_to_address>
**suggestion:** 在超时时抛出通用异常可能会模糊错误来源。
考虑抛出特定的异常，例如 `ToolTimeoutError`，而不是 `Exception`，以便更容易识别和处理超时错误。
建议的实现：
```python
except asyncio.TimeoutError:
raise ToolTimeoutError(
f"tool {tool.name} execution timeout after {run_context.context.tool_call_timeout} seconds."
)
```
```python
# NOTE: Tool 在这里直接请求发送消息给用户
# TODO: 是否需要判断 event.get_result() 是否为空?
# 如果为空,则说明没有发送消息给用户,并且返回值为空,将返回一个特殊的 TextContent,其内容如"工具没有返回内容"
yield None
class ToolTimeoutError(Exception):
"""Raised when a tool execution times out."""
pass
```
如果 `ToolTimeoutError` 已在你的代码库中的其他位置定义，则应在此文件顶部导入它，而不是在此处定义。
</issue_to_address>
### 评论 2
<location> `astrbot/dashboard/routes/tools.py:276-285` </location>
<code_context>
server_data = await request.json
config = server_data.get("mcp_server_config", None)
+ if not isinstance(config, dict) or not config:
+ return Response().error("无效的 MCP 服务器配置").__dict__
+
+ if "mcpServers" in config:
+ keys = list(config["mcpServers"].keys())
+ if not keys:
+ return Response().error("MCP 服务器配置不能为空").__dict__
+ if len(keys) > 1:
+ return Response().error("一次只能配置一个 MCP 服务器配置").__dict__
+ config = config["mcpServers"][keys[0]]
+ else:
+ if not config:
+ return Response().error("MCP 服务器配置不能为空").__dict__
+
</code_context>
<issue_to_address>
**suggestion:** MCP 服务器配置的验证逻辑可以简化，以避免冗余检查。
合并对 'not config' 和 'not isinstance(config, dict)' 的重复检查将使验证逻辑更清晰且更易于维护。
</issue_to_address>
### 评论 3
<location> `astrbot/core/agent/mcp_client.py:48` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** 抛出特定错误而不是通用 `Exception` 或 `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>如果一段代码抛出特定异常类型
而不是通用的
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
或 [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception)，
则调用代码可以：
- 获取有关错误类型的更多信息
- 为其定义特定的异常处理
这样，代码的调用者可以适当地处理错误。
你如何解决这个问题？
- 使用标准库中的 [内置异常](https://docs.python.org/3/library/exceptions.html)。
- [定义自己的错误类](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions)，它继承自 `Exception`。
因此，与其抛出 `Exception` 或 `BaseException` 的代码，例如
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
你可以让代码抛出特定错误，例如
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
或者
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### 评论 4
<location> `astrbot/core/agent/mcp_client.py:138` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** 抛出特定错误而不是通用 `Exception` 或 `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>如果一段代码抛出特定异常类型
而不是通用的
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
或 [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception)，
则调用代码可以：
- 获取有关错误类型的更多信息
- 为其定义特定的异常处理
这样，代码的调用者可以适当地处理错误。
你如何解决这个问题？
- 使用标准库中的 [内置异常](https://docs.python.org/3/library/exceptions.html)。
- [定义自己的错误类](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions)，它继承自 `Exception`。
因此，与其抛出 `Exception` 或 `BaseException` 的代码，例如
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
你可以让代码抛出特定错误，例如
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
或者
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### 评论 5
<location> `astrbot/core/pipeline/process_stage/method/llm_request.py:211-213` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** 抛出特定错误而不是通用 `Exception` 或 `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>如果一段代码抛出特定异常类型
而不是通用的
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
或 [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception)，
则调用代码可以：
- 获取有关错误类型的更多信息
- 为其定义特定的异常处理
这样，代码的调用者可以适当地处理错误。
你如何解决这个问题？
- 使用标准库中的 [内置异常](https://docs.python.org/3/library/exceptions.html)。
- [定义自己的错误类](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions)，它继承自 `Exception`。
因此，与其抛出 `Exception` 或 `BaseException` 的代码，例如
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
你可以让代码抛出特定错误，例如
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
或者
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### 评论 6
<location> `astrbot/core/pipeline/process_stage/method/llm_request.py:191` </location>
<code_context>
@classmethod
async def _execute_local(
cls,
tool: FunctionTool,
run_context: ContextWrapper[AstrAgentContext],
**tool_args,
):
if not run_context.event:
raise ValueError("Event must be provided for local function tools.")
# 检查 tool 下有没有 run 方法
if not tool.handler and not hasattr(tool, "run"):
raise ValueError("Tool must have a valid handler or 'run' method.")
awaitable = tool.handler or getattr(tool, "run")
wrapper = call_handler(
event=run_context.event,
handler=awaitable,
**tool_args,
)
# async for resp in wrapper:
while True:
try:
resp = await asyncio.wait_for(
anext(wrapper),
timeout=run_context.context.tool_call_timeout,
)
if resp is not None:
if isinstance(resp, mcp.types.CallToolResult):
yield resp
else:
text_content = mcp.types.TextContent(
type="text",
text=str(resp),
)
yield mcp.types.CallToolResult(content=[text_content])
else:
# NOTE: Tool 在这里直接请求发送消息给用户
# TODO: 是否需要判断 event.get_result() 是否为空?
# 如果为空,则说明没有发送消息给用户,并且返回值为空,将返回一个特殊的 TextContent,其内容如"工具没有返回内容"
yield None
except asyncio.TimeoutError:
raise Exception(
f"tool {tool.name} execution timeout after {run_context.context.tool_call_timeout} seconds."
)
except StopAsyncIteration:
break
</code_context>
<issue_to_address>
**issue (code-quality):** 我们发现了这些问题：
- 交换 if/else 分支 ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- 将 else 子句的嵌套 if 语句合并到 elif 中 ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
- 明确地从先前的错误中抛出 ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
</issue_to_address>
### 评论 7
<location> `astrbot/dashboard/routes/tools.py:285-288` </location>
<code_context>
async def test_mcp_connection(self):
"""
测试 MCP 服务器连接
"""
try:
server_data = await request.json
config = server_data.get("mcp_server_config", None)
if not isinstance(config, dict) or not config:
return Response().error("无效的 MCP 服务器配置").__dict__
if "mcpServers" in config:
keys = list(config["mcpServers"].keys())
if not keys:
return Response().error("MCP 服务器配置不能为空").__dict__
if len(keys) > 1:
return Response().error("一次只能配置一个 MCP 服务器配置").__dict__
config = config["mcpServers"][keys[0]]
else:
if not config:
return Response().error("MCP 服务器配置不能为空").__dict__
tools_name = await self.tool_mgr.test_mcp_server_connection(config)
return (
Response().ok(data=tools_name, message="🎉 MCP 服务器可用！").__dict__
)
except Exception as e:
logger.error(traceback.format_exc())
return Response().error(f"测试 MCP 连接失败: {str(e)}").__dict__
</code_context>
<issue_to_address>
**suggestion (code-quality):** 我们发现了这些问题：
- 将 else 子句的嵌套 if 语句合并到 elif 中 ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
- 在控制流跳转后将代码提升到 else 中 ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
```suggestion
else:
config = config["mcpServers"][keys[0]]
elif not config:
return Response().error("MCP 服务器配置不能为空").__dict__
```
</issue_to_address>帮助我更有用！请点击每个评论上的 👍 或 👎，我将使用反馈来改进你的评论。
Original comment in English
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `astrbot/core/pipeline/process_stage/method/llm_request.py:211` </location>
<code_context>
+ # 如果为空,则说明没有发送消息给用户,并且返回值为空,将返回一个特殊的 TextContent,其内容如"工具没有返回内容"
+ yield None
+ except asyncio.TimeoutError:
+ raise Exception(
+ f"tool {tool.name} execution timeout after {run_context.context.tool_call_timeout} seconds."
+ )
</code_context>
<issue_to_address>
**suggestion:** Raising a generic Exception on timeout may obscure the error source.
Consider raising a specific exception, such as ToolTimeoutError, instead of Exception to make timeout errors easier to identify and handle.
Suggested implementation:
```python
except asyncio.TimeoutError:
raise ToolTimeoutError(
f"tool {tool.name} execution timeout after {run_context.context.tool_call_timeout} seconds."
)
```
```python
# NOTE: Tool 在这里直接请求发送消息给用户
# TODO: 是否需要判断 event.get_result() 是否为空?
# 如果为空,则说明没有发送消息给用户,并且返回值为空,将返回一个特殊的 TextContent,其内容如"工具没有返回内容"
yield None
class ToolTimeoutError(Exception):
"""Raised when a tool execution times out."""
pass
```
If `ToolTimeoutError` is already defined elsewhere in your codebase, you should import it at the top of this file instead of defining it here.
</issue_to_address>
### Comment 2
<location> `astrbot/dashboard/routes/tools.py:276-285` </location>
<code_context>
server_data = await request.json
config = server_data.get("mcp_server_config", None)
+ if not isinstance(config, dict) or not config:
+ return Response().error("无效的 MCP 服务器配置").__dict__
+
+ if "mcpServers" in config:
+ keys = list(config["mcpServers"].keys())
+ if not keys:
+ return Response().error("MCP 服务器配置不能为空").__dict__
+ if len(keys) > 1:
+ return Response().error("一次只能配置一个 MCP 服务器配置").__dict__
+ config = config["mcpServers"][keys[0]]
+ else:
+ if not config:
+ return Response().error("MCP 服务器配置不能为空").__dict__
+
</code_context>
<issue_to_address>
**suggestion:** The validation logic for MCP server config could be streamlined to avoid redundant checks.
Consolidating the repeated checks for 'not config' and 'not isinstance(config, dict)' will make the validation logic clearer and easier to maintain.
</issue_to_address>
### Comment 3
<location> `astrbot/core/agent/mcp_client.py:48` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 4
<location> `astrbot/core/agent/mcp_client.py:138` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 5
<location> `astrbot/core/pipeline/process_stage/method/llm_request.py:211-213` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Raise a specific error instead of the general `Exception` or `BaseException` ([`raise-specific-error`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/raise-specific-error))
<details><summary>Explanation</summary>If a piece of code raises a specific exception type
rather than the generic
[`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException)
or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception),
the calling code can:
- get more information about what type of error it is
- define specific exception handling for it
This way, callers of the code can handle the error appropriately.
How can you solve this?
- Use one of the [built-in exceptions](https://docs.python.org/3/library/exceptions.html) of the standard library.
- [Define your own error class](https://docs.python.org/3/tutorial/errors.html#tut-userexceptions) that subclasses `Exception`.
So instead of having code raising `Exception` or `BaseException` like
```python
if incorrect_input(value):
raise Exception("The input is incorrect")
```
you can have code raising a specific error like
```python
if incorrect_input(value):
raise ValueError("The input is incorrect")
```
or
```python
class IncorrectInputError(Exception):
pass
if incorrect_input(value):
raise IncorrectInputError("The input is incorrect")
```
</details>
</issue_to_address>
### Comment 6
<location> `astrbot/core/pipeline/process_stage/method/llm_request.py:191` </location>
<code_context>
@classmethod
async def _execute_local(
cls,
tool: FunctionTool,
run_context: ContextWrapper[AstrAgentContext],
**tool_args,
):
if not run_context.event:
raise ValueError("Event must be provided for local function tools.")
# 检查 tool 下有没有 run 方法
if not tool.handler and not hasattr(tool, "run"):
raise ValueError("Tool must have a valid handler or 'run' method.")
awaitable = tool.handler or getattr(tool, "run")
wrapper = call_handler(
event=run_context.event,
handler=awaitable,
**tool_args,
)
# async for resp in wrapper:
while True:
try:
resp = await asyncio.wait_for(
anext(wrapper),
timeout=run_context.context.tool_call_timeout,
)
if resp is not None:
if isinstance(resp, mcp.types.CallToolResult):
yield resp
else:
text_content = mcp.types.TextContent(
type="text",
text=str(resp),
)
yield mcp.types.CallToolResult(content=[text_content])
else:
# NOTE: Tool 在这里直接请求发送消息给用户
# TODO: 是否需要判断 event.get_result() 是否为空?
# 如果为空,则说明没有发送消息给用户,并且返回值为空,将返回一个特殊的 TextContent,其内容如"工具没有返回内容"
yield None
except asyncio.TimeoutError:
raise Exception(
f"tool {tool.name} execution timeout after {run_context.context.tool_call_timeout} seconds."
)
except StopAsyncIteration:
break
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Merge else clause's nested if statement into elif ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
- Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
</issue_to_address>
### Comment 7
<location> `astrbot/dashboard/routes/tools.py:285-288` </location>
<code_context>
async def test_mcp_connection(self):
"""
测试 MCP 服务器连接
"""
try:
server_data = await request.json
config = server_data.get("mcp_server_config", None)
if not isinstance(config, dict) or not config:
return Response().error("无效的 MCP 服务器配置").__dict__
if "mcpServers" in config:
keys = list(config["mcpServers"].keys())
if not keys:
return Response().error("MCP 服务器配置不能为空").__dict__
if len(keys) > 1:
return Response().error("一次只能配置一个 MCP 服务器配置").__dict__
config = config["mcpServers"][keys[0]]
else:
if not config:
return Response().error("MCP 服务器配置不能为空").__dict__
tools_name = await self.tool_mgr.test_mcp_server_connection(config)
return (
Response().ok(data=tools_name, message="🎉 MCP 服务器可用！").__dict__
)
except Exception as e:
logger.error(traceback.format_exc())
return Response().error(f"测试 MCP 连接失败: {str(e)}").__dict__
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Merge else clause's nested if statement into elif ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
```suggestion
else:
config = config["mcpServers"][keys[0]]
elif not config:
return Response().error("MCP 服务器配置不能为空").__dict__
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Soulter
changed the title
LIghtJUNction
approved these changes
Oct 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
closes: #2939
Motivation / 动机
Modifications / 改动点
Verification Steps / 验证步骤
Screenshots or Test Results / 运行截图或测试结果
Compatibility & Breaking Changes / 兼容性与破坏性变更
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.Sourcery 总结
引入可配置的工具调用超时，并调整 MCP 服务器配置流程以处理 ModelScope 的嵌套设置，同时更新了默认值并改进了验证功能
新功能：
tool_call_timeout设置，默认为 60 秒，适用于本地和 MCP 工具执行mcpServers配置，并接受“transport”或“type”字段作为 MCP 客户端传输方式改进：
asyncio.wait_for强制执行工具调用超时，并在超时时引发异常read_timeout_seconds传递给session.call_toolOriginal summary in English
Summary by Sourcery
Introduce configurable tool call timeouts and adapt the MCP Server configuration flow to handle ModelScope's nested setup, with updated defaults and improved validation
New Features:
Enhancements: