[codex] fix mcp init timeout keyword mismatch#5743
Merged
zouyonghe merged 4 commits intoAstrBotDevs:masterfrom Mar 4, 2026
Merged
[codex] fix mcp init timeout keyword mismatch#5743zouyonghe merged 4 commits intoAstrBotDevs:masterfrom
zouyonghe merged 4 commits intoAstrBotDevs:masterfrom
Conversation
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical regression that caused MCP server initialization to fail due to a keyword argument mismatch. By correcting the argument name passed to the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dosubot
bot
added
the
size:M
|
Related Documentation Checked 1 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
dosubot
bot
added
the
area:provider
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/test_func_tool_manager.py" line_range="9" />
<code_context>
+from astrbot.core.provider.func_tool_manager import FunctionToolManager
+
+
+@pytest.mark.asyncio
+async def test_init_mcp_clients_passes_timeout_seconds_keyword(
+ monkeypatch: pytest.MonkeyPatch,
</code_context>
<issue_to_address>
**suggestion (testing):** 添加一个测试，用于验证在使用非默认的 `init_timeout` 值时的行为。
为了更好地覆盖此次回归，请同时测试一个非默认的超时时间（例如 `await manager.init_mcp_clients(init_timeout=3.5)`），并断言 `called["demo"]["timeout_seconds"]` 等于该值。这样可以验证当调用方覆盖超时时间时，该参数能够被正确传递。
</issue_to_address>帮助我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈来改进后续的评审。
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/test_func_tool_manager.py" line_range="9" />
<code_context>
+from astrbot.core.provider.func_tool_manager import FunctionToolManager
+
+
+@pytest.mark.asyncio
+async def test_init_mcp_clients_passes_timeout_seconds_keyword(
+ monkeypatch: pytest.MonkeyPatch,
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that exercises a non-default `init_timeout` value
To better cover the regression, please also test a non-default timeout (e.g. `await manager.init_mcp_clients(init_timeout=3.5)`) and assert that `called["demo"]["timeout_seconds"]` equals that value. This will validate the parameter is correctly forwarded when callers override the timeout.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/unit/test_func_tool_manager.py
Outdated
| from astrbot.core.provider.func_tool_manager import FunctionToolManager | ||
|
|
||
|
|
||
| @pytest.mark.asyncio |
Contributor
There was a problem hiding this comment.
suggestion (testing): 添加一个测试，用于验证在使用非默认的 init_timeout 值时的行为。
为了更好地覆盖此次回归，请同时测试一个非默认的超时时间（例如 await manager.init_mcp_clients(init_timeout=3.5)），并断言 called["demo"]["timeout_seconds"] 等于该值。这样可以验证当调用方覆盖超时时间时，该参数能够被正确传递。
Original comment in English
suggestion (testing): Add a test that exercises a non-default init_timeout value
To better cover the regression, please also test a non-default timeout (e.g. await manager.init_mcp_clients(init_timeout=3.5)) and assert that called["demo"]["timeout_seconds"] equals that value. This will validate the parameter is correctly forwarded when callers override the timeout.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a TypeError regression in MCP client initialization by correcting a mismatched keyword argument. The fix in init_mcp_clients is direct and accurate. The addition of a new unit test, test_init_mcp_clients_passes_timeout_seconds_keyword, is an excellent practice for preventing future regressions. The test is well-constructed, using monkeypatch and tmp_path to create an isolated and reliable check for the fix. The proactive static analysis for similar issues across the repository is also commendable. The changes are of high quality and improve the robustness of the MCP initialization process.
dosubot
bot
added
size:L
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/func_tool_manager.py" line_range="351-352" />
<code_context>
async def init_mcp_clients(
- self, raise_on_all_failed: bool = False
+ self,
+ raise_on_all_failed: bool = False,
+ *,
+ init_timeout: float | int | str | None = None,
) -> MCPInitSummary:
</code_context>
<issue_to_address>
**issue (bug_risk):** 新增的 `init_timeout` 参数路径绕过了环境变量，从而改变了文档中描述的行为。
目前只有在调用方传入非 `None` 的 `init_timeout` 时才会读取环境变量；如果省略/为 `None`，代码始终使用 `self._init_timeout_default`，因此会忽略 `MCP_INIT_TIMEOUT_ENV`。为了在允许显式覆盖的同时保留文档中的行为，建议委托给 `_resolve_timeout`：
```python
init_timeout_value = _resolve_timeout(
timeout=init_timeout,
env_name=MCP_INIT_TIMEOUT_ENV,
default=self._init_timeout_default,
)
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/func_tool_manager.py" line_range="351-352" />
<code_context>
async def init_mcp_clients(
- self, raise_on_all_failed: bool = False
+ self,
+ raise_on_all_failed: bool = False,
+ *,
+ init_timeout: float | int | str | None = None,
) -> MCPInitSummary:
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `init_timeout` parameter path bypasses the environment variable and changes the documented behavior.
Right now the env var is only read when the caller passes a non-`None` `init_timeout`; if it’s omitted/`None`, the code always uses `self._init_timeout_default`, so `MCP_INIT_TIMEOUT_ENV` is ignored. To preserve the documented behavior while still allowing explicit overrides, consider delegating to `_resolve_timeout`:
```python
init_timeout_value = _resolve_timeout(
timeout=init_timeout,
env_name=MCP_INIT_TIMEOUT_ENV,
default=self._init_timeout_default,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我这边发现了 1 个问题，并给出了一些整体反馈：
- 这三个新测试在前置构造和假实现
_start_mcp_server上有大量重复；可以考虑抽取一个小的 helper/fixture，用于构建带有 stub_start_mcp_server和数据目录的FunctionToolManager，这样可以保持测试的 DRY（不重复）并且更易扩展。 - 既然
init_mcp_clients现在接收一个仅限关键字参数的init_timeout，如果有更高层级的封装/调用方，可能值得更新它们，把这个参数透传下去，而不是只依赖环境变量中的默认值。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- 这三个新测试在前置构造和假实现 `_start_mcp_server` 上有大量重复；可以考虑抽取一个小的 helper/fixture，用于构建带有 stub `_start_mcp_server` 和数据目录的 `FunctionToolManager`，这样可以保持测试的 DRY（不重复）并且更易扩展。
- 既然 `init_mcp_clients` 现在接收一个仅限关键字参数的 `init_timeout`，如果有更高层级的封装/调用方，可能值得更新它们，把这个参数透传下去，而不是只依赖环境变量中的默认值。
## Individual Comments
### Comment 1
<location path="tests/unit/test_func_tool_manager.py" line_range="49-81" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_init_mcp_clients_passes_overridden_init_timeout(
+ monkeypatch: pytest.MonkeyPatch,
+ tmp_path,
+):
+ manager = FunctionToolManager()
+ data_dir = tmp_path / "data"
+ data_dir.mkdir()
+
+ (data_dir / "mcp_server.json").write_text(
+ json.dumps({"mcpServers": {"demo": {"active": True}}}),
+ encoding="utf-8",
+ )
+ monkeypatch.setattr(
+ func_tool_manager,
+ "get_astrbot_data_path",
+ lambda: data_dir,
+ )
+
+ called = {}
+
+ async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds):
+ called[name] = {
+ "cfg": cfg,
+ "shutdown_event_type": type(shutdown_event).__name__,
+ "timeout_seconds": timeout_seconds,
+ }
+
+ monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server)
+
+ summary = await manager.init_mcp_clients(init_timeout=3.5)
+
+ assert summary.total == 1
</code_context>
<issue_to_address>
**suggestion (testing):** Cover precedence when both `init_timeout` and `ASTRBOT_MCP_INIT_TIMEOUT` are set
You already cover the explicit `init_timeout` case and the env-var-only case. Please also add a test where both `ASTRBOT_MCP_INIT_TIMEOUT` and `init_timeout` are set, asserting that the explicit argument takes precedence. This will directly exercise the documented behavior and protect against regressions in `_resolve_timeout` or its call sites.
```suggestion
@pytest.mark.asyncio
async def test_init_mcp_clients_passes_overridden_init_timeout(
monkeypatch: pytest.MonkeyPatch,
tmp_path,
):
manager = FunctionToolManager()
data_dir = tmp_path / "data"
data_dir.mkdir()
(data_dir / "mcp_server.json").write_text(
json.dumps({"mcpServers": {"demo": {"active": True}}}),
encoding="utf-8",
)
monkeypatch.setattr(
func_tool_manager,
"get_astrbot_data_path",
lambda: data_dir,
)
called = {}
async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds):
called[name] = {
"cfg": cfg,
"shutdown_event_type": type(shutdown_event).__name__,
"timeout_seconds": timeout_seconds,
}
monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server)
summary = await manager.init_mcp_clients(init_timeout=3.5)
assert summary.total == 1
@pytest.mark.asyncio
async def test_init_mcp_clients_prefers_explicit_timeout_over_env(
monkeypatch: pytest.MonkeyPatch,
tmp_path,
):
manager = FunctionToolManager()
data_dir = tmp_path / "data"
data_dir.mkdir()
(data_dir / "mcp_server.json").write_text(
json.dumps({"mcpServers": {"demo": {"active": True}}}),
encoding="utf-8",
)
monkeypatch.setattr(
func_tool_manager,
"get_astrbot_data_path",
lambda: data_dir,
)
# Set an environment default that should be overridden by the explicit argument
monkeypatch.setenv("ASTRBOT_MCP_INIT_TIMEOUT", "7.0")
called = {}
async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds):
called[name] = {
"cfg": cfg,
"shutdown_event_type": type(shutdown_event).__name__,
"timeout_seconds": timeout_seconds,
}
monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server)
summary = await manager.init_mcp_clients(init_timeout=3.5)
assert summary.total == 1
# Explicit argument should take precedence over the env var default
assert called["demo"]["timeout_seconds"] == 3.5
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进评审质量。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The three new tests duplicate a lot of setup and the fake
_start_mcp_serverimplementation; consider extracting a small helper/fixture to build aFunctionToolManagerwith a stubbed_start_mcp_serverand data dir to keep them DRY and easier to extend. - Now that
init_mcp_clientstakes a keyword-onlyinit_timeout, it may be worth updating any higher-level wrappers/callers (if any exist) to thread through this parameter rather than relying solely on the environment default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The three new tests duplicate a lot of setup and the fake `_start_mcp_server` implementation; consider extracting a small helper/fixture to build a `FunctionToolManager` with a stubbed `_start_mcp_server` and data dir to keep them DRY and easier to extend.
- Now that `init_mcp_clients` takes a keyword-only `init_timeout`, it may be worth updating any higher-level wrappers/callers (if any exist) to thread through this parameter rather than relying solely on the environment default.
## Individual Comments
### Comment 1
<location path="tests/unit/test_func_tool_manager.py" line_range="49-81" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_init_mcp_clients_passes_overridden_init_timeout(
+ monkeypatch: pytest.MonkeyPatch,
+ tmp_path,
+):
+ manager = FunctionToolManager()
+ data_dir = tmp_path / "data"
+ data_dir.mkdir()
+
+ (data_dir / "mcp_server.json").write_text(
+ json.dumps({"mcpServers": {"demo": {"active": True}}}),
+ encoding="utf-8",
+ )
+ monkeypatch.setattr(
+ func_tool_manager,
+ "get_astrbot_data_path",
+ lambda: data_dir,
+ )
+
+ called = {}
+
+ async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds):
+ called[name] = {
+ "cfg": cfg,
+ "shutdown_event_type": type(shutdown_event).__name__,
+ "timeout_seconds": timeout_seconds,
+ }
+
+ monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server)
+
+ summary = await manager.init_mcp_clients(init_timeout=3.5)
+
+ assert summary.total == 1
</code_context>
<issue_to_address>
**suggestion (testing):** Cover precedence when both `init_timeout` and `ASTRBOT_MCP_INIT_TIMEOUT` are set
You already cover the explicit `init_timeout` case and the env-var-only case. Please also add a test where both `ASTRBOT_MCP_INIT_TIMEOUT` and `init_timeout` are set, asserting that the explicit argument takes precedence. This will directly exercise the documented behavior and protect against regressions in `_resolve_timeout` or its call sites.
```suggestion
@pytest.mark.asyncio
async def test_init_mcp_clients_passes_overridden_init_timeout(
monkeypatch: pytest.MonkeyPatch,
tmp_path,
):
manager = FunctionToolManager()
data_dir = tmp_path / "data"
data_dir.mkdir()
(data_dir / "mcp_server.json").write_text(
json.dumps({"mcpServers": {"demo": {"active": True}}}),
encoding="utf-8",
)
monkeypatch.setattr(
func_tool_manager,
"get_astrbot_data_path",
lambda: data_dir,
)
called = {}
async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds):
called[name] = {
"cfg": cfg,
"shutdown_event_type": type(shutdown_event).__name__,
"timeout_seconds": timeout_seconds,
}
monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server)
summary = await manager.init_mcp_clients(init_timeout=3.5)
assert summary.total == 1
@pytest.mark.asyncio
async def test_init_mcp_clients_prefers_explicit_timeout_over_env(
monkeypatch: pytest.MonkeyPatch,
tmp_path,
):
manager = FunctionToolManager()
data_dir = tmp_path / "data"
data_dir.mkdir()
(data_dir / "mcp_server.json").write_text(
json.dumps({"mcpServers": {"demo": {"active": True}}}),
encoding="utf-8",
)
monkeypatch.setattr(
func_tool_manager,
"get_astrbot_data_path",
lambda: data_dir,
)
# Set an environment default that should be overridden by the explicit argument
monkeypatch.setenv("ASTRBOT_MCP_INIT_TIMEOUT", "7.0")
called = {}
async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds):
called[name] = {
"cfg": cfg,
"shutdown_event_type": type(shutdown_event).__name__,
"timeout_seconds": timeout_seconds,
}
monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server)
summary = await manager.init_mcp_clients(init_timeout=3.5)
assert summary.total == 1
# Explicit argument should take precedence over the env var default
assert called["demo"]["timeout_seconds"] == 3.5
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/unit/test_func_tool_manager.py
Outdated
Comment on lines
+49
to
+81
| @pytest.mark.asyncio | ||
| async def test_init_mcp_clients_passes_overridden_init_timeout( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| tmp_path, | ||
| ): | ||
| manager = FunctionToolManager() | ||
| data_dir = tmp_path / "data" | ||
| data_dir.mkdir() | ||
|
|
||
| (data_dir / "mcp_server.json").write_text( | ||
| json.dumps({"mcpServers": {"demo": {"active": True}}}), | ||
| encoding="utf-8", | ||
| ) | ||
| monkeypatch.setattr( | ||
| func_tool_manager, | ||
| "get_astrbot_data_path", | ||
| lambda: data_dir, | ||
| ) | ||
|
|
||
| called = {} | ||
|
|
||
| async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds): | ||
| called[name] = { | ||
| "cfg": cfg, | ||
| "shutdown_event_type": type(shutdown_event).__name__, | ||
| "timeout_seconds": timeout_seconds, | ||
| } | ||
|
|
||
| monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server) | ||
|
|
||
| summary = await manager.init_mcp_clients(init_timeout=3.5) | ||
|
|
||
| assert summary.total == 1 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Cover precedence when both init_timeout and ASTRBOT_MCP_INIT_TIMEOUT are set
你已经覆盖了只传显式 init_timeout 和只用环境变量这两种情况。请再添加一个测试：同时设置 ASTRBOT_MCP_INIT_TIMEOUT 和 init_timeout，并断言显式参数优先生效。这样可以直接验证文档描述的行为，并防止 _resolve_timeout 或其调用点在将来出现回归。
Suggested change
| @pytest.mark.asyncio | |
| async def test_init_mcp_clients_passes_overridden_init_timeout( | |
| monkeypatch: pytest.MonkeyPatch, | |
| tmp_path, | |
| ): | |
| manager = FunctionToolManager() | |
| data_dir = tmp_path / "data" | |
| data_dir.mkdir() | |
| (data_dir / "mcp_server.json").write_text( | |
| json.dumps({"mcpServers": {"demo": {"active": True}}}), | |
| encoding="utf-8", | |
| ) | |
| monkeypatch.setattr( | |
| func_tool_manager, | |
| "get_astrbot_data_path", | |
| lambda: data_dir, | |
| ) | |
| called = {} | |
| async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds): | |
| called[name] = { | |
| "cfg": cfg, | |
| "shutdown_event_type": type(shutdown_event).__name__, | |
| "timeout_seconds": timeout_seconds, | |
| } | |
| monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server) | |
| summary = await manager.init_mcp_clients(init_timeout=3.5) | |
| assert summary.total == 1 | |
| @pytest.mark.asyncio | |
| async def test_init_mcp_clients_passes_overridden_init_timeout( | |
| monkeypatch: pytest.MonkeyPatch, | |
| tmp_path, | |
| ): | |
| manager = FunctionToolManager() | |
| data_dir = tmp_path / "data" | |
| data_dir.mkdir() | |
| (data_dir / "mcp_server.json").write_text( | |
| json.dumps({"mcpServers": {"demo": {"active": True}}}), | |
| encoding="utf-8", | |
| ) | |
| monkeypatch.setattr( | |
| func_tool_manager, | |
| "get_astrbot_data_path", | |
| lambda: data_dir, | |
| ) | |
| called = {} | |
| async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds): | |
| called[name] = { | |
| "cfg": cfg, | |
| "shutdown_event_type": type(shutdown_event).__name__, | |
| "timeout_seconds": timeout_seconds, | |
| } | |
| monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server) | |
| summary = await manager.init_mcp_clients(init_timeout=3.5) | |
| assert summary.total == 1 | |
| @pytest.mark.asyncio | |
| async def test_init_mcp_clients_prefers_explicit_timeout_over_env( | |
| monkeypatch: pytest.MonkeyPatch, | |
| tmp_path, | |
| ): | |
| manager = FunctionToolManager() | |
| data_dir = tmp_path / "data" | |
| data_dir.mkdir() | |
| (data_dir / "mcp_server.json").write_text( | |
| json.dumps({"mcpServers": {"demo": {"active": True}}}), | |
| encoding="utf-8", | |
| ) | |
| monkeypatch.setattr( | |
| func_tool_manager, | |
| "get_astrbot_data_path", | |
| lambda: data_dir, | |
| ) | |
| # Set an environment default that should be overridden by the explicit argument | |
| monkeypatch.setenv("ASTRBOT_MCP_INIT_TIMEOUT", "7.0") | |
| called = {} | |
| async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds): | |
| called[name] = { | |
| "cfg": cfg, | |
| "shutdown_event_type": type(shutdown_event).__name__, | |
| "timeout_seconds": timeout_seconds, | |
| } | |
| monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server) | |
| summary = await manager.init_mcp_clients(init_timeout=3.5) | |
| assert summary.total == 1 | |
| # Explicit argument should take precedence over the env var default | |
| assert called["demo"]["timeout_seconds"] == 3.5 |
Original comment in English
suggestion (testing): Cover precedence when both init_timeout and ASTRBOT_MCP_INIT_TIMEOUT are set
You already cover the explicit init_timeout case and the env-var-only case. Please also add a test where both ASTRBOT_MCP_INIT_TIMEOUT and init_timeout are set, asserting that the explicit argument takes precedence. This will directly exercise the documented behavior and protect against regressions in _resolve_timeout or its call sites.
Suggested change
| @pytest.mark.asyncio | |
| async def test_init_mcp_clients_passes_overridden_init_timeout( | |
| monkeypatch: pytest.MonkeyPatch, | |
| tmp_path, | |
| ): | |
| manager = FunctionToolManager() | |
| data_dir = tmp_path / "data" | |
| data_dir.mkdir() | |
| (data_dir / "mcp_server.json").write_text( | |
| json.dumps({"mcpServers": {"demo": {"active": True}}}), | |
| encoding="utf-8", | |
| ) | |
| monkeypatch.setattr( | |
| func_tool_manager, | |
| "get_astrbot_data_path", | |
| lambda: data_dir, | |
| ) | |
| called = {} | |
| async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds): | |
| called[name] = { | |
| "cfg": cfg, | |
| "shutdown_event_type": type(shutdown_event).__name__, | |
| "timeout_seconds": timeout_seconds, | |
| } | |
| monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server) | |
| summary = await manager.init_mcp_clients(init_timeout=3.5) | |
| assert summary.total == 1 | |
| @pytest.mark.asyncio | |
| async def test_init_mcp_clients_passes_overridden_init_timeout( | |
| monkeypatch: pytest.MonkeyPatch, | |
| tmp_path, | |
| ): | |
| manager = FunctionToolManager() | |
| data_dir = tmp_path / "data" | |
| data_dir.mkdir() | |
| (data_dir / "mcp_server.json").write_text( | |
| json.dumps({"mcpServers": {"demo": {"active": True}}}), | |
| encoding="utf-8", | |
| ) | |
| monkeypatch.setattr( | |
| func_tool_manager, | |
| "get_astrbot_data_path", | |
| lambda: data_dir, | |
| ) | |
| called = {} | |
| async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds): | |
| called[name] = { | |
| "cfg": cfg, | |
| "shutdown_event_type": type(shutdown_event).__name__, | |
| "timeout_seconds": timeout_seconds, | |
| } | |
| monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server) | |
| summary = await manager.init_mcp_clients(init_timeout=3.5) | |
| assert summary.total == 1 | |
| @pytest.mark.asyncio | |
| async def test_init_mcp_clients_prefers_explicit_timeout_over_env( | |
| monkeypatch: pytest.MonkeyPatch, | |
| tmp_path, | |
| ): | |
| manager = FunctionToolManager() | |
| data_dir = tmp_path / "data" | |
| data_dir.mkdir() | |
| (data_dir / "mcp_server.json").write_text( | |
| json.dumps({"mcpServers": {"demo": {"active": True}}}), | |
| encoding="utf-8", | |
| ) | |
| monkeypatch.setattr( | |
| func_tool_manager, | |
| "get_astrbot_data_path", | |
| lambda: data_dir, | |
| ) | |
| # Set an environment default that should be overridden by the explicit argument | |
| monkeypatch.setenv("ASTRBOT_MCP_INIT_TIMEOUT", "7.0") | |
| called = {} | |
| async def fake_start_mcp_server(*, name, cfg, shutdown_event, timeout_seconds): | |
| called[name] = { | |
| "cfg": cfg, | |
| "shutdown_event_type": type(shutdown_event).__name__, | |
| "timeout_seconds": timeout_seconds, | |
| } | |
| monkeypatch.setattr(manager, "_start_mcp_server", fake_start_mcp_server) | |
| summary = await manager.init_mcp_clients(init_timeout=3.5) | |
| assert summary.total == 1 | |
| # Explicit argument should take precedence over the env var default | |
| assert called["demo"]["timeout_seconds"] == 3.5 |
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - 我已经审查了你的改动，一切看起来都很棒！
帮我变得更有用吧！请在每条评论上点 👍 或 👎，我会根据你的反馈来改进之后的代码审查。
Original comment in English
Hey - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
astrbot-doc-agent bot
pushed a commit
to AstrBotDevs/AstrBot-docs
that referenced
this pull request
Mar 4, 2026
|
Generated docs update PR (pending manual review): AI change summary:
Experimental bot notice:
|
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 5, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 7, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 7, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 9, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 10, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 11, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 11, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 12, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
stevessr
pushed a commit
to stevessr/AstrBot
that referenced
this pull request
Mar 13, 2026
* fix: use timeout_seconds for mcp init startup * fix: support overridden mcp init timeout in startup * fix: resolve mcp init timeout from env when unset * fix: pass mcp init timeout through lifecycle chain
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
This PR fixes an MCP initialization regression where
init_mcp_clientspassed the wrong keyword argument to_start_mcp_server.User Impact
When MCP servers are initialized from
mcp_server.json, startup could fail with a runtimeTypeError(unexpected keyword argument 'timeout'). In that case MCP tools are not loaded at boot, so users may see missing MCP tool availability until manual intervention.Root Cause
FunctionToolManager._start_mcp_serveracceptstimeout_seconds, butinit_mcp_clientsinvoked it withtimeout=init_timeout. The mismatch bypassed timeout handling and raised at call time.Fix
init_mcp_clientsto call_start_mcp_server(..., timeout_seconds=init_timeout)._start_mcp_serverand asserts the timeout is forwarded using thetimeout_secondskeyword.Validation
uv run pytest -q tests/unit/test_func_tool_manager.py -k timeout_seconds -quv run ruff format .uv run ruff check .Additional Analysis
I also ran repository-wide static analysis for similar keyword/signature mismatches (AST-based scan of local function/method calls where targets have no
**kwargs) and found no additional issues of the same class.Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds关键字参数传递超时时间值。测试：
init_mcp_clients会通过timeout_seconds正确转发默认和自定义的初始化超时时间，并成功初始化一个处于活跃状态的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复:
timeout_seconds关键字传递超时值。测试:
init_mcp_clients会通过timeout_seconds关键字转发超时参数，并且能够根据配置成功初始化一个处于活动状态的 MCP 服务器。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds关键字参数传递超时时间值。测试：
init_mcp_clients会通过timeout_seconds正确转发默认和自定义的初始化超时时间，并成功初始化一个处于活跃状态的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds关键字参数传递超时时间。测试：
init_mcp_clients在启动 MCP 服务器时通过timeout_seconds关键字正确转发超时时间。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds关键字参数传递超时时间值。测试：
init_mcp_clients会通过timeout_seconds正确转发默认和自定义的初始化超时时间，并成功初始化一个处于活跃状态的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复:
timeout_seconds关键字传递超时值。测试:
init_mcp_clients会通过timeout_seconds关键字转发超时参数，并且能够根据配置成功初始化一个处于活动状态的 MCP 服务器。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds关键字参数传递超时时间值。测试：
init_mcp_clients会通过timeout_seconds正确转发默认和自定义的初始化超时时间，并成功初始化一个处于活跃状态的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests:
Bug 修复：
timeout_seconds传递初始化超时时间，防止运行时错误并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖后的以及从环境变量派生的初始化超时时间，并成功初始化已启用的 MCP 服务器配置。Original summary in English
Summary by Sourcery
修复 MCP 客户端初始化超时处理，并在核心初始化流程中传递可配置的超时时间。
Bug 修复：
timeout_seconds关键字传递初始化超时时间，防止运行时出现TypeError，并确保 MCP 工具能够正确完成初始化。测试：
init_mcp_clients能通过timeout_seconds正确转发默认、覆盖以及从环境变量派生的初始化超时时间，并确保在 provider/core 生命周期中正确传递该超时参数。Original summary in English
Summary by Sourcery
Fix MCP client initialization timeout handling and propagate configurable timeouts through the core initialization flow.
Bug Fixes:
Tests: