Skip to content

fix(apps): keep history when the compaction summary ends early - #8128

Open
HYYAlex wants to merge 1 commit into
google:mainfrom
HYYAlex:fix/compaction-reject-incomplete-summary
Open

HYYAlex wants to merge 1 commit into
google:mainfrom
HYYAlex:fix/compaction-reject-incomplete-summary

Conversation

@HYYAlex

@HYYAlex HYYAlex commented Nov 13, 2026

Copy link
Copy Markdown

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_events published an EventCompaction for any summary response that had content. It never checked finish_reason. When the summary model stopped with MAX_TOKENS (partial summary) or SAFETY (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 through finish_reason doesn't raise.

Solution:

Treat a summary whose finish_reason is anything other than STOP (or unset / FINISH_REASON_UNSPECIFIED) as a failed compaction. The summarizer logs a warning and returns None, which is what the BaseEventsSummarizer contract 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:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Added to tests/unittests/apps/test_llm_event_summarizer.py:

  • test_maybe_compact_events_rejects_incomplete_summary: MAX_TOKENS, SAFETY, and OTHER return None. These subtests fail without the fix.
  • test_maybe_compact_events_accepts_complete_summary: None, STOP, and FINISH_REASON_UNSPECIFIED still produce a compaction.
$ pytest tests/unittests/apps/test_llm_event_summarizer.py
12 passed, 6 subtests passed

$ pytest tests/unittests            # Python 3.11
24761 passed, 15 failed, 112 skipped

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 unmodified main in my environment, because of local HTTP proxy settings, and are unrelated to this change. I also ran tox for Python 3.12, 3.13 and 3.14. The results are the same: the only failures are the same proxy-related tools/ tests, plus telemetry/test_instrumentation.py::test_collection_reaches_concurrently_spawned_tasks once on 3.14, which passes when re-run on its own, both with and without this change.

Manual End-to-End (E2E) Tests:

A Runner with EventsCompactionConfig(compaction_interval=1, overlap_size=0, summarizer=LlmEventSummarizer(...)), where a scripted BaseLlm plays 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):

STOP       -> The delivery address is 123 Maple Avenue. | Where will it be delivered?
MAX_TOKENS -> The delivery address is | Where will it be delivered?
SAFETY     -> I cannot summarize this conversation. | Where will it be delivered?

After (this PR):

Discarding summary that ended with finish reason FinishReason.MAX_TOKENS; keeping the uncompacted history.
Discarding summary that ended with finish reason FinishReason.SAFETY; keeping the uncompacted history.
STOP       -> The delivery address is 123 Maple Avenue. | Where will it be delivered?
MAX_TOKENS -> The delivery address is 123 Maple Avenue. | Saved. | Where will it be delivered?
SAFETY     -> The delivery address is 123 Maple Avenue. | Saved. | Where will it be delivered?

The full script is in the linked issue.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

pre-commit run passes 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.

@google-cla

google-cla Bot commented Nov 13, 2026

Copy link
Copy Markdown

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
HYYAlex force-pushed the fix/compaction-reject-incomplete-summary branch from 63ca836 to 10b2beb Compare November 13, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Event compaction accepts a truncated (MAX_TOKENS) or blocked (SAFETY) summary and replaces the conversation history with it

2 participants