mcp: render blank text field in TextContent instead of omitempty #91
Conversation
| @@ -25,12 +25,19 @@ type TextContent struct { | |||
| } | |||
|
|
|||
| func (c *TextContent) MarshalJSON() ([]byte, error) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I opened #95 which addresses 2 other types in a slightly different way. I left this PR the same and added the TODO.
martinemde
force-pushed
the
martinemde/dont-omit-blank-text-content
branch
from
d5f724d to
2765d6f
Compare
|
Added one additional test where Text is not set to ensure we don't have any null values. Looks good. |
The text field is required. Some clients fail to parse the response if the text field is omitted and a blank text field is allowed. Return the field even when it is blank by removing omitempty.
martinemde
force-pushed
the
martinemde/dont-omit-blank-text-content
branch
from
3c48177 to
70221a8
Compare
Good idea, thanks. |
The text field is required, but current implementation omits the text field when it is a blank string.
Some clients fail to parse the response if the text field is omitted. A blank text result is a valid MCP response.
Claude desktop error message:
The solution is to return the field even when it is blank by removing
omitempty.