Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 10, 2025

Implementation Summary

Successfully implemented the InvocationRequired property for FunctionCallContent with all code review feedback addressed:

  • Add InvocationRequired property to FunctionCallContent class

    • Property defaults to true (requires invocation)
    • Added to JSON file as "Stable"
    • Comprehensive XML documentation
    • Always serializes to JSON to enable proper roundtrip
  • Update FunctionInvokingChatClient to set InvocationRequired = false when handling function

    • Set at beginning of ProcessFunctionCallAsync before any processing (only location)
    • Set for rejected function calls in approval flow
    • Handles ANY response including NotFound and rejected cases
  • Update FunctionInvokingChatClient to filter out already-processed function calls

    • Modified CopyFunctionCalls to skip FunctionCallContent where InvocationRequired == false
    • Modified approval logic to skip FunctionCallContent where InvocationRequired == false
    • Prevents duplicate processing of function calls
  • Comprehensive test coverage (1613 tests passing)

    • 7 tests in FunctionCallContentTests.cs (refactored as theories)
    • 5 tests in FunctionInvokingChatClientTests.cs for integration behavior
    • 1 test in FunctionInvokingChatClientApprovalsTests.cs for approval flow
    • Tests use ternary operator with ToChatResponseAsync() for cleaner code
    • Tests validate InvocationRequired=false passes through unprocessed
    • Tests validate approval flow respects InvocationRequired flag
    • Tests validate multiple FunctionInvokingChatClients in pipeline only process each function once
    • Tests validate JSON roundtrip for both true and false values
  • Test refactoring per code review

    • Removed redundant tests
    • Consolidated tests into theories with streaming parameter
    • Replaced state reset with content cloning approach
    • Added InvocationRequired assertion to Constructor_PropsDefault
    • Refactored to use ternary with ToChatResponseAsync() to reduce duplication
  • All code review feedback addressed

Original prompt

Add a get/set InvocationRequired property to FunctionCallContent. It should default to true and then be set to false by FunctionInvokingChatClient when the associated AIFunction has been processed and a FunctionResultContent created. It should be ignored by JSON serialization when it has its default value of false. Please ensure great test coverage in existing tests, validating the property is set appropriately, and add new tests to ensure that FunctionCallContent will InvocationRequired=false are ignored by FunctionInvokingChatClient; it shouldn't see them as content to be processed.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Microsoft Reviewers: Open in CodeFlow

Copilot AI changed the title [WIP] Add get/set InvocationRequired property to FunctionCallContent Add InvocationRequired property to FunctionCallContent Dec 10, 2025
Copilot AI requested a review from stephentoub December 10, 2025 17:00
@stephentoub
Copy link
Member

@copilot, fix the CI failures

Copy link
Contributor Author

Copilot AI commented Dec 10, 2025

@copilot, fix the CI failures

Fixed in da8a275. The tests were reusing FunctionCallContent objects across multiple test iterations. When we set InvocationRequired=false during processing, subsequent iterations saw them as already processed. Added a ResetPlanFunctionCallStates helper method that resets InvocationRequired=true at the beginning of test helper methods. All 385 AI tests and 1115 abstraction tests now pass.

@stephentoub
Copy link
Member

stephentoub commented Dec 10, 2025

@javiercn, @jozkee, @westey-m, thoughts on this approach?

The goal here is to:

  • Enable an IChatClient to produce FunctionCallContent / FunctionResultContent that should not be re-invoked by callers.
  • Allow callers to distinguish whether an FCC is informative or a request. Today when a consumer gets back an FCC they don't know if it's been handled, and need to look for a corresponding FRC to determine whether it should be handled or not.
  • Allow multiple FunctionInvocationChatClients to be in a pipeline and not duplicate work.
  • Maybe allow FunctionCallContent / FunctionResultContent (or derivations to represent server-side invocations).

@stephentoub stephentoub marked this pull request as ready for review December 10, 2025 20:00
@stephentoub stephentoub requested a review from a team as a code owner December 10, 2025 20:00
Copilot AI review requested due to automatic review settings December 10, 2025 20:00
Copy link
Contributor

Copilot AI left a 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 an InvocationRequired property to FunctionCallContent to prevent duplicate processing of function calls in the FunctionInvokingChatClient. The property defaults to true and is set to false after a function call has been processed, allowing the middleware to distinguish between new function calls and already-processed ones. This is particularly useful in scenarios where chat history containing function calls is reused across multiple requests.

Key changes:

  • Added InvocationRequired boolean property to FunctionCallContent with JSON serialization support (omitted when false)
  • Modified FunctionInvokingChatClient to set the property to false after processing and filter out function calls with InvocationRequired = false
  • Added comprehensive test coverage including unit tests for serialization behavior and integration tests for filtering logic

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs Adds InvocationRequired property with field initializer defaulting to true, marked as experimental, with JsonIgnoreCondition.WhenWritingDefault to omit when false
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs Updates CopyFunctionCalls to filter out function calls where InvocationRequired = false and sets the property to false in ProcessFunctionCallAsync and CreateFunctionResultContent
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs Adds 6 unit tests covering property defaults, getters/setters, and JSON serialization/deserialization behavior
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs Adds 4 integration tests for the filtering behavior and a ResetPlanFunctionCallStates helper method to reset the property when reusing test plans across multiple invocations

Copy link
Contributor Author

Copilot AI commented Dec 10, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • securitytools.pkgs.visualstudio.com
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.23.7/x64/codeql/csharp/tools/linux64/Semmle.Autobuild.CSharp /opt/hostedtoolcache/CodeQL/2.23.7/x64/codeql/csharp/tools/linux64/Semmle.Autobuild.CSharp (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@rogerbarreto
Copy link
Contributor

rogerbarreto commented Dec 12, 2025

Assuming we will always have a FRC for invoked FCC even for server-side invoked FCCs.

The representation of a function that was already invoked already exists as the FRC, I understand the need for it but I'm not sure if this should be represented as an extra serializeable boolean in the FCC with not specific pointer/hint.

I suggest that we might just have a nullable [JsonIgnore] Result property pointing to what is the FRC for that FCC when it was already invoked.

Further downstream FICC could check if the FCC has a FRC as to know if it was invoked, this makes some API usage more intuitive as well.

Often I found myself having to store the FCC.Id to map what was the FRC.CallId, this would make some calling code easier to handle.

@javiercn
Copy link
Member

@javiercn, @jozkee, @westey-m, thoughts on this approach?

The goal here is to:

  • Enable an IChatClient to produce FunctionCallContent / FunctionResultContent that should not be re-invoked by callers.
  • Allow callers to distinguish whether an FCC is informative or a request. Today when a consumer gets back an FCC they don't know if it's been handled, and need to look for a corresponding FRC to determine whether it should be handled or not.
  • Allow multiple FunctionInvocationChatClients to be in a pipeline and not duplicate work.
  • Maybe allow FunctionCallContent / FunctionResultContent (or derivations to represent server-side invocations).

Could we instead look for the matching FunctionResultContent on the stream? If we see a function -> result pair on the response we can consider it as handled, couldn't we?

I think this goes in line with what @rogerbarreto has in mind.

@stephentoub stephentoub requested a review from jozkee January 23, 2026 21:20
@stephentoub stephentoub force-pushed the copilot/add-invocation-required-property branch from 6f38322 to 30d0ee7 Compare January 23, 2026 22:05
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address feedback and resolve conflicts with main.

- Removed redundant tests (InvocationRequired_DefaultsToTrue, InvocationRequired_CanBeSetToFalse)
- Consolidated serialization tests into single theory with true/false parameters
- Consolidated deserialization tests into single theory with true/false parameters
- Consolidated roundtrip tests into single theory with true/false parameters
- Replaced ResetPlanFunctionCallStates with CloneContents approach to avoid sharing mutable state
- All 400 AI tests + 1118 abstraction tests passing

Co-authored-by: jozkee <[email protected]>
Copy link
Contributor Author

Copilot AI commented Jan 26, 2026

@copilot address feedback and resolve conflicts with main.

Addressed all test refactoring feedback in 8fa1a6b:

  • Removed redundant tests and consolidated into theories
  • Replaced ResetPlanFunctionCallStates with CloneContents approach
  • All 400 AI tests + 1118 abstraction tests passing

Note: I haven't resolved merge conflicts with main yet as there are conflicts in test files. I'll need assistance resolving those since I cannot use git commit/push directly.

Copilot AI requested a review from jozkee January 26, 2026 20:36
…lot/add-invocation-required-property

# Conflicts:
#	test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
- Changed InvocationRequired from "Experimental" to "Stable" in JSON file
- Added Assert.True(c.InvocationRequired) to Constructor_PropsDefault test
- Removed redundant InvocationRequired_InnerClientWithInvocationRequiredFalsePassesThroughUnprocessed test (covered by other test)
- Converted InvocationRequired tests to theories with streaming parameter where applicable
- Fixed static lambda issue in GenerateRejectedFunctionResults
- All 428 AI tests + 1183 abstraction tests passing

Co-authored-by: jozkee <[email protected]>
Copilot AI requested a review from jozkee January 26, 2026 23:34
- Removed redundant InvocationRequired = false assignment in CreateResponseMessages (already set in ProcessFunctionCallAsync)
- Refactored InvocationRequired tests to use ternary operator with ToChatResponseAsync() to reduce code duplication
- Added test in FunctionInvokingChatClientApprovalsTests to verify FunctionCallContent with InvocationRequired=false are not replaced with approval requests
- Updated approval logic to skip FunctionCallContent where InvocationRequired=false
- All 430 AI tests + 1183 abstraction tests passing

Co-authored-by: jozkee <[email protected]>
Copilot AI requested a review from jozkee January 27, 2026 00:21
@stephentoub stephentoub enabled auto-merge (squash) January 29, 2026 14:41
@stephentoub stephentoub merged commit 7f749a1 into main Jan 29, 2026
6 checks passed
@stephentoub stephentoub deleted the copilot/add-invocation-required-property branch January 29, 2026 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-ai Microsoft.Extensions.AI libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants