diff --git a/docs/MessageMerger-EdgeCases-TestCoverage.md b/docs/MessageMerger-EdgeCases-TestCoverage.md new file mode 100644 index 0000000000..eda1cfa21c --- /dev/null +++ b/docs/MessageMerger-EdgeCases-TestCoverage.md @@ -0,0 +1,213 @@ +# MessageMerger Edge Cases and Test Coverage + +This document identifies edge cases in `MessageMerger` and recommends additional test coverage. + +## Overview + +`MessageMerger` (`dotnet/src/Microsoft.Agents.AI.Hosting.AzureFunctions/MessageMerger.cs`) handles merging streaming `AgentResponseUpdate` messages into a final `AgentResponse`. It groups updates by `ResponseId`, then by `MessageId`, sorts messages by `CreatedAt` timestamp, and produces a consolidated response. + +--- + +## Edge Cases Identified + +| # | Edge Case | Risk | Current Test Coverage | +|---|-----------|------|----------------------| +| 1 | **Non-transitive timestamp comparison** — mixed timestamped/untimestamped updates with 3+ messages can produce inconsistent sort order | High | ❌ Not covered | +| 2 | **Cross-ResponseId ordering** — messages from different ResponseIds are emitted in first-seen order, not chronological | Medium | ❌ Not covered | +| 3 | **ResponseId=null updates always last** — dangling updates appended after all response-scoped messages regardless of arrival time | Medium | ❌ Not covered | +| 4 | **MessageId=null updates within a response** — keyed messages always precede message-id-less updates | Low | ❌ Not covered | +| 5 | **CreatedAt overwritten** — all messages in a merged response get same `CreatedAt`, erasing original timestamps | Low | Partial (test asserts message timestamps) | +| 6 | **Dangling metadata lost** — `FinishReason`, `Usage`, `AgentId` from ResponseId=null updates not merged | Medium | ❌ Not covered | +| 7 | **Unused `createdTimes` collection** — final response uses `UtcNow`, collected times are unused | Low (code smell) | N/A | + +--- + +## Edge Case Details + +### 1. Non-transitive Timestamp Comparison (High Risk) + +**Problem:** The `OrderBy` lambda uses a comparison that isn't transitive when some messages have `CreatedAt` and others don't: + +```csharp +.OrderBy(kvp => kvp.Value.CreatedAt ?? m.CreatedAt) +``` + +With three messages: +- A: `CreatedAt = 10`, idx=0 +- B: `CreatedAt = null`, idx=1 +- C: `CreatedAt = 5`, idx=2 + +The comparison produces: A < B (10 < 10=false, so equal), B < C (10 < 5=false), but A > C (10 > 5). This violates transitivity and can cause non-deterministic sort results. + +**Recommendation:** Use insertion order as fallback for null timestamps, or store original index. + +--- + +### 2. Cross-ResponseId Ordering (Medium Risk) + +**Problem:** When multiple `ResponseId`s arrive interleaved, messages are emitted in response-first-seen order: + +``` +Updates: R1-msg1 → R2-msg1 → R1-msg2 → R2-msg2 +Output: [R1-msg1, R1-msg2], [R2-msg1, R2-msg2] +``` + +This may not match chronological arrival order. + +--- + +### 3. ResponseId=null Updates Always Last (Medium Risk) + +**Problem:** Updates with `ResponseId = null` are grouped as "dangling" and always appended last, regardless of when they arrived: + +```csharp +if (update.ResponseId is null) +{ + danglingUpdates.Add(update); + continue; +} +``` + +This means metadata-only updates sent early in the stream appear at the end. + +--- + +### 4. MessageId=null Updates Within a Response (Low Risk) + +**Problem:** Within a `ResponseId` group, updates with `MessageId = null` are stored in a separate list and appended after keyed messages: + +```csharp +if (update.MessageId is null) +{ + grouping.Value.noIds.Add(update); +} +``` + +This is likely intentional but not documented or tested. + +--- + +### 5. CreatedAt Overwritten (Low Risk) + +**Problem:** All messages in the final response receive the same `CreatedAt` timestamp: + +```csharp +message.CreatedAt = m.CreatedAt ?? DateTimeOffset.UtcNow; +``` + +Individual message timestamps are overwritten with the response-level timestamp or current time. + +--- + +### 6. Dangling Metadata Lost (Medium Risk) + +**Problem:** When `ResponseId = null` updates contain `FinishReason`, `Usage`, or `AgentId`, these values are not merged into the final response: + +```csharp +// Dangling updates become orphan messages, not merged with response metadata +foreach (var orphan in danglingUpdates) +{ + orphanMessages.Add(CreateAgentMessage(orphan)); +} +``` + +--- + +### 7. Unused `createdTimes` Collection (Low Risk - Code Smell) + +**Problem:** The code collects `CreatedAt` values into `createdTimes` list but never uses them: + +```csharp +List createdTimes = []; +// ... later ... +if (update.CreatedAt.HasValue) createdTimes.Add(update.CreatedAt.Value); +// createdTimes is never used +``` + +--- + +## Recommended Additional Tests + +### High Priority + +1. **Non-transitive sorting with mixed timestamps** + ```csharp + [Fact] + public void MergeMessages_MixedTimestamps_ProducesStableOrder() + { + // Arrange: 3 messages - A (CreatedAt=10), B (null), C (CreatedAt=5) + // Act: Merge + // Assert: Order is deterministic (either chronological or insertion order) + } + ``` + +2. **Function call/result sequencing with 3+ messages** + ```csharp + [Fact] + public void MergeMessages_FunctionCallAndResultsWithMixedTimestamps_PreservesLogicalOrder() + { + // Arrange: FunctionCall (null), FunctionResult (T1), Assistant (null) + // Act: Merge + // Assert: FunctionCall precedes FunctionResult precedes Assistant + } + ``` + +### Medium Priority + +3. **Multiple ResponseIds interleaved** + ```csharp + [Fact] + public void MergeMessages_InterleavedResponseIds_GroupsByResponseId() + { + // Arrange: R1-msg1, R2-msg1, R1-msg2, R2-msg2 + // Act: Merge + // Assert: Messages grouped by ResponseId, verify order + } + ``` + +4. **Dangling updates with FinishReason/Usage** + ```csharp + [Fact] + public void MergeMessages_DanglingUpdatesWithMetadata_MetadataPropagates() + { + // Arrange: ResponseId=null update with FinishReason=Stop, Usage=(10,20,30) + // Act: Merge + // Assert: Final response contains FinishReason and Usage + } + ``` + +5. **ResponseId=null timing** + ```csharp + [Fact] + public void MergeMessages_DanglingUpdatesFirst_AppearsAfterKeyedMessages() + { + // Arrange: null-response update, then keyed update + // Act: Merge + // Assert: Keyed messages appear before dangling + } + ``` + +### Low Priority + +6. **MessageId=null updates ordering** + ```csharp + [Fact] + public void MergeMessages_MixedMessageIds_KeyedBeforeUnkeyed() + { + // Arrange: Mix of keyed and unkeyed updates within same ResponseId + // Act: Merge + // Assert: Keyed messages precede unkeyed in arrival order + } + ``` + +--- + +## Summary + +| Priority | Test Count | Risk Addressed | +|----------|------------|----------------| +| High | 2 | Non-deterministic sorting, Function sequencing | +| Medium | 3 | ResponseId grouping, Metadata propagation, Timing | +| Low | 1 | MessageId ordering | + +**Recommendation:** Add at minimum the 2 high-priority tests and 2 medium-priority tests (interleaved ResponseIds, dangling metadata) to ensure `MessageMerger` behaves correctly in production streaming scenarios.