Skip to content

Conversation

@westey-m
Copy link
Contributor

Motivation and Context

#2325

Description

  • Adding Remove
  • Switching to TryGet from Get
  • Removing indexer
  • Requiring all features to be non-null
  • Adding class restriction to TFeature

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 November 21, 2025 15:56
@github-actions github-actions bot changed the title Update AgentFeatureCollections with feedback .NET: Update AgentFeatureCollections with feedback Nov 21, 2025
Copilot finished reviewing on behalf of westey-m November 21, 2025 16: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 updates the AgentFeatureCollection API based on feedback from issue #2325. The changes improve the API design by replacing the indexer with a TryGet method, requiring non-null features, and adding a Remove method.

Key changes:

  • Replaced Get<TFeature>() method with TryGet<TFeature>(out TFeature feature) for safer feature retrieval
  • Removed the indexer this[Type key] in favor of explicit Set, TryGet, and Remove methods
  • Added Remove<TFeature>() method to explicitly remove features from the collection
  • Added class constraint to all generic type parameters and made all features non-nullable
  • Added WithFeature extension method for fluent API usage
  • Updated all usages across the codebase to use the new API

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
dotnet/src/Microsoft.Agents.AI.Abstractions/Features/IAgentFeatureCollection.cs Updated interface with TryGet, Remove, and Set methods, removed indexer
dotnet/src/Microsoft.Agents.AI.Abstractions/Features/AgentFeatureCollection.cs Implemented new API methods, renamed _defaults to _innerCollection for clarity
dotnet/src/Microsoft.Agents.AI.Abstractions/Features/AgentFeatureCollectionExtensions.cs Added new extension methods file with WithFeature helper
dotnet/src/Microsoft.Agents.AI.Abstractions/Features/ConversationIdAgentFeature.cs Updated documentation to remove reference to 3rd party stores
dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs Updated to use TryGet pattern instead of Get
dotnet/src/Microsoft.Agents.AI.CopilotStudio/CopilotStudioAgent.cs Updated to use TryGet pattern instead of Get
dotnet/src/Microsoft.Agents.AI.A2A/A2AAgent.cs Updated to use TryGet pattern instead of Get
dotnet/samples/GettingStarted/Agents/Agent_Step07_3rdPartyThreadStorage/Program.cs Updated sample to use new WithFeature extension method
dotnet/tests/Microsoft.Agents.AI.Abstractions.UnitTests/AgentFeatureCollectionTests.cs Updated tests to cover new API methods including Remove and TryGet

Copilot AI review requested due to automatic review settings November 21, 2025 16:38
Copilot finished reviewing on behalf of westey-m November 21, 2025 16:39
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

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

@stephentoub
Copy link
Member

I see AgentRunOptions now has both AdditionalProperties and Features:

public AdditionalPropertiesDictionary? AdditionalProperties { get; set; }
/// <summary>
/// Gets or sets the collection of features provided by the caller and middleware for this run.
/// </summary>
public IAgentFeatureCollection? Features { get; set; }

What's the use case for having both? When would I pick one over the other? Do we need Features if AdditionalProperties exists, or vice versa?

Copilot AI review requested due to automatic review settings November 21, 2025 17:27
@westey-m
Copy link
Contributor Author

I see AgentRunOptions now has both AdditionalProperties and Features:

public AdditionalPropertiesDictionary? AdditionalProperties { get; set; }
/// <summary>
/// Gets or sets the collection of features provided by the caller and middleware for this run.
/// </summary>
public IAgentFeatureCollection? Features { get; set; }

What's the use case for having both? When would I pick one over the other? Do we need Features if AdditionalProperties exists, or vice versa?

@stephentoub, yeah, this still bugs me as well. Let's discuss directly offline. I don't intend to merge the feature branch until we have made a decision on this.
CC @ReubenBond

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

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

Copilot AI review requested due to automatic review settings November 21, 2025 17:37
Copilot finished reviewing on behalf of westey-m November 21, 2025 17:38
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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Copilot AI review requested due to automatic review settings November 21, 2025 18:27
Copilot finished reviewing on behalf of westey-m November 21, 2025 18:30
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

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

Copy link
Member

@ReubenBond ReubenBond left a comment

Choose a reason for hiding this comment

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

Just one comment about the indexer removal

@westey-m westey-m merged commit 9d86adf into microsoft:feature-featurecollections-messagestore Nov 24, 2025
10 checks passed
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