-
Notifications
You must be signed in to change notification settings - Fork 1k
.NET: Merge AgentRunOptions.AdditionalProperties into ChatOptions.AdditionalProperties #3184
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?
.NET: Merge AgentRunOptions.AdditionalProperties into ChatOptions.AdditionalProperties #3184
Conversation
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 pull request implements the merging of AgentRunOptions.AdditionalProperties into ChatOptions.AdditionalProperties to enable proper propagation of additional properties through the agent execution stack. The PR also refactors ChatOptions merging tests into a dedicated test file for better organization.
Changes:
- Added logic to merge
AgentRunOptions.AdditionalPropertiesintoChatOptions.AdditionalPropertieswith highest precedence - Refactored ChatOptions merging unit tests from
ChatClientAgentTests.csinto a new dedicated fileChatClientAgent_ChatOptionsMergingTests.cs - Enhanced the
ChatOptionsMergingPrioritizesRequestOptionsOverAgentOptionsAsynctest to verify three-level precedence (agent → request → run options)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs | Added ApplyAgentRunOptionsOverrides logic to merge AgentRunOptions.AdditionalProperties into ChatOptions.AdditionalProperties; minor code style improvement in AdditionalProperties iteration |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatOptionsMergingTests.cs | New dedicated test file containing all ChatOptions merging tests, including enhanced test for three-level precedence validation |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgentTests.cs | Removed ChatOptions merging tests (moved to dedicated file) |
...et/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatOptionsMergingTests.cs
Outdated
Show resolved
Hide resolved
...et/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatOptionsMergingTests.cs
Outdated
Show resolved
Hide resolved
...et/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatOptionsMergingTests.cs
Show resolved
Hide resolved
| chatOptions.ContinuationToken = agentContinuationToken!.InnerToken; | ||
| } | ||
|
|
||
| // Add/Replace any additional properties from the AgentRunOptions, since they should always take precedence. |
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.
Just curious what's the rationale for them always taking precedence?
If the AgentRunOptions was actually a ChatClientAgentRunOptions and included a ChatOptions, we're saying that AgentRunOptions.AdditionalProperties takes precedence over ChatClientAgentRunOptions.ChatOptions.AdditionalProperties?
I don't have a reason to disagree, but I'm not sure what that's the obvious ordering?
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.
My thinking is that AgentRunOptions and ChatClientAgentRunOptions are more directly visible to callers than ChatClientAgentRunOptions.ChatOptions. As in, this is the main type that you discover when calling Run*, whereas ChatClientAgentRunOptions.ChatOptions is less discoverable and harder to use. You need to break out of the abstraction and then ignore the properties on AgentRunOptions directly and use the ones on ChatOptions. Therefore the top level type should take precedence, since it should be more commonly used.
That said, you could probably argue that the harder to use one should take precedence, since users would need to be more deliberate to use it. 😄
I don't feel strongly about it though, and happy to hear alternative arguments.
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 feel strongly about it, either.
My rationale for expecting it to be the other way was that a developer isn't going to be creating the ChatClientAgentRunOptions around a ChatOptions and specifying the same property explicitly in the AdditionalProperties on both, that just wouldn't make sense. My expectation is if they're in this situation, they've been passed the ChatOptions from somewhere higher out and are then wrapping that opaque options bag in a ChatClientAgentRunOptions, adding their own AdditionalProperties to the wrapper, at which point they could possibly conflict. Given that, I think our typical policy would be to give preference to the outermost caller, treating whatever they provide as an override.
Up to you. This is a corner case that's unlikely to happen very frequently if at all.
Motivation and Context
We should pass AgentRunOptions.AdditionalProperties to ChatOptions.AdditionalProperties in ChatClientAgent so that additional properties are replicated all the way down the stack.
The allows us to set a property on AgentRunOptions.AdditionalProperties without needing to break out of the abstraction and then access the property in a function tool via
FunctionInvokingChatClient.CurrentContext.Options?.AdditionalProperties#3179
Description
Contribution Checklist