fix(handoffs): avoid duplicating forwarded messages in nested handoff history#3791
fix(handoffs): avoid duplicating forwarded messages in nested handoff history#3791Otis0408 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ae4af5304
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not _is_already_summarized(plain_input, summarized_keys): | ||
| new_items_as_inputs.append(plain_input) |
There was a problem hiding this comment.
Preserve repeated current-turn items in nested summaries
When a current handoff turn emits the same serialized item as one already in the flattened history, this check drops it from new_items_as_inputs even though new_items are the current agent's outputs, not forwarded copies from the previous handoff. This loses legitimate repeated turns in downstream summaries; for example, Chat Completions outputs use the constant FAKE_RESPONSES_ID (src/agents/models/chatcmpl_converter.py:172-177), so two agents in a nested handoff chain that both say the same text produce identical keys, and the second agent's message is omitted from the next <CONVERSATION HISTORY> block and from later handoff summaries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, you're right. new_items are produced by the current agent turn, so they can never be forwarded copies of something the summary already records; applying the dedupe to them would silently drop a legitimate current-turn output that happened to serialize identically to an earlier item.
I've limited the dedupe to pre_handoff_items, which is where the verbatim-forwarded copies actually reappear, and added a regression test (test_new_items_are_kept_when_identical_to_summarized_history) that constructs exactly that case: a current-turn message whose serialized form matches an item already in the history. It fails if the dedupe is applied to new_items and passes now.
The nested-handoff-chain duplication test still passes, so the original bug remains fixed.
When nest_handoff_history is enabled, an agent's pre-handoff message is both summarized and forwarded verbatim to the next agent. On the next handoff that verbatim copy arrives as a pre-handoff item and was re-added to the transcript alongside the previous summary that already recorded it, listing the same message twice inside a single <CONVERSATION HISTORY> block. The duplication compounded on every subsequent handoff. Skip transcript items already represented in the flattened summary history.
5ae4af5 to
511e0e2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 511e0e2cf3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not _is_already_summarized(plain_input, summarized_keys): | ||
| pre_items_as_inputs.append(plain_input) |
There was a problem hiding this comment.
Preserve repeated pre-handoff turns in summaries
When a nested handoff agent has an earlier non-handoff turn before it delegates, pre_handoff_items contains that real turn output as well as forwarded copies. If that output serializes identically to any item in flattened_history (for example A says same, then B says same before using a tool and later hands off), this check suppresses B's message from pre_items_as_inputs; because assistant pre-items are also not forwarded raw, the downstream agent never sees B's turn in either the summary or model input. The dedupe needs to distinguish forwarded duplicates from legitimate pre-handoff outputs.
Useful? React with 👍 / 👎.
AmirF194
left a comment
There was a problem hiding this comment.
Nice fix, and the root-cause writeup matches what I see. I reproduced it end-to-end on current main in a clean container: in a 4-agent chain, agent D's <CONVERSATION HISTORY> lists A: msg at records #2 and #5 and B: msg at #6 and #9 (12 records). On this branch each collapses to a single occurrence (10 records) and the chain still hands off, so the forward-verbatim behaviour is preserved and only the in-summary duplicate is dropped.
The part I wanted to be sure of was over-deletion, since _summary_dedupe_key is a content hash that ignores position and occurrence count. I ran the worst case: two different agents each legitimately emitting byte-identical text with the same message id (FakeModel reuses id:"1"). main shows that text 3x (both legit messages plus the spurious forwarded copy); this branch shows it 2x, which is the correct count. It holds because current-turn messages are new_items, which you explicitly exempt, and the only thing that reintroduces a prior message as a pre_handoff_item is verbatim forwarding, so the dedup only ever removes true forwarded copies. With real (distinct) ids the collision surface is even smaller. Flagging it in case anyone else worries about the same thing.
Verification (clean Docker, main vs this branch via PYTHONPATH, provenance checked):
test_nested_handoff_chain_does_not_duplicate_summary_recordsfails onmain(assert 12 == 10) and passes here.- The other tests in
tests/test_handoff_history_duplication.py(14 total) pass on both, so the single-hop "present in summary + forwarded verbatim" behaviour is unchanged. - The handoff selection across the suite is green on this branch (137 passed for me; I skipped voice/models/mcp/etc. for missing optional deps, unrelated to this change).
One small, non-blocking thing: the test docstring cites Issue #2171, which is now closed. If this is residual multi-hop duplication beyond that issue's original fix, a one-line note (or a fresh tracking issue) would help the lineage, since the PR body has no issue link and won't auto-close anything.
I didn't re-run ruff/mypy, so this is behavioural verification only. Not a maintainer, just a careful outside review, leaving the merge call to the team.
|
Thank you for this — the over-deletion case is exactly what I was most worried about, and your byte-identical-text-with-shared-id experiment is a sharper probe than the one I wrote. Glad it holds for the reason you describe: Good catch on the lineage too. #2171 is closed and covered the single-hop case; what's left is the multi-hop residue that only appears from the second handoff onward, which is why the existing single-hop tests never caught it. I've added a "Relation to #2171" section to the PR description spelling that out, and noted that this deliberately doesn't auto-close that issue since it isn't a regression of it. |
Summary
Anyone using
RunConfig(nest_handoff_history=True)(orhandoff(..., nest_handoff_history=True)) with a chain of handoffs — the common triage -> specialist -> specialist pattern — gets a corrupted conversation summary: each agent's own pre-handoff message is listed twice inside the downstream<CONVERSATION HISTORY>block, and the duplication compounds with every additional hop.Root cause: an agent's pre-handoff message is both summarized into the transcript and forwarded verbatim to the next agent (this dual behavior is intentional and covered by existing tests). On the next handoff that verbatim copy arrives as a
pre_handoff_item, andnest_handoff_historyunconditionally re-added every pre/new item to the transcript (history.py) — right next to the flattened previous summary that already contained the same message. So the message appeared as two byte-identical numbered records in a single summary. At each further hop the forwarded copy is re-summarized again, so across a 4-agent chain A's and B's messages each show up twice in D's summary. This feeds the model a repeated transcript and wastes tokens, with the waste growing with chain length.The minimal fix: before adding a pre/new item to the summarized transcript, check whether the flattened history already records an identical item, and skip it if so. Forwarding behavior is unchanged, and the single-hop "message appears in the summary and is forwarded verbatim" behavior is preserved — only the duplicate within the summary is removed.
Before/after using the public API:
Before this fix, agent D's
<CONVERSATION HISTORY>listsA: msgtwice (records #2 and #5) andB: msgtwice (records #6 and #9). After the fix, each intermediate agent's message appears exactly once in the summary.Test plan
Added
tests/test_handoff_history_duplication.py::test_nested_handoff_chain_does_not_duplicate_summary_records, which runs a 4-agent handoff chain withnest_handoff_history=Trueand asserts (a) no two records inside the final agent's<CONVERSATION HISTORY>block are identical and (b) each intermediate agent's message appears exactly once in the summary.src/agents/handoffs/history.pytomainmakes it fail (12 == 10, two duplicate records).pytest tests/ -k handoff-> 198 passed. This includes the existing single-hop tests that assert the current-turn message is present in the downstream summary, which remain unchanged.ruff format,ruff check, andmypy src/agents/handoffs/history.pyare clean.Issue number
Checks
.agents/skills/code-change-verification/scripts/run.shRelation to #2171
Issue #2171 (now closed) covered the single-hop case: a message appearing both in the summary and forwarded verbatim to the immediate next agent. That fix is intact and its tests still pass unchanged.
What remains, and what this PR addresses, is the multi-hop residue: on the second and later handoffs the forwarded copy arrives as a
pre_handoff_itemand gets re-summarized alongside the flattened prior summary that already contains it, so the duplication compounds with chain length. This is why the existing single-hop tests never caught it. The new test exercises a 4-agent chain rather than a single handoff.Since this is residual behaviour beyond #2171's original scope rather than a regression of it, the PR intentionally does not auto-close that issue.