-
Notifications
You must be signed in to change notification settings - Fork 680
.NET: fix: WorkflowAsAgent Sample #1787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
6b9d132 to
e429504
Compare
There was a problem hiding this 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 refactors the ChatForwardingExecutor from an internal specialized component to a public reusable API, and updates the WorkflowAsAnAgent sample to use it instead of a custom implementation.
Key changes:
- Made
ChatForwardingExecutorpublic with comprehensive XML documentation - Moved it from
Microsoft.Agents.AI.Workflows.SpecializedtoMicrosoft.Agents.AI.Workflowsnamespace - Replaced custom
ConcurrentStartExecutorin sample withChatForwardingExecutor - Added
IResettableExecutorimplementation toConcurrentAggregationExecutorfor proper cleanup
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Workflows/ChatForwardingExecutor.cs | Promoted from internal to public, added comprehensive documentation, and marked ConfigureRoutes as sealed override |
| dotnet/samples/GettingStarted/Workflows/Agents/WorkflowAsAnAgent/WorkflowFactory.cs | Replaced custom ConcurrentStartExecutor with ChatForwardingExecutor and added reset capability to ConcurrentAggregationExecutor |
Comments suppressed due to low confidence (2)
dotnet/src/Microsoft.Agents.AI.Workflows/ChatForwardingExecutor.cs:20
- The
sealedmodifier on an override method is redundant since the containing class is alreadysealed. Removesealedfrom the method declaration.
dotnet/src/Microsoft.Agents.AI.Workflows/ChatForwardingExecutor.cs:12 - The newly public
ChatForwardingExecutorclass lacks unit test coverage. Consider adding tests to verify message forwarding behavior for all supported message types (string, ChatMessage, List, TurnToken).
dotnet/samples/GettingStarted/Workflows/Agents/WorkflowAsAnAgent/WorkflowFactory.cs
Show resolved
Hide resolved
e429504 to
a2ed9d9
Compare
b45909b to
52612b8
Compare
52612b8 to
184d3ea
Compare
|
Added one more fix to update for a change in how message aggregation happens in |
184d3ea to
f5a28fb
Compare
|
Pending #1612 |
f5a28fb to
b7906b0
Compare
* Also makes ChatForwardingExecutor public
Make ChatForwardingExecutor match the input types of ChatProtocolExecutor.
AIAgent always sends out List<ChatMessage> now.
b7906b0 to
d62975a
Compare
When we started introducing the different execution modes to solidify the concurrency model, we broke the WorkflowAsAgent Sample, which was not updated for either of
IResettableExecutoror Cross-Run Shareable.The fix is to make public and switch to using the
ChatForwardingExecutorin place of a self-definedConcurrentStartExecutor, and make theConcurrentAggregationExecutorimplementIResettableExecutorDescription
Contribution Checklist
Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.