fix: extend empty assistant message filter to _query_stream and add empty-list coverage#7723
Open
lingyun14beta wants to merge 7 commits intoAstrBotDevs:masterfrom
Open
Conversation
dosubot
Bot
added
size:M
![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 introduces a centralized method for filtering empty assistant messages to prevent 400 errors on strict APIs, applying it to both standard and streaming query paths, and includes comprehensive tests. Feedback identifies significant regressions in the tool call parsing logic, including increased complexity and the removal of necessary error handling for malformed JSON, and suggests a simplification for the empty content check.
Comment on lines
+875
to
+892
| for tool in tools.func_list: | ||
| if ( | ||
| tool_call.type == "function" | ||
| and tool.name == tool_call.function.name | ||
| ): | ||
| # workaround for #1454 | ||
| if isinstance(tool_call.function.arguments, str): | ||
| args = json.loads(tool_call.function.arguments) | ||
| except json.JSONDecodeError as e: | ||
| logger.error(f"解析参数失败: {e}") | ||
| args = {} | ||
| else: | ||
| args = tool_call.function.arguments | ||
| args_ls.append(args) | ||
| func_name_ls.append(tool_call.function.name) | ||
| tool_call_ids.append(tool_call.id) | ||
|
|
||
| # gemini-2.5 / gemini-3 series extra_content handling | ||
| extra_content = getattr(tool_call, "extra_content", None) | ||
| if extra_content is not None: | ||
| tool_call_extra_content_dict[tool_call.id] = extra_content | ||
|
|
||
| else: | ||
| args = tool_call.function.arguments | ||
| args_ls.append(args) | ||
| func_name_ls.append(tool_call.function.name) | ||
| tool_call_ids.append(tool_call.id) | ||
|
|
||
| # gemini-2.5 / gemini-3 series extra_content handling | ||
| extra_content = getattr(tool_call, "extra_content", None) | ||
| if extra_content is not None: | ||
| tool_call_extra_content_dict[tool_call.id] = extra_content |
Contributor
There was a problem hiding this comment.
This change introduces several regressions in the tool call parsing logic:
-
Efficiency: It adds an unnecessary nested loop over
tools.func_list, resulting in$O(N \times M)$ complexity where$N$ is the number of tool calls and$M$ is the number of available tools. -
Robustness: It removes the
try...exceptblock for JSON parsing of tool arguments. LLMs occasionally return malformed JSON, and removing this error handling will cause the entire request to crash. -
Correctness: It silently ignores tool calls that are not found in
tools.func_list. This can lead to an inconsistent state where the response role is set to"tool"but no tool calls are actually processed, potentially causing errors in downstream logic.
This logic should be reverted to the previous implementation which was more robust and efficient.
Suggested change
| for tool in tools.func_list: | |
| if ( | |
| tool_call.type == "function" | |
| and tool.name == tool_call.function.name | |
| ): | |
| # workaround for #1454 | |
| if isinstance(tool_call.function.arguments, str): | |
| args = json.loads(tool_call.function.arguments) | |
| except json.JSONDecodeError as e: | |
| logger.error(f"解析参数失败: {e}") | |
| args = {} | |
| else: | |
| args = tool_call.function.arguments | |
| args_ls.append(args) | |
| func_name_ls.append(tool_call.function.name) | |
| tool_call_ids.append(tool_call.id) | |
| # gemini-2.5 / gemini-3 series extra_content handling | |
| extra_content = getattr(tool_call, "extra_content", None) | |
| if extra_content is not None: | |
| tool_call_extra_content_dict[tool_call.id] = extra_content | |
| else: | |
| args = tool_call.function.arguments | |
| args_ls.append(args) | |
| func_name_ls.append(tool_call.function.name) | |
| tool_call_ids.append(tool_call.id) | |
| # gemini-2.5 / gemini-3 series extra_content handling | |
| extra_content = getattr(tool_call, "extra_content", None) | |
| if extra_content is not None: | |
| tool_call_extra_content_dict[tool_call.id] = extra_content | |
| if tool_call.type == "function": | |
| # workaround for #1454 | |
| if isinstance(tool_call.function.arguments, str): | |
| try: | |
| args = json.loads(tool_call.function.arguments) | |
| except json.JSONDecodeError as e: | |
| logger.error(f"解析参数失败: {e}") | |
| args = {} | |
| else: | |
| args = tool_call.function.arguments | |
| args_ls.append(args) | |
| func_name_ls.append(tool_call.function.name) | |
| tool_call_ids.append(tool_call.id) | |
| # gemini-2.5 / gemini-3 series extra_content handling | |
| extra_content = getattr(tool_call, "extra_content", None) | |
| if extra_content is not None: | |
| tool_call_extra_content_dict[tool_call.id] = extra_content |
Comment on lines
+543
to
+548
| empty_content = ( | ||
| content == "" | ||
| or content is None | ||
| or content == [] | ||
| or (isinstance(content, list) and len(content) == 0) | ||
| ) |
Contributor
There was a problem hiding this comment.
The check for empty content can be simplified. In Python, content == [] is sufficient to check for an empty list, making the additional isinstance and len check redundant.
empty_content = (
content == ""
or content is None
or content == []
)![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 3 issues, and left some high level feedback:
- In
_filter_empty_assistant_messages, theempty_contentcheck redundantly tests bothcontent == []andisinstance(content, list) and len(content) == 0; consider simplifying to a single emptiness check (e.g.,content in ("", None, [])or a small helper) to make the intent clearer. - The updated tool-calls handling in
_parse_openai_completionnow nests a loop overtools.func_listinside thefor tool_call in choice.message.tool_callsloop; consider building a lookup (e.g., dict keyed by function name) or breaking once a match is found to avoid unnecessary iterations and make the control flow clearer. - The three new
_query_streamtests share a lot of setup forprovider,chunks, and thefake_create/fake_streammachinery; consider parametrizing or extracting a shared helper to reduce duplication and make future changes to the streaming fixture easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_filter_empty_assistant_messages`, the `empty_content` check redundantly tests both `content == []` and `isinstance(content, list) and len(content) == 0`; consider simplifying to a single emptiness check (e.g., `content in ("", None, [])` or a small helper) to make the intent clearer.
- The updated tool-calls handling in `_parse_openai_completion` now nests a loop over `tools.func_list` inside the `for tool_call in choice.message.tool_calls` loop; consider building a lookup (e.g., dict keyed by function name) or breaking once a match is found to avoid unnecessary iterations and make the control flow clearer.
- The three new `_query_stream` tests share a lot of setup for `provider`, `chunks`, and the `fake_create`/`fake_stream` machinery; consider parametrizing or extracting a shared helper to reduce duplication and make future changes to the streaming fixture easier.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/openai_source.py" line_range="872-881" />
<code_context>
-
- if tool_call.type == "function":
- # workaround for #1454
- if isinstance(tool_call.function.arguments, str):
- try:
+ for tool in tools.func_list:
+ if (
+ tool_call.type == "function"
+ and tool.name == tool_call.function.name
+ ):
+ # workaround for #1454
+ if isinstance(tool_call.function.arguments, str):
args = json.loads(tool_call.function.arguments)
- except json.JSONDecodeError as e:
- logger.error(f"解析参数失败: {e}")
</code_context>
<issue_to_address>
**issue (bug_risk):** Restore defensive handling for invalid JSON in `tool_call.function.arguments`.
The previous version caught `json.JSONDecodeError`, logged, and fell back to `{}` for malformed JSON. The new code will now raise instead, changing behavior and potentially surfacing user-visible errors when providers send invalid arguments (which does occur). Please reintroduce a `try/except` or similar fallback so a single malformed tool call doesn’t fail the entire completion parsing flow.
</issue_to_address>
### Comment 2
<location path="astrbot/core/provider/sources/openai_source.py" line_range="875-874" />
<code_context>
+ for tool in tools.func_list:
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify behavior when a tool call name doesn’t match any tool and avoid potential duplicate handling.
Because arguments are only appended when `tool.name == tool_call.function.name`:
- Tool calls with no matching entry in `tools.func_list` are now silently dropped.
- Duplicate tool names in `tools.func_list` will cause the same tool call to be handled multiple times.
If the goal is to accept only known tools, consider breaking after the first match and logging unmatched tool calls to surface misconfigurations.
</issue_to_address>
### Comment 3
<location path="tests/test_openai_source.py" line_range="1642-1651" />
<code_context>
+ assert payloads["messages"][1]["role"] == "user"
+
+
+def test_filter_empty_assistant_messages_removes_none_content():
+ payloads = {
+ "messages": [
+ {"role": "user", "content": "hello"},
+ {"role": "assistant", "content": None},
+ {"role": "user", "content": "world"},
+ ]
+ }
+ ProviderOpenAIOfficial._filter_empty_assistant_messages(payloads)
+ assert len(payloads["messages"]) == 2
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the `None`-content test with more explicit assertions on list contents.
Right now this only checks the final list length. Please also assert that the two remaining items are the expected user messages (e.g., with explicit `role` and `content` checks) so the test fails if filtering ever mutates or reorders non‑assistant messages.
```suggestion
def test_filter_empty_assistant_messages_removes_none_content():
payloads = {
"messages": [
{"role": "user", "content": "hello"},
{"role": "assistant", "content": None},
{"role": "user", "content": "world"},
]
}
ProviderOpenAIOfficial._filter_empty_assistant_messages(payloads)
assert len(payloads["messages"]) == 2
# Ensure the remaining messages are the original user messages in order
assert payloads["messages"][0]["role"] == "user"
assert payloads["messages"][0]["content"] == "hello"
assert payloads["messages"][1]["role"] == "user"
assert payloads["messages"][1]["content"] == "world"
```
</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
+872
to
+881
| # 工具集未提供 | ||
| # Should be unreachable | ||
| raise Exception("工具集未提供") | ||
|
|
||
| if tool_call.type == "function": | ||
| # workaround for #1454 | ||
| if isinstance(tool_call.function.arguments, str): | ||
| try: | ||
| for tool in tools.func_list: | ||
| if ( | ||
| tool_call.type == "function" | ||
| and tool.name == tool_call.function.name | ||
| ): | ||
| # workaround for #1454 | ||
| if isinstance(tool_call.function.arguments, str): |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Restore defensive handling for invalid JSON in tool_call.function.arguments.
The previous version caught json.JSONDecodeError, logged, and fell back to {} for malformed JSON. The new code will now raise instead, changing behavior and potentially surfacing user-visible errors when providers send invalid arguments (which does occur). Please reintroduce a try/except or similar fallback so a single malformed tool call doesn’t fail the entire completion parsing flow.
| if tools is None: | ||
| # 工具集未提供 | ||
| # Should be unreachable | ||
| raise Exception("工具集未提供") |
Contributor
There was a problem hiding this comment.
question (bug_risk): Clarify behavior when a tool call name doesn’t match any tool and avoid potential duplicate handling.
Because arguments are only appended when tool.name == tool_call.function.name:
- Tool calls with no matching entry in
tools.func_listare now silently dropped. - Duplicate tool names in
tools.func_listwill cause the same tool call to be handled multiple times.
If the goal is to accept only known tools, consider breaking after the first match and logging unmatched tool calls to surface misconfigurations.
Comment on lines
+1642
to
+1651
| def test_filter_empty_assistant_messages_removes_none_content(): | ||
| payloads = { | ||
| "messages": [ | ||
| {"role": "user", "content": "hello"}, | ||
| {"role": "assistant", "content": None}, | ||
| {"role": "user", "content": "world"}, | ||
| ] | ||
| } | ||
| ProviderOpenAIOfficial._filter_empty_assistant_messages(payloads) | ||
| assert len(payloads["messages"]) == 2 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen the None-content test with more explicit assertions on list contents.
Right now this only checks the final list length. Please also assert that the two remaining items are the expected user messages (e.g., with explicit role and content checks) so the test fails if filtering ever mutates or reorders non‑assistant messages.
Suggested change
| def test_filter_empty_assistant_messages_removes_none_content(): | |
| payloads = { | |
| "messages": [ | |
| {"role": "user", "content": "hello"}, | |
| {"role": "assistant", "content": None}, | |
| {"role": "user", "content": "world"}, | |
| ] | |
| } | |
| ProviderOpenAIOfficial._filter_empty_assistant_messages(payloads) | |
| assert len(payloads["messages"]) == 2 | |
| def test_filter_empty_assistant_messages_removes_none_content(): | |
| payloads = { | |
| "messages": [ | |
| {"role": "user", "content": "hello"}, | |
| {"role": "assistant", "content": None}, | |
| {"role": "user", "content": "world"}, | |
| ] | |
| } | |
| ProviderOpenAIOfficial._filter_empty_assistant_messages(payloads) | |
| assert len(payloads["messages"]) == 2 | |
| # Ensure the remaining messages are the original user messages in order | |
| assert payloads["messages"][0]["role"] == "user" | |
| assert payloads["messages"][0]["content"] == "hello" | |
| assert payloads["messages"][1]["role"] == "user" | |
| assert payloads["messages"][1]["content"] == "world" |
dosubot
Bot
added
size:L
dosubot
Bot
added
size:M
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
1 participant
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 #7721 .PR #7202 added empty assistant message filtering in
_query(non-streaming) but_query_stream(streaming) was left unprotected. DeepSeek Reasoner only returnsreasoning_contentwhen making tool calls, leavingcontentas an empty stringor empty list after serialisation. Strict APIs like DeepSeek reject such messages
with HTTP 400. The existing filter in
_queryalso did not covercontent == [].Modifications / 改动点
_filter_empty_assistant_messagesstatic method to avoid code duplication.
_queryand_query_streambefore the API call.content == [](empty list) in addition tocontent == ""andcontent is None.tests/test_openai_source.pycovering thenew helper method and
_query_streamfiltering behaviour.Screenshots or Test Results / 运行截图或测试结果
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
Extend assistant message filtering to both streaming and non-streaming OpenAI-compatible queries and improve handling of empty assistant contents.
Bug Fixes:
Enhancements:
Tests: