fix: preserve summary role attribution - #3614
JackYPCOnline wants to merge 2 commits into
Conversation
Keep generated conversation summaries attributed to the assistant while retaining a leading user message for provider compatibility.
Advance past pinned and synthetic messages during repeated summarization, and normalize summaries restored from legacy sessions.
github-actions
Bot
added
size/m
bug
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@strandly-the-agent Can you review this PR see if this fix: |
strandly-the-agent
left a comment
strandly-the-agent
left a comment
There was a problem hiding this comment.
Does this fix the reported bug? Partly — and it adds a worse one.
The "elevated trust" premise doesn't hold. There is no role-keyed trust, permission, or authority boundary anywhere in strands-py. Every consumer of role is provider wire-format (models/*.py), telemetry bucketing (telemetry/tracer.py:1294), the guardrail wrap target (models/bedrock.py:460-475), or token-estimate baselining (event_loop/event_loop.py:139-152) — none of them gate authority. Relabeling can't revoke trust the SDK never granted, and if anything an assistant turn reads to a model as its own prior conclusion, which needs no compliance step at all. What the PR does fix is real: AI-authored text was mislabeled as user input. That's a provenance bug worth fixing — it just shouldn't be described as an injection mitigation, and per SECURITY.md a vuln report belongs in HackerOne / aws-security@amazon.com rather than a public thread. Ironically the relabel also drops the summary out of Bedrock guardrail scanning (🟡 #2).
Separately, I found a data-loss regression unrelated to the reported bug (🔴 #1).
Verified on 384ea76d vs main@a2656017:
- ✅ 84/84 in
test_summarizing_conversation_manager.py+test_repository_session_manager.py;ruffclean - 🔴 repro: snapshot round-trip →
removed_message_count 5→8;user:question 3is in the live history, absent after restore, and was never summarized. Pre-PR base:5→7, no loss. - ✅ fix verified: comparing on
tracking_id→5→6, restored == live, 84/84 still pass - ✅ live Bedrock: assistant-first is rejected by claude-3-haiku / nova-micro / mistral-7b (
A conversation must start with a user message) → the preamble is load-bearing, not decorative - 🔴 refuted a draft finding of my own:
assistant,assistantadjacency does not break providers — nova-micro and mistral-7b both return HTTP 200. Pre- and post-PR violate on complementary split parities anyway (~50/50), so it's the same class flipped, not a new one. Not counted against this PR.
Verdict: worth holding the draft until 🔴 #1 is addressed. The progress-loop rewrite is a genuine improvement and I'd keep it — it's a necessary fix for a stall that commit 1b29bee8 itself introduces (verified: against commit 1 alone, the second reduce_context is a byte-identical no-op (7,3)→(7,3); on the full PR (7,3)→(6,4)), and it terminates cleanly across pinned-everything, preserve >= len, both summary_ratio clamp bounds, and a 40-message/12-round run. This is agent-generated review for a human to arbitrate, not a gate.
Questions
(blocking-ish) The PR body frames this as provenance; your comment frames it as trust. Those select different designs, and I think only you can settle which you're after. If it's provenance, would metadata.custom do the job more cleanly? types/content.py:222 documents it as "Arbitrary user/framework metadata (e.g. compression provenance)" — literally this use case — with working precedent in compression/pin_message.py:90-104, and it's stripped before the model call (event_loop/streaming.py:519-520), so it costs you no role change, no guardrail side effect, no telemetry re-bucketing, and no persisted-state contract shift. If it's trust, the honest version is a content-level control (delimit the summary, state that it's machine-generated compression to be treated as data), and the role flip doesn't buy it.
(non-blocking) Before this PR all three summary producers agreed on role="user"; after it they don't. compression/context_compression.py:189-192 still hard-casts to user (with the docstring at :163 and comment still justifying the rationale this PR overrides), and the model-invokable agentic tool at _context_manager/modes/agentic/agentic_context.py:138,143-144 splices a bare user-role summary with no preamble — arguably the more exposed path for the stated threat, since a model can call it on fetched content. TS is unchanged too (strands-ts/.../summarizing-conversation-manager.ts:167). Is a follow-up planned to fix generate_summary once (it's not public per compression/__init__.py), or is the scope deliberately Python + this manager only? Worth saying so in the body either way, so the changelog doesn't read as an SDK-wide fix.
Appendix — non-blocking (4)
- ⚪ Telemetry re-bucketing.
telemetry/tracer.py:1294picks the redaction policy key offrole, so a summary of user content now lands undergen_ai.output.messagesinstead ofgen_ai.input.messages. Mechanical consequence of the relabel; misclassification, no leak. - ⚪
preserve_recent_messages=0→ history can end on the assistant summary, which livemistral.mistral-7b-instruct-v0:2rejects (requires the last turn in the conversation to be a user message); Claude/Nova accept it.preserve_recent_messagesis unvalidated (summarizing_conversation_manager.py:44,77) — that gap pre-dates this PR, so it's not yours, but is0meant to be legal at all? - ⚪ Preamble/prompt voice mismatch.
DEFAULT_SUMMARIZATION_PROMPT(compression/context_compression.py:23-50) is unchanged and still says "You MUST NOT respond conversationally", "You MUST NOT address the user directly", "format the summary in the third person" — written for replay as a user message, now emitted as an assistant turn narrating the user's own requests back at them. And"Previous conversation summary:"is a bare four-word label with no verb: it reads as a truncated user request or a section header priming more summarizing. Downstream of the Question above, so not a finding on its own. - ⚪ Preamble
tracking_idisn't stable across restores of a legacy session —_create_summary_preamble()(summarizing_conversation_manager.py:113-118) mints a fresh UUID each time it's used as the fallback at:102, which is mildly at odds with the durability contract attypes/content.py:236-240. Self-heals after the firstsync_agent(); no consumer keyed on it that I could find.
Things I checked that held up: removed_message_count accounting on the plain (non-snapshot) session path is byte-clean across two summarization rounds; no preamble accumulation (exactly 1 across 12 rounds and across restores); summary-of-summary converges; main→PR state migration works correctly (coercion + synthesized preamble); _fix_broken_tool_use mutates in place so it doesn't break identity and can't eat the preamble; the rewritten toolUse/toolResult comment at :294-296 is more accurate than what it replaced.
| persisted_to_summarize = [ | ||
| message | ||
| for message in to_summarize | ||
| if all(message is not previous for previous in previous_summary_messages) | ||
| ] |
There was a problem hiding this comment.
🔴 Identity-based filtering breaks across a snapshot round-trip → real messages are silently and permanently lost on the next session restore.
Agent.load_snapshot (agent/agent.py:1607-1611) copy.deepcopys data["messages"] and separately calls restore_from_session. After that, self._summary_preamble / self._summary_message are content-identical but not the same objects as the ones in agent.messages — so every is not previous check (here, and at :224-230, :232-234, :236-237) silently stops excluding them, and removed_message_count += len(persisted_to_summarize) at :233 over-counts by however many artifacts leaked in. That offset is the slice index into persisted session rows (session/repository_session_manager.py:219-223), so the over-count skips real rows forever.
Repro — FileSessionManager, 5 turns, summarize, take_snapshot(preset="session") → load_snapshot, summarize again, restore:
PR : removed_message_count 5 -> 8 (delta 3, but only ONE real persisted row was consumed)
LIVE : [preamble, summary, 'user:question 3', 'assistant:...', 'user:question 4', ...]
RESTORED : [preamble, summary, 'user:question 4', ...]
>>> 'user:question 3' is gone, and it was never summarized <<<
base : removed_message_count 5 -> 7 restored == live, no loss
Pre-PR is immune because its arithmetic (+= len(to_summarize) then -= 1) is identity-independent — so this is a regression introduced here. Reachable through the public Agent.take_snapshot/load_snapshot API, and already called that way in-repo per request at multiagent/a2a/executor.py:216 and per retry at vended_plugins/goal/plugin.py:365. (multiagent/swarm.py:115, multiagent/graph.py:256, agent/_agent_as_tool.py:268 all use the same messages = copy.deepcopy(...) shape.)
Every message here already carries a durable tracking_id — minted at :117 for the preamble and :242 for the summary — and types/content.py:236-240 documents it as "Durable, stable UUID… Survives session save/restore and snapshots… Preserved when a message is copied or restored." That's exactly the identity you want:
| persisted_to_summarize = [ | |
| message | |
| for message in to_summarize | |
| if all(message is not previous for previous in previous_summary_messages) | |
| ] | |
| persisted_to_summarize = [ | |
| message for message in to_summarize if not self._is_generated_summary_message(message) | |
| ] |
with a helper (and the same substitution at the three other filter sites):
def _is_generated_summary_message(self, message: Message, *, preamble_only: bool = False) -> bool:
"""Whether ``message`` is a preamble/summary this manager generated.
Compares on the durable ``tracking_id`` as well as object identity, so the check still
holds after a snapshot round-trip or any other copy of ``agent.messages``.
"""
candidates = (self._summary_preamble,) if preamble_only else (self._summary_preamble, self._summary_message)
for previous in candidates:
if previous is None:
continue
if message is previous:
return True
tracking_id = previous.get("tracking_id")
if tracking_id and message.get("tracking_id") == tracking_id:
return True
return FalseI ran this: the repro flips to 5 -> 6 with restored == live and no loss, all 84 tests still pass, ruff clean. Worth a regression test for the exact path too (save → summarize → snapshot round-trip → summarize → assert no persisted message is lost) — nothing in the suite exercises it today, which is how this got through.
| """ | ||
| return run_async(lambda: generate_summary(messages, agent.model, self.summarization_system_prompt)) | ||
| summary = run_async(lambda: generate_summary(messages, agent.model, self.summarization_system_prompt)) | ||
| return cast(Message, {**summary, "role": "assistant"}) |
There was a problem hiding this comment.
🟡 The relabel removes the summary from Bedrock guardrail scanning — the one role-keyed security mechanism in the SDK.
models/bedrock.py:460-475 (_find_last_user_text_message_index) plus the wrap at :509-511 target the last user message with text when guardrail_latest_message=True. Before this PR a summary of possibly-attacker-influenced scraped content was role="user" and got scanned. After it, the last user text message is the constant preamble, so that's all the guardrail sees:
PRE-PR (summary=user) -> text sent to guardrail: ['SUMMARY: <untrusted scraped text>']
POST-PR (summary=assistant) -> text sent to guardrail: ['Previous conversation summary:']
Reachable whenever no later user turn carries text — i.e. mid-tool-loop, where trailing user turns are toolResult-only (which is the exact case the :504-506 comment says that index lookup exists for).
🟡 rather than 🔴: it stops the summary being scanned, it doesn't disable guardrails for other messages, and I didn't test an end-to-end bypass against a live guardrail. But this PR is motivated by a security concern, so it's worth deciding deliberately rather than inheriting it as a side effect — either keep scannable content in the preamble, or extend the guardrail-target search to include the manager's attributed summary.
|
|
||
| result = summarization_agent("Please summarize this conversation.") | ||
| return cast(Message, {**result.message, "role": "user"}) | ||
| return cast(Message, {**result.message, "role": "assistant"}) |
There was a problem hiding this comment.
🟡 The coercion runs after the extension point, so a custom summarization_agent's role is silently overwritten with no opt-out.
summarization_agent is a documented public constructor parameter, and this line discards whatever role its result carried — as does :338 for the default model path. Before this PR the overwrite was to user, so this isn't new behavior, but it is newly load-bearing: a caller whose summarization agent deliberately shapes its output (or who depends on the pre-PR user contract) has no way to keep it.
Suggest either documenting it as an intentional non-overridable contract in the summarization_agent docstring (:57-58), or gating the coercion so a subclass overriding _generate_summary can opt out. Cheap either way; mostly I want the choice to be explicit rather than implicit.
| self._summary_preamble = None | ||
| return None | ||
|
|
||
| self._summary_message = cast(Message, {**self._summary_message, "role": "assistant"}) |
There was a problem hiding this comment.
🟡 Persisted-state contract changed with no version marker — and "No public API changes" is arguable because of it.
get_state() / restore_from_session() are a serialization contract with sessions written by other SDK versions. Adding the summary_preamble key is fine (there's precedent for additive keys: sliding_window_conversation_manager.py:134,147). The part that isn't additive is redefining what summary_message's role means, with nothing in the serialized dict saying which contract wrote it — while the SDK versions its other serialized contracts (types/_snapshot.py:33,45,60-63, experimental/checkpoint/checkpoint.py:40,57,73-80).
Concretely, state written by this branch and read by main restores agent.messages as ['assistant', 'user'] — assistant-first, which I live-verified is rejected by all three model families I could reach (claude-3-haiku, nova-micro, mistral-7b: ValidationException: A conversation must start with a user message). That's exactly what PR #698 (d9f8d8a7) existed to fix. It only bites a rollback or a mixed-version fleet sharing a session store, but sessions outlive SDK versions, so it's worth a schema_version on this manager's state slice (or at minimum a migration note in the PR body).
Also: given this contract shift, would you check whether the repo's api/needs-review label applies (team/API_BAR_RAISING.md)? The PR currently carries bug/python/area-context/size/m. Your call — I'm flagging the workflow, not asserting the verdict.
| assert manager.removed_message_count == 5 | ||
|
|
||
|
|
||
| def test_reduce_context_with_pinned_prefix_makes_progress(): |
There was a problem hiding this comment.
🟡 Coverage is missing exactly the lines the 🔴 above lives behind.
pytest --cov=strands.agent.conversation_manager.summarizing_conversation_manager --cov-report=term-missing on this branch:
summarizing_conversation_manager.py 119 5 26 5 93% 98-99, 214, 219, 237, 297->301
98-99—restore_from_session's no-prior-summary branch214,219— both newContextWindowOverflowExceptionraise paths of the progress loop this PR adds237—summary_input.insert(0, self._summary_message), i.e. the branch that keeps a prior summary from being dropped- (
297->301is pre-existing, not yours)
Mutation testing found one survivor: deleting the "summary_preamble" key from get_state() leaves all 84 tests green — because restore_from_session regenerates a textually identical preamble at :102, nothing distinguishes "restored the persisted one" from "silently regenerated one". Also, the += 1 → += 2 mutation in the loop is killed by only this test; nothing else drives the loop past its first iteration.
On this test specifically: it does pin down a real bug (against 1b29bee8 alone the second reduce_context is a byte-identical no-op (7,3)→(7,3); on the full PR (7,3)→(6,4)), so it's genuine, not padding. But it asserts magic tuples with no derivation. Consider also asserting the invariant that actually matters — removed_message_count strictly increases and len(agent.messages) strictly decreases between calls — which survives accounting refactors. And the missing test that matters most is a session save → restore → re-summarize round trip; that's the hole 🔴 #1 shipped through.
(Credit where it's due: tests/strands/session/test_repository_session_manager.py:153 flipping the fixture role to "user" is not a tautology — I mutation-verified it's the only test covering the backward-compat coercion.)
2 participants
Description
SummarizingConversationManagerstores model-generated summaries as user-authored history, losing their provenance across compression and session restore. Keep a fixed user preamble for provider compatibility while retaining the generated summary as an assistant message. No public API changes.Related Issues
No public issue (privately reported).
Documentation PR
No documentation changes; the public API is unchanged.
Type of Change
Bug fix
Testing
Focused tests, lint, package build, and commit hooks pass.
hatch run preparereached the all-version suite but was blocked by existing Python 3.14 OpenTelemetry exporter import errors intests/strands/telemetry/test_config.py.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.