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. WalkthroughAdds exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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. Comment |
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
mcp/utils_test.go (2)
9-94: Add clamp coverage for priorityIf we clamp priority to [0,1] in ParseAnnotations, add a test to lock behavior.
{ name: "invalid audience element type", data: map[string]any{ "audience": []any{"user", 123, "assistant"}, }, expected: &Annotations{ Audience: []Role{"user", "assistant"}, }, }, { name: "audience as []string", data: map[string]any{ "audience": []string{"assistant", "user"}, }, expected: &Annotations{ Audience: []Role{"assistant", "user"}, }, }, + { + name: "priority clamped above 1", + data: map[string]any{ + "priority": 2.5, + }, + expected: &Annotations{ + Priority: 1.0, + }, + }, + { + name: "priority clamped below 0", + data: map[string]any{ + "priority": -0.3, + }, + expected: &Annotations{ + Priority: 0.0, + }, + },
96-359: Solid coverage of annotation propagation across content typesEnd-to-end cases (with/without annotations), error paths, and per-type assertions look good. Consider adding one case where annotations is a non-object (e.g., string) to ensure it’s safely ignored.
mcp/utils.go (1)
539-558: Document and harden ParseAnnotations; clamp priority and normalize audience rolesAdd a doc comment, clamp Priority to [0,1], rename the loop var to avoid shadowing, and normalize/trim audience roles to lower-case using strings.TrimSpace. Note that cast.ToStringSlice only splits on whitespace—if comma-separated input must be supported, split on commas first.
Proposed diff in mcp/utils.go (lines 539–558):
- func ParseAnnotations(data map[string]any) *Annotations { + // ParseAnnotations parses an MCP annotations object from a loosely-typed map. + // - priority: accepts numbers and numeric strings; clamped to [0,1] + // - audience: accepts []string, []any, or space-separated string; normalized to lower-case; filters supported roles + func ParseAnnotations(data map[string]any) *Annotations { if data == nil { return nil } annotations := &Annotations{} if value, ok := data["priority"]; ok { - annotations.Priority = cast.ToFloat64(value) + annotations.Priority = cast.ToFloat64(value) + if annotations.Priority < 0 { + annotations.Priority = 0 + } else if annotations.Priority > 1 { + annotations.Priority = 1 + } } 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(strings.ToLower(strings.TrimSpace(s))) + if role == RoleUser || role == RoleAssistant { + annotations.Audience = append(annotations.Audience, role) + } + } } return annotations }And update imports to:
import ( "strings" "github.com/spf13/cast" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mcp/utils.go(4 hunks)mcp/utils_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
mcp/utils.go (3)
mcp/types.go (1)
Annotations(930-943)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)
mcp/utils_test.go (4)
mcp/types.go (16)
Annotations(930-943)Content(952-954)TextContent(958-965)TextContent(967-967)Annotated(948-950)ImageContent(971-980)ImageContent(982-982)AudioContent(986-995)AudioContent(997-997)ResourceLink(1000-1011)ResourceLink(1013-1013)EmbeddedResource(1019-1025)EmbeddedResource(1027-1027)Resource(683-700)TextResourceContents(741-751)TextResourceContents(753-753)mcp/prompts.go (1)
Role(81-81)mcp/utils.go (2)
ParseAnnotations(539-558)ParseContent(569-633)mcp/consts.go (5)
ContentTypeText(4-4)ContentTypeImage(5-5)ContentTypeAudio(6-6)ContentTypeLink(7-7)ContentTypeResource(8-8)
🔇 Additional comments (7)
mcp/utils.go (6)
572-575: Nice: optional annotations extraction is clean and isolatedReading a sub-map and deferring validation to ParseAnnotations keeps ParseContent simple.
580-583: LGTM: text content now attaches annotationsCorrectly sets Annotated.Annotations and preserves prior behavior when absent.
590-593: LGTM: image content annotation propagationChecks required fields and then attaches annotations.
600-603: LGTM: audio content annotation propagationMatches image handling, validation first, then annotate.
612-615: LGTM: resource link annotation propagationValidates uri/name as before and attaches annotations.
627-630: LGTM: embedded resource annotation propagationParses contents and attaches annotations on the content wrapper.
mcp/utils_test.go (1)
5-6: LGTM: assert/require importsBoth are used; import set is correct.
|
@ezynda3 when wiil merge this PR? |
2 participants
Description
Fixes #<issue_number> (if applicable)
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Tests