Skip to content

Conversation

@DeagleGross
Copy link
Contributor

@DeagleGross DeagleGross commented Dec 4, 2025

In #2477 the problem was raised: client's conversationId does not map to what IChatClient expects in the options.

We should not forward the client's conversationId into IChatClient.

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 December 4, 2025 11:38
@DeagleGross DeagleGross requested review from ReubenBond and removed request for Copilot December 4, 2025 11:39
@DeagleGross DeagleGross self-assigned this Dec 4, 2025
@DeagleGross DeagleGross requested review from ReubenBond and stephentoub and removed request for ReubenBond December 4, 2025 11: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

This PR addresses an issue where the client's conversationId was incorrectly being forwarded to the underlying IChatClient via ChatOptions.ConversationId. The fix ensures that conversation management remains at the hosting layer level and is not passed down to the chat client, which may have its own conversation handling or none at all.

Key changes:

  • Removed forwarding of ConversationId from request to ChatOptions in response executors
  • Added comprehensive tests to verify the fix works for both streaming and non-streaming scenarios
  • Enhanced test infrastructure to track ChatOptions passed to mock chat clients

Reviewed changes

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

File Description
dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/AIAgentResponseExecutor.cs Removed ConversationId assignment to ChatOptions with explanatory comment
dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/HostedAgentResponseExecutor.cs Removed ConversationId assignment to ChatOptions with explanatory comment
dotnet/tests/Microsoft.Agents.AI.Hosting.OpenAI.UnitTests/TestHelpers.cs Added LastChatOptions property to SimpleMockChatClient for test verification
dotnet/tests/Microsoft.Agents.AI.Hosting.OpenAI.UnitTests/OpenAIResponsesIntegrationTests.cs Added two integration tests and helper methods to verify conversationId is not forwarded to IChatClient

@DeagleGross DeagleGross added this pull request to the merge queue Dec 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2025
@DeagleGross DeagleGross added this pull request to the merge queue Dec 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 5, 2025
@DeagleGross DeagleGross added this pull request to the merge queue Dec 5, 2025
Merged via the queue into main with commit 417a529 Dec 5, 2025
14 of 15 checks passed
@DeagleGross DeagleGross deleted the dmkorolev/hosting-conversations branch December 5, 2025 15:12
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.

5 participants

SYSTEM_READY >> ...MS