From 1011a415c0ecce8b14a11d0436f234c1aa570785 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 May 2026 14:25:05 +0000 Subject: [PATCH] =?UTF-8?q?Add=20A2b:=20HandoffWorkflowBuilder=20variant?= =?UTF-8?q?=20of=20A2=20=E2=80=94=20completes=20cleanly?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/e7100d8a-ca6d-48e6-8210-78ba33dbd53c Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com> --- .../issue-5350-root-cause-validation-plan.md | 1 + ...ToolApprovalRequestCheckpointReproTests.cs | 45 +++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/docs/working/issue-5350-root-cause-validation-plan.md b/docs/working/issue-5350-root-cause-validation-plan.md index 3f1469f50b..f741045645 100644 --- a/docs/working/issue-5350-root-cause-validation-plan.md +++ b/docs/working/issue-5350-root-cause-validation-plan.md @@ -84,6 +84,7 @@ not reproduce in any of them; A2 did however uncover a *separate, unrelated* bug |------|--------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------| | A1 | ~~Use `ChatClientAgent` + `ApprovalRequiredAIFunction` bound into a `WorkflowBuilder`~~ — **covered by test #7 in this PR; passes.** | Was the largest gap to the OP's repro. Closed. | OP hypothesis disproved — `TARC.ToolCall is FunctionCallContent` post-resume, and the tool is actually invoked exactly once when the approval response is sent. | | A2 | Multi-agent variant: same as #7 but with the agent inside a `GroupChatBuilder` (the OP's actual orchestration) with `RoundRobinGroupChatManager`. | If group chat re-encodes TARC as part of `ChatMessage.Contents` (`AIContent` polymorphism is two-deep through `ToolApprovalRequestContent`), one branch may resolve and the other not. | OP hypothesis disproved — `TARC.ToolCall is FunctionCallContent` post-resume. **But:** sending the approval response surfaces a *different* real bug — `FunctionInvokingChatClient.ExtractAndRemoveApprovalRequestsAndResponses` throws `ArgumentException: An item with the same key has already been added. Key: ficc_call-1`. Pinned by test as documented misbehavior. | +| A2b | Same as A2 but using `HandoffWorkflowBuilder` (initial agent has the approval tool, a no-op peer agent makes the handoff graph valid; the mock chat client never emits a `handoff_to_*` call). | If the duplicate-key bug from A2 is broader than `RoundRobinGroupChatManager` and lives in the shared `AIAgentHostExecutor` / `ChatProtocolExecutor` path, the handoff workflow should hit it too. | OP hypothesis disproved — `TARC.ToolCall is FunctionCallContent` post-resume **and** the workflow completes cleanly: tool invoked exactly once, zero errors, zero executor failures. The duplicate-key bug from A2 does **not** occur on the handoff path, narrowing it to the group-chat-specific orchestration. | | A3 | Same as #7 but with the checkpoint `JsonElement` round-tripped through `string` + `JsonDocument.Parse` between commit and retrieve (`StringRoundTripJsonStore`), emulating the SQL `nvarchar` hop in the OP's Dapper store. | The OP uses Dapper + SQL Server. If the column / driver round-trip preserves ordering, this should be identity-preserving — but if it reorders metadata properties, the `$type` discriminator can be moved out of first position, which then requires `AllowOutOfOrderMetadataProperties = true`. | OP hypothesis disproved — byte-preserving string round-trip is identity-preserving for the relevant payload; `TARC.ToolCall is FunctionCallContent` post-resume; tool invoked exactly once. The OP's storage layer would have to *perturb* the JSON (e.g. reorder metadata) for this to reproduce. | | A4 | Same as #7 but with non-default `JsonSerializerOptions` (`JsonSerializerDefaults.Web`, no AIJsonUtilities resolver) passed as `customOptions` to `CheckpointManager.CreateJson`. | `JsonMarshaller.LookupTypeInfo` only goes to the external options when the internal chain doesn't know about the type. For most cases this won't trigger, but it's worth confirming that supplying a custom `JsonSerializerOptions` does not silently displace the internal chain. | OP hypothesis disproved — custom external options that DO NOT know about `AIContent` types are correctly ignored for known types; the internal `WorkflowsJsonUtilities.DefaultOptions` chain wins. `TARC.ToolCall is FunctionCallContent` post-resume; tool invoked exactly once. | diff --git a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/ToolApprovalRequestCheckpointReproTests.cs b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/ToolApprovalRequestCheckpointReproTests.cs index a3a2b2ce9b..2263ed8bc6 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/ToolApprovalRequestCheckpointReproTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/ToolApprovalRequestCheckpointReproTests.cs @@ -539,6 +539,51 @@ public class ToolApprovalRequestCheckpointReproTests }); } + /// + /// Track A2b — same as A2, but using a HandoffWorkflowBuilder instead of a group + /// chat. The initial agent is the same approval-tool-equipped + /// as test #7; a second dummy agent is registered solely so the handoff graph has a valid + /// peer (the mock chat client never emits a handoff_to_* call, so the workflow stays + /// on the initial agent). This isolates whether anything in the handoff-specific + /// orchestration (handoff tool injection, HandoffMessagesFilter, the + /// HandoffStartExecutor/HandoffEndExecutor wrap-up) perturbs the + /// payload across a JSON checkpoint resume. + /// + /// Finding: the handoff path is fully clean. The + /// type is preserved after resume and approving the resumed request completes the + /// workflow without errors and invokes the wrapped exactly once. + /// This is in contrast to A2 (group chat), which preserves the type but crashes on + /// approval with a duplicate-key in FICC. The OP's + /// hypothesis (TARC.ToolCall is not FunctionCallContent) still does not reproduce. + /// + /// + [Fact] + public async Task Repro_5350_A2b_HandoffWorkflowBuilder_WithApprovalRequiredTool_JsonCheckpointResume_PreservesFunctionCallContentAndInvokesToolAsync() + { + ReproHarness harness = new(); + + // Second agent is a no-op peer required to give the handoff graph a valid target. + // The mock chat client only ever emits a FunctionCallContent for GetWeather, so the + // initial agent never actually hands off; the second agent is never invoked. + MockChatClient peerChatClient = new((messages, options) => + new ChatResponse(new ChatMessage(ChatRole.Assistant, "(unused peer)"))); + ChatClientAgent peerAgent = new( + peerChatClient, + instructions: "Unused peer agent.", + name: "PeerAgent"); + + Workflow workflow = AgentWorkflowBuilder + .CreateHandoffBuilderWith(harness.Agent) + .WithHandoff(harness.Agent, peerAgent) + .Build(); + + await RunReproAsync( + workflow, + harness, + CheckpointManager.CreateJson(new InMemoryJsonStore()), + scenarioName: "A2b (handoff)"); + } + /// /// Track A3 — same as the maximal repro (test #7) but the checkpoint /// is round-tripped through +