-
Notifications
You must be signed in to change notification settings - Fork 853
MCP/Approvals stabilization #7245
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
Copilot
wants to merge
32
commits into
main
Choose a base branch
from
copilot/update-mcp-server-tool-invocation-again
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,104
−918
Open
Changes from all commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
2daa4b4
Initial plan
Copilot b5dac2d
Rebase
jozkee 052dbe3
Add CompatibilitySuppressions.xml
jozkee 59b5ebb
Fix ToolName->Name and CloneFcc to handle McpServerToolCallContent
Copilot 500863e
Remove duplicate assertion for mcpCallContent.Name
Copilot 0ee1ed6
Update handling of experimental McpServerTool types in sourcegen seri…
jozkee cd19f1e
Remove User prefix from Input contents and rename id -> requestId
jozkee d2d6aa2
Use string.Empty on mcp approval requests
jozkee b81cb33
Remove AuthorizationToken and make Headers settable
jozkee 4515843
Prefix RequestIds with ficc_ on FunctionInvokingChatClient
jozkee 7fe9c0d
Rename serverUrl -> serverAddress to align with string overload
jozkee b5bb861
OpenAI: Revert to use ErrorContent and TextContent
jozkee ca5d7d3
Remove Experimental
jozkee 23eed81
Update CompatibilitySuppressions.xml
jozkee 4cc083c
Cleanup JsonPolymorphic for Stabilized types
jozkee f5e3b37
Update test
jozkee 74a1165
Merge branch 'main' of https://github.com/dotnet/extensions into temp
jozkee 9a950b3
Fix merge errors
jozkee 66e28b1
Revert "Use string.Empty on mcp approval requests"
jozkee 8579d3c
Fix more merge errors
jozkee 2d8bd4f
Improve docs
jozkee 7fd2c4a
Address jsonpolymorphic feedback and improve tests
jozkee 2c5f6fb
McpToolCallApprovalResponseItem conversion to MEAI correctly and impl…
jozkee ced9e36
Fix roundtrip of McpServerToolResultContent.Result
jozkee 89a0ba9
Refactor approval request ID generation in FunctionInvokingChatClient
jozkee d0a56eb
Address documentation feedback for MCP content types and Headers prop…
Copilot f9e7e0e
Apply documentation suggestions from @jozkee
Copilot a43576a
Add remarks to McpServerToolCallContent constructor documentation
Copilot 420502e
Fix case-insensitive Bearer token extraction and add test, fix MakeAp…
Copilot ec9ebec
Trim both ends of header value and allow flexible whitespace after Be…
Copilot 6127a72
Address feedback
jozkee 80b4e96
Merge branch 'main' of https://github.com/dotnet/extensions into copi…
jozkee File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
429 changes: 429 additions & 0 deletions
429
src/Libraries/Microsoft.Extensions.AI.Abstractions/CompatibilitySuppressions.xml
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
31 changes: 31 additions & 0 deletions
31
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/InputRequestContent.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Text.Json.Serialization; | ||
| using Microsoft.Shared.Diagnostics; | ||
|
|
||
| namespace Microsoft.Extensions.AI; | ||
|
|
||
| /// <summary> | ||
| /// Represents a request for input from the user or application. | ||
| /// </summary> | ||
jozkee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| [JsonDerivedType(typeof(FunctionApprovalRequestContent), "functionApprovalRequest")] | ||
| public class InputRequestContent : AIContent | ||
| { | ||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="InputRequestContent"/> class. | ||
| /// </summary> | ||
| /// <param name="requestId">The unique identifier that correlates this request with its corresponding response.</param> | ||
| /// <exception cref="ArgumentNullException"><paramref name="requestId"/> is <see langword="null"/>.</exception> | ||
| /// <exception cref="ArgumentException"><paramref name="requestId"/> is empty or composed entirely of whitespace.</exception> | ||
| protected InputRequestContent(string requestId) | ||
| { | ||
| RequestId = Throw.IfNullOrWhitespace(requestId); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the unique identifier that correlates this request with its corresponding <see cref="InputResponseContent"/>. | ||
| /// </summary> | ||
| public string RequestId { get; } | ||
| } | ||
31 changes: 31 additions & 0 deletions
31
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/InputResponseContent.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Text.Json.Serialization; | ||
| using Microsoft.Shared.Diagnostics; | ||
|
|
||
| namespace Microsoft.Extensions.AI; | ||
|
|
||
| /// <summary> | ||
| /// Represents the response to an <see cref="InputRequestContent"/>. | ||
| /// </summary> | ||
jozkee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| [JsonDerivedType(typeof(FunctionApprovalResponseContent), "functionApprovalResponse")] | ||
| public class InputResponseContent : AIContent | ||
| { | ||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="InputResponseContent"/> class. | ||
| /// </summary> | ||
| /// <param name="requestId">The unique identifier that correlates this response with its corresponding request.</param> | ||
| /// <exception cref="ArgumentNullException"><paramref name="requestId"/> is <see langword="null"/>.</exception> | ||
| /// <exception cref="ArgumentException"><paramref name="requestId"/> is empty or composed entirely of whitespace.</exception> | ||
| protected InputResponseContent(string requestId) | ||
| { | ||
| RequestId = Throw.IfNullOrWhitespace(requestId); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the unique identifier that correlates this response with its corresponding <see cref="InputRequestContent"/>. | ||
| /// </summary> | ||
| public string RequestId { get; } | ||
| } | ||
42 changes: 0 additions & 42 deletions
42
...ries/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolApprovalRequestContent.cs
This file was deleted.
Oops, something went wrong.
33 changes: 0 additions & 33 deletions
33
...ies/Microsoft.Extensions.AI.Abstractions/Contents/McpServerToolApprovalResponseContent.cs
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are these right? Shouldn't they be InputRequestedContent / InputResponseContent?
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.
They're needed.
If we replace
functionApprovalRequestwith baseinputsRequest,Serialize<AIContent>(new FunctionApprovalRequestContent(…))wouldn't work.If we had both, it would work but it doesn't make sense to specify inputs as JsonDerived since that's non-instantiable, hence why is specified as
[JsonSerializable].Note: serialization of unknown types throws by default, so for FCC/FRC, we need to specify
mcpTool*Contentin both places.Uh oh!
There was an error while loading. Please reload this page.
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.
@eiriktsarpalis is that expected? The fact that this throws really surprises me:
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.
@stephentoub Isn't your question answered in the doc I linked to?
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.
Answered where? There's no ambiguity here.
Uh oh!
There was an error while loading. Please reload this page.
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.
"To handle unknown derived types, you must opt in to such support using an annotation on the base type". Looks like you are expecting recursive lookup of JsonDerivedTypes and STJ only looks for the attributes of the
T.Filed dotnet/runtime#124198, furthermore, it is by design: dotnet/runtime#81236 (comment) (thanks MihuBot).
Uh oh!
There was an error while loading. Please reload this page.
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.
Exactly.
I don't understand the explanation there, @eiriktsarpalis. If I have type Base, I'm the only one that can add attributes to Base. If I choose to add Middle in an attribute on Base, then either I'm also in control of Middle or I'm already giving away the ability to control what happens on deserialization. And if I'm in control of Middle, then recursively the same things apply to Middle, namely I'm the only one that can add attributes to it. What case are we trying to prevent here? The current solution is very awkward and unexpected, and it requires the same attribute with the same arguments in multiple places.
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.
It's not a behaviour we can guarantee in the general case, in a way that works well with trimmed/AOT apps:
One could argue of course that your example uses an unbroken chain of annotations which forces the entire hierarchy in a single project, but making a special case out of that is not something that was considered when the feature was being designed.
By default, polymorphic serialization is invariant but this can be changed via configuration in
JsonPolymorphicAttribute. There is an open API proposal that I suspect addresses the same problem in an inverted manner, by having derived types inherit the polymorphic configuration of their base. This does violate encapsulation concerns on first inspection, however by forcing the entire transitive hierarchy on the base type it also avoids ambiguities like the one pointed out above.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.
I don't understand your example. A doesn't have an attribute on it for B2. This is not the same case as mine.
I'm surprised we didn't consider that. This would seem to be the majority case. We should fix this.