fix(flows): inherit unset streaming fields on after_model_callback replacement - #7036
Open
Bruce-Yii wants to merge 1 commit into
Open
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
partialduring SSE streaming - every delta is persisted to the session as a final event #7035Problem:
after_model_callbackmay return a replacementLlmResponse("the actual model response will be ignored and the provided content will be returned to user"). During streaming, the callback runs on every response includingpartial=Truedeltas, and the replacement was used wholesale. SinceLlmResponse.partialdefaults toNone— and unset means final per the streaming protocol — any callback that rebuilds the response silently converted every delta into a final response.Runnerpersists every non-partial event, so each delta fragment was appended to the session as a separate complete model event (4 persisted events instead of 1 in the issue's repro), and SSE clients saw N "final" responses. The same field-loss class affectsturn_complete(live path closes its request queue on it).Solution:
In the module-level
_handle_after_model_callback(the single choke point for plugin + agent callbacks, covering both the live and SSE call sites), inheritpartial/turn_completefrom the replaced response when the replacement leaves them unset. An explicitly set value is always respected. Inheritance returns amodel_copyonly when a field actually needs filling, so explicitly complete replacements keep their identity and callback-owned objects are never mutated.usage_metadatais deliberately left out of this change to keep the fix minimal (noted as residual risk below).Testing Plan
Unit Tests:
New tests in
tests/unittests/flows/llm_flows/test_base_llm_flow_partial_handling.py(5 added, file now 9 tests):test_after_model_callback_replacement_inherits_partial— rebuilt replacement keeps the delta'spartial=True; the callback's object is not mutated.test_after_model_callback_replacement_inherits_turn_complete.test_after_model_callback_replacement_explicit_partial_respected— explicitpartial=Falsewins (guards against over-correction).test_after_model_callback_replacement_without_streaming_keeps_identity— non-streaming pass-through untouched.test_run_async_sse_rebuilt_responses_stay_partial— flow-level SSE run: deltas staypartial, final stays final.Fail-before verified by stashing the
src/fix: the 3 behavior tests fail on pristinemain(b018062), the 2 guard tests pass both before and after.pytestresults (Python 3.12, Windows):tests/unittests/flows/llm_flows/test_base_llm_flow_partial_handling.py+test_base_llm_flow.py: 109 passedtests/unittests/flows/llm_flows/(minus 2 files needing the uninstalled optionalgoogle.cloudspeech dep — pre-existing collection errors, also present without this change): 676 passed, 1 skippedtest_llm_agent_callbacks.py+test_model_callback_chain.py+tests/unittests/runners: 95 passed, 1 skipped, 3 xfailedruff checkclean;pyink==25.12(CI pin) andisort==8.0.1clean. (ruff formatdisagrees with the repo's pyink style on pristine files too, so it was intentionally not applied.)Manual End-to-End (E2E) Tests:
Ran the issue's verbatim repro script (
InMemoryRunner, fake streaming model, no API key) before/after:partial=None/final=True, 4 persisted model events.partial=True/True/True/None, 1 persisted model event (Hello [REDACTED] world.), matching the issue's documentedmodel_copyworkaround shape.Checklist
Additional context
usage_metadatahas the same loss shape (a rebuilt response drops token accounting). Left out deliberately for minimal scope; happy to follow up if maintainers want it covered too.