-
Notifications
You must be signed in to change notification settings - Fork 582
Synthesize return description into tool description when UseStructuredContent is false #1049
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
base: main
Are you sure you want to change the base?
Conversation
…ent is false Co-authored-by: stephentoub <[email protected]>
| return $"Returns: {returnDescription}"; | ||
| } | ||
|
|
||
| return $"{description} Returns: {returnDescription}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, let's add a \n newline rather than a space as the separator before Returns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in bb7a8ec - now using \n newline as separator.
| if (returnDescription is null) | ||
| { | ||
| return description; | ||
| } | ||
|
|
||
| // Synthesize a combined description | ||
| if (string.IsNullOrEmpty(description)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these checks should probably be via string.IsNullOrWhitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in bb7a8ec - updated both checks to use string.IsNullOrWhiteSpace.
| private static string? GetReturnDescription(JsonElement? returnJsonSchema) | ||
| { | ||
| if (returnJsonSchema is not JsonElement schema || | ||
| schema.ValueKind != JsonValueKind.Object || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be is not for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in bb7a8ec - changed to is not.
| if (returnJsonSchema is not JsonElement schema || | ||
| schema.ValueKind != JsonValueKind.Object || | ||
| !schema.TryGetProperty("description", out JsonElement descriptionElement) || | ||
| descriptionElement.ValueKind != JsonValueKind.String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in bb7a8ec - changed to is not.
…nd 'is not' pattern Co-authored-by: stephentoub <[email protected]>
It's ensuring that a [return: Description] isn't dropped. If you don't provide a [return: Description], this is a nop. If you do provide a [return: Description], and you've opted-into structured output, this is a nop. But if you do provide a [return: Description] and you don't opt-into structured output, then the [return: Description] is being completely ignored today; this change makes it so that it's instead included in the description. Why would that be undesirable? If you don't want that, why are you providing a return description? |
Yes, my bad. Deleted my comment shortly after writing it. |
UseStructuredContentis false (default),[return: Description]attributes are ignoredAIFunctionMcpServerTool.Create()insrc/ModelContextProtocol.Core/Server/AIFunctionMcpServerTool.csReturnJsonSchemacontains the description in a "description" propertyUseStructuredContentis NOT true andfunction.ReturnJsonSchemahas a description, synthesize a combined tool description\nseparator,IsNullOrWhiteSpace, andis notpattern)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.