Uh oh!
There was an error while loading. Please reload this page.
Add custom message selection for AggregatorAgent - #14315
Add custom message selection for AggregatorAgent#14315medimedi (medisean) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new AggregatorMode.Custom to AggregatorAgent so callers can choose which aggregated message is surfaced to the parent chat via a configurable MessageSelector callback, including support in the streaming invocation path.
Changes:
- Introduce
AggregatorMode.CustomandAggregatorAgent.MessageSelector. - Plumb
MessageSelectorintoAggregatorChanneland implement custom selection for discrete and streaming invocations. - Extend unit test coverage to exercise
Custommode initialization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dotnet/src/Agents/Abstractions/AggregatorAgent.cs | Adds Custom mode + MessageSelector API and passes selector into the channel. |
| dotnet/src/Agents/Abstractions/AggregatorChannel.cs | Implements custom message selection logic in discrete and streaming invocations. |
| dotnet/src/Agents/UnitTests/AggregatorAgentTests.cs | Updates existing theory to include AggregatorMode.Custom and sets a selector for the custom case. |
Suppressed comments (2)
dotnet/src/Agents/Abstractions/AggregatorChannel.cs:118
- Custom-mode streaming selection is newly added here but there are no unit tests covering InvokeStreamingAsync behavior (selector invocation, ordering, and returned content). Without coverage, regressions in the streaming path are likely to go unnoticed.
else if (agent.Mode == AggregatorMode.Custom)
{
IReadOnlyList<ChatMessageContent> generatedMessages = history.Take(history.Count - initialCount).ToList();
ChatMessageContent? selected = this.SelectMessage(generatedMessages);
if (selected is not null)
dotnet/src/Agents/Abstractions/AggregatorAgent.cs:114
- There is an extra statement terminator after DeserializeAsync, which is harmless but should be removed to keep the method clean and avoid tripping style analyzers.
await chat.DeserializeAsync(agentChatState).ConfigureAwait(false); ;
AggregatorChannel channel = new(chat, this.MessageSelector);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (agent.Mode == AggregatorMode.Custom) | ||
| { | ||
| ChatMessageContent? selected = this.SelectMessage(messages!); | ||
| if (selected is not null) | ||
| { | ||
| yield return (IsVisible: true, selected); | ||
| } | ||
| } |
| /// <summary> | ||
| /// Gets the callback used to select the message exposed to the owning chat when <see cref="Mode"/> is <see cref="AggregatorMode.Custom"/>. | ||
| /// The messages are ordered newest first, matching the aggregated chat history. | ||
| /// </summary> |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): d545ca9c9eb0
Model:claude-opus-4.8
Overview
The PR adds an AggregatorMode.Custom value and a developer-supplied MessageSelector
callback so callers can choose which aggregated message is surfaced to the owning chat.
The change is additive and well isolated: existing Flat/Nested behavior is untouched,
a null selector return is handled in both paths, and a missing selector under Custom
throws a clear InvalidOperationException rather than an NRE. The residual risk is that
the selector's input list is not treated consistently: the discrete InvokeAsync path
hands the selector an oldest-first list, contradicting both the documented "newest first"
contract and the streaming path (Finding 1), and the discrete path invokes the selector
even when no messages were generated while the streaming path guards against it (Finding 2).
The new test only asserts message count, so neither behavior is caught by CI.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas:dotnet/src/Agents/Abstractions/AggregatorChannel.cs
| if (agent.Mode == AggregatorMode.Custom) | ||
| { | ||
| ChatMessageContent? selected = this.SelectMessage(messages!); |
There was a problem hiding this comment.
In Custom mode the discrete path builds messages in the order emitted by_chat.InvokeAsync (line 48), which is generation order — oldest first. TheMessageSelector documentation states the input is "ordered newest first," and the
streaming InvokeStreamingAsync path does provide newest-first (history.Take(...) over the
descending GetChatMessagesAsync). As a result the same selector receives oppositely-ordered
lists across the two paths, and this discrete path contradicts the documented contract — for
example messages => messages[0] selects the oldest message here but the newest when
streaming. Reverse messages (or accumulate it newest-first) before calling SelectMessage
so both invocation paths honor the newest-first contract.
| yield return (IsVisible: true, message); | ||
| } | ||
| if (agent.Mode == AggregatorMode.Custom) |
There was a problem hiding this comment.
The discrete path invokes SelectMessage(messages!) unconditionally for Custom mode, even
when the inner chat produced no messages. The streaming path guards selection withif (history.Count > initialCount), so an empty result never reaches the selector there. An
index-based selector such as the messages => messages[0] used in the tests throwsArgumentOutOfRangeException mid-enumeration when messages is empty, while the streaming
path silently yields nothing for the same input. Guard this call with a non-empty check
(e.g. only invoke SelectMessage when messages!.Count > 0) to match the streaming behavior.
Fixes#10411\n\nSummary:\n- Add AggregatorMode.Custom for selecting which aggregated message is exposed to the parent chat.\n- Add a configurable MessageSelector callback with support for discrete and streaming invocations.\n- Extend AggregatorAgent coverage for custom selection.\n\nTests:\n- git diff --check passed.\n- .NET build could not run because dotnet is not installed in the environment.