Skip to content

Remove exception throw for unsuccessful HTTP responses in SendChatHistoryAsync#156

Merged
pontemonti merged 2 commits intousers/johanb/RealTimeThreatProtectionfrom
copilot/sub-pr-148
Dec 31, 2025
Merged

Remove exception throw for unsuccessful HTTP responses in SendChatHistoryAsync#156
pontemonti merged 2 commits intousers/johanb/RealTimeThreatProtectionfrom
copilot/sub-pr-148

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Dec 31, 2025

The SendChatHistoryAsync implementation threw InvalidOperationException on unsuccessful HTTP responses, contradicting the interface contract which states: "HTTP exceptions are caught and logged but not rethrown. The method will complete successfully even if the HTTP request fails."

Changes

  • Removed exception throw on non-success HTTP status codes - replaced with direct error logging
  • Restructured control flow - moved success log into else block for clarity

Before

if (!response.IsSuccessStatusCode)
{
    var errorContent = await response.Content.ReadAsStringAsync();
    throw new InvalidOperationException(
        $"Failed to send chat history. Status: {response.StatusCode}, Error: {errorContent}");
}
this._logger.LogInformation("Successfully sent chat history to MCP platform");

After

if (!response.IsSuccessStatusCode)
{
    var errorContent = await response.Content.ReadAsStringAsync();
    this._logger.LogError("Failed to send chat history. Status: {StatusCode}, Error: {ErrorContent}", 
        response.StatusCode, errorContent);
}
else
{
    this._logger.LogInformation("Successfully sent chat history to MCP platform");
}

The exception was being caught by the general exception handler and logged anyway - this eliminates the redundant throw/catch cycle and aligns behavior with documentation.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix feedback on API implementation for sending chat history Remove exception throw for unsuccessful HTTP responses in SendChatHistoryAsync Dec 31, 2025
Copilot AI requested a review from pontemonti December 31, 2025 06:12
@pontemonti pontemonti marked this pull request as ready for review December 31, 2025 06:15
@pontemonti pontemonti requested a review from a team as a code owner December 31, 2025 06:15
@pontemonti pontemonti merged commit bbfd7fa into users/johanb/RealTimeThreatProtection Dec 31, 2025
1 check passed
@pontemonti pontemonti deleted the copilot/sub-pr-148 branch December 31, 2025 06:15
pontemonti added a commit that referenced this pull request Jan 6, 2026
…148)

* Initial implementation of API to send chat history to MCP platform.

* Address PR feedback to clarify documentation.

* Address PR feedback about documentation

* Address PR feedback related to references to instance fields

* Address PR feedback related to documentation

* Address PR feedback about required vs optional properties

* Update src/Tooling/Core/Services/McpToolServerConfigurationService.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/Tooling/Core/Utils/Utility.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/Tests/Microsoft.Agents.A365.Tooling.Tests/Models/ChatHistoryMessageTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Use short-form Encoding.UTF8 instead of fully qualified name (#153)

* Initial plan

* Use Encoding.UTF8 instead of fully qualified name

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Document exception handling behavior in SendChatHistoryAsync XML comments (#152)

* Initial plan

* Update XML documentation for SendChatHistoryAsync methods to clarify exception handling

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Refactor HttpClient to use IHttpClientFactory to prevent socket exhaustion (#150)

* Initial plan

* Refactor HttpClient usage to use IHttpClientFactory

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Use RuntimeUtility.GetDefaultHttpClient with IHttpClientFactory

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Remove unnecessary using statement

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Add comprehensive test coverage for SendChatHistoryAsync parameter validation (#151)

* Initial plan

* Add comprehensive tests for SendChatHistoryAsync method

- Added tests for missing Activity properties (conversation ID, message ID, user message)
- All tests validate expected exception handling
- Tests verify that InvalidOperationException is thrown with appropriate error messages
- Tests cover both method overloads (with and without ToolOptions)
- Existing null parameter validation tests remain unchanged

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Fix tests to properly mock distinct Activity property scenarios

- SendChatHistoryAsync_MissingConversationId now properly mocks a null Conversation property
- SendChatHistoryAsync_MissingMessageId now mocks null Id with valid Conversation
- SendChatHistoryAsync_MissingUserMessage now mocks null Text with valid Conversation
- Each test validates a distinct error path through the method
- All 8 tests pass successfully

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Address code review feedback: improve code style

- Remove excessive blank lines between methods
- Use explicit cast syntax (Type)null! instead of null as Type!
- Maintain consistent null handling across all test methods
- All 8 tests continue to pass

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Update src/Tooling/Extensions/AgentFramework/Agent365AgentFrameworkSdkUserAgentConfiguration.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix missing IHttpClientFactory parameter in test constructor calls (#154)

* Initial plan

* Fix test failures by adding missing IHttpClientFactory parameter to McpToolServerConfigurationService constructor calls

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Remove exception throw for unsuccessful HTTP responses in SendChatHistoryAsync (#156)

* Initial plan

* Remove exception throw for failed HTTP requests in SendChatHistoryAsync

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Add happy path test coverage for SendChatHistoryAsync HTTP POST (#157)

* Initial plan

* Add comprehensive happy path tests for SendChatHistoryAsync method

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Fix flaky test behavior by using fixed timestamps instead of DateTimeOffset.UtcNow

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Update src/Tooling/Extensions/AzureAIFoundry/Agent365AzureAIFoundrySdkUserAgentConfiguration.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/Tooling/Extensions/SemanticKernel/Agent365SemanticKernelSdkUserAgentConfiguration.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/Tests/Microsoft.Agents.A365.Tooling.Tests/Services/McpToolServerConfigurationServiceTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/Tests/Microsoft.Agents.A365.Tooling.Tests/Services/McpToolServerConfigurationServiceTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/Tests/Microsoft.Agents.A365.Tooling.Tests/Services/McpToolServerConfigurationServiceTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Enforce IHttpClientFactory as required parameter in GetDefaultHttpClient (#162)

* Initial plan

* Make IHttpClientFactory required in GetDefaultHttpClient

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Add test coverage for non-success HTTP response scenarios in SendChatHistoryAsync (#161)

* Initial plan

* Add test coverage for non-success HTTP response scenarios (400, 401, 403, 404, 500)

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Refactor HTTP error status tests to use parameterized theory approach

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Update src/Tests/Microsoft.Agents.A365.Tooling.Tests/Services/McpToolServerConfigurationServiceTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/Tooling/Core/Models/ChatMessageRequest.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix HttpResponseMessage disposal in SendChatHistoryAsync (#163)

* Initial plan

* Wrap HttpResponseMessage in using statement for proper disposal

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Fix resource leak from undisposed HttpResponseMessage in test mocks (#164)

* Initial plan

* Fix HttpResponseMessage disposal issues by using lambda factory pattern

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Update src/Tests/Microsoft.Agents.A365.Tooling.Tests/Services/McpToolServerConfigurationServiceTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix HttpResponseMessage disposal race condition in async mock test (#165)

* Initial plan

* Fix HttpResponseMessage disposal race condition in test

Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>

* Redesign SendChatHistoryAsync to return a result object.

* Add CancellationToken argument

---------

Co-authored-by: Johan Broberg <johanb@microsoft.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pontemonti <7850950+pontemonti@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants