support audio content type in tools/call and prompts/get #249
Conversation
WalkthroughSupport for audio content was added throughout the codebase. A new Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/inprocess_test.go (1)
208-208: Update test error message to match new expectation.The error message still refers to expecting 1 content item, but the test now checks for 2 content items (text and audio).
- t.Errorf("Expected 1 content item, got %d", len(result.Content)) + t.Errorf("Expected 2 content items, got %d", len(result.Content))mcp/utils.go (1)
80-83: Fix incorrect comment forAsAudioContentfunction.The function comment incorrectly states "AsImageContent" instead of "AsAudioContent". This should be updated to match the actual function name and purpose.
-// AsImageContent attempts to cast the given interface to AudioContent +// AsAudioContent attempts to cast the given interface to AudioContent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/inprocess_test.go(4 hunks)mcp/prompts.go(1 hunks)mcp/tools.go(1 hunks)mcp/types.go(2 hunks)mcp/utils.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
mcp/prompts.go (1)
mcp/types.go (1)
Content(684-686)
mcp/tools.go (1)
mcp/types.go (1)
Content(684-686)
client/inprocess_test.go (2)
mcp/types.go (4)
AudioContent(714-721)AudioContent(723-723)Content(684-686)Params(118-118)mcp/prompts.go (2)
Role(68-68)RoleUser(71-71)
mcp/utils.go (2)
mcp/types.go (5)
AudioContent(714-721)AudioContent(723-723)Content(684-686)TextContent(690-695)TextContent(697-697)mcp/tools.go (1)
CallToolResult(34-41)
🔇 Additional comments (10)
mcp/prompts.go (1)
81-81: Correctly updated comment to include AudioContent as a valid content type.The comment has been appropriately updated to include
AudioContentas a possible value for theContentfield, which aligns with the new audio content support being added to the codebase.mcp/tools.go (1)
36-36: Correctly updated comment to include AudioContent as a valid content type.The comment has been appropriately updated to include
AudioContentas a possible value for theContentfield inCallToolResult, which aligns with the new audio content support being added to the codebase.client/inprocess_test.go (4)
39-43: Good implementation of the AudioContent test case.The addition of
AudioContentto the test tool response correctly tests the tool's ability to return audio content alongside text content. The implementation includes all required fields (Type, Data, and MIMEType).
85-92: Well-structured test for PromptMessage with AudioContent.This implementation properly tests the ability of prompt messages to include audio content. The Role is set correctly, and the AudioContent is well-formed with all required fields.
375-377: Good addition of arguments to the test prompt request.Adding arguments to the test prompt request ensures that the prompt handler's usage of arguments is properly tested, which provides more thorough test coverage.
384-385: Correctly updated test assertion for multiple messages.The test assertion has been properly updated to expect 2 messages instead of 1, reflecting the new prompt handler implementation that returns both text and audio content messages.
mcp/types.go (2)
659-659: Correctly updated comment to include AudioContent in SamplingMessage.This comment update properly documents that the
Contentfield inSamplingMessagecan now includeAudioContentalongside the existingTextContentandImageContenttypes.
712-724: Well-implemented AudioContent struct following established patterns.The new
AudioContentstruct is well-designed and follows the same pattern as existing content types:
- It embeds the
Annotatedtype like other content types- It has a
Typefield that must be "audio"- It includes fields for the base64-encoded audio data and its MIME type
- It properly implements the
isContent()method to satisfy theContentinterfaceThis implementation maintains consistency with the codebase's design patterns and will integrate seamlessly with existing code.
mcp/utils.go (2)
210-217: LGTM! Good implementation ofNewAudioContent.The implementation correctly follows the pattern established by other content type constructor functions, properly setting the type, data, and MIME type fields.
449-456: LGTM! Proper handling of audio content inParseContentfunction.The implementation correctly follows the pattern established for other content types, properly extracting and validating the data and MIME type fields for audio content.
| // NewToolResultAudio creates a new CallToolResult with both text and audio content | ||
| func NewToolResultAudio(text, imageData, mimeType string) *CallToolResult { | ||
| return &CallToolResult{ | ||
| Content: []Content{ | ||
| TextContent{ | ||
| Type: "text", | ||
| Text: text, | ||
| }, | ||
| AudioContent{ | ||
| Type: "audio", | ||
| Data: imageData, | ||
| MIMEType: mimeType, | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix parameter name in NewToolResultAudio function.
The parameter is named imageData but is being used for audio data, which is confusing and misleading. This should be renamed to match its actual purpose.
-func NewToolResultAudio(text, imageData, mimeType string) *CallToolResult {
+func NewToolResultAudio(text, audioData, mimeType string) *CallToolResult {
return &CallToolResult{
Content: []Content{
TextContent{
Type: "text",
Text: text,
},
AudioContent{
Type: "audio",
- Data: imageData,
+ Data: audioData,
MIMEType: mimeType,
},
},
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewToolResultAudio creates a new CallToolResult with both text and audio content | |
| func NewToolResultAudio(text, imageData, mimeType string) *CallToolResult { | |
| return &CallToolResult{ | |
| Content: []Content{ | |
| TextContent{ | |
| Type: "text", | |
| Text: text, | |
| }, | |
| AudioContent{ | |
| Type: "audio", | |
| Data: imageData, | |
| MIMEType: mimeType, | |
| }, | |
| }, | |
| } | |
| } | |
| // NewToolResultAudio creates a new CallToolResult with both text and audio content | |
| func NewToolResultAudio(text, audioData, mimeType string) *CallToolResult { | |
| return &CallToolResult{ | |
| Content: []Content{ | |
| TextContent{ | |
| Type: "text", | |
| Text: text, | |
| }, | |
| AudioContent{ | |
| Type: "audio", | |
| Data: audioData, | |
| MIMEType: mimeType, | |
| }, | |
| }, | |
| } | |
| } |
dugenkui03
changed the title
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation