fix(agent): 抑制工具调用轮次的预置文本外发#7902
Open
Sisyphbaous-DT-Project wants to merge 2 commits intoAstrBotDevs:masterfrom
Open
fix(agent): 抑制工具调用轮次的预置文本外发#7902Sisyphbaous-DT-Project wants to merge 2 commits intoAstrBotDevs:masterfrom
Sisyphbaous-DT-Project wants to merge 2 commits intoAstrBotDevs:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The repeated
(not has_tool_calls)checks before eachllm_resultyield block make the flow harder to read; consider wrapping the whole reasoning/result_chain/completion_text emission in a singleif not has_tool_calls:block to simplify the control structure. - Right now
has_tool_callsis derived solely fromllm_resp.tools_call_name; if other tool-related fields (e.g.tools_call_args/tools_call_ids) can be populated independently, it may be safer to centralize this into a singleLLMResponseproperty or helper that consistently defines "has tool calls" across the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated `(not has_tool_calls)` checks before each `llm_result` yield block make the flow harder to read; consider wrapping the whole reasoning/result_chain/completion_text emission in a single `if not has_tool_calls:` block to simplify the control structure.
- Right now `has_tool_calls` is derived solely from `llm_resp.tools_call_name`; if other tool-related fields (e.g. `tools_call_args`/`tools_call_ids`) can be populated independently, it may be safer to centralize this into a single `LLMResponse` property or helper that consistently defines "has tool calls" across the codebase.
## Individual Comments
### Comment 1
<location path="tests/test_tool_loop_agent_runner.py" line_range="263-272" />
<code_context>
)
+class PreToolTextThenFinalProvider(MockProvider):
+ def __init__(self, pre_tool_text: str):
+ super().__init__()
+ self.pre_tool_text = pre_tool_text
+
+ async def text_chat(self, **kwargs) -> LLMResponse:
+ self.call_count += 1
+ func_tool = kwargs.get("func_tool")
+ if func_tool is None or self.call_count > 1:
+ return LLMResponse(
+ role="assistant",
+ completion_text="final answer",
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that the tool handler is invoked to fully validate the tool-call branch.
Since the `PreToolTextThenFinalProvider` and new test already cover the `tools_call_name` + `completion_text` path, it would be valuable for at least one test to assert that the tool `handler` is actually awaited. For example, keep using an `AsyncMock` and add `tool.handler.assert_awaited_once()` (or check `await_count == 1`) so the test verifies the tool-call turn is executed, not just that pre-tool text is suppressed.
Suggested implementation:
```python
return LLMResponse(
role="assistant",
completion_text=self.pre_tool_text,
tools_call_name=["test_tool"],
tools_call_args=[{"query": "test"}],
tools_call_ids=["call_pre_tool"],
usage=TokenUsage(input_other=10, output=5),
)
async def test_tool_loop_agent_runner_awaits_tool_handler_for_pre_tool_text_then_final(
tool_loop_agent_runner: ToolLoopAgentRunner,
) -> None:
"""Ensure the tool handler is actually awaited when the LLM returns pre-tool text plus a tool call."""
from unittest.mock import AsyncMock
# Arrange
handler = AsyncMock(return_value="tool result")
tool = Tool( # Adjust to actual Tool/ToolSpec type used in this test module
name="test_tool",
description="test tool",
handler=handler,
)
provider = PreToolTextThenFinalProvider(pre_tool_text="pre-tool explanation")
# Act
result = await tool_loop_agent_runner.run(
query="test",
tools=[tool],
provider=provider,
)
# Assert: final answer is produced and the tool handler was awaited exactly once
assert "final answer" in result.response_text
handler.assert_awaited_once()
```
To make this compile and match your existing conventions, you will likely need to:
1. **Imports / types**
- Ensure `ToolLoopAgentRunner` and `Tool` (or your equivalent tool spec type) are imported or available in this file.
- If this file already imports them at the top, you can remove the inline `from unittest.mock import AsyncMock` and rely on any existing `AsyncMock` import instead.
2. **Runner API alignment**
- Adapt the `tool_loop_agent_runner.run(...)` call to the actual API in your codebase.
- Replace `query="test"` with the correct argument name if different (e.g. `question`, `prompt`, etc.).
- Replace `tools=[tool]` and `provider=provider` with the correct parameter names (e.g. `available_tools`, `llm_provider`, etc.).
3. **Result assertion**
- Adjust `result.response_text` to match the actual attribute name used for the final answer in your agent runner (e.g. `result.output_text`, `result.completion_text`, or `result.answer`).
4. **Tool construction**
- If your tooling abstraction uses a different constructor or fields (e.g. `ToolSpec(name=..., description=..., func=handler)`), modify the `Tool(...)` instantiation accordingly while keeping `handler` attached so that `handler.assert_awaited_once()` remains valid.
This test should then assert that:
- The agent completes with the "final answer" response, and
- The tool's async handler was awaited exactly once, thereby validating the tool-call branch including suppression of the pre-tool text.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Code Review
This pull request modifies the tool loop agent runner to suppress LLM text outputs, such as reasoning and completion text, when tool calls are present. It also updates system prompts to allow tool-only responses and discourage placeholder text. Feedback suggests that reasoning content should still be displayed to provide user context even during tool calls, and that redundant conditional checks should be refactored for better maintainability.
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
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.
Fixes #7901
本 PR 修复了工具调用轮次中误发
*No response*等无意义文本的问题。在 OpenAI-compatible provider 的返回中,部分模型会在同一个 assistant 响应里同时包含
content与tool_calls。此前
ToolLoopAgentRunner.step()在识别到工具调用后,仍会先把content或result_chain作为普通llm_result发送,再进入工具执行分支。这会导致
*No response*、I will check this first.等 pre-tool 文本提前发给用户,形成噪声消息。本次修复将 tool-call turn 视为中间执行轮次处理。
当
llm_resp.tools_call_name非空时,不再外发普通llm_result,而是直接进入工具调用流程。工具执行后的最终回答仍会按原流程正常发送。
Modifications / 改动点
修改
astrbot/core/agent/runners/tool_loop_agent_runner.py。新增
has_tool_calls = bool(llm_resp.tools_call_name)判断。当本轮包含工具调用时,不再
yield普通llm_result。保持工具执行、tool result 写入上下文、最终 assistant 回复流程不变。
不影响 skills-like re-query 失败后转普通回复的 fallback 路径。
修改
astrbot/core/astr_main_agent_resources.py。调整工具调用相关 prompt 语义,允许在不需要用户可见文本时只返回 tool calls。
明确禁止输出
"No response"一类占位文本。将“调用工具前必须说明”调整为“仅在有帮助时说明”。
修改
tests/test_tool_loop_agent_runner.py。新增回归测试
test_tool_call_turn_does_not_emit_pre_tool_llm_result。覆盖
*No response*与I will check this first.两类 pre-tool 文本。验证 pre-tool 文本不会作为
llm_result发送,且工具调用与最终回答链路仍正常。This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
已执行以下测试:
uv run pytest tests/test_tool_loop_agent_runner.py -k "tool_call_turn_does_not_emit_pre_tool_llm_result" -q结果:
完整 runner 测试:
结果:
格式化与静态检查:
结果:
说明:本 PR 仅覆盖非流式 final response 路径中的 tool-call turn 文本外发问题。
流式路径在最终确认 tool calls 前若已输出文本 delta,仍需后续单独设计 buffering 策略。
Checklist / 检查清单
requirements.txtandpyproject.toml. / 本 PR 未引入新依赖。Summary by Sourcery
Prevent pre-tool placeholder text from being emitted as user-visible LLM results during tool-calling turns and clarify tool-call prompting behavior.
Bug Fixes:
Enhancements:
Tests: