-
Notifications
You must be signed in to change notification settings - Fork 35
Add thought signature support for Microsoft.Extensions.AI #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Maps Part.ThoughtSignature to FunctionCallContent.AdditionalProperties (and TextContent) to enable bidirectional conversion of thought signatures.
Maps Part.ThoughtSignature to FunctionCallContent.AdditionalProperties (and TextContent) to enable bidirectional conversion of thought signatures. Includes unit tests.
|
Caution Review failedThe pull request is closed. WalkthroughThe changes extend content conversion methods in the Microsoft Generative AI extension to propagate ThoughtSignature metadata. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/GenerativeAI.Microsoft.Tests/MicrosoftExtension_Tests.cs (1)
313-407: Consider adding edge case tests (optional).The current tests provide good coverage of the main scenarios. For even more robust testing, you might consider adding tests for:
- Null or empty ThoughtSignature values
- Multiple properties in AdditionalProperties alongside ThoughtSignature
- Parts with no ThoughtSignature (ensuring AdditionalProperties isn't created unnecessarily)
However, the current implementation handles these cases correctly, so this is more of a nice-to-have for comprehensive test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/GenerativeAI.Microsoft/AdditionalPropertyKeys.cs(1 hunks)src/GenerativeAI.Microsoft/Extensions/MicrosoftExtensions.cs(3 hunks)tests/GenerativeAI.Microsoft.Tests/MicrosoftExtension_Tests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/GenerativeAI.Microsoft/Extensions/MicrosoftExtensions.cs (3)
src/GenerativeAI.Microsoft/AdditionalPropertyKeys.cs (1)
AdditionalPropertiesKeys(6-32)src/GenerativeAI/Extensions/GenerateContentResponseExtensions.cs (2)
Text(18-46)FunctionCall(53-71)src/GenerativeAI/Types/ContentGeneration/Tools/FunctionCalling/FunctionCall.cs (3)
FunctionCall(12-48)FunctionCall(38-41)FunctionCall(46-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (9.0.x)
🔇 Additional comments (9)
src/GenerativeAI.Microsoft/AdditionalPropertyKeys.cs (1)
27-31: LGTM! Clean addition of ThoughtSignature constant.The new constant follows the established pattern and is properly documented.
src/GenerativeAI.Microsoft/Extensions/MicrosoftExtensions.cs (4)
147-187: LGTM! Proper null-safe propagation of ThoughtSignature.The implementation correctly extracts ThoughtSignature from
content.AdditionalPropertiesand assigns it topart.ThoughtSignaturewith appropriate null checks.
580-589: LGTM! Clean propagation of ThoughtSignature to TextContent.The implementation properly checks for null and initializes
AdditionalPropertiesonly when needed.
591-601: LGTM! Consistent ThoughtSignature propagation for FunctionCallContent.The implementation follows the same pattern as TextContent, ensuring consistency across content types.
603-607: Based on the official Gemini API documentation, I can now provide a definitive rewritten review comment:
FunctionResultContent correctly does not propagate ThoughtSignature.
The Gemini API documentation clarifies that ThoughtSignature is an encrypted token returned by the model exclusively with model-generated
functionCallparts. It must be passed back in subsequent requests to maintain continuity. However,functionResponse(user-provided function results) is not model-generated content and therefore should not include ThoughtSignature. The current implementation is correct:FunctionCallContentpropagates ThoughtSignature (model output), whileFunctionResultContentdoes not (user input).tests/GenerativeAI.Microsoft.Tests/MicrosoftExtension_Tests.cs (4)
313-336: LGTM! Comprehensive test for FunctionCallContent ThoughtSignature propagation.The test properly verifies that ThoughtSignature flows from Part to FunctionCallContent through AdditionalProperties.
338-362: LGTM! Proper verification of reverse conversion.This test correctly verifies that ThoughtSignature flows from FunctionCallContent.AdditionalProperties back to Part.ThoughtSignature.
364-387: LGTM! Good coverage for TextContent ThoughtSignature propagation.The test ensures ThoughtSignature is correctly propagated for text responses.
389-407: LGTM! Complete round-trip coverage for TextContent.This test verifies the reverse conversion, ensuring ThoughtSignature is properly preserved in both directions.
From Gemini 3.0 onwards, it will be mandatory to assign thought signatures when performing function calls.
https://ai.google.dev/gemini-api/docs/thought-signatures#how_it_works
This poses a significant problem as thought signatures are lost in
Google_GenerativeAI.Microsoft.Consequently, we have implemented a solution whereby thought signatures are set within the AdditionalProperties of AIContent.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.