Skip to content

mcp: validate required fields in ImageContent and AudioContent #95

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

martinemde
Copy link
Contributor

@martinemde martinemde commented Jul 4, 2025

This is a follow-up related to #91 (though not requiring it).

ImageContent and AudioContent now return errors when required fields are missing during JSON marshaling, matching TypeScript schema requirements. This follows the pattern in ResourceContents in the same file.

  • ImageContent requires data and mimeType fields.
  • AudioContent requires data and mimeType fields.

Alternatives:

Another way to go about this would be to not omit required attrs when the values are blank. At least we would produce blank values which is preferable to omitted keys (which cause the typescript sdk client to fail rather than report a tool call malfunction to the user).

ImageContent and AudioContent now return errors when required fields
are missing during JSON marshaling, matching TypeScript schema requirements.
ImageContent requires data and mimeType fields. AudioContent requires
data and mimeType fields.

Added comprehensive test coverage for validation error cases.
findleyr pushed a commit that referenced this pull request Jul 4, 2025
I noticed this while working on #95. Seems Meta was accidentally skipped
in the custom wire format struct.
@findleyr
Copy link
Contributor

findleyr commented Jul 7, 2025

@martinemde I think your suggestion to remove the 'omitempty' attrs is preferable: the missing 'mimeType' or 'data' field is really an error in business logic, not marshalling.

For data, we should be careful to replace nil with an empty byte slice, as JSON null is not valid.

Thanks for picking this up!

}
if c.MIMEType == "" {
return nil, errors.New("ImageContent missing mimeType")
}
return json.Marshal(&wireContent{
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted, I think we should use an inline type rather than 'wireContent' here, so that we can make Data and MimeType not "omitempty". We should be sure to replace nil data with the empty slice though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants