mirror of
https://github.com/browser-use/browser-use.git
synced 2026-09-14 19:59:47 +08:00
fix(llm): stop dropping system messages in the Gemini request (#5664)
Fixes #5628
Fixes #5624
### Problem
`GoogleMessageSerializer.serialize_messages()` lost system instructions
in two independent ways, so this fixes both together — they live in the
same function and share the same state.
**1. Only the last system message survived (#5628).** With the default
`include_system_in_user=False`, each system message assigned to the same
variable:
```python
else:
system_message = message.content # overwrites the previous one
```
**2. The system text vanished entirely when an assistant turn came first
(#5624).** With `include_system_in_user=True`, the prepend was gated on
`not formatted_messages`:
```python
if include_system_in_user and system_parts and role == 'user' and not formatted_messages:
```
For a `system -> assistant -> user` ordering, `formatted_messages` is
already non-empty by the time the user message arrives, so the text was
never prepended — and because `system_message` stays `None` on that
branch, it was not returned as a system instruction either. It was
simply dropped.
### Fix
- Always collect into `system_parts`, regardless of the flag, so nothing
is overwritten.
- Drop the `not formatted_messages` guard. The documented behaviour
targets the first *user* message; what precedes it is irrelevant.
`system_parts` is cleared after use, so it still fires exactly once.
- Join whatever remains in `system_parts` into the returned instruction.
With `include_system_in_user=False` that is every system message, in
order. With it set, this only triggers when there was no user message to
merge into — previously that case discarded the text silently.
A single system message still produces the identical instruction string,
so existing callers see no change.
### Tests
New `tests/ci/models/test_google_serializer.py` covering the unchanged
single-message case, both messages surviving in order, the `system ->
assistant -> user` prepend, and the no-user-message fallback.
### Evidence
On `main` (fix reverted, new tests present):
```
test_single_system_message_becomes_the_system_instruction PASSED
test_all_system_messages_reach_the_system_instruction FAILED
test_system_text_is_prepended_even_when_an_assistant_message_comes_first FAILED
test_system_text_falls_back_to_the_instruction_when_there_is_no_user_message FAILED
3 failed, 1 passed in 6.63s
```
With this branch: `4 passed`.
`tests/ci/models` and `tests/ci/security` pass (212 tests), along with
`ruff check`, `ruff format`, and `pyright`.
<!-- This is an auto-generated description by cubic. -->
---
## Summary by cubic
Fixes #5628 and #5624: `GoogleMessageSerializer.serialize_messages()`
dropped system messages in two ways, collapsing multiple system messages
to the last one and losing system text entirely when an assistant
message preceded the first user message. The serializer now preserves
every system message, prepends them to the first user message even after
an assistant turn, and returns leftover text as the system instruction
when no user message exists or when the first user turn has already been
serialized. Single system messages still produce the identical
instruction string, and regression tests cover all five cases.
<sup>Written for commit 007d63516c.
Summary will update on new commits.</sup>
<a
href="https://cubic.dev/pr/browser-use/browser-use/pull/5664?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
This commit is contained in:
@@ -37,31 +37,24 @@ class GoogleMessageSerializer:
|
||||
messages = [m.model_copy(deep=True) for m in messages]
|
||||
|
||||
formatted_messages: ContentListUnion = []
|
||||
system_message: str | None = None
|
||||
system_parts: list[str] = []
|
||||
first_user_message_serialized = False
|
||||
|
||||
for i, message in enumerate(messages):
|
||||
role = message.role if hasattr(message, 'role') else None
|
||||
|
||||
# Handle system/developer messages
|
||||
if isinstance(message, SystemMessage) or role in ['system', 'developer']:
|
||||
# Extract system message content as string
|
||||
# Collect the text of every system message; the last one must not overwrite the earlier ones
|
||||
if isinstance(message.content, str):
|
||||
if include_system_in_user:
|
||||
system_parts.append(message.content)
|
||||
else:
|
||||
system_message = message.content
|
||||
system_parts.append(message.content)
|
||||
elif message.content is not None:
|
||||
# Handle Iterable of content parts
|
||||
parts = []
|
||||
for part in message.content:
|
||||
if part.type == 'text':
|
||||
parts.append(part.text)
|
||||
combined_text = '\n'.join(parts)
|
||||
if include_system_in_user:
|
||||
system_parts.append(combined_text)
|
||||
else:
|
||||
system_message = combined_text
|
||||
system_parts.append('\n'.join(parts))
|
||||
continue
|
||||
|
||||
# Determine the role for non-system messages
|
||||
@@ -78,7 +71,10 @@ class GoogleMessageSerializer:
|
||||
|
||||
# If this is the first user message and we have system parts, prepend them
|
||||
system_text = None
|
||||
if include_system_in_user and system_parts and role == 'user' and not formatted_messages:
|
||||
# Merge into the *first* user message. An earlier assistant turn must not disqualify it,
|
||||
# but a user message that has already been serialized must: the merge target is gone, so
|
||||
# the text falls through to the separate system instruction instead.
|
||||
if include_system_in_user and system_parts and role == 'user' and not first_user_message_serialized:
|
||||
system_text = '\n\n'.join(system_parts)
|
||||
system_parts = [] # Clear after using
|
||||
|
||||
@@ -120,5 +116,13 @@ class GoogleMessageSerializer:
|
||||
final_message = Content(role=role, parts=message_parts)
|
||||
# for some reason, the type checker is not able to infer the type of formatted_messages
|
||||
formatted_messages.append(final_message) # type: ignore
|
||||
if role == 'user':
|
||||
first_user_message_serialized = True
|
||||
|
||||
# Whatever is left was never merged into a user message, so it becomes the separate system
|
||||
# instruction. With include_system_in_user=False that is every system message. With it set,
|
||||
# it is the messages that had no first user message to merge into, either because the
|
||||
# conversation contains none or because one was already serialized before they arrived.
|
||||
system_message = '\n\n'.join(system_parts) if system_parts else None
|
||||
|
||||
return formatted_messages, system_message
|
||||
|
||||
@@ -0,0 +1,88 @@
|
||||
"""Regression tests for GoogleMessageSerializer's handling of system messages."""
|
||||
|
||||
from google.genai.types import Content, ContentListUnion, Part
|
||||
|
||||
from browser_use.llm.google.serializer import GoogleMessageSerializer
|
||||
from browser_use.llm.messages import AssistantMessage, SystemMessage, UserMessage
|
||||
|
||||
|
||||
def _role_and_text(contents: ContentListUnion) -> list[tuple[str | None, str | None]]:
|
||||
"""Flatten serialized contents into (role, first part text) pairs for assertions."""
|
||||
assert isinstance(contents, list)
|
||||
|
||||
pairs: list[tuple[str | None, str | None]] = []
|
||||
for content in contents:
|
||||
assert isinstance(content, Content)
|
||||
parts = content.parts
|
||||
assert parts is not None
|
||||
assert isinstance(parts[0], Part)
|
||||
pairs.append((content.role, parts[0].text))
|
||||
|
||||
return pairs
|
||||
|
||||
|
||||
def test_single_system_message_becomes_the_system_instruction():
|
||||
"""The common case must keep returning the system text verbatim."""
|
||||
contents, system_instruction = GoogleMessageSerializer.serialize_messages(
|
||||
[SystemMessage(content='Follow the base system rule.'), UserMessage(content='Continue the task.')]
|
||||
)
|
||||
|
||||
assert system_instruction == 'Follow the base system rule.'
|
||||
assert _role_and_text(contents) == [('user', 'Continue the task.')]
|
||||
|
||||
|
||||
def test_all_system_messages_reach_the_system_instruction():
|
||||
"""Every SystemMessage must survive, in order, instead of the last one winning."""
|
||||
contents, system_instruction = GoogleMessageSerializer.serialize_messages(
|
||||
[
|
||||
SystemMessage(content='Follow the base system rule.'),
|
||||
SystemMessage(content='Also follow the additional system rule.'),
|
||||
UserMessage(content='Continue the task.'),
|
||||
]
|
||||
)
|
||||
|
||||
assert system_instruction == 'Follow the base system rule.\n\nAlso follow the additional system rule.'
|
||||
assert _role_and_text(contents) == [('user', 'Continue the task.')]
|
||||
|
||||
|
||||
def test_system_text_is_prepended_even_when_an_assistant_message_comes_first():
|
||||
"""include_system_in_user must target the first *user* message, not the first message."""
|
||||
contents, system_instruction = GoogleMessageSerializer.serialize_messages(
|
||||
[
|
||||
SystemMessage(content='Follow the system rule.'),
|
||||
AssistantMessage(content='Earlier assistant turn.'),
|
||||
UserMessage(content='Continue the task.'),
|
||||
],
|
||||
include_system_in_user=True,
|
||||
)
|
||||
|
||||
assert system_instruction is None
|
||||
assert _role_and_text(contents) == [
|
||||
('model', 'Earlier assistant turn.'),
|
||||
('user', 'Follow the system rule.\n\nContinue the task.'),
|
||||
]
|
||||
|
||||
|
||||
def test_system_text_falls_back_to_the_instruction_when_there_is_no_user_message():
|
||||
"""With nothing to merge into, the system text must not be silently dropped."""
|
||||
_, system_instruction = GoogleMessageSerializer.serialize_messages(
|
||||
[SystemMessage(content='Follow the system rule.'), AssistantMessage(content='Earlier assistant turn.')],
|
||||
include_system_in_user=True,
|
||||
)
|
||||
|
||||
assert system_instruction == 'Follow the system rule.'
|
||||
|
||||
|
||||
def test_system_message_after_the_first_user_turn_is_not_merged_into_a_later_one():
|
||||
"""The merge target is the *first* user message; once it is gone, fall back to the instruction."""
|
||||
contents, system_instruction = GoogleMessageSerializer.serialize_messages(
|
||||
[
|
||||
UserMessage(content='First user turn.'),
|
||||
SystemMessage(content='Follow the system rule.'),
|
||||
UserMessage(content='Second user turn.'),
|
||||
],
|
||||
include_system_in_user=True,
|
||||
)
|
||||
|
||||
assert system_instruction == 'Follow the system rule.'
|
||||
assert _role_and_text(contents) == [('user', 'First user turn.'), ('user', 'Second user turn.')]
|
||||
Reference in New Issue
Block a user