Skip to content

Conversation

htilssu
Copy link
Contributor

@htilssu htilssu commented Oct 10, 2025

Motivation and Context

1. Why is this change required?

Creating a ChatClientAgentOptions instance with the constructor that accepts only the instruction argument caused the instruction text to be duplicated internally when the agent options were later merged into chat options.

2. What problem does it solve?

This change removes redundant instruction concatenation logic to ensure that Instructions are applied only once.
It prevents recursive or repeated values being passed into ChatOptions during agent initialization.

3. What scenario does it contribute to?

Improves consistency and correctness when initializing ChatClientAgent instances with predefined instructions, especially in scenarios where low-level options are already configured through CreateConfiguredChatOptions().

Description

Motivation and Context

When ChatClientAgent.RunAsync() is called with requestOptions == null,
the logic at

if (!string.IsNullOrWhiteSpace(this.Instructions))
{
chatOptions ??= new();
chatOptions.Instructions = string.IsNullOrWhiteSpace(chatOptions.Instructions) ? this.Instructions : $"{this.Instructions}\n{chatOptions.Instructions}";
}

manually concatenates this.Instructions with chatOptions.Instructions.

However, this.Instructions is just a proxy for
_agentOptions.Instructions

/// </remarks>
public string? Instructions => this._agentOptions?.Instructions;

and during ChatClientAgentOptions construction, the same instruction value is already
propagated into its ChatOptions field:

if (instructions is not null)
{
(this.ChatOptions ??= new()).Instructions = instructions;
}

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 Copilot AI review requested due to automatic review settings October 10, 2025 11:41
Copy link
Contributor

@Copilot 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

Fixes a bug where system instructions were duplicated when creating a ChatClientAgentOptions instance using the constructor that accepts only the instruction parameter. The issue occurred because instructions were being set both in the constructor and again during chat options merging.

  • Removed redundant instruction assignment in ChatClientAgentOptions constructor
  • Added test case to verify single instruction assignment behavior

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgentOptions.cs Removes duplicate instruction assignment logic from constructor
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgentTests.cs Adds test case to verify instructions are not duplicated when using constructor

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@github-actions github-actions bot changed the title Duplicate System Instruction when creating a ChatClientAgentOptions instance with the constructor .NET: Duplicate System Instruction when creating a ChatClientAgentOptions instance with the constructor Oct 10, 2025
@stephentoub stephentoub requested a review from westey-m October 10, 2025 15:41
@htilssu
Copy link
Contributor Author

htilssu commented Oct 10, 2025

I noticed that six tests are currently failing because they depend on the code in constructor I just removed.
Would you prefer that I update those tests to match the new implementation, or remove them if they’re no longer applicable?

@westey-m
Copy link
Contributor

I noticed that six tests are currently failing because they depend on the code in constructor I just removed. Would you prefer that I update those tests to match the new implementation, or remove them if they’re no longer applicable?

@htilssu, thanks for the contribution!
Yes, please update those tests to match the new implementation. From what I can tell they should still be applicable, just the part that checks for instructions in both the ChatClientAgentOptions and ChatOptions is wrong.

@htilssu
Copy link
Contributor Author

htilssu commented Oct 10, 2025

@htilssu, thanks for the contribution! Yes, please update those tests to match the new implementation. From what I can tell they should still be applicable, just the part that checks for instructions in both the ChatClientAgentOptions and ChatOptions is wrong.

@westey-m Thanks for the feedback! I've updated the unit tests accordingly to align with the new implementation.

@htilssu
Copy link
Contributor Author

htilssu commented Oct 15, 2025

@TaoChenOSU Could you please review the changes in this PR when you have a moment?

@westey-m westey-m enabled auto-merge October 16, 2025 09:15
@westey-m westey-m added this pull request to the merge queue Oct 16, 2025
Merged via the queue into microsoft:main with commit 9677899 Oct 16, 2025
14 checks passed
@htilssu htilssu deleted the htilssu/duplicate_system_instruction branch October 16, 2025 09:42
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