From 7965462d6c6fd680dfd96789f23964a6bb7e0c24 Mon Sep 17 00:00:00 2001 From: Matt Ezell Date: Wed, 29 Jul 2026 05:06:16 -0500 Subject: [PATCH] fix(compression): choose summary role by template-visible alternation The compaction summary's role was selected against the LITERAL neighbouring messages (compressed[-1] / tail_messages[0]). Mistral-family chat templates (Devstral, Mistral Small 3.x, Magistral) enforce user/assistant alternation but exempt the tool flow (tool results and assistant messages carrying tool_calls) from the check, so a protected head ending [user, assistant(tool_calls), tool] pinned the summary to role="user" while the last role the template counts is "user": the backend rejects the whole request with a Jinja alternation error (HTTP 500). The summary persists in the stored conversation, every retry replays the identical poisoned history, and the session is permanently unrecoverable. Fires on EVERY compaction against a Mistral-strict backend, captured byte-exact via a tee-proxy in front of a llama.cpp/llama-swap Devstral deployment. Fix: compute both neighbour roles through _template_visible_role(), which skips template-exempt messages. The #52160 (Anthropic user-first) and #58753 (zero-user-turn) forced-user guards are preserved; their forced shapes (summary-user followed only by exempt messages) are alternation-safe. When the visible head ends "assistant" and the visible tail opens "user", no standalone role can alternate and the existing merge-into-tail fallback now correctly fires (the literal logic emitted a standalone user summary there: a second poisoning shape). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01LGN45sMMbwM8cW9T9ga4ou --- agent/context_compressor.py | 89 ++++++- .../test_summary_role_template_alternation.py | 240 ++++++++++++++++++ 2 files changed, 323 insertions(+), 6 deletions(-) create mode 100644 tests/agent/test_summary_role_template_alternation.py diff --git a/agent/context_compressor.py b/agent/context_compressor.py index 4837925ce62..209253335b6 100644 --- a/agent/context_compressor.py +++ b/agent/context_compressor.py @@ -170,6 +170,35 @@ def _fresh_compaction_message_copy(msg: Dict[str, Any]) -> Dict[str, Any]: return fresh +def _template_visible_role(message: Any) -> Optional[str]: + """Role as counted by strict chat-template alternation checks. + + Mistral-family templates (Devstral, Mistral Small 3.x, Magistral) + enforce user/assistant alternation at render time but EXEMPT the tool + flow from the check: ``tool`` results and assistant messages carrying + ``tool_calls`` are skipped. A summary role chosen against the *literal* + neighbouring roles can therefore still violate alternation as the + template sees it. The canonical failure: the protected head ends + ``[user, assistant(tool_calls), tool]``, so the literal last role is + ``tool`` and the summary is pinned to ``role="user"`` -- but the last + role the template counts is ``user``, the template sees user -> user, + and llama.cpp / Mistral-hosted backends reject the ENTIRE request with + a Jinja alternation error (HTTP 500). Because the summary persists in + the stored conversation, every retry replays the same poisoned history + and the session is unrecoverable. + + Returns ``None`` for messages the alternation check skips. + """ + if not isinstance(message, dict): + return None + role = message.get("role") + if role == "tool": + return None + if role == "assistant" and message.get("tool_calls"): + return None + return role + + def _strip_persistence_markers(messages: List[Dict[str, Any]]) -> None: """Enforce the compaction invariant: no assembled message carries a session-store persistence marker. @@ -5420,8 +5449,43 @@ This compaction should PRIORITISE preserving all information related to the focu # last_head_role reads the assembled (post-strip) head; first_tail_role # reads the assembled (post-strip) tail_messages — a stripped stale # handoff must not influence alternation-safe role selection. - last_head_role = compressed[-1].get("role", "user") if compressed else "user" - first_tail_role = tail_messages[0].get("role", "user") if tail_messages else None + # Both are TEMPLATE-VISIBLE roles (``_template_visible_role``), not the + # literal list neighbours: strict Mistral-style templates skip tool + # results and assistant tool-call messages when enforcing + # user/assistant alternation, so the summary must alternate against + # the nearest message the template actually counts. Selecting against + # the literal neighbour (previously ``compressed[-1]``) emitted the + # summary as role="user" behind a ``[user, assistant(tool_calls), + # tool]`` head — which every Mistral-strict backend rejects with a + # Jinja alternation 500, permanently poisoning the session. + last_head_role: Optional[str] = "user" + if compressed: + last_head_role = next( + ( + role + for role in ( + _template_visible_role(m) for m in reversed(compressed) + ) + if role is not None + ), + # Head holds only template-exempt messages: the summary will + # be the first message the template counts, and the sequence + # must open with "user" (handled below alongside the forced + # cases). + None, + ) + first_tail_role = None + if tail_messages: + first_tail_role = next( + ( + role + for role in ( + _template_visible_role(m) for m in tail_messages + ) + if role is not None + ), + None, + ) # When the only protected head message is the system prompt, the # summary becomes the first *visible* message in the API request # (most adapters — Anthropic, Bedrock — send the system prompt as @@ -5455,9 +5519,15 @@ This compaction should PRIORITISE preserving all information related to the focu ) if not _user_survives: _force_user_leading = True - # Pick a role that avoids consecutive same-role with both neighbors. - # Priority: avoid colliding with head (already committed), then tail. - if last_head_role in {"assistant", "tool"} or _force_user_leading: + # Pick a role that alternates with both template-visible neighbors. + # Priority: alternate against the head (already committed), then tail. + # ``None`` (all-exempt head) means the summary opens the visible + # sequence, which strict templates require to start with "user". + if ( + last_head_role is None + or last_head_role in {"assistant", "tool"} + or _force_user_leading + ): summary_role = "user" else: summary_role = "assistant" @@ -5465,7 +5535,14 @@ This compaction should PRIORITISE preserving all information related to the focu # collide with the head, flip it. if first_tail_role is not None and summary_role == first_tail_role: flipped = "assistant" if summary_role == "user" else "user" - if flipped != last_head_role and not _force_user_leading: + # ``last_head_role is None`` (all-exempt head) pins the summary to + # "user" above; flipping to "assistant" would make the visible + # sequence open with "assistant", which strict templates reject. + if ( + flipped != last_head_role + and last_head_role is not None + and not _force_user_leading + ): summary_role = flipped else: # Both roles would create consecutive same-role messages diff --git a/tests/agent/test_summary_role_template_alternation.py b/tests/agent/test_summary_role_template_alternation.py new file mode 100644 index 00000000000..2de622445f9 --- /dev/null +++ b/tests/agent/test_summary_role_template_alternation.py @@ -0,0 +1,240 @@ +"""Regression coverage: the compaction summary role must alternate against +TEMPLATE-VISIBLE neighbours, not literal list neighbours. + +Mistral-family chat templates (Devstral, Mistral Small 3.x, Magistral) +enforce user/assistant alternation at render time but exempt the tool flow +from the check: ``tool`` results and assistant messages carrying +``tool_calls`` are skipped. The summary-role selection previously keyed off +the LITERAL last head message (``compressed[-1]``), producing this captured +failure (Hermes Desktop v0.19.0 against llama.cpp-served Devstral): + + [0] system + [1] user <- original first user turn, protected + [2] assistant (tool_calls, exempt) + [3] tool (exempt) + [4] user <- COMPACTION SUMMARY, pinned to "user" because the + literal previous role was "tool" + [5..] assistant(tool_calls)/tool pairs + +The template counts only [1] and [4]: user -> user, and the backend rejects +the ENTIRE request with a Jinja alternation error (HTTP 500): + + "After the optional system message, conversation roles must alternate + user and assistant roles except for tool calls and results" + +Because the summary persists in the stored conversation, every retry +replays the same poisoned history: the session is permanently broken, on +EVERY compaction, overflow or not. + +These tests pin the fix: neighbour roles for summary-role selection are +computed by ``_template_visible_role`` (skipping the exempt tool flow), and +the assembled output always satisfies the Mistral alternation check. The +existing #52160 / #58753 forced-user guards must keep winning: their forced +shapes (summary-user followed only by exempt messages) are alternation-safe. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from agent.context_compressor import ( + COMPRESSED_SUMMARY_METADATA_KEY, + SUMMARY_PREFIX, + _template_visible_role, +) + + +@pytest.fixture() +def compressor(): + from agent.context_compressor import ContextCompressor + + with patch( + "agent.context_compressor.get_model_context_length", + return_value=100_000, + ): + c = ContextCompressor( + model="test/model", + threshold_percent=0.50, + protect_first_n=3, + protect_last_n=20, + quiet_mode=True, + ) + c.tail_token_budget = 40 + return c + + +def _tool_turns(start: int, n: int, payload: str = "x" * 300) -> list[dict]: + out: list[dict] = [] + for i in range(start, start + n): + out.append( + { + "role": "assistant", + "content": None, + "tool_calls": [ + { + "id": f"c{i}", + "function": {"name": "terminal", "arguments": "{}"}, + } + ], + } + ) + out.append({"role": "tool", "content": payload, "tool_call_id": f"c{i}"}) + return out + + +def _summary_rows(messages: list[dict]) -> list[dict]: + return [ + m + for m in messages + if isinstance(m, dict) and m.get(COMPRESSED_SUMMARY_METADATA_KEY) + ] + + +def _mistral_alternation_ok(messages: list[dict]) -> bool: + """Replay the Mistral template's pre-flight alternation check: count + only user and assistant-without-tool_calls messages; the counted + sequence must go user, assistant, user, assistant, ...""" + expect = "user" + for m in messages: + role = _template_visible_role(m) + if role in (None, "system"): + continue + if role != expect: + return False + expect = "assistant" if expect == "user" else "user" + return True + + +class TestTemplateVisibleRoleHelper: + def test_tool_flow_is_exempt(self): + assert _template_visible_role({"role": "tool", "content": "r"}) is None + assert ( + _template_visible_role( + {"role": "assistant", "content": "", "tool_calls": [{"id": "c"}]} + ) + is None + ) + + def test_counted_roles_pass_through(self): + assert _template_visible_role({"role": "user", "content": "q"}) == "user" + assert ( + _template_visible_role({"role": "assistant", "content": "a"}) + == "assistant" + ) + assert _template_visible_role({"role": "system", "content": "s"}) == "system" + + def test_non_dict_is_exempt(self): + assert _template_visible_role(None) is None + assert _template_visible_role("not a message") is None + + +class TestSummaryRoleAlternatesAgainstVisibleNeighbours: + def test_captured_devstral_shape_emits_assistant_summary(self, compressor): + """The byte-captured poisoning shape: protected head ends + ``[user, assistant(tool_calls), tool]``, tail is all tool flow. + The literal previous role is ``tool`` (which used to pin the + summary to "user"); the template-visible previous role is + ``user``, so the summary must be emitted as ``assistant``.""" + c = compressor + messages = [{"role": "user", "content": "run a full systems diagnostic"}] + messages += _tool_turns(0, 30) + + mocked = f"{SUMMARY_PREFIX}\nrolled-up summary of the tool work" + with patch.object(c, "_generate_summary", return_value=mocked): + out = c.compress(messages, current_tokens=90_000) + + rows = _summary_rows(out) + assert len(rows) == 1 + assert rows[0].get("role") == "assistant", ( + "REGRESSION: compaction summary emitted as role=user directly " + "after a template-visible user turn (only exempt tool-flow " + "messages between). Mistral-strict templates reject the whole " + "request with a Jinja alternation 500 and the stored session is " + f"poisoned permanently. Got role={rows[0].get('role')!r}." + ) + + def test_captured_shape_passes_mistral_alternation(self, compressor): + c = compressor + messages = [{"role": "user", "content": "run a full systems diagnostic"}] + messages += _tool_turns(0, 30) + + mocked = f"{SUMMARY_PREFIX}\nrolled-up summary of the tool work" + with patch.object(c, "_generate_summary", return_value=mocked): + out = c.compress(messages, current_tokens=90_000) + + assert _mistral_alternation_ok(out), ( + "Compressed transcript fails the Mistral template alternation " + "check. Visible-role sequence: " + f"{[_template_visible_role(m) for m in out if _template_visible_role(m)]}" + ) + + def test_visible_head_assistant_visible_tail_user_merges(self, compressor): + """When the visible head ends ``assistant`` and the visible tail + opens ``user``, NO standalone role can satisfy alternation (user + collides with the tail, assistant with the head -- and the + intervening tool flow is template-exempt, so literal separation + does not help). The existing merge-into-tail fallback must fire. + The literal-role logic used to emit a standalone role="user" + summary here (literal previous role was ``tool``), which is a + second poisoning shape: visible user(summary) -> user(tail).""" + c = compressor + messages = [ + {"role": "user", "content": "question " + "q" * 200}, + {"role": "assistant", "content": "answer " + "a" * 200}, + ] + messages += _tool_turns(0, 30) + # A recent user turn preserved in the tail keeps the zero-user + # guard out of the picture. + messages += [ + {"role": "user", "content": "latest question"}, + {"role": "assistant", "content": "on it"}, + ] + + mocked = f"{SUMMARY_PREFIX}\nsummary body" + with patch.object(c, "_generate_summary", return_value=mocked): + out = c.compress(messages, current_tokens=90_000) + + rows = _summary_rows(out) + assert len(rows) == 1 + # Merged into a template-exempt tail message: invisible to the + # alternation check, summary content still delivered. + assert _template_visible_role(rows[0]) is None + assert _mistral_alternation_ok(out) + + +class TestForcedUserGuardsStillWin: + def test_zero_user_guard_still_forces_user(self, compressor): + """#58753: when no genuine user turn survives, the summary must + still be pinned to role=user (and that shape is alternation-safe + because everything after it is template-exempt).""" + c = compressor + c.compression_count = 1 # protect_first_n decays -> no head + messages = [{"role": "user", "content": "work kanban task 42"}] + messages += _tool_turns(0, 12) + + mocked = f"{SUMMARY_PREFIX}\nsummary body" + with patch.object(c, "_generate_summary", return_value=mocked): + out = c.compress(messages, current_tokens=90_000) + + rows = _summary_rows(out) + assert len(rows) == 1 + assert rows[0].get("role") == "user" + assert _mistral_alternation_ok(out) + + def test_no_literal_consecutive_user_roles(self, compressor): + """The pre-existing literal invariant still holds alongside the + template-visible one.""" + c = compressor + messages = [{"role": "user", "content": "run diagnostics"}] + messages += _tool_turns(0, 30) + + mocked = f"{SUMMARY_PREFIX}\nsummary body" + with patch.object(c, "_generate_summary", return_value=mocked): + out = c.compress(messages, current_tokens=90_000) + + for prev, cur in zip(out, out[1:]): + assert not ( + prev.get("role") == "user" and cur.get("role") == "user" + ), "compression introduced literal consecutive user-role messages"