feat: add support for call tool result annotations #547
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughParses annotations from maps and attaches them to all MCP content variants via a new ParseAnnotations function in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for call tool result annotations in the MCP (Model Context Protocol) implementation by implementing annotation parsing and integration into content types.
- Adds a new
ParseAnnotaionsfunction to parse priority and audience annotations from map data - Modifies the
ParseContentfunction to extract and assign annotations to all content types - Adds comprehensive test coverage for both annotation parsing and content parsing with annotations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| mcp/utils.go | Implements annotation parsing logic and integrates it into content creation |
| mcp/utils_test.go | Adds comprehensive test coverage for annotation parsing and content parsing functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
mcp/utils_test.go (1)
296-349: Consider an end-to-end test for ParseCallToolResult with annotationsThe PR objective focuses on call tool result annotations. Add a focused test to ensure ParseCallToolResult propagates annotations via ParseContent.
Example test to add:
func TestParseCallToolResult_WithAnnotations(t *testing.T) { raw := []byte(`{ "content": [ {"type": "text", "text": "ok", "annotations": {"priority": 1, "audience": ["user"]}} ], "isError": false }`) var rm json.RawMessage = raw res, err := ParseCallToolResult(&rm) assert.NoError(t, err) assert.NotNil(t, res) if assert.Len(t, res.Content, 1) { tc, ok := res.Content[0].(TextContent) assert.True(t, ok) if ok { if assert.NotNil(t, tc.Annotations) { assert.Equal(t, 1.0, tc.Annotations.Priority) assert.Equal(t, []Role{"user"}, tc.Annotations.Audience) } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
mcp/utils.go(4 hunks)mcp/utils_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Applied to files:
mcp/utils_test.go
🧬 Code Graph Analysis (2)
mcp/utils.go (3)
mcp/types.go (1)
Annotations(863-876)mcp/prompts.go (1)
Role(81-81)mcp/consts.go (3)
ContentTypeText(4-4)ContentTypeImage(5-5)ContentTypeAudio(6-6)
mcp/utils_test.go (4)
mcp/types.go (4)
Annotations(863-876)Content(885-887)Annotated(881-883)Resource(660-677)mcp/prompts.go (1)
Role(81-81)mcp/utils.go (2)
ParseAnnotaions(495-517)ParseContent(528-592)mcp/consts.go (5)
ContentTypeText(4-4)ContentTypeImage(5-5)ContentTypeAudio(6-6)ContentTypeLink(7-7)ContentTypeResource(8-8)
🔇 Additional comments (2)
mcp/utils.go (2)
531-535: Confirm semantics: should empty annotations be preserved or omitted?When an empty object is provided (annotations: {}), ParseAnnotations returns a non-nil &Annotations{} which leads to c.Annotations being non-nil. With json tags using omitempty, this will serialize as "annotations": {} rather than omitting the field entirely. If the desired behavior is to omit empty annotations on re-serialization, we should return nil when both priority and audience are zero/empty.
Currently tests assert an empty struct pointer; confirm desired semantics before we change it.
539-542: LGTM: Annotations are consistently propagated across all content variantsEach content constructor now attaches the parsed annotations. This ensures uniform behavior for text, image, audio, link, and embedded resource content.
Also applies to: 549-552, 559-562, 571-574, 586-589
Crescent617
force-pushed
the
feat-toolresp-annotation
branch
from
f6eb9bc to
785472c
Compare
Crescent617
force-pushed
the
feat-toolresp-annotation
branch
from
785472c to
2c37811
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mcp/utils.go (1)
536-539: Good integration point for optional annotations in ParseContentParsing the optional annotations map upfront and routing through ParseAnnotations is clean. Also, the prior “ParseAnnotaions” typo is resolved here—thanks for fixing.
Run these to verify there are no lingering references to the old name and that all content structs define Annotations:
#!/bin/bash # No remaining old symbol rg -n --type=go 'ParseAnnotaions' # Ensure Annotations field exists on all content structs rg -nPU '(?s)type\s+TextContent\s+struct\s*\{.*?Annotations\s*\*Annotations.*?\}' rg -nPU '(?s)type\s+ImageContent\s+struct\s*\{.*?Annotations\s*\*Annotations.*?\}' rg -nPU '(?s)type\s+AudioContent\s+struct\s*\{.*?Annotations\s*\*Annotations.*?\}' rg -nPU '(?s)type\s+ResourceLink\s+struct\s*\{.*?Annotations\s*\*Annotations.*?\}' rg -nPU '(?s)type\s+EmbeddedResource\s+struct\s*\{.*?Annotations\s*\*Annotations.*?\}'
🧹 Nitpick comments (1)
mcp/utils.go (1)
495-522: Harden ParseAnnotations for numeric types and add doc commentPriority is parsed only when it's float64. For robustness with programmatic inputs and json.Number, broaden accepted numeric types and document the function. This avoids silent drops when callers pass ints/uints.
Apply this diff:
-func ParseAnnotations(data map[string]any) *Annotations { +// ParseAnnotations builds an Annotations object from a loosely-typed map. +// Accepts: +// - priority: float64 and common integer/uint types, or json.Number +// - audience: []string or []any of strings +func ParseAnnotations(data map[string]any) *Annotations { if data == nil { return nil } annotations := &Annotations{} - if value, ok := data["priority"]; ok { - if priority, ok := value.(float64); ok { - annotations.Priority = priority - } - } + if value, ok := data["priority"]; ok { + switch p := value.(type) { + case float64: + annotations.Priority = p + case float32: + annotations.Priority = float64(p) + case int: + annotations.Priority = float64(p) + case int64: + annotations.Priority = float64(p) + case int32: + annotations.Priority = float64(p) + case uint: + annotations.Priority = float64(p) + case uint64: + annotations.Priority = float64(p) + case uint32: + annotations.Priority = float64(p) + case json.Number: + if f, err := p.Float64(); err == nil { + annotations.Priority = f + } + } + }As a follow-up, confirm whether the spec requires priority to be within [0,1]; if so, add a simple bounds check.
Does MCP’s annotations.priority enforce a [0,1] bound as implied by the struct comment in mcp/types.go? If yes, we should validate/clamp here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
mcp/utils.go(4 hunks)mcp/utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp/utils_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
mcp/utils.go (3)
mcp/types.go (1)
Annotations(863-876)mcp/prompts.go (1)
Role(81-81)mcp/consts.go (3)
ContentTypeText(4-4)ContentTypeImage(5-5)ContentTypeAudio(6-6)
🔇 Additional comments (1)
mcp/utils.go (1)
544-546: Attaching annotations to all content variants — LGTMConsistent assignment across Text, Image, Audio, Link, and Resource keeps semantics uniform. Minimal and clear.
Also applies to: 554-556, 564-566, 576-578, 591-593
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mcp/utils.go (1)
495-515: Avoid variable shadowing; optionally return nil when annotations are effectively empty; add doc comment
- The inner
a := Role(a)shadows the loop variable and hurts readability.- Consider returning
nilwhen both priority is not provided and audience is empty. This keeps the absence of annotations distinct from an explicitly empty object, and will play nicer withomitemptyif re-serialized.Apply this diff:
-func ParseAnnotations(data map[string]any) *Annotations { +// ParseAnnotations parses an annotations object from a loosely-typed map. +func ParseAnnotations(data map[string]any) *Annotations { if data == nil { return nil } annotations := &Annotations{} + var prioritySet bool if value, ok := data["priority"]; ok { - annotations.Priority = cast.ToFloat64(value) + annotations.Priority = cast.ToFloat64(value) + prioritySet = true } if value, ok := data["audience"]; ok { - for _, a := range cast.ToStringSlice(value) { - a := Role(a) - if a == RoleUser || a == RoleAssistant { - annotations.Audience = append(annotations.Audience, a) - } - } + for _, s := range cast.ToStringSlice(value) { + role := Role(s) + if role == RoleUser || role == RoleAssistant { + annotations.Audience = append(annotations.Audience, role) + } + } } - return annotations + if !prioritySet && len(annotations.Audience) == 0 { + return nil + } + return annotations }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
mcp/utils.go(4 hunks)mcp/utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp/utils_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
mcp/utils.go (3)
mcp/types.go (1)
Annotations(863-876)mcp/prompts.go (3)
Role(81-81)RoleUser(84-84)RoleAssistant(85-85)mcp/consts.go (3)
ContentTypeText(4-4)ContentTypeImage(5-5)ContentTypeAudio(6-6)
🔇 Additional comments (6)
mcp/utils.go (6)
528-532: LGTM: Safe extraction and parsing of annotationsUsing ExtractMap to gate parsing and a single ParseAnnotations entry point keeps ParseContent clean and resilient to type variance.
536-539: LGTM: TextContent gets annotations attachedThe construction + assignment pattern is clear and avoids altering the constructor signature.
546-549: LGTM: ImageContent annotations propagationValidation precedes construction and annotation assignment. Looks good.
556-559: LGTM: AudioContent annotations propagationMirrors the image path correctly, including validation.
568-571: LGTM: ResourceLink annotations propagationConsistent with other content types; no additional validation needed here beyond existing uri/name checks.
583-586: LGTM: EmbeddedResource annotations propagationAnnotations are attached at the content wrapper level, which aligns with how the other variants are handled.
Description
Fixes #<issue_number> (if applicable)
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit