Repository navigation
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. |
LlmEventSummarizer accepted any summary response with content, even when the model stopped with MAX_TOKENS or SAFETY. The truncated or refused text was published as an EventCompaction and replaced the original events in every later model request, so facts from the compacted range were lost. A summary that ends with a finish reason other than STOP (or no reason) is now treated as a failed compaction: it is logged and None is returned, as the BaseEventsSummarizer contract specifies, so the uncompacted history is kept.
HYYAlex
force-pushed
the
fix/compaction-reject-incomplete-summary
branch
from
November 13, 2026 13:33
63ca836 to
10b2beb
Compare
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.
Summary (in my own words)
When compacting chat history, the summary can be cut off or blocked, but the framework only checks whether the summary has any text, not why it ended. Any text counts as success, so a half-written summary replaces the complete old history. My fix: before using a summary, check its finish reason; if it did not end normally, treat the compaction as failed and keep the original history unchanged.
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):
Problem:
LlmEventSummarizer.maybe_summarize_eventspublished anEventCompactionfor any summary response that had content. It never checkedfinish_reason. When the summary model stopped withMAX_TOKENS(partial summary) orSAFETY(refusal text), that text replaced the original events in every later model request, so facts from the compacted range were silently lost. ADK already keeps the uncompacted history when the summarizer raises, but a response that reports failure throughfinish_reasondoesn't raise.Solution:
Treat a summary whose
finish_reasonis anything other thanSTOP(or unset /FINISH_REASON_UNSPECIFIED) as a failed compaction. The summarizer logs a warning and returnsNone, which is what theBaseEventsSummarizercontract specifies for a failed compaction ("If compaction failed, return None"). The uncompacted history is kept. Responses without a finish reason (for example from custom models) keep today's behavior.Testing Plan
Unit Tests:
Added to
tests/unittests/apps/test_llm_event_summarizer.py:test_maybe_compact_events_rejects_incomplete_summary:MAX_TOKENS,SAFETY, andOTHERreturnNone. These subtests fail without the fix.test_maybe_compact_events_accepts_complete_summary:None,STOP, andFINISH_REASON_UNSPECIFIEDstill produce a compaction.The 15 failures are in
tests/unittests/tools/(test_rest_api_tool.py,test_load_web_page.py,test_mcp_session_manager.py). They fail the same way on an unmodifiedmainin my environment, because of local HTTP proxy settings, and are unrelated to this change. I also rantoxfor Python 3.12, 3.13 and 3.14. The results are the same: the only failures are the same proxy-relatedtools/tests, plustelemetry/test_instrumentation.py::test_collection_reaches_concurrently_spawned_tasksonce on 3.14, which passes when re-run on its own, both with and without this change.Manual End-to-End (E2E) Tests:
A
RunnerwithEventsCompactionConfig(compaction_interval=1, overlap_size=0, summarizer=LlmEventSummarizer(...)), where a scriptedBaseLlmplays both the agent model and the summary model (no network). Turn 1: "The delivery address is 123 Maple Avenue." Turn 2: "Where will it be delivered?". The output shows the history the agent model receives on turn 2.Before (main):
After (this PR):
The full script is in the linked issue.
Checklist
Additional context
pre-commit runpasses on the changed files (ruff, isort, pyink, addlicense, ADK compliance checks, codespell).AI disclosure: I used an AI assistant to help investigate this issue and draft the code and this description. I reviewed every line, ran the tests myself, and can explain the change.