Skip to content

fix(python): summarizing conversation manager falls back to truncation instead of failing every turn - #5603

Merged
mkmeral merged 4 commits into
strands-agents:mainfrom
strandly-the-agent:agent-tasks/5598
Oct 29, 2026
Merged

mkmeral merged 4 commits into
strands-agents:mainfrom
strandly-the-agent:agent-tasks/5598

Conversation

@strandly-the-agent

@strandly-the-agent strandly-the-agent commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Requested by: @ekumar96 — cut PRs for the ContextOffloader / SummarizingConversationManager fixes blocking the AgentCore Harness launch (Slack, 2026-10-27).

Human Overview

Description

When reactive overflow recovery could not summarize (nothing old enough, or the summary call itself overflowed because one message is bigger than the window), reduce_context re-raised, the turn failed, and every later turn — even Say hello. — failed the same way.

Now, in reactive mode only, a summarization that fails with ContextWindowOverflowException (nothing old enough to summarize, or the summary call itself overflows) falls back to _truncate_oversized_text_blocks (new compression/truncate_text.py, private): every untruncated text block in a non-pinned message whose truncation saves more than 2,000 chars is replaced in one pass by a 2,000-char head + ... [truncated: N chars removed] ... + 2,000-char tail (the sliding-window marker format). The error propagates when nothing qualifies, and any other summarization error (throttling, network) is re-raised untouched — truncation is lossy and a retry may still succeed. Two further fixes keep the overflow retry loop bounded without any per-invocation state: _summarize_oldest now raises ContextWindowOverflowException when the only message old enough to summarize is the previous summary (re-summarizing it never shrinks the history — this is the pre-existing no-progress loop on main), so every reactive reduction either removes a message, truncates a block, or raises; and removed_message_count / _summary_message are updated only after the summary call succeeds, so a failed summary can no longer persist an inflated session offset.

Truncation edits agent.messages only — a session store keeps the original content (documented on reduce_context).

Related Issues

Fixes #5598

Documentation PR

None: internal behaviour of an existing manager; docstrings updated.

Type of Change

Bug fix

Testing

  • New: helper unit tests (test_truncate_text.py, 8 cases incl. marker-in-user-text, pinned, no re-truncate, no-op on preserve_chars<=0); manager tests for fallback on "insufficient messages", fallback on a summary call that overflows, no truncation on a throttled summary call, re-raise once nothing is left, summary-only range raises instead of re-summarizing (and is still folded in when real messages accompany it), removed_message_count unchanged on failure, proactive path untouched; end-to-end Agent test with a size-aware mock model: a 50k-char paste is truncated and Say hello. answers in 2 model calls.

  • pytest on summarizing/conversation_manager/session suites after round 3: 403 passed; round 2 across agent suites too: 591 passed, 1 pre-existing unrelated failure (test_baggage_attributes_stamped_on_agent_span, fails on the branch base too). ruff check/format + mypy clean; run-analysis.mjs: size/m.

  • Repro of the reviewer's scenarios on the fixed branch (repro.py in the artifacts): 12×9k-char history with a 20k window now stops cleanly after 21 model calls (main: RecursionError after ~1,000 calls once the fallback made the loop reachable); giant paste + FileSessionManager recovers in 3 calls with removed_message_count=0 and 14/14 messages restored.

  • I ran hatch run prepare (sandbox has no hatch; ran the equivalent ruff/mypy/pytest directly)

Additional Details

Independent review (fresh-context reviewer) and ledger

Round 1 verdict: changes requested — all 🔴/🟡 addressed in the second commit:

  • 🔴 removed_message_count was advanced before _generate_summary, so a fallback success persisted an inflated offset and messages vanished on session restore — bookkeeping moved after the summary call; test added.
  • 🔴 termination did not hold when summarization alternated fail/succeed (pre-existing no-progress loop on main, now reachable) — per-invocation cap of 10 reactive reductions; test added. The underlying no-progress loop on main (summarizing only the summary itself when len(messages) == preserve_recent_messages + 1) is worth its own issue.
  • 🟡 one block per retry with a ~48-char floor cost O(#blocks) model calls — now one pass over all blocks, each must save > preserve_chars.
  • 🟡 bare ... [truncated: substring made user text containing it un-truncatable — exact marker regex.
  • 🟡 truncation not persisted to the session store — documented; routing message edits through the session manager is out of scope.
  • 🔵 preserve_chars=0 grew the text; non-dict content blocks raised inside the handler; __init__ docstring; literal assertions — all fixed.
  • Narrowing except Exception was initially not taken; done in round 3 (f5e274f) at mkmeral's request — the fallback now triggers only on ContextWindowOverflowException.
  • An advanced-tier adversarial pass was also spawned but timed out (900s) before reporting; the reviewer's own adversarial probes above covered the same ground.

Round 2 (after mkmeral's review, commit 0d8b0eb): helper made private; the per-invocation reduction cap (_reactive_reductions, reset in apply_management) replaced by the no-progress guard in _summarize_oldest, which needs no reset and also protects direct reduce_context callers and subclasses that override apply_management. Fresh-context reviewer verdict: approve, no 🔴. Termination fuzzed over ~10,750 configurations (plain/tool-pair histories, pins, pin_first, ratio/preserve extremes): 0 non-terminating cases with the guard, max 16 reductions; the guard-stripped copy had ~4,100 no-progress fixpoints. Guard verified to fire after a FileSessionManager restore (dict equality; the restored summary carries its tracking id). Two 🟡 noted, not changed:

  • Proactive path behaviour change: with [summary] + preserve_recent_messages recents, main re-summarized the summary on every proactive trigger (one model call, lossy, no structural gain); now that logs a warning and leaves the history alone. Intentional — happy to scope the guard to the reactive path if preferred.
  • A model that regenerates an oversized tool result on every turn still makes a model call per turn: each overflow is resolved (truncate/summarize/raise), but nothing in the manager stops the model from re-creating the problem. That is the model's loop, bounded by invocation limits, not a reduce_context loop.

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works (human attestation pending)
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ger cannot summarize

When reactive overflow recovery fails to summarize (too few messages, or the
summary call itself overflows), truncate the largest text block in a
non-pinned message to a head+tail preview instead of re-raising. The error
propagates only once nothing is left to truncate, so an oversized paste no
longer permanently fails every later turn and the retry loop stays bounded.

Fixes strands-agents#5598
…reductions, count removals after the summary succeeds
@github-actions github-actions Bot added bug Something isn't working area-context Session or context related python Pull requests that update python code labels Oct 27, 2026

@mkmeral mkmeral left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@strandly-the-agent update pls

@codecov

codecov Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.82609% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../conversation_manager/compression/truncate_text.py 96.87% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@github-actions github-actions Bot added complexity/medium Touched functions have moderate cognitive complexity (11-25) size/m labels Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Request Changes

Solid, well-tested fix for #5598 — the truncation fallback, the per-invocation reduction cap, and moving removed_message_count/_summary_message bookkeeping after summary success all land correctly, and the test coverage (helper unit tests + manager fallback/cap/reset/proactive + e2e agent) is thorough. The two open maintainer threads are still unaddressed on this commit, and I found one related correctness concern around the cap.

Review themes
  • API surface (blocking): truncate_oversized_text_blocks is effectively public; per AGENTS.md internal helpers should be _-prefixed. See the open thread.
  • Reduction cap counts progress (important): the counter increments on every reactive reduction, not just no-progress ones, so a single large invocation that genuinely needs >10 summarization rounds would raise prematurely. Prefer counting only no-progress reductions (or resetting on history shrink). Detail in the thread reply.
  • Discoverability (minor): the reset lives in apply_management, far from the increment — a cross-referencing comment at both sites would save the next reader the hunt.
  • Known limitation (non-blocking): truncation rewrites agent.messages only, so a session restore re-inflates the original block and can re-trigger overflow. It's documented and routing through the session manager is reasonably out of scope — just calling it out for visibility.

Nice work bounding the loop and the careful session-offset handling — once the function is made private and the cap only counts non-progress, this looks good to go.

…on cap with a no-progress guard in _summarize_oldest
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Approve

Re-reviewed at 0d8b0eb (force-pushed since my last pass). Both points from the previous round are resolved, and the new approach is cleaner than what I'd suggested.

Resolution of prior feedback
  • Private helper ✅ — truncate_oversized_text_blocks → _truncate_oversized_text_blocks (_text_blocks was already private), matching the AGENTS.md internal-API rule. Resolves the open maintainer thread.
  • Reduction cap counted progress ✅ — the _reactive_reductions/_MAX_REACTIVE_REDUCTIONS cap (and the hidden reset in apply_management) is gone entirely. In its place, _summarize_oldest raises when to_summarize == [self._summary_message] — i.e. only when re-summarizing wouldn't shrink the history. This detects the actual no-progress condition instead of penalizing legitimate multi-round reductions, which also eliminates the "what resets the counter?" discoverability issue. Termination is still guaranteed: no-progress guard → truncation fallback → re-raise once nothing is left to truncate.
  • Tests ✅ — test_reduce_context_does_not_resummarize_only_the_previous_summary asserts no extra model call is made, and test_summarize_oldest_includes_previous_summary_with_older_messages guards the equality check from being too aggressive (summary still folded in when real messages accompany it). Ran the suite locally: 58 passed.

Nice iteration — removing the arbitrary cap in favor of a precise no-progress condition is the better design. LGTM.

@mkmeral mkmeral changed the title fix(python): SummarizingConversationManager falls back to truncation instead of failing every turn fix(python): summarizing conversation manager falls back to truncation instead of failing every turn Oct 28, 2026
@mkmeral
mkmeral marked this pull request as ready for review October 28, 2026 20:50
@mkmeral
mkmeral requested a review from a team as a code owner October 28, 2026 20:50
@mkmeral
mkmeral requested a review from arielnabavian October 28, 2026 20:50

@mkmeral mkmeral left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Approve

Re-reviewed at f5e274f. This round narrows the reactive truncation fallback to only fire on ContextWindowOverflowException; any other summarization failure (throttling, network, etc.) is re-raised untouched. Since truncation is lossy and transient errors may succeed on retry, this is exactly the right distinction.

Details
  • reduce_context now gates the fallback with isinstance(summarization_error, ContextWindowOverflowException) before calling _truncate_oversized_text_blocks, and the Raises docstring is updated to match.
  • New test test_reduce_context_does_not_truncate_when_summary_call_fails_for_other_reasons verifies a ModelThrottledException propagates and the oversized message is left intact — nicely complements the existing overflow-path test.
  • Ran the suite locally: 59 passed (one new test added).

All feedback from prior rounds remains resolved. LGTM.

@mkmeral
mkmeral enabled auto-merge (squash) October 29, 2026 16:31
@mkmeral
mkmeral merged commit e22fd05 into strands-agents:main Oct 29, 2026
36 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-context Session or context related bug Something isn't working complexity/medium Touched functions have moderate cognitive complexity (11-25) python Pull requests that update python code size/m

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] SummarizingConversationManager permanently breaks a session when summarization cannot reduce enough

3 participants