fix(subagent): enforce handoff args and provider fallback#6873
Open
Jacobinwwey wants to merge 7 commits intoAstrBotDevs:masterfrom
Open
fix(subagent): enforce handoff args and provider fallback#6873Jacobinwwey wants to merge 7 commits intoAstrBotDevs:masterfrom
Jacobinwwey wants to merge 7 commits intoAstrBotDevs:masterfrom
Conversation
dosubot
bot
added
the
size:L
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 significantly enhances the robustness and reliability of subagent handoff calls by introducing stricter argument validation and a resilient provider resolution mechanism. These changes prevent silent failures due to malformed arguments or missing provider configurations, providing immediate and actionable feedback to the main agent for self-correction. Highlights
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. Footnotes
|
dosubot
bot
added
area:core
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- With
additionalProperties: Falseadded to the handoff schema,background_taskis not declared indefault_parameters.properties, so any caller that includes it may now be rejected at the schema/validation layer—consider explicitly addingbackground_task(and any other expected args) to the schema to keep the contract consistent with the executor logic. - In
_resolve_handoff_provider_id, whenprovider_manageris missing or lacksget_provider_by_id, you still return the configured provider ID even though it cannot be validated; consider falling back toctx.get_current_chat_provider_id(umo)in this case so you don’t propagate potentially stale or invalid provider IDs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- With `additionalProperties: False` added to the handoff schema, `background_task` is not declared in `default_parameters.properties`, so any caller that includes it may now be rejected at the schema/validation layer—consider explicitly adding `background_task` (and any other expected args) to the schema to keep the contract consistent with the executor logic.
- In `_resolve_handoff_provider_id`, when `provider_manager` is missing or lacks `get_provider_by_id`, you still return the configured provider ID even though it cannot be validated; consider falling back to `ctx.get_current_chat_provider_id(umo)` in this case so you don’t propagate potentially stale or invalid provider IDs.
## Individual Comments
### Comment 1
<location path="astrbot/core/astr_agent_tool_exec.py" line_range="93-102" />
<code_context>
+ )
+
+ @classmethod
+ def _parse_background_task_arg(
+ cls,
+ tool_name: str,
+ value: T.Any,
+ ) -> tuple[bool, mcp.types.CallToolResult | None]:
+ if value is None:
+ return False, None
+ if isinstance(value, bool):
+ return value, None
+ if isinstance(value, str):
+ normalized = value.strip().lower()
+ if normalized in {"true", "1", "yes", "on"}:
+ return True, None
+ if normalized in {"false", "0", "no", "off", ""}:
+ return False, None
+
+ return False, cls._build_handoff_error_result(
+ tool_name=tool_name,
+ error_type="invalid_background_task",
+ fix_hint=(
+ "`background_task` must be a boolean (`true` or `false`)."
+ ),
+ action_hint=(
+ "Retry the same handoff with `background_task` as a boolean value."
+ ),
+ )
</code_context>
<issue_to_address>
**suggestion:** The `background_task` error message conflicts with the accepted string values.
The parser accepts several string forms (e.g. `"true"`, `"1"`, `"yes"`), but the error says `background_task` must be a boolean (`true` or `false`). This is misleading, since callers might think only native booleans are valid. Please either restrict the implementation to real booleans or update the error text to reflect the accepted string values.
```suggestion
return False, cls._build_handoff_error_result(
tool_name=tool_name,
error_type="invalid_background_task",
fix_hint=(
"`background_task` must be a boolean (`true` or `false`) or a string "
"equivalent such as `\"1\"`/`\"0\"`, `\"yes\"`/`\"no\"`, or "
"`\"on\"`/`\"off\"`."
),
action_hint=(
"Retry the same handoff with `background_task` set to a boolean or one "
"of the supported string equivalents (`\"true\"`, `\"false\"`, "
"`\"1\"`, `\"0\"`, `\"yes\"`, `\"no\"`, `\"on\"`, `\"off\"`)."
),
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: AstrBotTeam's Space pr4697的改动View Suggested Changes@@ -85,11 +85,36 @@
**委派工具参数**
transfer_to_* 工具现支持以下参数：
-- `input`：转交给子代理的任务输入（必填）
+- `input`：转交给子代理的任务输入（**必填**）
+ - 必须为非空字符串，需清晰描述委派任务
+ - 空字符串或仅含空白字符会被拒绝，返回结构化错误消息（错误类型：`missing_or_empty_input`），并提供重试指导
- `image_urls`（可选）：图片 URL 或本地路径数组，用于多模态任务。用户发送给主代理的图片会自动传递给子代理，也可以手动指定 HTTP/HTTPS 公网 URL 或本地文件路径。支持的图片格式包括：png、jpg、jpeg、gif、webp、bmp、tif、tiff、svg、heic。
-- `background_task`：设置任务执行模式
+- `background_task`：设置任务执行模式（可选，默认 `false`）
+ - 接受布尔值（`true` / `false`）或常见布尔字符串表示（`"true"`, `"false"`, `"1"`, `"0"`, `"yes"`, `"no"`, `"on"`, `"off"`）
- 若为 `true`，任务将以后台模式异步执行，通过 `SubagentRuntime.enqueue()` 方法提交到持久化任务队列，主代理立即返回任务 ID，完成后通过后台任务唤醒机制通知用户
- 若未设置或为 `false`，则为同步模式，主代理等待子代理完成任务
+ - 无效值（非布尔类型或无法识别的字符串）会被拒绝，返回结构化错误消息（错误类型：`invalid_background_task`），并提供重试指导
+
+**参数验证与错误处理（PR #6873）**
+
+[PR #6873](https://github.com/AstrBotDevs/AstrBot/pull/6873) 强化了 handoff 工具的参数验证，引入严格的 schema 约束和结构化错误响应：
+
+- **严格 schema 约束**：
+ - handoff 工具 schema 现强制要求 `required: ["input"]` 和 `additionalProperties: false`
+ - `input` 参数为必填，不允许传递未知参数
+ - 传递未知参数时，工具会拒绝请求并返回明确错误消息
+
+- **结构化错误响应**：
+ - 参数验证失败时返回包含 `error_type`、`fix` 提示和 `action` 指导的结构化错误消息
+ - 错误类型包括：
+ - `invalid_background_task`：`background_task` 参数无效（非布尔值或无法识别的字符串）
+ - `missing_or_empty_input`：`input` 参数缺失或为空字符串/空白字符
+ - 错误消息包含重试示例，指导 Agent 立即使用正确参数重试
+
+- **Provider 回退机制**：
+ - 若子代理配置的 `provider_id` 在 provider manager 中不存在，系统自动回退到当前会话的 provider ID
+ - 回退时记录警告日志，便于诊断配置过期或缺失的场景
+ - 确保子代理在 provider 配置失效时不会静默失败
更详细的参数说明和使用示例，请参阅官方 SubAgent 文档的[委派工具参数](https://docs.astrbot.app/zh/use/subagent.html#委派工具参数)和[多模态图片传递](https://docs.astrbot.app/zh/use/subagent.html#多模态图片传递)章节。
Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
![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 enhances the subagent handoff mechanism by enforcing a stricter argument schema, adding validation for input and background_task parameters, and implementing a fallback for missing provider IDs. These changes improve the robustness and reliability of agent handoffs. The new helper methods for parsing and validation in astrbot/core/astr_agent_tool_exec.py are well-structured. The accompanying tests cover the new logic, though I've suggested an improvement to increase test coverage for _parse_background_task_arg by using parameterization.
Comment on lines
+348
to
+367
| def test_parse_background_task_arg_rejects_invalid_value(): | ||
| is_bg, error = FunctionToolExecutor._parse_background_task_arg( | ||
| "transfer_to_subagent", | ||
| "not-a-bool", | ||
| ) | ||
|
|
||
| assert is_bg is False | ||
| assert error is not None | ||
| assert isinstance(error, mcp.types.CallToolResult) | ||
| assert "invalid_background_task" in error.content[0].text | ||
|
|
||
|
|
||
| def test_parse_background_task_arg_accepts_string_true(): | ||
| is_bg, error = FunctionToolExecutor._parse_background_task_arg( | ||
| "transfer_to_subagent", | ||
| "true", | ||
| ) | ||
|
|
||
| assert is_bg is True | ||
| assert error is None |
Contributor
There was a problem hiding this comment.
These two tests for _parse_background_task_arg can be consolidated into a single, more comprehensive parameterized test using pytest.mark.parametrize. This would improve test coverage by including all valid string representations for True and False (e.g., '1', 'yes', 'on', '0', 'no', 'off'), as well as various invalid cases, making the test suite more robust and easier to maintain.
@pytest.mark.parametrize(
("value", "expected_bool", "expect_error"),
[
# Valid True values
(True, True, False),
("true", True, False),
("1", True, False),
("yes", True, False),
("on", True, False),
(" TRUE ", True, False),
# Valid False values
(False, False, False),
("false", False, False),
("0", False, False),
("no", False, False),
("off", False, False),
("", False, False),
(" FALSE ", False, False),
(None, False, False),
# Invalid values
("not-a-bool", False, True),
("y", False, True),
("t", False, True),
(123, False, True),
({}, False, True),
],
)
def test_parse_background_task_arg(value, expected_bool, expect_error):
is_bg, error = FunctionToolExecutor._parse_background_task_arg(
"transfer_to_subagent",
value,
)
assert is_bg is expected_bool
if expect_error:
assert error is not None
assert isinstance(error, mcp.types.CallToolResult)
assert "invalid_background_task" in error.content[0].text
else:
assert error is None
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_resolve_handoff_provider_id, whenprovider_manageris missing or does not exposeget_provider_by_id, you currently return the configuredprovider_idas-is, which can still be stale; consider falling back toctx.get_current_chat_provider_id(umo)in that branch to avoid reintroducing silent misrouting in environments without a provider manager. - _build_handoff_error_result
embeds a concrete example payload string (withbackground_task: falseand onlyinput/background_task` fields); to avoid future drift if the handoff schema or typical usage changes, consider constructing this example dynamically from the current schema or at least centralizing the example in a single constant.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_resolve_handoff_provider_id`, when `provider_manager` is missing or does not expose `get_provider_by_id`, you currently return the configured `provider_id` as-is, which can still be stale; consider falling back to `ctx.get_current_chat_provider_id(umo)` in that branch to avoid reintroducing silent misrouting in environments without a provider manager.
- _build_handoff_error_result` embeds a concrete example payload string (with `background_task: false` and only `input`/`background_task` fields); to avoid future drift if the handoff schema or typical usage changes, consider constructing this example dynamically from the current schema or at least centralizing the example in a single constant.
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/handoff.py" line_range="42-43" />
<code_context>
def default_parameters(self) -> dict:
return {
"type": "object",
+ "required": ["input"],
+ "additionalProperties": False,
"properties": {
"input": {
</code_context>
<issue_to_address>
**issue (bug_risk):** Reconsider `additionalProperties: False` given runtime use of extra arguments like `background_task`.
With `additionalProperties: False`, any parameters not declared in `properties` will now be rejected, while the executor still expects fields like `background_task` (and possibly `image_urls`). This mismatch can break existing clients that send these fields. Either add all such fields to `properties` or keep `additionalProperties` relaxed until the schema and runtime behavior are aligned.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- _build_handoff_error_result hardcodes a background_task example in the guidance text even for non-background_task errors (e.g., missing_or_empty_input); consider parameterizing or making the example match the specific error type so the agent receives clearer, less confusing retry instructions.
- In _resolve_handoff_provider_id, if provider_manager is missing or lacks get_provider_by_id, the code returns the configured provider_id without checking it exists; you might want to fall back to ctx.get_current_chat_provider_id in this case as well to avoid silently using a stale provider ID.
- There is some duplication between the background_task parsing tests in test_astr_agent_tool_exec.py and the new parametrized tests in test_handoff_background_task_arg.py; consider consolidating into the parametrized suite to keep coverage while reducing redundancy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- _build_handoff_error_result hardcodes a background_task example in the guidance text even for non-background_task errors (e.g., missing_or_empty_input); consider parameterizing or making the example match the specific error type so the agent receives clearer, less confusing retry instructions.
- In _resolve_handoff_provider_id, if provider_manager is missing or lacks get_provider_by_id, the code returns the configured provider_id without checking it exists; you might want to fall back to ctx.get_current_chat_provider_id in this case as well to avoid silently using a stale provider ID.
- There is some duplication between the background_task parsing tests in test_astr_agent_tool_exec.py and the new parametrized tests in test_handoff_background_task_arg.py; consider consolidating into the parametrized suite to keep coverage while reducing redundancy.
## Individual Comments
### Comment 1
<location path="astrbot/core/astr_agent_tool_exec.py" line_range="86-93" />
<code_context>
+ return False, None
+ if isinstance(value, bool):
+ return value, None
+ if isinstance(value, str):
+ normalized = value.strip().lower()
+ if normalized in {"true", "1", "yes", "on"}:
+ return True, None
+ if normalized in {"false", "0", "no", "off", ""}:
+ return False, None
+
+ return False, cls._build_handoff_error_result(
+ tool_name=tool_name,
+ error_type="invalid_background_task",
</code_context>
<issue_to_address>
**suggestion:** Consider either supporting integer 0/1 for `background_task` or tightening the error message to avoid implying they are accepted.
Right now, `background_task` only accepts booleans or specific *string* values; numeric 0/1 are rejected even though the hint suggests broader equivalence. This can be confusing for callers whose JSON booleans end up as 0/1. Either broaden parsing to accept `int` 0/1 or update the hint to clearly state that only booleans and string equivalents are valid.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_astr_agent_tool_exec.py" line_range="348-357" />
<code_context>
assert image_urls == []
+
+
+def test_parse_background_task_arg_rejects_invalid_value():
+ is_bg, error = FunctionToolExecutor._parse_background_task_arg(
+ "transfer_to_subagent",
+ "not-a-bool",
+ )
+
+ assert is_bg is False
+ assert error is not None
+ assert isinstance(error, mcp.types.CallToolResult)
+ assert "invalid_background_task" in error.content[0].text
+
+
+def test_parse_background_task_arg_accepts_string_true():
+ is_bg, error = FunctionToolExecutor._parse_background_task_arg(
+ "transfer_to_subagent",
+ "true",
+ )
+
+ assert is_bg is True
+ assert error is None
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Background task parsing is tested twice in two files; consider consolidating to the parametrized suite to avoid duplication and future drift.
These tests duplicate the existing parametrized coverage in `tests/unit/test_handoff_background_task_arg.py` and risk drifting out of sync if behavior changes. Consider either removing them in favor of the parametrized suite or moving all `_parse_background_task_arg` tests into a single dedicated file as the source of truth.
Suggested implementation:
```python
)
assert image_urls == []
```
If there are additional `_parse_background_task_arg` tests further down in this file (e.g., for `"true"` or other values), they should also be removed from `test_astr_agent_tool_exec.py` and kept only in `tests/unit/test_handoff_background_task_arg.py` to maintain a single, consolidated test suite for this behavior.
</issue_to_address>
### Comment 3
<location path="tests/unit/test_handoff_background_task_arg.py" line_range="7-16" />
<code_context>
+@pytest.mark.parametrize(
</code_context>
<issue_to_address>
**suggestion (testing):** Background task parsing is well-covered at the helper level; consider adding an integration-level test for invalid `background_task` via `execute` to ensure the early-yield behavior is preserved.
The current parametrized test covers `_parse_background_task_arg`, but not how `FunctionToolExecutor.execute` behaves when `background_task` is invalid. Please add a small async test that calls `execute` (or `_execute_handoff`, if that’s the public entry) with an invalid `background_task` and asserts that it yields a single `CallToolResult` error and returns without invoking the handoff or any downstream calls.
Suggested implementation:
```python
import mcp
import pytest
from unittest.mock import AsyncMock
from astrbot.core.astr_agent_tool_exec import FunctionToolExecutor
```
```python
from astrbot.core.astr_agent_tool_exec import FunctionToolExecutor
@pytest.mark.asyncio
async def test_execute_invalid_background_task_early_error(monkeypatch):
"""Integration-level test: invalid background_task should yield a single error and not hand off."""
# Arrange: create an executor instance
executor = FunctionToolExecutor()
# If execute delegates into a separate handoff method, guard it so the test fails if it is invoked.
# Adjust the attribute name if the handoff entrypoint differs (e.g. `handoff`, `_handoff`, etc.).
if hasattr(executor, "_execute_handoff"):
monkeypatch.setattr(
executor,
"_execute_handoff",
AsyncMock(side_effect=AssertionError("_execute_handoff should not be called for invalid background_task")),
)
# Act: call execute with an invalid background_task value that should fail parsing
results = [
result async for result in executor.execute(
tool_name="some_tool",
arguments={},
background_task="not-a-bool", # intentionally invalid
)
]
# Assert: exactly one CallToolResult is yielded, it is an error, and no downstream calls are made.
assert len(results) == 1
assert isinstance(results[0], mcp.CallToolResult)
assert getattr(results[0], "is_error", True), "Expected the single CallToolResult to represent an error"
@pytest.mark.parametrize(
```
The new integration-level test assumes:
1. `FunctionToolExecutor()` can be constructed without arguments. If the real initializer requires parameters, update the test's `executor = FunctionToolExecutor()` line to pass appropriate arguments or use a helper/factory already used elsewhere in your tests.
2. `FunctionToolExecutor.execute` is an `async` generator that accepts `tool_name`, `arguments`, and `background_task` keyword arguments. If the signature differs, align the call in the test with the actual method signature.
3. The early-yield error path does not invoke `_execute_handoff`. If the actual handoff entrypoint has a different name (e.g., `handoff`, `_handoff`, `_execute_tool_handoff`), update the `hasattr` / `monkeypatch.setattr` to reference the correct attribute, or remove that block if there is no such method.
4. `mcp.CallToolResult` has an `is_error` attribute or similar. If the error indication is expressed differently (e.g., `result.error is not None`, `result.type == "error"`, etc.), adjust the final assertion to match your concrete API.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+348
to
+357
| def test_parse_background_task_arg_rejects_invalid_value(): | ||
| is_bg, error = FunctionToolExecutor._parse_background_task_arg( | ||
| "transfer_to_subagent", | ||
| "not-a-bool", | ||
| ) | ||
|
|
||
| assert is_bg is False | ||
| assert error is not None | ||
| assert isinstance(error, mcp.types.CallToolResult) | ||
| assert "invalid_background_task" in error.content[0].text |
Contributor
There was a problem hiding this comment.
suggestion (testing): Background task parsing is tested twice in two files; consider consolidating to the parametrized suite to avoid duplication and future drift.
These tests duplicate the existing parametrized coverage in tests/unit/test_handoff_background_task_arg.py and risk drifting out of sync if behavior changes. Consider either removing them in favor of the parametrized suite or moving all _parse_background_task_arg tests into a single dedicated file as the source of truth.
Suggested implementation:
)
assert image_urls == []If there are additional _parse_background_task_arg tests further down in this file (e.g., for "true" or other values), they should also be removed from test_astr_agent_tool_exec.py and kept only in tests/unit/test_handoff_background_task_arg.py to maintain a single, consolidated test suite for this behavior.
Comment on lines
+7
to
+16
| @pytest.mark.parametrize( | ||
| ("value", "expected_bool", "expect_error"), | ||
| [ | ||
| (True, True, False), | ||
| ("true", True, False), | ||
| ("1", True, False), | ||
| ("yes", True, False), | ||
| ("on", True, False), | ||
| (" TRUE ", True, False), | ||
| (False, False, False), |
Contributor
There was a problem hiding this comment.
suggestion (testing): Background task parsing is well-covered at the helper level; consider adding an integration-level test for invalid background_task via execute to ensure the early-yield behavior is preserved.
The current parametrized test covers _parse_background_task_arg, but not how FunctionToolExecutor.execute behaves when background_task is invalid. Please add a small async test that calls execute (or _execute_handoff, if that’s the public entry) with an invalid background_task and asserts that it yields a single CallToolResult error and returns without invoking the handoff or any downstream calls.
Suggested implementation:
import mcp
import pytest
from unittest.mock import AsyncMock
from astrbot.core.astr_agent_tool_exec import FunctionToolExecutorfrom astrbot.core.astr_agent_tool_exec import FunctionToolExecutor
@pytest.mark.asyncio
async def test_execute_invalid_background_task_early_error(monkeypatch):
"""Integration-level test: invalid background_task should yield a single error and not hand off."""
# Arrange: create an executor instance
executor = FunctionToolExecutor()
# If execute delegates into a separate handoff method, guard it so the test fails if it is invoked.
# Adjust the attribute name if the handoff entrypoint differs (e.g. `handoff`, `_handoff`, etc.).
if hasattr(executor, "_execute_handoff"):
monkeypatch.setattr(
executor,
"_execute_handoff",
AsyncMock(side_effect=AssertionError("_execute_handoff should not be called for invalid background_task")),
)
# Act: call execute with an invalid background_task value that should fail parsing
results = [
result async for result in executor.execute(
tool_name="some_tool",
arguments={},
background_task="not-a-bool", # intentionally invalid
)
]
# Assert: exactly one CallToolResult is yielded, it is an error, and no downstream calls are made.
assert len(results) == 1
assert isinstance(results[0], mcp.CallToolResult)
assert getattr(results[0], "is_error", True), "Expected the single CallToolResult to represent an error"
@pytest.mark.parametrize(The new integration-level test assumes:
FunctionToolExecutor()can be constructed without arguments. If the real initializer requires parameters, update the test'sexecutor = FunctionToolExecutor()line to pass appropriate arguments or use a helper/factory already used elsewhere in your tests.FunctionToolExecutor.executeis anasyncgenerator that acceptstool_name,arguments, andbackground_taskkeyword arguments. If the signature differs, align the call in the test with the actual method signature.- The early-yield error path does not invoke
_execute_handoff. If the actual handoff entrypoint has a different name (e.g.,handoff,_handoff,_execute_tool_handoff), update thehasattr/monkeypatch.setattrto reference the correct attribute, or remove that block if there is no such method. mcp.CallToolResulthas anis_errorattribute or similar. If the error indication is expressed differently (e.g.,result.error is not None,result.type == "error", etc.), adjust the final assertion to match your concrete API.
SourceryAI
reviewed
Mar 24, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_parse_background_task_argbehavior is covered both intest_astr_agent_tool_exec.py(two focused tests) andtest_handoff_background_task_arg.py(parametrized suite); consider consolidating these into a single location to avoid duplicated coverage and future drift. - Given that
_build_handoff_error_resultis now the central path for handoff validation failures, consider including a lightweight machine-readable field (e.g., a JSON snippet orerror_type:line that can be reliably parsed) in addition to the human-readable guidance to make it easier for callers to programmatically react to different failure types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_parse_background_task_arg` behavior is covered both in `test_astr_agent_tool_exec.py` (two focused tests) and `test_handoff_background_task_arg.py` (parametrized suite); consider consolidating these into a single location to avoid duplicated coverage and future drift.
- Given that `_build_handoff_error_result` is now the central path for handoff validation failures, consider including a lightweight machine-readable field (e.g., a JSON snippet or `error_type:` line that can be reliably parsed) in addition to the human-readable guidance to make it easier for callers to programmatically react to different failure types.
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/handoff.py" line_range="42-43" />
<code_context>
def default_parameters(self) -> dict:
return {
"type": "object",
+ "required": ["input"],
+ "additionalProperties": False,
"properties": {
"input": {
</code_context>
<issue_to_address>
**issue (bug_risk):** Making `input` required and disallowing additional properties conflicts with the `background_task` argument handling.
With `input` required and `additionalProperties=False`, any `background_task` key in `tool_args` is now invalid JSON Schema, even though `FunctionToolExecutor.execute` still reads it and `_build_handoff_error_result` shows it in the example. Either add `background_task` (with the correct type) to `properties` or relax `additionalProperties` to allow it; if it’s no longer supported, remove it from both the error example and the `tool_args` handling to keep schema and behavior aligned.
</issue_to_address>Hi @Jacobinwwey! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+42
to
+43
| "required": ["input"], | ||
| "additionalProperties": False, |
There was a problem hiding this comment.
issue (bug_risk): Making input required and disallowing additional properties conflicts with the background_task argument handling.
With input required and additionalProperties=False, any background_task key in tool_args is now invalid JSON Schema, even though FunctionToolExecutor.execute still reads it and _build_handoff_error_result shows it in the example. Either add background_task (with the correct type) to properties or relax additionalProperties to allow it; if it’s no longer supported, remove it from both the error example and the tool_args handling to keep schema and behavior aligned.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- With
default_parametersnow settingadditionalProperties: Falsewhile onlyinputis declared inproperties, any existing/expected handoff args likebackground_task,image_urls, etc. will be rejected at the schema level; consider explicitly listing all supported handoff parameters inpropertiesso they remain usable. - The handoff error responses embed
error_type,fix, andactiononly inside a free-form text blob; if downstream tooling needs to react programmatically, consider returning these as separate structured fields (e.g., in a JSON payload or metadata) to make automated handling easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- With `default_parameters` now setting `additionalProperties: False` while only `input` is declared in `properties`, any existing/expected handoff args like `background_task`, `image_urls`, etc. will be rejected at the schema level; consider explicitly listing all supported handoff parameters in `properties` so they remain usable.
- The handoff error responses embed `error_type`, `fix`, and `action` only inside a free-form text blob; if downstream tooling needs to react programmatically, consider returning these as separate structured fields (e.g., in a JSON payload or metadata) to make automated handling easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
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_handoff_background_task_arg.py" line_range="12-21" />
<code_context>
+from astrbot.core.astr_agent_tool_exec import FunctionToolExecutor
+
+
+@pytest.mark.parametrize(
+ ("value", "expected_bool", "expect_error"),
+ [
+ (True, True, False),
+ ("true", True, False),
+ ("1", True, False),
+ ("yes", True, False),
+ ("on", True, False),
+ (" TRUE ", True, False),
+ (False, False, False),
+ ("false", False, False),
+ ("0", False, False),
+ ("no", False, False),
+ ("off", False, False),
+ ("", False, False),
+ (" FALSE ", False, False),
+ (None, False, False),
+ ("not-a-bool", False, True),
+ ("y", False, True),
+ ("t", False, True),
+ (123, False, True),
+ ({}, False, True),
+ ],
+)
+def test_parse_background_task_arg(value, expected_bool, expect_error):
+ is_bg, error = FunctionToolExecutor._parse_background_task_arg(
+ "transfer_to_subagent",
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding dedicated tests for `_parse_background_task_arg` integration via `execute` with valid values
The parameterized helper tests cover value parsing well, but `FunctionToolExecutor.execute` is only exercised for the invalid string case. Please add `execute`-level tests where:
- A truthy background value (e.g. `True` or "true") results in exactly one `_execute_handoff_background` call, zero `_execute_handoff` calls.
- A falsey background value (e.g. `False`, "false", or "0") results in exactly one `_execute_handoff` call, zero `_execute_handoff_background` calls.
These can follow the pattern of `test_execute_invalid_background_task_early_error`, using monkeypatching and `call_count` assertions, and verifying that no error `CallToolResult` is returned.
Suggested implementation:
```python
if expect_error:
assert error is not None
assert isinstance(error, mcp.types.CallToolResult)
text_content = error.content[0]
assert isinstance(text_content, mcp.types.TextContent)
assert "invalid_background_task" in text_content.text
@pytest.mark.parametrize("background_value", [True, "true"])
def test_execute_truthy_background_uses_background_handoff(monkeypatch, background_value):
handoff_call_count = 0
background_call_count = 0
def fake_execute_handoff(*args, **kwargs):
nonlocal handoff_call_count
handoff_call_count += 1
# Return a non-error CallToolResult shape; adjust as needed for the real API.
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="foreground handoff")],
)
def fake_execute_handoff_background(*args, **kwargs):
nonlocal background_call_count
background_call_count += 1
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="background handoff")],
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff",
fake_execute_handoff,
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff_background",
fake_execute_handoff_background,
)
# Construct minimal arguments following the pattern of other tests in this module.
# These may need to be adjusted to match the actual FunctionToolExecutor.execute signature.
agent = Agent()
tool = HandoffTool(name="transfer_to_subagent")
context = ContextWrapper(agent=agent)
arguments = {"background_task": background_value}
result = FunctionToolExecutor.execute(
agent=agent,
tool=tool,
context=context,
arguments=arguments,
)
assert handoff_call_count == 0
assert background_call_count == 1
# Ensure no "invalid_background_task" error CallToolResult was returned.
results = result if isinstance(result, (list, tuple)) else [result]
for res in results:
if isinstance(res, mcp.types.CallToolResult) and res.content:
first = res.content[0]
if isinstance(first, mcp.types.TextContent):
assert "invalid_background_task" not in first.text
@pytest.mark.parametrize("background_value", [False, "false", "0"])
def test_execute_falsey_background_uses_foreground_handoff(monkeypatch, background_value):
handoff_call_count = 0
background_call_count = 0
def fake_execute_handoff(*args, **kwargs):
nonlocal handoff_call_count
handoff_call_count += 1
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="foreground handoff")],
)
def fake_execute_handoff_background(*args, **kwargs):
nonlocal background_call_count
background_call_count += 1
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="background handoff")],
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff",
fake_execute_handoff,
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff_background",
fake_execute_handoff_background,
)
agent = Agent()
tool = HandoffTool(name="transfer_to_subagent")
context = ContextWrapper(agent=agent)
arguments = {"background_task": background_value}
result = FunctionToolExecutor.execute(
agent=agent,
tool=tool,
context=context,
arguments=arguments,
)
assert handoff_call_count == 1
assert background_call_count == 0
results = result if isinstance(result, (list, tuple)) else [result]
for res in results:
if isinstance(res, mcp.types.CallToolResult) and res.content:
first = res.content[0]
if isinstance(first, mcp.types.TextContent):
assert "invalid_background_task" not in first.text
```
The new tests assume:
1. `FunctionToolExecutor.execute` has a signature like:
`execute(agent: Agent, tool: HandoffTool, context: ContextWrapper, arguments: dict)`.
2. `Agent`, `HandoffTool`, and `ContextWrapper` can be constructed with the minimal arguments shown.
To integrate with the actual codebase, you may need to:
1. Adjust the construction of `agent`, `tool`, and `context` to match their real constructors, or switch to using existing fixtures/helpers already used in `test_execute_invalid_background_task_early_error`.
2. Update the `execute` call to match the real parameter names and ordering.
3. If the real non-error `CallToolResult` shape differs, tweak the fake implementations of `_execute_handoff` and `_execute_handoff_background` to return whatever the production code expects.
4. If the production error detection differs from checking for `"invalid_background_task"` in `TextContent`, align the “no error” assertions with the pattern already used in `test_execute_invalid_background_task_early_error`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+12
to
+21
| @pytest.mark.parametrize( | ||
| ("value", "expected_bool", "expect_error"), | ||
| [ | ||
| (True, True, False), | ||
| ("true", True, False), | ||
| ("1", True, False), | ||
| ("yes", True, False), | ||
| ("on", True, False), | ||
| (" TRUE ", True, False), | ||
| (False, False, False), |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding dedicated tests for _parse_background_task_arg integration via execute with valid values
The parameterized helper tests cover value parsing well, but FunctionToolExecutor.execute is only exercised for the invalid string case. Please add execute-level tests where:
- A truthy background value (e.g.
Trueor "true") results in exactly one_execute_handoff_backgroundcall, zero_execute_handoffcalls. - A falsey background value (e.g.
False, "false", or "0") results in exactly one_execute_handoffcall, zero_execute_handoff_backgroundcalls.
These can follow the pattern of test_execute_invalid_background_task_early_error, using monkeypatching and call_count assertions, and verifying that no error CallToolResult is returned.
Suggested implementation:
if expect_error:
assert error is not None
assert isinstance(error, mcp.types.CallToolResult)
text_content = error.content[0]
assert isinstance(text_content, mcp.types.TextContent)
assert "invalid_background_task" in text_content.text
@pytest.mark.parametrize("background_value", [True, "true"])
def test_execute_truthy_background_uses_background_handoff(monkeypatch, background_value):
handoff_call_count = 0
background_call_count = 0
def fake_execute_handoff(*args, **kwargs):
nonlocal handoff_call_count
handoff_call_count += 1
# Return a non-error CallToolResult shape; adjust as needed for the real API.
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="foreground handoff")],
)
def fake_execute_handoff_background(*args, **kwargs):
nonlocal background_call_count
background_call_count += 1
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="background handoff")],
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff",
fake_execute_handoff,
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff_background",
fake_execute_handoff_background,
)
# Construct minimal arguments following the pattern of other tests in this module.
# These may need to be adjusted to match the actual FunctionToolExecutor.execute signature.
agent = Agent()
tool = HandoffTool(name="transfer_to_subagent")
context = ContextWrapper(agent=agent)
arguments = {"background_task": background_value}
result = FunctionToolExecutor.execute(
agent=agent,
tool=tool,
context=context,
arguments=arguments,
)
assert handoff_call_count == 0
assert background_call_count == 1
# Ensure no "invalid_background_task" error CallToolResult was returned.
results = result if isinstance(result, (list, tuple)) else [result]
for res in results:
if isinstance(res, mcp.types.CallToolResult) and res.content:
first = res.content[0]
if isinstance(first, mcp.types.TextContent):
assert "invalid_background_task" not in first.text
@pytest.mark.parametrize("background_value", [False, "false", "0"])
def test_execute_falsey_background_uses_foreground_handoff(monkeypatch, background_value):
handoff_call_count = 0
background_call_count = 0
def fake_execute_handoff(*args, **kwargs):
nonlocal handoff_call_count
handoff_call_count += 1
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="foreground handoff")],
)
def fake_execute_handoff_background(*args, **kwargs):
nonlocal background_call_count
background_call_count += 1
return mcp.types.CallToolResult(
content=[mcp.types.TextContent(type="text", text="background handoff")],
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff",
fake_execute_handoff,
)
monkeypatch.setattr(
FunctionToolExecutor,
"_execute_handoff_background",
fake_execute_handoff_background,
)
agent = Agent()
tool = HandoffTool(name="transfer_to_subagent")
context = ContextWrapper(agent=agent)
arguments = {"background_task": background_value}
result = FunctionToolExecutor.execute(
agent=agent,
tool=tool,
context=context,
arguments=arguments,
)
assert handoff_call_count == 1
assert background_call_count == 0
results = result if isinstance(result, (list, tuple)) else [result]
for res in results:
if isinstance(res, mcp.types.CallToolResult) and res.content:
first = res.content[0]
if isinstance(first, mcp.types.TextContent):
assert "invalid_background_task" not in first.textThe new tests assume:
FunctionToolExecutor.executehas a signature like:
execute(agent: Agent, tool: HandoffTool, context: ContextWrapper, arguments: dict).Agent,HandoffTool, andContextWrappercan be constructed with the minimal arguments shown.
To integrate with the actual codebase, you may need to:
- Adjust the construction of
agent,tool, andcontextto match their real constructors, or switch to using existing fixtures/helpers already used intest_execute_invalid_background_task_early_error. - Update the
executecall to match the real parameter names and ordering. - If the real non-error
CallToolResultshape differs, tweak the fake implementations of_execute_handoffand_execute_handoff_backgroundto return whatever the production code expects. - If the production error detection differs from checking for
"invalid_background_task"inTextContent, align the “no error” assertions with the pattern already used intest_execute_invalid_background_task_early_error.
SourceryAI
reviewed
Mar 24, 2026
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
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_astr_agent_tool_exec.py" line_range="348-357" />
<code_context>
+@pytest.mark.asyncio
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen empty-input test by asserting the handoff agent is never invoked
To fully validate the early-rejection behavior, also assert that `_fake_tool_loop_agent` is never invoked (e.g., via a flag or counter). This will confirm the error is raised before any downstream handoff execution occurs for empty input.
Suggested implementation:
```python
@pytest.mark.asyncio
async def test_execute_handoff_rejects_empty_input():
tool_loop_called = False
async def _fake_get_current_chat_provider_id(_umo):
return "provider-id"
async def _fake_tool_loop_agent(**_kwargs):
nonlocal tool_loop_called
tool_loop_called = True
return SimpleNamespace(completion_text="ok")
context = SimpleNamespace(
get_current_chat_provider_id=_fake_get_current_chat_provider_id,
tool_loop_agent=_fake_tool_loop_agent,
```
To fully implement the strengthened test, you should also:
1. After the code that triggers the empty-input rejection (likely the call to the handoff execution function and the corresponding `pytest.raises` block), add:
`assert tool_loop_called is False`
2. Ensure this assertion is placed at the end of `test_execute_handoff_rejects_empty_input`, after all other assertions that verify the rejection behavior.
</issue_to_address>Hi @Jacobinwwey! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+348
to
+357
| @pytest.mark.asyncio | ||
| async def test_execute_handoff_rejects_empty_input(): | ||
| async def _fake_get_current_chat_provider_id(_umo): | ||
| return "provider-id" | ||
|
|
||
| async def _fake_tool_loop_agent(**_kwargs): | ||
| return SimpleNamespace(completion_text="ok") | ||
|
|
||
| context = SimpleNamespace( | ||
| get_current_chat_provider_id=_fake_get_current_chat_provider_id, |
There was a problem hiding this comment.
suggestion (testing): Strengthen empty-input test by asserting the handoff agent is never invoked
To fully validate the early-rejection behavior, also assert that _fake_tool_loop_agent is never invoked (e.g., via a flag or counter). This will confirm the error is raised before any downstream handoff execution occurs for empty input.
Suggested implementation:
@pytest.mark.asyncio
async def test_execute_handoff_rejects_empty_input():
tool_loop_called = False
async def _fake_get_current_chat_provider_id(_umo):
return "provider-id"
async def _fake_tool_loop_agent(**_kwargs):
nonlocal tool_loop_called
tool_loop_called = True
return SimpleNamespace(completion_text="ok")
context = SimpleNamespace(
get_current_chat_provider_id=_fake_get_current_chat_provider_id,
tool_loop_agent=_fake_tool_loop_agent,To fully implement the strengthened test, you should also:
- After the code that triggers the empty-input rejection (likely the call to the handoff execution function and the corresponding
pytest.raisesblock), add:
assert tool_loop_called is False - Ensure this assertion is placed at the end of
test_execute_handoff_rejects_empty_input, after all other assertions that verify the rejection behavior.
Author
|
@sourcery-ai review |
Author
|
[Passed the review by Sourcery] |
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
3 participants
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.
Motivation / 动机
当前
transfer_to_*handoff 参数校验和 provider 选择在边界场景下仍可能产生不透明失败：background_task输入不规范时，主代理缺少稳定的自纠错指引；input为空时可能进入无效委派路径；provider_id时会导致 handoff 失败而非平滑降级。本 PR 统一 handoff 参数契约并补充 provider fallback，提升可恢复性与可观测性。
Complement With #5722 / 与 #5722 的互补关系
Modifications / 改动点
Handoff Contract / 参数契约
astrbot/core/agent/handoff.pyrequired: ["input"]additionalProperties: falsebackground_task与image_urls属性，保证 schema 与执行器行为一致Validation & Fallback / 校验与回退
astrbot/core/astr_agent_tool_exec.pybackground_task解析：支持bool与常见字符串布尔值；非法值返回结构化错误并给出重试指引input非空校验：空字符串/空白字符串立即拒绝provider_id不存在时，回退到当前会话 provider 并记录 warningTests / 测试覆盖
tests/unit/test_astr_agent_tool_exec.pyinput拒绝、缺失 provider 的 fallback 等关键路径tests/unit/test_handoff_background_task_arg.py_parse_background_task_arg的真值/假值/非法值execute层路由：truthy -> background handoff、falsey -> foreground handoffbackground_task早返回，不触发下游 handoffbackground_task在 properties 中）This is NOT a breaking change. / 这不是一个破坏性变更。
Test Results / 测试结果
本轮在当前环境执行：
结果：
py_compile: passed说明：
deprecated依赖，pytest在加载tests/conftest.py时失败（ModuleNotFoundError: deprecated），未在该环境完成完整单测回归。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.