fix(agent): run response hooks before send in skills_like no-tool fallback - #9789
fix(agent): run response hooks before send in skills_like no-tool fallback#9789ryeeda wants to merge 2 commits into
Conversation
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="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="941" />
<code_context>
)
+ # Finalize before yielding so response hooks can review or rewrite
+ # the fallback text before downstream stages send it.
+ await self._complete_with_assistant_response(llm_resp)
if llm_resp.reasoning_content:
yield AgentResponse(
</code_context>
<issue_to_address>
**issue (bug_risk):** When the skills-like re-query returns an `LLMResponse` with `role == "err"` and no tool calls, this newly added call treats the error as a successful assistant response: it transitions the runner to `DONE`, appends the error text to assistant history, and invokes response-completion hooks. The earlier error branch at line 883 is bypassed because the re-query occurs after that branch, so provider errors on this path no longer follow the normal error handling and history contract.
**Triggers:** When the skills-like re-query provider returns an error response rather than raising an exception.
**Suggested fix:** Check `llm_resp.role == "err"` before finalizing the fallback, and route it through the existing error-state/error-response handling instead of `_complete_with_assistant_response`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the hook-order change is wrong, a fallback response could be rewritten incorrectly and sent to the user, with the same incorrect text persisted in conversation history. Reverting prevents future occurrences, but it cannot retract messages already sent; the affected history is bounded and repairable.
Blocking findings: astrbot/core/agent/runners/tool_loop_agent_runner.py:941
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ndling Addresses review feedback: a re-query returning role == "err" (rather than raising) must transition to ERROR and yield an err response instead of being finalized as assistant content.
|
@sourcery-ai review |
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="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="958-960" />
<code_context>
+ ),
+ )
+ return
+ # Finalize before yielding so response hooks can review or rewrite
+ # the fallback text before downstream stages send it.
+ await self._complete_with_assistant_response(llm_resp)
if llm_resp.reasoning_content:
yield AgentResponse(
</code_context>
<issue_to_address>
**issue (broader_impact):** When an `OnLLMResponseEvent` hook rewrites `llm_resp` without also mutating `run_context.messages`, `_complete_with_assistant_response()` saves the original completion to history before invoking the hook, while the fallback sends the rewritten completion afterward. The delivered text and persisted assistant history therefore diverge in this newly changed fallback path.
**Triggers:** When a response hook performs content rewriting, redaction, or safety filtering by changing only the `LLMResponse`.
**Suggested fix:** Run the response hooks before constructing the assistant history message, or synchronize the appended history message from the finalized `LLMResponse` after the hooks complete.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and the fallback response is now finalized before it is yielded, allowing response and completion hooks to rewrite the text that is sent and saved to conversation history. If that ordering is wrong, a bad assistant message can be delivered externally and persisted; reverting the change cannot unsend it or automatically remove the saved history.
Blocking findings: astrbot/core/agent/runners/tool_loop_agent_runner.py:960
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Fixes #9788
Modifications / 改动点
skills_like工具重查询无工具时的 hook/发送顺序。_complete_with_assistant_response(),确保OnLLMResponseEvent与OnAgentDoneEvent能先审查或改写最终响应。LLMResponse,不复用 hook 前构造的消息组件。tests/test_skills_like_hook_order.py(fake provider 驱动真实 runner、scheduler 与 respond stage)。Why / 原因
旧顺序是
yield -> downstream send -> response hooks。这使所有依赖 response hook 做内容安全、脱敏、改写或审计的插件在该 fallback 分支失去发送前保障,也可能造成已发送文本与最终 history 不一致。新顺序是
response hooks -> build chain from finalized response -> yield -> downstream send -> after-message hook -> history save。与正常无工具终局路径(先_complete_with_assistant_response再 yield)保持一致。Verification Steps / 验证步骤
新增端到端契约测试以 fake provider 驱动真实 runner、scheduler 和 respond stage,验证:
skills_like无工具重查询时,response hook 改写后的文本才被发送;验证基于最新 master(
19d00fb):1 failed, 4 passed);5 passed;tests/test_astr_agent_run_util.py+tests/agent/:99 passed。Screenshots or Test Results / 运行截图或测试结果
warning 为既有
audioop弃用提示,与本次修改无关。Non-goals / 不在本 PR 范围
本 PR 不调整 streaming/live 发送语义。流式响应的逐块发送与终局 hook 改写之间需要单独的 API/生命周期设计。
Related / 相关链接
Checklist / 检查清单
Summary by Sourcery
Run response hooks before finalizing and sending skills-like fallback responses while preserving correct error handling and history consistency.
Bug Fixes:
Tests: