fix: 兼容 Moonshot thinking 下 tool_choice required 冲突#7727
fix: 兼容 Moonshot thinking 下 tool_choice required 冲突#7727Sisyphbaous-DT-Project wants to merge 5 commits intoAstrBotDevs:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The detection logic in
_is_tool_choice_required_incompatible_with_thinking_erroris quite broad (just checking for the presence of several words), which risks false positives; consider first matching the known upstream message exactly and only falling back to the heuristic if you need to support variations. - When downgrading
tool_choicefromrequiredtoautoin_handle_api_error, you mutatepayloadsin place; if the originalpayloadsis reused elsewhere in the call stack, it may be safer to work on a shallow copy for the retry to avoid unintended side effects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The detection logic in `_is_tool_choice_required_incompatible_with_thinking_error` is quite broad (just checking for the presence of several words), which risks false positives; consider first matching the known upstream message exactly and only falling back to the heuristic if you need to support variations.
- When downgrading `tool_choice` from `required` to `auto` in `_handle_api_error`, you mutate `payloads` in place; if the original `payloads` is reused elsewhere in the call stack, it may be safer to work on a shallow copy for the retry to avoid unintended side effects.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to handle API errors when tool_choice='required' is incompatible with a provider's 'thinking' mode. It adds a detection method for this specific error and modifies the error handler to automatically downgrade tool_choice to auto and retry the request. A corresponding test case has been added to verify this behavior. I have no feedback to provide.
|
收到,感谢你的及时反馈与补充。这些修改(收窄错误匹配条件、使用浅拷贝避免原地修改 |
659c809 to
7868324
Compare
3d4c5a6 to
51894c7
Compare
51894c7 to
526a766
Compare
|
@sourcery-ai review |
|
/gemini review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_handle_api_error, you call_extract_error_text_candidates(e)both inside_is_tool_choice_required_incompatible_with_thinking_errorand again in the laterrequired_tool_choiceblock; consider extracting candidates once and reusing them (possibly passing into the helper) to avoid duplication and keep the matching logic consistent. - The fallback logic for
tool_choice=requiredis split between_is_tool_choice_required_incompatible_with_thinking_errorand the subsequenthas_related_keywordsdebug branch; it might be clearer to centralize or more explicitly separate these two paths so future readers understand why some errors trigger downgrade and others only log.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_handle_api_error`, you call `_extract_error_text_candidates(e)` both inside `_is_tool_choice_required_incompatible_with_thinking_error` and again in the later `required_tool_choice` block; consider extracting candidates once and reusing them (possibly passing into the helper) to avoid duplication and keep the matching logic consistent.
- The fallback logic for `tool_choice=required` is split between `_is_tool_choice_required_incompatible_with_thinking_error` and the subsequent `has_related_keywords` debug branch; it might be clearer to centralize or more explicitly separate these two paths so future readers understand why some errors trigger downgrade and others only log.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to handle 'tool_choice=required' incompatibility with thinking mode in the OpenAI provider by automatically downgrading to 'auto' and retrying. It adds a specific error detection method and comprehensive unit tests. Feedback suggests optimizing the detection logic for performance and robustness, as well as streamlining the error handling flow to reduce redundant logging and duplicate computations.
|
@sourcery-ai 已根据这轮建议完成调整
已补充并通过相关测试与格式检查 |
|
@sourcery-ai review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements automatic error handling for cases where tool_choice='required' is incompatible with "thinking" mode in the OpenAI provider. A new detection method identifies this specific conflict in API error messages, allowing the system to automatically downgrade the tool_choice to auto and retry the request. The changes include comprehensive unit tests covering error detection, retry logic, and logging. I have no feedback to provide.
|
两个 bot 都没有提出新的可执行问题,当前实现看起来没有问题,可以继续推进合并 |
背景
Moonshot / Kimi 在开启 thinking 模式时,如果请求里带了
tool_choice="required",上游会直接返回 400典型报错是
tool_choice 'required' is incompatible with thinking enabled这个问题会让 skills_like 模式下的工具重查询直接中断
这次做了什么
tool_choice从required降级为auto,然后自动重试后面补充的完善
payloads直接改掉tool_choice=required->auto,排查起来更直接测试
补了几条针对性测试,主要覆盖这几种情况
required失败后会自动切到auto并成功返回payloads验证
已执行
uv run pytest tests/test_openai_source.py -k "required_thinking_conflict_uses_payload_copy or required_thinking_conflict_emits_fallback_log or incompatible_with_thinking" -quv run ruff check astrbot/core/provider/sources/openai_source.py tests/test_openai_source.pyuv run ruff format --check astrbot/core/provider/sources/openai_source.py tests/test_openai_source.py结果通过
Summary by Sourcery
Handle Moonshot/Kimi thinking-mode incompatibility with tool_choice="required" by detecting specific upstream errors and retrying with tool_choice="auto" at the provider layer.
Bug Fixes:
Tests: