fix(gemini): preserve assistant tool calls during history conversion#8787
fix(gemini): preserve assistant tool calls during history conversion#8787Rat0323 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the tests,
_first_model_partsrelies on__class__.__name__ == "ModelContent", which is brittle; consider importing and usingisinstance(..., types.ModelContent)instead so the tests don't silently break if class names or implementations change. - The test helper
_make_gemini_provider_for_conversationusesobject.__new__to bypassProviderGoogleGenAIinitialization, which can diverge from real usage as the class evolves; it would be safer to construct a real instance (or a lightweight subclass/factory) that runs__init__while still allowing you to inject the provider_config.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the tests, `_first_model_parts` relies on `__class__.__name__ == "ModelContent"`, which is brittle; consider importing and using `isinstance(..., types.ModelContent)` instead so the tests don't silently break if class names or implementations change.
- The test helper `_make_gemini_provider_for_conversation` uses `object.__new__` to bypass `ProviderGoogleGenAI` initialization, which can diverge from real usage as the class evolves; it would be safer to construct a real instance (or a lightweight subclass/factory) that runs `__init__` while still allowing you to inject the provider_config.
## Individual Comments
### Comment 1
<location path="astrbot/core/provider/sources/gemini_source.py" line_range="409-414" />
<code_context>
- elif not native_tool_enabled and "tool_calls" in message:
- parts = []
- for tool in message["tool_calls"]:
+ tool_calls = message.get("tool_calls") or []
+ if not native_tool_enabled and tool_calls:
+ for tool in tool_calls:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Reuse the `tool_calls` variable for the warning condition to keep the logic consistent.
Right now the warning condition still checks `"tool_calls" in message`, while the loop relies on the normalized `tool_calls = message.get("tool_calls") or []`. This can produce a warning even when `tool_calls` is effectively empty (e.g. key present but `None`). Using the normalized value in the condition (e.g. `if native_tool_enabled and tool_calls:`) keeps the warning aligned with the actual tool calls being processed.
</issue_to_address>
### Comment 2
<location path="tests/test_gemini_source.py" line_range="69-85" />
<code_context>
+ return model_content.parts or []
+
+
+def test_prepare_conversation_keeps_assistant_text_and_tool_calls():
+ provider = _make_gemini_provider_for_conversation()
+ payloads = {
+ "messages": [
+ {"role": "user", "content": "summarize this PR"},
+ _assistant_tool_call_message("I will inspect the changed files first."),
+ ]
+ }
+
+ parts = _first_model_parts(provider._prepare_conversation(payloads))
+
+ assert any(part.text == "I will inspect the changed files first." for part in parts)
+ assert [
+ part.function_call.name
+ for part in parts
+ if getattr(part, "function_call", None)
+ ] == ["get_pull_request_files"]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test where the assistant message contains only tool_calls and no textual content.
Since we now build `parts` incrementally and fall back to a placeholder when `parts` is empty, we should also cover an assistant turn where `content` is `None`/empty and only `tool_calls` are present, e.g. `{role: 'assistant', content: None, tool_calls: [...]}`. The test should assert that we serialize only the function calls and do not inject the placeholder text part in this case.
```suggestion
def test_prepare_conversation_keeps_assistant_text_and_tool_calls():
provider = _make_gemini_provider_for_conversation()
payloads = {
"messages": [
{"role": "user", "content": "summarize this PR"},
_assistant_tool_call_message("I will inspect the changed files first."),
]
}
parts = _first_model_parts(provider._prepare_conversation(payloads))
assert any(part.text == "I will inspect the changed files first." for part in parts)
assert [
part.function_call.name
for part in parts
if getattr(part, "function_call", None)
] == ["get_pull_request_files"]
def test_prepare_conversation_assistant_only_tool_calls_no_placeholder_text():
provider = _make_gemini_provider_for_conversation()
payloads = {
"messages": [
{"role": "user", "content": "summarize this PR"},
# Assistant turn with no textual content, only tool calls
{
"role": "assistant",
"content": None,
"tool_calls": [
{
"id": "call_1",
"type": "function",
"function": {
"name": "get_pull_request_files",
"arguments": "{}",
},
}
],
},
]
}
parts = _first_model_parts(provider._prepare_conversation(payloads))
# We should have only function_call parts and no injected placeholder text
assert all(
not getattr(part, "text", None) for part in parts
), "No placeholder text parts should be injected when assistant turn has only tool_calls"
assert [
part.function_call.name
for part in parts
if getattr(part, "function_call", None)
] == ["get_pull_request_files"]
```
</issue_to_address>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 refactors the assistant message processing logic for Gemini in gemini_source.py by consolidating the parts initialization and the append_or_extend call, and simplifies the handling of tool calls. It also adds corresponding unit tests in test_gemini_source.py. The reviewer identified a potential issue where empty or invalid parts could bypass the fallback placeholder check and cause API errors, and suggested filtering out empty parts beforehand.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
d68369d to
a4cc1fb
Compare
a4cc1fb to
36d917b
Compare
zouyonghe
left a comment
There was a problem hiding this comment.
Reviewed. The conversion now independently preserves assistant content and , including list-style think/text content and . The focused Gemini tests cover the regression shape. The current full pytest failure is unrelated and matches the FastAPI route-test compatibility issue fixed by #8783.
zouyonghe
left a comment
There was a problem hiding this comment.
Reviewed. The conversion now independently preserves assistant content and tool_calls, including list-style think/text content and tool_calls null. The focused Gemini tests cover the regression shape. The current full pytest failure is unrelated and matches the FastAPI route-test compatibility issue fixed by #8783.
Summary
This PR fixes Gemini conversation history conversion for assistant messages that contain both
contentandtool_calls.Previously,
ProviderGoogleGenAI._prepare_conversationhandled assistant message fields through mutually exclusive branches. When an assistant message had normal content and tool calls at the same time, only the content was serialized and the function-call parts were dropped. Later turns could then contain tool responses without the matching assistant function calls in history.The updated conversion serializes assistant content and tool calls independently, while also tolerating
tool_calls: null.Changes
tool_callseven when assistantcontentis also present.tool_calls: nullthe same as no tool calls.Validation
Both commands pass locally.
CI Note
The current full
Run pytest suiteCI failure is unrelated to this PR's Gemini diff. In the PR run,tests/test_gemini_source.pypassed, and the only failures were two existing dashboard route tests intests/test_fastapi_v1_dashboard.py:test_v1_openapi_alias_websocket_routes_are_mountedtest_dashboard_config_aliases_are_registered_on_fastapiThe important difference appears to be dependency resolution timing. The earlier green master CI run installed
fastapi==0.136.3, while this PR run installedfastapi==0.137.0because the repository does not commit a lockfile for CI. FastAPI 0.137.0 exposes included routers differently through_IncludedRouter, which affects tests that inspectapp.router.routesdirectly.