feat(gemini): add thought signature preservation for thinking models#1622
feat(gemini): add thought signature preservation for thinking models#1622pgrayy wants to merge 1 commit intostrands-agents:mainfrom
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
pgrayy
force-pushed
the
gemini-thought-signature
branch
from
192e3c7 to
2811c58
Compare
pgrayy
force-pushed
the
gemini-thought-signature
branch
from
2811c58 to
6450f71
Compare
|
/strands review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| current_tool_use["name"] = tool_use_data["name"] | ||
| current_tool_use["input"] = "" | ||
| if "reasoningSignature" in tool_use_data: | ||
| current_tool_use["reasoningSignature"] = tool_use_data["reasoningSignature"] |
There was a problem hiding this comment.
Issue: Missing test coverage for reasoningSignature handling.
Codecov reports 0% patch coverage for the streaming.py changes. The existing parameterized tests for handle_content_block_start and handle_content_block_stop should be extended to cover the reasoningSignature field.
Suggestion: Add test cases to the existing parameterized tests in test_streaming.py:
# In test_handle_content_block_start parameters:
(
{"start": {"toolUse": {"toolUseId": "test", "name": "test", "reasoningSignature": "YWJj"}}},
{"toolUseId": "test", "name": "test", "input": "", "reasoningSignature": "YWJj"},
),
# In test_handle_content_block_stop parameters (with reasoningSignature):
(
{
"content": [],
"current_tool_use": {"toolUseId": "123", "name": "test", "input": '{}', "reasoningSignature": "YWJj"},
"text": "",
"reasoningText": "",
"citationsContent": [],
"redactedContent": b"",
},
{
"content": [{"toolUse": {"toolUseId": "123", "name": "test", "input": {}, "reasoningSignature": "YWJj"}}],
...
},
),
|
Issue: PR description is incomplete. Please update the PR description to include:
This helps reviewers understand the context and purpose of the changes. |
Review SummaryAssessment: Request Changes OverviewThis PR adds thought signature preservation for Gemini thinking models by:
Key ThemesTest Coverage Gap (Important) PR Documentation What Looks Good
Requested Changes
Overall the implementation looks solid - just needs the test coverage addressed before merging. |
| tools = [] | ||
|
|
||
| # Only add function declarations tool if there are tool specs | ||
| if tool_specs: | ||
| tools.append( | ||
| genai.types.Tool( | ||
| function_declarations=[ | ||
| genai.types.FunctionDeclaration( | ||
| description=tool_spec["description"], | ||
| name=tool_spec["name"], | ||
| parameters_json_schema=tool_spec["inputSchema"]["json"], | ||
| ) | ||
| for tool_spec in tool_specs | ||
| ], | ||
| ), | ||
| ) | ||
|
|
There was a problem hiding this comment.
I don't think this change is adding any value. It's just a more "explicit" way to do the same thing.
| # Only include tools parameter if there are actual tools to pass | ||
| if tools: | ||
| config_kwargs["tools"] = tools | ||
|
|
There was a problem hiding this comment.
What happens if we continue doing what we are doing (sending over tools=[])?
| if event["data"].thought_signature: | ||
| tool_use_start["reasoningSignature"] = base64.b64encode( | ||
| event["data"].thought_signature | ||
| ).decode("ascii") |
There was a problem hiding this comment.
Why is the decode here different than the decode earlier at _format_request_content_part
4 participants
Description
Related Issues
Documentation PR
Type of Change
Bug fix
New feature
Breaking change
Documentation update
Other (please describe):
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.