fix: empty model output error may misfire when use gemini#7377
fix: empty model output error may misfire when use gemini#7377
Conversation
dosubot
bot
added
the
size:M
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
validate_outputflag on_process_content_partschanges error behavior in a non-obvious way; consider adding a short docstring note or renaming it (e.g.raise_on_empty_output) to make the semantics of the parameter clearer to future callers. - You now construct an empty
MessageChain(chain=[])in multiple branches whenvalidate_outputis false; consider extracting a small helper (e.g._empty_result_chain(llm_response)) to avoid duplication and ensure any future changes to the empty response shape stay consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `validate_output` flag on `_process_content_parts` changes error behavior in a non-obvious way; consider adding a short docstring note or renaming it (e.g. `raise_on_empty_output`) to make the semantics of the parameter clearer to future callers.
- You now construct an empty `MessageChain(chain=[])` in multiple branches when `validate_output` is false; consider extracting a small helper (e.g. `_empty_result_chain(llm_response)`) to avoid duplication and ensure any future changes to the empty response shape stay consistent.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/gemini_source.py" line_range="470-475" />
<code_context>
- "Gemini candidate content is empty. "
- f"finish_reason={candidate.finish_reason}"
- )
+ if validate_output:
+ raise EmptyModelOutputError(
+ "Gemini candidate content is empty. "
+ f"finish_reason={candidate.finish_reason}"
+ )
+ llm_response.result_chain = MessageChain(chain=[])
+ return llm_response.result_chain
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning an empty `MessageChain` when `candidate.content` is empty may mask streaming heartbeats/keep-alive chunks
In the streaming path, `_process_content_parts(..., validate_output=False)` will hit this branch for chunks where `candidate.content` is empty (e.g., heartbeats or tool-only updates). That now resets `llm_response.result_chain` to an empty `MessageChain`, discarding previously accumulated tokens. Before, this would raise and stop processing, making the issue visible. For streaming, it would be safer to treat empty `candidate.content` as a no-op (return the existing `result_chain` unchanged) to avoid losing prior content.
</issue_to_address>
### Comment 2
<location path="astrbot/core/provider/sources/gemini_source.py" line_range="717-720" />
<code_context>
llm_response.result_chain = self._process_content_parts(
chunk.candidates[0],
llm_response,
+ validate_output=False,
)
llm_response.id = chunk.response_id
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Streaming path now treats all chunks as non-validating, including the final one
Because both the streaming branch and the `last(...)` branch pass `validate_output=False`, the fully assembled `final_response` never gets the stricter validation. To allow leniency for partial/empty chunks but still enforce that the final response is valid, consider using `validate_output=False` only for intermediate chunks and `validate_output=True` for the final chunk, or run a separate final validation after aggregation.
Suggested implementation:
```python
llm_response.result_chain = self._process_content_parts(
chunk.candidates[0],
llm_response,
validate_output=False,
)
llm_response.id = chunk.response_id
if chunk.usage_metadata:
# For the aggregated final_response, enable validation so that
# the fully-assembled output is checked strictly.
final_response.result_chain = self._process_content_parts(
chunk.candidates[0],
final_response,
validate_output=True,
)
final_response.id = chunk.response_id
if chunk.usage_metadata:
```
To fully meet your comment (validating only the final aggregated response, not every chunk):
1. Identify where the streaming loop over `chunk`s terminates and where `final_response` is returned.
2. Move or add a single validation call there, e.g.:
```python
self._ensure_usable_response(
final_response,
response_id=final_response.id,
finish_reason=str(final_finish_reason) if final_finish_reason is not None else None,
)
```
3. If you do add this post-loop validation, you can consider reverting `validate_output=True` above to the default (omit the argument) or even leave it `False` again, relying solely on the final `_ensure_usable_response` call to enforce strict validation on the aggregated response.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if validate_output: | ||
| raise EmptyModelOutputError( | ||
| "Gemini candidate content is empty. " | ||
| f"finish_reason={candidate.finish_reason}" | ||
| ) | ||
| llm_response.result_chain = MessageChain(chain=[]) |
There was a problem hiding this comment.
issue (bug_risk): Returning an empty MessageChain when candidate.content is empty may mask streaming heartbeats/keep-alive chunks
In the streaming path, _process_content_parts(..., validate_output=False) will hit this branch for chunks where candidate.content is empty (e.g., heartbeats or tool-only updates). That now resets llm_response.result_chain to an empty MessageChain, discarding previously accumulated tokens. Before, this would raise and stop processing, making the issue visible. For streaming, it would be safer to treat empty candidate.content as a no-op (return the existing result_chain unchanged) to avoid losing prior content.
| llm_response.result_chain = self._process_content_parts( | ||
| chunk.candidates[0], | ||
| llm_response, | ||
| validate_output=False, |
There was a problem hiding this comment.
suggestion (bug_risk): Streaming path now treats all chunks as non-validating, including the final one
Because both the streaming branch and the last(...) branch pass validate_output=False, the fully assembled final_response never gets the stricter validation. To allow leniency for partial/empty chunks but still enforce that the final response is valid, consider using validate_output=False only for intermediate chunks and validate_output=True for the final chunk, or run a separate final validation after aggregation.
Suggested implementation:
llm_response.result_chain = self._process_content_parts(
chunk.candidates[0],
llm_response,
validate_output=False,
)
llm_response.id = chunk.response_id
if chunk.usage_metadata:
# For the aggregated final_response, enable validation so that
# the fully-assembled output is checked strictly.
final_response.result_chain = self._process_content_parts(
chunk.candidates[0],
final_response,
validate_output=True,
)
final_response.id = chunk.response_id
if chunk.usage_metadata:To fully meet your comment (validating only the final aggregated response, not every chunk):
-
Identify where the streaming loop over
chunks terminates and wherefinal_responseis returned. -
Move or add a single validation call there, e.g.:
self._ensure_usable_response( final_response, response_id=final_response.id, finish_reason=str(final_finish_reason) if final_finish_reason is not None else None, )
-
If you do add this post-loop validation, you can consider reverting
validate_output=Trueabove to the default (omit the argument) or even leave itFalseagain, relying solely on the final_ensure_usable_responsecall to enforce strict validation on the aggregated response.
dosubot
bot
added
the
area:provider
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a validate_output parameter to the _process_content_parts method in the Gemini provider, allowing the system to handle empty model outputs without immediately raising an exception, particularly during streaming. Review feedback highlights that the current placement of empty content checks may bypass important safety and policy validations, leading to generic error messages instead of specific safety warnings. Furthermore, a potential issue was identified in _query_stream where response metadata might be lost if the final chunk contains a finish reason but no content parts.
| if validate_output: | ||
| raise EmptyModelOutputError( | ||
| "Gemini candidate content is empty. " | ||
| f"finish_reason={candidate.finish_reason}" | ||
| ) | ||
| llm_response.result_chain = MessageChain(chain=[]) | ||
| return llm_response.result_chain |
There was a problem hiding this comment.
The early return (line 476) and the potential exception (line 471) occur before the safety and policy checks (lines 481-494). When Gemini blocks a response due to safety filters, it often returns a candidate with no content. In such cases, this function will either raise a generic EmptyModelOutputError or return an empty chain, bypassing the specific safety error messages. This leads to the "misfire" mentioned in the PR title, where the user sees an empty output error instead of a safety violation warning.
Consider moving the finish_reason validation logic (the safety and policy checks) to the very beginning of the function so they are executed even if candidate.content is missing.
| final_response.result_chain = self._process_content_parts( | ||
| chunk.candidates[0], | ||
| final_response, | ||
| validate_output=False, | ||
| ) |
There was a problem hiding this comment.
In _query_stream, if the final chunk has a finish_reason but no content.parts, the block starting at line 745 (in the full file) is skipped. Consequently, final_response is initialized later at line 760 without capturing the response_id and usage_metadata from that final chunk. This results in missing metadata for the complete response.
Consider extracting the metadata (id and usage) from the final chunk regardless of whether it contains new content parts.
1 participant
Modifications / 改动点
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
Bug Fixes: