diff --git a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md index 60fa86dfb6..dc7f37f200 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md +++ b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md @@ -2,7 +2,8 @@ ## Review Scope -This document reviews the current Magentic E2E implementation against the original plan in `MagenticE2E_TestPlan.md`. +This document reviews the current Magentic E2E implementation against the original plan in +`MagenticE2E_TestPlan.md`. Reviewed files: @@ -15,44 +16,262 @@ Reviewed files: ## Executive Summary -The current implementation contains **22 Magentic end-to-end tests** in `MagenticOrchestrationTests.cs`. Each test builds and executes a real workflow through `MagenticWorkflowBuilder.Build()`, exercising the workflow protocol, checkpoint/resume path, orchestrator, manager, event stream, and output path. +The current implementation contains **21 Magentic end-to-end tests** in +`MagenticOrchestrationTests.cs`. The tests build real workflows through +`MagenticWorkflowBuilder.Build()` and exercise the orchestrator, manager calls, event stream, +checkpoint/resume flow, plan-review requests, participant delegation, and yielded final outputs. -### Production Code Fixes Applied +The current change set is substantially aligned with the original plan and also includes production +fixes discovered while implementing the tests. The highest-value orchestration paths are covered: -Two behavioral divergences from the Python implementation were identified and fixed: +- Initial planning, progress-ledger creation, and final-answer generation. +- Plan signoff disabled flow. +- Plan review approval, single revision, multiple revisions, and stall-triggered review. +- Multi-round coordination without replanning on normal agent return. +- Round, reset, and stall limit enforcement. +- Stall detection from both `IsInLoop=true` and `IsProgressBeingMade=false`. +- Progress-ledger parse retry and max-retry reset behavior. +- Empty and invalid next-speaker handling. +- Direct routing assertion that the selected participant responds and the non-selected participant does not. +- Event emission for plan creation, replanning, progress-ledger updates, and warnings. -1. **Replan-on-every-turn bug**: `TakeTurnAsync` previously called `UpdatePlanAndDelegateAsync` on every turn, causing a full replan (facts + plan LLM calls) every time an agent returned control. Python's `_handle_response` goes directly to `_run_inner_loop` (progress ledger only). **Fixed**: `TakeTurnAsync` now only plans on the first turn; subsequent turns go directly to `RunCoordinationRoundAsync`. +The remaining gaps are mostly direct state-inspection and edge-case tests: serialized checkpoint +state, progress-ledger state preservation across resume, zero-participant behavior, post-termination +message rejection, and stronger direct assertion of instruction message delivery. -2. **StallCount threshold mismatch**: C# used `>=` (`StallCount >= MaxStallCount`) while Python uses `>` (`stall_count > max_stall_count`). **Fixed**: Changed to `>` to match Python semantics. `MaxStallCount` now means "the number of stalls tolerated before triggering a reset". +## Production Code Review -3. **IsStalled preservation on stall-triggered plan reviews**: `ResetAndReplanAsync` captured `IsStalled` before `Reset()` clears the stall count, threading it through `UpdatePlanAndDelegateAsync` → `SubmitPlanReviewRequestAsync` via a `replanAfterStall` parameter. This was fixed in a prior commit. +### Output protocol declaration -## Implemented E2E Tests +`MagenticOrchestrator.ConfigureProtocol()` now declares `.YieldsOutput>()`, which +matches the final output emitted by `PrepareFinalAnswerAsync()`. -| Test | Plan Area | What It Verifies | Status | -|---|---|---|---| -| `Task_Completes_When_RequestSatisfied` | Happy path | First coordination round reports satisfied and final answer is yielded. | Complete | -| `PlanReview_Approved_Proceeds` | Plan review / checkpoint-resume | Initial plan review pauses; approval resumes and completes. | Complete | -| `Initial_Plan_Emits_PlanCreatedEvent` | Event emission | Initial plan creation emits `MagenticPlanCreatedEvent`. | Complete | -| `NextSpeaker_Invalid_Triggers_FinalAnswer` | Next speaker validation | Invalid next speaker emits warning and forces final answer. | Complete | -| `ProgressLedger_Updated_Event_Emitted` | Progress ledger / events | Valid ledger update emits `MagenticProgressLedgerUpdatedEvent`. | Complete | -| `PlanSignoff_Disabled_Proceeds_Immediately` | Happy path | `RequirePlanSignoff(false)` skips plan review. | Complete | -| `NextSpeaker_Empty_Falls_Back_To_First` | Next speaker validation | Empty next speaker emits warning and falls back to first participant. | Complete | -| `Task_Completes_After_Multiple_Rounds` | Happy path / delegation loop | Non-satisfied round delegates; later round completes without replan. | Complete | -| `PlanReview_Revised_Triggers_Replan` | Plan review | One human revision triggers replan and second review. | Complete | -| `MaxRoundLimit_Terminates_Workflow` | Limits | Round limit yields maximum round count message. | Complete | -| `MaxStallCount_Triggers_Reset` | Stall detection / limits | `IsInLoop=true` reaches stall limit and triggers reset/replan. | Complete | -| `Instruction_Message_Sent_When_Present` | Edge case / delegation | Non-empty instruction path executes and completes over two rounds. | Complete | -| `PlanReview_On_Stall_Replan` | Plan review / stall | Stall with signoff creates a second review request with `IsStalled=true` and completes after approval. | Complete | -| `MaxResetLimit_Terminates_Workflow` | Limits | Reset limit yields maximum reset count message. | Complete | -| `ProgressLedger_Retry_On_Parse_Failure` | Progress ledger validation | Invalid ledger JSON warns, retry succeeds, workflow completes. | Complete | -| `ProgressLedger_Max_Retries_Triggers_Reset` | Progress ledger validation | All retry attempts fail, reset/replan occurs, workflow completes. | Complete | -| `Stall_NoProgress_Increments_StallCount` | Stall detection | `IsProgressBeingMade=false` triggers stall reset/replan. | Complete | -| `PlanReview_Multiple_Revisions` | Plan review | Two revisions occur before final approval and completion. | Complete | -| `Task_Delegates_To_Correct_Agent` | Happy path / routing | Two participants; selected speaker responds while non-selected does not. | Complete | -| `Progress_Made_Decrements_StallCount` | Stall detection | Stall count increments then decrements on progress; reset is avoided. | Complete | -| `Consecutive_Stalls_Trigger_Reset` | Stall detection | Two consecutive stalls reach threshold and trigger reset/replan. | Complete | +Assessment: + +- Correct and necessary for fully built workflow execution. +- Covered indirectly by every E2E test that completes with a final answer. + +### Normal agent return no longer replans + +`MagenticOrchestrator.TakeTurnAsync()` now distinguishes first turn from subsequent turns: + +- First turn initializes `MagenticTaskContext` and calls `UpdatePlanAndDelegateAsync()`. +- Subsequent turns go directly to `RunCoordinationRoundAsync()`. + +Assessment: + +- Correctly aligns .NET behavior with the Python Magentic loop, where agent responses resume the + inner loop and only request a new progress ledger. +- Prevents extra facts/plan manager calls on every participant response. +- Covered by multi-round tests that now assert one initial plan and no `MagenticReplannedEvent` on + normal participant return. + +### Stall threshold now matches Python + +`MagenticTaskContext.IsStalled` now uses: + +```csharp +this.TaskCounters.StallCount > this.TaskLimits.MaxStallCount +``` + +Assessment: + +- Correctly matches Python's `stall_count > max_stall_count` behavior. +- `MaxStallCount` should now be read as "number of stalls tolerated before reset". +- Test setups that need reset on the first stalled ledger use `WithMaxStalls(0)`. +- The original test plan text still describes `StallCount >= MaxStallCount`; that part of the plan + is stale relative to the chosen cross-language behavior. + +### Stall-triggered plan reviews preserve the stalled flag + +`ResetAndReplanAsync()` captures whether the task was stalled before clearing counters, then passes +that value through `UpdatePlanAndDelegateAsync()` and `SubmitPlanReviewRequestAsync()` as +`replanAfterStall`. + +Assessment: + +- Correctly satisfies the original plan expectation that `PlanReview_On_Stall_Replan` receives a + request with `IsStalled=true`. +- Covered by `PlanReview_On_Stall_Replan`, which asserts the initial review is not stalled and the + replanned review is stalled. + +### Progress-ledger retry and reset behavior + +`MagenticManager.UpdateProgressLedgerAsync()` retries invalid progress-ledger responses and emits a +`WorkflowWarningEvent` for parse/update failures. If retries are exhausted, the orchestrator catches +the exception and resets/replans. + +Assessment: + +- Covered by `ProgressLedger_Retry_On_Parse_Failure` and + `ProgressLedger_Max_Retries_Triggers_Reset`. +- The max-retry reset path also verifies `MagenticReplannedEvent` emission after reset. + +## Implemented Tests + +| Test | Original Plan Area | Assessment | +|---|---|---| +| `Task_Completes_When_RequestSatisfied` | Happy path | Complete. Verifies immediate satisfaction and final output. | +| `PlanReview_Approved_Proceeds` | Plan review / checkpoint-resume | Complete. Pauses for review, resumes with approval, completes. | +| `Initial_Plan_Emits_PlanCreatedEvent` | Event emission | Complete. Verifies initial plan event. | +| `NextSpeaker_Invalid_Triggers_FinalAnswer` | Next speaker validation | Complete. Invalid participant warning and final answer path. | +| `ProgressLedger_Updated_Event_Emitted` | Progress ledger / events | Complete. Verifies progress-ledger event and state. | +| `PlanSignoff_Disabled_Proceeds_Immediately` | Happy path | Complete. No pending review request when signoff is disabled. | +| `NextSpeaker_Empty_Falls_Back_To_First` | Next speaker validation | Mostly complete. Verifies warning and successful fallback, but the response setup still includes stale extra facts/plan responses from the old replan-on-return behavior. | +| `Task_Completes_After_Multiple_Rounds` | Happy path / delegation loop | Complete. Verifies multi-round completion without normal-return replan. | +| `PlanReview_Revised_Triggers_Replan` | Plan review | Complete. One revision triggers replan and second review. | +| `MaxRoundLimit_Terminates_Workflow` | Limits | Complete. Round limit yields termination message. | +| `MaxStallCount_Triggers_Reset` | Limits / stall detection | Complete. Uses `WithMaxStalls(0)` to reset on first stalled ledger under `>` semantics. | +| `Instruction_Message_Sent_When_Present` | Edge case / delegation | Partial. Verifies the instruction-bearing flow completes over two rounds, but does not directly assert the exact instruction message delivered to the participant. | +| `PlanReview_On_Stall_Replan` | Plan review / stall | Complete. Verifies stall-triggered review request has `IsStalled=true`. | +| `MaxResetLimit_Terminates_Workflow` | Limits | Complete. Reset limit yields termination message after one reset. | +| `ProgressLedger_Retry_On_Parse_Failure` | Progress ledger validation | Complete. Invalid ledger warns, retry succeeds, workflow completes. | +| `ProgressLedger_Max_Retries_Triggers_Reset` | Progress ledger validation | Complete. Exhausted retries warn, reset/replan occurs, workflow completes. | +| `Stall_NoProgress_Increments_StallCount` | Stall detection | Complete. No-progress ledger triggers reset/replan. | +| `Task_Delegates_To_Correct_Agent` | Happy path / routing | Complete. Directly asserts selected worker responds and non-selected worker does not. | +| `Progress_Made_Decrements_StallCount` | Stall detection | Complete. One stalled round followed by progress avoids reset and completes. | +| `Consecutive_Stalls_Trigger_Reset` | Stall detection | Complete. Uses `WithMaxStalls(1)` so the second consecutive stall triggers reset under `>` semantics. | +| `PlanReview_Multiple_Revisions` | Plan review | Complete. Two revisions before final approval and completion. | + +## Coverage Against Original Plan + +### 1. Happy Path Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `Task_Completes_When_RequestSatisfied` | Complete | Covered directly. | +| `Task_Delegates_To_Correct_Agent` | Complete | Direct selected/non-selected participant assertions were added. | +| `Task_Completes_After_Multiple_Rounds` | Complete | Covered with the corrected no-replan-on-return behavior. | +| `PlanSignoff_Disabled_Proceeds_Immediately` | Complete | Covered directly. | + +Summary: **4 complete / 4 planned**. + +### 2. Plan Review Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `PlanReview_Approved_Proceeds` | Complete | Covered with checkpoint/resume. | +| `PlanReview_Revised_Triggers_Replan` | Complete | Covered with one revision. | +| `PlanReview_Multiple_Revisions` | Complete | Covered with two revisions. | +| `PlanReview_On_Stall_Replan` | Complete | Covered, including `IsStalled=true` on the replanned request. | + +Summary: **4 complete / 4 planned**. + +### 3. Limit Enforcement Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `MaxRoundLimit_Terminates_Workflow` | Complete | Covered directly. | +| `MaxResetLimit_Terminates_Workflow` | Complete | Covered directly. | +| `MaxStallCount_Triggers_Reset` | Complete | Covered with the updated `>` stall threshold. | + +Summary: **3 complete / 3 planned**. + +### 4. Stall Detection Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `Stall_IsInLoop_Increments_StallCount` | Behaviorally covered | Covered through reset behavior when `IsInLoop=true`; the internal counter is not directly inspected. | +| `Stall_NoProgress_Increments_StallCount` | Behaviorally covered | Covered through reset behavior when `IsProgressBeingMade=false`; the internal counter is not directly inspected. | +| `Progress_Made_Decrements_StallCount` | Complete | Covered by avoiding reset after a later progress-making round. | +| `Consecutive_Stalls_Trigger_Reset` | Complete | Covered with a two-stall threshold under `>` semantics. | + +Summary: **2 complete, 2 behaviorally covered / 4 planned**. + +### 5. Progress Ledger Validation Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `ProgressLedger_Retry_On_Parse_Failure` | Complete | Covered directly. | +| `ProgressLedger_Max_Retries_Triggers_Reset` | Complete | Covered directly. | +| `ProgressLedger_Updated_Event_Emitted` | Complete | Covered directly. | + +Summary: **3 complete / 3 planned**. + +### 6. Next Speaker Validation Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `NextSpeaker_Empty_Falls_Back_To_First` | Mostly complete | Warning and fallback behavior are covered; test setup should remove stale extra facts/plan responses. | +| `NextSpeaker_Invalid_Triggers_FinalAnswer` | Complete | Covered directly. | +| `NextSpeaker_Valid_Delegates_Correctly` | Complete | Covered by `Task_Delegates_To_Correct_Agent`. | + +Summary: **2 complete, 1 mostly complete / 3 planned**. + +### 7. Event Emission Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `Initial_Plan_Emits_PlanCreatedEvent` | Complete | Covered directly. | +| `Replan_Emits_ReplannedEvent` | Complete | Covered by human revision, multiple revisions, stall reset, no-progress reset, and max-retry reset. | +| `Warning_Events_On_Errors` | Mostly complete | Warnings are covered across next-speaker and progress-ledger tests, but there is no single dedicated warning matrix test. | + +Summary: **2 complete, 1 mostly complete / 3 planned**. + +### 8. Checkpoint/Resume Tests + +| Planned Test | Current Status | Notes | +|---|---|---| +| `Checkpoint_Saves_TaskContext` | Not implemented | No direct assertion on serialized `MagenticTaskContext`. | +| `Checkpoint_Resume_Continues_Correctly` | Behaviorally covered | Resume is exercised by approval, revision, multiple revision, and stall-with-signoff scenarios. | +| `Checkpoint_Preserves_ProgressLedger` | Not implemented | No direct assertion that progress-ledger state survives checkpoint/restore. | + +Summary: **1 behaviorally covered, 2 not implemented / 3 planned**. + +### 9. Edge Cases + +| Planned Test | Current Status | Notes | +|---|---|---| +| `Empty_Team_Handling` | Not implemented | No zero-participant test. | +| `Single_Agent_Team` | Behaviorally covered | Most tests use a single participant, but there is no dedicated edge-case test. | +| `Instruction_Message_Sent_When_Present` | Partial | Flow is covered, but exact participant-delivered instruction is not directly asserted. | +| `Terminated_Context_Rejects_New_Messages` | Not implemented | No post-termination input test. | + +Summary: **1 behaviorally covered, 1 partial, 2 not implemented / 4 planned**. + +## Success Criteria Assessment + +| Success Criterion | Assessment | +|---|---| +| All logical forks in `MagenticOrchestrator` are covered by at least one test | **Substantially met.** Major user-visible branches are covered. Remaining uncovered areas are direct post-termination rejection and zero-participant behavior. | +| Tests use the same patterns as `HandoffOrchestrationTests` | **Met.** Tests use fully built workflows, streaming execution, checkpoint managers, pending requests, event collection, and output assertions. | +| Tests run against fully-built workflows | **Met.** Every test builds through `MagenticWorkflowBuilder(...).Build()`. | +| Each test verifies specific event emissions and state changes | **Mostly met.** Event and output assertions are strong. Internal counters and serialized checkpoint state are mostly inferred through behavior rather than inspected directly. | +| Tests cover both `requirePlanSignoff=true` and `false` paths | **Met.** Signoff and no-signoff flows are both exercised. | +| Checkpoint/resume functionality is verified | **Partially met.** Resume is exercised through plan-review workflows, but direct checkpoint payload validation is still missing. | + +## Review Findings and Recommended Follow-Up + +1. **Fix stale setup in `NextSpeaker_Empty_Falls_Back_To_First`.** The test still supplies + `factsResponse2` and `planResponse2` after the first progress ledger. Since normal agent return + no longer replans, those responses are consumed as invalid progress-ledger retry attempts before + the valid satisfied ledger. The test should be simplified to match the updated control flow: + initial facts, initial plan, empty-speaker ledger, satisfied ledger, final answer. + +2. **Clean up stale comments that describe the old stall threshold.** A few comments still describe + `MaxStallCount=1` or "reaches threshold" in old `>=` terms. The code and test setup now use + `>` semantics correctly, but comments should consistently say `StallCount > MaxStallCount`. + +3. **Update the original test plan if it remains a living document.** The plan currently states + `IsStalled (StallCount >= MaxStallCount)`. The implemented and cross-language-aligned behavior is + now `StallCount > MaxStallCount`. + +4. **Add direct checkpoint-state assertions if stable infrastructure exists.** Current tests prove + resume behavior works but do not inspect serialized `MagenticTaskContext` or progress-ledger + state. + +5. **Strengthen instruction-delivery verification.** `Instruction_Message_Sent_When_Present` + currently proves the flow completes with an instruction present. A stronger test would directly + observe the participant receiving the instruction message. + +6. **Consider dedicated edge-case tests.** The remaining planned gaps are zero participants, + explicit single-agent edge-case behavior, and rejected input after termination. ## Overall Conclusion -The Magentic E2E implementation covers **22 tests** with full coverage of limit enforcement, progress ledger validation, plan review, stall detection, and routing. Three production code fixes were made to align .NET behavior with the Python implementation. +The current Magentic E2E suite is a strong implementation of the original plan. It covers +**21 fully built workflow tests** and includes important production fixes that align .NET with the +Python Magentic orchestration model. The most important remaining work is cleanup and precision: +remove stale response setup from one fallback test, align comments and the original plan text with +the `>` stall threshold, and add direct state/edge-case tests if those behaviors need stronger +contract coverage.