Repository navigation
feat(tools): add propagate_conversation_history option to AgentTool - #6020
GiulioCMSanto wants to merge 3 commits into
Conversation
|
Response from ADK Triaging Agent Hello @GiulioCMSanto, thank you for creating this PR! It looks like a great addition to While you have provided an excellent description of your manual E2E test, to fully align with our contribution guidelines, could you please provide:
This information will help reviewers verify and understand the changes more efficiently. Thank you! |
|
I will start by reading the instructions in the The complete and detailed pull request analysis is available in the created artifact: Executive Summary of the PR
|
|
Hi @GiulioCMSanto , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you please fix the failing mypy-diff tests before we can proceed with the review. |
rohityan
added
the
request clarification
Done @rohityan ! Thank you for considering the PR 😄 |
There was a problem hiding this comment.
Hi @GiulioCMSanto, thanks for puting together this PR to add conversational history propagation! It’s a great addition for improving multi-turn awareness in tool agents.
To prevent potential token overflow or context-window issues in longer sessions, I've left a few suggestions to introduce an optional "max_history_messages" configuration parameter. This allows users to cap the injected history to the most recent N eligible turns.
I've also appended a unit test to verify the capping logic and parameter forwarding.
| if self.propagate_conversation_history: | ||
| await _inject_conversation_history( | ||
| tool_context, runner.session_service, session | ||
| ) |
There was a problem hiding this comment.
Pass the configured "max_history_messages" limit down into the helper function when injecting the history.
| if self.propagate_conversation_history: | |
| await _inject_conversation_history( | |
| tool_context, runner.session_service, session | |
| ) | |
| if self.propagate_conversation_history: | |
| await _inject_conversation_history( | |
| tool_context, | |
| runner.session_service, | |
| session, | |
| max_history_messages=self.max_history_messages, | |
| ) |
| *, | ||
| include_plugins: bool = True, | ||
| propagate_grounding_metadata: bool = False, | ||
| propagate_conversation_history: bool = False, |
There was a problem hiding this comment.
Accept max_history_messages as an optional configuration keyword argument in the AgentTool constructor.
| propagate_conversation_history: bool = False, | |
| propagate_conversation_history: bool = False, | |
| max_history_messages: Optional[int] = None, |
| self.skip_summarization: bool = skip_summarization | ||
| self.include_plugins = include_plugins | ||
| self.propagate_grounding_metadata = propagate_grounding_metadata | ||
| self.propagate_conversation_history = propagate_conversation_history |
There was a problem hiding this comment.
Store max_history_messages as an instance variable on the AgentTool.
| self.propagate_conversation_history = propagate_conversation_history | |
| self.propagate_conversation_history = propagate_conversation_history | |
| self.max_history_messages = max_history_messages |
| *, | ||
| include_plugins: bool = True, | ||
| propagate_grounding_metadata: bool = False, | ||
| propagate_conversation_history: bool = False, |
There was a problem hiding this comment.
Expose the max_history_messages parameter on the _TaskAgentTool subclass constructor.
| propagate_conversation_history: bool = False, | |
| propagate_conversation_history: bool = False, | |
| max_history_messages: Optional[int] = None, |
| skip_summarization, | ||
| include_plugins=include_plugins, | ||
| propagate_grounding_metadata=propagate_grounding_metadata, | ||
| propagate_conversation_history=propagate_conversation_history, |
There was a problem hiding this comment.
Forward the max_history_messages argument to the base AgentTool constructor via super().init.
| propagate_conversation_history=propagate_conversation_history, | |
| propagate_conversation_history=propagate_conversation_history, | |
| max_history_messages=max_history_messages, |
| async def _inject_conversation_history( | ||
| tool_context: ToolContext, | ||
| session_service: BaseSessionService, | ||
| session: Session, | ||
| ) -> None: |
There was a problem hiding this comment.
Add the optional max_history_messages parameter to the history injection helper function.
| async def _inject_conversation_history( | |
| tool_context: ToolContext, | |
| session_service: BaseSessionService, | |
| session: Session, | |
| ) -> None: | |
| async def _inject_conversation_history( | |
| tool_context: ToolContext, | |
| session_service: BaseSessionService, | |
| session: Session, | |
| max_history_messages: Optional[int] = None, | |
| ) -> None: |
| Args: | ||
| tool_context: The parent tool context containing the session to read | ||
| from. | ||
| session_service: The child runner's session service for appending | ||
| events. | ||
| session: The child session to inject events into. |
There was a problem hiding this comment.
Update max_history_messages argument in the helper function's docstring.
| Args: | |
| tool_context: The parent tool context containing the session to read | |
| from. | |
| session_service: The child runner's session service for appending | |
| events. | |
| session: The child session to inject events into. | |
| Args: | |
| tool_context: The parent tool context containing the session to read from. | |
| session_service: The child runner's session service for appending events. | |
| session: The child session to inject events into. | |
| max_history_messages: The maximum number of recent conversation turns to inject. |
| for parent_event in tool_context.session.events: | ||
| if not parent_event.content or not parent_event.content.parts: | ||
| continue | ||
| if parent_event.content.role not in ('user', 'model'): | ||
| continue | ||
| text_parts = [ | ||
| part | ||
| for part in parent_event.content.parts | ||
| if part.text and not part.thought | ||
| ] | ||
| if not text_parts: | ||
| continue | ||
| await session_service.append_event( | ||
| session, | ||
| Event( | ||
| author=parent_event.author, | ||
| content=types.Content( | ||
| role=parent_event.content.role, | ||
| parts=text_parts, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Gather and filter the eligible history events first, then slice them to match the configured limit before injecting.
| for parent_event in tool_context.session.events: | |
| if not parent_event.content or not parent_event.content.parts: | |
| continue | |
| if parent_event.content.role not in ('user', 'model'): | |
| continue | |
| text_parts = [ | |
| part | |
| for part in parent_event.content.parts | |
| if part.text and not part.thought | |
| ] | |
| if not text_parts: | |
| continue | |
| await session_service.append_event( | |
| session, | |
| Event( | |
| author=parent_event.author, | |
| content=types.Content( | |
| role=parent_event.content.role, | |
| parts=text_parts, | |
| ), | |
| ), | |
| ) | |
| eligible_events = [] | |
| for parent_event in tool_context.session.events: | |
| if not parent_event.content or not parent_event.content.parts: | |
| continue | |
| if parent_event.content.role not in ('user', 'model'): | |
| continue | |
| text_parts = [ | |
| part | |
| for part in parent_event.content.parts | |
| if part.text and not part.thought | |
| ] | |
| if not text_parts: | |
| continue | |
| eligible_events.append((parent_event, text_parts)) | |
| if max_history_messages is not None: | |
| eligible_events = eligible_events[-max_history_messages:] | |
| for parent_event, text_parts in eligible_events: | |
| await session_service.append_event( | |
| session, | |
| Event( | |
| author=parent_event.author, | |
| content=types.Content( | |
| role=parent_event.content.role, | |
| parts=text_parts, | |
| ), | |
| ), | |
| ) |
| for part in content.parts | ||
| if part.text | ||
| ] | ||
| assert 'I need 2 bedrooms' in user_texts |
There was a problem hiding this comment.
| assert 'I need 2 bedrooms' in user_texts | |
| assert 'I need 2 bedrooms' in user_texts | |
| @mark.asyncio | |
| async def test_propagate_history_with_max_messages_caps_injection(monkeypatch): | |
| """Setting max_history_messages limits injected history to the most recent turns.""" | |
| child_session_events: list[Event] = [] | |
| class RecordingSessionService(InMemorySessionService): | |
| async def append_event(self, session, event): | |
| child_session_events.append(event) | |
| return await super().append_event(session, event) | |
| monkeypatch.setattr( | |
| 'google.adk.sessions.in_memory_session_service.InMemorySessionService', | |
| RecordingSessionService, | |
| ) | |
| tool_agent = Agent(name='tool_agent', model='test-model') | |
| # Limit history to the 2 most recent messages | |
| agent_tool = AgentTool( | |
| agent=tool_agent, | |
| propagate_conversation_history=True, | |
| max_history_messages=2, | |
| ) | |
| root_agent = Agent(name='root_agent', model='test-model', tools=[agent_tool]) | |
| parent_session_service = InMemorySessionService() | |
| parent_session = await parent_session_service.create_session( | |
| app_name='test_app', user_id='user' | |
| ) | |
| # Add 3 historical text events | |
| messages = ['turn 1', 'turn 2', 'turn 3'] | |
| for i, msg in enumerate(messages): | |
| await parent_session_service.append_event( | |
| parent_session, | |
| Event( | |
| author='user' if i % 2 == 0 else 'root_agent', | |
| content=types.Content( | |
| role='user' if i % 2 == 0 else 'model', | |
| parts=[types.Part.from_text(text=msg)], | |
| ), | |
| ), | |
| ) | |
| async def _empty_async_generator(): | |
| if False: | |
| yield None | |
| class StubRunner: | |
| def __init__(self, **kwargs): | |
| self.session_service = kwargs.get('session_service') | |
| from ..plugins.plugin_manager import PluginManager | |
| self.plugin_manager = PluginManager(plugins=kwargs.get('plugins')) | |
| def run_async(self, *args, **kwargs): | |
| return _empty_async_generator() | |
| async def close(self): | |
| pass | |
| monkeypatch.setattr('google.adk.runners.Runner', StubRunner) | |
| invocation_context = InvocationContext( | |
| invocation_id='inv-id', | |
| agent=root_agent, | |
| session=parent_session, | |
| session_service=parent_session_service, | |
| memory_service=InMemoryMemoryService(), | |
| plugin_manager=PluginManager(), | |
| run_config=RunConfig(), | |
| ) | |
| tool_context = ToolContext(invocation_context) | |
| await agent_tool.run_async( | |
| args={'request': 'do something'}, tool_context=tool_context | |
| ) | |
| # Only the last 2 turns should have been injected | |
| assert len(child_session_events) == 2 | |
| assert child_session_events[0].content.parts[0].text == 'turn 2' | |
| assert child_session_events[1].content.parts[0].text == 'turn 3' |
Hey @godwin3737!!! Thank you so much for the suggestion! Let me add a discussion here! For context-window management, ADK already has the compaction system (EventsCompactionConfig) that handles summarization and truncation at the session level. Since the injected events come from the parent session, they're already subject to that mechanism. Given that, should we still insert max_history_messages or should we leave it to a follow up discussion for a future PR, and keep this one as-is? What's your take on that as well @rohityan ? If you still think it make sense, I apply the changes right away. |
Hey @GiulioCMSanto , great point about the parent's EventsCompactionConfig! My main thought here is about the boundary between the parent and the child. Even if the parent is compacted, its history might still be quite long (e.g., 20+ turns). If we propagate all of that into the child, we either bloat the child's token usage on every tool call, or we force the child's own EventsCompactionConfig to immediately trigger and truncate the injected messages. Relying on post-injection compaction could also introduces unnecessary latency, as the framework has to process (or even summarize) that large history block before the child agent can run. Having max_history_messages act as a pre-injection filter seems efficient. It ensures we only send the immediate context (like the last 2-3 turns) across the boundary, saving tokens and latency without relying on post-injection compaction. Let me know what you and @rohityan think! |
Let's wait for @i-yliu's review as well, now that we have an assignee!! |
5 participants
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Testing Plan
Unit Tests:
New tests added to
tests/unittests/tools/test_agent_tool.py:test_propagate_history_injects_user_and_model_eventstest_propagate_history_excludes_function_call_eventstest_propagate_history_creates_events_without_state_deltastate_delta(prevents child state corruption)test_propagate_history_disabled_by_defaultFalsebehavior is unchanged — zero history in childtest_propagate_history_child_sees_context_end_to_endManual End-to-End (E2E) Tests:
Tested with a multi-turn conversational agent where a parent routes to a tool agent for filter extraction. Without
propagate_conversation_history=True, the child misclassifies user constraints (treats mandatory requirements as soft preferences). With it enabled, the child correctly identifies non-negotiable requirements from the full conversation context.Checklist
Additional context
Implementation follows the
propagate_grounding_metadatapattern exactly:propagate_conversation_history: bool = FalseonAgentTool.__init___inject_conversation_historymodule-level helper (~25 lines)run_asyncaftercreate_session_TaskAgentTool.__init__Not modified (intentionally):
_SingleTurnAgentTool— usesrun_node(parent already shares context)AgentToolConfig/from_config— matching precedent (propagate_grounding_metadatadidn't wire through config either)Design decisions:
False— existing behavior unchanged for all current users.user/modeltext events are injected —function_call/function_responseevents are excluded because they reference tools that only exist in the parent runner.Eventobjects are created — preventsstate_deltafrom original events being replayed in the child session.thoughtparts are filtered out — internal reasoning shouldn't leak to the child.