.NET: Issue 5662#5668
Open
alliscode wants to merge 7 commits intomicrosoft:mainfrom
Open
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 88%
✓ Correctness
The PR replaces a broken placeholder FunctionCallContent in MCP approval responses with a faithfully reconstructed one (preserving CallId, Name, Arguments from the original request), and suppresses FunctionCallContent at the wire to prevent orphan function_call items. The logic is internally consistent: Record stores the details, ResolveEntry retrieves them, and the reconstructed FCC matches what the backend expects. Tests are updated to reflect the new behavior. No correctness issues found.
✓ Security Reliability
The changes replace a fallback-to-wire-id pattern with a fail-fast approach when no approval mapping exists, and suppress FunctionCallContent from wire output to prevent orphan function_call items. The code is well-structured from a security and reliability perspective: the state bag is server-side (not client-controllable), SHA-256 prevents wire-id collision attacks, deserialization failures are defensively handled, and the exception message with the wire id poses no injection risk in .NET's exception handling model. No security or reliability issues identified.
✓ Test Coverage
The PR updates tests comprehensively for both the FunctionCallContent wire suppression and the richer ToolApprovalIdMap round-trip. However, there is a clearly duplicated test (same scenario tested twice with trivially different wire ids), and edge cases for null arguments in the approval entry and the defensive JsonException path in TryLoadMap are not exercised.
✗ Design Approach
I found two design-level problems. First, the new approval map persists a custom
Dictionary<string, ApprovalEntry>directly intoAgentSessionStateBag, but that bag serializes values using the framework's default JSON metadata, which only source-generates a fixed set of known exchange types; in AOT/no-reflection configurations this makes any session containing an approval request unserializable. Second, the new hard failure on missing approval mappings turns pre-existing or state-lost approval requests into top-level request failures, even though the newTryLoadMappath explicitly expects older-format state and drops it.
Automated review by alliscode's agents
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Foundry Hosting wire converters to (1) suppress internal FunctionCallContent events from being emitted to the Responses API stream and (2) persist enough tool-call details during mcp_approval_request emission to reconstruct a lossless ToolApprovalResponseContent when the corresponding mcp_approval_response arrives on a later HTTP turn (issue #5662).
Changes:
- Suppress
FunctionCallContent(and related function-call wire events) inOutputConverterto prevent orphanedfunction_callitems and subsequent resume failures. - Persist a full approval mapping entry (AF request id + original function call details) in session state and reconstruct the original
FunctionCallContenton inbound approval responses. - Update/add unit tests to validate suppression behavior and lossless reconstruction, and to enforce fail-fast behavior when approval mappings are missing.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/OutputConverter.cs | Stops emitting function-call wire items and records full approval mappings when emitting mcp_approval_request. |
| dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/InputConverter.cs | Reconstructs ToolApprovalResponseContent using stored mapping entries and throws when the mapping is missing. |
| dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/ToolApprovalIdMap.cs | Changes stored state from wire-id→string to wire-id→approval entry (includes call id/name/args) and adds entry resolution helpers. |
| dotnet/tests/Microsoft.Agents.AI.Foundry.Hosting.UnitTests/OutputConverterTests.cs | Updates expectations to ensure function-call items are suppressed at the wire and adjusts output-item counts accordingly. |
| dotnet/tests/Microsoft.Agents.AI.Foundry.Hosting.UnitTests/OutputConverterWorkflowTests.cs | Updates workflow test expectations to account for suppressed function-call wire items. |
| dotnet/tests/Microsoft.Agents.AI.Foundry.Hosting.UnitTests/InputConverterTests.cs | Adds/updates tests for lossless tool-call reconstruction and missing-mapping failure behavior. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 1
alliscode
pushed a commit
to alliscode/agent-framework
that referenced
this pull request
May 6, 2026
Stop swallowing JsonException in ToolApprovalIdMap.TryLoadMap. The catch block recovered to an empty map and a stale comment claimed the caller would gracefully degrade via a 'wire-id fallback path' — but that path no longer exists: InputConverter.ConvertMcpApprovalResponse fails fast when no entry is found. Letting the JsonException propagate produces an error message that points at the actual cause (a state-bag format incompatibility), instead of converting it into a confusing 'no approval mapping recorded' InvalidOperationException one stack frame later. Refs microsoft#5662, PR microsoft#5668 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
alliscode
pushed a commit
to alliscode/agent-framework
that referenced
this pull request
May 6, 2026
Stop swallowing JsonException in ToolApprovalIdMap.TryLoadMap. The catch block recovered to an empty map and a stale comment claimed the caller would gracefully degrade via a 'wire-id fallback path' — but that path no longer exists: InputConverter.ConvertMcpApprovalResponse fails fast when no entry is found. Letting the JsonException propagate produces an error message that points at the actual cause (a state-bag format incompatibility), instead of converting it into a confusing 'no approval mapping recorded' InvalidOperationException one stack frame later. Refs microsoft#5662, PR microsoft#5668 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…icrosoft#5662) Make the wire<->AF approval translation in Microsoft.Agents.AI.Foundry.Hosting lossless so the resume turn pairs function_call/function_call_output correctly. Root cause: InputConverter.ConvertMcpApprovalResponse rebuilt FunctionCallContent with CallId set to the FICC-composed AF request id (ficc_<callId>) and Name hardcoded to 'mcp_approval'. This (a) broke Azure Conversations pairing because the persisted function_call had CallId <callId> without prefix, and (b) made FICC unable to invoke the original tool by name on resume. Fix: ToolApprovalIdMap now records the original FunctionCallContent (CallId, Name, Arguments) keyed by wire id at outbound time. InputConverter reconstructs the original FCC on inbound, falling back to the legacy placeholder when no mapping exists. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Foundry-Hosting's OutputConverter was emitting FunctionCallContent as wire `function_call` items while dropping the paired FunctionResultContent. The result: every auto-invoked tool call left an orphan `function_call` in the response store. The next turn (chained via previous_response_id or via a workflow that yields after one turn under externalLoop) reloaded that history and submitted it to Azure Conversations, which rejected it with HTTP 400 `No tool output found for function call ...`. Function call/result pairs are entirely internal to the agent's tool-calling loop and have no place on the wire. Approval-required calls already surface separately via ToolApprovalRequestContent → mcp_approval_request, so dropping FCC is safe. FCC's message-close behavior is preserved so pre-tool text doesn't accidentally concatenate with post-tool text under the same MessageId. Existing OutputConverter tests asserting FCC wire emission are updated to assert suppression. Verified end-to-end against the declarative-workflow-menu external_loop bench: three-turn previous_response_id chain (menu → carbonara price → EXIT) now completes, where it previously failed at turn 2 with HTTP 400. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous best-effort placeholder fallback in InputConverter.ConvertMcpApprovalResponse couldn't actually round-trip — it just delayed and obscured the failure as an HTTP 400 deep inside the agent loop. Throw InvalidOperationException with the wire id and a clear cause hint instead so the failure is local and actionable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
microsoft#5662) Replace blanket FCC suppression with deferred emission. FunctionCallContent is buffered (name + serialized arguments) keyed by CallId; the function_call and function_call_output wire items are only flushed once the matching FunctionResultContent arrives. - Auto-invoked FCC/FRC pairs surface as paired wire items so Azure's stored conversation has matched call+output and previous_response_id resume works (closes the orphan-function_call symptom from microsoft#5662). - Orphan FCCs (e.g. workflow paused at a checkpoint mid-tool-loop) are dropped so they never poison the response store. - Approval flows are unchanged: TARC still emits mcp_approval_request and the post-approval FRC has no buffered FCC to pair with so it is dropped; the approval round-trip handles its own pairing via mcp_approval_*. - Leaves the door open for future client-side function calling: that pattern would surface an FCC without an FRC, would need to opt out of buffering, but the wire shape is already correct. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the deferred-emission/buffer-and-drop strategy with direct emission of both function_call and function_call_output wire items. Rationale: a lone FunctionCallContent in OutputConverter's input can mean two semantically different things, and only the caller knows which: - Auto-invoke (FICC response surface): always paired with a matching FRC; both halves should appear on the wire as historical record. - HITL / port-pause request (typed RequestPort<FunctionCallContent,...> or workflow synthesizing a request): a lone FCC IS the wire signal that the caller must resume by supplying a function_call_output. Buffering+dropping orphans silently swallows the second case. Emitting both directly is the only correct shape for OpenAI Responses semantics. The InputConverter already accepts function_call_output and mcp_approval_response on resume, so the round-trip works for both kinds. The approval-flow round-trip fixes (ToolApprovalIdMap rich ApprovalEntry, fail-fast on missing mapping in ConvertMcpApprovalResponse) remain intact. Tests: updated 7 OutputConverter tests + 1 OutputConverterWorkflow test that asserted the old buffer/drop semantics; all 227 tests pass. Refs microsoft#5662 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop swallowing JsonException in ToolApprovalIdMap.TryLoadMap. The catch block recovered to an empty map and a stale comment claimed the caller would gracefully degrade via a 'wire-id fallback path' — but that path no longer exists: InputConverter.ConvertMcpApprovalResponse fails fast when no entry is found. Letting the JsonException propagate produces an error message that points at the actual cause (a state-bag format incompatibility), instead of converting it into a confusing 'no approval mapping recorded' InvalidOperationException one stack frame later. Refs microsoft#5662, PR microsoft#5668 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
peibekwe
reviewed
May 6, 2026
peibekwe
approved these changes
May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request makes significant improvements to how tool approval requests and responses are handled, ensuring that function call details are preserved and correctly reconstructed across HTTP turns. It also updates the logic to suppress internal function call events from being emitted on the wire, preventing orphaned function calls and related errors. Comprehensive tests are added and updated to validate these behaviors.