From 1cfa758a8d7ce04f5e5598d1ee5f955ef2bcad37 Mon Sep 17 00:00:00 2001 From: TanejaAnkisetty Date: Sun, 10 Aug 2025 23:07:28 +0100 Subject: [PATCH 1/8] fix: Ensure user content is included in LLM requests when provided by user --- src/google/adk/flows/llm_flows/contents.py | 2 +- src/google/adk/models/base_llm.py | 6 +++ tests/unittests/models/test_base_llm.py | 48 ++++++++++++++++++++++ 3 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 tests/unittests/models/test_base_llm.py diff --git a/src/google/adk/flows/llm_flows/contents.py b/src/google/adk/flows/llm_flows/contents.py index ae1bd44ad9..e3935688a4 100644 --- a/src/google/adk/flows/llm_flows/contents.py +++ b/src/google/adk/flows/llm_flows/contents.py @@ -223,12 +223,12 @@ def _get_contents( for event in events: if ( not event.content - or not event.content.role or not event.content.parts or event.content.parts[0].text == '' ): # Skip events without content, or generated neither by user nor by model # or has empty text. + # Doesn't skip events with user content but without a role. # E.g. events purely for mutating session states. continue diff --git a/src/google/adk/models/base_llm.py b/src/google/adk/models/base_llm.py index 159ae221a3..7ef2f10448 100644 --- a/src/google/adk/models/base_llm.py +++ b/src/google/adk/models/base_llm.py @@ -97,6 +97,12 @@ def _maybe_append_user_content(self, llm_request: LlmRequest): ) return + # Insert user role for the content where the user message exists + # but not the role + if (llm_request.contents[-1].parts): + llm_request.contents[-1].role = "user" + return + # Insert a user content to preserve user intent and to avoid empty # model response. if llm_request.contents[-1].role != 'user': diff --git a/tests/unittests/models/test_base_llm.py b/tests/unittests/models/test_base_llm.py new file mode 100644 index 0000000000..b4c4d43976 --- /dev/null +++ b/tests/unittests/models/test_base_llm.py @@ -0,0 +1,48 @@ +# Copyright 2025 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import pytest +from google.genai import types +from google.adk.models.llm_request import LlmRequest +from google.adk.models.lite_llm import _get_completion_inputs + + +@pytest.mark.parametrize("content_kwargs", [ + # Case 1: Explicit role provided + {"role": "user", "parts": [types.Part(text="This is an input text from user.")]}, + # Case 2: Role omitted, should still be treated as 'user' + {"parts": [types.Part(text="This is an input text from user.")]} +]) +def test_user_content_role_defaults_to_user(content_kwargs): + """ + Verifies that user-provided messages are always passed to the LLM as 'user' role, + regardless of whether the role is explicitly set in types.Content. + + The helper `_get_completion_inputs` should give normalize messages so that + explicit 'user' and implicit (missing role) are equivalent. + """ + llm_request = LlmRequest( + contents=[types.Content(**content_kwargs)], + config=types.GenerateContentConfig() + ) + + messages, _, _, _ = _get_completion_inputs(llm_request) + + assert all( + msg.get("role") == "user" for msg in messages + ), f"Expected role 'user' but got {messages}" + assert any( + "This is an input text from user." == (msg.get("content") or "") + for msg in messages + ), f"Expected the user text to be preserved, but got {messages}" \ No newline at end of file From 4548f3f241eecff12af8e23c14ff25b102abe163 Mon Sep 17 00:00:00 2001 From: TanejaAnkisetty Date: Thu, 21 Aug 2025 12:01:02 +0100 Subject: [PATCH 2/8] mandating the role attribute in the new_message of runner --- src/google/adk/flows/llm_flows/contents.py | 2 +- src/google/adk/models/base_llm.py | 6 ------ src/google/adk/runners.py | 4 ++++ 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/google/adk/flows/llm_flows/contents.py b/src/google/adk/flows/llm_flows/contents.py index e3935688a4..ae1bd44ad9 100644 --- a/src/google/adk/flows/llm_flows/contents.py +++ b/src/google/adk/flows/llm_flows/contents.py @@ -223,12 +223,12 @@ def _get_contents( for event in events: if ( not event.content + or not event.content.role or not event.content.parts or event.content.parts[0].text == '' ): # Skip events without content, or generated neither by user nor by model # or has empty text. - # Doesn't skip events with user content but without a role. # E.g. events purely for mutating session states. continue diff --git a/src/google/adk/models/base_llm.py b/src/google/adk/models/base_llm.py index 7ef2f10448..159ae221a3 100644 --- a/src/google/adk/models/base_llm.py +++ b/src/google/adk/models/base_llm.py @@ -97,12 +97,6 @@ def _maybe_append_user_content(self, llm_request: LlmRequest): ) return - # Insert user role for the content where the user message exists - # but not the role - if (llm_request.contents[-1].parts): - llm_request.contents[-1].role = "user" - return - # Insert a user content to preserve user intent and to avoid empty # model response. if llm_request.contents[-1].role != 'user': diff --git a/src/google/adk/runners.py b/src/google/adk/runners.py index 51fdb96583..819f4ea51e 100644 --- a/src/google/adk/runners.py +++ b/src/google/adk/runners.py @@ -201,6 +201,10 @@ async def run_async( ) if not session: raise ValueError(f'Session not found: {session_id}') + + # Validate new_message has a role + if not getattr(new_message, "role", None): + raise ValueError("new_message must have a role, but none was provided.") invocation_context = self._new_invocation_context( session, From c6a9a0ab76a88457fd8e59ab821d2fbe7197a214 Mon Sep 17 00:00:00 2001 From: "Wei Sun (Jack)" Date: Thu, 21 Aug 2025 09:53:51 -0700 Subject: [PATCH 3/8] Delete tests/unittests/models/test_base_llm.py --- tests/unittests/models/test_base_llm.py | 48 ------------------------- 1 file changed, 48 deletions(-) delete mode 100644 tests/unittests/models/test_base_llm.py diff --git a/tests/unittests/models/test_base_llm.py b/tests/unittests/models/test_base_llm.py deleted file mode 100644 index b4c4d43976..0000000000 --- a/tests/unittests/models/test_base_llm.py +++ /dev/null @@ -1,48 +0,0 @@ -# Copyright 2025 Google LLC -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import pytest -from google.genai import types -from google.adk.models.llm_request import LlmRequest -from google.adk.models.lite_llm import _get_completion_inputs - - -@pytest.mark.parametrize("content_kwargs", [ - # Case 1: Explicit role provided - {"role": "user", "parts": [types.Part(text="This is an input text from user.")]}, - # Case 2: Role omitted, should still be treated as 'user' - {"parts": [types.Part(text="This is an input text from user.")]} -]) -def test_user_content_role_defaults_to_user(content_kwargs): - """ - Verifies that user-provided messages are always passed to the LLM as 'user' role, - regardless of whether the role is explicitly set in types.Content. - - The helper `_get_completion_inputs` should give normalize messages so that - explicit 'user' and implicit (missing role) are equivalent. - """ - llm_request = LlmRequest( - contents=[types.Content(**content_kwargs)], - config=types.GenerateContentConfig() - ) - - messages, _, _, _ = _get_completion_inputs(llm_request) - - assert all( - msg.get("role") == "user" for msg in messages - ), f"Expected role 'user' but got {messages}" - assert any( - "This is an input text from user." == (msg.get("content") or "") - for msg in messages - ), f"Expected the user text to be preserved, but got {messages}" \ No newline at end of file From 50605c594597fa5bcf470cbcd32421b865b1f820 Mon Sep 17 00:00:00 2001 From: "Wei Sun (Jack)" Date: Thu, 21 Aug 2025 10:03:55 -0700 Subject: [PATCH 4/8] Move new_message check to top --- src/google/adk/runners.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/google/adk/runners.py b/src/google/adk/runners.py index dfcd8f53f8..c1c3244793 100644 --- a/src/google/adk/runners.py +++ b/src/google/adk/runners.py @@ -200,6 +200,11 @@ async def run_async( The events generated by the agent. """ + if new_message.role != 'user': + raise ValueError( + f'new_message must have a role of "user", got {new_message.role}' + ) + async def _run_with_trace( new_message: types.Content, ) -> AsyncGenerator[Event, None]: @@ -209,10 +214,6 @@ async def _run_with_trace( ) if not session: raise ValueError(f'Session not found: {session_id}') - - # Validate new_message has a role - if not getattr(new_message, "role", None): - raise ValueError("new_message must have a role, but none was provided.") invocation_context = self._new_invocation_context( session, From cdba361fd839594ef291b4a88ca1ba4233aa4090 Mon Sep 17 00:00:00 2001 From: "Wei Sun (Jack)" Date: Thu, 21 Aug 2025 10:06:01 -0700 Subject: [PATCH 5/8] Update Runner.run_async doc. --- src/google/adk/runners.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/google/adk/runners.py b/src/google/adk/runners.py index c1c3244793..9859528a9e 100644 --- a/src/google/adk/runners.py +++ b/src/google/adk/runners.py @@ -198,6 +198,11 @@ async def run_async( Yields: The events generated by the agent. + + Raises: + ValueError: If `new_message` does not have a role of "user" or has no + parts. + ValueError: If the session is not found. """ if new_message.role != 'user': From ba85f7c9c542509596216cda0faeea1b89da4fd8 Mon Sep 17 00:00:00 2001 From: "Wei Sun (Jack)" Date: Thu, 18 Sep 2025 20:55:23 -0700 Subject: [PATCH 6/8] Update runners.py --- src/google/adk/runners.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/google/adk/runners.py b/src/google/adk/runners.py index 9e76709555..8288edf5c6 100644 --- a/src/google/adk/runners.py +++ b/src/google/adk/runners.py @@ -270,10 +270,8 @@ async def run_async( """ run_config = run_config or RunConfig() - if new_message.role != 'user': - raise ValueError( - f'new_message must have a role of "user", got {new_message.role}' - ) + if not new_message.role: + new_message.role = "user" async def _run_with_trace( new_message: types.Content, From c0b45f978a13f9015d000ab8f93ca78af57343b3 Mon Sep 17 00:00:00 2001 From: "Wei Sun (Jack)" Date: Thu, 18 Sep 2025 20:56:13 -0700 Subject: [PATCH 7/8] Update runners.py --- src/google/adk/runners.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/google/adk/runners.py b/src/google/adk/runners.py index 8288edf5c6..0465fcf354 100644 --- a/src/google/adk/runners.py +++ b/src/google/adk/runners.py @@ -264,8 +264,6 @@ async def run_async( The events generated by the agent. Raises: - ValueError: If `new_message` does not have a role of "user" or has no - parts. ValueError: If the session is not found. """ run_config = run_config or RunConfig() From 381b01418d249b9e6bd91ebb518ff25339a8e47b Mon Sep 17 00:00:00 2001 From: "Wei Sun (Jack)" Date: Thu, 18 Sep 2025 21:02:03 -0700 Subject: [PATCH 8/8] Update runners.py --- src/google/adk/runners.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/google/adk/runners.py b/src/google/adk/runners.py index 0465fcf354..8ec653ca50 100644 --- a/src/google/adk/runners.py +++ b/src/google/adk/runners.py @@ -269,7 +269,7 @@ async def run_async( run_config = run_config or RunConfig() if not new_message.role: - new_message.role = "user" + new_message.role = 'user' async def _run_with_trace( new_message: types.Content,