Skip to content

fix(gemini): preserve assistant tool calls during history conversion#8787

Open
Rat0323 wants to merge 1 commit into
AstrBotDevs:masterfrom
Rat0323:fix/gemini-preserve-tool-calls
Open

fix(gemini): preserve assistant tool calls during history conversion#8787
Rat0323 wants to merge 1 commit into
AstrBotDevs:masterfrom
Rat0323:fix/gemini-preserve-tool-calls

Conversation

@Rat0323

@Rat0323 Rat0323 commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes Gemini conversation history conversion for assistant messages that contain both content and tool_calls.

Previously, ProviderGoogleGenAI._prepare_conversation handled 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

  • Preserve assistant tool_calls even when assistant content is also present.
  • Preserve list-style assistant content, including think/text parts, alongside tool calls.
  • Treat tool_calls: null the same as no tool calls.
  • Add focused regression tests for these Gemini history-conversion cases.

Validation

uv run python -m pytest tests/test_gemini_source.py -q
uv run ruff check astrbot/core/provider/sources/gemini_source.py tests/test_gemini_source.py

Both commands pass locally.

CI Note

The current full Run pytest suite CI failure is unrelated to this PR's Gemini diff. In the PR run, tests/test_gemini_source.py passed, and the only failures were two existing dashboard route tests in tests/test_fastapi_v1_dashboard.py:

  • test_v1_openapi_alias_websocket_routes_are_mounted
  • test_dashboard_config_aliases_are_registered_on_fastapi

The important difference appears to be dependency resolution timing. The earlier green master CI run installed fastapi==0.136.3, while this PR run installed fastapi==0.137.0 because the repository does not commit a lockfile for CI. FastAPI 0.137.0 exposes included routers differently through _IncludedRouter, which affects tests that inspect app.router.routes directly.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. labels Jun 14, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread astrbot/core/provider/sources/gemini_source.py
Comment thread tests/test_gemini_source.py

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread astrbot/core/provider/sources/gemini_source.py
@Rat0323 Rat0323 force-pushed the fix/gemini-preserve-tool-calls branch from d68369d to a4cc1fb Compare June 14, 2026 23:04
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jun 14, 2026
@Rat0323 Rat0323 force-pushed the fix/gemini-preserve-tool-calls branch from a4cc1fb to 36d917b Compare June 14, 2026 23:14

@zouyonghe zouyonghe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 15, 2026

@zouyonghe zouyonghe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zouyonghe zouyonghe mentioned this pull request Jun 15, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:provider The bug / feature is about AI Provider, Models, LLM Agent, LLM Agent Runner. lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants