fix(handoffs): filter duplicate items from model input when nest_handoff_history is enabled (#2171) - #1
Conversation
…off_history is enabled (openai#2171)
📝 WalkthroughWalkthroughThe changes introduce session history tracking that separates full event items from filtered items sent to the model. New Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agents/_run_impl.py (1)
245-270: Filtered handoff items are reintroduced into the next model call viagenerated_itemsThe wiring of
HandoffInputData.input_itemsandsession_step_itemsinexecute_handoffscorrectly separates concerns in isolation:
- For custom filters with
input_itemsset, you correctly:
- Keep
filtered.new_itemsas the full set for history (session_step_items),- Use
filtered.input_itemsasnew_step_items(filtered for the next model).- For nested history, you:
- Collapse history into
nested.input_history,- Keep full
nested.new_itemsinsession_step_items,- Use
nested.input_items(filtered) asnew_step_itemswhen available.However, the implementation has a critical flaw: because
SingleStepResult.generated_itemspreferssession_step_itemswhen set, andAgentRunnerfeedsgenerated_itemsdirectly into the next turn's model input, the filtering is bypassed:
In
AgentRunner.run(line 704) and_start_streaming(line 1216), you set:generated_items = turn_result.generated_itemsThen in
_run_single_turn(line 1623–1624) and the streaming flow (line 1406–1407), you construct the next model input as:input.extend([generated_item.to_input_item() for generated_item in generated_items])When
session_step_itemsis populated (i.e., whenever filtering or nesting is active),generated_itemscontains the unfiltered items (includingToolCallOutputItem/HandoffOutputItem), not the filterednew_step_itemsyou carefully computed. As a result, the next agent still sees rawfunction_call_output/HandoffOutputItemin its prompt, even thoughinput_itemsornest_handoff_historywere meant to prevent exactly that.The tests in
test_handoff_history_duplication.pyverify thenest_handoff_historyfunction's output in isolation but do not exercise the fullAgentRunnerloop, so this reintroduction of unfiltered items is not caught.Suggested fix: Separate the concepts explicitly. Use
session_step_itemsstrictly for session persistence and final result representation, while deriving the next-turn input from the filtered path:
- In the run loop, maintain two accumulators:
history_items = turn_result.generated_items(full history, usessession_step_items),input_items_for_next_turn = turn_result.pre_step_items + turn_result.new_step_items(filtered).- Pass
input_items_for_next_turnto_run_single_turnand_run_single_turn_streamedfor model input construction, while keepinghistory_itemsfor the finalRunResult.new_itemsand streaming observability.A similar split is needed in
_start_streamingwherestreamed_result.new_itemsis currently set toturn_result.generated_items.
🧹 Nitpick comments (3)
src/agents/handoffs/history.py (1)
29-35: Handoff history now cleanly separates summary-only vs forwarded itemsThe new
_SUMMARY_ONLY_INPUT_TYPESplus_should_forward_pre_item/_should_forward_new_itemgivenest_handoff_historya clear policy:
- Pre‑handoff assistant messages and any
function_call/function_call_outputitems are summarized intoinput_historyand removed frompre_handoff_items.- New items are always summarized but only forwarded via
input_itemswhen they either:
- Have a
role(user/assistant/system/developer), or- Are non‑function tools (e.g., shell, MCP outputs).
new_itemsremains untouched for session history, whileinput_itemsprovides a deduplicated view for the next agent. This matches the tests intests/test_handoff_history_duplication.pyand the stated goal of avoiding duplicate function tool calls/outputs in model input while retaining observability via the summary.Also applies to: 68-105, 252-268
tests/test_handoff_history_duplication.py (1)
1-276: New handoff history duplication tests accurately covernest_handoff_historybehaviorThis suite does a good job of pinning down the intended semantics:
- Tool
function_call/function_call_outputentries inpre_handoff_itemsare removed from the forwarded sets and only appear inside the summary.HandoffOutputItemis preserved innew_itemsfor session history but filtered out ofinput_itemsso it doesn’t hit the next model call twice.- Message items with a
roleare preserved in bothnew_itemsandinput_items.- The summary text is asserted to contain the filtered tool activity, keeping observability.
- The final scenario bounds the effective model input to “summary + messages” and explicitly checks that no
ToolCallOutputItem/HandoffOutputIteminstances appear in what’s treated as model input.These tests nicely lock in the new
HandoffInputData.input_itemsandnest_handoff_historycontracts; adding a couple of Runner-level end-to-end tests in the future (driving an actual handoff withnest_handoff_history=True) would further ensure the full pipeline respects these guarantees.tests/test_agent_runner_streamed.py (1)
234-234: Add detailed breakdown to the assertion comment.The expectation of 8 items appears correct, but the comment lacks detail about the composition. Consider adding a breakdown similar to lines 178-180 for clarity:
assert len(result.to_input_list()) == 8, ( "should have 8 items: summary (contains 2 original inputs + pre-handoff tool call/result), " "message, handoff call, handoff output, preamble message, tool call, tool result, final output" )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/agents/_run_impl.pysrc/agents/handoffs/__init__.pysrc/agents/handoffs/history.pysrc/agents/run.pytests/test_agent_runner.pytests/test_agent_runner_streamed.pytests/test_handoff_history_duplication.pytests/test_soft_cancel.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/test_agent_runner_streamed.py (1)
src/agents/result.py (1)
to_input_list(125-130)
tests/test_handoff_history_duplication.py (3)
src/agents/handoffs/__init__.py (1)
HandoffInputData(42-82)src/agents/handoffs/history.py (1)
nest_handoff_history(68-105)src/agents/items.py (1)
ToolCallOutputItem(257-287)
tests/test_agent_runner.py (1)
src/agents/result.py (1)
to_input_list(125-130)
src/agents/handoffs/history.py (1)
src/agents/handoffs/__init__.py (1)
clone(72-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (7)
tests/test_soft_cancel.py (1)
423-429: Use of"handoff_requested"event name matches streaming semanticsSwitching the check to
event.name == "handoff_requested"aligns this test withRunImpl.stream_step_items_to_queue, which emits"handoff_requested"forHandoffCallItem. This better reflects “handoff has been invoked” for soft-cancel timing.src/agents/handoffs/__init__.py (1)
41-71: Newinput_itemsfield is well-scoped and backwards compatibleThe
input_items: tuple[RunItem, ...] | None = Noneaddition cleanly distinguishes “items for next agent input” fromnew_itemsused for history, and the default keeps existing filters working. The docstring clearly describes the contract and how it’s meant to be used by handoff filters and history nesting.src/agents/run.py (1)
1703-1755: Streaming now preferssession_step_itemsfor observability (LGTM)Using
streaming_items = ( single_step_result.session_step_items if single_step_result.session_step_items is not None else single_step_result.new_step_items )and then removing items already present in
new_items_processed_responsebefore enqueueing gives the streaming path a consistent, de‑duplicated view of all per‑turn items, while still respecting the new session/history separation introduced in_run_impl.This change is internally consistent with the new
session_step_itemssemantics; the remaining question of how those semantics impact next‑turn model input is covered in the_run_impl.pycomment.tests/test_agent_runner.py (1)
110-140: Updatedto_input_list()expectations match new summary-based historyThe revised
len(result.to_input_list())assertions and messages in:
test_tool_call_runs(5 items: original input, first message, tool call, tool result, final message),test_handoffs(5 items: summary message + message + handoff call + handoff output + final message),test_structured_output(8 inputs including the nested summary),accurately reflect the new behavior where prior turns are collapsed into a summary and only key recent items are carried forward explicitly.
Also applies to: 173-178, 227-230
tests/test_agent_runner_streamed.py (3)
737-738: The original review identifies an apparent inconsistency that does not exist. The code is actually consistent and the comment on line 737 clearly explains the intended behavior:
- Line 717–720 shows that
to_input_list()contains 7 items, including "handoff output" (as stated in the comment)- Line 737–738 correctly sets
"handoff_output": 0in the stream events- The comment on line 737 explains: "handoff_output is summarized in conversation history, not duplicated as raw item"
This means the handoff_output information is included in
to_input_list()as part of the conversation summary (item #1), but it does not appear as a separaterun_item_stream_eventitem. This is the intended deduplication behavior and is clearly documented. No clarification or revision needed.Likely an incorrect or invalid review comment.
717-720: Verify that theto_input_list()count of 7 items is correct.The test assertion expects 7 items, described as: conversation summary (contains pre-handoff tool calls), message, handoff, handoff output, tool call, tool call result, final output. Based on the code structure,
to_input_list()returns original_items (2 input messages) concatenated with new_items generated during the run. Pre-handoff tool calls and outputs are summarized per the handoff history logic (lines 29-34 insrc/agents/handoffs/history.py), but the exact composition of new_items in the final result requires runtime verification to confirm the count is accurate.Run the test to verify:
python -m pytest tests/test_agent_runner_streamed.py::test_streaming_events -xvs
178-181: The assertion count of 5 items is correct. The session items are: summary message (containing original input and pre-handoff tool call/result from turn 1), message from turn 2, handoff call, handoff output, and done message from agent_1.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Greptile Summary
This PR fixes issue openai#2171 where
function_callandfunction_call_outputitems were being duplicated in the model input during agent handoffs whennest_handoff_historyis enabled.Key changes:
input_itemsfield inHandoffInputDatato separate filtered model input from complete session historysession_step_itemsfield inSingleStepResultto preserve unfiltered items for session saving and observabilitynest_handoff_history()that converts function calls to summary text while filtering them from direct model inputtest_handoff_history_duplication.py) covering various scenariosThe fix ensures agents receive concise summarized context during handoffs rather than raw duplicate tool calls, while maintaining full observability through session history.
Confidence Score: 5/5
Important Files Changed
input_itemsfield to HandoffInputData to separate model input from session historySequence Diagram
sequenceDiagram participant User participant Agent1 participant Model participant HandoffLogic participant Agent2 User->>Agent1: Initial request Agent1->>Model: Process input Model->>Agent1: Call tool (function_call) Agent1->>Agent1: Execute tool Agent1->>Agent1: Generate function_call_output Agent1->>Model: Tool result Model->>Agent1: Handoff to Agent2 (function_call) Note over Agent1,HandoffLogic: Handoff occurs Agent1->>HandoffLogic: Create HandoffInputData<br/>(pre_handoff_items + new_items) alt nest_handoff_history enabled HandoffLogic->>HandoffLogic: nest_handoff_history() Note over HandoffLogic: Filter function_call & function_call_output<br/>from pre_handoff_items and new_items HandoffLogic->>HandoffLogic: Create summary with filtered items as text HandoffLogic->>HandoffLogic: Set input_items (filtered for model) HandoffLogic->>HandoffLogic: Keep new_items unchanged (for session) end HandoffLogic->>Agent2: Pass filtered input_items to model Note over Agent2: Receives summary + messages only<br/>(no duplicate function_call/output) HandoffLogic->>HandoffLogic: Save full new_items to session Note over HandoffLogic: Session history preserves all items<br/>for observability Agent2->>Model: Continue with filtered input Model->>Agent2: Generate response Agent2->>User: Final output