Fix/content block code execution result#166
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a deserialization issue where the bash_code_execution_tool_result content block was incorrectly mapped as a list when it should be a single object. The fix introduces a specialized content block class with a custom converter to handle this case correctly during streaming operations. Additionally, the PR adds an ExpiresAt field to the container response.
Key Changes
- Created
BashCodeExecutionToolResultContentBlockto override theContentproperty type from list to single object - Implemented
ContentBlockConverterto deserialize content blocks based on their type - Added
ExpiresAtproperty toContainerResponsefor tracking container expiration
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Anthropic.SDK/Messaging/MessageResponse.cs | Added new BashCodeExecutionToolResultContentBlock class with overridden Content property |
| Anthropic.SDK/Messaging/Container.cs | Added ExpiresAt property to track container expiration dates |
| Anthropic.SDK/Extensions/ContentBlockConverter.cs | Implemented custom JSON converter for polymorphic ContentBlock deserialization |
| Anthropic.SDK/EndpointBase.cs | Registered ContentBlockConverter in streaming JSON serialization options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| using (var jsonDoc = JsonDocument.ParseValue(ref reader)) | ||
| { | ||
| var root = jsonDoc.RootElement; | ||
| var type = root.GetProperty("type").GetString(); | ||
|
|
||
| var optionsCopy = GetJsonOptionsCopy(options); | ||
|
|
||
| return type switch | ||
| { | ||
| "bash_code_execution_tool_result" => | ||
| JsonSerializer.Deserialize<BashCodeExecutionToolResultContentBlock>( | ||
| root.GetRawText(), | ||
| optionsCopy | ||
| ), | ||
| _ => JsonSerializer.Deserialize<ContentBlock>(root.GetRawText(), optionsCopy), | ||
| }; | ||
| } |
There was a problem hiding this comment.
[nitpick] Using statement with parentheses is an older C# syntax. Consider using the more modern 'using var' declaration pattern which automatically disposes at the end of the enclosing scope, improving code readability.
| using (var jsonDoc = JsonDocument.ParseValue(ref reader)) | |
| { | |
| var root = jsonDoc.RootElement; | |
| var type = root.GetProperty("type").GetString(); | |
| var optionsCopy = GetJsonOptionsCopy(options); | |
| return type switch | |
| { | |
| "bash_code_execution_tool_result" => | |
| JsonSerializer.Deserialize<BashCodeExecutionToolResultContentBlock>( | |
| root.GetRawText(), | |
| optionsCopy | |
| ), | |
| _ => JsonSerializer.Deserialize<ContentBlock>(root.GetRawText(), optionsCopy), | |
| }; | |
| } | |
| using var jsonDoc = JsonDocument.ParseValue(ref reader); | |
| var root = jsonDoc.RootElement; | |
| var type = root.GetProperty("type").GetString(); | |
| var optionsCopy = GetJsonOptionsCopy(options); | |
| return type switch | |
| { | |
| "bash_code_execution_tool_result" => | |
| JsonSerializer.Deserialize<BashCodeExecutionToolResultContentBlock>( | |
| root.GetRawText(), | |
| optionsCopy | |
| ), | |
| _ => JsonSerializer.Deserialize<ContentBlock>(root.GetRawText(), optionsCopy), | |
| }; |
| using (var jsonDoc = JsonDocument.ParseValue(ref reader)) | ||
| { | ||
| var root = jsonDoc.RootElement; | ||
| var type = root.GetProperty("type").GetString(); |
There was a problem hiding this comment.
Missing null check after GetProperty. If the 'type' property is missing from the JSON, this will throw a KeyNotFoundException. Consider using TryGetProperty to handle missing properties gracefully.
| var type = root.GetProperty("type").GetString(); | |
| string type = null; | |
| if (root.TryGetProperty("type", out var typeProperty)) | |
| { | |
| type = typeProperty.GetString(); | |
| } |
|
Thanks for the PR! I kind of went down a rabbit hole on this one and ended up taking a different approach. After adding an integration test, I was hitting a deserialization exception with the approach you took, not on the bash results, but rather on the text editor code results. I also saw an opportunity to knock out two birds with one stone, as I believe my approach will resolve a lingering issue that's been open for a while as well as satisfying your issue: #121 |
When using the code_execution tool with streaming, the chunk with
bash_code_execution_tool_resultcontent block has its content as an object instead of a list of content, which is currently mapped in the defaultContentBlock.In this PR I'm overriding it for the
bash_code_execution_tool_resultand adding a converter to make sure the response is deserialized to the correct type.I'm also adding the
ExpiresAtto the container response.