feat: update type definition of _meta field to text/blob resources. #591
Conversation
- Parses optional _meta field on the text/blob resource
- Add TestResourceContentsMetaField to verify _meta field handling
- Ensure backward compatibility with resources without _meta
WalkthroughConvert TextResourceContents.Meta and BlobResourceContents.Meta from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
mcp/types.go (3)
718-728: Normalize to map[string]any and document pass‑through semantics.Use
anyfor consistency across the file, and make it explicit this_metais raw pass‑through (unlike*Metaused elsewhere).type TextResourceContents struct { // Meta is a metadata object that is reserved by MCP for storing additional information. - Meta map[string]interface{} `json:"_meta,omitempty"` + // Raw per‑resource metadata; pass‑through as defined by MCP. Not the same as mcp.Meta. + Meta map[string]any `json:"_meta,omitempty"` // The URI of this resource. URI string `json:"uri"`
732-741: Apply the sameanynormalization to BlobResourceContents.Keeps types consistent across the codebase.
type BlobResourceContents struct { // Meta is a metadata object that is reserved by MCP for storing additional information. - Meta map[string]interface{} `json:"_meta,omitempty"` + Meta map[string]any `json:"_meta,omitempty"` // The URI of this resource. URI string `json:"uri"`
718-735: Intentional divergence from*Metaelsewhere – add a brief package comment or doc note.Since other structs use
*Metawhile resource contents use a raw map, add a short note in package docs or near the type explaining why (spec requires arbitrary keys; noprogressTokensemantics).Would you like me to draft a short doc blurb for package comments?
mcp/utils.go (3)
708-714: Use ExtractMap andanyfor brevity and consistency.Leverages the existing helper and aligns with the suggested
map[string]anychange.- var meta map[string]interface{} - if metaValue, ok := contentMap["_meta"]; ok { - if metaMap, ok := metaValue.(map[string]interface{}); ok { - meta = metaMap - } - } + meta := ExtractMap(contentMap, "_meta")
715-731: Option: validate_metatype when present.If
_metaexists but isn’t an object, consider returning an error for clearer diagnostics. Backward‑compat risk is low.if text := ExtractString(contentMap, "text"); text != "" { return TextResourceContents{ URI: uri, MIMEType: mimeType, Text: text, - Meta: meta, + Meta: meta, }, nil } if blob := ExtractString(contentMap, "blob"); blob != "" { return BlobResourceContents{ URI: uri, MIMEType: mimeType, Blob: blob, - Meta: meta, + Meta: meta, }, nil } + // If _meta was present but invalid (not an object), surface a helpful error. + if _, present := contentMap["_meta"]; present && meta == nil { + return nil, fmt.Errorf("_meta must be an object") + } + return nil, fmt.Errorf("unsupported resource type")If you prefer to keep lenient parsing, at least log/trace this case during tests.
715-724: Consider presence‑check vs non‑empty for text/blob (spec nuance).Current logic treats empty string as “absent.” If zero‑length text/blob is valid per MCP, switch to presence checks (
_, ok := contentMap["text"]).I can send a small diff once we confirm the spec intent.
mcp/types_test.go (3)
142-209: Good coverage of_metaon text/blob; add empty‑object case.Add a case where
_metais{}to ensure it round‑trips as an empty object (not omitted).tests := []struct { name string inputJSON string expectedType string - expectedMeta map[string]interface{} + expectedMeta map[string]any }{ + { + name: "TextResourceContents with empty _meta", + inputJSON: `{ + "uri":"file://empty-meta.txt", + "mimeType":"text/plain", + "text":"x", + "_meta": {} + }`, + expectedType: "text", + expectedMeta: map[string]any{}, + },
211-265: Strengthen assertions: distinguish “missing” vs “null”, and useany.Avoid false positives by checking the map lookup boolean; also normalize types to
any.- var contentMap map[string]interface{} + var contentMap map[string]any err := json.Unmarshal([]byte(tc.inputJSON), &contentMap) require.NoError(t, err) @@ - marshaledJSON, err := json.Marshal(resourceContent) + marshaledJSON, err := json.Marshal(resourceContent) require.NoError(t, err) - var marshaledMap map[string]interface{} + var marshaledMap map[string]any err = json.Unmarshal(marshaledJSON, &marshaledMap) require.NoError(t, err) - // Verify _meta field is preserved in marshaled output - if tc.expectedMeta != nil { - assert.Equal(t, tc.expectedMeta, marshaledMap["_meta"]) - } else { - assert.Nil(t, marshaledMap["_meta"]) - } + // Verify _meta field is preserved in marshaled output + v, ok := marshaledMap["_meta"] + if tc.expectedMeta != nil { + require.True(t, ok, "_meta should be present") + assert.Equal(t, tc.expectedMeta, v) + } else { + assert.False(t, ok, "_meta should be omitted when nil") + }
142-265: Optional: add a negative test for non‑object_meta.E.g.,
_meta: "oops"should error (if you adopt the stricter validation), or be ignored (if you keep lenient).Want me to draft this test both ways and guard it with a build tag until a decision is made?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mcp/types.go(2 hunks)mcp/types_test.go(1 hunks)mcp/utils.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
mcp/types_test.go (2)
mcp/utils.go (1)
ParseResourceContents(700-734)mcp/types.go (5)
TextResourceContents(718-728)TextResourceContents(730-730)Meta(121-133)BlobResourceContents(732-741)BlobResourceContents(743-743)
mcp/utils.go (1)
mcp/types.go (3)
TextResourceContents(718-728)TextResourceContents(730-730)Meta(121-133)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
mcp/utils.go (2)
708-714: _meta validation is correct; nit: avoid double lookup and make flow explicit.Single-pass check is a touch cleaner and avoids two map lookups.
- meta := ExtractMap(contentMap, "_meta") - - // Validate _meta field if present - if _, present := contentMap["_meta"]; present && meta == nil { - return nil, fmt.Errorf("_meta must be an object") - } + var meta map[string]any + if v, present := contentMap["_meta"]; present { + m, ok := v.(map[string]any) + if !ok { + return nil, fmt.Errorf("_meta must be an object") + } + meta = m + }
715-731: Union guard: reject resources that specify both text and blob.Spec-wise these are disjoint variants; being permissive can hide producer bugs.
- if text := ExtractString(contentMap, "text"); text != "" { - return TextResourceContents{ - Meta: meta, - URI: uri, - MIMEType: mimeType, - Text: text, - }, nil - } - - if blob := ExtractString(contentMap, "blob"); blob != "" { - return BlobResourceContents{ - Meta: meta, - URI: uri, - MIMEType: mimeType, - Blob: blob, - }, nil - } + text := ExtractString(contentMap, "text") + blob := ExtractString(contentMap, "blob") + if text != "" && blob != "" { + return nil, fmt.Errorf("resource must not specify both text and blob") + } + if text != "" { + return TextResourceContents{ + Meta: meta, + URI: uri, + MIMEType: mimeType, + Text: text, + }, nil + } + if blob != "" { + return BlobResourceContents{ + Meta: meta, + URI: uri, + MIMEType: mimeType, + Blob: blob, + }, nil + }mcp/types_test.go (2)
260-281: Optional: assert type of marshaled _meta and add an ambiguity test.
- Add assert.IsType(t, map[string]any{}, v) when _meta is present (non-empty).
- Consider a test where both "text" and "blob" are set to document expected behavior (ideally reject).
285-379: Negative cases look solid; minor gap for blob variants.You could mirror the array/boolean/null invalid _meta cases for blob, for symmetry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mcp/types.go(2 hunks)mcp/types_test.go(1 hunks)mcp/utils.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mcp/types.go
🧰 Additional context used
🧬 Code graph analysis (2)
mcp/types_test.go (2)
mcp/utils.go (1)
ParseResourceContents(700-734)mcp/types.go (5)
TextResourceContents(718-729)TextResourceContents(731-731)Meta(121-133)BlobResourceContents(733-743)BlobResourceContents(745-745)
mcp/utils.go (1)
mcp/types.go (3)
TextResourceContents(718-729)TextResourceContents(731-731)Meta(121-133)
🔇 Additional comments (3)
mcp/utils.go (2)
716-721: LGTM: _meta is plumbed through for text resources.
726-730: LGTM: _meta is plumbed through for blob resources.mcp/types_test.go (1)
142-283: Good coverage for presence/absence and round‑trip of _meta.Covers empty map omission and float64 number decoding.
|
@rabbah if you could fix the golangci-lint issues I'll go ahead and merge this. |
ezynda3
added
the
status: needs submitter response
…ark3labs#591) * feat: update type definition of _meta field to text/blob resources. - Parses optional _meta field on the text/blob resource - Add TestResourceContentsMetaField to verify _meta field handling - Ensure backward compatibility with resources without _meta * chore: apply coderabbit nits.
Description
Adds support for parsing the optional
_metafield in resource contents. This field is used in server-client interactions and a prime example of that is MCP UI.Fixes #585.
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit