Python: fix streaming transcript duplication with message injection and per-service-call persistence - #7605
Open
westey (westey-m) wants to merge 2 commits into
Open
Conversation
…ice-call persistence
westey (westey-m)
marked this pull request as ready for review
August 10, 2026 18:37
Contributor
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes duplicated streaming transcripts when message injection and per-service-call history persistence interact.
Changes:
- Preserves inner streaming continuation state during response rebuilding.
- Adds middleware and harness regression tests.
- Refreshes Python lockfile tooling versions.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
python/uv.lock |
Refreshes pyrefly and zuban locks. |
python/packages/core/agent_framework/_sessions.py |
Carries streaming conversation control state. |
python/packages/core/tests/core/test_middleware_with_chat.py |
Tests continuation-state propagation. |
python/packages/core/tests/core/test_harness_agent.py |
Tests transcript deduplication end to end. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Agent Framework Review — Iteration 1
Completed passes: 5 | Result: No high-severity findings
Scope: full PR (1 commit(s)): d21f3d2a7390
Review passes
- Correctness (
gpt-5.6-sol) — No issues found in this pass. - Security Reliability (
claude-opus-4.8) — No issues found in this pass. - Test Coverage (
gpt-5.6-sol) — No issues found in this pass. - Failure Modes (
claude-opus-4.8) — No issues found in this pass. - Design Approach (
claude-opus-4.8) — No issues found in this pass.
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.
Motivation & Context
When a harness/agent run combines message injection (e.g.
MessageInjectionMiddleware, used by the harness agent to inject todo-list context) with per-service-call history persistence (PerServiceCallHistoryPersistingMiddleware), the streaming path duplicates the conversation transcript after a tool call.PerServiceCallHistoryPersistingMiddlewaresignals "the provider already holds this history" by settingresponse.conversation_id = LOCAL_HISTORY_CONVERSATION_IDand callingresponse.mark_internal_conversation_id(). In the streaming path this is applied to the inner final response viaResponseStream.with_result_hook.MessageInjectionMiddlewarethen wrapped that stream in an outerResponseStreamfinalized byChatResponse.from_updates(...)— and because neither the sentinel conversation id nor the internal marker is ever emitted on an individual update, both were silently dropped.The function-invocation loop reads that outer response. Seeing
conversation_id is None,_prepare_messages_for_next_iterationre-sends the whole turn instead of justresponse.messages[-1:], on top of the history the persistence middleware had already injected. The result is a duplicated transcript in which an assistant message carryingtool_callsis followed by another copy of the user/assistant messages rather than by itstoolresults.Strict chat-completion endpoints reject this outright. Against OpenAI it fails with HTTP 400:
The non-streaming path is unaffected, since
_process_non_streamingpassescontext.resultthrough unchanged.Description & Review Guide
What are the major changes?
MessageInjectionMiddlewarenow propagates the inner response's stream-control state onto the outer rebuilt response. A new module-private helper_carry_over_stream_control_state(response, inner_response)copiesconversation_idwhen set, and sets or clears the internal-conversation-id marker to mirror the inner response._stream_injected_messagestakes aninner_responses: list[ChatResponse]accumulator and records each iteration'sawait stream.get_final_response()._finalize_injected_stream(...)static method builds the outer response viaChatResponse.from_updatesand then carries state over from the last inner response — matching what the non-streaming path returns.test_middleware_with_chat.pycovering both propagation of the sentinel and the "don't clobber a real service conversation id" case, and an end-to-end harness test intest_harness_agent.pyasserting the streaming transcript is not duplicated.python/uv.lockis refreshed. It was stale relative topython/pyproject.toml(pyrefly1.1.1→1.2.0,zuban0.9.0→0.9.1) onmain, which madeuv runre-lock mid-hook and fail thepoe-checkpre-commit hook with "files were modified by this hook".What is the impact of these changes?
_update_continuation_statewritesresponse.conversation_idintosession.service_session_idunlesshas_internal_conversation_id()is true, so propagating the sentinel without its marker would leak it into the session. The sentinel does not reach callers, because the function loop's own outer stream rebuilds from updates and the non-streaming path clears it via_clear_internal_conversation_id.What do you want reviewers to focus on?
Whether taking the last inner response is the right choice when message injection spans multiple service calls, and whether
_carry_over_stream_control_statebelongs onResponseStreamas shared behaviour rather than living privately in_sessions.py.The .NET implementation appears unaffected:
PerServiceCallChatHistoryPersistingChatClientstampsupdate.ConversationIdon each streaming update, so the state survives a rebuild from updates there.Related Issue
Fixes #7591
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.