-
Notifications
You must be signed in to change notification settings - Fork 848
Add Reason property to FunctionApprovalResponseContent for custom rejection messages #7140
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
… rejection messages Co-authored-by: stephentoub <[email protected]>
Co-authored-by: stephentoub <[email protected]>
stephentoub
left a comment
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.
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalResponseContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalResponseContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
…ge format Co-authored-by: stephentoub <[email protected]>
|
@NoofSaeidh, does this address your scenarios / concerns from #7139? |
|
cc: @rogerbarreto, @westey-m, @jozkee |
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.
Pull request overview
This PR adds a nullable Reason property to FunctionApprovalResponseContent to enable custom rejection messages for function call approvals. Previously, rejected function calls used a hard-coded error message. Now, users can provide specific reasons for rejections that will be included in the function result content sent to the downstream client.
Key Changes:
- Added
Reasonproperty (nullable string, get/set) toFunctionApprovalResponseContentwith documentation indicating it's optional and can be used for both approval and rejection - Updated rejection message format in
FunctionInvokingChatClient.GenerateRejectedFunctionResultsfrom "Error: Tool call invocation was rejected by user." to "Tool call invocation rejected." with optional custom reason appended when provided - Updated all existing test expectations to match the new default rejection message format and added comprehensive tests for custom rejection reasons
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalResponseContent.cs |
Adds nullable Reason property to allow custom messages for approval/rejection responses |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs |
Updates GenerateRejectedFunctionResults to use new rejection message format and append custom reason when provided using IsNullOrWhiteSpace check |
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs |
Updates existing test expectations for new default message format and adds two comprehensive tests (RejectedApprovalResponsesWithCustomReasonAsync and MixedApprovalResponsesWithCustomAndDefaultReasonsAsync) covering custom rejection reasons |
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalResponseContent.cs
Show resolved
Hide resolved
...ies/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientApprovalsTests.cs
Show resolved
Hide resolved
|
@copilot, please address the feedback |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionApprovalResponseContent.cs
Show resolved
Hide resolved
Co-authored-by: stephentoub <[email protected]>
Yes, thank you! |
|
@stephentoub actually looks like it is worth to add Reason into the CreateResponse method in FunctionApprovalRequestContent Line 40 in eedc644
|
Why not just set the property on the resulting object? |
Implementation: Add Reason property to FunctionApprovalResponseContent
Changes Completed
Add nullable
Reasonproperty toFunctionApprovalResponseContentclass<remarks>section for cleaner documentationUpdate
FunctionInvokingChatClient.GenerateRejectedFunctionResultsmethodIsNullOrWhiteSpacecheck as suggestedUpdate all tests for new message format
Add serialization tests for Reason property
FunctionApprovalResponseContentTests.Serialization_Roundtripsto test with both null and custom reasonAdd edge case tests for Reason property
RejectedApprovalResponsesWithEmptyOrWhitespaceReasonUsesDefaultMessageAsynctestIsNullOrWhiteSpacelogic treats all as default messageBuild and test validation
Code review feedback addressed
Summary
All PR review feedback has been addressed. The rejection message now clearly indicates it's a rejection while allowing custom context to be appended. The implementation includes proper serialization support and comprehensive edge case testing to document behavior with empty/whitespace/null reasons.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Microsoft Reviewers: Open in CodeFlow