Rewrite Magentic E2E implementation review

Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/3d15763b-3a68-488e-9412-3fa280e083c0

Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com>

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<List<ChatMessage>>()`, 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.