Skip to content

Serialize the function result, not the whole FunctionResultContent, in ToContentBlock - #1945

Open
dfedoryshchev wants to merge 1 commit into
modelcontextprotocol:mainfrom
dfedoryshchev:fix/function-result-content-text
Open

dfedoryshchev wants to merge 1 commit into
modelcontextprotocol:mainfrom
dfedoryshchev:fix/function-result-content-text

Conversation

@dfedoryshchev

Copy link
Copy Markdown
Contributor

ToContentBlock on a FunctionResultContent whose Result is not an AIContent builds the text block from the whole FunctionResultContent instead of from Result, so the text is a JSON object with $type, result and callId in it rather than the tool's output.

Through SampleAsync(IEnumerable<ChatMessage>, ...) / AsSamplingChatClient() with UseFunctionInvocation(), a local tool's result reaches the client as {"$type":"functionResult","result":"Weather in Paris: ...","callId":"call_weather_123"}, and that is what the client's model reads. That path is [Obsolete] with the rest of sampling, but ToContentBlock itself is public and not obsolete, so any caller converting a function result hits the same thing.

The fix serializes resultContent.Result, the same way AIFunctionMcpServerTool turns a non-content return value into text. StructuredContent was already taken from Result, so the two now agree.

EndToEnd_SamplingWithTools_... already goes through this path, but it only asserts that the text contains "Weather in Paris: sunny", which the wrapped JSON also does. I added one DoesNotContain("callId") line there, plus unit tests for a string result, a JsonElement result and an AIContent result (the last one is unchanged behaviour).

One thing you may want differently: a string result that comes through as a JsonElement still ends up quoted ("Weather in Paris: ..."), the same as a tool returning a JsonElement string today. I kept it consistent with that path rather than unwrapping strings here, but happy to change it if you prefer.

Tests pass on net10.0, net9.0, net8.0 and net472.

This branch has not been deployed

No deployments
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.

1 participant