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 117e58bdfa..485b76438a 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md +++ b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md @@ -9,65 +9,81 @@ Reviewed files: - `dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_TestPlan.md` - `dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticOrchestrationTests.cs` - `dotnet/src/Microsoft.Agents.AI.Workflows/Specialized/Magentic/MagenticOrchestrator.cs` +- `dotnet/src/Microsoft.Agents.AI.Workflows/Specialized/Magentic/MagenticTaskContext.cs` +- `dotnet/src/Microsoft.Agents.AI.Workflows/MagenticPlanReviewRequest.cs` ## Executive Summary -The current implementation is a comprehensive partial implementation of the original Magentic E2E plan. It contains **14 end-to-end tests** that exercise fully-built workflows created with `MagenticWorkflowBuilder.Build()` and cover the highest-value branches in `MagenticOrchestrator`. +The current implementation contains **14 Magentic end-to-end tests** that build and run real workflows through `MagenticWorkflowBuilder.Build()`. This is a strong partial implementation of the original plan, which estimated roughly 31 tests. -The implemented tests cover: +The implementation now covers the main user-visible orchestration paths: - Initial planning and final-answer completion. -- Explicit no-plan-signoff flow. -- Human plan approval with checkpoint/resume. -- Human plan revision triggering replanning. -- Multi-round delegation through a participant agent. +- Plan signoff disabled path. +- Plan review approval with checkpoint/resume. +- Plan review revision with checkpoint/resume and replanning. +- Multi-round delegation through a participant. - Progress ledger event emission. -- Empty next-speaker fallback. +- Empty next-speaker fallback warning. - Invalid next-speaker warning and forced final answer. - Round-limit termination. -- **Reset-limit termination.** +- Reset-limit termination. - Stall-triggered reset and replan. -- **Stall-triggered replan with plan signoff (human-in-the-loop after stall).** -- **Instruction message sent when present in progress ledger.** -- Replanned-event emission. +- Stall-triggered replan when plan signoff is enabled. +- Instruction-bearing progress ledger path. +- Replanned event emission. -The implementation also includes one production fix: `MagenticOrchestrator.ConfigureProtocol()` now declares `.YieldsOutput>()`, matching the type yielded by `PrepareFinalAnswerAsync`. +The implementation also includes a production fix in `MagenticOrchestrator.ConfigureProtocol()`: the protocol declares `.YieldsOutput>()`, matching the type yielded by `PrepareFinalAnswerAsync()`. -The original plan estimated roughly 31 tests. The remaining gaps are mostly lower-level granularity and edge cases: progress-ledger retry/failure behavior, direct checkpoint state assertions, explicit valid-speaker routing assertions, multiple plan revisions, and empty/single-team edge cases. +The remaining gaps are mostly around deeper state assertions and error/retry behavior: progress-ledger retry/failure handling, direct checkpoint state inspection, direct selected-participant routing assertions, multiple plan revisions, and edge cases such as empty teams and post-termination handling. -## Production Code Review +## Production Code Assessment -### `MagenticOrchestrator.ConfigureProtocol()` output declaration +### Output declaration -Current implementation: +`MagenticOrchestrator.ConfigureProtocol()` now declares: -- The protocol declares `.YieldsOutput>()`. +- `.YieldsOutput>()` Assessment: -- This change is correct and necessary. -- `PrepareFinalAnswerAsync()` yields `List` through `context.YieldOutputAsync(...)`. -- Without this declaration, a fully-built workflow can fail at runtime because the output type is not registered in the protocol. -- This is a production bug fix uncovered by the E2E tests, not test-only cleanup. +- Correct and necessary. +- `PrepareFinalAnswerAsync()` yields `List` via `context.YieldOutputAsync(...)`. +- Without this protocol declaration, fully-built workflows can fail at runtime when emitting the final answer output. +- This is a valid production fix discovered by the E2E tests. + +### Stall replan review request nuance + +The original plan states that `PlanReview_On_Stall_Replan` should send a plan review request with `IsStalled=true`. + +Current code path: + +1. `RunCoordinationRoundAsync()` detects a stall. +2. `ResetAndReplanAsync()` calls `taskContext.Reset()`. +3. `MagenticTaskContext.Reset()` clears `StallCount` to `0`. +4. `UpdatePlanAndDelegateAsync()` submits the new plan review request. +5. `SubmitPlanReviewRequestAsync()` sets `IsStalled` from `taskContext.IsStalled`. + +Because `StallCount` has already been reset, the replanned review request is not expected to have `IsStalled=true` with the current implementation. The current E2E test correctly verifies that a stall-triggered replan with signoff creates a second review request and can resume to completion, but it does **not** satisfy the original plan's exact `IsStalled=true` expectation. ## Implemented Tests | Test | Original Plan Area | What It Verifies | Assessment | |---|---|---|---| -| `Task_Completes_When_RequestSatisfied` | Happy path | First progress ledger says request is satisfied; workflow yields final answer and has no pending review request. | Complete | -| `PlanReview_Approved_Proceeds` | Plan review / checkpoint-resume | Plan review request is emitted, approval response resumes from checkpoint, and workflow completes. | Complete | +| `Task_Completes_When_RequestSatisfied` | Happy path | First coordination round reports satisfied; final answer is yielded and no review request remains pending. | Complete | +| `PlanReview_Approved_Proceeds` | Plan review / checkpoint-resume | Initial plan review pauses, approval resumes from checkpoint, and workflow completes. | Complete | | `Initial_Plan_Emits_PlanCreatedEvent` | Event emission | Initial plan creation emits `MagenticPlanCreatedEvent`. | Complete | -| `NextSpeaker_Invalid_Triggers_FinalAnswer` | Next speaker validation / warnings | Invalid `next_speaker` emits warning and causes final answer generation. | Complete | +| `NextSpeaker_Invalid_Triggers_FinalAnswer` | Next speaker validation / warnings | Invalid `next_speaker` emits a warning and forces final answer generation. | Complete | | `ProgressLedger_Updated_Event_Emitted` | Progress ledger / event emission | Successful ledger update emits `MagenticProgressLedgerUpdatedEvent`. | Complete | -| `PlanSignoff_Disabled_Proceeds_Immediately` | Happy path | `RequirePlanSignoff(false)` skips plan review requests and proceeds directly. | Complete | -| `NextSpeaker_Empty_Falls_Back_To_First` | Next speaker validation / warnings | Empty `next_speaker` emits warning, falls back to first participant, then completes on a later round. | Complete | -| `Task_Completes_After_Multiple_Rounds` | Happy path / delegation loop | A non-satisfied first round delegates to a participant; a later round completes. | Complete | -| `PlanReview_Revised_Triggers_Replan` | Plan review / replanning / checkpoint-resume | Revision response triggers replanning, emits `MagenticReplannedEvent`, requests review again, then completes after approval. | Complete | -| `MaxRoundLimit_Terminates_Workflow` | Limit enforcement | Round limit is checked before the next coordination round and yields the maximum round limit message. | Complete | -| `MaxStallCount_Triggers_Reset` | Stall detection / replanning | A stalled progress ledger reaches `MaxStallCount`, resets, replans, emits `MagenticReplannedEvent`, and then completes. | Complete | -| `Instruction_Message_Sent_When_Present` | Edge cases / delegation | Non-empty `instruction_or_question` in the progress ledger triggers the instruction path; the workflow delegates correctly and completes. | Complete | -| `PlanReview_On_Stall_Replan` | Plan review / stall / checkpoint-resume | Stall-triggered replan with `requirePlanSignoff=true` sends a new plan review request; approval resumes and completes. | Complete | -| `MaxResetLimit_Terminates_Workflow` | Limit enforcement | Stall triggers reset; on the next coordination round, the reset limit is detected and the workflow terminates with "maximum reset count limit". | Complete | +| `PlanSignoff_Disabled_Proceeds_Immediately` | Happy path | `RequirePlanSignoff(false)` skips plan review and proceeds directly to completion. | Complete | +| `NextSpeaker_Empty_Falls_Back_To_First` | Next speaker validation / warnings | Empty next speaker emits a warning, falls back to the first participant, and later completes. | Complete | +| `Task_Completes_After_Multiple_Rounds` | Happy path / delegation loop | One non-satisfied round delegates to a participant; a later round completes. | Complete | +| `PlanReview_Revised_Triggers_Replan` | Plan review / replanning / checkpoint-resume | Human revision triggers replanning, emits `MagenticReplannedEvent`, requests review again, and completes after approval. | Complete | +| `MaxRoundLimit_Terminates_Workflow` | Limit enforcement | Round limit is checked before the next ledger update and yields the maximum round limit message. | Complete | +| `MaxStallCount_Triggers_Reset` | Stall detection / replanning | A stalled ledger reaches `MaxStallCount`, resets, replans, emits `MagenticReplannedEvent`, and completes. | Complete | +| `Instruction_Message_Sent_When_Present` | Edge cases / delegation | A non-empty `instruction_or_question` path executes, delegation proceeds, and the workflow completes over two rounds. | Partial | +| `PlanReview_On_Stall_Replan` | Plan review / stall / checkpoint-resume | Stall-triggered reset and replan with `requirePlanSignoff=true` creates another review request; approval resumes and completes. | Mostly complete; does not assert `IsStalled=true`. | +| `MaxResetLimit_Terminates_Workflow` | Limit enforcement | After a stall-triggered reset, the next coordination round detects the reset limit and yields the maximum reset limit message. | Complete | ## Coverage Against Original Plan @@ -76,9 +92,9 @@ Assessment: | Planned Test | Status | Notes | |---|---|---| | `Task_Completes_When_RequestSatisfied` | Implemented | Covered directly. | -| `Task_Delegates_To_Correct_Agent` | Partially covered | Multi-round tests prove delegation can occur, but no test currently asserts that a specific selected participant receives the turn. | +| `Task_Delegates_To_Correct_Agent` | Partially covered | Existing multi-round tests prove delegation happens, but no test directly asserts that a specific selected participant received the turn. | | `Task_Completes_After_Multiple_Rounds` | Implemented | Covered directly with a participant response and second manager round. | -| `PlanSignoff_Disabled_Proceeds_Immediately` | Implemented | Covered directly and asserts no review request is emitted. | +| `PlanSignoff_Disabled_Proceeds_Immediately` | Implemented | Covered directly; also asserts no plan review request/event is emitted. | Summary: **3 complete, 1 partial**. @@ -87,39 +103,39 @@ Summary: **3 complete, 1 partial**. | Planned Test | Status | Notes | |---|---|---| | `PlanReview_Approved_Proceeds` | Implemented | Covered directly with checkpoint/resume. | -| `PlanReview_Revised_Triggers_Replan` | Implemented | Covered directly with revision, replan event, second review request, approval, and completion. | -| `PlanReview_Multiple_Revisions` | Not implemented | Current revision test covers one revision only. | -| `PlanReview_On_Stall_Replan` | Implemented | Covered directly: stall → reset → replan with plan signoff → new plan review → approval → completion. | +| `PlanReview_Revised_Triggers_Replan` | Implemented | Covered directly with one revision, replan event, second review request, approval, and completion. | +| `PlanReview_Multiple_Revisions` | Not implemented | Current revision coverage stops after one revision before approval. | +| `PlanReview_On_Stall_Replan` | Mostly implemented | Covers stall -> reset -> replan -> second plan review -> approval -> completion. Does not assert `IsStalled=true`, and current production flow resets stall state before submitting that review. | -Summary: **3 complete, 1 remaining**. +Summary: **2 complete, 1 mostly complete, 1 remaining**. ### 3. Limit Enforcement Tests | Planned Test | Status | Notes | |---|---|---| | `MaxRoundLimit_Terminates_Workflow` | Implemented | Covered directly. | -| `MaxResetLimit_Terminates_Workflow` | Implemented | Covered directly: stall → reset → replan → reset limit hit → termination with "maximum reset count limit". | +| `MaxResetLimit_Terminates_Workflow` | Implemented | Covered directly. | | `MaxStallCount_Triggers_Reset` | Implemented | Covered directly as stall-triggered reset/replan. | -Summary: **3 complete — all limit enforcement tests implemented**. +Summary: **3 complete — all planned limit enforcement tests are implemented**. ### 4. Stall Detection Tests | Planned Test | Status | Notes | |---|---|---| -| `Stall_IsInLoop_Increments_StallCount` | Partially covered | `MaxStallCount_Triggers_Reset` uses `IsInLoop=true`, but asserts reset/replan rather than directly asserting the counter. | -| `Stall_NoProgress_Increments_StallCount` | Not implemented | No direct coverage for `IsProgressBeingMade=false`. | -| `Progress_Made_Decrements_StallCount` | Not implemented | No direct coverage for stall count decrement. | -| `Consecutive_Stalls_Trigger_Reset` | Implemented in simplified form | Covered with `MaxStallCount=1`; not covered with multiple consecutive stalls. | +| `Stall_IsInLoop_Increments_StallCount` | Partially covered | `MaxStallCount_Triggers_Reset` uses `IsInLoop=true`; the test verifies reset/replan rather than directly inspecting `StallCount`. | +| `Stall_NoProgress_Increments_StallCount` | Not implemented | No test currently uses `IsProgressBeingMade=false` as the stall trigger. | +| `Progress_Made_Decrements_StallCount` | Not implemented | No direct coverage for decrementing stall count after progress resumes. | +| `Consecutive_Stalls_Trigger_Reset` | Partially covered | Covered only in the simplified `MaxStallCount=1` case, not with multiple consecutive stalls. | -Summary: **1 complete/simplified, 1 partial, 2 remaining**. +Summary: **2 partial, 2 remaining**. ### 5. Progress Ledger Validation Tests | Planned Test | Status | Notes | |---|---|---| -| `ProgressLedger_Retry_On_Parse_Failure` | Not implemented | Retry behavior remains uncovered. | -| `ProgressLedger_Max_Retries_Triggers_Reset` | Not implemented | Failure-after-retries reset path remains uncovered. | +| `ProgressLedger_Retry_On_Parse_Failure` | Not implemented | Retry after invalid JSON remains uncovered. | +| `ProgressLedger_Max_Retries_Triggers_Reset` | Not implemented | Failure after all retry attempts and reset/replan behavior remains uncovered. | | `ProgressLedger_Updated_Event_Emitted` | Implemented | Covered directly. | Summary: **1 complete, 2 remaining**. @@ -130,7 +146,7 @@ Summary: **1 complete, 2 remaining**. |---|---|---| | `NextSpeaker_Empty_Falls_Back_To_First` | Implemented | Covered directly. | | `NextSpeaker_Invalid_Triggers_FinalAnswer` | Implemented | Covered directly. | -| `NextSpeaker_Valid_Delegates_Correctly` | Partially covered | Valid speaker delegation happens in multi-round tests, but the selected executor is not asserted independently. | +| `NextSpeaker_Valid_Delegates_Correctly` | Partially covered | Valid delegation happens in multi-round tests, but the selected executor is not directly asserted. | Summary: **2 complete, 1 partial**. @@ -140,7 +156,7 @@ Summary: **2 complete, 1 partial**. |---|---|---| | `Initial_Plan_Emits_PlanCreatedEvent` | Implemented | Covered directly. | | `Replan_Emits_ReplannedEvent` | Implemented | Covered by plan revision, stall reset, and stall-with-plan-review tests. | -| `Warning_Events_On_Errors` | Partially covered | Empty and invalid next speaker warnings are covered; broader warning paths are not. | +| `Warning_Events_On_Errors` | Partially covered | Empty and invalid next-speaker warnings are covered; progress-ledger failure warnings are not. | Summary: **2 complete, 1 partial**. @@ -148,9 +164,9 @@ Summary: **2 complete, 1 partial**. | Planned Test | Status | Notes | |---|---|---| -| `Checkpoint_Saves_TaskContext` | Not implemented | No direct assertion on persisted `MagenticTaskContext` state. | -| `Checkpoint_Resume_Continues_Correctly` | Partially covered | Plan approval, plan revision, and stall-with-plan-review flows resume successfully from checkpoints. | -| `Checkpoint_Preserves_ProgressLedger` | Not implemented | No direct progress ledger persistence assertion. | +| `Checkpoint_Saves_TaskContext` | Not implemented | No direct assertion on serialized `MagenticTaskContext` state. | +| `Checkpoint_Resume_Continues_Correctly` | Partially covered | Resume is exercised through plan approval, plan revision, and stall-with-plan-review flows. | +| `Checkpoint_Preserves_ProgressLedger` | Not implemented | No direct assertion that progress ledger state is preserved across checkpoint/resume. | Summary: **1 partial, 2 remaining**. @@ -159,43 +175,47 @@ Summary: **1 partial, 2 remaining**. | Planned Test | Status | Notes | |---|---|---| | `Empty_Team_Handling` | Not implemented | No coverage for zero participants. | -| `Single_Agent_Team` | Not implemented | Existing tests commonly use one participant, but there is no dedicated single-agent edge-case assertion. | -| `Instruction_Message_Sent_When_Present` | Implemented | Covered directly: non-empty `instruction_or_question` triggers the instruction path, delegation succeeds, workflow completes. | +| `Single_Agent_Team` | Not implemented | Many tests use one participant, but there is no dedicated edge-case test. | +| `Instruction_Message_Sent_When_Present` | Partially implemented | The test exercises the instruction path and verifies successful two-round completion, but it does not directly observe the sent instruction message or persisted chat history. | | `Terminated_Context_Rejects_New_Messages` | Not implemented | No coverage for post-termination message handling. | -Summary: **1 complete, 3 remaining**. +Summary: **1 partial, 3 remaining**. ## Success Criteria Assessment | Success Criterion | Status | Assessment | |---|---|---| -| All logical forks in `MagenticOrchestrator` are covered by at least one test | Mostly met | The major forks are covered: signoff/no-signoff, approved/revised plan review, satisfied completion, empty/invalid speaker, round limit, reset limit, stall reset, stall-with-plan-review, and instruction sending. Remaining branches: progress ledger retry/failure and post-termination behavior. | -| Tests use the same patterns as `HandoffOrchestrationTests` | Met | Tests use fully-built workflows, `StreamingRun`, `CheckpointManager`, pending requests, workflow events, and collected outputs. | -| Tests run against fully-built workflows | Met | All tests build workflows through `MagenticWorkflowBuilder(...).Build()`. | -| Each test verifies specific event emissions and state changes | Mostly met | Event and output assertions are present. Some planned state transitions, especially internal counters and checkpoint state, are only indirectly covered. | -| Tests cover both `requirePlanSignoff=true` and `false` paths | Met | Plan review tests (including stall-triggered plan review) use `true`; happy path, next speaker, limit, instruction, and stall tests use `false`. | -| Checkpoint/resume functionality is verified | Partially met | Resume is exercised through plan review flows (including stall-triggered plan review), but direct serialized-state assertions are not implemented. | +| All logical forks in `MagenticOrchestrator` are covered by at least one test | Mostly met | Covered: signoff/no-signoff, approved/revised plan review, satisfied completion, empty/invalid speaker, round limit, reset limit, stall reset, stall-with-plan-review, instruction path. Remaining: progress-ledger retry/failure, post-termination behavior, and some direct state transitions. | +| Tests use the same patterns as `HandoffOrchestrationTests` | Met | Tests use fully-built workflows, `StreamingRun`, checkpointing, pending requests, events, and output collection. | +| 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 present, but some tests verify behavior indirectly rather than asserting internal counters, target executor routing, or persisted state. | +| Tests cover both `requirePlanSignoff=true` and `false` paths | Met | Plan review tests use `true`; happy path, next speaker, limit, stall, and instruction tests use `false`. | +| Checkpoint/resume functionality is verified | Partially met | Resume is exercised through human plan review flows, but direct checkpoint state preservation remains untested. | ## Key Observations -- The tests are true E2E tests for the workflow builder path rather than isolated protocol/unit tests. -- `TestReplayAgent` response ordering is important because each re-entry into `TakeTurnAsync` can trigger another facts update and plan update before the next coordination round. -- The current implementation provides strong coverage for high-risk orchestration behavior, especially human-in-the-loop and replanning flows. -- All three limit enforcement branches (round, reset, and stall) are now fully covered. -- The stall-with-plan-review path is now covered, exercising the checkpoint/resume flow during stall recovery. -- Remaining gaps are mostly about deeper verification (internal counters, checkpoint state) and progress ledger failure handling. +- The tests are genuine E2E tests for the workflow builder path, not isolated unit tests of orchestrator internals. +- `TestReplayAgent` response ordering is central to these tests because each re-entry to `TakeTurnAsync()` may consume another facts response and plan response before the next ledger response. +- The implementation now fully covers the planned limit enforcement category: round, reset, and stall limits. +- The current `PlanReview_On_Stall_Replan` test covers the user-visible stall-with-signoff flow, but the original plan's `IsStalled=true` expectation is not currently satisfied by production code after `Reset()` clears `StallCount`. +- The instruction test exercises the instruction branch but does not directly prove the exact instruction `ChatMessage` was delivered to a participant. +- Remaining gaps are concentrated in progress-ledger failure paths, direct checkpoint state inspection, and direct participant-routing assertions. ## Recommended Next Tests Continue one test at a time in this order: -1. `ProgressLedger_Retry_On_Parse_Failure` — requires a `TestReplayAgent` that returns invalid JSON on first attempt, valid on retry -2. `ProgressLedger_Max_Retries_Triggers_Reset` — requires a `TestReplayAgent` that returns invalid JSON for all attempts -3. `Task_Delegates_To_Correct_Agent` / `NextSpeaker_Valid_Delegates_Correctly` — assert the specific participant agent receives the turn -4. Direct checkpoint state preservation tests -5. `PlanReview_Multiple_Revisions` — multiple human revision rounds before final approval -6. Edge cases: empty team, dedicated single-agent team, post-termination rejection +1. `ProgressLedger_Retry_On_Parse_Failure` — return invalid ledger JSON once, then valid JSON, and assert retry warning plus successful completion. +2. `ProgressLedger_Max_Retries_Triggers_Reset` — return invalid ledger JSON for all attempts and assert warning/reset/replan behavior. +3. `Task_Delegates_To_Correct_Agent` / `NextSpeaker_Valid_Delegates_Correctly` — use multiple participants and assert only the selected participant receives the turn. +4. `PlanReview_Multiple_Revisions` — exercise multiple human revision responses before final approval. +5. Direct checkpoint state tests — inspect persisted `MagenticTaskContext` and progress ledger state where possible. +6. Dedicated edge cases — empty team behavior, single-agent team behavior, and post-termination rejection. +7. Strengthen `Instruction_Message_Sent_When_Present` if a stable event or test hook is available to directly observe the sent instruction message. +8. Clarify or fix the `PlanReview_On_Stall_Replan` `IsStalled` expectation: either adjust production behavior to preserve stall context in the review request or update the original plan expectation. ## Overall Conclusion -The current implementation covers 14 of the ~31 originally planned tests and should be considered a correct and valuable implementation covering all major orchestrator decision paths. It validates the previously missing output declaration in production code, covers all three limit enforcement branches, and exercises the most complex orchestrator flows including stall-triggered replanning with plan signoff. The remaining work is well-scoped and can continue incrementally, with priority given to progress-ledger failure/retry handling. +The current implementation is a valuable and correct partial completion of the original Magentic E2E plan. It covers **14 tests** and most major user-visible orchestration decisions, including all planned limit enforcement scenarios and the important human-in-the-loop flows. + +The next highest-value work is progress-ledger failure/retry coverage, followed by direct routing and checkpoint-state assertions. Two areas deserve special attention before calling the plan complete: the current inability to assert `IsStalled=true` on stall-triggered plan review requests after reset, and the indirect nature of the instruction-message test.