From 1c8e16b332129ad4354bd70a56d35e6e79d230da Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 13 May 2026 22:58:34 +0000 Subject: [PATCH] Rewrite Magentic E2E implementation review Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/21f3b1ae-183e-4fea-99ad-14efc19f084d Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> --- .../MagenticE2E_ImplementationReview.md | 227 +++++++++--------- 1 file changed, 110 insertions(+), 117 deletions(-) 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 bd953ade23..17d98d74d5 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md +++ b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/MagenticE2E_ImplementationReview.md @@ -9,220 +9,213 @@ 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/MagenticPlanReviewRequest.cs` ## Executive Summary -The current implementation contains **18 Magentic end-to-end tests** that build and run real workflows through `MagenticWorkflowBuilder.Build()`. The original plan estimated roughly 31 tests. +The current implementation contains **18 Magentic end-to-end tests** in `MagenticOrchestrationTests.cs`. Each test builds and executes a real workflow through `MagenticWorkflowBuilder.Build()`, so the test suite exercises the workflow protocol, checkpoint/resume path, orchestrator, manager, event stream, and output path together rather than testing isolated helpers only. -The implementation now covers all major user-visible orchestration paths: +The original plan estimated approximately 31 tests. The current implementation is a strong partial completion and now covers most high-value user-visible orchestration behavior, including: -- Initial planning and final-answer completion. -- Plan signoff disabled path. -- Plan review approval with checkpoint/resume. -- Plan review revision with checkpoint/resume and replanning. -- **Multiple plan revisions** before final approval. -- Multi-round delegation through a participant. -- Progress ledger event emission. -- **Progress ledger retry on parse failure** (invalid JSON → retry → success). -- **Progress ledger max retries triggering reset** (all retries fail → reset/replan). -- Empty next-speaker fallback warning. -- Invalid next-speaker warning and forced final answer. -- Round-limit termination. -- Reset-limit termination. -- Stall-triggered reset and replan (via `IsInLoop=true`). -- **Stall-triggered reset via no-progress** (`IsProgressBeingMade=false`). -- Stall-triggered replan when plan signoff is enabled. -- Instruction-bearing progress ledger path. +- Initial planning and final answer generation. +- Plan signoff disabled flow. +- Plan review approval flow. +- Single and multiple plan revision flows. +- Multi-round coordination. +- Round, reset, and stall limit enforcement. +- Progress ledger success, parse retry, and max-retry-failure reset behavior. +- Stall detection through both `IsInLoop=true` and `IsProgressBeingMade=false`. +- Empty and invalid next-speaker warning paths. - Replanned event emission. +- Basic instruction-bearing progress ledger flow. +- Checkpoint/resume through human review scenarios. -The implementation also includes a production fix in `MagenticOrchestrator.ConfigureProtocol()`: the protocol declares `.YieldsOutput>()`, matching the type yielded by `PrepareFinalAnswerAsync()`. - -The remaining gaps are primarily around direct state assertions, direct selected-participant routing assertions, and edge cases such as empty teams and post-termination handling. +The largest remaining gaps are direct participant-routing assertions, direct checkpoint-state assertions, the progress-made stall decrement path, and dedicated edge cases. ## Production Code Assessment -### Output declaration +### Output protocol declaration -`MagenticOrchestrator.ConfigureProtocol()` now declares: - -- `.YieldsOutput>()` +`MagenticOrchestrator.ConfigureProtocol()` declares `.YieldsOutput>()`, matching the `List` emitted by `PrepareFinalAnswerAsync()`. Assessment: -- 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. +- Correct and necessary for fully-built workflow execution. +- Covered indirectly by every E2E test that completes with a final answer. +- No additional production change is currently required for this area. + +### Progress ledger retry and reset behavior + +`MagenticManager.UpdateProgressLedgerAsync()` retries invalid progress ledger responses up to `TaskLimits.MaxProgressLedgerRetryCount` and emits a `WorkflowWarningEvent` for each parse/update failure. If every retry fails, the last exception is rethrown. `MagenticOrchestrator.RunCoordinationRoundAsync()` catches non-cancellation exceptions from progress ledger creation, emits a reset warning, and calls `ResetAndReplanAsync()`. + +Assessment: + +- `ProgressLedger_Retry_On_Parse_Failure` now covers retry recovery. +- `ProgressLedger_Max_Retries_Triggers_Reset` now covers max-retry failure and reset/replan recovery. +- The planned progress ledger validation category is now complete. ### Stall replan review request nuance -The original plan states that `PlanReview_On_Stall_Replan` should send a plan review request with `IsStalled=true`. +The original plan expected `PlanReview_On_Stall_Replan` to submit a review request with `IsStalled=true`. -Current code path: +Current behavior: -1. `RunCoordinationRoundAsync()` detects a stall. +1. `RunCoordinationRoundAsync()` detects the 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`. +4. `UpdatePlanAndDelegateAsync()` submits the replanned plan review request. +5. `SubmitPlanReviewRequestAsync()` derives `IsStalled` from the reset task context. -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. +Because the reset clears the stall count before the new review request is created, the current behavior does not preserve `IsStalled=true` on the replanned review request. The existing E2E test verifies the user-visible stall-with-signoff flow, but not the original `IsStalled=true` expectation. -## Implemented Tests +Changing this would affect behavior, so it should not be changed without an explicit product decision. -| Test | Original Plan Area | What It Verifies | Assessment | +## Implemented E2E Tests + +| Test | Plan Area | What It Verifies | Status | |---|---|---|---| -| `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 | +| `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 / 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 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 (`IsInLoop=true`) 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 — does not directly observe the sent instruction message. | -| `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 | -| `ProgressLedger_Retry_On_Parse_Failure` | Progress ledger validation | First ledger attempt returns invalid JSON (parse failure warning emitted); retry succeeds with valid JSON; workflow completes. | Complete | -| `ProgressLedger_Max_Retries_Triggers_Reset` | Progress ledger validation | All 3 retry attempts return invalid JSON; parse failure warnings emitted; exception triggers `ResetAndReplanAsync`; replan completes. | Complete | -| `Stall_NoProgress_Increments_StallCount` | Stall detection | `IsProgressBeingMade=false` (not `IsInLoop`) triggers stall increment, reset, and replan. | Complete | -| `PlanReview_Multiple_Revisions` | Plan review | Human rejects the plan twice before approving on the third review; two `MagenticReplannedEvent`s emitted; final answer produced. | 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. | 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. | Partial: does not directly inspect delivered instruction message. | +| `PlanReview_On_Stall_Replan` | Plan review / stall | Stall with signoff creates a second review request and completes after approval. | Mostly complete: does not assert `IsStalled=true`. | +| `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 | ## Coverage Against Original Plan ### 1. Happy Path Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| | `Task_Completes_When_RequestSatisfied` | Implemented | Covered directly. | -| `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; also asserts no plan review request/event is emitted. | +| `Task_Delegates_To_Correct_Agent` | Partially covered | Delegation occurs in multi-round tests, but no test directly proves that only the selected participant received the turn. | +| `Task_Completes_After_Multiple_Rounds` | Implemented | Covered directly. | +| `PlanSignoff_Disabled_Proceeds_Immediately` | Implemented | Covered directly. | Summary: **3 complete, 1 partial**. ### 2. Plan Review Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| | `PlanReview_Approved_Proceeds` | Implemented | Covered directly with checkpoint/resume. | -| `PlanReview_Revised_Triggers_Replan` | Implemented | Covered directly with one revision, replan event, second review request, approval, and completion. | -| `PlanReview_Multiple_Revisions` | **Implemented** | Two revisions before final approval; asserts two `MagenticReplannedEvent`s and correct plan content at each review stage. | -| `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. | +| `PlanReview_Revised_Triggers_Replan` | Implemented | Covered directly with one revision. | +| `PlanReview_Multiple_Revisions` | Implemented | Covered with two revisions before final approval. | +| `PlanReview_On_Stall_Replan` | Mostly implemented | Covers stall → reset → replan → second review → approval. Does not meet the original `IsStalled=true` expectation because current production behavior resets stall state first. | Summary: **3 complete, 1 mostly complete**. -### 3. Limit Enforcement Tests — all planned limit enforcement tests are implemented +### 3. Limit Enforcement Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| | `MaxRoundLimit_Terminates_Workflow` | Implemented | Covered directly. | | `MaxResetLimit_Terminates_Workflow` | Implemented | Covered directly. | -| `MaxStallCount_Triggers_Reset` | Implemented | Covered directly as stall-triggered reset/replan. | +| `MaxStallCount_Triggers_Reset` | Implemented | Covered directly. | Summary: **3 complete — all planned limit enforcement tests are implemented**. ### 4. Stall Detection Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| -| `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` | **Implemented** | Uses `IsProgressBeingMade=false` as the stall trigger; verifies reset/replan with `MagenticReplannedEvent`. | -| `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. | +| `Stall_IsInLoop_Increments_StallCount` | Partially covered | Covered via reset behavior when `IsInLoop=true`; internal counter is not directly inspected. | +| `Stall_NoProgress_Increments_StallCount` | Implemented | Covered via `IsProgressBeingMade=false` reset/replan behavior. | +| `Progress_Made_Decrements_StallCount` | Not implemented | No test verifies a stalled count decreasing after a progress-making ledger. | +| `Consecutive_Stalls_Trigger_Reset` | Partially covered | Covered only in the `MaxStallCount=1` case, not multiple consecutive stalls. | Summary: **1 complete, 2 partial, 1 remaining**. ### 5. Progress Ledger Validation Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| -| `ProgressLedger_Retry_On_Parse_Failure` | **Implemented** | Returns invalid JSON once, retries with valid JSON, asserts parse-failure warning and successful completion. | -| `ProgressLedger_Max_Retries_Triggers_Reset` | **Implemented** | Returns invalid JSON for all 3 retry attempts; asserts parse-failure warnings, reset-trigger warning, `MagenticReplannedEvent`, and successful completion after replan. | +| `ProgressLedger_Retry_On_Parse_Failure` | Implemented | Covered directly. | +| `ProgressLedger_Max_Retries_Triggers_Reset` | Implemented | Covered directly. | | `ProgressLedger_Updated_Event_Emitted` | Implemented | Covered directly. | Summary: **3 complete — all planned progress ledger validation tests are implemented**. ### 6. Next Speaker Validation Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| | `NextSpeaker_Empty_Falls_Back_To_First` | Implemented | Covered directly. | | `NextSpeaker_Invalid_Triggers_FinalAnswer` | Implemented | Covered directly. | -| `NextSpeaker_Valid_Delegates_Correctly` | Partially covered | Valid delegation happens in multi-round tests, but the selected executor is not directly asserted. | +| `NextSpeaker_Valid_Delegates_Correctly` | Partially covered | Valid delegation is exercised, but selected participant routing is not directly asserted. | Summary: **2 complete, 1 partial**. ### 7. Event Emission Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| | `Initial_Plan_Emits_PlanCreatedEvent` | Implemented | Covered directly. | -| `Replan_Emits_ReplannedEvent` | Implemented | Covered by plan revision, stall reset, stall-with-plan-review, no-progress stall, and max-retries-reset tests. | -| `Warning_Events_On_Errors` | **Mostly covered** | Empty and invalid next-speaker warnings are covered. Progress-ledger parse-failure warnings are now covered by retry and max-retries tests. | +| `Replan_Emits_ReplannedEvent` | Implemented | Covered by revision, multiple revision, stall reset, no-progress stall, and max-retry reset scenarios. | +| `Warning_Events_On_Errors` | Mostly implemented | Empty/invalid next speaker warnings and progress ledger parse/reset warnings are covered. | -Summary: **3 complete** (warning events now substantially covered via progress-ledger retry tests). +Summary: **2 complete, 1 mostly complete**. ### 8. Checkpoint/Resume Tests -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| -| `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, multiple revisions, and stall-with-plan-review flows. | -| `Checkpoint_Preserves_ProgressLedger` | Not implemented | No direct assertion that progress ledger state is preserved across checkpoint/resume. | +| `Checkpoint_Saves_TaskContext` | Not implemented | No direct assertion on serialized `MagenticTaskContext`. | +| `Checkpoint_Resume_Continues_Correctly` | Partially covered | Resume is exercised through approval, revision, multiple revisions, and stall-with-signoff. | +| `Checkpoint_Preserves_ProgressLedger` | Not implemented | No direct assertion that progress ledger state survives checkpoint/resume. | Summary: **1 partial, 2 remaining**. ### 9. Edge Cases -| Planned Test | Status | Notes | +| Planned Test | Current Status | Notes | |---|---|---| -| `Empty_Team_Handling` | Not implemented | No coverage for zero participants. | -| `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. | +| `Empty_Team_Handling` | Not implemented | No zero-participant coverage. | +| `Single_Agent_Team` | Partially covered | Many tests use one participant successfully, but there is no dedicated edge-case test. | +| `Instruction_Message_Sent_When_Present` | Partially implemented | Instruction path executes, but exact message delivery is not directly observed. | +| `Terminated_Context_Rejects_New_Messages` | Not implemented | No post-termination input coverage. | -Summary: **1 partial, 3 remaining**. +Summary: **2 partial, 2 remaining**. ## Success Criteria Assessment -| Success Criterion | Status | Assessment | -|---|---|---| -| All logical forks in `MagenticOrchestrator` are covered by at least one test | **Substantially met** | All major forks are now covered: signoff/no-signoff, approved/revised/multiple-revision plan review, satisfied completion, empty/invalid speaker, round limit, reset limit, stall reset (both `IsInLoop` and `!IsProgressBeingMade`), stall-with-plan-review, instruction path, progress-ledger retry, and progress-ledger max-retries-trigger-reset. Remaining: post-termination behavior. | -| 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. 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 (including multiple revisions) use `true`; happy path, next speaker, limit, stall, ledger retry, and instruction tests use `false`. | -| Checkpoint/resume functionality is verified | Partially met | Resume is exercised through plan approval, single revision, multiple revisions, and stall-with-plan-review flows. Direct checkpoint state preservation remains untested. | +| Success Criterion | Assessment | +|---|---| +| All logical forks in `MagenticOrchestrator` are covered by at least one test | **Substantially met.** Major user-visible branches are covered: signoff/no-signoff, approval/revision/multiple revision, satisfied completion, round/reset/stall limits, progress-ledger retry/failure, empty/invalid speaker, instruction path, and stall detection through both main stall conditions. Remaining gaps are mostly direct state/routing assertions and post-termination behavior. | +| Tests use the same patterns as `HandoffOrchestrationTests` | **Met.** Tests use fully-built workflows, streaming runs, checkpointing, pending requests, events, and output collection. | +| Tests run against fully-built workflows | **Met.** Every E2E test builds through `MagenticWorkflowBuilder(...).Build()`. | +| Each test verifies specific event emissions and state changes | **Mostly met.** Event/output assertions are present, but internal counters, selected executor routing, and serialized checkpoint state are not directly asserted. | +| Tests cover both `requirePlanSignoff=true` and `false` paths | **Met.** Plan review scenarios use signoff; happy path, limit, stall, next-speaker, instruction, and ledger retry scenarios use no-signoff. | +| Checkpoint/resume functionality is verified | **Partially met.** Resume is exercised through human-review workflows, but direct checkpoint state inspection is still missing. | -## Key Observations - -- 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, the planned progress ledger validation category, and the planned event emission category. -- Both stall-trigger paths are now covered: `IsInLoop=true` (via `MaxStallCount_Triggers_Reset`) and `IsProgressBeingMade=false` (via `Stall_NoProgress_Increments_StallCount`). -- 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. -- Progress-ledger failure/retry paths are now fully covered, including both single-retry recovery and all-retries-fail-trigger-reset scenarios. - -## Recommended Next Tests +## Remaining Gaps and Recommended Next Tests Continue one test at a time in this order: -1. `Task_Delegates_To_Correct_Agent` / `NextSpeaker_Valid_Delegates_Correctly` — use multiple participants and assert only the selected participant receives the turn. -2. Direct checkpoint state tests — inspect persisted `MagenticTaskContext` and progress ledger state where possible. -3. Dedicated edge cases — empty team behavior, single-agent team behavior, and post-termination rejection. -4. `Progress_Made_Decrements_StallCount` — use `MaxStallCount > 1` with one stall followed by a progress round and verify the stall count decrements (requires observing that the workflow does not reset when it otherwise would). -5. Strengthen `Instruction_Message_Sent_When_Present` if a stable event or test hook is available to directly observe the sent instruction message. -6. 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. +1. `Task_Delegates_To_Correct_Agent` / `NextSpeaker_Valid_Delegates_Correctly` — use multiple participants and directly assert that the selected participant receives the turn while non-selected participants do not. +2. `Progress_Made_Decrements_StallCount` — construct a scenario with `MaxStallCount > 1`, one stalled ledger, then one progress-making ledger, and prove reset is avoided because stall count decreased. +3. `Consecutive_Stalls_Trigger_Reset` — cover a multi-stall threshold rather than only the simplified `MaxStallCount=1` case. +4. Direct checkpoint-state tests — inspect serialized `MagenticTaskContext` and progress ledger state if the test infrastructure exposes a stable way to do so. +5. Dedicated edge-case tests — zero participants, explicit single-agent behavior, and post-termination rejection. +6. Strengthen `Instruction_Message_Sent_When_Present` if a stable observable hook exists for the exact instruction `ChatMessage` sent to the participant. +7. Resolve the `PlanReview_On_Stall_Replan` `IsStalled` expectation by making an explicit behavior decision: either preserve stalled context for review requests or update the original expectation to match current reset-first behavior. ## Overall Conclusion -The current implementation covers **18 tests** and all major user-visible orchestration decisions. Three entire plan categories are now fully implemented: limit enforcement, progress ledger validation, and event emission. The plan review category is substantially complete with the addition of multiple-revision coverage. +The current Magentic E2E implementation is a strong partial completion of the original plan. It now covers **18 tests** and fully implements the planned limit enforcement and progress ledger validation categories. Plan review coverage is also substantially complete, including approval, single revision, multiple revisions, and stall-triggered review after replan. -The next highest-value work is direct participant-routing assertions and direct checkpoint-state assertions. The remaining edge cases (empty team, single-agent team, post-termination rejection) and the `Progress_Made_Decrements_StallCount` test represent the final gaps before full plan coverage. +The next highest-value work is to improve observability of routing and state rather than adding more broad happy-path coverage. Direct participant-routing assertions and checkpoint-state assertions would close the most important remaining verification gaps. Any change to the stall-review `IsStalled` behavior should be confirmed before implementation because it would alter product behavior.