mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
Rework MessageMerger note into ADR 0026
Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/9a5433df-55bf-402c-aae8-c79a6d22e677 Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
committed by
GitHub
Unverified
GitHub
parent
53693d7aea
commit
90585ad912
@@ -1,384 +0,0 @@
|
||||
# Agent Hosting Executor Invariants and Test Coverage
|
||||
|
||||
This document defines the **invariants** that Agent hosting executors must maintain, identifies edge cases in `MessageMerger`, and recommends additional test coverage.
|
||||
|
||||
## Hosting Executor Invariants
|
||||
|
||||
Agent hosting executors (e.g., `HostedAgentResponseExecutor`, `AIAgentResponseExecutor`, `WorkflowOrchestrator`) **must** maintain the following invariants:
|
||||
|
||||
### Invariant 1: Single ResponseId per Turn
|
||||
|
||||
**For a given "turn", only one ResponseId is emitted.**
|
||||
|
||||
- If the underlying Agent does not provide a `ResponseId`, the hosting executor **must** assign one.
|
||||
- All `AgentResponseUpdate` items emitted during a single turn must share the same `ResponseId`.
|
||||
- This ensures clients can reliably group streaming updates into a single logical response.
|
||||
|
||||
**Relevant Code:**
|
||||
- `IdGenerator` creates a `ResponseId` in `InMemoryResponsesService.InitializeResponse()`
|
||||
- `AgentInvocationContext.ResponseId` provides the single ResponseId for the turn
|
||||
- `ToStreamingResponseAsync()` uses `context.ResponseId` for the `Response` object
|
||||
|
||||
### Invariant 2: Output Order Preservation for Untimestamped Messages
|
||||
|
||||
**For a given turn, all messages without `CreatedAt` must be merged in output (arrival) order.**
|
||||
|
||||
- When `AgentResponseUpdate` items lack a `CreatedAt` timestamp, their relative order must be preserved based on arrival sequence.
|
||||
- The current `MessageMerger.CompareByDateTimeOffset()` falls back to index comparison when timestamps are missing or equal, which satisfies this invariant.
|
||||
- Edge case: Mixed timestamped/untimestamped messages require careful handling to avoid non-transitive sort issues.
|
||||
|
||||
**Relevant Code:**
|
||||
- `MessageMerger.CompareByDateTimeOffset()` uses index as tiebreaker
|
||||
- `ResponseMergeState.AddUpdate()` preserves insertion order in lists
|
||||
|
||||
### Invariant 3: Agent Message Grouping (No Interleaving)
|
||||
|
||||
**For multi-agent systems speaking concurrently, a given agent's response messages must all be grouped together (not interleaved with other agents' messages).**
|
||||
|
||||
- When multiple agents emit responses concurrently (e.g., in handoff scenarios), messages from each agent must remain contiguous.
|
||||
- The merged output should group messages by agent, not interleave them arbitrarily.
|
||||
- This is critical for maintaining conversation coherence in multi-agent orchestration.
|
||||
|
||||
**Relevant Code:**
|
||||
- `MessageMerger` groups by `ResponseId`, then by `MessageId`
|
||||
- Multi-agent scenarios may emit different `ResponseId` values per agent
|
||||
- `ComputeMerged()` processes responses in dictionary iteration order (first-seen order)
|
||||
|
||||
---
|
||||
|
||||
## Overview
|
||||
|
||||
`MessageMerger` (`dotnet/src/Microsoft.Agents.AI.Workflows/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<DateTimeOffset> 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.
|
||||
|
||||
---
|
||||
|
||||
## Required Tests for Hosting Executor Invariants
|
||||
|
||||
### Invariant 1: Single ResponseId per Turn
|
||||
|
||||
```csharp
|
||||
[Fact]
|
||||
public async Task HostingExecutor_AssignsSingleResponseId_WhenAgentProvidesNone()
|
||||
{
|
||||
// Arrange: Agent that emits updates without ResponseId
|
||||
// Act: Execute through hosting executor
|
||||
// Assert: All emitted updates have the same ResponseId assigned by executor
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HostingExecutor_PreservesAgentResponseId_WhenProvided()
|
||||
{
|
||||
// Arrange: Agent that emits updates with a consistent ResponseId
|
||||
// Act: Execute through hosting executor
|
||||
// Assert: ResponseId from agent is preserved (or overridden if executor policy requires)
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HostingExecutor_RejectsMultipleResponseIds_InSingleTurn()
|
||||
{
|
||||
// Arrange: Agent that incorrectly emits updates with different ResponseIds
|
||||
// Act: Execute through hosting executor
|
||||
// Assert: Executor normalizes to single ResponseId or throws validation error
|
||||
}
|
||||
```
|
||||
|
||||
### Invariant 2: Output Order Preservation
|
||||
|
||||
```csharp
|
||||
[Fact]
|
||||
public void MessageMerger_PreservesInsertionOrder_WhenNoTimestamps()
|
||||
{
|
||||
// Arrange: Multiple updates without CreatedAt, in specific order A, B, C
|
||||
// Act: Merge
|
||||
// Assert: Output order is A, B, C
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MessageMerger_PreservesInsertionOrder_WhenMixedTimestamps()
|
||||
{
|
||||
// Arrange: Updates where some have CreatedAt and some don't
|
||||
// Act: Merge
|
||||
// Assert: Untimestamped updates maintain relative order among themselves
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MessageMerger_StableSort_WithThreeOrMoreMixedTimestampMessages()
|
||||
{
|
||||
// Arrange: 3+ messages with mixed null/non-null CreatedAt values
|
||||
// Act: Merge multiple times
|
||||
// Assert: Result is deterministic and consistent across runs
|
||||
}
|
||||
```
|
||||
|
||||
### Invariant 3: Agent Message Grouping
|
||||
|
||||
```csharp
|
||||
[Fact]
|
||||
public void MessageMerger_GroupsMessagesByAgent_InMultiAgentScenario()
|
||||
{
|
||||
// Arrange: Interleaved updates from Agent1 and Agent2
|
||||
// A1-msg1, A2-msg1, A1-msg2, A2-msg2
|
||||
// Act: Merge
|
||||
// Assert: Output groups Agent1 messages together, Agent2 messages together
|
||||
// Either [A1-msg1, A1-msg2, A2-msg1, A2-msg2] or [A2-msg1, A2-msg2, A1-msg1, A1-msg2]
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MessageMerger_MaintainsAgentGrouping_WithDifferentResponseIds()
|
||||
{
|
||||
// Arrange: Agent1 uses ResponseId=R1, Agent2 uses ResponseId=R2
|
||||
// Act: Merge with primaryResponseId
|
||||
// Assert: Messages from each agent are contiguous, not interleaved
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task HostingExecutor_HandoffScenario_MaintainsMessageCoherence()
|
||||
{
|
||||
// Arrange: Agent1 hands off to Agent2 mid-conversation
|
||||
// Act: Execute through hosting executor
|
||||
// Assert: Agent1's messages appear before Agent2's messages (no interleaving)
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void MessageMerger_PreservesAgentMessageOrder_WithConcurrentAgents()
|
||||
{
|
||||
// Arrange: Two agents emitting messages concurrently with timestamps
|
||||
// A1 at T1, A2 at T2, A1 at T3, A2 at T4 (where T1 < T2 < T3 < T4)
|
||||
// Act: Merge
|
||||
// Assert: Agent grouping is maintained, not sorted purely by timestamp
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Implementation Recommendations
|
||||
|
||||
### For Invariant 1 (Single ResponseId)
|
||||
|
||||
1. **Hosting Executor Layer**: The `AgentInvocationContext` already generates a `ResponseId`. Ensure this is consistently applied to all outgoing `AgentResponseUpdate` items.
|
||||
|
||||
2. **Validation**: Consider adding runtime validation that throws if an agent emits conflicting `ResponseId` values during a single turn.
|
||||
|
||||
3. **Code Location**: `ToStreamingResponseAsync()` in `AgentResponseUpdateExtensions.cs` should ensure the `context.ResponseId` is used consistently.
|
||||
|
||||
### For Invariant 2 (Output Order)
|
||||
|
||||
1. **Index Tracking**: Store original insertion index alongside each update to ensure stable sorting.
|
||||
|
||||
2. **Code Location**: `MessageMerger.ResponseMergeState.AddUpdate()` should track insertion order explicitly.
|
||||
|
||||
3. **Sort Stability**: Replace the current `OrderBy` with a stable sort that uses insertion index as the final tiebreaker.
|
||||
|
||||
### For Invariant 3 (Agent Grouping)
|
||||
|
||||
1. **Agent-First Grouping**: Modify `ComputeMerged()` to group by `AgentId` before processing.
|
||||
|
||||
2. **Deterministic Order**: Define explicit ordering rules (e.g., first-seen agent order, or alphabetical by AgentId).
|
||||
|
||||
3. **Code Location**: `MessageMerger.ComputeMerged()` needs logic to ensure agent messages are contiguous.
|
||||
@@ -0,0 +1,157 @@
|
||||
---
|
||||
status: accepted
|
||||
contact: lokitoth
|
||||
date: 2026-05-13
|
||||
deciders: lokitoth
|
||||
consulted:
|
||||
informed:
|
||||
---
|
||||
|
||||
# MessageMerger Streaming Merge Invariants
|
||||
|
||||
## Context and Problem Statement
|
||||
|
||||
`Microsoft.Agents.AI.Workflows.MessageMerger` is the internal component that
|
||||
folds a stream of `AgentResponseUpdate` items emitted by an agent (or by a
|
||||
hosting executor wrapping an agent) into a single `AgentResponse` for a turn.
|
||||
Multi-agent workflows (handoff, group chat, orchestration) rely on this
|
||||
merger to produce a coherent transcript even when updates arrive interleaved
|
||||
across responses, messages, and timestamps.
|
||||
|
||||
Prior to this change the contract that hosting executors and the merger
|
||||
should jointly enforce was implicit. The implementation also carried a small
|
||||
amount of dead state (`createdTimes`) that was collected but never consumed,
|
||||
suggesting that an earlier, timestamp-driven ordering scheme had been
|
||||
abandoned without documentation. There were no tests pinning down the
|
||||
ordering or grouping behavior, so any future refactor risked silently
|
||||
regressing it.
|
||||
|
||||
The problem this ADR addresses is therefore: **what merge guarantees does
|
||||
`MessageMerger` make to its callers, and how do we lock those guarantees in
|
||||
without changing observable behavior in this PR?**
|
||||
|
||||
## Decision Drivers
|
||||
|
||||
- **A. Predictable ordering**: developers consuming a merged `AgentResponse`
|
||||
must be able to reason about the order in which messages appear, even when
|
||||
some updates lack `CreatedAt`.
|
||||
- **B. Coherent multi-agent transcripts**: when several agents stream
|
||||
concurrently into one merger (handoff, orchestrator-as-agent), each
|
||||
agent's contribution must read as a contiguous block.
|
||||
- **C. Stable hosting-executor contract**: a turn must be addressable by a
|
||||
single `ResponseId`; updates without one are an exceptional, "dangling"
|
||||
case rather than the norm.
|
||||
- **D. Minimal behavioral change**: this work is intended to document and
|
||||
test current behavior, not to alter what users see today.
|
||||
- **E. Discoverability for future contributors**: known sharp edges (e.g.
|
||||
cross-`ResponseId` ordering, dangling-update placement) should be written
|
||||
down so they are not rediscovered as bugs.
|
||||
|
||||
## Considered Options
|
||||
|
||||
1. **Option 1 — Document invariants and pin them with tests; remove the
|
||||
dead `createdTimes` collection.** No behavioral change.
|
||||
2. **Option 2 — Rewrite `MessageMerger` to group strictly by `AgentId`,
|
||||
and to use a stable, transitive comparer that mixes `CreatedAt` with
|
||||
insertion index.** Behavioral change; would also require updating
|
||||
hosting executors that currently assume `ResponseId`-based grouping.
|
||||
3. **Option 3 — Leave the code and tests as-is; capture edge cases only
|
||||
in a working note.** No code or test change.
|
||||
|
||||
## Decision Outcome
|
||||
|
||||
Chosen option: **Option 1 — Document invariants and pin them with tests;
|
||||
remove the dead `createdTimes` collection.**
|
||||
|
||||
This option satisfies driver D (minimal behavioral change) and driver E
|
||||
(discoverability) while making real progress on A, B, and C: by writing the
|
||||
invariants down and covering them with regression tests, future refactors
|
||||
must either preserve the invariants or explicitly supersede this ADR. The
|
||||
dead `createdTimes` collection is removed because it actively misleads
|
||||
readers about the ordering strategy.
|
||||
|
||||
Option 2 was rejected for this iteration because it changes what callers
|
||||
observe (e.g., would re-order outputs in any flow that relies on
|
||||
`ResponseId`-based grouping today) and would require a coordinated change
|
||||
across hosting executors. It is a candidate for a follow-up ADR if the
|
||||
known edge cases below are reported as bugs in practice.
|
||||
|
||||
Option 3 was rejected because it leaves the invariants un-tested and the
|
||||
dead code in place, so the next refactor can break the contract without
|
||||
any signal.
|
||||
|
||||
### Invariants
|
||||
|
||||
The following invariants are now part of the contract of
|
||||
`MessageMerger`/hosting executors and are covered by tests in
|
||||
`MessageMergerTests`:
|
||||
|
||||
1. **Single `ResponseId` per turn.** Every `AgentResponseUpdate` produced
|
||||
by a hosting executor in a single turn shares one `ResponseId`. If the
|
||||
underlying agent does not supply one, the executor assigns it. Updates
|
||||
with `ResponseId == null` are treated as "dangling" and flattened into
|
||||
loose messages at the end of the merged response.
|
||||
2. **Output order preservation for untimestamped messages.** When updates
|
||||
lack `CreatedAt`, their relative order in the merged output matches
|
||||
their arrival order. `CompareByDateTimeOffset` falls back to insertion
|
||||
index when timestamps are missing or equal.
|
||||
3. **Per-`ResponseId` grouping (no interleaving).** Messages produced
|
||||
under one `ResponseId` are emitted as a contiguous block in the merged
|
||||
`AgentResponse`. In multi-agent scenarios where each agent uses its
|
||||
own `ResponseId`, this also yields per-agent grouping.
|
||||
|
||||
### Consequences
|
||||
|
||||
- Good, because the merge contract is now explicit and regression-tested,
|
||||
reducing the risk of silent behavioral drift.
|
||||
- Good, because removing the unused `createdTimes` collection eliminates
|
||||
a misleading code smell.
|
||||
- Good, because hosting-executor authors have a written checklist of
|
||||
invariants to satisfy.
|
||||
- Neutral, because no end-user behavior changes.
|
||||
- Bad, because several known edge cases (see below) are documented but
|
||||
not fixed; consumers may still encounter them.
|
||||
|
||||
## Validation
|
||||
|
||||
- Unit tests in
|
||||
`dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MessageMergerTests.cs`
|
||||
cover each invariant:
|
||||
- Insertion-order preservation with no timestamps.
|
||||
- Insertion-order preservation with mixed timestamps.
|
||||
- Determinism across repeated runs with mixed timestamps.
|
||||
- Per-`ResponseId` grouping for interleaved multi-agent streams.
|
||||
- Per-`ResponseId` grouping with distinct response ids.
|
||||
- Existing tests for assembly, function-call/result ordering, and
|
||||
`FinishReason` propagation continue to pass.
|
||||
|
||||
## More Information
|
||||
|
||||
### Known edge cases (intentionally not fixed in this ADR)
|
||||
|
||||
These are properties of the *current* `MessageMerger` that callers should
|
||||
be aware of. They are not invariants — they may change in a future ADR —
|
||||
but they are present in the shipped behavior covered by tests above.
|
||||
|
||||
| # | Edge case | Risk | Notes |
|
||||
|---|-----------|------|-------|
|
||||
| 1 | `CompareByDateTimeOffset` is not transitive when some messages have `CreatedAt` and others do not (e.g. A=10, B=null, C=5 yields A<B by index, B<C by index, but A>C by timestamp). | Medium | `List<T>.Sort` is not guaranteed to produce a unique ordering for non-transitive comparers, but the current tests verify that repeated runs over identical input produce identical output. Use `CreatedAt` consistently per response to avoid this. |
|
||||
| 2 | Cross-`ResponseId` ordering follows first-seen-`ResponseId` order, not chronological order across responses. | Medium | Acceptable today because each turn has a single `ResponseId` (Invariant 1); only matters if a caller deliberately interleaves multiple response ids. |
|
||||
| 3 | Updates with `ResponseId == null` are always emitted **after** all keyed responses, regardless of arrival time. | Medium | Documented as the "dangling" path; agents should always emit a `ResponseId`. |
|
||||
| 4 | Within a response, updates with `MessageId == null` are always emitted **after** keyed messages. | Low | Same rationale as #3, scoped to messages within a response. |
|
||||
| 5 | The merged response's `CreatedAt` is set to `DateTimeOffset.UtcNow`; per-response `CreatedAt` is propagated onto each contained `ChatMessage` instead of being preserved at the response level. | Low | Callers who need original per-update timestamps should read them from `RawRepresentation` or capture them before merging. |
|
||||
| 6 | Metadata on dangling (`ResponseId == null`) updates — `FinishReason`, `Usage`, `AgentId`, `AdditionalProperties` — is **not** merged into the final `AgentResponse`; only their `Messages` are surfaced. | Medium | Hosting executors must attach metadata to a keyed update if they want it reflected in the merged response. |
|
||||
|
||||
If any of these become observable problems in production, the appropriate
|
||||
follow-up is a new ADR that supersedes this one (likely realizing
|
||||
"Option 2") rather than a silent fix.
|
||||
|
||||
### Code references
|
||||
|
||||
- `dotnet/src/Microsoft.Agents.AI.Workflows/MessageMerger.cs` — merger
|
||||
implementation. The previously-unused `createdTimes` collection has
|
||||
been removed in this change.
|
||||
- `dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MessageMergerTests.cs` —
|
||||
invariant tests added in this change.
|
||||
- `AgentInvocationContext.ResponseId` and `ToStreamingResponseAsync` —
|
||||
the hosting-executor side of Invariant 1.
|
||||
Reference in New Issue
Block a user