-
Notifications
You must be signed in to change notification settings - Fork 33
Improved handling of FinishReason to avoid deserialization errors when Google introduces new values
#98
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
📝 WalkthroughWalkthroughThe pull request extends the FinishReason enum with eight new values (MODEL_ARMOR, IMAGE_PROHIBITED_CONTENT, IMAGE_RECITATION, IMAGE_OTHER, UNEXPECTED_TOOL_CALL, NO_IMAGE, TOO_MANY_TOOL_CALLS), introduces a lenient JSON converter that gracefully handles unknown enum values during deserialization, and updates error message handling for the new cases with corresponding unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (3)
tests/GenerativeAI.Tests/Converters/LenientFinishReasonConverter_Tests.cs (1)
7-31: Good test coverage for core scenarios.The tests cover the main use cases effectively. Consider adding tests for edge cases that the converter handles:
- Empty string input → should return
FINISH_REASON_UNSPECIFIED- Non-string JSON token (e.g., number) → should throw
JsonExceptionThese cases are implemented in the converter but not verified by tests.
🔎 Suggested additional tests
+ [Fact] + public void Read_EmptyString_ReturnsUnspecified() + { + var result = JsonSerializer.Deserialize<FinishReason>("\"\""); + result.ShouldBe(FinishReason.FINISH_REASON_UNSPECIFIED); + } + + [Fact] + public void Read_NonStringToken_ThrowsJsonException() + { + Should.Throw<JsonException>(() => JsonSerializer.Deserialize<FinishReason>("123")); + }src/GenerativeAI/Types/Converters/LenientFinishReasonConverter.cs (2)
14-16: Add complete XML documentation forReadmethod.As per coding guidelines, C# code should include comprehensive XML documentation. The method summary is present but
<param>and<returns>elements are missing.🔎 Suggested documentation
/// <summary> /// Reads and converts the JSON to FinishReason. /// </summary> + /// <param name="reader">The reader to read JSON from.</param> + /// <param name="typeToConvert">The type to convert to.</param> + /// <param name="options">Serialization options.</param> + /// <returns>The deserialized <see cref="FinishReason"/> value.</returns> + /// <exception cref="JsonException">Thrown when the token is not a string.</exception> public override FinishReason Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
38-40: Add complete XML documentation forWritemethod.🔎 Suggested documentation
/// <summary> /// Writes the FinishReason value as JSON. /// </summary> + /// <param name="writer">The writer to write JSON to.</param> + /// <param name="value">The <see cref="FinishReason"/> value to serialize.</param> + /// <param name="options">Serialization options.</param> public override void Write(Utf8JsonWriter writer, FinishReason value, JsonSerializerOptions options)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/GenerativeAI/Core/ResponseHelper.cssrc/GenerativeAI/Types/ContentGeneration/Outputs/FinishReason.cssrc/GenerativeAI/Types/Converters/LenientFinishReasonConverter.cstests/GenerativeAI.Tests/Converters/LenientFinishReasonConverter_Tests.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
C# code should use latest language features, enable nullable reference types, and include comprehensive XML documentation
Files:
tests/GenerativeAI.Tests/Converters/LenientFinishReasonConverter_Tests.cssrc/GenerativeAI/Types/Converters/LenientFinishReasonConverter.cssrc/GenerativeAI/Core/ResponseHelper.cssrc/GenerativeAI/Types/ContentGeneration/Outputs/FinishReason.cs
🧬 Code graph analysis (2)
src/GenerativeAI/Core/ResponseHelper.cs (1)
src/GenerativeAI/Types/Converters/LenientFinishReasonConverter.cs (1)
FinishReason(17-36)
src/GenerativeAI/Types/ContentGeneration/Outputs/FinishReason.cs (1)
src/GenerativeAI/Types/Converters/LenientFinishReasonConverter.cs (1)
LenientFinishReasonConverter(12-45)
⏰ 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 (4)
src/GenerativeAI/Core/ResponseHelper.cs (1)
76-82: LGTM!The new finish reason cases are properly handled with clear, descriptive messages that align with the enum documentation. The existing default case at line 83 provides a safety net for any additional future values.
src/GenerativeAI/Types/Converters/LenientFinishReasonConverter.cs (1)
17-36: LGTM!The converter logic is solid:
- Correctly validates token type before reading
- Handles empty/null strings by returning
FINISH_REASON_UNSPECIFIED- Uses case-insensitive parsing for flexibility
- Gracefully falls back to
OTHERfor unknown valuesThis prevents deserialization crashes when Google introduces new values.
src/GenerativeAI/Types/ContentGeneration/Outputs/FinishReason.cs (2)
9-15: LGTM!Excellent documentation update. The remarks section clearly explains the lenient converter behavior, and the
JsonConverterattribute switch toLenientFinishReasonConverterenables graceful handling of unknown enum values.
78-119: Well-structured enum expansion.The new enum values are properly implemented:
- Explicit numeric values (12-18) ensure binary/serialization compatibility
- Sequential numbering prevents gaps
- Each value has comprehensive XML documentation with API source references
- Consistent naming convention with existing values
FinishReasonenum to include additional values documented across Google’s APIs (including Vertex AI image/model-armor/tooling related reasons).LenientFinishReasonConverter) that parses known values case-insensitively and falls back toFinishReason.OTHERfor unknown strings instead of throwingSystem.Text.Json.JsonException.ResponseHelper.FormatErrorMessageto cover the newly added finish reasons.Net effect: streaming and non-streaming responses no longer crash on previously unknown
finishReasonvalues, while preserving existing behavior for all known reasons.Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.