Skip to content

Conversation

@westey-m
Copy link
Contributor

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

  • Replicate AgentRunOptions.AdditionalProperties to ChatOptions.AdditionalProperties in ChatClientAgent
  • Move unit tests for ChatOptions Merging in a separate class to reduce the size of ChatClientAgentTests
  • Modify ChatOptionsMergingPrioritizesRequestOptionsOverAgentOptionsAsync to verify the new scenario

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass, and I have added new tests where possible
  • Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

Copilot AI review requested due to automatic review settings January 12, 2026 18:41
@github-actions github-actions bot changed the title Merge AgentRunOptions.AdditionalProperties into ChatOptions.AdditionalProperties .NET: Merge AgentRunOptions.AdditionalProperties into ChatOptions.AdditionalProperties Jan 12, 2026
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 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.AdditionalProperties into ChatOptions.AdditionalProperties with highest precedence
  • Refactored ChatOptions merging unit tests from ChatClientAgentTests.cs into a new dedicated file ChatClientAgent_ChatOptionsMergingTests.cs
  • Enhanced the ChatOptionsMergingPrioritizesRequestOptionsOverAgentOptionsAsync test 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)

chatOptions.ContinuationToken = agentContinuationToken!.InnerToken;
}

// Add/Replace any additional properties from the AgentRunOptions, since they should always take precedence.
Copy link
Member

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?

Copy link
Contributor Author

@westey-m westey-m Jan 13, 2026

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.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants