fix(provider): skills-like re-query 对不支持 tool_choice 的模型降级为 auto 而非移除全部工具#7856
fix(provider): skills-like re-query 对不支持 tool_choice 的模型降级为 auto 而非移除全部工具#7856Hola-Gracias wants to merge 3 commits intoAstrBotDevs:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a _requery method in the tool loop agent runner to handle cases where a model does not support tool_choice='required', falling back to auto. It also refines error handling in the OpenAI source to distinguish between general tool support issues and specific tool_choice errors. Feedback suggests adding unit tests for the new fallback logic, addressing potential model name mismatches when using fallback providers, and implementing a caching mechanism for tool_choice support to avoid redundant API failures and reduce latency.
| async def _requery( | ||
| self, | ||
| contexts: list, | ||
| param_subset: ToolSet, | ||
| ) -> LLMResponse | None: | ||
| """Send a re-query with tool_choice='required', falling back to 'auto' if | ||
| the provider does not support the 'required' value (e.g. deepseek-reasoner). | ||
| """ | ||
| try: | ||
| return await self.provider.text_chat( | ||
| contexts=self._sanitize_contexts_for_provider(contexts), | ||
| func_tool=param_subset, | ||
| model=self.req.model, | ||
| session_id=self.req.session_id, | ||
| extra_user_content_parts=self.req.extra_user_content_parts, | ||
| tool_choice="required", | ||
| abort_signal=self._abort_signal, | ||
| ) | ||
| except Exception as e: | ||
| if "tool_choice" in str(e).lower(): | ||
| logger.info( | ||
| f"tool_choice='required' 不被当前模型支持,降级为 'auto' 重试。", | ||
| ) | ||
| logger.debug(f"原始错误: {e}") | ||
| return await self.provider.text_chat( | ||
| contexts=self._sanitize_contexts_for_provider(contexts), | ||
| func_tool=param_subset, | ||
| model=self.req.model, | ||
| session_id=self.req.session_id, | ||
| extra_user_content_parts=self.req.extra_user_content_parts, | ||
| tool_choice="auto", | ||
| abort_signal=self._abort_signal, | ||
| ) | ||
| raise |
| return await self.provider.text_chat( | ||
| contexts=self._sanitize_contexts_for_provider(contexts), | ||
| func_tool=param_subset, | ||
| model=self.req.model, |
| except Exception as e: | ||
| if "tool_choice" in str(e).lower(): | ||
| logger.info( | ||
| f"tool_choice='required' 不被当前模型支持,降级为 'auto' 重试。", | ||
| ) | ||
| logger.debug(f"原始错误: {e}") | ||
| return await self.provider.text_chat( | ||
| contexts=self._sanitize_contexts_for_provider(contexts), | ||
| func_tool=param_subset, | ||
| model=self.req.model, | ||
| session_id=self.req.session_id, | ||
| extra_user_content_parts=self.req.extra_user_content_parts, | ||
| tool_choice="auto", | ||
| abort_signal=self._abort_signal, | ||
| ) | ||
| raise |
There was a problem hiding this comment.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_requery, catching a broadExceptionand checking for'tool_choice'in the string is a bit fragile; consider narrowing the exception type (e.g., to your provider error class) or using structured error attributes if available to avoid misclassifying other failures. - The string checks in
_handle_api_errorand_requeryare duplicated and rely on specific English phrases; it might be worth centralizing these model-capability checks or introducing a helper to encapsulate the logic so behavior stays consistent as providers or messages evolve.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_requery`, catching a broad `Exception` and checking for `'tool_choice'` in the string is a bit fragile; consider narrowing the exception type (e.g., to your provider error class) or using structured error attributes if available to avoid misclassifying other failures.
- The string checks in `_handle_api_error` and `_requery` are duplicated and rely on specific English phrases; it might be worth centralizing these model-capability checks or introducing a helper to encapsulate the logic so behavior stays consistent as providers or messages evolve.
## Individual Comments
### Comment 1
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="1320-1329" />
<code_context>
return llm_resp, subset
+ async def _requery(
+ self,
+ contexts: list,
+ param_subset: ToolSet,
+ ) -> LLMResponse | None:
+ """Send a re-query with tool_choice='required', falling back to 'auto' if
+ the provider does not support the 'required' value (e.g. deepseek-reasoner).
+ """
+ try:
+ return await self.provider.text_chat(
+ contexts=self._sanitize_contexts_for_provider(contexts),
+ func_tool=param_subset,
+ model=self.req.model,
+ session_id=self.req.session_id,
+ extra_user_content_parts=self.req.extra_user_content_parts,
+ tool_choice="required",
+ abort_signal=self._abort_signal,
+ )
+ except Exception as e:
+ if "tool_choice" in str(e).lower():
+ logger.info(
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid catching and handling asyncio.CancelledError inside _requery
Catching `Exception` here will also swallow `asyncio.CancelledError`, breaking cooperative cancellation (e.g., aborted requests or upstream task cancels). Please re-raise `CancelledError` before handling other exceptions, e.g.:
```python
except Exception as e:
if isinstance(e, asyncio.CancelledError):
raise
if "tool_choice" in str(e).lower():
...
```
This keeps cancellation semantics intact while still handling the `tool_choice` fallback.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| async def _requery( | ||
| self, | ||
| contexts: list, | ||
| param_subset: ToolSet, | ||
| ) -> LLMResponse | None: | ||
| """Send a re-query with tool_choice='required', falling back to 'auto' if | ||
| the provider does not support the 'required' value (e.g. deepseek-reasoner). | ||
| """ | ||
| try: | ||
| return await self.provider.text_chat( |
There was a problem hiding this comment.
issue (bug_risk): Avoid catching and handling asyncio.CancelledError inside _requery
Catching Exception here will also swallow asyncio.CancelledError, breaking cooperative cancellation (e.g., aborted requests or upstream task cancels). Please re-raise CancelledError before handling other exceptions, e.g.:
except Exception as e:
if isinstance(e, asyncio.CancelledError):
raise
if "tool_choice" in str(e).lower():
...This keeps cancellation semantics intact while still handling the tool_choice fallback.
fix: #7853
Modifications / 改动点
错误处理器中「不支持工具」的判定排除 tool_choice 关键词,避免将 tool_choice 不兼容误判为模型不支持工具调用。同时将原始错误信息降级为 DEBUG 日志。
Screenshots or Test Results / 运行截图或测试结果
[2026-04-27 23:02:16.274] [Core] [INFO] [runners.tool_loop_agent_runner:1340]: tool_choice='required' 不被当前模型支持,降级为 'auto' 重试。
[2026-04-27 23:02:16.275] [Core] [DBUG] [runners.tool_loop_agent_runner:1343]: 原始错误: Error code: 400 - {'error': {'message': 'deepseek-reasoner does not support this tool_choice', 'type': 'invalid_request_error', 'param': None, 'code': 'invalid_request_error'}}
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Handle models that do not support
tool_choice="required"by degrading totool_choice="auto"while preserving tool definitions, and avoid misclassifyingtool_choiceincompatibility as lack of tool support.Bug Fixes:
tool_choice="required"support fall back totool_choice="auto"instead of dropping all tools.tool_choice-related errors as signals that a model does not support tools, and log the original error message at debug level.