Skip to content

mcp: render blank text field in TextContent instead of omitempty #91

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 4, 2025
Merged

mcp: render blank text field in TextContent instead of omitempty #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions mcp/content.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// Use of this source code is governed by an MIT-style
// license that can be found in the LICENSE file.

// TODO(findleyr): update JSON marshalling of all content types to preserve required fields.
// (See [TextContent.MarshalJSON], which handles this for text content).

package mcp

import (
Expand All @@ -25,12 +28,19 @@ type TextContent struct {
}

func (c *TextContent) MarshalJSON() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this oversight.
The same fix should apply to other content types, but you do not need to do that in this CL--we appreciate the fix as it is. However, could you please add

// TODO(findleyr): update JSON marshalling of all content types to preserve required fields.
// (See [TextContent.MarshalJSON], which handles this for text content).

near the top of content.go to address this bug for other content types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered whether other types needed it, and I wasn't really sure. Maybe semantically not omitting the empty response is better.

My logic was that an empty string is technically still a string, but is empty data still an image?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the source of truth here is the machine readable schema, which states that "data" is required:
https://github.com/modelcontextprotocol/modelcontextprotocol/blob/6601f565d1e40f840dbabb7679c5ddad45324efb/schema/2025-06-18/schema.json#L794

Of course, we need to be careful that we don't print JSON null.
A TODO is sufficient for now, and I'll fix when I get back (today and tomorrow are Google holidays).

Thanks again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #95 which addresses 2 other types in a slightly different way. I left this PR the same and added the TODO.

return json.Marshal(&wireContent{
// Custom wire format to ensure the required "text" field is always included, even when empty.
wire := struct {
Type string `json:"type"`
Text string `json:"text"`
Meta Meta `json:"_meta,omitempty"`
Annotations *Annotations `json:"annotations,omitempty"`
}{
Type: "text",
Text: c.Text,
Meta: c.Meta,
Annotations: c.Annotations,
})
}
return json.Marshal(wire)
}

func (c *TextContent) fromWire(wire *wireContent) {
Expand Down
8 changes: 8 additions & 0 deletions mcp/content_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ func TestContent(t *testing.T) {
&mcp.TextContent{Text: "hello"},
`{"type":"text","text":"hello"}`,
},
{
&mcp.TextContent{Text: ""},
`{"type":"text","text":""}`,
},
{
&mcp.TextContent{},
`{"type":"text","text":""}`,
},
{
&mcp.TextContent{
Text: "hello",
Expand Down
Loading