Repository navigation
Python: Fix code interpreter deltas dropped on prefix collision - #8957
Sadok Barbouche (cr-sbarbouche) wants to merge 9 commits into
Conversation
…text _merge_content_item_lists decided whether to concatenate two streamed code_interpreter chunks or replace the accumulated text with a new one by checking whether one chunk's text is a string-prefix of the other. That guess exists to handle providers that resend the complete code once streaming finishes (the OpenAI Responses API's code_interpreter_call_code.done event repeats the full code after a run of .delta events for the same call), but it ran on every merge, including between two ordinary deltas. Whenever two genuine deltas happened to line up as a prefix of each other, one got silently discarded. This is not an edge case for code: repeated indentation, a closing paren right after an opening one, a short number followed by a longer one starting with the same digit, all collide with this heuristic. Nothing raised, the result was just quietly wrong. Replace the guess with an explicit marker. A chunk is only treated as a full resend when its additional_properties carries the new _CONTENT_ITEM_SNAPSHOT_KEY; everything else concatenates. The OpenAI client now sets that marker on the done event's content instead of leaving the merge layer to infer it from text. Fixes microsoft#8956
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Empty content lists can now raise IndexError, and the new cross-package import is incompatible with an allowed core version.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Fixes streamed code-interpreter content corruption by distinguishing explicit snapshots from incremental deltas.
Changes:
- Adds snapshot-aware content merging.
- Tags OpenAI completion events as snapshots.
- Adds regression and integration tests.
| File | Description |
|---|---|
python/packages/core/agent_framework/_types.py |
Implements snapshot-aware merging. |
python/packages/core/tests/core/test_types.py |
Tests prefix collisions and snapshots. |
python/packages/core/tests/core/test_sessions.py |
Updates session coalescing coverage. |
python/packages/openai/agent_framework_openai/_chat_client.py |
Tags completed code as a snapshot. |
python/packages/openai/tests/openai/test_openai_chat_client.py |
Verifies OpenAI snapshot tagging. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Drop the openai package's cross-package import of the new core marker constant and redeclare it locally instead, matching how _MODEL_OUTPUT_KIND_KEY is already duplicated in that file. agent-framework-openai's pyproject.toml floors agent-framework-core at the current release, which doesn't carry the new symbol yet, so importing it directly would break that pinned combination. Guard _merge_content_item_lists against an empty existing or incoming list instead of indexing into it unconditionally. A code interpreter call can open with an empty inputs/outputs placeholder before any text streams in, and the removed prefix checks used to short-circuit that case by accident. Fix a comment that miscounted characters vs. chunks in the snapshot-marker example.
GitHub Actions
Inactive
Stop leaking the merge-only snapshot marker into finalized content. _merge_content_item_lists returned the tagged chunk as-is once it recognized a full resend, so _CONTENT_ITEM_SNAPSHOT_KEY stuck around in additional_properties after the merge was done, where to_dict() would persist it into history and middleware would see it. Pop the key off the copy before returning it; it has no meaning past this one merge step. Also recognize the OpenAI Responses API's code_interpreter_call_code.done event from its raw event type as a second, independent snapshot signal. agent-framework-openai's published releases stream that event without the marker, and their agent-framework-core floor allows installing this core version, so without this fallback, upgrading only core would turn a streamed delta followed by that provider's done event into duplicated code instead of a correct replace.
GitHub Actions
Inactive
GitHub Actions
Inactive
The previous stripping only ran inside _merge_content_item_lists, which never runs for a call_id's first occurrence - _coalesce_code_interpreter_content deep-copies that one directly and stores it in seen without going through any merge function. A call that streams a single tagged snapshot and no prior deltas hit exactly that path, so the marker still reached the finalized content. Factored the stripping into a shared _strip_snapshot_marker helper used by both the first-occurrence copy in _coalesce_code_interpreter_content and the merge path in _merge_content_item_lists, including its own existing-is-None early return, which had the same gap for a field (inputs or outputs) streaming its first chunk as a snapshot.
GitHub Actions
Inactive
_is_content_item_snapshot recognized a code_interpreter_call_code.done event by its raw event type string as well as the explicit marker, to keep working with agent-framework-openai releases published before this fix. That put OpenAI wire-protocol knowledge directly in the provider-agnostic merge code in _types.py, which core should not carry. Back to checking only the explicit _CONTENT_ITEM_SNAPSHOT_KEY marker. A pre-fix agent-framework-openai paired with this core no longer gets the done event recognized as a snapshot, so that specific old-client/new-core combination can duplicate code again; providers that set the marker (including the current agent-framework-openai, already updated in this PR) are unaffected.
GitHub Actions
Inactive
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Sadok Barbouche (@cr-sbarbouche) The final response.output_item.done path still emits item.code without the new authoritative-completion marker. In a normal stream with deltas, code_interpreter_call_code.done, and then output_item.done, the final full code is appended to the already completed snapshot, duplicating code in the final response and session history. This path needs the same completion handling and a full-event-sequence regression.
response.output_item.done never had a code_interpreter_call branch, unlike every sibling item type (shell_call, computer_call, image_generation_call, mcp_call, web_search_call) that gets its full state captured there. There is no incremental event for a code interpreter call's outputs (logs/images), so this meant streamed code interpreter results were silently always empty - outputs only ever got set from response.output_item.added, which fires before the call has run. Add that branch, reusing the same item-to-contents conversion already used by the non-streaming parser and response.output_item.added (now extracted into one shared helper instead of three near-identical copies). The done item's code is tagged as a snapshot like code_interpreter_call_code.done's is, since a normal stream sends both for the same call and the merge needs to recognize the second one as a repeat of the authoritative value rather than concatenate onto the first.
…delta-prefix-collision # Conflicts: # python/packages/core/tests/core/test_types.py # python/packages/openai/agent_framework_openai/_chat_client.py
GitHub Actions
Inactive
|
Eduard van Valkenburg (@eavanvalkenburg) re the output_item.done review: I dug into this carefully since I couldn't reproduce duplication with the code as it stood - turned out output_item.done had no code_interpreter_call branch at all, unlike every sibling item type (shell_call, computer_call, mcp_call, etc). So there was no duplication today, but a worse pre-existing gap: outputs (logs/images) were silently always empty for streamed code interpreter calls, since the only other place they're set is output_item.added, before the call has even run. Added the missing branch in 9c00bbb, reusing one shared helper now used by the non-streaming parser and both streaming handlers, with the done item's code tagged as a snapshot exactly like code_interpreter_call_code.done's is - so a stream that sends both doesn't duplicate. Verified both the original gap and the fix with a full added/delta/done/output_item.done sequence through the real client, plus unit and sequence-level regression tests. (I'd replied under the wrong, already-resolved thread by mistake first - apologies for the noise there.) |
|
Sadok Barbouche (@cr-sbarbouche) This change makes |
|
Thank you for the contribution. This pull request has had outstanding compatibility feedback about the package dependency floor for the new snapshot marker for more than three days without recent progress, so we are closing it for now to keep the review queue current. Please reopen it when you are ready to continue and address the outstanding item, and we will be happy to take another look. |
Motivation & Context
A code interpreter call's
inputs/outputsstream in as a series of small chunks, andChatResponse.from_updates/AgentResponse.from_updateshas to fold them back into one call._merge_content_item_listsguessed whether a new chunk was an ordinary delta (concatenate) or a provider resending the complete value (replace) by checking whether one chunk's text starts with the other's. That guess is wrong whenever two unrelated deltas simply happen to line up that way, which is common for code: repeated indentation, a closing paren right after an opening one, a short number followed by a longer one starting with the same digit. When it misfires, one of the two chunks is silently discarded — no exception, just quietly wrong or truncated code. See #8956 for the full writeup and a couple of concrete before/after examples.Description & Review Guide
_merge_content_item_listsinagent_framework/_types.pyno longer infers "is this a full resend?" from the chunk text. It now only treats a chunk as a full resend when it's explicitly tagged via a new_CONTENT_ITEM_SNAPSHOT_KEYentry in itsadditional_properties; everything else is concatenated unconditionally.OpenAIChatClient'scode_interpreter_call_code.donehandler — the one real producer of a "full resend after deltas" chunk in this repo — now sets that tag on the content it builds, since it's the only side that actually knows the value is a full resend rather than a delta._CONTENT_ITEM_SNAPSHOT_KEYis the right place to draw this line versus, say, a dedicated field onContent. I kept it as anadditional_propertiesentry to match the existingmodel_output_kindprecedent used for refusal text and avoid widening the publicContentsurface for something only the streaming-merge internals need to see.Related Issue
Fixes #8956
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.