Skip to content

Commit 4ae1f56

Browse files
alliscodeCopilot
andcommitted
Address PR #5668 review feedback round 2
- OutputConverter FRC: emit string results as raw text (no JSON-quoting), matching the wire contract for function_call_output.output. - OutputConverter FCC: validate non-empty CallId before closing the in-flight text message, so a skipped FCC no longer breaks output-item boundaries. - ToolApprovalIdMap.Record: take pre-serialized arguments JSON (string) and primitive callId/name. Drops [RequiresUnreferencedCode]/[RequiresDynamicCode] so trim/AOT warnings stop propagating to call sites. - ToolApprovalIdMap.Record: no-op when callId or name is empty. - Tests: dedup duplicate ConvertItemsToMessages_McpApprovalResponse no-mapping test; add coverage for empty-CallId boundary, raw-string FRC payload, and Record empty-key no-op. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 374b12e commit 4ae1f56

4 files changed

Lines changed: 108 additions & 38 deletions

File tree

dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/OutputConverter.cs

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,11 @@ public static async IAsyncEnumerable<ResponseStreamEvent> ConvertUpdatesToEvents
120120

121121
case FunctionCallContent functionCall:
122122
{
123+
if (functionCall.CallId is not { Length: > 0 })
124+
{
125+
break;
126+
}
127+
123128
foreach (var evt in CloseCurrentMessage(currentMessageBuilder, currentTextBuilder, accumulatedText))
124129
{
125130
yield return evt;
@@ -130,11 +135,6 @@ public static async IAsyncEnumerable<ResponseStreamEvent> ConvertUpdatesToEvents
130135
accumulatedText = null;
131136
previousMessageId = null;
132137

133-
if (functionCall.CallId is not { Length: > 0 })
134-
{
135-
break;
136-
}
137-
138138
var arguments = functionCall.Arguments is not null
139139
? JsonSerializer.Serialize(functionCall.Arguments)
140140
: "{}";
@@ -194,12 +194,19 @@ public static async IAsyncEnumerable<ResponseStreamEvent> ConvertUpdatesToEvents
194194
// wireId↔afRequestId mapping in the session state bag for later lookup
195195
// when the matching `mcp_approval_response` arrives on a subsequent turn.
196196
var wireId = ToolApprovalIdMap.ComputeWireId(approvalRequest.RequestId);
197-
ToolApprovalIdMap.Record(stateBag, wireId, approvalRequest.RequestId, approvalFunctionCall);
198197

199198
var approvalArguments = approvalFunctionCall.Arguments is not null
200199
? JsonSerializer.Serialize(approvalFunctionCall.Arguments)
201200
: "{}";
202201

202+
ToolApprovalIdMap.Record(
203+
stateBag,
204+
wireId,
205+
approvalRequest.RequestId,
206+
approvalFunctionCall.CallId,
207+
approvalFunctionCall.Name,
208+
approvalArguments);
209+
203210
var approvalItem = new OutputItemMcpApprovalRequest(
204211
wireId,
205212
"agent_framework",
@@ -272,17 +279,17 @@ public static async IAsyncEnumerable<ResponseStreamEvent> ConvertUpdatesToEvents
272279
accumulatedText = null;
273280
previousMessageId = null;
274281

275-
var outputJson = functionResult.Result switch
282+
var outputText = functionResult.Result switch
276283
{
277-
null => "null",
278-
string s => JsonSerializer.Serialize(s),
284+
null => string.Empty,
285+
string s => s,
279286
_ => JsonSerializer.Serialize(functionResult.Result),
280287
};
281288

282289
var itemId = GenerateItemId("fc");
283290
var outputItem = new OutputItemFunctionToolCallOutput(
284291
functionResult.CallId,
285-
BinaryData.FromString(outputJson));
292+
BinaryData.FromString(outputText));
286293

287294
var outputBuilder = stream.AddOutputItem<OutputItemFunctionToolCallOutput>(itemId);
288295
yield return outputBuilder.EmitAdded(outputItem);

dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/ToolApprovalIdMap.cs

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,8 @@
22

33
using System;
44
using System.Collections.Generic;
5-
using System.Diagnostics.CodeAnalysis;
65
using System.Security.Cryptography;
76
using System.Text;
8-
using System.Text.Json;
97
using Microsoft.Extensions.AI;
108

119
namespace Microsoft.Agents.AI.Foundry.Hosting;
@@ -62,27 +60,31 @@ public static string ComputeWireId(string afRequestId)
6260

6361
/// <summary>
6462
/// Records the wire-id → approval-entry mapping in the supplied state bag.
63+
/// Arguments are passed as already-serialized JSON to keep this method
64+
/// trim/AOT-friendly (no polymorphic <c>object</c> serialization here).
65+
/// No-op when <paramref name="callId"/> or <paramref name="name"/> is empty —
66+
/// without those fields the entry cannot be used to faithfully reconstruct
67+
/// the original <see cref="FunctionCallContent"/> on the inbound side.
6568
/// </summary>
66-
[RequiresUnreferencedCode("FunctionCallContent.Arguments serialization may require types that cannot be statically analyzed.")]
67-
[RequiresDynamicCode("FunctionCallContent.Arguments serialization may require runtime code generation.")]
68-
public static void Record(AgentSessionStateBag? stateBag, string wireId, string afRequestId, FunctionCallContent functionCall)
69+
public static void Record(AgentSessionStateBag? stateBag, string wireId, string afRequestId, string? callId, string? name, string? argumentsJson)
6970
{
7071
if (stateBag is null)
7172
{
7273
return;
7374
}
7475

75-
ArgumentNullException.ThrowIfNull(functionCall);
76+
if (string.IsNullOrEmpty(callId) || string.IsNullOrEmpty(name))
77+
{
78+
return;
79+
}
7680

7781
var map = LoadMap(stateBag);
7882
map[wireId] = new ApprovalEntry
7983
{
8084
AfRequestId = afRequestId,
81-
CallId = functionCall.CallId,
82-
Name = functionCall.Name,
83-
Arguments = functionCall.Arguments is not null
84-
? JsonSerializer.Serialize(functionCall.Arguments)
85-
: null,
85+
CallId = callId!,
86+
Name = name!,
87+
Arguments = argumentsJson,
8688
};
8789
stateBag.SetValue(StateBagKey, map);
8890
}

dotnet/tests/Microsoft.Agents.AI.Foundry.Hosting.UnitTests/InputConverterTests.cs

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -783,6 +783,11 @@ public void ConvertItemsToMessages_McpApprovalRequest_ProducesToolApprovalReques
783783
[Fact]
784784
public void ConvertItemsToMessages_McpApprovalResponse_ThrowsWhenNoMapping()
785785
{
786+
// Without a recorded ApprovalEntry the converter cannot reconstruct the original
787+
// function call faithfully — any placeholder it produced would still fail downstream
788+
// (FICC has no tool to invoke; Azure's stored function_call can't pair with the
789+
// synthetic id). Fail fast with a clear error instead of continuing into a confusing
790+
// HTTP 400 deep inside the agent loop.
786791
var wireId = "mcpr_" + new string('a', 50);
787792
var item = new MCPApprovalResponse(approvalRequestId: wireId, approve: true);
788793

@@ -800,7 +805,9 @@ public void ConvertItemsToMessages_McpApprovalResponse_ResolvesAfRequestIdFromSt
800805
stateBag,
801806
wireId,
802807
AfRequestId,
803-
new FunctionCallContent("call_xyz", "issue_refund", new Dictionary<string, object?> { ["order_id"] = 123 }));
808+
"call_xyz",
809+
"issue_refund",
810+
"{\"order_id\":123}");
804811

805812
var item = new MCPApprovalResponse(approvalRequestId: wireId, approve: false);
806813

@@ -848,7 +855,9 @@ public void ConvertOutputItemsToMessages_McpApprovalResponse_ProducesToolApprova
848855
stateBag,
849856
wireId,
850857
AfRequestId,
851-
new FunctionCallContent("call_history", "delete_file", new Dictionary<string, object?> { ["path"] = "/tmp/x" }));
858+
"call_history",
859+
"delete_file",
860+
"{\"path\":\"/tmp/x\"}");
852861

853862
var item = new OutputItemMcpApprovalResponseResource(
854863
id: "ar_history_id",
@@ -866,21 +875,6 @@ public void ConvertOutputItemsToMessages_McpApprovalResponse_ProducesToolApprova
866875
Assert.Equal("delete_file", fcc.Name);
867876
}
868877

869-
[Fact]
870-
public void ConvertItemsToMessages_McpApprovalResponse_NoMapping_Throws()
871-
{
872-
// Without a recorded ApprovalEntry the converter cannot reconstruct the original
873-
// function call faithfully — any placeholder it produced would still fail downstream
874-
// (FICC has no tool to invoke; Azure's stored function_call can't pair with the
875-
// synthetic id). Fail fast with a clear error instead of continuing into a confusing
876-
// HTTP 400 deep inside the agent loop.
877-
var wireId = "mcpr_" + new string('c', 50);
878-
var item = new MCPApprovalResponse(approvalRequestId: wireId, approve: true);
879-
880-
var ex = Assert.Throws<InvalidOperationException>(() => InputConverter.ConvertItemsToMessages([item]));
881-
Assert.Contains(wireId, ex.Message);
882-
}
883-
884878
[Fact]
885879
public void ConvertItemsToMessages_McpApprovalRequest_MalformedArguments_PreservesRaw()
886880
{
@@ -898,6 +892,28 @@ public void ConvertItemsToMessages_McpApprovalRequest_MalformedArguments_Preserv
898892
Assert.Equal("not valid json", fc.Arguments!["_raw"]?.ToString());
899893
}
900894

895+
[Fact]
896+
public void ToolApprovalIdMap_Record_EmptyCallId_IsNoOp()
897+
{
898+
var stateBag = new AgentSessionStateBag();
899+
var wireId = "mcpr_" + new string('d', 50);
900+
901+
ToolApprovalIdMap.Record(stateBag, wireId, "ficc_x", callId: string.Empty, name: "tool", argumentsJson: "{}");
902+
903+
Assert.Null(ToolApprovalIdMap.ResolveEntry(stateBag, wireId));
904+
}
905+
906+
[Fact]
907+
public void ToolApprovalIdMap_Record_EmptyName_IsNoOp()
908+
{
909+
var stateBag = new AgentSessionStateBag();
910+
var wireId = "mcpr_" + new string('e', 50);
911+
912+
ToolApprovalIdMap.Record(stateBag, wireId, "ficc_x", callId: "call_xyz", name: string.Empty, argumentsJson: "{}");
913+
914+
Assert.Null(ToolApprovalIdMap.ResolveEntry(stateBag, wireId));
915+
}
916+
901917
// ── input_file data-URI decoding (TryDecodeTextDataUri) ──
902918

903919
[Fact]

dotnet/tests/Microsoft.Agents.AI.Foundry.Hosting.UnitTests/OutputConverterTests.cs

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,51 @@ public async Task ConvertUpdatesToEventsAsync_FunctionCallThenResult_EmitsPaired
590590
Assert.IsType<ResponseCompletedEvent>(events[^1]);
591591
}
592592

593+
// K-05: An FCC with an empty CallId is dropped without disturbing in-flight text.
594+
[Fact]
595+
public async Task ConvertUpdatesToEventsAsync_FunctionCallEmptyCallIdMidText_PreservesTextBoundaryAsync()
596+
{
597+
var (stream, _) = CreateTestStream();
598+
var updates = new[]
599+
{
600+
new AgentResponseUpdate { MessageId = "msg_1", Contents = [new MeaiTextContent("Hello, ")] },
601+
new AgentResponseUpdate { Contents = [new FunctionCallContent(string.Empty, "skipped", new Dictionary<string, object?>())] },
602+
new AgentResponseUpdate { MessageId = "msg_1", Contents = [new MeaiTextContent("world!")] },
603+
};
604+
605+
var events = new List<ResponseStreamEvent>();
606+
await foreach (var evt in OutputConverter.ConvertUpdatesToEventsAsync(ToAsync(updates), stream))
607+
{
608+
events.Add(evt);
609+
}
610+
611+
// The FCC is skipped (no CallId), and because we now validate CallId before
612+
// closing the in-flight assistant message, both text deltas land in the same
613+
// output item — only one message-added event is emitted.
614+
Assert.Single(events.OfType<ResponseOutputItemAddedEvent>());
615+
Assert.Equal(2, events.OfType<ResponseTextDeltaEvent>().Count());
616+
Assert.IsType<ResponseCompletedEvent>(events[^1]);
617+
}
618+
619+
// K-06: FRC string results are emitted as raw text on the wire (not JSON-quoted).
620+
[Fact]
621+
public async Task ConvertUpdatesToEventsAsync_FunctionResultStringPayload_EmittedAsRawTextAsync()
622+
{
623+
var (stream, _) = CreateTestStream();
624+
var update = new AgentResponseUpdate { Contents = [new FunctionResultContent("call_1", "sunny")] };
625+
626+
var events = new List<ResponseStreamEvent>();
627+
await foreach (var evt in OutputConverter.ConvertUpdatesToEventsAsync(ToAsync(new[] { update }), stream))
628+
{
629+
events.Add(evt);
630+
}
631+
632+
var added = Assert.Single(events.OfType<ResponseOutputItemAddedEvent>());
633+
var output = Assert.IsType<OutputItemFunctionToolCallOutput>(added.Item);
634+
// String FRC payloads must not be double-encoded — `sunny`, not `"sunny"`.
635+
Assert.Equal("sunny", output.Output.ToString());
636+
}
637+
593638
// L-01
594639
[Fact]
595640
public async Task ConvertUpdatesToEventsAsync_ExecutorInvokedEvent_EmitsWorkflowActionItemAsync()

0 commit comments

Comments
 (0)