[Shivam Sharma (ShivamSharma43)]

Motivation & Context

With ToolApprovalMiddleware, approval decisions collected across separate AG-UI resumes are released as one batch. Both calls execute and both TOOL_CALL_RESULT events are streamed, but the MESSAGES_SNAPSHOT and the persisted thread snapshot keep only the result whose approval arrived in the final request. The next user turn then replays both calls with only one result, leaving an orphaned call in history.

Description & Review Guide

• What are the major changes?
  • At the end of _run_agent_stream, native approval results are removed from flow.tool_results on the assumption that _merge_resolved_approval_results_into_snapshot restores them from the request's messages. Results only reach messages by being matched to an approval response in the current request, so a decision collected in an earlier request had no match and its result was dropped from the snapshot.
  • The runner now records each native approval result as it streams and passes any that are missing from messages into the existing merge, so each result is persisted directly after the assistant message that issued its call, in call order.
  • The change is limited to the AG-UI runner. The core function-calling loop, approval lifecycle, and execution behavior are unchanged; results whose approval is in the current request follow the same path as before.
  • Added test_endpoint_queued_approval_snapshot_retains_every_released_result, which drives the queued two-approval flow through the endpoint with a snapshot store and checks the streamed snapshot, the persisted snapshot, result order, and next-turn replay. It fails on main with ['call_second']. _build_tool_approval_queue_endpoint gained an optional snapshot_store for it.
• What is the impact of these changes? Threads using queued tool approvals with a snapshot store keep every completed tool result, so later turns no longer replay orphaned calls.
• What do you want reviewers to focus on? Whether carrying these results into _merge_resolved_approval_results_into_snapshot is the right seam, versus folding them into messages when the result streams. I chose the former so the in-flight messages list is not changed while the run is streaming.

This preserves the documented "AG-UI approval result event: approved result emits once with content and persists in snapshot" behavior from docs/specs/004-python-function-calling-loop.md, so the spec is unchanged.

Related Issue

Fixes #8789

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
• This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gomission.ai (gomission)]

Thanks for picking up #8789. I independently reran the unchanged reproduction against 1b7ef230: the original six-case matrix goes from 3 failed / 3 passed on 218700d4 to 6 passed on this head. The original queued-approval result-loss case is repaired in that check.

I also confirmed the separate reused-provider-ID case raised by Evan Mattson (@moonbox3) in this review. Extending the existing three-occurrence endpoint fixture to inspect the emitted snapshot, saved snapshot and next-turn provider input gives:

Observation	Results
Tool executions and TOOL_CALL_RESULT events	all three, once each
Final MESSAGES_SNAPSHOT	second safe result only
Persisted snapshot	second safe result only
Next-turn function results	second safe result only

So my original distinct-ID reproduction passes, while the occurrence-aware regression remains open. These are local offline checks with the in-memory snapshot store; I haven't verified a deployed backend. Thanks for carrying the endpoint regression into the fix.

[Eduard van Valkenburg (eavanvalkenburg)]

/review

[github-actions]

MAF Automated Review — Iteration 1

Result: No findings
Scope: full PR (1 commit(s)): 1b7ef23054a5
Model: gpt-5.6-sol

Overview

The change records native queued approval results during streaming and carries only results absent from the current messages into the existing snapshot merge. Collection remains limited to lifecycle-authorized, non-hosted in-run results, while the new regression covers streamed snapshot order, persisted history, and next-turn replay. No additional publishable issue was established after reconciliation and deduplication against the supplied existing feedback.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

[Shivam Sharma (ShivamSharma43)]

Evan Mattson (@moonbox3) I addressed your occurrence-aware dedup point in 439f366: results are now grouped per call id in resolution order, and the carry-over filter matches by result object instead of call_id. The three-occurrence provider-shared case now keeps every result in the snapshot (test_endpoint_approval_snapshot_keeps_every_result_for_reused_call_id). I've also merged the latest main and CI is green. Could you take another look when you get a chance? Thanks!

[gomission.ai (gomission)]

I rechecked bfbd62c1 with the unchanged three-occurrence reproduction from my previous follow-up. The new occurrence-aware merge fixes the emitted and persisted snapshots, but the next-turn history still loses two results:

Observation	Result
Tool executions / TOOL_CALL_RESULT events	All three, once each
Final MESSAGES_SNAPSHOT	All three results, in order
Persisted snapshot	All three results, in order
Next user turn: messages received by the stub provider	guarded result only

Both new endpoint regressions pass locally (queued_approval_snapshot_retains_every_released_result and approval_snapshot_keeps_every_result_for_reused_call_id). My additional next-turn assertion fails. The current reused-ID test stops after reading the saved snapshot; extending the same fixture with a new user turn and capturing stream_fn's messages exposes the remaining loss.

The smaller reproduction is in the existing history sanitizer: _ordered_unique_tool_call_ids collapses three occurrences to one pending ID, then _sanitize_tool_history consumes that ID for the first result and drops the later tool messages. Distinct IDs retain all three results:

from agent_framework import Content, Message
from agent_framework_ag_ui._message_adapters import _sanitize_tool_history

for ids in [("shared",) * 3, ("call-a", "call-b", "call-c")]:
    messages = [Message(role="assistant", contents=[
        Content.from_function_call(
            id=f"occurrence-{n}", call_id=cid,
            name=f"tool_{n}", arguments="{}",
        ) for n, cid in enumerate(ids)
    ])]
    messages += [Message(role="tool", contents=[
        Content.from_function_result(call_id=cid, result=f"result-{n}")
    ]) for n, cid in enumerate(ids)]
    print([c.result for m in _sanitize_tool_history(messages)
           for c in m.contents if c.type == "function_result"])
# Shared ID: ['result-0']
# Distinct IDs: ['result-0', 'result-1', 'result-2']

This identifies a remaining replay boundary in the existing sanitizer; I am not claiming this PR introduced it. No competing patch from me. The snapshot correction itself is verified in these cases.

Environment: Python 3.12.11, core/AG-UI/orchestrations imported from this PR checkout, ag-ui-protocol==0.1.22, fastapi==0.141.1. Local offline endpoint calls and an in-memory snapshot store; no model calls or deployed backend tested. Reproductions executed with Codex for Mission. Original occurrence scenario credited to Evan Mattson (@moonbox3).

[Shivam Sharma (ShivamSharma43)]

gomission.ai (@gomission) thanks for the precise repro. 07c6c69 makes the replay path occurrence-aware: _sanitize_tool_history now tracks one pending entry per function call (so your shared-ID case keeps result-0/1/2), and _deduplicate_messages had the same collapse — it now keeps one result per declared occurrence. Going end-to-end, there's one more layer: _emit_tool_call treats consecutive same-call_id calls as argument chunks of one call, so the stream and snapshot carry a single tool call before the three results, and the next turn still replays one call. Since AG-UI identifies tool calls by toolCallId, fixing that changes stream emission rather than this snapshot merge. Evan Mattson (@moonbox3) would you prefer that in this PR or as a follow-up?

[gomission.ai (gomission)]

Retested exact head e016689982d0b0a6e2f7bdac2f1c22911d7c555d with the unchanged three-occurrence reproduction from my October 6 check.

The smaller sanitizer example I posted there now retains result-0/1/2 for the shared-ID case, and _deduplicate_messages preserves all three afterward. The distinct-ID control also passes. The three new cross-turn/abandoned-call regressions pass, along with the queued-approval snapshot regression and existing hidden-sibling execution regression: 5 passed.

The unchanged endpoint reproduction still fails only at its next-turn assertion: all three tools execute once; all three results are streamed and retained in the emitted and persisted snapshots; the next model turn receives only guarded result. Tracing the adapter shows one call declaration and three results entering _sanitize_tool_history, then one call and one result leaving it. This matches the remaining stream-declaration boundary you already identified. Your normalizer fixes are independently verified when all three declarations are present.

Leaving the same-batch stream-emission scope decision with the reviewers. This is a verification follow-up; no competing implementation from me.

Environment: Python 3.12.11 on macOS arm64; core/AG-UI/orchestrations imported from this PR checkout; ag-ui-protocol==0.1.22, fastapi==0.141.1, pytest==9.1.1. Offline stub-provider endpoint checks with the in-memory snapshot store; no full-suite, live-provider or deployed-backend validation. Checks run with Codex for Mission; original occurrence scenario credited to Evan Mattson (@moonbox3).

[Shivam Sharma (ShivamSharma43)]

5647ee9 fixes the Test Typing Checks failure. It came from a test I added in d1d99cd, which iterated tool_calls without narrowing it from Any | None. All five test checkers (mypy, zuban, pyrefly, ty, pyright) now pass on test_endpoint.py.

gomission.ai (@gomission) thanks for re-verifying e0166899.

On the remaining same-batch case: when hidden siblings share a call ID within one assistant turn, _emit_tool_call treats them as argument chunks of one call. The stream and snapshot then declare a single call ahead of three results. Fixing that changes how tool calls are streamed, not how the snapshot is merged. I'd like to open it as a separate issue and PR so this one stays focused on the snapshot and replay fixes. Evan Mattson (@moonbox3) Eduard van Valkenburg (@eavanvalkenburg) if you'd rather I handle it here, I'm happy to add it.