fix(a2a): Preserve thought metadata in bidirectional part conversions #4241
Conversation
adk-bot
added
the
core
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to preserve thought metadata during A2A to GenAI part conversions. The changes correctly handle the A2A to GenAI direction by extracting thought from metadata and passing it to the genai_types.Part constructors. The accompanying unit tests verify this for several part types.
However, the fix is incomplete for a full round-trip conversion. The reverse conversion (GenAI to A2A) in convert_genai_part_to_a2a_part does not preserve the thought field for most part types, which contradicts the goal of the PR. I've added comments in the test files suggesting adding more comprehensive round-trip tests to expose and address this issue. Additionally, test coverage for the current changes could be improved by adding tests for all DataPart variants.
| def test_text_part_with_thought_round_trip(self): | ||
| """Test round-trip conversion for text parts with thought=True.""" | ||
| # Arrange - Start with GenAI part with thought=True | ||
| original_text = "I'm reasoning about this problem..." | ||
| genai_part = genai_types.Part(text=original_text, thought=True) | ||
|
|
||
| # Act - Round trip: GenAI -> A2A -> GenAI | ||
| a2a_part = convert_genai_part_to_a2a_part(genai_part) | ||
| result_genai_part = convert_a2a_part_to_genai_part(a2a_part) | ||
|
|
||
| # Assert | ||
| assert result_genai_part is not None | ||
| assert isinstance(result_genai_part, genai_types.Part) | ||
| assert result_genai_part.text == original_text | ||
| assert result_genai_part.thought is True |
There was a problem hiding this comment.
These round-trip tests for TextPart are great for ensuring thought is preserved. However, the fix for preserving thought seems incomplete for the full round-trip. The convert_genai_part_to_a2a_part function only handles thought for TextPart and will drop it for other part types like FilePart and DataPart.
I recommend adding similar round-trip tests for other part types (e.g., test_file_part_with_thought_round_trip) to expose this bug. Such a test would start with a genai_types.Part containing file_data and thought=True, convert it to A2A and back, and then assert that thought is still True in the final genai_types.Part. This would currently fail and help ensure the fix is complete for all scenarios.
There was a problem hiding this comment.
can you please address this.
| def test_convert_data_part_function_call_with_thought_metadata(self): | ||
| """Test conversion of A2A DataPart with function call and thought metadata.""" | ||
| # Arrange | ||
| function_call_data = { | ||
| "name": "test_function", | ||
| "args": {"param1": "value1"}, | ||
| } | ||
| a2a_part = a2a_types.Part( | ||
| root=a2a_types.DataPart( | ||
| data=function_call_data, | ||
| metadata={ | ||
| _get_adk_metadata_key( | ||
| A2A_DATA_PART_METADATA_TYPE_KEY | ||
| ): A2A_DATA_PART_METADATA_TYPE_FUNCTION_CALL, | ||
| _get_adk_metadata_key("thought"): True, | ||
| }, | ||
| ) | ||
| ) | ||
|
|
||
| # Act | ||
| result = convert_a2a_part_to_genai_part(a2a_part) | ||
|
|
||
| # Assert | ||
| assert result is not None | ||
| assert result.function_call is not None | ||
| assert result.function_call.name == "test_function" | ||
| assert result.thought is True |
There was a problem hiding this comment.
This is a good test for function_call with thought metadata. However, the changes in part_converter.py also add thought preservation for other DataPart types like function_response, code_execution_result, and executable_code. To ensure full test coverage for the changes, please consider adding similar tests for these other DataPart types.
ryanaiagent
added
a2a
|
Hi @filipecaixeta , Thank you for your contribution! We appreciate you taking the time to submit this pull request. |
ryanaiagent
added
the
request clarification
filipecaixeta
changed the title
adk-bot
added
the
core
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):
#4242
2. Or, if no issue exists, describe the change:
Problem:
The part converter in
part_converter.pyhad asymmetric handling ofthoughtmetadata:A2A → GenAI (
convert_a2a_part_to_genai_part): Metadata was completely ignored, sothoughtwas alwaysNonefor all part types.GenAI → A2A (
convert_genai_part_to_a2a_part):thoughtwas only preserved forTextPart, but dropped forFilePart(file_data, inline_data) and allDataParttypes (function_call, function_response, code_execution_result, executable_code).This breaks multi-agent scenarios where reasoning/thought content needs to survive round-trip conversions between agents.
Solution:
A2A → GenAI: Extract
thoughtfrom part metadata at the beginning ofconvert_a2a_part_to_genai_partand pass it to allgenai_types.Part()constructors.GenAI → A2A: Add
thoughtmetadata preservation to all part types inconvert_genai_part_to_a2a_part:FilePartwithfile_dataFilePartwithinline_data(merged with existingvideo_metadatahandling)DataPartforfunction_call,function_response,code_execution_result,executable_codeTesting Plan
Unit Tests:
Manual End-to-End (E2E) Tests:
The fix can be verified by setting up two agents communicating via A2A where the sub-agent generates content with
thought=True. Before this fix, the parent agent would receive the content withthought=None. After this fix,thought=Trueis preserved through the full round-trip.Checklist
Additional context
Files changed:
src/google/adk/a2a/converters/part_converter.py- Added bidirectional thought metadata preservationtests/unittests/a2a/converters/test_part_converter.py- Added 17 new tests for thought preservation