Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/b2c60ce7-4d05-4a0d-b05d-d4284f5b7bb3 Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com>
8.3 KiB
Magentic E2E Implementation Review
Review Scope
This review compares the current implementation against the original plan in MagenticE2E_TestPlan.md.
Reviewed files:
dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_TestPlan.mddotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticOrchestrationTests.csdotnet/src/Microsoft.Agents.AI.Workflows/Specialized/Magentic/MagenticOrchestrator.cs
Summary
The current changes implement an initial subset of the Magentic E2E test plan and also fix one production bug uncovered by the first E2E test. The implemented tests run against fully-built MagenticWorkflowBuilder.Build() workflows and cover a small number of important branches: basic completion, plan approval, initial plan event emission, progress ledger event emission, and invalid next speaker handling.
The implementation does not yet satisfy the full original plan. The plan estimated roughly 31 tests across happy path, plan review, limits, stall detection, progress ledger validation, next speaker validation, event emission, checkpoint/resume, and edge cases. The current implementation contains 5 E2E tests.
Production Code Change
MagenticOrchestrator output declaration
Implemented:
MagenticOrchestrator.ConfigureProtocol()now declares.YieldsOutput<List<ChatMessage>>().
Assessment:
- This is a valid fix.
MagenticOrchestrator.PrepareFinalAnswerAsync()yieldsList<ChatMessage>viacontext.YieldOutputAsync(...), but the protocol previously did not declare that output type. - Without this declaration, a fully-built workflow failed at runtime with an error like:
Cannot output object of type List'1. Expecting one of []. - This fix aligns the protocol declaration with existing orchestrator behavior.
Implemented Test Coverage
| Original Plan Area | Planned Test | Implemented Test | Status |
|---|---|---|---|
| Happy Path | Task_Completes_When_RequestSatisfied |
Task_Completes_When_RequestSatisfied |
Complete |
| Plan Review | PlanReview_Approved_Proceeds |
PlanReview_Approved_Proceeds |
Complete |
| Event Emission | Initial_Plan_Emits_PlanCreatedEvent |
Initial_Plan_Emits_PlanCreatedEvent |
Complete |
| Progress Ledger Validation / Events | ProgressLedger_Updated_Event_Emitted |
ProgressLedger_Updated_Event_Emitted |
Complete |
| Next Speaker Validation | NextSpeaker_Invalid_Triggers_FinalAnswer |
NextSpeaker_Invalid_Triggers_FinalAnswer |
Complete |
Original Plan Coverage Status
1. Happy Path Tests
Implemented:
Task_Completes_When_RequestSatisfied
Not yet implemented:
Task_Delegates_To_Correct_AgentTask_Completes_After_Multiple_RoundsPlanSignoff_Disabled_Proceeds_Immediately
Assessment:
- Basic completion is covered.
- Delegation, multi-round execution, and explicit no-signoff behavior remain uncovered.
2. Plan Review Tests
Implemented:
PlanReview_Approved_Proceeds
Not yet implemented:
PlanReview_Revised_Triggers_ReplanPlanReview_Multiple_RevisionsPlanReview_On_Stall_Replan
Assessment:
- The approval flow is covered and uses checkpoint/resume mechanics.
- Revision and stall-triggered replanning paths remain uncovered.
3. Limit Enforcement Tests
Implemented:
- None
Not yet implemented:
MaxRoundLimit_Terminates_WorkflowMaxResetLimit_Terminates_WorkflowMaxStallCount_Triggers_Reset
Assessment:
- Limit handling remains uncovered.
- An earlier attempt at round-limit coverage exposed that constructing a deterministic full workflow path for repeated delegation needs careful response sequencing and likely should be added separately.
4. Stall Detection Tests
Implemented:
- None
Not yet implemented:
Stall_IsInLoop_Increments_StallCountStall_NoProgress_Increments_StallCountProgress_Made_Decrements_StallCountConsecutive_Stalls_Trigger_Reset
Assessment:
- Stall counting and reset behavior remain uncovered.
5. Progress Ledger Validation Tests
Implemented:
ProgressLedger_Updated_Event_Emitted
Not yet implemented:
ProgressLedger_Retry_On_Parse_FailureProgressLedger_Max_Retries_Triggers_Reset
Assessment:
- Successful ledger update event emission is covered.
- Retry and failure paths remain uncovered.
6. Next Speaker Validation Tests
Implemented:
NextSpeaker_Invalid_Triggers_FinalAnswer
Not yet implemented:
NextSpeaker_Empty_Falls_Back_To_FirstNextSpeaker_Valid_Delegates_Correctly
Assessment:
- Invalid next speaker handling is covered.
- Empty fallback and valid delegation remain uncovered.
7. Event Emission Tests
Implemented:
Initial_Plan_Emits_PlanCreatedEventProgressLedger_Updated_Event_Emitted
Not yet implemented:
Replan_Emits_ReplannedEvent- Broader
Warning_Events_On_Errors
Assessment:
- Initial plan and progress ledger events are covered.
- Replan and warning event coverage is still partial.
8. Checkpoint/Resume Tests
Implemented:
- Partial coverage through
PlanReview_Approved_Proceeds, which resumes from a checkpoint after a pending plan review request.
Not yet implemented:
Checkpoint_Saves_TaskContextCheckpoint_Resume_Continues_CorrectlyCheckpoint_Preserves_ProgressLedger
Assessment:
- Checkpoint/resume infrastructure is exercised indirectly.
- Direct state persistence assertions for
MagenticTaskContextandMagenticProgressLedgerare not yet implemented.
9. Edge Cases
Implemented:
- None
Not yet implemented:
Empty_Team_HandlingSingle_Agent_TeamInstruction_Message_Sent_When_PresentTerminated_Context_Rejects_New_Messages
Assessment:
- Edge cases remain uncovered.
Success Criteria Review
| Success Criterion | Status | Notes |
|---|---|---|
All logical forks in MagenticOrchestrator are covered by at least one test |
Not met | Only 5 branches/behaviors are covered so far. Limits, stalls, replanning, empty/valid next speaker, retry failures, and edge cases remain. |
Tests use the same patterns as HandoffOrchestrationTests |
Partially met | Tests use StreamingRun, CheckpointManager, RequestInfoEvent, WorkflowOutputEvent, and helper-based workflow execution patterns. |
| Tests run against fully-built workflows, not isolated components | Met for implemented tests | All implemented tests use MagenticWorkflowBuilder(...).Build(). |
| Each test verifies specific event emissions and state changes | Partially met | Event tests verify emitted events; completion and plan review verify outputs/requests. Some state changes are not directly asserted. |
Tests cover both requirePlanSignoff=true and false paths |
Met for initial subset | Task_Completes_When_RequestSatisfied uses false; PlanReview_Approved_Proceeds uses true. |
| Checkpoint/resume functionality is verified | Partially met | Plan approval flow resumes from checkpoint, but task context/progress ledger persistence is not directly asserted. |
Validation Results Observed
The implemented tests were built and run during implementation:
dotnet build tests/Microsoft.Agents.AI.Workflows.UnitTests/Microsoft.Agents.AI.Workflows.UnitTests.csproj -f net10.0dotnet test --project tests/Microsoft.Agents.AI.Workflows.UnitTests/Microsoft.Agents.AI.Workflows.UnitTests.csproj -f net10.0 --filter-method '*MagenticOrchestrationTests*' --no-build- Full workflow unit test run reported: 487 total, 465 succeeded, 22 skipped.
Parallel validation also passed. Code review reported one unrelated comment in a Python test file outside the changed Magentic files. CodeQL reported no alerts, though C# and Python analysis were skipped because database size was too large.
Recommended Next Steps
Continue implementation one test case at a time, prioritizing uncovered orchestrator branches in this order:
PlanSignoff_Disabled_Proceeds_ImmediatelyNextSpeaker_Empty_Falls_Back_To_FirstNextSpeaker_Valid_Delegates_CorrectlyPlanReview_Revised_Triggers_ReplanMaxRoundLimit_Terminates_WorkflowMaxStallCount_Triggers_ResetProgressLedger_Retry_On_Parse_Failure- Direct checkpoint state preservation tests
Overall Assessment
The current implementation is a correct and useful start, and it already caught and fixed a real production protocol declaration bug. However, it should be considered a partial implementation of the original Magentic E2E plan rather than completion of the full plan.