diff --git a/PR_DESCRIPTION.md b/PR_DESCRIPTION.md deleted file mode 100644 index 09701121e0..0000000000 --- a/PR_DESCRIPTION.md +++ /dev/null @@ -1,39 +0,0 @@ -### Motivation and Context - -This PR completes the Magentic end-to-end workflow coverage described in `MagenticE2E_TestPlan.md` and summarized in `MagenticE2E_ImplementationReview.md`. - -The Magentic orchestrator has several user-visible branches that are difficult to validate with isolated unit tests: planning and replanning, human plan review, participant routing, stall/reset handling, progress-ledger retry behavior, final-answer termination, checkpoint/resume, and invalid or erroneous workflow inputs. This change adds fully built workflow tests that exercise those behaviors through `MagenticWorkflowBuilder.Build()` and the in-process streaming workflow runtime. - -It also fixes production behavior discovered while adding the E2E tests: - -- Empty Magentic teams now fail during workflow build instead of producing a workflow that can fail later during execution. -- New top-level messages sent after Magentic termination now surface the orchestrator's terminal-state error as a workflow error, even though the workflow framework accepts queued messages. - -### Description - -This PR adds a Magentic E2E test suite and supporting implementation updates. - -Key changes: - -- Added `MagenticOrchestrationTests.cs` with **23 end-to-end tests** that run fully built Magentic workflows through the streaming workflow runtime. -- Covered happy-path completion, participant delegation, multi-round coordination, plan signoff, plan approval/revision flows, multiple revisions, stall-triggered replanning, checkpoint/resume behavior, round/reset limits, progress-ledger retry/reset handling, next-speaker validation, warning/event emission, instruction-message flow, empty-team validation, and post-termination message rejection. -- Added `MagenticE2E_TestPlan.md` documenting the intended E2E coverage across the Magentic orchestrator decision tree. -- Added/updated `MagenticE2E_ImplementationReview.md` documenting the final coverage state and production behavior verified by the suite. -- Updated `MagenticWorkflowBuilder.Build()` to throw `InvalidOperationException` when no participants have been added. -- Updated `MagenticOrchestrator.TakeTurnAsync()` to reject new messages after the Magentic task context is terminated, matching the existing terminal-state protection on plan-review responses. -- Aligned Magentic orchestration behavior with the expected coordination loop: normal participant returns resume progress-ledger coordination instead of replanning. -- Ensured stall handling uses the intended `StallCount > MaxStallCount` semantics and preserves stall context for plan review when replanning after a stall. -- Ensured the orchestrator declares the final-answer output protocol used by fully built workflow execution. - -Validation performed: - -- `dotnet build tests/Microsoft.Agents.AI.Workflows.UnitTests/Microsoft.Agents.AI.Workflows.UnitTests.csproj -f net10.0` -- `dotnet test --project tests/Microsoft.Agents.AI.Workflows.UnitTests/Microsoft.Agents.AI.Workflows.UnitTests.csproj -f net10.0 --filter-class '*MagenticOrchestrationTests'` -- Parallel validation completed with no review comments or security alerts. - -### Contribution Checklist - -- [x] The code builds clean without any errors or warnings -- [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/agent-framework/blob/main/CONTRIBUTING.md) -- [x] All unit tests pass, and I have added new tests where possible -- [ ] **Is this a breaking change?** If yes, add "[BREAKING]" prefix to the title of the PR. diff --git a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md deleted file mode 100644 index 1d0fea78a3..0000000000 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md +++ /dev/null @@ -1,263 +0,0 @@ -# Magentic E2E Implementation Review - -## Review Scope - -This document reviews the current Magentic E2E implementation against the original plan in -`MagenticE2E_TestPlan.md`. - -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/MagenticManager.cs` -- `dotnet/src/Microsoft.Agents.AI.Workflows/Specialized/Magentic/MagenticTaskContext.cs` -- `dotnet/src/Microsoft.Agents.AI.Workflows/MagenticWorkflowBuilder.cs` -- `dotnet/src/Microsoft.Agents.AI.Workflows/MagenticPlanReviewRequest.cs` - -## Executive Summary - -The current implementation contains **23 Magentic end-to-end tests** in -`MagenticOrchestrationTests.cs`. The suite builds real workflows through -`MagenticWorkflowBuilder.Build()` and exercises the orchestrator through streaming workflow -execution, pending plan-review requests, checkpoint/resume, event collection, participant routing, -reset/replan flows, build-time validation, post-termination rejection, and yielded final outputs. - -The implementation is fully aligned with the original plan. All planned orchestration paths are -covered: - -- `MagenticTaskContext.IsStalled` uses `StallCount > MaxStallCount`. -- `MaxStallCount` should be read as the number of stalls tolerated before reset. -- Direct checkpoint payload inspection is intentionally skipped because the serialized checkpoint - shape is an internal implementation detail. -- Checkpoint/resume is covered behaviorally by plan-review tests that pause and resume across - checkpoint boundaries. -- `MagenticWorkflowBuilder.Build()` now validates that at least one participant is present, - throwing `InvalidOperationException` on empty team. -- `MagenticOrchestrator.TakeTurnAsync()` now guards against post-termination messages, matching - the existing guard in `ProcessPlanReviewAsync`. The framework accepts the message - (`TrySendMessageAsync` returns true), but the orchestrator throws `InvalidOperationException` - which surfaces as a `WorkflowErrorEvent`. - -## Production Implementation Findings - -### Output protocol declaration - -`MagenticOrchestrator.ConfigureProtocol()` declares `.YieldsOutput>()`, matching -the final answer output emitted by the orchestrator. - -Assessment: **Complete.** Every final-answer E2E test depends on this protocol being declared -correctly for fully built workflow execution. - -### Normal participant return resumes coordination without replanning - -`MagenticOrchestrator.TakeTurnAsync()` now distinguishes the initial turn from subsequent -participant returns: - -- Initial user turn initializes `MagenticTaskContext` and calls the plan/update path. -- Participant returns go directly back into `RunCoordinationRoundAsync()`. - -Assessment: **Complete.** This matches the Python Magentic loop and is covered by the multi-round, -progress decrement, consecutive stall, and empty next-speaker fallback tests. These tests no longer -expect facts/plan manager calls after normal participant responses. - -### Stall threshold uses `>` semantics - -`MagenticTaskContext.IsStalled` evaluates `StallCount > MaxStallCount`. - -Assessment: **Complete.** This matches Python behavior. Tests that must reset on the first stalled -ledger use `WithMaxStalls(0)`, and tests that tolerate one stall before reset use -`WithMaxStalls(1)`. The original test plan and comments now describe the same `>` behavior. - -### Stall-triggered plan review preserves `IsStalled` - -`ResetAndReplanAsync()` captures whether reset was caused by a stall before counters are reset and -passes that value through the replan/signoff path. - -Assessment: **Complete.** `PlanReview_On_Stall_Replan` verifies that the initial review is not -stalled and the replanned review request has `IsStalled=true`. - -### Progress-ledger parse retry and reset behavior - -Invalid progress-ledger responses are retried, warnings are emitted, and exhausted retries trigger -reset/replan. - -Assessment: **Complete.** Covered by `ProgressLedger_Retry_On_Parse_Failure` and -`ProgressLedger_Max_Retries_Triggers_Reset`. - -### Empty-team build-time validation - -`MagenticWorkflowBuilder.Build()` now throws `InvalidOperationException` when no participants have -been added. - -Assessment: **Complete.** This is a new production behavior added alongside the test. The builder -previously allowed building with an empty team, which would crash at runtime when trying to select -the first participant as a fallback speaker. - -### Post-termination message rejection - -`MagenticOrchestrator.TakeTurnAsync()` now checks `IsTerminated` before processing any turn. This -matches the existing guard in `ProcessPlanReviewAsync`. The framework does not have a non-erroneous -terminal state — `TrySendMessageAsync` always returns true — but the Magentic orchestrator -explicitly throws `InvalidOperationException`, which surfaces as a `WorkflowErrorEvent`. - -Assessment: **Complete.** New production guard added to `TakeTurnAsync`. Tested by -`Terminated_Context_Rejects_New_Messages` which sends a chat message after the workflow yields -output and verifies the resulting `WorkflowErrorEvent`. - -## Implemented Test Inventory - -| Test | Original Plan Area | Current Assessment | -|---|---|---| -| `Task_Completes_When_RequestSatisfied` | Happy path | Complete. Immediate satisfaction yields final output. | -| `PlanReview_Approved_Proceeds` | Plan review / checkpoint-resume | Complete. Review pauses, approval resumes, workflow completes. | -| `Initial_Plan_Emits_PlanCreatedEvent` | Event emission | Complete. Verifies initial plan event. | -| `NextSpeaker_Invalid_Triggers_FinalAnswer` | Next speaker validation / warnings | Complete. Invalid participant warning and final-answer fallback. | -| `ProgressLedger_Updated_Event_Emitted` | Progress ledger / events | Complete. Verifies progress-ledger event. | -| `PlanSignoff_Disabled_Proceeds_Immediately` | Happy path | Complete. No pending review request when signoff is disabled. | -| `NextSpeaker_Empty_Falls_Back_To_First` | Next speaker validation / warnings | Complete. Empty speaker warns, falls back to first participant, and completes without stale replan responses. | -| `Task_Completes_After_Multiple_Rounds` | Happy path / coordination loop | Complete. Multiple coordination rounds complete without normal-return replan. | -| `PlanReview_Revised_Triggers_Replan` | Plan review | Complete. One revision triggers replan and a second review. | -| `MaxRoundLimit_Terminates_Workflow` | Limits | Complete. Round limit yields termination message. | -| `MaxStallCount_Triggers_Reset` | Limits / stall detection | Complete. First stalled ledger resets with `WithMaxStalls(0)`. | -| `Instruction_Message_Sent_When_Present` | Edge case / instruction delivery | Complete. Two-round flow proves the instruction code path executes; instruction content is internal messaging not observable from the E2E event stream. | -| `PlanReview_On_Stall_Replan` | Plan review / stall reset | Complete. Stall-triggered replan review has `IsStalled=true`. | -| `MaxResetLimit_Terminates_Workflow` | Limits | Complete. Reset limit yields termination message. | -| `ProgressLedger_Retry_On_Parse_Failure` | Progress ledger validation | Complete. Warning is emitted, 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 | Behaviorally covered. No-progress ledger causes reset/replan under configured threshold. | -| `Task_Delegates_To_Correct_Agent` | Happy path / routing | Complete. Selected participant responds; non-selected participant does not. | -| `Progress_Made_Decrements_StallCount` | Stall detection | Complete. Progress after a stall decrements/clears stall pressure and avoids reset. | -| `Consecutive_Stalls_Trigger_Reset` | Stall detection | Complete. Consecutive stalls exceed `MaxStallCount` and reset/replan. | -| `PlanReview_Multiple_Revisions` | Plan review | Complete. Multiple revisions are handled before approval and completion. | -| `Empty_Team_Build_Throws` | Edge case / empty team | Complete. `Build()` throws `InvalidOperationException` when no participants are added. New production validation added. | -| `Terminated_Context_Rejects_New_Messages` | Edge case / post-termination | Complete. Framework accepts the message, but Magentic throws `InvalidOperationException` surfaced as `WorkflowErrorEvent`. New production guard added to `TakeTurnAsync`. | - -## 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 | Covered with direct selected/non-selected participant assertions. | -| `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 updated `StallCount > MaxStallCount` semantics. | - -Summary: **3 complete / 3 planned**. - -### 4. Stall Detection Tests - -| Planned Test | Current Status | Notes | -|---|---|---| -| `Stall_IsInLoop_Increments_StallCount` | Behaviorally covered | Covered by reset behavior when `IsInLoop=true`; direct counter inspection is intentionally avoided. | -| `Stall_NoProgress_Increments_StallCount` | Behaviorally covered | Covered by reset behavior when `IsProgressBeingMade=false`; direct counter inspection is intentionally avoided. | -| `Progress_Made_Decrements_StallCount` | Complete | Covered by avoiding reset after later progress. | -| `Consecutive_Stalls_Trigger_Reset` | Complete | Covered with two stalls exceeding `MaxStallCount` 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` | Complete | Warning, fallback, and completion are covered with current no-replan flow. | -| `NextSpeaker_Invalid_Triggers_FinalAnswer` | Complete | Covered directly. | -| `NextSpeaker_Valid_Delegates_Correctly` | Complete | Covered by `Task_Delegates_To_Correct_Agent`. | - -Summary: **3 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 revision, multiple revisions, stall reset, no-progress reset, and max-retry reset paths. | -| `Warning_Events_On_Errors` | Complete | Warnings are asserted across empty/invalid next-speaker and progress-ledger failure tests. | - -Summary: **3 complete / 3 planned**. - -### 8. Checkpoint/Resume Tests - -| Planned Test | Current Status | Notes | -|---|---|---| -| `Checkpoint_Saves_TaskContext` | Intentionally skipped | Direct checkpoint payload inspection is skipped because the serialized checkpoint shape is internal. | -| `Checkpoint_Resume_Continues_Correctly` | Behaviorally covered | Approval, revision, multiple-revision, and stall-with-signoff tests all pause and resume through checkpoints. | -| `Checkpoint_Preserves_ProgressLedger` | Intentionally skipped | Direct checkpoint payload inspection is skipped because the serialized checkpoint shape is internal. | - -Summary: **1 behaviorally covered, 2 intentionally skipped / 3 planned**. - -### 9. Edge Cases - -| Planned Test | Current Status | Notes | -|---|---|---| -| `Empty_Team_Handling` | Complete | `Build()` throws `InvalidOperationException` when no participants are added. Production validation added. | -| `Single_Agent_Team` | Behaviorally covered | Most tests run with one participant, but there is no dedicated single-agent edge-case test. | -| `Instruction_Message_Sent_When_Present` | Complete | Two-round flow proves the instruction code path executes. Instruction content is internal messaging delivered via `context.SendMessageAsync()` and is not observable from the E2E event stream without custom test infrastructure. | -| `Terminated_Context_Rejects_New_Messages` | Complete | Framework accepts the queued message, but the Magentic orchestrator throws `InvalidOperationException` which surfaces as `WorkflowErrorEvent`. Production guard added to `TakeTurnAsync`. | - -Summary: **3 complete, 1 behaviorally covered / 4 planned**. - -## Success Criteria Assessment - -| Success Criterion | Assessment | -|---|---| -| All logical forks in `MagenticOrchestrator` are covered by at least one test | **Met.** All orchestration branches are covered, including the newly added `IsTerminated` guard in `TakeTurnAsync`. | -| 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.** Tests build through `MagenticWorkflowBuilder(...).Build()`. | -| Each test verifies specific event emissions and state changes | **Met.** Event/output assertions are strong; direct internal counter and checkpoint payload inspection are intentionally avoided. | -| Tests cover both `requirePlanSignoff=true` and `false` paths | **Met.** Signoff and no-signoff flows are both exercised. | -| Checkpoint/resume functionality is verified | **Behaviorally met.** Resume is exercised through plan-review workflows; direct checkpoint-state checking is intentionally skipped. | - -## Production Changes Made During Test Implementation - -Two production behavior changes were introduced alongside the tests: - -1. **`MagenticWorkflowBuilder.Build()`** — throws `InvalidOperationException` when the team list - is empty, preventing a runtime crash when the orchestrator tries to select a fallback speaker. - -2. **`MagenticOrchestrator.TakeTurnAsync()`** — added `IsTerminated` guard matching the existing - one in `ProcessPlanReviewAsync`. This ensures that both entry points (new messages and plan - review responses) consistently reject interaction after the workflow has terminated. - -## Overall Conclusion - -The Magentic E2E suite is a complete implementation of the original plan. It contains **23 tests** -covering all production behavior: planning, plan review, checkpointed resume, participant routing, -progress-ledger retries, warning paths, final-answer generation, reset/replan behavior, the updated -`StallCount > MaxStallCount` stall threshold, build-time validation of the team list, and -post-termination message rejection. - -The two intentionally skipped items (direct checkpoint payload inspection) are not testable through -the E2E streaming API without exposing internal implementation details. diff --git a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_TestPlan.md b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_TestPlan.md deleted file mode 100644 index 6f0ceb8403..0000000000 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_TestPlan.md +++ /dev/null @@ -1,233 +0,0 @@ -# Magentic E2E Test Plan - -## Overview - -This document outlines a comprehensive plan for adding end-to-end (E2E) tests for the Magentic orchestrator, similar to the existing `HandoffOrchestrationTests.cs` and smoke test patterns. These tests will drive a fully-built workflow through `MagenticWorkflowBuilder.Build()` and verify correct behavior at every logical fork in the orchestrator logic. - -## Background - -### Magentic Orchestrator Logic Flow - -Based on the analysis of `MagenticOrchestrator.cs`, the orchestrator follows this decision tree: - -``` -TakeTurn (Initial) - └─> UpdatePlanAndDelegateAsync - ├─> Create/Update Plan via MagenticManager.UpdatePlanAsync - ├─> Emit MagenticPlanCreatedEvent or MagenticReplannedEvent - └─> If requirePlanSignoff: SubmitPlanReviewRequestAsync - └─> ProcessPlanReviewAsync (on human response) - ├─> If Approved: DelegateToTeamAsync - └─> If Revise: Add review to chat, UpdatePlanAndDelegateAsync - └─> If !requirePlanSignoff: DelegateToTeamAsync - -DelegateToTeamAsync - └─> RunCoordinationRoundAsync - ├─> CHECK: Hit round limit? → Yield termination message, set IsTerminated - ├─> CHECK: Hit reset limit? → Yield termination message, set IsTerminated - ├─> Increment RoundCount - ├─> UpdateProgressLedgerAsync (with retries) - │ └─> On exception (non-cancellation): ResetAndReplanAsync - ├─> Emit MagenticProgressLedgerUpdatedEvent - ├─> CHECK: IsRequestSatisfied? → PrepareFinalAnswerAsync - ├─> CHECK: IsInLoop OR !IsProgressBeingMade? → Increment StallCount - │ └─> Else: Decrement StallCount (min 0) - ├─> CHECK: IsStalled (StallCount > MaxStallCount)? → ResetAndReplanAsync - ├─> Validate NextSpeaker → Fallback to first participant if empty - ├─> CHECK: Invalid NextSpeaker? → Warning + PrepareFinalAnswerAsync - └─> Send instruction + TurnToken to next agent - -PrepareFinalAnswerAsync - └─> Get final answer from manager, yield output, set IsTerminated - -ResetAndReplanAsync - └─> Reset context, send ResetChatSignal, UpdatePlanAndDelegateAsync -``` - -## Test Categories - -### 1. Happy Path Tests - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `Task_Completes_When_RequestSatisfied` | Manager reports task satisfied on first coordination round | Workflow yields final answer and terminates | -| `Task_Delegates_To_Correct_Agent` | Manager selects specific agent as next speaker | Selected agent receives TurnToken | -| `Task_Completes_After_Multiple_Rounds` | Task requires multiple coordination rounds before completion | Each round delegates to specified agent, final answer produced when satisfied | -| `PlanSignoff_Disabled_Proceeds_Immediately` | With `requirePlanSignoff=false` | Workflow proceeds to team delegation without plan review request | - -### 2. Plan Review Tests (Human-in-the-Loop) - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `PlanReview_Approved_Proceeds` | Human approves initial plan | Workflow continues to DelegateToTeamAsync | -| `PlanReview_Revised_Triggers_Replan` | Human requests plan revision | Revision added to chat, plan updated, MagenticReplannedEvent emitted | -| `PlanReview_Multiple_Revisions` | Human revises multiple times | Each revision triggers replan until approved | -| `PlanReview_On_Stall_Replan` | Stall triggers replan with plan signoff | Plan review request sent with IsStalled=true | - -### 3. Limit Enforcement Tests - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `MaxRoundLimit_Terminates_Workflow` | RoundCount exceeds MaxRoundCount | Workflow terminates with "maximum round count limit" message | -| `MaxResetLimit_Terminates_Workflow` | ResetCount exceeds MaxResetCount | Workflow terminates with "maximum reset count limit" message | -| `MaxStallCount_Triggers_Reset` | StallCount exceeds MaxStallCount | ResetAndReplanAsync called, ResetChatSignal sent | - -### 4. Stall Detection Tests - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `Stall_IsInLoop_Increments_StallCount` | ProgressLedger reports IsInLoop=true | StallCount incremented | -| `Stall_NoProgress_Increments_StallCount` | ProgressLedger reports IsProgressBeingMade=false | StallCount incremented | -| `Progress_Made_Decrements_StallCount` | ProgressLedger reports progress being made | StallCount decremented (min 0) | -| `Consecutive_Stalls_Trigger_Reset` | Multiple stalls in a row exceed MaxStallCount | Reset and replan triggered | - -### 5. Progress Ledger Validation Tests - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `ProgressLedger_Retry_On_Parse_Failure` | First attempt fails JSON parsing | Retry up to MaxProgressLedgerRetryCount times | -| `ProgressLedger_Max_Retries_Triggers_Reset` | All retry attempts fail | ResetAndReplanAsync called | -| `ProgressLedger_Updated_Event_Emitted` | Valid progress ledger update | MagenticProgressLedgerUpdatedEvent emitted | - -### 6. Next Speaker Validation Tests - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `NextSpeaker_Empty_Falls_Back_To_First` | ProgressLedger returns empty next_speaker | First team member selected, warning emitted | -| `NextSpeaker_Invalid_Triggers_FinalAnswer` | next_speaker doesn't match any team member | Warning emitted, final answer prepared | -| `NextSpeaker_Valid_Delegates_Correctly` | Valid team member specified | TurnToken sent to correct executor | - -### 7. Event Emission Tests - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `Initial_Plan_Emits_PlanCreatedEvent` | First plan creation | MagenticPlanCreatedEvent emitted with full task ledger | -| `Replan_Emits_ReplannedEvent` | Plan update after initial | MagenticReplannedEvent emitted | -| `Warning_Events_On_Errors` | Various warning conditions | WorkflowWarningEvent emitted with appropriate message | - -### 8. Checkpoint/Resume Tests - -> **Note:** Direct checkpoint-state inspection tests (`Checkpoint_Saves_TaskContext`, -> `Checkpoint_Preserves_ProgressLedger`) are **skipped** — the serialized checkpoint format is an -> internal implementation detail. Checkpoint resume is instead verified behaviorally through -> plan-review workflows that pause and resume across checkpoint boundaries. - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `Checkpoint_Saves_TaskContext` | ~~Workflow checkpointed mid-execution~~ | *Skipped — internal format* | -| `Checkpoint_Resume_Continues_Correctly` | Resume from checkpoint | TaskContext restored, execution continues from saved state (behaviorally covered by plan-review tests) | -| `Checkpoint_Preserves_ProgressLedger` | ~~Resume preserves ledger state~~ | *Skipped — internal format* | - -### 9. Edge Cases - -| Test Name | Description | Expected Behavior | -|-----------|-------------|-------------------| -| `Empty_Team_Handling` | No participants added | Appropriate error or fallback behavior | -| `Single_Agent_Team` | Only one participant | Workflow functions correctly with single agent | -| `Instruction_Message_Sent_When_Present` | ProgressLedger has instruction_or_question | Instruction added to chat history and sent | -| `Terminated_Context_Rejects_New_Messages` | After termination, new messages arrive | InvalidOperationException thrown | - -## Implementation Approach - -### Test Infrastructure - -Following the patterns established in `HandoffOrchestrationTests.cs`: - -1. **TestReplayAgent for Manager**: Configure to return specific plan/progress ledger responses -2. **TestEchoAgent for Participants**: Simple agents that echo or return controlled responses -3. **MockChatClient**: For fine-grained control of LLM responses -4. **WorkflowRunResult**: Capture updates, outputs, checkpoints, and pending requests -5. **RunWorkflowAsync helper**: Execute workflow and collect results - -### Key Test Helpers Needed - -```csharp -// Helper to create manager agent with specific responses -internal static TestReplayAgent CreateManagerAgent( - List> planResponses, - List> progressLedgerResponses, - List> finalAnswerResponses); - -// Helper to create progress ledger JSON responses -internal static ChatMessage CreateProgressLedgerResponse( - bool isRequestSatisfied, - bool isInLoop, - bool isProgressBeingMade, - string nextSpeaker, - string instructionOrQuestion); - -// Helper to build and run Magentic workflow -internal static Task RunMagenticWorkflowAsync( - AIAgent manager, - List team, - List input, - bool requirePlanSignoff = false, - TaskLimits? limits = null, - ExecutionEnvironment environment = ExecutionEnvironment.InProcess_Lockstep); -``` - -### Test File Structure - -Create new test file: `MagenticOrchestrationTests.cs` - -``` -dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/ -├── MagenticOrchestrationTests.cs (NEW - E2E tests) -├── MagenticOrchestratorTests.cs (existing - protocol tests) -├── MagenticManagerTests.cs (existing - manager unit tests) -├── MagenticProgressLedgerTests.cs (existing - ledger unit tests) -└── TestProgressLedgerState.cs (existing - reuse for E2E) -``` - -## Test Execution Order - -1. **Phase 1: Basic Flow Tests** - - Happy path completion - - Single agent delegation - - Multiple rounds - -2. **Phase 2: Plan Review Tests** - - Approval flow - - Revision flow - -3. **Phase 3: Limit and Stall Tests** - - Round limits - - Reset limits - - Stall detection and recovery - -4. **Phase 4: Error Handling Tests** - - Invalid next speaker - - Progress ledger failures - -5. **Phase 5: Checkpoint Tests** - - Save and restore - -## Success Criteria - -- [ ] All logical forks in `MagenticOrchestrator` are covered by at least one test -- [ ] Tests use the same patterns as `HandoffOrchestrationTests` -- [ ] Tests run against fully-built workflows (not isolated components) -- [ ] Each test verifies specific event emissions and state changes -- [ ] Tests cover both `requirePlanSignoff=true` and `false` paths -- [ ] Checkpoint/resume functionality is verified - -## Estimated Implementation - -| Component | Estimated Tests | Complexity | -|-----------|-----------------|------------| -| Happy Path | 4 | Low | -| Plan Review | 4 | Medium | -| Limits | 3 | Low | -| Stall Detection | 4 | Medium | -| Progress Ledger | 3 | Medium | -| Next Speaker | 3 | Low | -| Events | 3 | Low | -| Checkpoints | 3 | High | -| Edge Cases | 4 | Medium | -| **Total** | **~31 tests** | | - -## Dependencies - -- Existing test helpers: `TestReplayAgent`, `TestEchoAgent`, `TestProgressLedgerState` -- Existing execution infrastructure: `RunWorkflowAsync`, `CheckpointManager` -- Extensions may be needed for Magentic-specific assertions