Add issue #5350 repro tests + root-cause validation plan

Agent-Logs-Url: https://github.com/microsoft/agent-framework/sessions/79cbae85-b46f-470f-978d-7970d6f9e091

Co-authored-by: lokitoth <6936551+lokitoth@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
2026-05-19 13:31:10 +00:00
committed by GitHub
Unverified
parent 66a09a76af
commit e28682a853
2 changed files with 417 additions and 0 deletions
@@ -0,0 +1,161 @@
# Issue #5350 — Root-cause validation plan
**Issue:** [`.NET: [Bug]: Checkpoint round-trip loses ToolApprovalRequestContent.ToolCall concrete type (FunctionCallContent → base ToolCallContent), breaking FICC approval resume`](https://github.com/microsoft/agent-framework/issues/5350)
**Status of repro:** A focused repro test class
`dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/ToolApprovalRequestCheckpointReproTests.cs`
was added covering six progressively more end-to-end variants of the path the issue
describes. **All six tests pass** on `main`, i.e. *the bug as described does not
reproduce* at any of the layers exercised here:
| # | Test | What it exercises | Result |
|---|-------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------|
| 1 | `Repro_5350_ToolApprovalRequestContent_DirectJsonMarshallerRoundtrip_PreservesFunctionCallContent` | `JsonMarshaller.Marshal/Marshal<T>` on `ToolApprovalRequestContent` directly (same options chain as `CheckpointManager.CreateJson`) | Pass |
| 2 | `Repro_5350_ToolApprovalRequestContent_WrappedInPortableValue_PreservesFunctionCallContent` | Same, wrapped in a `PortableValue` (as `PortableMessageEnvelope` would store it) | Pass |
| 3 | `Repro_5350_ToolApprovalRequestContent_AsExternalRequestData_PreservesFunctionCallContent` | Same, as the `Data` payload of an `ExternalRequest` | Pass |
| 4 | `Repro_5350_DirectJsonMarshallerRoundtrip_IsDeterministic` | 25× repetition of #1 to rule out flakiness / JIT order | Pass |
| 5 | `Repro_5350_CaptureWireFormat_ForInspection` | Captures the on-the-wire shape so we can compare against the OP's SQL row | Pass |
| 6 | `Repro_5350_EndToEnd_JsonCheckpointResume_PreservesFunctionCallContentAsync` | Full `CheckpointManager.CreateJson(InMemoryJsonStore) → RunStreamingAsync → SuperStep checkpoint → ResumeStreamingAsync` cycle with a `RequestPort<TARC, TARR>` | Pass |
This is **consistent with [@lokitoth's second comment](https://github.com/microsoft/agent-framework/issues/5350#issuecomment-4379664401)**:
> Looking at the MEAI types, it does have `[JsonDerivedType(typeof(FunctionCallContent), "functionCall")]` set on it, and has had it that way for some time, so it is unlikely to be the root cause. […] `JsonMarshaller` […] takes an optional `JsonSerializationOptions` provided by the user […] but this is used only if the internal one (via `WorkflowsJsonUtilities`, which chains to `AgentAbstractionsJsonUtilities`, which then chains to the `AIJsonUtilities` class) [is missing the type].
Inspection of the actual wire format produced by `JsonMarshaller` for a
`ToolApprovalRequestContent` containing a `FunctionCallContent` confirms the
discriminator is emitted:
```jsonc
// TARC at top level
{
"toolCall": {
"$type": "functionCall", // ← polymorphism discriminator present
"name": "DoTheThing",
"arguments": { "x": 42 },
"informationalOnly": false,
"callId": "call-1"
},
"requestId": "req-1"
}
// TARC inside a PortableValue (= shape stored by PortableMessageEnvelope)
{
"typeId": {
"assemblyName": "Microsoft.Extensions.AI.Abstractions, Version=10.5.0.0, …",
"typeName": "Microsoft.Extensions.AI.ToolApprovalRequestContent"
},
"value": {
"toolCall": {
"$type": "functionCall",
},
"requestId": "req-1"
}
}
```
So the OP's stated root-cause hypothesis — *"`AIContent`/`ToolCallContent`/
`FunctionCallContent` are missing `[JsonPolymorphic]`/`[JsonDerivedType]`, or
`CheckpointManager.CreateJson` does not pull from `AIJsonUtilities.DefaultOptions`"*
— is **not** what is producing the failure described in the issue. The annotations
exist, the resolver chain wires them through, and the discriminator does survive
both the direct `JsonMarshaller` round-trip and a real `Run → checkpoint → Resume`
cycle for a `RequestPort` whose request type is `ToolApprovalRequestContent`.
The remaining sections lay out, in priority order, the work needed to identify
the *actual* root cause. The plan deliberately keeps the failing repro from
above as the baseline ("this is what works") and walks outward from it toward
the OP's reported scenario, varying one dimension at a time.
## Plan
### Track A — Reproduce in a configuration closer to the OP's pattern "B"
The OP's repro path differs from the new tests in three concrete ways. Each is a
candidate root cause; the test matrix below isolates them. Each row is "add a
test that reproduces the OP's symptom (`postResume.ToolCall is not FunctionCallContent`)".
| Step | Variable that changes vs. the passing tests in this repo | Why it matters | Pass criteria |
|------|--------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------|
| A1 | Use `ChatClientAgent` + `ApprovalRequiredAIFunction` bound into a `WorkflowBuilder` (the GroupChatToolApproval "pattern B" path), with a fake `IChatClient` that emits a single `FunctionCallContent`. | This is the only major piece of the OP's setup that the current tests do not exercise. The TARC in this path is *generated* by `FunctionInvokingChatClient` and flows through the `AIAgentHostExecutor`. | Test fails (TARC.ToolCall comes back as base type) → root cause is in the agent-host/FICC bridge, not the marshaller. |
| A2 | Same as A1 but with the request payload also surfaced as part of a `ChatMessage.Contents`-style transport (whatever the agent host actually serializes through). Inspect the on-the-wire JSON for the inner `toolCall` and look for an absent or differently-named `$type`. | If the TARC is round-tripped as `AIContent`/`ChatMessage` instead of as itself, the polymorphism is two-deep (`AIContent` → TARC, TARC → ToolCall). It is plausible that one branch resolves but the other doesn't. | Find missing `$type` in serialized payload. |
| A3 | Same as the passing test #6 in this file, but with the SQL Server-style store that round-trips the `JsonElement` through `string` and back (e.g. `element.GetRawText()``JsonDocument.Parse`). | The OP uses Dapper + SQL Server. If the column is `nvarchar` and the round-trip preserves ordering, this should be identity-preserving — but if the OP uses `jsonb`-like storage that reorders metadata properties, the `$type` discriminator can be moved out of "first" position, which then requires `AllowOutOfOrderMetadataProperties = true`. Note `JsonMarshaller` only propagates that one flag from the user's `customOptions`. | Test fails when ordering is permuted before deserialization. Confirms the storage layer is reordering metadata. |
| A4 | Same as #6 but with a non-default `JsonSerializerOptions` passed as `customOptions` to `CheckpointManager.CreateJson`, where the user-supplied options do **not** include the polymorphism resolver and `JsonMarshaller`'s `LookupTypeInfo` falls back to them for some type. | `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. | Either the external options are never consulted for `AIContent` (good), or there is a sneak path where they are (regression). |
### Track B — Validate the wire format the OP actually persists
Once Track A has reproduced (or has clearly failed to reproduce) the symptom,
ask the OP for one of the following, in order of preference:
1. The raw `JsonElement.GetRawText()` they pass to their SQL store on commit,
and the raw string they read back on `RetrieveCheckpointAsync` — for the
exact checkpoint that contains the failing `ToolApprovalRequestContent`.
This tells us in a single round-trip whether:
- the `$type` discriminator is present on commit (rules out write-side bug),
- the `$type` discriminator survives the SQL round-trip (rules out store bug),
- the discriminator is in metadata-first position (rules out the
`AllowOutOfOrderMetadataProperties` story).
2. A repro PR/gist that runs against `CheckpointManager.CreateJson(...)` with
an in-memory `JsonCheckpointStore` shim that mirrors how their SQL subclass
passes bytes through. The OP already offered this in the issue body
(*"I can produce a minimal standalone repro against a fake `IChatClient` if useful — let me know."*) — taking them up on it short-circuits a lot of guessing.
### Track C — Defense-in-depth fixes worth landing regardless of root cause
These are addressed in the OP's "Asks" section (asks 24) and are useful
documentation/ergonomics improvements even if the root cause turns out to be in
the OP's store implementation:
1. **Doc-only**: in the XML docs for `CheckpointManager.CreateJson` and on the
`JsonCheckpointStore` base class, explicitly state that the internal options
chain through `WorkflowsJsonUtilities → AgentAbstractionsJsonUtilities →
AIJsonUtilities`, and that user-supplied `customOptions` are consulted only
when the internal chain has no `TypeInfo` for a requested type.
2. **Doc-only**: document the contract that any user `JsonCheckpointStore`
subclass MUST preserve the exact byte sequence of the `JsonElement` it is
given. If the underlying store reorders JSON metadata, the consumer must opt
in by setting `AllowOutOfOrderMetadataProperties = true` on a
`JsonSerializerOptions` passed as `customOptions` to
`CheckpointManager.CreateJson(...)`.
3. **Optional, only if Track A reproduces**: add a regression test mirroring
the failing path under `dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests`
so the fix is locked in.
### Track D — Reject the OP's hypothesis (if not already done)
The combination of:
- the polymorphism annotations existing in `Microsoft.Extensions.AI` for some time
(per @lokitoth),
- the resolver chain in `WorkflowsJsonUtilities.CreateDefaultOptions()` already
putting `AgentAbstractionsJsonUtilities.DefaultOptions.TypeInfoResolver` first
(which itself puts `AIJsonUtilities.DefaultOptions.TypeInfoResolver` first),
- the wire format captured above showing `"$type": "functionCall"` is present, and
- the full `Run → checkpoint → Resume` test in this PR passing,
is sufficient to **disprove** the OP's stated hypothesis. Once Track A or
Track B identifies the actual cause, the issue should be updated with a brief
explanation of why the original guess was incorrect, so future readers don't
re-tread it.
## Pointers for the next investigator
- Repro tests:
`dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/ToolApprovalRequestCheckpointReproTests.cs`
- Marshaller under test:
`dotnet/src/Microsoft.Agents.AI.Workflows/Checkpointing/JsonMarshaller.cs`
- Where `CheckpointManager.CreateJson` enters that path:
`dotnet/src/Microsoft.Agents.AI.Workflows/CheckpointManager.cs`
- Options chain (TARC ⇒ resolver):
`dotnet/src/Microsoft.Agents.AI.Workflows/WorkflowsJsonUtilities.cs`
`dotnet/src/Microsoft.Agents.AI.Abstractions/AgentAbstractionsJsonUtilities.cs`
`Microsoft.Extensions.AI.AIJsonUtilities.DefaultOptions`
- `PortableValue`-side serialization (this is where `value.Value.GetType()` is
used on write, which is the most plausible *internal* place a polymorphism
bug could hide, but the wire capture above shows it is not the cause in the
TARC-as-RequestPort scenario):
`dotnet/src/Microsoft.Agents.AI.Workflows/Checkpointing/PortableValueConverter.cs`
- Likely next code to inspect for Track A1/A2 (agent-host serialization path):
`dotnet/src/Microsoft.Agents.AI.Workflows/AIAgentHostExecutor*.cs` and
whatever turns the FICC-generated TARC into something
`AIAgentHostExecutor` stores in its state bag.
@@ -0,0 +1,256 @@
// Copyright (c) Microsoft. All rights reserved.
using System;
using System.Collections.Generic;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using FluentAssertions;
using Microsoft.Agents.AI.Workflows.Checkpointing;
using Microsoft.Agents.AI.Workflows.InProc;
using Microsoft.Extensions.AI;
namespace Microsoft.Agents.AI.Workflows.UnitTests;
/// <summary>
/// Repro tests for issue #5350: <c>ToolApprovalRequestContent.ToolCall</c> reportedly loses its
/// concrete <see cref="FunctionCallContent"/> type after being persisted via a
/// <c>JsonCheckpointStore</c>-backed <c>CheckpointManager</c> and restored on resume.
///
/// These tests bypass the OP's SQL-backed store and HITL agent setup and directly exercise
/// the same JSON pipeline used by the checkpoint path (<see cref="JsonMarshaller"/> ->
/// <see cref="PortableValueConverter"/> -> <see cref="WorkflowsJsonUtilities"/>.DefaultOptions,
/// which chains through AgentAbstractionsJsonUtilities -> AIJsonUtilities), at progressively
/// more end-to-end layers up to a full <c>CheckpointManager.CreateJson(...)</c> +
/// <c>RunStreamingAsync</c> / <c>ResumeStreamingAsync</c> cycle.
///
/// At the time of writing all of these tests <b>pass</b>, which provides counter-evidence
/// against the root-cause hypothesis stated in the issue body (missing polymorphism
/// metadata / serializer-options chain). See
/// <c>docs/working/issue-5350-root-cause-validation-plan.md</c> for the full investigation
/// plan that builds on this baseline.
/// </summary>
public class ToolApprovalRequestCheckpointReproTests
{
private const string RequestId = "req-1";
private const string CallId = "call-1";
private const string FunctionName = "DoTheThing";
private static FunctionCallContent MakeFunctionCall() => new(
callId: CallId,
name: FunctionName,
arguments: new Dictionary<string, object?> { ["x"] = 42 });
private static ToolApprovalRequestContent MakeApprovalRequest()
=> new(RequestId, MakeFunctionCall());
/// <summary>
/// Direct round-trip of a <see cref="ToolApprovalRequestContent"/> through the same
/// <see cref="JsonMarshaller"/> used by <c>CheckpointManager.CreateJson(...)</c>.
///
/// Per the issue, after deserialization the <c>ToolCall</c> property (declared as the
/// abstract base <c>ToolCallContent</c>) is expected to remain a
/// <see cref="FunctionCallContent"/>. If polymorphism is preserved, this test passes;
/// if the discriminator is dropped on the wire or on read, <c>ToolCall</c> comes back
/// as something other than <see cref="FunctionCallContent"/> and FICC's pattern match
/// (<c>tarc.ToolCall is FunctionCallContent { InformationalOnly: false }</c>) silently
/// skips the approval pair.
/// </summary>
[Fact]
public void Repro_5350_ToolApprovalRequestContent_DirectJsonMarshallerRoundtrip_PreservesFunctionCallContent()
{
ToolApprovalRequestContent original = MakeApprovalRequest();
ToolApprovalRequestContent roundTripped = JsonSerializationTests.RunJsonRoundtrip(original);
roundTripped.Should().NotBeNull();
roundTripped.RequestId.Should().Be(RequestId);
roundTripped.ToolCall.Should().NotBeNull();
// This is the assertion that, per issue #5350, fails on resume.
roundTripped.ToolCall.Should().BeOfType<FunctionCallContent>(
"ToolApprovalRequestContent.ToolCall must round-trip as its concrete FunctionCallContent type, " +
"otherwise FunctionInvokingChatClient.ExtractAndRemoveApprovalRequestsAndResponses will silently " +
"skip the approval pair after a checkpoint resume (issue #5350).");
FunctionCallContent? fcc = roundTripped.ToolCall as FunctionCallContent;
fcc!.CallId.Should().Be(CallId);
fcc.Name.Should().Be(FunctionName);
}
/// <summary>
/// Same round-trip as above, but wrapped in a <see cref="PortableValue"/>. This more closely
/// mirrors how the workflow runtime stores externally-visible request payloads in a checkpoint
/// (<see cref="PortableMessageEnvelope"/> / <see cref="PortableValue"/>). The
/// <see cref="PortableValueConverter"/> serializes the inner value with
/// <c>marshaller.Marshal(value.Value, value.Value.GetType())</c>, so on the write side the
/// runtime type is used (which should include the discriminator), and on the read side the
/// inner value is materialized as a <see cref="JsonElement"/> then re-deserialized as the
/// declared type via <see cref="PortableValue.As{T}"/>.
/// </summary>
[Fact]
public void Repro_5350_ToolApprovalRequestContent_WrappedInPortableValue_PreservesFunctionCallContent()
{
PortableValue original = new(MakeApprovalRequest());
PortableValue result = JsonSerializationTests.RunJsonRoundtrip(original);
ToolApprovalRequestContent? extracted = result.As<ToolApprovalRequestContent>();
extracted.Should().NotBeNull();
extracted!.RequestId.Should().Be(RequestId);
extracted.ToolCall.Should().NotBeNull();
extracted.ToolCall.Should().BeOfType<FunctionCallContent>(
"PortableValue-wrapped ToolApprovalRequestContent must preserve the concrete " +
"FunctionCallContent on the ToolCall property after checkpoint round-trip (issue #5350).");
}
/// <summary>
/// Round-trip the <see cref="ToolApprovalRequestContent"/> as the payload of an
/// <see cref="ExternalRequest"/>, which is the actual on-the-wire shape for HITL approval
/// requests flowing out of a workflow. This is the closest serializer-only proxy for what
/// happens when the issue reporter calls
/// <c>request.TryGetDataAs&lt;ToolApprovalRequestContent&gt;()</c> after
/// <c>InProcessExecution.ResumeStreamingAsync(...)</c>.
/// </summary>
[Fact]
public void Repro_5350_ToolApprovalRequestContent_AsExternalRequestData_PreservesFunctionCallContent()
{
RequestPort<ToolApprovalRequestContent, ToolApprovalResponseContent> port
= RequestPort.Create<ToolApprovalRequestContent, ToolApprovalResponseContent>("Approval");
ExternalRequest original = ExternalRequest.Create(port, MakeApprovalRequest(), RequestId);
ExternalRequest result = JsonSerializationTests.RunJsonRoundtrip(original);
ToolApprovalRequestContent? extracted = result.Data.As<ToolApprovalRequestContent>();
extracted.Should().NotBeNull();
extracted!.ToolCall.Should().NotBeNull();
extracted.ToolCall.Should().BeOfType<FunctionCallContent>(
"ExternalRequest.Data restored from a JSON checkpoint must preserve the concrete " +
"FunctionCallContent on ToolApprovalRequestContent.ToolCall (issue #5350).");
}
/// <summary>
/// Stability check: run the direct round-trip many times in a row to demonstrate that
/// the failure mode (if present) is deterministic and not a flaky/JIT-order artifact.
/// </summary>
[Fact]
public void Repro_5350_DirectJsonMarshallerRoundtrip_IsDeterministic()
{
for (int i = 0; i < 25; i++)
{
ToolApprovalRequestContent original = MakeApprovalRequest();
ToolApprovalRequestContent roundTripped = JsonSerializationTests.RunJsonRoundtrip(original);
roundTripped.ToolCall.Should().BeOfType<FunctionCallContent>(
$"iteration {i}: ToolCall must consistently round-trip as FunctionCallContent");
}
}
/// <summary>
/// End-to-end checkpoint -> resume repro using the actual <c>CheckpointManager.CreateJson(...)</c>
/// path that the issue reporter uses. A trivial workflow whose entry point is a
/// <c>RequestPort&lt;ToolApprovalRequestContent, ToolApprovalResponseContent&gt;</c> emits a
/// pending external request containing a <see cref="FunctionCallContent"/>. We then:
/// 1. checkpoint the run while the request is pending,
/// 2. resume from the checkpoint via a fresh <c>InProcessExecution</c>,
/// 3. read the re-emitted <see cref="RequestInfoEvent"/>, and
/// 4. assert that <c>request.Data.As&lt;ToolApprovalRequestContent&gt;().ToolCall</c> is
/// still a <see cref="FunctionCallContent"/>.
///
/// This is the closest serializer-and-runtime repro for issue #5350 that does not require a
/// real <c>ChatClientAgent</c> + <c>ApprovalRequiredAIFunction</c>.
/// </summary>
[Fact]
public async Task Repro_5350_EndToEnd_JsonCheckpointResume_PreservesFunctionCallContentAsync()
{
RequestPort<ToolApprovalRequestContent, ToolApprovalResponseContent> requestPort
= RequestPort.Create<ToolApprovalRequestContent, ToolApprovalResponseContent>("ApprovalPort");
ForwardMessageExecutor<ToolApprovalResponseContent> processor = new("Processor");
Workflow workflow = new WorkflowBuilder(requestPort)
.AddEdge(requestPort, processor)
.Build();
CheckpointManager checkpointManager = CheckpointManager.CreateJson(new InMemoryJsonStore());
InProcessExecutionEnvironment env = InProcessExecution.OffThread;
ToolApprovalRequestContent input = MakeApprovalRequest();
CheckpointInfo? checkpoint = null;
ExternalRequest? originalPendingRequest = null;
await using (StreamingRun firstRun = await env.WithCheckpointing(checkpointManager)
.RunStreamingAsync(workflow, input))
{
await foreach (WorkflowEvent evt in firstRun.WatchStreamAsync(blockOnPendingRequest: false))
{
if (evt is RequestInfoEvent requestInfo)
{
originalPendingRequest ??= requestInfo.Request;
}
if (evt is SuperStepCompletedEvent step && step.CompletionInfo?.Checkpoint is { } cp)
{
checkpoint = cp;
}
}
}
originalPendingRequest.Should().NotBeNull("the workflow should have emitted the approval request");
checkpoint.Should().NotBeNull("a checkpoint should have been produced while the request was pending");
// Sanity: the pre-checkpoint payload should be a FunctionCallContent.
ToolApprovalRequestContent? preCheckpoint = originalPendingRequest!.Data.As<ToolApprovalRequestContent>();
preCheckpoint.Should().NotBeNull();
preCheckpoint!.ToolCall.Should().BeOfType<FunctionCallContent>(
"the pre-checkpoint pending request payload must already be a FunctionCallContent");
// Resume from the checkpoint and capture the re-emitted request.
await using StreamingRun resumed = await env.WithCheckpointing(checkpointManager)
.ResumeStreamingAsync(workflow, checkpoint!);
ExternalRequest? resumedPendingRequest = null;
using CancellationTokenSource cts = new(TimeSpan.FromSeconds(10));
await foreach (WorkflowEvent evt in resumed.WatchStreamAsync(blockOnPendingRequest: false, cts.Token))
{
if (evt is RequestInfoEvent requestInfo)
{
resumedPendingRequest ??= requestInfo.Request;
}
}
resumedPendingRequest.Should().NotBeNull("the resumed workflow should re-emit the pending request");
ToolApprovalRequestContent? postResume = resumedPendingRequest!.Data.As<ToolApprovalRequestContent>();
postResume.Should().NotBeNull(
"ExternalRequest.Data.As<ToolApprovalRequestContent>() should materialize the payload after a JSON checkpoint resume");
// The assertion that the issue reporter says fails.
postResume!.ToolCall.Should().NotBeNull();
postResume.ToolCall.Should().BeOfType<FunctionCallContent>(
"after CheckpointManager.CreateJson round-trip via ResumeStreamingAsync, " +
"ToolApprovalRequestContent.ToolCall must still be a FunctionCallContent so that " +
"FunctionInvokingChatClient's pattern match (`tarc.ToolCall is FunctionCallContent`) continues to fire " +
"(issue #5350).");
}
/// <summary>
/// Captures the raw JSON produced for a <see cref="ToolApprovalRequestContent"/> by the
/// checkpoint marshaller. This test always passes; it exists to make the on-the-wire shape
/// visible in test output / debugger when investigating issue #5350 (e.g. to confirm the
/// <c>"$type": "functionCall"</c> discriminator is or is not present for the inner
/// <c>toolCall</c> property).
/// </summary>
[Fact]
public void Repro_5350_CaptureWireFormat_ForInspection()
{
JsonMarshaller marshaller = new();
JsonElement element = marshaller.Marshal(MakeApprovalRequest());
string serialized = element.GetRawText();
// Always-true assertion — purpose of this test is to expose the wire format.
serialized.Should().NotBeNullOrEmpty();
serialized.Should().Contain(CallId, "the call id should be present in the serialized form");
}
}