Fix structured output backwards compatibility for string results #748
Conversation
PederHP
changed the title
| // If there is structuredOutput we must stringify it, as the MCP specification requires Content to be for backwards compatibility | ||
| // but otherwise not differ | ||
| Content = [new TextContentBlock { Text = structuredContent == null ? text : | ||
| JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo(typeof(object)))}], |
There was a problem hiding this comment.
Why not just resolve the type of structuredContent itself and skip one degree of indirection?
| JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo(typeof(object)))}], | |
| JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo<JsonNode>()))}], |
There was a problem hiding this comment.
Why not just resolve the type of structuredContent itself and skip one degree of indirection?
Suggested change
JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo(typeof(object)))}], JsonSerializer.Serialize(structuredContent, AIFunction.JsonSerializerOptions.GetTypeInfo<JsonNode>()))}],
Good point, but this goes for the default switch case too then?
There was a problem hiding this comment.
Yeah, I suppose we can leave it for consistency.
src/ModelContextProtocol.Core/Server/AIFunctionMcpServerTool.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eirik Tsarpalis <[email protected]>
|
Should an error be thrown on the AIContent related return types? If we go by the intent expressed by dsp and others, and It would be a breaking change, so probably best to wait with that until the spec is actually updated. Just wanted to mention that as pointed out in the issue, this is only a fix for the string case. I think that's the one most likely to cause trouble on actual production servers, so I still think it's worth fixing this one in isolation. Setting the attribute property to true for an AIContent or CallToolResult returning tool is a really exotic edge-case (to be fixed with the spec change imo - at the same time as any introduced schema validation requirements). |
Fixes #747
We might want to warn or give errors if
StructuredContentis present when there are incompatible content blocks - ie more than one text content block. I don't think it can actually happen, but if it can it will be a spec violation. Clients should not get different output in unstructuredContentifStructuredContentis provided.