From d09d67d98d7926e379c464d6a4b1ae83ea53832a Mon Sep 17 00:00:00 2001 From: maru0804 Date: Tue, 13 Jan 2026 21:52:38 +0900 Subject: [PATCH 1/3] fix(planners): allow BuiltInPlanner subclasses to override process_planning_response Previously, isinstance(planner, BuiltInPlanner) caused all subclasses to be skipped in the response processor, preventing custom process_planning_response implementations from being called. This change uses method override detection instead of type checking, ensuring: - BuiltInPlanner itself is skipped (returns None) - Subclasses without override are skipped (no side effects) - Subclasses with override have their method called Fixes #4133 --- .../adk/flows/llm_flows/_nl_planning.py | 6 +- .../flows/llm_flows/test_nl_planning.py | 115 ++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) diff --git a/src/google/adk/flows/llm_flows/_nl_planning.py b/src/google/adk/flows/llm_flows/_nl_planning.py index c81648ea73..066c0f7d87 100644 --- a/src/google/adk/flows/llm_flows/_nl_planning.py +++ b/src/google/adk/flows/llm_flows/_nl_planning.py @@ -82,7 +82,11 @@ async def run_async( return planner = _get_planner(invocation_context) - if not planner or isinstance(planner, BuiltInPlanner): + if ( + not planner + or type(planner).process_planning_response + is BuiltInPlanner.process_planning_response + ): return # Postprocess the LLM response. diff --git a/tests/unittests/flows/llm_flows/test_nl_planning.py b/tests/unittests/flows/llm_flows/test_nl_planning.py index e4bdff7332..ec16f30b9f 100644 --- a/tests/unittests/flows/llm_flows/test_nl_planning.py +++ b/tests/unittests/flows/llm_flows/test_nl_planning.py @@ -14,11 +14,17 @@ """Unit tests for NL planning logic.""" +from typing import List +from typing import Optional from unittest.mock import MagicMock +from unittest.mock import patch +from google.adk.agents.callback_context import CallbackContext from google.adk.agents.llm_agent import Agent from google.adk.flows.llm_flows._nl_planning import request_processor +from google.adk.flows.llm_flows._nl_planning import response_processor from google.adk.models.llm_request import LlmRequest +from google.adk.models.llm_response import LlmResponse from google.adk.planners.built_in_planner import BuiltInPlanner from google.adk.planners.plan_re_act_planner import PlanReActPlanner from google.genai import types @@ -126,3 +132,112 @@ async def test_remove_thought_from_request_with_thoughts(): for content in llm_request.contents for part in content.parts or [] ) + + +class OverriddenBuiltInPlanner(BuiltInPlanner): + """Subclass that overrides process_planning_response.""" + + def __init__(self, *, thinking_config: types.ThinkingConfig): + super().__init__(thinking_config=thinking_config) + self.process_planning_response_called = False + self.received_parts = None + + def process_planning_response( + self, + callback_context: CallbackContext, + response_parts: List[types.Part], + ) -> Optional[List[types.Part]]: + self.process_planning_response_called = True + self.received_parts = response_parts + return response_parts + + +class NonOverriddenBuiltInPlanner(BuiltInPlanner): + """Subclass that does NOT override process_planning_response.""" + + pass + + +@pytest.mark.asyncio +async def test_overridden_subclass_process_planning_response_called(): + """Test that subclasses overriding process_planning_response have it called. + + Regression test for issue #4133. + """ + planner = OverriddenBuiltInPlanner(thinking_config=types.ThinkingConfig()) + agent = Agent(name='test_agent', planner=planner) + invocation_context = await testing_utils.create_invocation_context( + agent=agent, user_content='test message' + ) + + response_parts = [ + types.Part(text='thinking...', thought=True), + types.Part(text='Here is my response'), + ] + llm_response = LlmResponse( + content=types.Content(role='model', parts=response_parts) + ) + + async for _ in response_processor.run_async(invocation_context, llm_response): + pass + + assert planner.process_planning_response_called + assert planner.received_parts == response_parts + + +@pytest.mark.asyncio +async def test_base_builtin_planner_process_planning_response_not_called(): + """Test that base BuiltInPlanner does not have process_planning_response called.""" + planner = BuiltInPlanner(thinking_config=types.ThinkingConfig()) + agent = Agent(name='test_agent', planner=planner) + invocation_context = await testing_utils.create_invocation_context( + agent=agent, user_content='test message' + ) + + response_parts = [ + types.Part(text='thinking...', thought=True), + types.Part(text='Here is my response'), + ] + llm_response = LlmResponse( + content=types.Content(role='model', parts=response_parts) + ) + + with patch.object( + BuiltInPlanner, + 'process_planning_response', + wraps=planner.process_planning_response, + ) as mock_method: + async for _ in response_processor.run_async( + invocation_context, llm_response + ): + pass + mock_method.assert_not_called() + + +@pytest.mark.asyncio +async def test_non_overridden_subclass_process_planning_response_not_called(): + """Test that subclasses NOT overriding process_planning_response are skipped.""" + planner = NonOverriddenBuiltInPlanner(thinking_config=types.ThinkingConfig()) + agent = Agent(name='test_agent', planner=planner) + invocation_context = await testing_utils.create_invocation_context( + agent=agent, user_content='test message' + ) + + response_parts = [ + types.Part(text='thinking...', thought=True), + types.Part(text='Here is my response'), + ] + llm_response = LlmResponse( + content=types.Content(role='model', parts=response_parts) + ) + + with patch.object( + BuiltInPlanner, + 'process_planning_response', + wraps=planner.process_planning_response, + ) as mock_method: + async for _ in response_processor.run_async( + invocation_context, llm_response + ): + pass + mock_method.assert_not_called() From 20a089d4642119361fb447b9a09d350e3a4e6c8e Mon Sep 17 00:00:00 2001 From: maru0804 Date: Tue, 13 Jan 2026 22:00:17 +0900 Subject: [PATCH 2/3] refactor(tests): combine duplicate tests using pytest.mark.parametrize Address review feedback by combining test_base_builtin_planner_process_planning_response_not_called and test_non_overridden_subclass_process_planning_response_not_called into a single parameterized test to reduce code duplication. --- .../flows/llm_flows/test_nl_planning.py | 42 +++++-------------- 1 file changed, 10 insertions(+), 32 deletions(-) diff --git a/tests/unittests/flows/llm_flows/test_nl_planning.py b/tests/unittests/flows/llm_flows/test_nl_planning.py index ec16f30b9f..347a2ede8f 100644 --- a/tests/unittests/flows/llm_flows/test_nl_planning.py +++ b/tests/unittests/flows/llm_flows/test_nl_planning.py @@ -186,38 +186,16 @@ async def test_overridden_subclass_process_planning_response_called(): @pytest.mark.asyncio -async def test_base_builtin_planner_process_planning_response_not_called(): - """Test that base BuiltInPlanner does not have process_planning_response called.""" - planner = BuiltInPlanner(thinking_config=types.ThinkingConfig()) - agent = Agent(name='test_agent', planner=planner) - invocation_context = await testing_utils.create_invocation_context( - agent=agent, user_content='test message' - ) - - response_parts = [ - types.Part(text='thinking...', thought=True), - types.Part(text='Here is my response'), - ] - llm_response = LlmResponse( - content=types.Content(role='model', parts=response_parts) - ) - - with patch.object( - BuiltInPlanner, - 'process_planning_response', - wraps=planner.process_planning_response, - ) as mock_method: - async for _ in response_processor.run_async( - invocation_context, llm_response - ): - pass - mock_method.assert_not_called() - - -@pytest.mark.asyncio -async def test_non_overridden_subclass_process_planning_response_not_called(): - """Test that subclasses NOT overriding process_planning_response are skipped.""" - planner = NonOverriddenBuiltInPlanner(thinking_config=types.ThinkingConfig()) +@pytest.mark.parametrize( + 'planner_class', + [BuiltInPlanner, NonOverriddenBuiltInPlanner], + ids=['base_class', 'non_overridden_subclass'], +) +async def test_process_planning_response_not_called_without_override( + planner_class, +): + """Test that process_planning_response is not called for base or non-overridden subclasses.""" + planner = planner_class(thinking_config=types.ThinkingConfig()) agent = Agent(name='test_agent', planner=planner) invocation_context = await testing_utils.create_invocation_context( agent=agent, user_content='test message' From c945c0459f3dda429b1fa85f729ae8a6c25704dc Mon Sep 17 00:00:00 2001 From: maru0804 Date: Tue, 13 Jan 2026 22:07:11 +0900 Subject: [PATCH 3/3] refactor(tests): remove unnecessary wraps parameter in patch.object Address review feedback: wraps is not needed since BuiltInPlanner.process_planning_response simply returns None. A standard MagicMock is sufficient for assert_not_called(). --- tests/unittests/flows/llm_flows/test_nl_planning.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unittests/flows/llm_flows/test_nl_planning.py b/tests/unittests/flows/llm_flows/test_nl_planning.py index 347a2ede8f..53a1b8d05a 100644 --- a/tests/unittests/flows/llm_flows/test_nl_planning.py +++ b/tests/unittests/flows/llm_flows/test_nl_planning.py @@ -212,7 +212,6 @@ async def test_process_planning_response_not_called_without_override( with patch.object( BuiltInPlanner, 'process_planning_response', - wraps=planner.process_planning_response, ) as mock_method: async for _ in response_processor.run_async( invocation_context, llm_response